Handle pull option precedence

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/[email protected]/ [2]
https://lore.kernel.org/git/CAL3xRKdOyVWvcLXK7zoXtFPiHBjgL24zi5hhg+3yjowwSUPgmg@mail.gmail.com/
[3]
https://lore.kernel.org/git/[email protected]/
[4]
https://lore.kernel.org/git/[email protected]/
[5]
https://lore.kernel.org/git/[email protected]/

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