Re: [PATCH] add-patch: roll over to next undecided hunk
- From
- Phillip Wood <phillip.wood123@gmail.com>
- Date
- Oct 8, 2025, 13:47 UTC
- Message-ID
- <bd51d7df-f0f2-44f5-8ebc-c95b944994bd@gmail.com>
- In-Reply-To
- <fcc003d6-c71f-4c41-a3a1-c9364d3bca9c@web.de>
On 03/10/2025 15:10, René Scharfe wrote:
Show 26 quoted lines
> 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.
Show 17 quoted lines
>>> +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é >