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

Commit Message

Elijah Newren July 17, 2021, 3:41 p.m. UTC

From: Elijah Newren <[e-mail]>

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

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

Signed-off-by: Elijah Newren <[e-mail]>
---
 t/t7601-merge-pull-config.sh | 154 +++++++++++++++++++++++++++++++++++
 1 file changed, 154 insertions(+)

Comments

Felipe Contreras July 17, 2021, 6:03 p.m. UTC | #1

Elijah Newren via GitGitGadget wrote:
> From: Elijah Newren <[e-mail]>
> 
> 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.

Junio C Hamano July 19, 2021, 6:23 p.m. UTC | #2

"Elijah Newren via GitGitGadget" <[e-mail]> writes:

> +test_does_rebase() {

Style: missing SP before ().

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

OK.

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

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

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

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

OK.

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

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

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

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

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

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

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

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

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

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

Both are good.

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

OK.

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

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

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

OK.

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

OK.

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

OK.

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

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

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

OK.

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

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


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

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

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

would be true, right?

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

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

Thanks.

Elijah Newren July 20, 2021, 5:10 p.m. UTC | #3

On Mon, Jul 19, 2021 at 11:23 AM Junio C Hamano <[e-mail]> wrote:
>
> "Elijah Newren via GitGitGadget" <[e-mail]> writes:
>
> > +test_does_rebase() {
>
> Style: missing SP before ().

Will fix.

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

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

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

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

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

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


In particular, `test_does_need_full_merge` would become
`test_falls_back_to_full_merge`


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

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

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

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

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

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

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

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

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

Junio C Hamano July 20, 2021, 6:22 p.m. UTC | #4

Elijah Newren <[e-mail]> writes:

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

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

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

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

Thanks.

Felipe Contreras July 20, 2021, 8:29 p.m. UTC | #5

Junio C Hamano wrote:
> "Elijah Newren via GitGitGadget" <[e-mail]> writes:

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

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

Patch

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