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

From: Felipe Contreras <felipe.contreras@gmail.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: "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>,
	"Jeff King" <peff@peff.net>,
	"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: Thu, 10 Dec 2020 03:08:59 -0600	[thread overview]
Message-ID: <CAMP44s3NNDL+zJjaukV9D2dJyU=ugSrnWz9o-whO9hKnBTxAow@mail.gmail.com> (raw)
In-Reply-To: <xmqqy2i6w45c.fsf@gitster.c.googlers.com>

On Thu, Dec 10, 2020 at 12:45 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Elijah Newren <newren@gmail.com> writes:
>
> > Have I missed some subtlety here?  This whole email appears to me to
> > be arguing against a strawman.  Reading Junio's other emails in this
> > thread[1][2], it's pretty clear he thinks the current behavior is
> > buggy and suggests how it should be changed.  From what I can tell,
> > you appear to be arguing against doing nothing and against only
> > accepting perfection, neither of which were positions I saw anyone
> > take.  In fact, the positions you argue for at length appear to
> > exactly match the ones he took[1][2].  What am I missing?
> >
> > [1] https://lore.kernel.org/git/xmqq360h8286.fsf@gitster.c.googlers.com/
> > [2] https://lore.kernel.org/git/xmqqlfe99yvy.fsf@gitster.c.googlers.com/
>
> I tend to agree that the endgame state I want is pretty similar to
> what Felipe wants.  The seeming confusion is probably due to what
> exactly I mean by "default" is different from what he means.
>
> I view the proposed "for unconfigured users, pull dies, and tells
> them to choose between rebase or merge before it can continue, when
> faced with a non-ff history" as a safe fallback behaviour until the
> users make their choice.
>
> It is a safe fallback to disable the more dangerous half of the
> command until the user gives enough information to the command to do
> its job without damaging the resulting history; it is not something
> the users would actively want to choose.
>
> And that is what I meant by the default behaviour.
>
> And when we stop in such a manner, it is sensible to give an error
> message telling them
>
>  - why we are stopping,
>
>  - what they can do to move the immediate situation forward
>    (i.e. command line option that lets them choose), and
>
>  - what they can do to make their choice permanent so that they
>    would never see the command stop when facing a non-ff history
>    (i.e. the configuration variables).
>
> Up to this point, I think both of us agree with the above.

I don't agree with the above.

The error I propose is just:

  The pull was not fast-forward, please either merge or rebase.

That's it. Nothing more.

I explained that was the final end goal in my list of steps [1]. I do
not think any suggestion for commands or configurations belongs in a
*permanent* error message.

Do we even have an error message that long?

> We start to differ after this point.  I would want to see only
> "rebase" and "merge in the "choice" in the above list.  Felipe, if I
> understand correctly, wants to add the third one, "ff-only", which
> means the more dangerous half of "pull" is disabled by default.

No. I don't want to present the user *any* configuration option in the
*permanent error* message, just: "please either merge or rebase".
That's it.

> I do not want to include that choice, as it would mean the more
> familiar pull.rebase=yes/no would no longer work, and we'd need to
> introduce a new variable, like pull.mode=ff-only.

Whether we present the user with that option or not doesn't matter.
"pull.rebase=yes/no", would still work. The only thing that changes is
what happens if the user does *not* set that option.

The reason "pull.mode=ff-only" needs to be introduced is that
--ff-only doesn't work. Otherwise there's no way the user cannot
select the "safe default" mode. It has absolutely nothing to do with
what we present the user with.

> Without introducing pull.mode, the only thing the users cannot do,
> as far as I can tell, is to explicitly ask for the behaviour of an
> unconfigured user (i.e. error out when faced with non-ff history)
> without being told about the way to use configuration variable to
> permanently record their choice.

That's right, which is a *crucial* thing they must have.

Part II of the proposal has these:

  pull: add pull.mode
  pull: add pull.mode=ff-only
  pull: advice of future changes

This is where we want to be for a good considerable amount of time. A
point where the user is a) warned about future changes, b) can
configure both the old and the new behavior, and c) we have time to
test and tune this new behavior (in case there are mistakes), *BEFORE*
forcing all our users to switch to this new mode (unless they haven't
configured otherwise).

This is what happened with "push.default=simple". We didn't just say:
the simple mode is the default mode, so just leave "push.default"
blank if you want this mode. No. Because in theory we might have
decided before v2.0 that another mode was to be the default, or
perhaps in v3.0 another one will. We want the user to be able to say:
this is the mode I want. Not whatever mode the Git project decides
it's better this day of the week: *this* mode.

There is nothing good about the user being unable to specify the mode
he/she wants instead of the default, even if at this moment in time
they happen to be the same.

Moreover, I can attach a patch on top of part I that skips right ahead
towards part 3, and flips the switch without any intermediary steps.
The patch is simple, but it's likely to be wrong. And I doubt anyone
has tested this patch series at this point. No amount of looking at
the code is going to spot all the problems.

People actually need to try this code.

Otherwise it's just some patches flying around that will never be
merged, and the problem will remain forever there.

> Other than that, the existing
> pull.rebase=yes/no is perfectly adequate.

You cannot specify "pull.rebase=ff-only", but I did send a patch for
that [2]. It was you the one that said it looked "quite funny" [3]:
"it looks quite funny for that single variable that controls the way
how pull works, whether rebase or merge is used, is pull.REBASE".

If you think it's fine to have this warning:

  The pull was not fast-forward, in the future you will have to choose
a merge, or a rebase.

  To quell this message you have two main options:

  1. Adopt the new behavior:

    git config --global pull.rebase ff-only

  2. Maintain the current behavior:

    git config --global pull.rebase false

  For now we will fall back to the traditional behavior (merge).
  Read "git pull --help" for more information.

For months, possibly a year, possibly until v3.0, instead of this one
(which is my proposal):

  The pull was not fast-forward, in the future you will have to choose
a merge, or a rebase.

  To quell this message you have two main options:

  1. Adopt the new behavior:

    git config --global pull.mode fast-forward

  2. Maintain the current behavior:

    git config --global pull.mode merge

  For now we will fall back to the traditional behavior (merge).
  Read "git pull --help" for more information.

By all means, let's go for that, make the decision.

But it was you who wanted to see how a pull.mode solution would look
like [4][5]. Well, this is how it looks like: 3 patches.

> Felipe seems to think
> otherwise and wants not just the "safe fallback behaviour", but can
> explicitly be configured using pull.mode=ff-only (and if that is not
> what Felipe thinks, perhaps we are already in agreement without
> realizing it).

No. It doesn't matter if it's "pull.mode=ff-only", or
"pull.rebase=ff-only", or "pull.ff=only", but one of those must turn
on the behavior that we want. And right now no option does.

> Thinking about it again, I guess pull.rebase=yes/no plus a new
> advise entry can easily give what Felipe seems to want.

It's not about what I want, it's about what the project wants. And I
think it's pretty clear what the project wants:

  1. git pull to eventually fail in non-fast-forward cases
  2. A grace period before that switch is flipped

That's it.

My proposal is the only one (so far) that gives us that.

Cheers.

[1] https://lore.kernel.org/git/CAMP44s2hUCd9qc83LReGyjy8N+u++eK6VjwGhDhrX0f0SbKmig@mail.gmail.com/
[2] https://lore.kernel.org/git/20201123224621.2573159-2-felipe.contreras@gmail.com/
[3] https://lore.kernel.org/git/xmqqim9vlkdn.fsf@gitster.c.googlers.com/
[4] https://lore.kernel.org/git/xmqqy2irjy4f.fsf@gitster.c.googlers.com/
[5] https://lore.kernel.org/git/xmqq7dqagtgx.fsf@gitster.c.googlers.com/

-- 
Felipe Contreras