[JGIT PATCH v2 1/3] Allow RefUpdate.setExpectedOldObjectId to accept RevCommit

[JGIT PATCH v2 1/3] Allow RefUpdate.setExpectedOldObjectId to accept RevCommit

[Shawn O. Pearce]

RevCommit overrides .equals() such that it only implements a
reference equality test.  If the expected old ObjectId was set
by the application to a RevCommit instance, it would always fail,
resulting in LOCK_FAILURE.  Instead use AnyObject.equals() to compare
the value, ignoring the possibly overloaded equals in RevCommit.

Signed-off-by: Shawn O. Pearce <sop@google.com>
---
 .../tst/org/spearce/jgit/lib/RefUpdateTest.java    |   52 ++++++++++++++++++++
 .../src/org/spearce/jgit/lib/RefUpdate.java        |    2 +-
 2 files changed, 53 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 800c0a4..a8ccf43 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
@@ -45,6 +45,7 @@
 
 import org.spearce.jgit.lib.RefUpdate.Result;
 import org.spearce.jgit.revwalk.RevCommit;
+import org.spearce.jgit.revwalk.RevWalk;
 
 public class RefUpdateTest extends RepositoryTestCase {
 
@@ -397,6 +398,57 @@ public void testUpdateRefLockFailureWrongOldValue() throws IOException {
 	}
 
 	/**
+	 * Try modify a ref forward, fast forward, checking old value first
+	 *
+	 * @throws IOException
+	 */
+	public void testUpdateRefForwardWithCheck1() throws IOException {
+		ObjectId ppid = db.resolve("refs/heads/master^");
+		ObjectId pid = db.resolve("refs/heads/master");
+
+		RefUpdate updateRef = db.updateRef("refs/heads/master");
+		updateRef.setNewObjectId(ppid);
+		updateRef.setForceUpdate(true);
+		Result update = updateRef.update();
+		assertEquals(Result.FORCED, update);
+		assertEquals(ppid, db.resolve("refs/heads/master"));
+
+		// real test
+		RefUpdate updateRef2 = db.updateRef("refs/heads/master");
+		updateRef2.setExpectedOldObjectId(ppid);
+		updateRef2.setNewObjectId(pid);
+		Result update2 = updateRef2.update();
+		assertEquals(Result.FAST_FORWARD, update2);
+		assertEquals(pid, db.resolve("refs/heads/master"));
+	}
+
+	/**
+	 * Try modify a ref forward, fast forward, checking old commit first
+	 *
+	 * @throws IOException
+	 */
+	public void testUpdateRefForwardWithCheck2() throws IOException {
+		ObjectId ppid = db.resolve("refs/heads/master^");
+		ObjectId pid = db.resolve("refs/heads/master");
+
+		RefUpdate updateRef = db.updateRef("refs/heads/master");
+		updateRef.setNewObjectId(ppid);
+		updateRef.setForceUpdate(true);
+		Result update = updateRef.update();
+		assertEquals(Result.FORCED, update);
+		assertEquals(ppid, db.resolve("refs/heads/master"));
+
+		// real test
+		RevCommit old = new RevWalk(db).parseCommit(ppid);
+		RefUpdate updateRef2 = db.updateRef("refs/heads/master");
+		updateRef2.setExpectedOldObjectId(old);
+		updateRef2.setNewObjectId(pid);
+		Result update2 = updateRef2.update();
+		assertEquals(Result.FAST_FORWARD, update2);
+		assertEquals(pid, db.resolve("refs/heads/master"));
+	}
+
+	/**
 	 * Try modify a ref that is locked
 	 *
 	 * @throws IOException
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 69399ec..8dffed2 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/lib/RefUpdate.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/lib/RefUpdate.java
@@ -466,7 +466,7 @@ private Result updateImpl(final RevWalk walk, final Store store)
 			if (expValue != null) {
 				final ObjectId o;
 				o = oldValue != null ? oldValue : ObjectId.zeroId();
-				if (!expValue.equals(o))
+				if (!AnyObjectId.equals(expValue, o))
 					return Result.LOCK_FAILURE;
 			}
 			if (oldValue == null)
-- 
1.6.4.2.395.ge3d52

[JGIT PATCH v2 2/3] Work around Sun javac compiler bug in RefUpdate

[Shawn O. Pearce]

Sun's javac, version 1.5.0_06 and also a Google internal fork of
OpenJDK 1.6.0 both apparently miscompile the for loop which is
looking for a conflicting ref name in the existing set of refs for
this repository.

Debugging this code showed the control flow to return LOCK_FAILURE
when startsWith returned false, which is highly illogical and the
exact opposite of what we have written here.

OpenJDK 1.6.0's javap tool was unable to disassemble the compiled
method.  Instead it simply failed to produce anything about
updateImpl.  So my remark about the code being compiled wrong
is only a guess based on how I observed the behavior, and not by
actually studying the resulting instructions.

Eclipse 3.4.2's JDT appears to have compiled the updateImpl method
correctly, and produces a working executable.  But this is a much
less common compiler to build Java libraries with.

Robin was able to successfully compile the ancestor version of this
code with both javac 1.5.0_19 and also 1.6.0_16, but not everyone
building JGit has these versions installed.

This refactoring to extract the name conflicting test out into its
own method appears to work around the Sun javac bug I observed above,
and the resulting class works correctly with any compiler tested.
The code is also more clear, so its a gain either way.

Signed-off-by: Shawn O. Pearce <sop@google.com>
---
 .../src/org/spearce/jgit/lib/RefUpdate.java        |   26 +++++++++++++-------
 1 files changed, 17 insertions(+), 9 deletions(-)

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 8dffed2..8226e10 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/lib/RefUpdate.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/lib/RefUpdate.java
@@ -449,15 +449,8 @@ private Result updateImpl(final RevWalk walk, final Store store)
 		RevObject newObj;
 		RevObject oldObj;
 
-		int lastSlash = getName().lastIndexOf('/');
-		if (lastSlash > 0)
-			if (db.getRepository().getRef(getName().substring(0, lastSlash)) != null)
-				return Result.LOCK_FAILURE;
-		String rName = getName() + "/";
-		for (Ref r : db.getAllRefs().values()) {
-			if (r.getName().startsWith(rName))
-				return Result.LOCK_FAILURE;
-		}
+		if (isNameConflicting())
+			return Result.LOCK_FAILURE;
 		lock = new LockFile(looseFile);
 		if (!lock.lock())
 			return Result.LOCK_FAILURE;
@@ -490,6 +483,21 @@ private Result updateImpl(final RevWalk walk, final Store store)
 		}
 	}
 
+	private boolean isNameConflicting() throws IOException {
+		final String myName = getName();
+		final int lastSlash = myName.lastIndexOf('/');
+		if (lastSlash > 0)
+			if (db.getRepository().getRef(myName.substring(0, lastSlash)) != null)
+				return true;
+
+		final String rName = myName + "/";
+		for (Ref r : db.getAllRefs().values()) {
+			if (r.getName().startsWith(rName))
+				return true;
+		}
+		return false;
+	}
+
 	private static RevObject safeParse(final RevWalk rw, final AnyObjectId id)
 			throws IOException {
 		try {
-- 
1.6.4.2.395.ge3d52

[JGIT PATCH v2 3/3] Fix DirCache.findEntry to work on an empty cache

[Shawn O. Pearce]

If the cache has no entries, we want to return -1 rather than throw
ArrayIndexOutOfBoundsException.  This binary search loop was stolen
from some other code which contained a test before the loop to see if
the collection was empty or not, but we failed to include that here.

Flipping the loop around to a standard while loop ensures we test
the condition properly first.

Signed-off-by: Shawn O. Pearce <sop@google.com>
---
 .../spearce/jgit/dircache/DirCacheBasicTest.java   |    6 ++++++
 .../src/org/spearce/jgit/dircache/DirCache.java    |    6 ++----
 2 files changed, 8 insertions(+), 4 deletions(-)

diff --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
index b3097ac..4d737c0 100644
--- a/org.spearce.jgit.test/tst/org/spearce/jgit/dircache/DirCacheBasicTest.java
+++ b/org.spearce.jgit.test/tst/org/spearce/jgit/dircache/DirCacheBasicTest.java
@@ -39,6 +39,7 @@
 
 import java.io.File;
 
+import org.spearce.jgit.lib.Constants;
 import org.spearce.jgit.lib.RepositoryTestCase;
 
 public class DirCacheBasicTest extends RepositoryTestCase {
@@ -182,4 +183,9 @@ public void testBuildThenClear() throws Exception {
 		assertEquals(0, dc.getEntryCount());
 	}
 
+	public void testFindOnEmpty() throws Exception {
+		final DirCache dc = DirCache.newInCore();
+		final byte[] path = Constants.encode("a");
+		assertEquals(-1, dc.findEntry(path, path.length));
+	}
 }
diff --git a/org.spearce.jgit/src/org/spearce/jgit/dircache/DirCache.java b/org.spearce.jgit/src/org/spearce/jgit/dircache/DirCache.java
index bfb7925..9f0810a 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/dircache/DirCache.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/dircache/DirCache.java
@@ -583,8 +583,6 @@ public void unlock() {
 	 *         information. If < 0 the entry does not exist in the index.
 	 */
 	public int findEntry(final String path) {
-		if (entryCnt == 0)
-			return -1;
 		final byte[] p = Constants.encode(path);
 		return findEntry(p, p.length);
 	}
@@ -592,7 +590,7 @@ public int findEntry(final String path) {
 	int findEntry(final byte[] p, final int pLen) {
 		int low = 0;
 		int high = entryCnt;
-		do {
+		while (low < high) {
 			int mid = (low + high) >>> 1;
 			final int cmp = cmp(p, pLen, sortedEntries[mid]);
 			if (cmp < 0)
@@ -603,7 +601,7 @@ else if (cmp == 0) {
 				return mid;
 			} else
 				low = mid + 1;
-		} while (low < high);
+		}
 		return -(low + 1);
 	}
 
-- 
1.6.4.2.395.ge3d52