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

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 3, 2025, 21:18 UTC
Message-ID: <xmqq1pnju1je.fsf@gitster.g>
In-Reply-To: <76665b6f-cb92-4694-bc89-5eb21197df34@web.de>

René Scharfe <l.s.r@web.de> writes:

Show 5 quoted lines

> git add --patch presents diff hunks one after the other, asking whether
> to add them.  If we mark some as undecided, e.g. with J, then it will
> start over after reaching the last hunk.  It always starts over at the
> very first hunk, though, even if we already decided on it.  Skip
> decided hunks when rolling over instead.

Wait a bit. With 'J', the user wants to walk the list of hunks, both decided and undecided ones, no? So ...

Show 18 quoted lines

> diff --git a/add-patch.c b/add-patch.c
> index b0389c5d5b..42a8394c92 100644
> --- a/add-patch.c
> +++ b/add-patch.c
> @@ -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;
> +				}
> +			}
> +		}

... why is it a good idea to skip decided ones?

With 'j', the story is probably different, but I didn't dig deep enough.

With 'K', we seem to do

		} else if (s->answer.buf[0] == 'K') {
			if (permitted & ALLOW_GOTO_PREVIOUS_HUNK)
				hunk_index--;
			else
				err(s, _("No previous hunk"));

and there is *no* guard around here to say "hey, hunk_index has gone negative, so we must move back to the last hunk", similar to what is happening here that does "hunk_index has gone beyond the end, so let's wrap around to the first one".

This is another bug that is maked by the fact that hunk_index is measured in size_t (even though there is no reason to do so). Using unsigned hunk_ix is simply crazy here, especially for a code that wants "just do hunk_index++ and adjust if it goes beyond the end" and similarly "just do hunk_index-- and adjust if it goes beyond the beginning".

In any case, as the result, going back with 'K' when we are on the first hunk is broken with the current code because we'd stay there, and with this patch, we'd move to the first undecided hunk. Neither is correct---we should move to the last hunk, I would think.