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

[Junio C Hamano <gitster@pobox.com>]

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.