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

[Felipe Contreras <felipe.contreras@gmail.com>]

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