From: Derrick Stolee <stolee@gmail.com>
Date: Fri, 27 Mar 2026 13:48:55 GMT
Subject: Re: [PATCH] read-cache: use index state repository for trace2 logging
Message-ID: <770465fe-c38f-45a9-b1b0-0ad682a35fab@gmail.com>
In-Reply-To: <pull.2253.git.git.1774606086325.gitgitgadget@gmail.com>

On 3/27/2026 6:08 AM, Jayesh Daga via GitGitGadget wrote:

>     Robustness: The ternary fallback ensures we avoid potential NULL pointer
>     dereferences while maintaining existing logging behavior in edge cases.

> +	r = istate->repo ? istate->repo : the_repository;

If I understand correctly, it is a bug if istate->repo is NULL.

Did you try running the test suite with istate->repo as a replacement for
the_repository in these tracing calls? Is there a legitimate scenario
where this would be NULL?

> +	trace2_data_intmax("index", r, "read/version",
>  			   istate->version);
> -	trace2_data_intmax("index", the_repository, "read/cache_nr",
> +	trace2_data_intmax("index", r, "read/cache_nr",
>  			   istate->cache_nr);

Other than that, this is a minor improvement in the right direction. I'd
rather that it be more complete if you are working in this file.

There are several places where we use the_repository and there is an
'istate' right there. If this change works, then why not apply that same
transformation in the other places?

There are TODO comments for many of these, including the hunk you are
editing (be sure to remove these!).

There are other cases that I see in this file:

* refresh_index() uses the_repository for progress.

* tweak_untracked_cache() has a local pointer 'r' that could
  be set to istate->repo

* tweak_split_index() passes the_repository to
  repo_config_get_split_index().

* do_read_index() uses the_repository when it could use
  istate->repo.

* read_index_from() uses the_repository for tracing.

* verify_index_from() uses the_repository->hash_algo but
  has an istate->repo that could be used.

* do_write_index() has several instances.

(At this point, I stopped taking inventory.)

There are other uses of the_repository that will be harder to remove as
there isn't a local istate at the time, so those aren't worth combining
with this kind of effort.

If you are already working in this space, then I recommend figuring out
how much we can rely on istate->repo and then apply that knowledge to
these cases as separate commits:

1. Replace the uses in the trace2 calls with istate->repo
   and delete the TODO comments.

2. Replace the other uses of the_repository when an istate
   exists already.

Thanks, -Stolee