Jeff King <peff@xxxxxxxx> writes:
> Oops, I totally missed the loop around the call to real_merge(). So
> yeah, I think this is rather tricky.
> ...
> 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):
If we have no plan and intention to extend "merge-tree" even more in
the future, then option 1 would be the approach with least patch
noise, and as your "something like this" shows, it is a nice and
clean solution. I very much like it.
But as the renovated "merge-tree" is a relatively young thing in our
toolbox, I suspect that more and more work may want to go into it.
And the other "official copy_merge_options()" approach would be a
more healthy solution in the longer run, I would think. If we were
to go that route, we should also give an interface to free the
resources held by the copy.
It is not that much code on top of the commit that is already queued
in 'next', I suspect. Perhaps something like this?
builtin/merge-tree.c | 4 +++-
merge-recursive.c | 16 ++++++++++++++++
merge-recursive.h | 3 +++
3 files changed, 22 insertions(+), 1 deletion(-)
diff --git c/builtin/merge-tree.c w/builtin/merge-tree.c
index 7024b5ce2e..a35e0452d6 100644
--- c/builtin/merge-tree.c
+++ w/builtin/merge-tree.c
@@ -425,10 +425,11 @@ 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_result result = { 0 };
int show_messages = o->show_messages;
+ struct merge_options opt;
+ copy_merge_options(&opt, &o->merge_options);
parent1 = get_merge_parent(branch1);
if (!parent1)
help_unknown_ref(branch1, "merge-tree",
@@ -507,6 +508,7 @@ static int real_merge(struct merge_tree_options *o,
if (o->use_stdin)
putchar(line_termination);
merge_finalize(&opt, &result);
+ clear_merge_options(&opt);
return !result.clean; /* result.clean < 0 handled above */
}
diff --git c/merge-recursive.c w/merge-recursive.c
index 0d7e57e2df..e3beb0801b 100644
--- c/merge-recursive.c
+++ w/merge-recursive.c
@@ -3912,6 +3912,22 @@ void init_merge_options(struct merge_options *opt,
opt->buffer_output = 0;
}
+/*
+ * For now, members of merge_options do not need deep copying, but
+ * it may change in the future, in which case we would need to update
+ * this, and also make a matching change to clear_merge_options() to
+ * release the resources held by a copied instance.
+ */
+void copy_merge_options(struct merge_options *dst, struct merge_options *src)
+{
+ *dst = *src;
+}
+
+void clear_merge_options(struct merge_options *opt UNUSED)
+{
+ ; /* no-op as our copy is shallow right now */
+}
+
int parse_merge_opt(struct merge_options *opt, const char *s)
{
const char *arg;
diff --git c/merge-recursive.h w/merge-recursive.h
index b88000e3c2..3d3b3e3c29 100644
--- c/merge-recursive.h
+++ w/merge-recursive.h
@@ -55,6 +55,9 @@ struct merge_options {
void init_merge_options(struct merge_options *opt, struct repository *repo);
+void copy_merge_options(struct merge_options *dst, struct merge_options *src);
+void clear_merge_options(struct merge_options *opt);
+
/* parse the option in s and update the relevant field of opt */
int parse_merge_opt(struct merge_options *opt, const char *s);
