{"thread":{"id":"28403","subject":"[PATCH v3 06/22] Do not allow \".lock\" at the end of any refname component","startedAt":"2011-09-15T21:10:21Z","lastAt":"2011-10-19T20:39:21Z","messageCount":60,"participants":["Michael Haggerty","Thomas Rast","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":3,"patchTotal":22},"messages":[{"id":"175610","messageId":"1316121043-29367-1-git-send-email-mhagger@alum.mit.edu","threadId":"28403","inReplyTo":null,"subject":"[PATCH v3 00/22] Clean up refname checks and normalization","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2011-09-15T21:10:21Z","receivedAt":"2011-09-15T21:10:21Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"This patch series cleans up refname checks and normalization in a\ndifferent way than v1 and v2. (Unnormalized refnames are those that\nhave extra leading slashes or runs of repeated slashes between\ncomponents, like \"//refs/heads///master\".)\n\nAs discussed on the mailing list [1], adding consistent support for\nunnormalized refnames everywhere was becoming a bottomless pit, so\ninstead I am taking the opposite approach--only accept normalized\nrefnames. Support for unnormalized refnames was broken anyway, so it\nshouldn't be missed.\n\nThere is only one command that is now meant to accept unnormalized\nrefnames:\n\n $ git check-ref-format --normalize //refs/heads///master\n refs/heads/master\n $\n\nThis command can be used to normalize refnames for use elsewhere.\n\nI have added a few internal checks that refnames have the correct\nformat; more tests should still be added. But this patch series is\ngetting quite long already, so I would like to submit it.\n\nDuring the development of this patch series, I discovered that\nfetch_with_import() in remote-helper.c sometimes passes NULL to\nread_ref(), and thereby to resolve_ref(). The two commits labeled\n\"remote: *\" fix this in a naive way. But somebody more familiar with\nthis code should check whether the fix is OK and more specifically\nwhether this is a symptom of a bigger problem in the remote-helper\ncode.\n\nI should mention that this cleanup is preparation for my main goal:\nstoring ref caches hierarchically. But there is still a lot of\ntangled up and seemingly redundant code in refs.c, so it might be a\nwhile before I get to the main project.\n\n[1] http://permalink.gmane.org/gmane.comp.version-control.git/181268\n\nMichael Haggerty (22):\n t1402: add some more tests\n git check-ref-format: add options --allow-onelevel and\n --refspec-pattern\n Change bad_ref_char() to return a boolean value\n Change check_ref_format() to take a flags argument\n Refactor check_refname_format()\n Do not allow \".lock\" at the end of any refname component\n Make collapse_slashes() allocate memory for its result\n Inline function refname_format_print()\n Change check_refname_format() to reject unnormalized refnames\n resolve_ref(): explicitly fail if a symlink is not readable\n resolve_ref(): use prefixcmp()\n resolve_ref(): only follow a symlink that contains a valid,\n normalized refname\n resolve_ref(): turn buffer into a proper string as soon as possible\n resolve_ref(): extract a function get_packed_ref()\n resolve_ref(): do not follow incorrectly-formatted symbolic refs\n remote: use xstrdup() instead of strdup()\n remote: avoid passing NULL to read_ref()\n resolve_ref(): verify that the input refname has the right format\n resolve_ref(): emit warnings for improperly-formatted references\n resolve_ref(): also treat a too-long SHA1 as invalid\n resolve_ref(): expand documentation\n add_ref(): verify that the refname is formatted correctly\n\n Documentation/git-check-ref-format.txt | 53 ++++++--\n builtin/check-ref-format.c | 61 ++++++---\n builtin/checkout.c | 2 +-\n builtin/fetch-pack.c | 2 +-\n builtin/receive-pack.c | 2 +-\n builtin/replace.c | 2 +-\n builtin/show-ref.c | 2 +-\n builtin/tag.c | 4 +-\n cache.h | 34 +++++-\n connect.c | 2 +-\n environment.c | 2 +-\n fast-import.c | 7 +-\n git_remote_helpers/git/git.py | 2 +-\n notes-merge.c | 5 +-\n pack-refs.c | 2 +-\n refs.c | 222 +++++++++++++++++++-------------\n refs.h | 21 +++-\n remote.c | 55 ++------\n sha1_name.c | 4 +-\n t/t1402-check-ref-format.sh | 120 +++++++++++++++--\n transport-helper.c | 10 +-\n transport.c | 16 +--\n walker.c | 2 +-\n 23 files changed, 408 insertions(+), 224 deletions(-)\n\n-- \n1.7.6.8.gd2879\n"},{"id":"175615","messageId":"1316121043-29367-2-git-send-email-mhagger@alum.mit.edu","threadId":"28403","inReplyTo":"1316121043-29367-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v3 01/22] t1402: add some more tests","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2011-09-15T21:10:22Z","receivedAt":"2011-09-15T21:10:22Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"The new tests reflect the status quo. Soon the rule for \"*.lock\" in\nrefname components will be tightened up.\n\nSigned-off-by: Michael Haggerty \n---\n t/t1402-check-ref-format.sh | 9 +++++++++\n 1 files changed, 9 insertions(+), 0 deletions(-)\n\ndiff --git a/t/t1402-check-ref-format.sh b/t/t1402-check-ref-format.sh\nindex ed4275a..dc43171 100755\n--- a/t/t1402-check-ref-format.sh\n+++ b/t/t1402-check-ref-format.sh\n@@ -13,6 +13,8 @@ invalid_ref() {\n \t\t\"test_must_fail git check-ref-format '$1'\"\n }\n \n+invalid_ref ''\n+invalid_ref '/'\n valid_ref 'heads/foo'\n invalid_ref 'foo'\n valid_ref 'foo/bar/baz'\n@@ -27,6 +29,9 @@ invalid_ref 'heads/foo..bar'\n invalid_ref 'heads/foo?bar'\n valid_ref 'foo./bar'\n invalid_ref 'heads/foo.lock'\n+invalid_ref 'heads///foo.lock'\n+valid_ref 'foo.lock/bar'\n+valid_ref 'foo.lock///bar'\n valid_ref 'heads/foo@bar'\n invalid_ref 'heads/v@{ation'\n invalid_ref 'heads/foo\\bar'\n@@ -83,5 +88,9 @@ invalid_ref_normalized '/foo'\n invalid_ref_normalized 'heads/foo/../bar'\n invalid_ref_normalized 'heads/./foo'\n invalid_ref_normalized 'heads\\foo'\n+invalid_ref_normalized 'heads/foo.lock'\n+invalid_ref_normalized 'heads///foo.lock'\n+valid_ref_normalized 'foo.lock/bar' 'foo.lock/bar'\n+valid_ref_normalized 'foo.lock///bar' 'foo.lock/bar'\n \n test_done\n-- \n1.7.6.8.gd2879\n"},{"id":"175609","messageId":"1316121043-29367-3-git-send-email-mhagger@alum.mit.edu","threadId":"28403","inReplyTo":"1316121043-29367-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v3 02/22] git check-ref-format: add options --allow-onelevel and --refspec-pattern","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2011-09-15T21:10:23Z","receivedAt":"2011-09-15T21:10:23Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Also add tests of the new options. (Actually, one big reason to add\nthe new options is to make it easy to test check_ref_format(), though\nthe options should also be useful to other scripts.)\n\nInterpret the result of check_ref_format() based on which types of\nrefnames are allowed. However, because check_ref_format() can only\nreturn a single value, one test case is still broken. Specifically,\nthe case \"git check-ref-format --onelevel '*'\" incorrectly succeeds\nbecause check_ref_format() returns CHECK_REF_FORMAT_ONELEVEL for this\nrefname even though the refname is also CHECK_REF_FORMAT_WILDCARD.\nThe type of check that leads to this failure is used elsewhere in\n\"real\" code and could lead to bugs; it will be fixed over the next few\ncommits.\n\nSigned-off-by: Michael Haggerty \n---\n Documentation/git-check-ref-format.txt | 29 +++++++++--\n builtin/check-ref-format.c | 56 +++++++++++++++++---\n t/t1402-check-ref-format.sh | 88 +++++++++++++++++++++++++++++---\n 3 files changed, 152 insertions(+), 21 deletions(-)\n\ndiff --git a/Documentation/git-check-ref-format.txt b/Documentation/git-check-ref-format.txt\nindex c9fdf84..dcb8cc3 100644\n--- a/Documentation/git-check-ref-format.txt\n+++ b/Documentation/git-check-ref-format.txt\n@@ -8,8 +8,8 @@ git-check-ref-format - Ensures that a reference name is well formed\n SYNOPSIS\n --------\n [verse]\n-'git check-ref-format' \n-'git check-ref-format' --print \n+'git check-ref-format' [--print]\n+ [--[no-]allow-onelevel] [--refspec-pattern] \n 'git check-ref-format' --branch \n \n DESCRIPTION\n@@ -32,14 +32,18 @@ git imposes the following rules on how references are named:\n \n . They must contain at least one `/`. This enforces the presence of a\n category like `heads/`, `tags/` etc. but the actual names are not\n- restricted.\n+ restricted. If the `--allow-onelevel` option is used, this rule\n+ is waived.\n \n . They cannot have two consecutive dots `..` anywhere.\n \n . They cannot have ASCII control characters (i.e. bytes whose\n values are lower than \\040, or \\177 `DEL`), space, tilde `~`,\n- caret `{caret}`, colon `:`, question-mark `?`, asterisk `*`,\n- or open bracket `[` anywhere.\n+ caret `{caret}`, or colon `:` anywhere.\n+\n+. They cannot have question-mark `?`, asterisk `{asterisk}`, or open\n+ bracket `[` anywhere. See the `--refspec-pattern` option below for\n+ an exception to this rule.\n \n . They cannot end with a slash `/` nor a dot `.`.\n \n@@ -78,6 +82,21 @@ were on. This option should be used by porcelains to accept this\n syntax anywhere a branch name is expected, so they can act as if you\n typed the branch name.\n \n+OPTIONS\n+-------\n+--allow-onelevel::\n+--no-allow-onelevel::\n+\tControls whether one-level refnames are accepted (i.e.,\n+\trefnames that do not contain multiple `/`-separated\n+\tcomponents). The default is `--no-allow-onelevel`.\n+\n+--refspec-pattern::\n+\tInterpret as a reference name pattern for a refspec\n+\t(as used with remote repositories). If this option is\n+\tenabled, is allowed to contain a single `{asterisk}`\n+\tin place of a one full pathname component (e.g.,\n+\t`foo/{asterisk}/bar` but not `foo/bar{asterisk}`).\n+\n EXAMPLES\n --------\n \ndiff --git a/builtin/check-ref-format.c b/builtin/check-ref-format.c\nindex 0723cf2..7295954 100644\n--- a/builtin/check-ref-format.c\n+++ b/builtin/check-ref-format.c\n@@ -8,7 +8,7 @@\n #include \"strbuf.h\"\n \n static const char builtin_check_ref_format_usage[] =\n-\"git check-ref-format [--print] \\n\"\n+\"git check-ref-format [--print] [options] \\n\"\n \" or: git check-ref-format --branch \";\n \n /*\n@@ -45,27 +45,65 @@ static int check_ref_format_branch(const char *arg)\n \treturn 0;\n }\n \n-static int check_ref_format_print(const char *arg)\n+static void refname_format_print(const char *arg)\n {\n \tchar *refname = xmalloc(strlen(arg) + 1);\n \n-\tif (check_ref_format(arg))\n-\t\treturn 1;\n \tcollapse_slashes(refname, arg);\n \tprintf(\"%s\\n\", refname);\n-\treturn 0;\n }\n \n+#define REFNAME_ALLOW_ONELEVEL 1\n+#define REFNAME_REFSPEC_PATTERN 2\n+\n int cmd_check_ref_format(int argc, const char **argv, const char *prefix)\n {\n+\tint i;\n+\tint print = 0;\n+\tint flags = 0;\n+\n \tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n \t\tusage(builtin_check_ref_format_usage);\n \n \tif (argc == 3 && !strcmp(argv[1], \"--branch\"))\n \t\treturn check_ref_format_branch(argv[2]);\n-\tif (argc == 3 && !strcmp(argv[1], \"--print\"))\n-\t\treturn check_ref_format_print(argv[2]);\n-\tif (argc != 2)\n+\n+\tfor (i = 1; i < argc && argv[i][0] == '-'; i++) {\n+\t\tif (!strcmp(argv[i], \"--print\"))\n+\t\t\tprint = 1;\n+\t\telse if (!strcmp(argv[i], \"--allow-onelevel\"))\n+\t\t\tflags |= REFNAME_ALLOW_ONELEVEL;\n+\t\telse if (!strcmp(argv[i], \"--no-allow-onelevel\"))\n+\t\t\tflags &= ~REFNAME_ALLOW_ONELEVEL;\n+\t\telse if (!strcmp(argv[i], \"--refspec-pattern\"))\n+\t\t\tflags |= REFNAME_REFSPEC_PATTERN;\n+\t\telse\n+\t\t\tusage(builtin_check_ref_format_usage);\n+\t}\n+\tif (! (i == argc - 1))\n \t\tusage(builtin_check_ref_format_usage);\n-\treturn !!check_ref_format(argv[1]);\n+\n+\tswitch (check_ref_format(argv[i])) {\n+\tcase CHECK_REF_FORMAT_OK:\n+\t\tbreak;\n+\tcase CHECK_REF_FORMAT_ERROR:\n+\t\treturn 1;\n+\tcase CHECK_REF_FORMAT_ONELEVEL:\n+\t\tif (!(flags & REFNAME_ALLOW_ONELEVEL))\n+\t\t\treturn 1;\n+\t\telse\n+\t\t\tbreak;\n+\tcase CHECK_REF_FORMAT_WILDCARD:\n+\t\tif (!(flags & REFNAME_REFSPEC_PATTERN))\n+\t\t\treturn 1;\n+\t\telse\n+\t\t\tbreak;\n+\tdefault:\n+\t\tdie(\"internal error: unexpected value from check_ref_format()\");\n+\t}\n+\n+\tif (print)\n+\t\trefname_format_print(argv[i]);\n+\n+\treturn 0;\n }\ndiff --git a/t/t1402-check-ref-format.sh b/t/t1402-check-ref-format.sh\nindex dc43171..f551eef 100755\n--- a/t/t1402-check-ref-format.sh\n+++ b/t/t1402-check-ref-format.sh\n@@ -5,25 +5,38 @@ test_description='Test git check-ref-format'\n . ./test-lib.sh\n \n valid_ref() {\n-\ttest_expect_success \"ref name '$1' is valid\" \\\n-\t\t\"git check-ref-format '$1'\"\n+\tif test \"$#\" = 1\n+\tthen\n+\t\ttest_expect_success \"ref name '$1' is valid\" \\\n+\t\t\t\"git check-ref-format '$1'\"\n+\telse\n+\t\ttest_expect_success \"ref name '$1' is valid with options $2\" \\\n+\t\t\t\"git check-ref-format $2 '$1'\"\n+\tfi\n }\n invalid_ref() {\n-\ttest_expect_success \"ref name '$1' is not valid\" \\\n-\t\t\"test_must_fail git check-ref-format '$1'\"\n+\tif test \"$#\" = 1\n+\tthen\n+\t\ttest_expect_success \"ref name '$1' is invalid\" \\\n+\t\t\t\"test_must_fail git check-ref-format '$1'\"\n+\telse\n+\t\ttest_expect_success \"ref name '$1' is invalid with options $2\" \\\n+\t\t\t\"test_must_fail git check-ref-format $2 '$1'\"\n+\tfi\n }\n \n invalid_ref ''\n invalid_ref '/'\n-valid_ref 'heads/foo'\n-invalid_ref 'foo'\n+invalid_ref '/' --allow-onelevel\n valid_ref 'foo/bar/baz'\n valid_ref 'refs///heads/foo'\n invalid_ref 'heads/foo/'\n valid_ref '/heads/foo'\n valid_ref '///heads/foo'\n-invalid_ref '/foo'\n invalid_ref './foo'\n+invalid_ref './foo/bar'\n+invalid_ref 'foo/./bar'\n+invalid_ref 'foo/bar/.'\n invalid_ref '.refs/foo'\n invalid_ref 'heads/foo..bar'\n invalid_ref 'heads/foo?bar'\n@@ -38,6 +51,67 @@ invalid_ref 'heads/foo\\bar'\n invalid_ref \"$(printf 'heads/foo\\t')\"\n invalid_ref \"$(printf 'heads/foo\\177')\"\n valid_ref \"$(printf 'heads/fu\\303\\237')\"\n+invalid_ref 'heads/*foo/bar' --refspec-pattern\n+invalid_ref 'heads/foo*/bar' --refspec-pattern\n+invalid_ref 'heads/f*o/bar' --refspec-pattern\n+\n+ref='foo'\n+invalid_ref \"$ref\"\n+valid_ref \"$ref\" --allow-onelevel\n+invalid_ref \"$ref\" --refspec-pattern\n+valid_ref \"$ref\" '--refspec-pattern --allow-onelevel'\n+\n+ref='foo/bar'\n+valid_ref \"$ref\"\n+valid_ref \"$ref\" --allow-onelevel\n+valid_ref \"$ref\" --refspec-pattern\n+valid_ref \"$ref\" '--refspec-pattern --allow-onelevel'\n+\n+ref='foo/*'\n+invalid_ref \"$ref\"\n+invalid_ref \"$ref\" --allow-onelevel\n+valid_ref \"$ref\" --refspec-pattern\n+valid_ref \"$ref\" '--refspec-pattern --allow-onelevel'\n+\n+ref='*/foo'\n+invalid_ref \"$ref\"\n+invalid_ref \"$ref\" --allow-onelevel\n+valid_ref \"$ref\" --refspec-pattern\n+valid_ref \"$ref\" '--refspec-pattern --allow-onelevel'\n+\n+ref='foo/*/bar'\n+invalid_ref \"$ref\"\n+invalid_ref \"$ref\" --allow-onelevel\n+valid_ref \"$ref\" --refspec-pattern\n+valid_ref \"$ref\" '--refspec-pattern --allow-onelevel'\n+\n+ref='*'\n+invalid_ref \"$ref\"\n+\n+#invalid_ref \"$ref\" --allow-onelevel\n+test_expect_failure \"ref name '$ref' is invalid with options --allow-onelevel\" \\\n+\t\"test_must_fail git check-ref-format --allow-onelevel '$ref'\"\n+\n+invalid_ref \"$ref\" --refspec-pattern\n+valid_ref \"$ref\" '--refspec-pattern --allow-onelevel'\n+\n+ref='foo/*/*'\n+invalid_ref \"$ref\" --refspec-pattern\n+invalid_ref \"$ref\" '--refspec-pattern --allow-onelevel'\n+\n+ref='*/foo/*'\n+invalid_ref \"$ref\" --refspec-pattern\n+invalid_ref \"$ref\" '--refspec-pattern --allow-onelevel'\n+\n+ref='*/*/foo'\n+invalid_ref \"$ref\" --refspec-pattern\n+invalid_ref \"$ref\" '--refspec-pattern --allow-onelevel'\n+\n+ref='/foo'\n+invalid_ref \"$ref\"\n+valid_ref \"$ref\" --allow-onelevel\n+invalid_ref \"$ref\" --refspec-pattern\n+valid_ref \"$ref\" '--refspec-pattern --allow-onelevel'\n \n test_expect_success \"check-ref-format --branch @{-1}\" '\n \tT=$(git write-tree) &&\n-- \n1.7.6.8.gd2879\n"},{"id":"175617","messageId":"1316121043-29367-4-git-send-email-mhagger@alum.mit.edu","threadId":"28403","inReplyTo":"1316121043-29367-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v3 03/22] Change bad_ref_char() to return a boolean value","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2011-09-15T21:10:24Z","receivedAt":"2011-09-15T21:10:24Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Previously most bad characters were indicated by returning 1, but \"*\"\nwas special-cased to return 2 instead of 1. One caller examined the\nreturn value to see whether the special case occurred.\n\nBut it is easier (to document and understand) for bad_ref_char()\nsimply to return a boolean value, treating \"*\" like any other bad\ncharacter. Special-case the handling of \"*\" (which only occurs in\nvery specific circumstances) at the caller. The resulting calling\ncode thereby also becomes more transparent.\n\nSigned-off-by: Michael Haggerty \n---\n refs.c | 15 ++++++---------\n 1 files changed, 6 insertions(+), 9 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex a615043..fd29d89 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -860,22 +860,21 @@ int for_each_rawref(each_ref_fn fn, void *cb_data)\n * - it contains a \"\\\" (backslash)\n */\n \n+/* Return true iff ch is not allowed in reference names. */\n static inline int bad_ref_char(int ch)\n {\n \tif (((unsigned) ch) <= ' ' || ch == 0x7f ||\n \t ch == '~' || ch == '^' || ch == ':' || ch == '\\\\')\n \t\treturn 1;\n \t/* 2.13 Pattern Matching Notation */\n-\tif (ch == '?' || ch == '[') /* Unsupported */\n+\tif (ch == '*' || ch == '?' || ch == '[') /* Unsupported */\n \t\treturn 1;\n-\tif (ch == '*') /* Supported at the end */\n-\t\treturn 2;\n \treturn 0;\n }\n \n int check_ref_format(const char *ref)\n {\n-\tint ch, level, bad_type, last;\n+\tint ch, level, last;\n \tint ret = CHECK_REF_FORMAT_OK;\n \tconst char *cp = ref;\n \n@@ -890,9 +889,8 @@ int check_ref_format(const char *ref)\n \t\t/* we are at the beginning of the path component */\n \t\tif (ch == '.')\n \t\t\treturn CHECK_REF_FORMAT_ERROR;\n-\t\tbad_type = bad_ref_char(ch);\n-\t\tif (bad_type) {\n-\t\t\tif (bad_type == 2 && (!*cp || *cp == '/') &&\n+\t\tif (bad_ref_char(ch)) {\n+\t\t\tif (ch == '*' && (!*cp || *cp == '/') &&\n \t\t\t ret == CHECK_REF_FORMAT_OK)\n \t\t\t\tret = CHECK_REF_FORMAT_WILDCARD;\n \t\t\telse\n@@ -902,8 +900,7 @@ int check_ref_format(const char *ref)\n \t\tlast = ch;\n \t\t/* scan the rest of the path component */\n \t\twhile ((ch = *cp++) != 0) {\n-\t\t\tbad_type = bad_ref_char(ch);\n-\t\t\tif (bad_type)\n+\t\t\tif (bad_ref_char(ch))\n \t\t\t\treturn CHECK_REF_FORMAT_ERROR;\n \t\t\tif (ch == '/')\n \t\t\t\tbreak;\n-- \n1.7.6.8.gd2879\n"},{"id":"175616","messageId":"1316121043-29367-5-git-send-email-mhagger@alum.mit.edu","threadId":"28403","inReplyTo":"1316121043-29367-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v3 04/22] Change check_ref_format() to take a flags argument","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2011-09-15T21:10:25Z","receivedAt":"2011-09-15T21:10:25Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Change check_ref_format() to take a flags argument that indicates what\nis acceptable in the reference name (analogous to \"git\ncheck-ref-format\"'s \"--allow-onelevel\" and \"--refspec-pattern\"). This\nis more convenient for callers and also fixes a failure in the test\nsuite (and likely elsewhere in the code) by enabling \"onelevel\" and\n\"refspec-pattern\" to be allowed independently of each other.\n\nAlso rename check_ref_format() to check_refname_format() to make it\nobvious that it deals with refnames rather than references themselves.\n\nSigned-off-by: Michael Haggerty \n---\n builtin/check-ref-format.c | 21 +---------------\n builtin/checkout.c | 2 +-\n builtin/fetch-pack.c | 2 +-\n builtin/receive-pack.c | 2 +-\n builtin/replace.c | 2 +-\n builtin/show-ref.c | 2 +-\n builtin/tag.c | 4 +-\n connect.c | 2 +-\n environment.c | 2 +-\n fast-import.c | 7 +----\n git_remote_helpers/git/git.py | 2 +-\n notes-merge.c | 5 ++-\n pack-refs.c | 2 +-\n refs.c | 42 ++++++++++++++-----------------\n refs.h | 17 +++++++++----\n remote.c | 53 +++++++++++------------------------------\n sha1_name.c | 4 +-\n t/t1402-check-ref-format.sh | 6 +----\n transport.c | 16 +++---------\n walker.c | 2 +-\n 20 files changed, 69 insertions(+), 126 deletions(-)\n\ndiff --git a/builtin/check-ref-format.c b/builtin/check-ref-format.c\nindex 7295954..8f41696 100644\n--- a/builtin/check-ref-format.c\n+++ b/builtin/check-ref-format.c\n@@ -53,9 +53,6 @@ static void refname_format_print(const char *arg)\n \tprintf(\"%s\\n\", refname);\n }\n \n-#define REFNAME_ALLOW_ONELEVEL 1\n-#define REFNAME_REFSPEC_PATTERN 2\n-\n int cmd_check_ref_format(int argc, const char **argv, const char *prefix)\n {\n \tint i;\n@@ -83,24 +80,8 @@ int cmd_check_ref_format(int argc, const char **argv, const char *prefix)\n \tif (! (i == argc - 1))\n \t\tusage(builtin_check_ref_format_usage);\n \n-\tswitch (check_ref_format(argv[i])) {\n-\tcase CHECK_REF_FORMAT_OK:\n-\t\tbreak;\n-\tcase CHECK_REF_FORMAT_ERROR:\n+\tif (check_refname_format(argv[i], flags))\n \t\treturn 1;\n-\tcase CHECK_REF_FORMAT_ONELEVEL:\n-\t\tif (!(flags & REFNAME_ALLOW_ONELEVEL))\n-\t\t\treturn 1;\n-\t\telse\n-\t\t\tbreak;\n-\tcase CHECK_REF_FORMAT_WILDCARD:\n-\t\tif (!(flags & REFNAME_REFSPEC_PATTERN))\n-\t\t\treturn 1;\n-\t\telse\n-\t\t\tbreak;\n-\tdefault:\n-\t\tdie(\"internal error: unexpected value from check_ref_format()\");\n-\t}\n \n \tif (print)\n \t\trefname_format_print(argv[i]);\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 3bb6525..574d2b6 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -882,7 +882,7 @@ static int parse_branchname_arg(int argc, const char **argv,\n \tnew->name = arg;\n \tsetup_branch_path(new);\n \n-\tif (check_ref_format(new->path) == CHECK_REF_FORMAT_OK &&\n+\tif (!check_refname_format(new->path, 0) &&\n \t resolve_ref(new->path, branch_rev, 1, NULL))\n \t\thashcpy(rev, branch_rev);\n \telse\ndiff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c\nindex 412bd32..b51e478 100644\n--- a/builtin/fetch-pack.c\n+++ b/builtin/fetch-pack.c\n@@ -544,7 +544,7 @@ static void filter_refs(struct ref **refs, int nr_match, char **match)\n \tfor (ref = *refs; ref; ref = next) {\n \t\tnext = ref->next;\n \t\tif (!memcmp(ref->name, \"refs/\", 5) &&\n-\t\t check_ref_format(ref->name + 5))\n+\t\t check_refname_format(ref->name + 5, 0))\n \t\t\t; /* trash */\n \t\telse if (args.fetch_all &&\n \t\t\t (!args.depth || prefixcmp(ref->name, \"refs/tags/\") )) {\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex ae164da..0600efa 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -356,7 +356,7 @@ static const char *update(struct command *cmd)\n \tstruct ref_lock *lock;\n \n \t/* only refs/... are allowed */\n-\tif (prefixcmp(name, \"refs/\") || check_ref_format(name + 5)) {\n+\tif (prefixcmp(name, \"refs/\") || check_refname_format(name + 5, 0)) {\n \t\trp_error(\"refusing to create funny ref '%s' remotely\", name);\n \t\treturn \"funny refname\";\n \t}\ndiff --git a/builtin/replace.c b/builtin/replace.c\nindex fe3a647..517fa10 100644\n--- a/builtin/replace.c\n+++ b/builtin/replace.c\n@@ -94,7 +94,7 @@ static int replace_object(const char *object_ref, const char *replace_ref,\n \t\t \"refs/replace/%s\",\n \t\t sha1_to_hex(object)) > sizeof(ref) - 1)\n \t\tdie(\"replace ref name too long: %.*s...\", 50, ref);\n-\tif (check_ref_format(ref))\n+\tif (check_refname_format(ref, 0))\n \t\tdie(\"'%s' is not a valid ref name.\", ref);\n \n \tif (!resolve_ref(ref, prev, 1, NULL))\ndiff --git a/builtin/show-ref.c b/builtin/show-ref.c\nindex 45f0340..fafb6dd 100644\n--- a/builtin/show-ref.c\n+++ b/builtin/show-ref.c\n@@ -145,7 +145,7 @@ static int exclude_existing(const char *match)\n \t\t\tif (strncmp(ref, match, matchlen))\n \t\t\t\tcontinue;\n \t\t}\n-\t\tif (check_ref_format(ref)) {\n+\t\tif (check_refname_format(ref, 0)) {\n \t\t\twarning(\"ref '%s' ignored\", ref);\n \t\t\tcontinue;\n \t\t}\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex 667515e..48be745 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -407,12 +407,12 @@ static int parse_msg_arg(const struct option *opt, const char *arg, int unset)\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+\t\treturn -1;\n \n \tstrbuf_reset(sb);\n \tstrbuf_addf(sb, \"refs/tags/%s\", name);\n \n-\treturn check_ref_format(sb->buf);\n+\treturn check_refname_format(sb->buf, 0);\n }\n \n int cmd_tag(int argc, const char **argv, const char *prefix)\ndiff --git a/connect.c b/connect.c\nindex ee1d4b4..51990fa 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -22,7 +22,7 @@ static int check_ref(const char *name, int len, unsigned int flags)\n \tlen -= 5;\n \n \t/* REF_NORMAL means that we don't want the magic fake tag refs */\n-\tif ((flags & REF_NORMAL) && check_ref_format(name) < 0)\n+\tif ((flags & REF_NORMAL) && check_refname_format(name, 0))\n \t\treturn 0;\n \n \t/* REF_HEADS means that we want regular branch heads */\ndiff --git a/environment.c b/environment.c\nindex e96edcf..8174b70 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -106,7 +106,7 @@ static char *expand_namespace(const char *raw_namespace)\n \t\tif (strcmp((*c)->buf, \"/\") != 0)\n \t\t\tstrbuf_addf(&buf, \"refs/namespaces/%s\", (*c)->buf);\n \tstrbuf_list_free(components);\n-\tif (check_ref_format(buf.buf) != CHECK_REF_FORMAT_OK)\n+\tif (check_refname_format(buf.buf, 0))\n \t\tdie(\"bad git namespace path \\\"%s\\\"\", raw_namespace);\n \tstrbuf_addch(&buf, '/');\n \treturn strbuf_detach(&buf, NULL);\ndiff --git a/fast-import.c b/fast-import.c\nindex 742e7da..f9347f5 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -722,13 +722,8 @@ static struct branch *new_branch(const char *name)\n \n \tif (b)\n \t\tdie(\"Invalid attempt to create duplicate branch: %s\", name);\n-\tswitch (check_ref_format(name)) {\n-\tcase 0: break; /* its valid */\n-\tcase CHECK_REF_FORMAT_ONELEVEL:\n-\t\tbreak; /* valid, but too few '/', allow anyway */\n-\tdefault:\n+\tif (check_refname_format(name, REFNAME_ALLOW_ONELEVEL))\n \t\tdie(\"Branch name doesn't conform to GIT standards: %s\", name);\n-\t}\n \n \tb = pool_calloc(1, sizeof(struct branch));\n \tb->name = pool_strdup(name);\ndiff --git a/git_remote_helpers/git/git.py b/git_remote_helpers/git/git.py\nindex a383e6c..007a1bf 100644\n--- a/git_remote_helpers/git/git.py\n+++ b/git_remote_helpers/git/git.py\n@@ -54,7 +54,7 @@ def valid_git_ref (ref_name):\n # The following is a reimplementation of the git check-ref-format\n # command. The rules were derived from the git check-ref-format(1)\n # manual page. This code should be replaced by a call to\n- # check_ref_format() in the git library, when such is available.\n+ # check_refname_format() in the git library, when such is available.\n if ref_name.endswith('/') or \\\n ref_name.startswith('.') or \\\n ref_name.count('/.') or \\\ndiff --git a/notes-merge.c b/notes-merge.c\nindex e1aaf43..3bbcc9d 100644\n--- a/notes-merge.c\n+++ b/notes-merge.c\n@@ -570,7 +570,8 @@ int notes_merge(struct notes_merge_options *o,\n \t/* Dereference o->local_ref into local_sha1 */\n \tif (!resolve_ref(o->local_ref, local_sha1, 0, NULL))\n \t\tdie(\"Failed to resolve local notes ref '%s'\", o->local_ref);\n-\telse if (!check_ref_format(o->local_ref) && is_null_sha1(local_sha1))\n+\telse if (!check_refname_format(o->local_ref, 0) &&\n+\t\tis_null_sha1(local_sha1))\n \t\tlocal = NULL; /* local_sha1 == null_sha1 indicates unborn ref */\n \telse if (!(local = lookup_commit_reference(local_sha1)))\n \t\tdie(\"Could not parse local commit %s (%s)\",\n@@ -583,7 +584,7 @@ int notes_merge(struct notes_merge_options *o,\n \t\t * Failed to get remote_sha1. If o->remote_ref looks like an\n \t\t * unborn ref, perform the merge using an empty notes tree.\n \t\t */\n-\t\tif (!check_ref_format(o->remote_ref)) {\n+\t\tif (!check_refname_format(o->remote_ref, 0)) {\n \t\t\thashclr(remote_sha1);\n \t\t\tremote = NULL;\n \t\t} else {\ndiff --git a/pack-refs.c b/pack-refs.c\nindex 1290570..23bbd00 100644\n--- a/pack-refs.c\n+++ b/pack-refs.c\n@@ -72,7 +72,7 @@ static void try_remove_empty_parents(char *name)\n \tfor (i = 0; i < 2; i++) { /* refs/{heads,tags,...}/ */\n \t\twhile (*p && *p != '/')\n \t\t\tp++;\n-\t\t/* tolerate duplicate slashes; see check_ref_format() */\n+\t\t/* tolerate duplicate slashes; see check_refname_format() */\n \t\twhile (*p == '/')\n \t\t\tp++;\n \t}\ndiff --git a/refs.c b/refs.c\nindex fd29d89..aaa8730 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -872,10 +872,9 @@ static inline int bad_ref_char(int ch)\n \treturn 0;\n }\n \n-int check_ref_format(const char *ref)\n+int check_refname_format(const char *ref, int flags)\n {\n \tint ch, level, last;\n-\tint ret = CHECK_REF_FORMAT_OK;\n \tconst char *cp = ref;\n \n \tlevel = 0;\n@@ -884,41 +883,42 @@ int check_ref_format(const char *ref)\n \t\t\t; /* tolerate duplicated slashes */\n \t\tif (!ch)\n \t\t\t/* should not end with slashes */\n-\t\t\treturn CHECK_REF_FORMAT_ERROR;\n+\t\t\treturn -1;\n \n \t\t/* we are at the beginning of the path component */\n \t\tif (ch == '.')\n-\t\t\treturn CHECK_REF_FORMAT_ERROR;\n+\t\t\treturn -1;\n \t\tif (bad_ref_char(ch)) {\n-\t\t\tif (ch == '*' && (!*cp || *cp == '/') &&\n-\t\t\t ret == CHECK_REF_FORMAT_OK)\n-\t\t\t\tret = CHECK_REF_FORMAT_WILDCARD;\n+\t\t\tif ((flags & REFNAME_REFSPEC_PATTERN) && ch == '*' &&\n+\t\t\t\t(!*cp || *cp == '/'))\n+\t\t\t\t/* Accept one wildcard as a full refname component. */\n+\t\t\t\tflags &= ~REFNAME_REFSPEC_PATTERN;\n \t\t\telse\n-\t\t\t\treturn CHECK_REF_FORMAT_ERROR;\n+\t\t\t\treturn -1;\n \t\t}\n \n \t\tlast = ch;\n \t\t/* scan the rest of the path component */\n \t\twhile ((ch = *cp++) != 0) {\n \t\t\tif (bad_ref_char(ch))\n-\t\t\t\treturn CHECK_REF_FORMAT_ERROR;\n+\t\t\t\treturn -1;\n \t\t\tif (ch == '/')\n \t\t\t\tbreak;\n \t\t\tif (last == '.' && ch == '.')\n-\t\t\t\treturn CHECK_REF_FORMAT_ERROR;\n+\t\t\t\treturn -1;\n \t\t\tif (last == '@' && ch == '{')\n-\t\t\t\treturn CHECK_REF_FORMAT_ERROR;\n+\t\t\t\treturn -1;\n \t\t\tlast = ch;\n \t\t}\n \t\tlevel++;\n \t\tif (!ch) {\n \t\t\tif (ref <= cp - 2 && cp[-2] == '.')\n-\t\t\t\treturn CHECK_REF_FORMAT_ERROR;\n-\t\t\tif (level < 2)\n-\t\t\t\treturn CHECK_REF_FORMAT_ONELEVEL;\n+\t\t\t\treturn -1;\n+\t\t\tif (level < 2 && !(flags & REFNAME_ALLOW_ONELEVEL))\n+\t\t\t\treturn -1;\n \t\t\tif (has_extension(ref, \".lock\"))\n-\t\t\t\treturn CHECK_REF_FORMAT_ERROR;\n-\t\t\treturn ret;\n+\t\t\t\treturn -1;\n+\t\t\treturn 0;\n \t\t}\n \t}\n }\n@@ -1103,7 +1103,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *ref, const unsigned char\n struct ref_lock *lock_ref_sha1(const char *ref, const unsigned char *old_sha1)\n {\n \tchar refpath[PATH_MAX];\n-\tif (check_ref_format(ref))\n+\tif (check_refname_format(ref, 0))\n \t\treturn NULL;\n \tstrcpy(refpath, mkpath(\"refs/%s\", ref));\n \treturn lock_ref_sha1_basic(refpath, old_sha1, 0, NULL);\n@@ -1111,13 +1111,9 @@ struct ref_lock *lock_ref_sha1(const char *ref, const unsigned char *old_sha1)\n \n struct ref_lock *lock_any_ref_for_update(const char *ref, const unsigned char *old_sha1, int flags)\n {\n-\tswitch (check_ref_format(ref)) {\n-\tdefault:\n+\tif (check_refname_format(ref, REFNAME_ALLOW_ONELEVEL))\n \t\treturn NULL;\n-\tcase 0:\n-\tcase CHECK_REF_FORMAT_ONELEVEL:\n-\t\treturn lock_ref_sha1_basic(ref, old_sha1, flags, NULL);\n-\t}\n+\treturn lock_ref_sha1_basic(ref, old_sha1, flags, NULL);\n }\n \n static struct lock_file packlock;\ndiff --git a/refs.h b/refs.h\nindex dfb086e..48540c0 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -97,11 +97,18 @@ int for_each_recent_reflog_ent(const char *ref, each_reflog_ent_fn fn, long, voi\n */\n extern int for_each_reflog(each_ref_fn, void *);\n \n-#define CHECK_REF_FORMAT_OK 0\n-#define CHECK_REF_FORMAT_ERROR (-1)\n-#define CHECK_REF_FORMAT_ONELEVEL (-2)\n-#define CHECK_REF_FORMAT_WILDCARD (-3)\n-extern int check_ref_format(const char *target);\n+#define REFNAME_ALLOW_ONELEVEL 1\n+#define REFNAME_REFSPEC_PATTERN 2\n+\n+/*\n+ * Return 0 iff ref has the correct format for a refname according to\n+ * the rules described in Documentation/git-check-ref-format.txt. If\n+ * REFNAME_ALLOW_ONELEVEL is set in flags, then accept one-level\n+ * reference names. If REFNAME_REFSPEC_PATTERN is set in flags, then\n+ * allow a \"*\" wildcard character in place of one of the name\n+ * components.\n+ */\n+extern int check_refname_format(const char *ref, int flags);\n \n extern const char *prettify_refname(const char *refname);\n extern char *shorten_unambiguous_ref(const char *ref, int strict);\ndiff --git a/remote.c b/remote.c\nindex b8ecfa5..6fcf809 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -493,23 +493,6 @@ static void read_config(void)\n }\n \n /*\n- * We need to make sure the remote-tracking branches are well formed, but a\n- * wildcard refspec in \"struct refspec\" must have a trailing slash. We\n- * temporarily drop the trailing '/' while calling check_ref_format(),\n- * and put it back. The caller knows that a CHECK_REF_FORMAT_ONELEVEL\n- * error return is Ok for a wildcard refspec.\n- */\n-static int verify_refname(char *name, int is_glob)\n-{\n-\tint result;\n-\n-\tresult = check_ref_format(name);\n-\tif (is_glob && result == CHECK_REF_FORMAT_WILDCARD)\n-\t\tresult = CHECK_REF_FORMAT_OK;\n-\treturn result;\n-}\n-\n-/*\n * This function frees a refspec array.\n * Warning: code paths should be checked to ensure that the src\n * and dst pointers are always freeable pointers as well\n@@ -532,13 +515,13 @@ static void free_refspecs(struct refspec *refspec, int nr_refspec)\n static struct refspec *parse_refspec_internal(int nr_refspec, const char **refspec, int fetch, int verify)\n {\n \tint i;\n-\tint st;\n \tstruct refspec *rs = xcalloc(sizeof(*rs), nr_refspec);\n \n \tfor (i = 0; i < nr_refspec; i++) {\n \t\tsize_t llen;\n \t\tint is_glob;\n \t\tconst char *lhs, *rhs;\n+\t\tint flags;\n \n \t\tis_glob = 0;\n \n@@ -576,6 +559,7 @@ static struct refspec *parse_refspec_internal(int nr_refspec, const char **refsp\n \n \t\trs[i].pattern = is_glob;\n \t\trs[i].src = xstrndup(lhs, llen);\n+\t\tflags = REFNAME_ALLOW_ONELEVEL | (is_glob ? REFNAME_REFSPEC_PATTERN : 0);\n \n \t\tif (fetch) {\n \t\t\t/*\n@@ -585,26 +569,20 @@ static struct refspec *parse_refspec_internal(int nr_refspec, const char **refsp\n \t\t\t */\n \t\t\tif (!*rs[i].src)\n \t\t\t\t; /* empty is ok */\n-\t\t\telse {\n-\t\t\t\tst = verify_refname(rs[i].src, is_glob);\n-\t\t\t\tif (st && st != CHECK_REF_FORMAT_ONELEVEL)\n-\t\t\t\t\tgoto invalid;\n-\t\t\t}\n+\t\t\telse if (check_refname_format(rs[i].src, flags))\n+\t\t\t\tgoto invalid;\n \t\t\t/*\n \t\t\t * RHS\n \t\t\t * - missing is ok, and is same as empty.\n \t\t\t * - empty is ok; it means not to store.\n \t\t\t * - otherwise it must be a valid looking ref.\n \t\t\t */\n-\t\t\tif (!rs[i].dst) {\n+\t\t\tif (!rs[i].dst)\n \t\t\t\t; /* ok */\n-\t\t\t} else if (!*rs[i].dst) {\n+\t\t\telse if (!*rs[i].dst)\n \t\t\t\t; /* ok */\n-\t\t\t} else {\n-\t\t\t\tst = verify_refname(rs[i].dst, is_glob);\n-\t\t\t\tif (st && st != CHECK_REF_FORMAT_ONELEVEL)\n-\t\t\t\t\tgoto invalid;\n-\t\t\t}\n+\t\t\telse if (check_refname_format(rs[i].dst, flags))\n+\t\t\t\tgoto invalid;\n \t\t} else {\n \t\t\t/*\n \t\t\t * LHS\n@@ -616,8 +594,7 @@ static struct refspec *parse_refspec_internal(int nr_refspec, const char **refsp\n \t\t\tif (!*rs[i].src)\n \t\t\t\t; /* empty is ok */\n \t\t\telse if (is_glob) {\n-\t\t\t\tst = verify_refname(rs[i].src, is_glob);\n-\t\t\t\tif (st && st != CHECK_REF_FORMAT_ONELEVEL)\n+\t\t\t\tif (check_refname_format(rs[i].src, flags))\n \t\t\t\t\tgoto invalid;\n \t\t\t}\n \t\t\telse\n@@ -630,14 +607,12 @@ static struct refspec *parse_refspec_internal(int nr_refspec, const char **refsp\n \t\t\t * - otherwise it must be a valid looking ref.\n \t\t\t */\n \t\t\tif (!rs[i].dst) {\n-\t\t\t\tst = verify_refname(rs[i].src, is_glob);\n-\t\t\t\tif (st && st != CHECK_REF_FORMAT_ONELEVEL)\n+\t\t\t\tif (check_refname_format(rs[i].src, flags))\n \t\t\t\t\tgoto invalid;\n \t\t\t} else if (!*rs[i].dst) {\n \t\t\t\tgoto invalid;\n \t\t\t} else {\n-\t\t\t\tst = verify_refname(rs[i].dst, is_glob);\n-\t\t\t\tif (st && st != CHECK_REF_FORMAT_ONELEVEL)\n+\t\t\t\tif (check_refname_format(rs[i].dst, flags))\n \t\t\t\t\tgoto invalid;\n \t\t\t}\n \t\t}\n@@ -1427,8 +1402,8 @@ int get_fetch_map(const struct ref *remote_refs,\n \n \tfor (rmp = &ref_map; *rmp; ) {\n \t\tif ((*rmp)->peer_ref) {\n-\t\t\tint st = check_ref_format((*rmp)->peer_ref->name + 5);\n-\t\t\tif (st && st != CHECK_REF_FORMAT_ONELEVEL) {\n+\t\t\tif (check_refname_format((*rmp)->peer_ref->name + 5,\n+\t\t\t\tREFNAME_ALLOW_ONELEVEL)) {\n \t\t\t\tstruct ref *ignore = *rmp;\n \t\t\t\terror(\"* Ignoring funny ref '%s' locally\",\n \t\t\t\t (*rmp)->peer_ref->name);\n@@ -1620,7 +1595,7 @@ static int one_local_ref(const char *refname, const unsigned char *sha1, int fla\n \tint len;\n \n \t/* we already know it starts with refs/ to get here */\n-\tif (check_ref_format(refname + 5))\n+\tif (check_refname_format(refname + 5, 0))\n \t\treturn 0;\n \n \tlen = strlen(refname) + 1;\ndiff --git a/sha1_name.c b/sha1_name.c\nindex ff5992a..143fd97 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -972,9 +972,9 @@ int strbuf_check_branch_ref(struct strbuf *sb, const char *name)\n {\n \tstrbuf_branchname(sb, name);\n \tif (name[0] == '-')\n-\t\treturn CHECK_REF_FORMAT_ERROR;\n+\t\treturn -1;\n \tstrbuf_splice(sb, 0, 0, \"refs/heads/\", 11);\n-\treturn check_ref_format(sb->buf);\n+\treturn check_refname_format(sb->buf, 0);\n }\n \n /*\ndiff --git a/t/t1402-check-ref-format.sh b/t/t1402-check-ref-format.sh\nindex f551eef..1cad88f 100755\n--- a/t/t1402-check-ref-format.sh\n+++ b/t/t1402-check-ref-format.sh\n@@ -87,11 +87,7 @@ valid_ref \"$ref\" '--refspec-pattern --allow-onelevel'\n \n ref='*'\n invalid_ref \"$ref\"\n-\n-#invalid_ref \"$ref\" --allow-onelevel\n-test_expect_failure \"ref name '$ref' is invalid with options --allow-onelevel\" \\\n-\t\"test_must_fail git check-ref-format --allow-onelevel '$ref'\"\n-\n+invalid_ref \"$ref\" --allow-onelevel\n invalid_ref \"$ref\" --refspec-pattern\n valid_ref \"$ref\" '--refspec-pattern --allow-onelevel'\n \ndiff --git a/transport.c b/transport.c\nindex fa279d5..feb2ff5 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -754,18 +754,10 @@ void transport_verify_remote_names(int nr_heads, const char **heads)\n \t\t\tcontinue;\n \n \t\tremote = remote ? (remote + 1) : local;\n-\t\tswitch (check_ref_format(remote)) {\n-\t\tcase 0: /* ok */\n-\t\tcase CHECK_REF_FORMAT_ONELEVEL:\n-\t\t\t/* ok but a single level -- that is fine for\n-\t\t\t * a match pattern.\n-\t\t\t */\n-\t\tcase CHECK_REF_FORMAT_WILDCARD:\n-\t\t\t/* ok but ends with a pattern-match character */\n-\t\t\tcontinue;\n-\t\t}\n-\t\tdie(\"remote part of refspec is not a valid name in %s\",\n-\t\t heads[i]);\n+\t\tif (check_refname_format(remote,\n+\t\t\t\tREFNAME_ALLOW_ONELEVEL|REFNAME_REFSPEC_PATTERN))\n+\t\t\tdie(\"remote part of refspec is not a valid name in %s\",\n+\t\t\t\theads[i]);\n \t}\n }\n \ndiff --git a/walker.c b/walker.c\nindex dce7128..be389dc 100644\n--- a/walker.c\n+++ b/walker.c\n@@ -190,7 +190,7 @@ static int interpret_target(struct walker *walker, char *target, unsigned char *\n {\n \tif (!get_sha1_hex(target, sha1))\n \t\treturn 0;\n-\tif (!check_ref_format(target)) {\n+\tif (!check_refname_format(target, 0)) {\n \t\tstruct ref *ref = alloc_ref(target);\n \t\tif (!walker->fetch_ref(walker, ref)) {\n \t\t\thashcpy(sha1, ref->old_sha1);\n-- \n1.7.6.8.gd2879\n"},{"id":"175613","messageId":"1316121043-29367-6-git-send-email-mhagger@alum.mit.edu","threadId":"28403","inReplyTo":"1316121043-29367-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v3 05/22] Refactor check_refname_format()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2011-09-15T21:10:26Z","receivedAt":"2011-09-15T21:10:26Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Among other things, extract a function check_refname_component().\n\nSigned-off-by: Michael Haggerty \n---\n refs.c | 95 +++++++++++++++++++++++++++++++++++++---------------------------\n 1 files changed, 55 insertions(+), 40 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex aaa8730..5259724 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -872,55 +872,70 @@ static inline int bad_ref_char(int ch)\n \treturn 0;\n }\n \n+/*\n+ * Try to read one refname component from the front of ref. Return\n+ * the length of the component found, or -1 if the component is not\n+ * legal.\n+ */\n+static int check_refname_component(const char *ref)\n+{\n+\tconst char *cp;\n+\tchar last = '\\0';\n+\n+\tfor (cp = ref; ; cp++) {\n+\t\tchar ch = *cp;\n+\t\tif (ch == '\\0' || ch == '/')\n+\t\t\tbreak;\n+\t\tif (bad_ref_char(ch))\n+\t\t\treturn -1; /* Illegal character in refname. */\n+\t\tif (last == '.' && ch == '.')\n+\t\t\treturn -1; /* Refname contains \"..\". */\n+\t\tif (last == '@' && ch == '{')\n+\t\t\treturn -1; /* Refname contains \"@{\". */\n+\t\tlast = ch;\n+\t}\n+\tif (cp == ref)\n+\t\treturn -1; /* Component has zero length. */\n+\tif (ref[0] == '.')\n+\t\treturn -1; /* Component starts with '.'. */\n+\treturn cp - ref;\n+}\n+\n int check_refname_format(const char *ref, int flags)\n {\n-\tint ch, level, last;\n-\tconst char *cp = ref;\n+\tint component_len, component_count = 0;\n \n-\tlevel = 0;\n \twhile (1) {\n-\t\twhile ((ch = *cp++) == '/')\n-\t\t\t; /* tolerate duplicated slashes */\n-\t\tif (!ch)\n-\t\t\t/* should not end with slashes */\n-\t\t\treturn -1;\n-\n-\t\t/* we are at the beginning of the path component */\n-\t\tif (ch == '.')\n-\t\t\treturn -1;\n-\t\tif (bad_ref_char(ch)) {\n-\t\t\tif ((flags & REFNAME_REFSPEC_PATTERN) && ch == '*' &&\n-\t\t\t\t(!*cp || *cp == '/'))\n+\t\twhile (*ref == '/')\n+\t\t\tref++; /* tolerate leading and repeated slashes */\n+\n+\t\t/* We are at the start of a path component. */\n+\t\tcomponent_len = check_refname_component(ref);\n+\t\tif (component_len < 0) {\n+\t\t\tif ((flags & REFNAME_REFSPEC_PATTERN) &&\n+\t\t\t\t\tref[0] == '*' &&\n+\t\t\t\t\t(ref[1] == '\\0' || ref[1] == '/')) {\n \t\t\t\t/* Accept one wildcard as a full refname component. */\n \t\t\t\tflags &= ~REFNAME_REFSPEC_PATTERN;\n-\t\t\telse\n-\t\t\t\treturn -1;\n-\t\t}\n-\n-\t\tlast = ch;\n-\t\t/* scan the rest of the path component */\n-\t\twhile ((ch = *cp++) != 0) {\n-\t\t\tif (bad_ref_char(ch))\n-\t\t\t\treturn -1;\n-\t\t\tif (ch == '/')\n-\t\t\t\tbreak;\n-\t\t\tif (last == '.' && ch == '.')\n-\t\t\t\treturn -1;\n-\t\t\tif (last == '@' && ch == '{')\n-\t\t\t\treturn -1;\n-\t\t\tlast = ch;\n-\t\t}\n-\t\tlevel++;\n-\t\tif (!ch) {\n-\t\t\tif (ref <= cp - 2 && cp[-2] == '.')\n-\t\t\t\treturn -1;\n-\t\t\tif (level < 2 && !(flags & REFNAME_ALLOW_ONELEVEL))\n-\t\t\t\treturn -1;\n-\t\t\tif (has_extension(ref, \".lock\"))\n+\t\t\t\tcomponent_len = 1;\n+\t\t\t} else {\n \t\t\t\treturn -1;\n-\t\t\treturn 0;\n+\t\t\t}\n \t\t}\n+\t\tcomponent_count++;\n+\t\tif (ref[component_len] == '\\0')\n+\t\t\tbreak;\n+\t\t/* Skip to next component. */\n+\t\tref += component_len + 1;\n \t}\n+\n+\tif (ref[component_len - 1] == '.')\n+\t\treturn -1; /* Refname ends with '.'. */\n+\tif (component_len >= 5 && !memcmp(&ref[component_len - 5], \".lock\", 5))\n+\t\treturn -1; /* Refname ends with \".lock\". */\n+\tif (!(flags & REFNAME_ALLOW_ONELEVEL) && component_count < 2)\n+\t\treturn -1; /* Refname has only one component. */\n+\treturn 0;\n }\n \n const char *prettify_refname(const char *name)\n-- \n1.7.6.8.gd2879\n"},{"id":"175608","messageId":"1316121043-29367-7-git-send-email-mhagger@alum.mit.edu","threadId":"28403","inReplyTo":"1316121043-29367-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v3 06/22] Do not allow \".lock\" at the end of any refname component","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2011-09-15T21:10:27Z","receivedAt":"2011-09-15T21:10:27Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Allowing any refname component to end with \".lock\" is looking for\ntrouble; for example,\n\n $ git br foo.lock/bar\n $ git br foo\n fatal: Unable to create '[...]/.git/refs/heads/foo.lock': File exists.\n\nTherefore, do not allow any refname component to end with \".lock\".\n\nSigned-off-by: Michael Haggerty \n---\n\nThis change was discussed on the mailing list [1]. It is regrettable\nthat we can't change the name of the lock files to something that\ncannot appear in a reference name (like .refname.lock), but such a\nchange would cause problems if two versions of git are simultaneously\naccessing the same repository.\n\n[1] http://thread.gmane.org/gmane.comp.version-control.git/181051/focus=181069\n\n Documentation/git-check-ref-format.txt | 4 +---\n refs.c | 4 ++--\n t/t1402-check-ref-format.sh | 8 ++++----\n 3 files changed, 7 insertions(+), 9 deletions(-)\n\ndiff --git a/Documentation/git-check-ref-format.txt b/Documentation/git-check-ref-format.txt\nindex dcb8cc3..9114751 100644\n--- a/Documentation/git-check-ref-format.txt\n+++ b/Documentation/git-check-ref-format.txt\n@@ -28,7 +28,7 @@ git imposes the following rules on how references are named:\n \n . They can include slash `/` for hierarchical (directory)\n grouping, but no slash-separated component can begin with a\n- dot `.`.\n+ dot `.` or end with the sequence `.lock`.\n \n . They must contain at least one `/`. This enforces the presence of a\n category like `heads/`, `tags/` etc. but the actual names are not\n@@ -47,8 +47,6 @@ git imposes the following rules on how references are named:\n \n . They cannot end with a slash `/` nor a dot `.`.\n \n-. They cannot end with the sequence `.lock`.\n-\n . They cannot contain a sequence `@{`.\n \n . They cannot contain a `\\`.\ndiff --git a/refs.c b/refs.c\nindex 5259724..5a0bd0f 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -898,6 +898,8 @@ static int check_refname_component(const char *ref)\n \t\treturn -1; /* Component has zero length. */\n \tif (ref[0] == '.')\n \t\treturn -1; /* Component starts with '.'. */\n+\tif (cp - ref >= 5 && !memcmp(cp - 5, \".lock\", 5))\n+\t\treturn -1; /* Refname ends with \".lock\". */\n \treturn cp - ref;\n }\n \n@@ -931,8 +933,6 @@ int check_refname_format(const char *ref, int flags)\n \n \tif (ref[component_len - 1] == '.')\n \t\treturn -1; /* Refname ends with '.'. */\n-\tif (component_len >= 5 && !memcmp(&ref[component_len - 5], \".lock\", 5))\n-\t\treturn -1; /* Refname ends with \".lock\". */\n \tif (!(flags & REFNAME_ALLOW_ONELEVEL) && component_count < 2)\n \t\treturn -1; /* Refname has only one component. */\n \treturn 0;\ndiff --git a/t/t1402-check-ref-format.sh b/t/t1402-check-ref-format.sh\nindex 1cad88f..419788f 100755\n--- a/t/t1402-check-ref-format.sh\n+++ b/t/t1402-check-ref-format.sh\n@@ -43,8 +43,8 @@ invalid_ref 'heads/foo?bar'\n valid_ref 'foo./bar'\n invalid_ref 'heads/foo.lock'\n invalid_ref 'heads///foo.lock'\n-valid_ref 'foo.lock/bar'\n-valid_ref 'foo.lock///bar'\n+invalid_ref 'foo.lock/bar'\n+invalid_ref 'foo.lock///bar'\n valid_ref 'heads/foo@bar'\n invalid_ref 'heads/v@{ation'\n invalid_ref 'heads/foo\\bar'\n@@ -160,7 +160,7 @@ invalid_ref_normalized 'heads/./foo'\n invalid_ref_normalized 'heads\\foo'\n invalid_ref_normalized 'heads/foo.lock'\n invalid_ref_normalized 'heads///foo.lock'\n-valid_ref_normalized 'foo.lock/bar' 'foo.lock/bar'\n-valid_ref_normalized 'foo.lock///bar' 'foo.lock/bar'\n+invalid_ref_normalized 'foo.lock/bar'\n+invalid_ref_normalized 'foo.lock///bar'\n \n test_done\n-- \n1.7.6.8.gd2879\n"},{"id":"175614","messageId":"1316121043-29367-8-git-send-email-mhagger@alum.mit.edu","threadId":"28403","inReplyTo":"1316121043-29367-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v3 07/22] Make collapse_slashes() allocate memory for its result","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2011-09-15T21:10:28Z","receivedAt":"2011-09-15T21:10:28Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"This will make upcoming changes a tiny bit easier.\n\nSigned-off-by: Michael Haggerty \n---\n builtin/check-ref-format.c | 19 ++++++++++---------\n 1 files changed, 10 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/check-ref-format.c b/builtin/check-ref-format.c\nindex 8f41696..989ee5c 100644\n--- a/builtin/check-ref-format.c\n+++ b/builtin/check-ref-format.c\n@@ -12,25 +12,28 @@ static const char builtin_check_ref_format_usage[] =\n \" or: git check-ref-format --branch \";\n \n /*\n- * Remove leading slashes and replace each run of adjacent slashes in\n- * src with a single slash, and write the result to dst.\n+ * Return a copy of refname but with leading slashes removed and runs\n+ * of adjacent slashes replaced with single slashes.\n *\n * This function is similar to normalize_path_copy(), but stripped down\n * to meet check_ref_format's simpler needs.\n */\n-static void collapse_slashes(char *dst, const char *src)\n+static char *collapse_slashes(const char *refname)\n {\n+\tchar *ret = xmalloc(strlen(refname) + 1);\n \tchar ch;\n \tchar prev = '/';\n+\tchar *cp = ret;\n \n-\twhile ((ch = *src++) != '\\0') {\n+\twhile ((ch = *refname++) != '\\0') {\n \t\tif (prev == '/' && ch == prev)\n \t\t\tcontinue;\n \n-\t\t*dst++ = ch;\n+\t\t*cp++ = ch;\n \t\tprev = ch;\n \t}\n-\t*dst = '\\0';\n+\t*cp = '\\0';\n+\treturn ret;\n }\n \n static int check_ref_format_branch(const char *arg)\n@@ -47,9 +50,7 @@ static int check_ref_format_branch(const char *arg)\n \n static void refname_format_print(const char *arg)\n {\n-\tchar *refname = xmalloc(strlen(arg) + 1);\n-\n-\tcollapse_slashes(refname, arg);\n+\tchar *refname = collapse_slashes(arg);\n \tprintf(\"%s\\n\", refname);\n }\n \n-- \n1.7.6.8.gd2879\n"},{"id":"175611","messageId":"1316121043-29367-9-git-send-email-mhagger@alum.mit.edu","threadId":"28403","inReplyTo":"1316121043-29367-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v3 08/22] Inline function refname_format_print()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2011-09-15T21:10:29Z","receivedAt":"2011-09-15T21:10:29Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Soon we will make printing independent of collapsing.\n\nSigned-off-by: Michael Haggerty \n---\n builtin/check-ref-format.c | 16 +++++++---------\n 1 files changed, 7 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/check-ref-format.c b/builtin/check-ref-format.c\nindex 989ee5c..f5df9aa 100644\n--- a/builtin/check-ref-format.c\n+++ b/builtin/check-ref-format.c\n@@ -48,17 +48,12 @@ static int check_ref_format_branch(const char *arg)\n \treturn 0;\n }\n \n-static void refname_format_print(const char *arg)\n-{\n-\tchar *refname = collapse_slashes(arg);\n-\tprintf(\"%s\\n\", refname);\n-}\n-\n int cmd_check_ref_format(int argc, const char **argv, const char *prefix)\n {\n \tint i;\n \tint print = 0;\n \tint flags = 0;\n+\tconst char *refname;\n \n \tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n \t\tusage(builtin_check_ref_format_usage);\n@@ -81,11 +76,14 @@ int cmd_check_ref_format(int argc, const char **argv, const char *prefix)\n \tif (! (i == argc - 1))\n \t\tusage(builtin_check_ref_format_usage);\n \n-\tif (check_refname_format(argv[i], flags))\n+\trefname = argv[i];\n+\tif (check_refname_format(refname, flags))\n \t\treturn 1;\n \n-\tif (print)\n-\t\trefname_format_print(argv[i]);\n+\tif (print) {\n+\t\trefname = collapse_slashes(refname);\n+\t\tprintf(\"%s\\n\", refname);\n+\t}\n \n \treturn 0;\n }\n-- \n1.7.6.8.gd2879\n"},{"id":"175612","messageId":"1316121043-29367-10-git-send-email-mhagger@alum.mit.edu","threadId":"28403","inReplyTo":"1316121043-29367-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v3 09/22] Change check_refname_format() to reject unnormalized refnames","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2011-09-15T21:10:30Z","receivedAt":"2011-09-15T21:10:30Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Since much of the infrastructure does not work correctly with\nunnormalized refnames, change check_refname_format() to reject them.\n\nSimilarly, change \"git check-ref-format\" to reject unnormalized\nrefnames by default. But add an option --normalize, which causes \"git\ncheck-ref-format\" to normalize the refname before checking its format,\nand print the normalized refname. This is exactly the behavior of the\nold --print option, which is retained but deprecated.\n\nSigned-off-by: Michael Haggerty \n---\n Documentation/git-check-ref-format.txt | 26 ++++++++++++++++++--------\n builtin/check-ref-format.c | 15 +++++++--------\n refs.c | 3 ---\n refs.h | 2 +-\n t/t1402-check-ref-format.sh | 31 +++++++++++++++++++++++--------\n 5 files changed, 49 insertions(+), 28 deletions(-)\n\ndiff --git a/Documentation/git-check-ref-format.txt b/Documentation/git-check-ref-format.txt\nindex 9114751..103e7b1 100644\n--- a/Documentation/git-check-ref-format.txt\n+++ b/Documentation/git-check-ref-format.txt\n@@ -8,8 +8,9 @@ git-check-ref-format - Ensures that a reference name is well formed\n SYNOPSIS\n --------\n [verse]\n-'git check-ref-format' [--print]\n- [--[no-]allow-onelevel] [--refspec-pattern] \n+'git check-ref-format' [--normalize]\n+ [--[no-]allow-onelevel] [--refspec-pattern]\n+ \n 'git check-ref-format' --branch \n \n DESCRIPTION\n@@ -45,7 +46,11 @@ git imposes the following rules on how references are named:\n bracket `[` anywhere. See the `--refspec-pattern` option below for\n an exception to this rule.\n \n-. They cannot end with a slash `/` nor a dot `.`.\n+. They cannot begin or end with a slash `/` or contain multiple\n+ consecutive slashes (see the `--normalize` option below for an\n+ exception to this rule)\n+\n+. They cannot end with a dot `.`.\n \n . They cannot contain a sequence `@{`.\n \n@@ -70,10 +75,6 @@ reference name expressions (see linkgit:gitrevisions[7]):\n \n . at-open-brace `@{` is used as a notation to access a reflog entry.\n \n-With the `--print` option, if 'refname' is acceptable, it prints the\n-canonicalized name of a hypothetical reference with that name. That is,\n-it prints 'refname' with any extra `/` characters removed.\n-\n With the `--branch` option, it expands the ``previous branch syntax''\n `@{-n}`. For example, `@{-1}` is a way to refer the last branch you\n were on. This option should be used by porcelains to accept this\n@@ -95,6 +96,15 @@ OPTIONS\n \tin place of a one full pathname component (e.g.,\n \t`foo/{asterisk}/bar` but not `foo/bar{asterisk}`).\n \n+--normalize::\n+\tNormalize 'refname' by removing any leading slash (`/`)\n+\tcharacters and collapsing runs of adjacent slashes between\n+\tname components into a single slash. Iff the normalized\n+\trefname is valid then print it to standard output and exit\n+\twith a status of 0. (`--print` is a deprecated way to spell\n+\t`--normalize`.)\n+\n+\n EXAMPLES\n --------\n \n@@ -107,7 +117,7 @@ $ git check-ref-format --branch @{-1}\n * Determine the reference name to use for a new branch:\n +\n ------------\n-$ ref=$(git check-ref-format --print \"refs/heads/$newbranch\") ||\n+$ ref=$(git check-ref-format --normalize \"refs/heads/$newbranch\") ||\n die \"we do not like '$newbranch' as a branch name.\"\n ------------\n \ndiff --git a/builtin/check-ref-format.c b/builtin/check-ref-format.c\nindex f5df9aa..28a7320 100644\n--- a/builtin/check-ref-format.c\n+++ b/builtin/check-ref-format.c\n@@ -8,7 +8,7 @@\n #include \"strbuf.h\"\n \n static const char builtin_check_ref_format_usage[] =\n-\"git check-ref-format [--print] [options] \\n\"\n+\"git check-ref-format [--normalize] [options] \\n\"\n \" or: git check-ref-format --branch \";\n \n /*\n@@ -51,7 +51,7 @@ static int check_ref_format_branch(const char *arg)\n int cmd_check_ref_format(int argc, const char **argv, const char *prefix)\n {\n \tint i;\n-\tint print = 0;\n+\tint normalize = 0;\n \tint flags = 0;\n \tconst char *refname;\n \n@@ -62,8 +62,8 @@ int cmd_check_ref_format(int argc, const char **argv, const char *prefix)\n \t\treturn check_ref_format_branch(argv[2]);\n \n \tfor (i = 1; i < argc && argv[i][0] == '-'; i++) {\n-\t\tif (!strcmp(argv[i], \"--print\"))\n-\t\t\tprint = 1;\n+\t\tif (!strcmp(argv[i], \"--normalize\") || !strcmp(argv[i], \"--print\"))\n+\t\t\tnormalize = 1;\n \t\telse if (!strcmp(argv[i], \"--allow-onelevel\"))\n \t\t\tflags |= REFNAME_ALLOW_ONELEVEL;\n \t\telse if (!strcmp(argv[i], \"--no-allow-onelevel\"))\n@@ -77,13 +77,12 @@ int cmd_check_ref_format(int argc, const char **argv, const char *prefix)\n \t\tusage(builtin_check_ref_format_usage);\n \n \trefname = argv[i];\n+\tif (normalize)\n+\t\trefname = collapse_slashes(refname);\n \tif (check_refname_format(refname, flags))\n \t\treturn 1;\n-\n-\tif (print) {\n-\t\trefname = collapse_slashes(refname);\n+\tif (normalize)\n \t\tprintf(\"%s\\n\", refname);\n-\t}\n \n \treturn 0;\n }\ndiff --git a/refs.c b/refs.c\nindex 5a0bd0f..d2aac24 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -908,9 +908,6 @@ int check_refname_format(const char *ref, int flags)\n \tint component_len, component_count = 0;\n \n \twhile (1) {\n-\t\twhile (*ref == '/')\n-\t\t\tref++; /* tolerate leading and repeated slashes */\n-\n \t\t/* We are at the start of a path component. */\n \t\tcomponent_len = check_refname_component(ref);\n \t\tif (component_len < 0) {\ndiff --git a/refs.h b/refs.h\nindex 48540c0..b0da5fc 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -106,7 +106,7 @@ extern int for_each_reflog(each_ref_fn, void *);\n * REFNAME_ALLOW_ONELEVEL is set in flags, then accept one-level\n * reference names. If REFNAME_REFSPEC_PATTERN is set in flags, then\n * allow a \"*\" wildcard character in place of one of the name\n- * components.\n+ * components. No leading or repeated slashes are accepted.\n */\n extern int check_refname_format(const char *ref, int flags);\n \ndiff --git a/t/t1402-check-ref-format.sh b/t/t1402-check-ref-format.sh\nindex 419788f..710fcca 100755\n--- a/t/t1402-check-ref-format.sh\n+++ b/t/t1402-check-ref-format.sh\n@@ -28,11 +28,17 @@ invalid_ref() {\n invalid_ref ''\n invalid_ref '/'\n invalid_ref '/' --allow-onelevel\n+invalid_ref '/' --normalize\n+invalid_ref '/' '--allow-onelevel --normalize'\n valid_ref 'foo/bar/baz'\n-valid_ref 'refs///heads/foo'\n+valid_ref 'foo/bar/baz' --normalize\n+invalid_ref 'refs///heads/foo'\n+valid_ref 'refs///heads/foo' --normalize\n invalid_ref 'heads/foo/'\n-valid_ref '/heads/foo'\n-valid_ref '///heads/foo'\n+invalid_ref '/heads/foo'\n+valid_ref '/heads/foo' --normalize\n+invalid_ref '///heads/foo'\n+valid_ref '///heads/foo' --normalize\n invalid_ref './foo'\n invalid_ref './foo/bar'\n invalid_ref 'foo/./bar'\n@@ -60,12 +66,15 @@ invalid_ref \"$ref\"\n valid_ref \"$ref\" --allow-onelevel\n invalid_ref \"$ref\" --refspec-pattern\n valid_ref \"$ref\" '--refspec-pattern --allow-onelevel'\n+invalid_ref \"$ref\" --normalize\n+valid_ref \"$ref\" '--allow-onelevel --normalize'\n \n ref='foo/bar'\n valid_ref \"$ref\"\n valid_ref \"$ref\" --allow-onelevel\n valid_ref \"$ref\" --refspec-pattern\n valid_ref \"$ref\" '--refspec-pattern --allow-onelevel'\n+valid_ref \"$ref\" --normalize\n \n ref='foo/*'\n invalid_ref \"$ref\"\n@@ -78,6 +87,8 @@ invalid_ref \"$ref\"\n invalid_ref \"$ref\" --allow-onelevel\n valid_ref \"$ref\" --refspec-pattern\n valid_ref \"$ref\" '--refspec-pattern --allow-onelevel'\n+invalid_ref \"$ref\" --normalize\n+valid_ref \"$ref\" '--refspec-pattern --normalize'\n \n ref='foo/*/bar'\n invalid_ref \"$ref\"\n@@ -105,9 +116,13 @@ invalid_ref \"$ref\" '--refspec-pattern --allow-onelevel'\n \n ref='/foo'\n invalid_ref \"$ref\"\n-valid_ref \"$ref\" --allow-onelevel\n+invalid_ref \"$ref\" --allow-onelevel\n invalid_ref \"$ref\" --refspec-pattern\n-valid_ref \"$ref\" '--refspec-pattern --allow-onelevel'\n+invalid_ref \"$ref\" '--refspec-pattern --allow-onelevel'\n+invalid_ref \"$ref\" --normalize\n+valid_ref \"$ref\" '--allow-onelevel --normalize'\n+invalid_ref \"$ref\" '--refspec-pattern --normalize'\n+valid_ref \"$ref\" '--refspec-pattern --allow-onelevel --normalize'\n \n test_expect_success \"check-ref-format --branch @{-1}\" '\n \tT=$(git write-tree) &&\n@@ -141,12 +156,12 @@ test_expect_success 'check-ref-format --branch from subdir' '\n \n valid_ref_normalized() {\n \ttest_expect_success \"ref name '$1' simplifies to '$2'\" \"\n-\t\trefname=\\$(git check-ref-format --print '$1') &&\n+\t\trefname=\\$(git check-ref-format --normalize '$1') &&\n \t\ttest \\\"\\$refname\\\" = '$2'\"\n }\n invalid_ref_normalized() {\n-\ttest_expect_success \"check-ref-format --print rejects '$1'\" \"\n-\t\ttest_must_fail git check-ref-format --print '$1'\"\n+\ttest_expect_success \"check-ref-format --normalize rejects '$1'\" \"\n+\t\ttest_must_fail git check-ref-format --normalize '$1'\"\n }\n \n valid_ref_normalized 'heads/foo' 'heads/foo'\n-- \n1.7.6.8.gd2879\n"},{"id":"175622","messageId":"1316121043-29367-11-git-send-email-mhagger@alum.mit.edu","threadId":"28403","inReplyTo":"1316121043-29367-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v3 10/22] resolve_ref(): explicitly fail if a symlink is not readable","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2011-09-15T21:10:31Z","receivedAt":"2011-09-15T21:10:31Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Previously the failure came later, after a few steps in which the\nlength was treated like the actual length of a string. Even though\nthe old code gave the same answers, it was somewhat misleading.\n\nSigned-off-by: Michael Haggerty \n---\n refs.c | 2 ++\n 1 files changed, 2 insertions(+), 0 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex d2aac24..c51fd45 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -518,6 +518,8 @@ const char *resolve_ref(const char *ref, unsigned char *sha1, int reading, int *\n \t\t/* Follow \"normalized\" - ie \"refs/..\" symlinks by hand */\n \t\tif (S_ISLNK(st.st_mode)) {\n \t\t\tlen = readlink(path, buffer, sizeof(buffer)-1);\n+\t\t\tif (len < 0)\n+\t\t\t\treturn NULL;\n \t\t\tif (len >= 5 && !memcmp(\"refs/\", buffer, 5)) {\n \t\t\t\tbuffer[len] = 0;\n \t\t\t\tstrcpy(ref_buffer, buffer);\n-- \n1.7.6.8.gd2879\n"},{"id":"175625","messageId":"1316121043-29367-12-git-send-email-mhagger@alum.mit.edu","threadId":"28403","inReplyTo":"1316121043-29367-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v3 11/22] resolve_ref(): use prefixcmp()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2011-09-15T21:10:32Z","receivedAt":"2011-09-15T21:10:32Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Terminate the link content string one step earlier, allowing\nprefixcmp() to be used instead of the less clear memcmp().\n\nSigned-off-by: Michael Haggerty \n---\n refs.c | 4 ++--\n 1 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex c51fd45..da9737f 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -520,8 +520,8 @@ const char *resolve_ref(const char *ref, unsigned char *sha1, int reading, int *\n \t\t\tlen = readlink(path, buffer, sizeof(buffer)-1);\n \t\t\tif (len < 0)\n \t\t\t\treturn NULL;\n-\t\t\tif (len >= 5 && !memcmp(\"refs/\", buffer, 5)) {\n-\t\t\t\tbuffer[len] = 0;\n+\t\t\tbuffer[len] = 0;\n+\t\t\tif (!prefixcmp(buffer, \"refs/\")) {\n \t\t\t\tstrcpy(ref_buffer, buffer);\n \t\t\t\tref = ref_buffer;\n \t\t\t\tif (flag)\n-- \n1.7.6.8.gd2879\n"},{"id":"175626","messageId":"1316121043-29367-13-git-send-email-mhagger@alum.mit.edu","threadId":"28403","inReplyTo":"1316121043-29367-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v3 12/22] resolve_ref(): only follow a symlink that contains a valid, normalized refname","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2011-09-15T21:10:33Z","receivedAt":"2011-09-15T21:10:33Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"\nSigned-off-by: Michael Haggerty \n---\n refs.c | 3 ++-\n 1 files changed, 2 insertions(+), 1 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex da9737f..8f0b871 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -521,7 +521,8 @@ const char *resolve_ref(const char *ref, unsigned char *sha1, int reading, int *\n \t\t\tif (len < 0)\n \t\t\t\treturn NULL;\n \t\t\tbuffer[len] = 0;\n-\t\t\tif (!prefixcmp(buffer, \"refs/\")) {\n+\t\t\tif (!prefixcmp(buffer, \"refs/\") &&\n+\t\t\t\t\t!check_refname_format(buffer, 0)) {\n \t\t\t\tstrcpy(ref_buffer, buffer);\n \t\t\t\tref = ref_buffer;\n \t\t\t\tif (flag)\n-- \n1.7.6.8.gd2879\n"},{"id":"175628","messageId":"1316121043-29367-14-git-send-email-mhagger@alum.mit.edu","threadId":"28403","inReplyTo":"1316121043-29367-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v3 13/22] resolve_ref(): turn buffer into a proper string as soon as possible","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2011-09-15T21:10:34Z","receivedAt":"2011-09-15T21:10:34Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Immediately strip off trailing spaces and null-terminate the string\nholding the contents of the reference file; this allows the use of\nstring functions and avoids the need to keep separate track of the\nstring's length. (get_sha1_hex() fails automatically if the string is\ntoo short.)\n\nSigned-off-by: Michael Haggerty \n---\n refs.c | 20 ++++++++++----------\n 1 files changed, 10 insertions(+), 10 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 8f0b871..79ab0eb 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -546,25 +546,25 @@ const char *resolve_ref(const char *ref, unsigned char *sha1, int reading, int *\n \t\t\treturn NULL;\n \t\tlen = read_in_full(fd, buffer, sizeof(buffer)-1);\n \t\tclose(fd);\n+\t\tif (len < 0)\n+\t\t\treturn NULL;\n+\t\twhile (len && isspace(buffer[len-1]))\n+\t\t\tlen--;\n+\t\tbuffer[len] = '\\0';\n \n \t\t/*\n \t\t * Is it a symbolic ref?\n \t\t */\n-\t\tif (len < 4 || memcmp(\"ref:\", buffer, 4))\n+\t\tif (prefixcmp(buffer, \"ref:\"))\n \t\t\tbreak;\n \t\tbuf = buffer + 4;\n-\t\tlen -= 4;\n-\t\twhile (len && isspace(*buf))\n-\t\t\tbuf++, len--;\n-\t\twhile (len && isspace(buf[len-1]))\n-\t\t\tlen--;\n-\t\tbuf[len] = 0;\n-\t\tmemcpy(ref_buffer, buf, len + 1);\n-\t\tref = ref_buffer;\n+\t\twhile (isspace(*buf))\n+\t\t\tbuf++;\n+\t\tref = strcpy(ref_buffer, buf);\n \t\tif (flag)\n \t\t\t*flag |= REF_ISSYMREF;\n \t}\n-\tif (len < 40 || get_sha1_hex(buffer, sha1))\n+\tif (get_sha1_hex(buffer, sha1))\n \t\treturn NULL;\n \treturn ref;\n }\n-- \n1.7.6.8.gd2879\n"},{"id":"175624","messageId":"1316121043-29367-15-git-send-email-mhagger@alum.mit.edu","threadId":"28403","inReplyTo":"1316121043-29367-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v3 14/22] resolve_ref(): extract a function get_packed_ref()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2011-09-15T21:10:35Z","receivedAt":"2011-09-15T21:10:35Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Making it a function and giving it a name makes the code clearer. I\nalso have a strong suspicion that the function will find other uses in\nthe future.\n\nSigned-off-by: Michael Haggerty \n---\n refs.c | 47 ++++++++++++++++++++++++++++++++++-------------\n 1 files changed, 34 insertions(+), 13 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 79ab0eb..473f7f6 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -466,6 +466,23 @@ int resolve_gitlink_ref(const char *path, const char *refname, unsigned char *re\n }\n \n /*\n+ * Try to read ref from the packed references. On success, set sha1\n+ * and return 0; otherwise, return -1.\n+ */\n+static int get_packed_ref(const char *ref, unsigned char *sha1)\n+{\n+\tstruct ref_list *list = get_packed_refs(NULL);\n+\twhile (list) {\n+\t\tif (!strcmp(ref, list->name)) {\n+\t\t\thashcpy(sha1, list->sha1);\n+\t\t\treturn 0;\n+\t\t}\n+\t\tlist = list->next;\n+\t}\n+\treturn -1;\n+}\n+\n+/*\n * If the \"reading\" argument is set, this function finds out what _object_\n * the ref points at by \"reading\" the ref. The ref, if it is not symbolic,\n * has to exist, and if it is symbolic, it has to point at an existing ref,\n@@ -497,22 +514,26 @@ const char *resolve_ref(const char *ref, unsigned char *sha1, int reading, int *\n \t\t\treturn NULL;\n \n \t\tgit_snpath(path, sizeof(path), \"%s\", ref);\n-\t\t/* Special case: non-existing file. */\n+\n \t\tif (lstat(path, &st) < 0) {\n-\t\t\tstruct ref_list *list = get_packed_refs(NULL);\n-\t\t\twhile (list) {\n-\t\t\t\tif (!strcmp(ref, list->name)) {\n-\t\t\t\t\thashcpy(sha1, list->sha1);\n-\t\t\t\t\tif (flag)\n-\t\t\t\t\t\t*flag |= REF_ISPACKED;\n-\t\t\t\t\treturn ref;\n-\t\t\t\t}\n-\t\t\t\tlist = list->next;\n+\t\t\tif (errno != ENOENT)\n+\t\t\t\treturn NULL;\n+\t\t\t/*\n+\t\t\t * The loose reference file does not exist;\n+\t\t\t * check for a packed reference.\n+\t\t\t */\n+\t\t\tif (!get_packed_ref(ref, sha1)) {\n+\t\t\t\tif (flag)\n+\t\t\t\t\t*flag |= REF_ISPACKED;\n+\t\t\t\treturn ref;\n \t\t\t}\n-\t\t\tif (reading || errno != ENOENT)\n+\t\t\t/* The reference is not a packed reference, either. */\n+\t\t\tif (reading) {\n \t\t\t\treturn NULL;\n-\t\t\thashclr(sha1);\n-\t\t\treturn ref;\n+\t\t\t} else {\n+\t\t\t\thashclr(sha1);\n+\t\t\t\treturn ref;\n+\t\t\t}\n \t\t}\n \n \t\t/* Follow \"normalized\" - ie \"refs/..\" symlinks by hand */\n-- \n1.7.6.8.gd2879\n"},{"id":"175627","messageId":"1316121043-29367-16-git-send-email-mhagger@alum.mit.edu","threadId":"28403","inReplyTo":"1316121043-29367-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v3 15/22] resolve_ref(): do not follow incorrectly-formatted symbolic refs","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2011-09-15T21:10:36Z","receivedAt":"2011-09-15T21:10:36Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Emit a warning and fail if a symbolic reference refers to an\nincorrectly-formatted refname.\n\nSigned-off-by: Michael Haggerty \n---\n refs.c | 5 +++++\n 1 files changed, 5 insertions(+), 0 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 473f7f6..b055501 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -581,6 +581,11 @@ const char *resolve_ref(const char *ref, unsigned char *sha1, int reading, int *\n \t\tbuf = buffer + 4;\n \t\twhile (isspace(*buf))\n \t\t\tbuf++;\n+\t\tif (check_refname_format(buf, REFNAME_ALLOW_ONELEVEL)) {\n+\t\t\twarning(\"symbolic reference in %s is formatted incorrectly\",\n+\t\t\t\tpath);\n+\t\t\treturn NULL;\n+\t\t}\n \t\tref = strcpy(ref_buffer, buf);\n \t\tif (flag)\n \t\t\t*flag |= REF_ISSYMREF;\n-- \n1.7.6.8.gd2879\n"},{"id":"175623","messageId":"1316121043-29367-17-git-send-email-mhagger@alum.mit.edu","threadId":"28403","inReplyTo":"1316121043-29367-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v3 16/22] remote: use xstrdup() instead of strdup()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2011-09-15T21:10:37Z","receivedAt":"2011-09-15T21:10:37Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"\nSigned-off-by: Michael Haggerty \n---\n remote.c | 2 +-\n transport-helper.c | 4 ++--\n 2 files changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/remote.c b/remote.c\nindex 6fcf809..e52aa9b 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -815,7 +815,7 @@ char *apply_refspecs(struct refspec *refspecs, int nr_refspec,\n \t\t\t\t\t\t refspec->dst, &ret))\n \t\t\t\treturn ret;\n \t\t} else if (!strcmp(refspec->src, name))\n-\t\t\treturn strdup(refspec->dst);\n+\t\t\treturn xstrdup(refspec->dst);\n \t}\n \treturn NULL;\n }\ndiff --git a/transport-helper.c b/transport-helper.c\nindex 4eab844..0713126 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -183,7 +183,7 @@ static struct child_process *get_helper(struct transport *transport)\n \t\t\tALLOC_GROW(refspecs,\n \t\t\t\t refspec_nr + 1,\n \t\t\t\t refspec_alloc);\n-\t\t\trefspecs[refspec_nr++] = strdup(capname + strlen(\"refspec \"));\n+\t\t\trefspecs[refspec_nr++] = xstrdup(capname + strlen(\"refspec \"));\n \t\t} else if (!strcmp(capname, \"connect\")) {\n \t\t\tdata->connect = 1;\n \t\t} else if (!prefixcmp(capname, \"export-marks \")) {\n@@ -445,7 +445,7 @@ static int fetch_with_import(struct transport *transport,\n \t\tif (data->refspecs)\n \t\t\tprivate = apply_refspecs(data->refspecs, data->refspec_nr, posn->name);\n \t\telse\n-\t\t\tprivate = strdup(posn->name);\n+\t\t\tprivate = xstrdup(posn->name);\n \t\tread_ref(private, posn->old_sha1);\n \t\tfree(private);\n \t}\n-- \n1.7.6.8.gd2879\n"},{"id":"175631","messageId":"1316121043-29367-18-git-send-email-mhagger@alum.mit.edu","threadId":"28403","inReplyTo":"1316121043-29367-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v3 17/22] remote: avoid passing NULL to read_ref()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2011-09-15T21:10:38Z","receivedAt":"2011-09-15T21:10:38Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"read_ref() can (and in test t5800, actually *does*) return NULL.\nDon't pass the NULL along to read_ref(). Coincidentally, this mistake\ndidn't make resolve_ref() blow up, but upcoming changes to\nresolve_ref() will make it less forgiving.\n\nSigned-off-by: Michael Haggerty \n---\n transport-helper.c | 6 ++++--\n 1 files changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/transport-helper.c b/transport-helper.c\nindex 0713126..6f227e2 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -446,8 +446,10 @@ static int fetch_with_import(struct transport *transport,\n \t\t\tprivate = apply_refspecs(data->refspecs, data->refspec_nr, posn->name);\n \t\telse\n \t\t\tprivate = xstrdup(posn->name);\n-\t\tread_ref(private, posn->old_sha1);\n-\t\tfree(private);\n+\t\tif (private) {\n+\t\t\tread_ref(private, posn->old_sha1);\n+\t\t\tfree(private);\n+\t\t}\n \t}\n \tstrbuf_release(&buf);\n \treturn 0;\n-- \n1.7.6.8.gd2879\n"},{"id":"175621","messageId":"1316121043-29367-19-git-send-email-mhagger@alum.mit.edu","threadId":"28403","inReplyTo":"1316121043-29367-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v3 18/22] resolve_ref(): verify that the input refname has the right format","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2011-09-15T21:10:39Z","receivedAt":"2011-09-15T21:10:39Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"\nSigned-off-by: Michael Haggerty \n---\n refs.c | 3 +++\n 1 files changed, 3 insertions(+), 0 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex b055501..ee3e0cc 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -504,6 +504,9 @@ const char *resolve_ref(const char *ref, unsigned char *sha1, int reading, int *\n \tif (flag)\n \t\t*flag = 0;\n \n+\tif (check_refname_format(ref, REFNAME_ALLOW_ONELEVEL))\n+\t\treturn NULL;\n+\n \tfor (;;) {\n \t\tchar path[PATH_MAX];\n \t\tstruct stat st;\n-- \n1.7.6.8.gd2879\n"},{"id":"175629","messageId":"1316121043-29367-20-git-send-email-mhagger@alum.mit.edu","threadId":"28403","inReplyTo":"1316121043-29367-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v3 19/22] resolve_ref(): emit warnings for improperly-formatted references","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2011-09-15T21:10:40Z","receivedAt":"2011-09-15T21:10:40Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"While resolving references, if a reference is found that is in an\nunrecognized format, emit a warning (and then fail, as before).\nWouldn't *you* want to know?\n\nSigned-off-by: Michael Haggerty \n---\n refs.c | 6 ++++--\n 1 files changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex ee3e0cc..2387f4e 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -500,6 +500,7 @@ const char *resolve_ref(const char *ref, unsigned char *sha1, int reading, int *\n \tssize_t len;\n \tchar buffer[256];\n \tstatic char ref_buffer[256];\n+\tchar path[PATH_MAX];\n \n \tif (flag)\n \t\t*flag = 0;\n@@ -508,7 +509,6 @@ const char *resolve_ref(const char *ref, unsigned char *sha1, int reading, int *\n \t\treturn NULL;\n \n \tfor (;;) {\n-\t\tchar path[PATH_MAX];\n \t\tstruct stat st;\n \t\tchar *buf;\n \t\tint fd;\n@@ -593,8 +593,10 @@ const char *resolve_ref(const char *ref, unsigned char *sha1, int reading, int *\n \t\tif (flag)\n \t\t\t*flag |= REF_ISSYMREF;\n \t}\n-\tif (get_sha1_hex(buffer, sha1))\n+\tif (get_sha1_hex(buffer, sha1)) {\n+\t\twarning(\"reference in %s is formatted incorrectly\", path);\n \t\treturn NULL;\n+\t}\n \treturn ref;\n }\n \n-- \n1.7.6.8.gd2879\n"},{"id":"175630","messageId":"1316121043-29367-21-git-send-email-mhagger@alum.mit.edu","threadId":"28403","inReplyTo":"1316121043-29367-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v3 20/22] resolve_ref(): also treat a too-long SHA1 as invalid","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2011-09-15T21:10:41Z","receivedAt":"2011-09-15T21:10:41Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"If the SHA1 in a reference file is not terminated by a space or\nend-of-file, consider it malformed and emit a warning.\n\nSigned-off-by: Michael Haggerty \n---\n refs.c | 3 ++-\n 1 files changed, 2 insertions(+), 1 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 2387f4e..0baa500 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -593,7 +593,8 @@ const char *resolve_ref(const char *ref, unsigned char *sha1, int reading, int *\n \t\tif (flag)\n \t\t\t*flag |= REF_ISSYMREF;\n \t}\n-\tif (get_sha1_hex(buffer, sha1)) {\n+\t/* Please note that FETCH_HEAD has a second line containing other data. */\n+\tif (get_sha1_hex(buffer, sha1) || (buffer[40] != '\\0' && !isspace(buffer[40]))) {\n \t\twarning(\"reference in %s is formatted incorrectly\", path);\n \t\treturn NULL;\n \t}\n-- \n1.7.6.8.gd2879\n"},{"id":"175620","messageId":"1316121043-29367-22-git-send-email-mhagger@alum.mit.edu","threadId":"28403","inReplyTo":"1316121043-29367-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v3 21/22] resolve_ref(): expand documentation","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2011-09-15T21:10:42Z","receivedAt":"2011-09-15T21:10:42Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Record information about resolve_ref(), hard-won via reverse\nengineering, in a comment for future spelunkers.\n\nSigned-off-by: Michael Haggerty \n---\n cache.h | 34 +++++++++++++++++++++++++++++++++-\n refs.c | 12 ------------\n 2 files changed, 33 insertions(+), 13 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 607c2ea..aea8685 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -822,7 +822,39 @@ static inline int get_sha1_with_context(const char *str, unsigned char *sha1, st\n extern int get_sha1_hex(const char *hex, unsigned char *sha1);\n extern char *sha1_to_hex(const unsigned char *sha1);\t/* static buffer result! */\n extern int read_ref(const char *filename, unsigned char *sha1);\n-extern const char *resolve_ref(const char *path, unsigned char *sha1, int, int *);\n+\n+/*\n+ * Resolve a reference, recursively following symbolic refererences.\n+ *\n+ * Store the referred-to object's name in sha1 and return the name of\n+ * the non-symbolic reference that ultimately pointed at it. The\n+ * return value, if not NULL, is a pointer into either a static buffer\n+ * or the input ref.\n+ *\n+ * If the reference cannot be resolved to an object, the behavior\n+ * depends on the \"reading\" argument:\n+ *\n+ * - If reading is set, return NULL.\n+ *\n+ * - If reading is not set, clear sha1 and return the name of the last\n+ * reference name in the chain, which will either be a non-symbolic\n+ * reference or an undefined reference. If this is a prelude to\n+ * \"writing\" to the ref, the return value is the name of the ref\n+ * that will actually be created or changed.\n+ *\n+ * If flag is non-NULL, set the value that it points to the\n+ * combination of REF_ISPACKED (if the reference was found among the\n+ * packed references) and REF_ISSYMREF (if the initial reference was a\n+ * symbolic reference).\n+ *\n+ * If ref is not a properly-formatted, normalized reference, return\n+ * NULL. If more than MAXDEPTH recursive symbolic lookups are needed,\n+ * give up and return NULL.\n+ *\n+ * errno is sometimes set on errors, but not always.\n+ */\n+extern const char *resolve_ref(const char *ref, unsigned char *sha1, int reading, int *flag);\n+\n extern int dwim_ref(const char *str, int len, unsigned char *sha1, char **ref);\n extern int dwim_log(const char *str, int len, unsigned char *sha1, char **ref);\n extern int interpret_branch_name(const char *str, struct strbuf *);\ndiff --git a/refs.c b/refs.c\nindex 0baa500..096b42c 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -482,18 +482,6 @@ static int get_packed_ref(const char *ref, unsigned char *sha1)\n \treturn -1;\n }\n \n-/*\n- * If the \"reading\" argument is set, this function finds out what _object_\n- * the ref points at by \"reading\" the ref. The ref, if it is not symbolic,\n- * has to exist, and if it is symbolic, it has to point at an existing ref,\n- * because the \"read\" goes through the symref to the ref it points at.\n- *\n- * The access that is not \"reading\" may often be \"writing\", but does not\n- * have to; it can be merely checking _where it leads to_. If it is a\n- * prelude to \"writing\" to the ref, a write to a symref that points at\n- * yet-to-be-born ref will create the real ref pointed by the symref.\n- * reading=0 allows the caller to check where such a symref leads to.\n- */\n const char *resolve_ref(const char *ref, unsigned char *sha1, int reading, int *flag)\n {\n \tint depth = MAXDEPTH;\n-- \n1.7.6.8.gd2879\n"},{"id":"175619","messageId":"1316121043-29367-23-git-send-email-mhagger@alum.mit.edu","threadId":"28403","inReplyTo":"1316121043-29367-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v3 22/22] add_ref(): verify that the refname is formatted correctly","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2011-09-15T21:10:43Z","receivedAt":"2011-09-15T21:10:43Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"In add_ref(), verify that the refname is formatted correctly before\nadding it to the ref_list. Here we have to allow refname components\nthat start with \".\", since (for example) the remote protocol uses\nsynthetic reference name \".have\". So add a new REFNAME_DOT_COMPONENT\nflag that can be passed to check_refname_format() to allow leading\ndots.\n\nSigned-off-by: Michael Haggerty \n---\n refs.c | 18 ++++++++++++++----\n refs.h | 6 +++++-\n 2 files changed, 19 insertions(+), 5 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 096b42c..832a52f 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -56,6 +56,8 @@ static struct ref_list *add_ref(const char *name, const unsigned char *sha1,\n \tentry = xmalloc(sizeof(struct ref_list) + len);\n \thashcpy(entry->sha1, sha1);\n \thashclr(entry->peeled);\n+\tif (check_refname_format(name, REFNAME_ALLOW_ONELEVEL|REFNAME_DOT_COMPONENT))\n+\t\tdie(\"Reference has invalid format: '%s'\", name);\n \tmemcpy(entry->name, name, len);\n \tentry->flag = flag;\n \tentry->next = list;\n@@ -900,7 +902,7 @@ static inline int bad_ref_char(int ch)\n * the length of the component found, or -1 if the component is not\n * legal.\n */\n-static int check_refname_component(const char *ref)\n+static int check_refname_component(const char *ref, int flags)\n {\n \tconst char *cp;\n \tchar last = '\\0';\n@@ -919,8 +921,16 @@ static int check_refname_component(const char *ref)\n \t}\n \tif (cp == ref)\n \t\treturn -1; /* Component has zero length. */\n-\tif (ref[0] == '.')\n-\t\treturn -1; /* Component starts with '.'. */\n+\tif (ref[0] == '.') {\n+\t\tif (!(flags & REFNAME_DOT_COMPONENT))\n+\t\t\treturn -1; /* Component starts with '.'. */\n+\t\t/*\n+\t\t * Even if leading dots are allowed, don't allow \".\"\n+\t\t * as a component (\"..\" is prevented by a rule above).\n+\t\t */\n+\t\tif (ref[1] == '\\0')\n+\t\t\treturn -1; /* Component equals \".\". */\n+\t}\n \tif (cp - ref >= 5 && !memcmp(cp - 5, \".lock\", 5))\n \t\treturn -1; /* Refname ends with \".lock\". */\n \treturn cp - ref;\n@@ -932,7 +942,7 @@ int check_refname_format(const char *ref, int flags)\n \n \twhile (1) {\n \t\t/* We are at the start of a path component. */\n-\t\tcomponent_len = check_refname_component(ref);\n+\t\tcomponent_len = check_refname_component(ref, flags);\n \t\tif (component_len < 0) {\n \t\t\tif ((flags & REFNAME_REFSPEC_PATTERN) &&\n \t\t\t\t\tref[0] == '*' &&\ndiff --git a/refs.h b/refs.h\nindex b0da5fc..d5ac133 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -99,6 +99,7 @@ extern int for_each_reflog(each_ref_fn, void *);\n \n #define REFNAME_ALLOW_ONELEVEL 1\n #define REFNAME_REFSPEC_PATTERN 2\n+#define REFNAME_DOT_COMPONENT 4\n \n /*\n * Return 0 iff ref has the correct format for a refname according to\n@@ -106,7 +107,10 @@ extern int for_each_reflog(each_ref_fn, void *);\n * REFNAME_ALLOW_ONELEVEL is set in flags, then accept one-level\n * reference names. If REFNAME_REFSPEC_PATTERN is set in flags, then\n * allow a \"*\" wildcard character in place of one of the name\n- * components. No leading or repeated slashes are accepted.\n+ * components. No leading or repeated slashes are accepted. If\n+ * REFNAME_DOT_COMPONENT is set in flags, then allow refname\n+ * components to start with \".\" (but not a whole component equal to\n+ * \".\" or \"..\").\n */\n extern int check_refname_format(const char *ref, int flags);\n \n-- \n1.7.6.8.gd2879\n"},{"id":"176048","messageId":"201109231017.55996.trast@student.ethz.ch","threadId":"28403","inReplyTo":"1316121043-29367-14-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH v3 13/22] resolve_ref(): turn buffer into a proper string as soon as possible","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2011-09-23T08:17:55Z","receivedAt":"2011-09-23T08:17:55Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Hi Michael\n\nMichael Haggerty wrote:\n> Immediately strip off trailing spaces and null-terminate the string\n> holding the contents of the reference file; this allows the use of\n> string functions and avoids the need to keep separate track of the\n> string's length. (get_sha1_hex() fails automatically if the string is\n> too short.)\n> \n> Signed-off-by: Michael Haggerty \n\nI'm getting valgrind failures in t1450-fsck and t3800-mktag which\nblame to this commit. For t1450 it looks as follows:\n\n ok 5 - object with bad sha1\n\n expecting success: \n git rev-parse HEAD^{tree} >.git/refs/heads/invalid &&\n test_when_finished \"git update-ref -d refs/heads/invalid\" &&\n git fsck 2>out &&\n cat out &&\n grep \"not a commit\" out\n\n ==19623== Use of uninitialised value of size 8\n ==19623== at 0x4B6747: hexval (cache.h:798)\n ==19623== by 0x4B6797: get_sha1_hex (hex.c:42)\n ==19623== by 0x4DD12A: resolve_ref (refs.c:588)\n ==19623== by 0x4DC777: get_ref_dir (refs.c:313)\n ==19623== by 0x4DC6FA: get_ref_dir (refs.c:302)\n ==19623== by 0x4DC963: get_loose_refs (refs.c:368)\n ==19623== by 0x4DD556: do_for_each_ref (refs.c:687)\n ==19623== by 0x4DDA05: for_each_replace_ref (refs.c:806)\n ==19623== by 0x4E5CE9: prepare_replace_object (replace_object.c:86)\n ==19623== by 0x4E5D3C: do_lookup_replace_object (replace_object.c:103)\n ==19623== by 0x4C99BB: lookup_replace_object (cache.h:764)\n ==19623== by 0x4C9FA6: parse_object (object.c:191)\n ==19623== Uninitialised value was created by a stack allocation\n ==19623== at 0x4DCE34: resolve_ref (refs.c:498)\n\nor when I run it at the tip of pu instead of at the commit itself,\nline numbers are like so:\n\n ==2308== Use of uninitialised value of size 8\n ==2308== at 0x4ADB8B: get_sha1_hex (cache.h:800)\n ==2308== by 0x4D4283: resolve_ref (refs.c:629)\n ==2308== by 0x4D4851: get_ref_dir (refs.c:361)\n ==2308== by 0x4D48C6: get_ref_dir (refs.c:350)\n ==2308== by 0x4D4D29: do_for_each_ref (refs.c:412)\n ==2308== by 0x4DCD93: do_lookup_replace_object (replace_object.c:86)\n ==2308== by 0x4C31F4: parse_object (cache.h:764)\n ==2308== by 0x4F2A1D: get_sha1_1 (sha1_name.c:567)\n ==2308== by 0x4F2D5F: get_sha1_with_context_1 (sha1_name.c:1117)\n ==2308== by 0x4F3543: get_sha1 (cache.h:822)\n ==2308== by 0x461B50: cmd_rev_parse (rev-parse.c:723)\n ==2308== by 0x404B71: run_builtin (git.c:308)\n ==2308== Uninitialised value was created by a stack allocation\n ==2308== at 0x4D4006: resolve_ref (refs.c:530)\n\nCan you look into this?\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"},{"id":"176061","messageId":"4E7C857D.8000304@alum.mit.edu","threadId":"28403","inReplyTo":"201109231017.55996.trast@student.ethz.ch","subject":"Re: [PATCH v3 13/22] resolve_ref(): turn buffer into a proper string as soon as possible","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2011-09-23T13:11:25Z","receivedAt":"2011-09-23T13:11:25Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 09/23/2011 10:17 AM, Thomas Rast wrote:\n> Michael Haggerty wrote:\n>> Immediately strip off trailing spaces and null-terminate the string\n>> holding the contents of the reference file; this allows the use of\n>> string functions and avoids the need to keep separate track of the\n>> string's length. (get_sha1_hex() fails automatically if the string is\n>> too short.)\n>>\n>> Signed-off-by: Michael Haggerty \n> \n> I'm getting valgrind failures in t1450-fsck and t3800-mktag which\n> blame to this commit. For t1450 it looks as follows:\n> \n> ok 5 - object with bad sha1\n> \n> expecting success: \n> git rev-parse HEAD^{tree} >.git/refs/heads/invalid &&\n> test_when_finished \"git update-ref -d refs/heads/invalid\" &&\n> git fsck 2>out &&\n> cat out &&\n> grep \"not a commit\" out\n> \n> ==19623== Use of uninitialised value of size 8\n> ==19623== at 0x4B6747: hexval (cache.h:798)\n> ==19623== by 0x4B6797: get_sha1_hex (hex.c:42)\n> ==19623== by 0x4DD12A: resolve_ref (refs.c:588)\n\nThanks for finding this.\n\nThe problem comes from passing get_sha1_hex() an arbitrary\nNUL-terminated string. get_sha1_hex() calls hexval(), which returns -1\nif it finds a NUL character, and therefore (correctly) rejects any\nstring that is less than 40 characters. The problem is that\nget_sha1_hex() reads characters pairwise, and even if the first\ncharacter in a pair is NUL, the second character is read anyway.\nTherefore, for strings whose strlen is an even number less than 40,\nget_sha1_hex() reads past the terminating NUL.\n\nI don't know whether get_sha1_hex() is *supposed* to work for arbitrary\nNUL-terminated strings because it is not documented. But other callers\nseem to make the same assumption that I did, so I think that I will fix\nget_sha1_hex() to not read past a NUL value (albeit at the cost of an\nextra check in each iteration of the loop).\n\nThe fix is trivial and will be sent shortly.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"176065","messageId":"1316785116-21831-1-git-send-email-mhagger@alum.mit.edu","threadId":"28403","inReplyTo":"4E7C857D.8000304@alum.mit.edu","subject":"[PATCH 1/1] get_sha1_hex(): do not read past a NUL character","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2011-09-23T13:38:36Z","receivedAt":"2011-09-23T13:38:36Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Previously, get_sha1_hex() would read one character past the end of a\nnull-terminated string whose strlen was an even number less than 40.\nAlthough the function correctly returned -1 in these cases, the extra\nmemory access might have been to uninitialized (or even, conceivably,\nunallocated) memory.\n\nAdd a check to avoid reading past the end of a string.\n\nThis problem was discovered by Thomas Rast \nusing valgrind.\n\nSigned-off-by: Michael Haggerty \n---\nIt is suggested to apply this bugfix before mh/check-ref-format-3;\notherwise the latter triggers the bug, leading to a valgrind error.\n\nThis patch could optionally be applied to maint (to which it can be\nrebased cleanly), though the bug that it fixes is relatively benign.\n\n cache.h | 9 +++++++++\n hex.c | 10 +++++++++-\n 2 files changed, 18 insertions(+), 1 deletions(-)\n\ndiff --git cache.h cache.h\nindex 607c2ea..e7bbc0d 100644\n--- cache.h\n+++ cache.h\n@@ -819,7 +819,16 @@ static inline int get_sha1_with_context(const char *str, unsigned char *sha1, st\n {\n \treturn get_sha1_with_context_1(str, sha1, orc, 0, NULL);\n }\n+\n+/*\n+ * Try to read a SHA1 in hexadecimal format from the 40 characters\n+ * starting at hex. Write the 20-byte result to sha1 in binary form.\n+ * Return 0 on success. Reading stops if a NUL is encountered in the\n+ * input, so it is safe to pass this function an arbitrary\n+ * null-terminated string.\n+ */\n extern int get_sha1_hex(const char *hex, unsigned char *sha1);\n+\n extern char *sha1_to_hex(const unsigned char *sha1);\t/* static buffer result! */\n extern int read_ref(const char *filename, unsigned char *sha1);\n extern const char *resolve_ref(const char *path, unsigned char *sha1, int, int *);\ndiff --git hex.c hex.c\nindex bb402fb..9ebc050 100644\n--- hex.c\n+++ hex.c\n@@ -39,7 +39,15 @@ int get_sha1_hex(const char *hex, unsigned char *sha1)\n {\n \tint i;\n \tfor (i = 0; i < 20; i++) {\n-\t\tunsigned int val = (hexval(hex[0]) << 4) | hexval(hex[1]);\n+\t\tunsigned int val;\n+\t\t/*\n+\t\t * hex[1]=='\\0' is caught when val is checked below,\n+\t\t * but if hex[0] is NUL we have to avoid reading\n+\t\t * past the end of the string:\n+\t\t */\n+\t\tif (!hex[0])\n+\t\t\treturn -1;\n+\t\tval = (hexval(hex[0]) << 4) | hexval(hex[1]);\n \t\tif (val & ~0xff)\n \t\t\treturn -1;\n \t\t*sha1++ = val;\n-- \n1.7.7.rc2\n"},{"id":"176088","messageId":"7vaa9vulsw.fsf@alter.siamese.dyndns.org","threadId":"28403","inReplyTo":"1316785116-21831-1-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 1/1] get_sha1_hex(): do not read past a NUL character","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-09-23T18:59:43Z","receivedAt":"2011-09-23T18:59:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> Previously, get_sha1_hex() would read one character past the end of a\n> null-terminated string whose strlen was an even number less than 40.\n> Although the function correctly returned -1 in these cases, the extra\n> memory access might have been to uninitialized (or even, conceivably,\n> unallocated) memory.\n>\n> Add a check to avoid reading past the end of a string.\n\nMakes sense; thanks.\n"},{"id":"176953","messageId":"201110052111.52701.trast@student.ethz.ch","threadId":"28403","inReplyTo":"7vaa9vulsw.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/1] get_sha1_hex(): do not read past a NUL character","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2011-10-05T19:11:52Z","receivedAt":"2011-10-05T19:11:52Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Junio C Hamano wrote:\n> Michael Haggerty writes:\n> \n> > Previously, get_sha1_hex() would read one character past the end of a\n> > null-terminated string whose strlen was an even number less than 40.\n> > Although the function correctly returned -1 in these cases, the extra\n> > memory access might have been to uninitialized (or even, conceivably,\n> > unallocated) memory.\n> >\n> > Add a check to avoid reading past the end of a string.\n> \n> Makes sense; thanks.\n\nHas this fixed patch ever made it to pu? I'm still seeing the same\nbreakage in the automated valgrind runs.\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"},{"id":"176958","messageId":"7v39f76up7.fsf@alter.siamese.dyndns.org","threadId":"28403","inReplyTo":"201110052111.52701.trast@student.ethz.ch","subject":"Re: [PATCH 1/1] get_sha1_hex(): do not read past a NUL character","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-10-05T20:37:40Z","receivedAt":"2011-10-05T20:37:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Rast writes:\n\n> Junio C Hamano wrote:\n>> Michael Haggerty writes:\n>> \n>> > Previously, get_sha1_hex() would read one character past the end of a\n>> > null-terminated string whose strlen was an even number less than 40.\n>> > Although the function correctly returned -1 in these cases, the extra\n>> > memory access might have been to uninitialized (or even, conceivably,\n>> > unallocated) memory.\n>> >\n>> > Add a check to avoid reading past the end of a string.\n>> \n>> Makes sense; thanks.\n>\n> Has this fixed patch ever made it to pu? I'm still seeing the same\n> breakage in the automated valgrind runs.\n\nI do not think so.\n\nI was under the impression that Michael wanted to include this in the\nearly part of a re-roll of check-ref-format series.\n\nI'll do the rebase myself. Thanks.\n"},{"id":"177376","messageId":"20111011161652.GA15629@sigill.intra.peff.net","threadId":"28403","inReplyTo":"1316121043-29367-20-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH v3 19/22] resolve_ref(): emit warnings for improperly-formatted references","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-10-11T16:16:53Z","receivedAt":"2011-10-11T16:16:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 15, 2011 at 11:10:40PM +0200, Michael Haggerty wrote:\n\n> While resolving references, if a reference is found that is in an\n> unrecognized format, emit a warning (and then fail, as before).\n> Wouldn't *you* want to know?\n\nNot necessarily. :)\n\nI noticed this today, and bisected it to this patch:\n\n $ git.v1.7.7 show config\n fatal: ambiguous argument 'config': unknown revision or path not in the working tree.\n Use '--' to separate paths from revisions\n\n $ git.mh.check-ref-format-3 show config\n warning: reference in .git/config is formatted incorrectly\n warning: reference in .git/config is formatted incorrectly\n fatal: ambiguous argument 'config': unknown revision or path not in the working tree.\n Use '--' to separate paths from revisions\n\nwhich is arguably not a big deal, as we eventually report failure\nanyway. But it's even worse with:\n\n $ git branch config v1.7.7\n $ git show --oneline config\n warning: reference in .git/config is formatted incorrectly\n 703f05a Git 1.7.7\n\nAs you probably guessed, this is due to the ref resolution rules first\nlooking directly in .git for refs, which catches things like \"HEAD\" and\nfully-qualified refs like \"refs/heads/foo\".\n\nWhich means we have been considering \".git/config\" as a possible ref for\na long time, which is arguably wrong. Your patch only makes it more\nobvious that this is the case.\n\nI wonder if the right solution is for us to be more picky about what can\nbe found in $GIT_DIR. Maybe matching all-uppercase, or starting with\n\"refs/\", which I think would match existing convention?\n\n-Peff\n"},{"id":"177385","messageId":"7vr52jfm8i.fsf@alter.siamese.dyndns.org","threadId":"28403","inReplyTo":"20111011161652.GA15629@sigill.intra.peff.net","subject":"Re: [PATCH v3 19/22] resolve_ref(): emit warnings for improperly-formatted references","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-10-11T17:53:49Z","receivedAt":"2011-10-11T17:53:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I wonder if the right solution is for us to be more picky about what can\n> be found in $GIT_DIR. Maybe matching all-uppercase, or starting with\n> \"refs/\", which I think would match existing convention?\n\nI think we've discussed tightening it a few years ago already.\n\nHEAD, MERGE_HEAD, FETCH_HEAD, etc. all are \"^[_A-Z]*$\" and it may even be\na good idea to insist \"^[_A-Z]*HEAD$\" or even \"^([A-Z][A-Z]*_)?HEAD$\".\n"},{"id":"177387","messageId":"7vmxd7flkw.fsf@alter.siamese.dyndns.org","threadId":"28403","inReplyTo":"7vr52jfm8i.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v3 19/22] resolve_ref(): emit warnings for improperly-formatted references","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-10-11T18:07:59Z","receivedAt":"2011-10-11T18:07:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Jeff King writes:\n>\n>> I wonder if the right solution is for us to be more picky about what can\n>> be found in $GIT_DIR. Maybe matching all-uppercase, or starting with\n>> \"refs/\", which I think would match existing convention?\n>\n> I think we've discussed tightening it a few years ago already.\n>\n> HEAD, MERGE_HEAD, FETCH_HEAD, etc. all are \"^[_A-Z]*$\" and it may even be\n> a good idea to insist \"^[_A-Z]*HEAD$\" or even \"^([A-Z][A-Z]*_)?HEAD$\".\n\nPerhaps like this? Only compile tested...\n\n sha1_name.c | 21 +++++++++++++++++++++\n 1 files changed, 21 insertions(+), 0 deletions(-)\n\ndiff --git a/sha1_name.c b/sha1_name.c\nindex ba976b4..5effb1a 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -261,6 +261,23 @@ static char *substitute_branch_name(const char **string, int *len)\n \treturn NULL;\n }\n \n+static int is_kind_of_head(const char *str)\n+{\n+\tint len = strlen(str) - 4;\n+\tif (len < 0 || strcmp(str + len, \"HEAD\"))\n+\t\treturn 0;\n+\tif (!len)\n+\t\treturn 1;\n+\tif (str[--len] != '_' || !len)\n+\t\treturn 0;\n+\twhile (len) {\n+\t\tchar ch = str[--len];\n+\t\tif (ch < 'A' || 'Z' < ch)\n+\t\t\treturn 0;\n+\t}\n+\treturn 1;\n+}\n+\n int dwim_ref(const char *str, int len, unsigned char *sha1, char **ref)\n {\n \tchar *last_branch = substitute_branch_name(&str, &len);\n@@ -274,6 +291,8 @@ int dwim_ref(const char *str, int len, unsigned char *sha1, char **ref)\n \t\tunsigned char *this_result;\n \t\tint flag;\n \n+\t\tif (p == ref_rev_parse_rules && !is_kind_of_head(str))\n+\t\t\tcontinue;\n \t\tthis_result = refs_found ? sha1_from_ref : sha1;\n \t\tmksnpath(fullref, sizeof(fullref), *p, len, str);\n \t\tr = resolve_ref(fullref, this_result, 1, &flag);\n@@ -302,6 +321,8 @@ int dwim_log(const char *str, int len, unsigned char *sha1, char **log)\n \t\tchar path[PATH_MAX];\n \t\tconst char *ref, *it;\n \n+\t\tif (p == ref_rev_parse_rules && !is_kind_of_head(str))\n+\t\t\tcontinue;\n \t\tmksnpath(path, sizeof(path), *p, len, str);\n \t\tref = resolve_ref(path, hash, 1, NULL);\n \t\tif (!ref)\n"},{"id":"177394","messageId":"7v39ezffq5.fsf_-_@alter.siamese.dyndns.org","threadId":"28403","inReplyTo":"7vmxd7flkw.fsf@alter.siamese.dyndns.org","subject":"Re* [PATCH v3 19/22] resolve_ref(): emit warnings for improperly-formatted references","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-10-11T20:14:26Z","receivedAt":"2011-10-11T20:14:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n>> I think we've discussed tightening it a few years ago already.\n>>\n>> HEAD, MERGE_HEAD, FETCH_HEAD, etc. all are \"^[_A-Z]*$\" and it may even be\n>> a good idea to insist \"^[_A-Z]*HEAD$\" or even \"^([A-Z][A-Z]*_)?HEAD$\".\n>\n> Perhaps like this? Only compile tested...\n\nNot quite. There are at least three bugs in the patch.\n\n - Some subsystems use random refnames like NOTES_MERGE_PARTIAL that would\n not match \"^([A-Z][A-Z]*_)?HEAD$\". The rule needs to be relaxed;\n\n - dwim_ref() can be fed \"refs/heads/master\" and is expected to dwim it to\n the master branch.\n\n - These codepaths get pointer+length so that it can be told to parse only\n the first 4 bytes in \"HEAD:$path\".\n\n-- >8 --\nSubject: [PATCH] Restrict ref-like names immediately below $GIT_DIR\n\nWe have always dwimmed the user input $string into a ref by first looking\ndirectly inside $GIT_DIR, and then in $GIT_DIR/refs, $GIT_DIR/refs/tags,\netc., and that is what made\n\n\tgit log HEAD..MERGE_HEAD\n\nwork correctly. This however means that\n\n\tgit rev-parse config\n git log index\n\nwould look at $GIT_DIR/config and $GIT_DIR/index and see if they are valid\nrefs.\n\nTo reduce confusion, let's not dwim a path immediately below $GIT_DIR that\nis not all-caps.\n\nSigned-off-by: Junio C Hamano \n---\n sha1_name.c | 23 +++++++++++++++++++++++\n 1 files changed, 23 insertions(+), 0 deletions(-)\n\ndiff --git a/sha1_name.c b/sha1_name.c\nindex 143fd97..5eb19c2 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -261,6 +261,25 @@ static char *substitute_branch_name(const char **string, int *len)\n \treturn NULL;\n }\n \n+static int ok_at_root_level(const char *str, int len)\n+{\n+\tint seen_non_root_char = 0;\n+\n+\twhile (len--) {\n+\t\tchar ch = *str++;\n+\n+\t\tif (ch == '/')\n+\t\t\treturn 1;\n+\t\t/*\n+\t\t * Only accept likes of .git/HEAD, .git/MERGE_HEAD at\n+\t\t * the root level as a ref.\n+\t\t */\n+\t\tif (ch != '_' && (ch < 'A' || 'Z' < ch))\n+\t\t\tseen_non_root_char = 1;\n+\t}\n+\treturn !seen_non_root_char;\n+}\n+\n int dwim_ref(const char *str, int len, unsigned char *sha1, char **ref)\n {\n \tchar *last_branch = substitute_branch_name(&str, &len);\n@@ -274,6 +293,8 @@ int dwim_ref(const char *str, int len, unsigned char *sha1, char **ref)\n \t\tunsigned char *this_result;\n \t\tint flag;\n \n+\t\tif (p == ref_rev_parse_rules && !ok_at_root_level(str, len))\n+\t\t\tcontinue;\n \t\tthis_result = refs_found ? sha1_from_ref : sha1;\n \t\tmksnpath(fullref, sizeof(fullref), *p, len, str);\n \t\tr = resolve_ref(fullref, this_result, 1, &flag);\n@@ -302,6 +323,8 @@ int dwim_log(const char *str, int len, unsigned char *sha1, char **log)\n \t\tchar path[PATH_MAX];\n \t\tconst char *ref, *it;\n \n+\t\tif (p == ref_rev_parse_rules && !ok_at_root_level(str, len))\n+\t\t\tcontinue;\n \t\tmksnpath(path, sizeof(path), *p, len, str);\n \t\tref = resolve_ref(path, hash, 1, NULL);\n \t\tif (!ref)\n"},{"id":"177396","messageId":"20111011203903.GA23069@sigill.intra.peff.net","threadId":"28403","inReplyTo":"7v39ezffq5.fsf_-_@alter.siamese.dyndns.org","subject":"Re: Re* [PATCH v3 19/22] resolve_ref(): emit warnings for improperly-formatted references","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-10-11T20:39:03Z","receivedAt":"2011-10-11T20:39:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Oct 11, 2011 at 01:14:26PM -0700, Junio C Hamano wrote:\n\n> Junio C Hamano writes:\n> \n> >> I think we've discussed tightening it a few years ago already.\n> >>\n> >> HEAD, MERGE_HEAD, FETCH_HEAD, etc. all are \"^[_A-Z]*$\" and it may even be\n> >> a good idea to insist \"^[_A-Z]*HEAD$\" or even \"^([A-Z][A-Z]*_)?HEAD$\".\n> >\n> > Perhaps like this? Only compile tested...\n> \n> Not quite. There are at least three bugs in the patch.\n> \n> - Some subsystems use random refnames like NOTES_MERGE_PARTIAL that would\n> not match \"^([A-Z][A-Z]*_)?HEAD$\". The rule needs to be relaxed;\n\nYeah, I found one of these also. I thought at first we could rename\nthings like NOTES_MERGE_PARTIAL, as it's more about internal\ncommunication within a specific version of git than it is about letting\nan external program peek at our state. But there really are several of\nthem. And I think it makes sense to keep this safety valve conservative,\nand try to document existing practice rather than dictating it. I'm\nworried that some porcelain or other tool is using their own all-caps\nname, and that tightening it too much would be breaking that.\n\nYour relaxed rule does match things like COMMIT_EDITMSG and NOTES_MSG,\nwhich are obviously bogus. At the same time, I'm not sure it's a big\ndeal. The point of this is to restrict the lookup to a class of names\nwhich are likely magical to git, and users should probably avoid the\nmagical namespace (i.e., it's still not a good idea to call your branch\n\"HEAD\"). Something like \"config\" is an easy branch name to unknowingly\nuse. Something like \"COMMIT_EDITMSG\" is not.\n\nYour rule does disallow RENAMED-REF, which is used in branch renaming.\nHowever, I'm having trouble figuring out what it's really for. It's not\nmentioned in the documentation at all, and c976d41, its origin, says\nonly:\n\n This also indroduces $GIT_DIR/RENAME_REF which is a \"metabranch\"\n used when renaming branches. It will always hold the original\n sha1 for the latest renamed branch.\n\n Additionally, if $GIT_DIR/logs/RENAME_REF exists,\n all branch rename events are logged there.\n\nBut in the code, it is spelled RENAMED-REF (with a dash). And as far as\nI can tell, does not actually create a reflog. And it's not documented\nanywhere, so I suspect nobody is using it. Maybe it is worth switching\nthat name.\n\n> - dwim_ref() can be fed \"refs/heads/master\" and is expected to dwim it to\n> the master branch.\n\nIt looks like your code will allow any subdirectory. I had thought to\nlimit it to \"refs/\". Otherwise, my \"config\" example could be\n\"objects/pack\", or \"lost-found/commits\", \"remotes/foo\", or something.\nObviously the longer the name, the smaller the possibility of an\naccidental collision. But I couldn't think of any other subdirectory\ninto which refs should go.\n\n-Peff\n"},{"id":"177401","messageId":"7vpqi3dxkr.fsf@alter.siamese.dyndns.org","threadId":"28403","inReplyTo":"20111011203903.GA23069@sigill.intra.peff.net","subject":"Re: Re* [PATCH v3 19/22] resolve_ref(): emit warnings for improperly-formatted references","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-10-11T21:31:48Z","receivedAt":"2011-10-11T21:31:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> But in the code, it is spelled RENAMED-REF (with a dash). And as far as\n> I can tell, does not actually create a reflog. And it's not documented\n> anywhere, so I suspect nobody is using it. Maybe it is worth switching\n> that name.\n\nOr even better get rid of it?\n\n>> - dwim_ref() can be fed \"refs/heads/master\" and is expected to dwim it to\n>> the master branch.\n>\n> It looks like your code will allow any subdirectory. I had thought to\n> limit it to \"refs/\". Otherwise, my \"config\" example could be\n> \"objects/pack\", or \"lost-found/commits\", \"remotes/foo\", or something.\n> Obviously the longer the name, the smaller the possibility of an\n> accidental collision. But I couldn't think of any other subdirectory\n> into which refs should go.\n\nI wanted to start as loose as possible to avoid negatively impacting\nexisting users, later to tighten. As fsck and friends never look outside\nof refs/, I think the prefix refs/ is a reasonable restriction that is\nsafe.\n"},{"id":"177414","messageId":"20111011225434.GA24142@sigill.intra.peff.net","threadId":"28403","inReplyTo":"7vpqi3dxkr.fsf@alter.siamese.dyndns.org","subject":"Re: Re* [PATCH v3 19/22] resolve_ref(): emit warnings for improperly-formatted references","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-10-11T22:54:34Z","receivedAt":"2011-10-11T22:54:34Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Oct 11, 2011 at 02:31:48PM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > But in the code, it is spelled RENAMED-REF (with a dash). And as far as\n> > I can tell, does not actually create a reflog. And it's not documented\n> > anywhere, so I suspect nobody is using it. Maybe it is worth switching\n> > that name.\n> \n> Or even better get rid of it?\n\nFine by me. I'm not sure anyone is even aware that it exists. Just to\ndouble-check, I grepped the list archives, and the biggest mention of it\nwas its presence causing a weird bug:\n\n http://thread.gmane.org/gmane.comp.version-control.git/143737\n\nGoogling turns up only confusion, nobody actually using it or\nrecommending that it be used.\n\nOTOH, it does actually serialize branch renames, since we take a lock on\nit. Maybe that's important. Cc'ing Lars. Certainly renaming it would be\nthe conservative choice.\n\n-Peff\n\nPS I mentioned above that it does not actually create a reflog. Digging\nin the code more, I think it is capable of it, but my git.git clone had\nRENAMED-REF, but no reflog. I would have thought core.logAllRefUpdates\nwould turn it on, but maybe there is a funny interaction with things not\nin refs/.\n"},{"id":"177416","messageId":"20111011230749.GA29785@sigill.intra.peff.net","threadId":"28403","inReplyTo":"7v39ezffq5.fsf_-_@alter.siamese.dyndns.org","subject":"Re: Re* [PATCH v3 19/22] resolve_ref(): emit warnings for improperly-formatted references","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-10-11T23:07:49Z","receivedAt":"2011-10-11T23:07:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Oct 11, 2011 at 01:14:26PM -0700, Junio C Hamano wrote:\n\n> Junio C Hamano writes:\n> \n> >> I think we've discussed tightening it a few years ago already.\n> >>\n> >> HEAD, MERGE_HEAD, FETCH_HEAD, etc. all are \"^[_A-Z]*$\" and it may even be\n> >> a good idea to insist \"^[_A-Z]*HEAD$\" or even \"^([A-Z][A-Z]*_)?HEAD$\".\n> >\n> > Perhaps like this? Only compile tested...\n> \n> Not quite. There are at least three bugs in the patch.\n> \n> - Some subsystems use random refnames like NOTES_MERGE_PARTIAL that would\n> not match \"^([A-Z][A-Z]*_)?HEAD$\". The rule needs to be relaxed;\n> \n> - dwim_ref() can be fed \"refs/heads/master\" and is expected to dwim it to\n> the master branch.\n> \n> - These codepaths get pointer+length so that it can be told to parse only\n> the first 4 bytes in \"HEAD:$path\".\n\nOne more bug. :)\n\nWe also look at ref_rev_parse_rules in shorten_unambiguous_ref. So even\nwith your patch, I still get the warning with:\n\n $ git branch config\n $ git for-each-ref --format='%(refname:short)' refs/heads/\n\nIt looks like we also use it in remote.c:count_refspec_match, but I\nhaven't figured out if that can trigger a warning or not.\n\n-Peff\n"},{"id":"177420","messageId":"7vehyjcckp.fsf@alter.siamese.dyndns.org","threadId":"28403","inReplyTo":"20111011230749.GA29785@sigill.intra.peff.net","subject":"Re: Re* [PATCH v3 19/22] resolve_ref(): emit warnings for improperly-formatted references","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-10-11T23:50:46Z","receivedAt":"2011-10-11T23:50:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> It looks like we also use it in remote.c:count_refspec_match, but I\n> haven't figured out if that can trigger a warning or not.\n\nIt starts sounding like that the ill-thought-out warning should be ripped\nout regardless of what other things we do.\n"},{"id":"177421","messageId":"20111012021128.GA32149@sigill.intra.peff.net","threadId":"28403","inReplyTo":"7vehyjcckp.fsf@alter.siamese.dyndns.org","subject":"Re: Re* [PATCH v3 19/22] resolve_ref(): emit warnings for improperly-formatted references","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-10-12T02:11:28Z","receivedAt":"2011-10-12T02:11:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Oct 11, 2011 at 04:50:46PM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > It looks like we also use it in remote.c:count_refspec_match, but I\n> > haven't figured out if that can trigger a warning or not.\n> \n> It starts sounding like that the ill-thought-out warning should be ripped\n> out regardless of what other things we do.\n\nMaybe. I think it is not the warning that is wrong, but that it is\nexposing a slightly hack-ish part of git (that we consider things like\n$GIT_DIR/config as possible refs, and just silently reject them because\nthey happen not to have the right format).\n\nOn the other hand, it has been working fine that way for years, so maybe\nit is not worth changing now.\n\nAt any rate, I think the changes should be all or nothing. If the\nwarning goes away, fine. But if the warning stays, and dwim_ref is going\nto have special rules for looking in the top-level $GIT_DIR, then things\nlike shorten_unambiguous_ref need to respect those rules, or we've just\ncreated a new bug.\n\n-Peff\n"},{"id":"177422","messageId":"4E9501EA.8060409@alum.mit.edu","threadId":"28403","inReplyTo":"7vehyjcckp.fsf@alter.siamese.dyndns.org","subject":"Re: Re* [PATCH v3 19/22] resolve_ref(): emit warnings for improperly-formatted references","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2011-10-12T02:56:42Z","receivedAt":"2011-10-12T02:56:42Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 10/12/2011 01:50 AM, Junio C Hamano wrote:\n> It starts sounding like that the ill-thought-out warning should be ripped\n> out regardless of what other things we do.\n\nISTM that the warning is proving its worth already by illuminating some\nquestionable practices (treating any file in $GIT_DIR as a potential\nreference) :-)\n\nHowever, it might be that fixing 100% of the questionable practices is\ntoo much work. In that case, I suggest that the warning not be ripped\nout altogether, but rather that the warning only be emitted for invalid\nreferences whose names start with \"refs/\".\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"177423","messageId":"7v39eyddoc.fsf@alter.siamese.dyndns.org","threadId":"28403","inReplyTo":"20111012021128.GA32149@sigill.intra.peff.net","subject":"Re: Re* [PATCH v3 19/22] resolve_ref(): emit warnings for improperly-formatted references","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-10-12T04:41:39Z","receivedAt":"2011-10-12T04:41:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> At any rate, I think the changes should be all or nothing. If the\n> warning goes away, fine. But if the warning stays, and dwim_ref is going\n> to have special rules for looking in the top-level $GIT_DIR, then things\n> like shorten_unambiguous_ref need to respect those rules, or we've just\n> created a new bug.\n\nWhether we remove the warning or not, I think it would be an improvement\nnot to look at random files directly underneath $GIT_DIR/. I am not sure\nhow we can be confident that we caught everything, though.\n\nIn other words, is shorten-unambiguous-refs the last one that needs\nfixing? How would we know for certain?\n\nAlso I tend to think Michael's \"only warn in refs/\" is probably not the\nright solution. When a caller asks to resolve_ref(MERGE_HEAD), one of\nthese things can be true:\n\n - A file $GIT_DIR/MERGE_HEAD does not exist; this is not inherently an\n error unless we were supposed to be in the middle of a conflicted\n merge.\n\n - A file $GIT_DIR/MERGE_HEAD exists, and records a correct 40-hexadecimal\n get_sha1_hex() can grok. This is perfectly normal.\n\n - A file $GIT_DIR/MERGE_HEAD exists, but get_sha1_hex() does not grok\n it. Michael warns against this twice, and I think it is a wrong thing\n to pass this unnoticed.\n\nOnce we tighten all the \"too loose accesses to $GIT_DIR/$random_filename\",\nwe might even want to have an option to cause the caller to die() in the\nerror case, and the logic is the same for refs under $GIT_DIR/refs/, not\njust the ref-like-things directly under $GIT_DIR.\n\nA regular ref, can also appear in $GIT_DIR/packed-refs, but a corruption\nof an entry in the file will be caught when the file is read and outside\nthe scope of this discussion, I think.\n"},{"id":"177425","messageId":"20111012045004.GA22413@sigill.intra.peff.net","threadId":"28403","inReplyTo":"7v39eyddoc.fsf@alter.siamese.dyndns.org","subject":"Re: Re* [PATCH v3 19/22] resolve_ref(): emit warnings for improperly-formatted references","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-10-12T04:50:05Z","receivedAt":"2011-10-12T04:50:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Oct 11, 2011 at 09:41:39PM -0700, Junio C Hamano wrote:\n\n> Whether we remove the warning or not, I think it would be an improvement\n> not to look at random files directly underneath $GIT_DIR/. I am not sure\n> how we can be confident that we caught everything, though.\n> \n> In other words, is shorten-unambiguous-refs the last one that needs\n> fixing? How would we know for certain?\n\nMy assumption was that the set of rules is defined by\nref_rev_parse_rules, so grepping for functions that access that would be\nsufficient.\n\nIt looks like there is also ref_fetch_rules, which serves a similar\npurpose. Uses of that would also need to be audited.\n\nI _think_ that should be enough for arbitrary lookup. I'm sure other\ncallsites directly say things like 'git_path(\"MERGE_HEAD\")', but that's\nnot a problem. They would be doing so with a well-defined top-level\nrefname. This is really just about the ref_rev_parse_rules lookups.\n\n-Peff\n"},{"id":"177447","messageId":"7vk48ab19q.fsf@alter.siamese.dyndns.org","threadId":"28403","inReplyTo":"20111011225434.GA24142@sigill.intra.peff.net","subject":"Re: Re* [PATCH v3 19/22] resolve_ref(): emit warnings for improperly-formatted references","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-10-12T16:52:33Z","receivedAt":"2011-10-12T16:52:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Subject: [PATCH] branch -m/-M: remove undocumented RENAMED-REF\n\nThe commit message for c976d41 (git-branch: add options and tests for\nbranch renaming, 2006-11-28) mentions RENAME_REF but otherwise this is not\ndocumented anywhere, and it does not appear in any of the tests.\n\nWorse yet, the name of the actual file is \"RENAMED-REF\".\n\nThis was supposed to hold the commit object name at the tip of the branch\nthe most recent \"branch -m/-M\" renamed, but that is not necessary in order\nto be able to recover from a mistake. Even when \"branch -M A B\" overwrites\nan existing branch B, what is kept in RENAMED-REF is the commit at the tip\nof the original branch A, not the commit B from the now-lost branch.\n\nJust remove this unused \"feature\".\n\nSigned-off-by: Junio C Hamano \n---\n\n * Let's do this.\n\n refs.c | 8 --------\n 1 files changed, 0 insertions(+), 8 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex cdedb45..ecfc22c 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -786,7 +786,6 @@ int delete_ref(const char *refname, unsigned char *sha1)\n \n int rename_ref(const char *oldref, const char *newref)\n {\n-\tstatic const char renamed_ref[] = \"RENAMED-REF\";\n \tunsigned char sha1[20], orig_sha1[20];\n \tint flag = 0, logmoved = 0;\n \tstruct ref_lock *lock;\n@@ -809,13 +808,6 @@ int rename_ref(const char *oldref, const char *newref)\n \tif (snprintf(msg, sizeof(msg), \"renamed %s to %s\", oldref, newref) > sizeof(msg))\n \t\treturn error(\"Refnames to long\");\n \n-\tlock = lock_ref_sha1_basic(renamed_ref, NULL, NULL);\n-\tif (!lock)\n-\t\treturn error(\"unable to lock %s\", renamed_ref);\n-\tlock->force_write = 1;\n-\tif (write_ref_sha1(lock, orig_sha1, msg))\n-\t\treturn error(\"unable to save current sha1 in %s\", renamed_ref);\n-\n \tif (log && rename(git_path(\"logs/%s\", oldref), git_path(\"tmp-renamed-log\")))\n \t\treturn error(\"unable to move logfile logs/%s to tmp-renamed-log: %s\",\n \t\t\toldref, strerror(errno));\n-- \n1.7.7.213.g8b0e1\n"},{"id":"177450","messageId":"7vzkh69k3r.fsf_-_@alter.siamese.dyndns.org","threadId":"28403","inReplyTo":"20111012045004.GA22413@sigill.intra.peff.net","subject":"[PATCH 1/2] refs.c: move dwim_ref()/dwim_log() from sha1_name.c","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-10-12T17:48:40Z","receivedAt":"2011-10-12T17:48:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"OK, so here is a re-roll of the one from yesterday. This one is new but is\na mere preparatory step.\n\n-- >8 --\n\nBoth dwim_ref()/dwim_log() functions are intimately related to the ref\nparsing rules defined in refs.c and better fits there. Move them together\nwith substitute_branch_name(), a file scope static helper function.\n\nSigned-off-by: Junio C Hamano \n---\n refs.c | 85 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n sha1_name.c | 85 -----------------------------------------------------------\n 2 files changed, 85 insertions(+), 85 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 832a52f..e3692bd 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1067,6 +1067,91 @@ static int is_refname_available(const char *ref, const char *oldref,\n \treturn 1;\n }\n \n+/*\n+ * *string and *len will only be substituted, and *string returned (for\n+ * later free()ing) if the string passed in is a magic short-hand form\n+ * to name a branch.\n+ */\n+static char *substitute_branch_name(const char **string, int *len)\n+{\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tint ret = interpret_branch_name(*string, &buf);\n+\n+\tif (ret == *len) {\n+\t\tsize_t size;\n+\t\t*string = strbuf_detach(&buf, &size);\n+\t\t*len = size;\n+\t\treturn (char *)*string;\n+\t}\n+\n+\treturn NULL;\n+}\n+\n+int dwim_ref(const char *str, int len, unsigned char *sha1, char **ref)\n+{\n+\tchar *last_branch = substitute_branch_name(&str, &len);\n+\tconst char **p, *r;\n+\tint refs_found = 0;\n+\n+\t*ref = NULL;\n+\tfor (p = ref_rev_parse_rules; *p; p++) {\n+\t\tchar fullref[PATH_MAX];\n+\t\tunsigned char sha1_from_ref[20];\n+\t\tunsigned char *this_result;\n+\t\tint flag;\n+\n+\t\tthis_result = refs_found ? sha1_from_ref : sha1;\n+\t\tmksnpath(fullref, sizeof(fullref), *p, len, str);\n+\t\tr = resolve_ref(fullref, this_result, 1, &flag);\n+\t\tif (r) {\n+\t\t\tif (!refs_found++)\n+\t\t\t\t*ref = xstrdup(r);\n+\t\t\tif (!warn_ambiguous_refs)\n+\t\t\t\tbreak;\n+\t\t} else if ((flag & REF_ISSYMREF) && strcmp(fullref, \"HEAD\"))\n+\t\t\twarning(\"ignoring dangling symref %s.\", fullref);\n+\t}\n+\tfree(last_branch);\n+\treturn refs_found;\n+}\n+\n+int dwim_log(const char *str, int len, unsigned char *sha1, char **log)\n+{\n+\tchar *last_branch = substitute_branch_name(&str, &len);\n+\tconst char **p;\n+\tint logs_found = 0;\n+\n+\t*log = NULL;\n+\tfor (p = ref_rev_parse_rules; *p; p++) {\n+\t\tstruct stat st;\n+\t\tunsigned char hash[20];\n+\t\tchar path[PATH_MAX];\n+\t\tconst char *ref, *it;\n+\n+\t\tmksnpath(path, sizeof(path), *p, len, str);\n+\t\tref = resolve_ref(path, hash, 1, NULL);\n+\t\tif (!ref)\n+\t\t\tcontinue;\n+\t\tif (!stat(git_path(\"logs/%s\", path), &st) &&\n+\t\t S_ISREG(st.st_mode))\n+\t\t\tit = path;\n+\t\telse if (strcmp(ref, path) &&\n+\t\t\t !stat(git_path(\"logs/%s\", ref), &st) &&\n+\t\t\t S_ISREG(st.st_mode))\n+\t\t\tit = ref;\n+\t\telse\n+\t\t\tcontinue;\n+\t\tif (!logs_found++) {\n+\t\t\t*log = xstrdup(it);\n+\t\t\thashcpy(sha1, hash);\n+\t\t}\n+\t\tif (!warn_ambiguous_refs)\n+\t\t\tbreak;\n+\t}\n+\tfree(last_branch);\n+\treturn logs_found;\n+}\n+\n static struct ref_lock *lock_ref_sha1_basic(const char *ref, const unsigned char *old_sha1, int flags, int *type_p)\n {\n \tchar *ref_file;\ndiff --git a/sha1_name.c b/sha1_name.c\nindex 143fd97..d423635 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -241,91 +241,6 @@ static int ambiguous_path(const char *path, int len)\n \treturn slash;\n }\n \n-/*\n- * *string and *len will only be substituted, and *string returned (for\n- * later free()ing) if the string passed in is a magic short-hand form\n- * to name a branch.\n- */\n-static char *substitute_branch_name(const char **string, int *len)\n-{\n-\tstruct strbuf buf = STRBUF_INIT;\n-\tint ret = interpret_branch_name(*string, &buf);\n-\n-\tif (ret == *len) {\n-\t\tsize_t size;\n-\t\t*string = strbuf_detach(&buf, &size);\n-\t\t*len = size;\n-\t\treturn (char *)*string;\n-\t}\n-\n-\treturn NULL;\n-}\n-\n-int dwim_ref(const char *str, int len, unsigned char *sha1, char **ref)\n-{\n-\tchar *last_branch = substitute_branch_name(&str, &len);\n-\tconst char **p, *r;\n-\tint refs_found = 0;\n-\n-\t*ref = NULL;\n-\tfor (p = ref_rev_parse_rules; *p; p++) {\n-\t\tchar fullref[PATH_MAX];\n-\t\tunsigned char sha1_from_ref[20];\n-\t\tunsigned char *this_result;\n-\t\tint flag;\n-\n-\t\tthis_result = refs_found ? sha1_from_ref : sha1;\n-\t\tmksnpath(fullref, sizeof(fullref), *p, len, str);\n-\t\tr = resolve_ref(fullref, this_result, 1, &flag);\n-\t\tif (r) {\n-\t\t\tif (!refs_found++)\n-\t\t\t\t*ref = xstrdup(r);\n-\t\t\tif (!warn_ambiguous_refs)\n-\t\t\t\tbreak;\n-\t\t} else if ((flag & REF_ISSYMREF) && strcmp(fullref, \"HEAD\"))\n-\t\t\twarning(\"ignoring dangling symref %s.\", fullref);\n-\t}\n-\tfree(last_branch);\n-\treturn refs_found;\n-}\n-\n-int dwim_log(const char *str, int len, unsigned char *sha1, char **log)\n-{\n-\tchar *last_branch = substitute_branch_name(&str, &len);\n-\tconst char **p;\n-\tint logs_found = 0;\n-\n-\t*log = NULL;\n-\tfor (p = ref_rev_parse_rules; *p; p++) {\n-\t\tstruct stat st;\n-\t\tunsigned char hash[20];\n-\t\tchar path[PATH_MAX];\n-\t\tconst char *ref, *it;\n-\n-\t\tmksnpath(path, sizeof(path), *p, len, str);\n-\t\tref = resolve_ref(path, hash, 1, NULL);\n-\t\tif (!ref)\n-\t\t\tcontinue;\n-\t\tif (!stat(git_path(\"logs/%s\", path), &st) &&\n-\t\t S_ISREG(st.st_mode))\n-\t\t\tit = path;\n-\t\telse if (strcmp(ref, path) &&\n-\t\t\t !stat(git_path(\"logs/%s\", ref), &st) &&\n-\t\t\t S_ISREG(st.st_mode))\n-\t\t\tit = ref;\n-\t\telse\n-\t\t\tcontinue;\n-\t\tif (!logs_found++) {\n-\t\t\t*log = xstrdup(it);\n-\t\t\thashcpy(sha1, hash);\n-\t\t}\n-\t\tif (!warn_ambiguous_refs)\n-\t\t\tbreak;\n-\t}\n-\tfree(last_branch);\n-\treturn logs_found;\n-}\n-\n static inline int upstream_mark(const char *string, int len)\n {\n \tconst char *suffix[] = { \"@{upstream}\", \"@{u}\" };\n-- \n1.7.7.213.g8b0e1\n"},{"id":"177451","messageId":"7vvcru9k22.fsf_-_@alter.siamese.dyndns.org","threadId":"28403","inReplyTo":"20111012045004.GA22413@sigill.intra.peff.net","subject":"[PATCH 2/2] Restrict ref-like names immediately below $GIT_DIR","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-10-12T17:49:41Z","receivedAt":"2011-10-12T17:49:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"We have always dwimmed the user input $string into a ref by first looking\ndirectly inside $GIT_DIR, and then in $GIT_DIR/refs, $GIT_DIR/refs/tags,\netc., and that is what made\n\n git log HEAD..MERGE_HEAD\n\nwork correctly. This however means that\n\n git rev-parse config\n git log index\n\nwould look at $GIT_DIR/config and $GIT_DIR/index and see if they are valid\nrefs.\n\nTo reduce confusion, let's not dwim a path immediately below $GIT_DIR that\nis not all-caps.\n\nSigned-off-by: Junio C Hamano \n---\n\n * And this adds coverage to refname_match() and shorten_unambiguous_ref()\n on top of the one from yesterday.\n \n refs.c | 36 ++++++++++++++++++++++++++++++++++--\n 1 files changed, 34 insertions(+), 2 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex e3692bd..e54c482 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -994,12 +994,34 @@ const char *ref_fetch_rules[] = {\n \tNULL\n };\n \n+static int refname_ok_at_root_level(const char *str, int len)\n+{\n+\tint seen_non_root_char = 0;\n+\n+\twhile (len--) {\n+\t\tchar ch = *str++;\n+\n+\t\tif (ch == '/')\n+\t\t\treturn 1;\n+\t\t/*\n+\t\t * Only accept likes of .git/HEAD, .git/MERGE_HEAD at\n+\t\t * the root level as a ref.\n+\t\t */\n+\t\tif (ch != '_' && (ch < 'A' || 'Z' < ch))\n+\t\t\tseen_non_root_char = 1;\n+\t}\n+\treturn !seen_non_root_char;\n+}\n+\n int refname_match(const char *abbrev_name, const char *full_name, const char **rules)\n {\n \tconst char **p;\n \tconst int abbrev_name_len = strlen(abbrev_name);\n \n \tfor (p = rules; *p; p++) {\n+\t\tif (p == rules &&\n+\t\t !refname_ok_at_root_level(abbrev_name, abbrev_name_len))\n+\t\t\tcontinue;\n \t\tif (!strcmp(full_name, mkpath(*p, abbrev_name_len, abbrev_name))) {\n \t\t\treturn 1;\n \t\t}\n@@ -1100,6 +1122,8 @@ int dwim_ref(const char *str, int len, unsigned char *sha1, char **ref)\n \t\tunsigned char *this_result;\n \t\tint flag;\n \n+\t\tif (p == ref_rev_parse_rules && !refname_ok_at_root_level(str, len))\n+\t\t\tcontinue;\n \t\tthis_result = refs_found ? sha1_from_ref : sha1;\n \t\tmksnpath(fullref, sizeof(fullref), *p, len, str);\n \t\tr = resolve_ref(fullref, this_result, 1, &flag);\n@@ -1128,6 +1152,8 @@ int dwim_log(const char *str, int len, unsigned char *sha1, char **log)\n \t\tchar path[PATH_MAX];\n \t\tconst char *ref, *it;\n \n+\t\tif (p == ref_rev_parse_rules && !refname_ok_at_root_level(str, len))\n+\t\t\tcontinue;\n \t\tmksnpath(path, sizeof(path), *p, len, str);\n \t\tref = resolve_ref(path, hash, 1, NULL);\n \t\tif (!ref)\n@@ -2045,12 +2071,14 @@ char *shorten_unambiguous_ref(const char *ref, int strict)\n \t/* buffer for scanf result, at most ref must fit */\n \tshort_name = xstrdup(ref);\n \n-\t/* skip first rule, it will always match */\n-\tfor (i = nr_rules - 1; i > 0 ; --i) {\n+\tfor (i = nr_rules - 1; i >= 0; i--) {\n \t\tint j;\n \t\tint rules_to_fail = i;\n \t\tint short_name_len;\n \n+\t\tif (!i && !refname_ok_at_root_level(ref, strlen(ref)))\n+\t\t\tcontinue;\n+\n \t\tif (1 != sscanf(ref, scanf_fmts[i], short_name))\n \t\t\tcontinue;\n \n@@ -2076,6 +2104,10 @@ char *shorten_unambiguous_ref(const char *ref, int strict)\n \t\t\tif (i == j)\n \t\t\t\tcontinue;\n \n+\t\t\tif (!j &&\n+\t\t\t !refname_ok_at_root_level(short_name, short_name_len))\n+\t\t\t\tcontinue;\n+\n \t\t\t/*\n \t\t\t * the short name is ambiguous, if it resolves\n \t\t\t * (with this previous rule) to a valid ref\n-- \n1.7.7.213.g8b0e1\n"},{"id":"177453","messageId":"4E95D60B.5030904@alum.mit.edu","threadId":"28403","inReplyTo":"7vvcru9k22.fsf_-_@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/2] Restrict ref-like names immediately below $GIT_DIR","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2011-10-12T18:01:47Z","receivedAt":"2011-10-12T18:01:47Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 10/12/2011 07:49 PM, Junio C Hamano wrote:\n> diff --git a/refs.c b/refs.c\n> index e3692bd..e54c482 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -994,12 +994,34 @@ const char *ref_fetch_rules[] = {\n> \tNULL\n> };\n> \n> +static int refname_ok_at_root_level(const char *str, int len)\n> +{\n> +\tint seen_non_root_char = 0;\n> +\n> +\twhile (len--) {\n> +\t\tchar ch = *str++;\n> +\n> +\t\tif (ch == '/')\n> +\t\t\treturn 1;\n> +\t\t/*\n> +\t\t * Only accept likes of .git/HEAD, .git/MERGE_HEAD at\n> +\t\t * the root level as a ref.\n> +\t\t */\n> +\t\tif (ch != '_' && (ch < 'A' || 'Z' < ch))\n> +\t\t\tseen_non_root_char = 1;\n> +\t}\n> +\treturn !seen_non_root_char;\n> +}\n> +\n\nNit: the seen_non_root_char variable can be replaced by an early \"return\n0\" from the loop and \"return 1\" if the loop falls through.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"177454","messageId":"7vr52i9j8g.fsf@alter.siamese.dyndns.org","threadId":"28403","inReplyTo":"4E95D60B.5030904@alum.mit.edu","subject":"Re: [PATCH 2/2] Restrict ref-like names immediately below $GIT_DIR","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-10-12T18:07:27Z","receivedAt":"2011-10-12T18:07:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> On 10/12/2011 07:49 PM, Junio C Hamano wrote:\n>> diff --git a/refs.c b/refs.c\n>> index e3692bd..e54c482 100644\n>> --- a/refs.c\n>> +++ b/refs.c\n>> @@ -994,12 +994,34 @@ const char *ref_fetch_rules[] = {\n>> \tNULL\n>> };\n>> \n>> +static int refname_ok_at_root_level(const char *str, int len)\n>> +{\n>> +\tint seen_non_root_char = 0;\n>> +\n>> +\twhile (len--) {\n>> +\t\tchar ch = *str++;\n>> +\n>> +\t\tif (ch == '/')\n>> +\t\t\treturn 1;\n>> +\t\t/*\n>> +\t\t * Only accept likes of .git/HEAD, .git/MERGE_HEAD at\n>> +\t\t * the root level as a ref.\n>> +\t\t */\n>> +\t\tif (ch != '_' && (ch < 'A' || 'Z' < ch))\n>> +\t\t\tseen_non_root_char = 1;\n>> +\t}\n>> +\treturn !seen_non_root_char;\n>> +}\n>> +\n>\n> Nit: the seen_non_root_char variable can be replaced by an early \"return\n> 0\" from the loop and \"return 1\" if the loop falls through.\n\nHmm, I thought that would fail when you feed \"refs/heads/master\" to the\nfunction.\n"},{"id":"177475","messageId":"7vipnu81al.fsf@alter.siamese.dyndns.org","threadId":"28403","inReplyTo":"20111011230749.GA29785@sigill.intra.peff.net","subject":"Re: Re* [PATCH v3 19/22] resolve_ref(): emit warnings for improperly-formatted references","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-10-12T19:20:18Z","receivedAt":"2011-10-12T19:20:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> We also look at ref_rev_parse_rules in shorten_unambiguous_ref. So even\n> with your patch, I still get the warning with:\n>\n> $ git branch config\n> $ git for-each-ref --format='%(refname:short)' refs/heads/\n>\n> It looks like we also use it in remote.c:count_refspec_match, but I\n> haven't figured out if that can trigger a warning or not.\n\nI do not think this is \"do not trigger a warning\" exercise. This is \"we no\nlonger consider names outside refs/ as potential magic refs unless they\nare all uppercase\".\n\nIf the updated dwim_ref() says $GIT_DIR/frotz will no longer create\nambiguity with $GIT_DIR/refs/heads/frotz, then refname_match() needs to\nknow about it, and cause count_refspec_match() to say that \"frotz\"\nuniquely names \"refs/heads/frotz\".\n\nThe same stands for shorten_unambiguous_ref(). If $GIT_DIR/frotz no longer\nis ambiguous with $GIT_DIR/refs/heads/frotz, then %(refname:shrot) for\nrefs/heads/frotz should yield \"frotz\", and we should not have to qualify\nit as \"heads/frotz\".\n"},{"id":"177476","messageId":"20111012192649.GA26636@sigill.intra.peff.net","threadId":"28403","inReplyTo":"7vipnu81al.fsf@alter.siamese.dyndns.org","subject":"Re: Re* [PATCH v3 19/22] resolve_ref(): emit warnings for improperly-formatted references","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-10-12T19:26:49Z","receivedAt":"2011-10-12T19:26:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Oct 12, 2011 at 12:20:18PM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > We also look at ref_rev_parse_rules in shorten_unambiguous_ref. So even\n> > with your patch, I still get the warning with:\n> >\n> > $ git branch config\n> > $ git for-each-ref --format='%(refname:short)' refs/heads/\n> >\n> > It looks like we also use it in remote.c:count_refspec_match, but I\n> > haven't figured out if that can trigger a warning or not.\n> \n> I do not think this is \"do not trigger a warning\" exercise. This is \"we no\n> longer consider names outside refs/ as potential magic refs unless they\n> are all uppercase\".\n> \n> If the updated dwim_ref() says $GIT_DIR/frotz will no longer create\n> ambiguity with $GIT_DIR/refs/heads/frotz, then refname_match() needs to\n> know about it, and cause count_refspec_match() to say that \"frotz\"\n> uniquely names \"refs/heads/frotz\".\n> \n> The same stands for shorten_unambiguous_ref(). If $GIT_DIR/frotz no longer\n> is ambiguous with $GIT_DIR/refs/heads/frotz, then %(refname:shrot) for\n> refs/heads/frotz should yield \"frotz\", and we should not have to qualify\n> it as \"heads/frotz\".\n\nAbsolutely. I was lazy in how I woreded it, but I meant that the warning\nwas the indicator that we were doing something buggy, not that it was\nthe bug itself. It just happened to be a convenient way to test. :)\n\nYour most recent patch looks right.\n\n-Peff\n"},{"id":"177490","messageId":"4E9609E3.1000300@alum.mit.edu","threadId":"28403","inReplyTo":"7vr52i9j8g.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/2] Restrict ref-like names immediately below $GIT_DIR","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2011-10-12T21:42:59Z","receivedAt":"2011-10-12T21:42:59Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 10/12/2011 08:07 PM, Junio C Hamano wrote:\n> Michael Haggerty writes:\n> \n>> On 10/12/2011 07:49 PM, Junio C Hamano wrote:\n>>> diff --git a/refs.c b/refs.c\n>>> index e3692bd..e54c482 100644\n>>> --- a/refs.c\n>>> +++ b/refs.c\n>>> @@ -994,12 +994,34 @@ const char *ref_fetch_rules[] = {\n>>> \tNULL\n>>> };\n>>> \n>>> +static int refname_ok_at_root_level(const char *str, int len)\n>>> +{\n>>> +\tint seen_non_root_char = 0;\n>>> +\n>>> +\twhile (len--) {\n>>> +\t\tchar ch = *str++;\n>>> +\n>>> +\t\tif (ch == '/')\n>>> +\t\t\treturn 1;\n>>> +\t\t/*\n>>> +\t\t * Only accept likes of .git/HEAD, .git/MERGE_HEAD at\n>>> +\t\t * the root level as a ref.\n>>> +\t\t */\n>>> +\t\tif (ch != '_' && (ch < 'A' || 'Z' < ch))\n>>> +\t\t\tseen_non_root_char = 1;\n>>> +\t}\n>>> +\treturn !seen_non_root_char;\n>>> +}\n>>> +\n>>\n>> Nit: the seen_non_root_char variable can be replaced by an early \"return\n>> 0\" from the loop and \"return 1\" if the loop falls through.\n> \n> Hmm, I thought that would fail when you feed \"refs/heads/master\" to the\n> function.\n\nYou're right. My brain must be scrambled from all of the rebasing that\nI have been doing ;-)\n\nHow about adding\n\n/*\n * Accept strings that are either ALL_CAPS or include a '/'.\n */\n\n(I think the underscore is implied by the example, but the comment could\nbe expanded if necessary to be explicit.)\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"177492","messageId":"20111012215105.GA4498@sigill.intra.peff.net","threadId":"28403","inReplyTo":"7vvcru9k22.fsf_-_@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/2] Restrict ref-like names immediately below $GIT_DIR","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-10-12T21:51:05Z","receivedAt":"2011-10-12T21:51:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Oct 12, 2011 at 10:49:41AM -0700, Junio C Hamano wrote:\n\n> +static int refname_ok_at_root_level(const char *str, int len)\n> +{\n> +\tint seen_non_root_char = 0;\n> +\n> +\twhile (len--) {\n> +\t\tchar ch = *str++;\n> +\n> +\t\tif (ch == '/')\n> +\t\t\treturn 1;\n> +\t\t/*\n> +\t\t * Only accept likes of .git/HEAD, .git/MERGE_HEAD at\n> +\t\t * the root level as a ref.\n> +\t\t */\n> +\t\tif (ch != '_' && (ch < 'A' || 'Z' < ch))\n> +\t\t\tseen_non_root_char = 1;\n> +\t}\n> +\treturn !seen_non_root_char;\n> +}\n\nI thought from your earlier comment:\n\n> I wanted to start as loose as possible to avoid negatively impacting\n> existing users, later to tighten. As fsck and friends never look\n> outside of refs/, I think the prefix refs/ is a reasonable restriction\n> that is safe.\n\nthat you did agree with tightening this up to allow just refs/ as a\nsubdirectory.\n\nSquashable patch is below.\n\ndiff --git a/refs.c b/refs.c\nindex 0f26d9d..b159c4a 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -994,21 +994,20 @@ int check_refname_format(const char *ref, int flags)\n \n static int refname_ok_at_root_level(const char *str, int len)\n {\n-\tint seen_non_root_char = 0;\n+\tif (len >= 5 && !memcmp(str, \"refs/\", 5))\n+\t\treturn 1;\n \n \twhile (len--) {\n \t\tchar ch = *str++;\n \n-\t\tif (ch == '/')\n-\t\t\treturn 1;\n \t\t/*\n \t\t * Only accept likes of .git/HEAD, .git/MERGE_HEAD at\n \t\t * the root level as a ref.\n \t\t */\n \t\tif (ch != '_' && (ch < 'A' || 'Z' < ch))\n-\t\t\tseen_non_root_char = 1;\n+\t\t\treturn 0;\n \t}\n-\treturn !seen_non_root_char;\n+\treturn 1;\n }\n \n int refname_match(const char *abbrev_name, const char *full_name, const char **rules)\n"},{"id":"177495","messageId":"7vobxl7snw.fsf@alter.siamese.dyndns.org","threadId":"28403","inReplyTo":"4E9609E3.1000300@alum.mit.edu","subject":"Re: [PATCH 2/2] Restrict ref-like names immediately below $GIT_DIR","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-10-12T22:26:43Z","receivedAt":"2011-10-12T22:26:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> /*\n> * Accept strings that are either ALL_CAPS or include a '/'.\n> */\n>\n> (I think the underscore is implied by the example, but the comment could\n> be expanded if necessary to be explicit.)\n\nI like that comment hints \"_\" by having it between all and caps ;-).\n"},{"id":"177981","messageId":"7v39epft32.fsf@alter.siamese.dyndns.org","threadId":"28403","inReplyTo":"4E9609E3.1000300@alum.mit.edu","subject":"Re: [PATCH 2/2] Restrict ref-like names immediately below $GIT_DIR","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-10-19T05:28:33Z","receivedAt":"2011-10-19T05:28:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n>>> Nit: the seen_non_root_char variable can be replaced by an early \"return\n>>> 0\" from the loop and \"return 1\" if the loop falls through.\n>> \n>> Hmm, I thought that would fail when you feed \"refs/heads/master\" to the\n>> function.\n>\n> You're right. My brain must be scrambled from all of the rebasing that\n> I have been doing ;-)\n>\n> How about adding\n>\n> /*\n> * Accept strings that are either ALL_CAPS or include a '/'.\n> */\n>\n> (I think the underscore is implied by the example, but the comment could\n> be expanded if necessary to be explicit.)\n\nI was trying to summarize this topic for Release Notes.\n\n Possibly incompatible changes\n -----------------------------\n\n * Special refs such as \"HEAD\", \"MERGE_HEAD\", and \"FETCH_HEAD\" are now\n restricted to names with only uppercase letters and underscore. All\n other refs must live under refs/ namespace. Earlier, you could\n confuse git by storing an object name in $GIT_DIR/tmp/junk and say\n \"git show tmp/junk\", but this will no longer work.\n\nBut noticed that \"git update-ref tmp/junk HEAD\" does create such a ref\nthat won't be recognized, and \"git check-ref-format tmp/junk\" is happy.\n\nI think we would need to restrict check_ref_format() so that these\ncommands (and possibly others, but I think that single function will cover\npretty much everything) also reject \"tmp/junk\" immediately below $GIT_DIR\nas a bad string. Otherwise we cannot merge these fixups, which would mean\nwe would have to revert the \"Clean up refname checks and normalization\"\nseries, at least the part that started emitting the \"warning\", which is a\nmess I would rather want to avoid.\n\nOpinions on how to get us out of this mess?\n"},{"id":"177982","messageId":"7vty75ec54.fsf@alter.siamese.dyndns.org","threadId":"28403","inReplyTo":"7v39epft32.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/2] Restrict ref-like names immediately below $GIT_DIR","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-10-19T06:19:51Z","receivedAt":"2011-10-19T06:19:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> I was trying to summarize this topic for Release Notes.\n>\n> Possibly incompatible changes\n> -----------------------------\n>\n> * Special refs such as \"HEAD\", \"MERGE_HEAD\", and \"FETCH_HEAD\" are now\n> restricted to names with only uppercase letters and underscore. All\n> other refs must live under refs/ namespace. Earlier, you could\n> confuse git by storing an object name in $GIT_DIR/tmp/junk and say\n> \"git show tmp/junk\", but this will no longer work.\n>\n> But noticed that \"git update-ref tmp/junk HEAD\" does create such a ref\n> that won't be recognized, and \"git check-ref-format tmp/junk\" is happy.\n>\n> I think we would need to restrict check_ref_format() so that these\n> commands (and possibly others, but I think that single function will cover\n> pretty much everything) also reject \"tmp/junk\" immediately below $GIT_DIR\n> as a bad string. Otherwise we cannot merge these fixups, which would mean\n> we would have to revert the \"Clean up refname checks and normalization\"\n> series, at least the part that started emitting the \"warning\", which is a\n> mess I would rather want to avoid.\n>\n> Opinions on how to get us out of this mess?\n\nAddendum.\n\nI was digging this further and see fairly large conflicts between the bulk\nof \"clean up refname checks and normalization\" topic and the logic to\navoid the additional warning by tightening the dwimmery.\n\ncheck_refname_format() has always assumed that it is OK to be called at\nany level of substring, and there are many code like this one (example is\nfrom remote.c::get_fetch_map()):\n\n for (rmp = &ref_map; *rmp; ) {\n if ((*rmp)->peer_ref) {\n if (check_refname_format((*rmp)->peer_ref->name + 5,\n REFNAME_ALLOW_ONELEVEL)) {\n struct ref *ignore = *rmp;\n error(\"* Ignoring funny ref '%s' locally\",\n (*rmp)->peer_ref->name);\n\nThis code somehow _knows_ that peer_ref->name begins with \"refs/\" and that\nis the reason it adds 5 to skip that known part. In this particular case,\nI think we can simply drop the +5 and allow-onelevel, but there are other\ninstances of the calls to the function that feeds the rest of the refname\nstring, skipping leading substring (not necessarily \"refs/\"), assuming\nthat any component string is either valid or invalid no matter where it\nappears in the full refname. I wouldn't be surprised if some callers do\nnot even have enough information to tell what the leading substring would\nbe in the full refname context (e.g. parsing of \"master:master\" refspec,\nrelying on the later dwimmery to add refs/heads/ in front, could just\nverify that \"master\" is in good format with allow-onelevel).\n\nThe new restriction bolted on to that logic in jc/check-ref-format-fixup\nseries to work around the new warning in 629cd3a (resolve_ref(): emit\nwarnings for improperly-formatted references, 2011-09-15) is incompatible\nwith the assumption, as we would need to check full refname, and treat the\nfirst refname component very differently from other components. It has to\nbe either \"refs\" in multi-component refname, or all caps in a one-level\none, but the callers of check_refname_format() are not designed to feed\nthe full refname to begin with.\n\nI am tempted to revert 629cd3a (resolve_ref(): emit warnings for\nimproperly-formatted references, 2011-09-15) that started giving the\nwarnings, and drop the jc/check-ref-format-fixup topic [*1*] altogether,\nbut that is a short-sighted workaround I would rather want to avoid. It\nessentially declares that the \"Clean up refname checks\" topic was a\nfailure and did not manage to really clean things up.\n\nA possible alternative might be to leave check_refname_format() and its\ncallers as they are, introduce check_full_refname() function that knows\nthe new restriction on top of check-ref-format-fixup, and use that in\nlock_ref_sha1(), lock_any_ref_for_update() and is_refname_available()\n[*2*]. That way, we can keep the potentially useful \"ill-formed contents\nin the ref\" warning and avoid possible confusion caused by random files\nthat are directly under $GIT_DIR, which would be far more preferable in\nthe longer term.\n\nAnybody wants to give it a try?\n\n\n[Footnote]\n\n*1* The topic is misnamed; it is about fixing dwimmery, not checking.\n\n*2* This currently does not seem to check the well-formedness of the ref\nit is creating at all; it should check the \"ref\", but not \"oldref\" to\nallow renaming a malformed ref to correct earlier mistakes.\n"},{"id":"177997","messageId":"4E9EEA4D.50207@alum.mit.edu","threadId":"28403","inReplyTo":"7vty75ec54.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/2] Restrict ref-like names immediately below $GIT_DIR","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2011-10-19T15:18:37Z","receivedAt":"2011-10-19T15:18:37Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 10/19/2011 08:19 AM, Junio C Hamano wrote:\n> Junio C Hamano writes:\n> \n>> I was trying to summarize this topic for Release Notes.\n>>\n>> Possibly incompatible changes\n>> -----------------------------\n>>\n>> * Special refs such as \"HEAD\", \"MERGE_HEAD\", and \"FETCH_HEAD\" are now\n>> restricted to names with only uppercase letters and underscore. All\n>> other refs must live under refs/ namespace. Earlier, you could\n>> confuse git by storing an object name in $GIT_DIR/tmp/junk and say\n>> \"git show tmp/junk\", but this will no longer work.\n>>\n>> But noticed that \"git update-ref tmp/junk HEAD\" does create such a ref\n>> that won't be recognized, and \"git check-ref-format tmp/junk\" is happy.\n>>\n>> I think we would need to restrict check_ref_format() so that these\n>> commands (and possibly others, but I think that single function will cover\n>> pretty much everything) also reject \"tmp/junk\" immediately below $GIT_DIR\n>> as a bad string. Otherwise we cannot merge these fixups, which would mean\n>> we would have to revert the \"Clean up refname checks and normalization\"\n>> series, at least the part that started emitting the \"warning\", which is a\n>> mess I would rather want to avoid.\n>>\n>> Opinions on how to get us out of this mess?\n> \n> Addendum.\n> \n> I was digging this further and see fairly large conflicts between the bulk\n> of \"clean up refname checks and normalization\" topic and the logic to\n> avoid the additional warning by tightening the dwimmery.\n> \n> check_refname_format() has always assumed that it is OK to be called at\n> any level of substring, and there are many code like this one (example is\n> from remote.c::get_fetch_map()):\n> \n> for (rmp = &ref_map; *rmp; ) {\n> if ((*rmp)->peer_ref) {\n> if (check_refname_format((*rmp)->peer_ref->name + 5,\n> REFNAME_ALLOW_ONELEVEL)) {\n> struct ref *ignore = *rmp;\n> error(\"* Ignoring funny ref '%s' locally\",\n> (*rmp)->peer_ref->name);\n> \n> This code somehow _knows_ that peer_ref->name begins with \"refs/\" and that\n> is the reason it adds 5 to skip that known part. In this particular case,\n> I think we can simply drop the +5 and allow-onelevel, but there are other\n> instances of the calls to the function that feeds the rest of the refname\n> string, skipping leading substring (not necessarily \"refs/\"), assuming\n> that any component string is either valid or invalid no matter where it\n> appears in the full refname. I wouldn't be surprised if some callers do\n> not even have enough information to tell what the leading substring would\n> be in the full refname context (e.g. parsing of \"master:master\" refspec,\n> relying on the later dwimmery to add refs/heads/ in front, could just\n> verify that \"master\" is in good format with allow-onelevel).\n> \n> The new restriction bolted on to that logic in jc/check-ref-format-fixup\n> series to work around the new warning in 629cd3a (resolve_ref(): emit\n> warnings for improperly-formatted references, 2011-09-15) is incompatible\n> with the assumption, as we would need to check full refname, and treat the\n> first refname component very differently from other components. It has to\n> be either \"refs\" in multi-component refname, or all caps in a one-level\n> one, but the callers of check_refname_format() are not designed to feed\n> the full refname to begin with.\n> \n> I am tempted to revert 629cd3a (resolve_ref(): emit warnings for\n> improperly-formatted references, 2011-09-15) that started giving the\n> warnings, and drop the jc/check-ref-format-fixup topic [*1*] altogether,\n> but that is a short-sighted workaround I would rather want to avoid. It\n> essentially declares that the \"Clean up refname checks\" topic was a\n> failure and did not manage to really clean things up.\n> \n> A possible alternative might be to leave check_refname_format() and its\n> callers as they are, introduce check_full_refname() function that knows\n> the new restriction on top of check-ref-format-fixup, and use that in\n> lock_ref_sha1(), lock_any_ref_for_update() and is_refname_available()\n> [*2*]. That way, we can keep the potentially useful \"ill-formed contents\n> in the ref\" warning and avoid possible confusion caused by random files\n> that are directly under $GIT_DIR, which would be far more preferable in\n> the longer term.\n> \n> Anybody wants to give it a try?\n\nI think that the refs/-or-ALL_CAPS test fits most naturally in\ncheck_refname_format(), controlled by an option flag.\n\nI'm starting by building parts of the solution, namely something like:\n\n* Add an option REFNAME_ALLOW_DWIM which causes check_refname_format()\nto accept reference names that *could* be dwimmed into a valid refname.\n Specifically, when this option is *not* specified, then the option must\nstart with \"refs/\" or be ALL_CAPS. When it *is* specified, the behavior\nwill be much like the current behavior when ALLOW_ONELEVEL is specified\n(in fact, the new option might make ALLOW_ONELEVEL obsolete).\n\n* Add a new function parse_refname_prefix(str, len, flags) which tries\nto read a refname from the front of str (of length len, not necessarily\nNUL-terminated) and returns the length of the part that could be used.\nThis function will support the same flag argument as\ncheck_refname_format() (in fact the latter would become a trivial\nwrapper of the new function). I expect that this function will be\nuseful for dwimmery.\n\nHopefully I'll have patches before the end of the (Berlin) day.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"178002","messageId":"7vr528di0d.fsf@alter.siamese.dyndns.org","threadId":"28403","inReplyTo":"4E9EEA4D.50207@alum.mit.edu","subject":"Re: [PATCH 2/2] Restrict ref-like names immediately below $GIT_DIR","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-10-19T17:10:42Z","receivedAt":"2011-10-19T17:10:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> On 10/19/2011 08:19 AM, Junio C Hamano wrote:\n> \n>> A possible alternative might be to leave check_refname_format() and its\n>> callers as they are, introduce check_full_refname() function that knows\n>> the new restriction on top of check-ref-format-fixup, and use that in\n>> lock_ref_sha1(), lock_any_ref_for_update() and is_refname_available()\n>> [*2*]. That way, we can keep the potentially useful \"ill-formed contents\n>> in the ref\" warning and avoid possible confusion caused by random files\n>> that are directly under $GIT_DIR, which would be far more preferable in\n>> the longer term.\n>> \n>> Anybody wants to give it a try?\n>\n> I think that the refs/-or-ALL_CAPS test fits most naturally in\n> check_refname_format(), controlled by an option flag.\n\nThat is fine too, if a separate function check_full_refname() would end up\nbeing nothing more than a thin wrapper that passes the \"we are checking a\nfull refname\" option.\n\n> I'm starting by building parts of the solution, namely something like:\n> ...\n>\n> Hopefully I'll have patches before the end of the (Berlin) day.\n\nIt is unclear how these two functions could be related to the issue, but\nhopefully we will soon find out when we see your patch. Thanks.\n"},{"id":"178007","messageId":"7vaa8wdbld.fsf@alter.siamese.dyndns.org","threadId":"28403","inReplyTo":"7v39epft32.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/2] Restrict ref-like names immediately below $GIT_DIR","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-10-19T19:29:18Z","receivedAt":"2011-10-19T19:29:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> I was trying to summarize this topic for Release Notes.\n>\n> Possibly incompatible changes\n> -----------------------------\n>\n> * Special refs such as \"HEAD\", \"MERGE_HEAD\", and \"FETCH_HEAD\" are now\n> restricted to names with only uppercase letters and underscore. All\n> other refs must live under refs/ namespace. Earlier, you could\n> confuse git by storing an object name in $GIT_DIR/tmp/junk and say\n> \"git show tmp/junk\", but this will no longer work.\n>\n> But noticed that \"git update-ref tmp/junk HEAD\" does create such a ref\n> that won't be recognized, and \"git check-ref-format tmp/junk\" is happy.\n>\n> I think we would need to restrict check_ref_format() so that these\n> commands (and possibly others, but I think that single function will cover\n> pretty much everything) also reject \"tmp/junk\" immediately below $GIT_DIR\n> as a bad string. Otherwise we cannot merge these fixups, which would mean\n> we would have to revert the \"Clean up refname checks and normalization\"\n> series, at least the part that started emitting the \"warning\", which is a\n> mess I would rather want to avoid.\n>\n> Opinions on how to get us out of this mess?\n\nLet me step back a bit to avoid making hasty decisions that may cause\nnegative effect on the end users only because a single topic with a\npossible fallout has been merged to 'master' already (a sane position for\nsuch a situation always has been to revert the merge of the topic so far,\nbut I ended up being hasty, only because the merge was deliberately made\nearly in the cycle to give us enough time to deal with fallouts).\n\nIn general, the \"Hey that file that appears in our dwimmed ref namespace\ndoes not store correct [0-9a-f]{40} contents\" warning message is a good\nthing to have. When we try the dwimmery and disambiguation, we however\nlook at the potential refs and warn disambiguity only when two or more\nsuch files have good contents. E.g. if I do this:\n\n $ git rev-parse HEAD >.git/refs/heads/frotz\n $ echo hello >.git/refs/tags/frotz\n $ git show frotz\n\nwe have never paid attention to the broken tag and showed the 'frotz'\nbranch without complaining. Once tags/frotz gets a real object name,\nhowever, we start giving ambiguity warnings.\n\nPerhaps that is what we should be fixing instead.\n\nRight now, dwim_ref() classifies candidates into two categories, ones that\nresolve_ref() succeeds, and others that resolve_ref() does not. And when\nwe have two or more candidates that successfully resolves, there is an\nambiguity.\n\nPerhaps we need the third kind: ones that exist on the filesystem but\nare ill-formed.\n\nWhen naïvely looked at, the code in 'master' may seem to be doing just\nthat, but it does _not_ have any way to affect the ambiguity detection\nlogic even if the caller wanted to. The warning is issued at a wrong\nlevel.\n\nPerhaps resolve_ref() should return in its *flag parameter that \"a file\nexists there but incorrectly formatted\", and dwim_ref() should notice and\nuse that information to warn about ambiguity and also illformed-ness.\n\nA patch is attached at the end of this message to minimally fix what is in\n'master' (without the jc/check-ref-format-fixup topic). This update allows\nus to make dwim_ref() notice such \"exists but broken\" candidates and later\ntake them into consideration when deciding if a name is ambiguous, but I\ndid not want to change the semantics from the traditional implementation,\nso it only uses this information to warn ref breakages. Also this squelches\nthe phony \"index is not well formed\" warning against \"git show index --\"\nby knowing that many files directly under $GIT_DIR are not ref-like things.\nMichaels's \"when taken as a full refname, is this string valid?\" update\nmentioned in the thread may be used to replace the strchr(fullref, '/') check\nthat can be seen in this patch.\n\n refs.c | 22 +++++++++++-----------\n refs.h | 5 +++--\n sha1_name.c | 5 ++++-\n 3 files changed, 18 insertions(+), 14 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex cab4394..0f58e46 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -4,9 +4,8 @@\n #include \"tag.h\"\n #include \"dir.h\"\n \n-/* ISSYMREF=01 and ISPACKED=02 are public interfaces */\n-#define REF_KNOWS_PEELED 04\n-#define REF_BROKEN 010\n+/* ISSYMREF=0x01, ISPACKED=0x02 and ISBROKEN=0x04 are public interfaces */\n+#define REF_KNOWS_PEELED 0x10\n \n struct ref_entry {\n \tunsigned char flag; /* ISSYMREF? ISPACKED? */\n@@ -329,12 +328,12 @@ static void get_ref_dir(const char *submodule, const char *base,\n \t\t\t\tflag = 0;\n \t\t\t\tif (resolve_gitlink_ref(submodule, ref, sha1) < 0) {\n \t\t\t\t\thashclr(sha1);\n-\t\t\t\t\tflag |= REF_BROKEN;\n+\t\t\t\t\tflag |= REF_ISBROKEN;\n \t\t\t\t}\n \t\t\t} else\n \t\t\t\tif (!resolve_ref(ref, sha1, 1, &flag)) {\n \t\t\t\t\thashclr(sha1);\n-\t\t\t\t\tflag |= REF_BROKEN;\n+\t\t\t\t\tflag |= REF_ISBROKEN;\n \t\t\t\t}\n \t\t\tadd_ref(ref, sha1, flag, array, NULL);\n \t\t}\n@@ -501,7 +500,6 @@ const char *resolve_ref(const char *ref, unsigned char *sha1, int reading, int *\n \tssize_t len;\n \tchar buffer[256];\n \tstatic char ref_buffer[256];\n-\tchar path[PATH_MAX];\n \n \tif (flag)\n \t\t*flag = 0;\n@@ -510,6 +508,7 @@ const char *resolve_ref(const char *ref, unsigned char *sha1, int reading, int *\n \t\treturn NULL;\n \n \tfor (;;) {\n+\t\tchar path[PATH_MAX];\n \t\tstruct stat st;\n \t\tchar *buf;\n \t\tint fd;\n@@ -586,8 +585,8 @@ const char *resolve_ref(const char *ref, unsigned char *sha1, int reading, int *\n \t\twhile (isspace(*buf))\n \t\t\tbuf++;\n \t\tif (check_refname_format(buf, REFNAME_ALLOW_ONELEVEL)) {\n-\t\t\twarning(\"symbolic reference in %s is formatted incorrectly\",\n-\t\t\t\tpath);\n+\t\t\tif (flag)\n+\t\t\t\t*flag |= REF_ISBROKEN;\n \t\t\treturn NULL;\n \t\t}\n \t\tref = strcpy(ref_buffer, buf);\n@@ -596,7 +595,8 @@ const char *resolve_ref(const char *ref, unsigned char *sha1, int reading, int *\n \t}\n \t/* Please note that FETCH_HEAD has a second line containing other data. */\n \tif (get_sha1_hex(buffer, sha1) || (buffer[40] != '\\0' && !isspace(buffer[40]))) {\n-\t\twarning(\"reference in %s is formatted incorrectly\", path);\n+\t\tif (flag)\n+\t\t\t*flag |= REF_ISBROKEN;\n \t\treturn NULL;\n \t}\n \treturn ref;\n@@ -624,8 +624,8 @@ static int do_one_ref(const char *base, each_ref_fn fn, int trim,\n \t\treturn 0;\n \n \tif (!(flags & DO_FOR_EACH_INCLUDE_BROKEN)) {\n-\t\tif (entry->flag & REF_BROKEN)\n-\t\t\treturn 0; /* ignore dangling symref */\n+\t\tif (entry->flag & REF_ISBROKEN)\n+\t\t\treturn 0; /* ignore dangling symref and corrupt ref */\n \t\tif (!has_sha1_file(entry->sha1)) {\n \t\t\terror(\"%s does not point to a valid object!\", entry->name);\n \t\t\treturn 0;\ndiff --git a/refs.h b/refs.h\nindex 0229c57..7442b29 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -10,8 +10,9 @@ struct ref_lock {\n \tint force_write;\n };\n \n-#define REF_ISSYMREF 01\n-#define REF_ISPACKED 02\n+#define REF_ISSYMREF 0x01\n+#define REF_ISPACKED 0x02\n+#define REF_ISBROKEN 0x04\n \n /*\n * Calls the specified function for each ref file until it returns nonzero,\ndiff --git a/sha1_name.c b/sha1_name.c\nindex ba976b4..1fe37c6 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -282,8 +282,11 @@ int dwim_ref(const char *str, int len, unsigned char *sha1, char **ref)\n \t\t\t\t*ref = xstrdup(r);\n \t\t\tif (!warn_ambiguous_refs)\n \t\t\t\tbreak;\n-\t\t} else if ((flag & REF_ISSYMREF) && strcmp(fullref, \"HEAD\"))\n+\t\t} else if ((flag & REF_ISSYMREF) && strcmp(fullref, \"HEAD\")) {\n \t\t\twarning(\"ignoring dangling symref %s.\", fullref);\n+\t\t} else if ((flag & REF_ISBROKEN) && strchr(fullref, '/')) {\n+\t\t\twarning(\"ignoring broken ref %s.\", fullref);\n+\t\t}\n \t}\n \tfree(last_branch);\n \treturn refs_found;\n"},{"id":"178009","messageId":"7v62jkdb3t.fsf_-_@alter.siamese.dyndns.org","threadId":"28403","inReplyTo":"7vaa8wdbld.fsf@alter.siamese.dyndns.org","subject":"[PATCH] resolve_ref(): report breakage to the caller without warning","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-10-19T19:39:50Z","receivedAt":"2011-10-19T19:39:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"629cd3a (resolve_ref(): emit warnings for improperly-formatted references,\n2011-09-15) made resolve_ref() warn against files that are found in the\ndirectories the ref dwimmery looks at. The intent may be good, but these\nmessages come from a wrong level of the API hierarchy.\n\nInstead record the breakage in \"flags\" whose purpose is to explain the\nresult of the function to the caller, who is in a much better position to\nmake intelligent decision based on the information.\n\nThis updates sha1_name.c::dwim_ref() to warn against such a broken\ncandidate only when it does not appear directly below $GIT_DIR to restore\nthe traditional behaviour, as we know many files directly underneath\n$GIT_DIR/ are are not refs.\n\nWarning against \"git show config --\" with \"$GIT_DIR/config does not look\nlike a well-formed ref\" does not make sense, and we may later tweak the\ndwimmery not to even consider them as candidates, but that is a longer\nterm topic.\n\nSigned-off-by: Junio C Hamano \n---\n\n * With a log message and the same patch text.\n\n refs.c | 22 +++++++++++-----------\n refs.h | 5 +++--\n sha1_name.c | 5 ++++-\n 3 files changed, 18 insertions(+), 14 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex cab4394..0f58e46 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -4,9 +4,8 @@\n #include \"tag.h\"\n #include \"dir.h\"\n \n-/* ISSYMREF=01 and ISPACKED=02 are public interfaces */\n-#define REF_KNOWS_PEELED 04\n-#define REF_BROKEN 010\n+/* ISSYMREF=0x01, ISPACKED=0x02 and ISBROKEN=0x04 are public interfaces */\n+#define REF_KNOWS_PEELED 0x10\n \n struct ref_entry {\n \tunsigned char flag; /* ISSYMREF? ISPACKED? */\n@@ -329,12 +328,12 @@ static void get_ref_dir(const char *submodule, const char *base,\n \t\t\t\tflag = 0;\n \t\t\t\tif (resolve_gitlink_ref(submodule, ref, sha1) < 0) {\n \t\t\t\t\thashclr(sha1);\n-\t\t\t\t\tflag |= REF_BROKEN;\n+\t\t\t\t\tflag |= REF_ISBROKEN;\n \t\t\t\t}\n \t\t\t} else\n \t\t\t\tif (!resolve_ref(ref, sha1, 1, &flag)) {\n \t\t\t\t\thashclr(sha1);\n-\t\t\t\t\tflag |= REF_BROKEN;\n+\t\t\t\t\tflag |= REF_ISBROKEN;\n \t\t\t\t}\n \t\t\tadd_ref(ref, sha1, flag, array, NULL);\n \t\t}\n@@ -501,7 +500,6 @@ const char *resolve_ref(const char *ref, unsigned char *sha1, int reading, int *\n \tssize_t len;\n \tchar buffer[256];\n \tstatic char ref_buffer[256];\n-\tchar path[PATH_MAX];\n \n \tif (flag)\n \t\t*flag = 0;\n@@ -510,6 +508,7 @@ const char *resolve_ref(const char *ref, unsigned char *sha1, int reading, int *\n \t\treturn NULL;\n \n \tfor (;;) {\n+\t\tchar path[PATH_MAX];\n \t\tstruct stat st;\n \t\tchar *buf;\n \t\tint fd;\n@@ -586,8 +585,8 @@ const char *resolve_ref(const char *ref, unsigned char *sha1, int reading, int *\n \t\twhile (isspace(*buf))\n \t\t\tbuf++;\n \t\tif (check_refname_format(buf, REFNAME_ALLOW_ONELEVEL)) {\n-\t\t\twarning(\"symbolic reference in %s is formatted incorrectly\",\n-\t\t\t\tpath);\n+\t\t\tif (flag)\n+\t\t\t\t*flag |= REF_ISBROKEN;\n \t\t\treturn NULL;\n \t\t}\n \t\tref = strcpy(ref_buffer, buf);\n@@ -596,7 +595,8 @@ const char *resolve_ref(const char *ref, unsigned char *sha1, int reading, int *\n \t}\n \t/* Please note that FETCH_HEAD has a second line containing other data. */\n \tif (get_sha1_hex(buffer, sha1) || (buffer[40] != '\\0' && !isspace(buffer[40]))) {\n-\t\twarning(\"reference in %s is formatted incorrectly\", path);\n+\t\tif (flag)\n+\t\t\t*flag |= REF_ISBROKEN;\n \t\treturn NULL;\n \t}\n \treturn ref;\n@@ -624,8 +624,8 @@ static int do_one_ref(const char *base, each_ref_fn fn, int trim,\n \t\treturn 0;\n \n \tif (!(flags & DO_FOR_EACH_INCLUDE_BROKEN)) {\n-\t\tif (entry->flag & REF_BROKEN)\n-\t\t\treturn 0; /* ignore dangling symref */\n+\t\tif (entry->flag & REF_ISBROKEN)\n+\t\t\treturn 0; /* ignore dangling symref and corrupt ref */\n \t\tif (!has_sha1_file(entry->sha1)) {\n \t\t\terror(\"%s does not point to a valid object!\", entry->name);\n \t\t\treturn 0;\ndiff --git a/refs.h b/refs.h\nindex 0229c57..7442b29 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -10,8 +10,9 @@ struct ref_lock {\n \tint force_write;\n };\n \n-#define REF_ISSYMREF 01\n-#define REF_ISPACKED 02\n+#define REF_ISSYMREF 0x01\n+#define REF_ISPACKED 0x02\n+#define REF_ISBROKEN 0x04\n \n /*\n * Calls the specified function for each ref file until it returns nonzero,\ndiff --git a/sha1_name.c b/sha1_name.c\nindex ba976b4..1fe37c6 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -282,8 +282,11 @@ int dwim_ref(const char *str, int len, unsigned char *sha1, char **ref)\n \t\t\t\t*ref = xstrdup(r);\n \t\t\tif (!warn_ambiguous_refs)\n \t\t\t\tbreak;\n-\t\t} else if ((flag & REF_ISSYMREF) && strcmp(fullref, \"HEAD\"))\n+\t\t} else if ((flag & REF_ISSYMREF) && strcmp(fullref, \"HEAD\")) {\n \t\t\twarning(\"ignoring dangling symref %s.\", fullref);\n+\t\t} else if ((flag & REF_ISBROKEN) && strchr(fullref, '/')) {\n+\t\t\twarning(\"ignoring broken ref %s.\", fullref);\n+\t\t}\n \t}\n \tfree(last_branch);\n \treturn refs_found;\n-- \n1.7.7.494.g7d7707\n"},{"id":"178011","messageId":"4E9F33B9.4040803@alum.mit.edu","threadId":"28403","inReplyTo":"7vaa8wdbld.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/2] Restrict ref-like names immediately below $GIT_DIR","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2011-10-19T20:31:53Z","receivedAt":"2011-10-19T20:31:53Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 10/19/2011 09:29 PM, Junio C Hamano wrote:\n> In general, the \"Hey that file that appears in our dwimmed ref namespace\n> does not store correct [0-9a-f]{40} contents\" warning message is a good\n> thing to have. When we try the dwimmery and disambiguation, we however\n> look at the potential refs and warn disambiguity only when two or more\n> such files have good contents. E.g. if I do this:\n> \n> $ git rev-parse HEAD >.git/refs/heads/frotz\n> $ echo hello >.git/refs/tags/frotz\n> $ git show frotz\n> \n> we have never paid attention to the broken tag and showed the 'frotz'\n> branch without complaining. Once tags/frotz gets a real object name,\n> however, we start giving ambiguity warnings.\n> \n> Perhaps that is what we should be fixing instead.\n\nThis all sounds very sane. dwim_ref() currently casts about wildly\nlooking for possible references, so it makes sense that it be selective\nabout what warnings it emits. I also agree with the principle that this\nwarning is better emitted from higher-level code.\n\n> Perhaps resolve_ref() should return in its *flag parameter that \"a file\n> exists there but incorrectly formatted\", and dwim_ref() should notice and\n> use that information to warn about ambiguity and also illformed-ness.\n\nISTM that such warnings should also be emitted when (for example) the\nfollowing commands encounter corrupt references: \"git fsck\", \"git\npack-refs\", \"git gc\", and \"git push\". (Maybe these commands already\nemit warnings; I haven't checked.) These commands are not run so\nfrequently (so that the warnings would not be so annoying). They are\nalso presumably not so promiscuous about where they look for refs and\ntherefore won't generate so many false alarms.\n\nBut it will be easy to add warnings to these commands using the\nREF_ISBROKEN flag that you made public.\n\n> A patch is attached at the end of this message to minimally fix what is in\n> 'master' (without the jc/check-ref-format-fixup topic). [...]\n\nI would have preferred this change being applied on top of your first\npatch in jc/check-ref-format-fixup, because moving these functions to\nrefs.c makes sense. (Not to mention that my \"REFNAME_FULL\" patch series\nis built on top of jc/check-ref-format-fixup.)\n\n> refs.c | 22 +++++++++++-----------\n> refs.h | 5 +++--\n> sha1_name.c | 5 ++++-\n> 3 files changed, 18 insertions(+), 14 deletions(-)\n> \n> diff --git a/refs.c b/refs.c\n> index cab4394..0f58e46 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -329,12 +328,12 @@ static void get_ref_dir(const char *submodule, const char *base,\n> \t\t\t\tflag = 0;\n> \t\t\t\tif (resolve_gitlink_ref(submodule, ref, sha1) < 0) {\n> \t\t\t\t\thashclr(sha1);\n> -\t\t\t\t\tflag |= REF_BROKEN;\n> +\t\t\t\t\tflag |= REF_ISBROKEN;\n> \t\t\t\t}\n\nThe renaming of this constant could have been done in a separate patch\nto reduce noise like this in the main patch.\n\nOtherwise the patch looks fine to me.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"178012","messageId":"7vwrc0bts6.fsf@alter.siamese.dyndns.org","threadId":"28403","inReplyTo":"4E9F33B9.4040803@alum.mit.edu","subject":"Re: [PATCH 2/2] Restrict ref-like names immediately below $GIT_DIR","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-10-19T20:39:21Z","receivedAt":"2011-10-19T20:39:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> I would have preferred this change being applied on top of your first\n> patch in jc/check-ref-format-fixup, because moving these functions to\n> refs.c makes sense.\n\nThanks for a quick sanity check, and I agree.\n"}]}