Re: [PATCH v6] merge-tree: add -X strategy option

From: Jeff King <peff@peff.net>
Date: Oct 9, 2023, 18:52 UTC
Message-ID: <20231009185234.GB3270793@coredump.intra.peff.net>
In-Reply-To: <xmqq4jiz3env.fsf@gitster.g>

On Mon, Oct 09, 2023 at 10:10:28AM -0700, Junio C Hamano wrote:

Show 22 quoted lines

> Jeff King <peff@peff.net> writes:
> 
> > I agree that struct-copying is an unusual pattern, and we'd potentially
> > run into problems with duplication. But I think it is even trickier than
> > that here. We also go on to actually _modify_ opt in this function,
> > assigning to various members (both directly, and I think the merge code
> > itself will write to opt->priv).
> >
> > So if we use a pointer (rather than struct assignment), those changes
> > will persist in the merge_options struct that was passed in. Which is
> > also weird.
> >
> > Between the two, I think using a pointer is probably the least-weird.
> > This real_merge() function is only called once, and is a static-local
> > helper for cmd_merge_tree(). So the two functions work as a single unit,
> > and munging "opt" is not a big deal.
> 
> It is called once per --stdin input to perform many merges in a row.
> The most obvious "structure to pointer to structure" conversion
> below seems to break an assertion (which is not very surprising, as
> it happens inside that --stdin loop), so I am tempted to revert the
> whole thing for now.

Oops, I totally missed the loop around the call to real_merge(). So yeah, I think this is rather tricky.

Before Izzy's patch, real_merge() always makes its own fresh merge_options. After, we have a template merge_options that we copy, but we are assuming that a shallow struct copy is OK (probably true, but an anti-pattern that may bite us later). If we add Phillip's suggestion on top, then we do not copy at all, and end up reusing the same options struct (which is definitely wrong).

I don't think there are any bugs with the state at the current tip of ty/merge-tree-strategy-options, but if we want to make it safer, I think we have two options:

  - delay the conversion of the "xopts" list into merge_options until we
    initialize it in real_merge(). This avoids breaking abstraction
    boundaries, but does mean that the sanity-checking of the options
    happens a little later (but not much in practice).

  - provide a copy_merge_options() function, which makes this kind of
    "set up a template and then copy it" pattern official. It can be a
    struct assignment for now, but it at least alerts anybody adding new
    options to the notion that a deep copy might be required.

Option 1 looks something like this (a lot of the hunks are just reverting the tip of that branch, so squashed in it would be even smaller):

diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c
index 7024b5ce2e..f9dbbdb867 100644
--- a/builtin/merge-tree.c
+++ b/builtin/merge-tree.c
@@ -415,7 +415,7 @@ struct merge_tree_options {
 	int show_messages;
 	int name_only;
 	int use_stdin;
-	struct merge_options merge_options;
+	struct strvec xopts;
 };
 
 static int real_merge(struct merge_tree_options *o,
@@ -425,7 +425,7 @@ static int real_merge(struct merge_tree_options *o,
 {
 	struct commit *parent1, *parent2;
 	struct commit_list *merge_bases = NULL;
-	struct merge_options opt = o->merge_options;
+	struct merge_options opt;
 	struct merge_result result = { 0 };
 	int show_messages = o->show_messages;
 
@@ -439,11 +439,17 @@ static int real_merge(struct merge_tree_options *o,
 		help_unknown_ref(branch2, "merge-tree",
 				 _("not something we can merge"));
 
+	init_merge_options(&opt, the_repository);
+
 	opt.show_rename_progress = 0;
 
 	opt.branch1 = branch1;
 	opt.branch2 = branch2;
 
+	for (size_t x = 0; x < o->xopts.nr; x++)
+		if (parse_merge_opt(&opt, o->xopts.v[x]))
+			die(_("unknown strategy option: -X%s"), o->xopts.v[x]);
+
 	if (merge_base) {
 		struct commit *base_commit;
 		struct tree *base_tree, *parent1_tree, *parent2_tree;
@@ -512,8 +518,7 @@ static int real_merge(struct merge_tree_options *o,
 
 int cmd_merge_tree(int argc, const char **argv, const char *prefix)
 {
-	struct merge_tree_options o = { .show_messages = -1 };
-	struct strvec xopts = STRVEC_INIT;
+	struct merge_tree_options o = { .show_messages = -1,.xopts = STRVEC_INIT };
 	int expected_remaining_argc;
 	int original_argc;
 	const char *merge_base = NULL;
@@ -549,24 +554,18 @@ int cmd_merge_tree(int argc, const char **argv, const char *prefix)
 			   &merge_base,
 			   N_("commit"),
 			   N_("specify a merge-base for the merge")),
-		OPT_STRVEC('X', "strategy-option", &xopts, N_("option=value"),
+		OPT_STRVEC('X', "strategy-option", &o.xopts, N_("option=value"),
 			N_("option for selected merge strategy")),
 		OPT_END()
 	};
 
-	/* Init merge options */
-	init_merge_options(&o.merge_options, the_repository);
-
 	/* Parse arguments */
 	original_argc = argc - 1; /* ignoring argv[0] */
 	argc = parse_options(argc, argv, prefix, mt_options,
 			     merge_tree_usage, PARSE_OPT_STOP_AT_NON_OPTION);
 
-	if (xopts.nr && o.mode == MODE_TRIVIAL)
+	if (o.xopts.nr && o.mode == MODE_TRIVIAL)
 		die(_("--trivial-merge is incompatible with all other options"));
-	for (int x = 0; x < xopts.nr; x++)
-		if (parse_merge_opt(&o.merge_options, xopts.v[x]))
-			die(_("unknown strategy option: -X%s"), xopts.v[x]);
 
 	/* Handle --stdin */
 	if (o.use_stdin) {