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

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

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

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

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