From 0155f8bf6e569b487251767be985332a438ccc39 Mon Sep 17 00:00:00 2001 From: Kamil Musin Date: Tue, 8 Oct 2024 13:36:00 +0200 Subject: [PATCH] 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 --- .../org/eclipse/jgit/merge/MergerTest.java | 70 +++++++++++++++++++ .../org/eclipse/jgit/merge/ResolveMerger.java | 17 ++++- 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. + * + *

+ * 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 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); -- 2.39.5