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

From: Elijah Newren <newren@gmail.com>
Date: Jan 20, 2021, 17:21 UTC
Message-ID: <CABPp-BFiNvPaAffQ0uBfpkHY1WM2gR3ssvpwzxpDhrZmZZjiDA@mail.gmail.com>
In-Reply-To: <0bccfd34ae5924aef0432fd6727debb75c052da5.1611161639.git.gitgitgadget@gmail.com>

On Wed, Jan 20, 2021 at 8:54 AM Derrick Stolee via GitGitGadget <gitgitgadget@gmail.com> wrote:

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

Show 6 quoted lines

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

Show 6 quoted lines

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

Show 5 quoted lines

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