Subject: Re: [PATCH] add-patch: roll over to next undecided hunk

From: René Scharfe <[email-masked]>
Date: Fri, 3 Oct 2025 16:10:27 +0200

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.

>> +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.

René