RevolveMerger: honor ignoreConflicts also for binary files

Currently difference in binary files during merge will cause them to be
added to unmergedPaths regardless of whether ignoreConflicts is true.

This creates an issue during merging with strategy "RECURSIVE", as it
makes it impossible to create a virtual commit if there is a difference
in a binary file. Resulting in the
CONFLICTS_DURING_MERGE_BASE_CALCULATION error being thrown.

This is especially problematic, since JGit has a
rather simplistic rules for considering file binary, which easily leads
to false positives.

What we should do instead is keep OURS. This will not lead to silently
ignoring difference in the final result. It will allow creation of
virtual merge-base commit, and then the difference would be presented
again in the final merge results. In essense it only affects what's
shown as BASE in 3-way merge.

Additionally, this is correct because
- It's consistent with treatment of other unmergeable entities, for
  example Gitlinks
- It's consistent with behaviour of CGit:
  - https://git-scm.com/docs/gitattributes#Documentation/gitattributes.txt-binary
    states on diffs in binary OURS is picked by default.
  - In code: https://git.kernel.org/pub/scm/git/git.git/tree/merge-ll.c#n81
- ignoreConflicts in CGit afterwards ignores all issues with content
  merging https://git.kernel.org/pub/scm/git/git.git/tree/merge-ort.c#n5201

We also adjust the behaviour when .gitattributes tell us to treat the
file as binary for the purpose of the merge.

We only change the behaviour when ignoreConlicts = true, as otherwise
the current behaviour works as intended.


Change-Id: I2b69f80a13d250aad3fe12dd438b2763f3022270

2 files changed, 85 insertions, 2 deletions

diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/merge/MergerTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/merge/MergerTest.java
index 3a036acaca..c6a6321cf8 100644
--- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/merge/MergerTest.java
+++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/merge/MergerTest.java
@@ -1792,7 +1792,77 @@ public class MergerTest extends RepositoryTestCase {
 		// children
 		mergeResult = git.merge().include(commitC3S).call();
 		assertEquals(mergeResult.getMergeStatus(), MergeStatus.MERGED);
+	}
+
+	/**
+	 * Merging two commits when binary files have equal content, but conflicting content in the
+	 * virtual ancestor.
+	 *
+	 * <p>
+	 * This test has the same set up as
+	 * {@code checkFileDirMergeConflictInVirtualAncestor_NoConflictInChildren}, only
+	 * with the content conflict in A1 and A2.
+	 */
+	@Theory
+	public void checkBinaryMergeConflictInVirtualAncestor(MergeStrategy strategy) throws Exception {
+		if (!strategy.equals(MergeStrategy.RECURSIVE)) {
+			return;
+		}
+
+		Git git = Git.wrap(db);
+
+		// master
+		writeTrashFile("c", "initial file");
+		git.add().addFilepattern("c").call();
+		RevCommit commitI = git.commit().setMessage("Initial commit").call();
+
+		writeTrashFile("a", "\0\1\1\1\1\0");  // content in Ancestor 1
+		git.add().addFilepattern("a").call();
+		RevCommit commitA1 = git.commit().setMessage("Ancestor 1").call();
+
+		writeTrashFile("a", "\0\1\2\3\4\5\0");  // content in Child 1 (commited on master)
+		git.add().addFilepattern("a").call();
+		// commit C1M
+		git.commit().setMessage("Child 1 on master").call();
+
+		git.checkout().setCreateBranch(true).setStartPoint(commitI).setName("branch-to-merge").call();
+		writeTrashFile("a", "\0\2\2\2\2\0");  // content in Ancestor 1
+		git.add().addFilepattern("a").call();
+		RevCommit commitA2 = git.commit().setMessage("Ancestor 2").call();
+
+		// second branch
+		git.checkout().setCreateBranch(true).setStartPoint(commitA1).setName("second-branch").call();
+		writeTrashFile("a", "\0\5\4\3\2\1\0");  // content in Child 2 (commited on second-branch)
+		git.add().addFilepattern("a").call();
+		// commit C2S
+		git.commit().setMessage("Child 2 on second-branch").call();
 
+		// Merge branch-to-merge into second-branch
+		MergeResult mergeResult = git.merge().include(commitA2).setStrategy(strategy).call();
+		assertEquals(mergeResult.getNewHead(), null);
+		assertEquals(mergeResult.getMergeStatus(), MergeStatus.CONFLICTING);
+		// Resolve the conflict manually
+		writeTrashFile("a", "\0\3\3\3\3\0");  // merge conflict resolution
+		git.add().addFilepattern("a").call();
+		RevCommit commitC3S = git.commit().setMessage("Child 3 on second bug - resolve merge conflict").call();
+
+		// Merge branch-to-merge into master
+		git.checkout().setName("master").call();
+		mergeResult = git.merge().include(commitA2).setStrategy(strategy).call();
+		assertEquals(mergeResult.getNewHead(), null);
+		assertEquals(mergeResult.getMergeStatus(), MergeStatus.CONFLICTING);
+
+		// Resolve the conflict manually - set the same value as in resolution above
+		writeTrashFile("a", "\0\3\3\3\3\0");  // merge conflict resolution
+		git.add().addFilepattern("a").call();
+		// commit C4M
+		git.commit().setMessage("Child 4 on master - resolve merge conflict").call();
+
+		// Merge C4M (second-branch) into master (C3S)
+		// Conflict in virtual base should be here, but there are no conflicts in
+		// children
+		mergeResult = git.merge().include(commitC3S).call();
+		assertEquals(mergeResult.getMergeStatus(), MergeStatus.MERGED);
 	}
 
 	/**
diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/merge/ResolveMerger.java b/org.eclipse.jgit/src/org/eclipse/jgit/merge/ResolveMerger.java
index 1ad41be423..50c2c1570c 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/merge/ResolveMerger.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/merge/ResolveMerger.java
@@ -1273,6 +1273,13 @@ public class ResolveMerger extends ThreeWayMerger {
 					default:
 						break;
 				}
+				if (ignoreConflicts) {
+					// If the path is selected to be treated as binary via attributes, we do not perform
+					// content merge. When ignoreConflicts = true, we simply keep OURS to allow virtual commit
+					// to be built.
+					keep(ourDce);
+					return true;
+				}
 				// add the conflicting path to merge result
 				String currentPath = tw.getPathString();
 				MergeResult<RawText> result = new MergeResult<>(
@@ -1312,8 +1319,12 @@ public class ResolveMerger extends ThreeWayMerger {
 					addToCheckout(currentPath, null, attributes);
 					return true;
 				} catch (BinaryBlobException e) {
-					// if the file is binary in either OURS, THEIRS or BASE
-					// here, we don't have an option to ignore conflicts
+					// The file is binary in either OURS, THEIRS or BASE
+					if (ignoreConflicts) {
+						// When ignoreConflicts = true, we simply keep OURS to allow virtual commit to be built.
+						keep(ourDce);
+						return true;
+					}
 				}
 			}
 			switch (getContentMergeStrategy()) {
@@ -1354,6 +1365,8 @@ public class ResolveMerger extends ThreeWayMerger {
 					}
 				}
 			} else {
+				// This is reachable if contentMerge() call above threw BinaryBlobException, so we don't
+				// need to check ignoreConflicts here, since it's already handled above.
 				result.setContainsConflicts(true);
 				addConflict(base, ours, theirs);
 				unmergedPaths.add(currentPath);