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

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