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

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 defd93b10752b5e56be4ca16ba4bcf8bd56ac3ba..91364ce98432077355287f0aee9c12106a9df7d6 100644 (file)
--- 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 16f8cdfef7f04d9e9fb5c86b1dc9d6ac77af25c4..76f537817995dba3cf8e3ef85394ad7fd2783e00 100644 (file)
--- 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;