[RFC 00/13] Checking full vs. partial refnames

[RFC 00/13] Checking full vs. partial refnames

[mhagger]

From: Michael Haggerty <mhagger@alum.mit.edu>

There are many places where it is necessary to determine whether a
refname is a complete, valid, top-level reference name in the "refs/"
tree, one of the special refnames like "HEAD" or "FETCH_HEAD", or
whether it is potentially a valid fragment of a refname that can be
DWIMed into a true reference.  Until now such checks have been
incomplete and/or scattered around.

The first three patches in this series beef up check_refname_format()
(and adds another static function, parse_refname_prefix()) with the
ability to make such checks when the REFNAME_FULL flag is used.

The fourth patch removes the checking of refnames passed to
add_extra_ref(), allowing the function to tolerate oddities like
"refs/tags/3.1.1.1^{}".

The rest of the patches consist of wild-assed guesses about some
callers of check_refname_format() that I suppose can use the
REFNAME_FULL option, thereby tightening up what they accept.  About
all I can say is that the test suite passes with these patches
applied.  But recent experience indicates that the test suite has a
lot of holes.  Therefore, it would be great if experts would look over
these suggestions.

There are many other callers of check_refname_format() that I haven't
touched, because I'm not even brave enough to make wild-assed guesses
about them.  (Since I left them without the REFNAME_FULL option, they
rather allow too many references through than too few.)  It would be
great if a more experienced developer would look at the other callers
and decide which need to be changed.

BTW, this patch series does *not* fix the specific problem that Junio
mentioned (that "git update-ref tmp/junk HEAD" does not reject the
bogus refname), nor probably many others.  The gruelling work is not
this patch series; it is the effort of tracking down all of the code
paths that might try to pass bogus refnames to the refs API.

This patch series applies on top of jc/check-ref-format-fixup, and
also rebases smoothly to the current "next".

Michael Haggerty (13):
  check_refname_component(): iterate via index rather than via pointer
  parse_refname_prefix(): new function
  Teach check_refname_format() to check full refnames
  add_ref(): move the call of check_refname_format() to callers
  receive-pack::update(): use check_refname_format(..., REFNAME_FULL)
  strbuf_check_branch_ref(): use check_refname_format(...,
    REFNAME_FULL)
  one_local_ref(): use check_refname_format(..., REFNAME_FULL)
  expand_namespace(): the refname is full, so use REFNAME_FULL option
  new_branch(): verify that new branch name is a valid full refname
  strbuf_check_tag_ref(): the refname is full, so use REFNAME_FULL
    option
  replace_object(): the refname is full, so use REFNAME_FULL option
  resolve_ref: use check_refname_format(..., REFNAME_FULL)
  filter_refs(): the refname is full, so use REFNAME_FULL option

 Documentation/git-check-ref-format.txt |   14 ++++
 builtin/check-ref-format.c             |    4 +
 builtin/fetch-pack.c                   |    2 +-
 builtin/receive-pack.c                 |    2 +-
 builtin/replace.c                      |    2 +-
 builtin/tag.c                          |    2 +-
 environment.c                          |    2 +-
 fast-import.c                          |    2 +-
 refs.c                                 |  124 ++++++++++++++++++++------------
 refs.h                                 |   12 ++-
 remote.c                               |    2 +-
 sha1_name.c                            |    2 +-
 t/t1402-check-ref-format.sh            |   31 ++++++++
 13 files changed, 143 insertions(+), 58 deletions(-)

-- 
1.7.7

[RFC 01/13] check_refname_component(): iterate via index rather than via pointer

[mhagger]

From: Michael Haggerty <mhagger@alum.mit.edu>

This will make later changes easier.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs.c |   14 +++++++-------
 1 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/refs.c b/refs.c
index 0d74d70..3b41252 100644
--- a/refs.c
+++ b/refs.c
@@ -912,11 +912,11 @@ static inline int bad_ref_char(int ch)
  */
 static int check_refname_component(const char *ref, int flags)
 {
-	const char *cp;
+	int i, len = strlen(ref);
 	char last = '\0';
 
-	for (cp = ref; ; cp++) {
-		char ch = *cp;
+	for (i = 0; i < len; i++) {
+		char ch = ref[i];
 		if (ch == '\0' || ch == '/')
 			break;
 		if (bad_ref_char(ch))
@@ -927,7 +927,7 @@ static int check_refname_component(const char *ref, int flags)
 			return -1; /* Refname contains "@{". */
 		last = ch;
 	}
-	if (cp == ref)
+	if (i == 0)
 		return -1; /* Component has zero length. */
 	if (ref[0] == '.') {
 		if (!(flags & REFNAME_DOT_COMPONENT))
@@ -936,12 +936,12 @@ static int check_refname_component(const char *ref, int flags)
 		 * Even if leading dots are allowed, don't allow "."
 		 * as a component (".." is prevented by a rule above).
 		 */
-		if (ref[1] == '\0')
+		if (i == 1)
 			return -1; /* Component equals ".". */
 	}
-	if (cp - ref >= 5 && !memcmp(cp - 5, ".lock", 5))
+	if (i >= 5 && !memcmp(&ref[i - 5], ".lock", 5))
 		return -1; /* Refname ends with ".lock". */
-	return cp - ref;
+	return i;
 }
 
 int check_refname_format(const char *ref, int flags)
-- 
1.7.7

[RFC 02/13] parse_refname_prefix(): new function

[mhagger]

From: Michael Haggerty <mhagger@alum.mit.edu>

Add a function parse_refname_prefix() that can read a possible refname
from the front of a string.  This is like check_refname_format(),
except:

* It accepts (string, len) parameters and can therefore handle
  non-NUL-terminated strings.

* It returns the length of the part of the string that was parsed,
  allowing it to be used for refnames that are followed by additional
  characters.

Re-implement check_refname_format() using the new function.

Rename function check_refname_component() to parse_refname_component()
for consistency.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs.c |   64 ++++++++++++++++++++++++++++++++++++++++++++--------------------
 1 files changed, 44 insertions(+), 20 deletions(-)

diff --git a/refs.c b/refs.c
index 3b41252..387da83 100644
--- a/refs.c
+++ b/refs.c
@@ -907,24 +907,30 @@ static inline int bad_ref_char(int ch)
 
 /*
  * Try to read one refname component from the front of ref.  Return
- * the length of the component found, or -1 if the component is not
- * legal.
+ * the length of the component found, or -1 if the start of the string
+ * cannot be interpreted as a component of a legal refname.
  */
-static int check_refname_component(const char *ref, int flags)
+static int parse_refname_component(const char *ref, int len, int flags)
 {
-	int i, len = strlen(ref);
+	int i;
 	char last = '\0';
 
 	for (i = 0; i < len; i++) {
 		char ch = ref[i];
-		if (ch == '\0' || ch == '/')
-			break;
+		if (ch == '/')
+			break; /* Component terminated by "..". */
 		if (bad_ref_char(ch))
-			return -1; /* Illegal character in refname. */
-		if (last == '.' && ch == '.')
-			return -1; /* Refname contains "..". */
-		if (last == '@' && ch == '{')
-			return -1; /* Refname contains "@{". */
+			break; /* Component terminated by illegal character. */
+		if (last == '.' && ch == '.') {
+			/* Component terminated by "..". */
+			i--;
+			break;
+		}
+		if (last == '@' && ch == '{') {
+			/* Component terminated by "@{". */
+			i--;
+			break;
+		}
 		last = ch;
 	}
 	if (i == 0)
@@ -944,17 +950,26 @@ static int check_refname_component(const char *ref, int flags)
 	return i;
 }
 
-int check_refname_format(const char *ref, int flags)
+/*
+ * Try to interpret the beginning of a string as a refname.  Return
+ * the length of the part of the string that could constitute a valid
+ * refname, or -1 if the start of the string cannot possibly be
+ * interpreted as a refname.  flags has the same interpretation as for
+ * check_refname_format().
+ */
+static int parse_refname_prefix(const char *ref, int len, int flags)
 {
 	int component_len, component_count = 0;
+	const char *p = ref;
+	int valid_len;
 
 	while (1) {
-		/* We are at the start of a path component. */
-		component_len = check_refname_component(ref, flags);
+		/* p is at the start of a path component. */
+		component_len = parse_refname_component(p, len, flags);
 		if (component_len < 0) {
 			if ((flags & REFNAME_REFSPEC_PATTERN) &&
-					ref[0] == '*' &&
-					(ref[1] == '\0' || ref[1] == '/')) {
+					len && p[0] == '*' &&
+					(len == 1 || p[1] == '/')) {
 				/* Accept one wildcard as a full refname component. */
 				flags &= ~REFNAME_REFSPEC_PATTERN;
 				component_len = 1;
@@ -963,17 +978,26 @@ int check_refname_format(const char *ref, int flags)
 			}
 		}
 		component_count++;
-		if (ref[component_len] == '\0')
+		if (component_len == len || p[component_len] != '/')
 			break;
 		/* Skip to next component. */
-		ref += component_len + 1;
+		p += component_len + 1;
+		len -= component_len + 1;
 	}
 
-	if (ref[component_len - 1] == '.')
+	valid_len = p + component_len - ref;
+
+	if (ref[valid_len - 1] == '.')
 		return -1; /* Refname ends with '.'. */
 	if (!(flags & REFNAME_ALLOW_ONELEVEL) && component_count < 2)
 		return -1; /* Refname has only one component. */
-	return 0;
+	return valid_len;
+}
+
+int check_refname_format(const char *ref, int flags)
+{
+	int len = strlen(ref);
+	return parse_refname_prefix(ref, len, flags) == len ? 0 : -1;
 }
 
 const char *prettify_refname(const char *name)
-- 
1.7.7

[RFC 03/13] Teach check_refname_format() to check full refnames

[mhagger]

From: Michael Haggerty <mhagger@alum.mit.edu>

A full refname has to start with "refs/" or consist entirely of capital
letters and underscores.  Add an option REFNAME_FULL that causes
check_refname_format() and parse_refname_prefix() to verify that the
refname is a valid full refname.

Add a "--full" option to "git check-ref-format" that can be used to
access the new option.  Use this to add some tests involving "git
check-ref-format --full".

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 Documentation/git-check-ref-format.txt |   14 +++++++++++
 builtin/check-ref-format.c             |    4 +++
 refs.c                                 |   39 ++++++++++++++++++-------------
 refs.h                                 |   12 ++++++---
 t/t1402-check-ref-format.sh            |   31 +++++++++++++++++++++++++
 5 files changed, 80 insertions(+), 20 deletions(-)

diff --git a/Documentation/git-check-ref-format.txt b/Documentation/git-check-ref-format.txt
index 103e7b1..9a8aafb 100644
--- a/Documentation/git-check-ref-format.txt
+++ b/Documentation/git-check-ref-format.txt
@@ -27,6 +27,11 @@ if refs are packed by `git gc`).
 
 git imposes the following rules on how references are named:
 
+. They must either start with `refs/` (e.g., `refs/heads/master`) or
+  consist entirely of capital letters and underscores (e.g., `HEAD` or
+  `MERGE_HEAD`).  This rule is enforced if the `--full` option is
+  used.
+
 . They can include slash `/` for hierarchical (directory)
   grouping, but no slash-separated component can begin with a
   dot `.` or end with the sequence `.lock`.
@@ -83,6 +88,15 @@ typed the branch name.
 
 OPTIONS
 -------
+--full::
+--no-full::
+	Controls whether the refname must represent a full refname
+	(i.e., starting with `refs/`).  If `--full` and
+	`--allow-onelevel` are both specified, then special top-level
+	refnames consisting of capital letters and underscored (like
+	`HEAD` or `MERGE_HEAD`) are also allowed.  The default is
+	`--no-full`.
+
 --allow-onelevel::
 --no-allow-onelevel::
 	Controls whether one-level refnames are accepted (i.e.,
diff --git a/builtin/check-ref-format.c b/builtin/check-ref-format.c
index 28a7320..a73626d 100644
--- a/builtin/check-ref-format.c
+++ b/builtin/check-ref-format.c
@@ -64,6 +64,10 @@ int cmd_check_ref_format(int argc, const char **argv, const char *prefix)
 	for (i = 1; i < argc && argv[i][0] == '-'; i++) {
 		if (!strcmp(argv[i], "--normalize") || !strcmp(argv[i], "--print"))
 			normalize = 1;
+		else if (!strcmp(argv[i], "--full"))
+			flags |= REFNAME_FULL;
+		else if (!strcmp(argv[i], "--no-full"))
+			flags &= ~REFNAME_FULL;
 		else if (!strcmp(argv[i], "--allow-onelevel"))
 			flags |= REFNAME_ALLOW_ONELEVEL;
 		else if (!strcmp(argv[i], "--no-allow-onelevel"))
diff --git a/refs.c b/refs.c
index 387da83..8299e51 100644
--- a/refs.c
+++ b/refs.c
@@ -980,6 +980,11 @@ static int parse_refname_prefix(const char *ref, int len, int flags)
 		component_count++;
 		if (component_len == len || p[component_len] != '/')
 			break;
+		if (component_count == 1 && (flags & REFNAME_FULL)) {
+			/* That was the first of multiple components */
+			if (component_len != 4 || strncmp(p, "refs", 4))
+				return -1;
+		}
 		/* Skip to next component. */
 		p += component_len + 1;
 		len -= component_len + 1;
@@ -989,8 +994,22 @@ static int parse_refname_prefix(const char *ref, int len, int flags)
 
 	if (ref[valid_len - 1] == '.')
 		return -1; /* Refname ends with '.'. */
-	if (!(flags & REFNAME_ALLOW_ONELEVEL) && component_count < 2)
-		return -1; /* Refname has only one component. */
+
+	if (component_count == 1) {
+		if (!(flags & REFNAME_ALLOW_ONELEVEL))
+			return -1;
+
+		if (flags & REFNAME_FULL) {
+			/* Make sure that the refname is ALL_CAPS. */
+			int i;
+			for (i = 0; i < component_len; i++) {
+				char ch = p[i];
+				if (ch != '_' && (ch < 'A' || 'Z' < ch))
+					return -1;
+			}
+		}
+	}
+
 	return valid_len;
 }
 
@@ -1028,20 +1047,8 @@ const char *ref_fetch_rules[] = {
 
 static int refname_ok_at_root_level(const char *str, int len)
 {
-	if (len >= 5 && !memcmp(str, "refs/", 5))
-		return 1;
-
-	while (len--) {
-		char ch = *str++;
-
-		/*
-		 * Only accept likes of .git/HEAD, .git/MERGE_HEAD at
-		 * the root level as a ref.
-		 */
-		if (ch != '_' && (ch < 'A' || 'Z' < ch))
-			return 0;
-	}
-	return 1;
+	return parse_refname_prefix(str, len,
+				    REFNAME_ALLOW_ONELEVEL|REFNAME_FULL) == len;
 }
 
 int refname_match(const char *abbrev_name, const char *full_name, const char **rules)
diff --git a/refs.h b/refs.h
index 0229c57..7707cda 100644
--- a/refs.h
+++ b/refs.h
@@ -97,9 +97,10 @@ int for_each_recent_reflog_ent(const char *ref, each_reflog_ent_fn fn, long, voi
  */
 extern int for_each_reflog(each_ref_fn, void *);
 
-#define REFNAME_ALLOW_ONELEVEL 1
-#define REFNAME_REFSPEC_PATTERN 2
-#define REFNAME_DOT_COMPONENT 4
+#define REFNAME_ALLOW_ONELEVEL 01
+#define REFNAME_REFSPEC_PATTERN 02
+#define REFNAME_DOT_COMPONENT 04
+#define REFNAME_FULL 010
 
 /*
  * Return 0 iff ref has the correct format for a refname according to
@@ -110,7 +111,10 @@ extern int for_each_reflog(each_ref_fn, void *);
  * components.  No leading or repeated slashes are accepted.  If
  * REFNAME_DOT_COMPONENT is set in flags, then allow refname
  * components to start with "." (but not a whole component equal to
- * "." or "..").
+ * "." or "..").  If REFNAME_FULL is set in flags, then additionally
+ * verify that the refname is a valid full refname--that it either
+ * starts with "refs/" or that it consists of only capital letters and
+ * underscores (like "HEAD" or "MERGE_HEAD").
  */
 extern int check_refname_format(const char *ref, int flags);
 
diff --git a/t/t1402-check-ref-format.sh b/t/t1402-check-ref-format.sh
index 1ae4d87..2f8a48e 100755
--- a/t/t1402-check-ref-format.sh
+++ b/t/t1402-check-ref-format.sh
@@ -70,6 +70,7 @@ invalid_ref "$ref" --refspec-pattern
 valid_ref "$ref" '--refspec-pattern --allow-onelevel'
 invalid_ref "$ref" --normalize
 valid_ref "$ref" '--allow-onelevel --normalize'
+invalid_ref "$ref" '--full'
 
 ref='foo/bar'
 valid_ref "$ref"
@@ -77,12 +78,19 @@ valid_ref "$ref" --allow-onelevel
 valid_ref "$ref" --refspec-pattern
 valid_ref "$ref" '--refspec-pattern --allow-onelevel'
 valid_ref "$ref" --normalize
+invalid_ref "$ref" '--full'
 
 ref='foo/*'
 invalid_ref "$ref"
 invalid_ref "$ref" --allow-onelevel
 valid_ref "$ref" --refspec-pattern
 valid_ref "$ref" '--refspec-pattern --allow-onelevel'
+invalid_ref "$ref" '--full'
+invalid_ref "$ref" '--refspec-pattern --full'
+
+ref='refs/*'
+invalid_ref "$ref" '--full'
+valid_ref "$ref" '--refspec-pattern --full'
 
 ref='*/foo'
 invalid_ref "$ref"
@@ -91,18 +99,23 @@ valid_ref "$ref" --refspec-pattern
 valid_ref "$ref" '--refspec-pattern --allow-onelevel'
 invalid_ref "$ref" --normalize
 valid_ref "$ref" '--refspec-pattern --normalize'
+invalid_ref "$ref" '--full'
+invalid_ref "$ref" '--refspec-pattern --full'
 
 ref='foo/*/bar'
 invalid_ref "$ref"
 invalid_ref "$ref" --allow-onelevel
 valid_ref "$ref" --refspec-pattern
 valid_ref "$ref" '--refspec-pattern --allow-onelevel'
+invalid_ref "$ref" '--full'
 
 ref='*'
 invalid_ref "$ref"
 invalid_ref "$ref" --allow-onelevel
 invalid_ref "$ref" --refspec-pattern
 valid_ref "$ref" '--refspec-pattern --allow-onelevel'
+invalid_ref "$ref" '--full'
+invalid_ref "$ref" '--refspec-pattern --full'
 
 ref='foo/*/*'
 invalid_ref "$ref" --refspec-pattern
@@ -126,6 +139,24 @@ valid_ref NOT_MINGW "$ref" '--allow-onelevel --normalize'
 invalid_ref NOT_MINGW "$ref" '--refspec-pattern --normalize'
 valid_ref NOT_MINGW "$ref" '--refspec-pattern --allow-onelevel --normalize'
 
+ref='HEAD'
+invalid_ref "$ref"
+valid_ref "$ref" --allow-onelevel
+invalid_ref "$ref" --full
+valid_ref "$ref" '--allow-onelevel --full'
+
+valid_ref FETCH_HEAD '--allow-onelevel --full'
+valid_ref refs/foo --full
+invalid_ref HEAD/ '--allow-onelevel --full'
+valid_ref HEAD/ '--allow-onelevel --full --normalize'
+invalid_ref refs '--full'
+invalid_ref refs '--allow-onelevel --full'
+invalid_ref refs/ '--allow-onelevel --full'
+invalid_ref HEAD/foo '--full'
+invalid_ref head '--allow-onelevel --full'
+valid_ref 'refs/foo/*' '--refspec-pattern --full'
+valid_ref 'refs/*/foo' '--refspec-pattern --full'
+
 test_expect_success "check-ref-format --branch @{-1}" '
 	T=$(git write-tree) &&
 	sha1=$(echo A | git commit-tree $T) &&
-- 
1.7.7

[RFC 04/13] add_ref(): move the call of check_refname_format() to callers

[mhagger]

From: Michael Haggerty <mhagger@alum.mit.edu>

Do not call check_refname_format() in add_ref(); instead change its
callers to do the check.  (In fact, don't do any checking in
add_extra_ref(), because that function handles bizarre things like
"refs/tags/3.1.1.1^{}".)

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---

I'm still not clear on how extra_refs are used.  Are they generated
from local refs or are they generated from remote refs?  If the
latter, then it is probably irresponsible not to do *some* sanity
checking in add_extra_ref() to prevent any chance of refnames like
"../../../etc/passwd".

 refs.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/refs.c b/refs.c
index 8299e51..a40dfa5 100644
--- a/refs.c
+++ b/refs.c
@@ -60,8 +60,6 @@ static void add_ref(const char *name, const unsigned char *sha1,
 	entry = xmalloc(sizeof(struct ref_entry) + len);
 	hashcpy(entry->sha1, sha1);
 	hashclr(entry->peeled);
-	if (check_refname_format(name, REFNAME_ALLOW_ONELEVEL|REFNAME_DOT_COMPONENT))
-		die("Reference has invalid format: '%s'", name);
 	memcpy(entry->name, name, len);
 	entry->flag = flag;
 	if (new_entry)
@@ -232,6 +230,8 @@ static void read_packed_refs(FILE *f, struct ref_array *array)
 
 		name = parse_ref_line(refline, sha1);
 		if (name) {
+			if (check_refname_format(name, REFNAME_FULL))
+				die("Reference has invalid format: '%s'", name);
 			add_ref(name, sha1, flag, array, &last);
 			continue;
 		}
@@ -336,6 +336,8 @@ static void get_ref_dir(const char *submodule, const char *base,
 					hashclr(sha1);
 					flag |= REF_BROKEN;
 				}
+			if (check_refname_format(ref, REFNAME_FULL))
+				die("Reference has invalid format: '%s'", ref);
 			add_ref(ref, sha1, flag, array, NULL);
 		}
 		free(ref);
-- 
1.7.7

[RFC 05/13] receive-pack::update(): use check_refname_format(..., REFNAME_FULL)

[mhagger]

From: Michael Haggerty <mhagger@alum.mit.edu>

Replace local code with a use of check_refname_format()'s new
REFNAME_FULL option.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 builtin/receive-pack.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 261b610..508451b 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -401,7 +401,7 @@ static const char *update(struct command *cmd)
 	struct ref_lock *lock;
 
 	/* only refs/... are allowed */
-	if (prefixcmp(name, "refs/") || check_refname_format(name + 5, 0)) {
+	if (check_refname_format(name, REFNAME_FULL)) {
 		rp_error("refusing to create funny ref '%s' remotely", name);
 		return "funny refname";
 	}
-- 
1.7.7

[RFC 06/13] strbuf_check_branch_ref(): use check_refname_format(..., REFNAME_FULL)

[mhagger]

From: Michael Haggerty <mhagger@alum.mit.edu>

The reference name should be a full one, so adjust the check
accordingly.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 sha1_name.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/sha1_name.c b/sha1_name.c
index 03ffc2c..335da37 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -883,7 +883,7 @@ int strbuf_check_branch_ref(struct strbuf *sb, const char *name)
 	if (name[0] == '-')
 		return -1;
 	strbuf_splice(sb, 0, 0, "refs/heads/", 11);
-	return check_refname_format(sb->buf, 0);
+	return check_refname_format(sb->buf, REFNAME_FULL);
 }
 
 /*
-- 
1.7.7

[RFC 07/13] one_local_ref(): use check_refname_format(..., REFNAME_FULL)

[mhagger]

From: Michael Haggerty <mhagger@alum.mit.edu>

Instead of manually skipping over "refs/", let check_refname_format()
handle the whole refname.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 remote.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/remote.c b/remote.c
index e52aa9b..30ee6d8 100644
--- a/remote.c
+++ b/remote.c
@@ -1595,7 +1595,7 @@ static int one_local_ref(const char *refname, const unsigned char *sha1, int fla
 	int len;
 
 	/* we already know it starts with refs/ to get here */
-	if (check_refname_format(refname + 5, 0))
+	if (check_refname_format(refname, REFNAME_FULL))
 		return 0;
 
 	len = strlen(refname) + 1;
-- 
1.7.7

[RFC 08/13] expand_namespace(): the refname is full, so use REFNAME_FULL option

[mhagger]

From: Michael Haggerty <mhagger@alum.mit.edu>


Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 environment.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/environment.c b/environment.c
index 0bee6a7..a6d6f04 100644
--- a/environment.c
+++ b/environment.c
@@ -107,7 +107,7 @@ static char *expand_namespace(const char *raw_namespace)
 		if (strcmp((*c)->buf, "/") != 0)
 			strbuf_addf(&buf, "refs/namespaces/%s", (*c)->buf);
 	strbuf_list_free(components);
-	if (check_refname_format(buf.buf, 0))
+	if (check_refname_format(buf.buf, REFNAME_FULL))
 		die("bad git namespace path \"%s\"", raw_namespace);
 	strbuf_addch(&buf, '/');
 	return strbuf_detach(&buf, NULL);
-- 
1.7.7

[RFC 09/13] new_branch(): verify that new branch name is a valid full refname

[mhagger]

From: Michael Haggerty <mhagger@alum.mit.edu>


Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---

Is it possible to omit the REFNAME_ALLOW_ONELEVEL option from this
call?

 fast-import.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/fast-import.c b/fast-import.c
index 8d8ea3c..51cf898 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -722,7 +722,7 @@ static struct branch *new_branch(const char *name)
 
 	if (b)
 		die("Invalid attempt to create duplicate branch: %s", name);
-	if (check_refname_format(name, REFNAME_ALLOW_ONELEVEL))
+	if (check_refname_format(name, REFNAME_FULL|REFNAME_ALLOW_ONELEVEL))
 		die("Branch name doesn't conform to GIT standards: %s", name);
 
 	b = pool_calloc(1, sizeof(struct branch));
-- 
1.7.7

[RFC 10/13] strbuf_check_tag_ref(): the refname is full, so use REFNAME_FULL option

[mhagger]

From: Michael Haggerty <mhagger@alum.mit.edu>


Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 builtin/tag.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/builtin/tag.c b/builtin/tag.c
index 9b6fd95..abf3cb0 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -412,7 +412,7 @@ static int strbuf_check_tag_ref(struct strbuf *sb, const char *name)
 	strbuf_reset(sb);
 	strbuf_addf(sb, "refs/tags/%s", name);
 
-	return check_refname_format(sb->buf, 0);
+	return check_refname_format(sb->buf, REFNAME_FULL);
 }
 
 int cmd_tag(int argc, const char **argv, const char *prefix)
-- 
1.7.7

[RFC 11/13] replace_object(): the refname is full, so use REFNAME_FULL option

[mhagger]

From: Michael Haggerty <mhagger@alum.mit.edu>


Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 builtin/replace.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/builtin/replace.c b/builtin/replace.c
index 517fa10..f5d4f77 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -94,7 +94,7 @@ static int replace_object(const char *object_ref, const char *replace_ref,
 		     "refs/replace/%s",
 		     sha1_to_hex(object)) > sizeof(ref) - 1)
 		die("replace ref name too long: %.*s...", 50, ref);
-	if (check_refname_format(ref, 0))
+	if (check_refname_format(ref, REFNAME_FULL))
 		die("'%s' is not a valid ref name.", ref);
 
 	if (!resolve_ref(ref, prev, 1, NULL))
-- 
1.7.7

[RFC 12/13] resolve_ref: use check_refname_format(..., REFNAME_FULL)

[mhagger]

From: Michael Haggerty <mhagger@alum.mit.edu>

...instead of own inline code.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/refs.c b/refs.c
index a40dfa5..67df3a6 100644
--- a/refs.c
+++ b/refs.c
@@ -548,8 +548,7 @@ const char *resolve_ref(const char *ref, unsigned char *sha1, int reading, int *
 			if (len < 0)
 				return NULL;
 			buffer[len] = 0;
-			if (!prefixcmp(buffer, "refs/") &&
-					!check_refname_format(buffer, 0)) {
+			if (!check_refname_format(buffer, REFNAME_FULL)) {
 				strcpy(ref_buffer, buffer);
 				ref = ref_buffer;
 				if (flag)
-- 
1.7.7

[RFC 13/13] filter_refs(): the refname is full, so use REFNAME_FULL option

[mhagger]

From: Michael Haggerty <mhagger@alum.mit.edu>


Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 builtin/fetch-pack.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index c6bc8eb..d72aa44 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -546,7 +546,7 @@ static void filter_refs(struct ref **refs, int nr_match, char **match)
 	for (ref = *refs; ref; ref = next) {
 		next = ref->next;
 		if (!memcmp(ref->name, "refs/", 5) &&
-		    check_refname_format(ref->name + 5, 0))
+		    check_refname_format(ref->name, REFNAME_FULL))
 			; /* trash */
 		else if (args.fetch_all &&
 			 (!args.depth || prefixcmp(ref->name, "refs/tags/") )) {
-- 
1.7.7

Re: [RFC 04/13] add_ref(): move the call of check_refname_format() to callers

[Junio C Hamano]

mhagger@alum.mit.edu writes:

> I'm still not clear on how extra_refs are used.  Are they generated
> from local refs or are they generated from remote refs?  If the
> latter, then it is probably irresponsible not to do *some* sanity
> checking in add_extra_ref() to prevent any chance of refnames like
> "../../../etc/passwd".

No, add_extra_ref() already tells us what their values are, these are
never used to actually read from filesystem. Their refname field has
almost no value other than for debugging and we probably shouldn't even
insist on uniqueness among extra refs or for that matter collision with
the real refs. As I mentioned in an earlier message, their only raison
d'être is to be found by for_each_ref() that feeds revision machinery with
up to which commits we know we have complete histories for. A sample call
chain looks like this:

 - cmd_clone()
  - setup_reference()
   - add_one_reference()
     - add_extra_ref()
       adds refs in other repositories we borrow from as "extra"
  - transport_fetch_refs()
   - fetch_refs_via_pack()
    - fetch_pack()
     - do_fetch_pack()
      - find_common()
       - for_each_ref(rev_list_insert_ref)

That way find_common() thinks histories leading to these extra refs are
already complete on our end (i.e. we have all the necessary objects), and
by subtracting that from what we are asking from the other end, we can
reduce the amount of history that needs to be transferred.

Re: [RFC 09/13] new_branch(): verify that new branch name is a valid full refname

[Junio C Hamano]

mhagger@alum.mit.edu writes:

> From: Michael Haggerty <mhagger@alum.mit.edu>
>
>
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---
>
> Is it possible to omit the REFNAME_ALLOW_ONELEVEL option from this
> call?

I _think_ it takes an unadorned branch name, so the most prudent would be
to check the result of prefixing "refs/heads/" to *name with REFNAME_FULL.

>  fast-import.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/fast-import.c b/fast-import.c
> index 8d8ea3c..51cf898 100644
> --- a/fast-import.c
> +++ b/fast-import.c
> @@ -722,7 +722,7 @@ static struct branch *new_branch(const char *name)
>  
>  	if (b)
>  		die("Invalid attempt to create duplicate branch: %s", name);
> -	if (check_refname_format(name, REFNAME_ALLOW_ONELEVEL))
> +	if (check_refname_format(name, REFNAME_FULL|REFNAME_ALLOW_ONELEVEL))
>  		die("Branch name doesn't conform to GIT standards: %s", name);
>  
>  	b = pool_calloc(1, sizeof(struct branch));

Re: [RFC 04/13] add_ref(): move the call of check_refname_format() to callers

[Michael Haggerty]

On 10/19/2011 11:49 PM, Junio C Hamano wrote:
> mhagger@alum.mit.edu writes:
>> I'm still not clear on how extra_refs are used.  Are they generated
>> from local refs or are they generated from remote refs?  If the
>> latter, then it is probably irresponsible not to do *some* sanity
>> checking in add_extra_ref() to prevent any chance of refnames like
>> "../../../etc/passwd".
> 
> No, add_extra_ref() already tells us what their values are, these are
> never used to actually read from filesystem. Their refname field has
> almost no value other than for debugging and we probably shouldn't even
> insist on uniqueness among extra refs or for that matter collision with
> the real refs. [...]

Thanks for the explanation.  I'm inspired to separate them a little bit
more from "real" refs because they are such a special case.  For
example, maybe it would make sense to add a function
for_each_extra_ref() to avoid having to mix them with real refs in the
iteration.  OTOH not important AFAICS.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/