* [PATCH 0/1] built-in stash: fix segmentation fault when files were added with intent @ 2019-01-18 9:50 Johannes Schindelin via GitGitGadget 2019-01-18 9:50 ` [PATCH 1/1] " Matthew Kraai via GitGitGadget 0 siblings, 1 reply; 6+ messages in thread From: Johannes Schindelin via GitGitGadget @ 2019-01-18 9:50 UTC (permalink / raw) To: git; +Cc: Paul-Sebastian Ungureanu, Junio C Hamano 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 [https://github.com/git-for-windows/git/issues/2006] and fixed [https://github.com/git-for-windows/git/pull/2008] 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. Matthew Kraai (1): stash: fix segmentation fault when files were added with intent builtin/stash.c | 3 ++- t/t3903-stash.sh | 8 ++++++++ 2 files changed, 10 insertions(+), 1 deletion(-) base-commit: bec65d5b783ef5ce4c655c26ad8f25c04b001dd1 Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-110%2Fdscho%2Fstash-ita-v1 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-110/dscho/stash-ita-v1 Pull-Request: https://github.com/gitgitgadget/git/pull/110 -- gitgitgadget ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/1] stash: fix segmentation fault when files were added with intent 2019-01-18 9:50 [PATCH 0/1] built-in stash: fix segmentation fault when files were added with intent Johannes Schindelin via GitGitGadget @ 2019-01-18 9:50 ` Matthew Kraai via GitGitGadget 2019-01-18 20:42 ` Junio C Hamano 0 siblings, 1 reply; 6+ messages in thread From: Matthew Kraai via GitGitGadget @ 2019-01-18 9:50 UTC (permalink / raw) To: git; +Cc: Paul-Sebastian Ungureanu, Junio C Hamano, Matthew Kraai From: Matthew Kraai <mkraai@its.jnj.com> 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 https://github.com/git-for-windows/git/issues/2006. [jes: modified the commit message in preparation for sending this patch to the Git mailing list.] Signed-off-by: Matthew Kraai <mkraai@its.jnj.com> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> --- builtin/stash.c | 3 ++- t/t3903-stash.sh | 8 ++++++++ 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/builtin/stash.c b/builtin/stash.c index 3ee8a41cda..74e6ff62b5 100644 --- a/builtin/stash.c +++ b/builtin/stash.c @@ -1048,6 +1048,8 @@ static int stash_working_tree(struct stash_info *info, struct pathspec ps) struct strbuf diff_output = STRBUF_INIT; struct index_state istate = { NULL }; + init_revisions(&rev, NULL); + set_alternate_index_output(stash_index_path.buf); if (reset_tree(&info->i_tree, 0, 0)) { ret = -1; @@ -1055,7 +1057,6 @@ static int stash_working_tree(struct stash_info *info, struct pathspec ps) } set_alternate_index_output(NULL); - init_revisions(&rev, NULL); rev.prune_data = ps; rev.diffopt.output_format = DIFF_FORMAT_CALLBACK; rev.diffopt.format_callback = add_diff_to_buf; diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh index b67d7a1120..7dfa3a8038 100755 --- a/t/t3903-stash.sh +++ b/t/t3903-stash.sh @@ -287,6 +287,14 @@ test_expect_success 'stash an added file' ' test new = "$(cat file3)" ' +test_expect_success 'stash --intent-to-add file' ' + git reset --hard && + echo new >file4 && + git add --intent-to-add file4 && + test_when_finished "git rm -f file4" && + test_must_fail git stash +' + test_expect_success 'stash rm then recreate' ' git reset --hard && git rm file && -- gitgitgadget ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] stash: fix segmentation fault when files were added with intent 2019-01-18 9:50 ` [PATCH 1/1] " Matthew Kraai via GitGitGadget @ 2019-01-18 20:42 ` Junio C Hamano 2019-01-21 15:14 ` Johannes Schindelin 0 siblings, 1 reply; 6+ messages in thread From: Junio C Hamano @ 2019-01-18 20:42 UTC (permalink / raw) To: Matthew Kraai via GitGitGadget Cc: git, Paul-Sebastian Ungureanu, Matthew Kraai "Matthew Kraai via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: Matthew Kraai <mkraai@its.jnj.com> > > 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. It is a bit strange that the paragraph for "detailed explanation" is shorter than the paragraph it attempts to clarify. Dropping those two words "Detailed explanation:" easily fixes awkwardness ;-). > +test_expect_success 'stash --intent-to-add file' ' > + git reset --hard && > + echo new >file4 && > + git add --intent-to-add file4 && > + test_when_finished "git rm -f file4" && > + test_must_fail git stash > +' This still must fail because an index with an I-T-A cannot be included in a stash, but test_must_fail will make sure that the command does not suffer an uncontrolled crash. Good. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] stash: fix segmentation fault when files were added with intent 2019-01-18 20:42 ` Junio C Hamano @ 2019-01-21 15:14 ` Johannes Schindelin 2019-01-22 18:33 ` Junio C Hamano 0 siblings, 1 reply; 6+ messages in thread From: Johannes Schindelin @ 2019-01-21 15:14 UTC (permalink / raw) To: Junio C Hamano Cc: Matthew Kraai via GitGitGadget, git, Paul-Sebastian Ungureanu, Matthew Kraai Hi Junio, On Fri, 18 Jan 2019, Junio C Hamano wrote: > "Matthew Kraai via GitGitGadget" <gitgitgadget@gmail.com> writes: > > > From: Matthew Kraai <mkraai@its.jnj.com> > > > > 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. > > It is a bit strange that the paragraph for "detailed explanation" is > shorter than the paragraph it attempts to clarify. > > Dropping those two words "Detailed explanation:" easily fixes > awkwardness ;-). If you must. For me, it is not the quantity, but the quality of the words that makes it a detailed explanation. You see, as a mathematician, I can give you a very condensed, super detailed, complicated proof for many a lemma which is substantially shorter than the understandable, English explanation that I would want to give to non-mathematicians. Likewise, if you take a step back, and try to forget for a moment that you are very familiar with Git's source code, you will without any doubt *have* to admit that the detailed explanation requires a *lot* of knowledge already, while the paragraph before that does not. So if you find that awkward, I respectfully disagree. And if you insist on removing those "two words" (to make the paragraph *even* shorter, which is apparently something you took exception with), I have no tools to stop you. Just let it be known that it is against the wish of the author of those lines. > > +test_expect_success 'stash --intent-to-add file' ' > > + git reset --hard && > > + echo new >file4 && > > + git add --intent-to-add file4 && > > + test_when_finished "git rm -f file4" && > > + test_must_fail git stash > > +' > > This still must fail because an index with an I-T-A cannot be > included in a stash, but test_must_fail will make sure that the > command does not suffer an uncontrolled crash. Good. Indeed. And Matthew even adopted your preferred strategy of combining the demonstration of the bug with the fix. In the meantime, I have found the totally intuitive (and equally documented) command line that I can use when I want to see whether a given branch is buggy and when I cannot simply `git cherry-pick <commit-demonstrating-a-bug>`: git cherry-pick <commit-fixing-the-bug-and-adding-a-test> git checkout HEAD^ -- :^/t/ Ciao, Johannes ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] stash: fix segmentation fault when files were added with intent 2019-01-21 15:14 ` Johannes Schindelin @ 2019-01-22 18:33 ` Junio C Hamano 2019-01-23 11:38 ` Johannes Schindelin 0 siblings, 1 reply; 6+ messages in thread From: Junio C Hamano @ 2019-01-22 18:33 UTC (permalink / raw) To: Johannes Schindelin Cc: Matthew Kraai via GitGitGadget, git, Paul-Sebastian Ungureanu, Matthew Kraai Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > when I want to see whether a given branch is buggy and when I cannot > simply `git cherry-pick <commit-demonstrating-a-bug>`: > > git cherry-pick <commit-fixing-the-bug-and-adding-a-test> > git checkout HEAD^ -- :^/t/ Yup. It is easy to just apply the t/ part to grab the test update to see breakage (which I already said when I told you to have a fix and test protecting the future breakage of the fix in a single patch long time ago). ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] stash: fix segmentation fault when files were added with intent 2019-01-22 18:33 ` Junio C Hamano @ 2019-01-23 11:38 ` Johannes Schindelin 0 siblings, 0 replies; 6+ messages in thread From: Johannes Schindelin @ 2019-01-23 11:38 UTC (permalink / raw) To: Junio C Hamano Cc: Matthew Kraai via GitGitGadget, git, Paul-Sebastian Ungureanu, Matthew Kraai Hi Junio, On Tue, 22 Jan 2019, Junio C Hamano wrote: > Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > > > when I want to see whether a given branch is buggy and when I cannot > > simply `git cherry-pick <commit-demonstrating-a-bug>`: > > > > git cherry-pick <commit-fixing-the-bug-and-adding-a-test> > > git checkout HEAD^ -- :^/t/ > > Yup. It is easy to just apply the t/ part to grab the test update > to see breakage (which I already said when I told you to have a fix > and test protecting the future breakage of the fix in a single patch > long time ago). Sorry, that was not my point. My point is that git cherry-pick <commit-fixing-the-bug-and-adding-a-test> git checkout HEAD^ -- :^/t/ is *ridiculously* less intuitive than git cherry-pick <commit-demonstrating-a-bug> and I would rather you stop promoting the former over the latter. After all, Git's purpose in life is to make things easier and quicker and less error-prone, rathern than slower, more complicated and unintuitive. And I am sure you agree with me on that goal, so I do not understand why you promote that a bit more. Ciao, Dscho ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-01-23 11:38 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-01-18 9:50 [PATCH 0/1] built-in stash: fix segmentation fault when files were added with intent Johannes Schindelin via GitGitGadget 2019-01-18 9:50 ` [PATCH 1/1] " Matthew Kraai via GitGitGadget 2019-01-18 20:42 ` Junio C Hamano 2019-01-21 15:14 ` Johannes Schindelin 2019-01-22 18:33 ` Junio C Hamano 2019-01-23 11:38 ` Johannes Schindelin
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).