[RFC PATCH 0/1] add-patch: Allow reworking with a file after deciding on its hunks

[RFC PATCH 0/1] add-patch: Allow reworking with a file after deciding on its hunks

[Abraham Samuel Adekunle]

Hello,
In the discussion between Phillip Wood and Junio C Hamano in [1], Junio suggested
some enhancements to the UI of the add interactive.

They include;
i. Add a way to see the previous hunk decision of the current hunk after the
   user navigates back with K/J.
ii. Allow reworking with a file after deciding on all its hunks without
    auto advancing.
iii. When having multiple modified files, allow switching from the current file to
     another file whose hunks have already been decided.

I have been able to work on 'i' above in [2] and this RFC seeks to find suggestions
on 'ii' and maybe 'iii' as this enters design realms

The patch is in no way a final version. I just want to present something that
members giving suggestions can work with.

While trying to follow the suggestions in [1] in the patch, when all hunks
have been decided;
	* A what_now prompts appears, allowing navigation with J/K, q to quit
	and '>' to go to the next file if there is a next file

	* If K/J is used to return to a hunk from the what_now mode, after any new decision,
	on the hunk, the user is brought back to the what_now prompt since all hunks
	had previously been decided on.

I would appreciate your thoughts on this.
Thanks

1. https://lore.kernel.org/git/xmqqseg9azdc.fsf@gitster.g/
2. https://lore.kernel.org/git/aV_IGCld5T_dBxTs@Adekunles-MacBook-Air.local/

Abraham Samuel Adekunle (1):
  add-patch: Allow reworking with a file after deciding on all its hunks

 add-patch.c | 71 ++++++++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 65 insertions(+), 6 deletions(-)

-- 
2.39.5 (Apple Git-154)

[RFC PATCH 1/1] add-patch: Allow reworking with a file after deciding on all its hunks

[Abraham Samuel Adekunle]

After deciding on all hunks in a file, the interactive session
advances automatically to the next file if there is another,
or the process ends.

Allow for reworking with a file by introducing a what_now prompt which
allows for navigating with J/K or advancing to the next file if there is one.

Signed-off-by: Abraham Samuel Adekunle <abrahamadekunle50@gmail.com>
---
 add-patch.c | 71 ++++++++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 65 insertions(+), 6 deletions(-)

diff --git a/add-patch.c b/add-patch.c
index 173a53241e..1ac565b0ab 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -1449,7 +1449,7 @@ static int patch_update_file(struct add_p_state *s,
 	struct hunk *hunk;
 	char ch;
 	struct child_process cp = CHILD_PROCESS_INIT;
-	int colored = !!s->colored.len, quit = 0, use_pager = 0;
+	int colored = !!s->colored.len, quit = 0, use_pager = 0, skip_what_now = 0;
 	enum prompt_mode_type prompt_mode_type;
 
 	/* Empty added files have no hunks */
@@ -1498,12 +1498,61 @@ static int patch_update_file(struct add_p_state *s,
 
 		/* Everything decided? */
 		if (undecided_previous < 0 && undecided_next < 0 &&
-		    hunk->use != UNDECIDED_HUNK)
-			break;
+		    hunk->use != UNDECIDED_HUNK && !skip_what_now ) {
+			const char *prompt_whatnow;
+			/* Allow navigation between hunks or go to next file */
 
+			if (s->file_diff_nr > 1)
+				prompt_whatnow = _("What now? [J,K,q,>]? ");
+			else
+				prompt_whatnow = _("What now? [J,K,q]? ");
+			printf("%s %s",
+				s->s.prompt_color,
+				prompt_whatnow);
+			if (*s->s.reset_color_interactive)
+				fputs(s->s.reset_color_interactive, stdout);
+			fflush(stdout);
+			if (read_single_character(s) == EOF) {
+				quit = 1;
+				break;
+			}
+			if (!s->answer.len)
+				continue;
+			if (s->answer.buf[0] == '>' && s->file_diff_nr > 1) {
+				skip_what_now = 0;
+				break;
+			}
+			else if (s->answer.buf[0] == 'K') {
+				if (file_diff->hunk_nr > 1) {
+					hunk_index = dec_mod(hunk_index, file_diff->hunk_nr);
+					skip_what_now = 1;
+				}
+				else
+					err(s, _("No other hunk"));
+				continue;
+			}
+			else if (s->answer.buf[0] == 'J') {
+				if (file_diff->hunk_nr > 1) {
+					hunk_index = inc_mod(hunk_index, file_diff->hunk_nr);
+					skip_what_now = 1;
+				}
+				else
+					err(s, _("No other hunk"));
+				continue;
+			}
+			else if (s->answer.buf[0] == 'q') {
+				skip_what_now = 0;
+				quit = 1;
+				break;
+			}
+			else {
+				err(s, _("All hunks decided (use '?' for help)"));
+				continue;
+			}
+		}
 		strbuf_reset(&s->buf);
 		if (file_diff->hunk_nr) {
-			if (rendered_hunk_index != hunk_index) {
+			if (rendered_hunk_index != hunk_index || skip_what_now == 1) {
 				if (use_pager) {
 					setup_pager(the_repository);
 					sigchain_push(SIGPIPE, SIG_IGN);
@@ -1586,12 +1635,18 @@ static int patch_update_file(struct add_p_state *s,
 		if (ch == 'y') {
 			hunk->use = USE_HUNK;
 soft_increment:
-			hunk_index = undecided_next < 0 ?
-				file_diff->hunk_nr : undecided_next;
+			if (skip_what_now) {
+				hunk_index = inc_mod(hunk_index, file_diff->hunk_nr);
+				skip_what_now = 0;
+			} else
+				hunk_index = undecided_next < 0 ?
+					file_diff->hunk_nr : undecided_next;
 		} else if (ch == 'n') {
 			hunk->use = SKIP_HUNK;
 			goto soft_increment;
 		} else if (ch == 'a') {
+			if (skip_what_now)
+				skip_what_now = 0;
 			if (file_diff->hunk_nr) {
 				for (; hunk_index < file_diff->hunk_nr; hunk_index++) {
 					hunk = file_diff->hunk + hunk_index;
@@ -1604,6 +1659,8 @@ static int patch_update_file(struct add_p_state *s,
 				hunk->use = USE_HUNK;
 			}
 		} else if (ch == 'd') {
+			if (skip_what_now)
+				skip_what_now = 0;
 			if (file_diff->hunk_nr) {
 				for (; hunk_index < file_diff->hunk_nr; hunk_index++) {
 					hunk = file_diff->hunk + hunk_index;
@@ -1616,6 +1673,8 @@ static int patch_update_file(struct add_p_state *s,
 				hunk->use = SKIP_HUNK;
 			}
 		} else if (ch == 'q') {
+			if (skip_what_now)
+				skip_what_now = 0;
 			quit = 1;
 			break;
 		} else if (s->answer.buf[0] == 'K') {
-- 
2.39.5 (Apple Git-154)

Re: [RFC PATCH 1/1] add-patch: Allow reworking with a file after deciding on all its hunks

[Junio C Hamano]

Abraham Samuel Adekunle <abrahamadekunle50@gmail.com> writes:

> After deciding on all hunks in a file, the interactive session
> advances automatically to the next file if there is another,
> or the process ends.
>
> Allow for reworking with a file by introducing a what_now prompt which
> allows for navigating with J/K or advancing to the next file if there is one.

Describe "how" you are allowing these new things that users used not
to be able to do (no, not in the "by adding this variable and
switching on its value" sense, but in the "now deciding on all the
hunks in a file does not automatically advance to the next file, and
the user has to do X to move forward" sense).

> -	int colored = !!s->colored.len, quit = 0, use_pager = 0;
> +	int colored = !!s->colored.len, quit = 0, use_pager = 0, skip_what_now = 0;

This is getting overly long.  Wouldn't it be easier to follow if a
preliminary patch split these existing variables into three
independent definitions, and the main patch adds the fourth one?

> +			if (s->file_diff_nr > 1)
> +				prompt_whatnow = _("What now? [J,K,q,>]? ");
> +			else
> +				prompt_whatnow = _("What now? [J,K,q]? ");

I wonder if ">" has to be made so special.  Wouldn't it be easier to
reason about the logic if ">" (and probably "<" to go back by one
file) are added to the prompt in the same logic that decides 'g',
'k', 's', etc. should be shown using the "permitted" variable?

And when the inter-file navigation is in the permitted set (i.e.,
there are multiple files involved), you'd show ">" (or "<", or both
if you are dealing with the second file among three files) and ask,
instead of silently moving to the next one, or something like that.

Organizing the logic that way will also allow you to move to the
next file _without_ first having to decide on all hunks in the
current file.  Just say ">" to deal with the next file first, and
after you are done, either come back with "<", or the system notices
that there are undecided hunks in the earlier file and takes you
back automatically.

I also have a hunch that with such a code structure you may not even
need skip_what_now flag, but I haven't even written the code in my
head, so if somebody tries to do so, they may discover the reason
why such a flag is still needed.

>  		strbuf_reset(&s->buf);
>  		if (file_diff->hunk_nr) {
> -			if (rendered_hunk_index != hunk_index) {
> +			if (rendered_hunk_index != hunk_index || skip_what_now == 1) {

Style (which may become irrelevant, as I just said the variable may
not be needed after all, but anyway).  Elsewhere skip_what_now is
used only for "is it zero, or is it not zero?".  Comparing
explicitly with 1 only here makes readers suspect if assigning 2 or
70 to the variable has special meanings and wastes their brain
cycles.

Re: [RFC PATCH 1/1] add-patch: Allow reworking with a file after deciding on all its hunks

[Samuel Abraham]

On Fri, Jan 23, 2026 at 5:38 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Abraham Samuel Adekunle <abrahamadekunle50@gmail.com> writes:
>
> > After deciding on all hunks in a file, the interactive session
> > advances automatically to the next file if there is another,
> > or the process ends.
> >
> > Allow for reworking with a file by introducing a what_now prompt which
> > allows for navigating with J/K or advancing to the next file if there is one.
>
> Describe "how" you are allowing these new things that users used not
> to be able to do (no, not in the "by adding this variable and
> switching on its value" sense, but in the "now deciding on all the
> hunks in a file does not automatically advance to the next file, and
> the user has to do X to move forward" sense).

Thank you for your feedback Junio.

Okay, this is noted.

>
> > -     int colored = !!s->colored.len, quit = 0, use_pager = 0;
> > +     int colored = !!s->colored.len, quit = 0, use_pager = 0, skip_what_now = 0;
>
> This is getting overly long.  Wouldn't it be easier to follow if a
> preliminary patch split these existing variables into three
> independent definitions, and the main patch adds the fourth one?

Yes it will be easier to follow.
I will do that.

>
> > +                     if (s->file_diff_nr > 1)
> > +                             prompt_whatnow = _("What now? [J,K,q,>]? ");
> > +                     else
> > +                             prompt_whatnow = _("What now? [J,K,q]? ");
>
> I wonder if ">" has to be made so special.  Wouldn't it be easier to
> reason about the logic if ">" (and probably "<" to go back by one
> file) are added to the prompt in the same logic that decides 'g',
> 'k', 's', etc. should be shown using the "permitted" variable?

Yes this makes a lot of sense.
Thank you for the guidance.

>
> And when the inter-file navigation is in the permitted set (i.e.,
> there are multiple files involved), you'd show ">" (or "<", or both
> if you are dealing with the second file among three files) and ask,
> instead of silently moving to the next one, or something like that.

Yes I understand.

>
> Organizing the logic that way will also allow you to move to the
> next file _without_ first having to decide on all hunks in the
> current file.  Just say ">" to deal with the next file first, and
> after you are done, either come back with "<", or the system notices
> that there are undecided hunks in the earlier file and takes you
> back automatically.

This is very insightful.
I will work with this design in mind
Thank you

>
> I also have a hunch that with such a code structure you may not even
> need skip_what_now flag, but I haven't even written the code in my
> head, so if somebody tries to do so, they may discover the reason
> why such a flag is still needed.

Yes

>
> >               strbuf_reset(&s->buf);
> >               if (file_diff->hunk_nr) {
> > -                     if (rendered_hunk_index != hunk_index) {
> > +                     if (rendered_hunk_index != hunk_index || skip_what_now == 1) {
>
> Style (which may become irrelevant, as I just said the variable may
> not be needed after all, but anyway).  Elsewhere skip_what_now is
> used only for "is it zero, or is it not zero?".  Comparing
> explicitly with 1 only here makes readers suspect if assigning 2 or
> 70 to the variable has special meanings and wastes their brain
> cycles.

I will give more thoughtful efforts into the next versions I will send after
your recommendations.

Thank you very much

Abraham.

[PATCH v2 0/1] Allow reworking with a file when making hunk decisions

[Abraham Samuel Adekunle]

Hello,
After review and suggestions from Junio, I have been able to add the '<' and '>'
options for going to the previous file and next file respectively.
If there is only one file, neither of the options will be available, if we are in the
second of three or more file, both '<' and '>' will be available and if we are at the last file,
only '<' will be available.

This will enable simultaneous hunk decisions between between files.
After all decisions have been made in a file, a prompt shows which asks
"All hunks decided. What now?" that allows reworking with the file,
moving to the next or previous file as the case may be.

Since all hunks in the file have been decided, if the user navigates to a particular
hunk with 'K' or 'J' and redecides on an already decided hunk with options such as, 'y' or 'n',
the user is taken back to the first hunk with the "what now?" prompt shown.

The decision to use 'q' as a submit is because after some or all the decisions have been made
in a file, 'q' submits them as is even though in the `help_patch_text` it say `q` will
not stage the current hunk and all hunks after it. This is not true if hunks decisions
have been made and the user navigates with 'K' and 'J' or uses 'a' to select all hunks and
'q' after wards

I have not attempted to work on the t/3701-interactive.sh yet but this will be done
after concensus on the UI when all hunks have been decided.

Abraham Samuel Adekunle (1):
  Allow reworking with a file after deciding on all its hunks

 add-patch.c | 139 ++++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 102 insertions(+), 37 deletions(-)

-- 
2.39.5 (Apple Git-154)

[PATCH v2 1/1] Allow reworking with a file after deciding on all its hunks

[Abraham Samuel Adekunle]

After deciding on all hunks in a file, the interactive session
advances automatically to the next file if there is another,
or the process ends.

Now the process does not advance automatically. A user can choose to
go to the next file by pressing '>' or the previous file by pressing '<',
before or after deciding on all hunks in the current file.

After all hunks have been decided in a file, a prompt appears,
which allow the user to still rework with the file by applying
the options available in the permit set for that hunk, and
after all the decisions, the user presses 'q' to submit.

Signed-off-by: Abraham Samuel Adekunle <abrahamadekunle50@gmail.com>
---
Changes in v2:
=============
- Added '<' and '>' to the permit set
- All patches are now applied after all decisions in all files have been
  made by submitting with 'q'.

 add-patch.c | 139 ++++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 102 insertions(+), 37 deletions(-)

diff --git a/add-patch.c b/add-patch.c
index 173a53241e..edb2fab3fd 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -1418,6 +1418,8 @@ N_("j - go to the next undecided hunk, roll over at the bottom\n"
    "e - manually edit the current hunk\n"
    "p - print the current hunk\n"
    "P - print the current hunk using the pager\n"
+   "> - go to the next file\n"
+   "< - go to the previous file\n"
    "? - print help\n");
 
 static size_t dec_mod(size_t a, size_t m)
@@ -1441,6 +1443,17 @@ static bool get_first_undecided(const struct file_diff *file_diff, size_t *idx)
 	return false;
 }
 
+static size_t get_file_diff_index(struct add_p_state *s, struct file_diff *file_diff) {
+	size_t idx = 0;
+	for (size_t i = 0; i < s->file_diff_nr; i++) {
+		if (s->file_diff + i == file_diff) {
+			idx = i;
+			break;
+		}
+	}
+	return idx;
+}
+
 static int patch_update_file(struct add_p_state *s,
 			     struct file_diff *file_diff)
 {
@@ -1448,9 +1461,10 @@ static int patch_update_file(struct add_p_state *s,
 	ssize_t i, undecided_previous, undecided_next, rendered_hunk_index = -1;
 	struct hunk *hunk;
 	char ch;
-	struct child_process cp = CHILD_PROCESS_INIT;
 	int colored = !!s->colored.len, quit = 0, use_pager = 0;
 	enum prompt_mode_type prompt_mode_type;
+	size_t file_diff_index = get_file_diff_index(s, file_diff);
+	int all_decided = 0;
 
 	/* Empty added files have no hunks */
 	if (!file_diff->hunk_nr && !file_diff->added)
@@ -1467,7 +1481,9 @@ static int patch_update_file(struct add_p_state *s,
 			ALLOW_GOTO_NEXT_UNDECIDED_HUNK = 1 << 3,
 			ALLOW_SEARCH_AND_GOTO = 1 << 4,
 			ALLOW_SPLIT = 1 << 5,
-			ALLOW_EDIT = 1 << 6
+			ALLOW_EDIT = 1 << 6,
+			ALLOW_GOTO_PREVIOUS_FILE = 1 << 7,
+			ALLOW_GOTO_NEXT_FILE = 1 << 8
 		} permitted = 0;
 
 		if (hunk_index >= file_diff->hunk_nr)
@@ -1499,8 +1515,7 @@ static int patch_update_file(struct add_p_state *s,
 		/* Everything decided? */
 		if (undecided_previous < 0 && undecided_next < 0 &&
 		    hunk->use != UNDECIDED_HUNK)
-			break;
-
+				all_decided = 1;
 		strbuf_reset(&s->buf);
 		if (file_diff->hunk_nr) {
 			if (rendered_hunk_index != hunk_index) {
@@ -1548,6 +1563,16 @@ static int patch_update_file(struct add_p_state *s,
 				permitted |= ALLOW_EDIT;
 				strbuf_addstr(&s->buf, ",e");
 			}
+			if (file_diff_index >= 0 &&
+				file_diff_index < s->file_diff_nr - 1) {
+				permitted |= ALLOW_GOTO_NEXT_FILE;
+				strbuf_addstr(&s->buf, ",>");
+			}
+			if (file_diff_index > 0 &&
+				file_diff_index <= s->file_diff_nr - 1) {
+				permitted |= ALLOW_GOTO_PREVIOUS_FILE;
+				strbuf_addstr(&s->buf, ",<");
+			}
 			strbuf_addstr(&s->buf, ",p,P");
 		}
 		if (file_diff->deleted)
@@ -1566,6 +1591,9 @@ static int patch_update_file(struct add_p_state *s,
 						: 1));
 		printf(_(s->mode->prompt_mode[prompt_mode_type]),
 		       s->buf.buf);
+		if (all_decided)
+			printf(_("\n%s All hunks decided. What now? "),
+				s->s.prompt_color);
 		if (*s->s.reset_color_interactive)
 			fputs(s->s.reset_color_interactive, stdout);
 		fflush(stdout);
@@ -1618,7 +1646,24 @@ static int patch_update_file(struct add_p_state *s,
 		} else if (ch == 'q') {
 			quit = 1;
 			break;
-		} else if (s->answer.buf[0] == 'K') {
+		} else if (s->answer.buf[0] == '>') {
+			if (permitted & ALLOW_GOTO_NEXT_FILE) {
+				quit = 0;
+				break;
+			} else {
+				err(s, _("No next file"));
+				continue;
+			}
+		} else if (s->answer.buf[0] == '<') {
+			if (permitted & ALLOW_GOTO_PREVIOUS_FILE) {
+				quit = 2;
+				break;
+			} else {
+				err(s, _("No previous file"));
+				continue;
+			}
+		}
+		else if (s->answer.buf[0] == 'K') {
 			if (permitted & ALLOW_GOTO_PREVIOUS_HUNK)
 				hunk_index = dec_mod(hunk_index,
 						     file_diff->hunk_nr);
@@ -1775,33 +1820,6 @@ static int patch_update_file(struct add_p_state *s,
 		}
 	}
 
-	/* Any hunk to be used? */
-	for (i = 0; i < file_diff->hunk_nr; i++)
-		if (file_diff->hunk[i].use == USE_HUNK)
-			break;
-
-	if (i < file_diff->hunk_nr ||
-	    (!file_diff->hunk_nr && file_diff->head.use == USE_HUNK)) {
-		/* At least one hunk selected: apply */
-		strbuf_reset(&s->buf);
-		reassemble_patch(s, file_diff, 0, &s->buf);
-
-		discard_index(s->s.r->index);
-		if (s->mode->apply_for_checkout)
-			apply_for_checkout(s, &s->buf,
-					   s->mode->is_reverse);
-		else {
-			setup_child_process(s, &cp, "apply", NULL);
-			strvec_pushv(&cp.args, s->mode->apply_args);
-			if (pipe_command(&cp, s->buf.buf, s->buf.len,
-					 NULL, 0, NULL, 0))
-				error(_("'git apply' failed"));
-		}
-		if (repo_read_index(s->s.r) >= 0)
-			repo_refresh_and_write_index(s->s.r, REFRESH_QUIET, 0,
-						     1, NULL, NULL, NULL);
-	}
-
 	putchar('\n');
 	return quit;
 }
@@ -1813,7 +1831,9 @@ int run_add_p(struct repository *r, enum add_p_mode mode,
 	struct add_p_state s = {
 		{ r }, STRBUF_INIT, STRBUF_INIT, STRBUF_INIT, STRBUF_INIT
 	};
-	size_t i, binary_count = 0;
+	size_t i, j, binary_count = 0;
+	size_t patch_update_result = 0;
+	struct child_process cp = CHILD_PROCESS_INIT;
 
 	init_add_i_state(&s.s, r, o);
 
@@ -1852,11 +1872,56 @@ int run_add_p(struct repository *r, enum add_p_mode mode,
 		return -1;
 	}
 
-	for (i = 0; i < s.file_diff_nr; i++)
-		if (s.file_diff[i].binary && !s.file_diff[i].hunk_nr)
+	for (i = 0; i < s.file_diff_nr;) {
+		if (s.file_diff[i].binary && !s.file_diff[i].hunk_nr) {
 			binary_count++;
-		else if (patch_update_file(&s, s.file_diff + i))
-			break;
+			i++;
+			continue;
+		}
+		else {
+			patch_update_result = patch_update_file(&s, s.file_diff + i);
+			if (patch_update_result == 0) {
+				i++;
+				continue;
+			}
+			if (patch_update_result == 1)
+				break;
+			if (patch_update_result == 2) {
+				i--;
+				continue;
+			}
+		}
+	}
+	for (i = 0; i < s.file_diff_nr; i++) {
+
+			/* Any hunk to be used? */
+		for (j = 0; j < s.file_diff[i].hunk_nr; j++)
+			if (s.file_diff[i].hunk[j].use == USE_HUNK)
+				break;
+
+		if (j < s.file_diff[i].hunk_nr ||
+	    (!s.file_diff[i].hunk_nr && s.file_diff[i].head.use == USE_HUNK)) {
+			/* At least one hunk selected: apply */
+			strbuf_reset(&s.buf);
+			reassemble_patch(&s, s.file_diff + i, 0, &s.buf);
+
+			discard_index(s.s.r->index);
+			if (s.mode->apply_for_checkout)
+				apply_for_checkout(&s, &s.buf,
+						s.mode->is_reverse);
+			else {
+				setup_child_process(&s, &cp, "apply", NULL);
+				strvec_pushv(&cp.args, s.mode->apply_args);
+				if (pipe_command(&cp, s.buf.buf, s.buf.len,
+						NULL, 0, NULL, 0))
+					error(_("'git apply' failed"));
+			}
+			if (repo_read_index(s.s.r) >= 0)
+				repo_refresh_and_write_index(s.s.r, REFRESH_QUIET, 0,
+								1, NULL, NULL, NULL);
+		}
+
+	}
 
 	if (s.file_diff_nr == 0)
 		err(&s, _("No changes."));
-- 
2.39.5 (Apple Git-154)

Re: [PATCH v2 0/1] Allow reworking with a file when making hunk decisions

[Junio C Hamano]

Abraham Samuel Adekunle <abrahamadekunle50@gmail.com> writes:

> If there is only one file, neither of the options will be
> available, if we are in the second of three or more file, both '<'
> and '>' will be available and if we are at the last file, only '<'
> will be available.

An obvious alternative would be to treat the files as a ring, going
next from the last one would take you to the first one, etc., but I
think what you described is just as good.

> This will enable simultaneous hunk decisions between between files.
> After all decisions have been made in a file, a prompt shows which asks
> "All hunks decided. What now?" that allows reworking with the file,
> moving to the next or previous file as the case may be.

I forgot to mention this in the previous review, but this would be a
change that existing users may be surprised by.  We _might_ need to
introduce a flag to enable this as a new and optional feature.

> The decision to use 'q' as a submit is because after some or all
> the decisions have been made in a file, 'q' submits them as is
> even though in the `help_patch_text` it say `q` will not stage the
> current hunk and all hunks after it.

The users do need to _knowingly_ leave some hunks undecided and
apply what they already decided to use, and I think 'q' is an
appropriate option to use.  It is what the current system does,
and I do not think it changes with this new feature.

Thanks.

Re: [PATCH v2 1/1] Allow reworking with a file after deciding on all its hunks

[Junio C Hamano]

Abraham Samuel Adekunle <abrahamadekunle50@gmail.com> writes:

> diff --git a/add-patch.c b/add-patch.c
> index 173a53241e..edb2fab3fd 100644
> --- a/add-patch.c
> +++ b/add-patch.c
> @@ -1418,6 +1418,8 @@ N_("j - go to the next undecided hunk, roll over at the bottom\n"
>     "e - manually edit the current hunk\n"
>     "p - print the current hunk\n"
>     "P - print the current hunk using the pager\n"
> +   "> - go to the next file\n"
> +   "< - go to the previous file\n"
>     "? - print help\n");

As I said earlier, these may have to be optional.  It may give
existing users a jarring experience to be given a prompt after
deciding on all the hunks in a file, when they expect to be on
the next file already.

> @@ -1441,6 +1443,17 @@ static bool get_first_undecided(const struct file_diff *file_diff, size_t *idx)
>  	return false;
>  }
>  
> +static size_t get_file_diff_index(struct add_p_state *s, struct file_diff *file_diff) {
> +	size_t idx = 0;
> +	for (size_t i = 0; i < s->file_diff_nr; i++) {
> +		if (s->file_diff + i == file_diff) {
> +			idx = i;
> +			break;
> +		}
> +	}
> +	return idx;
> +}

Yuck.  Can't we lose the need for this function if we change the
interface into patch_update_file so that it takes the index of the
file (i.e., instead of "&s.file_diff[i]", pass "i")?  There is only
one caller to patch_update_file() which is run_add_p(), so such a
clean-up should be trivial.

>  static int patch_update_file(struct add_p_state *s,
>  			     struct file_diff *file_diff)
>  {
> @@ -1448,9 +1461,10 @@ static int patch_update_file(struct add_p_state *s,
>  	ssize_t i, undecided_previous, undecided_next, rendered_hunk_index = -1;
>  	struct hunk *hunk;
>  	char ch;
> -	struct child_process cp = CHILD_PROCESS_INIT;

This is related to the hoisting of the actual patch application to
the caller, but it is not explained why such a change is needed, and
it byitself, even without the "jump to the next file before deciding
on all the hunks" feature.  What problem is it solving???

If it is necessary to move the code to run "git apply" to the
caller, would it make sense to split this patch into at least two
patches, one to do such a move, possibly another patch to change the
function signature of patch_update_file() so that it takes the file
index instead of file_diff struct, and finally another patch to
allow jumping around the files?

>  	int colored = !!s->colored.len, quit = 0, use_pager = 0;
>  	enum prompt_mode_type prompt_mode_type;
> +	size_t file_diff_index = get_file_diff_index(s, file_diff);
> +	int all_decided = 0;
>  
>  	/* Empty added files have no hunks */
>  	if (!file_diff->hunk_nr && !file_diff->added)
> @@ -1467,7 +1481,9 @@ static int patch_update_file(struct add_p_state *s,
>  			ALLOW_GOTO_NEXT_UNDECIDED_HUNK = 1 << 3,
>  			ALLOW_SEARCH_AND_GOTO = 1 << 4,
>  			ALLOW_SPLIT = 1 << 5,
> -			ALLOW_EDIT = 1 << 6
> +			ALLOW_EDIT = 1 << 6,
> +			ALLOW_GOTO_PREVIOUS_FILE = 1 << 7,
> +			ALLOW_GOTO_NEXT_FILE = 1 << 8
>  		} permitted = 0;
>  
>  		if (hunk_index >= file_diff->hunk_nr)
> @@ -1499,8 +1515,7 @@ static int patch_update_file(struct add_p_state *s,
>  		/* Everything decided? */
>  		if (undecided_previous < 0 && undecided_next < 0 &&
>  		    hunk->use != UNDECIDED_HUNK)
> -			break;
> -
> +				all_decided = 1;
>  		strbuf_reset(&s->buf);
>  		if (file_diff->hunk_nr) {
>  			if (rendered_hunk_index != hunk_index) {
> @@ -1548,6 +1563,16 @@ static int patch_update_file(struct add_p_state *s,
>  				permitted |= ALLOW_EDIT;
>  				strbuf_addstr(&s->buf, ",e");
>  			}
> +			if (file_diff_index >= 0 &&
> +				file_diff_index < s->file_diff_nr - 1) {
> +				permitted |= ALLOW_GOTO_NEXT_FILE;
> +				strbuf_addstr(&s->buf, ",>");
> +			}
> +			if (file_diff_index > 0 &&
> +				file_diff_index <= s->file_diff_nr - 1) {
> +				permitted |= ALLOW_GOTO_PREVIOUS_FILE;
> +				strbuf_addstr(&s->buf, ",<");
> +			}

As can be seen in what patch_update_file() does when the user says
'J' or 'K', hunks in a file are treated as a ring, and these
commands are enabled as long as there are more than one hunks.

Perhaps that is more familiar than "when we hit the floor, we cannot
sink deeper, and when we hit the ceiling, we cannot float more",
which seems to be what the above implements.

>  			strbuf_addstr(&s->buf, ",p,P");
>  		}
>  		if (file_diff->deleted)
> @@ -1566,6 +1591,9 @@ static int patch_update_file(struct add_p_state *s,
>  						: 1));
>  		printf(_(s->mode->prompt_mode[prompt_mode_type]),
>  		       s->buf.buf);
> +		if (all_decided)
> +			printf(_("\n%s All hunks decided. What now? "),
> +				s->s.prompt_color);
>  		if (*s->s.reset_color_interactive)
>  			fputs(s->s.reset_color_interactive, stdout);
>  		fflush(stdout);
> @@ -1618,7 +1646,24 @@ static int patch_update_file(struct add_p_state *s,
>  		} else if (ch == 'q') {
>  			quit = 1;
>  			break;
> -		} else if (s->answer.buf[0] == 'K') {
> +		} else if (s->answer.buf[0] == '>') {
> +			if (permitted & ALLOW_GOTO_NEXT_FILE) {
> +				quit = 0;
> +				break;
> +			} else {
> +				err(s, _("No next file"));
> +				continue;
> +			}
> +		} else if (s->answer.buf[0] == '<') {
> +			if (permitted & ALLOW_GOTO_PREVIOUS_FILE) {
> +				quit = 2;
> +				break;

What's the magic number "2"?  Should "quit" become an enum with
elements that are more meaningfully named?

> +			} else {
> +				err(s, _("No previous file"));
> +				continue;
> +			}
> +		}
> +		else if (s->answer.buf[0] == 'K') {
>  			if (permitted & ALLOW_GOTO_PREVIOUS_HUNK)
>  				hunk_index = dec_mod(hunk_index,
>  						     file_diff->hunk_nr);
> @@ -1775,33 +1820,6 @@ static int patch_update_file(struct add_p_state *s,
>  		}
>  	}
>  
> -	/* Any hunk to be used? */
> -	for (i = 0; i < file_diff->hunk_nr; i++)
> -		if (file_diff->hunk[i].use == USE_HUNK)
> -			break;
> -
> -	if (i < file_diff->hunk_nr ||
> -	    (!file_diff->hunk_nr && file_diff->head.use == USE_HUNK)) {
> -		/* At least one hunk selected: apply */
> -		strbuf_reset(&s->buf);
> -		reassemble_patch(s, file_diff, 0, &s->buf);
> -
> -		discard_index(s->s.r->index);
> -		if (s->mode->apply_for_checkout)
> -			apply_for_checkout(s, &s->buf,
> -					   s->mode->is_reverse);
> -		else {
> -			setup_child_process(s, &cp, "apply", NULL);
> -			strvec_pushv(&cp.args, s->mode->apply_args);
> -			if (pipe_command(&cp, s->buf.buf, s->buf.len,
> -					 NULL, 0, NULL, 0))
> -				error(_("'git apply' failed"));
> -		}
> -		if (repo_read_index(s->s.r) >= 0)
> -			repo_refresh_and_write_index(s->s.r, REFRESH_QUIET, 0,
> -						     1, NULL, NULL, NULL);
> -	}

It is not obvious why the above code needs to be hoisted to the
caller.  

>  	putchar('\n');
>  	return quit;
>  }
> @@ -1813,7 +1831,9 @@ int run_add_p(struct repository *r, enum add_p_mode mode,
>  	struct add_p_state s = {
>  		{ r }, STRBUF_INIT, STRBUF_INIT, STRBUF_INIT, STRBUF_INIT
>  	};
> -	size_t i, binary_count = 0;
> +	size_t i, j, binary_count = 0;
> +	size_t patch_update_result = 0;

Hmph, I think patch_update_file() returns "int quit".  Why do we
want overly wide type to store the result, which cannot even express
negative number to potentially signal a failure?

> +	struct child_process cp = CHILD_PROCESS_INIT;
>  
>  	init_add_i_state(&s.s, r, o);
>  
> @@ -1852,11 +1872,56 @@ int run_add_p(struct repository *r, enum add_p_mode mode,
>  		return -1;
>  	}
>  
> -	for (i = 0; i < s.file_diff_nr; i++)
> -		if (s.file_diff[i].binary && !s.file_diff[i].hunk_nr)
> +	for (i = 0; i < s.file_diff_nr;) {
> +		if (s.file_diff[i].binary && !s.file_diff[i].hunk_nr) {
>  			binary_count++;
> -		else if (patch_update_file(&s, s.file_diff + i))
> -			break;
> +			i++;
> +			continue;
> +		}
> +		else {
> +			patch_update_result = patch_update_file(&s, s.file_diff + i);
> +			if (patch_update_result == 0) {
> +				i++;
> +				continue;
> +			}
> +			if (patch_update_result == 1)
> +				break;
> +			if (patch_update_result == 2) {
> +				i--;
> +				continue;
> +			}
> +		}
> +	}
> +	for (i = 0; i < s.file_diff_nr; i++) {
> +
> +			/* Any hunk to be used? */
> +		for (j = 0; j < s.file_diff[i].hunk_nr; j++)
> +			if (s.file_diff[i].hunk[j].use == USE_HUNK)
> +				break;
> +
> +		if (j < s.file_diff[i].hunk_nr ||
> +	    (!s.file_diff[i].hunk_nr && s.file_diff[i].head.use == USE_HUNK)) {
> +			/* At least one hunk selected: apply */
> +			strbuf_reset(&s.buf);
> +			reassemble_patch(&s, s.file_diff + i, 0, &s.buf);
> +
> +			discard_index(s.s.r->index);
> +			if (s.mode->apply_for_checkout)
> +				apply_for_checkout(&s, &s.buf,
> +						s.mode->is_reverse);
> +			else {
> +				setup_child_process(&s, &cp, "apply", NULL);
> +				strvec_pushv(&cp.args, s.mode->apply_args);
> +				if (pipe_command(&cp, s.buf.buf, s.buf.len,
> +						NULL, 0, NULL, 0))
> +					error(_("'git apply' failed"));
> +			}
> +			if (repo_read_index(s.s.r) >= 0)
> +				repo_refresh_and_write_index(s.s.r, REFRESH_QUIET, 0,
> +								1, NULL, NULL, NULL);
> +		}
> +
> +	}

One upside of having "git apply" at the end of patch_update_file()
is that you can "^C" out of "git add -p" or your terminal connection
can be cut off, after dealing with hunks in a few early files, and
these early part of your work that you have already done are already
reflected to the working tree files.  By hoisting the logic to the
caller, this is making the update all-or-none, which is good in
transactional systems, but can make a horrible experience for an
interactive use where you make progress while thinking.

So I am not yet convinced if this change makes sense---it could be
because of the lack of justification for this change.



>  
>  	if (s.file_diff_nr == 0)
>  		err(&s, _("No changes."));

Re: [PATCH v2 0/1] Allow reworking with a file when making hunk decisions

[Samuel Abraham]

On Tue, Jan 27, 2026 at 6:04 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Abraham Samuel Adekunle <abrahamadekunle50@gmail.com> writes:
>
> > If there is only one file, neither of the options will be
> > available, if we are in the second of three or more file, both '<'
> > and '>' will be available and if we are at the last file, only '<'
> > will be available.
>
> An obvious alternative would be to treat the files as a ring, going
> next from the last one would take you to the first one, etc., but I
> think what you described is just as good.

Okay I will work that

>
> > This will enable simultaneous hunk decisions between between files.
> > After all decisions have been made in a file, a prompt shows which asks
> > "All hunks decided. What now?" that allows reworking with the file,
> > moving to the next or previous file as the case may be.
>
> I forgot to mention this in the previous review, but this would be a
> change that existing users may be surprised by.  We _might_ need to
> introduce a flag to enable this as a new and optional feature.

Yes I thought about this too and it is a good idea.
Thanks

>
> > The decision to use 'q' as a submit is because after some or all
> > the decisions have been made in a file, 'q' submits them as is
> > even though in the `help_patch_text` it say `q` will not stage the
> > current hunk and all hunks after it.
>
> The users do need to _knowingly_ leave some hunks undecided and
> apply what they already decided to use, and I think 'q' is an
> appropriate option to use.  It is what the current system does,
> and I do not think it changes with this new feature.

Okay thank you.

Re: [PATCH v2 1/1] Allow reworking with a file after deciding on all its hunks

[Samuel Abraham]

On Tue, Jan 27, 2026 at 9:48 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Abraham Samuel Adekunle <abrahamadekunle50@gmail.com> writes:
>
> > diff --git a/add-patch.c b/add-patch.c
> > index 173a53241e..edb2fab3fd 100644
> > --- a/add-patch.c
> > +++ b/add-patch.c
> > @@ -1418,6 +1418,8 @@ N_("j - go to the next undecided hunk, roll over at the bottom\n"
> >     "e - manually edit the current hunk\n"
> >     "p - print the current hunk\n"
> >     "P - print the current hunk using the pager\n"
> > +   "> - go to the next file\n"
> > +   "< - go to the previous file\n"
> >     "? - print help\n");
>
> As I said earlier, these may have to be optional.  It may give
> existing users a jarring experience to be given a prompt after
> deciding on all the hunks in a file, when they expect to be on
> the next file already.

Yes I agree.
I will work on making it an optional feature.

>
> > @@ -1441,6 +1443,17 @@ static bool get_first_undecided(const struct file_diff *file_diff, size_t *idx)
> >       return false;
> >  }
> >
> > +static size_t get_file_diff_index(struct add_p_state *s, struct file_diff *file_diff) {
> > +     size_t idx = 0;
> > +     for (size_t i = 0; i < s->file_diff_nr; i++) {
> > +             if (s->file_diff + i == file_diff) {
> > +                     idx = i;
> > +                     break;
> > +             }
> > +     }
> > +     return idx;
> > +}
>
> Yuck.  Can't we lose the need for this function if we change the
> interface into patch_update_file so that it takes the index of the
> file (i.e., instead of "&s.file_diff[i]", pass "i")?  There is only
> one caller to patch_update_file() which is run_add_p(), so such a
> clean-up should be trivial.

Ah yes this is definitely a sweet and better option.

>
> >  static int patch_update_file(struct add_p_state *s,
> >                            struct file_diff *file_diff)
> >  {
> > @@ -1448,9 +1461,10 @@ static int patch_update_file(struct add_p_state *s,
> >       ssize_t i, undecided_previous, undecided_next, rendered_hunk_index = -1;
> >       struct hunk *hunk;
> >       char ch;
> > -     struct child_process cp = CHILD_PROCESS_INIT;
>
> This is related to the hoisting of the actual patch application to
> the caller, but it is not explained why such a change is needed, and
> it byitself, even without the "jump to the next file before deciding
> on all the hunks" feature.  What problem is it solving???

I explained this below

>
> If it is necessary to move the code to run "git apply" to the
> caller, would it make sense to split this patch into at least two
> patches, one to do such a move, possibly another patch to change the
> function signature of patch_update_file() so that it takes the file
> index instead of file_diff struct, and finally another patch to
> allow jumping around the files?

Okay yes it would make much sense.

>
> >       int colored = !!s->colored.len, quit = 0, use_pager = 0;
> >       enum prompt_mode_type prompt_mode_type;
> > +     size_t file_diff_index = get_file_diff_index(s, file_diff);
> > +     int all_decided = 0;
> >
> >       /* Empty added files have no hunks */
> >       if (!file_diff->hunk_nr && !file_diff->added)
> > @@ -1467,7 +1481,9 @@ static int patch_update_file(struct add_p_state *s,
> >                       ALLOW_GOTO_NEXT_UNDECIDED_HUNK = 1 << 3,
> >                       ALLOW_SEARCH_AND_GOTO = 1 << 4,
> >                       ALLOW_SPLIT = 1 << 5,
> > -                     ALLOW_EDIT = 1 << 6
> > +                     ALLOW_EDIT = 1 << 6,
> > +                     ALLOW_GOTO_PREVIOUS_FILE = 1 << 7,
> > +                     ALLOW_GOTO_NEXT_FILE = 1 << 8
> >               } permitted = 0;
> >
> >               if (hunk_index >= file_diff->hunk_nr)
> > @@ -1499,8 +1515,7 @@ static int patch_update_file(struct add_p_state *s,
> >               /* Everything decided? */
> >               if (undecided_previous < 0 && undecided_next < 0 &&
> >                   hunk->use != UNDECIDED_HUNK)
> > -                     break;
> > -
> > +                             all_decided = 1;
> >               strbuf_reset(&s->buf);
> >               if (file_diff->hunk_nr) {
> >                       if (rendered_hunk_index != hunk_index) {
> > @@ -1548,6 +1563,16 @@ static int patch_update_file(struct add_p_state *s,
> >                               permitted |= ALLOW_EDIT;
> >                               strbuf_addstr(&s->buf, ",e");
> >                       }
> > +                     if (file_diff_index >= 0 &&
> > +                             file_diff_index < s->file_diff_nr - 1) {
> > +                             permitted |= ALLOW_GOTO_NEXT_FILE;
> > +                             strbuf_addstr(&s->buf, ",>");
> > +                     }
> > +                     if (file_diff_index > 0 &&
> > +                             file_diff_index <= s->file_diff_nr - 1) {
> > +                             permitted |= ALLOW_GOTO_PREVIOUS_FILE;
> > +                             strbuf_addstr(&s->buf, ",<");
> > +                     }
>
> As can be seen in what patch_update_file() does when the user says
> 'J' or 'K', hunks in a file are treated as a ring, and these
> commands are enabled as long as there are more than one hunks.
>
> Perhaps that is more familiar than "when we hit the floor, we cannot
> sink deeper, and when we hit the ceiling, we cannot float more",
> which seems to be what the above implements.

Yes I understand this now.
It does make sense this way.

>
> >                       strbuf_addstr(&s->buf, ",p,P");
> >               }
> >               if (file_diff->deleted)
> > @@ -1566,6 +1591,9 @@ static int patch_update_file(struct add_p_state *s,
> >                                               : 1));
> >               printf(_(s->mode->prompt_mode[prompt_mode_type]),
> >                      s->buf.buf);
> > +             if (all_decided)
> > +                     printf(_("\n%s All hunks decided. What now? "),
> > +                             s->s.prompt_color);
> >               if (*s->s.reset_color_interactive)
> >                       fputs(s->s.reset_color_interactive, stdout);
> >               fflush(stdout);
> > @@ -1618,7 +1646,24 @@ static int patch_update_file(struct add_p_state *s,
> >               } else if (ch == 'q') {
> >                       quit = 1;
> >                       break;
> > -             } else if (s->answer.buf[0] == 'K') {
> > +             } else if (s->answer.buf[0] == '>') {
> > +                     if (permitted & ALLOW_GOTO_NEXT_FILE) {
> > +                             quit = 0;
> > +                             break;
> > +                     } else {
> > +                             err(s, _("No next file"));
> > +                             continue;
> > +                     }
> > +             } else if (s->answer.buf[0] == '<') {
> > +                     if (permitted & ALLOW_GOTO_PREVIOUS_FILE) {
> > +                             quit = 2;
> > +                             break;
>
> What's the magic number "2"?  Should "quit" become an enum with
> elements that are more meaningfully named?

Okay, yes an enum would be better.

>
> > +                     } else {
> > +                             err(s, _("No previous file"));
> > +                             continue;
> > +                     }
> > +             }
> > +             else if (s->answer.buf[0] == 'K') {
> >                       if (permitted & ALLOW_GOTO_PREVIOUS_HUNK)
> >                               hunk_index = dec_mod(hunk_index,
> >                                                    file_diff->hunk_nr);
> > @@ -1775,33 +1820,6 @@ static int patch_update_file(struct add_p_state *s,
> >               }
> >       }
> >
> > -     /* Any hunk to be used? */
> > -     for (i = 0; i < file_diff->hunk_nr; i++)
> > -             if (file_diff->hunk[i].use == USE_HUNK)
> > -                     break;
> > -
> > -     if (i < file_diff->hunk_nr ||
> > -         (!file_diff->hunk_nr && file_diff->head.use == USE_HUNK)) {
> > -             /* At least one hunk selected: apply */
> > -             strbuf_reset(&s->buf);
> > -             reassemble_patch(s, file_diff, 0, &s->buf);
> > -
> > -             discard_index(s->s.r->index);
> > -             if (s->mode->apply_for_checkout)
> > -                     apply_for_checkout(s, &s->buf,
> > -                                        s->mode->is_reverse);
> > -             else {
> > -                     setup_child_process(s, &cp, "apply", NULL);
> > -                     strvec_pushv(&cp.args, s->mode->apply_args);
> > -                     if (pipe_command(&cp, s->buf.buf, s->buf.len,
> > -                                      NULL, 0, NULL, 0))
> > -                             error(_("'git apply' failed"));
> > -             }
> > -             if (repo_read_index(s->s.r) >= 0)
> > -                     repo_refresh_and_write_index(s->s.r, REFRESH_QUIET, 0,
> > -                                                  1, NULL, NULL, NULL);
> > -     }
>
> It is not obvious why the above code needs to be hoisted to the
> caller.

I explained this below.

>
> >       putchar('\n');
> >       return quit;
> >  }
> > @@ -1813,7 +1831,9 @@ int run_add_p(struct repository *r, enum add_p_mode mode,
> >       struct add_p_state s = {
> >               { r }, STRBUF_INIT, STRBUF_INIT, STRBUF_INIT, STRBUF_INIT
> >       };
> > -     size_t i, binary_count = 0;
> > +     size_t i, j, binary_count = 0;
> > +     size_t patch_update_result = 0;
>
> Hmph, I think patch_update_file() returns "int quit".  Why do we
> want overly wide type to store the result, which cannot even express
> negative number to potentially signal a failure?

Sorry, this is a mistake on my part

>
> > +     struct child_process cp = CHILD_PROCESS_INIT;
> >
> >       init_add_i_state(&s.s, r, o);
> >
> > @@ -1852,11 +1872,56 @@ int run_add_p(struct repository *r, enum add_p_mode mode,
> >               return -1;
> >       }
> >
> > -     for (i = 0; i < s.file_diff_nr; i++)
> > -             if (s.file_diff[i].binary && !s.file_diff[i].hunk_nr)
> > +     for (i = 0; i < s.file_diff_nr;) {
> > +             if (s.file_diff[i].binary && !s.file_diff[i].hunk_nr) {
> >                       binary_count++;
> > -             else if (patch_update_file(&s, s.file_diff + i))
> > -                     break;
> > +                     i++;
> > +                     continue;
> > +             }
> > +             else {
> > +                     patch_update_result = patch_update_file(&s, s.file_diff + i);
> > +                     if (patch_update_result == 0) {
> > +                             i++;
> > +                             continue;
> > +                     }
> > +                     if (patch_update_result == 1)
> > +                             break;
> > +                     if (patch_update_result == 2) {
> > +                             i--;
> > +                             continue;
> > +                     }
> > +             }
> > +     }
> > +     for (i = 0; i < s.file_diff_nr; i++) {
> > +
> > +                     /* Any hunk to be used? */
> > +             for (j = 0; j < s.file_diff[i].hunk_nr; j++)
> > +                     if (s.file_diff[i].hunk[j].use == USE_HUNK)
> > +                             break;
> > +
> > +             if (j < s.file_diff[i].hunk_nr ||
> > +         (!s.file_diff[i].hunk_nr && s.file_diff[i].head.use == USE_HUNK)) {
> > +                     /* At least one hunk selected: apply */
> > +                     strbuf_reset(&s.buf);
> > +                     reassemble_patch(&s, s.file_diff + i, 0, &s.buf);
> > +
> > +                     discard_index(s.s.r->index);
> > +                     if (s.mode->apply_for_checkout)
> > +                             apply_for_checkout(&s, &s.buf,
> > +                                             s.mode->is_reverse);
> > +                     else {
> > +                             setup_child_process(&s, &cp, "apply", NULL);
> > +                             strvec_pushv(&cp.args, s.mode->apply_args);
> > +                             if (pipe_command(&cp, s.buf.buf, s.buf.len,
> > +                                             NULL, 0, NULL, 0))
> > +                                     error(_("'git apply' failed"));
> > +                     }
> > +                     if (repo_read_index(s.s.r) >= 0)
> > +                             repo_refresh_and_write_index(s.s.r, REFRESH_QUIET, 0,
> > +                                                             1, NULL, NULL, NULL);
> > +             }
> > +
> > +     }
>
> One upside of having "git apply" at the end of patch_update_file()
> is that you can "^C" out of "git add -p" or your terminal connection
> can be cut off, after dealing with hunks in a few early files, and
> these early part of your work that you have already done are already
> reflected to the working tree files.  By hoisting the logic to the
> caller, this is making the update all-or-none, which is good in
> transactional systems, but can make a horrible experience for an
> interactive use where you make progress while thinking.
>
> So I am not yet convinced if this change makes sense---it could be
> because of the lack of justification for this change.

What I observed after adding the '>' and '<' options is that if a user chooses
to use a hunk A in file 1, and then goes to file 2 with '>', comes back to
file 1 with '<', and decides on hunk A to skip it instead, because
patch_update_file() has
applied the file with the hunk the user initially decided to use
before proceeding to file
2 with '>', coming back to redecide and say skip does not apply the
latest decision
and when you check the index, the file with the hunks which the user
initially decided to
use but changed to skip is present in the index.

But if the user initially decided to skip a hunk in a file, goes to
the next file with '>'
and back to the first file, changes the decision on the hunk to use,
it applies the patch
with the hunk because the hunk was not initially selected when the
patch was applied.
But if he now goes away and comes back to the file a third time and
chooses to skip the
hunk, then quits with 'q', because he had selected to use the hunk the
second time,
choosing skip again will not work.

So basically, initially choosing to use a hunk in a file, going to
another file and coming
back to this file then choosing to skip it does not register the
latest skip decision
on that hunk.

That was why I decided to do it this way.
I will appreciate a better suggestion from you

Thanks
Abraham.

Re: [PATCH v2 1/1] Allow reworking with a file after deciding on all its hunks

[Samuel Abraham]

On Wed, Jan 28, 2026 at 12:26 PM Samuel Abraham
<abrahamadekunle50@gmail.com> wrote:
>
> On Tue, Jan 27, 2026 at 9:48 PM Junio C Hamano <gitster@pobox.com> wrote:
> >
> > Abraham Samuel Adekunle <abrahamadekunle50@gmail.com> writes:
> >
> > > diff --git a/add-patch.c b/add-patch.c
> > > index 173a53241e..edb2fab3fd 100644
> > > --- a/add-patch.c
> > > +++ b/add-patch.c
> > > @@ -1418,6 +1418,8 @@ N_("j - go to the next undecided hunk, roll over at the bottom\n"
> > >     "e - manually edit the current hunk\n"
> > >     "p - print the current hunk\n"
> > >     "P - print the current hunk using the pager\n"
> > > +   "> - go to the next file\n"
> > > +   "< - go to the previous file\n"
> > >     "? - print help\n");
> >
> > As I said earlier, these may have to be optional.  It may give
> > existing users a jarring experience to be given a prompt after
> > deciding on all the hunks in a file, when they expect to be on
> > the next file already.
>
> Yes I agree.
> I will work on making it an optional feature.
>
> >
> > > @@ -1441,6 +1443,17 @@ static bool get_first_undecided(const struct file_diff *file_diff, size_t *idx)
> > >       return false;
> > >  }
> > >
> > > +static size_t get_file_diff_index(struct add_p_state *s, struct file_diff *file_diff) {
> > > +     size_t idx = 0;
> > > +     for (size_t i = 0; i < s->file_diff_nr; i++) {
> > > +             if (s->file_diff + i == file_diff) {
> > > +                     idx = i;
> > > +                     break;
> > > +             }
> > > +     }
> > > +     return idx;
> > > +}
> >
> > Yuck.  Can't we lose the need for this function if we change the
> > interface into patch_update_file so that it takes the index of the
> > file (i.e., instead of "&s.file_diff[i]", pass "i")?  There is only
> > one caller to patch_update_file() which is run_add_p(), so such a
> > clean-up should be trivial.
>
> Ah yes this is definitely a sweet and better option.
>
> >
> > >  static int patch_update_file(struct add_p_state *s,
> > >                            struct file_diff *file_diff)
> > >  {
> > > @@ -1448,9 +1461,10 @@ static int patch_update_file(struct add_p_state *s,
> > >       ssize_t i, undecided_previous, undecided_next, rendered_hunk_index = -1;
> > >       struct hunk *hunk;
> > >       char ch;
> > > -     struct child_process cp = CHILD_PROCESS_INIT;
> >
> > This is related to the hoisting of the actual patch application to
> > the caller, but it is not explained why such a change is needed, and
> > it byitself, even without the "jump to the next file before deciding
> > on all the hunks" feature.  What problem is it solving???
>
> I explained this below
>
> >
> > If it is necessary to move the code to run "git apply" to the
> > caller, would it make sense to split this patch into at least two
> > patches, one to do such a move, possibly another patch to change the
> > function signature of patch_update_file() so that it takes the file
> > index instead of file_diff struct, and finally another patch to
> > allow jumping around the files?
>
> Okay yes it would make much sense.
>
> >
> > >       int colored = !!s->colored.len, quit = 0, use_pager = 0;
> > >       enum prompt_mode_type prompt_mode_type;
> > > +     size_t file_diff_index = get_file_diff_index(s, file_diff);
> > > +     int all_decided = 0;
> > >
> > >       /* Empty added files have no hunks */
> > >       if (!file_diff->hunk_nr && !file_diff->added)
> > > @@ -1467,7 +1481,9 @@ static int patch_update_file(struct add_p_state *s,
> > >                       ALLOW_GOTO_NEXT_UNDECIDED_HUNK = 1 << 3,
> > >                       ALLOW_SEARCH_AND_GOTO = 1 << 4,
> > >                       ALLOW_SPLIT = 1 << 5,
> > > -                     ALLOW_EDIT = 1 << 6
> > > +                     ALLOW_EDIT = 1 << 6,
> > > +                     ALLOW_GOTO_PREVIOUS_FILE = 1 << 7,
> > > +                     ALLOW_GOTO_NEXT_FILE = 1 << 8
> > >               } permitted = 0;
> > >
> > >               if (hunk_index >= file_diff->hunk_nr)
> > > @@ -1499,8 +1515,7 @@ static int patch_update_file(struct add_p_state *s,
> > >               /* Everything decided? */
> > >               if (undecided_previous < 0 && undecided_next < 0 &&
> > >                   hunk->use != UNDECIDED_HUNK)
> > > -                     break;
> > > -
> > > +                             all_decided = 1;
> > >               strbuf_reset(&s->buf);
> > >               if (file_diff->hunk_nr) {
> > >                       if (rendered_hunk_index != hunk_index) {
> > > @@ -1548,6 +1563,16 @@ static int patch_update_file(struct add_p_state *s,
> > >                               permitted |= ALLOW_EDIT;
> > >                               strbuf_addstr(&s->buf, ",e");
> > >                       }
> > > +                     if (file_diff_index >= 0 &&
> > > +                             file_diff_index < s->file_diff_nr - 1) {
> > > +                             permitted |= ALLOW_GOTO_NEXT_FILE;
> > > +                             strbuf_addstr(&s->buf, ",>");
> > > +                     }
> > > +                     if (file_diff_index > 0 &&
> > > +                             file_diff_index <= s->file_diff_nr - 1) {
> > > +                             permitted |= ALLOW_GOTO_PREVIOUS_FILE;
> > > +                             strbuf_addstr(&s->buf, ",<");
> > > +                     }
> >
> > As can be seen in what patch_update_file() does when the user says
> > 'J' or 'K', hunks in a file are treated as a ring, and these
> > commands are enabled as long as there are more than one hunks.
> >
> > Perhaps that is more familiar than "when we hit the floor, we cannot
> > sink deeper, and when we hit the ceiling, we cannot float more",
> > which seems to be what the above implements.
>
> Yes I understand this now.
> It does make sense this way.
>
> >
> > >                       strbuf_addstr(&s->buf, ",p,P");
> > >               }
> > >               if (file_diff->deleted)
> > > @@ -1566,6 +1591,9 @@ static int patch_update_file(struct add_p_state *s,
> > >                                               : 1));
> > >               printf(_(s->mode->prompt_mode[prompt_mode_type]),
> > >                      s->buf.buf);
> > > +             if (all_decided)
> > > +                     printf(_("\n%s All hunks decided. What now? "),
> > > +                             s->s.prompt_color);
> > >               if (*s->s.reset_color_interactive)
> > >                       fputs(s->s.reset_color_interactive, stdout);
> > >               fflush(stdout);
> > > @@ -1618,7 +1646,24 @@ static int patch_update_file(struct add_p_state *s,
> > >               } else if (ch == 'q') {
> > >                       quit = 1;
> > >                       break;
> > > -             } else if (s->answer.buf[0] == 'K') {
> > > +             } else if (s->answer.buf[0] == '>') {
> > > +                     if (permitted & ALLOW_GOTO_NEXT_FILE) {
> > > +                             quit = 0;
> > > +                             break;
> > > +                     } else {
> > > +                             err(s, _("No next file"));
> > > +                             continue;
> > > +                     }
> > > +             } else if (s->answer.buf[0] == '<') {
> > > +                     if (permitted & ALLOW_GOTO_PREVIOUS_FILE) {
> > > +                             quit = 2;
> > > +                             break;
> >
> > What's the magic number "2"?  Should "quit" become an enum with
> > elements that are more meaningfully named?
>
> Okay, yes an enum would be better.
>
> >
> > > +                     } else {
> > > +                             err(s, _("No previous file"));
> > > +                             continue;
> > > +                     }
> > > +             }
> > > +             else if (s->answer.buf[0] == 'K') {
> > >                       if (permitted & ALLOW_GOTO_PREVIOUS_HUNK)
> > >                               hunk_index = dec_mod(hunk_index,
> > >                                                    file_diff->hunk_nr);
> > > @@ -1775,33 +1820,6 @@ static int patch_update_file(struct add_p_state *s,
> > >               }
> > >       }
> > >
> > > -     /* Any hunk to be used? */
> > > -     for (i = 0; i < file_diff->hunk_nr; i++)
> > > -             if (file_diff->hunk[i].use == USE_HUNK)
> > > -                     break;
> > > -
> > > -     if (i < file_diff->hunk_nr ||
> > > -         (!file_diff->hunk_nr && file_diff->head.use == USE_HUNK)) {
> > > -             /* At least one hunk selected: apply */
> > > -             strbuf_reset(&s->buf);
> > > -             reassemble_patch(s, file_diff, 0, &s->buf);
> > > -
> > > -             discard_index(s->s.r->index);
> > > -             if (s->mode->apply_for_checkout)
> > > -                     apply_for_checkout(s, &s->buf,
> > > -                                        s->mode->is_reverse);
> > > -             else {
> > > -                     setup_child_process(s, &cp, "apply", NULL);
> > > -                     strvec_pushv(&cp.args, s->mode->apply_args);
> > > -                     if (pipe_command(&cp, s->buf.buf, s->buf.len,
> > > -                                      NULL, 0, NULL, 0))
> > > -                             error(_("'git apply' failed"));
> > > -             }
> > > -             if (repo_read_index(s->s.r) >= 0)
> > > -                     repo_refresh_and_write_index(s->s.r, REFRESH_QUIET, 0,
> > > -                                                  1, NULL, NULL, NULL);
> > > -     }
> >
> > It is not obvious why the above code needs to be hoisted to the
> > caller.
>
> I explained this below.
>
> >
> > >       putchar('\n');
> > >       return quit;
> > >  }
> > > @@ -1813,7 +1831,9 @@ int run_add_p(struct repository *r, enum add_p_mode mode,
> > >       struct add_p_state s = {
> > >               { r }, STRBUF_INIT, STRBUF_INIT, STRBUF_INIT, STRBUF_INIT
> > >       };
> > > -     size_t i, binary_count = 0;
> > > +     size_t i, j, binary_count = 0;
> > > +     size_t patch_update_result = 0;
> >
> > Hmph, I think patch_update_file() returns "int quit".  Why do we
> > want overly wide type to store the result, which cannot even express
> > negative number to potentially signal a failure?
>
> Sorry, this is a mistake on my part
>
> >
> > > +     struct child_process cp = CHILD_PROCESS_INIT;
> > >
> > >       init_add_i_state(&s.s, r, o);
> > >
> > > @@ -1852,11 +1872,56 @@ int run_add_p(struct repository *r, enum add_p_mode mode,
> > >               return -1;
> > >       }
> > >
> > > -     for (i = 0; i < s.file_diff_nr; i++)
> > > -             if (s.file_diff[i].binary && !s.file_diff[i].hunk_nr)
> > > +     for (i = 0; i < s.file_diff_nr;) {
> > > +             if (s.file_diff[i].binary && !s.file_diff[i].hunk_nr) {
> > >                       binary_count++;
> > > -             else if (patch_update_file(&s, s.file_diff + i))
> > > -                     break;
> > > +                     i++;
> > > +                     continue;
> > > +             }
> > > +             else {
> > > +                     patch_update_result = patch_update_file(&s, s.file_diff + i);
> > > +                     if (patch_update_result == 0) {
> > > +                             i++;
> > > +                             continue;
> > > +                     }
> > > +                     if (patch_update_result == 1)
> > > +                             break;
> > > +                     if (patch_update_result == 2) {
> > > +                             i--;
> > > +                             continue;
> > > +                     }
> > > +             }
> > > +     }
> > > +     for (i = 0; i < s.file_diff_nr; i++) {
> > > +
> > > +                     /* Any hunk to be used? */
> > > +             for (j = 0; j < s.file_diff[i].hunk_nr; j++)
> > > +                     if (s.file_diff[i].hunk[j].use == USE_HUNK)
> > > +                             break;
> > > +
> > > +             if (j < s.file_diff[i].hunk_nr ||
> > > +         (!s.file_diff[i].hunk_nr && s.file_diff[i].head.use == USE_HUNK)) {
> > > +                     /* At least one hunk selected: apply */
> > > +                     strbuf_reset(&s.buf);
> > > +                     reassemble_patch(&s, s.file_diff + i, 0, &s.buf);
> > > +
> > > +                     discard_index(s.s.r->index);
> > > +                     if (s.mode->apply_for_checkout)
> > > +                             apply_for_checkout(&s, &s.buf,
> > > +                                             s.mode->is_reverse);
> > > +                     else {
> > > +                             setup_child_process(&s, &cp, "apply", NULL);
> > > +                             strvec_pushv(&cp.args, s.mode->apply_args);
> > > +                             if (pipe_command(&cp, s.buf.buf, s.buf.len,
> > > +                                             NULL, 0, NULL, 0))
> > > +                                     error(_("'git apply' failed"));
> > > +                     }
> > > +                     if (repo_read_index(s.s.r) >= 0)
> > > +                             repo_refresh_and_write_index(s.s.r, REFRESH_QUIET, 0,
> > > +                                                             1, NULL, NULL, NULL);
> > > +             }
> > > +
> > > +     }
> >
> > One upside of having "git apply" at the end of patch_update_file()
> > is that you can "^C" out of "git add -p" or your terminal connection
> > can be cut off, after dealing with hunks in a few early files, and
> > these early part of your work that you have already done are already
> > reflected to the working tree files.  By hoisting the logic to the
> > caller, this is making the update all-or-none, which is good in
> > transactional systems, but can make a horrible experience for an
> > interactive use where you make progress while thinking.
> >
> > So I am not yet convinced if this change makes sense---it could be
> > because of the lack of justification for this change.
>
> What I observed after adding the '>' and '<' options is that if a user chooses
> to use a hunk A in file 1, and then goes to file 2 with '>', comes back to
> file 1 with '<', and decides on hunk A to skip it instead, because
> patch_update_file() has
> applied the file with the hunk the user initially decided to use
> before proceeding to file
> 2 with '>', coming back to redecide and say skip does not apply the
> latest decision
> and when you check the index, the file with the hunks which the user
> initially decided to
> use but changed to skip is present in the index.
>
> But if the user initially decided to skip a hunk in a file, goes to
> the next file with '>'
> and back to the first file, changes the decision on the hunk to use,
> it applies the patch
> with the hunk because the hunk was not initially selected when the
> patch was applied.
> But if he now goes away and comes back to the file a third time and
> chooses to skip the
> hunk, then quits with 'q', because he had selected to use the hunk the
> second time,
> choosing skip again will not work.
>
> So basically, initially choosing to use a hunk in a file, going to
> another file and coming
> back to this file then choosing to skip it does not register the
> latest skip decision
> on that hunk.
>
> That was why I decided to do it this way.
> I will appreciate a better suggestion from you
>
> Thanks
> Abraham.

Hello Junio, thank you for your review.
Here I explain my decision to move the "git apply" in patch_update_file()
to the caller.

Does it sound like a valid reason to make the move?
Thanks

Abraham

Re: [PATCH v2 1/1] Allow reworking with a file after deciding on all its hunks

[Junio C Hamano]

Samuel Abraham <abrahamadekunle50@gmail.com> writes:

> Hello Junio, thank you for your review.
> Here I explain my decision to move the "git apply" in patch_update_file()
> to the caller.
>
> Does it sound like a valid reason to make the move?

I am not sure, but as long as this is an optional feature, users can
choose not to opt in if they do not like the new "all or none"
semantics, I guess.

Thanks.

Re: [PATCH v2 1/1] Allow reworking with a file after deciding on all its hunks

[Samuel Abraham]

On Fri, Jan 30, 2026 at 5:29 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Samuel Abraham <abrahamadekunle50@gmail.com> writes:
>
> > Hello Junio, thank you for your review.
> > Here I explain my decision to move the "git apply" in patch_update_file()
> > to the caller.
> >
> > Does it sound like a valid reason to make the move?
>
> I am not sure, but as long as this is an optional feature, users can
> choose not to opt in if they do not like the new "all or none"
> semantics, I guess.
>
> Thanks.

Okay thank you.
I will work on making it an optional feature

Abraham

Re: [PATCH v2 1/1] Allow reworking with a file after deciding on all its hunks

[Junio C Hamano]

Samuel Abraham <abrahamadekunle50@gmail.com> writes:

>> What I observed after adding the '>' and '<' options is that if a user chooses
>> to use a hunk A in file 1, and then goes to file 2 with '>', comes back to
>> file 1 with '<', and decides on hunk A to skip it instead, because
>> patch_update_file() has
>> applied the file with the hunk the user initially decided to use
>> before proceeding to file
>> 2 with '>', coming back to redecide and say skip does not apply the
>> latest decision
>> and when you check the index, the file with the hunks which the user
>> initially decided to
>> use but changed to skip is present in the index.

I am not sure if I would like the end result or rather prefer your
"all-or-none", so please do not take this as "here is a better way
to implement it" suggestion.

But you should be able to keep the current semantics, if you wanted
to, even if you apply the chosen hunks when you switch files, like
the original code has been doing forever since it was written.  You
know which hunks you applied, so after applying before moving on to
the next file, you can drop these hunks from the list of hunks to be
decided for application.  When the user comes back to the current
file to decide on other hunks, you know that the already used hunks
would get in the way, so why keep them?

Having said that, I think the all-or-none mode may be handy if one
makes the current working tree dirty with many little unrelated and
insignificant changes and the only way to make sense is to see the
"git diff --cached" output after adding some and leaving others, at
least in the way some people work.  I usually am very incremental
when doing "git add -p", in that while using the command in one
terminal, I run "git diff --cached" to see if I added unwanted
things by mistake and "git diff" to see if I left out necessary
things, so I would probably not be using the mode.  But that is just
my hunch without using the new interface long enough.

Thanks.

Re: [PATCH v2 1/1] Allow reworking with a file after deciding on all its hunks

[Samuel Abraham]

On Sat, Jan 31, 2026 at 8:25 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Samuel Abraham <abrahamadekunle50@gmail.com> writes:
>
> >> What I observed after adding the '>' and '<' options is that if a user chooses
> >> to use a hunk A in file 1, and then goes to file 2 with '>', comes back to
> >> file 1 with '<', and decides on hunk A to skip it instead, because
> >> patch_update_file() has
> >> applied the file with the hunk the user initially decided to use
> >> before proceeding to file
> >> 2 with '>', coming back to redecide and say skip does not apply the
> >> latest decision
> >> and when you check the index, the file with the hunks which the user
> >> initially decided to
> >> use but changed to skip is present in the index.
>
> I am not sure if I would like the end result or rather prefer your
> "all-or-none", so please do not take this as "here is a better way
> to implement it" suggestion.
>
> But you should be able to keep the current semantics, if you wanted
> to, even if you apply the chosen hunks when you switch files, like
> the original code has been doing forever since it was written.  You
> know which hunks you applied, so after applying before moving on to
> the next file, you can drop these hunks from the list of hunks to be
> decided for application.  When the user comes back to the current
> file to decide on other hunks, you know that the already used hunks
> would get in the way, so why keep them?

Yes thank you so much for suggesting this approach.

>
> Having said that, I think the all-or-none mode may be handy if one
> makes the current working tree dirty with many little unrelated and
> insignificant changes and the only way to make sense is to see the
> "git diff --cached" output after adding some and leaving others, at
> least in the way some people work.  I usually am very incremental
> when doing "git add -p", in that while using the command in one
> terminal, I run "git diff --cached" to see if I added unwanted
> things by mistake and "git diff" to see if I left out necessary
> things, so I would probably not be using the mode.  But that is just
> my hunch without using the new interface long enough.

Okay I think retaining "git apply" in patch_update_file() and dropping
the hunks the user has already decided on when coming back to the file
makes sense.
By using this approach, we skip files that have been fully decided and applied,
only showing files that;
i.  have been applied but also have undecided hunks.
ii.  not been applied and still have undecided hunks
when the user navigates with ">" and "<".

This will allow you to still run git diff--cached to see what has been
added while also being able
to see what has not been added, while navigating around files.

Thank you.
Abraham

Re: [PATCH v2 1/1] Allow reworking with a file after deciding on all its hunks

[Junio C Hamano]

Samuel Abraham <abrahamadekunle50@gmail.com> writes:

>> I am not sure if I would like the end result or rather prefer your
>> "all-or-none", so please do not take this as "here is a better way
>> to implement it" suggestion.
>>
>> But you should be able to keep the current semantics, if you wanted
>> to, even if you apply the chosen hunks when you switch files, like
>> the original code has been doing forever since it was written.  You
>> know which hunks you applied, so after applying before moving on to
>> the next file, you can drop these hunks from the list of hunks to be
>> decided for application.  When the user comes back to the current
>> file to decide on other hunks, you know that the already used hunks
>> would get in the way, so why keep them?
>
> Yes thank you so much for suggesting this approach.

Not so fast.  I explicitly said I am *NOT* suggesting anything.

And thinking about it more, I do not think it makes any sense to do
anything other than "all-or-none" when the command is working in
your new "you can move to different files before you decide on all
hunks in the current file" mode (which I think we agreed to make it
an optional mode).  Why?  After deciding yes, no, no among 5 hunks
in the first file (leaving the hunks #4 and #5 undecided), you jump
to the second file, do something there, and imagine that you come
back.  If we drop the alrady applied hunks like the suggestion,
which I did not make ;-), we'd then give you four hunks (as hunk #1
has been already applied), and even though you have already decided
not to use hunks #2 and #3, you *can* revisit them with "J" or "K",
change your mind and use them if you wanted to.  But it is too late
for the hunk #1.  It looks utterly inconsistent if you cannot change
your mind on hunk #1 but can on hunks #2 and #3 and it reduces the
usefulness of "you do not have to decide right now and visit other
files before you do so" mode.

Thanks.

Re: [PATCH v2 1/1] Allow reworking with a file after deciding on all its hunks

[Samuel Abraham]

On Mon, Feb 2, 2026 at 6:26 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Samuel Abraham <abrahamadekunle50@gmail.com> writes:
>
> >> I am not sure if I would like the end result or rather prefer your
> >> "all-or-none", so please do not take this as "here is a better way
> >> to implement it" suggestion.
> >>
> >> But you should be able to keep the current semantics, if you wanted
> >> to, even if you apply the chosen hunks when you switch files, like
> >> the original code has been doing forever since it was written.  You
> >> know which hunks you applied, so after applying before moving on to
> >> the next file, you can drop these hunks from the list of hunks to be
> >> decided for application.  When the user comes back to the current
> >> file to decide on other hunks, you know that the already used hunks
> >> would get in the way, so why keep them?
> >
> > Yes thank you so much for suggesting this approach.
>
> Not so fast.  I explicitly said I am *NOT* suggesting anything.

Yes you did.

>
> And thinking about it more, I do not think it makes any sense to do
> anything other than "all-or-none" when the command is working in
> your new "you can move to different files before you decide on all
> hunks in the current file" mode (which I think we agreed to make it
> an optional mode).  Why?  After deciding yes, no, no among 5 hunks
> in the first file (leaving the hunks #4 and #5 undecided), you jump
> to the second file, do something there, and imagine that you come
> back.  If we drop the alrady applied hunks like the suggestion,
> which I did not make ;-),

:D

> we'd then give you four hunks (as hunk #1
> has been already applied), and even though you have already decided
> not to use hunks #2 and #3, you *can* revisit them with "J" or "K",
> change your mind and use them if you wanted to.  But it is too late
> for the hunk #1.  It looks utterly inconsistent if you cannot change
> your mind on hunk #1 but can on hunks #2 and #3 and it reduces the
> usefulness of "you do not have to decide right now and visit other
> files before you do so" mode.
>
> Thanks.

Okay yes that would be very inconsistent.

I briefly thought about this.
If a user decides USE on some hunks and goes to the next file, and we
apply the patch, the user comes and decides SKIP on those hunk(s),
can't we "unapply" those hunks using "git apply -R"?
I have not really thought about the complexities but it seems to be
something that might be complex.
I just thought to share to hear your thoughts

Thanks
Abraham

[PATCH v3 0/3] introduce new option `rework-with-file`

[Abraham Samuel Adekunle]

Hello,
After further review from Junio, I have been able to make reworking with a
file during hunk selection an optional feature by passing the `rework-with-file`
flag to the --patch option in the interactive machinery.

With the option, users can navigate in between files and while deciding on
hunks as they wish with the '>' and '<' option for going to the next and
previous file respectively if there are more than one file.

The process shows a prompt which allows reworking with the file and changing
previous decisions if need be, going to the next or previous file if possible
or using 'q' to submit and end the process.

Patch 1 implements the new 'rework-with-file' options, Patch 2 makes some changes
to allow interfile navigation when the option is supplied and Patch 3 modifies
the code to allow the patches to be applied only after all decisions have
been made and session ends when this option is enabled.

Abraham Samuel Adekunle (3):
  interactive -p: add new `--rework-with-file` flag to interactive
    machinery
  add-patch: Allow interfile navigation when selecting hunks
  add-patch: Allow proper 'git apply' when using the --rework-with-file
    flag

 add-interactive.c     |   3 +
 add-interactive.h     |   5 +-
 add-patch.c           | 161 +++++++++++++++++++++++++++++++-----------
 builtin/add.c         |   4 ++
 builtin/checkout.c    |   6 ++
 builtin/reset.c       |   4 ++
 builtin/stash.c       |   8 +++
 t/t9902-completion.sh |   1 +
 8 files changed, 148 insertions(+), 44 deletions(-)

-- 
2.39.5 (Apple Git-154)

[PATCH v3 1/3] interactive -p: add new `--rework-with-file` flag to interactive machinery

[Abraham Samuel Adekunle]

When using the interactive add, reset, stash or checkout machinery, we do
not have the option of reworking with a file because the session automatically
advances to the next file or ends if we have just one file, immediately all hunks
in a file are decided on.

Introduce the flag "--rework-with-file" when interactively selecting patches with the
'--patch' option, which does not auto advance, thereby allowing users the option
to rework with files.
This ensures the current auto-advance method stays as the default method.

Signed-off-by: Abraham Samuel Adekunle <abrahamadekunle50@gmail.com>
---
 add-interactive.c     | 3 +++
 add-interactive.h     | 5 +++--
 builtin/add.c         | 4 ++++
 builtin/checkout.c    | 6 ++++++
 builtin/reset.c       | 4 ++++
 builtin/stash.c       | 8 ++++++++
 t/t9902-completion.sh | 1 +
 7 files changed, 29 insertions(+), 2 deletions(-)

diff --git a/add-interactive.c b/add-interactive.c
index 68fc09547d..4eda115da8 100644
--- a/add-interactive.c
+++ b/add-interactive.c
@@ -64,6 +64,7 @@ void init_add_i_state(struct add_i_state *s, struct repository *r,
 	s->r = r;
 	s->context = -1;
 	s->interhunkcontext = -1;
+	s->no_auto_advance = 0;
 
 	s->use_color_interactive = check_color_config(r, "color.interactive");
 
@@ -124,6 +125,8 @@ void init_add_i_state(struct add_i_state *s, struct repository *r,
 			die(_("%s cannot be negative"), "--inter-hunk-context");
 		s->interhunkcontext = add_p_opt->interhunkcontext;
 	}
+	if (add_p_opt->no_auto_advance)
+		s->no_auto_advance = 1;
 }
 
 void clear_add_i_state(struct add_i_state *s)
diff --git a/add-interactive.h b/add-interactive.h
index da49502b76..aef2feca56 100644
--- a/add-interactive.h
+++ b/add-interactive.h
@@ -6,9 +6,10 @@
 struct add_p_opt {
 	int context;
 	int interhunkcontext;
+	int no_auto_advance;
 };
 
-#define ADD_P_OPT_INIT { .context = -1, .interhunkcontext = -1 }
+#define ADD_P_OPT_INIT { .context = -1, .interhunkcontext = -1, .no_auto_advance = 0 }
 
 struct add_i_state {
 	struct repository *r;
@@ -28,7 +29,7 @@ struct add_i_state {
 
 	int use_single_key;
 	char *interactive_diff_filter, *interactive_diff_algorithm;
-	int context, interhunkcontext;
+	int context, interhunkcontext, no_auto_advance;
 };
 
 void init_add_i_state(struct add_i_state *s, struct repository *r,
diff --git a/builtin/add.c b/builtin/add.c
index 32709794b3..408827cf54 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -256,6 +256,8 @@ static struct option builtin_add_options[] = {
 	OPT_GROUP(""),
 	OPT_BOOL('i', "interactive", &add_interactive, N_("interactive picking")),
 	OPT_BOOL('p', "patch", &patch_interactive, N_("select hunks interactively")),
+	OPT_BOOL(0, "rework-with-file", &add_p_opt.no_auto_advance,
+		 N_("rework with files when selecting hunks interactively")),
 	OPT_DIFF_UNIFIED(&add_p_opt.context),
 	OPT_DIFF_INTERHUNK_CONTEXT(&add_p_opt.interhunkcontext),
 	OPT_BOOL('e', "edit", &edit_interactive, N_("edit current diff and apply")),
@@ -418,6 +420,8 @@ int cmd_add(int argc,
 			die(_("the option '%s' requires '%s'"), "--unified", "--interactive/--patch");
 		if (add_p_opt.interhunkcontext != -1)
 			die(_("the option '%s' requires '%s'"), "--inter-hunk-context", "--interactive/--patch");
+		if (add_p_opt.no_auto_advance)
+			die(_("the option '%s' requires '%s'"), "--rework-with-file", "--interactive/--patch");
 	}
 
 	if (edit_interactive) {
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 261699e2f5..3e98d06be1 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -63,6 +63,7 @@ struct checkout_opts {
 	int patch_mode;
 	int patch_context;
 	int patch_interhunk_context;
+	int no_auto_advance;
 	int quiet;
 	int merge;
 	int force;
@@ -549,6 +550,7 @@ static int checkout_paths(const struct checkout_opts *opts,
 		struct add_p_opt add_p_opt = {
 			.context = opts->patch_context,
 			.interhunkcontext = opts->patch_interhunk_context,
+			.no_auto_advance = opts->no_auto_advance
 		};
 		const char *rev = new_branch_info->name;
 		char rev_oid[GIT_MAX_HEXSZ + 1];
@@ -1747,6 +1749,8 @@ static struct option *add_checkout_path_options(struct checkout_opts *opts,
 			      N_("checkout their version for unmerged files"),
 			      3, PARSE_OPT_NONEG),
 		OPT_BOOL('p', "patch", &opts->patch_mode, N_("select hunks interactively")),
+		OPT_BOOL(0, "rework-with-file", &opts->no_auto_advance,
+			 N_("rework with files when selecting hunks interactively")),
 		OPT_DIFF_UNIFIED(&opts->patch_context),
 		OPT_DIFF_INTERHUNK_CONTEXT(&opts->patch_interhunk_context),
 		OPT_BOOL(0, "ignore-skip-worktree-bits", &opts->ignore_skipworktree,
@@ -1801,6 +1805,8 @@ static int checkout_main(int argc, const char **argv, const char *prefix,
 			die(_("the option '%s' requires '%s'"), "--unified", "--patch");
 		if (opts->patch_interhunk_context != -1)
 			die(_("the option '%s' requires '%s'"), "--inter-hunk-context", "--patch");
+		if (opts->no_auto_advance)
+			die(_("the option '%s' requires '%s'"), "--rework-with-file", "--patch");
 	}
 
 	if (opts->show_progress < 0) {
diff --git a/builtin/reset.c b/builtin/reset.c
index ed35802af1..1e7b93785d 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -371,6 +371,8 @@ int cmd_reset(int argc,
 			       PARSE_OPT_OPTARG,
 			       option_parse_recurse_submodules_worktree_updater),
 		OPT_BOOL('p', "patch", &patch_mode, N_("select hunks interactively")),
+		OPT_BOOL(0, "rework-with-file", &add_p_opt.no_auto_advance,
+			 N_("rework with files when selecting hunks interactively")),
 		OPT_DIFF_UNIFIED(&add_p_opt.context),
 		OPT_DIFF_INTERHUNK_CONTEXT(&add_p_opt.interhunkcontext),
 		OPT_BOOL('N', "intent-to-add", &intent_to_add,
@@ -443,6 +445,8 @@ int cmd_reset(int argc,
 			die(_("the option '%s' requires '%s'"), "--unified", "--patch");
 		if (add_p_opt.interhunkcontext != -1)
 			die(_("the option '%s' requires '%s'"), "--inter-hunk-context", "--patch");
+		if (add_p_opt.no_auto_advance)
+			die(_("the option '%s' requires '%s'"), "--rework-with-file", "--patch");
 	}
 
 	/* git reset tree [--] paths... can be used to
diff --git a/builtin/stash.c b/builtin/stash.c
index 948eba06fb..1311707ea6 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -1849,6 +1849,8 @@ static int push_stash(int argc, const char **argv, const char *prefix,
 			 N_("stash staged changes only")),
 		OPT_BOOL('p', "patch", &patch_mode,
 			 N_("stash in patch mode")),
+		OPT_BOOL(0, "rework-with-file", &add_p_opt.no_auto_advance,
+			 N_("rework with files when selecting hunks interactively")),
 		OPT_DIFF_UNIFIED(&add_p_opt.context),
 		OPT_DIFF_INTERHUNK_CONTEXT(&add_p_opt.interhunkcontext),
 		OPT__QUIET(&quiet, N_("quiet mode")),
@@ -1911,6 +1913,8 @@ static int push_stash(int argc, const char **argv, const char *prefix,
 			die(_("the option '%s' requires '%s'"), "--unified", "--patch");
 		if (add_p_opt.interhunkcontext != -1)
 			die(_("the option '%s' requires '%s'"), "--inter-hunk-context", "--patch");
+		if (add_p_opt.no_auto_advance)
+			die(_("the option '%s' requires '%s'"), "--rework-with-file", "--patch");
 	}
 
 	if (add_p_opt.context < -1)
@@ -1952,6 +1956,8 @@ static int save_stash(int argc, const char **argv, const char *prefix,
 			 N_("stash staged changes only")),
 		OPT_BOOL('p', "patch", &patch_mode,
 			 N_("stash in patch mode")),
+		OPT_BOOL(0, "rework-with-file", &add_p_opt.no_auto_advance,
+			 N_("rework with files when selecting hunks interactively")),
 		OPT_DIFF_UNIFIED(&add_p_opt.context),
 		OPT_DIFF_INTERHUNK_CONTEXT(&add_p_opt.interhunkcontext),
 		OPT__QUIET(&quiet, N_("quiet mode")),
@@ -1983,6 +1989,8 @@ static int save_stash(int argc, const char **argv, const char *prefix,
 			die(_("the option '%s' requires '%s'"), "--unified", "--patch");
 		if (add_p_opt.interhunkcontext != -1)
 			die(_("the option '%s' requires '%s'"), "--inter-hunk-context", "--patch");
+		if (add_p_opt.no_auto_advance)
+			die(_("the option '%s' requires '%s'"), "--rework-with-file", "--patch");
 	}
 
 	ret = do_push_stash(&ps, stash_msg, quiet, keep_index,
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 964e1f1569..302534e92d 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -2601,6 +2601,7 @@ test_expect_success 'double dash "git checkout"' '
 	--ignore-skip-worktree-bits Z
 	--ignore-other-worktrees Z
 	--recurse-submodules Z
+	--rework-with-file Z
 	--progress Z
 	--guess Z
 	--no-guess Z
-- 
2.39.5 (Apple Git-154)

[PATCH v3 2/3] add-patch: Allow interfile navigation when selecting hunks

[Abraham Samuel Adekunle]

After deciding on all hunks in a file, the interactive session
advances automatically to the next file if there is another,
or the process ends.

Now using the `--rework-with-file` flag with `--patch` the process does not
advance automatically. A user can choose to go to the next file by pressing
'>' or the previous file by pressing '<', before or after deciding on all
hunks in the current file.

After all hunks have been decided in a file, a prompt appears,
which allow the user to still rework with the file by applying
the options available in the permit set for that hunk, and
after all the decisions, the user presses 'q' to submit.

This feature is enabled by passing the `--rework-with-file` flag
to `--patch` option of the subcommands add, stash, reset,
and checkout

Signed-off-by: Abraham Samuel Adekunle <abrahamadekunle50@gmail.com>
---
 add-patch.c | 95 ++++++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 80 insertions(+), 15 deletions(-)

diff --git a/add-patch.c b/add-patch.c
index 173a53241e..2bd839f17e 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -1418,6 +1418,8 @@ N_("j - go to the next undecided hunk, roll over at the bottom\n"
    "e - manually edit the current hunk\n"
    "p - print the current hunk\n"
    "P - print the current hunk using the pager\n"
+   "> - go to the next file\n"
+   "< - go to the previous file\n"
    "? - print help\n");
 
 static size_t dec_mod(size_t a, size_t m)
@@ -1430,6 +1432,12 @@ static size_t inc_mod(size_t a, size_t m)
 	return a < m - 1 ? a + 1 : 0;
 }
 
+enum patch_update_response {
+	NEXT_FILE = 0,
+	QUIT,
+	PREVIOUS_FILE,
+};
+
 static bool get_first_undecided(const struct file_diff *file_diff, size_t *idx)
 {
 	for (size_t i = 0; i < file_diff->hunk_nr; i++) {
@@ -1441,7 +1449,7 @@ static bool get_first_undecided(const struct file_diff *file_diff, size_t *idx)
 	return false;
 }
 
-static int patch_update_file(struct add_p_state *s,
+static enum patch_update_response patch_update_file(struct add_p_state *s,
 			     struct file_diff *file_diff)
 {
 	size_t hunk_index = 0;
@@ -1449,12 +1457,14 @@ static int patch_update_file(struct add_p_state *s,
 	struct hunk *hunk;
 	char ch;
 	struct child_process cp = CHILD_PROCESS_INIT;
-	int colored = !!s->colored.len, quit = 0, use_pager = 0;
+	int colored = !!s->colored.len, use_pager = 0;
 	enum prompt_mode_type prompt_mode_type;
+	int all_decided = 0;
+	enum patch_update_response ret = NEXT_FILE;
 
 	/* Empty added files have no hunks */
 	if (!file_diff->hunk_nr && !file_diff->added)
-		return 0;
+		return NEXT_FILE;
 
 	strbuf_reset(&s->buf);
 	render_diff_header(s, file_diff, colored, &s->buf);
@@ -1467,7 +1477,9 @@ static int patch_update_file(struct add_p_state *s,
 			ALLOW_GOTO_NEXT_UNDECIDED_HUNK = 1 << 3,
 			ALLOW_SEARCH_AND_GOTO = 1 << 4,
 			ALLOW_SPLIT = 1 << 5,
-			ALLOW_EDIT = 1 << 6
+			ALLOW_EDIT = 1 << 6,
+			ALLOW_GOTO_PREVIOUS_FILE = 1 << 7,
+			ALLOW_GOTO_NEXT_FILE = 1 << 8
 		} permitted = 0;
 
 		if (hunk_index >= file_diff->hunk_nr)
@@ -1498,9 +1510,12 @@ static int patch_update_file(struct add_p_state *s,
 
 		/* Everything decided? */
 		if (undecided_previous < 0 && undecided_next < 0 &&
-		    hunk->use != UNDECIDED_HUNK)
-			break;
-
+		    hunk->use != UNDECIDED_HUNK) {
+				if (s->s.no_auto_advance)
+					all_decided = 1;
+				else
+					break;
+			}
 		strbuf_reset(&s->buf);
 		if (file_diff->hunk_nr) {
 			if (rendered_hunk_index != hunk_index) {
@@ -1548,6 +1563,14 @@ static int patch_update_file(struct add_p_state *s,
 				permitted |= ALLOW_EDIT;
 				strbuf_addstr(&s->buf, ",e");
 			}
+			if (s->s.no_auto_advance && s->file_diff_nr > 1) {
+				permitted |= ALLOW_GOTO_NEXT_FILE;
+				strbuf_addstr(&s->buf, ",>");
+			}
+			if (s->s.no_auto_advance && s->file_diff_nr > 1) {
+				permitted |= ALLOW_GOTO_PREVIOUS_FILE;
+				strbuf_addstr(&s->buf, ",<");
+			}
 			strbuf_addstr(&s->buf, ",p,P");
 		}
 		if (file_diff->deleted)
@@ -1566,11 +1589,14 @@ static int patch_update_file(struct add_p_state *s,
 						: 1));
 		printf(_(s->mode->prompt_mode[prompt_mode_type]),
 		       s->buf.buf);
+		if (s->s.no_auto_advance && all_decided)
+			printf(_("\n%s All hunks decided. What now? "),
+				s->s.prompt_color);
 		if (*s->s.reset_color_interactive)
 			fputs(s->s.reset_color_interactive, stdout);
 		fflush(stdout);
 		if (read_single_character(s) == EOF) {
-			quit = 1;
+			ret = QUIT;
 			break;
 		}
 
@@ -1616,9 +1642,26 @@ static int patch_update_file(struct add_p_state *s,
 				hunk->use = SKIP_HUNK;
 			}
 		} else if (ch == 'q') {
-			quit = 1;
+			ret = QUIT;
 			break;
-		} else if (s->answer.buf[0] == 'K') {
+		} else if (s->s.no_auto_advance && s->answer.buf[0] == '>') {
+			if (permitted & ALLOW_GOTO_NEXT_FILE) {
+				ret = NEXT_FILE;
+				break;
+			} else {
+				err(s, _("No next file"));
+				continue;
+			}
+		} else if (s->s.no_auto_advance && s->answer.buf[0] == '<') {
+			if (permitted & ALLOW_GOTO_PREVIOUS_FILE) {
+				ret = PREVIOUS_FILE;
+				break;
+			} else {
+				err(s, _("No previous file"));
+				continue;
+			}
+		}
+		else if (s->answer.buf[0] == 'K') {
 			if (permitted & ALLOW_GOTO_PREVIOUS_HUNK)
 				hunk_index = dec_mod(hunk_index,
 						     file_diff->hunk_nr);
@@ -1803,7 +1846,7 @@ static int patch_update_file(struct add_p_state *s,
 	}
 
 	putchar('\n');
-	return quit;
+	return ret;
 }
 
 int run_add_p(struct repository *r, enum add_p_mode mode,
@@ -1814,6 +1857,7 @@ int run_add_p(struct repository *r, enum add_p_mode mode,
 		{ r }, STRBUF_INIT, STRBUF_INIT, STRBUF_INIT, STRBUF_INIT
 	};
 	size_t i, binary_count = 0;
+	enum patch_update_response ret;
 
 	init_add_i_state(&s.s, r, o);
 
@@ -1852,11 +1896,32 @@ int run_add_p(struct repository *r, enum add_p_mode mode,
 		return -1;
 	}
 
-	for (i = 0; i < s.file_diff_nr; i++)
-		if (s.file_diff[i].binary && !s.file_diff[i].hunk_nr)
+	for (i = 0; i < s.file_diff_nr;) {
+		if (s.file_diff[i].binary && !s.file_diff[i].hunk_nr) {
 			binary_count++;
-		else if (patch_update_file(&s, s.file_diff + i))
-			break;
+			i++;
+			continue;
+		}
+		else {
+			ret = patch_update_file(&s, s.file_diff + i);
+			if (ret == NEXT_FILE) {
+				if (s.s.no_auto_advance && i == s.file_diff_nr - 1)
+					i = 0;
+				else
+					i++;
+				continue;
+			}
+			if (ret == QUIT)
+				break;
+			if (s.s.no_auto_advance && ret == PREVIOUS_FILE) {
+				if (i == 0)
+					i = s.file_diff_nr - 1;
+				else
+					i--;
+				continue;
+			}
+		}
+    }
 
 	if (s.file_diff_nr == 0)
 		err(&s, _("No changes."));
-- 
2.39.5 (Apple Git-154)

[PATCH v3 3/3] add-patch: Allow proper 'git apply' when using the --rework-with-file flag

[Abraham Samuel Adekunle]

When the flag `--rework-with-file` is used with `--patch`, if the user
has decided `USE` on a hunk in a file, goes to another file, and then
returns to this file and changes the previous decision on the hunk to
`SKIP`, because the patch has already been applied, the last decision
is not registered and the now SKIPPED hunk is still applied.

Modify the logic to allow all files to be applied only after the user
has finished making hunk decisions and quits. This will ensure the last
decision of the user is applied regardless of how the user navigates back
and forth and decides.

This change does not affect the default behaviour of applying the auto
advancing after deciding on all hunks in a file.

Signed-off-by: Abraham Samuel Adekunle <abrahamadekunle50@gmail.com>
---
 add-patch.c | 66 +++++++++++++++++++++++++++++++----------------------
 1 file changed, 39 insertions(+), 27 deletions(-)

diff --git a/add-patch.c b/add-patch.c
index 2bd839f17e..9fb33715c2 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -1422,6 +1422,40 @@ N_("j - go to the next undecided hunk, roll over at the bottom\n"
    "< - go to the previous file\n"
    "? - print help\n");
 
+static void apply_patch(struct add_p_state *s, struct file_diff *file_diff)
+{
+	struct child_process cp = CHILD_PROCESS_INIT;
+	size_t j;
+
+		/* Any hunk to be used? */
+	for (j = 0; j < file_diff->hunk_nr; j++)
+		if (file_diff->hunk[j].use == USE_HUNK)
+			break;
+
+	if (j < file_diff->hunk_nr ||
+		(!file_diff->hunk_nr && file_diff->head.use == USE_HUNK)) {
+		/* At least one hunk selected: apply */
+		strbuf_reset(&s->buf);
+		reassemble_patch(s, file_diff, 0, &s->buf);
+
+		discard_index(s->s.r->index);
+		if (s->mode->apply_for_checkout)
+			apply_for_checkout(s, &s->buf,
+					s->mode->is_reverse);
+		else {
+			setup_child_process(s, &cp, "apply", NULL);
+			strvec_pushv(&cp.args, s->mode->apply_args);
+			if (pipe_command(&cp, s->buf.buf, s->buf.len,
+					NULL, 0, NULL, 0))
+				error(_("'git apply' failed"));
+		}
+		if (repo_read_index(s->s.r) >= 0)
+			repo_refresh_and_write_index(s->s.r, REFRESH_QUIET, 0,
+							1, NULL, NULL, NULL);
+	}
+
+}
+
 static size_t dec_mod(size_t a, size_t m)
 {
 	return a > 0 ? a - 1 : m - 1;
@@ -1456,7 +1490,6 @@ static enum patch_update_response patch_update_file(struct add_p_state *s,
 	ssize_t i, undecided_previous, undecided_next, rendered_hunk_index = -1;
 	struct hunk *hunk;
 	char ch;
-	struct child_process cp = CHILD_PROCESS_INIT;
 	int colored = !!s->colored.len, use_pager = 0;
 	enum prompt_mode_type prompt_mode_type;
 	int all_decided = 0;
@@ -1818,32 +1851,8 @@ static enum patch_update_response patch_update_file(struct add_p_state *s,
 		}
 	}
 
-	/* Any hunk to be used? */
-	for (i = 0; i < file_diff->hunk_nr; i++)
-		if (file_diff->hunk[i].use == USE_HUNK)
-			break;
-
-	if (i < file_diff->hunk_nr ||
-	    (!file_diff->hunk_nr && file_diff->head.use == USE_HUNK)) {
-		/* At least one hunk selected: apply */
-		strbuf_reset(&s->buf);
-		reassemble_patch(s, file_diff, 0, &s->buf);
-
-		discard_index(s->s.r->index);
-		if (s->mode->apply_for_checkout)
-			apply_for_checkout(s, &s->buf,
-					   s->mode->is_reverse);
-		else {
-			setup_child_process(s, &cp, "apply", NULL);
-			strvec_pushv(&cp.args, s->mode->apply_args);
-			if (pipe_command(&cp, s->buf.buf, s->buf.len,
-					 NULL, 0, NULL, 0))
-				error(_("'git apply' failed"));
-		}
-		if (repo_read_index(s->s.r) >= 0)
-			repo_refresh_and_write_index(s->s.r, REFRESH_QUIET, 0,
-						     1, NULL, NULL, NULL);
-	}
+	if (!s->s.no_auto_advance)
+		apply_patch(s, file_diff);
 
 	putchar('\n');
 	return ret;
@@ -1922,6 +1931,9 @@ int run_add_p(struct repository *r, enum add_p_mode mode,
 			}
 		}
     }
+	for (i = 0; i < s.file_diff_nr; i++)
+		if (s.s.no_auto_advance)
+			apply_patch(&s, s.file_diff + i);
 
 	if (s.file_diff_nr == 0)
 		err(&s, _("No changes."));
-- 
2.39.5 (Apple Git-154)

Re: [PATCH v3 1/3] interactive -p: add new `--rework-with-file` flag to interactive machinery

[Junio C Hamano]

Abraham Samuel Adekunle <abrahamadekunle50@gmail.com> writes:

> When using the interactive add, reset, stash or checkout machinery, we do
> not have the option of reworking with a file because the session automatically
> advances to the next file or ends if we have just one file, immediately all hunks
> in a file are decided on.

The last part of the sentence after the last comma does not read
very well, at least to me.

We recommend to fold lines in such a way that after a few e-mail
exchange and quoting it will still stay within 80-column, so a
practical fill-column value lies somewhere around ~70.  Your lines a
slightly longer.

> Introduce the flag "--rework-with-file" when interactively selecting patches with the
> '--patch' option, which does not auto advance, thereby allowing users the option
> to rework with files.
> This ensures the current auto-advance method stays as the default method.

OK.  There may be suggestions for better option names from others; I
do not think of any right now.

> diff --git a/add-interactive.h b/add-interactive.h
> index da49502b76..aef2feca56 100644
> --- a/add-interactive.h
> +++ b/add-interactive.h
> @@ -6,9 +6,10 @@
>  struct add_p_opt {
>  	int context;
>  	int interhunkcontext;
> +	int no_auto_advance;
>  };

We add a new risk of double-negation confusion, e.g.,

    if (!opt->no_auto_advance)
	... do the auto-advance thing ...

where it may be easier to follow if it were written

    if (opt->auto_advance)
	... do the auto-advance thing ...

Would it make it harder to arrange the code if we made this member
"auto_advance" that defaults to "true"?  We have ADD_P_OPT_INIT that
everybody is supposed to call already, like this

> -#define ADD_P_OPT_INIT { .context = -1, .interhunkcontext = -1 }
> +#define ADD_P_OPT_INIT { .context = -1, .interhunkcontext = -1, .no_auto_advance = 0 }

so I do not imagine it would be too much hassle.

> @@ -28,7 +29,7 @@ struct add_i_state {
>  
>  	int use_single_key;
>  	char *interactive_diff_filter, *interactive_diff_algorithm;
> -	int context, interhunkcontext;
> +	int context, interhunkcontext, no_auto_advance;
>  };

Likewise.

> diff --git a/builtin/add.c b/builtin/add.c
> index 32709794b3..408827cf54 100644
> --- a/builtin/add.c
> +++ b/builtin/add.c
> @@ -256,6 +256,8 @@ static struct option builtin_add_options[] = {
>  	OPT_GROUP(""),
>  	OPT_BOOL('i', "interactive", &add_interactive, N_("interactive picking")),
>  	OPT_BOOL('p', "patch", &patch_interactive, N_("select hunks interactively")),
> +	OPT_BOOL(0, "rework-with-file", &add_p_opt.no_auto_advance,
> +		 N_("rework with files when selecting hunks interactively")),

Likewise.

Re: [PATCH v3 2/3] add-patch: Allow interfile navigation when selecting hunks

[Junio C Hamano]

Abraham Samuel Adekunle <abrahamadekunle50@gmail.com> writes:

> -		} else if (s->answer.buf[0] == 'K') {
> +		} else if (s->s.no_auto_advance && s->answer.buf[0] == '>') {
> +			if (permitted & ALLOW_GOTO_NEXT_FILE) {
> +				ret = NEXT_FILE;
> +...
> +				continue;
> +			}
> +		}
> +		else if (s->answer.buf[0] == 'K') {

This funny-looking diff is a sign that the coding guideline was
followed in the preimage but not in the postimage, by splitting
the "} else if (condition) {" into two lines for 'K'.

Re: [PATCH v3 2/3] add-patch: Allow interfile navigation when selecting hunks

[Junio C Hamano]

Abraham Samuel Adekunle <abrahamadekunle50@gmail.com> writes:

> +	for (i = 0; i < s.file_diff_nr;) {
> +		if (s.file_diff[i].binary && !s.file_diff[i].hunk_nr) {
>  			binary_count++;
> +			i++;
> +			continue;
> +		}

This "continue" is a commonly seen good trick to avoid having the
nesting go too deep.  As we know the case where the condition holds
have already been dealt with and moved to the next iteration at this
point, we can ...

> +		else {

... omit this extra "else" block and write what is inside for
everybody (not just "those who did not pass the if condition above",
which is what "else" tells us).

> +			ret = patch_update_file(&s, s.file_diff + i);
> +			if (ret == NEXT_FILE) {
> +				if (s.s.no_auto_advance && i == s.file_diff_nr - 1)
> +					i = 0;
> +				else
> +					i++;
> +				continue;
> +			}
> +			if (ret == QUIT)
> +				break;
> +			if (s.s.no_auto_advance && ret == PREVIOUS_FILE) {
> +				if (i == 0)
> +					i = s.file_diff_nr - 1;
> +				else
> +					i--;
> +				continue;
> +			}

The asymmetry between next/quit and prev feels curious.

The patch_update_file() helper returns QUIT when the user tells us
to (regardless of auto-advance setting), PREVIOUS when '<' is given
but that is only possible with auto-advance disabled, and NEXT in
all other cases.  The check inside the NEXT case for auto-advance is
to decide if we want to overflow 'i' beyond file_diff_nr to complete
the session, or we want to wrap-around back to the first file.

But ret can be PREV only under auto-advance disabled, so the check
there feels totally redundant.

And we want to treat the list of files as a ring buffer only when
auto-advance is set to false.  This may work in practice but the
logic feels convoluted.  

The patch_update_file() knows how many files there are to decide if
we want to offer '<' and '>'.  It also knows the file index within
the file_diff_nr for the file it is handling.  I wonder if it should
do a bit more with its return value to help the caller?  E.g.,
perhaps it can return the next 'i' if it wants the caller to advance
(and decide to do the ring-buffer if needed), or if it wants to tell
the caller that everything is done by returning some sentinel value
(e.g., -1)?  Then this part of the caller can just be

	if ((i = patch_update_file(&s, i)) < 0)
		break; /* all done */

perhaps?

By the way, I just noticed that the new local variable you added to
patch_update_file() is called "ret" but that is hiding a different
variable "int ret" that is used to handle the '/' command.  It
should be renamed to avoid the name collision.

Re: [PATCH v3 3/3] add-patch: Allow proper 'git apply' when using the --rework-with-file flag

[Junio C Hamano]

Abraham Samuel Adekunle <abrahamadekunle50@gmail.com> writes:

> Subject: Re: [PATCH v3 3/3] add-patch: Allow proper 'git apply' when using the --rework-with-file flag

Style.  Downcase "Allow".  Applies to [2/3].

Avoid "proper" as it is not obvious to everybody what you find
proper and why you find it proper.  Applies to any value-judgement
adjective.

    Subject: [PATCH v3 3/3] add-patch: allow all-or-none application of a patch

or something?

> +static void apply_patch(struct add_p_state *s, struct file_diff *file_diff)
> +{
> +	struct child_process cp = CHILD_PROCESS_INIT;
> +	size_t j;
> +
> +		/* Any hunk to be used? */

Funny indentaion?

> +	for (j = 0; j < file_diff->hunk_nr; j++)
> +		if (file_diff->hunk[j].use == USE_HUNK)
> +			break;
> +
> +	if (j < file_diff->hunk_nr ||
> +		(!file_diff->hunk_nr && file_diff->head.use == USE_HUNK)) {
> +		/* At least one hunk selected: apply */
> +		strbuf_reset(&s->buf);
> +		reassemble_patch(s, file_diff, 0, &s->buf);
> +
> +		discard_index(s->s.r->index);
> +		if (s->mode->apply_for_checkout)
> +			apply_for_checkout(s, &s->buf,
> +					s->mode->is_reverse);
> +		else {
> +			setup_child_process(s, &cp, "apply", NULL);
> +			strvec_pushv(&cp.args, s->mode->apply_args);
> +			if (pipe_command(&cp, s->buf.buf, s->buf.len,
> +					NULL, 0, NULL, 0))
> +				error(_("'git apply' failed"));
> +		}
> +		if (repo_read_index(s->s.r) >= 0)
> +			repo_refresh_and_write_index(s->s.r, REFRESH_QUIET, 0,
> +							1, NULL, NULL, NULL);
> +	}
> +
> +}

I suspect that the extraction of this helper function out of its
original place in patch_update_file() should be done in its own
patch.

Do we need new tests to cover this new feature?

Re: [PATCH v3 0/3] introduce new option `rework-with-file`

[Junio C Hamano]

Abraham Samuel Adekunle <abrahamadekunle50@gmail.com> writes:

> Abraham Samuel Adekunle (3):
>   interactive -p: add new `--rework-with-file` flag to interactive
>     machinery
>   add-patch: Allow interfile navigation when selecting hunks
>   add-patch: Allow proper 'git apply' when using the --rework-with-file
>     flag

By the way, this series is conflicting with your other series that
has been in 'next' and is ready to graduate.  Perhaps it is time to
consider rebasing.

Re: [PATCH v3 2/3] add-patch: Allow interfile navigation when selecting hunks

[Junio C Hamano]

Abraham Samuel Adekunle <abrahamadekunle50@gmail.com> writes:

> @@ -1566,11 +1589,14 @@ static int patch_update_file(struct add_p_state *s,
>  						: 1));
>  		printf(_(s->mode->prompt_mode[prompt_mode_type]),
>  		       s->buf.buf);
> +		if (s->s.no_auto_advance && all_decided)
> +			printf(_("\n%s All hunks decided. What now? "),
> +				s->s.prompt_color);

This gives an ordinary prompt for the hunk and then another one
after it if we notice everything has been decided.  I am wondering
if it wants to be more like

	if (!s->auto_advance && all_decided)
		say What now?
	else
		ask the usual

?

Re: [PATCH v3 1/3] interactive -p: add new `--rework-with-file` flag to interactive machinery

[Samuel Abraham]

On Fri, Feb 6, 2026 at 7:25 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Abraham Samuel Adekunle <abrahamadekunle50@gmail.com> writes:
>
> > When using the interactive add, reset, stash or checkout machinery, we do
> > not have the option of reworking with a file because the session automatically
> > advances to the next file or ends if we have just one file, immediately all hunks
> > in a file are decided on.
>
> The last part of the sentence after the last comma does not read
> very well, at least to me.

Okay I will reword it.

>
> We recommend to fold lines in such a way that after a few e-mail
> exchange and quoting it will still stay within 80-column, so a
> practical fill-column value lies somewhere around ~70.  Your lines a
> slightly longer.

Okay thank you.
I will watch out for this.

>
> > Introduce the flag "--rework-with-file" when interactively selecting patches with the
> > '--patch' option, which does not auto advance, thereby allowing users the option
> > to rework with files.
> > This ensures the current auto-advance method stays as the default method.
>
> OK.  There may be suggestions for better option names from others; I
> do not think of any right now.

Okay

>
> > diff --git a/add-interactive.h b/add-interactive.h
> > index da49502b76..aef2feca56 100644
> > --- a/add-interactive.h
> > +++ b/add-interactive.h
> > @@ -6,9 +6,10 @@
> >  struct add_p_opt {
> >       int context;
> >       int interhunkcontext;
> > +     int no_auto_advance;
> >  };
>
> We add a new risk of double-negation confusion, e.g.,
>
>     if (!opt->no_auto_advance)
>         ... do the auto-advance thing ...
>
> where it may be easier to follow if it were written
>
>     if (opt->auto_advance)
>         ... do the auto-advance thing ...
>
> Would it make it harder to arrange the code if we made this member
> "auto_advance" that defaults to "true"?  We have ADD_P_OPT_INIT that
> everybody is supposed to call already, like this
>
> > -#define ADD_P_OPT_INIT { .context = -1, .interhunkcontext = -1 }
> > +#define ADD_P_OPT_INIT { .context = -1, .interhunkcontext = -1, .no_auto_advance = 0 }
>
> so I do not imagine it would be too much hassle.

No it won't.

>
> > @@ -28,7 +29,7 @@ struct add_i_state {
> >
> >       int use_single_key;
> >       char *interactive_diff_filter, *interactive_diff_algorithm;
> > -     int context, interhunkcontext;
> > +     int context, interhunkcontext, no_auto_advance;
> >  };
>
> Likewise.

Noted.

>
> > diff --git a/builtin/add.c b/builtin/add.c
> > index 32709794b3..408827cf54 100644
> > --- a/builtin/add.c
> > +++ b/builtin/add.c
> > @@ -256,6 +256,8 @@ static struct option builtin_add_options[] = {
> >       OPT_GROUP(""),
> >       OPT_BOOL('i', "interactive", &add_interactive, N_("interactive picking")),
> >       OPT_BOOL('p', "patch", &patch_interactive, N_("select hunks interactively")),
> > +     OPT_BOOL(0, "rework-with-file", &add_p_opt.no_auto_advance,
> > +              N_("rework with files when selecting hunks interactively")),
>
> Likewise.

Thanks.

Abraham

Re: [PATCH v3 2/3] add-patch: Allow interfile navigation when selecting hunks

[Samuel Abraham]

On Fri, Feb 6, 2026 at 7:35 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Abraham Samuel Adekunle <abrahamadekunle50@gmail.com> writes:
>
> > -             } else if (s->answer.buf[0] == 'K') {
> > +             } else if (s->s.no_auto_advance && s->answer.buf[0] == '>') {
> > +                     if (permitted & ALLOW_GOTO_NEXT_FILE) {
> > +                             ret = NEXT_FILE;
> > +...
> > +                             continue;
> > +                     }
> > +             }
> > +             else if (s->answer.buf[0] == 'K') {
>
> This funny-looking diff is a sign that the coding guideline was
> followed in the preimage but not in the postimage, by splitting
> the "} else if (condition) {" into two lines for 'K'.

Sorry, I will fix it

Re: [PATCH v3 2/3] add-patch: Allow interfile navigation when selecting hunks

[Samuel Abraham]

On Fri, Feb 6, 2026 at 7:54 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Abraham Samuel Adekunle <abrahamadekunle50@gmail.com> writes:
>
> > +     for (i = 0; i < s.file_diff_nr;) {
> > +             if (s.file_diff[i].binary && !s.file_diff[i].hunk_nr) {
> >                       binary_count++;
> > +                     i++;
> > +                     continue;
> > +             }
>
> This "continue" is a commonly seen good trick to avoid having the
> nesting go too deep.  As we know the case where the condition holds
> have already been dealt with and moved to the next iteration at this
> point, we can ...
>
> > +             else {
>
> ... omit this extra "else" block and write what is inside for
> everybody (not just "those who did not pass the if condition above",
> which is what "else" tells us).

Yes.

>
> > +                     ret = patch_update_file(&s, s.file_diff + i);
> > +                     if (ret == NEXT_FILE) {
> > +                             if (s.s.no_auto_advance && i == s.file_diff_nr - 1)
> > +                                     i = 0;
> > +                             else
> > +                                     i++;
> > +                             continue;
> > +                     }
> > +                     if (ret == QUIT)
> > +                             break;
> > +                     if (s.s.no_auto_advance && ret == PREVIOUS_FILE) {
> > +                             if (i == 0)
> > +                                     i = s.file_diff_nr - 1;
> > +                             else
> > +                                     i--;
> > +                             continue;
> > +                     }
>
> The asymmetry between next/quit and prev feels curious.
>
> The patch_update_file() helper returns QUIT when the user tells us
> to (regardless of auto-advance setting), PREVIOUS when '<' is given
> but that is only possible with auto-advance disabled, and NEXT in
> all other cases.  The check inside the NEXT case for auto-advance is
> to decide if we want to overflow 'i' beyond file_diff_nr to complete
> the session, or we want to wrap-around back to the first file.
>
> But ret can be PREV only under auto-advance disabled, so the check
> there feels totally redundant.
>
> And we want to treat the list of files as a ring buffer only when
> auto-advance is set to false.  This may work in practice but the
> logic feels convoluted.
>
> The patch_update_file() knows how many files there are to decide if
> we want to offer '<' and '>'.  It also knows the file index within
> the file_diff_nr for the file it is handling.  I wonder if it should
> do a bit more with its return value to help the caller?  E.g.,
> perhaps it can return the next 'i' if it wants the caller to advance
> (and decide to do the ring-buffer if needed), or if it wants to tell
> the caller that everything is done by returning some sentinel value
> (e.g., -1)?  Then this part of the caller can just be
>
>         if ((i = patch_update_file(&s, i)) < 0)
>                 break; /* all done */
>
> perhaps?

Yes this makes sense.
Thank you

>
> By the way, I just noticed that the new local variable you added to
> patch_update_file() is called "ret" but that is hiding a different
> variable "int ret" that is used to handle the '/' command.  It
> should be renamed to avoid the name collision.
>

Okay thank you

Abraham

Re: [PATCH v3 2/3] add-patch: Allow interfile navigation when selecting hunks

[Samuel Abraham]

On Fri, Feb 6, 2026 at 8:21 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Abraham Samuel Adekunle <abrahamadekunle50@gmail.com> writes:
>
> > @@ -1566,11 +1589,14 @@ static int patch_update_file(struct add_p_state *s,
> >                                               : 1));
> >               printf(_(s->mode->prompt_mode[prompt_mode_type]),
> >                      s->buf.buf);
> > +             if (s->s.no_auto_advance && all_decided)
> > +                     printf(_("\n%s All hunks decided. What now? "),
> > +                             s->s.prompt_color);
>
> This gives an ordinary prompt for the hunk and then another one
> after it if we notice everything has been decided.  I am wondering
> if it wants to be more like
>
>         if (!s->auto_advance && all_decided)
>                 say What now?
>         else
>                 ask the usual
>
> ?

Okay noted
Thank you

Abraham

Re: [PATCH v3 3/3] add-patch: Allow proper 'git apply' when using the --rework-with-file flag

[Samuel Abraham]

On Fri, Feb 6, 2026 at 8:02 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Abraham Samuel Adekunle <abrahamadekunle50@gmail.com> writes:
>
> > Subject: Re: [PATCH v3 3/3] add-patch: Allow proper 'git apply' when using the --rework-with-file flag
>
> Style.  Downcase "Allow".  Applies to [2/3].
>
> Avoid "proper" as it is not obvious to everybody what you find
> proper and why you find it proper.  Applies to any value-judgement
> adjective.
>
>     Subject: [PATCH v3 3/3] add-patch: allow all-or-none application of a patch
>
> or something?

Okay

>
> > +static void apply_patch(struct add_p_state *s, struct file_diff *file_diff)
> > +{
> > +     struct child_process cp = CHILD_PROCESS_INIT;
> > +     size_t j;
> > +
> > +             /* Any hunk to be used? */
>
> Funny indentaion?

Sorry

>
> > +     for (j = 0; j < file_diff->hunk_nr; j++)
> > +             if (file_diff->hunk[j].use == USE_HUNK)
> > +                     break;
> > +
> > +     if (j < file_diff->hunk_nr ||
> > +             (!file_diff->hunk_nr && file_diff->head.use == USE_HUNK)) {
> > +             /* At least one hunk selected: apply */
> > +             strbuf_reset(&s->buf);
> > +             reassemble_patch(s, file_diff, 0, &s->buf);
> > +
> > +             discard_index(s->s.r->index);
> > +             if (s->mode->apply_for_checkout)
> > +                     apply_for_checkout(s, &s->buf,
> > +                                     s->mode->is_reverse);
> > +             else {
> > +                     setup_child_process(s, &cp, "apply", NULL);
> > +                     strvec_pushv(&cp.args, s->mode->apply_args);
> > +                     if (pipe_command(&cp, s->buf.buf, s->buf.len,
> > +                                     NULL, 0, NULL, 0))
> > +                             error(_("'git apply' failed"));
> > +             }
> > +             if (repo_read_index(s->s.r) >= 0)
> > +                     repo_refresh_and_write_index(s->s.r, REFRESH_QUIET, 0,
> > +                                                     1, NULL, NULL, NULL);
> > +     }
> > +
> > +}
>
> I suspect that the extraction of this helper function out of its
> original place in patch_update_file() should be done in its own
> patch.

Okay

>
> Do we need new tests to cover this new feature?

Yes I will include the tests in the next version.
Thanks

Abraham

Re: [PATCH v3 0/3] introduce new option `rework-with-file`

[Samuel Abraham]

On Fri, Feb 6, 2026 at 8:19 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Abraham Samuel Adekunle <abrahamadekunle50@gmail.com> writes:
>
> > Abraham Samuel Adekunle (3):
> >   interactive -p: add new `--rework-with-file` flag to interactive
> >     machinery
> >   add-patch: Allow interfile navigation when selecting hunks
> >   add-patch: Allow proper 'git apply' when using the --rework-with-file
> >     flag
>
> By the way, this series is conflicting with your other series that
> has been in 'next' and is ready to graduate.  Perhaps it is time to
> consider rebasing.

:D
Okay thank you very much.

Abraham

Re: [PATCH v3 2/3] add-patch: Allow interfile navigation when selecting hunks

[Samuel Abraham]

On Fri, Feb 6, 2026 at 8:21 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Abraham Samuel Adekunle <abrahamadekunle50@gmail.com> writes:
>
> > @@ -1566,11 +1589,14 @@ static int patch_update_file(struct add_p_state *s,
> >                                               : 1));
> >               printf(_(s->mode->prompt_mode[prompt_mode_type]),
> >                      s->buf.buf);
> > +             if (s->s.no_auto_advance && all_decided)
> > +                     printf(_("\n%s All hunks decided. What now? "),
> > +                             s->s.prompt_color);
>
> This gives an ordinary prompt for the hunk and then another one
> after it if we notice everything has been decided.  I am wondering
> if it wants to be more like
>
>         if (!s->auto_advance && all_decided)
>                 say What now?
>         else
>                 ask the usual
>
> ?

Hello Junio
Please just a small curiosity.

If I do it this way, the user will not be able to see the options available
once they have decided on all hunks and want to rework the file.
The options for a hunk will not be visible if they navigate with say K or J
and want to change decisions on a hunk.
They will always be greeted with What now? without the available options.

Re: [PATCH v3 2/3] add-patch: Allow interfile navigation when selecting hunks

[Junio C Hamano]

Samuel Abraham <abrahamadekunle50@gmail.com> writes:

> On Fri, Feb 6, 2026 at 8:21 PM Junio C Hamano <gitster@pobox.com> wrote:
>>
>> Abraham Samuel Adekunle <abrahamadekunle50@gmail.com> writes:
>>
>> > @@ -1566,11 +1589,14 @@ static int patch_update_file(struct add_p_state *s,
>> >                                               : 1));
>> >               printf(_(s->mode->prompt_mode[prompt_mode_type]),
>> >                      s->buf.buf);
>> > +             if (s->s.no_auto_advance && all_decided)
>> > +                     printf(_("\n%s All hunks decided. What now? "),
>> > +                             s->s.prompt_color);
>>
>> This gives an ordinary prompt for the hunk and then another one
>> after it if we notice everything has been decided.  I am wondering
>> if it wants to be more like
>>
>>         if (!s->auto_advance && all_decided)
>>                 say What now?
>>         else
>>                 ask the usual
>>
>> ?
>
> Hello Junio
> Please just a small curiosity.
>
> If I do it this way, the user will not be able to see the options available
> once they have decided on all hunks and want to rework the file.
> The options for a hunk will not be visible if they navigate with say K or J
> and want to change decisions on a hunk.
> They will always be greeted with What now? without the available options.

Ah, OK.

But then after deciding on all hunks and not telling the prompt to
move to another file, the user will keep seeing this extra line of
prompt?

It somehow smells like a waste of a whole line just to remind the
user that all hunks in the file have now been decided.

There was a separate topic that added "(was: [yn])" to the prompt
when the prompt asks about a hunk that already has been decided on.
As we only need a single bit "all hunks decided", can we do
something similar, I wonder?  At the beginning of the main prompt,
we show which of the N available hunks we are currently at, e.g.,

 (1/2) Stage this hunk (was: y) [y,n,q,a,d,k,K,j,J,g,/,e,p,P,?]?

Perhaps we can add a third number to indicate how many of the
available hunks the user has already decided, or something, that can
be used to avoid this wasted line?  Or is it a good thing that we
are loud in this case using a whole line to remind the user that it
may be time to move on?  I dunno.

In any case, even though I am not 100% sure that this design to
devote an extra line for this single bit of information is the best
one, I now understand the need for conveying it.  Thanks.

Re: [PATCH v3 2/3] add-patch: Allow interfile navigation when selecting hunks

[Samuel Abraham]

On Thu, Feb 12, 2026 at 6:25 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Samuel Abraham <abrahamadekunle50@gmail.com> writes:
>
> > On Fri, Feb 6, 2026 at 8:21 PM Junio C Hamano <gitster@pobox.com> wrote:
> >>
> >> Abraham Samuel Adekunle <abrahamadekunle50@gmail.com> writes:
> >>
> >> > @@ -1566,11 +1589,14 @@ static int patch_update_file(struct add_p_state *s,
> >> >                                               : 1));
> >> >               printf(_(s->mode->prompt_mode[prompt_mode_type]),
> >> >                      s->buf.buf);
> >> > +             if (s->s.no_auto_advance && all_decided)
> >> > +                     printf(_("\n%s All hunks decided. What now? "),
> >> > +                             s->s.prompt_color);
> >>
> >> This gives an ordinary prompt for the hunk and then another one
> >> after it if we notice everything has been decided.  I am wondering
> >> if it wants to be more like
> >>
> >>         if (!s->auto_advance && all_decided)
> >>                 say What now?
> >>         else
> >>                 ask the usual
> >>
> >> ?
> >
> > Hello Junio
> > Please just a small curiosity.
> >
> > If I do it this way, the user will not be able to see the options available
> > once they have decided on all hunks and want to rework the file.
> > The options for a hunk will not be visible if they navigate with say K or J
> > and want to change decisions on a hunk.
> > They will always be greeted with What now? without the available options.
>
> Ah, OK.
>
> But then after deciding on all hunks and not telling the prompt to
> move to another file, the user will keep seeing this extra line of
> prompt?
>
> It somehow smells like a waste of a whole line just to remind the
> user that all hunks in the file have now been decided.
>
> There was a separate topic that added "(was: [yn])" to the prompt
> when the prompt asks about a hunk that already has been decided on.
> As we only need a single bit "all hunks decided", can we do
> something similar, I wonder?  At the beginning of the main prompt,
> we show which of the N available hunks we are currently at, e.g.,
>
>  (1/2) Stage this hunk (was: y) [y,n,q,a,d,k,K,j,J,g,/,e,p,P,?]?
>
> Perhaps we can add a third number to indicate how many of the
> available hunks the user has already decided, or something, that can
> be used to avoid this wasted line?  Or is it a good thing that we
> are loud in this case using a whole line to remind the user that it
> may be time to move on?  I dunno.

I thought of a suggestion where after deciding on all hunks in the
file, the user
will be able to see the "what now prompt", the options for the current hunk and
also the previous decision on the hunk since at this point, all the
hunks would have been decided on.

I tried something like

What now? (was: n) [y,n,q,a,d,s,e,>,<,p,P,?]?

This does not show the number of the hunk we are currently at and the
"Stage this hunk" since the decision had been made initially but the "whatnow"
prompt still provides a chance to change the decision, while showing
the previous
decision on the hunk by asking "What now?" instead.
The options have the default [y,n,q,a,d] and the remaining options are populated
from the permit set for the hunk. SO the user can still carry out the
normal actions on
the hunk.

In response to your earlier question, if the user decides on all hunks in a
file and does not go to the next file, he'll see the prompt above and
that is what will keep
showing if he remains in the file, no extra line.
If he navigates away, the hunk re-renders with the "what now" prompt
when he comes
back.
If he had made all decisions in a file and decides to split a
splittable hunk, then the normal
prompt shows for those hunks since they are now undecided.

What do you think about this?
Thanks

Abraham

Re: [PATCH v3 2/3] add-patch: Allow interfile navigation when selecting hunks

[Junio C Hamano]

Samuel Abraham <abrahamadekunle50@gmail.com> writes:

>> There was a separate topic that added "(was: [yn])" to the prompt
>> when the prompt asks about a hunk that already has been decided on.
>> As we only need a single bit "all hunks decided", can we do
>> something similar, I wonder?  At the beginning of the main prompt,
>> we show which of the N available hunks we are currently at, e.g.,
>>
>>  (1/2) Stage this hunk (was: y) [y,n,q,a,d,k,K,j,J,g,/,e,p,P,?]?
>>
>> Perhaps we can add a third number to indicate how many of the
>> available hunks the user has already decided, or something, that can
>> be used to avoid this wasted line?  Or is it a good thing that we
>> are loud in this case using a whole line to remind the user that it
>> may be time to move on?  I dunno.
>
> I thought of a suggestion where after deciding on all hunks in the
> file, the user
> will be able to see the "what now prompt", the options for the current hunk and
> also the previous decision on the hunk since at this point, all the
> hunks would have been decided on.
>
> I tried something like
>
> What now? (was: n) [y,n,q,a,d,s,e,>,<,p,P,?]?
>
> This does not show the number of the hunk we are currently at and the
> "Stage this hunk" since the decision had been made initially but the "whatnow"
> prompt still provides a chance to change the decision, while showing
> the previous
> decision on the hunk by asking "What now?" instead.
> The options have the default [y,n,q,a,d] and the remaining options are populated
> from the permit set for the hunk. SO the user can still carry out the
> normal actions on
> the hunk.

I like the compactness of that myself, but I have to say that the
end-users may feel lost and utterly confused with the distinction,
if they are left without being explained why we switch between
"Stage this" (which by the way changes phrasing depending on what
you are doing) and "What now".

Is it so important to indicate that everything in the hunk has been
decided?  They'd lose 'j' 'k' when there no longer remains undecided
ones, and every hunk they revisit with 'J' or 'K' would say (was: X),
which may be a clue enough that they are done with the file, and
when they really really wanted to make sure, perhaps they can type
'?' and that help can spend a line to say "Out of 8 hunks, you have
already decided to use 3 hunks, and skip 5 hunks" or something?

I dunno.

Re: [PATCH v3 2/3] add-patch: Allow interfile navigation when selecting hunks

[Samuel Abraham]

On Thu, Feb 12, 2026 at 10:31 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Samuel Abraham <abrahamadekunle50@gmail.com> writes:
>
> >> There was a separate topic that added "(was: [yn])" to the prompt
> >> when the prompt asks about a hunk that already has been decided on.
> >> As we only need a single bit "all hunks decided", can we do
> >> something similar, I wonder?  At the beginning of the main prompt,
> >> we show which of the N available hunks we are currently at, e.g.,
> >>
> >>  (1/2) Stage this hunk (was: y) [y,n,q,a,d,k,K,j,J,g,/,e,p,P,?]?
> >>
> >> Perhaps we can add a third number to indicate how many of the
> >> available hunks the user has already decided, or something, that can
> >> be used to avoid this wasted line?  Or is it a good thing that we
> >> are loud in this case using a whole line to remind the user that it
> >> may be time to move on?  I dunno.
> >
> > I thought of a suggestion where after deciding on all hunks in the
> > file, the user
> > will be able to see the "what now prompt", the options for the current hunk and
> > also the previous decision on the hunk since at this point, all the
> > hunks would have been decided on.
> >
> > I tried something like
> >
> > What now? (was: n) [y,n,q,a,d,s,e,>,<,p,P,?]?
> >
> > This does not show the number of the hunk we are currently at and the
> > "Stage this hunk" since the decision had been made initially but the "whatnow"
> > prompt still provides a chance to change the decision, while showing
> > the previous
> > decision on the hunk by asking "What now?" instead.
> > The options have the default [y,n,q,a,d] and the remaining options are populated
> > from the permit set for the hunk. SO the user can still carry out the
> > normal actions on
> > the hunk.
>
> I like the compactness of that myself, but I have to say that the
> end-users may feel lost and utterly confused with the distinction,
> if they are left without being explained why we switch between
> "Stage this" (which by the way changes phrasing depending on what
> you are doing) and "What now".

Yes I agree.

>
> Is it so important to indicate that everything in the hunk has been
> decided?  They'd lose 'j' 'k' when there no longer remains undecided
> ones, and every hunk they revisit with 'J' or 'K' would say (was: X),
> which may be a clue enough that they are done with the file, and
> when they really really wanted to make sure, perhaps they can type
> '?' and that help can spend a line to say "Out of 8 hunks, you have
> already decided to use 3 hunks, and skip 5 hunks" or something?
>
> I dunno.

Yes I also think this works.
It is not so important to indicate that everything has been decided.
The (was: X) shows that the hunk has been decided on and it
should be enough.
I will add the information to the help_patch section.
Thanks

Abraham

[PATCH v4 0/4] introduce new option `--auto-advance`

[Abraham Samuel Adekunle]

Hello,

After after more reviews and deliberations, I have been able to
rename the new option name to `--auto-advance`, where the
--no-auto-advance implements the feature and does not auto advance
while --auto-advance is the default and maintains the current
behaviour.

With the option, users can navigate in between files while deciding
on hunks as they wish with the '>' and '<' option for going to the
next and previous file respectively if there are more than one file.

Patch 1 implements the new `--no-auto-advance` options, Patch 2
modifies the function `patch_update_file()` to instead take the index
of the file as parameter instead of the file_diff. Patch 3 moves the
'git apply' logic into a function so that we can reuse this logic when
implementing the all or none application of patches.
Patch 4 implements the interfile navigation, and adds tests to the
interactive test file.

Changes in v4:
==============
- Renamed option to `--no-auto-advance` with `auto-advance` being
  the default option
- Modified the function signature of 'patch_update_file()' to accept
  the index of the file diff
- Moved git apply logic into function for reuse
- Removed the whatnow prompt. Now the hunks in the file keep
  showing even after all hunks have been decided
- Added hunk summary to patch help remainder to show the user the
  hunk deails
- Added tests ot t3701-add-interactive.sh

Abraham Samuel Adekunle (4):
  interactive -p: add new `--auto-advance` flag
  add-patch: modify patch_update_file() signature
  add-patch: allow all-or-none application of patches
  add-patch: allow interfile navigation when selecting hunks

 add-interactive.c          |   4 +
 add-interactive.h          |   5 +-
 add-patch.c                | 157 +++++++++++++++++++++++++++----------
 builtin/add.c              |   4 +
 builtin/checkout.c         |   7 ++
 builtin/reset.c            |   4 +
 builtin/stash.c            |   8 ++
 t/t3701-add-interactive.sh | 100 +++++++++++++++++++++++
 t/t9902-completion.sh      |   1 +
 9 files changed, 246 insertions(+), 44 deletions(-)

-- 
2.39.5 (Apple Git-154)

[PATCH v4 1/4] interactive -p: add new `--auto-advance` flag

[Abraham Samuel Adekunle]

When using the interactive add, reset, stash or checkout machinery,
we do not have the option of reworking with a file when selecting
hunks, because the session automatically advances to the next file
or ends if we have just one file.

Introduce the flag `--auto-advance` which auto advances by default,
when interactively selecting patches with the '--patch' option.
However, the `--no-auto-advance` option does not auto advance, thereby
allowing users the option to rework with files.

Signed-off-by: Abraham Samuel Adekunle <abrahamadekunle50@gmail.com>
---
 add-interactive.c     | 4 ++++
 add-interactive.h     | 5 +++--
 builtin/add.c         | 4 ++++
 builtin/checkout.c    | 7 +++++++
 builtin/reset.c       | 4 ++++
 builtin/stash.c       | 8 ++++++++
 t/t9902-completion.sh | 1 +
 7 files changed, 31 insertions(+), 2 deletions(-)

diff --git a/add-interactive.c b/add-interactive.c
index 95ec5a89f8..c3a36cd11f 100644
--- a/add-interactive.c
+++ b/add-interactive.c
@@ -64,6 +64,7 @@ void init_add_i_state(struct add_i_state *s, struct repository *r,
 	s->r = r;
 	s->context = -1;
 	s->interhunkcontext = -1;
+	s->auto_advance = 1;
 
 	s->use_color_interactive = check_color_config(r, "color.interactive");
 
@@ -124,6 +125,8 @@ void init_add_i_state(struct add_i_state *s, struct repository *r,
 			die(_("%s cannot be negative"), "--inter-hunk-context");
 		s->interhunkcontext = add_p_opt->interhunkcontext;
 	}
+	if (!add_p_opt->auto_advance)
+		s->auto_advance = 0;
 }
 
 void clear_add_i_state(struct add_i_state *s)
@@ -1017,6 +1020,7 @@ static int run_patch(struct add_i_state *s, const struct pathspec *ps,
 		struct add_p_opt add_p_opt = {
 			.context = s->context,
 			.interhunkcontext = s->interhunkcontext,
+			.auto_advance = s->auto_advance
 		};
 		struct strvec args = STRVEC_INIT;
 		struct pathspec ps_selected = { 0 };
diff --git a/add-interactive.h b/add-interactive.h
index da49502b76..cea29a6965 100644
--- a/add-interactive.h
+++ b/add-interactive.h
@@ -6,9 +6,10 @@
 struct add_p_opt {
 	int context;
 	int interhunkcontext;
+	int auto_advance;
 };
 
-#define ADD_P_OPT_INIT { .context = -1, .interhunkcontext = -1 }
+#define ADD_P_OPT_INIT { .context = -1, .interhunkcontext = -1, .auto_advance = 1 }
 
 struct add_i_state {
 	struct repository *r;
@@ -28,7 +29,7 @@ struct add_i_state {
 
 	int use_single_key;
 	char *interactive_diff_filter, *interactive_diff_algorithm;
-	int context, interhunkcontext;
+	int context, interhunkcontext, auto_advance;
 };
 
 void init_add_i_state(struct add_i_state *s, struct repository *r,
diff --git a/builtin/add.c b/builtin/add.c
index 32709794b3..4357f87b7f 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -256,6 +256,8 @@ static struct option builtin_add_options[] = {
 	OPT_GROUP(""),
 	OPT_BOOL('i', "interactive", &add_interactive, N_("interactive picking")),
 	OPT_BOOL('p', "patch", &patch_interactive, N_("select hunks interactively")),
+	OPT_BOOL(0, "auto-advance", &add_p_opt.auto_advance,
+		 N_("auto advance to the next file when selecting hunks interactively")),
 	OPT_DIFF_UNIFIED(&add_p_opt.context),
 	OPT_DIFF_INTERHUNK_CONTEXT(&add_p_opt.interhunkcontext),
 	OPT_BOOL('e', "edit", &edit_interactive, N_("edit current diff and apply")),
@@ -418,6 +420,8 @@ int cmd_add(int argc,
 			die(_("the option '%s' requires '%s'"), "--unified", "--interactive/--patch");
 		if (add_p_opt.interhunkcontext != -1)
 			die(_("the option '%s' requires '%s'"), "--inter-hunk-context", "--interactive/--patch");
+		if (!add_p_opt.auto_advance)
+			die(_("the option '%s' requires '%s'"), "--no-auto-advance", "--interactive/--patch");
 	}
 
 	if (edit_interactive) {
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 0ba4f03f2e..fad35a9284 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -63,6 +63,7 @@ struct checkout_opts {
 	int patch_mode;
 	int patch_context;
 	int patch_interhunk_context;
+	int auto_advance;
 	int quiet;
 	int merge;
 	int force;
@@ -111,6 +112,7 @@ struct checkout_opts {
 	.merge = -1, \
 	.patch_context = -1, \
 	.patch_interhunk_context = -1, \
+	.auto_advance = 1, \
 }
 
 struct branch_info {
@@ -549,6 +551,7 @@ static int checkout_paths(const struct checkout_opts *opts,
 		struct add_p_opt add_p_opt = {
 			.context = opts->patch_context,
 			.interhunkcontext = opts->patch_interhunk_context,
+			.auto_advance = opts->auto_advance
 		};
 		const char *rev = new_branch_info->name;
 		char rev_oid[GIT_MAX_HEXSZ + 1];
@@ -1803,6 +1806,8 @@ static int checkout_main(int argc, const char **argv, const char *prefix,
 			die(_("the option '%s' requires '%s'"), "--unified", "--patch");
 		if (opts->patch_interhunk_context != -1)
 			die(_("the option '%s' requires '%s'"), "--inter-hunk-context", "--patch");
+		if (!opts->auto_advance)
+			die(_("the option '%s' requires '%s'"), "--no-auto-advance", "--patch");
 	}
 
 	if (opts->show_progress < 0) {
@@ -2001,6 +2006,8 @@ int cmd_checkout(int argc,
 		OPT_BOOL(0, "guess", &opts.dwim_new_local_branch,
 			 N_("second guess 'git checkout <no-such-branch>' (default)")),
 		OPT_BOOL(0, "overlay", &opts.overlay_mode, N_("use overlay mode (default)")),
+		OPT_BOOL(0, "auto-advance", &opts.auto_advance,
+			 N_("auto advance to the next file when selecting hunks interactively")),
 		OPT_END()
 	};
 
diff --git a/builtin/reset.c b/builtin/reset.c
index c48d9845f8..88f95f9fc7 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -371,6 +371,8 @@ int cmd_reset(int argc,
 			       PARSE_OPT_OPTARG,
 			       option_parse_recurse_submodules_worktree_updater),
 		OPT_BOOL('p', "patch", &patch_mode, N_("select hunks interactively")),
+		OPT_BOOL(0, "auto-advance", &add_p_opt.auto_advance,
+			 N_("auto advance to the next file when selecting hunks interactively")),
 		OPT_DIFF_UNIFIED(&add_p_opt.context),
 		OPT_DIFF_INTERHUNK_CONTEXT(&add_p_opt.interhunkcontext),
 		OPT_BOOL('N', "intent-to-add", &intent_to_add,
@@ -443,6 +445,8 @@ int cmd_reset(int argc,
 			die(_("the option '%s' requires '%s'"), "--unified", "--patch");
 		if (add_p_opt.interhunkcontext != -1)
 			die(_("the option '%s' requires '%s'"), "--inter-hunk-context", "--patch");
+		if (!add_p_opt.auto_advance)
+			die(_("the option '%s' requires '%s'"), "--no-auto-advance", "--patch");
 	}
 
 	/* git reset tree [--] paths... can be used to
diff --git a/builtin/stash.c b/builtin/stash.c
index 193e3ea47a..f98487f4cd 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -1849,6 +1849,8 @@ static int push_stash(int argc, const char **argv, const char *prefix,
 			 N_("stash staged changes only")),
 		OPT_BOOL('p', "patch", &patch_mode,
 			 N_("stash in patch mode")),
+		OPT_BOOL(0, "auto-advance", &add_p_opt.auto_advance,
+			 N_("auto advance to the next file when selecting hunks interactively")),
 		OPT_DIFF_UNIFIED(&add_p_opt.context),
 		OPT_DIFF_INTERHUNK_CONTEXT(&add_p_opt.interhunkcontext),
 		OPT__QUIET(&quiet, N_("quiet mode")),
@@ -1911,6 +1913,8 @@ static int push_stash(int argc, const char **argv, const char *prefix,
 			die(_("the option '%s' requires '%s'"), "--unified", "--patch");
 		if (add_p_opt.interhunkcontext != -1)
 			die(_("the option '%s' requires '%s'"), "--inter-hunk-context", "--patch");
+		if (!add_p_opt.auto_advance)
+			die(_("the option '%s' requires '%s'"), "--no-auto-advance", "--patch");
 	}
 
 	if (add_p_opt.context < -1)
@@ -1952,6 +1956,8 @@ static int save_stash(int argc, const char **argv, const char *prefix,
 			 N_("stash staged changes only")),
 		OPT_BOOL('p', "patch", &patch_mode,
 			 N_("stash in patch mode")),
+		OPT_BOOL(0, "auto-advance", &add_p_opt.auto_advance,
+			 N_("auto advance to the next file when selecting hunks interactively")),
 		OPT_DIFF_UNIFIED(&add_p_opt.context),
 		OPT_DIFF_INTERHUNK_CONTEXT(&add_p_opt.interhunkcontext),
 		OPT__QUIET(&quiet, N_("quiet mode")),
@@ -1983,6 +1989,8 @@ static int save_stash(int argc, const char **argv, const char *prefix,
 			die(_("the option '%s' requires '%s'"), "--unified", "--patch");
 		if (add_p_opt.interhunkcontext != -1)
 			die(_("the option '%s' requires '%s'"), "--inter-hunk-context", "--patch");
+		if (!add_p_opt.auto_advance)
+			die(_("the option '%s' requires '%s'"), "--no-auto-advance", "--patch");
 	}
 
 	ret = do_push_stash(&ps, stash_msg, quiet, keep_index,
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index ffb9c8b522..2f9a597ec7 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -2601,6 +2601,7 @@ test_expect_success 'double dash "git checkout"' '
 	--ignore-skip-worktree-bits Z
 	--ignore-other-worktrees Z
 	--recurse-submodules Z
+	--auto-advance Z
 	--progress Z
 	--guess Z
 	--no-guess Z
-- 
2.39.5 (Apple Git-154)

[PATCH v4 2/4] add-patch: modify patch_update_file() signature

[Abraham Samuel Adekunle]

The function `patch_update_file()` takes the `add_p_state` struct
pointer and the current `struct file_diff` pointer and returns an
int.

When using the `--no-auto-advance` flag, we want to be able to request
the next or previous file from the caller.

Modify the function signature to instead take the index of the
current `file_diff` and the `add_p_state` struct pointer so that we
can compute the `file_diff` from the index while also having
access to the file index. This will help us request the next or
previous file from the caller.

Signed-off-by: Abraham Samuel Adekunle <abrahamadekunle50@gmail.com>
---
 add-patch.c | 35 ++++++++++++++++++++++-------------
 1 file changed, 22 insertions(+), 13 deletions(-)

diff --git a/add-patch.c b/add-patch.c
index df8f2e6d74..673ea659ff 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -1441,20 +1441,21 @@ static bool get_first_undecided(const struct file_diff *file_diff, size_t *idx)
 	return false;
 }
 
-static int patch_update_file(struct add_p_state *s,
-			     struct file_diff *file_diff)
+static ssize_t patch_update_file(struct add_p_state *s, size_t idx)
 {
 	size_t hunk_index = 0;
 	ssize_t i, undecided_previous, undecided_next, rendered_hunk_index = -1;
 	struct hunk *hunk;
 	char ch;
 	struct child_process cp = CHILD_PROCESS_INIT;
-	int colored = !!s->colored.len, quit = 0, use_pager = 0;
+	int colored = !!s->colored.len, use_pager = 0;
 	enum prompt_mode_type prompt_mode_type;
+	struct file_diff *file_diff = s->file_diff + idx;
+	ssize_t patch_update_resp = (ssize_t)idx;
 
 	/* Empty added files have no hunks */
 	if (!file_diff->hunk_nr && !file_diff->added)
-		return 0;
+		return patch_update_resp + 1;
 
 	strbuf_reset(&s->buf);
 	render_diff_header(s, file_diff, colored, &s->buf);
@@ -1499,9 +1500,10 @@ static int patch_update_file(struct add_p_state *s,
 
 		/* Everything decided? */
 		if (undecided_previous < 0 && undecided_next < 0 &&
-		    hunk->use != UNDECIDED_HUNK)
-			break;
-
+		    hunk->use != UNDECIDED_HUNK) {
+				patch_update_resp++;
+				break;
+		}
 		strbuf_reset(&s->buf);
 		if (file_diff->hunk_nr) {
 			if (rendered_hunk_index != hunk_index) {
@@ -1577,7 +1579,7 @@ static int patch_update_file(struct add_p_state *s,
 			fputs(s->s.reset_color_interactive, stdout);
 		fflush(stdout);
 		if (read_single_character(s) == EOF) {
-			quit = 1;
+			patch_update_resp = -1;
 			break;
 		}
 
@@ -1623,7 +1625,7 @@ static int patch_update_file(struct add_p_state *s,
 				hunk->use = SKIP_HUNK;
 			}
 		} else if (ch == 'q') {
-			quit = 1;
+			patch_update_resp = -1;
 			break;
 		} else if (s->answer.buf[0] == 'K') {
 			if (permitted & ALLOW_GOTO_PREVIOUS_HUNK)
@@ -1810,7 +1812,7 @@ static int patch_update_file(struct add_p_state *s,
 	}
 
 	putchar('\n');
-	return quit;
+	return patch_update_resp;
 }
 
 int run_add_p(struct repository *r, enum add_p_mode mode,
@@ -1821,6 +1823,7 @@ int run_add_p(struct repository *r, enum add_p_mode mode,
 		{ r }, STRBUF_INIT, STRBUF_INIT, STRBUF_INIT, STRBUF_INIT
 	};
 	size_t i, binary_count = 0;
+	ssize_t patch_update_resp;
 
 	init_add_i_state(&s.s, r, o);
 
@@ -1859,11 +1862,17 @@ int run_add_p(struct repository *r, enum add_p_mode mode,
 		return -1;
 	}
 
-	for (i = 0; i < s.file_diff_nr; i++)
-		if (s.file_diff[i].binary && !s.file_diff[i].hunk_nr)
+	for (i = 0; i < s.file_diff_nr;) {
+		if (s.file_diff[i].binary && !s.file_diff[i].hunk_nr) {
 			binary_count++;
-		else if (patch_update_file(&s, s.file_diff + i))
+			i++;
+			continue;
+		}
+		patch_update_resp = patch_update_file(&s, i);
+		if (patch_update_resp < 0)
 			break;
+		i = (size_t)patch_update_resp;
+    }
 
 	if (s.file_diff_nr == 0)
 		err(&s, _("No changes."));
-- 
2.39.5 (Apple Git-154)

[PATCH v4 3/4] add-patch: allow all-or-none application of patches

[Abraham Samuel Adekunle]

When the flag `--no-auto-advance` is used with `--patch`,
if the user has decided `USE` on a hunk in a file, goes to another
file, and then returns to this file and changes the previous
decision on the hunk to `SKIP`, because the patch has already
been applied, the last decision is not registered and the now
SKIPPED hunk is still applied.

Move the logic for applying patches into a function so that we can
reuse this logic to implement the all or non application of the patches
after the user is done with the hunk selection.

Signed-off-by: Abraham Samuel Adekunle <abrahamadekunle50@gmail.com>
---
 add-patch.c | 62 ++++++++++++++++++++++++++++++-----------------------
 1 file changed, 35 insertions(+), 27 deletions(-)

diff --git a/add-patch.c b/add-patch.c
index 673ea659ff..7d4f17e432 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -1420,6 +1420,40 @@ N_("j - go to the next undecided hunk, roll over at the bottom\n"
    "P - print the current hunk using the pager\n"
    "? - print help\n");
 
+static void apply_patch(struct add_p_state *s, struct file_diff *file_diff)
+{
+	struct child_process cp = CHILD_PROCESS_INIT;
+	size_t j;
+
+	/* Any hunk to be used? */
+	for (j = 0; j < file_diff->hunk_nr; j++)
+		if (file_diff->hunk[j].use == USE_HUNK)
+			break;
+
+	if (j < file_diff->hunk_nr ||
+		(!file_diff->hunk_nr && file_diff->head.use == USE_HUNK)) {
+		/* At least one hunk selected: apply */
+		strbuf_reset(&s->buf);
+		reassemble_patch(s, file_diff, 0, &s->buf);
+
+		discard_index(s->s.r->index);
+		if (s->mode->apply_for_checkout)
+			apply_for_checkout(s, &s->buf,
+					s->mode->is_reverse);
+		else {
+			setup_child_process(s, &cp, "apply", NULL);
+			strvec_pushv(&cp.args, s->mode->apply_args);
+			if (pipe_command(&cp, s->buf.buf, s->buf.len,
+					NULL, 0, NULL, 0))
+				error(_("'git apply' failed"));
+		}
+		if (repo_read_index(s->s.r) >= 0)
+			repo_refresh_and_write_index(s->s.r, REFRESH_QUIET, 0,
+							1, NULL, NULL, NULL);
+	}
+
+}
+
 static size_t dec_mod(size_t a, size_t m)
 {
 	return a > 0 ? a - 1 : m - 1;
@@ -1447,7 +1481,6 @@ static ssize_t patch_update_file(struct add_p_state *s, size_t idx)
 	ssize_t i, undecided_previous, undecided_next, rendered_hunk_index = -1;
 	struct hunk *hunk;
 	char ch;
-	struct child_process cp = CHILD_PROCESS_INIT;
 	int colored = !!s->colored.len, use_pager = 0;
 	enum prompt_mode_type prompt_mode_type;
 	struct file_diff *file_diff = s->file_diff + idx;
@@ -1784,32 +1817,7 @@ static ssize_t patch_update_file(struct add_p_state *s, size_t idx)
 		}
 	}
 
-	/* Any hunk to be used? */
-	for (i = 0; i < file_diff->hunk_nr; i++)
-		if (file_diff->hunk[i].use == USE_HUNK)
-			break;
-
-	if (i < file_diff->hunk_nr ||
-	    (!file_diff->hunk_nr && file_diff->head.use == USE_HUNK)) {
-		/* At least one hunk selected: apply */
-		strbuf_reset(&s->buf);
-		reassemble_patch(s, file_diff, 0, &s->buf);
-
-		discard_index(s->s.r->index);
-		if (s->mode->apply_for_checkout)
-			apply_for_checkout(s, &s->buf,
-					   s->mode->is_reverse);
-		else {
-			setup_child_process(s, &cp, "apply", NULL);
-			strvec_pushv(&cp.args, s->mode->apply_args);
-			if (pipe_command(&cp, s->buf.buf, s->buf.len,
-					 NULL, 0, NULL, 0))
-				error(_("'git apply' failed"));
-		}
-		if (repo_read_index(s->s.r) >= 0)
-			repo_refresh_and_write_index(s->s.r, REFRESH_QUIET, 0,
-						     1, NULL, NULL, NULL);
-	}
+	apply_patch(s, file_diff);
 
 	putchar('\n');
 	return patch_update_resp;
-- 
2.39.5 (Apple Git-154)

[PATCH v4 4/4] add-patch: allow interfile navigation when selecting hunks

[Abraham Samuel Adekunle]

After deciding on all hunks in a file, the interactive session
advances automatically to the next file if there is another,
or the process ends.

Now using the `--no-auto-advance` flag with `--patch`, the process
does not advance automatically. A user can choose to go to the next
file by pressing '>' or the previous file by pressing '<', before or
after deciding on all hunks in the current file.

After all hunks have been decided in a file, the user can still
rework with the file by applying the options available in the permit
set for that hunk, and after all the decisions, the user presses 'q'
to submit.
After all hunks have been decided, the user can press '?' which will
show the hunk selection summary in the help patch remainder text
including the total hunks, number of hunks marked for use and number
of hunks marked for skip.

This feature is enabled by passing the `--no-auto-advance` flag
to `--patch` option of the subcommands add, stash, reset,
and checkout.

Signed-off-by: Abraham Samuel Adekunle <abrahamadekunle50@gmail.com>
---
 add-patch.c                |  66 ++++++++++++++++++++++--
 t/t3701-add-interactive.sh | 100 +++++++++++++++++++++++++++++++++++++
 2 files changed, 161 insertions(+), 5 deletions(-)

diff --git a/add-patch.c b/add-patch.c
index 7d4f17e432..6b9ae4da30 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -1418,7 +1418,10 @@ N_("j - go to the next undecided hunk, roll over at the bottom\n"
    "e - manually edit the current hunk\n"
    "p - print the current hunk\n"
    "P - print the current hunk using the pager\n"
-   "? - print help\n");
+   "> - go to the next file, roll over at the bottom\n"
+   "< - go to the previous file, roll over at the top\n"
+   "? - print help\n"
+   "HUNKS SUMMARY - Hunks: %d, USE: %d, SKIP: %d\n");
 
 static void apply_patch(struct add_p_state *s, struct file_diff *file_diff)
 {
@@ -1483,6 +1486,7 @@ static ssize_t patch_update_file(struct add_p_state *s, size_t idx)
 	char ch;
 	int colored = !!s->colored.len, use_pager = 0;
 	enum prompt_mode_type prompt_mode_type;
+	int all_decided = 0;
 	struct file_diff *file_diff = s->file_diff + idx;
 	ssize_t patch_update_resp = (ssize_t)idx;
 
@@ -1502,7 +1506,9 @@ static ssize_t patch_update_file(struct add_p_state *s, size_t idx)
 			ALLOW_GOTO_NEXT_UNDECIDED_HUNK = 1 << 3,
 			ALLOW_SEARCH_AND_GOTO = 1 << 4,
 			ALLOW_SPLIT = 1 << 5,
-			ALLOW_EDIT = 1 << 6
+			ALLOW_EDIT = 1 << 6,
+			ALLOW_GOTO_PREVIOUS_FILE = 1 << 7,
+			ALLOW_GOTO_NEXT_FILE = 1 << 8
 		} permitted = 0;
 
 		if (hunk_index >= file_diff->hunk_nr)
@@ -1534,8 +1540,12 @@ static ssize_t patch_update_file(struct add_p_state *s, size_t idx)
 		/* Everything decided? */
 		if (undecided_previous < 0 && undecided_next < 0 &&
 		    hunk->use != UNDECIDED_HUNK) {
-				patch_update_resp++;
-				break;
+				if (!s->s.auto_advance)
+					all_decided = 1;
+				else {
+					patch_update_resp++;
+					break;
+				}
 		}
 		strbuf_reset(&s->buf);
 		if (file_diff->hunk_nr) {
@@ -1584,6 +1594,14 @@ static ssize_t patch_update_file(struct add_p_state *s, size_t idx)
 				permitted |= ALLOW_EDIT;
 				strbuf_addstr(&s->buf, ",e");
 			}
+			if (!s->s.auto_advance && s->file_diff_nr > 1) {
+				permitted |= ALLOW_GOTO_NEXT_FILE;
+				strbuf_addstr(&s->buf, ",>");
+			}
+			if (!s->s.auto_advance && s->file_diff_nr > 1) {
+				permitted |= ALLOW_GOTO_PREVIOUS_FILE;
+				strbuf_addstr(&s->buf, ",<");
+			}
 			strbuf_addstr(&s->buf, ",p,P");
 		}
 		if (file_diff->deleted)
@@ -1660,6 +1678,28 @@ static ssize_t patch_update_file(struct add_p_state *s, size_t idx)
 		} else if (ch == 'q') {
 			patch_update_resp = -1;
 			break;
+		} else if (!s->s.auto_advance && s->answer.buf[0] == '>') {
+			if (permitted & ALLOW_GOTO_NEXT_FILE) {
+				if (patch_update_resp == s->file_diff_nr - 1)
+					patch_update_resp = 0;
+				else
+					patch_update_resp++;
+				break;
+			} else {
+				err(s, _("No next file"));
+				continue;
+			}
+		} else if (!s->s.auto_advance && s->answer.buf[0] == '<') {
+			if (permitted & ALLOW_GOTO_PREVIOUS_FILE) {
+				if (patch_update_resp == 0)
+					patch_update_resp = s->file_diff_nr - 1;
+				else
+					patch_update_resp--;
+				break;
+			} else {
+				err(s, _("No previous file"));
+				continue;
+			}
 		} else if (s->answer.buf[0] == 'K') {
 			if (permitted & ALLOW_GOTO_PREVIOUS_HUNK)
 				hunk_index = dec_mod(hunk_index,
@@ -1805,6 +1845,18 @@ static ssize_t patch_update_file(struct add_p_state *s, size_t idx)
 				 * commands shown in the prompt that are not
 				 * always available.
 				 */
+				if (all_decided && !strncmp(p, "HUNKS SUMMARY", 13)) {
+					int total = file_diff->hunk_nr, used = 0, skipped = 0;
+
+					for (i = 0; i < file_diff->hunk_nr; i++) {
+						if (file_diff->hunk[i].use == USE_HUNK)
+							used += 1;
+						if (file_diff->hunk[i].use == SKIP_HUNK)
+							skipped += 1;
+					}
+					color_fprintf_ln(stdout, s->s.help_color, _(p),
+							 total, used, skipped);
+				}
 				if (*p != '?' && !strchr(s->buf.buf, *p))
 					continue;
 
@@ -1817,7 +1869,8 @@ static ssize_t patch_update_file(struct add_p_state *s, size_t idx)
 		}
 	}
 
-	apply_patch(s, file_diff);
+	if (s->s.auto_advance)
+		apply_patch(s, file_diff);
 
 	putchar('\n');
 	return patch_update_resp;
@@ -1881,6 +1934,9 @@ int run_add_p(struct repository *r, enum add_p_mode mode,
 			break;
 		i = (size_t)patch_update_resp;
     }
+	if (!s.s.auto_advance)
+		for (i = 0; i < s.file_diff_nr; i++)
+			apply_patch(&s, s.file_diff + i);
 
 	if (s.file_diff_nr == 0)
 		err(&s, _("No changes."));
diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index 5ce9c6dd60..6e120a4001 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -1441,5 +1441,105 @@ test_expect_success 'EOF quits' '
 	test_grep file out &&
 	test_grep ! file2 out
 '
+for cmd in add checkout reset "stash save" "stash push"
+do
+	test_expect_success "$cmd rejects invalid --no-auto-advance options" '
+		test_must_fail git $cmd --no-auto-advance 2>actual &&
+		test_grep -E  "requires .*--(interactive|patch)" actual
+	'
+done
+
+test_expect_success 'manual advance (">") moves to next file with --no-auto-advance' '
+	git reset --hard &&
+	echo line1 >first-file &&
+	echo line2 >second-file &&
+	git add -A &&
+	git commit -m initial >/dev/null 2>&1 &&
+	echo change_first >>first-file &&
+	echo change_second >>second-file &&
+
+	printf ">\nq\n" | git add -p --no-auto-advance >output.test 2>&1 &&
+	test_grep  -E "(a|b)/second-file" output.test
+'
+
+test_expect_success 'select n on a hunk, go to another file, come back and change to y stages' '
+	git reset --hard &&
+	echo one >f1 &&
+	echo one >f2 &&
+	git add -A &&
+	git commit -m initial >/dev/null 2>&1 &&
+	echo change1 >>f1 &&
+	echo change2 >>f2 &&
+
+	printf "n\n>\n<\ny\nq\n" | git add -p --no-auto-advance >output.staged 2>&1 &&
+	git diff --cached --name-only >staged &&
+	test_grep -E "(a/f1)" output.staged
+'
+
+test_expect_success 'select y on a hunk, go to another file, come back and change to n does not stage' '
+	git reset --hard &&
+	echo one >f1 &&
+	echo one >f2 &&
+	git add -A &&
+	git commit -m initial >/dev/null 2>&1 &&
+	echo change1 >>f1 &&
+	echo change2 >>f2 &&
+
+	printf "y\n>\n<\nn\nq\n" | git add -p --no-auto-advance >output.unstaged 2>&1 &&
+	git diff --cached --name-only >staged &&
+	test_must_be_empty staged
+'
+
+test_expect_success 'deciding all hunks in a file does not auto advance' '
+	git reset --hard &&
+	echo line >stay &&
+	echo line >other &&
+	git add -A &&
+	git commit -m initial >/dev/null 2>&1 &&
+	echo change >>stay &&
+	echo change >>other &&
+	test_write_lines y | git add -p --no-auto-advance >raw-output 2>&1 &&
+	test_grep "(1/1) Stage this hunk (was: y)" raw-output &&
+	test_grep ! "diff --git a/stay b/stay" raw-output
+'
+test_expect_success 'HUNKS SUMMARY does not show in help text when there are undecided hunks' '
+	git reset --hard &&
+	test_write_lines 1 2 3 4 5 6 7 8 9 >f &&
+	git add f &&
+	git commit -m initial >/dev/null 2>&1 &&
+	test_write_lines 1 X 3 4 Y 6 7 Z 9 >f &&
+	test_write_lines s y n | git add -p --no-auto-advance >raw-nostat 2>&1 &&
+	test_grep ! "HUNKS SUMMARY - Hunks: " raw-nostat
+'
+
+test_expect_success 'help text shows HUNK SUMMARY when all hunks have been decided' '
+	git reset --hard &&
+	test_write_lines 1 2 3 4 5 6 7 8 9 >f2 &&
+	git add f2 &&
+	git commit -m initial >/dev/null 2>&1 &&
+	test_write_lines 1 X 3 4 Y 6 7 Z 9 >f2 &&
+	printf "s\ny\nn\ny\n?\n" | git add -p --no-auto-advance >raw-stat 2>&1 &&
+	test_grep "HUNKS SUMMARY - Hunks: 3, USE: 2, SKIP: 1" raw-stat
+'
+
+test_expect_success 'selective staging across multiple files with --no-advance' '
+	git reset --hard &&
+	test_write_lines 1 2 3 4 5 6 7 8 9 >a.file &&
+	test_write_lines 1 2 3 4 5 6 7 8 9 >b.file &&
+	test_write_lines 1 2 3 4 5 6 7 8 9 >c.file &&
+	git add -A &&
+	git commit -m initial >/dev/null 2>&1 &&
+	test_write_lines 1 A2 3 4 A5 6 7 8 9 >a.file &&
+	test_write_lines 1 2 B3 4 5 6 7 B8 9 >b.file &&
+	test_write_lines C1 2 3 4 5 C6 7 8 9 >c.file &&
+	printf "s\ny\nn\n>\ns\nn\ny\n>\ns\ny\ny\nq\n" | git add -p --no-auto-advance >output.index 2>&1 &&
+	git diff --cached >staged.diff &&
+	test_grep "+A2" staged.diff &&
+	test_grep ! "+A5" staged.diff &&
+	test_grep "+B8" staged.diff &&
+	test_grep ! "+B3" staged.diff &&
+	test_grep "+C1" staged.diff &&
+	test_grep "+C6" staged.diff
+'
 
 test_done
-- 
2.39.5 (Apple Git-154)

Re: [PATCH v4 1/4] interactive -p: add new `--auto-advance` flag

[Junio C Hamano]

Abraham Samuel Adekunle <abrahamadekunle50@gmail.com> writes:

> When using the interactive add, reset, stash or checkout machinery,
> we do not have the option of reworking with a file when selecting
> hunks, because the session automatically advances to the next file
> or ends if we have just one file.
>
> Introduce the flag `--auto-advance` which auto advances by default,
> when interactively selecting patches with the '--patch' option.
> However, the `--no-auto-advance` option does not auto advance, thereby
> allowing users the option to rework with files.
>
> Signed-off-by: Abraham Samuel Adekunle <abrahamadekunle50@gmail.com>
> ---
>  add-interactive.c     | 4 ++++
>  add-interactive.h     | 5 +++--
>  builtin/add.c         | 4 ++++
>  builtin/checkout.c    | 7 +++++++
>  builtin/reset.c       | 4 ++++
>  builtin/stash.c       | 8 ++++++++
>  t/t9902-completion.sh | 1 +
>  7 files changed, 31 insertions(+), 2 deletions(-)
>
> diff --git a/add-interactive.c b/add-interactive.c
> index 95ec5a89f8..c3a36cd11f 100644
> --- a/add-interactive.c
> +++ b/add-interactive.c
> @@ -64,6 +64,7 @@ void init_add_i_state(struct add_i_state *s, struct repository *r,
>  	s->r = r;
>  	s->context = -1;
>  	s->interhunkcontext = -1;
> +	s->auto_advance = 1;
>  
>  	s->use_color_interactive = check_color_config(r, "color.interactive");
>  
> @@ -124,6 +125,8 @@ void init_add_i_state(struct add_i_state *s, struct repository *r,
>  			die(_("%s cannot be negative"), "--inter-hunk-context");
>  		s->interhunkcontext = add_p_opt->interhunkcontext;
>  	}
> +	if (!add_p_opt->auto_advance)
> +		s->auto_advance = 0;
>  }

I am confused.  Why do we need above two hunks in this function?
Wouldn't it suffice to do

	s->auto_advance = add_p_opt->auto_advance;

in the first hunk, instead of assigning 1 to it?

>  struct add_i_state {
>  	struct repository *r;
> @@ -28,7 +29,7 @@ struct add_i_state {
>  
>  	int use_single_key;
>  	char *interactive_diff_filter, *interactive_diff_algorithm;
> -	int context, interhunkcontext;
> +	int context, interhunkcontext, auto_advance;

Please don't do this.

The original is already bad to have two members on the same line,
but is tolerated as they represent somewhat related concepts.  The
auto_advance member has nothing to do with these two.

Re: [PATCH v4 2/4] add-patch: modify patch_update_file() signature

[Junio C Hamano]

Abraham Samuel Adekunle <abrahamadekunle50@gmail.com> writes:

> -static int patch_update_file(struct add_p_state *s,
> -			     struct file_diff *file_diff)
> +static ssize_t patch_update_file(struct add_p_state *s, size_t idx)

Why ssize_t?  Are we going to handle that many hunks that we do not
expect to fit in a platform natural "int" type?  If we are not doing
anything about "idx" being more than half the type, which apparently
is the case ...

>  {
>  	size_t hunk_index = 0;
>  	ssize_t i, undecided_previous, undecided_next, rendered_hunk_index = -1;
>  	struct hunk *hunk;
>  	char ch;
>  	struct child_process cp = CHILD_PROCESS_INIT;
> -	int colored = !!s->colored.len, quit = 0, use_pager = 0;
> +	int colored = !!s->colored.len, use_pager = 0;
>  	enum prompt_mode_type prompt_mode_type;
> +	struct file_diff *file_diff = s->file_diff + idx;
> +	ssize_t patch_update_resp = (ssize_t)idx;

... with the cast that is not checked here, wouldn't it make sense
to just use the platform natural "int" everywhere?  Your code is not
"safe" either way.  I do not think we expect to handle 2 billion
hunks, so even on 32-bit platforms, platform natural "int" should be
plenty.  Instead of religiously using size_t and ssize_t to count
things without extra care, I'd rather see us check the error
condition for real, if that is what we really care about (and that
can still be done while leaving the codebase cleaner by sticking to
the platform natural "int").

Enough ranting.  Anyway.

If we really are bothered that we cannot handle 3 billion hunks, we
could avoid losing half the number range by returning
s->file_diff.file_diff_nr (which is one more than there are elements
in s->file_diff[] array) or ((size_t)-1).  That would allow us to
return size_t from here.  I care about this a bit more than "why use
size_t when int is perfectly fine", because some platforms that are
not quite POSIX can have ssize_t that is not as wide as size_t.

Re: [PATCH v4 1/4] interactive -p: add new `--auto-advance` flag

[Samuel Abraham]

On Sat, Feb 14, 2026 at 12:04 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Abraham Samuel Adekunle <abrahamadekunle50@gmail.com> writes:
>
> > When using the interactive add, reset, stash or checkout machinery,
> > we do not have the option of reworking with a file when selecting
> > hunks, because the session automatically advances to the next file
> > or ends if we have just one file.
> >
> > Introduce the flag `--auto-advance` which auto advances by default,
> > when interactively selecting patches with the '--patch' option.
> > However, the `--no-auto-advance` option does not auto advance, thereby
> > allowing users the option to rework with files.
> >
> > Signed-off-by: Abraham Samuel Adekunle <abrahamadekunle50@gmail.com>
> > ---
> >  add-interactive.c     | 4 ++++
> >  add-interactive.h     | 5 +++--
> >  builtin/add.c         | 4 ++++
> >  builtin/checkout.c    | 7 +++++++
> >  builtin/reset.c       | 4 ++++
> >  builtin/stash.c       | 8 ++++++++
> >  t/t9902-completion.sh | 1 +
> >  7 files changed, 31 insertions(+), 2 deletions(-)
> >
> > diff --git a/add-interactive.c b/add-interactive.c
> > index 95ec5a89f8..c3a36cd11f 100644
> > --- a/add-interactive.c
> > +++ b/add-interactive.c
> > @@ -64,6 +64,7 @@ void init_add_i_state(struct add_i_state *s, struct repository *r,
> >       s->r = r;
> >       s->context = -1;
> >       s->interhunkcontext = -1;
> > +     s->auto_advance = 1;
> >
> >       s->use_color_interactive = check_color_config(r, "color.interactive");
> >
> > @@ -124,6 +125,8 @@ void init_add_i_state(struct add_i_state *s, struct repository *r,
> >                       die(_("%s cannot be negative"), "--inter-hunk-context");
> >               s->interhunkcontext = add_p_opt->interhunkcontext;
> >       }
> > +     if (!add_p_opt->auto_advance)
> > +             s->auto_advance = 0;
> >  }
>
> I am confused.  Why do we need above two hunks in this function?
> Wouldn't it suffice to do
>
>         s->auto_advance = add_p_opt->auto_advance;
>
> in the first hunk, instead of assigning 1 to it?

Yes thank you

>
> >  struct add_i_state {
> >       struct repository *r;
> > @@ -28,7 +29,7 @@ struct add_i_state {
> >
> >       int use_single_key;
> >       char *interactive_diff_filter, *interactive_diff_algorithm;
> > -     int context, interhunkcontext;
> > +     int context, interhunkcontext, auto_advance;
>
> Please don't do this.
>
> The original is already bad to have two members on the same line,
> but is tolerated as they represent somewhat related concepts.  The
> auto_advance member has nothing to do with these two.
>

Okay

Re: [PATCH v4 2/4] add-patch: modify patch_update_file() signature

[Samuel Abraham]

On Sat, Feb 14, 2026 at 12:34 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Abraham Samuel Adekunle <abrahamadekunle50@gmail.com> writes:
>
> > -static int patch_update_file(struct add_p_state *s,
> > -                          struct file_diff *file_diff)
> > +static ssize_t patch_update_file(struct add_p_state *s, size_t idx)
>
> Why ssize_t?  Are we going to handle that many hunks that we do not
> expect to fit in a platform natural "int" type?  If we are not doing
> anything about "idx" being more than half the type, which apparently
> is the case ...
>
> >  {
> >       size_t hunk_index = 0;
> >       ssize_t i, undecided_previous, undecided_next, rendered_hunk_index = -1;
> >       struct hunk *hunk;
> >       char ch;
> >       struct child_process cp = CHILD_PROCESS_INIT;
> > -     int colored = !!s->colored.len, quit = 0, use_pager = 0;
> > +     int colored = !!s->colored.len, use_pager = 0;
> >       enum prompt_mode_type prompt_mode_type;
> > +     struct file_diff *file_diff = s->file_diff + idx;
> > +     ssize_t patch_update_resp = (ssize_t)idx;
>
> ... with the cast that is not checked here, wouldn't it make sense
> to just use the platform natural "int" everywhere?  Your code is not
> "safe" either way.  I do not think we expect to handle 2 billion
> hunks, so even on 32-bit platforms, platform natural "int" should be
> plenty.  Instead of religiously using size_t and ssize_t to count
> things without extra care, I'd rather see us check the error
> condition for real, if that is what we really care about (and that
> can still be done while leaving the codebase cleaner by sticking to
> the platform natural "int").
>
> Enough ranting.  Anyway.
>
> If we really are bothered that we cannot handle 3 billion hunks, we
> could avoid losing half the number range by returning
> s->file_diff.file_diff_nr (which is one more than there are elements
> in s->file_diff[] array) or ((size_t)-1).  That would allow us to
> return size_t from here.  I care about this a bit more than "why use
> size_t when int is perfectly fine", because some platforms that are
> not quite POSIX can have ssize_t that is not as wide as size_t.

Hello Junio.
Thank you for the review.

I wanted to be able to return a negative value while also making sure to
return a type of the same size as "i" since that is what we use to
update the caller
for the next or previous "i".
But I know better now to think about platforms that are not quite POSIX.
I will return the s->file_diff_nr and check for that instead.

Thanks
Abraham

[PATCH v5 0/4] introduce new option `--auto-advance`

[Abraham Samuel Adekunle]

Hello,

After after more reviews and deliberations, I have been able to
rename the new option name to `--auto-advance`, where the
--no-auto-advance implements the feature and does not auto advance
while --auto-advance is the default and maintains the current
behaviour.

With the option, users can navigate in between files while deciding
on hunks as they wish with the '>' and '<' option for going to the
next and previous file respectively if there are more than one file.

Patch 1 implements the new `--no-auto-advance` options, Patch 2
modifies the function `patch_update_file()` to instead take the index
of the file as parameter instead of the file_diff. Patch 3 moves the
'git apply' logic into a function so that we can reuse this logic when
implementing the all or none application of patches.
Patch 4 implements the interfile navigation, and adds tests to the
interactive test file.

Changes in v5:
==============

- Moved 'auto_advance' member in struct add_i_state to its own line
- Removed redundant lodic to set s->auto-advance in init_add_i_state()
- Modified patch_update_file() to return size_t
- Modified the logic which checks when to quit in run_add_p() to check
  for s->file_diff_nr instead of a negative value.

Abraham Samuel Adekunle (4):
  interactive -p: add new `--auto-advance` flag
  add-patch: modify patch_update_file() signature
  add-patch: allow all-or-none application of patches
  add-patch: allow interfile navigation when selecting hunks

 add-interactive.c          |   2 +
 add-interactive.h          |   4 +-
 add-patch.c                | 154 +++++++++++++++++++++++++++----------
 builtin/add.c              |   4 +
 builtin/checkout.c         |   7 ++
 builtin/reset.c            |   4 +
 builtin/stash.c            |   8 ++
 t/t3701-add-interactive.sh | 100 ++++++++++++++++++++++++
 t/t9902-completion.sh      |   1 +
 9 files changed, 241 insertions(+), 43 deletions(-)

-- 
2.39.5 (Apple Git-154)

[PATCH v5 1/4] interactive -p: add new `--auto-advance` flag

[Abraham Samuel Adekunle]

When using the interactive add, reset, stash or checkout machinery,
we do not have the option of reworking with a file when selecting
hunks, because the session automatically advances to the next file
or ends if we have just one file.

Introduce the flag `--auto-advance` which auto advances by default,
when interactively selecting patches with the '--patch' option.
However, the `--no-auto-advance` option does not auto advance, thereby
allowing users the option to rework with files.

Signed-off-by: Abraham Samuel Adekunle <abrahamadekunle50@gmail.com>
---
 add-interactive.c     | 2 ++
 add-interactive.h     | 4 +++-
 builtin/add.c         | 4 ++++
 builtin/checkout.c    | 7 +++++++
 builtin/reset.c       | 4 ++++
 builtin/stash.c       | 8 ++++++++
 t/t9902-completion.sh | 1 +
 7 files changed, 29 insertions(+), 1 deletion(-)

diff --git a/add-interactive.c b/add-interactive.c
index 95ec5a89f8..1580639682 100644
--- a/add-interactive.c
+++ b/add-interactive.c
@@ -64,6 +64,7 @@ void init_add_i_state(struct add_i_state *s, struct repository *r,
 	s->r = r;
 	s->context = -1;
 	s->interhunkcontext = -1;
+	s->auto_advance = add_p_opt->auto_advance;
 
 	s->use_color_interactive = check_color_config(r, "color.interactive");
 
@@ -1017,6 +1018,7 @@ static int run_patch(struct add_i_state *s, const struct pathspec *ps,
 		struct add_p_opt add_p_opt = {
 			.context = s->context,
 			.interhunkcontext = s->interhunkcontext,
+			.auto_advance = s->auto_advance
 		};
 		struct strvec args = STRVEC_INIT;
 		struct pathspec ps_selected = { 0 };
diff --git a/add-interactive.h b/add-interactive.h
index da49502b76..7843397775 100644
--- a/add-interactive.h
+++ b/add-interactive.h
@@ -6,9 +6,10 @@
 struct add_p_opt {
 	int context;
 	int interhunkcontext;
+	int auto_advance;
 };
 
-#define ADD_P_OPT_INIT { .context = -1, .interhunkcontext = -1 }
+#define ADD_P_OPT_INIT { .context = -1, .interhunkcontext = -1, .auto_advance = 1 }
 
 struct add_i_state {
 	struct repository *r;
@@ -29,6 +30,7 @@ struct add_i_state {
 	int use_single_key;
 	char *interactive_diff_filter, *interactive_diff_algorithm;
 	int context, interhunkcontext;
+	int auto_advance;
 };
 
 void init_add_i_state(struct add_i_state *s, struct repository *r,
diff --git a/builtin/add.c b/builtin/add.c
index 32709794b3..4357f87b7f 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -256,6 +256,8 @@ static struct option builtin_add_options[] = {
 	OPT_GROUP(""),
 	OPT_BOOL('i', "interactive", &add_interactive, N_("interactive picking")),
 	OPT_BOOL('p', "patch", &patch_interactive, N_("select hunks interactively")),
+	OPT_BOOL(0, "auto-advance", &add_p_opt.auto_advance,
+		 N_("auto advance to the next file when selecting hunks interactively")),
 	OPT_DIFF_UNIFIED(&add_p_opt.context),
 	OPT_DIFF_INTERHUNK_CONTEXT(&add_p_opt.interhunkcontext),
 	OPT_BOOL('e', "edit", &edit_interactive, N_("edit current diff and apply")),
@@ -418,6 +420,8 @@ int cmd_add(int argc,
 			die(_("the option '%s' requires '%s'"), "--unified", "--interactive/--patch");
 		if (add_p_opt.interhunkcontext != -1)
 			die(_("the option '%s' requires '%s'"), "--inter-hunk-context", "--interactive/--patch");
+		if (!add_p_opt.auto_advance)
+			die(_("the option '%s' requires '%s'"), "--no-auto-advance", "--interactive/--patch");
 	}
 
 	if (edit_interactive) {
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 0ba4f03f2e..fad35a9284 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -63,6 +63,7 @@ struct checkout_opts {
 	int patch_mode;
 	int patch_context;
 	int patch_interhunk_context;
+	int auto_advance;
 	int quiet;
 	int merge;
 	int force;
@@ -111,6 +112,7 @@ struct checkout_opts {
 	.merge = -1, \
 	.patch_context = -1, \
 	.patch_interhunk_context = -1, \
+	.auto_advance = 1, \
 }
 
 struct branch_info {
@@ -549,6 +551,7 @@ static int checkout_paths(const struct checkout_opts *opts,
 		struct add_p_opt add_p_opt = {
 			.context = opts->patch_context,
 			.interhunkcontext = opts->patch_interhunk_context,
+			.auto_advance = opts->auto_advance
 		};
 		const char *rev = new_branch_info->name;
 		char rev_oid[GIT_MAX_HEXSZ + 1];
@@ -1803,6 +1806,8 @@ static int checkout_main(int argc, const char **argv, const char *prefix,
 			die(_("the option '%s' requires '%s'"), "--unified", "--patch");
 		if (opts->patch_interhunk_context != -1)
 			die(_("the option '%s' requires '%s'"), "--inter-hunk-context", "--patch");
+		if (!opts->auto_advance)
+			die(_("the option '%s' requires '%s'"), "--no-auto-advance", "--patch");
 	}
 
 	if (opts->show_progress < 0) {
@@ -2001,6 +2006,8 @@ int cmd_checkout(int argc,
 		OPT_BOOL(0, "guess", &opts.dwim_new_local_branch,
 			 N_("second guess 'git checkout <no-such-branch>' (default)")),
 		OPT_BOOL(0, "overlay", &opts.overlay_mode, N_("use overlay mode (default)")),
+		OPT_BOOL(0, "auto-advance", &opts.auto_advance,
+			 N_("auto advance to the next file when selecting hunks interactively")),
 		OPT_END()
 	};
 
diff --git a/builtin/reset.c b/builtin/reset.c
index c48d9845f8..88f95f9fc7 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -371,6 +371,8 @@ int cmd_reset(int argc,
 			       PARSE_OPT_OPTARG,
 			       option_parse_recurse_submodules_worktree_updater),
 		OPT_BOOL('p', "patch", &patch_mode, N_("select hunks interactively")),
+		OPT_BOOL(0, "auto-advance", &add_p_opt.auto_advance,
+			 N_("auto advance to the next file when selecting hunks interactively")),
 		OPT_DIFF_UNIFIED(&add_p_opt.context),
 		OPT_DIFF_INTERHUNK_CONTEXT(&add_p_opt.interhunkcontext),
 		OPT_BOOL('N', "intent-to-add", &intent_to_add,
@@ -443,6 +445,8 @@ int cmd_reset(int argc,
 			die(_("the option '%s' requires '%s'"), "--unified", "--patch");
 		if (add_p_opt.interhunkcontext != -1)
 			die(_("the option '%s' requires '%s'"), "--inter-hunk-context", "--patch");
+		if (!add_p_opt.auto_advance)
+			die(_("the option '%s' requires '%s'"), "--no-auto-advance", "--patch");
 	}
 
 	/* git reset tree [--] paths... can be used to
diff --git a/builtin/stash.c b/builtin/stash.c
index 193e3ea47a..f98487f4cd 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -1849,6 +1849,8 @@ static int push_stash(int argc, const char **argv, const char *prefix,
 			 N_("stash staged changes only")),
 		OPT_BOOL('p', "patch", &patch_mode,
 			 N_("stash in patch mode")),
+		OPT_BOOL(0, "auto-advance", &add_p_opt.auto_advance,
+			 N_("auto advance to the next file when selecting hunks interactively")),
 		OPT_DIFF_UNIFIED(&add_p_opt.context),
 		OPT_DIFF_INTERHUNK_CONTEXT(&add_p_opt.interhunkcontext),
 		OPT__QUIET(&quiet, N_("quiet mode")),
@@ -1911,6 +1913,8 @@ static int push_stash(int argc, const char **argv, const char *prefix,
 			die(_("the option '%s' requires '%s'"), "--unified", "--patch");
 		if (add_p_opt.interhunkcontext != -1)
 			die(_("the option '%s' requires '%s'"), "--inter-hunk-context", "--patch");
+		if (!add_p_opt.auto_advance)
+			die(_("the option '%s' requires '%s'"), "--no-auto-advance", "--patch");
 	}
 
 	if (add_p_opt.context < -1)
@@ -1952,6 +1956,8 @@ static int save_stash(int argc, const char **argv, const char *prefix,
 			 N_("stash staged changes only")),
 		OPT_BOOL('p', "patch", &patch_mode,
 			 N_("stash in patch mode")),
+		OPT_BOOL(0, "auto-advance", &add_p_opt.auto_advance,
+			 N_("auto advance to the next file when selecting hunks interactively")),
 		OPT_DIFF_UNIFIED(&add_p_opt.context),
 		OPT_DIFF_INTERHUNK_CONTEXT(&add_p_opt.interhunkcontext),
 		OPT__QUIET(&quiet, N_("quiet mode")),
@@ -1983,6 +1989,8 @@ static int save_stash(int argc, const char **argv, const char *prefix,
 			die(_("the option '%s' requires '%s'"), "--unified", "--patch");
 		if (add_p_opt.interhunkcontext != -1)
 			die(_("the option '%s' requires '%s'"), "--inter-hunk-context", "--patch");
+		if (!add_p_opt.auto_advance)
+			die(_("the option '%s' requires '%s'"), "--no-auto-advance", "--patch");
 	}
 
 	ret = do_push_stash(&ps, stash_msg, quiet, keep_index,
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index ffb9c8b522..2f9a597ec7 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -2601,6 +2601,7 @@ test_expect_success 'double dash "git checkout"' '
 	--ignore-skip-worktree-bits Z
 	--ignore-other-worktrees Z
 	--recurse-submodules Z
+	--auto-advance Z
 	--progress Z
 	--guess Z
 	--no-guess Z
-- 
2.39.5 (Apple Git-154)

[PATCH v5 2/4] add-patch: modify patch_update_file() signature

[Abraham Samuel Adekunle]

The function `patch_update_file()` takes the add_p_state struct
pointer and the current `struct file_diff` pointer and returns an
int.

When using the `--no-auto-advance` flag, we want to be able to request
the next or previous file from the caller.

Modify the function signature to instead take the index of the
current `file_diff` and the `add_p_state` struct pointer so that we
can compute the `file_diff` from the index while also having
access to the file index. This will help us request the next or
previous file from the caller.

Signed-off-by: Abraham Samuel Adekunle <abrahamadekunle50@gmail.com>
---
 add-patch.c | 32 +++++++++++++++++++-------------
 1 file changed, 19 insertions(+), 13 deletions(-)

diff --git a/add-patch.c b/add-patch.c
index df8f2e6d74..8e21ea1246 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -1441,20 +1441,21 @@ static bool get_first_undecided(const struct file_diff *file_diff, size_t *idx)
 	return false;
 }
 
-static int patch_update_file(struct add_p_state *s,
-			     struct file_diff *file_diff)
+static size_t patch_update_file(struct add_p_state *s, size_t idx)
 {
 	size_t hunk_index = 0;
 	ssize_t i, undecided_previous, undecided_next, rendered_hunk_index = -1;
 	struct hunk *hunk;
 	char ch;
 	struct child_process cp = CHILD_PROCESS_INIT;
-	int colored = !!s->colored.len, quit = 0, use_pager = 0;
+	int colored = !!s->colored.len, use_pager = 0;
 	enum prompt_mode_type prompt_mode_type;
+	struct file_diff *file_diff = s->file_diff + idx;
+	size_t patch_update_resp = idx;
 
 	/* Empty added files have no hunks */
 	if (!file_diff->hunk_nr && !file_diff->added)
-		return 0;
+		return patch_update_resp + 1;
 
 	strbuf_reset(&s->buf);
 	render_diff_header(s, file_diff, colored, &s->buf);
@@ -1499,9 +1500,10 @@ static int patch_update_file(struct add_p_state *s,
 
 		/* Everything decided? */
 		if (undecided_previous < 0 && undecided_next < 0 &&
-		    hunk->use != UNDECIDED_HUNK)
-			break;
-
+		    hunk->use != UNDECIDED_HUNK) {
+				patch_update_resp++;
+				break;
+		}
 		strbuf_reset(&s->buf);
 		if (file_diff->hunk_nr) {
 			if (rendered_hunk_index != hunk_index) {
@@ -1577,7 +1579,7 @@ static int patch_update_file(struct add_p_state *s,
 			fputs(s->s.reset_color_interactive, stdout);
 		fflush(stdout);
 		if (read_single_character(s) == EOF) {
-			quit = 1;
+			patch_update_resp = s->file_diff_nr;
 			break;
 		}
 
@@ -1623,7 +1625,7 @@ static int patch_update_file(struct add_p_state *s,
 				hunk->use = SKIP_HUNK;
 			}
 		} else if (ch == 'q') {
-			quit = 1;
+			patch_update_resp = s->file_diff_nr;
 			break;
 		} else if (s->answer.buf[0] == 'K') {
 			if (permitted & ALLOW_GOTO_PREVIOUS_HUNK)
@@ -1810,7 +1812,7 @@ static int patch_update_file(struct add_p_state *s,
 	}
 
 	putchar('\n');
-	return quit;
+	return patch_update_resp;
 }
 
 int run_add_p(struct repository *r, enum add_p_mode mode,
@@ -1859,11 +1861,15 @@ int run_add_p(struct repository *r, enum add_p_mode mode,
 		return -1;
 	}
 
-	for (i = 0; i < s.file_diff_nr; i++)
-		if (s.file_diff[i].binary && !s.file_diff[i].hunk_nr)
+	for (i = 0; i < s.file_diff_nr;) {
+		if (s.file_diff[i].binary && !s.file_diff[i].hunk_nr) {
 			binary_count++;
-		else if (patch_update_file(&s, s.file_diff + i))
+			i++;
+			continue;
+		}
+		 if ((i = patch_update_file(&s, i)) == s.file_diff_nr)
 			break;
+    }
 
 	if (s.file_diff_nr == 0)
 		err(&s, _("No changes."));
-- 
2.39.5 (Apple Git-154)

[PATCH v5 3/4] add-patch: allow all-or-none application of patches

[Abraham Samuel Adekunle]

When the flag `--no-auto-advance` is used with `--patch`,
if the user has decided `USE` on a hunk in a file, goes to another
file, and then returns to this file and changes the previous
decision on the hunk to `SKIP`, because the patch has already
been applied, the last decision is not registered and the now
SKIPPED hunk is still applied.

Move the logic for applying patches into a function so that we can
reuse this logic to implement the all or non application of the patches
after the user is done with the hunk selection.

Signed-off-by: Abraham Samuel Adekunle <abrahamadekunle50@gmail.com>
---
 add-patch.c | 62 ++++++++++++++++++++++++++++++-----------------------
 1 file changed, 35 insertions(+), 27 deletions(-)

diff --git a/add-patch.c b/add-patch.c
index 8e21ea1246..07526e7fb6 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -1420,6 +1420,40 @@ N_("j - go to the next undecided hunk, roll over at the bottom\n"
    "P - print the current hunk using the pager\n"
    "? - print help\n");
 
+static void apply_patch(struct add_p_state *s, struct file_diff *file_diff)
+{
+	struct child_process cp = CHILD_PROCESS_INIT;
+	size_t j;
+
+	/* Any hunk to be used? */
+	for (j = 0; j < file_diff->hunk_nr; j++)
+		if (file_diff->hunk[j].use == USE_HUNK)
+			break;
+
+	if (j < file_diff->hunk_nr ||
+		(!file_diff->hunk_nr && file_diff->head.use == USE_HUNK)) {
+		/* At least one hunk selected: apply */
+		strbuf_reset(&s->buf);
+		reassemble_patch(s, file_diff, 0, &s->buf);
+
+		discard_index(s->s.r->index);
+		if (s->mode->apply_for_checkout)
+			apply_for_checkout(s, &s->buf,
+					s->mode->is_reverse);
+		else {
+			setup_child_process(s, &cp, "apply", NULL);
+			strvec_pushv(&cp.args, s->mode->apply_args);
+			if (pipe_command(&cp, s->buf.buf, s->buf.len,
+					NULL, 0, NULL, 0))
+				error(_("'git apply' failed"));
+		}
+		if (repo_read_index(s->s.r) >= 0)
+			repo_refresh_and_write_index(s->s.r, REFRESH_QUIET, 0,
+							1, NULL, NULL, NULL);
+	}
+
+}
+
 static size_t dec_mod(size_t a, size_t m)
 {
 	return a > 0 ? a - 1 : m - 1;
@@ -1447,7 +1481,6 @@ static size_t patch_update_file(struct add_p_state *s, size_t idx)
 	ssize_t i, undecided_previous, undecided_next, rendered_hunk_index = -1;
 	struct hunk *hunk;
 	char ch;
-	struct child_process cp = CHILD_PROCESS_INIT;
 	int colored = !!s->colored.len, use_pager = 0;
 	enum prompt_mode_type prompt_mode_type;
 	struct file_diff *file_diff = s->file_diff + idx;
@@ -1784,32 +1817,7 @@ static size_t patch_update_file(struct add_p_state *s, size_t idx)
 		}
 	}
 
-	/* Any hunk to be used? */
-	for (i = 0; i < file_diff->hunk_nr; i++)
-		if (file_diff->hunk[i].use == USE_HUNK)
-			break;
-
-	if (i < file_diff->hunk_nr ||
-	    (!file_diff->hunk_nr && file_diff->head.use == USE_HUNK)) {
-		/* At least one hunk selected: apply */
-		strbuf_reset(&s->buf);
-		reassemble_patch(s, file_diff, 0, &s->buf);
-
-		discard_index(s->s.r->index);
-		if (s->mode->apply_for_checkout)
-			apply_for_checkout(s, &s->buf,
-					   s->mode->is_reverse);
-		else {
-			setup_child_process(s, &cp, "apply", NULL);
-			strvec_pushv(&cp.args, s->mode->apply_args);
-			if (pipe_command(&cp, s->buf.buf, s->buf.len,
-					 NULL, 0, NULL, 0))
-				error(_("'git apply' failed"));
-		}
-		if (repo_read_index(s->s.r) >= 0)
-			repo_refresh_and_write_index(s->s.r, REFRESH_QUIET, 0,
-						     1, NULL, NULL, NULL);
-	}
+	apply_patch(s, file_diff);
 
 	putchar('\n');
 	return patch_update_resp;
-- 
2.39.5 (Apple Git-154)

[PATCH v5 4/4] add-patch: allow interfile navigation when selecting hunks

[Abraham Samuel Adekunle]

After deciding on all hunks in a file, the interactive session
advances automatically to the next file if there is another,
or the process ends.

Now using the `--no-auto-advance` flag with `--patch`, the process
does not advance automatically. A user can choose to go to the next
file by pressing '>' or the previous file by pressing '<', before or
after deciding on all hunks in the current file.

After all hunks have been decided in a file, the user can still
rework with the file by applying the options available in the permit
set for that hunk, and after all the decisions, the user presses 'q'
to submit.
After all hunks have been decided, the user can press '?' which will
show the hunk selection summary in the help patch remainder text
including the total hunks, number of hunks marked for use and number
of hunks marked for skip.

This feature is enabled by passing the `--no-auto-advance` flag
to `--patch` option of the subcommands add, stash, reset,
and checkout.

Signed-off-by: Abraham Samuel Adekunle <abrahamadekunle50@gmail.com>
---
 add-patch.c                |  66 ++++++++++++++++++++++--
 t/t3701-add-interactive.sh | 100 +++++++++++++++++++++++++++++++++++++
 2 files changed, 161 insertions(+), 5 deletions(-)

diff --git a/add-patch.c b/add-patch.c
index 07526e7fb6..b3fb08f416 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -1418,7 +1418,10 @@ N_("j - go to the next undecided hunk, roll over at the bottom\n"
    "e - manually edit the current hunk\n"
    "p - print the current hunk\n"
    "P - print the current hunk using the pager\n"
-   "? - print help\n");
+   "> - go to the next file, roll over at the bottom\n"
+   "< - go to the previous file, roll over at the top\n"
+   "? - print help\n"
+   "HUNKS SUMMARY - Hunks: %d, USE: %d, SKIP: %d\n");
 
 static void apply_patch(struct add_p_state *s, struct file_diff *file_diff)
 {
@@ -1483,6 +1486,7 @@ static size_t patch_update_file(struct add_p_state *s, size_t idx)
 	char ch;
 	int colored = !!s->colored.len, use_pager = 0;
 	enum prompt_mode_type prompt_mode_type;
+	int all_decided = 0;
 	struct file_diff *file_diff = s->file_diff + idx;
 	size_t patch_update_resp = idx;
 
@@ -1502,7 +1506,9 @@ static size_t patch_update_file(struct add_p_state *s, size_t idx)
 			ALLOW_GOTO_NEXT_UNDECIDED_HUNK = 1 << 3,
 			ALLOW_SEARCH_AND_GOTO = 1 << 4,
 			ALLOW_SPLIT = 1 << 5,
-			ALLOW_EDIT = 1 << 6
+			ALLOW_EDIT = 1 << 6,
+			ALLOW_GOTO_PREVIOUS_FILE = 1 << 7,
+			ALLOW_GOTO_NEXT_FILE = 1 << 8
 		} permitted = 0;
 
 		if (hunk_index >= file_diff->hunk_nr)
@@ -1534,8 +1540,12 @@ static size_t patch_update_file(struct add_p_state *s, size_t idx)
 		/* Everything decided? */
 		if (undecided_previous < 0 && undecided_next < 0 &&
 		    hunk->use != UNDECIDED_HUNK) {
-				patch_update_resp++;
-				break;
+				if (!s->s.auto_advance)
+					all_decided = 1;
+				else {
+					patch_update_resp++;
+					break;
+				}
 		}
 		strbuf_reset(&s->buf);
 		if (file_diff->hunk_nr) {
@@ -1584,6 +1594,14 @@ static size_t patch_update_file(struct add_p_state *s, size_t idx)
 				permitted |= ALLOW_EDIT;
 				strbuf_addstr(&s->buf, ",e");
 			}
+			if (!s->s.auto_advance && s->file_diff_nr > 1) {
+				permitted |= ALLOW_GOTO_NEXT_FILE;
+				strbuf_addstr(&s->buf, ",>");
+			}
+			if (!s->s.auto_advance && s->file_diff_nr > 1) {
+				permitted |= ALLOW_GOTO_PREVIOUS_FILE;
+				strbuf_addstr(&s->buf, ",<");
+			}
 			strbuf_addstr(&s->buf, ",p,P");
 		}
 		if (file_diff->deleted)
@@ -1660,6 +1678,28 @@ static size_t patch_update_file(struct add_p_state *s, size_t idx)
 		} else if (ch == 'q') {
 			patch_update_resp = s->file_diff_nr;
 			break;
+		} else if (!s->s.auto_advance && s->answer.buf[0] == '>') {
+			if (permitted & ALLOW_GOTO_NEXT_FILE) {
+				if (patch_update_resp == s->file_diff_nr - 1)
+					patch_update_resp = 0;
+				else
+					patch_update_resp++;
+				break;
+			} else {
+				err(s, _("No next file"));
+				continue;
+			}
+		} else if (!s->s.auto_advance && s->answer.buf[0] == '<') {
+			if (permitted & ALLOW_GOTO_PREVIOUS_FILE) {
+				if (patch_update_resp == 0)
+					patch_update_resp = s->file_diff_nr - 1;
+				else
+					patch_update_resp--;
+				break;
+			} else {
+				err(s, _("No previous file"));
+				continue;
+			}
 		} else if (s->answer.buf[0] == 'K') {
 			if (permitted & ALLOW_GOTO_PREVIOUS_HUNK)
 				hunk_index = dec_mod(hunk_index,
@@ -1805,6 +1845,18 @@ static size_t patch_update_file(struct add_p_state *s, size_t idx)
 				 * commands shown in the prompt that are not
 				 * always available.
 				 */
+				if (all_decided && !strncmp(p, "HUNKS SUMMARY", 13)) {
+					int total = file_diff->hunk_nr, used = 0, skipped = 0;
+
+					for (i = 0; i < file_diff->hunk_nr; i++) {
+						if (file_diff->hunk[i].use == USE_HUNK)
+							used += 1;
+						if (file_diff->hunk[i].use == SKIP_HUNK)
+							skipped += 1;
+					}
+					color_fprintf_ln(stdout, s->s.help_color, _(p),
+							 total, used, skipped);
+				}
 				if (*p != '?' && !strchr(s->buf.buf, *p))
 					continue;
 
@@ -1817,7 +1869,8 @@ static size_t patch_update_file(struct add_p_state *s, size_t idx)
 		}
 	}
 
-	apply_patch(s, file_diff);
+	if (s->s.auto_advance)
+		apply_patch(s, file_diff);
 
 	putchar('\n');
 	return patch_update_resp;
@@ -1878,6 +1931,9 @@ int run_add_p(struct repository *r, enum add_p_mode mode,
 		 if ((i = patch_update_file(&s, i)) == s.file_diff_nr)
 			break;
     }
+	if (!s.s.auto_advance)
+		for (i = 0; i < s.file_diff_nr; i++)
+			apply_patch(&s, s.file_diff + i);
 
 	if (s.file_diff_nr == 0)
 		err(&s, _("No changes."));
diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index 5ce9c6dd60..6e120a4001 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -1441,5 +1441,105 @@ test_expect_success 'EOF quits' '
 	test_grep file out &&
 	test_grep ! file2 out
 '
+for cmd in add checkout reset "stash save" "stash push"
+do
+	test_expect_success "$cmd rejects invalid --no-auto-advance options" '
+		test_must_fail git $cmd --no-auto-advance 2>actual &&
+		test_grep -E  "requires .*--(interactive|patch)" actual
+	'
+done
+
+test_expect_success 'manual advance (">") moves to next file with --no-auto-advance' '
+	git reset --hard &&
+	echo line1 >first-file &&
+	echo line2 >second-file &&
+	git add -A &&
+	git commit -m initial >/dev/null 2>&1 &&
+	echo change_first >>first-file &&
+	echo change_second >>second-file &&
+
+	printf ">\nq\n" | git add -p --no-auto-advance >output.test 2>&1 &&
+	test_grep  -E "(a|b)/second-file" output.test
+'
+
+test_expect_success 'select n on a hunk, go to another file, come back and change to y stages' '
+	git reset --hard &&
+	echo one >f1 &&
+	echo one >f2 &&
+	git add -A &&
+	git commit -m initial >/dev/null 2>&1 &&
+	echo change1 >>f1 &&
+	echo change2 >>f2 &&
+
+	printf "n\n>\n<\ny\nq\n" | git add -p --no-auto-advance >output.staged 2>&1 &&
+	git diff --cached --name-only >staged &&
+	test_grep -E "(a/f1)" output.staged
+'
+
+test_expect_success 'select y on a hunk, go to another file, come back and change to n does not stage' '
+	git reset --hard &&
+	echo one >f1 &&
+	echo one >f2 &&
+	git add -A &&
+	git commit -m initial >/dev/null 2>&1 &&
+	echo change1 >>f1 &&
+	echo change2 >>f2 &&
+
+	printf "y\n>\n<\nn\nq\n" | git add -p --no-auto-advance >output.unstaged 2>&1 &&
+	git diff --cached --name-only >staged &&
+	test_must_be_empty staged
+'
+
+test_expect_success 'deciding all hunks in a file does not auto advance' '
+	git reset --hard &&
+	echo line >stay &&
+	echo line >other &&
+	git add -A &&
+	git commit -m initial >/dev/null 2>&1 &&
+	echo change >>stay &&
+	echo change >>other &&
+	test_write_lines y | git add -p --no-auto-advance >raw-output 2>&1 &&
+	test_grep "(1/1) Stage this hunk (was: y)" raw-output &&
+	test_grep ! "diff --git a/stay b/stay" raw-output
+'
+test_expect_success 'HUNKS SUMMARY does not show in help text when there are undecided hunks' '
+	git reset --hard &&
+	test_write_lines 1 2 3 4 5 6 7 8 9 >f &&
+	git add f &&
+	git commit -m initial >/dev/null 2>&1 &&
+	test_write_lines 1 X 3 4 Y 6 7 Z 9 >f &&
+	test_write_lines s y n | git add -p --no-auto-advance >raw-nostat 2>&1 &&
+	test_grep ! "HUNKS SUMMARY - Hunks: " raw-nostat
+'
+
+test_expect_success 'help text shows HUNK SUMMARY when all hunks have been decided' '
+	git reset --hard &&
+	test_write_lines 1 2 3 4 5 6 7 8 9 >f2 &&
+	git add f2 &&
+	git commit -m initial >/dev/null 2>&1 &&
+	test_write_lines 1 X 3 4 Y 6 7 Z 9 >f2 &&
+	printf "s\ny\nn\ny\n?\n" | git add -p --no-auto-advance >raw-stat 2>&1 &&
+	test_grep "HUNKS SUMMARY - Hunks: 3, USE: 2, SKIP: 1" raw-stat
+'
+
+test_expect_success 'selective staging across multiple files with --no-advance' '
+	git reset --hard &&
+	test_write_lines 1 2 3 4 5 6 7 8 9 >a.file &&
+	test_write_lines 1 2 3 4 5 6 7 8 9 >b.file &&
+	test_write_lines 1 2 3 4 5 6 7 8 9 >c.file &&
+	git add -A &&
+	git commit -m initial >/dev/null 2>&1 &&
+	test_write_lines 1 A2 3 4 A5 6 7 8 9 >a.file &&
+	test_write_lines 1 2 B3 4 5 6 7 B8 9 >b.file &&
+	test_write_lines C1 2 3 4 5 C6 7 8 9 >c.file &&
+	printf "s\ny\nn\n>\ns\nn\ny\n>\ns\ny\ny\nq\n" | git add -p --no-auto-advance >output.index 2>&1 &&
+	git diff --cached >staged.diff &&
+	test_grep "+A2" staged.diff &&
+	test_grep ! "+A5" staged.diff &&
+	test_grep "+B8" staged.diff &&
+	test_grep ! "+B3" staged.diff &&
+	test_grep "+C1" staged.diff &&
+	test_grep "+C6" staged.diff
+'
 
 test_done
-- 
2.39.5 (Apple Git-154)

Re: [PATCH v5 0/4] introduce new option `--auto-advance`

[Junio C Hamano]

Abraham Samuel Adekunle <abrahamadekunle50@gmail.com> writes:

> After after more reviews and deliberations, I have been able to
> rename the new option name to `--auto-advance`, where the
> --no-auto-advance implements the feature and does not auto advance
> while --auto-advance is the default and maintains the current
> behaviour.

Haven't seen any reviews on this latest round, and I think what we
have here may be good enough.  Shall we merge it down to 'next'?
Any comments?

Thanks.

Re: [PATCH v5 0/4] introduce new option `--auto-advance`

[Samuel Abraham]

On Fri, Feb 20, 2026 at 11:32 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Abraham Samuel Adekunle <abrahamadekunle50@gmail.com> writes:
>
> > After after more reviews and deliberations, I have been able to
> > rename the new option name to `--auto-advance`, where the
> > --no-auto-advance implements the feature and does not auto advance
> > while --auto-advance is the default and maintains the current
> > behaviour.
>
> Haven't seen any reviews on this latest round, and I think what we
> have here may be good enough.  Shall we merge it down to 'next'?
> Any comments?
>
Okay.
Thank you Junio, Phillip and everyone for the patience and guidance
in completing this patch series and also the one for my GSoC microproject.
It has been a great learning process.
I appreciate it.
Thanks

Abraham