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

[Thomas Gummerer <t.gummerer@gmail.com>]

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;