[JGIT PATCH (RESEND) 1/3] Allow RefUpdate.setExpectedOldObjectId to accept RevCommit

[JGIT PATCH (RESEND) 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>
Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 .../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.1.341.gf2a44

[JGIT PATCH (RESEND) 2/3] Work around Sun javac compiler error in RefUpdate

[Shawn O. Pearce]

Sun's javac, version 5 and 6, apparently miscompiles 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.

Sun'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'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.

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

Signed-off-by: Shawn O. Pearce <sop@google.com>
Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 .../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.1.341.gf2a44

[JGIT PATCH (RESEND) 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>
Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 .../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.1.341.gf2a44

Re: [JGIT PATCH (RESEND) 3/3] Fix DirCache.findEntry to work on an empty cache

[Robin Rosenberg]

onsdag 02 september 2009 01:16:50 skrev "Shawn O. Pearce" <spearce@spearce.org>:
> 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>
> Signed-off-by: Shawn O. Pearce <spearce@spearce.org>

Is this a new policy?

-- robin

Re: [JGIT PATCH (RESEND) 3/3] Fix DirCache.findEntry to work on an empty cache

[Shawn O. Pearce]

Robin Rosenberg <robin.rosenberg@dewire.com> wrote:
> > Signed-off-by: Shawn O. Pearce <sop@google.com>
> > Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
> 
> Is this a new policy?

Call it a new habit.  I send from my @spearce.org because that's my
"public identity", but this was on work time, so I added an extra
SOB line to indicate that yes, my employer is also OK with this.
Not that anyone probably doubted that in the first place though...
I work for a company that is pretty friendly towards open source.

-- 
Shawn.

Re: [JGIT PATCH (RESEND) 3/3] Fix DirCache.findEntry to work on an empty cache

[Nicolas Pitre]

On Thu, 3 Sep 2009, Shawn O. Pearce wrote:

> Robin Rosenberg <robin.rosenberg@dewire.com> wrote:
> > > Signed-off-by: Shawn O. Pearce <sop@google.com>
> > > Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
> > 
> > Is this a new policy?
> 
> Call it a new habit.  I send from my @spearce.org because that's my
> "public identity", but this was on work time, so I added an extra
> SOB line to indicate that yes, my employer is also OK with this.

You might as well simply use your Google SOB alone, even if your From is 
@spearce.org.  That's what I do with my Linux patches done on $day_job 
time.


Nicolas

Re: [JGIT PATCH (RESEND) 1/3] Allow RefUpdate.setExpectedOldObjectId to accept RevCommit

[Shawn O. Pearce]

"Shawn O. Pearce" <spearce@spearce.org> wrote:
> Subject: Re: [JGIT PATCH (RESEND) 1/3] Allow RefUpdate.setExpectedOldObjectId to accept RevCommit

Sorry, this is not a resend, its the first time I've sent it...

-- 
Shawn.