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

From: Felipe Contreras <felipe.contreras@gmail.com>
To: Jeff King <peff@peff.net>, Junio C Hamano <gitster@pobox.com>
Cc: "Felipe Contreras" <felipe.contreras@gmail.com>,
	"Elijah Newren" <newren@gmail.com>,
	"Jacob Keller" <jacob.keller@gmail.com>,
	"Git Mailing List" <git@vger.kernel.org>,
	"Vít Ondruch" <vondruch@redhat.com>,
	"Alex Henrie" <alexhenrie24@gmail.com>,
	"Philip Oakley" <philipoakley@iee.email>,
	"Theodore Y . Ts'o" <tytso@mit.edu>,
	"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: Mon, 14 Dec 2020 21:49:33 -0600	[thread overview]
Message-ID: <5fd8324d70ae5_d7c48208fa@natae.notmuch> (raw)
In-Reply-To: <X9ggAxk/z0D9Qom+@coredump.intra.peff.net>

Jeff King wrote:
> On Thu, Dec 10, 2020 at 11:17:01PM -0800, Junio C Hamano wrote:
> 
> > You want to let the user express: "I do not want to choose either
> > rebase or merge.  I want 'pull' to fail when it needs to deal with
> > non-ff history.  But I do not need to be told about command line
> > option and configuration every time."
> > 
> > I said I don't (I view that disabling half the "git pull" just a
> > safe fallback behaviour until the user chooses between merge and
> > rebase), but if we wanted to offer it as a valid choice to users, we
> > can do so.  We just make it possible to squelch the latter two parts
> > of the three-part message---you leave pull.rebase unconfigured and
> > squelch the latter two parts of the message, and you got the "stop
> > me, I do not merge or rebase, but don't even tell me how to further
> > configure" already.
> 
> FWIW, I would be such a user who would want to squelch the error but
> keep the ff-only behavior (and have been doing so for years via
> pull.ff=only).

If you have configured pull.ff=only, you should be getting this error:

	Not possible to fast-forward, aborting.

We would like this error instead:

  Not a fast-forward, either merge or rebase.

And after that it should be obvious what this command should do with
your current configuration:

  git pull --merge

But it would fail, even after specifying you want to do a merge, because
"git pull --ff-only --merge" fails.

> So I think this is a valuable thing to have. And it does work just fine
> already, without pull.mode. The reasons to care about pull.mode (IMHO)
> are mostly about explaining it:
> 
>   - it isn't respected with rebase. So if you set pull.rebase=true, now
>     pull.ff=only does nothing. This is arguably confusing, though I
>     doubt it comes up much in practice.
> 
>   - having a single tri-state of merge/rebase/error for "what do I do
>     when pulling a non-fast-forward merge" is conceptually simple and
>     easy to explain.

Indeed. It's *mostly* about explaining it, but there's also what I
stated above:

  git -c pull.mode=fast-forward pull
  git -c pull.mode=fast-forward pull --merge

We want the first one to fail, and the second one to succeed, which
can't be done with the current "pull.ff=only".

> So I like pull.mode in that sense. But it is also weighed against the
> fact that we'd still have to support pull.rebase, and we'd still have to
> support pull.ff, and explain how those interact with pull.mode (and I
> think any new error in this area must be squelched by those existing
> variables, or it would be a regression for people who already picked
> their default long ago).

The interactions should be easy:

  * pull.rebase=false -> pull.mode=merge
  * pull.rebase=the-rest -> pull.mode=rebase
  * pull.ff=only -> pull.mode=fast-forward

(note: I'm not entirely sure of the last one)

Then, any --merge/--rebase option overrides the mode, but not so the
--ff options.

We would still like this to fail:

  git -c pull.mode=merge pull --ff-only

Right?

In other words: pull.mode is strongly linked with pull.rebase, but not so
much wih --ff*.

> > But there is an established way used in this project when we allow
> > squelching overly-helpful help messages, and we can apply it here as
> > well.  That way:
> > 
> >  - unconfigured folks would get all the three parts of the messages,
> >    just like the current system.
> > 
> >  - if you tell rebase or merge, you do not see any.
> > 
> >  - if you do not choose between rebase or merge, you can still
> >    squelch the latter two by setting advice.pullNonFF to false.
> > 
> > The last one is "keep the more dangerous half of 'git pull' disabled,
> > without getting told how to enable it over and over", which is what
> > you want to be able to specify.
> 
> Using advice.* to squelch the advice would be fine with me, provided it
> was _also_ squelched by the existing config options.
> 
> Which I think is where your thinking is ending up.

Yes. At least that's my thinking.

Cheers.

-- 
Felipe Contreras