Broken handling of "J" hunks for "add --interactive"?

Broken handling of "J" hunks for "add --interactive"?

[Windl, Ulrich]

Thank you for filling out a Git bug report!
Please answer the following questions to help us understand your issue.

What did you do before the bug happened? (Steps to reproduce your issue)
git add --interactive

answer some "y", some "n", one "J"

What did you expect to happen? (Expected behavior)
git will ask at end for exactly the one "J" hunk

What happened instead? (Actual behavior)
git asked about the hunk rejected before the "J" hunk also
(asked for two hunks instead of one)

What's different between what you expected and what actually happened?
I did not observer that in an older version of git (like 2.26.2)

Anything else you want to add:

Please review the rest of the bug report below.
You can delete any lines you don't wish to share.


[System Info]
git version:
git version 2.51.0
cpu: x86_64
no commit associated with this build
sizeof-long: 8
sizeof-size_t: 8
shell-path: /bin/sh
libcurl: 8.6.0
OpenSSL: OpenSSL 3.1.4 24 Oct 2023
zlib: 1.2.13
SHA-1: SHA1_DC
SHA-256: SHA256_BLK
default-ref-format: files
default-hash: sha1
uname: Linux 6.4.0-150600.23.65-default #1 SMP PREEMPT_DYNAMIC Tue Aug 12 00:37:41 UTC 2025 (aedcb04) x86_64
compiler info: gnuc: 7.5
libc info: glibc: 2.38
$SHELL (typically, interactive shell): /bin/bash


[Enabled Hooks]

[PATCH] add-patch: roll over to next undecided hunk

[René Scharfe]

git add --patch presents diff hunks one after the other, asking whether
to add them.  If we mark some as undecided, e.g. with J, then it will
start over after reaching the last hunk.  It always starts over at the
very first hunk, though, even if we already decided on it.  Skip
decided hunks when rolling over instead.

Reported-by: Windl, Ulrich <u.windl@ukr.de>
Signed-off-by: René Scharfe <l.s.r@web.de>
---
 add-patch.c                |  9 ++++++++-
 t/t3701-add-interactive.sh | 20 ++++++++++++++++++++
 2 files changed, 28 insertions(+), 1 deletion(-)

diff --git a/add-patch.c b/add-patch.c
index b0389c5d5b..42a8394c92 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -1436,8 +1436,15 @@ static int patch_update_file(struct add_p_state *s,
 	render_diff_header(s, file_diff, colored, &s->buf);
 	fputs(s->buf.buf, stdout);
 	for (;;) {
-		if (hunk_index >= file_diff->hunk_nr)
+		if (hunk_index >= file_diff->hunk_nr) {
 			hunk_index = 0;
+			for (i = 0; i < file_diff->hunk_nr; i++) {
+				if (file_diff->hunk[i].use == UNDECIDED_HUNK) {
+					hunk_index = i;
+					break;
+				}
+			}
+		}
 		hunk = file_diff->hunk_nr
 				? file_diff->hunk + hunk_index
 				: &file_diff->head;
diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index d9fe289a7a..fa6ec5f835 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -1321,6 +1321,26 @@ test_expect_success 'stash accepts -U and --inter-hunk-context' '
 	test_grep "@@ -2,20 +2,20 @@" actual
 '
 
+test_expect_success 'roll over to next undecided (1)' '
+	test_write_lines a b c d e f g h i j k l m n o p q >file &&
+	git add file &&
+	test_write_lines X b c d e f g h X j k l m n o p X >file &&
+	test_write_lines J y y q | git add -p >actual &&
+	test_write_lines 1 2 3 1 >expect &&
+	sed -ne "s-/.*--" -e "s-^(--p" <actual >hunks &&
+	test_cmp expect hunks
+'
+
+test_expect_success 'roll over to next undecided (2)' '
+	test_write_lines a b c d e f g h i j k l m n o p q >file &&
+	git add file &&
+	test_write_lines X b c d e f g h X j k l m n o p X >file &&
+	test_write_lines y J y q | git add -p >actual &&
+	test_write_lines 1 2 3 2 >expect &&
+	sed -ne "s-/.*--" -e "s-^(--p" <actual >hunks &&
+	test_cmp expect hunks
+'
+
 test_expect_success 'set up base for -p color tests' '
 	echo commit >file &&
 	git commit -am "commit state" &&
-- 
2.51.0

Re: [PATCH] add-patch: roll over to next undecided hunk

[Phillip Wood]

Hi René

On 03/10/2025 13:16, René Scharfe wrote:
> git add --patch presents diff hunks one after the other, asking whether
> to add them.  If we mark some as undecided, e.g. with J, then it will
> start over after reaching the last hunk.  It always starts over at the
> very first hunk, though, even if we already decided on it.  Skip
> decided hunks when rolling over instead.

Nice

> @@ -1436,8 +1436,15 @@ static int patch_update_file(struct add_p_state *s,
>   	render_diff_header(s, file_diff, colored, &s->buf);
>   	fputs(s->buf.buf, stdout);
>   	for (;;) {
> -		if (hunk_index >= file_diff->hunk_nr)
> +		if (hunk_index >= file_diff->hunk_nr) {
>   			hunk_index = 0;
> +			for (i = 0; i < file_diff->hunk_nr; i++) {
> +				if (file_diff->hunk[i].use == UNDECIDED_HUNK) {
> +					hunk_index = i;
> +					break;
> +				}
> +			}
> +		}
>   		hunk = file_diff->hunk_nr
>   				? file_diff->hunk + hunk_index

If there were no undecided hunks then this will be out of bounds because 
hunk_index >= file_diff->hunk_nr. Are we absolutely certain that we 
cannot reach this point without at least one hunk being undecided?

> +test_expect_success 'roll over to next undecided (1)' '
> +	test_write_lines a b c d e f g h i j k l m n o p q >file &&
> +	git add file &&
> +	test_write_lines X b c d e f g h X j k l m n o p X >file &&
> +	test_write_lines J y y q | git add -p >actual &&
> +	test_write_lines 1 2 3 1 >expect &&
> +	sed -ne "s-/.*--" -e "s-^(--p" <actual >hunks &&
> +	test_cmp expect hunks
> +'

I'm not sure what this first test adds, the one below checks that we 
find the first undecided hunk which seems to be the important thing to 
check.

Thanks

Phillip

> +test_expect_success 'roll over to next undecided (2)' '
> +	test_write_lines a b c d e f g h i j k l m n o p q >file &&
> +	git add file &&
> +	test_write_lines X b c d e f g h X j k l m n o p X >file &&
> +	test_write_lines y J y q | git add -p >actual &&
> +	test_write_lines 1 2 3 2 >expect &&
> +	sed -ne "s-/.*--" -e "s-^(--p" <actual >hunks &&
> +	test_cmp expect hunks
> +'
> +
>   test_expect_success 'set up base for -p color tests' '
>   	echo commit >file &&
>   	git commit -am "commit state" &&

Re: [PATCH] add-patch: roll over to next undecided hunk

[René Scharfe]

On 10/3/25 3:41 PM, Phillip Wood wrote:
> 
>> @@ -1436,8 +1436,15 @@ static int patch_update_file(struct add_p_state *s,
>>       render_diff_header(s, file_diff, colored, &s->buf);
>>       fputs(s->buf.buf, stdout);
>>       for (;;) {
>> -        if (hunk_index >= file_diff->hunk_nr)
>> +        if (hunk_index >= file_diff->hunk_nr) {
>>               hunk_index = 0;
>> +            for (i = 0; i < file_diff->hunk_nr; i++) {
>> +                if (file_diff->hunk[i].use == UNDECIDED_HUNK) {
>> +                    hunk_index = i;
>> +                    break;
>> +                }
>> +            }
>> +        }
>>           hunk = file_diff->hunk_nr
>>                   ? file_diff->hunk + hunk_index
> 
> If there were no undecided hunks then this will be out of bounds
> because hunk_index >= file_diff->hunk_nr. Are we absolutely certain
> that we cannot reach this point without at least one hunk being
> undecided?

The new loop only sets hunk_index if i < file_diff->hunk_nr.  If
it finds no undecided hunk then it does nothing.

>> +test_expect_success 'roll over to next undecided (1)' '
>> +    test_write_lines a b c d e f g h i j k l m n o p q >file &&
>> +    git add file &&
>> +    test_write_lines X b c d e f g h X j k l m n o p X >file &&
>> +    test_write_lines J y y q | git add -p >actual &&
>> +    test_write_lines 1 2 3 1 >expect &&
>> +    sed -ne "s-/.*--" -e "s-^(--p" <actual >hunks &&
>> +    test_cmp expect hunks
>> +'
> 
> I'm not sure what this first test adds, the one below checks that we
> find the first undecided hunk which seems to be the important thing
> to check.

It's a regression test for the case that the original code got
right by accident.  It may seem superfluous, but I actually
triggered it in my first attempt at a fix.

René

Re: [PATCH] add-patch: roll over to next undecided hunk

[Phillip Wood]

On 03/10/2025 15:10, René Scharfe wrote:
> On 10/3/25 3:41 PM, Phillip Wood wrote:
>>
>>> @@ -1436,8 +1436,15 @@ static int patch_update_file(struct add_p_state *s,
>>>        render_diff_header(s, file_diff, colored, &s->buf);
>>>        fputs(s->buf.buf, stdout);
>>>        for (;;) {
>>> -        if (hunk_index >= file_diff->hunk_nr)
>>> +        if (hunk_index >= file_diff->hunk_nr) {
>>>                hunk_index = 0;
>>> +            for (i = 0; i < file_diff->hunk_nr; i++) {
>>> +                if (file_diff->hunk[i].use == UNDECIDED_HUNK) {
>>> +                    hunk_index = i;
>>> +                    break;
>>> +                }
>>> +            }
>>> +        }
>>>            hunk = file_diff->hunk_nr
>>>                    ? file_diff->hunk + hunk_index
>>
>> If there were no undecided hunks then this will be out of bounds
>> because hunk_index >= file_diff->hunk_nr. Are we absolutely certain
>> that we cannot reach this point without at least one hunk being
>> undecided?
> 
> The new loop only sets hunk_index if i < file_diff->hunk_nr.  If
> it finds no undecided hunk then it does nothing.

Exactly - that's what I was worried about. However I'd missed the fact 
that we still set hunk_index to zero before the loop so I thought it was 
unchanged from file_diff->hunk_nr when in fact it is unchanged from zero 
which is safe.

>>> +test_expect_success 'roll over to next undecided (1)' '
>>> +    test_write_lines a b c d e f g h i j k l m n o p q >file &&
>>> +    git add file &&
>>> +    test_write_lines X b c d e f g h X j k l m n o p X >file &&
>>> +    test_write_lines J y y q | git add -p >actual &&
>>> +    test_write_lines 1 2 3 1 >expect &&
>>> +    sed -ne "s-/.*--" -e "s-^(--p" <actual >hunks &&
>>> +    test_cmp expect hunks
>>> +'
>>
>> I'm not sure what this first test adds, the one below checks that we
>> find the first undecided hunk which seems to be the important thing
>> to check.
> 
> It's a regression test for the case that the original code got
> right by accident.  It may seem superfluous, but I actually
> triggered it in my first attempt at a fix.

Ah, interesting, I'd assumed it was superfluous but it seems it isn't.

I've had a quick read through of what Junio has in "seen" from the last 
iteration of this series and it looked like a nice improvement to the 
usability.

Thanks

Phillip

> René
> 

Re: [PATCH] add-patch: roll over to next undecided hunk

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

> git add --patch presents diff hunks one after the other, asking whether
> to add them.  If we mark some as undecided, e.g. with J, then it will

Perhaps "mark" -> "leave".

I somehow find it awkward to say "mark as undecided", as I have
always viewed J/K as a way to skip a hunk, leaving it undecided.

Besides, "J" lets you revisit a hunk that you earlier have decided
to use of hold off, and it leaves your last decision on that hunk.
A statement that implies "J marks as undecided" is misleading.

> start over after reaching the last hunk.  It always starts over at the
> very first hunk, though, even if we already decided on it.  Skip
> decided hunks when rolling over instead.

Nicely analyzed.

> Reported-by: Windl, Ulrich <u.windl@ukr.de>
> Signed-off-by: René Scharfe <l.s.r@web.de>
> ---
>  add-patch.c                |  9 ++++++++-
>  t/t3701-add-interactive.sh | 20 ++++++++++++++++++++
>  2 files changed, 28 insertions(+), 1 deletion(-)
>
> diff --git a/add-patch.c b/add-patch.c
> index b0389c5d5b..42a8394c92 100644
> --- a/add-patch.c
> +++ b/add-patch.c
> @@ -1436,8 +1436,15 @@ static int patch_update_file(struct add_p_state *s,
>  	render_diff_header(s, file_diff, colored, &s->buf);
>  	fputs(s->buf.buf, stdout);
>  	for (;;) {
> -		if (hunk_index >= file_diff->hunk_nr)
> +		if (hunk_index >= file_diff->hunk_nr) {
>  			hunk_index = 0;
> +			for (i = 0; i < file_diff->hunk_nr; i++) {
> +				if (file_diff->hunk[i].use == UNDECIDED_HUNK) {
> +					hunk_index = i;
> +					break;
> +				}
> +			}
> +		}

OK.

This is probably a closely related tangent, but last night I was
looking this function and found that its per-hunk loop does
completely bogus thing.  For example, find a case where you have
more than one hunks, among which there are splittable and
non-splittable hunks (a hunk is splittable if there are context
lines between an added or a removed line).  Start cycling the hunks
without making any decisions with "J" or "K".  Once you visited a
splittable hunk (where you'd see 's' among the possible choices),
coming back to an unsplittable hunk will now let you split it!  's'
may not be visible among the choices, but telling it to 's'plit will
give you "Split into 1", which is a technically correct nonsense.

This is because the handling of "permitted" in that function only
adds, without resetting at the end of processing the current hunk.
Yet it does something like this:

	for (;;) {
		...
		strbuf_reset(&s->buf);
		if (file_diff->hunk_nr) {
			... add choices to the prompt ...
			if (hunk->splittable_into > 1) {
				permitted |= ALLOW_SPLIT;
				strbuf_addstr(&s->buf, ",s");
			}
			...
		}
		...
		printf(_(s->mode->prompt_mode[prompt_mode_type]),
		       s->buf.buf);
		if (*s->s.reset_color_interactive)
			fputs(s->s.reset_color_interactive, stdout);
		fflush(stdout);
		if (read_single_character(s) == EOF)
			break;
		ch = tolower(s->answer.buf[0]);
		... dispatch on the command character ...
		if (ch == 'y') {
			...
		} else if (s->answer.buf[0] == 's') {
			size_t splittable_into = hunk->splittable_into;
			if (!(permitted & ALLOW_SPLIT)) {
				err(s, _("Sorry, cannot split this hunk"));
			} else if (!split_hunk(s, file_diff,
					     hunk - file_diff->hunk)) {
				color_fprintf_ln(stdout, s->s.header_color,
						 _("Split into %d hunks."),
						 (int)splittable_into);
				rendered_hunk_index = -1;
			}
		...

Notice that the prompt is built correctly but that information is
*not* used when deciding if the operation is possible?

This is another ancient regression that was introduced while
rewriting this program in C near the end of 2019, I think.  And this
causes many other bugs in this area, like 'k' at the very first hunk
gets complaint "No previous hunk" only once (you move to the next
one with 'j' and come back to the first hunk with 'k', and then 'k'
no longer complains, even though it is not among the choice).

With this bug, however, we have gained a bit of useful feature, I
think.  Even though j/J should not be offered when we are at the
last hunk for a file, we do wrap-around to the first hunk.  I just
checked the original code before the C rewrite, and even though it
were written defensively so that incrementing the current hunk
number to 5 when you have only 4 hunks would take you back to the
initial hunk (instead of barfing), because we did not have this
"permitted is never reset" bug, it actually did not allow you to go
beyond the end with j/J.  Today's code seems to have inherited this
defensive adjustment to stay within the available hunks, and with
the "permitted is never reset" bug, we are taken back to the first
hunk.

Re: [PATCH] add-patch: roll over to next undecided hunk

[René Scharfe]

On 10/3/25 6:11 PM, Junio C Hamano wrote:
> René Scharfe <l.s.r@web.de> writes:
> 
>> git add --patch presents diff hunks one after the other, asking whether
>> to add them.  If we mark some as undecided, e.g. with J, then it will
> 
> Perhaps "mark" -> "leave".
> 
> I somehow find it awkward to say "mark as undecided", as I have
> always viewed J/K as a way to skip a hunk, leaving it undecided.
> 
> Besides, "J" lets you revisit a hunk that you earlier have decided
> to use of hold off, and it leaves your last decision on that hunk.
> A statement that implies "J marks as undecided" is misleading.

Right, j/J/k/K leave the use/skip/undecided status of the current hunk
unchanged.  "leave this hunk undecided" in the documentation is
misleading as well, because these options will not leave a hunk
undecided if we made a decision on it before:

               j - leave this hunk undecided, see next undecided hunk
               J - leave this hunk undecided, see next hunk
               k - leave this hunk undecided, see previous undecided hunk
               K - leave this hunk undecided, see previous hunk

Perhaps omit it?

               j - go to next undecided hunk
               J - go to next hunk
               k - go to previous undecided hunk
               K - go to previous hunk

Weird that one can switch between use and skip, but there's no
way to revert back to undecided.

>> start over after reaching the last hunk.  It always starts over at the
>> very first hunk, though, even if we already decided on it.  Skip
>> decided hunks when rolling over instead.
> 
> Nicely analyzed.
> 
>> Reported-by: Windl, Ulrich <u.windl@ukr.de>
>> Signed-off-by: René Scharfe <l.s.r@web.de>
>> ---
>>  add-patch.c                |  9 ++++++++-
>>  t/t3701-add-interactive.sh | 20 ++++++++++++++++++++
>>  2 files changed, 28 insertions(+), 1 deletion(-)
>>
>> diff --git a/add-patch.c b/add-patch.c
>> index b0389c5d5b..42a8394c92 100644
>> --- a/add-patch.c
>> +++ b/add-patch.c
>> @@ -1436,8 +1436,15 @@ static int patch_update_file(struct add_p_state *s,
>>  	render_diff_header(s, file_diff, colored, &s->buf);
>>  	fputs(s->buf.buf, stdout);
>>  	for (;;) {
>> -		if (hunk_index >= file_diff->hunk_nr)
>> +		if (hunk_index >= file_diff->hunk_nr) {
>>  			hunk_index = 0;
>> +			for (i = 0; i < file_diff->hunk_nr; i++) {
>> +				if (file_diff->hunk[i].use == UNDECIDED_HUNK) {
>> +					hunk_index = i;
>> +					break;
>> +				}
>> +			}
>> +		}
> 
> OK.
> 
> This is probably a closely related tangent, but last night I was
> looking this function and found that its per-hunk loop does
> completely bogus thing.  For example, find a case where you have
> more than one hunks, among which there are splittable and
> non-splittable hunks (a hunk is splittable if there are context
> lines between an added or a removed line).  Start cycling the hunks
> without making any decisions with "J" or "K".  Once you visited a
> splittable hunk (where you'd see 's' among the possible choices),
> coming back to an unsplittable hunk will now let you split it!  's'
> may not be visible among the choices, but telling it to 's'plit will
> give you "Split into 1", which is a technically correct nonsense.
> 
> This is because the handling of "permitted" in that function only
> adds, without resetting at the end of processing the current hunk.
> Yet it does something like this:
> 
> 	for (;;) {
> 		...
> 		strbuf_reset(&s->buf);
> 		if (file_diff->hunk_nr) {
> 			... add choices to the prompt ...
> 			if (hunk->splittable_into > 1) {
> 				permitted |= ALLOW_SPLIT;
> 				strbuf_addstr(&s->buf, ",s");
> 			}
> 			...
> 		}
> 		...
> 		printf(_(s->mode->prompt_mode[prompt_mode_type]),
> 		       s->buf.buf);
> 		if (*s->s.reset_color_interactive)
> 			fputs(s->s.reset_color_interactive, stdout);
> 		fflush(stdout);
> 		if (read_single_character(s) == EOF)
> 			break;
> 		ch = tolower(s->answer.buf[0]);
> 		... dispatch on the command character ...
> 		if (ch == 'y') {
> 			...
> 		} else if (s->answer.buf[0] == 's') {
> 			size_t splittable_into = hunk->splittable_into;
> 			if (!(permitted & ALLOW_SPLIT)) {
> 				err(s, _("Sorry, cannot split this hunk"));
> 			} else if (!split_hunk(s, file_diff,
> 					     hunk - file_diff->hunk)) {
> 				color_fprintf_ln(stdout, s->s.header_color,
> 						 _("Split into %d hunks."),
> 						 (int)splittable_into);
> 				rendered_hunk_index = -1;
> 			}
> 		...
> 
> Notice that the prompt is built correctly but that information is
> *not* used when deciding if the operation is possible?
> 
> This is another ancient regression that was introduced while
> rewriting this program in C near the end of 2019, I think.  And this
> causes many other bugs in this area, like 'k' at the very first hunk
> gets complaint "No previous hunk" only once (you move to the next
> one with 'j' and come back to the first hunk with 'k', and then 'k'
> no longer complains, even though it is not among the choice).

This should be easy to fix by resetting permitted at the start of the
loop, no?  Patch below.

> With this bug, however, we have gained a bit of useful feature, I
> think.  Even though j/J should not be offered when we are at the
> last hunk for a file, we do wrap-around to the first hunk.  I just
> checked the original code before the C rewrite, and even though it
> were written defensively so that incrementing the current hunk
> number to 5 when you have only 4 hunks would take you back to the
> initial hunk (instead of barfing), because we did not have this
> "permitted is never reset" bug, it actually did not allow you to go
> beyond the end with j/J.  Today's code seems to have inherited this
> defensive adjustment to stay within the available hunks, and with
> the "permitted is never reset" bug, we are taken back to the first
> hunk.
y/n/e on the last hunk roll over, which makes sense to me.  Their
movement part is not mentioned in the documentation, by the way.

With the patch below j/J are stopped by the floor, as seemingly
intended.  Not sure if the (now accidental) roll-over behavior is
better for them.


 add-patch.c                | 19 ++++++++++---------
 t/t3701-add-interactive.sh | 19 +++++++++++++++++++
 2 files changed, 29 insertions(+), 9 deletions(-)

diff --git a/add-patch.c b/add-patch.c
index 42a8394c92..1012840019 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -1418,15 +1418,6 @@ static int patch_update_file(struct add_p_state *s,
 	struct child_process cp = CHILD_PROCESS_INIT;
 	int colored = !!s->colored.len, quit = 0, use_pager = 0;
 	enum prompt_mode_type prompt_mode_type;
-	enum {
-		ALLOW_GOTO_PREVIOUS_HUNK = 1 << 0,
-		ALLOW_GOTO_PREVIOUS_UNDECIDED_HUNK = 1 << 1,
-		ALLOW_GOTO_NEXT_HUNK = 1 << 2,
-		ALLOW_GOTO_NEXT_UNDECIDED_HUNK = 1 << 3,
-		ALLOW_SEARCH_AND_GOTO = 1 << 4,
-		ALLOW_SPLIT = 1 << 5,
-		ALLOW_EDIT = 1 << 6
-	} permitted = 0;
 
 	/* Empty added files have no hunks */
 	if (!file_diff->hunk_nr && !file_diff->added)
@@ -1436,6 +1427,16 @@ static int patch_update_file(struct add_p_state *s,
 	render_diff_header(s, file_diff, colored, &s->buf);
 	fputs(s->buf.buf, stdout);
 	for (;;) {
+		enum {
+			ALLOW_GOTO_PREVIOUS_HUNK = 1 << 0,
+			ALLOW_GOTO_PREVIOUS_UNDECIDED_HUNK = 1 << 1,
+			ALLOW_GOTO_NEXT_HUNK = 1 << 2,
+			ALLOW_GOTO_NEXT_UNDECIDED_HUNK = 1 << 3,
+			ALLOW_SEARCH_AND_GOTO = 1 << 4,
+			ALLOW_SPLIT = 1 << 5,
+			ALLOW_EDIT = 1 << 6
+		} permitted = 0;
+
 		if (hunk_index >= file_diff->hunk_nr) {
 			hunk_index = 0;
 			for (i = 0; i < file_diff->hunk_nr; i++) {
diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index fa6ec5f835..33b307b8ff 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -1341,6 +1341,25 @@ test_expect_success 'roll over to next undecided (2)' '
 	test_cmp expect hunks
 '
 
+test_expect_success 'invalid options are rejected' '
+	test_write_lines a b c d e f g h i j k >file &&
+	git add file &&
+	test_write_lines X b c d e f g h X j X >file &&
+	test_write_lines j j J k k K s q | git add -p >out &&
+	sed -ne "s/ @@.*//" -e "s/ \$//" -e "/^(/p" <out >actual &&
+	cat >expect <<-EOF &&
+	(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]?
+	(2/2) Stage this hunk [y,n,q,a,d,k,K,g,/,s,e,p,?]? No next hunk
+	(2/2) Stage this hunk [y,n,q,a,d,k,K,g,/,s,e,p,?]? No next hunk
+	(2/2) Stage this hunk [y,n,q,a,d,k,K,g,/,s,e,p,?]?
+	(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]? No previous hunk
+	(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]? No previous hunk
+	(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]? Sorry, cannot split this hunk
+	(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]?
+	EOF
+	test_cmp expect actual
+'
+
 test_expect_success 'set up base for -p color tests' '
 	echo commit >file &&
 	git commit -am "commit state" &&

Re: [PATCH] add-patch: roll over to next undecided hunk

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

> Weird that one can switch between use and skip, but there's no
> way to revert back to undecided.

Yes, but Phillip's "if you split the resulting hunks will revert to
undecided" topic, together with "you can split one hunk into one"
bug that is caused by the "permitted is never reset" bug, if you can
navigate back to what you already decided to use or skip, you can
say "split" to revert it undecided ;-).

> This should be easy to fix by resetting permitted at the start of the
> loop, no?  Patch below.
>
>> With this bug, however, we have gained a bit of useful feature, I
>> think.  Even though j/J should not be offered when we are at the
>> last hunk for a file, we do wrap-around to the first hunk.  I just
>> checked the original code before the C rewrite, and even though it
>> were written defensively so that incrementing the current hunk
>> number to 5 when you have only 4 hunks would take you back to the
>> initial hunk (instead of barfing), because we did not have this
>> "permitted is never reset" bug, it actually did not allow you to go
>> beyond the end with j/J.  Today's code seems to have inherited this
>> defensive adjustment to stay within the available hunks, and with
>> the "permitted is never reset" bug, we are taken back to the first
>> hunk.
> y/n/e on the last hunk roll over, which makes sense to me.  Their
> movement part is not mentioned in the documentation, by the way.
>
> With the patch below j/J are stopped by the floor, as seemingly
> intended.  Not sure if the (now accidental) roll-over behavior is
> better for them.

Yes.  Even if it is accidental, people are too used the roll-over
behaviour.  So at least we should always allow J/K and probably
allow j/k as long as there at least is a single undecided hunk, if
we were to do this fix, and make the prompt string to match.

I only am aware of this bugginess in "j,k,J,K,s" but that is only
because I did not look at others.  I wouldn't be surprised if they
were even buggier.

Thanks.

Re: [PATCH] add-patch: roll over to next undecided hunk

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

> On 10/3/25 6:11 PM, Junio C Hamano wrote:
>> René Scharfe <l.s.r@web.de> writes:
>> 
>>> git add --patch presents diff hunks one after the other, asking whether
>>> to add them.  If we mark some as undecided, e.g. with J, then it will
>> 
>> Perhaps "mark" -> "leave".
>> 
>> I somehow find it awkward to say "mark as undecided", as I have
>> always viewed J/K as a way to skip a hunk, leaving it undecided.
>> 
>> Besides, "J" lets you revisit a hunk that you earlier have decided
>> to use of hold off, and it leaves your last decision on that hunk.
>> A statement that implies "J marks as undecided" is misleading.
>
> Right, j/J/k/K leave the use/skip/undecided status of the current hunk
> unchanged.

Yes.  If the one you are walking away with 'J' were already
selected, the scenario you describe in the proposed log message
would not work, so "mark" -> "leave" is the right thing to do in
that context.  But ...

> "leave this hunk undecided" in the documentation is
> misleading as well, because these options will not leave a hunk
> undecided if we made a decision on it before:

... as you say, I agree that your updated version

>                j - go to next undecided hunk
>                J - go to next hunk
>                k - go to previous undecided hunk
>                K - go to previous hunk

in the documentation or help would be a good change.

> Weird that one can switch between use and skip, but there's no
> way to revert back to undecided.

Another thing that is missing (and these two are not a regression in
the C version, but the same in my scripted original) is that once
you decided on _all_ hunks of a file, there is no way to come back
and tell the tool that you changed your mind.

Re: [PATCH] add-patch: roll over to next undecided hunk

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

> git add --patch presents diff hunks one after the other, asking whether
> to add them.  If we mark some as undecided, e.g. with J, then it will
> start over after reaching the last hunk.  It always starts over at the
> very first hunk, though, even if we already decided on it.  Skip
> decided hunks when rolling over instead.

Wait a bit.  With 'J', the user wants to walk the list of hunks,
both decided and undecided ones, no?  So ...

> diff --git a/add-patch.c b/add-patch.c
> index b0389c5d5b..42a8394c92 100644
> --- a/add-patch.c
> +++ b/add-patch.c
> @@ -1436,8 +1436,15 @@ static int patch_update_file(struct add_p_state *s,
>  	render_diff_header(s, file_diff, colored, &s->buf);
>  	fputs(s->buf.buf, stdout);
>  	for (;;) {
> -		if (hunk_index >= file_diff->hunk_nr)
> +		if (hunk_index >= file_diff->hunk_nr) {
>  			hunk_index = 0;
> +			for (i = 0; i < file_diff->hunk_nr; i++) {
> +				if (file_diff->hunk[i].use == UNDECIDED_HUNK) {
> +					hunk_index = i;
> +					break;
> +				}
> +			}
> +		}

... why is it a good idea to skip decided ones?

With 'j', the story is probably different, but I didn't dig deep
enough.

With 'K', we seem to do

		} else if (s->answer.buf[0] == 'K') {
			if (permitted & ALLOW_GOTO_PREVIOUS_HUNK)
				hunk_index--;
			else
				err(s, _("No previous hunk"));

and there is *no* guard around here to say "hey, hunk_index has gone
negative, so we must move back to the last hunk", similar to what is
happening here that does "hunk_index has gone beyond the end, so
let's wrap around to the first one".

This is another bug that is maked by the fact that hunk_index is
measured in size_t (even though there is no reason to do so).  Using
unsigned hunk_ix is simply crazy here, especially for a code that
wants "just do hunk_index++ and adjust if it goes beyond the end"
and similarly "just do hunk_index-- and adjust if it goes beyond the
beginning".

In any case, as the result, going back with 'K' when we are on the
first hunk is broken with the current code because we'd stay there,
and with this patch, we'd move to the first undecided hunk.  Neither
is correct---we should move to the last hunk, I would think.

[PATCH v2 0/5] add-patch: roll over to next undecided hunk

[René Scharfe]

Changes since v1:
- add patches 1 and 5 to address issues that came up in conversation
- split out roll-over of option j
- let options J, k, and K roll over as well
- broader test coverage

  add-patch: improve help for options j, J, k, and K
  add-patch: document that option J rolls over
  add-patch: let options y, n, j, and e roll over to next undecided
  add-patch: let options k and K roll over like j and J
  add-patch: reset "permitted" at loop start

 Documentation/git-add.adoc |  8 ++--
 add-patch.c                | 52 +++++++++++++++++---------
 t/t3701-add-interactive.sh | 76 ++++++++++++++++++++++++++++++--------
 3 files changed, 99 insertions(+), 37 deletions(-)

-- 
2.51.0

[PATCH v2 1/5] add-patch: improve help for options j, J, k, and K

[René Scharfe]

The options j, J, k, and K don't affect the status of the current hunk.
They just go to a different one.  This is true whether the current hunk
is undecided or not.  Avoid misunderstanding by no longer mentioning
the current hunk explicitly in their help texts.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 Documentation/git-add.adoc | 8 ++++----
 add-patch.c                | 8 ++++----
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/Documentation/git-add.adoc b/Documentation/git-add.adoc
index ad629c46c5..3266ccf105 100644
--- a/Documentation/git-add.adoc
+++ b/Documentation/git-add.adoc
@@ -342,10 +342,10 @@ patch::
        d - do not stage this hunk or any of the later hunks in the file
        g - select a hunk to go to
        / - search for a hunk matching the given regex
-       j - leave this hunk undecided, see next undecided hunk
-       J - leave this hunk undecided, see next hunk
-       k - leave this hunk undecided, see previous undecided hunk
-       K - leave this hunk undecided, see previous hunk
+       j - go to the next undecided hunk
+       J - go to the next hunk
+       k - go to the previous undecided hunk
+       K - go to the previous hunk
        s - split the current hunk into smaller hunks
        e - manually edit the current hunk
        p - print the current hunk
diff --git a/add-patch.c b/add-patch.c
index b0389c5d5b..912266a3f8 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -1397,10 +1397,10 @@ static size_t display_hunks(struct add_p_state *s,
 }
 
 static const char help_patch_remainder[] =
-N_("j - leave this hunk undecided, see next undecided hunk\n"
-   "J - leave this hunk undecided, see next hunk\n"
-   "k - leave this hunk undecided, see previous undecided hunk\n"
-   "K - leave this hunk undecided, see previous hunk\n"
+N_("j - go to the next undecided hunk\n"
+   "J - go to the next hunk\n"
+   "k - go to the previous undecided hunk\n"
+   "K - go to the previous hunk\n"
    "g - select a hunk to go to\n"
    "/ - search for a hunk matching the given regex\n"
    "s - split the current hunk into smaller hunks\n"
-- 
2.51.0

Re: [PATCH v2 1/5] add-patch: improve help for options j, J, k, and K

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

> The options j, J, k, and K don't affect the status of the current hunk.
> They just go to a different one.  This is true whether the current hunk
> is undecided or not.  Avoid misunderstanding by no longer mentioning
> the current hunk explicitly in their help texts.
>
> Signed-off-by: René Scharfe <l.s.r@web.de>
> ---
>  Documentation/git-add.adoc | 8 ++++----
>  add-patch.c                | 8 ++++----
>  2 files changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/Documentation/git-add.adoc b/Documentation/git-add.adoc
> index ad629c46c5..3266ccf105 100644
> --- a/Documentation/git-add.adoc
> +++ b/Documentation/git-add.adoc
> @@ -342,10 +342,10 @@ patch::
>         d - do not stage this hunk or any of the later hunks in the file
>         g - select a hunk to go to
>         / - search for a hunk matching the given regex
> -       j - leave this hunk undecided, see next undecided hunk
> -       J - leave this hunk undecided, see next hunk
> -       k - leave this hunk undecided, see previous undecided hunk
> -       K - leave this hunk undecided, see previous hunk
> +       j - go to the next undecided hunk
> +       J - go to the next hunk
> +       k - go to the previous undecided hunk
> +       K - go to the previous hunk

These obviously make sense, but I wonder if y/n should also say that
they not just make a decision on the current hunk, but after doing
so they move you forward (and if so, that may fall within the theme
of this step, which is to improve the help text on options).

Re: [PATCH v2 1/5] add-patch: improve help for options j, J, k, and K

[René Scharfe]

On 10/5/25 11:30 PM, Junio C Hamano wrote:
> René Scharfe <l.s.r@web.de> writes:
> 
>> The options j, J, k, and K don't affect the status of the current hunk.
>> They just go to a different one.  This is true whether the current hunk
>> is undecided or not.  Avoid misunderstanding by no longer mentioning
>> the current hunk explicitly in their help texts.
>>
>> Signed-off-by: René Scharfe <l.s.r@web.de>
>> ---
>>  Documentation/git-add.adoc | 8 ++++----
>>  add-patch.c                | 8 ++++----
>>  2 files changed, 8 insertions(+), 8 deletions(-)
>>
>> diff --git a/Documentation/git-add.adoc b/Documentation/git-add.adoc
>> index ad629c46c5..3266ccf105 100644
>> --- a/Documentation/git-add.adoc
>> +++ b/Documentation/git-add.adoc
>> @@ -342,10 +342,10 @@ patch::
>>         d - do not stage this hunk or any of the later hunks in the file
>>         g - select a hunk to go to
>>         / - search for a hunk matching the given regex
>> -       j - leave this hunk undecided, see next undecided hunk
>> -       J - leave this hunk undecided, see next hunk
>> -       k - leave this hunk undecided, see previous undecided hunk
>> -       K - leave this hunk undecided, see previous hunk
>> +       j - go to the next undecided hunk
>> +       J - go to the next hunk
>> +       k - go to the previous undecided hunk
>> +       K - go to the previous hunk
> 
> These obviously make sense, but I wonder if y/n should also say that
> they not just make a decision on the current hunk, but after doing
> so they move you forward

Yes.

> (and if so, that may fall within the theme
> of this step, which is to improve the help text on options).

I see it more narrowly: This patch removes unnecessary references to the
hunk's status, while a y/n doc patch would add missing pieces.

Hmm, would the help text need to adapt to whether the current hunk is
the last undecided one?  E.g., "stage this hunk, implies 'j'" if j is an
allowed option and "stage this hunk and quit" otherwise?  Stuff for a
separate series, I think.

René

Re: [PATCH v2 1/5] add-patch: improve help for options j, J, k, and K

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

> I see it more narrowly: This patch removes unnecessary references to the
> hunk's status, while a y/n doc patch would add missing pieces.

Good.

> Hmm, would the help text need to adapt to whether the current hunk is
> the last undecided one?  E.g., "stage this hunk, implies 'j'" if j is an
> allowed option and "stage this hunk and quit" otherwise?  Stuff for a
> separate series, I think.

Sounds good.

RE: [EXT] [PATCH v2 1/5] add-patch: improve help for options j, J, k, and K

[Windl, Ulrich]

Hi!

Sorry for the delay, but I was on vacation without access to this mailbox.

For the patch
diff --git a/Documentation/git-add.adoc b/Documentation/git-add.adoc
index ad629c46c5..3266ccf105 100644

I don't see an actual improvement, and I'd prefer the previous version of the doc.
Likwise for
diff --git a/add-patch.c b/add-patch.c
index b0389c5d5b..912266a3f8 100644

Kind regards,
Ulrich Windl

> -----Original Message-----
> From: René Scharfe <l.s.r@web.de>
> Sent: Sunday, October 5, 2025 5:55 PM
> To: git@vger.kernel.org
> Cc: Windl, Ulrich <u.windl@ukr.de>; Junio C Hamano <gitster@pobox.com>;
> Phillip Wood <phillip.wood@dunelm.org.uk>
> Subject: [EXT] [PATCH v2 1/5] add-patch: improve help for options j, J, k, and
> K
> 
> Sicherheits-Hinweis: Diese E-Mail wurde von einer Person außerhalb des UKR
> gesendet. Seien Sie vorsichtig vor gefälschten Absendern, wenn Sie auf Links
> klicken, Anhänge öffnen oder weitere Aktionen ausführen, bevor Sie die
> Echtheit überprüft haben.
> 
> The options j, J, k, and K don't affect the status of the current hunk.
> They just go to a different one.  This is true whether the current hunk
> is undecided or not.  Avoid misunderstanding by no longer mentioning
> the current hunk explicitly in their help texts.
> 
> Signed-off-by: René Scharfe <l.s.r@web.de>
> ---
>  Documentation/git-add.adoc | 8 ++++----
>  add-patch.c                | 8 ++++----
>  2 files changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/Documentation/git-add.adoc b/Documentation/git-add.adoc
> index ad629c46c5..3266ccf105 100644
> --- a/Documentation/git-add.adoc
> +++ b/Documentation/git-add.adoc
> @@ -342,10 +342,10 @@ patch::
>         d - do not stage this hunk or any of the later hunks in the file
>         g - select a hunk to go to
>         / - search for a hunk matching the given regex
> -       j - leave this hunk undecided, see next undecided hunk
> -       J - leave this hunk undecided, see next hunk
> -       k - leave this hunk undecided, see previous undecided hunk
> -       K - leave this hunk undecided, see previous hunk
> +       j - go to the next undecided hunk
> +       J - go to the next hunk
> +       k - go to the previous undecided hunk
> +       K - go to the previous hunk
>         s - split the current hunk into smaller hunks
>         e - manually edit the current hunk
>         p - print the current hunk
> diff --git a/add-patch.c b/add-patch.c
> index b0389c5d5b..912266a3f8 100644
> --- a/add-patch.c
> +++ b/add-patch.c
> @@ -1397,10 +1397,10 @@ static size_t display_hunks(struct add_p_state
> *s,
>  }
> 
>  static const char help_patch_remainder[] =
> -N_("j - leave this hunk undecided, see next undecided hunk\n"
> -   "J - leave this hunk undecided, see next hunk\n"
> -   "k - leave this hunk undecided, see previous undecided hunk\n"
> -   "K - leave this hunk undecided, see previous hunk\n"
> +N_("j - go to the next undecided hunk\n"
> +   "J - go to the next hunk\n"
> +   "k - go to the previous undecided hunk\n"
> +   "K - go to the previous hunk\n"
>     "g - select a hunk to go to\n"
>     "/ - search for a hunk matching the given regex\n"
>     "s - split the current hunk into smaller hunks\n"
> --
> 2.51.0

Re: [EXT] [PATCH v2 1/5] add-patch: improve help for options j, J, k, and K

[Junio C Hamano]

"Windl, Ulrich" <u.windl@ukr.de> writes:

> For the patch
> diff --git a/Documentation/git-add.adoc b/Documentation/git-add.adoc
> index ad629c46c5..3266ccf105 100644
>
> I don't see an actual improvement, and I'd prefer the previous
> version of the doc.

We'd prefer to see something more concrete that refuses the
reasoning that led to the change, than a subjective "I don't see,
I'd prefer".

At least, the commit log messge given by 2c3cc43f (add-patch:
improve help for options j, J, k, and K, 2025-10-06) explains why
the change is an improvement, and I found it sensible.
(<b5034851-65bd-49da-b270-48b68d9210ff@web.de>) 

The old description said 'j' leaves this hunk undecided and goes to
the next undecided hunk, but it is both pointless and misleading to
say 'leave this hunk undecided'.  Unlike 'y' or 'n', the movement
options 'j', 'k' are not about changing the state of the current
thing we are on (so it is pointless to say "LEAVE it undecided"),
and more importantly, when we say 'j', the state of the current
thing we are on may not necessarily be 'undecided' (so it is
misleading to say "leave it UNDECIDED").

RE: [EXT] Re: [PATCH v2 1/5] add-patch: improve help for options j, J, k, and K

[Windl, Ulrich]

OK,

fair enough: I think the original wording is more clear, so I don't see the need to change it at all.
Possible corner cases ecepted, e.g. whether "next" can wrap at the end or not.

Kind regards,
Ulrich Windl

> -----Original Message-----
> From: Junio C Hamano <gitster@pobox.com>
> Sent: Saturday, November 1, 2025 9:19 AM
> To: Windl, Ulrich <u.windl@ukr.de>
> Cc: René Scharfe <l.s.r@web.de>; git@vger.kernel.org; Phillip Wood
> <phillip.wood@dunelm.org.uk>
> Subject: [EXT] Re: [PATCH v2 1/5] add-patch: improve help for options j, J, k,
> and K
> 
> Sicherheits-Hinweis: Diese E-Mail wurde von einer Person außerhalb des UKR
> gesendet. Seien Sie vorsichtig vor gefälschten Absendern, wenn Sie auf Links
> klicken, Anhänge öffnen oder weitere Aktionen ausführen, bevor Sie die
> Echtheit überprüft haben.
> 
> "Windl, Ulrich" <u.windl@ukr.de> writes:
> 
> > For the patch
> > diff --git a/Documentation/git-add.adoc b/Documentation/git-add.adoc
> > index ad629c46c5..3266ccf105 100644
> >
> > I don't see an actual improvement, and I'd prefer the previous
> > version of the doc.
> 
> We'd prefer to see something more concrete that refuses the
> reasoning that led to the change, than a subjective "I don't see,
> I'd prefer".
> 
> At least, the commit log messge given by 2c3cc43f (add-patch:
> improve help for options j, J, k, and K, 2025-10-06) explains why
> the change is an improvement, and I found it sensible.
> (<b5034851-65bd-49da-b270-48b68d9210ff@web.de>)
> 
> The old description said 'j' leaves this hunk undecided and goes to
> the next undecided hunk, but it is both pointless and misleading to
> say 'leave this hunk undecided'.  Unlike 'y' or 'n', the movement
> options 'j', 'k' are not about changing the state of the current
> thing we are on (so it is pointless to say "LEAVE it undecided"),
> and more importantly, when we say 'j', the state of the current
> thing we are on may not necessarily be 'undecided' (so it is
> misleading to say "leave it UNDECIDED").

[PATCH v2 2/5] add-patch: document that option J rolls over

[René Scharfe]

The variable "permitted" is only not reset after moving to a different
hunk, so it only accumulates permission and doesn't necessarily reflect
those of the current hunk.  This may be a bug, but is actually useful
with the option J, which can be used at the last hunk to roll over to
the first hunk.  Make this particular behavior official.

Suggested-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: René Scharfe <l.s.r@web.de>
---
 Documentation/git-add.adoc |  2 +-
 add-patch.c                |  4 ++--
 t/t3701-add-interactive.sh | 18 ++++++++++++++----
 3 files changed, 17 insertions(+), 7 deletions(-)

diff --git a/Documentation/git-add.adoc b/Documentation/git-add.adoc
index 3266ccf105..5c05a3a7f9 100644
--- a/Documentation/git-add.adoc
+++ b/Documentation/git-add.adoc
@@ -343,7 +343,7 @@ patch::
        g - select a hunk to go to
        / - search for a hunk matching the given regex
        j - go to the next undecided hunk
-       J - go to the next hunk
+       J - go to the next hunk, roll over at the bottom
        k - go to the previous undecided hunk
        K - go to the previous hunk
        s - split the current hunk into smaller hunks
diff --git a/add-patch.c b/add-patch.c
index 912266a3f8..bef2ba7a25 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -1398,7 +1398,7 @@ static size_t display_hunks(struct add_p_state *s,
 
 static const char help_patch_remainder[] =
 N_("j - go to the next undecided hunk\n"
-   "J - go to the next hunk\n"
+   "J - go to the next hunk, roll over at the bottom\n"
    "k - go to the previous undecided hunk\n"
    "K - go to the previous hunk\n"
    "g - select a hunk to go to\n"
@@ -1493,7 +1493,7 @@ static int patch_update_file(struct add_p_state *s,
 				permitted |= ALLOW_GOTO_NEXT_UNDECIDED_HUNK;
 				strbuf_addstr(&s->buf, ",j");
 			}
-			if (hunk_index + 1 < file_diff->hunk_nr) {
+			if (file_diff->hunk_nr > 1) {
 				permitted |= ALLOW_GOTO_NEXT_HUNK;
 				strbuf_addstr(&s->buf, ",J");
 			}
diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index d9fe289a7a..d5d2e120ab 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -334,7 +334,7 @@ test_expect_success 'different prompts for mode change/deleted' '
 	cat >expect <<-\EOF &&
 	(1/1) Stage deletion [y,n,q,a,d,p,?]?
 	(1/2) Stage mode change [y,n,q,a,d,j,J,g,/,p,?]?
-	(2/2) Stage this hunk [y,n,q,a,d,K,g,/,e,p,?]?
+	(2/2) Stage this hunk [y,n,q,a,d,K,J,g,/,e,p,?]?
 	EOF
 	test_cmp expect actual.filtered
 '
@@ -521,7 +521,7 @@ test_expect_success 'split hunk setup' '
 test_expect_success 'goto hunk 1 with "g 1"' '
 	test_when_finished "git reset" &&
 	tr _ " " >expect <<-EOF &&
-	(2/2) Stage this hunk [y,n,q,a,d,K,g,/,e,p,?]? + 1:  -1,2 +1,3          +15
+	(2/2) Stage this hunk [y,n,q,a,d,K,J,g,/,e,p,?]? + 1:  -1,2 +1,3          +15
 	_ 2:  -2,4 +3,8          +21
 	go to which hunk? @@ -1,2 +1,3 @@
 	_10
@@ -550,7 +550,7 @@ test_expect_success 'goto hunk 1 with "g1"' '
 test_expect_success 'navigate to hunk via regex /pattern' '
 	test_when_finished "git reset" &&
 	tr _ " " >expect <<-EOF &&
-	(2/2) Stage this hunk [y,n,q,a,d,K,g,/,e,p,?]? @@ -1,2 +1,3 @@
+	(2/2) Stage this hunk [y,n,q,a,d,K,J,g,/,e,p,?]? @@ -1,2 +1,3 @@
 	_10
 	+15
 	_20
@@ -805,7 +805,7 @@ test_expect_success 'colors can be overridden' '
 	<YELLOW>(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]? <RESET><MAGENTA>@@ -3 +3,2 @@<RESET>
 	<CYAN> more-context<RESET>
 	<BLUE>+<RESET><BLUE>another-one<RESET>
-	<YELLOW>(2/2) Stage this hunk [y,n,q,a,d,K,g,/,e,p,?]? <RESET><MAGENTA>@@ -1,3 +1,3 @@<RESET>
+	<YELLOW>(2/2) Stage this hunk [y,n,q,a,d,K,J,g,/,e,p,?]? <RESET><MAGENTA>@@ -1,3 +1,3 @@<RESET>
 	<CYAN> context<RESET>
 	<BOLD>-old<RESET>
 	<BLUE>+new<RESET>
@@ -1354,4 +1354,14 @@ do
 	'
 done
 
+test_expect_success 'option J rolls over' '
+	test_write_lines a b c d e f g h i >file &&
+	git add file &&
+	test_write_lines X b c d e f g h X >file &&
+	test_write_lines J J q | git add -p >out &&
+	test_write_lines 1 2 1 >expect &&
+	sed -n -e "s-/.*--" -e "s/^(//p" <out >actual &&
+	test_cmp expect actual
+'
+
 test_done
-- 
2.51.0

Re: [PATCH v2 2/5] add-patch: document that option J rolls over

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

> The variable "permitted" is only not reset after moving to a different

"only not" -> "not".

> hunk, so it only accumulates permission and doesn't necessarily reflect
> those of the current hunk.  This may be a bug, but is actually useful
> with the option J, which can be used at the last hunk to roll over to
> the first hunk.  Make this particular behavior official.
>
> Suggested-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: René Scharfe <l.s.r@web.de>
> ---
>  Documentation/git-add.adoc |  2 +-
>  add-patch.c                |  4 ++--
>  t/t3701-add-interactive.sh | 18 ++++++++++++++----
>  3 files changed, 17 insertions(+), 7 deletions(-)
>
> diff --git a/Documentation/git-add.adoc b/Documentation/git-add.adoc
> index 3266ccf105..5c05a3a7f9 100644
> --- a/Documentation/git-add.adoc
> +++ b/Documentation/git-add.adoc
> @@ -343,7 +343,7 @@ patch::
>         g - select a hunk to go to
>         / - search for a hunk matching the given regex
>         j - go to the next undecided hunk
> -       J - go to the next hunk
> +       J - go to the next hunk, roll over at the bottom
>         k - go to the previous undecided hunk
>         K - go to the previous hunk
>         s - split the current hunk into smaller hunks
> diff --git a/add-patch.c b/add-patch.c
> index 912266a3f8..bef2ba7a25 100644
> --- a/add-patch.c
> +++ b/add-patch.c
> @@ -1398,7 +1398,7 @@ static size_t display_hunks(struct add_p_state *s,
>  
>  static const char help_patch_remainder[] =
>  N_("j - go to the next undecided hunk\n"
> -   "J - go to the next hunk\n"
> +   "J - go to the next hunk, roll over at the bottom\n"
>     "k - go to the previous undecided hunk\n"
>     "K - go to the previous hunk\n"
>     "g - select a hunk to go to\n"
> @@ -1493,7 +1493,7 @@ static int patch_update_file(struct add_p_state *s,
>  				permitted |= ALLOW_GOTO_NEXT_UNDECIDED_HUNK;
>  				strbuf_addstr(&s->buf, ",j");
>  			}
> -			if (hunk_index + 1 < file_diff->hunk_nr) {
> +			if (file_diff->hunk_nr > 1) {
>  				permitted |= ALLOW_GOTO_NEXT_HUNK;
>  				strbuf_addstr(&s->buf, ",J");
>  			}
> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
> index d9fe289a7a..d5d2e120ab 100755
> --- a/t/t3701-add-interactive.sh
> +++ b/t/t3701-add-interactive.sh
> @@ -334,7 +334,7 @@ test_expect_success 'different prompts for mode change/deleted' '
>  	cat >expect <<-\EOF &&
>  	(1/1) Stage deletion [y,n,q,a,d,p,?]?
>  	(1/2) Stage mode change [y,n,q,a,d,j,J,g,/,p,?]?
> -	(2/2) Stage this hunk [y,n,q,a,d,K,g,/,e,p,?]?
> +	(2/2) Stage this hunk [y,n,q,a,d,K,J,g,/,e,p,?]?
>  	EOF
>  	test_cmp expect actual.filtered
>  '
> @@ -521,7 +521,7 @@ test_expect_success 'split hunk setup' '
>  test_expect_success 'goto hunk 1 with "g 1"' '
>  	test_when_finished "git reset" &&
>  	tr _ " " >expect <<-EOF &&
> -	(2/2) Stage this hunk [y,n,q,a,d,K,g,/,e,p,?]? + 1:  -1,2 +1,3          +15
> +	(2/2) Stage this hunk [y,n,q,a,d,K,J,g,/,e,p,?]? + 1:  -1,2 +1,3          +15
>  	_ 2:  -2,4 +3,8          +21
>  	go to which hunk? @@ -1,2 +1,3 @@
>  	_10
> @@ -550,7 +550,7 @@ test_expect_success 'goto hunk 1 with "g1"' '
>  test_expect_success 'navigate to hunk via regex /pattern' '
>  	test_when_finished "git reset" &&
>  	tr _ " " >expect <<-EOF &&
> -	(2/2) Stage this hunk [y,n,q,a,d,K,g,/,e,p,?]? @@ -1,2 +1,3 @@
> +	(2/2) Stage this hunk [y,n,q,a,d,K,J,g,/,e,p,?]? @@ -1,2 +1,3 @@
>  	_10
>  	+15
>  	_20
> @@ -805,7 +805,7 @@ test_expect_success 'colors can be overridden' '
>  	<YELLOW>(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]? <RESET><MAGENTA>@@ -3 +3,2 @@<RESET>
>  	<CYAN> more-context<RESET>
>  	<BLUE>+<RESET><BLUE>another-one<RESET>
> -	<YELLOW>(2/2) Stage this hunk [y,n,q,a,d,K,g,/,e,p,?]? <RESET><MAGENTA>@@ -1,3 +1,3 @@<RESET>
> +	<YELLOW>(2/2) Stage this hunk [y,n,q,a,d,K,J,g,/,e,p,?]? <RESET><MAGENTA>@@ -1,3 +1,3 @@<RESET>
>  	<CYAN> context<RESET>
>  	<BOLD>-old<RESET>
>  	<BLUE>+new<RESET>
> @@ -1354,4 +1354,14 @@ do
>  	'
>  done
>  
> +test_expect_success 'option J rolls over' '
> +	test_write_lines a b c d e f g h i >file &&
> +	git add file &&
> +	test_write_lines X b c d e f g h X >file &&
> +	test_write_lines J J q | git add -p >out &&
> +	test_write_lines 1 2 1 >expect &&
> +	sed -n -e "s-/.*--" -e "s/^(//p" <out >actual &&
> +	test_cmp expect actual
> +'
> +
>  test_done

[PATCH v2 3/5] add-patch: let options y, n, j, and e roll over to next undecided

[René Scharfe]

The options y, n, and e mark the current hunk as decided.  If there's
another undecided hunk towards the bottom of the hunk array they go
there.  If there isn't, but there is another undecided hunk towards the
top then they go to the very first hunk, no matter if it has already
been decided on.

The option j does basically the same move.  Technically it is not
allowed if there's no undecided hunk towards the bottom, but the
variable "permitted" is never reset, so this permission is retained
from the very first hunk.  That may a bug, but this behavior is at
least consistent with y, n, and e and arguably more useful than
refusing to move.

Improve the roll-over behavior of these four options by moving to the
first undecided hunk instead of hunk 1, consistent with what they do
when not rolling over.

Reported-by: Windl, Ulrich <u.windl@ukr.de>
Signed-off-by: René Scharfe <l.s.r@web.de>
---
 Documentation/git-add.adoc |  2 +-
 add-patch.c                | 11 +++++++++--
 t/t3701-add-interactive.sh | 22 ++++++++++++++++++++++
 3 files changed, 32 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-add.adoc b/Documentation/git-add.adoc
index 5c05a3a7f9..596cdeff93 100644
--- a/Documentation/git-add.adoc
+++ b/Documentation/git-add.adoc
@@ -342,7 +342,7 @@ patch::
        d - do not stage this hunk or any of the later hunks in the file
        g - select a hunk to go to
        / - search for a hunk matching the given regex
-       j - go to the next undecided hunk
+       j - go to the next undecided hunk, roll over at the bottom
        J - go to the next hunk, roll over at the bottom
        k - go to the previous undecided hunk
        K - go to the previous hunk
diff --git a/add-patch.c b/add-patch.c
index bef2ba7a25..da75618dcb 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -1397,7 +1397,7 @@ static size_t display_hunks(struct add_p_state *s,
 }
 
 static const char help_patch_remainder[] =
-N_("j - go to the next undecided hunk\n"
+N_("j - go to the next undecided hunk, roll over at the bottom\n"
    "J - go to the next hunk, roll over at the bottom\n"
    "k - go to the previous undecided hunk\n"
    "K - go to the previous hunk\n"
@@ -1408,6 +1408,11 @@ N_("j - go to the next undecided hunk\n"
    "p - print the current hunk, 'P' to use the pager\n"
    "? - print help\n");
 
+static size_t inc_mod(size_t a, size_t m)
+{
+	return a < m - 1 ? a + 1 : 0;
+}
+
 static int patch_update_file(struct add_p_state *s,
 			     struct file_diff *file_diff)
 {
@@ -1451,7 +1456,9 @@ static int patch_update_file(struct add_p_state *s,
 					break;
 				}
 
-			for (i = hunk_index + 1; i < file_diff->hunk_nr; i++)
+			for (i = inc_mod(hunk_index, file_diff->hunk_nr);
+			     i != hunk_index;
+			     i = inc_mod(i, file_diff->hunk_nr))
 				if (file_diff->hunk[i].use == UNDECIDED_HUNK) {
 					undecided_next = i;
 					break;
diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index d5d2e120ab..8086d3da71 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -1364,4 +1364,26 @@ test_expect_success 'option J rolls over' '
 	test_cmp expect actual
 '
 
+test_expect_success 'options y, n, j, e roll over to next undecided (1)' '
+	test_write_lines a b c d e f g h i j k l m n o p q >file &&
+	git add file &&
+	test_write_lines X b c d e f g h X j k l m n o p X >file &&
+	test_set_editor : &&
+	test_write_lines g3 y g3 n g3 j g3 e q | git add -p >out &&
+	test_write_lines 1  3 1  3 1  3 1  3 1 >expect &&
+	sed -n -e "s-/.*--" -e "s/^(//p" <out >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'options y, n, j, e roll over to next undecided (2)' '
+	test_write_lines a b c d e f g h i j k l m n o p q >file &&
+	git add file &&
+	test_write_lines X b c d e f g h X j k l m n o p X >file &&
+	test_set_editor : &&
+	test_write_lines y g3 y g3 n g3 j g3 e q | git add -p >out &&
+	test_write_lines 1 2  3 2  3 2  3 2  3 2 >expect &&
+	sed -n -e "s-/.*--" -e "s/^(//p" <out >actual &&
+	test_cmp expect actual
+'
+
 test_done
-- 
2.51.0

[PATCH v2 4/5] add-patch: let options k and K roll over like j and J

[René Scharfe]

Options j and J roll over at the bottom and go to the first undecided
hunk and hunk 1, respectively.  Let options k and K do the same when
they reach the top of the hunk array, so let them go to the last
undecided hunk and the last hunk, respectively, for consistency.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 Documentation/git-add.adoc |  4 ++--
 add-patch.c                | 18 ++++++++++++-----
 t/t3701-add-interactive.sh | 40 +++++++++++++++++++-------------------
 3 files changed, 35 insertions(+), 27 deletions(-)

diff --git a/Documentation/git-add.adoc b/Documentation/git-add.adoc
index 596cdeff93..3116a2cac5 100644
--- a/Documentation/git-add.adoc
+++ b/Documentation/git-add.adoc
@@ -344,8 +344,8 @@ patch::
        / - search for a hunk matching the given regex
        j - go to the next undecided hunk, roll over at the bottom
        J - go to the next hunk, roll over at the bottom
-       k - go to the previous undecided hunk
-       K - go to the previous hunk
+       k - go to the previous undecided hunk, roll over at the top
+       K - go to the previous hunk, roll over at the top
        s - split the current hunk into smaller hunks
        e - manually edit the current hunk
        p - print the current hunk
diff --git a/add-patch.c b/add-patch.c
index da75618dcb..52e881d3b0 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -1399,8 +1399,8 @@ static size_t display_hunks(struct add_p_state *s,
 static const char help_patch_remainder[] =
 N_("j - go to the next undecided hunk, roll over at the bottom\n"
    "J - go to the next hunk, roll over at the bottom\n"
-   "k - go to the previous undecided hunk\n"
-   "K - go to the previous hunk\n"
+   "k - go to the previous undecided hunk, roll over at the top\n"
+   "K - go to the previous hunk, roll over at the top\n"
    "g - select a hunk to go to\n"
    "/ - search for a hunk matching the given regex\n"
    "s - split the current hunk into smaller hunks\n"
@@ -1408,6 +1408,11 @@ N_("j - go to the next undecided hunk, roll over at the bottom\n"
    "p - print the current hunk, 'P' to use the pager\n"
    "? - print help\n");
 
+static size_t dec_mod(size_t a, size_t m)
+{
+	return a > 0 ? a - 1 : m - 1;
+}
+
 static size_t inc_mod(size_t a, size_t m)
 {
 	return a < m - 1 ? a + 1 : 0;
@@ -1450,7 +1455,9 @@ static int patch_update_file(struct add_p_state *s,
 		undecided_next = -1;
 
 		if (file_diff->hunk_nr) {
-			for (i = hunk_index - 1; i >= 0; i--)
+			for (i = dec_mod(hunk_index, file_diff->hunk_nr);
+			     i != hunk_index;
+			     i = dec_mod(i, file_diff->hunk_nr))
 				if (file_diff->hunk[i].use == UNDECIDED_HUNK) {
 					undecided_previous = i;
 					break;
@@ -1492,7 +1499,7 @@ static int patch_update_file(struct add_p_state *s,
 				permitted |= ALLOW_GOTO_PREVIOUS_UNDECIDED_HUNK;
 				strbuf_addstr(&s->buf, ",k");
 			}
-			if (hunk_index) {
+			if (file_diff->hunk_nr > 1) {
 				permitted |= ALLOW_GOTO_PREVIOUS_HUNK;
 				strbuf_addstr(&s->buf, ",K");
 			}
@@ -1584,7 +1591,8 @@ static int patch_update_file(struct add_p_state *s,
 			}
 		} else if (s->answer.buf[0] == 'K') {
 			if (permitted & ALLOW_GOTO_PREVIOUS_HUNK)
-				hunk_index--;
+				hunk_index = dec_mod(hunk_index,
+						     file_diff->hunk_nr);
 			else
 				err(s, _("No previous hunk"));
 		} else if (s->answer.buf[0] == 'J') {
diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index 8086d3da71..385e55c783 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -333,7 +333,7 @@ test_expect_success 'different prompts for mode change/deleted' '
 	sed -n "s/^\(([0-9/]*) Stage .*?\).*/\1/p" actual >actual.filtered &&
 	cat >expect <<-\EOF &&
 	(1/1) Stage deletion [y,n,q,a,d,p,?]?
-	(1/2) Stage mode change [y,n,q,a,d,j,J,g,/,p,?]?
+	(1/2) Stage mode change [y,n,q,a,d,k,K,j,J,g,/,p,?]?
 	(2/2) Stage this hunk [y,n,q,a,d,K,J,g,/,e,p,?]?
 	EOF
 	test_cmp expect actual.filtered
@@ -527,7 +527,7 @@ test_expect_success 'goto hunk 1 with "g 1"' '
 	_10
 	+15
 	_20
-	(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]?_
+	(1/2) Stage this hunk [y,n,q,a,d,k,K,j,J,g,/,e,p,?]?_
 	EOF
 	test_write_lines s y g 1 | git add -p >actual &&
 	tail -n 7 <actual >actual.trimmed &&
@@ -540,7 +540,7 @@ test_expect_success 'goto hunk 1 with "g1"' '
 	_10
 	+15
 	_20
-	(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]?_
+	(1/2) Stage this hunk [y,n,q,a,d,k,K,j,J,g,/,e,p,?]?_
 	EOF
 	test_write_lines s y g1 | git add -p >actual &&
 	tail -n 4 <actual >actual.trimmed &&
@@ -554,7 +554,7 @@ test_expect_success 'navigate to hunk via regex /pattern' '
 	_10
 	+15
 	_20
-	(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]?_
+	(1/2) Stage this hunk [y,n,q,a,d,k,K,j,J,g,/,e,p,?]?_
 	EOF
 	test_write_lines s y /1,2 | git add -p >actual &&
 	tail -n 5 <actual >actual.trimmed &&
@@ -567,7 +567,7 @@ test_expect_success 'navigate to hunk via regex / pattern' '
 	_10
 	+15
 	_20
-	(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]?_
+	(1/2) Stage this hunk [y,n,q,a,d,k,K,j,J,g,/,e,p,?]?_
 	EOF
 	test_write_lines s y / 1,2 | git add -p >actual &&
 	tail -n 4 <actual >actual.trimmed &&
@@ -579,11 +579,11 @@ test_expect_success 'print again the hunk' '
 	tr _ " " >expect <<-EOF &&
 	+15
 	 20
-	(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]? @@ -1,2 +1,3 @@
+	(1/2) Stage this hunk [y,n,q,a,d,k,K,j,J,g,/,e,p,?]? @@ -1,2 +1,3 @@
 	 10
 	+15
 	 20
-	(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]?_
+	(1/2) Stage this hunk [y,n,q,a,d,k,K,j,J,g,/,e,p,?]?_
 	EOF
 	test_write_lines s y g 1 p | git add -p >actual &&
 	tail -n 7 <actual >actual.trimmed &&
@@ -595,11 +595,11 @@ test_expect_success TTY 'print again the hunk (PAGER)' '
 	cat >expect <<-EOF &&
 	<GREEN>+<RESET><GREEN>15<RESET>
 	 20<RESET>
-	<BOLD;BLUE>(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]? <RESET>PAGER <CYAN>@@ -1,2 +1,3 @@<RESET>
+	<BOLD;BLUE>(1/2) Stage this hunk [y,n,q,a,d,k,K,j,J,g,/,e,p,?]? <RESET>PAGER <CYAN>@@ -1,2 +1,3 @@<RESET>
 	PAGER  10<RESET>
 	PAGER <GREEN>+<RESET><GREEN>15<RESET>
 	PAGER  20<RESET>
-	<BOLD;BLUE>(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]? <RESET>
+	<BOLD;BLUE>(1/2) Stage this hunk [y,n,q,a,d,k,K,j,J,g,/,e,p,?]? <RESET>
 	EOF
 	test_write_lines s y g 1 P |
 	(
@@ -802,7 +802,7 @@ test_expect_success 'colors can be overridden' '
 	<BOLD>-old<RESET>
 	<BLUE>+<RESET><BLUE>new<RESET>
 	<CYAN> more-context<RESET>
-	<YELLOW>(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]? <RESET><MAGENTA>@@ -3 +3,2 @@<RESET>
+	<YELLOW>(1/2) Stage this hunk [y,n,q,a,d,k,K,j,J,g,/,e,p,?]? <RESET><MAGENTA>@@ -3 +3,2 @@<RESET>
 	<CYAN> more-context<RESET>
 	<BLUE>+<RESET><BLUE>another-one<RESET>
 	<YELLOW>(2/2) Stage this hunk [y,n,q,a,d,K,J,g,/,e,p,?]? <RESET><MAGENTA>@@ -1,3 +1,3 @@<RESET>
@@ -810,7 +810,7 @@ test_expect_success 'colors can be overridden' '
 	<BOLD>-old<RESET>
 	<BLUE>+new<RESET>
 	<CYAN> more-context<RESET>
-	<YELLOW>(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]? <RESET>
+	<YELLOW>(1/2) Stage this hunk [y,n,q,a,d,k,K,j,J,g,/,e,p,?]? <RESET>
 	EOF
 	test_cmp expect actual
 '
@@ -1354,34 +1354,34 @@ do
 	'
 done
 
-test_expect_success 'option J rolls over' '
+test_expect_success 'options J, K roll over' '
 	test_write_lines a b c d e f g h i >file &&
 	git add file &&
 	test_write_lines X b c d e f g h X >file &&
-	test_write_lines J J q | git add -p >out &&
-	test_write_lines 1 2 1 >expect &&
+	test_write_lines J J K q | git add -p >out &&
+	test_write_lines 1 2 1 2 >expect &&
 	sed -n -e "s-/.*--" -e "s/^(//p" <out >actual &&
 	test_cmp expect actual
 '
 
-test_expect_success 'options y, n, j, e roll over to next undecided (1)' '
+test_expect_success 'options y, n, j, k, e roll over to next undecided (1)' '
 	test_write_lines a b c d e f g h i j k l m n o p q >file &&
 	git add file &&
 	test_write_lines X b c d e f g h X j k l m n o p X >file &&
 	test_set_editor : &&
-	test_write_lines g3 y g3 n g3 j g3 e q | git add -p >out &&
-	test_write_lines 1  3 1  3 1  3 1  3 1 >expect &&
+	test_write_lines g3 y g3 n g3 j g3 e k q | git add -p >out &&
+	test_write_lines 1  3 1  3 1  3 1  3 1 2 >expect &&
 	sed -n -e "s-/.*--" -e "s/^(//p" <out >actual &&
 	test_cmp expect actual
 '
 
-test_expect_success 'options y, n, j, e roll over to next undecided (2)' '
+test_expect_success 'options y, n, j, k, e roll over to next undecided (2)' '
 	test_write_lines a b c d e f g h i j k l m n o p q >file &&
 	git add file &&
 	test_write_lines X b c d e f g h X j k l m n o p X >file &&
 	test_set_editor : &&
-	test_write_lines y g3 y g3 n g3 j g3 e q | git add -p >out &&
-	test_write_lines 1 2  3 2  3 2  3 2  3 2 >expect &&
+	test_write_lines y g3 y g3 n g3 j g3 e g1 k q | git add -p >out &&
+	test_write_lines 1 2  3 2  3 2  3 2  3 2  1 2 >expect &&
 	sed -n -e "s-/.*--" -e "s/^(//p" <out >actual &&
 	test_cmp expect actual
 '
-- 
2.51.0

Re: [PATCH v2 4/5] add-patch: let options k and K roll over like j and J

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

> @@ -1584,7 +1591,8 @@ static int patch_update_file(struct add_p_state *s,
>  			}
>  		} else if (s->answer.buf[0] == 'K') {
>  			if (permitted & ALLOW_GOTO_PREVIOUS_HUNK)
> -				hunk_index--;
> +				hunk_index = dec_mod(hunk_index,
> +						     file_diff->hunk_nr);
>  			else
>  				err(s, _("No previous hunk"));

I was wondering if we want to always allow J and K; even when you
have only one hunk, you can still wrap around to come back to the
current hunk, and that we can do without any extra checking logic.

But it is also OK to require 2 or more hunks to "switch" to the
other hunk, which is what you do with

			if (file_diff->hunk_nr > 1) {
				permitted |= ALLOW_GOTO_PREVIOUS_HUNK;
				strbuf_addstr(&s->buf, ",K");
			}

to require more than 1.  But the error message "No previous hunk"
sounds somewhat awkward.  If user accepts the circular nature of how
we decide what "previous" is, then when we have a single hunk, the
current hunk itself _is_ the previous hunk, but because we insist
that there are at least 2, that interpretation would not work.  With
"wraparound" semantics, "No other hunk(s)", would be a better way to
give the error, no?  The same comment applies to 'J'.

>  		} else if (s->answer.buf[0] == 'J') {

This makes perfect sense, but then, after this post-context we have this:

			if (permitted & ALLOW_GOTO_NEXT_HUNK)
				hunk_index++;
			else
				err(s, _("No next hunk"));

and it sticks out that the post-increment of hunk_index here is not
using inc_mod() for symmetry.

I am wondering if with that updated (I would not say "fixed"), if we
can lose the "oops we overflowed so let's wrap around" belt-and-suspender
code at the beginning of the loop, i.e.

	for (;;) {
		enum {
			ALLOW_GOTO_PREVIOUS_HUNK = 1 << 0,
			...
			ALLOW_EDIT = 1 << 6
		} permitted = 0;

		if (hunk_index >= file_diff->hunk_nr)
			hunk_index = 0;

or if there still are other code that rely on this "oops we
overflowed" adjustment?

Other than that the resulting code with the whole series applied was
a very pleasant read.

Thanks.

Re: [PATCH v2 4/5] add-patch: let options k and K roll over like j and J

[René Scharfe]

On 10/5/25 10:55 PM, Junio C Hamano wrote:
> René Scharfe <l.s.r@web.de> writes:
> 
>> @@ -1584,7 +1591,8 @@ static int patch_update_file(struct add_p_state *s,
>>  			}
>>  		} else if (s->answer.buf[0] == 'K') {
>>  			if (permitted & ALLOW_GOTO_PREVIOUS_HUNK)
>> -				hunk_index--;
>> +				hunk_index = dec_mod(hunk_index,
>> +						     file_diff->hunk_nr);
>>  			else
>>  				err(s, _("No previous hunk"));
> 
> I was wondering if we want to always allow J and K; even when you
> have only one hunk, you can still wrap around to come back to the
> current hunk, and that we can do without any extra checking logic.
> 
> But it is also OK to require 2 or more hunks to "switch" to the
> other hunk, which is what you do with
> 
> 			if (file_diff->hunk_nr > 1) {
> 				permitted |= ALLOW_GOTO_PREVIOUS_HUNK;
> 				strbuf_addstr(&s->buf, ",K");
> 			}
> 
> to require more than 1.  But the error message "No previous hunk"
> sounds somewhat awkward.  If user accepts the circular nature of how
> we decide what "previous" is, then when we have a single hunk, the
> current hunk itself _is_ the previous hunk, but because we insist
> that there are at least 2, that interpretation would not work.  With
> "wraparound" semantics, "No other hunk(s)", would be a better way to
> give the error, no?  The same comment applies to 'J'.

OK.
>>  		} else if (s->answer.buf[0] == 'J') {
> 
> This makes perfect sense, but then, after this post-context we have this:
> 
> 			if (permitted & ALLOW_GOTO_NEXT_HUNK)
> 				hunk_index++;
> 			else
> 				err(s, _("No next hunk"));
> 
> and it sticks out that the post-increment of hunk_index here is not
> using inc_mod() for symmetry.

This symmetry _is_ tantalizing.  Had the call originally, removed it
because it was unnecessary and didn't fit the narrative.
> I am wondering if with that updated (I would not say "fixed"), if we
> can lose the "oops we overflowed so let's wrap around" belt-and-suspender
> code at the beginning of the loop, i.e.
> 
> 	for (;;) {
> 		enum {
> 			ALLOW_GOTO_PREVIOUS_HUNK = 1 << 0,
> 			...
> 			ALLOW_EDIT = 1 << 6
> 		} permitted = 0;
> 
> 		if (hunk_index >= file_diff->hunk_nr)
> 			hunk_index = 0;
> 
> or if there still are other code that rely on this "oops we
> overflowed" adjustment?
Good question, gave me the idea that a and d should roll over as well.

Other than that there's just the so-called soft_increment, which would
need something like this:

diff --git a/add-patch.c b/add-patch.c
index b0389c5d5b..59a9eb586d 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -1546,8 +1546,7 @@ 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;
+			hunk_index = undecided_next < 0 ? 0 : undecided_next;
 		} else if (ch == 'n') {
 			hunk->use = SKIP_HUNK;
 			goto soft_increment;

Or undecided_next could be set to 0 before the if/else cascade, then
this becomes a simple assignment and we can get rid of the goto.

Couldn't find a way to remove the back-to-square-1 check that would be
significantly better overall and thus worth the hassle, though.

René

[PATCH v2 5/5] add-patch: reset "permitted" at loop start

[René Scharfe]

Don't accumulate allowed options from any visited hunks, start fresh at
the top of the loop instead and only record the allowed options for the
current hunk.

Reported-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: René Scharfe <l.s.r@web.de>
---
 add-patch.c                | 19 ++++++++++---------
 t/t3701-add-interactive.sh | 14 ++++++++++++++
 2 files changed, 24 insertions(+), 9 deletions(-)

diff --git a/add-patch.c b/add-patch.c
index 52e881d3b0..7b489d0a75 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -1428,15 +1428,6 @@ static int patch_update_file(struct add_p_state *s,
 	struct child_process cp = CHILD_PROCESS_INIT;
 	int colored = !!s->colored.len, quit = 0, use_pager = 0;
 	enum prompt_mode_type prompt_mode_type;
-	enum {
-		ALLOW_GOTO_PREVIOUS_HUNK = 1 << 0,
-		ALLOW_GOTO_PREVIOUS_UNDECIDED_HUNK = 1 << 1,
-		ALLOW_GOTO_NEXT_HUNK = 1 << 2,
-		ALLOW_GOTO_NEXT_UNDECIDED_HUNK = 1 << 3,
-		ALLOW_SEARCH_AND_GOTO = 1 << 4,
-		ALLOW_SPLIT = 1 << 5,
-		ALLOW_EDIT = 1 << 6
-	} permitted = 0;
 
 	/* Empty added files have no hunks */
 	if (!file_diff->hunk_nr && !file_diff->added)
@@ -1446,6 +1437,16 @@ static int patch_update_file(struct add_p_state *s,
 	render_diff_header(s, file_diff, colored, &s->buf);
 	fputs(s->buf.buf, stdout);
 	for (;;) {
+		enum {
+			ALLOW_GOTO_PREVIOUS_HUNK = 1 << 0,
+			ALLOW_GOTO_PREVIOUS_UNDECIDED_HUNK = 1 << 1,
+			ALLOW_GOTO_NEXT_HUNK = 1 << 2,
+			ALLOW_GOTO_NEXT_UNDECIDED_HUNK = 1 << 3,
+			ALLOW_SEARCH_AND_GOTO = 1 << 4,
+			ALLOW_SPLIT = 1 << 5,
+			ALLOW_EDIT = 1 << 6
+		} permitted = 0;
+
 		if (hunk_index >= file_diff->hunk_nr)
 			hunk_index = 0;
 		hunk = file_diff->hunk_nr
diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index 385e55c783..8c24a76e59 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -1386,4 +1386,18 @@ test_expect_success 'options y, n, j, k, e roll over to next undecided (2)' '
 	test_cmp expect actual
 '
 
+test_expect_success 'invalid option s is rejected' '
+	test_write_lines a b c d e f g h i j k >file &&
+	git add file &&
+	test_write_lines X b X d e f g h i j X >file &&
+	test_write_lines j s q | git add -p >out &&
+	sed -ne "s/ @@.*//" -e "s/ \$//" -e "/^(/p" <out >actual &&
+	cat >expect <<-EOF &&
+	(1/2) Stage this hunk [y,n,q,a,d,k,K,j,J,g,/,s,e,p,?]?
+	(2/2) Stage this hunk [y,n,q,a,d,k,K,j,J,g,/,e,p,?]? Sorry, cannot split this hunk
+	(2/2) Stage this hunk [y,n,q,a,d,k,K,j,J,g,/,e,p,?]?
+	EOF
+	test_cmp expect actual
+'
+
 test_done
-- 
2.51.0

[PATCH v3 0/6] add-patch: roll over to next undecided hunk

[René Scharfe]

Changes since v1:
- added patch 5 for a and d
- made error messages direction-neutral
- removed stray "only" from commit message of patch 2

  add-patch: improve help for options j, J, k, and K
  add-patch: document that option J rolls over
  add-patch: let options y, n, j, and e roll over to next undecided
  add-patch: let options k and K roll over like j and J
  add-patch: let options a and d roll over like y and n
  add-patch: reset "permitted" at loop start

 Documentation/git-add.adoc |  8 ++--
 add-patch.c                | 75 ++++++++++++++++++++++++++-----------
 t/t3701-add-interactive.sh | 76 ++++++++++++++++++++++++++++++--------
 3 files changed, 118 insertions(+), 41 deletions(-)

Interdiff against v2:
diff --git a/add-patch.c b/add-patch.c
index 7b489d0a75..45839ceac5 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -1418,6 +1418,17 @@ static size_t inc_mod(size_t a, size_t m)
 	return a < m - 1 ? a + 1 : 0;
 }
 
+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++) {
+		if (file_diff->hunk[i].use == UNDECIDED_HUNK) {
+			*idx = i;
+			return true;
+		}
+	}
+	return false;
+}
+
 static int patch_update_file(struct add_p_state *s,
 			     struct file_diff *file_diff)
 {
@@ -1573,6 +1584,8 @@ static int patch_update_file(struct add_p_state *s,
 					if (hunk->use == UNDECIDED_HUNK)
 						hunk->use = USE_HUNK;
 				}
+				if (!get_first_undecided(file_diff, &hunk_index))
+					hunk_index = 0;
 			} else if (hunk->use == UNDECIDED_HUNK) {
 				hunk->use = USE_HUNK;
 			}
@@ -1583,6 +1596,8 @@ static int patch_update_file(struct add_p_state *s,
 					if (hunk->use == UNDECIDED_HUNK)
 						hunk->use = SKIP_HUNK;
 				}
+				if (!get_first_undecided(file_diff, &hunk_index))
+					hunk_index = 0;
 			} else if (hunk->use == UNDECIDED_HUNK) {
 				hunk->use = SKIP_HUNK;
 			}
@@ -1595,22 +1610,22 @@ static int patch_update_file(struct add_p_state *s,
 				hunk_index = dec_mod(hunk_index,
 						     file_diff->hunk_nr);
 			else
-				err(s, _("No previous hunk"));
+				err(s, _("No other hunk"));
 		} else if (s->answer.buf[0] == 'J') {
 			if (permitted & ALLOW_GOTO_NEXT_HUNK)
 				hunk_index++;
 			else
-				err(s, _("No next hunk"));
+				err(s, _("No other hunk"));
 		} else if (s->answer.buf[0] == 'k') {
 			if (permitted & ALLOW_GOTO_PREVIOUS_UNDECIDED_HUNK)
 				hunk_index = undecided_previous;
 			else
-				err(s, _("No previous hunk"));
+				err(s, _("No other undecided hunk"));
 		} else if (s->answer.buf[0] == 'j') {
 			if (permitted & ALLOW_GOTO_NEXT_UNDECIDED_HUNK)
 				hunk_index = undecided_next;
 			else
-				err(s, _("No next hunk"));
+				err(s, _("No other undecided hunk"));
 		} else if (s->answer.buf[0] == 'g') {
 			char *pend;
 			unsigned long response;
diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index 8c24a76e59..403aaee356 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -1364,24 +1364,24 @@ test_expect_success 'options J, K roll over' '
 	test_cmp expect actual
 '
 
-test_expect_success 'options y, n, j, k, e roll over to next undecided (1)' '
+test_expect_success 'options y, n, a, d, j, k, e roll over to next undecided (1)' '
 	test_write_lines a b c d e f g h i j k l m n o p q >file &&
 	git add file &&
 	test_write_lines X b c d e f g h X j k l m n o p X >file &&
 	test_set_editor : &&
-	test_write_lines g3 y g3 n g3 j g3 e k q | git add -p >out &&
-	test_write_lines 1  3 1  3 1  3 1  3 1 2 >expect &&
+	test_write_lines g3 y g3 n g3 a g3 d g3 j g3 e k q | git add -p >out &&
+	test_write_lines 1  3 1  3 1  3 1  3 1  3 1  3 1 2 >expect &&
 	sed -n -e "s-/.*--" -e "s/^(//p" <out >actual &&
 	test_cmp expect actual
 '
 
-test_expect_success 'options y, n, j, k, e roll over to next undecided (2)' '
+test_expect_success 'options y, n, a, d, j, k, e roll over to next undecided (2)' '
 	test_write_lines a b c d e f g h i j k l m n o p q >file &&
 	git add file &&
 	test_write_lines X b c d e f g h X j k l m n o p X >file &&
 	test_set_editor : &&
-	test_write_lines y g3 y g3 n g3 j g3 e g1 k q | git add -p >out &&
-	test_write_lines 1 2  3 2  3 2  3 2  3 2  1 2 >expect &&
+	test_write_lines y g3 y g3 n g3 a g3 d g3 j g3 e g1 k q | git add -p >out &&
+	test_write_lines 1 2  3 2  3 2  3 2  3 2  3 2  3 2  1 2 >expect &&
 	sed -n -e "s-/.*--" -e "s/^(//p" <out >actual &&
 	test_cmp expect actual
 '
-- 
2.51.0

[PATCH v3 1/6] add-patch: improve help for options j, J, k, and K

[René Scharfe]

The options j, J, k, and K don't affect the status of the current hunk.
They just go to a different one.  This is true whether the current hunk
is undecided or not.  Avoid misunderstanding by no longer mentioning
the current hunk explicitly in their help texts.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 Documentation/git-add.adoc | 8 ++++----
 add-patch.c                | 8 ++++----
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/Documentation/git-add.adoc b/Documentation/git-add.adoc
index ad629c46c5..3266ccf105 100644
--- a/Documentation/git-add.adoc
+++ b/Documentation/git-add.adoc
@@ -342,10 +342,10 @@ patch::
        d - do not stage this hunk or any of the later hunks in the file
        g - select a hunk to go to
        / - search for a hunk matching the given regex
-       j - leave this hunk undecided, see next undecided hunk
-       J - leave this hunk undecided, see next hunk
-       k - leave this hunk undecided, see previous undecided hunk
-       K - leave this hunk undecided, see previous hunk
+       j - go to the next undecided hunk
+       J - go to the next hunk
+       k - go to the previous undecided hunk
+       K - go to the previous hunk
        s - split the current hunk into smaller hunks
        e - manually edit the current hunk
        p - print the current hunk
diff --git a/add-patch.c b/add-patch.c
index b0389c5d5b..912266a3f8 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -1397,10 +1397,10 @@ static size_t display_hunks(struct add_p_state *s,
 }
 
 static const char help_patch_remainder[] =
-N_("j - leave this hunk undecided, see next undecided hunk\n"
-   "J - leave this hunk undecided, see next hunk\n"
-   "k - leave this hunk undecided, see previous undecided hunk\n"
-   "K - leave this hunk undecided, see previous hunk\n"
+N_("j - go to the next undecided hunk\n"
+   "J - go to the next hunk\n"
+   "k - go to the previous undecided hunk\n"
+   "K - go to the previous hunk\n"
    "g - select a hunk to go to\n"
    "/ - search for a hunk matching the given regex\n"
    "s - split the current hunk into smaller hunks\n"
-- 
2.51.0

[PATCH v3 2/6] add-patch: document that option J rolls over

[René Scharfe]

The variable "permitted" is not reset after moving to a different hunk,
so it only accumulates permission and doesn't necessarily reflect those
of the current hunk.  This may be a bug, but is actually useful with the
option J, which can be used at the last hunk to roll over to the first
hunk.  Make this particular behavior official.

Also adjust the error message, as it will only be shown if there's just
a single hunk.

Suggested-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: René Scharfe <l.s.r@web.de>
---
 Documentation/git-add.adoc |  2 +-
 add-patch.c                |  6 +++---
 t/t3701-add-interactive.sh | 18 ++++++++++++++----
 3 files changed, 18 insertions(+), 8 deletions(-)

diff --git a/Documentation/git-add.adoc b/Documentation/git-add.adoc
index 3266ccf105..5c05a3a7f9 100644
--- a/Documentation/git-add.adoc
+++ b/Documentation/git-add.adoc
@@ -343,7 +343,7 @@ patch::
        g - select a hunk to go to
        / - search for a hunk matching the given regex
        j - go to the next undecided hunk
-       J - go to the next hunk
+       J - go to the next hunk, roll over at the bottom
        k - go to the previous undecided hunk
        K - go to the previous hunk
        s - split the current hunk into smaller hunks
diff --git a/add-patch.c b/add-patch.c
index 912266a3f8..1f466ec9c0 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -1398,7 +1398,7 @@ static size_t display_hunks(struct add_p_state *s,
 
 static const char help_patch_remainder[] =
 N_("j - go to the next undecided hunk\n"
-   "J - go to the next hunk\n"
+   "J - go to the next hunk, roll over at the bottom\n"
    "k - go to the previous undecided hunk\n"
    "K - go to the previous hunk\n"
    "g - select a hunk to go to\n"
@@ -1493,7 +1493,7 @@ static int patch_update_file(struct add_p_state *s,
 				permitted |= ALLOW_GOTO_NEXT_UNDECIDED_HUNK;
 				strbuf_addstr(&s->buf, ",j");
 			}
-			if (hunk_index + 1 < file_diff->hunk_nr) {
+			if (file_diff->hunk_nr > 1) {
 				permitted |= ALLOW_GOTO_NEXT_HUNK;
 				strbuf_addstr(&s->buf, ",J");
 			}
@@ -1584,7 +1584,7 @@ static int patch_update_file(struct add_p_state *s,
 			if (permitted & ALLOW_GOTO_NEXT_HUNK)
 				hunk_index++;
 			else
-				err(s, _("No next hunk"));
+				err(s, _("No other hunk"));
 		} else if (s->answer.buf[0] == 'k') {
 			if (permitted & ALLOW_GOTO_PREVIOUS_UNDECIDED_HUNK)
 				hunk_index = undecided_previous;
diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index d9fe289a7a..d5d2e120ab 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -334,7 +334,7 @@ test_expect_success 'different prompts for mode change/deleted' '
 	cat >expect <<-\EOF &&
 	(1/1) Stage deletion [y,n,q,a,d,p,?]?
 	(1/2) Stage mode change [y,n,q,a,d,j,J,g,/,p,?]?
-	(2/2) Stage this hunk [y,n,q,a,d,K,g,/,e,p,?]?
+	(2/2) Stage this hunk [y,n,q,a,d,K,J,g,/,e,p,?]?
 	EOF
 	test_cmp expect actual.filtered
 '
@@ -521,7 +521,7 @@ test_expect_success 'split hunk setup' '
 test_expect_success 'goto hunk 1 with "g 1"' '
 	test_when_finished "git reset" &&
 	tr _ " " >expect <<-EOF &&
-	(2/2) Stage this hunk [y,n,q,a,d,K,g,/,e,p,?]? + 1:  -1,2 +1,3          +15
+	(2/2) Stage this hunk [y,n,q,a,d,K,J,g,/,e,p,?]? + 1:  -1,2 +1,3          +15
 	_ 2:  -2,4 +3,8          +21
 	go to which hunk? @@ -1,2 +1,3 @@
 	_10
@@ -550,7 +550,7 @@ test_expect_success 'goto hunk 1 with "g1"' '
 test_expect_success 'navigate to hunk via regex /pattern' '
 	test_when_finished "git reset" &&
 	tr _ " " >expect <<-EOF &&
-	(2/2) Stage this hunk [y,n,q,a,d,K,g,/,e,p,?]? @@ -1,2 +1,3 @@
+	(2/2) Stage this hunk [y,n,q,a,d,K,J,g,/,e,p,?]? @@ -1,2 +1,3 @@
 	_10
 	+15
 	_20
@@ -805,7 +805,7 @@ test_expect_success 'colors can be overridden' '
 	<YELLOW>(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]? <RESET><MAGENTA>@@ -3 +3,2 @@<RESET>
 	<CYAN> more-context<RESET>
 	<BLUE>+<RESET><BLUE>another-one<RESET>
-	<YELLOW>(2/2) Stage this hunk [y,n,q,a,d,K,g,/,e,p,?]? <RESET><MAGENTA>@@ -1,3 +1,3 @@<RESET>
+	<YELLOW>(2/2) Stage this hunk [y,n,q,a,d,K,J,g,/,e,p,?]? <RESET><MAGENTA>@@ -1,3 +1,3 @@<RESET>
 	<CYAN> context<RESET>
 	<BOLD>-old<RESET>
 	<BLUE>+new<RESET>
@@ -1354,4 +1354,14 @@ do
 	'
 done
 
+test_expect_success 'option J rolls over' '
+	test_write_lines a b c d e f g h i >file &&
+	git add file &&
+	test_write_lines X b c d e f g h X >file &&
+	test_write_lines J J q | git add -p >out &&
+	test_write_lines 1 2 1 >expect &&
+	sed -n -e "s-/.*--" -e "s/^(//p" <out >actual &&
+	test_cmp expect actual
+'
+
 test_done
-- 
2.51.0

[PATCH v3 3/6] add-patch: let options y, n, j, and e roll over to next undecided

[René Scharfe]

The options y, n, and e mark the current hunk as decided.  If there's
another undecided hunk towards the bottom of the hunk array they go
there.  If there isn't, but there is another undecided hunk towards the
top then they go to the very first hunk, no matter if it has already
been decided on.

The option j does basically the same move.  Technically it is not
allowed if there's no undecided hunk towards the bottom, but the
variable "permitted" is never reset, so this permission is retained
from the very first hunk.  That may a bug, but this behavior is at
least consistent with y, n, and e and arguably more useful than
refusing to move.

Improve the roll-over behavior of these four options by moving to the
first undecided hunk instead of hunk 1, consistent with what they do
when not rolling over.

Also adjust the error message for j, as it will only be shown if
there's no other undecided hunk in either direction.

Reported-by: Windl, Ulrich <u.windl@ukr.de>
Signed-off-by: René Scharfe <l.s.r@web.de>
---
 Documentation/git-add.adoc |  2 +-
 add-patch.c                | 13 ++++++++++---
 t/t3701-add-interactive.sh | 22 ++++++++++++++++++++++
 3 files changed, 33 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-add.adoc b/Documentation/git-add.adoc
index 5c05a3a7f9..596cdeff93 100644
--- a/Documentation/git-add.adoc
+++ b/Documentation/git-add.adoc
@@ -342,7 +342,7 @@ patch::
        d - do not stage this hunk or any of the later hunks in the file
        g - select a hunk to go to
        / - search for a hunk matching the given regex
-       j - go to the next undecided hunk
+       j - go to the next undecided hunk, roll over at the bottom
        J - go to the next hunk, roll over at the bottom
        k - go to the previous undecided hunk
        K - go to the previous hunk
diff --git a/add-patch.c b/add-patch.c
index 1f466ec9c0..106bfcb275 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -1397,7 +1397,7 @@ static size_t display_hunks(struct add_p_state *s,
 }
 
 static const char help_patch_remainder[] =
-N_("j - go to the next undecided hunk\n"
+N_("j - go to the next undecided hunk, roll over at the bottom\n"
    "J - go to the next hunk, roll over at the bottom\n"
    "k - go to the previous undecided hunk\n"
    "K - go to the previous hunk\n"
@@ -1408,6 +1408,11 @@ N_("j - go to the next undecided hunk\n"
    "p - print the current hunk, 'P' to use the pager\n"
    "? - print help\n");
 
+static size_t inc_mod(size_t a, size_t m)
+{
+	return a < m - 1 ? a + 1 : 0;
+}
+
 static int patch_update_file(struct add_p_state *s,
 			     struct file_diff *file_diff)
 {
@@ -1451,7 +1456,9 @@ static int patch_update_file(struct add_p_state *s,
 					break;
 				}
 
-			for (i = hunk_index + 1; i < file_diff->hunk_nr; i++)
+			for (i = inc_mod(hunk_index, file_diff->hunk_nr);
+			     i != hunk_index;
+			     i = inc_mod(i, file_diff->hunk_nr))
 				if (file_diff->hunk[i].use == UNDECIDED_HUNK) {
 					undecided_next = i;
 					break;
@@ -1594,7 +1601,7 @@ static int patch_update_file(struct add_p_state *s,
 			if (permitted & ALLOW_GOTO_NEXT_UNDECIDED_HUNK)
 				hunk_index = undecided_next;
 			else
-				err(s, _("No next hunk"));
+				err(s, _("No other undecided hunk"));
 		} else if (s->answer.buf[0] == 'g') {
 			char *pend;
 			unsigned long response;
diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index d5d2e120ab..8086d3da71 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -1364,4 +1364,26 @@ test_expect_success 'option J rolls over' '
 	test_cmp expect actual
 '
 
+test_expect_success 'options y, n, j, e roll over to next undecided (1)' '
+	test_write_lines a b c d e f g h i j k l m n o p q >file &&
+	git add file &&
+	test_write_lines X b c d e f g h X j k l m n o p X >file &&
+	test_set_editor : &&
+	test_write_lines g3 y g3 n g3 j g3 e q | git add -p >out &&
+	test_write_lines 1  3 1  3 1  3 1  3 1 >expect &&
+	sed -n -e "s-/.*--" -e "s/^(//p" <out >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'options y, n, j, e roll over to next undecided (2)' '
+	test_write_lines a b c d e f g h i j k l m n o p q >file &&
+	git add file &&
+	test_write_lines X b c d e f g h X j k l m n o p X >file &&
+	test_set_editor : &&
+	test_write_lines y g3 y g3 n g3 j g3 e q | git add -p >out &&
+	test_write_lines 1 2  3 2  3 2  3 2  3 2 >expect &&
+	sed -n -e "s-/.*--" -e "s/^(//p" <out >actual &&
+	test_cmp expect actual
+'
+
 test_done
-- 
2.51.0

[PATCH v3 4/6] add-patch: let options k and K roll over like j and J

[René Scharfe]

Options j and J roll over at the bottom and go to the first undecided
hunk and hunk 1, respectively.  Let options k and K do the same when
they reach the top of the hunk array, so let them go to the last
undecided hunk and the last hunk, respectively, for consistency.  Also
use the same direction-neutral error messages.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 Documentation/git-add.adoc |  4 ++--
 add-patch.c                | 22 ++++++++++++++-------
 t/t3701-add-interactive.sh | 40 +++++++++++++++++++-------------------
 3 files changed, 37 insertions(+), 29 deletions(-)

diff --git a/Documentation/git-add.adoc b/Documentation/git-add.adoc
index 596cdeff93..3116a2cac5 100644
--- a/Documentation/git-add.adoc
+++ b/Documentation/git-add.adoc
@@ -344,8 +344,8 @@ patch::
        / - search for a hunk matching the given regex
        j - go to the next undecided hunk, roll over at the bottom
        J - go to the next hunk, roll over at the bottom
-       k - go to the previous undecided hunk
-       K - go to the previous hunk
+       k - go to the previous undecided hunk, roll over at the top
+       K - go to the previous hunk, roll over at the top
        s - split the current hunk into smaller hunks
        e - manually edit the current hunk
        p - print the current hunk
diff --git a/add-patch.c b/add-patch.c
index 106bfcb275..4f314c16ec 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -1399,8 +1399,8 @@ static size_t display_hunks(struct add_p_state *s,
 static const char help_patch_remainder[] =
 N_("j - go to the next undecided hunk, roll over at the bottom\n"
    "J - go to the next hunk, roll over at the bottom\n"
-   "k - go to the previous undecided hunk\n"
-   "K - go to the previous hunk\n"
+   "k - go to the previous undecided hunk, roll over at the top\n"
+   "K - go to the previous hunk, roll over at the top\n"
    "g - select a hunk to go to\n"
    "/ - search for a hunk matching the given regex\n"
    "s - split the current hunk into smaller hunks\n"
@@ -1408,6 +1408,11 @@ N_("j - go to the next undecided hunk, roll over at the bottom\n"
    "p - print the current hunk, 'P' to use the pager\n"
    "? - print help\n");
 
+static size_t dec_mod(size_t a, size_t m)
+{
+	return a > 0 ? a - 1 : m - 1;
+}
+
 static size_t inc_mod(size_t a, size_t m)
 {
 	return a < m - 1 ? a + 1 : 0;
@@ -1450,7 +1455,9 @@ static int patch_update_file(struct add_p_state *s,
 		undecided_next = -1;
 
 		if (file_diff->hunk_nr) {
-			for (i = hunk_index - 1; i >= 0; i--)
+			for (i = dec_mod(hunk_index, file_diff->hunk_nr);
+			     i != hunk_index;
+			     i = dec_mod(i, file_diff->hunk_nr))
 				if (file_diff->hunk[i].use == UNDECIDED_HUNK) {
 					undecided_previous = i;
 					break;
@@ -1492,7 +1499,7 @@ static int patch_update_file(struct add_p_state *s,
 				permitted |= ALLOW_GOTO_PREVIOUS_UNDECIDED_HUNK;
 				strbuf_addstr(&s->buf, ",k");
 			}
-			if (hunk_index) {
+			if (file_diff->hunk_nr > 1) {
 				permitted |= ALLOW_GOTO_PREVIOUS_HUNK;
 				strbuf_addstr(&s->buf, ",K");
 			}
@@ -1584,9 +1591,10 @@ static int patch_update_file(struct add_p_state *s,
 			}
 		} else if (s->answer.buf[0] == 'K') {
 			if (permitted & ALLOW_GOTO_PREVIOUS_HUNK)
-				hunk_index--;
+				hunk_index = dec_mod(hunk_index,
+						     file_diff->hunk_nr);
 			else
-				err(s, _("No previous hunk"));
+				err(s, _("No other hunk"));
 		} else if (s->answer.buf[0] == 'J') {
 			if (permitted & ALLOW_GOTO_NEXT_HUNK)
 				hunk_index++;
@@ -1596,7 +1604,7 @@ static int patch_update_file(struct add_p_state *s,
 			if (permitted & ALLOW_GOTO_PREVIOUS_UNDECIDED_HUNK)
 				hunk_index = undecided_previous;
 			else
-				err(s, _("No previous hunk"));
+				err(s, _("No other undecided hunk"));
 		} else if (s->answer.buf[0] == 'j') {
 			if (permitted & ALLOW_GOTO_NEXT_UNDECIDED_HUNK)
 				hunk_index = undecided_next;
diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index 8086d3da71..385e55c783 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -333,7 +333,7 @@ test_expect_success 'different prompts for mode change/deleted' '
 	sed -n "s/^\(([0-9/]*) Stage .*?\).*/\1/p" actual >actual.filtered &&
 	cat >expect <<-\EOF &&
 	(1/1) Stage deletion [y,n,q,a,d,p,?]?
-	(1/2) Stage mode change [y,n,q,a,d,j,J,g,/,p,?]?
+	(1/2) Stage mode change [y,n,q,a,d,k,K,j,J,g,/,p,?]?
 	(2/2) Stage this hunk [y,n,q,a,d,K,J,g,/,e,p,?]?
 	EOF
 	test_cmp expect actual.filtered
@@ -527,7 +527,7 @@ test_expect_success 'goto hunk 1 with "g 1"' '
 	_10
 	+15
 	_20
-	(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]?_
+	(1/2) Stage this hunk [y,n,q,a,d,k,K,j,J,g,/,e,p,?]?_
 	EOF
 	test_write_lines s y g 1 | git add -p >actual &&
 	tail -n 7 <actual >actual.trimmed &&
@@ -540,7 +540,7 @@ test_expect_success 'goto hunk 1 with "g1"' '
 	_10
 	+15
 	_20
-	(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]?_
+	(1/2) Stage this hunk [y,n,q,a,d,k,K,j,J,g,/,e,p,?]?_
 	EOF
 	test_write_lines s y g1 | git add -p >actual &&
 	tail -n 4 <actual >actual.trimmed &&
@@ -554,7 +554,7 @@ test_expect_success 'navigate to hunk via regex /pattern' '
 	_10
 	+15
 	_20
-	(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]?_
+	(1/2) Stage this hunk [y,n,q,a,d,k,K,j,J,g,/,e,p,?]?_
 	EOF
 	test_write_lines s y /1,2 | git add -p >actual &&
 	tail -n 5 <actual >actual.trimmed &&
@@ -567,7 +567,7 @@ test_expect_success 'navigate to hunk via regex / pattern' '
 	_10
 	+15
 	_20
-	(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]?_
+	(1/2) Stage this hunk [y,n,q,a,d,k,K,j,J,g,/,e,p,?]?_
 	EOF
 	test_write_lines s y / 1,2 | git add -p >actual &&
 	tail -n 4 <actual >actual.trimmed &&
@@ -579,11 +579,11 @@ test_expect_success 'print again the hunk' '
 	tr _ " " >expect <<-EOF &&
 	+15
 	 20
-	(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]? @@ -1,2 +1,3 @@
+	(1/2) Stage this hunk [y,n,q,a,d,k,K,j,J,g,/,e,p,?]? @@ -1,2 +1,3 @@
 	 10
 	+15
 	 20
-	(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]?_
+	(1/2) Stage this hunk [y,n,q,a,d,k,K,j,J,g,/,e,p,?]?_
 	EOF
 	test_write_lines s y g 1 p | git add -p >actual &&
 	tail -n 7 <actual >actual.trimmed &&
@@ -595,11 +595,11 @@ test_expect_success TTY 'print again the hunk (PAGER)' '
 	cat >expect <<-EOF &&
 	<GREEN>+<RESET><GREEN>15<RESET>
 	 20<RESET>
-	<BOLD;BLUE>(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]? <RESET>PAGER <CYAN>@@ -1,2 +1,3 @@<RESET>
+	<BOLD;BLUE>(1/2) Stage this hunk [y,n,q,a,d,k,K,j,J,g,/,e,p,?]? <RESET>PAGER <CYAN>@@ -1,2 +1,3 @@<RESET>
 	PAGER  10<RESET>
 	PAGER <GREEN>+<RESET><GREEN>15<RESET>
 	PAGER  20<RESET>
-	<BOLD;BLUE>(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]? <RESET>
+	<BOLD;BLUE>(1/2) Stage this hunk [y,n,q,a,d,k,K,j,J,g,/,e,p,?]? <RESET>
 	EOF
 	test_write_lines s y g 1 P |
 	(
@@ -802,7 +802,7 @@ test_expect_success 'colors can be overridden' '
 	<BOLD>-old<RESET>
 	<BLUE>+<RESET><BLUE>new<RESET>
 	<CYAN> more-context<RESET>
-	<YELLOW>(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]? <RESET><MAGENTA>@@ -3 +3,2 @@<RESET>
+	<YELLOW>(1/2) Stage this hunk [y,n,q,a,d,k,K,j,J,g,/,e,p,?]? <RESET><MAGENTA>@@ -3 +3,2 @@<RESET>
 	<CYAN> more-context<RESET>
 	<BLUE>+<RESET><BLUE>another-one<RESET>
 	<YELLOW>(2/2) Stage this hunk [y,n,q,a,d,K,J,g,/,e,p,?]? <RESET><MAGENTA>@@ -1,3 +1,3 @@<RESET>
@@ -810,7 +810,7 @@ test_expect_success 'colors can be overridden' '
 	<BOLD>-old<RESET>
 	<BLUE>+new<RESET>
 	<CYAN> more-context<RESET>
-	<YELLOW>(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]? <RESET>
+	<YELLOW>(1/2) Stage this hunk [y,n,q,a,d,k,K,j,J,g,/,e,p,?]? <RESET>
 	EOF
 	test_cmp expect actual
 '
@@ -1354,34 +1354,34 @@ do
 	'
 done
 
-test_expect_success 'option J rolls over' '
+test_expect_success 'options J, K roll over' '
 	test_write_lines a b c d e f g h i >file &&
 	git add file &&
 	test_write_lines X b c d e f g h X >file &&
-	test_write_lines J J q | git add -p >out &&
-	test_write_lines 1 2 1 >expect &&
+	test_write_lines J J K q | git add -p >out &&
+	test_write_lines 1 2 1 2 >expect &&
 	sed -n -e "s-/.*--" -e "s/^(//p" <out >actual &&
 	test_cmp expect actual
 '
 
-test_expect_success 'options y, n, j, e roll over to next undecided (1)' '
+test_expect_success 'options y, n, j, k, e roll over to next undecided (1)' '
 	test_write_lines a b c d e f g h i j k l m n o p q >file &&
 	git add file &&
 	test_write_lines X b c d e f g h X j k l m n o p X >file &&
 	test_set_editor : &&
-	test_write_lines g3 y g3 n g3 j g3 e q | git add -p >out &&
-	test_write_lines 1  3 1  3 1  3 1  3 1 >expect &&
+	test_write_lines g3 y g3 n g3 j g3 e k q | git add -p >out &&
+	test_write_lines 1  3 1  3 1  3 1  3 1 2 >expect &&
 	sed -n -e "s-/.*--" -e "s/^(//p" <out >actual &&
 	test_cmp expect actual
 '
 
-test_expect_success 'options y, n, j, e roll over to next undecided (2)' '
+test_expect_success 'options y, n, j, k, e roll over to next undecided (2)' '
 	test_write_lines a b c d e f g h i j k l m n o p q >file &&
 	git add file &&
 	test_write_lines X b c d e f g h X j k l m n o p X >file &&
 	test_set_editor : &&
-	test_write_lines y g3 y g3 n g3 j g3 e q | git add -p >out &&
-	test_write_lines 1 2  3 2  3 2  3 2  3 2 >expect &&
+	test_write_lines y g3 y g3 n g3 j g3 e g1 k q | git add -p >out &&
+	test_write_lines 1 2  3 2  3 2  3 2  3 2  1 2 >expect &&
 	sed -n -e "s-/.*--" -e "s/^(//p" <out >actual &&
 	test_cmp expect actual
 '
-- 
2.51.0

[PATCH v3 5/6] add-patch: let options a and d roll over like y and n

[René Scharfe]

Options a and d stage and unstage all undecided hunks towards the bottom
of the array of hunks, respectively, and then roll over to the very
first hunk.  The first part is similar to y and n if the current hunk is
the last one in the array, but they roll over to the next undecided
hunk if there is any.  That's more useful; do it for a and d as well.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 add-patch.c                | 15 +++++++++++++++
 t/t3701-add-interactive.sh | 12 ++++++------
 2 files changed, 21 insertions(+), 6 deletions(-)

diff --git a/add-patch.c b/add-patch.c
index 4f314c16ec..6da13a78b5 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -1418,6 +1418,17 @@ static size_t inc_mod(size_t a, size_t m)
 	return a < m - 1 ? a + 1 : 0;
 }
 
+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++) {
+		if (file_diff->hunk[i].use == UNDECIDED_HUNK) {
+			*idx = i;
+			return true;
+		}
+	}
+	return false;
+}
+
 static int patch_update_file(struct add_p_state *s,
 			     struct file_diff *file_diff)
 {
@@ -1572,6 +1583,8 @@ static int patch_update_file(struct add_p_state *s,
 					if (hunk->use == UNDECIDED_HUNK)
 						hunk->use = USE_HUNK;
 				}
+				if (!get_first_undecided(file_diff, &hunk_index))
+					hunk_index = 0;
 			} else if (hunk->use == UNDECIDED_HUNK) {
 				hunk->use = USE_HUNK;
 			}
@@ -1582,6 +1595,8 @@ static int patch_update_file(struct add_p_state *s,
 					if (hunk->use == UNDECIDED_HUNK)
 						hunk->use = SKIP_HUNK;
 				}
+				if (!get_first_undecided(file_diff, &hunk_index))
+					hunk_index = 0;
 			} else if (hunk->use == UNDECIDED_HUNK) {
 				hunk->use = SKIP_HUNK;
 			}
diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index 385e55c783..9d81b0542e 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -1364,24 +1364,24 @@ test_expect_success 'options J, K roll over' '
 	test_cmp expect actual
 '
 
-test_expect_success 'options y, n, j, k, e roll over to next undecided (1)' '
+test_expect_success 'options y, n, a, d, j, k, e roll over to next undecided (1)' '
 	test_write_lines a b c d e f g h i j k l m n o p q >file &&
 	git add file &&
 	test_write_lines X b c d e f g h X j k l m n o p X >file &&
 	test_set_editor : &&
-	test_write_lines g3 y g3 n g3 j g3 e k q | git add -p >out &&
-	test_write_lines 1  3 1  3 1  3 1  3 1 2 >expect &&
+	test_write_lines g3 y g3 n g3 a g3 d g3 j g3 e k q | git add -p >out &&
+	test_write_lines 1  3 1  3 1  3 1  3 1  3 1  3 1 2 >expect &&
 	sed -n -e "s-/.*--" -e "s/^(//p" <out >actual &&
 	test_cmp expect actual
 '
 
-test_expect_success 'options y, n, j, k, e roll over to next undecided (2)' '
+test_expect_success 'options y, n, a, d, j, k, e roll over to next undecided (2)' '
 	test_write_lines a b c d e f g h i j k l m n o p q >file &&
 	git add file &&
 	test_write_lines X b c d e f g h X j k l m n o p X >file &&
 	test_set_editor : &&
-	test_write_lines y g3 y g3 n g3 j g3 e g1 k q | git add -p >out &&
-	test_write_lines 1 2  3 2  3 2  3 2  3 2  1 2 >expect &&
+	test_write_lines y g3 y g3 n g3 a g3 d g3 j g3 e g1 k q | git add -p >out &&
+	test_write_lines 1 2  3 2  3 2  3 2  3 2  3 2  3 2  1 2 >expect &&
 	sed -n -e "s-/.*--" -e "s/^(//p" <out >actual &&
 	test_cmp expect actual
 '
-- 
2.51.0

[PATCH v3 6/6] add-patch: reset "permitted" at loop start

[René Scharfe]

Don't accumulate allowed options from any visited hunks, start fresh at
the top of the loop instead and only record the allowed options for the
current hunk.

Reported-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: René Scharfe <l.s.r@web.de>
---
 add-patch.c                | 19 ++++++++++---------
 t/t3701-add-interactive.sh | 14 ++++++++++++++
 2 files changed, 24 insertions(+), 9 deletions(-)

diff --git a/add-patch.c b/add-patch.c
index 6da13a78b5..45839ceac5 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -1439,15 +1439,6 @@ static int patch_update_file(struct add_p_state *s,
 	struct child_process cp = CHILD_PROCESS_INIT;
 	int colored = !!s->colored.len, quit = 0, use_pager = 0;
 	enum prompt_mode_type prompt_mode_type;
-	enum {
-		ALLOW_GOTO_PREVIOUS_HUNK = 1 << 0,
-		ALLOW_GOTO_PREVIOUS_UNDECIDED_HUNK = 1 << 1,
-		ALLOW_GOTO_NEXT_HUNK = 1 << 2,
-		ALLOW_GOTO_NEXT_UNDECIDED_HUNK = 1 << 3,
-		ALLOW_SEARCH_AND_GOTO = 1 << 4,
-		ALLOW_SPLIT = 1 << 5,
-		ALLOW_EDIT = 1 << 6
-	} permitted = 0;
 
 	/* Empty added files have no hunks */
 	if (!file_diff->hunk_nr && !file_diff->added)
@@ -1457,6 +1448,16 @@ static int patch_update_file(struct add_p_state *s,
 	render_diff_header(s, file_diff, colored, &s->buf);
 	fputs(s->buf.buf, stdout);
 	for (;;) {
+		enum {
+			ALLOW_GOTO_PREVIOUS_HUNK = 1 << 0,
+			ALLOW_GOTO_PREVIOUS_UNDECIDED_HUNK = 1 << 1,
+			ALLOW_GOTO_NEXT_HUNK = 1 << 2,
+			ALLOW_GOTO_NEXT_UNDECIDED_HUNK = 1 << 3,
+			ALLOW_SEARCH_AND_GOTO = 1 << 4,
+			ALLOW_SPLIT = 1 << 5,
+			ALLOW_EDIT = 1 << 6
+		} permitted = 0;
+
 		if (hunk_index >= file_diff->hunk_nr)
 			hunk_index = 0;
 		hunk = file_diff->hunk_nr
diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index 9d81b0542e..403aaee356 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -1386,4 +1386,18 @@ test_expect_success 'options y, n, a, d, j, k, e roll over to next undecided (2)
 	test_cmp expect actual
 '
 
+test_expect_success 'invalid option s is rejected' '
+	test_write_lines a b c d e f g h i j k >file &&
+	git add file &&
+	test_write_lines X b X d e f g h i j X >file &&
+	test_write_lines j s q | git add -p >out &&
+	sed -ne "s/ @@.*//" -e "s/ \$//" -e "/^(/p" <out >actual &&
+	cat >expect <<-EOF &&
+	(1/2) Stage this hunk [y,n,q,a,d,k,K,j,J,g,/,s,e,p,?]?
+	(2/2) Stage this hunk [y,n,q,a,d,k,K,j,J,g,/,e,p,?]? Sorry, cannot split this hunk
+	(2/2) Stage this hunk [y,n,q,a,d,k,K,j,J,g,/,e,p,?]?
+	EOF
+	test_cmp expect actual
+'
+
 test_done
-- 
2.51.0

RE: [EXT] [PATCH v3 6/6] add-patch: reset "permitted" at loop start

[Windl, Ulrich]

Just a comment of personal taste: I think declaring an anonymous enum inside a loop is just bad style. I think that gcc is smart enough to optimize if "permitted" is declared outside the loop, or make the "permitted" use a typedef for a "named enum" (declared outside the loop while the variable may be inside the loop).

> -----Original Message-----
> From: René Scharfe <l.s.r@web.de>
> Sent: Monday, October 6, 2025 7:24 PM
> To: git@vger.kernel.org
> Cc: Windl, Ulrich <u.windl@ukr.de>; Junio C Hamano <gitster@pobox.com>;
> Phillip Wood <phillip.wood@dunelm.org.uk>
> Subject: [EXT] [PATCH v3 6/6] add-patch: reset "permitted" at loop start
> 
[...] 
>  	for (;;) {
> +		enum {
> +			ALLOW_GOTO_PREVIOUS_HUNK = 1 << 0,
> +			ALLOW_GOTO_PREVIOUS_UNDECIDED_HUNK = 1 <<
> 1,
> +			ALLOW_GOTO_NEXT_HUNK = 1 << 2,
> +			ALLOW_GOTO_NEXT_UNDECIDED_HUNK = 1 << 3,
> +			ALLOW_SEARCH_AND_GOTO = 1 << 4,
> +			ALLOW_SPLIT = 1 << 5,
> +			ALLOW_EDIT = 1 << 6
> +		} permitted = 0;
> +
>  		if (hunk_index >= file_diff->hunk_nr)
>  			hunk_index = 0;
>  		hunk = file_diff->hunk_nr

Re: [EXT] [PATCH v3 6/6] add-patch: reset "permitted" at loop start

[Junio C Hamano]

"Windl, Ulrich" <u.windl@ukr.de> writes:

> Just a comment of personal taste: I think declaring an anonymous
> enum inside a loop is just bad style. I think that gcc is smart
> enough to optimize if "permitted" is declared outside the loop, or
> make the "permitted" use a typedef for a "named enum" (declared
> outside the loop while the variable may be inside the loop).

If this is more than just a personal preference (which to me does
sound like), a patch to improve it on top is very much welcomed.

The change itself would be just reverting the code movement, drop
the 0 initialization and resetting the ariable at the top of the
loop every iteration.  But the rationale being that it would give
compilers a chance to do a better job, I'd prefer to see a compiler
person write the proposed log message, possibly backed by data
(perhaps "generated assembly is objectively better---compare this
and that" in this case?  I dunno).

Thanks.

>> -----Original Message-----
>> From: René Scharfe <l.s.r@web.de>
>> Sent: Monday, October 6, 2025 7:24 PM
>> To: git@vger.kernel.org
>> Cc: Windl, Ulrich <u.windl@ukr.de>; Junio C Hamano <gitster@pobox.com>;
>> Phillip Wood <phillip.wood@dunelm.org.uk>
>> Subject: [EXT] [PATCH v3 6/6] add-patch: reset "permitted" at loop start
>> 
> [...] 
>>  	for (;;) {
>> +		enum {
>> +			ALLOW_GOTO_PREVIOUS_HUNK = 1 << 0,
>> +			ALLOW_GOTO_PREVIOUS_UNDECIDED_HUNK = 1 <<
>> 1,
>> +			ALLOW_GOTO_NEXT_HUNK = 1 << 2,
>> +			ALLOW_GOTO_NEXT_UNDECIDED_HUNK = 1 << 3,
>> +			ALLOW_SEARCH_AND_GOTO = 1 << 4,
>> +			ALLOW_SPLIT = 1 << 5,
>> +			ALLOW_EDIT = 1 << 6
>> +		} permitted = 0;
>> +
>>  		if (hunk_index >= file_diff->hunk_nr)
>>  			hunk_index = 0;
>>  		hunk = file_diff->hunk_nr

Re: [PATCH v3 0/6] add-patch: roll over to next undecided hunk

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

> Changes since v1:
> - added patch 5 for a and d
> - made error messages direction-neutral
> - removed stray "only" from commit message of patch 2
>
>   add-patch: improve help for options j, J, k, and K
>   add-patch: document that option J rolls over
>   add-patch: let options y, n, j, and e roll over to next undecided
>   add-patch: let options k and K roll over like j and J
>   add-patch: let options a and d roll over like y and n
>   add-patch: reset "permitted" at loop start

Will queue.  Should we mark it for 'next'?

Thanks.  

Re: [PATCH v3 0/6] add-patch: roll over to next undecided hunk

[René Scharfe]

On 10/6/25 8:00 PM, Junio C Hamano wrote:
> René Scharfe <l.s.r@web.de> writes:
> 
>> Changes since v1:
>> - added patch 5 for a and d
>> - made error messages direction-neutral
>> - removed stray "only" from commit message of patch 2
>>
>>   add-patch: improve help for options j, J, k, and K
>>   add-patch: document that option J rolls over
>>   add-patch: let options y, n, j, and e roll over to next undecided
>>   add-patch: let options k and K roll over like j and J
>>   add-patch: let options a and d roll over like y and n
>>   add-patch: reset "permitted" at loop start
> 
> Will queue.  Should we mark it for 'next'?

Oh, already?  Fine with me.

René

Re: [PATCH v3 0/6] add-patch: roll over to next undecided hunk

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

> On 10/6/25 8:00 PM, Junio C Hamano wrote:
>> René Scharfe <l.s.r@web.de> writes:
>> 
>>> Changes since v1:
>>> - added patch 5 for a and d
>>> - made error messages direction-neutral
>>> - removed stray "only" from commit message of patch 2
>>>
>>>   add-patch: improve help for options j, J, k, and K
>>>   add-patch: document that option J rolls over
>>>   add-patch: let options y, n, j, and e roll over to next undecided
>>>   add-patch: let options k and K roll over like j and J
>>>   add-patch: let options a and d roll over like y and n
>>>   add-patch: reset "permitted" at loop start
>> 
>> Will queue.  Should we mark it for 'next'?
>
> Oh, already?  Fine with me.

Was just double-checking if there are things you wanted to imrpove.

Thanks.