From 78420b7d0a65d591d00f32675efb0a42cda6c84a Mon Sep 17 00:00:00 2001 From: Marc Strapetz Date: Fri, 23 Feb 2018 13:34:23 +0100 Subject: [PATCH] Fix processing of gitignore negations Processing of negated rules, like !bin/ was not working correctly: they were interpreted too broad, resulting in unexpected untracked files which should actually be ignored Bug: 409664 Change-Id: I0a422fd6607941461bf2175c9105a0311612efa0 Signed-off-by: Marc Strapetz --- .../eclipse/jgit/ignore/CGitIgnoreTest.java | 64 +++++ .../eclipse/jgit/ignore/IgnoreNodeTest.java | 233 +++++++++++++++++- .../org/eclipse/jgit/ignore/IgnoreNode.java | 67 ++--- .../jgit/treewalk/WorkingTreeIterator.java | 139 ++++++++--- 4 files changed, 439 insertions(+), 64 deletions(-) diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/ignore/CGitIgnoreTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/ignore/CGitIgnoreTest.java index 155bd2730..3b11616fe 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/ignore/CGitIgnoreTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/ignore/CGitIgnoreTest.java @@ -321,4 +321,68 @@ public class CGitIgnoreTest extends RepositoryTestCase { writeTrashFile(".gitignore", "[\\[\\]]\n"); assertSameAsCGit(); } + + @Test + public void testSimpleRootGitIgnoreGlobalNegation1() throws Exception { + // see IgnoreNodeTest.testSimpleRootGitIgnoreGlobalNegation1 + createFiles("x1", "a/x2", "x3/y"); + writeTrashFile(".gitignore", "*\n!x*"); + assertSameAsCGit(); + } + + @Test + public void testRepeatedNegationInDifferentFiles5() throws Exception { + // see IgnoreNodeTest.testRepeatedNegationInDifferentFiles5 + createFiles("a/b/e/nothere.o"); + writeTrashFile(".gitignore", "e"); + writeTrashFile("a/.gitignore", "e"); + writeTrashFile("a/b/.gitignore", "!e"); + assertSameAsCGit(); + } + + @Test + public void testRepeatedNegationInDifferentFilesWithWildmatcher1() + throws Exception { + createFiles("e", "x/e/f", "a/e/x1", "a/e/x2", "a/e/y", "a/e/sub/y"); + writeTrashFile(".gitignore", "a/e/**"); + writeTrashFile("a/.gitignore", "!e/x*"); + assertSameAsCGit(); + } + + @Test + public void testRepeatedNegationInDifferentFilesWithWildmatcher2() + throws Exception { + createFiles("e", "dir/f", "dir/g/h", "a/dir/i", "a/dir/j/k", + "a/b/dir/l", "a/b/dir/m/n", "a/b/dir/m/o/p", "a/q/dir/r", + "a/q/dir/dir/s", "c/d/dir/x", "c/d/dir/y"); + writeTrashFile(".gitignore", "**/dir/*"); + writeTrashFile("a/.gitignore", "!dir/*"); + writeTrashFile("a/b/.gitignore", "!**/dir/*"); + writeTrashFile("c/.gitignore", "!d/dir/x"); + assertSameAsCGit(); + } + + @Test + public void testNegationForSubDirectoryWithinIgnoredDirectoryHasNoEffect1() + throws Exception { + createFiles("e", "a/f", "a/b/g", "a/b/h/i"); + writeTrashFile(".gitignore", "a/b"); + writeTrashFile("a/.gitignore", "!b/*"); + assertSameAsCGit(); + } + + /* + * See https://bugs.eclipse.org/bugs/show_bug.cgi?id=407475 + */ + @Test + public void testNegationAllExceptJavaInSrcAndExceptChildDirInSrc() + throws Exception { + // see + // IgnoreNodeTest.testNegationAllExceptJavaInSrcAndExceptChildDirInSrc + createFiles("nothere.o", "src/keep.java", "src/nothere.o", + "src/a/keep.java", "src/a/keep.o"); + writeTrashFile(".gitignore", "/*\n!/src/"); + writeTrashFile("src/.gitignore", "*\n!*.java\n!*/"); + assertSameAsCGit(); + } } diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/ignore/IgnoreNodeTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/ignore/IgnoreNodeTest.java index 1178eb3e8..1a02df466 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/ignore/IgnoreNodeTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/ignore/IgnoreNodeTest.java @@ -80,6 +80,141 @@ public class IgnoreNodeTest extends RepositoryTestCase { private TreeWalk walk; + @Test + public void testSimpleRootGitIgnoreGlobalIgnore() throws IOException { + writeIgnoreFile(".gitignore", "x"); + + writeTrashFile("a/x/file", ""); + writeTrashFile("b/x", ""); + writeTrashFile("x/file", ""); + + beginWalk(); + assertEntry(F, tracked, ".gitignore"); + assertEntry(D, tracked, "a"); + assertEntry(D, ignored, "a/x"); + assertEntry(F, ignored, "a/x/file"); + assertEntry(D, tracked, "b"); + assertEntry(F, ignored, "b/x"); + assertEntry(D, ignored, "x"); + assertEntry(F, ignored, "x/file"); + endWalk(); + } + + @Test + public void testSimpleRootGitIgnoreGlobalDirIgnore() throws IOException { + writeIgnoreFile(".gitignore", "x/"); + + writeTrashFile("a/x/file", ""); + writeTrashFile("x/file", ""); + + beginWalk(); + assertEntry(F, tracked, ".gitignore"); + assertEntry(D, tracked, "a"); + assertEntry(D, ignored, "a/x"); + assertEntry(F, ignored, "a/x/file"); + assertEntry(D, ignored, "x"); + assertEntry(F, ignored, "x/file"); + endWalk(); + } + + @Test + public void testSimpleRootGitIgnoreWildMatcher() throws IOException { + writeIgnoreFile(".gitignore", "**"); + + writeTrashFile("a/x", ""); + writeTrashFile("y", ""); + + beginWalk(); + assertEntry(F, ignored, ".gitignore"); + assertEntry(D, ignored, "a"); + assertEntry(F, ignored, "a/x"); + assertEntry(F, ignored, "y"); + endWalk(); + } + + @Test + public void testSimpleRootGitIgnoreWildMatcherDirOnly() throws IOException { + writeIgnoreFile(".gitignore", "**/"); + + writeTrashFile("a/x", ""); + writeTrashFile("y", ""); + + beginWalk(); + assertEntry(F, tracked, ".gitignore"); + assertEntry(D, ignored, "a"); + assertEntry(F, ignored, "a/x"); + assertEntry(F, tracked, "y"); + endWalk(); + } + + @Test + public void testSimpleRootGitIgnoreGlobalNegation1() throws IOException { + writeIgnoreFile(".gitignore", "*", "!x*"); + writeTrashFile("x1", ""); + writeTrashFile("a/x2", ""); + writeTrashFile("x3/y", ""); + + beginWalk(); + assertEntry(F, ignored, ".gitignore"); + assertEntry(D, ignored, "a"); + assertEntry(F, ignored, "a/x2"); + assertEntry(F, tracked, "x1"); + assertEntry(D, tracked, "x3"); + assertEntry(F, ignored, "x3/y"); + endWalk(); + } + + @Test + public void testSimpleRootGitIgnoreGlobalNegation2() throws IOException { + writeIgnoreFile(".gitignore", "*", "!x*", "!/a"); + writeTrashFile("x1", ""); + writeTrashFile("a/x2", ""); + writeTrashFile("x3/y", ""); + + beginWalk(); + assertEntry(F, ignored, ".gitignore"); + assertEntry(D, tracked, "a"); + assertEntry(F, tracked, "a/x2"); + assertEntry(F, tracked, "x1"); + assertEntry(D, tracked, "x3"); + assertEntry(F, ignored, "x3/y"); + endWalk(); + } + + @Test + public void testSimpleRootGitIgnoreGlobalNegation3() throws IOException { + writeIgnoreFile(".gitignore", "*", "!x*", "!x*/**"); + writeTrashFile("x1", ""); + writeTrashFile("a/x2", ""); + writeTrashFile("x3/y", ""); + + beginWalk(); + assertEntry(F, ignored, ".gitignore"); + assertEntry(D, ignored, "a"); + assertEntry(F, ignored, "a/x2"); + assertEntry(F, tracked, "x1"); + assertEntry(D, tracked, "x3"); + assertEntry(F, tracked, "x3/y"); + endWalk(); + } + + @Test + public void testSimpleRootGitIgnoreGlobalNegation4() throws IOException { + writeIgnoreFile(".gitignore", "*", "!**/"); + writeTrashFile("x1", ""); + writeTrashFile("a/x2", ""); + writeTrashFile("x3/y", ""); + + beginWalk(); + assertEntry(F, ignored, ".gitignore"); + assertEntry(D, tracked, "a"); + assertEntry(F, ignored, "a/x2"); + assertEntry(F, ignored, "x1"); + assertEntry(D, tracked, "x3"); + assertEntry(F, ignored, "x3/y"); + endWalk(); + } + @Test public void testRules() throws IOException { writeIgnoreFile(".git/info/exclude", "*~", "/out"); @@ -210,7 +345,7 @@ public class IgnoreNodeTest extends RepositoryTestCase { assertEntry(F, ignored, "src/.gitignore"); assertEntry(D, tracked, "src/a"); assertEntry(F, tracked, "src/a/keep.java"); - assertEntry(F, tracked, "src/a/keep.o"); + assertEntry(F, ignored, "src/a/keep.o"); assertEntry(F, tracked, "src/keep.java"); assertEntry(F, ignored, "src/nothere.o"); endWalk(); @@ -315,6 +450,102 @@ public class IgnoreNodeTest extends RepositoryTestCase { endWalk(); } + @Test + public void testRepeatedNegationInDifferentFiles5() throws IOException { + writeIgnoreFile(".gitignore", "e"); + writeIgnoreFile("a/.gitignore", "e"); + writeIgnoreFile("a/b/.gitignore", "!e"); + writeTrashFile("a/b/e/nothere.o", ""); + + beginWalk(); + assertEntry(F, tracked, ".gitignore"); + assertEntry(D, tracked, "a"); + assertEntry(F, tracked, "a/.gitignore"); + assertEntry(D, tracked, "a/b"); + assertEntry(F, tracked, "a/b/.gitignore"); + assertEntry(D, tracked, "a/b/e"); + assertEntry(F, tracked, "a/b/e/nothere.o"); + endWalk(); + } + + @Test + public void testIneffectiveNegationDifferentLevels1() throws IOException { + writeIgnoreFile(".gitignore", "a/b/e/", "!a/b/e/*"); + writeTrashFile("a/b/e/nothere.o", ""); + + beginWalk(); + assertEntry(F, tracked, ".gitignore"); + assertEntry(D, tracked, "a"); + assertEntry(D, tracked, "a/b"); + assertEntry(D, ignored, "a/b/e"); + assertEntry(F, ignored, "a/b/e/nothere.o"); + endWalk(); + } + + @Test + public void testIneffectiveNegationDifferentLevels2() throws IOException { + writeIgnoreFile(".gitignore", "a/b/e/"); + writeIgnoreFile("a/.gitignore", "!b/e/*"); + writeTrashFile("a/b/e/nothere.o", ""); + + beginWalk(); + assertEntry(F, tracked, ".gitignore"); + assertEntry(D, tracked, "a"); + assertEntry(F, tracked, "a/.gitignore"); + assertEntry(D, tracked, "a/b"); + assertEntry(D, ignored, "a/b/e"); + assertEntry(F, ignored, "a/b/e/nothere.o"); + endWalk(); + } + + @Test + public void testIneffectiveNegationDifferentLevels3() throws IOException { + writeIgnoreFile(".gitignore", "a/b/e/"); + writeIgnoreFile("a/b/.gitignore", "!e/*"); + writeTrashFile("a/b/e/nothere.o", ""); + + beginWalk(); + assertEntry(F, tracked, ".gitignore"); + assertEntry(D, tracked, "a"); + assertEntry(D, tracked, "a/b"); + assertEntry(F, tracked, "a/b/.gitignore"); + assertEntry(D, ignored, "a/b/e"); + assertEntry(F, ignored, "a/b/e/nothere.o"); + endWalk(); + } + + @Test + public void testIneffectiveNegationDifferentLevels4() throws IOException { + writeIgnoreFile(".gitignore", "a/b/e/"); + writeIgnoreFile("a/b/e/.gitignore", "!*"); + writeTrashFile("a/b/e/nothere.o", ""); + + beginWalk(); + assertEntry(F, tracked, ".gitignore"); + assertEntry(D, tracked, "a"); + assertEntry(D, tracked, "a/b"); + assertEntry(D, ignored, "a/b/e"); + assertEntry(F, ignored, "a/b/e/.gitignore"); + assertEntry(F, ignored, "a/b/e/nothere.o"); + endWalk(); + } + + @Test + public void testIneffectiveNegationDifferentLevels5() throws IOException { + writeIgnoreFile("a/.gitignore", "b/e/"); + writeIgnoreFile("a/b/.gitignore", "!e/*"); + writeTrashFile("a/b/e/nothere.o", ""); + + beginWalk(); + assertEntry(D, tracked, "a"); + assertEntry(F, tracked, "a/.gitignore"); + assertEntry(D, tracked, "a/b"); + assertEntry(F, tracked, "a/b/.gitignore"); + assertEntry(D, ignored, "a/b/e"); + assertEntry(F, ignored, "a/b/e/nothere.o"); + endWalk(); + } + @Test public void testEmptyIgnoreNode() { // Rules are never empty: WorkingTreeIterator optimizes empty files away diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/ignore/IgnoreNode.java b/org.eclipse.jgit/src/org/eclipse/jgit/ignore/IgnoreNode.java index 1ad60101e..621978333 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/ignore/IgnoreNode.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/ignore/IgnoreNode.java @@ -145,7 +145,12 @@ public class IgnoreNode { * @return status of the path. */ public MatchResult isIgnored(String entryPath, boolean isDirectory) { - return isIgnored(entryPath, isDirectory, false); + final Boolean result = checkIgnored(entryPath, isDirectory); + if (result == null) { + return MatchResult.CHECK_PARENT; + } + + return result ? MatchResult.IGNORED : MatchResult.NOT_IGNORED; } /** @@ -159,45 +164,47 @@ public class IgnoreNode { * true if the target item is a directory. * @param negateFirstMatch * true if the first match should be negated + * @deprecated negateFirstMatch is not honored anymore * @return status of the path. * @since 3.6 */ + @Deprecated public MatchResult isIgnored(String entryPath, boolean isDirectory, boolean negateFirstMatch) { - if (rules.isEmpty()) - if (negateFirstMatch) - return MatchResult.CHECK_PARENT_NEGATE_FIRST_MATCH; - else - return MatchResult.CHECK_PARENT; + final Boolean result = checkIgnored(entryPath, isDirectory); + if (result == null) { + return negateFirstMatch + ? MatchResult.CHECK_PARENT_NEGATE_FIRST_MATCH + : MatchResult.CHECK_PARENT; + } + + return result ? MatchResult.IGNORED : MatchResult.NOT_IGNORED; + } - // Parse rules in the reverse order that they were read + /** + * Determine if an entry path matches an ignore rule. + * + * @param entryPath + * the path to test. The path must be relative to this ignore + * node's own repository path, and in repository path format + * (uses '/' and not '\'). + * @param isDirectory + * true if the target item is a directory. + * @return Boolean.TRUE, if the entry is ignored; Boolean.FALSE, if the + * entry is forced to be not ignored (negated match); or null, if + * undetermined + * @since 4.11 + */ + public Boolean checkIgnored(String entryPath, boolean isDirectory) { + // Parse rules in the reverse order that they were read because later + // rules have higher priority for (int i = rules.size() - 1; i > -1; i--) { FastIgnoreRule rule = rules.get(i); - if (rule.isMatch(entryPath, isDirectory)) { - if (rule.getResult()) { - // rule matches: path could be ignored - if (negateFirstMatch) - // ignore current match, reset "negate" flag, continue - negateFirstMatch = false; - else - // valid match, just return - return MatchResult.IGNORED; - } else { - // found negated rule - if (negateFirstMatch) - // not possible to re-include excluded ignore rule - return MatchResult.NOT_IGNORED; - else - // set the flag and continue - negateFirstMatch = true; - } + if (rule.isMatch(entryPath, isDirectory, true)) { + return Boolean.valueOf(rule.getResult()); } } - if (negateFirstMatch) - // negated rule found but there is no previous rule in *this* file - return MatchResult.CHECK_PARENT_NEGATE_FIRST_MATCH; - // *this* file has no matching rules - return MatchResult.CHECK_PARENT; + return null; } /** {@inheritDoc} */ 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 0b1547535..f87573055 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/treewalk/WorkingTreeIterator.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/treewalk/WorkingTreeIterator.java @@ -60,6 +60,8 @@ import java.text.MessageFormat; import java.util.Arrays; import java.util.Collections; import java.util.Comparator; +import java.util.HashMap; +import java.util.Map; import org.eclipse.jgit.api.errors.FilterFailedException; import org.eclipse.jgit.attributes.AttributesNode; @@ -661,54 +663,60 @@ public abstract class WorkingTreeIterator extends AbstractTreeIterator { * a relevant ignore rule file exists but cannot be read. */ protected boolean isEntryIgnored(final int pLen) throws IOException { - return isEntryIgnored(pLen, mode, false); + return isEntryIgnored(pLen, mode); } /** - * Determine if the entry path is ignored by an ignore rule. Consider - * possible rule negation from child iterator. + * Determine if the entry path is ignored by an ignore rule. * * @param pLen * the length of the path in the path buffer. * @param fileMode * the original iterator file mode - * @param negatePrevious - * true if the previous matching iterator rule was negation * @return true if the entry is ignored by an ignore rule. * @throws IOException * a relevant ignore rule file exists but cannot be read. */ - private boolean isEntryIgnored(final int pLen, int fileMode, - boolean negatePrevious) + private boolean isEntryIgnored(final int pLen, int fileMode) throws IOException { + // The ignore code wants path to start with a '/' if possible. + // If we have the '/' in our path buffer because we are inside + // a sub-directory include it in the range we convert to string. + // + final int pOff = 0 < pathOffset ? pathOffset - 1 : pathOffset; + String pathRel = TreeWalk.pathOf(this.path, pOff, pLen); + String parentRel = getParentPath(pathRel); + + // CGit is processing .gitignore files by starting at the root of the + // repository and then recursing into subdirectories. With this + // approach, top-level ignored directories will be processed first which + // allows to skip entire subtrees and further .gitignore-file processing + // within these subtrees. + // + // We will follow the same approach by marking directories as "ignored" + // here. This allows to have a simplified FastIgnore.checkIgnore() + // implementation (both in terms of code and computational complexity): + // + // Without the "ignored" flag, we would have to apply the ignore-check + // to a path and all of its parents always(!), to determine whether a + // path is ignored directly or by one of its parent directories; with + // the "ignored" flag, we know at this point that the parent directory + // is definitely not ignored, thus the path can only become ignored if + // there is a rule matching the path itself. + if (isDirectoryIgnored(parentRel)) { + return true; + } + IgnoreNode rules = getIgnoreNode(); - if (rules != null) { - // The ignore code wants path to start with a '/' if possible. - // If we have the '/' in our path buffer because we are inside - // a subdirectory include it in the range we convert to string. - // - int pOff = pathOffset; - if (0 < pOff) - pOff--; - String p = TreeWalk.pathOf(path, pOff, pLen); - switch (rules.isIgnored(p, FileMode.TREE.equals(fileMode), - negatePrevious)) { - case IGNORED: - return true; - case NOT_IGNORED: - return false; - case CHECK_PARENT: - negatePrevious = false; - break; - case CHECK_PARENT_NEGATE_FIRST_MATCH: - negatePrevious = true; - break; - } + final Boolean ignored = rules != null + ? rules.checkIgnored(pathRel, FileMode.TREE.equals(fileMode)) + : null; + if (ignored != null) { + return ignored.booleanValue(); } - if (parent instanceof WorkingTreeIterator) - return ((WorkingTreeIterator) parent).isEntryIgnored(pLen, fileMode, - negatePrevious); - return false; + return parent instanceof WorkingTreeIterator + && ((WorkingTreeIterator) parent).isEntryIgnored(pLen, + fileMode); } private IgnoreNode getIgnoreNode() throws IOException { @@ -1372,6 +1380,8 @@ public abstract class WorkingTreeIterator extends AbstractTreeIterator { /** Position of the matching {@link DirCacheIterator}. */ int dirCacheTree; + final Map directoryToIgnored = new HashMap<>(); + IteratorState(WorkingTreeOptions options) { this.options = options; this.nameEncoder = Constants.CHARSET.newEncoder(); @@ -1448,4 +1458,67 @@ public abstract class WorkingTreeIterator extends AbstractTreeIterator { } return eolStreamTypeHolder.get(); } + + private boolean isDirectoryIgnored(String pathRel) throws IOException { + final int pOff = 0 < pathOffset ? pathOffset - 1 : pathOffset; + final String base = TreeWalk.pathOf(this.path, 0, pOff); + final String pathAbs = concatPath(base, pathRel); + return isDirectoryIgnored(pathRel, pathAbs); + } + + private boolean isDirectoryIgnored(String pathRel, String pathAbs) + throws IOException { + assert pathRel.length() == 0 || (pathRel.charAt(0) != '/' + && pathRel.charAt(pathRel.length() - 1) != '/'); + assert pathAbs.length() == 0 || (pathAbs.charAt(0) != '/' + && pathAbs.charAt(pathAbs.length() - 1) != '/'); + assert pathAbs.endsWith(pathRel); + + Boolean ignored = state.directoryToIgnored.get(pathAbs); + if (ignored != null) { + return ignored; + } + + final String parentRel = getParentPath(pathRel); + if (parentRel != null && isDirectoryIgnored(parentRel)) { + state.directoryToIgnored.put(pathAbs, Boolean.TRUE); + return true; + } + + final IgnoreNode ignoreNode = getIgnoreNode(); + for (String path = pathRel; ignoreNode != null + && !"".equals(path); path = getParentPath(path)) { + ignored = ignoreNode.checkIgnored(path, true); + if (ignored != null) { + state.directoryToIgnored.put(pathAbs, ignored); + return ignored; + } + } + + if (!(this.parent instanceof WorkingTreeIterator)) { + state.directoryToIgnored.put(pathAbs, Boolean.FALSE); + return false; + } + + final WorkingTreeIterator wtParent = (WorkingTreeIterator) this.parent; + final String parentRelPath = concatPath( + TreeWalk.pathOf(this.path, wtParent.pathOffset, pathOffset - 1), + pathRel); + assert concatPath(TreeWalk.pathOf(wtParent.path, 0, + Math.max(0, wtParent.pathOffset - 1)), parentRelPath) + .equals(pathAbs); + return wtParent.isDirectoryIgnored(parentRelPath, pathAbs); + } + + private static String getParentPath(String path) { + final int slashIndex = path.lastIndexOf('/', path.length() - 2); + if (slashIndex > 0) { + return path.substring(path.charAt(0) == '/' ? 1 : 0, slashIndex); + } + return path.length() > 0 ? "" : null; + } + + private static String concatPath(String p1, String p2) { + return p1 + (p1.length() > 0 && p2.length() > 0 ? "/" : "") + p2; + } }