Revert "Teach PackWriter how to reuse an existing object list"

This reverts commit f5fe2dca3cb9f57891e1a4b18832fcc158d0c490.

I regret adding this feature to the public API.  Caches aren't always
the best idea, as they require work to maintain.  Here the cache is
redundant information that must be computed, and when it grows stale
must be removed.  The redundant information takes up more disk space,
about the same size as the pack-*.idx files are.  For the linux-2.6
repository, that's more than 40 MB for a 400 MB repository.  So the
cache is a 10% increase in disk usage.

The entire point of this cache is to improve PackWriter performance,
and only PackWriter performance, and only when sending an initial
clone to a new client.  There may be better ways to optimize this, and
until we have a solid solution, we shouldn't be using a separate cache
in JGit.

3 files changed, 21 insertions, 271 deletions

diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/lib/ObjectReader.java b/org.eclipse.jgit/src/org/eclipse/jgit/lib/ObjectReader.java
index 85864dbbf6..cd3706bbe3 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/lib/ObjectReader.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/lib/ObjectReader.java
@@ -46,14 +46,11 @@ package org.eclipse.jgit.lib;
 import java.io.IOException;
 import java.util.ArrayList;
 import java.util.Collection;
-import java.util.Collections;
 import java.util.Iterator;
 import java.util.List;
-import java.util.Set;
 
 import org.eclipse.jgit.errors.IncorrectObjectTypeException;
 import org.eclipse.jgit.errors.MissingObjectException;
-import org.eclipse.jgit.revwalk.ObjectListIterator;
 import org.eclipse.jgit.revwalk.ObjectWalk;
 import org.eclipse.jgit.revwalk.RevCommit;
 import org.eclipse.jgit.revwalk.RevWalk;
@@ -431,35 +428,6 @@ public abstract class ObjectReader {
 	}
 
 	/**
-	 * Obtain the available pre-computed object reachability lists.
-	 * <p>
-	 * The lists are indexed by commit ObjectId, so the returned set contains
-	 * the commit ObjectIds naming each set.
-	 *
-	 * @return set of commit ObjectIds that identify lists.
-	 */
-	public Set<ObjectId> getAvailableObjectLists() {
-		return Collections.emptySet();
-	}
-
-	/**
-	 * Open a pre-computed object list for reading.
-	 *
-	 * @param listName
-	 *            a commit ObjectId previously returned by
-	 *            {@link #getAvailableObjectLists()}.
-	 * @param walker
-	 *            the revision pool to use when looking up objects.
-	 * @return the list iterator.
-	 * @throws IOException
-	 *             the reader cannot load the precomputed list.
-	 */
-	public ObjectListIterator openObjectList(AnyObjectId listName,
-			ObjectWalk walker) throws IOException {
-		throw new UnsupportedOperationException();
-	}
-
-	/**
 	 * Release any resources used by this reader.
 	 * <p>
 	 * A reader that has been released can be used again, but may need to be
diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/revwalk/ObjectListIterator.java b/org.eclipse.jgit/src/org/eclipse/jgit/revwalk/ObjectListIterator.java
deleted file mode 100644
index ab9cbec12d..0000000000
--- a/org.eclipse.jgit/src/org/eclipse/jgit/revwalk/ObjectListIterator.java
+++ /dev/null
@@ -1,140 +0,0 @@
-/*
- * Copyright (C) 2011, Google Inc.
- * and other copyright owners as documented in the project's IP log.
- *
- * This program and the accompanying materials are made available
- * under the terms of the Eclipse Distribution License v1.0 which
- * accompanies this distribution, is reproduced below, and is
- * available at http://www.eclipse.org/org/documents/edl-v10.php
- *
- * All rights reserved.
- *
- * Redistribution and use in source and binary forms, with or
- * without modification, are permitted provided that the following
- * conditions are met:
- *
- * - Redistributions of source code must retain the above copyright
- *   notice, this list of conditions and the following disclaimer.
- *
- * - Redistributions in binary form must reproduce the above
- *   copyright notice, this list of conditions and the following
- *   disclaimer in the documentation and/or other materials provided
- *   with the distribution.
- *
- * - Neither the name of the Eclipse Foundation, Inc. nor the
- *   names of its contributors may be used to endorse or promote
- *   products derived from this software without specific prior
- *   written permission.
- *
- * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND
- * CONTRIBUTORS "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES,
- * INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES
- * OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
- * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR
- * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
- * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT
- * NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
- * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER
- * CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT,
- * STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
- * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF
- * ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
- */
-
-package org.eclipse.jgit.revwalk;
-
-import java.io.IOException;
-
-import org.eclipse.jgit.lib.AnyObjectId;
-import org.eclipse.jgit.storage.pack.PackWriter;
-
-/**
- * Iterates through an open object list.
- * <p>
- * A cached object list should be constructed by enumerating from a single
- * stable commit back to the beginning of the project, using an ObjectWalk:
- *
- * <pre>
- * ObjectWalk walk = new ObjectWalk(repository);
- * walk.markStart(walk.parseCommit(listName));
- *
- * RevCommit commit;
- * while ((commit = walk.next()) != null)
- * 	list.addCommit(commit);
- *
- * RevObject object;
- * while ((object == walk.nextObject()) != null)
- * 	list.addObject(object, walk.getPathHasCode());
- * </pre>
- * <p>
- * {@link PackWriter} relies on the list covering a single commit, and going all
- * the way back to the root. If a list contains multiple starting commits the
- * PackWriter will include all of those objects, even if the client did not ask
- * for them, or should not have been given the objects.
- */
-public abstract class ObjectListIterator {
-	private final ObjectWalk walk;
-
-	/**
-	 * Initialize the list iterator.
-	 *
-	 * @param walk
-	 *            the revision pool the iterator will use when allocating the
-	 *            returned objects.
-	 */
-	protected ObjectListIterator(ObjectWalk walk) {
-		this.walk = walk;
-	}
-
-	/**
-	 * Lookup an object from the revision pool.
-	 *
-	 * @param id
-	 *            the object to allocate.
-	 * @param type
-	 *            the type of the object. The type must be accurate, as it is
-	 *            used to allocate the proper RevObject instance.
-	 * @return the object.
-	 */
-	protected RevObject lookupAny(AnyObjectId id, int type) {
-		return walk.lookupAny(id, type);
-	}
-
-	/**
-	 * Pop the next most recent commit.
-	 * <p>
-	 * Commits should be returned in descending commit time order, or in
-	 * topological order. Either ordering is acceptable for a list to use.
-	 *
-	 * @return next most recent commit; null if traversal is over.
-	 * @throws IOException
-	 *             the list cannot be read.
-	 */
-	public abstract RevCommit next() throws IOException;
-
-	/**
-	 * Pop the next most recent object.
-	 * <p>
-	 * Only RevTree and RevBlob may be returned from this method, as these are
-	 * the only non-commit types reachable from a RevCommit. Lists may return
-	 * the objects clustered by type, or clustered by order of first-discovery
-	 * when walking from the most recent to the oldest commit.
-	 *
-	 * @return the next object. Null at the end of the list.
-	 * @throws IOException
-	 *             the list cannot be read.
-	 */
-	public abstract RevObject nextObject() throws IOException;
-
-	/**
-	 * Get the current object's path hash code.
-	 * <p>
-	 * The path hash code should be cached from the ObjectWalk.
-	 *
-	 * @return path hash code; any integer may be returned.
-	 */
-	public abstract int getPathHashCode();
-
-	/** Release the resources associated with this iterator. */
-	public abstract void release();
-}
diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/storage/pack/PackWriter.java b/org.eclipse.jgit/src/org/eclipse/jgit/storage/pack/PackWriter.java
index 15939de7ac..17c5a12d40 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/storage/pack/PackWriter.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/storage/pack/PackWriter.java
@@ -86,7 +86,6 @@ import org.eclipse.jgit.lib.ProgressMonitor;
 import org.eclipse.jgit.lib.Repository;
 import org.eclipse.jgit.lib.ThreadSafeProgressMonitor;
 import org.eclipse.jgit.revwalk.AsyncRevObjectQueue;
-import org.eclipse.jgit.revwalk.ObjectListIterator;
 import org.eclipse.jgit.revwalk.ObjectWalk;
 import org.eclipse.jgit.revwalk.RevFlag;
 import org.eclipse.jgit.revwalk.RevObject;
@@ -377,8 +376,9 @@ public class PackWriter {
 			throws IOException {
 		if (countingMonitor == null)
 			countingMonitor = NullProgressMonitor.INSTANCE;
-		findObjectsToPack(countingMonitor, interestingObjects,
+		ObjectWalk walker = setUpWalker(interestingObjects,
 				uninterestingObjects);
+		findObjectsToPack(countingMonitor, walker);
 	}
 
 	/**
@@ -972,14 +972,11 @@ public class PackWriter {
 		out.write(packcsum);
 	}
 
-	private void findObjectsToPack(final ProgressMonitor countingMonitor,
+	private ObjectWalk setUpWalker(
 			final Collection<? extends ObjectId> interestingObjects,
 			final Collection<? extends ObjectId> uninterestingObjects)
 			throws MissingObjectException, IOException,
 			IncorrectObjectTypeException {
-		countingMonitor.beginTask(JGitText.get().countingObjects,
-				ProgressMonitor.UNKNOWN);
-
 		List<ObjectId> all = new ArrayList<ObjectId>(interestingObjects.size());
 		for (ObjectId id : interestingObjects)
 			all.add(id.copy());
@@ -994,18 +991,13 @@ public class PackWriter {
 			not = Collections.emptySet();
 
 		final ObjectWalk walker = new ObjectWalk(reader);
-		final RevFlag hasObjectList = walker.newFlag("hasObjectList");
-
 		walker.setRetainBody(false);
-		if (not.isEmpty()) {
+		if (not.isEmpty())
 			walker.sort(RevSort.COMMIT_TIME_DESC);
-			for (ObjectId listName : reader.getAvailableObjectLists())
-				walker.lookupCommit(listName).add(hasObjectList);
-		} else {
+		else
 			walker.sort(RevSort.TOPO);
-			if (thin)
-				walker.sort(RevSort.BOUNDARY, true);
-		}
+		if (thin && !not.isEmpty())
+			walker.sort(RevSort.BOUNDARY, true);
 
 		AsyncRevObjectQueue q = walker.parseAny(all, true);
 		try {
@@ -1028,91 +1020,25 @@ public class PackWriter {
 		} finally {
 			q.release();
 		}
+		return walker;
+	}
 
-		RevObject listName = null;
+	private void findObjectsToPack(final ProgressMonitor countingMonitor,
+			final ObjectWalk walker) throws MissingObjectException,
+			IncorrectObjectTypeException,			IOException {
+		countingMonitor.beginTask(JGitText.get().countingObjects,
+				ProgressMonitor.UNKNOWN);
 		RevObject o;
 
 		while ((o = walker.next()) != null) {
-			if (o.has(hasObjectList)) {
-				listName = o;
-				break;
-			}
-			addResultOrBase(o, 0);
+			addObject(o, 0);
 			countingMonitor.update(1);
 		}
-		if (listName != null) {
-			addByObjectList(listName, countingMonitor, walker,
-					interestingObjects);
-		} else {
-			while ((o = walker.nextObject()) != null) {
-				addResultOrBase(o, walker.getPathHashCode());
-				countingMonitor.update(1);
-			}
-		}
-		countingMonitor.endTask();
-	}
-
-	private void addByObjectList(RevObject listName,
-			final ProgressMonitor countingMonitor, final ObjectWalk walker,
-			final Collection<? extends ObjectId> interestingObjects)
-			throws MissingObjectException, IncorrectObjectTypeException,
-			IOException {
-		int restartedProgress = objectsMap.size();
-
-		for (List<ObjectToPack> list : objectsLists)
-			list.clear();
-		objectsMap.clear();
-
-		walker.reset();
-		walker.markUninteresting(listName);
-
-		for (ObjectId id : interestingObjects)
-			walker.markStart(walker.lookupOrNull(id));
-
-		RevFlag added = walker.newFlag("added");
-		RevObject o;
-		while ((o = walker.next()) != null) {
-			addResult(o, 0);
-			o.add(added);
-			if (restartedProgress != 0)
-				restartedProgress--;
-			else
-				countingMonitor.update(1);
-		}
 		while ((o = walker.nextObject()) != null) {
-			addResult(o, walker.getPathHashCode());
-			o.add(added);
-			if (restartedProgress != 0)
-				restartedProgress--;
-			else
-				countingMonitor.update(1);
-		}
-
-		ObjectListIterator itr = reader.openObjectList(listName, walker);
-		try {
-			while ((o = itr.next()) != null) {
-				if (o.has(added))
-					continue;
-				addResult(o, 0);
-				o.add(added);
-				if (restartedProgress != 0)
-					restartedProgress--;
-				else
-					countingMonitor.update(1);
-			}
-			while ((o = itr.nextObject()) != null) {
-				if (o.has(added))
-					continue;
-				addResult(o, itr.getPathHashCode());
-				o.add(added);
-				if (restartedProgress != 0)
-					restartedProgress--;
-				else
-					countingMonitor.update(1);
-			}
-		} finally {
-			itr.release();
+			addObject(o, walker.getPathHashCode());
+			countingMonitor.update(1);
 		}
+		countingMonitor.endTask();
 	}
 
 	/**
@@ -1129,10 +1055,10 @@ public class PackWriter {
 	 */
 	public void addObject(final RevObject object)
 			throws IncorrectObjectTypeException {
-		addResultOrBase(object, 0);
+		addObject(object, 0);
 	}
 
-	private void addResultOrBase(final RevObject object, final int pathHashCode)
+	private void addObject(final RevObject object, final int pathHashCode)
 			throws IncorrectObjectTypeException {
 		if (object.has(RevFlag.UNINTERESTING)) {
 			switch (object.getType()) {
@@ -1145,13 +1071,9 @@ public class PackWriter {
 				thin = true;
 				break;
 			}
-		} else {
-			addResult(object, pathHashCode);
+			return;
 		}
-	}
 
-	private void addResult(final RevObject object, final int pathHashCode)
-			throws IncorrectObjectTypeException {
 		final ObjectToPack otp;
 		if (reuseSupport != null)
 			otp = reuseSupport.newObjectToPack(object);