unpack-trees: use explicit repository in trace2 calls by jayesh0104 · Pull Request #2258 · git/git

jayesh0104 · 2026-03-30T18:28:30Z

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

v3:

squash duplicate commits into a single commit
fix commit message

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
cc: Justin Tobler jltobler@gmail.com
cc: Ayush Chandekar ayu.chandekar@gmail.com
cc: Siddharth Asthana siddharthasthana31@gmail.com

cc: Patrick Steinhardt ps@pks.im

jayesh0104 · 2026-03-30T20:12:09Z

/submit

gitgitgadget-git · 2026-03-30T20:13:48Z

Submitted as pull.2258.git.git.1774901607564.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-git-2258/jayesh0104/unpack-trees-trace2-repo-v1

To fetch this version to local tag pr-git-2258/jayesh0104/unpack-trees-trace2-repo-v1:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-git-2258/jayesh0104/unpack-trees-trace2-repo-v1

gitgitgadget-git · 2026-03-31T01:10:35Z

This patch series was integrated into seen via 0c1d458.

jayesh0104 · 2026-03-31T15:33:14Z

/submit

gitgitgadget-git · 2026-03-31T15:34:48Z

Submitted as pull.2258.v2.git.git.1774971267.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-git-2258/jayesh0104/unpack-trees-trace2-repo-v2

To fetch this version to local tag pr-git-2258/jayesh0104/unpack-trees-trace2-repo-v2:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-git-2258/jayesh0104/unpack-trees-trace2-repo-v2

gitgitgadget-git · 2026-04-01T20:06:07Z

This branch is now known as jd/unpack-trees-wo-the-repository.

gitgitgadget-git · 2026-04-01T20:06:08Z

This patch series was integrated into seen via 6407076.

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 clarifies the data flow and is a step towards eliminating global repository usage. Suggested-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Jayesh Daga <jayeshdaga99@gmail.com>

gitgitgadget-git · 2026-04-02T16:20:54Z

This patch series was integrated into seen via 916c387.

gitgitgadget-git · 2026-04-02T22:08:39Z

This patch series was integrated into seen via a61a5fc.

gitgitgadget-git · 2026-04-03T20:36:52Z

This patch series was integrated into seen via ee8a57b.

gitgitgadget-git · 2026-04-04T04:09:44Z

This patch series was integrated into seen via a421c81.

gitgitgadget-git · 2026-04-07T02:00:44Z

This patch series was integrated into seen via d59b6cd.

gitgitgadget-git · 2026-04-07T07:25:06Z

There was a status update in the "New Topics" section about the branch jd/unpack-trees-wo-the-repository on the Git mailing list:

A handful of inappropriate uses of the_repository have been
rewritten to use the right repository structure instance in the
unpack-trees.c codepath.

Will merge to 'next'.
source: <pull.2258.git.git.1774901607564.gitgitgadget@gmail.com>

gitgitgadget-git · 2026-04-07T07:26:34Z

Patrick Steinhardt wrote on the Git mailing list (how to reply to this email):

On Mon, Mar 30, 2026 at 08:13:27PM +0000, Jayesh Daga via GitGitGadget wrote:
> 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

gitgitgadget-git · 2026-04-07T07:26:36Z

User Patrick Steinhardt <ps@pks.im> has been added to the cc: list.

gitgitgadget-git · 2026-04-07T07:31:48Z

Junio C Hamano wrote on the Git mailing list (how to reply to this email):

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.

gitgitgadget-git · 2026-04-07T07:35:47Z

Junio C Hamano wrote on the Git mailing list (how to reply to this email):

Junio C Hamano <gitster@pobox.com> writes:

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

gitgitgadget-git · 2026-04-07T07:36:36Z

Junio C Hamano wrote on the Git mailing list (how to reply to this email):

"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?

gitgitgadget-git · 2026-04-07T08:44:20Z

There was a status update in the "New Topics" section about the branch jd/unpack-trees-wo-the-repository on the Git mailing list:

A handful of inappropriate uses of the_repository have been
rewritten to use the right repository structure instance in the
unpack-trees.c codepath.

Will merge to 'next'.
source: <pull.2258.git.git.1774901607564.gitgitgadget@gmail.com>

gitgitgadget-git · 2026-04-07T08:45:45Z

Patrick Steinhardt wrote on the Git mailing list (how to reply to this email):

On Mon, Mar 30, 2026 at 08:13:27PM +0000, Jayesh Daga via GitGitGadget wrote:
> 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

gitgitgadget-git · 2026-04-07T09:03:31Z

There was a status update in the "Cooking" section about the branch jd/unpack-trees-wo-the-repository on the Git mailing list:

A handful of inappropriate uses of the_repository have been
rewritten to use the right repository structure instance in the
unpack-trees.c codepath.

Comments?
source: <pull.2258.v2.git.git.1774971267.gitgitgadget@gmail.com>

gitgitgadget-git · 2026-04-08T01:30:27Z

This patch series was integrated into seen via 6306f22.

gitgitgadget-git · 2026-04-08T16:47:48Z

This patch series was integrated into seen via 892c268.

gitgitgadget-git · 2026-04-08T17:56:02Z

There was a status update in the "Cooking" section about the branch jd/unpack-trees-wo-the-repository on the Git mailing list:

A handful of inappropriate uses of the_repository have been
rewritten to use the right repository structure instance in the
unpack-trees.c codepath.

Comments?
source: <pull.2258.v2.git.git.1774971267.gitgitgadget@gmail.com>

gitgitgadget-git · 2026-04-08T22:34:43Z

This patch series was integrated into seen via c48cb4b.

github-project-automation bot moved this to Todo in Op-Amp Frequency Response and Applications Suite Mar 30, 2026

github-project-automation bot added this to Op-Amp Frequency Response and Applications Suite Mar 30, 2026

gitgitgadget-git bot added the seen label Mar 31, 2026

jayesh0104 force-pushed the unpack-trees-trace2-repo branch from 717da16 to f03ea19 Compare March 31, 2026 05:55

jayesh0104 force-pushed the unpack-trees-trace2-repo branch 2 times, most recently from fbdf327 to f4cf317 Compare April 2, 2026 15:16

Conversation

jayesh0104 commented Mar 30, 2026 • edited by gitgitgadget-git bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jayesh0104 commented Mar 30, 2026

Uh oh!

gitgitgadget-git bot commented Mar 30, 2026

Uh oh!

gitgitgadget-git bot commented Mar 31, 2026

Uh oh!

jayesh0104 commented Mar 31, 2026

Uh oh!

gitgitgadget-git bot commented Mar 31, 2026

Uh oh!

gitgitgadget-git bot commented Apr 1, 2026

Uh oh!

gitgitgadget-git bot commented Apr 1, 2026

Uh oh!

gitgitgadget-git bot commented Apr 2, 2026

Uh oh!

gitgitgadget-git bot commented Apr 2, 2026

Uh oh!

gitgitgadget-git bot commented Apr 3, 2026

Uh oh!

gitgitgadget-git bot commented Apr 4, 2026

Uh oh!

gitgitgadget-git bot commented Apr 7, 2026

Uh oh!

gitgitgadget-git bot commented Apr 7, 2026

Uh oh!

gitgitgadget-git bot commented Apr 7, 2026

Uh oh!

gitgitgadget-git bot commented Apr 7, 2026

Uh oh!

gitgitgadget-git bot commented Apr 7, 2026

Uh oh!

gitgitgadget-git bot commented Apr 7, 2026

Uh oh!

gitgitgadget-git bot commented Apr 7, 2026

Uh oh!

gitgitgadget-git bot commented Apr 7, 2026

Uh oh!

gitgitgadget-git bot commented Apr 7, 2026

Uh oh!

gitgitgadget-git bot commented Apr 7, 2026

Uh oh!

gitgitgadget-git bot commented Apr 8, 2026

Uh oh!

gitgitgadget-git bot commented Apr 8, 2026

Uh oh!

gitgitgadget-git bot commented Apr 8, 2026

Uh oh!

gitgitgadget-git bot commented Apr 8, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

jayesh0104 commented Mar 30, 2026 •

edited by gitgitgadget-git bot

Loading