[JGIT PATCH 5/5] Avoid incorrect output of UNINTERESTING commits when clock skew occurs

["Shawn O. Pearce" <spearce@spearce.org>]

The prior commit added functionality to scan a few extra commits when
we otherwise would have aborted due to everything left being colored
UNINTERESTING.  When that happens we may wind up coloring a commit
that we already produced to the caller, giving the caller an incorrect
result set.

If we insert a fully buffered generator in the pipeline, such as that
used for RevSort.TOPO or RevSort.REVERSE, we can easily filter these
late-colored commits out before we show them to the application.  But
otherwise we delay the output by 6 commits, just long enough to give
PendingGenerator a reasonable chance at getting the coloring right.

This is only an approximation.  It is still possible for commits to
produce when they should be UNINTERESTING, such as if more than the
OVER_SCAN limit had clock skew between two branches and the common
merge base, even if we are fully buffering our output.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 .../org/spearce/jgit/revwalk/DelayRevQueue.java    |   92 ++++++++++++++++++++
 .../jgit/revwalk/FixUninterestingGenerator.java    |   77 ++++++++++++++++
 .../org/spearce/jgit/revwalk/PendingGenerator.java |    2 +-
 .../org/spearce/jgit/revwalk/StartGenerator.java   |   23 ++++-
 4 files changed, 189 insertions(+), 5 deletions(-)
 create mode 100644 org.spearce.jgit/src/org/spearce/jgit/revwalk/DelayRevQueue.java
 create mode 100644 org.spearce.jgit/src/org/spearce/jgit/revwalk/FixUninterestingGenerator.java

diff --git a/org.spearce.jgit/src/org/spearce/jgit/revwalk/DelayRevQueue.java b/org.spearce.jgit/src/org/spearce/jgit/revwalk/DelayRevQueue.java
new file mode 100644
index 0000000..1eb7064
--- /dev/null
+++ b/org.spearce.jgit/src/org/spearce/jgit/revwalk/DelayRevQueue.java
@@ -0,0 +1,92 @@
+/*
+ * Copyright (C) 2009, Google Inc.
+ *
+ * All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or
+ * without modification, are permitted provided that the following
+ * conditions are met:
+ *
+ * - Redistributions of source code must retain the above copyright
+ *   notice, this list of conditions and the following disclaimer.
+ *
+ * - Redistributions in binary form must reproduce the above
+ *   copyright notice, this list of conditions and the following
+ *   disclaimer in the documentation and/or other materials provided
+ *   with the distribution.
+ *
+ * - Neither the name of the Git Development Community nor the
+ *   names of its contributors may be used to endorse or promote
+ *   products derived from this software without specific prior
+ *   written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND
+ * CONTRIBUTORS "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES,
+ * INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES
+ * OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
+ * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR
+ * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT
+ * NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
+ * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER
+ * CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT,
+ * STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
+ * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF
+ * ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+
+package org.spearce.jgit.revwalk;
+
+import java.io.IOException;
+
+import org.spearce.jgit.errors.IncorrectObjectTypeException;
+import org.spearce.jgit.errors.MissingObjectException;
+
+/**
+ * Delays commits to be at least {@link PendingGenerator#OVER_SCAN} late.
+ * <p>
+ * This helps to "fix up" weird corner cases resulting from clock skew, by
+ * slowing down what we produce to the caller we get a better chance to ensure
+ * PendingGenerator reached back far enough in the graph to correctly mark
+ * commits {@link RevWalk#UNINTERESTING} if necessary.
+ * <p>
+ * This generator should appear before {@link FixUninterestingGenerator} if the
+ * lower level {@link #pending} isn't already fully buffered.
+ */
+final class DelayRevQueue extends Generator {
+	private static final int OVER_SCAN = PendingGenerator.OVER_SCAN;
+
+	private final Generator pending;
+
+	private final FIFORevQueue delay;
+
+	private int size;
+
+	DelayRevQueue(final Generator g) {
+		pending = g;
+		delay = new FIFORevQueue();
+	}
+
+	@Override
+	int outputType() {
+		return pending.outputType();
+	}
+
+	@Override
+	RevCommit next() throws MissingObjectException,
+			IncorrectObjectTypeException, IOException {
+		while (size < OVER_SCAN) {
+			final RevCommit c = pending.next();
+			if (c == null)
+				break;
+			delay.add(c);
+			size++;
+		}
+
+		final RevCommit c = delay.next();
+		if (c == null)
+			return null;
+		size--;
+		return c;
+	}
+}
diff --git a/org.spearce.jgit/src/org/spearce/jgit/revwalk/FixUninterestingGenerator.java b/org.spearce.jgit/src/org/spearce/jgit/revwalk/FixUninterestingGenerator.java
new file mode 100644
index 0000000..6a9ba61
--- /dev/null
+++ b/org.spearce.jgit/src/org/spearce/jgit/revwalk/FixUninterestingGenerator.java
@@ -0,0 +1,77 @@
+/*
+ * Copyright (C) 2009, Google Inc.
+ *
+ * All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or
+ * without modification, are permitted provided that the following
+ * conditions are met:
+ *
+ * - Redistributions of source code must retain the above copyright
+ *   notice, this list of conditions and the following disclaimer.
+ *
+ * - Redistributions in binary form must reproduce the above
+ *   copyright notice, this list of conditions and the following
+ *   disclaimer in the documentation and/or other materials provided
+ *   with the distribution.
+ *
+ * - Neither the name of the Git Development Community nor the
+ *   names of its contributors may be used to endorse or promote
+ *   products derived from this software without specific prior
+ *   written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND
+ * CONTRIBUTORS "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES,
+ * INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES
+ * OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
+ * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR
+ * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT
+ * NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
+ * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER
+ * CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT,
+ * STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
+ * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF
+ * ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+
+package org.spearce.jgit.revwalk;
+
+import java.io.IOException;
+
+import org.spearce.jgit.errors.IncorrectObjectTypeException;
+import org.spearce.jgit.errors.MissingObjectException;
+
+/**
+ * Filters out commits marked {@link RevWalk#UNINTERESTING}.
+ * <p>
+ * This generator is only in front of another generator that has fully buffered
+ * commits, such that we are called only after the {@link PendingGenerator} has
+ * exhausted its input queue and given up. It skips over any uninteresting
+ * commits that may have leaked out of the PendingGenerator due to clock skew
+ * being detected in the commit objects.
+ */
+final class FixUninterestingGenerator extends Generator {
+	private final Generator pending;
+
+	FixUninterestingGenerator(final Generator g) {
+		pending = g;
+	}
+
+	@Override
+	int outputType() {
+		return pending.outputType();
+	}
+
+	@Override
+	RevCommit next() throws MissingObjectException,
+			IncorrectObjectTypeException, IOException {
+		for (;;) {
+			final RevCommit c = pending.next();
+			if (c == null)
+				return null;
+			if ((c.flags & RevWalk.UNINTERESTING) == 0)
+				return c;
+		}
+	}
+}
diff --git a/org.spearce.jgit/src/org/spearce/jgit/revwalk/PendingGenerator.java b/org.spearce.jgit/src/org/spearce/jgit/revwalk/PendingGenerator.java
index cd24e8f..ebbb39f 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/revwalk/PendingGenerator.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/revwalk/PendingGenerator.java
@@ -69,7 +69,7 @@
 	 * constant to 1 additional commit due to the use of a pre-increment
 	 * operator when accessing the value.
 	 */
-	private static final int OVER_SCAN = 5 + 1;
+	static final int OVER_SCAN = 5 + 1;
 
 	/** A commit near the end of time, to initialize {@link #last} with. */
 	private static final RevCommit INIT_LAST;
diff --git a/org.spearce.jgit/src/org/spearce/jgit/revwalk/StartGenerator.java b/org.spearce.jgit/src/org/spearce/jgit/revwalk/StartGenerator.java
index bf7067a..3535250 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/revwalk/StartGenerator.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/revwalk/StartGenerator.java
@@ -90,6 +90,7 @@ RevCommit next() throws MissingObjectException,
 			return mbg.next();
 		}
 
+		final boolean uninteresting = q.anybodyHasFlag(RevWalk.UNINTERESTING);
 		boolean boundary = walker.hasRevSort(RevSort.BOUNDARY);
 
 		if (!boundary && walker instanceof ObjectWalk) {
@@ -99,7 +100,7 @@ RevCommit next() throws MissingObjectException,
 			//
 			boundary = true;
 		}
-		if (boundary && !q.anybodyHasFlag(RevWalk.UNINTERESTING)) {
+		if (boundary && !uninteresting) {
 			// If we were not fed uninteresting commits we will never
 			// construct a boundary. There is no reason to include the
 			// extra overhead associated with that in our pipeline.
@@ -107,16 +108,19 @@ RevCommit next() throws MissingObjectException,
 			boundary = false;
 		}
 
+		final DateRevQueue pending;
 		int pendingOutputType = 0;
-		if (!(q instanceof DateRevQueue))
-			q = new DateRevQueue(q);
+		if (q instanceof DateRevQueue)
+			pending = (DateRevQueue)q;
+		else
+			pending = new DateRevQueue(q);
 		if (tf != TreeFilter.ALL) {
 			rf = AndRevFilter.create(rf, new RewriteTreeFilter(w, tf));
 			pendingOutputType |= HAS_REWRITE | NEEDS_REWRITE;
 		}
 
 		walker.queue = q;
-		g = new PendingGenerator(w, (DateRevQueue) q, rf, pendingOutputType);
+		g = new PendingGenerator(w, pending, rf, pendingOutputType);
 
 		if (boundary) {
 			// Because the boundary generator may produce uninteresting
@@ -143,6 +147,17 @@ RevCommit next() throws MissingObjectException,
 			g = new LIFORevQueue(g);
 		if (boundary)
 			g = new BoundaryGenerator(w, g);
+		else if (uninteresting) {
+			// Try to protect ourselves from uninteresting commits producing
+			// due to clock skew in the commit time stamps. Delay such that
+			// we have a chance at coloring enough of the graph correctly,
+			// and then strip any UNINTERESTING nodes that may have leaked
+			// through early.
+			//
+			if (pending.peek() != null)
+				g = new DelayRevQueue(g);
+			g = new FixUninterestingGenerator(g);
+		}
 
 		w.pending = g;
 		return g.next();
-- 
1.6.2.288.gc3f22