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

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.

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

Nicely analyzed.

> Reported-by: Windl, Ulrich <u.windl@ukr.de>
> Signed-off-by: René Scharfe <l.s.r@web.de>
> ---
>  add-patch.c                |  9 ++++++++-
>  t/t3701-add-interactive.sh | 20 ++++++++++++++++++++
>  2 files changed, 28 insertions(+), 1 deletion(-)
>
> 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;
> +				}
> +			}
> +		}

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.