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

From: Derrick Stolee <[email-masked]>
Date: Wed, 20 Jan 2021 14:10:30 -0500

On 1/20/2021 12:21 PM, Elijah Newren wrote:
> On Wed, Jan 20, 2021 at 8:54 AM Derrick Stolee via GitGitGadget
> <[email-masked]> wrote:...
>> +
>> +       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?)

It's something I need in the future when I use the cache_tree_update()
in more places. I think I call it two times, and either I need to
initialize the cache_tree member outside of both, or just make it a
feature of the method that it will re-initialize the cache-tree.

Note: the implementation treats an initialized, but empty cache-tree
as "invalid" so update_one() correctly populates the full tree.

Thanks,
-Stolee