{"thread":{"id":"39918","subject":"[PATCH v4] refs: loosen restrictions on wildcard '*' refspecs","startedAt":"2015-07-22T18:32:09Z","lastAt":"2015-07-22T19:36:31Z","messageCount":2,"participants":["Jacob Keller","Junio C Hamano"],"isPatch":true,"patchVersion":4,"patchTotal":null},"messages":[{"id":"266614","messageId":"1437589929-14546-1-git-send-email-jacob.e.keller@intel.com","threadId":"39918","inReplyTo":null,"subject":"[PATCH v4] refs: loosen restrictions on wildcard '*' refspecs","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2015-07-22T18:32:09Z","receivedAt":"2015-07-22T18:32:09Z","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\n* Changed since v3\n- updated the description of 5 in refs.c as suggested by David Turner\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..a65f16fedaa0 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: *, 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":"266624","messageId":"xmqq380grnls.fsf@gitster.dls.corp.google.com","threadId":"39918","inReplyTo":"1437589929-14546-1-git-send-email-jacob.e.keller@intel.com","subject":"Re: [PATCH v4] refs: loosen restrictions on wildcard '*' refspecs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-07-22T19:36:31Z","receivedAt":"2015-07-22T19:36:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jacob Keller writes:\n\n> diff --git a/refs.c b/refs.c\n> index ce8cd8d45001..a65f16fedaa0 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: *, reject unless REFNAME_REFSPEC_PATTERN is set\n\nThe fact that this patch does not have to change the description for\n'4:' is an indication that the original description for '4:' was\nincomplete. Otherwise the original would have listed \"*\" among\nothers like \"~\", \"^\", and this patch would have updated it.\n\nThis mixes a fix/cleanup with an enhancement.\n\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\nThis also mixes a fix/cleanup with an enhancement. The original\nshould have had these \", or\" but it didn't.\n\nCan you split this patch into two, i.e.\n\n * [1/2] is to only clean-up the places these two hunks apply,\n without changing the behaviour at all.\n\n Please make sure that updated description for \"4:\" covers\n everything that is \"a bad character\". We noticed the lack of '*'\n only because of your patch, but I do not know (and did not check)\n if that was the only thing that was missing.\n\n * [2/2] is what you really wanted to do with this patch,\n i.e. updating the entry for '*' in the disposition table and all\n changes outside the above two hunks.\n\nThanks.\n"}]}