[PATCH] refname format cleanup

[PATCH] refname format cleanup

[Michael Schubert]

Previous discussion:

 http://thread.gmane.org/gmane.comp.version-control.git/200129/focus=200146

I'm not sure if I've drawn the right conclusions from the previous
thread, so please let me know in case that's the wrong way to go..

 * refs: disallow ref components starting with hyphen
 * symbolic-ref: check format of given refname

 builtin/symbolic-ref.c  |  4 +++-
 builtin/tag.c           |  3 ---
 refs.c                  |  2 ++
 sha1_name.c             |  2 --
 t/t1401-symbolic-ref.sh | 10 ++++++++++
 5 files changed, 15 insertions(+), 6 deletions(-)

[PATCH 1/2] refs: disallow ref components starting with hyphen

[Michael Schubert]

Currently, we allow refname components to start with a hyphen. There's
no good reason to do so and it troubles the parseopt infrastructure.
Explicitly refuse refname components starting with a hyphen inside
check_refname_component().

Revert 63486240, which is obsolete now.

Signed-off-by: Michael Schubert <mschub@elegosoft.com>
---
 builtin/tag.c | 3 ---
 refs.c        | 2 ++
 sha1_name.c   | 2 --
 3 files changed, 2 insertions(+), 5 deletions(-)

diff --git a/builtin/tag.c b/builtin/tag.c
index 7b1be85..c99fb42 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -403,9 +403,6 @@ static int parse_msg_arg(const struct option *opt, const char *arg, int unset)
 
 static int strbuf_check_tag_ref(struct strbuf *sb, const char *name)
 {
-	if (name[0] == '-')
-		return -1;
-
 	strbuf_reset(sb);
 	strbuf_addf(sb, "refs/tags/%s", name);
 
diff --git a/refs.c b/refs.c
index da74a2b..5714681 100644
--- a/refs.c
+++ b/refs.c
@@ -62,6 +62,8 @@ static int check_refname_component(const char *refname, int flags)
 		if (refname[1] == '\0')
 			return -1; /* Component equals ".". */
 	}
+	if (refname[0] == '-')
+		return -1; /* Component starts with '-'. */
 	if (cp - refname >= 5 && !memcmp(cp - 5, ".lock", 5))
 		return -1; /* Refname ends with ".lock". */
 	return cp - refname;
diff --git a/sha1_name.c b/sha1_name.c
index 5d81ea0..132d369 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -892,8 +892,6 @@ int strbuf_branchname(struct strbuf *sb, const char *name)
 int strbuf_check_branch_ref(struct strbuf *sb, const char *name)
 {
 	strbuf_branchname(sb, name);
-	if (name[0] == '-')
-		return -1;
 	strbuf_splice(sb, 0, 0, "refs/heads/", 11);
 	return check_refname_format(sb->buf, 0);
 }
-- 
1.7.11.2.196.ga22866b

Re: [PATCH 1/2] refs: disallow ref components starting with hyphen

[Michael Haggerty]

On 07/16/2012 02:13 PM, Michael Schubert wrote:
> Currently, we allow refname components to start with a hyphen. There's
> no good reason to do so and it troubles the parseopt infrastructure.
> Explicitly refuse refname components starting with a hyphen inside
> check_refname_component().

Your change to refs.c looks correct.  However, you should also update 
the documentation of the refname rules at the top of refs.c and also in

     Documentation/git-check-ref-format.txt

Michael

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

Re: [PATCH 1/2] refs: disallow ref components starting with hyphen

[Junio C Hamano]

Michael Schubert <mschub@elegosoft.com> writes:

> Currently, we allow refname components to start with a hyphen. There's
> no good reason to do so...

That is way too weak as a justification to potentially break
existing repositories.

Refusal upon attempted creation is probably OK, which is why the two
checks you removed in your patches are fine.  I do not know if it is
justifiable to do that in check_refname_component() that is used in
the reading codepath.

> diff --git a/builtin/tag.c b/builtin/tag.c
> index 7b1be85..c99fb42 100644
> --- a/builtin/tag.c
> +++ b/builtin/tag.c
> @@ -403,9 +403,6 @@ static int parse_msg_arg(const struct option *opt, const char *arg, int unset)
>  
>  static int strbuf_check_tag_ref(struct strbuf *sb, const char *name)
>  {
> -	if (name[0] == '-')
> -		return -1;
> -
>  	strbuf_reset(sb);
>  	strbuf_addf(sb, "refs/tags/%s", name);
>  
> diff --git a/refs.c b/refs.c
> index da74a2b..5714681 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -62,6 +62,8 @@ static int check_refname_component(const char *refname, int flags)
>  		if (refname[1] == '\0')
>  			return -1; /* Component equals ".". */
>  	}
> +	if (refname[0] == '-')
> +		return -1; /* Component starts with '-'. */
>  	if (cp - refname >= 5 && !memcmp(cp - 5, ".lock", 5))
>  		return -1; /* Refname ends with ".lock". */
>  	return cp - refname;
> diff --git a/sha1_name.c b/sha1_name.c
> index 5d81ea0..132d369 100644
> --- a/sha1_name.c
> +++ b/sha1_name.c
> @@ -892,8 +892,6 @@ int strbuf_branchname(struct strbuf *sb, const char *name)
>  int strbuf_check_branch_ref(struct strbuf *sb, const char *name)
>  {
>  	strbuf_branchname(sb, name);
> -	if (name[0] == '-')
> -		return -1;
>  	strbuf_splice(sb, 0, 0, "refs/heads/", 11);
>  	return check_refname_format(sb->buf, 0);
>  }

Re: [PATCH 1/2] refs: disallow ref components starting with hyphen

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Michael Schubert <mschub@elegosoft.com> writes:
>
>> Currently, we allow refname components to start with a hyphen. There's
>> no good reason to do so...
>
> That is way too weak as a justification to potentially break
> existing repositories.
>
> Refusal upon attempted creation is probably OK, which is why the two
> checks you removed in your patches are fine.

Just to clarify, I meant that the existing checks were OK because
they were meant to prevent creation.  I didn't mean removal of them
was OK.

[PATCH 2/2] symbolic-ref: check format of given refname

[Michael Schubert]

Currently, it's possible to update HEAD with a nonsense reference since
no strict validation ist performed. Example:

	$ git symbolic-ref HEAD 'refs/heads/master
    >
    >
    > '

Fix this by checking the given reference with check_refname_format().

Signed-off-by: Michael Schubert <mschub@elegosoft.com>
---
 builtin/symbolic-ref.c  |  4 +++-
 t/t1401-symbolic-ref.sh | 10 ++++++++++
 2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/builtin/symbolic-ref.c b/builtin/symbolic-ref.c
index 801d62e..a529541 100644
--- a/builtin/symbolic-ref.c
+++ b/builtin/symbolic-ref.c
@@ -44,13 +44,15 @@ int cmd_symbolic_ref(int argc, const char **argv, const char *prefix)
 	git_config(git_default_config, NULL);
 	argc = parse_options(argc, argv, prefix, options,
 			     git_symbolic_ref_usage, 0);
-	if (msg &&!*msg)
+	if (msg && !*msg)
 		die("Refusing to perform update with empty message");
 	switch (argc) {
 	case 1:
 		check_symref(argv[0], quiet);
 		break;
 	case 2:
+		if (check_refname_format(argv[1], 0))
+			die("No valid reference format: '%s'", argv[1]);
 		if (!strcmp(argv[0], "HEAD") &&
 		    prefixcmp(argv[1], "refs/"))
 			die("Refusing to point HEAD outside of refs/");
diff --git a/t/t1401-symbolic-ref.sh b/t/t1401-symbolic-ref.sh
index 2c96551..b1cd508 100755
--- a/t/t1401-symbolic-ref.sh
+++ b/t/t1401-symbolic-ref.sh
@@ -27,6 +27,16 @@ test_expect_success 'symbolic-ref refuses non-ref for HEAD' '
 '
 reset_to_sane
 
+test_expect_success 'symbolic-ref refuses ref with leading dot' '
+	test_must_fail git symbolic-ref HEAD refs/heads/.foo
+'
+reset_to_sane
+
+test_expect_success 'symbolic-ref refuses ref with leading dash' '
+	test_must_fail git symbolic-ref HEAD refs/heads/-foo
+'
+reset_to_sane
+
 test_expect_success 'symbolic-ref refuses bare sha1' '
 	echo content >file && git add file && git commit -m one &&
 	test_must_fail git symbolic-ref HEAD `git rev-parse HEAD`
-- 
1.7.11.2.196.ga22866b

Re: [PATCH 2/2] symbolic-ref: check format of given refname

[Michael Haggerty]

On 07/16/2012 02:13 PM, Michael Schubert wrote:
> Currently, it's possible to update HEAD with a nonsense reference since
> no strict validation ist performed. Example:
>
> 	$ git symbolic-ref HEAD 'refs/heads/master
>      >
>      >
>      > '
>
> Fix this by checking the given reference with check_refname_format().
>
> Signed-off-by: Michael Schubert <mschub@elegosoft.com>
> ---
>   builtin/symbolic-ref.c  |  4 +++-
>   t/t1401-symbolic-ref.sh | 10 ++++++++++
>   2 files changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/symbolic-ref.c b/builtin/symbolic-ref.c
> index 801d62e..a529541 100644
> --- a/builtin/symbolic-ref.c
> +++ b/builtin/symbolic-ref.c
> @@ -44,13 +44,15 @@ int cmd_symbolic_ref(int argc, const char **argv, const char *prefix)
>   	git_config(git_default_config, NULL);
>   	argc = parse_options(argc, argv, prefix, options,
>   			     git_symbolic_ref_usage, 0);
> -	if (msg &&!*msg)
> +	if (msg && !*msg)
>   		die("Refusing to perform update with empty message");
>   	switch (argc) {
>   	case 1:
>   		check_symref(argv[0], quiet);
>   		break;
>   	case 2:
> +		if (check_refname_format(argv[1], 0))
> +			die("No valid reference format: '%s'", argv[1]);

The error message is awkward.  I suggest something like

     "Reference name has invalid format: '%s'"

Michael

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

Re: [PATCH 2/2] symbolic-ref: check format of given refname

[Junio C Hamano]

Michael Schubert <mschub@elegosoft.com> writes:

> Currently, it's possible to update HEAD with a nonsense reference since
> no strict validation ist performed. Example:
>
> 	$ git symbolic-ref HEAD 'refs/heads/master
>     >
>     >
>     > '
>
> Fix this by checking the given reference with check_refname_format().
>
> Signed-off-by: Michael Schubert <mschub@elegosoft.com>
> ---
>  builtin/symbolic-ref.c  |  4 +++-
>  t/t1401-symbolic-ref.sh | 10 ++++++++++
>  2 files changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/symbolic-ref.c b/builtin/symbolic-ref.c
> index 801d62e..a529541 100644
> --- a/builtin/symbolic-ref.c
> +++ b/builtin/symbolic-ref.c
> @@ -44,13 +44,15 @@ int cmd_symbolic_ref(int argc, const char **argv, const char *prefix)
>  	git_config(git_default_config, NULL);
>  	argc = parse_options(argc, argv, prefix, options,
>  			     git_symbolic_ref_usage, 0);
> -	if (msg &&!*msg)
> +	if (msg && !*msg)
>  		die("Refusing to perform update with empty message");
>  	switch (argc) {
>  	case 1:
>  		check_symref(argv[0], quiet);
>  		break;
>  	case 2:
> +		if (check_refname_format(argv[1], 0))
> +			die("No valid reference format: '%s'", argv[1]);
>  		if (!strcmp(argv[0], "HEAD") &&
>  		    prefixcmp(argv[1], "refs/"))
>  			die("Refusing to point HEAD outside of refs/");

The existing context lines above may give a clue why this patch is
not such a good idea.  We only limit HEAD to point under refs/ but
allow advanced users and scripts creative uses of other kinds of
symrefs.  Shouldn't the patch apply the new restriction only to HEAD
as well?

By the way, should "git symbolic-ref _ HEAD" work?

> diff --git a/t/t1401-symbolic-ref.sh b/t/t1401-symbolic-ref.sh
> index 2c96551..b1cd508 100755
> --- a/t/t1401-symbolic-ref.sh
> +++ b/t/t1401-symbolic-ref.sh
> @@ -27,6 +27,16 @@ test_expect_success 'symbolic-ref refuses non-ref for HEAD' '
>  '
>  reset_to_sane
>  
> +test_expect_success 'symbolic-ref refuses ref with leading dot' '
> +	test_must_fail git symbolic-ref HEAD refs/heads/.foo
> +'
> +reset_to_sane
> +
> +test_expect_success 'symbolic-ref refuses ref with leading dash' '
> +	test_must_fail git symbolic-ref HEAD refs/heads/-foo
> +'
> +reset_to_sane
> +
>  test_expect_success 'symbolic-ref refuses bare sha1' '
>  	echo content >file && git add file && git commit -m one &&
>  	test_must_fail git symbolic-ref HEAD `git rev-parse HEAD`