Re: [PATCH v5] submodule merge: update conflict error message

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

Calvin Wan <calvinwan@google.com> writes:

> @@ -1835,6 +1853,7 @@ static int merge_submodule(struct merge_options *opt,
>  			   "resolution exists: %s"),
>  			 path, sb.buf);
>  		strbuf_release(&sb);
> +		resolution_exists = 1;
>  		break;
>  	default:
>  		for (i = 0; i < merges.nr; i++)
> @@ -1845,10 +1864,22 @@ static int merge_submodule(struct merge_options *opt,
>  			 _("Failed to merge submodule %s, but multiple "
>  			   "possible merges exist:\n%s"), path, sb.buf);
>  		strbuf_release(&sb);
> +		resolution_exists = 1;
>  	}

These work on the result of calling find_first_merges(), but is it
possible that we are asked to call this function more than once
because we see conflicted submodule updates at two or more paths?

I may be misreading the code, but find_first_merges(), either the
version we see in this file, or the one in merge-recursive.c, or its
original introduced in 68d03e4a (Implement automatic fast-forward
merge for submodules, 2010-07-07), look safe to be called twice.  It
runs the get_revision() machinery, smudging the object flags while
walking the history, but I do not see any code that cleans up these
flags for the second traversal.

Also, this is not a new problem, but I am afraid that the logic to
find existing merges in find_first_merges() might be overly loose.
It tries to find existing merges that can reach the two commits, and
then finds, among these merges, the one that is not descendant of
any other such merges.  Don't we end up finding a merge M

    A---o---M
           /
          B

when a superproject merge needs a merge of A and B in the submodule?
That is certainly a merge that contains both A and B and it may be
closer to A and B than any other existing merges, but it still may
not be a merge between A and B (in the depicted case, an extra
commit 'o' nobody ordered is included for free in the result).  I am
not seeing how existing code tries to avoid such a situation.

Thanks.