From: Jonathan Nieder <jrn@google.com>
Date: Tue, 15 Dec 2015 03:57:24 +0000 (-0800)
Subject: Do not let PathFilter.create("a/b") match 'a' unless 'a' is a subtree
X-Git-Tag: v4.2.0.201601211800-r~4^2~64^2
X-Git-Url: https://source.dussan.org/?a=commitdiff_plain;h=refs%2Fchanges%2F91%2F62691%2F2;p=jgit.git

Do not let PathFilter.create("a/b") match 'a' unless 'a' is a subtree

PathFilter and PathFilterGroup form JGit's implementation of git's
path-limiting feature in commands like log and diff.  To save time
when traversing trees, a path specification

	foo/bar/baz

tells the tree walker not to traverse unrelated trees like qux/.  It
does that by returning false from include when the tree walker is
visiting qux and true when it is visiting foo.

Unfortunately that test was implemented to be slightly over-eager: it
doesn't only return true when asked whether to visit a subtree "foo"
but when asked about a plain file "foo" as well.  As a result, diffs
and logs restricted to some-file/non-existing-suffix unexpectedly
match against some-file:

 $ jgit log -- LICENSE/no-such-file
 commit 629fd0d594d242eab26161b0dac34f7576fd4d3d
 Author: Shawn O. Pearce <spearce@spearce.org>
 Date:   Fri Jul 02 14:52:49 2010 -0700

     Clean up LICENSE file
[...]

Fix it by checking against the entry's mode.

Gitiles +log has the same bug and benefits from the same fix.

Callers know not to worry about what subtrees are included in the tree
walk because shouldBeRecursive() returns true in this case, so this
behavior change should be safe.  This also better matches the behavior
of C git:

 $ empty=$(git mktree </dev/null)
 $ git diff-tree --abbrev $empty HEAD -- LICENSE/no-such-file
 $ git diff-tree --abbrev $empty HEAD -- tools/no-such-file
 :000000 040000 0000000... b62648d... A  tools

Bug: 484266
Change-Id: Ib4d53bddd8413a9548622c7b25b338d287d8889d
---

diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/treewalk/filter/PathFilterGroupTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/treewalk/filter/PathFilterGroupTest.java
index 2968067238..5edc1924f2 100644
--- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/treewalk/filter/PathFilterGroupTest.java
+++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/treewalk/filter/PathFilterGroupTest.java
@@ -132,12 +132,22 @@ public class PathFilterGroupTest {
 	@Test
 	public void testKeyIsPrefixOfFilter() throws MissingObjectException,
 			IncorrectObjectTypeException, IOException {
-		assertMatches(Sets.of("b/c"), fakeWalk("b"));
-		assertMatches(Sets.of("c/d/e", "c/d/f"), fakeWalk("c/d"));
-		assertMatches(Sets.of("c/d/e", "c/d/f"), fakeWalk("c"));
-		assertMatches(Sets.of("d/e/f/g", "d/e/f/g.x"), fakeWalk("d/e/f"));
-		assertMatches(Sets.of("d/e/f/g", "d/e/f/g.x"), fakeWalk("d/e"));
-		assertMatches(Sets.of("d/e/f/g", "d/e/f/g.x"), fakeWalk("d"));
+		assertMatches(Sets.of("b/c"), fakeWalkAtSubtree("b"));
+		assertMatches(Sets.of("c/d/e", "c/d/f"), fakeWalkAtSubtree("c/d"));
+		assertMatches(Sets.of("c/d/e", "c/d/f"), fakeWalkAtSubtree("c"));
+		assertMatches(Sets.of("d/e/f/g", "d/e/f/g.x"),
+				fakeWalkAtSubtree("d/e/f"));
+		assertMatches(Sets.of("d/e/f/g", "d/e/f/g.x"),
+				fakeWalkAtSubtree("d/e"));
+		assertMatches(Sets.of("d/e/f/g", "d/e/f/g.x"), fakeWalkAtSubtree("d"));
+
+		assertNoMatches(fakeWalk("b"));
+		assertNoMatches(fakeWalk("c/d"));
+		assertNoMatches(fakeWalk("c"));
+		assertNoMatches(fakeWalk("d/e/f"));
+		assertNoMatches(fakeWalk("d/e"));
+		assertNoMatches(fakeWalk("d"));
+
 	}
 
 	@Test
@@ -263,4 +273,25 @@ public class PathFilterGroupTest {
 		return ret;
 	}
 
+	TreeWalk fakeWalkAtSubtree(final String path) throws IOException {
+		DirCache dc = DirCache.newInCore();
+		DirCacheEditor dce = dc.editor();
+		dce.add(new DirCacheEditor.PathEdit(path + "/README") {
+			public void apply(DirCacheEntry ent) {
+				ent.setFileMode(FileMode.REGULAR_FILE);
+			}
+		});
+		dce.finish();
+
+		TreeWalk ret = new TreeWalk((ObjectReader) null);
+		ret.addTree(new DirCacheIterator(dc));
+		ret.next();
+		while (!path.equals(ret.getPathString())) {
+			if (ret.isSubtree()) {
+				ret.enterSubtree();
+			}
+			ret.next();
+		}
+		return ret;
+	}
 }
diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/treewalk/TreeWalk.java b/org.eclipse.jgit/src/org/eclipse/jgit/treewalk/TreeWalk.java
index 06dc0bf6d0..438549520e 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/treewalk/TreeWalk.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/treewalk/TreeWalk.java
@@ -861,10 +861,13 @@ public class TreeWalk implements AutoCloseable, AttributesProvider {
 	 * Test if the supplied path matches the current entry's path.
 	 * <p>
 	 * This method tests that the supplied path is exactly equal to the current
-	 * entry, or is one of its parent directories. It is faster to use this
+	 * entry or is one of its parent directories. It is faster to use this
 	 * method then to use {@link #getPathString()} to first create a String
 	 * object, then test <code>startsWith</code> or some other type of string
 	 * match function.
+	 * <p>
+	 * If the current entry is a subtree, then all paths within the subtree
+	 * are considered to match it.
 	 *
 	 * @param p
 	 *            path buffer to test. Callers should ensure the path does not
@@ -900,7 +903,7 @@ public class TreeWalk implements AutoCloseable, AttributesProvider {
 			// If p[ci] == '/' then pattern matches this subtree,
 			// otherwise we cannot be certain so we return -1.
 			//
-			return p[ci] == '/' ? 0 : -1;
+			return p[ci] == '/' && FileMode.TREE.equals(t.mode) ? 0 : -1;
 		}
 
 		// Both strings are identical.
diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/treewalk/filter/PathFilterGroup.java b/org.eclipse.jgit/src/org/eclipse/jgit/treewalk/filter/PathFilterGroup.java
index bdfde0bfcd..7601956c43 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/treewalk/filter/PathFilterGroup.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/treewalk/filter/PathFilterGroup.java
@@ -245,9 +245,9 @@ public class PathFilterGroup {
 				int hash = hasher.nextHash();
 				if (fullpaths.contains(rp, hasher.length(), hash))
 					return true;
-				if (!hasher.hasNext())
-					if (prefixes.contains(rp, hasher.length(), hash))
-						return true;
+				if (!hasher.hasNext() && walker.isSubtree()
+						&& prefixes.contains(rp, hasher.length(), hash))
+					return true;
 			}
 
 			final int cmp = walker.isPathPrefix(max, max.length);