Re: [PATCH v3 04/24] merge-recursive: provide a better label for diff3 common ancestor

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

Elijah Newren <newren@gmail.com> writes:

> @@ -3507,6 +3507,11 @@ int merge_recursive(struct merge_options *opt,
>  	struct commit *merged_common_ancestors;
>  	struct tree *mrtree;
>  	int clean;
> +	int num_merge_bases;
> +	struct strbuf merge_base_abbrev = STRBUF_INIT;
> +
> +	if (!opt->call_depth)
> +		assert(opt->ancestor == NULL);

Hmph.  Do we have anything to say on this field when call_depth is
not zero?  Is it OK for opt->ancestor to be sometimes NULL and non
NULL some other times?

> @@ -3528,6 +3533,7 @@ int merge_recursive(struct merge_options *opt,
>  			output_commit_title(opt, iter->item);
>  	}
>  
> +	num_merge_bases = commit_list_count(ca);

Criss-cross merge with very large number of merge bases is rare, so
it is OK to count them all, even though we only care about "is it
zero, is it one, or is it two or more?"

I suspect this does not have to count, though, if we really wanted
to avoid counting.

>  	merged_common_ancestors = pop_commit(&ca);
>  	if (merged_common_ancestors == NULL) {
>  		/* if there is no common ancestor, use an empty tree */

Here is the case where we can already decide the ancestor name for
the later merge_trees() should be "empty tree".

And if merged_common_ancestors is not NULL, ca may have run out (in
which case, we only have a single merge base), or ca still has
another merge base (in which case, we have two or more).  So, if you
add
		ancestor_name = "empty tree";
	} else if (ca) {
		ancestor_name = "merged common ancestors";
	} else {
		ancestor_name = abbrev_name(merged_common_ancestors);
	}

to that if() statement above, that should be sufficient, no?

opt is used for inner merge in the for() loop, so you would probably
need another "char *" variable without contaminating opt->ancestor_name
at this point, and then assign the value in the temporary to the
opt->ancestor field where the original always assigned "merged
common ancestors".

> @@ -3568,10 +3574,23 @@ int merge_recursive(struct merge_options *opt,
>  	if (!opt->call_depth)
>  		repo_read_index(opt->repo);
>  
> -	opt->ancestor = "merged common ancestors";
> +	switch (num_merge_bases) {
> +	case 0:
> +		opt->ancestor = "<empty tree>";

Also, I do not see a reason why you want angle bra-ket pair around
"empty tree".  You are already using "merged common ancestors"
literal phrase without any special marker syntax.

Thanks.