git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [JGIT PATCH 2/2] Make Repository.isValidRefName compatible with Git 1.6.3

From
Robin Rosenberg <robin.rosenberg@dewire.com>
Date
May 7, 2009, 23:02 UTC
Message-ID
<200905080102.44053.robin.rosenberg@dewire.com>
In-Reply-To
<1241708714-20326-2-git-send-email-spearce@spearce.org>
NAK, ammended patch follows.

Disallows @{, names that end with '.', '\' (that's the "almost" part) plus explicitly allows unicode names.

-- robin
>From 0854d96e2774bd084cc673ab16cf61c489f0562e Mon Sep 17 00:00:00 2001
From: Shawn O. Pearce <spearce@spearce.org>
Date: Thu, 7 May 2009 08:05:14 -0700
Subject: [EGIT PATCH] Make Repository.isValidRefName almost compatible with Git 1.6.3

In 3e262b95c509 I taught C Git to disallow refs whose names end in ".lock". This suffix is used by the atomic update mechanism as a signal that the ref is being modified. When reading a loose ref directory both JGit and C Git skip over any files whose names end with this suffix, as the file is assumed to be one of these magic locking files from another concurrent process. Consequently, any ref that ends with this name will become invisible once created.

In cbdffe4093be Junio disallows names that looks like reflog queries as well as names that ends with periods.

We also add a suite of tests for the isValidRefName function, fix its formatting to better conform to current style conventions, and correct the result for "master"; this is not a valid ref name as it has only 1 path component. At least 2 path components is required.

In addition to 1.6.3 we disallow names with a '\' (backslash) since that is a directory separator in Windows. Allowing JGit users to create such names is just asking for trouble. This does not necessarily prevent JGit from working with such refs in other situations.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
Signed-off-by: Robin Rosenberg <robin.rosenberg@dewire.com>
---
 .../tst/org/spearce/jgit/lib/ValidRefNameTest.java |  171 ++++++++++++++++++++
 .../src/org/spearce/jgit/lib/Repository.java       |   33 +++--
 2 files changed, 191 insertions(+), 13 deletions(-)
 create mode 100644 org.spearce.jgit.test/tst/org/spearce/jgit/lib/ValidRefNameTest.java
diff --git a/org.spearce.jgit.test/tst/org/spearce/jgit/lib/ValidRefNameTest.java b/org.spearce.jgit.test/tst/org/spearce/jgit/lib/ValidRefNameTest.java
new file mode 100644
index 0000000..cb6b9a7
--- /dev/null
+++ b/org.spearce.jgit.test/tst/org/spearce/jgit/lib/ValidRefNameTest.java
@@ -0,0 +1,171 @@
+/*
+ * Copyright (C) 2009, Google Inc.
+ *
+ * All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or
+ * without modification, are permitted provided that the following
+ * conditions are met:
+ *
+ * - Redistributions of source code must retain the above copyright
+ *   notice, this list of conditions and the following disclaimer.
+ *
+ * - Redistributions in binary form must reproduce the above
+ *   copyright notice, this list of conditions and the following
+ *   disclaimer in the documentation and/or other materials provided
+ *   with the distribution.
+ *
+ * - Neither the name of the Git Development Community nor the
+ *   names of its contributors may be used to endorse or promote
+ *   products derived from this software without specific prior
+ *   written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND
+ * CONTRIBUTORS "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES,
+ * INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES
+ * OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
+ * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR
+ * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT
+ * NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
+ * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER
+ * CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT,
+ * STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
+ * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF
+ * ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+
+package org.spearce.jgit.lib;
+
+import junit.framework.TestCase;
+
+public class ValidRefNameTest extends TestCase {
+	private static void assertValid(final boolean exp, final String name) {
+		assertEquals("\"" + name + "\"", exp, Repository.isValidRefName(name));
+	}
+
+	public void testEmptyString() {
+		assertValid(false, "");
+		assertValid(false, "/");
+	}
+
+	public void testMustHaveTwoComponents() {
+		assertValid(false, "master");
+		assertValid(true, "heads/master");
+	}
+
+	public void testValidHead() {
+		assertValid(true, "refs/heads/master");
+		assertValid(true, "refs/heads/pu");
+		assertValid(true, "refs/heads/z");
+		assertValid(true, "refs/heads/FoO");
+	}
+
+	public void testValidTag() {
+		assertValid(true, "refs/tags/v1.0");
+	}
+
+	public void testNoLockSuffix() {
+		assertValid(false, "refs/heads/master.lock");
+	}
+
+	public void testNoDirectorySuffix() {
+		assertValid(false, "refs/heads/master/");
+	}
+
+	public void testNoSpace() {
+		assertValid(false, "refs/heads/i haz space");
+	}
+
+	public void testNoAsciiControlCharacters() {
+		for (char c = '\0'; c < ' '; c++)
+			assertValid(false, "refs/heads/mast" + c + "er");
+	}
+
+	public void testNoBareDot() {
+		assertValid(false, "refs/heads/.");
+		assertValid(false, "refs/heads/..");
+		assertValid(false, "refs/heads/./master");
+		assertValid(false, "refs/heads/../master");
+	}
+
+	public void testNoLeadingOrTrailingDot() {
+		assertValid(false, ".");
+		assertValid(false, "refs/heads/.bar");
+		assertValid(false, "refs/heads/..bar");
+		assertValid(false, "refs/heads/bar.");
+	}
+
+	public void testContainsDot() {
+		assertValid(true, "refs/heads/m.a.s.t.e.r");
+		assertValid(false, "refs/heads/master..pu");
+	}
+
+	public void testNoMagicRefCharacters() {
+		assertValid(false, "refs/heads/master^");
+		assertValid(false, "refs/heads/^master");
+		assertValid(false, "^refs/heads/master");
+
+		assertValid(false, "refs/heads/master~");
+		assertValid(false, "refs/heads/~master");
+		assertValid(false, "~refs/heads/master");
+
+		assertValid(false, "refs/heads/master:");
+		assertValid(false, "refs/heads/:master");
+		assertValid(false, ":refs/heads/master");
+	}
+
+	public void testShellGlob() {
+		assertValid(false, "refs/heads/master?");
+		assertValid(false, "refs/heads/?master");
+		assertValid(false, "?refs/heads/master");
+
+		assertValid(false, "refs/heads/master[");
+		assertValid(false, "refs/heads/[master");
+		assertValid(false, "[refs/heads/master");
+
+		assertValid(false, "refs/heads/master*");
+		assertValid(false, "refs/heads/*master");
+		assertValid(false, "*refs/heads/master");
+	}
+
+	public void testValidSpecialCharacters() {
+		assertValid(true, "refs/heads/!");
+		assertValid(true, "refs/heads/\"");
+		assertValid(true, "refs/heads/#");
+		assertValid(true, "refs/heads/$");
+		assertValid(true, "refs/heads/%");
+		assertValid(true, "refs/heads/&");
+		assertValid(true, "refs/heads/'");
+		assertValid(true, "refs/heads/(");
+		assertValid(true, "refs/heads/)");
+		assertValid(true, "refs/heads/+");
+		assertValid(true, "refs/heads/,");
+		assertValid(true, "refs/heads/-");
+		assertValid(true, "refs/heads/;");
+		assertValid(true, "refs/heads/<");
+		assertValid(true, "refs/heads/=");
+		assertValid(true, "refs/heads/>");
+		assertValid(true, "refs/heads/@");
+		assertValid(true, "refs/heads/]");
+		assertValid(true, "refs/heads/_");
+		assertValid(true, "refs/heads/`");
+		assertValid(true, "refs/heads/{");
+		assertValid(true, "refs/heads/|");
+		assertValid(true, "refs/heads/}");
+
+		// This is valid on UNIX, but not on Windows
+		// hence we make in invalid due to non-portability
+		//
+		assertValid(false, "refs/heads/\\");
+	}
+
+	public void testUnicodeNames() {
+		assertValid(true, "refs/heads/\u00e5ngstr\u00f6m");
+	}
+
+	public void testRefLogQueryIsValidRef() {
+		assertValid(false, "refs/heads/master@{1}");
+		assertValid(false, "refs/heads/master@{1.hour.ago}");
+	}
+}
diff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/Repository.java b/org.spearce.jgit/src/org/spearce/jgit/lib/Repository.java
index 5baa7a0..a47207d 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/lib/Repository.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/lib/Repository.java
@@ -931,6 +931,8 @@ public RepositoryState getRepositoryState() {
 	 * a special meaning in a Git object reference expression. Some other
 	 * dangerous characters are also excluded.
 	 *
+	 * For portability reasons '\' is excluded
+	 *
 	 * @param refName
 	 *
 	 * @return true if refName is a valid ref name
@@ -939,36 +941,41 @@ public static boolean isValidRefName(final String refName) {
 		final int len = refName.length();
 		if (len == 0)
 			return false;
+		if (refName.endsWith(".lock"))
+			return false;
 
+		int components = 1;
 		char p = '\0';
-		for (int i=0; i<len; ++i) {
-			char c = refName.charAt(i);
+		for (int i = 0; i < len; i++) {
+			final char c = refName.charAt(i);
 			if (c <= ' ')
 				return false;
-			switch(c) {
+			switch (c) {
 			case '.':
-				if (i == 0)
-					return false;
-				if (p == '/')
+				switch (p) {
+				case '\0': case '/': case '.':
 					return false;
-				if (p == '.')
+				}
+				if (i == len -1)
 					return false;
 				break;
 			case '/':
-				if (i == 0)
+				if (i == 0 || i == len - 1)
 					return false;
-				if (i == len -1)
+				components++;
+				break;
+			case '@':
+				if (p == '{')
 					return false;
 				break;
 			case '~': case '^': case ':':
-			case '?': case '[':
-				return false;
-			case '*':
+			case '?': case '[': case '*':
+			case '\\':
 				return false;
 			}
 			p = c;
 		}
-		return true;
+		return components > 1;
 	}
 
 	/**

-- 
1.6.3
Previous: Shawn O. PearceNext: Linus Torvalds
Message 3 of 18 in “Add support for boolean config values "yes", "no"”
  1. 1/2 Add support for boolean config values "yes", "no"Shawn O. Pearce, May 7, 2009
  2. 2/2 Make Repository.isValidRefName compatible with Git 1.6.3Shawn O. Pearce, May 7, 2009
  3. Robin RosenbergMay 7, 2009
  4. Linus TorvaldsMay 7, 2009
  5. Junio C HamanoMay 8, 2009
  6. Shawn O. PearceMay 8, 2009
  7. Alex RiesenMay 8, 2009
  8. Johannes SchindelinMay 8, 2009
  9. Junio C HamanoMay 8, 2009
  10. Johannes SchindelinMay 8, 2009
  11. Alex RiesenMay 8, 2009
  12. Shawn O. PearceMay 8, 2009
  13. Brandon CaseyMay 7, 2009
  14. 1/2 Add support for boolean config values "on", "off"Shawn O. Pearce, May 7, 2009
  15. Shawn O. PearceMay 8, 2009
  16. Junio C HamanoMay 8, 2009
  17. Shawn O. PearceMay 8, 2009
  18. Junio C HamanoMay 8, 2009

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.