[PATCH 0/9] Handle pull option precedence

[PATCH 0/9] Handle pull option precedence

[Elijah Newren via GitGitGadget]

Based on a recent list of rules for flag/option precedence for git-pull[1]
from Junio (particularly focusing on rebase vs. merge vs. fast-forward),
here's an attempt to implement and document it. Given multiple recent
surprises from users about some of these behaviors[2][3] and a coworker just
yesterday expressing some puzzlement with git-pull and rebase vs. merge, it
seems like a good time to address some of these issues.

Since the handling of conflicting options was holding up two of Alex's
patches[4][5], and his patches fix some of the tests, I also include those
two patches in my series, with a few small changes to the first (so I've
kept him as author) and more substantial changes to the second (so I've
given him an Initial-patch-by attribution).

Quick overview:

 * Patches 1-2: new testcases (see the commit messages for the rules)
 * Patch 3: Alex's recent patch (abort if --ff-only but can't do so)
 * Patches 4-6: fix the precedence parts Alex didn't cover
 * Patch 7: Alex's other patch, abort if rebase vs. merge not specified
 * Patch 8: Compatibility of git-pull with merge-options.txt (think
   rebasing)
 * Patch 9: Fix multiple heads handling too

[1] https://lore.kernel.org/git/xmqqwnpqot4m.fsf@gitster.g/ [2]
https://lore.kernel.org/git/CAL3xRKdOyVWvcLXK7zoXtFPiHBjgL24zi5hhg+3yjowwSUPgmg@mail.gmail.com/
[3]
https://lore.kernel.org/git/c62933fb-96b2-99f5-7169-372f486f6e39@aixigo.com/
[4]
https://lore.kernel.org/git/20210711012604.947321-1-alexhenrie24@gmail.com/
[5]
https://lore.kernel.org/git/20210627000855.530985-1-alexhenrie24@gmail.com/

Alex Henrie (1):
  pull: abort if --ff-only is given and fast-forwarding is impossible

Elijah Newren (8):
  t7601: add relative precedence tests for merge and rebase
    flags/options
  t7601: add tests of interactions with multiple merge heads and config
  pull: since --ff-only overrides, handle it first
  pull: ensure --rebase overrides ability to ff
  pull: make --rebase and --no-rebase override pull.ff=only
  pull: abort by default when fast-forwarding is not possible
  pull: update docs & code for option compatibility with rebasing
  pull: fix handling of multiple heads

 Documentation/git-pull.txt      |  22 ++--
 Documentation/merge-options.txt |  25 ++++
 advice.c                        |   5 +
 advice.h                        |   1 +
 builtin/merge.c                 |   2 +-
 builtin/pull.c                  |  55 ++++++---
 t/t4013-diff-various.sh         |   2 +-
 t/t5520-pull.sh                 |  26 ++--
 t/t5521-pull-options.sh         |   4 +-
 t/t5524-pull-msg.sh             |   4 +-
 t/t5553-set-upstream.sh         |  14 +--
 t/t5604-clone-reference.sh      |   4 +-
 t/t6402-merge-rename.sh         |  18 +--
 t/t6409-merge-subtree.sh        |   6 +-
 t/t6417-merge-ours-theirs.sh    |  10 +-
 t/t7601-merge-pull-config.sh    | 212 +++++++++++++++++++++++++++++---
 t/t7603-merge-reduce-heads.sh   |   2 +-
 17 files changed, 321 insertions(+), 91 deletions(-)


base-commit: 75ae10bc75336db031ee58d13c5037b929235912
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1049%2Fnewren%2Fhandle-pull-option-precedence-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1049/newren/handle-pull-option-precedence-v1
Pull-Request: https://github.com/git/git/pull/1049
-- 
gitgitgadget

[PATCH 1/9] t7601: add relative precedence tests for merge and rebase flags/options

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

The interaction of rebase and merge flags and options was not well
tested.  Add several tests to check for correct behavior from the
following rules:
    * --ff-only takes precedence over --[no-]rebase
      * Corollary: pull.ff=only overrides pull.rebase
    * --rebase[=!false] takes precedence over --no-ff and --ff
      * Corollary: pull.rebase=!false overrides pull.ff=!only
    * command line flags take precedence over config, except:
      * --no-rebase heeds pull.ff=!only
      * pull.rebase=!false takes precedence over --no-ff and --ff

For more details behind these rules and a larger table of individual
cases, refer to https://lore.kernel.org/git/xmqqwnpqot4m.fsf@gitster.g/
and the links found therein.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 t/t7601-merge-pull-config.sh | 154 +++++++++++++++++++++++++++++++++++
 1 file changed, 154 insertions(+)

diff --git a/t/t7601-merge-pull-config.sh b/t/t7601-merge-pull-config.sh
index 52e8ccc933a..b24c98cc1b6 100755
--- a/t/t7601-merge-pull-config.sh
+++ b/t/t7601-merge-pull-config.sh
@@ -143,6 +143,160 @@ test_expect_success 'pull.rebase not set and --ff-only given (not-fast-forward)'
 	test_i18ngrep ! "Pulling without specifying how to reconcile" err
 '
 
+test_does_rebase() {
+	git reset --hard c2 &&
+	git "$@" . c1 &&
+	# Check that we actually did a rebase
+	git rev-list --count HEAD >actual &&
+	git rev-list --merges --count HEAD >>actual &&
+	test_write_lines 3 0 >expect &&
+	test_cmp expect actual &&
+	rm actual expect
+}
+
+test_does_merge_noff() {
+	git reset --hard c0 &&
+	git "$@" . c1 &&
+	# Check that we actually did a merge
+	git rev-list --count HEAD >actual &&
+	git rev-list --merges --count HEAD >>actual &&
+	test_write_lines 3 1 >expect &&
+	test_cmp expect actual &&
+	rm actual expect
+}
+
+test_does_merge_ff() {
+	git reset --hard c0 &&
+	git "$@" . c1 &&
+	# Check that we actually did a merge
+	git rev-list --count HEAD >actual &&
+	git rev-list --merges --count HEAD >>actual &&
+	test_write_lines 2 0 >expect &&
+	test_cmp expect actual &&
+	rm actual expect
+}
+
+test_does_need_full_merge() {
+	git reset --hard c2 &&
+	git "$@" . c1 &&
+	# Check that we actually did a merge
+	git rev-list --count HEAD >actual &&
+	git rev-list --merges --count HEAD >>actual &&
+	test_write_lines 4 1 >expect &&
+	test_cmp expect actual &&
+	rm actual expect
+}
+
+test_attempts_fast_forward() {
+	git reset --hard c2 &&
+	test_must_fail git "$@" . c1 2>err &&
+	test_i18ngrep "Not possible to fast-forward, aborting" err
+}
+
+#
+# Rule 1: --ff-only takes precedence over --[no-]rebase
+# (Corollary: pull.ff=only overrides pull.rebase)
+#
+test_expect_failure '--ff-only takes precedence over --rebase' '
+	test_attempts_fast_forward pull --rebase --ff-only
+'
+
+test_expect_failure '--ff-only takes precedence over --rebase even if first' '
+	test_attempts_fast_forward pull --ff-only --rebase
+'
+
+test_expect_success '--ff-only takes precedence over --no-rebase' '
+	test_attempts_fast_forward pull --ff-only --no-rebase
+'
+
+test_expect_failure 'pull.ff=only overrides pull.rebase=true' '
+	test_attempts_fast_forward -c pull.ff=only -c pull.rebase=true pull
+'
+
+test_expect_success 'pull.ff=only overrides pull.rebase=false' '
+	test_attempts_fast_forward -c pull.ff=only -c pull.rebase=false pull
+'
+
+# Rule 2: --rebase=[!false] takes precedence over --no-ff and --ff
+# (Corollary: pull.rebase=!false overrides pull.ff=!only)
+test_expect_success '--rebase takes precedence over --no-ff' '
+	test_does_rebase pull --rebase --no-ff
+'
+
+test_expect_success '--rebase takes precedence over --ff' '
+	test_does_rebase pull --rebase --ff
+'
+
+test_expect_success 'pull.rebase=true takes precedence over pull.ff=false' '
+	test_does_rebase -c pull.rebase=true -c pull.ff=false pull
+'
+
+test_expect_success 'pull.rebase=true takes precedence over pull.ff=true' '
+	test_does_rebase -c pull.rebase=true -c pull.ff=true pull
+'
+
+# Rule 3: command line flags take precedence over config
+test_expect_failure '--ff-only takes precedence over pull.rebase=true' '
+	test_attempts_fast_forward -c pull.rebase=true pull --ff-only
+'
+
+test_expect_success '--ff-only takes precedence over pull.rebase=false' '
+	test_attempts_fast_forward -c pull.rebase=false pull --ff-only
+'
+
+test_expect_failure '--no-rebase overrides pull.ff=only' '
+	test_does_need_full_merge -c pull.ff=only pull --no-rebase
+'
+
+test_expect_success '--rebase takes precedence over pull.ff=only' '
+	test_does_rebase -c pull.ff=only pull --rebase
+'
+
+test_expect_success '--rebase takes precedence over pull.ff=true' '
+	test_does_rebase -c pull.ff=true pull --rebase
+'
+
+test_expect_success '--rebase takes precedence over pull.ff=false' '
+	test_does_rebase -c pull.ff=false pull --rebase
+'
+
+test_expect_success '--rebase takes precedence over pull.ff unset' '
+	test_does_rebase pull --rebase
+'
+
+# Rule 4: --no-rebase heeds pull.ff=!only or explict --ff or --no-ff
+
+test_expect_success '--no-rebase works with --no-ff' '
+	test_does_merge_noff pull --no-rebase --no-ff
+'
+
+test_expect_success '--no-rebase works with --ff' '
+	test_does_merge_ff pull --no-rebase --ff
+'
+
+test_expect_success '--no-rebase does ff if pull.ff unset' '
+	test_does_merge_ff pull --no-rebase
+'
+
+test_expect_success '--no-rebase heeds pull.ff=true' '
+	test_does_merge_ff -c pull.ff=true pull --no-rebase
+'
+
+test_expect_success '--no-rebase heeds pull.ff=false' '
+	test_does_merge_noff -c pull.ff=false pull --no-rebase
+'
+
+# Rule 5: pull.rebase=!false takes precedence over --no-ff and --ff
+test_expect_success 'pull.rebase=true takes precedence over --no-ff' '
+	test_does_rebase -c pull.rebase=true pull --no-ff
+'
+
+test_expect_success 'pull.rebase=true takes precedence over --ff' '
+	test_does_rebase -c pull.rebase=true pull --ff
+'
+
+# End of precedence rules
+
 test_expect_success 'merge c1 with c2' '
 	git reset --hard c1 &&
 	test -f c0.c &&
-- 
gitgitgadget

[PATCH 2/9] t7601: add tests of interactions with multiple merge heads and config

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

There were already code checking that --rebase was incompatible with
a merge of multiple heads.  However, we were sometimes throwing warnings
about lack of specification of rebase vs. merge when given multiple
heads.  Since rebasing is disallowed with multiple merge heads, that
seems like a poor warning to print; we should instead just assume
merging is wanted.

Add a few tests checking multiple merge head behavior.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 t/t7601-merge-pull-config.sh | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/t/t7601-merge-pull-config.sh b/t/t7601-merge-pull-config.sh
index b24c98cc1b6..40d8fb95113 100755
--- a/t/t7601-merge-pull-config.sh
+++ b/t/t7601-merge-pull-config.sh
@@ -297,6 +297,26 @@ test_expect_success 'pull.rebase=true takes precedence over --ff' '
 
 # End of precedence rules
 
+test_expect_failure 'Multiple heads does not warn about fast forwarding' '
+	git reset --hard c1 &&
+	git pull . c2 c3 2>err &&
+	test_i18ngrep ! "Pulling without specifying how to reconcile" err
+'
+
+test_expect_success 'Cannot fast-forward with multiple heads' '
+	git reset --hard c0 &&
+	test_must_fail git -c pull.ff=only pull . c1 c2 c3 2>err &&
+	test_i18ngrep ! "Pulling without specifying how to reconcile" err &&
+	test_i18ngrep "Not possible to fast-forward, aborting" err
+'
+
+test_expect_success 'Cannot rebase with multiple heads' '
+	git reset --hard c0 &&
+	test_must_fail git -c pull.rebase=true pull . c1 c2 c3 2>err &&
+	test_i18ngrep ! "Pulling without specifying how to reconcile" err &&
+	test_i18ngrep "Cannot rebase onto multiple branches." err
+'
+
 test_expect_success 'merge c1 with c2' '
 	git reset --hard c1 &&
 	test -f c0.c &&
-- 
gitgitgadget

[PATCH 3/9] pull: abort if --ff-only is given and fast-forwarding is impossible

[Alex Henrie via GitGitGadget]

From: Alex Henrie <alexhenrie24@gmail.com>

The warning about pulling without specifying how to reconcile divergent
branches says that after setting pull.rebase to true, --ff-only can
still be passed on the command line to require a fast-forward. Make that
actually work.

Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>
[en: updated tests; note 3 fixes and 1 new failure]
Signed-off-by: Elijah Newren <newren@gmail.com>
---
 advice.c                     |  5 +++++
 advice.h                     |  1 +
 builtin/merge.c              |  2 +-
 builtin/pull.c               | 11 ++++++++---
 t/t7601-merge-pull-config.sh | 10 +++++-----
 5 files changed, 20 insertions(+), 9 deletions(-)

diff --git a/advice.c b/advice.c
index 0b9c89c48ab..337e8f342bc 100644
--- a/advice.c
+++ b/advice.c
@@ -286,6 +286,11 @@ void NORETURN die_conclude_merge(void)
 	die(_("Exiting because of unfinished merge."));
 }
 
+void NORETURN die_ff_impossible(void)
+{
+	die(_("Not possible to fast-forward, aborting."));
+}
+
 void advise_on_updating_sparse_paths(struct string_list *pathspec_list)
 {
 	struct string_list_item *item;
diff --git a/advice.h b/advice.h
index bd26c385d00..16240438387 100644
--- a/advice.h
+++ b/advice.h
@@ -95,6 +95,7 @@ void advise_if_enabled(enum advice_type type, const char *advice, ...);
 int error_resolve_conflict(const char *me);
 void NORETURN die_resolve_conflict(const char *me);
 void NORETURN die_conclude_merge(void);
+void NORETURN die_ff_impossible(void);
 void advise_on_updating_sparse_paths(struct string_list *pathspec_list);
 void detach_advice(const char *new_name);
 
diff --git a/builtin/merge.c b/builtin/merge.c
index a8a843b1f54..aa920ac524f 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -1620,7 +1620,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 	}
 
 	if (fast_forward == FF_ONLY)
-		die(_("Not possible to fast-forward, aborting."));
+		die_ff_impossible();
 
 	if (autostash)
 		create_autostash(the_repository,
diff --git a/builtin/pull.c b/builtin/pull.c
index 3e13f810843..d9796604825 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -1046,9 +1046,14 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 
 	can_ff = get_can_ff(&orig_head, &merge_heads.oid[0]);
 
-	if (rebase_unspecified && !opt_ff && !can_ff) {
-		if (opt_verbosity >= 0)
-			show_advice_pull_non_ff();
+	if (!can_ff) {
+		if (opt_ff) {
+			if (!strcmp(opt_ff, "--ff-only"))
+				die_ff_impossible();
+		} else {
+			if (rebase_unspecified && opt_verbosity >= 0)
+				show_advice_pull_non_ff();
+		}
 	}
 
 	if (opt_rebase) {
diff --git a/t/t7601-merge-pull-config.sh b/t/t7601-merge-pull-config.sh
index 40d8fb95113..23a1fbc17b3 100755
--- a/t/t7601-merge-pull-config.sh
+++ b/t/t7601-merge-pull-config.sh
@@ -197,11 +197,11 @@ test_attempts_fast_forward() {
 # Rule 1: --ff-only takes precedence over --[no-]rebase
 # (Corollary: pull.ff=only overrides pull.rebase)
 #
-test_expect_failure '--ff-only takes precedence over --rebase' '
+test_expect_success '--ff-only takes precedence over --rebase' '
 	test_attempts_fast_forward pull --rebase --ff-only
 '
 
-test_expect_failure '--ff-only takes precedence over --rebase even if first' '
+test_expect_success '--ff-only takes precedence over --rebase even if first' '
 	test_attempts_fast_forward pull --ff-only --rebase
 '
 
@@ -209,7 +209,7 @@ test_expect_success '--ff-only takes precedence over --no-rebase' '
 	test_attempts_fast_forward pull --ff-only --no-rebase
 '
 
-test_expect_failure 'pull.ff=only overrides pull.rebase=true' '
+test_expect_success 'pull.ff=only overrides pull.rebase=true' '
 	test_attempts_fast_forward -c pull.ff=only -c pull.rebase=true pull
 '
 
@@ -236,7 +236,7 @@ test_expect_success 'pull.rebase=true takes precedence over pull.ff=true' '
 '
 
 # Rule 3: command line flags take precedence over config
-test_expect_failure '--ff-only takes precedence over pull.rebase=true' '
+test_expect_success '--ff-only takes precedence over pull.rebase=true' '
 	test_attempts_fast_forward -c pull.rebase=true pull --ff-only
 '
 
@@ -248,7 +248,7 @@ test_expect_failure '--no-rebase overrides pull.ff=only' '
 	test_does_need_full_merge -c pull.ff=only pull --no-rebase
 '
 
-test_expect_success '--rebase takes precedence over pull.ff=only' '
+test_expect_failure '--rebase takes precedence over pull.ff=only' '
 	test_does_rebase -c pull.ff=only pull --rebase
 '
 
-- 
gitgitgadget

[PATCH 4/9] pull: since --ff-only overrides, handle it first

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

There are both merge and rebase branches in the logic, and previously
both had to handle fast-forwarding.  Merge handled that implicitly
(because git merge handles it directly), while in rebase it was
explicit.  Given that the --ff-only flag is meant to override any
--rebase or --no-rebase, make the code reflect that by handling
--ff-only before the merge-vs-rebase logic.

No functional changes, just making it easier to verify that the codeflow
matches our precedence rules.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 builtin/pull.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/builtin/pull.c b/builtin/pull.c
index d9796604825..5c9cbea37c9 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -1046,15 +1046,16 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 
 	can_ff = get_can_ff(&orig_head, &merge_heads.oid[0]);
 
-	if (!can_ff) {
-		if (opt_ff) {
-			if (!strcmp(opt_ff, "--ff-only"))
-				die_ff_impossible();
-		} else {
-			if (rebase_unspecified && opt_verbosity >= 0)
-				show_advice_pull_non_ff();
-		}
+	/* ff-only takes precedence over rebase */
+	if (opt_ff && !strcmp(opt_ff, "--ff-only")) {
+		if (!can_ff)
+			die_ff_impossible();
+		else
+			return run_merge();
 	}
+	/* If no action specified and we can't fast forward, then warn. */
+	if (!opt_ff && rebase_unspecified && !can_ff)
+		show_advice_pull_non_ff();
 
 	if (opt_rebase) {
 		int ret = 0;
-- 
gitgitgadget

[PATCH 5/9] pull: ensure --rebase overrides ability to ff

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

Now that the handling of fast-forward-only in combination with rebases
has been moved before the merge-vs-rebase logic, we have an unnecessary
special fast-forward case left within the rebase logic.  Actually, more
than unnecessary, it's actually a violation of the rules.  As per
https://lore.kernel.org/git/xmqqwnpqot4m.fsf@gitster.g/, --rebase is
supposed to override all ff flags other than an explicit --ff-only.
Ensure that it does so by removing the fast-forward special case that
exists within the rebase logic.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 builtin/pull.c  | 8 +-------
 t/t5520-pull.sh | 6 +++---
 2 files changed, 4 insertions(+), 10 deletions(-)

diff --git a/builtin/pull.c b/builtin/pull.c
index 5c9cbea37c9..5ba376a7487 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -1070,13 +1070,7 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 		    submodule_touches_in_range(the_repository, &upstream, &curr_head))
 			die(_("cannot rebase with locally recorded submodule modifications"));
 
-		if (can_ff) {
-			/* we can fast-forward this without invoking rebase */
-			opt_ff = "--ff-only";
-			ret = run_merge();
-		} else {
-			ret = run_rebase(&newbase, &upstream);
-		}
+		ret = run_rebase(&newbase, &upstream);
 
 		if (!ret && (recurse_submodules == RECURSE_SUBMODULES_ON ||
 			     recurse_submodules == RECURSE_SUBMODULES_ON_DEMAND))
diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index e2c0c510222..4b50488141f 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh
@@ -295,7 +295,7 @@ test_expect_success '--rebase (merge) fast forward' '
 	# The above only validates the result.  Did we actually bypass rebase?
 	git reflog -1 >reflog.actual &&
 	sed "s/^[0-9a-f][0-9a-f]*/OBJID/" reflog.actual >reflog.fuzzy &&
-	echo "OBJID HEAD@{0}: pull --rebase . ff: Fast-forward" >reflog.expected &&
+	echo "OBJID HEAD@{0}: pull --rebase . ff (finish): returning to refs/heads/to-rebase" >reflog.expected &&
 	test_cmp reflog.expected reflog.fuzzy
 '
 
@@ -307,8 +307,8 @@ test_expect_success '--rebase (am) fast forward' '
 
 	# The above only validates the result.  Did we actually bypass rebase?
 	git reflog -1 >reflog.actual &&
-	sed "s/^[0-9a-f][0-9a-f]*/OBJID/" reflog.actual >reflog.fuzzy &&
-	echo "OBJID HEAD@{0}: pull --rebase . ff: Fast-forward" >reflog.expected &&
+	sed -e "s/^[0-9a-f][0-9a-f]*/OBJID/" -e "s/[0-9a-f][0-9a-f]*$/OBJID/" reflog.actual >reflog.fuzzy &&
+	echo "OBJID HEAD@{0}: rebase finished: refs/heads/to-rebase onto OBJID" >reflog.expected &&
 	test_cmp reflog.expected reflog.fuzzy
 '
 
-- 
gitgitgadget

[PATCH 6/9] pull: make --rebase and --no-rebase override pull.ff=only

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

Fix the last few precedence tests failing in t7601 by now implementing
the logic to have --[no-]rebase override a pull.ff=only config setting.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 builtin/pull.c               | 5 ++++-
 t/t7601-merge-pull-config.sh | 4 ++--
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/builtin/pull.c b/builtin/pull.c
index 5ba376a7487..7e7c90f6a10 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -966,8 +966,11 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 
 	parse_repo_refspecs(argc, argv, &repo, &refspecs);
 
-	if (!opt_ff)
+	if (!opt_ff) {
 		opt_ff = xstrdup_or_null(config_get_ff());
+		if (opt_rebase >= 0 && opt_ff && !strcmp(opt_ff, "--ff-only"))
+			opt_ff = "--ff";
+	}
 
 	if (opt_rebase < 0)
 		opt_rebase = config_get_rebase(&rebase_unspecified);
diff --git a/t/t7601-merge-pull-config.sh b/t/t7601-merge-pull-config.sh
index 23a1fbc17b3..12787d07289 100755
--- a/t/t7601-merge-pull-config.sh
+++ b/t/t7601-merge-pull-config.sh
@@ -244,11 +244,11 @@ test_expect_success '--ff-only takes precedence over pull.rebase=false' '
 	test_attempts_fast_forward -c pull.rebase=false pull --ff-only
 '
 
-test_expect_failure '--no-rebase overrides pull.ff=only' '
+test_expect_success '--no-rebase overrides pull.ff=only' '
 	test_does_need_full_merge -c pull.ff=only pull --no-rebase
 '
 
-test_expect_failure '--rebase takes precedence over pull.ff=only' '
+test_expect_success '--rebase takes precedence over pull.ff=only' '
 	test_does_rebase -c pull.ff=only pull --rebase
 '
 
-- 
gitgitgadget

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

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

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.

Initial-patch-by: Alex Henrie <alexhenrie24@gmail.com>
Signed-off-by: Elijah Newren <newren@gmail.com>
---
 Documentation/git-pull.txt    | 20 +++++++++++-------
 builtin/pull.c                | 10 +++++----
 t/t4013-diff-various.sh       |  2 +-
 t/t5520-pull.sh               | 20 +++++++++---------
 t/t5521-pull-options.sh       |  4 ++--
 t/t5524-pull-msg.sh           |  4 ++--
 t/t5553-set-upstream.sh       | 14 ++++++------
 t/t5604-clone-reference.sh    |  4 ++--
 t/t6402-merge-rename.sh       | 18 ++++++++--------
 t/t6409-merge-subtree.sh      |  6 +++---
 t/t6417-merge-ours-theirs.sh  | 10 ++++-----
 t/t7601-merge-pull-config.sh  | 40 +++++++++++++++++------------------
 t/t7603-merge-reduce-heads.sh |  2 +-
 13 files changed, 80 insertions(+), 74 deletions(-)

diff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt
index 5c3fb67c014..cad3f6bfcee 100644
--- a/Documentation/git-pull.txt
+++ b/Documentation/git-pull.txt
@@ -15,14 +15,18 @@ SYNOPSIS
 DESCRIPTION
 -----------
 
-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'.
+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`, `--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 configuration options or command line flags,
+will call either `git merge` or `git rebase` to reconcile diverging
+branches.
 
 <repository> should be the name of a remote repository as
 passed to linkgit:git-fetch[1].  <refspec> can name an
diff --git a/builtin/pull.c b/builtin/pull.c
index 7e7c90f6a10..2d7f2d765ab 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -927,9 +927,9 @@ static int get_can_ff(struct object_id *orig_head, struct object_id *orig_merge_
 
 static void show_advice_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"
+	advise(_("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"
 		 "\n"
 		 "  git config pull.rebase false  # merge (the default strategy)\n"
 		 "  git config pull.rebase true   # rebase\n"
@@ -1057,8 +1057,10 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 			return run_merge();
 	}
 	/* If no action specified and we can't fast forward, then warn. */
-	if (!opt_ff && rebase_unspecified && !can_ff)
+	if (!opt_ff && rebase_unspecified && !can_ff) {
 		show_advice_pull_non_ff();
+		die(_("Need to specify how to reconcile divergent branches."));
+	}
 
 	if (opt_rebase) {
 		int ret = 0;
diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
index 7fadc985ccc..eb989f7f191 100755
--- a/t/t4013-diff-various.sh
+++ b/t/t4013-diff-various.sh
@@ -65,7 +65,7 @@ test_expect_success setup '
 	export GIT_AUTHOR_DATE GIT_COMMITTER_DATE &&
 
 	git checkout master &&
-	git pull -s ours . side &&
+	git pull -s ours --no-rebase . side &&
 
 	GIT_AUTHOR_DATE="2006-06-26 00:05:00 +0000" &&
 	GIT_COMMITTER_DATE="2006-06-26 00:05:00 +0000" &&
diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index 4b50488141f..56f5864b269 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh
@@ -136,12 +136,12 @@ test_expect_success 'the default remote . should not break explicit pull' '
 	git reset --hard HEAD^ &&
 	echo file >expect &&
 	test_cmp expect file &&
-	git pull . second &&
+	git pull --no-rebase . second &&
 	echo modified >expect &&
 	test_cmp expect file &&
 	git reflog -1 >reflog.actual &&
 	sed "s/^[0-9a-f][0-9a-f]*/OBJID/" reflog.actual >reflog.fuzzy &&
-	echo "OBJID HEAD@{0}: pull . second: Fast-forward" >reflog.expected &&
+	echo "OBJID HEAD@{0}: pull --no-rebase . second: Fast-forward" >reflog.expected &&
 	test_cmp reflog.expected reflog.fuzzy
 '
 
@@ -226,7 +226,7 @@ test_expect_success 'fail if the index has unresolved entries' '
 	test_commit modified2 file &&
 	git ls-files -u >unmerged &&
 	test_must_be_empty unmerged &&
-	test_must_fail git pull . second &&
+	test_must_fail git pull --no-rebase . second &&
 	git ls-files -u >unmerged &&
 	test_file_not_empty unmerged &&
 	cp file expected &&
@@ -409,37 +409,37 @@ test_expect_success 'pull --rebase --no-autostash & rebase.autostash unset' '
 
 test_expect_success 'pull succeeds with dirty working directory and merge.autostash set' '
 	test_config merge.autostash true &&
-	test_pull_autostash 2
+	test_pull_autostash 2 --no-rebase
 '
 
 test_expect_success 'pull --autostash & merge.autostash=true' '
 	test_config merge.autostash true &&
-	test_pull_autostash 2 --autostash
+	test_pull_autostash 2 --autostash --no-rebase
 '
 
 test_expect_success 'pull --autostash & merge.autostash=false' '
 	test_config merge.autostash false &&
-	test_pull_autostash 2 --autostash
+	test_pull_autostash 2 --autostash --no-rebase
 '
 
 test_expect_success 'pull --autostash & merge.autostash unset' '
 	test_unconfig merge.autostash &&
-	test_pull_autostash 2 --autostash
+	test_pull_autostash 2 --autostash --no-rebase
 '
 
 test_expect_success 'pull --no-autostash & merge.autostash=true' '
 	test_config merge.autostash true &&
-	test_pull_autostash_fail --no-autostash
+	test_pull_autostash_fail --no-autostash --no-rebase
 '
 
 test_expect_success 'pull --no-autostash & merge.autostash=false' '
 	test_config merge.autostash false &&
-	test_pull_autostash_fail --no-autostash
+	test_pull_autostash_fail --no-autostash --no-rebase
 '
 
 test_expect_success 'pull --no-autostash & merge.autostash unset' '
 	test_unconfig merge.autostash &&
-	test_pull_autostash_fail --no-autostash
+	test_pull_autostash_fail --no-autostash --no-rebase
 '
 
 test_expect_success 'pull.rebase' '
diff --git a/t/t5521-pull-options.sh b/t/t5521-pull-options.sh
index 63a688bdbf5..7601c919fdc 100755
--- a/t/t5521-pull-options.sh
+++ b/t/t5521-pull-options.sh
@@ -113,7 +113,7 @@ test_expect_success 'git pull --force' '
 	git pull two &&
 	test_commit A &&
 	git branch -f origin &&
-	git pull --all --force
+	git pull --no-rebase --all --force
 	)
 '
 
@@ -179,7 +179,7 @@ test_expect_success 'git pull --allow-unrelated-histories' '
 	(
 		cd dst &&
 		test_must_fail git pull ../src side &&
-		git pull --allow-unrelated-histories ../src side
+		git pull --no-rebase --allow-unrelated-histories ../src side
 	)
 '
 
diff --git a/t/t5524-pull-msg.sh b/t/t5524-pull-msg.sh
index c278adaa5a2..b2be3605f5a 100755
--- a/t/t5524-pull-msg.sh
+++ b/t/t5524-pull-msg.sh
@@ -28,7 +28,7 @@ test_expect_success setup '
 test_expect_success pull '
 (
 	cd cloned &&
-	git pull --log &&
+	git pull --no-rebase --log &&
 	git log -2 &&
 	git cat-file commit HEAD >result &&
 	grep Dollar result
@@ -41,7 +41,7 @@ test_expect_success '--log=1 limits shortlog length' '
 	git reset --hard HEAD^ &&
 	test "$(cat afile)" = original &&
 	test "$(cat bfile)" = added &&
-	git pull --log=1 &&
+	git pull --no-rebase --log=1 &&
 	git log -3 &&
 	git cat-file commit HEAD >result &&
 	grep Dollar result &&
diff --git a/t/t5553-set-upstream.sh b/t/t5553-set-upstream.sh
index b1d614ce18c..9c12c0f8c32 100755
--- a/t/t5553-set-upstream.sh
+++ b/t/t5553-set-upstream.sh
@@ -108,27 +108,27 @@ test_expect_success 'setup commit on main and other pull' '
 
 test_expect_success 'pull --set-upstream upstream main sets branch main but not other' '
 	clear_config main other &&
-	git pull --set-upstream upstream main &&
+	git pull --no-rebase --set-upstream upstream main &&
 	check_config main upstream refs/heads/main &&
 	check_config_missing other
 '
 
 test_expect_success 'pull --set-upstream main:other2 does not set the branch other2' '
 	clear_config other2 &&
-	git pull --set-upstream upstream main:other2 &&
+	git pull --no-rebase --set-upstream upstream main:other2 &&
 	check_config_missing other2
 '
 
 test_expect_success 'pull --set-upstream upstream other sets branch main' '
 	clear_config main other &&
-	git pull --set-upstream upstream other &&
+	git pull --no-rebase --set-upstream upstream other &&
 	check_config main upstream refs/heads/other &&
 	check_config_missing other
 '
 
 test_expect_success 'pull --set-upstream upstream tag does not set the tag' '
 	clear_config three &&
-	git pull --tags --set-upstream upstream three &&
+	git pull --no-rebase --tags --set-upstream upstream three &&
 	check_config_missing three
 '
 
@@ -144,16 +144,16 @@ test_expect_success 'pull --set-upstream http://nosuchdomain.example.com fails w
 
 test_expect_success 'pull --set-upstream upstream HEAD sets branch HEAD' '
 	clear_config main other &&
-	git pull --set-upstream upstream HEAD &&
+	git pull --no-rebase --set-upstream upstream HEAD &&
 	check_config main upstream HEAD &&
 	git checkout other &&
-	git pull --set-upstream upstream HEAD &&
+	git pull --no-rebase --set-upstream upstream HEAD &&
 	check_config other upstream HEAD
 '
 
 test_expect_success 'pull --set-upstream upstream with more than one branch does nothing' '
 	clear_config main three &&
-	git pull --set-upstream upstream main three &&
+	git pull --no-rebase --set-upstream upstream main three &&
 	check_config_missing main &&
 	check_config_missing three
 '
diff --git a/t/t5604-clone-reference.sh b/t/t5604-clone-reference.sh
index e845d621f61..24340e6d56e 100755
--- a/t/t5604-clone-reference.sh
+++ b/t/t5604-clone-reference.sh
@@ -87,7 +87,7 @@ test_expect_success 'updating origin' '
 '
 
 test_expect_success 'pulling changes from origin' '
-	git -C C pull origin
+	git -C C pull --no-rebase origin
 '
 
 # the 2 local objects are commit and tree from the merge
@@ -96,7 +96,7 @@ test_expect_success 'that alternate to origin gets used' '
 '
 
 test_expect_success 'pulling changes from origin' '
-	git -C D pull origin
+	git -C D pull --no-rebase origin
 '
 
 # the 5 local objects are expected; file3 blob, commit in A to add it
diff --git a/t/t6402-merge-rename.sh b/t/t6402-merge-rename.sh
index 425dad97d54..02a842697b8 100755
--- a/t/t6402-merge-rename.sh
+++ b/t/t6402-merge-rename.sh
@@ -103,7 +103,7 @@ test_expect_success 'setup' '
 test_expect_success 'pull renaming branch into unrenaming one' \
 '
 	git show-branch &&
-	test_expect_code 1 git pull . white &&
+	test_expect_code 1 git pull --no-rebase . white &&
 	git ls-files -s &&
 	git ls-files -u B >b.stages &&
 	test_line_count = 3 b.stages &&
@@ -121,7 +121,7 @@ test_expect_success 'pull renaming branch into another renaming one' \
 	rm -f B &&
 	git reset --hard &&
 	git checkout red &&
-	test_expect_code 1 git pull . white &&
+	test_expect_code 1 git pull --no-rebase . white &&
 	git ls-files -u B >b.stages &&
 	test_line_count = 3 b.stages &&
 	git ls-files -s N >n.stages &&
@@ -137,7 +137,7 @@ test_expect_success 'pull unrenaming branch into renaming one' \
 '
 	git reset --hard &&
 	git show-branch &&
-	test_expect_code 1 git pull . main &&
+	test_expect_code 1 git pull --no-rebase . main &&
 	git ls-files -u B >b.stages &&
 	test_line_count = 3 b.stages &&
 	git ls-files -s N >n.stages &&
@@ -153,7 +153,7 @@ test_expect_success 'pull conflicting renames' \
 '
 	git reset --hard &&
 	git show-branch &&
-	test_expect_code 1 git pull . blue &&
+	test_expect_code 1 git pull --no-rebase . blue &&
 	git ls-files -u A >a.stages &&
 	test_line_count = 1 a.stages &&
 	git ls-files -u B >b.stages &&
@@ -173,7 +173,7 @@ test_expect_success 'interference with untracked working tree file' '
 	git reset --hard &&
 	git show-branch &&
 	echo >A this file should not matter &&
-	test_expect_code 1 git pull . white &&
+	test_expect_code 1 git pull --no-rebase . white &&
 	test_path_is_file A
 '
 
@@ -183,7 +183,7 @@ test_expect_success 'interference with untracked working tree file' '
 	git show-branch &&
 	rm -f A &&
 	echo >A this file should not matter &&
-	test_expect_code 1 git pull . red &&
+	test_expect_code 1 git pull --no-rebase . red &&
 	test_path_is_file A
 '
 
@@ -193,7 +193,7 @@ test_expect_success 'interference with untracked working tree file' '
 	git checkout -f main &&
 	git tag -f anchor &&
 	git show-branch &&
-	git pull . yellow &&
+	git pull --no-rebase . yellow &&
 	test_path_is_missing M &&
 	git reset --hard anchor
 '
@@ -220,7 +220,7 @@ test_expect_success 'updated working tree file should prevent the merge' '
 	echo >>M one line addition &&
 	cat M >M.saved &&
 	git update-index M &&
-	test_expect_code 128 git pull . yellow &&
+	test_expect_code 128 git pull --no-rebase . yellow &&
 	test_cmp M M.saved &&
 	rm -f M.saved
 '
@@ -232,7 +232,7 @@ test_expect_success 'interference with untracked working tree file' '
 	git tag -f anchor &&
 	git show-branch &&
 	echo >M this file should not matter &&
-	git pull . main &&
+	git pull --no-rebase . main &&
 	test_path_is_file M &&
 	! {
 		git ls-files -s |
diff --git a/t/t6409-merge-subtree.sh b/t/t6409-merge-subtree.sh
index d406b2343cb..ba7890ec521 100755
--- a/t/t6409-merge-subtree.sh
+++ b/t/t6409-merge-subtree.sh
@@ -100,7 +100,7 @@ test_expect_success 'merge update' '
 	git checkout -b topic_2 &&
 	git commit -m "update git-gui" &&
 	cd ../git &&
-	git pull -s subtree gui topic_2 &&
+	git pull --no-rebase -s subtree gui topic_2 &&
 	git ls-files -s >actual &&
 	(
 		echo "100644 $o3 0	git-gui/git-gui.sh" &&
@@ -129,7 +129,7 @@ test_expect_success 'initial ambiguous subtree' '
 test_expect_success 'merge using explicit' '
 	cd ../git &&
 	git reset --hard topic_2 &&
-	git pull -Xsubtree=git-gui gui topic_2 &&
+	git pull --no-rebase -Xsubtree=git-gui gui topic_2 &&
 	git ls-files -s >actual &&
 	(
 		echo "100644 $o3 0	git-gui/git-gui.sh" &&
@@ -142,7 +142,7 @@ test_expect_success 'merge using explicit' '
 test_expect_success 'merge2 using explicit' '
 	cd ../git &&
 	git reset --hard topic_2 &&
-	git pull -Xsubtree=git-gui2 gui topic_2 &&
+	git pull --no-rebase -Xsubtree=git-gui2 gui topic_2 &&
 	git ls-files -s >actual &&
 	(
 		echo "100644 $o1 0	git-gui/git-gui.sh" &&
diff --git a/t/t6417-merge-ours-theirs.sh b/t/t6417-merge-ours-theirs.sh
index ac9aee9a662..ec065d6a658 100755
--- a/t/t6417-merge-ours-theirs.sh
+++ b/t/t6417-merge-ours-theirs.sh
@@ -69,11 +69,11 @@ test_expect_success 'binary file with -Xours/-Xtheirs' '
 '
 
 test_expect_success 'pull passes -X to underlying merge' '
-	git reset --hard main && git pull -s recursive -Xours . side &&
-	git reset --hard main && git pull -s recursive -X ours . side &&
-	git reset --hard main && git pull -s recursive -Xtheirs . side &&
-	git reset --hard main && git pull -s recursive -X theirs . side &&
-	git reset --hard main && test_must_fail git pull -s recursive -X bork . side
+	git reset --hard main && git pull --no-rebase -s recursive -Xours . side &&
+	git reset --hard main && git pull --no-rebase -s recursive -X ours . side &&
+	git reset --hard main && git pull --no-rebase -s recursive -Xtheirs . side &&
+	git reset --hard main && git pull --no-rebase -s recursive -X theirs . side &&
+	git reset --hard main && test_must_fail git pull --no-rebase -s recursive -X bork . side
 '
 
 test_expect_success SYMLINKS 'symlink with -Xours/-Xtheirs' '
diff --git a/t/t7601-merge-pull-config.sh b/t/t7601-merge-pull-config.sh
index 12787d07289..673b92afbab 100755
--- a/t/t7601-merge-pull-config.sh
+++ b/t/t7601-merge-pull-config.sh
@@ -27,120 +27,120 @@ test_expect_success 'setup' '
 	git tag c3
 '
 
-test_expect_success 'pull.rebase not set' '
+test_expect_success 'pull.rebase not set, ff possible' '
 	git reset --hard c0 &&
 	git pull . c1 2>err &&
-	test_i18ngrep ! "Pulling without specifying how to reconcile" err
+	test_i18ngrep ! "You have divergent branches" err
 '
 
 test_expect_success 'pull.rebase not set and pull.ff=true' '
 	git reset --hard c0 &&
 	test_config pull.ff true &&
 	git pull . c1 2>err &&
-	test_i18ngrep ! "Pulling without specifying how to reconcile" err
+	test_i18ngrep ! "You have divergent branches" err
 '
 
 test_expect_success 'pull.rebase not set and pull.ff=false' '
 	git reset --hard c0 &&
 	test_config pull.ff false &&
 	git pull . c1 2>err &&
-	test_i18ngrep ! "Pulling without specifying how to reconcile" err
+	test_i18ngrep ! "You have divergent branches" err
 '
 
 test_expect_success 'pull.rebase not set and pull.ff=only' '
 	git reset --hard c0 &&
 	test_config pull.ff only &&
 	git pull . c1 2>err &&
-	test_i18ngrep ! "Pulling without specifying how to reconcile" err
+	test_i18ngrep ! "You have divergent branches" err
 '
 
 test_expect_success 'pull.rebase not set and --rebase given' '
 	git reset --hard c0 &&
 	git pull --rebase . c1 2>err &&
-	test_i18ngrep ! "Pulling without specifying how to reconcile" err
+	test_i18ngrep ! "You have divergent branches" err
 '
 
 test_expect_success 'pull.rebase not set and --no-rebase given' '
 	git reset --hard c0 &&
 	git pull --no-rebase . c1 2>err &&
-	test_i18ngrep ! "Pulling without specifying how to reconcile" err
+	test_i18ngrep ! "You have divergent branches" err
 '
 
 test_expect_success 'pull.rebase not set and --ff given' '
 	git reset --hard c0 &&
 	git pull --ff . c1 2>err &&
-	test_i18ngrep ! "Pulling without specifying how to reconcile" err
+	test_i18ngrep ! "You have divergent branches" err
 '
 
 test_expect_success 'pull.rebase not set and --no-ff given' '
 	git reset --hard c0 &&
 	git pull --no-ff . c1 2>err &&
-	test_i18ngrep ! "Pulling without specifying how to reconcile" err
+	test_i18ngrep ! "You have divergent branches" err
 '
 
 test_expect_success 'pull.rebase not set and --ff-only given' '
 	git reset --hard c0 &&
 	git pull --ff-only . c1 2>err &&
-	test_i18ngrep ! "Pulling without specifying how to reconcile" err
+	test_i18ngrep ! "You have divergent branches" err
 '
 
 test_expect_success 'pull.rebase not set (not-fast-forward)' '
 	git reset --hard c2 &&
-	git -c color.advice=always pull . c1 2>err &&
+	test_must_fail git -c color.advice=always pull . c1 2>err &&
 	test_decode_color <err >decoded &&
 	test_i18ngrep "<YELLOW>hint: " decoded &&
-	test_i18ngrep "Pulling without specifying how to reconcile" decoded
+	test_i18ngrep "You have divergent branches" decoded
 '
 
 test_expect_success 'pull.rebase not set and pull.ff=true (not-fast-forward)' '
 	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 ! "You have divergent branches" err
 '
 
 test_expect_success 'pull.rebase not set and pull.ff=false (not-fast-forward)' '
 	git reset --hard c2 &&
 	test_config pull.ff false &&
 	git pull . c1 2>err &&
-	test_i18ngrep ! "Pulling without specifying how to reconcile" err
+	test_i18ngrep ! "You have divergent branches" err
 '
 
 test_expect_success 'pull.rebase not set and pull.ff=only (not-fast-forward)' '
 	git reset --hard c2 &&
 	test_config pull.ff only &&
 	test_must_fail git pull . c1 2>err &&
-	test_i18ngrep ! "Pulling without specifying how to reconcile" err
+	test_i18ngrep ! "You have divergent branches" err
 '
 
 test_expect_success 'pull.rebase not set and --rebase given (not-fast-forward)' '
 	git reset --hard c2 &&
 	git pull --rebase . c1 2>err &&
-	test_i18ngrep ! "Pulling without specifying how to reconcile" err
+	test_i18ngrep ! "You have divergent branches" err
 '
 
 test_expect_success 'pull.rebase not set and --no-rebase given (not-fast-forward)' '
 	git reset --hard c2 &&
 	git pull --no-rebase . c1 2>err &&
-	test_i18ngrep ! "Pulling without specifying how to reconcile" err
+	test_i18ngrep ! "You have divergent branches" err
 '
 
 test_expect_success 'pull.rebase not set and --ff given (not-fast-forward)' '
 	git reset --hard c2 &&
 	git pull --ff . c1 2>err &&
-	test_i18ngrep ! "Pulling without specifying how to reconcile" err
+	test_i18ngrep ! "You have divergent branches" err
 '
 
 test_expect_success 'pull.rebase not set and --no-ff given (not-fast-forward)' '
 	git reset --hard c2 &&
 	git pull --no-ff . c1 2>err &&
-	test_i18ngrep ! "Pulling without specifying how to reconcile" err
+	test_i18ngrep ! "You have divergent branches" err
 '
 
 test_expect_success 'pull.rebase not set and --ff-only given (not-fast-forward)' '
 	git reset --hard c2 &&
 	test_must_fail git pull --ff-only . c1 2>err &&
-	test_i18ngrep ! "Pulling without specifying how to reconcile" err
+	test_i18ngrep ! "You have divergent branches" err
 '
 
 test_does_rebase() {
diff --git a/t/t7603-merge-reduce-heads.sh b/t/t7603-merge-reduce-heads.sh
index 98948955ae5..27cd94ad6f7 100755
--- a/t/t7603-merge-reduce-heads.sh
+++ b/t/t7603-merge-reduce-heads.sh
@@ -68,7 +68,7 @@ test_expect_success 'merge c1 with c2, c3, c4, c5' '
 
 test_expect_success 'pull c2, c3, c4, c5 into c1' '
 	git reset --hard c1 &&
-	git pull . c2 c3 c4 c5 &&
+	git pull --no-rebase . c2 c3 c4 c5 &&
 	test "$(git rev-parse c1)" != "$(git rev-parse HEAD)" &&
 	test "$(git rev-parse c1)" = "$(git rev-parse HEAD^1)" &&
 	test "$(git rev-parse c2)" = "$(git rev-parse HEAD^2)" &&
-- 
gitgitgadget

[PATCH 8/9] pull: update docs & code for option compatibility with rebasing

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

git-pull.txt includes merge-options.txt, which is written assuming
merges will happen.  git-pull has allowed rebases for many years; update
the documentation to reflect that.

While at it, pass any `--signoff` flag through to the rebase backend too
so that we don't have to document it as merge-specific.  Rebase has
supported the --signoff flag for years now as well.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 Documentation/git-pull.txt      |  2 ++
 Documentation/merge-options.txt | 25 +++++++++++++++++++++++++
 builtin/pull.c                  |  2 ++
 3 files changed, 29 insertions(+)

diff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt
index cad3f6bfcee..6e6e95a7595 100644
--- a/Documentation/git-pull.txt
+++ b/Documentation/git-pull.txt
@@ -125,6 +125,8 @@ When false, merge the current branch into the upstream branch.
 +
 When `interactive`, enable the interactive mode of rebase.
 +
+Note that `--ff-only` takes precedence over any `--rebase` flag.
++
 See `pull.rebase`, `branch.<name>.rebase` and `branch.autoSetupRebase` in
 linkgit:git-config[1] if you want to make `git pull` always use
 `--rebase` instead of merging.
diff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt
index eb0aabd396f..3b70abf3c87 100644
--- a/Documentation/merge-options.txt
+++ b/Documentation/merge-options.txt
@@ -2,6 +2,9 @@
 --no-commit::
 	Perform the merge and commit the result. This option can
 	be used to override --no-commit.
+ifdef::git-pull[]
+	Only useful when merging.
+endif::git-pull[]
 +
 With --no-commit perform the merge and stop just before creating
 a merge commit, to give the user a chance to inspect and further
@@ -58,6 +61,10 @@ could instead be resolved as a fast-forward.
 +
 With `--ff-only`, resolve the merge as a fast-forward when possible.
 When not possible, refuse to merge and exit with a non-zero status.
+ifdef::git-pull[]
++
+Note that `--no-ff` and `--ff` are ignored when rebasing is requested.
+endif::git-pull[]
 
 -S[<keyid>]::
 --gpg-sign[=<keyid>]::
@@ -73,6 +80,9 @@ When not possible, refuse to merge and exit with a non-zero status.
 	In addition to branch names, populate the log message with
 	one-line descriptions from at most <n> actual commits that are being
 	merged. See also linkgit:git-fmt-merge-msg[1].
+ifdef::git-pull[]
+	Only useful when merging.
+endif::git-pull[]
 +
 With --no-log do not list one-line descriptions from the
 actual commits being merged.
@@ -102,10 +112,17 @@ With --no-squash perform the merge and commit the result. This
 option can be used to override --squash.
 +
 With --squash, --commit is not allowed, and will fail.
+ifdef::git-pull[]
++
+Only useful when merging.
+endif::git-pull[]
 
 --no-verify::
 	This option bypasses the pre-merge and commit-msg hooks.
 	See also linkgit:githooks[5].
+ifdef::git-pull[]
+	Only useful when merging.
+endif::git-pull[]
 
 -s <strategy>::
 --strategy=<strategy>::
@@ -127,6 +144,10 @@ With --squash, --commit is not allowed, and will fail.
 	default trust model, this means the signing key has been signed by
 	a trusted key.  If the tip commit of the side branch is not signed
 	with a valid key, the merge is aborted.
+ifdef::git-pull[]
++
+Only useful when merging.
+endif::git-pull[]
 
 --summary::
 --no-summary::
@@ -166,3 +187,7 @@ endif::git-pull[]
 	projects that started their lives independently. As that is
 	a very rare occasion, no configuration variable to enable
 	this by default exists and will not be added.
+ifdef::git-pull[]
++
+Only useful when merging.
+endif::git-pull[]
diff --git a/builtin/pull.c b/builtin/pull.c
index 2d7f2d765ab..3a61b92f328 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -893,6 +893,8 @@ static int run_rebase(const struct object_id *newbase,
 	strvec_pushv(&args, opt_strategy_opts.v);
 	if (opt_gpg_sign)
 		strvec_push(&args, opt_gpg_sign);
+	if (opt_signoff)
+		strvec_push(&args, opt_signoff);
 	if (opt_autostash == 0)
 		strvec_push(&args, "--no-autostash");
 	else if (opt_autostash == 1)
-- 
gitgitgadget

[PATCH 9/9] pull: fix handling of multiple heads

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

With multiple heads, we should not allow rebasing or fast-forwarding.
Also, it seems wrong to have our can_ff computation return true, so fix
that while we are at it too (we won't actually use the can_ff flag due
to setting opt_ff to "--no-ff", but it's confusing to leave it as
computed to be true).

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 builtin/pull.c               | 20 ++++++++++++++++----
 t/t7601-merge-pull-config.sh |  4 ++--
 2 files changed, 18 insertions(+), 6 deletions(-)

diff --git a/builtin/pull.c b/builtin/pull.c
index 3a61b92f328..beaf6ee0653 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -913,12 +913,18 @@ static int run_rebase(const struct object_id *newbase,
 	return ret;
 }
 
-static int get_can_ff(struct object_id *orig_head, struct object_id *orig_merge_head)
+static int get_can_ff(struct object_id *orig_head,
+		      struct oid_array *merge_heads)
 {
 	int ret;
 	struct commit_list *list = NULL;
 	struct commit *merge_head, *head;
+	struct object_id *orig_merge_head;
 
+	if (merge_heads->nr > 1)
+		return 0;
+
+	orig_merge_head = &merge_heads->oid[0];
 	head = lookup_commit_reference(the_repository, orig_head);
 	commit_list_insert(head, &list);
 	merge_head = lookup_commit_reference(the_repository, orig_merge_head);
@@ -1046,10 +1052,16 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 			die(_("Cannot merge multiple branches into empty head."));
 		return pull_into_void(merge_heads.oid, &curr_head);
 	}
-	if (opt_rebase && merge_heads.nr > 1)
-		die(_("Cannot rebase onto multiple branches."));
+	if (merge_heads.nr > 1) {
+		if (opt_rebase)
+			die(_("Cannot rebase onto multiple branches."));
+		if (opt_ff && !strcmp(opt_ff, "--ff-only"))
+			die(_("Cannot fast-forward to multiple branches."));
+		if (!opt_ff)
+			opt_ff = "--no-ff";
+	}
 
-	can_ff = get_can_ff(&orig_head, &merge_heads.oid[0]);
+	can_ff = get_can_ff(&orig_head, &merge_heads);
 
 	/* ff-only takes precedence over rebase */
 	if (opt_ff && !strcmp(opt_ff, "--ff-only")) {
diff --git a/t/t7601-merge-pull-config.sh b/t/t7601-merge-pull-config.sh
index 673b92afbab..29105b5b1ed 100755
--- a/t/t7601-merge-pull-config.sh
+++ b/t/t7601-merge-pull-config.sh
@@ -297,7 +297,7 @@ test_expect_success 'pull.rebase=true takes precedence over --ff' '
 
 # End of precedence rules
 
-test_expect_failure 'Multiple heads does not warn about fast forwarding' '
+test_expect_success 'Multiple heads does not warn about fast forwarding' '
 	git reset --hard c1 &&
 	git pull . c2 c3 2>err &&
 	test_i18ngrep ! "Pulling without specifying how to reconcile" err
@@ -307,7 +307,7 @@ test_expect_success 'Cannot fast-forward with multiple heads' '
 	git reset --hard c0 &&
 	test_must_fail git -c pull.ff=only pull . c1 c2 c3 2>err &&
 	test_i18ngrep ! "Pulling without specifying how to reconcile" err &&
-	test_i18ngrep "Not possible to fast-forward, aborting" err
+	test_i18ngrep "Cannot fast-forward to multiple branches" err
 '
 
 test_expect_success 'Cannot rebase with multiple heads' '
-- 
gitgitgadget

RE: [PATCH 1/9] t7601: add relative precedence tests for merge and rebase flags/options

[Felipe Contreras]

Elijah Newren via GitGitGadget wrote:
> From: Elijah Newren <newren@gmail.com>
> 
> The interaction of rebase and merge flags and options was not well
> tested.  Add several tests to check for correct behavior from the
> following rules:
>     * --ff-only takes precedence over --[no-]rebase
>       * Corollary: pull.ff=only overrides pull.rebase
>     * --rebase[=!false] takes precedence over --no-ff and --ff
>       * Corollary: pull.rebase=!false overrides pull.ff=!only
>     * command line flags take precedence over config, except:
>       * --no-rebase heeds pull.ff=!only
>       * pull.rebase=!false takes precedence over --no-ff and --ff

This is wrong, --ff-only is meant only for merge, not rebase.

You are testing for behavior that 1) has not been agreed to, and 2) is
not documented.

This is what the current documentation says about all the --*ff*
options:

  --ff::
  --no-ff::
  --ff-only::
    Specifies how a merge is handled when the merged-in history is
    already a descendant of the current history.  `--ff` is the
    default unless merging an annotated (and possibly signed) tag
    that is not stored in its natural place in the `refs/tags/`
    hierarchy, in which case `--no-ff` is assumed.
  +
  With `--ff`, when possible resolve the merge as a fast-forward (only
  update the branch pointer to match the merged branch; do not create a
  merge commit).  When not possible (when the merged-in history is not a
  descendant of the current history), create a merge commit.
  +
  With `--no-ff`, create a merge commit in all cases, even when the merge
  could instead be resolved as a fast-forward.
  +
  With `--ff-only`, resolve the merge as a fast-forward when possible.
  When not possible, refuse to merge and exit with a non-zero status.

-- 
Felipe Contreras

RE: [PATCH 2/9] t7601: add tests of interactions with multiple merge heads and config

[Felipe Contreras]

Elijah Newren via GitGitGadget wrote:
> From: Elijah Newren <newren@gmail.com>
> 
> There were already code checking that --rebase was incompatible with
> a merge of multiple heads.  However, we were sometimes throwing warnings
> about lack of specification of rebase vs. merge when given multiple
> heads.  Since rebasing is disallowed with multiple merge heads, that
> seems like a poor warning to print; we should instead just assume
> merging is wanted.

Where is that explained in the documentation?

-- 
Felipe Contreras

RE: [PATCH 3/9] pull: abort if --ff-only is given and fast-forwarding is impossible

[Felipe Contreras]

Alex Henrie via GitGitGadget wrote:
> From: Alex Henrie <alexhenrie24@gmail.com>
> 
> The warning about pulling without specifying how to reconcile divergent
> branches says that after setting pull.rebase to true, --ff-only can
> still be passed on the command line to require a fast-forward. Make that
> actually work.

Once again, this behavior is not explained in the documentation.

And this breaks existing behavior.

A person currently can configure `pull.ff=only` to tell git that by
default she wants `git pull` to fail if the merge is not a fast-forward,
but she can override that default behavior with `git pull --rebase`.

Now that fails as well.

So even though the warning has been saying that users need to specify if
to merge or rebase, and the user has specified a rebase, it now fails,
so it's a step backwards.

-- 
Felipe Contreras

RE: [PATCH 4/9] pull: since --ff-only overrides, handle it first

[Felipe Contreras]

Elijah Newren via GitGitGadget wrote:
> From: Elijah Newren <newren@gmail.com>
> 
> There are both merge and rebase branches in the logic, and previously
> both had to handle fast-forwarding.  Merge handled that implicitly
> (because git merge handles it directly), while in rebase it was
> explicit.  Given that the --ff-only flag is meant to override any
> --rebase or --no-rebase, make the code reflect that by handling
> --ff-only before the merge-vs-rebase logic.
> 
> No functional changes, just making it easier to verify that the codeflow
> matches our precedence rules.

But it does have a functional changes. Now you are calling run_merge()
without update_submodules().

-- 
Felipe Contreras

RE: [PATCH 5/9] pull: ensure --rebase overrides ability to ff

[Felipe Contreras]

Elijah Newren via GitGitGadget wrote:
> From: Elijah Newren <newren@gmail.com>
> 
> Now that the handling of fast-forward-only in combination with rebases
> has been moved before the merge-vs-rebase logic, we have an unnecessary
> special fast-forward case left within the rebase logic.  Actually, more
> than unnecessary, it's actually a violation of the rules.  As per
> https://lore.kernel.org/git/xmqqwnpqot4m.fsf@gitster.g/, --rebase is
> supposed to override all ff flags other than an explicit --ff-only.
> Ensure that it does so by removing the fast-forward special case that
> exists within the rebase logic.

What Junio said in one mail are not the rules.

This goes against what is described in the documentation.

-- 
Felipe Contreras

RE: [PATCH 6/9] pull: make --rebase and --no-rebase override pull.ff=only

[Felipe Contreras]

Elijah Newren via GitGitGadget wrote:
> From: Elijah Newren <newren@gmail.com>
> 
> Fix the last few precedence tests failing in t7601 by now implementing
> the logic to have --[no-]rebase override a pull.ff=only config setting.

This is a semantic change that breaks current behavior.

Now this does something different:

  git -c pull.ff=only pull --merge

*And* the new behavior is not documented anywhere.

-- 
Felipe Contreras

RE: [PATCH 7/9] pull: abort by default when fast-forwarding is not possible

[Felipe Contreras]

Elijah Newren via GitGitGadget wrote:
> From: Elijah Newren <newren@gmail.com>
> 
> 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.

Once again. We have not warned the users of this upcoming change a
*single* day. We have not told them how to turn the new behavior, and
how to keep the old behavior.

If you want to guarantee user complaints, this is the way to do it.

-- 
Felipe Contreras

RE: [PATCH 0/9] Handle pull option precedence

[Felipe Contreras]

Elijah Newren via GitGitGadget wrote:
> Based on a recent list of rules for flag/option precedence for git-pull[1]
> from Junio (particularly focusing on rebase vs. merge vs. fast-forward),
> here's an attempt to implement and document it. Given multiple recent
> surprises from users about some of these behaviors[2][3] and a coworker just
> yesterday expressing some puzzlement with git-pull and rebase vs. merge, it
> seems like a good time to address some of these issues.
> 
> Since the handling of conflicting options was holding up two of Alex's
> patches[4][5], and his patches fix some of the tests, I also include those
> two patches in my series, with a few small changes to the first (so I've
> kept him as author) and more substantial changes to the second (so I've
> given him an Initial-patch-by attribution).

Once again this is the wrong approach.

  1. It's inconsistent with the current documentation
  2. It treats configuration and command line arguments differently when
     the documentation says they are the same
  3. It breaks behavior users are currently relying on
  4. It changes default behavior without warning users that such
     behavior is going to change (0 days of deprecation)

-- 
Felipe Contreras

Re: [PATCH 1/9] t7601: add relative precedence tests for merge and rebase flags/options

[Junio C Hamano]

"Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:

> +test_does_rebase() {

Style: missing SP before ().

> +	git reset --hard c2 &&
> +	git "$@" . c1 &&

OK.

It is clear, from the "." (this repository), that this is designed
to test nothing but "git pull", and it is somewhat unfortunate that
we cannot spell out 'git pull' here (instead we ask the callers to
always pass 'pull' subcommand from the command line), but that is
understandable, as the primary reason we lack 'pull' from this
command line is because we expect to pass one-shot configuration to
the command via "-c pull.rebase=X".

> +	# Check that we actually did a rebase
> +	git rev-list --count HEAD >actual &&
> +	git rev-list --merges --count HEAD >>actual &&
> +	test_write_lines 3 0 >expect &&

The answer is 3 and zero.  OK.  The point of having this helper is
not because we want to do "pull --rebase" of different histories on
top of different base, but because we want to ensure that with
different configuration and command line options, the same "pull"
will result in the same flattening rebase.  So it is not a problem
at all that these numbers are hardcoded (it might make it less
fragile to count only commits above c2, but it probably would not
matter).

> +	test_cmp expect actual &&
> +	rm actual expect
> +}

OK.

> +test_does_merge_noff() {
> +	git reset --hard c0 &&
> +	git "$@" . c1 &&
> +	# Check that we actually did a merge
> +	git rev-list --count HEAD >actual &&
> +	git rev-list --merges --count HEAD >>actual &&
> +	test_write_lines 3 1 >expect &&
> +	test_cmp expect actual &&
> +	rm actual expect
> +}
> +
> +test_does_merge_ff() {
> +	git reset --hard c0 &&
> +	git "$@" . c1 &&
> +	# Check that we actually did a merge
> +	git rev-list --count HEAD >actual &&
> +	git rev-list --merges --count HEAD >>actual &&
> +	test_write_lines 2 0 >expect &&
> +	test_cmp expect actual &&
> +	rm actual expect
> +}
> +
> +test_does_need_full_merge() {
> +	git reset --hard c2 &&
> +	git "$@" . c1 &&
> +	# Check that we actually did a merge
> +	git rev-list --count HEAD >actual &&
> +	git rev-list --merges --count HEAD >>actual &&
> +	test_write_lines 4 1 >expect &&
> +	test_cmp expect actual &&
> +	rm actual expect
> +}
> +
> +test_attempts_fast_forward() {
> +	git reset --hard c2 &&
> +	test_must_fail git "$@" . c1 2>err &&
> +	test_i18ngrep "Not possible to fast-forward, aborting" err
> +}

The same reasoning says these test_does_X helpers make sense.  I am
not sure about the name does_need_full_merge though---what does it
want to ensure is not very clear to me.  Is it named that way because
you found "test_does_merge" (when contrasted to "test_does_merge_ff")
sounds too weak?

> +#
> +# Rule 1: --ff-only takes precedence over --[no-]rebase
> +# (Corollary: pull.ff=only overrides pull.rebase)
> +#
> +test_expect_failure '--ff-only takes precedence over --rebase' '
> +	test_attempts_fast_forward pull --rebase --ff-only
> +'
> +
> +test_expect_failure '--ff-only takes precedence over --rebase even if first' '
> +	test_attempts_fast_forward pull --ff-only --rebase
> +'
> +
> +test_expect_success '--ff-only takes precedence over --no-rebase' '
> +	test_attempts_fast_forward pull --ff-only --no-rebase
> +'
> +
> +test_expect_failure 'pull.ff=only overrides pull.rebase=true' '
> +	test_attempts_fast_forward -c pull.ff=only -c pull.rebase=true pull
> +'
> +
> +test_expect_success 'pull.ff=only overrides pull.rebase=false' '
> +	test_attempts_fast_forward -c pull.ff=only -c pull.rebase=false pull
> +'

OK.  These all ensure that when the history does not fast-forward,
the command will fail when --ff-only tells us to allow only
fast-forward.  I am not sure "takes precedence" is a meaningful
label, though.  It is more like "ff-only means ff-only and fails
when the upstream history is not a descendant, no matter how the
possible integration is set to be performed".

> +# Rule 2: --rebase=[!false] takes precedence over --no-ff and --ff
> +# (Corollary: pull.rebase=!false overrides pull.ff=!only)
> +test_expect_success '--rebase takes precedence over --no-ff' '
> +	test_does_rebase pull --rebase --no-ff
> +'
> +
> +test_expect_success '--rebase takes precedence over --ff' '
> +	test_does_rebase pull --rebase --ff
> +'
> +
> +test_expect_success 'pull.rebase=true takes precedence over pull.ff=false' '
> +	test_does_rebase -c pull.rebase=true -c pull.ff=false pull
> +'
> +
> +test_expect_success 'pull.rebase=true takes precedence over pull.ff=true' '
> +	test_does_rebase -c pull.rebase=true -c pull.ff=true pull
> +'

Sounds sensible.  Again, I do not view this as precedence, though.
"--ff" is merely "if there is nothing else needs to be done, it is
OK to fast-forward to their history", so with --rebase, it either
(1) gets ignored when we have something to be done, i.e. our own
development to replay on top of their history, or (2) becomes a
no-op as there truly isn't any development of our own.

And "--no-ff" is more or less a meaningless thing to say ("I do not
want to just fast-forward when I do not have anything meaningful to
add, I want an empty merge commit to mark where I was") in the
context of "--rebase".  Erroring out only when their histroy is
descendant of ours and "--no-ff" and "--rebase=<set>" are given
explicitly from the command line might make sense, but I do not
think of a sensible corrective action the end-user wants to do after
seeing such an error (after all, there was nothing to rebase on top
of their history), so I think ignoring is a more acceptable outcome
when we have nothing to replay.

Do we ensure that "pull --rebase --ff" fast-forwards when the
history truly fast-forwards?  test_does_rebase only and always
checks what happens when pulling c1 into c2 and nothing else, so I
do not think the above are testing that case.

IOW, I think "test_does_merge_ff pull --rebase --ff" wants to be
there somewhere?

> +# Rule 3: command line flags take precedence over config
> +test_expect_failure '--ff-only takes precedence over pull.rebase=true' '
> +	test_attempts_fast_forward -c pull.rebase=true pull --ff-only
> +'
> +
> +test_expect_success '--ff-only takes precedence over pull.rebase=false' '
> +	test_attempts_fast_forward -c pull.rebase=false pull --ff-only
> +'

Both are good.

> +test_expect_failure '--no-rebase overrides pull.ff=only' '
> +	test_does_need_full_merge -c pull.ff=only pull --no-rebase
> +'
> +
> +test_expect_success '--rebase takes precedence over pull.ff=only' '
> +	test_does_rebase -c pull.ff=only pull --rebase
> +'

OK.

> +test_expect_success '--rebase takes precedence over pull.ff=true' '
> +	test_does_rebase -c pull.ff=true pull --rebase
> +'
> +
> +test_expect_success '--rebase takes precedence over pull.ff=false' '
> +	test_does_rebase -c pull.ff=false pull --rebase
> +'
> +
> +test_expect_success '--rebase takes precedence over pull.ff unset' '
> +	test_does_rebase pull --rebase
> +'

These three are correct but again I do not see them as precedence
matter.

> +# Rule 4: --no-rebase heeds pull.ff=!only or explict --ff or --no-ff
> +
> +test_expect_success '--no-rebase works with --no-ff' '
> +	test_does_merge_noff pull --no-rebase --no-ff
> +'

OK.

> +test_expect_success '--no-rebase works with --ff' '
> +	test_does_merge_ff pull --no-rebase --ff
> +'

OK.

> +test_expect_success '--no-rebase does ff if pull.ff unset' '
> +	test_does_merge_ff pull --no-rebase
> +'

OK.

> +test_expect_success '--no-rebase heeds pull.ff=true' '
> +	test_does_merge_ff -c pull.ff=true pull --no-rebase
> +'

OK (pull.ff=true is the default anyway).

> +test_expect_success '--no-rebase heeds pull.ff=false' '
> +	test_does_merge_noff -c pull.ff=false pull --no-rebase
> +'

OK.

> +# Rule 5: pull.rebase=!false takes precedence over --no-ff and --ff
> +test_expect_success 'pull.rebase=true takes precedence over --no-ff' '
> +	test_does_rebase -c pull.rebase=true pull --no-ff
> +'

OK.  When we do have our own commits they do not have, there is no
point in letting --no-ff do anything special.  It would be sensible
to replay ours on top of theirs.


> +test_expect_success 'pull.rebase=true takes precedence over --ff' '
> +	test_does_rebase -c pull.rebase=true pull --ff
> +'

Again, I am not sure if this is "precedence" issue.  "ff" merely
means "fast-forwarding is allowed, when we do not have anything to
add to their history", and we do have our own work in the test
scenario test_does_rebase presents us, so rebasing would be quite
sensible.  Similarly

    test_does_need_full_merge -c pull.rebase=false pull --ff

would be true, right?

> +# End of precedence rules
> +
>  test_expect_success 'merge c1 with c2' '
>  	git reset --hard c1 &&
>  	test -f c0.c &&

The series of new tests makes me wonder if there is a good way to
ensure we covered full matrix, but I didn't see any that smelled
wrong.

Thanks.

Re: [PATCH 1/9] t7601: add relative precedence tests for merge and rebase flags/options

[Elijah Newren]

On Mon, Jul 19, 2021 at 11:23 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > +test_does_rebase() {
>
> Style: missing SP before ().

Will fix.

...
> > +test_does_merge_noff() {
> > +     git reset --hard c0 &&
> > +     git "$@" . c1 &&
> > +     # Check that we actually did a merge
> > +     git rev-list --count HEAD >actual &&
> > +     git rev-list --merges --count HEAD >>actual &&
> > +     test_write_lines 3 1 >expect &&
> > +     test_cmp expect actual &&
> > +     rm actual expect
> > +}
> > +
> > +test_does_merge_ff() {
> > +     git reset --hard c0 &&
> > +     git "$@" . c1 &&
> > +     # Check that we actually did a merge
> > +     git rev-list --count HEAD >actual &&
> > +     git rev-list --merges --count HEAD >>actual &&
> > +     test_write_lines 2 0 >expect &&
> > +     test_cmp expect actual &&
> > +     rm actual expect
> > +}
> > +
> > +test_does_need_full_merge() {
> > +     git reset --hard c2 &&
> > +     git "$@" . c1 &&
> > +     # Check that we actually did a merge
> > +     git rev-list --count HEAD >actual &&
> > +     git rev-list --merges --count HEAD >>actual &&
> > +     test_write_lines 4 1 >expect &&
> > +     test_cmp expect actual &&
> > +     rm actual expect
> > +}
> > +
> > +test_attempts_fast_forward() {
> > +     git reset --hard c2 &&
> > +     test_must_fail git "$@" . c1 2>err &&
> > +     test_i18ngrep "Not possible to fast-forward, aborting" err
> > +}
>
> The same reasoning says these test_does_X helpers make sense.  I am
> not sure about the name does_need_full_merge though---what does it
> want to ensure is not very clear to me.  Is it named that way because
> you found "test_does_merge" (when contrasted to "test_does_merge_ff")
> sounds too weak?

I should probably rename the functions for clarity; I'm testing the
matrix of ff/noff possibilities:

# Prefers merge over fast-forward; rename from test_does_merge
test_does_merge_when_ff_possible ()

# Prefers fast-forward over merge; rename from test_does_merge_ff
test_does_fast_forward ()

# Attempts fast forward and bails since it is impossible
test_attempts_fast_forward ()

# Does a merge when a fast forward is not possible
test_falls_back_to_full_merge ()


In particular, `test_does_need_full_merge` would become
`test_falls_back_to_full_merge`


> > +#
> > +# Rule 1: --ff-only takes precedence over --[no-]rebase
> > +# (Corollary: pull.ff=only overrides pull.rebase)
> > +#
> > +test_expect_failure '--ff-only takes precedence over --rebase' '
> > +     test_attempts_fast_forward pull --rebase --ff-only
> > +'
> > +
> > +test_expect_failure '--ff-only takes precedence over --rebase even if first' '
> > +     test_attempts_fast_forward pull --ff-only --rebase
> > +'
> > +
> > +test_expect_success '--ff-only takes precedence over --no-rebase' '
> > +     test_attempts_fast_forward pull --ff-only --no-rebase
> > +'
> > +
> > +test_expect_failure 'pull.ff=only overrides pull.rebase=true' '
> > +     test_attempts_fast_forward -c pull.ff=only -c pull.rebase=true pull
> > +'
> > +
> > +test_expect_success 'pull.ff=only overrides pull.rebase=false' '
> > +     test_attempts_fast_forward -c pull.ff=only -c pull.rebase=false pull
> > +'
>
> OK.  These all ensure that when the history does not fast-forward,
> the command will fail when --ff-only tells us to allow only
> fast-forward.  I am not sure "takes precedence" is a meaningful
> label, though.  It is more like "ff-only means ff-only and fails
> when the upstream history is not a descendant, no matter how the
> possible integration is set to be performed".

So, I think you're saying you view fast-forwards as a subset of valid
rebases (and fast-forwards are also a subset of valid rmerges), and
thus you view --ff-only --rebase as an instruction to only proceed if
both command line flags can be satisfied.

That makes sense, but I don't know how to put that into a test
description that isn't ridiculously long.  I tried replacing "takes
precedence over" with just "overrides" but you might not like that
either.  If you've got better wording for the comments before each
group and the test descriptions, I'm all ears.  Otherwise I'll just
take my best stab at it.

> > +# Rule 2: --rebase=[!false] takes precedence over --no-ff and --ff
> > +# (Corollary: pull.rebase=!false overrides pull.ff=!only)
> > +test_expect_success '--rebase takes precedence over --no-ff' '
> > +     test_does_rebase pull --rebase --no-ff
> > +'
> > +
> > +test_expect_success '--rebase takes precedence over --ff' '
> > +     test_does_rebase pull --rebase --ff
> > +'
> > +
> > +test_expect_success 'pull.rebase=true takes precedence over pull.ff=false' '
> > +     test_does_rebase -c pull.rebase=true -c pull.ff=false pull
> > +'
> > +
> > +test_expect_success 'pull.rebase=true takes precedence over pull.ff=true' '
> > +     test_does_rebase -c pull.rebase=true -c pull.ff=true pull
> > +'
>
> Sounds sensible.  Again, I do not view this as precedence, though.
> "--ff" is merely "if there is nothing else needs to be done, it is
> OK to fast-forward to their history", so with --rebase, it either
> (1) gets ignored when we have something to be done, i.e. our own
> development to replay on top of their history, or (2) becomes a
> no-op as there truly isn't any development of our own.
>
> And "--no-ff" is more or less a meaningless thing to say ("I do not
> want to just fast-forward when I do not have anything meaningful to
> add, I want an empty merge commit to mark where I was") in the
> context of "--rebase".  Erroring out only when their histroy is
> descendant of ours and "--no-ff" and "--rebase=<set>" are given
> explicitly from the command line might make sense, but I do not
> think of a sensible corrective action the end-user wants to do after
> seeing such an error (after all, there was nothing to rebase on top
> of their history), so I think ignoring is a more acceptable outcome
> when we have nothing to replay.
>
> Do we ensure that "pull --rebase --ff" fast-forwards when the
> history truly fast-forwards?  test_does_rebase only and always
> checks what happens when pulling c1 into c2 and nothing else, so I
> do not think the above are testing that case.
>
> IOW, I think "test_does_merge_ff pull --rebase --ff" wants to be
> there somewhere?

Sounds good; I added "--rebase --ff fast-forwards when possible" test.

...
> > +test_expect_success '--rebase takes precedence over pull.ff=true' '
> > +     test_does_rebase -c pull.ff=true pull --rebase
> > +'
> > +
> > +test_expect_success '--rebase takes precedence over pull.ff=false' '
> > +     test_does_rebase -c pull.ff=false pull --rebase
> > +'
> > +
> > +test_expect_success '--rebase takes precedence over pull.ff unset' '
> > +     test_does_rebase pull --rebase
> > +'
>
> These three are correct but again I do not see them as precedence
> matter.
>
...
>
> > +test_expect_success 'pull.rebase=true takes precedence over --ff' '
> > +     test_does_rebase -c pull.rebase=true pull --ff
> > +'
>
> Again, I am not sure if this is "precedence" issue.  "ff" merely
> means "fast-forwarding is allowed, when we do not have anything to
> add to their history", and we do have our own work in the test
> scenario test_does_rebase presents us, so rebasing would be quite
> sensible.  Similarly
>
>     test_does_need_full_merge -c pull.rebase=false pull --ff
>
> would be true, right?

Yep, I added that test since you thought to ask to make sure it's covered.

> > +# End of precedence rules
> > +
> >  test_expect_success 'merge c1 with c2' '
> >       git reset --hard c1 &&
> >       test -f c0.c &&
>
> The series of new tests makes me wonder if there is a good way to
> ensure we covered full matrix, but I didn't see any that smelled
> wrong.

Fair question.  I covered all the ones in
https://lore.kernel.org/git/xmqqwnpqot4m.fsf@gitster.g/, except that
when '*' was given, I might have just picked a representative example
rather than all possible options.  I also just used --rebase as a
proxy for any --rebase=* option other than --rebase=false.  Of course,
that table may have been incomplete but it at least covered the ones I
could think of.

Re: [PATCH 1/9] t7601: add relative precedence tests for merge and rebase flags/options

[Junio C Hamano]

Elijah Newren <newren@gmail.com> writes:

>> OK.  These all ensure that when the history does not fast-forward,
>> the command will fail when --ff-only tells us to allow only
>> fast-forward.  I am not sure "takes precedence" is a meaningful
>> label, though.  It is more like "ff-only means ff-only and fails
>> when the upstream history is not a descendant, no matter how the
>> possible integration is set to be performed".
>
> So, I think you're saying you view fast-forwards as a subset of valid
> rebases (and fast-forwards are also a subset of valid rmerges), and
> thus you view --ff-only --rebase as an instruction to only proceed if
> both command line flags can be satisfied.

Ah, I didn't think of it myself, but now you put it in these words,
I do agree that the view makes sense.  When we have nothing of our
own, a degenerated form of a rebase is a fast-forward, even more so
than a fast-forward being a degenerated form of a merge.

> That makes sense, but I don't know how to put that into a test
> description that isn't ridiculously long.

Me neither.  Let's not waste too much brain-cycles over this.

Thanks.

Re: [PATCH 1/9] t7601: add relative precedence tests for merge and rebase flags/options

[Felipe Contreras]

Junio C Hamano wrote:
> "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:

> > +test_expect_failure '--no-rebase overrides pull.ff=only' '
> > +	test_does_need_full_merge -c pull.ff=only pull --no-rebase
> > +'
> > +
> > +test_expect_success '--rebase takes precedence over pull.ff=only' '
> > +	test_does_rebase -c pull.ff=only pull --rebase
> > +'
> 
> OK.

Since when is it OK to test for a new behavior that 1. will break
compatibility, 2. is not documented, and 3. has zero consensus?

-- 
Felipe Contreras

Re: [PATCH 2/9] t7601: add tests of interactions with multiple merge heads and config

[Junio C Hamano]

"Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:

> +test_expect_failure 'Multiple heads does not warn about fast forwarding' '
> +	git reset --hard c1 &&
> +	git pull . c2 c3 2>err &&
> +	test_i18ngrep ! "Pulling without specifying how to reconcile" err
> +'

This does not look like "warning about fast-forwarding".

But more importantly, are we sure we want to expect this outcome?

We are at c1 and try to integrate with c2 and c3 at the same time,
neither of which is a descendant of c1.

We know that the only possible action is to create an octopus in
this case, and that it is pretty much fundamental (i.e. it is not
like "rebase" with further development will be able to handle this
case).  I however do not know if it is also obvious to total newbies
who haven't even chosen between merge and rebase.  I can see them
complaining "why didn't I get asked to choose between rebase and
merge" if we went ahead and created an octopus merge, especially the
ones who would choose pull.rebase=yes once they learned Git a bit
more.

> +test_expect_success 'Cannot fast-forward with multiple heads' '
> +	git reset --hard c0 &&
> +	test_must_fail git -c pull.ff=only pull . c1 c2 c3 2>err &&
> +	test_i18ngrep ! "Pulling without specifying how to reconcile" err &&
> +	test_i18ngrep "Not possible to fast-forward, aborting" err
> +'

This one looks sensible to me.

> +test_expect_success 'Cannot rebase with multiple heads' '
> +	git reset --hard c0 &&
> +	test_must_fail git -c pull.rebase=true pull . c1 c2 c3 2>err &&
> +	test_i18ngrep ! "Pulling without specifying how to reconcile" err &&
> +	test_i18ngrep "Cannot rebase onto multiple branches." err
> +'

This one, too.

Thanks.

Re: [PATCH 5/9] pull: ensure --rebase overrides ability to ff

[Junio C Hamano]

"Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Elijah Newren <newren@gmail.com>
>
> Now that the handling of fast-forward-only in combination with rebases
> has been moved before the merge-vs-rebase logic, we have an unnecessary
> special fast-forward case left within the rebase logic.

It is correct to say that we could call run_rebase() and it will do
the right thing, even when can_ff is true, in this codepath.

But I am not sure if you want to do this as a part of this series.

The code in question comes from 33b842a1 (pull: fast-forward "pull
--rebase=true", 2016-06-29).  It was meant as a mere optimization
to avoid calling run_rebase() when we know we have nothing to replay
on top of their head after we fast-forward, and doing a pure
fast-forward is easier by calling run_merge().

In other words, the code this patch touches should not have anything
to do with the rebase-vs-fast-forward logic.

Now, if a benchmarking tells us that there is no difference between
calling an empty run_rebase() and run_merge(), I'd be perfectly fine
with this change, with the justification that we are "discarding an
earlier optimization that has become irrelevant".  But that would be
totally outside the theme of this topic, I would think.

Thanks.

> @@ -1070,13 +1070,7 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
>  		    submodule_touches_in_range(the_repository, &upstream, &curr_head))
>  			die(_("cannot rebase with locally recorded submodule modifications"));
>  
> -		if (can_ff) {
> -			/* we can fast-forward this without invoking rebase */
> -			opt_ff = "--ff-only";
> -			ret = run_merge();
> -		} else {
> -			ret = run_rebase(&newbase, &upstream);
> -		}
> +		ret = run_rebase(&newbase, &upstream);
>  
>  		if (!ret && (recurse_submodules == RECURSE_SUBMODULES_ON ||
>  			     recurse_submodules == RECURSE_SUBMODULES_ON_DEMAND))
> diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
> index e2c0c510222..4b50488141f 100755
> --- a/t/t5520-pull.sh
> +++ b/t/t5520-pull.sh
> @@ -295,7 +295,7 @@ test_expect_success '--rebase (merge) fast forward' '
>  	# The above only validates the result.  Did we actually bypass rebase?
>  	git reflog -1 >reflog.actual &&
>  	sed "s/^[0-9a-f][0-9a-f]*/OBJID/" reflog.actual >reflog.fuzzy &&
> -	echo "OBJID HEAD@{0}: pull --rebase . ff: Fast-forward" >reflog.expected &&
> +	echo "OBJID HEAD@{0}: pull --rebase . ff (finish): returning to refs/heads/to-rebase" >reflog.expected &&
>  	test_cmp reflog.expected reflog.fuzzy
>  '
>  
> @@ -307,8 +307,8 @@ test_expect_success '--rebase (am) fast forward' '
>  
>  	# The above only validates the result.  Did we actually bypass rebase?
>  	git reflog -1 >reflog.actual &&
> -	sed "s/^[0-9a-f][0-9a-f]*/OBJID/" reflog.actual >reflog.fuzzy &&
> -	echo "OBJID HEAD@{0}: pull --rebase . ff: Fast-forward" >reflog.expected &&
> +	sed -e "s/^[0-9a-f][0-9a-f]*/OBJID/" -e "s/[0-9a-f][0-9a-f]*$/OBJID/" reflog.actual >reflog.fuzzy &&
> +	echo "OBJID HEAD@{0}: rebase finished: refs/heads/to-rebase onto OBJID" >reflog.expected &&
>  	test_cmp reflog.expected reflog.fuzzy
>  '

Re: [PATCH 6/9] pull: make --rebase and --no-rebase override pull.ff=only

[Junio C Hamano]

"Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Elijah Newren <newren@gmail.com>
>
> Fix the last few precedence tests failing in t7601 by now implementing
> the logic to have --[no-]rebase override a pull.ff=only config setting.

Because we only have opt_rebase and opt_ff, unless we are in a very
early part of the code, we cannot tell if "opt_rebase >= 0" we see
came from the command line or from the configuration, and because
pull.rebase, pull.ff, --rebase=<kind>, and <--ff|--ff-only|--no-ff>
have intricate interactions among them, we'd probably need to behave
differently depending on where "opt_rebase >= 0" came from.

The new code works only because we haven't called config_get_rebase()
yet in this part.  If we swapped the order of "if ff is not set,
then figure it out from the configuration" (the code you are
extending) and "if rebase is not set, read from the configuration",
then the logic would not work.

The resulting code looks correct, but it is subtle and may deserve
an in-code comment.

> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
>  builtin/pull.c               | 5 ++++-
>  t/t7601-merge-pull-config.sh | 4 ++--
>  2 files changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/builtin/pull.c b/builtin/pull.c
> index 5ba376a7487..7e7c90f6a10 100644
> --- a/builtin/pull.c
> +++ b/builtin/pull.c
> @@ -966,8 +966,11 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
>  
>  	parse_repo_refspecs(argc, argv, &repo, &refspecs);
>  
> -	if (!opt_ff)
> +	if (!opt_ff) {
>  		opt_ff = xstrdup_or_null(config_get_ff());

		/*
		 * Do we have --rebase=<kind> or --no-rebase from the
		 * command line (note that we haven't read pull.rebase yet)?
		 * If so, defeat the configured pull.ff=only; instead of
		 * failing to integrate divergent histories, we want the
		 * rebase or merge to happen.
		 */

> +		if (opt_rebase >= 0 && opt_ff && !strcmp(opt_ff, "--ff-only"))
> +			opt_ff = "--ff";
> +	}

Thanks.

Re: [PATCH 6/9] pull: make --rebase and --no-rebase override pull.ff=only

[Felipe Contreras]

Junio C Hamano wrote:
> "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
> > From: Elijah Newren <newren@gmail.com>
> >
> > Fix the last few precedence tests failing in t7601 by now implementing
> > the logic to have --[no-]rebase override a pull.ff=only config setting.
> 
> Because we only have opt_rebase and opt_ff, unless we are in a very
> early part of the code, we cannot tell if "opt_rebase >= 0" we see
> came from the command line or from the configuration, and because
> pull.rebase, pull.ff, --rebase=<kind>, and <--ff|--ff-only|--no-ff>
> have intricate interactions among them, we'd probably need to behave
> differently depending on where "opt_rebase >= 0" came from.

Where is that backwards-incompatible change explained in the
documentation?

-- 
Felipe Contreras

Re: [PATCH 5/9] pull: ensure --rebase overrides ability to ff

[Elijah Newren]

On Tue, Jul 20, 2021 at 4:35 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > From: Elijah Newren <newren@gmail.com>
> >
> > Now that the handling of fast-forward-only in combination with rebases
> > has been moved before the merge-vs-rebase logic, we have an unnecessary
> > special fast-forward case left within the rebase logic.
>
> It is correct to say that we could call run_rebase() and it will do
> the right thing, even when can_ff is true, in this codepath.
>
> But I am not sure if you want to do this as a part of this series.

It turns out my commit message was wrong, and so was the patch.  My
changes did have a functional change, substituting one form of
submodule breakage with another.

I'll replace this with a patch that fixes the submodule issue.  (When
--ff-only is specified, we should not rebase submodules.  We should
fast-forward both the parent module and the submodules.)

Re: [PATCH 8/9] pull: update docs & code for option compatibility with rebasing

[Junio C Hamano]

"Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Elijah Newren <newren@gmail.com>
>
> git-pull.txt includes merge-options.txt, which is written assuming
> merges will happen.  git-pull has allowed rebases for many years; update
> the documentation to reflect that.

Yes, as I might have said elsewhere recently, "pull --rebase" has
been afterthought and the way how it interacts with many niche
"features" of "git pull" has never been designed but started with
whatever the code happened to do and then fixed/tweaked as people
found it suboptimal.  Hopefully with this series we are covering
these interactions and whipping them into a much better shape with
coherent semantics.

> diff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt
> index cad3f6bfcee..6e6e95a7595 100644
> --- a/Documentation/git-pull.txt
> +++ b/Documentation/git-pull.txt
> @@ -125,6 +125,8 @@ When false, merge the current branch into the upstream branch.
>  +
>  When `interactive`, enable the interactive mode of rebase.
>  +
> +Note that `--ff-only` takes precedence over any `--rebase` flag.
> ++

"`ff-only` overrides any `--rebase` flag"?

> @@ -58,6 +61,10 @@ could instead be resolved as a fast-forward.
>  +
>  With `--ff-only`, resolve the merge as a fast-forward when possible.
>  When not possible, refuse to merge and exit with a non-zero status.

This part may want to become conditional with git-pull[] to use a
verb different than "merge"?

> +ifdef::git-pull[]
> ++
> +Note that `--no-ff` and `--ff` are ignored when rebasing is requested.
> +endif::git-pull[]

OK.

Re: [PATCH 8/9] pull: update docs & code for option compatibility with rebasing

[Elijah Newren]

On Tue, Jul 20, 2021 at 5:17 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > From: Elijah Newren <newren@gmail.com>
> >
> > git-pull.txt includes merge-options.txt, which is written assuming
> > merges will happen.  git-pull has allowed rebases for many years; update
> > the documentation to reflect that.
>
> Yes, as I might have said elsewhere recently, "pull --rebase" has
> been afterthought and the way how it interacts with many niche
> "features" of "git pull" has never been designed but started with
> whatever the code happened to do and then fixed/tweaked as people
> found it suboptimal.  Hopefully with this series we are covering
> these interactions and whipping them into a much better shape with
> coherent semantics.
>
> > diff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt
> > index cad3f6bfcee..6e6e95a7595 100644
> > --- a/Documentation/git-pull.txt
> > +++ b/Documentation/git-pull.txt
> > @@ -125,6 +125,8 @@ When false, merge the current branch into the upstream branch.
> >  +
> >  When `interactive`, enable the interactive mode of rebase.
> >  +
> > +Note that `--ff-only` takes precedence over any `--rebase` flag.
> > ++
>
> "`ff-only` overrides any `--rebase` flag"?

Yeah, ignore that, I had already ripped it out after your comments on
patch 1 yesterday.  I've also got more changes to the pull
documentation to try to do more of what your sentence above says,
"covering these interactions and whipping them into a much better
shape."

> > @@ -58,6 +61,10 @@ could instead be resolved as a fast-forward.
> >  +
> >  With `--ff-only`, resolve the merge as a fast-forward when possible.
> >  When not possible, refuse to merge and exit with a non-zero status.
>
> This part may want to become conditional with git-pull[] to use a
> verb different than "merge"?

Yep, that's one of the other changes I'm adding and will send out soon.

> > +ifdef::git-pull[]
> > ++
> > +Note that `--no-ff` and `--ff` are ignored when rebasing is requested.
> > +endif::git-pull[]
>
> OK.

Re: [PATCH 2/9] t7601: add tests of interactions with multiple merge heads and config

[Elijah Newren]

On Tue, Jul 20, 2021 at 4:11 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > +test_expect_failure 'Multiple heads does not warn about fast forwarding' '
> > +     git reset --hard c1 &&
> > +     git pull . c2 c3 2>err &&
> > +     test_i18ngrep ! "Pulling without specifying how to reconcile" err
> > +'
>
> This does not look like "warning about fast-forwarding".
>
> But more importantly, are we sure we want to expect this outcome?
>
> We are at c1 and try to integrate with c2 and c3 at the same time,
> neither of which is a descendant of c1.
>
> We know that the only possible action is to create an octopus in
> this case, and that it is pretty much fundamental (i.e. it is not
> like "rebase" with further development will be able to handle this
> case).  I however do not know if it is also obvious to total newbies
> who haven't even chosen between merge and rebase.  I can see them
> complaining "why didn't I get asked to choose between rebase and
> merge" if we went ahead and created an octopus merge, especially the
> ones who would choose pull.rebase=yes once they learned Git a bit
> more.

That's a fair point; I'll modify the test accordingly (and update the
description).

> > +test_expect_success 'Cannot fast-forward with multiple heads' '
> > +     git reset --hard c0 &&
> > +     test_must_fail git -c pull.ff=only pull . c1 c2 c3 2>err &&
> > +     test_i18ngrep ! "Pulling without specifying how to reconcile" err &&
> > +     test_i18ngrep "Not possible to fast-forward, aborting" err
> > +'
>
> This one looks sensible to me.
>
> > +test_expect_success 'Cannot rebase with multiple heads' '
> > +     git reset --hard c0 &&
> > +     test_must_fail git -c pull.rebase=true pull . c1 c2 c3 2>err &&
> > +     test_i18ngrep ! "Pulling without specifying how to reconcile" err &&
> > +     test_i18ngrep "Cannot rebase onto multiple branches." err
> > +'
>
> This one, too.
>
> Thanks.

Re: [PATCH 8/9] pull: update docs & code for option compatibility with rebasing

[Junio C Hamano]

Elijah Newren <newren@gmail.com> writes:

>> > +Note that `--ff-only` takes precedence over any `--rebase` flag.
>> > ++
>>
>> "`ff-only` overrides any `--rebase` flag"?
>
> Yeah, ignore that, I had already ripped it out after your comments on
> patch 1 yesterday.  I've also got more changes to the pull
> documentation to try to do more of what your sentence above says,
> "covering these interactions and whipping them into a much better
> shape."

OK.  After thinking about it further, "overrides" is not the word,
either, I guess.  It is more like that there are "merge", "rebase"
and "fast-forward-only" for the user to choose from, with "merge"
having two sub-variants (i.e. what happens when we do not have any
development of our own) and "rebase" with a few sub-variants
(i.e. interactive? shape-preserving?).

In any case, when the user says "I'll take nothing but fast-forward
update to their history", it is a bug if we did not fail when their
history is not a descendant of ours, and we have already fixed it in
an earlier part of the series.  This would be a good place to
clarify what the correct behaviour ought to be, if the existing
documentation is not already.

Thanks.

[PATCH v2 0/8] Handle pull option precedence

[Elijah Newren via GitGitGadget]

Based on a recent list of rules for flag/option precedence for git-pull[1]
from Junio (particularly focusing on rebase vs. merge vs. fast-forward),
here's an attempt to implement and document it. Given multiple recent
surprises from users about some of these behaviors[2][3] and a coworker just
yesterday expressing some puzzlement with git-pull and rebase vs. merge, it
seems like a good time to address some of these issues.

Since the handling of conflicting options was holding up two of Alex's
patches[4][5], and his patches fix some of the tests, I also include those
two patches in my series, with a few small changes to the first (so I've
kept him as author) and more substantial changes to the second (so I've
given him an Initial-patch-by attribution).

Changes since v1:

 * Rebased on latest master (resolved a simple conflict with
   dd/test-stdout-count-lines)
 * Patch 1: based on feedback from Junio, fixed some style issues, clarified
   function names, added a few new tests, and took a stab at fixing up the
   comments and test descriptions (but still unsure if I hit the mark on the
   last point)
 * Patch 2: changed the test expectations for one of the multiple head tests
   as per Junio's suggestion, and made one of the other tests expect a more
   specific error message
 * Patches 4 & 5 were squashed and fixed: these now address a submodule bug
   interaction with --ff-only
 * Old patch 6 (now 5): added a code comment explaining a subtle point
 * Old patch 8 (now 7): a few more documentation updates, especially making
   --ff-only not sound merge-specific
 * Old patch 9 (now 8): Updates for new test expectation from patch 2

Quick overview:

 * Patches 1-2: new testcases (see the commit messages for the rules)
 * Patch 3: Alex's recent patch (abort if --ff-only but can't do so)
 * Patches 4-6: fix the precedence parts Alex didn't cover
 * Patch 7: Alex's other patch, abort if rebase vs. merge not specified
 * Patch 8: Compatibility of git-pull with merge-options.txt (think
   rebasing)
 * Patch 9: Fix multiple heads handling too

[1] https://lore.kernel.org/git/xmqqwnpqot4m.fsf@gitster.g/ [2]
https://lore.kernel.org/git/CAL3xRKdOyVWvcLXK7zoXtFPiHBjgL24zi5hhg+3yjowwSUPgmg@mail.gmail.com/
[3]
https://lore.kernel.org/git/c62933fb-96b2-99f5-7169-372f486f6e39@aixigo.com/
[4]
https://lore.kernel.org/git/20210711012604.947321-1-alexhenrie24@gmail.com/
[5]
https://lore.kernel.org/git/20210627000855.530985-1-alexhenrie24@gmail.com/

Alex Henrie (1):
  pull: abort if --ff-only is given and fast-forwarding is impossible

Elijah Newren (7):
  t7601: test interaction of merge/rebase/fast-forward flags and options
  t7601: add tests of interactions with multiple merge heads and config
  pull: since --ff-only overrides, handle it first
  pull: make --rebase and --no-rebase override pull.ff=only
  pull: abort by default when fast-forwarding is not possible
  pull: update docs & code for option compatibility with rebasing
  pull: fix handling of multiple heads

 Documentation/git-merge.txt     |   2 +
 Documentation/git-pull.txt      |  21 +--
 Documentation/merge-options.txt |  40 ++++++
 advice.c                        |   5 +
 advice.h                        |   1 +
 builtin/merge.c                 |   2 +-
 builtin/pull.c                  |  63 ++++++---
 t/t4013-diff-various.sh         |   2 +-
 t/t5520-pull.sh                 |  26 ++--
 t/t5521-pull-options.sh         |   4 +-
 t/t5524-pull-msg.sh             |   4 +-
 t/t5553-set-upstream.sh         |  14 +-
 t/t5604-clone-reference.sh      |   4 +-
 t/t6402-merge-rename.sh         |  18 +--
 t/t6409-merge-subtree.sh        |   6 +-
 t/t6417-merge-ours-theirs.sh    |  10 +-
 t/t7601-merge-pull-config.sh    | 244 +++++++++++++++++++++++++++++---
 t/t7603-merge-reduce-heads.sh   |   2 +-
 18 files changed, 375 insertions(+), 93 deletions(-)


base-commit: daab8a564f8bbac55f70f8bf86c070e001a9b006
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1049%2Fnewren%2Fhandle-pull-option-precedence-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1049/newren/handle-pull-option-precedence-v2
Pull-Request: https://github.com/git/git/pull/1049

Range-diff vs v1:

  1:  6cb771297f5 !  1:  17560927211 t7601: add relative precedence tests for merge and rebase flags/options
     @@ Metadata
      Author: Elijah Newren <newren@gmail.com>
      
       ## Commit message ##
     -    t7601: add relative precedence tests for merge and rebase flags/options
     +    t7601: test interaction of merge/rebase/fast-forward flags and options
      
          The interaction of rebase and merge flags and options was not well
          tested.  Add several tests to check for correct behavior from the
          following rules:
     -        * --ff-only takes precedence over --[no-]rebase
     -          * Corollary: pull.ff=only overrides pull.rebase
     -        * --rebase[=!false] takes precedence over --no-ff and --ff
     -          * Corollary: pull.rebase=!false overrides pull.ff=!only
     +        * --ff-only vs. --[no-]rebase
     +          (and the related pull.ff=only vs. pull.rebase)
     +        * --rebase[=!false] vs. --no-ff and --ff
     +          (and the related pull.rebase=!false overrides pull.ff=!only)
              * command line flags take precedence over config, except:
                * --no-rebase heeds pull.ff=!only
     -          * pull.rebase=!false takes precedence over --no-ff and --ff
     +          * pull.rebase=!false vs --no-ff and --ff
      
          For more details behind these rules and a larger table of individual
          cases, refer to https://lore.kernel.org/git/xmqqwnpqot4m.fsf@gitster.g/
     @@ t/t7601-merge-pull-config.sh: test_expect_success 'pull.rebase not set and --ff-
       	test_i18ngrep ! "Pulling without specifying how to reconcile" err
       '
       
     -+test_does_rebase() {
     ++test_does_rebase () {
      +	git reset --hard c2 &&
      +	git "$@" . c1 &&
      +	# Check that we actually did a rebase
     @@ t/t7601-merge-pull-config.sh: test_expect_success 'pull.rebase not set and --ff-
      +	rm actual expect
      +}
      +
     -+test_does_merge_noff() {
     ++# Prefers merge over fast-forward
     ++test_does_merge_when_ff_possible () {
      +	git reset --hard c0 &&
      +	git "$@" . c1 &&
      +	# Check that we actually did a merge
     @@ t/t7601-merge-pull-config.sh: test_expect_success 'pull.rebase not set and --ff-
      +	rm actual expect
      +}
      +
     -+test_does_merge_ff() {
     ++# Prefers fast-forward over merge or rebase
     ++test_does_fast_forward () {
      +	git reset --hard c0 &&
      +	git "$@" . c1 &&
     -+	# Check that we actually did a merge
     ++
     ++	# Check that we did not get any merges
      +	git rev-list --count HEAD >actual &&
      +	git rev-list --merges --count HEAD >>actual &&
      +	test_write_lines 2 0 >expect &&
      +	test_cmp expect actual &&
     ++
     ++	# Check that we ended up at c1
     ++	git rev-parse HEAD >actual &&
     ++	git rev-parse c1^{commit} >expect &&
     ++	test_cmp actual expect &&
     ++
     ++	# Remove temporary files
      +	rm actual expect
      +}
      +
     -+test_does_need_full_merge() {
     ++# Doesn't fail when fast-forward not possible; does a merge
     ++test_falls_back_to_full_merge () {
      +	git reset --hard c2 &&
      +	git "$@" . c1 &&
      +	# Check that we actually did a merge
     @@ t/t7601-merge-pull-config.sh: test_expect_success 'pull.rebase not set and --ff-
      +	rm actual expect
      +}
      +
     -+test_attempts_fast_forward() {
     ++# Attempts fast forward, which is impossible, and bails
     ++test_attempts_fast_forward () {
      +	git reset --hard c2 &&
      +	test_must_fail git "$@" . c1 2>err &&
      +	test_i18ngrep "Not possible to fast-forward, aborting" err
      +}
      +
      +#
     -+# Rule 1: --ff-only takes precedence over --[no-]rebase
     -+# (Corollary: pull.ff=only overrides pull.rebase)
     ++# Group 1: Interaction of --ff-only with --[no-]rebase
     ++# (And related interaction of pull.ff=only with pull.rebase)
      +#
     -+test_expect_failure '--ff-only takes precedence over --rebase' '
     ++test_expect_failure '--ff-only overrides --rebase' '
      +	test_attempts_fast_forward pull --rebase --ff-only
      +'
      +
     -+test_expect_failure '--ff-only takes precedence over --rebase even if first' '
     ++test_expect_failure '--ff-only overrides --rebase even if first' '
      +	test_attempts_fast_forward pull --ff-only --rebase
      +'
      +
     -+test_expect_success '--ff-only takes precedence over --no-rebase' '
     ++test_expect_success '--ff-only overrides --no-rebase' '
      +	test_attempts_fast_forward pull --ff-only --no-rebase
      +'
      +
     @@ t/t7601-merge-pull-config.sh: test_expect_success 'pull.rebase not set and --ff-
      +	test_attempts_fast_forward -c pull.ff=only -c pull.rebase=false pull
      +'
      +
     -+# Rule 2: --rebase=[!false] takes precedence over --no-ff and --ff
     -+# (Corollary: pull.rebase=!false overrides pull.ff=!only)
     -+test_expect_success '--rebase takes precedence over --no-ff' '
     ++# Group 2: --rebase=[!false] overrides --no-ff and --ff
     ++# (And related interaction of pull.rebase=!false and pull.ff=!only)
     ++test_expect_success '--rebase overrides --no-ff' '
      +	test_does_rebase pull --rebase --no-ff
      +'
      +
     -+test_expect_success '--rebase takes precedence over --ff' '
     ++test_expect_success '--rebase overrides --ff' '
      +	test_does_rebase pull --rebase --ff
      +'
      +
     -+test_expect_success 'pull.rebase=true takes precedence over pull.ff=false' '
     ++test_expect_success '--rebase fast-forwards when possible' '
     ++	test_does_fast_forward pull --rebase --ff
     ++'
     ++
     ++test_expect_success 'pull.rebase=true overrides pull.ff=false' '
      +	test_does_rebase -c pull.rebase=true -c pull.ff=false pull
      +'
      +
     -+test_expect_success 'pull.rebase=true takes precedence over pull.ff=true' '
     ++test_expect_success 'pull.rebase=true overrides pull.ff=true' '
      +	test_does_rebase -c pull.rebase=true -c pull.ff=true pull
      +'
      +
     -+# Rule 3: command line flags take precedence over config
     ++# Group 3: command line flags take precedence over config
      +test_expect_failure '--ff-only takes precedence over pull.rebase=true' '
      +	test_attempts_fast_forward -c pull.rebase=true pull --ff-only
      +'
     @@ t/t7601-merge-pull-config.sh: test_expect_success 'pull.rebase not set and --ff-
      +	test_attempts_fast_forward -c pull.rebase=false pull --ff-only
      +'
      +
     -+test_expect_failure '--no-rebase overrides pull.ff=only' '
     -+	test_does_need_full_merge -c pull.ff=only pull --no-rebase
     ++test_expect_failure '--no-rebase takes precedence over pull.ff=only' '
     ++	test_falls_back_to_full_merge -c pull.ff=only pull --no-rebase
      +'
      +
      +test_expect_success '--rebase takes precedence over pull.ff=only' '
      +	test_does_rebase -c pull.ff=only pull --rebase
      +'
      +
     -+test_expect_success '--rebase takes precedence over pull.ff=true' '
     ++test_expect_success '--rebase overrides pull.ff=true' '
      +	test_does_rebase -c pull.ff=true pull --rebase
      +'
      +
     -+test_expect_success '--rebase takes precedence over pull.ff=false' '
     ++test_expect_success '--rebase overrides pull.ff=false' '
      +	test_does_rebase -c pull.ff=false pull --rebase
      +'
      +
     -+test_expect_success '--rebase takes precedence over pull.ff unset' '
     ++test_expect_success '--rebase overrides pull.ff unset' '
      +	test_does_rebase pull --rebase
      +'
      +
     -+# Rule 4: --no-rebase heeds pull.ff=!only or explict --ff or --no-ff
     ++# Group 4: --no-rebase heeds pull.ff=!only or explict --ff or --no-ff
      +
      +test_expect_success '--no-rebase works with --no-ff' '
     -+	test_does_merge_noff pull --no-rebase --no-ff
     ++	test_does_merge_when_ff_possible pull --no-rebase --no-ff
      +'
      +
      +test_expect_success '--no-rebase works with --ff' '
     -+	test_does_merge_ff pull --no-rebase --ff
     ++	test_does_fast_forward pull --no-rebase --ff
      +'
      +
      +test_expect_success '--no-rebase does ff if pull.ff unset' '
     -+	test_does_merge_ff pull --no-rebase
     ++	test_does_fast_forward pull --no-rebase
      +'
      +
      +test_expect_success '--no-rebase heeds pull.ff=true' '
     -+	test_does_merge_ff -c pull.ff=true pull --no-rebase
     ++	test_does_fast_forward -c pull.ff=true pull --no-rebase
      +'
      +
      +test_expect_success '--no-rebase heeds pull.ff=false' '
     -+	test_does_merge_noff -c pull.ff=false pull --no-rebase
     ++	test_does_merge_when_ff_possible -c pull.ff=false pull --no-rebase
      +'
      +
     -+# Rule 5: pull.rebase=!false takes precedence over --no-ff and --ff
     -+test_expect_success 'pull.rebase=true takes precedence over --no-ff' '
     ++# Group 5: pull.rebase=!false in combination with --no-ff or --ff
     ++test_expect_success 'pull.rebase=true and --no-ff' '
      +	test_does_rebase -c pull.rebase=true pull --no-ff
      +'
      +
     -+test_expect_success 'pull.rebase=true takes precedence over --ff' '
     ++test_expect_success 'pull.rebase=true and --ff' '
      +	test_does_rebase -c pull.rebase=true pull --ff
      +'
      +
     -+# End of precedence rules
     ++test_expect_success 'pull.rebase=false and --no-ff' '
     ++	test_does_merge_when_ff_possible -c pull.rebase=false pull --no-ff
     ++'
     ++
     ++test_expect_success 'pull.rebase=false and --ff, ff possible' '
     ++	test_does_fast_forward -c pull.rebase=false pull --ff
     ++'
     ++
     ++test_expect_success 'pull.rebase=false and --ff, ff not possible' '
     ++	test_falls_back_to_full_merge -c pull.rebase=false pull --ff
     ++'
     ++
     ++# End of groupings for conflicting merge vs. rebase flags/options
      +
       test_expect_success 'merge c1 with c2' '
       	git reset --hard c1 &&
  2:  329802382bf !  2:  66fe7f7f934 t7601: add tests of interactions with multiple merge heads and config
     @@ Commit message
          Signed-off-by: Elijah Newren <newren@gmail.com>
      
       ## t/t7601-merge-pull-config.sh ##
     -@@ t/t7601-merge-pull-config.sh: test_expect_success 'pull.rebase=true takes precedence over --ff' '
     +@@ t/t7601-merge-pull-config.sh: test_expect_success 'pull.rebase=false and --ff, ff not possible' '
       
     - # End of precedence rules
     + # End of groupings for conflicting merge vs. rebase flags/options
       
     -+test_expect_failure 'Multiple heads does not warn about fast forwarding' '
     ++test_expect_failure 'Multiple heads warns about inability to fast forward' '
      +	git reset --hard c1 &&
     -+	git pull . c2 c3 2>err &&
     -+	test_i18ngrep ! "Pulling without specifying how to reconcile" err
     ++	test_must_fail git pull . c2 c3 2>err &&
     ++	test_i18ngrep "Pulling without specifying how to reconcile" err
      +'
      +
     -+test_expect_success 'Cannot fast-forward with multiple heads' '
     ++test_expect_failure 'Multiple can never be fast forwarded' '
      +	git reset --hard c0 &&
      +	test_must_fail git -c pull.ff=only pull . c1 c2 c3 2>err &&
      +	test_i18ngrep ! "Pulling without specifying how to reconcile" err &&
     -+	test_i18ngrep "Not possible to fast-forward, aborting" err
     ++	# In addition to calling out "cannot fast-forward", we very much
     ++	# want the "multiple branches" piece to be called out to users.
     ++	test_i18ngrep "Cannot fast-forward to multiple branches" err
      +'
      +
      +test_expect_success 'Cannot rebase with multiple heads' '
  3:  ae54afd8b01 !  3:  c45cd239666 pull: abort if --ff-only is given and fast-forwarding is impossible
     @@ builtin/pull.c: int cmd_pull(int argc, const char **argv, const char *prefix)
       	if (opt_rebase) {
      
       ## t/t7601-merge-pull-config.sh ##
     -@@ t/t7601-merge-pull-config.sh: test_attempts_fast_forward() {
     - # Rule 1: --ff-only takes precedence over --[no-]rebase
     - # (Corollary: pull.ff=only overrides pull.rebase)
     +@@ t/t7601-merge-pull-config.sh: test_attempts_fast_forward () {
     + # Group 1: Interaction of --ff-only with --[no-]rebase
     + # (And related interaction of pull.ff=only with pull.rebase)
       #
     --test_expect_failure '--ff-only takes precedence over --rebase' '
     -+test_expect_success '--ff-only takes precedence over --rebase' '
     +-test_expect_failure '--ff-only overrides --rebase' '
     ++test_expect_success '--ff-only overrides --rebase' '
       	test_attempts_fast_forward pull --rebase --ff-only
       '
       
     --test_expect_failure '--ff-only takes precedence over --rebase even if first' '
     -+test_expect_success '--ff-only takes precedence over --rebase even if first' '
     +-test_expect_failure '--ff-only overrides --rebase even if first' '
     ++test_expect_success '--ff-only overrides --rebase even if first' '
       	test_attempts_fast_forward pull --ff-only --rebase
       '
       
     -@@ t/t7601-merge-pull-config.sh: test_expect_success '--ff-only takes precedence over --no-rebase' '
     +@@ t/t7601-merge-pull-config.sh: test_expect_success '--ff-only overrides --no-rebase' '
       	test_attempts_fast_forward pull --ff-only --no-rebase
       '
       
     @@ t/t7601-merge-pull-config.sh: test_expect_success '--ff-only takes precedence ov
       	test_attempts_fast_forward -c pull.ff=only -c pull.rebase=true pull
       '
       
     -@@ t/t7601-merge-pull-config.sh: test_expect_success 'pull.rebase=true takes precedence over pull.ff=true' '
     +@@ t/t7601-merge-pull-config.sh: test_expect_success 'pull.rebase=true overrides pull.ff=true' '
       '
       
     - # Rule 3: command line flags take precedence over config
     + # Group 3: command line flags take precedence over config
      -test_expect_failure '--ff-only takes precedence over pull.rebase=true' '
      +test_expect_success '--ff-only takes precedence over pull.rebase=true' '
       	test_attempts_fast_forward -c pull.rebase=true pull --ff-only
       '
       
     -@@ t/t7601-merge-pull-config.sh: test_expect_failure '--no-rebase overrides pull.ff=only' '
     - 	test_does_need_full_merge -c pull.ff=only pull --no-rebase
     +@@ t/t7601-merge-pull-config.sh: test_expect_failure '--no-rebase takes precedence over pull.ff=only' '
     + 	test_falls_back_to_full_merge -c pull.ff=only pull --no-rebase
       '
       
      -test_expect_success '--rebase takes precedence over pull.ff=only' '
  4:  de4b460b09d <  -:  ----------- pull: since --ff-only overrides, handle it first
  5:  3d9ff69198e !  4:  1a821d3b1dd pull: ensure --rebase overrides ability to ff
     @@ Metadata
      Author: Elijah Newren <newren@gmail.com>
      
       ## Commit message ##
     -    pull: ensure --rebase overrides ability to ff
     +    pull: since --ff-only overrides, handle it first
      
     -    Now that the handling of fast-forward-only in combination with rebases
     -    has been moved before the merge-vs-rebase logic, we have an unnecessary
     -    special fast-forward case left within the rebase logic.  Actually, more
     -    than unnecessary, it's actually a violation of the rules.  As per
     -    https://lore.kernel.org/git/xmqqwnpqot4m.fsf@gitster.g/, --rebase is
     -    supposed to override all ff flags other than an explicit --ff-only.
     -    Ensure that it does so by removing the fast-forward special case that
     -    exists within the rebase logic.
     +    There are both merge and rebase branches in the logic, and previously
     +    both had to handle fast-forwarding.  Merge handled that implicitly
     +    (because git merge handles it directly), while in rebase it was
     +    explicit.  Given that the --ff-only flag is meant to override any
     +    --rebase or --no-rebase, make the code reflect that by handling
     +    --ff-only before the merge-vs-rebase logic.
     +
     +    It turns out that this also fixes a bug for submodules.  Previously,
     +    when --ff-only was given, the code would run `merge --ff-only` on the
     +    main module, and then run `submodule update --recursive --rebase` on the
     +    submodules.  With this change, we still run `merge --ff-only` on the
     +    main module, but now run `submodule update --recursive --checkout` on
     +    the submodules.  I believe this better reflects the intent of --ff-only
     +    to have it apply to both the main module and the submodules.
     +
     +    (Sidenote: It is somewhat interesting that all merges pass `--checkout`
     +    to submodule update, even when `--no-ff` is specified, meaning that it
     +    will only do fast-forward merges for submodules.  This was discussed in
     +    commit a6d7eb2c7a ("pull: optionally rebase submodules (remote submodule
     +    changes only)", 2017-06-23).  The same limitations apply now as then, so
     +    we are not trying to fix this at this time.)
      
          Signed-off-by: Elijah Newren <newren@gmail.com>
      
       ## builtin/pull.c ##
      @@ builtin/pull.c: int cmd_pull(int argc, const char **argv, const char *prefix)
     + 
     + 	can_ff = get_can_ff(&orig_head, &merge_heads.oid[0]);
     + 
     +-	if (!can_ff) {
     +-		if (opt_ff) {
     +-			if (!strcmp(opt_ff, "--ff-only"))
     +-				die_ff_impossible();
     +-		} else {
     +-			if (rebase_unspecified && opt_verbosity >= 0)
     +-				show_advice_pull_non_ff();
     +-		}
     ++	/* ff-only takes precedence over rebase */
     ++	if (opt_ff && !strcmp(opt_ff, "--ff-only")) {
     ++		if (!can_ff)
     ++			die_ff_impossible();
     ++		opt_rebase = REBASE_FALSE;
     + 	}
     ++	/* If no action specified and we can't fast forward, then warn. */
     ++	if (!opt_ff && rebase_unspecified && !can_ff)
     ++		show_advice_pull_non_ff();
     + 
     + 	if (opt_rebase) {
     + 		int ret = 0;
     +@@ builtin/pull.c: int cmd_pull(int argc, const char **argv, const char *prefix)
       		    submodule_touches_in_range(the_repository, &upstream, &curr_head))
       			die(_("cannot rebase with locally recorded submodule modifications"));
       
  6:  b379fea097d !  5:  9b116f3d284 pull: make --rebase and --no-rebase override pull.ff=only
     @@ builtin/pull.c: int cmd_pull(int argc, const char **argv, const char *prefix)
      -	if (!opt_ff)
      +	if (!opt_ff) {
       		opt_ff = xstrdup_or_null(config_get_ff());
     ++		/*
     ++		 * A subtle point: opt_ff was set on the line above via
     ++		 * reading from config.  opt_rebase, in contrast, is set
     ++		 * before this point via command line options.  The setting
     ++		 * of opt_rebase via reading from config (using
     ++		 * config_get_rebase()) does not happen until later.  We
     ++		 * are relying on the next if-condition happening before
     ++		 * the config_get_rebase() call so that an explicit
     ++		 * "--rebase" can override a config setting of
     ++		 * pull.ff=only.
     ++		 */
      +		if (opt_rebase >= 0 && opt_ff && !strcmp(opt_ff, "--ff-only"))
      +			opt_ff = "--ff";
      +	}
     @@ t/t7601-merge-pull-config.sh: test_expect_success '--ff-only takes precedence ov
       	test_attempts_fast_forward -c pull.rebase=false pull --ff-only
       '
       
     --test_expect_failure '--no-rebase overrides pull.ff=only' '
     -+test_expect_success '--no-rebase overrides pull.ff=only' '
     - 	test_does_need_full_merge -c pull.ff=only pull --no-rebase
     +-test_expect_failure '--no-rebase takes precedence over pull.ff=only' '
     ++test_expect_success '--no-rebase takes precedence over pull.ff=only' '
     + 	test_falls_back_to_full_merge -c pull.ff=only pull --no-rebase
       '
       
      -test_expect_failure '--rebase takes precedence over pull.ff=only' '
  7:  dca0455898a !  6:  f061f8b4e75 pull: abort by default when fast-forwarding is not possible
     @@ builtin/pull.c: static int get_can_ff(struct object_id *orig_head, struct object
       		 "  git config pull.rebase false  # merge (the default strategy)\n"
       		 "  git config pull.rebase true   # rebase\n"
      @@ builtin/pull.c: int cmd_pull(int argc, const char **argv, const char *prefix)
     - 			return run_merge();
     + 		opt_rebase = REBASE_FALSE;
       	}
       	/* If no action specified and we can't fast forward, then warn. */
      -	if (!opt_ff && rebase_unspecified && !can_ff)
     @@ t/t6402-merge-rename.sh: test_expect_success 'setup' '
      -	test_expect_code 1 git pull . white &&
      +	test_expect_code 1 git pull --no-rebase . white &&
       	git ls-files -s &&
     - 	git ls-files -u B >b.stages &&
     - 	test_line_count = 3 b.stages &&
     + 	test_stdout_line_count = 3 git ls-files -u B &&
     + 	test_stdout_line_count = 1 git ls-files -s N &&
      @@ t/t6402-merge-rename.sh: test_expect_success 'pull renaming branch into another renaming one' \
       	rm -f B &&
       	git reset --hard &&
       	git checkout red &&
      -	test_expect_code 1 git pull . white &&
      +	test_expect_code 1 git pull --no-rebase . white &&
     - 	git ls-files -u B >b.stages &&
     - 	test_line_count = 3 b.stages &&
     - 	git ls-files -s N >n.stages &&
     + 	test_stdout_line_count = 3 git ls-files -u B &&
     + 	test_stdout_line_count = 1 git ls-files -s N &&
     + 	sed -ne "/^g/{
      @@ t/t6402-merge-rename.sh: test_expect_success 'pull unrenaming branch into renaming one' \
       '
       	git reset --hard &&
       	git show-branch &&
      -	test_expect_code 1 git pull . main &&
      +	test_expect_code 1 git pull --no-rebase . main &&
     - 	git ls-files -u B >b.stages &&
     - 	test_line_count = 3 b.stages &&
     - 	git ls-files -s N >n.stages &&
     + 	test_stdout_line_count = 3 git ls-files -u B &&
     + 	test_stdout_line_count = 1 git ls-files -s N &&
     + 	sed -ne "/^g/{
      @@ t/t6402-merge-rename.sh: test_expect_success 'pull conflicting renames' \
       '
       	git reset --hard &&
       	git show-branch &&
      -	test_expect_code 1 git pull . blue &&
      +	test_expect_code 1 git pull --no-rebase . blue &&
     - 	git ls-files -u A >a.stages &&
     - 	test_line_count = 1 a.stages &&
     - 	git ls-files -u B >b.stages &&
     + 	test_stdout_line_count = 1 git ls-files -u A &&
     + 	test_stdout_line_count = 1 git ls-files -u B &&
     + 	test_stdout_line_count = 1 git ls-files -u C &&
      @@ t/t6402-merge-rename.sh: test_expect_success 'interference with untracked working tree file' '
       	git reset --hard &&
       	git show-branch &&
     @@ t/t7601-merge-pull-config.sh: test_expect_success 'setup' '
      +	test_i18ngrep ! "You have divergent branches" err
       '
       
     - test_does_rebase() {
     + test_does_rebase () {
     +@@ t/t7601-merge-pull-config.sh: test_expect_success 'pull.rebase=false and --ff, ff not possible' '
     + 
     + # End of groupings for conflicting merge vs. rebase flags/options
     + 
     +-test_expect_failure 'Multiple heads warns about inability to fast forward' '
     ++test_expect_success 'Multiple heads warns about inability to fast forward' '
     + 	git reset --hard c1 &&
     + 	test_must_fail git pull . c2 c3 2>err &&
     +-	test_i18ngrep "Pulling without specifying how to reconcile" err
     ++	test_i18ngrep "You have divergent branches" err
     + '
     + 
     + test_expect_failure 'Multiple can never be fast forwarded' '
     + 	git reset --hard c0 &&
     + 	test_must_fail git -c pull.ff=only pull . c1 c2 c3 2>err &&
     +-	test_i18ngrep ! "Pulling without specifying how to reconcile" err &&
     ++	test_i18ngrep ! "You have divergent branches" err &&
     + 	# In addition to calling out "cannot fast-forward", we very much
     + 	# want the "multiple branches" piece to be called out to users.
     + 	test_i18ngrep "Cannot fast-forward to multiple branches" err
     +@@ t/t7601-merge-pull-config.sh: test_expect_failure 'Multiple can never be fast forwarded' '
     + test_expect_success 'Cannot rebase with multiple heads' '
     + 	git reset --hard c0 &&
     + 	test_must_fail git -c pull.rebase=true pull . c1 c2 c3 2>err &&
     +-	test_i18ngrep ! "Pulling without specifying how to reconcile" err &&
     ++	test_i18ngrep ! "You have divergent branches" err &&
     + 	test_i18ngrep "Cannot rebase onto multiple branches." err
     + '
     + 
      
       ## t/t7603-merge-reduce-heads.sh ##
      @@ t/t7603-merge-reduce-heads.sh: test_expect_success 'merge c1 with c2, c3, c4, c5' '
  8:  d1952f014f2 !  7:  90d49e0fb78 pull: update docs & code for option compatibility with rebasing
     @@ Commit message
      
          Signed-off-by: Elijah Newren <newren@gmail.com>
      
     + ## Documentation/git-merge.txt ##
     +@@ Documentation/git-merge.txt: merge has resulted in conflicts.
     + 
     + OPTIONS
     + -------
     ++:git-merge: 1
     ++
     + include::merge-options.txt[]
     + 
     + -m <msg>::
     +
       ## Documentation/git-pull.txt ##
     -@@ Documentation/git-pull.txt: When false, merge the current branch into the upstream branch.
     - +
     - When `interactive`, enable the interactive mode of rebase.
     - +
     -+Note that `--ff-only` takes precedence over any `--rebase` flag.
     -++
     - See `pull.rebase`, `branch.<name>.rebase` and `branch.autoSetupRebase` in
     - linkgit:git-config[1] if you want to make `git pull` always use
     - `--rebase` instead of merging.
     +@@ Documentation/git-pull.txt: 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`, `--ff`, or `--rebase`
     +-(or the corresponding configuration options in `pull.ff` or
     +-`pull.rebase`).
     ++reconcile the divergent branches with `--rebase` or `--no-rebase` (or
     ++the corresponding configuration option in `pull.rebase`).
     + 
     + More precisely, `git pull` runs `git fetch` with the given parameters
     + and then depending on configuration options or command line flags,
     +-will call either `git merge` or `git rebase` to reconcile diverging
     ++will call either `git rebase` or `git merge` to reconcile diverging
     + branches.
     + 
     + <repository> should be the name of a remote repository as
     +@@ Documentation/git-pull.txt: published that history already.  Do *not* use this option
     + unless you have read linkgit:git-rebase[1] carefully.
     + 
     + --no-rebase::
     +-	Override earlier --rebase.
     ++	This is shorthand for --rebase=false.
     + 
     + Options related to fetching
     + ~~~~~~~~~~~~~~~~~~~~~~~~~~~
      
       ## Documentation/merge-options.txt ##
      @@
     @@ Documentation/merge-options.txt
       +
       With --no-commit perform the merge and stop just before creating
       a merge commit, to give the user a chance to inspect and further
     -@@ Documentation/merge-options.txt: could instead be resolved as a fast-forward.
     +@@ Documentation/merge-options.txt: set to `no` at the beginning of them.
     + 	to `MERGE_MSG` before being passed on to the commit machinery in the
     + 	case of a merge conflict.
     + 
     ++ifdef::git-merge[]
     + --ff::
     + --no-ff::
     + --ff-only::
     +@@ Documentation/merge-options.txt: set to `no` at the beginning of them.
     + 	default unless merging an annotated (and possibly signed) tag
     + 	that is not stored in its natural place in the `refs/tags/`
     + 	hierarchy, in which case `--no-ff` is assumed.
     ++endif::git-merge[]
     ++ifdef::git-pull[]
     ++--ff-only::
     ++	Only update to the new history if there is no divergent local
     ++	history.  This is the default when no method for reconciling
     ++	divergent histories is provided (via the --rebase=* flags).
     ++
     ++--ff::
     ++--no-ff::
     ++	When merging rather than rebasing, specifies how a merge is
     ++	handled when the merged-in history is already a descendant of
     ++	the current history.  If merging is requested, `--ff` is the
     ++	default unless merging an annotated (and possibly signed) tag
     ++	that is not stored in its natural place in the `refs/tags/`
     ++	hierarchy, in which case `--no-ff` is assumed.
     ++endif::git-pull[]
     + +
     + With `--ff`, when possible resolve the merge as a fast-forward (only
     + update the branch pointer to match the merged branch; do not create a
     +@@ Documentation/merge-options.txt: descendant of the current history), create a merge commit.
     + +
     + With `--no-ff`, create a merge commit in all cases, even when the merge
     + could instead be resolved as a fast-forward.
     ++ifdef::git-merge[]
       +
       With `--ff-only`, resolve the merge as a fast-forward when possible.
       When not possible, refuse to merge and exit with a non-zero status.
     -+ifdef::git-pull[]
     -++
     -+Note that `--no-ff` and `--ff` are ignored when rebasing is requested.
     -+endif::git-pull[]
     ++endif::git-merge[]
       
       -S[<keyid>]::
       --gpg-sign[=<keyid>]::
  9:  3d8df246772 !  8:  f03b15b7eb0 pull: fix handling of multiple heads
     @@ Commit message
          pull: fix handling of multiple heads
      
          With multiple heads, we should not allow rebasing or fast-forwarding.
     -    Also, it seems wrong to have our can_ff computation return true, so fix
     -    that while we are at it too (we won't actually use the can_ff flag due
     -    to setting opt_ff to "--no-ff", but it's confusing to leave it as
     -    computed to be true).
     +    Make sure any fast-forward request calls out specifically the fact that
     +    multiple branches are in play.  Also, since we cannot fast-forward to
     +    multiple branches, fix our computation of can_ff.
      
          Signed-off-by: Elijah Newren <newren@gmail.com>
      
     @@ builtin/pull.c: int cmd_pull(int argc, const char **argv, const char *prefix)
      +			die(_("Cannot rebase onto multiple branches."));
      +		if (opt_ff && !strcmp(opt_ff, "--ff-only"))
      +			die(_("Cannot fast-forward to multiple branches."));
     -+		if (!opt_ff)
     -+			opt_ff = "--no-ff";
      +	}
       
      -	can_ff = get_can_ff(&orig_head, &merge_heads.oid[0]);
     @@ builtin/pull.c: int cmd_pull(int argc, const char **argv, const char *prefix)
       	if (opt_ff && !strcmp(opt_ff, "--ff-only")) {
      
       ## t/t7601-merge-pull-config.sh ##
     -@@ t/t7601-merge-pull-config.sh: test_expect_success 'pull.rebase=true takes precedence over --ff' '
     - 
     - # End of precedence rules
     +@@ t/t7601-merge-pull-config.sh: test_expect_success 'Multiple heads warns about inability to fast forward' '
     + 	test_i18ngrep "You have divergent branches" err
     + '
       
     --test_expect_failure 'Multiple heads does not warn about fast forwarding' '
     -+test_expect_success 'Multiple heads does not warn about fast forwarding' '
     - 	git reset --hard c1 &&
     - 	git pull . c2 c3 2>err &&
     - 	test_i18ngrep ! "Pulling without specifying how to reconcile" err
     -@@ t/t7601-merge-pull-config.sh: test_expect_success 'Cannot fast-forward with multiple heads' '
     +-test_expect_failure 'Multiple can never be fast forwarded' '
     ++test_expect_success 'Multiple can never be fast forwarded' '
       	git reset --hard c0 &&
       	test_must_fail git -c pull.ff=only pull . c1 c2 c3 2>err &&
     - 	test_i18ngrep ! "Pulling without specifying how to reconcile" err &&
     --	test_i18ngrep "Not possible to fast-forward, aborting" err
     -+	test_i18ngrep "Cannot fast-forward to multiple branches" err
     - '
     - 
     - test_expect_success 'Cannot rebase with multiple heads' '
     + 	test_i18ngrep ! "You have divergent branches" err &&

-- 
gitgitgadget

[PATCH v2 1/8] t7601: test interaction of merge/rebase/fast-forward flags and options

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

The interaction of rebase and merge flags and options was not well
tested.  Add several tests to check for correct behavior from the
following rules:
    * --ff-only vs. --[no-]rebase
      (and the related pull.ff=only vs. pull.rebase)
    * --rebase[=!false] vs. --no-ff and --ff
      (and the related pull.rebase=!false overrides pull.ff=!only)
    * command line flags take precedence over config, except:
      * --no-rebase heeds pull.ff=!only
      * pull.rebase=!false vs --no-ff and --ff

For more details behind these rules and a larger table of individual
cases, refer to https://lore.kernel.org/git/xmqqwnpqot4m.fsf@gitster.g/
and the links found therein.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 t/t7601-merge-pull-config.sh | 182 +++++++++++++++++++++++++++++++++++
 1 file changed, 182 insertions(+)

diff --git a/t/t7601-merge-pull-config.sh b/t/t7601-merge-pull-config.sh
index 52e8ccc933a..a44e6e69f2b 100755
--- a/t/t7601-merge-pull-config.sh
+++ b/t/t7601-merge-pull-config.sh
@@ -143,6 +143,188 @@ test_expect_success 'pull.rebase not set and --ff-only given (not-fast-forward)'
 	test_i18ngrep ! "Pulling without specifying how to reconcile" err
 '
 
+test_does_rebase () {
+	git reset --hard c2 &&
+	git "$@" . c1 &&
+	# Check that we actually did a rebase
+	git rev-list --count HEAD >actual &&
+	git rev-list --merges --count HEAD >>actual &&
+	test_write_lines 3 0 >expect &&
+	test_cmp expect actual &&
+	rm actual expect
+}
+
+# Prefers merge over fast-forward
+test_does_merge_when_ff_possible () {
+	git reset --hard c0 &&
+	git "$@" . c1 &&
+	# Check that we actually did a merge
+	git rev-list --count HEAD >actual &&
+	git rev-list --merges --count HEAD >>actual &&
+	test_write_lines 3 1 >expect &&
+	test_cmp expect actual &&
+	rm actual expect
+}
+
+# Prefers fast-forward over merge or rebase
+test_does_fast_forward () {
+	git reset --hard c0 &&
+	git "$@" . c1 &&
+
+	# Check that we did not get any merges
+	git rev-list --count HEAD >actual &&
+	git rev-list --merges --count HEAD >>actual &&
+	test_write_lines 2 0 >expect &&
+	test_cmp expect actual &&
+
+	# Check that we ended up at c1
+	git rev-parse HEAD >actual &&
+	git rev-parse c1^{commit} >expect &&
+	test_cmp actual expect &&
+
+	# Remove temporary files
+	rm actual expect
+}
+
+# Doesn't fail when fast-forward not possible; does a merge
+test_falls_back_to_full_merge () {
+	git reset --hard c2 &&
+	git "$@" . c1 &&
+	# Check that we actually did a merge
+	git rev-list --count HEAD >actual &&
+	git rev-list --merges --count HEAD >>actual &&
+	test_write_lines 4 1 >expect &&
+	test_cmp expect actual &&
+	rm actual expect
+}
+
+# Attempts fast forward, which is impossible, and bails
+test_attempts_fast_forward () {
+	git reset --hard c2 &&
+	test_must_fail git "$@" . c1 2>err &&
+	test_i18ngrep "Not possible to fast-forward, aborting" err
+}
+
+#
+# Group 1: Interaction of --ff-only with --[no-]rebase
+# (And related interaction of pull.ff=only with pull.rebase)
+#
+test_expect_failure '--ff-only overrides --rebase' '
+	test_attempts_fast_forward pull --rebase --ff-only
+'
+
+test_expect_failure '--ff-only overrides --rebase even if first' '
+	test_attempts_fast_forward pull --ff-only --rebase
+'
+
+test_expect_success '--ff-only overrides --no-rebase' '
+	test_attempts_fast_forward pull --ff-only --no-rebase
+'
+
+test_expect_failure 'pull.ff=only overrides pull.rebase=true' '
+	test_attempts_fast_forward -c pull.ff=only -c pull.rebase=true pull
+'
+
+test_expect_success 'pull.ff=only overrides pull.rebase=false' '
+	test_attempts_fast_forward -c pull.ff=only -c pull.rebase=false pull
+'
+
+# Group 2: --rebase=[!false] overrides --no-ff and --ff
+# (And related interaction of pull.rebase=!false and pull.ff=!only)
+test_expect_success '--rebase overrides --no-ff' '
+	test_does_rebase pull --rebase --no-ff
+'
+
+test_expect_success '--rebase overrides --ff' '
+	test_does_rebase pull --rebase --ff
+'
+
+test_expect_success '--rebase fast-forwards when possible' '
+	test_does_fast_forward pull --rebase --ff
+'
+
+test_expect_success 'pull.rebase=true overrides pull.ff=false' '
+	test_does_rebase -c pull.rebase=true -c pull.ff=false pull
+'
+
+test_expect_success 'pull.rebase=true overrides pull.ff=true' '
+	test_does_rebase -c pull.rebase=true -c pull.ff=true pull
+'
+
+# Group 3: command line flags take precedence over config
+test_expect_failure '--ff-only takes precedence over pull.rebase=true' '
+	test_attempts_fast_forward -c pull.rebase=true pull --ff-only
+'
+
+test_expect_success '--ff-only takes precedence over pull.rebase=false' '
+	test_attempts_fast_forward -c pull.rebase=false pull --ff-only
+'
+
+test_expect_failure '--no-rebase takes precedence over pull.ff=only' '
+	test_falls_back_to_full_merge -c pull.ff=only pull --no-rebase
+'
+
+test_expect_success '--rebase takes precedence over pull.ff=only' '
+	test_does_rebase -c pull.ff=only pull --rebase
+'
+
+test_expect_success '--rebase overrides pull.ff=true' '
+	test_does_rebase -c pull.ff=true pull --rebase
+'
+
+test_expect_success '--rebase overrides pull.ff=false' '
+	test_does_rebase -c pull.ff=false pull --rebase
+'
+
+test_expect_success '--rebase overrides pull.ff unset' '
+	test_does_rebase pull --rebase
+'
+
+# Group 4: --no-rebase heeds pull.ff=!only or explict --ff or --no-ff
+
+test_expect_success '--no-rebase works with --no-ff' '
+	test_does_merge_when_ff_possible pull --no-rebase --no-ff
+'
+
+test_expect_success '--no-rebase works with --ff' '
+	test_does_fast_forward pull --no-rebase --ff
+'
+
+test_expect_success '--no-rebase does ff if pull.ff unset' '
+	test_does_fast_forward pull --no-rebase
+'
+
+test_expect_success '--no-rebase heeds pull.ff=true' '
+	test_does_fast_forward -c pull.ff=true pull --no-rebase
+'
+
+test_expect_success '--no-rebase heeds pull.ff=false' '
+	test_does_merge_when_ff_possible -c pull.ff=false pull --no-rebase
+'
+
+# Group 5: pull.rebase=!false in combination with --no-ff or --ff
+test_expect_success 'pull.rebase=true and --no-ff' '
+	test_does_rebase -c pull.rebase=true pull --no-ff
+'
+
+test_expect_success 'pull.rebase=true and --ff' '
+	test_does_rebase -c pull.rebase=true pull --ff
+'
+
+test_expect_success 'pull.rebase=false and --no-ff' '
+	test_does_merge_when_ff_possible -c pull.rebase=false pull --no-ff
+'
+
+test_expect_success 'pull.rebase=false and --ff, ff possible' '
+	test_does_fast_forward -c pull.rebase=false pull --ff
+'
+
+test_expect_success 'pull.rebase=false and --ff, ff not possible' '
+	test_falls_back_to_full_merge -c pull.rebase=false pull --ff
+'
+
+# End of groupings for conflicting merge vs. rebase flags/options
+
 test_expect_success 'merge c1 with c2' '
 	git reset --hard c1 &&
 	test -f c0.c &&
-- 
gitgitgadget

[PATCH v2 2/8] t7601: add tests of interactions with multiple merge heads and config

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

There were already code checking that --rebase was incompatible with
a merge of multiple heads.  However, we were sometimes throwing warnings
about lack of specification of rebase vs. merge when given multiple
heads.  Since rebasing is disallowed with multiple merge heads, that
seems like a poor warning to print; we should instead just assume
merging is wanted.

Add a few tests checking multiple merge head behavior.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 t/t7601-merge-pull-config.sh | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/t/t7601-merge-pull-config.sh b/t/t7601-merge-pull-config.sh
index a44e6e69f2b..21db1e9e14b 100755
--- a/t/t7601-merge-pull-config.sh
+++ b/t/t7601-merge-pull-config.sh
@@ -325,6 +325,28 @@ test_expect_success 'pull.rebase=false and --ff, ff not possible' '
 
 # End of groupings for conflicting merge vs. rebase flags/options
 
+test_expect_failure 'Multiple heads warns about inability to fast forward' '
+	git reset --hard c1 &&
+	test_must_fail git pull . c2 c3 2>err &&
+	test_i18ngrep "Pulling without specifying how to reconcile" err
+'
+
+test_expect_failure 'Multiple can never be fast forwarded' '
+	git reset --hard c0 &&
+	test_must_fail git -c pull.ff=only pull . c1 c2 c3 2>err &&
+	test_i18ngrep ! "Pulling without specifying how to reconcile" err &&
+	# In addition to calling out "cannot fast-forward", we very much
+	# want the "multiple branches" piece to be called out to users.
+	test_i18ngrep "Cannot fast-forward to multiple branches" err
+'
+
+test_expect_success 'Cannot rebase with multiple heads' '
+	git reset --hard c0 &&
+	test_must_fail git -c pull.rebase=true pull . c1 c2 c3 2>err &&
+	test_i18ngrep ! "Pulling without specifying how to reconcile" err &&
+	test_i18ngrep "Cannot rebase onto multiple branches." err
+'
+
 test_expect_success 'merge c1 with c2' '
 	git reset --hard c1 &&
 	test -f c0.c &&
-- 
gitgitgadget

[PATCH v2 3/8] pull: abort if --ff-only is given and fast-forwarding is impossible

[Alex Henrie via GitGitGadget]

From: Alex Henrie <alexhenrie24@gmail.com>

The warning about pulling without specifying how to reconcile divergent
branches says that after setting pull.rebase to true, --ff-only can
still be passed on the command line to require a fast-forward. Make that
actually work.

Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>
[en: updated tests; note 3 fixes and 1 new failure]
Signed-off-by: Elijah Newren <newren@gmail.com>
---
 advice.c                     |  5 +++++
 advice.h                     |  1 +
 builtin/merge.c              |  2 +-
 builtin/pull.c               | 11 ++++++++---
 t/t7601-merge-pull-config.sh | 10 +++++-----
 5 files changed, 20 insertions(+), 9 deletions(-)

diff --git a/advice.c b/advice.c
index 0b9c89c48ab..337e8f342bc 100644
--- a/advice.c
+++ b/advice.c
@@ -286,6 +286,11 @@ void NORETURN die_conclude_merge(void)
 	die(_("Exiting because of unfinished merge."));
 }
 
+void NORETURN die_ff_impossible(void)
+{
+	die(_("Not possible to fast-forward, aborting."));
+}
+
 void advise_on_updating_sparse_paths(struct string_list *pathspec_list)
 {
 	struct string_list_item *item;
diff --git a/advice.h b/advice.h
index bd26c385d00..16240438387 100644
--- a/advice.h
+++ b/advice.h
@@ -95,6 +95,7 @@ void advise_if_enabled(enum advice_type type, const char *advice, ...);
 int error_resolve_conflict(const char *me);
 void NORETURN die_resolve_conflict(const char *me);
 void NORETURN die_conclude_merge(void);
+void NORETURN die_ff_impossible(void);
 void advise_on_updating_sparse_paths(struct string_list *pathspec_list);
 void detach_advice(const char *new_name);
 
diff --git a/builtin/merge.c b/builtin/merge.c
index a8a843b1f54..aa920ac524f 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -1620,7 +1620,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 	}
 
 	if (fast_forward == FF_ONLY)
-		die(_("Not possible to fast-forward, aborting."));
+		die_ff_impossible();
 
 	if (autostash)
 		create_autostash(the_repository,
diff --git a/builtin/pull.c b/builtin/pull.c
index 3e13f810843..d9796604825 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -1046,9 +1046,14 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 
 	can_ff = get_can_ff(&orig_head, &merge_heads.oid[0]);
 
-	if (rebase_unspecified && !opt_ff && !can_ff) {
-		if (opt_verbosity >= 0)
-			show_advice_pull_non_ff();
+	if (!can_ff) {
+		if (opt_ff) {
+			if (!strcmp(opt_ff, "--ff-only"))
+				die_ff_impossible();
+		} else {
+			if (rebase_unspecified && opt_verbosity >= 0)
+				show_advice_pull_non_ff();
+		}
 	}
 
 	if (opt_rebase) {
diff --git a/t/t7601-merge-pull-config.sh b/t/t7601-merge-pull-config.sh
index 21db1e9e14b..d1f621725ad 100755
--- a/t/t7601-merge-pull-config.sh
+++ b/t/t7601-merge-pull-config.sh
@@ -209,11 +209,11 @@ test_attempts_fast_forward () {
 # Group 1: Interaction of --ff-only with --[no-]rebase
 # (And related interaction of pull.ff=only with pull.rebase)
 #
-test_expect_failure '--ff-only overrides --rebase' '
+test_expect_success '--ff-only overrides --rebase' '
 	test_attempts_fast_forward pull --rebase --ff-only
 '
 
-test_expect_failure '--ff-only overrides --rebase even if first' '
+test_expect_success '--ff-only overrides --rebase even if first' '
 	test_attempts_fast_forward pull --ff-only --rebase
 '
 
@@ -221,7 +221,7 @@ test_expect_success '--ff-only overrides --no-rebase' '
 	test_attempts_fast_forward pull --ff-only --no-rebase
 '
 
-test_expect_failure 'pull.ff=only overrides pull.rebase=true' '
+test_expect_success 'pull.ff=only overrides pull.rebase=true' '
 	test_attempts_fast_forward -c pull.ff=only -c pull.rebase=true pull
 '
 
@@ -252,7 +252,7 @@ test_expect_success 'pull.rebase=true overrides pull.ff=true' '
 '
 
 # Group 3: command line flags take precedence over config
-test_expect_failure '--ff-only takes precedence over pull.rebase=true' '
+test_expect_success '--ff-only takes precedence over pull.rebase=true' '
 	test_attempts_fast_forward -c pull.rebase=true pull --ff-only
 '
 
@@ -264,7 +264,7 @@ test_expect_failure '--no-rebase takes precedence over pull.ff=only' '
 	test_falls_back_to_full_merge -c pull.ff=only pull --no-rebase
 '
 
-test_expect_success '--rebase takes precedence over pull.ff=only' '
+test_expect_failure '--rebase takes precedence over pull.ff=only' '
 	test_does_rebase -c pull.ff=only pull --rebase
 '
 
-- 
gitgitgadget

[PATCH v2 4/8] pull: since --ff-only overrides, handle it first

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

There are both merge and rebase branches in the logic, and previously
both had to handle fast-forwarding.  Merge handled that implicitly
(because git merge handles it directly), while in rebase it was
explicit.  Given that the --ff-only flag is meant to override any
--rebase or --no-rebase, make the code reflect that by handling
--ff-only before the merge-vs-rebase logic.

It turns out that this also fixes a bug for submodules.  Previously,
when --ff-only was given, the code would run `merge --ff-only` on the
main module, and then run `submodule update --recursive --rebase` on the
submodules.  With this change, we still run `merge --ff-only` on the
main module, but now run `submodule update --recursive --checkout` on
the submodules.  I believe this better reflects the intent of --ff-only
to have it apply to both the main module and the submodules.

(Sidenote: It is somewhat interesting that all merges pass `--checkout`
to submodule update, even when `--no-ff` is specified, meaning that it
will only do fast-forward merges for submodules.  This was discussed in
commit a6d7eb2c7a ("pull: optionally rebase submodules (remote submodule
changes only)", 2017-06-23).  The same limitations apply now as then, so
we are not trying to fix this at this time.)

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 builtin/pull.c  | 24 +++++++++---------------
 t/t5520-pull.sh |  6 +++---
 2 files changed, 12 insertions(+), 18 deletions(-)

diff --git a/builtin/pull.c b/builtin/pull.c
index d9796604825..92150f976cd 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -1046,15 +1046,15 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 
 	can_ff = get_can_ff(&orig_head, &merge_heads.oid[0]);
 
-	if (!can_ff) {
-		if (opt_ff) {
-			if (!strcmp(opt_ff, "--ff-only"))
-				die_ff_impossible();
-		} else {
-			if (rebase_unspecified && opt_verbosity >= 0)
-				show_advice_pull_non_ff();
-		}
+	/* ff-only takes precedence over rebase */
+	if (opt_ff && !strcmp(opt_ff, "--ff-only")) {
+		if (!can_ff)
+			die_ff_impossible();
+		opt_rebase = REBASE_FALSE;
 	}
+	/* If no action specified and we can't fast forward, then warn. */
+	if (!opt_ff && rebase_unspecified && !can_ff)
+		show_advice_pull_non_ff();
 
 	if (opt_rebase) {
 		int ret = 0;
@@ -1069,13 +1069,7 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 		    submodule_touches_in_range(the_repository, &upstream, &curr_head))
 			die(_("cannot rebase with locally recorded submodule modifications"));
 
-		if (can_ff) {
-			/* we can fast-forward this without invoking rebase */
-			opt_ff = "--ff-only";
-			ret = run_merge();
-		} else {
-			ret = run_rebase(&newbase, &upstream);
-		}
+		ret = run_rebase(&newbase, &upstream);
 
 		if (!ret && (recurse_submodules == RECURSE_SUBMODULES_ON ||
 			     recurse_submodules == RECURSE_SUBMODULES_ON_DEMAND))
diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index e2c0c510222..4b50488141f 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh
@@ -295,7 +295,7 @@ test_expect_success '--rebase (merge) fast forward' '
 	# The above only validates the result.  Did we actually bypass rebase?
 	git reflog -1 >reflog.actual &&
 	sed "s/^[0-9a-f][0-9a-f]*/OBJID/" reflog.actual >reflog.fuzzy &&
-	echo "OBJID HEAD@{0}: pull --rebase . ff: Fast-forward" >reflog.expected &&
+	echo "OBJID HEAD@{0}: pull --rebase . ff (finish): returning to refs/heads/to-rebase" >reflog.expected &&
 	test_cmp reflog.expected reflog.fuzzy
 '
 
@@ -307,8 +307,8 @@ test_expect_success '--rebase (am) fast forward' '
 
 	# The above only validates the result.  Did we actually bypass rebase?
 	git reflog -1 >reflog.actual &&
-	sed "s/^[0-9a-f][0-9a-f]*/OBJID/" reflog.actual >reflog.fuzzy &&
-	echo "OBJID HEAD@{0}: pull --rebase . ff: Fast-forward" >reflog.expected &&
+	sed -e "s/^[0-9a-f][0-9a-f]*/OBJID/" -e "s/[0-9a-f][0-9a-f]*$/OBJID/" reflog.actual >reflog.fuzzy &&
+	echo "OBJID HEAD@{0}: rebase finished: refs/heads/to-rebase onto OBJID" >reflog.expected &&
 	test_cmp reflog.expected reflog.fuzzy
 '
 
-- 
gitgitgadget

[PATCH v2 5/8] pull: make --rebase and --no-rebase override pull.ff=only

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

Fix the last few precedence tests failing in t7601 by now implementing
the logic to have --[no-]rebase override a pull.ff=only config setting.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 builtin/pull.c               | 16 +++++++++++++++-
 t/t7601-merge-pull-config.sh |  4 ++--
 2 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/builtin/pull.c b/builtin/pull.c
index 92150f976cd..da1d4b19666 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -966,8 +966,22 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 
 	parse_repo_refspecs(argc, argv, &repo, &refspecs);
 
-	if (!opt_ff)
+	if (!opt_ff) {
 		opt_ff = xstrdup_or_null(config_get_ff());
+		/*
+		 * A subtle point: opt_ff was set on the line above via
+		 * reading from config.  opt_rebase, in contrast, is set
+		 * before this point via command line options.  The setting
+		 * of opt_rebase via reading from config (using
+		 * config_get_rebase()) does not happen until later.  We
+		 * are relying on the next if-condition happening before
+		 * the config_get_rebase() call so that an explicit
+		 * "--rebase" can override a config setting of
+		 * pull.ff=only.
+		 */
+		if (opt_rebase >= 0 && opt_ff && !strcmp(opt_ff, "--ff-only"))
+			opt_ff = "--ff";
+	}
 
 	if (opt_rebase < 0)
 		opt_rebase = config_get_rebase(&rebase_unspecified);
diff --git a/t/t7601-merge-pull-config.sh b/t/t7601-merge-pull-config.sh
index d1f621725ad..0c0dfecd2a3 100755
--- a/t/t7601-merge-pull-config.sh
+++ b/t/t7601-merge-pull-config.sh
@@ -260,11 +260,11 @@ test_expect_success '--ff-only takes precedence over pull.rebase=false' '
 	test_attempts_fast_forward -c pull.rebase=false pull --ff-only
 '
 
-test_expect_failure '--no-rebase takes precedence over pull.ff=only' '
+test_expect_success '--no-rebase takes precedence over pull.ff=only' '
 	test_falls_back_to_full_merge -c pull.ff=only pull --no-rebase
 '
 
-test_expect_failure '--rebase takes precedence over pull.ff=only' '
+test_expect_success '--rebase takes precedence over pull.ff=only' '
 	test_does_rebase -c pull.ff=only pull --rebase
 '
 
-- 
gitgitgadget

[PATCH v2 6/8] pull: abort by default when fast-forwarding is not possible

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

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.

Initial-patch-by: Alex Henrie <alexhenrie24@gmail.com>
Signed-off-by: Elijah Newren <newren@gmail.com>
---
 Documentation/git-pull.txt    | 20 +++++++++------
 builtin/pull.c                | 10 +++++---
 t/t4013-diff-various.sh       |  2 +-
 t/t5520-pull.sh               | 20 +++++++--------
 t/t5521-pull-options.sh       |  4 +--
 t/t5524-pull-msg.sh           |  4 +--
 t/t5553-set-upstream.sh       | 14 +++++-----
 t/t5604-clone-reference.sh    |  4 +--
 t/t6402-merge-rename.sh       | 18 ++++++-------
 t/t6409-merge-subtree.sh      |  6 ++---
 t/t6417-merge-ours-theirs.sh  | 10 ++++----
 t/t7601-merge-pull-config.sh  | 48 +++++++++++++++++------------------
 t/t7603-merge-reduce-heads.sh |  2 +-
 13 files changed, 84 insertions(+), 78 deletions(-)

diff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt
index 5c3fb67c014..cad3f6bfcee 100644
--- a/Documentation/git-pull.txt
+++ b/Documentation/git-pull.txt
@@ -15,14 +15,18 @@ SYNOPSIS
 DESCRIPTION
 -----------
 
-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'.
+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`, `--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 configuration options or command line flags,
+will call either `git merge` or `git rebase` to reconcile diverging
+branches.
 
 <repository> should be the name of a remote repository as
 passed to linkgit:git-fetch[1].  <refspec> can name an
diff --git a/builtin/pull.c b/builtin/pull.c
index da1d4b19666..94464ca196f 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -927,9 +927,9 @@ static int get_can_ff(struct object_id *orig_head, struct object_id *orig_merge_
 
 static void show_advice_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"
+	advise(_("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"
 		 "\n"
 		 "  git config pull.rebase false  # merge (the default strategy)\n"
 		 "  git config pull.rebase true   # rebase\n"
@@ -1067,8 +1067,10 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 		opt_rebase = REBASE_FALSE;
 	}
 	/* If no action specified and we can't fast forward, then warn. */
-	if (!opt_ff && rebase_unspecified && !can_ff)
+	if (!opt_ff && rebase_unspecified && !can_ff) {
 		show_advice_pull_non_ff();
+		die(_("Need to specify how to reconcile divergent branches."));
+	}
 
 	if (opt_rebase) {
 		int ret = 0;
diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
index 7fadc985ccc..eb989f7f191 100755
--- a/t/t4013-diff-various.sh
+++ b/t/t4013-diff-various.sh
@@ -65,7 +65,7 @@ test_expect_success setup '
 	export GIT_AUTHOR_DATE GIT_COMMITTER_DATE &&
 
 	git checkout master &&
-	git pull -s ours . side &&
+	git pull -s ours --no-rebase . side &&
 
 	GIT_AUTHOR_DATE="2006-06-26 00:05:00 +0000" &&
 	GIT_COMMITTER_DATE="2006-06-26 00:05:00 +0000" &&
diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index 4b50488141f..56f5864b269 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh
@@ -136,12 +136,12 @@ test_expect_success 'the default remote . should not break explicit pull' '
 	git reset --hard HEAD^ &&
 	echo file >expect &&
 	test_cmp expect file &&
-	git pull . second &&
+	git pull --no-rebase . second &&
 	echo modified >expect &&
 	test_cmp expect file &&
 	git reflog -1 >reflog.actual &&
 	sed "s/^[0-9a-f][0-9a-f]*/OBJID/" reflog.actual >reflog.fuzzy &&
-	echo "OBJID HEAD@{0}: pull . second: Fast-forward" >reflog.expected &&
+	echo "OBJID HEAD@{0}: pull --no-rebase . second: Fast-forward" >reflog.expected &&
 	test_cmp reflog.expected reflog.fuzzy
 '
 
@@ -226,7 +226,7 @@ test_expect_success 'fail if the index has unresolved entries' '
 	test_commit modified2 file &&
 	git ls-files -u >unmerged &&
 	test_must_be_empty unmerged &&
-	test_must_fail git pull . second &&
+	test_must_fail git pull --no-rebase . second &&
 	git ls-files -u >unmerged &&
 	test_file_not_empty unmerged &&
 	cp file expected &&
@@ -409,37 +409,37 @@ test_expect_success 'pull --rebase --no-autostash & rebase.autostash unset' '
 
 test_expect_success 'pull succeeds with dirty working directory and merge.autostash set' '
 	test_config merge.autostash true &&
-	test_pull_autostash 2
+	test_pull_autostash 2 --no-rebase
 '
 
 test_expect_success 'pull --autostash & merge.autostash=true' '
 	test_config merge.autostash true &&
-	test_pull_autostash 2 --autostash
+	test_pull_autostash 2 --autostash --no-rebase
 '
 
 test_expect_success 'pull --autostash & merge.autostash=false' '
 	test_config merge.autostash false &&
-	test_pull_autostash 2 --autostash
+	test_pull_autostash 2 --autostash --no-rebase
 '
 
 test_expect_success 'pull --autostash & merge.autostash unset' '
 	test_unconfig merge.autostash &&
-	test_pull_autostash 2 --autostash
+	test_pull_autostash 2 --autostash --no-rebase
 '
 
 test_expect_success 'pull --no-autostash & merge.autostash=true' '
 	test_config merge.autostash true &&
-	test_pull_autostash_fail --no-autostash
+	test_pull_autostash_fail --no-autostash --no-rebase
 '
 
 test_expect_success 'pull --no-autostash & merge.autostash=false' '
 	test_config merge.autostash false &&
-	test_pull_autostash_fail --no-autostash
+	test_pull_autostash_fail --no-autostash --no-rebase
 '
 
 test_expect_success 'pull --no-autostash & merge.autostash unset' '
 	test_unconfig merge.autostash &&
-	test_pull_autostash_fail --no-autostash
+	test_pull_autostash_fail --no-autostash --no-rebase
 '
 
 test_expect_success 'pull.rebase' '
diff --git a/t/t5521-pull-options.sh b/t/t5521-pull-options.sh
index 63a688bdbf5..7601c919fdc 100755
--- a/t/t5521-pull-options.sh
+++ b/t/t5521-pull-options.sh
@@ -113,7 +113,7 @@ test_expect_success 'git pull --force' '
 	git pull two &&
 	test_commit A &&
 	git branch -f origin &&
-	git pull --all --force
+	git pull --no-rebase --all --force
 	)
 '
 
@@ -179,7 +179,7 @@ test_expect_success 'git pull --allow-unrelated-histories' '
 	(
 		cd dst &&
 		test_must_fail git pull ../src side &&
-		git pull --allow-unrelated-histories ../src side
+		git pull --no-rebase --allow-unrelated-histories ../src side
 	)
 '
 
diff --git a/t/t5524-pull-msg.sh b/t/t5524-pull-msg.sh
index c278adaa5a2..b2be3605f5a 100755
--- a/t/t5524-pull-msg.sh
+++ b/t/t5524-pull-msg.sh
@@ -28,7 +28,7 @@ test_expect_success setup '
 test_expect_success pull '
 (
 	cd cloned &&
-	git pull --log &&
+	git pull --no-rebase --log &&
 	git log -2 &&
 	git cat-file commit HEAD >result &&
 	grep Dollar result
@@ -41,7 +41,7 @@ test_expect_success '--log=1 limits shortlog length' '
 	git reset --hard HEAD^ &&
 	test "$(cat afile)" = original &&
 	test "$(cat bfile)" = added &&
-	git pull --log=1 &&
+	git pull --no-rebase --log=1 &&
 	git log -3 &&
 	git cat-file commit HEAD >result &&
 	grep Dollar result &&
diff --git a/t/t5553-set-upstream.sh b/t/t5553-set-upstream.sh
index b1d614ce18c..9c12c0f8c32 100755
--- a/t/t5553-set-upstream.sh
+++ b/t/t5553-set-upstream.sh
@@ -108,27 +108,27 @@ test_expect_success 'setup commit on main and other pull' '
 
 test_expect_success 'pull --set-upstream upstream main sets branch main but not other' '
 	clear_config main other &&
-	git pull --set-upstream upstream main &&
+	git pull --no-rebase --set-upstream upstream main &&
 	check_config main upstream refs/heads/main &&
 	check_config_missing other
 '
 
 test_expect_success 'pull --set-upstream main:other2 does not set the branch other2' '
 	clear_config other2 &&
-	git pull --set-upstream upstream main:other2 &&
+	git pull --no-rebase --set-upstream upstream main:other2 &&
 	check_config_missing other2
 '
 
 test_expect_success 'pull --set-upstream upstream other sets branch main' '
 	clear_config main other &&
-	git pull --set-upstream upstream other &&
+	git pull --no-rebase --set-upstream upstream other &&
 	check_config main upstream refs/heads/other &&
 	check_config_missing other
 '
 
 test_expect_success 'pull --set-upstream upstream tag does not set the tag' '
 	clear_config three &&
-	git pull --tags --set-upstream upstream three &&
+	git pull --no-rebase --tags --set-upstream upstream three &&
 	check_config_missing three
 '
 
@@ -144,16 +144,16 @@ test_expect_success 'pull --set-upstream http://nosuchdomain.example.com fails w
 
 test_expect_success 'pull --set-upstream upstream HEAD sets branch HEAD' '
 	clear_config main other &&
-	git pull --set-upstream upstream HEAD &&
+	git pull --no-rebase --set-upstream upstream HEAD &&
 	check_config main upstream HEAD &&
 	git checkout other &&
-	git pull --set-upstream upstream HEAD &&
+	git pull --no-rebase --set-upstream upstream HEAD &&
 	check_config other upstream HEAD
 '
 
 test_expect_success 'pull --set-upstream upstream with more than one branch does nothing' '
 	clear_config main three &&
-	git pull --set-upstream upstream main three &&
+	git pull --no-rebase --set-upstream upstream main three &&
 	check_config_missing main &&
 	check_config_missing three
 '
diff --git a/t/t5604-clone-reference.sh b/t/t5604-clone-reference.sh
index e845d621f61..24340e6d56e 100755
--- a/t/t5604-clone-reference.sh
+++ b/t/t5604-clone-reference.sh
@@ -87,7 +87,7 @@ test_expect_success 'updating origin' '
 '
 
 test_expect_success 'pulling changes from origin' '
-	git -C C pull origin
+	git -C C pull --no-rebase origin
 '
 
 # the 2 local objects are commit and tree from the merge
@@ -96,7 +96,7 @@ test_expect_success 'that alternate to origin gets used' '
 '
 
 test_expect_success 'pulling changes from origin' '
-	git -C D pull origin
+	git -C D pull --no-rebase origin
 '
 
 # the 5 local objects are expected; file3 blob, commit in A to add it
diff --git a/t/t6402-merge-rename.sh b/t/t6402-merge-rename.sh
index 3da2896e3bc..3a32b1a45cf 100755
--- a/t/t6402-merge-rename.sh
+++ b/t/t6402-merge-rename.sh
@@ -103,7 +103,7 @@ test_expect_success 'setup' '
 test_expect_success 'pull renaming branch into unrenaming one' \
 '
 	git show-branch &&
-	test_expect_code 1 git pull . white &&
+	test_expect_code 1 git pull --no-rebase . white &&
 	git ls-files -s &&
 	test_stdout_line_count = 3 git ls-files -u B &&
 	test_stdout_line_count = 1 git ls-files -s N &&
@@ -119,7 +119,7 @@ test_expect_success 'pull renaming branch into another renaming one' \
 	rm -f B &&
 	git reset --hard &&
 	git checkout red &&
-	test_expect_code 1 git pull . white &&
+	test_expect_code 1 git pull --no-rebase . white &&
 	test_stdout_line_count = 3 git ls-files -u B &&
 	test_stdout_line_count = 1 git ls-files -s N &&
 	sed -ne "/^g/{
@@ -133,7 +133,7 @@ test_expect_success 'pull unrenaming branch into renaming one' \
 '
 	git reset --hard &&
 	git show-branch &&
-	test_expect_code 1 git pull . main &&
+	test_expect_code 1 git pull --no-rebase . main &&
 	test_stdout_line_count = 3 git ls-files -u B &&
 	test_stdout_line_count = 1 git ls-files -s N &&
 	sed -ne "/^g/{
@@ -147,7 +147,7 @@ test_expect_success 'pull conflicting renames' \
 '
 	git reset --hard &&
 	git show-branch &&
-	test_expect_code 1 git pull . blue &&
+	test_expect_code 1 git pull --no-rebase . blue &&
 	test_stdout_line_count = 1 git ls-files -u A &&
 	test_stdout_line_count = 1 git ls-files -u B &&
 	test_stdout_line_count = 1 git ls-files -u C &&
@@ -163,7 +163,7 @@ test_expect_success 'interference with untracked working tree file' '
 	git reset --hard &&
 	git show-branch &&
 	echo >A this file should not matter &&
-	test_expect_code 1 git pull . white &&
+	test_expect_code 1 git pull --no-rebase . white &&
 	test_path_is_file A
 '
 
@@ -173,7 +173,7 @@ test_expect_success 'interference with untracked working tree file' '
 	git show-branch &&
 	rm -f A &&
 	echo >A this file should not matter &&
-	test_expect_code 1 git pull . red &&
+	test_expect_code 1 git pull --no-rebase . red &&
 	test_path_is_file A
 '
 
@@ -183,7 +183,7 @@ test_expect_success 'interference with untracked working tree file' '
 	git checkout -f main &&
 	git tag -f anchor &&
 	git show-branch &&
-	git pull . yellow &&
+	git pull --no-rebase . yellow &&
 	test_path_is_missing M &&
 	git reset --hard anchor
 '
@@ -210,7 +210,7 @@ test_expect_success 'updated working tree file should prevent the merge' '
 	echo >>M one line addition &&
 	cat M >M.saved &&
 	git update-index M &&
-	test_expect_code 128 git pull . yellow &&
+	test_expect_code 128 git pull --no-rebase . yellow &&
 	test_cmp M M.saved &&
 	rm -f M.saved
 '
@@ -222,7 +222,7 @@ test_expect_success 'interference with untracked working tree file' '
 	git tag -f anchor &&
 	git show-branch &&
 	echo >M this file should not matter &&
-	git pull . main &&
+	git pull --no-rebase . main &&
 	test_path_is_file M &&
 	! {
 		git ls-files -s |
diff --git a/t/t6409-merge-subtree.sh b/t/t6409-merge-subtree.sh
index d406b2343cb..ba7890ec521 100755
--- a/t/t6409-merge-subtree.sh
+++ b/t/t6409-merge-subtree.sh
@@ -100,7 +100,7 @@ test_expect_success 'merge update' '
 	git checkout -b topic_2 &&
 	git commit -m "update git-gui" &&
 	cd ../git &&
-	git pull -s subtree gui topic_2 &&
+	git pull --no-rebase -s subtree gui topic_2 &&
 	git ls-files -s >actual &&
 	(
 		echo "100644 $o3 0	git-gui/git-gui.sh" &&
@@ -129,7 +129,7 @@ test_expect_success 'initial ambiguous subtree' '
 test_expect_success 'merge using explicit' '
 	cd ../git &&
 	git reset --hard topic_2 &&
-	git pull -Xsubtree=git-gui gui topic_2 &&
+	git pull --no-rebase -Xsubtree=git-gui gui topic_2 &&
 	git ls-files -s >actual &&
 	(
 		echo "100644 $o3 0	git-gui/git-gui.sh" &&
@@ -142,7 +142,7 @@ test_expect_success 'merge using explicit' '
 test_expect_success 'merge2 using explicit' '
 	cd ../git &&
 	git reset --hard topic_2 &&
-	git pull -Xsubtree=git-gui2 gui topic_2 &&
+	git pull --no-rebase -Xsubtree=git-gui2 gui topic_2 &&
 	git ls-files -s >actual &&
 	(
 		echo "100644 $o1 0	git-gui/git-gui.sh" &&
diff --git a/t/t6417-merge-ours-theirs.sh b/t/t6417-merge-ours-theirs.sh
index ac9aee9a662..ec065d6a658 100755
--- a/t/t6417-merge-ours-theirs.sh
+++ b/t/t6417-merge-ours-theirs.sh
@@ -69,11 +69,11 @@ test_expect_success 'binary file with -Xours/-Xtheirs' '
 '
 
 test_expect_success 'pull passes -X to underlying merge' '
-	git reset --hard main && git pull -s recursive -Xours . side &&
-	git reset --hard main && git pull -s recursive -X ours . side &&
-	git reset --hard main && git pull -s recursive -Xtheirs . side &&
-	git reset --hard main && git pull -s recursive -X theirs . side &&
-	git reset --hard main && test_must_fail git pull -s recursive -X bork . side
+	git reset --hard main && git pull --no-rebase -s recursive -Xours . side &&
+	git reset --hard main && git pull --no-rebase -s recursive -X ours . side &&
+	git reset --hard main && git pull --no-rebase -s recursive -Xtheirs . side &&
+	git reset --hard main && git pull --no-rebase -s recursive -X theirs . side &&
+	git reset --hard main && test_must_fail git pull --no-rebase -s recursive -X bork . side
 '
 
 test_expect_success SYMLINKS 'symlink with -Xours/-Xtheirs' '
diff --git a/t/t7601-merge-pull-config.sh b/t/t7601-merge-pull-config.sh
index 0c0dfecd2a3..742ed3981c7 100755
--- a/t/t7601-merge-pull-config.sh
+++ b/t/t7601-merge-pull-config.sh
@@ -27,120 +27,120 @@ test_expect_success 'setup' '
 	git tag c3
 '
 
-test_expect_success 'pull.rebase not set' '
+test_expect_success 'pull.rebase not set, ff possible' '
 	git reset --hard c0 &&
 	git pull . c1 2>err &&
-	test_i18ngrep ! "Pulling without specifying how to reconcile" err
+	test_i18ngrep ! "You have divergent branches" err
 '
 
 test_expect_success 'pull.rebase not set and pull.ff=true' '
 	git reset --hard c0 &&
 	test_config pull.ff true &&
 	git pull . c1 2>err &&
-	test_i18ngrep ! "Pulling without specifying how to reconcile" err
+	test_i18ngrep ! "You have divergent branches" err
 '
 
 test_expect_success 'pull.rebase not set and pull.ff=false' '
 	git reset --hard c0 &&
 	test_config pull.ff false &&
 	git pull . c1 2>err &&
-	test_i18ngrep ! "Pulling without specifying how to reconcile" err
+	test_i18ngrep ! "You have divergent branches" err
 '
 
 test_expect_success 'pull.rebase not set and pull.ff=only' '
 	git reset --hard c0 &&
 	test_config pull.ff only &&
 	git pull . c1 2>err &&
-	test_i18ngrep ! "Pulling without specifying how to reconcile" err
+	test_i18ngrep ! "You have divergent branches" err
 '
 
 test_expect_success 'pull.rebase not set and --rebase given' '
 	git reset --hard c0 &&
 	git pull --rebase . c1 2>err &&
-	test_i18ngrep ! "Pulling without specifying how to reconcile" err
+	test_i18ngrep ! "You have divergent branches" err
 '
 
 test_expect_success 'pull.rebase not set and --no-rebase given' '
 	git reset --hard c0 &&
 	git pull --no-rebase . c1 2>err &&
-	test_i18ngrep ! "Pulling without specifying how to reconcile" err
+	test_i18ngrep ! "You have divergent branches" err
 '
 
 test_expect_success 'pull.rebase not set and --ff given' '
 	git reset --hard c0 &&
 	git pull --ff . c1 2>err &&
-	test_i18ngrep ! "Pulling without specifying how to reconcile" err
+	test_i18ngrep ! "You have divergent branches" err
 '
 
 test_expect_success 'pull.rebase not set and --no-ff given' '
 	git reset --hard c0 &&
 	git pull --no-ff . c1 2>err &&
-	test_i18ngrep ! "Pulling without specifying how to reconcile" err
+	test_i18ngrep ! "You have divergent branches" err
 '
 
 test_expect_success 'pull.rebase not set and --ff-only given' '
 	git reset --hard c0 &&
 	git pull --ff-only . c1 2>err &&
-	test_i18ngrep ! "Pulling without specifying how to reconcile" err
+	test_i18ngrep ! "You have divergent branches" err
 '
 
 test_expect_success 'pull.rebase not set (not-fast-forward)' '
 	git reset --hard c2 &&
-	git -c color.advice=always pull . c1 2>err &&
+	test_must_fail git -c color.advice=always pull . c1 2>err &&
 	test_decode_color <err >decoded &&
 	test_i18ngrep "<YELLOW>hint: " decoded &&
-	test_i18ngrep "Pulling without specifying how to reconcile" decoded
+	test_i18ngrep "You have divergent branches" decoded
 '
 
 test_expect_success 'pull.rebase not set and pull.ff=true (not-fast-forward)' '
 	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 ! "You have divergent branches" err
 '
 
 test_expect_success 'pull.rebase not set and pull.ff=false (not-fast-forward)' '
 	git reset --hard c2 &&
 	test_config pull.ff false &&
 	git pull . c1 2>err &&
-	test_i18ngrep ! "Pulling without specifying how to reconcile" err
+	test_i18ngrep ! "You have divergent branches" err
 '
 
 test_expect_success 'pull.rebase not set and pull.ff=only (not-fast-forward)' '
 	git reset --hard c2 &&
 	test_config pull.ff only &&
 	test_must_fail git pull . c1 2>err &&
-	test_i18ngrep ! "Pulling without specifying how to reconcile" err
+	test_i18ngrep ! "You have divergent branches" err
 '
 
 test_expect_success 'pull.rebase not set and --rebase given (not-fast-forward)' '
 	git reset --hard c2 &&
 	git pull --rebase . c1 2>err &&
-	test_i18ngrep ! "Pulling without specifying how to reconcile" err
+	test_i18ngrep ! "You have divergent branches" err
 '
 
 test_expect_success 'pull.rebase not set and --no-rebase given (not-fast-forward)' '
 	git reset --hard c2 &&
 	git pull --no-rebase . c1 2>err &&
-	test_i18ngrep ! "Pulling without specifying how to reconcile" err
+	test_i18ngrep ! "You have divergent branches" err
 '
 
 test_expect_success 'pull.rebase not set and --ff given (not-fast-forward)' '
 	git reset --hard c2 &&
 	git pull --ff . c1 2>err &&
-	test_i18ngrep ! "Pulling without specifying how to reconcile" err
+	test_i18ngrep ! "You have divergent branches" err
 '
 
 test_expect_success 'pull.rebase not set and --no-ff given (not-fast-forward)' '
 	git reset --hard c2 &&
 	git pull --no-ff . c1 2>err &&
-	test_i18ngrep ! "Pulling without specifying how to reconcile" err
+	test_i18ngrep ! "You have divergent branches" err
 '
 
 test_expect_success 'pull.rebase not set and --ff-only given (not-fast-forward)' '
 	git reset --hard c2 &&
 	test_must_fail git pull --ff-only . c1 2>err &&
-	test_i18ngrep ! "Pulling without specifying how to reconcile" err
+	test_i18ngrep ! "You have divergent branches" err
 '
 
 test_does_rebase () {
@@ -325,16 +325,16 @@ test_expect_success 'pull.rebase=false and --ff, ff not possible' '
 
 # End of groupings for conflicting merge vs. rebase flags/options
 
-test_expect_failure 'Multiple heads warns about inability to fast forward' '
+test_expect_success 'Multiple heads warns about inability to fast forward' '
 	git reset --hard c1 &&
 	test_must_fail git pull . c2 c3 2>err &&
-	test_i18ngrep "Pulling without specifying how to reconcile" err
+	test_i18ngrep "You have divergent branches" err
 '
 
 test_expect_failure 'Multiple can never be fast forwarded' '
 	git reset --hard c0 &&
 	test_must_fail git -c pull.ff=only pull . c1 c2 c3 2>err &&
-	test_i18ngrep ! "Pulling without specifying how to reconcile" err &&
+	test_i18ngrep ! "You have divergent branches" err &&
 	# In addition to calling out "cannot fast-forward", we very much
 	# want the "multiple branches" piece to be called out to users.
 	test_i18ngrep "Cannot fast-forward to multiple branches" err
@@ -343,7 +343,7 @@ test_expect_failure 'Multiple can never be fast forwarded' '
 test_expect_success 'Cannot rebase with multiple heads' '
 	git reset --hard c0 &&
 	test_must_fail git -c pull.rebase=true pull . c1 c2 c3 2>err &&
-	test_i18ngrep ! "Pulling without specifying how to reconcile" err &&
+	test_i18ngrep ! "You have divergent branches" err &&
 	test_i18ngrep "Cannot rebase onto multiple branches." err
 '
 
diff --git a/t/t7603-merge-reduce-heads.sh b/t/t7603-merge-reduce-heads.sh
index 98948955ae5..27cd94ad6f7 100755
--- a/t/t7603-merge-reduce-heads.sh
+++ b/t/t7603-merge-reduce-heads.sh
@@ -68,7 +68,7 @@ test_expect_success 'merge c1 with c2, c3, c4, c5' '
 
 test_expect_success 'pull c2, c3, c4, c5 into c1' '
 	git reset --hard c1 &&
-	git pull . c2 c3 c4 c5 &&
+	git pull --no-rebase . c2 c3 c4 c5 &&
 	test "$(git rev-parse c1)" != "$(git rev-parse HEAD)" &&
 	test "$(git rev-parse c1)" = "$(git rev-parse HEAD^1)" &&
 	test "$(git rev-parse c2)" = "$(git rev-parse HEAD^2)" &&
-- 
gitgitgadget

[PATCH v2 7/8] pull: update docs & code for option compatibility with rebasing

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

git-pull.txt includes merge-options.txt, which is written assuming
merges will happen.  git-pull has allowed rebases for many years; update
the documentation to reflect that.

While at it, pass any `--signoff` flag through to the rebase backend too
so that we don't have to document it as merge-specific.  Rebase has
supported the --signoff flag for years now as well.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 Documentation/git-merge.txt     |  2 ++
 Documentation/git-pull.txt      |  9 ++++----
 Documentation/merge-options.txt | 40 +++++++++++++++++++++++++++++++++
 builtin/pull.c                  |  2 ++
 4 files changed, 48 insertions(+), 5 deletions(-)

diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt
index 3819fadac1f..e4f3352eb58 100644
--- a/Documentation/git-merge.txt
+++ b/Documentation/git-merge.txt
@@ -61,6 +61,8 @@ merge has resulted in conflicts.
 
 OPTIONS
 -------
+:git-merge: 1
+
 include::merge-options.txt[]
 
 -m <msg>::
diff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt
index cad3f6bfcee..077961dfa1f 100644
--- a/Documentation/git-pull.txt
+++ b/Documentation/git-pull.txt
@@ -19,13 +19,12 @@ 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`, `--ff`, or `--rebase`
-(or the corresponding configuration options in `pull.ff` or
-`pull.rebase`).
+reconcile the divergent branches with `--rebase` or `--no-rebase` (or
+the corresponding configuration option in `pull.rebase`).
 
 More precisely, `git pull` runs `git fetch` with the given parameters
 and then depending on configuration options or command line flags,
-will call either `git merge` or `git rebase` to reconcile diverging
+will call either `git rebase` or `git merge` to reconcile diverging
 branches.
 
 <repository> should be the name of a remote repository as
@@ -136,7 +135,7 @@ published that history already.  Do *not* use this option
 unless you have read linkgit:git-rebase[1] carefully.
 
 --no-rebase::
-	Override earlier --rebase.
+	This is shorthand for --rebase=false.
 
 Options related to fetching
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~
diff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt
index eb0aabd396f..907327bf759 100644
--- a/Documentation/merge-options.txt
+++ b/Documentation/merge-options.txt
@@ -2,6 +2,9 @@
 --no-commit::
 	Perform the merge and commit the result. This option can
 	be used to override --no-commit.
+ifdef::git-pull[]
+	Only useful when merging.
+endif::git-pull[]
 +
 With --no-commit perform the merge and stop just before creating
 a merge commit, to give the user a chance to inspect and further
@@ -39,6 +42,7 @@ set to `no` at the beginning of them.
 	to `MERGE_MSG` before being passed on to the commit machinery in the
 	case of a merge conflict.
 
+ifdef::git-merge[]
 --ff::
 --no-ff::
 --ff-only::
@@ -47,6 +51,22 @@ set to `no` at the beginning of them.
 	default unless merging an annotated (and possibly signed) tag
 	that is not stored in its natural place in the `refs/tags/`
 	hierarchy, in which case `--no-ff` is assumed.
+endif::git-merge[]
+ifdef::git-pull[]
+--ff-only::
+	Only update to the new history if there is no divergent local
+	history.  This is the default when no method for reconciling
+	divergent histories is provided (via the --rebase=* flags).
+
+--ff::
+--no-ff::
+	When merging rather than rebasing, specifies how a merge is
+	handled when the merged-in history is already a descendant of
+	the current history.  If merging is requested, `--ff` is the
+	default unless merging an annotated (and possibly signed) tag
+	that is not stored in its natural place in the `refs/tags/`
+	hierarchy, in which case `--no-ff` is assumed.
+endif::git-pull[]
 +
 With `--ff`, when possible resolve the merge as a fast-forward (only
 update the branch pointer to match the merged branch; do not create a
@@ -55,9 +75,11 @@ descendant of the current history), create a merge commit.
 +
 With `--no-ff`, create a merge commit in all cases, even when the merge
 could instead be resolved as a fast-forward.
+ifdef::git-merge[]
 +
 With `--ff-only`, resolve the merge as a fast-forward when possible.
 When not possible, refuse to merge and exit with a non-zero status.
+endif::git-merge[]
 
 -S[<keyid>]::
 --gpg-sign[=<keyid>]::
@@ -73,6 +95,9 @@ When not possible, refuse to merge and exit with a non-zero status.
 	In addition to branch names, populate the log message with
 	one-line descriptions from at most <n> actual commits that are being
 	merged. See also linkgit:git-fmt-merge-msg[1].
+ifdef::git-pull[]
+	Only useful when merging.
+endif::git-pull[]
 +
 With --no-log do not list one-line descriptions from the
 actual commits being merged.
@@ -102,10 +127,17 @@ With --no-squash perform the merge and commit the result. This
 option can be used to override --squash.
 +
 With --squash, --commit is not allowed, and will fail.
+ifdef::git-pull[]
++
+Only useful when merging.
+endif::git-pull[]
 
 --no-verify::
 	This option bypasses the pre-merge and commit-msg hooks.
 	See also linkgit:githooks[5].
+ifdef::git-pull[]
+	Only useful when merging.
+endif::git-pull[]
 
 -s <strategy>::
 --strategy=<strategy>::
@@ -127,6 +159,10 @@ With --squash, --commit is not allowed, and will fail.
 	default trust model, this means the signing key has been signed by
 	a trusted key.  If the tip commit of the side branch is not signed
 	with a valid key, the merge is aborted.
+ifdef::git-pull[]
++
+Only useful when merging.
+endif::git-pull[]
 
 --summary::
 --no-summary::
@@ -166,3 +202,7 @@ endif::git-pull[]
 	projects that started their lives independently. As that is
 	a very rare occasion, no configuration variable to enable
 	this by default exists and will not be added.
+ifdef::git-pull[]
++
+Only useful when merging.
+endif::git-pull[]
diff --git a/builtin/pull.c b/builtin/pull.c
index 94464ca196f..a15d4154093 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -893,6 +893,8 @@ static int run_rebase(const struct object_id *newbase,
 	strvec_pushv(&args, opt_strategy_opts.v);
 	if (opt_gpg_sign)
 		strvec_push(&args, opt_gpg_sign);
+	if (opt_signoff)
+		strvec_push(&args, opt_signoff);
 	if (opt_autostash == 0)
 		strvec_push(&args, "--no-autostash");
 	else if (opt_autostash == 1)
-- 
gitgitgadget

[PATCH v2 8/8] pull: fix handling of multiple heads

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

With multiple heads, we should not allow rebasing or fast-forwarding.
Make sure any fast-forward request calls out specifically the fact that
multiple branches are in play.  Also, since we cannot fast-forward to
multiple branches, fix our computation of can_ff.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 builtin/pull.c               | 18 ++++++++++++++----
 t/t7601-merge-pull-config.sh |  2 +-
 2 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/builtin/pull.c b/builtin/pull.c
index a15d4154093..d6631d23d17 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -913,12 +913,18 @@ static int run_rebase(const struct object_id *newbase,
 	return ret;
 }
 
-static int get_can_ff(struct object_id *orig_head, struct object_id *orig_merge_head)
+static int get_can_ff(struct object_id *orig_head,
+		      struct oid_array *merge_heads)
 {
 	int ret;
 	struct commit_list *list = NULL;
 	struct commit *merge_head, *head;
+	struct object_id *orig_merge_head;
 
+	if (merge_heads->nr > 1)
+		return 0;
+
+	orig_merge_head = &merge_heads->oid[0];
 	head = lookup_commit_reference(the_repository, orig_head);
 	commit_list_insert(head, &list);
 	merge_head = lookup_commit_reference(the_repository, orig_merge_head);
@@ -1057,10 +1063,14 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 			die(_("Cannot merge multiple branches into empty head."));
 		return pull_into_void(merge_heads.oid, &curr_head);
 	}
-	if (opt_rebase && merge_heads.nr > 1)
-		die(_("Cannot rebase onto multiple branches."));
+	if (merge_heads.nr > 1) {
+		if (opt_rebase)
+			die(_("Cannot rebase onto multiple branches."));
+		if (opt_ff && !strcmp(opt_ff, "--ff-only"))
+			die(_("Cannot fast-forward to multiple branches."));
+	}
 
-	can_ff = get_can_ff(&orig_head, &merge_heads.oid[0]);
+	can_ff = get_can_ff(&orig_head, &merge_heads);
 
 	/* ff-only takes precedence over rebase */
 	if (opt_ff && !strcmp(opt_ff, "--ff-only")) {
diff --git a/t/t7601-merge-pull-config.sh b/t/t7601-merge-pull-config.sh
index 742ed3981c7..1f652f433ee 100755
--- a/t/t7601-merge-pull-config.sh
+++ b/t/t7601-merge-pull-config.sh
@@ -331,7 +331,7 @@ test_expect_success 'Multiple heads warns about inability to fast forward' '
 	test_i18ngrep "You have divergent branches" err
 '
 
-test_expect_failure 'Multiple can never be fast forwarded' '
+test_expect_success 'Multiple can never be fast forwarded' '
 	git reset --hard c0 &&
 	test_must_fail git -c pull.ff=only pull . c1 c2 c3 2>err &&
 	test_i18ngrep ! "You have divergent branches" err &&
-- 
gitgitgadget

RE: [PATCH v2 0/8] Handle pull option precedence

[Felipe Contreras]

It's a bit tedious to keep repeating all the ways this approach is
flawed, but here we go again.

Elijah Newren via GitGitGadget wrote:
> Based on a recent list of rules for flag/option precedence for git-pull[1]
> from Junio (particularly focusing on rebase vs. merge vs. fast-forward),
> here's an attempt to implement and document it. Given multiple recent
> surprises from users about some of these behaviors[2][3]

It might have been a surprise for you, but Son Luong Ngoc's usage of the
options is correct.

>  * Patches 1-2: new testcases (see the commit messages for the rules)

These test cases check for backwards incompatible changes, not what the
code is doing (and is supposed to be doing) right now.

>  * Patch 3: Alex's recent patch (abort if --ff-only but can't do so)

This breaks backwards compatibility.

Users will complain.

>  * Patches 4-6: fix the precedence parts Alex didn't cover

Yet another backwards incompatible change.

>  * Patch 7: Alex's other patch, abort if rebase vs. merge not specified

More brekage without a single day of deprecation warning.

>  * Patch 8: Compatibility of git-pull with merge-options.txt (think
>    rebasing)

This doesn't update anything in Documentation/config, which is now
clearly broken.

-- 
Felipe Contreras

RE: [PATCH v2 1/8] t7601: test interaction of merge/rebase/fast-forward flags and options

[Felipe Contreras]

Elijah Newren via GitGitGadget wrote:
> From: Elijah Newren <newren@gmail.com>
> 
> The interaction of rebase and merge flags and options was not well
> tested.  Add several tests to check for correct behavior from the
> following rules:
>     * --ff-only vs. --[no-]rebase
>       (and the related pull.ff=only vs. pull.rebase)
>     * --rebase[=!false] vs. --no-ff and --ff
>       (and the related pull.rebase=!false overrides pull.ff=!only)

This breaks current behavior.

>     * command line flags take precedence over config, except:
>       * --no-rebase heeds pull.ff=!only
>       * pull.rebase=!false vs --no-ff and --ff

This isn't explaind anywhere.

> --- a/t/t7601-merge-pull-config.sh
> +++ b/t/t7601-merge-pull-config.sh
> @@ -143,6 +143,188 @@

> +#
> +# Group 1: Interaction of --ff-only with --[no-]rebase
> +# (And related interaction of pull.ff=only with pull.rebase)
> +#
> +test_expect_failure '--ff-only overrides --rebase' '
> +	test_attempts_fast_forward pull --rebase --ff-only
> +'
> +
> +test_expect_failure '--ff-only overrides --rebase even if first' '
> +	test_attempts_fast_forward pull --ff-only --rebase
> +'
> +
> +test_expect_success '--ff-only overrides --no-rebase' '
> +	test_attempts_fast_forward pull --ff-only --no-rebase
> +'

These make absolutely no sense. Why does --rebase override pull.ff=only,
but not --ff-only?

> +test_expect_failure 'pull.ff=only overrides pull.rebase=true' '
> +	test_attempts_fast_forward -c pull.ff=only -c pull.rebase=true pull
> +'
> +
> +test_expect_success 'pull.ff=only overrides pull.rebase=false' '
> +	test_attempts_fast_forward -c pull.ff=only -c pull.rebase=false pull
> +'

This breaks existing behavior.

Users expect `-c pull.ff=only -c pull.rebase=true` to do a rebase.

> +test_expect_failure '--no-rebase takes precedence over pull.ff=only' '
> +	test_falls_back_to_full_merge -c pull.ff=only pull --no-rebase
> +'

This breaks existing behavior.

-- 
Felipe Contreras

RE: [PATCH v2 3/8] pull: abort if --ff-only is given and fast-forwarding is impossible

[Felipe Contreras]

Alex Henrie via GitGitGadget wrote:
> From: Alex Henrie <alexhenrie24@gmail.com>
> 
> The warning about pulling without specifying how to reconcile divergent
> branches says that after setting pull.rebase to true, --ff-only can
> still be passed on the command line to require a fast-forward. Make that
> actually work.

This is a backwards-incompatible change.

-- 
Felipe Contreras

RE: [PATCH v2 5/8] pull: make --rebase and --no-rebase override pull.ff=only

[Felipe Contreras]

Elijah Newren via GitGitGadget wrote:
> From: Elijah Newren <newren@gmail.com>
> 
> Fix the last few precedence tests failing in t7601 by now implementing
> the logic to have --[no-]rebase override a pull.ff=only config setting.

Once again, this breaks existing behavior.

-- 
Felipe Contreras

RE: [PATCH v2 6/8] pull: abort by default when fast-forwarding is not possible

[Felipe Contreras]

Elijah Newren via GitGitGadget wrote:
> From: Elijah Newren <newren@gmail.com>
> 
> 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.

Yes, but the warning doesn't say anything about changing the defaults or
deprecation.

-- 
Felipe Contreras

Re: [PATCH v2 4/8] pull: since --ff-only overrides, handle it first

[Matthias Baumgarten]

On 7/21/21 3:42 AM, Elijah Newren via GitGitGadget wrote:
> There are both merge and rebase branches in the logic, and previously
> both had to handle fast-forwarding.  Merge handled that implicitly
> (because git merge handles it directly), while in rebase it was
> explicit.  Given that the --ff-only flag is meant to override any
> --rebase or --no-rebase, make the code reflect that by handling
> --ff-only before the merge-vs-rebase logic.

Great. That now will work as I would expect it to.

-- 
aixigo AG
Karl-Friedrich-Str. 68, 52072 Aachen, Germany
phone: +49 (0)241 559709-390, fax: +49 (0)241 559709-99
email: matthias.baumgarten@aixigo.com
web: https://www.aixigo.com
District Court Aachen – HRB 8057
Board: Christian Friedrich, Tobias Haustein
Chairman of the Supervisory Board: Dr. Roland Schlager

Re: [PATCH v2 4/8] pull: since --ff-only overrides, handle it first

[Junio C Hamano]

"Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:

> -	if (!can_ff) {
> -		if (opt_ff) {
> -			if (!strcmp(opt_ff, "--ff-only"))
> -				die_ff_impossible();
> -		} else {
> -			if (rebase_unspecified && opt_verbosity >= 0)
> -				show_advice_pull_non_ff();
> -		}
> +	/* ff-only takes precedence over rebase */
> +	if (opt_ff && !strcmp(opt_ff, "--ff-only")) {
> +		if (!can_ff)
> +			die_ff_impossible();
> +		opt_rebase = REBASE_FALSE;
>  	}
> +	/* If no action specified and we can't fast forward, then warn. */
> +	if (!opt_ff && rebase_unspecified && !can_ff)
> +		show_advice_pull_non_ff();

This part makes sense, but ...

> @@ -1069,13 +1069,7 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
>  		    submodule_touches_in_range(the_repository, &upstream, &curr_head))
>  			die(_("cannot rebase with locally recorded submodule modifications"));
>  
> -		if (can_ff) {
> -			/* we can fast-forward this without invoking rebase */
> -			opt_ff = "--ff-only";
> -			ret = run_merge();
> -		} else {
> -			ret = run_rebase(&newbase, &upstream);
> -		}
> +		ret = run_rebase(&newbase, &upstream);

... as I already pointed out, this does not seem to belong to the
change.

What makes this hunk necessary?

We used to use run_merge() to fast-forward, now we let run_rebase()
to first "checkout" their tip, which ends up to be a fast-forward in
the "can_ff" situation.  As a side effect, opt_ff gets contaminated
with the current code, but that would not affect what happens after
this part (i.e. call to rebase_submodules()).

Re: [PATCH v2 4/8] pull: since --ff-only overrides, handle it first

[Felipe Contreras]

Matthias Baumgarten wrote:
> On 7/21/21 3:42 AM, Elijah Newren via GitGitGadget wrote:
> > There are both merge and rebase branches in the logic, and previously
> > both had to handle fast-forwarding.  Merge handled that implicitly
> > (because git merge handles it directly), while in rebase it was
> > explicit.  Given that the --ff-only flag is meant to override any
> > --rebase or --no-rebase, make the code reflect that by handling
> > --ff-only before the merge-vs-rebase logic.
> 
> Great. That now will work as I would expect it to.

Yes, but what about the current users that have setup pull.ff=only
already?

They are already relying on existing behavior.

What do you think users currently expect this to do?

  [pull]
    ff = only
    rebase = true

  git pull --no-rebase

-- 
Felipe Contreras

Re: [PATCH v2 4/8] pull: since --ff-only overrides, handle it first

[Elijah Newren]

On Wed, Jul 21, 2021 at 1:18 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > -     if (!can_ff) {
> > -             if (opt_ff) {
> > -                     if (!strcmp(opt_ff, "--ff-only"))
> > -                             die_ff_impossible();
> > -             } else {
> > -                     if (rebase_unspecified && opt_verbosity >= 0)
> > -                             show_advice_pull_non_ff();
> > -             }
> > +     /* ff-only takes precedence over rebase */
> > +     if (opt_ff && !strcmp(opt_ff, "--ff-only")) {
> > +             if (!can_ff)
> > +                     die_ff_impossible();
> > +             opt_rebase = REBASE_FALSE;
> >       }
> > +     /* If no action specified and we can't fast forward, then warn. */
> > +     if (!opt_ff && rebase_unspecified && !can_ff)
> > +             show_advice_pull_non_ff();
>
> This part makes sense, but ...
>
> > @@ -1069,13 +1069,7 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
> >                   submodule_touches_in_range(the_repository, &upstream, &curr_head))
> >                       die(_("cannot rebase with locally recorded submodule modifications"));
> >
> > -             if (can_ff) {
> > -                     /* we can fast-forward this without invoking rebase */
> > -                     opt_ff = "--ff-only";
> > -                     ret = run_merge();
> > -             } else {
> > -                     ret = run_rebase(&newbase, &upstream);
> > -             }
> > +             ret = run_rebase(&newbase, &upstream);
>
> ... as I already pointed out, this does not seem to belong to the
> change.
>
> What makes this hunk necessary?
>
> We used to use run_merge() to fast-forward, now we let run_rebase()
> to first "checkout" their tip, which ends up to be a fast-forward in
> the "can_ff" situation.  As a side effect, opt_ff gets contaminated
> with the current code, but that would not affect what happens after
> this part (i.e. call to rebase_submodules()).

Indeed, you are right.  Sorry about that, I'll re-roll with this hunk
removed, and the modifications to t5520 pulled out as well.

[PATCH v3 0/8] Handle pull option precedence

[Elijah Newren via GitGitGadget]

Based on a recent list of rules for flag/option precedence for git-pull[1]
from Junio (particularly focusing on rebase vs. merge vs. fast-forward),
here's an attempt to implement and document it. Given multiple recent
surprises from users about some of these behaviors[2][3] and a coworker just
yesterday expressing some puzzlement with git-pull and rebase vs. merge, it
seems like a good time to address some of these issues.

Since the handling of conflicting options was holding up two of Alex's
patches[4][5], and his patches fix some of the tests, I also include those
two patches in my series, with a few small changes to the first (so I've
kept him as author) and more substantial changes to the second (so I've
given him an Initial-patch-by attribution).

Changes since v2:

 * Remove some unnecessary changes in patch 4, pointed out by Junio.

Changes since v1:

 * Rebased on latest master (resolved a simple conflict with
   dd/test-stdout-count-lines)
 * Patch 1: based on feedback from Junio, fixed some style issues, clarified
   function names, added a few new tests, and took a stab at fixing up the
   comments and test descriptions (but still unsure if I hit the mark on the
   last point)
 * Patch 2: changed the test expectations for one of the multiple head tests
   as per Junio's suggestion, and made one of the other tests expect a more
   specific error message
 * Patches 4 & 5 were squashed and fixed: these now address a submodule bug
   interaction with --ff-only
 * Old patch 6 (now 5): added a code comment explaining a subtle point
 * Old patch 8 (now 7): a few more documentation updates, especially making
   --ff-only not sound merge-specific
 * Old patch 9 (now 8): Updates for new test expectation from patch 2

Quick overview:

 * Patches 1-2: new testcases (see the commit messages for the rules)
 * Patch 3: Alex's recent patch (abort if --ff-only but can't do so)
 * Patches 4-6: fix the precedence parts Alex didn't cover
 * Patch 7: Alex's other patch, abort if rebase vs. merge not specified
 * Patch 8: Compatibility of git-pull with merge-options.txt (think
   rebasing)
 * Patch 9: Fix multiple heads handling too

[1] https://lore.kernel.org/git/xmqqwnpqot4m.fsf@gitster.g/ [2]
https://lore.kernel.org/git/CAL3xRKdOyVWvcLXK7zoXtFPiHBjgL24zi5hhg+3yjowwSUPgmg@mail.gmail.com/
[3]
https://lore.kernel.org/git/c62933fb-96b2-99f5-7169-372f486f6e39@aixigo.com/
[4]
https://lore.kernel.org/git/20210711012604.947321-1-alexhenrie24@gmail.com/
[5]
https://lore.kernel.org/git/20210627000855.530985-1-alexhenrie24@gmail.com/

Alex Henrie (1):
  pull: abort if --ff-only is given and fast-forwarding is impossible

Elijah Newren (7):
  t7601: test interaction of merge/rebase/fast-forward flags and options
  t7601: add tests of interactions with multiple merge heads and config
  pull: since --ff-only overrides, handle it first
  pull: make --rebase and --no-rebase override pull.ff=only
  pull: abort by default when fast-forwarding is not possible
  pull: update docs & code for option compatibility with rebasing
  pull: fix handling of multiple heads

 Documentation/git-merge.txt     |   2 +
 Documentation/git-pull.txt      |  21 +--
 Documentation/merge-options.txt |  40 ++++++
 advice.c                        |   5 +
 advice.h                        |   1 +
 builtin/merge.c                 |   2 +-
 builtin/pull.c                  |  55 +++++--
 t/t4013-diff-various.sh         |   2 +-
 t/t5520-pull.sh                 |  20 +--
 t/t5521-pull-options.sh         |   4 +-
 t/t5524-pull-msg.sh             |   4 +-
 t/t5553-set-upstream.sh         |  14 +-
 t/t5604-clone-reference.sh      |   4 +-
 t/t6402-merge-rename.sh         |  18 +--
 t/t6409-merge-subtree.sh        |   6 +-
 t/t6417-merge-ours-theirs.sh    |  10 +-
 t/t7601-merge-pull-config.sh    | 244 +++++++++++++++++++++++++++++---
 t/t7603-merge-reduce-heads.sh   |   2 +-
 18 files changed, 371 insertions(+), 83 deletions(-)


base-commit: daab8a564f8bbac55f70f8bf86c070e001a9b006
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1049%2Fnewren%2Fhandle-pull-option-precedence-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1049/newren/handle-pull-option-precedence-v3
Pull-Request: https://github.com/git/git/pull/1049

Range-diff vs v2:

 1:  17560927211 = 1:  17560927211 t7601: test interaction of merge/rebase/fast-forward flags and options
 2:  66fe7f7f934 = 2:  66fe7f7f934 t7601: add tests of interactions with multiple merge heads and config
 3:  c45cd239666 = 3:  c45cd239666 pull: abort if --ff-only is given and fast-forwarding is impossible
 4:  1a821d3b1dd ! 4:  0682b2250f4 pull: since --ff-only overrides, handle it first
     @@ builtin/pull.c: int cmd_pull(int argc, const char **argv, const char *prefix)
       
       	if (opt_rebase) {
       		int ret = 0;
     -@@ builtin/pull.c: int cmd_pull(int argc, const char **argv, const char *prefix)
     - 		    submodule_touches_in_range(the_repository, &upstream, &curr_head))
     - 			die(_("cannot rebase with locally recorded submodule modifications"));
     - 
     --		if (can_ff) {
     --			/* we can fast-forward this without invoking rebase */
     --			opt_ff = "--ff-only";
     --			ret = run_merge();
     --		} else {
     --			ret = run_rebase(&newbase, &upstream);
     --		}
     -+		ret = run_rebase(&newbase, &upstream);
     - 
     - 		if (!ret && (recurse_submodules == RECURSE_SUBMODULES_ON ||
     - 			     recurse_submodules == RECURSE_SUBMODULES_ON_DEMAND))
     -
     - ## t/t5520-pull.sh ##
     -@@ t/t5520-pull.sh: test_expect_success '--rebase (merge) fast forward' '
     - 	# The above only validates the result.  Did we actually bypass rebase?
     - 	git reflog -1 >reflog.actual &&
     - 	sed "s/^[0-9a-f][0-9a-f]*/OBJID/" reflog.actual >reflog.fuzzy &&
     --	echo "OBJID HEAD@{0}: pull --rebase . ff: Fast-forward" >reflog.expected &&
     -+	echo "OBJID HEAD@{0}: pull --rebase . ff (finish): returning to refs/heads/to-rebase" >reflog.expected &&
     - 	test_cmp reflog.expected reflog.fuzzy
     - '
     - 
     -@@ t/t5520-pull.sh: test_expect_success '--rebase (am) fast forward' '
     - 
     - 	# The above only validates the result.  Did we actually bypass rebase?
     - 	git reflog -1 >reflog.actual &&
     --	sed "s/^[0-9a-f][0-9a-f]*/OBJID/" reflog.actual >reflog.fuzzy &&
     --	echo "OBJID HEAD@{0}: pull --rebase . ff: Fast-forward" >reflog.expected &&
     -+	sed -e "s/^[0-9a-f][0-9a-f]*/OBJID/" -e "s/[0-9a-f][0-9a-f]*$/OBJID/" reflog.actual >reflog.fuzzy &&
     -+	echo "OBJID HEAD@{0}: rebase finished: refs/heads/to-rebase onto OBJID" >reflog.expected &&
     - 	test_cmp reflog.expected reflog.fuzzy
     - '
     - 
 5:  9b116f3d284 = 5:  b242d001132 pull: make --rebase and --no-rebase override pull.ff=only
 6:  f061f8b4e75 = 6:  7447c719bd7 pull: abort by default when fast-forwarding is not possible
 7:  90d49e0fb78 = 7:  726082f2e79 pull: update docs & code for option compatibility with rebasing
 8:  f03b15b7eb0 = 8:  768dec71558 pull: fix handling of multiple heads

-- 
gitgitgadget

[PATCH v3 1/8] t7601: test interaction of merge/rebase/fast-forward flags and options

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

The interaction of rebase and merge flags and options was not well
tested.  Add several tests to check for correct behavior from the
following rules:
    * --ff-only vs. --[no-]rebase
      (and the related pull.ff=only vs. pull.rebase)
    * --rebase[=!false] vs. --no-ff and --ff
      (and the related pull.rebase=!false overrides pull.ff=!only)
    * command line flags take precedence over config, except:
      * --no-rebase heeds pull.ff=!only
      * pull.rebase=!false vs --no-ff and --ff

For more details behind these rules and a larger table of individual
cases, refer to https://lore.kernel.org/git/xmqqwnpqot4m.fsf@gitster.g/
and the links found therein.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 t/t7601-merge-pull-config.sh | 182 +++++++++++++++++++++++++++++++++++
 1 file changed, 182 insertions(+)

diff --git a/t/t7601-merge-pull-config.sh b/t/t7601-merge-pull-config.sh
index 52e8ccc933a..a44e6e69f2b 100755
--- a/t/t7601-merge-pull-config.sh
+++ b/t/t7601-merge-pull-config.sh
@@ -143,6 +143,188 @@ test_expect_success 'pull.rebase not set and --ff-only given (not-fast-forward)'
 	test_i18ngrep ! "Pulling without specifying how to reconcile" err
 '
 
+test_does_rebase () {
+	git reset --hard c2 &&
+	git "$@" . c1 &&
+	# Check that we actually did a rebase
+	git rev-list --count HEAD >actual &&
+	git rev-list --merges --count HEAD >>actual &&
+	test_write_lines 3 0 >expect &&
+	test_cmp expect actual &&
+	rm actual expect
+}
+
+# Prefers merge over fast-forward
+test_does_merge_when_ff_possible () {
+	git reset --hard c0 &&
+	git "$@" . c1 &&
+	# Check that we actually did a merge
+	git rev-list --count HEAD >actual &&
+	git rev-list --merges --count HEAD >>actual &&
+	test_write_lines 3 1 >expect &&
+	test_cmp expect actual &&
+	rm actual expect
+}
+
+# Prefers fast-forward over merge or rebase
+test_does_fast_forward () {
+	git reset --hard c0 &&
+	git "$@" . c1 &&
+
+	# Check that we did not get any merges
+	git rev-list --count HEAD >actual &&
+	git rev-list --merges --count HEAD >>actual &&
+	test_write_lines 2 0 >expect &&
+	test_cmp expect actual &&
+
+	# Check that we ended up at c1
+	git rev-parse HEAD >actual &&
+	git rev-parse c1^{commit} >expect &&
+	test_cmp actual expect &&
+
+	# Remove temporary files
+	rm actual expect
+}
+
+# Doesn't fail when fast-forward not possible; does a merge
+test_falls_back_to_full_merge () {
+	git reset --hard c2 &&
+	git "$@" . c1 &&
+	# Check that we actually did a merge
+	git rev-list --count HEAD >actual &&
+	git rev-list --merges --count HEAD >>actual &&
+	test_write_lines 4 1 >expect &&
+	test_cmp expect actual &&
+	rm actual expect
+}
+
+# Attempts fast forward, which is impossible, and bails
+test_attempts_fast_forward () {
+	git reset --hard c2 &&
+	test_must_fail git "$@" . c1 2>err &&
+	test_i18ngrep "Not possible to fast-forward, aborting" err
+}
+
+#
+# Group 1: Interaction of --ff-only with --[no-]rebase
+# (And related interaction of pull.ff=only with pull.rebase)
+#
+test_expect_failure '--ff-only overrides --rebase' '
+	test_attempts_fast_forward pull --rebase --ff-only
+'
+
+test_expect_failure '--ff-only overrides --rebase even if first' '
+	test_attempts_fast_forward pull --ff-only --rebase
+'
+
+test_expect_success '--ff-only overrides --no-rebase' '
+	test_attempts_fast_forward pull --ff-only --no-rebase
+'
+
+test_expect_failure 'pull.ff=only overrides pull.rebase=true' '
+	test_attempts_fast_forward -c pull.ff=only -c pull.rebase=true pull
+'
+
+test_expect_success 'pull.ff=only overrides pull.rebase=false' '
+	test_attempts_fast_forward -c pull.ff=only -c pull.rebase=false pull
+'
+
+# Group 2: --rebase=[!false] overrides --no-ff and --ff
+# (And related interaction of pull.rebase=!false and pull.ff=!only)
+test_expect_success '--rebase overrides --no-ff' '
+	test_does_rebase pull --rebase --no-ff
+'
+
+test_expect_success '--rebase overrides --ff' '
+	test_does_rebase pull --rebase --ff
+'
+
+test_expect_success '--rebase fast-forwards when possible' '
+	test_does_fast_forward pull --rebase --ff
+'
+
+test_expect_success 'pull.rebase=true overrides pull.ff=false' '
+	test_does_rebase -c pull.rebase=true -c pull.ff=false pull
+'
+
+test_expect_success 'pull.rebase=true overrides pull.ff=true' '
+	test_does_rebase -c pull.rebase=true -c pull.ff=true pull
+'
+
+# Group 3: command line flags take precedence over config
+test_expect_failure '--ff-only takes precedence over pull.rebase=true' '
+	test_attempts_fast_forward -c pull.rebase=true pull --ff-only
+'
+
+test_expect_success '--ff-only takes precedence over pull.rebase=false' '
+	test_attempts_fast_forward -c pull.rebase=false pull --ff-only
+'
+
+test_expect_failure '--no-rebase takes precedence over pull.ff=only' '
+	test_falls_back_to_full_merge -c pull.ff=only pull --no-rebase
+'
+
+test_expect_success '--rebase takes precedence over pull.ff=only' '
+	test_does_rebase -c pull.ff=only pull --rebase
+'
+
+test_expect_success '--rebase overrides pull.ff=true' '
+	test_does_rebase -c pull.ff=true pull --rebase
+'
+
+test_expect_success '--rebase overrides pull.ff=false' '
+	test_does_rebase -c pull.ff=false pull --rebase
+'
+
+test_expect_success '--rebase overrides pull.ff unset' '
+	test_does_rebase pull --rebase
+'
+
+# Group 4: --no-rebase heeds pull.ff=!only or explict --ff or --no-ff
+
+test_expect_success '--no-rebase works with --no-ff' '
+	test_does_merge_when_ff_possible pull --no-rebase --no-ff
+'
+
+test_expect_success '--no-rebase works with --ff' '
+	test_does_fast_forward pull --no-rebase --ff
+'
+
+test_expect_success '--no-rebase does ff if pull.ff unset' '
+	test_does_fast_forward pull --no-rebase
+'
+
+test_expect_success '--no-rebase heeds pull.ff=true' '
+	test_does_fast_forward -c pull.ff=true pull --no-rebase
+'
+
+test_expect_success '--no-rebase heeds pull.ff=false' '
+	test_does_merge_when_ff_possible -c pull.ff=false pull --no-rebase
+'
+
+# Group 5: pull.rebase=!false in combination with --no-ff or --ff
+test_expect_success 'pull.rebase=true and --no-ff' '
+	test_does_rebase -c pull.rebase=true pull --no-ff
+'
+
+test_expect_success 'pull.rebase=true and --ff' '
+	test_does_rebase -c pull.rebase=true pull --ff
+'
+
+test_expect_success 'pull.rebase=false and --no-ff' '
+	test_does_merge_when_ff_possible -c pull.rebase=false pull --no-ff
+'
+
+test_expect_success 'pull.rebase=false and --ff, ff possible' '
+	test_does_fast_forward -c pull.rebase=false pull --ff
+'
+
+test_expect_success 'pull.rebase=false and --ff, ff not possible' '
+	test_falls_back_to_full_merge -c pull.rebase=false pull --ff
+'
+
+# End of groupings for conflicting merge vs. rebase flags/options
+
 test_expect_success 'merge c1 with c2' '
 	git reset --hard c1 &&
 	test -f c0.c &&
-- 
gitgitgadget

[PATCH v3 2/8] t7601: add tests of interactions with multiple merge heads and config

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

There were already code checking that --rebase was incompatible with
a merge of multiple heads.  However, we were sometimes throwing warnings
about lack of specification of rebase vs. merge when given multiple
heads.  Since rebasing is disallowed with multiple merge heads, that
seems like a poor warning to print; we should instead just assume
merging is wanted.

Add a few tests checking multiple merge head behavior.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 t/t7601-merge-pull-config.sh | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/t/t7601-merge-pull-config.sh b/t/t7601-merge-pull-config.sh
index a44e6e69f2b..21db1e9e14b 100755
--- a/t/t7601-merge-pull-config.sh
+++ b/t/t7601-merge-pull-config.sh
@@ -325,6 +325,28 @@ test_expect_success 'pull.rebase=false and --ff, ff not possible' '
 
 # End of groupings for conflicting merge vs. rebase flags/options
 
+test_expect_failure 'Multiple heads warns about inability to fast forward' '
+	git reset --hard c1 &&
+	test_must_fail git pull . c2 c3 2>err &&
+	test_i18ngrep "Pulling without specifying how to reconcile" err
+'
+
+test_expect_failure 'Multiple can never be fast forwarded' '
+	git reset --hard c0 &&
+	test_must_fail git -c pull.ff=only pull . c1 c2 c3 2>err &&
+	test_i18ngrep ! "Pulling without specifying how to reconcile" err &&
+	# In addition to calling out "cannot fast-forward", we very much
+	# want the "multiple branches" piece to be called out to users.
+	test_i18ngrep "Cannot fast-forward to multiple branches" err
+'
+
+test_expect_success 'Cannot rebase with multiple heads' '
+	git reset --hard c0 &&
+	test_must_fail git -c pull.rebase=true pull . c1 c2 c3 2>err &&
+	test_i18ngrep ! "Pulling without specifying how to reconcile" err &&
+	test_i18ngrep "Cannot rebase onto multiple branches." err
+'
+
 test_expect_success 'merge c1 with c2' '
 	git reset --hard c1 &&
 	test -f c0.c &&
-- 
gitgitgadget

[PATCH v3 3/8] pull: abort if --ff-only is given and fast-forwarding is impossible

[Alex Henrie via GitGitGadget]

From: Alex Henrie <alexhenrie24@gmail.com>

The warning about pulling without specifying how to reconcile divergent
branches says that after setting pull.rebase to true, --ff-only can
still be passed on the command line to require a fast-forward. Make that
actually work.

Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>
[en: updated tests; note 3 fixes and 1 new failure]
Signed-off-by: Elijah Newren <newren@gmail.com>
---
 advice.c                     |  5 +++++
 advice.h                     |  1 +
 builtin/merge.c              |  2 +-
 builtin/pull.c               | 11 ++++++++---
 t/t7601-merge-pull-config.sh | 10 +++++-----
 5 files changed, 20 insertions(+), 9 deletions(-)

diff --git a/advice.c b/advice.c
index 0b9c89c48ab..337e8f342bc 100644
--- a/advice.c
+++ b/advice.c
@@ -286,6 +286,11 @@ void NORETURN die_conclude_merge(void)
 	die(_("Exiting because of unfinished merge."));
 }
 
+void NORETURN die_ff_impossible(void)
+{
+	die(_("Not possible to fast-forward, aborting."));
+}
+
 void advise_on_updating_sparse_paths(struct string_list *pathspec_list)
 {
 	struct string_list_item *item;
diff --git a/advice.h b/advice.h
index bd26c385d00..16240438387 100644
--- a/advice.h
+++ b/advice.h
@@ -95,6 +95,7 @@ void advise_if_enabled(enum advice_type type, const char *advice, ...);
 int error_resolve_conflict(const char *me);
 void NORETURN die_resolve_conflict(const char *me);
 void NORETURN die_conclude_merge(void);
+void NORETURN die_ff_impossible(void);
 void advise_on_updating_sparse_paths(struct string_list *pathspec_list);
 void detach_advice(const char *new_name);
 
diff --git a/builtin/merge.c b/builtin/merge.c
index a8a843b1f54..aa920ac524f 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -1620,7 +1620,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 	}
 
 	if (fast_forward == FF_ONLY)
-		die(_("Not possible to fast-forward, aborting."));
+		die_ff_impossible();
 
 	if (autostash)
 		create_autostash(the_repository,
diff --git a/builtin/pull.c b/builtin/pull.c
index 3e13f810843..d9796604825 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -1046,9 +1046,14 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 
 	can_ff = get_can_ff(&orig_head, &merge_heads.oid[0]);
 
-	if (rebase_unspecified && !opt_ff && !can_ff) {
-		if (opt_verbosity >= 0)
-			show_advice_pull_non_ff();
+	if (!can_ff) {
+		if (opt_ff) {
+			if (!strcmp(opt_ff, "--ff-only"))
+				die_ff_impossible();
+		} else {
+			if (rebase_unspecified && opt_verbosity >= 0)
+				show_advice_pull_non_ff();
+		}
 	}
 
 	if (opt_rebase) {
diff --git a/t/t7601-merge-pull-config.sh b/t/t7601-merge-pull-config.sh
index 21db1e9e14b..d1f621725ad 100755
--- a/t/t7601-merge-pull-config.sh
+++ b/t/t7601-merge-pull-config.sh
@@ -209,11 +209,11 @@ test_attempts_fast_forward () {
 # Group 1: Interaction of --ff-only with --[no-]rebase
 # (And related interaction of pull.ff=only with pull.rebase)
 #
-test_expect_failure '--ff-only overrides --rebase' '
+test_expect_success '--ff-only overrides --rebase' '
 	test_attempts_fast_forward pull --rebase --ff-only
 '
 
-test_expect_failure '--ff-only overrides --rebase even if first' '
+test_expect_success '--ff-only overrides --rebase even if first' '
 	test_attempts_fast_forward pull --ff-only --rebase
 '
 
@@ -221,7 +221,7 @@ test_expect_success '--ff-only overrides --no-rebase' '
 	test_attempts_fast_forward pull --ff-only --no-rebase
 '
 
-test_expect_failure 'pull.ff=only overrides pull.rebase=true' '
+test_expect_success 'pull.ff=only overrides pull.rebase=true' '
 	test_attempts_fast_forward -c pull.ff=only -c pull.rebase=true pull
 '
 
@@ -252,7 +252,7 @@ test_expect_success 'pull.rebase=true overrides pull.ff=true' '
 '
 
 # Group 3: command line flags take precedence over config
-test_expect_failure '--ff-only takes precedence over pull.rebase=true' '
+test_expect_success '--ff-only takes precedence over pull.rebase=true' '
 	test_attempts_fast_forward -c pull.rebase=true pull --ff-only
 '
 
@@ -264,7 +264,7 @@ test_expect_failure '--no-rebase takes precedence over pull.ff=only' '
 	test_falls_back_to_full_merge -c pull.ff=only pull --no-rebase
 '
 
-test_expect_success '--rebase takes precedence over pull.ff=only' '
+test_expect_failure '--rebase takes precedence over pull.ff=only' '
 	test_does_rebase -c pull.ff=only pull --rebase
 '
 
-- 
gitgitgadget

[PATCH v3 4/8] pull: since --ff-only overrides, handle it first

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

There are both merge and rebase branches in the logic, and previously
both had to handle fast-forwarding.  Merge handled that implicitly
(because git merge handles it directly), while in rebase it was
explicit.  Given that the --ff-only flag is meant to override any
--rebase or --no-rebase, make the code reflect that by handling
--ff-only before the merge-vs-rebase logic.

It turns out that this also fixes a bug for submodules.  Previously,
when --ff-only was given, the code would run `merge --ff-only` on the
main module, and then run `submodule update --recursive --rebase` on the
submodules.  With this change, we still run `merge --ff-only` on the
main module, but now run `submodule update --recursive --checkout` on
the submodules.  I believe this better reflects the intent of --ff-only
to have it apply to both the main module and the submodules.

(Sidenote: It is somewhat interesting that all merges pass `--checkout`
to submodule update, even when `--no-ff` is specified, meaning that it
will only do fast-forward merges for submodules.  This was discussed in
commit a6d7eb2c7a ("pull: optionally rebase submodules (remote submodule
changes only)", 2017-06-23).  The same limitations apply now as then, so
we are not trying to fix this at this time.)

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 builtin/pull.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/builtin/pull.c b/builtin/pull.c
index d9796604825..1f452020375 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -1046,15 +1046,15 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 
 	can_ff = get_can_ff(&orig_head, &merge_heads.oid[0]);
 
-	if (!can_ff) {
-		if (opt_ff) {
-			if (!strcmp(opt_ff, "--ff-only"))
-				die_ff_impossible();
-		} else {
-			if (rebase_unspecified && opt_verbosity >= 0)
-				show_advice_pull_non_ff();
-		}
+	/* ff-only takes precedence over rebase */
+	if (opt_ff && !strcmp(opt_ff, "--ff-only")) {
+		if (!can_ff)
+			die_ff_impossible();
+		opt_rebase = REBASE_FALSE;
 	}
+	/* If no action specified and we can't fast forward, then warn. */
+	if (!opt_ff && rebase_unspecified && !can_ff)
+		show_advice_pull_non_ff();
 
 	if (opt_rebase) {
 		int ret = 0;
-- 
gitgitgadget

[PATCH v3 5/8] pull: make --rebase and --no-rebase override pull.ff=only

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

Fix the last few precedence tests failing in t7601 by now implementing
the logic to have --[no-]rebase override a pull.ff=only config setting.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 builtin/pull.c               | 16 +++++++++++++++-
 t/t7601-merge-pull-config.sh |  4 ++--
 2 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/builtin/pull.c b/builtin/pull.c
index 1f452020375..9bf03255291 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -966,8 +966,22 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 
 	parse_repo_refspecs(argc, argv, &repo, &refspecs);
 
-	if (!opt_ff)
+	if (!opt_ff) {
 		opt_ff = xstrdup_or_null(config_get_ff());
+		/*
+		 * A subtle point: opt_ff was set on the line above via
+		 * reading from config.  opt_rebase, in contrast, is set
+		 * before this point via command line options.  The setting
+		 * of opt_rebase via reading from config (using
+		 * config_get_rebase()) does not happen until later.  We
+		 * are relying on the next if-condition happening before
+		 * the config_get_rebase() call so that an explicit
+		 * "--rebase" can override a config setting of
+		 * pull.ff=only.
+		 */
+		if (opt_rebase >= 0 && opt_ff && !strcmp(opt_ff, "--ff-only"))
+			opt_ff = "--ff";
+	}
 
 	if (opt_rebase < 0)
 		opt_rebase = config_get_rebase(&rebase_unspecified);
diff --git a/t/t7601-merge-pull-config.sh b/t/t7601-merge-pull-config.sh
index d1f621725ad..0c0dfecd2a3 100755
--- a/t/t7601-merge-pull-config.sh
+++ b/t/t7601-merge-pull-config.sh
@@ -260,11 +260,11 @@ test_expect_success '--ff-only takes precedence over pull.rebase=false' '
 	test_attempts_fast_forward -c pull.rebase=false pull --ff-only
 '
 
-test_expect_failure '--no-rebase takes precedence over pull.ff=only' '
+test_expect_success '--no-rebase takes precedence over pull.ff=only' '
 	test_falls_back_to_full_merge -c pull.ff=only pull --no-rebase
 '
 
-test_expect_failure '--rebase takes precedence over pull.ff=only' '
+test_expect_success '--rebase takes precedence over pull.ff=only' '
 	test_does_rebase -c pull.ff=only pull --rebase
 '
 
-- 
gitgitgadget

[PATCH v3 6/8] pull: abort by default when fast-forwarding is not possible

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

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.

Initial-patch-by: Alex Henrie <alexhenrie24@gmail.com>
Signed-off-by: Elijah Newren <newren@gmail.com>
---
 Documentation/git-pull.txt    | 20 +++++++++------
 builtin/pull.c                | 10 +++++---
 t/t4013-diff-various.sh       |  2 +-
 t/t5520-pull.sh               | 20 +++++++--------
 t/t5521-pull-options.sh       |  4 +--
 t/t5524-pull-msg.sh           |  4 +--
 t/t5553-set-upstream.sh       | 14 +++++-----
 t/t5604-clone-reference.sh    |  4 +--
 t/t6402-merge-rename.sh       | 18 ++++++-------
 t/t6409-merge-subtree.sh      |  6 ++---
 t/t6417-merge-ours-theirs.sh  | 10 ++++----
 t/t7601-merge-pull-config.sh  | 48 +++++++++++++++++------------------
 t/t7603-merge-reduce-heads.sh |  2 +-
 13 files changed, 84 insertions(+), 78 deletions(-)

diff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt
index 5c3fb67c014..cad3f6bfcee 100644
--- a/Documentation/git-pull.txt
+++ b/Documentation/git-pull.txt
@@ -15,14 +15,18 @@ SYNOPSIS
 DESCRIPTION
 -----------
 
-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'.
+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`, `--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 configuration options or command line flags,
+will call either `git merge` or `git rebase` to reconcile diverging
+branches.
 
 <repository> should be the name of a remote repository as
 passed to linkgit:git-fetch[1].  <refspec> can name an
diff --git a/builtin/pull.c b/builtin/pull.c
index 9bf03255291..4514a1478ea 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -927,9 +927,9 @@ static int get_can_ff(struct object_id *orig_head, struct object_id *orig_merge_
 
 static void show_advice_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"
+	advise(_("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"
 		 "\n"
 		 "  git config pull.rebase false  # merge (the default strategy)\n"
 		 "  git config pull.rebase true   # rebase\n"
@@ -1067,8 +1067,10 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 		opt_rebase = REBASE_FALSE;
 	}
 	/* If no action specified and we can't fast forward, then warn. */
-	if (!opt_ff && rebase_unspecified && !can_ff)
+	if (!opt_ff && rebase_unspecified && !can_ff) {
 		show_advice_pull_non_ff();
+		die(_("Need to specify how to reconcile divergent branches."));
+	}
 
 	if (opt_rebase) {
 		int ret = 0;
diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
index 7fadc985ccc..eb989f7f191 100755
--- a/t/t4013-diff-various.sh
+++ b/t/t4013-diff-various.sh
@@ -65,7 +65,7 @@ test_expect_success setup '
 	export GIT_AUTHOR_DATE GIT_COMMITTER_DATE &&
 
 	git checkout master &&
-	git pull -s ours . side &&
+	git pull -s ours --no-rebase . side &&
 
 	GIT_AUTHOR_DATE="2006-06-26 00:05:00 +0000" &&
 	GIT_COMMITTER_DATE="2006-06-26 00:05:00 +0000" &&
diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index e2c0c510222..672001a18bd 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh
@@ -136,12 +136,12 @@ test_expect_success 'the default remote . should not break explicit pull' '
 	git reset --hard HEAD^ &&
 	echo file >expect &&
 	test_cmp expect file &&
-	git pull . second &&
+	git pull --no-rebase . second &&
 	echo modified >expect &&
 	test_cmp expect file &&
 	git reflog -1 >reflog.actual &&
 	sed "s/^[0-9a-f][0-9a-f]*/OBJID/" reflog.actual >reflog.fuzzy &&
-	echo "OBJID HEAD@{0}: pull . second: Fast-forward" >reflog.expected &&
+	echo "OBJID HEAD@{0}: pull --no-rebase . second: Fast-forward" >reflog.expected &&
 	test_cmp reflog.expected reflog.fuzzy
 '
 
@@ -226,7 +226,7 @@ test_expect_success 'fail if the index has unresolved entries' '
 	test_commit modified2 file &&
 	git ls-files -u >unmerged &&
 	test_must_be_empty unmerged &&
-	test_must_fail git pull . second &&
+	test_must_fail git pull --no-rebase . second &&
 	git ls-files -u >unmerged &&
 	test_file_not_empty unmerged &&
 	cp file expected &&
@@ -409,37 +409,37 @@ test_expect_success 'pull --rebase --no-autostash & rebase.autostash unset' '
 
 test_expect_success 'pull succeeds with dirty working directory and merge.autostash set' '
 	test_config merge.autostash true &&
-	test_pull_autostash 2
+	test_pull_autostash 2 --no-rebase
 '
 
 test_expect_success 'pull --autostash & merge.autostash=true' '
 	test_config merge.autostash true &&
-	test_pull_autostash 2 --autostash
+	test_pull_autostash 2 --autostash --no-rebase
 '
 
 test_expect_success 'pull --autostash & merge.autostash=false' '
 	test_config merge.autostash false &&
-	test_pull_autostash 2 --autostash
+	test_pull_autostash 2 --autostash --no-rebase
 '
 
 test_expect_success 'pull --autostash & merge.autostash unset' '
 	test_unconfig merge.autostash &&
-	test_pull_autostash 2 --autostash
+	test_pull_autostash 2 --autostash --no-rebase
 '
 
 test_expect_success 'pull --no-autostash & merge.autostash=true' '
 	test_config merge.autostash true &&
-	test_pull_autostash_fail --no-autostash
+	test_pull_autostash_fail --no-autostash --no-rebase
 '
 
 test_expect_success 'pull --no-autostash & merge.autostash=false' '
 	test_config merge.autostash false &&
-	test_pull_autostash_fail --no-autostash
+	test_pull_autostash_fail --no-autostash --no-rebase
 '
 
 test_expect_success 'pull --no-autostash & merge.autostash unset' '
 	test_unconfig merge.autostash &&
-	test_pull_autostash_fail --no-autostash
+	test_pull_autostash_fail --no-autostash --no-rebase
 '
 
 test_expect_success 'pull.rebase' '
diff --git a/t/t5521-pull-options.sh b/t/t5521-pull-options.sh
index 63a688bdbf5..7601c919fdc 100755
--- a/t/t5521-pull-options.sh
+++ b/t/t5521-pull-options.sh
@@ -113,7 +113,7 @@ test_expect_success 'git pull --force' '
 	git pull two &&
 	test_commit A &&
 	git branch -f origin &&
-	git pull --all --force
+	git pull --no-rebase --all --force
 	)
 '
 
@@ -179,7 +179,7 @@ test_expect_success 'git pull --allow-unrelated-histories' '
 	(
 		cd dst &&
 		test_must_fail git pull ../src side &&
-		git pull --allow-unrelated-histories ../src side
+		git pull --no-rebase --allow-unrelated-histories ../src side
 	)
 '
 
diff --git a/t/t5524-pull-msg.sh b/t/t5524-pull-msg.sh
index c278adaa5a2..b2be3605f5a 100755
--- a/t/t5524-pull-msg.sh
+++ b/t/t5524-pull-msg.sh
@@ -28,7 +28,7 @@ test_expect_success setup '
 test_expect_success pull '
 (
 	cd cloned &&
-	git pull --log &&
+	git pull --no-rebase --log &&
 	git log -2 &&
 	git cat-file commit HEAD >result &&
 	grep Dollar result
@@ -41,7 +41,7 @@ test_expect_success '--log=1 limits shortlog length' '
 	git reset --hard HEAD^ &&
 	test "$(cat afile)" = original &&
 	test "$(cat bfile)" = added &&
-	git pull --log=1 &&
+	git pull --no-rebase --log=1 &&
 	git log -3 &&
 	git cat-file commit HEAD >result &&
 	grep Dollar result &&
diff --git a/t/t5553-set-upstream.sh b/t/t5553-set-upstream.sh
index b1d614ce18c..9c12c0f8c32 100755
--- a/t/t5553-set-upstream.sh
+++ b/t/t5553-set-upstream.sh
@@ -108,27 +108,27 @@ test_expect_success 'setup commit on main and other pull' '
 
 test_expect_success 'pull --set-upstream upstream main sets branch main but not other' '
 	clear_config main other &&
-	git pull --set-upstream upstream main &&
+	git pull --no-rebase --set-upstream upstream main &&
 	check_config main upstream refs/heads/main &&
 	check_config_missing other
 '
 
 test_expect_success 'pull --set-upstream main:other2 does not set the branch other2' '
 	clear_config other2 &&
-	git pull --set-upstream upstream main:other2 &&
+	git pull --no-rebase --set-upstream upstream main:other2 &&
 	check_config_missing other2
 '
 
 test_expect_success 'pull --set-upstream upstream other sets branch main' '
 	clear_config main other &&
-	git pull --set-upstream upstream other &&
+	git pull --no-rebase --set-upstream upstream other &&
 	check_config main upstream refs/heads/other &&
 	check_config_missing other
 '
 
 test_expect_success 'pull --set-upstream upstream tag does not set the tag' '
 	clear_config three &&
-	git pull --tags --set-upstream upstream three &&
+	git pull --no-rebase --tags --set-upstream upstream three &&
 	check_config_missing three
 '
 
@@ -144,16 +144,16 @@ test_expect_success 'pull --set-upstream http://nosuchdomain.example.com fails w
 
 test_expect_success 'pull --set-upstream upstream HEAD sets branch HEAD' '
 	clear_config main other &&
-	git pull --set-upstream upstream HEAD &&
+	git pull --no-rebase --set-upstream upstream HEAD &&
 	check_config main upstream HEAD &&
 	git checkout other &&
-	git pull --set-upstream upstream HEAD &&
+	git pull --no-rebase --set-upstream upstream HEAD &&
 	check_config other upstream HEAD
 '
 
 test_expect_success 'pull --set-upstream upstream with more than one branch does nothing' '
 	clear_config main three &&
-	git pull --set-upstream upstream main three &&
+	git pull --no-rebase --set-upstream upstream main three &&
 	check_config_missing main &&
 	check_config_missing three
 '
diff --git a/t/t5604-clone-reference.sh b/t/t5604-clone-reference.sh
index e845d621f61..24340e6d56e 100755
--- a/t/t5604-clone-reference.sh
+++ b/t/t5604-clone-reference.sh
@@ -87,7 +87,7 @@ test_expect_success 'updating origin' '
 '
 
 test_expect_success 'pulling changes from origin' '
-	git -C C pull origin
+	git -C C pull --no-rebase origin
 '
 
 # the 2 local objects are commit and tree from the merge
@@ -96,7 +96,7 @@ test_expect_success 'that alternate to origin gets used' '
 '
 
 test_expect_success 'pulling changes from origin' '
-	git -C D pull origin
+	git -C D pull --no-rebase origin
 '
 
 # the 5 local objects are expected; file3 blob, commit in A to add it
diff --git a/t/t6402-merge-rename.sh b/t/t6402-merge-rename.sh
index 3da2896e3bc..3a32b1a45cf 100755
--- a/t/t6402-merge-rename.sh
+++ b/t/t6402-merge-rename.sh
@@ -103,7 +103,7 @@ test_expect_success 'setup' '
 test_expect_success 'pull renaming branch into unrenaming one' \
 '
 	git show-branch &&
-	test_expect_code 1 git pull . white &&
+	test_expect_code 1 git pull --no-rebase . white &&
 	git ls-files -s &&
 	test_stdout_line_count = 3 git ls-files -u B &&
 	test_stdout_line_count = 1 git ls-files -s N &&
@@ -119,7 +119,7 @@ test_expect_success 'pull renaming branch into another renaming one' \
 	rm -f B &&
 	git reset --hard &&
 	git checkout red &&
-	test_expect_code 1 git pull . white &&
+	test_expect_code 1 git pull --no-rebase . white &&
 	test_stdout_line_count = 3 git ls-files -u B &&
 	test_stdout_line_count = 1 git ls-files -s N &&
 	sed -ne "/^g/{
@@ -133,7 +133,7 @@ test_expect_success 'pull unrenaming branch into renaming one' \
 '
 	git reset --hard &&
 	git show-branch &&
-	test_expect_code 1 git pull . main &&
+	test_expect_code 1 git pull --no-rebase . main &&
 	test_stdout_line_count = 3 git ls-files -u B &&
 	test_stdout_line_count = 1 git ls-files -s N &&
 	sed -ne "/^g/{
@@ -147,7 +147,7 @@ test_expect_success 'pull conflicting renames' \
 '
 	git reset --hard &&
 	git show-branch &&
-	test_expect_code 1 git pull . blue &&
+	test_expect_code 1 git pull --no-rebase . blue &&
 	test_stdout_line_count = 1 git ls-files -u A &&
 	test_stdout_line_count = 1 git ls-files -u B &&
 	test_stdout_line_count = 1 git ls-files -u C &&
@@ -163,7 +163,7 @@ test_expect_success 'interference with untracked working tree file' '
 	git reset --hard &&
 	git show-branch &&
 	echo >A this file should not matter &&
-	test_expect_code 1 git pull . white &&
+	test_expect_code 1 git pull --no-rebase . white &&
 	test_path_is_file A
 '
 
@@ -173,7 +173,7 @@ test_expect_success 'interference with untracked working tree file' '
 	git show-branch &&
 	rm -f A &&
 	echo >A this file should not matter &&
-	test_expect_code 1 git pull . red &&
+	test_expect_code 1 git pull --no-rebase . red &&
 	test_path_is_file A
 '
 
@@ -183,7 +183,7 @@ test_expect_success 'interference with untracked working tree file' '
 	git checkout -f main &&
 	git tag -f anchor &&
 	git show-branch &&
-	git pull . yellow &&
+	git pull --no-rebase . yellow &&
 	test_path_is_missing M &&
 	git reset --hard anchor
 '
@@ -210,7 +210,7 @@ test_expect_success 'updated working tree file should prevent the merge' '
 	echo >>M one line addition &&
 	cat M >M.saved &&
 	git update-index M &&
-	test_expect_code 128 git pull . yellow &&
+	test_expect_code 128 git pull --no-rebase . yellow &&
 	test_cmp M M.saved &&
 	rm -f M.saved
 '
@@ -222,7 +222,7 @@ test_expect_success 'interference with untracked working tree file' '
 	git tag -f anchor &&
 	git show-branch &&
 	echo >M this file should not matter &&
-	git pull . main &&
+	git pull --no-rebase . main &&
 	test_path_is_file M &&
 	! {
 		git ls-files -s |
diff --git a/t/t6409-merge-subtree.sh b/t/t6409-merge-subtree.sh
index d406b2343cb..ba7890ec521 100755
--- a/t/t6409-merge-subtree.sh
+++ b/t/t6409-merge-subtree.sh
@@ -100,7 +100,7 @@ test_expect_success 'merge update' '
 	git checkout -b topic_2 &&
 	git commit -m "update git-gui" &&
 	cd ../git &&
-	git pull -s subtree gui topic_2 &&
+	git pull --no-rebase -s subtree gui topic_2 &&
 	git ls-files -s >actual &&
 	(
 		echo "100644 $o3 0	git-gui/git-gui.sh" &&
@@ -129,7 +129,7 @@ test_expect_success 'initial ambiguous subtree' '
 test_expect_success 'merge using explicit' '
 	cd ../git &&
 	git reset --hard topic_2 &&
-	git pull -Xsubtree=git-gui gui topic_2 &&
+	git pull --no-rebase -Xsubtree=git-gui gui topic_2 &&
 	git ls-files -s >actual &&
 	(
 		echo "100644 $o3 0	git-gui/git-gui.sh" &&
@@ -142,7 +142,7 @@ test_expect_success 'merge using explicit' '
 test_expect_success 'merge2 using explicit' '
 	cd ../git &&
 	git reset --hard topic_2 &&
-	git pull -Xsubtree=git-gui2 gui topic_2 &&
+	git pull --no-rebase -Xsubtree=git-gui2 gui topic_2 &&
 	git ls-files -s >actual &&
 	(
 		echo "100644 $o1 0	git-gui/git-gui.sh" &&
diff --git a/t/t6417-merge-ours-theirs.sh b/t/t6417-merge-ours-theirs.sh
index ac9aee9a662..ec065d6a658 100755
--- a/t/t6417-merge-ours-theirs.sh
+++ b/t/t6417-merge-ours-theirs.sh
@@ -69,11 +69,11 @@ test_expect_success 'binary file with -Xours/-Xtheirs' '
 '
 
 test_expect_success 'pull passes -X to underlying merge' '
-	git reset --hard main && git pull -s recursive -Xours . side &&
-	git reset --hard main && git pull -s recursive -X ours . side &&
-	git reset --hard main && git pull -s recursive -Xtheirs . side &&
-	git reset --hard main && git pull -s recursive -X theirs . side &&
-	git reset --hard main && test_must_fail git pull -s recursive -X bork . side
+	git reset --hard main && git pull --no-rebase -s recursive -Xours . side &&
+	git reset --hard main && git pull --no-rebase -s recursive -X ours . side &&
+	git reset --hard main && git pull --no-rebase -s recursive -Xtheirs . side &&
+	git reset --hard main && git pull --no-rebase -s recursive -X theirs . side &&
+	git reset --hard main && test_must_fail git pull --no-rebase -s recursive -X bork . side
 '
 
 test_expect_success SYMLINKS 'symlink with -Xours/-Xtheirs' '
diff --git a/t/t7601-merge-pull-config.sh b/t/t7601-merge-pull-config.sh
index 0c0dfecd2a3..742ed3981c7 100755
--- a/t/t7601-merge-pull-config.sh
+++ b/t/t7601-merge-pull-config.sh
@@ -27,120 +27,120 @@ test_expect_success 'setup' '
 	git tag c3
 '
 
-test_expect_success 'pull.rebase not set' '
+test_expect_success 'pull.rebase not set, ff possible' '
 	git reset --hard c0 &&
 	git pull . c1 2>err &&
-	test_i18ngrep ! "Pulling without specifying how to reconcile" err
+	test_i18ngrep ! "You have divergent branches" err
 '
 
 test_expect_success 'pull.rebase not set and pull.ff=true' '
 	git reset --hard c0 &&
 	test_config pull.ff true &&
 	git pull . c1 2>err &&
-	test_i18ngrep ! "Pulling without specifying how to reconcile" err
+	test_i18ngrep ! "You have divergent branches" err
 '
 
 test_expect_success 'pull.rebase not set and pull.ff=false' '
 	git reset --hard c0 &&
 	test_config pull.ff false &&
 	git pull . c1 2>err &&
-	test_i18ngrep ! "Pulling without specifying how to reconcile" err
+	test_i18ngrep ! "You have divergent branches" err
 '
 
 test_expect_success 'pull.rebase not set and pull.ff=only' '
 	git reset --hard c0 &&
 	test_config pull.ff only &&
 	git pull . c1 2>err &&
-	test_i18ngrep ! "Pulling without specifying how to reconcile" err
+	test_i18ngrep ! "You have divergent branches" err
 '
 
 test_expect_success 'pull.rebase not set and --rebase given' '
 	git reset --hard c0 &&
 	git pull --rebase . c1 2>err &&
-	test_i18ngrep ! "Pulling without specifying how to reconcile" err
+	test_i18ngrep ! "You have divergent branches" err
 '
 
 test_expect_success 'pull.rebase not set and --no-rebase given' '
 	git reset --hard c0 &&
 	git pull --no-rebase . c1 2>err &&
-	test_i18ngrep ! "Pulling without specifying how to reconcile" err
+	test_i18ngrep ! "You have divergent branches" err
 '
 
 test_expect_success 'pull.rebase not set and --ff given' '
 	git reset --hard c0 &&
 	git pull --ff . c1 2>err &&
-	test_i18ngrep ! "Pulling without specifying how to reconcile" err
+	test_i18ngrep ! "You have divergent branches" err
 '
 
 test_expect_success 'pull.rebase not set and --no-ff given' '
 	git reset --hard c0 &&
 	git pull --no-ff . c1 2>err &&
-	test_i18ngrep ! "Pulling without specifying how to reconcile" err
+	test_i18ngrep ! "You have divergent branches" err
 '
 
 test_expect_success 'pull.rebase not set and --ff-only given' '
 	git reset --hard c0 &&
 	git pull --ff-only . c1 2>err &&
-	test_i18ngrep ! "Pulling without specifying how to reconcile" err
+	test_i18ngrep ! "You have divergent branches" err
 '
 
 test_expect_success 'pull.rebase not set (not-fast-forward)' '
 	git reset --hard c2 &&
-	git -c color.advice=always pull . c1 2>err &&
+	test_must_fail git -c color.advice=always pull . c1 2>err &&
 	test_decode_color <err >decoded &&
 	test_i18ngrep "<YELLOW>hint: " decoded &&
-	test_i18ngrep "Pulling without specifying how to reconcile" decoded
+	test_i18ngrep "You have divergent branches" decoded
 '
 
 test_expect_success 'pull.rebase not set and pull.ff=true (not-fast-forward)' '
 	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 ! "You have divergent branches" err
 '
 
 test_expect_success 'pull.rebase not set and pull.ff=false (not-fast-forward)' '
 	git reset --hard c2 &&
 	test_config pull.ff false &&
 	git pull . c1 2>err &&
-	test_i18ngrep ! "Pulling without specifying how to reconcile" err
+	test_i18ngrep ! "You have divergent branches" err
 '
 
 test_expect_success 'pull.rebase not set and pull.ff=only (not-fast-forward)' '
 	git reset --hard c2 &&
 	test_config pull.ff only &&
 	test_must_fail git pull . c1 2>err &&
-	test_i18ngrep ! "Pulling without specifying how to reconcile" err
+	test_i18ngrep ! "You have divergent branches" err
 '
 
 test_expect_success 'pull.rebase not set and --rebase given (not-fast-forward)' '
 	git reset --hard c2 &&
 	git pull --rebase . c1 2>err &&
-	test_i18ngrep ! "Pulling without specifying how to reconcile" err
+	test_i18ngrep ! "You have divergent branches" err
 '
 
 test_expect_success 'pull.rebase not set and --no-rebase given (not-fast-forward)' '
 	git reset --hard c2 &&
 	git pull --no-rebase . c1 2>err &&
-	test_i18ngrep ! "Pulling without specifying how to reconcile" err
+	test_i18ngrep ! "You have divergent branches" err
 '
 
 test_expect_success 'pull.rebase not set and --ff given (not-fast-forward)' '
 	git reset --hard c2 &&
 	git pull --ff . c1 2>err &&
-	test_i18ngrep ! "Pulling without specifying how to reconcile" err
+	test_i18ngrep ! "You have divergent branches" err
 '
 
 test_expect_success 'pull.rebase not set and --no-ff given (not-fast-forward)' '
 	git reset --hard c2 &&
 	git pull --no-ff . c1 2>err &&
-	test_i18ngrep ! "Pulling without specifying how to reconcile" err
+	test_i18ngrep ! "You have divergent branches" err
 '
 
 test_expect_success 'pull.rebase not set and --ff-only given (not-fast-forward)' '
 	git reset --hard c2 &&
 	test_must_fail git pull --ff-only . c1 2>err &&
-	test_i18ngrep ! "Pulling without specifying how to reconcile" err
+	test_i18ngrep ! "You have divergent branches" err
 '
 
 test_does_rebase () {
@@ -325,16 +325,16 @@ test_expect_success 'pull.rebase=false and --ff, ff not possible' '
 
 # End of groupings for conflicting merge vs. rebase flags/options
 
-test_expect_failure 'Multiple heads warns about inability to fast forward' '
+test_expect_success 'Multiple heads warns about inability to fast forward' '
 	git reset --hard c1 &&
 	test_must_fail git pull . c2 c3 2>err &&
-	test_i18ngrep "Pulling without specifying how to reconcile" err
+	test_i18ngrep "You have divergent branches" err
 '
 
 test_expect_failure 'Multiple can never be fast forwarded' '
 	git reset --hard c0 &&
 	test_must_fail git -c pull.ff=only pull . c1 c2 c3 2>err &&
-	test_i18ngrep ! "Pulling without specifying how to reconcile" err &&
+	test_i18ngrep ! "You have divergent branches" err &&
 	# In addition to calling out "cannot fast-forward", we very much
 	# want the "multiple branches" piece to be called out to users.
 	test_i18ngrep "Cannot fast-forward to multiple branches" err
@@ -343,7 +343,7 @@ test_expect_failure 'Multiple can never be fast forwarded' '
 test_expect_success 'Cannot rebase with multiple heads' '
 	git reset --hard c0 &&
 	test_must_fail git -c pull.rebase=true pull . c1 c2 c3 2>err &&
-	test_i18ngrep ! "Pulling without specifying how to reconcile" err &&
+	test_i18ngrep ! "You have divergent branches" err &&
 	test_i18ngrep "Cannot rebase onto multiple branches." err
 '
 
diff --git a/t/t7603-merge-reduce-heads.sh b/t/t7603-merge-reduce-heads.sh
index 98948955ae5..27cd94ad6f7 100755
--- a/t/t7603-merge-reduce-heads.sh
+++ b/t/t7603-merge-reduce-heads.sh
@@ -68,7 +68,7 @@ test_expect_success 'merge c1 with c2, c3, c4, c5' '
 
 test_expect_success 'pull c2, c3, c4, c5 into c1' '
 	git reset --hard c1 &&
-	git pull . c2 c3 c4 c5 &&
+	git pull --no-rebase . c2 c3 c4 c5 &&
 	test "$(git rev-parse c1)" != "$(git rev-parse HEAD)" &&
 	test "$(git rev-parse c1)" = "$(git rev-parse HEAD^1)" &&
 	test "$(git rev-parse c2)" = "$(git rev-parse HEAD^2)" &&
-- 
gitgitgadget

[PATCH v3 7/8] pull: update docs & code for option compatibility with rebasing

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

git-pull.txt includes merge-options.txt, which is written assuming
merges will happen.  git-pull has allowed rebases for many years; update
the documentation to reflect that.

While at it, pass any `--signoff` flag through to the rebase backend too
so that we don't have to document it as merge-specific.  Rebase has
supported the --signoff flag for years now as well.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 Documentation/git-merge.txt     |  2 ++
 Documentation/git-pull.txt      |  9 ++++----
 Documentation/merge-options.txt | 40 +++++++++++++++++++++++++++++++++
 builtin/pull.c                  |  2 ++
 4 files changed, 48 insertions(+), 5 deletions(-)

diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt
index 3819fadac1f..e4f3352eb58 100644
--- a/Documentation/git-merge.txt
+++ b/Documentation/git-merge.txt
@@ -61,6 +61,8 @@ merge has resulted in conflicts.
 
 OPTIONS
 -------
+:git-merge: 1
+
 include::merge-options.txt[]
 
 -m <msg>::
diff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt
index cad3f6bfcee..077961dfa1f 100644
--- a/Documentation/git-pull.txt
+++ b/Documentation/git-pull.txt
@@ -19,13 +19,12 @@ 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`, `--ff`, or `--rebase`
-(or the corresponding configuration options in `pull.ff` or
-`pull.rebase`).
+reconcile the divergent branches with `--rebase` or `--no-rebase` (or
+the corresponding configuration option in `pull.rebase`).
 
 More precisely, `git pull` runs `git fetch` with the given parameters
 and then depending on configuration options or command line flags,
-will call either `git merge` or `git rebase` to reconcile diverging
+will call either `git rebase` or `git merge` to reconcile diverging
 branches.
 
 <repository> should be the name of a remote repository as
@@ -136,7 +135,7 @@ published that history already.  Do *not* use this option
 unless you have read linkgit:git-rebase[1] carefully.
 
 --no-rebase::
-	Override earlier --rebase.
+	This is shorthand for --rebase=false.
 
 Options related to fetching
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~
diff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt
index eb0aabd396f..907327bf759 100644
--- a/Documentation/merge-options.txt
+++ b/Documentation/merge-options.txt
@@ -2,6 +2,9 @@
 --no-commit::
 	Perform the merge and commit the result. This option can
 	be used to override --no-commit.
+ifdef::git-pull[]
+	Only useful when merging.
+endif::git-pull[]
 +
 With --no-commit perform the merge and stop just before creating
 a merge commit, to give the user a chance to inspect and further
@@ -39,6 +42,7 @@ set to `no` at the beginning of them.
 	to `MERGE_MSG` before being passed on to the commit machinery in the
 	case of a merge conflict.
 
+ifdef::git-merge[]
 --ff::
 --no-ff::
 --ff-only::
@@ -47,6 +51,22 @@ set to `no` at the beginning of them.
 	default unless merging an annotated (and possibly signed) tag
 	that is not stored in its natural place in the `refs/tags/`
 	hierarchy, in which case `--no-ff` is assumed.
+endif::git-merge[]
+ifdef::git-pull[]
+--ff-only::
+	Only update to the new history if there is no divergent local
+	history.  This is the default when no method for reconciling
+	divergent histories is provided (via the --rebase=* flags).
+
+--ff::
+--no-ff::
+	When merging rather than rebasing, specifies how a merge is
+	handled when the merged-in history is already a descendant of
+	the current history.  If merging is requested, `--ff` is the
+	default unless merging an annotated (and possibly signed) tag
+	that is not stored in its natural place in the `refs/tags/`
+	hierarchy, in which case `--no-ff` is assumed.
+endif::git-pull[]
 +
 With `--ff`, when possible resolve the merge as a fast-forward (only
 update the branch pointer to match the merged branch; do not create a
@@ -55,9 +75,11 @@ descendant of the current history), create a merge commit.
 +
 With `--no-ff`, create a merge commit in all cases, even when the merge
 could instead be resolved as a fast-forward.
+ifdef::git-merge[]
 +
 With `--ff-only`, resolve the merge as a fast-forward when possible.
 When not possible, refuse to merge and exit with a non-zero status.
+endif::git-merge[]
 
 -S[<keyid>]::
 --gpg-sign[=<keyid>]::
@@ -73,6 +95,9 @@ When not possible, refuse to merge and exit with a non-zero status.
 	In addition to branch names, populate the log message with
 	one-line descriptions from at most <n> actual commits that are being
 	merged. See also linkgit:git-fmt-merge-msg[1].
+ifdef::git-pull[]
+	Only useful when merging.
+endif::git-pull[]
 +
 With --no-log do not list one-line descriptions from the
 actual commits being merged.
@@ -102,10 +127,17 @@ With --no-squash perform the merge and commit the result. This
 option can be used to override --squash.
 +
 With --squash, --commit is not allowed, and will fail.
+ifdef::git-pull[]
++
+Only useful when merging.
+endif::git-pull[]
 
 --no-verify::
 	This option bypasses the pre-merge and commit-msg hooks.
 	See also linkgit:githooks[5].
+ifdef::git-pull[]
+	Only useful when merging.
+endif::git-pull[]
 
 -s <strategy>::
 --strategy=<strategy>::
@@ -127,6 +159,10 @@ With --squash, --commit is not allowed, and will fail.
 	default trust model, this means the signing key has been signed by
 	a trusted key.  If the tip commit of the side branch is not signed
 	with a valid key, the merge is aborted.
+ifdef::git-pull[]
++
+Only useful when merging.
+endif::git-pull[]
 
 --summary::
 --no-summary::
@@ -166,3 +202,7 @@ endif::git-pull[]
 	projects that started their lives independently. As that is
 	a very rare occasion, no configuration variable to enable
 	this by default exists and will not be added.
+ifdef::git-pull[]
++
+Only useful when merging.
+endif::git-pull[]
diff --git a/builtin/pull.c b/builtin/pull.c
index 4514a1478ea..2f1d1f4037d 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -893,6 +893,8 @@ static int run_rebase(const struct object_id *newbase,
 	strvec_pushv(&args, opt_strategy_opts.v);
 	if (opt_gpg_sign)
 		strvec_push(&args, opt_gpg_sign);
+	if (opt_signoff)
+		strvec_push(&args, opt_signoff);
 	if (opt_autostash == 0)
 		strvec_push(&args, "--no-autostash");
 	else if (opt_autostash == 1)
-- 
gitgitgadget

[PATCH v3 8/8] pull: fix handling of multiple heads

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

With multiple heads, we should not allow rebasing or fast-forwarding.
Make sure any fast-forward request calls out specifically the fact that
multiple branches are in play.  Also, since we cannot fast-forward to
multiple branches, fix our computation of can_ff.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 builtin/pull.c               | 18 ++++++++++++++----
 t/t7601-merge-pull-config.sh |  2 +-
 2 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/builtin/pull.c b/builtin/pull.c
index 2f1d1f4037d..b311ea6b9df 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -913,12 +913,18 @@ static int run_rebase(const struct object_id *newbase,
 	return ret;
 }
 
-static int get_can_ff(struct object_id *orig_head, struct object_id *orig_merge_head)
+static int get_can_ff(struct object_id *orig_head,
+		      struct oid_array *merge_heads)
 {
 	int ret;
 	struct commit_list *list = NULL;
 	struct commit *merge_head, *head;
+	struct object_id *orig_merge_head;
 
+	if (merge_heads->nr > 1)
+		return 0;
+
+	orig_merge_head = &merge_heads->oid[0];
 	head = lookup_commit_reference(the_repository, orig_head);
 	commit_list_insert(head, &list);
 	merge_head = lookup_commit_reference(the_repository, orig_merge_head);
@@ -1057,10 +1063,14 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 			die(_("Cannot merge multiple branches into empty head."));
 		return pull_into_void(merge_heads.oid, &curr_head);
 	}
-	if (opt_rebase && merge_heads.nr > 1)
-		die(_("Cannot rebase onto multiple branches."));
+	if (merge_heads.nr > 1) {
+		if (opt_rebase)
+			die(_("Cannot rebase onto multiple branches."));
+		if (opt_ff && !strcmp(opt_ff, "--ff-only"))
+			die(_("Cannot fast-forward to multiple branches."));
+	}
 
-	can_ff = get_can_ff(&orig_head, &merge_heads.oid[0]);
+	can_ff = get_can_ff(&orig_head, &merge_heads);
 
 	/* ff-only takes precedence over rebase */
 	if (opt_ff && !strcmp(opt_ff, "--ff-only")) {
diff --git a/t/t7601-merge-pull-config.sh b/t/t7601-merge-pull-config.sh
index 742ed3981c7..1f652f433ee 100755
--- a/t/t7601-merge-pull-config.sh
+++ b/t/t7601-merge-pull-config.sh
@@ -331,7 +331,7 @@ test_expect_success 'Multiple heads warns about inability to fast forward' '
 	test_i18ngrep "You have divergent branches" err
 '
 
-test_expect_failure 'Multiple can never be fast forwarded' '
+test_expect_success 'Multiple can never be fast forwarded' '
 	git reset --hard c0 &&
 	test_must_fail git -c pull.ff=only pull . c1 c2 c3 2>err &&
 	test_i18ngrep ! "You have divergent branches" err &&
-- 
gitgitgadget

RE: [PATCH v3 0/8] Handle pull option precedence

[Felipe Contreras]

Elijah Newren via GitGitGadget wrote:
> Based on a recent list of rules for flag/option precedence for git-pull[1]
> from Junio (particularly focusing on rebase vs. merge vs. fast-forward),
> here's an attempt to implement and document it. Given multiple recent
> surprises from users about some of these behaviors[2][3] and a coworker just
> yesterday expressing some puzzlement with git-pull and rebase vs. merge, it
> seems like a good time to address some of these issues.
> 
> Since the handling of conflicting options was holding up two of Alex's
> patches[4][5], and his patches fix some of the tests, I also include those
> two patches in my series, with a few small changes to the first (so I've
> kept him as author) and more substantial changes to the second (so I've
> given him an Initial-patch-by attribution).
> 
> Changes since v2:
> 
>  * Remove some unnecessary changes in patch 4, pointed out by Junio.

All my previous objections to v2 still stand:

https://lore.kernel.org/git/60f80feeb4461_1305dc208b8@natae.notmuch/

-- 
Felipe Contreras