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 296806723830b0d36c5ce52237c0a87e9ce01bd2..5edc1924f22f977315cefd83ed91461181635358 100644 (file)
--- 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 06dc0bf6d0c7e19778646ebf54991d8c5f1e5d8b..438549520efaf3d9fda2664fd0df45f9455c67b2 100644 (file)
--- 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 bdfde0bfcd089bda6612aff022bd21cb1e555b37..7601956c435c9ca2a8eecdc354526bf309d39c58 100644 (file)
--- 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);