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

From: Junio C Hamano <gitster@pobox.com>
To: Felipe Contreras <felipe.contreras@gmail.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 23:17:01 -0800	[thread overview]
Message-ID: <xmqqtussirsl.fsf@gitster.c.googlers.com> (raw)
In-Reply-To: CAMP44s3NNDL+zJjaukV9D2dJyU=ugSrnWz9o-whO9hKnBTxAow@mail.gmail.com

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.

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

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.

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.

Now, how would we make it possible to squelch the latter two parts?
It is not a good idea to introduce pull.mode=ff-only for that
purpose (pull.mode=rebase and pull.rebase=yes would unnecessarily
become two redundant ways to do the same thing if we added a new
pull.mode variable).

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.

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

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.