On Fri, Dec 11, 2020 at 4:02 AM Junio C Hamano [off-list ref] wrote:

Felipe Contreras [off-list ref] writes:

quoted

quoted

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*

quoted

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.

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.

quoted

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