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

[Junio C Hamano <gitster@pobox.com>]

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.

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 */
 }