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

From: Phillip Wood <phillip.wood123@gmail.com>
To: Junio C Hamano <gitster@pobox.com>,
	Phillip Wood via GitGitGadget <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org,
	Philippe Blain <levraiphilippeblain@gmail.com>,
	Denton Liu <liu.denton@gmail.com>,
	Johannes Schindelin <Johannes.Schindelin@gmx.de>,
	Phillip Wood <phillip.wood@dunelm.org.uk>
Subject: Re: [PATCH 3/5] rebase: factor out merge_base calculation
Date: Tue, 16 Aug 2022 14:50:05 +0100	[thread overview]
Message-ID: <c5b01472-7da9-6051-b127-1a8b8ddd2944@gmail.com> (raw)
In-Reply-To: <xmqqczd12zhv.fsf@gitster.g>

Hi Junio

On 15/08/2022 18:22, Junio C Hamano wrote:
> "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>
>> Separate out calculating the merge base between onto and head from the
>> check for whether we can fast-forward or not. This means we can skip
>> the fast-forward checks when the rebase is forced and avoid
>> calculating the merge-base twice when --keep-base is given.

I should clarify that this is referring to the merge base of onto and 
upstream.

>>
>> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
>> ---
>> Note the unnecessary braces around "if (keep_base)" are added here
>> reduce the churn on the next commit.
>> ---
>>   builtin/rebase.c | 35 +++++++++++++++++++++++------------
>>   1 file changed, 23 insertions(+), 12 deletions(-)
>>
>> diff --git a/builtin/rebase.c b/builtin/rebase.c
>> index 6cf9c95f4e1..86ea731ca3a 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(merge_base, null_oid());
>> +	if (is_null_oid(merge_base))
>>   		goto done;
>> -	}
>>   
>> -	oidcpy(merge_base, &merge_bases->item->object.oid);
>>   	if (!oideq(merge_base, &onto->object.oid))
>>   		goto done;
> 
> Looking at the change in "git show -W", it seems that this function
> no longer touches merge_bases at all, other than initializing it to
> NULL at the beginning and then calling free_commit_list() on it at
> the end.  Shouldn't it be removed?

There is still the line

	merge_bases = get_merge_bases(upstream, head);

lower down. I should remove the call to free_commit_list() just above 
that line though as it is no longer needed.

>> @@ -902,6 +898,20 @@ done:
>>   	return res && is_linear_history(onto, head);
>>   }
>>   
>> +static void fill_merge_base(struct rebase_options *options,
>> +			    struct object_id *merge_base)
>> +{
>> +	struct commit_list *merge_bases = NULL;
>> +
>> +	merge_bases = get_merge_bases(options->onto, options->orig_head);
>> +	if (!merge_bases || merge_bases->next)
>> +		oidcpy(merge_base, null_oid());
>> +	else
>> +		oidcpy(merge_base, &merge_bases->item->object.oid);
>> +
>> +	free_commit_list(merge_bases);
>> +}
>> +
>>   static int parse_opt_am(const struct option *opt, const char *arg, int unset)
>>   {
>>   	struct rebase_options *opts = opt->value;
>> @@ -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);

This is actually unnecessary as we do

	options.onto = lookup_commit_reference_by_name(options.onto_name);

above when calculating onto.

>> +	} else {
>> +		fill_merge_base(&options, &merge_base);
>> +	}
> 
> No need for braces around single-statement block on either side.
> 
> This is not a new issue introduced by this patch per-se, but
> "merge_base" is becoming less and less accurate description of what
> this variable really is.  Perhaps it is a good time to rename it?

Yes, merge_base is not a very descriptive name as it prompts the 
question "merge base of which commits". I think base_commit or 
branch_base would be better.

> It is "the base commit to rebuild the history on top of", aka "onto
> commit", isn't it?

I think it is the base commit of the branch i.e. we rebase all the 
commits in the range merge_base..branch onto the "onto commit".

> We often use merge-base between the upstream and
> our tip of the history for it,

I'm pretty sure it is always the merge-base of the "onto commit" and our 
tip of the history. When using --keep-base we calculate the "onto 
commit" as the merge base of upstream and our tip of the history which 
makes it look were using that for merge_base but that commit is also the 
merge-base of the "onto commit" and our tip of history.

Best Wishes

Phillip

> but the variable often does not even
> hold the merge-base in it, not because we cannot compute a single
> merge-base but because depending on the operation mode we do not
> want to use merge-base for further operation using that variable.