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

From: Derrick Stolee <stolee@gmail.com>
Date: Mar 29, 2026, 14:57 UTC
Message-ID: <d1afbb2c-84d2-45da-ade5-c86397bd24de@gmail.com>
In-Reply-To: <xmqqy0jbhkch.fsf@gitster.g>

On 3/28/2026 2:36 PM, Junio C Hamano wrote:

> "Jayesh Daga via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 24 quoted lines

>> -	/*
>> -	 * 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