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

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

[Shreyansh Paliwal]

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

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

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

[Junio C Hamano]

Shreyansh Paliwal <shreyanshpaliwalcmsmn@gmail.com> writes:

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


> 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

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

[Shreyansh Paliwal]

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

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

[Junio C Hamano]

Shreyansh Paliwal <shreyanshpaliwalcmsmn@gmail.com> writes:

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

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

[Junio C Hamano]

Shreyansh Paliwal <shreyanshpaliwalcmsmn@gmail.com> writes:

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

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

[Junio C Hamano]

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.

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

[Shreyansh Paliwal]

> 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

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

[Shreyansh Paliwal]

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

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

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

[Junio C Hamano]

Shreyansh Paliwal <shreyanshpaliwalcmsmn@gmail.com> writes:

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

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