| Message ID | 2efbfc94187d9f0968e5b670c9152651cd8f1a5b.1665650564.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | rebase --keep-base: imply --reapply-cherry-picks and --no-fork-point | expand |
On Thu, Oct 13 2022, Phillip Wood via GitGitGadget wrote: > From: Phillip Wood <phillip.wood@dunelm.org.uk> > > Separate out calculating the merge base between 'onto' and 'HEAD' from > the check for whether we can fast-forward or not. This means we can skip > the fast-forward checks when the rebase is forced and avoid calculating > the merge-base between 'HEAD' and 'onto' when --keep-base is given. > > Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> > --- > builtin/rebase.c | 34 ++++++++++++++++++++-------------- > 1 file changed, 20 insertions(+), 14 deletions(-) > > diff --git a/builtin/rebase.c b/builtin/rebase.c > index cbafcc41e75..40619a0fb2d 100644 > --- a/builtin/rebase.c > +++ b/builtin/rebase.c > @@ -871,13 +871,9 @@ static int can_fast_forward(struct commit *onto, struct commit *upstream, > struct commit_list *merge_bases = NULL; > int res = 0; > > - merge_bases = get_merge_bases(onto, head); > - if (!merge_bases || merge_bases->next) { > - oidcpy(branch_base, null_oid()); > - goto done; > - } > + if (is_null_oid(branch_base)) > + goto done; /* fill_branch_base() found multiple merge bases */ > > - oidcpy(branch_base, &merge_bases->item->object.oid); > if (!oideq(branch_base, &onto->object.oid)) > goto done; > > @@ -887,7 +883,6 @@ static int can_fast_forward(struct commit *onto, struct commit *upstream, > if (!upstream) > goto done; > > - free_commit_list(merge_bases); > merge_bases = get_merge_bases(upstream, head); > if (!merge_bases || merge_bases->next) > goto done; > @@ -902,6 +897,20 @@ done: > return res && is_linear_history(onto, head); > } > > +static void fill_branch_base(struct rebase_options *options, > + struct object_id *branch_base) > +{ > + struct commit_list *merge_bases = NULL; > + > + merge_bases = get_merge_bases(options->onto, options->orig_head); > + if (!merge_bases || merge_bases->next) > + oidcpy(branch_base, null_oid()); > + else > + oidcpy(branch_base, &merge_bases->item->object.oid); > + > + free_commit_list(merge_bases); > +} I wondered if this could be a bit shorter/less wrap-y with shorter variable names, anyway, I see it's code copied from above, so nevermind in advance... :) static void fill_branch_base(struct rebase_options *o, struct object_id *dst) { struct commit_list *mb = get_merge_bases(o->onto, o->orig_head); const struct object_id *src = (!mb || mb->next) ? null_oid() : &mb->item->object.oid; oidcpy(dst, src); free_commit_list(mb); } > @@ -1669,8 +1678,8 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) > if (!options.onto) > die(_("Does not point to a valid commit '%s'"), > options.onto_name); > + fill_branch_base(&options, &branch_base); > } > - > if (options.fork_point > 0) > options.restrict_revision = > get_fork_point(options.upstream_name, options.orig_head); I wouldn't mind the stray whitespace change, but here it seems unintentional, in 7/8 your change on top is: @@ -1680,6 +1691,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) options.onto_name); fill_branch_base(&options, &branch_base); } + if (keep_base && options.reapply_cherry_picks) + options.upstream = options.onto; + if (options.fork_point > 0) options.restrict_revision = get_fork_point(options.upstream_name, options.orig_head); Presumably we want to have \n\n spacing for both of those, and to not remove the spacing here in 6/8, only to add it back?
On 13/10/2022 20:21, Ævar Arnfjörð Bjarmason wrote: >> +static void fill_branch_base(struct rebase_options *options, >> + struct object_id *branch_base) >> +{ >> + struct commit_list *merge_bases = NULL; >> + >> + merge_bases = get_merge_bases(options->onto, options->orig_head); >> + if (!merge_bases || merge_bases->next) >> + oidcpy(branch_base, null_oid()); >> + else >> + oidcpy(branch_base, &merge_bases->item->object.oid); >> + >> + free_commit_list(merge_bases); >> +} > > I wondered if this could be a bit shorter/less wrap-y Where's the wrapping? > with shorter > variable names, anyway, I see it's code copied from above, so nevermind > in advance... :) As it is copied it is easier to review leaving it as is I think. > static void fill_branch_base(struct rebase_options *o, struct object_id *dst) > { > struct commit_list *mb = get_merge_bases(o->onto, o->orig_head); > const struct object_id *src = (!mb || mb->next) ? null_oid() : > &mb->item->object.oid; > > oidcpy(dst, src); > free_commit_list(mb); > } > > >> @@ -1669,8 +1678,8 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) >> if (!options.onto) >> die(_("Does not point to a valid commit '%s'"), >> options.onto_name); >> + fill_branch_base(&options, &branch_base); >> } >> - >> if (options.fork_point > 0) >> options.restrict_revision = >> get_fork_point(options.upstream_name, options.orig_head); > > I wouldn't mind the stray whitespace change, but here it seems > unintentional, in 7/8 your change on top is: Thanks, well spotted, I'm sure I've fixed this at least once already, I must have reintroduced it when fixing a rebase conflict. I'll fix it again. Best Wishes Phillip > @@ -1680,6 +1691,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) > options.onto_name); > fill_branch_base(&options, &branch_base); > } > + if (keep_base && options.reapply_cherry_picks) > + options.upstream = options.onto; > + > if (options.fork_point > 0) > options.restrict_revision = > get_fork_point(options.upstream_name, options.orig_head); > > Presumably we want to have \n\n spacing for both of those, and to not > remove the spacing here in 6/8, only to add it back?
On Mon, Oct 17 2022, Phillip Wood wrote: > On 13/10/2022 20:21, Ævar Arnfjörð Bjarmason wrote: >>> +static void fill_branch_base(struct rebase_options *options, >>> + struct object_id *branch_base) >>> +{ >>> + struct commit_list *merge_bases = NULL; >>> + >>> + merge_bases = get_merge_bases(options->onto, options->orig_head); >>> + if (!merge_bases || merge_bases->next) >>> + oidcpy(branch_base, null_oid()); >>> + else >>> + oidcpy(branch_base, &merge_bases->item->object.oid); >>> + >>> + free_commit_list(merge_bases); >>> +} >> I wondered if this could be a bit shorter/less wrap-y > > Where's the wrapping? Sorry about being unclear, I meant (but completely failed to get across) that you seemed to be pre-declaring the "merge_bases" to avoid wrapping the "get_merge_bases()" line. But reading it again maybe it was just copied as-is from the pre-image. In any case as we're moving this to a new function maybe a fix-up to make it: struct commit_list *merge_bases = get_merge_bases(options->onto, options->orig_head); would be marginally easier to read, as we never use that NULL-init (which again, is also an issue in the pre-image). Anyway, if you want to keep this all as-is that's fine with me. >> with shorter >> variable names, anyway, I see it's code copied from above, so nevermind >> in advance... :) > > As it is copied it is easier to review leaving it as is I think. *nod*
diff --git a/builtin/rebase.c b/builtin/rebase.c index cbafcc41e75..40619a0fb2d 100644 --- a/builtin/rebase.c +++ b/builtin/rebase.c @@ -871,13 +871,9 @@ static int can_fast_forward(struct commit *onto, struct commit *upstream, struct commit_list *merge_bases = NULL; int res = 0; - merge_bases = get_merge_bases(onto, head); - if (!merge_bases || merge_bases->next) { - oidcpy(branch_base, null_oid()); - goto done; - } + if (is_null_oid(branch_base)) + goto done; /* fill_branch_base() found multiple merge bases */ - oidcpy(branch_base, &merge_bases->item->object.oid); if (!oideq(branch_base, &onto->object.oid)) goto done; @@ -887,7 +883,6 @@ static int can_fast_forward(struct commit *onto, struct commit *upstream, if (!upstream) goto done; - free_commit_list(merge_bases); merge_bases = get_merge_bases(upstream, head); if (!merge_bases || merge_bases->next) goto done; @@ -902,6 +897,20 @@ done: return res && is_linear_history(onto, head); } +static void fill_branch_base(struct rebase_options *options, + struct object_id *branch_base) +{ + struct commit_list *merge_bases = NULL; + + merge_bases = get_merge_bases(options->onto, options->orig_head); + if (!merge_bases || merge_bases->next) + oidcpy(branch_base, null_oid()); + else + oidcpy(branch_base, &merge_bases->item->object.oid); + + free_commit_list(merge_bases); +} + static int parse_opt_am(const struct option *opt, const char *arg, int unset) { struct rebase_options *opts = opt->value; @@ -1669,8 +1678,8 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) if (!options.onto) die(_("Does not point to a valid commit '%s'"), options.onto_name); + fill_branch_base(&options, &branch_base); } - if (options.fork_point > 0) options.restrict_revision = get_fork_point(options.upstream_name, options.orig_head); @@ -1698,13 +1707,10 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) * 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. - * - * Note that can_fast_forward() initializes branch_base, so we have to - * call it before checking allow_preemptive_ff. */ - if (can_fast_forward(options.onto, options.upstream, options.restrict_revision, - options.orig_head, &branch_base) && - allow_preemptive_ff) { + if (allow_preemptive_ff && + can_fast_forward(options.onto, options.upstream, options.restrict_revision, + options.orig_head, &branch_base)) { int flag; if (!(options.flags & REBASE_FORCE)) {