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

Commit Message

Derrick Stolee Jan. 20, 2021, 4:53 p.m. UTC

From: Derrick Stolee <[e-mail]>

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 <[e-mail]>
---
 cache-tree.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

Comments

Elijah Newren Jan. 20, 2021, 5:21 p.m. UTC | #1

On Wed, Jan 20, 2021 at 8:54 AM Derrick Stolee via GitGitGadget
<[e-mail]> wrote:
>
> From: Derrick Stolee <[e-mail]>
>
> 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 <[e-mail]>
> ---
>  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

Derrick Stolee Jan. 20, 2021, 7:10 p.m. UTC | #2

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

Patch

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)