From: Phillip Wood <phillip.wood123@gmail.com>
Date: Mon, 17 Oct 2022 09:39:54 GMT
Subject: Re: [PATCH v3 6/8] rebase: factor out branch_base calculation
Message-ID: <c0213afe-658d-8562-3d2a-9e254c742730@dunelm.org.uk>
In-Reply-To: <221013.867d13ldzm.gmgdl@evledraar.gmail.com>

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?

> 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.
  	
> 	static void fill_branch_base(struct rebase_options *o, struct object_id *dst)
> 	{
> 		struct commit_list *mb = get_merge_bases(o->onto, o->orig_head);
> 		const struct object_id *src = (!mb || mb->next) ? null_oid() :
> 			&mb->item->object.oid;
> 	
> 		oidcpy(dst, src);
> 		free_commit_list(mb);
> 	}
> 
> 	
>> @@ -1669,8 +1678,8 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>>   		if (!options.onto)
>>   			die(_("Does not point to a valid commit '%s'"),
>>   				options.onto_name);
>> +		fill_branch_base(&options, &branch_base);
>>   	}
>> -
>>   	if (options.fork_point > 0)
>>   		options.restrict_revision =
>>   			get_fork_point(options.upstream_name, options.orig_head);
> 
> I wouldn't mind the stray whitespace change, but here it seems
> unintentional, in 7/8 your change on top is:

Thanks, well spotted, I'm sure I've fixed this at least once already, I 
must have reintroduced it when fixing a rebase conflict. I'll fix it again.

Best Wishes

Phillip
	
> 	@@ -1680,6 +1691,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
> 	 				options.onto_name);
> 	 		fill_branch_base(&options, &branch_base);
> 	 	}
> 	+	if (keep_base && options.reapply_cherry_picks)
> 	+		options.upstream = options.onto;
> 	+
> 	 	if (options.fork_point > 0)
> 	 		options.restrict_revision =
> 	 			get_fork_point(options.upstream_name, options.orig_head);
> 
> Presumably we want to have \n\n spacing for both of those, and to not
> remove the spacing here in 6/8, only to add it back?