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

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Oct 12, 2021, 10:16 UTC
Message-ID: <cd62bf13-fdf1-af00-cc25-ef20abcfe152@gmail.com>
In-Reply-To: <xmqqlf30edvf.fsf@gitster.g>

On 10/10/2021 23:03, Junio C Hamano wrote:

Show 22 quoted lines

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

Thanks for clarifying that, I had misunderstood what you had written.

Show 31 quoted lines

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

I haven't really thought this through but could we teach unpack_trees() to call prime_cache_tree() rather than cache_tree_update() when that would be safe? For callers that use oneway_merge() merge it should always be safe I think and it might be possible to modify twoway_merge() to signal if the final tree in the index matches the second one passed to it. We could have a more general mechanism for the callback to signal if it is safe to prime the tree but I suspect the callers that are using custom callbacks are not updating the whole tree.

Best Wishes

Phillip

Show 7 quoted lines

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