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

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 28, 2026, 18:36 UTC
Message-ID: <xmqqy0jbhkch.fsf@gitster.g>
In-Reply-To: <pull.2253.v2.git.git.1774682046750.gitgitgadget@gmail.com>

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

Show 5 quoted lines

> From: Jayesh Daga <jayeshdaga99@gmail.com>
>
> trace2_data_intmax() calls in do_read_index() currently use the
> global 'the_repository' instance, even though the index_state
> already carries an explicit repository pointer (istate->repo).

Drop "currently". The canonical order in log messages in this project is to describe the current state of the relevant part of the codebase in present tense, pulling attention of readers to what the author finds problematic, and propose a solution. And finally give orders to somebody sitting in front of the keyboard to make the change.

> index_state instances are initialized via INDEX_STATE_INIT(r),
> which sets istate->repo. Using the_repository here is therefore
> redundant and obscures the actual repository context associated
> with the index being read.

Even worse, it could do a wrong thing, so saying "redundant" here somewhat misses the point.

> In particular, using the global repository can lead to misleading
> trace2 output in scenarios where multiple repository instances are
> in use (such as tests or future refactoring toward better library
> boundaries), as events may be attributed to the wrong repository.

Exactly.

Show 22 quoted lines

> diff --git a/read-cache.c b/read-cache.c
> index 5049f9baca..46ffb49cab 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -2206,6 +2206,7 @@ int do_read_index(struct index_state *istate, const char *path, int must_exist)
>  	size_t extension_offset = 0;
>  	int nr_threads, cpus;
>  	struct index_entry_offset_table *ieot = NULL;
> +	struct repository *r;
>  
>  	if (istate->initialized)
>  		return istate->cache_nr;
> @@ -2309,13 +2310,12 @@ int do_read_index(struct index_state *istate, const char *path, int must_exist)
>  	}
>  	munmap((void *)mmap, mmap_size);
>  
> -	/*
> -	 * 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;

Show 7 quoted lines

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

Thanks.