Fix PathSuffixFilter: can decide only on full paths

On a subtree, a PathSuffixFilter must return -1 ("indeterminate"),
not 0 ("include"), otherwise negation goes wrong: an indeterminate
result (-1) is passed on, but a decision (0/1) is inverted.

As a result a negated PathSuffixFilter would skip all folders.

Bug: 574253
Change-Id: I27fe785c0d772392a5b5efe0a7b1c9cafcb6e566
Signed-off-by: Thomas Wolf <thomas.wolf@paranor.ch>

3 files changed, 105 insertions, 6 deletions

diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/diff/DiffFormatterTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/diff/DiffFormatterTest.java
index 62824d3aec..b694f4aaf7 100644
--- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/diff/DiffFormatterTest.java
+++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/diff/DiffFormatterTest.java
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2010, 2020 Google Inc. and others
+ * Copyright (C) 2010, 2021 Google Inc. and others
  *
  * This program and the accompanying materials are made available under the
  * terms of the Eclipse Distribution License v. 1.0 which is available at
@@ -18,6 +18,8 @@ import static org.junit.Assert.assertTrue;
 import java.io.BufferedOutputStream;
 import java.io.ByteArrayOutputStream;
 import java.io.File;
+import java.util.ArrayList;
+import java.util.List;
 
 import org.eclipse.jgit.api.Git;
 import org.eclipse.jgit.api.Status;
@@ -34,8 +36,12 @@ import org.eclipse.jgit.patch.FileHeader;
 import org.eclipse.jgit.patch.HunkHeader;
 import org.eclipse.jgit.revwalk.RevCommit;
 import org.eclipse.jgit.storage.file.FileBasedConfig;
+import org.eclipse.jgit.treewalk.CanonicalTreeParser;
 import org.eclipse.jgit.treewalk.FileTreeIterator;
+import org.eclipse.jgit.treewalk.filter.OrTreeFilter;
 import org.eclipse.jgit.treewalk.filter.PathFilter;
+import org.eclipse.jgit.treewalk.filter.PathSuffixFilter;
+import org.eclipse.jgit.treewalk.filter.TreeFilter;
 import org.eclipse.jgit.util.FileUtils;
 import org.eclipse.jgit.util.RawParseUtils;
 import org.eclipse.jgit.util.io.DisabledOutputStream;
@@ -492,6 +498,68 @@ public class DiffFormatterTest extends RepositoryTestCase {
 	}
 
 	@Test
+	public void testFilter() throws Exception {
+		RevCommit parent;
+		RevCommit head;
+		try (Git git = new Git(db)) {
+			writeTrashFile("foo.txt", "foo\n");
+			writeTrashFile("src/some.txt", "some\n");
+			writeTrashFile("src/image.png", "image\n");
+			writeTrashFile("src/test.pdf", "test\n");
+			writeTrashFile("src/xyz.txt", "xyz\n");
+			git.add().addFilepattern(".").call();
+			parent = git.commit().setMessage("initial").call();
+			writeTrashFile("foo.txt", "FOO\n");
+			writeTrashFile("src/some.txt", "SOME\n");
+			writeTrashFile("src/image.png", "IMAGE\n");
+			writeTrashFile("src/test.pdf", "TEST\n");
+			writeTrashFile("src/xyz.txt", "XYZ\n");
+			git.add().addFilepattern(".").call();
+			head = git.commit().setMessage("second").call();
+		}
+		try (ByteArrayOutputStream os = new ByteArrayOutputStream();
+				DiffFormatter dfmt = new DiffFormatter(os)) {
+			dfmt.setRepository(db);
+			List<TreeFilter> skip = new ArrayList<>();
+			skip.add(PathSuffixFilter.create(".png"));
+			skip.add(PathSuffixFilter.create(".pdf"));
+			dfmt.setPathFilter(OrTreeFilter.create(skip).negate());
+			dfmt.format(
+					new CanonicalTreeParser(null, db.newObjectReader(),
+							parent.getTree()),
+					new CanonicalTreeParser(null, db.newObjectReader(),
+							head.getTree()));
+			dfmt.flush();
+
+			String actual = os.toString("UTF-8");
+
+			String expected = "diff --git a/foo.txt b/foo.txt\n"
+					+ "index 257cc56..b7d6715 100644\n"
+					+ "--- a/foo.txt\n"
+					+ "+++ b/foo.txt\n"
+					+ "@@ -1 +1 @@\n"
+					+ "-foo\n"
+					+ "+FOO\n"
+					+ "diff --git a/src/some.txt b/src/some.txt\n"
+					+ "index 363ef61..76cea5f 100644\n"
+					+ "--- a/src/some.txt\n"
+					+ "+++ b/src/some.txt\n"
+					+ "@@ -1 +1 @@\n"
+					+ "-some\n"
+					+ "+SOME\n"
+					+ "diff --git a/src/xyz.txt b/src/xyz.txt\n"
+					+ "index cd470e6..d4e3ab0 100644\n"
+					+ "--- a/src/xyz.txt\n"
+					+ "+++ b/src/xyz.txt\n"
+					+ "@@ -1 +1 @@\n"
+					+ "-xyz\n"
+					+ "+XYZ\n";
+
+			assertEquals(expected, actual);
+		}
+	}
+
+	@Test
 	public void testTrackedFileInIgnoredFolderChanged()
 			throws Exception {
 		String expectedDiff = "diff --git a/empty/empty/foo b/empty/empty/foo\n"
diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/treewalk/filter/PathSuffixFilterTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/treewalk/filter/PathSuffixFilterTest.java
index d8133913df..0b5a7356c8 100644
--- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/treewalk/filter/PathSuffixFilterTest.java
+++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/treewalk/filter/PathSuffixFilterTest.java
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2009, 2013 Google Inc. and others
+ * Copyright (C) 2009, 2021 Google Inc. and others
  *
  * This program and the accompanying materials are made available under the
  * terms of the Eclipse Distribution License v. 1.0 which is available at
@@ -41,10 +41,11 @@ public class PathSuffixFilterTest extends RepositoryTestCase {
 
 	@Test
 	public void testRecursiveFiltering() throws IOException {
-		ObjectId treeId = createTree("a.sth", "a.txt", "sub/b.sth", "sub/b.txt");
+		ObjectId treeId = createTree("a.sth", "a.txt", "sub/b.sth", "sub/b.txt",
+				"t.sth", "t.txt");
 
 		List<String> paths = getMatchingPaths(".txt", treeId, true);
-		List<String> expected = Arrays.asList("a.txt", "sub/b.txt");
+		List<String> expected = Arrays.asList("a.txt", "sub/b.txt", "t.txt");
 
 		assertEquals(expected, paths);
 	}
@@ -59,6 +60,17 @@ public class PathSuffixFilterTest extends RepositoryTestCase {
 		assertEquals(Arrays.asList("abc", "c"), getMatchingPaths("c", treeId));
 	}
 
+	@Test
+	public void testNegated() throws IOException {
+		ObjectId treeId = createTree("a.sth", "a.txt", "sub/b.sth",
+				"sub/b.txt", "t.sth", "t.txt");
+
+		List<String> paths = getMatchingPaths(".txt", treeId, true, true);
+		List<String> expected = Arrays.asList("a.sth", "sub/b.sth", "t.sth");
+
+		assertEquals(expected, paths);
+	}
+
 	private ObjectId createTree(String... paths) throws IOException {
 		final ObjectInserter odi = db.newObjectInserter();
 		final DirCache dc = db.readDirCache();
@@ -80,8 +92,18 @@ public class PathSuffixFilterTest extends RepositoryTestCase {
 
 	private List<String> getMatchingPaths(String suffixFilter,
 			final ObjectId treeId, boolean recursiveWalk) throws IOException {
+		return getMatchingPaths(suffixFilter, treeId, recursiveWalk, false);
+	}
+
+	private List<String> getMatchingPaths(String suffixFilter,
+			final ObjectId treeId, boolean recursiveWalk, boolean negated)
+			throws IOException {
 		try (TreeWalk tw = new TreeWalk(db)) {
-			tw.setFilter(PathSuffixFilter.create(suffixFilter));
+			TreeFilter filter = PathSuffixFilter.create(suffixFilter);
+			if (negated) {
+				filter = filter.negate();
+			}
+			tw.setFilter(filter);
 			tw.setRecursive(recursiveWalk);
 			tw.addTree(treeId);
 
diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/treewalk/filter/PathSuffixFilter.java b/org.eclipse.jgit/src/org/eclipse/jgit/treewalk/filter/PathSuffixFilter.java
index 0c74bfdf66..3816d5ed00 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/treewalk/filter/PathSuffixFilter.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/treewalk/filter/PathSuffixFilter.java
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2009, Google Inc. and others
+ * Copyright (C) 2009, 2021 Google Inc. and others
  *
  * This program and the accompanying materials are made available under the
  * terms of the Eclipse Distribution License v. 1.0 which is available at
@@ -73,6 +73,15 @@ public class PathSuffixFilter extends TreeFilter {
 
 	}
 
+	@Override
+	public int matchFilter(TreeWalk walker) throws MissingObjectException,
+			IncorrectObjectTypeException, IOException {
+		if (walker.isSubtree()) {
+			return -1;
+		}
+		return super.matchFilter(walker);
+	}
+
 	/** {@inheritDoc} */
 	@Override
 	public boolean shouldBeRecursive() {