{"thread":{"id":"31035","subject":"[PATCH] refname format cleanup","startedAt":"2012-07-16T12:12:59Z","lastAt":"2012-07-16T17:49:26Z","messageCount":8,"participants":["Michael Schubert","Michael Haggerty","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"195115","messageId":"1342440781-18816-1-git-send-email-mschub@elegosoft.com","threadId":"31035","inReplyTo":null,"subject":"[PATCH] refname format cleanup","fromName":"Michael Schubert","fromEmail":"mschub@elegosoft.com","sentAt":"2012-07-16T12:12:59Z","receivedAt":"2012-07-16T12:12:59Z","isPatch":true,"sender":{"key":"mschub@elegosoft.com","avatar":null},"body":"Previous discussion:\n\n http://thread.gmane.org/gmane.comp.version-control.git/200129/focus=200146\n\nI'm not sure if I've drawn the right conclusions from the previous\nthread, so please let me know in case that's the wrong way to go..\n\n * refs: disallow ref components starting with hyphen\n * symbolic-ref: check format of given refname\n\n builtin/symbolic-ref.c  |  4 +++-\n builtin/tag.c           |  3 ---\n refs.c                  |  2 ++\n sha1_name.c             |  2 --\n t/t1401-symbolic-ref.sh | 10 ++++++++++\n 5 files changed, 15 insertions(+), 6 deletions(-)\n"},{"id":"195116","messageId":"1342440781-18816-2-git-send-email-mschub@elegosoft.com","threadId":"31035","inReplyTo":"1342440781-18816-1-git-send-email-mschub@elegosoft.com","subject":"[PATCH 1/2] refs: disallow ref components starting with hyphen","fromName":"Michael Schubert","fromEmail":"mschub@elegosoft.com","sentAt":"2012-07-16T12:13:00Z","receivedAt":"2012-07-16T12:13:00Z","isPatch":true,"sender":{"key":"mschub@elegosoft.com","avatar":null},"body":"Currently, we allow refname components to start with a hyphen. There's\nno good reason to do so and it troubles the parseopt infrastructure.\nExplicitly refuse refname components starting with a hyphen inside\ncheck_refname_component().\n\nRevert 63486240, which is obsolete now.\n\nSigned-off-by: Michael Schubert <mschub@elegosoft.com>\n---\n builtin/tag.c | 3 ---\n refs.c        | 2 ++\n sha1_name.c   | 2 --\n 3 files changed, 2 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex 7b1be85..c99fb42 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -403,9 +403,6 @@ static int parse_msg_arg(const struct option *opt, const char *arg, int unset)\n \n static int strbuf_check_tag_ref(struct strbuf *sb, const char *name)\n {\n-\tif (name[0] == '-')\n-\t\treturn -1;\n-\n \tstrbuf_reset(sb);\n \tstrbuf_addf(sb, \"refs/tags/%s\", name);\n \ndiff --git a/refs.c b/refs.c\nindex da74a2b..5714681 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -62,6 +62,8 @@ static int check_refname_component(const char *refname, int flags)\n \t\tif (refname[1] == '\\0')\n \t\t\treturn -1; /* Component equals \".\". */\n \t}\n+\tif (refname[0] == '-')\n+\t\treturn -1; /* Component starts with '-'. */\n \tif (cp - refname >= 5 && !memcmp(cp - 5, \".lock\", 5))\n \t\treturn -1; /* Refname ends with \".lock\". */\n \treturn cp - refname;\ndiff --git a/sha1_name.c b/sha1_name.c\nindex 5d81ea0..132d369 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -892,8 +892,6 @@ int strbuf_branchname(struct strbuf *sb, const char *name)\n int strbuf_check_branch_ref(struct strbuf *sb, const char *name)\n {\n \tstrbuf_branchname(sb, name);\n-\tif (name[0] == '-')\n-\t\treturn -1;\n \tstrbuf_splice(sb, 0, 0, \"refs/heads/\", 11);\n \treturn check_refname_format(sb->buf, 0);\n }\n-- \n1.7.11.2.196.ga22866b\n"},{"id":"195117","messageId":"1342440781-18816-3-git-send-email-mschub@elegosoft.com","threadId":"31035","inReplyTo":"1342440781-18816-1-git-send-email-mschub@elegosoft.com","subject":"[PATCH 2/2] symbolic-ref: check format of given refname","fromName":"Michael Schubert","fromEmail":"mschub@elegosoft.com","sentAt":"2012-07-16T12:13:01Z","receivedAt":"2012-07-16T12:13:01Z","isPatch":true,"sender":{"key":"mschub@elegosoft.com","avatar":null},"body":"Currently, it's possible to update HEAD with a nonsense reference since\nno strict validation ist performed. Example:\n\n\t$ git symbolic-ref HEAD 'refs/heads/master\n    >\n    >\n    > '\n\nFix this by checking the given reference with check_refname_format().\n\nSigned-off-by: Michael Schubert <mschub@elegosoft.com>\n---\n builtin/symbolic-ref.c  |  4 +++-\n t/t1401-symbolic-ref.sh | 10 ++++++++++\n 2 files changed, 13 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/symbolic-ref.c b/builtin/symbolic-ref.c\nindex 801d62e..a529541 100644\n--- a/builtin/symbolic-ref.c\n+++ b/builtin/symbolic-ref.c\n@@ -44,13 +44,15 @@ int cmd_symbolic_ref(int argc, const char **argv, const char *prefix)\n \tgit_config(git_default_config, NULL);\n \targc = parse_options(argc, argv, prefix, options,\n \t\t\t     git_symbolic_ref_usage, 0);\n-\tif (msg &&!*msg)\n+\tif (msg && !*msg)\n \t\tdie(\"Refusing to perform update with empty message\");\n \tswitch (argc) {\n \tcase 1:\n \t\tcheck_symref(argv[0], quiet);\n \t\tbreak;\n \tcase 2:\n+\t\tif (check_refname_format(argv[1], 0))\n+\t\t\tdie(\"No valid reference format: '%s'\", argv[1]);\n \t\tif (!strcmp(argv[0], \"HEAD\") &&\n \t\t    prefixcmp(argv[1], \"refs/\"))\n \t\t\tdie(\"Refusing to point HEAD outside of refs/\");\ndiff --git a/t/t1401-symbolic-ref.sh b/t/t1401-symbolic-ref.sh\nindex 2c96551..b1cd508 100755\n--- a/t/t1401-symbolic-ref.sh\n+++ b/t/t1401-symbolic-ref.sh\n@@ -27,6 +27,16 @@ test_expect_success 'symbolic-ref refuses non-ref for HEAD' '\n '\n reset_to_sane\n \n+test_expect_success 'symbolic-ref refuses ref with leading dot' '\n+\ttest_must_fail git symbolic-ref HEAD refs/heads/.foo\n+'\n+reset_to_sane\n+\n+test_expect_success 'symbolic-ref refuses ref with leading dash' '\n+\ttest_must_fail git symbolic-ref HEAD refs/heads/-foo\n+'\n+reset_to_sane\n+\n test_expect_success 'symbolic-ref refuses bare sha1' '\n \techo content >file && git add file && git commit -m one &&\n \ttest_must_fail git symbolic-ref HEAD `git rev-parse HEAD`\n-- \n1.7.11.2.196.ga22866b\n"},{"id":"195119","messageId":"500414A0.9080802@alum.mit.edu","threadId":"31035","inReplyTo":"1342440781-18816-2-git-send-email-mschub@elegosoft.com","subject":"Re: [PATCH 1/2] refs: disallow ref components starting with hyphen","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-07-16T13:18:24Z","receivedAt":"2012-07-16T13:18:24Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 07/16/2012 02:13 PM, Michael Schubert wrote:\n> Currently, we allow refname components to start with a hyphen. There's\n> no good reason to do so and it troubles the parseopt infrastructure.\n> Explicitly refuse refname components starting with a hyphen inside\n> check_refname_component().\n\nYour change to refs.c looks correct.  However, you should also update \nthe documentation of the refname rules at the top of refs.c and also in\n\n     Documentation/git-check-ref-format.txt\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"195120","messageId":"500415F5.3060600@alum.mit.edu","threadId":"31035","inReplyTo":"1342440781-18816-3-git-send-email-mschub@elegosoft.com","subject":"Re: [PATCH 2/2] symbolic-ref: check format of given refname","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-07-16T13:24:05Z","receivedAt":"2012-07-16T13:24:05Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 07/16/2012 02:13 PM, Michael Schubert wrote:\n> Currently, it's possible to update HEAD with a nonsense reference since\n> no strict validation ist performed. Example:\n>\n> \t$ git symbolic-ref HEAD 'refs/heads/master\n>      >\n>      >\n>      > '\n>\n> Fix this by checking the given reference with check_refname_format().\n>\n> Signed-off-by: Michael Schubert <mschub@elegosoft.com>\n> ---\n>   builtin/symbolic-ref.c  |  4 +++-\n>   t/t1401-symbolic-ref.sh | 10 ++++++++++\n>   2 files changed, 13 insertions(+), 1 deletion(-)\n>\n> diff --git a/builtin/symbolic-ref.c b/builtin/symbolic-ref.c\n> index 801d62e..a529541 100644\n> --- a/builtin/symbolic-ref.c\n> +++ b/builtin/symbolic-ref.c\n> @@ -44,13 +44,15 @@ int cmd_symbolic_ref(int argc, const char **argv, const char *prefix)\n>   \tgit_config(git_default_config, NULL);\n>   \targc = parse_options(argc, argv, prefix, options,\n>   \t\t\t     git_symbolic_ref_usage, 0);\n> -\tif (msg &&!*msg)\n> +\tif (msg && !*msg)\n>   \t\tdie(\"Refusing to perform update with empty message\");\n>   \tswitch (argc) {\n>   \tcase 1:\n>   \t\tcheck_symref(argv[0], quiet);\n>   \t\tbreak;\n>   \tcase 2:\n> +\t\tif (check_refname_format(argv[1], 0))\n> +\t\t\tdie(\"No valid reference format: '%s'\", argv[1]);\n\nThe error message is awkward.  I suggest something like\n\n     \"Reference name has invalid format: '%s'\"\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"195131","messageId":"7v7gu34ow0.fsf@alter.siamese.dyndns.org","threadId":"31035","inReplyTo":"1342440781-18816-2-git-send-email-mschub@elegosoft.com","subject":"Re: [PATCH 1/2] refs: disallow ref components starting with hyphen","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-07-16T17:06:07Z","receivedAt":"2012-07-16T17:06:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Schubert <mschub@elegosoft.com> writes:\n\n> Currently, we allow refname components to start with a hyphen. There's\n> no good reason to do so...\n\nThat is way too weak as a justification to potentially break\nexisting repositories.\n\nRefusal upon attempted creation is probably OK, which is why the two\nchecks you removed in your patches are fine.  I do not know if it is\njustifiable to do that in check_refname_component() that is used in\nthe reading codepath.\n\n> diff --git a/builtin/tag.c b/builtin/tag.c\n> index 7b1be85..c99fb42 100644\n> --- a/builtin/tag.c\n> +++ b/builtin/tag.c\n> @@ -403,9 +403,6 @@ static int parse_msg_arg(const struct option *opt, const char *arg, int unset)\n>  \n>  static int strbuf_check_tag_ref(struct strbuf *sb, const char *name)\n>  {\n> -\tif (name[0] == '-')\n> -\t\treturn -1;\n> -\n>  \tstrbuf_reset(sb);\n>  \tstrbuf_addf(sb, \"refs/tags/%s\", name);\n>  \n> diff --git a/refs.c b/refs.c\n> index da74a2b..5714681 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -62,6 +62,8 @@ static int check_refname_component(const char *refname, int flags)\n>  \t\tif (refname[1] == '\\0')\n>  \t\t\treturn -1; /* Component equals \".\". */\n>  \t}\n> +\tif (refname[0] == '-')\n> +\t\treturn -1; /* Component starts with '-'. */\n>  \tif (cp - refname >= 5 && !memcmp(cp - 5, \".lock\", 5))\n>  \t\treturn -1; /* Refname ends with \".lock\". */\n>  \treturn cp - refname;\n> diff --git a/sha1_name.c b/sha1_name.c\n> index 5d81ea0..132d369 100644\n> --- a/sha1_name.c\n> +++ b/sha1_name.c\n> @@ -892,8 +892,6 @@ int strbuf_branchname(struct strbuf *sb, const char *name)\n>  int strbuf_check_branch_ref(struct strbuf *sb, const char *name)\n>  {\n>  \tstrbuf_branchname(sb, name);\n> -\tif (name[0] == '-')\n> -\t\treturn -1;\n>  \tstrbuf_splice(sb, 0, 0, \"refs/heads/\", 11);\n>  \treturn check_refname_format(sb->buf, 0);\n>  }\n"},{"id":"195132","messageId":"7v394r4old.fsf@alter.siamese.dyndns.org","threadId":"31035","inReplyTo":"1342440781-18816-3-git-send-email-mschub@elegosoft.com","subject":"Re: [PATCH 2/2] symbolic-ref: check format of given refname","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-07-16T17:12:30Z","receivedAt":"2012-07-16T17:12:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Schubert <mschub@elegosoft.com> writes:\n\n> Currently, it's possible to update HEAD with a nonsense reference since\n> no strict validation ist performed. Example:\n>\n> \t$ git symbolic-ref HEAD 'refs/heads/master\n>     >\n>     >\n>     > '\n>\n> Fix this by checking the given reference with check_refname_format().\n>\n> Signed-off-by: Michael Schubert <mschub@elegosoft.com>\n> ---\n>  builtin/symbolic-ref.c  |  4 +++-\n>  t/t1401-symbolic-ref.sh | 10 ++++++++++\n>  2 files changed, 13 insertions(+), 1 deletion(-)\n>\n> diff --git a/builtin/symbolic-ref.c b/builtin/symbolic-ref.c\n> index 801d62e..a529541 100644\n> --- a/builtin/symbolic-ref.c\n> +++ b/builtin/symbolic-ref.c\n> @@ -44,13 +44,15 @@ int cmd_symbolic_ref(int argc, const char **argv, const char *prefix)\n>  \tgit_config(git_default_config, NULL);\n>  \targc = parse_options(argc, argv, prefix, options,\n>  \t\t\t     git_symbolic_ref_usage, 0);\n> -\tif (msg &&!*msg)\n> +\tif (msg && !*msg)\n>  \t\tdie(\"Refusing to perform update with empty message\");\n>  \tswitch (argc) {\n>  \tcase 1:\n>  \t\tcheck_symref(argv[0], quiet);\n>  \t\tbreak;\n>  \tcase 2:\n> +\t\tif (check_refname_format(argv[1], 0))\n> +\t\t\tdie(\"No valid reference format: '%s'\", argv[1]);\n>  \t\tif (!strcmp(argv[0], \"HEAD\") &&\n>  \t\t    prefixcmp(argv[1], \"refs/\"))\n>  \t\t\tdie(\"Refusing to point HEAD outside of refs/\");\n\nThe existing context lines above may give a clue why this patch is\nnot such a good idea.  We only limit HEAD to point under refs/ but\nallow advanced users and scripts creative uses of other kinds of\nsymrefs.  Shouldn't the patch apply the new restriction only to HEAD\nas well?\n\nBy the way, should \"git symbolic-ref _ HEAD\" work?\n\n> diff --git a/t/t1401-symbolic-ref.sh b/t/t1401-symbolic-ref.sh\n> index 2c96551..b1cd508 100755\n> --- a/t/t1401-symbolic-ref.sh\n> +++ b/t/t1401-symbolic-ref.sh\n> @@ -27,6 +27,16 @@ test_expect_success 'symbolic-ref refuses non-ref for HEAD' '\n>  '\n>  reset_to_sane\n>  \n> +test_expect_success 'symbolic-ref refuses ref with leading dot' '\n> +\ttest_must_fail git symbolic-ref HEAD refs/heads/.foo\n> +'\n> +reset_to_sane\n> +\n> +test_expect_success 'symbolic-ref refuses ref with leading dash' '\n> +\ttest_must_fail git symbolic-ref HEAD refs/heads/-foo\n> +'\n> +reset_to_sane\n> +\n>  test_expect_success 'symbolic-ref refuses bare sha1' '\n>  \techo content >file && git add file && git commit -m one &&\n>  \ttest_must_fail git symbolic-ref HEAD `git rev-parse HEAD`\n"},{"id":"195136","messageId":"7vpq7v38bd.fsf@alter.siamese.dyndns.org","threadId":"31035","inReplyTo":"7v7gu34ow0.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/2] refs: disallow ref components starting with hyphen","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-07-16T17:49:26Z","receivedAt":"2012-07-16T17:49:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Michael Schubert <mschub@elegosoft.com> writes:\n>\n>> Currently, we allow refname components to start with a hyphen. There's\n>> no good reason to do so...\n>\n> That is way too weak as a justification to potentially break\n> existing repositories.\n>\n> Refusal upon attempted creation is probably OK, which is why the two\n> checks you removed in your patches are fine.\n\nJust to clarify, I meant that the existing checks were OK because\nthey were meant to prevent creation.  I didn't mean removal of them\nwas OK.\n"}]}