From e60ea7324f37bd0a1086e27fad52ecc5232e9c06 Mon Sep 17 00:00:00 2001 From: Demetr Starshov Date: Tue, 11 Aug 2020 17:57:10 -0700 Subject: [PATCH 1/6] ResolveMerger: Adding test cases for GITLINK merge Add test cases which cover content-merge resolve logic. Git clients try to agressively merge blobs by content, but GITLINK types of entries can't be merged with each other or with blobs. This change ensures all possible permutations which can trigger blob and GITLINK content merge are covered. Signed-off-by: Demetr Starshov Change-Id: I7e83a28a14d4d2f9e0ba2b1cffbf3224fb7f3fef --- .../jgit/junit/RepositoryTestCase.java | 16 + .../eclipse/jgit/merge/GitlinkMergeTest.java | 291 ++++++++++++++++++ 2 files changed, 307 insertions(+) create mode 100644 org.eclipse.jgit.test/tst/org/eclipse/jgit/merge/GitlinkMergeTest.java diff --git a/org.eclipse.jgit.junit/src/org/eclipse/jgit/junit/RepositoryTestCase.java b/org.eclipse.jgit.junit/src/org/eclipse/jgit/junit/RepositoryTestCase.java index de11e2c00..cc84f197a 100644 --- a/org.eclipse.jgit.junit/src/org/eclipse/jgit/junit/RepositoryTestCase.java +++ b/org.eclipse.jgit.junit/src/org/eclipse/jgit/junit/RepositoryTestCase.java @@ -34,6 +34,7 @@ import org.eclipse.jgit.dircache.DirCacheCheckout; import org.eclipse.jgit.dircache.DirCacheEntry; import org.eclipse.jgit.internal.storage.file.FileRepository; +import org.eclipse.jgit.lib.AnyObjectId; import org.eclipse.jgit.lib.Constants; import org.eclipse.jgit.lib.FileMode; import org.eclipse.jgit.lib.ObjectId; @@ -512,6 +513,21 @@ protected DirCacheEntry createEntry(final String path, final FileMode mode, return entry; } + /** + * Create DirCacheEntry + * + * @param path + * @param objectId + * @return the DirCacheEntry + */ + protected DirCacheEntry createGitLink(String path, AnyObjectId objectId) { + final DirCacheEntry entry = new DirCacheEntry(path, + DirCacheEntry.STAGE_0); + entry.setFileMode(FileMode.GITLINK); + entry.setObjectId(objectId); + return entry; + } + /** * Assert files are equal * diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/merge/GitlinkMergeTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/merge/GitlinkMergeTest.java new file mode 100644 index 000000000..4165812e2 --- /dev/null +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/merge/GitlinkMergeTest.java @@ -0,0 +1,291 @@ +/* + * Copyright (c) 2020, Google LLC and others + * + * This program and the accompanying materials are made available under the + * terms of the Eclipse Distribution License v. 1.0 which is available at + * http://www.eclipse.org/org/documents/edl-v10.php. + * + * SPDX-License-Identifier: BSD-3-Clause + */ + +package org.eclipse.jgit.merge; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; + +import java.io.IOException; + +import org.eclipse.jgit.annotations.Nullable; +import org.eclipse.jgit.dircache.DirCache; +import org.eclipse.jgit.dircache.DirCacheBuilder; +import org.eclipse.jgit.dircache.DirCacheEntry; +import org.eclipse.jgit.lib.CommitBuilder; +import org.eclipse.jgit.lib.FileMode; +import org.eclipse.jgit.lib.ObjectId; +import org.eclipse.jgit.lib.ObjectInserter; +import org.eclipse.jgit.lib.PersonIdent; +import org.eclipse.jgit.test.resources.SampleDataRepositoryTestCase; +import org.eclipse.jgit.treewalk.TreeWalk; +import org.junit.Ignore; +import org.junit.Test; + +public class GitlinkMergeTest extends SampleDataRepositoryTestCase { + private static final String LINK_ID1 = "DEADBEEFDEADBEEFBABEDEADBEEFDEADBEEFBABE"; + private static final String LINK_ID2 = "DEADDEADDEADDEADDEADDEADDEADDEADDEADDEAD"; + private static final String LINK_ID3 = "BEEFBEEFBEEFBEEFBEEFBEEFBEEFBEEFBEEFBEEF"; + + private static final String SUBMODULE_PATH = "submodule.link"; + + @Test + public void testGitLinkMerging_UpdateUpdate() throws Exception { + testGitLink(LINK_ID1, LINK_ID2, LINK_ID3, newResolveMerger(), false); + } + + @Test + public void testGitLinkMerging_bothAddedSameLink() throws Exception { + assertGitLinkValue( + testGitLink(null, LINK_ID2, LINK_ID2, newResolveMerger(), true), + LINK_ID2); + } + + @Test + public void testGitLinkMerging_bothAddedDifferentLink() throws Exception { + testGitLink(null, LINK_ID2, LINK_ID3, newResolveMerger(), false); + } + + @Test + public void testGitLinkMerging_AddNew_ignoreConflicts() throws Exception { + assertGitLinkValue( + testGitLink(null, null, LINK_ID3, newIgnoreConflictMerger(), + true), + LINK_ID3); + } + + @Test + @Ignore("broken - doesn't ignore conflicts") + public void testGitLinkMerging_UpdateUpdate_ignoreConflicts() + throws Exception { + assertGitLinkValue(testGitLink(LINK_ID1, LINK_ID2, LINK_ID3, + newIgnoreConflictMerger(), true), LINK_ID2); + } + + @Test + @Ignore("broken - doesn't ignore conflicts") + public void testGitLinkMerging_bothAddedSameLink_ignoreConflicts() + throws Exception { + assertGitLinkValue(testGitLink(null, LINK_ID2, LINK_ID2, + newIgnoreConflictMerger(), true), LINK_ID2); + } + + @Test + @Ignore("broken - doesn't ignore conflicts") + public void testGitLinkMerging_bothAddedDifferentLink_ignoreConflicts() + throws Exception { + assertGitLinkValue(testGitLink(null, LINK_ID2, LINK_ID3, + newIgnoreConflictMerger(), true), LINK_ID2); + } + + protected Merger testGitLink(@Nullable String baseLink, + @Nullable String oursLink, @Nullable String theirsLink, + Merger merger, boolean shouldMerge) + throws Exception { + DirCache treeB = db.readDirCache(); + DirCache treeO = db.readDirCache(); + DirCache treeT = db.readDirCache(); + + DirCacheBuilder bTreeBuilder = treeB.builder(); + DirCacheBuilder oTreeBuilder = treeO.builder(); + DirCacheBuilder tTreeBuilder = treeT.builder(); + + maybeAddLink(bTreeBuilder, baseLink); + maybeAddLink(oTreeBuilder, oursLink); + maybeAddLink(tTreeBuilder, theirsLink); + + bTreeBuilder.finish(); + oTreeBuilder.finish(); + tTreeBuilder.finish(); + + ObjectInserter ow = db.newObjectInserter(); + ObjectId b = commit(ow, treeB, new ObjectId[] {}); + ObjectId o = commit(ow, treeO, new ObjectId[] { b }); + ObjectId t = commit(ow, treeT, new ObjectId[] { b }); + + boolean merge = merger.merge(new ObjectId[] { o, t }); + assertEquals(shouldMerge, merge); + + return merger; + } + + private Merger newResolveMerger() { + return MergeStrategy.RESOLVE.newMerger(db, true); + } + + private Merger newIgnoreConflictMerger() { + return new ResolveMerger(db, true) { + @Override + protected boolean mergeImpl() throws IOException { + // emulate call with ignore conflicts. + return mergeTrees(mergeBase(), sourceTrees[0], sourceTrees[1], + true); + } + }; + } + + @Test + public void testGitLinkMerging_blobWithLink() throws Exception { + DirCache treeB = db.readDirCache(); + DirCache treeO = db.readDirCache(); + DirCache treeT = db.readDirCache(); + + DirCacheBuilder bTreeBuilder = treeB.builder(); + DirCacheBuilder oTreeBuilder = treeO.builder(); + DirCacheBuilder tTreeBuilder = treeT.builder(); + + bTreeBuilder.add( + createEntry(SUBMODULE_PATH, FileMode.REGULAR_FILE, "blob")); + oTreeBuilder.add( + createEntry(SUBMODULE_PATH, FileMode.REGULAR_FILE, "blob 2")); + + maybeAddLink(tTreeBuilder, LINK_ID3); + + bTreeBuilder.finish(); + oTreeBuilder.finish(); + tTreeBuilder.finish(); + + ObjectInserter ow = db.newObjectInserter(); + ObjectId b = commit(ow, treeB, new ObjectId[] {}); + ObjectId o = commit(ow, treeO, new ObjectId[] { b }); + ObjectId t = commit(ow, treeT, new ObjectId[] { b }); + + Merger resolveMerger = MergeStrategy.RESOLVE.newMerger(db); + boolean merge = resolveMerger.merge(new ObjectId[] { o, t }); + assertFalse(merge); + } + + @Test + public void testGitLinkMerging_linkWithBlob() throws Exception { + DirCache treeB = db.readDirCache(); + DirCache treeO = db.readDirCache(); + DirCache treeT = db.readDirCache(); + + DirCacheBuilder bTreeBuilder = treeB.builder(); + DirCacheBuilder oTreeBuilder = treeO.builder(); + DirCacheBuilder tTreeBuilder = treeT.builder(); + + maybeAddLink(bTreeBuilder, LINK_ID1); + maybeAddLink(oTreeBuilder, LINK_ID2); + tTreeBuilder.add( + createEntry(SUBMODULE_PATH, FileMode.REGULAR_FILE, "blob 3")); + + bTreeBuilder.finish(); + oTreeBuilder.finish(); + tTreeBuilder.finish(); + + ObjectInserter ow = db.newObjectInserter(); + ObjectId b = commit(ow, treeB, new ObjectId[] {}); + ObjectId o = commit(ow, treeO, new ObjectId[] { b }); + ObjectId t = commit(ow, treeT, new ObjectId[] { b }); + + Merger resolveMerger = MergeStrategy.RESOLVE.newMerger(db); + boolean merge = resolveMerger.merge(new ObjectId[] { o, t }); + assertFalse(merge); + } + + @Test + public void testGitLinkMerging_linkWithLink() throws Exception { + DirCache treeB = db.readDirCache(); + DirCache treeO = db.readDirCache(); + DirCache treeT = db.readDirCache(); + + DirCacheBuilder bTreeBuilder = treeB.builder(); + DirCacheBuilder oTreeBuilder = treeO.builder(); + DirCacheBuilder tTreeBuilder = treeT.builder(); + + bTreeBuilder.add( + createEntry(SUBMODULE_PATH, FileMode.REGULAR_FILE, "blob")); + maybeAddLink(oTreeBuilder, LINK_ID2); + maybeAddLink(tTreeBuilder, LINK_ID3); + + bTreeBuilder.finish(); + oTreeBuilder.finish(); + tTreeBuilder.finish(); + + ObjectInserter ow = db.newObjectInserter(); + ObjectId b = commit(ow, treeB, new ObjectId[] {}); + ObjectId o = commit(ow, treeO, new ObjectId[] { b }); + ObjectId t = commit(ow, treeT, new ObjectId[] { b }); + + Merger resolveMerger = MergeStrategy.RESOLVE.newMerger(db); + boolean merge = resolveMerger.merge(new ObjectId[] { o, t }); + assertFalse(merge); + } + + @Test + @Ignore("broken - try to do content-merge with GITLINK") + public void testGitLinkMerging_blobWithBlobFromLink() throws Exception { + DirCache treeB = db.readDirCache(); + DirCache treeO = db.readDirCache(); + DirCache treeT = db.readDirCache(); + + DirCacheBuilder bTreeBuilder = treeB.builder(); + DirCacheBuilder oTreeBuilder = treeO.builder(); + DirCacheBuilder tTreeBuilder = treeT.builder(); + + maybeAddLink(bTreeBuilder, LINK_ID1); + oTreeBuilder.add( + createEntry(SUBMODULE_PATH, FileMode.REGULAR_FILE, "blob 2")); + tTreeBuilder.add( + createEntry(SUBMODULE_PATH, FileMode.REGULAR_FILE, "blob 3")); + + bTreeBuilder.finish(); + oTreeBuilder.finish(); + tTreeBuilder.finish(); + + ObjectInserter ow = db.newObjectInserter(); + ObjectId b = commit(ow, treeB, new ObjectId[] {}); + ObjectId o = commit(ow, treeO, new ObjectId[] { b }); + ObjectId t = commit(ow, treeT, new ObjectId[] { b }); + + Merger resolveMerger = MergeStrategy.RESOLVE.newMerger(db); + boolean merge = resolveMerger.merge(new ObjectId[] { o, t }); + assertFalse(merge); + } + + private void maybeAddLink(DirCacheBuilder builder, + @Nullable String linkId) { + if (linkId == null) { + return; + } + DirCacheEntry newLink = createGitLink(SUBMODULE_PATH, + ObjectId.fromString(linkId)); + builder.add(newLink); + } + + private void assertGitLinkValue(Merger resolveMerger, String expectedValue) + throws Exception { + try (TreeWalk tw = new TreeWalk(db)) { + tw.setRecursive(true); + tw.reset(resolveMerger.getResultTreeId()); + + assertTrue(tw.next()); + assertEquals(SUBMODULE_PATH, tw.getPathString()); + assertEquals(ObjectId.fromString(expectedValue), tw.getObjectId(0)); + + assertFalse(tw.next()); + } + } + + private static ObjectId commit(ObjectInserter odi, DirCache treeB, + ObjectId[] parentIds) throws Exception { + CommitBuilder c = new CommitBuilder(); + c.setTreeId(treeB.writeTree(odi)); + c.setAuthor(new PersonIdent("A U Thor", "a.u.thor", 1L, 0)); + c.setCommitter(c.getAuthor()); + c.setParentIds(parentIds); + c.setMessage("Tree " + c.getTreeId().name()); + ObjectId id = odi.insert(c); + odi.flush(); + return id; + } +} From 3da7ea50a996efccee198ece79c90c2513b482a7 Mon Sep 17 00:00:00 2001 From: Demetr Starshov Date: Tue, 11 Aug 2020 18:24:33 -0700 Subject: [PATCH 2/6] ResolveMerger: extracting createGitLinksMergeResult method Signed-off-by: Demetr Starshov Change-Id: Ibc8b954266b1b4b9b9f404e3433f0d7cdae107e8 --- .../org/eclipse/jgit/merge/ResolveMerger.java | 22 ++++++++++++------- 1 file changed, 14 insertions(+), 8 deletions(-) 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 506d33312..85f3fa58b 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/merge/ResolveMerger.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/merge/ResolveMerger.java @@ -745,14 +745,8 @@ protected boolean processEntry(CanonicalTreeParser base, add(tw.getRawPath(), theirs, DirCacheEntry.STAGE_3, EPOCH, 0); if (gitlinkConflict) { - MergeResult result = new MergeResult<>( - Arrays.asList( - new SubmoduleConflict(base == null ? null - : base.getEntryObjectId()), - new SubmoduleConflict(ours == null ? null - : ours.getEntryObjectId()), - new SubmoduleConflict(theirs == null ? null - : theirs.getEntryObjectId()))); + MergeResult result = createGitLinksMergeResult( + base, ours, theirs); result.setContainsConflicts(true); mergeResults.put(tw.getPathString(), result); if (!ignoreConflicts) { @@ -825,6 +819,18 @@ protected boolean processEntry(CanonicalTreeParser base, return true; } + private MergeResult createGitLinksMergeResult( + CanonicalTreeParser base, CanonicalTreeParser ours, + CanonicalTreeParser theirs) { + return new MergeResult<>(Arrays.asList( + new SubmoduleConflict( + base == null ? null : base.getEntryObjectId()), + new SubmoduleConflict( + ours == null ? null : ours.getEntryObjectId()), + new SubmoduleConflict( + theirs == null ? null : theirs.getEntryObjectId()))); + } + /** * Does the content merge. The three texts base, ours and theirs are * specified with {@link CanonicalTreeParser}. If any of the parsers is From 29e998a0e6415fe2f1439be5fe28537c6d3d3dea Mon Sep 17 00:00:00 2001 From: Demetr Starshov Date: Tue, 11 Aug 2020 18:30:49 -0700 Subject: [PATCH 3/6] ResolveMerger: improving content merge readability Separate "GITLINK conflict" and "attributes can't be content merged" cases. Signed-off-by: Demetr Starshov Change-Id: I29424e13ea1738af750196e7bf4315256a6095b6 --- .../org/eclipse/jgit/merge/ResolveMerger.java | 26 ++++++++++--------- 1 file changed, 14 insertions(+), 12 deletions(-) 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 85f3fa58b..508f7ba62 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/merge/ResolveMerger.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/merge/ResolveMerger.java @@ -739,24 +739,26 @@ protected boolean processEntry(CanonicalTreeParser base, boolean gitlinkConflict = isGitLink(modeO) || isGitLink(modeT); // Don't attempt to resolve submodule link conflicts - if (gitlinkConflict || !attributes.canBeContentMerged()) { + if (gitlinkConflict) { add(tw.getRawPath(), base, DirCacheEntry.STAGE_1, EPOCH, 0); add(tw.getRawPath(), ours, DirCacheEntry.STAGE_2, EPOCH, 0); add(tw.getRawPath(), theirs, DirCacheEntry.STAGE_3, EPOCH, 0); - if (gitlinkConflict) { - MergeResult result = createGitLinksMergeResult( - base, ours, theirs); - result.setContainsConflicts(true); - mergeResults.put(tw.getPathString(), result); - if (!ignoreConflicts) { - unmergedPaths.add(tw.getPathString()); - } - } else { - // attribute merge issues are conflicts but not failures + MergeResult result = createGitLinksMergeResult( + base, ours, theirs); + result.setContainsConflicts(true); + mergeResults.put(tw.getPathString(), result); + if (!ignoreConflicts) { unmergedPaths.add(tw.getPathString()); } return true; + } else if (!attributes.canBeContentMerged()) { + add(tw.getRawPath(), base, DirCacheEntry.STAGE_1, EPOCH, 0); + add(tw.getRawPath(), ours, DirCacheEntry.STAGE_2, EPOCH, 0); + add(tw.getRawPath(), theirs, DirCacheEntry.STAGE_3, EPOCH, 0); + // attribute merge issues are conflicts but not failures + unmergedPaths.add(tw.getPathString()); + return true; } // Check worktree before modifying files @@ -819,7 +821,7 @@ protected boolean processEntry(CanonicalTreeParser base, return true; } - private MergeResult createGitLinksMergeResult( + private static MergeResult createGitLinksMergeResult( CanonicalTreeParser base, CanonicalTreeParser ours, CanonicalTreeParser theirs) { return new MergeResult<>(Arrays.asList( From c084729f79c840e74557a5df38b5259247c4ddbd Mon Sep 17 00:00:00 2001 From: Demetr Starshov Date: Wed, 12 Aug 2020 14:47:15 -0700 Subject: [PATCH 4/6] ResolveMerger: choose OURS on gitlink when ignoreConflicts Option ignoreConflicts is used when a caller want to create a virtual commit and use it in a future merge (recursive merge) or show it on UI (e.g. Gerrit). According to contract in case of ignoreConflicts ResolveMerger should populate only stage 0 for entries with merge conflicts as if there is no conflict. Current implementation breaks this contract for cases when gitlink revision is ambiguous. Therefore, always select 'ours' when we merge in ignoreConflicts mode. This will satisfy the contract contract, so recursive merge can succeed, however it is an arbitrary decision, so it is not guaranteed to select best GITLINK in all cases. GITLINK merging is a special case of recursive merge because of limitations of GITLINK type of entry. It can't contain more than 1 sha-1 so jgit can't write merge conflicts in place like it can with a blob. Ideally we could signal the conflict with a special value (like '0000...'), but that must be supported by all tooling (git fsck, c-git)." Signed-off-by: Demetr Starshov Change-Id: Id4e9bebc8e828f7a1ef9f83259159137df477d89 --- .../eclipse/jgit/merge/GitlinkMergeTest.java | 5 ----- .../org/eclipse/jgit/merge/ResolveMerger.java | 20 ++++++++++--------- 2 files changed, 11 insertions(+), 14 deletions(-) diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/merge/GitlinkMergeTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/merge/GitlinkMergeTest.java index 4165812e2..3f291576b 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/merge/GitlinkMergeTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/merge/GitlinkMergeTest.java @@ -27,7 +27,6 @@ import org.eclipse.jgit.lib.PersonIdent; import org.eclipse.jgit.test.resources.SampleDataRepositoryTestCase; import org.eclipse.jgit.treewalk.TreeWalk; -import org.junit.Ignore; import org.junit.Test; public class GitlinkMergeTest extends SampleDataRepositoryTestCase { @@ -63,7 +62,6 @@ public void testGitLinkMerging_AddNew_ignoreConflicts() throws Exception { } @Test - @Ignore("broken - doesn't ignore conflicts") public void testGitLinkMerging_UpdateUpdate_ignoreConflicts() throws Exception { assertGitLinkValue(testGitLink(LINK_ID1, LINK_ID2, LINK_ID3, @@ -71,7 +69,6 @@ public void testGitLinkMerging_UpdateUpdate_ignoreConflicts() } @Test - @Ignore("broken - doesn't ignore conflicts") public void testGitLinkMerging_bothAddedSameLink_ignoreConflicts() throws Exception { assertGitLinkValue(testGitLink(null, LINK_ID2, LINK_ID2, @@ -79,7 +76,6 @@ public void testGitLinkMerging_bothAddedSameLink_ignoreConflicts() } @Test - @Ignore("broken - doesn't ignore conflicts") public void testGitLinkMerging_bothAddedDifferentLink_ignoreConflicts() throws Exception { assertGitLinkValue(testGitLink(null, LINK_ID2, LINK_ID3, @@ -222,7 +218,6 @@ public void testGitLinkMerging_linkWithLink() throws Exception { } @Test - @Ignore("broken - try to do content-merge with GITLINK") public void testGitLinkMerging_blobWithBlobFromLink() throws Exception { DirCache treeB = db.readDirCache(); DirCache treeO = db.readDirCache(); 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 508f7ba62..a88da6581 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/merge/ResolveMerger.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/merge/ResolveMerger.java @@ -588,7 +588,8 @@ protected boolean processEntry(CanonicalTreeParser base, final int modeO = tw.getRawMode(T_OURS); final int modeT = tw.getRawMode(T_THEIRS); final int modeB = tw.getRawMode(T_BASE); - + boolean gitLinkMerging = isGitLink(modeO) || isGitLink(modeT) + || isGitLink(modeB); if (modeO == 0 && modeT == 0 && modeB == 0) // File is either untracked or new, staged but uncommitted return true; @@ -737,20 +738,20 @@ protected boolean processEntry(CanonicalTreeParser base, return false; } - boolean gitlinkConflict = isGitLink(modeO) || isGitLink(modeT); - // Don't attempt to resolve submodule link conflicts - if (gitlinkConflict) { + if (gitLinkMerging && ignoreConflicts) { + // Always select 'ours' in case of GITLINK merge failures so + // a caller can use virtual commit. + add(tw.getRawPath(), ours, DirCacheEntry.STAGE_0, EPOCH, 0); + return true; + } else if (gitLinkMerging) { add(tw.getRawPath(), base, DirCacheEntry.STAGE_1, EPOCH, 0); add(tw.getRawPath(), ours, DirCacheEntry.STAGE_2, EPOCH, 0); add(tw.getRawPath(), theirs, DirCacheEntry.STAGE_3, EPOCH, 0); - MergeResult result = createGitLinksMergeResult( base, ours, theirs); result.setContainsConflicts(true); mergeResults.put(tw.getPathString(), result); - if (!ignoreConflicts) { - unmergedPaths.add(tw.getPathString()); - } + unmergedPaths.add(tw.getPathString()); return true; } else if (!attributes.canBeContentMerged()) { add(tw.getRawPath(), base, DirCacheEntry.STAGE_1, EPOCH, 0); @@ -806,7 +807,8 @@ protected boolean processEntry(CanonicalTreeParser base, } if (nonTree(modeT)) { if (e != null) { - addToCheckout(tw.getPathString(), e, attributes); + addToCheckout(tw.getPathString(), e, + attributes); } } } From 2ae84c320a34ce3e29eaae72292ff7f1b5d723ba Mon Sep 17 00:00:00 2001 From: Demetr Starshov Date: Wed, 26 Aug 2020 18:32:39 -0700 Subject: [PATCH 5/6] ResolveMerger: Adding test cases for GITLINK deletion Add test cases which cover content-merge resolve logic for deletion. Sign-off-by: Demetr Starshov Change-Id: I2f2b37e29adc973a5a0cfcc5c8bc32a2c38efdfa --- .../eclipse/jgit/merge/GitlinkMergeTest.java | 91 +++++++++++++++++++ 1 file changed, 91 insertions(+) diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/merge/GitlinkMergeTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/merge/GitlinkMergeTest.java index 3f291576b..7ca1e8ce3 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/merge/GitlinkMergeTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/merge/GitlinkMergeTest.java @@ -27,6 +27,7 @@ import org.eclipse.jgit.lib.PersonIdent; import org.eclipse.jgit.test.resources.SampleDataRepositoryTestCase; import org.eclipse.jgit.treewalk.TreeWalk; +import org.junit.Ignore; import org.junit.Test; public class GitlinkMergeTest extends SampleDataRepositoryTestCase { @@ -36,6 +37,33 @@ public class GitlinkMergeTest extends SampleDataRepositoryTestCase { private static final String SUBMODULE_PATH = "submodule.link"; + @Test + @Ignore("Broken") + public void testGitLinkMerging_AddNew() throws Exception { + assertGitLinkValue( + testGitLink(null, null, LINK_ID3, newResolveMerger(), true), + LINK_ID3); + } + + @Test + @Ignore("Broken") + public void testGitLinkMerging_Delete() throws Exception { + assertGitLinkDoesntExist(testGitLink(LINK_ID1, LINK_ID1, null, + newResolveMerger(), true)); + } + + @Test + @Ignore("Broken") + public void testGitLinkMerging_UpdateDelete() throws Exception { + testGitLink(LINK_ID1, LINK_ID2, null, newResolveMerger(), false); + } + + @Test + @Ignore("Broken") + public void testGitLinkMerging_DeleteUpdate() throws Exception { + testGitLink(LINK_ID1, null, LINK_ID3, newResolveMerger(), false); + } + @Test public void testGitLinkMerging_UpdateUpdate() throws Exception { testGitLink(LINK_ID1, LINK_ID2, LINK_ID3, newResolveMerger(), false); @@ -61,6 +89,29 @@ public void testGitLinkMerging_AddNew_ignoreConflicts() throws Exception { LINK_ID3); } + @Test + @Ignore("Broken") + public void testGitLinkMerging_Delete_ignoreConflicts() throws Exception { + assertGitLinkDoesntExist(testGitLink(LINK_ID1, LINK_ID1, null, + newIgnoreConflictMerger(), true)); + } + + @Test + @Ignore("Broken") + public void testGitLinkMerging_UpdateDelete_ignoreConflicts() + throws Exception { + assertGitLinkValue(testGitLink(LINK_ID1, LINK_ID2, null, + newIgnoreConflictMerger(), true), LINK_ID2); + } + + @Test + @Ignore("Broken") + public void testGitLinkMerging_DeleteUpdate_ignoreConflicts() + throws Exception { + assertGitLinkDoesntExist(testGitLink(LINK_ID1, null, LINK_ID3, + newIgnoreConflictMerger(), true)); + } + @Test public void testGitLinkMerging_UpdateUpdate_ignoreConflicts() throws Exception { @@ -247,6 +298,36 @@ public void testGitLinkMerging_blobWithBlobFromLink() throws Exception { assertFalse(merge); } + @Test + @Ignore("Broken") + public void testGitLinkMerging_linkBlobDeleted() throws Exception { + // We changed a link to a blob, others has deleted this link. + DirCache treeB = db.readDirCache(); + DirCache treeO = db.readDirCache(); + DirCache treeT = db.readDirCache(); + + DirCacheBuilder bTreeBuilder = treeB.builder(); + DirCacheBuilder oTreeBuilder = treeO.builder(); + DirCacheBuilder tTreeBuilder = treeT.builder(); + + maybeAddLink(bTreeBuilder, LINK_ID1); + oTreeBuilder.add( + createEntry(SUBMODULE_PATH, FileMode.REGULAR_FILE, "blob 2")); + + bTreeBuilder.finish(); + oTreeBuilder.finish(); + tTreeBuilder.finish(); + + ObjectInserter ow = db.newObjectInserter(); + ObjectId b = commit(ow, treeB, new ObjectId[] {}); + ObjectId o = commit(ow, treeO, new ObjectId[] { b }); + ObjectId t = commit(ow, treeT, new ObjectId[] { b }); + + Merger resolveMerger = MergeStrategy.RESOLVE.newMerger(db); + boolean merge = resolveMerger.merge(new ObjectId[] { o, t }); + assertFalse(merge); + } + private void maybeAddLink(DirCacheBuilder builder, @Nullable String linkId) { if (linkId == null) { @@ -271,6 +352,16 @@ private void assertGitLinkValue(Merger resolveMerger, String expectedValue) } } + private void assertGitLinkDoesntExist(Merger resolveMerger) + throws Exception { + try (TreeWalk tw = new TreeWalk(db)) { + tw.setRecursive(true); + tw.reset(resolveMerger.getResultTreeId()); + + assertFalse(tw.next()); + } + } + private static ObjectId commit(ObjectInserter odi, DirCache treeB, ObjectId[] parentIds) throws Exception { CommitBuilder c = new CommitBuilder(); From 214c4afc2c9a7306ac989df218b7cab30ee5e026 Mon Sep 17 00:00:00 2001 From: Demetr Starshov Date: Wed, 12 Aug 2020 15:01:10 -0700 Subject: [PATCH 6/6] ResolveMerger: do not content-merge gitlinks on del/mod conflicts Previously ResolveMerger tried to make a fulltext merge entry in case one of sides got deleted regardless of file mode. This is not applicable for GITLINK type of entry. After this change it is rendering appropriate merge result. Signed-off-by: Demetr Starshov Change-Id: Ibdb4557bf8781bdb48bcee6529e37dc80582ed7e --- .../eclipse/jgit/merge/GitlinkMergeTest.java | 9 --- .../org/eclipse/jgit/merge/ResolveMerger.java | 72 +++++++++++-------- 2 files changed, 44 insertions(+), 37 deletions(-) diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/merge/GitlinkMergeTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/merge/GitlinkMergeTest.java index 7ca1e8ce3..c850b4d74 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/merge/GitlinkMergeTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/merge/GitlinkMergeTest.java @@ -27,7 +27,6 @@ import org.eclipse.jgit.lib.PersonIdent; import org.eclipse.jgit.test.resources.SampleDataRepositoryTestCase; import org.eclipse.jgit.treewalk.TreeWalk; -import org.junit.Ignore; import org.junit.Test; public class GitlinkMergeTest extends SampleDataRepositoryTestCase { @@ -38,7 +37,6 @@ public class GitlinkMergeTest extends SampleDataRepositoryTestCase { private static final String SUBMODULE_PATH = "submodule.link"; @Test - @Ignore("Broken") public void testGitLinkMerging_AddNew() throws Exception { assertGitLinkValue( testGitLink(null, null, LINK_ID3, newResolveMerger(), true), @@ -46,20 +44,17 @@ public void testGitLinkMerging_AddNew() throws Exception { } @Test - @Ignore("Broken") public void testGitLinkMerging_Delete() throws Exception { assertGitLinkDoesntExist(testGitLink(LINK_ID1, LINK_ID1, null, newResolveMerger(), true)); } @Test - @Ignore("Broken") public void testGitLinkMerging_UpdateDelete() throws Exception { testGitLink(LINK_ID1, LINK_ID2, null, newResolveMerger(), false); } @Test - @Ignore("Broken") public void testGitLinkMerging_DeleteUpdate() throws Exception { testGitLink(LINK_ID1, null, LINK_ID3, newResolveMerger(), false); } @@ -90,14 +85,12 @@ public void testGitLinkMerging_AddNew_ignoreConflicts() throws Exception { } @Test - @Ignore("Broken") public void testGitLinkMerging_Delete_ignoreConflicts() throws Exception { assertGitLinkDoesntExist(testGitLink(LINK_ID1, LINK_ID1, null, newIgnoreConflictMerger(), true)); } @Test - @Ignore("Broken") public void testGitLinkMerging_UpdateDelete_ignoreConflicts() throws Exception { assertGitLinkValue(testGitLink(LINK_ID1, LINK_ID2, null, @@ -105,7 +98,6 @@ public void testGitLinkMerging_UpdateDelete_ignoreConflicts() } @Test - @Ignore("Broken") public void testGitLinkMerging_DeleteUpdate_ignoreConflicts() throws Exception { assertGitLinkDoesntExist(testGitLink(LINK_ID1, null, LINK_ID3, @@ -299,7 +291,6 @@ public void testGitLinkMerging_blobWithBlobFromLink() throws Exception { } @Test - @Ignore("Broken") public void testGitLinkMerging_linkBlobDeleted() throws Exception { // We changed a link to a blob, others has deleted this link. DirCache treeB = db.readDirCache(); 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 a88da6581..6c217fdf2 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/merge/ResolveMerger.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/merge/ResolveMerger.java @@ -757,6 +757,7 @@ protected boolean processEntry(CanonicalTreeParser base, add(tw.getRawPath(), base, DirCacheEntry.STAGE_1, EPOCH, 0); add(tw.getRawPath(), ours, DirCacheEntry.STAGE_2, EPOCH, 0); add(tw.getRawPath(), theirs, DirCacheEntry.STAGE_3, EPOCH, 0); + // attribute merge issues are conflicts but not failures unmergedPaths.add(tw.getPathString()); return true; @@ -783,40 +784,55 @@ protected boolean processEntry(CanonicalTreeParser base, // OURS or THEIRS has been deleted if (((modeO != 0 && !tw.idEqual(T_BASE, T_OURS)) || (modeT != 0 && !tw .idEqual(T_BASE, T_THEIRS)))) { - MergeResult result = contentMerge(base, ours, theirs, - attributes); - - if (ignoreConflicts) { - // In case a conflict is detected the working tree file is - // again filled with new content (containing conflict - // markers). But also stage 0 of the index is filled with - // that content. - result.setContainsConflicts(false); - updateIndex(base, ours, theirs, result, attributes); - } else { + if (gitLinkMerging && ignoreConflicts) { + add(tw.getRawPath(), ours, DirCacheEntry.STAGE_0, EPOCH, 0); + } else if (gitLinkMerging) { add(tw.getRawPath(), base, DirCacheEntry.STAGE_1, EPOCH, 0); add(tw.getRawPath(), ours, DirCacheEntry.STAGE_2, EPOCH, 0); - DirCacheEntry e = add(tw.getRawPath(), theirs, - DirCacheEntry.STAGE_3, EPOCH, 0); + add(tw.getRawPath(), theirs, DirCacheEntry.STAGE_3, EPOCH, 0); + MergeResult result = createGitLinksMergeResult( + base, ours, theirs); + result.setContainsConflicts(true); + mergeResults.put(tw.getPathString(), result); + unmergedPaths.add(tw.getPathString()); + } else { + MergeResult result = contentMerge(base, ours, + theirs, attributes); - // OURS was deleted checkout THEIRS - if (modeO == 0) { - // Check worktree before checking out THEIRS - if (isWorktreeDirty(work, ourDce)) { - return false; - } - if (nonTree(modeT)) { - if (e != null) { - addToCheckout(tw.getPathString(), e, - attributes); + if (ignoreConflicts) { + // In case a conflict is detected the working tree file + // is again filled with new content (containing conflict + // markers). But also stage 0 of the index is filled + // with that content. + result.setContainsConflicts(false); + updateIndex(base, ours, theirs, result, attributes); + } else { + add(tw.getRawPath(), base, DirCacheEntry.STAGE_1, EPOCH, + 0); + add(tw.getRawPath(), ours, DirCacheEntry.STAGE_2, EPOCH, + 0); + DirCacheEntry e = add(tw.getRawPath(), theirs, + DirCacheEntry.STAGE_3, EPOCH, 0); + + // OURS was deleted checkout THEIRS + if (modeO == 0) { + // Check worktree before checking out THEIRS + if (isWorktreeDirty(work, ourDce)) { + return false; + } + if (nonTree(modeT)) { + if (e != null) { + addToCheckout(tw.getPathString(), e, + attributes); + } } } + + unmergedPaths.add(tw.getPathString()); + + // generate a MergeResult for the deleted file + mergeResults.put(tw.getPathString(), result); } - - unmergedPaths.add(tw.getPathString()); - - // generate a MergeResult for the deleted file - mergeResults.put(tw.getPathString(), result); } } }