* [JGIT PATCH 1/3] Add lookupBlob to RevWalk @ 2009-03-13 18:11 Shawn O. Pearce 2009-03-13 18:11 ` [JGIT PATCH 2/3] Fix ObjectWalk to handle single-entry subtrees correctly Shawn O. Pearce 0 siblings, 1 reply; 6+ messages in thread From: Shawn O. Pearce @ 2009-03-13 18:11 UTC (permalink / raw) To: Robin Rosenberg; +Cc: git Its useful if you want to get a handle on a blob object. Signed-off-by: Shawn O. Pearce <spearce@spearce.org> --- .../src/org/spearce/jgit/revwalk/RevWalk.java | 19 +++++++++++++++++++ 1 files changed, 19 insertions(+), 0 deletions(-) diff --git a/org.spearce.jgit/src/org/spearce/jgit/revwalk/RevWalk.java b/org.spearce.jgit/src/org/spearce/jgit/revwalk/RevWalk.java index 316f722..e47f9d9 100644 --- a/org.spearce.jgit/src/org/spearce/jgit/revwalk/RevWalk.java +++ b/org.spearce.jgit/src/org/spearce/jgit/revwalk/RevWalk.java @@ -510,6 +510,25 @@ public void setTreeFilter(final TreeFilter newFilter) { } /** + * Locate a reference to a blob without loading it. + * <p> + * The blob may or may not exist in the repository. It is impossible to tell + * from this method's return value. + * + * @param id + * name of the blob object. + * @return reference to the blob object. Never null. + */ + public RevBlob lookupBlob(final AnyObjectId id) { + RevBlob c = (RevBlob) objects.get(id); + if (c == null) { + c = new RevBlob(id); + objects.add(c); + } + return c; + } + + /** * Locate a reference to a tree without loading it. * <p> * The tree may or may not exist in the repository. It is impossible to tell -- 1.6.2.288.gc3f22 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [JGIT PATCH 2/3] Fix ObjectWalk to handle single-entry subtrees correctly 2009-03-13 18:11 [JGIT PATCH 1/3] Add lookupBlob to RevWalk Shawn O. Pearce @ 2009-03-13 18:11 ` Shawn O. Pearce 2009-03-13 18:11 ` [JGIT PATCH 3/3] Use a common skipObject method to avoid UNINTERESTING items Shawn O. Pearce 2009-03-13 18:37 ` [JGIT PATCH 2/3] Fix ObjectWalk to handle single-entry subtrees correctly Shawn O. Pearce 0 siblings, 2 replies; 6+ messages in thread From: Shawn O. Pearce @ 2009-03-13 18:11 UTC (permalink / raw) To: Robin Rosenberg; +Cc: git "jgit push" has been pushing more objects than necessary for a while now, due to ObjectWalk failing to mark a subtree uninteresting if it contains exactly one subtree child, such as "org.spearce.egit.core/src" which has only one child, "org". The bug was caused by the child iterator being advanced past the first item before we even evaluated it, as the for loop always invoked treeWalk.next() at the end of each iteration. However, a new subtree iterator is positioned on the first item and must not call next() until after we have handled that first entry. While debugging this I also noticed strange behavior from the ObjectWalk evaulating a subtree again, after we had exited it. This was because the parent iterator was never advanced past the child when we entered into it. We now advance the parent whenever we leave the child. Note that its important we do the parent advance *after* we exit the child, as they share the same path buffer. Advancing the parent before we enter into the child would cause the child to corrupt the parent's next item path. Signed-off-by: Shawn O. Pearce <spearce@spearce.org> --- .../src/org/spearce/jgit/revwalk/ObjectWalk.java | 54 +++++++++++--------- .../spearce/jgit/treewalk/CanonicalTreeParser.java | 40 +++++++++++++- 2 files changed, 66 insertions(+), 28 deletions(-) diff --git a/org.spearce.jgit/src/org/spearce/jgit/revwalk/ObjectWalk.java b/org.spearce.jgit/src/org/spearce/jgit/revwalk/ObjectWalk.java index 0d67a38..a481639 100644 --- a/org.spearce.jgit/src/org/spearce/jgit/revwalk/ObjectWalk.java +++ b/org.spearce.jgit/src/org/spearce/jgit/revwalk/ObjectWalk.java @@ -82,7 +82,7 @@ private boolean fromTreeWalk; - private boolean enterSubtree; + private RevTree nextSubtree; /** * Create a new revision and object walker for a given repository. @@ -239,50 +239,52 @@ public RevObject nextObject() throws MissingObjectException, IncorrectObjectTypeException, IOException { fromTreeWalk = false; - if (enterSubtree) { - treeWalk = treeWalk.createSubtreeIterator(db, idBuffer, curs); - enterSubtree = false; + if (nextSubtree != null) { + treeWalk = treeWalk.createSubtreeIterator0(db, nextSubtree, curs); + nextSubtree = null; } - for (; !treeWalk.eof(); treeWalk = treeWalk.next()) { + while (!treeWalk.eof()) { final FileMode mode = treeWalk.getEntryFileMode(); final int sType = mode.getObjectType(); switch (sType) { case Constants.OBJ_BLOB: { treeWalk.getEntryObjectId(idBuffer); - final RevObject o = lookupAny(idBuffer, sType); + final RevBlob o = lookupBlob(idBuffer); if ((o.flags & SEEN) != 0) - continue; + break; o.flags |= SEEN; if ((o.flags & UNINTERESTING) != 0 && !hasRevSort(RevSort.BOUNDARY)) - continue; + break; fromTreeWalk = true; return o; } case Constants.OBJ_TREE: { treeWalk.getEntryObjectId(idBuffer); - final RevObject o = lookupAny(idBuffer, sType); + final RevTree o = lookupTree(idBuffer); if ((o.flags & SEEN) != 0) - continue; + break; o.flags |= SEEN; if ((o.flags & UNINTERESTING) != 0 && !hasRevSort(RevSort.BOUNDARY)) - continue; - enterSubtree = true; + break; + nextSubtree = o; fromTreeWalk = true; return o; } default: if (FileMode.GITLINK.equals(mode)) - continue; + break; treeWalk.getEntryObjectId(idBuffer); throw new CorruptObjectException("Invalid mode " + mode + " for " + idBuffer.name() + " " + treeWalk.getEntryPathString() + " in " + currentTree + "."); } + + treeWalk = treeWalk.next(); } for (;;) { @@ -361,7 +363,7 @@ public String getPathString() { public void dispose() { super.dispose(); pendingObjects = new BlockObjQueue(); - enterSubtree = false; + nextSubtree = null; currentTree = null; } @@ -369,7 +371,7 @@ public void dispose() { protected void reset(final int retainFlags) { super.reset(retainFlags); pendingObjects = new BlockObjQueue(); - enterSubtree = false; + nextSubtree = null; } private void addObject(final RevObject o) { @@ -387,34 +389,36 @@ private void markTreeUninteresting(final RevTree tree) tree.flags |= UNINTERESTING; treeWalk = treeWalk.resetRoot(db, tree, curs); - for (;!treeWalk.eof(); treeWalk = treeWalk.next()) { + while (!treeWalk.eof()) { final FileMode mode = treeWalk.getEntryFileMode(); final int sType = mode.getObjectType(); switch (sType) { case Constants.OBJ_BLOB: { treeWalk.getEntryObjectId(idBuffer); - lookupAny(idBuffer, sType).flags |= UNINTERESTING; - continue; + lookupBlob(idBuffer).flags |= UNINTERESTING; + break; } case Constants.OBJ_TREE: { treeWalk.getEntryObjectId(idBuffer); - final RevObject subtree = lookupAny(idBuffer, sType); - if ((subtree.flags & UNINTERESTING) == 0) { - subtree.flags |= UNINTERESTING; - treeWalk = treeWalk.createSubtreeIterator(db, idBuffer, - curs); + final RevTree t = lookupTree(idBuffer); + if ((t.flags & UNINTERESTING) == 0) { + t.flags |= UNINTERESTING; + treeWalk = treeWalk.createSubtreeIterator0(db, t, curs); + continue; } - continue; + break; } default: if (FileMode.GITLINK.equals(mode)) - continue; + break; treeWalk.getEntryObjectId(idBuffer); throw new CorruptObjectException("Invalid mode " + mode + " for " + idBuffer.name() + " " + treeWalk.getEntryPathString() + " in " + tree + "."); } + + treeWalk = treeWalk.next(); } } } diff --git a/org.spearce.jgit/src/org/spearce/jgit/treewalk/CanonicalTreeParser.java b/org.spearce.jgit/src/org/spearce/jgit/treewalk/CanonicalTreeParser.java index 8028b14..5c331ca 100644 --- a/org.spearce.jgit/src/org/spearce/jgit/treewalk/CanonicalTreeParser.java +++ b/org.spearce.jgit/src/org/spearce/jgit/treewalk/CanonicalTreeParser.java @@ -145,8 +145,19 @@ public CanonicalTreeParser resetRoot(final Repository repo, /** @return this iterator, or its parent, if the tree is at eof. */ public CanonicalTreeParser next() { - next(1); - return eof() && parent != null ? (CanonicalTreeParser) parent : this; + CanonicalTreeParser p = this; + for (;;) { + p.next(1); + if (p.eof() && parent != null) { + // Parent was left pointing at the entry for us; advance + // the parent to the next entry, possibly unwinding many + // levels up the tree. + // + p = (CanonicalTreeParser) p.parent; + continue; + } + return p; + } } /** @@ -192,8 +203,31 @@ public CanonicalTreeParser createSubtreeIterator(final Repository repo, final ObjectId me = idBuffer.toObjectId(); throw new IncorrectObjectTypeException(me, Constants.TYPE_TREE); } + return createSubtreeIterator0(repo, idBuffer, curs); + } + + /** + * Back door to quickly create a subtree iterator for any subtree. + * <p> + * Don't use this unless you are ObjectWalk. The method is meant to be + * called only once the current entry has been identified as a tree and its + * identity has been converted into an ObjectId. + * + * @param repo + * repository to load the tree data from. + * @param id + * ObjectId of the tree to open. + * @param curs + * window cursor to use during repository access. + * @return a new parser that walks over the current subtree. + * @throws IOException + * a loose object or pack file could not be read. + */ + public final CanonicalTreeParser createSubtreeIterator0( + final Repository repo, final AnyObjectId id, final WindowCursor curs) + throws IOException { final CanonicalTreeParser p = new CanonicalTreeParser(this); - p.reset(repo, idBuffer, curs); + p.reset(repo, id, curs); return p; } -- 1.6.2.288.gc3f22 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [JGIT PATCH 3/3] Use a common skipObject method to avoid UNINTERESTING items 2009-03-13 18:11 ` [JGIT PATCH 2/3] Fix ObjectWalk to handle single-entry subtrees correctly Shawn O. Pearce @ 2009-03-13 18:11 ` Shawn O. Pearce 2009-03-15 0:25 ` Robin Rosenberg 2009-03-13 18:37 ` [JGIT PATCH 2/3] Fix ObjectWalk to handle single-entry subtrees correctly Shawn O. Pearce 1 sibling, 1 reply; 6+ messages in thread From: Shawn O. Pearce @ 2009-03-13 18:11 UTC (permalink / raw) To: Robin Rosenberg; +Cc: git All cases are using the same logic to decide that we should skip this current object and not return it to the caller. A common implementation makes the code easier to follow, especially as it reduces the ugly line wrap involved in the loop body. Signed-off-by: Shawn O. Pearce <spearce@spearce.org> --- .../src/org/spearce/jgit/revwalk/ObjectWalk.java | 12 +++++++----- 1 files changed, 7 insertions(+), 5 deletions(-) diff --git a/org.spearce.jgit/src/org/spearce/jgit/revwalk/ObjectWalk.java b/org.spearce.jgit/src/org/spearce/jgit/revwalk/ObjectWalk.java index a481639..b92629e 100644 --- a/org.spearce.jgit/src/org/spearce/jgit/revwalk/ObjectWalk.java +++ b/org.spearce.jgit/src/org/spearce/jgit/revwalk/ObjectWalk.java @@ -255,8 +255,7 @@ public RevObject nextObject() throws MissingObjectException, if ((o.flags & SEEN) != 0) break; o.flags |= SEEN; - if ((o.flags & UNINTERESTING) != 0 - && !hasRevSort(RevSort.BOUNDARY)) + if (skipObject(o)) break; fromTreeWalk = true; return o; @@ -267,8 +266,7 @@ public RevObject nextObject() throws MissingObjectException, if ((o.flags & SEEN) != 0) break; o.flags |= SEEN; - if ((o.flags & UNINTERESTING) != 0 - && !hasRevSort(RevSort.BOUNDARY)) + if (skipObject(o)) break; nextSubtree = o; fromTreeWalk = true; @@ -294,7 +292,7 @@ public RevObject nextObject() throws MissingObjectException, if ((o.flags & SEEN) != 0) continue; o.flags |= SEEN; - if ((o.flags & UNINTERESTING) != 0 && !hasRevSort(RevSort.BOUNDARY)) + if (skipObject(o)) continue; if (o instanceof RevTree) { currentTree = (RevTree) o; @@ -304,6 +302,10 @@ public RevObject nextObject() throws MissingObjectException, } } + private final boolean skipObject(final RevObject o) { + return (o.flags & UNINTERESTING) != 0 && !hasRevSort(RevSort.BOUNDARY); + } + /** * Verify all interesting objects are available, and reachable. * <p> -- 1.6.2.288.gc3f22 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [JGIT PATCH 3/3] Use a common skipObject method to avoid UNINTERESTING items 2009-03-13 18:11 ` [JGIT PATCH 3/3] Use a common skipObject method to avoid UNINTERESTING items Shawn O. Pearce @ 2009-03-15 0:25 ` Robin Rosenberg 2009-03-16 14:13 ` Shawn O. Pearce 0 siblings, 1 reply; 6+ messages in thread From: Robin Rosenberg @ 2009-03-15 0:25 UTC (permalink / raw) To: Shawn O. Pearce; +Cc: git fredag 13 mars 2009 19:11:52 skrev "Shawn O. Pearce" <spearce@spearce.org>: > All cases are using the same logic to decide that we should skip > this current object and not return it to the caller. A common > implementation makes the code easier to follow, especially as it > reduces the ugly line wrap involved in the loop body. Java conventions dictate that this method should be called shouldSkipObject. It is a boolean method that actually does not itself skip any objects. I can amend that for you. -- robin ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [JGIT PATCH 3/3] Use a common skipObject method to avoid UNINTERESTING items 2009-03-15 0:25 ` Robin Rosenberg @ 2009-03-16 14:13 ` Shawn O. Pearce 0 siblings, 0 replies; 6+ messages in thread From: Shawn O. Pearce @ 2009-03-16 14:13 UTC (permalink / raw) To: Robin Rosenberg; +Cc: git Robin Rosenberg <robin.rosenberg.lists@dewire.com> wrote: > fredag 13 mars 2009 19:11:52 skrev "Shawn O. Pearce" <spearce@spearce.org>: > > All cases are using the same logic to decide that we should skip > > this current object and not return it to the caller. A common > > implementation makes the code easier to follow, especially as it > > reduces the ugly line wrap involved in the loop body. > > Java conventions dictate that this method should be called shouldSkipObject. It > is a boolean method that actually does not itself skip any objects. > > I can amend that for you. Yup, amend away. shouldSkipObject is a much better name. Thanks. -- Shawn. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [JGIT PATCH 2/3] Fix ObjectWalk to handle single-entry subtrees correctly 2009-03-13 18:11 ` [JGIT PATCH 2/3] Fix ObjectWalk to handle single-entry subtrees correctly Shawn O. Pearce 2009-03-13 18:11 ` [JGIT PATCH 3/3] Use a common skipObject method to avoid UNINTERESTING items Shawn O. Pearce @ 2009-03-13 18:37 ` Shawn O. Pearce 1 sibling, 0 replies; 6+ messages in thread From: Shawn O. Pearce @ 2009-03-13 18:37 UTC (permalink / raw) To: Robin Rosenberg; +Cc: git "Shawn O. Pearce" <spearce@spearce.org> wrote: > diff --git a/org.spearce.jgit/src/org/spearce/jgit/treewalk/CanonicalTreeParser.java b/org.spearce.jgit/src/org/spearce/jgit/treewalk/CanonicalTreeParser.java > index 8028b14..5c331ca 100644 > --- a/org.spearce.jgit/src/org/spearce/jgit/treewalk/CanonicalTreeParser.java > +++ b/org.spearce.jgit/src/org/spearce/jgit/treewalk/CanonicalTreeParser.java > @@ -145,8 +145,19 @@ public CanonicalTreeParser resetRoot(final Repository repo, > > /** @return this iterator, or its parent, if the tree is at eof. */ > public CanonicalTreeParser next() { > - next(1); > - return eof() && parent != null ? (CanonicalTreeParser) parent : this; > + CanonicalTreeParser p = this; > + for (;;) { > + p.next(1); > + if (p.eof() && parent != null) { > + // Parent was left pointing at the entry for us; advance > + // the parent to the next entry, possibly unwinding many > + // levels up the tree. > + // > + p = (CanonicalTreeParser) p.parent; > + continue; > + } > + return p; > + } The parent != null test is wrong, we need "p.parent" here: diff --git a/org.spearce.jgit/src/org/spearce/jgit/treewalk/CanonicalTreeParser.java b/org.spearce.jgit/src/org/spearce/jgit/treewalk/CanonicalTreeParser.java index 5c331ca..7f89cff 100644 --- a/org.spearce.jgit/src/org/spearce/jgit/treewalk/CanonicalTreeParser.java +++ b/org.spearce.jgit/src/org/spearce/jgit/treewalk/CanonicalTreeParser.java @@ -148,7 +148,7 @@ public CanonicalTreeParser next() { CanonicalTreeParser p = this; for (;;) { p.next(1); - if (p.eof() && parent != null) { + if (p.eof() && p.parent != null) { // Parent was left pointing at the entry for us; advance // the parent to the next entry, possibly unwinding many // levels up the tree. -- Shawn. ^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2009-03-16 14:16 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2009-03-13 18:11 [JGIT PATCH 1/3] Add lookupBlob to RevWalk Shawn O. Pearce 2009-03-13 18:11 ` [JGIT PATCH 2/3] Fix ObjectWalk to handle single-entry subtrees correctly Shawn O. Pearce 2009-03-13 18:11 ` [JGIT PATCH 3/3] Use a common skipObject method to avoid UNINTERESTING items Shawn O. Pearce 2009-03-15 0:25 ` Robin Rosenberg 2009-03-16 14:13 ` Shawn O. Pearce 2009-03-13 18:37 ` [JGIT PATCH 2/3] Fix ObjectWalk to handle single-entry subtrees correctly Shawn O. Pearce
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).