{"thread":{"id":"39814","subject":"[PATCH v2] refs: loosen restrictions on wildcard '*' refspecs","startedAt":"2015-07-08T13:00:43Z","lastAt":"2015-07-10T03:45:53Z","messageCount":2,"participants":["Jacob Keller"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"265847","messageId":"1436360443-26036-1-git-send-email-jacob.keller@gmail.com","threadId":"39814","inReplyTo":null,"subject":"[PATCH v2] refs: loosen restrictions on wildcard '*' refspecs","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2015-07-08T13:00:43Z","receivedAt":"2015-07-08T13:00:43Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"This patch updates the check_refname_component logic in order to allow for\na less strict refspec format in regards to REFNAME_REFSPEC_PATTERN.\nPreviously the '*' could only replace a single full component, and could\nnot replace arbitrary text. Now, refs such as `foo/bar*:foo/bar*` will be\naccepted. This allows for somewhat more flexibility in references and does\nnot break any current users. The ref matching code already allows this but\nthe check_refname_format did not.\n\nThis patch also streamlines the code by making this new check part of\ncheck_refname_component instead of checking after we error during\ncheck_refname_format, which makes more sense with how we check other\nissues in refname components.\n\nSigned-off-by: Jacob Keller <jacob.keller@gmail.com>\nCc: Daniel Barkalow <barkalow@iabervon.iabervon.org>\nCc: Junio C Hamano <gitster@pobox.com>\n---\n\n- v2\n* update test suite\n\n Documentation/git-check-ref-format.txt |  4 ++--\n refs.c                                 | 39 +++++++++++++++++++---------------\n refs.h                                 |  4 ++--\n t/t1402-check-ref-format.sh            |  8 ++++---\n 4 files changed, 31 insertions(+), 24 deletions(-)\n\ndiff --git a/Documentation/git-check-ref-format.txt b/Documentation/git-check-ref-format.txt\nindex fc02959..9044dfa 100644\n--- a/Documentation/git-check-ref-format.txt\n+++ b/Documentation/git-check-ref-format.txt\n@@ -94,8 +94,8 @@ OPTIONS\n \tInterpret <refname> as a reference name pattern for a refspec\n \t(as used with remote repositories).  If this option is\n \tenabled, <refname> is allowed to contain a single `*`\n-\tin place of a one full pathname component (e.g.,\n-\t`foo/*/bar` but not `foo/bar*`).\n+\tin the refspec (e.g., `foo/bar*/baz` or `foo/bar*baz/`\n+\tbut not `foo/bar*/baz*`).\n \n --normalize::\n \tNormalize 'refname' by removing any leading slash (`/`)\ndiff --git a/refs.c b/refs.c\nindex 7ac05cf..8702644 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -20,11 +20,12 @@ struct ref_lock {\n  * 2: ., look for a preceding . to reject .. in refs\n  * 3: {, look for a preceding @ to reject @{ in refs\n  * 4: A bad character: ASCII control characters, \"~\", \"^\", \":\" or SP\n+ * 5: check for patterns to reject unless REFNAME_REFSPEC_PATTERN is set\n  */\n static unsigned char refname_disposition[256] = {\n \t1, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4,\n \t4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4,\n-\t4, 0, 0, 0, 0, 0, 0, 0, 0, 0, 4, 0, 0, 0, 2, 1,\n+\t4, 0, 0, 0, 0, 0, 0, 0, 0, 0, 5, 0, 0, 0, 2, 1,\n \t0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 4, 0, 0, 0, 0, 4,\n \t0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,\n \t0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 4, 4, 0, 4, 0,\n@@ -71,11 +72,13 @@ static unsigned char refname_disposition[256] = {\n  * - any path component of it begins with \".\", or\n  * - it has double dots \"..\", or\n  * - it has ASCII control character, \"~\", \"^\", \":\" or SP, anywhere, or\n- * - it ends with a \"/\".\n- * - it ends with \".lock\"\n- * - it contains a \"\\\" (backslash)\n+ * - it ends with a \"/\", or\n+ * - it ends with \".lock\", or\n+ * - it contains a \"\\\" (backslash), or\n+ * - it contains a \"@{\" portion, or\n+ * - it contains a '*' unless REFNAME_REFSPEC_PATTERN is set\n  */\n-static int check_refname_component(const char *refname, int flags)\n+static int check_refname_component(const char *refname, int *flags)\n {\n \tconst char *cp;\n \tchar last = '\\0';\n@@ -96,6 +99,16 @@ static int check_refname_component(const char *refname, int flags)\n \t\t\tbreak;\n \t\tcase 4:\n \t\t\treturn -1;\n+\t\tcase 5:\n+\t\t\tif (!(*flags & REFNAME_REFSPEC_PATTERN))\n+\t\t\t\treturn -1; /* refspec can't be a pattern */\n+\n+\t\t\t/*\n+\t\t\t * Unset the pattern flag so that we only accept a single glob for\n+\t\t\t * the entire refspec.\n+\t\t\t */\n+\t\t\t*flags &= ~ REFNAME_REFSPEC_PATTERN;\n+\t\t\tbreak;\n \t\t}\n \t\tlast = ch;\n \t}\n@@ -120,18 +133,10 @@ int check_refname_format(const char *refname, 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(refname, flags);\n-\t\tif (component_len <= 0) {\n-\t\t\tif ((flags & REFNAME_REFSPEC_PATTERN) &&\n-\t\t\t\t\trefname[0] == '*' &&\n-\t\t\t\t\t(refname[1] == '\\0' || refname[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\t\tcomponent_len = 1;\n-\t\t\t} else {\n-\t\t\t\treturn -1;\n-\t\t\t}\n-\t\t}\n+\t\tcomponent_len = check_refname_component(refname, &flags);\n+\t\tif (component_len <= 0)\n+\t\t\treturn -1;\n+\n \t\tcomponent_count++;\n \t\tif (refname[component_len] == '\\0')\n \t\t\tbreak;\ndiff --git a/refs.h b/refs.h\nindex 8c3d433..1fd1272 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -224,8 +224,8 @@ extern int for_each_reflog(each_ref_fn, void *);\n  * to the rules described in Documentation/git-check-ref-format.txt.\n  * If 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+ * allow a single \"*\" wildcard character in the refspec. No leading or\n+ * repeated slashes are accepted.\n  */\n extern int check_refname_format(const char *refname, int flags);\n \ndiff --git a/t/t1402-check-ref-format.sh b/t/t1402-check-ref-format.sh\nindex e5dc62e..0790edf 100755\n--- a/t/t1402-check-ref-format.sh\n+++ b/t/t1402-check-ref-format.sh\n@@ -62,9 +62,11 @@ 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+valid_ref 'heads/*foo/bar' --refspec-pattern\n+valid_ref 'heads/foo*/bar' --refspec-pattern\n+valid_ref 'heads/f*o/bar' --refspec-pattern\n+invalid_ref 'heads/f*o*/bar' --refspec-pattern\n+invalid_ref 'heads/foo*/bar*' --refspec-pattern\n \n ref='foo'\n invalid_ref \"$ref\"\n-- \n2.5.0.rc1.2.gbb9760d\n"},{"id":"265965","messageId":"CA+P7+xr9kFPYVmVVyi919Wb2=Aq_yRa5tx-Z7Qr80V6Kt5hWOw@mail.gmail.com","threadId":"39814","inReplyTo":"1436360443-26036-1-git-send-email-jacob.keller@gmail.com","subject":"Re: [PATCH v2] refs: loosen restrictions on wildcard '*' refspecs","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2015-07-10T03:45:53Z","receivedAt":"2015-07-10T03:45:53Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Wed, Jul 8, 2015 at 6:00 AM, Jacob Keller <jacob.keller@gmail.com> wrote:\n> This patch updates the check_refname_component logic in order to allow for\n> a less strict refspec format in regards to REFNAME_REFSPEC_PATTERN.\n> Previously the '*' could only replace a single full component, and could\n> not replace arbitrary text. Now, refs such as `foo/bar*:foo/bar*` will be\n> accepted. This allows for somewhat more flexibility in references and does\n> not break any current users. The ref matching code already allows this but\n> the check_refname_format did not.\n>\n> This patch also streamlines the code by making this new check part of\n> check_refname_component instead of checking after we error during\n> check_refname_format, which makes more sense with how we check other\n> issues in refname components.\n>\n> Signed-off-by: Jacob Keller <jacob.keller@gmail.com>\n> Cc: Daniel Barkalow <barkalow@iabervon.iabervon.org>\n> Cc: Junio C Hamano <gitster@pobox.com>\n> ---\n>\n> - v2\n> * update test suite\n>\n>  Documentation/git-check-ref-format.txt |  4 ++--\n>  refs.c                                 | 39 +++++++++++++++++++---------------\n>  refs.h                                 |  4 ++--\n>  t/t1402-check-ref-format.sh            |  8 ++++---\n>  4 files changed, 31 insertions(+), 24 deletions(-)\n>\n> diff --git a/Documentation/git-check-ref-format.txt b/Documentation/git-check-ref-format.txt\n> index fc02959..9044dfa 100644\n> --- a/Documentation/git-check-ref-format.txt\n> +++ b/Documentation/git-check-ref-format.txt\n> @@ -94,8 +94,8 @@ OPTIONS\n>         Interpret <refname> as a reference name pattern for a refspec\n>         (as used with remote repositories).  If this option is\n>         enabled, <refname> is allowed to contain a single `*`\n> -       in place of a one full pathname component (e.g.,\n> -       `foo/*/bar` but not `foo/bar*`).\n> +       in the refspec (e.g., `foo/bar*/baz` or `foo/bar*baz/`\n> +       but not `foo/bar*/baz*`).\n>\n>  --normalize::\n>         Normalize 'refname' by removing any leading slash (`/`)\n> diff --git a/refs.c b/refs.c\n> index 7ac05cf..8702644 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -20,11 +20,12 @@ struct ref_lock {\n>   * 2: ., look for a preceding . to reject .. in refs\n>   * 3: {, look for a preceding @ to reject @{ in refs\n>   * 4: A bad character: ASCII control characters, \"~\", \"^\", \":\" or SP\n> + * 5: check for patterns to reject unless REFNAME_REFSPEC_PATTERN is set\n>   */\n>  static unsigned char refname_disposition[256] = {\n>         1, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4,\n>         4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4,\n> -       4, 0, 0, 0, 0, 0, 0, 0, 0, 0, 4, 0, 0, 0, 2, 1,\n> +       4, 0, 0, 0, 0, 0, 0, 0, 0, 0, 5, 0, 0, 0, 2, 1,\n>         0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 4, 0, 0, 0, 0, 4,\n>         0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,\n>         0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 4, 4, 0, 4, 0,\n> @@ -71,11 +72,13 @@ static unsigned char refname_disposition[256] = {\n>   * - any path component of it begins with \".\", or\n>   * - it has double dots \"..\", or\n>   * - it has ASCII control character, \"~\", \"^\", \":\" or SP, anywhere, or\n> - * - it ends with a \"/\".\n> - * - it ends with \".lock\"\n> - * - it contains a \"\\\" (backslash)\n> + * - it ends with a \"/\", or\n> + * - it ends with \".lock\", or\n> + * - it contains a \"\\\" (backslash), or\n> + * - it contains a \"@{\" portion, or\n> + * - it contains a '*' unless REFNAME_REFSPEC_PATTERN is set\n>   */\n> -static int check_refname_component(const char *refname, int flags)\n> +static int check_refname_component(const char *refname, int *flags)\n>  {\n>         const char *cp;\n>         char last = '\\0';\n> @@ -96,6 +99,16 @@ static int check_refname_component(const char *refname, int flags)\n>                         break;\n>                 case 4:\n>                         return -1;\n> +               case 5:\n> +                       if (!(*flags & REFNAME_REFSPEC_PATTERN))\n> +                               return -1; /* refspec can't be a pattern */\n> +\n> +                       /*\n> +                        * Unset the pattern flag so that we only accept a single glob for\n> +                        * the entire refspec.\n> +                        */\n> +                       *flags &= ~ REFNAME_REFSPEC_PATTERN;\n> +                       break;\n>                 }\n>                 last = ch;\n>         }\n> @@ -120,18 +133,10 @@ int check_refname_format(const char *refname, int flags)\n>\n>         while (1) {\n>                 /* We are at the start of a path component. */\n> -               component_len = check_refname_component(refname, flags);\n> -               if (component_len <= 0) {\n> -                       if ((flags & REFNAME_REFSPEC_PATTERN) &&\n> -                                       refname[0] == '*' &&\n> -                                       (refname[1] == '\\0' || refname[1] == '/')) {\n> -                               /* Accept one wildcard as a full refname component. */\n> -                               flags &= ~REFNAME_REFSPEC_PATTERN;\n> -                               component_len = 1;\n> -                       } else {\n> -                               return -1;\n> -                       }\n> -               }\n> +               component_len = check_refname_component(refname, &flags);\n> +               if (component_len <= 0)\n> +                       return -1;\n> +\n>                 component_count++;\n>                 if (refname[component_len] == '\\0')\n>                         break;\n> diff --git a/refs.h b/refs.h\n> index 8c3d433..1fd1272 100644\n> --- a/refs.h\n> +++ b/refs.h\n> @@ -224,8 +224,8 @@ extern int for_each_reflog(each_ref_fn, void *);\n>   * to the rules described in Documentation/git-check-ref-format.txt.\n>   * If 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> + * allow a single \"*\" wildcard character in the refspec. No leading or\n> + * repeated slashes are accepted.\n>   */\n>  extern int check_refname_format(const char *refname, int flags);\n>\n> diff --git a/t/t1402-check-ref-format.sh b/t/t1402-check-ref-format.sh\n> index e5dc62e..0790edf 100755\n> --- a/t/t1402-check-ref-format.sh\n> +++ b/t/t1402-check-ref-format.sh\n> @@ -62,9 +62,11 @@ 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> +valid_ref 'heads/*foo/bar' --refspec-pattern\n> +valid_ref 'heads/foo*/bar' --refspec-pattern\n> +valid_ref 'heads/f*o/bar' --refspec-pattern\n> +invalid_ref 'heads/f*o*/bar' --refspec-pattern\n> +invalid_ref 'heads/foo*/bar*' --refspec-pattern\n>\n>  ref='foo'\n>  invalid_ref \"$ref\"\n> --\n> 2.5.0.rc1.2.gbb9760d\n>\n\nHi Junio,\n\nping on this? I think I updated it as per your discussion on v2, but I\nhaven't seen any comments on this version yet.\n\nRegards,\nJake\n"}]}