{"thread":{"id":"64238","subject":"Broken handling of \"J\" hunks for \"add --interactive\"?","startedAt":"2025-10-02T09:25:03Z","lastAt":"2025-11-03T12:43:19Z","messageCount":37,"participants":["Windl, Ulrich","René Scharfe","Phillip Wood","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"527772","messageId":"c72518099a3b465c8761e41210fe3fcb@ukr.de","threadId":"64238","inReplyTo":null,"subject":"Broken handling of \"J\" hunks for \"add --interactive\"?","fromName":"Windl, Ulrich","fromEmail":"u.windl@ukr.de","sentAt":"2025-10-02T09:23:51Z","receivedAt":"2025-10-02T09:25:03Z","isPatch":false,"sender":{"key":"u.windl@ukr.de","avatar":null},"body":"Thank you for filling out a Git bug report!\nPlease answer the following questions to help us understand your issue.\n\nWhat did you do before the bug happened? (Steps to reproduce your issue)\ngit add --interactive\n\nanswer some \"y\", some \"n\", one \"J\"\n\nWhat did you expect to happen? (Expected behavior)\ngit will ask at end for exactly the one \"J\" hunk\n\nWhat happened instead? (Actual behavior)\ngit asked about the hunk rejected before the \"J\" hunk also\n(asked for two hunks instead of one)\n\nWhat's different between what you expected and what actually happened?\nI did not observer that in an older version of git (like 2.26.2)\n\nAnything else you want to add:\n\nPlease review the rest of the bug report below.\nYou can delete any lines you don't wish to share.\n\n\n[System Info]\ngit version:\ngit version 2.51.0\ncpu: x86_64\nno commit associated with this build\nsizeof-long: 8\nsizeof-size_t: 8\nshell-path: /bin/sh\nlibcurl: 8.6.0\nOpenSSL: OpenSSL 3.1.4 24 Oct 2023\nzlib: 1.2.13\nSHA-1: SHA1_DC\nSHA-256: SHA256_BLK\ndefault-ref-format: files\ndefault-hash: sha1\nuname: Linux 6.4.0-150600.23.65-default #1 SMP PREEMPT_DYNAMIC Tue Aug 12 00:37:41 UTC 2025 (aedcb04) x86_64\ncompiler info: gnuc: 7.5\nlibc info: glibc: 2.38\n$SHELL (typically, interactive shell): /bin/bash\n\n\n[Enabled Hooks]\n"},{"id":"527877","messageId":"76665b6f-cb92-4694-bc89-5eb21197df34@web.de","threadId":"64238","inReplyTo":"c72518099a3b465c8761e41210fe3fcb@ukr.de","subject":"[PATCH] add-patch: roll over to next undecided hunk","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2025-10-03T12:16:44Z","receivedAt":"2025-10-03T12:16:52Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"git add --patch presents diff hunks one after the other, asking whether\nto add them. If we mark some as undecided, e.g. with J, then it will\nstart over after reaching the last hunk. It always starts over at the\nvery first hunk, though, even if we already decided on it. Skip\ndecided hunks when rolling over instead.\n\nReported-by: Windl, Ulrich \nSigned-off-by: René Scharfe \n---\n add-patch.c | 9 ++++++++-\n t/t3701-add-interactive.sh | 20 ++++++++++++++++++++\n 2 files changed, 28 insertions(+), 1 deletion(-)\n\ndiff --git a/add-patch.c b/add-patch.c\nindex b0389c5d5b..42a8394c92 100644\n--- a/add-patch.c\n+++ b/add-patch.c\n@@ -1436,8 +1436,15 @@ static int patch_update_file(struct add_p_state *s,\n \trender_diff_header(s, file_diff, colored, &s->buf);\n \tfputs(s->buf.buf, stdout);\n \tfor (;;) {\n-\t\tif (hunk_index >= file_diff->hunk_nr)\n+\t\tif (hunk_index >= file_diff->hunk_nr) {\n \t\t\thunk_index = 0;\n+\t\t\tfor (i = 0; i < file_diff->hunk_nr; i++) {\n+\t\t\t\tif (file_diff->hunk[i].use == UNDECIDED_HUNK) {\n+\t\t\t\t\thunk_index = i;\n+\t\t\t\t\tbreak;\n+\t\t\t\t}\n+\t\t\t}\n+\t\t}\n \t\thunk = file_diff->hunk_nr\n \t\t\t\t? file_diff->hunk + hunk_index\n \t\t\t\t: &file_diff->head;\ndiff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\nindex d9fe289a7a..fa6ec5f835 100755\n--- a/t/t3701-add-interactive.sh\n+++ b/t/t3701-add-interactive.sh\n@@ -1321,6 +1321,26 @@ test_expect_success 'stash accepts -U and --inter-hunk-context' '\n \ttest_grep \"@@ -2,20 +2,20 @@\" actual\n '\n \n+test_expect_success 'roll over to next undecided (1)' '\n+\ttest_write_lines a b c d e f g h i j k l m n o p q >file &&\n+\tgit add file &&\n+\ttest_write_lines X b c d e f g h X j k l m n o p X >file &&\n+\ttest_write_lines J y y q | git add -p >actual &&\n+\ttest_write_lines 1 2 3 1 >expect &&\n+\tsed -ne \"s-/.*--\" -e \"s-^(--p\" hunks &&\n+\ttest_cmp expect hunks\n+'\n+\n+test_expect_success 'roll over to next undecided (2)' '\n+\ttest_write_lines a b c d e f g h i j k l m n o p q >file &&\n+\tgit add file &&\n+\ttest_write_lines X b c d e f g h X j k l m n o p X >file &&\n+\ttest_write_lines y J y q | git add -p >actual &&\n+\ttest_write_lines 1 2 3 2 >expect &&\n+\tsed -ne \"s-/.*--\" -e \"s-^(--p\" hunks &&\n+\ttest_cmp expect hunks\n+'\n+\n test_expect_success 'set up base for -p color tests' '\n \techo commit >file &&\n \tgit commit -am \"commit state\" &&\n-- \n2.51.0\n"},{"id":"527879","messageId":"8fdfb03a-6bbc-46a0-a8fe-9ad75aba555a@gmail.com","threadId":"64238","inReplyTo":"76665b6f-cb92-4694-bc89-5eb21197df34@web.de","subject":"Re: [PATCH] add-patch: roll over to next undecided hunk","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2025-10-03T13:41:35Z","receivedAt":"2025-10-03T13:41:40Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi René\n\nOn 03/10/2025 13:16, René Scharfe wrote:\n> git add --patch presents diff hunks one after the other, asking whether\n> to add them. If we mark some as undecided, e.g. with J, then it will\n> start over after reaching the last hunk. It always starts over at the\n> very first hunk, though, even if we already decided on it. Skip\n> decided hunks when rolling over instead.\n\nNice\n\n> @@ -1436,8 +1436,15 @@ static int patch_update_file(struct add_p_state *s,\n> \trender_diff_header(s, file_diff, colored, &s->buf);\n> \tfputs(s->buf.buf, stdout);\n> \tfor (;;) {\n> -\t\tif (hunk_index >= file_diff->hunk_nr)\n> +\t\tif (hunk_index >= file_diff->hunk_nr) {\n> \t\t\thunk_index = 0;\n> +\t\t\tfor (i = 0; i < file_diff->hunk_nr; i++) {\n> +\t\t\t\tif (file_diff->hunk[i].use == UNDECIDED_HUNK) {\n> +\t\t\t\t\thunk_index = i;\n> +\t\t\t\t\tbreak;\n> +\t\t\t\t}\n> +\t\t\t}\n> +\t\t}\n> \t\thunk = file_diff->hunk_nr\n> \t\t\t\t? file_diff->hunk + hunk_index\n\nIf there were no undecided hunks then this will be out of bounds because \nhunk_index >= file_diff->hunk_nr. Are we absolutely certain that we \ncannot reach this point without at least one hunk being undecided?\n\n> +test_expect_success 'roll over to next undecided (1)' '\n> +\ttest_write_lines a b c d e f g h i j k l m n o p q >file &&\n> +\tgit add file &&\n> +\ttest_write_lines X b c d e f g h X j k l m n o p X >file &&\n> +\ttest_write_lines J y y q | git add -p >actual &&\n> +\ttest_write_lines 1 2 3 1 >expect &&\n> +\tsed -ne \"s-/.*--\" -e \"s-^(--p\" hunks &&\n> +\ttest_cmp expect hunks\n> +'\n\nI'm not sure what this first test adds, the one below checks that we \nfind the first undecided hunk which seems to be the important thing to \ncheck.\n\nThanks\n\nPhillip\n\n> +test_expect_success 'roll over to next undecided (2)' '\n> +\ttest_write_lines a b c d e f g h i j k l m n o p q >file &&\n> +\tgit add file &&\n> +\ttest_write_lines X b c d e f g h X j k l m n o p X >file &&\n> +\ttest_write_lines y J y q | git add -p >actual &&\n> +\ttest_write_lines 1 2 3 2 >expect &&\n> +\tsed -ne \"s-/.*--\" -e \"s-^(--p\" hunks &&\n> +\ttest_cmp expect hunks\n> +'\n> +\n> test_expect_success 'set up base for -p color tests' '\n> \techo commit >file &&\n> \tgit commit -am \"commit state\" &&\n\n"},{"id":"527881","messageId":"fcc003d6-c71f-4c41-a3a1-c9364d3bca9c@web.de","threadId":"64238","inReplyTo":"8fdfb03a-6bbc-46a0-a8fe-9ad75aba555a@gmail.com","subject":"Re: [PATCH] add-patch: roll over to next undecided hunk","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2025-10-03T14:10:27Z","receivedAt":"2025-10-03T14:10:32Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"On 10/3/25 3:41 PM, Phillip Wood wrote:\n> \n>> @@ -1436,8 +1436,15 @@ static int patch_update_file(struct add_p_state *s,\n>>       render_diff_header(s, file_diff, colored, &s->buf);\n>>       fputs(s->buf.buf, stdout);\n>>       for (;;) {\n>> -        if (hunk_index >= file_diff->hunk_nr)\n>> +        if (hunk_index >= file_diff->hunk_nr) {\n>>               hunk_index = 0;\n>> +            for (i = 0; i < file_diff->hunk_nr; i++) {\n>> +                if (file_diff->hunk[i].use == UNDECIDED_HUNK) {\n>> +                    hunk_index = i;\n>> +                    break;\n>> +                }\n>> +            }\n>> +        }\n>>           hunk = file_diff->hunk_nr\n>>                   ? file_diff->hunk + hunk_index\n> \n> If there were no undecided hunks then this will be out of bounds\n> because hunk_index >= file_diff->hunk_nr. Are we absolutely certain\n> that we cannot reach this point without at least one hunk being\n> undecided?\n\nThe new loop only sets hunk_index if i < file_diff->hunk_nr. If\nit finds no undecided hunk then it does nothing.\n\n>> +test_expect_success 'roll over to next undecided (1)' '\n>> +    test_write_lines a b c d e f g h i j k l m n o p q >file &&\n>> +    git add file &&\n>> +    test_write_lines X b c d e f g h X j k l m n o p X >file &&\n>> +    test_write_lines J y y q | git add -p >actual &&\n>> +    test_write_lines 1 2 3 1 >expect &&\n>> +    sed -ne \"s-/.*--\" -e \"s-^(--p\" hunks &&\n>> +    test_cmp expect hunks\n>> +'\n> \n> I'm not sure what this first test adds, the one below checks that we\n> find the first undecided hunk which seems to be the important thing\n> to check.\n\nIt's a regression test for the case that the original code got\nright by accident. It may seem superfluous, but I actually\ntriggered it in my first attempt at a fix.\n\nRené\n\n"},{"id":"527883","messageId":"xmqqo6qoufqp.fsf@gitster.g","threadId":"64238","inReplyTo":"76665b6f-cb92-4694-bc89-5eb21197df34@web.de","subject":"Re: [PATCH] add-patch: roll over to next undecided hunk","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-10-03T16:11:58Z","receivedAt":"2025-10-03T16:12:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n> git add --patch presents diff hunks one after the other, asking whether\n> to add them. If we mark some as undecided, e.g. with J, then it will\n\nPerhaps \"mark\" -> \"leave\".\n\nI somehow find it awkward to say \"mark as undecided\", as I have\nalways viewed J/K as a way to skip a hunk, leaving it undecided.\n\nBesides, \"J\" lets you revisit a hunk that you earlier have decided\nto use of hold off, and it leaves your last decision on that hunk.\nA statement that implies \"J marks as undecided\" is misleading.\n\n> start over after reaching the last hunk. It always starts over at the\n> very first hunk, though, even if we already decided on it. Skip\n> decided hunks when rolling over instead.\n\nNicely analyzed.\n\n> Reported-by: Windl, Ulrich \n> Signed-off-by: René Scharfe \n> ---\n> add-patch.c | 9 ++++++++-\n> t/t3701-add-interactive.sh | 20 ++++++++++++++++++++\n> 2 files changed, 28 insertions(+), 1 deletion(-)\n>\n> diff --git a/add-patch.c b/add-patch.c\n> index b0389c5d5b..42a8394c92 100644\n> --- a/add-patch.c\n> +++ b/add-patch.c\n> @@ -1436,8 +1436,15 @@ static int patch_update_file(struct add_p_state *s,\n> \trender_diff_header(s, file_diff, colored, &s->buf);\n> \tfputs(s->buf.buf, stdout);\n> \tfor (;;) {\n> -\t\tif (hunk_index >= file_diff->hunk_nr)\n> +\t\tif (hunk_index >= file_diff->hunk_nr) {\n> \t\t\thunk_index = 0;\n> +\t\t\tfor (i = 0; i < file_diff->hunk_nr; i++) {\n> +\t\t\t\tif (file_diff->hunk[i].use == UNDECIDED_HUNK) {\n> +\t\t\t\t\thunk_index = i;\n> +\t\t\t\t\tbreak;\n> +\t\t\t\t}\n> +\t\t\t}\n> +\t\t}\n\nOK.\n\nThis is probably a closely related tangent, but last night I was\nlooking this function and found that its per-hunk loop does\ncompletely bogus thing. For example, find a case where you have\nmore than one hunks, among which there are splittable and\nnon-splittable hunks (a hunk is splittable if there are context\nlines between an added or a removed line). Start cycling the hunks\nwithout making any decisions with \"J\" or \"K\". Once you visited a\nsplittable hunk (where you'd see 's' among the possible choices),\ncoming back to an unsplittable hunk will now let you split it! 's'\nmay not be visible among the choices, but telling it to 's'plit will\ngive you \"Split into 1\", which is a technically correct nonsense.\n\nThis is because the handling of \"permitted\" in that function only\nadds, without resetting at the end of processing the current hunk.\nYet it does something like this:\n\n\tfor (;;) {\n\t\t...\n\t\tstrbuf_reset(&s->buf);\n\t\tif (file_diff->hunk_nr) {\n\t\t\t... add choices to the prompt ...\n\t\t\tif (hunk->splittable_into > 1) {\n\t\t\t\tpermitted |= ALLOW_SPLIT;\n\t\t\t\tstrbuf_addstr(&s->buf, \",s\");\n\t\t\t}\n\t\t\t...\n\t\t}\n\t\t...\n\t\tprintf(_(s->mode->prompt_mode[prompt_mode_type]),\n\t\t s->buf.buf);\n\t\tif (*s->s.reset_color_interactive)\n\t\t\tfputs(s->s.reset_color_interactive, stdout);\n\t\tfflush(stdout);\n\t\tif (read_single_character(s) == EOF)\n\t\t\tbreak;\n\t\tch = tolower(s->answer.buf[0]);\n\t\t... dispatch on the command character ...\n\t\tif (ch == 'y') {\n\t\t\t...\n\t\t} else if (s->answer.buf[0] == 's') {\n\t\t\tsize_t splittable_into = hunk->splittable_into;\n\t\t\tif (!(permitted & ALLOW_SPLIT)) {\n\t\t\t\terr(s, _(\"Sorry, cannot split this hunk\"));\n\t\t\t} else if (!split_hunk(s, file_diff,\n\t\t\t\t\t hunk - file_diff->hunk)) {\n\t\t\t\tcolor_fprintf_ln(stdout, s->s.header_color,\n\t\t\t\t\t\t _(\"Split into %d hunks.\"),\n\t\t\t\t\t\t (int)splittable_into);\n\t\t\t\trendered_hunk_index = -1;\n\t\t\t}\n\t\t...\n\nNotice that the prompt is built correctly but that information is\n*not* used when deciding if the operation is possible?\n\nThis is another ancient regression that was introduced while\nrewriting this program in C near the end of 2019, I think. And this\ncauses many other bugs in this area, like 'k' at the very first hunk\ngets complaint \"No previous hunk\" only once (you move to the next\none with 'j' and come back to the first hunk with 'k', and then 'k'\nno longer complains, even though it is not among the choice).\n\nWith this bug, however, we have gained a bit of useful feature, I\nthink. Even though j/J should not be offered when we are at the\nlast hunk for a file, we do wrap-around to the first hunk. I just\nchecked the original code before the C rewrite, and even though it\nwere written defensively so that incrementing the current hunk\nnumber to 5 when you have only 4 hunks would take you back to the\ninitial hunk (instead of barfing), because we did not have this\n\"permitted is never reset\" bug, it actually did not allow you to go\nbeyond the end with j/J. Today's code seems to have inherited this\ndefensive adjustment to stay within the available hunks, and with\nthe \"permitted is never reset\" bug, we are taken back to the first\nhunk.\n\n"},{"id":"527901","messageId":"737e78f5-6337-4964-8385-9c35897f5dff@web.de","threadId":"64238","inReplyTo":"xmqqo6qoufqp.fsf@gitster.g","subject":"Re: [PATCH] add-patch: roll over to next undecided hunk","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2025-10-03T19:53:00Z","receivedAt":"2025-10-03T19:58:20Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"On 10/3/25 6:11 PM, Junio C Hamano wrote:\n> René Scharfe writes:\n> \n>> git add --patch presents diff hunks one after the other, asking whether\n>> to add them. If we mark some as undecided, e.g. with J, then it will\n> \n> Perhaps \"mark\" -> \"leave\".\n> \n> I somehow find it awkward to say \"mark as undecided\", as I have\n> always viewed J/K as a way to skip a hunk, leaving it undecided.\n> \n> Besides, \"J\" lets you revisit a hunk that you earlier have decided\n> to use of hold off, and it leaves your last decision on that hunk.\n> A statement that implies \"J marks as undecided\" is misleading.\n\nRight, j/J/k/K leave the use/skip/undecided status of the current hunk\nunchanged. \"leave this hunk undecided\" in the documentation is\nmisleading as well, because these options will not leave a hunk\nundecided if we made a decision on it before:\n\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\nPerhaps omit it?\n\n j - go to next undecided hunk\n J - go to next hunk\n k - go to previous undecided hunk\n K - go to previous hunk\n\nWeird that one can switch between use and skip, but there's no\nway to revert back to undecided.\n\n>> start over after reaching the last hunk. It always starts over at the\n>> very first hunk, though, even if we already decided on it. Skip\n>> decided hunks when rolling over instead.\n> \n> Nicely analyzed.\n> \n>> Reported-by: Windl, Ulrich \n>> Signed-off-by: René Scharfe \n>> ---\n>> add-patch.c | 9 ++++++++-\n>> t/t3701-add-interactive.sh | 20 ++++++++++++++++++++\n>> 2 files changed, 28 insertions(+), 1 deletion(-)\n>>\n>> diff --git a/add-patch.c b/add-patch.c\n>> index b0389c5d5b..42a8394c92 100644\n>> --- a/add-patch.c\n>> +++ b/add-patch.c\n>> @@ -1436,8 +1436,15 @@ static int patch_update_file(struct add_p_state *s,\n>> \trender_diff_header(s, file_diff, colored, &s->buf);\n>> \tfputs(s->buf.buf, stdout);\n>> \tfor (;;) {\n>> -\t\tif (hunk_index >= file_diff->hunk_nr)\n>> +\t\tif (hunk_index >= file_diff->hunk_nr) {\n>> \t\t\thunk_index = 0;\n>> +\t\t\tfor (i = 0; i < file_diff->hunk_nr; i++) {\n>> +\t\t\t\tif (file_diff->hunk[i].use == UNDECIDED_HUNK) {\n>> +\t\t\t\t\thunk_index = i;\n>> +\t\t\t\t\tbreak;\n>> +\t\t\t\t}\n>> +\t\t\t}\n>> +\t\t}\n> \n> OK.\n> \n> This is probably a closely related tangent, but last night I was\n> looking this function and found that its per-hunk loop does\n> completely bogus thing. For example, find a case where you have\n> more than one hunks, among which there are splittable and\n> non-splittable hunks (a hunk is splittable if there are context\n> lines between an added or a removed line). Start cycling the hunks\n> without making any decisions with \"J\" or \"K\". Once you visited a\n> splittable hunk (where you'd see 's' among the possible choices),\n> coming back to an unsplittable hunk will now let you split it! 's'\n> may not be visible among the choices, but telling it to 's'plit will\n> give you \"Split into 1\", which is a technically correct nonsense.\n> \n> This is because the handling of \"permitted\" in that function only\n> adds, without resetting at the end of processing the current hunk.\n> Yet it does something like this:\n> \n> \tfor (;;) {\n> \t\t...\n> \t\tstrbuf_reset(&s->buf);\n> \t\tif (file_diff->hunk_nr) {\n> \t\t\t... add choices to the prompt ...\n> \t\t\tif (hunk->splittable_into > 1) {\n> \t\t\t\tpermitted |= ALLOW_SPLIT;\n> \t\t\t\tstrbuf_addstr(&s->buf, \",s\");\n> \t\t\t}\n> \t\t\t...\n> \t\t}\n> \t\t...\n> \t\tprintf(_(s->mode->prompt_mode[prompt_mode_type]),\n> \t\t s->buf.buf);\n> \t\tif (*s->s.reset_color_interactive)\n> \t\t\tfputs(s->s.reset_color_interactive, stdout);\n> \t\tfflush(stdout);\n> \t\tif (read_single_character(s) == EOF)\n> \t\t\tbreak;\n> \t\tch = tolower(s->answer.buf[0]);\n> \t\t... dispatch on the command character ...\n> \t\tif (ch == 'y') {\n> \t\t\t...\n> \t\t} else if (s->answer.buf[0] == 's') {\n> \t\t\tsize_t splittable_into = hunk->splittable_into;\n> \t\t\tif (!(permitted & ALLOW_SPLIT)) {\n> \t\t\t\terr(s, _(\"Sorry, cannot split this hunk\"));\n> \t\t\t} else if (!split_hunk(s, file_diff,\n> \t\t\t\t\t hunk - file_diff->hunk)) {\n> \t\t\t\tcolor_fprintf_ln(stdout, s->s.header_color,\n> \t\t\t\t\t\t _(\"Split into %d hunks.\"),\n> \t\t\t\t\t\t (int)splittable_into);\n> \t\t\t\trendered_hunk_index = -1;\n> \t\t\t}\n> \t\t...\n> \n> Notice that the prompt is built correctly but that information is\n> *not* used when deciding if the operation is possible?\n> \n> This is another ancient regression that was introduced while\n> rewriting this program in C near the end of 2019, I think. And this\n> causes many other bugs in this area, like 'k' at the very first hunk\n> gets complaint \"No previous hunk\" only once (you move to the next\n> one with 'j' and come back to the first hunk with 'k', and then 'k'\n> no longer complains, even though it is not among the choice).\n\nThis should be easy to fix by resetting permitted at the start of the\nloop, no? Patch below.\n\n> With this bug, however, we have gained a bit of useful feature, I\n> think. Even though j/J should not be offered when we are at the\n> last hunk for a file, we do wrap-around to the first hunk. I just\n> checked the original code before the C rewrite, and even though it\n> were written defensively so that incrementing the current hunk\n> number to 5 when you have only 4 hunks would take you back to the\n> initial hunk (instead of barfing), because we did not have this\n> \"permitted is never reset\" bug, it actually did not allow you to go\n> beyond the end with j/J. Today's code seems to have inherited this\n> defensive adjustment to stay within the available hunks, and with\n> the \"permitted is never reset\" bug, we are taken back to the first\n> hunk.\ny/n/e on the last hunk roll over, which makes sense to me. Their\nmovement part is not mentioned in the documentation, by the way.\n\nWith the patch below j/J are stopped by the floor, as seemingly\nintended. Not sure if the (now accidental) roll-over behavior is\nbetter for them.\n\n\n add-patch.c | 19 ++++++++++---------\n t/t3701-add-interactive.sh | 19 +++++++++++++++++++\n 2 files changed, 29 insertions(+), 9 deletions(-)\n\ndiff --git a/add-patch.c b/add-patch.c\nindex 42a8394c92..1012840019 100644\n--- a/add-patch.c\n+++ b/add-patch.c\n@@ -1418,15 +1418,6 @@ static int patch_update_file(struct add_p_state *s,\n \tstruct child_process cp = CHILD_PROCESS_INIT;\n \tint colored = !!s->colored.len, quit = 0, use_pager = 0;\n \tenum prompt_mode_type prompt_mode_type;\n-\tenum {\n-\t\tALLOW_GOTO_PREVIOUS_HUNK = 1 << 0,\n-\t\tALLOW_GOTO_PREVIOUS_UNDECIDED_HUNK = 1 << 1,\n-\t\tALLOW_GOTO_NEXT_HUNK = 1 << 2,\n-\t\tALLOW_GOTO_NEXT_UNDECIDED_HUNK = 1 << 3,\n-\t\tALLOW_SEARCH_AND_GOTO = 1 << 4,\n-\t\tALLOW_SPLIT = 1 << 5,\n-\t\tALLOW_EDIT = 1 << 6\n-\t} permitted = 0;\n \n \t/* Empty added files have no hunks */\n \tif (!file_diff->hunk_nr && !file_diff->added)\n@@ -1436,6 +1427,16 @@ static int patch_update_file(struct add_p_state *s,\n \trender_diff_header(s, file_diff, colored, &s->buf);\n \tfputs(s->buf.buf, stdout);\n \tfor (;;) {\n+\t\tenum {\n+\t\t\tALLOW_GOTO_PREVIOUS_HUNK = 1 << 0,\n+\t\t\tALLOW_GOTO_PREVIOUS_UNDECIDED_HUNK = 1 << 1,\n+\t\t\tALLOW_GOTO_NEXT_HUNK = 1 << 2,\n+\t\t\tALLOW_GOTO_NEXT_UNDECIDED_HUNK = 1 << 3,\n+\t\t\tALLOW_SEARCH_AND_GOTO = 1 << 4,\n+\t\t\tALLOW_SPLIT = 1 << 5,\n+\t\t\tALLOW_EDIT = 1 << 6\n+\t\t} permitted = 0;\n+\n \t\tif (hunk_index >= file_diff->hunk_nr) {\n \t\t\thunk_index = 0;\n \t\t\tfor (i = 0; i < file_diff->hunk_nr; i++) {\ndiff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\nindex fa6ec5f835..33b307b8ff 100755\n--- a/t/t3701-add-interactive.sh\n+++ b/t/t3701-add-interactive.sh\n@@ -1341,6 +1341,25 @@ test_expect_success 'roll over to next undecided (2)' '\n \ttest_cmp expect hunks\n '\n \n+test_expect_success 'invalid options are rejected' '\n+\ttest_write_lines a b c d e f g h i j k >file &&\n+\tgit add file &&\n+\ttest_write_lines X b c d e f g h X j X >file &&\n+\ttest_write_lines j j J k k K s q | git add -p >out &&\n+\tsed -ne \"s/ @@.*//\" -e \"s/ \\$//\" -e \"/^(/p\" actual &&\n+\tcat >expect <<-EOF &&\n+\t(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]?\n+\t(2/2) Stage this hunk [y,n,q,a,d,k,K,g,/,s,e,p,?]? No next hunk\n+\t(2/2) Stage this hunk [y,n,q,a,d,k,K,g,/,s,e,p,?]? No next hunk\n+\t(2/2) Stage this hunk [y,n,q,a,d,k,K,g,/,s,e,p,?]?\n+\t(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]? No previous hunk\n+\t(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]? No previous hunk\n+\t(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]? Sorry, cannot split this hunk\n+\t(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]?\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'set up base for -p color tests' '\n \techo commit >file &&\n \tgit commit -am \"commit state\" &&\n\n"},{"id":"527903","messageId":"xmqqcy73u3de.fsf@gitster.g","threadId":"64238","inReplyTo":"737e78f5-6337-4964-8385-9c35897f5dff@web.de","subject":"Re: [PATCH] add-patch: roll over to next undecided hunk","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-10-03T20:39:09Z","receivedAt":"2025-10-03T20:39:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n> Weird that one can switch between use and skip, but there's no\n> way to revert back to undecided.\n\nYes, but Phillip's \"if you split the resulting hunks will revert to\nundecided\" topic, together with \"you can split one hunk into one\"\nbug that is caused by the \"permitted is never reset\" bug, if you can\nnavigate back to what you already decided to use or skip, you can\nsay \"split\" to revert it undecided ;-).\n\n> This should be easy to fix by resetting permitted at the start of the\n> loop, no? Patch below.\n>\n>> With this bug, however, we have gained a bit of useful feature, I\n>> think. Even though j/J should not be offered when we are at the\n>> last hunk for a file, we do wrap-around to the first hunk. I just\n>> checked the original code before the C rewrite, and even though it\n>> were written defensively so that incrementing the current hunk\n>> number to 5 when you have only 4 hunks would take you back to the\n>> initial hunk (instead of barfing), because we did not have this\n>> \"permitted is never reset\" bug, it actually did not allow you to go\n>> beyond the end with j/J. Today's code seems to have inherited this\n>> defensive adjustment to stay within the available hunks, and with\n>> the \"permitted is never reset\" bug, we are taken back to the first\n>> hunk.\n> y/n/e on the last hunk roll over, which makes sense to me. Their\n> movement part is not mentioned in the documentation, by the way.\n>\n> With the patch below j/J are stopped by the floor, as seemingly\n> intended. Not sure if the (now accidental) roll-over behavior is\n> better for them.\n\nYes. Even if it is accidental, people are too used the roll-over\nbehaviour. So at least we should always allow J/K and probably\nallow j/k as long as there at least is a single undecided hunk, if\nwe were to do this fix, and make the prompt string to match.\n\nI only am aware of this bugginess in \"j,k,J,K,s\" but that is only\nbecause I did not look at others. I wouldn't be surprised if they\nwere even buggier.\n\nThanks.\n\n"},{"id":"527904","messageId":"xmqq8qhru37a.fsf@gitster.g","threadId":"64238","inReplyTo":"737e78f5-6337-4964-8385-9c35897f5dff@web.de","subject":"Re: [PATCH] add-patch: roll over to next undecided hunk","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-10-03T20:42:49Z","receivedAt":"2025-10-03T20:42:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n> On 10/3/25 6:11 PM, Junio C Hamano wrote:\n>> René Scharfe writes:\n>> \n>>> git add --patch presents diff hunks one after the other, asking whether\n>>> to add them. If we mark some as undecided, e.g. with J, then it will\n>> \n>> Perhaps \"mark\" -> \"leave\".\n>> \n>> I somehow find it awkward to say \"mark as undecided\", as I have\n>> always viewed J/K as a way to skip a hunk, leaving it undecided.\n>> \n>> Besides, \"J\" lets you revisit a hunk that you earlier have decided\n>> to use of hold off, and it leaves your last decision on that hunk.\n>> A statement that implies \"J marks as undecided\" is misleading.\n>\n> Right, j/J/k/K leave the use/skip/undecided status of the current hunk\n> unchanged.\n\nYes. If the one you are walking away with 'J' were already\nselected, the scenario you describe in the proposed log message\nwould not work, so \"mark\" -> \"leave\" is the right thing to do in\nthat context. But ...\n\n> \"leave this hunk undecided\" in the documentation is\n> misleading as well, because these options will not leave a hunk\n> undecided if we made a decision on it before:\n\n... as you say, I agree that your updated version\n\n> j - go to next undecided hunk\n> J - go to next hunk\n> k - go to previous undecided hunk\n> K - go to previous hunk\n\nin the documentation or help would be a good change.\n\n> Weird that one can switch between use and skip, but there's no\n> way to revert back to undecided.\n\nAnother thing that is missing (and these two are not a regression in\nthe C version, but the same in my scripted original) is that once\nyou decided on _all_ hunks of a file, there is no way to come back\nand tell the tool that you changed your mind.\n"},{"id":"527909","messageId":"xmqq1pnju1je.fsf@gitster.g","threadId":"64238","inReplyTo":"76665b6f-cb92-4694-bc89-5eb21197df34@web.de","subject":"Re: [PATCH] add-patch: roll over to next undecided hunk","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-10-03T21:18:45Z","receivedAt":"2025-10-03T21:18:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n> git add --patch presents diff hunks one after the other, asking whether\n> to add them. If we mark some as undecided, e.g. with J, then it will\n> start over after reaching the last hunk. It always starts over at the\n> very first hunk, though, even if we already decided on it. Skip\n> decided hunks when rolling over instead.\n\nWait a bit. With 'J', the user wants to walk the list of hunks,\nboth decided and undecided ones, no? So ...\n\n> diff --git a/add-patch.c b/add-patch.c\n> index b0389c5d5b..42a8394c92 100644\n> --- a/add-patch.c\n> +++ b/add-patch.c\n> @@ -1436,8 +1436,15 @@ static int patch_update_file(struct add_p_state *s,\n> \trender_diff_header(s, file_diff, colored, &s->buf);\n> \tfputs(s->buf.buf, stdout);\n> \tfor (;;) {\n> -\t\tif (hunk_index >= file_diff->hunk_nr)\n> +\t\tif (hunk_index >= file_diff->hunk_nr) {\n> \t\t\thunk_index = 0;\n> +\t\t\tfor (i = 0; i < file_diff->hunk_nr; i++) {\n> +\t\t\t\tif (file_diff->hunk[i].use == UNDECIDED_HUNK) {\n> +\t\t\t\t\thunk_index = i;\n> +\t\t\t\t\tbreak;\n> +\t\t\t\t}\n> +\t\t\t}\n> +\t\t}\n\n... why is it a good idea to skip decided ones?\n\nWith 'j', the story is probably different, but I didn't dig deep\nenough.\n\nWith 'K', we seem to do\n\n\t\t} else if (s->answer.buf[0] == 'K') {\n\t\t\tif (permitted & ALLOW_GOTO_PREVIOUS_HUNK)\n\t\t\t\thunk_index--;\n\t\t\telse\n\t\t\t\terr(s, _(\"No previous hunk\"));\n\nand there is *no* guard around here to say \"hey, hunk_index has gone\nnegative, so we must move back to the last hunk\", similar to what is\nhappening here that does \"hunk_index has gone beyond the end, so\nlet's wrap around to the first one\".\n\nThis is another bug that is maked by the fact that hunk_index is\nmeasured in size_t (even though there is no reason to do so). Using\nunsigned hunk_ix is simply crazy here, especially for a code that\nwants \"just do hunk_index++ and adjust if it goes beyond the end\"\nand similarly \"just do hunk_index-- and adjust if it goes beyond the\nbeginning\".\n\nIn any case, as the result, going back with 'K' when we are on the\nfirst hunk is broken with the current code because we'd stay there,\nand with this patch, we'd move to the first undecided hunk. Neither\nis correct---we should move to the last hunk, I would think.\n\n"},{"id":"527940","messageId":"17ef29a7-5214-4729-82eb-92a2af33e465@web.de","threadId":"64238","inReplyTo":"c72518099a3b465c8761e41210fe3fcb@ukr.de","subject":"[PATCH v2 0/5] add-patch: roll over to next undecided hunk","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2025-10-05T15:45:58Z","receivedAt":"2025-10-05T15:46:12Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Changes since v1:\n- add patches 1 and 5 to address issues that came up in conversation\n- split out roll-over of option j\n- let options J, k, and K roll over as well\n- broader test coverage\n\n add-patch: improve help for options j, J, k, and K\n add-patch: document that option J rolls over\n add-patch: let options y, n, j, and e roll over to next undecided\n add-patch: let options k and K roll over like j and J\n add-patch: reset \"permitted\" at loop start\n\n Documentation/git-add.adoc | 8 ++--\n add-patch.c | 52 +++++++++++++++++---------\n t/t3701-add-interactive.sh | 76 ++++++++++++++++++++++++++++++--------\n 3 files changed, 99 insertions(+), 37 deletions(-)\n\n-- \n2.51.0\n"},{"id":"527941","messageId":"75b08ed6-4f0f-4ede-b84a-c2f1c3d15734@web.de","threadId":"64238","inReplyTo":"17ef29a7-5214-4729-82eb-92a2af33e465@web.de","subject":"[PATCH v2 1/5] add-patch: improve help for options j, J, k, and K","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2025-10-05T15:55:10Z","receivedAt":"2025-10-05T15:55:18Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"The options j, J, k, and K don't affect the status of the current hunk.\nThey just go to a different one. This is true whether the current hunk\nis undecided or not. Avoid misunderstanding by no longer mentioning\nthe current hunk explicitly in their help texts.\n\nSigned-off-by: René Scharfe \n---\n Documentation/git-add.adoc | 8 ++++----\n add-patch.c | 8 ++++----\n 2 files changed, 8 insertions(+), 8 deletions(-)\n\ndiff --git a/Documentation/git-add.adoc b/Documentation/git-add.adoc\nindex ad629c46c5..3266ccf105 100644\n--- a/Documentation/git-add.adoc\n+++ b/Documentation/git-add.adoc\n@@ -342,10 +342,10 @@ patch::\n d - do not stage this hunk or any of the later hunks in the file\n g - select a hunk to go to\n / - search for a hunk matching the given regex\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+ 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 s - split the current hunk into smaller hunks\n e - manually edit the current hunk\n p - print the current hunk\ndiff --git a/add-patch.c b/add-patch.c\nindex b0389c5d5b..912266a3f8 100644\n--- a/add-patch.c\n+++ b/add-patch.c\n@@ -1397,10 +1397,10 @@ static size_t display_hunks(struct add_p_state *s,\n }\n \n static const char help_patch_remainder[] =\n-N_(\"j - leave this hunk undecided, see next undecided hunk\\n\"\n- \"J - leave this hunk undecided, see next hunk\\n\"\n- \"k - leave this hunk undecided, see previous undecided hunk\\n\"\n- \"K - leave this hunk undecided, see previous hunk\\n\"\n+N_(\"j - go to the next undecided hunk\\n\"\n+ \"J - go to the next hunk\\n\"\n+ \"k - go to the previous undecided hunk\\n\"\n+ \"K - go to the previous hunk\\n\"\n \"g - select a hunk to go to\\n\"\n \"/ - search for a hunk matching the given regex\\n\"\n \"s - split the current hunk into smaller hunks\\n\"\n-- \n2.51.0\n"},{"id":"527942","messageId":"3e1f51b4-b654-4fec-9774-8a76ee6f6cc3@web.de","threadId":"64238","inReplyTo":"17ef29a7-5214-4729-82eb-92a2af33e465@web.de","subject":"[PATCH v2 3/5] add-patch: let options y, n, j, and e roll over to next undecided","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2025-10-05T15:55:34Z","receivedAt":"2025-10-05T15:55:36Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"The options y, n, and e mark the current hunk as decided. If there's\nanother undecided hunk towards the bottom of the hunk array they go\nthere. If there isn't, but there is another undecided hunk towards the\ntop then they go to the very first hunk, no matter if it has already\nbeen decided on.\n\nThe option j does basically the same move. Technically it is not\nallowed if there's no undecided hunk towards the bottom, but the\nvariable \"permitted\" is never reset, so this permission is retained\nfrom the very first hunk. That may a bug, but this behavior is at\nleast consistent with y, n, and e and arguably more useful than\nrefusing to move.\n\nImprove the roll-over behavior of these four options by moving to the\nfirst undecided hunk instead of hunk 1, consistent with what they do\nwhen not rolling over.\n\nReported-by: Windl, Ulrich \nSigned-off-by: René Scharfe \n---\n Documentation/git-add.adoc | 2 +-\n add-patch.c | 11 +++++++++--\n t/t3701-add-interactive.sh | 22 ++++++++++++++++++++++\n 3 files changed, 32 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/git-add.adoc b/Documentation/git-add.adoc\nindex 5c05a3a7f9..596cdeff93 100644\n--- a/Documentation/git-add.adoc\n+++ b/Documentation/git-add.adoc\n@@ -342,7 +342,7 @@ patch::\n d - do not stage this hunk or any of the later hunks in the file\n g - select a hunk to go to\n / - search for a hunk matching the given regex\n- j - go to the next undecided hunk\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\ndiff --git a/add-patch.c b/add-patch.c\nindex bef2ba7a25..da75618dcb 100644\n--- a/add-patch.c\n+++ b/add-patch.c\n@@ -1397,7 +1397,7 @@ static size_t display_hunks(struct add_p_state *s,\n }\n \n static const char help_patch_remainder[] =\n-N_(\"j - go to the next undecided hunk\\n\"\n+N_(\"j - go to the next undecided hunk, roll over at the bottom\\n\"\n \"J - go to the next hunk, roll over at the bottom\\n\"\n \"k - go to the previous undecided hunk\\n\"\n \"K - go to the previous hunk\\n\"\n@@ -1408,6 +1408,11 @@ N_(\"j - go to the next undecided hunk\\n\"\n \"p - print the current hunk, 'P' to use the pager\\n\"\n \"? - print help\\n\");\n \n+static size_t inc_mod(size_t a, size_t m)\n+{\n+\treturn a < m - 1 ? a + 1 : 0;\n+}\n+\n static int patch_update_file(struct add_p_state *s,\n \t\t\t struct file_diff *file_diff)\n {\n@@ -1451,7 +1456,9 @@ static int patch_update_file(struct add_p_state *s,\n \t\t\t\t\tbreak;\n \t\t\t\t}\n \n-\t\t\tfor (i = hunk_index + 1; i < file_diff->hunk_nr; i++)\n+\t\t\tfor (i = inc_mod(hunk_index, file_diff->hunk_nr);\n+\t\t\t i != hunk_index;\n+\t\t\t i = inc_mod(i, file_diff->hunk_nr))\n \t\t\t\tif (file_diff->hunk[i].use == UNDECIDED_HUNK) {\n \t\t\t\t\tundecided_next = i;\n \t\t\t\t\tbreak;\ndiff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\nindex d5d2e120ab..8086d3da71 100755\n--- a/t/t3701-add-interactive.sh\n+++ b/t/t3701-add-interactive.sh\n@@ -1364,4 +1364,26 @@ test_expect_success 'option J rolls over' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'options y, n, j, e roll over to next undecided (1)' '\n+\ttest_write_lines a b c d e f g h i j k l m n o p q >file &&\n+\tgit add file &&\n+\ttest_write_lines X b c d e f g h X j k l m n o p X >file &&\n+\ttest_set_editor : &&\n+\ttest_write_lines g3 y g3 n g3 j g3 e q | git add -p >out &&\n+\ttest_write_lines 1 3 1 3 1 3 1 3 1 >expect &&\n+\tsed -n -e \"s-/.*--\" -e \"s/^(//p\" actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'options y, n, j, e roll over to next undecided (2)' '\n+\ttest_write_lines a b c d e f g h i j k l m n o p q >file &&\n+\tgit add file &&\n+\ttest_write_lines X b c d e f g h X j k l m n o p X >file &&\n+\ttest_set_editor : &&\n+\ttest_write_lines y g3 y g3 n g3 j g3 e q | git add -p >out &&\n+\ttest_write_lines 1 2 3 2 3 2 3 2 3 2 >expect &&\n+\tsed -n -e \"s-/.*--\" -e \"s/^(//p\" actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.51.0\n"},{"id":"527943","messageId":"187aac4d-18b6-41df-a181-7f42e3cbc0d4@web.de","threadId":"64238","inReplyTo":"17ef29a7-5214-4729-82eb-92a2af33e465@web.de","subject":"[PATCH v2 2/5] add-patch: document that option J rolls over","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2025-10-05T15:55:24Z","receivedAt":"2025-10-05T15:55:36Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"The variable \"permitted\" is only not reset after moving to a different\nhunk, so it only accumulates permission and doesn't necessarily reflect\nthose of the current hunk. This may be a bug, but is actually useful\nwith the option J, which can be used at the last hunk to roll over to\nthe first hunk. Make this particular behavior official.\n\nSuggested-by: Junio C Hamano \nSigned-off-by: René Scharfe \n---\n Documentation/git-add.adoc | 2 +-\n add-patch.c | 4 ++--\n t/t3701-add-interactive.sh | 18 ++++++++++++++----\n 3 files changed, 17 insertions(+), 7 deletions(-)\n\ndiff --git a/Documentation/git-add.adoc b/Documentation/git-add.adoc\nindex 3266ccf105..5c05a3a7f9 100644\n--- a/Documentation/git-add.adoc\n+++ b/Documentation/git-add.adoc\n@@ -343,7 +343,7 @@ patch::\n g - select a hunk to go to\n / - search for a hunk matching the given regex\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 s - split the current hunk into smaller hunks\ndiff --git a/add-patch.c b/add-patch.c\nindex 912266a3f8..bef2ba7a25 100644\n--- a/add-patch.c\n+++ b/add-patch.c\n@@ -1398,7 +1398,7 @@ static size_t display_hunks(struct add_p_state *s,\n \n static const char help_patch_remainder[] =\n N_(\"j - go to the next undecided hunk\\n\"\n- \"J - go to the next hunk\\n\"\n+ \"J - go to the next hunk, roll over at the bottom\\n\"\n \"k - go to the previous undecided hunk\\n\"\n \"K - go to the previous hunk\\n\"\n \"g - select a hunk to go to\\n\"\n@@ -1493,7 +1493,7 @@ static int patch_update_file(struct add_p_state *s,\n \t\t\t\tpermitted |= ALLOW_GOTO_NEXT_UNDECIDED_HUNK;\n \t\t\t\tstrbuf_addstr(&s->buf, \",j\");\n \t\t\t}\n-\t\t\tif (hunk_index + 1 < file_diff->hunk_nr) {\n+\t\t\tif (file_diff->hunk_nr > 1) {\n \t\t\t\tpermitted |= ALLOW_GOTO_NEXT_HUNK;\n \t\t\t\tstrbuf_addstr(&s->buf, \",J\");\n \t\t\t}\ndiff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\nindex d9fe289a7a..d5d2e120ab 100755\n--- a/t/t3701-add-interactive.sh\n+++ b/t/t3701-add-interactive.sh\n@@ -334,7 +334,7 @@ test_expect_success 'different prompts for mode change/deleted' '\n \tcat >expect <<-\\EOF &&\n \t(1/1) Stage deletion [y,n,q,a,d,p,?]?\n \t(1/2) Stage mode change [y,n,q,a,d,j,J,g,/,p,?]?\n-\t(2/2) Stage this hunk [y,n,q,a,d,K,g,/,e,p,?]?\n+\t(2/2) Stage this hunk [y,n,q,a,d,K,J,g,/,e,p,?]?\n \tEOF\n \ttest_cmp expect actual.filtered\n '\n@@ -521,7 +521,7 @@ test_expect_success 'split hunk setup' '\n test_expect_success 'goto hunk 1 with \"g 1\"' '\n \ttest_when_finished \"git reset\" &&\n \ttr _ \" \" >expect <<-EOF &&\n-\t(2/2) Stage this hunk [y,n,q,a,d,K,g,/,e,p,?]? + 1: -1,2 +1,3 +15\n+\t(2/2) Stage this hunk [y,n,q,a,d,K,J,g,/,e,p,?]? + 1: -1,2 +1,3 +15\n \t_ 2: -2,4 +3,8 +21\n \tgo to which hunk? @@ -1,2 +1,3 @@\n \t_10\n@@ -550,7 +550,7 @@ test_expect_success 'goto hunk 1 with \"g1\"' '\n test_expect_success 'navigate to hunk via regex /pattern' '\n \ttest_when_finished \"git reset\" &&\n \ttr _ \" \" >expect <<-EOF &&\n-\t(2/2) Stage this hunk [y,n,q,a,d,K,g,/,e,p,?]? @@ -1,2 +1,3 @@\n+\t(2/2) Stage this hunk [y,n,q,a,d,K,J,g,/,e,p,?]? @@ -1,2 +1,3 @@\n \t_10\n \t+15\n \t_20\n@@ -805,7 +805,7 @@ test_expect_success 'colors can be overridden' '\n \t(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]? @@ -3 +3,2 @@\n \t more-context\n \t+another-one\n-\t(2/2) Stage this hunk [y,n,q,a,d,K,g,/,e,p,?]? @@ -1,3 +1,3 @@\n+\t(2/2) Stage this hunk [y,n,q,a,d,K,J,g,/,e,p,?]? @@ -1,3 +1,3 @@\n \t context\n \t-old\n \t+new\n@@ -1354,4 +1354,14 @@ do\n \t'\n done\n \n+test_expect_success 'option J rolls over' '\n+\ttest_write_lines a b c d e f g h i >file &&\n+\tgit add file &&\n+\ttest_write_lines X b c d e f g h X >file &&\n+\ttest_write_lines J J q | git add -p >out &&\n+\ttest_write_lines 1 2 1 >expect &&\n+\tsed -n -e \"s-/.*--\" -e \"s/^(//p\" actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.51.0\n"},{"id":"527944","messageId":"f99b93d5-3de2-4077-8818-9272e812c289@web.de","threadId":"64238","inReplyTo":"17ef29a7-5214-4729-82eb-92a2af33e465@web.de","subject":"[PATCH v2 4/5] add-patch: let options k and K roll over like j and J","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2025-10-05T15:55:46Z","receivedAt":"2025-10-05T15:55:51Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Options j and J roll over at the bottom and go to the first undecided\nhunk and hunk 1, respectively. Let options k and K do the same when\nthey reach the top of the hunk array, so let them go to the last\nundecided hunk and the last hunk, respectively, for consistency.\n\nSigned-off-by: René Scharfe \n---\n Documentation/git-add.adoc | 4 ++--\n add-patch.c | 18 ++++++++++++-----\n t/t3701-add-interactive.sh | 40 +++++++++++++++++++-------------------\n 3 files changed, 35 insertions(+), 27 deletions(-)\n\ndiff --git a/Documentation/git-add.adoc b/Documentation/git-add.adoc\nindex 596cdeff93..3116a2cac5 100644\n--- a/Documentation/git-add.adoc\n+++ b/Documentation/git-add.adoc\n@@ -344,8 +344,8 @@ patch::\n / - search for a hunk matching the given regex\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 s - split the current hunk into smaller hunks\n e - manually edit the current hunk\n p - print the current hunk\ndiff --git a/add-patch.c b/add-patch.c\nindex da75618dcb..52e881d3b0 100644\n--- a/add-patch.c\n+++ b/add-patch.c\n@@ -1399,8 +1399,8 @@ static size_t display_hunks(struct add_p_state *s,\n static const char help_patch_remainder[] =\n N_(\"j - go to the next undecided hunk, roll over at the bottom\\n\"\n \"J - go to the next hunk, roll over at the bottom\\n\"\n- \"k - go to the previous undecided hunk\\n\"\n- \"K - go to the previous hunk\\n\"\n+ \"k - go to the previous undecided hunk, roll over at the top\\n\"\n+ \"K - go to the previous hunk, roll over at the top\\n\"\n \"g - select a hunk to go to\\n\"\n \"/ - search for a hunk matching the given regex\\n\"\n \"s - split the current hunk into smaller hunks\\n\"\n@@ -1408,6 +1408,11 @@ N_(\"j - go to the next undecided hunk, roll over at the bottom\\n\"\n \"p - print the current hunk, 'P' to use the pager\\n\"\n \"? - print help\\n\");\n \n+static size_t dec_mod(size_t a, size_t m)\n+{\n+\treturn a > 0 ? a - 1 : m - 1;\n+}\n+\n static size_t inc_mod(size_t a, size_t m)\n {\n \treturn a < m - 1 ? a + 1 : 0;\n@@ -1450,7 +1455,9 @@ static int patch_update_file(struct add_p_state *s,\n \t\tundecided_next = -1;\n \n \t\tif (file_diff->hunk_nr) {\n-\t\t\tfor (i = hunk_index - 1; i >= 0; i--)\n+\t\t\tfor (i = dec_mod(hunk_index, file_diff->hunk_nr);\n+\t\t\t i != hunk_index;\n+\t\t\t i = dec_mod(i, file_diff->hunk_nr))\n \t\t\t\tif (file_diff->hunk[i].use == UNDECIDED_HUNK) {\n \t\t\t\t\tundecided_previous = i;\n \t\t\t\t\tbreak;\n@@ -1492,7 +1499,7 @@ static int patch_update_file(struct add_p_state *s,\n \t\t\t\tpermitted |= ALLOW_GOTO_PREVIOUS_UNDECIDED_HUNK;\n \t\t\t\tstrbuf_addstr(&s->buf, \",k\");\n \t\t\t}\n-\t\t\tif (hunk_index) {\n+\t\t\tif (file_diff->hunk_nr > 1) {\n \t\t\t\tpermitted |= ALLOW_GOTO_PREVIOUS_HUNK;\n \t\t\t\tstrbuf_addstr(&s->buf, \",K\");\n \t\t\t}\n@@ -1584,7 +1591,8 @@ static int patch_update_file(struct add_p_state *s,\n \t\t\t}\n \t\t} else if (s->answer.buf[0] == 'K') {\n \t\t\tif (permitted & ALLOW_GOTO_PREVIOUS_HUNK)\n-\t\t\t\thunk_index--;\n+\t\t\t\thunk_index = dec_mod(hunk_index,\n+\t\t\t\t\t\t file_diff->hunk_nr);\n \t\t\telse\n \t\t\t\terr(s, _(\"No previous hunk\"));\n \t\t} else if (s->answer.buf[0] == 'J') {\ndiff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\nindex 8086d3da71..385e55c783 100755\n--- a/t/t3701-add-interactive.sh\n+++ b/t/t3701-add-interactive.sh\n@@ -333,7 +333,7 @@ test_expect_success 'different prompts for mode change/deleted' '\n \tsed -n \"s/^\\(([0-9/]*) Stage .*?\\).*/\\1/p\" actual >actual.filtered &&\n \tcat >expect <<-\\EOF &&\n \t(1/1) Stage deletion [y,n,q,a,d,p,?]?\n-\t(1/2) Stage mode change [y,n,q,a,d,j,J,g,/,p,?]?\n+\t(1/2) Stage mode change [y,n,q,a,d,k,K,j,J,g,/,p,?]?\n \t(2/2) Stage this hunk [y,n,q,a,d,K,J,g,/,e,p,?]?\n \tEOF\n \ttest_cmp expect actual.filtered\n@@ -527,7 +527,7 @@ test_expect_success 'goto hunk 1 with \"g 1\"' '\n \t_10\n \t+15\n \t_20\n-\t(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]?_\n+\t(1/2) Stage this hunk [y,n,q,a,d,k,K,j,J,g,/,e,p,?]?_\n \tEOF\n \ttest_write_lines s y g 1 | git add -p >actual &&\n \ttail -n 7 actual.trimmed &&\n@@ -540,7 +540,7 @@ test_expect_success 'goto hunk 1 with \"g1\"' '\n \t_10\n \t+15\n \t_20\n-\t(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]?_\n+\t(1/2) Stage this hunk [y,n,q,a,d,k,K,j,J,g,/,e,p,?]?_\n \tEOF\n \ttest_write_lines s y g1 | git add -p >actual &&\n \ttail -n 4 actual.trimmed &&\n@@ -554,7 +554,7 @@ test_expect_success 'navigate to hunk via regex /pattern' '\n \t_10\n \t+15\n \t_20\n-\t(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]?_\n+\t(1/2) Stage this hunk [y,n,q,a,d,k,K,j,J,g,/,e,p,?]?_\n \tEOF\n \ttest_write_lines s y /1,2 | git add -p >actual &&\n \ttail -n 5 actual.trimmed &&\n@@ -567,7 +567,7 @@ test_expect_success 'navigate to hunk via regex / pattern' '\n \t_10\n \t+15\n \t_20\n-\t(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]?_\n+\t(1/2) Stage this hunk [y,n,q,a,d,k,K,j,J,g,/,e,p,?]?_\n \tEOF\n \ttest_write_lines s y / 1,2 | git add -p >actual &&\n \ttail -n 4 actual.trimmed &&\n@@ -579,11 +579,11 @@ test_expect_success 'print again the hunk' '\n \ttr _ \" \" >expect <<-EOF &&\n \t+15\n \t 20\n-\t(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]? @@ -1,2 +1,3 @@\n+\t(1/2) Stage this hunk [y,n,q,a,d,k,K,j,J,g,/,e,p,?]? @@ -1,2 +1,3 @@\n \t 10\n \t+15\n \t 20\n-\t(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]?_\n+\t(1/2) Stage this hunk [y,n,q,a,d,k,K,j,J,g,/,e,p,?]?_\n \tEOF\n \ttest_write_lines s y g 1 p | git add -p >actual &&\n \ttail -n 7 actual.trimmed &&\n@@ -595,11 +595,11 @@ test_expect_success TTY 'print again the hunk (PAGER)' '\n \tcat >expect <<-EOF &&\n \t+15\n \t 20\n-\t(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]? PAGER @@ -1,2 +1,3 @@\n+\t(1/2) Stage this hunk [y,n,q,a,d,k,K,j,J,g,/,e,p,?]? PAGER @@ -1,2 +1,3 @@\n \tPAGER 10\n \tPAGER +15\n \tPAGER 20\n-\t(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]? \n+\t(1/2) Stage this hunk [y,n,q,a,d,k,K,j,J,g,/,e,p,?]? \n \tEOF\n \ttest_write_lines s y g 1 P |\n \t(\n@@ -802,7 +802,7 @@ test_expect_success 'colors can be overridden' '\n \t-old\n \t+new\n \t more-context\n-\t(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]? @@ -3 +3,2 @@\n+\t(1/2) Stage this hunk [y,n,q,a,d,k,K,j,J,g,/,e,p,?]? @@ -3 +3,2 @@\n \t more-context\n \t+another-one\n \t(2/2) Stage this hunk [y,n,q,a,d,K,J,g,/,e,p,?]? @@ -1,3 +1,3 @@\n@@ -810,7 +810,7 @@ test_expect_success 'colors can be overridden' '\n \t-old\n \t+new\n \t more-context\n-\t(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]? \n+\t(1/2) Stage this hunk [y,n,q,a,d,k,K,j,J,g,/,e,p,?]? \n \tEOF\n \ttest_cmp expect actual\n '\n@@ -1354,34 +1354,34 @@ do\n \t'\n done\n \n-test_expect_success 'option J rolls over' '\n+test_expect_success 'options J, K roll over' '\n \ttest_write_lines a b c d e f g h i >file &&\n \tgit add file &&\n \ttest_write_lines X b c d e f g h X >file &&\n-\ttest_write_lines J J q | git add -p >out &&\n-\ttest_write_lines 1 2 1 >expect &&\n+\ttest_write_lines J J K q | git add -p >out &&\n+\ttest_write_lines 1 2 1 2 >expect &&\n \tsed -n -e \"s-/.*--\" -e \"s/^(//p\" actual &&\n \ttest_cmp expect actual\n '\n \n-test_expect_success 'options y, n, j, e roll over to next undecided (1)' '\n+test_expect_success 'options y, n, j, k, e roll over to next undecided (1)' '\n \ttest_write_lines a b c d e f g h i j k l m n o p q >file &&\n \tgit add file &&\n \ttest_write_lines X b c d e f g h X j k l m n o p X >file &&\n \ttest_set_editor : &&\n-\ttest_write_lines g3 y g3 n g3 j g3 e q | git add -p >out &&\n-\ttest_write_lines 1 3 1 3 1 3 1 3 1 >expect &&\n+\ttest_write_lines g3 y g3 n g3 j g3 e k q | git add -p >out &&\n+\ttest_write_lines 1 3 1 3 1 3 1 3 1 2 >expect &&\n \tsed -n -e \"s-/.*--\" -e \"s/^(//p\" actual &&\n \ttest_cmp expect actual\n '\n \n-test_expect_success 'options y, n, j, e roll over to next undecided (2)' '\n+test_expect_success 'options y, n, j, k, e roll over to next undecided (2)' '\n \ttest_write_lines a b c d e f g h i j k l m n o p q >file &&\n \tgit add file &&\n \ttest_write_lines X b c d e f g h X j k l m n o p X >file &&\n \ttest_set_editor : &&\n-\ttest_write_lines y g3 y g3 n g3 j g3 e q | git add -p >out &&\n-\ttest_write_lines 1 2 3 2 3 2 3 2 3 2 >expect &&\n+\ttest_write_lines y g3 y g3 n g3 j g3 e g1 k q | git add -p >out &&\n+\ttest_write_lines 1 2 3 2 3 2 3 2 3 2 1 2 >expect &&\n \tsed -n -e \"s-/.*--\" -e \"s/^(//p\" actual &&\n \ttest_cmp expect actual\n '\n-- \n2.51.0\n"},{"id":"527945","messageId":"5dc0941b-2bc1-4107-b39e-8312ff7c08f9@web.de","threadId":"64238","inReplyTo":"17ef29a7-5214-4729-82eb-92a2af33e465@web.de","subject":"[PATCH v2 5/5] add-patch: reset \"permitted\" at loop start","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2025-10-05T15:55:50Z","receivedAt":"2025-10-05T15:56:03Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Don't accumulate allowed options from any visited hunks, start fresh at\nthe top of the loop instead and only record the allowed options for the\ncurrent hunk.\n\nReported-by: Junio C Hamano \nSigned-off-by: René Scharfe \n---\n add-patch.c | 19 ++++++++++---------\n t/t3701-add-interactive.sh | 14 ++++++++++++++\n 2 files changed, 24 insertions(+), 9 deletions(-)\n\ndiff --git a/add-patch.c b/add-patch.c\nindex 52e881d3b0..7b489d0a75 100644\n--- a/add-patch.c\n+++ b/add-patch.c\n@@ -1428,15 +1428,6 @@ static int patch_update_file(struct add_p_state *s,\n \tstruct child_process cp = CHILD_PROCESS_INIT;\n \tint colored = !!s->colored.len, quit = 0, use_pager = 0;\n \tenum prompt_mode_type prompt_mode_type;\n-\tenum {\n-\t\tALLOW_GOTO_PREVIOUS_HUNK = 1 << 0,\n-\t\tALLOW_GOTO_PREVIOUS_UNDECIDED_HUNK = 1 << 1,\n-\t\tALLOW_GOTO_NEXT_HUNK = 1 << 2,\n-\t\tALLOW_GOTO_NEXT_UNDECIDED_HUNK = 1 << 3,\n-\t\tALLOW_SEARCH_AND_GOTO = 1 << 4,\n-\t\tALLOW_SPLIT = 1 << 5,\n-\t\tALLOW_EDIT = 1 << 6\n-\t} permitted = 0;\n \n \t/* Empty added files have no hunks */\n \tif (!file_diff->hunk_nr && !file_diff->added)\n@@ -1446,6 +1437,16 @@ static int patch_update_file(struct add_p_state *s,\n \trender_diff_header(s, file_diff, colored, &s->buf);\n \tfputs(s->buf.buf, stdout);\n \tfor (;;) {\n+\t\tenum {\n+\t\t\tALLOW_GOTO_PREVIOUS_HUNK = 1 << 0,\n+\t\t\tALLOW_GOTO_PREVIOUS_UNDECIDED_HUNK = 1 << 1,\n+\t\t\tALLOW_GOTO_NEXT_HUNK = 1 << 2,\n+\t\t\tALLOW_GOTO_NEXT_UNDECIDED_HUNK = 1 << 3,\n+\t\t\tALLOW_SEARCH_AND_GOTO = 1 << 4,\n+\t\t\tALLOW_SPLIT = 1 << 5,\n+\t\t\tALLOW_EDIT = 1 << 6\n+\t\t} permitted = 0;\n+\n \t\tif (hunk_index >= file_diff->hunk_nr)\n \t\t\thunk_index = 0;\n \t\thunk = file_diff->hunk_nr\ndiff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\nindex 385e55c783..8c24a76e59 100755\n--- a/t/t3701-add-interactive.sh\n+++ b/t/t3701-add-interactive.sh\n@@ -1386,4 +1386,18 @@ test_expect_success 'options y, n, j, k, e roll over to next undecided (2)' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'invalid option s is rejected' '\n+\ttest_write_lines a b c d e f g h i j k >file &&\n+\tgit add file &&\n+\ttest_write_lines X b X d e f g h i j X >file &&\n+\ttest_write_lines j s q | git add -p >out &&\n+\tsed -ne \"s/ @@.*//\" -e \"s/ \\$//\" -e \"/^(/p\" actual &&\n+\tcat >expect <<-EOF &&\n+\t(1/2) Stage this hunk [y,n,q,a,d,k,K,j,J,g,/,s,e,p,?]?\n+\t(2/2) Stage this hunk [y,n,q,a,d,k,K,j,J,g,/,e,p,?]? Sorry, cannot split this hunk\n+\t(2/2) Stage this hunk [y,n,q,a,d,k,K,j,J,g,/,e,p,?]?\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.51.0\n"},{"id":"527947","messageId":"xmqqh5wdrrub.fsf@gitster.g","threadId":"64238","inReplyTo":"f99b93d5-3de2-4077-8818-9272e812c289@web.de","subject":"Re: [PATCH v2 4/5] add-patch: let options k and K roll over like j and J","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-10-05T20:55:40Z","receivedAt":"2025-10-05T20:55:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n> @@ -1584,7 +1591,8 @@ static int patch_update_file(struct add_p_state *s,\n> \t\t\t}\n> \t\t} else if (s->answer.buf[0] == 'K') {\n> \t\t\tif (permitted & ALLOW_GOTO_PREVIOUS_HUNK)\n> -\t\t\t\thunk_index--;\n> +\t\t\t\thunk_index = dec_mod(hunk_index,\n> +\t\t\t\t\t\t file_diff->hunk_nr);\n> \t\t\telse\n> \t\t\t\terr(s, _(\"No previous hunk\"));\n\nI was wondering if we want to always allow J and K; even when you\nhave only one hunk, you can still wrap around to come back to the\ncurrent hunk, and that we can do without any extra checking logic.\n\nBut it is also OK to require 2 or more hunks to \"switch\" to the\nother hunk, which is what you do with\n\n\t\t\tif (file_diff->hunk_nr > 1) {\n\t\t\t\tpermitted |= ALLOW_GOTO_PREVIOUS_HUNK;\n\t\t\t\tstrbuf_addstr(&s->buf, \",K\");\n\t\t\t}\n\nto require more than 1. But the error message \"No previous hunk\"\nsounds somewhat awkward. If user accepts the circular nature of how\nwe decide what \"previous\" is, then when we have a single hunk, the\ncurrent hunk itself _is_ the previous hunk, but because we insist\nthat there are at least 2, that interpretation would not work. With\n\"wraparound\" semantics, \"No other hunk(s)\", would be a better way to\ngive the error, no? The same comment applies to 'J'.\n\n> \t\t} else if (s->answer.buf[0] == 'J') {\n\nThis makes perfect sense, but then, after this post-context we have this:\n\n\t\t\tif (permitted & ALLOW_GOTO_NEXT_HUNK)\n\t\t\t\thunk_index++;\n\t\t\telse\n\t\t\t\terr(s, _(\"No next hunk\"));\n\nand it sticks out that the post-increment of hunk_index here is not\nusing inc_mod() for symmetry.\n\nI am wondering if with that updated (I would not say \"fixed\"), if we\ncan lose the \"oops we overflowed so let's wrap around\" belt-and-suspender\ncode at the beginning of the loop, i.e.\n\n\tfor (;;) {\n\t\tenum {\n\t\t\tALLOW_GOTO_PREVIOUS_HUNK = 1 << 0,\n\t\t\t...\n\t\t\tALLOW_EDIT = 1 << 6\n\t\t} permitted = 0;\n\n\t\tif (hunk_index >= file_diff->hunk_nr)\n\t\t\thunk_index = 0;\n\nor if there still are other code that rely on this \"oops we\noverflowed\" adjustment?\n\nOther than that the resulting code with the whole series applied was\na very pleasant read.\n\nThanks.\n"},{"id":"527954","messageId":"xmqqbjmlrq8g.fsf@gitster.g","threadId":"64238","inReplyTo":"75b08ed6-4f0f-4ede-b84a-c2f1c3d15734@web.de","subject":"Re: [PATCH v2 1/5] add-patch: improve help for options j, J, k, and K","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-10-05T21:30:23Z","receivedAt":"2025-10-05T21:30:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n> The options j, J, k, and K don't affect the status of the current hunk.\n> They just go to a different one. This is true whether the current hunk\n> is undecided or not. Avoid misunderstanding by no longer mentioning\n> the current hunk explicitly in their help texts.\n>\n> Signed-off-by: René Scharfe \n> ---\n> Documentation/git-add.adoc | 8 ++++----\n> add-patch.c | 8 ++++----\n> 2 files changed, 8 insertions(+), 8 deletions(-)\n>\n> diff --git a/Documentation/git-add.adoc b/Documentation/git-add.adoc\n> index ad629c46c5..3266ccf105 100644\n> --- a/Documentation/git-add.adoc\n> +++ b/Documentation/git-add.adoc\n> @@ -342,10 +342,10 @@ patch::\n> d - do not stage this hunk or any of the later hunks in the file\n> g - select a hunk to go to\n> / - search for a hunk matching the given regex\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> + 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\nThese obviously make sense, but I wonder if y/n should also say that\nthey not just make a decision on the current hunk, but after doing\nso they move you forward (and if so, that may fall within the theme\nof this step, which is to improve the help text on options).\n\n"},{"id":"527955","messageId":"xmqq7bx9rq7w.fsf@gitster.g","threadId":"64238","inReplyTo":"187aac4d-18b6-41df-a181-7f42e3cbc0d4@web.de","subject":"Re: [PATCH v2 2/5] add-patch: document that option J rolls over","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-10-05T21:30:43Z","receivedAt":"2025-10-05T21:30:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n> The variable \"permitted\" is only not reset after moving to a different\n\n\"only not\" -> \"not\".\n\n> hunk, so it only accumulates permission and doesn't necessarily reflect\n> those of the current hunk. This may be a bug, but is actually useful\n> with the option J, which can be used at the last hunk to roll over to\n> the first hunk. Make this particular behavior official.\n>\n> Suggested-by: Junio C Hamano \n> Signed-off-by: René Scharfe \n> ---\n> Documentation/git-add.adoc | 2 +-\n> add-patch.c | 4 ++--\n> t/t3701-add-interactive.sh | 18 ++++++++++++++----\n> 3 files changed, 17 insertions(+), 7 deletions(-)\n>\n> diff --git a/Documentation/git-add.adoc b/Documentation/git-add.adoc\n> index 3266ccf105..5c05a3a7f9 100644\n> --- a/Documentation/git-add.adoc\n> +++ b/Documentation/git-add.adoc\n> @@ -343,7 +343,7 @@ patch::\n> g - select a hunk to go to\n> / - search for a hunk matching the given regex\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> s - split the current hunk into smaller hunks\n> diff --git a/add-patch.c b/add-patch.c\n> index 912266a3f8..bef2ba7a25 100644\n> --- a/add-patch.c\n> +++ b/add-patch.c\n> @@ -1398,7 +1398,7 @@ static size_t display_hunks(struct add_p_state *s,\n> \n> static const char help_patch_remainder[] =\n> N_(\"j - go to the next undecided hunk\\n\"\n> - \"J - go to the next hunk\\n\"\n> + \"J - go to the next hunk, roll over at the bottom\\n\"\n> \"k - go to the previous undecided hunk\\n\"\n> \"K - go to the previous hunk\\n\"\n> \"g - select a hunk to go to\\n\"\n> @@ -1493,7 +1493,7 @@ static int patch_update_file(struct add_p_state *s,\n> \t\t\t\tpermitted |= ALLOW_GOTO_NEXT_UNDECIDED_HUNK;\n> \t\t\t\tstrbuf_addstr(&s->buf, \",j\");\n> \t\t\t}\n> -\t\t\tif (hunk_index + 1 < file_diff->hunk_nr) {\n> +\t\t\tif (file_diff->hunk_nr > 1) {\n> \t\t\t\tpermitted |= ALLOW_GOTO_NEXT_HUNK;\n> \t\t\t\tstrbuf_addstr(&s->buf, \",J\");\n> \t\t\t}\n> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\n> index d9fe289a7a..d5d2e120ab 100755\n> --- a/t/t3701-add-interactive.sh\n> +++ b/t/t3701-add-interactive.sh\n> @@ -334,7 +334,7 @@ test_expect_success 'different prompts for mode change/deleted' '\n> \tcat >expect <<-\\EOF &&\n> \t(1/1) Stage deletion [y,n,q,a,d,p,?]?\n> \t(1/2) Stage mode change [y,n,q,a,d,j,J,g,/,p,?]?\n> -\t(2/2) Stage this hunk [y,n,q,a,d,K,g,/,e,p,?]?\n> +\t(2/2) Stage this hunk [y,n,q,a,d,K,J,g,/,e,p,?]?\n> \tEOF\n> \ttest_cmp expect actual.filtered\n> '\n> @@ -521,7 +521,7 @@ test_expect_success 'split hunk setup' '\n> test_expect_success 'goto hunk 1 with \"g 1\"' '\n> \ttest_when_finished \"git reset\" &&\n> \ttr _ \" \" >expect <<-EOF &&\n> -\t(2/2) Stage this hunk [y,n,q,a,d,K,g,/,e,p,?]? + 1: -1,2 +1,3 +15\n> +\t(2/2) Stage this hunk [y,n,q,a,d,K,J,g,/,e,p,?]? + 1: -1,2 +1,3 +15\n> \t_ 2: -2,4 +3,8 +21\n> \tgo to which hunk? @@ -1,2 +1,3 @@\n> \t_10\n> @@ -550,7 +550,7 @@ test_expect_success 'goto hunk 1 with \"g1\"' '\n> test_expect_success 'navigate to hunk via regex /pattern' '\n> \ttest_when_finished \"git reset\" &&\n> \ttr _ \" \" >expect <<-EOF &&\n> -\t(2/2) Stage this hunk [y,n,q,a,d,K,g,/,e,p,?]? @@ -1,2 +1,3 @@\n> +\t(2/2) Stage this hunk [y,n,q,a,d,K,J,g,/,e,p,?]? @@ -1,2 +1,3 @@\n> \t_10\n> \t+15\n> \t_20\n> @@ -805,7 +805,7 @@ test_expect_success 'colors can be overridden' '\n> \t(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]? @@ -3 +3,2 @@\n> \t more-context\n> \t+another-one\n> -\t(2/2) Stage this hunk [y,n,q,a,d,K,g,/,e,p,?]? @@ -1,3 +1,3 @@\n> +\t(2/2) Stage this hunk [y,n,q,a,d,K,J,g,/,e,p,?]? @@ -1,3 +1,3 @@\n> \t context\n> \t-old\n> \t+new\n> @@ -1354,4 +1354,14 @@ do\n> \t'\n> done\n> \n> +test_expect_success 'option J rolls over' '\n> +\ttest_write_lines a b c d e f g h i >file &&\n> +\tgit add file &&\n> +\ttest_write_lines X b c d e f g h X >file &&\n> +\ttest_write_lines J J q | git add -p >out &&\n> +\ttest_write_lines 1 2 1 >expect &&\n> +\tsed -n -e \"s-/.*--\" -e \"s/^(//p\" actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> test_done\n"},{"id":"527997","messageId":"16d5908b-bed6-4ad2-bb27-9c6523f904d0@web.de","threadId":"64238","inReplyTo":"xmqqbjmlrq8g.fsf@gitster.g","subject":"Re: [PATCH v2 1/5] add-patch: improve help for options j, J, k, and K","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2025-10-06T17:17:53Z","receivedAt":"2025-10-06T17:18:01Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"On 10/5/25 11:30 PM, Junio C Hamano wrote:\n> René Scharfe writes:\n> \n>> The options j, J, k, and K don't affect the status of the current hunk.\n>> They just go to a different one. This is true whether the current hunk\n>> is undecided or not. Avoid misunderstanding by no longer mentioning\n>> the current hunk explicitly in their help texts.\n>>\n>> Signed-off-by: René Scharfe \n>> ---\n>> Documentation/git-add.adoc | 8 ++++----\n>> add-patch.c | 8 ++++----\n>> 2 files changed, 8 insertions(+), 8 deletions(-)\n>>\n>> diff --git a/Documentation/git-add.adoc b/Documentation/git-add.adoc\n>> index ad629c46c5..3266ccf105 100644\n>> --- a/Documentation/git-add.adoc\n>> +++ b/Documentation/git-add.adoc\n>> @@ -342,10 +342,10 @@ patch::\n>> d - do not stage this hunk or any of the later hunks in the file\n>> g - select a hunk to go to\n>> / - search for a hunk matching the given regex\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>> + 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> \n> These obviously make sense, but I wonder if y/n should also say that\n> they not just make a decision on the current hunk, but after doing\n> so they move you forward\n\nYes.\n\n> (and if so, that may fall within the theme\n> of this step, which is to improve the help text on options).\n\nI see it more narrowly: This patch removes unnecessary references to the\nhunk's status, while a y/n doc patch would add missing pieces.\n\nHmm, would the help text need to adapt to whether the current hunk is\nthe last undecided one? E.g., \"stage this hunk, implies 'j'\" if j is an\nallowed option and \"stage this hunk and quit\" otherwise? Stuff for a\nseparate series, I think.\n\nRené\n\n"},{"id":"527998","messageId":"0ea56923-2041-43bd-8c35-cc93c3c95c70@web.de","threadId":"64238","inReplyTo":"xmqqh5wdrrub.fsf@gitster.g","subject":"Re: [PATCH v2 4/5] add-patch: let options k and K roll over like j and J","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2025-10-06T17:18:04Z","receivedAt":"2025-10-06T17:18:12Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"On 10/5/25 10:55 PM, Junio C Hamano wrote:\n> René Scharfe writes:\n> \n>> @@ -1584,7 +1591,8 @@ static int patch_update_file(struct add_p_state *s,\n>> \t\t\t}\n>> \t\t} else if (s->answer.buf[0] == 'K') {\n>> \t\t\tif (permitted & ALLOW_GOTO_PREVIOUS_HUNK)\n>> -\t\t\t\thunk_index--;\n>> +\t\t\t\thunk_index = dec_mod(hunk_index,\n>> +\t\t\t\t\t\t file_diff->hunk_nr);\n>> \t\t\telse\n>> \t\t\t\terr(s, _(\"No previous hunk\"));\n> \n> I was wondering if we want to always allow J and K; even when you\n> have only one hunk, you can still wrap around to come back to the\n> current hunk, and that we can do without any extra checking logic.\n> \n> But it is also OK to require 2 or more hunks to \"switch\" to the\n> other hunk, which is what you do with\n> \n> \t\t\tif (file_diff->hunk_nr > 1) {\n> \t\t\t\tpermitted |= ALLOW_GOTO_PREVIOUS_HUNK;\n> \t\t\t\tstrbuf_addstr(&s->buf, \",K\");\n> \t\t\t}\n> \n> to require more than 1. But the error message \"No previous hunk\"\n> sounds somewhat awkward. If user accepts the circular nature of how\n> we decide what \"previous\" is, then when we have a single hunk, the\n> current hunk itself _is_ the previous hunk, but because we insist\n> that there are at least 2, that interpretation would not work. With\n> \"wraparound\" semantics, \"No other hunk(s)\", would be a better way to\n> give the error, no? The same comment applies to 'J'.\n\nOK.\n>> \t\t} else if (s->answer.buf[0] == 'J') {\n> \n> This makes perfect sense, but then, after this post-context we have this:\n> \n> \t\t\tif (permitted & ALLOW_GOTO_NEXT_HUNK)\n> \t\t\t\thunk_index++;\n> \t\t\telse\n> \t\t\t\terr(s, _(\"No next hunk\"));\n> \n> and it sticks out that the post-increment of hunk_index here is not\n> using inc_mod() for symmetry.\n\nThis symmetry _is_ tantalizing. Had the call originally, removed it\nbecause it was unnecessary and didn't fit the narrative.\n> I am wondering if with that updated (I would not say \"fixed\"), if we\n> can lose the \"oops we overflowed so let's wrap around\" belt-and-suspender\n> code at the beginning of the loop, i.e.\n> \n> \tfor (;;) {\n> \t\tenum {\n> \t\t\tALLOW_GOTO_PREVIOUS_HUNK = 1 << 0,\n> \t\t\t...\n> \t\t\tALLOW_EDIT = 1 << 6\n> \t\t} permitted = 0;\n> \n> \t\tif (hunk_index >= file_diff->hunk_nr)\n> \t\t\thunk_index = 0;\n> \n> or if there still are other code that rely on this \"oops we\n> overflowed\" adjustment?\nGood question, gave me the idea that a and d should roll over as well.\n\nOther than that there's just the so-called soft_increment, which would\nneed something like this:\n\ndiff --git a/add-patch.c b/add-patch.c\nindex b0389c5d5b..59a9eb586d 100644\n--- a/add-patch.c\n+++ b/add-patch.c\n@@ -1546,8 +1546,7 @@ static int patch_update_file(struct add_p_state *s,\n \t\tif (ch == 'y') {\n \t\t\thunk->use = USE_HUNK;\n soft_increment:\n-\t\t\thunk_index = undecided_next < 0 ?\n-\t\t\t\tfile_diff->hunk_nr : undecided_next;\n+\t\t\thunk_index = undecided_next < 0 ? 0 : undecided_next;\n \t\t} else if (ch == 'n') {\n \t\t\thunk->use = SKIP_HUNK;\n \t\t\tgoto soft_increment;\n\nOr undecided_next could be set to 0 before the if/else cascade, then\nthis becomes a simple assignment and we can get rid of the goto.\n\nCouldn't find a way to remove the back-to-square-1 check that would be\nsignificantly better overall and thus worth the hassle, though.\n\nRené\n\n"},{"id":"527999","messageId":"fe8e8097-2b05-4dd2-a754-f59e4ba5f95a@web.de","threadId":"64238","inReplyTo":"c72518099a3b465c8761e41210fe3fcb@ukr.de","subject":"[PATCH v3 0/6] add-patch: roll over to next undecided hunk","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2025-10-06T17:18:10Z","receivedAt":"2025-10-06T17:18:15Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Changes since v1:\n- added patch 5 for a and d\n- made error messages direction-neutral\n- removed stray \"only\" from commit message of patch 2\n\n add-patch: improve help for options j, J, k, and K\n add-patch: document that option J rolls over\n add-patch: let options y, n, j, and e roll over to next undecided\n add-patch: let options k and K roll over like j and J\n add-patch: let options a and d roll over like y and n\n add-patch: reset \"permitted\" at loop start\n\n Documentation/git-add.adoc | 8 ++--\n add-patch.c | 75 ++++++++++++++++++++++++++-----------\n t/t3701-add-interactive.sh | 76 ++++++++++++++++++++++++++++++--------\n 3 files changed, 118 insertions(+), 41 deletions(-)\n\nInterdiff against v2:\ndiff --git a/add-patch.c b/add-patch.c\nindex 7b489d0a75..45839ceac5 100644\n--- a/add-patch.c\n+++ b/add-patch.c\n@@ -1418,6 +1418,17 @@ static size_t inc_mod(size_t a, size_t m)\n \treturn a < m - 1 ? a + 1 : 0;\n }\n \n+static bool get_first_undecided(const struct file_diff *file_diff, size_t *idx)\n+{\n+\tfor (size_t i = 0; i < file_diff->hunk_nr; i++) {\n+\t\tif (file_diff->hunk[i].use == UNDECIDED_HUNK) {\n+\t\t\t*idx = i;\n+\t\t\treturn true;\n+\t\t}\n+\t}\n+\treturn false;\n+}\n+\n static int patch_update_file(struct add_p_state *s,\n \t\t\t struct file_diff *file_diff)\n {\n@@ -1573,6 +1584,8 @@ static int patch_update_file(struct add_p_state *s,\n \t\t\t\t\tif (hunk->use == UNDECIDED_HUNK)\n \t\t\t\t\t\thunk->use = USE_HUNK;\n \t\t\t\t}\n+\t\t\t\tif (!get_first_undecided(file_diff, &hunk_index))\n+\t\t\t\t\thunk_index = 0;\n \t\t\t} else if (hunk->use == UNDECIDED_HUNK) {\n \t\t\t\thunk->use = USE_HUNK;\n \t\t\t}\n@@ -1583,6 +1596,8 @@ static int patch_update_file(struct add_p_state *s,\n \t\t\t\t\tif (hunk->use == UNDECIDED_HUNK)\n \t\t\t\t\t\thunk->use = SKIP_HUNK;\n \t\t\t\t}\n+\t\t\t\tif (!get_first_undecided(file_diff, &hunk_index))\n+\t\t\t\t\thunk_index = 0;\n \t\t\t} else if (hunk->use == UNDECIDED_HUNK) {\n \t\t\t\thunk->use = SKIP_HUNK;\n \t\t\t}\n@@ -1595,22 +1610,22 @@ static int patch_update_file(struct add_p_state *s,\n \t\t\t\thunk_index = dec_mod(hunk_index,\n \t\t\t\t\t\t file_diff->hunk_nr);\n \t\t\telse\n-\t\t\t\terr(s, _(\"No previous hunk\"));\n+\t\t\t\terr(s, _(\"No other hunk\"));\n \t\t} else if (s->answer.buf[0] == 'J') {\n \t\t\tif (permitted & ALLOW_GOTO_NEXT_HUNK)\n \t\t\t\thunk_index++;\n \t\t\telse\n-\t\t\t\terr(s, _(\"No next hunk\"));\n+\t\t\t\terr(s, _(\"No other hunk\"));\n \t\t} else if (s->answer.buf[0] == 'k') {\n \t\t\tif (permitted & ALLOW_GOTO_PREVIOUS_UNDECIDED_HUNK)\n \t\t\t\thunk_index = undecided_previous;\n \t\t\telse\n-\t\t\t\terr(s, _(\"No previous hunk\"));\n+\t\t\t\terr(s, _(\"No other undecided hunk\"));\n \t\t} else if (s->answer.buf[0] == 'j') {\n \t\t\tif (permitted & ALLOW_GOTO_NEXT_UNDECIDED_HUNK)\n \t\t\t\thunk_index = undecided_next;\n \t\t\telse\n-\t\t\t\terr(s, _(\"No next hunk\"));\n+\t\t\t\terr(s, _(\"No other undecided hunk\"));\n \t\t} else if (s->answer.buf[0] == 'g') {\n \t\t\tchar *pend;\n \t\t\tunsigned long response;\ndiff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\nindex 8c24a76e59..403aaee356 100755\n--- a/t/t3701-add-interactive.sh\n+++ b/t/t3701-add-interactive.sh\n@@ -1364,24 +1364,24 @@ test_expect_success 'options J, K roll over' '\n \ttest_cmp expect actual\n '\n \n-test_expect_success 'options y, n, j, k, e roll over to next undecided (1)' '\n+test_expect_success 'options y, n, a, d, j, k, e roll over to next undecided (1)' '\n \ttest_write_lines a b c d e f g h i j k l m n o p q >file &&\n \tgit add file &&\n \ttest_write_lines X b c d e f g h X j k l m n o p X >file &&\n \ttest_set_editor : &&\n-\ttest_write_lines g3 y g3 n g3 j g3 e k q | git add -p >out &&\n-\ttest_write_lines 1 3 1 3 1 3 1 3 1 2 >expect &&\n+\ttest_write_lines g3 y g3 n g3 a g3 d g3 j g3 e k q | git add -p >out &&\n+\ttest_write_lines 1 3 1 3 1 3 1 3 1 3 1 3 1 2 >expect &&\n \tsed -n -e \"s-/.*--\" -e \"s/^(//p\" actual &&\n \ttest_cmp expect actual\n '\n \n-test_expect_success 'options y, n, j, k, e roll over to next undecided (2)' '\n+test_expect_success 'options y, n, a, d, j, k, e roll over to next undecided (2)' '\n \ttest_write_lines a b c d e f g h i j k l m n o p q >file &&\n \tgit add file &&\n \ttest_write_lines X b c d e f g h X j k l m n o p X >file &&\n \ttest_set_editor : &&\n-\ttest_write_lines y g3 y g3 n g3 j g3 e g1 k q | git add -p >out &&\n-\ttest_write_lines 1 2 3 2 3 2 3 2 3 2 1 2 >expect &&\n+\ttest_write_lines y g3 y g3 n g3 a g3 d g3 j g3 e g1 k q | git add -p >out &&\n+\ttest_write_lines 1 2 3 2 3 2 3 2 3 2 3 2 3 2 1 2 >expect &&\n \tsed -n -e \"s-/.*--\" -e \"s/^(//p\" actual &&\n \ttest_cmp expect actual\n '\n-- \n2.51.0\n"},{"id":"528000","messageId":"b5034851-65bd-49da-b270-48b68d9210ff@web.de","threadId":"64238","inReplyTo":"fe8e8097-2b05-4dd2-a754-f59e4ba5f95a@web.de","subject":"[PATCH v3 1/6] add-patch: improve help for options j, J, k, and K","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2025-10-06T17:19:23Z","receivedAt":"2025-10-06T17:19:25Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"The options j, J, k, and K don't affect the status of the current hunk.\nThey just go to a different one. This is true whether the current hunk\nis undecided or not. Avoid misunderstanding by no longer mentioning\nthe current hunk explicitly in their help texts.\n\nSigned-off-by: René Scharfe \n---\n Documentation/git-add.adoc | 8 ++++----\n add-patch.c | 8 ++++----\n 2 files changed, 8 insertions(+), 8 deletions(-)\n\ndiff --git a/Documentation/git-add.adoc b/Documentation/git-add.adoc\nindex ad629c46c5..3266ccf105 100644\n--- a/Documentation/git-add.adoc\n+++ b/Documentation/git-add.adoc\n@@ -342,10 +342,10 @@ patch::\n d - do not stage this hunk or any of the later hunks in the file\n g - select a hunk to go to\n / - search for a hunk matching the given regex\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+ 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 s - split the current hunk into smaller hunks\n e - manually edit the current hunk\n p - print the current hunk\ndiff --git a/add-patch.c b/add-patch.c\nindex b0389c5d5b..912266a3f8 100644\n--- a/add-patch.c\n+++ b/add-patch.c\n@@ -1397,10 +1397,10 @@ static size_t display_hunks(struct add_p_state *s,\n }\n \n static const char help_patch_remainder[] =\n-N_(\"j - leave this hunk undecided, see next undecided hunk\\n\"\n- \"J - leave this hunk undecided, see next hunk\\n\"\n- \"k - leave this hunk undecided, see previous undecided hunk\\n\"\n- \"K - leave this hunk undecided, see previous hunk\\n\"\n+N_(\"j - go to the next undecided hunk\\n\"\n+ \"J - go to the next hunk\\n\"\n+ \"k - go to the previous undecided hunk\\n\"\n+ \"K - go to the previous hunk\\n\"\n \"g - select a hunk to go to\\n\"\n \"/ - search for a hunk matching the given regex\\n\"\n \"s - split the current hunk into smaller hunks\\n\"\n-- \n2.51.0\n"},{"id":"528001","messageId":"6603453d-e2d3-423b-acff-c9cfe1fbbf82@web.de","threadId":"64238","inReplyTo":"fe8e8097-2b05-4dd2-a754-f59e4ba5f95a@web.de","subject":"[PATCH v3 2/6] add-patch: document that option J rolls over","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2025-10-06T17:20:31Z","receivedAt":"2025-10-06T17:20:38Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"The variable \"permitted\" is not reset after moving to a different hunk,\nso it only accumulates permission and doesn't necessarily reflect those\nof the current hunk. This may be a bug, but is actually useful with the\noption J, which can be used at the last hunk to roll over to the first\nhunk. Make this particular behavior official.\n\nAlso adjust the error message, as it will only be shown if there's just\na single hunk.\n\nSuggested-by: Junio C Hamano \nSigned-off-by: René Scharfe \n---\n Documentation/git-add.adoc | 2 +-\n add-patch.c | 6 +++---\n t/t3701-add-interactive.sh | 18 ++++++++++++++----\n 3 files changed, 18 insertions(+), 8 deletions(-)\n\ndiff --git a/Documentation/git-add.adoc b/Documentation/git-add.adoc\nindex 3266ccf105..5c05a3a7f9 100644\n--- a/Documentation/git-add.adoc\n+++ b/Documentation/git-add.adoc\n@@ -343,7 +343,7 @@ patch::\n g - select a hunk to go to\n / - search for a hunk matching the given regex\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 s - split the current hunk into smaller hunks\ndiff --git a/add-patch.c b/add-patch.c\nindex 912266a3f8..1f466ec9c0 100644\n--- a/add-patch.c\n+++ b/add-patch.c\n@@ -1398,7 +1398,7 @@ static size_t display_hunks(struct add_p_state *s,\n \n static const char help_patch_remainder[] =\n N_(\"j - go to the next undecided hunk\\n\"\n- \"J - go to the next hunk\\n\"\n+ \"J - go to the next hunk, roll over at the bottom\\n\"\n \"k - go to the previous undecided hunk\\n\"\n \"K - go to the previous hunk\\n\"\n \"g - select a hunk to go to\\n\"\n@@ -1493,7 +1493,7 @@ static int patch_update_file(struct add_p_state *s,\n \t\t\t\tpermitted |= ALLOW_GOTO_NEXT_UNDECIDED_HUNK;\n \t\t\t\tstrbuf_addstr(&s->buf, \",j\");\n \t\t\t}\n-\t\t\tif (hunk_index + 1 < file_diff->hunk_nr) {\n+\t\t\tif (file_diff->hunk_nr > 1) {\n \t\t\t\tpermitted |= ALLOW_GOTO_NEXT_HUNK;\n \t\t\t\tstrbuf_addstr(&s->buf, \",J\");\n \t\t\t}\n@@ -1584,7 +1584,7 @@ static int patch_update_file(struct add_p_state *s,\n \t\t\tif (permitted & ALLOW_GOTO_NEXT_HUNK)\n \t\t\t\thunk_index++;\n \t\t\telse\n-\t\t\t\terr(s, _(\"No next hunk\"));\n+\t\t\t\terr(s, _(\"No other hunk\"));\n \t\t} else if (s->answer.buf[0] == 'k') {\n \t\t\tif (permitted & ALLOW_GOTO_PREVIOUS_UNDECIDED_HUNK)\n \t\t\t\thunk_index = undecided_previous;\ndiff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\nindex d9fe289a7a..d5d2e120ab 100755\n--- a/t/t3701-add-interactive.sh\n+++ b/t/t3701-add-interactive.sh\n@@ -334,7 +334,7 @@ test_expect_success 'different prompts for mode change/deleted' '\n \tcat >expect <<-\\EOF &&\n \t(1/1) Stage deletion [y,n,q,a,d,p,?]?\n \t(1/2) Stage mode change [y,n,q,a,d,j,J,g,/,p,?]?\n-\t(2/2) Stage this hunk [y,n,q,a,d,K,g,/,e,p,?]?\n+\t(2/2) Stage this hunk [y,n,q,a,d,K,J,g,/,e,p,?]?\n \tEOF\n \ttest_cmp expect actual.filtered\n '\n@@ -521,7 +521,7 @@ test_expect_success 'split hunk setup' '\n test_expect_success 'goto hunk 1 with \"g 1\"' '\n \ttest_when_finished \"git reset\" &&\n \ttr _ \" \" >expect <<-EOF &&\n-\t(2/2) Stage this hunk [y,n,q,a,d,K,g,/,e,p,?]? + 1: -1,2 +1,3 +15\n+\t(2/2) Stage this hunk [y,n,q,a,d,K,J,g,/,e,p,?]? + 1: -1,2 +1,3 +15\n \t_ 2: -2,4 +3,8 +21\n \tgo to which hunk? @@ -1,2 +1,3 @@\n \t_10\n@@ -550,7 +550,7 @@ test_expect_success 'goto hunk 1 with \"g1\"' '\n test_expect_success 'navigate to hunk via regex /pattern' '\n \ttest_when_finished \"git reset\" &&\n \ttr _ \" \" >expect <<-EOF &&\n-\t(2/2) Stage this hunk [y,n,q,a,d,K,g,/,e,p,?]? @@ -1,2 +1,3 @@\n+\t(2/2) Stage this hunk [y,n,q,a,d,K,J,g,/,e,p,?]? @@ -1,2 +1,3 @@\n \t_10\n \t+15\n \t_20\n@@ -805,7 +805,7 @@ test_expect_success 'colors can be overridden' '\n \t(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]? @@ -3 +3,2 @@\n \t more-context\n \t+another-one\n-\t(2/2) Stage this hunk [y,n,q,a,d,K,g,/,e,p,?]? @@ -1,3 +1,3 @@\n+\t(2/2) Stage this hunk [y,n,q,a,d,K,J,g,/,e,p,?]? @@ -1,3 +1,3 @@\n \t context\n \t-old\n \t+new\n@@ -1354,4 +1354,14 @@ do\n \t'\n done\n \n+test_expect_success 'option J rolls over' '\n+\ttest_write_lines a b c d e f g h i >file &&\n+\tgit add file &&\n+\ttest_write_lines X b c d e f g h X >file &&\n+\ttest_write_lines J J q | git add -p >out &&\n+\ttest_write_lines 1 2 1 >expect &&\n+\tsed -n -e \"s-/.*--\" -e \"s/^(//p\" actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.51.0\n"},{"id":"528002","messageId":"02128b8e-74dc-4347-89d7-00dcef5dda8b@web.de","threadId":"64238","inReplyTo":"fe8e8097-2b05-4dd2-a754-f59e4ba5f95a@web.de","subject":"[PATCH v3 3/6] add-patch: let options y, n, j, and e roll over to next undecided","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2025-10-06T17:21:19Z","receivedAt":"2025-10-06T17:21:24Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"The options y, n, and e mark the current hunk as decided. If there's\nanother undecided hunk towards the bottom of the hunk array they go\nthere. If there isn't, but there is another undecided hunk towards the\ntop then they go to the very first hunk, no matter if it has already\nbeen decided on.\n\nThe option j does basically the same move. Technically it is not\nallowed if there's no undecided hunk towards the bottom, but the\nvariable \"permitted\" is never reset, so this permission is retained\nfrom the very first hunk. That may a bug, but this behavior is at\nleast consistent with y, n, and e and arguably more useful than\nrefusing to move.\n\nImprove the roll-over behavior of these four options by moving to the\nfirst undecided hunk instead of hunk 1, consistent with what they do\nwhen not rolling over.\n\nAlso adjust the error message for j, as it will only be shown if\nthere's no other undecided hunk in either direction.\n\nReported-by: Windl, Ulrich \nSigned-off-by: René Scharfe \n---\n Documentation/git-add.adoc | 2 +-\n add-patch.c | 13 ++++++++++---\n t/t3701-add-interactive.sh | 22 ++++++++++++++++++++++\n 3 files changed, 33 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/git-add.adoc b/Documentation/git-add.adoc\nindex 5c05a3a7f9..596cdeff93 100644\n--- a/Documentation/git-add.adoc\n+++ b/Documentation/git-add.adoc\n@@ -342,7 +342,7 @@ patch::\n d - do not stage this hunk or any of the later hunks in the file\n g - select a hunk to go to\n / - search for a hunk matching the given regex\n- j - go to the next undecided hunk\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\ndiff --git a/add-patch.c b/add-patch.c\nindex 1f466ec9c0..106bfcb275 100644\n--- a/add-patch.c\n+++ b/add-patch.c\n@@ -1397,7 +1397,7 @@ static size_t display_hunks(struct add_p_state *s,\n }\n \n static const char help_patch_remainder[] =\n-N_(\"j - go to the next undecided hunk\\n\"\n+N_(\"j - go to the next undecided hunk, roll over at the bottom\\n\"\n \"J - go to the next hunk, roll over at the bottom\\n\"\n \"k - go to the previous undecided hunk\\n\"\n \"K - go to the previous hunk\\n\"\n@@ -1408,6 +1408,11 @@ N_(\"j - go to the next undecided hunk\\n\"\n \"p - print the current hunk, 'P' to use the pager\\n\"\n \"? - print help\\n\");\n \n+static size_t inc_mod(size_t a, size_t m)\n+{\n+\treturn a < m - 1 ? a + 1 : 0;\n+}\n+\n static int patch_update_file(struct add_p_state *s,\n \t\t\t struct file_diff *file_diff)\n {\n@@ -1451,7 +1456,9 @@ static int patch_update_file(struct add_p_state *s,\n \t\t\t\t\tbreak;\n \t\t\t\t}\n \n-\t\t\tfor (i = hunk_index + 1; i < file_diff->hunk_nr; i++)\n+\t\t\tfor (i = inc_mod(hunk_index, file_diff->hunk_nr);\n+\t\t\t i != hunk_index;\n+\t\t\t i = inc_mod(i, file_diff->hunk_nr))\n \t\t\t\tif (file_diff->hunk[i].use == UNDECIDED_HUNK) {\n \t\t\t\t\tundecided_next = i;\n \t\t\t\t\tbreak;\n@@ -1594,7 +1601,7 @@ static int patch_update_file(struct add_p_state *s,\n \t\t\tif (permitted & ALLOW_GOTO_NEXT_UNDECIDED_HUNK)\n \t\t\t\thunk_index = undecided_next;\n \t\t\telse\n-\t\t\t\terr(s, _(\"No next hunk\"));\n+\t\t\t\terr(s, _(\"No other undecided hunk\"));\n \t\t} else if (s->answer.buf[0] == 'g') {\n \t\t\tchar *pend;\n \t\t\tunsigned long response;\ndiff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\nindex d5d2e120ab..8086d3da71 100755\n--- a/t/t3701-add-interactive.sh\n+++ b/t/t3701-add-interactive.sh\n@@ -1364,4 +1364,26 @@ test_expect_success 'option J rolls over' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'options y, n, j, e roll over to next undecided (1)' '\n+\ttest_write_lines a b c d e f g h i j k l m n o p q >file &&\n+\tgit add file &&\n+\ttest_write_lines X b c d e f g h X j k l m n o p X >file &&\n+\ttest_set_editor : &&\n+\ttest_write_lines g3 y g3 n g3 j g3 e q | git add -p >out &&\n+\ttest_write_lines 1 3 1 3 1 3 1 3 1 >expect &&\n+\tsed -n -e \"s-/.*--\" -e \"s/^(//p\" actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'options y, n, j, e roll over to next undecided (2)' '\n+\ttest_write_lines a b c d e f g h i j k l m n o p q >file &&\n+\tgit add file &&\n+\ttest_write_lines X b c d e f g h X j k l m n o p X >file &&\n+\ttest_set_editor : &&\n+\ttest_write_lines y g3 y g3 n g3 j g3 e q | git add -p >out &&\n+\ttest_write_lines 1 2 3 2 3 2 3 2 3 2 >expect &&\n+\tsed -n -e \"s-/.*--\" -e \"s/^(//p\" actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.51.0\n"},{"id":"528004","messageId":"f46cd8f4-5382-4879-963d-3a31ed1552a7@web.de","threadId":"64238","inReplyTo":"fe8e8097-2b05-4dd2-a754-f59e4ba5f95a@web.de","subject":"[PATCH v3 4/6] add-patch: let options k and K roll over like j and J","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2025-10-06T17:22:38Z","receivedAt":"2025-10-06T17:22:50Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Options j and J roll over at the bottom and go to the first undecided\nhunk and hunk 1, respectively. Let options k and K do the same when\nthey reach the top of the hunk array, so let them go to the last\nundecided hunk and the last hunk, respectively, for consistency. Also\nuse the same direction-neutral error messages.\n\nSigned-off-by: René Scharfe \n---\n Documentation/git-add.adoc | 4 ++--\n add-patch.c | 22 ++++++++++++++-------\n t/t3701-add-interactive.sh | 40 +++++++++++++++++++-------------------\n 3 files changed, 37 insertions(+), 29 deletions(-)\n\ndiff --git a/Documentation/git-add.adoc b/Documentation/git-add.adoc\nindex 596cdeff93..3116a2cac5 100644\n--- a/Documentation/git-add.adoc\n+++ b/Documentation/git-add.adoc\n@@ -344,8 +344,8 @@ patch::\n / - search for a hunk matching the given regex\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 s - split the current hunk into smaller hunks\n e - manually edit the current hunk\n p - print the current hunk\ndiff --git a/add-patch.c b/add-patch.c\nindex 106bfcb275..4f314c16ec 100644\n--- a/add-patch.c\n+++ b/add-patch.c\n@@ -1399,8 +1399,8 @@ static size_t display_hunks(struct add_p_state *s,\n static const char help_patch_remainder[] =\n N_(\"j - go to the next undecided hunk, roll over at the bottom\\n\"\n \"J - go to the next hunk, roll over at the bottom\\n\"\n- \"k - go to the previous undecided hunk\\n\"\n- \"K - go to the previous hunk\\n\"\n+ \"k - go to the previous undecided hunk, roll over at the top\\n\"\n+ \"K - go to the previous hunk, roll over at the top\\n\"\n \"g - select a hunk to go to\\n\"\n \"/ - search for a hunk matching the given regex\\n\"\n \"s - split the current hunk into smaller hunks\\n\"\n@@ -1408,6 +1408,11 @@ N_(\"j - go to the next undecided hunk, roll over at the bottom\\n\"\n \"p - print the current hunk, 'P' to use the pager\\n\"\n \"? - print help\\n\");\n \n+static size_t dec_mod(size_t a, size_t m)\n+{\n+\treturn a > 0 ? a - 1 : m - 1;\n+}\n+\n static size_t inc_mod(size_t a, size_t m)\n {\n \treturn a < m - 1 ? a + 1 : 0;\n@@ -1450,7 +1455,9 @@ static int patch_update_file(struct add_p_state *s,\n \t\tundecided_next = -1;\n \n \t\tif (file_diff->hunk_nr) {\n-\t\t\tfor (i = hunk_index - 1; i >= 0; i--)\n+\t\t\tfor (i = dec_mod(hunk_index, file_diff->hunk_nr);\n+\t\t\t i != hunk_index;\n+\t\t\t i = dec_mod(i, file_diff->hunk_nr))\n \t\t\t\tif (file_diff->hunk[i].use == UNDECIDED_HUNK) {\n \t\t\t\t\tundecided_previous = i;\n \t\t\t\t\tbreak;\n@@ -1492,7 +1499,7 @@ static int patch_update_file(struct add_p_state *s,\n \t\t\t\tpermitted |= ALLOW_GOTO_PREVIOUS_UNDECIDED_HUNK;\n \t\t\t\tstrbuf_addstr(&s->buf, \",k\");\n \t\t\t}\n-\t\t\tif (hunk_index) {\n+\t\t\tif (file_diff->hunk_nr > 1) {\n \t\t\t\tpermitted |= ALLOW_GOTO_PREVIOUS_HUNK;\n \t\t\t\tstrbuf_addstr(&s->buf, \",K\");\n \t\t\t}\n@@ -1584,9 +1591,10 @@ static int patch_update_file(struct add_p_state *s,\n \t\t\t}\n \t\t} else if (s->answer.buf[0] == 'K') {\n \t\t\tif (permitted & ALLOW_GOTO_PREVIOUS_HUNK)\n-\t\t\t\thunk_index--;\n+\t\t\t\thunk_index = dec_mod(hunk_index,\n+\t\t\t\t\t\t file_diff->hunk_nr);\n \t\t\telse\n-\t\t\t\terr(s, _(\"No previous hunk\"));\n+\t\t\t\terr(s, _(\"No other hunk\"));\n \t\t} else if (s->answer.buf[0] == 'J') {\n \t\t\tif (permitted & ALLOW_GOTO_NEXT_HUNK)\n \t\t\t\thunk_index++;\n@@ -1596,7 +1604,7 @@ static int patch_update_file(struct add_p_state *s,\n \t\t\tif (permitted & ALLOW_GOTO_PREVIOUS_UNDECIDED_HUNK)\n \t\t\t\thunk_index = undecided_previous;\n \t\t\telse\n-\t\t\t\terr(s, _(\"No previous hunk\"));\n+\t\t\t\terr(s, _(\"No other undecided hunk\"));\n \t\t} else if (s->answer.buf[0] == 'j') {\n \t\t\tif (permitted & ALLOW_GOTO_NEXT_UNDECIDED_HUNK)\n \t\t\t\thunk_index = undecided_next;\ndiff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\nindex 8086d3da71..385e55c783 100755\n--- a/t/t3701-add-interactive.sh\n+++ b/t/t3701-add-interactive.sh\n@@ -333,7 +333,7 @@ test_expect_success 'different prompts for mode change/deleted' '\n \tsed -n \"s/^\\(([0-9/]*) Stage .*?\\).*/\\1/p\" actual >actual.filtered &&\n \tcat >expect <<-\\EOF &&\n \t(1/1) Stage deletion [y,n,q,a,d,p,?]?\n-\t(1/2) Stage mode change [y,n,q,a,d,j,J,g,/,p,?]?\n+\t(1/2) Stage mode change [y,n,q,a,d,k,K,j,J,g,/,p,?]?\n \t(2/2) Stage this hunk [y,n,q,a,d,K,J,g,/,e,p,?]?\n \tEOF\n \ttest_cmp expect actual.filtered\n@@ -527,7 +527,7 @@ test_expect_success 'goto hunk 1 with \"g 1\"' '\n \t_10\n \t+15\n \t_20\n-\t(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]?_\n+\t(1/2) Stage this hunk [y,n,q,a,d,k,K,j,J,g,/,e,p,?]?_\n \tEOF\n \ttest_write_lines s y g 1 | git add -p >actual &&\n \ttail -n 7 actual.trimmed &&\n@@ -540,7 +540,7 @@ test_expect_success 'goto hunk 1 with \"g1\"' '\n \t_10\n \t+15\n \t_20\n-\t(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]?_\n+\t(1/2) Stage this hunk [y,n,q,a,d,k,K,j,J,g,/,e,p,?]?_\n \tEOF\n \ttest_write_lines s y g1 | git add -p >actual &&\n \ttail -n 4 actual.trimmed &&\n@@ -554,7 +554,7 @@ test_expect_success 'navigate to hunk via regex /pattern' '\n \t_10\n \t+15\n \t_20\n-\t(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]?_\n+\t(1/2) Stage this hunk [y,n,q,a,d,k,K,j,J,g,/,e,p,?]?_\n \tEOF\n \ttest_write_lines s y /1,2 | git add -p >actual &&\n \ttail -n 5 actual.trimmed &&\n@@ -567,7 +567,7 @@ test_expect_success 'navigate to hunk via regex / pattern' '\n \t_10\n \t+15\n \t_20\n-\t(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]?_\n+\t(1/2) Stage this hunk [y,n,q,a,d,k,K,j,J,g,/,e,p,?]?_\n \tEOF\n \ttest_write_lines s y / 1,2 | git add -p >actual &&\n \ttail -n 4 actual.trimmed &&\n@@ -579,11 +579,11 @@ test_expect_success 'print again the hunk' '\n \ttr _ \" \" >expect <<-EOF &&\n \t+15\n \t 20\n-\t(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]? @@ -1,2 +1,3 @@\n+\t(1/2) Stage this hunk [y,n,q,a,d,k,K,j,J,g,/,e,p,?]? @@ -1,2 +1,3 @@\n \t 10\n \t+15\n \t 20\n-\t(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]?_\n+\t(1/2) Stage this hunk [y,n,q,a,d,k,K,j,J,g,/,e,p,?]?_\n \tEOF\n \ttest_write_lines s y g 1 p | git add -p >actual &&\n \ttail -n 7 actual.trimmed &&\n@@ -595,11 +595,11 @@ test_expect_success TTY 'print again the hunk (PAGER)' '\n \tcat >expect <<-EOF &&\n \t+15\n \t 20\n-\t(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]? PAGER @@ -1,2 +1,3 @@\n+\t(1/2) Stage this hunk [y,n,q,a,d,k,K,j,J,g,/,e,p,?]? PAGER @@ -1,2 +1,3 @@\n \tPAGER 10\n \tPAGER +15\n \tPAGER 20\n-\t(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]? \n+\t(1/2) Stage this hunk [y,n,q,a,d,k,K,j,J,g,/,e,p,?]? \n \tEOF\n \ttest_write_lines s y g 1 P |\n \t(\n@@ -802,7 +802,7 @@ test_expect_success 'colors can be overridden' '\n \t-old\n \t+new\n \t more-context\n-\t(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]? @@ -3 +3,2 @@\n+\t(1/2) Stage this hunk [y,n,q,a,d,k,K,j,J,g,/,e,p,?]? @@ -3 +3,2 @@\n \t more-context\n \t+another-one\n \t(2/2) Stage this hunk [y,n,q,a,d,K,J,g,/,e,p,?]? @@ -1,3 +1,3 @@\n@@ -810,7 +810,7 @@ test_expect_success 'colors can be overridden' '\n \t-old\n \t+new\n \t more-context\n-\t(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]? \n+\t(1/2) Stage this hunk [y,n,q,a,d,k,K,j,J,g,/,e,p,?]? \n \tEOF\n \ttest_cmp expect actual\n '\n@@ -1354,34 +1354,34 @@ do\n \t'\n done\n \n-test_expect_success 'option J rolls over' '\n+test_expect_success 'options J, K roll over' '\n \ttest_write_lines a b c d e f g h i >file &&\n \tgit add file &&\n \ttest_write_lines X b c d e f g h X >file &&\n-\ttest_write_lines J J q | git add -p >out &&\n-\ttest_write_lines 1 2 1 >expect &&\n+\ttest_write_lines J J K q | git add -p >out &&\n+\ttest_write_lines 1 2 1 2 >expect &&\n \tsed -n -e \"s-/.*--\" -e \"s/^(//p\" actual &&\n \ttest_cmp expect actual\n '\n \n-test_expect_success 'options y, n, j, e roll over to next undecided (1)' '\n+test_expect_success 'options y, n, j, k, e roll over to next undecided (1)' '\n \ttest_write_lines a b c d e f g h i j k l m n o p q >file &&\n \tgit add file &&\n \ttest_write_lines X b c d e f g h X j k l m n o p X >file &&\n \ttest_set_editor : &&\n-\ttest_write_lines g3 y g3 n g3 j g3 e q | git add -p >out &&\n-\ttest_write_lines 1 3 1 3 1 3 1 3 1 >expect &&\n+\ttest_write_lines g3 y g3 n g3 j g3 e k q | git add -p >out &&\n+\ttest_write_lines 1 3 1 3 1 3 1 3 1 2 >expect &&\n \tsed -n -e \"s-/.*--\" -e \"s/^(//p\" actual &&\n \ttest_cmp expect actual\n '\n \n-test_expect_success 'options y, n, j, e roll over to next undecided (2)' '\n+test_expect_success 'options y, n, j, k, e roll over to next undecided (2)' '\n \ttest_write_lines a b c d e f g h i j k l m n o p q >file &&\n \tgit add file &&\n \ttest_write_lines X b c d e f g h X j k l m n o p X >file &&\n \ttest_set_editor : &&\n-\ttest_write_lines y g3 y g3 n g3 j g3 e q | git add -p >out &&\n-\ttest_write_lines 1 2 3 2 3 2 3 2 3 2 >expect &&\n+\ttest_write_lines y g3 y g3 n g3 j g3 e g1 k q | git add -p >out &&\n+\ttest_write_lines 1 2 3 2 3 2 3 2 3 2 1 2 >expect &&\n \tsed -n -e \"s-/.*--\" -e \"s/^(//p\" actual &&\n \ttest_cmp expect actual\n '\n-- \n2.51.0\n"},{"id":"528005","messageId":"a00f7c63-0d29-4a7c-bef1-bd7bf94d3420@web.de","threadId":"64238","inReplyTo":"fe8e8097-2b05-4dd2-a754-f59e4ba5f95a@web.de","subject":"[PATCH v3 5/6] add-patch: let options a and d roll over like y and n","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2025-10-06T17:23:34Z","receivedAt":"2025-10-06T17:23:40Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Options a and d stage and unstage all undecided hunks towards the bottom\nof the array of hunks, respectively, and then roll over to the very\nfirst hunk. The first part is similar to y and n if the current hunk is\nthe last one in the array, but they roll over to the next undecided\nhunk if there is any. That's more useful; do it for a and d as well.\n\nSigned-off-by: René Scharfe \n---\n add-patch.c | 15 +++++++++++++++\n t/t3701-add-interactive.sh | 12 ++++++------\n 2 files changed, 21 insertions(+), 6 deletions(-)\n\ndiff --git a/add-patch.c b/add-patch.c\nindex 4f314c16ec..6da13a78b5 100644\n--- a/add-patch.c\n+++ b/add-patch.c\n@@ -1418,6 +1418,17 @@ static size_t inc_mod(size_t a, size_t m)\n \treturn a < m - 1 ? a + 1 : 0;\n }\n \n+static bool get_first_undecided(const struct file_diff *file_diff, size_t *idx)\n+{\n+\tfor (size_t i = 0; i < file_diff->hunk_nr; i++) {\n+\t\tif (file_diff->hunk[i].use == UNDECIDED_HUNK) {\n+\t\t\t*idx = i;\n+\t\t\treturn true;\n+\t\t}\n+\t}\n+\treturn false;\n+}\n+\n static int patch_update_file(struct add_p_state *s,\n \t\t\t struct file_diff *file_diff)\n {\n@@ -1572,6 +1583,8 @@ static int patch_update_file(struct add_p_state *s,\n \t\t\t\t\tif (hunk->use == UNDECIDED_HUNK)\n \t\t\t\t\t\thunk->use = USE_HUNK;\n \t\t\t\t}\n+\t\t\t\tif (!get_first_undecided(file_diff, &hunk_index))\n+\t\t\t\t\thunk_index = 0;\n \t\t\t} else if (hunk->use == UNDECIDED_HUNK) {\n \t\t\t\thunk->use = USE_HUNK;\n \t\t\t}\n@@ -1582,6 +1595,8 @@ static int patch_update_file(struct add_p_state *s,\n \t\t\t\t\tif (hunk->use == UNDECIDED_HUNK)\n \t\t\t\t\t\thunk->use = SKIP_HUNK;\n \t\t\t\t}\n+\t\t\t\tif (!get_first_undecided(file_diff, &hunk_index))\n+\t\t\t\t\thunk_index = 0;\n \t\t\t} else if (hunk->use == UNDECIDED_HUNK) {\n \t\t\t\thunk->use = SKIP_HUNK;\n \t\t\t}\ndiff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\nindex 385e55c783..9d81b0542e 100755\n--- a/t/t3701-add-interactive.sh\n+++ b/t/t3701-add-interactive.sh\n@@ -1364,24 +1364,24 @@ test_expect_success 'options J, K roll over' '\n \ttest_cmp expect actual\n '\n \n-test_expect_success 'options y, n, j, k, e roll over to next undecided (1)' '\n+test_expect_success 'options y, n, a, d, j, k, e roll over to next undecided (1)' '\n \ttest_write_lines a b c d e f g h i j k l m n o p q >file &&\n \tgit add file &&\n \ttest_write_lines X b c d e f g h X j k l m n o p X >file &&\n \ttest_set_editor : &&\n-\ttest_write_lines g3 y g3 n g3 j g3 e k q | git add -p >out &&\n-\ttest_write_lines 1 3 1 3 1 3 1 3 1 2 >expect &&\n+\ttest_write_lines g3 y g3 n g3 a g3 d g3 j g3 e k q | git add -p >out &&\n+\ttest_write_lines 1 3 1 3 1 3 1 3 1 3 1 3 1 2 >expect &&\n \tsed -n -e \"s-/.*--\" -e \"s/^(//p\" actual &&\n \ttest_cmp expect actual\n '\n \n-test_expect_success 'options y, n, j, k, e roll over to next undecided (2)' '\n+test_expect_success 'options y, n, a, d, j, k, e roll over to next undecided (2)' '\n \ttest_write_lines a b c d e f g h i j k l m n o p q >file &&\n \tgit add file &&\n \ttest_write_lines X b c d e f g h X j k l m n o p X >file &&\n \ttest_set_editor : &&\n-\ttest_write_lines y g3 y g3 n g3 j g3 e g1 k q | git add -p >out &&\n-\ttest_write_lines 1 2 3 2 3 2 3 2 3 2 1 2 >expect &&\n+\ttest_write_lines y g3 y g3 n g3 a g3 d g3 j g3 e g1 k q | git add -p >out &&\n+\ttest_write_lines 1 2 3 2 3 2 3 2 3 2 3 2 3 2 1 2 >expect &&\n \tsed -n -e \"s-/.*--\" -e \"s/^(//p\" actual &&\n \ttest_cmp expect actual\n '\n-- \n2.51.0\n"},{"id":"528006","messageId":"ed73a585-5074-4e36-9f41-228909513237@web.de","threadId":"64238","inReplyTo":"fe8e8097-2b05-4dd2-a754-f59e4ba5f95a@web.de","subject":"[PATCH v3 6/6] add-patch: reset \"permitted\" at loop start","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2025-10-06T17:24:28Z","receivedAt":"2025-10-06T17:24:31Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Don't accumulate allowed options from any visited hunks, start fresh at\nthe top of the loop instead and only record the allowed options for the\ncurrent hunk.\n\nReported-by: Junio C Hamano \nSigned-off-by: René Scharfe \n---\n add-patch.c | 19 ++++++++++---------\n t/t3701-add-interactive.sh | 14 ++++++++++++++\n 2 files changed, 24 insertions(+), 9 deletions(-)\n\ndiff --git a/add-patch.c b/add-patch.c\nindex 6da13a78b5..45839ceac5 100644\n--- a/add-patch.c\n+++ b/add-patch.c\n@@ -1439,15 +1439,6 @@ static int patch_update_file(struct add_p_state *s,\n \tstruct child_process cp = CHILD_PROCESS_INIT;\n \tint colored = !!s->colored.len, quit = 0, use_pager = 0;\n \tenum prompt_mode_type prompt_mode_type;\n-\tenum {\n-\t\tALLOW_GOTO_PREVIOUS_HUNK = 1 << 0,\n-\t\tALLOW_GOTO_PREVIOUS_UNDECIDED_HUNK = 1 << 1,\n-\t\tALLOW_GOTO_NEXT_HUNK = 1 << 2,\n-\t\tALLOW_GOTO_NEXT_UNDECIDED_HUNK = 1 << 3,\n-\t\tALLOW_SEARCH_AND_GOTO = 1 << 4,\n-\t\tALLOW_SPLIT = 1 << 5,\n-\t\tALLOW_EDIT = 1 << 6\n-\t} permitted = 0;\n \n \t/* Empty added files have no hunks */\n \tif (!file_diff->hunk_nr && !file_diff->added)\n@@ -1457,6 +1448,16 @@ static int patch_update_file(struct add_p_state *s,\n \trender_diff_header(s, file_diff, colored, &s->buf);\n \tfputs(s->buf.buf, stdout);\n \tfor (;;) {\n+\t\tenum {\n+\t\t\tALLOW_GOTO_PREVIOUS_HUNK = 1 << 0,\n+\t\t\tALLOW_GOTO_PREVIOUS_UNDECIDED_HUNK = 1 << 1,\n+\t\t\tALLOW_GOTO_NEXT_HUNK = 1 << 2,\n+\t\t\tALLOW_GOTO_NEXT_UNDECIDED_HUNK = 1 << 3,\n+\t\t\tALLOW_SEARCH_AND_GOTO = 1 << 4,\n+\t\t\tALLOW_SPLIT = 1 << 5,\n+\t\t\tALLOW_EDIT = 1 << 6\n+\t\t} permitted = 0;\n+\n \t\tif (hunk_index >= file_diff->hunk_nr)\n \t\t\thunk_index = 0;\n \t\thunk = file_diff->hunk_nr\ndiff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\nindex 9d81b0542e..403aaee356 100755\n--- a/t/t3701-add-interactive.sh\n+++ b/t/t3701-add-interactive.sh\n@@ -1386,4 +1386,18 @@ test_expect_success 'options y, n, a, d, j, k, e roll over to next undecided (2)\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'invalid option s is rejected' '\n+\ttest_write_lines a b c d e f g h i j k >file &&\n+\tgit add file &&\n+\ttest_write_lines X b X d e f g h i j X >file &&\n+\ttest_write_lines j s q | git add -p >out &&\n+\tsed -ne \"s/ @@.*//\" -e \"s/ \\$//\" -e \"/^(/p\" actual &&\n+\tcat >expect <<-EOF &&\n+\t(1/2) Stage this hunk [y,n,q,a,d,k,K,j,J,g,/,s,e,p,?]?\n+\t(2/2) Stage this hunk [y,n,q,a,d,k,K,j,J,g,/,e,p,?]? Sorry, cannot split this hunk\n+\t(2/2) Stage this hunk [y,n,q,a,d,k,K,j,J,g,/,e,p,?]?\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.51.0\n"},{"id":"528010","messageId":"xmqq8qhnq5cz.fsf@gitster.g","threadId":"64238","inReplyTo":"16d5908b-bed6-4ad2-bb27-9c6523f904d0@web.de","subject":"Re: [PATCH v2 1/5] add-patch: improve help for options j, J, k, and K","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-10-06T17:58:52Z","receivedAt":"2025-10-06T17:58:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n> I see it more narrowly: This patch removes unnecessary references to the\n> hunk's status, while a y/n doc patch would add missing pieces.\n\nGood.\n\n> Hmm, would the help text need to adapt to whether the current hunk is\n> the last undecided one? E.g., \"stage this hunk, implies 'j'\" if j is an\n> allowed option and \"stage this hunk and quit\" otherwise? Stuff for a\n> separate series, I think.\n\nSounds good.\n"},{"id":"528011","messageId":"xmqq4isbq59z.fsf@gitster.g","threadId":"64238","inReplyTo":"fe8e8097-2b05-4dd2-a754-f59e4ba5f95a@web.de","subject":"Re: [PATCH v3 0/6] add-patch: roll over to next undecided hunk","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-10-06T18:00:40Z","receivedAt":"2025-10-06T18:00:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n> Changes since v1:\n> - added patch 5 for a and d\n> - made error messages direction-neutral\n> - removed stray \"only\" from commit message of patch 2\n>\n> add-patch: improve help for options j, J, k, and K\n> add-patch: document that option J rolls over\n> add-patch: let options y, n, j, and e roll over to next undecided\n> add-patch: let options k and K roll over like j and J\n> add-patch: let options a and d roll over like y and n\n> add-patch: reset \"permitted\" at loop start\n\nWill queue. Should we mark it for 'next'?\n\nThanks. \n"},{"id":"528036","messageId":"4f4e5627-0804-4194-98ae-3345c992862d@web.de","threadId":"64238","inReplyTo":"xmqq4isbq59z.fsf@gitster.g","subject":"Re: [PATCH v3 0/6] add-patch: roll over to next undecided hunk","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2025-10-06T20:05:25Z","receivedAt":"2025-10-06T20:05:31Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"On 10/6/25 8:00 PM, Junio C Hamano wrote:\n> René Scharfe writes:\n> \n>> Changes since v1:\n>> - added patch 5 for a and d\n>> - made error messages direction-neutral\n>> - removed stray \"only\" from commit message of patch 2\n>>\n>> add-patch: improve help for options j, J, k, and K\n>> add-patch: document that option J rolls over\n>> add-patch: let options y, n, j, and e roll over to next undecided\n>> add-patch: let options k and K roll over like j and J\n>> add-patch: let options a and d roll over like y and n\n>> add-patch: reset \"permitted\" at loop start\n> \n> Will queue. Should we mark it for 'next'?\n\nOh, already? Fine with me.\n\nRené\n\n"},{"id":"528046","messageId":"xmqqecrfofjn.fsf@gitster.g","threadId":"64238","inReplyTo":"4f4e5627-0804-4194-98ae-3345c992862d@web.de","subject":"Re: [PATCH v3 0/6] add-patch: roll over to next undecided hunk","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-10-06T22:01:48Z","receivedAt":"2025-10-06T22:01:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n> On 10/6/25 8:00 PM, Junio C Hamano wrote:\n>> René Scharfe writes:\n>> \n>>> Changes since v1:\n>>> - added patch 5 for a and d\n>>> - made error messages direction-neutral\n>>> - removed stray \"only\" from commit message of patch 2\n>>>\n>>> add-patch: improve help for options j, J, k, and K\n>>> add-patch: document that option J rolls over\n>>> add-patch: let options y, n, j, and e roll over to next undecided\n>>> add-patch: let options k and K roll over like j and J\n>>> add-patch: let options a and d roll over like y and n\n>>> add-patch: reset \"permitted\" at loop start\n>> \n>> Will queue. Should we mark it for 'next'?\n>\n> Oh, already? Fine with me.\n\nWas just double-checking if there are things you wanted to imrpove.\n\nThanks.\n"},{"id":"528249","messageId":"bd51d7df-f0f2-44f5-8ebc-c95b944994bd@gmail.com","threadId":"64238","inReplyTo":"fcc003d6-c71f-4c41-a3a1-c9364d3bca9c@web.de","subject":"Re: [PATCH] add-patch: roll over to next undecided hunk","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2025-10-08T13:47:21Z","receivedAt":"2025-10-08T13:47:25Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 03/10/2025 15:10, René Scharfe wrote:\n> On 10/3/25 3:41 PM, Phillip Wood wrote:\n>>\n>>> @@ -1436,8 +1436,15 @@ static int patch_update_file(struct add_p_state *s,\n>>>       render_diff_header(s, file_diff, colored, &s->buf);\n>>>       fputs(s->buf.buf, stdout);\n>>>       for (;;) {\n>>> -        if (hunk_index >= file_diff->hunk_nr)\n>>> +        if (hunk_index >= file_diff->hunk_nr) {\n>>>               hunk_index = 0;\n>>> +            for (i = 0; i < file_diff->hunk_nr; i++) {\n>>> +                if (file_diff->hunk[i].use == UNDECIDED_HUNK) {\n>>> +                    hunk_index = i;\n>>> +                    break;\n>>> +                }\n>>> +            }\n>>> +        }\n>>>           hunk = file_diff->hunk_nr\n>>>                   ? file_diff->hunk + hunk_index\n>>\n>> If there were no undecided hunks then this will be out of bounds\n>> because hunk_index >= file_diff->hunk_nr. Are we absolutely certain\n>> that we cannot reach this point without at least one hunk being\n>> undecided?\n> \n> The new loop only sets hunk_index if i < file_diff->hunk_nr. If\n> it finds no undecided hunk then it does nothing.\n\nExactly - that's what I was worried about. However I'd missed the fact \nthat we still set hunk_index to zero before the loop so I thought it was \nunchanged from file_diff->hunk_nr when in fact it is unchanged from zero \nwhich is safe.\n\n>>> +test_expect_success 'roll over to next undecided (1)' '\n>>> +    test_write_lines a b c d e f g h i j k l m n o p q >file &&\n>>> +    git add file &&\n>>> +    test_write_lines X b c d e f g h X j k l m n o p X >file &&\n>>> +    test_write_lines J y y q | git add -p >actual &&\n>>> +    test_write_lines 1 2 3 1 >expect &&\n>>> +    sed -ne \"s-/.*--\" -e \"s-^(--p\" hunks &&\n>>> +    test_cmp expect hunks\n>>> +'\n>>\n>> I'm not sure what this first test adds, the one below checks that we\n>> find the first undecided hunk which seems to be the important thing\n>> to check.\n> \n> It's a regression test for the case that the original code got\n> right by accident. It may seem superfluous, but I actually\n> triggered it in my first attempt at a fix.\n\nAh, interesting, I'd assumed it was superfluous but it seems it isn't.\n\nI've had a quick read through of what Junio has in \"seen\" from the last \niteration of this series and it looked like a nice improvement to the \nusability.\n\nThanks\n\nPhillip\n\n> René\n> \n\n"},{"id":"530022","messageId":"697bf0301cd9459195bdd3cc79e517ae@ukr.de","threadId":"64238","inReplyTo":"75b08ed6-4f0f-4ede-b84a-c2f1c3d15734@web.de","subject":"RE: [EXT] [PATCH v2 1/5] add-patch: improve help for options j, J, k, and K","fromName":"Windl, Ulrich","fromEmail":"u.windl@ukr.de","sentAt":"2025-10-31T10:08:14Z","receivedAt":"2025-10-31T10:09:27Z","isPatch":true,"sender":{"key":"u.windl@ukr.de","avatar":null},"body":"Hi!\n\nSorry for the delay, but I was on vacation without access to this mailbox.\n\nFor the patch\ndiff --git a/Documentation/git-add.adoc b/Documentation/git-add.adoc\nindex ad629c46c5..3266ccf105 100644\n\nI don't see an actual improvement, and I'd prefer the previous version of the doc.\nLikwise for\ndiff --git a/add-patch.c b/add-patch.c\nindex b0389c5d5b..912266a3f8 100644\n\nKind regards,\nUlrich Windl\n\n> -----Original Message-----\n> From: René Scharfe \n> Sent: Sunday, October 5, 2025 5:55 PM\n> To: git@vger.kernel.org\n> Cc: Windl, Ulrich ; Junio C Hamano ;\n> Phillip Wood \n> Subject: [EXT] [PATCH v2 1/5] add-patch: improve help for options j, J, k, and\n> K\n> \n> Sicherheits-Hinweis: Diese E-Mail wurde von einer Person außerhalb des UKR\n> gesendet. Seien Sie vorsichtig vor gefälschten Absendern, wenn Sie auf Links\n> klicken, Anhänge öffnen oder weitere Aktionen ausführen, bevor Sie die\n> Echtheit überprüft haben.\n> \n> The options j, J, k, and K don't affect the status of the current hunk.\n> They just go to a different one. This is true whether the current hunk\n> is undecided or not. Avoid misunderstanding by no longer mentioning\n> the current hunk explicitly in their help texts.\n> \n> Signed-off-by: René Scharfe \n> ---\n> Documentation/git-add.adoc | 8 ++++----\n> add-patch.c | 8 ++++----\n> 2 files changed, 8 insertions(+), 8 deletions(-)\n> \n> diff --git a/Documentation/git-add.adoc b/Documentation/git-add.adoc\n> index ad629c46c5..3266ccf105 100644\n> --- a/Documentation/git-add.adoc\n> +++ b/Documentation/git-add.adoc\n> @@ -342,10 +342,10 @@ patch::\n> d - do not stage this hunk or any of the later hunks in the file\n> g - select a hunk to go to\n> / - search for a hunk matching the given regex\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> + 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> s - split the current hunk into smaller hunks\n> e - manually edit the current hunk\n> p - print the current hunk\n> diff --git a/add-patch.c b/add-patch.c\n> index b0389c5d5b..912266a3f8 100644\n> --- a/add-patch.c\n> +++ b/add-patch.c\n> @@ -1397,10 +1397,10 @@ static size_t display_hunks(struct add_p_state\n> *s,\n> }\n> \n> static const char help_patch_remainder[] =\n> -N_(\"j - leave this hunk undecided, see next undecided hunk\\n\"\n> - \"J - leave this hunk undecided, see next hunk\\n\"\n> - \"k - leave this hunk undecided, see previous undecided hunk\\n\"\n> - \"K - leave this hunk undecided, see previous hunk\\n\"\n> +N_(\"j - go to the next undecided hunk\\n\"\n> + \"J - go to the next hunk\\n\"\n> + \"k - go to the previous undecided hunk\\n\"\n> + \"K - go to the previous hunk\\n\"\n> \"g - select a hunk to go to\\n\"\n> \"/ - search for a hunk matching the given regex\\n\"\n> \"s - split the current hunk into smaller hunks\\n\"\n> --\n> 2.51.0\n"},{"id":"530024","messageId":"77991a11c53f40b8b0a050a4d081809a@ukr.de","threadId":"64238","inReplyTo":"ed73a585-5074-4e36-9f41-228909513237@web.de","subject":"RE: [EXT] [PATCH v3 6/6] add-patch: reset \"permitted\" at loop start","fromName":"Windl, Ulrich","fromEmail":"u.windl@ukr.de","sentAt":"2025-10-31T10:28:47Z","receivedAt":"2025-10-31T10:28:50Z","isPatch":true,"sender":{"key":"u.windl@ukr.de","avatar":null},"body":"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).\n\n> -----Original Message-----\n> From: René Scharfe \n> Sent: Monday, October 6, 2025 7:24 PM\n> To: git@vger.kernel.org\n> Cc: Windl, Ulrich ; Junio C Hamano ;\n> Phillip Wood \n> Subject: [EXT] [PATCH v3 6/6] add-patch: reset \"permitted\" at loop start\n> \n[...] \n> \tfor (;;) {\n> +\t\tenum {\n> +\t\t\tALLOW_GOTO_PREVIOUS_HUNK = 1 << 0,\n> +\t\t\tALLOW_GOTO_PREVIOUS_UNDECIDED_HUNK = 1 <<\n> 1,\n> +\t\t\tALLOW_GOTO_NEXT_HUNK = 1 << 2,\n> +\t\t\tALLOW_GOTO_NEXT_UNDECIDED_HUNK = 1 << 3,\n> +\t\t\tALLOW_SEARCH_AND_GOTO = 1 << 4,\n> +\t\t\tALLOW_SPLIT = 1 << 5,\n> +\t\t\tALLOW_EDIT = 1 << 6\n> +\t\t} permitted = 0;\n> +\n> \t\tif (hunk_index >= file_diff->hunk_nr)\n> \t\t\thunk_index = 0;\n> \t\thunk = file_diff->hunk_nr\n\n"},{"id":"530036","messageId":"xmqqfraz2jb6.fsf@gitster.g","threadId":"64238","inReplyTo":"77991a11c53f40b8b0a050a4d081809a@ukr.de","subject":"Re: [EXT] [PATCH v3 6/6] add-patch: reset \"permitted\" at loop start","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-10-31T15:16:13Z","receivedAt":"2025-10-31T15:16:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Windl, Ulrich\" writes:\n\n> Just a comment of personal taste: I think declaring an anonymous\n> enum inside a loop is just bad style. I think that gcc is smart\n> enough to optimize if \"permitted\" is declared outside the loop, or\n> make the \"permitted\" use a typedef for a \"named enum\" (declared\n> outside the loop while the variable may be inside the loop).\n\nIf this is more than just a personal preference (which to me does\nsound like), a patch to improve it on top is very much welcomed.\n\nThe change itself would be just reverting the code movement, drop\nthe 0 initialization and resetting the ariable at the top of the\nloop every iteration. But the rationale being that it would give\ncompilers a chance to do a better job, I'd prefer to see a compiler\nperson write the proposed log message, possibly backed by data\n(perhaps \"generated assembly is objectively better---compare this\nand that\" in this case? I dunno).\n\nThanks.\n\n>> -----Original Message-----\n>> From: René Scharfe \n>> Sent: Monday, October 6, 2025 7:24 PM\n>> To: git@vger.kernel.org\n>> Cc: Windl, Ulrich ; Junio C Hamano ;\n>> Phillip Wood \n>> Subject: [EXT] [PATCH v3 6/6] add-patch: reset \"permitted\" at loop start\n>> \n> [...] \n>> \tfor (;;) {\n>> +\t\tenum {\n>> +\t\t\tALLOW_GOTO_PREVIOUS_HUNK = 1 << 0,\n>> +\t\t\tALLOW_GOTO_PREVIOUS_UNDECIDED_HUNK = 1 <<\n>> 1,\n>> +\t\t\tALLOW_GOTO_NEXT_HUNK = 1 << 2,\n>> +\t\t\tALLOW_GOTO_NEXT_UNDECIDED_HUNK = 1 << 3,\n>> +\t\t\tALLOW_SEARCH_AND_GOTO = 1 << 4,\n>> +\t\t\tALLOW_SPLIT = 1 << 5,\n>> +\t\t\tALLOW_EDIT = 1 << 6\n>> +\t\t} permitted = 0;\n>> +\n>> \t\tif (hunk_index >= file_diff->hunk_nr)\n>> \t\t\thunk_index = 0;\n>> \t\thunk = file_diff->hunk_nr\n"},{"id":"530058","messageId":"xmqqjz0axj1i.fsf@gitster.g","threadId":"64238","inReplyTo":"697bf0301cd9459195bdd3cc79e517ae@ukr.de","subject":"Re: [EXT] [PATCH v2 1/5] add-patch: improve help for options j, J, k, and K","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-11-01T08:18:33Z","receivedAt":"2025-11-01T08:18:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Windl, Ulrich\" writes:\n\n> For the patch\n> diff --git a/Documentation/git-add.adoc b/Documentation/git-add.adoc\n> index ad629c46c5..3266ccf105 100644\n>\n> I don't see an actual improvement, and I'd prefer the previous\n> version of the doc.\n\nWe'd prefer to see something more concrete that refuses the\nreasoning that led to the change, than a subjective \"I don't see,\nI'd prefer\".\n\nAt least, the commit log messge given by 2c3cc43f (add-patch:\nimprove help for options j, J, k, and K, 2025-10-06) explains why\nthe change is an improvement, and I found it sensible.\n() \n\nThe old description said 'j' leaves this hunk undecided and goes to\nthe next undecided hunk, but it is both pointless and misleading to\nsay 'leave this hunk undecided'. Unlike 'y' or 'n', the movement\noptions 'j', 'k' are not about changing the state of the current\nthing we are on (so it is pointless to say \"LEAVE it undecided\"),\nand more importantly, when we say 'j', the state of the current\nthing we are on may not necessarily be 'undecided' (so it is\nmisleading to say \"leave it UNDECIDED\").\n"},{"id":"530115","messageId":"61fcb89b5843474693ea6d6c90609180@ukr.de","threadId":"64238","inReplyTo":"xmqqjz0axj1i.fsf@gitster.g","subject":"RE: [EXT] Re: [PATCH v2 1/5] add-patch: improve help for options j, J, k, and K","fromName":"Windl, Ulrich","fromEmail":"u.windl@ukr.de","sentAt":"2025-11-03T12:43:10Z","receivedAt":"2025-11-03T12:43:19Z","isPatch":true,"sender":{"key":"u.windl@ukr.de","avatar":null},"body":"OK,\n\nfair enough: I think the original wording is more clear, so I don't see the need to change it at all.\nPossible corner cases ecepted, e.g. whether \"next\" can wrap at the end or not.\n\nKind regards,\nUlrich Windl\n\n> -----Original Message-----\n> From: Junio C Hamano \n> Sent: Saturday, November 1, 2025 9:19 AM\n> To: Windl, Ulrich \n> Cc: René Scharfe ; git@vger.kernel.org; Phillip Wood\n> \n> Subject: [EXT] Re: [PATCH v2 1/5] add-patch: improve help for options j, J, k,\n> and K\n> \n> Sicherheits-Hinweis: Diese E-Mail wurde von einer Person außerhalb des UKR\n> gesendet. Seien Sie vorsichtig vor gefälschten Absendern, wenn Sie auf Links\n> klicken, Anhänge öffnen oder weitere Aktionen ausführen, bevor Sie die\n> Echtheit überprüft haben.\n> \n> \"Windl, Ulrich\" writes:\n> \n> > For the patch\n> > diff --git a/Documentation/git-add.adoc b/Documentation/git-add.adoc\n> > index ad629c46c5..3266ccf105 100644\n> >\n> > I don't see an actual improvement, and I'd prefer the previous\n> > version of the doc.\n> \n> We'd prefer to see something more concrete that refuses the\n> reasoning that led to the change, than a subjective \"I don't see,\n> I'd prefer\".\n> \n> At least, the commit log messge given by 2c3cc43f (add-patch:\n> improve help for options j, J, k, and K, 2025-10-06) explains why\n> the change is an improvement, and I found it sensible.\n> ()\n> \n> The old description said 'j' leaves this hunk undecided and goes to\n> the next undecided hunk, but it is both pointless and misleading to\n> say 'leave this hunk undecided'. Unlike 'y' or 'n', the movement\n> options 'j', 'k' are not about changing the state of the current\n> thing we are on (so it is pointless to say \"LEAVE it undecided\"),\n> and more importantly, when we say 'j', the state of the current\n> thing we are on may not necessarily be 'undecided' (so it is\n> misleading to say \"leave it UNDECIDED\").\n"}]}