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

From: Felipe Contreras <felipe.contreras@gmail.com>
Date: Dec 14, 2020, 22:30 UTC
Message-ID: <5fd7e77c78e06_d59852083e@natae.notmuch>
In-Reply-To: <xmqqh7ooje03.fsf@gitster.c.googlers.com>

Junio C Hamano wrote:

Show 7 quoted lines

> Felipe Contreras <felipe.contreras@gmail.com> writes:
> 
> > This is irrelevant.
> 
> Oh, now you are saying that you do not need a way to squelch these
> message to help unconfigured users choose between rebase or merge?
> I am confused.

We need a way to squelch the *advise* message, yes.

But this is a secondary priority. The first priority is not to break current behavior users rely on.

Show 9 quoted lines

> > As long as it's an error I don't care if it's short or long. I'm
> > against turning on an error from one version to the next.
> 
> Now you are changing your mind?  Current version and past ones do
> not make it an error to pull non-ff history without choosing rebase
> or merge---we go ahead and merge anyway.  I thought that in the far
> future agreed between two of us, it would be turnend into an error
> at some point.  There needs one version that turns it into an error
> for that to happen.  Puzzled.

Yes, in the future, *after* a deprecation period.

Once the users have had a chance to configure git to do what they want, have tried the new mode, haven't had issues with the new mode, or if they disagree with the proposed *future* behavior; complain in the forums at their disposal.

Not before.

Show 32 quoted lines

> >> I too initially thought that pull.mode may be needed, but probably I
> >> was wrong.  I do think this can be done without pull.mode at all, at
> >> least in two ways, without adding different ways to do the same
> >> thing.
> >>
> >>  - When pull.rebase is set to 'no' and pull.ff is set to 'only',
> >>    "git pull" that sees a non-ff history should error out safely.
> >>    The user is telling that their preference is to merge, but the
> >>    difference between merge and rebase does not really matter
> >>    because pull.ff=only would mean we forbid merges of non-ff
> >>    history anyway.  The message you'd get would be "fatal: Not
> >>    possible to fast-forward, aborting." though.
> >>
> >>  - Or with the advice that hides the latter two points, a user can
> >>    unset pull.rebase and set the advice.pullNonFF to false to get
> >>    the same behaviour (i.e. disable the more dangerous half of
> >>    "pull") with just the "we stopped" error message.
> >
> > So, after your hypothetical patch, there would be no difference between:
> >
> >   git -c pull.rebase=no -c pull.ff=only pull
> >
> > and:
> >
> >   git -c advice.pullnonff=false pull
> >
> > ?
> 
> We do not have to or implement both, but either should give us the
> "when pull sees a non-ff history, it should stop without merging or
> rebasing, and the user won't be given the advice on how to choose
> between merge and rebase" behaviour, I would think.

Right, so both should error out.

And what should these do?

  git -c pull.rebase=no -c pull.ff=only pull --merge

  git -c advice.pullnonff=false pull --merge

I'm going to answer because I think it's obvious what you would expect: if you pass --merge, both should succeed.

Except they won't, because "git pull --ff-only --merge" fails.

Correct?

Show 8 quoted lines

> >> I think either of these are close enough to what you want, and I
> >> think the latter gives us more flexibility in how we tone down the
> >> message with advice.pullNonFF.
> >
> > You are missing at least two things.
> 
> I am guessing that the '?' above I just answered is one you wanted
> to ask me, but what's the other one?

Yes. The other is what I explained above; we need a grace deprecation period where we can explain to the user in a simple way what to expect in the future.

And it's much easier to explain to the user that:

  git pull --merge -> pull.mode = merge
  git pull --rebase -> pull.mode = rebase
  git pull -> pull.mode = fast-forward

Than:

  git pull --merge -> pull.rebase = false
  git pull --rebase -> pull.rebase = true
  git pull -> pull.rebase = false + pull.ff = only

But those are not the only two. For example there's this additional problem of how to interact with the other values of pull.rebase (other than true and false):

  pull.mode = merge
  pull.rebase = merges

I would expect in this particular configuration that:

  git pull -> git pull --merge
  git pull --rebase -> git pull --rebase=merges

There's no way to represent that with just pull.rebase.

And there's more: some people suggested other modes in 2013, like "pull.mode=none" (essentially "git fetch"), or "pull.mode=merge-reverse-parents".

But the first one should be enough ("git pull --ff-only --merge" doesn't work).

Cheers.

-- 
Felipe Contreras