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

The Git List

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

patchadd-patch: use repository instance from add_i_state instead of the_repository

9 messages between Mar 17, 2026 and Mar 18, 2026, from Shreyansh Paliwal, Junio C Hamano.

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

Shreyansh PaliwalMar 17, 2026, 15:50 UTC on lore

Functions parse_diff(), edit_hunk_manually() and patch_update_file() use the_repository even though a repository instance is already available via struct add_i_state s which is defined in struct add_p_state *s.

Use 's->s.r' instead of the_repository to avoid relying on global state. All callers pass a valid add_p_state and this does not change any behavior.

This aligns with the ongoing effort to reduce usage of the_repository global state.

Signed-off-by: Shreyansh Paliwal <shreyanshpaliwalcmsmn@gmail.com>
---
 add-patch.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
Show changes to add-patch.c +4 −5
diff --git a/add-patch.c b/add-patch.c
index 8c03f710d3..30df920723 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -434,8 +434,8 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps)
 		strvec_push(&args,
 			    /* could be on an unborn branch */
 			    !strcmp("HEAD", s->revision) &&
-			    repo_get_oid(the_repository, "HEAD", &oid) ?
-			    empty_tree_oid_hex(the_repository->hash_algo) : s->revision);
+			    repo_get_oid(s->s.r, "HEAD", &oid) ?
+			    empty_tree_oid_hex(s->s.r->hash_algo) : s->revision);
 	}
 	color_arg_index = args.nr;
 	/* Use `--no-color` explicitly, just in case `diff.color = always`. */
@@ -1147,7 +1147,7 @@ static int edit_hunk_manually(struct add_p_state *s, struct hunk *hunk)
 				"removed, then the edit is\n"
 				"aborted and the hunk is left unchanged.\n"));

-	if (strbuf_edit_interactively(the_repository, &s->buf,
+	if (strbuf_edit_interactively(s->s.r, &s->buf,
 				      "addp-hunk-edit.diff", NULL) < 0)
 		return -1;

@@ -1551,7 +1551,7 @@ static size_t patch_update_file(struct add_p_state *s, size_t idx)
 		if (file_diff->hunk_nr) {
 			if (rendered_hunk_index != hunk_index) {
 				if (use_pager) {
-					setup_pager(the_repository);
+					setup_pager(s->s.r);
 					sigchain_push(SIGPIPE, SIG_IGN);
 				}
 				render_hunk(s, hunk, 0, colored, &s->buf);
--
2.53.0
Junio C HamanoMar 17, 2026, 16:47 UTC in reply to Shreyansh Paliwal on lore

Re: [PATCH] add-patch: use repository instance from add_i_state instead of the_repository

Shreyansh Paliwal <shreyanshpaliwalcmsmn@gmail.com> writes:
Show 9 quoted lines
> Functions parse_diff(), edit_hunk_manually() and patch_update_file() use
> the_repository even though a repository instance is already available via
> struct add_i_state s which is defined in struct add_p_state *s.
>
> Use 's->s.r' instead of the_repository to avoid relying on global state. All
> callers pass a valid add_p_state and this does not change any behavior.
>
> This aligns with the ongoing effort to reduce usage of the_repository global
> state.

So we can call this "reduce" but cannot say "eliminate" yet, as the files uses comment_line_str?

The <environment.h> header lists some global variables inside "#ifndef USE_THE_REPOSITORY_VARIABLE/#endif" block, and the comment-line stuff is among them.

The patch looks correct.  Will queue.  Thanks.
Show 40 quoted lines
> Signed-off-by: Shreyansh Paliwal <shreyanshpaliwalcmsmn@gmail.com>
> ---
>  add-patch.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/add-patch.c b/add-patch.c
> index 8c03f710d3..30df920723 100644
> --- a/add-patch.c
> +++ b/add-patch.c
> @@ -434,8 +434,8 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps)
>  		strvec_push(&args,
>  			    /* could be on an unborn branch */
>  			    !strcmp("HEAD", s->revision) &&
> -			    repo_get_oid(the_repository, "HEAD", &oid) ?
> -			    empty_tree_oid_hex(the_repository->hash_algo) : s->revision);
> +			    repo_get_oid(s->s.r, "HEAD", &oid) ?
> +			    empty_tree_oid_hex(s->s.r->hash_algo) : s->revision);
>  	}
>  	color_arg_index = args.nr;
>  	/* Use `--no-color` explicitly, just in case `diff.color = always`. */
> @@ -1147,7 +1147,7 @@ static int edit_hunk_manually(struct add_p_state *s, struct hunk *hunk)
>  				"removed, then the edit is\n"
>  				"aborted and the hunk is left unchanged.\n"));
>
> -	if (strbuf_edit_interactively(the_repository, &s->buf,
> +	if (strbuf_edit_interactively(s->s.r, &s->buf,
>  				      "addp-hunk-edit.diff", NULL) < 0)
>  		return -1;
>
> @@ -1551,7 +1551,7 @@ static size_t patch_update_file(struct add_p_state *s, size_t idx)
>  		if (file_diff->hunk_nr) {
>  			if (rendered_hunk_index != hunk_index) {
>  				if (use_pager) {
> -					setup_pager(the_repository);
> +					setup_pager(s->s.r);
>  					sigchain_push(SIGPIPE, SIG_IGN);
>  				}
>  				render_hunk(s, hunk, 0, colored, &s->buf);
> --
> 2.53.0
Shreyansh PaliwalMar 17, 2026, 16:51 UTC in reply to Junio C Hamano on lore

Re: [PATCH] add-patch: use repository instance from add_i_state instead of the_repository

Show 17 quoted lines
> > Functions parse_diff(), edit_hunk_manually() and patch_update_file() use
> > the_repository even though a repository instance is already available via
> > struct add_i_state s which is defined in struct add_p_state *s.
> >
> > Use 's->s.r' instead of the_repository to avoid relying on global state. All
> > callers pass a valid add_p_state and this does not change any behavior.
> >
> > This aligns with the ongoing effort to reduce usage of the_repository global
> > state.
>
> So we can call this "reduce" but cannot say "eliminate" yet, as the
> files uses comment_line_str?
>
> The <environment.h> header lists some global variables inside
> "#ifndef USE_THE_REPOSITORY_VARIABLE/#endif" block, and the
> comment-line stuff is among them.
>

Yes that's right, there is an instance of comment_line_str, should I add this in the commit message and send a reroll ?

Junio C HamanoMar 17, 2026, 18:04 UTC in reply to Shreyansh Paliwal on lore

Re: [PATCH] add-patch: use repository instance from add_i_state instead of the_repository

Shreyansh Paliwal <shreyanshpaliwalcmsmn@gmail.com> writes:
Show 20 quoted lines
>> > Functions parse_diff(), edit_hunk_manually() and patch_update_file() use
>> > the_repository even though a repository instance is already available via
>> > struct add_i_state s which is defined in struct add_p_state *s.
>> >
>> > Use 's->s.r' instead of the_repository to avoid relying on global state. All
>> > callers pass a valid add_p_state and this does not change any behavior.
>> >
>> > This aligns with the ongoing effort to reduce usage of the_repository global
>> > state.
>>
>> So we can call this "reduce" but cannot say "eliminate" yet, as the
>> files uses comment_line_str?
>>
>> The <environment.h> header lists some global variables inside
>> "#ifndef USE_THE_REPOSITORY_VARIABLE/#endif" block, and the
>> comment-line stuff is among them.
>>
>
> Yes that's right, there is an instance of comment_line_str, should I add this
> in the commit message and send a reroll ?
No need to.  I was just wondering if there are something I missed.
Junio C HamanoMar 17, 2026, 20:10 UTC in reply to Shreyansh Paliwal on lore

Re: [PATCH] add-patch: use repository instance from add_i_state instead of the_repository

Shreyansh Paliwal <shreyanshpaliwalcmsmn@gmail.com> writes:
Show 20 quoted lines
>> > Functions parse_diff(), edit_hunk_manually() and patch_update_file() use
>> > the_repository even though a repository instance is already available via
>> > struct add_i_state s which is defined in struct add_p_state *s.
>> >
>> > Use 's->s.r' instead of the_repository to avoid relying on global state. All
>> > callers pass a valid add_p_state and this does not change any behavior.
>> >
>> > This aligns with the ongoing effort to reduce usage of the_repository global
>> > state.
>>
>> So we can call this "reduce" but cannot say "eliminate" yet, as the
>> files uses comment_line_str?
>>
>> The <environment.h> header lists some global variables inside
>> "#ifndef USE_THE_REPOSITORY_VARIABLE/#endif" block, and the
>> comment-line stuff is among them.
>>
>
> Yes that's right, there is an instance of comment_line_str, should I add this
> in the commit message and send a reroll ?

Having said that, please make sure your patch works well with patches others are working on. In this case, s->s.r would no longer exist after this one:

commit d51b61f5dab9c8e715fa792f31d572bc96fb5687
Author: Patrick Steinhardt <ps@pks.im>
Date:   Mon Mar 2 13:13:07 2026 +0100
    add-patch: remove dependency on "add-interactive" subsystem
    
    With the preceding commit we have split out interactive configuration
    that is used by both "git add -p" and "git add -i". But we still
    initialize that configuration in the "add -p" subsystem by calling
    `init_add_i_state()`, even though we only do so to initialize the
    interactive configuration as well as a repository pointer.
    
    Stop doing so and instead store and initialize the interactive
    configuration in `struct add_p_state` directly.
    
    Signed-off-by: Patrick Steinhardt <ps@pks.im>
    Signed-off-by: Junio C Hamano <gitster@pobox.com>

A good way to ensure that you do not send a patch that does not work well with others is to make a trial merge to 'next' and 'seen' and ensure that they produce working Git, after making sure your patch applied directly on top of 'master' works well.

Thanks.
Junio C HamanoMar 17, 2026, 20:25 UTC in reply to Junio C Hamano on lore

Re: [PATCH] add-patch: use repository instance from add_i_state instead of the_repository

Junio C Hamano <gitster@pobox.com> writes:
Show 26 quoted lines
> Having said that, please make sure your patch works well with
> patches others are working on.  In this case, s->s.r would no longer
> exist after this one:
>
> commit d51b61f5dab9c8e715fa792f31d572bc96fb5687
> Author: Patrick Steinhardt <ps@pks.im>
> Date:   Mon Mar 2 13:13:07 2026 +0100
>
>     add-patch: remove dependency on "add-interactive" subsystem
>     
>     With the preceding commit we have split out interactive configuration
>     that is used by both "git add -p" and "git add -i". But we still
>     initialize that configuration in the "add -p" subsystem by calling
>     `init_add_i_state()`, even though we only do so to initialize the
>     interactive configuration as well as a repository pointer.
>     
>     Stop doing so and instead store and initialize the interactive
>     configuration in `struct add_p_state` directly.
>     
>     Signed-off-by: Patrick Steinhardt <ps@pks.im>
>     Signed-off-by: Junio C Hamano <gitster@pobox.com>
>
> A good way to ensure that you do not send a patch that does not work
> well with others is to make a trial merge to 'next' and 'seen' and
> ensure that they produce working Git, after making sure your patch
> applied directly on top of 'master' works well.

The above "good way" is to notice and stop yourself from sending a patch that wants s->s.r to still exist.

After that happens, it is tempting to rebuild your change on top of 'next'. But please do *NOT* do so.

Instead, identify such dependencies (i.e. other topics with changes relative to what 'master' has, that break what you developed directly on top of 'master'), and then merge them to 'master' yourself. And then bulid your topic on top of the merge. Work hard to limit your dependencies to absolute minimum, as these topics will take your work hostage---until they get merged down to 'master', your topic will not be able to be merged to 'master'.

In this case, you'll be likely to do something like
    $ git checkout -b sp/add-patch-with-fewer-the-repository origin/master
    $ git merge --no-ff origin/ps-history-split
    $ edit ... && git add ... && make test
    $ git commit -m 'add-patch: use repository instance...'

to build your single patch series on top of 'master' taken from my tree, with Patrick's history-split topic merged into it. After the commit is made, send out only your work (i.e., above the merge of Patrick's topic) to the list and you're done.

Thanks.
Shreyansh PaliwalMar 18, 2026, 07:01 UTC in reply to Junio C Hamano on lore

Re: [PATCH] add-patch: use repository instance from add_i_state instead of the_repository

Show 55 quoted lines
> Junio C Hamano <gitster@pobox.com> writes:
>
> > Having said that, please make sure your patch works well with
> > patches others are working on.  In this case, s->s.r would no longer
> > exist after this one:
> >
> > commit d51b61f5dab9c8e715fa792f31d572bc96fb5687
> > Author: Patrick Steinhardt <ps@pks.im>
> > Date:   Mon Mar 2 13:13:07 2026 +0100
> >
> >     add-patch: remove dependency on "add-interactive" subsystem
> >
> >     With the preceding commit we have split out interactive configuration
> >     that is used by both "git add -p" and "git add -i". But we still
> >     initialize that configuration in the "add -p" subsystem by calling
> >     `init_add_i_state()`, even though we only do so to initialize the
> >     interactive configuration as well as a repository pointer.
> >
> >     Stop doing so and instead store and initialize the interactive
> >     configuration in `struct add_p_state` directly.
> >
> >     Signed-off-by: Patrick Steinhardt <ps@pks.im>
> >     Signed-off-by: Junio C Hamano <gitster@pobox.com>
> >
> > A good way to ensure that you do not send a patch that does not work
> > well with others is to make a trial merge to 'next' and 'seen' and
> > ensure that they produce working Git, after making sure your patch
> > applied directly on top of 'master' works well.
>
> The above "good way" is to notice and stop yourself from sending a
> patch that wants s->s.r to still exist.
>
> After that happens, it is tempting to rebuild your change on top of
> 'next'.  But please do *NOT* do so.
>
> Instead, identify such dependencies (i.e. other topics with changes
> relative to what 'master' has, that break what you developed
> directly on top of 'master'), and then merge them to 'master'
> yourself.  And then bulid your topic on top of the merge.  Work hard
> to limit your dependencies to absolute minimum, as these topics will
> take your work hostage---until they get merged down to 'master',
> your topic will not be able to be merged to 'master'.
>
> In this case, you'll be likely to do something like
>
>     $ git checkout -b sp/add-patch-with-fewer-the-repository origin/master
>     $ git merge --no-ff origin/ps-history-split
>     $ edit ... && git add ... && make test
>     $ git commit -m 'add-patch: use repository instance...'
>
> to build your single patch series on top of 'master' taken from my
> tree, with Patrick's history-split topic merged into it.
> After the commit is made, send out only your work (i.e., above the
> merge of Patrick's topic) to the list and you're done.
>

Thanks a lot for this useful tip. Going forward, I will first try to ensure that there are no on-going conflicting patches by a trial merge to seen/next branch. Then after if there are any I would merge those dependent patches locally on a copy of master, thereafter build my changes on that and send them.

Best, Shreyansh

Shreyansh PaliwalMar 18, 2026, 09:00 UTC in reply to Shreyansh Paliwal on lore

[PATCH v2 ] add-patch: use repository instance from add_p_state instead of the_repository

Functions parse_diff(), edit_hunk_manually() and patch_update_file() use the_repository even though a repository instance is already available via struct add_p_state *s. Use 's->r' instead of the_repository to avoid relying on global state. All callers pass a valid add_p_state and this does not change any behavior.

This follows recent refactoring that removed 'add_i_state' and moved repository pointer and other add-patch config into struct add_p_state [1]. This aligns with the ongoing effort to reduce usage of the_repository global state.

[1]- https://lore.kernel.org/git/20260302-pks-history-split-v1-3-444fc987a324@pks.im/
Signed-off-by: Shreyansh Paliwal <shreyanshpaliwalcmsmn@gmail.com>
---
Changes in v2:
 - made changes on top of ps/history-split and used s->r instead of s->s.r
 add-patch.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
Show changes to add-patch.c +4 −5
diff --git a/add-patch.c b/add-patch.c
index 4e28e5c187..f27edcbe8d 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -558,8 +558,8 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps)
 		strvec_push(&args,
 			    /* could be on an unborn branch */
 			    !strcmp("HEAD", s->revision) &&
-			    repo_get_oid(the_repository, "HEAD", &oid) ?
-			    empty_tree_oid_hex(the_repository->hash_algo) : s->revision);
+			    repo_get_oid(s->r, "HEAD", &oid) ?
+			    empty_tree_oid_hex(s->r->hash_algo) : s->revision);
 	}
 	color_arg_index = args.nr;
 	/* Use `--no-color` explicitly, just in case `diff.color = always`. */
@@ -1271,7 +1271,7 @@ static int edit_hunk_manually(struct add_p_state *s, struct hunk *hunk)
 				"removed, then the edit is\n"
 				"aborted and the hunk is left unchanged.\n"));

-	if (strbuf_edit_interactively(the_repository, &s->buf,
+	if (strbuf_edit_interactively(s->r, &s->buf,
 				      "addp-hunk-edit.diff", NULL) < 0)
 		return -1;

@@ -1679,7 +1679,7 @@ static size_t patch_update_file(struct add_p_state *s,
 		if (file_diff->hunk_nr) {
 			if (rendered_hunk_index != hunk_index) {
 				if (use_pager) {
-					setup_pager(the_repository);
+					setup_pager(s->r);
 					sigchain_push(SIGPIPE, SIG_IGN);
 				}
 				render_hunk(s, hunk, 0, colored, &s->buf);
--
2.53.0
Junio C HamanoMar 18, 2026, 16:43 UTC in reply to Shreyansh Paliwal on lore

Re: [PATCH v2 ] add-patch: use repository instance from add_p_state instead of the_repository

Shreyansh Paliwal <shreyanshpaliwalcmsmn@gmail.com> writes:
Show 17 quoted lines
> Functions parse_diff(), edit_hunk_manually() and patch_update_file() use
> the_repository even though a repository instance is already available via
> struct add_p_state *s.
> Use 's->r' instead of the_repository to avoid relying on global state.
> All callers pass a valid add_p_state and this does not change any behavior.
>
> This follows recent refactoring that removed 'add_i_state' and moved
> repository pointer and other add-patch config into struct add_p_state [1].
> This aligns with the ongoing effort to reduce usage of the_repository
> global state.
>
> [1]- https://lore.kernel.org/git/20260302-pks-history-split-v1-3-444fc987a324@pks.im/
>
> Signed-off-by: Shreyansh Paliwal <shreyanshpaliwalcmsmn@gmail.com>
> ---
> Changes in v2:
>  - made changes on top of ps/history-split and used s->r instead of s->s.r
Good.  Brief and clear description on what this patch was based on.
Thanks.
Show 39 quoted lines
>
>  add-patch.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/add-patch.c b/add-patch.c
> index 4e28e5c187..f27edcbe8d 100644
> --- a/add-patch.c
> +++ b/add-patch.c
> @@ -558,8 +558,8 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps)
>  		strvec_push(&args,
>  			    /* could be on an unborn branch */
>  			    !strcmp("HEAD", s->revision) &&
> -			    repo_get_oid(the_repository, "HEAD", &oid) ?
> -			    empty_tree_oid_hex(the_repository->hash_algo) : s->revision);
> +			    repo_get_oid(s->r, "HEAD", &oid) ?
> +			    empty_tree_oid_hex(s->r->hash_algo) : s->revision);
>  	}
>  	color_arg_index = args.nr;
>  	/* Use `--no-color` explicitly, just in case `diff.color = always`. */
> @@ -1271,7 +1271,7 @@ static int edit_hunk_manually(struct add_p_state *s, struct hunk *hunk)
>  				"removed, then the edit is\n"
>  				"aborted and the hunk is left unchanged.\n"));
>
> -	if (strbuf_edit_interactively(the_repository, &s->buf,
> +	if (strbuf_edit_interactively(s->r, &s->buf,
>  				      "addp-hunk-edit.diff", NULL) < 0)
>  		return -1;
>
> @@ -1679,7 +1679,7 @@ static size_t patch_update_file(struct add_p_state *s,
>  		if (file_diff->hunk_nr) {
>  			if (rendered_hunk_index != hunk_index) {
>  				if (use_pager) {
> -					setup_pager(the_repository);
> +					setup_pager(s->r);
>  					sigchain_push(SIGPIPE, SIG_IGN);
>  				}
>  				render_hunk(s, hunk, 0, colored, &s->buf);
> --
> 2.53.0

Back to recent threads

[PATCH] add-patch: use repository instance from add_i_state instead of the_repository | The Git List