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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Jul 15, 2021, 05:18 UTC
Message-ID: <CAPig+cQUQYrkH7YsFio=JBZnaM3FRumti6OtvOTu6cUo4kWKtg@mail.gmail.com>
In-Reply-To: <7e12c45fc9a94e7b56a6efdc085ebe081dd40afc.1626316849.git.gitgitgadget@gmail.com>

On Wed, Jul 14, 2021 at 10:41 PM Elijah Newren via GitGitGadget <gitgitgadget@gmail.com> wrote:

Show 5 quoted lines

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

Show 10 quoted lines

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

Show 10 quoted lines

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

Show 11 quoted lines

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