Re: [PATCH v2 02/14] pull: improve default warning

From: Elijah Newren <newren@gmail.com>
To: Felipe Contreras <felipe.contreras@gmail.com>
Cc: "Git Mailing List" <git@vger.kernel.org>,
	"Vít Ondruch" <vondruch@redhat.com>,
	"Alex Henrie" <alexhenrie24@gmail.com>,
	"Junio C Hamano" <gitster@pobox.com>, "Jeff King" <peff@peff.net>,
	"Philip Oakley" <philipoakley@iee.email>,
	"Theodore Y . Ts'o" <tytso@mit.edu>,
	"Jacob Keller" <jacob.keller@gmail.com>,
	"John Keeping" <john@keeping.me.uk>,
	"Richard Hansen" <rhansen@rhansen.org>,
	"Brian M. Carlson" <sandals@crustytoothpaste.net>,
	"W. Trevor King" <wking@tremily.us>
Subject: Re: [PATCH v2 02/14] pull: improve default warning
Date: Fri, 4 Dec 2020 14:59:53 -0800	[thread overview]
Message-ID: <CABPp-BFdjj=+4jk0vo=kpNc6ug1=UgtKfXJZkseyyxut2VB=Uw@mail.gmail.com> (raw)
In-Reply-To: <20201204061623.1170745-3-felipe.contreras@gmail.com>

On Thu, Dec 3, 2020 at 10:16 PM Felipe Contreras
<felipe.contreras@gmail.com> wrote:
>
> We want to:
>
> 1. Be clear about what "specifying" means; merge or rebase.
> 2. Mention a direct shortcut for people that just want to get on with
>    their lives: git pull --no-rebase.

This is a shortcut for what?

> 3. Have a quick reference for users to understand what this
>    "fast-forward" business means.
>
> This patch does all three.
>
> Thanks to the previous patch now "git pull --help" explains what a
> fast-forward is, and a further patch changes --no-rebase to --merge so
> it's actually user friendly.
>
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  builtin/pull.c | 23 ++++++++++++-----------
>  1 file changed, 12 insertions(+), 11 deletions(-)
>
> diff --git a/builtin/pull.c b/builtin/pull.c
> index 1034372f8b..22a9ffcade 100644
> --- a/builtin/pull.c
> +++ b/builtin/pull.c
> @@ -346,17 +346,18 @@ static enum rebase_type config_get_rebase(void)
>
>         if (opt_verbosity >= 0 && !opt_ff) {
>                 advise(_("Pulling without specifying how to reconcile divergent branches is\n"
> -                        "discouraged. You can squelch this message by running one of the following\n"
> -                        "commands sometime before your next pull:\n"
> -                        "\n"
> -                        "  git config pull.rebase false  # merge (the default strategy)\n"
> -                        "  git config pull.rebase true   # 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"));
> +                       "discouraged; you need to specify if you want a merge, or a rebase.\n"

...want a merge, a rebase, or neither.

> +                       "You can squelch this message by running one of the following commands:\n"
> +                       "\n"
> +                       "  git config pull.rebase false  # merge (the default strategy)\n"

Should this be labelled as the default given the desire to make
--ff-only the default?  Perhaps I'm jumping ahead and you plan to
change that later in this series.

> +                       "  git config pull.rebase true   # 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.\n"

Good up to here.

> +                       "If unsure, run \"git pull --no-rebase\".\n"

Why is that safe to suggest?  The original text may not have been the
easiest to parse, but this seems more problematic to me.

> +                       "Read \"git pull --help\" for more information."

Nice addition.

> +                       ));
>         }
>
>         return REBASE_FALSE;
> --
> 2.29.2
>