diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/RebaseCommandTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/RebaseCommandTest.java index 86239023d..d1522e98e 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/RebaseCommandTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/RebaseCommandTest.java @@ -563,10 +563,10 @@ public class RebaseCommandTest extends RepositoryTestCase { RevCommit newD = rw.next(); assertDerivedFrom(newD, d); assertEquals(2, newD.getParentCount()); - RevCommit newC = rw.next(); - assertDerivedFrom(newC, c); RevCommit newE = rw.next(); assertEquals(e, newE); + RevCommit newC = rw.next(); + assertDerivedFrom(newC, c); assertEquals(newC, newD.getParent(0)); assertEquals(e, newD.getParent(1)); assertEquals(g, rw.next()); diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/revplot/PlotCommitListTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/revplot/PlotCommitListTest.java index 4e0bba2f2..45225a2bd 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/revplot/PlotCommitListTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/revplot/PlotCommitListTest.java @@ -254,12 +254,12 @@ public class PlotCommitListTest extends RevWalkTestCase { int posI = test.commit(i).lanePos(childPositions).parents(h) .getLanePos(); test.commit(h).lanePos(posI).parents(f); - test.commit(g).lanePos(childPositions).parents(a); test.commit(f).lanePos(posI).parents(e, d); + test.commit(d).lanePos(1).parents(b); test.commit(e).lanePos(posI).parents(c); - test.commit(d).lanePos(2).parents(b); test.commit(c).lanePos(posI).parents(b); test.commit(b).lanePos(posI).parents(a); + test.commit(g).lanePos(childPositions).parents(a); test.commit(a).lanePos(0).parents(); } } @@ -325,42 +325,42 @@ public class PlotCommitListTest extends RevWalkTestCase { .lanePos(mainPos); test.commit(merge_update_eclipse) .parents(add_a_clear, update_eclipse).lanePos(mainPos); + test.commit(update_eclipse).parents(add_Maven).lanePos(2); test.commit(add_a_clear).parents(fix_broken).lanePos(mainPos); test.commit(fix_broken).parents(merge_disable_comment) .lanePos(mainPos); test.commit(merge_disable_comment) .parents(merge_resolve_handler, disable_comment) .lanePos(mainPos); - test.commit(disable_comment).parents(clone_operation).lanePos(2); + test.commit(disable_comment).parents(clone_operation).lanePos(3); test.commit(merge_resolve_handler) .parents(clone_operation, resolve_handler).lanePos(mainPos); - test.commit(update_eclipse).parents(add_Maven).lanePos(3); + test.commit(resolve_handler).parents(merge_fix).lanePos(4); test.commit(clone_operation).parents(merge_changeset_implementation) .lanePos(mainPos); test.commit(merge_changeset_implementation) .parents(merge_disable_source, changeset_implementation) .lanePos(mainPos); + test.commit(changeset_implementation).parents(clear_repositorycache) + .lanePos(1); test.commit(merge_disable_source) .parents(update_eclipse_iplog2, disable_source) .lanePos(mainPos); + test.commit(disable_source).parents(merge_use_remote).lanePos(3); test.commit(update_eclipse_iplog2).parents(merge_use_remote) .lanePos(mainPos); - test.commit(disable_source).parents(merge_use_remote).lanePos(1); test.commit(merge_use_remote) .parents(update_eclipse_iplog, use_remote).lanePos(mainPos); - test.commit(changeset_implementation).parents(clear_repositorycache) - .lanePos(2); + test.commit(use_remote).parents(clear_repositorycache).lanePos(3); test.commit(update_eclipse_iplog).parents(merge_add_Maven) .lanePos(mainPos); test.commit(merge_add_Maven).parents(findToolBar_layout, add_Maven) .lanePos(mainPos); + test.commit(add_Maven).parents(clear_repositorycache).lanePos(2); test.commit(findToolBar_layout).parents(clear_repositorycache) .lanePos(mainPos); - test.commit(use_remote).parents(clear_repositorycache).lanePos(1); - test.commit(add_Maven).parents(clear_repositorycache).lanePos(3); test.commit(clear_repositorycache).parents(merge_remove) .lanePos(mainPos); - test.commit(resolve_handler).parents(merge_fix).lanePos(4); test.commit(merge_remove).parents(add_simple, remove_unused) .lanePos(mainPos); test.commit(remove_unused).parents(merge_fix).lanePos(1); @@ -453,33 +453,36 @@ public class PlotCommitListTest extends RevWalkTestCase { pcl.source(pw); pcl.fillTo(Integer.MAX_VALUE); - // test that the commits b1, b2 and b3 are on the same position - int bPos = pcl.get(9).lane.position; // b1 - assertEquals("b2 is an a different position", bPos, - pcl.get(7).lane.position); - assertEquals("b3 is on a different position", bPos, - pcl.get(4).lane.position); - - // test that nothing blocks the connections between b1, b2 and b3 - assertNotEquals("b lane is blocked by c", bPos, - pcl.get(8).lane.position); - assertNotEquals("b lane is blocked by a2", bPos, - pcl.get(6).lane.position); - assertNotEquals("b lane is blocked by d", bPos, - pcl.get(5).lane.position); + Set positions = asSet(0, 1); + CommitListAssert test = new CommitListAssert(pcl); + int posA = test.commit(a5).lanePos(positions).getLanePos(); + test.commit(a4); + test.commit(a3).lanePos(posA); + test.commit(e); + test.commit(d); + test.commit(a2).lanePos(posA); + int posB = test.commit(b3).lanePos(positions).getLanePos(); + test.commit(b2).lanePos(posB); + test.commit(b1).lanePos(posB); + test.commit(c); + test.commit(a1).lanePos(posA); + test.noMoreCommits(); + assertNotEquals("a lane is the same as b lane", posA, posB); } } /** *
 	 *    b3
+	 * a5 |
+	 * |  |
 	 * a4 |
 	 * | \|
 	 * |  b2
 	 * a3 |
 	 * | \|
-	 * a2 |
 	 * |  b1
+	 * a2 |
 	 * | /
 	 * a1
 	 * 
@@ -494,10 +497,11 @@ public class PlotCommitListTest extends RevWalkTestCase { final RevCommit a3 = commit(a2, b1); final RevCommit b2 = commit(b1); final RevCommit a4 = commit(a3, b2); + final RevCommit a5 = commit(a4); final RevCommit b3 = commit(b2); try (PlotWalk pw = new PlotWalk(db)) { - pw.markStart(pw.lookupCommit(a4)); + pw.markStart(pw.lookupCommit(a5)); pw.markStart(pw.lookupCommit(b3)); PlotCommitList pcl = new PlotCommitList<>(); pcl.source(pw); @@ -506,11 +510,12 @@ public class PlotCommitListTest extends RevWalkTestCase { Set positions = asSet(0, 1); CommitListAssert test = new CommitListAssert(pcl); int posB = test.commit(b3).lanePos(positions).getLanePos(); - int posA = test.commit(a4).lanePos(positions).getLanePos(); + int posA = test.commit(a5).lanePos(positions).getLanePos(); + test.commit(a4).lanePos(posA); test.commit(b2).lanePos(posB); test.commit(a3).lanePos(posA); - test.commit(a2).lanePos(posA); test.commit(b1).lanePos(posB); + test.commit(a2).lanePos(posA); test.commit(a1).lanePos(posA); test.noMoreCommits(); } @@ -519,13 +524,17 @@ public class PlotCommitListTest extends RevWalkTestCase { /** *
 	 * a4
-	 * |   b3
-	 * a3  |
-	 * | \\|
-	 * |   |\\
-	 * |   b2||
-	 * a2  | //
-	 * |   b1
+	 * |
+	 * a3
+	 * | \\
+	 * a2  \\
+	 * |    \\
+	 * |  b3 ||
+	 * |  |  ||
+	 * |  b2 ||
+	 * |  | //
+	 * |  b1
+	 * |  |
 	 * | /
 	 * a1
 	 * 
@@ -552,10 +561,10 @@ public class PlotCommitListTest extends RevWalkTestCase { Set positions = asSet(0, 1); CommitListAssert test = new CommitListAssert(pcl); int posA = test.commit(a4).lanePos(positions).getLanePos(); - int posB = test.commit(b3).lanePos(positions).getLanePos(); test.commit(a3).lanePos(posA); - test.commit(b2).lanePos(posB); test.commit(a2).lanePos(posA); + int posB = test.commit(b3).lanePos(positions).getLanePos(); + test.commit(b2).lanePos(posB); // b1 is not repositioned, uses "detour lane" // (drawn as a double arc in the ascii graph above) test.commit(b1).lanePos(posB); @@ -569,13 +578,14 @@ public class PlotCommitListTest extends RevWalkTestCase { * b2 * a4 | * | \ | - * a3 \| + * | b1 + * a3 | * | \ | * | c | * | / | * a2 | - * | b1 - * / + * | | + * | / * | / * a1 * @@ -604,10 +614,10 @@ public class PlotCommitListTest extends RevWalkTestCase { CommitListAssert test = new CommitListAssert(pcl); int posB = test.commit(b2).lanePos(positions).getLanePos(); int posA = test.commit(a4).lanePos(positions).getLanePos(); + test.commit(b1).lanePos(posB); // repositioned to go around c test.commit(a3).lanePos(posA); test.commit(c).lanePos(positions); test.commit(a2).lanePos(posA); - test.commit(b1).lanePos(posB); // repositioned to go around c test.commit(a1).lanePos(posA); test.noMoreCommits(); } diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/revwalk/RevWalkSortTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/revwalk/RevWalkSortTest.java index 6f110fa31..3f29e09e3 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/revwalk/RevWalkSortTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/revwalk/RevWalkSortTest.java @@ -144,4 +144,110 @@ public class RevWalkSortTest extends RevWalkTestCase { assertCommit(d, rw.next()); assertNull(rw.next()); } + + @Test + public void testSort_TOPO_OutOfOrderCommitTimes() throws Exception { + // b is committed before c2 in a different line of history. + // + final RevCommit a = commit(); + final RevCommit c1 = commit(a); + final RevCommit b = commit(a); + final RevCommit c2 = commit(c1); + final RevCommit d = commit(b, c2); + + rw.sort(RevSort.TOPO); + markStart(d); + assertCommit(d, rw.next()); + assertCommit(c2, rw.next()); + assertCommit(c1, rw.next()); + assertCommit(b, rw.next()); + assertCommit(a, rw.next()); + assertNull(rw.next()); + } + + @Test + public void testSort_TOPO_MultipleLinesOfHistory() throws Exception { + final RevCommit a1 = commit(); + final RevCommit b1 = commit(a1); + final RevCommit a2 = commit(a1, b1); + final RevCommit b2 = commit(b1); + final RevCommit b3 = commit(b1); + final RevCommit a3 = commit(a2, b2); + final RevCommit a4 = commit(a3, b3); + + rw.sort(RevSort.TOPO); + markStart(a4); + assertCommit(a4, rw.next()); + assertCommit(b3, rw.next()); + assertCommit(a3, rw.next()); + assertCommit(b2, rw.next()); + assertCommit(a2, rw.next()); + assertCommit(b1, rw.next()); + assertCommit(a1, rw.next()); + assertNull(rw.next()); + } + + @Test + public void testSort_TOPO_REVERSE_MultipleLinesOfHistory() + throws Exception { + final RevCommit a1 = commit(); + final RevCommit b1 = commit(a1); + final RevCommit a2 = commit(a1, b1); + final RevCommit b2 = commit(b1); + final RevCommit b3 = commit(b1); + final RevCommit a3 = commit(a2, b2); + final RevCommit a4 = commit(a3, b3); + + rw.sort(RevSort.TOPO); + rw.sort(RevSort.REVERSE, true); + markStart(a4); + assertCommit(a1, rw.next()); + assertCommit(b1, rw.next()); + assertCommit(a2, rw.next()); + assertCommit(b2, rw.next()); + assertCommit(a3, rw.next()); + assertCommit(b3, rw.next()); + assertCommit(a4, rw.next()); + assertNull(rw.next()); + } + + @Test + public void testSort_TOPO_ParentOfMultipleStartChildren() throws Exception { + final RevCommit a = commit(); + final RevCommit b = commit(a); + final RevCommit c = commit(a); + final RevCommit d1 = commit(a); + final RevCommit d2 = commit(d1); + final RevCommit e = commit(a); + + rw.sort(RevSort.TOPO); + markStart(b); + markStart(c); + markStart(d2); + markStart(e); + assertCommit(e, rw.next()); + assertCommit(d2, rw.next()); + assertCommit(d1, rw.next()); + assertCommit(c, rw.next()); + assertCommit(b, rw.next()); + assertCommit(a, rw.next()); + assertNull(rw.next()); + } + + @Test + public void testSort_TOPO_Uninteresting() throws Exception { + final RevCommit a1 = commit(); + final RevCommit a2 = commit(a1); + final RevCommit a3 = commit(a2); + final RevCommit b = commit(a1); + final RevCommit a4 = commit(a3, b); + + rw.sort(RevSort.TOPO); + markStart(a4); + markUninteresting(a2); + assertCommit(a4, rw.next()); + assertCommit(b, rw.next()); + assertCommit(a3, rw.next()); + assertNull(rw.next()); + } } diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/revwalk/RevObject.java b/org.eclipse.jgit/src/org/eclipse/jgit/revwalk/RevObject.java index 5ce4bc33b..1abcf6962 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/revwalk/RevObject.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/revwalk/RevObject.java @@ -151,7 +151,7 @@ public abstract class RevObject extends ObjectIdOwnerMap.Entry { * buffer to append a debug description of core RevFlags onto. */ protected void appendCoreFlags(StringBuilder s) { - s.append((flags & RevWalk.TOPO_DELAY) != 0 ? 'o' : '-'); + s.append((flags & RevWalk.TOPO_QUEUED) != 0 ? 'o' : '-'); s.append((flags & RevWalk.TEMP_MARK) != 0 ? 't' : '-'); s.append((flags & RevWalk.REWRITE) != 0 ? 'r' : '-'); s.append((flags & RevWalk.UNINTERESTING) != 0 ? 'u' : '-'); 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 f425e8761..383428c85 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/revwalk/RevWalk.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/revwalk/RevWalk.java @@ -125,11 +125,11 @@ public class RevWalk implements Iterable, AutoCloseable { /** * Temporary mark for use within {@link TopoSortGenerator}. *

- * This mark indicates the commit could not produce when it wanted to, as at - * least one child was behind it. Commits with this flag are delayed until - * all children have been output first. + * This mark indicates the commit has been queued for emission in + * {@link TopoSortGenerator} and can be produced. This mark is removed when + * the commit has been produced. */ - static final int TOPO_DELAY = 1 << 5; + static final int TOPO_QUEUED = 1 << 5; /** Number of flag bits we keep internal for our own use. See above flags. */ static final int RESERVED_FLAGS = 6; diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/revwalk/TopoSortGenerator.java b/org.eclipse.jgit/src/org/eclipse/jgit/revwalk/TopoSortGenerator.java index 7a5db43a7..3c553b06c 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/revwalk/TopoSortGenerator.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/revwalk/TopoSortGenerator.java @@ -17,7 +17,7 @@ import org.eclipse.jgit.errors.MissingObjectException; /** Sorts commits in topological order. */ class TopoSortGenerator extends Generator { - private static final int TOPO_DELAY = RevWalk.TOPO_DELAY; + private static final int TOPO_QUEUED = RevWalk.TOPO_QUEUED; private final FIFORevQueue pending; @@ -47,12 +47,16 @@ class TopoSortGenerator extends Generator { if (c == null) { break; } - for (RevCommit p : c.parents) { - p.inDegree++; - if (firstParent) { - break; + if ((c.flags & TOPO_QUEUED) == 0) { + for (RevCommit p : c.parents) { + p.inDegree++; + + if (firstParent) { + break; + } } } + c.flags |= TOPO_QUEUED; pending.add(c); } } @@ -71,34 +75,42 @@ class TopoSortGenerator extends Generator { RevCommit next() throws MissingObjectException, IncorrectObjectTypeException, IOException { for (;;) { - final RevCommit c = pending.next(); - if (c == null) + RevCommit c = pending.next(); + if (c == null) { return null; + } if (c.inDegree > 0) { // At least one of our children is missing. We delay // production until all of our children are output. // - c.flags |= TOPO_DELAY; continue; } - // All of our children have already produced, - // so it is OK for us to produce now as well. - // + if ((c.flags & TOPO_QUEUED) == 0) { + // c is a parent that already produced or a parent that + // was never in the priority queue and should never produce. + // + continue; + } + for (RevCommit p : c.parents) { - if (--p.inDegree == 0 && (p.flags & TOPO_DELAY) != 0) { - // This parent tried to come before us, but we are - // his last child. unpop the parent so it goes right - // behind this child. + if (--p.inDegree == 0 && (p.flags & TOPO_QUEUED) != 0) { + // The parent has no unproduced interesting children. unpop + // the parent so it goes right behind this child. This means + // that this parent commit may appear in "pending" more than + // once, but this is safe since upon the second and + // subsequent iterations with this commit, it will no longer + // have TOPO_QUEUED set, and thus will be skipped. // - p.flags &= ~TOPO_DELAY; pending.unpop(p); } if (firstParent) { break; } } + + c.flags &= ~TOPO_QUEUED; return c; } }