Re: [PATCH v2 2/2] merge: remember conflict labels
On 05/10/2026 17:31, Junio C Hamano wrote:
Show 17 quoted lines
> 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*.
Show 6 quoted lines
> 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
Show 6 quoted lines
> > > [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.