Revert "RevWalk: Store RevFilter metrics within a new RevFilterStats class" This reverts commit 87615097835bce8ac687e8d7f1993d25f585afab. Reason for revert: Removing the `getChangedPathFilterXXX` methods in Ib8eff35e8e4f88efd4cb7ad40e5b603a4dbf50ff broke public API of `TreeRevFilter` Change-Id: I139c64b78429ece8cec61f1c9064620d8779964c
diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/revwalk/RevWalkCommitGraphTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/revwalk/RevWalkCommitGraphTest.java index e841e54..41b2c24 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/revwalk/RevWalkCommitGraphTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/revwalk/RevWalkCommitGraphTest.java
@@ -172,15 +172,14 @@ public void testChangedPathFilter_allModify() throws Exception { assertEquals(c1, rw.next()); assertNull(rw.next()); - RevWalk.RevFilterStats rfs = rw.getRevFilterStats(); // all commits modified file1 but c1 did not have a parent - assertEquals(3, rfs.getChangedPathFilterTruePositive()); + assertEquals(3, trf.getChangedPathFilterTruePositive()); // No false positives - assertEquals(0, rfs.getChangedPathFilterFalsePositive()); + assertEquals(0, trf.getChangedPathFilterFalsePositive()); // No negatives because all 4 commits had modified file1 - assertEquals(0, rfs.getChangedPathFilterNegative()); + assertEquals(0, trf.getChangedPathFilterNegative()); } @Test @@ -203,15 +202,14 @@ public void testChangedPathFilter_someModify() throws Exception { assertEquals(c1, rw.next()); assertNull(rw.next()); - RevWalk.RevFilterStats rfs = rw.getRevFilterStats(); // c4 and c3 modified file1. c1 did not have a parent - assertEquals(2, rfs.getChangedPathFilterTruePositive()); + assertEquals(2, trf.getChangedPathFilterTruePositive()); // No false positives - assertEquals(0, rfs.getChangedPathFilterFalsePositive()); + assertEquals(0, trf.getChangedPathFilterFalsePositive()); // c2 did not modify file1 - assertEquals(1, rfs.getChangedPathFilterNegative()); + assertEquals(1, trf.getChangedPathFilterNegative()); } @Test @@ -235,16 +233,15 @@ public void testChangedPathFilterWithMultiPaths() throws Exception { assertEquals(c1, rw.next()); assertNull(rw.next()); - RevWalk.RevFilterStats rfs = rw.getRevFilterStats(); // all commits have modified either file1 or file2, c1 did not have a // parent - assertEquals(3, rfs.getChangedPathFilterTruePositive()); + assertEquals(3, trf.getChangedPathFilterTruePositive()); // No false positives - assertEquals(0, rfs.getChangedPathFilterFalsePositive()); + assertEquals(0, trf.getChangedPathFilterFalsePositive()); // No negative - assertEquals(0, rfs.getChangedPathFilterNegative()); + assertEquals(0, trf.getChangedPathFilterNegative()); } @Test @@ -274,17 +271,16 @@ public void testChangedPathFilterWithFollowFilter() throws Exception { assertEquals(c1, rw.next()); assertNull(rw.next()); - RevWalk.RevFilterStats rfs = rw.getRevFilterStats(); // Path "renamed-file" is in c3's bloom filter, and another path "file" // is in c1's bloom filter (we know of "file" because the rev walk // detected that "renamed-file" is a renaming of "file") - assertEquals(2, rfs.getChangedPathFilterTruePositive()); + assertEquals(2, trf.getChangedPathFilterTruePositive()); // No false positives - assertEquals(0, rfs.getChangedPathFilterFalsePositive()); + assertEquals(0, trf.getChangedPathFilterFalsePositive()); // 2 commits that have exactly one parent and don't match path - assertEquals(2, rfs.getChangedPathFilterNegative()); + assertEquals(2, trf.getChangedPathFilterNegative()); } @Test @@ -317,16 +313,15 @@ public void testChangedPathFilter_pathFilter_or_pathFilter_binaryOperation() assertEquals(c1, rw.next()); assertNull(rw.next()); - RevWalk.RevFilterStats rfs = rw.getRevFilterStats(); // c2 and c3 has either file1 or file2, c1 is not counted as // ChangedPathFilter only applies to commits with 1 parent - assertEquals(2, rfs.getChangedPathFilterTruePositive()); + assertEquals(2, trf.getChangedPathFilterTruePositive()); // No false positives - assertEquals(0, rfs.getChangedPathFilterFalsePositive()); + assertEquals(0, trf.getChangedPathFilterFalsePositive()); // c4 and c5 did not modify file1 or file2 - assertEquals(2, rfs.getChangedPathFilterNegative()); + assertEquals(2, trf.getChangedPathFilterNegative()); } @Test @@ -359,16 +354,15 @@ public void testChangedPathFilter_pathFilter_or_pathFilter_or_pathFilter_listOpe assertEquals(c1, rw.next()); assertNull(rw.next()); - RevWalk.RevFilterStats rfs = rw.getRevFilterStats(); // c2 and c3 has either modified file1 or file2 or file3, c1 is not // counted as ChangedPathFilter only applies to commits with 1 parent - assertEquals(3, rfs.getChangedPathFilterTruePositive()); + assertEquals(3, trf.getChangedPathFilterTruePositive()); // No false positives - assertEquals(0, rfs.getChangedPathFilterFalsePositive()); + assertEquals(0, trf.getChangedPathFilterFalsePositive()); // c5 does not modify either file1 or file2 or file3 - assertEquals(1, rfs.getChangedPathFilterNegative()); + assertEquals(1, trf.getChangedPathFilterNegative()); } @Test @@ -396,17 +390,16 @@ public void testChangedPathFilter_pathFilter_or_nonPathFilter_binaryOperation() assertEquals(c1, rw.next()); assertNull(rw.next()); - RevWalk.RevFilterStats rfs = rw.getRevFilterStats(); // c2 modified file1, c3 defaulted positive due to ANY_DIFF, c1 is not // counted as ChangedPathFilter only applies to commits with 1 parent - assertEquals(2, rfs.getChangedPathFilterTruePositive()); + assertEquals(2, trf.getChangedPathFilterTruePositive()); // c4 defaulted positive due to ANY_DIFF, but didn't no diff with its // parent c3 - assertEquals(1, rfs.getChangedPathFilterFalsePositive()); + assertEquals(1, trf.getChangedPathFilterFalsePositive()); // No negative due to the OrTreeFilter - assertEquals(0, rfs.getChangedPathFilterNegative()); + assertEquals(0, trf.getChangedPathFilterNegative()); } @Test @@ -433,15 +426,14 @@ public void testChangedPathFilter_nonPathFilter_or_nonPathFilter_binaryOperation assertEquals(c1, rw.next()); assertNull(rw.next()); - RevWalk.RevFilterStats rfs = rw.getRevFilterStats(); // No true positives since there's no pathFilter - assertEquals(0, rfs.getChangedPathFilterTruePositive()); + assertEquals(0, trf.getChangedPathFilterTruePositive()); // No false positives since there's no pathFilter - assertEquals(0, rfs.getChangedPathFilterFalsePositive()); + assertEquals(0, trf.getChangedPathFilterFalsePositive()); // No negative since there's no pathFilter - assertEquals(0, rfs.getChangedPathFilterNegative()); + assertEquals(0, trf.getChangedPathFilterNegative()); } @Test @@ -466,18 +458,17 @@ public void testChangedPathFilter_pathFilter_and_pathFilter_binaryOperation() assertNull(rw.next()); - RevWalk.RevFilterStats rfs = rw.getRevFilterStats(); // c1 is not counted as ChangedPathFilter only applies to commits with 1 // parent - assertEquals(0, rfs.getChangedPathFilterTruePositive()); + assertEquals(0, trf.getChangedPathFilterTruePositive()); // c2 has modified both file 1 and file2, // however nothing is returned from TreeWalk since a TreeHead // cannot be two paths at once - assertEquals(1, rfs.getChangedPathFilterFalsePositive()); + assertEquals(1, trf.getChangedPathFilterFalsePositive()); // No negatives - assertEquals(0, rfs.getChangedPathFilterNegative()); + assertEquals(0, trf.getChangedPathFilterNegative()); } @Test @@ -503,17 +494,16 @@ public void testChangedPathFilter_pathFilter_and_pathFilter_and_pathFilter_listO rw.setRevFilter(trf); assertNull(rw.next()); - RevWalk.RevFilterStats rfs = rw.getRevFilterStats(); // c1 is not counted as ChangedPathFilter only applies to commits with 1 // parent - assertEquals(0, rfs.getChangedPathFilterTruePositive()); + assertEquals(0, trf.getChangedPathFilterTruePositive()); // No false positives - assertEquals(0, rfs.getChangedPathFilterFalsePositive()); + assertEquals(0, trf.getChangedPathFilterFalsePositive()); // c2 and c3 can not possibly have both file1, file2, and file3 as // treeHead at once - assertEquals(2, rfs.getChangedPathFilterNegative()); + assertEquals(2, trf.getChangedPathFilterNegative()); } @Test @@ -539,16 +529,15 @@ public void testChangedPathFilter_pathFilter_and_nonPathFilter_binaryOperation() assertEquals(c1, rw.next()); assertNull(rw.next()); - RevWalk.RevFilterStats rfs = rw.getRevFilterStats(); // c2 modified file1 and c1 is not counted as ChangedPathFilter only // applies to commits with 1 parent - assertEquals(1, rfs.getChangedPathFilterTruePositive()); + assertEquals(1, trf.getChangedPathFilterTruePositive()); // No false positives - assertEquals(0, rfs.getChangedPathFilterFalsePositive()); + assertEquals(0, trf.getChangedPathFilterFalsePositive()); // c3 did not modify file1 - assertEquals(1, rfs.getChangedPathFilterNegative()); + assertEquals(1, trf.getChangedPathFilterNegative()); } @Test @@ -574,15 +563,14 @@ public void testChangedPathFilter_nonPathFilter_and_nonPathFilter_binaryOperatio assertEquals(c1, rw.next()); assertNull(rw.next()); - RevWalk.RevFilterStats rfs = rw.getRevFilterStats(); // No true positives since there's no path - assertEquals(0, rfs.getChangedPathFilterTruePositive()); + assertEquals(0, trf.getChangedPathFilterTruePositive()); // No false positives since there's no path - assertEquals(0, rfs.getChangedPathFilterFalsePositive()); + assertEquals(0, trf.getChangedPathFilterFalsePositive()); // No negative since there's no path - assertEquals(0, rfs.getChangedPathFilterNegative()); + assertEquals(0, trf.getChangedPathFilterNegative()); } @Test
diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/revwalk/RevWalk.java b/org.eclipse.jgit/src/org/eclipse/jgit/revwalk/RevWalk.java index 54f950e..d31a89b 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/revwalk/RevWalk.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/revwalk/RevWalk.java
@@ -218,8 +218,6 @@ public class RevWalk implements Iterable<RevCommit>, AutoCloseable { private boolean firstParent; - private RevFilterStats revFilterStats; - boolean shallowCommitsInitialized; private ProgressMonitor monitor = NullProgressMonitor.INSTANCE; @@ -264,7 +262,6 @@ public RevWalk(ObjectReader or) { treeFilter = TreeFilter.ALL; this.closeReader = closeReader; commitGraph = null; - revFilterStats = new RevFilterStats(); } static AbstractRevQueue newDateRevQueue(boolean firstParent) { @@ -882,17 +879,6 @@ boolean getRewriteParents() { } /** - * Get stats recorded within the RevFilter used in the RevWalk. - * - * @return {@link RevFilterStats} with stats recorded by RevFilters. - * - * @since 7.8 - */ - public RevFilterStats getRevFilterStats() { - return revFilterStats; - } - - /** * Should the body of a commit or tag be retained after parsing its headers? * <p> * Usually the body is always retained, but some application code might not @@ -1700,7 +1686,6 @@ protected void reset(int retainFlags) { roots.clear(); queue = newDateRevQueue(firstParent); pending = new StartGenerator(this); - revFilterStats = new RevFilterStats(); } /** @@ -1723,7 +1708,6 @@ public void dispose() { queue = newDateRevQueue(firstParent); pending = new StartGenerator(this); shallowCommitsInitialized = false; - revFilterStats = new RevFilterStats(); } /** @@ -1926,111 +1910,4 @@ void initializeShallowCommits(RevCommit rc) throws IOException { } } } - - /** - * Statistics related RevFilter collected during the lifecycle of RevWalk. - * - * @since 7.8 - */ - public static final class RevFilterStats { - - private long changedPathFilterTruePositive; - - private long changedPathFilterFalsePositive; - - private long changedPathFilterNegative; - - private long numCommitsThroughTreeRevFilter; - - private long numTreesParsedInTreeRevFilter; - - private RevFilterStats() { - } - - /** - * Increment the changedPathFilterTruePositive count - */ - void incrementChangedPathFilterTruePositive() { - changedPathFilterTruePositive++; - } - - /** - * Increment the changedPathFilterFalsePositive count - */ - void incrementChangedPathFilterFalsePositive() { - changedPathFilterFalsePositive++; - } - - /** - * Increment the changedPathFilterNegative count - */ - void incrementChangedPathFilterNegative() { - changedPathFilterNegative++; - } - - /** - * Increment the numCommitsThroughTreeRevFilter count - */ - void incrementCommitsThroughTreeRevFilter() { - numCommitsThroughTreeRevFilter++; - } - - /** - * Increment the numTreesParsedInTreeRevFilter count - * @param numTrees - * number of trees parsed - */ - void incrementNumTreesParsedInTreeRevFilter(int numTrees) { - numTreesParsedInTreeRevFilter += numTrees; - } - - /** - * Return how many times a changed path filter correctly predicted that - * a path was changed in a commit, for statistics gathering purposes. - * - * @return count of true positives - */ - public long getChangedPathFilterTruePositive() { - return changedPathFilterTruePositive; - } - - /** - * Return how many times a changed path filter wrongly predicted that a - * path was changed in a commit, for statistics gathering purposes. - * - * @return count of false positives - */ - public long getChangedPathFilterFalsePositive() { - return changedPathFilterFalsePositive; - } - - /** - * Return how many times a changed path filter predicted that a path was - * not changed in a commit (allowing that commit to be skipped), for - * statistics gathering purposes. - * - * @return count of negatives - */ - public long getChangedPathFilterNegative() { - return changedPathFilterNegative; - } - - /** - * Return how many times a commit was evaluated by treeRevFilter - * - * @return count of treeRevFilter include calls - */ - public long getNumCommitsThroughTreeRevFilter() { - return numCommitsThroughTreeRevFilter; - } - - /** - * Return how many times a tree was parsed within TreeRevFilter - * - * @return count of trees parsed within TreeRevFilter - */ - public long getNumTreesParsedInTreeRevFilter() { - return numTreesParsedInTreeRevFilter; - } - } }
diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/revwalk/TreeRevFilter.java b/org.eclipse.jgit/src/org/eclipse/jgit/revwalk/TreeRevFilter.java index ba99e51..ed8640a 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/revwalk/TreeRevFilter.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/revwalk/TreeRevFilter.java
@@ -49,6 +49,12 @@ public class TreeRevFilter extends RevFilter { private final MutableBoolean changedPathFilterUsed = new MutableBoolean(); + private long changedPathFilterTruePositive = 0; + + private long changedPathFilterFalsePositive = 0; + + private long changedPathFilterNegative = 0; + /** * Create a {@link org.eclipse.jgit.revwalk.filter.RevFilter} from a * {@link org.eclipse.jgit.treewalk.filter.TreeFilter}. @@ -103,7 +109,6 @@ public RevFilter clone() { public boolean include(RevWalk walker, RevCommit c) throws StopWalkException, MissingObjectException, IncorrectObjectTypeException, IOException { - walker.getRevFilterStats().incrementCommitsThroughTreeRevFilter(); c.flags |= FILTER_APPLIED; // Reset the tree filter to scan this commit and parents. // @@ -120,8 +125,6 @@ public boolean include(RevWalk walker, RevCommit c) } trees[nParents] = c.getTree(); tw.reset(trees); - walker.getRevFilterStats() - .incrementNumTreesParsedInTreeRevFilter(trees.length); changedPathFilterUsed.reset(); if (nParents == 1) { @@ -143,17 +146,14 @@ public boolean include(RevWalk walker, RevCommit c) } if (changedPathFilterUsed.get()) { if (chgs > 0) { - walker.getRevFilterStats() - .incrementChangedPathFilterTruePositive(); + changedPathFilterTruePositive++; } else { - walker.getRevFilterStats() - .incrementChangedPathFilterFalsePositive(); + changedPathFilterFalsePositive++; } } } else { if (changedPathFilterUsed.get()) { - walker.getRevFilterStats() - .incrementChangedPathFilterNegative(); + changedPathFilterNegative++; } } @@ -272,6 +272,40 @@ public boolean requiresCommitBody() { return false; } + /** + * Return how many times a changed path filter correctly predicted that a + * path was changed in a commit, for statistics gathering purposes. + * + * @return count of true positives + * @since 6.7 + */ + public long getChangedPathFilterTruePositive() { + return changedPathFilterTruePositive; + } + + /** + * Return how many times a changed path filter wrongly predicted that a path + * was changed in a commit, for statistics gathering purposes. + * + * @return count of false positives + * @since 6.7 + */ + public long getChangedPathFilterFalsePositive() { + return changedPathFilterFalsePositive; + } + + /** + * Return how many times a changed path filter predicted that a path was not + * changed in a commit (allowing that commit to be skipped), for statistics + * gathering purposes. + * + * @return count of negatives + * @since 6.7 + */ + public long getChangedPathFilterNegative() { + return changedPathFilterNegative; + } + private void updateFollowFilter(ObjectId[] trees, DiffConfig cfg, RevCommit commit) throws IOException { TreeWalk tw = pathFilter;