From 147e24a7b224fe6d643bb24100030ff70bfbca35 Mon Sep 17 00:00:00 2001 From: Andrey Loskutov Date: Wed, 22 Oct 2014 09:31:42 +0200 Subject: [PATCH] Consider parent rules if ignore rule is negated The change tries to make jgit behave more like native CLI git regarding the negation rules. According to [1] "... prefix "!" which negates the pattern; any matching file excluded by a previous pattern will become included again." Negating the pattern should not automatically make the file *not ignored* - other pattern rules have to be considered too. The fix adds test cases for both bugs 448094 and 407475. [1] https://www.kernel.org/pub/software/scm/git/docs/gitignore.html Bug: 448094 Bug: 407475 Change-Id: I322954200dd3c683e3d8f4adc48506eb99e56ae1 Signed-off-by: Andrey Loskutov Signed-off-by: Matthias Sohn --- .../eclipse/jgit/ignore/IgnoreNodeTest.java | 213 +++++++++++++++++- .../org/eclipse/jgit/ignore/IgnoreNode.java | 64 +++++- .../jgit/treewalk/WorkingTreeIterator.java | 27 ++- 3 files changed, 294 insertions(+), 10 deletions(-) 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 0076ba124..571f3186d 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 @@ -44,13 +44,16 @@ package org.eclipse.jgit.ignore; import static org.eclipse.jgit.junit.Assert.assertEquals; import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertTrue; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNotNull; import java.io.File; import java.io.IOException; +import java.util.Arrays; import org.eclipse.jgit.errors.CorruptObjectException; +import org.eclipse.jgit.ignore.IgnoreNode.MatchResult; import org.eclipse.jgit.junit.RepositoryTestCase; import org.eclipse.jgit.lib.FileMode; import org.eclipse.jgit.treewalk.FileTreeIterator; @@ -110,15 +113,26 @@ public class IgnoreNodeTest extends RepositoryTestCase { assertEntry(F, ignored, "src/config/lex.out"); assertEntry(D, tracked, "src/config/old"); assertEntry(F, ignored, "src/config/old/lex.out"); + endWalk(); } @Test public void testNegation() throws IOException { - writeIgnoreFile(".gitignore", "*.o"); + // ignore all *.o files and ignore all "d" directories + writeIgnoreFile(".gitignore", "*.o", "d"); + + // negate "ignore" for a/b/keep.o file only writeIgnoreFile("src/a/b/.gitignore", "!keep.o"); writeTrashFile("src/a/b/keep.o", ""); writeTrashFile("src/a/b/nothere.o", ""); + // negate "ignore" for "d" + writeIgnoreFile("src/c/.gitignore", "!d"); + // negate "ignore" for c/d/keep.o file only + writeIgnoreFile("src/c/d/.gitignore", "!keep.o"); + writeTrashFile("src/c/d/keep.o", ""); + writeTrashFile("src/c/d/nothere.o", ""); + beginWalk(); assertEntry(F, tracked, ".gitignore"); assertEntry(D, tracked, "src"); @@ -127,6 +141,186 @@ public class IgnoreNodeTest extends RepositoryTestCase { assertEntry(F, tracked, "src/a/b/.gitignore"); assertEntry(F, tracked, "src/a/b/keep.o"); assertEntry(F, ignored, "src/a/b/nothere.o"); + + assertEntry(D, tracked, "src/c"); + assertEntry(F, tracked, "src/c/.gitignore"); + assertEntry(D, tracked, "src/c/d"); + assertEntry(F, tracked, "src/c/d/.gitignore"); + assertEntry(F, tracked, "src/c/d/keep.o"); + // must be ignored: "!d" should not negate *both* "d" and *.o rules! + assertEntry(F, ignored, "src/c/d/nothere.o"); + endWalk(); + } + + /* + * See https://bugs.eclipse.org/bugs/show_bug.cgi?id=407475 + */ + @Test + public void testNegateAllExceptJavaInSrc() throws IOException { + // ignore all files except from src directory + writeIgnoreFile(".gitignore", "/*", "!/src/"); + writeTrashFile("nothere.o", ""); + + // ignore all files except java + writeIgnoreFile("src/.gitignore", "*", "!*.java"); + + writeTrashFile("src/keep.java", ""); + writeTrashFile("src/nothere.o", ""); + writeTrashFile("src/a/nothere.o", ""); + + beginWalk(); + assertEntry(F, ignored, ".gitignore"); + assertEntry(F, ignored, "nothere.o"); + assertEntry(D, tracked, "src"); + assertEntry(F, ignored, "src/.gitignore"); + assertEntry(D, ignored, "src/a"); + assertEntry(F, ignored, "src/a/nothere.o"); + assertEntry(F, tracked, "src/keep.java"); + assertEntry(F, ignored, "src/nothere.o"); + endWalk(); + } + + /* + * See https://bugs.eclipse.org/bugs/show_bug.cgi?id=407475 + */ + @Test + public void testNegationAllExceptJavaInSrcAndExceptChildDirInSrc() + throws IOException { + // ignore all files except from src directory + writeIgnoreFile(".gitignore", "/*", "!/src/"); + writeTrashFile("nothere.o", ""); + + // ignore all files except java in src folder and all children folders. + // Last ignore rule breaks old jgit via bug 407475 + writeIgnoreFile("src/.gitignore", "*", "!*.java", "!*/"); + + writeTrashFile("src/keep.java", ""); + writeTrashFile("src/nothere.o", ""); + writeTrashFile("src/a/keep.java", ""); + writeTrashFile("src/a/keep.o", ""); + + beginWalk(); + assertEntry(F, ignored, ".gitignore"); + assertEntry(F, ignored, "nothere.o"); + assertEntry(D, tracked, "src"); + 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, tracked, "src/keep.java"); + assertEntry(F, ignored, "src/nothere.o"); + endWalk(); + } + + /* + * See https://bugs.eclipse.org/bugs/show_bug.cgi?id=448094 + */ + @Test + public void testRepeatedNegation() throws IOException { + writeIgnoreFile(".gitignore", "e", "!e", "e", "!e", "e"); + + writeTrashFile("e/nothere.o", ""); + + beginWalk(); + assertEntry(F, tracked, ".gitignore"); + assertEntry(D, ignored, "e"); + assertEntry(F, ignored, "e/nothere.o"); + endWalk(); + } + + /* + * See https://bugs.eclipse.org/bugs/show_bug.cgi?id=448094 + */ + @Test + public void testRepeatedNegationInDifferentFiles1() throws IOException { + writeIgnoreFile(".gitignore", "*.o", "e"); + + writeIgnoreFile("e/.gitignore", "!e"); + writeTrashFile("e/nothere.o", ""); + + beginWalk(); + assertEntry(F, tracked, ".gitignore"); + assertEntry(D, ignored, "e"); + assertEntry(F, ignored, "e/.gitignore"); + assertEntry(F, ignored, "e/nothere.o"); + endWalk(); + } + + /* + * See https://bugs.eclipse.org/bugs/show_bug.cgi?id=448094 + */ + @Test + public void testRepeatedNegationInDifferentFiles2() throws IOException { + writeIgnoreFile(".gitignore", "*.o", "e"); + + writeIgnoreFile("a/.gitignore", "!e"); + writeTrashFile("a/e/nothere.o", ""); + + beginWalk(); + assertEntry(F, tracked, ".gitignore"); + assertEntry(D, tracked, "a"); + assertEntry(F, tracked, "a/.gitignore"); + assertEntry(D, tracked, "a/e"); + assertEntry(F, ignored, "a/e/nothere.o"); + endWalk(); + } + + /* + * See https://bugs.eclipse.org/bugs/show_bug.cgi?id=448094 + */ + @Test + public void testRepeatedNegationInDifferentFiles3() throws IOException { + writeIgnoreFile(".gitignore", "*.o"); + + 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, ignored, "a/b/e/nothere.o"); + endWalk(); + } + + @Test + public void testRepeatedNegationInDifferentFiles4() throws IOException { + writeIgnoreFile(".gitignore", "*.o"); + + writeIgnoreFile("a/.gitignore", "e"); + // Rules are never empty: WorkingTreeIterator optimizes empty rules away + // paranoia check in case this optimization will be removed + writeIgnoreFile("a/b/.gitignore", "#"); + writeIgnoreFile("a/b/c/.gitignore", "!e"); + writeTrashFile("a/b/c/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/c"); + assertEntry(F, tracked, "a/b/c/.gitignore"); + assertEntry(D, tracked, "a/b/c/e"); + assertEntry(F, ignored, "a/b/c/e/nothere.o"); + endWalk(); + } + + @Test + public void testEmptyIgnoreNode() { + // Rules are never empty: WorkingTreeIterator optimizes empty files away + // So we have to test it manually in case third party clients use + // IgnoreNode directly. + IgnoreNode node = new IgnoreNode(); + assertEquals(MatchResult.CHECK_PARENT, node.isIgnored("", false)); + assertEquals(MatchResult.CHECK_PARENT, node.isIgnored("", false, false)); + assertEquals(MatchResult.CHECK_PARENT_NEGATE_FIRST_MATCH, + node.isIgnored("", false, true)); } @Test @@ -145,6 +339,7 @@ public class IgnoreNodeTest extends RepositoryTestCase { assertEntry(F, tracked, ".gitignore"); assertEntry(D, ignored, "out"); assertEntry(F, ignored, "out/foo"); + endWalk(); } @Test @@ -164,6 +359,7 @@ public class IgnoreNodeTest extends RepositoryTestCase { assertEntry(D, tracked, "src/a"); assertEntry(F, tracked, "src/a/a"); assertEntry(F, tracked, "src/a/b"); + endWalk(); } @Test @@ -175,6 +371,15 @@ public class IgnoreNodeTest extends RepositoryTestCase { assertEntry(F, tracked, ".gitignore"); assertEntry(D, tracked, "a"); assertEntry(F, tracked, "a/a"); + endWalk(); + } + + @Test + public void testToString() throws Exception { + assertEquals(Arrays.asList("").toString(), new IgnoreNode().toString()); + assertEquals(Arrays.asList("hello").toString(), + new IgnoreNode(Arrays.asList(new FastIgnoreRule("hello"))) + .toString()); } private void beginWalk() throws CorruptObjectException { @@ -182,6 +387,10 @@ public class IgnoreNodeTest extends RepositoryTestCase { walk.addTree(new FileTreeIterator(db)); } + private void endWalk() throws IOException { + assertFalse("Not all files tested", walk.next()); + } + private void assertEntry(FileMode type, boolean entryIgnored, String pathName) throws IOException { assertTrue("walk has entry", walk.next()); 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 ff74f7cab..efaeacd53 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/ignore/IgnoreNode.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/ignore/IgnoreNode.java @@ -67,7 +67,15 @@ public class IgnoreNode { IGNORED, /** The ignore status is unknown, check inherited rules. */ - CHECK_PARENT; + CHECK_PARENT, + + /** + * The first previous (parent) ignore rule match (if any) should be + * negated, and then inherited rules applied. + * + * @since 3.6 + */ + CHECK_PARENT_NEGATE_FIRST_MATCH; } /** The rules that have been parsed into this node. */ @@ -128,19 +136,63 @@ public class IgnoreNode { * @return status of the path. */ public MatchResult isIgnored(String entryPath, boolean isDirectory) { + return isIgnored(entryPath, isDirectory, false); + } + + /** + * 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. + * @param negateFirstMatch + * true if the first match should be negated + * @return status of the path. + * @since 3.6 + */ + public MatchResult isIgnored(String entryPath, boolean isDirectory, + boolean negateFirstMatch) { if (rules.isEmpty()) - return MatchResult.CHECK_PARENT; + if (negateFirstMatch) + return MatchResult.CHECK_PARENT_NEGATE_FIRST_MATCH; + else + return MatchResult.CHECK_PARENT; // Parse rules in the reverse order that they were read for (int i = rules.size() - 1; i > -1; i--) { FastIgnoreRule rule = rules.get(i); if (rule.isMatch(entryPath, isDirectory)) { - if (rule.getResult()) - return MatchResult.IGNORED; - else - return MatchResult.NOT_IGNORED; + 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 (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; } + + @Override + public String toString() { + return rules.toString(); + } } 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 cc5ef1807..6311da6b6 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/treewalk/WorkingTreeIterator.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/treewalk/WorkingTreeIterator.java @@ -573,6 +573,23 @@ 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, false); + } + + /** + * Determine if the entry path is ignored by an ignore rule. Consider + * possible rule negation from child iterator. + * + * @param pLen + * the length of the path in the path buffer. + * @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, boolean negatePrevious) + throws IOException { IgnoreNode rules = getIgnoreNode(); if (rules != null) { // The ignore code wants path to start with a '/' if possible. @@ -583,17 +600,23 @@ public abstract class WorkingTreeIterator extends AbstractTreeIterator { if (0 < pOff) pOff--; String p = TreeWalk.pathOf(path, pOff, pLen); - switch (rules.isIgnored(p, FileMode.TREE.equals(mode))) { + switch (rules.isIgnored(p, FileMode.TREE.equals(mode), + 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; } } if (parent instanceof WorkingTreeIterator) - return ((WorkingTreeIterator) parent).isEntryIgnored(pLen); + return ((WorkingTreeIterator) parent).isEntryIgnored(pLen, + negatePrevious); return false; }