{"thread":{"id":"17868","subject":"[JGIT PATCH] Do not store ObjectId subclasses in RefDatabase caches","startedAt":"2009-02-18T03:36:51Z","lastAt":"2009-02-18T03:36:51Z","messageCount":1,"participants":["Shawn O. Pearce"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"105266","messageId":"1234928211-30408-1-git-send-email-spearce@spearce.org","threadId":"17868","inReplyTo":null,"subject":"[JGIT PATCH] Do not store ObjectId subclasses in RefDatabase caches","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2009-02-18T03:36:51Z","receivedAt":"2009-02-18T03:36:51Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"We cannot permit an ObjectId subclass, such as RevCommit, to be\ncached inside of the RefDatabase cache.  Instead of toObjectId()\nuse copy() to ensure we get exactly an ObjectId for the new value,\nas that is the instance we will cache in the RefDatabase.\n\nCaching the ObjectId subclass in RefDatabase will cause any later\ncached read of the ref to return that RevCommit, confusing all\napplication code using \"ref.getObjectId().equals(...)\" due to the\noverridden definition of equals(ObjectId).\n\nReceivePack was bitten by this, sometimes reporting \"invalid old id\"\nto clients if the ref was last updated using a RevCommit instead\nof an ObjectId.  It also caused the history of that repository to\nbe pinned in memory as RevCommit instances, all of them reachable\nfrom the RefDatabase cache of the last updated Ref.\n\nSigned-off-by: Shawn O. Pearce <spearce@spearce.org>\n---\n\n I know I've sent a bunch of patches, but this one is fairly\n critical, and is simple enough.  Applications that are able\n to cache their Repository for a while can get nailed by this\n ObjectId.equals() quirk.  Gerrit2 is getting hit for example.\n\n I think we need to revisit RevObject extending ObjectId.  But\n doing that is much more invasive than 1 line of code, and we\n would probably want this change to use copy() anyway.  So my\n suggestion is, we include this now, and I'll try to split the\n RevObject tree off ObjectId later this week to fix the root\n cause of the problem.\n\n .../tst/org/spearce/jgit/lib/RefUpdateTest.java    |   18 ++++++++++++++++++\n .../src/org/spearce/jgit/lib/RefUpdate.java        |    2 +-\n 2 files changed, 19 insertions(+), 1 deletions(-)\n\ndiff --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\nindex 12f9ada..55d7441 100644\n--- a/org.spearce.jgit.test/tst/org/spearce/jgit/lib/RefUpdateTest.java\n+++ b/org.spearce.jgit.test/tst/org/spearce/jgit/lib/RefUpdateTest.java\n@@ -43,6 +43,7 @@\n import java.util.Map.Entry;\n \n import org.spearce.jgit.lib.RefUpdate.Result;\n+import org.spearce.jgit.revwalk.RevCommit;\n \n public class RefUpdateTest extends RepositoryTestCase {\n \n@@ -64,6 +65,23 @@ private void delete(final RefUpdate ref, final Result expected,\n \t\tassertEquals(!removed, db.getAllRefs().containsKey(ref.getName()));\n \t}\n \n+\tpublic void testNoCacheObjectIdSubclass() throws IOException {\n+\t\tfinal String newRef = \"refs/heads/abc\";\n+\t\tfinal RefUpdate ru = updateRef(newRef);\n+\t\tfinal RevCommit newid = new RevCommit(ru.getNewObjectId()) {\n+\t\t\t// empty\n+\t\t};\n+\t\tru.setNewObjectId(newid);\n+\t\tru.update();\n+\t\tfinal Ref r = db.getAllRefs().get(newRef);\n+\t\tassertNotNull(r);\n+\t\tassertEquals(newRef, r.getName());\n+\t\tassertNotNull(r.getObjectId());\n+\t\tassertNotSame(newid, r.getObjectId());\n+\t\tassertSame(ObjectId.class, r.getObjectId().getClass());\n+\t\tassertEquals(newid.copy(), r.getObjectId());\n+\t}\n+\n \tpublic void testLooseDelete() throws IOException {\n \t\tfinal String newRef = \"refs/heads/abc\";\n \t\tRefUpdate ref = updateRef(newRef);\ndiff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/RefUpdate.java b/org.spearce.jgit/src/org/spearce/jgit/lib/RefUpdate.java\nindex 7ad2bab..32095c4 100644\n--- a/org.spearce.jgit/src/org/spearce/jgit/lib/RefUpdate.java\n+++ b/org.spearce.jgit/src/org/spearce/jgit/lib/RefUpdate.java\n@@ -197,7 +197,7 @@ public ObjectId getNewObjectId() {\n \t *            the new value.\n \t */\n \tpublic void setNewObjectId(final AnyObjectId id) {\n-\t\tnewValue = id.toObjectId();\n+\t\tnewValue = id.copy();\n \t}\n \n \t/**\n-- \n1.6.2.rc1.209.gfe624\n"}]}