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: Fri, 11 Dec 2020 05:33:35 -0600 [thread overview]
Message-ID: <CAMP44s3CcNAT4dFogyo61zV+D1pZ-0K+1rDBk_BUk-RYVUW0RQ@mail.gmail.com> (raw)
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:
>
> 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*
> > 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.
> > 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
next prev parent reply other threads:[~2020-12-11 11:35 UTC|newest]
Thread overview: 79+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-12-04 6:16 [PATCH v2 00/14] pull: default warning improvements Felipe Contreras
2020-12-04 6:16 ` [PATCH v2 01/14] doc: pull: explain what is a fast-forward Felipe Contreras
2020-12-04 22:55 ` Elijah Newren
2020-12-05 1:21 ` Jacob Keller
2020-12-04 6:16 ` [PATCH v2 02/14] pull: improve default warning Felipe Contreras
2020-12-04 22:59 ` Elijah Newren
2020-12-05 0:12 ` Felipe Contreras
2020-12-05 0:56 ` Elijah Newren
2020-12-05 1:56 ` Felipe Contreras
2020-12-05 9:23 ` Chris Torek
2020-12-05 11:47 ` Felipe Contreras
2020-12-05 16:28 ` Elijah Newren
2020-12-05 21:27 ` Felipe Contreras
2020-12-06 1:01 ` Elijah Newren
2020-12-06 14:31 ` Felipe Contreras
2020-12-07 7:26 ` Junio C Hamano
2020-12-07 9:16 ` Felipe Contreras
2020-12-07 18:16 ` Junio C Hamano
2020-12-07 19:09 ` Felipe Contreras
2020-12-07 19:53 ` Junio C Hamano
2020-12-07 22:14 ` Felipe Contreras
2020-12-07 23:30 ` Jacob Keller
2020-12-08 2:23 ` Junio C Hamano
2020-12-08 3:15 ` Felipe Contreras
2020-12-08 20:16 ` Junio C Hamano
2020-12-09 9:52 ` Felipe Contreras
2020-12-09 19:05 ` Elijah Newren
2020-12-10 2:39 ` Felipe Contreras
2020-12-10 6:45 ` Junio C Hamano
2020-12-10 9:08 ` Felipe Contreras
2020-12-10 10:01 ` Felipe Contreras
2020-12-11 7:17 ` Junio C Hamano
2020-12-11 11:33 ` Felipe Contreras [this message]
2020-12-14 21:04 ` Junio C Hamano
2020-12-14 22:30 ` Felipe Contreras
2020-12-14 23:14 ` Junio C Hamano
2020-12-15 2:36 ` Felipe Contreras
2020-12-15 2:31 ` Jeff King
2020-12-15 3:49 ` Felipe Contreras
2020-12-15 11:18 ` Junio C Hamano
2020-12-15 12:53 ` Felipe Contreras
2020-12-07 9:23 ` Felipe Contreras
2020-12-04 6:16 ` [PATCH v2 03/14] pull: refactor fast-forward check Felipe Contreras
2020-12-04 6:16 ` [PATCH v2 04/14] pull: cleanup autostash check Felipe Contreras
2020-12-04 23:07 ` Elijah Newren
2020-12-05 0:47 ` Felipe Contreras
2020-12-05 0:57 ` Elijah Newren
2020-12-04 6:16 ` [PATCH v2 05/14] pull: trivial cleanup Felipe Contreras
2020-12-04 23:09 ` Elijah Newren
2020-12-05 0:48 ` Felipe Contreras
2020-12-04 6:16 ` [PATCH v2 06/14] pull: move default warning Felipe Contreras
2020-12-04 23:18 ` Elijah Newren
2020-12-04 23:36 ` Felipe Contreras
2020-12-04 6:16 ` [PATCH v2 07/14] pull: display default warning only when non-ff Felipe Contreras
2020-12-04 23:24 ` Elijah Newren
2020-12-05 1:03 ` Felipe Contreras
2020-12-04 6:16 ` [PATCH v2 08/14] pull: trivial whitespace style fix Felipe Contreras
2020-12-04 6:16 ` [PATCH v2 09/14] pull: introduce --merge option Felipe Contreras
2020-12-04 23:27 ` Elijah Newren
2020-12-05 1:06 ` Felipe Contreras
2020-12-04 6:16 ` [PATCH v2 10/14] pull: add proper error with --ff-only Felipe Contreras
2020-12-04 23:34 ` Elijah Newren
2020-12-05 1:18 ` Felipe Contreras
2020-12-04 6:16 ` [PATCH v2 11/14] tentative: pull: change the semantics of --ff-only Felipe Contreras
2020-12-04 23:39 ` Elijah Newren
2020-12-05 4:01 ` Felipe Contreras
2020-12-05 4:06 ` [PATCH] experiment: pull: change --ff-only and default mode Felipe Contreras
2020-12-05 17:29 ` Elijah Newren
2020-12-05 18:16 ` Felipe Contreras
2020-12-07 7:34 ` Junio C Hamano
2020-12-05 11:37 ` [PATCH v2 11/14] tentative: pull: change the semantics of --ff-only Felipe Contreras
2020-12-04 6:16 ` [PATCH v2 12/14] pull: show warning with --ff Felipe Contreras
2020-12-04 23:41 ` Elijah Newren
2020-12-05 1:25 ` Felipe Contreras
2020-12-04 6:16 ` [PATCH v2 13/14] test: merge-pull-config: trivial cleanup Felipe Contreras
2020-12-04 23:41 ` Elijah Newren
2020-12-04 6:16 ` [PATCH v2 14/14] test: pull-options: revert unnecessary changes Felipe Contreras
2020-12-04 23:49 ` Elijah Newren
2020-12-05 1:28 ` Felipe Contreras
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAMP44s3CcNAT4dFogyo61zV+D1pZ-0K+1rDBk_BUk-RYVUW0RQ@mail.gmail.com \
--to=felipe.contreras@gmail.com \
--cc=alexhenrie24@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=jacob.keller@gmail.com \
--cc=john@keeping.me.uk \
--cc=newren@gmail.com \
--cc=peff@peff.net \
--cc=philipoakley@iee.email \
--cc=rhansen@rhansen.org \
--cc=sandals@crustytoothpaste.net \
--cc=tytso@mit.edu \
--cc=vondruch@redhat.com \
--cc=wking@tremily.us \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).