From 4678f4b7354404f1ea40af6f213d3b2f7d674e3f Mon Sep 17 00:00:00 2001 From: Thomas Wolf Date: Sat, 25 Aug 2018 01:58:09 +0200 Subject: [PATCH] Fix IndexDiffs for git links After cloning a repo with a submodule, non-recursively, JGit would encounter in its TreeWalk in IndexDiff: * first, a missing gitlink (in index & HEAD, not in working tree) * second, the untracked folder (not in index and head, in working tree) As a result, it would report the submodule as missing. Canonical git reports a clean workspace. The root cause of this is that the path of a gitlink "x" did not compare equal to the path of a tree "x" in JGit. Correct Paths.compare() to account for that. If two paths are otherwise equal, then let gitlinks match both trees and files. Matching trees solves the bug. Matching files is necessary to handle the case where the gitlink directory was replaced by a file; see the new test case IndexDiffSubmoduleTest.testSubmoduleReplacedByFile(). Comparisons of unequal paths are left untouched, so the sort order is unchanged. After the fix, another bug(?) in WorkingTreeIterator became apparent: with core.dirNoGitLinks = true, it was no longer possible to overwrite a gitlink in the index. This is now fixed in WorkingTreeIterator. Add new test cases for the bug itself and for some related cases (submodule directory deleted or replaced by a file) in IndexDiffSubmoduleTest. Add a test for missing files in IndexDiffTest, and adapt the PathsTest to test matching gitlinks. Bug: 467631 Change-Id: I0549d10d46b1858e5eec3cc15095aa9f1d5f5280 Signed-off-by: Thomas Wolf --- .../org/eclipse/jgit/api/AddCommandTest.java | 3 +- .../eclipse/jgit/api/CleanCommandTest.java | 21 ++++++++ .../jgit/lib/IndexDiffSubmoduleTest.java | 53 +++++++++++++++++++ .../org/eclipse/jgit/lib/IndexDiffTest.java | 22 ++++++++ .../tst/org/eclipse/jgit/util/PathsTest.java | 18 +++++++ .../jgit/treewalk/WorkingTreeIterator.java | 2 +- .../src/org/eclipse/jgit/util/Paths.java | 14 ++++- 7 files changed, 129 insertions(+), 4 deletions(-) diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/AddCommandTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/AddCommandTest.java index 16cec6485..1a5793ce3 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/AddCommandTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/AddCommandTest.java @@ -1245,7 +1245,8 @@ public class AddCommandTest extends RepositoryTestCase { ConfigConstants.CONFIG_KEY_DIRNOGITLINKS, true); config.save(); - assert (db.getConfig().get(WorkingTreeOptions.KEY).isDirNoGitLinks()); + assertTrue( + db.getConfig().get(WorkingTreeOptions.KEY).isDirNoGitLinks()); try (Git git = new Git(db)) { git.add().addFilepattern("nested-repo").call(); diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/CleanCommandTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/CleanCommandTest.java index 065b5b4c3..139f199f7 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/CleanCommandTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/CleanCommandTest.java @@ -233,6 +233,27 @@ public class CleanCommandTest extends RepositoryTestCase { assertTrue(cleanedFiles.contains("ignored-dir/")); } + @Test + public void testCleanDirsWithPrefixFolder() throws Exception { + String path = "sub/foo.txt"; + writeTrashFile(path, "sub is a prefix of sub-noclean"); + git.add().addFilepattern(path).call(); + Status beforeCleanStatus = git.status().call(); + assertTrue(beforeCleanStatus.getAdded().contains(path)); + + Set cleanedFiles = git.clean().setCleanDirectories(true).call(); + + // The "sub" directory should not be cleaned. + assertTrue(!cleanedFiles.contains(path + "/")); + + assertTrue(cleanedFiles.contains("File2.txt")); + assertTrue(cleanedFiles.contains("File3.txt")); + assertTrue(!cleanedFiles.contains("sub-noclean/File1.txt")); + assertTrue(cleanedFiles.contains("sub-noclean/File2.txt")); + assertTrue(cleanedFiles.contains("sub-clean/")); + assertTrue(cleanedFiles.size() == 4); + } + @Test public void testCleanDirsWithSubmodule() throws Exception { SubmoduleAddCommand command = new SubmoduleAddCommand(db); diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/lib/IndexDiffSubmoduleTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/lib/IndexDiffSubmoduleTest.java index d89aabe75..93ada4f30 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/lib/IndexDiffSubmoduleTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/lib/IndexDiffSubmoduleTest.java @@ -51,6 +51,7 @@ import java.io.File; import java.io.IOException; import java.util.Set; +import org.eclipse.jgit.api.CloneCommand; import org.eclipse.jgit.api.Git; import org.eclipse.jgit.api.errors.GitAPIException; import org.eclipse.jgit.errors.NoWorkTreeException; @@ -109,6 +110,58 @@ public class IndexDiffSubmoduleTest extends RepositoryTestCase { assertFalse(indexDiff.diff()); } + private Repository cloneWithoutCloningSubmodule() throws Exception { + File directory = createTempDirectory( + "testCloneWithoutCloningSubmodules"); + CloneCommand clone = Git.cloneRepository(); + clone.setDirectory(directory); + clone.setCloneSubmodules(false); + clone.setURI(db.getDirectory().toURI().toString()); + Git git2 = clone.call(); + addRepoToClose(git2.getRepository()); + return git2.getRepository(); + } + + @Theory + public void testCleanAfterClone(IgnoreSubmoduleMode mode) throws Exception { + Repository db2 = cloneWithoutCloningSubmodule(); + IndexDiff indexDiff = new IndexDiff(db2, Constants.HEAD, + new FileTreeIterator(db2)); + indexDiff.setIgnoreSubmoduleMode(mode); + assertFalse(indexDiff.diff()); + } + + @Theory + public void testMissingIfDirectoryGone(IgnoreSubmoduleMode mode) + throws Exception { + recursiveDelete(submodule_trash); + IndexDiff indexDiff = new IndexDiff(db, Constants.HEAD, + new FileTreeIterator(db)); + indexDiff.setIgnoreSubmoduleMode(mode); + boolean hasChanges = indexDiff.diff(); + if (mode != IgnoreSubmoduleMode.ALL) { + assertTrue(hasChanges); + assertEquals("[modules/submodule]", + indexDiff.getMissing().toString()); + } else { + assertFalse(hasChanges); + } + } + + @Theory + public void testSubmoduleReplacedByFile(IgnoreSubmoduleMode mode) + throws Exception { + recursiveDelete(submodule_trash); + writeTrashFile("modules/submodule", "nonsense"); + IndexDiff indexDiff = new IndexDiff(db, Constants.HEAD, + new FileTreeIterator(db)); + indexDiff.setIgnoreSubmoduleMode(mode); + assertTrue(indexDiff.diff()); + assertEquals("[]", indexDiff.getMissing().toString()); + assertEquals("[]", indexDiff.getUntracked().toString()); + assertEquals("[modules/submodule]", indexDiff.getModified().toString()); + } + @Theory public void testDirtyRootWorktree(IgnoreSubmoduleMode mode) throws IOException { diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/lib/IndexDiffTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/lib/IndexDiffTest.java index 580b08b42..ba5aaf1b1 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/lib/IndexDiffTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/lib/IndexDiffTest.java @@ -118,6 +118,28 @@ public class IndexDiffTest extends RepositoryTestCase { assertEquals(Collections.EMPTY_SET, diff.getUntrackedFolders()); } + @Test + public void testMissing() throws Exception { + File file2 = writeTrashFile("file2", "file2"); + File file3 = writeTrashFile("dir/file3", "dir/file3"); + Git git = Git.wrap(db); + git.add().addFilepattern("file2").addFilepattern("dir/file3").call(); + git.commit().setMessage("commit").call(); + assertTrue(file2.delete()); + assertTrue(file3.delete()); + IndexDiff diff = new IndexDiff(db, Constants.HEAD, + new FileTreeIterator(db)); + diff.diff(); + assertEquals(2, diff.getMissing().size()); + assertTrue(diff.getMissing().contains("file2")); + assertTrue(diff.getMissing().contains("dir/file3")); + assertEquals(0, diff.getChanged().size()); + assertEquals(0, diff.getModified().size()); + assertEquals(0, diff.getAdded().size()); + assertEquals(0, diff.getRemoved().size()); + assertEquals(Collections.EMPTY_SET, diff.getUntrackedFolders()); + } + @Test public void testRemoved() throws IOException { writeTrashFile("file2", "file2"); diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/util/PathsTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/util/PathsTest.java index 7542ec891..f78cbe260 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/util/PathsTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/util/PathsTest.java @@ -88,15 +88,30 @@ public class PathsTest { assertEquals(0, compare( a, 0, a.length, FileMode.TREE.getBits(), b, 0, b.length, FileMode.TREE.getBits())); + assertEquals(0, compare( + a, 0, a.length, FileMode.TREE.getBits(), + b, 0, b.length, FileMode.GITLINK.getBits())); + assertEquals(0, compare( + a, 0, a.length, FileMode.GITLINK.getBits(), + b, 0, b.length, FileMode.GITLINK.getBits())); + assertEquals(0, compare( + a, 0, a.length, FileMode.GITLINK.getBits(), + b, 0, b.length, FileMode.TREE.getBits())); assertEquals(0, compare( a, 0, a.length, FileMode.REGULAR_FILE.getBits(), b, 0, b.length, FileMode.REGULAR_FILE.getBits())); assertEquals(-47, compare( a, 0, a.length, FileMode.REGULAR_FILE.getBits(), b, 0, b.length, FileMode.TREE.getBits())); + assertEquals(0, compare( + a, 0, a.length, FileMode.REGULAR_FILE.getBits(), + b, 0, b.length, FileMode.GITLINK.getBits())); assertEquals(47, compare( a, 0, a.length, FileMode.TREE.getBits(), b, 0, b.length, FileMode.REGULAR_FILE.getBits())); + assertEquals(0, compare( + a, 0, a.length, FileMode.GITLINK.getBits(), + b, 0, b.length, FileMode.REGULAR_FILE.getBits())); assertEquals(0, compareSameName( a, 0, a.length, @@ -104,6 +119,9 @@ public class PathsTest { assertEquals(0, compareSameName( a, 0, a.length, b, 0, b.length, FileMode.REGULAR_FILE.getBits())); + assertEquals(0, compareSameName( + a, 0, a.length, + b, 0, b.length, FileMode.GITLINK.getBits())); a = Constants.encode("a.c"); b = Constants.encode("a"); diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/treewalk/WorkingTreeIterator.java b/org.eclipse.jgit/src/org/eclipse/jgit/treewalk/WorkingTreeIterator.java index 7c6bfb9d6..1fa1db584 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/treewalk/WorkingTreeIterator.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/treewalk/WorkingTreeIterator.java @@ -1045,7 +1045,7 @@ public abstract class WorkingTreeIterator extends AbstractTreeIterator { } } if (FileMode.GITLINK == iMode - && FileMode.TREE == wtMode) { + && FileMode.TREE == wtMode && !getOptions().isDirNoGitLinks()) { return iMode; } if (FileMode.TREE == iMode diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/util/Paths.java b/org.eclipse.jgit/src/org/eclipse/jgit/util/Paths.java index 6be7ddbe1..be1b95e0d 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/util/Paths.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/util/Paths.java @@ -1,5 +1,5 @@ /* - * Copyright (C) 2016, Google Inc. + * Copyright (C) 2016, 2018 Google Inc. * and other copyright owners as documented in the project's IP log. * * This program and the accompanying materials are made available @@ -43,6 +43,7 @@ package org.eclipse.jgit.util; +import static org.eclipse.jgit.lib.FileMode.TYPE_GITLINK; import static org.eclipse.jgit.lib.FileMode.TYPE_MASK; import static org.eclipse.jgit.lib.FileMode.TYPE_TREE; @@ -106,7 +107,7 @@ public class Paths { aPath, aPos, aEnd, aMode, bPath, bPos, bEnd, bMode); if (cmp == 0) { - cmp = lastPathChar(aMode) - lastPathChar(bMode); + cmp = modeCompare(aMode, bMode); } return cmp; } @@ -183,6 +184,15 @@ public class Paths { return 0; } + private static int modeCompare(int aMode, int bMode) { + if ((aMode & TYPE_MASK) == TYPE_GITLINK + || (bMode & TYPE_MASK) == TYPE_GITLINK) { + // Git links can be equal to files or folders + return 0; + } + return lastPathChar(aMode) - lastPathChar(bMode); + } + private Paths() { } }