Re: [PATCH v2 1/8] cache-tree: clean up cache_tree_update()

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 22, 2021, 19:11 UTC
Message-ID: <xmqqlfck948f.fsf@gitster.c.googlers.com>
In-Reply-To: <f9dccaed0acae91b06f5f9ccaf0f9116b65f3f09.1611320639.git.gitgitgadget@gmail.com>

"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 6 quoted lines

> From: Derrick Stolee <dstolee@microsoft.com>
>
> Make the method safer by allocating a cache_tree member for the given
> index_state if it is not already present. This is preferrable to a
> BUG() statement or returning with an error because future callers will
> want to populate an empty cache-tree using this method.

How do the current callers prepare the istate to be passed to this function? The implications of this question obviously are:

 - why is it unreasonable for future callers to follow suit?  or

 - now the callers are no longer responsible to give an empty
   cache_tree to istate before calling this function, shouldn't
   existing callers lose code that is no longer needed?

I would imagine that the first is easily answered with "but that would be more code on the callers' side", but then the second one becomes even more relevant, no?

> Also drop local variables that are used exactly once and can be found
> directly from the 'istate' parameter.

It actuall is used twice, no?

I do find it an improvement because it makes it clearer that istate->{cache,cache_nr} comes in pairs.

I wonder if verify_cache() wants to take istate as a parameter, replacing the first two? update_one() shifts where it starts working in the array and reduces the number of entries as it digs deeper, so it still has to keep taking the (base, nr) pair (iow, its second and third parameters cannot be replaced with an istate).

Show 35 quoted lines

> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
>  cache-tree.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/cache-tree.c b/cache-tree.c
> index 3f1a8d4f1b7..c1e49901c17 100644
> --- a/cache-tree.c
> +++ b/cache-tree.c
> @@ -436,16 +436,20 @@ static int update_one(struct cache_tree *it,
>  
>  int cache_tree_update(struct index_state *istate, int flags)
>  {
> -	struct cache_tree *it = istate->cache_tree;
> -	struct cache_entry **cache = istate->cache;
> -	int entries = istate->cache_nr;
> -	int skip, i = verify_cache(cache, entries, flags);
> +	int skip, i;
> +
> +	i = verify_cache(istate->cache, istate->cache_nr, flags);
>  
>  	if (i)
>  		return i;
> +
> +	if (!istate->cache_tree)
> +		istate->cache_tree = cache_tree();
> +
>  	trace_performance_enter();
>  	trace2_region_enter("cache_tree", "update", the_repository);
> -	i = update_one(it, cache, entries, "", 0, &skip, flags);
> +	i = update_one(istate->cache_tree, istate->cache, istate->cache_nr,
> +		       "", 0, &skip, flags);
>  	trace2_region_leave("cache_tree", "update", the_repository);
>  	trace_performance_leave("cache_tree_update");
>  	if (i < 0)