Handle pull option precedence
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
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
@@ -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
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
Re: [PATCH 1/9] t7601: add relative precedence tests for merge and rebase flags/options
In reply to Elijah Newren via GitGitGadgetQuoted 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.
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_Xhelpers make sense. I am not sure about the namedoes_need_full_mergethough---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_rebaseonly 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_ffpull --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_rebasepresents us, so rebasing would be quite sensible. Similarly
Quoted 3 lines
test_does_need_full_merge -c pull.rebase=false pull --ffwould 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.
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.
Re: [PATCH 1/9] t7601: add relative precedence tests for merge and rebase flags/options
In reply to Junio C HamanoQuoted 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
PATCH 2/9 t7601: add tests of interactions with multiple merge heads and config
In reply to Elijah Newren via GitGitGadgetFrom: 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
@@ -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
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
Re: [PATCH 2/9] t7601: add tests of interactions with multiple merge heads and config
In reply to Elijah Newren via GitGitGadgett/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.
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.
PATCH 3/9 pull: abort if --ff-only is given and fast-forwarding is impossible
In reply to Elijah Newren via GitGitGadgetFrom: 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.
--- 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
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
PATCH 4/9 pull: since --ff-only overrides, handle it first
In reply to Elijah Newren via GitGitGadgetFrom: 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
@@ -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
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
PATCH 5/9 pull: ensure --rebase overrides ability to ff
In reply to Elijah Newren via GitGitGadgetFrom: 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
@@ -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
@@ -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
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
Re: [PATCH 5/9] pull: ensure --rebase overrides ability to ff
In reply to Elijah Newren via GitGitGadgetQuoted 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@@ -295,7 +295,7 @@ test_expect_success '--rebase (merge) fast forward' ' # The above only validates the result. Did we actually bypass rebase? git reflog -1 >reflog.actual && sed "s/^[0-9a-f][0-9a-f]*/OBJID/" reflog.actual >reflog.fuzzy && - echo "OBJID HEAD@{0}: pull --rebase . ff: Fast-forward" >reflog.expected && + echo "OBJID HEAD@{0}: pull --rebase . ff (finish): returning to refs/heads/to-rebase" >reflog.expected && test_cmp reflog.expected reflog.fuzzy '@@ -307,8 +307,8 @@ test_expect_success '--rebase (am) fast forward' ' # The above only validates the result. Did we actually bypass rebase? git reflog -1 >reflog.actual && - sed "s/^[0-9a-f][0-9a-f]*/OBJID/" reflog.actual >reflog.fuzzy && - echo "OBJID HEAD@{0}: pull --rebase . ff: Fast-forward" >reflog.expected && + sed -e "s/^[0-9a-f][0-9a-f]*/OBJID/" -e "s/[0-9a-f][0-9a-f]*$/OBJID/" reflog.actual >reflog.fuzzy && + echo "OBJID HEAD@{0}: rebase finished: refs/heads/to-rebase onto OBJID" >reflog.expected && test_cmp reflog.expected reflog.fuzzy '
Re: [PATCH 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 whencan_ffis 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.)
PATCH 6/9 pull: make --rebase and --no-rebase override pull.ff=only
In reply to Elijah Newren via GitGitGadgetFrom: 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
@@ -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
@@ -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
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
Re: [PATCH 6/9] pull: make --rebase and --no-rebase override pull.ff=only
In reply to Elijah Newren via GitGitGadgetQuoted 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
Signed-off-by Elijah Newren <[email protected]>--- builtin/pull.c | 5 ++++- t/t7601-merge-pull-config.sh | 4 ++-- 2 files changed, 6 insertions(+), 3 deletions(-)builtin/pull.c +0
-0
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.
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_rebaseandopt_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
PATCH 7/9 pull: abort by default when fast-forwarding is not possible
In reply to Elijah Newren via GitGitGadgetFrom: 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
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
PATCH 8/9 pull: update docs & code for option compatibility with rebasing
In reply to Elijah Newren via GitGitGadgetFrom: 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
@@ -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
@@ -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
@@ -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
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@@ -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.
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@@ -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-onlyoverrides any--rebaseflag"?
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.
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-onlytakes precedence over any--rebaseflag. ++"
ff-onlyoverrides any--rebaseflag"?Yeah, ignore that, I had already ripped it out after your comments on patch 1 yesterday. I've also got more changes to the pull documentation to try to do more of what your sentence above says, "covering these interactions and whipping them into a much better shape."
OK. After thinking about it further, "overrides" is not the word, either, I guess. It is more like that there are "merge", "rebase" and "fast-forward-only" for the user to choose from, with "merge" having two sub-variants (i.e. what happens when we do not have any development of our own) and "rebase" with a few sub-variants (i.e. interactive? shape-preserving?).
In any case, when the user says "I'll take nothing but fast-forward update to their history", it is a bug if we did not fail when their history is not a descendant of ours, and we have already fixed it in an earlier part of the series. This would be a good place to clarify what the correct behaviour ought to be, if the existing documentation is not already.
Thanks.
PATCH 9/9 pull: fix handling of multiple heads
In reply to Elijah Newren via GitGitGadgetFrom: 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
@@ -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
@@ -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
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
v2 cover Handle pull option precedence
In reply to Elijah Newren via GitGitGadgetBased 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
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
@@ -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
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
PATCH v2 2/8 t7601: add tests of interactions with multiple merge heads and config
In reply to Elijah Newren via GitGitGadgetFrom: 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
@@ -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
PATCH v2 3/8 pull: abort if --ff-only is given and fast-forwarding is impossible
In reply to Elijah Newren via GitGitGadgetFrom: 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.
--- 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
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
PATCH v2 4/8 pull: since --ff-only overrides, handle it first
In reply to Elijah Newren via GitGitGadgetFrom: 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
@@ -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
@@ -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
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
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
Re: [PATCH v2 4/8] pull: since --ff-only overrides, handle it first
In reply to Elijah Newren via GitGitGadgetQuoted 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()).
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 letrun_rebase()to first "checkout" their tip, which ends up to be a fast-forward in the "can_ff" situation. As a side effect,opt_ffgets contaminated with the current code, but that would not affect what happens after this part (i.e. call torebase_submodules()).
Indeed, you are right. Sorry about that, I'll re-roll with this hunk removed, and the modifications to t5520 pulled out as well.
PATCH v2 5/8 pull: make --rebase and --no-rebase override pull.ff=only
In reply to Elijah Newren via GitGitGadgetFrom: 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
@@ -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
@@ -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
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
PATCH v2 6/8 pull: abort by default when fast-forwarding is not possible
In reply to Elijah Newren via GitGitGadgetFrom: 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
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
PATCH v2 7/8 pull: update docs & code for option compatibility with rebasing
In reply to Elijah Newren via GitGitGadgetFrom: 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
@@ -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
@@ -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
@@ -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
@@ -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
PATCH v2 8/8 pull: fix handling of multiple heads
In reply to Elijah Newren via GitGitGadgetFrom: 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
@@ -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
@@ -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
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
v3 cover Handle pull option precedence
In reply to Elijah Newren via GitGitGadgetBased 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 headsSignature · 1 line
gitgitgadget
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
@@ -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
PATCH v3 2/8 t7601: add tests of interactions with multiple merge heads and config
In reply to Elijah Newren via GitGitGadgetFrom: 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
@@ -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
PATCH v3 3/8 pull: abort if --ff-only is given and fast-forwarding is impossible
In reply to Elijah Newren via GitGitGadgetFrom: 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.
--- 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
PATCH v3 4/8 pull: since --ff-only overrides, handle it first
In reply to Elijah Newren via GitGitGadgetFrom: 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
@@ -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
PATCH v3 5/8 pull: make --rebase and --no-rebase override pull.ff=only
In reply to Elijah Newren via GitGitGadgetFrom: 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
@@ -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
@@ -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
PATCH v3 6/8 pull: abort by default when fast-forwarding is not possible
In reply to Elijah Newren via GitGitGadgetFrom: 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
PATCH v3 7/8 pull: update docs & code for option compatibility with rebasing
In reply to Elijah Newren via GitGitGadgetFrom: 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
@@ -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
@@ -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
@@ -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
@@ -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
PATCH v3 8/8 pull: fix handling of multiple heads
In reply to Elijah Newren via GitGitGadgetFrom: 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
@@ -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
@@ -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
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