diff mbox series

[v2,02/14] pull: improve default warning

Message ID 20201204061623.1170745-3-felipe.contreras@gmail.com (mailing list archive)
State New, archived
Headers show
Series pull: default warning improvements | expand

Commit Message

Felipe Contreras Dec. 4, 2020, 6:16 a.m. UTC
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.
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(-)

Comments

Elijah Newren Dec. 4, 2020, 10:59 p.m. UTC | #1
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
>
Felipe Contreras Dec. 5, 2020, 12:12 a.m. UTC | #2
On Fri, Dec 4, 2020 at 5:00 PM Elijah Newren <newren@gmail.com> wrote:
>
> 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?

  git config --global pull.rebase false
  git pull

It's a shorter way of saying: "do a 'git pull' like you've always done
but don't warn me".

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

There is no "git pull --no-merge". Years ago some people argued for a
"pull.mode=none" (essentially making "git pull" the same as "git
fetch"). But right now there's no option to do that.

There's an option to do --ff-only, but that's still a merge.

Perhaps: a merge, a rebase, or a fast-forward?

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

That's right.

In the previous series which does indeed make "pull.mode=ff-only" the
default [1], I do change the warning to specify the future default
[2], but in that series the warnings is changed to:

  The pull was not fast-forward, in the future you will have to choose
a merge, or a rebase.
  To squelch this message and maintain the current behavior, use:

    git config --global pull.mode merge

  To squelch this message and adopt the new behavior now, use:

    git config --global push.mode ff-only

  Falling back to old style for now (merge).
  Read "git pull --help" for more information.

Since that series didn't get any traction, I decided to only implement
step 1: fix the current situation. And later on another series would
do step 2: introduce "pull.mode=ff-only" and do the preparations to
make it the default.

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

Because "git pull" has been doing the same as "git pull --no-rebase"
for more than a decade. It's safe for people to continue with this
behavior for a few more months.

Some people need to get things done today, and they are not interested
in future changes, nor changing their default configuration, or what
the warning has to say.

They just want "git pull" to do the same as yesterday, and the year
before, without being bothered with an annoying warning.

Those people can start training their fingers to do "git pull
--merge", and learn the problems with "git pull" later.

We want to respect the user's time, and not force them to read the
warning today.

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

Especially since now it explains what a fast-forward is.
Elijah Newren Dec. 5, 2020, 12:56 a.m. UTC | #3
Hi Felipe,

On Fri, Dec 4, 2020 at 4:12 PM Felipe Contreras
<felipe.contreras@gmail.com> wrote:
>
> On Fri, Dec 4, 2020 at 5:00 PM Elijah Newren <newren@gmail.com> wrote:
> >
> > 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?
>
>   git config --global pull.rebase false
>   git pull
>
> It's a shorter way of saying: "do a 'git pull' like you've always done
> but don't warn me".
>
> > > 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.
>
> There is no "git pull --no-merge". Years ago some people argued for a
> "pull.mode=none" (essentially making "git pull" the same as "git
> fetch"). But right now there's no option to do that.
>
> There's an option to do --ff-only, but that's still a merge.

I disagree.  I'm well aware that checkout_fast_forward() (which is
what is ultimately called to do the fast-forwarding) is in a file
called merge.c, but that doesn't make it a merge.  I don't believe it
was anything more than a convenient place to dump some extra code at
the time.

> Perhaps: a merge, a rebase, or a fast-forward?

Sure, that works; in fact, that's much better than my suggestion.  I like it.

> > > +                       "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.
>
> That's right.
>
> In the previous series which does indeed make "pull.mode=ff-only" the
> default [1], I do change the warning to specify the future default
> [2], but in that series the warnings is changed to:
>
>   The pull was not fast-forward, in the future you will have to choose
> a merge, or a rebase.
>   To squelch this message and maintain the current behavior, use:
>
>     git config --global pull.mode merge
>
>   To squelch this message and adopt the new behavior now, use:
>
>     git config --global push.mode ff-only
>
>   Falling back to old style for now (merge).
>   Read "git pull --help" for more information.
>
> Since that series didn't get any traction, I decided to only implement
> step 1: fix the current situation. And later on another series would
> do step 2: introduce "pull.mode=ff-only" and do the preparations to
> make it the default.

I like this longer plan.  However, on the shorter scale plan...I think
the suggestion to use "git pull --no-rebase" makes the current
situation worse rather than fixing it.

> > > +                       "  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.
>
> Because "git pull" has been doing the same as "git pull --no-rebase"
> for more than a decade. It's safe for people to continue with this
> behavior for a few more months.
>
> Some people need to get things done today, and they are not interested
> in future changes, nor changing their default configuration, or what
> the warning has to say.
>
> They just want "git pull" to do the same as yesterday, and the year
> before, without being bothered with an annoying warning.
>
> Those people can start training their fingers to do "git pull
> --merge", and learn the problems with "git pull" later.
>
> We want to respect the user's time, and not force them to read the
> warning today.

The warning was added because sending users down paths that break
things badly is a waste of user's time, and often a much bigger waste
of user's time than making them read up on the meaning behind the two
different choices of what kind of changes they can make.  I agree the
warning went too far, but I fully agree with the original folks who
put this warning here that we need to have it for at least some cases
and that there is a decision to be made.  Though I am just one voice,
and perhaps others will agree with you on your point here, I'll
continue to disagree with blindly suggesting "git pull --no-rebase".

> > > +                       "Read \"git pull --help\" for more information."
> >
> > Nice addition.
>
> Especially since now it explains what a fast-forward is.

Indeed.  :-)
Felipe Contreras Dec. 5, 2020, 1:56 a.m. UTC | #4
On Fri, Dec 4, 2020 at 6:56 PM Elijah Newren <newren@gmail.com> wrote:
>
> Hi Felipe,
>
> On Fri, Dec 4, 2020 at 4:12 PM Felipe Contreras
> <felipe.contreras@gmail.com> wrote:
> >
> > On Fri, Dec 4, 2020 at 5:00 PM Elijah Newren <newren@gmail.com> wrote:
> > >
> > > 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?
> >
> >   git config --global pull.rebase false
> >   git pull
> >
> > It's a shorter way of saying: "do a 'git pull' like you've always done
> > but don't warn me".
> >
> > > > 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.
> >
> > There is no "git pull --no-merge". Years ago some people argued for a
> > "pull.mode=none" (essentially making "git pull" the same as "git
> > fetch"). But right now there's no option to do that.
> >
> > There's an option to do --ff-only, but that's still a merge.
>
> I disagree.  I'm well aware that checkout_fast_forward() (which is
> what is ultimately called to do the fast-forwarding) is in a file
> called merge.c, but that doesn't make it a merge.  I don't believe it
> was anything more than a convenient place to dump some extra code at
> the time.

Right. Maybe my mind is tainted by doing so many fast-forward "git
merge"s to update the current branch.

But in my mind "fast-forward" is not a noun, it's an adjective, so I
still expect a noun: fast-forward $what. And I don't have any other
noun to put there but "merge".

> > Perhaps: a merge, a rebase, or a fast-forward?
>
> Sure, that works; in fact, that's much better than my suggestion.  I like it.

Great.

> > > > +                       "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.
> >
> > That's right.
> >
> > In the previous series which does indeed make "pull.mode=ff-only" the
> > default [1], I do change the warning to specify the future default
> > [2], but in that series the warnings is changed to:
> >
> >   The pull was not fast-forward, in the future you will have to choose
> > a merge, or a rebase.
> >   To squelch this message and maintain the current behavior, use:
> >
> >     git config --global pull.mode merge
> >
> >   To squelch this message and adopt the new behavior now, use:
> >
> >     git config --global push.mode ff-only
> >
> >   Falling back to old style for now (merge).
> >   Read "git pull --help" for more information.
> >
> > Since that series didn't get any traction, I decided to only implement
> > step 1: fix the current situation. And later on another series would
> > do step 2: introduce "pull.mode=ff-only" and do the preparations to
> > make it the default.
>
> I like this longer plan.

Yeah. It's a better plan, just more work for me.

> However, on the shorter scale plan...I think
> the suggestion to use "git pull --no-rebase" makes the current
> situation worse rather than fixing it.

Well, I already said I partly agree with you: in the --ff-only case
the suggestion should not be brought forward.

But in the "git pull" default case, *today* it's doing a merge. If
uttering --merge and thus making the current behavior explicit instead
of implicit seems dangerous it's because it is. But not documenting it
doesn't make it any less dangerous.

> > > > +                       "  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.
> >
> > Because "git pull" has been doing the same as "git pull --no-rebase"
> > for more than a decade. It's safe for people to continue with this
> > behavior for a few more months.
> >
> > Some people need to get things done today, and they are not interested
> > in future changes, nor changing their default configuration, or what
> > the warning has to say.
> >
> > They just want "git pull" to do the same as yesterday, and the year
> > before, without being bothered with an annoying warning.
> >
> > Those people can start training their fingers to do "git pull
> > --merge", and learn the problems with "git pull" later.
> >
> > We want to respect the user's time, and not force them to read the
> > warning today.
>
> The warning was added because sending users down paths that break
> things badly is a waste of user's time, and often a much bigger waste
> of user's time than making them read up on the meaning behind the two
> different choices of what kind of changes they can make.  I agree the
> warning went too far, but I fully agree with the original folks who
> put this warning here that we need to have it for at least some cases
> and that there is a decision to be made.  Though I am just one voice,
> and perhaps others will agree with you on your point here, I'll
> continue to disagree with blindly suggesting "git pull --no-rebase".

I think the warning is good. I implemented something like that years
ago, and I believe Junio did so too. It should have been implemented
differently though.

And yes, we want to annoy the users, and nudge them in the right
direction, but still leave them the *option* to easily ignore us.

I'm against forcing anyone to do anything. If the user wants to
analyze the warning, they can do so, if not, they can just do $cmd and
go about their day as usual. But they were warned.

Cheers.
Chris Torek Dec. 5, 2020, 9:23 a.m. UTC | #5
On Fri, Dec 4, 2020 at 5:59 PM Felipe Contreras
<felipe.contreras@gmail.com> wrote:
> But in my mind "fast-forward" is not a noun, it's an adjective, so I
> still expect a noun: fast-forward $what. And I don't have any other
> noun to put there but "merge".

It's a "fast-forward operation". :-)

(The operands are branch name and hash ID.)

It's just been nouned, like "a merge" instead of "a merge commit".

> > > Perhaps: a merge, a rebase, or a fast-forward?
> >
> > Sure, that works; in fact, that's much better than my suggestion.  I like it.

I like this one too.

In a perfect world, I'd have had `git pull` be the user facing command
that does one of: fetch only, fetch-and-fast-forward, fetch-and-rebase,
or fetch-and-merge.  (Obviously one can achieve the fetch-only by
running `git fetch`, but `git fetch` is a plumbing command.)

In that same perfect world, the default probably would have been
fetch only, but fetch-and-fast-forward (or fail if not fast-forward-able)
seems like an OK default as well. But we have to deal with the
imperfect world we have. This seems like an OK way to do that.
Felipe Contreras Dec. 5, 2020, 11:47 a.m. UTC | #6
On Sat, Dec 5, 2020 at 3:23 AM Chris Torek <chris.torek@gmail.com> wrote:
>
> On Fri, Dec 4, 2020 at 5:59 PM Felipe Contreras
> <felipe.contreras@gmail.com> wrote:
> > But in my mind "fast-forward" is not a noun, it's an adjective, so I
> > still expect a noun: fast-forward $what. And I don't have any other
> > noun to put there but "merge".
>
> It's a "fast-forward operation". :-)

A fast-forward operation that does what?

> It's just been nouned, like "a merge" instead of "a merge commit".

Except there's a difference between telling git: "do a fast-forward
merge", and "do a fast-forward merge commit". The latter doesn't
really make sense.

> > > > Perhaps: a merge, a rebase, or a fast-forward?
> > >
> > > Sure, that works; in fact, that's much better than my suggestion.  I like it.
>
> I like this one too.
>
> In a perfect world, I'd have had `git pull` be the user facing command
> that does one of: fetch only, fetch-and-fast-forward, fetch-and-rebase,
> or fetch-and-merge.  (Obviously one can achieve the fetch-only by
> running `git fetch`, but `git fetch` is a plumbing command.)
>
> In that same perfect world, the default probably would have been
> fetch only, but fetch-and-fast-forward (or fail if not fast-forward-able)
> seems like an OK default as well. But we have to deal with the
> imperfect world we have. This seems like an OK way to do that.

In the slightly more perfect word I'm aiming for those two modes could
be achieved with the pull.mode configuration:

* "pull.mode=none" would be the same as "fetch only"
* "pull.mode=ff-only" would be the same as "fetch-and-fast-forward"

But first this configuration has to actually be introduced. It didn't
happen in 2014, maybe it will happen this time. Who knows.

Cheers.
Elijah Newren Dec. 5, 2020, 4:28 p.m. UTC | #7
On Fri, Dec 4, 2020 at 5:56 PM Felipe Contreras
<felipe.contreras@gmail.com> wrote:
>
> On Fri, Dec 4, 2020 at 6:56 PM Elijah Newren <newren@gmail.com> wrote:
> >
> > Hi Felipe,
> >
> > On Fri, Dec 4, 2020 at 4:12 PM Felipe Contreras
> > <felipe.contreras@gmail.com> wrote:
> > >
> > > On Fri, Dec 4, 2020 at 5:00 PM Elijah Newren <newren@gmail.com> wrote:
> > > >
> > > > On Thu, Dec 3, 2020 at 10:16 PM Felipe Contreras
> > > > <felipe.contreras@gmail.com> wrote:
[...]
> > > > > +                       "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.
> > >
> > > That's right.
> > >
> > > In the previous series which does indeed make "pull.mode=ff-only" the
> > > default [1], I do change the warning to specify the future default
> > > [2], but in that series the warnings is changed to:
> > >
> > >   The pull was not fast-forward, in the future you will have to choose
> > > a merge, or a rebase.
> > >   To squelch this message and maintain the current behavior, use:
> > >
> > >     git config --global pull.mode merge
> > >
> > >   To squelch this message and adopt the new behavior now, use:
> > >
> > >     git config --global push.mode ff-only
> > >
> > >   Falling back to old style for now (merge).
> > >   Read "git pull --help" for more information.
> > >
> > > Since that series didn't get any traction, I decided to only implement
> > > step 1: fix the current situation. And later on another series would
> > > do step 2: introduce "pull.mode=ff-only" and do the preparations to
> > > make it the default.
> >
> > I like this longer plan.
>
> Yeah. It's a better plan, just more work for me.
>
> > However, on the shorter scale plan...I think
> > the suggestion to use "git pull --no-rebase" makes the current
> > situation worse rather than fixing it.
>
> Well, I already said I partly agree with you: in the --ff-only case
> the suggestion should not be brought forward.
>
> But in the "git pull" default case, *today* it's doing a merge. If
> uttering --merge and thus making the current behavior explicit instead
> of implicit seems dangerous it's because it is. But not documenting it
> doesn't make it any less dangerous.

Sounds like we agree that the future should be ff-only-as-default.  I
also agree with you that the primary problem is the current default
behavior, and I'll agree with you that documenting the current default
is okay.  However, I disagree that your wording here:

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

does anything of the sort.  It does not mention that this is the
default behavior the users would get if they provided no flags.  More
importantly, it makes a recommendation...and one that undercuts the
point of the message.  It makes it feel like the message shouldn't
exist at all in any circumstances.  I even suspect that adding that
sentence may undercut any efforts towards changing the default to
ff-only-as-default.  While I'm a big fan of most of what you've done
in this series, I will object to its merging for as long as this
stays.  (I definitely don't have veto power or anything close to it,
just stating what my opinion is.)
Felipe Contreras Dec. 5, 2020, 9:27 p.m. UTC | #8
On Sat, Dec 5, 2020 at 10:28 AM Elijah Newren <newren@gmail.com> wrote:
> On Fri, Dec 4, 2020 at 5:56 PM Felipe Contreras
> <felipe.contreras@gmail.com> wrote:

> > Well, I already said I partly agree with you: in the --ff-only case
> > the suggestion should not be brought forward.
> >
> > But in the "git pull" default case, *today* it's doing a merge. If
> > uttering --merge and thus making the current behavior explicit instead
> > of implicit seems dangerous it's because it is. But not documenting it
> > doesn't make it any less dangerous.
>
> Sounds like we agree that the future should be ff-only-as-default.  I
> also agree with you that the primary problem is the current default
> behavior, and I'll agree with you that documenting the current default
> is okay.  However, I disagree that your wording here:
>
> +                       "If unsure, run \"git pull --no-rebase\".\n"
>
> does anything of the sort.  It does not mention that this is the
> default behavior the users would get if they provided no flags.

But that is not the warning, this is the warning:

  Pulling without specifying how to reconcile divergent branches is discouraged;
  you need to specify if you want a merge, a rebase, or a fast-forward.
  You can squelch this message by running one of the following commands:

    git config pull.rebase false  # merge (the default strategy)
    git config pull.rebase true   # rebase
    git config pull.ff only       # fast-forward only

  You can replace "git config" with "git config --global" to set a default
  preference for all repositories.
  If unsure, run "git pull --merge".
  Read "git pull --help" for more information.

This warning says:

1. There's 3 options: merge, rebase, fast-forward
2. merge is the default strategy
3. If unsure, specify --merge (the default strategy)

So taken altogether it does say what is the default strategy.

> More
> importantly, it makes a recommendation...and one that undercuts the
> point of the message.

So?

When boarding a plane the flight attendants do a safety demonstration
that passengers should pay attention to. If one passenger is not
paying attention (listening to music on headphones, and reading a
book) what should the crew do?

1. Remove the passenger's headphones and force him to pay attention to
the safety demonstration
2. Let the passenger ignore the safety demonstration

Human beings are independent agents responsible for their own actions.
You as a separate human being--a crew member--can argue that it's not
in the best interest of the passenger to ignore the safety
demonstration, and you may be right, but the passenger decisions are
still the passenger's decisions, even if they are bad.

Do you think the crew should disregard the passenger's volition and
force him to pay attention to the safety demonstration?

> It makes it feel like the message shouldn't
> exist at all in any circumstances.  I even suspect that adding that
> sentence may undercut any efforts towards changing the default to
> ff-only-as-default.  While I'm a big fan of most of what you've done
> in this series, I will object to its merging for as long as this
> stays.  (I definitely don't have veto power or anything close to it,
> just stating what my opinion is.)

The current warning should not exist at all.

The complaint from Vít Ondruch [1] that reignited this series is a
valid one. A *permanent* warning is not good. We should have a
*temporary* warning with the express purpose of notifying users of an
upcoming change.

If we have not yet decided on what should be the default (Junio seems
to have casted some doubt on the consensus [2]), and we don't have a
clear path forward to implement such change (we can't even tell users
to use "pull.ff=only", since eventually it may be
"pull.mode=ff-only"), then we must remove the warning.

It was a mistake to put a *permanent* warning before deciding to
change the default.

So, there's two options:

1. We decide on a path forward and fix the warning so it *temporarily*
explains what will happen in the future
2. We remove the *permanent* warning

Since we are already here, we might as well take advantage of that
warning and repurpose it. But in the meantime--while the git project
decides what to do, and what configurations to suggest the users to
change--we should at the very least waste as little as the user's time
as possible, and give him/her a quick opt-out.

Yes, a quick opt-out defeats the purpose of a warning, but we must
respect the users' volition. The user may be on a deadline trying to
push some changes to production before the weekend, and after a system
update be annoyed with this warning on every pull. The user may not
have time to look at the warning, decide he wants to read the warning
in the future, maybe next Monday, and thus not configure anything to
silence it.

What's wrong with a user saying "I don't have time for this now,
please tell me what to do for now, I'll look at the warning later"? If
anything for those users the configuration is the wrong thing to do,
because being in a hurry they just choose the first configuration and
forget about the warning without actually looking at it (because they
didn't have time), and it will not appear any more. By typing "git
pull --merge" the user can get rid of the warning *for now*, but the
next time he does "git pull" the warning will reappear, and at that
time perhaps the user does have the time to read it, and look at the
manpage.

Nobody likes their workflow to be interrupted and be forced to do anything.

I don't think my patches plus that suggestion for a quick opt-out are
in any way worse than the current situation. If you think they are,
then we'll just have to agree to disagree.

I quote the voice of Vít Ondruch, which I think represents the typical
user: "please select any strategy considered more appropriate and stop
warning me".

Cheers.

[1] https://lore.kernel.org/git/742df4c2-2bc5-8a4b-8de1-cd5e48718398@redhat.com/
[2] https://lore.kernel.org/git/xmqqh7p1fjml.fsf@gitster.c.googlers.com/
Elijah Newren Dec. 6, 2020, 1:01 a.m. UTC | #9
On Sat, Dec 5, 2020 at 1:28 PM Felipe Contreras
<felipe.contreras@gmail.com> wrote:
>
> On Sat, Dec 5, 2020 at 10:28 AM Elijah Newren <newren@gmail.com> wrote:
> > On Fri, Dec 4, 2020 at 5:56 PM Felipe Contreras
> > <felipe.contreras@gmail.com> wrote:
>
> > > Well, I already said I partly agree with you: in the --ff-only case
> > > the suggestion should not be brought forward.
> > >
> > > But in the "git pull" default case, *today* it's doing a merge. If
> > > uttering --merge and thus making the current behavior explicit instead
> > > of implicit seems dangerous it's because it is. But not documenting it
> > > doesn't make it any less dangerous.
> >
> > Sounds like we agree that the future should be ff-only-as-default.  I
> > also agree with you that the primary problem is the current default
> > behavior, and I'll agree with you that documenting the current default
> > is okay.  However, I disagree that your wording here:
> >
> > +                       "If unsure, run \"git pull --no-rebase\".\n"
> >
> > does anything of the sort.  It does not mention that this is the
> > default behavior the users would get if they provided no flags.
>
> But that is not the warning, this is the warning:
>
>   Pulling without specifying how to reconcile divergent branches is discouraged;
>   you need to specify if you want a merge, a rebase, or a fast-forward.
>   You can squelch this message by running one of the following commands:
>
>     git config pull.rebase false  # merge (the default strategy)
>     git config pull.rebase true   # rebase
>     git config pull.ff only       # fast-forward only
>
>   You can replace "git config" with "git config --global" to set a default
>   preference for all repositories.
>   If unsure, run "git pull --merge".
>   Read "git pull --help" for more information.
>
> This warning says:
>
> 1. There's 3 options: merge, rebase, fast-forward
> 2. merge is the default strategy
> 3. If unsure, specify --merge (the default strategy)
>
> So taken altogether it does say what is the default strategy.

We don't need to take them together.  #2 by itself states the default
strategy.  I don't see why defending #3 as being for the purpose of
documenting the default strategy is helpful, since it doesn't do that.

> > More
> > importantly, it makes a recommendation...and one that undercuts the
> > point of the message.
>
> So?
>
> When boarding a plane the flight attendants do a safety demonstration
> that passengers should pay attention to. If one passenger is not
> paying attention (listening to music on headphones, and reading a
> book) what should the crew do?
>
> 1. Remove the passenger's headphones and force him to pay attention to
> the safety demonstration
> 2. Let the passenger ignore the safety demonstration
>
> Human beings are independent agents responsible for their own actions.
> You as a separate human being--a crew member--can argue that it's not
> in the best interest of the passenger to ignore the safety
> demonstration, and you may be right, but the passenger decisions are
> still the passenger's decisions, even if they are bad.
>
> Do you think the crew should disregard the passenger's volition and
> force him to pay attention to the safety demonstration?
>
> > It makes it feel like the message shouldn't
> > exist at all in any circumstances.  I even suspect that adding that
> > sentence may undercut any efforts towards changing the default to
> > ff-only-as-default.  While I'm a big fan of most of what you've done
> > in this series, I will object to its merging for as long as this
> > stays.  (I definitely don't have veto power or anything close to it,
> > just stating what my opinion is.)
>
> The current warning should not exist at all.
>
> The complaint from Vít Ondruch [1] that reignited this series is a
> valid one. A *permanent* warning is not good. We should have a
> *temporary* warning with the express purpose of notifying users of an
> upcoming change.
>
> If we have not yet decided on what should be the default (Junio seems
> to have casted some doubt on the consensus [2]), and we don't have a

Useful link there.  Based on his comments, we may want to make
--ff-only, --merge, and --rebase all be mutually exclusive and result
in an error message if more than one is specified at the command line.
(But still have the command line countermand any option set in the
config, of course).

> clear path forward to implement such change (we can't even tell users
> to use "pull.ff=only", since eventually it may be
> "pull.mode=ff-only"), then we must remove the warning.
>
> It was a mistake to put a *permanent* warning before deciding to
> change the default.
>
> So, there's two options:
>
> 1. We decide on a path forward and fix the warning so it *temporarily*
> explains what will happen in the future
> 2. We remove the *permanent* warning
>
> Since we are already here, we might as well take advantage of that
> warning and repurpose it. But in the meantime--while the git project
> decides what to do, and what configurations to suggest the users to
> change--we should at the very least waste as little as the user's time
> as possible, and give him/her a quick opt-out.
>
> Yes, a quick opt-out defeats the purpose of a warning, but we must
> respect the users' volition. The user may be on a deadline trying to
> push some changes to production before the weekend, and after a system
> update be annoyed with this warning on every pull. The user may not
> have time to look at the warning, decide he wants to read the warning
> in the future, maybe next Monday, and thus not configure anything to
> silence it.
>
> What's wrong with a user saying "I don't have time for this now,
> please tell me what to do for now, I'll look at the warning later"? If
> anything for those users the configuration is the wrong thing to do,
> because being in a hurry they just choose the first configuration and
> forget about the warning without actually looking at it (because they
> didn't have time), and it will not appear any more. By typing "git
> pull --merge" the user can get rid of the warning *for now*, but the
> next time he does "git pull" the warning will reappear, and at that
> time perhaps the user does have the time to read it, and look at the
> manpage.
>
> Nobody likes their workflow to be interrupted and be forced to do anything.
>
> I don't think my patches plus that suggestion for a quick opt-out are
> in any way worse than the current situation. If you think they are,
> then we'll just have to agree to disagree.

Yes, we can agree to disagree on this particular point.
Felipe Contreras Dec. 6, 2020, 2:31 p.m. UTC | #10
On Sat, Dec 5, 2020 at 7:01 PM Elijah Newren <newren@gmail.com> wrote:
>
> On Sat, Dec 5, 2020 at 1:28 PM Felipe Contreras
> <felipe.contreras@gmail.com> wrote:
> >
> > On Sat, Dec 5, 2020 at 10:28 AM Elijah Newren <newren@gmail.com> wrote:
> > > On Fri, Dec 4, 2020 at 5:56 PM Felipe Contreras
> > > <felipe.contreras@gmail.com> wrote:
> >
> > > > Well, I already said I partly agree with you: in the --ff-only case
> > > > the suggestion should not be brought forward.
> > > >
> > > > But in the "git pull" default case, *today* it's doing a merge. If
> > > > uttering --merge and thus making the current behavior explicit instead
> > > > of implicit seems dangerous it's because it is. But not documenting it
> > > > doesn't make it any less dangerous.
> > >
> > > Sounds like we agree that the future should be ff-only-as-default.  I
> > > also agree with you that the primary problem is the current default
> > > behavior, and I'll agree with you that documenting the current default
> > > is okay.  However, I disagree that your wording here:
> > >
> > > +                       "If unsure, run \"git pull --no-rebase\".\n"
> > >
> > > does anything of the sort.  It does not mention that this is the
> > > default behavior the users would get if they provided no flags.
> >
> > But that is not the warning, this is the warning:
> >
> >   Pulling without specifying how to reconcile divergent branches is discouraged;
> >   you need to specify if you want a merge, a rebase, or a fast-forward.
> >   You can squelch this message by running one of the following commands:
> >
> >     git config pull.rebase false  # merge (the default strategy)
> >     git config pull.rebase true   # rebase
> >     git config pull.ff only       # fast-forward only
> >
> >   You can replace "git config" with "git config --global" to set a default
> >   preference for all repositories.
> >   If unsure, run "git pull --merge".
> >   Read "git pull --help" for more information.
> >
> > This warning says:
> >
> > 1. There's 3 options: merge, rebase, fast-forward
> > 2. merge is the default strategy
> > 3. If unsure, specify --merge (the default strategy)
> >
> > So taken altogether it does say what is the default strategy.
>
> We don't need to take them together.

No, but that's what I'm proposing.

> #2 by itself states the default strategy.

Yes, with configuration, not with commands.

> I don't see why defending #3 as being for the purpose of
> documenting the default strategy is helpful, since it doesn't do that.

I disagree. It does do that, but it serves a more urgent purpose: it
tells the user how to ignore us quickly, for now.

> > The current warning should not exist at all.
> >
> > The complaint from Vít Ondruch [1] that reignited this series is a
> > valid one. A *permanent* warning is not good. We should have a
> > *temporary* warning with the express purpose of notifying users of an
> > upcoming change.
> >
> > If we have not yet decided on what should be the default (Junio seems
> > to have casted some doubt on the consensus [2]), and we don't have a
>
> Useful link there.  Based on his comments, we may want to make
> --ff-only, --merge, and --rebase all be mutually exclusive and result
> in an error message if more than one is specified at the command line.
> (But still have the command line countermand any option set in the
> config, of course).

Yes, but that's a change in behavior.

Moreover, what do you suggest this should do?

  git -c "pull.rebase=true" -c "pull.ff=only" pull

I have already tried to make pull.ff=only work in several ways. It's
just not a clean solution.

Cheers.
Junio C Hamano Dec. 7, 2020, 7:26 a.m. UTC | #11
Elijah Newren <newren@gmail.com> writes:

>>   Pulling without specifying how to reconcile divergent branches is discouraged;
>>   you need to specify if you want a merge, a rebase, or a fast-forward.
>>   You can squelch this message by running one of the following commands:
>>
>>     git config pull.rebase false  # merge (the default strategy)
>>     git config pull.rebase true   # rebase
>>     git config pull.ff only       # fast-forward only
>>
>>   You can replace "git config" with "git config --global" to set a default
>>   preference for all repositories.
>>   If unsure, run "git pull --merge".
>>   Read "git pull --help" for more information.
>>
>> This warning says:
>>
>> 1. There's 3 options: merge, rebase, fast-forward
>> 2. merge is the default strategy
>> 3. If unsure, specify --merge (the default strategy)
>>
>> So taken altogether it does say what is the default strategy.
>
> We don't need to take them together.  #2 by itself states the default
> strategy.  I don't see why defending #3 as being for the purpose of
> documenting the default strategy is helpful, since it doesn't do that.

Would it work if the line were

	If unsure, run "git -c pull.rebase=false pull".

then?

The reason why I bring this up is because the first part only talks
about the choices in the terms of configuration variables, and for
those who are afraid of committing, there is no easy way, unless
they know the "git -c" single-shot mechanism, to "go ahead with one
choice in an experimental basis to see if that is what they want",
and "specify --merge" does not click well with "pull.rebase=false"
that is given in the first part.  "git pull --no-rebase" may also
work better than "git pull --merge" from that point of view, though.

But see below.


>> Do you think the crew should disregard the passenger's volition and
>> force him to pay attention to the safety demonstration?

An example that is irrelevant.  Passengers who sit on the aisle side
and clutter the floor with their luggages can pose public hazard,
and it is quite sensible for crews to remove them from the plane.  

A team member who runs "pull --[no-]rebase" pushes the result, which
may be a new history in a wrong shape, back to the shared repository
probably falls into a different category.  ... Or perhaps in the
same public hazard category?

> Useful link there.  Based on his comments, we may want to make
> --ff-only, --merge, and --rebase all be mutually exclusive and result
> in an error message if more than one is specified at the command line.

I hope "his comment" does not refer to what I said.  Redefining the
semantics of --ff-only (but not --ff=no and friends) to make it
mutually incompatible with merge/rebase was what Felipe wanted to
do, not me.

FWIW, I see the general direction as

 - When the history you are pulling is not a descendant of what you
   have on your current branch, you must show your preference
   between merging their history into yours or rebasing your history
   onto theirs.

 - In such a case, we error out, with an error message telling them
   that they must tell us which one they want in this invocation
   (e.g. with "--rebase" or "--no-rebase").  We further tell them
   that pull.rebase can be used to record their preference, if they
   almost always use the same.

	Side note: I earlier said "see below" to refer to this.  I
	think the message should offer what they can do right now
	with command line option first, and then give an option to
	configure.  IOW, the message quoted in the beginning of this
	response gives information in a wrong order.  Also,
	"discouraged" is misleading, isn't it?  When we give this
	message, we refuse to proceed until the user specifies.

	Another thing to consider is that pull.ff=only (and
	--ff-only) may be wrong choice to offer in the error message
	quoted above, if one of the objectives is to reduce the
	"annoying" message that tells the user that they must
	choose.  By definition, we do not give the message when our
	side do not have our own development, so "I could run 'pull
	--ff-only'" is a unusable knowledge to those who see the
	message and want to get out of the situation.

 - But when the history being pulled is a descendant of the current
   branch, and the user does not give us preference between merge
   and rebase (either from the command line or with configuration),
   there is no need to give such an error message---if you do not
   have your own development, we can just fast-forward the current
   branch to what we fetched from the other side.  This does not
   happen with the current system, which is what we want to fix.

 - Also when the history from the other side is a descedant and the
   user has preference, either to rebase or to merge, either from
   the command line or with configuration, we can fast-forward the
   current branch to their tip, but there are wrinkles:

   . when --ff=no is given and the choice is to merge (not rebase),
     we must create an extra merge commit.

   . when the choice is to rebase, by definition, rebasing what you
     have on top of theirs in this case is the same as fast-forwarding
     the current branch to theirs, because "what you have" is an
     empty set.

   . I am not sure how "pull --rebase --ff=no" should interact if we
     don't have our own development.  Rebasing our work on top of
     theirs is philosophically incompatible with marking where the
     side branch begins and ends with an extra commit, so it either
     should be rejected when the command line and configuration is
     parsed (i.e. regardless of the shape of the history we have at
     hand), or --ff=no gets silently ignored when --rebase is given.
     I haven't thought this one through.
Felipe Contreras Dec. 7, 2020, 9:16 a.m. UTC | #12
On Mon, Dec 7, 2020 at 1:26 AM Junio C Hamano <gitster@pobox.com> wrote:
> Elijah Newren <newren@gmail.com> writes:

> >> Do you think the crew should disregard the passenger's volition and
> >> force him to pay attention to the safety demonstration?
>
> An example that is irrelevant.  Passengers who sit on the aisle side
> and clutter the floor with their luggages can pose public hazard,
> and it is quite sensible for crews to remove them from the plane.

That's a completely different example that mixes policy with
regulations. Apples and oranges.

> A team member who runs "pull --[no-]rebase" pushes the result, which
> may be a new history in a wrong shape, back to the shared repository
> probably falls into a different category.  ... Or perhaps in the
> same public hazard category?

Yes, but if it's a public hazard or not is up to the maintainers of
specific projects. Some projects are perfectly fine with those merges.

It's the maintainer of the project that should decide what is the best
policy for the project, and instruct his/her contributors.

It's not up for individuals to decide what is a public hazard, and
it's not up to Git either. Similarly; passengers can't decide what
raises to the level of public hazzard, but neither can a flight
attendant.

> > Useful link there.  Based on his comments, we may want to make
> > --ff-only, --merge, and --rebase all be mutually exclusive and result
> > in an error message if more than one is specified at the command line.
>
> I hope "his comment" does not refer to what I said.  Redefining the
> semantics of --ff-only (but not --ff=no and friends) to make it
> mutually incompatible with merge/rebase was what Felipe wanted to
> do, not me.

That's not true.

What I want is to introduce a "pull.mode=ff-only", not change the
semantics of --ff-only.

What I have been saying is that you can't make --ff-only the default
without changing its semantics.

But since the patch series that introduced such pull.mode [1] was
completely ignored, I decided to take a shot at making --ff-only work.

You said [2]:

> > What we want to see can be done without such backward incompatible
> > changes, e.g. declaring the combination of "--ff-only" and
> > "--[no-]rebase" incompatible and/or the last one wins, I would say, and
> > I suspect Alex's RFC was an attempt to make the first step in that
> > direction.

But in all my attempts the desired result cannot be done without
backwards incompatible changes. I'm just the messenger.

To be crystal clear: I don't want to change the semantics of
--ff-only; I want to introduce a new "pull.mode=ff-only".

> FWIW, I see the general direction as
>
>  - When the history you are pulling is not a descendant of what you
>    have on your current branch, you must show your preference
>    between merging their history into yours or rebasing your history
>    onto theirs.

Agreed.

>  - In such a case, we error out, with an error message telling them
>    that they must tell us which one they want in this invocation
>    (e.g. with "--rebase" or "--no-rebase").  We further tell them
>    that pull.rebase can be used to record their preference, if they
>    almost always use the same.

Not quite. See below.

>         Side note: I earlier said "see below" to refer to this.  I
>         think the message should offer what they can do right now
>         with command line option first, and then give an option to
>         configure.  IOW, the message quoted in the beginning of this
>         response gives information in a wrong order.  Also,
>         "discouraged" is misleading, isn't it?  When we give this
>         message, we refuse to proceed until the user specifies.

That's why I keep pointing out the end goal. In the end it's not one
message, it's *two*:

You can see my suggestion for both messages in my patch series for
pull.mode [1].

When the user has specified "pull.mode=ff-only", the command dies with:

  The pull was not fast-forward, please either merge or rebase.

When the user has not specified any mode (by default), the command
would output a warning:

  The pull was not fast-forward, in the future you will have to choose a
  merge, or a rebase.

  To squelch this message and maintain the current behavior, use:

    git config --global pull.mode merge

  To squelch this message and adopt the new behavior now, use:

    git config --global push.mode ff-only

  Falling back to old style for now (merge).
  Read 'git pull --help' for more information.

But in order to reach that point we first need to decide what will be
the default mode, and how it will be configured (--ff-only or
"pull.mode=ff-only").

In the meantime this particular series is not committing for any
particular decision, and thus it's changing the warning message for
the *current* situation in which the command does not fail, but only
shows a warning. So today it's merely discouraged.

This particular patch is pretty far from the end goal. It's just patch
#2 of part 1.

>         Another thing to consider is that pull.ff=only (and
>         --ff-only) may be wrong choice to offer in the error message
>         quoted above, if one of the objectives is to reduce the
>         "annoying" message that tells the user that they must
>         choose.  By definition, we do not give the message when our
>         side do not have our own development, so "I could run 'pull
>         --ff-only'" is a unusable knowledge to those who see the
>         message and want to get out of the situation.

Those commands will squelch the warning, but won't change the
underlying nature of the warning, which is that the user must choose
to either merge or rebase.

The temporary warning will disappear, but the permanent error message will not:

  The pull was not fast-forward, please either merge or rebase.

>  - But when the history being pulled is a descendant of the current
>    branch, and the user does not give us preference between merge
>    and rebase (either from the command line or with configuration),
>    there is no need to give such an error message---if you do not
>    have your own development, we can just fast-forward the current
>    branch to what we fetched from the other side.  This does not
>    happen with the current system, which is what we want to fix.

It's not an error message, it's a warning. But yes.

>  - Also when the history from the other side is a descedant and the
>    user has preference, either to rebase or to merge, either from
>    the command line or with configuration, we can fast-forward the
>    current branch to their tip, but there are wrinkles:
>
>    . when --ff=no is given and the choice is to merge (not rebase),
>      we must create an extra merge commit.

--ff receives no arguments, it's --no-ff.

Yes.

>    . when the choice is to rebase, by definition, rebasing what you
>      have on top of theirs in this case is the same as fast-forwarding
>      the current branch to theirs, because "what you have" is an
>      empty set.

Agreed.

>    . I am not sure how "pull --rebase --ff=no" should interact if we
>      don't have our own development.  Rebasing our work on top of
>      theirs is philosophically incompatible with marking where the
>      side branch begins and ends with an extra commit, so it either
>      should be rejected when the command line and configuration is
>      parsed (i.e. regardless of the shape of the history we have at
>      hand), or --ff=no gets silently ignored when --rebase is given.
>      I haven't thought this one through.

I thought of this instance as well. In my opinion such combination
should fail. But it doesn't actually affect anything.


Just to put this series in context: it's only part 1; it does not
introduce pull.mode, and it doesn't make --ff-only the default.
There's one patch prefixed with "tentative" that changes the semantics
of --ff-only to see how it could look like when making it the default,
but in no way am I proposing that patch to be merged.

The series is entirely about improving the current situation, nothing
contentious.

In part 2 I will introduce pull.mode, which is the solution I propose,
and that would be basically rebasing [1] on top of this series
(hopefully).

I would have hoped that by this point it would have been clear why
--ff-only is not the way forward. But oh well, we'll see what happens.

Cheers.

[1] https://lore.kernel.org/git/20201125032938.786393-1-felipe.contreras@gmail.com/
[2] https://lore.kernel.org/git/xmqqpn3qfhps.fsf@gitster.c.googlers.com
Felipe Contreras Dec. 7, 2020, 9:23 a.m. UTC | #13
On Fri, Dec 4, 2020 at 6:56 PM Elijah Newren <newren@gmail.com> wrote:

> > > > 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.
> >
> > There is no "git pull --no-merge". Years ago some people argued for a
> > "pull.mode=none" (essentially making "git pull" the same as "git
> > fetch"). But right now there's no option to do that.
> >
> > There's an option to do --ff-only, but that's still a merge.
>
> I disagree.  I'm well aware that checkout_fast_forward() (which is
> what is ultimately called to do the fast-forwarding) is in a file
> called merge.c, but that doesn't make it a merge.  I don't believe it
> was anything more than a convenient place to dump some extra code at
> the time.
>
> > Perhaps: a merge, a rebase, or a fast-forward?
>
> Sure, that works; in fact, that's much better than my suggestion.  I like it.

Actually no. If we are talking about divergent branches you cannot fast-forward.

The original text was fine.
Junio C Hamano Dec. 7, 2020, 6:16 p.m. UTC | #14
Felipe Contreras <felipe.contreras@gmail.com> writes:

> When the user has specified "pull.mode=ff-only", the command dies with:
>
>   The pull was not fast-forward, please either merge or rebase.
>
> When the user has not specified any mode (by default), the command
> would output a warning:
>
>   The pull was not fast-forward, in the future you will have to choose a
>   merge, or a rebase.

I quite don't get it.  They say the same thing.  At that point the
user needs to choose between merge and rebase to move forward and
ff-only would not be a sensible choice to offer the user.

> Just to put this series in context: it's only part 1; it does not
> introduce pull.mode, and it doesn't make --ff-only the default.

I'd view the "in a non-fast-forward situation, the warning kicks in
to those who haven't chosen between merge and rebase (i.e. no
pull.rebase set to either true or false, and pull.ff not set to
only), which is a bit more gentle than the current situtation" a
good stopping point.  That state is already making ff-only the
default for unconfigured users, or you can view it as shipping "git
pull" in a shape that has the more dangerous half of its feature
disabled to avoid hurting users.  So I am not sure why you keep
saying you do not have --ff-only as the default.
Felipe Contreras Dec. 7, 2020, 7:09 p.m. UTC | #15
On Mon, Dec 7, 2020 at 12:16 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
> > When the user has specified "pull.mode=ff-only", the command dies with:
> >
> >   The pull was not fast-forward, please either merge or rebase.
> >
> > When the user has not specified any mode (by default), the command
> > would output a warning:
> >
> >   The pull was not fast-forward, in the future you will have to choose a
> >   merge, or a rebase.
>
> I quite don't get it.  They say the same thing.  At that point the
> user needs to choose between merge and rebase to move forward and
> ff-only would not be a sensible choice to offer the user.

The error happens *only* when the user has explicitly set
"pull.mode=ff-only", and the pull dies. At that point; yes, the user
must choose between one or the other.

But the warning happens by default, when the user has not chosen any
mode, and the pull succeeds. The user doesn't need to choose; the pull
continues as if the user chose the "merge" mode, which is the historic
behavior.

They start by saying the same thing. But one errors out and says the
user must choose, and the other warns that in the future the user must
choose.

> > Just to put this series in context: it's only part 1; it does not
> > introduce pull.mode, and it doesn't make --ff-only the default.
>
> I'd view the "in a non-fast-forward situation, the warning kicks in
> to those who haven't chosen between merge and rebase (i.e. no
> pull.rebase set to either true or false, and pull.ff not set to
> only), which is a bit more gentle than the current situtation" a
> good stopping point.  That state is already making ff-only the
> default for unconfigured users, or you can view it as shipping "git
> pull" in a shape that has the more dangerous half of its feature
> disabled to avoid hurting users.  So I am not sure why you keep
> saying you do not have --ff-only as the default.

The warning doesn't make the pull fail, ff-only does.

On the current master this works:

  test_expect_success 'non-fast-forward (default)' '
    setup_non_ff &&
    git pull
  '

And after my patch series (part 1) it still does. The default behavior
has not changed.

I'm not sure what it is that I'm not explaining correctly.
Junio C Hamano Dec. 7, 2020, 7:53 p.m. UTC | #16
Felipe Contreras <felipe.contreras@gmail.com> writes:

> They start by saying the same thing. But one errors out and says the
> user must choose, and the other warns that in the future the user must
> choose.

Then I do not see the point in giving the warning---even in the
future they do not have to choose as long as they are merely
following along.

>> > Just to put this series in context: it's only part 1; it does not
>> > introduce pull.mode, and it doesn't make --ff-only the default.
>>
>> I'd view the "in a non-fast-forward situation, the warning kicks in
>> to those who haven't chosen between merge and rebase (i.e. no
>> pull.rebase set to either true or false, and pull.ff not set to
>> only), which is a bit more gentle than the current situtation" a
>> good stopping point.  That state is already making ff-only the
>> default for unconfigured users, or you can view it as shipping "git
>> pull" in a shape that has the more dangerous half of its feature
>> disabled to avoid hurting users.  So I am not sure why you keep
>> saying you do not have --ff-only as the default.
>
> The warning doesn't make the pull fail, ff-only does.

Then probably you are giving an error and a warning at a wrong
place.

 - When history fast-forwards, and the user hasn't chosen between
   rebase or merge, there is no need to give any warning.  Just
   succeed by fast-forwarding.

 - When history does not fast-forward and the user hasn't chosen
   between rebase or merge, whether pull.ff is set to "only" or not,
   we should fail and the error message can instruct the user to
   choose between rebase and merge; there is no "ff-only" option
   that is useful in the situation.

And that essentially makes the "ff-only" mode the safe default that
castrates one half of the feature (the more dangerous half) of "git
pull".  Why do we make it more complicated than that by warning that
the user must choose in the future?  They will see an error tell
them that when they start pulling while on their own work, and I do
not see a need to bother them before that point.
Felipe Contreras Dec. 7, 2020, 10:14 p.m. UTC | #17
On Mon, Dec 7, 2020 at 1:53 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
> > They start by saying the same thing. But one errors out and says the
> > user must choose, and the other warns that in the future the user must
> > choose.
>
> Then I do not see the point in giving the warning---even in the
> future they do not have to choose as long as they are merely
> following along.

They don't. With my patch series they see the warning only when the
pull is non-fast-forward.

> >> > Just to put this series in context: it's only part 1; it does not
> >> > introduce pull.mode, and it doesn't make --ff-only the default.
> >>
> >> I'd view the "in a non-fast-forward situation, the warning kicks in
> >> to those who haven't chosen between merge and rebase (i.e. no
> >> pull.rebase set to either true or false, and pull.ff not set to
> >> only), which is a bit more gentle than the current situtation" a
> >> good stopping point.  That state is already making ff-only the
> >> default for unconfigured users, or you can view it as shipping "git
> >> pull" in a shape that has the more dangerous half of its feature
> >> disabled to avoid hurting users.  So I am not sure why you keep
> >> saying you do not have --ff-only as the default.
> >
> > The warning doesn't make the pull fail, ff-only does.
>
> Then probably you are giving an error and a warning at a wrong
> place.
>
>  - When history fast-forwards, and the user hasn't chosen between
>    rebase or merge, there is no need to give any warning.  Just
>    succeed by fast-forwarding.

Yes. That's what my patch [1] in this series does.

>  - When history does not fast-forward and the user hasn't chosen
>    between rebase or merge, whether pull.ff is set to "only" or not,
>    we should fail and the error message can instruct the user to
>    choose between rebase and merge; there is no "ff-only" option
>    that is useful in the situation.

Yes. *Eventually*, that's part 3.

> And that essentially makes the "ff-only" mode the safe default that
> castrates one half of the feature (the more dangerous half) of "git
> pull".  Why do we make it more complicated than that by warning that
> the user must choose in the future?  They will see an error tell
> them that when they start pulling while on their own work, and I do
> not see a need to bother them before that point.

Because the amount of time users have seen the correct error message
telling them about this upcoming change is *zero*.

No one is aware of this backwards-incompatible change that is
upcoming, since the current warning message doesn't specify any such
future change.

Moreover, no one has configured their pull.mode, because no one has
seen the message that tells them to do so.

And no one has had time to try out the upcoming "pull.mode=ff-only".
They haven't had time to find bugs on it, or weird interactions, or
suggestions how to improve it. Because the code has not been deployed,
and no warning has told them to tentatively enable that mode.

When the default for push.default was changed, it was a good thing
that we gave users (and ourselves) a grace period to try it out before
permanently flipping the switch. The transition went smoothly.

I think the progression of the warning is hard to see from all the
patch series. I'm sending all my patches and I will explain in a
timeline how it progresses until eventually we reach the desired end
goal.

Cheers.

[1] https://lore.kernel.org/git/20201204061623.1170745-8-felipe.contreras@gmail.com/
Jacob Keller Dec. 7, 2020, 11:30 p.m. UTC | #18
On Mon, Dec 7, 2020 at 11:53 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
> > They start by saying the same thing. But one errors out and says the
> > user must choose, and the other warns that in the future the user must
> > choose.
>
> Then I do not see the point in giving the warning---even in the
> future they do not have to choose as long as they are merely
> following along.
>

I think the key point is that this "in the future" is referring to "a
future version of git will make this an error"

This might be better if it said something like "The pull was not
fast-forward. In a future version of git you will need to specify
whether to merge or rebase, using pull.mode"

or something similar. In theory, this warning will go away once that
future version of git changes so that pull.mode defaults to ff-only.

The difference being that a warning will allow the command to continue
doing the default of today (merging), where as an error will stop the
command essentially just after the fetch portion finishes, without
changing the branch.

Thanks,
Jake

> >> > Just to put this series in context: it's only part 1; it does not
> >> > introduce pull.mode, and it doesn't make --ff-only the default.
> >>
> >> I'd view the "in a non-fast-forward situation, the warning kicks in
> >> to those who haven't chosen between merge and rebase (i.e. no
> >> pull.rebase set to either true or false, and pull.ff not set to
> >> only), which is a bit more gentle than the current situtation" a
> >> good stopping point.  That state is already making ff-only the
> >> default for unconfigured users, or you can view it as shipping "git
> >> pull" in a shape that has the more dangerous half of its feature
> >> disabled to avoid hurting users.  So I am not sure why you keep
> >> saying you do not have --ff-only as the default.
> >
> > The warning doesn't make the pull fail, ff-only does.
>
> Then probably you are giving an error and a warning at a wrong
> place.
>
>  - When history fast-forwards, and the user hasn't chosen between
>    rebase or merge, there is no need to give any warning.  Just
>    succeed by fast-forwarding.
>

Correct.

>  - When history does not fast-forward and the user hasn't chosen
>    between rebase or merge, whether pull.ff is set to "only" or not,
>    we should fail and the error message can instruct the user to
>    choose between rebase and merge; there is no "ff-only" option
>    that is useful in the situation.
>

Yes, I understand that this is the destination Felipe wants us to end up at.

> And that essentially makes the "ff-only" mode the safe default that
> castrates one half of the feature (the more dangerous half) of "git
> pull".  Why do we make it more complicated than that by warning that
> the user must choose in the future?  They will see an error tell
> them that when they start pulling while on their own work, and I do
> not see a need to bother them before that point.

The warning would be in an earlier version of git before that is the
default. The warning is for a transition period between today when we
merge by default (with a warning) and when we fail with an error
without completing the pull. Once the default is the ff-only mode,
then the warning is no longer necessary, as we will get an error
indicating that you must choose.

Thanks,
Jake
Junio C Hamano Dec. 8, 2020, 2:23 a.m. UTC | #19
Jacob Keller <jacob.keller@gmail.com> writes:

> I think the key point is that this "in the future" is referring to "a
> future version of git will make this an error"
>
> This might be better if it said something like "The pull was not
> fast-forward. In a future version of git you will need to specify
> whether to merge or rebase, using pull.mode"

Oh, I've actually been assuming that the current "warn but go ahead
anyway asssuming the preference is to merge" can just be declared as
a bug (iow, there is no need to say 'in the future'---we'd fix the
bug right away).

> or something similar. In theory, this warning will go away once that
> future version of git changes so that pull.mode defaults to ff-only.
>
> The difference being that a warning will allow the command to continue
> doing the default of today (merging), where as an error will stop the
> command essentially just after the fetch portion finishes, without
> changing the branch.

Yup.  If we want to take things slow, that is fine by me as well,
but I am not sure if that is even necessary, given how annoying the
existing "loudly warn but still go ahead" behaviour is, and how easy
for existing users to have squelched the annoyance by choosing
between rebase and merge already.  I've always assumed that any
existing users who started using Git in the past several years have
already set pull.rebase to one or the other value and they won't be
affected by fixing "git pull" to just error out.
Felipe Contreras Dec. 8, 2020, 3:15 a.m. UTC | #20
On Mon, Dec 7, 2020 at 8:23 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Jacob Keller <jacob.keller@gmail.com> writes:
>
> > I think the key point is that this "in the future" is referring to "a
> > future version of git will make this an error"
> >
> > This might be better if it said something like "The pull was not
> > fast-forward. In a future version of git you will need to specify
> > whether to merge or rebase, using pull.mode"
>
> Oh, I've actually been assuming that the current "warn but go ahead
> anyway asssuming the preference is to merge" can just be declared as
> a bug (iow, there is no need to say 'in the future'---we'd fix the
> bug right away).

It is a bug, in my opinion. If we were not planning on changing the
default, I would say drop the warning altogether.

*But*, if we are going to change the default, the warning can be
repurposed to say "in the future this will fail". That requires other
changes though.

> > or something similar. In theory, this warning will go away once that
> > future version of git changes so that pull.mode defaults to ff-only.
> >
> > The difference being that a warning will allow the command to continue
> > doing the default of today (merging), where as an error will stop the
> > command essentially just after the fetch portion finishes, without
> > changing the branch.

Exactly. And we can choose between those behaviors with a
configuration, like it happened with push.default.

> Yup.  If we want to take things slow, that is fine by me as well,
> but I am not sure if that is even necessary, given how annoying the
> existing "loudly warn but still go ahead" behaviour is, and how easy
> for existing users to have squelched the annoyance by choosing
> between rebase and merge already.  I've always assumed that any
> existing users who started using Git in the past several years have
> already set pull.rebase to one or the other value and they won't be
> affected by fixing "git pull" to just error out.

Existing users that are using up-to-date distributions, maybe. Debian
stable is using v2.20.1. In general it's not a good idea to assume
anything about our users. Recently a user of Oh My Zsh reported an
issue with the backwards compatibility code of git-completion; he was
using v2.17 I think.

That is exemplified by the fact that this whole thread started from a
user that refused to configure pull.rebase and expected the Git
project to just do the right thing (which is basically choosing a
useful default).

Cheers.
Junio C Hamano Dec. 8, 2020, 8:16 p.m. UTC | #21
Felipe Contreras <felipe.contreras@gmail.com> writes:

> That is exemplified by the fact that this whole thread started from a
> user that refused to configure pull.rebase and expected the Git
> project to just do the right thing (which is basically choosing a
> useful default).

Which is basically asking for impossible, and I do not think it is a
good idea to focus our effort to satisfy such a request in general.
There is no useful default that suits everybody in this particular
case.

The "force ff-only" approach indeed gives a very sensible default
behaviour of dying for those who haven't expressed the choice
between rebase and merge in a situation where the difference between
the two results in histories of different shapes.

But for anybody who uses git for real (read: produces his or her own
history), it would be pretty much a useless default that forces the
user to say rebase or merge every time 'git pull' is run.  That is
why I am not enthused by the pull.mode=(rebase/merge/ff-only)
configuration.  The third choice does help completeness.  When a
user asks "the documentation tells pull.mode can be set to
non-default behaviour---what value can one set it to to get the
default behaviour of not allowing any original work?", it can be
answered if we had pull.mode=ff-only.  But other than that, I do not
see any real use for the choice, which would mean in practice,
pull.mode would have only two useful values, rebase or merge.  That
does not feel a good enough reason to supersede what already exists,
which is pull.rebase=yes/no.

Perhaps there is a good reason why certain classes of users would
want to configure pull.mode=ff-only (i.e. "I might type 'git pull'
by mistake, please stop me if I did so on a branch I have real work
already.").  If that is the case, I would very much agree that it
would be awkward to express that choice in the current framework to
choose between pull.rebase=yes/no and pull.mode=(rebase/merge/ff-only)
would become a lot easier to explain.

I dunno.
Felipe Contreras Dec. 9, 2020, 9:52 a.m. UTC | #22
On Tue, Dec 8, 2020 at 2:16 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
> > That is exemplified by the fact that this whole thread started from a
> > user that refused to configure pull.rebase and expected the Git
> > project to just do the right thing (which is basically choosing a
> > useful default).
>
> Which is basically asking for impossible, and I do not think it is a
> good idea to focus our effort to satisfy such a request in general.
> There is no useful default that suits everybody in this particular
> case.

I think I already made this point, but this is the nirvana fallacy
(the perfect is the enemy of the good) [1]. Just because we can't have
the perfect solution doesn't mean we can't pursue something better
than the current state.

What was asked was not a perfect solution, just a better default. If
right now the default is good enough for 20% of users, and with some
changes we can make it better for 40%... that's progress. We don't
have to wait until we have a solution for 100% of them.

> But for anybody who uses git for real (read: produces his or her own
> history), it would be pretty much a useless default that forces the
> user to say rebase or merge every time 'git pull' is run.

This is not true.

I will give you a real example.

I create a branch named "fc/margin" based on "master", I make my
changes, I push the branch to my personal repository, and create a
pull request. This is the typical triangular workflow.

Then I do "git pull [--ff-only]". What will happen? 1) As long as my
branch is not merged upstream, I will get an error, and my branch will
stay where it is. But then, 2) when my branch is finally merged to
"master" it will be fast-forwarded, so now both "fc/margin" and
"origin/master" point to the same commit.

A. Did I use git "for real"? (produce my own history)
B. Was "git pull [--ff-only]" useful in this case?

I think that one of the problems is that Git has so many different
workflows that finding another person that has your same workflow is
like finding a person with your same birthday. It's not impossible,
just takes more than a few tries.

Also, and this is not a deriding question, I'm genuinely curious: how
often do you send pull requests?

BTW, this example above is real [2]. In my particular case very often
I'm creating history, I'm just not the one pulling it.

> But other than that, I do not
> see any real use for the choice, which would mean in practice,
> pull.mode would have only two useful values, rebase or merge.  That
> does not feel a good enough reason to supersede what already exists,
> which is pull.rebase=yes/no.

The fact that you don't see the use doesn't mean the use is not there.

Why do you think this issue keeps coming back again and again, and
again? And every time it comes back people say the same thing:
"fast-forward-only merges should be the default".

Unfortunately it's not that simple. It's a rabbit hole that leads to a
cacophony of issues in git pull. However, we can fix some of them
*today*.

> Perhaps there is a good reason why certain classes of users would
> want to configure pull.mode=ff-only (i.e. "I might type 'git pull'
> by mistake, please stop me if I did so on a branch I have real work
> already.").  If that is the case, I would very much agree that it
> would be awkward to express that choice in the current framework to
> choose between pull.rebase=yes/no and pull.mode=(rebase/merge/ff-only)
> would become a lot easier to explain.

There's three options:

1. pull.ff=only (that doesn't work IMO)
2. pull.rebase=ff-only (that works, but it's kind of wonky)
3. pull.mode=ff-only (maybe it should be pull.mode=fast-forward)

But the current option (pull.mode=merge) just doesn't fly. And BTW, I
did create a poll in reddit's r/git [3], and 67% (of 789) said they
didn't specify anything, just "git pull".

So, most people simply do "git pull" and hope for the best.

Moreover, in 2014 I said if we don't fix it now (which is likely), we
will be discussing it again [4], and here we are now. And I'm saying
it again: leave the mode as "merge", we will be discussing this again.

I could do some mail archeology if you want, but this issue starts to
be mentioned at least since 2010, and virtually everyone (except one
person) agreed the default must change, even Linus Torvalds. Reading
back what Linus said [5], it's something very, *very* close to what
I'm proposing (I would argue my proposal is better).

So you let me know. Do you want me to dig a decade of discussions and
coalesce those conclusions into a summary so we can decide how to
proceed? Or should I drop the plan? Only that if we drop it, I
*guarantee* we will discuss it yet again years later.

Moreover, this is the reason why I split the series in 3. Even if you
decide you don't want to change the default, part I of the series can
still be merged *today*, and everyone would benefit.

Cheers.

[1] https://en.wikipedia.org/wiki/Nirvana_fallacy
[2] https://github.com/git/git.github.io/commit/5d90fd64d80847e3d873da43515750bc04b639e2
[3] https://www.reddit.com/r/git/comments/k6uj7c/do_you_use_git_pull/
[4] https://lore.kernel.org/git/536429e97cf5a_200c12912f094@nysa.notmuch/
[5] https://lore.kernel.org/git/CA+55aFz2Uvq4vmyjJPao5tS-uuVvKm6mbP7Uz8sdq1VMxMGJCw@mail.gmail.com/
Elijah Newren Dec. 9, 2020, 7:05 p.m. UTC | #23
Hi,

On Wed, Dec 9, 2020 at 1:53 AM Felipe Contreras
<felipe.contreras@gmail.com> wrote:
>
> On Tue, Dec 8, 2020 at 2:16 PM Junio C Hamano <gitster@pobox.com> wrote:
> >
> > Felipe Contreras <felipe.contreras@gmail.com> writes:
> >
> > > That is exemplified by the fact that this whole thread started from a
> > > user that refused to configure pull.rebase and expected the Git
> > > project to just do the right thing (which is basically choosing a
> > > useful default).
> >
> > Which is basically asking for impossible, and I do not think it is a
> > good idea to focus our effort to satisfy such a request in general.
> > There is no useful default that suits everybody in this particular
> > case.
>
> I think I already made this point, but this is the nirvana fallacy
> (the perfect is the enemy of the good) [1]. Just because we can't have
> the perfect solution doesn't mean we can't pursue something better
> than the current state.
>
> What was asked was not a perfect solution, just a better default. If
> right now the default is good enough for 20% of users, and with some
> changes we can make it better for 40%... that's progress. We don't
> have to wait until we have a solution for 100% of them.
>
> > But for anybody who uses git for real (read: produces his or her own
> > history), it would be pretty much a useless default that forces the
> > user to say rebase or merge every time 'git pull' is run.
>
> This is not true.
>
> I will give you a real example.
>
> I create a branch named "fc/margin" based on "master", I make my
> changes, I push the branch to my personal repository, and create a
> pull request. This is the typical triangular workflow.
>
> Then I do "git pull [--ff-only]". What will happen? 1) As long as my
> branch is not merged upstream, I will get an error, and my branch will
> stay where it is. But then, 2) when my branch is finally merged to
> "master" it will be fast-forwarded, so now both "fc/margin" and
> "origin/master" point to the same commit.
>
> A. Did I use git "for real"? (produce my own history)
> B. Was "git pull [--ff-only]" useful in this case?
>
> I think that one of the problems is that Git has so many different
> workflows that finding another person that has your same workflow is
> like finding a person with your same birthday. It's not impossible,
> just takes more than a few tries.
>
> Also, and this is not a deriding question, I'm genuinely curious: how
> often do you send pull requests?
>
> BTW, this example above is real [2]. In my particular case very often
> I'm creating history, I'm just not the one pulling it.
>
> > But other than that, I do not
> > see any real use for the choice, which would mean in practice,
> > pull.mode would have only two useful values, rebase or merge.  That
> > does not feel a good enough reason to supersede what already exists,
> > which is pull.rebase=yes/no.
>
> The fact that you don't see the use doesn't mean the use is not there.
>
> Why do you think this issue keeps coming back again and again, and
> again? And every time it comes back people say the same thing:
> "fast-forward-only merges should be the default".
>
> Unfortunately it's not that simple. It's a rabbit hole that leads to a
> cacophony of issues in git pull. However, we can fix some of them
> *today*.
>
> > Perhaps there is a good reason why certain classes of users would
> > want to configure pull.mode=ff-only (i.e. "I might type 'git pull'
> > by mistake, please stop me if I did so on a branch I have real work
> > already.").  If that is the case, I would very much agree that it
> > would be awkward to express that choice in the current framework to
> > choose between pull.rebase=yes/no and pull.mode=(rebase/merge/ff-only)
> > would become a lot easier to explain.
>
> There's three options:
>
> 1. pull.ff=only (that doesn't work IMO)
> 2. pull.rebase=ff-only (that works, but it's kind of wonky)
> 3. pull.mode=ff-only (maybe it should be pull.mode=fast-forward)
>
> But the current option (pull.mode=merge) just doesn't fly. And BTW, I
> did create a poll in reddit's r/git [3], and 67% (of 789) said they
> didn't specify anything, just "git pull".
>
> So, most people simply do "git pull" and hope for the best.
>
> Moreover, in 2014 I said if we don't fix it now (which is likely), we
> will be discussing it again [4], and here we are now. And I'm saying
> it again: leave the mode as "merge", we will be discussing this again.
>
> I could do some mail archeology if you want, but this issue starts to
> be mentioned at least since 2010, and virtually everyone (except one
> person) agreed the default must change, even Linus Torvalds. Reading
> back what Linus said [5], it's something very, *very* close to what
> I'm proposing (I would argue my proposal is better).
>
> So you let me know. Do you want me to dig a decade of discussions and
> coalesce those conclusions into a summary so we can decide how to
> proceed? Or should I drop the plan? Only that if we drop it, I
> *guarantee* we will discuss it yet again years later.
>
> Moreover, this is the reason why I split the series in 3. Even if you
> decide you don't want to change the default, part I of the series can
> still be merged *today*, and everyone would benefit.

Have I missed some subtlety here?  This whole email appears to me to
be arguing against a strawman.  Reading Junio's other emails in this
thread[1][2], it's pretty clear he thinks the current behavior is
buggy and suggests how it should be changed.  From what I can tell,
you appear to be arguing against doing nothing and against only
accepting perfection, neither of which were positions I saw anyone
take.  In fact, the positions you argue for at length appear to
exactly match the ones he took[1][2].  What am I missing?

[1] https://lore.kernel.org/git/xmqq360h8286.fsf@gitster.c.googlers.com/
[2] https://lore.kernel.org/git/xmqqlfe99yvy.fsf@gitster.c.googlers.com/
Felipe Contreras Dec. 10, 2020, 2:39 a.m. UTC | #24
Hello,

On Wed, Dec 9, 2020 at 1:05 PM Elijah Newren <newren@gmail.com> wrote:
> On Wed, Dec 9, 2020 at 1:53 AM Felipe Contreras
> <felipe.contreras@gmail.com> wrote:

> > I could do some mail archeology if you want, but this issue starts to
> > be mentioned at least since 2010, and virtually everyone (except one
> > person) agreed the default must change, even Linus Torvalds. Reading
> > back what Linus said [5], it's something very, *very* close to what
> > I'm proposing (I would argue my proposal is better).
> >
> > So you let me know. Do you want me to dig a decade of discussions and
> > coalesce those conclusions into a summary so we can decide how to
> > proceed? Or should I drop the plan? Only that if we drop it, I
> > *guarantee* we will discuss it yet again years later.
> >
> > Moreover, this is the reason why I split the series in 3. Even if you
> > decide you don't want to change the default, part I of the series can
> > still be merged *today*, and everyone would benefit.
>
> Have I missed some subtlety here?  This whole email appears to me to
> be arguing against a strawman.  Reading Junio's other emails in this
> thread[1][2], it's pretty clear he thinks the current behavior is
> buggy and suggests how it should be changed.  From what I can tell,
> you appear to be arguing against doing nothing and against only
> accepting perfection, neither of which were positions I saw anyone
> take.  In fact, the positions you argue for at length appear to
> exactly match the ones he took[1][2].  What am I missing?

People change their minds.

Perhaps I misinterpreted something, but when Junio said "I dunno" I
take it to mean: he is unsure my proposal "pull.mode=ff-only" makes
sense. He also said "for anybody who uses git for real, [force
-ff-only] would be pretty much a useless default", which I take it to
mean that perhaps we shouldn't change the default to that.

Back in 2013 Junio said "I now see how such a default makes sense."
[1], but we are in 2020 and such a default that made sense is not the
default.

Mind reading is a really bad habbit [2]; I don't know what other
people think, and I would not presume to know.

All I know is that the path forward is unclear. And because I
prognosticated that, I split the series in 3 parts, and I've yet to
see any objection to part I, which would improve the situation for
users *today*.

So, while we make up our collective minds, there's no reason to bother
our users with an annoying warning on *every* *single* *pull*.

Cheers.

[1] https://lore.kernel.org/git/7vli74baym.fsf@alter.siamese.dyndns.org/
[2] https://cogbtherapy.com/cbt-blog/common-cognitive-distortions-mind-reading
Junio C Hamano Dec. 10, 2020, 6:45 a.m. UTC | #25
Elijah Newren <newren@gmail.com> writes:

> Have I missed some subtlety here?  This whole email appears to me to
> be arguing against a strawman.  Reading Junio's other emails in this
> thread[1][2], it's pretty clear he thinks the current behavior is
> buggy and suggests how it should be changed.  From what I can tell,
> you appear to be arguing against doing nothing and against only
> accepting perfection, neither of which were positions I saw anyone
> take.  In fact, the positions you argue for at length appear to
> exactly match the ones he took[1][2].  What am I missing?
>
> [1] https://lore.kernel.org/git/xmqq360h8286.fsf@gitster.c.googlers.com/
> [2] https://lore.kernel.org/git/xmqqlfe99yvy.fsf@gitster.c.googlers.com/

I tend to agree that the endgame state I want is pretty similar to
what Felipe wants.  The seeming confusion is probably due to what
exactly I mean by "default" is different from what he means.

I view the proposed "for unconfigured users, pull dies, and tells
them to choose between rebase or merge before it can continue, when
faced with a non-ff history" as a safe fallback behaviour until the
users make their choice.

It is a safe fallback to disable the more dangerous half of the
command until the user gives enough information to the command to do
its job without damaging the resulting history; it is not something
the users would actively want to choose.  

And that is what I meant by the default behaviour.

And when we stop in such a manner, it is sensible to give an error
message telling them 

 - why we are stopping,

 - what they can do to move the immediate situation forward
   (i.e. command line option that lets them choose), and

 - what they can do to make their choice permanent so that they
   would never see the command stop when facing a non-ff history
   (i.e. the configuration variables).

Up to this point, I think both of us agree with the above.

We start to differ after this point.  I would want to see only
"rebase" and "merge in the "choice" in the above list.  Felipe, if I
understand correctly, wants to add the third one, "ff-only", which
means the more dangerous half of "pull" is disabled by default.

I do not want to include that choice, as it would mean the more
familiar pull.rebase=yes/no would no longer work, and we'd need to
introduce a new variable, like pull.mode=ff-only.  It would mean for
those who use "pull" to consolidate histories on their side and
other people's side, whether they use rebase or merge, would now
have two ways to do the same thing (i.e. pull.rebase=no or
pull.mode=merge).  That is just making the end user experience more
complex than necessary.

Without introducing pull.mode, the only thing the users cannot do,
as far as I can tell, is to explicitly ask for the behaviour of an
unconfigured user (i.e. error out when faced with non-ff history)
without being told about the way to use configuration variable to
permanently record their choice.  Other than that, the existing
pull.rebase=yes/no is perfectly adequate.  Felipe seems to think
otherwise and wants not just the "safe fallback behaviour", but can
explicitly be configured using pull.mode=ff-only (and if that is not
what Felipe thinks, perhaps we are already in agreement without
realizing it).

Thinking about it again, I guess pull.rebase=yes/no plus a new
advise entry can easily give what Felipe seems to want.  We teach
"git pull", when it is not told to rebase or merge (either via the
command line --[no-]rebase or via pull.rebase configuration) and
when it sees a non-ff history, to

 - error out with a message telling the user that we are stopping,
   because the two histories needs consolidating to conclude this
   pull, but the two available ways would give vastly different
   result.

 - check the advice.pullNonFF configuration (as usual, it defaults
   to true) and if true, further tell the user that

    (1) they can either run "git pull --rebase" or "git pull
        --no-rebase",

    (2) they can configure their choice permanently with
        pull.rebase=yes/no, and

    (3) they can leave "pull" in the half-disabled state but stop
        seeing this message by setting advice.pullNonFF to false.

The last part (i.e. advice.pullnonFF) is the only new thing I said
in the various discussions on this topic (simply because I just came
up with the idea).  Everything else I've already said it elsewhere.

I dunno.
Felipe Contreras Dec. 10, 2020, 9:08 a.m. UTC | #26
On Thu, Dec 10, 2020 at 12:45 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Elijah Newren <newren@gmail.com> writes:
>
> > Have I missed some subtlety here?  This whole email appears to me to
> > be arguing against a strawman.  Reading Junio's other emails in this
> > thread[1][2], it's pretty clear he thinks the current behavior is
> > buggy and suggests how it should be changed.  From what I can tell,
> > you appear to be arguing against doing nothing and against only
> > accepting perfection, neither of which were positions I saw anyone
> > take.  In fact, the positions you argue for at length appear to
> > exactly match the ones he took[1][2].  What am I missing?
> >
> > [1] https://lore.kernel.org/git/xmqq360h8286.fsf@gitster.c.googlers.com/
> > [2] https://lore.kernel.org/git/xmqqlfe99yvy.fsf@gitster.c.googlers.com/
>
> I tend to agree that the endgame state I want is pretty similar to
> what Felipe wants.  The seeming confusion is probably due to what
> exactly I mean by "default" is different from what he means.
>
> I view the proposed "for unconfigured users, pull dies, and tells
> them to choose between rebase or merge before it can continue, when
> faced with a non-ff history" as a safe fallback behaviour until the
> users make their choice.
>
> It is a safe fallback to disable the more dangerous half of the
> command until the user gives enough information to the command to do
> its job without damaging the resulting history; it is not something
> the users would actively want to choose.
>
> And that is what I meant by the default behaviour.
>
> And when we stop in such a manner, it is sensible to give an error
> message telling them
>
>  - why we are stopping,
>
>  - what they can do to move the immediate situation forward
>    (i.e. command line option that lets them choose), and
>
>  - what they can do to make their choice permanent so that they
>    would never see the command stop when facing a non-ff history
>    (i.e. the configuration variables).
>
> Up to this point, I think both of us agree with the above.

I don't agree with the above.

The error I propose is just:

  The pull was not fast-forward, please either merge or rebase.

That's it. Nothing more.

I explained that was the final end goal in my list of steps [1]. I do
not think any suggestion for commands or configurations belongs in a
*permanent* error message.

Do we even have an error message that long?

> We start to differ after this point.  I would want to see only
> "rebase" and "merge in the "choice" in the above list.  Felipe, if I
> understand correctly, wants to add the third one, "ff-only", which
> means the more dangerous half of "pull" is disabled by default.

No. I don't want to present the user *any* configuration option in the
*permanent error* message, just: "please either merge or rebase".
That's it.

> I do not want to include that choice, as it would mean the more
> familiar pull.rebase=yes/no would no longer work, and we'd need to
> introduce a new variable, like pull.mode=ff-only.

Whether we present the user with that option or not doesn't matter.
"pull.rebase=yes/no", would still work. The only thing that changes is
what happens if the user does *not* set that option.

The reason "pull.mode=ff-only" needs to be introduced is that
--ff-only doesn't work. Otherwise there's no way the user cannot
select the "safe default" mode. It has absolutely nothing to do with
what we present the user with.

> Without introducing pull.mode, the only thing the users cannot do,
> as far as I can tell, is to explicitly ask for the behaviour of an
> unconfigured user (i.e. error out when faced with non-ff history)
> without being told about the way to use configuration variable to
> permanently record their choice.

That's right, which is a *crucial* thing they must have.

Part II of the proposal has these:

  pull: add pull.mode
  pull: add pull.mode=ff-only
  pull: advice of future changes

This is where we want to be for a good considerable amount of time. A
point where the user is a) warned about future changes, b) can
configure both the old and the new behavior, and c) we have time to
test and tune this new behavior (in case there are mistakes), *BEFORE*
forcing all our users to switch to this new mode (unless they haven't
configured otherwise).

This is what happened with "push.default=simple". We didn't just say:
the simple mode is the default mode, so just leave "push.default"
blank if you want this mode. No. Because in theory we might have
decided before v2.0 that another mode was to be the default, or
perhaps in v3.0 another one will. We want the user to be able to say:
this is the mode I want. Not whatever mode the Git project decides
it's better this day of the week: *this* mode.

There is nothing good about the user being unable to specify the mode
he/she wants instead of the default, even if at this moment in time
they happen to be the same.

Moreover, I can attach a patch on top of part I that skips right ahead
towards part 3, and flips the switch without any intermediary steps.
The patch is simple, but it's likely to be wrong. And I doubt anyone
has tested this patch series at this point. No amount of looking at
the code is going to spot all the problems.

People actually need to try this code.

Otherwise it's just some patches flying around that will never be
merged, and the problem will remain forever there.

> Other than that, the existing
> pull.rebase=yes/no is perfectly adequate.

You cannot specify "pull.rebase=ff-only", but I did send a patch for
that [2]. It was you the one that said it looked "quite funny" [3]:
"it looks quite funny for that single variable that controls the way
how pull works, whether rebase or merge is used, is pull.REBASE".

If you think it's fine to have this warning:

  The pull was not fast-forward, in the future you will have to choose
a merge, or a rebase.

  To quell this message you have two main options:

  1. Adopt the new behavior:

    git config --global pull.rebase ff-only

  2. Maintain the current behavior:

    git config --global pull.rebase false

  For now we will fall back to the traditional behavior (merge).
  Read "git pull --help" for more information.

For months, possibly a year, possibly until v3.0, instead of this one
(which is my proposal):

  The pull was not fast-forward, in the future you will have to choose
a merge, or a rebase.

  To quell this message you have two main options:

  1. Adopt the new behavior:

    git config --global pull.mode fast-forward

  2. Maintain the current behavior:

    git config --global pull.mode merge

  For now we will fall back to the traditional behavior (merge).
  Read "git pull --help" for more information.

By all means, let's go for that, make the decision.

But it was you who wanted to see how a pull.mode solution would look
like [4][5]. Well, this is how it looks like: 3 patches.

> Felipe seems to think
> otherwise and wants not just the "safe fallback behaviour", but can
> explicitly be configured using pull.mode=ff-only (and if that is not
> what Felipe thinks, perhaps we are already in agreement without
> realizing it).

No. It doesn't matter if it's "pull.mode=ff-only", or
"pull.rebase=ff-only", or "pull.ff=only", but one of those must turn
on the behavior that we want. And right now no option does.

> Thinking about it again, I guess pull.rebase=yes/no plus a new
> advise entry can easily give what Felipe seems to want.

It's not about what I want, it's about what the project wants. And I
think it's pretty clear what the project wants:

  1. git pull to eventually fail in non-fast-forward cases
  2. A grace period before that switch is flipped

That's it.

My proposal is the only one (so far) that gives us that.

Cheers.

[1] https://lore.kernel.org/git/CAMP44s2hUCd9qc83LReGyjy8N+u++eK6VjwGhDhrX0f0SbKmig@mail.gmail.com/
[2] https://lore.kernel.org/git/20201123224621.2573159-2-felipe.contreras@gmail.com/
[3] https://lore.kernel.org/git/xmqqim9vlkdn.fsf@gitster.c.googlers.com/
[4] https://lore.kernel.org/git/xmqqy2irjy4f.fsf@gitster.c.googlers.com/
[5] https://lore.kernel.org/git/xmqq7dqagtgx.fsf@gitster.c.googlers.com/
Felipe Contreras Dec. 10, 2020, 10:01 a.m. UTC | #27
On Thu, Dec 10, 2020 at 3:08 AM Felipe Contreras
<felipe.contreras@gmail.com> wrote:

> It's not about what I want, it's about what the project wants. And I
> think it's pretty clear what the project wants:
>
>   1. git pull to eventually fail in non-fast-forward cases
>   2. A grace period before that switch is flipped
>
> That's it.
>
> My proposal is the only one (so far) that gives us that.

Not to mention that part I is not even contentious. We can improve the
situation of users *today*.
Junio C Hamano Dec. 11, 2020, 7:17 a.m. UTC | #28
Felipe Contreras <felipe.contreras@gmail.com> writes:

>> And when we stop in such a manner, it is sensible to give an error
>> message telling them
>>
>>  - why we are stopping,
>>
>>  - what they can do to move the immediate situation forward
>>    (i.e. command line option that lets them choose), and
>>
>>  - what they can do to make their choice permanent so that they
>>    would never see the command stop when facing a non-ff history
>>    (i.e. the configuration variables).
>>
>> Up to this point, I think both of us agree with the above.
>
> I don't agree with the above.
>
> The error I propose is just:
>
>   The pull was not fast-forward, please either merge or rebase.
>
> That's it. Nothing more.

It says "why we are stopping." quite well.  It would be a good
message to use as the first part of the three-part message I
mentioned above.

> I explained that was the final end goal in my list of steps [1]. I do
> not think any suggestion for commands or configurations belongs in a
> *permanent* error message.

In the design I have in mind in the message you are responding to,
the users who haven't told their choice to Git would be the only
folks who get all three.

You want to let the user express: "I do not want to choose either
rebase or merge.  I want 'pull' to fail when it needs to deal with
non-ff history.  But I do not need to be told about command line
option and configuration every time."

I said I don't (I view that disabling half the "git pull" just a
safe fallback behaviour until the user chooses between merge and
rebase), but if we wanted to offer it as a valid choice to users, we
can do so.  We just make it possible to squelch the latter two parts
of the three-part message---you leave pull.rebase unconfigured and
squelch the latter two parts of the message, and you got the "stop
me, I do not merge or rebase, but don't even tell me how to further
configure" already.

I agree the latter two should not be part of *permanent* error
message.  And my suggestion did not intend to make them so---it
should have been quite obvious to who read the message you are
responding to through to the end and understood what it said.

Now, how would we make it possible to squelch the latter two parts?
It is not a good idea to introduce pull.mode=ff-only for that
purpose (pull.mode=rebase and pull.rebase=yes would unnecessarily
become two redundant ways to do the same thing if we added a new
pull.mode variable).

But there is an established way used in this project when we allow
squelching overly-helpful help messages, and we can apply it here as
well.  That way:

 - unconfigured folks would get all the three parts of the messages,
   just like the current system.

 - if you tell rebase or merge, you do not see any.

 - if you do not choose between rebase or merge, you can still
   squelch the latter two by setting advice.pullNonFF to false.

The last one is "keep the more dangerous half of 'git pull' disabled,
without getting told how to enable it over and over", which is what
you want to be able to specify.

> The reason "pull.mode=ff-only" needs to be introduced is that
> --ff-only doesn't work. Otherwise there's no way the user cannot
> select the "safe default" mode. It has absolutely nothing to do with
> what we present the user with.

I too initially thought that pull.mode may be needed, but probably I
was wrong.  I do think this can be done without pull.mode at all, at
least in two ways, without adding different ways to do the same
thing.

 - When pull.rebase is set to 'no' and pull.ff is set to 'only',
   "git pull" that sees a non-ff history should error out safely.
   The user is telling that their preference is to merge, but the
   difference between merge and rebase does not really matter
   because pull.ff=only would mean we forbid merges of non-ff
   history anyway.  The message you'd get would be "fatal: Not
   possible to fast-forward, aborting." though.

 - Or with the advice that hides the latter two points, a user can
   unset pull.rebase and set the advice.pullNonFF to false to get
   the same behaviour (i.e. disable the more dangerous half of
   "pull") with just the "we stopped" error message.

I think either of these are close enough to what you want, and I
think the latter gives us more flexibility in how we tone down the
message with advice.pullNonFF.
Felipe Contreras Dec. 11, 2020, 11:33 a.m. UTC | #29
On Fri, Dec 11, 2020 at 4:02 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
> >> And when we stop in such a manner, it is sensible to give an error
> >> message telling them
> >>
> >>  - why we are stopping,
> >>
> >>  - what they can do to move the immediate situation forward
> >>    (i.e. command line option that lets them choose), and
> >>
> >>  - what they can do to make their choice permanent so that they
> >>    would never see the command stop when facing a non-ff history
> >>    (i.e. the configuration variables).
> >>
> >> Up to this point, I think both of us agree with the above.
> >
> > I don't agree with the above.
> >
> > The error I propose is just:
> >
> >   The pull was not fast-forward, please either merge or rebase.
> >
> > That's it. Nothing more.
>
> It says "why we are stopping." quite well.  It would be a good
> message to use as the first part of the three-part message I
> mentioned above.

The two key parts of the message are:

1. It is an *error*
2. It is *permanent*

> > I explained that was the final end goal in my list of steps [1]. I do
> > not think any suggestion for commands or configurations belongs in a
> > *permanent* error message.
>
> In the design I have in mind in the message you are responding to,
> the users who haven't told their choice to Git would be the only
> folks who get all three.

What would that error message look like? And do you have any other
example of the current user interface where such a condescending long
error message is displayed?

> You want to let the user express: "I do not want to choose either
> rebase or merge.  I want 'pull' to fail when it needs to deal with
> non-ff history.  But I do not need to be told about command line
> option and configuration every time."

That's right.

> I said I don't (I view that disabling half the "git pull" just a
> safe fallback behaviour until the user chooses between merge and
> rebase), but if we wanted to offer it as a valid choice to users, we
> can do so.  We just make it possible to squelch the latter two parts
> of the three-part message---you leave pull.rebase unconfigured and
> squelch the latter two parts of the message, and you got the "stop
> me, I do not merge or rebase, but don't even tell me how to further
> configure" already.
>
> I agree the latter two should not be part of *permanent* error
> message.  And my suggestion did not intend to make them so---it
> should have been quite obvious to who read the message you are
> responding to through to the end and understood what it said.

It doesn't matter (much) if it's temporary or permanent, it's still an
*error* message.

Currently it's a warning, and people are complaining, even though the
pull still works.

And you want to make it an error, and *always* fail? Even though the
user has not been warned that such a change was coming and how to
evade it?

I'm against that. I would rather do nothing than intentionally break
user experience without warning.

Johannes said he didn't mind the warning. I want the warning to
remain, just make it a different warning. You want to break his
experience without a grace period and suddenly make it an error.

> Now, how would we make it possible to squelch the latter two parts?

...

This is irrelevant.

As long as it's an error I don't care if it's short or long. I'm
against turning on an error from one version to the next.

> > The reason "pull.mode=ff-only" needs to be introduced is that
> > --ff-only doesn't work. Otherwise there's no way the user cannot
> > select the "safe default" mode. It has absolutely nothing to do with
> > what we present the user with.
>
> I too initially thought that pull.mode may be needed, but probably I
> was wrong.  I do think this can be done without pull.mode at all, at
> least in two ways, without adding different ways to do the same
> thing.
>
>  - When pull.rebase is set to 'no' and pull.ff is set to 'only',
>    "git pull" that sees a non-ff history should error out safely.
>    The user is telling that their preference is to merge, but the
>    difference between merge and rebase does not really matter
>    because pull.ff=only would mean we forbid merges of non-ff
>    history anyway.  The message you'd get would be "fatal: Not
>    possible to fast-forward, aborting." though.
>
>  - Or with the advice that hides the latter two points, a user can
>    unset pull.rebase and set the advice.pullNonFF to false to get
>    the same behaviour (i.e. disable the more dangerous half of
>    "pull") with just the "we stopped" error message.

So, after your hypothetical patch, there would be no difference between:

  git -c pull.rebase=no -c pull.ff=only pull

and:

  git -c advice.pullnonff=false pull

?

> I think either of these are close enough to what you want, and I
> think the latter gives us more flexibility in how we tone down the
> message with advice.pullNonFF.

You are missing at least two things.

I'll wait for your response.

Cheers.
Junio C Hamano Dec. 14, 2020, 9:04 p.m. UTC | #30
Felipe Contreras <felipe.contreras@gmail.com> writes:

> This is irrelevant.

Oh, now you are saying that you do not need a way to squelch these
message to help unconfigured users choose between rebase or merge?
I am confused.

> As long as it's an error I don't care if it's short or long. I'm
> against turning on an error from one version to the next.

Now you are changing your mind?  Current version and past ones do
not make it an error to pull non-ff history without choosing rebase
or merge---we go ahead and merge anyway.  I thought that in the far
future agreed between two of us, it would be turnend into an error
at some point.  There needs one version that turns it into an error
for that to happen.  Puzzled.

>> I too initially thought that pull.mode may be needed, but probably I
>> was wrong.  I do think this can be done without pull.mode at all, at
>> least in two ways, without adding different ways to do the same
>> thing.
>>
>>  - When pull.rebase is set to 'no' and pull.ff is set to 'only',
>>    "git pull" that sees a non-ff history should error out safely.
>>    The user is telling that their preference is to merge, but the
>>    difference between merge and rebase does not really matter
>>    because pull.ff=only would mean we forbid merges of non-ff
>>    history anyway.  The message you'd get would be "fatal: Not
>>    possible to fast-forward, aborting." though.
>>
>>  - Or with the advice that hides the latter two points, a user can
>>    unset pull.rebase and set the advice.pullNonFF to false to get
>>    the same behaviour (i.e. disable the more dangerous half of
>>    "pull") with just the "we stopped" error message.
>
> So, after your hypothetical patch, there would be no difference between:
>
>   git -c pull.rebase=no -c pull.ff=only pull
>
> and:
>
>   git -c advice.pullnonff=false pull
>
> ?

We do not have to or implement both, but either should give us the
"when pull sees a non-ff history, it should stop without merging or
rebasing, and the user won't be given the advice on how to choose
between merge and rebase" behaviour, I would think.

>> I think either of these are close enough to what you want, and I
>> think the latter gives us more flexibility in how we tone down the
>> message with advice.pullNonFF.
>
> You are missing at least two things.

I am guessing that the '?' above I just answered is one you wanted
to ask me, but what's the other one?
Felipe Contreras Dec. 14, 2020, 10:30 p.m. UTC | #31
Junio C Hamano wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
> 
> > This is irrelevant.
> 
> Oh, now you are saying that you do not need a way to squelch these
> message to help unconfigured users choose between rebase or merge?
> I am confused.

We need a way to squelch the *advise* message, yes.

But this is a secondary priority. The first priority is not to break
current behavior users rely on.

> > As long as it's an error I don't care if it's short or long. I'm
> > against turning on an error from one version to the next.
> 
> Now you are changing your mind?  Current version and past ones do
> not make it an error to pull non-ff history without choosing rebase
> or merge---we go ahead and merge anyway.  I thought that in the far
> future agreed between two of us, it would be turnend into an error
> at some point.  There needs one version that turns it into an error
> for that to happen.  Puzzled.

Yes, in the future, *after* a deprecation period.

Once the users have had a chance to configure git to do what they want,
have tried the new mode, haven't had issues with the new mode, or if
they disagree with the proposed *future* behavior; complain in the
forums at their disposal.

Not before.

> >> I too initially thought that pull.mode may be needed, but probably I
> >> was wrong.  I do think this can be done without pull.mode at all, at
> >> least in two ways, without adding different ways to do the same
> >> thing.
> >>
> >>  - When pull.rebase is set to 'no' and pull.ff is set to 'only',
> >>    "git pull" that sees a non-ff history should error out safely.
> >>    The user is telling that their preference is to merge, but the
> >>    difference between merge and rebase does not really matter
> >>    because pull.ff=only would mean we forbid merges of non-ff
> >>    history anyway.  The message you'd get would be "fatal: Not
> >>    possible to fast-forward, aborting." though.
> >>
> >>  - Or with the advice that hides the latter two points, a user can
> >>    unset pull.rebase and set the advice.pullNonFF to false to get
> >>    the same behaviour (i.e. disable the more dangerous half of
> >>    "pull") with just the "we stopped" error message.
> >
> > So, after your hypothetical patch, there would be no difference between:
> >
> >   git -c pull.rebase=no -c pull.ff=only pull
> >
> > and:
> >
> >   git -c advice.pullnonff=false pull
> >
> > ?
> 
> We do not have to or implement both, but either should give us the
> "when pull sees a non-ff history, it should stop without merging or
> rebasing, and the user won't be given the advice on how to choose
> between merge and rebase" behaviour, I would think.

Right, so both should error out.

And what should these do?

  git -c pull.rebase=no -c pull.ff=only pull --merge

  git -c advice.pullnonff=false pull --merge

I'm going to answer because I think it's obvious what you would expect:
if you pass --merge, both should succeed.

Except they won't, because "git pull --ff-only --merge" fails.

Correct?

> >> I think either of these are close enough to what you want, and I
> >> think the latter gives us more flexibility in how we tone down the
> >> message with advice.pullNonFF.
> >
> > You are missing at least two things.
> 
> I am guessing that the '?' above I just answered is one you wanted
> to ask me, but what's the other one?

Yes. The other is what I explained above; we need a grace deprecation
period where we can explain to the user in a simple way what to expect
in the future.

And it's much easier to explain to the user that:

  git pull --merge -> pull.mode = merge
  git pull --rebase -> pull.mode = rebase
  git pull -> pull.mode = fast-forward

Than:

  git pull --merge -> pull.rebase = false
  git pull --rebase -> pull.rebase = true
  git pull -> pull.rebase = false + pull.ff = only

But those are not the only two. For example there's this additional
problem of how to interact with the other values of pull.rebase (other
than true and false):

  pull.mode = merge
  pull.rebase = merges

I would expect in this particular configuration that:

  git pull -> git pull --merge
  git pull --rebase -> git pull --rebase=merges

There's no way to represent that with just pull.rebase.

And there's more: some people suggested other modes in 2013, like
"pull.mode=none" (essentially "git fetch"), or
"pull.mode=merge-reverse-parents".

But the first one should be enough ("git pull --ff-only --merge" doesn't
work).

Cheers.
Junio C Hamano Dec. 14, 2020, 11:14 p.m. UTC | #32
Felipe Contreras <felipe.contreras@gmail.com> writes:

>> We do not have to or implement both, but either should give us the
>> "when pull sees a non-ff history, it should stop without merging or
>> rebasing, and the user won't be given the advice on how to choose
>> between merge and rebase" behaviour, I would think.
>
> Right, so both should error out.

One of them, not both; the one we choose to implement would make it
fail, I would think.

> And what should these do?
>
>   git -c pull.rebase=no -c pull.ff=only pull --merge

There is no --merge, so let's reread with s/--merge/--no-rebase/;
the command line would be saying "I only want to fast-forward, or I
want the command to fail".  The advice code should not trigger
(i.e. we gave an explicit preference to merge and not to rebase, so
rebase_unspecified would be false), but the configured preference
that says "we take only fast-forward merges" will still be in effect.
If the history is fast-forward, the merge backend will happily
advance our history.  If not, the merge backend will happily fail
the pull.

If you want to countermand the configured preference from the
command line and allow a merge to be done when non-ff history is
given, what you'd need to override is not --merge/--no-rebase.  The
configuration pull.rebase=no already says you do not want rebase and
you want merge).  You want to override --ff-only, so I'd expect
"pull --ff" (or "pull --no-ff") to be how you override your
configured preference and merge in a non-ff history, either by fast
forwarding or creating an extra merge commit.

>   git -c advice.pullnonff=false pull --merge

Again with s/--merge/--no-rebase/; but that is showing the
preference not to rebase and to merge from the command line, so
shouldn't it just go ahead and merge without any advice?


> I'm going to answer because I think it's obvious what you would expect:
> if you pass --merge, both should succeed.
>
> Except they won't, because "git pull --ff-only --merge" fails.
>
> Correct?

See above.
Jeff King Dec. 15, 2020, 2:31 a.m. UTC | #33
On Thu, Dec 10, 2020 at 11:17:01PM -0800, Junio C Hamano wrote:

> You want to let the user express: "I do not want to choose either
> rebase or merge.  I want 'pull' to fail when it needs to deal with
> non-ff history.  But I do not need to be told about command line
> option and configuration every time."
> 
> I said I don't (I view that disabling half the "git pull" just a
> safe fallback behaviour until the user chooses between merge and
> rebase), but if we wanted to offer it as a valid choice to users, we
> can do so.  We just make it possible to squelch the latter two parts
> of the three-part message---you leave pull.rebase unconfigured and
> squelch the latter two parts of the message, and you got the "stop
> me, I do not merge or rebase, but don't even tell me how to further
> configure" already.

FWIW, I would be such a user who would want to squelch the error but
keep the ff-only behavior (and have been doing so for years via
pull.ff=only).

I primarily use "git pull" only with branches where I am tracking
upstream work, but don't have any of my own (e.g., if I do all of my
work on topic branches, I may still keep a "master" branch that mirrors
"origin/master", and use "git checkout master && git pull" to
fast-forward it).

So I think this is a valuable thing to have. And it does work just fine
already, without pull.mode. The reasons to care about pull.mode (IMHO)
are mostly about explaining it:

  - it isn't respected with rebase. So if you set pull.rebase=true, now
    pull.ff=only does nothing. This is arguably confusing, though I
    doubt it comes up much in practice.

  - having a single tri-state of merge/rebase/error for "what do I do
    when pulling a non-fast-forward merge" is conceptually simple and
    easy to explain.

So I like pull.mode in that sense. But it is also weighed against the
fact that we'd still have to support pull.rebase, and we'd still have to
support pull.ff, and explain how those interact with pull.mode (and I
think any new error in this area must be squelched by those existing
variables, or it would be a regression for people who already picked
their default long ago).

> But there is an established way used in this project when we allow
> squelching overly-helpful help messages, and we can apply it here as
> well.  That way:
> 
>  - unconfigured folks would get all the three parts of the messages,
>    just like the current system.
> 
>  - if you tell rebase or merge, you do not see any.
> 
>  - if you do not choose between rebase or merge, you can still
>    squelch the latter two by setting advice.pullNonFF to false.
> 
> The last one is "keep the more dangerous half of 'git pull' disabled,
> without getting told how to enable it over and over", which is what
> you want to be able to specify.

Using advice.* to squelch the advice would be fine with me, provided it
was _also_ squelched by the existing config options.

Which I think is where your thinking is ending up.

-Peff
Felipe Contreras Dec. 15, 2020, 2:36 a.m. UTC | #34
Junio C Hamano wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
> 
> >> We do not have to or implement both, but either should give us the
> >> "when pull sees a non-ff history, it should stop without merging or
> >> rebasing, and the user won't be given the advice on how to choose
> >> between merge and rebase" behaviour, I would think.
> >
> > Right, so both should error out.
> 
> One of them, not both; the one we choose to implement would make it
> fail, I would think.

If only one of them fail, then it's impossible to tell the user:

  If you want to try the new behavior, type this command:

    git config $something

We have to go straight for the error, with no deprecation period.

> > And what should these do?
> >
> >   git -c pull.rebase=no -c pull.ff=only pull --merge
> 
> There is no --merge, so let's reread with s/--merge/--no-rebase/;

Back in 2013 everyone found it easier to speak in terms of --merge, even
you, BTW, who was the first one to send a patch with --merge [1], after
Linus suggested it [2].

> the command line would be saying "I only want to fast-forward, or I
> want the command to fail".  The advice code should not trigger
> (i.e. we gave an explicit preference to merge and not to rebase, so
> rebase_unspecified would be false), but the configured preference
> that says "we take only fast-forward merges" will still be in effect.
> If the history is fast-forward, the merge backend will happily
> advance our history.  If not, the merge backend will happily fail
> the pull.
> 
> If you want to countermand the configured preference from the
> command line and allow a merge to be done when non-ff history is
> given, what you'd need to override is not --merge/--no-rebase.  The
> configuration pull.rebase=no already says you do not want rebase and
> you want merge).  You want to override --ff-only, so I'd expect
> "pull --ff" (or "pull --no-ff") to be how you override your
> configured preference and merge in a non-ff history, either by fast
> forwarding or creating an extra merge commit.

Precisely, so you can't tell the user:

  If you want to try the new behavior, type this command:

    git config pull.ff only

Because, after the user gets the error:

  Not a fast-forward, either merge or rebase.

She cannot do:

  git pull --merge

Which is the best idiom to specify she wants a merge.

The command will still fail, even after the user has specified she wants
the new mode, and that she wants to merge in this perticular case.

> >   git -c advice.pullnonff=false pull --merge
> 
> Again with s/--merge/--no-rebase/; but that is showing the
> preference not to rebase and to merge from the command line, so
> shouldn't it just go ahead and merge without any advice?

Yes, but *before* we make this the default, what configuration get us
there today?

> > I'm going to answer because I think it's obvious what you would expect:
> > if you pass --merge, both should succeed.
> >
> > Except they won't, because "git pull --ff-only --merge" fails.
> >
> > Correct?
> 
> See above.

That's a "correct". There's no configuration today that gives us both:

 1. fail: git -c $config pull
 2. pass: git -c $config pull --merge

Like this does:

 1. fail: git -c pull.mode=fast-forward pull
 2. pass: git -c pull.mode=fast-forward pull --merge

Cheers.

[1] https://lore.kernel.org/git/7v4ncjs5az.fsf_-_@alter.siamese.dyndns.org/
[2] https://lore.kernel.org/git/CA+55aFz2Uvq4vmyjJPao5tS-uuVvKm6mbP7Uz8sdq1VMxMGJCw@mail.gmail.com/
Felipe Contreras Dec. 15, 2020, 3:49 a.m. UTC | #35
Jeff King wrote:
> On Thu, Dec 10, 2020 at 11:17:01PM -0800, Junio C Hamano wrote:
> 
> > You want to let the user express: "I do not want to choose either
> > rebase or merge.  I want 'pull' to fail when it needs to deal with
> > non-ff history.  But I do not need to be told about command line
> > option and configuration every time."
> > 
> > I said I don't (I view that disabling half the "git pull" just a
> > safe fallback behaviour until the user chooses between merge and
> > rebase), but if we wanted to offer it as a valid choice to users, we
> > can do so.  We just make it possible to squelch the latter two parts
> > of the three-part message---you leave pull.rebase unconfigured and
> > squelch the latter two parts of the message, and you got the "stop
> > me, I do not merge or rebase, but don't even tell me how to further
> > configure" already.
> 
> FWIW, I would be such a user who would want to squelch the error but
> keep the ff-only behavior (and have been doing so for years via
> pull.ff=only).

If you have configured pull.ff=only, you should be getting this error:

	Not possible to fast-forward, aborting.

We would like this error instead:

  Not a fast-forward, either merge or rebase.

And after that it should be obvious what this command should do with
your current configuration:

  git pull --merge

But it would fail, even after specifying you want to do a merge, because
"git pull --ff-only --merge" fails.

> So I think this is a valuable thing to have. And it does work just fine
> already, without pull.mode. The reasons to care about pull.mode (IMHO)
> are mostly about explaining it:
> 
>   - it isn't respected with rebase. So if you set pull.rebase=true, now
>     pull.ff=only does nothing. This is arguably confusing, though I
>     doubt it comes up much in practice.
> 
>   - having a single tri-state of merge/rebase/error for "what do I do
>     when pulling a non-fast-forward merge" is conceptually simple and
>     easy to explain.

Indeed. It's *mostly* about explaining it, but there's also what I
stated above:

  git -c pull.mode=fast-forward pull
  git -c pull.mode=fast-forward pull --merge

We want the first one to fail, and the second one to succeed, which
can't be done with the current "pull.ff=only".

> So I like pull.mode in that sense. But it is also weighed against the
> fact that we'd still have to support pull.rebase, and we'd still have to
> support pull.ff, and explain how those interact with pull.mode (and I
> think any new error in this area must be squelched by those existing
> variables, or it would be a regression for people who already picked
> their default long ago).

The interactions should be easy:

  * pull.rebase=false -> pull.mode=merge
  * pull.rebase=the-rest -> pull.mode=rebase
  * pull.ff=only -> pull.mode=fast-forward

(note: I'm not entirely sure of the last one)

Then, any --merge/--rebase option overrides the mode, but not so the
--ff options.

We would still like this to fail:

  git -c pull.mode=merge pull --ff-only

Right?

In other words: pull.mode is strongly linked with pull.rebase, but not so
much wih --ff*.

> > But there is an established way used in this project when we allow
> > squelching overly-helpful help messages, and we can apply it here as
> > well.  That way:
> > 
> >  - unconfigured folks would get all the three parts of the messages,
> >    just like the current system.
> > 
> >  - if you tell rebase or merge, you do not see any.
> > 
> >  - if you do not choose between rebase or merge, you can still
> >    squelch the latter two by setting advice.pullNonFF to false.
> > 
> > The last one is "keep the more dangerous half of 'git pull' disabled,
> > without getting told how to enable it over and over", which is what
> > you want to be able to specify.
> 
> Using advice.* to squelch the advice would be fine with me, provided it
> was _also_ squelched by the existing config options.
> 
> Which I think is where your thinking is ending up.

Yes. At least that's my thinking.

Cheers.
Junio C Hamano Dec. 15, 2020, 11:18 a.m. UTC | #36
Jeff King <peff@peff.net> writes:

> So I like pull.mode in that sense. But it is also weighed against the
> fact that we'd still have to support pull.rebase, and we'd still have to
> support pull.ff, and explain how those interact with pull.mode (and I
> think any new error in this area must be squelched by those existing
> variables, or it would be a regression for people who already picked
> their default long ago).

I agree that if we were starting from scratch, things would have
been much simpler; choose among three ways to reconcile histories
(merge, rebase, or ff-only), without adding --[no-]rebase, and allow
--[no-]ff only when merging (i.e. things like --ff-only --ff,
--no-ff --rebase, would be nonsense combinations).  The additional
complexity of introducing pull.mode is the primary thing I am
hesitant to support it, as we have to design and explain how
existing knobs interact with newer one.

> Using advice.* to squelch the advice would be fine with me, provided it
> was _also_ squelched by the existing config options.

Meaning "once you choose between rebase or merge, facing a non-ff
history would not trigger the advice message"?  I think that is
already the case with the released versions of Git, and I think that
is a good thing to keep.  The advice is only for unconfigured folks
who did not tell --[no-]rebase from the command line.  One bad thing
about it in the released versions is that it would trigger even when
the history fast-forwards.  The latest round of patches in 'seen'
squelches the advice when pulling a fast-forward history even for
unconfigured folks as we'd just fast-forward without erroring out.

> Which I think is where your thinking is ending up.

Pretty much.
Felipe Contreras Dec. 15, 2020, 12:53 p.m. UTC | #37
Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
> 
> > So I like pull.mode in that sense. But it is also weighed against the
> > fact that we'd still have to support pull.rebase, and we'd still have to
> > support pull.ff, and explain how those interact with pull.mode (and I
> > think any new error in this area must be squelched by those existing
> > variables, or it would be a regression for people who already picked
> > their default long ago).
> 
> I agree that if we were starting from scratch, things would have
> been much simpler; choose among three ways to reconcile histories
> (merge, rebase, or ff-only), without adding --[no-]rebase, and allow
> --[no-]ff only when merging (i.e. things like --ff-only --ff,
> --no-ff --rebase, would be nonsense combinations).  The additional
> complexity of introducing pull.mode is the primary thing I am
> hesitant to support it, as we have to design and explain how
> existing knobs interact with newer one.

I have already tried all the options.

If we go through a deprecation period (which I insist we must), the
options are these:

 1. Change the semantics of --ff-only (breaks backwards-compatibility)
 2. Add a "pull.rebase=ff-only" option (very weird)
 3. Add a "pull.mode" configuration (tons of advantages)

Yes, we have to map the modes to the existing knobs, but it's not complex:

 * pull.rebase = false  <=> pull.mode = merge
 * pull.rebase = true   <=> pull.mode = rebase
 * pull.rebase = *       -> pull.mode = rebase

That's it. We don't even have to map --ff-only (which has different
semantics anyway).

Cheers.
diff mbox series

Patch

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"
+			"You can squelch this message by running one of the following commands:\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.\n"
+			"If unsure, run \"git pull --no-rebase\".\n"
+			"Read \"git pull --help\" for more information."
+			));
 	}
 
 	return REBASE_FALSE;