{"thread":{"id":"18306","subject":"[JGIT PATCH 1/3] Add lookupBlob to RevWalk","startedAt":"2009-03-13T18:11:50Z","lastAt":"2009-03-16T14:13:29Z","messageCount":6,"participants":["Shawn O. Pearce","Robin Rosenberg"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"107975","messageId":"1236967912-15088-1-git-send-email-spearce@spearce.org","threadId":"18306","inReplyTo":null,"subject":"[JGIT PATCH 1/3] Add lookupBlob to RevWalk","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2009-03-13T18:11:50Z","receivedAt":"2009-03-13T18:11:50Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Its useful if you want to get a handle on a blob object.\n\nSigned-off-by: Shawn O. Pearce <spearce@spearce.org>\n---\n .../src/org/spearce/jgit/revwalk/RevWalk.java      |   19 +++++++++++++++++++\n 1 files changed, 19 insertions(+), 0 deletions(-)\n\ndiff --git a/org.spearce.jgit/src/org/spearce/jgit/revwalk/RevWalk.java b/org.spearce.jgit/src/org/spearce/jgit/revwalk/RevWalk.java\nindex 316f722..e47f9d9 100644\n--- a/org.spearce.jgit/src/org/spearce/jgit/revwalk/RevWalk.java\n+++ b/org.spearce.jgit/src/org/spearce/jgit/revwalk/RevWalk.java\n@@ -510,6 +510,25 @@ public void setTreeFilter(final TreeFilter newFilter) {\n \t}\n \n \t/**\n+\t * Locate a reference to a blob without loading it.\n+\t * <p>\n+\t * The blob may or may not exist in the repository. It is impossible to tell\n+\t * from this method's return value.\n+\t * \n+\t * @param id\n+\t *            name of the blob object.\n+\t * @return reference to the blob object. Never null.\n+\t */\n+\tpublic RevBlob lookupBlob(final AnyObjectId id) {\n+\t\tRevBlob c = (RevBlob) objects.get(id);\n+\t\tif (c == null) {\n+\t\t\tc = new RevBlob(id);\n+\t\t\tobjects.add(c);\n+\t\t}\n+\t\treturn c;\n+\t}\n+\n+\t/**\n \t * Locate a reference to a tree without loading it.\n \t * <p>\n \t * The tree may or may not exist in the repository. It is impossible to tell\n-- \n1.6.2.288.gc3f22\n"},{"id":"107976","messageId":"1236967912-15088-2-git-send-email-spearce@spearce.org","threadId":"18306","inReplyTo":"1236967912-15088-1-git-send-email-spearce@spearce.org","subject":"[JGIT PATCH 2/3] Fix ObjectWalk to handle single-entry subtrees correctly","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2009-03-13T18:11:51Z","receivedAt":"2009-03-13T18:11:51Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"\"jgit push\" has been pushing more objects than necessary\nfor a while now, due to ObjectWalk failing to mark a subtree\nuninteresting if it contains exactly one subtree child, such as\n\"org.spearce.egit.core/src\" which has only one child, \"org\".\n\nThe bug was caused by the child iterator being advanced past the\nfirst item before we even evaluated it, as the for loop always\ninvoked treeWalk.next() at the end of each iteration.  However,\na new subtree iterator is positioned on the first item and must\nnot call next() until after we have handled that first entry.\n\nWhile debugging this I also noticed strange behavior from the\nObjectWalk evaulating a subtree again, after we had exited it.\nThis was because the parent iterator was never advanced past the\nchild when we entered into it.  We now advance the parent whenever\nwe leave the child.\n\nNote that its important we do the parent advance *after* we exit the\nchild, as they share the same path buffer.  Advancing the parent\nbefore we enter into the child would cause the child to corrupt\nthe parent's next item path.\n\nSigned-off-by: Shawn O. Pearce <spearce@spearce.org>\n---\n .../src/org/spearce/jgit/revwalk/ObjectWalk.java   |   54 +++++++++++---------\n .../spearce/jgit/treewalk/CanonicalTreeParser.java |   40 +++++++++++++-\n 2 files changed, 66 insertions(+), 28 deletions(-)\n\ndiff --git a/org.spearce.jgit/src/org/spearce/jgit/revwalk/ObjectWalk.java b/org.spearce.jgit/src/org/spearce/jgit/revwalk/ObjectWalk.java\nindex 0d67a38..a481639 100644\n--- a/org.spearce.jgit/src/org/spearce/jgit/revwalk/ObjectWalk.java\n+++ b/org.spearce.jgit/src/org/spearce/jgit/revwalk/ObjectWalk.java\n@@ -82,7 +82,7 @@\n \n \tprivate boolean fromTreeWalk;\n \n-\tprivate boolean enterSubtree;\n+\tprivate RevTree nextSubtree;\n \n \t/**\n \t * Create a new revision and object walker for a given repository.\n@@ -239,50 +239,52 @@ public RevObject nextObject() throws MissingObjectException,\n \t\t\tIncorrectObjectTypeException, IOException {\n \t\tfromTreeWalk = false;\n \n-\t\tif (enterSubtree) {\n-\t\t\ttreeWalk = treeWalk.createSubtreeIterator(db, idBuffer, curs);\n-\t\t\tenterSubtree = false;\n+\t\tif (nextSubtree != null) {\n+\t\t\ttreeWalk = treeWalk.createSubtreeIterator0(db, nextSubtree, curs);\n+\t\t\tnextSubtree = null;\n \t\t}\n \n-\t\tfor (; !treeWalk.eof(); treeWalk = treeWalk.next()) {\n+\t\twhile (!treeWalk.eof()) {\n \t\t\tfinal FileMode mode = treeWalk.getEntryFileMode();\n \t\t\tfinal int sType = mode.getObjectType();\n \n \t\t\tswitch (sType) {\n \t\t\tcase Constants.OBJ_BLOB: {\n \t\t\t\ttreeWalk.getEntryObjectId(idBuffer);\n-\t\t\t\tfinal RevObject o = lookupAny(idBuffer, sType);\n+\t\t\t\tfinal RevBlob o = lookupBlob(idBuffer);\n \t\t\t\tif ((o.flags & SEEN) != 0)\n-\t\t\t\t\tcontinue;\n+\t\t\t\t\tbreak;\n \t\t\t\to.flags |= SEEN;\n \t\t\t\tif ((o.flags & UNINTERESTING) != 0\n \t\t\t\t\t\t&& !hasRevSort(RevSort.BOUNDARY))\n-\t\t\t\t\tcontinue;\n+\t\t\t\t\tbreak;\n \t\t\t\tfromTreeWalk = true;\n \t\t\t\treturn o;\n \t\t\t}\n \t\t\tcase Constants.OBJ_TREE: {\n \t\t\t\ttreeWalk.getEntryObjectId(idBuffer);\n-\t\t\t\tfinal RevObject o = lookupAny(idBuffer, sType);\n+\t\t\t\tfinal RevTree o = lookupTree(idBuffer);\n \t\t\t\tif ((o.flags & SEEN) != 0)\n-\t\t\t\t\tcontinue;\n+\t\t\t\t\tbreak;\n \t\t\t\to.flags |= SEEN;\n \t\t\t\tif ((o.flags & UNINTERESTING) != 0\n \t\t\t\t\t\t&& !hasRevSort(RevSort.BOUNDARY))\n-\t\t\t\t\tcontinue;\n-\t\t\t\tenterSubtree = true;\n+\t\t\t\t\tbreak;\n+\t\t\t\tnextSubtree = o;\n \t\t\t\tfromTreeWalk = true;\n \t\t\t\treturn o;\n \t\t\t}\n \t\t\tdefault:\n \t\t\t\tif (FileMode.GITLINK.equals(mode))\n-\t\t\t\t\tcontinue;\n+\t\t\t\t\tbreak;\n \t\t\t\ttreeWalk.getEntryObjectId(idBuffer);\n \t\t\t\tthrow new CorruptObjectException(\"Invalid mode \" + mode\n \t\t\t\t\t\t+ \" for \" + idBuffer.name() + \" \"\n \t\t\t\t\t\t+ treeWalk.getEntryPathString() + \" in \" + currentTree\n \t\t\t\t\t\t+ \".\");\n \t\t\t}\n+\n+\t\t\ttreeWalk = treeWalk.next();\n \t\t}\n \n \t\tfor (;;) {\n@@ -361,7 +363,7 @@ public String getPathString() {\n \tpublic void dispose() {\n \t\tsuper.dispose();\n \t\tpendingObjects = new BlockObjQueue();\n-\t\tenterSubtree = false;\n+\t\tnextSubtree = null;\n \t\tcurrentTree = null;\n \t}\n \n@@ -369,7 +371,7 @@ public void dispose() {\n \tprotected void reset(final int retainFlags) {\n \t\tsuper.reset(retainFlags);\n \t\tpendingObjects = new BlockObjQueue();\n-\t\tenterSubtree = false;\n+\t\tnextSubtree = null;\n \t}\n \n \tprivate void addObject(final RevObject o) {\n@@ -387,34 +389,36 @@ private void markTreeUninteresting(final RevTree tree)\n \t\ttree.flags |= UNINTERESTING;\n \n \t\ttreeWalk = treeWalk.resetRoot(db, tree, curs);\n-\t\tfor (;!treeWalk.eof(); treeWalk = treeWalk.next()) {\n+\t\twhile (!treeWalk.eof()) {\n \t\t\tfinal FileMode mode = treeWalk.getEntryFileMode();\n \t\t\tfinal int sType = mode.getObjectType();\n \n \t\t\tswitch (sType) {\n \t\t\tcase Constants.OBJ_BLOB: {\n \t\t\t\ttreeWalk.getEntryObjectId(idBuffer);\n-\t\t\t\tlookupAny(idBuffer, sType).flags |= UNINTERESTING;\n-\t\t\t\tcontinue;\n+\t\t\t\tlookupBlob(idBuffer).flags |= UNINTERESTING;\n+\t\t\t\tbreak;\n \t\t\t}\n \t\t\tcase Constants.OBJ_TREE: {\n \t\t\t\ttreeWalk.getEntryObjectId(idBuffer);\n-\t\t\t\tfinal RevObject subtree = lookupAny(idBuffer, sType);\n-\t\t\t\tif ((subtree.flags & UNINTERESTING) == 0) {\n-\t\t\t\t\tsubtree.flags |= UNINTERESTING;\n-\t\t\t\t\ttreeWalk = treeWalk.createSubtreeIterator(db, idBuffer,\n-\t\t\t\t\t\t\tcurs);\n+\t\t\t\tfinal RevTree t = lookupTree(idBuffer);\n+\t\t\t\tif ((t.flags & UNINTERESTING) == 0) {\n+\t\t\t\t\tt.flags |= UNINTERESTING;\n+\t\t\t\t\ttreeWalk = treeWalk.createSubtreeIterator0(db, t, curs);\n+\t\t\t\t\tcontinue;\n \t\t\t\t}\n-\t\t\t\tcontinue;\n+\t\t\t\tbreak;\n \t\t\t}\n \t\t\tdefault:\n \t\t\t\tif (FileMode.GITLINK.equals(mode))\n-\t\t\t\t\tcontinue;\n+\t\t\t\t\tbreak;\n \t\t\t\ttreeWalk.getEntryObjectId(idBuffer);\n \t\t\t\tthrow new CorruptObjectException(\"Invalid mode \" + mode\n \t\t\t\t\t\t+ \" for \" + idBuffer.name() + \" \"\n \t\t\t\t\t\t+ treeWalk.getEntryPathString() + \" in \" + tree + \".\");\n \t\t\t}\n+\n+\t\t\ttreeWalk = treeWalk.next();\n \t\t}\n \t}\n }\ndiff --git a/org.spearce.jgit/src/org/spearce/jgit/treewalk/CanonicalTreeParser.java b/org.spearce.jgit/src/org/spearce/jgit/treewalk/CanonicalTreeParser.java\nindex 8028b14..5c331ca 100644\n--- a/org.spearce.jgit/src/org/spearce/jgit/treewalk/CanonicalTreeParser.java\n+++ b/org.spearce.jgit/src/org/spearce/jgit/treewalk/CanonicalTreeParser.java\n@@ -145,8 +145,19 @@ public CanonicalTreeParser resetRoot(final Repository repo,\n \n \t/** @return this iterator, or its parent, if the tree is at eof. */\n \tpublic CanonicalTreeParser next() {\n-\t\tnext(1);\n-\t\treturn eof() && parent != null ? (CanonicalTreeParser) parent : this;\n+\t\tCanonicalTreeParser p = this;\n+\t\tfor (;;) {\n+\t\t\tp.next(1);\n+\t\t\tif (p.eof() && parent != null) {\n+\t\t\t\t// Parent was left pointing at the entry for us; advance\n+\t\t\t\t// the parent to the next entry, possibly unwinding many\n+\t\t\t\t// levels up the tree.\n+\t\t\t\t//\n+\t\t\t\tp = (CanonicalTreeParser) p.parent;\n+\t\t\t\tcontinue;\n+\t\t\t}\n+\t\t\treturn p;\n+\t\t}\n \t}\n \n \t/**\n@@ -192,8 +203,31 @@ public CanonicalTreeParser createSubtreeIterator(final Repository repo,\n \t\t\tfinal ObjectId me = idBuffer.toObjectId();\n \t\t\tthrow new IncorrectObjectTypeException(me, Constants.TYPE_TREE);\n \t\t}\n+\t\treturn createSubtreeIterator0(repo, idBuffer, curs);\n+\t}\n+\n+\t/**\n+\t * Back door to quickly create a subtree iterator for any subtree.\n+\t * <p>\n+\t * Don't use this unless you are ObjectWalk. The method is meant to be\n+\t * called only once the current entry has been identified as a tree and its\n+\t * identity has been converted into an ObjectId.\n+\t *\n+\t * @param repo\n+\t *            repository to load the tree data from.\n+\t * @param id\n+\t *            ObjectId of the tree to open.\n+\t * @param curs\n+\t *            window cursor to use during repository access.\n+\t * @return a new parser that walks over the current subtree.\n+\t * @throws IOException\n+\t *             a loose object or pack file could not be read.\n+\t */\n+\tpublic final CanonicalTreeParser createSubtreeIterator0(\n+\t\t\tfinal Repository repo, final AnyObjectId id, final WindowCursor curs)\n+\t\t\tthrows IOException {\n \t\tfinal CanonicalTreeParser p = new CanonicalTreeParser(this);\n-\t\tp.reset(repo, idBuffer, curs);\n+\t\tp.reset(repo, id, curs);\n \t\treturn p;\n \t}\n \n-- \n1.6.2.288.gc3f22\n"},{"id":"107977","messageId":"1236967912-15088-3-git-send-email-spearce@spearce.org","threadId":"18306","inReplyTo":"1236967912-15088-2-git-send-email-spearce@spearce.org","subject":"[JGIT PATCH 3/3] Use a common skipObject method to avoid UNINTERESTING items","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2009-03-13T18:11:52Z","receivedAt":"2009-03-13T18:11:52Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"All cases are using the same logic to decide that we should skip\nthis current object and not return it to the caller.  A common\nimplementation makes the code easier to follow, especially as it\nreduces the ugly line wrap involved in the loop body.\n\nSigned-off-by: Shawn O. Pearce <spearce@spearce.org>\n---\n .../src/org/spearce/jgit/revwalk/ObjectWalk.java   |   12 +++++++-----\n 1 files changed, 7 insertions(+), 5 deletions(-)\n\ndiff --git a/org.spearce.jgit/src/org/spearce/jgit/revwalk/ObjectWalk.java b/org.spearce.jgit/src/org/spearce/jgit/revwalk/ObjectWalk.java\nindex a481639..b92629e 100644\n--- a/org.spearce.jgit/src/org/spearce/jgit/revwalk/ObjectWalk.java\n+++ b/org.spearce.jgit/src/org/spearce/jgit/revwalk/ObjectWalk.java\n@@ -255,8 +255,7 @@ public RevObject nextObject() throws MissingObjectException,\n \t\t\t\tif ((o.flags & SEEN) != 0)\n \t\t\t\t\tbreak;\n \t\t\t\to.flags |= SEEN;\n-\t\t\t\tif ((o.flags & UNINTERESTING) != 0\n-\t\t\t\t\t\t&& !hasRevSort(RevSort.BOUNDARY))\n+\t\t\t\tif (skipObject(o))\n \t\t\t\t\tbreak;\n \t\t\t\tfromTreeWalk = true;\n \t\t\t\treturn o;\n@@ -267,8 +266,7 @@ public RevObject nextObject() throws MissingObjectException,\n \t\t\t\tif ((o.flags & SEEN) != 0)\n \t\t\t\t\tbreak;\n \t\t\t\to.flags |= SEEN;\n-\t\t\t\tif ((o.flags & UNINTERESTING) != 0\n-\t\t\t\t\t\t&& !hasRevSort(RevSort.BOUNDARY))\n+\t\t\t\tif (skipObject(o))\n \t\t\t\t\tbreak;\n \t\t\t\tnextSubtree = o;\n \t\t\t\tfromTreeWalk = true;\n@@ -294,7 +292,7 @@ public RevObject nextObject() throws MissingObjectException,\n \t\t\tif ((o.flags & SEEN) != 0)\n \t\t\t\tcontinue;\n \t\t\to.flags |= SEEN;\n-\t\t\tif ((o.flags & UNINTERESTING) != 0 && !hasRevSort(RevSort.BOUNDARY))\n+\t\t\tif (skipObject(o))\n \t\t\t\tcontinue;\n \t\t\tif (o instanceof RevTree) {\n \t\t\t\tcurrentTree = (RevTree) o;\n@@ -304,6 +302,10 @@ public RevObject nextObject() throws MissingObjectException,\n \t\t}\n \t}\n \n+\tprivate final boolean skipObject(final RevObject o) {\n+\t\treturn (o.flags & UNINTERESTING) != 0 && !hasRevSort(RevSort.BOUNDARY);\n+\t}\n+\n \t/**\n \t * Verify all interesting objects are available, and reachable.\n \t * <p>\n-- \n1.6.2.288.gc3f22\n"},{"id":"107978","messageId":"20090313183703.GG22920@spearce.org","threadId":"18306","inReplyTo":"1236967912-15088-2-git-send-email-spearce@spearce.org","subject":"Re: [JGIT PATCH 2/3] Fix ObjectWalk to handle single-entry subtrees correctly","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2009-03-13T18:37:03Z","receivedAt":"2009-03-13T18:37:03Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"\"Shawn O. Pearce\" <spearce@spearce.org> wrote:\n> diff --git a/org.spearce.jgit/src/org/spearce/jgit/treewalk/CanonicalTreeParser.java b/org.spearce.jgit/src/org/spearce/jgit/treewalk/CanonicalTreeParser.java\n> index 8028b14..5c331ca 100644\n> --- a/org.spearce.jgit/src/org/spearce/jgit/treewalk/CanonicalTreeParser.java\n> +++ b/org.spearce.jgit/src/org/spearce/jgit/treewalk/CanonicalTreeParser.java\n> @@ -145,8 +145,19 @@ public CanonicalTreeParser resetRoot(final Repository repo,\n>  \n>  \t/** @return this iterator, or its parent, if the tree is at eof. */\n>  \tpublic CanonicalTreeParser next() {\n> -\t\tnext(1);\n> -\t\treturn eof() && parent != null ? (CanonicalTreeParser) parent : this;\n> +\t\tCanonicalTreeParser p = this;\n> +\t\tfor (;;) {\n> +\t\t\tp.next(1);\n> +\t\t\tif (p.eof() && parent != null) {\n> +\t\t\t\t// Parent was left pointing at the entry for us; advance\n> +\t\t\t\t// the parent to the next entry, possibly unwinding many\n> +\t\t\t\t// levels up the tree.\n> +\t\t\t\t//\n> +\t\t\t\tp = (CanonicalTreeParser) p.parent;\n> +\t\t\t\tcontinue;\n> +\t\t\t}\n> +\t\t\treturn p;\n> +\t\t}\n\nThe parent != null test is wrong, we need \"p.parent\" here:\n\ndiff --git a/org.spearce.jgit/src/org/spearce/jgit/treewalk/CanonicalTreeParser.java b/org.spearce.jgit/src/org/spearce/jgit/treewalk/CanonicalTreeParser.java\nindex 5c331ca..7f89cff 100644\n--- a/org.spearce.jgit/src/org/spearce/jgit/treewalk/CanonicalTreeParser.java\n+++ b/org.spearce.jgit/src/org/spearce/jgit/treewalk/CanonicalTreeParser.java\n@@ -148,7 +148,7 @@ public CanonicalTreeParser next() {\n \t\tCanonicalTreeParser p = this;\n \t\tfor (;;) {\n \t\t\tp.next(1);\n-\t\t\tif (p.eof() && parent != null) {\n+\t\t\tif (p.eof() && p.parent != null) {\n \t\t\t\t// Parent was left pointing at the entry for us; advance\n \t\t\t\t// the parent to the next entry, possibly unwinding many\n \t\t\t\t// levels up the tree.\n-- \nShawn.\n"},{"id":"108029","messageId":"200903150125.56987.robin.rosenberg.lists@dewire.com","threadId":"18306","inReplyTo":"1236967912-15088-3-git-send-email-spearce@spearce.org","subject":"Re: [JGIT PATCH 3/3] Use a common skipObject method to avoid UNINTERESTING items","fromName":"Robin Rosenberg","fromEmail":"robin.rosenberg.lists@dewire.com","sentAt":"2009-03-15T00:25:56Z","receivedAt":"2009-03-15T00:25:56Z","isPatch":true,"sender":{"key":"robin.rosenberg@dewire.com","avatar":"https://avatars.githubusercontent.com/u/46357?v=4"},"body":"fredag 13 mars 2009 19:11:52 skrev \"Shawn O. Pearce\" <spearce@spearce.org>:\n> All cases are using the same logic to decide that we should skip\n> this current object and not return it to the caller.  A common\n> implementation makes the code easier to follow, especially as it\n> reduces the ugly line wrap involved in the loop body.\n\nJava conventions dictate that this method should be called shouldSkipObject. It\nis a boolean method that actually does not itself skip any objects.\n\nI can amend that for you.\n\n-- robin\n"},{"id":"108083","messageId":"20090316141329.GL22920@spearce.org","threadId":"18306","inReplyTo":"200903150125.56987.robin.rosenberg.lists@dewire.com","subject":"Re: [JGIT PATCH 3/3] Use a common skipObject method to avoid UNINTERESTING items","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2009-03-16T14:13:29Z","receivedAt":"2009-03-16T14:13:29Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Robin Rosenberg <robin.rosenberg.lists@dewire.com> wrote:\n> fredag 13 mars 2009 19:11:52 skrev \"Shawn O. Pearce\" <spearce@spearce.org>:\n> > All cases are using the same logic to decide that we should skip\n> > this current object and not return it to the caller.  A common\n> > implementation makes the code easier to follow, especially as it\n> > reduces the ugly line wrap involved in the loop body.\n> \n> Java conventions dictate that this method should be called shouldSkipObject. It\n> is a boolean method that actually does not itself skip any objects.\n> \n> I can amend that for you.\n\nYup, amend away.  shouldSkipObject is a much better name.  Thanks.\n\n-- \nShawn.\n"}]}