v2, 14 partspull: default warning improvements

>
> This essentially reverts commit f15e7cf5cc.
>
> Once commit d9f15d37f1 introduced the autostash option for the merge
> mode, it's not necessary to skip the fast-forward run_merge() when
> autostash is set.

> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  builtin/pull.c | 15 ++++++---------
>  1 file changed, 6 insertions(+), 9 deletions(-)
>
> diff --git a/builtin/pull.c b/builtin/pull.c
> index 6279e9ee37..c38548dab8 100644
> --- a/builtin/pull.c
> +++ b/builtin/pull.c
> @@ -927,7 +927,6 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
>         struct oid_array merge_heads = OID_ARRAY_INIT;
>         struct object_id orig_head, curr_head;
>         struct object_id rebase_fork_point;
> -       int autostash;
>
>         if (!getenv("GIT_REFLOG_ACTION"))
>                 set_reflog_message(argc, argv);
> @@ -960,8 +959,8 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
>         if (get_oid("HEAD", &orig_head))
>                 oidclr(&orig_head);
>
> -       autostash = config_autostash;
>         if (opt_rebase) {
> +               int autostash = config_autostash;
>                 if (opt_autostash != -1)
>                         autostash = opt_autostash;
>
> @@ -1030,13 +1029,11 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
>                      recurse_submodules == RECURSE_SUBMODULES_ON_DEMAND) &&
>                     submodule_touches_in_range(the_repository, &rebase_fork_point, &curr_head))
>                         die(_("cannot rebase with locally recorded submodule modifications"));
> -               if (!autostash) {
> -                       if (get_can_ff(&orig_head, &merge_heads.oid[0])) {
> -                               /* we can fast-forward this without invoking rebase */
> -                               opt_ff = "--ff-only";
> -                               ran_ff = 1;
> -                               ret = run_merge();
> -                       }
> +               if (get_can_ff(&orig_head, &merge_heads.oid[0])) {
> +                       /* we can fast-forward this without invoking rebase */
> +                       opt_ff = "--ff-only";
> +                       ran_ff = 1;
> +                       ret = run_merge();
>                 }
>                 if (!ran_ff)
>                         ret = run_rebase(&curr_head, merge_heads.oid, &rebase_fork_point);
> --
> 2.29.2
>

>
> On Thu, Dec 3, 2020 at 10:16 PM Felipe Contreras
> <felipe.contreras@gmail.com> wrote:
> >
> > This essentially reverts commit f15e7cf5cc.
> >
> > Once commit d9f15d37f1 introduced the autostash option for the merge
> > mode, it's not necessary to skip the fast-forward run_merge() when
> > autostash is set.
>
> It helps reviewers and future code readers if you provide a little
> context when referring to commits, making use of git log's
> --pretty=reference option to get the output.

> So, for example, here
> your commit would read:
>
> """
> This essentially reverts commit f15e7cf5cc (pull: ff --rebase
> --autostash works in dirty repo, 2017-06-01).
>
> Once commit d9f15d37f1 (pull: pass --autostash to merge, 2020-04-07)
> introduced the autostash option for the merge
> mode, it's not necessary to skip the fast-forward run_merge() when
> autostash is set.
> """
>
> I still found it slightly hard to follow the explanation even with the
> added summaries, though.

>
> On Fri, Dec 4, 2020 at 5:07 PM Elijah Newren <newren@gmail.com> wrote:
> >
> > On Thu, Dec 3, 2020 at 10:16 PM Felipe Contreras
> > <felipe.contreras@gmail.com> wrote:
> > >
> > > This essentially reverts commit f15e7cf5cc.
> > >
> > > Once commit d9f15d37f1 introduced the autostash option for the merge
> > > mode, it's not necessary to skip the fast-forward run_merge() when
> > > autostash is set.
> >
> > It helps reviewers and future code readers if you provide a little
> > context when referring to commits, making use of git log's
> > --pretty=reference option to get the output.
>
> Yes, I actually have this alias:
>
>   short = show --quiet --format='%C(auto)%h (%s)%C(reset)'
>
> Which shows almost the same thing. I've updated it to
> --format=reference. Thanks for the suggestion.
>
> And I usually add those descriptions.
>
> > So, for example, here
> > your commit would read:
> >
> > """
> > This essentially reverts commit f15e7cf5cc (pull: ff --rebase
> > --autostash works in dirty repo, 2017-06-01).
> >
> > Once commit d9f15d37f1 (pull: pass --autostash to merge, 2020-04-07)
> > introduced the autostash option for the merge
> > mode, it's not necessary to skip the fast-forward run_merge() when
> > autostash is set.
> > """
> >
> > I still found it slightly hard to follow the explanation even with the
> > added summaries, though.
>
> And that's the reason I didn't add them. You need to look at both the
> commits to understand why one cancels the other. In my opinion in this
> particular case the description only makes the text harder to read.
>
> Probably some better $subjects like f15e7cf5cc (pull: skip ff merge
> shortcut on --rebase --autostash) would have helped.
>
> > An extra sentence at the end of the second
> > paragraph to make it clear what is being changed ("So, change the code
> > to fast-forward even when autostash is set.") seems to help.
>
> OK. That's implied by "it's not necessary to skip the fast-forward"
> but it's better to be explicit.
>
> How about this:
>
> Currently "git pull --rebase" takes a shortcut in the case a
> fast-forward merge is possible; run_merge() is called with --ff-only.
>
> However, "git merge" didn't have an --autostash option, so, when "git
> pull --rebase --autostash" was called *and* the fast-forward merge
> shortcut was taken, then the pull failed.
>
> This was fixed in commit f15e7cf5cc (pull: ff --rebase --autostash
> works in dirty repo, 2017-06-01) by simply skipping the fast-forward
> merge shortcut.
>
> Later on "git merge" learned the --autostash option [a03b55530a
> (merge: teach --autostash option, 2020-04-07)], and so did "git pull"
> [d9f15d37f1 (pull: pass --autostash to merge, 2020-04-07)].
>
> Therefore it's not necessary to skip the fast-forward merge shortcut
> anymore when called with --rebase --autostash.
>
> Let's always take the fast-forward merge shortcut by essentially
> reverting f15e7cf5cc.

>
> We want users to know what is a fast-forward in order to understand the
> default warning.
>
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  Documentation/git-pull.txt | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
>
> diff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt
> index 5c3fb67c01..dc812139f4 100644
> --- a/Documentation/git-pull.txt
> +++ b/Documentation/git-pull.txt
> @@ -38,6 +38,20 @@ as set by linkgit:git-branch[1] `--track`.
>  Assume the following history exists and the current branch is
>  "`master`":
>
> +------------
> +         A---B---C master on origin
> +        /
> +    D---E master
> +------------
> +
> +Then `git pull` will merge in a fast-foward way up to the new master.
> +
> +------------
> +    D---E---A---B---C master, origin/master
> +------------
> +
> +However, a non-fast-foward case looks very different.
> +
>  ------------
>           A---B---C master on origin
>          /
> --
> 2.29.2

>
> We want users to know what is a fast-forward in order to understand the
> default warning.
>
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  Documentation/git-pull.txt | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
>
> diff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt
> index 5c3fb67c01..dc812139f4 100644
> --- a/Documentation/git-pull.txt
> +++ b/Documentation/git-pull.txt
> @@ -38,6 +38,20 @@ as set by linkgit:git-branch[1] `--track`.
>  Assume the following history exists and the current branch is
>  "`master`":
>
> +------------
> +         A---B---C master on origin
> +        /
> +    D---E master
> +------------
> +

> +Then `git pull` will merge in a fast-foward way up to the new master.
> +
> +------------
> +    D---E---A---B---C master, origin/master
> +------------
> +
> +However, a non-fast-foward case looks very different.
> +
>  ------------
>           A---B---C master on origin
>          /
> --
> 2.29.2
>

>
> 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(-)
>
> 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"

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

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

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

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

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

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

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

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

>
> 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". :-)

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

>
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  builtin/pull.c | 39 +++++++++++++++++++++++----------------
>  1 file changed, 23 insertions(+), 16 deletions(-)
>
> diff --git a/builtin/pull.c b/builtin/pull.c
> index 8daba7539c..f82e214fc8 100644
> --- a/builtin/pull.c
> +++ b/builtin/pull.c
> @@ -27,6 +27,8 @@
>  #include "commit-reach.h"
>  #include "sequencer.h"
>
> +static int default_mode;
> +
>  /**
>   * Parses the value of --rebase. If value is a false value, returns
>   * REBASE_FALSE. If value is a true value, returns REBASE_TRUE. If value is
> @@ -344,21 +346,7 @@ static enum rebase_type config_get_rebase(void)
>         if (!git_config_get_value("pull.rebase", &value))
>                 return parse_config_rebase("pull.rebase", value, 1);
>
> -       if (opt_verbosity >= 0 && !opt_ff) {
> -               advise(_("Pulling without specifying how to reconcile divergent branches is\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."
> -                       ));
> -       }
> +       default_mode = 1;
>
>         return REBASE_FALSE;
>  }
> @@ -927,6 +915,7 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
>         struct oid_array merge_heads = OID_ARRAY_INIT;
>         struct object_id orig_head, curr_head;
>         struct object_id rebase_fork_point;
> +       int can_ff;
>
>         if (!getenv("GIT_REFLOG_ACTION"))
>                 set_reflog_message(argc, argv);
> @@ -1022,6 +1011,24 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
>         if (opt_rebase && merge_heads.nr > 1)
>                 die(_("Cannot rebase onto multiple branches."));
>
> +       can_ff = get_can_ff(&orig_head, &merge_heads.oid[0]);
> +
> +       if (default_mode && opt_verbosity >= 0 && !opt_ff) {
> +               advise(_("Pulling without specifying how to reconcile divergent branches is\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."
> +                       ));
> +       }
> +
>         if (opt_rebase) {
>                 int ret = 0;
>                 if ((recurse_submodules == RECURSE_SUBMODULES_ON ||
> @@ -1029,7 +1036,7 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
>                     submodule_touches_in_range(the_repository, &rebase_fork_point, &curr_head))
>                         die(_("cannot rebase with locally recorded submodule modifications"));
>
> -               if (get_can_ff(&orig_head, &merge_heads.oid[0])) {
> +               if (can_ff) {
>                         /* we can fast-forward this without invoking rebase */
>                         opt_ff = "--ff-only";
>                         ret = run_merge();
> --
> 2.29.2
>

>
> On Thu, Dec 3, 2020 at 10:16 PM Felipe Contreras
> <felipe.contreras@gmail.com> wrote:
> >
> > Up to the point where can check if we can fast-forward or not.
>
> Seem to be missing some subjects in that sentence.  ;-)  Perhaps:
>
> Move the default warning to the point where we can check if we can
> fast-forward or not.

> However,
> looking over the patch and pulling up the code, I actually find it
> pretty odd that this warning was in a function named
> config_get_rebase().  The warning is not rebase-specific, and so
> clearly does not belong there.  And for such a function name, the only
> kinds of warnings I'd expect are ones where the user configured some
> option but set it to a value that cannot make sense.  So it all around
> seems like the wrong place to me, and I find your patch to be a good
> cleanup.  It would benefit from a slightly improved commit message
> though.  :-)

>
> There's no need to store ran_ff. Now it's obvious from the conditionals.
>
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  builtin/pull.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/builtin/pull.c b/builtin/pull.c
> index c38548dab8..8daba7539c 100644
> --- a/builtin/pull.c
> +++ b/builtin/pull.c
> @@ -1024,19 +1024,18 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
>
>         if (opt_rebase) {
>                 int ret = 0;
> -               int ran_ff = 0;
>                 if ((recurse_submodules == RECURSE_SUBMODULES_ON ||
>                      recurse_submodules == RECURSE_SUBMODULES_ON_DEMAND) &&
>                     submodule_touches_in_range(the_repository, &rebase_fork_point, &curr_head))
>                         die(_("cannot rebase with locally recorded submodule modifications"));
> +

>                 if (get_can_ff(&orig_head, &merge_heads.oid[0])) {
>                         /* we can fast-forward this without invoking rebase */
>                         opt_ff = "--ff-only";
> -                       ran_ff = 1;
>                         ret = run_merge();
> -               }
> -               if (!ran_ff)
> +               } else {
>                         ret = run_rebase(&curr_head, merge_heads.oid, &rebase_fork_point);
> +               }
>
>                 if (!ret && (recurse_submodules == RECURSE_SUBMODULES_ON ||
>                              recurse_submodules == RECURSE_SUBMODULES_ON_DEMAND))
> --
> 2.29.2

>
> On Thu, Dec 3, 2020 at 10:16 PM Felipe Contreras
> <felipe.contreras@gmail.com> wrote:
> >
> > There's no need to store ran_ff. Now it's obvious from the conditionals.
> >
> > Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> > ---
> >  builtin/pull.c | 7 +++----
> >  1 file changed, 3 insertions(+), 4 deletions(-)
> >
> > diff --git a/builtin/pull.c b/builtin/pull.c
> > index c38548dab8..8daba7539c 100644
> > --- a/builtin/pull.c
> > +++ b/builtin/pull.c
> > @@ -1024,19 +1024,18 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
> >
> >         if (opt_rebase) {
> >                 int ret = 0;
> > -               int ran_ff = 0;
> >                 if ((recurse_submodules == RECURSE_SUBMODULES_ON ||
> >                      recurse_submodules == RECURSE_SUBMODULES_ON_DEMAND) &&
> >                     submodule_touches_in_range(the_repository, &rebase_fork_point, &curr_head))
> >                         die(_("cannot rebase with locally recorded submodule modifications"));
> > +
> spurious change?
>
> (It does make it look nicer, though.)

> >                 if (get_can_ff(&orig_head, &merge_heads.oid[0])) {
> >                         /* we can fast-forward this without invoking rebase */
> >                         opt_ff = "--ff-only";
> > -                       ran_ff = 1;
> >                         ret = run_merge();
> > -               }
> > -               if (!ran_ff)
> > +               } else {
> >                         ret = run_rebase(&curr_head, merge_heads.oid, &rebase_fork_point);
> > +               }
> >
> >                 if (!ret && (recurse_submodules == RECURSE_SUBMODULES_ON ||
> >                              recurse_submodules == RECURSE_SUBMODULES_ON_DEMAND))
> > --
> > 2.29.2
>
> Yeah, reads better to me after this cleanup.

>
> The current error is not user-friendly:
>
>   fatal: not possible to fast-forward, aborting.
>
> We want something that actually explains what is going on:
>
>   The pull was not fast-forward, please either merge or rebase.
>   If unsure, run "git pull --merge".

> The user can get rid of the warning by doing either --merge or
> --rebase.
>
> Except: doing "git pull --merge" is not actually enough; we would return
> to the previous behavior: "fatal: not possible to fast-forward, aborting."

>
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  builtin/pull.c  | 34 ++++++++++++++++-----------
>  t/t5520-pull.sh | 62 +++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 82 insertions(+), 14 deletions(-)
>
> diff --git a/builtin/pull.c b/builtin/pull.c
> index 6ea95c9fc9..f54ff36b57 100644
> --- a/builtin/pull.c
> +++ b/builtin/pull.c
> @@ -1015,20 +1015,26 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
>
>         can_ff = get_can_ff(&orig_head, &merge_heads.oid[0]);
>
> -       if (default_mode && !can_ff && opt_verbosity >= 0 && !opt_ff) {
> -               advise(_("Pulling without specifying how to reconcile divergent branches is\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 --merge\".\n"
> -                       "Read \"git pull --help\" for more information."
> -                       ));
> +       if (!can_ff && default_mode) {
> +               if (opt_ff && !strcmp(opt_ff, "--ff-only")) {
> +                       die(_("The pull was not fast-forward, please either merge or rebase.\n"
> +                               "If unsure, run \"git pull --merge\"."));

> +               }
> +               if (opt_verbosity >= 0 && !opt_ff) {
> +                       advise(_("Pulling without specifying how to reconcile divergent branches is\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 --merge\".\n"
> +                               "Read \"git pull --help\" for more information."
> +                               ));
> +               }
>         }
>
>         if (opt_rebase) {
> diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
> index 9fae07cdfa..fdd1f79b06 100755
> --- a/t/t5520-pull.sh
> +++ b/t/t5520-pull.sh
> @@ -819,4 +819,66 @@ test_expect_success 'git pull --rebase against local branch' '
>         test_cmp expect file2
>  '
>
> +test_expect_success 'git pull fast-forward (ff-only)' '
> +       test_when_finished "git checkout master && git branch -D other test" &&
> +       test_config pull.ff only &&
> +       git checkout -b other master &&
> +       >new &&
> +       git add new &&
> +       git commit -m new &&
> +       git checkout -b test -t other &&
> +       git reset --hard master &&
> +       git pull
> +'
> +
> +test_expect_success 'git pull non-fast-forward (ff-only)' '
> +       test_when_finished "git checkout master && git branch -D other test" &&
> +       test_config pull.ff only &&
> +       git checkout -b other master^ &&
> +       >new &&
> +       git add new &&
> +       git commit -m new &&
> +       git checkout -b test -t other &&
> +       git reset --hard master &&
> +       test_must_fail git pull
> +'
> +
> +test_expect_failure 'git pull non-fast-forward with merge (ff-only)' '
> +       test_when_finished "git checkout master && git branch -D other test" &&
> +       test_config pull.ff only &&
> +       git checkout -b other master^ &&
> +       >new &&
> +       git add new &&
> +       git commit -m new &&
> +       git checkout -b test -t other &&
> +       git reset --hard master &&
> +       git pull --no-rebase

> +'
> +
> +test_expect_success 'git pull non-fast-forward with rebase (ff-only)' '
> +       test_when_finished "git checkout master && git branch -D other test" &&
> +       test_config pull.ff only &&
> +       git checkout -b other master^ &&
> +       >new &&
> +       git add new &&
> +       git commit -m new &&
> +       git checkout -b test -t other &&
> +       git reset --hard master &&
> +       git pull --rebase
> +'
> +
> +test_expect_success 'git pull non-fast-forward error message' '
> +       test_when_finished "git checkout master && git branch -D other test" &&
> +       test_config pull.ff only &&
> +       git checkout -b other master^ &&
> +       >new &&
> +       git add new &&
> +       git commit -m new &&
> +       git checkout -b test -t other &&
> +       git reset --hard master &&
> +       test_must_fail git pull 2> error &&
> +       cat error &&
> +       grep -q "The pull was not fast-forward" error
> +'
> +
>  test_done
> --
> 2.29.2

>
> On Thu, Dec 3, 2020 at 10:16 PM Felipe Contreras
> <felipe.contreras@gmail.com> wrote:
> >
> > The current error is not user-friendly:
> >
> >   fatal: not possible to fast-forward, aborting.
> >
> > We want something that actually explains what is going on:
> >
> >   The pull was not fast-forward, please either merge or rebase.
> >   If unsure, run "git pull --merge".
>
> Sorry to be a broken record, but I don't like the suggestion in the
> second sentence.  I've said it enough times that I'll quit harping on
> each instance.

> > The user can get rid of the warning by doing either --merge or
> > --rebase.
> >
> > Except: doing "git pull --merge" is not actually enough; we would return
> > to the previous behavior: "fatal: not possible to fast-forward, aborting."
>
> Do you mean that the changes in this patch aren't enough and that
> future patches will address this shortcoming?  Or do you mean that
> prior to this patch such a bug existed?  Or something else?

> > Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> > ---
> >  builtin/pull.c  | 34 ++++++++++++++++-----------
> >  t/t5520-pull.sh | 62 +++++++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 82 insertions(+), 14 deletions(-)
> >
> > diff --git a/builtin/pull.c b/builtin/pull.c
> > index 6ea95c9fc9..f54ff36b57 100644
> > --- a/builtin/pull.c
> > +++ b/builtin/pull.c
> > @@ -1015,20 +1015,26 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
> >
> >         can_ff = get_can_ff(&orig_head, &merge_heads.oid[0]);
> >
> > -       if (default_mode && !can_ff && opt_verbosity >= 0 && !opt_ff) {
> > -               advise(_("Pulling without specifying how to reconcile divergent branches is\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 --merge\".\n"
> > -                       "Read \"git pull --help\" for more information."
> > -                       ));
> > +       if (!can_ff && default_mode) {
> > +               if (opt_ff && !strcmp(opt_ff, "--ff-only")) {
> > +                       die(_("The pull was not fast-forward, please either merge or rebase.\n"
> > +                               "If unsure, run \"git pull --merge\"."));
>
> This is a much better message for when the user requests --ff-only.

> > +               }
> > +               if (opt_verbosity >= 0 && !opt_ff) {
> > +                       advise(_("Pulling without specifying how to reconcile divergent branches is\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 --merge\".\n"
> > +                               "Read \"git pull --help\" for more information."
> > +                               ));
> > +               }
> >         }
> >
> >         if (opt_rebase) {
> > diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
> > index 9fae07cdfa..fdd1f79b06 100755
> > --- a/t/t5520-pull.sh
> > +++ b/t/t5520-pull.sh
> > @@ -819,4 +819,66 @@ test_expect_success 'git pull --rebase against local branch' '
> >         test_cmp expect file2
> >  '
> >
> > +test_expect_success 'git pull fast-forward (ff-only)' '
> > +       test_when_finished "git checkout master && git branch -D other test" &&
> > +       test_config pull.ff only &&
> > +       git checkout -b other master &&
> > +       >new &&
> > +       git add new &&
> > +       git commit -m new &&
> > +       git checkout -b test -t other &&
> > +       git reset --hard master &&
> > +       git pull
> > +'
> > +
> > +test_expect_success 'git pull non-fast-forward (ff-only)' '
> > +       test_when_finished "git checkout master && git branch -D other test" &&
> > +       test_config pull.ff only &&
> > +       git checkout -b other master^ &&
> > +       >new &&
> > +       git add new &&
> > +       git commit -m new &&
> > +       git checkout -b test -t other &&
> > +       git reset --hard master &&
> > +       test_must_fail git pull
> > +'
> > +
> > +test_expect_failure 'git pull non-fast-forward with merge (ff-only)' '
> > +       test_when_finished "git checkout master && git branch -D other test" &&
> > +       test_config pull.ff only &&
> > +       git checkout -b other master^ &&
> > +       >new &&
> > +       git add new &&
> > +       git commit -m new &&
> > +       git checkout -b test -t other &&
> > +       git reset --hard master &&
> > +       git pull --no-rebase
>
> Do you mean `git pull --merge`?

>
> It's unclear why --ff should remove the warning, since:
>
>   git pull --ff
>
> Is implicitly the same as:
>
>   git pull
>
> Unless of course pull.ff is specified otherwise.
>
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  builtin/pull.c               | 2 +-
>  t/t7601-merge-pull-config.sh | 4 ++--
>  2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/builtin/pull.c b/builtin/pull.c
> index ebf2ac687b..e389ffcdc3 100644
> --- a/builtin/pull.c
> +++ b/builtin/pull.c
> @@ -1020,7 +1020,7 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
>                         die(_("The pull was not fast-forward, please either merge or rebase.\n"
>                                 "If unsure, run \"git pull --merge\"."));
>                 }
> -               if (opt_verbosity >= 0 && !opt_ff) {
> +               if (opt_verbosity >= 0 && (!opt_ff || !strcmp(opt_ff, "--ff"))) {
>                         advise(_("Pulling without specifying how to reconcile divergent branches is\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"
> diff --git a/t/t7601-merge-pull-config.sh b/t/t7601-merge-pull-config.sh
> index 1de64e6cc5..d709799f8b 100755
> --- a/t/t7601-merge-pull-config.sh
> +++ b/t/t7601-merge-pull-config.sh
> @@ -46,7 +46,7 @@ test_expect_success 'pull.rebase not set and pull.ff=true' '
>         git reset --hard c2 &&
>         test_config pull.ff true &&
>         git pull . c1 2>err &&
> -       test_i18ngrep ! "Pulling without specifying how to reconcile" err
> +       test_i18ngrep "Pulling without specifying how to reconcile" err
>  '
>
>  test_expect_success 'pull.rebase not set and pull.ff=false' '
> @@ -78,7 +78,7 @@ test_expect_success 'pull.rebase not set and --merge given' '
>  test_expect_success 'pull.rebase not set and --ff given' '
>         git reset --hard c2 &&
>         git pull --ff . c1 2>err &&
> -       test_i18ngrep ! "Pulling without specifying how to reconcile" err
> +       test_i18ngrep "Pulling without specifying how to reconcile" err
>  '
>
>  test_expect_success 'pull.rebase not set and --no-ff given' '
> --
> 2.29.2

>
> On Thu, Dec 3, 2020 at 10:16 PM Felipe Contreras
> <felipe.contreras@gmail.com> wrote:
> >
> > It's unclear why --ff should remove the warning, since:
> >
> >   git pull --ff
> >
> > Is implicitly the same as:
> >
> >   git pull
> >
> > Unless of course pull.ff is specified otherwise.
> >
> > Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> > ---
> >  builtin/pull.c               | 2 +-
> >  t/t7601-merge-pull-config.sh | 4 ++--
> >  2 files changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/builtin/pull.c b/builtin/pull.c
> > index ebf2ac687b..e389ffcdc3 100644
> > --- a/builtin/pull.c
> > +++ b/builtin/pull.c
> > @@ -1020,7 +1020,7 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
> >                         die(_("The pull was not fast-forward, please either merge or rebase.\n"
> >                                 "If unsure, run \"git pull --merge\"."));
> >                 }
> > -               if (opt_verbosity >= 0 && !opt_ff) {
> > +               if (opt_verbosity >= 0 && (!opt_ff || !strcmp(opt_ff, "--ff"))) {
> >                         advise(_("Pulling without specifying how to reconcile divergent branches is\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"
> > diff --git a/t/t7601-merge-pull-config.sh b/t/t7601-merge-pull-config.sh
> > index 1de64e6cc5..d709799f8b 100755
> > --- a/t/t7601-merge-pull-config.sh
> > +++ b/t/t7601-merge-pull-config.sh
> > @@ -46,7 +46,7 @@ test_expect_success 'pull.rebase not set and pull.ff=true' '
> >         git reset --hard c2 &&
> >         test_config pull.ff true &&
> >         git pull . c1 2>err &&
> > -       test_i18ngrep ! "Pulling without specifying how to reconcile" err
> > +       test_i18ngrep "Pulling without specifying how to reconcile" err
> >  '
> >
> >  test_expect_success 'pull.rebase not set and pull.ff=false' '
> > @@ -78,7 +78,7 @@ test_expect_success 'pull.rebase not set and --merge given' '
> >  test_expect_success 'pull.rebase not set and --ff given' '
> >         git reset --hard c2 &&
> >         git pull --ff . c1 2>err &&
> > -       test_i18ngrep ! "Pulling without specifying how to reconcile" err
> > +       test_i18ngrep "Pulling without specifying how to reconcile" err
> >  '
> >
> >  test_expect_success 'pull.rebase not set and --no-ff given' '
> > --
> > 2.29.2
>
> Ah, this answers one of my earlier questions.  I'd be tempted to move
> this earlier in the series; it seems like an obvious cleanup and helps
> explain the "opt_ff" flag that keeps coming up for readers of the
> series.

>
> Cc: Alex Henrie <alexhenrie24@gmail.com>
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  t/t5521-pull-options.sh | 22 +++++++++++-----------
>  1 file changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/t/t5521-pull-options.sh b/t/t5521-pull-options.sh
> index db1a381cd9..1a4fe2583a 100755
> --- a/t/t5521-pull-options.sh
> +++ b/t/t5521-pull-options.sh
> @@ -11,10 +11,10 @@ test_expect_success 'setup' '
>          git commit -m one)
>  '
>
> -test_expect_success 'git pull -q --no-rebase' '
> +test_expect_success 'git pull -q' '
>         mkdir clonedq &&
>         (cd clonedq && git init &&
> -       git pull -q --no-rebase "../parent" >out 2>err &&
> +       git pull -q "../parent" >out 2>err &&
>         test_must_be_empty err &&
>         test_must_be_empty out)
>  '
> @@ -30,10 +30,10 @@ test_expect_success 'git pull -q --rebase' '
>         test_must_be_empty out)
>  '
>
> -test_expect_success 'git pull --no-rebase' '
> +test_expect_success 'git pull' '
>         mkdir cloned &&
>         (cd cloned && git init &&
> -       git pull --no-rebase "../parent" >out 2>err &&
> +       git pull "../parent" >out 2>err &&
>         test -s err &&
>         test_must_be_empty out)
>  '
> @@ -46,10 +46,10 @@ test_expect_success 'git pull --rebase' '
>         test_must_be_empty out)
>  '
>
> -test_expect_success 'git pull -v --no-rebase' '
> +test_expect_success 'git pull -v' '
>         mkdir clonedv &&
>         (cd clonedv && git init &&
> -       git pull -v --no-rebase "../parent" >out 2>err &&
> +       git pull -v "../parent" >out 2>err &&
>         test -s err &&
>         test_must_be_empty out)
>  '
> @@ -62,25 +62,25 @@ test_expect_success 'git pull -v --rebase' '
>         test_must_be_empty out)
>  '
>
> -test_expect_success 'git pull -v -q --no-rebase' '
> +test_expect_success 'git pull -v -q' '
>         mkdir clonedvq &&
>         (cd clonedvq && git init &&
> -       git pull -v -q --no-rebase "../parent" >out 2>err &&
> +       git pull -v -q "../parent" >out 2>err &&
>         test_must_be_empty out &&
>         test_must_be_empty err)
>  '
>
> -test_expect_success 'git pull -q -v --no-rebase' '
> +test_expect_success 'git pull -q -v' '
>         mkdir clonedqv &&
>         (cd clonedqv && git init &&
> -       git pull -q -v --no-rebase "../parent" >out 2>err &&
> +       git pull -q -v "../parent" >out 2>err &&
>         test_must_be_empty out &&
>         test -s err)
>  '
>  test_expect_success 'git pull --cleanup errors early on invalid argument' '
>         mkdir clonedcleanup &&
>         (cd clonedcleanup && git init &&
> -       test_must_fail git pull --no-rebase --cleanup invalid "../parent" >out 2>err &&
> +       test_must_fail git pull --cleanup invalid "../parent" >out 2>err &&
>         test_must_be_empty out &&
>         test -s err)
>  '
> --
> 2.29.2
>

>
> On Thu, Dec 3, 2020 at 10:16 PM Felipe Contreras
> <felipe.contreras@gmail.com> wrote:
> >
> > Commit d18c950a69 changed these tests, but it's unclear why. Probably
> > because earlier versions of the patch series died instead of printing a
> > warning.
>
> Another case where log --pretty=reference notation would be more
> helpful in referring to commits.
>
> Looking up that commit, I see that commit made a number of other test
> changes which seem to be better motivated by the commit message and
> code changes.  I wonder if we could make that clearer in the commit
> message somehow.  Perhaps
>
> Commit d18c950a69 ("pull: warn if the user didn't say whether to
> rebase or to merge", 2020-03-09) changed a number of tests in t5521
> and added some new tests in t7601, but it is not clear why the changes
> in t5521 were made...

>
> Commit e01ae2a4a7 introduced an extra space.
>
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  t/t7601-merge-pull-config.sh | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/t/t7601-merge-pull-config.sh b/t/t7601-merge-pull-config.sh
> index d709799f8b..8a6aae564a 100755
> --- a/t/t7601-merge-pull-config.sh
> +++ b/t/t7601-merge-pull-config.sh
> @@ -33,7 +33,6 @@ test_expect_success 'pull.rebase not set' '
>         test_decode_color <err >decoded &&
>         test_i18ngrep "<YELLOW>hint: " decoded &&
>         test_i18ngrep "Pulling without specifying how to reconcile" decoded
> -
>  '
>
>  test_expect_success 'pull.rebase not set (fast-forward)' '
> --
> 2.29.2

> Currently --rebase already ignores --ff-only (or any other --ff option),
> but --merge fails.
>
> Make it so --ff-only is only considered in the default mode.
>
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  builtin/pull.c  | 4 ++++
>  t/t5520-pull.sh | 2 +-
>  2 files changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/pull.c b/builtin/pull.c
> index f54ff36b57..ebf2ac687b 100644
> --- a/builtin/pull.c
> +++ b/builtin/pull.c
> @@ -1037,6 +1037,10 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
>                 }
>         }
>
> +       /* Disable --ff-only when --merge is specified */
> +       if (!can_ff && !default_mode && !opt_rebase && opt_ff && !strcmp(opt_ff, "--ff-only"))
> +               opt_ff = NULL;
> +
>         if (opt_rebase) {
>                 int ret = 0;
>                 if ((recurse_submodules == RECURSE_SUBMODULES_ON ||
> diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
> index fdd1f79b06..eec6224fb0 100755
> --- a/t/t5520-pull.sh
> +++ b/t/t5520-pull.sh
> @@ -843,7 +843,7 @@ test_expect_success 'git pull non-fast-forward (ff-only)' '
>         test_must_fail git pull
>  '
>
> -test_expect_failure 'git pull non-fast-forward with merge (ff-only)' '
> +test_expect_success 'git pull non-fast-forward with merge (ff-only)' '
>         test_when_finished "git checkout master && git branch -D other test" &&
>         test_config pull.ff only &&
>         git checkout -b other master^ &&
> --
> 2.29.2
>

>
> On Thu, Dec 3, 2020 at 10:16 PM Felipe Contreras
> <felipe.contreras@gmail.com> wrote:
> >
> > We want --ff-only to make sense only when no --merge or --rebase option
> > is specified.
>
> A lot of git commands have opposite options, and git allows them both
> to be specified with the last one winning.  Thus, much like
>   git log --patch --no-patch
> mean show logs without patches and
>   git log --no-patch --patch
> means show logs with patches, I would similarly expect the following
> two commands to have opposite behavior:
>   git pull --merge --no-ff
>   git pull --no-ff --merge

> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  builtin/pull.c  | 40 ++++++++++++++++++++++++++--------------
>  t/t5520-pull.sh | 23 +++++++++++++++++++++++
>  2 files changed, 49 insertions(+), 14 deletions(-)
>
> diff --git a/builtin/pull.c b/builtin/pull.c
> index e389ffcdc3..95ecbdaad5 100644
> --- a/builtin/pull.c
> +++ b/builtin/pull.c
> @@ -62,6 +62,7 @@ static int parse_opt_rebase(const struct option *opt, const char *arg, int unset
>                 *value = parse_config_rebase("--rebase", arg, 0);
>         else
>                 *value = unset ? REBASE_FALSE : REBASE_TRUE;
> +       if (*value != REBASE_INVALID) default_mode = 0;
>         return *value == REBASE_INVALID ? -1 : 0;
>  }
>
> @@ -114,6 +115,23 @@ static int opt_show_forced_updates = -1;
>  static char *set_upstream;
>  static struct strvec opt_fetch = STRVEC_INIT;
>
> +static int parse_opt_ff_only(const struct option *opt, const char *arg, int unset)
> +{
> +       char **value = opt->value;
> +       opt_rebase = REBASE_FALSE;
> +       *value = "--ff-only";
> +       return 0;
> +}
> +
> +static int parse_opt_merge(const struct option *opt, const char *arg, int unset)
> +{
> +       char **value = opt->value;
> +       opt_ff = NULL;
> +       *value = REBASE_FALSE;
> +       default_mode = 0;
> +       return 0;
> +}
> +
>  static struct option pull_options[] = {
>         /* Shared options */
>         OPT__VERBOSITY(&opt_verbosity),
> @@ -131,8 +149,9 @@ static struct option pull_options[] = {
>                 "(false|true|merges|preserve|interactive)",
>                 N_("incorporate changes by rebasing rather than merging"),
>                 PARSE_OPT_OPTARG, parse_opt_rebase),
> -       OPT_SET_INT('m', "merge", &opt_rebase,
> -               N_("incorporate changes by merging"), 0),
> +       OPT_CALLBACK_F('m', "merge", &opt_rebase, NULL,
> +               N_("incorporate changes by merging"),
> +               PARSE_OPT_NOARG | PARSE_OPT_NONEG, parse_opt_merge),
>         OPT_PASSTHRU('n', NULL, &opt_diffstat, NULL,
>                 N_("do not show a diffstat at the end of the merge"),
>                 PARSE_OPT_NOARG | PARSE_OPT_NONEG),
> @@ -161,9 +180,9 @@ static struct option pull_options[] = {
>         OPT_PASSTHRU(0, "ff", &opt_ff, NULL,
>                 N_("allow fast-forward"),
>                 PARSE_OPT_NOARG),
> -       OPT_PASSTHRU(0, "ff-only", &opt_ff, NULL,
> +       OPT_CALLBACK_F(0, "ff-only", &opt_ff, NULL,
>                 N_("abort if fast-forward is not possible"),
> -               PARSE_OPT_NOARG | PARSE_OPT_NONEG),
> +               PARSE_OPT_NOARG | PARSE_OPT_NONEG, parse_opt_ff_only),
>         OPT_PASSTHRU(0, "verify-signatures", &opt_verify_signatures, NULL,
>                 N_("verify that the named commit has a valid GPG signature"),
>                 PARSE_OPT_NOARG),
> @@ -924,6 +943,9 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
>
>         git_config(git_pull_config, NULL);
>
> +       opt_ff = xstrdup_or_null(config_get_ff());
> +       opt_rebase = config_get_rebase();
> +
>         argc = parse_options(argc, argv, prefix, pull_options, pull_usage, 0);
>
>         if (cleanup_arg)
> @@ -935,12 +957,6 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
>
>         parse_repo_refspecs(argc, argv, &repo, &refspecs);
>
> -       if (!opt_ff)
> -               opt_ff = xstrdup_or_null(config_get_ff());
> -
> -       if (opt_rebase < 0)
> -               opt_rebase = config_get_rebase();
> -
>         if (read_cache_unmerged())
>                 die_resolve_conflict("pull");
>
> @@ -1037,10 +1053,6 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
>                 }
>         }
>
> -       /* Disable --ff-only when --merge is specified */
> -       if (!can_ff && !default_mode && !opt_rebase && opt_ff && !strcmp(opt_ff, "--ff-only"))
> -               opt_ff = NULL;
> -
>         if (opt_rebase) {
>                 int ret = 0;
>                 if ((recurse_submodules == RECURSE_SUBMODULES_ON ||
> diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
> index f48d0f8d50..c0cfde54e1 100755
> --- a/t/t5520-pull.sh
> +++ b/t/t5520-pull.sh
> @@ -881,4 +881,27 @@ test_expect_success 'git pull non-fast-forward error message' '
>         grep -q "The pull was not fast-forward" error
>  '
>
> +test_expect_success 'git pull --merge overrides --ffonly' '

> +       test_when_finished "git checkout master && git branch -D other test" &&
> +       test_config pull.ff only &&
> +       git checkout -b other master^ &&
> +       >new &&
> +       git add new &&
> +       git commit -m new &&
> +       git checkout -b test -t other &&
> +       git reset --hard master &&
> +       git pull --ff-only --merge
> +'
> +
> +test_expect_success 'git pull --ff-only overrides --merge' '
> +       test_when_finished "git checkout master && git branch -D other test" &&
> +       git checkout -b other master^ &&
> +       >new &&
> +       git add new &&
> +       git commit -m new &&
> +       git checkout -b test -t other &&
> +       git reset --hard master &&
> +       test_must_fail git pull --merge --ff-only
> +'

>
> On Fri, Dec 4, 2020 at 8:06 PM Felipe Contreras
> <felipe.contreras@gmail.com> wrote:
>
> This commit message should say something more than just "change x", it
> should have some words about what it is change from or to.  I think
> the thrust of the patch is allowing --ff-only, --merge, and --rebase
> to countermand an earlier otherwise-conflicting option.  Perhaps a
> commit message of the form:

> On Fri, Dec 4, 2020 at 8:06 PM Felipe Contreras
> <felipe.contreras@gmail.com> wrote:
>
> This commit message should say something more than just "change x", it
> should have some words about what it is change from or to.  I think
> the thrust of the patch is allowing --ff-only, --merge, and --rebase
> to countermand an earlier otherwise-conflicting option.  Perhaps a
> commit message of the form:
>
> In git, we sometimes allow conflicting command line options with the
> last one winning, e.g.
>   git log --patch --no-patch
>   git log --no-patch --patch
> other times we just error out when conflicting options are given, e.g.
>   git checkout -b --orphan NEWBRANCH
>   git checkout --orphan -b NEWBRANCH
> Previously, we did neither with --no-ff, --merge, and --rebase.
> Change these options to have a last-one-wins behavior.

>
> On Fri, Dec 4, 2020 at 5:39 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 --ff-only to make sense only when no --merge or --rebase option
> > > is specified.
> >
> > A lot of git commands have opposite options, and git allows them both
> > to be specified with the last one winning.  Thus, much like
> >   git log --patch --no-patch
> > mean show logs without patches and
> >   git log --no-patch --patch
> > means show logs with patches, I would similarly expect the following
> > two commands to have opposite behavior:
> >   git pull --merge --no-ff
> >   git pull --no-ff --merge
>
> Good point.
>
> Although I presume you meant --ff-only.
>
> Taking that into consideration it may be possible to make --ff-only
> work straightforwardly.
>
> Further changes to the code are needed though. In the meantime I'm
> sending a quick patch on top of this series.

> This requires the tests to pick another base, so the merge is not
> fast-forward.
>
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  Documentation/git-pull.txt   |  3 +++
>  builtin/pull.c               |  2 +-
>  t/t7601-merge-pull-config.sh | 28 +++++++++++++++++-----------
>  3 files changed, 21 insertions(+), 12 deletions(-)
>
> diff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt
> index dc812139f4..ad33d2472c 100644
> --- a/Documentation/git-pull.txt
> +++ b/Documentation/git-pull.txt
> @@ -60,6 +60,9 @@ However, a non-fast-foward case looks very different.
>         origin/master in your repository
>  ------------
>
> +By default `git pull` will warn about these situations, however, most likely
> +you would want to force a merge, which you can do with `git pull --no-rebase`.
> +

>  Then "`git pull`" will fetch and replay the changes from the remote
>  `master` branch since it diverged from the local `master` (i.e., `E`)
>  until its current commit (`C`) on top of `master` and record the
> diff --git a/builtin/pull.c b/builtin/pull.c
> index f82e214fc8..f0b1c6bfea 100644
> --- a/builtin/pull.c
> +++ b/builtin/pull.c
> @@ -1013,7 +1013,7 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
>
>         can_ff = get_can_ff(&orig_head, &merge_heads.oid[0]);
>
> -       if (default_mode && opt_verbosity >= 0 && !opt_ff) {
> +       if (default_mode && !can_ff && opt_verbosity >= 0 && !opt_ff) {
>                 advise(_("Pulling without specifying how to reconcile divergent branches is\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"
> diff --git a/t/t7601-merge-pull-config.sh b/t/t7601-merge-pull-config.sh
> index 6774e9d86f..6b4adab8b1 100755
> --- a/t/t7601-merge-pull-config.sh
> +++ b/t/t7601-merge-pull-config.sh
> @@ -28,7 +28,7 @@ test_expect_success 'setup' '
>  '
>
>  test_expect_success 'pull.rebase not set' '
> -       git reset --hard c0 &&
> +       git reset --hard c2 &&
>         git -c color.advice=always pull . c1 2>err &&
>         test_decode_color <err >decoded &&
>         test_i18ngrep "<YELLOW>hint: " decoded &&
> @@ -36,54 +36,60 @@ test_expect_success 'pull.rebase not set' '
>
>  '
>
> -test_expect_success 'pull.rebase not set and pull.ff=true' '
> +test_expect_success 'pull.rebase not set (fast-forward)' '
>         git reset --hard c0 &&
> +       git pull . c1 2>err &&
> +       test_i18ngrep ! "Pulling without specifying how to reconcile" err
> +'
> +
> +test_expect_success 'pull.rebase not set and pull.ff=true' '
> +       git reset --hard c2 &&
>         test_config pull.ff true &&
>         git pull . c1 2>err &&
>         test_i18ngrep ! "Pulling without specifying how to reconcile" err
>  '
>
>  test_expect_success 'pull.rebase not set and pull.ff=false' '
> -       git reset --hard c0 &&
> +       git reset --hard c2 &&
>         test_config pull.ff false &&
>         git pull . c1 2>err &&
>         test_i18ngrep ! "Pulling without specifying how to reconcile" err
>  '
>
>  test_expect_success 'pull.rebase not set and pull.ff=only' '
> -       git reset --hard c0 &&
> +       git reset --hard c2 &&
>         test_config pull.ff only &&
> -       git pull . c1 2>err &&
> +       test_must_fail git pull . c1 2>err &&

>         test_i18ngrep ! "Pulling without specifying how to reconcile" err
>  '
>
>  test_expect_success 'pull.rebase not set and --rebase given' '
> -       git reset --hard c0 &&
> +       git reset --hard c2 &&
>         git pull --rebase . c1 2>err &&
>         test_i18ngrep ! "Pulling without specifying how to reconcile" err
>  '
>
>  test_expect_success 'pull.rebase not set and --no-rebase given' '
> -       git reset --hard c0 &&
> +       git reset --hard c2 &&
>         git pull --no-rebase . c1 2>err &&
>         test_i18ngrep ! "Pulling without specifying how to reconcile" err
>  '
>
>  test_expect_success 'pull.rebase not set and --ff given' '
> -       git reset --hard c0 &&
> +       git reset --hard c2 &&
>         git pull --ff . c1 2>err &&
>         test_i18ngrep ! "Pulling without specifying how to reconcile" err
>  '
>
>  test_expect_success 'pull.rebase not set and --no-ff given' '
> -       git reset --hard c0 &&
> +       git reset --hard c2 &&
>         git pull --no-ff . c1 2>err &&
>         test_i18ngrep ! "Pulling without specifying how to reconcile" err
>  '
>
>  test_expect_success 'pull.rebase not set and --ff-only given' '
> -       git reset --hard c0 &&
> -       git pull --ff-only . c1 2>err &&
> +       git reset --hard c2 &&
> +       test_must_fail git pull --ff-only . c1 2>err &&

>         test_i18ngrep ! "Pulling without specifying how to reconcile" err
>  '
>
> --
> 2.29.2

>
> On Thu, Dec 3, 2020 at 10:16 PM Felipe Contreras
> <felipe.contreras@gmail.com> wrote:
> >
> > There's no need to display the annoying warning on every pull... only
> > the ones that are not fast-forward.
>
> I like this.  :-)
>
> > This requires the tests to pick another base, so the merge is not
> > fast-forward.
> >
> > Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> > ---
> >  Documentation/git-pull.txt   |  3 +++
> >  builtin/pull.c               |  2 +-
> >  t/t7601-merge-pull-config.sh | 28 +++++++++++++++++-----------
> >  3 files changed, 21 insertions(+), 12 deletions(-)
> >
> > diff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt
> > index dc812139f4..ad33d2472c 100644
> > --- a/Documentation/git-pull.txt
> > +++ b/Documentation/git-pull.txt
> > @@ -60,6 +60,9 @@ However, a non-fast-foward case looks very different.
> >         origin/master in your repository
> >  ------------
> >
> > +By default `git pull` will warn about these situations, however, most likely
> > +you would want to force a merge, which you can do with `git pull --no-rebase`.
> > +
>
> Everything in this sentence after the first comma seems dangerous to
> me for a number of workflows.

> >  Then "`git pull`" will fetch and replay the changes from the remote
> >  `master` branch since it diverged from the local `master` (i.e., `E`)
> >  until its current commit (`C`) on top of `master` and record the
> > diff --git a/builtin/pull.c b/builtin/pull.c
> > index f82e214fc8..f0b1c6bfea 100644
> > --- a/builtin/pull.c
> > +++ b/builtin/pull.c
> > @@ -1013,7 +1013,7 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
> >
> >         can_ff = get_can_ff(&orig_head, &merge_heads.oid[0]);
> >
> > -       if (default_mode && opt_verbosity >= 0 && !opt_ff) {
> > +       if (default_mode && !can_ff && opt_verbosity >= 0 && !opt_ff) {
> >                 advise(_("Pulling without specifying how to reconcile divergent branches is\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"
> > diff --git a/t/t7601-merge-pull-config.sh b/t/t7601-merge-pull-config.sh
> > index 6774e9d86f..6b4adab8b1 100755
> > --- a/t/t7601-merge-pull-config.sh
> > +++ b/t/t7601-merge-pull-config.sh
> > @@ -28,7 +28,7 @@ test_expect_success 'setup' '
> >  '
> >
> >  test_expect_success 'pull.rebase not set' '
> > -       git reset --hard c0 &&
> > +       git reset --hard c2 &&
> >         git -c color.advice=always pull . c1 2>err &&
> >         test_decode_color <err >decoded &&
> >         test_i18ngrep "<YELLOW>hint: " decoded &&
> > @@ -36,54 +36,60 @@ test_expect_success 'pull.rebase not set' '
> >
> >  '
> >
> > -test_expect_success 'pull.rebase not set and pull.ff=true' '
> > +test_expect_success 'pull.rebase not set (fast-forward)' '
> >         git reset --hard c0 &&
> > +       git pull . c1 2>err &&
> > +       test_i18ngrep ! "Pulling without specifying how to reconcile" err
> > +'
> > +
> > +test_expect_success 'pull.rebase not set and pull.ff=true' '
> > +       git reset --hard c2 &&
> >         test_config pull.ff true &&
> >         git pull . c1 2>err &&
> >         test_i18ngrep ! "Pulling without specifying how to reconcile" err
> >  '
> >
> >  test_expect_success 'pull.rebase not set and pull.ff=false' '
> > -       git reset --hard c0 &&
> > +       git reset --hard c2 &&
> >         test_config pull.ff false &&
> >         git pull . c1 2>err &&
> >         test_i18ngrep ! "Pulling without specifying how to reconcile" err
> >  '
> >
> >  test_expect_success 'pull.rebase not set and pull.ff=only' '
> > -       git reset --hard c0 &&
> > +       git reset --hard c2 &&
> >         test_config pull.ff only &&
> > -       git pull . c1 2>err &&
> > +       test_must_fail git pull . c1 2>err &&
>
> It makes sense to me that you'd need test_must_fail here...but why
> wasn't it needed before??  How did this test pass without it before
> this series?

>
> Previously --no-rebase (which still works for backwards compatbility).
>
> Now we can update the default warning, and the git-pull(1) man page to
> use --merge instead of the non-intuitive --no-rebase.
>
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  Documentation/git-pull.txt   | 9 ++++++---
>  builtin/pull.c               | 4 +++-
>  t/t7601-merge-pull-config.sh | 4 ++--
>  3 files changed, 11 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt
> index ad33d2472c..c220da143a 100644
> --- a/Documentation/git-pull.txt
> +++ b/Documentation/git-pull.txt
> @@ -61,7 +61,7 @@ However, a non-fast-foward case looks very different.
>  ------------
>
>  By default `git pull` will warn about these situations, however, most likely
> -you would want to force a merge, which you can do with `git pull --no-rebase`.
> +you would want to force a merge, which you can do with `git pull --merge`.

>
>  Then "`git pull`" will fetch and replay the changes from the remote
>  `master` branch since it diverged from the local `master` (i.e., `E`)
> @@ -148,8 +148,11 @@ It rewrites history, which does not bode well when you
>  published that history already.  Do *not* use this option
>  unless you have read linkgit:git-rebase[1] carefully.
>
> ---no-rebase::
> -       Override earlier --rebase.
> +-m::
> +--merge::
> +       Force a merge.
> ++
> +Previously this was --no-rebase, but that usage has been deprecated.
>
>  Options related to fetching
>  ~~~~~~~~~~~~~~~~~~~~~~~~~~~
> diff --git a/builtin/pull.c b/builtin/pull.c
> index b200f7544c..6ea95c9fc9 100644
> --- a/builtin/pull.c
> +++ b/builtin/pull.c
> @@ -131,6 +131,8 @@ static struct option pull_options[] = {
>                 "(false|true|merges|preserve|interactive)",
>                 N_("incorporate changes by rebasing rather than merging"),
>                 PARSE_OPT_OPTARG, parse_opt_rebase),
> +       OPT_SET_INT('m', "merge", &opt_rebase,
> +               N_("incorporate changes by merging"), 0),
>         OPT_PASSTHRU('n', NULL, &opt_diffstat, NULL,
>                 N_("do not show a diffstat at the end of the merge"),
>                 PARSE_OPT_NOARG | PARSE_OPT_NONEG),
> @@ -1024,7 +1026,7 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
>                         "\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"
> +                       "If unsure, run \"git pull --merge\".\n"

>                         "Read \"git pull --help\" for more information."
>                         ));
>         }
> diff --git a/t/t7601-merge-pull-config.sh b/t/t7601-merge-pull-config.sh
> index 6b4adab8b1..1de64e6cc5 100755
> --- a/t/t7601-merge-pull-config.sh
> +++ b/t/t7601-merge-pull-config.sh
> @@ -69,9 +69,9 @@ test_expect_success 'pull.rebase not set and --rebase given' '
>         test_i18ngrep ! "Pulling without specifying how to reconcile" err
>  '
>
> -test_expect_success 'pull.rebase not set and --no-rebase given' '
> +test_expect_success 'pull.rebase not set and --merge given' '
>         git reset --hard c2 &&
> -       git pull --no-rebase . c1 2>err &&
> +       git pull --merge . c1 2>err &&
>         test_i18ngrep ! "Pulling without specifying how to reconcile" err
>  '
>
> --
> 2.29.2

>
> On Thu, Dec 3, 2020 at 10:16 PM Felipe Contreras
> <felipe.contreras@gmail.com> wrote:
> >
> > Previously --no-rebase (which still works for backwards compatbility).
> >
> > Now we can update the default warning, and the git-pull(1) man page to
> > use --merge instead of the non-intuitive --no-rebase.
> >
> > Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> > ---
> >  Documentation/git-pull.txt   | 9 ++++++---
> >  builtin/pull.c               | 4 +++-
> >  t/t7601-merge-pull-config.sh | 4 ++--
> >  3 files changed, 11 insertions(+), 6 deletions(-)
> >
> > diff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt
> > index ad33d2472c..c220da143a 100644
> > --- a/Documentation/git-pull.txt
> > +++ b/Documentation/git-pull.txt
> > @@ -61,7 +61,7 @@ However, a non-fast-foward case looks very different.
> >  ------------
> >
> >  By default `git pull` will warn about these situations, however, most likely
> > -you would want to force a merge, which you can do with `git pull --no-rebase`.
> > +you would want to force a merge, which you can do with `git pull --merge`.
>
> I'm glad you updated all the references, but as noted earlier in the
> series I think this suggestion is problematic.