Re: [PATCH v2 4/5] add-patch: let options k and K roll over like j and J

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

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

> @@ -1584,7 +1591,8 @@ static int patch_update_file(struct add_p_state *s,
>  			}
>  		} else if (s->answer.buf[0] == 'K') {
>  			if (permitted & ALLOW_GOTO_PREVIOUS_HUNK)
> -				hunk_index--;
> +				hunk_index = dec_mod(hunk_index,
> +						     file_diff->hunk_nr);
>  			else
>  				err(s, _("No previous hunk"));

I was wondering if we want to always allow J and K; even when you
have only one hunk, you can still wrap around to come back to the
current hunk, and that we can do without any extra checking logic.

But it is also OK to require 2 or more hunks to "switch" to the
other hunk, which is what you do with

			if (file_diff->hunk_nr > 1) {
				permitted |= ALLOW_GOTO_PREVIOUS_HUNK;
				strbuf_addstr(&s->buf, ",K");
			}

to require more than 1.  But the error message "No previous hunk"
sounds somewhat awkward.  If user accepts the circular nature of how
we decide what "previous" is, then when we have a single hunk, the
current hunk itself _is_ the previous hunk, but because we insist
that there are at least 2, that interpretation would not work.  With
"wraparound" semantics, "No other hunk(s)", would be a better way to
give the error, no?  The same comment applies to 'J'.

>  		} else if (s->answer.buf[0] == 'J') {

This makes perfect sense, but then, after this post-context we have this:

			if (permitted & ALLOW_GOTO_NEXT_HUNK)
				hunk_index++;
			else
				err(s, _("No next hunk"));

and it sticks out that the post-increment of hunk_index here is not
using inc_mod() for symmetry.

I am wondering if with that updated (I would not say "fixed"), if we
can lose the "oops we overflowed so let's wrap around" belt-and-suspender
code at the beginning of the loop, i.e.

	for (;;) {
		enum {
			ALLOW_GOTO_PREVIOUS_HUNK = 1 << 0,
			...
			ALLOW_EDIT = 1 << 6
		} permitted = 0;

		if (hunk_index >= file_diff->hunk_nr)
			hunk_index = 0;

or if there still are other code that rely on this "oops we
overflowed" adjustment?

Other than that the resulting code with the whole series applied was
a very pleasant read.

Thanks.