threads / patch / 56138

patchdoc: pull: fix rebase=false documentation

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

## tl;dr

12 messages between Jul 21, 2021 and Jul 24, 2021. Diffs are folded; open one to read it.

replies: 11people: 4as markdown or json

Felipe Contreras· Jul 21, 2021, 22:15 UTC · lore

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(-)
Show changes to Documentation/git-pull.txt +1 −1
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
Junio C Hamano· Jul 21, 2021, 23:33 UTC · re: Felipe Contreras · lore

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

Felipe Contreras <felipe.contreras@gmail.com> writes:
Show 13 quoted lines
> 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.
Junio C Hamano· Jul 22, 2021, 00:21 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <gitster@pobox.com> writes:
Show 17 quoted lines
> 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/
Felipe Contreras· Jul 22, 2021, 01:24 UTC · re: Junio C Hamano · lore

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

Junio C Hamano wrote:
Show 22 quoted lines
> 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
Jeff King· Jul 23, 2021, 07:30 UTC · re: Felipe Contreras · lore

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

On Wed, Jul 21, 2021 at 08:24:25PM -0500, Felipe Contreras wrote:
Show 9 quoted lines
> 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:

Show 17 quoted lines
> 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
Philip Oakley· Jul 23, 2021, 10:05 UTC · re: Jeff King · lore

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

On 23/07/2021 08:30, Jeff King wrote:
Show 33 quoted lines
> 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.

Show 6 quoted lines
> 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

Felipe Contreras· Jul 23, 2021, 16:54 UTC · re: Philip Oakley · lore

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

Philip Oakley wrote:
Show 37 quoted lines
> 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
Junio C Hamano· Jul 23, 2021, 15:58 UTC · re: Jeff King · lore

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

Jeff King <peff@peff.net> writes:
Show 6 quoted lines
> 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.

Felipe Contreras· Jul 23, 2021, 18:29 UTC · re: Junio C Hamano · lore

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

Junio C Hamano wrote:
Show 26 quoted lines
> 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
Junio C Hamano· Jul 23, 2021, 21:44 UTC · re: Felipe Contreras · lore

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

Felipe Contreras <felipe.contreras@gmail.com> writes:
Show 5 quoted lines
>> 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.

Felipe Contreras· Jul 24, 2021, 03:56 UTC · re: Junio C Hamano · lore

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

Junio C Hamano wrote:
Show 11 quoted lines
> 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
Felipe Contreras· Jul 23, 2021, 16:36 UTC · re: Jeff King · lore

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

Jeff King wrote:
Show 35 quoted lines
> 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

← back to recent threads