Re: [PATCH v2 10/14] pull: add proper error with --ff-only

[Felipe Contreras <felipe.contreras@gmail.com>]

On Fri, Dec 4, 2020 at 5:34 PM Elijah Newren <newren@gmail.com> wrote:
>
> 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.

In this particular case it does make sense to remove the suggestion,
since it has nothing to do with the current "git pull" default.

The user must decide in this case.

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

I mean this is the shortcoming of using the --ff-only approach, and
why I think a "pull.mode=ff-only" must be introduced.

This patch does as much as it can without changing the semantics of
--ff-only, and it shows the limitations of the approach, as you can
see from the failed test below, and the next patch.

It's inherent to the current behavior of --ff-only.

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

Right. Sans the "git pull --merge" comment :p

> > +               }
> > +               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`?

Yes. You found all the issues I spotted in my last review after I sent
the patches.

Good eyes.

Already fixed.

Cheers.

-- 
Felipe Contreras