From 8277f62897a99539c6a3654e3a9ae8357d0f74be Mon Sep 17 00:00:00 2001 From: Thomas Wolf Date: Sun, 10 Mar 2019 18:02:54 +0100 Subject: [PATCH] Revert 4678f4b and provide another solution for bug 467631 Making gitlinks and folders match in a tree walk was the wrong approach to fix bug 467631. The problem is that in such a conflict the tree walk may then not descend into the folder. Revert the changes to Paths.java and PathsTest.java from commit 4678f4b. Instead test for the problem case from bug 467631 explicitly in IndexDiff. Add Daniel's test case from bug 545162, and add yet another test case for DiffEntry.scan() that covers the problem originally reported in bug 545162. Bug: 545162 Change-Id: Ie2214c5d5ee32ac6596b621f0f1c7b86d38fa9b7 Also-by: Daniel Veihelmann Signed-off-by: Thomas Wolf --- .../org/eclipse/jgit/diff/DiffEntryTest.java | 63 +++++++++++++++++++ .../jgit/lib/IndexDiffSubmoduleTest.java | 34 +++++++++- .../tst/org/eclipse/jgit/util/PathsTest.java | 18 ------ .../src/org/eclipse/jgit/lib/IndexDiff.java | 44 +++++++++++-- .../src/org/eclipse/jgit/util/Paths.java | 14 +---- 5 files changed, 138 insertions(+), 35 deletions(-) diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/diff/DiffEntryTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/diff/DiffEntryTest.java index 73c230ac6..de768118b 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/diff/DiffEntryTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/diff/DiffEntryTest.java @@ -59,9 +59,12 @@ import org.eclipse.jgit.diff.DiffEntry.ChangeType; import org.eclipse.jgit.dircache.DirCache; import org.eclipse.jgit.dircache.DirCacheEditor; import org.eclipse.jgit.dircache.DirCacheEditor.PathEdit; +import org.eclipse.jgit.internal.storage.file.FileRepository; import org.eclipse.jgit.dircache.DirCacheEntry; +import org.eclipse.jgit.junit.JGitTestUtil; import org.eclipse.jgit.junit.RepositoryTestCase; import org.eclipse.jgit.lib.FileMode; +import org.eclipse.jgit.lib.Repository; import org.eclipse.jgit.revwalk.RevCommit; import org.eclipse.jgit.treewalk.EmptyTreeIterator; import org.eclipse.jgit.treewalk.FileTreeIterator; @@ -417,4 +420,64 @@ public class DiffEntryTest extends RepositoryTestCase { assertEquals(FileMode.REGULAR_FILE, diff.getOldMode()); } } + + @Test + public void shouldReportSubmoduleReplacedByFileMove() throws Exception { + // Create a submodule + FileRepository submoduleStandalone = createWorkRepository(); + JGitTestUtil.writeTrashFile(submoduleStandalone, "fileInSubmodule", + "submodule"); + Git submoduleStandaloneGit = Git.wrap(submoduleStandalone); + submoduleStandaloneGit.add().addFilepattern("fileInSubmodule").call(); + submoduleStandaloneGit.commit().setMessage("add file to submodule") + .call(); + + Repository submodule_db = Git.wrap(db).submoduleAdd() + .setPath("modules/submodule") + .setURI(submoduleStandalone.getDirectory().toURI().toString()) + .call(); + File submodule_trash = submodule_db.getWorkTree(); + addRepoToClose(submodule_db); + writeTrashFile("fileInRoot", "root"); + Git rootGit = Git.wrap(db); + rootGit.add().addFilepattern("fileInRoot").call(); + rootGit.commit().setMessage("add submodule and root file").call(); + // Dummy change on fileInRoot + writeTrashFile("fileInRoot", "changed"); + rootGit.add().addFilepattern("fileInRoot").call(); + RevCommit firstCommit = rootGit.commit().setMessage("change root file") + .call(); + // Remove the submodule again and move fileInRoot into that subfolder + rootGit.rm().setCached(true).addFilepattern("modules/submodule").call(); + recursiveDelete(submodule_trash); + JGitTestUtil.deleteTrashFile(db, "fileInRoot"); + // Move the fileInRoot file + writeTrashFile("modules/submodule/fileInRoot", "changed"); + rootGit.rm().addFilepattern("fileInRoot").addFilepattern("modules/") + .call(); + rootGit.add().addFilepattern("modules/").call(); + RevCommit secondCommit = rootGit.commit() + .setMessage("remove submodule and move root file") + .call(); + // Diff should report submodule having been deleted and file moved + // (deleted and added) + try (TreeWalk walk = new TreeWalk(db)) { + walk.addTree(firstCommit.getTree()); + walk.addTree(secondCommit.getTree()); + walk.setRecursive(true); + List diffs = DiffEntry.scan(walk); + assertEquals(3, diffs.size()); + DiffEntry e = diffs.get(0); + assertEquals(DiffEntry.ChangeType.DELETE, e.getChangeType()); + assertEquals("fileInRoot", e.getOldPath()); + e = diffs.get(1); + assertEquals(DiffEntry.ChangeType.DELETE, e.getChangeType()); + assertEquals("modules/submodule", e.getOldPath()); + assertEquals(FileMode.GITLINK, e.getOldMode()); + e = diffs.get(2); + assertEquals(DiffEntry.ChangeType.ADD, e.getChangeType()); + assertEquals("modules/submodule/fileInRoot", e.getNewPath()); + } + + } } 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 93ada4f30..fa7f5ab52 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 @@ -43,12 +43,14 @@ package org.eclipse.jgit.lib; +import static org.junit.Assert.assertArrayEquals; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; import java.io.File; import java.io.IOException; +import java.util.Arrays; import java.util.Set; import org.eclipse.jgit.api.CloneCommand; @@ -128,7 +130,8 @@ public class IndexDiffSubmoduleTest extends RepositoryTestCase { IndexDiff indexDiff = new IndexDiff(db2, Constants.HEAD, new FileTreeIterator(db2)); indexDiff.setIgnoreSubmoduleMode(mode); - assertFalse(indexDiff.diff()); + boolean changed = indexDiff.diff(); + assertFalse(changed); } @Theory @@ -263,4 +266,33 @@ public class IndexDiffSubmoduleTest extends RepositoryTestCase { assertDiff(indexDiff, mode, IgnoreSubmoduleMode.ALL, IgnoreSubmoduleMode.DIRTY, IgnoreSubmoduleMode.UNTRACKED); } + + @Theory + public void testSubmoduleReplacedByMovedFile(IgnoreSubmoduleMode mode) + throws Exception { + Git git = Git.wrap(db); + git.rm().setCached(true).addFilepattern("modules/submodule").call(); + recursiveDelete(submodule_trash); + JGitTestUtil.deleteTrashFile(db, "fileInRoot"); + // Move the fileInRoot file + writeTrashFile("modules/submodule/fileInRoot", "root"); + git.rm().addFilepattern("fileInRoot").addFilepattern("modules/").call(); + git.add().addFilepattern("modules/").call(); + IndexDiff indexDiff = new IndexDiff(db, Constants.HEAD, + new FileTreeIterator(db)); + indexDiff.setIgnoreSubmoduleMode(mode); + assertTrue(indexDiff.diff()); + String[] removed = indexDiff.getRemoved().toArray(new String[0]); + Arrays.sort(removed); + if (IgnoreSubmoduleMode.ALL.equals(mode)) { + assertArrayEquals(new String[] { "fileInRoot" }, removed); + } else { + assertArrayEquals( + new String[] { "fileInRoot", "modules/submodule" }, + removed); + } + assertEquals("[modules/submodule/fileInRoot]", + indexDiff.getAdded().toString()); + } + } 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 f78cbe260..7542ec891 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,30 +88,15 @@ 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, @@ -119,9 +104,6 @@ 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/lib/IndexDiff.java b/org.eclipse.jgit/src/org/eclipse/jgit/lib/IndexDiff.java index 94b9ddc18..f37c31075 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/lib/IndexDiff.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/lib/IndexDiff.java @@ -47,7 +47,11 @@ package org.eclipse.jgit.lib; +import java.io.File; import java.io.IOException; +import java.nio.file.DirectoryIteratorException; +import java.nio.file.DirectoryStream; +import java.nio.file.Files; import java.text.MessageFormat; import java.util.ArrayList; import java.util.Collection; @@ -261,6 +265,8 @@ public class IndexDiff { private Set missing = new HashSet<>(); + private Set missingSubmodules = new HashSet<>(); + private Set modified = new HashSet<>(); private Set untracked = new HashSet<>(); @@ -501,9 +507,15 @@ public class IndexDiff { if (dirCacheIterator != null) { if (workingTreeIterator == null) { // in index, not in workdir => missing - if (!isEntryGitLink(dirCacheIterator) - || ignoreSubmoduleMode != IgnoreSubmoduleMode.ALL) - missing.add(treeWalk.getPathString()); + boolean isGitLink = isEntryGitLink(dirCacheIterator); + if (!isGitLink + || ignoreSubmoduleMode != IgnoreSubmoduleMode.ALL) { + String path = treeWalk.getPathString(); + missing.add(path); + if (isGitLink) { + missingSubmodules.add(path); + } + } } else { if (workingTreeIterator.isModified( dirCacheIterator.getDirCacheEntry(), true, @@ -543,8 +555,8 @@ public class IndexDiff { smw.getPath()), e); } try (Repository subRepo = smw.getRepository()) { + String subRepoPath = smw.getPath(); if (subRepo != null) { - String subRepoPath = smw.getPath(); ObjectId subHead = subRepo.resolve("HEAD"); //$NON-NLS-1$ if (subHead != null && !subHead.equals(smw.getObjectId())) { @@ -573,6 +585,21 @@ public class IndexDiff { recordFileMode(subRepoPath, FileMode.GITLINK); } } + } else if (missingSubmodules.remove(subRepoPath)) { + // If the directory is there and empty but the submodule + // repository in .git/modules doesn't exist yet it isn't + // "missing". + File gitDir = new File( + new File(repository.getDirectory(), + Constants.MODULES), + subRepoPath); + if (!gitDir.isDirectory()) { + File dir = SubmoduleWalk.getSubmoduleDirectory( + repository, subRepoPath); + if (dir.isDirectory() && !hasFiles(dir)) { + missing.remove(subRepoPath); + } + } } } } @@ -592,6 +619,15 @@ public class IndexDiff { return true; } + private boolean hasFiles(File directory) { + try (DirectoryStream dir = Files + .newDirectoryStream(directory.toPath())) { + return dir.iterator().hasNext(); + } catch (DirectoryIteratorException | IOException e) { + return false; + } + } + private void recordFileMode(String path, FileMode mode) { Set values = fileModes.get(mode); if (path != null) { 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 be1b95e0d..6be7ddbe1 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, 2018 Google Inc. + * Copyright (C) 2016, Google Inc. * and other copyright owners as documented in the project's IP log. * * This program and the accompanying materials are made available @@ -43,7 +43,6 @@ 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; @@ -107,7 +106,7 @@ public class Paths { aPath, aPos, aEnd, aMode, bPath, bPos, bEnd, bMode); if (cmp == 0) { - cmp = modeCompare(aMode, bMode); + cmp = lastPathChar(aMode) - lastPathChar(bMode); } return cmp; } @@ -184,15 +183,6 @@ 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() { } }