From: Phillip Wood <phillip.wood123@gmail.com>
Date: Wed, 08 Oct 2025 13:47:21 GMT
Subject: Re: [PATCH] add-patch: roll over to next undecided hunk
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:
> 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é
>