{"thread":{"id":"50259","subject":"[PATCH 0/1] built-in stash: fix segmentation fault when files were added with intent","startedAt":"2019-01-18T09:50:19Z","lastAt":"2019-01-23T11:38:12Z","messageCount":6,"participants":["Johannes Schindelin via GitGitGadget","Matthew Kraai via GitGitGadget","Junio C Hamano","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"367073","messageId":"pull.110.git.gitgitgadget@gmail.com","threadId":"50259","inReplyTo":null,"subject":"[PATCH 0/1] built-in stash: fix segmentation fault when files were added with intent","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-01-18T09:50:15Z","receivedAt":"2019-01-18T09:50:19Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Git for Windows offered the built-in stash early: in v2.19.0 it was offered\nas an experimental option, and in v2.20.0 it was enabled by default.\n\nOne corner case was identified\n[https://github.com/git-for-windows/git/issues/2006] and fixed\n[https://github.com/git-for-windows/git/pull/2008] in the meantime, and this\ncontribution brings it to the Git mailing list (with a commit message that\nwas \"lightly edited for clarity\", as they say).\n\nThis patch applies on top of ps/stash-in-c.\n\nGranted, it fixes a regression in that patch series, but as Paul is busy\nwith University, I would suggest accepting this bug fix on top, just this\ntime, as if we had stash-in-c already in next.\n\nMatthew Kraai (1):\n stash: fix segmentation fault when files were added with intent\n\n builtin/stash.c | 3 ++-\n t/t3903-stash.sh | 8 ++++++++\n 2 files changed, 10 insertions(+), 1 deletion(-)\n\n\nbase-commit: bec65d5b783ef5ce4c655c26ad8f25c04b001dd1\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-110%2Fdscho%2Fstash-ita-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-110/dscho/stash-ita-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/110\n-- \ngitgitgadget\n"},{"id":"367074","messageId":"b5bbc7793c50991c7cb5a5188f53ccc3a14b23f6.1547805015.git.gitgitgadget@gmail.com","threadId":"50259","inReplyTo":"pull.110.git.gitgitgadget@gmail.com","subject":"[PATCH 1/1] stash: fix segmentation fault when files were added with intent","fromName":"Matthew Kraai via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-01-18T09:50:16Z","receivedAt":"2019-01-18T09:50:20Z","isPatch":true,"sender":{"key":"name:Matthew Kraai","avatar":null},"body":"From: Matthew Kraai \n\nAfter `git add -N `, the index is in a special state. A state for\nwhich the built-in stash was not prepared, as it failed to initialize\nthe `rev` structure in that case before using `&rev.pending`.\n\nDetailed explanation: If `reset_tree()` returns a non-zero value,\n`stash_working_tree()` calls `object_array_clear()` with `&rev.pending`.\nIf `rev` is not initialized, this causes a segmentation fault.\n\nPrevent this by initializing `rev` before calling `reset_tree()`.\n\nThis fixes https://github.com/git-for-windows/git/issues/2006.\n\n[jes: modified the commit message in preparation for sending this patch\nto the Git mailing list.]\n\nSigned-off-by: Matthew Kraai \nSigned-off-by: Johannes Schindelin \n---\n builtin/stash.c | 3 ++-\n t/t3903-stash.sh | 8 ++++++++\n 2 files changed, 10 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/stash.c b/builtin/stash.c\nindex 3ee8a41cda..74e6ff62b5 100644\n--- a/builtin/stash.c\n+++ b/builtin/stash.c\n@@ -1048,6 +1048,8 @@ static int stash_working_tree(struct stash_info *info, struct pathspec ps)\n \tstruct strbuf diff_output = STRBUF_INIT;\n \tstruct index_state istate = { NULL };\n \n+\tinit_revisions(&rev, NULL);\n+\n \tset_alternate_index_output(stash_index_path.buf);\n \tif (reset_tree(&info->i_tree, 0, 0)) {\n \t\tret = -1;\n@@ -1055,7 +1057,6 @@ static int stash_working_tree(struct stash_info *info, struct pathspec ps)\n \t}\n \tset_alternate_index_output(NULL);\n \n-\tinit_revisions(&rev, NULL);\n \trev.prune_data = ps;\n \trev.diffopt.output_format = DIFF_FORMAT_CALLBACK;\n \trev.diffopt.format_callback = add_diff_to_buf;\ndiff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\nindex b67d7a1120..7dfa3a8038 100755\n--- a/t/t3903-stash.sh\n+++ b/t/t3903-stash.sh\n@@ -287,6 +287,14 @@ test_expect_success 'stash an added file' '\n \ttest new = \"$(cat file3)\"\n '\n \n+test_expect_success 'stash --intent-to-add file' '\n+\tgit reset --hard &&\n+\techo new >file4 &&\n+\tgit add --intent-to-add file4 &&\n+\ttest_when_finished \"git rm -f file4\" &&\n+\ttest_must_fail git stash\n+'\n+\n test_expect_success 'stash rm then recreate' '\n \tgit reset --hard &&\n \tgit rm file &&\n-- \ngitgitgadget\n"},{"id":"367140","messageId":"xmqq5zulk88e.fsf@gitster-ct.c.googlers.com","threadId":"50259","inReplyTo":"b5bbc7793c50991c7cb5a5188f53ccc3a14b23f6.1547805015.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/1] stash: fix segmentation fault when files were added with intent","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-01-18T20:42:57Z","receivedAt":"2019-01-18T20:43:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Matthew Kraai via GitGitGadget\" writes:\n\n> From: Matthew Kraai \n>\n> After `git add -N `, the index is in a special state. A state for\n> which the built-in stash was not prepared, as it failed to initialize\n> the `rev` structure in that case before using `&rev.pending`.\n>\n> Detailed explanation: If `reset_tree()` returns a non-zero value,\n> `stash_working_tree()` calls `object_array_clear()` with `&rev.pending`.\n> If `rev` is not initialized, this causes a segmentation fault.\n\nIt is a bit strange that the paragraph for \"detailed explanation\" is\nshorter than the paragraph it attempts to clarify.\n\nDropping those two words \"Detailed explanation:\" easily fixes\nawkwardness ;-).\n\n> +test_expect_success 'stash --intent-to-add file' '\n> +\tgit reset --hard &&\n> +\techo new >file4 &&\n> +\tgit add --intent-to-add file4 &&\n> +\ttest_when_finished \"git rm -f file4\" &&\n> +\ttest_must_fail git stash\n> +'\n\nThis still must fail because an index with an I-T-A cannot be\nincluded in a stash, but test_must_fail will make sure that the\ncommand does not suffer an uncontrolled crash. Good.\n"},{"id":"367240","messageId":"nycvar.QRO.7.76.6.1901211556150.41@tvgsbejvaqbjf.bet","threadId":"50259","inReplyTo":"xmqq5zulk88e.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/1] stash: fix segmentation fault when files were added with intent","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-01-21T15:14:52Z","receivedAt":"2019-01-21T15:15:20Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Fri, 18 Jan 2019, Junio C Hamano wrote:\n\n> \"Matthew Kraai via GitGitGadget\" writes:\n> \n> > From: Matthew Kraai \n> >\n> > After `git add -N `, the index is in a special state. A state for\n> > which the built-in stash was not prepared, as it failed to initialize\n> > the `rev` structure in that case before using `&rev.pending`.\n> >\n> > Detailed explanation: If `reset_tree()` returns a non-zero value,\n> > `stash_working_tree()` calls `object_array_clear()` with `&rev.pending`.\n> > If `rev` is not initialized, this causes a segmentation fault.\n> \n> It is a bit strange that the paragraph for \"detailed explanation\" is\n> shorter than the paragraph it attempts to clarify.\n> \n> Dropping those two words \"Detailed explanation:\" easily fixes\n> awkwardness ;-).\n\nIf you must.\n\nFor me, it is not the quantity, but the quality of the words that makes it\na detailed explanation. You see, as a mathematician, I can give you a very\ncondensed, super detailed, complicated proof for many a lemma which is\nsubstantially shorter than the understandable, English explanation that\nI would want to give to non-mathematicians.\n\nLikewise, if you take a step back, and try to forget for a moment that you\nare very familiar with Git's source code, you will without any doubt\n*have* to admit that the detailed explanation requires a *lot* of\nknowledge already, while the paragraph before that does not.\n\nSo if you find that awkward, I respectfully disagree. And if you insist on\nremoving those \"two words\" (to make the paragraph *even* shorter, which is\napparently something you took exception with), I have no tools to stop\nyou.\n\nJust let it be known that it is against the wish of the author of those\nlines.\n\n> > +test_expect_success 'stash --intent-to-add file' '\n> > +\tgit reset --hard &&\n> > +\techo new >file4 &&\n> > +\tgit add --intent-to-add file4 &&\n> > +\ttest_when_finished \"git rm -f file4\" &&\n> > +\ttest_must_fail git stash\n> > +'\n> \n> This still must fail because an index with an I-T-A cannot be\n> included in a stash, but test_must_fail will make sure that the\n> command does not suffer an uncontrolled crash. Good.\n\nIndeed. And Matthew even adopted your preferred strategy of combining the\ndemonstration of the bug with the fix. In the meantime, I have found the\ntotally intuitive (and equally documented) command line that I can use\nwhen I want to see whether a given branch is buggy and when I cannot\nsimply `git cherry-pick `:\n\n\tgit cherry-pick \n\tgit checkout HEAD^ -- :^/t/\n\nCiao,\nJohannes\n"},{"id":"367338","messageId":"xmqqd0ooh79n.fsf@gitster-ct.c.googlers.com","threadId":"50259","inReplyTo":"nycvar.QRO.7.76.6.1901211556150.41@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 1/1] stash: fix segmentation fault when files were added with intent","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-01-22T18:33:24Z","receivedAt":"2019-01-22T18:33:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> when I want to see whether a given branch is buggy and when I cannot\n> simply `git cherry-pick `:\n>\n> \tgit cherry-pick \n> \tgit checkout HEAD^ -- :^/t/\n\nYup. It is easy to just apply the t/ part to grab the test update\nto see breakage (which I already said when I told you to have a fix\nand test protecting the future breakage of the fix in a single patch\nlong time ago).\n\n"},{"id":"367411","messageId":"nycvar.QRO.7.76.6.1901231236060.45@tvgsbejvaqbjf.bet","threadId":"50259","inReplyTo":"xmqqd0ooh79n.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/1] stash: fix segmentation fault when files were added with intent","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-01-23T11:38:02Z","receivedAt":"2019-01-23T11:38:12Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Tue, 22 Jan 2019, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n> \n> > when I want to see whether a given branch is buggy and when I cannot\n> > simply `git cherry-pick `:\n> >\n> > \tgit cherry-pick \n> > \tgit checkout HEAD^ -- :^/t/\n> \n> Yup. It is easy to just apply the t/ part to grab the test update\n> to see breakage (which I already said when I told you to have a fix\n> and test protecting the future breakage of the fix in a single patch\n> long time ago).\n\nSorry, that was not my point.\n\nMy point is that\n\n\tgit cherry-pick \n\tgit checkout HEAD^ -- :^/t/\n\nis *ridiculously* less intuitive than\n\n\tgit cherry-pick \n\nand I would rather you stop promoting the former over the latter. After\nall, Git's purpose in life is to make things easier and quicker and less\nerror-prone, rathern than slower, more complicated and unintuitive.\n\nAnd I am sure you agree with me on that goal, so I do not understand why\nyou promote that a bit more.\n\nCiao,\nDscho\n"}]}