threads / patch / 31035

patchrefname format cleanup

Subject: [PATCH] refname format cleanup

## tl;dr

8 messages between Jul 16, 2012 and Jul 16, 2012. Diffs are folded; open one to read it.

replies: 7people: 3as markdown or json

Michael Schubert· Jul 16, 2012, 12:12 UTC · lore
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(-)
Michael Schubert· Jul 16, 2012, 12:13 UTC · re: Michael Schubert · lore

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

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(-)
Show changes to 3 files +2 −5

builtin/tag.c, refs.c, sha1_name.c

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
Michael Haggerty· Jul 16, 2012, 13:18 UTC · re: Michael Schubert · lore

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

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/
Junio C Hamano· Jul 16, 2012, 17:06 UTC · re: Michael Schubert · lore

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

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.

Show 40 quoted lines
> 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);
>  }
Junio C Hamano· Jul 16, 2012, 17:49 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <gitster@pobox.com> writes:
Show 10 quoted lines
> 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.

Michael Schubert· Jul 16, 2012, 12:13 UTC · re: Michael Schubert · lore

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

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(-)
Show changes to 2 files +13 −1

builtin/symbolic-ref.c, t/t1401-symbolic-ref.sh

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
Michael Haggerty· Jul 16, 2012, 13:24 UTC · re: Michael Schubert · lore

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

On 07/16/2012 02:13 PM, Michael Schubert wrote:
Show 34 quoted lines
> 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/
Junio C Hamano· Jul 16, 2012, 17:12 UTC · re: Michael Schubert · lore

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

Michael Schubert <mschub@elegosoft.com> writes:
Show 37 quoted lines
> 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?
Show 21 quoted lines
> 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`

← back to recent threads