{"thread":{"id":"8184","subject":"[PATCH] Move refspec pattern matching to match_refs().","startedAt":"2007-05-17T02:19:12Z","lastAt":"2007-05-17T05:55:59Z","messageCount":3,"participants":["Daniel Barkalow","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"42360","messageId":"Pine.LNX.4.64.0705162217250.18541@iabervon.org","threadId":"8184","inReplyTo":null,"subject":"[PATCH] Move refspec pattern matching to match_refs().","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2007-05-17T02:19:12Z","receivedAt":"2007-05-17T02:19:12Z","isPatch":true,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"This means that send-pack and http-push will support pattern refspecs,\nso builtin-push.c doesn't have to expand them, and also git push can\njust turn --tags into \"refs/tags/*\", further simplifying builtin-push.c\n\ncheck_ref_format() gets a third \"conditionally okay\" result for\nsomething that's valid as a pattern but not as a particular ref.\n\nSigned-off-by: Daniel Barkalow <barkalow@iabervon.org>\n---\n builtin-push.c |  133 +++++++++----------------------------------------------\n refs.c         |   25 +++++++---\n remote.c       |   35 +++++++++++++-\n send-pack.c    |    1 +\n 4 files changed, 72 insertions(+), 122 deletions(-)\n\ndiff --git a/builtin-push.c b/builtin-push.c\nindex 6084899..2612f07 100644\n--- a/builtin-push.c\n+++ b/builtin-push.c\n@@ -9,7 +9,7 @@\n \n static const char push_usage[] = \"git-push [--all] [--tags] [--receive-pack=<git-receive-pack>] [--repo=all] [-f | --force] [-v] [<repository> <refspec>...]\";\n \n-static int all, tags, force, thin = 1, verbose;\n+static int all, force, thin = 1, verbose;\n static const char *receivepack;\n \n static const char **refspec;\n@@ -23,114 +23,24 @@ static void add_refspec(const char *ref)\n \trefspec_nr = nr;\n }\n \n-static int expand_one_ref(const char *ref, const unsigned char *sha1, int flag, void *cb_data)\n-{\n-\t/* Ignore the \"refs/\" at the beginning of the refname */\n-\tref += 5;\n-\n-\tif (!prefixcmp(ref, \"tags/\"))\n-\t\tadd_refspec(xstrdup(ref));\n-\treturn 0;\n-}\n-\n-static void expand_refspecs(void)\n-{\n-\tif (all) {\n-\t\tif (refspec_nr)\n-\t\t\tdie(\"cannot mix '--all' and a refspec\");\n-\n-\t\t/*\n-\t\t * No need to expand \"--all\" - we'll just use\n-\t\t * the \"--all\" flag to send-pack\n-\t\t */\n-\t\treturn;\n-\t}\n-\tif (!tags)\n-\t\treturn;\n-\tfor_each_ref(expand_one_ref, NULL);\n-}\n-\n-struct wildcard_cb {\n-\tconst char *from_prefix;\n-\tint from_prefix_len;\n-\tconst char *to_prefix;\n-\tint to_prefix_len;\n-\tint force;\n-};\n-\n-static int expand_wildcard_ref(const char *ref, const unsigned char *sha1, int flag, void *cb_data)\n-{\n-\tstruct wildcard_cb *cb = cb_data;\n-\tint len = strlen(ref);\n-\tchar *expanded, *newref;\n-\n-\tif (len < cb->from_prefix_len ||\n-\t    memcmp(cb->from_prefix, ref, cb->from_prefix_len))\n-\t\treturn 0;\n-\texpanded = xmalloc(len * 2 + cb->force +\n-\t\t\t   (cb->to_prefix_len - cb->from_prefix_len) + 2);\n-\tnewref = expanded + cb->force;\n-\tif (cb->force)\n-\t\texpanded[0] = '+';\n-\tmemcpy(newref, ref, len);\n-\tnewref[len] = ':';\n-\tmemcpy(newref + len + 1, cb->to_prefix, cb->to_prefix_len);\n-\tstrcpy(newref + len + 1 + cb->to_prefix_len,\n-\t       ref + cb->from_prefix_len);\n-\tadd_refspec(expanded);\n-\treturn 0;\n-}\n-\n-static int wildcard_ref(const char *ref)\n-{\n-\tint len;\n-\tconst char *colon;\n-\tstruct wildcard_cb cb;\n-\n-\tmemset(&cb, 0, sizeof(cb));\n-\tif (ref[0] == '+') {\n-\t\tcb.force = 1;\n-\t\tref++;\n-\t}\n-\tlen = strlen(ref);\n-\tcolon = strchr(ref, ':');\n-\tif (! (colon && ref < colon &&\n-\t       colon[-2] == '/' && colon[-1] == '*' &&\n-\t       /* \"<mine>/<asterisk>:<yours>/<asterisk>\" is at least 7 bytes */\n-\t       7 <= len &&\n-\t       ref[len-2] == '/' && ref[len-1] == '*') )\n-\t\treturn 0 ;\n-\tcb.from_prefix = ref;\n-\tcb.from_prefix_len = colon - ref - 1;\n-\tcb.to_prefix = colon + 1;\n-\tcb.to_prefix_len = len - (colon - ref) - 2;\n-\tfor_each_ref(expand_wildcard_ref, &cb);\n-\treturn 1;\n-}\n-\n static void set_refspecs(const char **refs, int nr)\n {\n-\tif (nr) {\n-\t\tint i;\n-\t\tfor (i = 0; i < nr; i++) {\n-\t\t\tconst char *ref = refs[i];\n-\t\t\tif (!strcmp(\"tag\", ref)) {\n-\t\t\t\tchar *tag;\n-\t\t\t\tint len;\n-\t\t\t\tif (nr <= ++i)\n-\t\t\t\t\tdie(\"tag shorthand without <tag>\");\n-\t\t\t\tlen = strlen(refs[i]) + 11;\n-\t\t\t\ttag = xmalloc(len);\n-\t\t\t\tstrcpy(tag, \"refs/tags/\");\n-\t\t\t\tstrcat(tag, refs[i]);\n-\t\t\t\tref = tag;\n-\t\t\t}\n-\t\t\telse if (wildcard_ref(ref))\n-\t\t\t\tcontinue;\n-\t\t\tadd_refspec(ref);\n+\tint i;\n+\tfor (i = 0; i < nr; i++) {\n+\t\tconst char *ref = refs[i];\n+\t\tif (!strcmp(\"tag\", ref)) {\n+\t\t\tchar *tag;\n+\t\t\tint len;\n+\t\t\tif (nr <= ++i)\n+\t\t\t\tdie(\"tag shorthand without <tag>\");\n+\t\t\tlen = strlen(refs[i]) + 11;\n+\t\t\ttag = xmalloc(len);\n+\t\t\tstrcpy(tag, \"refs/tags/\");\n+\t\t\tstrcat(tag, refs[i]);\n+\t\t\tref = tag;\n \t\t}\n+\t\tadd_refspec(ref);\n \t}\n-\texpand_refspecs();\n }\n \n static int do_push(const char *repo)\n@@ -149,11 +59,9 @@ static int do_push(const char *repo)\n \t\tsprintf(rp, \"--receive-pack=%s\", remote->receivepack);\n \t\treceivepack = rp;\n \t}\n-\tif (!refspec && !all && !tags && remote->push_refspec_nr) {\n-\t\tfor (i = 0; i < remote->push_refspec_nr; i++) {\n-\t\t\tif (!wildcard_ref(remote->push_refspec[i]))\n-\t\t\t\tadd_refspec(remote->push_refspec[i]);\n-\t\t}\n+\tif (!refspec && !all && remote->push_refspec_nr) {\n+\t\trefspec = remote->push_refspec;\n+\t\trefspec_nr = remote->push_refspec_nr;\n \t}\n \n \targv = xmalloc((refspec_nr + 10) * sizeof(char *));\n@@ -240,7 +148,7 @@ int cmd_push(int argc, const char **argv, const char *prefix)\n \t\t\tcontinue;\n \t\t}\n \t\tif (!strcmp(arg, \"--tags\")) {\n-\t\t\ttags = 1;\n+\t\t\tadd_refspec(\"refs/tags/*\");\n \t\t\tcontinue;\n \t\t}\n \t\tif (!strcmp(arg, \"--force\") || !strcmp(arg, \"-f\")) {\n@@ -266,5 +174,8 @@ int cmd_push(int argc, const char **argv, const char *prefix)\n \t\tusage(push_usage);\n \t}\n \tset_refspecs(argv + i, argc - i);\n+\tif (all && refspec)\n+\t\tusage(push_usage);\n+\n \treturn do_push(repo);\n }\ndiff --git a/refs.c b/refs.c\nindex 2ae3235..cd63f37 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -603,15 +603,18 @@ int get_ref_sha1(const char *ref, unsigned char *sha1)\n \n static inline int bad_ref_char(int ch)\n {\n-\treturn (((unsigned) ch) <= ' ' ||\n-\t\tch == '~' || ch == '^' || ch == ':' ||\n-\t\t/* 2.13 Pattern Matching Notation */\n-\t\tch == '?' || ch == '*' || ch == '[');\n+\tif (((unsigned) ch) <= ' ' ||\n+\t    ch == '~' || ch == '^' || ch == ':')\n+\t\treturn 1;\n+\t/* 2.13 Pattern Matching Notation */\n+\tif (ch == '?' || ch == '*' || ch == '[')\n+\t\treturn 2;\n+\treturn 0;\n }\n \n int check_ref_format(const char *ref)\n {\n-\tint ch, level;\n+\tint ch, level, bad_type;\n \tconst char *cp = ref;\n \n \tlevel = 0;\n@@ -622,13 +625,19 @@ int check_ref_format(const char *ref)\n \t\t\treturn -1; /* should not end with slashes */\n \n \t\t/* we are at the beginning of the path component */\n-\t\tif (ch == '.' || bad_ref_char(ch))\n+\t\tif (ch == '.')\n \t\t\treturn -1;\n+\t\tbad_type = bad_ref_char(ch);\n+\t\tif (bad_type) {\n+\t\t\treturn (bad_type == 2 && !*cp) ? -3 : -1;\n+\t\t}\n \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\tbad_type = bad_ref_char(ch);\n+\t\t\tif (bad_type) {\n+\t\t\t\treturn (bad_type == 2 && !*cp) ? -3 : -1;\n+\t\t\t}\n \t\t\tif (ch == '/')\n \t\t\t\tbreak;\n \t\t\tif (ch == '.' && *cp == '.')\ndiff --git a/remote.c b/remote.c\nindex 46fe8d9..05b16ad 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -415,6 +415,10 @@ static int match_explicit_refs(struct ref *src, struct ref *dst,\n \t\tstruct ref *matched_src, *matched_dst;\n \n \t\tconst char *dst_value = rs[i].dst;\n+\n+\t\tif (rs[i].pattern)\n+\t\t\tcontinue;\n+\n \t\tif (dst_value == NULL)\n \t\t\tdst_value = rs[i].src;\n \n@@ -497,23 +501,48 @@ static struct ref *find_ref_by_name(struct ref *list, const char *name)\n \treturn NULL;\n }\n \n+static int check_pattern_match(struct refspec *rs, int rs_nr, struct ref *src)\n+{\n+\tint i;\n+\tif (!rs_nr)\n+\t\treturn 1;\n+\tfor (i = 0; i < rs_nr; i++) {\n+\t\tif (rs[i].pattern && !prefixcmp(src->name, rs[i].src))\n+\t\t\treturn 1;\n+\t}\n+\treturn 0;\n+}\n+\n int match_refs(struct ref *src, struct ref *dst, struct ref ***dst_tail,\n \t       int nr_refspec, char **refspec, int all)\n {\n \tstruct refspec *rs =\n \t\tparse_ref_spec(nr_refspec, (const char **) refspec);\n \n-\tif (nr_refspec)\n-\t\treturn match_explicit_refs(src, dst, dst_tail, rs, nr_refspec);\n+\tif (nr_refspec) {\n+\t\tif (match_explicit_refs(src, dst, dst_tail, rs, nr_refspec))\n+\t\t\treturn -1;\n+\t}\n \n \t/* pick the remainder */\n \tfor ( ; src; src = src->next) {\n \t\tstruct ref *dst_peer;\n \t\tif (src->peer_ref)\n \t\t\tcontinue;\n+\t\tif (!check_pattern_match(rs, nr_refspec, src))\n+\t\t\tcontinue;\n+\n \t\tdst_peer = find_ref_by_name(dst, src->name);\n-\t\tif ((dst_peer && dst_peer->peer_ref) || (!dst_peer && !all))\n+\t\tif (dst_peer && dst_peer->peer_ref) {\n+\t\t\t/* We're already sending something to this ref. */\n+\t\t\tcontinue;\n+\t\t}\n+\t\tif (!dst_peer && !nr_refspec && !all) {\n+\t\t\t/* Remote doesn't have it, and we have no\n+\t\t\t * explicit pattern, and we don't have\n+\t\t\t * --all. */\n \t\t\tcontinue;\n+\t\t}\n \t\tif (!dst_peer) {\n \t\t\t/* Create a new one and link it */\n \t\t\tint len = strlen(src->name) + 1;\ndiff --git a/send-pack.c b/send-pack.c\nindex 59352c8..697dbbc 100644\n--- a/send-pack.c\n+++ b/send-pack.c\n@@ -354,6 +354,7 @@ static void verify_remote_names(int nr_heads, char **heads)\n \t\tcase -2: /* ok but a single level -- that is fine for\n \t\t\t  * a match pattern.\n \t\t\t  */\n+\t\tcase -3: /* 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-- \n1.5.2.rc2.90.gc593-dirty\n"},{"id":"42374","messageId":"7vhcqcavn5.fsf@assigned-by-dhcp.cox.net","threadId":"8184","inReplyTo":"Pine.LNX.4.64.0705162217250.18541@iabervon.org","subject":"Re: [PATCH] Move refspec pattern matching to match_refs().","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-05-17T05:04:14Z","receivedAt":"2007-05-17T05:04:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Daniel Barkalow <barkalow@iabervon.org> writes:\n\n> This means that send-pack and http-push will support pattern refspecs,\n> so builtin-push.c doesn't have to expand them, and also git push can\n> just turn --tags into \"refs/tags/*\", further simplifying builtin-push.c\n\nNice.\n\n> @@ -266,5 +174,8 @@ int cmd_push(int argc, const char **argv, const char *prefix)\n>  \t\tusage(push_usage);\n>  \t}\n>  \tset_refspecs(argv + i, argc - i);\n> +\tif (all && refspec)\n> +\t\tusage(push_usage);\n> +\n>  \treturn do_push(repo);\n>  }\n\nIs this hunk an independent bugfix?  I think send-pack has its\nown check but I guess http-push lacked its own check?\n\n> diff --git a/refs.c b/refs.c\n> index 2ae3235..cd63f37 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -603,15 +603,18 @@ int get_ref_sha1(const char *ref, unsigned char *sha1)\n>  \n>  static inline int bad_ref_char(int ch)\n>  {\n> -\treturn (((unsigned) ch) <= ' ' ||\n> -\t\tch == '~' || ch == '^' || ch == ':' ||\n> -\t\t/* 2.13 Pattern Matching Notation */\n> -\t\tch == '?' || ch == '*' || ch == '[');\n> +\tif (((unsigned) ch) <= ' ' ||\n> +\t    ch == '~' || ch == '^' || ch == ':')\n> +\t\treturn 1;\n> +\t/* 2.13 Pattern Matching Notation */\n> +\tif (ch == '?' || ch == '*' || ch == '[')\n> +\t\treturn 2;\n> +\treturn 0;\n>  }\n>  \n>  int check_ref_format(const char *ref)\n>  {\n> -\tint ch, level;\n> +\tint ch, level, bad_type;\n>  \tconst char *cp = ref;\n>  \n>  \tlevel = 0;\n> @@ -622,13 +625,19 @@ int check_ref_format(const char *ref)\n>  \t\t\treturn -1; /* should not end with slashes */\n>  \n>  \t\t/* we are at the beginning of the path component */\n> -\t\tif (ch == '.' || bad_ref_char(ch))\n> +\t\tif (ch == '.')\n>  \t\t\treturn -1;\n> +\t\tbad_type = bad_ref_char(ch);\n> +\t\tif (bad_type) {\n> +\t\t\treturn (bad_type == 2 && !*cp) ? -3 : -1;\n> +\t\t}\n>  \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\tbad_type = bad_ref_char(ch);\n> +\t\t\tif (bad_type) {\n> +\t\t\t\treturn (bad_type == 2 && !*cp) ? -3 : -1;\n> +\t\t\t}\n>  \t\t\tif (ch == '/')\n>  \t\t\t\tbreak;\n>  \t\t\tif (ch == '.' && *cp == '.')\n> diff --git a/remote.c b/remote.c\n> index 46fe8d9..05b16ad 100644\n> --- a/remote.c\n> +++ b/remote.c\n>...\n> @@ -497,23 +501,48 @@ static struct ref *find_ref_by_name(struct ref *list, const char *name)\n>...\n>  int match_refs(struct ref *src, struct ref *dst, struct ref ***dst_tail,\n>  \t       int nr_refspec, char **refspec, int all)\n>  {\n>  \tstruct refspec *rs =\n>  \t\tparse_ref_spec(nr_refspec, (const char **) refspec);\n>  \n> -\tif (nr_refspec)\n> -\t\treturn match_explicit_refs(src, dst, dst_tail, rs, nr_refspec);\n> +\tif (nr_refspec) {\n> +\t\tif (match_explicit_refs(src, dst, dst_tail, rs, nr_refspec))\n> +\t\t\treturn -1;\n> +\t}\n\nStyle?  \"if (nr_refspec && match_explicit...)\" and then you can\nlose the excess braces.\n\n>  \n>  \t/* pick the remainder */\n>  \tfor ( ; src; src = src->next) {\n>  \t\tstruct ref *dst_peer;\n>  \t\tif (src->peer_ref)\n>  \t\t\tcontinue;\n> +\t\tif (!check_pattern_match(rs, nr_refspec, src))\n> +\t\t\tcontinue;\n> +\n>  \t\tdst_peer = find_ref_by_name(dst, src->name);\n> -\t\tif ((dst_peer && dst_peer->peer_ref) || (!dst_peer && !all))\n> +\t\tif (dst_peer && dst_peer->peer_ref) {\n> +\t\t\t/* We're already sending something to this ref. */\n> +\t\t\tcontinue;\n> +\t\t}\n> +\t\tif (!dst_peer && !nr_refspec && !all) {\n> +\t\t\t/* Remote doesn't have it, and we have no\n> +\t\t\t * explicit pattern, and we don't have\n> +\t\t\t * --all. */\n>  \t\t\tcontinue;\n> +\t\t}\n>  \t\tif (!dst_peer) {\n>  \t\t\t/* Create a new one and link it */\n>  \t\t\tint len = strlen(src->name) + 1;\n\nStyle?  Excess braces...\n\n> diff --git a/send-pack.c b/send-pack.c\n> index 59352c8..697dbbc 100644\n> --- a/send-pack.c\n> +++ b/send-pack.c\n> @@ -354,6 +354,7 @@ static void verify_remote_names(int nr_heads, char **heads)\n>  \t\tcase -2: /* ok but a single level -- that is fine for\n>  \t\t\t  * a match pattern.\n>  \t\t\t  */\n> +\t\tcase -3: /* 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\nI am not sure what is going on here.  Your new code returns -3\nwhen the pattern has any metacharacter at the end, and\nmetacharacter in the middle gives -1.  Does that mean the code\nwould say \"alright, that is a pattern\" when it sees \"refs/heads/foo[\"?\n\nI think we can go two ways.\n\n (1) Although the current code does not support it, the intent\n     for the globbing refspec \"refs/*:refs/remotes/origin/*\" was\n     to allow \"refs/heads/[a-z]*:refs/remotes/origin/[a-z]*\" (I\n     am not sure about the RHS, but it should be clear that what\n     is intended is \"grab only the ones that begin with [a-z]\n     and track\" in that example).  If we were to eventually do\n     this, I think check_ref_format() should probably be a bit\n     more careful when parsing glob() patterns (e.g. matching\n     bra-ket).\n     \n (2) As my uncertainty about the RHS above shows, we may not\n     support more general glob patterns and stay with only the\n     trailing \"/*\".  At least that is what we have now.  Maybe\n     check_ref_format should return \"good but ends with meta\"\n     only when the refspec consists of all good ref_char\n     followed by \"/*\" at the end.\n\nMy current preference is the latter.\n"},{"id":"42379","messageId":"Pine.LNX.4.64.0705170106270.18541@iabervon.org","threadId":"8184","inReplyTo":"7vhcqcavn5.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] Move refspec pattern matching to match_refs().","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2007-05-17T05:55:59Z","receivedAt":"2007-05-17T05:55:59Z","isPatch":true,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Wed, 16 May 2007, Junio C Hamano wrote:\n\n> Daniel Barkalow <barkalow@iabervon.org> writes:\n> \n> > This means that send-pack and http-push will support pattern refspecs,\n> > so builtin-push.c doesn't have to expand them, and also git push can\n> > just turn --tags into \"refs/tags/*\", further simplifying builtin-push.c\n> \n> Nice.\n> \n> > @@ -266,5 +174,8 @@ int cmd_push(int argc, const char **argv, const char *prefix)\n> >  \t\tusage(push_usage);\n> >  \t}\n> >  \tset_refspecs(argv + i, argc - i);\n> > +\tif (all && refspec)\n> > +\t\tusage(push_usage);\n> > +\n> >  \treturn do_push(repo);\n> >  }\n> \n> Is this hunk an independent bugfix?  I think send-pack has its\n> own check but I guess http-push lacked its own check?\n\nThis replaces the die() in expand_refspecs(), which was at the end of \nset_refspecs(). I think the idea is that \"git push --all foo bar\" isn't a \nconsistancy problem, but it suggests that the user is confused, and so the \nerror should be up front if there is one.\n\n> > diff --git a/refs.c b/refs.c\n> > index 2ae3235..cd63f37 100644\n> > --- a/refs.c\n> > +++ b/refs.c\n> > @@ -603,15 +603,18 @@ int get_ref_sha1(const char *ref, unsigned char *sha1)\n> >  \n> >  static inline int bad_ref_char(int ch)\n> >  {\n> > -\treturn (((unsigned) ch) <= ' ' ||\n> > -\t\tch == '~' || ch == '^' || ch == ':' ||\n> > -\t\t/* 2.13 Pattern Matching Notation */\n> > -\t\tch == '?' || ch == '*' || ch == '[');\n> > +\tif (((unsigned) ch) <= ' ' ||\n> > +\t    ch == '~' || ch == '^' || ch == ':')\n> > +\t\treturn 1;\n> > +\t/* 2.13 Pattern Matching Notation */\n> > +\tif (ch == '?' || ch == '*' || ch == '[')\n> > +\t\treturn 2;\n> > +\treturn 0;\n> >  }\n> >  \n> >  int check_ref_format(const char *ref)\n> >  {\n> > -\tint ch, level;\n> > +\tint ch, level, bad_type;\n> >  \tconst char *cp = ref;\n> >  \n> >  \tlevel = 0;\n> > @@ -622,13 +625,19 @@ int check_ref_format(const char *ref)\n> >  \t\t\treturn -1; /* should not end with slashes */\n> >  \n> >  \t\t/* we are at the beginning of the path component */\n> > -\t\tif (ch == '.' || bad_ref_char(ch))\n> > +\t\tif (ch == '.')\n> >  \t\t\treturn -1;\n> > +\t\tbad_type = bad_ref_char(ch);\n> > +\t\tif (bad_type) {\n> > +\t\t\treturn (bad_type == 2 && !*cp) ? -3 : -1;\n> > +\t\t}\n> >  \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\tbad_type = bad_ref_char(ch);\n> > +\t\t\tif (bad_type) {\n> > +\t\t\t\treturn (bad_type == 2 && !*cp) ? -3 : -1;\n> > +\t\t\t}\n> >  \t\t\tif (ch == '/')\n> >  \t\t\t\tbreak;\n> >  \t\t\tif (ch == '.' && *cp == '.')\n> > diff --git a/remote.c b/remote.c\n> > index 46fe8d9..05b16ad 100644\n> > --- a/remote.c\n> > +++ b/remote.c\n> >...\n> > @@ -497,23 +501,48 @@ static struct ref *find_ref_by_name(struct ref *list, const char *name)\n> >...\n> >  int match_refs(struct ref *src, struct ref *dst, struct ref ***dst_tail,\n> >  \t       int nr_refspec, char **refspec, int all)\n> >  {\n> >  \tstruct refspec *rs =\n> >  \t\tparse_ref_spec(nr_refspec, (const char **) refspec);\n> >  \n> > -\tif (nr_refspec)\n> > -\t\treturn match_explicit_refs(src, dst, dst_tail, rs, nr_refspec);\n> > +\tif (nr_refspec) {\n> > +\t\tif (match_explicit_refs(src, dst, dst_tail, rs, nr_refspec))\n> > +\t\t\treturn -1;\n> > +\t}\n> \n> Style?  \"if (nr_refspec && match_explicit...)\" and then you can\n> lose the excess braces.\n\nActually, just \"if (match_explicit(...))\" is fine. It'll do nothing and \nreturn 0 if !nr_refspec.\n\n> >  \t/* pick the remainder */\n> >  \tfor ( ; src; src = src->next) {\n> >  \t\tstruct ref *dst_peer;\n> >  \t\tif (src->peer_ref)\n> >  \t\t\tcontinue;\n> > +\t\tif (!check_pattern_match(rs, nr_refspec, src))\n> > +\t\t\tcontinue;\n> > +\n> >  \t\tdst_peer = find_ref_by_name(dst, src->name);\n> > -\t\tif ((dst_peer && dst_peer->peer_ref) || (!dst_peer && !all))\n> > +\t\tif (dst_peer && dst_peer->peer_ref) {\n> > +\t\t\t/* We're already sending something to this ref. */\n> > +\t\t\tcontinue;\n> > +\t\t}\n> > +\t\tif (!dst_peer && !nr_refspec && !all) {\n> > +\t\t\t/* Remote doesn't have it, and we have no\n> > +\t\t\t * explicit pattern, and we don't have\n> > +\t\t\t * --all. */\n> >  \t\t\tcontinue;\n> > +\t\t}\n> >  \t\tif (!dst_peer) {\n> >  \t\t\t/* Create a new one and link it */\n> >  \t\t\tint len = strlen(src->name) + 1;\n> \n> Style?  Excess braces...\n\nA comment doesn't count as a second \"thing\" to be in a conditional for the \npurposes of style? Multiple equally-indented lines without braces \ndistracts me with thinking that the actual statement might be misindented.\n\n> I am not sure what is going on here.  Your new code returns -3\n> when the pattern has any metacharacter at the end, and\n> metacharacter in the middle gives -1.  Does that mean the code\n> would say \"alright, that is a pattern\" when it sees \"refs/heads/foo[\"?\n>\n> I think we can go two ways.\n> \n>  (1) Although the current code does not support it, the intent\n>      for the globbing refspec \"refs/*:refs/remotes/origin/*\" was\n>      to allow \"refs/heads/[a-z]*:refs/remotes/origin/[a-z]*\" (I\n>      am not sure about the RHS, but it should be clear that what\n>      is intended is \"grab only the ones that begin with [a-z]\n>      and track\" in that example).  If we were to eventually do\n>      this, I think check_ref_format() should probably be a bit\n>      more careful when parsing glob() patterns (e.g. matching\n>      bra-ket).\n>      \n>  (2) As my uncertainty about the RHS above shows, we may not\n>      support more general glob patterns and stay with only the\n>      trailing \"/*\".  At least that is what we have now.  Maybe\n>      check_ref_format should return \"good but ends with meta\"\n>      only when the refspec consists of all good ref_char\n>      followed by \"/*\" at the end.\n> \n> My current preference is the latter.\n\nThe latter is probably the way to go for now. But as far as I can tell, \nrefs/heads/db-*:refs/heads/* is currently supported, too.\n\nSo, for (2), I'd make bad_ref_char only return 2 for '*', and return 1 for \n'?' and '['.\n\nWe can let more stuff get through if we make the parser able to parse it.\n\n\t-Daniel\n*This .sig left intentionally blank*\n"}]}