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.
Thanks.
git: merge-ort.c:5110: merge_incore_recursive: Assertion `opt->ancestor == NULL' failed.
./test-lib.sh: line 1067: 738791 Done printf "c1 c3\nc2 -- c1 c3\nc2 c3"
738792 Aborted | git -C repo merge-tree --stdin > actual
builtin/merge-tree.c | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git c/builtin/merge-tree.c w/builtin/merge-tree.c
index 7024b5ce2e..1cb1fba2de 100644
--- c/builtin/merge-tree.c
+++ w/builtin/merge-tree.c
@@ -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 = &o->merge_options;
struct merge_result result = { 0 };
int show_messages = o->show_messages;
@@ -439,10 +439,10 @@ static int real_merge(struct merge_tree_options *o,
help_unknown_ref(branch2, "merge-tree",
_("not something we can merge"));
- opt.show_rename_progress = 0;
+ opt->show_rename_progress = 0;
- opt.branch1 = branch1;
- opt.branch2 = branch2;
+ opt->branch1 = branch1;
+ opt->branch2 = branch2;
if (merge_base) {
struct commit *base_commit;
@@ -452,11 +452,11 @@ static int real_merge(struct merge_tree_options *o,
if (!base_commit)
die(_("could not lookup commit '%s'"), merge_base);
- opt.ancestor = merge_base;
+ opt->ancestor = merge_base;
base_tree = repo_get_commit_tree(the_repository, base_commit);
parent1_tree = repo_get_commit_tree(the_repository, parent1);
parent2_tree = repo_get_commit_tree(the_repository, parent2);
- merge_incore_nonrecursive(&opt, base_tree, parent1_tree, parent2_tree, &result);
+ merge_incore_nonrecursive(opt, base_tree, parent1_tree, parent2_tree, &result);
} else {
/*
* Get the merge bases, in reverse order; see comment above
@@ -467,7 +467,7 @@ static int real_merge(struct merge_tree_options *o,
if (!merge_bases && !o->allow_unrelated_histories)
die(_("refusing to merge unrelated histories"));
merge_bases = reverse_commit_list(merge_bases);
- merge_incore_recursive(&opt, merge_bases, parent1, parent2, &result);
+ merge_incore_recursive(opt, merge_bases, parent1, parent2, &result);
}
if (result.clean < 0)
@@ -501,12 +501,12 @@ static int real_merge(struct merge_tree_options *o,
}
if (show_messages) {
putchar(line_termination);
- merge_display_update_messages(&opt, line_termination == '\0',
+ merge_display_update_messages(opt, line_termination == '\0',
&result);
}
if (o->use_stdin)
putchar(line_termination);
- merge_finalize(&opt, &result);
+ merge_finalize(opt, &result);
return !result.clean; /* result.clean < 0 handled above */
}
