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

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

Perhaps "mark" -> "leave".

I somehow find it awkward to say "mark as undecided", as I have always viewed J/K as a way to skip a hunk, leaving it undecided.

Besides, "J" lets you revisit a hunk that you earlier have decided to use of hold off, and it leaves your last decision on that hunk. A statement that implies "J marks as undecided" is misleading.

Nicely analyzed.

OK.

This is probably a closely related tangent, but last night I was looking this function and found that its per-hunk loop does completely bogus thing. For example, find a case where you have more than one hunks, among which there are splittable and non-splittable hunks (a hunk is splittable if there are context lines between an added or a removed line). Start cycling the hunks without making any decisions with "J" or "K". Once you visited a splittable hunk (where you'd see 's' among the possible choices), coming back to an unsplittable hunk will now let you split it! 's' may not be visible among the choices, but telling it to 's'plit will give you "Split into 1", which is a technically correct nonsense.

This is because the handling of "permitted" in that function only adds, without resetting at the end of processing the current hunk. Yet it does something like this:

	for (;;) {
		...
		strbuf_reset(&s->buf);
		if (file_diff->hunk_nr) {
			... add choices to the prompt ...
			if (hunk->splittable_into > 1) {
				permitted |= ALLOW_SPLIT;
				strbuf_addstr(&s->buf, ",s");
			}
			...
		}
		...
		printf(_(s->mode->prompt_mode[prompt_mode_type]),
		       s->buf.buf);
		if (*s->s.reset_color_interactive)
			fputs(s->s.reset_color_interactive, stdout);
		fflush(stdout);
		if (read_single_character(s) == EOF)
			break;
		ch = tolower(s->answer.buf[0]);
		... dispatch on the command character ...
		if (ch == 'y') {
			...
		} else if (s->answer.buf[0] == 's') {
			size_t splittable_into = hunk->splittable_into;
			if (!(permitted & ALLOW_SPLIT)) {
				err(s, _("Sorry, cannot split this hunk"));
			} else if (!split_hunk(s, file_diff,
					     hunk - file_diff->hunk)) {
				color_fprintf_ln(stdout, s->s.header_color,
						 _("Split into %d hunks."),
						 (int)splittable_into);
				rendered_hunk_index = -1;
			}
		...

Notice that the prompt is built correctly but that information is *not* used when deciding if the operation is possible?

This is another ancient regression that was introduced while rewriting this program in C near the end of 2019, I think. And this causes many other bugs in this area, like 'k' at the very first hunk gets complaint "No previous hunk" only once (you move to the next one with 'j' and come back to the first hunk with 'k', and then 'k' no longer complains, even though it is not among the choice).

With this bug, however, we have gained a bit of useful feature, I think. Even though j/J should not be offered when we are at the last hunk for a file, we do wrap-around to the first hunk. I just checked the original code before the C rewrite, and even though it were written defensively so that incrementing the current hunk number to 5 when you have only 4 hunks would take you back to the initial hunk (instead of barfing), because we did not have this "permitted is never reset" bug, it actually did not allow you to go beyond the end with j/J. Today's code seems to have inherited this defensive adjustment to stay within the available hunks, and with the "permitted is never reset" bug, we are taken back to the first hunk.