From: Phillip Wood Date: Tue, 06 Oct 2026 15:05:41 GMT Subject: Re: [PATCH v2 2/2] merge: remember conflict labels Message-ID: <9f3d7277-e038-47d6-8554-175178e911cf@gmail.com> In-Reply-To: On 05/10/2026 17:31, Junio C Hamano wrote: > Phillip Wood 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.