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