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

From: Elijah Newren <newren@gmail.com>
To: Felipe Contreras <felipe.contreras@gmail.com>
Cc: "Git Mailing List" <git@vger.kernel.org>,
	"Vít Ondruch" <vondruch@redhat.com>,
	"Alex Henrie" <alexhenrie24@gmail.com>,
	"Junio C Hamano" <gitster@pobox.com>, "Jeff King" <peff@peff.net>,
	"Philip Oakley" <philipoakley@iee.email>,
	"Theodore Y . Ts'o" <tytso@mit.edu>,
	"Jacob Keller" <jacob.keller@gmail.com>,
	"John Keeping" <john@keeping.me.uk>,
	"Richard Hansen" <rhansen@rhansen.org>,
	"Brian M. Carlson" <sandals@crustytoothpaste.net>,
	"W. Trevor King" <wking@tremily.us>
Subject: Re: [PATCH v2 02/14] pull: improve default warning
Date: Sat, 5 Dec 2020 08:28:16 -0800	[thread overview]
Message-ID: <CABPp-BF4rXBOKsn8bG6y3QUEtNVV9K2Pk5NmwrU5818CqhRt_Q@mail.gmail.com> (raw)
In-Reply-To: <CAMP44s1=aZL7BMKSjzKJ7qYqg-usScwzRUJmaOhsCGvQ4ieYow@mail.gmail.com>

On Fri, Dec 4, 2020 at 5:56 PM Felipe Contreras
<felipe.contreras@gmail.com> wrote:
>
> On Fri, Dec 4, 2020 at 6:56 PM Elijah Newren <newren@gmail.com> wrote:
> >
> > 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:
[...]
> > > > > +                       "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.
>
> Yeah. It's a better plan, just more work for me.
>
> > 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.
>
> Well, I already said I partly agree with you: in the --ff-only case
> the suggestion should not be brought forward.
>
> But in the "git pull" default case, *today* it's doing a merge. If
> uttering --merge and thus making the current behavior explicit instead
> of implicit seems dangerous it's because it is. But not documenting it
> doesn't make it any less dangerous.

Sounds like we agree that the future should be ff-only-as-default.  I
also agree with you that the primary problem is the current default
behavior, and I'll agree with you that documenting the current default
is okay.  However, I disagree that your wording here:

+                       "If unsure, run \"git pull --no-rebase\".\n"

does anything of the sort.  It does not mention that this is the
default behavior the users would get if they provided no flags.  More
importantly, it makes a recommendation...and one that undercuts the
point of the message.  It makes it feel like the message shouldn't
exist at all in any circumstances.  I even suspect that adding that
sentence may undercut any efforts towards changing the default to
ff-only-as-default.  While I'm a big fan of most of what you've done
in this series, I will object to its merging for as long as this
stays.  (I definitely don't have veto power or anything close to it,
just stating what my opinion is.)