Re: [RFC WIP PATCH v8 11/13] rebase: eliminate side-effects from can_fast_forward()
- From
Johannes Schindelin <johannes.schindelin@gmx.de>
- Date
- May 17, 2019, 21:16 UTC
- Message-ID
- <nycvar.QRO.7.76.6.1905172301210.46@tvgsbejvaqbjf.bet>
- In-Reply-To
- <20190508001252.15752-12-avarab@gmail.com>
Hi Ævar,
On Wed, 8 May 2019, Ævar Arnfjörð Bjarmason wrote:
Show 15 quoted lines
> The can_fast_forward() function is potentially much more expensive
> than is_interactive() since it e.g. might need to call
> is_linear_history().
>
> So reversing the two looks like an obvious improvement, but doing so
> reveals a previously hidden caveat: We need the can_fast_forward()
> function to populate data used later, namely the "merge_bases"
> variable. This has been the case since it was added in
> 9a48a615b4 ("builtin rebase: try to fast forward when possible",
> 2018-09-04).
>
> So let's refactor it into two functions. Now we'll always call
> populate_merge_bases(), and then only call can_fast_forward() if
> is_interactive() is false, making this both faster in pathological
> cases, and more importantly easier to follow.True.
We might want to mention, though, what exactly that this "pathological case" is: rebasing commits onto an unrelated history.
Show 14 quoted lines
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index ae6b9b42b8..cb5d7fcb53 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -878,24 +878,30 @@ static int is_linear_history(struct commit *from, struct commit *to)
> return 1;
> }
>
> -static int can_fast_forward(struct commit *onto, struct commit *upstream,
> +static void populate_merge_bases(struct commit *head, struct commit *onto,
> + struct commit_list *merge_bases,
> + struct object_id *merge_base)
> +{
> + merge_bases = get_merge_bases(onto, head);Hmm. This overrides whatever was passed in via the parameter. Did you mean to make the parameter of type `struct commit_list **`, i.e. a *pointer to a pointer*?
Show 29 quoted lines
> + if (!merge_bases || merge_bases->next) {
> + oidcpy(merge_base, &null_oid);
> + return;
> + }
> + oidcpy(merge_base, &merge_bases->item->object.oid);
> +}
> +
> +static int can_fast_forward(struct commit *head,
> + struct commit *onto, struct commit *upstream,
> struct commit *restrict_revision,
> - struct object_id *head_oid, struct object_id *merge_base)
> + struct object_id *head_oid,
> + struct commit_list *merge_bases,
> + struct object_id *merge_base)
> {
> - struct commit *head = lookup_commit(the_repository, head_oid);
> - struct commit_list *merge_bases = NULL;
> int res = 0;
>
> if (!head)
> 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);Do we even use `merge_bases` after this anymore? I guess not. So we should get rid of this function parameter at this point.
> if (!oideq(merge_base, &onto->object.oid))
Uh oh. With this patch, `merge_base` can be `NULL` at this point, namely when trying to rebase commits onto an unrelated history.
Show 19 quoted lines
> goto done;
>
> @@ -1154,6 +1160,8 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
> OPT_END(),
> };
> int i;
> + struct commit *head_commit;
> + struct commit_list *merge_bases = NULL;
>
> if (argc == 2 && !strcmp(argv[1], "-h"))
> usage_with_options(builtin_rebase_usage,
> @@ -1703,9 +1711,14 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
> * 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.upstream, options.restrict_revision,
> - &options.orig_head, &merge_base) &&
> - !is_interactive(&options)) {
> + head_commit = lookup_commit(the_repository, &options.orig_head);Hmm. I do not see this line (or any equivalent code) removed in this patch. Is it possible that we do not need to (re-?)initialize `head_commit` at this point?
I guess I should read the entire patch series because that `head_commit` isn't there in the worktree I am looking at (oh, if only I had an easy way to review code, but I have to review static patches instead, not even in a proper IDE... ;-)). Will try to find time to do that next week.
Ciao, Dscho
Show 14 quoted lines
> + if (head_commit)
> + populate_merge_bases(head_commit, options.onto, merge_bases,
> + &merge_base);
> + if (!is_interactive(&options) &&
> + can_fast_forward(head_commit, options.onto, options.upstream,
> + options.restrict_revision, &options.orig_head,
> + merge_bases, &merge_base)) {
> int flag;
>
> if (!(options.flags & REBASE_FORCE)) {
> --
> 2.21.0.1020.gf2820cf01a
>
>