diff mbox series

[2/2] pull: improve default warning

Message ID 20210621175234.1079004-3-felipe.contreras@gmail.com (mailing list archive)
State Superseded
Headers show
Series pull: documentation improvements | expand

Commit Message

Felipe Contreras June 21, 2021, 5:52 p.m. UTC
According to feedback from GitHub trainers [1], most newcomers don't
understand what a rebase is. So in the default warning we want to
provide our users with a command that does the most sensible thing,
fixes the divergence, gets rid of the warning, with the minimum mental
effort, and happens to be the default:

  git pull --no-rebase (later --merge)

In addition, we don't want to start by recommending a permanent
configuration, but a temporary solution so they start training their
fingers and maybe learn how to do a rebase. So we start with the commands.

Also, we need to be clear about what we mean by "specifying"; merge, or
rebase.

Moreover, thanks to the previous patch now "git pull --help" explains
what a fast-forward is, let's mention that reference.

And finally, use --global in the configuration commands like we did with
push.default.

[1] https://lore.kernel.org/git/20130909201751.GA14437@sigill.intra.peff.net/

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 builtin/pull.c | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

Comments

Alex Henrie June 21, 2021, 6:05 p.m. UTC | #1
On Mon, Jun 21, 2021 at 11:52 AM Felipe Contreras
<felipe.contreras@gmail.com> wrote:
>
> +                "If unsure, run \"git pull --no-rebase\".\n"

I don't think the message should recommend merging over rebasing;
which strategy is the correct one depends entirely on the project's
workflow and the user's role within that workflow. The eventual goal
is to get rid of the default here and make the user make an educated
choice, which does imply some work on the user's part, but it avoids
the massive headaches created by users merging without understanding
what they're doing.

-Alex
Felipe Contreras June 21, 2021, 6:51 p.m. UTC | #2
Alex Henrie wrote:
> On Mon, Jun 21, 2021 at 11:52 AM Felipe Contreras
> <felipe.contreras@gmail.com> wrote:
> >
> > +                "If unsure, run \"git pull --no-rebase\".\n"
> 
> I don't think the message should recommend merging over rebasing;

This is the default strategy.

Doing `git pull` is the same as doing `git pull --no-rebase`, except
with the warning.

> The eventual goal is to get rid of the default here and make the user
> make an educated choice, which does imply some work on the user's
> part, but it avoids the massive headaches created by users merging
> without understanding what they're doing.

Indeed, but any minute change in git's UI is a gargantuan task that
takes several years--or even decades--to accomplish, if it ever happens.
I started this patch in 2013, and here we are.

This patch series is not attempting to do the impossible.

Cheers.
Alex Henrie June 21, 2021, 9:47 p.m. UTC | #3
On Mon, Jun 21, 2021 at 12:51 PM Felipe Contreras
<felipe.contreras@gmail.com> wrote:
>
> Alex Henrie wrote:
> > On Mon, Jun 21, 2021 at 11:52 AM Felipe Contreras
> > <felipe.contreras@gmail.com> wrote:
> > >
> > > +                "If unsure, run \"git pull --no-rebase\".\n"
> >
> > I don't think the message should recommend merging over rebasing;
>
> This is the default strategy.

Yes, but it shouldn't be, and we shouldn't make the problem worse by
encouraging people to default to merging without thinking.

> > The eventual goal is to get rid of the default here and make the user
> > make an educated choice, which does imply some work on the user's
> > part, but it avoids the massive headaches created by users merging
> > without understanding what they're doing.
>
> Indeed, but any minute change in git's UI is a gargantuan task that
> takes several years--or even decades--to accomplish, if it ever happens.
> I started this patch in 2013, and here we are.

Although what needs to be done had been envisioned by some as early as
2013, the warning has only been around since Git 2.27 (released in
June 2020), and it was only restricted to pulls where fast-forwarding
is impossible in Git 2.31 (released in March 2021). The good news is
that (unless I'm mistaken) there are no more changes that need to be
made prior to changing the message from from "advise" to "die". All
that needs to be done is to set a date to make the switch. For
comparison, users were given from Git 1.8 to Git 2.0 (October 2012 to
May 2014, 1 year and 7 months) to acclimate when push.default changed
from "matching" to "simple". So how about we plan to stop merging by
default in Git 2.40 (due around the end of 2022 or beginning of 2023),
and update the warning message to advise the users of the pending
behavioral change?

-Alex
Felipe Contreras June 21, 2021, 10:12 p.m. UTC | #4
Alex Henrie wrote:
> On Mon, Jun 21, 2021 at 12:51 PM Felipe Contreras
> <felipe.contreras@gmail.com> wrote:
> >
> > Alex Henrie wrote:
> > > On Mon, Jun 21, 2021 at 11:52 AM Felipe Contreras
> > > <felipe.contreras@gmail.com> wrote:
> > > >
> > > > +                "If unsure, run \"git pull --no-rebase\".\n"
> > >
> > > I don't think the message should recommend merging over rebasing;
> >
> > This is the default strategy.
> 
> Yes, but it shouldn't be,

Feel free to propose a patch to change that, in the meantime this is the
default.

> and we shouldn't make the problem worse by encouraging people to
> default to merging without thinking.

We are not.

> > > The eventual goal is to get rid of the default here and make the user
> > > make an educated choice, which does imply some work on the user's
> > > part, but it avoids the massive headaches created by users merging
> > > without understanding what they're doing.
> >
> > Indeed, but any minute change in git's UI is a gargantuan task that
> > takes several years--or even decades--to accomplish, if it ever happens.
> > I started this patch in 2013, and here we are.
> 
> Although what needs to be done had been envisioned by some as early as
> 2013, the warning has only been around since Git 2.27 (released in
> June 2020), and it was only restricted to pulls where fast-forwarding
> is impossible in Git 2.31 (released in March 2021). The good news is
> that (unless I'm mistaken) there are no more changes that need to be
> made prior to changing the message from from "advise" to "die".

There is *a lot* that needs to be done.

 1. Update the documentation
 2. Add a --merge option (instead of the ackward --no-rebase)
 3. Fix all the wrong behavior with --ff, --no-ff, and -ff-only
 4. Add a pull.mode configuration
 5. Add a configuration for the mode in which we want to die
 6. Fix inconsistencies in the UI

Only *then* can we even begin to throw a warning stating that the
default behavior might change in the future, and how to try the new
behavior.

> All that needs to be done is to set a date to make the switch.

No, there's a lot more.

> For comparison, users were given from Git 1.8 to Git 2.0 (October 2012
> to May 2014, 1 year and 7 months) to acclimate when push.default
> changed from "matching" to "simple".

We haven't told the users the default mode is going to change for a
single day. We don't have a configuration to tell them to turn on to
enable the new mode, nor do we have a configuration to tell them to
enable to stay in the old mode.

There's no configuration in git 2.32 that is equivalent to what I
proposed with pull.mode=fast-forward?


In the meantime there's no reason to have subpar documentation.
Alex Henrie June 22, 2021, 3:15 a.m. UTC | #5
On Mon, Jun 21, 2021 at 4:12 PM Felipe Contreras
<felipe.contreras@gmail.com> wrote:
>
> Alex Henrie wrote:
> > On Mon, Jun 21, 2021 at 12:51 PM Felipe Contreras
> > <felipe.contreras@gmail.com> wrote:
> > >
> > > Alex Henrie wrote:
> > > > The eventual goal is to get rid of the default here and make the user
> > > > make an educated choice, which does imply some work on the user's
> > > > part, but it avoids the massive headaches created by users merging
> > > > without understanding what they're doing.
> > >
> > > Indeed, but any minute change in git's UI is a gargantuan task that
> > > takes several years--or even decades--to accomplish, if it ever happens.
> > > I started this patch in 2013, and here we are.
> >
> > Although what needs to be done had been envisioned by some as early as
> > 2013, the warning has only been around since Git 2.27 (released in
> > June 2020), and it was only restricted to pulls where fast-forwarding
> > is impossible in Git 2.31 (released in March 2021). The good news is
> > that (unless I'm mistaken) there are no more changes that need to be
> > made prior to changing the message from from "advise" to "die".
>
> There is *a lot* that needs to be done.
>
>  1. Update the documentation
>  2. Add a --merge option (instead of the ackward --no-rebase)
>  3. Fix all the wrong behavior with --ff, --no-ff, and -ff-only
>  4. Add a pull.mode configuration
>  5. Add a configuration for the mode in which we want to die
>  6. Fix inconsistencies in the UI

I agree with you that the documentation should be updated when the
change is made (#1), and maybe there should be a config option to go
back to the behavior of warning but doing the merge anyway (#5). The
rest I think are things that would be nice to have but don't preclude
making the switch because aborting instead of merging would not
introduce any new UI limitations or inconsistencies. Of course, it's
ultimately up to Junio and the wider Git community, and I would love
to hear their thoughts about it.

> In the meantime there's no reason to have subpar documentation.

My only serious objection to this patch is the instruction to merge if
you don't know what to do instead of asking the repository maintainer
what to do or reading the Git documentation. I don't have a strong
opinion on the rest of the patch.

-Alex
Felipe Contreras June 22, 2021, 4:26 a.m. UTC | #6
Alex Henrie wrote:
> On Mon, Jun 21, 2021 at 4:12 PM Felipe Contreras
> <felipe.contreras@gmail.com> wrote:
> >
> > Alex Henrie wrote:

> > > Although what needs to be done had been envisioned by some as early as
> > > 2013, the warning has only been around since Git 2.27 (released in
> > > June 2020), and it was only restricted to pulls where fast-forwarding
> > > is impossible in Git 2.31 (released in March 2021). The good news is
> > > that (unless I'm mistaken) there are no more changes that need to be
> > > made prior to changing the message from from "advise" to "die".
> >
> > There is *a lot* that needs to be done.
> >
> >  1. Update the documentation
> >  2. Add a --merge option (instead of the ackward --no-rebase)
> >  3. Fix all the wrong behavior with --ff, --no-ff, and -ff-only
> >  4. Add a pull.mode configuration
> >  5. Add a configuration for the mode in which we want to die
> >  6. Fix inconsistencies in the UI
> 
> I agree with you that the documentation should be updated when the
> change is made (#1),

I'm saying the documentation needs to be updated _before_ the change is
made. There's no reason not to have the fast-forward example in the
documentation.

> and maybe there should be a config option to go back to the behavior
> of warning but doing the merge anyway (#5).

Before that we need a configuration to turn the behavior on.

> The rest I think are things that would be nice to have but don't
> preclude making the switch because aborting instead of merging would
> not introduce any new UI limitations or inconsistencies.

They don't preclude the switch, but the switch should be precluded by a
warning, and the warning would be something like:

  The pull was not fast-forward, in the future you will have to choose a
  merge, or a rebase.

  To quell this message you have two main options:

  1. Adopt the new behavior:

    git config --global pull.mode ff-only

  2. Maintain the current behavior:

    git config --global pull.mode merge

  For now we will fall back to the traditional behavior (merge).

  Read "git pull --help" for more information.

Without having the changes I listed this warning can't be as useful.

> Of course, it's ultimately up to Junio and the wider Git community,
> and I would love to hear their thoughts about it.

I would not hold my breath.

> > In the meantime there's no reason to have subpar documentation.
> 
> My only serious objection to this patch is the instruction to merge if
> you don't know what to do instead of asking the repository maintainer
> what to do or reading the Git documentation. I don't have a strong
> opinion on the rest of the patch.

I would be fine if the patch is merged without that line, but I believe
the patch is better with that line.

The line doesn't say "do this this if you don't know what to do", it
says if you are *unsure*. That is not the same thing.

And the user *already* did a merge, as that's what 'git pull' does by
default. The advice throws a warning, but proceeds with the merge.

The only thing the line is telling the user is how to muffle the message
if she is unsure of the previous muffling options.

Would you be happier with this?

  The simplest way to maintain the current behavior is to just do
  "git pull --no-rebase".

Cheers.
Elijah Newren June 22, 2021, 3:06 p.m. UTC | #7
On Mon, Jun 21, 2021 at 8:15 PM Alex Henrie <alexhenrie24@gmail.com> wrote:
>
> My only serious objection to this patch is the instruction to merge if
> you don't know what to do instead of asking the repository maintainer
> what to do or reading the Git documentation. I don't have a strong
> opinion on the rest of the patch.

You're not alone, Alex; I objected to that part as well.  (See e.g.
https://lore.kernel.org/git/CABPp-BF4rXBOKsn8bG6y3QUEtNVV9K2Pk5NmwrU5818CqhRt_Q@mail.gmail.com/
and various other emails in that thread, ending with "agree to
disagree" later).  I still object to it as I did then.

I'm curious whether it'll just be resubmitted again multiple times,
eventually with a cover letter that repeats something along the lines
of "these are the non-controversial changes from last-year series
which...don't have any reason not to be merged."
Alex Henrie June 22, 2021, 9:22 p.m. UTC | #8
On Tue, Jun 22, 2021 at 9:06 AM Elijah Newren <newren@gmail.com> wrote:
>
> On Mon, Jun 21, 2021 at 8:15 PM Alex Henrie <alexhenrie24@gmail.com> wrote:
> >
> > My only serious objection to this patch is the instruction to merge if
> > you don't know what to do instead of asking the repository maintainer
> > what to do or reading the Git documentation. I don't have a strong
> > opinion on the rest of the patch.
>
> You're not alone, Alex; I objected to that part as well.  (See e.g.
> https://lore.kernel.org/git/CABPp-BF4rXBOKsn8bG6y3QUEtNVV9K2Pk5NmwrU5818CqhRt_Q@mail.gmail.com/
> and various other emails in that thread, ending with "agree to
> disagree" later).  I still object to it as I did then.

Thanks for the link, and sorry for not having followed this
conversation closely enough to have seen your previous replies. While
we're on the subject, do you have any thoughts on what (if anything)
more should be done before making the switch to aborting instead of
merging with a warning in `git pull`?

-Alex
Felipe Contreras June 23, 2021, 1:09 a.m. UTC | #9
Elijah Newren wrote:
> On Mon, Jun 21, 2021 at 8:15 PM Alex Henrie <alexhenrie24@gmail.com> wrote:
> >
> > My only serious objection to this patch is the instruction to merge if
> > you don't know what to do instead of asking the repository maintainer
> > what to do or reading the Git documentation. I don't have a strong
> > opinion on the rest of the patch.
> 
> You're not alone, Alex; I objected to that part as well.  (See e.g.
> https://lore.kernel.org/git/CABPp-BF4rXBOKsn8bG6y3QUEtNVV9K2Pk5NmwrU5818CqhRt_Q@mail.gmail.com/
> and various other emails in that thread, ending with "agree to
> disagree" later).  I still object to it as I did then.

You made your disagreement known in [1], I responded to it with a
devastating argument in [2], and you immediately withtdrew from the
discussion in [3] without engaging my argument at all.

In total you engaged with my arguments zero times.

This is all you replied:

  Yes, we can agree to disagree on this particular point.

To make the record straight, I'm restating the argument you avoided in full:

  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/

> I'm curious whether it'll just be resubmitted again multiple times,
> eventually with a cover letter that repeats something along the lines
> of "these are the non-controversial changes from last-year series
> which...don't have any reason not to be merged."

The fact that **one** person was not 100% on board with a change doesn't
make it controversial.

You made the conscious choice to withdraw from the discussion
immediately, so just like a person who abandons an election cycle and
decides not to vote, you are leving the future of the matter in the
hands of others.

If you want to rejoin the discussion, feel free to respond to my
argument that you dodged last round [3].

Cheers.

[1] https://lore.kernel.org/git/CABPp-BF4rXBOKsn8bG6y3QUEtNVV9K2Pk5NmwBF4rXBOKsn8bG6y3QUEtNVV9K2Pk5NmwrU5818CqhRt_QrU5818CqhRt_Q@mail.gmail.com/
[2] https://lore.kernel.org/git/CAMP44s2L24jhCG9ps72--ZiJkXUovR726jCf8JTLHAs0jV7Whg@mail.gmail.com/
[3] https://lore.kernel.org/git/CABPp-BGdNt8TBMTE9zvaicF5AtvyTBhpiJXqkuZc7mBLGbw0Qw@mail.gmail.com/
Elijah Newren June 23, 2021, 2:20 a.m. UTC | #10
On Tue, Jun 22, 2021 at 2:22 PM Alex Henrie <alexhenrie24@gmail.com> wrote:
>
> On Tue, Jun 22, 2021 at 9:06 AM Elijah Newren <newren@gmail.com> wrote:
> >
> > On Mon, Jun 21, 2021 at 8:15 PM Alex Henrie <alexhenrie24@gmail.com> wrote:
> > >
> > > My only serious objection to this patch is the instruction to merge if
> > > you don't know what to do instead of asking the repository maintainer
> > > what to do or reading the Git documentation. I don't have a strong
> > > opinion on the rest of the patch.
> >
> > You're not alone, Alex; I objected to that part as well.  (See e.g.
> > https://lore.kernel.org/git/CABPp-BF4rXBOKsn8bG6y3QUEtNVV9K2Pk5NmwrU5818CqhRt_Q@mail.gmail.com/
> > and various other emails in that thread, ending with "agree to
> > disagree" later).  I still object to it as I did then.
>
> Thanks for the link, and sorry for not having followed this
> conversation closely enough to have seen your previous replies. While

No worries, you were trying to be a good citizen by reviewing patches,
and the patches didn't come with links to the old threads (even if you
recursively followed links provided in each email as far as I can
tell), so I wouldn't expect you to know.  But I saw you expressing
similar sentiments as I had previously so I dug out my old email and
linked it.

> we're on the subject, do you have any thoughts on what (if anything)
> more should be done before making the switch to aborting instead of
> merging with a warning in `git pull`?

I think Junio already answered that over here:
https://lore.kernel.org/git/xmqq360h8286.fsf@gitster.c.googlers.com/
(he discussed it multiple times in that thread, but hopefully that's a
good enough example).
Felipe Contreras June 23, 2021, 4:18 a.m. UTC | #11
Elijah Newren wrote:
> On Tue, Jun 22, 2021 at 2:22 PM Alex Henrie <alexhenrie24@gmail.com> wrote:
> >
> > On Tue, Jun 22, 2021 at 9:06 AM Elijah Newren <newren@gmail.com> wrote:
> > >
> > > On Mon, Jun 21, 2021 at 8:15 PM Alex Henrie <alexhenrie24@gmail.com> wrote:
> > > >
> > > > My only serious objection to this patch is the instruction to merge if
> > > > you don't know what to do instead of asking the repository maintainer
> > > > what to do or reading the Git documentation. I don't have a strong
> > > > opinion on the rest of the patch.
> > >
> > > You're not alone, Alex; I objected to that part as well.  (See e.g.
> > > https://lore.kernel.org/git/CABPp-BF4rXBOKsn8bG6y3QUEtNVV9K2Pk5NmwrU5818CqhRt_Q@mail.gmail.com/
> > > and various other emails in that thread, ending with "agree to
> > > disagree" later).  I still object to it as I did then.
> >
> > Thanks for the link, and sorry for not having followed this
> > conversation closely enough to have seen your previous replies. While
> 
> No worries, you were trying to be a good citizen by reviewing patches,
> and the patches didn't come with links to the old threads

That's not true. This patch series [1] came with a link to the previous
patch series [2].

I didn't, cannot, and shouldn't contain hundreds of links to the hundres
of responses to this topic over the past 10 years.

[1] https://lore.kernel.org/git/20210621175234.1079004-1-felipe.contreras@gmail.com/
[2] https://lore.kernel.org/git/20201218211026.1937168-1-felipe.contreras@gmail.com/
Elijah Newren June 23, 2021, 6:47 a.m. UTC | #12
On Tue, Jun 22, 2021 at 9:18 PM Felipe Contreras
<felipe.contreras@gmail.com> wrote:
>
> Elijah Newren wrote:
> > On Tue, Jun 22, 2021 at 2:22 PM Alex Henrie <alexhenrie24@gmail.com> wrote:
> > >
....
> > > Thanks for the link, and sorry for not having followed this
> > > conversation closely enough to have seen your previous replies. While
> >
> > No worries, you were trying to be a good citizen by reviewing patches,
> > and the patches didn't come with links to the old threads
>
> That's not true. This patch series [1] came with a link to the previous
> patch series [2].
>
> I didn't, cannot, and shouldn't contain hundreds of links to the hundres
> of responses to this topic over the past 10 years.

Sorry for being unclear; by links I meant either direct or indirect
ones.  Your series came with a link to the previous series.  The
previous series, however, only contained a link to a series you were
basing upon rather than to a series which contained the changes you
were resubmitting.  Thus, following the links in each submission would
not get you back to the old discussions (I double checked).

Also, this particular point was not meant as a critique of your
current submission.  I don't even think it's all that big a deal for
the previous submission either (it's an easy oversight to make given
that Junio submitted a portion of one of your old series).  My point
was simply that Alex didn't need to feel bad for not having been aware
of all the old discussions; even if he had tried to dive deep by
recursively following all the links, he wouldn't find it.
Elijah Newren June 23, 2021, 7:54 a.m. UTC | #13
On Tue, Jun 22, 2021 at 6:09 PM Felipe Contreras
<felipe.contreras@gmail.com> wrote:
>
> Elijah Newren wrote:
...
> > You're not alone, Alex; I objected to that part as well.  (See e.g.
> > https://lore.kernel.org/git/CABPp-BF4rXBOKsn8bG6y3QUEtNVV9K2Pk5NmwrU5818CqhRt_Q@mail.gmail.com/
> > and various other emails in that thread, ending with "agree to
> > disagree" later).  I still object to it as I did then.
>
> You made your disagreement known in [1], I responded to it with a
> devastating argument in [2], and you immediately withtdrew from the
> discussion in [3] without engaging my argument at all.

I didn't find anything new or persuasive in your rehashing of your
arguments.  I had stated my disagreement twice already, and having us
both repeat our arguments does no one any good, so I just stated we
can agree to disagree.

> > I'm curious whether it'll just be resubmitted again multiple times,
> > eventually with a cover letter that repeats something along the lines
> > of "these are the non-controversial changes from last-year series
> > which...don't have any reason not to be merged."
>
> The fact that **one** person was not 100% on board with a change doesn't
> make it controversial.

This is a disconcerting response.  I would have thought perhaps you
might say "Whoops, forgot about that part of the thread", or "Sorry,
didn't mean to include that line".  Perhaps I shouldn't be surprised
that you instead decided to try to redefine goal posts, but it's still
discouraging.

I also find your characterization of the old thread disappointing; I
clearly cared enough to state my objection in three separate emails,
so it's more than just "not 100% on board".  And Junio referred to the
analogy in your "devastating argument" as "irrelevant", so it's not
clear you convinced others either.

> You made the conscious choice to withdraw from the discussion
> immediately, so just like a person who abandons an election cycle and
> decides not to vote, you are leving the future of the matter in the
> hands of others.

This is quite a disappointing argument.  If this position were to be
accepted broadly within the project, it would suggest scorched-earth
last-man standing tactics -- just arguing until the other side runs
out of energy.  If that was used to determine our forward strategy,
it'd result in a massive waste of energy, people feeling drained and
losing motivation to contribute, some people just deciding to leave
the project, and a myriad of other negative outcomes.  In fact,
occurrences of such behavior has already had such outcomes.

Rehashing the same arguments repeatedly damages the discourse within
the project as well as the project itself.  There's no point in doing
so.
Felipe Contreras June 23, 2021, 5:24 p.m. UTC | #14
Elijah Newren wrote:
> On Tue, Jun 22, 2021 at 9:18 PM Felipe Contreras
> <felipe.contreras@gmail.com> wrote:
> >
> > Elijah Newren wrote:
> > > On Tue, Jun 22, 2021 at 2:22 PM Alex Henrie <alexhenrie24@gmail.com> wrote:
> > > >
> ....
> > > > Thanks for the link, and sorry for not having followed this
> > > > conversation closely enough to have seen your previous replies. While
> > >
> > > No worries, you were trying to be a good citizen by reviewing patches,
> > > and the patches didn't come with links to the old threads
> >
> > That's not true. This patch series [1] came with a link to the previous
> > patch series [2].
> >
> > I didn't, cannot, and shouldn't contain hundreds of links to the hundres
> > of responses to this topic over the past 10 years.
> 
> Sorry for being unclear; by links I meant either direct or indirect
> ones.  Your series came with a link to the previous series.  The
> previous series, however, only contained a link to a series you were
> basing upon rather than to a series which contained the changes you
> were resubmitting.  Thus, following the links in each submission would
> not get you back to the old discussions (I double checked).

That's true, but there's no combination of links that would get you all
the discussions.

There were 4 different approaches attempted, each one with their own
thread. Also, since other people didn't follow the threads correctly I
had to send different versions of those threads.

> Also, this particular point was not meant as a critique of your
> current submission.  I don't even think it's all that big a deal for
> the previous submission either (it's an easy oversight to make given
> that Junio submitted a portion of one of your old series).  My point
> was simply that Alex didn't need to feel bad for not having been aware
> of all the old discussions; even if he had tried to dive deep by
> recursively following all the links, he wouldn't find it.

He was aware of the old discussions, as he was part of them, but he
chose to disengage from them [1].

Either way I don't see why would anyone should feel bad about not being
aware of all the hundreds of responses over dozens of threads over
several years.

All we can ask from contributors giving their feedback for free is their
attempt to do their best, nothing more.

Cheers.

[1] http://lore.kernel.org/git/CAMMLpeQA7VW_C4yw_8n6j_SCoGr8k4VUOUaEp98UxUAMR6-MVw@mail.gmail.com
Felipe Contreras June 23, 2021, 6:19 p.m. UTC | #15
Elijah Newren wrote:
> On Tue, Jun 22, 2021 at 6:09 PM Felipe Contreras
> <felipe.contreras@gmail.com> wrote:
> >
> > Elijah Newren wrote:
> ...
> > > You're not alone, Alex; I objected to that part as well.  (See e.g.
> > > https://lore.kernel.org/git/CABPp-BF4rXBOKsn8bG6y3QUEtNVV9K2Pk5NmwrU5818CqhRt_Q@mail.gmail.com/
> > > and various other emails in that thread, ending with "agree to
> > > disagree" later).  I still object to it as I did then.
> >
> > You made your disagreement known in [1], I responded to it with a
> > devastating argument in [2], and you immediately withtdrew from the
> > discussion in [3] without engaging my argument at all.
> 
> I didn't find anything new or persuasive in your rehashing of your
> arguments.

The fact that you don't find something persuasive doesn't mean there
wasn't anything persuasive.

Moreover, the argument was completely new.

> I had stated my disagreement twice already, and having us both repeat
> our arguments does no one any good, so I just stated we can agree to
> disagree.

I did not repeat my argument, I made a completely new argument.

> > > I'm curious whether it'll just be resubmitted again multiple times,
> > > eventually with a cover letter that repeats something along the lines
> > > of "these are the non-controversial changes from last-year series
> > > which...don't have any reason not to be merged."
> >
> > The fact that **one** person was not 100% on board with a change doesn't
> > make it controversial.
> 
> This is a disconcerting response.

Why? That's the definition of the word "controversial". In order for X
to be controversial a substantive amount of people need to have opposing
views.

The fact that a few people deny the roundness of the Earth doesn't make
that idea "controversial".

Similarly, the fact that one person disagrees with X doesn't make X
controversial.

> I also find your characterization of the old thread disappointing; I
> clearly cared enough to state my objection in three separate emails,
> so it's more than just "not 100% on board".

I said "change", not "thread".

And, you were on board with 23 out of 24 lines of the patch. So you were
96% on board. That's just a mathematical fact.

> And Junio referred to the analogy in your "devastating argument" as
> "irrelevant", so it's not clear you convinced others either.

Junio is not infallible. The fact that Junio said X was irrelevant
doesn't mean X was irrelevant.

And in this particular case he was wrong, as I explained in [1], because
he equated apples (regulations) to oranges (policy).

To be specific: this is a false equivalence fallacy.

When I pointed out that fallacy he didn't bother to reply [2].

> > You made the conscious choice to withdraw from the discussion
> > immediately, so just like a person who abandons an election cycle and
> > decides not to vote, you are leving the future of the matter in the
> > hands of others.
> 
> This is quite a disappointing argument.  If this position were to be
> accepted broadly within the project, it would suggest scorched-earth
> last-man standing tactics -- just arguing until the other side runs
> out of energy.  If that was used to determine our forward strategy,
> it'd result in a massive waste of energy, people feeling drained and
> losing motivation to contribute, some people just deciding to leave
> the project, and a myriad of other negative outcomes.

This is how progress is achieved in all areas of society, from public
debates, to trials, to elections.

You can't just stand up and leave when the debate is only 25% complete,
and then expect to win it.

Either you care enough see it through, or you don't.

Even in sports, and in video games, completing the match is the bare
minimum to win.

Unlike in a video game, the fact that you left the debate doesn't
necessarily mean you lost, but you definitely did not win it.

> In fact, occurrences of such behavior has already had such outcomes.

That's your opinion, my opinion is the exact opposite.

The reason people have left the project is because even though they
care, and they stay for 100% of the race, if some bigwig who only said
"I disagree" and left having only participated in 10% of the debate, the
debate is over, always in favor of the incumbents.

This is not a fair marketplace of ideas where the best ideas win, it's
not a meritocracy, and discourages anyone who is not already part of the
big club.

> Rehashing the same arguments repeatedly damages the discourse within
> the project as well as the project itself.  There's no point in doing
> so.

All philosophers would disagree with you.

There are some debates that have lasted for millennia and still continue
today. Many arguments for free will are repeated exactly the same, but
some are slightly different, and merit a new look. As long as we don't
solve this debate, it will continue, and everyone that cares sees value
in it.

Similarly, until "git pull" does something sensible by default (which
isn't the case now), these debates will continue, and there's value in
them.

Perhaps if other people didn't stand up and left in the middle of the
debate in 2013 we would have solved the issue back then and we wouldn't
be here.

But here we are yet again, and if people continue ignoring arguments and
leaving the debate before it's even 10% done, we will be here again in
2022, 2024, and probably more.

Especially if they straight-up reject patches that according to
themselves are 96% perfectly fine.

Cheers.

[1] https://lore.kernel.org/git/CAMP44s2XFQoda_PMULWha-rj9HhNfEddO5fikmswk9=AWN4RCw@mail.gmail.com/
[2] https://lore.kernel.org/git/xmqqpn3lbhxn.fsf@gitster.c.googlers.com/
Alex Henrie June 24, 2021, 3:38 a.m. UTC | #16
On Tue, Jun 22, 2021 at 8:20 PM Elijah Newren <newren@gmail.com> wrote:
>
> On Tue, Jun 22, 2021 at 2:22 PM Alex Henrie <alexhenrie24@gmail.com> wrote:
> >
> > While
> > we're on the subject, do you have any thoughts on what (if anything)
> > more should be done before making the switch to aborting instead of
> > merging with a warning in `git pull`?
>
> I think Junio already answered that over here:
> https://lore.kernel.org/git/xmqq360h8286.fsf@gitster.c.googlers.com/
> (he discussed it multiple times in that thread, but hopefully that's a
> good enough example).

Wow, if I understand correctly from that message, Junio wouldn't mind
making the switch right away. That's very encouraging.

On Wed, Jun 23, 2021 at 12:19 PM Felipe Contreras
<felipe.contreras@gmail.com> wrote:
>
> Similarly, until "git pull" does something sensible by default (which
> isn't the case now), these debates will continue, and there's value in
> them.

At this point, I'm inclined to push for s/advise/die/ in pull.c in the
next release, without a transitional period, just to end the argument
over how to best explain the current awkward situation. (I'm sure
there will be more arguments after that, but hopefully they won't be
as tiresome.)

-Alex
Felipe Contreras June 24, 2021, 5:55 a.m. UTC | #17
Alex Henrie wrote:
> On Tue, Jun 22, 2021 at 8:20 PM Elijah Newren <newren@gmail.com> wrote:
> >
> > On Tue, Jun 22, 2021 at 2:22 PM Alex Henrie <alexhenrie24@gmail.com> wrote:
> > >
> > > While
> > > we're on the subject, do you have any thoughts on what (if anything)
> > > more should be done before making the switch to aborting instead of
> > > merging with a warning in `git pull`?
> >
> > I think Junio already answered that over here:
> > https://lore.kernel.org/git/xmqq360h8286.fsf@gitster.c.googlers.com/
> > (he discussed it multiple times in that thread, but hopefully that's a
> > good enough example).
> 
> Wow, if I understand correctly from that message, Junio wouldn't mind
> making the switch right away. That's very encouraging.

Junio has not paid enough attention to this topic. He is not aware of
all the problems, and once he does he will very likely change his mind.

> On Wed, Jun 23, 2021 at 12:19 PM Felipe Contreras
> <felipe.contreras@gmail.com> wrote:
> >
> > Similarly, until "git pull" does something sensible by default (which
> > isn't the case now), these debates will continue, and there's value in
> > them.
> 
> At this point, I'm inclined to push for s/advise/die/ in pull.c in the
> next release, without a transitional period, just to end the argument
> over how to best explain the current awkward situation. (I'm sure
> there will be more arguments after that, but hopefully they won't be
> as tiresome.)

Give it a try. You will inevitably stumble upon all the problems I
already fixed.

In the meantime what's the problem with v2?

Cheers.

[1] https://lore.kernel.org/git/20210623004815.1807-1-felipe.contreras@gmail.com/
Alex Henrie June 27, 2021, 12:17 a.m. UTC | #18
On Wed, Jun 23, 2021 at 11:55 PM Felipe Contreras
<felipe.contreras@gmail.com> wrote:
>
> Alex Henrie wrote:
> > On Wed, Jun 23, 2021 at 12:19 PM Felipe Contreras
> > <felipe.contreras@gmail.com> wrote:
> > >
> > > Similarly, until "git pull" does something sensible by default (which
> > > isn't the case now), these debates will continue, and there's value in
> > > them.
> >
> > At this point, I'm inclined to push for s/advise/die/ in pull.c in the
> > next release, without a transitional period, just to end the argument
> > over how to best explain the current awkward situation. (I'm sure
> > there will be more arguments after that, but hopefully they won't be
> > as tiresome.)
>
> Give it a try. You will inevitably stumble upon all the problems I
> already fixed.

Patch sent.

> In the meantime what's the problem with v2?

I think that setting pull.rebase on a per-repository basis (instead of
globally or per-invocation) makes for the easiest workflow in the
majority of cases, so I would prefer to continue to recommend that to
users primarily, but I don't have a strong opinion.

-Alex
Felipe Contreras June 27, 2021, 4:21 a.m. UTC | #19
Alex Henrie wrote:
> On Wed, Jun 23, 2021 at 11:55 PM Felipe Contreras
> <felipe.contreras@gmail.com> wrote:
> >
> > Alex Henrie wrote:
> > > On Wed, Jun 23, 2021 at 12:19 PM Felipe Contreras
> > > <felipe.contreras@gmail.com> wrote:
> > > >
> > > > Similarly, until "git pull" does something sensible by default (which
> > > > isn't the case now), these debates will continue, and there's value in
> > > > them.
> > >
> > > At this point, I'm inclined to push for s/advise/die/ in pull.c in the
> > > next release, without a transitional period, just to end the argument
> > > over how to best explain the current awkward situation. (I'm sure
> > > there will be more arguments after that, but hopefully they won't be
> > > as tiresome.)
> >
> > Give it a try. You will inevitably stumble upon all the problems I
> > already fixed.
> 
> Patch sent.

I sent a bunch of comments to the approach you sent. I think that's not
all the issues, but it's been a while since the last time I took a good
look at this, I feel I'm still missing one issue.

> > In the meantime what's the problem with v2?
> 
> I think that setting pull.rebase on a per-repository basis (instead of
> globally or per-invocation) makes for the easiest workflow in the
> majority of cases, so I would prefer to continue to recommend that to
> users primarily, but I don't have a strong opinion.

OK. What happens if you don't have the configuration in the particular
repository you are using?
diff mbox series

Patch

diff --git a/builtin/pull.c b/builtin/pull.c
index 3e13f81084..48e25a5061 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -927,18 +927,20 @@  static int get_can_ff(struct object_id *orig_head, struct object_id *orig_merge_
 
 static void show_advice_pull_non_ff(void)
 {
-	advise(_("Pulling without specifying how to reconcile divergent branches is\n"
-		 "discouraged. You can squelch this message by running one of the following\n"
-		 "commands sometime before your next pull:\n"
+	advise(_("Pulling without specifying how to reconcile divergent branches is discouraged;\n"
+		 "you need to specify if you want a merge, or a rebase.\n"
 		 "\n"
-		 "  git config pull.rebase false  # merge (the default strategy)\n"
-		 "  git config pull.rebase true   # rebase\n"
-		 "  git config pull.ff only       # fast-forward only\n"
+		 "  git pull --no-rebase # the default (merge)\n"
+		 "  git pull --rebase\n"
 		 "\n"
-		 "You can replace \"git config\" with \"git config --global\" to set a default\n"
-		 "preference for all repositories. You can also pass --rebase, --no-rebase,\n"
-		 "or --ff-only on the command line to override the configured default per\n"
-		 "invocation.\n"));
+		 "You can squelch this message by running one of the following commands:\n"
+		 "\n"
+		 "  git config --global pull.rebase false  # merge\n"
+		 "  git config --global pull.rebase true   # rebase\n"
+		 "  git config --global pull.ff only       # fast-forward only\n"
+		 "\n"
+		 "If unsure, run \"git pull --no-rebase\".\n"
+		 "Read \"git pull --help\" for more information."));
 }
 
 int cmd_pull(int argc, const char **argv, const char *prefix)