Handle conflicting pull options

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 <[email protected]>

We've recently discussed how to handle conflicting configuration and command line options in git pull, including at least a few different proposals[1][2][3] to handle different subsets of the possibilities. We also have a user report from someone who had conflicting configuration and got surprised when one of the options overruled the other -- with no warning from the command or the documentation that such would happen.

From: Elijah Newren <[email protected]>

The --rebase[=...] flags and the various ff flags are incompatible, except that --no-rebase (or --rebase=false) work with any of the ff flags, and --ff works with any of the rebase flags.

From: Elijah Newren <[email protected]>

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.

Quoted 5 lines · Elijah Newren via GitGitGadget

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 <[email protected]>

Teeny tiny nits below...

Documentation/git-pull.txt SYNOPSIS · line 22

Quoted 10 lines

Documentation/git-pull.txt +0 -8

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

Documentation/git-pull.txt SYNOPSIS · line 27

Quoted 10 lines

Documentation/git-pull.txt +10 -0

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

Documentation/git-pull.txt SYNOPSIS · line 30

Quoted 2 lines

Documentation/git-pull.txt +0 -0

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

Quoted 7 lines

builtin/pull.c +0 -2

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

builtin/pull.c int cmd_pull(int argc, const char **argv, const char *prefix) · line 1079

Quoted 4 lines

builtin/pull.c +2 -1

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

t/t7601-merge-pull-config.sh test_expect_success 'pull.rebase not set and --ff-only given (not-fast-forward)' · line 211

Quoted 13 lines · Elijah Newren via GitGitGadget

The --rebase[=...] flags and the various ff flags are incompatible, except that --no-rebase (or --rebase=false) work with any of the ff flags, and --ff works with any of the rebase flags.

[...]

Signed-off-by Elijah Newren <[email protected]>

---

t/t7601-merge-pull-config.sh +6 -0

+test_expect_success 'report conflicting configuration' '
+       git reset --hard c2 &&
+       test_must_fail git -c pull.ff=false -c pull.rebase=true pull . c1 2>err &&
+       test_i18ngrep "pull.rebase and pull.ff are incompatible; please unset one" err
+
+'

nit: unnecessary and inconsistent blank line at end of test

Documentation/config/pull.txt pull.ff:: · line 16

Quoted 9 lines · Elijah Newren via GitGitGadget

Documentation/config/pull.txt +1 -1

 
 pull.rebase::
 	When true, rebase branches on top of the fetched branch, instead
 	of merging the default branch from the default remote when "git
 	pull" is run. See "branch.<name>.rebase" for setting this on a
-	per-branch basis.
+	per-branch basis.  Incompatible with pull.ff.

Likewise.

I think it technically is OK to say "only ff update is allowed" or "ff update is allowed when able" while saying pull.rebase=yes.

- For those who say ff=only, the actual value of pull.rebase would not matter (i.e. the integration immediately finishes by updating to what was obtained from the upstream because there is no new development on our own on top of theirs to either replay or merge).

- For those who merely allow ff, an update may fast-forward even when pull.rebase is set to true (or any non-false value), while we'll replay our own development on top of theirs when their history is not descendent of ours.

So I do not see need to declare these combinations "incompatible". But perhaps I am missing some subtle cases?

Documentation/config/pull.txt pull.ff:: · line 10

Quoted 9 lines · Elijah Newren via GitGitGadget

Documentation/config/pull.txt +1 -0

index 54048306095..e70ed99e408 100644
@@ -7,12 +7,13 @@ pull.ff::
      line). When set to `only`, only such fast-forward merges are
      allowed (equivalent to giving the `--ff-only` option from the
      command line). This setting overrides `merge.ff` when pulling.
+     Incompatible with pull.rebase.

This update may mean well, but I sense that the description needs to be tightened up a bit more. Unless you mean to say that I am not allowed to say "[pull] rebase=no" when I set "[pull] ff", that is.

Or do you mean pull.ff=only is incompatible with any setting of pull.rebase?

Or do you mean pull.ff=only is incompatible with any setting of pull.rebase other than false?

Or do you mean any setting of pull.ff is imcompatible with any setting of pull.rebase other than false?

(You do not have to answer---the above questions just demonstrate that the description is unnecessarily loose).

Documentation/config/pull.txt pull.ff:: · line 16

Quoted 6 lines

Documentation/config/pull.txt +1 -1

 pull.rebase::
      When true, rebase branches on top of the fetched branch, instead
      of merging the default branch from the default remote when "git
      pull" is run. See "branch.<name>.rebase" for setting this on a
-     per-branch basis.
+     per-branch basis.  Incompatible with pull.ff.

Likewise.

I think it technically is OK to say "only ff update is allowed" or "ff update is allowed when able" while saying pull.rebase=yes.

- For those who say ff=only, the actual value of pull.rebase would not matter (i.e. the integration immediately finishes by updating to what was obtained from the upstream because there is no new development on our own on top of theirs to either replay or merge).

- For those who merely allow ff, an update may fast-forward even when pull.rebase is set to true (or any non-false value), while we'll replay our own development on top of theirs when their history is not descendent of ours.

So I do not see need to declare these combinations "incompatible". But perhaps I am missing some subtle cases?

Documentation/git-pull.txt When false, merge the current branch into the upstream branch. · line 126

Quoted 11 lines

Documentation/git-pull.txt +3 -0

index 5c3fb67c014..03e8694e146 100644
@@ -121,6 +121,9 @@ When false, merge the current branch into the upstream branch.
 +
 When `interactive`, enable the interactive mode of rebase.
 +
+Note that these flags are incompatible with --no-ff and --ff-only; if
+such incompatible flags are given, the last one will take precedence.
++

I am not sure about these, either. Again, --ff-only (whether it is combined with --rebase or --rebase=no) would mean that the operation would fail when we have our own development on top of their history, and we repoint the tip of our history to theirs when we do not.

We see "--no-ff" is explained to "always create an unnecessary extra merge", bit I am not sure it is the right mental model to apply when the user prefers rebasing. The merge workflow is to treat our history as the primary and merging theirs in, so when theirs is a descendant (i.e. we have no new development of our own), some people may want to mark "we were there before we updated from them" with an extra merge. Without --no-ff, the resulting history would be quite different in the merge workflow if you have or do not have your own development. But the rebase workflow is to treat their history as the primary and replay our own development on top of theirs, and the

shape of the resulting history would be the same whether you have your own development on top of theirs. We start from their tip and then replay our own development on top (which could be an empty set), and there is nothing "--no-ff" would need to do differently to keep the resulting history similar to cases where we do have something of our own. IOW, "--no-ff" becoming a no-op in a "rebase" workflow is a natural consequence, and there is no strong reason to say it is incompatible.

Let me ask two questions:

1. When is it beneficial for users to set both pull.ff and pull.rebase?
2. Is it harmful to users for us to allow both to be set when we will just ignore one?

I believe the answer to (1) is "never", and the answer to (2) is "yes".

For the second question in particular, I can think of two example cases:

2a) Users start with pull.ff=only, perhaps suggested by someone else and left in their config for a long time. When users hit a case that can't fast-forward and they either ask for help or find a post on stack overflow that suggests setting pull.rebase=true, they do so and then get no warning that the setting they just added is being ignored.

2b) Perhaps users start with pull.rebase=true (suggested by a colleague and forgot about it as they are more of a tester than a developer and thus usually only see fast-forwards). Then at some point they need to function as an integrator, and they read the docs and determine that pull.ff=false should do what they want to create merge commits.

But then they get shocked that they've rebased public commits (and perhaps also pushed them out) when they wanted merges. Our docs have pretty clearly stated that pull.ff=false and --no-ff create merges.

But first let me check if I understand you correctly: I think you're suggesting that
* if --ff-only is given along with any --rebase[=*] flags, then we
silently ignore the --rebase[=*] flags.
* if --no-ff is given along with any --rebase flag other than
--rebase=false, then we silently ignore --no-ff.

Let me ask the same two questions as for the config: Does it benefit the users to allow them to specify both flags? Does it hurt some users, especially if undocumented that one of the flags will be ignored?

I thought this was the biggest hole in my proposal: there is both a git merge --no-ff and a git rebase --no-ff, and they avoid fast-forwards in different ways with different results.

Quoted 3 lines · Elijah Newren

I thought this was the biggest hole in my proposal: there is both a git merge --no-ff and a git rebase --no-ff, and they avoid fast-forwards in different ways with different results.

When you say "git rebase --no-ff upstream", you are telling the machinery that even if your current work is direct descendant of the upstream, you want your commits freshly rebuilt. But in the context of "pull", you being descendant of the other side is called "already up to date", not even "fast forward" (the ancestry relationship between you and the other side is the other way around).

Quoted 2 lines · Elijah Newren

Let me ask two questions:

Quoted 5 lines

1. When is it beneficial for users to set both pull.ff and pull.rebase?
2. Is it harmful to users for us to allow both to be set when we will just ignore one?

I believe the answer to (1) is "never", and the answer to (2) is "yes".

I agree (1) never gives you anything, even though it does not hurt, and (2) is "meh".

Quoted 9 lines · Junio C Hamano

Quoted 6 lines · Elijah Newren

Let me ask two questions:

1. When is it beneficial for users to set both pull.ff and pull.rebase?
2. Is it harmful to users for us to allow both to be set when we will just ignore one?

Quoted 4 lines

Quoted 1 line

I believe the answer to (1) is "never", and the answer to (2) is "yes".

I agree (1) never gives you anything, even though it does not hurt, and (2) is "meh".

Okay, let's drop this series then.

builtin/pull.c +4 -13

-static void show_advice_pull_non_ff(void)
+static void NORETURN die_pull_non_ff(void)
 {
-	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"));
+	die(_("You have divergent branches and need to specify how to reconcile them.\n"
+	      "You can do so by running one of the following commands sometime before\n"
+	      "your next pull:\n"

builtin/pull.c int cmd_pull(int argc, const char **argv, const char *prefix) · line 1079

Quoted 11 lines · Elijah Newren via GitGitGadget

int cmd_pull(int argc, const char **argv, const char *prefix)

builtin/pull.c +2 -3

@@ -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();
 		}

Here we should:

show_advice_pull_non_ff();
die(_("some much briefer summary"))

I.e. we should not being showing giantic advice-y die() messages, the die messages should always be brief, but we might also show advice just before dying.

Quoted 15 lines · Elijah Newren

Quoted 13 lines · Junio C Hamano

Quoted 7 lines · Elijah Newren

Let me ask two questions:

1. When is it beneficial for users to set both pull.ff and pull.rebase?
2. Is it harmful to users for us to allow both to be set when we will just ignore one?

I believe the answer to (1) is "never", and the answer to (2) is "yes".

I agree (1) never gives you anything, even though it does not hurt, and (2) is "meh".

Quoted 1 line

Okay, let's drop this series then.

Not so fast. I did have problem with some combinations you hinted (vaguely---so it is more like "combinations I thought you hinted"), but making sure various combinations of options and configuration variables work sensibly is a worthy goal to have, I would think.

the dozen+ recent messages in the various recent threads, I think I've figured out your opinion in all but two cases, but I have no idea your intent on those two (I would have thought --rebase override there too, but you excluded that), and I'm rather uncertain I've correctly understood you for the other ones (I really hope gmail doesn't whitespace damage the following table):

   pull.ff  pull.rebase  commandline            action
     *          *        --ff-only --rebase     fast-forward only[1]
     *          *        --ff-only --no-rebase  fast-forward only
     *          *        --ff-only              fast-forward only[1]

     *          *        --rebase --no-ff       rebase[1]
     *          *        --rebase --ff          rebase[1]

     *          *        --no-rebase --no-ff    merge --no-ff
     *          *        --no-rebase --ff       merge --ff

    <unset>     *        --no-rebase            merge --ff
    only        *        --no-rebase            merge --ff[2]
    false       *        --no-rebase            merge --no-ff
    true        *        --no-rebase            merge --ff

    <unset>     *        --rebase               rebase
    only        *        --rebase               rebase[2]
    false       *        --rebase               ?[2]
    true        *        --rebase               ?[2]

     *       <unset>     --no-ff                merge --no-ff
     *        false      --no-ff                merge --no-ff
     *       <unset>     --ff                   merge --ff
     *        false      --ff                   merge --ff

     *       !false      --no-ff                rebase (ignore --no-ff)[2][3]
     *       !false      --ff                   rebase (ignore --ff)[2][3]

It appears you, Phillip, and I all had different opinions about correct behavior and in a few cases though the documentation clearly implied what we thought. So, I'd have to say the documentation is rather unclear as well. However, even if the above table is filled out, it may be complicated enough that I'm at a bit of a loss about how to update the documentation to explain it short of including the table in the documentation.

Quoted 4 lines · Elijah Newren

    <unset>     *        --no-rebase            merge --ff
    only        *        --no-rebase            merge --ff[2]
    false       *        --no-rebase            merge --no-ff
    true        *        --no-rebase            merge --ff

I think the second one deserves an explanation. The rationale for ignoring --ff-only is because the act of giving an explicit "--rebase" or "--no-rebase" from the command line, when the configured default is "I expect to have no development on my own here, and only want to follow along", is a sign enough that the user does not want to follow along in this particular invocation of "pull". And the rationale for the entire thing to become --ff is only because between --ff and --no-ff, the former is the default.

About the second one, I would understand it if it became "merge --ff-only", too. That is more trivially explained. I however suspect that it would be less useful, but that is open to discussion.