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>

diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/revwalk/ObjectWalkTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/revwalk/ObjectWalkTest.java
index 582406c01be57d890e5c23822023e2c406f01242..0ff4d512e82f7e4fea8992edca780e5ec84c7071 100644 (file)
--- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/revwalk/ObjectWalkTest.java
+++ b/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 class ObjectWalkTest extends RevWalkTestCase {
                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());
+       }
 }

diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/revwalk/ObjectWalk.java b/org.eclipse.jgit/src/org/eclipse/jgit/revwalk/ObjectWalk.java
index b3acf518cf239cdacd63d2e57656fd7f6003434d..ddf40ac104c2d04b34f2b37cffbf63238430d7d0 100644 (file)
--- a/org.eclipse.jgit/src/org/eclipse/jgit/revwalk/ObjectWalk.java
+++ b/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 class ObjectWalk extends RevWalk {
         */
        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 class ObjectWalk extends RevWalk {
                                o.flags |= SEEN;
                                if (shouldSkipObject(o))
                                        break;
-                               fromTreeWalk = true;
+                               last = o;
                                return o;
                        }
                        case Constants.OBJ_TREE: {
@@ -274,8 +266,7 @@ public class ObjectWalk extends RevWalk {
                                o.flags |= SEEN;
                                if (shouldSkipObject(o))
                                        break;
-                               nextSubtree = o;
-                               fromTreeWalk = true;
+                               last = o;
                                return o;
                        }
                        default:
@@ -291,6 +282,7 @@ public class ObjectWalk extends RevWalk {
                        treeWalk = treeWalk.next();
                }
 
+               last = null;
                for (;;) {
                        final RevObject o = pendingObjects.next();
                        if (o == null)
@@ -308,6 +300,18 @@ public class ObjectWalk extends RevWalk {
                }
        }
 
+       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 class ObjectWalk extends RevWalk {
         *         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 class ObjectWalk extends RevWalk {
                super.dispose();
                pendingObjects = new BlockObjQueue();
                treeWalk = new CanonicalTreeParser();
-               nextSubtree = null;
                currentTree = null;
+               last = null;
        }
 
        @Override
@@ -381,7 +385,8 @@ public class ObjectWalk extends RevWalk {
                super.reset(retainFlags);
                pendingObjects = new BlockObjQueue();
                treeWalk = new CanonicalTreeParser();
-               nextSubtree = null;
+               currentTree = null;
+               last = null;
        }
 
        private void addObject(final RevObject o) {