From 567315af548017cc58eadb91bba493b74c391009 Mon Sep 17 00:00:00 2001 From: Sruteesh Date: Wed, 14 Feb 2024 22:19:39 +0530 Subject: [PATCH] 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 --- .../merge/MergeGitAttributeTest.java | 9 +- .../org/eclipse/jgit/merge/MergerTest.java | 186 ++++++++++++++++++ .../org/eclipse/jgit/merge/ResolveMerger.java | 73 ++++--- 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 795029188d0..6b23de3320a 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.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 void mergeTextualFile_SetBinaryMerge_Conflict() } } - /* - * 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 void mergeBinaryFile_SetMerge_Conflict() 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 022e8cd55e7..3f99fe2b260 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.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.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 void checkModeMergeConflictInVirtualAncestor(MergeStrategy strategy) thro } + /** + * 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 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 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 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 13cccee16bc..825ef17c3fd 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 @@ protected boolean processEntry(CanonicalTreeParser base, default: break; } + // add the conflicting path to merge result + String currentPath = tw.getPathString(); + MergeResult 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 @@ protected boolean processEntry(CanonicalTreeParser base, MergeResult 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 @@ protected boolean processEntry(CanonicalTreeParser base, 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 @@ protected boolean processEntry(CanonicalTreeParser base, } } } 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