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

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

[Dragan Simic]

Mention in the documentation that --rebase defaults to false.

Signed-off-by: Dragan Simic <dsimic@manjaro.org>
---
 Documentation/git-pull.txt | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt
index 0e14f8b5b2..d28790388e 100644
--- a/Documentation/git-pull.txt
+++ b/Documentation/git-pull.txt
@@ -116,7 +116,8 @@ When set to `merges`, rebase using `git rebase --rebase-merges` so that
 the local merge commits are included in the rebase (see
 linkgit:git-rebase[1] for details).
 +
-When false, merge the upstream branch into the current branch.
+When false, merge the upstream branch into the current branch. This is
+the default.
 +
 When `interactive`, enable the interactive mode of rebase.
 +

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

[Junio C Hamano]

Dragan Simic <dsimic@manjaro.org> writes:

> Mention in the documentation that --rebase defaults to false.

I am not quite sure if this is an improvement, though.

It is true that, if you do not have any of your own funny
configuration, your "git pull" will *not* rebase.  

But that is "if you do not give the --rebase option, you do not
trigger the rebase action".  Which is quite natural, but it is
different from what you wrote above, isn't it?

When people say "the default for `--rebase` is false", they mean
this:

    I can say "git pull --rebase=<kind>" to specify how my current
    branch is rebased on top of the upstream.  But if I do not
    specify the <kind>, i.e. "git pull --rebase", the command will
    act as if I gave 'false' as the <kind>.

At least, I would think that is what the word "default" means.

And I would be surprised if the "default" in that sense is 'false';
isn't the default <kind> 'true' --- meaning "perform the plain
vanilla 'git rebase'", unless you explicitly asked for 'merges',
'interactive' or 'false'?

After the context of the hunk your patch touches, there is a
description on `pull.rebase`.  Down there, if you do not set
`pull.rebase` or `branch.<name>.rebase` for the current branch at
all, the system acts as if you had `false` for these variables.  In
other words, the default for these variables is `false`, meaning "do
not rebase, just merge".  But the default option argument for the
`--rebase` option given without argument would not be `false`, I
would think.

> Signed-off-by: Dragan Simic <dsimic@manjaro.org>
> ---
>  Documentation/git-pull.txt | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt
> index 0e14f8b5b2..d28790388e 100644
> --- a/Documentation/git-pull.txt
> +++ b/Documentation/git-pull.txt
> @@ -116,7 +116,8 @@ When set to `merges`, rebase using `git rebase --rebase-merges` so that
>  the local merge commits are included in the rebase (see
>  linkgit:git-rebase[1] for details).
>  +
> -When false, merge the upstream branch into the current branch.
> +When false, merge the upstream branch into the current branch. This is
> +the default.
>  +
>  When `interactive`, enable the interactive mode of rebase.
>  +

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

[Linus Arver]

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

> Dragan Simic <dsimic@manjaro.org> writes:
>
>> Mention in the documentation that --rebase defaults to false.
>
> I am not quite sure if this is an improvement, though.
>
> It is true that, if you do not have any of your own funny
> configuration, your "git pull" will *not* rebase.  
>
> But that is "if you do not give the --rebase option, you do not
> trigger the rebase action".  Which is quite natural, but it is
> different from what you wrote above, isn't it?
>
> When people say "the default for `--rebase` is false", they mean
> this:
>
>     I can say "git pull --rebase=<kind>" to specify how my current
>     branch is rebased on top of the upstream.  But if I do not
>     specify the <kind>, i.e. "git pull --rebase", the command will
>     act as if I gave 'false' as the <kind>.
>
> At least, I would think that is what the word "default" means.
>
> And I would be surprised if the "default" in that sense is 'false';
> isn't the default <kind> 'true' --- meaning "perform the plain
> vanilla 'git rebase'", unless you explicitly asked for 'merges',
> 'interactive' or 'false'?
>
> After the context of the hunk your patch touches, there is a
> description on `pull.rebase`.  Down there, if you do not set
> `pull.rebase` or `branch.<name>.rebase` for the current branch at
> all, the system acts as if you had `false` for these variables.  In
> other words, the default for these variables is `false`, meaning "do
> not rebase, just merge".  But the default option argument for the
> `--rebase` option given without argument would not be `false`, I
> would think.

I think there are two reasonable interpretations of the word "default"
here.

(1) "git pull": This is equivalent to "git pull --rebase=false". So one
could say that "git pull" defaults to "--rebase=false".

(2) "git pull --rebase": This is equivalent to "git pull --rebase=true".
So one could say that the "--rebase" flag (when it is used as "--rebase"
without the "=..." part) defaults to "--rebase=true".

I assume Dragan was thinking of only the meaning in (1). The alternate
meaning of (2) which you brought up makes sense too.

I think both flavors of "default" are reasonable interpretations because
most (if not all) Git users will start out with (1) in mind as they get
familiar with how "git pull" works without any flags (i.e., the
"default" git-pull behavior). Eventually they'll learn about (2) and
realize how the <kind> defaults to "true" with the "--rebase" flag.

Aside: interestingly, there appears to be a "--no-rebase" option that
means "--rebase=false" (see cd67e4d46b (Teach 'git pull' about --rebase,
2007-11-28)):

       --no-rebase
           This is shorthand for --rebase=false.

>> Signed-off-by: Dragan Simic <dsimic@manjaro.org>
>> ---
>>  Documentation/git-pull.txt | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt
>> index 0e14f8b5b2..d28790388e 100644
>> --- a/Documentation/git-pull.txt
>> +++ b/Documentation/git-pull.txt
>> @@ -116,7 +116,8 @@ When set to `merges`, rebase using `git rebase --rebase-merges` so that
>>  the local merge commits are included in the rebase (see
>>  linkgit:git-rebase[1] for details).
>>  +
>> -When false, merge the upstream branch into the current branch.
>> +When false, merge the upstream branch into the current branch. This is
>> +the default.
>>  +
>>  When `interactive`, enable the interactive mode of rebase.
>>  +

For reference here is a more complete view of the docs for this flag:

    -r::
    --rebase[=false|true|merges|interactive]::
        When true, rebase the current branch on top of the upstream
        branch after fetching. If there is a remote-tracking branch
        corresponding to the upstream branch and the upstream branch
        was rebased since last fetched, the rebase uses that information
        to avoid rebasing non-local changes.
    +
    When set to `merges`, rebase using `git rebase --rebase-merges` so that
    the local merge commits are included in the rebase (see
    linkgit:git-rebase[1] for details).
    +
    When false, merge the upstream branch into the current branch.
    +
    When `interactive`, enable the interactive mode of rebase.
    +
    See `pull.rebase`, `branch.<name>.rebase` and `branch.autoSetupRebase` in
    linkgit:git-config[1] if you want to make `git pull` always use
    `--rebase` instead of merging.

How about adding something like this instead as the very first paragraph
for this flag?

    Supplying "--rebase" defaults to "--rebase=true". Running git-pull
    without arguments implies "--rebase=false", unless relevant
    configuration variables have been set otherwise.

This way, we can discuss what "true" and "false" mean first to get these
two flavors of "defaults" sorted out (perhaps also mentioning
--no-rebase while we're at it). Then we can discuss the other <kind>
values like "merges" and "interactive".

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

[Dragan Simic]

On 2023-09-15 01:57, Linus Arver wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>> Dragan Simic <dsimic@manjaro.org> writes:
>> 
>>> Mention in the documentation that --rebase defaults to false.
>> 
>> I am not quite sure if this is an improvement, though.
>> 
>> It is true that, if you do not have any of your own funny
>> configuration, your "git pull" will *not* rebase.
>> 
>> But that is "if you do not give the --rebase option, you do not
>> trigger the rebase action".  Which is quite natural, but it is
>> different from what you wrote above, isn't it?
>> 
>> When people say "the default for `--rebase` is false", they mean
>> this:
>> 
>>     I can say "git pull --rebase=<kind>" to specify how my current
>>     branch is rebased on top of the upstream.  But if I do not
>>     specify the <kind>, i.e. "git pull --rebase", the command will
>>     act as if I gave 'false' as the <kind>.
>> 
>> At least, I would think that is what the word "default" means.
>> 
>> And I would be surprised if the "default" in that sense is 'false';
>> isn't the default <kind> 'true' --- meaning "perform the plain
>> vanilla 'git rebase'", unless you explicitly asked for 'merges',
>> 'interactive' or 'false'?
>> 
>> After the context of the hunk your patch touches, there is a
>> description on `pull.rebase`.  Down there, if you do not set
>> `pull.rebase` or `branch.<name>.rebase` for the current branch at
>> all, the system acts as if you had `false` for these variables.  In
>> other words, the default for these variables is `false`, meaning "do
>> not rebase, just merge".  But the default option argument for the
>> `--rebase` option given without argument would not be `false`, I
>> would think.
> 
> I think there are two reasonable interpretations of the word "default"
> here.
> 
> (1) "git pull": This is equivalent to "git pull --rebase=false". So one
> could say that "git pull" defaults to "--rebase=false".
> 
> (2) "git pull --rebase": This is equivalent to "git pull 
> --rebase=true".
> So one could say that the "--rebase" flag (when it is used as 
> "--rebase"
> without the "=..." part) defaults to "--rebase=true".
> 
> I assume Dragan was thinking of only the meaning in (1). The alternate
> meaning of (2) which you brought up makes sense too.
> 
> I think both flavors of "default" are reasonable interpretations 
> because
> most (if not all) Git users will start out with (1) in mind as they get
> familiar with how "git pull" works without any flags (i.e., the
> "default" git-pull behavior). Eventually they'll learn about (2) and
> realize how the <kind> defaults to "true" with the "--rebase" flag.
> 
> Aside: interestingly, there appears to be a "--no-rebase" option that
> means "--rebase=false" (see cd67e4d46b (Teach 'git pull' about 
> --rebase,
> 2007-11-28)):
> 
>        --no-rebase
>            This is shorthand for --rebase=false.
> 
> For reference here is a more complete view of the docs for this flag:
> 
>     -r::
>     --rebase[=false|true|merges|interactive]::
>         When true, rebase the current branch on top of the upstream
>         branch after fetching. If there is a remote-tracking branch
>         corresponding to the upstream branch and the upstream branch
>         was rebased since last fetched, the rebase uses that 
> information
>         to avoid rebasing non-local changes.
>     +
>     When set to `merges`, rebase using `git rebase --rebase-merges` so 
> that
>     the local merge commits are included in the rebase (see
>     linkgit:git-rebase[1] for details).
>     +
>     When false, merge the upstream branch into the current branch.
>     +
>     When `interactive`, enable the interactive mode of rebase.
>     +
>     See `pull.rebase`, `branch.<name>.rebase` and 
> `branch.autoSetupRebase` in
>     linkgit:git-config[1] if you want to make `git pull` always use
>     `--rebase` instead of merging.
> 
> How about adding something like this instead as the very first 
> paragraph
> for this flag?
> 
>     Supplying "--rebase" defaults to "--rebase=true". Running git-pull
>     without arguments implies "--rebase=false", unless relevant
>     configuration variables have been set otherwise.
> 
> This way, we can discuss what "true" and "false" mean first to get 
> these
> two flavors of "defaults" sorted out (perhaps also mentioning
> --no-rebase while we're at it). Then we can discuss the other <kind>
> values like "merges" and "interactive".

I'm really, really happy to see such a detailed approach to refining the 
git documentation!  Both Junio's and Linus's feedback are highly 
appreciated.

I totally agree about adding the above-proposed text as the opening 
paragraph for this option.  It's much better than just saying "this is 
the default", which is admittedly confusing and somewhat misleading.

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

[Junio C Hamano]

Linus Arver <linusa@google.com> writes:

> Aside: interestingly, there appears to be a "--no-rebase" option that
> means "--rebase=false" (see cd67e4d46b (Teach 'git pull' about --rebase,
> 2007-11-28)):
>
>        --no-rebase
>            This is shorthand for --rebase=false.
> ...
> How about adding something like this instead as the very first paragraph
> for this flag?
>
>     Supplying "--rebase" defaults to "--rebase=true". Running git-pull
>     without arguments implies "--rebase=false", unless relevant
>     configuration variables have been set otherwise.

Phrase nit.

	$ git pull origin

does run the command with arguments.  What you mean is "running
git-pull without any --rebase arguments implies --no-rebase", but
that is saying "not giving --rebase=<any> and not giving --rebase
means not rebasing", which makes my head spin.

"--no-rebase" as a command line option does have use to defeat
configured pull.rebase that is not set to "false", and allowing
"pull.rebase" to be set to "false" does have use to defeat settings
for the same variable made by lower-precedence configuration file.
"--rebase=false" does not have any reason to exist, except for
making the repertoire of "--rebase=<kind>" to be complete.

So, I am still not sure if saying "'git pull' (no other arguments
and no configuration) is equivalent to 'git pull --rebase=false'"
adds much value.

If --no-rebase and --rebase=false are explained in terms of why
these options that specify such an unnatural action (after all, you
say "do this" or "do it this way", but do not usually have to say
"do not do it that way") need to exist.

If I were writing this patch, I would rearrange the existing text
like so:

 * Update the description of "--no-rebase" *NOT* to depend on
   --rebase=false.  Instead move it higher and say

   - The default for "git pull" is to "merge" the other history into
     your history, but optionally you can "rebase" your history on
     top of the other history.

   - There are configuration variables (pull.rebase and
     branch.<name>.rebase) that trigger the optional behaviour, and
     when you set it, your "git pull" would "rebase".

   - The "--no-rebase" option is to defeat such configuration to
     tell the command to "merge" for this particular invocation.

 * Update the description of "--rebase=<kind>" and move the
   paragraph that begins with "When false" to the end, something
   like:

   - `--rebase` alone is equivalent to `--rebase=true`.
   - When set to 'merges'...
   - When set to 'interactive'...
   - See `pull.rebase`, ..., if you want to make `git pull` always
     rebase your history on top of theirs, instead of merging their
     history to yours.
   - `--rebase=false` is synonym to `--no-rebase`.

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

[Junio C Hamano]

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

> Linus Arver <linusa@google.com> writes:
>
>>     Supplying "--rebase" defaults to "--rebase=true". Running git-pull
>>     without arguments implies "--rebase=false", unless relevant
>>     configuration variables have been set otherwise.
>
> Phrase nit.
>
> 	$ git pull origin
>
> does run the command with arguments.  What you mean is "running
> git-pull without any --rebase arguments implies --no-rebase", but

A nit of my own: "--rebase arguments" -> "--rebase option"

Sorry for the noise ;-)

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

[Linus Arver]

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

> Linus Arver <linusa@google.com> writes:
>
>> Aside: interestingly, there appears to be a "--no-rebase" option that
>> means "--rebase=false" (see cd67e4d46b (Teach 'git pull' about --rebase,
>> 2007-11-28)):
>>
>>        --no-rebase
>>            This is shorthand for --rebase=false.
>> ...
>> How about adding something like this instead as the very first paragraph
>> for this flag?
>>
>>     Supplying "--rebase" defaults to "--rebase=true". Running git-pull
>>     without arguments implies "--rebase=false", unless relevant
>>     configuration variables have been set otherwise.
>
> Phrase nit.
>
> 	$ git pull origin
>
> does run the command with arguments.

Ah, good catch.

> What you mean is "running
> git-pull without any --rebase arguments implies --no-rebase",

Right (modulo your "--rebase arguments" -> "--rebase option" correction
in your follow-up email).

> but
> that is saying "not giving --rebase=<any> and not giving --rebase
> means not rebasing", which makes my head spin.

Me too.

> "--no-rebase" as a command line option does have use to defeat
> configured pull.rebase that is not set to "false"

Yes, I noticed this too after digging around a bit more after I sent my
message. Thanks for clarifying for the record.

> and allowing
> "pull.rebase" to be set to "false" does have use to defeat settings
> for the same variable made by lower-precedence configuration file.

Indeed! I did not think of this. IOW, Git can be configured in
multiple places (the "pull.rebase" setting in ~/.gitconfig can be
overridden by the same config in ~/myrepo/.git/config).

> "--rebase=false" does not have any reason to exist, except for
> making the repertoire of "--rebase=<kind>" to be complete.

Agreed. In a sense, the docs for "--rebase=false" should say that it is
a synonym for "--no-rebase" (because "--no-rebase" came first,
historically), and not the other way around (that "--no-rebase" is
shorthand for "--rebase=false").

> So, I am still not sure if saying "'git pull' (no other arguments
> and no configuration) is equivalent to 'git pull --rebase=false'"
> adds much value.

Fair point. That is, there are so many gotchas and "edge case"-like
behaviors to consider here, so the statement "'git pull' (no other arguments
and no configuration) is equivalent to 'git pull --rebase=false'" is an
oversimplification that can be misleading. I agree.

> If --no-rebase and --rebase=false are explained in terms of why
> these options that specify such an unnatural action (after all, you
> say "do this" or "do it this way", but do not usually have to say
> "do not do it that way") need to exist.
>
> If I were writing this patch, I would rearrange the existing text
> like so:
>
>  * Update the description of "--no-rebase" *NOT* to depend on
>    --rebase=false.  Instead move it higher and say
>
>    - The default for "git pull" is to "merge" the other history into
>      your history, but optionally you can "rebase" your history on
>      top of the other history.
>
>    - There are configuration variables (pull.rebase and
>      branch.<name>.rebase) that trigger the optional behaviour, and
>      when you set it, your "git pull" would "rebase".
>
>    - The "--no-rebase" option is to defeat such configuration to
>      tell the command to "merge" for this particular invocation.
>
>  * Update the description of "--rebase=<kind>" and move the
>    paragraph that begins with "When false" to the end, something
>    like:
>
>    - `--rebase` alone is equivalent to `--rebase=true`.
>    - When set to 'merges'...
>    - When set to 'interactive'...
>    - See `pull.rebase`, ..., if you want to make `git pull` always
>      rebase your history on top of theirs, instead of merging their
>      history to yours.
>    - `--rebase=false` is synonym to `--no-rebase`.

I think this captures the finer details while still preserving the
spirit of Dragan's original patch, so SGTM.

@Dragan if you are OK with doing the (much more substantial) change as
suggested, please do. Thanks!

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

[Dragan Simic]

On 2023-09-15 23:12, Linus Arver wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>> Linus Arver <linusa@google.com> writes:
>> 
>>> Aside: interestingly, there appears to be a "--no-rebase" option that
>>> means "--rebase=false" (see cd67e4d46b (Teach 'git pull' about 
>>> --rebase,
>>> 2007-11-28)):
>>> 
>>>        --no-rebase
>>>            This is shorthand for --rebase=false.
>>> ...
>>> How about adding something like this instead as the very first 
>>> paragraph
>>> for this flag?
>>> 
>>>     Supplying "--rebase" defaults to "--rebase=true". Running 
>>> git-pull
>>>     without arguments implies "--rebase=false", unless relevant
>>>     configuration variables have been set otherwise.
>> 
>> Phrase nit.
>> 
>> 	$ git pull origin
>> 
>> does run the command with arguments.
> 
> Ah, good catch.
> 
>> What you mean is "running
>> git-pull without any --rebase arguments implies --no-rebase",
> 
> Right (modulo your "--rebase arguments" -> "--rebase option" correction
> in your follow-up email).
> 
>> but
>> that is saying "not giving --rebase=<any> and not giving --rebase
>> means not rebasing", which makes my head spin.
> 
> Me too.
> 
>> "--no-rebase" as a command line option does have use to defeat
>> configured pull.rebase that is not set to "false"
> 
> Yes, I noticed this too after digging around a bit more after I sent my
> message. Thanks for clarifying for the record.
> 
>> and allowing
>> "pull.rebase" to be set to "false" does have use to defeat settings
>> for the same variable made by lower-precedence configuration file.
> 
> Indeed! I did not think of this. IOW, Git can be configured in
> multiple places (the "pull.rebase" setting in ~/.gitconfig can be
> overridden by the same config in ~/myrepo/.git/config).
> 
>> "--rebase=false" does not have any reason to exist, except for
>> making the repertoire of "--rebase=<kind>" to be complete.
> 
> Agreed. In a sense, the docs for "--rebase=false" should say that it is
> a synonym for "--no-rebase" (because "--no-rebase" came first,
> historically), and not the other way around (that "--no-rebase" is
> shorthand for "--rebase=false").
> 
>> So, I am still not sure if saying "'git pull' (no other arguments
>> and no configuration) is equivalent to 'git pull --rebase=false'"
>> adds much value.
> 
> Fair point. That is, there are so many gotchas and "edge case"-like
> behaviors to consider here, so the statement "'git pull' (no other 
> arguments
> and no configuration) is equivalent to 'git pull --rebase=false'" is an
> oversimplification that can be misleading. I agree.
> 
>> If --no-rebase and --rebase=false are explained in terms of why
>> these options that specify such an unnatural action (after all, you
>> say "do this" or "do it this way", but do not usually have to say
>> "do not do it that way") need to exist.
>> 
>> If I were writing this patch, I would rearrange the existing text
>> like so:
>> 
>>  * Update the description of "--no-rebase" *NOT* to depend on
>>    --rebase=false.  Instead move it higher and say
>> 
>>    - The default for "git pull" is to "merge" the other history into
>>      your history, but optionally you can "rebase" your history on
>>      top of the other history.
>> 
>>    - There are configuration variables (pull.rebase and
>>      branch.<name>.rebase) that trigger the optional behaviour, and
>>      when you set it, your "git pull" would "rebase".
>> 
>>    - The "--no-rebase" option is to defeat such configuration to
>>      tell the command to "merge" for this particular invocation.
>> 
>>  * Update the description of "--rebase=<kind>" and move the
>>    paragraph that begins with "When false" to the end, something
>>    like:
>> 
>>    - `--rebase` alone is equivalent to `--rebase=true`.
>>    - When set to 'merges'...
>>    - When set to 'interactive'...
>>    - See `pull.rebase`, ..., if you want to make `git pull` always
>>      rebase your history on top of theirs, instead of merging their
>>      history to yours.
>>    - `--rebase=false` is synonym to `--no-rebase`.
> 
> I think this captures the finer details while still preserving the
> spirit of Dragan's original patch, so SGTM.
> 
> @Dragan if you are OK with doing the (much more substantial) change as
> suggested, please do. Thanks!

Sure, thanks.  I'll put together v2 of the patch in the next couple of 
days, taking into account all the fine details of the awesome feedback 
provided by both of you, so we can have it discussed and refined 
further.