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:

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