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

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

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

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.