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

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 23, 2026, 16:38 UTC
Message-ID: <xmqqv7gsi8s6.fsf@gitster.g>
In-Reply-To: <e98d8aa20fb4a82b93b9887e38eb8289252b936d.1769164663.git.abrahamadekunle50@gmail.com>

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

Show 6 quoted lines

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