{"thread":{"id":"20854","subject":"[JGIT PATCH v2 3/3] Fix DirCache.findEntry to work on an empty cache","startedAt":"2009-09-04T16:22:43Z","lastAt":"2009-09-04T16:22:45Z","messageCount":3,"participants":["Shawn O. Pearce"],"isPatch":true,"patchVersion":2,"patchTotal":3},"messages":[{"id":"122445","messageId":"1252081365-2335-1-git-send-email-spearce@spearce.org","threadId":"20854","inReplyTo":null,"subject":"[JGIT PATCH v2 1/3] Allow RefUpdate.setExpectedOldObjectId to accept RevCommit","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2009-09-04T16:22:43Z","receivedAt":"2009-09-04T16:22:43Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"RevCommit overrides .equals() such that it only implements a\nreference equality test.  If the expected old ObjectId was set\nby the application to a RevCommit instance, it would always fail,\nresulting in LOCK_FAILURE.  Instead use AnyObject.equals() to compare\nthe value, ignoring the possibly overloaded equals in RevCommit.\n\nSigned-off-by: Shawn O. Pearce <sop@google.com>\n---\n .../tst/org/spearce/jgit/lib/RefUpdateTest.java    |   52 ++++++++++++++++++++\n .../src/org/spearce/jgit/lib/RefUpdate.java        |    2 +-\n 2 files changed, 53 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 800c0a4..a8ccf43 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@@ -45,6 +45,7 @@\n \n import org.spearce.jgit.lib.RefUpdate.Result;\n import org.spearce.jgit.revwalk.RevCommit;\n+import org.spearce.jgit.revwalk.RevWalk;\n \n public class RefUpdateTest extends RepositoryTestCase {\n \n@@ -397,6 +398,57 @@ public void testUpdateRefLockFailureWrongOldValue() throws IOException {\n \t}\n \n \t/**\n+\t * Try modify a ref forward, fast forward, checking old value first\n+\t *\n+\t * @throws IOException\n+\t */\n+\tpublic void testUpdateRefForwardWithCheck1() throws IOException {\n+\t\tObjectId ppid = db.resolve(\"refs/heads/master^\");\n+\t\tObjectId pid = db.resolve(\"refs/heads/master\");\n+\n+\t\tRefUpdate updateRef = db.updateRef(\"refs/heads/master\");\n+\t\tupdateRef.setNewObjectId(ppid);\n+\t\tupdateRef.setForceUpdate(true);\n+\t\tResult update = updateRef.update();\n+\t\tassertEquals(Result.FORCED, update);\n+\t\tassertEquals(ppid, db.resolve(\"refs/heads/master\"));\n+\n+\t\t// real test\n+\t\tRefUpdate updateRef2 = db.updateRef(\"refs/heads/master\");\n+\t\tupdateRef2.setExpectedOldObjectId(ppid);\n+\t\tupdateRef2.setNewObjectId(pid);\n+\t\tResult update2 = updateRef2.update();\n+\t\tassertEquals(Result.FAST_FORWARD, update2);\n+\t\tassertEquals(pid, db.resolve(\"refs/heads/master\"));\n+\t}\n+\n+\t/**\n+\t * Try modify a ref forward, fast forward, checking old commit first\n+\t *\n+\t * @throws IOException\n+\t */\n+\tpublic void testUpdateRefForwardWithCheck2() throws IOException {\n+\t\tObjectId ppid = db.resolve(\"refs/heads/master^\");\n+\t\tObjectId pid = db.resolve(\"refs/heads/master\");\n+\n+\t\tRefUpdate updateRef = db.updateRef(\"refs/heads/master\");\n+\t\tupdateRef.setNewObjectId(ppid);\n+\t\tupdateRef.setForceUpdate(true);\n+\t\tResult update = updateRef.update();\n+\t\tassertEquals(Result.FORCED, update);\n+\t\tassertEquals(ppid, db.resolve(\"refs/heads/master\"));\n+\n+\t\t// real test\n+\t\tRevCommit old = new RevWalk(db).parseCommit(ppid);\n+\t\tRefUpdate updateRef2 = db.updateRef(\"refs/heads/master\");\n+\t\tupdateRef2.setExpectedOldObjectId(old);\n+\t\tupdateRef2.setNewObjectId(pid);\n+\t\tResult update2 = updateRef2.update();\n+\t\tassertEquals(Result.FAST_FORWARD, update2);\n+\t\tassertEquals(pid, db.resolve(\"refs/heads/master\"));\n+\t}\n+\n+\t/**\n \t * Try modify a ref that is locked\n \t *\n \t * @throws IOException\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 69399ec..8dffed2 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@@ -466,7 +466,7 @@ private Result updateImpl(final RevWalk walk, final Store store)\n \t\t\tif (expValue != null) {\n \t\t\t\tfinal ObjectId o;\n \t\t\t\to = oldValue != null ? oldValue : ObjectId.zeroId();\n-\t\t\t\tif (!expValue.equals(o))\n+\t\t\t\tif (!AnyObjectId.equals(expValue, o))\n \t\t\t\t\treturn Result.LOCK_FAILURE;\n \t\t\t}\n \t\t\tif (oldValue == null)\n-- \n1.6.4.2.395.ge3d52\n"},{"id":"122446","messageId":"1252081365-2335-2-git-send-email-spearce@spearce.org","threadId":"20854","inReplyTo":"1252081365-2335-1-git-send-email-spearce@spearce.org","subject":"[JGIT PATCH v2 2/3] Work around Sun javac compiler bug in RefUpdate","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2009-09-04T16:22:44Z","receivedAt":"2009-09-04T16:22:44Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Sun's javac, version 1.5.0_06 and also a Google internal fork of\nOpenJDK 1.6.0 both apparently miscompile the for loop which is\nlooking for a conflicting ref name in the existing set of refs for\nthis repository.\n\nDebugging this code showed the control flow to return LOCK_FAILURE\nwhen startsWith returned false, which is highly illogical and the\nexact opposite of what we have written here.\n\nOpenJDK 1.6.0's javap tool was unable to disassemble the compiled\nmethod.  Instead it simply failed to produce anything about\nupdateImpl.  So my remark about the code being compiled wrong\nis only a guess based on how I observed the behavior, and not by\nactually studying the resulting instructions.\n\nEclipse 3.4.2's JDT appears to have compiled the updateImpl method\ncorrectly, and produces a working executable.  But this is a much\nless common compiler to build Java libraries with.\n\nRobin was able to successfully compile the ancestor version of this\ncode with both javac 1.5.0_19 and also 1.6.0_16, but not everyone\nbuilding JGit has these versions installed.\n\nThis refactoring to extract the name conflicting test out into its\nown method appears to work around the Sun javac bug I observed above,\nand the resulting class works correctly with any compiler tested.\nThe code is also more clear, so its a gain either way.\n\nSigned-off-by: Shawn O. Pearce <sop@google.com>\n---\n .../src/org/spearce/jgit/lib/RefUpdate.java        |   26 +++++++++++++-------\n 1 files changed, 17 insertions(+), 9 deletions(-)\n\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 8dffed2..8226e10 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@@ -449,15 +449,8 @@ private Result updateImpl(final RevWalk walk, final Store store)\n \t\tRevObject newObj;\n \t\tRevObject oldObj;\n \n-\t\tint lastSlash = getName().lastIndexOf('/');\n-\t\tif (lastSlash > 0)\n-\t\t\tif (db.getRepository().getRef(getName().substring(0, lastSlash)) != null)\n-\t\t\t\treturn Result.LOCK_FAILURE;\n-\t\tString rName = getName() + \"/\";\n-\t\tfor (Ref r : db.getAllRefs().values()) {\n-\t\t\tif (r.getName().startsWith(rName))\n-\t\t\t\treturn Result.LOCK_FAILURE;\n-\t\t}\n+\t\tif (isNameConflicting())\n+\t\t\treturn Result.LOCK_FAILURE;\n \t\tlock = new LockFile(looseFile);\n \t\tif (!lock.lock())\n \t\t\treturn Result.LOCK_FAILURE;\n@@ -490,6 +483,21 @@ private Result updateImpl(final RevWalk walk, final Store store)\n \t\t}\n \t}\n \n+\tprivate boolean isNameConflicting() throws IOException {\n+\t\tfinal String myName = getName();\n+\t\tfinal int lastSlash = myName.lastIndexOf('/');\n+\t\tif (lastSlash > 0)\n+\t\t\tif (db.getRepository().getRef(myName.substring(0, lastSlash)) != null)\n+\t\t\t\treturn true;\n+\n+\t\tfinal String rName = myName + \"/\";\n+\t\tfor (Ref r : db.getAllRefs().values()) {\n+\t\t\tif (r.getName().startsWith(rName))\n+\t\t\t\treturn true;\n+\t\t}\n+\t\treturn false;\n+\t}\n+\n \tprivate static RevObject safeParse(final RevWalk rw, final AnyObjectId id)\n \t\t\tthrows IOException {\n \t\ttry {\n-- \n1.6.4.2.395.ge3d52\n"},{"id":"122444","messageId":"1252081365-2335-3-git-send-email-spearce@spearce.org","threadId":"20854","inReplyTo":"1252081365-2335-2-git-send-email-spearce@spearce.org","subject":"[JGIT PATCH v2 3/3] Fix DirCache.findEntry to work on an empty cache","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2009-09-04T16:22:45Z","receivedAt":"2009-09-04T16:22:45Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"If the cache has no entries, we want to return -1 rather than throw\nArrayIndexOutOfBoundsException.  This binary search loop was stolen\nfrom some other code which contained a test before the loop to see if\nthe collection was empty or not, but we failed to include that here.\n\nFlipping the loop around to a standard while loop ensures we test\nthe condition properly first.\n\nSigned-off-by: Shawn O. Pearce <sop@google.com>\n---\n .../spearce/jgit/dircache/DirCacheBasicTest.java   |    6 ++++++\n .../src/org/spearce/jgit/dircache/DirCache.java    |    6 ++----\n 2 files changed, 8 insertions(+), 4 deletions(-)\n\ndiff --git a/org.spearce.jgit.test/tst/org/spearce/jgit/dircache/DirCacheBasicTest.java b/org.spearce.jgit.test/tst/org/spearce/jgit/dircache/DirCacheBasicTest.java\nindex b3097ac..4d737c0 100644\n--- a/org.spearce.jgit.test/tst/org/spearce/jgit/dircache/DirCacheBasicTest.java\n+++ b/org.spearce.jgit.test/tst/org/spearce/jgit/dircache/DirCacheBasicTest.java\n@@ -39,6 +39,7 @@\n \n import java.io.File;\n \n+import org.spearce.jgit.lib.Constants;\n import org.spearce.jgit.lib.RepositoryTestCase;\n \n public class DirCacheBasicTest extends RepositoryTestCase {\n@@ -182,4 +183,9 @@ public void testBuildThenClear() throws Exception {\n \t\tassertEquals(0, dc.getEntryCount());\n \t}\n \n+\tpublic void testFindOnEmpty() throws Exception {\n+\t\tfinal DirCache dc = DirCache.newInCore();\n+\t\tfinal byte[] path = Constants.encode(\"a\");\n+\t\tassertEquals(-1, dc.findEntry(path, path.length));\n+\t}\n }\ndiff --git a/org.spearce.jgit/src/org/spearce/jgit/dircache/DirCache.java b/org.spearce.jgit/src/org/spearce/jgit/dircache/DirCache.java\nindex bfb7925..9f0810a 100644\n--- a/org.spearce.jgit/src/org/spearce/jgit/dircache/DirCache.java\n+++ b/org.spearce.jgit/src/org/spearce/jgit/dircache/DirCache.java\n@@ -583,8 +583,6 @@ public void unlock() {\n \t *         information. If < 0 the entry does not exist in the index.\n \t */\n \tpublic int findEntry(final String path) {\n-\t\tif (entryCnt == 0)\n-\t\t\treturn -1;\n \t\tfinal byte[] p = Constants.encode(path);\n \t\treturn findEntry(p, p.length);\n \t}\n@@ -592,7 +590,7 @@ public int findEntry(final String path) {\n \tint findEntry(final byte[] p, final int pLen) {\n \t\tint low = 0;\n \t\tint high = entryCnt;\n-\t\tdo {\n+\t\twhile (low < high) {\n \t\t\tint mid = (low + high) >>> 1;\n \t\t\tfinal int cmp = cmp(p, pLen, sortedEntries[mid]);\n \t\t\tif (cmp < 0)\n@@ -603,7 +601,7 @@ else if (cmp == 0) {\n \t\t\t\treturn mid;\n \t\t\t} else\n \t\t\t\tlow = mid + 1;\n-\t\t} while (low < high);\n+\t\t}\n \t\treturn -(low + 1);\n \t}\n \n-- \n1.6.4.2.395.ge3d52\n"}]}