| Message ID | 20191007155211.23067-1-newren@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | merge-recursive: fix the fix to the diff3 common ancestor label | expand |
Elijah Newren <newren@gmail.com> writes: > This resulted in garbage strings being printed for the virtual merge > bases, which was visible in git.git by just merging commit b744c3af07 > into commit 6d8cb22a4f. There are two ways to fix this: set > opt->ancestor to NULL after using it to avoid re-use, or add a > !opt->priv->call_depth check to the if block for using a pre-defined > opt->ancestor. Apply both fixes. Thanks for quickly fixing this. Will apply. > Signed-off-by: Elijah Newren <newren@gmail.com> > --- > merge-recursive.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/merge-recursive.c b/merge-recursive.c > index e12d91f48a..2653ba9a50 100644 > --- a/merge-recursive.c > +++ b/merge-recursive.c > @@ -3550,7 +3550,7 @@ static int merge_recursive_internal(struct merge_options *opt, > merged_merge_bases = make_virtual_commit(opt->repo, tree, > "ancestor"); > ancestor_name = "empty tree"; > - } else if (opt->ancestor) { > + } else if (opt->ancestor && !opt->priv->call_depth) { > ancestor_name = opt->ancestor; > } else if (merge_bases) { > ancestor_name = "merged common ancestors"; > @@ -3600,6 +3600,7 @@ static int merge_recursive_internal(struct merge_options *opt, > merged_merge_bases), > &result_tree); > strbuf_release(&merge_base_abbrev); > + opt->ancestor = NULL; /* avoid accidental re-use of opt->ancestor */ > if (clean < 0) { > flush_output(opt); > return clean;
Elijah Newren <newren@gmail.com> writes: > In commit 208e69a4ebce ("merge-recursive: fix the diff3 common ancestor I think the above was an earlier incarntion of what is now known as 8e4ec337 ("merge-recursive: fix the diff3 common ancestor label for virtual commits", 2019-10-01). > label for virtual commits", 2019-09-30) which was a fix to commit > 743474cbfa8b ("merge-recursive: provide a better label for diff3 common > ... > The handling for "constructed merge base" worked by allowing > opt->ancestor to be set in merge_recursive_generic(), so we payed s/payed/paid/ > attention to the setting of that variable in merge_recursive_internal(). Thanks, again.
On Mon, Oct 7, 2019 at 7:36 PM Junio C Hamano <gitster@pobox.com> wrote: > > Elijah Newren <newren@gmail.com> writes: > > > In commit 208e69a4ebce ("merge-recursive: fix the diff3 common ancestor > > I think the above was an earlier incarntion of what is now known as > 8e4ec337 ("merge-recursive: fix the diff3 common ancestor label for > virtual commits", 2019-10-01). Oops, yes. > > label for virtual commits", 2019-09-30) which was a fix to commit > > 743474cbfa8b ("merge-recursive: provide a better label for diff3 common > > ... > > The handling for "constructed merge base" worked by allowing > > opt->ancestor to be set in merge_recursive_generic(), so we payed > > s/payed/paid/ Ugh, two simple mistakes in the commit message. I see you've not only proofread the commit but fixed up the commit message for me in pu; thanks. Elijah
diff --git a/merge-recursive.c b/merge-recursive.c index e12d91f48a..2653ba9a50 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -3550,7 +3550,7 @@ static int merge_recursive_internal(struct merge_options *opt, merged_merge_bases = make_virtual_commit(opt->repo, tree, "ancestor"); ancestor_name = "empty tree"; - } else if (opt->ancestor) { + } else if (opt->ancestor && !opt->priv->call_depth) { ancestor_name = opt->ancestor; } else if (merge_bases) { ancestor_name = "merged common ancestors"; @@ -3600,6 +3600,7 @@ static int merge_recursive_internal(struct merge_options *opt, merged_merge_bases), &result_tree); strbuf_release(&merge_base_abbrev); + opt->ancestor = NULL; /* avoid accidental re-use of opt->ancestor */ if (clean < 0) { flush_output(opt); return clean;
In commit 208e69a4ebce ("merge-recursive: fix the diff3 common ancestor label for virtual commits", 2019-09-30) which was a fix to commit 743474cbfa8b ("merge-recursive: provide a better label for diff3 common ancestor", 2019-08-17), the label for the common ancestor was changed from always being "merged common ancestors" to instead be based on the number of merge bases and whether the merge base was a real commit or a virtual one: >=2: "merged common ancestors" 1, via merge_recursive_generic: "constructed merge base" 1, otherwise: <abbreviated commit hash> 0: "<empty tree>" The handling for "constructed merge base" worked by allowing opt->ancestor to be set in merge_recursive_generic(), so we payed attention to the setting of that variable in merge_recursive_internal(). Now, for the outer merge, the code flow was simply the following: ancestor_name = "merged merge bases" loop over merge_bases: merge_recursive_internal() The first merge base not needing recursion would determine its own ancestor_name however necessary and thus run ancestor_name = $SOMETHING empty loop over merge_bases... opt->ancestor = ancestor_name merge_trees_internal() Now, the next set of merge_bases that would need to be merged after this particular merge had completed would note that opt->ancestor has been set to something (to a local ancestor_name variable that has since been popped off the stack), and thus it would run: ... else if (opt->ancestor) { ancestor_name = opt->ancestor; /* OOPS! */ loop over merge_bases: merge_recursive_internal() opt->ancestor = ancestor_name merge_trees_internal() This resulted in garbage strings being printed for the virtual merge bases, which was visible in git.git by just merging commit b744c3af07 into commit 6d8cb22a4f. There are two ways to fix this: set opt->ancestor to NULL after using it to avoid re-use, or add a !opt->priv->call_depth check to the if block for using a pre-defined opt->ancestor. Apply both fixes. Signed-off-by: Elijah Newren <newren@gmail.com> --- merge-recursive.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)