From a8e683fef6acf3e9f00ac2648fff60b13a28fb13 Mon Sep 17 00:00:00 2001 From: Thomas Wolf Date: Tue, 16 Aug 2022 01:02:21 +0200 Subject: [PATCH] [merge] Fix merge conflicts with symlinks Previous code would do a content merge on symlinks, and write the merge result to the working tree as a file. C git doesn't do this; it leaves a symlink in the working tree unchanged, or in a delete-modify conflict it would check out "theirs". Moreover, previous code would write the merge result to the link target, not to the link. This would overwrite an existing link target, or fail if the link pointed to a directory. In link/file conflicts or file/link conflicts, C git always puts the file into the working tree. Change conflict handling accordingly. Add tests for all the conflict cases. Bug: 580347 Change-Id: I3cffcb4bcf8e336a85186031fff23f0c4b6ee19d Signed-off-by: Thomas Wolf --- .../eclipse/jgit/junit/TestRepository.java | 18 ++ .../eclipse/jgit/merge/SymlinkMergeTest.java | 296 ++++++++++++++++++ .../org/eclipse/jgit/merge/ResolveMerger.java | 168 ++++++---- 3 files changed, 429 insertions(+), 53 deletions(-) create mode 100644 org.eclipse.jgit.test/tst/org/eclipse/jgit/merge/SymlinkMergeTest.java diff --git a/org.eclipse.jgit.junit/src/org/eclipse/jgit/junit/TestRepository.java b/org.eclipse.jgit.junit/src/org/eclipse/jgit/junit/TestRepository.java index 54e4a09ee5..483b9a7c81 100644 --- a/org.eclipse.jgit.junit/src/org/eclipse/jgit/junit/TestRepository.java +++ b/org.eclipse.jgit.junit/src/org/eclipse/jgit/junit/TestRepository.java @@ -275,6 +275,24 @@ public class TestRepository implements AutoCloseable { return e; } + /** + * Construct a symlink mode tree entry. + * + * @param path + * path of the symlink. + * @param blob + * a blob, previously constructed in the repository. + * @return the entry. + * @throws Exception + * @since 6.3 + */ + public DirCacheEntry link(String path, RevBlob blob) throws Exception { + DirCacheEntry e = new DirCacheEntry(path); + e.setFileMode(FileMode.SYMLINK); + e.setObjectId(blob); + return e; + } + /** * Construct a tree from a specific listing of file entries. * diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/merge/SymlinkMergeTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/merge/SymlinkMergeTest.java new file mode 100644 index 0000000000..3cdc8da34e --- /dev/null +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/merge/SymlinkMergeTest.java @@ -0,0 +1,296 @@ +/* + * Copyright (C) 2022 Thomas Wolf 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 + * https://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 static org.junit.Assume.assumeTrue; + +import java.io.File; +import java.io.IOException; +import java.nio.file.Files; +import java.nio.file.LinkOption; + +import org.eclipse.jgit.api.Git; +import org.eclipse.jgit.api.MergeResult; +import org.eclipse.jgit.api.MergeResult.MergeStatus; +import org.eclipse.jgit.api.ResetCommand.ResetType; +import org.eclipse.jgit.junit.RepositoryTestCase; +import org.eclipse.jgit.junit.TestRepository; +import org.eclipse.jgit.lib.ConfigConstants; +import org.eclipse.jgit.lib.Repository; +import org.eclipse.jgit.lib.StoredConfig; +import org.eclipse.jgit.revwalk.RevCommit; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.Parameterized; +import org.junit.runners.Parameterized.Parameter; +import org.junit.runners.Parameterized.Parameters; + +/** + * Tests for merges involving symlinks. + */ +@RunWith(Parameterized.class) +public class SymlinkMergeTest extends RepositoryTestCase { + + @Parameters(name = "target={0}, core.symlinks={1}") + public static Object[][] parameters() { + return new Object[][] { + { Target.NONE, Boolean.TRUE }, + { Target.FILE, Boolean.TRUE }, + { Target.DIRECTORY, Boolean.TRUE }, + { Target.NONE, Boolean.FALSE }, + { Target.FILE, Boolean.FALSE }, + { Target.DIRECTORY, Boolean.FALSE }, + }; + } + + public enum Target { + NONE, FILE, DIRECTORY + } + + @Parameter(0) + public Target target; + + @Parameter(1) + public boolean useSymLinks; + + private void setTargets() throws IOException { + switch (target) { + case DIRECTORY: + assertTrue(new File(trash, "target").mkdir()); + assertTrue(new File(trash, "target1").mkdir()); + assertTrue(new File(trash, "target2").mkdir()); + break; + case FILE: + writeTrashFile("target", "t"); + writeTrashFile("target1", "t1"); + writeTrashFile("target2", "t2"); + break; + default: + break; + } + } + + private void checkTargets() throws IOException { + File t = new File(trash, "target"); + File t1 = new File(trash, "target1"); + File t2 = new File(trash, "target2"); + switch (target) { + case DIRECTORY: + assertTrue(t.isDirectory()); + assertTrue(t1.isDirectory()); + assertTrue(t2.isDirectory()); + break; + case FILE: + checkFile(t, "t"); + checkFile(t1, "t1"); + checkFile(t2, "t2"); + break; + default: + assertFalse(Files.exists(t.toPath(), LinkOption.NOFOLLOW_LINKS)); + assertFalse(Files.exists(t1.toPath(), LinkOption.NOFOLLOW_LINKS)); + assertFalse(Files.exists(t2.toPath(), LinkOption.NOFOLLOW_LINKS)); + break; + } + } + + private void assertSymLink(File link, String content) throws Exception { + if (useSymLinks) { + assertTrue(Files.isSymbolicLink(link.toPath())); + assertEquals(content, db.getFS().readSymLink(link)); + } else { + assertFalse(Files.isSymbolicLink(link.toPath())); + assertTrue(link.isFile()); + checkFile(link, content); + } + } + + // Link/link conflict: C git records the conflict but leaves the link in the + // working tree unchanged. + + @Test + public void mergeWithSymlinkConflict() throws Exception { + assumeTrue(db.getFS().supportsSymlinks() || !useSymLinks); + StoredConfig config = db.getConfig(); + config.setBoolean(ConfigConstants.CONFIG_CORE_SECTION, null, + ConfigConstants.CONFIG_KEY_SYMLINKS, useSymLinks); + config.save(); + try (TestRepository repo = new TestRepository<>(db)) { + db.incrementOpen(); + // Create the links directly in the git repo, then use a hard reset + // to get them into the workspace. This enables us to run these + // tests also with core.symLinks = false. + RevCommit base = repo + .commit(repo.tree(repo.link("link", repo.blob("target")))); + RevCommit side = repo.commit( + repo.tree(repo.link("link", repo.blob("target1"))), base); + RevCommit head = repo.commit( + repo.tree(repo.link("link", repo.blob("target2"))), base); + try (Git git = new Git(db)) { + setTargets(); + git.reset().setMode(ResetType.HARD).setRef(head.name()).call(); + File link = new File(trash, "link"); + assertSymLink(link, "target2"); + MergeResult result = git.merge().include(side) + .setMessage("merged").call(); + assertEquals(MergeStatus.CONFLICTING, result.getMergeStatus()); + // Link should be unmodified + assertSymLink(link, "target2"); + checkTargets(); + assertEquals("[link, mode:120000, stage:1, content:target]" + + "[link, mode:120000, stage:2, content:target2]" + + "[link, mode:120000, stage:3, content:target1]", + indexState(CONTENT)); + } + } + } + + // In file/link conflicts, C git never does a content merge. It records the + // stages in the index, and always puts the file into the workspace. + + @Test + public void mergeWithFileSymlinkConflict() throws Exception { + assumeTrue(db.getFS().supportsSymlinks() || !useSymLinks); + StoredConfig config = db.getConfig(); + config.setBoolean(ConfigConstants.CONFIG_CORE_SECTION, null, + ConfigConstants.CONFIG_KEY_SYMLINKS, useSymLinks); + config.save(); + try (TestRepository repo = new TestRepository<>(db)) { + db.incrementOpen(); + RevCommit base = repo.commit(repo.tree()); + RevCommit side = repo.commit( + repo.tree(repo.link("link", repo.blob("target1"))), base); + RevCommit head = repo.commit( + repo.tree(repo.file("link", repo.blob("not a link"))), + base); + try (Git git = new Git(db)) { + setTargets(); + git.reset().setMode(ResetType.HARD).setRef(head.name()).call(); + File link = new File(trash, "link"); + assertFalse(Files.isSymbolicLink(link.toPath())); + checkFile(link, "not a link"); + MergeResult result = git.merge().include(side) + .setMessage("merged").call(); + assertEquals(MergeStatus.CONFLICTING, result.getMergeStatus()); + // File should be unmodified + assertFalse(Files.isSymbolicLink(link.toPath())); + checkFile(link, "not a link"); + checkTargets(); + assertEquals("[link, mode:100644, stage:2, content:not a link]" + + "[link, mode:120000, stage:3, content:target1]", + indexState(CONTENT)); + } + } + } + + @Test + public void mergeWithSymlinkFileConflict() throws Exception { + assumeTrue(db.getFS().supportsSymlinks() || !useSymLinks); + StoredConfig config = db.getConfig(); + config.setBoolean(ConfigConstants.CONFIG_CORE_SECTION, null, + ConfigConstants.CONFIG_KEY_SYMLINKS, useSymLinks); + config.save(); + try (TestRepository repo = new TestRepository<>(db)) { + db.incrementOpen(); + RevCommit base = repo.commit(repo.tree()); + RevCommit side = repo.commit( + repo.tree(repo.file("link", repo.blob("not a link"))), + base); + RevCommit head = repo.commit( + repo.tree(repo.link("link", repo.blob("target2"))), base); + try (Git git = new Git(db)) { + setTargets(); + git.reset().setMode(ResetType.HARD).setRef(head.name()).call(); + File link = new File(trash, "link"); + assertSymLink(link, "target2"); + MergeResult result = git.merge().include(side) + .setMessage("merged").call(); + assertEquals(MergeStatus.CONFLICTING, result.getMergeStatus()); + // Should now be a file! + assertFalse(Files.isSymbolicLink(link.toPath())); + checkFile(link, "not a link"); + checkTargets(); + assertEquals("[link, mode:120000, stage:2, content:target2]" + + "[link, mode:100644, stage:3, content:not a link]", + indexState(CONTENT)); + } + } + } + + // In Delete/modify conflicts with the non-deleted side a link, C git puts + // the link into the working tree. + + @Test + public void mergeWithSymlinkDeleteModify() throws Exception { + assumeTrue(db.getFS().supportsSymlinks() || !useSymLinks); + StoredConfig config = db.getConfig(); + config.setBoolean(ConfigConstants.CONFIG_CORE_SECTION, null, + ConfigConstants.CONFIG_KEY_SYMLINKS, useSymLinks); + config.save(); + try (TestRepository repo = new TestRepository<>(db)) { + db.incrementOpen(); + RevCommit base = repo + .commit(repo.tree(repo.link("link", repo.blob("target")))); + RevCommit side = repo.commit( + repo.tree(repo.link("link", repo.blob("target1"))), base); + RevCommit head = repo.commit(repo.tree(), base); + try (Git git = new Git(db)) { + setTargets(); + git.reset().setMode(ResetType.HARD).setRef(head.name()).call(); + File link = new File(trash, "link"); + assertFalse( + Files.exists(link.toPath(), LinkOption.NOFOLLOW_LINKS)); + MergeResult result = git.merge().include(side) + .setMessage("merged").call(); + assertEquals(MergeStatus.CONFLICTING, result.getMergeStatus()); + // Link should have the content from side + assertSymLink(link, "target1"); + checkTargets(); + assertEquals("[link, mode:120000, stage:1, content:target]" + + "[link, mode:120000, stage:3, content:target1]", + indexState(CONTENT)); + } + } + } + + @Test + public void mergeWithSymlinkModifyDelete() throws Exception { + assumeTrue(db.getFS().supportsSymlinks() || !useSymLinks); + StoredConfig config = db.getConfig(); + config.setBoolean(ConfigConstants.CONFIG_CORE_SECTION, null, + ConfigConstants.CONFIG_KEY_SYMLINKS, useSymLinks); + config.save(); + try (TestRepository repo = new TestRepository<>(db)) { + db.incrementOpen(); + RevCommit base = repo + .commit(repo.tree(repo.link("link", repo.blob("target")))); + RevCommit side = repo.commit(repo.tree(), base); + RevCommit head = repo.commit( + repo.tree(repo.link("link", repo.blob("target2"))), base); + try (Git git = new Git(db)) { + setTargets(); + git.reset().setMode(ResetType.HARD).setRef(head.name()).call(); + File link = new File(trash, "link"); + assertSymLink(link, "target2"); + MergeResult result = git.merge().include(side) + .setMessage("merged").call(); + assertEquals(MergeStatus.CONFLICTING, result.getMergeStatus()); + // Link should be unmodified + assertSymLink(link, "target2"); + checkTargets(); + assertEquals("[link, mode:120000, stage:1, content:target]" + + "[link, mode:120000, stage:2, content:target2]", + indexState(CONTENT)); + } + } + } +} 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 23f8e4a5d5..8b9b569c38 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/merge/ResolveMerger.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/merge/ResolveMerger.java @@ -3,7 +3,7 @@ * Copyright (C) 2010-2012, Matthias Sohn * Copyright (C) 2012, Research In Motion Limited * Copyright (C) 2017, Obeo (mathieu.cartaud@obeo.fr) - * Copyright (C) 2018, 2022 Thomas Wolf and others + * Copyright (C) 2018, 2022 Thomas Wolf 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 @@ -320,6 +320,25 @@ public class ResolveMerger extends ThreeWayMerger { return null; } + /** + * Adds the conflict stages for the current path of {@link #tw} to the index + * builder and returns the "theirs" stage; if present. + * + * @param base + * of the conflict + * @param ours + * of the conflict + * @param theirs + * of the conflict + * @return the {@link DirCacheEntry} for the "theirs" stage, or {@code null} + */ + private DirCacheEntry addConflict(CanonicalTreeParser base, + CanonicalTreeParser ours, CanonicalTreeParser theirs) { + add(tw.getRawPath(), base, DirCacheEntry.STAGE_1, EPOCH, 0); + add(tw.getRawPath(), ours, DirCacheEntry.STAGE_2, EPOCH, 0); + return add(tw.getRawPath(), theirs, DirCacheEntry.STAGE_3, EPOCH, 0); + } + /** * adds a entry to the index builder which is a copy of the specified * DirCacheEntry @@ -501,9 +520,7 @@ public class ResolveMerger extends ThreeWayMerger { // length. // This path can be skipped on ignoreConflicts, so the caller // could use virtual commit. - 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); + addConflict(base, ours, theirs); unmergedPaths.add(tw.getPathString()); mergeResults.put(tw.getPathString(), new MergeResult<>(Collections.emptyList())); @@ -608,9 +625,7 @@ public class ResolveMerger extends ThreeWayMerger { 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); + addConflict(base, ours, theirs); MergeResult result = createGitLinksMergeResult( base, ours, theirs); result.setContainsConflicts(true); @@ -631,9 +646,7 @@ public class ResolveMerger extends ThreeWayMerger { default: break; } - 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); + addConflict(base, ours, theirs); // attribute merge issues are conflicts but not failures unmergedPaths.add(tw.getPathString()); @@ -646,30 +659,61 @@ public class ResolveMerger extends ThreeWayMerger { } MergeResult result = null; - try { - result = contentMerge(base, ours, theirs, attributes, - getContentMergeStrategy()); - } catch (BinaryBlobException e) { + boolean hasSymlink = FileMode.SYMLINK.equals(modeO) + || FileMode.SYMLINK.equals(modeT); + 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 theirEntry = add(tw.getRawPath(), theirs, - DirCacheEntry.STAGE_0, EPOCH, 0); - addToCheckout(tw.getPathString(), theirEntry, attributes); - return true; - default: - result = new MergeResult<>(Collections.emptyList()); - result.setContainsConflicts(true); - break; + 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); + } + return true; + default: + result = new MergeResult<>(Collections.emptyList()); + result.setContainsConflicts(true); + break; } } if (ignoreConflicts) { result.setContainsConflicts(false); } - updateIndex(base, ours, theirs, result, attributes[T_OURS]); String currentPath = tw.getPathString(); + if (hasSymlink) { + if (ignoreConflicts) { + if (((modeT & FileMode.TYPE_MASK) == FileMode.TYPE_FILE)) { + DirCacheEntry e = add(tw.getRawPath(), theirs, + DirCacheEntry.STAGE_0, EPOCH, 0); + addToCheckout(currentPath, e, attributes); + } else { + keep(ourDce); + } + } else { + // Record the conflict + DirCacheEntry e = addConflict(base, ours, theirs); + mergeResults.put(currentPath, result); + // 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) + && e != null) { + addToCheckout(currentPath, e, attributes); + } + } + } else { + updateIndex(base, ours, theirs, result, attributes[T_OURS]); + } if (result.containsConflicts() && !ignoreConflicts) { unmergedPaths.add(currentPath); } @@ -683,40 +727,58 @@ public class ResolveMerger extends ThreeWayMerger { 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); - add(tw.getRawPath(), theirs, DirCacheEntry.STAGE_3, EPOCH, 0); + addConflict(base, ours, theirs); MergeResult result = createGitLinksMergeResult( base, ours, theirs); result.setContainsConflicts(true); mergeResults.put(tw.getPathString(), result); unmergedPaths.add(tw.getPathString()); } else { + boolean isSymLink = ((modeO | modeT) + & FileMode.TYPE_MASK) == FileMode.TYPE_SYMLINK; // Content merge strategy does not apply to delete-modify // conflicts! MergeResult result; - try { - result = contentMerge(base, ours, theirs, attributes, - ContentMergeStrategy.CONFLICT); - } catch (BinaryBlobException e) { + if (isSymLink) { + // No need to do a content merge result = new MergeResult<>(Collections.emptyList()); result.setContainsConflicts(true); + } else { + try { + result = contentMerge(base, ours, theirs, + attributes, ContentMergeStrategy.CONFLICT); + } catch (BinaryBlobException e) { + result = new MergeResult<>(Collections.emptyList()); + result.setContainsConflicts(true); + } } 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[T_OURS]); + if (isSymLink) { + if (modeO != 0) { + keep(ourDce); + } else { + // Check out theirs + if (isWorktreeDirty(work, ourDce)) { + return false; + } + DirCacheEntry e = add(tw.getRawPath(), theirs, + DirCacheEntry.STAGE_0, EPOCH, 0); + if (e != null) { + addToCheckout(tw.getPathString(), e, + attributes); + } + } + } else { + // 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. + updateIndex(base, ours, theirs, result, + attributes[T_OURS]); + } } 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); + DirCacheEntry e = addConflict(base, ours, theirs); // OURS was deleted checkout THEIRS if (modeO == 0) { @@ -862,25 +924,25 @@ public class ResolveMerger extends ThreeWayMerger { // A conflict occurred, the file will contain conflict markers // the index will be populated with the three stages and the // workdir (if used) contains the halfway merged content. - 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); + addConflict(base, ours, theirs); mergeResults.put(tw.getPathString(), result); return; } // No conflict occurred, the file will contain fully merged content. // The index will be populated with the new merged version. - Instant lastModified = - mergedFile == null ? null : nonNullRepo().getFS().lastModifiedInstant(mergedFile); + Instant lastModified = mergedFile == null ? null + : nonNullRepo().getFS().lastModifiedInstant(mergedFile); // Set the mode for the new content. Fall back to REGULAR_FILE if // we can't merge modes of OURS and THEIRS. int newMode = mergeFileModes(tw.getRawMode(0), tw.getRawMode(1), tw.getRawMode(2)); FileMode mode = newMode == FileMode.MISSING.getBits() ? FileMode.REGULAR_FILE : FileMode.fromBits(newMode); - workTreeUpdater.insertToIndex(rawMerged.openInputStream(), tw.getPathString().getBytes(UTF_8), mode, - DirCacheEntry.STAGE_0, lastModified, (int) rawMerged.length(), + workTreeUpdater.insertToIndex(rawMerged.openInputStream(), + tw.getPathString().getBytes(UTF_8), mode, + DirCacheEntry.STAGE_0, lastModified, + (int) rawMerged.length(), attributes.get(Constants.ATTR_MERGE)); } finally { if (rawMerged != null) { -- 2.39.5