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

From: Junio C Hamano <gitster@pobox.com>
To: "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, newren@gmail.com,
	Derrick Stolee <stolee@gmail.com>,
	Derrick Stolee <derrickstolee@github.com>,
	Derrick Stolee <dstolee@microsoft.com>
Subject: Re: [PATCH v2 1/8] cache-tree: clean up cache_tree_update()
Date: Fri, 22 Jan 2021 11:11:12 -0800	[thread overview]
Message-ID: <xmqqlfck948f.fsf@gitster.c.googlers.com> (raw)
In-Reply-To: <f9dccaed0acae91b06f5f9ccaf0f9116b65f3f09.1611320639.git.gitgitgadget@gmail.com> (Derrick Stolee via GitGitGadget's message of "Fri, 22 Jan 2021 13:03:52 +0000")

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

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

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