Hi Elijah,
On Thu, 25 Jul 2019, Elijah Newren wrote:
> In commit 7ca56aa07619 ("merge-recursive: add a label for ancestor",
> 2010-03-20), a label was added for the '||||||' line to make it have
> the more informative heading '|||||| merged common ancestors', with
> the statement:
>
> It would be nicer to use a more informative label. Perhaps someone
> will provide one some day.
>
> This chosen label was perfectly reasonable when recursiveness kicks in,
> i.e. when there are multiple merge bases. (I can't think of a better
> label in such cases.) But it is actually somewhat misleading when there
> is a unique merge base or no merge base. Change this based on the
> number of merge bases:
> >=2: "merged common ancestors"
> 1: <abbreviated commit hash>
> 0: "<empty tree>"
Nice!
> Also, the code in merge_3way making use of opt->ancestor was overly
> complex because it tried to handle the case of opt->ancestor being NULL.
> We always set it first, though, so just add an assert that opt->ancestor
> is not NULL and simplify the surrounding code.
>
> Tests have also been added to check that we get the right ancestor name
> for each of the three cases.
>
> Signed-off-by: Elijah Newren <newren@xxxxxxxxx>
> ---
> merge-recursive.c | 35 ++++--
> t/t6036-recursive-corner-cases.sh | 8 +-
> t/t6047-diff3-conflict-markers.sh | 191 ++++++++++++++++++++++++++++++
> 3 files changed, 220 insertions(+), 14 deletions(-)
> create mode 100755 t/t6047-diff3-conflict-markers.sh
>
> diff --git a/merge-recursive.c b/merge-recursive.c
> index 4cd6599296..8ac53cacdf 100644
> --- a/merge-recursive.c
> +++ b/merge-recursive.c
> @@ -1034,7 +1034,7 @@ static int merge_3way(struct merge_options *opt,
> {
> mmfile_t orig, src1, src2;
> struct ll_merge_options ll_opts = {0};
> - char *base_name, *name1, *name2;
> + char *base, *name1, *name2;
Renaming this variable at the same time as doing other, more involved
changes, made this patch a bit harder to review for me, I must admit.
> int merge_status;
>
> ll_opts.renormalize = opt->renormalize;
> @@ -1058,16 +1058,13 @@ static int merge_3way(struct merge_options *opt,
> }
> }
>
> - assert(a->path && b->path && o->path);
> - if (strcmp(a->path, b->path) ||
> - (opt->ancestor != NULL && strcmp(a->path, o->path) != 0)) {
> - base_name = opt->ancestor == NULL ? NULL :
> - mkpathdup("%s:%s", opt->ancestor, o->path);
> + assert(a->path && b->path && o->path && opt->ancestor);
> + if (strcmp(a->path, b->path) || strcmp(a->path, o->path) != 0) {
> + base = mkpathdup("%s:%s", opt->ancestor, o->path);
> name1 = mkpathdup("%s:%s", branch1, a->path);
> name2 = mkpathdup("%s:%s", branch2, b->path);
> } else {
> - base_name = opt->ancestor == NULL ? NULL :
> - mkpathdup("%s", opt->ancestor);
> + base = mkpathdup("%s", opt->ancestor);
> name1 = mkpathdup("%s", branch1);
> name2 = mkpathdup("%s", branch2);
> }
> @@ -1076,11 +1073,11 @@ static int merge_3way(struct merge_options *opt,
> read_mmblob(&src1, &a->oid);
> read_mmblob(&src2, &b->oid);
>
> - merge_status = ll_merge(result_buf, a->path, &orig, base_name,
> + merge_status = ll_merge(result_buf, a->path, &orig, base,
> &src1, name1, &src2, name2,
> opt->repo->index, &ll_opts);
>
> - free(base_name);
> + free(base);
> free(name1);
> free(name2);
> free(orig.ptr);
> @@ -3517,6 +3514,8 @@ static int merge_recursive_internal(struct merge_options *opt,
> struct commit *merged_merge_bases;
> struct tree *result_tree;
> int clean;
> + int num_merge_bases;
> + struct strbuf commit_name = STRBUF_INIT;
>
> if (show(opt, 4)) {
> output(opt, 4, _("Merging:"));
> @@ -3538,6 +3537,7 @@ static int merge_recursive_internal(struct merge_options *opt,
> output_commit_title(opt, iter->item);
> }
>
> + num_merge_bases = commit_list_count(merge_bases);
At first, I thought that this does not require a separate variable
because it is used exactly once.
But then I realized that the next line changes the number of commits in
`merge_bases`, so it actually _is_ required.
> merged_merge_bases = pop_commit(&merge_bases);
> if (merged_merge_bases == NULL) {
> /* if there is no common ancestor, use an empty tree */
> @@ -3579,13 +3579,26 @@ static int merge_recursive_internal(struct merge_options *opt,
> if (!opt->priv->call_depth)
> repo_read_index(opt->repo);
>
> - opt->ancestor = "merged common ancestors";
> + switch (num_merge_bases) {
> + case 0:
> + opt->ancestor = "<empty tree>";
> + break;
> + case 1:
> + strbuf_add_unique_abbrev(&commit_name,
The name `commit_name` would suggest a different usage to me. How about
`pretty_merge_base`?
> + &merged_merge_bases->object.oid,
> + DEFAULT_ABBREV);
> + opt->ancestor = commit_name.buf;
> + break;
> + default:
> + opt->ancestor = "merged common ancestors";
> + }
> clean = merge_trees_internal(opt,
> repo_get_commit_tree(opt->repo, h1),
> repo_get_commit_tree(opt->repo, h2),
> repo_get_commit_tree(opt->repo,
> merged_merge_bases),
> &result_tree);
> + strbuf_release(&commit_name);
> if (clean < 0) {
> flush_output(opt);
> return clean;
I was a bit too tired to look more closely at the test cases, in
particular after seeing the enormous complexity of the added test
script. I wonder whether it really has to be all that complex (e.g. why
use a complicated `test_seq` when a `test_commit A A.t
"1${LF}2${LF}3${LF}A${LF}` would suffice? Why start by `git checkout
--orphan` on a just-created repository?)
But otherwise, the patch series looks pretty good to me.
Thanks,
Dscho
