Fix MissingObjectException race in ObjectDirectory

Johannes Carlsson identified a race condition[1] that can lead to
spurious MissingObjectExceptions at read time. If two threads are
active inside of ObjectDirectory looking for a packed object and the
packList is currently the empty NO_PACKS list, thread A will find
no object and eventually consider tryAgain1(). If thread A is put
to sleep and this point and thread B also does not find the object,
loads the packs, when thread A wakes up its tryAgain1 would return
false and the thread never considers the packs.

Rework the internal API of ObjectDirectory to keep a handle on the
exact PackList that was iterated by thread A, allowing it to always
retry walking through the packs if the new PackList is different.

This had some ripple effect into the CachedObjectDirectory and
the shared FileObjectDatabase interface. The new code should be
slightly easier to follow, especially from the perspective of the
CachedObjectDirectory trying to minimize the number of open system
calls it makes to files matching "$GIT_DIR/objects/??/?x{38}".

[1] http://dev.eclipse.org/mhonarc/lists/jgit-dev/msg02401.html

Change-Id: I9a1c9d6ad6cb38404b7b9178167b714077561353

org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/CachedObjectDirectory.java

@@ -72,11 +72,11 @@ class CachedObjectDirectory extends FileObjectDatabase {
 	 * The set that contains unpacked objects identifiers, it is created when
 	 * the cached instance is created.
 	 */
-	private final ObjectIdOwnerMap<UnpackedObjectId> unpackedObjects = new ObjectIdOwnerMap<UnpackedObjectId>();
+	private ObjectIdOwnerMap<UnpackedObjectId> unpackedObjects;
 
 	private final ObjectDirectory wrapped;
 
-	private AlternateHandle[] alts;
+	private CachedObjectDirectory[] alts;
 
 	/**
 	 * The constructor
@@ -86,11 +86,15 @@ class CachedObjectDirectory extends FileObjectDatabase {
 	 */
 	CachedObjectDirectory(ObjectDirectory wrapped) {
 		this.wrapped = wrapped;
+		this.unpackedObjects = scanLoose();
+	}
 
+	private ObjectIdOwnerMap<UnpackedObjectId> scanLoose() {
+		ObjectIdOwnerMap<UnpackedObjectId> m = new ObjectIdOwnerMap<UnpackedObjectId>();
 		File objects = wrapped.getDirectory();
 		String[] fanout = objects.list();
 		if (fanout == null)
-			fanout = new String[0];
+			return m;
 		for (String d : fanout) {
 			if (d.length() != 2)
 				continue;
@@ -102,12 +106,13 @@ class CachedObjectDirectory extends FileObjectDatabase {
 					continue;
 				try {
 					ObjectId id = ObjectId.fromString(d + e);
-					unpackedObjects.add(new UnpackedObjectId(id));
+					m.add(new UnpackedObjectId(id));
 				} catch (IllegalArgumentException notAnObject) {
 					// ignoring the file that does not represent loose object
 				}
 			}
 		}
+		return m;
 	}
 
 	@Override
@@ -121,13 +126,13 @@ public ObjectDatabase newCachedDatabase() {
 	}
 
 	@Override
-	FileObjectDatabase newCachedFileObjectDatabase() {
+	File getDirectory() {
-		return this;
+		return wrapped.getDirectory();
 	}
 
 	@Override
-	File getDirectory() {
+	File fileFor(AnyObjectId id) {
-		return wrapped.getDirectory();
+		return wrapped.fileFor(id);
 	}
 
 	@Override
@@ -145,15 +150,12 @@ Set<ObjectId> getShallowCommits() throws IOException {
 		return wrapped.getShallowCommits();
 	}
 
-	@Override
+	private CachedObjectDirectory[] myAlternates() {
-	AlternateHandle[] myAlternates() {
 		if (alts == null) {
-			AlternateHandle[] src = wrapped.myAlternates();
+			ObjectDirectory.AlternateHandle[] src = wrapped.myAlternates();
-			alts = new AlternateHandle[src.length];
+			alts = new CachedObjectDirectory[src.length];
-			for (int i = 0; i < alts.length; i++) {
+			for (int i = 0; i < alts.length; i++)
-				FileObjectDatabase s = src[i].db;
+				alts[i] = src[i].db.newCachedFileObjectDatabase();
-				alts[i] = new AlternateHandle(s.newCachedFileObjectDatabase());
-			}
 		}
 		return alts;
 	}
@@ -170,61 +172,53 @@ void resolve(Set<ObjectId> matches, AbbreviatedObjectId id)
 	}
 
 	@Override
-	boolean tryAgain1() {
+	public boolean has(final AnyObjectId objectId) throws IOException {
-		return wrapped.tryAgain1();
+		if (unpackedObjects.contains(objectId))
-	}
+			return true;
-
+		if (wrapped.hasPackedObject(objectId))
-	@Override
+			return true;
-	public boolean has(final AnyObjectId objectId) {
+		for (CachedObjectDirectory alt : myAlternates()) {
-		return hasObjectImpl1(objectId);
+			if (alt.has(objectId))
-	}
+				return true;
-
+		}
-	@Override
+		return false;
-	boolean hasObject1(AnyObjectId objectId) {
-		return unpackedObjects.contains(objectId)
-				|| wrapped.hasObject1(objectId);
 	}
 
 	@Override
 	ObjectLoader openObject(final WindowCursor curs,
 			final AnyObjectId objectId) throws IOException {
-		return openObjectImpl1(curs, objectId);
+		ObjectLoader ldr = openLooseObject(curs, objectId);
-	}
+		if (ldr != null)
-
+			return ldr;
-	@Override
+		ldr = wrapped.openPackedObject(curs, objectId);
-	ObjectLoader openObject1(WindowCursor curs, AnyObjectId objectId)
+		if (ldr != null)
-			throws IOException {
+			return ldr;
-		if (unpackedObjects.contains(objectId))
+		for (CachedObjectDirectory alt : myAlternates()) {
-			return wrapped.openObject2(curs, objectId.name(), objectId);
+			ldr = alt.openObject(curs, objectId);
-		return wrapped.openObject1(curs, objectId);
+			if (ldr != null)
-	}
+				return ldr;
-
+		}
-	@Override
-	boolean hasObject2(String objectId) {
-		return unpackedObjects.contains(ObjectId.fromString(objectId));
-	}
-
-	@Override
-	ObjectLoader openObject2(WindowCursor curs, String objectName,
-			AnyObjectId objectId) throws IOException {
-		if (unpackedObjects.contains(objectId))
-			return wrapped.openObject2(curs, objectName, objectId);
 		return null;
 	}
 
 	@Override
-	long getObjectSize1(WindowCursor curs, AnyObjectId objectId) throws IOException {
+	long getObjectSize(WindowCursor curs, AnyObjectId objectId)
-		if (unpackedObjects.contains(objectId))
+			throws IOException {
-			return wrapped.getObjectSize2(curs, objectId.name(), objectId);
+		// Object size is unlikely to be requested from contexts using
-		return wrapped.getObjectSize1(curs, objectId);
+		// this type. Don't bother trying to accelerate the lookup.
+		return wrapped.getObjectSize(curs, objectId);
 	}
 
 	@Override
-	long getObjectSize2(WindowCursor curs, String objectName, AnyObjectId objectId)
+	ObjectLoader openLooseObject(WindowCursor curs, AnyObjectId id)
 			throws IOException {
-		if (unpackedObjects.contains(objectId))
+		if (unpackedObjects.contains(id)) {
-			return wrapped.getObjectSize2(curs, objectName, objectId);
+			ObjectLoader ldr = wrapped.openLooseObject(curs, id);
-		return -1;
+			if (ldr != null)
+				return ldr;
+			unpackedObjects = scanLoose();
+		}
+		return null;
 	}
 
 	@Override

org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/FileObjectDatabase.java

@@ -74,61 +74,6 @@ public ObjectDirectoryInserter newInserter() {
 		return new ObjectDirectoryInserter(this, getConfig());
 	}
 
-	/**
-	 * Does the requested object exist in this database?
-	 * <p>
-	 * Alternates (if present) are searched automatically.
-	 *
-	 * @param objectId
-	 *            identity of the object to test for existence of.
-	 * @return true if the specified object is stored in this database, or any
-	 *         of the alternate databases.
-	 */
-	public boolean has(final AnyObjectId objectId) {
-		return hasObjectImpl1(objectId) || hasObjectImpl2(objectId.name());
-	}
-
-	/**
-	 * Compute the location of a loose object file.
-	 *
-	 * @param objectId
-	 *            identity of the loose object to map to the directory.
-	 * @return location of the object, if it were to exist as a loose object.
-	 */
-	File fileFor(final AnyObjectId objectId) {
-		return fileFor(objectId.name());
-	}
-
-	File fileFor(final String objectName) {
-		final String d = objectName.substring(0, 2);
-		final String f = objectName.substring(2);
-		return new File(new File(getDirectory(), d), f);
-	}
-
-	final boolean hasObjectImpl1(final AnyObjectId objectId) {
-		if (hasObject1(objectId))
-			return true;
-
-		for (final AlternateHandle alt : myAlternates()) {
-			if (alt.db.hasObjectImpl1(objectId))
-				return true;
-		}
-
-		return tryAgain1() && hasObject1(objectId);
-	}
-
-	final boolean hasObjectImpl2(final String objectId) {
-		if (hasObject2(objectId))
-			return true;
-
-		for (final AlternateHandle alt : myAlternates()) {
-			if (alt.db.hasObjectImpl2(objectId))
-				return true;
-		}
-
-		return false;
-	}
-
 	abstract void resolve(Set<ObjectId> matches, AbbreviatedObjectId id)
 			throws IOException;
 
@@ -138,180 +83,26 @@ abstract void resolve(Set<ObjectId> matches, AbbreviatedObjectId id)
 
 	abstract Set<ObjectId> getShallowCommits() throws IOException;
 
-	/**
-	 * Open an object from this database.
-	 * <p>
-	 * Alternates (if present) are searched automatically.
-	 *
-	 * @param curs
-	 *            temporary working space associated with the calling thread.
-	 * @param objectId
-	 *            identity of the object to open.
-	 * @return a {@link ObjectLoader} for accessing the data of the named
-	 *         object, or null if the object does not exist.
-	 * @throws IOException
-	 */
-	ObjectLoader openObject(final WindowCursor curs, final AnyObjectId objectId)
-			throws IOException {
-		ObjectLoader ldr;
-
-		ldr = openObjectImpl1(curs, objectId);
-		if (ldr != null)
-			return ldr;
-
-		ldr = openObjectImpl2(curs, objectId.name(), objectId);
-		if (ldr != null)
-			return ldr;
-
-		return null;
-	}
-
-	final ObjectLoader openObjectImpl1(final WindowCursor curs,
-			final AnyObjectId objectId) throws IOException {
-		ObjectLoader ldr;
-
-		ldr = openObject1(curs, objectId);
-		if (ldr != null)
-			return ldr;
-
-		for (final AlternateHandle alt : myAlternates()) {
-			ldr = alt.db.openObjectImpl1(curs, objectId);
-			if (ldr != null)
-				return ldr;
-		}
-
-		if (tryAgain1()) {
-			ldr = openObject1(curs, objectId);
-			if (ldr != null)
-				return ldr;
-		}
-
-		return null;
-	}
-
-	final ObjectLoader openObjectImpl2(final WindowCursor curs,
-			final String objectName, final AnyObjectId objectId)
-			throws IOException {
-		ObjectLoader ldr;
-
-		ldr = openObject2(curs, objectName, objectId);
-		if (ldr != null)
-			return ldr;
-
-		for (final AlternateHandle alt : myAlternates()) {
-			ldr = alt.db.openObjectImpl2(curs, objectName, objectId);
-			if (ldr != null)
-				return ldr;
-		}
-
-		return null;
-	}
-
-	long getObjectSize(WindowCursor curs, AnyObjectId objectId)
-			throws IOException {
-		long sz = getObjectSizeImpl1(curs, objectId);
-		if (0 <= sz)
-			return sz;
-		return getObjectSizeImpl2(curs, objectId.name(), objectId);
-	}
-
-	final long getObjectSizeImpl1(final WindowCursor curs,
-			final AnyObjectId objectId) throws IOException {
-		long sz;
-
-		sz = getObjectSize1(curs, objectId);
-		if (0 <= sz)
-			return sz;
-
-		for (final AlternateHandle alt : myAlternates()) {
-			sz = alt.db.getObjectSizeImpl1(curs, objectId);
-			if (0 <= sz)
-				return sz;
-		}
-
-		if (tryAgain1()) {
-			sz = getObjectSize1(curs, objectId);
-			if (0 <= sz)
-				return sz;
-		}
-
-		return -1;
-	}
-
-	final long getObjectSizeImpl2(final WindowCursor curs,
-			final String objectName, final AnyObjectId objectId)
-			throws IOException {
-		long sz;
-
-		sz = getObjectSize2(curs, objectName, objectId);
-		if (0 <= sz)
-			return sz;
-
-		for (final AlternateHandle alt : myAlternates()) {
-			sz = alt.db.getObjectSizeImpl2(curs, objectName, objectId);
-			if (0 <= sz)
-				return sz;
-		}
-
-		return -1;
-	}
-
 	abstract void selectObjectRepresentation(PackWriter packer,
 			ObjectToPack otp, WindowCursor curs) throws IOException;
 
 	abstract File getDirectory();
 
-	abstract AlternateHandle[] myAlternates();
+	abstract File fileFor(AnyObjectId id);
 
-	abstract boolean tryAgain1();
+	abstract ObjectLoader openObject(WindowCursor curs, AnyObjectId objectId)
-
-	abstract boolean hasObject1(AnyObjectId objectId);
-
-	abstract boolean hasObject2(String objectId);
-
-	abstract ObjectLoader openObject1(WindowCursor curs, AnyObjectId objectId)
 			throws IOException;
 
-	abstract ObjectLoader openObject2(WindowCursor curs, String objectName,
+	abstract long getObjectSize(WindowCursor curs, AnyObjectId objectId)
-			AnyObjectId objectId) throws IOException;
-
-	abstract long getObjectSize1(WindowCursor curs, AnyObjectId objectId)
 			throws IOException;
 
-	abstract long getObjectSize2(WindowCursor curs, String objectName,
+	abstract ObjectLoader openLooseObject(WindowCursor curs, AnyObjectId id)
-			AnyObjectId objectId) throws IOException;
+			throws IOException;
 
 	abstract InsertLooseObjectResult insertUnpackedObject(File tmp,
 			ObjectId id, boolean createDuplicate) throws IOException;
 
 	abstract PackFile openPack(File pack) throws IOException;
 
-	abstract FileObjectDatabase newCachedFileObjectDatabase();
-
 	abstract Collection<PackFile> getPacks();
-
-	static class AlternateHandle {
-		final FileObjectDatabase db;
-
-		AlternateHandle(FileObjectDatabase db) {
-			this.db = db;
-		}
-
-		void close() {
-			db.close();
-		}
-	}
-
-	static class AlternateRepository extends AlternateHandle {
-		final FileRepository repository;
-
-		AlternateRepository(FileRepository r) {
-			super(r.getObjectDatabase());
-			repository = r;
-		}
-
-		void close() {
-			repository.close();
-		}
-	}
 }

org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/FileRepository.java

@@ -59,8 +59,8 @@
 import org.eclipse.jgit.events.ConfigChangedListener;
 import org.eclipse.jgit.events.IndexChangedEvent;
 import org.eclipse.jgit.internal.JGitText;
-import org.eclipse.jgit.internal.storage.file.FileObjectDatabase.AlternateHandle;
+import org.eclipse.jgit.internal.storage.file.ObjectDirectory.AlternateHandle;
-import org.eclipse.jgit.internal.storage.file.FileObjectDatabase.AlternateRepository;
+import org.eclipse.jgit.internal.storage.file.ObjectDirectory.AlternateRepository;
 import org.eclipse.jgit.lib.BaseRepositoryBuilder;
 import org.eclipse.jgit.lib.ConfigConstants;
 import org.eclipse.jgit.lib.Constants;
@@ -368,7 +368,7 @@ public FileBasedConfig getConfig() {
 	 */
 	public Set<ObjectId> getAdditionalHaves() {
 		HashSet<ObjectId> r = new HashSet<ObjectId>();
-		for (AlternateHandle d : objectDatabase. myAlternates()) {
+		for (AlternateHandle d : objectDatabase.myAlternates()) {
 			if (d instanceof AlternateRepository) {
 				Repository repo;
 

org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/LargePackedDeltaObject.java

@@ -173,7 +173,7 @@ public ObjectStream openStream() throws MissingObjectException, IOException {
 		//
 		final ObjectId myId = getObjectId();
 		final WindowCursor wc = new WindowCursor(db);
-		ObjectLoader ldr = db.openObject2(wc, myId.name(), myId);
+		ObjectLoader ldr = db.openLooseObject(wc, myId);
 		if (ldr != null)
 			return ldr.openStream();
 

org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/ObjectDirectory.java

@@ -200,31 +200,19 @@ public void close() {
 		unpackedObjectCache.clear();
 
 		final PackList packs = packList.get();
-		packList.set(NO_PACKS);
+		if (packs != NO_PACKS && packList.compareAndSet(packs, NO_PACKS)) {
-		for (final PackFile p : packs.packs)
+			for (PackFile p : packs.packs)
-			p.close();
+				p.close();
+		}
 
 		// Fully close all loaded alternates and clear the alternate list.
 		AlternateHandle[] alt = alternates.get();
-		if (alt != null) {
+		if (alt != null && alternates.compareAndSet(alt, null)) {
-			alternates.set(null);
 			for(final AlternateHandle od : alt)
 				od.close();
 		}
 	}
 
-	/**
-	 * Compute the location of a loose object file.
-	 *
-	 * @param objectId
-	 *            identity of the loose object to map to the directory.
-	 * @return location of the object, if it were to exist as a loose object.
-	 */
-	@Override
-	public File fileFor(final AnyObjectId objectId) {
-		return super.fileFor(objectId);
-	}
-
 	/**
 	 * @return unmodifiable collection of all known pack files local to this
 	 *         directory. Most recent packs are presented first. Packs most
@@ -277,37 +265,64 @@ public PackFile openPack(final File pack)
 
 	@Override
 	public String toString() {
-		return "ObjectDirectory[" + getDirectory() + "]"; //$NON-NLS-1$
+		return "ObjectDirectory[" + getDirectory() + "]"; //$NON-NLS-1$ //$NON-NLS-2$
 	}
 
-	boolean hasObject1(final AnyObjectId objectId) {
+	@Override
-		if (unpackedObjectCache.isUnpacked(objectId))
+	public boolean has(AnyObjectId objectId) {
+		return unpackedObjectCache.isUnpacked(objectId)
+				|| hasPackedInSelfOrAlternate(objectId)
+				|| hasLooseInSelfOrAlternate(objectId);
+	}
+
+	private boolean hasPackedInSelfOrAlternate(AnyObjectId objectId) {
+		if (hasPackedObject(objectId))
 			return true;
-		for (final PackFile p : packList.get().packs) {
+		for (AlternateHandle alt : myAlternates()) {
-			try {
+			if (alt.db.hasPackedInSelfOrAlternate(objectId))
-				if (p.hasObject(objectId)) {
+				return true;
-					return true;
-				}
-			} catch (IOException e) {
-				// The hasObject call should have only touched the index,
-				// so any failure here indicates the index is unreadable
-				// by this process, and the pack is likewise not readable.
-				//
-				removePack(p);
-				continue;
-			}
 		}
 		return false;
 	}
 
+	private boolean hasLooseInSelfOrAlternate(AnyObjectId objectId) {
+		if (fileFor(objectId).exists())
+			return true;
+		for (AlternateHandle alt : myAlternates()) {
+			if (alt.db.hasLooseInSelfOrAlternate(objectId))
+				return true;
+		}
+		return false;
+	}
+
+	boolean hasPackedObject(AnyObjectId objectId) {
+		PackList pList;
+		do {
+			pList = packList.get();
+			for (PackFile p : pList.packs) {
+				try {
+					if (p.hasObject(objectId))
+						return true;
+				} catch (IOException e) {
+					// The hasObject call should have only touched the index,
+					// so any failure here indicates the index is unreadable
+					// by this process, and the pack is likewise not readable.
+					removePack(p);
+				}
+			}
+		} while (searchPacksAgain(pList));
+		return false;
+	}
+
+	@Override
 	void resolve(Set<ObjectId> matches, AbbreviatedObjectId id)
 			throws IOException {
 		// Go through the packs once. If we didn't find any resolutions
 		// scan for new packs and check once more.
-		//
 		int oldSize = matches.size();
-		PackList pList = packList.get();
+		PackList pList;
-		for (;;) {
+		do {
+			pList = packList.get();
 			for (PackFile p : pList.packs) {
 				try {
 					p.resolve(matches, id, RESOLVE_ABBREV_LIMIT);
@@ -319,15 +334,7 @@ void resolve(Set<ObjectId> matches, AbbreviatedObjectId id)
 				if (matches.size() > RESOLVE_ABBREV_LIMIT)
 					return;
 			}
-			if (matches.size() == oldSize) {
+		} while (matches.size() == oldSize && searchPacksAgain(pList));
-				PackList nList = scanPacks(pList);
-				if (nList == pList || nList.packs.length == 0)
-					break;
-				pList = nList;
-				continue;
-			}
-			break;
-		}
 
 		String fanOut = id.name().substring(0, 2);
 		String[] entries = new File(getDirectory(), fanOut).list();
@@ -354,74 +361,164 @@ void resolve(Set<ObjectId> matches, AbbreviatedObjectId id)
 		}
 	}
 
-	ObjectLoader openObject1(final WindowCursor curs,
+	@Override
-			final AnyObjectId objectId) throws IOException {
+	ObjectLoader openObject(WindowCursor curs, AnyObjectId objectId)
+			throws IOException {
 		if (unpackedObjectCache.isUnpacked(objectId)) {
-			ObjectLoader ldr = openObject2(curs, objectId.name(), objectId);
+			ObjectLoader ldr = openLooseObject(curs, objectId);
 			if (ldr != null)
 				return ldr;
-			else
-				unpackedObjectCache.remove(objectId);
-		}
-
-		PackList pList = packList.get();
-		SEARCH: for (;;) {
-			for (final PackFile p : pList.packs) {
-				try {
-					final ObjectLoader ldr = p.get(curs, objectId);
-					if (ldr != null)
-						return ldr;
-				} catch (PackMismatchException e) {
-					// Pack was modified; refresh the entire pack list.
-					//
-					pList = scanPacks(pList);
-					continue SEARCH;
-				} catch (IOException e) {
-					// Assume the pack is corrupted.
-					//
-					removePack(p);
-				}
-			}
-			return null;
 		}
+		ObjectLoader ldr = openPackedFromSelfOrAlternate(curs, objectId);
+		if (ldr != null)
+			return ldr;
+		return openLooseFromSelfOrAlternate(curs, objectId);
 	}
 
-	long getObjectSize1(final WindowCursor curs, final AnyObjectId objectId)
+	private ObjectLoader openPackedFromSelfOrAlternate(WindowCursor curs,
-			throws IOException {
+			AnyObjectId objectId) {
-		PackList pList = packList.get();
+		ObjectLoader ldr = openPackedObject(curs, objectId);
-		SEARCH: for (;;) {
+		if (ldr != null)
-			for (final PackFile p : pList.packs) {
+			return ldr;
-				try {
+		for (AlternateHandle alt : myAlternates()) {
-					long sz = p.getObjectSize(curs, objectId);
+			ldr = alt.db.openPackedFromSelfOrAlternate(curs, objectId);
-					if (0 <= sz)
+			if (ldr != null)
-						return sz;
+				return ldr;
-				} catch (PackMismatchException e) {
-					// Pack was modified; refresh the entire pack list.
-					//
-					pList = scanPacks(pList);
-					continue SEARCH;
-				} catch (IOException e) {
-					// Assume the pack is corrupted.
-					//
-					removePack(p);
-				}
-			}
-			return -1;
 		}
+		return null;
 	}
 
-	@Override
+	private ObjectLoader openLooseFromSelfOrAlternate(WindowCursor curs,
-	long getObjectSize2(WindowCursor curs, String objectName,
 			AnyObjectId objectId) throws IOException {
+		ObjectLoader ldr = openLooseObject(curs, objectId);
+		if (ldr != null)
+			return ldr;
+		for (AlternateHandle alt : myAlternates()) {
+			ldr = alt.db.openLooseFromSelfOrAlternate(curs, objectId);
+			if (ldr != null)
+				return ldr;
+		}
+		return null;
+	}
+
+	ObjectLoader openPackedObject(WindowCursor curs, AnyObjectId objectId) {
+		PackList pList;
+		do {
+			SEARCH: for (;;) {
+				pList = packList.get();
+				for (PackFile p : pList.packs) {
+					try {
+						ObjectLoader ldr = p.get(curs, objectId);
+						if (ldr != null)
+							return ldr;
+					} catch (PackMismatchException e) {
+						// Pack was modified; refresh the entire pack list.
+						if (searchPacksAgain(pList))
+							continue SEARCH;
+					} catch (IOException e) {
+						// Assume the pack is corrupted.
+						removePack(p);
+					}
+				}
+				break SEARCH;
+			}
+		} while (searchPacksAgain(pList));
+		return null;
+	}
+
+	ObjectLoader openLooseObject(WindowCursor curs, AnyObjectId id)
+			throws IOException {
 		try {
-			File path = fileFor(objectName);
+			File path = fileFor(id);
 			FileInputStream in = new FileInputStream(path);
 			try {
-				return UnpackedObject.getSize(in, objectId, curs);
+				unpackedObjectCache.add(id);
+				return UnpackedObject.open(in, path, id, curs);
 			} finally {
 				in.close();
 			}
 		} catch (FileNotFoundException noFile) {
+			unpackedObjectCache.remove(id);
+			return null;
+		}
+	}
+
+	long getObjectSize(WindowCursor curs, AnyObjectId id)
+			throws IOException {
+		if (unpackedObjectCache.isUnpacked(id)) {
+			long len = getLooseObjectSize(curs, id);
+			if (0 <= len)
+				return len;
+		}
+		long len = getPackedSizeFromSelfOrAlternate(curs, id);
+		if (0 <= len)
+			return len;
+		return getLooseSizeFromSelfOrAlternate(curs, id);
+	}
+
+	private long getPackedSizeFromSelfOrAlternate(WindowCursor curs,
+			AnyObjectId id) {
+		long len = getPackedObjectSize(curs, id);
+		if (0 <= len)
+			return len;
+		for (AlternateHandle alt : myAlternates()) {
+			len = alt.db.getPackedSizeFromSelfOrAlternate(curs, id);
+			if (0 <= len)
+				return len;
+		}
+		return -1;
+	}
+
+	private long getLooseSizeFromSelfOrAlternate(WindowCursor curs,
+			AnyObjectId id) throws IOException {
+		long len = getLooseObjectSize(curs, id);
+		if (0 <= len)
+			return len;
+		for (AlternateHandle alt : myAlternates()) {
+			len = alt.db.getLooseSizeFromSelfOrAlternate(curs, id);
+			if (0 <= len)
+				return len;
+		}
+		return -1;
+	}
+
+	private long getPackedObjectSize(WindowCursor curs, AnyObjectId id) {
+		PackList pList;
+		do {
+			SEARCH: for (;;) {
+				pList = packList.get();
+				for (PackFile p : pList.packs) {
+					try {
+						long len = p.getObjectSize(curs, id);
+						if (0 <= len)
+							return len;
+					} catch (PackMismatchException e) {
+						// Pack was modified; refresh the entire pack list.
+						if (searchPacksAgain(pList))
+							continue SEARCH;
+					} catch (IOException e) {
+						// Assume the pack is corrupted.
+						removePack(p);
+					}
+				}
+				break SEARCH;
+			}
+		} while (searchPacksAgain(pList));
+		return -1;
+	}
+
+	private long getLooseObjectSize(WindowCursor curs, AnyObjectId id)
+			throws IOException {
+		try {
+			FileInputStream in = new FileInputStream(fileFor(id));
+			try {
+				unpackedObjectCache.add(id);
+				return UnpackedObject.getSize(in, id, curs);
+			} finally {
+				in.close();
+			}
+		} catch (FileNotFoundException noFile) {
+			unpackedObjectCache.remove(id);
 			return -1;
 		}
 	}
@@ -454,28 +551,6 @@ void selectObjectRepresentation(PackWriter packer, ObjectToPack otp,
 			h.db.selectObjectRepresentation(packer, otp, curs);
 	}
 
-	boolean hasObject2(final String objectName) {
-		return fileFor(objectName).exists();
-	}
-
-	ObjectLoader openObject2(final WindowCursor curs,
-			final String objectName, final AnyObjectId objectId)
-			throws IOException {
-		try {
-			File path = fileFor(objectName);
-			FileInputStream in = new FileInputStream(path);
-			try {
-				unpackedObjectCache.add(objectId);
-				return UnpackedObject.open(in, path, objectId, curs);
-			} finally {
-				in.close();
-			}
-		} catch (FileNotFoundException noFile) {
-			unpackedObjectCache.remove(objectId);
-			return null;
-		}
-	}
-
 	@Override
 	InsertLooseObjectResult insertUnpackedObject(File tmp, ObjectId id,
 			boolean createDuplicate) throws IOException {
@@ -530,11 +605,8 @@ InsertLooseObjectResult insertUnpackedObject(File tmp, ObjectId id,
 		return InsertLooseObjectResult.FAILURE;
 	}
 
-	boolean tryAgain1() {
+	private boolean searchPacksAgain(PackList old) {
-		final PackList old = packList.get();
+		return old.snapshot.isModified(packDirectory) && old != scanPacks(old);
-		if (old.snapshot.isModified(packDirectory))
-			return old != scanPacks(old);
-		return false;
 	}
 
 	Config getConfig() {
@@ -794,6 +866,20 @@ private AlternateHandle openAlternate(File objdir) throws IOException {
 		return new AlternateHandle(db);
 	}
 
+	/**
+	 * Compute the location of a loose object file.
+	 *
+	 * @param objectId
+	 *            identity of the loose object to map to the directory.
+	 * @return location of the object, if it were to exist as a loose object.
+	 */
+	public File fileFor(AnyObjectId objectId) {
+		String n = objectId.name();
+		String d = n.substring(0, 2);
+		String f = n.substring(2);
+		return new File(new File(getDirectory(), d), f);
+	}
+
 	private static final class PackList {
 		/** State just before reading the pack directory. */
 		final FileSnapshot snapshot;
@@ -807,12 +893,37 @@ private static final class PackList {
 		}
 	}
 
+	static class AlternateHandle {
+		final ObjectDirectory db;
+
+		AlternateHandle(ObjectDirectory db) {
+			this.db = db;
+		}
+
+		void close() {
+			db.close();
+		}
+	}
+
+	static class AlternateRepository extends AlternateHandle {
+		final FileRepository repository;
+
+		AlternateRepository(FileRepository r) {
+			super(r.getObjectDatabase());
+			repository = r;
+		}
+
+		void close() {
+			repository.close();
+		}
+	}
+
 	@Override
 	public ObjectDatabase newCachedDatabase() {
 		return newCachedFileObjectDatabase();
 	}
 
-	FileObjectDatabase newCachedFileObjectDatabase() {
+	CachedObjectDirectory newCachedFileObjectDatabase() {
 		return new CachedObjectDirectory(this);
 	}
 }