{"thread":{"id":"50860","subject":"[PATCH v3.5 3/4] rebase: fast-forward --onto in more cases","startedAt":"2019-04-02T05:09:25Z","lastAt":"2019-04-02T05:09:25Z","messageCount":1,"participants":["Denton Liu"],"isPatch":true,"patchVersion":3,"patchTotal":4},"messages":[{"id":"372965","messageId":"7806809c32c8dae96fd7b1c4da6af4d74e2a2783.1554181577.git.liu.denton@gmail.com","threadId":"50860","inReplyTo":null,"subject":"[PATCH v3.5 3/4] rebase: fast-forward --onto in more cases","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-04-02T05:09:20Z","receivedAt":"2019-04-02T05:09:25Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Before, when we had the following graph,\n\n\tA---B---C (master)\n\t    \\\n\t     D (side)\n\nrunning 'git rebase --onto master... master side' would result in D\nbeing always rebased, no matter what. However, the desired behavior is\nthat rebase should notice that this is fast-forwardable and do that\ninstead.\n\nAdd detection to `can_fast_forward` so that this case can be detected\nand a fast-forward will be performed. First of all, rewrite the function\nto use gotos which simplifies the logic. Next, since the\n\n\toptions.upstream &&\n\t!oidcmp(&options.upstream->object.oid, &options.onto->object.oid)\n\nconditions were removed in `cmd_rebase`, we reintroduce a substitute in\n`can_fast_forward`. In particular, checking the merge bases of\n`upstream` and `head` fixes a failing case in t3416.\n\nThe abbreviated graph for t3416 is as follows:\n\n\t\t    F---G topic\n\t\t   /\n\t  A---B---C---D---E master\n\nand the failing command was\n\n\tgit rebase --onto master...topic F topic\n\nBefore, Git would see that there was one merge base (C), and the merge\nand onto were the same so it would incorrectly return 1, indicating that\nwe could fast-forward. This would cause the rebased graph to be 'ABCFG'\nwhen we were expecting 'ABCG'.\n\nWith the additional logic, we detect that upstream and head's merge base\nis F. Since onto isn't F, it means we're not rebasing the full set of\ncommits from master..topic. Since we're excluding some commits, a\nfast-forward cannot be performed and so we correctly return 0.\n\nAdd '-f' to test cases that failed as a result of this change because\nthey were not expecting a fast-forward so that a rebase is forced.\n\nWhile we're at it, remove a trailing whitespace from rebase.c.\n\nSigned-off-by: Denton Liu <liu.denton@gmail.com>\n---\n\nHi Junio,\n\nCould you please use this patch instead of v3's 3/4? The code should be\nthe same but the log message goes into a lot more detail and (hopefully)\naddresses the concerns you had earlier.\n\nThanks,\n\nDenton\n\n builtin/rebase.c               | 40 +++++++++++++++++++++++-----------\n t/t3400-rebase.sh              |  2 +-\n t/t3404-rebase-interactive.sh  |  2 +-\n t/t3432-rebase-fast-forward.sh |  2 +-\n 4 files changed, 30 insertions(+), 16 deletions(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 77deebc65c..7aa6a090d4 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -895,12 +895,12 @@ static int is_linear_history(struct commit *from, struct commit *to)\n \treturn 1;\n }\n \n-static int can_fast_forward(struct commit *onto, struct object_id *head_oid,\n-\t\t\t    struct object_id *merge_base)\n+static int can_fast_forward(struct commit *onto, struct commit *upstream,\n+\t\t\t    struct object_id *head_oid, struct object_id *merge_base)\n {\n \tstruct commit *head = lookup_commit(the_repository, head_oid);\n-\tstruct commit_list *merge_bases;\n-\tint res;\n+\tstruct commit_list *merge_bases = NULL;\n+\tint res = 0;\n \n \tif (!head)\n \t\treturn 0;\n@@ -908,12 +908,29 @@ static int can_fast_forward(struct commit *onto, struct object_id *head_oid,\n \tmerge_bases = get_merge_bases(onto, head);\n \tif (merge_bases && !merge_bases->next) {\n \t\toidcpy(merge_base, &merge_bases->item->object.oid);\n-\t\tres = oideq(merge_base, &onto->object.oid);\n+\t\tif (!oideq(merge_base, &onto->object.oid))\n+\t\t\tgoto done;\n \t} else {\n \t\toidcpy(merge_base, &null_oid);\n-\t\tres = 0;\n+\t\tgoto done;\n \t}\n+\n+\tif (!upstream)\n+\t\tgoto done;\n+\n \tfree_commit_list(merge_bases);\n+\tmerge_bases = get_merge_bases(upstream, head);\n+\tif (merge_bases && !merge_bases->next) {\n+\t\tif (!oideq(&onto->object.oid, &merge_bases->item->object.oid))\n+\t\t\tgoto done;\n+\t} else\n+\t\tgoto done;\n+\n+\tres = 1;\n+\n+done:\n+\tif (merge_bases)\n+\t\tfree_commit_list(merge_bases);\n \treturn res && is_linear_history(onto, head);\n }\n \n@@ -1682,13 +1699,10 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \n \t/*\n \t * Check if we are already based on onto with linear history,\n-\t * but this should be done only when upstream and onto are the same\n-\t * and if this is not an interactive rebase.\n+\t * but this should be done if this is not an interactive rebase.\n \t */\n-\tif (can_fast_forward(options.onto, &options.orig_head, &merge_base) &&\n-\t    !is_interactive(&options) && !options.restrict_revision &&\n-\t    options.upstream &&\n-\t    !oidcmp(&options.upstream->object.oid, &options.onto->object.oid)) {\n+\tif (can_fast_forward(options.onto, options.upstream, &options.orig_head, &merge_base) &&\n+\t    !is_interactive(&options) && !options.restrict_revision) {\n \t\tint flag;\n \n \t\tif (!(options.flags & REBASE_FORCE)) {\n@@ -1782,7 +1796,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \tstrbuf_addf(&msg, \"%s: checkout %s\",\n \t\t    getenv(GIT_REFLOG_ACTION_ENVIRONMENT), options.onto_name);\n \tif (reset_head(&options.onto->object.oid, \"checkout\", NULL,\n-\t\t       RESET_HEAD_DETACH | RESET_ORIG_HEAD | \n+\t\t       RESET_HEAD_DETACH | RESET_ORIG_HEAD |\n \t\t       RESET_HEAD_RUN_POST_CHECKOUT_HOOK,\n \t\t       NULL, msg.buf))\n \t\tdie(_(\"Could not detach HEAD\"));\ndiff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh\nindex 460d0523be..604d624ff8 100755\n--- a/t/t3400-rebase.sh\n+++ b/t/t3400-rebase.sh\n@@ -295,7 +295,7 @@ test_expect_success 'rebase--am.sh and --show-current-patch' '\n \t\techo two >>init.t &&\n \t\tgit commit -a -m two &&\n \t\tgit tag two &&\n-\t\ttest_must_fail git rebase --onto init HEAD^ &&\n+\t\ttest_must_fail git rebase -f --onto init HEAD^ &&\n \t\tGIT_TRACE=1 git rebase --show-current-patch >/dev/null 2>stderr &&\n \t\tgrep \"show.*$(git rev-parse two)\" stderr\n \t)\ndiff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\nindex b60b11f9f2..f054186cc7 100755\n--- a/t/t3404-rebase-interactive.sh\n+++ b/t/t3404-rebase-interactive.sh\n@@ -1066,7 +1066,7 @@ test_expect_success C_LOCALE_OUTPUT 'rebase --edit-todo does not work on non-int\n \tgit reset --hard &&\n \tgit checkout conflict-branch &&\n \tset_fake_editor &&\n-\ttest_must_fail git rebase --onto HEAD~2 HEAD~ &&\n+\ttest_must_fail git rebase -f --onto HEAD~2 HEAD~ &&\n \ttest_must_fail git rebase --edit-todo &&\n \tgit rebase --abort\n '\ndiff --git a/t/t3432-rebase-fast-forward.sh b/t/t3432-rebase-fast-forward.sh\nindex 3e6362dd9c..414b4216d6 100755\n--- a/t/t3432-rebase-fast-forward.sh\n+++ b/t/t3432-rebase-fast-forward.sh\n@@ -54,6 +54,6 @@ test_expect_success 'add work to upstream' '\n changes='our and their changes'\n test_rebase_same_head success '--onto B B'\n test_rebase_same_head success '--onto B... B'\n-test_rebase_same_head failure '--onto master... master'\n+test_rebase_same_head success '--onto master... master'\n \n test_done\n-- \n2.21.0.199.g48d46b195d.dirty\n\n"}]}