All of lore.kernel.org
 help / color / mirror / Atom feed
From: Felipe Contreras <felipe.contreras@gmail.com>
To: Elijah Newren <newren@gmail.com>
Cc: "Git Mailing List" <git@vger.kernel.org>,
	"Vít Ondruch" <vondruch@redhat.com>,
	"Alex Henrie" <alexhenrie24@gmail.com>,
	"Junio C Hamano" <gitster@pobox.com>, "Jeff King" <peff@peff.net>,
	"Philip Oakley" <philipoakley@iee.email>,
	"Theodore Y . Ts'o" <tytso@mit.edu>,
	"Jacob Keller" <jacob.keller@gmail.com>,
	"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: Sat, 5 Dec 2020 15:27:57 -0600	[thread overview]
Message-ID: <CAMP44s2L24jhCG9ps72--ZiJkXUovR726jCf8JTLHAs0jV7Whg@mail.gmail.com> (raw)
In-Reply-To: <CABPp-BF4rXBOKsn8bG6y3QUEtNVV9K2Pk5NmwrU5818CqhRt_Q@mail.gmail.com>

On Sat, Dec 5, 2020 at 10:28 AM Elijah Newren <newren@gmail.com> wrote:
> On Fri, Dec 4, 2020 at 5:56 PM Felipe Contreras
> <felipe.contreras@gmail.com> wrote:

> > Well, I already said I partly agree with you: in the --ff-only case
> > the suggestion should not be brought forward.
> >
> > But in the "git pull" default case, *today* it's doing a merge. If
> > uttering --merge and thus making the current behavior explicit instead
> > of implicit seems dangerous it's because it is. But not documenting it
> > doesn't make it any less dangerous.
>
> Sounds like we agree that the future should be ff-only-as-default.  I
> also agree with you that the primary problem is the current default
> behavior, and I'll agree with you that documenting the current default
> is okay.  However, I disagree that your wording here:
>
> +                       "If unsure, run \"git pull --no-rebase\".\n"
>
> does anything of the sort.  It does not mention that this is the
> default behavior the users would get if they provided no flags.

But that is not the warning, this is the warning:

  Pulling without specifying how to reconcile divergent branches is discouraged;
  you need to specify if you want a merge, a rebase, or a fast-forward.
  You can squelch this message by running one of the following commands:

    git config pull.rebase false  # merge (the default strategy)
    git config pull.rebase true   # rebase
    git config pull.ff only       # fast-forward only

  You can replace "git config" with "git config --global" to set a default
  preference for all repositories.
  If unsure, run "git pull --merge".
  Read "git pull --help" for more information.

This warning says:

1. There's 3 options: merge, rebase, fast-forward
2. merge is the default strategy
3. If unsure, specify --merge (the default strategy)

So taken altogether it does say what is the default strategy.

> More
> importantly, it makes a recommendation...and one that undercuts the
> point of the message.

So?

When boarding a plane the flight attendants do a safety demonstration
that passengers should pay attention to. If one passenger is not
paying attention (listening to music on headphones, and reading a
book) what should the crew do?

1. Remove the passenger's headphones and force him to pay attention to
the safety demonstration
2. Let the passenger ignore the safety demonstration

Human beings are independent agents responsible for their own actions.
You as a separate human being--a crew member--can argue that it's not
in the best interest of the passenger to ignore the safety
demonstration, and you may be right, but the passenger decisions are
still the passenger's decisions, even if they are bad.

Do you think the crew should disregard the passenger's volition and
force him to pay attention to the safety demonstration?

> It makes it feel like the message shouldn't
> exist at all in any circumstances.  I even suspect that adding that
> sentence may undercut any efforts towards changing the default to
> ff-only-as-default.  While I'm a big fan of most of what you've done
> in this series, I will object to its merging for as long as this
> stays.  (I definitely don't have veto power or anything close to it,
> just stating what my opinion is.)

The current warning should not exist at all.

The complaint from Vít Ondruch [1] that reignited this series is a
valid one. A *permanent* warning is not good. We should have a
*temporary* warning with the express purpose of notifying users of an
upcoming change.

If we have not yet decided on what should be the default (Junio seems
to have casted some doubt on the consensus [2]), and we don't have a
clear path forward to implement such change (we can't even tell users
to use "pull.ff=only", since eventually it may be
"pull.mode=ff-only"), then we must remove the warning.

It was a mistake to put a *permanent* warning before deciding to
change the default.

So, there's two options:

1. We decide on a path forward and fix the warning so it *temporarily*
explains what will happen in the future
2. We remove the *permanent* warning

Since we are already here, we might as well take advantage of that
warning and repurpose it. But in the meantime--while the git project
decides what to do, and what configurations to suggest the users to
change--we should at the very least waste as little as the user's time
as possible, and give him/her a quick opt-out.

Yes, a quick opt-out defeats the purpose of a warning, but we must
respect the users' volition. The user may be on a deadline trying to
push some changes to production before the weekend, and after a system
update be annoyed with this warning on every pull. The user may not
have time to look at the warning, decide he wants to read the warning
in the future, maybe next Monday, and thus not configure anything to
silence it.

What's wrong with a user saying "I don't have time for this now,
please tell me what to do for now, I'll look at the warning later"? If
anything for those users the configuration is the wrong thing to do,
because being in a hurry they just choose the first configuration and
forget about the warning without actually looking at it (because they
didn't have time), and it will not appear any more. By typing "git
pull --merge" the user can get rid of the warning *for now*, but the
next time he does "git pull" the warning will reappear, and at that
time perhaps the user does have the time to read it, and look at the
manpage.

Nobody likes their workflow to be interrupted and be forced to do anything.

I don't think my patches plus that suggestion for a quick opt-out are
in any way worse than the current situation. If you think they are,
then we'll just have to agree to disagree.

I quote the voice of Vít Ondruch, which I think represents the typical
user: "please select any strategy considered more appropriate and stop
warning me".

Cheers.

[1] https://lore.kernel.org/git/742df4c2-2bc5-8a4b-8de1-cd5e48718398@redhat.com/
[2] https://lore.kernel.org/git/xmqqh7p1fjml.fsf@gitster.c.googlers.com/

-- 
Felipe Contreras

  reply	other threads:[~2020-12-05 21:29 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 [this message]
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
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=CAMP44s2L24jhCG9ps72--ZiJkXUovR726jCf8JTLHAs0jV7Whg@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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.