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

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

From: Matthew Kraai <[email protected]>

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.]

---
 builtin/stash.c  | 3 ++-
 t/t3903-stash.sh | 8 ++++++++
 2 files changed, 10 insertions(+), 1 deletion(-)

index 3ee8a41cda..74e6ff62b5 100644
@@ -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;

index b67d7a1120..7dfa3a8038 100755
@@ -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

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 ;-).

Hi Junio,

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.

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

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).

Hi Junio,

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