From db5ce6b5c24408ae27eb6fa0b6289d51b129baeb Mon Sep 17 00:00:00 2001 From: Ronald Bhuleskar Date: Wed, 25 Oct 2023 13:10:33 -0700 Subject: [PATCH] StartGenerator: Fix parent rewrite with non-default RevFilter StartGenerator is responsible for propagating the RevWalk's parent rewrite setting, but it currently only does so when a non-default TreeFilter is set, when it should also do so if the default TreeFilter is used with a non-default RevFilter. Adding a new if condition within StartGenerator to enable parent rewrite with non-default RevFilter. TreeRevFilter relied on the old buggy functionality and has been modified to explicitly refrain from rewriting parents. Change-Id: I4e4ff67fb279edbcc3461496b132cea774fb742f --- .../jgit/revwalk/TreeRevFilterTest.java | 1 + .../TreeRevFilterWithRewriteParentsTest.java | 112 ++++++++++++++++++ .../eclipse/jgit/revwalk/StartGenerator.java | 10 +- .../eclipse/jgit/revwalk/TreeRevFilter.java | 24 +--- 4 files changed, 123 insertions(+), 24 deletions(-) create mode 100644 org.eclipse.jgit.test/tst/org/eclipse/jgit/revwalk/TreeRevFilterWithRewriteParentsTest.java diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/revwalk/TreeRevFilterTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/revwalk/TreeRevFilterTest.java index 298facfd1..f67a623ff 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/revwalk/TreeRevFilterTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/revwalk/TreeRevFilterTest.java @@ -21,6 +21,7 @@ public class TreeRevFilterTest extends RevWalkTestCase { private RevFilter treeRevFilter() { + rw.setRewriteParents(false); return new TreeRevFilter(rw, TreeFilter.ANY_DIFF); } diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/revwalk/TreeRevFilterWithRewriteParentsTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/revwalk/TreeRevFilterWithRewriteParentsTest.java new file mode 100644 index 000000000..100f2e416 --- /dev/null +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/revwalk/TreeRevFilterWithRewriteParentsTest.java @@ -0,0 +1,112 @@ +/* + * Copyright (C) 2023, Google LLC and others + * + * This program and the accompanying materials are made available under the + * terms of the Eclipse Distribution License v. 1.0 which is available at + * https://www.eclipse.org/org/documents/edl-v10.php. + * + * SPDX-License-Identifier: BSD-3-Clause + */ + +package org.eclipse.jgit.revwalk; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNull; + +import org.eclipse.jgit.revwalk.filter.RevFilter; +import org.eclipse.jgit.treewalk.filter.TreeFilter; +import org.junit.Test; + +public class TreeRevFilterWithRewriteParentsTest extends RevWalkTestCase { + private RevFilter treeRevFilter() { + rw.setRewriteParents(true); + return new TreeRevFilter(rw, TreeFilter.ANY_DIFF); + } + + @Test + public void testStringOfPearls_FilePath1() + throws Exception { + RevCommit a = commit(tree(file("d/f", blob("a")))); + RevCommit b = commit(tree(file("d/f", blob("a"))), a); + RevCommit c = commit(tree(file("d/f", blob("b"))), b); + rw.setRevFilter(treeRevFilter()); + markStart(c); + + assertCommit(c, rw.next()); + assertEquals(1, c.getParentCount()); + assertCommit(a, c.getParent(0)); + + assertCommit(a, rw.next()); // b was skipped + assertEquals(0, a.getParentCount()); + assertNull(rw.next()); + } + + @Test + public void testStringOfPearls_FilePath2() throws Exception { + RevCommit a = commit(tree(file("d/f", blob("a")))); + RevCommit b = commit(tree(file("d/f", blob("a"))), a); + RevCommit c = commit(tree(file("d/f", blob("b"))), b); + RevCommit d = commit(tree(file("d/f", blob("b"))), c); + rw.setRevFilter(treeRevFilter()); + markStart(d); + + // d was skipped + assertCommit(c, rw.next()); + assertEquals(1, c.getParentCount()); + assertCommit(a, c.getParent(0)); + + // b was skipped + assertCommit(a, rw.next()); + assertEquals(0, a.getParentCount()); + assertNull(rw.next()); + } + + @Test + public void testStringOfPearls_DirPath2() throws Exception { + RevCommit a = commit(tree(file("d/f", blob("a")))); + RevCommit b = commit(tree(file("d/f", blob("a"))), a); + RevCommit c = commit(tree(file("d/f", blob("b"))), b); + RevCommit d = commit(tree(file("d/f", blob("b"))), c); + rw.setRevFilter(treeRevFilter()); + markStart(d); + + // d was skipped + assertCommit(c, rw.next()); + assertEquals(1, c.getParentCount()); + assertCommit(a, c.getParent(0)); + + // b was skipped + assertCommit(a, rw.next()); + assertEquals(0, a.getParentCount()); + assertNull(rw.next()); + } + + @Test + public void testStringOfPearls_FilePath3() throws Exception { + RevCommit a = commit(tree(file("d/f", blob("a")))); + RevCommit b = commit(tree(file("d/f", blob("a"))), a); + RevCommit c = commit(tree(file("d/f", blob("b"))), b); + RevCommit d = commit(tree(file("d/f", blob("b"))), c); + RevCommit e = commit(tree(file("d/f", blob("b"))), d); + RevCommit f = commit(tree(file("d/f", blob("b"))), e); + RevCommit g = commit(tree(file("d/f", blob("b"))), f); + RevCommit h = commit(tree(file("d/f", blob("b"))), g); + RevCommit i = commit(tree(file("d/f", blob("c"))), h); + rw.setRevFilter(treeRevFilter()); + markStart(i); + + assertCommit(i, rw.next()); + assertEquals(1, i.getParentCount()); + assertCommit(c, i.getParent(0)); + + // h..d was skipped + assertCommit(c, rw.next()); + assertEquals(1, c.getParentCount()); + assertCommit(a, c.getParent(0)); + + // b was skipped + assertCommit(a, rw.next()); + assertEquals(0, a.getParentCount()); + assertNull(rw.next()); + } +} diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/revwalk/StartGenerator.java b/org.eclipse.jgit/src/org/eclipse/jgit/revwalk/StartGenerator.java index a79901ca1..2f7c4d579 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/revwalk/StartGenerator.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/revwalk/StartGenerator.java @@ -97,14 +97,14 @@ RevCommit next() throws MissingObjectException, pending = (DateRevQueue)q; else pending = new DateRevQueue(q); + if (rf != RevFilter.ALL && w.getRewriteParents()) { + pendingOutputType |= HAS_REWRITE | NEEDS_REWRITE; + } if (tf != TreeFilter.ALL) { - int rewriteFlag; if (w.getRewriteParents()) { pendingOutputType |= HAS_REWRITE | NEEDS_REWRITE; - rewriteFlag = RevWalk.REWRITE; - } else - rewriteFlag = 0; - rf = AndRevFilter.create(new TreeRevFilter(w, tf, rewriteFlag), rf); + } + rf = AndRevFilter.create(new TreeRevFilter(w, tf), rf); } walker.queue = q; 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 43571a686..408595463 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/revwalk/TreeRevFilter.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/revwalk/TreeRevFilter.java @@ -60,21 +60,8 @@ public class TreeRevFilter extends RevFilter { * Create a {@link org.eclipse.jgit.revwalk.filter.RevFilter} from a * {@link org.eclipse.jgit.treewalk.filter.TreeFilter}. * - * @param walker - * walker used for reading trees. - * @param t - * filter to compare against any changed paths in each commit. If - * a {@link org.eclipse.jgit.revwalk.FollowFilter}, will be - * replaced with a new filter following new paths after a rename. - * @since 3.5 - */ - public TreeRevFilter(RevWalk walker, TreeFilter t) { - this(walker, t, 0); - } - - /** - * Create a filter for the first phase of a parent-rewriting limited - * revision walk. + * When revWalk's rewrite parent flag is set, it creates a filter for the + * first phase of a parent-rewriting limited revision walk. *

* This filter is ANDed to evaluate before all other filters and ties the * configured {@link TreeFilter} into the revision walking process. @@ -91,14 +78,13 @@ public TreeRevFilter(RevWalk walker, TreeFilter t) { * filter to compare against any changed paths in each commit. If * a {@link FollowFilter}, will be replaced with a new filter * following new paths after a rename. - * @param rewriteFlag - * flag to color commits to be removed from the simplified DAT. + * @since 3.5 */ - TreeRevFilter(RevWalk walker, TreeFilter t, int rewriteFlag) { + public TreeRevFilter(RevWalk walker, TreeFilter t) { pathFilter = new TreeWalk(walker.reader); pathFilter.setFilter(t); pathFilter.setRecursive(t.shouldBeRecursive()); - this.rewriteFlag = rewriteFlag; + this.rewriteFlag = walker.getRewriteParents() ? RevWalk.REWRITE : 0; } @Override