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

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 7, 2022, 18:12 UTC
Message-ID: <xmqqo7vruk92.fsf@gitster.g>
In-Reply-To: <fc45b996d34af43f8e333f7c43a8b06489a1fda2.1662561470.git.gitgitgadget@gmail.com>

"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 13 quoted lines

> 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.

Show 13 quoted lines

> +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.