From: Phillip Wood <phillip.wood123@gmail•com>
To: "René Scharfe" <l.s.r@web•de>,
phillip.wood@dunelm•org.uk, "Windl, Ulrich" <u.windl@ukr•de>,
"git@vger•kernel.org" <git@vger•kernel.org>
Cc: Junio C Hamano <gitster@pobox•com>
Subject: Re: [PATCH] add-patch: roll over to next undecided hunk
Date: Wed, 8 Oct 2025 14:47:21 +0100 [thread overview]
Message-ID: <bd51d7df-f0f2-44f5-8ebc-c95b944994bd@gmail.com> (raw)
In-Reply-To: <fcc003d6-c71f-4c41-a3a1-c9364d3bca9c@web.de>
On 03/10/2025 15:10, René Scharfe wrote:
> On 10/3/25 3:41 PM, Phillip Wood wrote:
>>
>>> @@ -1436,8 +1436,15 @@ static int patch_update_file(struct add_p_state *s,
>>> render_diff_header(s, file_diff, colored, &s->buf);
>>> fputs(s->buf.buf, stdout);
>>> for (;;) {
>>> - if (hunk_index >= file_diff->hunk_nr)
>>> + if (hunk_index >= file_diff->hunk_nr) {
>>> hunk_index = 0;
>>> + for (i = 0; i < file_diff->hunk_nr; i++) {
>>> + if (file_diff->hunk[i].use == UNDECIDED_HUNK) {
>>> + hunk_index = i;
>>> + break;
>>> + }
>>> + }
>>> + }
>>> hunk = file_diff->hunk_nr
>>> ? file_diff->hunk + hunk_index
>>
>> If there were no undecided hunks then this will be out of bounds
>> because hunk_index >= file_diff->hunk_nr. Are we absolutely certain
>> that we cannot reach this point without at least one hunk being
>> undecided?
>
> The new loop only sets hunk_index if i < file_diff->hunk_nr. If
> it finds no undecided hunk then it does nothing.
Exactly - that's what I was worried about. However I'd missed the fact
that we still set hunk_index to zero before the loop so I thought it was
unchanged from file_diff->hunk_nr when in fact it is unchanged from zero
which is safe.
>>> +test_expect_success 'roll over to next undecided (1)' '
>>> + test_write_lines a b c d e f g h i j k l m n o p q >file &&
>>> + git add file &&
>>> + test_write_lines X b c d e f g h X j k l m n o p X >file &&
>>> + test_write_lines J y y q | git add -p >actual &&
>>> + test_write_lines 1 2 3 1 >expect &&
>>> + sed -ne "s-/.*--" -e "s-^(--p" <actual >hunks &&
>>> + test_cmp expect hunks
>>> +'
>>
>> I'm not sure what this first test adds, the one below checks that we
>> find the first undecided hunk which seems to be the important thing
>> to check.
>
> It's a regression test for the case that the original code got
> right by accident. It may seem superfluous, but I actually
> triggered it in my first attempt at a fix.
Ah, interesting, I'd assumed it was superfluous but it seems it isn't.
I've had a quick read through of what Junio has in "seen" from the last
iteration of this series and it looked like a nice improvement to the
usability.
Thanks
Phillip
> René
>
next prev parent reply other threads:[~2025-10-08 13:47 UTC|newest]
Thread overview: 37+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-10-02 9:23 Broken handling of "J" hunks for "add --interactive"? Windl, Ulrich
2025-10-03 12:16 ` [PATCH] add-patch: roll over to next undecided hunk René Scharfe
2025-10-03 13:41 ` Phillip Wood
2025-10-03 14:10 ` René Scharfe
2025-10-08 13:47 ` Phillip Wood [this message]
2025-10-03 16:11 ` Junio C Hamano
2025-10-03 19:53 ` René Scharfe
2025-10-03 20:39 ` Junio C Hamano
2025-10-03 20:42 ` Junio C Hamano
2025-10-03 21:18 ` Junio C Hamano
2025-10-05 15:45 ` [PATCH v2 0/5] " René Scharfe
2025-10-05 15:55 ` [PATCH v2 1/5] add-patch: improve help for options j, J, k, and K René Scharfe
2025-10-05 21:30 ` Junio C Hamano
2025-10-06 17:17 ` René Scharfe
2025-10-06 17:58 ` Junio C Hamano
2025-10-31 10:08 ` [EXT] " Windl, Ulrich
2025-11-01 8:18 ` Junio C Hamano
2025-11-03 12:43 ` [EXT] " Windl, Ulrich
2025-10-05 15:55 ` [PATCH v2 2/5] add-patch: document that option J rolls over René Scharfe
2025-10-05 21:30 ` Junio C Hamano
2025-10-05 15:55 ` [PATCH v2 3/5] add-patch: let options y, n, j, and e roll over to next undecided René Scharfe
2025-10-05 15:55 ` [PATCH v2 4/5] add-patch: let options k and K roll over like j and J René Scharfe
2025-10-05 20:55 ` Junio C Hamano
2025-10-06 17:18 ` René Scharfe
2025-10-05 15:55 ` [PATCH v2 5/5] add-patch: reset "permitted" at loop start René Scharfe
2025-10-06 17:18 ` [PATCH v3 0/6] add-patch: roll over to next undecided hunk René Scharfe
2025-10-06 17:19 ` [PATCH v3 1/6] add-patch: improve help for options j, J, k, and K René Scharfe
2025-10-06 17:20 ` [PATCH v3 2/6] add-patch: document that option J rolls over René Scharfe
2025-10-06 17:21 ` [PATCH v3 3/6] add-patch: let options y, n, j, and e roll over to next undecided René Scharfe
2025-10-06 17:22 ` [PATCH v3 4/6] add-patch: let options k and K roll over like j and J René Scharfe
2025-10-06 17:23 ` [PATCH v3 5/6] add-patch: let options a and d roll over like y and n René Scharfe
2025-10-06 17:24 ` [PATCH v3 6/6] add-patch: reset "permitted" at loop start René Scharfe
2025-10-31 10:28 ` [EXT] " Windl, Ulrich
2025-10-31 15:16 ` Junio C Hamano
2025-10-06 18:00 ` [PATCH v3 0/6] add-patch: roll over to next undecided hunk Junio C Hamano
2025-10-06 20:05 ` René Scharfe
2025-10-06 22:01 ` Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=bd51d7df-f0f2-44f5-8ebc-c95b944994bd@gmail.com \
--to=phillip.wood123@gmail$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=gitster@pobox$(echo .)com \
--cc=l.s.r@web$(echo .)de \
--cc=phillip.wood@dunelm$(echo .)org.uk \
--cc=u.windl@ukr$(echo .)de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox