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

["Jayesh Daga via GitGitGadget" <gitgitgadget@gmail.com>]

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

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.

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.

Switch these calls to use istate->repo directly, making the
association between the index and its repository explicit.

Since istate->repo is expected to be non-NULL, enforce this
assumption with a BUG() check so that any violation of this
invariant is caught early.

Also remove the now-obsolete TODO comment.

Signed-off-by: Jayesh Daga <jayeshdaga99@gmail.com>
---
    [GSoC] read-cache: use index state repository for trace2 logging
    
    
    HIGH LEVEL
    ==========
    
    trace2_data_intmax() calls in read-cache.c currently use the global
    'the_repository' instance, even though index_state already carries an
    explicit repository pointer (istate->repo).
    
    Use istate->repo directly in these calls, making the repository context
    associated with the index explicit.
    
    
    Low-level (Implementation & Justification)
    ==========================================
    
    index_state instances are initialized via INDEX_STATE_INIT(r), which
    sets istate->repo. Therefore, istate->repo is expected to be non-NULL in
    normal code paths.
    
    Using the global 'the_repository' in this context is redundant and can
    obscure the actual repository associated with the index being read.
    
    In particular, relying on the global repository may lead to misleading
    trace2 output in scenarios where multiple repository instances are in
    use (e.g., tests or future refactoring toward improved library
    boundaries), as events could be attributed to the wrong repository.
    
    Changes:
    
     * Replace uses of the_repository in trace2_data_intmax() with
       istate->repo
     * Enforce the expectation that istate->repo is non-NULL via BUG()
     * Remove the now-obsolete TODO comment
    
    Example:
    
     * trace2_data_intmax("index", the_repository, "read/version", ...)
    
     * trace2_data_intmax("index", istate->repo, "read/version", ...)
    
    
    Summary
    =======
    
    Use the repository associated with index_state for trace2 logging
    instead of the global repository instance, making the repository context
    explicit and consistent with existing initialization guarantees.
    
    
    Changes since v1:
    =================
    
     * Dropped fallback to the_repository; rely on istate->repo invariant
     * Added BUG() check for NULL istate->repo
     * Removed TODO comment
     * Improved commit message with detailed rationale
     * Cleaned up duplicate commits and fixed history

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2253%2Fjayesh0104%2Ftrace2-istate-repo-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2253/jayesh0104/trace2-istate-repo-v2
Pull-Request: https://github.com/git/git/pull/2253

Range-diff vs v1:

 1:  ba6d2add1d ! 1:  7fdfaaef9b read-cache: use index state repository for trace2 logging
     @@
       ## Metadata ##
     -Author: jayesh0104 <jayeshdaga99@gmail.com>
     +Author: Jayesh Daga <jayeshdaga99@gmail.com>
      
       ## Commit message ##
     -    read-cache: use index state repository for trace2 logging
     +    read-cache: use istate->repo for trace2 logging
      
     -    Replace uses of the_repository in trace2_data_intmax() with
     -    istate->repo, which represents the repository associated with
     -    the index state.
     +    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).
      
     -    This avoids relying on global repository state and aligns with
     -    other parts of the codebase (e.g., sparse-index.c) that pass the
     -    repository instance explicitly.
     +    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.
      
     -    No functional change intended.
     +    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.
      
     -    Signed-off-by: jayesh0104 <jayeshdaga99@gmail.com>
     +    Switch these calls to use istate->repo directly, making the
     +    association between the index and its repository explicit.
     +
     +    Since istate->repo is expected to be non-NULL, enforce this
     +    assumption with a BUG() check so that any violation of this
     +    invariant is caught early.
     +
     +    Also remove the now-obsolete TODO comment.
     +
     +    Signed-off-by: Jayesh Daga <jayeshdaga99@gmail.com>
      
       ## read-cache.c ##
      @@ read-cache.c: int do_read_index(struct index_state *istate, const char *path, int must_exist)
     @@ read-cache.c: int do_read_index(struct index_state *istate, const char *path, in
       	if (istate->initialized)
       		return istate->cache_nr;
      @@ read-cache.c: int do_read_index(struct index_state *istate, const char *path, int must_exist)
     - 	 * TODO trace2: replace "the_repository" with the actual repo instance
     - 	 * that is associated with the given "istate".
     - 	 */
     + 	}
     + 	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 ? istate->repo : the_repository;
     ++	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",


 read-cache.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

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;
+	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);
 
 	/*

base-commit: ca1db8a0f7dc0dbea892e99f5b37c5fe5861be71
-- 
gitgitgadget