From: Jayesh Daga via GitGitGadget Date: Sat, 28 Mar 2026 07:14:06 GMT Subject: [PATCH v2] read-cache: use istate->repo for trace2 logging Message-ID: In-Reply-To: From: Jayesh Daga 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 --- [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 +Author: Jayesh Daga ## 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 + 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 ## 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