[JGIT PATCH 00/10] Object access improvements

[JGIT PATCH 00/10] Object access improvements

[Shawn O. Pearce]

This series tries to address issue 79[1] and GERRIT-81[2] by
detecting when packs have been overwritten on disk, or when
new packs have appeared and old packs have disappeared.

The first 5 patches are quite trivial and are just things I
found while working on the real issue.

Patch 6 is rather intrusive.  It starts a refactoring I have
always wanted to do, which is to split the object database out
of the Repository class, and also start to divorce it from the
local filesystem.  The library is in the same state after patch
5 as it was before patch 5; that is issue 79 is still an issue,
but everything else is fine.

Patch 7 and 8 fix issue 79.

Patch 9 is a rewritten version of your 1/3 posted Sun 19 Apr,
providing test cases for issue 79 / GERRIT-81.

Patch 10 is a broken test case.  Basically as I explain in the
commit message of 7, PackWriter can still crash randomly.

With 1-9 applied, issue 79 can be marked fixed, but GERRIT-81 is
still a problem, as any concurrent PackWriter thread may randomly
crash during a repack.

[1] http://code.google.com/p/egit/issues/detail?id=79
[2] http://jira.source.android.com/jira/browse/GERRIT-81

Shawn O. Pearce (10):
  Safely handle closing an already closed WindowedFile
  Change empty tree test case to use a temporary repository
  Replace hand-coded read fully loop with NB.readFully
  Clear the reverse index when closing a PackFile
  Introduce a new exception type for an in-place pack modification
  Refactor object database access with new abstraction
  Rescan packs if a pack is modified in-place (part 1)
  Scan for new packs if GIT_DIR/objects/pack has been modified
  Add test cases for loading new (or replaced) pack files
  BROKEN TEST: ObjectLoader stays valid across repacks

 .../org/spearce/jgit/lib/ConcurrentRepackTest.java |  252 +++++++++++
 .../org/spearce/jgit/lib/RepositoryTestCase.java   |    2 -
 .../tst/org/spearce/jgit/lib/T0003_Basic.java      |   11 +-
 .../tst/org/spearce/jgit/lib/T0004_PackReader.java |    1 -
 .../spearce/jgit/errors/PackMismatchException.java |   55 +++
 .../src/org/spearce/jgit/lib/ObjectDatabase.java   |  367 ++++++++++++++++
 .../src/org/spearce/jgit/lib/ObjectDirectory.java  |  438 ++++++++++++++++++++
 .../src/org/spearce/jgit/lib/PackFile.java         |   36 ++-
 .../src/org/spearce/jgit/lib/Repository.java       |  174 +--------
 .../org/spearce/jgit/lib/UnpackedObjectLoader.java |   34 +-
 .../src/org/spearce/jgit/lib/WindowedFile.java     |   16 +-
 11 files changed, 1183 insertions(+), 203 deletions(-)
 create mode 100644 org.spearce.jgit.test/tst/org/spearce/jgit/lib/ConcurrentRepackTest.java
 create mode 100644 org.spearce.jgit/src/org/spearce/jgit/errors/PackMismatchException.java
 create mode 100644 org.spearce.jgit/src/org/spearce/jgit/lib/ObjectDatabase.java
 create mode 100644 org.spearce.jgit/src/org/spearce/jgit/lib/ObjectDirectory.java

[JGIT PATCH 01/10] Safely handle closing an already closed WindowedFile

[Shawn O. Pearce]

If cacheOpen throws an exception (e.g. FileNotFoundException) we
may wind up in cacheClose, where the fd is null because we didn't
successfully construct the RandomAccessFile.  In such cases there
is nothing to attempt to close.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 .../src/org/spearce/jgit/lib/WindowedFile.java     |   16 +++++++++-------
 1 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/WindowedFile.java b/org.spearce.jgit/src/org/spearce/jgit/lib/WindowedFile.java
index 938f44c..9293eb9 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/lib/WindowedFile.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/lib/WindowedFile.java
@@ -297,14 +297,16 @@ void cacheOpen() throws IOException {
 	}
 
 	void cacheClose() {
-		try {
-			fd.close();
-		} catch (IOException err) {
-			// Ignore a close event. We had it open only for reading.
-			// There should not be errors related to network buffers
-			// not flushed, etc.
+		if (fd != null) {
+			try {
+				fd.close();
+			} catch (IOException err) {
+				// Ignore a close event. We had it open only for reading.
+				// There should not be errors related to network buffers
+				// not flushed, etc.
+			}
+			fd = null;
 		}
-		fd = null;
 	}
 
 	void allocWindow(final WindowCursor curs, final int windowId,
-- 
1.6.3.rc1.188.ga02b

[JGIT PATCH 02/10] Change empty tree test case to use a temporary repository

[Shawn O. Pearce]

This test case looks for a loose object of the empty tree, but our
stock test repository contains the empty tree in a pack file.  So
the only way we can ensure it will be written is if we write to an
empty repository.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 .../tst/org/spearce/jgit/lib/T0003_Basic.java      |   11 +++++------
 1 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/org.spearce.jgit.test/tst/org/spearce/jgit/lib/T0003_Basic.java b/org.spearce.jgit.test/tst/org/spearce/jgit/lib/T0003_Basic.java
index 6a296be..b9e8d1d 100644
--- a/org.spearce.jgit.test/tst/org/spearce/jgit/lib/T0003_Basic.java
+++ b/org.spearce.jgit.test/tst/org/spearce/jgit/lib/T0003_Basic.java
@@ -75,14 +75,13 @@ public void test002_WriteEmptyTree() throws IOException {
 		// open when we create it we won't write the object file out as a loose
 		// object (as it already exists in the pack).
 		//
-		db.closePacks();
-
-		final Tree t = new Tree(db);
-		t.accept(new WriteTree(trash, db), TreeEntry.MODIFIED_ONLY);
+		final Repository newdb = createNewEmptyRepo();
+		final Tree t = new Tree(newdb);
+		t.accept(new WriteTree(trash, newdb), TreeEntry.MODIFIED_ONLY);
 		assertEquals("4b825dc642cb6eb9a060e54bf8d69288fbee4904", t.getId()
 				.name());
-		final File o = new File(new File(new File(trash_git, "objects"), "4b"),
-				"825dc642cb6eb9a060e54bf8d69288fbee4904");
+		final File o = new File(new File(new File(newdb.getDirectory(),
+				"objects"), "4b"), "825dc642cb6eb9a060e54bf8d69288fbee4904");
 		assertTrue("Exists " + o, o.isFile());
 		assertTrue("Read-only " + o, !o.canWrite());
 	}
-- 
1.6.3.rc1.188.ga02b

[JGIT PATCH 03/10] Replace hand-coded read fully loop with NB.readFully

[Shawn O. Pearce]

This code predates the NB utility class.  I'd prefer to reuse the
code over keeping this duplicate copy of the logic.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 .../org/spearce/jgit/lib/UnpackedObjectLoader.java |   14 ++++++--------
 1 files changed, 6 insertions(+), 8 deletions(-)

diff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/UnpackedObjectLoader.java b/org.spearce.jgit/src/org/spearce/jgit/lib/UnpackedObjectLoader.java
index f2cae87..b086821 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/lib/UnpackedObjectLoader.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/lib/UnpackedObjectLoader.java
@@ -46,6 +46,7 @@
 
 import org.spearce.jgit.errors.CorruptObjectException;
 import org.spearce.jgit.util.MutableInteger;
+import org.spearce.jgit.util.NB;
 import org.spearce.jgit.util.RawParseUtils;
 
 /**
@@ -74,17 +75,14 @@ public UnpackedObjectLoader(final Repository db, final AnyObjectId id)
 
 	private static byte[] readCompressed(final Repository db,
 			final AnyObjectId id) throws FileNotFoundException, IOException {
-		final FileInputStream objStream = new FileInputStream(db.toFile(id));
-		final byte[] compressed;
+		final FileInputStream in = new FileInputStream(db.toFile(id));
 		try {
-			compressed = new byte[objStream.available()];
-			int off = 0;
-			while (off < compressed.length)
-				off += objStream.read(compressed, off, compressed.length - off);
+			final byte[] compressed = new byte[(int) in.getChannel().size()];
+			NB.readFully(in, compressed, 0, compressed.length);
+			return compressed;
 		} finally {
-			objStream.close();
+			in.close();
 		}
-		return compressed;
 	}
 
 	/**
-- 
1.6.3.rc1.188.ga02b

[JGIT PATCH 04/10] Clear the reverse index when closing a PackFile

[Shawn O. Pearce]

We clear the forward index (ObjectId -> offset) when we close the pack,
so we might as well clear the reverse index (offset -> ObjectId).

The reverse index is protected by the lock on "this", just like the
forward index is.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 .../src/org/spearce/jgit/lib/PackFile.java         |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/PackFile.java b/org.spearce.jgit/src/org/spearce/jgit/lib/PackFile.java
index 1e6f170..b525a82 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/lib/PackFile.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/lib/PackFile.java
@@ -155,6 +155,7 @@ public void close() {
 		pack.close();
 		synchronized (this) {
 			loadedIdx = null;
+			reverseIdx = null;
 		}
 	}
 
-- 
1.6.3.rc1.188.ga02b

[JGIT PATCH 05/10] Introduce a new exception type for an in-place pack modification

[Shawn O. Pearce]

If a pack file is modified in place by "git gc" or "git repack"
it means that the list of objects contained within the pack has
not changed, but it is possible for the layout of the pack to
be modified.

The layout can change when the compressed version of any object
is changed, and usually happens because the packer has found a
smaller way to represent a contained object.  It also can change
if an object's delta base encoding format is switched, such as from
OBJ_REF to OBJ_OFS.

This new exception type can be caught by callers to recognize that
the object contents of the pack is quite likely the same, but that
any offsets and cached windows are wrong and need to be computed
or loaded again.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 .../spearce/jgit/errors/PackMismatchException.java |   55 ++++++++++++++++++++
 .../src/org/spearce/jgit/lib/PackFile.java         |   15 +++--
 2 files changed, 64 insertions(+), 6 deletions(-)
 create mode 100644 org.spearce.jgit/src/org/spearce/jgit/errors/PackMismatchException.java

diff --git a/org.spearce.jgit/src/org/spearce/jgit/errors/PackMismatchException.java b/org.spearce.jgit/src/org/spearce/jgit/errors/PackMismatchException.java
new file mode 100644
index 0000000..b96f9e8
--- /dev/null
+++ b/org.spearce.jgit/src/org/spearce/jgit/errors/PackMismatchException.java
@@ -0,0 +1,55 @@
+/*
+ * Copyright (C) 2009, Google Inc.
+ *
+ * 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 Git Development Community 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.spearce.jgit.errors;
+
+import java.io.IOException;
+
+/** Thrown when a PackFile no longer matches the PackIndex. */
+public class PackMismatchException extends IOException {
+	private static final long serialVersionUID = 1L;
+
+	/**
+	 * Construct a pack modification error.
+	 *
+	 * @param why
+	 *            description of the type of error.
+	 */
+	public PackMismatchException(final String why) {
+		super(why);
+	}
+}
diff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/PackFile.java b/org.spearce.jgit/src/org/spearce/jgit/lib/PackFile.java
index b525a82..bda4843 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/lib/PackFile.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/lib/PackFile.java
@@ -51,6 +51,7 @@
 import java.util.zip.DataFormatException;
 
 import org.spearce.jgit.errors.CorruptObjectException;
+import org.spearce.jgit.errors.PackMismatchException;
 import org.spearce.jgit.util.NB;
 
 /**
@@ -292,16 +293,18 @@ private void onOpenPack() throws IOException {
 		final long packCnt = NB.decodeUInt32(intbuf, 0);
 		final long idxCnt = idx.getObjectCount();
 		if (idxCnt != packCnt)
-			throw new IOException("Pack index"
-					+ " object count mismatch; expected " + packCnt
-					+ " found " + idxCnt + ": " + pack.getName());
+			throw new PackMismatchException("Pack object count mismatch:"
+					+ " pack " + packCnt
+					+ " index " + idxCnt
+					+ ": " + pack.getName());
 
 		final byte[] csumbuf = new byte[20];
 		pack.readFully(pack.length() - 20, csumbuf, curs);
 		if (!Arrays.equals(csumbuf, idx.packChecksum))
-			throw new IOException("Pack index mismatch; pack SHA-1 is "
-					+ ObjectId.fromRaw(csumbuf).name() + ", index expects "
-					+ ObjectId.fromRaw(idx.packChecksum).name());
+			throw new PackMismatchException("Pack checksum mismatch:"
+					+ " pack " + ObjectId.fromRaw(csumbuf).name()
+					+ " index " + ObjectId.fromRaw(idx.packChecksum).name()
+					+ ": " + pack.getName());
 	}
 
 	private PackedObjectLoader reader(final WindowCursor curs,
-- 
1.6.3.rc1.188.ga02b

[JGIT PATCH 06/10] Refactor object database access with new abstraction

[Shawn O. Pearce]

By moving object database access out of the Repository class we
start to open the door for less-traditional databases, such as
storing objects in a distributed hash table or in-memory.

This change doesn't introduce any new functionality, nor does it
fix any existing bugs.  A Repository instance still assumes that
its object database is in "$GIT_DIR/objects", and there is no way
to override that assumption.

Unlike the prior implementation, alternates are maintained as
discrete instances, rather than being inlined into the same list
as the repository's own object directory.  This cleans up that
search logic as its now more clear when we scan the alternate
and when we scan the local repository.  It also opens the door
for sharing shared repositories, e.g. if an IDE were to open
two repositories both forked from the same reference location
we could reuse the same ObjectDatabase instance between them.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 .../org/spearce/jgit/lib/RepositoryTestCase.java   |    2 -
 .../tst/org/spearce/jgit/lib/T0004_PackReader.java |    1 -
 .../src/org/spearce/jgit/lib/ObjectDatabase.java   |  354 +++++++++++++++++
 .../src/org/spearce/jgit/lib/ObjectDirectory.java  |  401 ++++++++++++++++++++
 .../src/org/spearce/jgit/lib/Repository.java       |  174 +--------
 .../org/spearce/jgit/lib/UnpackedObjectLoader.java |   22 +-
 6 files changed, 779 insertions(+), 175 deletions(-)
 create mode 100644 org.spearce.jgit/src/org/spearce/jgit/lib/ObjectDatabase.java
 create mode 100644 org.spearce.jgit/src/org/spearce/jgit/lib/ObjectDirectory.java

diff --git a/org.spearce.jgit.test/tst/org/spearce/jgit/lib/RepositoryTestCase.java b/org.spearce.jgit.test/tst/org/spearce/jgit/lib/RepositoryTestCase.java
index 588daf4..b85d3eb 100644
--- a/org.spearce.jgit.test/tst/org/spearce/jgit/lib/RepositoryTestCase.java
+++ b/org.spearce.jgit.test/tst/org/spearce/jgit/lib/RepositoryTestCase.java
@@ -297,8 +297,6 @@ copyFile(JGitTestUtil.getTestResourceFile(packs[k] + ".idx"), new File(packDir,
 
 		copyFile(JGitTestUtil.getTestResourceFile("packed-refs"), new File(trash_git,"packed-refs"));
 
-		db.scanForPacks();
-
 		fakeSystemReader.values.clear();
 		fakeSystemReader.values.put(Constants.OS_USER_NAME_KEY, Constants.OS_USER_NAME_KEY);
 		fakeSystemReader.values.put(Constants.GIT_AUTHOR_NAME_KEY, Constants.GIT_AUTHOR_NAME_KEY);
diff --git a/org.spearce.jgit.test/tst/org/spearce/jgit/lib/T0004_PackReader.java b/org.spearce.jgit.test/tst/org/spearce/jgit/lib/T0004_PackReader.java
index b9e5b49..e35e623 100644
--- a/org.spearce.jgit.test/tst/org/spearce/jgit/lib/T0004_PackReader.java
+++ b/org.spearce.jgit.test/tst/org/spearce/jgit/lib/T0004_PackReader.java
@@ -89,7 +89,6 @@ copyFile(new File(todopack, packname + ".pack"), new File(packDir,
 				packname + ".pack"));
 		copyFile(new File(todopack, packname + ".idx"), new File(packDir,
 				packname + ".idx"));
-		db.scanForPacks();
 		Tree t;
 
 		t = db
diff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/ObjectDatabase.java b/org.spearce.jgit/src/org/spearce/jgit/lib/ObjectDatabase.java
new file mode 100644
index 0000000..ed1290f
--- /dev/null
+++ b/org.spearce.jgit/src/org/spearce/jgit/lib/ObjectDatabase.java
@@ -0,0 +1,354 @@
+/*
+ * Copyright (C) 2009, Google Inc.
+ *
+ * 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 Git Development Community 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.spearce.jgit.lib;
+
+import java.io.IOException;
+import java.util.Collection;
+import java.util.concurrent.atomic.AtomicReference;
+
+/**
+ * Abstraction of arbitrary object storage.
+ * <p>
+ * An object database stores one or more Git objects, indexed by their unique
+ * {@link ObjectId}. Optionally an object database can reference one or more
+ * alternates; other ObjectDatabase instances that are searched in addition to
+ * the current database.
+ * <p>
+ * Databases are usually divided into two halves: a half that is considered to
+ * be fast to search, and a half that is considered to be slow to search. When
+ * alternates are present the fast half is fully searched (recursively through
+ * all alternates) before the slow half is considered.
+ */
+public abstract class ObjectDatabase {
+	/** Constant indicating no alternate databases exist. */
+	protected static final ObjectDatabase[] NO_ALTERNATES = {};
+
+	private final AtomicReference<ObjectDatabase[]> alternates;
+
+	/** Initialize a new database instance for access. */
+	protected ObjectDatabase() {
+		alternates = new AtomicReference<ObjectDatabase[]>();
+	}
+
+	/**
+	 * Does this database exist yet?
+	 * 
+	 * @return true if this database is already created; false if the caller
+	 *         should invoke {@link #create()} to create this database location.
+	 */
+	public boolean exists() {
+		return true;
+	}
+
+	/**
+	 * Initialize a new object database at this location.
+	 * 
+	 * @throws IOException
+	 *             the database could not be created.
+	 */
+	public void create() throws IOException {
+		// Assume no action is required.
+	}
+
+	/**
+	 * Close any resources held by this database and its active alternates.
+	 */
+	public final void close() {
+		closeSelf();
+		closeAlternates();
+	}
+
+	/**
+	 * Close any resources held by this database only; ignoring alternates.
+	 * <p>
+	 * To fully close this database and its referenced alternates, the caller
+	 * should instead invoke {@link #close()}.
+	 */
+	public void closeSelf() {
+		// Assume no action is required.
+	}
+
+	/** Fully close all loaded alternates and clear the alternate list. */
+	public final void closeAlternates() {
+		ObjectDatabase[] alt = alternates.get();
+		if (alt != null) {
+			alternates.set(null);
+			for (final ObjectDatabase d : alt) {
+				d.close();
+			}
+		}
+	}
+
+	/**
+	 * 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 final boolean hasObject(final AnyObjectId objectId) {
+		return hasObjectImpl1(objectId) || hasObjectImpl2(objectId.name());
+	}
+
+	private final boolean hasObjectImpl1(final AnyObjectId objectId) {
+		if (hasObject1(objectId)) {
+			return true;
+		}
+		for (final ObjectDatabase alt : getAlternates()) {
+			if (alt.hasObjectImpl1(objectId)) {
+				return true;
+			}
+		}
+		return false;
+	}
+
+	private final boolean hasObjectImpl2(final String objectId) {
+		if (hasObject2(objectId)) {
+			return true;
+		}
+		for (final ObjectDatabase alt : getAlternates()) {
+			if (alt.hasObjectImpl2(objectId)) {
+				return true;
+			}
+		}
+		return false;
+	}
+
+	/**
+	 * Fast half of {@link #hasObject(AnyObjectId)}.
+	 * 
+	 * @param objectId
+	 *            identity of the object to test for existence of.
+	 * @return true if the specified object is stored in this database.
+	 */
+	protected abstract boolean hasObject1(AnyObjectId objectId);
+
+	/**
+	 * Slow half of {@link #hasObject(AnyObjectId)}.
+	 * 
+	 * @param objectName
+	 *            identity of the object to test for existence of.
+	 * @return true if the specified object is stored in this database.
+	 */
+	protected boolean hasObject2(String objectName) {
+		// Assume the search took place during hasObject1.
+		return false;
+	}
+
+	/**
+	 * 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
+	 */
+	public final 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;
+	}
+
+	private ObjectLoader openObjectImpl1(final WindowCursor curs,
+			final AnyObjectId objectId) throws IOException {
+		ObjectLoader ldr;
+
+		ldr = openObject1(curs, objectId);
+		if (ldr != null) {
+			return ldr;
+		}
+		for (final ObjectDatabase alt : getAlternates()) {
+			ldr = alt.openObjectImpl1(curs, objectId);
+			if (ldr != null) {
+				return ldr;
+			}
+		}
+		return null;
+	}
+
+	private 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 ObjectDatabase alt : getAlternates()) {
+			ldr = alt.openObjectImpl2(curs, objectName, objectId);
+			if (ldr != null) {
+				return ldr;
+			}
+		}
+		return null;
+	}
+
+	/**
+	 * Fast half of {@link #openObject(WindowCursor, AnyObjectId)}.
+	 * 
+	 * @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
+	 */
+	protected abstract ObjectLoader openObject1(WindowCursor curs,
+			AnyObjectId objectId) throws IOException;
+
+	/**
+	 * Slow half of {@link #openObject(WindowCursor, AnyObjectId)}.
+	 * 
+	 * @param curs
+	 *            temporary working space associated with the calling thread.
+	 * @param objectName
+	 *            name of the object to open.
+	 * @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
+	 */
+	protected ObjectLoader openObject2(WindowCursor curs, String objectName,
+			AnyObjectId objectId) throws IOException {
+		// Assume the search took place during openObject1.
+		return null;
+	}
+
+	/**
+	 * Open the object from all packs containing it.
+	 * <p>
+	 * If any alternates are present, their packs are also considered.
+	 * 
+	 * @param out
+	 *            result collection of loaders for this object, filled with
+	 *            loaders from all packs containing specified object
+	 * @param curs
+	 *            temporary working space associated with the calling thread.
+	 * @param objectId
+	 *            id of object to search for
+	 * @throws IOException
+	 */
+	final void openObjectInAllPacks(final Collection<PackedObjectLoader> out,
+			final WindowCursor curs, final AnyObjectId objectId)
+			throws IOException {
+		openObjectInAllPacks1(out, curs, objectId);
+		for (final ObjectDatabase alt : getAlternates()) {
+			alt.openObjectInAllPacks1(out, curs, objectId);
+		}
+	}
+
+	/**
+	 * Open the object from all packs containing it.
+	 * 
+	 * @param out
+	 *            result collection of loaders for this object, filled with
+	 *            loaders from all packs containing specified object
+	 * @param curs
+	 *            temporary working space associated with the calling thread.
+	 * @param objectId
+	 *            id of object to search for
+	 * @throws IOException
+	 */
+	void openObjectInAllPacks1(Collection<PackedObjectLoader> out,
+			WindowCursor curs, AnyObjectId objectId) throws IOException {
+		// Assume no pack support
+	}
+
+	/**
+	 * Get the alternate databases known to this database.
+	 * 
+	 * @return the alternate list. Never null, but may be an empty array.
+	 */
+	public final ObjectDatabase[] getAlternates() {
+		ObjectDatabase[] r = alternates.get();
+		if (r == null) {
+			synchronized (alternates) {
+				r = alternates.get();
+				if (r == null) {
+					try {
+						r = loadAlternates();
+					} catch (IOException e) {
+						r = NO_ALTERNATES;
+					}
+					alternates.set(r);
+				}
+			}
+		}
+		return r;
+	}
+
+	/**
+	 * Load the list of alternate databases into memory.
+	 * <p>
+	 * This method is invoked by {@link #getAlternates()} if the alternate list
+	 * has not yet been populated, or if {@link #closeAlternates()} has been
+	 * called on this instance and the alternate list is needed again.
+	 * <p>
+	 * If the alternate array is empty, implementors should consider using the
+	 * constant {@link #NO_ALTERNATES}.
+	 * 
+	 * @return the alternate list for this database.
+	 * @throws IOException
+	 *             the alternate list could not be accessed. The empty alternate
+	 *             array {@link #NO_ALTERNATES} will be assumed by the caller.
+	 */
+	protected ObjectDatabase[] loadAlternates() throws IOException {
+		return NO_ALTERNATES;
+	}
+}
diff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/ObjectDirectory.java b/org.spearce.jgit/src/org/spearce/jgit/lib/ObjectDirectory.java
new file mode 100644
index 0000000..e7156c4
--- /dev/null
+++ b/org.spearce.jgit/src/org/spearce/jgit/lib/ObjectDirectory.java
@@ -0,0 +1,401 @@
+/*
+ * Copyright (C) 2009, Google Inc.
+ *
+ * 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 Git Development Community 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.spearce.jgit.lib;
+
+import java.io.BufferedReader;
+import java.io.File;
+import java.io.FileNotFoundException;
+import java.io.FileReader;
+import java.io.FilenameFilter;
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.concurrent.atomic.AtomicReference;
+
+import org.spearce.jgit.util.FS;
+
+/**
+ * Traditional file system based {@link ObjectDatabase}.
+ * <p>
+ * This is the classical object database representation for a Git repository,
+ * where objects are stored loose by hashing them into directories by their
+ * {@link ObjectId}, or are stored in compressed containers known as
+ * {@link PackFile}s.
+ */
+public class ObjectDirectory extends ObjectDatabase {
+	private static final PackFile[] NO_PACKS = {};
+
+	private final File objects;
+
+	private final File infoDirectory;
+
+	private final File packDirectory;
+
+	private final File alternatesFile;
+
+	private final AtomicReference<PackFile[]> packList;
+
+	/**
+	 * Initialize a reference to an on-disk object directory.
+	 * 
+	 * @param dir
+	 *            the location of the <code>objects</code> directory.
+	 */
+	public ObjectDirectory(final File dir) {
+		objects = dir;
+		infoDirectory = new File(objects, "info");
+		packDirectory = new File(objects, "pack");
+		alternatesFile = new File(infoDirectory, "alternates");
+
+		packList = new AtomicReference<PackFile[]>();
+	}
+
+	/**
+	 * @return the location of the <code>objects</code> directory.
+	 */
+	public final File getDirectory() {
+		return objects;
+	}
+
+	@Override
+	public boolean exists() {
+		return objects.exists();
+	}
+
+	@Override
+	public void create() throws IOException {
+		objects.mkdirs();
+		infoDirectory.mkdir();
+		packDirectory.mkdir();
+	}
+
+	@Override
+	public void closeSelf() {
+		PackFile[] packs = packList.get();
+		if (packs != null) {
+			packList.set(null);
+			for (final PackFile p : packs) {
+				p.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.
+	 */
+	public File fileFor(final AnyObjectId objectId) {
+		return fileFor(objectId.name());
+	}
+
+	private File fileFor(final String objectName) {
+		final String d = objectName.substring(0, 2);
+		final String f = objectName.substring(2);
+		return new File(new File(objects, d), f);
+	}
+
+	/**
+	 * Add a single existing pack to the list of available pack files.
+	 * 
+	 * @param pack
+	 *            path of the pack file to open.
+	 * @param idx
+	 *            path of the corresponding index file.
+	 * @throws IOException
+	 *             index file could not be opened, read, or is not recognized as
+	 *             a Git pack file index.
+	 */
+	public void openPack(final File pack, final File idx) throws IOException {
+		final String p = pack.getName();
+		final String i = idx.getName();
+
+		if (p.length() != 50 || !p.startsWith("pack-") || !p.endsWith(".pack"))
+			throw new IOException("Not a valid pack " + pack);
+
+		if (i.length() != 49 || !i.startsWith("pack-") || !i.endsWith(".idx"))
+			throw new IOException("Not a valid pack " + idx);
+
+		if (!p.substring(0, 45).equals(i.substring(0, 45)))
+			throw new IOException("Pack " + pack + "does not match index");
+
+		insertPack(new PackFile(idx, pack));
+	}
+
+	@Override
+	public String toString() {
+		return "ObjectDirectory[" + getDirectory() + "]";
+	}
+
+	@Override
+	protected boolean hasObject1(final AnyObjectId objectId) {
+		for (final PackFile p : 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);
+				continue;
+			}
+		}
+		return false;
+	}
+
+	@Override
+	protected ObjectLoader openObject1(final WindowCursor curs,
+			final AnyObjectId objectId) throws IOException {
+		for (final PackFile p : packs()) {
+			try {
+				final ObjectLoader ldr = p.get(curs, objectId);
+				if (ldr != null) {
+					return ldr;
+				}
+			} catch (IOException e) {
+				// Assume the pack is corrupted.
+				//
+				removePack(p);
+				continue;
+			}
+		}
+		return null;
+	}
+
+	@Override
+	void openObjectInAllPacks1(final Collection<PackedObjectLoader> out,
+			final WindowCursor curs, final AnyObjectId objectId)
+			throws IOException {
+		for (final PackFile p : packs()) {
+			try {
+				final PackedObjectLoader ldr = p.get(curs, objectId);
+				if (ldr != null) {
+					out.add(ldr);
+				}
+			} catch (IOException e) {
+				// Assume the pack is corrupted.
+				//
+				removePack(p);
+				continue;
+			}
+		}
+	}
+
+	@Override
+	protected boolean hasObject2(final String objectName) {
+		return fileFor(objectName).exists();
+	}
+
+	@Override
+	protected ObjectLoader openObject2(final WindowCursor curs,
+			final String objectName, final AnyObjectId objectId)
+			throws IOException {
+		try {
+			return new UnpackedObjectLoader(fileFor(objectName), objectId);
+		} catch (FileNotFoundException noFile) {
+			return null;
+		}
+	}
+
+	private void insertPack(final PackFile pf) {
+		PackFile[] o, n;
+		do {
+			o = packs();
+			n = new PackFile[1 + o.length];
+			n[0] = pf;
+			System.arraycopy(o, 0, n, 1, o.length);
+		} while (!packList.compareAndSet(o, n));
+	}
+
+	private void removePack(final PackFile deadPack) {
+		PackFile[] o, n;
+		do {
+			o = packList.get();
+			if (o == null || !inList(o, deadPack)) {
+				break;
+
+			} else if (o.length == 1) {
+				n = NO_PACKS;
+
+			} else {
+				n = new PackFile[o.length - 1];
+				int j = 0;
+				for (final PackFile p : o) {
+					if (p != deadPack) {
+						n[j++] = p;
+					}
+				}
+			}
+		} while (!packList.compareAndSet(o, n));
+		deadPack.close();
+	}
+
+	private static boolean inList(final PackFile[] list, final PackFile pack) {
+		for (final PackFile p : list) {
+			if (p == pack) {
+				return true;
+			}
+		}
+		return false;
+	}
+
+	private PackFile[] packs() {
+		PackFile[] r = packList.get();
+		if (r == null) {
+			r = scanPacks(r);
+		}
+		return r;
+	}
+
+	private PackFile[] scanPacks(final PackFile[] original) {
+		synchronized (packList) {
+			PackFile[] o, n;
+			do {
+				o = packList.get();
+				if (o != original) {
+					// Another thread did the scan for us, while we
+					// were blocked on the monitor above.
+					//
+					return o;
+				}
+				n = scanPacksImpl(o != null ? o : NO_PACKS);
+			} while (!packList.compareAndSet(o, n));
+			return n;
+		}
+	}
+
+	private PackFile[] scanPacksImpl(final PackFile[] old) {
+		final Map<String, PackFile> forReuse = reuseMap(old);
+		final String[] idxList = listPackIdx();
+		final List<PackFile> list = new ArrayList<PackFile>(idxList.length);
+		for (final String indexName : idxList) {
+			final String base = indexName.substring(0, indexName.length() - 4);
+			final String packName = base + ".pack";
+
+			final PackFile oldPack = forReuse.remove(packName);
+			if (oldPack != null) {
+				list.add(oldPack);
+				continue;
+			}
+
+			final File packFile = new File(packDirectory, packName);
+			if (!packFile.isFile()) {
+				// Sometimes C Git's HTTP fetch transport leaves a
+				// .idx file behind and does not download the .pack.
+				// We have to skip over such useless indexes.
+				//
+				continue;
+			}
+
+			final File idxFile = new File(packDirectory, indexName);
+			list.add(new PackFile(idxFile, packFile));
+		}
+
+		for (final PackFile p : forReuse.values()) {
+			p.close();
+		}
+
+		if (list.isEmpty()) {
+			return NO_PACKS;
+		}
+		final PackFile[] r = list.toArray(new PackFile[list.size()]);
+		Arrays.sort(r, PackFile.SORT);
+		return r;
+	}
+
+	private static Map<String, PackFile> reuseMap(final PackFile[] old) {
+		final Map<String, PackFile> forReuse = new HashMap<String, PackFile>();
+		for (final PackFile p : old) {
+			final PackFile prior = forReuse.put(p.getPackFile().getName(), p);
+			if (prior != null) {
+				// This should never occur. It should be impossible for us
+				// to have two pack files with the same name, as all of them
+				// came out of the same directory. If it does, we promised to
+				// close any PackFiles we did not reuse, so close the one we
+				// just evicted out of the reuse map.
+				//
+				prior.close();
+			}
+		}
+		return forReuse;
+	}
+
+	private String[] listPackIdx() {
+		final String[] idxList = packDirectory.list(new FilenameFilter() {
+			public boolean accept(final File baseDir, final String n) {
+				// Must match "pack-[0-9a-f]{40}.idx" to be an index.
+				return n.length() == 49 && n.endsWith(".idx")
+						&& n.startsWith("pack-");
+			}
+		});
+		return idxList != null ? idxList : new String[0];
+	}
+
+	@Override
+	protected ObjectDatabase[] loadAlternates() throws IOException {
+		final BufferedReader br = open(alternatesFile);
+		final List<ObjectDirectory> l = new ArrayList<ObjectDirectory>(4);
+		try {
+			String line;
+			while ((line = br.readLine()) != null) {
+				l.add(new ObjectDirectory(FS.resolve(objects, line)));
+			}
+		} finally {
+			br.close();
+		}
+
+		if (l.isEmpty()) {
+			return NO_ALTERNATES;
+		}
+		return l.toArray(new ObjectDirectory[l.size()]);
+	}
+
+	private static BufferedReader open(final File f)
+			throws FileNotFoundException {
+		return new BufferedReader(new FileReader(f));
+	}
+}
diff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/Repository.java b/org.spearce.jgit/src/org/spearce/jgit/lib/Repository.java
index cfd92b8..f7bacf3 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/lib/Repository.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/lib/Repository.java
@@ -43,10 +43,8 @@
 import java.io.File;
 import java.io.FileNotFoundException;
 import java.io.FileReader;
-import java.io.FilenameFilter;
 import java.io.IOException;
 import java.util.ArrayList;
-import java.util.Arrays;
 import java.util.Collection;
 import java.util.Collections;
 import java.util.HashMap;
@@ -93,9 +91,7 @@
 
 	private final RefDatabase refs;
 
-	private File[] objectDirectoryList;
-
-	private PackFile[] packFileList;
+	private final ObjectDirectory objectDatabase;
 
 	private GitIndex index;
 
@@ -113,21 +109,11 @@
 	 */
 	public Repository(final File d) throws IOException {
 		gitDir = d.getAbsoluteFile();
-		try {
-			objectDirectoryList = readObjectsDirs(
-					FS.resolve(gitDir, "objects"), new ArrayList<File>())
-					.toArray(new File[0]);
-		} catch (IOException e) {
-			IOException ex = new IOException("Cannot find all object dirs for " + gitDir);
-			ex.initCause(e);
-			throw ex;
-		}
 		refs = new RefDatabase(this);
-		packFileList = new PackFile[0];
+		objectDatabase = new ObjectDirectory(FS.resolve(gitDir, "objects"));
 		config = new RepositoryConfig(this);
 
-		final boolean isExisting = objectDirectoryList[0].exists();
-		if (isExisting) {
+		if (objectDatabase.exists()) {
 			getConfig().load();
 			final String repositoryFormatVersion = getConfig().getString(
 					"core", null, "repositoryFormatVersion");
@@ -138,25 +124,6 @@ public Repository(final File d) throws IOException {
 		} else {
 			getConfig().create();
 		}
-		if (isExisting)
-			scanForPacks();
-	}
-
-	private static Collection<File> readObjectsDirs(File objectsDir,
-			Collection<File> ret) throws IOException {
-		ret.add(objectsDir);
-		final File altFile = FS.resolve(objectsDir, "info/alternates");
-		if (altFile.exists()) {
-			BufferedReader ar = new BufferedReader(new FileReader(altFile));
-			try {
-				for (String alt=ar.readLine(); alt!=null; alt=ar.readLine()) {
-					readObjectsDirs(FS.resolve(objectsDir, alt), ret);
-				}
-			} finally {
-				ar.close();
-			}
-		}
-		return ret;
 	}
 
 	/**
@@ -173,10 +140,7 @@ public synchronized void create() throws IOException {
 
 		gitDir.mkdirs();
 		refs.create();
-
-		objectDirectoryList[0].mkdirs();
-		new File(objectDirectoryList[0], "pack").mkdir();
-		new File(objectDirectoryList[0], "info").mkdir();
+		objectDatabase.create();
 
 		new File(gitDir, "branches").mkdir();
 		new File(gitDir, "remotes").mkdir();
@@ -187,14 +151,6 @@ public synchronized void create() throws IOException {
 		getConfig().save();
 	}
 
-	private synchronized File[] objectsDirs(){
-		return objectDirectoryList;
-	}
-
-	private synchronized PackFile[] packs(){
-		return packFileList;
-	}
-
 	/**
 	 * @return GIT_DIR
 	 */
@@ -206,7 +162,7 @@ public File getDirectory() {
 	 * @return the directory containing the objects owned by this repository.
 	 */
 	public File getObjectsDirectory() {
-		return objectsDirs()[0];
+		return objectDatabase.getDirectory();
 	}
 
 	/**
@@ -227,16 +183,7 @@ public RepositoryConfig getConfig() {
 	 * @return suggested file name
 	 */
 	public File toFile(final AnyObjectId objectId) {
-		final String n = objectId.name();
-		String d=n.substring(0, 2);
-		String f=n.substring(2);
-		final File[] objectsDirs = objectsDirs();
-		for (File objectsDir : objectsDirs) {
-			File ret = new File(new File(objectsDir, d), f);
-			if (ret.exists())
-				return ret;
-		}
-		return new File(new File(objectsDirs[0], d), f);
+		return objectDatabase.fileFor(objectId);
 	}
 
 	/**
@@ -245,20 +192,7 @@ public File toFile(final AnyObjectId objectId) {
 	 *         known shared repositories.
 	 */
 	public boolean hasObject(final AnyObjectId objectId) {
-		final PackFile[] packs = packs();
-		int k = packs.length;
-		while (k > 0) {
-			try {
-				if (packs[--k].hasObject(objectId))
-					return true;
-			} catch (IOException e) {
-				// Assume that means the pack is invalid, and such
-				// packs are treated as though they are empty.
-				//
-				continue;
-			}
-		}
-		return toFile(objectId).isFile();
+		return objectDatabase.hasObject(objectId);
 	}
 
 	/**
@@ -291,18 +225,7 @@ public ObjectLoader openObject(final AnyObjectId id)
 	 */
 	public ObjectLoader openObject(final WindowCursor curs, final AnyObjectId id)
 			throws IOException {
-		final PackFile[] packs = packs();
-		int k = packs.length;
-		while (k > 0) {
-			final ObjectLoader ol = packs[--k].get(curs, id);
-			if (ol != null)
-				return ol;
-		}
-		try {
-			return new UnpackedObjectLoader(this, id);
-		} catch (FileNotFoundException fnfe) {
-			return null;
-		}
+		return objectDatabase.openObject(curs, id);
 	}
 
 	/**
@@ -339,11 +262,7 @@ public ObjectLoader openObject(final WindowCursor curs, final AnyObjectId id)
 	void openObjectInAllPacks(final AnyObjectId objectId,
 			final Collection<PackedObjectLoader> resultLoaders,
 			final WindowCursor curs) throws IOException {
-		for (PackFile pack : packs()) {
-			final PackedObjectLoader loader = pack.get(curs, objectId);
-			if (loader != null)
-				resultLoaders.add(loader);
-		}
+		objectDatabase.openObjectInAllPacks(resultLoaders, curs, objectId);
 	}
 
 	/**
@@ -772,13 +691,7 @@ private ObjectId resolveSimple(final String revstr) throws IOException {
 	 * Close all resources used by this repository
 	 */
 	public void close() {
-		closePacks();
-	}
-
-	synchronized void closePacks() {
-		for (int k = packFileList.length - 1; k >= 0; k--)
-			packFileList[k].close();
-		packFileList = new PackFile[0];
+		objectDatabase.close();
 	}
 
 	/**
@@ -793,72 +706,7 @@ synchronized void closePacks() {
 	 *             a Git pack file index.
 	 */
 	public void openPack(final File pack, final File idx) throws IOException {
-		final String p = pack.getName();
-		final String i = idx.getName();
-		if (p.length() != 50 || !p.startsWith("pack-") || !p.endsWith(".pack"))
-		    throw new IllegalArgumentException("Not a valid pack " + pack);
-		if (i.length() != 49 || !i.startsWith("pack-") || !i.endsWith(".idx"))
-		    throw new IllegalArgumentException("Not a valid pack " + idx);
-		if (!p.substring(0,45).equals(i.substring(0,45)))
-			throw new IllegalArgumentException("Pack " + pack
-					+ "does not match index " + idx);
-
-		synchronized (this) {
-			final PackFile[] cur = packFileList;
-			final PackFile[] arr = new PackFile[cur.length + 1];
-			System.arraycopy(cur, 0, arr, 1, cur.length);
-			arr[0] = new PackFile(idx, pack);
-			packFileList = arr;
-		}
-	}
-
-	/**
-	 * Scan the object dirs, including alternates for packs
-	 * to use.
-	 */
-	public void scanForPacks() {
-		final ArrayList<PackFile> p = new ArrayList<PackFile>();
-		p.addAll(Arrays.asList(packs()));
-		for (final File d : objectsDirs())
-			scanForPacks(new File(d, "pack"), p);
-		final PackFile[] arr = new PackFile[p.size()];
-		p.toArray(arr);
-		Arrays.sort(arr, PackFile.SORT);
-		synchronized (this) {
-			packFileList = arr;
-		}
-	}
-
-	private void scanForPacks(final File packDir, Collection<PackFile> packList) {
-		final String[] idxList = packDir.list(new FilenameFilter() {
-			public boolean accept(final File baseDir, final String n) {
-				// Must match "pack-[0-9a-f]{40}.idx" to be an index.
-				return n.length() == 49 && n.endsWith(".idx")
-						&& n.startsWith("pack-");
-			}
-		});
-		if (idxList != null) {
-			SCAN: for (final String indexName : idxList) {
-				final String n = indexName.substring(0, indexName.length() - 4);
-				final File idxFile = new File(packDir, n + ".idx");
-				final File packFile = new File(packDir, n + ".pack");
-
-				if (!packFile.isFile()) {
-					// Sometimes C Git's http fetch transport leaves a
-					// .idx file behind and does not download the .pack.
-					// We have to skip over such useless indexes.
-					//
-					continue;
-				}
-
-				for (final PackFile p : packList) {
-					if (packFile.equals(p.getPackFile()))
-						continue SCAN;
-				}
-
-				packList.add(new PackFile(idxFile, packFile));
-			}
-		}
+		objectDatabase.openPack(pack, idx);
 	}
 
     /**
diff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/UnpackedObjectLoader.java b/org.spearce.jgit/src/org/spearce/jgit/lib/UnpackedObjectLoader.java
index b086821..7552b42 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/lib/UnpackedObjectLoader.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/lib/UnpackedObjectLoader.java
@@ -38,6 +38,7 @@
 
 package org.spearce.jgit.lib;
 
+import java.io.File;
 import java.io.FileInputStream;
 import java.io.FileNotFoundException;
 import java.io.IOException;
@@ -60,22 +61,25 @@
 	private final byte[] bytes;
 
 	/**
-	 * Construct an ObjectLoader for the specified SHA-1
+	 * Construct an ObjectLoader to read from the file.
 	 *
-	 * @param db
-	 *            repository
+	 * @param path
+	 *            location of the loose object to read.
 	 * @param id
-	 *            SHA-1
+	 *            expected identity of the object being loaded, if known.
+	 * @throws FileNotFoundException
+	 *             the loose object file does not exist.
 	 * @throws IOException
+	 *             the loose object file exists, but is corrupt.
 	 */
-	public UnpackedObjectLoader(final Repository db, final AnyObjectId id)
+	public UnpackedObjectLoader(final File path, final AnyObjectId id)
 			throws IOException {
-		this(readCompressed(db, id), id);
+		this(readCompressed(path), id);
 	}
 
-	private static byte[] readCompressed(final Repository db,
-			final AnyObjectId id) throws FileNotFoundException, IOException {
-		final FileInputStream in = new FileInputStream(db.toFile(id));
+	private static byte[] readCompressed(final File path)
+			throws FileNotFoundException, IOException {
+		final FileInputStream in = new FileInputStream(path);
 		try {
 			final byte[] compressed = new byte[(int) in.getChannel().size()];
 			NB.readFully(in, compressed, 0, compressed.length);
-- 
1.6.3.rc1.188.ga02b

[JGIT PATCH 07/10] Rescan packs if a pack is modified in-place (part 1)

[Shawn O. Pearce]

If a pack file is modified in place it usually means that some other
process has repacked this repository, but the contents of this one
pack remained the same, but the object offsets may be different due
to different pack creation settings.  In such cases we can no longer
use the existing PacKFile or PackIndex instances and instead we need
to create new ones.

This isn't a full solution to the problem.  It is possible for an
application to obtain a PackedObjectLoader, see the PackFile get
closed due to pressure on the WindowCache, then fail when the pack
is reopened due to the pack being recreated on disk.  PackWriter
is very susceptible to this, as it caches PackedObjectLoaders for
a long time before it uses them to copy data out of an existing
pack file.

This particular solution only catches the failure where we had
opened the index a long time ago, closed the pack, and are now
opening it again in order to construct a PackedObjectLoader for
the caller.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 .../src/org/spearce/jgit/lib/ObjectDirectory.java  |   66 +++++++++++++------
 .../src/org/spearce/jgit/lib/PackFile.java         |   20 +++++-
 2 files changed, 63 insertions(+), 23 deletions(-)

diff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/ObjectDirectory.java b/org.spearce.jgit/src/org/spearce/jgit/lib/ObjectDirectory.java
index e7156c4..36f221e 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/lib/ObjectDirectory.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/lib/ObjectDirectory.java
@@ -51,6 +51,7 @@
 import java.util.Map;
 import java.util.concurrent.atomic.AtomicReference;
 
+import org.spearce.jgit.errors.PackMismatchException;
 import org.spearce.jgit.util.FS;
 
 /**
@@ -190,38 +191,53 @@ protected boolean hasObject1(final AnyObjectId objectId) {
 	@Override
 	protected ObjectLoader openObject1(final WindowCursor curs,
 			final AnyObjectId objectId) throws IOException {
-		for (final PackFile p : packs()) {
-			try {
-				final ObjectLoader ldr = p.get(curs, objectId);
-				if (ldr != null) {
-					return ldr;
+		PackFile[] pList = packs();
+		SEARCH: for (;;) {
+			for (final PackFile p : pList) {
+				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);
 				}
-			} catch (IOException e) {
-				// Assume the pack is corrupted.
-				//
-				removePack(p);
-				continue;
 			}
+			return null;
 		}
-		return null;
 	}
 
 	@Override
 	void openObjectInAllPacks1(final Collection<PackedObjectLoader> out,
 			final WindowCursor curs, final AnyObjectId objectId)
 			throws IOException {
-		for (final PackFile p : packs()) {
-			try {
-				final PackedObjectLoader ldr = p.get(curs, objectId);
-				if (ldr != null) {
-					out.add(ldr);
+		PackFile[] pList = packs();
+		SEARCH: for (;;) {
+			for (final PackFile p : pList) {
+				try {
+					final PackedObjectLoader ldr = p.get(curs, objectId);
+					if (ldr != null) {
+						out.add(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);
 				}
-			} catch (IOException e) {
-				// Assume the pack is corrupted.
-				//
-				removePack(p);
-				continue;
 			}
+			break SEARCH;
 		}
 	}
 
@@ -350,6 +366,14 @@ synchronized (packList) {
 	private static Map<String, PackFile> reuseMap(final PackFile[] old) {
 		final Map<String, PackFile> forReuse = new HashMap<String, PackFile>();
 		for (final PackFile p : old) {
+			if (p.invalid()) {
+				// The pack instance is corrupted, and cannot be safely used
+				// again. Do not include it in our reuse map.
+				//
+				p.close();
+				continue;
+			}
+
 			final PackFile prior = forReuse.put(p.getPackFile().getName(), p);
 			if (prior != null) {
 				// This should never occur. It should be impossible for us
diff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/PackFile.java b/org.spearce.jgit/src/org/spearce/jgit/lib/PackFile.java
index bda4843..74ffef8 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/lib/PackFile.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/lib/PackFile.java
@@ -73,6 +73,8 @@ public int compare(final PackFile a, final PackFile b) {
 
 	private int packLastModified;
 
+	private volatile boolean invalid;
+
 	private PackIndex loadedIdx;
 
 	private PackReverseIndex reverseIdx;
@@ -91,14 +93,24 @@ public PackFile(final File idxFile, final File packFile) {
 		pack = new WindowedFile(packFile) {
 			@Override
 			protected void onOpen() throws IOException {
-				onOpenPack();
+				try {
+					onOpenPack();
+				} catch (IOException e) {
+					invalid = true;
+					throw e;
+				}
 			}
 		};
 	}
 
 	private synchronized PackIndex idx() throws IOException {
 		if (loadedIdx == null) {
-			loadedIdx = PackIndex.open(idxFile);
+			try {
+				loadedIdx = PackIndex.open(idxFile);
+			} catch (IOException e) {
+				invalid = true;
+				throw e;
+			}
 		}
 		return loadedIdx;
 	}
@@ -267,6 +279,10 @@ boolean supportsFastCopyRawData() throws IOException {
 		return idx().hasCRC32Support();
 	}
 
+	boolean invalid() {
+		return invalid;
+	}
+
 	private void onOpenPack() throws IOException {
 		final PackIndex idx = idx();
 		final WindowCursor curs = new WindowCursor();
-- 
1.6.3.rc1.188.ga02b

[JGIT PATCH 08/10] Scan for new packs if GIT_DIR/objects/pack has been modified

[Shawn O. Pearce]

If GIT_DIR/objects/pack directory has changed then one or more packs
may have been added to the directory, or removed from it, due to a
concurrent application performing a repack.  In such cases we need
to reload the list of available packs before conducting a search.

As packs are infrequently created or modified relative to searches,
we only scan for new packs if our search through existing packs has
turned up no results.

Transports that create packs locally through IndexPack automatically
register their new PackFile, so even those cases will typically not
require scanning the object directory again.

As misses are fairly common during IndexPack (due to its automatic
collision detection logic) we avoid scanning for packs unless the
GIT_DIR/objects/pack directory has been modified since the last
scan of it.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 .../src/org/spearce/jgit/lib/ObjectDatabase.java   |   15 ++++++++++++++-
 .../src/org/spearce/jgit/lib/ObjectDirectory.java  |   13 +++++++++++++
 2 files changed, 27 insertions(+), 1 deletions(-)

diff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/ObjectDatabase.java b/org.spearce.jgit/src/org/spearce/jgit/lib/ObjectDatabase.java
index ed1290f..ec228a1 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/lib/ObjectDatabase.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/lib/ObjectDatabase.java
@@ -137,7 +137,7 @@ private final boolean hasObjectImpl1(final AnyObjectId objectId) {
 				return true;
 			}
 		}
-		return false;
+		return tryAgain1() && hasObject1(objectId);
 	}
 
 	private final boolean hasObjectImpl2(final String objectId) {
@@ -216,6 +216,12 @@ private ObjectLoader openObjectImpl1(final WindowCursor curs,
 				return ldr;
 			}
 		}
+		if (tryAgain1()) {
+			ldr = openObject1(curs, objectId);
+			if (ldr != null) {
+				return ldr;
+			}
+		}
 		return null;
 	}
 
@@ -311,6 +317,13 @@ void openObjectInAllPacks1(Collection<PackedObjectLoader> out,
 	}
 
 	/**
+	 * @return true if the fast-half search should be tried again.
+	 */
+	protected boolean tryAgain1() {
+		return false;
+	}
+
+	/**
 	 * Get the alternate databases known to this database.
 	 * 
 	 * @return the alternate list. Never null, but may be an empty array.
diff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/ObjectDirectory.java b/org.spearce.jgit/src/org/spearce/jgit/lib/ObjectDirectory.java
index 36f221e..d29c63a 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/lib/ObjectDirectory.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/lib/ObjectDirectory.java
@@ -75,6 +75,8 @@
 
 	private final AtomicReference<PackFile[]> packList;
 
+	private volatile long packDirectoryLastModified;
+
 	/**
 	 * Initialize a reference to an on-disk object directory.
 	 * 
@@ -257,6 +259,16 @@ protected ObjectLoader openObject2(final WindowCursor curs,
 		}
 	}
 
+	@Override
+	protected boolean tryAgain1() {
+		final PackFile[] old = packList.get();
+		if (packDirectoryLastModified <= packDirectory.lastModified()) {
+			scanPacks(old);
+			return true;
+		}
+		return false;
+	}
+
 	private void insertPack(final PackFile pf) {
 		PackFile[] o, n;
 		do {
@@ -389,6 +401,7 @@ synchronized (packList) {
 	}
 
 	private String[] listPackIdx() {
+		packDirectoryLastModified = packDirectory.lastModified();
 		final String[] idxList = packDirectory.list(new FilenameFilter() {
 			public boolean accept(final File baseDir, final String n) {
 				// Must match "pack-[0-9a-f]{40}.idx" to be an index.
-- 
1.6.3.rc1.188.ga02b

[JGIT PATCH 09/10] Add test cases for loading new (or replaced) pack files

[Shawn O. Pearce]

Originally-by: Robin Rosenberg <robin.rosenberg@dewire.com>
Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 .../org/spearce/jgit/lib/ConcurrentRepackTest.java |  206 ++++++++++++++++++++
 1 files changed, 206 insertions(+), 0 deletions(-)
 create mode 100644 org.spearce.jgit.test/tst/org/spearce/jgit/lib/ConcurrentRepackTest.java

diff --git a/org.spearce.jgit.test/tst/org/spearce/jgit/lib/ConcurrentRepackTest.java b/org.spearce.jgit.test/tst/org/spearce/jgit/lib/ConcurrentRepackTest.java
new file mode 100644
index 0000000..6eb368c
--- /dev/null
+++ b/org.spearce.jgit.test/tst/org/spearce/jgit/lib/ConcurrentRepackTest.java
@@ -0,0 +1,206 @@
+/*
+ * Copyright (C) 2009, Robin Rosenberg <robin.rosenberg@dewire.com>
+ * Copyright (C) 2009, Google Inc.
+ *
+ * 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 Git Development Community 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.spearce.jgit.lib;
+
+import java.io.File;
+import java.io.FileOutputStream;
+import java.io.IOException;
+
+import org.spearce.jgit.errors.IncorrectObjectTypeException;
+import org.spearce.jgit.errors.MissingObjectException;
+import org.spearce.jgit.revwalk.RevObject;
+import org.spearce.jgit.revwalk.RevWalk;
+
+public class ConcurrentRepackTest extends RepositoryTestCase {
+	public void setUp() throws Exception {
+		WindowCacheConfig windowCacheConfig = new WindowCacheConfig();
+		windowCacheConfig.setPackedGitOpenFiles(1);
+		WindowCache.reconfigure(windowCacheConfig);
+		super.setUp();
+	}
+
+	protected void tearDown() throws Exception {
+		super.tearDown();
+		WindowCacheConfig windowCacheConfig = new WindowCacheConfig();
+		WindowCache.reconfigure(windowCacheConfig);
+	}
+
+	public void testObjectInNewPack() throws IncorrectObjectTypeException,
+			IOException {
+		// Create a new object in a new pack, and test that it is present.
+		//
+		final Repository eden = createNewEmptyRepo();
+		final RevObject o1 = writeBlob(eden, "o1");
+		pack(eden, o1);
+		assertEquals(o1.name(), parse(o1).name());
+	}
+
+	public void testObjectMovedToNewPack1()
+			throws IncorrectObjectTypeException, IOException {
+		// Create an object and pack it. Then remove that pack and put the
+		// object into a different pack file, with some other object. We
+		// still should be able to access the objects.
+		//
+		final Repository eden = createNewEmptyRepo();
+		final RevObject o1 = writeBlob(eden, "o1");
+		final File[] out1 = pack(eden, o1);
+		assertEquals(o1.name(), parse(o1).name());
+
+		final RevObject o2 = writeBlob(eden, "o2");
+		pack(eden, o2, o1);
+
+		// Force close, and then delete, the old pack.
+		//
+		whackCache();
+		delete(out1);
+
+		// Now here is the interesting thing. Will git figure the new
+		// object exists in the new pack, and not the old one.
+		//
+		assertEquals(o2.name(), parse(o2).name());
+		assertEquals(o1.name(), parse(o1).name());
+	}
+
+	public void testObjectMovedWithinPack()
+			throws IncorrectObjectTypeException, IOException {
+		// Create an object and pack it.
+		//
+		final Repository eden = createNewEmptyRepo();
+		final RevObject o1 = writeBlob(eden, "o1");
+		final File[] out1 = pack(eden, o1);
+		assertEquals(o1.name(), parse(o1).name());
+
+		// Force close the old pack.
+		//
+		whackCache();
+
+		// Now overwrite the old pack in place. This method of creating a
+		// different pack under the same file name is partially broken. We
+		// should also have a different file name because the list of objects
+		// within the pack has been modified.
+		//
+		final RevObject o2 = writeBlob(eden, "o2");
+		final PackWriter pw = new PackWriter(eden, NullProgressMonitor.INSTANCE);
+		pw.addObject(o2);
+		pw.addObject(o1);
+		write(out1, pw);
+
+		// Try the old name, then the new name. The old name should cause the
+		// pack to reload when it opens and the index and pack mismatch.
+		//
+		assertEquals(o1.name(), parse(o1).name());
+		assertEquals(o2.name(), parse(o2).name());
+	}
+
+	private static void whackCache() {
+		final WindowCacheConfig config = new WindowCacheConfig();
+
+		config.setPackedGitOpenFiles(0);
+		WindowCache.reconfigure(config);
+
+		config.setPackedGitOpenFiles(1);
+		WindowCache.reconfigure(config);
+	}
+
+	private RevObject parse(final AnyObjectId id)
+			throws MissingObjectException, IOException {
+		return new RevWalk(db).parseAny(id);
+	}
+
+	private File[] pack(final Repository src, final RevObject... list)
+			throws IOException {
+		final PackWriter pw = new PackWriter(src, NullProgressMonitor.INSTANCE);
+		for (final RevObject o : list) {
+			pw.addObject(o);
+		}
+
+		final ObjectId name = pw.computeName();
+		final File packFile = fullPackFileName(name, ".pack");
+		final File idxFile = fullPackFileName(name, ".idx");
+		final File[] files = new File[] { packFile, idxFile };
+		write(files, pw);
+		return files;
+	}
+
+	private static void write(final File[] files, final PackWriter pw)
+			throws IOException {
+		FileOutputStream out;
+
+		out = new FileOutputStream(files[0]);
+		try {
+			pw.writePack(out);
+		} finally {
+			out.close();
+		}
+
+		out = new FileOutputStream(files[1]);
+		try {
+			pw.writeIndex(out);
+		} finally {
+			out.close();
+		}
+	}
+
+	private static void delete(final File[] list) {
+		for (final File f : list) {
+			f.delete();
+			assertFalse(f + " was removed", f.exists());
+		}
+	}
+
+	private File fullPackFileName(final ObjectId name, final String suffix) {
+		final File packdir = new File(db.getObjectsDirectory(), "pack");
+		return new File(packdir, "pack-" + name.name() + suffix);
+	}
+
+	private RevObject writeBlob(final Repository repo, final String data)
+			throws IOException {
+		final RevWalk revWalk = new RevWalk(repo);
+		final byte[] bytes = data.getBytes(Constants.CHARACTER_ENCODING);
+		final ObjectWriter ow = new ObjectWriter(repo);
+		final ObjectId id = ow.writeBlob(bytes);
+		try {
+			parse(id);
+			fail("Object " + id.name() + " should not exist in test repository");
+		} catch (MissingObjectException e) {
+			// Ok
+		}
+		return revWalk.lookupBlob(id);
+	}
+}
-- 
1.6.3.rc1.188.ga02b

[JGIT PATCH 10/10] BROKEN TEST: ObjectLoader stays valid across repacks

[Shawn O. Pearce]

This doesn't doesn't work.

What we are trying to verify is that an ObjectLoader remains valid
if the underlying storage for the object has moved, such as when a
repository is repacked, the old pack was deleted, and the object is
now in the new pack.
---
 .../org/spearce/jgit/lib/ConcurrentRepackTest.java |   46 ++++++++++++++++++++
 1 files changed, 46 insertions(+), 0 deletions(-)

diff --git a/org.spearce.jgit.test/tst/org/spearce/jgit/lib/ConcurrentRepackTest.java b/org.spearce.jgit.test/tst/org/spearce/jgit/lib/ConcurrentRepackTest.java
index 6eb368c..b1b1af2 100644
--- a/org.spearce.jgit.test/tst/org/spearce/jgit/lib/ConcurrentRepackTest.java
+++ b/org.spearce.jgit.test/tst/org/spearce/jgit/lib/ConcurrentRepackTest.java
@@ -41,6 +41,7 @@
 import java.io.File;
 import java.io.FileOutputStream;
 import java.io.IOException;
+import java.util.Arrays;
 
 import org.spearce.jgit.errors.IncorrectObjectTypeException;
 import org.spearce.jgit.errors.MissingObjectException;
@@ -128,6 +129,51 @@ public void testObjectMovedWithinPack()
 		assertEquals(o2.name(), parse(o2).name());
 	}
 
+	public void testObjectMovedToNewPack2()
+			throws IncorrectObjectTypeException, IOException {
+		// Create an object and pack it. Then remove that pack and put the
+		// object into a different pack file, with some other object. We
+		// still should be able to access the objects.
+		//
+		final Repository eden = createNewEmptyRepo();
+		final RevObject o1 = writeBlob(eden, "o1");
+		final File[] out1 = pack(eden, o1);
+		assertEquals(o1.name(), parse(o1).name());
+
+		final ObjectLoader load1 = db.openBlob(o1);
+		assertNotNull(load1);
+
+		final RevObject o2 = writeBlob(eden, "o2");
+		pack(eden, o2, o1);
+
+		// Force close, and then delete, the old pack.
+		//
+		whackCache();
+		delete(out1);
+
+		// Now here is the interesting thing... can the loader we made
+		// earlier still resolve the object, even though its underlying
+		// pack is gone, but the object still exists.
+		//
+		final ObjectLoader load2 = db.openBlob(o1);
+		assertNotNull(load2);
+		assertNotSame(load1, load2);
+
+		// Currently load1.getCachedBytes NPEs due to the openCount == 1,
+		// but fd == null. Aside from the underlying pack being gone, the
+		// WindowCache didn't reset the openCount to 0 when it closed the
+		// file during the mass cache eviction. Thus we never tried to do
+		// an subsequent open.
+		//
+		final byte[] data2 = load2.getCachedBytes();
+		final byte[] data1 = load1.getCachedBytes();
+		assertNotNull(data2);
+		assertNotNull(data1);
+		assertNotSame(data1, data2); // cache should be per-pack, not per object
+		assertTrue(Arrays.equals(data1, data2));
+		assertEquals(load2.getType(), load1.getType());
+	}
+
 	private static void whackCache() {
 		final WindowCacheConfig config = new WindowCacheConfig();
 
-- 
1.6.3.rc1.188.ga02b

Re: [JGIT PATCH 10/10] BROKEN TEST: ObjectLoader stays valid across repacks

[Robin Rosenberg]

tisdag 21 april 2009 03:21:12 skrev "Shawn O. Pearce" <spearce@spearce.org>:
> This doesn't doesn't work.
> 
> What we are trying to verify is that an ObjectLoader remains valid
> if the underlying storage for the object has moved, such as when a
> repository is repacked, the old pack was deleted, and the object is
> now in the new pack.

So, I had an idea and started hacking and suddenly the supposedly ok cases
started crashing like this.

org.spearce.jgit.errors.MissingObjectException: Missing unknown 4a75554761c96be80602c05145d1ef41c77e1b72
	at org.spearce.jgit.revwalk.RevWalk.parseAny(RevWalk.java:704)
	at org.spearce.jgit.lib.ConcurrentRepackTest.parse(ConcurrentRepackTest.java:189)
	at org.spearce.jgit.lib.ConcurrentRepackTest.testObjectMovedWithinPack(ConcurrentRepackTest.java:129)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25)
	at java.lang.reflect.Method.invoke(Method.java:597)
	at junit.framework.TestCase.runTest(TestCase.java:164)
	at junit.framework.TestCase.runBare(TestCase.java:130)
	at junit.framework.TestResult$1.protect(TestResult.java:106)
	at junit.framework.TestResult.runProtected(TestResult.java:124)
	at junit.framework.TestResult.run(TestResult.java:109)
	at junit.framework.TestCase.run(TestCase.java:120)
	at org.eclipse.jdt.internal.junit.runner.junit3.JUnit3TestReference.run(JUnit3TestReference.java:130)
	at org.eclipse.jdt.internal.junit.runner.TestExecution.run(TestExecution.java:38)
	at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:460)
	at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:673)
	at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.run(RemoteTestRunner.java:386)
	at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.main(RemoteTestRunner.java:196)

After pruning the other changes, it still crashes , even with mvn clean test

HashCode /home/me/SW/EGIT.contrib/org.spearce.jgit.test/trash/trash1240354367548.0/.git/objects/pack/pack-34be9032ac282b11fa9babdc2b2a93ca996c9c2f.pack = 327983069
HashCode /home/me/SW/EGIT.contrib/org.spearce.jgit.test/trash/trash1240354367548.0/.git/objects/pack/pack-df2982f284bbabb6bdb59ee3fcc6eb0983e20371.pack = 458811222
HashCode /home/me/SW/EGIT.contrib/org.spearce.jgit.test/trash/trash1240354367548.0/.git/objects/pack/pack-9fb5b411fe6dfa89cc2e6b89d2bd8e5de02b5745.pack = 331392325
HashCode /home/me/SW/EGIT.contrib/org.spearce.jgit.test/trash/trash1240354367548.0/.git/objects/pack/pack-546ff360fe3488adb20860ce3436a2d6373d2796.pack = 547821429
HashCode /home/me/SW/EGIT.contrib/org.spearce.jgit.test/trash/trash1240354367548.0/.git/objects/pack/pack-e6d07037cbcf13376308a0a995d1fa48f8f76aaa.pack = 536578194
HashCode /home/me/SW/EGIT.contrib/org.spearce.jgit.test/trash/trash1240354367548.0/.git/objects/pack/pack-3280af9c07ee18a87705ef50b0cc4cd20266cf12.pack = 885722359
HashCode /home/me/SW/EGIT.contrib/org.spearce.jgit.test/trash/trash1240354367548.0/.git/objects/pack/pack-146aac1f6a11aed8e06af7d9f5cc1b4b8beedb36.pack = 830203467

The hash codes printed are the same everytime it crashes, removing the invalid flags will create these codes and the test succeeds.

HashCode /home/me/SW/EGIT.contrib/org.spearce.jgit.test/trash/trash1240354568171.0/.git/objects/pack/pack-34be9032ac282b11fa9babdc2b2a93ca996c9c2f.pack = 327983069
HashCode /home/me/SW/EGIT.contrib/org.spearce.jgit.test/trash/trash1240354568171.0/.git/objects/pack/pack-df2982f284bbabb6bdb59ee3fcc6eb0983e20371.pack = 458811222
HashCode /home/me/SW/EGIT.contrib/org.spearce.jgit.test/trash/trash1240354568171.0/.git/objects/pack/pack-9fb5b411fe6dfa89cc2e6b89d2bd8e5de02b5745.pack = 331392325
HashCode /home/me/SW/EGIT.contrib/org.spearce.jgit.test/trash/trash1240354568171.0/.git/objects/pack/pack-546ff360fe3488adb20860ce3436a2d6373d2796.pack = 547821429
HashCode /home/me/SW/EGIT.contrib/org.spearce.jgit.test/trash/trash1240354568171.0/.git/objects/pack/pack-e6d07037cbcf13376308a0a995d1fa48f8f76aaa.pack = 536578194
HashCode /home/me/SW/EGIT.contrib/org.spearce.jgit.test/trash/trash1240354568171.0/.git/objects/pack/pack-3280af9c07ee18a87705ef50b0cc4cd20266cf12.pack = 885722359
HashCode /home/me/SW/EGIT.contrib/org.spearce.jgit.test/trash/trash1240354568171.0/.git/objects/pack/pack-146aac1f6a11aed8e06af7d9f5cc1b4b8beedb36.pack = 830203467

One cannot obviously assume they are the same, but the numbers might be a lead to why it crashes here. Looks
like as hash collision and a failure of the "equals" part to distinguish different WindowedFile's.

-- robin

diff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/WindowedFile.java b/org.spearce.jgit/src/org/spearce/jgit/lib/WindowedFile.java
index 9293eb9..f9e1991 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/lib/WindowedFile.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/lib/WindowedFile.java
@@ -80,6 +80,8 @@
 	/** Total number of windows actively in the associated cache. */
 	int openCount;
 
+	boolean invalid;
+
 	/**
 	 * Open a file for reading through window caching.
 	 * 
@@ -93,6 +95,10 @@ public WindowedFile(final File file) {
 		// value in WindowCache.hash(), without doing the multiply there.
 		//
 		hash = System.identityHashCode(this) * 31;
+		System.out.print("HashCode ");
+		System.out.print(file.getPath());
+		System.out.print(" = ");
+		System.out.println(hash);
 		length = Long.MAX_VALUE;
 	}
 

Re: [JGIT PATCH 10/10] BROKEN TEST: ObjectLoader stays valid across repacks

[Shawn O. Pearce]

Robin Rosenberg <robin.rosenberg.lists@dewire.com> wrote:
> So, I had an idea and started hacking and suddenly the supposedly ok cases
> started crashing like this.
> 
> org.spearce.jgit.errors.MissingObjectException: Missing unknown 4a75554761c96be80602c05145d1ef41c77e1b72

You broke it!  :-)

> The hash codes printed are the same everytime it crashes, removing
> the invalid flags will create these codes and the test succeeds.

I am not surprised.  The hash codes are derived from the system
identity hash code for the object, which are assigned using a rather
deterministic algorithm.  Given the same sequence of tests through
the JVM and the same heap state, you are likely going to have the
same hash code for the same objects every run.

Actually, I think you missed it, but the two sets of hash codes
you printed here in the email are identical.
 
> One cannot obviously assume they are the same, but the numbers might be a lead to why it crashes here. Looks
> like as hash collision and a failure of the "equals" part to distinguish different WindowedFile's.

Nope, that's not it.  We never use .equals() on WindowedFile,
and we never use that hash field you were printing as any part
of an equality computation.  That hash field is mixed with the
offset of a window to hash it into the WindowCache.  We *always*
use reference equality ("foo.wp == wp") to test a WindowedFile.

> diff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/WindowedFile.java b/org.spearce.jgit/src/org/spearce/jgit/lib/WindowedFile.java
> index 9293eb9..f9e1991 100644
> --- a/org.spearce.jgit/src/org/spearce/jgit/lib/WindowedFile.java
> +++ b/org.spearce.jgit/src/org/spearce/jgit/lib/WindowedFile.java
> @@ -80,6 +80,8 @@
>  	/** Total number of windows actively in the associated cache. */
>  	int openCount;
>  
> +	boolean invalid;

This shadowed the "invalid" field in PackFile, causing the anonymous
inner class on l.93-103 that subclasses WindowedFile to write to
this new field, while PackFile still read from its own field.

If you had called this "invalid2", or marked it private, you
wouldn't have seen the test failure.


FWIW, I've decided to merge PackFile and WindowedFile together.
There is no point in keeping them separate anymore.  We used to
use WindowedFile for both the .pack and the .idx, but long ago
you showed it was faster to load the entire .idx into memory in
the PackIndex structure.  And this split is causing confusion,
like the one you just stepped in.

-- 
Shawn.

Re: [JGIT PATCH 10/10] BROKEN TEST: ObjectLoader stays valid across repacks

[Shawn O. Pearce]

Robin Rosenberg <robin.rosenberg.lists@dewire.com> wrote:
> tisdag 21 april 2009 03:21:12 skrev "Shawn O. Pearce" <spearce@spearce.org>:
> > This doesn't doesn't work.
> > 
> > What we are trying to verify is that an ObjectLoader remains valid
> > if the underlying storage for the object has moved, such as when a
> > repository is repacked, the old pack was deleted, and the object is
> > now in the new pack.
> 
> So, I had an idea and started hacking [...]

Yea, I also have an idea.  I'm working up a patch series that should
fix this bug.  I'll post it later this evening once its complete
and passes tests.

-- 
Shawn.

[JGIT PATCH v2 09/10] Add test cases for loading new (or replaced) pack files

[Shawn O. Pearce]

Originally-by: Robin Rosenberg <robin.rosenberg@dewire.com>
Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---

 Matches with v2 08/10.  Tests take a few seconds longer now as
 we have to delay at least 1 second on most filesystems; longer if
 their modification timestamp granularity is larger than 1 second.

 .../org/spearce/jgit/lib/ConcurrentRepackTest.java |  222 ++++++++++++++++++++
 1 files changed, 222 insertions(+), 0 deletions(-)
 create mode 100644 org.spearce.jgit.test/tst/org/spearce/jgit/lib/ConcurrentRepackTest.java

diff --git a/org.spearce.jgit.test/tst/org/spearce/jgit/lib/ConcurrentRepackTest.java b/org.spearce.jgit.test/tst/org/spearce/jgit/lib/ConcurrentRepackTest.java
new file mode 100644
index 0000000..825fbb8
--- /dev/null
+++ b/org.spearce.jgit.test/tst/org/spearce/jgit/lib/ConcurrentRepackTest.java
@@ -0,0 +1,222 @@
+/*
+ * Copyright (C) 2009, Robin Rosenberg <robin.rosenberg@dewire.com>
+ * Copyright (C) 2009, Google Inc.
+ *
+ * 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 Git Development Community 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.spearce.jgit.lib;
+
+import java.io.File;
+import java.io.FileOutputStream;
+import java.io.IOException;
+
+import org.spearce.jgit.errors.IncorrectObjectTypeException;
+import org.spearce.jgit.errors.MissingObjectException;
+import org.spearce.jgit.revwalk.RevObject;
+import org.spearce.jgit.revwalk.RevWalk;
+
+public class ConcurrentRepackTest extends RepositoryTestCase {
+	public void setUp() throws Exception {
+		WindowCacheConfig windowCacheConfig = new WindowCacheConfig();
+		windowCacheConfig.setPackedGitOpenFiles(1);
+		WindowCache.reconfigure(windowCacheConfig);
+		super.setUp();
+	}
+
+	protected void tearDown() throws Exception {
+		super.tearDown();
+		WindowCacheConfig windowCacheConfig = new WindowCacheConfig();
+		WindowCache.reconfigure(windowCacheConfig);
+	}
+
+	public void testObjectInNewPack() throws IncorrectObjectTypeException,
+			IOException {
+		// Create a new object in a new pack, and test that it is present.
+		//
+		final Repository eden = createNewEmptyRepo();
+		final RevObject o1 = writeBlob(eden, "o1");
+		pack(eden, o1);
+		assertEquals(o1.name(), parse(o1).name());
+	}
+
+	public void testObjectMovedToNewPack1()
+			throws IncorrectObjectTypeException, IOException {
+		// Create an object and pack it. Then remove that pack and put the
+		// object into a different pack file, with some other object. We
+		// still should be able to access the objects.
+		//
+		final Repository eden = createNewEmptyRepo();
+		final RevObject o1 = writeBlob(eden, "o1");
+		final File[] out1 = pack(eden, o1);
+		assertEquals(o1.name(), parse(o1).name());
+
+		final RevObject o2 = writeBlob(eden, "o2");
+		pack(eden, o2, o1);
+
+		// Force close, and then delete, the old pack.
+		//
+		whackCache();
+		delete(out1);
+
+		// Now here is the interesting thing. Will git figure the new
+		// object exists in the new pack, and not the old one.
+		//
+		assertEquals(o2.name(), parse(o2).name());
+		assertEquals(o1.name(), parse(o1).name());
+	}
+
+	public void testObjectMovedWithinPack()
+			throws IncorrectObjectTypeException, IOException {
+		// Create an object and pack it.
+		//
+		final Repository eden = createNewEmptyRepo();
+		final RevObject o1 = writeBlob(eden, "o1");
+		final File[] out1 = pack(eden, o1);
+		assertEquals(o1.name(), parse(o1).name());
+
+		// Force close the old pack.
+		//
+		whackCache();
+
+		// Now overwrite the old pack in place. This method of creating a
+		// different pack under the same file name is partially broken. We
+		// should also have a different file name because the list of objects
+		// within the pack has been modified.
+		//
+		final RevObject o2 = writeBlob(eden, "o2");
+		final PackWriter pw = new PackWriter(eden, NullProgressMonitor.INSTANCE);
+		pw.addObject(o2);
+		pw.addObject(o1);
+		write(out1, pw);
+
+		// Try the old name, then the new name. The old name should cause the
+		// pack to reload when it opens and the index and pack mismatch.
+		//
+		assertEquals(o1.name(), parse(o1).name());
+		assertEquals(o2.name(), parse(o2).name());
+	}
+
+	private static void whackCache() {
+		final WindowCacheConfig config = new WindowCacheConfig();
+
+		config.setPackedGitOpenFiles(0);
+		WindowCache.reconfigure(config);
+
+		config.setPackedGitOpenFiles(1);
+		WindowCache.reconfigure(config);
+	}
+
+	private RevObject parse(final AnyObjectId id)
+			throws MissingObjectException, IOException {
+		return new RevWalk(db).parseAny(id);
+	}
+
+	private File[] pack(final Repository src, final RevObject... list)
+			throws IOException {
+		final PackWriter pw = new PackWriter(src, NullProgressMonitor.INSTANCE);
+		for (final RevObject o : list) {
+			pw.addObject(o);
+		}
+
+		final ObjectId name = pw.computeName();
+		final File packFile = fullPackFileName(name, ".pack");
+		final File idxFile = fullPackFileName(name, ".idx");
+		final File[] files = new File[] { packFile, idxFile };
+		write(files, pw);
+		return files;
+	}
+
+	private static void write(final File[] files, final PackWriter pw)
+			throws IOException {
+		final long begin = files[0].getParentFile().lastModified();
+		FileOutputStream out;
+
+		out = new FileOutputStream(files[0]);
+		try {
+			pw.writePack(out);
+		} finally {
+			out.close();
+		}
+
+		out = new FileOutputStream(files[1]);
+		try {
+			pw.writeIndex(out);
+		} finally {
+			out.close();
+		}
+
+		touch(begin, files[0].getParentFile());
+	}
+
+	private static void delete(final File[] list) {
+		final long begin = list[0].getParentFile().lastModified();
+		for (final File f : list) {
+			f.delete();
+			assertFalse(f + " was removed", f.exists());
+		}
+		touch(begin, list[0].getParentFile());
+	}
+
+	private static void touch(final long begin, final File dir) {
+		while (begin >= dir.lastModified()) {
+			try {
+				Thread.sleep(25);
+			} catch (InterruptedException ie) {
+				//
+			}
+			dir.setLastModified(System.currentTimeMillis());
+		}
+	}
+
+	private File fullPackFileName(final ObjectId name, final String suffix) {
+		final File packdir = new File(db.getObjectsDirectory(), "pack");
+		return new File(packdir, "pack-" + name.name() + suffix);
+	}
+
+	private RevObject writeBlob(final Repository repo, final String data)
+			throws IOException {
+		final RevWalk revWalk = new RevWalk(repo);
+		final byte[] bytes = data.getBytes(Constants.CHARACTER_ENCODING);
+		final ObjectWriter ow = new ObjectWriter(repo);
+		final ObjectId id = ow.writeBlob(bytes);
+		try {
+			parse(id);
+			fail("Object " + id.name() + " should not exist in test repository");
+		} catch (MissingObjectException e) {
+			// Ok
+		}
+		return revWalk.lookupBlob(id);
+	}
+}
-- 
1.6.3.rc1.188.ga02b

Re: [JGIT PATCH 08/10] Scan for new packs if GIT_DIR/objects/pack has been modified

[Shawn O. Pearce]

"Shawn O. Pearce" <spearce@spearce.org> wrote:
> If GIT_DIR/objects/pack directory has changed then one or more packs
...
> +	@Override
> +	protected boolean tryAgain1() {
> +		final PackFile[] old = packList.get();
> +		if (packDirectoryLastModified <= packDirectory.lastModified()) {

Sadly we needed to use <= here to avoid racy-git cases.  Worse,
unlike with the DirCache we don't have another file system inode
we can use to compare timestamps against.

Consequently, I just realized my commit message isn't entirely
accurate.  In reality we rescan the #F*@!*! directory every time
we have a miss, because the time stamp hasn't changed.

I can't think of another method to use here.  I'd love to avoid
the rescan on a miss if the aren't any new packs in the directory,
but I just don't see how we can do that and avoid the racy-git case
at the same time.

We could try to fudge it by saying < here, but the test cases in
9/10 would then require a Thread.sleep(2) or something to force
enough time to pass for the directory modification time to advance.

I think in real applications, its "good enough" to allow this amount
of fudging.  We shouldn't see more than one repack per repository
per FS clock tick.  And if we do, the admin should be shot on sight.

git-repack is careful to ensure the new pack is linked into the
directory before the old packs are unlinked, so we really shouldn't
ever scan and miss a pack.

-- 
Shawn.

[JGIT PATCH v2 08/10] Scan for new packs if GIT_DIR/objects/pack has been modified

[Shawn O. Pearce]

If GIT_DIR/objects/pack directory has changed then one or more packs
may have been added to the directory, or removed from it, due to a
concurrent application performing a repack.  In such cases we need
to reload the list of available packs before conducting a search.

As packs are infrequently created or modified relative to searches,
we only scan for new packs if our search through existing packs has
turned up no results.

Transports that create packs locally through IndexPack automatically
register their new PackFile, so even those cases will typically not
require scanning the object directory again.

As misses are fairly common during IndexPack (due to its automatic
collision detection logic) we avoid scanning for packs unless the
GIT_DIR/objects/pack directory has been modified since the last
scan of it.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---

 "Shawn O. Pearce" <spearce@spearce.org> wrote:
 > 
 > We could try to fudge it by saying < here, [...]
 > I think in real applications, its "good enough" to allow this amount
 > of fudging.

 And this version makes it so.  The interdiff is just <= changed to <.

 .../src/org/spearce/jgit/lib/ObjectDatabase.java   |   15 ++++++++++++++-
 .../src/org/spearce/jgit/lib/ObjectDirectory.java  |   13 +++++++++++++
 2 files changed, 27 insertions(+), 1 deletions(-)

diff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/ObjectDatabase.java b/org.spearce.jgit/src/org/spearce/jgit/lib/ObjectDatabase.java
index ed1290f..ec228a1 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/lib/ObjectDatabase.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/lib/ObjectDatabase.java
@@ -137,7 +137,7 @@ private final boolean hasObjectImpl1(final AnyObjectId objectId) {
 				return true;
 			}
 		}
-		return false;
+		return tryAgain1() && hasObject1(objectId);
 	}
 
 	private final boolean hasObjectImpl2(final String objectId) {
@@ -216,6 +216,12 @@ private ObjectLoader openObjectImpl1(final WindowCursor curs,
 				return ldr;
 			}
 		}
+		if (tryAgain1()) {
+			ldr = openObject1(curs, objectId);
+			if (ldr != null) {
+				return ldr;
+			}
+		}
 		return null;
 	}
 
@@ -311,6 +317,13 @@ void openObjectInAllPacks1(Collection<PackedObjectLoader> out,
 	}
 
 	/**
+	 * @return true if the fast-half search should be tried again.
+	 */
+	protected boolean tryAgain1() {
+		return false;
+	}
+
+	/**
 	 * Get the alternate databases known to this database.
 	 * 
 	 * @return the alternate list. Never null, but may be an empty array.
diff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/ObjectDirectory.java b/org.spearce.jgit/src/org/spearce/jgit/lib/ObjectDirectory.java
index 36f221e..6ba0180 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/lib/ObjectDirectory.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/lib/ObjectDirectory.java
@@ -75,6 +75,8 @@
 
 	private final AtomicReference<PackFile[]> packList;
 
+	private volatile long packDirectoryLastModified;
+
 	/**
 	 * Initialize a reference to an on-disk object directory.
 	 * 
@@ -257,6 +259,16 @@ protected ObjectLoader openObject2(final WindowCursor curs,
 		}
 	}
 
+	@Override
+	protected boolean tryAgain1() {
+		final PackFile[] old = packList.get();
+		if (packDirectoryLastModified < packDirectory.lastModified()) {
+			scanPacks(old);
+			return true;
+		}
+		return false;
+	}
+
 	private void insertPack(final PackFile pf) {
 		PackFile[] o, n;
 		do {
@@ -389,6 +401,7 @@ synchronized (packList) {
 	}
 
 	private String[] listPackIdx() {
+		packDirectoryLastModified = packDirectory.lastModified();
 		final String[] idxList = packDirectory.list(new FilenameFilter() {
 			public boolean accept(final File baseDir, final String n) {
 				// Must match "pack-[0-9a-f]{40}.idx" to be an index.
-- 
1.6.3.rc1.188.ga02b