Re: [PATCH v3 6/8] rebase: factor out branch_base calculation

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Oct 17, 2022, 11:23 UTC
Message-ID: <221017.86tu42k7x9.gmgdl@evledraar.gmail.com>
In-Reply-To: <c0213afe-658d-8562-3d2a-9e254c742730@dunelm.org.uk>

On Mon, Oct 17 2022, Phillip Wood wrote:

Show 17 quoted lines

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

Show 5 quoted lines

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