rebase: fast-forward --onto in more cases

Before, when we had the following graph,

A---B---C (master)
\
D (side)

running 'git rebase --onto master... master side' would result in D
being always rebased, no matter what. However, the desired behavior is
that rebase should notice that this is fast-forwardable and do that
instead.

Add detection to `can_fast_forward` so that this case can be detected
and a fast-forward will be performed. First of all, rewrite the function
to use gotos which simplifies the logic. Next, since the

options.upstream &&
!oidcmp(&options.upstream->object.oid, &options.onto->object.oid)

conditions were removed in `cmd_rebase`, we reintroduce a substitute in
`can_fast_forward`. In particular, checking the merge bases of
`upstream` and `head` fixes a failing case in t3416.

The abbreviated graph for t3416 is as follows:

F---G topic
/
A---B---C---D---E master

and the failing command was

git rebase --onto master...topic F topic

Before, Git would see that there was one merge base (C), and the merge
and onto were the same so it would incorrectly return 1, indicating that
we could fast-forward. This would cause the rebased graph to be 'ABCFG'
when we were expecting 'ABCG'.

With the additional logic, we detect that upstream and head's merge base
is F. Since onto isn't F, it means we're not rebasing the full set of
commits from master..topic. Since we're excluding some commits, a
fast-forward cannot be performed and so we correctly return 0.

Add '-f' to test cases that failed as a result of this change because
they were not expecting a fast-forward so that a rebase is forced.

Helped-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Signed-off-by: Denton Liu <liu.denton@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/builtin/rebase.c b/builtin/rebase.c
index 1ddad4612640e09dda9b81174974802eaf8ee71d..1e1406c8ba59e6ff90392480de2b67fc67635049 100644 (file)
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1260,8 +1260,8 @@ static int is_linear_history(struct commit *from, struct commit *to)
        return 1;
 }
 
-static int can_fast_forward(struct commit *onto, struct object_id *head_oid,
-                           struct object_id *merge_base)
+static int can_fast_forward(struct commit *onto, struct commit *upstream,
+                           struct object_id *head_oid, struct object_id *merge_base)
 {
        struct commit *head = lookup_commit(the_repository, head_oid);
        struct commit_list *merge_bases = NULL;
@@ -1280,6 +1280,17 @@ static int can_fast_forward(struct commit *onto, struct object_id *head_oid,
        if (!oideq(merge_base, &onto->object.oid))
                goto done;
 
+       if (!upstream)
+               goto done;
+
+       free_commit_list(merge_bases);
+       merge_bases = get_merge_bases(upstream, head);
+       if (!merge_bases || merge_bases->next)
+               goto done;
+
+       if (!oideq(&onto->object.oid, &merge_bases->item->object.oid))
+               goto done;
+
        res = 1;
 
 done:
@@ -2027,13 +2038,13 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 
        /*
         * Check if we are already based on onto with linear history,
-        * but this should be done only when upstream and onto are the same
-        * and if this is not an interactive rebase.
+        * in which case we could fast-forward without replacing the commits
+        * with new commits recreated by replaying their changes. This
+        * optimization must not be done if this is an interactive rebase.
         */
-       if (can_fast_forward(options.onto, &options.orig_head, &merge_base) &&
-           !is_interactive(&options) && !options.restrict_revision &&
-           options.upstream &&
-           !oidcmp(&options.upstream->object.oid, &options.onto->object.oid)) {
+       if (can_fast_forward(options.onto, options.upstream, &options.orig_head,
+                   &merge_base) &&
+           !is_interactive(&options) && !options.restrict_revision) {
                int flag;
 
                if (!(options.flags & REBASE_FORCE)) {

diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
index 80b23fd3269c7828660469207728f67087c82467..d7c724bea3b28feaa14ec8b48ba9e1ffc0e91b18 100755 (executable)
--- a/t/t3400-rebase.sh
+++ b/t/t3400-rebase.sh
@@ -295,7 +295,7 @@ test_expect_success 'rebase --am and --show-current-patch' '
                echo two >>init.t &&
                git commit -a -m two &&
                git tag two &&
-               test_must_fail git rebase --onto init HEAD^ &&
+               test_must_fail git rebase -f --onto init HEAD^ &&
                GIT_TRACE=1 git rebase --show-current-patch >/dev/null 2>stderr &&
                grep "show.*$(git rev-parse two)" stderr
        )

diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 461dd539ffd4803c62d54e22e8921fbefa2c0786..3cc9052f10ab83086b1ae96b53deb7f100796a44 100755 (executable)
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1058,7 +1058,7 @@ test_expect_success C_LOCALE_OUTPUT 'rebase --edit-todo does not work on non-int
        git reset --hard &&
        git checkout conflict-branch &&
        set_fake_editor &&
-       test_must_fail git rebase --onto HEAD~2 HEAD~ &&
+       test_must_fail git rebase -f --onto HEAD~2 HEAD~ &&
        test_must_fail git rebase --edit-todo &&
        git rebase --abort
 '

diff --git a/t/t3432-rebase-fast-forward.sh b/t/t3432-rebase-fast-forward.sh
index 58ecb33e08a7e18c0ac13f5f2599436aff5f2b72..35bc94142d4765f655f4e3aae3c36b986b70729f 100755 (executable)
--- a/t/t3432-rebase-fast-forward.sh
+++ b/t/t3432-rebase-fast-forward.sh
@@ -106,9 +106,9 @@ test_expect_success 'add work same to upstream' '
 changes='our and their changes'
 test_rebase_same_head success noop same success noop-force diff --onto B B
 test_rebase_same_head success noop same success noop-force diff --onto B... B
-test_rebase_same_head failure work same success work diff --onto master... master
+test_rebase_same_head success noop same success work diff --onto master... master
 test_rebase_same_head failure work same success work diff --fork-point --onto B B
 test_rebase_same_head failure work same success work diff --fork-point --onto B... B
-test_rebase_same_head failure work same success work diff --fork-point --onto master... master
+test_rebase_same_head success noop same success work diff --fork-point --onto master... master
 
 test_done