[JGIT PATCH 1/3] Add lookupBlob to RevWalk

[JGIT PATCH 1/3] Add lookupBlob to RevWalk

[Shawn O. Pearce]

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

[JGIT PATCH 2/3] Fix ObjectWalk to handle single-entry subtrees correctly

[Shawn O. Pearce]

"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

[JGIT PATCH 3/3] Use a common skipObject method to avoid UNINTERESTING items

[Shawn O. Pearce]

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

Re: [JGIT PATCH 2/3] Fix ObjectWalk to handle single-entry subtrees correctly

[Shawn O. Pearce]

"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.

Re: [JGIT PATCH 3/3] Use a common skipObject method to avoid UNINTERESTING items

[Robin Rosenberg]

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

Re: [JGIT PATCH 3/3] Use a common skipObject method to avoid UNINTERESTING items

[Shawn O. Pearce]

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.