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

From: Calvin Wan <calvinwan@google.com>
Date: Jul 19, 2022, 19:07 UTC
Message-ID: <CAFySSZDoNX_5mxdKaT8JWkpYxBFxivQc_p2q_4bQk6YsT8QeZA@mail.gmail.com>
In-Reply-To: <xmqqsfmxd17s.fsf@gitster.g>

> 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?

This does get called multiple times if we see conflicted submodule updates at two or more paths.

Show 7 quoted lines

> 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.

I don't quite understand which flags need to be cleaned up for the second traversal.

Show 16 quoted lines

> 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.

It is true that we find merge M and it isn't representative of a merge of A and B in the submodule. In this case, the existing code prints:

"Failed to merge submodule %s, but a possible merge resolution exists: %s"

While this part doesn't claim M to be a guaranteed merge resolution, my change adds this line:

"or update to an existing commit which has merged those changes such as one listed above"

Instead of adding more verbosity to this language, it seems like a better idea to remove "such as one listed above" entirely (and subsequently any of my code that flags merge resolutions).