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

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