[JGIT PATCH] Do not store ObjectId subclasses in RefDatabase caches

From: Shawn O. Pearce <spearce@spearce.org>
Date: Feb 18, 2009, 03:36 UTC
Message-ID: <1234928211-30408-1-git-send-email-spearce@spearce.org>

We cannot permit an ObjectId subclass, such as RevCommit, to be cached inside of the RefDatabase cache. Instead of toObjectId() use copy() to ensure we get exactly an ObjectId for the new value, as that is the instance we will cache in the RefDatabase.

Caching the ObjectId subclass in RefDatabase will cause any later cached read of the ref to return that RevCommit, confusing all application code using "ref.getObjectId().equals(...)" due to the overridden definition of equals(ObjectId).

ReceivePack was bitten by this, sometimes reporting "invalid old id" to clients if the ref was last updated using a RevCommit instead of an ObjectId. It also caused the history of that repository to be pinned in memory as RevCommit instances, all of them reachable from the RefDatabase cache of the last updated Ref.

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

 I know I've sent a bunch of patches, but this one is fairly
 critical, and is simple enough.  Applications that are able
 to cache their Repository for a while can get nailed by this
 ObjectId.equals() quirk.  Gerrit2 is getting hit for example.

 I think we need to revisit RevObject extending ObjectId.  But
 doing that is much more invasive than 1 line of code, and we
 would probably want this change to use copy() anyway.  So my
 suggestion is, we include this now, and I'll try to split the
 RevObject tree off ObjectId later this week to fix the root
 cause of the problem.

 .../tst/org/spearce/jgit/lib/RefUpdateTest.java    |   18 ++++++++++++++++++
 .../src/org/spearce/jgit/lib/RefUpdate.java        |    2 +-
 2 files changed, 19 insertions(+), 1 deletions(-)

diff --git a/org.spearce.jgit.test/tst/org/spearce/jgit/lib/RefUpdateTest.java b/org.spearce.jgit.test/tst/org/spearce/jgit/lib/RefUpdateTest.java
index 12f9ada..55d7441 100644
--- a/org.spearce.jgit.test/tst/org/spearce/jgit/lib/RefUpdateTest.java
+++ b/org.spearce.jgit.test/tst/org/spearce/jgit/lib/RefUpdateTest.java
@@ -43,6 +43,7 @@
 import java.util.Map.Entry;
 
 import org.spearce.jgit.lib.RefUpdate.Result;
+import org.spearce.jgit.revwalk.RevCommit;
 
 public class RefUpdateTest extends RepositoryTestCase {
 
@@ -64,6 +65,23 @@ private void delete(final RefUpdate ref, final Result expected,
 		assertEquals(!removed, db.getAllRefs().containsKey(ref.getName()));
 	}
 
+	public void testNoCacheObjectIdSubclass() throws IOException {
+		final String newRef = "refs/heads/abc";
+		final RefUpdate ru = updateRef(newRef);
+		final RevCommit newid = new RevCommit(ru.getNewObjectId()) {
+			// empty
+		};
+		ru.setNewObjectId(newid);
+		ru.update();
+		final Ref r = db.getAllRefs().get(newRef);
+		assertNotNull(r);
+		assertEquals(newRef, r.getName());
+		assertNotNull(r.getObjectId());
+		assertNotSame(newid, r.getObjectId());
+		assertSame(ObjectId.class, r.getObjectId().getClass());
+		assertEquals(newid.copy(), r.getObjectId());
+	}
+
 	public void testLooseDelete() throws IOException {
 		final String newRef = "refs/heads/abc";
 		RefUpdate ref = updateRef(newRef);
diff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/RefUpdate.java b/org.spearce.jgit/src/org/spearce/jgit/lib/RefUpdate.java
index 7ad2bab..32095c4 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/lib/RefUpdate.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/lib/RefUpdate.java
@@ -197,7 +197,7 @@ public ObjectId getNewObjectId() {
 	 *            the new value.
 	 */
 	public void setNewObjectId(final AnyObjectId id) {
-		newValue = id.toObjectId();
+		newValue = id.copy();
 	}
 
 	/**

-- 
1.6.2.rc1.209.gfe624