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

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 23, 2019, 16:56 UTC
Message-ID: <xmqqa7bzn7le.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20190823052737.GA10592@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 5 quoted lines

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

Show 6 quoted lines

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