TreeRevFilter: Enable merge commit BF serving We currently are generating Bloom Filters for merge commits via CommitGraphWriter, but they are not being served due to the lack of tests confirming their functionality. Serving them in TreeRevFilter requires a way to rewrite their parent list in the event that BF returns false. Adding parent rewrite when BF returns false on a merge commit. Adding additional monitoring related to serving BF for merge commits so their functionality matches the default treeWalk code path behavior. Change-Id: I1250d0cf2a670966a247821cd715292fe7701b66
diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/revwalk/AbstractRevWalkWithCommitGraphTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/revwalk/AbstractRevWalkWithCommitGraphTest.java index 4276f10..98fe28b 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/revwalk/AbstractRevWalkWithCommitGraphTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/revwalk/AbstractRevWalkWithCommitGraphTest.java
@@ -63,6 +63,24 @@ protected final Ref branch(RevCommit commit, String name) throws Exception { .setStartPoint(commit.name()).call(); } + protected List<RevCommit> travel(RevWalk walk, boolean enableCommitGraph) { + db.getConfig().setBoolean(ConfigConstants.CONFIG_CORE_SECTION, null, + ConfigConstants.CONFIG_COMMIT_GRAPH, enableCommitGraph); + + List<RevCommit> commits = new ArrayList<>(); + + if (enableCommitGraph) { + assertTrue(walk.commitGraph().getCommitCnt() > 0); + } else { + assertEquals(EMPTY, walk.commitGraph()); + } + + for (RevCommit commit : walk) { + commits.add(commit); + } + return commits; + } + protected final List<RevCommit> travel(TreeFilter treeFilter, RevFilter revFilter, RevSort revSort, boolean enableCommitGraph, String... starts) @@ -79,18 +97,7 @@ protected final List<RevCommit> travel(TreeFilter treeFilter, for (String start : starts) { walk.markStart(walk.lookupCommit(db.resolve(start))); } - List<RevCommit> commits = new ArrayList<>(); - - if (enableCommitGraph) { - assertTrue(walk.commitGraph().getCommitCnt() > 0); - } else { - assertEquals(EMPTY, walk.commitGraph()); - } - - for (RevCommit commit : walk) { - commits.add(commit); - } - return commits; + return travel(walk, enableCommitGraph); } }
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..2427ee4 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
@@ -152,6 +152,220 @@ public void testTreeFilter() throws Exception { } @Test + public void testChangedPathFilterMergeCommit_followFilter() + throws Exception { + RevCommit root1 = commit(tree(file("file1", blob("1")))); + RevCommit root2 = commit(tree(file("file1", blob("2")))); + RevCommit root3 = commit(tree(file("file1", blob("3")))); + RevCommit merge1 = commit(tree(file("file1", blob("1"))), root1, root2); + RevCommit merge2 = commit(tree(file("file1", blob("1"))), merge1, + root3); + RevCommit tip2 = commit(tree(file("file1", blob("1"))), merge2); + RevCommit tip = commit(tree(file("file2", blob("1"))), tip2); + + branch(tip, "master"); + + enableAndWriteCommitGraph(); + + FollowFilter followFilter = FollowFilter.create("file2", + db.getConfig().get(DiffConfig.KEY)); + + rw.setTreeFilter(followFilter); + rw.setRevFilter(RevFilter.ALL); + rw.sort(RevSort.NONE); + rw.setRetainBody(false); + rw.markStart(rw.lookupCommit(db.resolve("master"))); + + assertCommits( + // no CG nor BF + travel(followFilter, RevFilter.ALL, RevSort.NONE, false, + "master"), + // with CG and BF + travel(rw, true)); + + RevWalk.RevFilterStats rfs = rw.getRevFilterStats(); + + // tip did a rename but didn't change content + assertEquals(1, rfs.getChangedPathFilterTruePositive()); + + assertEquals(0, rfs.getChangedPathFilterFalsePositive()); + + // tip2, merge2, merge1 didn't change content relative to their base + // parent + assertEquals(3, rfs.getChangedPathFilterNegative()); + } + + @Test + public void testChangedPathFilterMergeCommit_usedBaseParentAsRewrite() + throws Exception { + RevCommit root1 = commit(tree(file("file1", blob("1")))); + RevCommit root2 = commit(tree(file("file1", blob("2")))); + RevCommit root3 = commit(tree(file("file1", blob("3")))); + RevCommit merge1 = commit(tree(file("file1", blob("1"))), root1, root2); + RevCommit merge2 = commit(tree(file("file1", blob("1"))), merge1, + root3); + + branch(merge2, "master"); + + enableAndWriteCommitGraph(); + + ChangedPathTreeFilter changedPathTreeFilter = ChangedPathTreeFilter.create("file1"); + + rw.setTreeFilter(changedPathTreeFilter); + rw.setRevFilter(RevFilter.ALL); + rw.sort(RevSort.NONE); + rw.setRetainBody(false); + rw.markStart(rw.lookupCommit(db.resolve("master"))); + + assertCommits( + // no CG nor BF + travel(changedPathTreeFilter, RevFilter.ALL, RevSort.NONE, false, + "master"), + // with CG and BF + travel(rw, true)); + + RevWalk.RevFilterStats rfs = rw.getRevFilterStats(); + // both merge1 and merge2 used their base parent as redirect + assertEquals(2, rfs.getNumMergeCommitsUsedBaseParentAsRedirect()); + assertEquals(0, + rfs.getNumMergeCommitsUsedPullRequestParentAsRedirect()); + assertEquals(0, rfs.getNumMergeCommitsHadNoRedirect()); + assertEquals(0, + rfs.getNumMergeCommitsHadNoDiffWithAnyInterestingParent()); + } + + @Test + public void testChangedPathFilterMergeCommit_usedPullRequestParentAsRewrite() + throws Exception { + RevCommit root1 = commit(tree(file("file1", blob("1")))); + RevCommit root2 = commit(tree(file("file1", blob("2")))); + RevCommit root3 = commit(tree(file("file1", blob("3")))); + RevCommit merge1 = commit(tree(file("file1", blob("2"))), root1, root2); + RevCommit merge2 = commit(tree(file("file1", blob("2"))), root3, + merge1); + + branch(merge2, "master"); + + enableAndWriteCommitGraph(); + + ChangedPathTreeFilter changedPathTreeFilter = ChangedPathTreeFilter.create("file1"); + + rw.setTreeFilter(changedPathTreeFilter); + rw.setRevFilter(RevFilter.ALL); + rw.sort(RevSort.NONE); + rw.setRetainBody(false); + rw.markStart(rw.lookupCommit(db.resolve("master"))); + + assertCommits( + // no CG nor BF + travel(changedPathTreeFilter, RevFilter.ALL, RevSort.NONE, false, + "master"), + // with CG and BF + travel(rw, true)); + + RevWalk.RevFilterStats rfs = rw.getRevFilterStats(); + // both merge1 and merge2 used their 2nd parent as redirect + assertEquals(0, rfs.getNumMergeCommitsUsedBaseParentAsRedirect()); + assertEquals(2, + rfs.getNumMergeCommitsUsedPullRequestParentAsRedirect()); + assertEquals(0, rfs.getNumMergeCommitsHadNoRedirect()); + assertEquals(0, + rfs.getNumMergeCommitsHadNoDiffWithAnyInterestingParent()); + } + + @Test + public void testChangedPathFilterMergeCommit_noParentRedirect() + throws Exception { + RevCommit root1 = commit(tree(file("file1", blob("1")))); + RevCommit root2 = commit(tree(file("file1", blob("2")))); + RevCommit root3 = commit(tree(file("file1", blob("3")))); + RevCommit merge1 = commit(tree(file("file1", blob("4"))), root1, root2); + RevCommit merge2 = commit(tree(file("file1", blob("5"))), root3, + merge1); + + branch(merge2, "master"); + + enableAndWriteCommitGraph(); + + ChangedPathTreeFilter changedPathTreeFilter = ChangedPathTreeFilter.create("file1"); + rw.setTreeFilter(changedPathTreeFilter); + rw.setRevFilter(RevFilter.ALL); + rw.sort(RevSort.NONE); + rw.setRetainBody(false); + rw.markStart(rw.lookupCommit(db.resolve("master"))); + + assertCommits( + // no CG nor BF + travel(changedPathTreeFilter, RevFilter.ALL, RevSort.NONE, false, + "master"), + // with CG and BF + travel(rw, true)); + + RevWalk.RevFilterStats rfs = rw.getRevFilterStats(); + // both merge1 and merge2 did not need redirect since they are different + // from all of their parents + assertEquals(0, rfs.getNumMergeCommitsUsedBaseParentAsRedirect()); + assertEquals(0, + rfs.getNumMergeCommitsUsedPullRequestParentAsRedirect()); + assertEquals(2, rfs.getNumMergeCommitsHadNoRedirect()); + assertEquals(0, + rfs.getNumMergeCommitsHadNoDiffWithAnyInterestingParent()); + } + + @Test + public void testChangedPathFilterMergeCommit_noInterestingParentForRedirect() + throws Exception { + RevCommit root1 = commit(tree(file("file1", blob("1")))); + RevCommit root2 = commit(tree(file("file1", blob("2")))); + RevCommit root3 = commit(tree(file("file1", blob("2")))); + RevCommit root4 = commit(tree(file("file1", blob("3")))); + + RevCommit merge1 = commit(tree(file("file1", blob("1"))), root1, root2); + RevCommit merge2 = commit(tree(file("file1", blob("2"))), root3, root4); + RevCommit merge3 = commit(tree(file("file1", blob("1"))), merge1, + merge2); + + branch(merge3, "master"); + + ChangedPathTreeFilter changedPathTreeFilter = ChangedPathTreeFilter.create("file1"); + + RevWalk expectedRevWalk = new RevWalk(db); + expectedRevWalk.setTreeFilter(changedPathTreeFilter); + expectedRevWalk.setRevFilter(RevFilter.ALL); + expectedRevWalk.sort(RevSort.NONE); + expectedRevWalk.setRetainBody(false); + expectedRevWalk + .markStart(expectedRevWalk.lookupCommit(db.resolve("master"))); + expectedRevWalk.markUninteresting(expectedRevWalk.lookupCommit(merge1)); + expectedRevWalk.markUninteresting(expectedRevWalk.lookupCommit(root3)); + + enableAndWriteCommitGraph(); + rw.setTreeFilter(changedPathTreeFilter); + rw.setRevFilter(RevFilter.ALL); + rw.sort(RevSort.NONE); + rw.setRetainBody(false); + rw.markStart(rw.lookupCommit(db.resolve("master"))); + rw.markUninteresting(rw.lookupCommit(merge1)); + rw.markUninteresting(rw.lookupCommit(root3)); + + assertCommits( + // no CG nor BF + travel(expectedRevWalk, false), + // with CG and BF + travel(rw, true)); + + RevWalk.RevFilterStats rfs = rw.getRevFilterStats(); + // both merge3 and merge2 had same content base parent but they were + // UNINTERESTING + assertEquals(0, rfs.getNumMergeCommitsUsedBaseParentAsRedirect()); + assertEquals(0, + rfs.getNumMergeCommitsUsedPullRequestParentAsRedirect()); + assertEquals(0, rfs.getNumMergeCommitsHadNoRedirect()); + assertEquals(2, + rfs.getNumMergeCommitsHadNoDiffWithAnyInterestingParent()); + } + + @Test public void testChangedPathFilter_allModify() throws Exception { RevCommit c1 = commit(tree(file("file1", blob("1")))); RevCommit c2 = commit(tree(file("file2", blob("2"))), c1);
diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/treewalk/filter/ChangedPathTreeFilterTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/treewalk/filter/ChangedPathTreeFilterTest.java index 88f6b75..1d608da 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/treewalk/filter/ChangedPathTreeFilterTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/treewalk/filter/ChangedPathTreeFilterTest.java
@@ -88,32 +88,65 @@ public void shouldTreeWalk_yes_noCpf_noReport() { assertTrue(result); } - private static class FakeRevCommit extends RevCommit { + @Test + public void shouldTreeWalk_yes_noParents() { + ChangedPathTreeFilter f = ChangedPathTreeFilter.create("what/ever"); + boolean result = f.shouldTreeWalk(FakeRevCommit.noCpf(0), null, null); + assertTrue(result); + } + + @Test + public void shouldTreeWalk_yes_noParents_usingCpf() { + ChangedPathTreeFilter f = ChangedPathTreeFilter.create("a/b"); + // If no parents, always treewalk + boolean result = f.shouldTreeWalk( + FakeRevCommit.withCpfFor(0, "what/ever"), null, null); + + assertTrue(result); + } + + private static class FakeRevCommit extends RevCommit { static RevCommit withCpfFor(String... paths) { - return new FakeRevCommit( - ChangedPathFilter.fromPaths(Arrays.stream(paths) - .map(str -> ByteBuffer.wrap(str.getBytes(UTF_8))) - .collect(Collectors.toSet()))); + return withCpfFor(1, paths); } + static RevCommit withCpfFor(int numParents, String... paths) { + return new FakeRevCommit(numParents, + ChangedPathFilter.fromPaths(Arrays.stream(paths) + .map(str -> ByteBuffer.wrap(str.getBytes(UTF_8))) + .collect(Collectors.toSet()))); + } + static RevCommit noCpf() { - return new FakeRevCommit(null); + return noCpf(1); } + static RevCommit noCpf(int numParents) { + return new FakeRevCommit(numParents, null); + } + private final ChangedPathFilter cpf; + private final int numParents; + /** * Create a new commit reference. * * @param cpf * changedPathFilter */ - protected FakeRevCommit(ChangedPathFilter cpf) { + protected FakeRevCommit(int numParents, ChangedPathFilter cpf) { super(ObjectId.zeroId()); this.cpf = cpf; + this.numParents = numParents; } + @Override + public int getParentCount() { + return numParents; + } + @Override public ChangedPathFilter getChangedPathFilter(RevWalk rw) { return cpf;
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..ebff12b 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/revwalk/RevWalk.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/revwalk/RevWalk.java
@@ -1944,10 +1944,46 @@ public static final class RevFilterStats { private long numTreesParsedInTreeRevFilter; + private long numMergeCommitsUsedBaseParentAsRedirect; + + private long numMergeCommitsUsedPullRequestParentAsRedirect; + + private long numMergeCommitsHadNoRedirect; + + private long numMergeCommitsHadNoDiffWithAnyInterestingParent; + private RevFilterStats() { } /** + * Increment the numMergeCommitsUsedBaseParentAsRedirect count + */ + public void incrementNumMergeCommitsUsedBaseParentAsRedirect() { + numMergeCommitsUsedBaseParentAsRedirect++; + } + + /** + * Increment the numMergeCommitsUsedPullRequestParentAsRedirect count + */ + public void incrementNumMergeCommitsUsedPullRequestParentAsRedirect() { + numMergeCommitsUsedPullRequestParentAsRedirect++; + } + + /** + * Increment the numMergeCommitsHadNoDiffWithAnyInterestingParent count + */ + public void incrementNumMergeCommitsHadNoDiffButNoInterestingParent() { + numMergeCommitsHadNoDiffWithAnyInterestingParent++; + } + + /** + * Increment the numMergeCommitsHadNoRedirect count + */ + public void incrementNumMergeCommitsHadNoRedirect() { + numMergeCommitsHadNoRedirect++; + } + + /** * Increment the changedPathFilterTruePositive count */ void incrementChangedPathFilterTruePositive() { @@ -1977,14 +2013,54 @@ void incrementCommitsThroughTreeRevFilter() { /** * Increment the numTreesParsedInTreeRevFilter count + * * @param numTrees - * number of trees parsed + * number of trees */ void incrementNumTreesParsedInTreeRevFilter(int numTrees) { numTreesParsedInTreeRevFilter += numTrees; } /** + * Return the number of merge commits used the base parent to redirect + * the RevWalk + * + * @return count + */ + public long getNumMergeCommitsUsedBaseParentAsRedirect() { + return numMergeCommitsUsedBaseParentAsRedirect; + } + + /** + * Return the number of merge commits used a pull request parent to + * redirect the RevWalk + * + * @return count + */ + public long getNumMergeCommitsUsedPullRequestParentAsRedirect() { + return numMergeCommitsUsedPullRequestParentAsRedirect; + } + + /** + * Return the number of merge commits did not need be redirected + * + * @return count + */ + public long getNumMergeCommitsHadNoRedirect() { + return numMergeCommitsHadNoRedirect; + } + + /** + * Return the number of merge commits had no diff and had no interesting + * parent to redirect + * + * @return count + */ + public long getNumMergeCommitsHadNoDiffWithAnyInterestingParent() { + return numMergeCommitsHadNoDiffWithAnyInterestingParent; + } + + /** * Return how many times a changed path filter correctly predicted that * a path was changed in a commit, for statistics gathering purposes. *
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 c32b1d2..803a7d4 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/revwalk/TreeRevFilter.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/revwalk/TreeRevFilter.java
@@ -116,15 +116,18 @@ public boolean include(RevWalk walker, RevCommit c) throws IOException { private boolean shouldInclude(RevWalk walker, RevCommit c) throws IOException { changedPathFilterUsed.reset(); - if (c.getParentCount() == 1) { - TreeFilter tf = pathFilter.getFilter(); - boolean shouldTreeWalk = tf.shouldTreeWalk(c, walker, - changedPathFilterUsed); - if (!shouldTreeWalk) { - stats.incrementChangedPathFilterNegative(); - return false; + TreeFilter tf = pathFilter.getFilter(); + boolean shouldTreeWalk = tf.shouldTreeWalk(c, walker, + changedPathFilterUsed); + if (!shouldTreeWalk) { + stats.incrementChangedPathFilterNegative(); + if (c.getParentCount() > 1) { + stats.incrementNumMergeCommitsUsedBaseParentAsRedirect(); + c.parents = new RevCommit[] { c.getParent(0) }; } + return false; } + boolean shouldInclude = includeByTreeWalk(walker, c); if (changedPathFilterUsed.get()) { if (shouldInclude) { @@ -236,6 +239,12 @@ private boolean includeByTreeWalk(RevWalk walker, RevCommit c) continue; } + if (i == 0) { + stats.incrementNumMergeCommitsUsedBaseParentAsRedirect(); + } else { + stats.incrementNumMergeCommitsUsedPullRequestParentAsRedirect(); + } + c.parents = new RevCommit[] { p }; return false; } @@ -262,6 +271,7 @@ private boolean includeByTreeWalk(RevWalk walker, RevCommit c) // way from all of our parents. We have to take the blame for // that difference. // + stats.incrementNumMergeCommitsHadNoRedirect(); return true; } @@ -269,6 +279,7 @@ private boolean includeByTreeWalk(RevWalk walker, RevCommit c) // as they are and allow those parents to flow into pending // for further scanning. // + stats.incrementNumMergeCommitsHadNoDiffButNoInterestingParent(); return false; } }
diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/treewalk/filter/ChangedPathTreeFilter.java b/org.eclipse.jgit/src/org/eclipse/jgit/treewalk/filter/ChangedPathTreeFilter.java index a74b9b6..6371d2a 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/treewalk/filter/ChangedPathTreeFilter.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/treewalk/filter/ChangedPathTreeFilter.java
@@ -14,6 +14,7 @@ import org.eclipse.jgit.internal.storage.commitgraph.ChangedPathFilter; import org.eclipse.jgit.lib.Constants; import org.eclipse.jgit.revwalk.RevCommit; +import org.eclipse.jgit.revwalk.RevFlag; import org.eclipse.jgit.revwalk.RevWalk; import org.eclipse.jgit.treewalk.TreeWalk; import org.eclipse.jgit.util.StringUtils; @@ -90,6 +91,13 @@ private ChangedPathTreeFilter(String... paths) { @Override public boolean shouldTreeWalk(RevCommit c, RevWalk rw, MutableBoolean cpfUsed) { + // don't apply cpf to root commits + // other logic might have overwritten the parentList + // to shortcut the walk. + if (c.getParentCount() == 0) { + return true; + } + ChangedPathFilter cpf = c.getChangedPathFilter(rw); if (cpf == null) { return true; @@ -98,7 +106,21 @@ public boolean shouldTreeWalk(RevCommit c, RevWalk rw, cpfUsed.orValue(true); } // return true if at least one path might exist in cpf - return rawPaths.stream().anyMatch(cpf::maybeContains); + if (rawPaths.stream().anyMatch(cpf::maybeContains)) { + return true; + } + + if (c.getParentCount() == 1) { + return false; + } + + // only for merge commits + RevCommit baseParent = c.getParent(0); + if (baseParent.has(RevFlag.UNINTERESTING)) { + // merge commit CPF can only redirect to the base parent + return true; + } + return false; } @Override