ResolveMerger: Fix the issue with binary modify-modify conflicts

1) If the file was marked as binary by git attributes, we should add the
path to conflicts if content differs in OURS and THEIRS
2) If the path is a file in OURS, THEIRS and BASE and if it is a binary
in any one of them, no content merge should be attempted and the file
content is kept as is in the work tree

Bug: jgit-14
Change-Id: I9201bdc53a55f8f40adade4b6a36ee8ae25f4db8

3 files changed, 230 insertions, 38 deletions

diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/attributes/merge/MergeGitAttributeTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/attributes/merge/MergeGitAttributeTest.java
index 795029188d..6b23de3320 100644
--- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/attributes/merge/MergeGitAttributeTest.java
+++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/attributes/merge/MergeGitAttributeTest.java
@@ -10,7 +10,6 @@
 package org.eclipse.jgit.attributes.merge;
 
 import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertTrue;
@@ -42,7 +41,6 @@ import org.eclipse.jgit.revwalk.RevCommit;
 import org.eclipse.jgit.treewalk.FileTreeIterator;
 import org.eclipse.jgit.treewalk.TreeWalk;
 import org.eclipse.jgit.treewalk.filter.PathFilter;
-import org.junit.Ignore;
 import org.junit.Test;
 
 public class MergeGitAttributeTest extends RepositoryTestCase {
@@ -268,12 +266,7 @@ public class MergeGitAttributeTest extends RepositoryTestCase {
 		}
 	}
 
-	/*
-	 * This test is commented because JGit add conflict markers in binary files.
-	 * cf. https://www.eclipse.org/forums/index.php/t/1086511/
-	 */
 	@Test
-	@Ignore
 	public void mergeBinaryFile_NoAttr_Conflict() throws IllegalStateException,
 			IOException, NoHeadException, ConcurrentRefUpdateException,
 			CheckoutConflictException, InvalidMergeHeadsException,
@@ -433,7 +426,7 @@ public class MergeGitAttributeTest extends RepositoryTestCase {
 			try (FileInputStream mergeResultFile = new FileInputStream(
 					db.getWorkTree().toPath().resolve(ENABLED_CHECKED_GIF)
 							.toFile())) {
-				assertFalse(contentEquals(
+				assertTrue(contentEquals(
 						getClass().getResourceAsStream(ENABLED_CHECKED_GIF),
 						mergeResultFile));
 			}
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 022e8cd55e..3f99fe2b26 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
@@ -22,9 +22,12 @@ import java.io.ByteArrayOutputStream;
 import java.io.File;
 import java.io.FileInputStream;
 import java.io.IOException;
+import java.nio.file.Files;
 import java.time.Instant;
 import java.util.Arrays;
+import java.util.HashSet;
 import java.util.Map;
+import java.util.Set;
 
 import org.eclipse.jgit.api.Git;
 import org.eclipse.jgit.api.MergeResult;
@@ -51,6 +54,7 @@ import org.eclipse.jgit.lib.ObjectInserter;
 import org.eclipse.jgit.lib.ObjectLoader;
 import org.eclipse.jgit.lib.ObjectReader;
 import org.eclipse.jgit.lib.ObjectStream;
+import org.eclipse.jgit.lib.Repository;
 import org.eclipse.jgit.lib.StoredConfig;
 import org.eclipse.jgit.merge.ResolveMerger.MergeFailureReason;
 import org.eclipse.jgit.revwalk.RevCommit;
@@ -1789,6 +1793,188 @@ public class MergerTest extends RepositoryTestCase {
 
 	}
 
+	/**
+	 * File is binary in ours, theirs and base with different content in each of
+	 * them. Content of the file should not change after the merge conflict as
+	 * no conflict markers are added to the binary files
+	 */
+	@Theory
+	public void oursBinaryTheirsBinaryBaseBinary(MergeStrategy strategy)
+			throws Exception {
+		Git git = Git.wrap(db);
+		String binaryFile = "file";
+
+		writeTrashFile(binaryFile, "\u0000\u0001");
+		git.add().addFilepattern(binaryFile).call();
+		RevCommit parent = git.commit().setMessage("BASE COMMIT").call();
+		String fileHashInBase = getFileHashInWorkTree(git, binaryFile);
+
+		writeTrashFile(binaryFile, "\u0001\u0002");
+		git.add().addFilepattern(binaryFile).call();
+		RevCommit child1 = git.commit().setMessage("THEIRS COMMIT").call();
+		String fileHashInChild1 = getFileHashInWorkTree(git, binaryFile);
+
+		git.checkout().setCreateBranch(true).setStartPoint(parent)
+				.setName("side").call();
+
+		writeTrashFile(binaryFile, "\u0002\u0000");
+		git.add().addFilepattern(binaryFile).call();
+		git.commit().setMessage("OURS COMMIT").call();
+		String fileHashInChild2 = getFileHashInWorkTree(git, binaryFile);
+
+		MergeResult mergeResult = git.merge().setStrategy(strategy)
+				.include(child1).call();
+
+		// check if the merge caused a conflict
+		assertTrue(mergeResult.getConflicts() != null
+				&& !mergeResult.getConflicts().isEmpty());
+		String fileHashInChild2AfterMerge = getFileHashInWorkTree(git,
+				binaryFile);
+
+		// check if the file content changed during a conflicting merge
+		assertEquals(fileHashInChild2AfterMerge, fileHashInChild2);
+
+		Set<String> hashesInIndexFile = new HashSet<>();
+		DirCache indexContent = git.getRepository().readDirCache();
+		for (int i = 0; i < indexContent.getEntryCount(); ++i) {
+			DirCacheEntry indexEntry = indexContent.getEntry(i);
+			if (binaryFile.equals(indexEntry.getPathString())) {
+				hashesInIndexFile.add(indexEntry.getObjectId().name());
+			}
+		}
+
+		// check if all the three stages are added to index file
+		assertTrue(hashesInIndexFile.contains(fileHashInBase));
+		assertTrue(hashesInIndexFile.contains(fileHashInChild1));
+		assertTrue(hashesInIndexFile.contains(fileHashInChild2));
+	}
+
+	/**
+	 * File is text in ours and theirs with different content but binary in
+	 * base. Even in this case, file will be treated as a binary and no conflict
+	 * markers are added to it
+	 */
+	@Theory
+	public void oursAndTheirsDifferentTextBaseBinary(MergeStrategy strategy)
+			throws Exception {
+		Git git = Git.wrap(db);
+		String binaryFile = "file";
+
+		writeTrashFile(binaryFile, "\u0000\u0001");
+		git.add().addFilepattern(binaryFile).call();
+		RevCommit parent = git.commit().setMessage("BASE COMMIT").call();
+		String fileHashInBase = getFileHashInWorkTree(git, binaryFile);
+
+		writeTrashFile(binaryFile, "TEXT1");
+		git.add().addFilepattern(binaryFile).call();
+		RevCommit child1 = git.commit().setMessage("THEIRS COMMIT").call();
+		String fileHashInChild1 = getFileHashInWorkTree(git, binaryFile);
+
+		git.checkout().setCreateBranch(true).setStartPoint(parent)
+				.setName("side").call();
+
+		writeTrashFile(binaryFile, "TEXT2");
+		git.add().addFilepattern(binaryFile).call();
+		git.commit().setMessage("OURS COMMIT").call();
+		String fileHashInChild2 = getFileHashInWorkTree(git, binaryFile);
+
+		MergeResult mergeResult = git.merge().setStrategy(strategy)
+				.include(child1).call();
+
+		assertTrue(mergeResult.getConflicts() != null
+				&& !mergeResult.getConflicts().isEmpty());
+		String fileHashInChild2AfterMerge = getFileHashInWorkTree(git,
+				binaryFile);
+
+		assertEquals(fileHashInChild2AfterMerge, fileHashInChild2);
+
+		Set<String> hashesInIndexFile = new HashSet<>();
+		DirCache indexContent = git.getRepository().readDirCache();
+		for (int i = 0; i < indexContent.getEntryCount(); ++i) {
+			DirCacheEntry indexEntry = indexContent.getEntry(i);
+			if (binaryFile.equals(indexEntry.getPathString())) {
+				hashesInIndexFile.add(indexEntry.getObjectId().name());
+			}
+		}
+
+		assertTrue(hashesInIndexFile.contains(fileHashInBase));
+		assertTrue(hashesInIndexFile.contains(fileHashInChild1));
+		assertTrue(hashesInIndexFile.contains(fileHashInChild2));
+	}
+
+	/**
+	 * Tests the scenario where a file is expected to be treated as binary
+	 * according to Git attributes
+	 */
+	@Theory
+	public void fileInBinaryInAttribute(MergeStrategy strategy)
+			throws Exception {
+		Git git = Git.wrap(db);
+		String binaryFile = "file.bin";
+
+		writeTrashFile(".gitattributes", binaryFile + " binary");
+		git.add().addFilepattern(".gitattributes").call();
+		git.commit().setMessage("ADDING GITATTRIBUTES").call();
+
+		writeTrashFile(binaryFile, "\u0000\u0001");
+		git.add().addFilepattern(binaryFile).call();
+		RevCommit parent = git.commit().setMessage("BASE COMMIT").call();
+		String fileHashInBase = getFileHashInWorkTree(git, binaryFile);
+
+		writeTrashFile(binaryFile, "\u0001\u0002");
+		git.add().addFilepattern(binaryFile).call();
+		RevCommit child1 = git.commit().setMessage("THEIRS COMMIT").call();
+		String fileHashInChild1 = getFileHashInWorkTree(git, binaryFile);
+
+		git.checkout().setCreateBranch(true).setStartPoint(parent)
+				.setName("side").call();
+
+		writeTrashFile(binaryFile, "\u0002\u0000");
+		git.add().addFilepattern(binaryFile).call();
+		git.commit().setMessage("OURS COMMIT").call();
+		String fileHashInChild2 = getFileHashInWorkTree(git, binaryFile);
+
+		MergeResult mergeResult = git.merge().setStrategy(strategy)
+				.include(child1).call();
+
+		// check if the merge caused a conflict
+		assertTrue(mergeResult.getConflicts() != null
+				&& !mergeResult.getConflicts().isEmpty());
+		String fileHashInChild2AfterMerge = getFileHashInWorkTree(git,
+				binaryFile);
+
+		// check if the file content changed during a conflicting merge
+		assertEquals(fileHashInChild2AfterMerge, fileHashInChild2);
+
+		Set<String> hashesInIndexFile = new HashSet<>();
+		DirCache indexContent = git.getRepository().readDirCache();
+		for (int i = 0; i < indexContent.getEntryCount(); ++i) {
+			DirCacheEntry indexEntry = indexContent.getEntry(i);
+			if (binaryFile.equals(indexEntry.getPathString())) {
+				hashesInIndexFile.add(indexEntry.getObjectId().name());
+			}
+		}
+
+		// check if all the three stages are added to index file
+		assertTrue(hashesInIndexFile.contains(fileHashInBase));
+		assertTrue(hashesInIndexFile.contains(fileHashInChild1));
+		assertTrue(hashesInIndexFile.contains(fileHashInChild2));
+	}
+
+	private String getFileHashInWorkTree(Git git, String filePath)
+			throws IOException {
+		Repository repository = git.getRepository();
+		ObjectInserter objectInserter = repository.newObjectInserter();
+
+		File conflictingFile = new File(repository.getWorkTree(), filePath);
+		byte[] fileContent = Files.readAllBytes(conflictingFile.toPath());
+		ObjectId blobId = objectInserter.insert(Constants.OBJ_BLOB,
+				fileContent);
+		objectInserter.flush();
+
+		return blobId.name();
+	}
+
 	private void writeSubmodule(String path, ObjectId commit)
 			throws IOException, ConfigInvalidException {
 		addSubmoduleToIndex(path, commit);
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 13cccee16b..825ef17c3f 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/merge/ResolveMerger.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/merge/ResolveMerger.java
@@ -1274,10 +1274,15 @@ public class ResolveMerger extends ThreeWayMerger {
 					default:
 						break;
 				}
+				// add the conflicting path to merge result
+				String currentPath = tw.getPathString();
+				MergeResult<RawText> result = new MergeResult<>(
+						Collections.emptyList());
+				result.setContainsConflicts(true);
+				mergeResults.put(currentPath, result);
 				addConflict(base, ours, theirs);
-
 				// attribute merge issues are conflicts but not failures
-				unmergedPaths.add(tw.getPathString());
+				unmergedPaths.add(currentPath);
 				return true;
 			}
 
@@ -1289,38 +1294,48 @@ public class ResolveMerger extends ThreeWayMerger {
 			MergeResult<RawText> result = null;
 			boolean hasSymlink = FileMode.SYMLINK.equals(modeO)
 					|| FileMode.SYMLINK.equals(modeT);
+
+			String currentPath = tw.getPathString();
+			// if the path is not a symlink in ours and theirs
 			if (!hasSymlink) {
 				try {
 					result = contentMerge(base, ours, theirs, attributes,
 							getContentMergeStrategy());
-				} catch (BinaryBlobException e) {
-					// result == null
-				}
-			}
-			if (result == null) {
-				switch (getContentMergeStrategy()) {
-				case OURS:
-					keep(ourDce);
-					return true;
-				case THEIRS:
-					DirCacheEntry e = add(tw.getRawPath(), theirs,
-							DirCacheEntry.STAGE_0, EPOCH, 0);
-					if (e != null) {
-						addToCheckout(tw.getPathString(), e, attributes);
+					if (result.containsConflicts() && !ignoreConflicts) {
+						result.setContainsConflicts(true);
+						unmergedPaths.add(currentPath);
+					} else if (ignoreConflicts) {
+						result.setContainsConflicts(false);
 					}
+					updateIndex(base, ours, theirs, result, attributes[T_OURS]);
+					workTreeUpdater.markAsModified(currentPath);
+					// Entry is null - only add the metadata
+					addToCheckout(currentPath, null, attributes);
 					return true;
-				default:
-					result = new MergeResult<>(Collections.emptyList());
-					result.setContainsConflicts(true);
-					break;
+				} catch (BinaryBlobException e) {
+					// if the file is binary in either OURS, THEIRS or BASE
+					// here, we don't have an option to ignore conflicts
 				}
 			}
-			if (ignoreConflicts) {
-				result.setContainsConflicts(false);
+			switch (getContentMergeStrategy()) {
+			case OURS:
+				keep(ourDce);
+				return true;
+			case THEIRS:
+				DirCacheEntry e = add(tw.getRawPath(), theirs,
+						DirCacheEntry.STAGE_0, EPOCH, 0);
+				if (e != null) {
+					addToCheckout(currentPath, e, attributes);
+				}
+				return true;
+			default:
+				result = new MergeResult<>(Collections.emptyList());
+				result.setContainsConflicts(true);
+				break;
 			}
-			String currentPath = tw.getPathString();
 			if (hasSymlink) {
 				if (ignoreConflicts) {
+					result.setContainsConflicts(false);
 					if (((modeT & FileMode.TYPE_MASK) == FileMode.TYPE_FILE)) {
 						DirCacheEntry e = add(tw.getRawPath(), theirs,
 								DirCacheEntry.STAGE_0, EPOCH, 0);
@@ -1329,9 +1344,9 @@ public class ResolveMerger extends ThreeWayMerger {
 						keep(ourDce);
 					}
 				} else {
-					// Record the conflict
 					DirCacheEntry e = addConflict(base, ours, theirs);
 					mergeResults.put(currentPath, result);
+					unmergedPaths.add(currentPath);
 					// If theirs is a file, check it out. In link/file
 					// conflicts, C git prefers the file.
 					if (((modeT & FileMode.TYPE_MASK) == FileMode.TYPE_FILE)
@@ -1340,14 +1355,12 @@ public class ResolveMerger extends ThreeWayMerger {
 					}
 				}
 			} else {
-				updateIndex(base, ours, theirs, result, attributes[T_OURS]);
-			}
-			if (result.containsConflicts() && !ignoreConflicts) {
+				result.setContainsConflicts(true);
+				addConflict(base, ours, theirs);
 				unmergedPaths.add(currentPath);
+				mergeResults.put(currentPath, result);
 			}
-			workTreeUpdater.markAsModified(currentPath);
-			// Entry is null - only adds the metadata.
-			addToCheckout(currentPath, null, attributes);
+			return true;
 		} else if (modeO != modeT) {
 			// OURS or THEIRS has been deleted
 			if (((modeO != 0 && !tw.idEqual(T_BASE, T_OURS)) || (modeT != 0 && !tw