On Mon, Oct 09, 2023 at 10:10:28AM -0700, Junio C Hamano wrote:
> Jeff King <peff@xxxxxxxx> 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) {
