Re: [PATCH v2 02/14] pull: improve default warning

[Elijah Newren <newren@gmail.com>]

Hi Felipe,

On Fri, Dec 4, 2020 at 4:12 PM Felipe Contreras
<felipe.contreras@gmail.com> wrote:
>
> On Fri, Dec 4, 2020 at 5:00 PM Elijah Newren <newren@gmail.com> wrote:
> >
> > On Thu, Dec 3, 2020 at 10:16 PM Felipe Contreras
> > <felipe.contreras@gmail.com> wrote:
> > >
> > > We want to:
> > >
> > > 1. Be clear about what "specifying" means; merge or rebase.
> > > 2. Mention a direct shortcut for people that just want to get on with
> > >    their lives: git pull --no-rebase.
> >
> > This is a shortcut for what?
>
>   git config --global pull.rebase false
>   git pull
>
> It's a shorter way of saying: "do a 'git pull' like you've always done
> but don't warn me".
>
> > > 3. Have a quick reference for users to understand what this
> > >    "fast-forward" business means.
> > >
> > > This patch does all three.
> > >
> > > Thanks to the previous patch now "git pull --help" explains what a
> > > fast-forward is, and a further patch changes --no-rebase to --merge so
> > > it's actually user friendly.
> > >
> > > Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> > > ---
> > >  builtin/pull.c | 23 ++++++++++++-----------
> > >  1 file changed, 12 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/builtin/pull.c b/builtin/pull.c
> > > index 1034372f8b..22a9ffcade 100644
> > > --- a/builtin/pull.c
> > > +++ b/builtin/pull.c
> > > @@ -346,17 +346,18 @@ static enum rebase_type config_get_rebase(void)
> > >
> > >         if (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"));
> > > +                       "discouraged; you need to specify if you want a merge, or a rebase.\n"
> >
> > ...want a merge, a rebase, or neither.
>
> There is no "git pull --no-merge". Years ago some people argued for a
> "pull.mode=none" (essentially making "git pull" the same as "git
> fetch"). But right now there's no option to do that.
>
> There's an option to do --ff-only, but that's still a merge.

I disagree.  I'm well aware that checkout_fast_forward() (which is
what is ultimately called to do the fast-forwarding) is in a file
called merge.c, but that doesn't make it a merge.  I don't believe it
was anything more than a convenient place to dump some extra code at
the time.

> Perhaps: a merge, a rebase, or a fast-forward?

Sure, that works; in fact, that's much better than my suggestion.  I like it.

> > > +                       "You can squelch this message by running one of the following commands:\n"
> > > +                       "\n"
> > > +                       "  git config pull.rebase false  # merge (the default strategy)\n"
> >
> > Should this be labelled as the default given the desire to make
> > --ff-only the default?  Perhaps I'm jumping ahead and you plan to
> > change that later in this series.
>
> That's right.
>
> In the previous series which does indeed make "pull.mode=ff-only" the
> default [1], I do change the warning to specify the future default
> [2], but in that series the warnings is changed to:
>
>   The pull was not fast-forward, in the future you will have to choose
> a merge, or a rebase.
>   To squelch this message and maintain the current behavior, use:
>
>     git config --global pull.mode merge
>
>   To squelch this message and adopt the new behavior now, use:
>
>     git config --global push.mode ff-only
>
>   Falling back to old style for now (merge).
>   Read "git pull --help" for more information.
>
> Since that series didn't get any traction, I decided to only implement
> step 1: fix the current situation. And later on another series would
> do step 2: introduce "pull.mode=ff-only" and do the preparations to
> make it the default.

I like this longer plan.  However, on the shorter scale plan...I think
the suggestion to use "git pull --no-rebase" makes the current
situation worse rather than fixing it.

> > > +                       "  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.\n"
> >
> > Good up to here.
> >
> > > +                       "If unsure, run \"git pull --no-rebase\".\n"
> >
> > Why is that safe to suggest?  The original text may not have been the
> > easiest to parse, but this seems more problematic to me.
>
> Because "git pull" has been doing the same as "git pull --no-rebase"
> for more than a decade. It's safe for people to continue with this
> behavior for a few more months.
>
> Some people need to get things done today, and they are not interested
> in future changes, nor changing their default configuration, or what
> the warning has to say.
>
> They just want "git pull" to do the same as yesterday, and the year
> before, without being bothered with an annoying warning.
>
> Those people can start training their fingers to do "git pull
> --merge", and learn the problems with "git pull" later.
>
> We want to respect the user's time, and not force them to read the
> warning today.

The warning was added because sending users down paths that break
things badly is a waste of user's time, and often a much bigger waste
of user's time than making them read up on the meaning behind the two
different choices of what kind of changes they can make.  I agree the
warning went too far, but I fully agree with the original folks who
put this warning here that we need to have it for at least some cases
and that there is a decision to be made.  Though I am just one voice,
and perhaps others will agree with you on your point here, I'll
continue to disagree with blindly suggesting "git pull --no-rebase".

> > > +                       "Read \"git pull --help\" for more information."
> >
> > Nice addition.
>
> Especially since now it explains what a fast-forward is.

Indeed.  :-)