Re: [PATCH v2 5/7] rebase: factor out branch_base calculation

[Junio C Hamano <gitster@pobox.com>]

"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> 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.