Re: [PATCH v3 6/8] reset: make sparse-aware (except --mixed)

[Junio C Hamano <gitster@pobox.com>]

Phillip Wood <phillip.wood123@gmail.com> writes:

> On 08/10/2021 19:31, Junio C Hamano wrote:
>> Victoria Dye <vdye@github.com> writes:
>> 
>>> Phillip Wood wrote:
>> 
>>>> I was looking at the callers to prime_cache_tree() this morning
>>>> and would like to suggest an alternative approach - just delete
>>>> prime_cache_tree() and all of its callers!
>> Do you mean the calls added by new patches without understanding
>> what they are doing, or all calls to it?
>
> I mean all calls to prime_cache_tree() after having understood (or at
> least thinking that I understand) what they are doing.

Sorry, my statement was confusingly written.  I meant "calls added
by new patches, written by those who do not understand what
prime_cache_tree() calls are doing", but after re-reading it, I
think it could be taken to be referring to "you may be commenting
without understanding what prime_cache_tree() calls are doing",
which wasn't my intention.

> (a) a successful call to unpack_trees() updates the cache tree
>
> (b) all the existing calls to prime_cache_tree() follow a successful
> call to unpack_trees() and nothing touches in index in between the
> call to unpack_trees() and prime_cache_tree().

Ahh, OK.

I think we originally avoided calling cache_tree_update() lightly
(because it is essentially a "write-tree", a fairly heavy-weight
operation, without I/O) and instead relied on prime_cache_tree() to
get degraded cache-tree back into freshness.

What I forgot was that 52fca218 (unpack-trees: populate cache-tree
on successful merge, 2015-07-28) added cache_tree_update() there at
the end of unpack_trees().  The commit covers quite a wide range of
operations---the log message says "merge", but in fact anything that
uses unpack_trees() including branch switching and the resetting of
the index are affected, and they cause a full reconstruction of the
cache tree by calling cache_tree_update().

For most callers of prime_cache_tree(), like the ones in "git
read-tree" and "git reset", it is immediately obvious that we just
read from the same tree, and we should have everything from the tree
and nothing else in the resulting index, so it is clear that the
prime_cache_tree() call is recreating the same cache-tree
information that we already should have computed ourselves, and
these calls can go (or if "prime" is still cheaper than "update",
these callers can pass an option to tell unpack_trees() to skip the
cache_tree_update() call, because they will call "prime" immediately
after).

For other callers it is not immediately obvious, but I trust you are
correctly reading the code ;-)

Thanks.