Re: [RFC PATCH 1/1] add-patch: Allow reworking with a file after deciding on all its hunks

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

Abraham Samuel Adekunle <abrahamadekunle50@gmail.com> writes:

> After deciding on all hunks in a file, the interactive session
> advances automatically to the next file if there is another,
> or the process ends.
>
> Allow for reworking with a file by introducing a what_now prompt which
> allows for navigating with J/K or advancing to the next file if there is one.

Describe "how" you are allowing these new things that users used not
to be able to do (no, not in the "by adding this variable and
switching on its value" sense, but in the "now deciding on all the
hunks in a file does not automatically advance to the next file, and
the user has to do X to move forward" sense).

> -	int colored = !!s->colored.len, quit = 0, use_pager = 0;
> +	int colored = !!s->colored.len, quit = 0, use_pager = 0, skip_what_now = 0;

This is getting overly long.  Wouldn't it be easier to follow if a
preliminary patch split these existing variables into three
independent definitions, and the main patch adds the fourth one?

> +			if (s->file_diff_nr > 1)
> +				prompt_whatnow = _("What now? [J,K,q,>]? ");
> +			else
> +				prompt_whatnow = _("What now? [J,K,q]? ");

I wonder if ">" has to be made so special.  Wouldn't it be easier to
reason about the logic if ">" (and probably "<" to go back by one
file) are added to the prompt in the same logic that decides 'g',
'k', 's', etc. should be shown using the "permitted" variable?

And when the inter-file navigation is in the permitted set (i.e.,
there are multiple files involved), you'd show ">" (or "<", or both
if you are dealing with the second file among three files) and ask,
instead of silently moving to the next one, or something like that.

Organizing the logic that way will also allow you to move to the
next file _without_ first having to decide on all hunks in the
current file.  Just say ">" to deal with the next file first, and
after you are done, either come back with "<", or the system notices
that there are undecided hunks in the earlier file and takes you
back automatically.

I also have a hunch that with such a code structure you may not even
need skip_what_now flag, but I haven't even written the code in my
head, so if somebody tries to do so, they may discover the reason
why such a flag is still needed.

>  		strbuf_reset(&s->buf);
>  		if (file_diff->hunk_nr) {
> -			if (rendered_hunk_index != hunk_index) {
> +			if (rendered_hunk_index != hunk_index || skip_what_now == 1) {

Style (which may become irrelevant, as I just said the variable may
not be needed after all, but anyway).  Elsewhere skip_what_now is
used only for "is it zero, or is it not zero?".  Comparing
explicitly with 1 only here makes readers suspect if assigning 2 or
70 to the variable has special meanings and wastes their brain
cycles.