"Phillip Wood via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes:
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index dd5e0e1feb6..b5c78ce1fb0 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());
> + if (is_null_oid(branch_base))
> goto done;
Naïvely, I would have expected the condition of branch_base being
null to mean "the caller doesn't have branch base specified (like
parsing --onto), so we would need to compute one" and this
"branch_base is null, so leave without doing anything" looked
confusing at first.
After computing options.onto_name (given either with --onto or
falling back to .upstream_name), we make sure branch_base is
correctly filled, either from an explicit A...B notation or a single
committish object name given and converted with a call to
fill_branch_base(). And fill_branch_base() uses null object name as
a signal that it punted due to multiple merge bases.
> +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);
> +}
So it is doing the same thing as the original, but unlike the
original, it is not immediately obvious why "goto done" is the right
thing to do here. Perhaps it deserves a comment, e.g.
if (is_null_oid(branch_base))
goto done; /* fill_branch_base() already checked and punted */
or something.
Thanks.
