[PATCH] doc: pull: fix rebase=false documentation

[PATCH] doc: pull: fix rebase=false documentation

[Felipe Contreras]

The behavior of --rebase=false is obviously wrong (as has been discussed
in big threads before [1] [2]): we want our users to merge their current
branch into the upstream branch, but this is not what the code is
currently doing; it does the *opposite*.

Fix the documentation so that we explain what the code actually does,
not what we wish it did.

[1] https://lore.kernel.org/git/20130522115042.GA20649@inner.h.apk.li/
[2] https://lore.kernel.org/git/4ay6w9i74cygt6ii1b0db7wg.1398433713382@email.android.com/

Cc: Stephen Haberman <stephen@exigencecorp.com>
Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 Documentation/git-pull.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt
index 5c3fb67c01..7f4b2d1982 100644
--- a/Documentation/git-pull.txt
+++ b/Documentation/git-pull.txt
@@ -117,7 +117,7 @@ When set to `preserve` (deprecated in favor of `merges`), rebase with the
 `--preserve-merges` option passed to `git rebase` so that locally created
 merge commits will not be flattened.
 +
-When false, merge the current branch into the upstream branch.
+When false, merge the upstream branch into the current branch.
 +
 When `interactive`, enable the interactive mode of rebase.
 +
-- 
2.32.0.40.gb9b36f9b52

Re: [PATCH] doc: pull: fix rebase=false documentation

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> diff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt
> index 5c3fb67c01..7f4b2d1982 100644
> --- a/Documentation/git-pull.txt
> +++ b/Documentation/git-pull.txt
> @@ -117,7 +117,7 @@ When set to `preserve` (deprecated in favor of `merges`), rebase with the
>  `--preserve-merges` option passed to `git rebase` so that locally created
>  merge commits will not be flattened.
>  +
> -When false, merge the current branch into the upstream branch.
> +When false, merge the upstream branch into the current branch.
>  +
>  When `interactive`, enable the interactive mode of rebase.
>  +

Looks correct.  Will queue.  Thanks.

Re: [PATCH] doc: pull: fix rebase=false documentation

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> diff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt
>> index 5c3fb67c01..7f4b2d1982 100644
>> --- a/Documentation/git-pull.txt
>> +++ b/Documentation/git-pull.txt
>> @@ -117,7 +117,7 @@ When set to `preserve` (deprecated in favor of `merges`), rebase with the
>>  `--preserve-merges` option passed to `git rebase` so that locally created
>>  merge commits will not be flattened.
>>  +
>> -When false, merge the current branch into the upstream branch.
>> +When false, merge the upstream branch into the current branch.
>>  +
>>  When `interactive`, enable the interactive mode of rebase.
>>  +
>
> Looks correct.  Will queue.  Thanks.

By the way, I'll update the proposed log message to say only that
the documentation needs to be fixed as it does not say what the
command does.  We should be able to fix the inaccuracies in the
documentation quickly without advocating different behaviour or
trashing the current behaviour in the proposed log message.

I also happen to think that "flipping the merge order" is not a good
thing to do anyway [*1*]; keeping the log message to state just "the
description does not reflect reality-fix it" has the added benefit
that we do not have to debate it.


[Footnote]

*1* https://public-inbox.org/git/7vd2shheic.fsf@alter.siamese.dyndns.org/

Re: [PATCH] doc: pull: fix rebase=false documentation

[Felipe Contreras]

Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> > Felipe Contreras <felipe.contreras@gmail.com> writes:
> >
> >> diff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt
> >> index 5c3fb67c01..7f4b2d1982 100644
> >> --- a/Documentation/git-pull.txt
> >> +++ b/Documentation/git-pull.txt
> >> @@ -117,7 +117,7 @@ When set to `preserve` (deprecated in favor of `merges`), rebase with the
> >>  `--preserve-merges` option passed to `git rebase` so that locally created
> >>  merge commits will not be flattened.
> >>  +
> >> -When false, merge the current branch into the upstream branch.
> >> +When false, merge the upstream branch into the current branch.
> >>  +
> >>  When `interactive`, enable the interactive mode of rebase.
> >>  +
> >
> > Looks correct.  Will queue.  Thanks.
> 
> By the way, I'll update the proposed log message to say only that
> the documentation needs to be fixed as it does not say what the
> command does.

Fine by me, although I'd appreciate if you minimize the use of your
words and maximize the use of mine.

  The documentation says --rebase=false merges our current branch into
  the upstream branch, and while many people think that's what should
  happen, that's not what actually happens; it's the *opposite*.

  Fix the documentation so that we explain what the code actually does.

> We should be able to fix the inaccuracies in the
> documentation quickly without advocating different behaviour or
> trashing the current behaviour in the proposed log message.

I'm not trashing the current behavior, I'm explaining what the consensus
is. I spent several man-days re-reading old threads, and this is the
consensus of what should happen:

  1. git pull              # merge HEAD into upstream
  2. git pull origin topic # merge topic into HEAD

Of the people that expressed an opinion, 100% of them stated that what
`git pull` does in the first case today is not desirable.

> I also happen to think that "flipping the merge order" is not a good
> thing to do anyway [*1*]; keeping the log message to state just "the
> description does not reflect reality-fix it" has the added benefit
> that we do not have to debate it.

But people have spent many hours debating it already.

Yes, you are correct that if *everyone* followed the topic branch
workflow, everything would work correctly, but that's not what happens
in reality, in reality people do all kinds of workflows, and wrong
merges are pervasive.

Everyone--including Linus, Jeff, and you--agree that there's two
different ways of using `git pull`: integrator versus developer.

When a user is doing `git pull` to synchronize changes to push to the
same branch, that's a centralized two-way workflow, so he is acting both
as an integrator and as a developer, and it's in that particular case
that the order of the parents should be reversed. Everyone agrees on
that.

When the user the opposite explicitely: `git pull origin master`
Linus calls it a "back-merge" [1], and in that case the order of the
parents should not be reversed.

This has already been discussed many times, and the problem remains.

[1] https://lore.kernel.org/git/CA+55aFwM0vy+pw-Xv=gA19ULMwAXNPhdO3qR5A3hkMrZKJFNSQ@mail.gmail.com/

-- 
Felipe Contreras

Re: [PATCH] doc: pull: fix rebase=false documentation

[Jeff King]

On Wed, Jul 21, 2021 at 08:24:25PM -0500, Felipe Contreras wrote:

> I'm not trashing the current behavior, I'm explaining what the consensus
> is. I spent several man-days re-reading old threads, and this is the
> consensus of what should happen:
> 
>   1. git pull              # merge HEAD into upstream
>   2. git pull origin topic # merge topic into HEAD
> 
> Of the people that expressed an opinion, 100% of them stated that what
> `git pull` does in the first case today is not desirable.

I did not participate in the threads you linked earlier, so I am
probably not in that 100%. But you did use my name below:

> Yes, you are correct that if *everyone* followed the topic branch
> workflow, everything would work correctly, but that's not what happens
> in reality, in reality people do all kinds of workflows, and wrong
> merges are pervasive.
> 
> Everyone--including Linus, Jeff, and you--agree that there's two
> different ways of using `git pull`: integrator versus developer.
> 
> When a user is doing `git pull` to synchronize changes to push to the
> same branch, that's a centralized two-way workflow, so he is acting both
> as an integrator and as a developer, and it's in that particular case
> that the order of the parents should be reversed. Everyone agrees on
> that.
> 
> When the user the opposite explicitely: `git pull origin master`
> Linus calls it a "back-merge" [1], and in that case the order of the
> parents should not be reversed.

So I feel compelled to say now that I do not think that changing the
order of parents for "git pull" is the obviously correct thing to do.
And likewise, in the one thread I do remember participating in, I
expressed something similar:

  https://lore.kernel.org/git/20140502214817.GA10801@sigill.intra.peff.net/

-Peff

Re: [PATCH] doc: pull: fix rebase=false documentation

[Philip Oakley]

On 23/07/2021 08:30, Jeff King wrote:
> On Wed, Jul 21, 2021 at 08:24:25PM -0500, Felipe Contreras wrote:
>
>> I'm not trashing the current behavior, I'm explaining what the consensus
>> is. I spent several man-days re-reading old threads, and this is the
>> consensus of what should happen:
>>
>>   1. git pull              # merge HEAD into upstream
>>   2. git pull origin topic # merge topic into HEAD
>>
>> Of the people that expressed an opinion, 100% of them stated that what
>> `git pull` does in the first case today is not desirable.
> I did not participate in the threads you linked earlier, so I am
> probably not in that 100%. But you did use my name below:
>
>> Yes, you are correct that if *everyone* followed the topic branch
>> workflow, everything would work correctly, but that's not what happens
>> in reality, in reality people do all kinds of workflows, and wrong
>> merges are pervasive.
>>
>> Everyone--including Linus, Jeff, and you--agree that there's two
>> different ways of using `git pull`: integrator versus developer.
>>
>> When a user is doing `git pull` to synchronize changes to push to the
>> same branch, that's a centralized two-way workflow, so he is acting both
>> as an integrator and as a developer, and it's in that particular case
>> that the order of the parents should be reversed. Everyone agrees on
>> that.
>>
>> When the user the opposite explicitely: `git pull origin master`
>> Linus calls it a "back-merge" [1], and in that case the order of the
>> parents should not be reversed.
> So I feel compelled to say now that I do not think that changing the
> order of parents for "git pull" is the obviously correct thing to do.
While I never `pull` because it's not right for me as a 'contributor', I
do agree that the default 'maintainer' view of `pull` will need to be
retained for long term backward compatibility.

What I have rarely seen in the discussion is explanation that is based
on workflow style, though the potential `update` command (1) may break
some of the deadlock about the direction of 'pull requests', and
possibly confusion regarding the location of the 'golden' publish repo.

(1) there are a lot of 'update' commands floating about, esp on Git for
Windows. If there is a suitably named `update` command to do the `pull
--contributor` merge-ff swap then many of the issues could fade away.

> And likewise, in the one thread I do remember participating in, I
> expressed something similar:
>
>   https://lore.kernel.org/git/20140502214817.GA10801@sigill.intra.peff.net/
>
> -Peff
--
Philip

Re: [PATCH] doc: pull: fix rebase=false documentation

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> So I feel compelled to say now that I do not think that changing the
> order of parents for "git pull" is the obviously correct thing to do.
> And likewise, in the one thread I do remember participating in, I
> expressed something similar:
>
>   https://lore.kernel.org/git/20140502214817.GA10801@sigill.intra.peff.net/

Thanks for the link.  Many articles in the thread are repeating the
same opinion over and over (and later even descend into ad-hominem
attacks) and it is not worth anybody's time to read all of them, but
I found that there still were some gems.

In an worldview where the first-parent chain is the trunk history,
merging in the upstream where you push back to into your working
repository where your new work is happening as the second parent
before pushing it back would obviously make the history that used to
be trunk to lose the first-parent-ness at that point.  And if you
ask if I just said is correct, everybody would say it is.  So there
is a concensus that the result of "git pull upstream main" becomes
a wrong shape for people in one workflow.

But that does not necessarily mean swapping the parent order would
produce the history of a right shape, either, even for those with
the "first-parent chain is the trunk" worldview.

Re: [PATCH] doc: pull: fix rebase=false documentation

[Felipe Contreras]

Jeff King wrote:
> On Wed, Jul 21, 2021 at 08:24:25PM -0500, Felipe Contreras wrote:
> 
> > I'm not trashing the current behavior, I'm explaining what the consensus
> > is. I spent several man-days re-reading old threads, and this is the
> > consensus of what should happen:
> > 
> >   1. git pull              # merge HEAD into upstream
> >   2. git pull origin topic # merge topic into HEAD
> > 
> > Of the people that expressed an opinion, 100% of them stated that what
> > `git pull` does in the first case today is not desirable.
> 
> I did not participate in the threads you linked earlier, so I am
> probably not in that 100%. But you did use my name below:
> 
> > Yes, you are correct that if *everyone* followed the topic branch
> > workflow, everything would work correctly, but that's not what happens
> > in reality, in reality people do all kinds of workflows, and wrong
> > merges are pervasive.
> > 
> > Everyone--including Linus, Jeff, and you--agree that there's two
> > different ways of using `git pull`: integrator versus developer.
> > 
> > When a user is doing `git pull` to synchronize changes to push to the
> > same branch, that's a centralized two-way workflow, so he is acting both
> > as an integrator and as a developer, and it's in that particular case
> > that the order of the parents should be reversed. Everyone agrees on
> > that.
> > 
> > When the user the opposite explicitely: `git pull origin master`
> > Linus calls it a "back-merge" [1], and in that case the order of the
> > parents should not be reversed.
> 
> So I feel compelled to say now that I do not think that changing the
> order of parents for "git pull" is the obviously correct thing to do.

That's not the same as saying it's the wrong thing to do.

More importantly, while for you it might not be the obviously correct
thing to do, it should be obvious that there is a problem. Whether
reversing the parents in case #1 is the best solution or not is
debatable.

> And likewise, in the one thread I do remember participating in, I
> expressed something similar:
> 
>   https://lore.kernel.org/git/20140502214817.GA10801@sigill.intra.peff.net/

In that comment you mentioned that you were not against reversing the
order of the parents, but if we did, that it should done with a
configuration that the user explicitely turns on:

  I wanted to make one point: if we are going to do such a switch, let's
  please make it something the user explicitly turns on.

I am not against adding a configuration to reverse the parents only on
case #1. Additionally we could delineate a migration path that mentions
that in the future this will be default, although that can be done and
discussed later.

But the fact that there's a problem is not debatable.


Either way it's precisely because `git pull` is a command with so much
inertia that it's hard to change (although not impossible), that I
decided to create a new `git update` command whose whole purpose is to
implement case #1, which is intended for normal developers, not
integrators, and there the order of the parents is reversed by default.

-- 
Felipe Contreras

Re: [PATCH] doc: pull: fix rebase=false documentation

[Felipe Contreras]

Philip Oakley wrote:
> On 23/07/2021 08:30, Jeff King wrote:
> > On Wed, Jul 21, 2021 at 08:24:25PM -0500, Felipe Contreras wrote:
> >
> >> I'm not trashing the current behavior, I'm explaining what the consensus
> >> is. I spent several man-days re-reading old threads, and this is the
> >> consensus of what should happen:
> >>
> >>   1. git pull              # merge HEAD into upstream
> >>   2. git pull origin topic # merge topic into HEAD
> >>
> >> Of the people that expressed an opinion, 100% of them stated that what
> >> `git pull` does in the first case today is not desirable.
> > I did not participate in the threads you linked earlier, so I am
> > probably not in that 100%. But you did use my name below:
> >
> >> Yes, you are correct that if *everyone* followed the topic branch
> >> workflow, everything would work correctly, but that's not what happens
> >> in reality, in reality people do all kinds of workflows, and wrong
> >> merges are pervasive.
> >>
> >> Everyone--including Linus, Jeff, and you--agree that there's two
> >> different ways of using `git pull`: integrator versus developer.
> >>
> >> When a user is doing `git pull` to synchronize changes to push to the
> >> same branch, that's a centralized two-way workflow, so he is acting both
> >> as an integrator and as a developer, and it's in that particular case
> >> that the order of the parents should be reversed. Everyone agrees on
> >> that.
> >>
> >> When the user the opposite explicitely: `git pull origin master`
> >> Linus calls it a "back-merge" [1], and in that case the order of the
> >> parents should not be reversed.
> > So I feel compelled to say now that I do not think that changing the
> > order of parents for "git pull" is the obviously correct thing to do.
> While I never `pull` because it's not right for me as a 'contributor', I
> do agree that the default 'maintainer' view of `pull` will need to be
> retained for long term backward compatibility.

Of course, but a maintainer never does `git pull` to merge a pull request,
she does `git pull github/john topic`, does she not?

Nobody was in favor of reversing the parents in the case of
`git pull $where $what`, that would be the wrong thing to do.

So the maintainer view of `git pull` would remain fine.

> What I have rarely seen in the discussion is explanation that is based
> on workflow style, though the potential `update` command (1) may break
> some of the deadlock about the direction of 'pull requests', and
> possibly confusion regarding the location of the 'golden' publish repo.

I think that's because most of the people that follow a workflow don't
have this problem.

It's only newcomers that don't follow any workflow that are hit by this.
Another name for this no-workflow is trunk-based development [1].
Essentially everyone pulls and pushes to the same branch.

People that use topic branches don't need `git update`, people who
follow trunk-based development do.

> (1) there are a lot of 'update' commands floating about, esp on Git for
> Windows. If there is a suitably named `update` command to do the `pull
> --contributor` merge-ff swap then many of the issues could fade away.

Indeed. And at least when I was maintaining my git-fc fork, people did
enjoy my implementation of `git update`.

[1] https://trunkbaseddevelopment.com/

-- 
Felipe Contreras

Re: [PATCH] doc: pull: fix rebase=false documentation

[Felipe Contreras]

Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
> 
> > So I feel compelled to say now that I do not think that changing the
> > order of parents for "git pull" is the obviously correct thing to do.
> > And likewise, in the one thread I do remember participating in, I
> > expressed something similar:
> >
> >   https://lore.kernel.org/git/20140502214817.GA10801@sigill.intra.peff.net/
> 
> Thanks for the link.  Many articles in the thread are repeating the
> same opinion over and over (and later even descend into ad-hominem
> attacks) and it is not worth anybody's time to read all of them, but
> I found that there still were some gems.
> 
> In an worldview where the first-parent chain is the trunk history,
> merging in the upstream where you push back to into your working
> repository where your new work is happening as the second parent
> before pushing it back would obviously make the history that used to
> be trunk to lose the first-parent-ness at that point.  And if you
> ask if I just said is correct, everybody would say it is.  So there
> is a concensus that the result of "git pull upstream main" becomes
> a wrong shape for people in one workflow.
> 
> But that does not necessarily mean swapping the parent order would
> produce the history of a right shape, either, even for those with
> the "first-parent chain is the trunk" worldview.

Why not? Everyone who saw a problem agreed it would.

Reversing the order of the parents creates a merge commit like so:

  Y---X-+
         \
  B---A---M  trunk

Most git experts work with topic branches, and when you do that, you get
the same thing:

    topic
      |
      v
  Y---X-+
         \
  B---A---M  master

If you merge topic to master, the first parent of the merge commit is A.

If you do `git pull --reverse-parents` on a trunk-based workflow as
above, you would get exactly the same shape of the history.

How is it not the right shape?

-- 
Felipe Contreras

Re: [PATCH] doc: pull: fix rebase=false documentation

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> writes:

>> But that does not necessarily mean swapping the parent order would
>> produce the history of a right shape, either, even for those with
>> the "first-parent chain is the trunk" worldview.
>
> Why not?

I gave detailed write-up in two discussion threads linked already in
this thread from 2013 and 2014; as I said, I do not think we want to
debate that as part of the discussion of this documentation fix.

Re: [PATCH] doc: pull: fix rebase=false documentation

[Felipe Contreras]

Junio C Hamano wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
> 
> >> But that does not necessarily mean swapping the parent order would
> >> produce the history of a right shape, either, even for those with
> >> the "first-parent chain is the trunk" worldview.
> >
> > Why not?
> 
> I gave detailed write-up in two discussion threads linked already in
> this thread from 2013 and 2014; as I said, I do not think we want to
> debate that as part of the discussion of this documentation fix.

Sure, the discussion about the order of the parents is orthogonal to the
discussion of this particular documentation fix, but my comment that the
order of the parents is "obviously wrong" is correct. Even you said so
yourself in the 2013/2014 threads (I would gladly provide the links if
you require them).

I've spent several man-days re-reading the old threads (multiple times
even, and not just the ones from 2013/1014), and I just spent several
man-hours re-reading them yet again today.

My conclussion remains: the fact that the behavior is wrong is not
debatable (at least in some of the circumstances).

I will provide a summary in a different thread soon.

-- 
Felipe Contreras