From db54736e714e3c7e8e54b38bbf9f1c2d0026d15d Mon Sep 17 00:00:00 2001 From: "Shawn O. Pearce" Date: Tue, 2 Feb 2010 12:40:04 -0800 Subject: [PATCH] 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 --- .../eclipse/jgit/revwalk/ObjectWalkTest.java | 44 +++++++++++++++++++ .../org/eclipse/jgit/revwalk/ObjectWalk.java | 41 +++++++++-------- 2 files changed, 67 insertions(+), 18 deletions(-) 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 582406c01..0ff4d512e 100644 --- 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 b3acf518c..ddf40ac10 100644 --- 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) {