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

[Junio C Hamano <gitster@pobox.com>]

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

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

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