RevWalk: new topo sort to not mix lines of history

The topological sort algorithm in TopoSortGenerator for RevWalk may mix
multiple lines of history, producing results that differ from C git's
git-log whose man page states: "Show no parents before all of its
children are shown, and avoid showing commits on multiple lines of
history intermixed." Lines of history are mixed because
TopoSortGenerator merely delays producing a commit until all of its
children have been produced; it does not immediately produce a commit
after its last child has been produced.

Therefore, add a new RevSort option called TOPO_KEEP_BRANCH_TOGETHER
with a new topo sort algorithm in TopoNonIntermixGenerator. In the
Generator, when the last child of a commit has been produced, unpop
that commit so that it will be returned upon the subsequent call to
next(). To avoid producing duplicates, mark commits that have not yet
been produced as TOPO_QUEUED so that when a commit is popped, it is
produced if and only if TOPO_QUEUED is set.

To support nesting with other generators that may produce the same
commit multiple times like DepthGenerator (for example, StartGenerator
does this), do not increment parent inDegree for the same child commit
more than once.

Commit b5e764abd2 modified the existing
TopoSortGenerator to avoid mixing lines of history, but it was reverted
in e40c38ab08 because the new behavior
caused problems for EGit users. This motivated adding a new Generator
for the new behavior.

Signed-off-by: Alex Spradlin <alexaspradlin@google.com>
Change-Id: Icbb24eac98c00e45c175b01e1c8122554f617933

org.eclipse.jgit.test/tst/org/eclipse/jgit/revwalk/FirstParentRevWalkTest.java

@@ -163,6 +163,23 @@ public class FirstParentRevWalkTest extends RevWalkTestCase {
 		assertNull(rw.next());
 	}
 
+	@Test
+	public void testTopoNonIntermixSort() throws Exception {
+		RevCommit a = commit();
+		RevCommit b1 = commit(a);
+		RevCommit b2 = commit(a);
+		RevCommit c = commit(b1, b2);
+
+		rw.reset();
+		rw.sort(RevSort.TOPO_KEEP_BRANCH_TOGETHER);
+		rw.setFirstParent(true);
+		markStart(c);
+		assertCommit(c, rw.next());
+		assertCommit(b1, rw.next());
+		assertCommit(a, rw.next());
+		assertNull(rw.next());
+	}
+
 	@Test
 	public void testCommitTimeSort() throws Exception {
 		RevCommit a = commit();
@@ -428,4 +445,39 @@ public class FirstParentRevWalkTest extends RevWalkTestCase {
 		assertCommit(c, rw.next());
 		assertNull(rw.next());
 	}
+
+	@Test
+	public void testWithTopoNonIntermixSortAndTreeFilter() throws Exception {
+		RevCommit a = commit();
+		RevCommit b = commit(tree(file("0", blob("b"))), a);
+		RevCommit c = commit(tree(file("0", blob("c"))), b, a);
+		RevCommit d = commit(tree(file("0", blob("d"))), c);
+
+		rw.reset();
+		rw.setFirstParent(true);
+		rw.sort(RevSort.TOPO_KEEP_BRANCH_TOGETHER, true);
+		rw.setTreeFilter(PathFilterGroup.createFromStrings("0"));
+		markStart(d);
+		assertCommit(d, rw.next());
+		assertCommit(c, rw.next());
+		assertCommit(b, rw.next());
+		assertNull(rw.next());
+	}
+
+	@Test
+	public void testWithTopoNonIntermixSortAndTreeFilter2() throws Exception {
+		RevCommit a = commit();
+		RevCommit b = commit(tree(file("0", blob("b"))), a);
+		RevCommit c = commit(tree(file("0", blob("c"))), a, b);
+		RevCommit d = commit(tree(file("0", blob("d"))), c);
+
+		rw.reset();
+		rw.setFirstParent(true);
+		rw.sort(RevSort.TOPO_KEEP_BRANCH_TOGETHER, true);
+		rw.setTreeFilter(PathFilterGroup.createFromStrings("0"));
+		markStart(d);
+		assertCommit(d, rw.next());
+		assertCommit(c, rw.next());
+		assertNull(rw.next());
+	}
 }

org.eclipse.jgit.test/tst/org/eclipse/jgit/revwalk/RevWalkSortTest.java

@@ -10,9 +10,12 @@
 
 package org.eclipse.jgit.revwalk;
 
+import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
 
+import org.eclipse.jgit.internal.JGitText;
 import org.junit.Test;
 
 public class RevWalkSortTest extends RevWalkTestCase {
@@ -144,4 +147,171 @@ public class RevWalkSortTest extends RevWalkTestCase {
 		assertCommit(d, rw.next());
 		assertNull(rw.next());
 	}
+
+	@Test
+	public void testSort_TOPO_NON_INTERMIX() throws Exception {
+		// c1 is back dated before its parent.
+		//
+		final RevCommit a = commit();
+		final RevCommit b = commit(a);
+		final RevCommit c1 = commit(-5, b);
+		final RevCommit c2 = commit(10, b);
+		final RevCommit d = commit(c1, c2);
+
+		rw.sort(RevSort.TOPO_KEEP_BRANCH_TOGETHER);
+		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_NON_INTERMIX_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_KEEP_BRANCH_TOGETHER);
+		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_NON_INTERMIX_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_KEEP_BRANCH_TOGETHER);
+		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_NON_INTERMIX_REVERSE() throws Exception {
+		// c1 is back dated before its parent.
+		//
+		final RevCommit a = commit();
+		final RevCommit b = commit(a);
+		final RevCommit c1 = commit(-5, b);
+		final RevCommit c2 = commit(10, b);
+		final RevCommit d = commit(c1, c2);
+
+		rw.sort(RevSort.TOPO_KEEP_BRANCH_TOGETHER);
+		rw.sort(RevSort.REVERSE, true);
+		markStart(d);
+		assertCommit(a, rw.next());
+		assertCommit(b, rw.next());
+		assertCommit(c1, rw.next());
+		assertCommit(c2, rw.next());
+		assertCommit(d, rw.next());
+		assertNull(rw.next());
+	}
+
+	@Test
+	public void testSort_TOPO_NON_INTERMIX_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_KEEP_BRANCH_TOGETHER);
+		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_NON_INTERMIX_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_KEEP_BRANCH_TOGETHER);
+		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_NON_INTERMIX_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_KEEP_BRANCH_TOGETHER);
+		markStart(a4);
+		markUninteresting(a2);
+		assertCommit(a4, rw.next());
+		assertCommit(b, rw.next());
+		assertCommit(a3, rw.next());
+		assertNull(rw.next());
+	}
+
+	@Test
+	public void testSort_TOPO_NON_INTERMIX_and_TOPO_throws() throws Exception {
+		final RevCommit a = commit();
+
+		rw.sort(RevSort.TOPO_KEEP_BRANCH_TOGETHER);
+		rw.sort(RevSort.TOPO, true);
+		markStart(a);
+		try {
+			rw.next();
+			fail("did not throw IllegalStateException");
+		} catch (IllegalStateException e) {
+			assertEquals(
+					JGitText.get().cannotCombineTopoSortWithTopoNonIntermixSort,
+					e.getMessage());
+		}
+	}
 }

org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties

@@ -54,6 +54,7 @@ cannotChangeActionOnComment=Cannot change action on comment line in git-rebase-t
 cannotCheckoutFromUnbornBranch=Cannot check out from unborn branch
 cannotCheckoutOursSwitchBranch=Checking out ours/theirs is only possible when checking out index, not when switching branches.
 cannotCombineSquashWithNoff=Cannot combine --squash with --no-ff.
+cannotCombineTopoSortWithTopoNonIntermixSort=Cannot combine sorts TOPO and TOPO_NON_INTERMIX.
 cannotCombineTreeFilterWithRevFilter=Cannot combine TreeFilter {0} with RevFilter {1}.
 cannotCommitOnARepoWithState=Cannot commit on a repo with state: {0}
 cannotCommitWriteTo=Cannot commit write to {0}

org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java

@@ -82,6 +82,7 @@ public class JGitText extends TranslationBundle {
 	/***/ public String cannotCheckoutFromUnbornBranch;
 	/***/ public String cannotCheckoutOursSwitchBranch;
 	/***/ public String cannotCombineSquashWithNoff;
+	/***/ public String cannotCombineTopoSortWithTopoNonIntermixSort;
 	/***/ public String cannotCombineTreeFilterWithRevFilter;
 	/***/ public String cannotCommitOnARepoWithState;
 	/***/ public String cannotCommitWriteTo;

org.eclipse.jgit/src/org/eclipse/jgit/revwalk/RevObject.java

@@ -152,6 +152,7 @@ public abstract class RevObject extends ObjectIdOwnerMap.Entry {
 	 */
 	protected void appendCoreFlags(StringBuilder s) {
 		s.append((flags & RevWalk.TOPO_DELAY) != 0 ? 'o' : '-');
+		s.append((flags & RevWalk.TOPO_QUEUED) != 0 ? 'q' : '-');
 		s.append((flags & RevWalk.TEMP_MARK) != 0 ? 't' : '-');
 		s.append((flags & RevWalk.REWRITE) != 0 ? 'r' : '-');
 		s.append((flags & RevWalk.UNINTERESTING) != 0 ? 'u' : '-');

org.eclipse.jgit/src/org/eclipse/jgit/revwalk/RevSort.java

@@ -39,6 +39,18 @@ public enum RevSort {
 	 */
 	TOPO,
 
+	/**
+	 * Topological sorting (all children before parents) without intermixing
+	 * lines of history.
+	 * <p>
+	 * This behavior more closely resembles C Git's git-log --topo-order than
+	 * {@link #TOPO}. See C Git's topo-order <a href=
+	 * "https://git-scm.com/docs/git-log#Documentation/git-log.txt---topo-order">description</a>.
+	 *
+	 * @since 5.8
+	 */
+	TOPO_KEEP_BRANCH_TOGETHER,
+
 	/**
 	 * Flip the output into the reverse ordering.
 	 * <p>

org.eclipse.jgit/src/org/eclipse/jgit/revwalk/RevWalk.java

@@ -131,8 +131,17 @@ public class RevWalk implements Iterable<RevCommit>, AutoCloseable {
 	 */
 	static final int TOPO_DELAY = 1 << 5;
 
+	/**
+	 * Temporary mark for use within {@link TopoNonIntermixSortGenerator}.
+	 * <p>
+	 * 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_QUEUED = 1 << 6;
+
 	/** Number of flag bits we keep internal for our own use. See above flags. */
-	static final int RESERVED_FLAGS = 6;
+	static final int RESERVED_FLAGS = 7;
 
 	private static final int APP_FLAGS = -1 & ~((1 << RESERVED_FLAGS) - 1);
 

org.eclipse.jgit/src/org/eclipse/jgit/revwalk/StartGenerator.java

@@ -135,8 +135,18 @@ class StartGenerator extends Generator {
 		}
 
 		if (walker.hasRevSort(RevSort.TOPO)
-				&& (g.outputType() & SORT_TOPO) == 0)
+				&& walker.hasRevSort(RevSort.TOPO_KEEP_BRANCH_TOGETHER)) {
+			throw new IllegalStateException(JGitText
+					.get().cannotCombineTopoSortWithTopoNonIntermixSort);
+		}
+
+		if (walker.hasRevSort(RevSort.TOPO)
+				&& (g.outputType() & SORT_TOPO) == 0) {
 			g = new TopoSortGenerator(g);
+		} else if (walker.hasRevSort(RevSort.TOPO_KEEP_BRANCH_TOGETHER)
+				&& (g.outputType() & SORT_TOPO) == 0) {
+			g = new TopoNonIntermixSortGenerator(g);
+		}
 		if (walker.hasRevSort(RevSort.REVERSE))
 			g = new LIFORevQueue(g);
 		if (boundary)

org.eclipse.jgit/src/org/eclipse/jgit/revwalk/TopoNonIntermixSortGenerator.java

@@ -0,0 +1,117 @@
+/*
+ * Copyright (C) 2020, 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 java.io.IOException;
+
+import org.eclipse.jgit.errors.IncorrectObjectTypeException;
+import org.eclipse.jgit.errors.MissingObjectException;
+
+/** Sorts commits in topological order without intermixing lines of history. */
+class TopoNonIntermixSortGenerator extends Generator {
+	private static final int TOPO_QUEUED = RevWalk.TOPO_QUEUED;
+
+	private final FIFORevQueue pending;
+
+	private final int outputType;
+
+	/**
+	 * Create a new sorter and completely spin the generator.
+	 * <p>
+	 * When the constructor completes the supplied generator will have no
+	 * commits remaining, as all of the commits will be held inside of this
+	 * generator's internal buffer.
+	 *
+	 * @param s
+	 *            generator to pull all commits out of, and into this buffer.
+	 * @throws MissingObjectException
+	 * @throws IncorrectObjectTypeException
+	 * @throws IOException
+	 */
+	TopoNonIntermixSortGenerator(Generator s) throws MissingObjectException,
+			IncorrectObjectTypeException, IOException {
+		super(s.firstParent);
+		pending = new FIFORevQueue(firstParent);
+		outputType = s.outputType() | SORT_TOPO;
+		s.shareFreeList(pending);
+		for (;;) {
+			final RevCommit c = s.next();
+			if (c == null) {
+				break;
+			}
+			if ((c.flags & TOPO_QUEUED) == 0) {
+				for (RevCommit p : c.parents) {
+					p.inDegree++;
+
+					if (firstParent) {
+						break;
+					}
+				}
+			}
+			c.flags |= TOPO_QUEUED;
+			pending.add(c);
+		}
+	}
+
+	@Override
+	int outputType() {
+		return outputType;
+	}
+
+	@Override
+	void shareFreeList(BlockRevQueue q) {
+		q.shareFreeList(pending);
+	}
+
+	@Override
+	RevCommit next() throws MissingObjectException,
+			IncorrectObjectTypeException, IOException {
+		for (;;) {
+			final 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.
+				//
+				continue;
+			}
+
+			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_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.
+					//
+					pending.unpop(p);
+				}
+				if (firstParent) {
+					break;
+				}
+			}
+
+			c.flags &= ~TOPO_QUEUED;
+			return c;
+		}
+	}
+}