{"thread":{"id":"39916","subject":"[PATCH v3] refs: loosen restrictions on wildcard '*' refspecs","startedAt":"2015-07-22T16:06:20Z","lastAt":"2015-07-22T18:24:39Z","messageCount":3,"participants":["Jacob Keller","David Turner"],"isPatch":true,"patchVersion":3,"patchTotal":null},"messages":[{"id":"266609","messageId":"1437581180-26883-1-git-send-email-jacob.e.keller@intel.com","threadId":"39916","inReplyTo":null,"subject":"[PATCH v3] refs: loosen restrictions on wildcard '*' refspecs","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2015-07-22T16:06:20Z","receivedAt":"2015-07-22T16:06:20Z","isPatch":true,"sender":{"key":"jacob.e.keller@intel.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"From: Jacob Keller \n\nUpdate the check_refname_component logic in order to allow for a less\nstrict refspec format in regards to REFNAME_REFSPEC_PATTERN. Previously\nthe '*' could only replace a single full component, and could not\nreplace arbitrary text. Now, refs such as `foo/bar*:foo/bar*` will be\naccepted. This allows for somewhat more flexibility in references and\ndoes not break any current users. The ref matching code already allows\nthis but the check_refname_format did not. Note this does also allow\nrefs such as `foo/bar*:foe/baz*`, that is, arbitrary renames. This was\nalready possible with namespace sections before, but now is possible\neven as part of the pattern section. Since users have to explicitly type\nthese into the configuration it does not seem an issue.\n\nAlso streamline the code by making this new check part of\ncheck_refname_component instead of checking after we error during\ncheck_refname_format, which fits better with how other issues in refname\ncomponents are checked.\n\nSigned-off-by: Jacob Keller \nCc: Daniel Barkalow \nCc: Junio C Hamano \n---\n\nI updated the patch description a bit. This is also re-based onto the\nnext/ branch in-case. This is mostly a resend so that anyone interested\nin review has another chance to see the patch.\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 fc02959ba4ab..9044dfaadae1 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 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 `*`\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 ce8cd8d45001..3002015ff289 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 e82fca51f501..1809a1d57577 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -278,8 +278,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 e5dc62e9efbf..0790edf60de2 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.4.3\n"},{"id":"266612","messageId":"1437589261.4520.2.camel@twopensource.com","threadId":"39916","inReplyTo":"1437581180-26883-1-git-send-email-jacob.e.keller@intel.com","subject":"Re: [PATCH v3] refs: loosen restrictions on wildcard '*' refspecs","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2015-07-22T18:21:01Z","receivedAt":"2015-07-22T18:21:01Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"On Wed, 2015-07-22 at 09:06 -0700, Jacob Keller wrote:\n> From: Jacob Keller \n> \n> Update the check_refname_component logic in order to allow for a less\n> strict refspec format in regards to REFNAME_REFSPEC_PATTERN. Previously\n> the '*' could only replace a single full component, and could not\n> replace arbitrary text. Now, refs such as `foo/bar*:foo/bar*` will be\n> accepted. This allows for somewhat more flexibility in references and\n> does not break any current users. The ref matching code already allows\n> this but the check_refname_format did not. Note this does also allow\n> refs such as `foo/bar*:foe/baz*`, that is, arbitrary renames. This was\n> already possible with namespace sections before, but now is possible\n> even as part of the pattern section. Since users have to explicitly type\n> these into the configuration it does not seem an issue.\n> \n> Also streamline 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 fits better with how other issues in refname\n> components are checked.\n> \n> Signed-off-by: Jacob Keller \n> Cc: Daniel Barkalow \n> Cc: Junio C Hamano \n> ---\n> \n> I updated the patch description a bit. This is also re-based onto the\n> next/ branch in-case. This is mostly a resend so that anyone interested\n> in review has another chance to see the patch.\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 fc02959ba4ab..9044dfaadae1 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 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 `*`\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 (`/`)\n> diff --git a/refs.c b/refs.c\n> index ce8cd8d45001..3002015ff289 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\nHow about this:\n+ 5: *, reject unless REFNAME_REFSPEC_PATTERN is set\n\n(I guess it's possible that we would later allow other pattern chars,\nbut we could change the message then).\n"},{"id":"266613","messageId":"CA+P7+xrnRTJZ1j1bPx=3CNtOiNLxHux5bR9b1gaJKeM2_LZ6Kw@mail.gmail.com","threadId":"39916","inReplyTo":"1437589261.4520.2.camel@twopensource.com","subject":"Re: [PATCH v3] refs: loosen restrictions on wildcard '*' refspecs","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2015-07-22T18:24:39Z","receivedAt":"2015-07-22T18:24:39Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Wed, Jul 22, 2015 at 11:21 AM, David Turner wrote:\n> On Wed, 2015-07-22 at 09:06 -0700, Jacob Keller wrote:\n>> From: Jacob Keller \n>>\n>> Update the check_refname_component logic in order to allow for a less\n>> strict refspec format in regards to REFNAME_REFSPEC_PATTERN. Previously\n>> the '*' could only replace a single full component, and could not\n>> replace arbitrary text. Now, refs such as `foo/bar*:foo/bar*` will be\n>> accepted. This allows for somewhat more flexibility in references and\n>> does not break any current users. The ref matching code already allows\n>> this but the check_refname_format did not. Note this does also allow\n>> refs such as `foo/bar*:foe/baz*`, that is, arbitrary renames. This was\n>> already possible with namespace sections before, but now is possible\n>> even as part of the pattern section. Since users have to explicitly type\n>> these into the configuration it does not seem an issue.\n>>\n>> Also streamline 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 fits better with how other issues in refname\n>> components are checked.\n>>\n>> Signed-off-by: Jacob Keller \n>> Cc: Daniel Barkalow \n>> Cc: Junio C Hamano \n>> ---\n>>\n>> I updated the patch description a bit. This is also re-based onto the\n>> next/ branch in-case. This is mostly a resend so that anyone interested\n>> in review has another chance to see the patch.\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 fc02959ba4ab..9044dfaadae1 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 as a reference name pattern for a refspec\n>> (as used with remote repositories). If this option is\n>> enabled, 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 ce8cd8d45001..3002015ff289 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> How about this:\n> + 5: *, reject unless REFNAME_REFSPEC_PATTERN is set\n>\n> (I guess it's possible that we would later allow other pattern chars,\n> but we could change the message then).\n>\n>\n>\n\nOk that makes sense\n\nRegards,\nJake\n"}]}