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

Thomas Gummerer <t.gummerer@gmail.com> writes:

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.