Allow @ in branch names and tighten syntax checking

Valid refs are defined by git-check-ref-format(1). In addition
we will not try to perform a lookup of an invalid ref name in
Repository.resolve().

Reported by R Shapiro in the Eclipse JGit Forum.

Change-Id: I0b098eec9ecb98a9ce16b1cfb476729aaf2fb190

2 files changed, 86 insertions, 7 deletions

diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/lib/RepositoryResolveTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/lib/RepositoryResolveTest.java
index defd93b107..91364ce984 100644
--- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/lib/RepositoryResolveTest.java
+++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/lib/RepositoryResolveTest.java
@@ -47,14 +47,18 @@
 package org.eclipse.jgit.lib;
 
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertSame;
+import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
 
 import java.io.IOException;
 
 import org.eclipse.jgit.api.Git;
+import org.eclipse.jgit.errors.AmbiguousObjectException;
 import org.eclipse.jgit.errors.IncorrectObjectTypeException;
+import org.eclipse.jgit.errors.RevisionSyntaxException;
 import org.eclipse.jgit.revwalk.RevCommit;
 import org.junit.Test;
 
@@ -133,9 +137,11 @@ public class RepositoryResolveTest extends SampleDataRepositoryTestCase {
 	public void testDistance_past_root() throws IOException {
 		assertEquals("42e4e7c5e507e113ebbb7801b16b52cf867b7ce1",db.resolve("6462e7d8024396b14d7651e2ec11e2bbf07a05c4~1").name());
 		assertNull(db.resolve("6462e7d8024396b14d7651e2ec11e2bbf07a05c4~~"));
+		assertNull(db.resolve("6462e7d8024396b14d7651e2ec11e2bbf07a05c4^^"));
 		assertNull(db.resolve("6462e7d8024396b14d7651e2ec11e2bbf07a05c4~2"));
 		assertNull(db.resolve("6462e7d8024396b14d7651e2ec11e2bbf07a05c4~99"));
 		assertNull(db.resolve("42e4e7c5e507e113ebbb7801b16b52cf867b7ce1~~"));
+		assertNull(db.resolve("42e4e7c5e507e113ebbb7801b16b52cf867b7ce1^^"));
 		assertNull(db.resolve("42e4e7c5e507e113ebbb7801b16b52cf867b7ce1~2"));
 		assertNull(db.resolve("42e4e7c5e507e113ebbb7801b16b52cf867b7ce1~99"));
 		assertEquals("42e4e7c5e507e113ebbb7801b16b52cf867b7ce1",db.resolve("master~6").name());
@@ -289,6 +295,53 @@ public class RepositoryResolveTest extends SampleDataRepositoryTestCase {
 		assertEquals("refs/remotes/origin/main", db.simplify("@{upstream}"));
 	}
 
+	@Test
+	public void invalidNames() throws AmbiguousObjectException, IOException {
+		assertTrue(Repository.isValidRefName("x/a"));
+		assertTrue(Repository.isValidRefName("x/a.b"));
+		assertTrue(Repository.isValidRefName("x/a@b"));
+		assertTrue(Repository.isValidRefName("x/a@b{x}"));
+		assertTrue(Repository.isValidRefName("x/a/b"));
+		assertTrue(Repository.isValidRefName("x/a]b")); // odd, yes..
+		assertTrue(Repository.isValidRefName("x/\u00a0")); // unicode is fine,
+															// even hard space
+		assertFalse(Repository.isValidRefName("x/.a"));
+		assertFalse(Repository.isValidRefName("x/a."));
+		assertFalse(Repository.isValidRefName("x/a..b"));
+		assertFalse(Repository.isValidRefName("x//a"));
+		assertFalse(Repository.isValidRefName("x/a/"));
+		assertFalse(Repository.isValidRefName("x/a//b"));
+		assertFalse(Repository.isValidRefName("x/a[b"));
+		assertFalse(Repository.isValidRefName("x/a^b"));
+		assertFalse(Repository.isValidRefName("x/a*b"));
+		assertFalse(Repository.isValidRefName("x/a?b"));
+		assertFalse(Repository.isValidRefName("x/a~1"));
+		assertFalse(Repository.isValidRefName("x/a\\b"));
+		assertFalse(Repository.isValidRefName("x/a\u0000"));
+
+		assertUnparseable(".");
+		assertUnparseable("x@{3");
+		assertUnparseable("x[b");
+		assertUnparseable("x y");
+		assertUnparseable("x.");
+		assertUnparseable(".x");
+		assertUnparseable("a..b");
+		assertUnparseable("x\\b");
+		assertUnparseable("a~b");
+		assertUnparseable("a^b");
+		assertUnparseable("a\u0000");
+	}
+
+	private void assertUnparseable(String s) throws AmbiguousObjectException,
+			IOException {
+		try {
+			db.resolve(s);
+			fail("'" + s + "' should be unparseable");
+		} catch (RevisionSyntaxException e) {
+			// good
+		}
+	}
+
 	private static ObjectId id(String name) {
 		return ObjectId.fromString(name);
 	}
diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/lib/Repository.java b/org.eclipse.jgit/src/org/eclipse/jgit/lib/Repository.java
index 16f8cdfef7..76f5378179 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/lib/Repository.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/lib/Repository.java
@@ -429,7 +429,12 @@ public abstract class Repository {
 			case '^':
 				if (rev == null) {
 					if (name == null)
-						name = new String(revChars, done, i);
+						if (done == 0)
+							name = new String(revChars, done, i);
+						else {
+							done = i + 1;
+							break;
+						}
 					rev = parseSimple(rw, name);
 					name = null;
 					if (rev == null)
@@ -471,7 +476,7 @@ public abstract class Repository {
 								rev = commit.getParent(pnum - 1);
 						}
 						i = j - 1;
-						done = i;
+						done = j;
 						break;
 					case '{':
 						int k;
@@ -512,7 +517,6 @@ public abstract class Repository {
 						} else
 							throw new IncorrectObjectTypeException(rev,
 									Constants.TYPE_COMMIT);
-
 					}
 				} else {
 					rev = rw.peel(rev);
@@ -526,11 +530,17 @@ public abstract class Repository {
 						throw new IncorrectObjectTypeException(rev,
 								Constants.TYPE_COMMIT);
 				}
+				done = i + 1;
 				break;
 			case '~':
 				if (rev == null) {
 					if (name == null)
-						name = new String(revChars, done, i);
+						if (done == 0)
+							name = new String(revChars, done, i);
+						else {
+							done = i + 1;
+							break;
+						}
 					rev = parseSimple(rw, name);
 					name = null;
 					if (rev == null)
@@ -568,10 +578,13 @@ public abstract class Repository {
 					--dist;
 				}
 				i = l - 1;
+				done = l;
 				break;
 			case '@':
 				if (rev != null)
 					throw new RevisionSyntaxException(revstr);
+				if (i + 1 < revChars.length && revChars[i + 1] != '{')
+					continue;
 				int m;
 				String time = null;
 				for (m = i + 2; m < revChars.length; ++m) {
@@ -588,6 +601,8 @@ public abstract class Repository {
 							// Currently checked out branch, HEAD if
 							// detached
 							name = Constants.HEAD;
+						if (!Repository.isValidRefName("x/" + name))
+							throw new RevisionSyntaxException(revstr);
 						Ref ref = getRef(name);
 						name = null;
 						if (ref == null)
@@ -636,6 +651,8 @@ public abstract class Repository {
 							name = new String(revChars, done, i);
 						if (name.equals(""))
 							name = Constants.HEAD;
+						if (!Repository.isValidRefName("x/" + name))
+							throw new RevisionSyntaxException(revstr);
 						Ref ref = getRef(name);
 						name = null;
 						if (ref == null)
@@ -680,7 +697,11 @@ public abstract class Repository {
 			return rev.copy();
 		if (name != null)
 			return name;
+		if (rev == null && done == revstr.length())
+			return null;
 		name = revstr.substring(done);
+		if (!Repository.isValidRefName("x/" + name))
+			throw new RevisionSyntaxException(revstr);
 		if (getRef(name) != null)
 			return name;
 		return resolveSimple(name);
@@ -709,9 +730,11 @@ public abstract class Repository {
 		if (ObjectId.isId(revstr))
 			return ObjectId.fromString(revstr);
 
-		Ref r = getRefDatabase().getRef(revstr);
-		if (r != null)
-			return r.getObjectId();
+		if (Repository.isValidRefName("x/" + revstr)) {
+			Ref r = getRefDatabase().getRef(revstr);
+			if (r != null)
+				return r.getObjectId();
+		}
 
 		if (AbbreviatedObjectId.isId(revstr))
 			return resolveAbbreviation(revstr);
@@ -1132,6 +1155,8 @@ public abstract class Repository {
 			case '/':
 				if (i == 0 || i == len - 1)
 					return false;
+				if (p == '/')
+					return false;
 				components++;
 				break;
 			case '{':
@@ -1141,6 +1166,7 @@ public abstract class Repository {
 			case '~': case '^': case ':':
 			case '?': case '[': case '*':
 			case '\\':
+			case '\u007F':
 				return false;
 			}
 			p = c;