{"thread":{"id":"52210","subject":"[BUG] git stash pop --quiet deletes files in git 2.24.0","startedAt":"2019-11-07T10:36:18Z","lastAt":"2019-11-14T02:07:37Z","messageCount":13,"participants":["Grzegorz Rajchman","Thomas Gummerer","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"385694","messageId":"CAMcnqp22tEFva4vYHYLzY83JqDHGzDbDGoUod21Dhtnvv=h_Pg@mail.gmail.com","threadId":"52210","inReplyTo":null,"subject":"[BUG] git stash pop --quiet deletes files in git 2.24.0","fromName":"Grzegorz Rajchman","fromEmail":"rayman17@gmail.com","sentAt":"2019-11-07T10:36:05Z","receivedAt":"2019-11-07T10:36:18Z","isPatch":false,"sender":{"key":"rayman17@gmail.com","avatar":null},"body":"Hi, this is the first time I report an issue in git so I hope I'm\ndoing it right.\n\nI have experienced some unexpected behaviour with git stash pop\n--quiet in git 2.24.0. I use stash in a pre-commit hook script. In it,\nI stash non-staged changes to keep the working directory clean while\nrunning some linters, then I restore the stash by running pop, but\nafter the recent git update I noticed that it stages all previously\nchecked files as deleted.\n\nSteps to reproduce:\n\n  mkdir test-git-stash\n  cd test-git-stash/\n  git init\n  echo foo > foo.txt\n  git add . && git commit -m 'init'\n  echo bar > foo.txt\n  git stash save --quiet --include-untracked --keep-index\n  git stash pop --quiet\n  git status\n\nThis will unexpectedly output:\n\n  On branch master\n  Changes to be committed:\n    (use \"git restore --staged <file>...\" to unstage)\n      deleted:    foo.txt\n\n  Untracked files:\n    (use \"git add <file>...\" to include in what will be committed)\n      foo.txt\n\nNotice that foo.txt was staged as deleted whilst still being present\non the disk.\n\nHowever, if I remove --quiet flag from stash pop:\n\n  git restore --staged foo.txt\n  git stash save --quiet --include-untracked --keep-index\n  git stash pop\n  git status\n\nThen it works as expected. It used to work as expected in git prior to 2.24.0\n\nMy OS is Ubuntu 19.04.\n"},{"id":"385714","messageId":"20191107184912.GA3115@cat","threadId":"52210","inReplyTo":"CAMcnqp22tEFva4vYHYLzY83JqDHGzDbDGoUod21Dhtnvv=h_Pg@mail.gmail.com","subject":"Re: [BUG] git stash pop --quiet deletes files in git 2.24.0","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2019-11-07T18:49:12Z","receivedAt":"2019-11-07T18:49:19Z","isPatch":false,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"On 11/07, Grzegorz Rajchman wrote:\n> Hi, this is the first time I report an issue in git so I hope I'm\n> doing it right.\n\nThanks for the report.  You are indeed doing this right, and the\nincluded reproduction is very helpful.\n\nI broke this in 34933d0eff (\"stash: make sure to write refreshed\ncache\", 2019-09-11), which wasn't caught by the tests, nor by me as I\ndon't use the --quiet flag normally.\n\nBelow is a fix for this, but I want to understand the problem a bit\nbetter and write some tests before sending a patch.\n\nindex ab30d1e920..2dd9c9bbcd 100644\n--- a/builtin/stash.c\n+++ b/builtin/stash.c\n@@ -473,22 +473,20 @@ static int do_apply_stash(const char *prefix, struct stash_info *info,\n \n                if (reset_tree(&c_tree, 0, 1)) {\n                        strbuf_release(&out);\n                        return -1;\n                }\n \n                ret = update_index(&out);\n                strbuf_release(&out);\n                if (ret)\n                        return -1;\n-\n-               discard_cache();\n        }\n \n        if (quiet) {\n                if (refresh_and_write_cache(REFRESH_QUIET, 0, 0))\n                        warning(\"could not refresh index\");\n        } else {\n                struct child_process cp = CHILD_PROCESS_INIT;\n \n                /*\n                 * Status is quite simple and could be replaced with calls to\n\n\n> I have experienced some unexpected behaviour with git stash pop\n> --quiet in git 2.24.0. I use stash in a pre-commit hook script. In it,\n> I stash non-staged changes to keep the working directory clean while\n> running some linters, then I restore the stash by running pop, but\n> after the recent git update I noticed that it stages all previously\n> checked files as deleted.\n> \n> Steps to reproduce:\n> \n>   mkdir test-git-stash\n>   cd test-git-stash/\n>   git init\n>   echo foo > foo.txt\n>   git add . && git commit -m 'init'\n>   echo bar > foo.txt\n>   git stash save --quiet --include-untracked --keep-index\n>   git stash pop --quiet\n>   git status\n> \n> This will unexpectedly output:\n> \n>   On branch master\n>   Changes to be committed:\n>     (use \"git restore --staged <file>...\" to unstage)\n>       deleted:    foo.txt\n> \n>   Untracked files:\n>     (use \"git add <file>...\" to include in what will be committed)\n>       foo.txt\n> \n> Notice that foo.txt was staged as deleted whilst still being present\n> on the disk.\n> \n> However, if I remove --quiet flag from stash pop:\n> \n>   git restore --staged foo.txt\n>   git stash save --quiet --include-untracked --keep-index\n>   git stash pop\n>   git status\n> \n> Then it works as expected. It used to work as expected in git prior to 2.24.0\n> \n> My OS is Ubuntu 19.04.\n"},{"id":"385745","messageId":"xmqq7e4bp06l.fsf@gitster-ct.c.googlers.com","threadId":"52210","inReplyTo":"20191107184912.GA3115@cat","subject":"Re: [BUG] git stash pop --quiet deletes files in git 2.24.0","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-11-08T02:32:50Z","receivedAt":"2019-11-08T02:32:59Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Gummerer <t.gummerer@gmail.com> writes:\n\n> On 11/07, Grzegorz Rajchman wrote:\n>> Hi, this is the first time I report an issue in git so I hope I'm\n>> doing it right.\n>\n> Thanks for the report.  You are indeed doing this right, and the\n> included reproduction is very helpful.\n>\n> I broke this in 34933d0eff (\"stash: make sure to write refreshed\n> cache\", 2019-09-11), which wasn't caught by the tests, nor by me as I\n> don't use the --quiet flag normally.\n>\n> Below is a fix for this, but I want to understand the problem a bit\n> better and write some tests before sending a patch.\n\nOK, thanks for quickly looking into this.\n\nThe commit added two places where refresh_and_write_cache() gets\ncalled.\n\nThe first one at the very beginning of do_apply_stash() used to be\nrefresh_cache() that immediately follows read_cache_preload().  We\nare writing back exactly what we read from the filesystem [*], so\nthis should be a no-op from the correctness POV, with benefit of\nhaving a refreshed cache on disk.\n\n\tSide note.  This argument assumes that no caller has called\n\tread_cache() before calling us and did its own in-core index\n\toperation.  In such a case, the in-core index is already out\n\tof sync with the on-disk one due to our own operation, and\n\tread_cache() will not overwrite already initilized in-core\n\tindex, so we will write out what the original code did not\n\twant to, which would be a bug.\n\nThe second one happens after we do all the 3-way merges to replay\nthe change between the base commit and the working tree state\nrecorded in the stash, and then adjust the index to the desired\nstate:\n\n - If we are propagating the change to the index recorded in the\n   stash to the current index, reset_tree() reads the index_tree\n   that has been computed earlier in the function to update the\n   in-core index and the on-disk index.\n\n - Otherwise, we compute paths added between the base commit and the\n   working tree state recorded in the stash (i.e. those that were\n   created but not yet commited when the stash was made), go back to\n   the in-core index state we had upon entry to this function\n   (i.e. c_tree), and then add these new paths from the working tree\n   directly to the on-disk index without updating the in-core\n   index.  Notice that this leaves the in-core index stale wrt the\n   on-disk index---but the stale in-core index gets discarded.\n\nThen the code goes on to do:\n\n - under --quiet, refresh_cache() used to be called to silently\n   refresh the in-core index.  34933d0eff made it to also write the\n   in-core index to on-disk index.  OOPS.  The in-core index has\n   been discarded at this point.\n\n - otherwise, \"git status\" is spawned and directly acted on the\n   on-disk index (this also has a side effect of writing a refreshed\n   on-disk index).\n\nSo, I do not think removing that discard_cache() alone solves the\nbreakage exposed by 34933d0eff.  Discarding and re-reading the\non-disk index there would restore correctness, but then you would\nwant to make sure that we are not wasting the overall cost for the\nI/O and refreshing.\n\nI think the safer immediate short-term fix is to revert the change\nto the quiet codepath and let it only refresh the in-core index.\n\n> index ab30d1e920..2dd9c9bbcd 100644\n> --- a/builtin/stash.c\n> +++ b/builtin/stash.c\n> @@ -473,22 +473,20 @@ static int do_apply_stash(const char *prefix, struct stash_info *info,\n>  \n>                 if (reset_tree(&c_tree, 0, 1)) {\n>                         strbuf_release(&out);\n>                         return -1;\n>                 }\n>  \n>                 ret = update_index(&out);\n>                 strbuf_release(&out);\n>                 if (ret)\n>                         return -1;\n> -\n> -               discard_cache();\n>         }\n>  \n>         if (quiet) {\n>                 if (refresh_and_write_cache(REFRESH_QUIET, 0, 0))\n>                         warning(\"could not refresh index\");\n>         } else {\n>                 struct child_process cp = CHILD_PROCESS_INIT;\n"},{"id":"385793","messageId":"20191108165929.GB3115@cat","threadId":"52210","inReplyTo":"xmqq7e4bp06l.fsf@gitster-ct.c.googlers.com","subject":"Re: [BUG] git stash pop --quiet deletes files in git 2.24.0","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2019-11-08T16:59:29Z","receivedAt":"2019-11-08T16:59:34Z","isPatch":false,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"On 11/08, Junio C Hamano wrote:\n> Thomas Gummerer <t.gummerer@gmail.com> writes:\n> \n> > On 11/07, Grzegorz Rajchman wrote:\n> >> Hi, this is the first time I report an issue in git so I hope I'm\n> >> doing it right.\n> >\n> > Thanks for the report.  You are indeed doing this right, and the\n> > included reproduction is very helpful.\n> >\n> > I broke this in 34933d0eff (\"stash: make sure to write refreshed\n> > cache\", 2019-09-11), which wasn't caught by the tests, nor by me as I\n> > don't use the --quiet flag normally.\n> >\n> > Below is a fix for this, but I want to understand the problem a bit\n> > better and write some tests before sending a patch.\n> \n> OK, thanks for quickly looking into this.\n> \n> The commit added two places where refresh_and_write_cache() gets\n> called.\n> \n> The first one at the very beginning of do_apply_stash() used to be\n> refresh_cache() that immediately follows read_cache_preload().  We\n> are writing back exactly what we read from the filesystem [*], so\n> this should be a no-op from the correctness POV, with benefit of\n> having a refreshed cache on disk.\n> \n> \tSide note.  This argument assumes that no caller has called\n> \tread_cache() before calling us and did its own in-core index\n> \toperation.  In such a case, the in-core index is already out\n> \tof sync with the on-disk one due to our own operation, and\n> \tread_cache() will not overwrite already initilized in-core\n> \tindex, so we will write out what the original code did not\n> \twant to, which would be a bug.\n> \n> The second one happens after we do all the 3-way merges to replay\n> the change between the base commit and the working tree state\n> recorded in the stash, and then adjust the index to the desired\n> state:\n> \n>  - If we are propagating the change to the index recorded in the\n>    stash to the current index, reset_tree() reads the index_tree\n>    that has been computed earlier in the function to update the\n>    in-core index and the on-disk index.\n> \n>  - Otherwise, we compute paths added between the base commit and the\n>    working tree state recorded in the stash (i.e. those that were\n>    created but not yet commited when the stash was made), go back to\n>    the in-core index state we had upon entry to this function\n>    (i.e. c_tree), and then add these new paths from the working tree\n>    directly to the on-disk index without updating the in-core\n>    index.  Notice that this leaves the in-core index stale wrt the\n>    on-disk index---but the stale in-core index gets discarded.\n> \n> Then the code goes on to do:\n> \n>  - under --quiet, refresh_cache() used to be called to silently\n>    refresh the in-core index.  34933d0eff made it to also write the\n>    in-core index to on-disk index.  OOPS.  The in-core index has\n>    been discarded at this point.\n\nYup, this is certainly my bad, we shouldn't be writing the discarded\nindex of course.  I don't think what we were doing here before was\ncorrect either though.  The only thing that would be called after this\nis 'do_stash_drop()', which only executes external commands.\n\nI think the original intention here was to replace\n'git status >/dev/null 2>&1' from the shell script, which as you note\nbelow did refresh the index.\n\nFrom what you are saying above, and from my testing I think this\nrefresh is actually unnecessary, and we could just remove it outright.\nI'm still trying to think if there could be any way refreshing the\nindex could be useful (before I introduce another bug here\ninadvertently).  If I can't come up with anything I'll send a patch\nwith the corresponding test case removing the 'refresh_cache()'\ncompletely.\n\n>  - otherwise, \"git status\" is spawned and directly acted on the\n>    on-disk index (this also has a side effect of writing a refreshed\n>    on-disk index).\n> \n> So, I do not think removing that discard_cache() alone solves the\n> breakage exposed by 34933d0eff.  Discarding and re-reading the\n> on-disk index there would restore correctness, but then you would\n> want to make sure that we are not wasting the overall cost for the\n> I/O and refreshing.\n> \n> I think the safer immediate short-term fix is to revert the change\n> to the quiet codepath and let it only refresh the in-core index.\n> \n> > index ab30d1e920..2dd9c9bbcd 100644\n> > --- a/builtin/stash.c\n> > +++ b/builtin/stash.c\n> > @@ -473,22 +473,20 @@ static int do_apply_stash(const char *prefix, struct stash_info *info,\n> >  \n> >                 if (reset_tree(&c_tree, 0, 1)) {\n> >                         strbuf_release(&out);\n> >                         return -1;\n> >                 }\n> >  \n> >                 ret = update_index(&out);\n> >                 strbuf_release(&out);\n> >                 if (ret)\n> >                         return -1;\n> > -\n> > -               discard_cache();\n> >         }\n> >  \n> >         if (quiet) {\n> >                 if (refresh_and_write_cache(REFRESH_QUIET, 0, 0))\n> >                         warning(\"could not refresh index\");\n> >         } else {\n> >                 struct child_process cp = CHILD_PROCESS_INIT;\n"},{"id":"385838","messageId":"xmqqk188l0pn.fsf@gitster-ct.c.googlers.com","threadId":"52210","inReplyTo":"20191108165929.GB3115@cat","subject":"Re: [BUG] git stash pop --quiet deletes files in git 2.24.0","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-11-10T06:11:48Z","receivedAt":"2019-11-10T06:11:59Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Gummerer <t.gummerer@gmail.com> writes:\n\n> On 11/08, Junio C Hamano wrote:\n>> So, I do not think removing that discard_cache() alone solves the\n>> breakage exposed by 34933d0eff.  Discarding and re-reading the\n>> on-disk index there would restore correctness, but then you would\n>> want to make sure that we are not wasting the overall cost for the\n>> I/O and refreshing.\n>> \n>> I think the safer immediate short-term fix is to revert the change\n>> to the quiet codepath and let it only refresh the in-core index.\n>\n> Yup, this is certainly my bad, we shouldn't be writing the discarded\n> index of course.  I don't think what we were doing here before was\n> correct either though.  The only thing that would be called after this\n> is 'do_stash_drop()', which only executes external commands.\n\nRight.  Removing discard alone would not be a correct fix exactly\nfor that reason: the in-core index was stale wrt the on-disk index.\n\nIf the program later used in-core index for further processing\n(which is not, and that is why the short-term solution of reverting\nthat hunk would work), we would have been operating on a wrong data.\nSo for the fix that keeps data we have in-core always up-to-date, we\nshould be re-reading from the on-disk index there after discard().\n\nAnd in the longer term, it would likely be the right direction, as\nthe \"git status\" invocation on the non-quiet codepath would want to\nbecome an in-core direct calls into wt-status machinery instead of\nfork+exec eventually.\n\n> From what you are saying above, and from my testing I think this\n> refresh is actually unnecessary, and we could just remove it outright.\n\nPerhaps.  But later it will bite us when somebody wants to rewrite\nthe \"status at the end\" part in C.\n\nBesides, if the original was \"update-index -q --refresh\" in the\nscripted Porcelain after an pop was attempted, it would have shown\nthe unmerged paths as \"needs merge\", wouldn't it?  For that, we need\nto have something (I do not remember if refresh_and_write_cache()\nwould be the in-core API call to do so offhand).\n\nThanks.\n"},{"id":"385925","messageId":"20191111195641.GC3115@cat","threadId":"52210","inReplyTo":"xmqqk188l0pn.fsf@gitster-ct.c.googlers.com","subject":"Re: [BUG] git stash pop --quiet deletes files in git 2.24.0","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2019-11-11T19:56:41Z","receivedAt":"2019-11-11T19:56:46Z","isPatch":false,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"On 11/10, Junio C Hamano wrote:\n> Thomas Gummerer <t.gummerer@gmail.com> writes:\n> \n> > On 11/08, Junio C Hamano wrote:\n> >> So, I do not think removing that discard_cache() alone solves the\n> >> breakage exposed by 34933d0eff.  Discarding and re-reading the\n> >> on-disk index there would restore correctness, but then you would\n> >> want to make sure that we are not wasting the overall cost for the\n> >> I/O and refreshing.\n> >> \n> >> I think the safer immediate short-term fix is to revert the change\n> >> to the quiet codepath and let it only refresh the in-core index.\n> >\n> > Yup, this is certainly my bad, we shouldn't be writing the discarded\n> > index of course.  I don't think what we were doing here before was\n> > correct either though.  The only thing that would be called after this\n> > is 'do_stash_drop()', which only executes external commands.\n> \n> Right.  Removing discard alone would not be a correct fix exactly\n> for that reason: the in-core index was stale wrt the on-disk index.\n> \n> If the program later used in-core index for further processing\n> (which is not, and that is why the short-term solution of reverting\n> that hunk would work), we would have been operating on a wrong data.\n> So for the fix that keeps data we have in-core always up-to-date, we\n> should be re-reading from the on-disk index there after discard().\n> \n> And in the longer term, it would likely be the right direction, as\n> the \"git status\" invocation on the non-quiet codepath would want to\n> become an in-core direct calls into wt-status machinery instead of\n> fork+exec eventually.\n\nRight.  I'd argue that that's even the right direction in the short\nterm.  It does require some more I/O but it also prevents similar\nmistakes.  And I don't think one additional read of the index is going\nto make it or break it for performance here, there are plenty of reads\nalready, and there's probably better ways to speed 'git stash' up.\n\n> > From what you are saying above, and from my testing I think this\n> > refresh is actually unnecessary, and we could just remove it outright.\n> \n> Perhaps.  But later it will bite us when somebody wants to rewrite\n> the \"status at the end\" part in C.\n\nHmm, wouldn't the not re-reading the index part bite us there, rather\nthan the not refreshing the index?\n\nIn the 'has_index' codepath, we write the index to disk, so we already\nhave a fresh one in-core.  This codepath is what used to require\nrefreshing the index afterwards, but no longer does.\n\nPreviously we used to use 'git read-tree \"$unstashed_index_tree\"'\nthere, which does require a 'git update-index -q --refresh'\nafterwards.  However we have replaced that with an internal call to\n'reset_tree', which always sets 'o.merge = 1' for unpack-trees.  Which\nin turn means that the index is already refreshed appropriately iiuc.\n\nIn the other codepath we do 'git update-index --add --stdin', which\nalso doesn't require refreshing the index, but does require the\n'discard_cache()' + 'read_cache()' afterwards, so we're not left in a\nhalf state.\n\n> Besides, if the original was \"update-index -q --refresh\" in the\n> scripted Porcelain after an pop was attempted, it would have shown\n> the unmerged paths as \"needs merge\", wouldn't it?  For that, we need\n> to have something (I do not remember if refresh_and_write_cache()\n> would be the in-core API call to do so offhand).\n\nThe original used 'git status >/dev/null 2>&1' to refresh the index\nafter the 'git read-tree' I mentioned above, but would not show the\n\"needs merge\" message, so I think we're okay on that front.\n\nBelow is the patch that I believe has the least chances of biting us\nin the future, with the appropriate updated tests.  I had considered\nleaving the 'refresh_and_write_cache()' call there, but as I was\nwriting the commit message I had a harder and harder time justifying\nthat, so it's gone now, which I think is the right thing to do.\nLeaving it there would be okay as well, however I don't think it would\nhave any benefit.\n\n--- >8 ---\nSubject: [PATCH] stash: make sure we have a valid index before writing it\n\nIn 'do_apply_stash()' we refresh the index in the end.  Since\n34933d0eff (\"stash: make sure to write refreshed cache\", 2019-09-11),\nwe also write that refreshed index when --quiet is given to 'git stash\napply'.\n\nHowever if '--index' is not given to 'git stash apply', we also\ndiscard the index in the else clause just before.  This leads to\nwriting the discarded index, which means we essentially write an empty\nindex file.  This is obviously not correct, or the behaviour the user\nwanted.  We should not modify the users index without being asked to\ndo so.\n\nMake sure to re-read the index after discarding the current in-core\nindex, to avoid dealing with outdated information.\n\nWe can drop the 'refresh_and_write_cache' completely in the quiet\ncase.  Previously in legacy stash we relied on 'git status' to refresh\nthe index after calling 'git read-tree' when '--index' was passed to\n'git apply'.  However the 'reset_tree()' call that replaced 'git\nread-tree' always passes options that are equivalent to '-m', making\nthe refresh of the index unnecessary.\n\nWe could also drop the 'discard_cache()' + 'read_cache()', however\nthat would make it easy to fall into the same trap as 34933d0eff did,\nso it's better to avoid that.\n\nSigned-off-by: Thomas Gummerer <t.gummerer@gmail.com>\n---\n builtin/stash.c  | 6 ++----\n t/t3903-stash.sh | 5 ++++-\n 2 files changed, 6 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/stash.c b/builtin/stash.c\nindex ab30d1e920..d00567285f 100644\n--- a/builtin/stash.c\n+++ b/builtin/stash.c\n@@ -482,12 +482,10 @@ static int do_apply_stash(const char *prefix, struct stash_info *info,\n \t\t\treturn -1;\n \n \t\tdiscard_cache();\n+\t\tread_cache();\n \t}\n \n-\tif (quiet) {\n-\t\tif (refresh_and_write_cache(REFRESH_QUIET, 0, 0))\n-\t\t\twarning(\"could not refresh index\");\n-\t} else {\n+\tif (!quiet) {\n \t\tstruct child_process cp = CHILD_PROCESS_INIT;\n \n \t\t/*\ndiff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\nindex 392954d6dd..b1c973e3d9 100755\n--- a/t/t3903-stash.sh\n+++ b/t/t3903-stash.sh\n@@ -232,8 +232,9 @@ test_expect_success 'save -q is quiet' '\n \ttest_must_be_empty output.out\n '\n \n-test_expect_success 'pop -q is quiet' '\n+test_expect_success 'pop -q works and is quiet' '\n \tgit stash pop -q >output.out 2>&1 &&\n+\ttest bar = \"$(git show :file)\" &&\n \ttest_must_be_empty output.out\n '\n \n@@ -242,6 +243,8 @@ test_expect_success 'pop -q --index works and is quiet' '\n \tgit add file &&\n \tgit stash save --quiet &&\n \tgit stash pop -q --index >output.out 2>&1 &&\n+\tgit diff-files file2 >file2.diff &&\n+\ttest_must_be_empty file2.diff &&\n \ttest foo = \"$(git show :file)\" &&\n \ttest_must_be_empty output.out\n '\n-- \n2.24.0.155.gd9f6f3b619\n\n"},{"id":"385981","messageId":"xmqqftitfz5u.fsf@gitster-ct.c.googlers.com","threadId":"52210","inReplyTo":"20191111195641.GC3115@cat","subject":"Re: [BUG] git stash pop --quiet deletes files in git 2.24.0","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-11-12T05:21:01Z","receivedAt":"2019-11-12T05:21:12Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Gummerer <t.gummerer@gmail.com> writes:\n\n>> > From what you are saying above, and from my testing I think this\n>> > refresh is actually unnecessary, and we could just remove it outright.\n>> \n>> Perhaps.  But later it will bite us when somebody wants to rewrite\n>> the \"status at the end\" part in C.\n>\n> Hmm, wouldn't the not re-reading the index part bite us there, rather\n> than the not refreshing the index?\n\nYes.  Just removing the refresh-and-write that caused us to write\nout incorrect data would \"fix\" the bug, while leaving the bug of not\nre-reading to bite us later.\n\n> Below is the patch that I believe has the least chances of biting us\n> in the future, with the appropriate updated tests.  I had considered\n> leaving the 'refresh_and_write_cache()' call there, but as I was\n> writing the commit message I had a harder and harder time justifying\n> that, so it's gone now, which I think is the right thing to do.\n> Leaving it there would be okay as well, however I don't think it would\n> have any benefit.\n>\n> --- >8 ---\n> Subject: [PATCH] stash: make sure we have a valid index before writing it\n>\n> In 'do_apply_stash()' we refresh the index in the end.  Since\n> 34933d0eff (\"stash: make sure to write refreshed cache\", 2019-09-11),\n> we also write that refreshed index when --quiet is given to 'git stash\n> apply'.\n>\n> However if '--index' is not given to 'git stash apply', we also\n> discard the index in the else clause just before.  This leads to\n> writing the discarded index, which means we essentially write an empty\n> index file.  This is obviously not correct, or the behaviour the user\n> wanted.  We should not modify the users index without being asked to\n> do so.\n>\n> Make sure to re-read the index after discarding the current in-core\n> index, to avoid dealing with outdated information.\n\nYup.  The \"!has_index\" codepath calls update_index() that turns the\non-disk index into the desired shape (would it help explaining that\nin the previous paragraph, by the way?) so all we need to do is to\nread it back into core.  Makes sense.\n\n> We can drop the 'refresh_and_write_cache' completely in the quiet\n> case.  Previously in legacy stash we relied on 'git status' to refresh\n> the index after calling 'git read-tree' when '--index' was passed to\n> 'git apply'.  However the 'reset_tree()' call that replaced 'git\n> read-tree' always passes options that are equivalent to '-m', making\n> the refresh of the index unnecessary.\n\nOK.\n\n> We could also drop the 'discard_cache()' + 'read_cache()', however\n> that would make it easy to fall into the same trap as 34933d0eff did,\n> so it's better to avoid that.\n\nThis is the discarded alternative of the main fix we saw earlier.\nPerhaps it may make the flow of thought easier to follow if we moved\nit up before talking about \"refresh-and-write can be thrown away\"?\n\n> Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>\n> ---\n>  builtin/stash.c  | 6 ++----\n>  t/t3903-stash.sh | 5 ++++-\n>  2 files changed, 6 insertions(+), 5 deletions(-)\n>\n> diff --git a/builtin/stash.c b/builtin/stash.c\n> index ab30d1e920..d00567285f 100644\n> --- a/builtin/stash.c\n> +++ b/builtin/stash.c\n> @@ -482,12 +482,10 @@ static int do_apply_stash(const char *prefix, struct stash_info *info,\n>  \t\t\treturn -1;\n>  \n>  \t\tdiscard_cache();\n> +\t\tread_cache();\n\nA comment\n\n    /* read back the result of update_index() back from the disk */\n\nbefore discard_cache() may be warranted?\n\n>  \t}\n>  \n> -\tif (quiet) {\n> -\t\tif (refresh_and_write_cache(REFRESH_QUIET, 0, 0))\n> -\t\t\twarning(\"could not refresh index\");\n> -\t} else {\n\nOK.\n\n> +\tif (!quiet) {\n>  \t\tstruct child_process cp = CHILD_PROCESS_INIT;\n>  \n>  \t\t/*\n> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\n> index 392954d6dd..b1c973e3d9 100755\n> --- a/t/t3903-stash.sh\n> +++ b/t/t3903-stash.sh\n> @@ -232,8 +232,9 @@ test_expect_success 'save -q is quiet' '\n>  \ttest_must_be_empty output.out\n>  '\n>  \n> -test_expect_success 'pop -q is quiet' '\n> +test_expect_success 'pop -q works and is quiet' '\n>  \tgit stash pop -q >output.out 2>&1 &&\n> +\ttest bar = \"$(git show :file)\" &&\n\nAh, this is to ensure that we didn't lose the \"file\" from the index?\n\nDenton is on the quest of removing \"$(git command substitution)\"\nused in a way that might hide the error from git invocation in a\nseparate thread [*1*].  This may want to become\n\n\tgit rev-parse --verify :file &&\n\nor\n\n\tgit show :file >actual && echo bar >expect &&\n\ttest_cmp expect actual &&\n\nperhaps?\n\n>  \ttest_must_be_empty output.out\n>  '\n>  \n> @@ -242,6 +243,8 @@ test_expect_success 'pop -q --index works and is quiet' '\n>  \tgit add file &&\n>  \tgit stash save --quiet &&\n>  \tgit stash pop -q --index >output.out 2>&1 &&\n> +\tgit diff-files file2 >file2.diff &&\n> +\ttest_must_be_empty file2.diff &&\n>  \ttest foo = \"$(git show :file)\" &&\n>  \ttest_must_be_empty output.out\n>  '\n\nDittto.\n\nThanks.\n\n\n[Reference]\n\n*1* <2f9052fd94ebb6fe93ea6fe2e7cd3c717635c822.1573517561.git.liu.denton@gmail.com>\n\nNote that \"var=$(git subcmd)\" is special and will signal us a failure\nof the git invocation.\n"},{"id":"386105","messageId":"20191113111539.GA3047@cat","threadId":"52210","inReplyTo":"xmqqftitfz5u.fsf@gitster-ct.c.googlers.com","subject":"Re: [BUG] git stash pop --quiet deletes files in git 2.24.0","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2019-11-13T11:15:39Z","receivedAt":"2019-11-13T11:15:45Z","isPatch":false,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"On 11/12, Junio C Hamano wrote:\n> Thomas Gummerer <t.gummerer@gmail.com> writes:\n> \n> >> > From what you are saying above, and from my testing I think this\n> >> > refresh is actually unnecessary, and we could just remove it outright.\n> >> \n> >> Perhaps.  But later it will bite us when somebody wants to rewrite\n> >> the \"status at the end\" part in C.\n> >\n> > Hmm, wouldn't the not re-reading the index part bite us there, rather\n> > than the not refreshing the index?\n> \n> Yes.  Just removing the refresh-and-write that caused us to write\n> out incorrect data would \"fix\" the bug, while leaving the bug of not\n> re-reading to bite us later.\n> \n> > Below is the patch that I believe has the least chances of biting us\n> > in the future, with the appropriate updated tests.  I had considered\n> > leaving the 'refresh_and_write_cache()' call there, but as I was\n> > writing the commit message I had a harder and harder time justifying\n> > that, so it's gone now, which I think is the right thing to do.\n> > Leaving it there would be okay as well, however I don't think it would\n> > have any benefit.\n> >\n> > --- >8 ---\n> > Subject: [PATCH] stash: make sure we have a valid index before writing it\n> >\n> > In 'do_apply_stash()' we refresh the index in the end.  Since\n> > 34933d0eff (\"stash: make sure to write refreshed cache\", 2019-09-11),\n> > we also write that refreshed index when --quiet is given to 'git stash\n> > apply'.\n> >\n> > However if '--index' is not given to 'git stash apply', we also\n> > discard the index in the else clause just before.  This leads to\n> > writing the discarded index, which means we essentially write an empty\n> > index file.  This is obviously not correct, or the behaviour the user\n> > wanted.  We should not modify the users index without being asked to\n> > do so.\n> >\n> > Make sure to re-read the index after discarding the current in-core\n> > index, to avoid dealing with outdated information.\n> \n> Yup.  The \"!has_index\" codepath calls update_index() that turns the\n> on-disk index into the desired shape (would it help explaining that\n> in the previous paragraph, by the way?) so all we need to do is to\n> read it back into core.  Makes sense.\n\nWill add some more explanation about that.\n\n> > We could also drop the 'discard_cache()' + 'read_cache()', however\n> > that would make it easy to fall into the same trap as 34933d0eff did,\n> > so it's better to avoid that.\n> \n> This is the discarded alternative of the main fix we saw earlier.\n> Perhaps it may make the flow of thought easier to follow if we moved\n> it up before talking about \"refresh-and-write can be thrown away\"?\n\nThanks, will move.\n\n> > Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>\n> > ---\n> >  builtin/stash.c  | 6 ++----\n> >  t/t3903-stash.sh | 5 ++++-\n> >  2 files changed, 6 insertions(+), 5 deletions(-)\n> >\n> > diff --git a/builtin/stash.c b/builtin/stash.c\n> > index ab30d1e920..d00567285f 100644\n> > --- a/builtin/stash.c\n> > +++ b/builtin/stash.c\n> > @@ -482,12 +482,10 @@ static int do_apply_stash(const char *prefix, struct stash_info *info,\n> >  \t\t\treturn -1;\n> >  \n> >  \t\tdiscard_cache();\n> > +\t\tread_cache();\n> \n> A comment\n> \n>     /* read back the result of update_index() back from the disk */\n> \n> before discard_cache() may be warranted?\n\nYeah that makes sense, will add.\n\n> >  \t}\n> >  \n> > -\tif (quiet) {\n> > -\t\tif (refresh_and_write_cache(REFRESH_QUIET, 0, 0))\n> > -\t\t\twarning(\"could not refresh index\");\n> > -\t} else {\n> \n> OK.\n> \n> > +\tif (!quiet) {\n> >  \t\tstruct child_process cp = CHILD_PROCESS_INIT;\n> >  \n> >  \t\t/*\n> > diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\n> > index 392954d6dd..b1c973e3d9 100755\n> > --- a/t/t3903-stash.sh\n> > +++ b/t/t3903-stash.sh\n> > @@ -232,8 +232,9 @@ test_expect_success 'save -q is quiet' '\n> >  \ttest_must_be_empty output.out\n> >  '\n> >  \n> > -test_expect_success 'pop -q is quiet' '\n> > +test_expect_success 'pop -q works and is quiet' '\n> >  \tgit stash pop -q >output.out 2>&1 &&\n> > +\ttest bar = \"$(git show :file)\" &&\n> \n> Ah, this is to ensure that we didn't lose the \"file\" from the index?\n> \n> Denton is on the quest of removing \"$(git command substitution)\"\n> used in a way that might hide the error from git invocation in a\n> separate thread [*1*].  This may want to become\n> \n> \tgit rev-parse --verify :file &&\n> \n> or\n> \n> \tgit show :file >actual && echo bar >expect &&\n> \ttest_cmp expect actual &&\n> \n> perhaps?\n\nHmm I just copy-pasted this from somewhere else in this test file.\nI'll add a preparatory patch getting rid of \"$(git command substitution)\"\nas I don't believe Denton got to t3903 yet.\n\nThere's some more opportunities for modernization of this test file,\nbut I refrained from doing that to not blow up this bug fix series too\nmuch.\n\n> >  \ttest_must_be_empty output.out\n> >  '\n> >  \n> > @@ -242,6 +243,8 @@ test_expect_success 'pop -q --index works and is quiet' '\n> >  \tgit add file &&\n> >  \tgit stash save --quiet &&\n> >  \tgit stash pop -q --index >output.out 2>&1 &&\n> > +\tgit diff-files file2 >file2.diff &&\n> > +\ttest_must_be_empty file2.diff &&\n> >  \ttest foo = \"$(git show :file)\" &&\n> >  \ttest_must_be_empty output.out\n> >  '\n> \n> Dittto.\n> \n> Thanks.\n> \n> \n> [Reference]\n> \n> *1* <2f9052fd94ebb6fe93ea6fe2e7cd3c717635c822.1573517561.git.liu.denton@gmail.com>\n> \n> Note that \"var=$(git subcmd)\" is special and will signal us a failure\n> of the git invocation.\n"},{"id":"386106","messageId":"20191113111718.21412-1-t.gummerer@gmail.com","threadId":"52210","inReplyTo":"20191111195641.GC3115@cat","subject":"[PATCH v2 1/2] t3903: avoid git commands inside command substitution","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2019-11-13T11:17:17Z","receivedAt":"2019-11-13T11:17:25Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"Running git commands inside command substitution can hide errors.\nAvoid doing so in t3903.\n\nSigned-off-by: Thomas Gummerer <t.gummerer@gmail.com>\n---\n t/t3903-stash.sh | 99 +++++++++++++++++++++++++++++++++---------------\n 1 file changed, 69 insertions(+), 30 deletions(-)\n\ndiff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\nindex 392954d6dd..db7cc6e664 100755\n--- a/t/t3903-stash.sh\n+++ b/t/t3903-stash.sh\n@@ -34,7 +34,7 @@ index 0cfbf08..00750ed 100644\n EOF\n \n test_expect_success 'parents of stash' '\n-\ttest $(git rev-parse stash^) = $(git rev-parse HEAD) &&\n+\ttest_cmp_rev stash^ HEAD &&\n \tgit diff stash^2..stash >output &&\n \ttest_cmp expect output\n '\n@@ -68,8 +68,11 @@ test_expect_success 'apply stashed changes' '\n \tgit commit -m other-file &&\n \tgit stash apply &&\n \ttest 3 = $(cat file) &&\n-\ttest 1 = $(git show :file) &&\n-\ttest 1 = $(git show HEAD:file)\n+\techo 1 >expect &&\n+\tgit show :file >actual &&\n+\ttest_cmp expect actual &&\n+\tgit show HEAD:file >actual &&\n+\ttest_cmp expect actual\n '\n \n test_expect_success 'apply stashed changes (including index)' '\n@@ -80,8 +83,12 @@ test_expect_success 'apply stashed changes (including index)' '\n \tgit commit -m other-file &&\n \tgit stash apply --index &&\n \ttest 3 = $(cat file) &&\n-\ttest 2 = $(git show :file) &&\n-\ttest 1 = $(git show HEAD:file)\n+\techo 2 >expect &&\n+\tgit show :file >actual &&\n+\ttest_cmp expect actual &&\n+\techo 1 >expect &&\n+\tgit show HEAD:file >actual &&\n+\ttest_cmp expect actual\n '\n \n test_expect_success 'unstashing in a subdirectory' '\n@@ -107,8 +114,11 @@ test_expect_success 'drop top stash' '\n \ttest_cmp expected actual &&\n \tgit stash apply &&\n \ttest 3 = $(cat file) &&\n-\ttest 1 = $(git show :file) &&\n-\ttest 1 = $(git show HEAD:file)\n+\techo 1 >expect &&\n+\tgit show :file >actual &&\n+\ttest_cmp expect actual &&\n+\tgit show HEAD:file >actual &&\n+\ttest_cmp expect actual\n '\n \n test_expect_success 'drop middle stash' '\n@@ -118,17 +128,24 @@ test_expect_success 'drop middle stash' '\n \techo 9 >file &&\n \tgit stash &&\n \tgit stash drop stash@{1} &&\n-\ttest 2 = $(git stash list | wc -l) &&\n+\tgit stash list >output &&\n+\ttest_line_count = 2 output &&\n \tgit stash apply &&\n \ttest 9 = $(cat file) &&\n-\ttest 1 = $(git show :file) &&\n-\ttest 1 = $(git show HEAD:file) &&\n+\techo 1 >expect &&\n+\tgit show :file >actual &&\n+\ttest_cmp expect actual &&\n+\tgit show HEAD:file >actual &&\n+\ttest_cmp expect actual &&\n \tgit reset --hard &&\n \tgit stash drop &&\n \tgit stash apply &&\n \ttest 3 = $(cat file) &&\n-\ttest 1 = $(git show :file) &&\n-\ttest 1 = $(git show HEAD:file)\n+\techo 1 >expect &&\n+\tgit show :file >actual &&\n+\ttest_cmp expect actual &&\n+\tgit show HEAD:file >actual &&\n+\ttest_cmp expect actual\n '\n \n test_expect_success 'drop middle stash by index' '\n@@ -138,26 +155,37 @@ test_expect_success 'drop middle stash by index' '\n \techo 9 >file &&\n \tgit stash &&\n \tgit stash drop 1 &&\n-\ttest 2 = $(git stash list | wc -l) &&\n+\tgit stash list >output &&\n+\ttest_line_count = 2 output &&\n \tgit stash apply &&\n \ttest 9 = $(cat file) &&\n-\ttest 1 = $(git show :file) &&\n-\ttest 1 = $(git show HEAD:file) &&\n+\techo 1 >expect &&\n+\tgit show :file >actual &&\n+\ttest_cmp expect actual &&\n+\tgit show HEAD:file >actual &&\n+\ttest_cmp expect actual &&\n \tgit reset --hard &&\n \tgit stash drop &&\n \tgit stash apply &&\n \ttest 3 = $(cat file) &&\n-\ttest 1 = $(git show :file) &&\n-\ttest 1 = $(git show HEAD:file)\n+\techo 1 >expect &&\n+\tgit show :file >actual &&\n+\ttest_cmp expect actual &&\n+\tgit show HEAD:file >actual &&\n+\ttest_cmp expect actual\n '\n \n test_expect_success 'stash pop' '\n \tgit reset --hard &&\n \tgit stash pop &&\n \ttest 3 = $(cat file) &&\n-\ttest 1 = $(git show :file) &&\n-\ttest 1 = $(git show HEAD:file) &&\n-\ttest 0 = $(git stash list | wc -l)\n+\techo 1 >expect &&\n+\tgit show :file >actual &&\n+\ttest_cmp expect actual &&\n+\tgit show HEAD:file >actual &&\n+\ttest_cmp expect actual &&\n+\tgit stash list >output &&\n+\ttest_must_be_empty output\n '\n \n cat >expect <<EOF\n@@ -207,8 +235,10 @@ test_expect_success 'stash branch' '\n \techo baz >file &&\n \tgit commit file -m second &&\n \tgit stash branch stashbranch &&\n-\ttest refs/heads/stashbranch = $(git symbolic-ref HEAD) &&\n-\ttest $(git rev-parse HEAD) = $(git rev-parse master^) &&\n+\techo \"refs/heads/stashbranch\" >expect3 &&\n+\tgit symbolic-ref HEAD >actual &&\n+\ttest_cmp expect3 actual &&\n+\ttest_cmp_rev HEAD master^ &&\n \tgit diff --cached >output &&\n \ttest_cmp expect output &&\n \tgit diff >output &&\n@@ -217,7 +247,8 @@ test_expect_success 'stash branch' '\n \tgit commit -m alternate\\ second &&\n \tgit diff master..stashbranch >output &&\n \ttest_cmp output expect2 &&\n-\ttest 0 = $(git stash list | wc -l)\n+\tgit stash list >output &&\n+\ttest_must_be_empty output\n '\n \n test_expect_success 'apply -q is quiet' '\n@@ -242,7 +273,9 @@ test_expect_success 'pop -q --index works and is quiet' '\n \tgit add file &&\n \tgit stash save --quiet &&\n \tgit stash pop -q --index >output.out 2>&1 &&\n-\ttest foo = \"$(git show :file)\" &&\n+\techo foo >expect &&\n+\tgit show :file >actual &&\n+\ttest_cmp expect actual &&\n \ttest_must_be_empty output.out\n '\n \n@@ -500,7 +533,8 @@ test_expect_success 'stash branch - no stashes on stack, stash-like argument' '\n \tgit stash branch stash-branch ${STASH_ID} &&\n \ttest_when_finished \"git reset --hard HEAD && git checkout master &&\n \tgit branch -D stash-branch\" &&\n-\ttest $(git ls-files --modified | wc -l) -eq 1\n+\tgit ls-files --modified >output &&\n+\ttest_line_count = 1 output\n '\n \n test_expect_success 'stash branch - stashes on stack, stash-like argument' '\n@@ -516,7 +550,8 @@ test_expect_success 'stash branch - stashes on stack, stash-like argument' '\n \tgit stash branch stash-branch ${STASH_ID} &&\n \ttest_when_finished \"git reset --hard HEAD && git checkout master &&\n \tgit branch -D stash-branch\" &&\n-\ttest $(git ls-files --modified | wc -l) -eq 1\n+\tgit ls-files --modified >actual &&\n+\ttest_line_count = 1 actual\n '\n \n test_expect_success 'stash branch complains with no arguments' '\n@@ -638,7 +673,8 @@ test_expect_success 'drop: fail early if specified stash is not a stash ref' '\n \tgit stash &&\n \techo bar >file &&\n \tgit stash &&\n-\ttest_must_fail git stash drop $(git rev-parse stash@{0}) &&\n+\tstash=$(git rev-parse stash@{0}) &&\n+\ttest_must_fail git stash drop $stash &&\n \tgit stash pop &&\n \ttest bar = \"$(cat file)\" &&\n \tgit reset --hard HEAD\n@@ -652,7 +688,8 @@ test_expect_success 'pop: fail early if specified stash is not a stash ref' '\n \tgit stash &&\n \techo bar >file &&\n \tgit stash &&\n-\ttest_must_fail git stash pop $(git rev-parse stash@{0}) &&\n+\tstash=$(git rev-parse stash@{0}) &&\n+\ttest_must_fail git stash pop $stash &&\n \tgit stash pop &&\n \ttest bar = \"$(cat file)\" &&\n \tgit reset --hard HEAD\n@@ -789,7 +826,7 @@ test_expect_success 'stash where working directory contains \"HEAD\" file' '\n \tgit stash &&\n \tgit diff-files --quiet &&\n \tgit diff-index --cached --quiet HEAD &&\n-\ttest \"$(git rev-parse stash^)\" = \"$(git rev-parse HEAD)\" &&\n+\ttest_cmp_rev stash^ HEAD &&\n \tgit diff stash^..stash >output &&\n \ttest_cmp expect output\n '\n@@ -807,7 +844,9 @@ test_expect_success 'store updates stash ref and reflog' '\n \tgit reset --hard &&\n \ttest_path_is_missing bazzy &&\n \tgit stash store -m quuxery $STASH_ID &&\n-\ttest $(git rev-parse stash) = $STASH_ID &&\n+\techo $STASH_ID >expect &&\n+\tgit rev-parse stash >actual &&\n+\ttest_cmp expect actual &&\n \tgit reflog --format=%H stash| grep $STASH_ID &&\n \tgit stash pop &&\n \tgrep quux bazzy\n-- \n2.24.0.155.gd9f6f3b619\n\n"},{"id":"386107","messageId":"20191113111718.21412-2-t.gummerer@gmail.com","threadId":"52210","inReplyTo":"20191113111718.21412-1-t.gummerer@gmail.com","subject":"[PATCH v2 2/2] stash: make sure we have a valid index before writing it","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2019-11-13T11:17:18Z","receivedAt":"2019-11-13T11:17:26Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"In 'do_apply_stash()' we refresh the index in the end.  Since\n34933d0eff (\"stash: make sure to write refreshed cache\", 2019-09-11),\nwe also write that refreshed index when --quiet is given to 'git stash\napply'.\n\nHowever if '--index' is not given to 'git stash apply', we also\ndiscard the index in the else clause just before.  We need to do so\nbecause we use an external 'git update-index --add --stdin', which\nleads to an out of date in-core index.\n\nLater we call 'refresh_and_write_cache', which now leads to writing\nthe discarded index, which means we essentially write an empty index\nfile.  This is obviously not correct, or the behaviour the user\nwanted.  We should not modify the users index without being asked to\ndo so.\n\nMake sure to re-read the index after discarding the current in-core\nindex, to avoid dealing with outdated information.  Instead we could\nalso drop the 'discard_cache()' + 'read_cache()', however that would\nmake it easy to fall into the same trap as 34933d0eff did, so it's\nbetter to avoid that.\n\nWe can also drop the 'refresh_and_write_cache' completely in the quiet\ncase.  Previously in legacy stash we relied on 'git status' to refresh\nthe index after calling 'git read-tree' when '--index' was passed to\n'git apply'.  However the 'reset_tree()' call that replaced 'git\nread-tree' always passes options that are equivalent to '-m', making\nthe refresh of the index unnecessary.\n\nReported-by: Grzegorz Rajchman <rayman17@gmail.com>\nSigned-off-by: Thomas Gummerer <t.gummerer@gmail.com>\n---\n builtin/stash.c  | 7 +++----\n t/t3903-stash.sh | 7 ++++++-\n 2 files changed, 9 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/stash.c b/builtin/stash.c\nindex ab30d1e920..372fbdb7ac 100644\n--- a/builtin/stash.c\n+++ b/builtin/stash.c\n@@ -481,13 +481,12 @@ static int do_apply_stash(const char *prefix, struct stash_info *info,\n \t\tif (ret)\n \t\t\treturn -1;\n \n+\t\t/* read back the result of update_index() back from the disk */\n \t\tdiscard_cache();\n+\t\tread_cache();\n \t}\n \n-\tif (quiet) {\n-\t\tif (refresh_and_write_cache(REFRESH_QUIET, 0, 0))\n-\t\t\twarning(\"could not refresh index\");\n-\t} else {\n+\tif (!quiet) {\n \t\tstruct child_process cp = CHILD_PROCESS_INIT;\n \n \t\t/*\ndiff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\nindex db7cc6e664..0157771e24 100755\n--- a/t/t3903-stash.sh\n+++ b/t/t3903-stash.sh\n@@ -263,8 +263,11 @@ test_expect_success 'save -q is quiet' '\n \ttest_must_be_empty output.out\n '\n \n-test_expect_success 'pop -q is quiet' '\n+test_expect_success 'pop -q works and is quiet' '\n \tgit stash pop -q >output.out 2>&1 &&\n+\techo bar >expect &&\n+\tgit show :file >actual &&\n+\ttest_cmp expect actual &&\n \ttest_must_be_empty output.out\n '\n \n@@ -273,6 +276,8 @@ test_expect_success 'pop -q --index works and is quiet' '\n \tgit add file &&\n \tgit stash save --quiet &&\n \tgit stash pop -q --index >output.out 2>&1 &&\n+\tgit diff-files file2 >file2.diff &&\n+\ttest_must_be_empty file2.diff &&\n \techo foo >expect &&\n \tgit show :file >actual &&\n \ttest_cmp expect actual &&\n-- \n2.24.0.155.gd9f6f3b619\n\n"},{"id":"386122","messageId":"xmqq4kz7c37i.fsf@gitster-ct.c.googlers.com","threadId":"52210","inReplyTo":"20191113111539.GA3047@cat","subject":"Re: [BUG] git stash pop --quiet deletes files in git 2.24.0","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-11-13T13:31:45Z","receivedAt":"2019-11-13T13:31:50Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Gummerer <t.gummerer@gmail.com> writes:\n\n>> ...  This may want to become\n>> \n>> \tgit rev-parse --verify :file &&\n>> \n>> or\n>> \n>> \tgit show :file >actual && echo bar >expect &&\n>> \ttest_cmp expect actual &&\n>> \n>> perhaps?\n>\n> Hmm I just copy-pasted this from somewhere else in this test file.\n> I'll add a preparatory patch getting rid of \"$(git command substitution)\"\n> as I don't believe Denton got to t3903 yet.\n>\n> There's some more opportunities for modernization of this test file,\n> but I refrained from doing that to not blow up this bug fix series too\n> much.\n\nIt is very much appreciated that you aimed to keep the topic focused\non the fixing.  What I meant was merely to avoid making things worse\nby adding more of $(git command substitution), not cleaning up the\nexisting ones.\n\nThanks.\n"},{"id":"386124","messageId":"20191113150136.GB3047@cat","threadId":"52210","inReplyTo":"xmqq4kz7c37i.fsf@gitster-ct.c.googlers.com","subject":"[PATCH v3] stash: make sure we have a valid index before writing it","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2019-11-13T15:01:36Z","receivedAt":"2019-11-13T15:01:43Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"On 11/13, Junio C Hamano wrote:\n> Thomas Gummerer <t.gummerer@gmail.com> writes:\n> \n> >> ...  This may want to become\n> >> \n> >> \tgit rev-parse --verify :file &&\n> >> \n> >> or\n> >> \n> >> \tgit show :file >actual && echo bar >expect &&\n> >> \ttest_cmp expect actual &&\n> >> \n> >> perhaps?\n> >\n> > Hmm I just copy-pasted this from somewhere else in this test file.\n> > I'll add a preparatory patch getting rid of \"$(git command substitution)\"\n> > as I don't believe Denton got to t3903 yet.\n> >\n> > There's some more opportunities for modernization of this test file,\n> > but I refrained from doing that to not blow up this bug fix series too\n> > much.\n> \n> It is very much appreciated that you aimed to keep the topic focused\n> on the fixing.  What I meant was merely to avoid making things worse\n> by adding more of $(git command substitution), not cleaning up the\n> existing ones.\n\nI misunderstood then because the other case you had pointed out wasn't\nintroduced in my patch, but was just in the context. I have already\nsent the series with the preparatory cleanup.  I'm happy to just go\nwithout the cleanup though.  Since I'm already sending this email,\nI'll just add the patch doing just that below.\n\n--- >8 ---\nSubject: [PATCH v3] stash: make sure we have a valid index before writing it\n\nIn 'do_apply_stash()' we refresh the index in the end.  Since\n34933d0eff (\"stash: make sure to write refreshed cache\", 2019-09-11),\nwe also write that refreshed index when --quiet is given to 'git stash\napply'.\n\nHowever if '--index' is not given to 'git stash apply', we also\ndiscard the index in the else clause just before.  We need to do so\nbecause we use an external 'git update-index --add --stdin', which\nleads to an out of date in-core index.\n\nLater we call 'refresh_and_write_cache', which now leads to writing\nthe discarded index, which means we essentially write an empty index\nfile.  This is obviously not correct, or the behaviour the user\nwanted.  We should not modify the users index without being asked to\ndo so.\n\nMake sure to re-read the index after discarding the current in-core\nindex, to avoid dealing with outdated information.  Instead we could\nalso drop the 'discard_cache()' + 'read_cache()', however that would\nmake it easy to fall into the same trap as 34933d0eff did, so it's\nbetter to avoid that.\n\nWe can also drop the 'refresh_and_write_cache' completely in the quiet\ncase.  Previously in legacy stash we relied on 'git status' to refresh\nthe index after calling 'git read-tree' when '--index' was passed to\n'git apply'.  However the 'reset_tree()' call that replaced 'git\nread-tree' always passes options that are equivalent to '-m', making\nthe refresh of the index unnecessary.\n\nReported-by: Grzegorz Rajchman <rayman17@gmail.com>\nSigned-off-by: Thomas Gummerer <t.gummerer@gmail.com>\n---\n builtin/stash.c  | 7 +++----\n t/t3903-stash.sh | 7 ++++++-\n 2 files changed, 9 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/stash.c b/builtin/stash.c\nindex ab30d1e920..372fbdb7ac 100644\n--- a/builtin/stash.c\n+++ b/builtin/stash.c\n@@ -481,13 +481,12 @@ static int do_apply_stash(const char *prefix, struct stash_info *info,\n \t\tif (ret)\n \t\t\treturn -1;\n \n+\t\t/* read back the result of update_index() back from the disk */\n \t\tdiscard_cache();\n+\t\tread_cache();\n \t}\n \n-\tif (quiet) {\n-\t\tif (refresh_and_write_cache(REFRESH_QUIET, 0, 0))\n-\t\t\twarning(\"could not refresh index\");\n-\t} else {\n+\tif (!quiet) {\n \t\tstruct child_process cp = CHILD_PROCESS_INIT;\n \n \t\t/*\ndiff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\nindex 392954d6dd..9de1c3616a 100755\n--- a/t/t3903-stash.sh\n+++ b/t/t3903-stash.sh\n@@ -232,8 +232,11 @@ test_expect_success 'save -q is quiet' '\n \ttest_must_be_empty output.out\n '\n \n-test_expect_success 'pop -q is quiet' '\n+test_expect_success 'pop -q works and is quiet' '\n \tgit stash pop -q >output.out 2>&1 &&\n+\techo bar >expect &&\n+\tgit show :file >actual &&\n+\ttest_cmp expect actual &&\n \ttest_must_be_empty output.out\n '\n \n@@ -242,6 +245,8 @@ test_expect_success 'pop -q --index works and is quiet' '\n \tgit add file &&\n \tgit stash save --quiet &&\n \tgit stash pop -q --index >output.out 2>&1 &&\n+\tgit diff-files file2 >file2.diff &&\n+\ttest_must_be_empty file2.diff &&\n \ttest foo = \"$(git show :file)\" &&\n \ttest_must_be_empty output.out\n '\n-- \n2.24.0.155.gd9f6f3b619\n\n"},{"id":"386151","messageId":"xmqqmucz9pnn.fsf@gitster-ct.c.googlers.com","threadId":"52210","inReplyTo":"20191113150136.GB3047@cat","subject":"Re: [PATCH v3] stash: make sure we have a valid index before writing it","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-11-14T02:07:24Z","receivedAt":"2019-11-14T02:07:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Gummerer <t.gummerer@gmail.com> writes:\n\n> Subject: [PATCH v3] stash: make sure we have a valid index before writing it\n>\n> In 'do_apply_stash()' we refresh the index in the end.  Since\n> 34933d0eff (\"stash: make sure to write refreshed cache\", 2019-09-11),\n> we also write that refreshed index when --quiet is given to 'git stash\n> apply'.\n>\n> However if '--index' is not given to 'git stash apply', we also\n> discard the index in the else clause just before.  We need to do so\n> because we use an external 'git update-index --add --stdin', which\n> leads to an out of date in-core index.\n>\n> Later we call 'refresh_and_write_cache', which now leads to writing\n> the discarded index, which means we essentially write an empty index\n> file.  This is obviously not correct, or the behaviour the user\n> wanted.  We should not modify the users index without being asked to\n> do so.\n>\n> Make sure to re-read the index after discarding the current in-core\n> index, to avoid dealing with outdated information.  Instead we could\n> also drop the 'discard_cache()' + 'read_cache()', however that would\n> make it easy to fall into the same trap as 34933d0eff did, so it's\n> better to avoid that.\n>\n> We can also drop the 'refresh_and_write_cache' completely in the quiet\n> case.  Previously in legacy stash we relied on 'git status' to refresh\n> the index after calling 'git read-tree' when '--index' was passed to\n> 'git apply'.  However the 'reset_tree()' call that replaced 'git\n> read-tree' always passes options that are equivalent to '-m', making\n> the refresh of the index unnecessary.\n>\n> Reported-by: Grzegorz Rajchman <rayman17@gmail.com>\n> Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>\n> ---\n\nThanks.  This looks good and minimal ;-)\n\n>  builtin/stash.c  | 7 +++----\n>  t/t3903-stash.sh | 7 ++++++-\n>  2 files changed, 9 insertions(+), 5 deletions(-)\n>\n> diff --git a/builtin/stash.c b/builtin/stash.c\n> index ab30d1e920..372fbdb7ac 100644\n> --- a/builtin/stash.c\n> +++ b/builtin/stash.c\n> @@ -481,13 +481,12 @@ static int do_apply_stash(const char *prefix, struct stash_info *info,\n>  \t\tif (ret)\n>  \t\t\treturn -1;\n>  \n> +\t\t/* read back the result of update_index() back from the disk */\n>  \t\tdiscard_cache();\n> +\t\tread_cache();\n>  \t}\n>  \n> -\tif (quiet) {\n> -\t\tif (refresh_and_write_cache(REFRESH_QUIET, 0, 0))\n> -\t\t\twarning(\"could not refresh index\");\n> -\t} else {\n> +\tif (!quiet) {\n>  \t\tstruct child_process cp = CHILD_PROCESS_INIT;\n>  \n>  \t\t/*\n> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\n> index 392954d6dd..9de1c3616a 100755\n> --- a/t/t3903-stash.sh\n> +++ b/t/t3903-stash.sh\n> @@ -232,8 +232,11 @@ test_expect_success 'save -q is quiet' '\n>  \ttest_must_be_empty output.out\n>  '\n>  \n> -test_expect_success 'pop -q is quiet' '\n> +test_expect_success 'pop -q works and is quiet' '\n>  \tgit stash pop -q >output.out 2>&1 &&\n> +\techo bar >expect &&\n> +\tgit show :file >actual &&\n> +\ttest_cmp expect actual &&\n>  \ttest_must_be_empty output.out\n>  '\n>  \n> @@ -242,6 +245,8 @@ test_expect_success 'pop -q --index works and is quiet' '\n>  \tgit add file &&\n>  \tgit stash save --quiet &&\n>  \tgit stash pop -q --index >output.out 2>&1 &&\n> +\tgit diff-files file2 >file2.diff &&\n> +\ttest_must_be_empty file2.diff &&\n>  \ttest foo = \"$(git show :file)\" &&\n>  \ttest_must_be_empty output.out\n>  '\n"}]}