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 <loskutov@gmx.de>
Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>

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());

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();
+	}
 }

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;
 	}