Git development
 help / color / mirror / Atom feed
From: "Shawn O. Pearce" <spearce@spearce.org>
To: Robin Rosenberg <robin.rosenberg@dewire.com>
Cc: git@vger.kernel.org, "Shawn O. Pearce" <sop@google.com>
Subject: [JGIT PATCH v2 2/3] Work around Sun javac compiler bug in RefUpdate
Date: Fri,  4 Sep 2009 09:22:44 -0700	[thread overview]
Message-ID: <1252081365-2335-2-git-send-email-spearce@spearce.org> (raw)
In-Reply-To: <1252081365-2335-1-git-send-email-spearce@spearce.org>

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

  reply	other threads:[~2009-09-04 16:24 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2009-09-04 16:22 [JGIT PATCH v2 1/3] Allow RefUpdate.setExpectedOldObjectId to accept RevCommit Shawn O. Pearce
2009-09-04 16:22 ` Shawn O. Pearce [this message]
2009-09-04 16:22   ` [JGIT PATCH v2 3/3] Fix DirCache.findEntry to work on an empty cache Shawn O. Pearce

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1252081365-2335-2-git-send-email-spearce@spearce.org \
    --to=spearce@spearce.org \
    --cc=git@vger.kernel.org \
    --cc=robin.rosenberg@dewire.com \
    --cc=sop@google.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox