| Message ID | 20191001065849.29491-1-newren@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] merge-recursive: fix the diff3 common ancestor label for virtual commits | expand |
On Mon, Sep 30, 2019 at 11:58:49PM -0700, Elijah Newren wrote: > In 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: > > >=2: "merged common ancestors" > 1: <abbreviated commit hash> > 0: "<empty tree>" > > Unfortunately, this did not take into account that when we have a single > merge base, that merge base could be fake or constructed. In such > cases, this resulted in a label of "00000000". Of course, the previous > label of "merged common ancestors" was also misleading for this case. Yeah, I agree the original was not great, either, but the "0000000" looked like a bug to me. Hey, at least we didn't segfault! :) > Since we have an API that is explicitly about creating fake merge base > commits in merge_recursive_generic(), we should provide a better label > when using that API with one merge base. So, when > merge_recursive_generic() is called with one merge base, set the label > to: > > "constructed merge base" That makes perfect sense to me. Thanks for the quick turnaround. > Changes since v1: > - We only had a problem if the number of fake merge bases was exactly > one; update the patch to check for that and update the commit message > accordingly. That makes sense. We'd still want to say "merged common ancestors" even if one of those ancestors was fake. > merge-recursive.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) The patch itself looks good to me (though admittedly I'm not too familiar with this area). Maybe worth squashing in this test? diff --git a/t/t6047-diff3-conflict-markers.sh b/t/t6047-diff3-conflict-markers.sh index 3fb68e0aae..860542aad0 100755 --- a/t/t6047-diff3-conflict-markers.sh +++ b/t/t6047-diff3-conflict-markers.sh @@ -186,4 +186,17 @@ test_expect_success 'check multiple merge bases' ' ) ' +test_expect_success 'rebase describes fake ancestor base' ' + test_create_repo rebase && + ( + cd rebase && + test_commit base file && + test_commit master file && + git checkout -b side HEAD^ && + test_commit side file && + test_must_fail git -c merge.conflictstyle=diff3 rebase master && + grep "||||||| constructed merge base" file + ) +' + test_done -Peff
diff --git a/merge-recursive.c b/merge-recursive.c index b058741f00..e12d91f48a 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -3550,6 +3550,8 @@ 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) { + ancestor_name = opt->ancestor; } else if (merge_bases) { ancestor_name = "merged common ancestors"; } else { @@ -3689,7 +3691,8 @@ int merge_recursive(struct merge_options *opt, { int clean; - assert(opt->ancestor == NULL); + assert(opt->ancestor == NULL || + !strcmp(opt->ancestor, "constructed merge base")); if (merge_start(opt, repo_get_commit_tree(opt->repo, h1))) return -1; @@ -3741,6 +3744,8 @@ int merge_recursive_generic(struct merge_options *opt, oid_to_hex(merge_bases[i])); commit_list_insert(base, &ca); } + if (num_merge_bases == 1) + opt->ancestor = "constructed merge base"; } repo_hold_locked_index(opt->repo, &lock, LOCK_DIE_ON_ERROR);
In 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: >=2: "merged common ancestors" 1: <abbreviated commit hash> 0: "<empty tree>" Unfortunately, this did not take into account that when we have a single merge base, that merge base could be fake or constructed. In such cases, this resulted in a label of "00000000". Of course, the previous label of "merged common ancestors" was also misleading for this case. Since we have an API that is explicitly about creating fake merge base commits in merge_recursive_generic(), we should provide a better label when using that API with one merge base. So, when merge_recursive_generic() is called with one merge base, set the label to: "constructed merge base" Note that callers of merge_recursive_generic() include the builtin commands git-am (in combination with git apply --build-fake-ancestor), git-merge-recursive, and git-stash. Reported-by: Jeff King <peff@peff.net> Signed-off-by: Elijah Newren <newren@gmail.com> --- Applies to the top of en/merge-recursive-cleanup, which is in next. Changes since v1: - We only had a problem if the number of fake merge bases was exactly one; update the patch to check for that and update the commit message accordingly. Range-diff: 1: e3b5015985 ! 1: 3f4444bfd7 merge-recursive: fix the diff3 common ancestor label for virtual commits @@ Commit message Unfortunately, this did not take into account that when we have a single merge base, that merge base could be fake or constructed. In such cases, this resulted in a label of "00000000". Of course, the previous - label of "merged common ancestors" was also misleading for these cases. - Since we have an API that is explicitly about creating fake commits in - merge_recursive_generic(), we should provide a better label when using - that API. So, when merge_recursive_generic() is called, set the label + label of "merged common ancestors" was also misleading for this case. + Since we have an API that is explicitly about creating fake merge base + commits in merge_recursive_generic(), we should provide a better label + when using that API with one merge base. So, when + merge_recursive_generic() is called with one merge base, set the label to: "constructed merge base" - Note that users of merge_recursive_generic include the builtin commands - git-am (in combination with git apply --build-fake-ancestor), + Note that callers of merge_recursive_generic() include the builtin + commands git-am (in combination with git apply --build-fake-ancestor), git-merge-recursive, and git-stash. Reported-by: Jeff King <peff@peff.net> @@ merge-recursive.c: int merge_recursive_generic(struct merge_options *opt, oid_to_hex(merge_bases[i])); commit_list_insert(base, &ca); } -+ opt->ancestor = "constructed merge base"; ++ if (num_merge_bases == 1) ++ opt->ancestor = "constructed merge base"; } repo_hold_locked_index(opt->repo, &lock, LOCK_DIE_ON_ERROR); merge-recursive.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)