Skip to content

built-in stash: fix segmentation fault when files were added with intent #110

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 27 commits into from
Closed

built-in stash: fix segmentation fault when files were added with intent #110

wants to merge 27 commits into from

Conversation

dscho
Copy link
Member

@dscho dscho commented Jan 18, 2019 •
edited
Loading

Git for Windows offered the built-in stash early: in v2.19.0 it was offered as an experimental option, and in v2.20.0 it was enabled by default.

One corner case was identified and fixed in the meantime, and this contribution brings it to the Git mailing list (with a commit message that was "lightly edited for clarity", as they say).

This patch applies on top of ps/stash-in-c.

Granted, it fixes a regression in that patch series, but as Paul is busy with University, I would suggest accepting this bug fix on top, just this time, as if we had stash-in-c already in next.

Cc: Paul-Sebastian Ungureanu [email protected]

ungps and others added 27 commits January 4, 2019 14:19
@ungps @gitster
sha1-name.c: add get_oidf() which acts like get_oid()
4e763fa 
Compared to `get_oid()`, `get_oidf()` has as parameters
a pointer to `object_id`, a printf format string and
additional arguments. This will help simplify the code
in subsequent commits.

Original-idea-by: Johannes Schindelin <[email protected]>
Signed-off-by: Paul-Sebastian Ungureanu <[email protected]>
Signed-off-by: Junio C Hamano <[email protected]>
@ungps @gitster
strbuf.c: add strbuf_join_argv()
0c1599c 
Implement `strbuf_join_argv()` to join arguments
into a strbuf.

Signed-off-by: Paul-Sebastian Ungureanu <[email protected]>
Signed-off-by: Junio C Hamano <[email protected]>
@ungps @gitster
strbuf.c: add strbuf_insertf() and strbuf_vinsertf()
bfc3fe3 
Implement `strbuf_insertf()` and `strbuf_vinsertf()` to
insert data using a printf format string.

Original-idea-by: Johannes Schindelin <[email protected]>
Signed-off-by: Paul-Sebastian Ungureanu <[email protected]>
Signed-off-by: Junio C Hamano <[email protected]>
@dscho @gitster
ident: add the ability to provide a "fallback identity"
97f5607 
In 3bc2111 (stash: tolerate missing user identity, 2018-11-18),
`git stash` learned to provide a fallback identity for the case that no
proper name/email was given (and `git stash` does not really care about
a correct identity anyway, but it does want to create a commit object).

In preparation for the same functionality in the upcoming built-in
version of `git stash`, let's offer the same functionality as an API
function.

Signed-off-by: Johannes Schindelin <[email protected]>
Signed-off-by: Junio C Hamano <[email protected]>
@klusark @gitster
stash: improve option parsing test coverage
0654fe7 
In preparation for converting the stash command incrementally to
a builtin command, this patch improves test coverage of the option
parsing. Both for having too many parameters, or too few.

Signed-off-by: Joel Teichroeb <[email protected]>
Signed-off-by: Paul-Sebastian Ungureanu <[email protected]>
Signed-off-by: Junio C Hamano <[email protected]>
@ungps @gitster
t3903: modernize style
caf7bc3 
Remove whitespaces after redirection operators and wrap
long lines.

Signed-off-by: Paul-Sebastian Ungureanu <[email protected]>
Signed-off-by: Junio C Hamano <[email protected]>
@ungps @gitster
stash: rename test cases to be more descriptive
ee77c6a 
Rename some test cases' labels to be more descriptive and under 80
characters per line.

Signed-off-by: Paul-Sebastian Ungureanu <[email protected]>
Signed-off-by: Junio C Hamano <[email protected]>
@ungps @gitster
stash: add tests for git stash show config
cb2152e 
This commit introduces tests for `git stash show`
config. It tests all the cases where `stash.showStat`
and `stash.showPatch` are unset or set to true / false.

Signed-off-by: Paul-Sebastian Ungureanu <[email protected]>
Signed-off-by: Junio C Hamano <[email protected]>
@ungps @gitster
stash: mention options in show synopsis
983084d 
Mention in the documentation, that `show` accepts any
option known to `git diff`.

Signed-off-by: Paul-Sebastian Ungureanu <[email protected]>
Signed-off-by: Junio C Hamano <[email protected]>
@klusark @gitster
stash: convert apply to builtin
f6bbd78 
Add a builtin helper for performing stash commands. Converting
all at once proved hard to review, so starting with just apply
lets conversion get started without the other commands being
finished.

The helper is being implemented as a drop in replacement for
stash so that when it is complete it can simply be renamed and
the shell script deleted.

Delete the contents of the apply_stash shell function and replace
it with a call to stash--helper apply until pop is also
converted.

Signed-off-by: Joel Teichroeb <[email protected]>
Signed-off-by: Paul-Sebastian Ungureanu <[email protected]>
Signed-off-by: Junio C Hamano <[email protected]>
@klusark @gitster
stash: convert drop and clear to builtin
cdca49b 
Add the drop and clear commands to the builtin helper. These two
are each simple, but are being added together as they are quite
related.

We have to unfortunately keep the drop and clear functions in the
shell script as functions are called with parameters internally
that are not valid when the commands are called externally. Once
pop is converted they can both be removed.

Signed-off-by: Joel Teichroeb <[email protected]>
Signed-off-by: Paul-Sebastian Ungureanu <[email protected]>
Signed-off-by: Junio C Hamano <[email protected]>
@klusark @gitster
stash: convert branch to builtin
f596f33 
Add stash branch to the helper and delete the apply_to_branch
function from the shell script.

Checkout does not currently provide a function for checking out
a branch as cmd_checkout does a large amount of sanity checks
first that we require here.

Signed-off-by: Joel Teichroeb <[email protected]>
Signed-off-by: Paul-Sebastian Ungureanu <[email protected]>
Signed-off-by: Junio C Hamano <[email protected]>
@klusark @gitster
stash: convert pop to builtin
e1d0187 
Add stash pop to the helper and delete the pop_stash, drop_stash,
assert_stash_ref functions from the shell script now that they
are no longer needed.

Signed-off-by: Joel Teichroeb <[email protected]>
Signed-off-by: Paul-Sebastian Ungureanu <[email protected]>
Signed-off-by: Junio C Hamano <[email protected]>
@ungps @gitster
stash: convert list to builtin
9b77b07 
Add stash list to the helper and delete the list_stash function
from the shell script.

Signed-off-by: Paul-Sebastian Ungureanu <[email protected]>
Signed-off-by: Junio C Hamano <[email protected]>
@ungps @gitster
stash: convert show to builtin
b4493f2 
Add stash show to the helper and delete the show_stash, have_stash,
assert_stash_like, is_stash_like and parse_flags_and_rev functions
from the shell script now that they are no longer needed.

In shell version, although `git stash show` accepts `--index` and
`--quiet` options, it ignores them. In C, both options are passed
further to `git diff`.

Signed-off-by: Paul-Sebastian Ungureanu <[email protected]>
Signed-off-by: Junio C Hamano <[email protected]>
@ungps @gitster
stash: convert store to builtin
847eb0b 
Add stash store to the helper and delete the store_stash function
from the shell script.

Signed-off-by: Paul-Sebastian Ungureanu <[email protected]>
Signed-off-by: Junio C Hamano <[email protected]>
@ungps @gitster
stash: convert create to builtin
1f5a011 
Add stash create to the helper.

Signed-off-by: Paul-Sebastian Ungureanu <[email protected]>
Signed-off-by: Junio C Hamano <[email protected]>
@ungps @gitster
stash: convert push to builtin
fa38428 
Add stash push to the helper.

Signed-off-by: Paul-Sebastian Ungureanu <[email protected]>
Signed-off-by: Junio C Hamano <[email protected]>
@ungps @gitster
stash: make push -q quiet
9a95010 
There is a change in behaviour with this commit. When there was
no initial commit, the shell version of stash would still display
a message. This commit makes `push` to not display any message if
`--quiet` or `-q` is specified. Add tests for `--quiet`.

Signed-off-by: Paul-Sebastian Ungureanu <[email protected]>
Signed-off-by: Junio C Hamano <[email protected]>
@ungps @gitster
stash: convert save to builtin
cf5b27d 
Add stash save to the helper and delete functions which are no
longer needed (`show_help()`, `save_stash()`, `push_stash()`,
`create_stash()`, `clear_stash()`, `untracked_files()` and
`no_changes()`).

Signed-off-by: Paul-Sebastian Ungureanu <[email protected]>
Signed-off-by: Junio C Hamano <[email protected]>
@ungps @gitster
stash: optimize get_untracked_files() and check_changes()
168e6cf 
This commits introduces a optimization by avoiding calling the
same functions again. For example, `git stash push -u`
would call at some points the following functions:

 * `check_changes()` (inside `do_push_stash()`)
 * `do_create_stash()`, which calls: `check_changes()` and
`get_untracked_files()`

Note that `check_changes()` also calls `get_untracked_files()`.
So, `check_changes()` is called 2 times and `get_untracked_files()`
3 times.

The old function `check_changes()` now consists of two functions:
`get_untracked_files()` and `check_changes_tracked_files()`.

These are the call chains for `push` and `create`:

 * `push_stash()` -> `do_push_stash()` -> `do_create_stash()`

 * `create_stash()` -> `do_create_stash()`

To prevent calling the same functions over and over again,
`check_changes()` inside `do_create_stash()` is now placed
in the caller functions (`create_stash()` and `do_push_stash()`).
This way `check_changes()` and `get_untracked files()` are called
only one time.

https://public-inbox.org/git/[email protected]/

Signed-off-by: Paul-Sebastian Ungureanu <[email protected]>
Signed-off-by: Junio C Hamano <[email protected]>
@ungps @gitster
stash: replace all write-tree child processes with API calls
559edea 
This commit replaces spawning `git write-tree` with API calls.

Signed-off-by: Paul-Sebastian Ungureanu <[email protected]>
Signed-off-by: Junio C Hamano <[email protected]>
@ungps @gitster
stash: convert stash--helper.c into stash.c
51809c7 
The old shell script `git-stash.sh`  was removed and replaced
entirely by `builtin/stash.c`. In order to do that, `create` and
`push` were adapted to work without `stash.sh`. For example, before
this commit, `git stash create` called `git stash--helper create
--message "$*"`. If it called `git stash--helper create "$@"`, then
some of these changes wouldn't have been necessary.

This commit also removes the word `helper` since now stash is
called directly and not by a shell script.

Signed-off-by: Paul-Sebastian Ungureanu <[email protected]>
Signed-off-by: Junio C Hamano <[email protected]>
@dscho @gitster
stash: add back the original, scripted git stash
c907fe1 
This simply copies the version as of sd/stash-wo-user-name verbatim. As
of now, it is not hooked up.

The next commit will change the builtin `stash` to hand off to the
scripted `git stash` when `stash.useBuiltin=false`.

Signed-off-by: Johannes Schindelin <[email protected]>
Signed-off-by: Junio C Hamano <[email protected]>
@dscho @gitster
stash: optionally use the scripted version again
26799a2 
We recently converted the `git stash` command from Unix shell scripts
to builtins.

Let's end users a way out when they discover a bug in the
builtin command: `stash.useBuiltin`.

As the file name `git-stash` is already in use, let's rename the
scripted backend to `git-legacy-stash`.

To make the test suite pass with `stash.useBuiltin=false`, this commit
also backports rudimentary support for `-q` (but only *just* enough
to appease the test suite), and adds a super-ugly hack to force exit
code 129 for `git stash -h`.

Signed-off-by: Johannes Schindelin <[email protected]>
Signed-off-by: Junio C Hamano <[email protected]>
@dscho @gitster
tests: add a special setup where stash.useBuiltin is off
bec65d5 
Add a GIT_TEST_STASH_USE_BUILTIN=false test mode which is equivalent
to running with stash.useBuiltin=false. This is needed to spot that
we're not introducing any regressions in the legacy stash version
while we're carrying both it and the new built-in version.

This imitates the equivalent treatment for the built-in rebase in
62c2393 (tests: add a special setup where rebase.useBuiltin is off,
2018-11-14).

Signed-off-by: Johannes Schindelin <[email protected]>
Signed-off-by: Junio C Hamano <[email protected]>
@kraai @dscho
stash: fix segmentation fault when files were added with intent
b5bbc77 
After `git add -N <file>`, the index is in a special state. A state for
which the built-in stash was not prepared, as it failed to initialize
the `rev` structure in that case before using `&rev.pending`.

Detailed explanation: If `reset_tree()` returns a non-zero value,
`stash_working_tree()` calls `object_array_clear()` with `&rev.pending`.
If `rev` is not initialized, this causes a segmentation fault.

Prevent this by initializing `rev` before calling `reset_tree()`.

This fixes git-for-windows#2006.

[jes: modified the commit message in preparation for sending this patch
to the Git mailing list.]

Signed-off-by: Matthew Kraai <[email protected]>
Signed-off-by: Johannes Schindelin <[email protected]>
@dscho
Copy link
Member Author

dscho commented Jan 18, 2019

/submit

@gitgitgadget
Copy link

gitgitgadget bot commented Jan 18, 2019

Submitted as [email protected]

@gitgitgadget
Copy link

gitgitgadget bot commented Jan 18, 2019

This branch is now known as ps/stash-in-c.

@gitgitgadget
Copy link

gitgitgadget bot commented Jan 31, 2019

This patch series was integrated into pu via git@3196008.

@gitgitgadget
Copy link

gitgitgadget bot commented Feb 1, 2019

This patch series was integrated into pu via git@65680b3.

@gitgitgadget
Copy link

gitgitgadget bot commented Feb 4, 2019

This patch series was integrated into pu via git@df96341.

@gitgitgadget
Copy link

gitgitgadget bot commented Feb 5, 2019

This patch series was integrated into pu via git@f1461ae.

@gitgitgadget
Copy link

gitgitgadget bot commented Feb 6, 2019

This patch series was integrated into pu via git@f985f09.

@gitgitgadget
Copy link

gitgitgadget bot commented Feb 7, 2019

This patch series was integrated into pu via git@6dddc7f.

@gitgitgadget
Copy link

gitgitgadget bot commented Feb 7, 2019

This patch series was integrated into pu via git@4897248.

@gitgitgadget
Copy link

gitgitgadget bot commented Feb 8, 2019

This patch series was integrated into pu via git@b5405ec.

@gitgitgadget
Copy link

gitgitgadget bot commented Feb 8, 2019

This patch series was integrated into pu via git@9d174c5.

@gitgitgadget
Copy link

gitgitgadget bot commented Feb 9, 2019

This patch series was integrated into pu via git@d1a492c.

@gitgitgadget
Copy link

gitgitgadget bot commented Feb 12, 2019

This patch series was integrated into pu via git@14202ee.

@gitgitgadget
Copy link

gitgitgadget bot commented Feb 12, 2019

This patch series was integrated into pu via git@8362e5b.

@gitgitgadget
Copy link

gitgitgadget bot commented Feb 12, 2019

This patch series was integrated into pu via git@e79481a.

@gitgitgadget
Copy link

gitgitgadget bot commented Feb 13, 2019

This patch series was integrated into pu via git@a293102.

@gitgitgadget
Copy link

gitgitgadget bot commented Feb 14, 2019

This patch series was integrated into pu via git@33fc334.

@gitgitgadget
Copy link

gitgitgadget bot commented Feb 14, 2019

This patch series was integrated into pu via git@9bbf1f8.

@gitgitgadget
Copy link

gitgitgadget bot commented Feb 16, 2019

This patch series was integrated into pu via git@359e096.

@gitgitgadget
Copy link

gitgitgadget bot commented Feb 19, 2019

This patch series was integrated into pu via git@4031d00.

@gitgitgadget
Copy link

gitgitgadget bot commented Feb 19, 2019

This patch series was integrated into pu via git@bae1198.

@gitgitgadget
Copy link

gitgitgadget bot commented Feb 19, 2019

This patch series was integrated into pu via git@f048608.

@gitgitgadget
Copy link

gitgitgadget bot commented Feb 22, 2019

This patch series was integrated into pu via git@bfdda4e.

@gitgitgadget
Copy link

gitgitgadget bot commented Feb 22, 2019

This patch series was integrated into pu via git@1ba525c.

@gitgitgadget
Copy link

gitgitgadget bot commented Feb 23, 2019

This patch series was integrated into pu via git@39772e5.

@gitgitgadget
Copy link

gitgitgadget bot commented Feb 24, 2019

This patch series was integrated into pu via git@ac0407f.

@gitgitgadget
Copy link

gitgitgadget bot commented Feb 24, 2019

This patch series was integrated into pu via git@e52eff8.

@dscho dscho force-pushed the ps/stash-in-c branch 2 times, most recently from d911cd6 to 7906af0 Compare March 7, 2019 03:20
@dscho
Copy link
Member Author

dscho commented Apr 29, 2019

This patch was split apart, partially squashed into d4788af, with the regression test added via b6e531e, and all of it made it into master.

@dscho dscho closed this Apr 29, 2019
@dscho dscho deleted the stash-ita branch April 29, 2019 22:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
pu
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@dscho @ungps @klusark @kraai