read-cache: use index state repository for trace2 logging

From: jayesh0104 <[email protected]>

Replace uses of the_repository in trace2_data_intmax() with istate->repo, which represents the repository associated with the index state.

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.

No functional change intended.

---
[GSoC] read-cache: use index state repository for trace2 logging

HIGH LEVEL
==========

The current implementation of trace2_data_intmax() in read-cache.c relies on the global the_repository instance.

As part of the ongoing effort to "lib-ify" the Git codebase and reduce dependence on global state, this patch transitions those calls to use the repository instance associated with the index_state.

Low-level (Implementation & Justification)
==========================================

In read-cache.c, the index_state (istate) typically carries a pointer to its associated repository. However, because istate->repo is not guaranteed to be initialized in all code paths (e.g., certain low-level utility or testing contexts), this patch implements a defensive fallback pattern.

Changes:

Introduced a local repository pointer r that prefers istate->repo but falls back to the_repository if the former is NULL.

Updated trace2_data_intmax() calls to use this context-aware pointer.

+ struct repository *r = istate->repo ? istate->repo : the_repository;
- trace2_data_intmax("index", the_repository, "read/version", istate->version);
+ trace2_data_intmax("index", r, "read/version", istate->version);

Benefits:

Thread Safety & Modernization: Aligns with the project's goal of moving away from the_repository.

Robustness: The ternary fallback ensures we avoid potential NULL pointer dereferences while maintaining existing logging behavior in edge cases.

Consistency: Follows patterns seen in other modernized areas of the codebase.

Summary
=======

Transitioned trace2 logging in read-cache.c from global to local repository context.

Implemented a safety fallback to the_repository to handle uninitialized istate->repo pointers.

No functional changes to telemetry output are intended.

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

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

index 5049f9baca..2c5c5165e0 100644
@@ -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;

@@ -2313,9 +2314,10 @@ 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".
 	 */
-	trace2_data_intmax("index", the_repository, "read/version",
+	r = istate->repo ? istate->repo : the_repository;
+	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

Because INDEX_STATE_INIT(r) assigns the repository as the first thing, I tend to agree. A (bare) repository can lack the index so repo->index might be NULL, but if you have an istate instance, it should always know which repository it came from.

;-) Long timers always aim higher than posted patches.

Excellent suggestion.
Thanks.

By the way, Jeyesh, do you really want to be known with a numbered "jayesh0104" as your name? These author identities are cast in stone in commit objects and will stay with the project.

Also see Documentation/SubmittingPatches::[dco,real-name].

Hi Junio, Derrick,

Thanks for the detailed review and suggestions.

On the fallback to the_repository: I agree with your observation that istate->repo being NULL would indicate a bug rather than a scenario to defensively handle. My initial intent was to be conservative in case there were edge paths where istate->repo might not be initialized, but given that INDEX_STATE_INIT(r) sets this unconditionally, it makes sense to rely on that invariant instead of masking potential issues. I will drop the fallback and use istate->repo directly (and verify via the test suite).

Regarding scope, Derrick’s suggestion to split this into separate commits makes sense. I’ll proceed as follows:

1. A focused patch that replaces the_repository with istate->repo in the trace2 calls within this file and removes the associated TODO comments.

2. A follow-up patch that replaces other uses of the_repository in places where an istate is already available (e.g., refresh_index(), tweak_untracked_cache(), do_write_index(), etc.), keeping changes logically grouped for easier review.

I’ll also run the full test suite after removing the fallback to confirm there are no hidden assumptions.

Junio, thanks also for pointing out the author identity. I’ll update it to use my real name in the next version.

Thanks again for the guidance.

Best,
Jayesh

From: Jayesh Daga <[email protected]>

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.

---
[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 <[email protected]>
+Author: Jayesh Daga <[email protected]>

## 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 <[email protected]>
+ 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 <[email protected]>

## 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(-)

index 5049f9baca..46ffb49cab 100644
@@ -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

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.

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

Exactly.

	r = istate->repo;

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

trace2 calls in read-cache.c use the global 'the_repository', even though the relevant index_state provides an explicit repository pointer via 'istate->repo'.

Using the global repository can result in incorrect trace2 output when multiple repository instances are in use, as events may be attributed to the wrong repository.

Use 'istate->repo' instead in these call sites to ensure correct repository attribution.

Changes since v2:

* Apply the change consistently across read-cache.c
* Drop unnecessary intermediate variable
* Remove obsolete TODO comments
* Update commit message

Jayesh Daga (1):
read-cache: use istate->repo for trace2 logging

jayesh0104 (1):
repo: add paths.git_dir repo info key

 Documentation/git-repo.adoc |  5 +++++
 builtin/repo.c              |  7 +++++++
 read-cache.c                | 32 ++++++++------------------------
 t/t1900-repo-info.sh        | 10 ++++++++++
 4 files changed, 30 insertions(+), 24 deletions(-)

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

Range-diff vs v2:

 -:  ---------- > 1:  5a8165b05d repo: add paths.git_dir repo info key
 1:  7fdfaaef9b ! 2:  9bb6d0fa01 read-cache: use istate->repo for trace2 logging

@@ Metadata
## Commit message ##
read-cache: use istate->repo for trace2 logging

- 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).
+ trace2 calls in read-cache.c use the global 'the_repository',
+ even though the relevant index_state provides an explicit
+ repository pointer via '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.
+ Using the global repository can result in incorrect trace2
+ output when multiple repository instances are in use, as
+ events may be attributed to the wrong repository.

- 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.
+ Use 'istate->repo' instead to ensure correct repository
+ attribution.

## read-cache.c ##
-@@ read-cache.c: 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;
@@ read-cache.c: int do_read_index(struct index_state *istate, const char *path, int must_exist)
}
munmap((void *)mmap, mmap_size);
@@ read-cache.c: int do_read_index(struct index_state *istate, const char *path, in
- * 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",
++ trace2_data_intmax("index", istate->repo, "read/version",
istate->version);
- trace2_data_intmax("index", the_repository, "read/cache_nr",
-+ trace2_data_intmax("index", r, "read/cache_nr",
++ trace2_data_intmax("index", istate->repo, "read/cache_nr",
istate->cache_nr);

/*
+@@ read-cache.c: int read_index_from(struct index_state *istate, const char *path,
+ if (istate->initialized)
+ return istate->cache_nr;
+
+- /*
+- * TODO trace2: replace "the_repository" with the actual repo instance
+- * that is associated with the given "istate".
+- */
+- trace2_region_enter_printf("index", "do_read_index", the_repository,
++ trace2_region_enter_printf("index", "do_read_index", istate->repo,
+ "%s", path);
+ trace_performance_enter();
+ ret = do_read_index(istate, path, 0);
+ trace_performance_leave("read cache %s", path);
+- trace2_region_leave_printf("index", "do_read_index", the_repository,
++ trace2_region_leave_printf("index", "do_read_index", istate->repo,
+ "%s", path);
+
+ split_index = istate->split_index;
+@@ read-cache.c: static int do_write_index(struct index_state *istate, struct tempfile *tempfile,
+ istate->timestamp.nsec = ST_MTIME_NSEC(st);
+ trace_performance_since(start, "write index, changed mask = %x", istate->cache_changed);
+
+- /*
+- * TODO trace2: replace "the_repository" with the actual repo instance
+- * that is associated with the given "istate".
+- */
+- trace2_data_intmax("index", the_repository, "write/version",
++ trace2_data_intmax("index", istate->repo, "write/version",
+ istate->version);
+- trace2_data_intmax("index", the_repository, "write/cache_nr",
++ trace2_data_intmax("index", istate->repo, "write/cache_nr",
+ istate->cache_nr);
+
+ ret = 0;
+@@ read-cache.c: static int do_write_locked_index(struct index_state *istate,
+ return ret;
+ }
+
+- /*
+- * TODO trace2: replace "the_repository" with the actual repo instance
+- * that is associated with the given "istate".
+- */
+- trace2_region_enter_printf("index", "do_write_index", the_repository,
++ trace2_region_enter_printf("index", "do_write_index", istate->repo,
+ "%s", get_lock_file_path(lock));
+ ret = do_write_index(istate, lock->tempfile, write_extensions, flags);
+- trace2_region_leave_printf("index", "do_write_index", the_repository,
++ trace2_region_leave_printf("index", "do_write_index", istate->repo,
+ "%s", get_lock_file_path(lock));
+
+ if (was_full)

gitgitgadget

From: jayesh0104 <[email protected]>

Introduce a new repo info key paths.git_dir to expose the repository's gitdir path, equivalent to git rev-parse --git-dir.

This improves consistency and allows tools to retrieve the gitdir path without invoking external commands.

The implementation adds support in repo.c and integrates it into the repo info reporting mechanism. Documentation is updated to describe the new key, and tests are added to verify that the value matches the output of git rev-parse --git-dir.

---
 Documentation/git-repo.adoc |  5 +++++
 builtin/repo.c              |  7 +++++++
 t/t1900-repo-info.sh        | 10 ++++++++++
 3 files changed, 22 insertions(+)

index 42262c1983..d17d911ec6 100644
@@ -95,6 +95,11 @@ In order to obtain a set of values from `git repo info`, you should provide
 the keys that identify them. Here's a list of the available keys and the
 values that they return:
 
+`paths.git_dir`::
+	The path to the Git directory for the repository (equivalent to
+	`git rev-parse --git-dir`).
+
+
 `layout.bare`::
 	`true` if this is a bare repository, otherwise `false`.
 

index 55f9b9095c..3067107cad 100644
@@ -66,11 +66,18 @@ static int get_references_format(struct repository *repo, struct strbuf *buf)
 	return 0;
 }
 
+static int get_paths_git_dir(struct repository *repo, struct strbuf *buf)
+{
+	strbuf_addstr(buf, repo_get_git_dir(repo));
+	return 0;
+}
+
 /* repo_info_field keys must be in lexicographical order */
 static const struct repo_info_field repo_info_field[] = {
 	{ "layout.bare", get_layout_bare },
 	{ "layout.shallow", get_layout_shallow },
 	{ "object.format", get_object_format },
+	{ "paths.git_dir", get_paths_git_dir },
 	{ "references.format", get_references_format },
 };
 

index a9eb07abe8..63be0849c4 100755
@@ -149,4 +149,14 @@ test_expect_success 'git repo info --keys uses lines as its default output forma
 	test_cmp expect actual
 '
 
+test_expect_success 'paths.git_dir matches rev-parse --git-dir' '
+	git init repo &&
+	(
+		cd repo &&
+		git repo info paths.git_dir >actual &&
+		echo "paths.git_dir=$(git rev-parse --git-dir)" >expect &&
+		test_cmp expect actual
+	)
+'
+
 test_done

gitgitgadget

From: Jayesh Daga <[email protected]>

trace2 calls in read-cache.c use the global 'the_repository', even though the relevant index_state provides an explicit repository pointer via 'istate->repo'.

Using the global repository can result in incorrect trace2 output when multiple repository instances are in use, as events may be attributed to the wrong repository.

Use 'istate->repo' instead to ensure correct repository attribution.

---
 read-cache.c | 32 ++++++++------------------------
 1 file changed, 8 insertions(+), 24 deletions(-)

index 5049f9baca..b1074fbf06 100644
@@ -2309,13 +2309,9 @@ 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",
+	trace2_data_intmax("index", istate->repo, "read/version",
 			   istate->version);
-	trace2_data_intmax("index", the_repository, "read/cache_nr",
+	trace2_data_intmax("index", istate->repo, "read/cache_nr",
 			   istate->cache_nr);
 
 	/*

@@ -2360,16 +2356,12 @@ int read_index_from(struct index_state *istate, const char *path,
 	if (istate->initialized)
 		return istate->cache_nr;
 
-	/*
-	 * TODO trace2: replace "the_repository" with the actual repo instance
-	 * that is associated with the given "istate".
-	 */
-	trace2_region_enter_printf("index", "do_read_index", the_repository,
+	trace2_region_enter_printf("index", "do_read_index", istate->repo,
 				   "%s", path);
 	trace_performance_enter();
 	ret = do_read_index(istate, path, 0);
 	trace_performance_leave("read cache %s", path);
-	trace2_region_leave_printf("index", "do_read_index", the_repository,
+	trace2_region_leave_printf("index", "do_read_index", istate->repo,
 				   "%s", path);
 
 	split_index = istate->split_index;

@@ -3096,13 +3088,9 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,
 	istate->timestamp.nsec = ST_MTIME_NSEC(st);
 	trace_performance_since(start, "write index, changed mask = %x", istate->cache_changed);
 
-	/*
-	 * TODO trace2: replace "the_repository" with the actual repo instance
-	 * that is associated with the given "istate".
-	 */
-	trace2_data_intmax("index", the_repository, "write/version",
+	trace2_data_intmax("index", istate->repo, "write/version",
 			   istate->version);
-	trace2_data_intmax("index", the_repository, "write/cache_nr",
+	trace2_data_intmax("index", istate->repo, "write/cache_nr",
 			   istate->cache_nr);
 
 	ret = 0;

@@ -3144,14 +3132,10 @@ static int do_write_locked_index(struct index_state *istate,
 		return ret;
 	}
 
-	/*
-	 * TODO trace2: replace "the_repository" with the actual repo instance
-	 * that is associated with the given "istate".
-	 */
-	trace2_region_enter_printf("index", "do_write_index", the_repository,
+	trace2_region_enter_printf("index", "do_write_index", istate->repo,
 				   "%s", get_lock_file_path(lock));
 	ret = do_write_index(istate, lock->tempfile, write_extensions, flags);
-	trace2_region_leave_printf("index", "do_write_index", the_repository,
+	trace2_region_leave_printf("index", "do_write_index", istate->repo,
 				   "%s", get_lock_file_path(lock));
 
 	if (was_full)

gitgitgadget

From: Jayesh Daga <[email protected]>

trace2 calls in read-cache.c use the global 'the_repository', even though the relevant index_state provides an explicit repository pointer via 'istate->repo'.

Using the global repository can result in incorrect trace2 output when multiple repository instances are in use, as events may be attributed to the wrong repository.

Use 'istate->repo' instead to ensure correct repository attribution.

---
[GSoC] read-cache: use index state repository for trace2 logging

trace2 calls in read-cache.c use the global 'the_repository', even though the relevant index_state provides an explicit repository pointer via 'istate->repo'.

Using the global repository can result in incorrect trace2 output when multiple repository instances are in use, as events may be attributed to the wrong repository.

Use 'istate->repo' instead in these call sites to ensure correct repository attribution.

v4:

* reroll as clean series; drop unrelated commit.

v2:

* Apply the change consistently across read-cache.c
* Drop unnecessary intermediate variable
* Remove obsolete TODO comments
* Update commit message

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

Range-diff vs v3:

 1:  5a8165b05d < -:  ---------- repo: add paths.git_dir repo info key
 2:  9bb6d0fa01 = 1:  c99d731efa read-cache: use istate->repo for trace2 logging

 read-cache.c | 32 ++++++++------------------------
 1 file changed, 8 insertions(+), 24 deletions(-)

index 5049f9baca..b1074fbf06 100644
@@ -2309,13 +2309,9 @@ 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",
+	trace2_data_intmax("index", istate->repo, "read/version",
 			   istate->version);
-	trace2_data_intmax("index", the_repository, "read/cache_nr",
+	trace2_data_intmax("index", istate->repo, "read/cache_nr",
 			   istate->cache_nr);
 
 	/*

@@ -2360,16 +2356,12 @@ int read_index_from(struct index_state *istate, const char *path,
 	if (istate->initialized)
 		return istate->cache_nr;
 
-	/*
-	 * TODO trace2: replace "the_repository" with the actual repo instance
-	 * that is associated with the given "istate".
-	 */
-	trace2_region_enter_printf("index", "do_read_index", the_repository,
+	trace2_region_enter_printf("index", "do_read_index", istate->repo,
 				   "%s", path);
 	trace_performance_enter();
 	ret = do_read_index(istate, path, 0);
 	trace_performance_leave("read cache %s", path);
-	trace2_region_leave_printf("index", "do_read_index", the_repository,
+	trace2_region_leave_printf("index", "do_read_index", istate->repo,
 				   "%s", path);
 
 	split_index = istate->split_index;

@@ -3096,13 +3088,9 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,
 	istate->timestamp.nsec = ST_MTIME_NSEC(st);
 	trace_performance_since(start, "write index, changed mask = %x", istate->cache_changed);
 
-	/*
-	 * TODO trace2: replace "the_repository" with the actual repo instance
-	 * that is associated with the given "istate".
-	 */
-	trace2_data_intmax("index", the_repository, "write/version",
+	trace2_data_intmax("index", istate->repo, "write/version",
 			   istate->version);
-	trace2_data_intmax("index", the_repository, "write/cache_nr",
+	trace2_data_intmax("index", istate->repo, "write/cache_nr",
 			   istate->cache_nr);
 
 	ret = 0;

@@ -3144,14 +3132,10 @@ static int do_write_locked_index(struct index_state *istate,
 		return ret;
 	}
 
-	/*
-	 * TODO trace2: replace "the_repository" with the actual repo instance
-	 * that is associated with the given "istate".
-	 */
-	trace2_region_enter_printf("index", "do_write_index", the_repository,
+	trace2_region_enter_printf("index", "do_write_index", istate->repo,
 				   "%s", get_lock_file_path(lock));
 	ret = do_write_index(istate, lock->tempfile, write_extensions, flags);
-	trace2_region_leave_printf("index", "do_write_index", the_repository,
+	trace2_region_leave_printf("index", "do_write_index", istate->repo,
 				   "%s", get_lock_file_path(lock));
 
 	if (was_full)

base-commit: 5361983c075154725be47b65cca9a2421789e410

gitgitgadget

A range-diff with v2 would have been much more relevant, but the patch below looks super boring compared to v2 which is very good. Just replacing the_repository with istate->repo and nothing else, which is exactly we expect to see from the patch title above ;-)

Will queue. Thanks.

Thanks for putting in the work to create this complete commit. LGTM.

-Stolee

Hi Derrick,

Thanks for the review and for taking the time to look at this.

Appreciate it!

Thanks,
Jayesh