Fix ObjectWalk corruption when skipping over empty trees

The supplied test case comes out of the example tree identified by
Robert de Wilde and Ilari on #git:

  $ git ls-tree -rt a54f1a85ebf6a7f53aa60a45a1be33f8b078fb7e
  040000 tree bfe058ad536cdb12e127cde63b01472c960ea105    A
  040000 tree 4b825dc642cb6eb9a060e54bf8d69288fbee4904    A/A
  040000 tree 4b825dc642cb6eb9a060e54bf8d69288fbee4904    A/B
  100644 blob abbbfafe3129f85747aba7bfac992af77134c607    B

In this tree, "B" was being skipped because "A/A" as an empty tree
was immediately followed by "A/B", also an empty tree, but the
ObjectWalk broke out too early and never visited "B".

Bug: 286653
Change-Id: I25bcb0bc99d0cbbbdd9c2bd625ad6a691a6d0335
Signed-off-by: Shawn O. Pearce <spearce@spearce.org>

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

@@ -43,6 +43,11 @@
 
 package org.eclipse.jgit.revwalk;
 
+import org.eclipse.jgit.lib.FileTreeEntry;
+import org.eclipse.jgit.lib.ObjectId;
+import org.eclipse.jgit.lib.ObjectWriter;
+import org.eclipse.jgit.lib.Tree;
+
 public class ObjectWalkTest extends RevWalkTestCase {
 	protected ObjectWalk objw;
 
@@ -194,4 +199,43 @@ public void testCull() throws Exception {
 		assertSame(f2, objw.nextObject());
 		assertNull(objw.nextObject());
 	}
+
+	public void testEmptyTreeCorruption() throws Exception {
+		ObjectId bId = ObjectId
+				.fromString("abbbfafe3129f85747aba7bfac992af77134c607");
+		final RevTree tree_root, tree_A, tree_AB;
+		final RevCommit b;
+		{
+			Tree root = new Tree(db);
+			Tree A = root.addTree("A");
+			FileTreeEntry B = root.addFile("B");
+			B.setId(bId);
+
+			Tree A_A = A.addTree("A");
+			Tree A_B = A.addTree("B");
+
+			final ObjectWriter ow = new ObjectWriter(db);
+			A_A.setId(ow.writeTree(A_A));
+			A_B.setId(ow.writeTree(A_B));
+			A.setId(ow.writeTree(A));
+			root.setId(ow.writeTree(root));
+
+			tree_root = rw.parseTree(root.getId());
+			tree_A = rw.parseTree(A.getId());
+			tree_AB = rw.parseTree(A_A.getId());
+			assertSame(tree_AB, rw.parseTree(A_B.getId()));
+			b = commit(rw.parseTree(root.getId()));
+		}
+
+		markStart(b);
+
+		assertCommit(b, objw.next());
+		assertNull(objw.next());
+
+		assertSame(tree_root, objw.nextObject());
+		assertSame(tree_A, objw.nextObject());
+		assertSame(tree_AB, objw.nextObject());
+		assertSame(rw.lookupBlob(bId), objw.nextObject());
+		assertNull(objw.nextObject());
+	}
 }

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

@@ -86,9 +86,7 @@ public class ObjectWalk extends RevWalk {
 
 	private RevTree currentTree;
 
-	private boolean fromTreeWalk;
-
-	private RevTree nextSubtree;
+	private RevObject last;
 
 	/**
 	 * Create a new revision and object walker for a given repository.
@@ -243,18 +241,12 @@ public RevCommit next() throws MissingObjectException,
 	 */
 	public RevObject nextObject() throws MissingObjectException,
 			IncorrectObjectTypeException, IOException {
-		fromTreeWalk = false;
-
-		if (nextSubtree != null) {
-			treeWalk = treeWalk.createSubtreeIterator0(db, nextSubtree, curs);
-			nextSubtree = null;
-		}
+		if (last != null)
+			treeWalk = last instanceof RevTree ? enter(last) : treeWalk.next();
 
 		while (!treeWalk.eof()) {
 			final FileMode mode = treeWalk.getEntryFileMode();
-			final int sType = mode.getObjectType();
-
-			switch (sType) {
+			switch (mode.getObjectType()) {
 			case Constants.OBJ_BLOB: {
 				treeWalk.getEntryObjectId(idBuffer);
 				final RevBlob o = lookupBlob(idBuffer);
@@ -263,7 +255,7 @@ public RevObject nextObject() throws MissingObjectException,
 				o.flags |= SEEN;
 				if (shouldSkipObject(o))
 					break;
-				fromTreeWalk = true;
+				last = o;
 				return o;
 			}
 			case Constants.OBJ_TREE: {
@@ -274,8 +266,7 @@ public RevObject nextObject() throws MissingObjectException,
 				o.flags |= SEEN;
 				if (shouldSkipObject(o))
 					break;
-				nextSubtree = o;
-				fromTreeWalk = true;
+				last = o;
 				return o;
 			}
 			default:
@@ -291,6 +282,7 @@ public RevObject nextObject() throws MissingObjectException,
 			treeWalk = treeWalk.next();
 		}
 
+		last = null;
 		for (;;) {
 			final RevObject o = pendingObjects.next();
 			if (o == null)
@@ -308,6 +300,18 @@ public RevObject nextObject() throws MissingObjectException,
 		}
 	}
 
+	private CanonicalTreeParser enter(RevObject tree) throws IOException {
+		CanonicalTreeParser p = treeWalk.createSubtreeIterator0(db, tree, curs);
+		if (p.eof()) {
+			// We can't tolerate the subtree being an empty tree, as
+			// that will break us out early before we visit all names.
+			// If it is, advance to the parent's next record.
+			//
+			return treeWalk.next();
+		}
+		return p;
+	}
+
 	private final boolean shouldSkipObject(final RevObject o) {
 		return (o.flags & UNINTERESTING) != 0 && !hasRevSort(RevSort.BOUNDARY);
 	}
@@ -364,7 +368,7 @@ public void checkConnectivity() throws MissingObjectException,
 	 *         has no path, such as for annotated tags or root level trees.
 	 */
 	public String getPathString() {
-		return fromTreeWalk ? treeWalk.getEntryPathString() : null;
+		return last != null ? treeWalk.getEntryPathString() : null;
 	}
 
 	@Override
@@ -372,8 +376,8 @@ public void dispose() {
 		super.dispose();
 		pendingObjects = new BlockObjQueue();
 		treeWalk = new CanonicalTreeParser();
-		nextSubtree = null;
 		currentTree = null;
+		last = null;
 	}
 
 	@Override
@@ -381,7 +385,8 @@ protected void reset(final int retainFlags) {
 		super.reset(retainFlags);
 		pendingObjects = new BlockObjQueue();
 		treeWalk = new CanonicalTreeParser();
-		nextSubtree = null;
+		currentTree = null;
+		last = null;
 	}
 
 	private void addObject(final RevObject o) {