[BUG] builtin "stash apply" does not refresh index

[BUG] builtin "stash apply" does not refresh index

[Jeff King]

Try this:

  # a repo with one file
  git init repo
  cd repo
  echo base >file
  git add file
  git commit -m base

  # now stash a modification
  echo modified >file
  git stash

  # then make the file stat dirty. This will change inode; on systems
  # without inodes, probably "sleep 1; touch file" would work.
  cp file other
  mv other file

  # now try to apply the stash
  git stash apply

The final command gives me:

  error: Your local changes to the following files would be overwritten by merge:
	file
  Please commit your changes or stash them before you merge.
  Aborting

This started with 8a0fc8d19d (stash: convert apply to builtin,
2019-02-25), which is in v2.22.0. Interestingly, do_stash_apply() does
in fact call refresh_cache(). But it looks like we don't ever write it
out to disk. So when we later call merge_recursive(), it presumably uses
the on-disk index, not what we have in memory.

It's not clear to me where the fix should go, though. Should "stash
apply" be writing out the refreshed index before proceeding? Or should
merge_recursive() be more careful about refreshing the index after it
locks it? The former is what happened with stash as a shell script, but
the latter would save us an otherwise pointless write.

I'm also not sure if other parts of stash might have a similar bug due
to the conversion to C. I think it's an easy mistake when converting
"git update-index --refresh" into "refresh_cache()" and then some other
part of the code takes the index lock (and thus looks at that index, not
what we have in memory).

-Peff

Re: [BUG] builtin "stash apply" does not refresh index

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> This started with 8a0fc8d19d (stash: convert apply to builtin,
> 2019-02-25), which is in v2.22.0. Interestingly, do_stash_apply() does
> in fact call refresh_cache(). But it looks like we don't ever write it
> out to disk. So when we later call merge_recursive(), it presumably uses
> the on-disk index, not what we have in memory.

Ouch.

> It's not clear to me where the fix should go, though. Should
> "stash apply" be writing out the refreshed index before
> proceeding? Or should merge_recursive() be more careful about
> refreshing the index after it locks it?  The former is what
> happened with stash as a shell script, but the latter would save
> us an otherwise pointless write.

I wonder if the third solution be possible.  "I am a new caller of
merge-recursive machinery, I did some operations on the working tree
and the index (in core), and while I would, in the old world order,
write out the in-core index to on-disk and call the merge-recursive
machinery, expecing the callee to read the on-disk index, for the
sake of performance, I am using this new calling convention, where a
new entry point of merge-recursivey machinery allows me to call it
and tell it to use the in-core index."  For that to happen, the
codepath in the merge-recursive machinery entered from the new entry
point needs to learn not only omit a call to read_cache() but also
it needs to write in-core index out lazily for the caller (e.g.
imagine it needs to exit early without doing any of the merge?  The
caller skipped the "otherwise pointless write", which needs to now
happen before the whole thing exits).

And before the third one, introduction of a new entry point that
makes merge-recursive machinery inherit the already populated
in-core index, happens, I think the right solution is to write the
in-core index out---the write is not pointless.