[BUG] git stash pop --quiet deletes files in git 2.24.0

[BUG] git stash pop --quiet deletes files in git 2.24.0

[Grzegorz Rajchman]

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.

Re: [BUG] git stash pop --quiet deletes files in git 2.24.0

[Thomas Gummerer]

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.

Re: [BUG] git stash pop --quiet deletes files in git 2.24.0

[Junio C Hamano]

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;

Re: [BUG] git stash pop --quiet deletes files in git 2.24.0

[Thomas Gummerer]

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;

Re: [BUG] git stash pop --quiet deletes files in git 2.24.0

[Junio C Hamano]

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.

Re: [BUG] git stash pop --quiet deletes files in git 2.24.0

[Thomas Gummerer]

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

Re: [BUG] git stash pop --quiet deletes files in git 2.24.0

[Junio C Hamano]

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.

Re: [BUG] git stash pop --quiet deletes files in git 2.24.0

[Thomas Gummerer]

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.

Re: [BUG] git stash pop --quiet deletes files in git 2.24.0

[Junio C Hamano]

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.

[PATCH v3] stash: make sure we have a valid index before writing it

[Thomas Gummerer]

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

Re: [PATCH v3] stash: make sure we have a valid index before writing it

[Junio C Hamano]

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
>  '

[PATCH v2 1/2] t3903: avoid git commands inside command substitution

[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

[PATCH v2 2/2] stash: make sure we have a valid index before writing it

[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