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

From: Felipe Contreras <felipe.contreras@gmail.com>
Date: Dec 11, 2020, 11:33 UTC
Message-ID: <CAMP44s3CcNAT4dFogyo61zV+D1pZ-0K+1rDBk_BUk-RYVUW0RQ@mail.gmail.com>
In-Reply-To: <xmqqtussirsl.fsf@gitster.c.googlers.com>

On Fri, Dec 11, 2020 at 4:02 AM Junio C Hamano <gitster@pobox.com> wrote:

Show 28 quoted lines

>
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
> >> 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.
>
> It says "why we are stopping." quite well.  It would be a good
> message to use as the first part of the three-part message I
> mentioned above.

The two key parts of the message are:

1. It is an *error*
2. It is *permanent*

Show 7 quoted lines

> > 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.
>
> In the design I have in mind in the message you are responding to,
> the users who haven't told their choice to Git would be the only
> folks who get all three.

What would that error message look like? And do you have any other example of the current user interface where such a condescending long error message is displayed?

> 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."

That's right.

Show 13 quoted lines

> 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.
>
> I agree the latter two should not be part of *permanent* error
> message.  And my suggestion did not intend to make them so---it
> should have been quite obvious to who read the message you are
> responding to through to the end and understood what it said.

It doesn't matter (much) if it's temporary or permanent, it's still an *error* message.

Currently it's a warning, and people are complaining, even though the pull still works.

And you want to make it an error, and *always* fail? Even though the user has not been warned that such a change was coming and how to evade it?

I'm against that. I would rather do nothing than intentionally break user experience without warning.

Johannes said he didn't mind the warning. I want the warning to remain, just make it a different warning. You want to break his experience without a grace period and suddenly make it an error.

> Now, how would we make it possible to squelch the latter two parts?

...

This is irrelevant.

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.

Show 22 quoted lines

> > 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.
>
> 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

> 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'll wait for your response.

Cheers.

-- 
Felipe Contreras