From: René Scharfe Date: Fri, 03 Oct 2025 19:53:00 GMT Subject: Re: [PATCH] add-patch: roll over to next undecided hunk Message-ID: <737e78f5-6337-4964-8385-9c35897f5dff@web.de> In-Reply-To: On 10/3/25 6:11 PM, Junio C Hamano wrote: > René Scharfe writes: > >> git add --patch presents diff hunks one after the other, asking whether >> to add them. If we mark some as undecided, e.g. with J, then it will > > Perhaps "mark" -> "leave". > > I somehow find it awkward to say "mark as undecided", as I have > always viewed J/K as a way to skip a hunk, leaving it undecided. > > Besides, "J" lets you revisit a hunk that you earlier have decided > to use of hold off, and it leaves your last decision on that hunk. > A statement that implies "J marks as undecided" is misleading. Right, j/J/k/K leave the use/skip/undecided status of the current hunk unchanged. "leave this hunk undecided" in the documentation is misleading as well, because these options will not leave a hunk undecided if we made a decision on it before: j - leave this hunk undecided, see next undecided hunk J - leave this hunk undecided, see next hunk k - leave this hunk undecided, see previous undecided hunk K - leave this hunk undecided, see previous hunk Perhaps omit it? j - go to next undecided hunk J - go to next hunk k - go to previous undecided hunk K - go to previous hunk Weird that one can switch between use and skip, but there's no way to revert back to undecided. >> start over after reaching the last hunk. It always starts over at the >> very first hunk, though, even if we already decided on it. Skip >> decided hunks when rolling over instead. > > Nicely analyzed. > >> Reported-by: Windl, Ulrich >> Signed-off-by: René Scharfe >> --- >> add-patch.c | 9 ++++++++- >> t/t3701-add-interactive.sh | 20 ++++++++++++++++++++ >> 2 files changed, 28 insertions(+), 1 deletion(-) >> >> diff --git a/add-patch.c b/add-patch.c >> index b0389c5d5b..42a8394c92 100644 >> --- a/add-patch.c >> +++ b/add-patch.c >> @@ -1436,8 +1436,15 @@ static int patch_update_file(struct add_p_state *s, >> render_diff_header(s, file_diff, colored, &s->buf); >> fputs(s->buf.buf, stdout); >> for (;;) { >> - if (hunk_index >= file_diff->hunk_nr) >> + if (hunk_index >= file_diff->hunk_nr) { >> hunk_index = 0; >> + for (i = 0; i < file_diff->hunk_nr; i++) { >> + if (file_diff->hunk[i].use == UNDECIDED_HUNK) { >> + hunk_index = i; >> + break; >> + } >> + } >> + } > > OK. > > This is probably a closely related tangent, but last night I was > looking this function and found that its per-hunk loop does > completely bogus thing. For example, find a case where you have > more than one hunks, among which there are splittable and > non-splittable hunks (a hunk is splittable if there are context > lines between an added or a removed line). Start cycling the hunks > without making any decisions with "J" or "K". Once you visited a > splittable hunk (where you'd see 's' among the possible choices), > coming back to an unsplittable hunk will now let you split it! 's' > may not be visible among the choices, but telling it to 's'plit will > give you "Split into 1", which is a technically correct nonsense. > > This is because the handling of "permitted" in that function only > adds, without resetting at the end of processing the current hunk. > Yet it does something like this: > > for (;;) { > ... > strbuf_reset(&s->buf); > if (file_diff->hunk_nr) { > ... add choices to the prompt ... > if (hunk->splittable_into > 1) { > permitted |= ALLOW_SPLIT; > strbuf_addstr(&s->buf, ",s"); > } > ... > } > ... > printf(_(s->mode->prompt_mode[prompt_mode_type]), > s->buf.buf); > if (*s->s.reset_color_interactive) > fputs(s->s.reset_color_interactive, stdout); > fflush(stdout); > if (read_single_character(s) == EOF) > break; > ch = tolower(s->answer.buf[0]); > ... dispatch on the command character ... > if (ch == 'y') { > ... > } else if (s->answer.buf[0] == 's') { > size_t splittable_into = hunk->splittable_into; > if (!(permitted & ALLOW_SPLIT)) { > err(s, _("Sorry, cannot split this hunk")); > } else if (!split_hunk(s, file_diff, > hunk - file_diff->hunk)) { > color_fprintf_ln(stdout, s->s.header_color, > _("Split into %d hunks."), > (int)splittable_into); > rendered_hunk_index = -1; > } > ... > > Notice that the prompt is built correctly but that information is > *not* used when deciding if the operation is possible? > > This is another ancient regression that was introduced while > rewriting this program in C near the end of 2019, I think. And this > causes many other bugs in this area, like 'k' at the very first hunk > gets complaint "No previous hunk" only once (you move to the next > one with 'j' and come back to the first hunk with 'k', and then 'k' > no longer complains, even though it is not among the choice). This should be easy to fix by resetting permitted at the start of the loop, no? Patch below. > With this bug, however, we have gained a bit of useful feature, I > think. Even though j/J should not be offered when we are at the > last hunk for a file, we do wrap-around to the first hunk. I just > checked the original code before the C rewrite, and even though it > were written defensively so that incrementing the current hunk > number to 5 when you have only 4 hunks would take you back to the > initial hunk (instead of barfing), because we did not have this > "permitted is never reset" bug, it actually did not allow you to go > beyond the end with j/J. Today's code seems to have inherited this > defensive adjustment to stay within the available hunks, and with > the "permitted is never reset" bug, we are taken back to the first > hunk. y/n/e on the last hunk roll over, which makes sense to me. Their movement part is not mentioned in the documentation, by the way. With the patch below j/J are stopped by the floor, as seemingly intended. Not sure if the (now accidental) roll-over behavior is better for them. add-patch.c | 19 ++++++++++--------- t/t3701-add-interactive.sh | 19 +++++++++++++++++++ 2 files changed, 29 insertions(+), 9 deletions(-) diff --git a/add-patch.c b/add-patch.c index 42a8394c92..1012840019 100644 --- a/add-patch.c +++ b/add-patch.c @@ -1418,15 +1418,6 @@ static int patch_update_file(struct add_p_state *s, struct child_process cp = CHILD_PROCESS_INIT; int colored = !!s->colored.len, quit = 0, use_pager = 0; enum prompt_mode_type prompt_mode_type; - enum { - ALLOW_GOTO_PREVIOUS_HUNK = 1 << 0, - ALLOW_GOTO_PREVIOUS_UNDECIDED_HUNK = 1 << 1, - ALLOW_GOTO_NEXT_HUNK = 1 << 2, - ALLOW_GOTO_NEXT_UNDECIDED_HUNK = 1 << 3, - ALLOW_SEARCH_AND_GOTO = 1 << 4, - ALLOW_SPLIT = 1 << 5, - ALLOW_EDIT = 1 << 6 - } permitted = 0; /* Empty added files have no hunks */ if (!file_diff->hunk_nr && !file_diff->added) @@ -1436,6 +1427,16 @@ static int patch_update_file(struct add_p_state *s, render_diff_header(s, file_diff, colored, &s->buf); fputs(s->buf.buf, stdout); for (;;) { + enum { + ALLOW_GOTO_PREVIOUS_HUNK = 1 << 0, + ALLOW_GOTO_PREVIOUS_UNDECIDED_HUNK = 1 << 1, + ALLOW_GOTO_NEXT_HUNK = 1 << 2, + ALLOW_GOTO_NEXT_UNDECIDED_HUNK = 1 << 3, + ALLOW_SEARCH_AND_GOTO = 1 << 4, + ALLOW_SPLIT = 1 << 5, + ALLOW_EDIT = 1 << 6 + } permitted = 0; + if (hunk_index >= file_diff->hunk_nr) { hunk_index = 0; for (i = 0; i < file_diff->hunk_nr; i++) { diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh index fa6ec5f835..33b307b8ff 100755 --- a/t/t3701-add-interactive.sh +++ b/t/t3701-add-interactive.sh @@ -1341,6 +1341,25 @@ test_expect_success 'roll over to next undecided (2)' ' test_cmp expect hunks ' +test_expect_success 'invalid options are rejected' ' + test_write_lines a b c d e f g h i j k >file && + git add file && + test_write_lines X b c d e f g h X j X >file && + test_write_lines j j J k k K s q | git add -p >out && + sed -ne "s/ @@.*//" -e "s/ \$//" -e "/^(/p" actual && + cat >expect <<-EOF && + (1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]? + (2/2) Stage this hunk [y,n,q,a,d,k,K,g,/,s,e,p,?]? No next hunk + (2/2) Stage this hunk [y,n,q,a,d,k,K,g,/,s,e,p,?]? No next hunk + (2/2) Stage this hunk [y,n,q,a,d,k,K,g,/,s,e,p,?]? + (1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]? No previous hunk + (1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]? No previous hunk + (1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]? Sorry, cannot split this hunk + (1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,p,?]? + EOF + test_cmp expect actual +' + test_expect_success 'set up base for -p color tests' ' echo commit >file && git commit -am "commit state" &&