Re: [PATCH v5 2/3] pull: move default warning

From: Felipe Contreras <felipe.contreras@gmail.com>
Date: Dec 11, 2020, 07:55 UTC
Message-ID: <CAMP44s170qZuZqmh_gVLGgJJ5005HzDM0fdQKQamjHkZCOu7_Q@mail.gmail.com>
In-Reply-To: <xmqqy2i4ltnv.fsf@gitster.c.googlers.com>

On Fri, Dec 11, 2020 at 12:54 AM Junio C Hamano <gitster@pobox.com> wrote:

Show 21 quoted lines

>
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
> > Eventually we want to be able to display the warning only when
> > fast-forward merges are not possible.
> >
> > In order to do so we need to move the default warning up to the point
> > where we can check if we can fast-forward or not.
>
> Makes sense.
>
> > Additionally, config_get_rebase() was probably never its true home.
>
> I agree with this point.  I've always found it suboptimal.
>
> > This requires a temporary variable to check if we are in the
> > "default mode" (no --rebase or --no-rebase specified).
>
> Two points:
>
>  - "mode" is so overused a word; a more focused word is preferrable.

There's literally only one instance in the file, and it's a call to an external function.

Plus, it refers to "git pull" without any --merge or --rebase (that's the default mode in my book).

Show 5 quoted lines

>  - by introducing a local variable in cmd_pull() and passing a
>    pointer to it to config_get_rebase(), we can easily avoid having
>    to rely on an extra global variable.
>
> I'd suggest addressing the above along the following lines.

...

> to possibly cause it to set to true, and use that instead of the
> global variable to decide if we want to give the help text.

Yeah, there's only 38 global variables. We wouldn't want another one which makes the code pretty straightforward.

Show 9 quoted lines

> > @@ -1040,6 +1029,21 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
> >       if (opt_rebase && merge_heads.nr > 1)
> >               die(_("Cannot rebase onto multiple branches."));
>
> And this is the point where we finish various error checks and
> starts to run either rebase or merge.  It is as late as we could
> delay the "non-ff and you are not configured" message.  In other
> words, the place chosen in cmd_pull() to move this code to is
> optimal.

Which is right before the fork between rebase and merge.

Show 20 quoted lines

> > +     if (default_mode && opt_verbosity >= 0 && !opt_ff) {
> > +             advise(_("Pulling without specifying how to reconcile divergent branches is\n"
> > +                      "discouraged. You can squelch this message by running one of the following\n"
> > +                      "commands sometime before your next pull:\n"
> > +                      "\n"
> > +                      "  git config pull.rebase false  # merge (the default strategy)\n"
> > +                      "  git config pull.rebase true   # rebase\n"
> > +                      "  git config pull.ff only       # fast-forward only\n"
> > +                      "\n"
> > +                      "You can replace \"git config\" with \"git config --global\" to set a default\n"
> > +                      "preference for all repositories. You can also pass --rebase, --no-rebase,\n"
> > +                      "or --ff-only on the command line to override the configured default per\n"
> > +                      "invocation.\n"));
> > +     }
>
> Either as a part of this step, as a part of the next step, or a
> separate follow-up patch, we should
>
>  - create a single-purpose helper function that only calls advise()
>    with the message and returns; name it show_advice_pull_non_ff().

If we are going for low-hanging fruit there were many in v4 of this series, which actually improve behavior, not just code organization, but OK.

Show 16 quoted lines

>  - correct the if() statement above, so that regardless of verbosity
>    level, we can do _something_ common when the history does not
>    fast-forward.  I.e.
>
>         if (rebase_unspecified && !opt_ff) {
>                 if (opt_verbosity >= 0)
>                         show_advice_pull_non_ff();
>         }
>
> These would allow us to further turn the logic to
>
>         if (rebase_unspecified && !opt_ff) {
>                 if (opt_verbosity >= 0 && advice_pull_non_ff)
>                         show_advice_pull_non_ff();
>                 die("not a fast-forward; must merge or rebase");
>         }

Should actually be something like:

        if (rebase_unspecified && !can_ff)
                die("Not a fast-forward; must either merge or rebase");

But yeah.

-- 
Felipe Contreras