Skip to content

Commit

Permalink
ResolveMerger: Fix the issue with binary modify-modify conflicts
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@kumarp149 @msohn
kumarp149 authored and msohn committed Apr 25, 2024
1 parent e97623d commit 567315a
Show file tree
Hide file tree
Showing 3 changed files with 230 additions and 38 deletions.
9 changes: 1 addition & 8 deletions org.eclipse.jgit.test/tst/org/eclipse/jgit/attributes/merge/MergeGitAttributeTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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));
}
Expand Down
186 changes: 186 additions & 0 deletions org.eclipse.jgit.test/tst/org/eclipse/jgit/merge/MergerTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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<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);
Expand Down
73 changes: 43 additions & 30 deletions org.eclipse.jgit/src/org/eclipse/jgit/merge/ResolveMerger.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1274,10 +1274,15 @@ protected boolean processEntry(CanonicalTreeParser base,
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;
}

Expand All @@ -1289,38 +1294,48 @@ protected boolean processEntry(CanonicalTreeParser base,
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);
Expand All @@ -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)
Expand All @@ -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
Expand Down

0 comments on commit 567315a

Please sign in to comment.