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

[Junio C Hamano <gitster@pobox.com>]

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

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

> 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;

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