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

[Elijah Newren <newren@gmail.com>]

On Wed, Jan 20, 2021 at 8:54 AM Derrick Stolee via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> 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.
>
> Also drop local variables that are used exactly once and can be found
> directly from the 'istate' parameter.
>
> 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);

All mechanical changes so far; these look obviously correct.

>
>         if (i)
>                 return i;
> +
> +       if (!istate->cache_tree)
> +               istate->cache_tree = cache_tree();

This is the only substantive change.  It seems fairly innocuous, but
it makes me wonder the reasoning...I don't know/remember enough about
cache_tree handling to know when this would or wouldn't have already
been allocated.  It seems that this would have had to segfault below
if istate->cache_tree were ever NULL, and I don't see you mentioning
any bug you are fixing, so I presume this means you are going to be
adding new codepaths somewhere that cause this function to be reached
under different circumstances than previously had been and you need it
to be more safe for those.  Is that correct?  Or is it just an
abundance of caution thing that you're adding?  If the latter, any
reason you chose to allocate one rather than assume it's a violation
of design invariants and BUG() instead?  (Perhaps the commit message
could add a sentence about the rationale for the extra safety?)

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

Another mechanical update; looks good.

>         trace2_region_leave("cache_tree", "update", the_repository);
>         trace_performance_leave("cache_tree_update");
>         if (i < 0)
> --
> gitgitgadget