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 <thomas.wolf@paranor.ch>

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 16cec6485c1bf0a8573f13fda0c905bb242245f5..1a5793ce31d6f1ca3d08e50af685731f04596313 100644 (file)
--- 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 065b5b4c3e6f39b8147fb4577b8e079d2c8e13b4..139f199f7a92b55a45d21ddd149ce3053e82171e 100644 (file)
--- 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<String> 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 d89aabe75f8e9cf042bb7fb68d32005ae5cd086e..93ada4f301bf81d98c0c698c982711b089bdd706 100644 (file)
--- 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 580b08b42f069293a6de94b737b53c61ca34241a..ba5aaf1b18d0396d28631eb088a3e91dde02df24 100644 (file)
--- 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 7542ec89104a48e43c863e101714df21afa1234d..f78cbe2601580adbb32cf3ee0ccf1e495f9c6e6e 100644 (file)
--- 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 7c6bfb9d63935625ef7afc32073bd7197c3b112f..1fa1db5847265cbbdb14c8a7a021ad7f7db124a1 100644 (file)
--- 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 6be7ddbe12d238faac95dca18503b7de655653c8..be1b95e0d4aa10ec00a698e2cc716f285fcff22c 100644 (file)
--- 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() {
        }
 }