From: René Scharfe Date: Fri, 03 Oct 2025 14:10:27 GMT Subject: Re: [PATCH] add-patch: roll over to next undecided hunk Message-ID: In-Reply-To: <8fdfb03a-6bbc-46a0-a8fe-9ad75aba555a@gmail.com> 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" 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é