* [BUG] git stash pop --quiet deletes files in git 2.24.0 @ 2019-11-07 10:36 Grzegorz Rajchman 2019-11-07 18:49 ` Thomas Gummerer 0 siblings, 1 reply; 13+ messages in thread From: Grzegorz Rajchman @ 2019-11-07 10:36 UTC (permalink / raw) To: git Hi, this is the first time I report an issue in git so I hope I'm doing it right. I have experienced some unexpected behaviour with git stash pop --quiet in git 2.24.0. I use stash in a pre-commit hook script. In it, I stash non-staged changes to keep the working directory clean while running some linters, then I restore the stash by running pop, but after the recent git update I noticed that it stages all previously checked files as deleted. Steps to reproduce: mkdir test-git-stash cd test-git-stash/ git init echo foo > foo.txt git add . && git commit -m 'init' echo bar > foo.txt git stash save --quiet --include-untracked --keep-index git stash pop --quiet git status This will unexpectedly output: On branch master Changes to be committed: (use "git restore --staged <file>..." to unstage) deleted: foo.txt Untracked files: (use "git add <file>..." to include in what will be committed) foo.txt Notice that foo.txt was staged as deleted whilst still being present on the disk. However, if I remove --quiet flag from stash pop: git restore --staged foo.txt git stash save --quiet --include-untracked --keep-index git stash pop git status Then it works as expected. It used to work as expected in git prior to 2.24.0 My OS is Ubuntu 19.04. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [BUG] git stash pop --quiet deletes files in git 2.24.0 2019-11-07 10:36 [BUG] git stash pop --quiet deletes files in git 2.24.0 Grzegorz Rajchman @ 2019-11-07 18:49 ` Thomas Gummerer 2019-11-08 2:32 ` Junio C Hamano 0 siblings, 1 reply; 13+ messages in thread From: Thomas Gummerer @ 2019-11-07 18:49 UTC (permalink / raw) To: Grzegorz Rajchman; +Cc: git On 11/07, Grzegorz Rajchman wrote: > Hi, this is the first time I report an issue in git so I hope I'm > doing it right. Thanks for the report. You are indeed doing this right, and the included reproduction is very helpful. I broke this in 34933d0eff ("stash: make sure to write refreshed cache", 2019-09-11), which wasn't caught by the tests, nor by me as I don't use the --quiet flag normally. Below is a fix for this, but I want to understand the problem a bit better and write some tests before sending a patch. index ab30d1e920..2dd9c9bbcd 100644 --- a/builtin/stash.c +++ b/builtin/stash.c @@ -473,22 +473,20 @@ static int do_apply_stash(const char *prefix, struct stash_info *info, if (reset_tree(&c_tree, 0, 1)) { strbuf_release(&out); return -1; } ret = update_index(&out); strbuf_release(&out); if (ret) return -1; - - discard_cache(); } if (quiet) { if (refresh_and_write_cache(REFRESH_QUIET, 0, 0)) warning("could not refresh index"); } else { struct child_process cp = CHILD_PROCESS_INIT; /* * Status is quite simple and could be replaced with calls to > I have experienced some unexpected behaviour with git stash pop > --quiet in git 2.24.0. I use stash in a pre-commit hook script. In it, > I stash non-staged changes to keep the working directory clean while > running some linters, then I restore the stash by running pop, but > after the recent git update I noticed that it stages all previously > checked files as deleted. > > Steps to reproduce: > > mkdir test-git-stash > cd test-git-stash/ > git init > echo foo > foo.txt > git add . && git commit -m 'init' > echo bar > foo.txt > git stash save --quiet --include-untracked --keep-index > git stash pop --quiet > git status > > This will unexpectedly output: > > On branch master > Changes to be committed: > (use "git restore --staged <file>..." to unstage) > deleted: foo.txt > > Untracked files: > (use "git add <file>..." to include in what will be committed) > foo.txt > > Notice that foo.txt was staged as deleted whilst still being present > on the disk. > > However, if I remove --quiet flag from stash pop: > > git restore --staged foo.txt > git stash save --quiet --include-untracked --keep-index > git stash pop > git status > > Then it works as expected. It used to work as expected in git prior to 2.24.0 > > My OS is Ubuntu 19.04. ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [BUG] git stash pop --quiet deletes files in git 2.24.0 2019-11-07 18:49 ` Thomas Gummerer @ 2019-11-08 2:32 ` Junio C Hamano 2019-11-08 16:59 ` Thomas Gummerer 0 siblings, 1 reply; 13+ messages in thread From: Junio C Hamano @ 2019-11-08 2:32 UTC (permalink / raw) To: Thomas Gummerer; +Cc: Grzegorz Rajchman, git Thomas Gummerer <t.gummerer@gmail.com> writes: > On 11/07, Grzegorz Rajchman wrote: >> Hi, this is the first time I report an issue in git so I hope I'm >> doing it right. > > Thanks for the report. You are indeed doing this right, and the > included reproduction is very helpful. > > I broke this in 34933d0eff ("stash: make sure to write refreshed > cache", 2019-09-11), which wasn't caught by the tests, nor by me as I > don't use the --quiet flag normally. > > Below is a fix for this, but I want to understand the problem a bit > better and write some tests before sending a patch. OK, thanks for quickly looking into this. The commit added two places where refresh_and_write_cache() gets called. The first one at the very beginning of do_apply_stash() used to be refresh_cache() that immediately follows read_cache_preload(). We are writing back exactly what we read from the filesystem [*], so this should be a no-op from the correctness POV, with benefit of having a refreshed cache on disk. Side note. This argument assumes that no caller has called read_cache() before calling us and did its own in-core index operation. In such a case, the in-core index is already out of sync with the on-disk one due to our own operation, and read_cache() will not overwrite already initilized in-core index, so we will write out what the original code did not want to, which would be a bug. The second one happens after we do all the 3-way merges to replay the change between the base commit and the working tree state recorded in the stash, and then adjust the index to the desired state: - If we are propagating the change to the index recorded in the stash to the current index, reset_tree() reads the index_tree that has been computed earlier in the function to update the in-core index and the on-disk index. - Otherwise, we compute paths added between the base commit and the working tree state recorded in the stash (i.e. those that were created but not yet commited when the stash was made), go back to the in-core index state we had upon entry to this function (i.e. c_tree), and then add these new paths from the working tree directly to the on-disk index without updating the in-core index. Notice that this leaves the in-core index stale wrt the on-disk index---but the stale in-core index gets discarded. Then the code goes on to do: - under --quiet, refresh_cache() used to be called to silently refresh the in-core index. 34933d0eff made it to also write the in-core index to on-disk index. OOPS. The in-core index has been discarded at this point. - otherwise, "git status" is spawned and directly acted on the on-disk index (this also has a side effect of writing a refreshed on-disk index). So, I do not think removing that discard_cache() alone solves the breakage exposed by 34933d0eff. Discarding and re-reading the on-disk index there would restore correctness, but then you would want to make sure that we are not wasting the overall cost for the I/O and refreshing. I think the safer immediate short-term fix is to revert the change to the quiet codepath and let it only refresh the in-core index. > index ab30d1e920..2dd9c9bbcd 100644 > --- a/builtin/stash.c > +++ b/builtin/stash.c > @@ -473,22 +473,20 @@ static int do_apply_stash(const char *prefix, struct stash_info *info, > > if (reset_tree(&c_tree, 0, 1)) { > strbuf_release(&out); > return -1; > } > > ret = update_index(&out); > strbuf_release(&out); > if (ret) > return -1; > - > - discard_cache(); > } > > if (quiet) { > if (refresh_and_write_cache(REFRESH_QUIET, 0, 0)) > warning("could not refresh index"); > } else { > struct child_process cp = CHILD_PROCESS_INIT; ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [BUG] git stash pop --quiet deletes files in git 2.24.0 2019-11-08 2:32 ` Junio C Hamano @ 2019-11-08 16:59 ` Thomas Gummerer 2019-11-10 6:11 ` Junio C Hamano 0 siblings, 1 reply; 13+ messages in thread From: Thomas Gummerer @ 2019-11-08 16:59 UTC (permalink / raw) To: Junio C Hamano; +Cc: Grzegorz Rajchman, git On 11/08, Junio C Hamano wrote: > Thomas Gummerer <t.gummerer@gmail.com> writes: > > > On 11/07, Grzegorz Rajchman wrote: > >> Hi, this is the first time I report an issue in git so I hope I'm > >> doing it right. > > > > Thanks for the report. You are indeed doing this right, and the > > included reproduction is very helpful. > > > > I broke this in 34933d0eff ("stash: make sure to write refreshed > > cache", 2019-09-11), which wasn't caught by the tests, nor by me as I > > don't use the --quiet flag normally. > > > > Below is a fix for this, but I want to understand the problem a bit > > better and write some tests before sending a patch. > > OK, thanks for quickly looking into this. > > The commit added two places where refresh_and_write_cache() gets > called. > > The first one at the very beginning of do_apply_stash() used to be > refresh_cache() that immediately follows read_cache_preload(). We > are writing back exactly what we read from the filesystem [*], so > this should be a no-op from the correctness POV, with benefit of > having a refreshed cache on disk. > > Side note. This argument assumes that no caller has called > read_cache() before calling us and did its own in-core index > operation. In such a case, the in-core index is already out > of sync with the on-disk one due to our own operation, and > read_cache() will not overwrite already initilized in-core > index, so we will write out what the original code did not > want to, which would be a bug. > > The second one happens after we do all the 3-way merges to replay > the change between the base commit and the working tree state > recorded in the stash, and then adjust the index to the desired > state: > > - If we are propagating the change to the index recorded in the > stash to the current index, reset_tree() reads the index_tree > that has been computed earlier in the function to update the > in-core index and the on-disk index. > > - Otherwise, we compute paths added between the base commit and the > working tree state recorded in the stash (i.e. those that were > created but not yet commited when the stash was made), go back to > the in-core index state we had upon entry to this function > (i.e. c_tree), and then add these new paths from the working tree > directly to the on-disk index without updating the in-core > index. Notice that this leaves the in-core index stale wrt the > on-disk index---but the stale in-core index gets discarded. > > Then the code goes on to do: > > - under --quiet, refresh_cache() used to be called to silently > refresh the in-core index. 34933d0eff made it to also write the > in-core index to on-disk index. OOPS. The in-core index has > been discarded at this point. Yup, this is certainly my bad, we shouldn't be writing the discarded index of course. I don't think what we were doing here before was correct either though. The only thing that would be called after this is 'do_stash_drop()', which only executes external commands. I think the original intention here was to replace 'git status >/dev/null 2>&1' from the shell script, which as you note below did refresh the index. From what you are saying above, and from my testing I think this refresh is actually unnecessary, and we could just remove it outright. I'm still trying to think if there could be any way refreshing the index could be useful (before I introduce another bug here inadvertently). If I can't come up with anything I'll send a patch with the corresponding test case removing the 'refresh_cache()' completely. > - otherwise, "git status" is spawned and directly acted on the > on-disk index (this also has a side effect of writing a refreshed > on-disk index). > > So, I do not think removing that discard_cache() alone solves the > breakage exposed by 34933d0eff. Discarding and re-reading the > on-disk index there would restore correctness, but then you would > want to make sure that we are not wasting the overall cost for the > I/O and refreshing. > > I think the safer immediate short-term fix is to revert the change > to the quiet codepath and let it only refresh the in-core index. > > > index ab30d1e920..2dd9c9bbcd 100644 > > --- a/builtin/stash.c > > +++ b/builtin/stash.c > > @@ -473,22 +473,20 @@ static int do_apply_stash(const char *prefix, struct stash_info *info, > > > > if (reset_tree(&c_tree, 0, 1)) { > > strbuf_release(&out); > > return -1; > > } > > > > ret = update_index(&out); > > strbuf_release(&out); > > if (ret) > > return -1; > > - > > - discard_cache(); > > } > > > > if (quiet) { > > if (refresh_and_write_cache(REFRESH_QUIET, 0, 0)) > > warning("could not refresh index"); > > } else { > > struct child_process cp = CHILD_PROCESS_INIT; ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [BUG] git stash pop --quiet deletes files in git 2.24.0 2019-11-08 16:59 ` Thomas Gummerer @ 2019-11-10 6:11 ` Junio C Hamano 2019-11-11 19:56 ` Thomas Gummerer 0 siblings, 1 reply; 13+ messages in thread From: Junio C Hamano @ 2019-11-10 6:11 UTC (permalink / raw) To: Thomas Gummerer; +Cc: Grzegorz Rajchman, git Thomas Gummerer <t.gummerer@gmail.com> writes: > On 11/08, Junio C Hamano wrote: >> So, I do not think removing that discard_cache() alone solves the >> breakage exposed by 34933d0eff. Discarding and re-reading the >> on-disk index there would restore correctness, but then you would >> want to make sure that we are not wasting the overall cost for the >> I/O and refreshing. >> >> I think the safer immediate short-term fix is to revert the change >> to the quiet codepath and let it only refresh the in-core index. > > Yup, this is certainly my bad, we shouldn't be writing the discarded > index of course. I don't think what we were doing here before was > correct either though. The only thing that would be called after this > is 'do_stash_drop()', which only executes external commands. Right. Removing discard alone would not be a correct fix exactly for that reason: the in-core index was stale wrt the on-disk index. If the program later used in-core index for further processing (which is not, and that is why the short-term solution of reverting that hunk would work), we would have been operating on a wrong data. So for the fix that keeps data we have in-core always up-to-date, we should be re-reading from the on-disk index there after discard(). And in the longer term, it would likely be the right direction, as the "git status" invocation on the non-quiet codepath would want to become an in-core direct calls into wt-status machinery instead of fork+exec eventually. > From what you are saying above, and from my testing I think this > refresh is actually unnecessary, and we could just remove it outright. Perhaps. But later it will bite us when somebody wants to rewrite the "status at the end" part in C. Besides, if the original was "update-index -q --refresh" in the scripted Porcelain after an pop was attempted, it would have shown the unmerged paths as "needs merge", wouldn't it? For that, we need to have something (I do not remember if refresh_and_write_cache() would be the in-core API call to do so offhand). Thanks. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [BUG] git stash pop --quiet deletes files in git 2.24.0 2019-11-10 6:11 ` Junio C Hamano @ 2019-11-11 19:56 ` Thomas Gummerer 2019-11-12 5:21 ` Junio C Hamano 2019-11-13 11:17 ` [PATCH v2 1/2] t3903: avoid git commands inside command substitution Thomas Gummerer 0 siblings, 2 replies; 13+ messages in thread From: Thomas Gummerer @ 2019-11-11 19:56 UTC (permalink / raw) To: Junio C Hamano; +Cc: Grzegorz Rajchman, git On 11/10, Junio C Hamano wrote: > Thomas Gummerer <t.gummerer@gmail.com> writes: > > > On 11/08, Junio C Hamano wrote: > >> So, I do not think removing that discard_cache() alone solves the > >> breakage exposed by 34933d0eff. Discarding and re-reading the > >> on-disk index there would restore correctness, but then you would > >> want to make sure that we are not wasting the overall cost for the > >> I/O and refreshing. > >> > >> I think the safer immediate short-term fix is to revert the change > >> to the quiet codepath and let it only refresh the in-core index. > > > > Yup, this is certainly my bad, we shouldn't be writing the discarded > > index of course. I don't think what we were doing here before was > > correct either though. The only thing that would be called after this > > is 'do_stash_drop()', which only executes external commands. > > Right. Removing discard alone would not be a correct fix exactly > for that reason: the in-core index was stale wrt the on-disk index. > > If the program later used in-core index for further processing > (which is not, and that is why the short-term solution of reverting > that hunk would work), we would have been operating on a wrong data. > So for the fix that keeps data we have in-core always up-to-date, we > should be re-reading from the on-disk index there after discard(). > > And in the longer term, it would likely be the right direction, as > the "git status" invocation on the non-quiet codepath would want to > become an in-core direct calls into wt-status machinery instead of > fork+exec eventually. Right. I'd argue that that's even the right direction in the short term. It does require some more I/O but it also prevents similar mistakes. And I don't think one additional read of the index is going to make it or break it for performance here, there are plenty of reads already, and there's probably better ways to speed 'git stash' up. > > From what you are saying above, and from my testing I think this > > refresh is actually unnecessary, and we could just remove it outright. > > Perhaps. But later it will bite us when somebody wants to rewrite > the "status at the end" part in C. Hmm, wouldn't the not re-reading the index part bite us there, rather than the not refreshing the index? In the 'has_index' codepath, we write the index to disk, so we already have a fresh one in-core. This codepath is what used to require refreshing the index afterwards, but no longer does. Previously we used to use 'git read-tree "$unstashed_index_tree"' there, which does require a 'git update-index -q --refresh' afterwards. However we have replaced that with an internal call to 'reset_tree', which always sets 'o.merge = 1' for unpack-trees. Which in turn means that the index is already refreshed appropriately iiuc. In the other codepath we do 'git update-index --add --stdin', which also doesn't require refreshing the index, but does require the 'discard_cache()' + 'read_cache()' afterwards, so we're not left in a half state. > Besides, if the original was "update-index -q --refresh" in the > scripted Porcelain after an pop was attempted, it would have shown > the unmerged paths as "needs merge", wouldn't it? For that, we need > to have something (I do not remember if refresh_and_write_cache() > would be the in-core API call to do so offhand). The original used 'git status >/dev/null 2>&1' to refresh the index after the 'git read-tree' I mentioned above, but would not show the "needs merge" message, so I think we're okay on that front. Below is the patch that I believe has the least chances of biting us in the future, with the appropriate updated tests. I had considered leaving the 'refresh_and_write_cache()' call there, but as I was writing the commit message I had a harder and harder time justifying that, so it's gone now, which I think is the right thing to do. Leaving it there would be okay as well, however I don't think it would have any benefit. --- >8 --- Subject: [PATCH] stash: make sure we have a valid index before writing it In 'do_apply_stash()' we refresh the index in the end. Since 34933d0eff ("stash: make sure to write refreshed cache", 2019-09-11), we also write that refreshed index when --quiet is given to 'git stash apply'. However if '--index' is not given to 'git stash apply', we also discard the index in the else clause just before. This leads to writing the discarded index, which means we essentially write an empty index file. This is obviously not correct, or the behaviour the user wanted. We should not modify the users index without being asked to do so. Make sure to re-read the index after discarding the current in-core index, to avoid dealing with outdated information. We can drop the 'refresh_and_write_cache' completely in the quiet case. Previously in legacy stash we relied on 'git status' to refresh the index after calling 'git read-tree' when '--index' was passed to 'git apply'. However the 'reset_tree()' call that replaced 'git read-tree' always passes options that are equivalent to '-m', making the refresh of the index unnecessary. We could also drop the 'discard_cache()' + 'read_cache()', however that would make it easy to fall into the same trap as 34933d0eff did, so it's better to avoid that. Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com> --- builtin/stash.c | 6 ++---- t/t3903-stash.sh | 5 ++++- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/builtin/stash.c b/builtin/stash.c index ab30d1e920..d00567285f 100644 --- a/builtin/stash.c +++ b/builtin/stash.c @@ -482,12 +482,10 @@ static int do_apply_stash(const char *prefix, struct stash_info *info, return -1; discard_cache(); + read_cache(); } - if (quiet) { - if (refresh_and_write_cache(REFRESH_QUIET, 0, 0)) - warning("could not refresh index"); - } else { + if (!quiet) { struct child_process cp = CHILD_PROCESS_INIT; /* diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh index 392954d6dd..b1c973e3d9 100755 --- a/t/t3903-stash.sh +++ b/t/t3903-stash.sh @@ -232,8 +232,9 @@ test_expect_success 'save -q is quiet' ' test_must_be_empty output.out ' -test_expect_success 'pop -q is quiet' ' +test_expect_success 'pop -q works and is quiet' ' git stash pop -q >output.out 2>&1 && + test bar = "$(git show :file)" && test_must_be_empty output.out ' @@ -242,6 +243,8 @@ test_expect_success 'pop -q --index works and is quiet' ' git add file && git stash save --quiet && git stash pop -q --index >output.out 2>&1 && + git diff-files file2 >file2.diff && + test_must_be_empty file2.diff && test foo = "$(git show :file)" && test_must_be_empty output.out ' -- 2.24.0.155.gd9f6f3b619 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [BUG] git stash pop --quiet deletes files in git 2.24.0 2019-11-11 19:56 ` Thomas Gummerer @ 2019-11-12 5:21 ` Junio C Hamano 2019-11-13 11:15 ` Thomas Gummerer 2019-11-13 11:17 ` [PATCH v2 1/2] t3903: avoid git commands inside command substitution Thomas Gummerer 1 sibling, 1 reply; 13+ messages in thread From: Junio C Hamano @ 2019-11-12 5:21 UTC (permalink / raw) To: Thomas Gummerer; +Cc: Grzegorz Rajchman, git Thomas Gummerer <t.gummerer@gmail.com> writes: >> > From what you are saying above, and from my testing I think this >> > refresh is actually unnecessary, and we could just remove it outright. >> >> Perhaps. But later it will bite us when somebody wants to rewrite >> the "status at the end" part in C. > > Hmm, wouldn't the not re-reading the index part bite us there, rather > than the not refreshing the index? Yes. Just removing the refresh-and-write that caused us to write out incorrect data would "fix" the bug, while leaving the bug of not re-reading to bite us later. > Below is the patch that I believe has the least chances of biting us > in the future, with the appropriate updated tests. I had considered > leaving the 'refresh_and_write_cache()' call there, but as I was > writing the commit message I had a harder and harder time justifying > that, so it's gone now, which I think is the right thing to do. > Leaving it there would be okay as well, however I don't think it would > have any benefit. > > --- >8 --- > Subject: [PATCH] stash: make sure we have a valid index before writing it > > In 'do_apply_stash()' we refresh the index in the end. Since > 34933d0eff ("stash: make sure to write refreshed cache", 2019-09-11), > we also write that refreshed index when --quiet is given to 'git stash > apply'. > > However if '--index' is not given to 'git stash apply', we also > discard the index in the else clause just before. This leads to > writing the discarded index, which means we essentially write an empty > index file. This is obviously not correct, or the behaviour the user > wanted. We should not modify the users index without being asked to > do so. > > Make sure to re-read the index after discarding the current in-core > index, to avoid dealing with outdated information. Yup. The "!has_index" codepath calls update_index() that turns the on-disk index into the desired shape (would it help explaining that in the previous paragraph, by the way?) so all we need to do is to read it back into core. Makes sense. > We can drop the 'refresh_and_write_cache' completely in the quiet > case. Previously in legacy stash we relied on 'git status' to refresh > the index after calling 'git read-tree' when '--index' was passed to > 'git apply'. However the 'reset_tree()' call that replaced 'git > read-tree' always passes options that are equivalent to '-m', making > the refresh of the index unnecessary. OK. > We could also drop the 'discard_cache()' + 'read_cache()', however > that would make it easy to fall into the same trap as 34933d0eff did, > so it's better to avoid that. This is the discarded alternative of the main fix we saw earlier. Perhaps it may make the flow of thought easier to follow if we moved it up before talking about "refresh-and-write can be thrown away"? > Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com> > --- > builtin/stash.c | 6 ++---- > t/t3903-stash.sh | 5 ++++- > 2 files changed, 6 insertions(+), 5 deletions(-) > > diff --git a/builtin/stash.c b/builtin/stash.c > index ab30d1e920..d00567285f 100644 > --- a/builtin/stash.c > +++ b/builtin/stash.c > @@ -482,12 +482,10 @@ static int do_apply_stash(const char *prefix, struct stash_info *info, > return -1; > > discard_cache(); > + read_cache(); A comment /* read back the result of update_index() back from the disk */ before discard_cache() may be warranted? > } > > - if (quiet) { > - if (refresh_and_write_cache(REFRESH_QUIET, 0, 0)) > - warning("could not refresh index"); > - } else { OK. > + if (!quiet) { > struct child_process cp = CHILD_PROCESS_INIT; > > /* > diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh > index 392954d6dd..b1c973e3d9 100755 > --- a/t/t3903-stash.sh > +++ b/t/t3903-stash.sh > @@ -232,8 +232,9 @@ test_expect_success 'save -q is quiet' ' > test_must_be_empty output.out > ' > > -test_expect_success 'pop -q is quiet' ' > +test_expect_success 'pop -q works and is quiet' ' > git stash pop -q >output.out 2>&1 && > + test bar = "$(git show :file)" && Ah, this is to ensure that we didn't lose the "file" from the index? Denton is on the quest of removing "$(git command substitution)" used in a way that might hide the error from git invocation in a separate thread [*1*]. This may want to become git rev-parse --verify :file && or git show :file >actual && echo bar >expect && test_cmp expect actual && perhaps? > test_must_be_empty output.out > ' > > @@ -242,6 +243,8 @@ test_expect_success 'pop -q --index works and is quiet' ' > git add file && > git stash save --quiet && > git stash pop -q --index >output.out 2>&1 && > + git diff-files file2 >file2.diff && > + test_must_be_empty file2.diff && > test foo = "$(git show :file)" && > test_must_be_empty output.out > ' Dittto. Thanks. [Reference] *1* <2f9052fd94ebb6fe93ea6fe2e7cd3c717635c822.1573517561.git.liu.denton@gmail.com> Note that "var=$(git subcmd)" is special and will signal us a failure of the git invocation. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [BUG] git stash pop --quiet deletes files in git 2.24.0 2019-11-12 5:21 ` Junio C Hamano @ 2019-11-13 11:15 ` Thomas Gummerer 2019-11-13 13:31 ` Junio C Hamano 0 siblings, 1 reply; 13+ messages in thread From: Thomas Gummerer @ 2019-11-13 11:15 UTC (permalink / raw) To: Junio C Hamano; +Cc: Grzegorz Rajchman, git On 11/12, Junio C Hamano wrote: > Thomas Gummerer <t.gummerer@gmail.com> writes: > > >> > From what you are saying above, and from my testing I think this > >> > refresh is actually unnecessary, and we could just remove it outright. > >> > >> Perhaps. But later it will bite us when somebody wants to rewrite > >> the "status at the end" part in C. > > > > Hmm, wouldn't the not re-reading the index part bite us there, rather > > than the not refreshing the index? > > Yes. Just removing the refresh-and-write that caused us to write > out incorrect data would "fix" the bug, while leaving the bug of not > re-reading to bite us later. > > > Below is the patch that I believe has the least chances of biting us > > in the future, with the appropriate updated tests. I had considered > > leaving the 'refresh_and_write_cache()' call there, but as I was > > writing the commit message I had a harder and harder time justifying > > that, so it's gone now, which I think is the right thing to do. > > Leaving it there would be okay as well, however I don't think it would > > have any benefit. > > > > --- >8 --- > > Subject: [PATCH] stash: make sure we have a valid index before writing it > > > > In 'do_apply_stash()' we refresh the index in the end. Since > > 34933d0eff ("stash: make sure to write refreshed cache", 2019-09-11), > > we also write that refreshed index when --quiet is given to 'git stash > > apply'. > > > > However if '--index' is not given to 'git stash apply', we also > > discard the index in the else clause just before. This leads to > > writing the discarded index, which means we essentially write an empty > > index file. This is obviously not correct, or the behaviour the user > > wanted. We should not modify the users index without being asked to > > do so. > > > > Make sure to re-read the index after discarding the current in-core > > index, to avoid dealing with outdated information. > > Yup. The "!has_index" codepath calls update_index() that turns the > on-disk index into the desired shape (would it help explaining that > in the previous paragraph, by the way?) so all we need to do is to > read it back into core. Makes sense. Will add some more explanation about that. > > We could also drop the 'discard_cache()' + 'read_cache()', however > > that would make it easy to fall into the same trap as 34933d0eff did, > > so it's better to avoid that. > > This is the discarded alternative of the main fix we saw earlier. > Perhaps it may make the flow of thought easier to follow if we moved > it up before talking about "refresh-and-write can be thrown away"? Thanks, will move. > > Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com> > > --- > > builtin/stash.c | 6 ++---- > > t/t3903-stash.sh | 5 ++++- > > 2 files changed, 6 insertions(+), 5 deletions(-) > > > > diff --git a/builtin/stash.c b/builtin/stash.c > > index ab30d1e920..d00567285f 100644 > > --- a/builtin/stash.c > > +++ b/builtin/stash.c > > @@ -482,12 +482,10 @@ static int do_apply_stash(const char *prefix, struct stash_info *info, > > return -1; > > > > discard_cache(); > > + read_cache(); > > A comment > > /* read back the result of update_index() back from the disk */ > > before discard_cache() may be warranted? Yeah that makes sense, will add. > > } > > > > - if (quiet) { > > - if (refresh_and_write_cache(REFRESH_QUIET, 0, 0)) > > - warning("could not refresh index"); > > - } else { > > OK. > > > + if (!quiet) { > > struct child_process cp = CHILD_PROCESS_INIT; > > > > /* > > diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh > > index 392954d6dd..b1c973e3d9 100755 > > --- a/t/t3903-stash.sh > > +++ b/t/t3903-stash.sh > > @@ -232,8 +232,9 @@ test_expect_success 'save -q is quiet' ' > > test_must_be_empty output.out > > ' > > > > -test_expect_success 'pop -q is quiet' ' > > +test_expect_success 'pop -q works and is quiet' ' > > git stash pop -q >output.out 2>&1 && > > + test bar = "$(git show :file)" && > > Ah, this is to ensure that we didn't lose the "file" from the index? > > Denton is on the quest of removing "$(git command substitution)" > used in a way that might hide the error from git invocation in a > separate thread [*1*]. This may want to become > > git rev-parse --verify :file && > > or > > git show :file >actual && echo bar >expect && > test_cmp expect actual && > > perhaps? Hmm I just copy-pasted this from somewhere else in this test file. I'll add a preparatory patch getting rid of "$(git command substitution)" as I don't believe Denton got to t3903 yet. There's some more opportunities for modernization of this test file, but I refrained from doing that to not blow up this bug fix series too much. > > test_must_be_empty output.out > > ' > > > > @@ -242,6 +243,8 @@ test_expect_success 'pop -q --index works and is quiet' ' > > git add file && > > git stash save --quiet && > > git stash pop -q --index >output.out 2>&1 && > > + git diff-files file2 >file2.diff && > > + test_must_be_empty file2.diff && > > test foo = "$(git show :file)" && > > test_must_be_empty output.out > > ' > > Dittto. > > Thanks. > > > [Reference] > > *1* <2f9052fd94ebb6fe93ea6fe2e7cd3c717635c822.1573517561.git.liu.denton@gmail.com> > > Note that "var=$(git subcmd)" is special and will signal us a failure > of the git invocation. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [BUG] git stash pop --quiet deletes files in git 2.24.0 2019-11-13 11:15 ` Thomas Gummerer @ 2019-11-13 13:31 ` Junio C Hamano 2019-11-13 15:01 ` [PATCH v3] stash: make sure we have a valid index before writing it Thomas Gummerer 0 siblings, 1 reply; 13+ messages in thread From: Junio C Hamano @ 2019-11-13 13:31 UTC (permalink / raw) To: Thomas Gummerer; +Cc: Grzegorz Rajchman, git Thomas Gummerer <t.gummerer@gmail.com> writes: >> ... This may want to become >> >> git rev-parse --verify :file && >> >> or >> >> git show :file >actual && echo bar >expect && >> test_cmp expect actual && >> >> perhaps? > > Hmm I just copy-pasted this from somewhere else in this test file. > I'll add a preparatory patch getting rid of "$(git command substitution)" > as I don't believe Denton got to t3903 yet. > > There's some more opportunities for modernization of this test file, > but I refrained from doing that to not blow up this bug fix series too > much. It is very much appreciated that you aimed to keep the topic focused on the fixing. What I meant was merely to avoid making things worse by adding more of $(git command substitution), not cleaning up the existing ones. Thanks. ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v3] stash: make sure we have a valid index before writing it 2019-11-13 13:31 ` Junio C Hamano @ 2019-11-13 15:01 ` Thomas Gummerer 2019-11-14 2:07 ` Junio C Hamano 0 siblings, 1 reply; 13+ messages in thread From: Thomas Gummerer @ 2019-11-13 15:01 UTC (permalink / raw) To: Junio C Hamano; +Cc: Grzegorz Rajchman, git On 11/13, Junio C Hamano wrote: > Thomas Gummerer <t.gummerer@gmail.com> writes: > > >> ... This may want to become > >> > >> git rev-parse --verify :file && > >> > >> or > >> > >> git show :file >actual && echo bar >expect && > >> test_cmp expect actual && > >> > >> perhaps? > > > > Hmm I just copy-pasted this from somewhere else in this test file. > > I'll add a preparatory patch getting rid of "$(git command substitution)" > > as I don't believe Denton got to t3903 yet. > > > > There's some more opportunities for modernization of this test file, > > but I refrained from doing that to not blow up this bug fix series too > > much. > > It is very much appreciated that you aimed to keep the topic focused > on the fixing. What I meant was merely to avoid making things worse > by adding more of $(git command substitution), not cleaning up the > existing ones. I misunderstood then because the other case you had pointed out wasn't introduced in my patch, but was just in the context. I have already sent the series with the preparatory cleanup. I'm happy to just go without the cleanup though. Since I'm already sending this email, I'll just add the patch doing just that below. --- >8 --- Subject: [PATCH v3] stash: make sure we have a valid index before writing it In 'do_apply_stash()' we refresh the index in the end. Since 34933d0eff ("stash: make sure to write refreshed cache", 2019-09-11), we also write that refreshed index when --quiet is given to 'git stash apply'. However if '--index' is not given to 'git stash apply', we also discard the index in the else clause just before. We need to do so because we use an external 'git update-index --add --stdin', which leads to an out of date in-core index. Later we call 'refresh_and_write_cache', which now leads to writing the discarded index, which means we essentially write an empty index file. This is obviously not correct, or the behaviour the user wanted. We should not modify the users index without being asked to do so. Make sure to re-read the index after discarding the current in-core index, to avoid dealing with outdated information. Instead we could also drop the 'discard_cache()' + 'read_cache()', however that would make it easy to fall into the same trap as 34933d0eff did, so it's better to avoid that. We can also drop the 'refresh_and_write_cache' completely in the quiet case. Previously in legacy stash we relied on 'git status' to refresh the index after calling 'git read-tree' when '--index' was passed to 'git apply'. However the 'reset_tree()' call that replaced 'git read-tree' always passes options that are equivalent to '-m', making the refresh of the index unnecessary. Reported-by: Grzegorz Rajchman <rayman17@gmail.com> Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com> --- builtin/stash.c | 7 +++---- t/t3903-stash.sh | 7 ++++++- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/builtin/stash.c b/builtin/stash.c index ab30d1e920..372fbdb7ac 100644 --- a/builtin/stash.c +++ b/builtin/stash.c @@ -481,13 +481,12 @@ static int do_apply_stash(const char *prefix, struct stash_info *info, if (ret) return -1; + /* read back the result of update_index() back from the disk */ discard_cache(); + read_cache(); } - if (quiet) { - if (refresh_and_write_cache(REFRESH_QUIET, 0, 0)) - warning("could not refresh index"); - } else { + if (!quiet) { struct child_process cp = CHILD_PROCESS_INIT; /* diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh index 392954d6dd..9de1c3616a 100755 --- a/t/t3903-stash.sh +++ b/t/t3903-stash.sh @@ -232,8 +232,11 @@ test_expect_success 'save -q is quiet' ' test_must_be_empty output.out ' -test_expect_success 'pop -q is quiet' ' +test_expect_success 'pop -q works and is quiet' ' git stash pop -q >output.out 2>&1 && + echo bar >expect && + git show :file >actual && + test_cmp expect actual && test_must_be_empty output.out ' @@ -242,6 +245,8 @@ test_expect_success 'pop -q --index works and is quiet' ' git add file && git stash save --quiet && git stash pop -q --index >output.out 2>&1 && + git diff-files file2 >file2.diff && + test_must_be_empty file2.diff && test foo = "$(git show :file)" && test_must_be_empty output.out ' -- 2.24.0.155.gd9f6f3b619 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v3] stash: make sure we have a valid index before writing it 2019-11-13 15:01 ` [PATCH v3] stash: make sure we have a valid index before writing it Thomas Gummerer @ 2019-11-14 2:07 ` Junio C Hamano 0 siblings, 0 replies; 13+ messages in thread From: Junio C Hamano @ 2019-11-14 2:07 UTC (permalink / raw) To: Thomas Gummerer; +Cc: Grzegorz Rajchman, git Thomas Gummerer <t.gummerer@gmail.com> writes: > Subject: [PATCH v3] stash: make sure we have a valid index before writing it > > In 'do_apply_stash()' we refresh the index in the end. Since > 34933d0eff ("stash: make sure to write refreshed cache", 2019-09-11), > we also write that refreshed index when --quiet is given to 'git stash > apply'. > > However if '--index' is not given to 'git stash apply', we also > discard the index in the else clause just before. We need to do so > because we use an external 'git update-index --add --stdin', which > leads to an out of date in-core index. > > Later we call 'refresh_and_write_cache', which now leads to writing > the discarded index, which means we essentially write an empty index > file. This is obviously not correct, or the behaviour the user > wanted. We should not modify the users index without being asked to > do so. > > Make sure to re-read the index after discarding the current in-core > index, to avoid dealing with outdated information. Instead we could > also drop the 'discard_cache()' + 'read_cache()', however that would > make it easy to fall into the same trap as 34933d0eff did, so it's > better to avoid that. > > We can also drop the 'refresh_and_write_cache' completely in the quiet > case. Previously in legacy stash we relied on 'git status' to refresh > the index after calling 'git read-tree' when '--index' was passed to > 'git apply'. However the 'reset_tree()' call that replaced 'git > read-tree' always passes options that are equivalent to '-m', making > the refresh of the index unnecessary. > > Reported-by: Grzegorz Rajchman <rayman17@gmail.com> > Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com> > --- Thanks. This looks good and minimal ;-) > builtin/stash.c | 7 +++---- > t/t3903-stash.sh | 7 ++++++- > 2 files changed, 9 insertions(+), 5 deletions(-) > > diff --git a/builtin/stash.c b/builtin/stash.c > index ab30d1e920..372fbdb7ac 100644 > --- a/builtin/stash.c > +++ b/builtin/stash.c > @@ -481,13 +481,12 @@ static int do_apply_stash(const char *prefix, struct stash_info *info, > if (ret) > return -1; > > + /* read back the result of update_index() back from the disk */ > discard_cache(); > + read_cache(); > } > > - if (quiet) { > - if (refresh_and_write_cache(REFRESH_QUIET, 0, 0)) > - warning("could not refresh index"); > - } else { > + if (!quiet) { > struct child_process cp = CHILD_PROCESS_INIT; > > /* > diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh > index 392954d6dd..9de1c3616a 100755 > --- a/t/t3903-stash.sh > +++ b/t/t3903-stash.sh > @@ -232,8 +232,11 @@ test_expect_success 'save -q is quiet' ' > test_must_be_empty output.out > ' > > -test_expect_success 'pop -q is quiet' ' > +test_expect_success 'pop -q works and is quiet' ' > git stash pop -q >output.out 2>&1 && > + echo bar >expect && > + git show :file >actual && > + test_cmp expect actual && > test_must_be_empty output.out > ' > > @@ -242,6 +245,8 @@ test_expect_success 'pop -q --index works and is quiet' ' > git add file && > git stash save --quiet && > git stash pop -q --index >output.out 2>&1 && > + git diff-files file2 >file2.diff && > + test_must_be_empty file2.diff && > test foo = "$(git show :file)" && > test_must_be_empty output.out > ' ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v2 1/2] t3903: avoid git commands inside command substitution 2019-11-11 19:56 ` Thomas Gummerer 2019-11-12 5:21 ` Junio C Hamano @ 2019-11-13 11:17 ` Thomas Gummerer 2019-11-13 11:17 ` [PATCH v2 2/2] stash: make sure we have a valid index before writing it Thomas Gummerer 1 sibling, 1 reply; 13+ messages in thread From: Thomas Gummerer @ 2019-11-13 11:17 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Grzegorz Rajchman, Thomas Gummerer Running git commands inside command substitution can hide errors. Avoid doing so in t3903. Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com> --- t/t3903-stash.sh | 99 +++++++++++++++++++++++++++++++++--------------- 1 file changed, 69 insertions(+), 30 deletions(-) diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh index 392954d6dd..db7cc6e664 100755 --- a/t/t3903-stash.sh +++ b/t/t3903-stash.sh @@ -34,7 +34,7 @@ index 0cfbf08..00750ed 100644 EOF test_expect_success 'parents of stash' ' - test $(git rev-parse stash^) = $(git rev-parse HEAD) && + test_cmp_rev stash^ HEAD && git diff stash^2..stash >output && test_cmp expect output ' @@ -68,8 +68,11 @@ test_expect_success 'apply stashed changes' ' git commit -m other-file && git stash apply && test 3 = $(cat file) && - test 1 = $(git show :file) && - test 1 = $(git show HEAD:file) + echo 1 >expect && + git show :file >actual && + test_cmp expect actual && + git show HEAD:file >actual && + test_cmp expect actual ' test_expect_success 'apply stashed changes (including index)' ' @@ -80,8 +83,12 @@ test_expect_success 'apply stashed changes (including index)' ' git commit -m other-file && git stash apply --index && test 3 = $(cat file) && - test 2 = $(git show :file) && - test 1 = $(git show HEAD:file) + echo 2 >expect && + git show :file >actual && + test_cmp expect actual && + echo 1 >expect && + git show HEAD:file >actual && + test_cmp expect actual ' test_expect_success 'unstashing in a subdirectory' ' @@ -107,8 +114,11 @@ test_expect_success 'drop top stash' ' test_cmp expected actual && git stash apply && test 3 = $(cat file) && - test 1 = $(git show :file) && - test 1 = $(git show HEAD:file) + echo 1 >expect && + git show :file >actual && + test_cmp expect actual && + git show HEAD:file >actual && + test_cmp expect actual ' test_expect_success 'drop middle stash' ' @@ -118,17 +128,24 @@ test_expect_success 'drop middle stash' ' echo 9 >file && git stash && git stash drop stash@{1} && - test 2 = $(git stash list | wc -l) && + git stash list >output && + test_line_count = 2 output && git stash apply && test 9 = $(cat file) && - test 1 = $(git show :file) && - test 1 = $(git show HEAD:file) && + echo 1 >expect && + git show :file >actual && + test_cmp expect actual && + git show HEAD:file >actual && + test_cmp expect actual && git reset --hard && git stash drop && git stash apply && test 3 = $(cat file) && - test 1 = $(git show :file) && - test 1 = $(git show HEAD:file) + echo 1 >expect && + git show :file >actual && + test_cmp expect actual && + git show HEAD:file >actual && + test_cmp expect actual ' test_expect_success 'drop middle stash by index' ' @@ -138,26 +155,37 @@ test_expect_success 'drop middle stash by index' ' echo 9 >file && git stash && git stash drop 1 && - test 2 = $(git stash list | wc -l) && + git stash list >output && + test_line_count = 2 output && git stash apply && test 9 = $(cat file) && - test 1 = $(git show :file) && - test 1 = $(git show HEAD:file) && + echo 1 >expect && + git show :file >actual && + test_cmp expect actual && + git show HEAD:file >actual && + test_cmp expect actual && git reset --hard && git stash drop && git stash apply && test 3 = $(cat file) && - test 1 = $(git show :file) && - test 1 = $(git show HEAD:file) + echo 1 >expect && + git show :file >actual && + test_cmp expect actual && + git show HEAD:file >actual && + test_cmp expect actual ' test_expect_success 'stash pop' ' git reset --hard && git stash pop && test 3 = $(cat file) && - test 1 = $(git show :file) && - test 1 = $(git show HEAD:file) && - test 0 = $(git stash list | wc -l) + echo 1 >expect && + git show :file >actual && + test_cmp expect actual && + git show HEAD:file >actual && + test_cmp expect actual && + git stash list >output && + test_must_be_empty output ' cat >expect <<EOF @@ -207,8 +235,10 @@ test_expect_success 'stash branch' ' echo baz >file && git commit file -m second && git stash branch stashbranch && - test refs/heads/stashbranch = $(git symbolic-ref HEAD) && - test $(git rev-parse HEAD) = $(git rev-parse master^) && + echo "refs/heads/stashbranch" >expect3 && + git symbolic-ref HEAD >actual && + test_cmp expect3 actual && + test_cmp_rev HEAD master^ && git diff --cached >output && test_cmp expect output && git diff >output && @@ -217,7 +247,8 @@ test_expect_success 'stash branch' ' git commit -m alternate\ second && git diff master..stashbranch >output && test_cmp output expect2 && - test 0 = $(git stash list | wc -l) + git stash list >output && + test_must_be_empty output ' test_expect_success 'apply -q is quiet' ' @@ -242,7 +273,9 @@ test_expect_success 'pop -q --index works and is quiet' ' git add file && git stash save --quiet && git stash pop -q --index >output.out 2>&1 && - test foo = "$(git show :file)" && + echo foo >expect && + git show :file >actual && + test_cmp expect actual && test_must_be_empty output.out ' @@ -500,7 +533,8 @@ test_expect_success 'stash branch - no stashes on stack, stash-like argument' ' git stash branch stash-branch ${STASH_ID} && test_when_finished "git reset --hard HEAD && git checkout master && git branch -D stash-branch" && - test $(git ls-files --modified | wc -l) -eq 1 + git ls-files --modified >output && + test_line_count = 1 output ' test_expect_success 'stash branch - stashes on stack, stash-like argument' ' @@ -516,7 +550,8 @@ test_expect_success 'stash branch - stashes on stack, stash-like argument' ' git stash branch stash-branch ${STASH_ID} && test_when_finished "git reset --hard HEAD && git checkout master && git branch -D stash-branch" && - test $(git ls-files --modified | wc -l) -eq 1 + git ls-files --modified >actual && + test_line_count = 1 actual ' test_expect_success 'stash branch complains with no arguments' ' @@ -638,7 +673,8 @@ test_expect_success 'drop: fail early if specified stash is not a stash ref' ' git stash && echo bar >file && git stash && - test_must_fail git stash drop $(git rev-parse stash@{0}) && + stash=$(git rev-parse stash@{0}) && + test_must_fail git stash drop $stash && git stash pop && test bar = "$(cat file)" && git reset --hard HEAD @@ -652,7 +688,8 @@ test_expect_success 'pop: fail early if specified stash is not a stash ref' ' git stash && echo bar >file && git stash && - test_must_fail git stash pop $(git rev-parse stash@{0}) && + stash=$(git rev-parse stash@{0}) && + test_must_fail git stash pop $stash && git stash pop && test bar = "$(cat file)" && git reset --hard HEAD @@ -789,7 +826,7 @@ test_expect_success 'stash where working directory contains "HEAD" file' ' git stash && git diff-files --quiet && git diff-index --cached --quiet HEAD && - test "$(git rev-parse stash^)" = "$(git rev-parse HEAD)" && + test_cmp_rev stash^ HEAD && git diff stash^..stash >output && test_cmp expect output ' @@ -807,7 +844,9 @@ test_expect_success 'store updates stash ref and reflog' ' git reset --hard && test_path_is_missing bazzy && git stash store -m quuxery $STASH_ID && - test $(git rev-parse stash) = $STASH_ID && + echo $STASH_ID >expect && + git rev-parse stash >actual && + test_cmp expect actual && git reflog --format=%H stash| grep $STASH_ID && git stash pop && grep quux bazzy -- 2.24.0.155.gd9f6f3b619 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v2 2/2] stash: make sure we have a valid index before writing it 2019-11-13 11:17 ` [PATCH v2 1/2] t3903: avoid git commands inside command substitution Thomas Gummerer @ 2019-11-13 11:17 ` Thomas Gummerer 0 siblings, 0 replies; 13+ messages in thread From: Thomas Gummerer @ 2019-11-13 11:17 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Grzegorz Rajchman, Thomas Gummerer In 'do_apply_stash()' we refresh the index in the end. Since 34933d0eff ("stash: make sure to write refreshed cache", 2019-09-11), we also write that refreshed index when --quiet is given to 'git stash apply'. However if '--index' is not given to 'git stash apply', we also discard the index in the else clause just before. We need to do so because we use an external 'git update-index --add --stdin', which leads to an out of date in-core index. Later we call 'refresh_and_write_cache', which now leads to writing the discarded index, which means we essentially write an empty index file. This is obviously not correct, or the behaviour the user wanted. We should not modify the users index without being asked to do so. Make sure to re-read the index after discarding the current in-core index, to avoid dealing with outdated information. Instead we could also drop the 'discard_cache()' + 'read_cache()', however that would make it easy to fall into the same trap as 34933d0eff did, so it's better to avoid that. We can also drop the 'refresh_and_write_cache' completely in the quiet case. Previously in legacy stash we relied on 'git status' to refresh the index after calling 'git read-tree' when '--index' was passed to 'git apply'. However the 'reset_tree()' call that replaced 'git read-tree' always passes options that are equivalent to '-m', making the refresh of the index unnecessary. Reported-by: Grzegorz Rajchman <rayman17@gmail.com> Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com> --- builtin/stash.c | 7 +++---- t/t3903-stash.sh | 7 ++++++- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/builtin/stash.c b/builtin/stash.c index ab30d1e920..372fbdb7ac 100644 --- a/builtin/stash.c +++ b/builtin/stash.c @@ -481,13 +481,12 @@ static int do_apply_stash(const char *prefix, struct stash_info *info, if (ret) return -1; + /* read back the result of update_index() back from the disk */ discard_cache(); + read_cache(); } - if (quiet) { - if (refresh_and_write_cache(REFRESH_QUIET, 0, 0)) - warning("could not refresh index"); - } else { + if (!quiet) { struct child_process cp = CHILD_PROCESS_INIT; /* diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh index db7cc6e664..0157771e24 100755 --- a/t/t3903-stash.sh +++ b/t/t3903-stash.sh @@ -263,8 +263,11 @@ test_expect_success 'save -q is quiet' ' test_must_be_empty output.out ' -test_expect_success 'pop -q is quiet' ' +test_expect_success 'pop -q works and is quiet' ' git stash pop -q >output.out 2>&1 && + echo bar >expect && + git show :file >actual && + test_cmp expect actual && test_must_be_empty output.out ' @@ -273,6 +276,8 @@ test_expect_success 'pop -q --index works and is quiet' ' git add file && git stash save --quiet && git stash pop -q --index >output.out 2>&1 && + git diff-files file2 >file2.diff && + test_must_be_empty file2.diff && echo foo >expect && git show :file >actual && test_cmp expect actual && -- 2.24.0.155.gd9f6f3b619 ^ permalink raw reply related [flat|nested] 13+ messages in thread
end of thread, other threads:[~2019-11-14 2:07 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-11-07 10:36 [BUG] git stash pop --quiet deletes files in git 2.24.0 Grzegorz Rajchman 2019-11-07 18:49 ` Thomas Gummerer 2019-11-08 2:32 ` Junio C Hamano 2019-11-08 16:59 ` Thomas Gummerer 2019-11-10 6:11 ` Junio C Hamano 2019-11-11 19:56 ` Thomas Gummerer 2019-11-12 5:21 ` Junio C Hamano 2019-11-13 11:15 ` Thomas Gummerer 2019-11-13 13:31 ` Junio C Hamano 2019-11-13 15:01 ` [PATCH v3] stash: make sure we have a valid index before writing it Thomas Gummerer 2019-11-14 2:07 ` Junio C Hamano 2019-11-13 11:17 ` [PATCH v2 1/2] t3903: avoid git commands inside command substitution Thomas Gummerer 2019-11-13 11:17 ` [PATCH v2 2/2] stash: make sure we have a valid index before writing it Thomas Gummerer
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 NNTP newsgroup(s).