Re: [PATCH 5/5] pull: abort by default when fast-forwarding is not possible

[Elijah Newren <newren@gmail.com>]

On Wed, Jul 14, 2021 at 10:19 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
>
> On Wed, Jul 14, 2021 at 10:41 PM Elijah Newren via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
> > We have for some time shown a long warning when the user does not
> > specify how to reconcile divergent branches with git pull.  Make it an
> > error now.
> >
> > Signed-off-by: Elijah Newren <newren@gmail.com>
>
> Teeny tiny nits below...
>
> > diff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt
> > @@ -15,14 +15,16 @@ SYNOPSIS
> > -Incorporates changes from a remote repository into the current
> > -branch.  In its default mode, `git pull` is shorthand for
> > -`git fetch` followed by `git merge FETCH_HEAD`.
> > -
> > -More precisely, 'git pull' runs 'git fetch' with the given
> > -parameters and calls 'git merge' to merge the retrieved branch
> > -heads into the current branch.
> > -With `--rebase`, it runs 'git rebase' instead of 'git merge'.
>
> The original text has an odd mix of apostrophes and backticks...
>
> > +Incorporates changes from a remote repository into the current branch.
> > +If the current branch is behind the remote, then by default it will
> > +fast-forward the current branch to match the remote.  If the current
> > +branch and the remote have diverged, the user needs to specify how to
> > +reconcile the divergent branches with --no-ff or --rebase (or the
> > +corresponding configuration options in pull.ff or pull.rebase).
> > +
> > +More precisely, 'git pull' runs 'git fetch' with the given parameters
> > +and then depending on config options or command line flags, will call
> > +either 'git merge' or 'git rebase' to reconcile diverging branches.
>
> ... and the revised text adds "no quotes" to the selection. These
> days, we'd probably backtick all of these:
>
>     --no-ff
>     --rebase
>     pull.ff
>     pull.rebase
>     git pull
>     git fetch
>     git merge
>     git rebase
>
> The rest of this document is actually pretty good about using
> backticks, though there are some exceptions.
>
> There's also an odd mix of "configuration options" and "config
> options" in the revised text. Perhaps stick with "configuration
> options" to be a bit more formal?
>
> >  <repository> should be the name of a remote repository as
> >  passed to linkgit:git-fetch[1].  <refspec> can name an
>
> And, as an aside, we'd backtick <repository> and <refspec>, though
> your patch isn't touching that, so outside the scope of this change.

I'll try to fix these up, at least those in the nearby area I'm
modifying, if others agree with the general approach towards --ff-only
& --no-ff vs. --rebase.

> > diff --git a/builtin/pull.c b/builtin/pull.c
> > @@ -1074,9 +1074,8 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
> >                 if (opt_ff) {
> >                         if (!strcmp(opt_ff, "--ff-only"))
> >                                 die_ff_impossible();
> > -               } else {
> > -                       if (rebase_unspecified && opt_verbosity >= 0)
> > -                               show_advice_pull_non_ff();
> > +               } else if (rebase_unspecified) {
> > +                       die_pull_non_ff();
> >                 }
>
> When reading the previous patch, I was wondering why an `if else`
> wasn't used, and here I see that it does become an `if else`. I guess
> you didn't want to alter Alex's original patch?

Yep.  :-)