| Message ID | ec55da07191e7f0a1d31342053c1496405ba7d3a.1555366891.git.liu.denton@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | rebase: teach rebase --keep-base | expand |
Denton Liu <liu.denton@gmail.com> writes: > Before, when we had the following graph, > > A---B---C (master) > \ > D (side) This is minor, but comparing the above with below > F---G topic > / > A---B---C---D---E master you'll notice that branches growing downwards in your picture (this applies also to an illustration in your tests) are off by one column. D / A---B---C \ E > @@ -1682,13 +1699,10 @@ 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. > + * but this should be done if this is not an interactive rebase. > */ Two issues. - One is shared with the original (i.e. not a problem with this patch), but what "this" refers to is not what has already been stated in the sentence. We check, to see if we are in a situation where a specific optimization is possible. But this (== optimization to fast-forward without actually replaying the commit's changes) should be done only under such and such condition. - The other is more grave. "should be done if this is not an interactive rebase" drops "only" from "only if" in the original, which changes the meaning of the sentence (it can be read as "we check if we can optimize, but the optimization should be done for rebase-i regardless of the result of that said check", which is not what you mean). Perhaps Check if we are ..., 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.
Hi Denton It's good to see rebase fast-forwarding properly when it should On 15/04/2019 23:29, Denton Liu wrote: > 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. > > While we're at it, remove a trailing whitespace from rebase.c. > > Signed-off-by: Denton Liu <liu.denton@gmail.com> > --- > builtin/rebase.c | 40 +++++++++++++++++++++++----------- > t/t3400-rebase.sh | 2 +- > t/t3404-rebase-interactive.sh | 2 +- > t/t3432-rebase-fast-forward.sh | 4 ++-- > 4 files changed, 31 insertions(+), 17 deletions(-) > > diff --git a/builtin/rebase.c b/builtin/rebase.c > index 77deebc65c..7aa6a090d4 100644 > --- a/builtin/rebase.c > +++ b/builtin/rebase.c > @@ -895,12 +895,12 @@ 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; > - int res; > + struct commit_list *merge_bases = NULL; > + int res = 0; > > if (!head) > return 0; > @@ -908,12 +908,29 @@ static int can_fast_forward(struct commit *onto, struct object_id *head_oid, > merge_bases = get_merge_bases(onto, head); > if (merge_bases && !merge_bases->next) { > oidcpy(merge_base, &merge_bases->item->object.oid); > - res = oideq(merge_base, &onto->object.oid); > + if (!oideq(merge_base, &onto->object.oid)) > + goto done; > } else { > oidcpy(merge_base, &null_oid); > - res = 0; > + goto done; > } > + > + if (!upstream) > + goto done; > + > free_commit_list(merge_bases); > + merge_bases = get_merge_bases(upstream, head); > + if (merge_bases && !merge_bases->next) { > + if (!oideq(&onto->object.oid, &merge_bases->item->object.oid)) > + goto done; > + } else > + goto done; > + > + res = 1; > + > +done: > + if (merge_bases) > + free_commit_list(merge_bases); > return res && is_linear_history(onto, head); > } > I had a hard time following all those gotos. When you 'goto done' in both branches of an if statement it is hard to work out which cases fall through to the rest of the code. If I've understood it correctly I think it is clearer as merge_bases = get_merge_bases(onto, head); if (merge_bases && !merge_bases->next) { oidcpy(merge_base, &merge_bases->item->object.oid); if (oideq(merge_base, &onto->object.oid) && upstream) { free_commit_list(merge_bases); merge_bases = get_merge_bases(upstream, head); if (merge_bases && !merge_bases->next) if (oideq(&onto->object.oid, &merge_bases->item->object.oid)) res = 1; } } else { oidcpy(merge_base, &null_oid); } if (merge_bases) free_commit_list(merge_bases); return res && is_linear_history(onto, head); } The nested if's aren't great but I think it is easier to follow > @@ -1682,13 +1699,10 @@ 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. > + * but this should be done if this is not 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) && This is rather long, perhaps break the argument list Best Wishes Phillip > + !is_interactive(&options) && !options.restrict_revision) { > int flag; > > if (!(options.flags & REBASE_FORCE)) { > @@ -1782,7 +1796,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) > strbuf_addf(&msg, "%s: checkout %s", > getenv(GIT_REFLOG_ACTION_ENVIRONMENT), options.onto_name); > if (reset_head(&options.onto->object.oid, "checkout", NULL, > - RESET_HEAD_DETACH | RESET_ORIG_HEAD | > + RESET_HEAD_DETACH | RESET_ORIG_HEAD | > RESET_HEAD_RUN_POST_CHECKOUT_HOOK, > NULL, msg.buf)) > die(_("Could not detach HEAD")); > diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh > index 460d0523be..604d624ff8 100755 > --- a/t/t3400-rebase.sh > +++ b/t/t3400-rebase.sh > @@ -295,7 +295,7 @@ test_expect_success 'rebase--am.sh 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 b60b11f9f2..f054186cc7 100755 > --- a/t/t3404-rebase-interactive.sh > +++ b/t/t3404-rebase-interactive.sh > @@ -1066,7 +1066,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 4f04d67fd7..d0e5b1f3e6 100755 > --- a/t/t3432-rebase-fast-forward.sh > +++ b/t/t3432-rebase-fast-forward.sh > @@ -64,9 +64,9 @@ test_expect_success 'add work to upstream' ' > changes='our and their changes' > test_rebase_same_head success --onto B B > test_rebase_same_head success --onto B... B > -test_rebase_same_head failure --onto master... master > +test_rebase_same_head success --onto master... master > test_rebase_same_head failure --fork-point --onto B B > test_rebase_same_head failure --fork-point --onto B... B > -test_rebase_same_head failure --fork-point --onto master... master > +test_rebase_same_head success --fork-point --onto master... master > > test_done >
On Tue, Apr 16, 2019 at 02:59:12PM +0100, Phillip Wood wrote: > Hi Denton > > It's good to see rebase fast-forwarding properly when it should > > On 15/04/2019 23:29, Denton Liu wrote: > > 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. > > > > While we're at it, remove a trailing whitespace from rebase.c. > > > > Signed-off-by: Denton Liu <liu.denton@gmail.com> > > --- > > builtin/rebase.c | 40 +++++++++++++++++++++++----------- > > t/t3400-rebase.sh | 2 +- > > t/t3404-rebase-interactive.sh | 2 +- > > t/t3432-rebase-fast-forward.sh | 4 ++-- > > 4 files changed, 31 insertions(+), 17 deletions(-) > > > > diff --git a/builtin/rebase.c b/builtin/rebase.c > > index 77deebc65c..7aa6a090d4 100644 > > --- a/builtin/rebase.c > > +++ b/builtin/rebase.c > > @@ -895,12 +895,12 @@ 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; > > - int res; > > + struct commit_list *merge_bases = NULL; > > + int res = 0; > > if (!head) > > return 0; > > @@ -908,12 +908,29 @@ static int can_fast_forward(struct commit *onto, struct object_id *head_oid, > > merge_bases = get_merge_bases(onto, head); > > if (merge_bases && !merge_bases->next) { > > oidcpy(merge_base, &merge_bases->item->object.oid); > > - res = oideq(merge_base, &onto->object.oid); > > + if (!oideq(merge_base, &onto->object.oid)) > > + goto done; > > } else { > > oidcpy(merge_base, &null_oid); > > - res = 0; > > + goto done; > > } > > + > > + if (!upstream) > > + goto done; > > + > > free_commit_list(merge_bases); > > + merge_bases = get_merge_bases(upstream, head); > > + if (merge_bases && !merge_bases->next) { > > + if (!oideq(&onto->object.oid, &merge_bases->item->object.oid)) > > + goto done; > > + } else > > + goto done; > > + > > + res = 1; > > + > > +done: > > + if (merge_bases) > > + free_commit_list(merge_bases); > > return res && is_linear_history(onto, head); > > } > > I had a hard time following all those gotos. When you 'goto done' in both > branches of an if statement it is hard to work out which cases fall through > to the rest of the code. If I've understood it correctly I think it is > clearer as > > merge_bases = get_merge_bases(onto, head); > if (merge_bases && !merge_bases->next) { > oidcpy(merge_base, &merge_bases->item->object.oid); > if (oideq(merge_base, &onto->object.oid) && upstream) { > free_commit_list(merge_bases); > merge_bases = get_merge_bases(upstream, head); > if (merge_bases && !merge_bases->next) > if (oideq(&onto->object.oid, > &merge_bases->item->object.oid)) > res = 1; > } > } else { > oidcpy(merge_base, &null_oid); > } > > if (merge_bases) > free_commit_list(merge_bases); > return res && is_linear_history(onto, head); > } > > The nested if's aren't great but I think it is easier to follow I am pretty impartial between gotos and ifs. If no one else has any strong opinions between the two, I'll reroll with ifs. > > > @@ -1682,13 +1699,10 @@ 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. > > + * but this should be done if this is not 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) && > > This is rather long, perhaps break the argument list Thanks, will do. > > Best Wishes > > Phillip > > + !is_interactive(&options) && !options.restrict_revision) { > > int flag; > > if (!(options.flags & REBASE_FORCE)) { > > @@ -1782,7 +1796,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) > > strbuf_addf(&msg, "%s: checkout %s", > > getenv(GIT_REFLOG_ACTION_ENVIRONMENT), options.onto_name); > > if (reset_head(&options.onto->object.oid, "checkout", NULL, > > - RESET_HEAD_DETACH | RESET_ORIG_HEAD | > > + RESET_HEAD_DETACH | RESET_ORIG_HEAD | > > RESET_HEAD_RUN_POST_CHECKOUT_HOOK, > > NULL, msg.buf)) > > die(_("Could not detach HEAD")); > > diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh > > index 460d0523be..604d624ff8 100755 > > --- a/t/t3400-rebase.sh > > +++ b/t/t3400-rebase.sh > > @@ -295,7 +295,7 @@ test_expect_success 'rebase--am.sh 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 b60b11f9f2..f054186cc7 100755 > > --- a/t/t3404-rebase-interactive.sh > > +++ b/t/t3404-rebase-interactive.sh > > @@ -1066,7 +1066,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 4f04d67fd7..d0e5b1f3e6 100755 > > --- a/t/t3432-rebase-fast-forward.sh > > +++ b/t/t3432-rebase-fast-forward.sh > > @@ -64,9 +64,9 @@ test_expect_success 'add work to upstream' ' > > changes='our and their changes' > > test_rebase_same_head success --onto B B > > test_rebase_same_head success --onto B... B > > -test_rebase_same_head failure --onto master... master > > +test_rebase_same_head success --onto master... master > > test_rebase_same_head failure --fork-point --onto B B > > test_rebase_same_head failure --fork-point --onto B... B > > -test_rebase_same_head failure --fork-point --onto master... master > > +test_rebase_same_head success --fork-point --onto master... master > > test_done > >
On 17/04/2019 07:44, Denton Liu wrote: > On Tue, Apr 16, 2019 at 02:59:12PM +0100, Phillip Wood wrote: >> Hi Denton >> >> It's good to see rebase fast-forwarding properly when it should >> >> On 15/04/2019 23:29, Denton Liu wrote: >>> 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. >>> >>> While we're at it, remove a trailing whitespace from rebase.c. >>> >>> Signed-off-by: Denton Liu <liu.denton@gmail.com> >>> --- >>> builtin/rebase.c | 40 +++++++++++++++++++++++----------- >>> t/t3400-rebase.sh | 2 +- >>> t/t3404-rebase-interactive.sh | 2 +- >>> t/t3432-rebase-fast-forward.sh | 4 ++-- >>> 4 files changed, 31 insertions(+), 17 deletions(-) >>> >>> diff --git a/builtin/rebase.c b/builtin/rebase.c >>> index 77deebc65c..7aa6a090d4 100644 >>> --- a/builtin/rebase.c >>> +++ b/builtin/rebase.c >>> @@ -895,12 +895,12 @@ 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; >>> - int res; >>> + struct commit_list *merge_bases = NULL; >>> + int res = 0; >>> if (!head) >>> return 0; >>> @@ -908,12 +908,29 @@ static int can_fast_forward(struct commit *onto, struct object_id *head_oid, >>> merge_bases = get_merge_bases(onto, head); >>> if (merge_bases && !merge_bases->next) { >>> oidcpy(merge_base, &merge_bases->item->object.oid); >>> - res = oideq(merge_base, &onto->object.oid); >>> + if (!oideq(merge_base, &onto->object.oid)) >>> + goto done; >>> } else { >>> oidcpy(merge_base, &null_oid); >>> - res = 0; >>> + goto done; >>> } >>> + >>> + if (!upstream) >>> + goto done; >>> + >>> free_commit_list(merge_bases); >>> + merge_bases = get_merge_bases(upstream, head); >>> + if (merge_bases && !merge_bases->next) { >>> + if (!oideq(&onto->object.oid, &merge_bases->item->object.oid)) >>> + goto done; >>> + } else >>> + goto done; >>> + >>> + res = 1; >>> + >>> +done: >>> + if (merge_bases) >>> + free_commit_list(merge_bases); >>> return res && is_linear_history(onto, head); >>> } >> >> I had a hard time following all those gotos. When you 'goto done' in both >> branches of an if statement it is hard to work out which cases fall through >> to the rest of the code. If I've understood it correctly I think it is >> clearer as >> >> merge_bases = get_merge_bases(onto, head); >> if (merge_bases && !merge_bases->next) { >> oidcpy(merge_base, &merge_bases->item->object.oid); >> if (oideq(merge_base, &onto->object.oid) && upstream) { >> free_commit_list(merge_bases); >> merge_bases = get_merge_bases(upstream, head); >> if (merge_bases && !merge_bases->next) >> if (oideq(&onto->object.oid, >> &merge_bases->item->object.oid)) >> res = 1; that would be better as res = oideq(&onto->object.oid, &merge_bases->item->object.oid); without the last if >> } >> } else { >> oidcpy(merge_base, &null_oid); >> } >> >> if (merge_bases) >> free_commit_list(merge_bases); >> return res && is_linear_history(onto, head); >> } >> >> The nested if's aren't great but I think it is easier to follow > > I am pretty impartial between gotos and ifs. If no one else has any > strong opinions between the two, I'll reroll with ifs. If you want to keep the goto approach then refactoring the ifs as follows is clearer as it avoids jumping from both arms, each stage is a simple single armed if (something) goto done merge_bases = get_merge_bases(onto, head); if (!merge_bases || merge_bases->next) { oidcpy(merge_base, &null_oid); goto done; } oidcpy(merge_base, &merge_bases->item->object.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) res = oideq(&onto->object.oid, &merge_bases->item->object.oid); done: if (merge_bases) free_commit_list(merge_bases); return res && is_linear_history(onto, head); } Best Wishes Phillip >> >>> @@ -1682,13 +1699,10 @@ 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. >>> + * but this should be done if this is not 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) && >> >> This is rather long, perhaps break the argument list > > Thanks, will do. > >> >> Best Wishes >> >> Phillip >>> + !is_interactive(&options) && !options.restrict_revision) { >>> int flag; >>> if (!(options.flags & REBASE_FORCE)) { >>> @@ -1782,7 +1796,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) >>> strbuf_addf(&msg, "%s: checkout %s", >>> getenv(GIT_REFLOG_ACTION_ENVIRONMENT), options.onto_name); >>> if (reset_head(&options.onto->object.oid, "checkout", NULL, >>> - RESET_HEAD_DETACH | RESET_ORIG_HEAD | >>> + RESET_HEAD_DETACH | RESET_ORIG_HEAD | >>> RESET_HEAD_RUN_POST_CHECKOUT_HOOK, >>> NULL, msg.buf)) >>> die(_("Could not detach HEAD")); >>> diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh >>> index 460d0523be..604d624ff8 100755 >>> --- a/t/t3400-rebase.sh >>> +++ b/t/t3400-rebase.sh >>> @@ -295,7 +295,7 @@ test_expect_success 'rebase--am.sh 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 b60b11f9f2..f054186cc7 100755 >>> --- a/t/t3404-rebase-interactive.sh >>> +++ b/t/t3404-rebase-interactive.sh >>> @@ -1066,7 +1066,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 4f04d67fd7..d0e5b1f3e6 100755 >>> --- a/t/t3432-rebase-fast-forward.sh >>> +++ b/t/t3432-rebase-fast-forward.sh >>> @@ -64,9 +64,9 @@ test_expect_success 'add work to upstream' ' >>> changes='our and their changes' >>> test_rebase_same_head success --onto B B >>> test_rebase_same_head success --onto B... B >>> -test_rebase_same_head failure --onto master... master >>> +test_rebase_same_head success --onto master... master >>> test_rebase_same_head failure --fork-point --onto B B >>> test_rebase_same_head failure --fork-point --onto B... B >>> -test_rebase_same_head failure --fork-point --onto master... master >>> +test_rebase_same_head success --fork-point --onto master... master >>> test_done >>>
Hi Junio, On Mon, Apr 15, 2019 at 03:29:24PM -0700, Denton Liu wrote: > 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. > > While we're at it, remove a trailing whitespace from rebase.c. > > Signed-off-by: Denton Liu <liu.denton@gmail.com> > --- > builtin/rebase.c | 40 +++++++++++++++++++++++----------- > t/t3400-rebase.sh | 2 +- > t/t3404-rebase-interactive.sh | 2 +- > t/t3432-rebase-fast-forward.sh | 4 ++-- > 4 files changed, 31 insertions(+), 17 deletions(-) > > diff --git a/builtin/rebase.c b/builtin/rebase.c > index 77deebc65c..7aa6a090d4 100644 > --- a/builtin/rebase.c > +++ b/builtin/rebase.c > @@ -895,12 +895,12 @@ 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; > - int res; > + struct commit_list *merge_bases = NULL; > + int res = 0; > > if (!head) > return 0; > @@ -908,12 +908,29 @@ static int can_fast_forward(struct commit *onto, struct object_id *head_oid, > merge_bases = get_merge_bases(onto, head); > if (merge_bases && !merge_bases->next) { > oidcpy(merge_base, &merge_bases->item->object.oid); > - res = oideq(merge_base, &onto->object.oid); > + if (!oideq(merge_base, &onto->object.oid)) > + goto done; > } else { > oidcpy(merge_base, &null_oid); > - res = 0; > + goto done; > } > + > + if (!upstream) > + goto done; > + > free_commit_list(merge_bases); > + merge_bases = get_merge_bases(upstream, head); > + if (merge_bases && !merge_bases->next) { > + if (!oideq(&onto->object.oid, &merge_bases->item->object.oid)) > + goto done; > + } else > + goto done; > + > + res = 1; > + > +done: > + if (merge_bases) > + free_commit_list(merge_bases); > return res && is_linear_history(onto, head); > } > > @@ -1682,13 +1699,10 @@ 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. > + * but this should be done if this is not an interactive rebase. > */ I forgot to incorporate your comment about this comment block in the last reroll. I'm not sure if this is worth another reroll so could you please change the comment block to this: /* * Check if we are already based on onto with linear history, * 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. */ Thanks, Denton > - 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)) { > @@ -1782,7 +1796,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) > strbuf_addf(&msg, "%s: checkout %s", > getenv(GIT_REFLOG_ACTION_ENVIRONMENT), options.onto_name); > if (reset_head(&options.onto->object.oid, "checkout", NULL, > - RESET_HEAD_DETACH | RESET_ORIG_HEAD | > + RESET_HEAD_DETACH | RESET_ORIG_HEAD | > RESET_HEAD_RUN_POST_CHECKOUT_HOOK, > NULL, msg.buf)) > die(_("Could not detach HEAD")); > diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh > index 460d0523be..604d624ff8 100755 > --- a/t/t3400-rebase.sh > +++ b/t/t3400-rebase.sh > @@ -295,7 +295,7 @@ test_expect_success 'rebase--am.sh 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 b60b11f9f2..f054186cc7 100755 > --- a/t/t3404-rebase-interactive.sh > +++ b/t/t3404-rebase-interactive.sh > @@ -1066,7 +1066,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 4f04d67fd7..d0e5b1f3e6 100755 > --- a/t/t3432-rebase-fast-forward.sh > +++ b/t/t3432-rebase-fast-forward.sh > @@ -64,9 +64,9 @@ test_expect_success 'add work to upstream' ' > changes='our and their changes' > test_rebase_same_head success --onto B B > test_rebase_same_head success --onto B... B > -test_rebase_same_head failure --onto master... master > +test_rebase_same_head success --onto master... master > test_rebase_same_head failure --fork-point --onto B B > test_rebase_same_head failure --fork-point --onto B... B > -test_rebase_same_head failure --fork-point --onto master... master > +test_rebase_same_head success --fork-point --onto master... master > > test_done > -- > 2.21.0.921.gb27c68c4e9 >
diff --git a/builtin/rebase.c b/builtin/rebase.c index 77deebc65c..7aa6a090d4 100644 --- a/builtin/rebase.c +++ b/builtin/rebase.c @@ -895,12 +895,12 @@ 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; - int res; + struct commit_list *merge_bases = NULL; + int res = 0; if (!head) return 0; @@ -908,12 +908,29 @@ static int can_fast_forward(struct commit *onto, struct object_id *head_oid, merge_bases = get_merge_bases(onto, head); if (merge_bases && !merge_bases->next) { oidcpy(merge_base, &merge_bases->item->object.oid); - res = oideq(merge_base, &onto->object.oid); + if (!oideq(merge_base, &onto->object.oid)) + goto done; } else { oidcpy(merge_base, &null_oid); - res = 0; + goto done; } + + if (!upstream) + goto done; + free_commit_list(merge_bases); + merge_bases = get_merge_bases(upstream, head); + if (merge_bases && !merge_bases->next) { + if (!oideq(&onto->object.oid, &merge_bases->item->object.oid)) + goto done; + } else + goto done; + + res = 1; + +done: + if (merge_bases) + free_commit_list(merge_bases); return res && is_linear_history(onto, head); } @@ -1682,13 +1699,10 @@ 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. + * but this should be done if this is not 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)) { @@ -1782,7 +1796,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) strbuf_addf(&msg, "%s: checkout %s", getenv(GIT_REFLOG_ACTION_ENVIRONMENT), options.onto_name); if (reset_head(&options.onto->object.oid, "checkout", NULL, - RESET_HEAD_DETACH | RESET_ORIG_HEAD | + RESET_HEAD_DETACH | RESET_ORIG_HEAD | RESET_HEAD_RUN_POST_CHECKOUT_HOOK, NULL, msg.buf)) die(_("Could not detach HEAD")); diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh index 460d0523be..604d624ff8 100755 --- a/t/t3400-rebase.sh +++ b/t/t3400-rebase.sh @@ -295,7 +295,7 @@ test_expect_success 'rebase--am.sh 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 b60b11f9f2..f054186cc7 100755 --- a/t/t3404-rebase-interactive.sh +++ b/t/t3404-rebase-interactive.sh @@ -1066,7 +1066,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 4f04d67fd7..d0e5b1f3e6 100755 --- a/t/t3432-rebase-fast-forward.sh +++ b/t/t3432-rebase-fast-forward.sh @@ -64,9 +64,9 @@ test_expect_success 'add work to upstream' ' changes='our and their changes' test_rebase_same_head success --onto B B test_rebase_same_head success --onto B... B -test_rebase_same_head failure --onto master... master +test_rebase_same_head success --onto master... master test_rebase_same_head failure --fork-point --onto B B test_rebase_same_head failure --fork-point --onto B... B -test_rebase_same_head failure --fork-point --onto master... master +test_rebase_same_head success --fork-point --onto master... master test_done
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. While we're at it, remove a trailing whitespace from rebase.c. Signed-off-by: Denton Liu <liu.denton@gmail.com> --- builtin/rebase.c | 40 +++++++++++++++++++++++----------- t/t3400-rebase.sh | 2 +- t/t3404-rebase-interactive.sh | 2 +- t/t3432-rebase-fast-forward.sh | 4 ++-- 4 files changed, 31 insertions(+), 17 deletions(-)