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

2012-07-16 Thread Junio C Hamano
Junio C Hamano  writes:

> Michael Schubert  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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

2012-07-16 Thread Junio C Hamano
Michael Schubert  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);
>  }
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

2012-07-16 Thread 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
mhag...@alum.mit.edu
http://softwareswirl.blogspot.com/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html