Blame correctly in the presence of conflicting merges

Problem:
The BlameGenerator used the RevFlag SEEN to mark commits it had
already looked at (but not necessarily processed), to prevent
processing a commit multiple times. If a commit is a conflicting
merge that contains lines of the merge base, that have been deleted
in its first parent, either these lines or the lines untouched
since the merge base would not be blamed properly.

This happens for example if a file is modified on a main branch in an
earlier commit M and on a side branch in a later commit S. For this
example, M deletes some lines relative to the common base commit B,
and S modifies a subset of these lines, leaving some other of these
lines untouched.

Then side is merged into main, creating a conflict for these
lines. The merge resolution shall carry over some unmodified lines
from B that would otherwise be deleted by M. The route to blame
these lines is via S to B. They can't be blamed via M, as they
don't exist there anymore.

    Q
    |\
    | \
    | S
    | |
    M |
    | /
    |/
    B

Blaming the merged file first blames via S, because that is the
most recent commit. Doing so, it also looks at B to blame the
unmodified lines of B carried over by S into the merge result. In the
course of this, B is submitted for later processing and marked SEEN.

Later M is blamed. It notices that its parent commit B has been
SEEN and aborts processing for M. B is blamed after that, but only
for the lines that survived via S.

As a result, only the lines contributed by S or by B via S are
blamed. All the other lines that were unchanges by both M and S,
which should have been blamed to B via M, are not blamed.

Solution:
Don't abort processing when encountering a SEEN commit. Rather add the
new region list of lines to be blamed to those of the already SEEN and
enqueued commit's region list. This way when the B commit of the
above example is processed, it will blame both the lines of M and S,
yielding a complete blame result.

Bug: 374382
Change-Id: I369059597608022948009ea7708cc8190f05a8d3
Signed-off-by: Konrad Kügler <swamblumat-eclipsebugs@yahoo.de>
Signed-off-by: Shawn Pearce <spearce@spearce.org>

org.eclipse.jgit.test/tst/org/eclipse/jgit/api/BlameCommandTest.java

@@ -386,4 +386,74 @@ public class BlameCommandTest extends RepositoryTestCase {
 		assertEquals(commit, lines.getSourceCommit(1));
 		assertEquals(commit, lines.getSourceCommit(2));
 	}
+
+	@Test
+	public void testConflictingMerge1() throws Exception {
+		Git git = new Git(db);
+
+		RevCommit base = commitFile("file.txt", join("0", "1", "2", "3", "4"),
+				"master");
+
+		git.checkout().setName("side").setCreateBranch(true)
+				.setStartPoint(base).call();
+		RevCommit side = commitFile("file.txt",
+				join("0", "1 side", "2", "3 on side", "4"), "side");
+
+		commitFile("file.txt", join("0", "1", "2"), "master");
+
+		checkoutBranch("refs/heads/master");
+		git.merge().include(side).call();
+
+		// The merge results in a conflict, which we resolve using mostly the
+		// side branch contents. Especially the "4" survives.
+		RevCommit merge = commitFile("file.txt",
+				join("0", "1 side", "2", "3 resolved", "4"), "master");
+
+		BlameCommand command = new BlameCommand(db);
+		command.setFilePath("file.txt");
+		BlameResult lines = command.call();
+
+		assertEquals(5, lines.getResultContents().size());
+		assertEquals(base, lines.getSourceCommit(0));
+		assertEquals(side, lines.getSourceCommit(1));
+		assertEquals(base, lines.getSourceCommit(2));
+		assertEquals(merge, lines.getSourceCommit(3));
+		assertEquals(base, lines.getSourceCommit(4));
+	}
+
+	// this test inverts the order of the master and side commit and is
+	// otherwise identical to testConflictingMerge1
+	@Test
+	public void testConflictingMerge2() throws Exception {
+		Git git = new Git(db);
+
+		RevCommit base = commitFile("file.txt", join("0", "1", "2", "3", "4"),
+				"master");
+
+		commitFile("file.txt", join("0", "1", "2"), "master");
+
+		git.checkout().setName("side").setCreateBranch(true)
+				.setStartPoint(base).call();
+		RevCommit side = commitFile("file.txt",
+				join("0", "1 side", "2", "3 on side", "4"), "side");
+
+		checkoutBranch("refs/heads/master");
+		git.merge().include(side).call();
+
+		// The merge results in a conflict, which we resolve using mostly the
+		// side branch contents. Especially the "4" survives.
+		RevCommit merge = commitFile("file.txt",
+				join("0", "1 side", "2", "3 resolved", "4"), "master");
+
+		BlameCommand command = new BlameCommand(db);
+		command.setFilePath("file.txt");
+		BlameResult lines = command.call();
+
+		assertEquals(5, lines.getResultContents().size());
+		assertEquals(base, lines.getSourceCommit(0));
+		assertEquals(side, lines.getSourceCommit(1));
+		assertEquals(base, lines.getSourceCommit(2));
+		assertEquals(merge, lines.getSourceCommit(3));
+		assertEquals(base, lines.getSourceCommit(4));
+	}
 }

org.eclipse.jgit/src/org/eclipse/jgit/blame/BlameGenerator.java

@@ -121,7 +121,7 @@ public class BlameGenerator {
 	/** Revision pool used to acquire commits from. */
 	private RevWalk revPool;
 
-	/** Indicates the commit has already been processed. */
+	/** Indicates the commit was put into the queue at least once. */
 	private RevFlag SEEN;
 
 	private ObjectReader reader;
@@ -532,6 +532,7 @@ public class BlameGenerator {
 	private void push(BlobCandidate toInsert) {
 		Candidate c = queue;
 		if (c != null) {
+			c.remove(SEEN); // will be pushed by toInsert
 			c.regionList = null;
 			toInsert.parent = c;
 		}
@@ -539,8 +540,24 @@ public class BlameGenerator {
 	}
 
 	private void push(Candidate toInsert) {
-		// Mark sources to ensure they get discarded (above) if
-		// another path to the same commit.
+		if (toInsert.has(SEEN)) {
+			// We have already added a Candidate for this commit to the queue,
+			// this can happen if the commit is a merge base for two or more
+			// parallel branches that were merged together.
+			//
+			// It is likely the candidate was not yet processed. The queue
+			// sorts descending by commit time and usually descendant commits
+			// have higher timestamps than the ancestors.
+			//
+			// Find the existing candidate and merge the new candidate's
+			// region list into it.
+			for (Candidate p = queue; p != null; p = p.queueNext) {
+				if (p.canMergeRegions(toInsert)) {
+					p.mergeRegions(toInsert);
+					return;
+				}
+			}
+		}
 		toInsert.add(SEEN);
 
 		// Insert into the queue using descending commit time, so
@@ -567,8 +584,6 @@ public class BlameGenerator {
 		RevCommit parent = n.getParent(0);
 		if (parent == null)
 			return split(n.getNextCandidate(0), n);
-		if (parent.has(SEEN))
-			return false;
 		revPool.parseHeaders(parent);
 
 		if (find(parent, n.sourcePath)) {
@@ -636,20 +651,12 @@ public class BlameGenerator {
 	private boolean processMerge(Candidate n) throws IOException {
 		int pCnt = n.getParentCount();
 
-		for (int pIdx = 0; pIdx < pCnt; pIdx++) {
-			RevCommit parent = n.getParent(pIdx);
-			if (parent.has(SEEN))
-				continue;
-			revPool.parseHeaders(parent);
-		}
-
 		// If any single parent exactly matches the merge, follow only
 		// that one parent through history.
 		ObjectId[] ids = null;
 		for (int pIdx = 0; pIdx < pCnt; pIdx++) {
 			RevCommit parent = n.getParent(pIdx);
-			if (parent.has(SEEN))
-				continue;
+			revPool.parseHeaders(parent);
 			if (!find(parent, n.sourcePath))
 				continue;
 			if (!(n instanceof ReverseCandidate) && idBuf.equals(n.sourceBlob)) {
@@ -668,8 +675,6 @@ public class BlameGenerator {
 			renames = new DiffEntry[pCnt];
 			for (int pIdx = 0; pIdx < pCnt; pIdx++) {
 				RevCommit parent = n.getParent(pIdx);
-				if (parent.has(SEEN))
-					continue;
 				if (ids != null && ids[pIdx] != null)
 					continue;
 
@@ -702,8 +707,6 @@ public class BlameGenerator {
 		Candidate[] parents = new Candidate[pCnt];
 		for (int pIdx = 0; pIdx < pCnt; pIdx++) {
 			RevCommit parent = n.getParent(pIdx);
-			if (parent.has(SEEN))
-				continue;
 
 			Candidate p;
 			if (renames != null && renames[pIdx] != null) {

org.eclipse.jgit/src/org/eclipse/jgit/blame/Candidate.java

@@ -124,10 +124,18 @@ class Candidate {
 		return null;
 	}
 
+	boolean has(RevFlag flag) {
+		return sourceCommit.has(flag);
+	}
+
 	void add(RevFlag flag) {
 		sourceCommit.add(flag);
 	}
 
+	void remove(RevFlag flag) {
+		sourceCommit.remove(flag);
+	}
+
 	int getTime() {
 		return sourceCommit.getCommitTime();
 	}
@@ -275,6 +283,42 @@ class Candidate {
 		return r;
 	}
 
+	boolean canMergeRegions(Candidate other) {
+		return sourceCommit == other.sourceCommit
+				&& sourcePath.getPath().equals(other.sourcePath.getPath());
+	}
+
+	void mergeRegions(Candidate other) {
+		// regionList is always sorted by resultStart. Merge join two
+		// linked lists, preserving the ordering. Combine neighboring
+		// regions to reduce the number of results seen by callers.
+		Region a = clearRegionList();
+		Region b = other.clearRegionList();
+		Region t = null;
+
+		while (a != null && b != null) {
+			if (a.resultStart < b.resultStart) {
+				Region n = a.next;
+				t = add(t, this, a);
+				a = n;
+			} else {
+				Region n = b.next;
+				t = add(t, this, b);
+				b = n;
+			}
+		}
+
+		if (a != null) {
+			Region n = a.next;
+			t = add(t, this, a);
+			t.next = n;
+		} else /* b != null */{
+			Region n = b.next;
+			t = add(t, this, b);
+			t.next = n;
+		}
+	}
+
 	@SuppressWarnings("nls")
 	@Override
 	public String toString() {
@@ -369,11 +413,22 @@ class Candidate {
 			return parent;
 		}
 
+		@Override
+		boolean has(RevFlag flag) {
+			return true; // Pretend flag was added; sourceCommit is null.
+		}
+
 		@Override
 		void add(RevFlag flag) {
 			// Do nothing, sourceCommit is null.
 		}
 
+		@Override
+
+		void remove(RevFlag flag) {
+			// Do nothing, sourceCommit is null.
+		}
+
 		@Override
 		int getTime() {
 			return Integer.MAX_VALUE;