Skip to contentSkip to conversation

Handle pull option precedence

PATCH Superseded Elijah Newren via GitGitGadget 5 participants 59 messages Jul 17, 2021 – Jul 22, 2021 Documentation

lore
Share & tools
lore
#0 lore raw

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

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

Signature · 1 line
gitgitgadget
#1 lore raw

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

From: Elijah Newren <[email protected]>

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/[email protected]/
and the links found therein.

---
 t/t7601-merge-pull-config.sh | 154 +++++++++++++++++++++++++++++++++++
 1 file changed, 154 insertions(+)
t/t7601-merge-pull-config.sh +154 -0
index 52e8ccc933a..b24c98cc1b6 100755
@@ -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 &&
Signature · 1 line
gitgitgadget
#2 lore raw

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

Quoted 12 lines · Elijah Newren via GitGitGadget

From: Elijah Newren <[email protected]>

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.

Signature · 1 line
Felipe Contreras
#3 lore raw

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

In reply to Elijah Newren via GitGitGadget
Quoted 1 line · Elijah Newren via GitGitGadget

+test_does_rebase() {

Style: missing SP before ().

Review

Quoted 2 lines
(quoted diff) +2 -0
+	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".

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

Quoted 4 lines
t/t7601-merge-pull-config.sh +4 -0
+	# 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).

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

Quoted 3 lines
t/t7601-merge-pull-config.sh +3 -0
+	test_cmp expect actual &&
+	rm actual expect
+}

OK.

Quoted 28 lines
t/t7601-merge-pull-config.sh +28 -0
+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 &&

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

Quoted 10 lines
t/t7601-merge-pull-config.sh +10 -0
+	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?

Quoted 13 lines
t/t7601-merge-pull-config.sh +13 -0
+#
+# 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' '

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

Quoted 10 lines
t/t7601-merge-pull-config.sh +10 -0
+	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".

Quoted 7 lines
t/t7601-merge-pull-config.sh +7 -0
+# 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' '

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

Quoted 10 lines
t/t7601-merge-pull-config.sh +10 -0
+	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?

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

Quoted 8 lines
t/t7601-merge-pull-config.sh +8 -0
+# 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.

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

Quoted 7 lines
t/t7601-merge-pull-config.sh +7 -0
+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.

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

Quoted 11 lines
t/t7601-merge-pull-config.sh +11 -0
+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.

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

Quoted 5 lines
t/t7601-merge-pull-config.sh +5 -0
+# 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.

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

Quoted 3 lines
t/t7601-merge-pull-config.sh +3 -0
+test_expect_success '--no-rebase works with --ff' '
+	test_does_merge_ff pull --no-rebase --ff
+'

OK.

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

Quoted 3 lines
t/t7601-merge-pull-config.sh +3 -0
+test_expect_success '--no-rebase does ff if pull.ff unset' '
+	test_does_merge_ff pull --no-rebase
+'

OK.

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

Quoted 3 lines
t/t7601-merge-pull-config.sh +3 -0
+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).

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

Quoted 3 lines
t/t7601-merge-pull-config.sh +3 -0
+test_expect_success '--no-rebase heeds pull.ff=false' '
+	test_does_merge_noff -c pull.ff=false pull --no-rebase
+'

OK.

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

Quoted 4 lines
t/t7601-merge-pull-config.sh +4 -0
+# 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.

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

Quoted 3 lines
t/t7601-merge-pull-config.sh +3 -0
+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?

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

Quoted 5 lines
t/t7601-merge-pull-config.sh +2 -0
+# 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.

#4 lore raw

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

Quoted 6 lines · Junio C Hamano
Quoted 1 line · Elijah Newren via GitGitGadget

+test_does_rebase() {

Style: missing SP before ().

Will fix.

...

Quoted 36 lines
Quoted 36 lines
t/t7601-merge-pull-config.sh +36 -0
+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 &&
Quoted 8 lines

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

Quoted 2 lines
t/t7601-merge-pull-config.sh +2 -0
+     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

Quoted 22 lines
Quoted 22 lines
t/t7601-merge-pull-config.sh +22 -0
+#
+# 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
Quoted 8 lines
Quoted 1 line

+'

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.

Quoted 36 lines
Quoted 7 lines
t/t7601-merge-pull-config.sh +7 -0
+# 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' '

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

Quoted 10 lines
t/t7601-merge-pull-config.sh +10 -0
+     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.

Quoted 7 lines

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.

...

Quoted 15 lines

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

Quoted 11 lines
t/t7601-merge-pull-config.sh +11 -0
+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.

...

Quoted 11 lines

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

Quoted 4 lines
t/t7601-merge-pull-config.sh +3 -0
+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

Quoted 3 lines
    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.

Quoted 9 lines

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

Quoted 5 lines
t/t7601-merge-pull-config.sh +2 -0
+# 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/[email protected]/, 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.

#5 lore raw

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

Quoted 11 lines · Elijah Newren
Quoted 6 lines

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.

Quoted 2 lines

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.

#6 lore raw

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

In reply to Junio C Hamano
Quoted 11 lines · Junio C Hamano

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

Quoted 7 lines · Elijah Newren via GitGitGadget
t/t7601-merge-pull-config.sh +7 -0
+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?

Signature · 1 line
Felipe Contreras
#7 lore raw

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

In reply to Elijah Newren via GitGitGadget

From: Elijah Newren <[email protected]>

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.

---
 t/t7601-merge-pull-config.sh | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)
t/t7601-merge-pull-config.sh +20 -0
index b24c98cc1b6..40d8fb95113 100755
@@ -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 &&
Signature · 1 line
gitgitgadget
#8 lore raw

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

Quoted 8 lines · Elijah Newren via GitGitGadget

From: Elijah Newren <[email protected]>

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?

Signature · 1 line
Felipe Contreras
#9 lore raw

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

In reply to Elijah Newren via GitGitGadget

t/t7601-merge-pull-config.sh test_expect_success 'pull.rebase=true takes precedence over --ff' ' · line 304

Quoted 5 lines · Elijah Newren via GitGitGadget
t/t7601-merge-pull-config.sh +5 -0
+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.

t/t7601-merge-pull-config.sh test_expect_success 'pull.rebase=true takes precedence over --ff' ' · line 311

Quoted 6 lines
t/t7601-merge-pull-config.sh +6 -0
+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.

t/t7601-merge-pull-config.sh test_expect_success 'pull.rebase=true takes precedence over --ff' ' · line 318

Quoted 6 lines
t/t7601-merge-pull-config.sh +6 -0
+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.

#10 lore raw

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

Quoted 16 lines · Junio C Hamano

t/t7601-merge-pull-config.sh test_expect_success 'pull.rebase=true takes precedence over --ff' ' · line 304

Quoted 5 lines · Elijah Newren via GitGitGadget
t/t7601-merge-pull-config.sh +5 -0
+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.

Quoted 9 lines

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

Quoted 19 lines

t/t7601-merge-pull-config.sh test_expect_success 'pull.rebase=true takes precedence over --ff' ' · line 311

Quoted 6 lines
t/t7601-merge-pull-config.sh +6 -0
+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.

t/t7601-merge-pull-config.sh test_expect_success 'pull.rebase=true takes precedence over --ff' ' · line 318

Quoted 6 lines
t/t7601-merge-pull-config.sh +6 -0
+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.

#11 lore raw

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

In reply to Elijah Newren via GitGitGadget

From: Alex Henrie <[email protected]>

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.

[en: updated tests; note 3 fixes and 1 new failure]
---
 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(-)
advice.c +5 -0
index 0b9c89c48ab..337e8f342bc 100644
@@ -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;
advice.h +1 -0
index bd26c385d00..16240438387 100644
@@ -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);
 
builtin/merge.c +1 -1
index a8a843b1f54..aa920ac524f 100644
@@ -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,
builtin/pull.c +8 -3
index 3e13f810843..d9796604825 100644
@@ -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) {
t/t7601-merge-pull-config.sh +5 -5
index 40d8fb95113..23a1fbc17b3 100755
@@ -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
 '
 
Signature · 1 line
gitgitgadget
#12 lore raw

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

Quoted 6 lines · Alex Henrie via GitGitGadget

From: Alex Henrie <[email protected]>

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.

Signature · 1 line
Felipe Contreras
#13 lore raw

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

In reply to Elijah Newren via GitGitGadget

From: Elijah Newren <[email protected]>

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.

---
 builtin/pull.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)
builtin/pull.c +9 -8
index d9796604825..5c9cbea37c9 100644
@@ -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;
Signature · 1 line
gitgitgadget
#14 lore raw

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

Quoted 11 lines · Elijah Newren via GitGitGadget

From: Elijah Newren <[email protected]>

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

Signature · 1 line
Felipe Contreras
#15 lore raw

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

In reply to Elijah Newren via GitGitGadget

From: Elijah Newren <[email protected]>

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

---
 builtin/pull.c  | 8 +-------
 t/t5520-pull.sh | 6 +++---
 2 files changed, 4 insertions(+), 10 deletions(-)
builtin/pull.c +1 -7
index 5c9cbea37c9..5ba376a7487 100644
@@ -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))
t/t5520-pull.sh +3 -3
index e2c0c510222..4b50488141f 100755
@@ -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
 '
 
Signature · 1 line
gitgitgadget
#16 lore raw

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

Quoted 10 lines · Elijah Newren via GitGitGadget

From: Elijah Newren <[email protected]>

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

Signature · 1 line
Felipe Contreras
#17 lore raw

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

In reply to Elijah Newren via GitGitGadget
Quoted 5 lines · Elijah Newren via GitGitGadget

From: Elijah Newren <[email protected]>

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.

Quoted 38 lines
builtin/pull.c +1 -7
@@ -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))
t/t5520-pull.sh +3 -3
index e2c0c510222..4b50488141f 100755
@@ -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
 '
#18 lore raw

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

Quoted 5 lines · Junio C Hamano
Quoted 2 lines · Elijah Newren via GitGitGadget

From: Elijah Newren <[email protected]>

Quoted 8 lines
Quoted 3 lines

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

#19 lore raw

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

In reply to Elijah Newren via GitGitGadget

From: Elijah Newren <[email protected]>

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.

---
 builtin/pull.c               | 5 ++++-
 t/t7601-merge-pull-config.sh | 4 ++--
 2 files changed, 6 insertions(+), 3 deletions(-)
builtin/pull.c +4 -1
index 5ba376a7487..7e7c90f6a10 100644
@@ -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);
t/t7601-merge-pull-config.sh +2 -2
index 23a1fbc17b3..12787d07289 100755
@@ -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
 '
 
Signature · 1 line
gitgitgadget
#20 lore raw

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

Quoted 4 lines · Elijah Newren via GitGitGadget

From: Elijah Newren <[email protected]>

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.

Signature · 1 line
Felipe Contreras
#21 lore raw

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

In reply to Elijah Newren via GitGitGadget
Quoted 4 lines · Elijah Newren via GitGitGadget

From: Elijah Newren <[email protected]>

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.

Quoted 11 lines
---
 builtin/pull.c               | 5 ++++-
 t/t7601-merge-pull-config.sh | 4 ++--
 2 files changed, 6 insertions(+), 3 deletions(-)
builtin/pull.c +0 -0
index 5ba376a7487..7e7c90f6a10 100644

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

Quoted 6 lines
builtin/pull.c +1 -1
@@ -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.
*/

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

Quoted 3 lines
builtin/pull.c +3 -0
+		if (opt_rebase >= 0 && opt_ff && !strcmp(opt_ff, "--ff-only"))
+			opt_ff = "--ff";
+	}

Thanks.

#22 lore raw

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

Quoted 7 lines · Junio C Hamano
Quoted 4 lines · Elijah Newren via GitGitGadget

From: Elijah Newren <[email protected]>

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.

Quoted 6 lines

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?

Signature · 1 line
Felipe Contreras
#23 lore raw

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

In reply to Elijah Newren via GitGitGadget

From: Elijah Newren <[email protected]>

We have for some time shown a long warning when the user does not specify how to reconcile divergent branches with git pull. Make it an error now.

---
 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(-)
Documentation/git-pull.txt +12 -8
index 5c3fb67c014..cad3f6bfcee 100644
@@ -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
builtin/pull.c +6 -4
index 7e7c90f6a10..2d7f2d765ab 100644
@@ -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;
t/t4013-diff-various.sh +1 -1
index 7fadc985ccc..eb989f7f191 100755
@@ -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" &&
t/t5520-pull.sh +10 -10
index 4b50488141f..56f5864b269 100755
@@ -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' '
t/t5521-pull-options.sh +2 -2
index 63a688bdbf5..7601c919fdc 100755
@@ -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
 	)
 '
 
t/t5524-pull-msg.sh +2 -2
index c278adaa5a2..b2be3605f5a 100755
@@ -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 &&
t/t5553-set-upstream.sh +7 -7
index b1d614ce18c..9c12c0f8c32 100755
@@ -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
 '
t/t5604-clone-reference.sh +2 -2
index e845d621f61..24340e6d56e 100755
@@ -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
t/t6402-merge-rename.sh +9 -9
index 425dad97d54..02a842697b8 100755
@@ -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 |
t/t6409-merge-subtree.sh +3 -3
index d406b2343cb..ba7890ec521 100755
@@ -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" &&
t/t6417-merge-ours-theirs.sh +5 -5
index ac9aee9a662..ec065d6a658 100755
@@ -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' '
t/t7601-merge-pull-config.sh +20 -20
index 12787d07289..673b92afbab 100755
@@ -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() {
t/t7603-merge-reduce-heads.sh +1 -1
index 98948955ae5..27cd94ad6f7 100755
@@ -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)" &&
Signature · 1 line
gitgitgadget
#24 lore raw

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

Quoted 5 lines · Elijah Newren via GitGitGadget

From: Elijah Newren <[email protected]>

We have for some time shown a long warning when the user does not specify how to reconcile divergent branches with git pull. Make it an error now.

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.

Signature · 1 line
Felipe Contreras
#25 lore raw

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

In reply to Elijah Newren via GitGitGadget

From: Elijah Newren <[email protected]>

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.

---
 Documentation/git-pull.txt      |  2 ++
 Documentation/merge-options.txt | 25 +++++++++++++++++++++++++
 builtin/pull.c                  |  2 ++
 3 files changed, 29 insertions(+)
Documentation/git-pull.txt +2 -0
index cad3f6bfcee..6e6e95a7595 100644
@@ -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.
Documentation/merge-options.txt +25 -0
index eb0aabd396f..3b70abf3c87 100644
@@ -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[]
builtin/pull.c +2 -0
index 2d7f2d765ab..3a61b92f328 100644
@@ -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)
Signature · 1 line
gitgitgadget
#26 lore raw

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

Quoted 5 lines · Elijah Newren via GitGitGadget

From: Elijah Newren <[email protected]>

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.

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

Quoted 10 lines
Documentation/git-pull.txt +2 -0
index cad3f6bfcee..6e6e95a7595 100644
@@ -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"?

Documentation/git-pull.txt could instead be resolved as a fast-forward. · line 63

Quoted 4 lines
Documentation/git-pull.txt +0 -0
@@ -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"?

Documentation/git-pull.txt could instead be resolved as a fast-forward. · line 67

Quoted 4 lines
Documentation/git-pull.txt +4 -0
+ifdef::git-pull[]
++
+Note that `--no-ff` and `--ff` are ignored when rebasing is requested.
+endif::git-pull[]

OK.

#27 lore raw

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

Quoted 17 lines · Junio C Hamano
Quoted 5 lines · Elijah Newren via GitGitGadget

From: Elijah Newren <[email protected]>

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.

Quoted 12 lines

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

Quoted 10 lines
Documentation/git-pull.txt +2 -0
index cad3f6bfcee..6e6e95a7595 100644
@@ -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."

Quoted 7 lines

Documentation/git-pull.txt could instead be resolved as a fast-forward. · line 63

Quoted 4 lines
Documentation/git-pull.txt +0 -0
@@ -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.

Quoted 6 lines

Documentation/git-pull.txt could instead be resolved as a fast-forward. · line 67

Quoted 4 lines
Documentation/git-pull.txt +4 -0
+ifdef::git-pull[]
++
+Note that `--no-ff` and `--ff` are ignored when rebasing is requested.
+endif::git-pull[]

OK.

#28 lore raw

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

Quoted 10 lines · Elijah Newren
Quoted 4 lines
Quoted 2 lines

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

#29 lore raw

PATCH 9/9 pull: fix handling of multiple heads

In reply to Elijah Newren via GitGitGadget

From: Elijah Newren <[email protected]>

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

---
 builtin/pull.c               | 20 ++++++++++++++++----
 t/t7601-merge-pull-config.sh |  4 ++--
 2 files changed, 18 insertions(+), 6 deletions(-)
builtin/pull.c +16 -4
index 3a61b92f328..beaf6ee0653 100644
@@ -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")) {
t/t7601-merge-pull-config.sh +2 -2
index 673b92afbab..29105b5b1ed 100755
@@ -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' '
Signature · 1 line
gitgitgadget
#30 lore raw
In reply to Elijah Newren via GitGitGadget
Quoted 7 lines · 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.

Quoted 5 lines

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)

Signature · 1 line
Felipe Contreras
#31 lore raw

v2 cover Handle pull option precedence

In reply to 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/[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                  |  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 <[email protected]>
      
       ## 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/[email protected]/
     @@ 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

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

## 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/[email protected]/, --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.)

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

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

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

Signature · 1 line
gitgitgadget
#32 lore raw

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

From: Elijah Newren <[email protected]>

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/[email protected]/
and the links found therein.

---
 t/t7601-merge-pull-config.sh | 182 +++++++++++++++++++++++++++++++++++
 1 file changed, 182 insertions(+)
t/t7601-merge-pull-config.sh +182 -0
index 52e8ccc933a..a44e6e69f2b 100755
@@ -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 &&
Signature · 1 line
gitgitgadget
#33 lore raw

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

Quoted 9 lines · Elijah Newren via GitGitGadget

From: Elijah Newren <[email protected]>

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.

Quoted 3 lines

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

Quoted 9 lines
t/t7601-merge-pull-config.sh +5 -0
@@ -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' '

t/t7601-merge-pull-config.sh line 158

Quoted 10 lines
t/t7601-merge-pull-config.sh +10 -0
+	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?

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

Quoted 7 lines
t/t7601-merge-pull-config.sh +7 -0
+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.

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

Quoted 3 lines
t/t7601-merge-pull-config.sh +3 -0
+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.

Signature · 1 line
Felipe Contreras
#34 lore raw

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

In reply to Elijah Newren via GitGitGadget

From: Elijah Newren <[email protected]>

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.

---
 t/t7601-merge-pull-config.sh | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)
t/t7601-merge-pull-config.sh +22 -0
index a44e6e69f2b..21db1e9e14b 100755
@@ -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 &&
Signature · 1 line
gitgitgadget
#35 lore raw

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

In reply to Elijah Newren via GitGitGadget

From: Alex Henrie <[email protected]>

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.

[en: updated tests; note 3 fixes and 1 new failure]
---
 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(-)
advice.c +5 -0
index 0b9c89c48ab..337e8f342bc 100644
@@ -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;
advice.h +1 -0
index bd26c385d00..16240438387 100644
@@ -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);
 
builtin/merge.c +1 -1
index a8a843b1f54..aa920ac524f 100644
@@ -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,
builtin/pull.c +8 -3
index 3e13f810843..d9796604825 100644
@@ -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) {
t/t7601-merge-pull-config.sh +5 -5
index 21db1e9e14b..d1f621725ad 100755
@@ -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
 '
 
Signature · 1 line
gitgitgadget
#36 lore raw

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

Quoted 6 lines · Alex Henrie via GitGitGadget

From: Alex Henrie <[email protected]>

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.

Signature · 1 line
Felipe Contreras
#37 lore raw

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

In reply to Elijah Newren via GitGitGadget

From: Elijah Newren <[email protected]>

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

---
 builtin/pull.c  | 24 +++++++++---------------
 t/t5520-pull.sh |  6 +++---
 2 files changed, 12 insertions(+), 18 deletions(-)
builtin/pull.c +9 -15
index d9796604825..92150f976cd 100644
@@ -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))
t/t5520-pull.sh +3 -3
index e2c0c510222..4b50488141f 100755
@@ -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
 '
 
Signature · 1 line
gitgitgadget
#38 lore raw

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

Quoted 6 lines · Elijah Newren via GitGitGadget

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.

Signature · 8 lines
aixigo AG
Karl-Friedrich-Str. 68, 52072 Aachen, Germany
phone: +49 (0)241 559709-390, fax: +49 (0)241 559709-99
email: [email protected]
web: https://www.aixigo.com
District Court Aachen – HRB 8057
Board: Christian Friedrich, Tobias Haustein
Chairman of the Supervisory Board: Dr. Roland Schlager
#39 lore raw

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

Quoted 9 lines · Matthias Baumgarten
Quoted 6 lines · Elijah Newren via GitGitGadget

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
Signature · 1 line
Felipe Contreras
#40 lore raw

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

In reply to Elijah Newren via GitGitGadget
Quoted 7 lines · Elijah Newren via GitGitGadget
builtin/pull.c +0 -7
-	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();

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

Quoted 10 lines
builtin/pull.c +8 -1
-		}
+	/* 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 ...

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

Quoted 12 lines
builtin/pull.c +1 -7
@@ -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()).

#41 lore raw

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

Quoted 39 lines · Junio C Hamano
Quoted 7 lines · Elijah Newren via GitGitGadget
builtin/pull.c +0 -7
-     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();

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

Quoted 10 lines
builtin/pull.c +8 -1
-             }
+     /* 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 ...

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

Quoted 12 lines
builtin/pull.c +1 -7
@@ -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.

Quoted 7 lines

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.

#42 lore raw

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

In reply to Elijah Newren via GitGitGadget

From: Elijah Newren <[email protected]>

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.

---
 builtin/pull.c               | 16 +++++++++++++++-
 t/t7601-merge-pull-config.sh |  4 ++--
 2 files changed, 17 insertions(+), 3 deletions(-)
builtin/pull.c +15 -1
index 92150f976cd..da1d4b19666 100644
@@ -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);
t/t7601-merge-pull-config.sh +2 -2
index d1f621725ad..0c0dfecd2a3 100755
@@ -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
 '
 
Signature · 1 line
gitgitgadget
#43 lore raw

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

Quoted 4 lines · Elijah Newren via GitGitGadget

From: Elijah Newren <[email protected]>

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.

Signature · 1 line
Felipe Contreras
#44 lore raw

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

In reply to Elijah Newren via GitGitGadget

From: Elijah Newren <[email protected]>

We have for some time shown a long warning when the user does not specify how to reconcile divergent branches with git pull. Make it an error now.

---
 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(-)
Documentation/git-pull.txt +12 -8
index 5c3fb67c014..cad3f6bfcee 100644
@@ -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
builtin/pull.c +6 -4
index da1d4b19666..94464ca196f 100644
@@ -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;
t/t4013-diff-various.sh +1 -1
index 7fadc985ccc..eb989f7f191 100755
@@ -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" &&
t/t5520-pull.sh +10 -10
index 4b50488141f..56f5864b269 100755
@@ -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' '
t/t5521-pull-options.sh +2 -2
index 63a688bdbf5..7601c919fdc 100755
@@ -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
 	)
 '
 
t/t5524-pull-msg.sh +2 -2
index c278adaa5a2..b2be3605f5a 100755
@@ -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 &&
t/t5553-set-upstream.sh +7 -7
index b1d614ce18c..9c12c0f8c32 100755
@@ -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
 '
t/t5604-clone-reference.sh +2 -2
index e845d621f61..24340e6d56e 100755
@@ -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
t/t6402-merge-rename.sh +9 -9
index 3da2896e3bc..3a32b1a45cf 100755
@@ -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 |
t/t6409-merge-subtree.sh +3 -3
index d406b2343cb..ba7890ec521 100755
@@ -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" &&
t/t6417-merge-ours-theirs.sh +5 -5
index ac9aee9a662..ec065d6a658 100755
@@ -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' '
t/t7601-merge-pull-config.sh +24 -24

167 lines not on this page · raw message

t/t7603-merge-reduce-heads.sh +1 -1
index 98948955ae5..27cd94ad6f7 100755
@@ -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)" &&
Signature · 1 line
gitgitgadget
#45 lore raw

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

Quoted 5 lines · Elijah Newren via GitGitGadget

From: Elijah Newren <[email protected]>

We have for some time shown a long warning when the user does not specify how to reconcile divergent branches with git pull. Make it an error now.

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

Signature · 1 line
Felipe Contreras
#46 lore raw

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

In reply to Elijah Newren via GitGitGadget

From: Elijah Newren <[email protected]>

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.

---
 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(-)
Documentation/git-merge.txt +2 -0
index 3819fadac1f..e4f3352eb58 100644
@@ -61,6 +61,8 @@ merge has resulted in conflicts.
 
 OPTIONS
 -------
+:git-merge: 1
+
 include::merge-options.txt[]
 
 -m <msg>::
Documentation/git-pull.txt +4 -5
index cad3f6bfcee..077961dfa1f 100644
@@ -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
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~
Documentation/merge-options.txt +40 -0
index eb0aabd396f..907327bf759 100644
@@ -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[]
builtin/pull.c +2 -0
index 94464ca196f..a15d4154093 100644
@@ -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)
Signature · 1 line
gitgitgadget
#47 lore raw

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

In reply to Elijah Newren via GitGitGadget

From: Elijah Newren <[email protected]>

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.

---
 builtin/pull.c               | 18 ++++++++++++++----
 t/t7601-merge-pull-config.sh |  2 +-
 2 files changed, 15 insertions(+), 5 deletions(-)
builtin/pull.c +14 -4
index a15d4154093..d6631d23d17 100644
@@ -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")) {
t/t7601-merge-pull-config.sh +1 -1
index 742ed3981c7..1f652f433ee 100755
@@ -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 &&
Signature · 1 line
gitgitgadget
#48 lore raw
In reply to Elijah Newren via GitGitGadget

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

Quoted 4 lines · 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]

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

Quoted 1 line

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

Quoted 1 line

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

This breaks backwards compatibility.

Users will complain.

Quoted 1 line

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

Yet another backwards incompatible change.

Quoted 1 line

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

More brekage without a single day of deprecation warning.

Quoted 2 lines

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

Signature · 1 line
Felipe Contreras
#49 lore raw

v3 cover Handle pull option precedence

In reply to 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/[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
Signature · 1 line
gitgitgadget
#50 lore raw

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

From: Elijah Newren <[email protected]>

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/[email protected]/
and the links found therein.

---
 t/t7601-merge-pull-config.sh | 182 +++++++++++++++++++++++++++++++++++
 1 file changed, 182 insertions(+)
t/t7601-merge-pull-config.sh +182 -0
index 52e8ccc933a..a44e6e69f2b 100755
@@ -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 &&
Signature · 1 line
gitgitgadget
#51 lore raw

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

In reply to Elijah Newren via GitGitGadget

From: Elijah Newren <[email protected]>

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.

---
 t/t7601-merge-pull-config.sh | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)
t/t7601-merge-pull-config.sh +22 -0
index a44e6e69f2b..21db1e9e14b 100755
@@ -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 &&
Signature · 1 line
gitgitgadget
#52 lore raw

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

In reply to Elijah Newren via GitGitGadget

From: Alex Henrie <[email protected]>

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.

[en: updated tests; note 3 fixes and 1 new failure]
---
 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(-)
advice.c +5 -0
index 0b9c89c48ab..337e8f342bc 100644
@@ -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;
advice.h +1 -0
index bd26c385d00..16240438387 100644
@@ -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);
 
builtin/merge.c +1 -1
index a8a843b1f54..aa920ac524f 100644
@@ -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,
builtin/pull.c +8 -3
index 3e13f810843..d9796604825 100644
@@ -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) {
t/t7601-merge-pull-config.sh +5 -5
index 21db1e9e14b..d1f621725ad 100755
@@ -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
 '
 
Signature · 1 line
gitgitgadget
#53 lore raw

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

In reply to Elijah Newren via GitGitGadget

From: Elijah Newren <[email protected]>

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

---
 builtin/pull.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)
builtin/pull.c +8 -8
index d9796604825..1f452020375 100644
@@ -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;
Signature · 1 line
gitgitgadget
#54 lore raw

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

In reply to Elijah Newren via GitGitGadget

From: Elijah Newren <[email protected]>

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.

---
 builtin/pull.c               | 16 +++++++++++++++-
 t/t7601-merge-pull-config.sh |  4 ++--
 2 files changed, 17 insertions(+), 3 deletions(-)
builtin/pull.c +15 -1
index 1f452020375..9bf03255291 100644
@@ -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);
t/t7601-merge-pull-config.sh +2 -2
index d1f621725ad..0c0dfecd2a3 100755
@@ -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
 '
 
Signature · 1 line
gitgitgadget
#55 lore raw

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

In reply to Elijah Newren via GitGitGadget

From: Elijah Newren <[email protected]>

We have for some time shown a long warning when the user does not specify how to reconcile divergent branches with git pull. Make it an error now.

---
 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(-)
Documentation/git-pull.txt +12 -8
index 5c3fb67c014..cad3f6bfcee 100644
@@ -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
builtin/pull.c +6 -4
index 9bf03255291..4514a1478ea 100644
@@ -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;
t/t4013-diff-various.sh +1 -1
index 7fadc985ccc..eb989f7f191 100755
@@ -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" &&
t/t5520-pull.sh +10 -10
index e2c0c510222..672001a18bd 100755
@@ -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' '
t/t5521-pull-options.sh +2 -2
index 63a688bdbf5..7601c919fdc 100755
@@ -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
 	)
 '
 
t/t5524-pull-msg.sh +2 -2
index c278adaa5a2..b2be3605f5a 100755
@@ -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 &&
t/t5553-set-upstream.sh +7 -7
index b1d614ce18c..9c12c0f8c32 100755
@@ -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
 '
t/t5604-clone-reference.sh +2 -2
index e845d621f61..24340e6d56e 100755
@@ -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
t/t6402-merge-rename.sh +9 -9
index 3da2896e3bc..3a32b1a45cf 100755
@@ -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 |
t/t6409-merge-subtree.sh +3 -3
index d406b2343cb..ba7890ec521 100755
@@ -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" &&
t/t6417-merge-ours-theirs.sh +5 -5
index ac9aee9a662..ec065d6a658 100755
@@ -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' '
t/t7601-merge-pull-config.sh +24 -24

167 lines not on this page · raw message

t/t7603-merge-reduce-heads.sh +1 -1
index 98948955ae5..27cd94ad6f7 100755
@@ -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)" &&
Signature · 1 line
gitgitgadget
#56 lore raw

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

In reply to Elijah Newren via GitGitGadget

From: Elijah Newren <[email protected]>

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.

---
 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(-)
Documentation/git-merge.txt +2 -0
index 3819fadac1f..e4f3352eb58 100644
@@ -61,6 +61,8 @@ merge has resulted in conflicts.
 
 OPTIONS
 -------
+:git-merge: 1
+
 include::merge-options.txt[]
 
 -m <msg>::
Documentation/git-pull.txt +4 -5
index cad3f6bfcee..077961dfa1f 100644
@@ -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
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~
Documentation/merge-options.txt +40 -0
index eb0aabd396f..907327bf759 100644
@@ -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[]
builtin/pull.c +2 -0
index 4514a1478ea..2f1d1f4037d 100644
@@ -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)
Signature · 1 line
gitgitgadget
#57 lore raw

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

In reply to Elijah Newren via GitGitGadget

From: Elijah Newren <[email protected]>

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.

---
 builtin/pull.c               | 18 ++++++++++++++----
 t/t7601-merge-pull-config.sh |  2 +-
 2 files changed, 15 insertions(+), 5 deletions(-)
builtin/pull.c +14 -4
index 2f1d1f4037d..b311ea6b9df 100644
@@ -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")) {
t/t7601-merge-pull-config.sh +1 -1
index 742ed3981c7..1f652f433ee 100755
@@ -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 &&
Signature · 1 line
gitgitgadget
#58 lore raw
In reply to Elijah Newren via GitGitGadget
Quoted 13 lines · 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).

Quoted 3 lines

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

Signature · 1 line
Felipe Contreras