unpack-trees: use explicit repository in trace2 calls

PATCH Superseded Jayesh Daga via GitGitGadget 3 participants 9 messages Mar 30 – Jun 9

lore

Share & tools

lore

Jayesh Daga via GitGitGadget

author

2026-03-30 20:13 UTC #0 lore raw

PATCH 1/1 unpack-trees: use explicit repository in trace2 calls

From: Jayesh Daga <[email protected]>

trace2 calls in unpack-trees.c use the global 'the_repository', even though the relevant context provides an explicit repository pointer via 'istate->repo' or the local 'repo' variable.

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 explicit repository pointers instead to ensure correct repository attribution.

Signed-off-by Jayesh Daga <[email protected]>

---
unpack-trees: use explicit repository in trace2 calls

trace2 calls in unpack-trees.c use the global 'the_repository', even though the relevant context provides an explicit repository pointer via 'istate->repo' or the local 'repo' variable.

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 explicit repository pointers instead in these call sites to ensure correct repository attribution.

Signed-off-by Jayesh Daga [email protected]

cc Karthik Nayak [email protected]

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

 unpack-trees.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

unpack-trees.c +6 -6

index 998a1e6dc7..191b9d4769 100644
@@ -1780,14 +1780,14 @@ static int clear_ce_flags(struct index_state *istate,
 
 	xsnprintf(label, sizeof(label), "clear_ce_flags(0x%08lx,0x%08lx)",
 		  (unsigned long)select_mask, (unsigned long)clear_mask);
-	trace2_region_enter("unpack_trees", label, the_repository);
+	trace2_region_enter("unpack_trees", label, istate->repo);
 	rval = clear_ce_flags_1(istate,
 				istate->cache,
 				istate->cache_nr,
 				&prefix,
 				select_mask, clear_mask,
 				pl, 0, 0);
-	trace2_region_leave("unpack_trees", label, the_repository);
+	trace2_region_leave("unpack_trees", label, istate->repo);
 
 	stop_progress(&istate->progress);
 	return rval;

@@ -1903,7 +1903,7 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
 		BUG("o->df_conflict_entry is an output only field");
 
 	trace_performance_enter();
-	trace2_region_enter("unpack_trees", "unpack_trees", the_repository);
+	trace2_region_enter("unpack_trees", "unpack_trees", repo);
 
 	prepare_repo_settings(repo);
 	if (repo->settings.command_requires_full_index) {

@@ -2007,9 +2007,9 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
 		}
 
 		trace_performance_enter();
-		trace2_region_enter("unpack_trees", "traverse_trees", the_repository);
+		trace2_region_enter("unpack_trees", "traverse_trees", repo);
 		ret = traverse_trees(o->src_index, len, t, &info);
-		trace2_region_leave("unpack_trees", "traverse_trees", the_repository);
+		trace2_region_leave("unpack_trees", "traverse_trees", repo);
 		trace_performance_leave("traverse_trees");
 		if (ret < 0)
 			goto return_failed;

@@ -2106,7 +2106,7 @@ done:
 		dir_clear(o->internal.dir);
 		o->internal.dir = NULL;
 	}
-	trace2_region_leave("unpack_trees", "unpack_trees", the_repository);
+	trace2_region_leave("unpack_trees", "unpack_trees", repo);
 	trace_performance_leave("unpack_trees");
 	return ret;

base-commit: 5361983c075154725be47b65cca9a2421789e410

Signature · 1 line

gitgitgadget

Patrick Steinhardt

2026-03-31 05:31 UTC #1 lore raw

Quoted 6 lines · Jayesh Daga via GitGitGadget

From: Jayesh Daga <[email protected]>
unpack-trees.c +0 -0
index 998a1e6dc7..191b9d4769 100644

unpack-trees.c int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options · line 1909

Quoted 8 lines

unpack-trees.c +1 -1

@@ -1903,7 +1903,7 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
 		BUG("o->df_conflict_entry is an output only field");
 
 	trace_performance_enter();
-	trace2_region_enter("unpack_trees", "unpack_trees", the_repository);
+	trace2_region_enter("unpack_trees", "unpack_trees", repo);
 
 	prepare_repo_settings(repo);
 	if (repo->settings.command_requires_full_index) {

The changes in unpack_trees() are a bit misleading -- while it reads as if we don't use the_repository anymore, we still do because the function starts with:

  int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options *o)
  {
  	struct repository *repo = the_repository;

So would it make sense to maybe have a separate patch where we inject a repository as a parameter to unpack_trees()?

Once that's done we only have a handful of other places, and in all but two cases we have a repository available via the index. Do we maybe want to go all the way so that we can drop USE_THE_REPOSITORY_VARIABLE at the end of this series?

Thanks!

Patrick

Junio C Hamano

2026-03-31 15:32 UTC #2 lore raw

Quoted 10 lines · Patrick Steinhardt

The changes in unpack_trees() are a bit misleading -- while it reads as if we don't use the_repository anymore, we still do because the function starts with:
  int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options *o)
  {
  	struct repository *repo = the_repository;
So would it make sense to maybe have a separate patch where we inject a repository as a parameter to unpack_trees()?

We can see that "struct unpack_trees_options" is rich enough in the merge context that it would be a natural place to have it unless it is already tehre.

In fact, o->dst_index->repo should probably be what you want, and because it would be insane to start from an index in a repo and store the resulting updated index in another repo, there probably needs an assert(o->dst_index->repo == o->src_index->repo) somewhere.

Jayesh Daga via GitGitGadget

author

2026-03-31 15:34 UTC #4 lore raw

v2 cover unpack-trees: use explicit repository in trace2 calls

In reply to Jayesh Daga via GitGitGadget

trace2 calls in unpack-trees.c use the global 'the_repository', even though the relevant context provides an explicit repository pointer via 'istate->repo' or the local 'repo' variable.

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 explicit repository pointers instead in these call sites to ensure correct repository attribution.

Signed-off-by Jayesh Daga [email protected]

v2:

* Use repository from src_index instead of the_repository
* Address review feedback from Patrick Steinhardt
* Avoid introducing new API or struct fields

cc Karthik Nayak [email protected]

Jayesh Daga (2):
unpack-trees: use repository from index instead of global
unpack-trees: use repository from index instead of global

 unpack-trees.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

base-commit: 5361983c075154725be47b65cca9a2421789e410
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2258%2Fjayesh0104%2Funpack-trees-trace2-repo-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2258/jayesh0104/unpack-trees-trace2-repo-v2
Pull-Request: https://github.com/git/git/pull/2258

Range-diff vs v1:

1: 717da16044 ! 1: f03ea194e3 unpack-trees: use explicit repository in trace2 calls
@@ Metadata
Author: Jayesh Daga <[email protected]>

## Commit message ##
- unpack-trees: use explicit repository in trace2 calls
+ unpack-trees: use repository from index instead of global

- trace2 calls in unpack-trees.c use the global 'the_repository',
- even though the relevant context provides an explicit repository
- pointer via 'istate->repo' or the local 'repo' variable.
+ unpack_trees() currently initializes its repository from the
+ global 'the_repository', even though a repository instance is
+ already available via the source index.

- 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 'o->src_index->repo' instead of the global variable,
+ reducing reliance on global repository state.

- Use explicit repository pointers instead to ensure correct
- repository attribution.
+ This is a step towards eliminating global repository usage in
+ unpack_trees().

+ Suggested-by: Patrick Steinhardt <[email protected]>

Signed-off-by Jayesh Daga <[email protected]>

## unpack-trees.c ##
-: ---------- > 2: fbdf3271b7 unpack-trees: use repository from index instead of global

Signature · 1 line

gitgitgadget

Jayesh Daga via GitGitGadget

author

2026-03-31 15:34 UTC #5 lore raw

PATCH v2 1/2 unpack-trees: use repository from index instead of global

From: Jayesh Daga <[email protected]>

unpack_trees() currently initializes its repository from the global 'the_repository', even though a repository instance is already available via the source index.

Use 'o->src_index->repo' instead of the global variable, reducing reliance on global repository state.

This is a step towards eliminating global repository usage in unpack_trees().

Signed-off-by Jayesh Daga <[email protected]>

---
 unpack-trees.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

unpack-trees.c +6 -6

index 998a1e6dc7..191b9d4769 100644
@@ -1780,14 +1780,14 @@ static int clear_ce_flags(struct index_state *istate,
 
 	xsnprintf(label, sizeof(label), "clear_ce_flags(0x%08lx,0x%08lx)",
 		  (unsigned long)select_mask, (unsigned long)clear_mask);
-	trace2_region_enter("unpack_trees", label, the_repository);
+	trace2_region_enter("unpack_trees", label, istate->repo);
 	rval = clear_ce_flags_1(istate,
 				istate->cache,
 				istate->cache_nr,
 				&prefix,
 				select_mask, clear_mask,
 				pl, 0, 0);
-	trace2_region_leave("unpack_trees", label, the_repository);
+	trace2_region_leave("unpack_trees", label, istate->repo);
 
 	stop_progress(&istate->progress);
 	return rval;

@@ -1903,7 +1903,7 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
 		BUG("o->df_conflict_entry is an output only field");
 
 	trace_performance_enter();
-	trace2_region_enter("unpack_trees", "unpack_trees", the_repository);
+	trace2_region_enter("unpack_trees", "unpack_trees", repo);
 
 	prepare_repo_settings(repo);
 	if (repo->settings.command_requires_full_index) {

@@ -2007,9 +2007,9 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
 		}
 
 		trace_performance_enter();
-		trace2_region_enter("unpack_trees", "traverse_trees", the_repository);
+		trace2_region_enter("unpack_trees", "traverse_trees", repo);
 		ret = traverse_trees(o->src_index, len, t, &info);
-		trace2_region_leave("unpack_trees", "traverse_trees", the_repository);
+		trace2_region_leave("unpack_trees", "traverse_trees", repo);
 		trace_performance_leave("traverse_trees");
 		if (ret < 0)
 			goto return_failed;

@@ -2106,7 +2106,7 @@ done:
 		dir_clear(o->internal.dir);
 		o->internal.dir = NULL;
 	}
-	trace2_region_leave("unpack_trees", "unpack_trees", the_repository);
+	trace2_region_leave("unpack_trees", "unpack_trees", repo);
 	trace_performance_leave("unpack_trees");
 	return ret;

Signature · 1 line

gitgitgadget

Jayesh Daga via GitGitGadget

author

2026-03-31 15:34 UTC #6 lore raw

PATCH v2 2/2 unpack-trees: use repository from index instead of global

In reply to Jayesh Daga via GitGitGadget

From: Jayesh Daga <[email protected]>

unpack_trees() currently initializes its repository from the global 'the_repository', even though a repository instance is already available via the source index.

Use 'o->src_index->repo' instead of the global variable, reducing reliance on global repository state.

This is a step towards eliminating global repository usage in unpack_trees().

Signed-off-by Jayesh Daga <[email protected]>

---
 unpack-trees.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

unpack-trees.c +1 -1

index 191b9d4769..b42020f16b 100644
@@ -1882,7 +1882,7 @@ static int verify_absent(const struct cache_entry *,
  */
 int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options *o)
 {
-	struct repository *repo = the_repository;
+	struct repository *repo = o->src_index->repo;
 	int i, ret;
 	static struct cache_entry *dfc;
 	struct pattern_list pl;

Signature · 1 line

gitgitgadget

Junio C Hamano

2026-03-31 20:28 UTC #3 lore raw

In reply to Junio C Hamano

Quoted 13 lines · Junio C Hamano

Quoted 10 lines · Patrick Steinhardt
The changes in unpack_trees() are a bit misleading -- while it reads as if we don't use the_repository anymore, we still do because the function starts with:
  int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options *o)
  {
  	struct repository *repo = the_repository;
So would it make sense to maybe have a separate patch where we inject a repository as a parameter to unpack_trees()?

Quoted 8 lines

We can see that "struct unpack_trees_options" is rich enough in the merge context that it would be a natural place to have it unless it is already tehre.
In fact, o->dst_index->repo should probably be what you want, and because it would be insane to start from an index in a repo and store the resulting updated index in another repo, there probably needs an assert(o->dst_index->repo == o->src_index->repo) somewhere.

Actually, assert(dst_index->repo == src_index->repo) is probably not what we want, as dst_index can legitimately be NULL, even since 34110cd4 (Make 'unpack_trees()' have a separate source and destination index, 2008-03-06) introduced srparete src/dst indices to unpack_trees() API.

We will always unpack into our own internal index, but we will take the source from wherever specified, and we will optionally write the result to a specified index (optionally, because not everybody even _wants_ any
result: the index diffing really wants to just walk the tree and index in parallel).

So o->src_index->repo is what we want in this case, I think.

Junio C Hamano

2026-03-31 21:35 UTC #7 lore raw

In reply to Jayesh Daga via GitGitGadget

Quoted 3 lines · Jayesh Daga via GitGitGadget

Jayesh Daga (2):
unpack-trees: use repository from index instead of global
unpack-trees: use repository from index instead of global

That is unusual to have two commits with identical title and identical proposed log messages yet with different patch text.

Do you perhaps want to squash them into a single commit?

Junio C Hamano

2026-06-09 21:41 UTC #8 lore raw

Quoted 10 lines · Junio C Hamano

Quoted 3 lines · Jayesh Daga via GitGitGadget
Jayesh Daga (2):
unpack-trees: use repository from index instead of global
unpack-trees: use repository from index instead of global
That is unusual to have two commits with identical title and identical proposed log messages yet with different patch text.
Do you perhaps want to squash them into a single commit?

After not hearing anything for full two months, I just decided to squash these two patches into one, as the second one looked clearly like "ooops, I screwed up and left one pice behind, so here is an incremental update". I'll mark the result for 'next'.