Re: [PATCH v3 2/3] add-patch: Allow interfile navigation when selecting hunks

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 6, 2026, 18:54 UTC
Message-ID: <xmqqwm0pem83.fsf@gitster.g>
In-Reply-To: <24692afa3f0a67d3f3eba776cc745287c5d71e94.1770390576.git.abrahamadekunle50@gmail.com>

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

Show 6 quoted lines

> +	for (i = 0; i < s.file_diff_nr;) {
> +		if (s.file_diff[i].binary && !s.file_diff[i].hunk_nr) {
>  			binary_count++;
> +			i++;
> +			continue;
> +		}

This "continue" is a commonly seen good trick to avoid having the nesting go too deep. As we know the case where the condition holds have already been dealt with and moved to the next iteration at this point, we can ...

> +		else {

... omit this extra "else" block and write what is inside for everybody (not just "those who did not pass the if condition above", which is what "else" tells us).

Show 17 quoted lines

> +			ret = patch_update_file(&s, s.file_diff + i);
> +			if (ret == NEXT_FILE) {
> +				if (s.s.no_auto_advance && i == s.file_diff_nr - 1)
> +					i = 0;
> +				else
> +					i++;
> +				continue;
> +			}
> +			if (ret == QUIT)
> +				break;
> +			if (s.s.no_auto_advance && ret == PREVIOUS_FILE) {
> +				if (i == 0)
> +					i = s.file_diff_nr - 1;
> +				else
> +					i--;
> +				continue;
> +			}

The asymmetry between next/quit and prev feels curious.

The patch_update_file() helper returns QUIT when the user tells us to (regardless of auto-advance setting), PREVIOUS when '<' is given but that is only possible with auto-advance disabled, and NEXT in all other cases. The check inside the NEXT case for auto-advance is to decide if we want to overflow 'i' beyond file_diff_nr to complete the session, or we want to wrap-around back to the first file.

But ret can be PREV only under auto-advance disabled, so the check there feels totally redundant.

And we want to treat the list of files as a ring buffer only when auto-advance is set to false. This may work in practice but the logic feels convoluted.

The patch_update_file() knows how many files there are to decide if we want to offer '<' and '>'. It also knows the file index within the file_diff_nr for the file it is handling. I wonder if it should do a bit more with its return value to help the caller? E.g., perhaps it can return the next 'i' if it wants the caller to advance (and decide to do the ring-buffer if needed), or if it wants to tell the caller that everything is done by returning some sentinel value (e.g., -1)? Then this part of the caller can just be

	if ((i = patch_update_file(&s, i)) < 0)
		break; /* all done */

perhaps?

By the way, I just noticed that the new local variable you added to patch_update_file() is called "ret" but that is hiding a different variable "int ret" that is used to handle the '/' command. It should be renamed to avoid the name collision.