Re: [PATCH v2 2/2] merge: remember conflict labels

[Phillip Wood <phillip.wood123@gmail.com>]

On 05/10/2026 17:31, Junio C Hamano wrote:
> Phillip Wood <phillip.wood123@gmail.com> writes:
> 
>> Note that merge_switch_to_result()
>> we assign "result->priv" to "opt->priv" and later clear "opt->priv" in
>> order to get a pointer to the private struct as result->priv is void*.
> 
> I missed this part.
> 
>>   		trace2_region_leave("merge", "write_auto_merge", opt->repo);
>> +
>> +		trace2_region_enter("merge", "write_merge_labels", opt->repo);
>> +		opt->priv = result->priv;
>> +		write_merge_labels(opt->repo, opt->priv->labels[0], opt->priv->labels[1],
>> +				   opt->priv->labels[2]);
>> +		opt->priv = NULL;
>> +		trace2_region_leave("merge", "write_merge_labels", opt->repo);
> 
> Would it be better to do it this way instead?
> 
> 	struct merge_options_internal *priv = result->priv;
> 	write_merge_labels(opt->repo,
> 			   priv->labels[0], priv->labels[1], priv->labels[2]);

Yes, maybe we should have a preparatory commit that adds that "priv" 
variable and updates the existing code that does the same dance. Another 
option would be to make result->priv a pointer to an opaque struct like 
opts->priv - I don't really see any advantage in keeping it as a void*.

> Also, with the way merge labels are prepared and passed around, I
> wonder if we should just tighten its function signature and take
> 
> 	write_merge_labels(struct repository *repo, const char *labels[3])
> 
> so that this calling site becomes[*]
> 
> 	struct merge_options_internal *priv = result->priv;
> 	write_merge_labels(opt->repo, priv->labels);

That's a nice idea

Thanks

Phillip

> 
> 
> [Footnote]
> 
>   * Here, I deviate from the usual naming convention to call an array
>     of things in singular (so the second label would become
>     label[2]), because from the point of view of the API consumer,
>     "labels" as a unit is what they pass around, and call it in
>     plural.