From 72fa0a53e79265c11703ce848408ed328aed1e55 Mon Sep 17 00:00:00 2001 From: Xing Huang Date: Thu, 7 Mar 2024 11:41:36 -0600 Subject: TreeRevFilter: correct changedPathFilter usage for multi-paths inclusion The expected behavior of TreeRevFilter when filtering multiple file paths is to include commits that changed at least one of the given paths; only skipping them if they did not change any of the given paths. The current changedPathFilter utilization logic is skipping a commit if there exists at least one given path that the commit did not change, disregarding the rest of the given paths. Enforcing all given paths to be checked by the changedPathFilter, only skipping a commit if changedPathFilter return negative on all given paths. Signed-off-by: Xing Huang Change-Id: Ib7a9e496b37ec737722fbf33c5d0f05d5d910a8d --- .../jgit/revwalk/RevWalkCommitGraphTest.java | 30 ++++++++++++++++++++++ .../org/eclipse/jgit/revwalk/TreeRevFilter.java | 7 ++--- .../eclipse/jgit/treewalk/filter/TreeFilter.java | 2 +- 3 files changed, 33 insertions(+), 6 deletions(-) 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 8215a795b2..c2f8f10631 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 @@ -38,6 +38,7 @@ import org.eclipse.jgit.revwalk.filter.RevFilter; import org.eclipse.jgit.storage.file.FileBasedConfig; import org.eclipse.jgit.treewalk.filter.AndTreeFilter; import org.eclipse.jgit.treewalk.filter.PathFilter; +import org.eclipse.jgit.treewalk.filter.PathFilterGroup; import org.eclipse.jgit.treewalk.filter.TreeFilter; import org.junit.Test; @@ -196,6 +197,35 @@ public class RevWalkCommitGraphTest extends RevWalkTestCase { assertEquals(2, trf.getChangedPathFilterNegative()); } + @Test + public void testChangedPathFilterWithMultiPaths() throws Exception { + RevCommit c1 = commitFile("file1", "1", "master"); + RevCommit c2 = commitFile("file1", "2", "master"); + RevCommit c3 = commitFile("file2", "3", "master"); + RevCommit c4 = commitFile("file3", "4", "master"); + + enableAndWriteCommitGraph(); + + TreeRevFilter trf = new TreeRevFilter(rw, + PathFilterGroup.createFromStrings(List.of("file1", "file2"))); + rw.markStart(rw.lookupCommit(c4)); + rw.setRevFilter(trf); + assertEquals(c3, rw.next()); + assertEquals(c2, rw.next()); + assertEquals(c1, rw.next()); + assertNull(rw.next()); + + // c2 and c3 has either file1 or file2, c1 did not use ChangedPathFilter + // since it has no parent + assertEquals(2, trf.getChangedPathFilterTruePositive()); + + // No false positives + assertEquals(0, trf.getChangedPathFilterFalsePositive()); + + // c4 does not match either file1 or file2 + assertEquals(1, trf.getChangedPathFilterNegative()); + } + @Test public void testChangedPathFilterWithFollowFilter() throws Exception { RevCommit c0 = commit(tree()); 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 43571a6868..99943b78e6 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/revwalk/TreeRevFilter.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/revwalk/TreeRevFilter.java @@ -139,11 +139,8 @@ public class TreeRevFilter extends RevFilter { .getPathsBestEffort(); if (paths.isPresent()) { changedPathFilterUsed = true; - for (byte[] path : paths.get()) { - if (!cpf.maybeContains(path)) { - mustCalculateChgs = false; - break; - } + if (paths.get().stream().noneMatch(cpf::maybeContains)) { + mustCalculateChgs = false; } } } diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/treewalk/filter/TreeFilter.java b/org.eclipse.jgit/src/org/eclipse/jgit/treewalk/filter/TreeFilter.java index 22d430bc27..a9066dc8f8 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/treewalk/filter/TreeFilter.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/treewalk/filter/TreeFilter.java @@ -210,7 +210,7 @@ public abstract class TreeFilter { public abstract boolean shouldBeRecursive(); /** - * If this filter checks that a specific set of paths have all been + * If this filter checks that at least one of the paths in a set has been * modified, returns that set of paths to be checked against a changed path * filter. Otherwise, returns empty. * -- cgit v1.2.3