threads / patch / 20817

patch, 3 partsAllow RefUpdate.setExpectedOldObjectId to accept RevCommit

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

## tl;dr

7 messages between Sep 1, 2009 and Sep 4, 2009. Diffs are folded; open one to read it.

replies: 6people: 3as markdown or json

Shawn O. Pearce· Sep 1, 2009, 23:16 UTC · lore

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(-)
Show changes to 2 files +53 −1

org.spearce.jgit.test/tst/org/spearce/jgit/lib/RefUpdateTest.java, org.spearce.jgit/src/org/spearce/jgit/lib/RefUpdate.java

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
Shawn O. Pearce· Sep 1, 2009, 23:16 UTC · re: Shawn O. Pearce · lore

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

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(-)
Show changes to org.spearce.jgit/src/org/spearce/jgit/lib/RefUpdate.java +17 −9
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
Shawn O. Pearce· Sep 1, 2009, 23:16 UTC · re: Shawn O. Pearce · lore

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

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(-)
Show changes to 2 files +8 −4

org.spearce.jgit.test/tst/org/spearce/jgit/dircache/DirCacheBasicTest.java, org.spearce.jgit/src/org/spearce/jgit/dircache/DirCache.java

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
Robin Rosenberg· Sep 3, 2009, 23:14 UTC · re: Shawn O. Pearce · lore

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

onsdag 02 september 2009 01:16:50 skrev "Shawn O. Pearce" <spearce@spearce.org>:
Show 10 quoted lines
> 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
Shawn O. Pearce· Sep 3, 2009, 23:19 UTC · re: Robin Rosenberg · lore

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

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.
Nicolas Pitre· Sep 4, 2009, 00:06 UTC · re: Shawn O. Pearce · lore

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

On Thu, 3 Sep 2009, Shawn O. Pearce wrote:
Show 9 quoted lines
> 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
Shawn O. Pearce· Sep 1, 2009, 23:18 UTC · re: Shawn O. Pearce · lore

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

"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.

← back to recent threads