Volume XXII, number 280Wednesday, October 7, 2026Latest message 4 hours ago

The Git List

News and archive of git@vger.kernel.org, since April 2005

patchunpack-trees: use explicit repository in trace2 calls

9 messages between Mar 30, 2026 and Jun 9, 2026, from Jayesh Daga via GitGitGadget, Patrick Steinhardt, Junio C Hamano.

Plain Markdown or JSON for tools and agents. Diffs are folded; open one to read it.

Jayesh Daga via GitGitGadgetMar 30, 2026, 20:13 UTC on lore
From: Jayesh Daga <jayeshdaga99@gmail.com>

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 <jayeshdaga99@gmail.com>
---
    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 jayeshdaga99@gmail.com
    
    cc :Karthik Nayak karthik.188@gmail.com
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(-)
Show changes to unpack-trees.c +6 −6
diff --git a/unpack-trees.c b/unpack-trees.c
index 998a1e6dc7..191b9d4769 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -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

-- 
gitgitgadget
Patrick SteinhardtMar 31, 2026, 05:31 UTC in reply to Jayesh Daga via GitGitGadget on lore

Re: [PATCH] unpack-trees: use explicit repository in trace2 calls

On Mon, Mar 30, 2026 at 08:13:27PM +0000, Jayesh Daga via GitGitGadget wrote:
Show 14 quoted lines
> From: Jayesh Daga <jayeshdaga99@gmail.com>
> diff --git a/unpack-trees.c b/unpack-trees.c
> index 998a1e6dc7..191b9d4769 100644
> --- a/unpack-trees.c
> +++ b/unpack-trees.c
> @@ -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 HamanoMar 31, 2026, 15:32 UTC in reply to Patrick Steinhardt on lore

Re: [PATCH] unpack-trees: use explicit repository in trace2 calls

Patrick Steinhardt <ps@pks.im> writes:
Show 10 quoted lines
> 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 GitGitGadgetMar 31, 2026, 15:34 UTC in reply to Jayesh Daga via GitGitGadget on lore

[PATCH v2 0/2] 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 jayeshdaga99@gmail.com
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 karthik.188@gmail.com
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 <jayeshdaga99@gmail.com>
      
       ## 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 <ps@pks.im>
          Signed-off-by: Jayesh Daga <jayeshdaga99@gmail.com>
      
       ## unpack-trees.c ##
 -:  ---------- > 2:  fbdf3271b7 unpack-trees: use repository from index instead of global
-- 
gitgitgadget
Jayesh Daga via GitGitGadgetMar 31, 2026, 15:34 UTC in reply to Jayesh Daga via GitGitGadget on lore

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

From: Jayesh Daga <jayeshdaga99@gmail.com>

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

Suggested-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Jayesh Daga <jayeshdaga99@gmail.com>
---
 unpack-trees.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)
Show changes to unpack-trees.c +6 −6
diff --git a/unpack-trees.c b/unpack-trees.c
index 998a1e6dc7..191b9d4769 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -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;
 

-- 
gitgitgadget
Jayesh Daga via GitGitGadgetMar 31, 2026, 15:34 UTC in reply to Jayesh Daga via GitGitGadget on lore

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

From: Jayesh Daga <jayeshdaga99@gmail.com>

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

Suggested-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Jayesh Daga <jayeshdaga99@gmail.com>
---
 unpack-trees.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to unpack-trees.c +1 −1
diff --git a/unpack-trees.c b/unpack-trees.c
index 191b9d4769..b42020f16b 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -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;

-- 
gitgitgadget
Junio C HamanoMar 31, 2026, 20:27 UTC in reply to Junio C Hamano on lore

Re: [PATCH] unpack-trees: use explicit repository in trace2 calls

Junio C Hamano <gitster@pobox.com> writes:
Show 21 quoted lines
> Patrick Steinhardt <ps@pks.im> writes:
>
>> 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.

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 HamanoMar 31, 2026, 21:35 UTC in reply to Jayesh Daga via GitGitGadget on lore

Re: [PATCH v2 0/2] unpack-trees: use explicit repository in trace2 calls

"Jayesh Daga via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 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 HamanoJun 9, 2026, 21:41 UTC in reply to Junio C Hamano on lore

Re: [PATCH v2 0/2] unpack-trees: use explicit repository in trace2 calls

Junio C Hamano <gitster@pobox.com> writes:
Show 10 quoted lines
> "Jayesh Daga via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> 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'.

Back to recent threads