Re: [PATCH v2] read-cache: use istate->repo for trace2 logging

[Derrick Stolee <stolee@gmail.com>]

On 3/28/2026 2:36 PM, Junio C Hamano wrote:
> "Jayesh Daga via GitGitGadget" <gitgitgadget@gmail.com> writes:

>> -	/*
>> -	 * TODO trace2: replace "the_repository" with the actual repo instance
>> -	 * that is associated with the given "istate".
>> -	 */
>> -	trace2_data_intmax("index", the_repository, "read/version",
>> +	r=istate->repo;
> 
> Have SP on both sides of an assignment operator '=', i.e.
> 
> 	r = istate->repo;
> 
>> +	if (!r)
>> +	    BUG("istate->repo is NULL in do_read_index");
>> +	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);
> 
> Or you can do without an intermediate variable 'r'.  Replacing
> "the_repository" with "istate->repo" would make the resulting line
> shorter already, and more importantly, readers do not have to
> remember that 'r' is an alias for 'istate->repo' while reading the
> code.

And I don't think we need these BUG() statements, so I'm sorry if
that was inferred from my statement. This kind of bug isn't
something that a developer will stumble upon by accidentally
calling the method incorrectly, but instead would be a substantial
break of the index_state structure. A segfault would be enough to
catch this in testing.

Have you thought about applying this pattern to the rest of the
trace2 statements in this file? Or did you want review to solidify
on this section first?

Thanks,
-Stolee