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

From: Linus Arver <linusa@google.com>
Date: Sep 15, 2023, 21:12 UTC
Message-ID: <owlya5tnjg4f.fsf@fine.c.googlers.com>
In-Reply-To: <xmqqh6nvfi2p.fsf@gitster.g>

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

Show 21 quoted lines

> 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.

Show 33 quoted lines

> 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!