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

On 1/20/2021 12:21 PM, Elijah Newren wrote:

On Wed, Jan 20, 2021 at 8:54 AM Derrick Stolee via GitGitGadget
[off-list ref] wrote:...

quoted

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