Re: [PATCH] pull: conflict hint pull.rebase suggestion should offer "merges" vs "true"

From: Alex Henrie <alexhenrie24@gmail.com>
Date: Feb 16, 2023, 03:22 UTC
Message-ID: <CAMMLpeTPEoKVTbfc17w+Y9qn7jOGmQi_Ux0Y3sFW5QTgGWJ=SA@mail.gmail.com>
In-Reply-To: <pull.1474.git.1675614276549.gitgitgadget@gmail.com>

On Sun, Feb 5, 2023 at 9:41 AM Tao Klerks via GitGitGadget <gitgitgadget@gmail.com> wrote:

Show 58 quoted lines

>
> From: Tao Klerks <tao@klerks.biz>
>
> When "git pull" is called without a conflict-handling instruction or
> configuration, it displays a hint proposing "pull.rebase" and "pull.ff"
> config options for future handling.
>
> The hint offers three permanent settings, "merge", rebase", and "ff". The
> proposed command for "rebase" is "git config pull.rebase true".
>
> Unfortunately, this rebase configuration can easily lead to non-expert users
> accidentally rebasing not their own commits, instead others' commits, if the
> new commits they have locally before the "pull" include a merge of another
> branch, eg "main".
>
> Since 2018 in git version "2.18", it has supported a new rebase flag
> "--rebase-merges", with corresponding pull.rebase config option "merges".
> This new option is ideal for rebasing local work on "pull", as it will
> not "mangle"/flatten any local merge commits but rather recreate them.
>
> Change the pull conflict hint text to propose "pull.rebase merges" instead
> of "pull.rebase true", and "git pull --rebase=merges" instead of
> "git pull --rebase".
>
> Signed-off-by: Tao Klerks <tao@klerks.biz>
> ---
>     pull: conflict hint pull.rebase suggestion should offer "merges" vs
>     "true"
>
>     Hint change as proposed in
>     https://lore.kernel.org/git/xmqqa61uo3q0.fsf@gitster.g/
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1474%2FTaoK%2Ftao-fetch-rebase-hint-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1474/TaoK/tao-fetch-rebase-hint-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/1474
>
>  builtin/pull.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/builtin/pull.c b/builtin/pull.c
> index 1ab4de0005d..535364fbb07 100644
> --- a/builtin/pull.c
> +++ b/builtin/pull.c
> @@ -967,13 +967,13 @@ static void show_advice_pull_non_ff(void)
>                  "your next pull:\n"
>                  "\n"
>                  "  git config pull.rebase false  # merge\n"
> -                "  git config pull.rebase true   # rebase\n"
> +                "  git config pull.rebase merges # rebase\n"
>                  "  git config pull.ff only       # fast-forward only\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"));
> +                "preference for all repositories. You can also pass --rebase=merges,\n"
> +                "--no-rebase, or --ff-only on the command line to override the configured\n"
> +                "default per invocation.\n"));

Hi Tao, thank you for sharing your experiences with non-experts using `git pull`. I am always curious to see how people who are learning Git react to it, and I am very interested in making Git as straightforward as possible.

I'm afraid I have several objections to this patch...

- The proposed wording is likely to further confuse novices. It's
asking the user to choose between the reconciliation strategies of
merging and rebasing, but then says to use the unintuitive combination
"rebase=merges" which sounds like it's going to make a merge commit at
the end of the branch anyway.

- The proposed wording makes it sound like there's something wrong
with doing a regular rebase, but that's not usually the case because
in practice a regular rebase is almost always equivalent to
rebase=merges. A regular rebase may even be what the user really
wants: For example, the user might choose to merge when pulling and
then change their mind and decide that they really wanted to rebase.
Repeating the pull with the regular -r or --rebase flag fixes the
mistake.

- `git pull -ri` (or its longer form `git pull --rebase=interactive`)
is generally more useful than `git pull --rebase=merges`, but once
rebase=merges has been specified, there's no way to specify
rebase=interactive also. Recommending rebase=merges steers people away
from rebase=interactive, hiding useful functionality from the user.

Now, this is not to say that there's no room for improvement. I like the rebase=merges option and I wish everyone knew about it because there are situations where it really is the best option. I suggest leaving the existing text alone, but adding an additional paragraph, something like:

Note that --rebase or pull.rebase=true will drop existing merge commits and rebase all of the commits from all of the merged branches. If you want to rebase but preserve existing merge commits, use --rebase=merges or pull.rebase=merges instead.

-Alex