Re: [PATCH 3/5] rebase: factor out merge_base calculation

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Aug 30, 2022, 15:03 UTC
Message-ID: <dc1b42fa-0420-6d15-03ab-60d34a42ed73@gmail.com>
In-Reply-To: <20220824220251.1682480-1-jonathantanmy@google.com>

Hi Jonathan

Thanks for taking a look at this series

On 24/08/2022 23:02, Jonathan Tan wrote:

Show 18 quoted lines

> "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
>> @@ -1668,7 +1678,11 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>>   			die(_("Does not point to a valid commit '%s'"),
>>   				options.onto_name);
>>   	}
>> -
>> +	if (keep_base) {
>> +		oidcpy(&merge_base, &options.onto->object.oid);
>> +	} else {
>> +		fill_merge_base(&options, &merge_base);
>> +	}
>>   	if (options.fork_point > 0)
>>   		options.restrict_revision =
>>   			get_fork_point(options.upstream_name, options.orig_head);
> 
> This patch makes sense, except for this part: why is fill_merge_base()
> only called for non-keep_base, but it seemed to be unconditionally
> called before? For what it's worth, all tests pass even with this diff:

It's an optimization, we have already calculated the merge base above in the "onto" calculation when --keep-base is given. This is what I meant by "avoid calculating the merge-base twice when --keep-base is given" in the commit message, maybe I should try and come up with some clearer wording.

Best Wishes

Phillip

Show 6 quoted lines

>    -       if (keep_base) {
>    -               oidcpy(&merge_base, &options.onto->object.oid);
>    -       } else {
>    -               fill_merge_base(&options, &merge_base);
>    -       }
>    +       fill_merge_base(&options, &merge_base);