{"thread":{"id":"27307","subject":"Tags named '-'","startedAt":"2011-05-09T15:21:36Z","lastAt":"2011-05-10T09:47:03Z","messageCount":7,"participants":["Alex Vandiver","Junio C Hamano","Sverre Rabbelier","Michael Schubert"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"167505","messageId":"1304954496.11377.11.camel@kohr-ah","threadId":"27307","inReplyTo":null,"subject":"Tags named '-'","fromName":"Alex Vandiver","fromEmail":"alex@chmrr.net","sentAt":"2011-05-09T15:21:36Z","receivedAt":"2011-05-09T15:21:36Z","isPatch":false,"sender":{"key":"alex@chmrr.net","avatar":"https://avatars.githubusercontent.com/u/28347?v=4"},"body":"Heya,\n  The functionality of `git checkout -` is very useful, and I am hopeful\nthat `git merge -` will eventually land, as it matches my workflow well\nas well.  However, the porcelain is not entirely consistent about\nforbidding '-' as a ref name:\n\n        ~/tmp $ git init\n        Initialized empty Git repository in /home/chmrr/tmp/.git/\n        ~/tmp (master #) $ git commit --allow-empty -m 'First commit'\n        [master (root-commit) b7402ef] First commit\n        ~/tmp (master) $ git checkout -b other\n        Switched to a new branch 'other'\n        ~/tmp (other) $ git checkout -\n        Switched to branch 'master'\n        ~/tmp (master) $ git checkout -b -\n        fatal: git checkout: we do not like '-' as a branch name.\n        ~/tmp (master) $ git tag -\n        ~/tmp (master) $ git tag -l\n        -\n        ~/tmp (master) $ git checkout -\n        Switched to branch 'other'\n\nThe likely best fix is to disallow '-' as a tag name, as well, which\nwould be handy because the typo of leaving off the 'l' on '-l', for\nexample, which is not an uncommon mistake that I have seen.\n - Alex\n"},{"id":"167514","messageId":"7v39knpxbe.fsf@alter.siamese.dyndns.org","threadId":"27307","inReplyTo":"1304954496.11377.11.camel@kohr-ah","subject":"Re: Tags named '-'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-05-09T18:01:57Z","receivedAt":"2011-05-09T18:01:57Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alex Vandiver <alex@chmrr.net> writes:\n\n> The likely best fix is to disallow '-' as a tag name, as well.\n\nSounds sane.  Patches welcome.\n"},{"id":"167529","messageId":"BANLkTinDHCq4DAiN-3CQPjcZe7LswOxuZA@mail.gmail.com","threadId":"27307","inReplyTo":"1304954496.11377.11.camel@kohr-ah","subject":"Re: Tags named '-'","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2011-05-09T22:04:16Z","receivedAt":"2011-05-09T22:04:16Z","isPatch":false,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"Heya,\n\nOn Mon, May 9, 2011 at 17:21, Alex Vandiver <alex@chmrr.net> wrote:\n>  The functionality of `git checkout -` is very useful, and I am hopeful\n> that `git merge -` will eventually land, as it matches my workflow well\n> as well.\n\nSpeaking of which, my cursory check of 'man git-checkout' did not give\nme any information on 'git checkout -'. ISTR it lets you check out the\nbranch you were previously on, but I'm not sure. Is it undocumented,\nor am I missing it (entirely possible, searching for an option named\n'-' is somewhat difficult).\n\n-- \nCheers,\n\nSverre Rabbelier\n"},{"id":"167548","messageId":"4DC87113.4030204@elegosoft.com","threadId":"27307","inReplyTo":"7v39knpxbe.fsf@alter.siamese.dyndns.org","subject":"[RFC/PATCH] tag: disallow '-' as tag name","fromName":"Michael Schubert","fromEmail":"mschub@elegosoft.com","sentAt":"2011-05-09T22:56:19Z","receivedAt":"2011-05-09T22:56:19Z","isPatch":true,"sender":{"key":"mschub@elegosoft.com","avatar":null},"body":"Add strbuf_check_tag_ref() as helper to check a refname for a tag.\n\nSigned-off-by: Michael Schubert <mschub@elegosoft.com>\n---\n builtin/tag.c |   30 ++++++++++++++++++++++--------\n 1 files changed, 22 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex b66b34a..f087a7f 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -352,11 +352,26 @@ static int parse_msg_arg(const struct option *opt, const char *arg, int unset)\n \treturn 0;\n }\n \n+static int strbuf_check_tag_ref(struct strbuf *sb, const char *name)\n+{\n+\tif (name[0] == '-')\n+\t\treturn CHECK_REF_FORMAT_ERROR;\n+\n+\tstrbuf_reset(sb);\n+\tstrbuf_add(sb, \"refs/tags/\", 10);\n+\tstrbuf_add(sb, name, strlen(name));\n+\n+\tif (sb->len > PATH_MAX)\n+\t\tdie(_(\"tag name too long: %.*s...\"), 50, name);\n+\n+\treturn check_ref_format(sb->buf);\n+}\n+\n int cmd_tag(int argc, const char **argv, const char *prefix)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n+\tstruct strbuf ref = STRBUF_INIT;\n \tunsigned char object[20], prev[20];\n-\tchar ref[PATH_MAX];\n \tconst char *object_ref, *tag;\n \tstruct ref_lock *lock;\n \n@@ -452,12 +467,10 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \tif (get_sha1(object_ref, object))\n \t\tdie(_(\"Failed to resolve '%s' as a valid ref.\"), object_ref);\n \n-\tif (snprintf(ref, sizeof(ref), \"refs/tags/%s\", tag) > sizeof(ref) - 1)\n-\t\tdie(_(\"tag name too long: %.*s...\"), 50, tag);\n-\tif (check_ref_format(ref))\n+\tif (strbuf_check_tag_ref(&ref, tag))\n \t\tdie(_(\"'%s' is not a valid tag name.\"), tag);\n \n-\tif (!resolve_ref(ref, prev, 1, NULL))\n+\tif (!resolve_ref(ref.buf, prev, 1, NULL))\n \t\thashclr(prev);\n \telse if (!force)\n \t\tdie(_(\"tag '%s' already exists\"), tag);\n@@ -466,14 +479,15 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \t\tcreate_tag(object, tag, &buf, msg.given || msgfile,\n \t\t\t   sign, prev, object);\n \n-\tlock = lock_any_ref_for_update(ref, prev, 0);\n+\tlock = lock_any_ref_for_update(ref.buf, prev, 0);\n \tif (!lock)\n-\t\tdie(_(\"%s: cannot lock the ref\"), ref);\n+\t\tdie(_(\"%s: cannot lock the ref\"), ref.buf);\n \tif (write_ref_sha1(lock, object, NULL) < 0)\n-\t\tdie(_(\"%s: cannot update the ref\"), ref);\n+\t\tdie(_(\"%s: cannot update the ref\"), ref.buf);\n \tif (force && hashcmp(prev, object))\n \t\tprintf(_(\"Updated tag '%s' (was %s)\\n\"), tag, find_unique_abbrev(prev, DEFAULT_ABBREV));\n \n \tstrbuf_release(&buf);\n+\tstrbuf_release(&ref);\n \treturn 0;\n }\n-- \n1.7.5.1 \n"},{"id":"167550","messageId":"7v62pjo4km.fsf@alter.siamese.dyndns.org","threadId":"27307","inReplyTo":"4DC87113.4030204@elegosoft.com","subject":"Re: [RFC/PATCH] tag: disallow '-' as tag name","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-05-09T23:08:09Z","receivedAt":"2011-05-09T23:08:09Z","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> Add strbuf_check_tag_ref() as helper to check a refname for a tag.\n>\n> Signed-off-by: Michael Schubert <mschub@elegosoft.com>\n> ---\n\nThat was quick ;-).\n\n>  builtin/tag.c |   30 ++++++++++++++++++++++--------\n>  1 files changed, 22 insertions(+), 8 deletions(-)\n>\n> diff --git a/builtin/tag.c b/builtin/tag.c\n> index b66b34a..f087a7f 100644\n> --- a/builtin/tag.c\n> +++ b/builtin/tag.c\n> @@ -352,11 +352,26 @@ static int parse_msg_arg(const struct option *opt, const char *arg, int unset)\n>  \treturn 0;\n>  }\n>  \n> +static int strbuf_check_tag_ref(struct strbuf *sb, const char *name)\n> +{\n> +\tif (name[0] == '-')\n> +\t\treturn CHECK_REF_FORMAT_ERROR;\n\nSo contrary to what the title claims, it forbids a tag that begins with '-',\ne.g. '-foo', not just a single dash.  That is fine by me (we do the same\nin strbuf-check-branch-ref) but it needs to be explained better.\n\n> +\tstrbuf_reset(sb);\n> +\tstrbuf_add(sb, \"refs/tags/\", 10);\n> +\tstrbuf_add(sb, name, strlen(name));\n\nstrbuf_addf(sb, \"refs/tags/%s\", name)?\n\n> +\tif (sb->len > PATH_MAX)\n> +\t\tdie(_(\"tag name too long: %.*s...\"), 50, name);\n\nI think that should be\n\n\tif (PATH_MAX <= sb->len)\n\nbut I do not see the point of checking against PATH_MAX if you are already\nusing a strbuf...\n"},{"id":"167558","messageId":"4DC87A84.4070604@elegosoft.com","threadId":"27307","inReplyTo":"7v62pjo4km.fsf@alter.siamese.dyndns.org","subject":"[RFC/PATCH v2] tag: disallow '-' as tag name","fromName":"Michael Schubert","fromEmail":"mschub@elegosoft.com","sentAt":"2011-05-09T23:36:36Z","receivedAt":"2011-05-09T23:36:36Z","isPatch":true,"sender":{"key":"mschub@elegosoft.com","avatar":null},"body":"\nDisallow '-' as tag name just as any tag name starting with '-' to be\nconsistent with \"git checkout\".\n\nAdd strbuf_check_tag_ref() as helper to check a refname for a tag.\n\nSigned-off-by: Michael Schubert <mschub@elegosoft.com>\n---\n builtin/tag.c |   26 ++++++++++++++++++--------\n 1 files changed, 18 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex b66b34a..ec926fc 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -352,11 +352,22 @@ static int parse_msg_arg(const struct option *opt, const char *arg, int unset)\n \treturn 0;\n }\n \n+static int strbuf_check_tag_ref(struct strbuf *sb, const char *name)\n+{\n+\tif (name[0] == '-')\n+\t\treturn CHECK_REF_FORMAT_ERROR;\n+\n+\tstrbuf_reset(sb);\n+\tstrbuf_addf(sb, \"refs/tags/%s\", name);\n+\n+\treturn check_ref_format(sb->buf);\n+}\n+\n int cmd_tag(int argc, const char **argv, const char *prefix)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n+\tstruct strbuf ref = STRBUF_INIT;\n \tunsigned char object[20], prev[20];\n-\tchar ref[PATH_MAX];\n \tconst char *object_ref, *tag;\n \tstruct ref_lock *lock;\n \n@@ -452,12 +463,10 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \tif (get_sha1(object_ref, object))\n \t\tdie(_(\"Failed to resolve '%s' as a valid ref.\"), object_ref);\n \n-\tif (snprintf(ref, sizeof(ref), \"refs/tags/%s\", tag) > sizeof(ref) - 1)\n-\t\tdie(_(\"tag name too long: %.*s...\"), 50, tag);\n-\tif (check_ref_format(ref))\n+\tif (strbuf_check_tag_ref(&ref, tag))\n \t\tdie(_(\"'%s' is not a valid tag name.\"), tag);\n \n-\tif (!resolve_ref(ref, prev, 1, NULL))\n+\tif (!resolve_ref(ref.buf, prev, 1, NULL))\n \t\thashclr(prev);\n \telse if (!force)\n \t\tdie(_(\"tag '%s' already exists\"), tag);\n@@ -466,14 +475,15 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \t\tcreate_tag(object, tag, &buf, msg.given || msgfile,\n \t\t\t   sign, prev, object);\n \n-\tlock = lock_any_ref_for_update(ref, prev, 0);\n+\tlock = lock_any_ref_for_update(ref.buf, prev, 0);\n \tif (!lock)\n-\t\tdie(_(\"%s: cannot lock the ref\"), ref);\n+\t\tdie(_(\"%s: cannot lock the ref\"), ref.buf);\n \tif (write_ref_sha1(lock, object, NULL) < 0)\n-\t\tdie(_(\"%s: cannot update the ref\"), ref);\n+\t\tdie(_(\"%s: cannot update the ref\"), ref.buf);\n \tif (force && hashcmp(prev, object))\n \t\tprintf(_(\"Updated tag '%s' (was %s)\\n\"), tag, find_unique_abbrev(prev, DEFAULT_ABBREV));\n \n \tstrbuf_release(&buf);\n+\tstrbuf_release(&ref);\n \treturn 0;\n }\n-- \n1.7.5.1\n"},{"id":"167588","messageId":"4DC90997.4060208@elegosoft.com","threadId":"27307","inReplyTo":"BANLkTik7PYjGMMfxaNPubYR7M1OgBrF_qw@mail.gmail.com","subject":"Re: [RFC/PATCH v2] tag: disallow '-' as tag name","fromName":"Michael Schubert","fromEmail":"mschub@elegosoft.com","sentAt":"2011-05-10T09:47:03Z","receivedAt":"2011-05-10T09:47:03Z","isPatch":true,"sender":{"key":"mschub@elegosoft.com","avatar":null},"body":"On 05/10/2011 09:07 AM, Sverre Rabbelier wrote:\n>> Disallow '-' as tag name just as any tag name starting with '-' to be\n>> consistent with \"git checkout\".\n> \n> This was hard for me to parse, how about::\n> \n> Disallow '-' as tag name, as well as tag names starting with '-', to\n> be consistent with \"git checkout\".\n\nThanks.\n\n-- >8 --\nSubject: [PATCH] tag: disallow '-' as tag name\n\nDisallow '-' as tag name, as well as tag names starting with '-', to be\nconsistent with \"git checkout\".\n\nAdd strbuf_check_tag_ref() as helper to check a refname for a tag.\n\nSigned-off-by: Michael Schubert <mschub@elegosoft.com>\n---\n builtin/tag.c |   26 ++++++++++++++++++--------\n 1 files changed, 18 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex b66b34a..ec926fc 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -352,11 +352,22 @@ static int parse_msg_arg(const struct option *opt, const char *arg, int unset)\n \treturn 0;\n }\n \n+static int strbuf_check_tag_ref(struct strbuf *sb, const char *name)\n+{\n+\tif (name[0] == '-')\n+\t\treturn CHECK_REF_FORMAT_ERROR;\n+\n+\tstrbuf_reset(sb);\n+\tstrbuf_addf(sb, \"refs/tags/%s\", name);\n+\n+\treturn check_ref_format(sb->buf);\n+}\n+\n int cmd_tag(int argc, const char **argv, const char *prefix)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n+\tstruct strbuf ref = STRBUF_INIT;\n \tunsigned char object[20], prev[20];\n-\tchar ref[PATH_MAX];\n \tconst char *object_ref, *tag;\n \tstruct ref_lock *lock;\n \n@@ -452,12 +463,10 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \tif (get_sha1(object_ref, object))\n \t\tdie(_(\"Failed to resolve '%s' as a valid ref.\"), object_ref);\n \n-\tif (snprintf(ref, sizeof(ref), \"refs/tags/%s\", tag) > sizeof(ref) - 1)\n-\t\tdie(_(\"tag name too long: %.*s...\"), 50, tag);\n-\tif (check_ref_format(ref))\n+\tif (strbuf_check_tag_ref(&ref, tag))\n \t\tdie(_(\"'%s' is not a valid tag name.\"), tag);\n \n-\tif (!resolve_ref(ref, prev, 1, NULL))\n+\tif (!resolve_ref(ref.buf, prev, 1, NULL))\n \t\thashclr(prev);\n \telse if (!force)\n \t\tdie(_(\"tag '%s' already exists\"), tag);\n@@ -466,14 +475,15 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \t\tcreate_tag(object, tag, &buf, msg.given || msgfile,\n \t\t\t   sign, prev, object);\n \n-\tlock = lock_any_ref_for_update(ref, prev, 0);\n+\tlock = lock_any_ref_for_update(ref.buf, prev, 0);\n \tif (!lock)\n-\t\tdie(_(\"%s: cannot lock the ref\"), ref);\n+\t\tdie(_(\"%s: cannot lock the ref\"), ref.buf);\n \tif (write_ref_sha1(lock, object, NULL) < 0)\n-\t\tdie(_(\"%s: cannot update the ref\"), ref);\n+\t\tdie(_(\"%s: cannot update the ref\"), ref.buf);\n \tif (force && hashcmp(prev, object))\n \t\tprintf(_(\"Updated tag '%s' (was %s)\\n\"), tag, find_unique_abbrev(prev, DEFAULT_ABBREV));\n \n \tstrbuf_release(&buf);\n+\tstrbuf_release(&ref);\n \treturn 0;\n }\n-- \n1.7.5.1\n"}]}