{"thread":{"id":"12785","subject":"[PATCH] Permit refspec source side to parse as a sha1","startedAt":"2008-03-21T00:54:54Z","lastAt":"2008-03-26T06:22:32Z","messageCount":20,"participants":["Daniel Barkalow","Junio C Hamano","Linus Torvalds","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"72546","messageId":"alpine.LNX.1.00.0803202049090.19665@iabervon.org","threadId":"12785","inReplyTo":null,"subject":"[PATCH] Permit refspec source side to parse as a sha1","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2008-03-21T00:54:54Z","receivedAt":"2008-03-21T00:54:54Z","isPatch":true,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"This fixes \"git push origin HEAD~1:foo\". \"git fetch origin HEAD~1:foo\"\nwill report \"Couldn't find remote ref HEAD~1\", while\n\"git fetch origin HEAD**1:foo\" reports \"Invalid refspec 'HEAD**1:foo'\"\n\nThat is, HEAD~1 is something you're not allowed to ask the remote for, \nwhile HEAD**1 doesn't mean anything.\n\nSigned-off-by: Daniel Barkalow <barkalow@iabervon.org>\n---\nNote that this actually tries to look up the source side, so \"git \nfetch origin HEAD^3:foo\" usually gives a wrongish error message. But this \nonly applies to error cases which nobody is likely to attempt anyway, and \nthey still come out as errors regardless.\n\n remote.c |    4 +++-\n 1 files changed, 3 insertions(+), 1 deletions(-)\n\ndiff --git a/remote.c b/remote.c\nindex 9700a33..d737579 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -434,8 +434,10 @@ struct refspec *parse_ref_spec(int nr_refspec, const char **refspec)\n \t\trs[i].src = xstrndup(sp, ep - sp);\n \n \t\tif (*rs[i].src) {\n+\t\t\tunsigned char sha1[20];\n \t\t\tst = check_ref_format(rs[i].src);\n-\t\t\tif (st && st != CHECK_REF_FORMAT_ONELEVEL)\n+\t\t\tif (st && st != CHECK_REF_FORMAT_ONELEVEL &&\n+\t\t\t    get_sha1(rs[i].src, sha1))\n \t\t\t\tdie(\"Invalid refspec '%s'\", refspec[i]);\n \t\t}\n \t\tif (rs[i].dst && *rs[i].dst) {\n-- \n1.5.4.3.610.gea6cd\n"},{"id":"72558","messageId":"7v4pb0vhrg.fsf@gitster.siamese.dyndns.org","threadId":"12785","inReplyTo":"alpine.LNX.1.00.0803202049090.19665@iabervon.org","subject":"Re: [PATCH] Permit refspec source side to parse as a sha1","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-03-21T04:10:59Z","receivedAt":"2008-03-21T04:10:59Z","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 fixes \"git push origin HEAD~1:foo\". \"git fetch origin HEAD~1:foo\"\n> will report \"Couldn't find remote ref HEAD~1\", while\n> \"git fetch origin HEAD**1:foo\" reports \"Invalid refspec 'HEAD**1:foo'\"\n>\n> That is, HEAD~1 is something you're not allowed to ask the remote for, \n> while HEAD**1 doesn't mean anything.\n>\n> Signed-off-by: Daniel Barkalow <barkalow@iabervon.org>\n> ---\n> Note that this actually tries to look up the source side, so \"git \n> fetch origin HEAD^3:foo\" usually gives a wrongish error message.\n\nThis is very wrong.  When you do not have anything on your own (i.e. HEAD\npoints at an unborn branch), \"git fetch origin HEAD:master\" would barf?\nWe should _never_ look at the refs we have; the check is about the syntax.\n\nWhy are fetch and push refspec parsing code share the same function and\nenforce exactly the same error checking to begin with?  Is it because we\nare too lazy to implement two semantics that has to be different and try\nto \"share\" code by only implementing checks for \"common denominator\"?\n\nI think that is the root of the problem, because their syntax are designed\nto look similar, but their semantics are different [*1*].\n\nFor a fetch refspec, LHS is the remote end and you do not even know what\nrefs are available over there; the _only_ thing you care about is that it\nis well formed.\n\nOn the other hand, for a push refspec, LHS is the local end and _must_\nname a valid commit you are pushing (unless you are pushing empty to\ndelete the ref).\n\nI really think we cannot afford piling hacks on top of hacks to hide the\nbroken interface forever.  We have two different things to validate, and\nthe callers all know what they have when calling us to validate.  We\nshould not have a single loose validation that only catches \"it cannot be\neither fetch nor push refspec\" breakage.\n\n        As a side note, I have a vague recollection that we used to treat\n        a refspec from the command line and a refspec from the config\n        differently in some contexts, and rules applied to them might have\n        been different.  I do not think of what the context was offhand,\n        but we might need to further split the implementation of the\n        rules.\n\nBut at least I think something like this patch would lead us in the right\ndirection.\n\n---\n\n builtin-fetch.c |    3 +-\n remote.c        |  114 +++++++++++++++++++++++++++++++++++-------------------\n remote.h        |    3 +-\n 3 files changed, 78 insertions(+), 42 deletions(-)\n\ndiff --git a/builtin-fetch.c b/builtin-fetch.c\nindex b2b9935..a11548c 100644\n--- a/builtin-fetch.c\n+++ b/builtin-fetch.c\n@@ -652,5 +652,6 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \n \tsignal(SIGINT, unlock_pack_on_signal);\n \tatexit(unlock_pack);\n-\treturn do_fetch(transport, parse_ref_spec(ref_nr, refs), ref_nr);\n+\treturn do_fetch(transport,\n+\t\t\tparse_fetch_refspec(ref_nr, refs), ref_nr);\n }\ndiff --git a/remote.c b/remote.c\nindex 9700a33..3225f07 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -393,58 +393,92 @@ static void read_config(void)\n \talias_all_urls();\n }\n \n-struct refspec *parse_ref_spec(int nr_refspec, const char **refspec)\n+static struct refspec *parse_refspec_internal(int nr_refspec, const char **refspec, int fetch)\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\tconst char *sp, *ep, *gp;\n-\t\tsp = refspec[i];\n-\t\tif (*sp == '+') {\n+\t\tsize_t llen, rlen;\n+\t\tint is_glob;\n+\t\tconst char *lhs, *rhs;\n+\n+\t\tllen = rlen = is_glob = 0;\n+\n+\t\tlhs = refspec[i];\n+\t\tif (*lhs == '+') {\n \t\t\trs[i].force = 1;\n-\t\t\tsp++;\n+\t\t\tlhs++;\n \t\t}\n-\t\tgp = strstr(sp, \"/*\");\n-\t\tep = strchr(sp, ':');\n-\t\tif (gp && ep && gp > ep)\n-\t\t\tgp = NULL;\n-\t\tif (ep) {\n-\t\t\tif (ep[1]) {\n-\t\t\t\tconst char *glob = strstr(ep + 1, \"/*\");\n-\t\t\t\tif (glob && glob[2])\n-\t\t\t\t\tglob = NULL;\n-\t\t\t\tif (!glob)\n-\t\t\t\t\tgp = NULL;\n-\t\t\t\tif (gp)\n-\t\t\t\t\trs[i].dst = xstrndup(ep + 1,\n-\t\t\t\t\t\t\t     glob - ep - 1);\n-\t\t\t\telse\n-\t\t\t\t\trs[i].dst = xstrdup(ep + 1);\n-\t\t\t}\n+\n+\t\trhs = strrchr(lhs, ':');\n+\t\tif (rhs && *++rhs) {\n+\t\t\t/* Has RHS */\n+\t\t\trlen = strlen(rhs);\n+\t\t\tis_glob = (2 <= rlen && !strcmp(rhs + rlen - 2, \"/*\"));\n \t\t} else {\n-\t\t\tep = sp + strlen(sp);\n+\t\t\trhs = NULL;\n \t\t}\n-\t\tif (gp && gp + 2 != ep)\n-\t\t\tgp = NULL;\n-\t\tif (gp) {\n-\t\t\trs[i].pattern = 1;\n-\t\t\tep = gp;\n+\n+\t\tllen = (rhs ? (rhs - lhs - 1) : strlen(lhs));\n+\t\tif (is_glob != (2 <= llen && !memcmp(lhs + llen - 2, \"/*\", 2)))\n+\t\t\tgoto invalid;\n+\n+\t\tif (is_glob) {\n+\t\t\tllen -= 2;\n+\t\t\trlen -= 2;\n \t\t}\n-\t\trs[i].src = xstrndup(sp, ep - sp);\n+\t\trs[i].pattern = is_glob;\n+\t\tif (rlen)\n+\t\t\trs[i].dst = xstrndup(rhs, rlen);\n+\t\trs[i].src = xstrndup(lhs, llen);\n \n-\t\tif (*rs[i].src) {\n+\t\t/*\n+\t\t * Do we want to validate LHS?\n+\t\t *\n+\t\t * Fetch refspec must have LHS that names a valid\n+\t\t * looking ref (unless it is empty, which defaults to\n+\t\t * HEAD).\n+\t\t *\n+\t\t * Push refspec \"LHS\" without \":RHS\" is a synonym for\n+\t\t * \"LHS:LHS\", and LHS must be a valid looking ref.\n+\t\t *\n+\t\t * For push with explicit RHS, LHS can be any valid\n+\t\t * extended SHA-1 expression (or even \"empty\" for\n+\t\t * removal) and we cannot check for error until it\n+\t\t * actually gets used.\n+\t\t */\n+\t\tif ((fetch && *rs[i].src) || (!fetch && !rhs)) {\n \t\t\tst = check_ref_format(rs[i].src);\n \t\t\tif (st && st != CHECK_REF_FORMAT_ONELEVEL)\n-\t\t\t\tdie(\"Invalid refspec '%s'\", refspec[i]);\n+\t\t\t\tgoto invalid;\n \t\t}\n-\t\tif (rs[i].dst && *rs[i].dst) {\n+\n+\t\t/*\n+\t\t * RHS: both push destination and fetch tracking ref\n+\t\t * must name a valid looking ref.\n+\t\t */\n+\t\tif (rs[i].dst) {\n \t\t\tst = check_ref_format(rs[i].dst);\n \t\t\tif (st && st != CHECK_REF_FORMAT_ONELEVEL)\n-\t\t\t\tdie(\"Invalid refspec '%s'\", refspec[i]);\n+\t\t\t\tgoto invalid;\n \t\t}\n \t}\n \treturn rs;\n+\n+ invalid:\n+\tdie(\"Invalid refspec '%s'\", refspec[i]);\n+}\n+\n+struct refspec *parse_fetch_refspec(int nr_refspec, const char **refspec)\n+{\n+\treturn parse_refspec_internal(nr_refspec, refspec, 1);\n+}\n+\n+struct refspec *parse_push_refspec(int nr_refspec, const char **refspec)\n+{\n+\treturn parse_refspec_internal(nr_refspec, refspec, 0);\n }\n \n static int valid_remote_nick(const char *name)\n@@ -475,8 +509,8 @@ struct remote *remote_get(const char *name)\n \t\tadd_url_alias(ret, name);\n \tif (!ret->url)\n \t\treturn NULL;\n-\tret->fetch = parse_ref_spec(ret->fetch_refspec_nr, ret->fetch_refspec);\n-\tret->push = parse_ref_spec(ret->push_refspec_nr, ret->push_refspec);\n+\tret->fetch = parse_fetch_refspec(ret->fetch_refspec_nr, ret->fetch_refspec);\n+\tret->push = parse_push_refspec(ret->push_refspec_nr, ret->push_refspec);\n \treturn ret;\n }\n \n@@ -489,11 +523,11 @@ int for_each_remote(each_remote_fn fn, void *priv)\n \t\tif (!r)\n \t\t\tcontinue;\n \t\tif (!r->fetch)\n-\t\t\tr->fetch = parse_ref_spec(r->fetch_refspec_nr,\n-\t\t\t\t\tr->fetch_refspec);\n+\t\t\tr->fetch = parse_fetch_refspec(r->fetch_refspec_nr,\n+\t\t\t\t\t\t       r->fetch_refspec);\n \t\tif (!r->push)\n-\t\t\tr->push = parse_ref_spec(r->push_refspec_nr,\n-\t\t\t\t\tr->push_refspec);\n+\t\t\tr->push = parse_push_refspec(r->push_refspec_nr,\n+\t\t\t\t\t\t     r->push_refspec);\n \t\tresult = fn(r, priv);\n \t}\n \treturn result;\n@@ -824,7 +858,7 @@ int match_refs(struct ref *src, struct ref *dst, struct ref ***dst_tail,\n \t       int nr_refspec, const char **refspec, int flags)\n {\n \tstruct refspec *rs =\n-\t\tparse_ref_spec(nr_refspec, (const char **) refspec);\n+\t\tparse_push_refspec(nr_refspec, (const char **) refspec);\n \tint send_all = flags & MATCH_REFS_ALL;\n \tint send_mirror = flags & MATCH_REFS_MIRROR;\n \ndiff --git a/remote.h b/remote.h\nindex f1dedf1..7e9ae79 100644\n--- a/remote.h\n+++ b/remote.h\n@@ -67,7 +67,8 @@ void free_refs(struct ref *ref);\n  */\n void ref_remove_duplicates(struct ref *ref_map);\n \n-struct refspec *parse_ref_spec(int nr_refspec, const char **refspec);\n+struct refspec *parse_fetch_refspec(int nr_refspec, const char **refspec);\n+struct refspec *parse_push_refspec(int nr_refspec, const char **refspec);\n \n int match_refs(struct ref *src, struct ref *dst, struct ref ***dst_tail,\n \t       int nr_refspec, const char **refspec, int all);\n"},{"id":"72565","messageId":"7vabksu1ct.fsf@gitster.siamese.dyndns.org","threadId":"12785","inReplyTo":"7v4pb0vhrg.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] Permit refspec source side to parse as a sha1","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-03-21T04:50:42Z","receivedAt":"2008-03-21T04:50:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> I really think we cannot afford piling hacks on top of hacks to hide the\n> broken interface forever.  We have two different things to validate, and\n> the callers all know what they have when calling us to validate.  We\n> should not have a single loose validation that only catches \"it cannot be\n> either fetch nor push refspec\" breakage.\n> ...\n> But at least I think something like this patch would lead us in the right\n> direction.\n\nOn top of the one I sent out, with this patch, you can:\n\n\t$ git push other ':/remote show:refs/heads/new'\n\nto send the commit you would review with \"git show ':/remote show'\".\n\n---\n builtin-send-pack.c |    2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/builtin-send-pack.c b/builtin-send-pack.c\nindex 930e0fb..bb9c33a 100644\n--- a/builtin-send-pack.c\n+++ b/builtin-send-pack.c\n@@ -537,7 +537,7 @@ static void verify_remote_names(int nr_heads, const char **heads)\n \tint i;\n \n \tfor (i = 0; i < nr_heads; i++) {\n-\t\tconst char *remote = strchr(heads[i], ':');\n+\t\tconst char *remote = strrchr(heads[i], ':');\n \n \t\tremote = remote ? (remote + 1) : heads[i];\n \t\tswitch (check_ref_format(remote)) {\n"},{"id":"72568","messageId":"alpine.LNX.1.00.0803210014100.19665@iabervon.org","threadId":"12785","inReplyTo":"7v4pb0vhrg.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] Permit refspec source side to parse as a sha1","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2008-03-21T05:09:24Z","receivedAt":"2008-03-21T05:09:24Z","isPatch":true,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Thu, 20 Mar 2008, Junio C Hamano wrote:\n\n> Daniel Barkalow <barkalow@iabervon.org> writes:\n> \n> > This fixes \"git push origin HEAD~1:foo\". \"git fetch origin HEAD~1:foo\"\n> > will report \"Couldn't find remote ref HEAD~1\", while\n> > \"git fetch origin HEAD**1:foo\" reports \"Invalid refspec 'HEAD**1:foo'\"\n> >\n> > That is, HEAD~1 is something you're not allowed to ask the remote for, \n> > while HEAD**1 doesn't mean anything.\n> >\n> > Signed-off-by: Daniel Barkalow <barkalow@iabervon.org>\n> > ---\n> > Note that this actually tries to look up the source side, so \"git \n> > fetch origin HEAD^3:foo\" usually gives a wrongish error message.\n> \n> This is very wrong.  When you do not have anything on your own (i.e. HEAD\n> points at an unborn branch), \"git fetch origin HEAD:master\" would barf?\n\nNo, HEAD is syntactically valid as a ref, so it's accepted regardless of \nanything else.\n\n> We should _never_ look at the refs we have; the check is about the syntax.\n\nUnfortunately, there's not (so far as I know) a handy way to check the \nsyntax of a sha1 name.\n\n> Why are fetch and push refspec parsing code share the same function and\n> enforce exactly the same error checking to begin with?  Is it because we\n> are too lazy to implement two semantics that has to be different and try\n> to \"share\" code by only implementing checks for \"common denominator\"?\n\nWe enforce additional, purpose-specific error-checking after parsing, when \nwe're actually trying to use the refspecs. The problem is that we can't \nmake the syntactic check sufficient lenient to cover all valid syntax, but \ntight enough to prohibit any syntax errors, without getting tangled in a \nbit of semantics.\n\n> I think that is the root of the problem, because their syntax are designed\n> to look similar, but their semantics are different [*1*].\n\nThe syntax is identical. The semantics is not the parser's problem, and is \nmore appropriate for the callers to handle.\n\n> For a fetch refspec, LHS is the remote end and you do not even know what\n> refs are available over there; the _only_ thing you care about is that it\n> is well formed.\n> \n> On the other hand, for a push refspec, LHS is the local end and _must_\n> name a valid commit you are pushing (unless you are pushing empty to\n> delete the ref).\n\nNot really. If you're fetching, and you have a push refspec configured for \nthe same ref, and that refspec has a LHS which, for the current HEAD, \ndoesn't refer to a valid commit at the moment, that's fine. It's only a \nproblem if you try to use it while it doesn't point at a valid commit, or \nif it isn't valid syntax for a sha1 name (like HEAD*2).\n\n> I really think we cannot afford piling hacks on top of hacks to hide the\n> broken interface forever.  We have two different things to validate, and\n> the callers all know what they have when calling us to validate.  We\n> should not have a single loose validation that only catches \"it cannot be\n> either fetch nor push refspec\" breakage.\n\nSo we should call the same code to parse the string, have that code do no \nvalidation at all, and have the caller validate the return as appropriate. \nThe parsing doesn't depend at all on whether it's for fetching or pushing.\n\n>         As a side note, I have a vague recollection that we used to treat\n>         a refspec from the command line and a refspec from the config\n>         differently in some contexts, and rules applied to them might have\n>         been different.  I do not think of what the context was offhand,\n>         but we might need to further split the implementation of the\n>         rules.\n\nI think it was that we used to break \"push = HEAD\", but we support it now.\n\n\t-Daniel\n*This .sig left intentionally blank*\n"},{"id":"72570","messageId":"7vmyosskyu.fsf@gitster.siamese.dyndns.org","threadId":"12785","inReplyTo":"alpine.LNX.1.00.0803210014100.19665@iabervon.org","subject":"Re: [PATCH] Permit refspec source side to parse as a sha1","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-03-21T05:30:01Z","receivedAt":"2008-03-21T05:30:01Z","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> So we should call the same code to parse the string, have that code do no \n> validation at all, and have the caller validate the return as appropriate. \n> The parsing doesn't depend at all on whether it's for fetching or pushing.\n\nObviously you did not read my patch before responding.\n\nWhile I would agree that removing the check from parsing and add necessary\nchecks to all callers would be another way to catch the same kind of\nbreakages,\n\n (1) it would be more code to change, and I do not see a clear advantage,\n     in that approach, especially given the discussion that lead to\n     $gmane/77413;\n\n (2) the error reporting by the callers that use the parsed result will\n     not be able to say \"Invalid refspec '%s'\" on the source string,\n     simply because the source string is not available to them anymore;\n\n (3) worse yet, the older code even discarded part of the user input, so\n     the error reporting that uses the parsed src/dst pair may not even be\n     similar to the problematic input the user gave us to begin with,\n     making it harder for the user to spot what we did not like and\n     correct it.\n\nIn any case, don't you agree that the patch you responded to is much\neasier to understand what we are (and we are not) checking than the\noriginal code?\n"},{"id":"72573","messageId":"alpine.LNX.1.00.0803210134070.19665@iabervon.org","threadId":"12785","inReplyTo":"7vmyosskyu.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] Permit refspec source side to parse as a sha1","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2008-03-21T05:57:52Z","receivedAt":"2008-03-21T05:57:52Z","isPatch":true,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Thu, 20 Mar 2008, Junio C Hamano wrote:\n\n> Daniel Barkalow <barkalow@iabervon.org> writes:\n> \n> > So we should call the same code to parse the string, have that code do no \n> > validation at all, and have the caller validate the return as appropriate. \n> > The parsing doesn't depend at all on whether it's for fetching or pushing.\n> \n> Obviously you did not read my patch before responding.\n> \n> While I would agree that removing the check from parsing and add necessary\n> checks to all callers would be another way to catch the same kind of\n> breakages,\n> \n>  (1) it would be more code to change, and I do not see a clear advantage,\n>      in that approach, especially given the discussion that lead to\n>      $gmane/77413;\n\nSurely it's no more code to change all of the callers of parse_ref_spec to \nalso call a function to validate it as appropriate than it is to change \nall of the callers of parse_ref_spec to call a different function instead.\n \n>  (2) the error reporting by the callers that use the parsed result will\n>      not be able to say \"Invalid refspec '%s'\" on the source string,\n>      simply because the source string is not available to them anymore;\n\nThat's easy enough to keep, and likely even worthwhile; it would probably \nbe nice, for example, to have \"git fetch origin typo/*:something/*\" give \nan error when the pattern doesn't match anything, and that error would \nneed the refspec string.\n\n>  (3) worse yet, the older code even discarded part of the user input, so\n>      the error reporting that uses the parsed src/dst pair may not even be\n>      similar to the problematic input the user gave us to begin with,\n>      making it harder for the user to spot what we did not like and\n>      correct it.\n\nThat was a bug that was fixed in the intermediate version.\n\n> In any case, don't you agree that the patch you responded to is much\n> easier to understand what we are (and we are not) checking than the\n> original code?\n\nNo, I think it's much more complicated. It mixes the semantics of what an \nempty side means for a particular use of refspecs into the parsing of the \nstring. At the very least, the checks should be outside of _internal() in \nthe functions for particular uses.\n\n\t-Daniel\n*This .sig left intentionally blank*\n"},{"id":"72575","messageId":"7v3aqksic6.fsf@gitster.siamese.dyndns.org","threadId":"12785","inReplyTo":"alpine.LNX.1.00.0803210134070.19665@iabervon.org","subject":"Re: [PATCH] Permit refspec source side to parse as a sha1","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-03-21T06:26:49Z","receivedAt":"2008-03-21T06:26:49Z","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> On Thu, 20 Mar 2008, Junio C Hamano wrote:\n> ...\n>> In any case, don't you agree that the patch you responded to is much\n>> easier to understand what we are (and we are not) checking than the\n>> original code?\n>\n> No, I think it's much more complicated. It mixes the semantics of what an \n> empty side means for a particular use of refspecs into the parsing of the \n> string. At the very least, the checks should be outside of _internal() in \n> the functions for particular uses.\n\nThe thing is, the syntax is the same between fetch and push only to a\ndegree.  They are both <LHS> ':' <RHS>.  What is allowed on LHS and RHS\nare quite different even at the syntactic level.\n\nI already know our criteria when judging if a particular code is clean or\ncomplex are _vastly_ different, from the experience working with you in\nother parts of the system (namely, read-tree 3-way rules and\nunpack_trees() rewrite that happened quite a long time ago).\n\nWhile I would note that you thought my version is more complex to read, I\nwould not argue about this issue with you anymore, except saying that I\nstrongly disagree.\n"},{"id":"72600","messageId":"alpine.LNX.1.00.0803211148120.19665@iabervon.org","threadId":"12785","inReplyTo":"7v3aqksic6.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] Permit refspec source side to parse as a sha1","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2008-03-21T16:08:47Z","receivedAt":"2008-03-21T16:08:47Z","isPatch":true,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Thu, 20 Mar 2008, Junio C Hamano wrote:\n\n> Daniel Barkalow <barkalow@iabervon.org> writes:\n> \n> > On Thu, 20 Mar 2008, Junio C Hamano wrote:\n> > ...\n> >> In any case, don't you agree that the patch you responded to is much\n> >> easier to understand what we are (and we are not) checking than the\n> >> original code?\n> >\n> > No, I think it's much more complicated. It mixes the semantics of what an \n> > empty side means for a particular use of refspecs into the parsing of the \n> > string. At the very least, the checks should be outside of _internal() in \n> > the functions for particular uses.\n> \n> The thing is, the syntax is the same between fetch and push only to a\n> degree.  They are both <LHS> ':' <RHS>.  What is allowed on LHS and RHS\n> are quite different even at the syntactic level.\n\nThere's also the optional + at the beginning and the way of forming \npatterns, and the need to distinguish not having a colon with not having \nanything on one or the other side of the colon.\n\n> I already know our criteria when judging if a particular code is clean or\n> complex are _vastly_ different, from the experience working with you in\n> other parts of the system (namely, read-tree 3-way rules and\n> unpack_trees() rewrite that happened quite a long time ago).\n\nAgreed.\n\nAnd I do think that the rewrite of the part of the function before the \nsemantic checks is clearer than the corresponding original (although the \npatch text made it hard to see; we really need some magic to make diff not \nbother to save 4 identifier-free lines and just show it as a complete \nreplacement).\n\n\t-Daniel\n*This .sig left intentionally blank*\n"},{"id":"72629","messageId":"7vlk4boh6v.fsf@gitster.siamese.dyndns.org","threadId":"12785","inReplyTo":"alpine.LNX.1.00.0803211148120.19665@iabervon.org","subject":"[PATCH] remote.c: Fix overtight refspec validation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-03-21T22:17:28Z","receivedAt":"2008-03-21T22:17:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"We tightened the refspec validation code in an earlier ef00d15 (Tighten\nrefspec processing, 2008-03-17) per my suggestion, but the suggestion was\nmisguided to begin with and it broke this usage:\n\n    $ git push origin HEAD~12:master\n\nThe syntax of push refspecs and fetch refspecs are similar in that they\nare both colon separated LHS and RHS (possibly prefixed with a + to\nforce), but the similarity ends there.  For example, LHS in a push refspec\ncan be anything that evaluates to a valid object name at runtime (except\nwhen colon and RHS is missing, or it is a glob), while it must be a\nvalid-looking refname in a fetch refspec.  To validate them correctly, the\ncaller needs to be able to say which kind of refspecs they are.  It is\nunreasonable to keep a single interface that cannot tell which kind it is\ndealing with, and ask it to behave sensibly.\n\nThis commit separates the parsing of the two into different functions, and\nclarifies the code to implement the parsing proper (i.e. splitting into\ntwo parts, making sure both sides are wildcard or neither side is).\n\nThis happens to also allow pushing a commit named with the esoteric \"look\nfor that string\" syntax:\n\n    $ git push ../test.git ':/remote.c: Fix overtight refspec:master'\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n\n  Daniel Barkalow <barkalow@iabervon.org> writes:\n\n  > ... (although the \n  > patch text made it hard to see; we really need some magic to make diff not \n  > bother to save 4 identifier-free lines and just show it as a complete \n  > replacement).\n\n  Yeah, I tend to agree and some people seem to favor \"diff -c\" for this\n  reason.\n\n  In any case, here is an updated patch with a better description and\n  necessary tests.  The \"validly looking extended SHA-1 expression\" check\n  is wholly outside the scope of 1.5.5 regression fix so I did not even\n  attempt to address it, but what this commit fixes needs to be addressed\n  before -rc1.\n\n builtin-fetch.c     |    3 +-\n builtin-send-pack.c |    2 +-\n remote.c            |  122 ++++++++++++++++++++++++++++++++++-----------------\n remote.h            |    3 +-\n t/t5511-refspec.sh  |   67 ++++++++++++++++++++++++++++\n 5 files changed, 153 insertions(+), 44 deletions(-)\n create mode 100755 t/t5511-refspec.sh\n\ndiff --git a/builtin-fetch.c b/builtin-fetch.c\nindex b2b9935..a11548c 100644\n--- a/builtin-fetch.c\n+++ b/builtin-fetch.c\n@@ -652,5 +652,6 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \n \tsignal(SIGINT, unlock_pack_on_signal);\n \tatexit(unlock_pack);\n-\treturn do_fetch(transport, parse_ref_spec(ref_nr, refs), ref_nr);\n+\treturn do_fetch(transport,\n+\t\t\tparse_fetch_refspec(ref_nr, refs), ref_nr);\n }\ndiff --git a/builtin-send-pack.c b/builtin-send-pack.c\nindex 930e0fb..bb9c33a 100644\n--- a/builtin-send-pack.c\n+++ b/builtin-send-pack.c\n@@ -537,7 +537,7 @@ static void verify_remote_names(int nr_heads, const char **heads)\n \tint i;\n \n \tfor (i = 0; i < nr_heads; i++) {\n-\t\tconst char *remote = strchr(heads[i], ':');\n+\t\tconst char *remote = strrchr(heads[i], ':');\n \n \t\tremote = remote ? (remote + 1) : heads[i];\n \t\tswitch (check_ref_format(remote)) {\ndiff --git a/remote.c b/remote.c\nindex 9700a33..4117bfc 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -393,58 +393,98 @@ static void read_config(void)\n \talias_all_urls();\n }\n \n-struct refspec *parse_ref_spec(int nr_refspec, const char **refspec)\n+static struct refspec *parse_refspec_internal(int nr_refspec, const char **refspec, int fetch)\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\tconst char *sp, *ep, *gp;\n-\t\tsp = refspec[i];\n-\t\tif (*sp == '+') {\n+\t\tsize_t llen, rlen;\n+\t\tint is_glob;\n+\t\tconst char *lhs, *rhs;\n+\n+\t\tllen = rlen = is_glob = 0;\n+\n+\t\tlhs = refspec[i];\n+\t\tif (*lhs == '+') {\n \t\t\trs[i].force = 1;\n-\t\t\tsp++;\n+\t\t\tlhs++;\n \t\t}\n-\t\tgp = strstr(sp, \"/*\");\n-\t\tep = strchr(sp, ':');\n-\t\tif (gp && ep && gp > ep)\n-\t\t\tgp = NULL;\n-\t\tif (ep) {\n-\t\t\tif (ep[1]) {\n-\t\t\t\tconst char *glob = strstr(ep + 1, \"/*\");\n-\t\t\t\tif (glob && glob[2])\n-\t\t\t\t\tglob = NULL;\n-\t\t\t\tif (!glob)\n-\t\t\t\t\tgp = NULL;\n-\t\t\t\tif (gp)\n-\t\t\t\t\trs[i].dst = xstrndup(ep + 1,\n-\t\t\t\t\t\t\t     glob - ep - 1);\n-\t\t\t\telse\n-\t\t\t\t\trs[i].dst = xstrdup(ep + 1);\n-\t\t\t}\n-\t\t} else {\n-\t\t\tep = sp + strlen(sp);\n+\n+\t\trhs = strrchr(lhs, ':');\n+\t\tif (rhs) {\n+\t\t\trhs++;\n+\t\t\trlen = strlen(rhs);\n+\t\t\tis_glob = (2 <= rlen && !strcmp(rhs + rlen - 2, \"/*\"));\n+\t\t\trs[i].dst = xstrndup(rhs, rlen - is_glob * 2);\n \t\t}\n-\t\tif (gp && gp + 2 != ep)\n-\t\t\tgp = NULL;\n-\t\tif (gp) {\n-\t\t\trs[i].pattern = 1;\n-\t\t\tep = gp;\n+\n+\t\tllen = (rhs ? (rhs - lhs - 1) : strlen(lhs));\n+\t\tif (is_glob != (2 <= llen && !memcmp(lhs + llen - 2, \"/*\", 2)))\n+\t\t\tgoto invalid;\n+\n+\t\tif (is_glob) {\n+\t\t\tllen -= 2;\n+\t\t\trlen -= 2;\n \t\t}\n-\t\trs[i].src = xstrndup(sp, ep - sp);\n+\t\trs[i].pattern = is_glob;\n+\t\trs[i].src = xstrndup(lhs, llen);\n \n-\t\tif (*rs[i].src) {\n+\t\t/*\n+\t\t * Do we want to validate LHS?\n+\t\t *\n+\t\t * - Fetch refspec must have LHS that names a valid\n+\t\t *   looking ref (unless it is empty, which defaults to\n+\t\t *   HEAD).\n+\t\t *\n+\t\t * - Push refspec \"LHS\" without \":RHS\" is a synonym for\n+\t\t *   \"LHS:LHS\", and LHS must be a valid looking ref\n+\t\t *   (cannot be empty).\n+\t\t *\n+\t\t * - Push refspec \"LHS:RHS\" that is a glob must be\n+\t\t *   a valid looking ref (unless it is empty to remove)\n+\t\t *\n+\t\t * Hence we check non-empty LHS for fetch, and\n+\t\t * colonless or glob LHS for push here.\n+\t\t *\n+\t\t * Note that push refspec \"LHS:RHS\" that is not a glob\n+\t\t * can have any valid extended SHA-1 expression (or\n+\t\t * \"empty\" for removal) in LHS, and we cannot check\n+\t\t * for error until it actually gets used.\n+\t\t */\n+\t\tif (fetch ? (*rs[i].src) : (!rhs || is_glob)) {\n \t\t\tst = check_ref_format(rs[i].src);\n \t\t\tif (st && st != CHECK_REF_FORMAT_ONELEVEL)\n-\t\t\t\tdie(\"Invalid refspec '%s'\", refspec[i]);\n+\t\t\t\tgoto invalid;\n \t\t}\n-\t\tif (rs[i].dst && *rs[i].dst) {\n+\n+\t\t/*\n+\t\t * RHS: both push destination and fetch tracking ref\n+\t\t * must name a valid looking ref, but in fetch, colon\n+\t\t * followed by emptiness is the same as not having the\n+\t\t * colon nor RHS.\n+\t\t */\n+\t\tif (rs[i].dst && (!fetch || *rs[i].dst)) {\n \t\t\tst = check_ref_format(rs[i].dst);\n \t\t\tif (st && st != CHECK_REF_FORMAT_ONELEVEL)\n-\t\t\t\tdie(\"Invalid refspec '%s'\", refspec[i]);\n+\t\t\t\tgoto invalid;\n \t\t}\n \t}\n \treturn rs;\n+\n+ invalid:\n+\tdie(\"Invalid refspec '%s'\", refspec[i]);\n+}\n+\n+struct refspec *parse_fetch_refspec(int nr_refspec, const char **refspec)\n+{\n+\treturn parse_refspec_internal(nr_refspec, refspec, 1);\n+}\n+\n+struct refspec *parse_push_refspec(int nr_refspec, const char **refspec)\n+{\n+\treturn parse_refspec_internal(nr_refspec, refspec, 0);\n }\n \n static int valid_remote_nick(const char *name)\n@@ -475,8 +515,8 @@ struct remote *remote_get(const char *name)\n \t\tadd_url_alias(ret, name);\n \tif (!ret->url)\n \t\treturn NULL;\n-\tret->fetch = parse_ref_spec(ret->fetch_refspec_nr, ret->fetch_refspec);\n-\tret->push = parse_ref_spec(ret->push_refspec_nr, ret->push_refspec);\n+\tret->fetch = parse_fetch_refspec(ret->fetch_refspec_nr, ret->fetch_refspec);\n+\tret->push = parse_push_refspec(ret->push_refspec_nr, ret->push_refspec);\n \treturn ret;\n }\n \n@@ -489,11 +529,11 @@ int for_each_remote(each_remote_fn fn, void *priv)\n \t\tif (!r)\n \t\t\tcontinue;\n \t\tif (!r->fetch)\n-\t\t\tr->fetch = parse_ref_spec(r->fetch_refspec_nr,\n-\t\t\t\t\tr->fetch_refspec);\n+\t\t\tr->fetch = parse_fetch_refspec(r->fetch_refspec_nr,\n+\t\t\t\t\t\t       r->fetch_refspec);\n \t\tif (!r->push)\n-\t\t\tr->push = parse_ref_spec(r->push_refspec_nr,\n-\t\t\t\t\tr->push_refspec);\n+\t\t\tr->push = parse_push_refspec(r->push_refspec_nr,\n+\t\t\t\t\t\t     r->push_refspec);\n \t\tresult = fn(r, priv);\n \t}\n \treturn result;\n@@ -824,7 +864,7 @@ int match_refs(struct ref *src, struct ref *dst, struct ref ***dst_tail,\n \t       int nr_refspec, const char **refspec, int flags)\n {\n \tstruct refspec *rs =\n-\t\tparse_ref_spec(nr_refspec, (const char **) refspec);\n+\t\tparse_push_refspec(nr_refspec, (const char **) refspec);\n \tint send_all = flags & MATCH_REFS_ALL;\n \tint send_mirror = flags & MATCH_REFS_MIRROR;\n \ndiff --git a/remote.h b/remote.h\nindex f1dedf1..7e9ae79 100644\n--- a/remote.h\n+++ b/remote.h\n@@ -67,7 +67,8 @@ void free_refs(struct ref *ref);\n  */\n void ref_remove_duplicates(struct ref *ref_map);\n \n-struct refspec *parse_ref_spec(int nr_refspec, const char **refspec);\n+struct refspec *parse_fetch_refspec(int nr_refspec, const char **refspec);\n+struct refspec *parse_push_refspec(int nr_refspec, const char **refspec);\n \n int match_refs(struct ref *src, struct ref *dst, struct ref ***dst_tail,\n \t       int nr_refspec, const char **refspec, int all);\ndiff --git a/t/t5511-refspec.sh b/t/t5511-refspec.sh\nnew file mode 100755\nindex 0000000..7360311\n--- /dev/null\n+++ b/t/t5511-refspec.sh\n@@ -0,0 +1,67 @@\n+#!/bin/sh\n+\n+test_description='refspec parsing'\n+\n+. ./test-lib.sh\n+\n+test_refspec () {\n+\n+\tkind=$1 refspec=$2 expect=$3\n+\tgit config remote.frotz.url \".\" &&\n+\tgit config --remove-section remote.frotz &&\n+\tgit config remote.frotz.url \".\" &&\n+\tgit config \"remote.frotz.$kind\" \"$refspec\" &&\n+\tif test \"$expect\" != invalid\n+\tthen\n+\t\ttitle=\"$kind $refspec\"\n+\t\ttest='git ls-remote frotz'\n+\telse\n+\t\ttitle=\"$kind $refspec (invalid)\"\n+\t\ttest='test_must_fail git ls-remote frotz'\n+\tfi\n+\ttest_expect_success \"$title\" \"$test\"\n+}\n+\n+\n+test_refspec push 'refs/heads/*:refs/remotes/frotz/*'\n+test_refspec push 'refs/heads/*:refs/remotes/frotz'\t\tinvalid\n+test_refspec push 'refs/heads:refs/remotes/frotz/*'\t\tinvalid\n+test_refspec push 'refs/heads/master:refs/remotes/frotz/xyzzy'\n+\n+\n+# These have invalid LHS, but we do not have a formal \"valid sha-1\n+# expression syntax checker\" so they are not checked with the current\n+# code.  They will be caught downstream anyway, but we may want to\n+# have tighter check later...\n+\n+: test_refspec push 'refs/heads/master::refs/remotes/frotz/xyzzy'\tinvalid\n+: test_refspec push 'refs/heads/maste :refs/remotes/frotz/xyzzy'\tinvalid\n+\n+test_refspec fetch 'refs/heads/*:refs/remotes/frotz/*'\n+test_refspec fetch 'refs/heads/*:refs/remotes/frotz'\t\tinvalid\n+test_refspec fetch 'refs/heads:refs/remotes/frotz/*'\t\tinvalid\n+test_refspec fetch 'refs/heads/master:refs/remotes/frotz/xyzzy'\n+test_refspec fetch 'refs/heads/master::refs/remotes/frotz/xyzzy'\tinvalid\n+test_refspec fetch 'refs/heads/maste :refs/remotes/frotz/xyzzy'\tinvalid\n+\n+test_refspec push 'master~1:refs/remotes/frotz/backup'\n+test_refspec fetch 'master~1:refs/remotes/frotz/backup'\t\tinvalid\n+test_refspec push 'HEAD~4:refs/remotes/frotz/new'\n+test_refspec fetch 'HEAD~4:refs/remotes/frotz/new'\t\tinvalid\n+\n+test_refspec push 'HEAD'\n+test_refspec fetch 'HEAD'\n+test_refspec push 'refs/heads/ nitfol'\t\t\t\tinvalid\n+test_refspec fetch 'refs/heads/ nitfol'\t\t\t\tinvalid\n+\n+test_refspec push 'HEAD:'\t\t\t\t\tinvalid\n+test_refspec fetch 'HEAD:'\n+test_refspec push 'refs/heads/ nitfol:'\t\t\t\tinvalid\n+test_refspec fetch 'refs/heads/ nitfol:'\t\t\tinvalid\n+\n+test_refspec push ':refs/remotes/frotz/deleteme'\n+test_refspec fetch ':refs/remotes/frotz/HEAD-to-me'\n+test_refspec push ':refs/remotes/frotz/delete me'\t\tinvalid\n+test_refspec fetch ':refs/remotes/frotz/HEAD to me'\t\tinvalid\n+\n+test_done\n-- \n1.5.5.rc0.139.g9315d\n"},{"id":"72630","messageId":"alpine.LNX.1.00.0803211840480.19665@iabervon.org","threadId":"12785","inReplyTo":"7vlk4boh6v.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] remote.c: Fix overtight refspec validation","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2008-03-21T23:12:15Z","receivedAt":"2008-03-21T23:12:15Z","isPatch":true,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Fri, 21 Mar 2008, Junio C Hamano wrote:\n\n> We tightened the refspec validation code in an earlier ef00d15 (Tighten\n> refspec processing, 2008-03-17) per my suggestion, but the suggestion was\n> misguided to begin with and it broke this usage:\n> \n>     $ git push origin HEAD~12:master\n> \n> The syntax of push refspecs and fetch refspecs are similar in that they\n> are both colon separated LHS and RHS (possibly prefixed with a + to\n> force), but the similarity ends there.  For example, LHS in a push refspec\n> can be anything that evaluates to a valid object name at runtime (except\n> when colon and RHS is missing, or it is a glob), while it must be a\n> valid-looking refname in a fetch refspec.  To validate them correctly, the\n> caller needs to be able to say which kind of refspecs they are.  It is\n> unreasonable to keep a single interface that cannot tell which kind it is\n> dealing with, and ask it to behave sensibly.\n> \n> This commit separates the parsing of the two into different functions, and\n> clarifies the code to implement the parsing proper (i.e. splitting into\n> two parts, making sure both sides are wildcard or neither side is).\n> \n> This happens to also allow pushing a commit named with the esoteric \"look\n> for that string\" syntax:\n> \n>     $ git push ../test.git ':/remote.c: Fix overtight refspec:master'\n\n\n> \n> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n> ---\n> \n>   Daniel Barkalow <barkalow@iabervon.org> writes:\n> \n>   > ... (although the \n>   > patch text made it hard to see; we really need some magic to make diff not \n>   > bother to save 4 identifier-free lines and just show it as a complete \n>   > replacement).\n> \n>   Yeah, I tend to agree and some people seem to favor \"diff -c\" for this\n>   reason.\n> \n>   In any case, here is an updated patch with a better description and\n>   necessary tests.  The \"validly looking extended SHA-1 expression\" check\n>   is wholly outside the scope of 1.5.5 regression fix so I did not even\n>   attempt to address it, but what this commit fixes needs to be addressed\n>   before -rc1.\n> \n>  builtin-fetch.c     |    3 +-\n>  builtin-send-pack.c |    2 +-\n>  remote.c            |  122 ++++++++++++++++++++++++++++++++++-----------------\n>  remote.h            |    3 +-\n>  t/t5511-refspec.sh  |   67 ++++++++++++++++++++++++++++\n>  5 files changed, 153 insertions(+), 44 deletions(-)\n>  create mode 100755 t/t5511-refspec.sh\n> \n> diff --git a/builtin-fetch.c b/builtin-fetch.c\n> index b2b9935..a11548c 100644\n> --- a/builtin-fetch.c\n> +++ b/builtin-fetch.c\n> @@ -652,5 +652,6 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n>  \n>  \tsignal(SIGINT, unlock_pack_on_signal);\n>  \tatexit(unlock_pack);\n> -\treturn do_fetch(transport, parse_ref_spec(ref_nr, refs), ref_nr);\n> +\treturn do_fetch(transport,\n> +\t\t\tparse_fetch_refspec(ref_nr, refs), ref_nr);\n>  }\n> diff --git a/builtin-send-pack.c b/builtin-send-pack.c\n> index 930e0fb..bb9c33a 100644\n> --- a/builtin-send-pack.c\n> +++ b/builtin-send-pack.c\n> @@ -537,7 +537,7 @@ static void verify_remote_names(int nr_heads, const char **heads)\n>  \tint i;\n>  \n>  \tfor (i = 0; i < nr_heads; i++) {\n> -\t\tconst char *remote = strchr(heads[i], ':');\n> +\t\tconst char *remote = strrchr(heads[i], ':');\n>  \n>  \t\tremote = remote ? (remote + 1) : heads[i];\n>  \t\tswitch (check_ref_format(remote)) {\n> diff --git a/remote.c b/remote.c\n> index 9700a33..4117bfc 100644\n> --- a/remote.c\n> +++ b/remote.c\n> @@ -393,58 +393,98 @@ static void read_config(void)\n>  \talias_all_urls();\n>  }\n>  \n> -struct refspec *parse_ref_spec(int nr_refspec, const char **refspec)\n> +static struct refspec *parse_refspec_internal(int nr_refspec, const char **refspec, int fetch)\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\tconst char *sp, *ep, *gp;\n> -\t\tsp = refspec[i];\n> -\t\tif (*sp == '+') {\n> +\t\tsize_t llen, rlen;\n> +\t\tint is_glob;\n> +\t\tconst char *lhs, *rhs;\n> +\n> +\t\tllen = rlen = is_glob = 0;\n> +\n> +\t\tlhs = refspec[i];\n> +\t\tif (*lhs == '+') {\n>  \t\t\trs[i].force = 1;\n> -\t\t\tsp++;\n> +\t\t\tlhs++;\n>  \t\t}\n> -\t\tgp = strstr(sp, \"/*\");\n> -\t\tep = strchr(sp, ':');\n> -\t\tif (gp && ep && gp > ep)\n> -\t\t\tgp = NULL;\n> -\t\tif (ep) {\n> -\t\t\tif (ep[1]) {\n> -\t\t\t\tconst char *glob = strstr(ep + 1, \"/*\");\n> -\t\t\t\tif (glob && glob[2])\n> -\t\t\t\t\tglob = NULL;\n> -\t\t\t\tif (!glob)\n> -\t\t\t\t\tgp = NULL;\n> -\t\t\t\tif (gp)\n> -\t\t\t\t\trs[i].dst = xstrndup(ep + 1,\n> -\t\t\t\t\t\t\t     glob - ep - 1);\n> -\t\t\t\telse\n> -\t\t\t\t\trs[i].dst = xstrdup(ep + 1);\n> -\t\t\t}\n> -\t\t} else {\n> -\t\t\tep = sp + strlen(sp);\n> +\n> +\t\trhs = strrchr(lhs, ':');\n> +\t\tif (rhs) {\n> +\t\t\trhs++;\n> +\t\t\trlen = strlen(rhs);\n> +\t\t\tis_glob = (2 <= rlen && !strcmp(rhs + rlen - 2, \"/*\"));\n> +\t\t\trs[i].dst = xstrndup(rhs, rlen - is_glob * 2);\n>  \t\t}\n> -\t\tif (gp && gp + 2 != ep)\n> -\t\t\tgp = NULL;\n> -\t\tif (gp) {\n> -\t\t\trs[i].pattern = 1;\n> -\t\t\tep = gp;\n> +\n> +\t\tllen = (rhs ? (rhs - lhs - 1) : strlen(lhs));\n> +\t\tif (is_glob != (2 <= llen && !memcmp(lhs + llen - 2, \"/*\", 2)))\n> +\t\t\tgoto invalid;\n> +\n> +\t\tif (is_glob) {\n> +\t\t\tllen -= 2;\n> +\t\t\trlen -= 2;\n>  \t\t}\n> -\t\trs[i].src = xstrndup(sp, ep - sp);\n> +\t\trs[i].pattern = is_glob;\n> +\t\trs[i].src = xstrndup(lhs, llen);\n>  \n> -\t\tif (*rs[i].src) {\n> +\t\t/*\n> +\t\t * Do we want to validate LHS?\n> +\t\t *\n> +\t\t * - Fetch refspec must have LHS that names a valid\n> +\t\t *   looking ref (unless it is empty, which defaults to\n> +\t\t *   HEAD).\n> +\t\t *\n> +\t\t * - Push refspec \"LHS\" without \":RHS\" is a synonym for\n> +\t\t *   \"LHS:LHS\", and LHS must be a valid looking ref\n> +\t\t *   (cannot be empty).\n> +\t\t *\n> +\t\t * - Push refspec \"LHS:RHS\" that is a glob must be\n> +\t\t *   a valid looking ref (unless it is empty to remove)\n> +\t\t *\n> +\t\t * Hence we check non-empty LHS for fetch, and\n> +\t\t * colonless or glob LHS for push here.\n> +\t\t *\n> +\t\t * Note that push refspec \"LHS:RHS\" that is not a glob\n> +\t\t * can have any valid extended SHA-1 expression (or\n> +\t\t * \"empty\" for removal) in LHS, and we cannot check\n> +\t\t * for error until it actually gets used.\n> +\t\t */\n\nWouldn't this be clearer and not meaningfully harder in \nparse_fetch_refspec and parse_push_refspec? There are, between the two \ntypes of checking and the two sides to check, a total of 6 expressions \nused to determine whether something has to be valid as a head, and only \none pair of them are the same (rs[i].dst), and that's trivial (you don't \ncheck the part after the colon if there's no colon).\n\n> +\t\tif (fetch ? (*rs[i].src) : (!rhs || is_glob)) {\n\nThis is an odd combination of locals and struct members.\n                                         : (!rs[i].dst || rs[i].pattern) {\n\n>  \t\t\tst = check_ref_format(rs[i].src);\n>  \t\t\tif (st && st != CHECK_REF_FORMAT_ONELEVEL)\n> -\t\t\t\tdie(\"Invalid refspec '%s'\", refspec[i]);\n> +\t\t\t\tgoto invalid;\n>  \t\t}\n> -\t\tif (rs[i].dst && *rs[i].dst) {\n> +\n> +\t\t/*\n> +\t\t * RHS: both push destination and fetch tracking ref\n> +\t\t * must name a valid looking ref, but in fetch, colon\n> +\t\t * followed by emptiness is the same as not having the\n> +\t\t * colon nor RHS.\n> +\t\t */\n> +\t\tif (rs[i].dst && (!fetch || *rs[i].dst)) {\n>  \t\t\tst = check_ref_format(rs[i].dst);\n>  \t\t\tif (st && st != CHECK_REF_FORMAT_ONELEVEL)\n> -\t\t\t\tdie(\"Invalid refspec '%s'\", refspec[i]);\n> +\t\t\t\tgoto invalid;\n>  \t\t}\n>  \t}\n>  \treturn rs;\n> +\n> + invalid:\n> +\tdie(\"Invalid refspec '%s'\", refspec[i]);\n> +}\n"},{"id":"72631","messageId":"7vy78bmxx1.fsf@gitster.siamese.dyndns.org","threadId":"12785","inReplyTo":"alpine.LNX.1.00.0803211840480.19665@iabervon.org","subject":"Re: [PATCH] remote.c: Fix overtight refspec validation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-03-21T23:59:06Z","receivedAt":"2008-03-21T23:59:06Z","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>> +\t\t/*\n>> +\t\t * Do we want to validate LHS?\n>> + ...\n>> +\t\t * Hence we check non-empty LHS for fetch, and\n>> +\t\t * colonless or glob LHS for push here.\n>> +\t\t */\n>\n> Wouldn't this be clearer and not meaningfully harder in \n> parse_fetch_refspec and parse_push_refspec?\n\nDo you mean you want the callers of this internal implementation to also\nloop over the input set of refs?  I think that would be more complex code\nbut I do not see much gain by doing so.\n\n>> +\t\tif (fetch ? (*rs[i].src) : (!rhs || is_glob)) {\n>\n> This is an odd combination of locals and struct members.\n>                                        : (!rs[i].dst || rs[i].pattern) {\n\nSorry, I do not understand what's wrong about it.\n\n\t!!rhs === (did we see a colon) === !!rs[].dst\n        is_glob === (did they both end with \"/*\") === rs[].pattern\n\nThey are equivalent, and local variables are primarily what the logic\nworks on and bases its decisions to store what in rs[] structures.\n\nAhh...  do you mean:\n\n\t(*rs[i].src) === (is lhs non empty?) === !!llen\n\nI guess using \"llen\" there is more consistent and is moderately cleaner.\n\nPerhaps squash this as a clean-up?\n\ndiff --git a/remote.c b/remote.c\nindex 4117bfc..86113b7 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -453,7 +453,7 @@ static struct refspec *parse_refspec_internal(int nr_refspec, const char **refsp\n \t\t * \"empty\" for removal) in LHS, and we cannot check\n \t\t * for error until it actually gets used.\n \t\t */\n-\t\tif (fetch ? (*rs[i].src) : (!rhs || is_glob)) {\n+\t\tif (fetch ? llen : (!rhs || is_glob)) {\n \t\t\tst = check_ref_format(rs[i].src);\n \t\t\tif (st && st != CHECK_REF_FORMAT_ONELEVEL)\n \t\t\t\tgoto invalid;\n"},{"id":"72633","messageId":"alpine.LNX.1.00.0803212021190.19665@iabervon.org","threadId":"12785","inReplyTo":"7vy78bmxx1.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] remote.c: Fix overtight refspec validation","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2008-03-22T00:36:42Z","receivedAt":"2008-03-22T00:36:42Z","isPatch":true,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Fri, 21 Mar 2008, Junio C Hamano wrote:\n\n> Daniel Barkalow <barkalow@iabervon.org> writes:\n> \n> >> +\t\t/*\n> >> +\t\t * Do we want to validate LHS?\n> >> + ...\n> >> +\t\t * Hence we check non-empty LHS for fetch, and\n> >> +\t\t * colonless or glob LHS for push here.\n> >> +\t\t */\n> >\n> > Wouldn't this be clearer and not meaningfully harder in \n> > parse_fetch_refspec and parse_push_refspec?\n> \n> Do you mean you want the callers of this internal implementation to also\n> loop over the input set of refs?  I think that would be more complex code\n> but I do not see much gain by doing so.\n\nI think it's more breadth but less depth. It would make the internal \nimplementation not depend on fetch, and put the checks that only apply to \nfetch out of the push code path and vice versa.\n\nOr just have a section\n\n\tif (fetch) {\n\t\t// checks for fetch LHS\n\t\t// checks for fetch RHS\n\t} else {\n\t\t// checks for push LHS\n\t\t// checks for push RHS\n\t}\n\nThe body of the condition is only four lines, after all.\n\n> >> +\t\tif (fetch ? (*rs[i].src) : (!rhs || is_glob)) {\n> >\n> > This is an odd combination of locals and struct members.\n> >                                        : (!rs[i].dst || rs[i].pattern) {\n> \n> Sorry, I do not understand what's wrong about it.\n> \n> \t!!rhs === (did we see a colon) === !!rs[].dst\n>         is_glob === (did they both end with \"/*\") === rs[].pattern\n> \n> They are equivalent, and local variables are primarily what the logic\n> works on and bases its decisions to store what in rs[] structures.\n\nConsidering that it's already stored values into the struct, it might as \nwell use those, and those are presumably more familiar to the average \nreader, because all of the git code that uses refspecs other than this \nfunction uses them.\n\n> Ahh...  do you mean:\n> \n> \t(*rs[i].src) === (is lhs non empty?) === !!llen\n> \n> I guess using \"llen\" there is more consistent and is moderately cleaner.\n\nI'd go the other way, but having them all from the same set makes more \nsense than one from one set and two from the other, regardless of which \nway. If you go this way, you should probably also include the the rhs \nchecks, and the argument to check_ref_format().\n\n\t-Daniel\n*This .sig left intentionally blank*\n"},{"id":"72696","messageId":"7viqzeilp0.fsf@gitster.siamese.dyndns.org","threadId":"12785","inReplyTo":"alpine.LNX.1.00.0803212021190.19665@iabervon.org","subject":"Re: [PATCH] remote.c: Fix overtight refspec validation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-03-22T19:48:59Z","receivedAt":"2008-03-22T19:48:59Z","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> On Fri, 21 Mar 2008, Junio C Hamano wrote:\n> ...\n>> Do you mean you want the callers of this internal implementation to also\n>> loop over the input set of refs?  I think that would be more complex code\n>> but I do not see much gain by doing so.\n>\n> I think it's more breadth but less depth. It would make the internal \n> implementation not depend on fetch, and put the checks that only apply to \n> fetch out of the push code path and vice versa.\n>\n> Or just have a section\n>\n> \tif (fetch) {\n> \t\t// checks for fetch LHS\n> \t\t// checks for fetch RHS\n> \t} else {\n> \t\t// checks for push LHS\n> \t\t// checks for push RHS\n> \t}\n>\n> The body of the condition is only four lines, after all.\n\nThere are two commits on jc/refspec-fix branch merged to 'pu'.  The\nearlier one is my version, and the one on top is based on the above\nsuggestion.  I do not know which one is clearer, more readable and\nmaintainable.\n\n>> Ahh...  do you mean:\n>> \n>> \t(*rs[i].src) === (is lhs non empty?) === !!llen\n>> \n>> I guess using \"llen\" there is more consistent and is moderately cleaner.\n>\n> I'd go the other way, but having them all from the same set makes more \n> sense than one from one set and two from the other, regardless of which \n> way. If you go this way, you should probably also include the the rhs \n> checks, and the argument to check_ref_format().\n\nIt is very much sensible to look at either the local variables it used\nduring the parsing and the tentative result it produced while deciding\nwhat to check and how, and it also is very sensible to validate what it\ngives back to the user and/or what it knows is equal to what it gives back\nto the user.  When they are equivalent, it is mostly a matter of taste\nwhich to use for deciding and which to use for validating.  But it makes\nmore sense to prefer its local variables for logic to decide what to do\nand how, and validate what we are actually going to give back.\n\nIf you mean to suggest to change parameter given to check_ref_format()\nfrom rs[i].{src,dst} to something else based on the local variables we\nused, that's backwards.\n\nBut we have already agreed that our brains are wired differently when it\ncomes to taste; I'd prefer not pursuing bikeshedding any further.\n\nUnless your suggestion _isn't_ bikeshedding, that is.\n\nWhat I did so far was to spend time to respond with code that fixes\nexisting breakages, while what you did so far was to kibitz instead of\nshowing code.  Showing code might make me realize that the way you may\nwant to go is more than bikeshedding and actual improvements to\nreadability and maintainability, but honestly speaking I am tired of\nlooking at this part of the code for now, so...\n"},{"id":"72703","messageId":"alpine.LNX.1.00.0803221559220.19665@iabervon.org","threadId":"12785","inReplyTo":"7viqzeilp0.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] remote.c: Fix overtight refspec validation","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2008-03-22T20:45:20Z","receivedAt":"2008-03-22T20:45:20Z","isPatch":true,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Sat, 22 Mar 2008, Junio C Hamano wrote:\n\n> Daniel Barkalow <barkalow@iabervon.org> writes:\n> \n> > On Fri, 21 Mar 2008, Junio C Hamano wrote:\n> > ...\n> >> Do you mean you want the callers of this internal implementation to also\n> >> loop over the input set of refs?  I think that would be more complex code\n> >> but I do not see much gain by doing so.\n> >\n> > I think it's more breadth but less depth. It would make the internal \n> > implementation not depend on fetch, and put the checks that only apply to \n> > fetch out of the push code path and vice versa.\n> >\n> > Or just have a section\n> >\n> > \tif (fetch) {\n> > \t\t// checks for fetch LHS\n> > \t\t// checks for fetch RHS\n> > \t} else {\n> > \t\t// checks for push LHS\n> > \t\t// checks for push RHS\n> > \t}\n> >\n> > The body of the condition is only four lines, after all.\n> \n> There are two commits on jc/refspec-fix branch merged to 'pu'.  The\n> earlier one is my version, and the one on top is based on the above\n> suggestion.  I do not know which one is clearer, more readable and\n> maintainable.\n\nI like the second one quite a bit. If nothing else, it's got, as the\ncomments, the best documentation of refspecs I've seen, and then it's got \ncode which clearly implements those rules. I think the explicitness is \nworth the extra lines.\n\nThanks for taking care of this; as a result of starting a new job, I \nhaven't had much in the way of coding attention for other stuff the past \ncouple of weeks.\n\n\t-Daniel\n*This .sig left intentionally blank*\n"},{"id":"73096","messageId":"alpine.LFD.1.00.0803251841420.2775@woody.linux-foundation.org","threadId":"12785","inReplyTo":"7vlk4boh6v.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] remote.c: Fix overtight refspec validation","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2008-03-26T01:45:25Z","receivedAt":"2008-03-26T01:45:25Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Fri, 21 Mar 2008, Junio C Hamano wrote:\n>\n> We tightened the refspec validation code in an earlier ef00d15 (Tighten\n> refspec processing, 2008-03-17) per my suggestion, but the suggestion was\n> misguided to begin with and it broke this usage:\n\nIt seems to have also broken \"git push --tags\".\n\nI now get \n\n\tfatal: Invalid refspec 'refs/tags/*'\n\nwhich seems a bit sad.\n\n\t\t\tLinus\n"},{"id":"73101","messageId":"7vod92jh3u.fsf@gitster.siamese.dyndns.org","threadId":"12785","inReplyTo":"alpine.LFD.1.00.0803251841420.2775@woody.linux-foundation.org","subject":"Re: [PATCH] remote.c: Fix overtight refspec validation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-03-26T03:31:49Z","receivedAt":"2008-03-26T03:31:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds <torvalds@linux-foundation.org> writes:\n\n> On Fri, 21 Mar 2008, Junio C Hamano wrote:\n>>\n>> We tightened the refspec validation code in an earlier ef00d15 (Tighten\n>> refspec processing, 2008-03-17) per my suggestion, but the suggestion was\n>> misguided to begin with and it broke this usage:\n>\n> It seems to have also broken \"git push --tags\".\n>\n> I now get \n>\n> \tfatal: Invalid refspec 'refs/tags/*'\n>\n> which seems a bit sad.\n\nYeah that indeed is very sad.  Given that Andrew identified another\nregression on the fetch side today, I am beginning to suspect that 1.5.4\nwas still too early to merge the C rewrite.  On the other hand, until we\ntag, new features are not tested in the field, so...\n\nEspecially that \"refs/tags/*\" is sad in that it is leaking an internal\nimplementation detail.  I do not think the original code ever used\nwildcards on the push side, and it probably was a good idea to allow\nwildcards when the code was rewritten.\n\nIn any case, this should fix it.\n\n builtin-push.c |    2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/builtin-push.c b/builtin-push.c\nindex b68c681..5316d6d 100644\n--- a/builtin-push.c\n+++ b/builtin-push.c\n@@ -120,7 +120,7 @@ int cmd_push(int argc, const char **argv, const char *prefix)\n \tif (verbose)\n \t\tflags |= TRANSPORT_PUSH_VERBOSE;\n \tif (tags)\n-\t\tadd_refspec(\"refs/tags/*\");\n+\t\tadd_refspec(\"refs/tags/*:refs/tags/*\");\n \tif (all)\n \t\tflags |= TRANSPORT_PUSH_ALL;\n \tif (mirror)\n"},{"id":"73108","messageId":"7vfxuejf9d.fsf@gitster.siamese.dyndns.org","threadId":"12785","inReplyTo":"7vod92jh3u.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] remote.c: Fix overtight refspec validation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-03-26T04:11:42Z","receivedAt":"2008-03-26T04:11:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Especially that \"refs/tags/*\" is sad in that it is leaking an internal\n> implementation detail.  I do not think the original code ever used\n> wildcards on the push side, and it probably was a good idea to allow\n> wildcards when the code was rewritten.\n\nHaving thought about this a bit more, I think this patch would be more\nuseful.  Please discard the previous patch to builtin-push.c and replace\nwith this patch.\n\nNow it allows you to say:\n\n\t[remote \"neigh\"]\n        \turl = ../neighbour\n                push = refs/tags/*\n\nto propagate all tags one-to-one, without having to say\n\"refs/tags/*:refs/tags/*\".\n\nThis however has unintended side effect of allowing \n\n\t[remote \"bour\"]\n        \turl = ../neighbour\n                fetch = refs/heads/*\n\nat the syntax level.  I do not know offhand the fetch backends are\nprepared to deal with such wildcard patterns.\n\nDaniel?\n\n---\n remote.c |   18 +++++++++++-------\n 1 files changed, 11 insertions(+), 7 deletions(-)\n\ndiff --git a/remote.c b/remote.c\nindex 40ed246..c39d831 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -417,17 +417,21 @@ static struct refspec *parse_refspec_internal(int nr_refspec, const char **refsp\n \t\t\trhs++;\n \t\t\trlen = strlen(rhs);\n \t\t\tis_glob = (2 <= rlen && !strcmp(rhs + rlen - 2, \"/*\"));\n-\t\t\trs[i].dst = xstrndup(rhs, rlen - is_glob * 2);\n+\t\t\tif (is_glob)\n+\t\t\t\trlen -= 2;\n+\t\t\trs[i].dst = xstrndup(rhs, rlen);\n \t\t}\n \n \t\tllen = (rhs ? (rhs - lhs - 1) : strlen(lhs));\n-\t\tif (is_glob != (2 <= llen && !memcmp(lhs + llen - 2, \"/*\", 2)))\n-\t\t\tgoto invalid;\n-\n-\t\tif (is_glob) {\n+\t\tif (2 <= llen && !memcmp(lhs + llen - 2, \"/*\", 2)) {\n+\t\t\tif (rhs && !is_glob)\n+\t\t\t\tgoto invalid;\n+\t\t\tis_glob = 1;\n \t\t\tllen -= 2;\n-\t\t\trlen -= 2;\n+\t\t} else if (rhs && is_glob) {\n+\t\t\tgoto invalid;\n \t\t}\n+\n \t\trs[i].pattern = is_glob;\n \t\trs[i].src = xstrndup(lhs, llen);\n \n@@ -446,7 +450,7 @@ static struct refspec *parse_refspec_internal(int nr_refspec, const char **refsp\n \t\t\t}\n \t\t\t/*\n \t\t\t * RHS\n-\t\t\t * - missing is allowed.\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"},{"id":"73110","messageId":"alpine.LNX.1.00.0803260130390.19665@iabervon.org","threadId":"12785","inReplyTo":"7vfxuejf9d.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] remote.c: Fix overtight refspec validation","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2008-03-26T05:42:02Z","receivedAt":"2008-03-26T05:42:02Z","isPatch":true,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Tue, 25 Mar 2008, Junio C Hamano wrote:\n\n> This however has unintended side effect of allowing \n> \n> \t[remote \"bour\"]\n>         \turl = ../neighbour\n>                 fetch = refs/heads/*\n> \n> at the syntax level.  I do not know offhand the fetch backends are\n> prepared to deal with such wildcard patterns.\n> \n> Daniel?\n\nIt's not a matter of the backends, which don't implement any of the \ncontrol flow in the fetch direction; it's get_expanded_map(), which needs \nto be told that you can have something match a pattern but not have a \nlocal tracking ref.\n\nOTOH, the only use for such a pattern is an octopus merge of whatever \nbranches a remote happens to have, right? I remember thinking this was a \nnon-useful refspec when I was dealing with the fetch code (and then \nforgetting that it was useful for push). It might be better to just \ndisallow it in the direction-specific semantic checks.\n\nHere's the patch to make it work, anyway:\n----\n\ncommit 6a8bcb917e1aa9b3c972f14f618ab573e457ebee\nAuthor: Daniel Barkalow <barkalow@iabervon.org>\nDate:   Wed Mar 26 01:39:07 2008 -0400\n\n    Support fetching refspecs like \"refs/heads/*\"\n    \n    Signed-off-by: Daniel Barkalow <barkalow@iabervon.org>\n\ndiff --git a/remote.c b/remote.c\nindex a027bca..f8f4b34 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -998,22 +998,24 @@ static struct ref *get_expanded_map(const struct ref *remote_refs,\n \tstruct ref **tail = &ret;\n \n \tint remote_prefix_len = strlen(refspec->src);\n-\tint local_prefix_len = strlen(refspec->dst);\n+\tint local_prefix_len = refspec->dst ? strlen(refspec->dst) : 0;\n \n \tfor (ref = remote_refs; ref; ref = ref->next) {\n \t\tif (strchr(ref->name, '^'))\n \t\t\tcontinue; /* a dereference item */\n \t\tif (!prefixcmp(ref->name, refspec->src)) {\n-\t\t\tconst char *match;\n \t\t\tstruct ref *cpy = copy_ref(ref);\n-\t\t\tmatch = ref->name + remote_prefix_len;\n-\n-\t\t\tcpy->peer_ref = alloc_ref(local_prefix_len +\n-\t\t\t\t\t\t  strlen(match) + 1);\n-\t\t\tsprintf(cpy->peer_ref->name, \"%s%s\",\n-\t\t\t\trefspec->dst, match);\n-\t\t\tif (refspec->force)\n-\t\t\t\tcpy->peer_ref->force = 1;\n+\n+\t\t\tif (refspec->dst) {\n+\t\t\t\tconst char *match = ref->name + \n+\t\t\t\t\tremote_prefix_len;\n+\t\t\t\tcpy->peer_ref = alloc_ref(local_prefix_len +\n+\t\t\t\t\t\t\t  strlen(match) + 1);\n+\t\t\t\tsprintf(cpy->peer_ref->name, \"%s%s\",\n+\t\t\t\t\trefspec->dst, match);\n+\t\t\t\tif (refspec->force)\n+\t\t\t\t\tcpy->peer_ref->force = 1;\n+\t\t\t}\n \t\t\t*tail = cpy;\n \t\t\ttail = &cpy->next;\n \t\t}\n"},{"id":"73111","messageId":"7vtziughqo.fsf@gitster.siamese.dyndns.org","threadId":"12785","inReplyTo":"alpine.LNX.1.00.0803260130390.19665@iabervon.org","subject":"Re: [PATCH] remote.c: Fix overtight refspec validation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-03-26T05:46:23Z","receivedAt":"2008-03-26T05:46:23Z","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> OTOH, the only use for such a pattern is an octopus merge of whatever \n> branches a remote happens to have, right? I remember thinking this was a \n> non-useful refspec when I was dealing with the fetch code (and then \n> forgetting that it was useful for push). It might be better to just \n> disallow it in the direction-specific semantic checks.\n\nI agree.  refs/*:refs/* could have been useful before --mirror, but refs/*\n(fetch but not store) is not useful at all, so what I plan to commit looks\nlike this (i.e. \"the third time lucky\" edition).\n\n-- >8 --\n[PATCH] refspec: allow colon-less wildcard \"refs/category/*\"\n\n\"git push --tags elsewhere\" is implemented in terms of wildcarded refspec\n\"refs/tags/*\" these days, and the user wants to push the tags under the\nsame name to the other branch.  This resurrects the support for it.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n remote.c |   18 +++++++++++-------\n 1 files changed, 11 insertions(+), 7 deletions(-)\n\ndiff --git a/remote.c b/remote.c\nindex 40ed246..04f7521 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -417,17 +417,21 @@ static struct refspec *parse_refspec_internal(int nr_refspec, const char **refsp\n \t\t\trhs++;\n \t\t\trlen = strlen(rhs);\n \t\t\tis_glob = (2 <= rlen && !strcmp(rhs + rlen - 2, \"/*\"));\n-\t\t\trs[i].dst = xstrndup(rhs, rlen - is_glob * 2);\n+\t\t\tif (is_glob)\n+\t\t\t\trlen -= 2;\n+\t\t\trs[i].dst = xstrndup(rhs, rlen);\n \t\t}\n \n \t\tllen = (rhs ? (rhs - lhs - 1) : strlen(lhs));\n-\t\tif (is_glob != (2 <= llen && !memcmp(lhs + llen - 2, \"/*\", 2)))\n-\t\t\tgoto invalid;\n-\n-\t\tif (is_glob) {\n+\t\tif (2 <= llen && !memcmp(lhs + llen - 2, \"/*\", 2)) {\n+\t\t\tif ((rhs && !is_glob) || (!rhs && fetch))\n+\t\t\t\tgoto invalid;\n+\t\t\tis_glob = 1;\n \t\t\tllen -= 2;\n-\t\t\trlen -= 2;\n+\t\t} else if (rhs && is_glob) {\n+\t\t\tgoto invalid;\n \t\t}\nj+\n \t\trs[i].pattern = is_glob;\n \t\trs[i].src = xstrndup(lhs, llen);\n \n@@ -446,7 +450,7 @@ static struct refspec *parse_refspec_internal(int nr_refspec, const char **refsp\n \t\t\t}\n \t\t\t/*\n \t\t\t * RHS\n-\t\t\t * - missing is allowed.\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-- \n1.5.5.rc1.128.g340c\n"},{"id":"73115","messageId":"20080326062232.GB26286@coredump.intra.peff.net","threadId":"12785","inReplyTo":"7vtziughqo.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] remote.c: Fix overtight refspec validation","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-03-26T06:22:32Z","receivedAt":"2008-03-26T06:22:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 25, 2008 at 10:46:23PM -0700, Junio C Hamano wrote:\n\n> I agree.  refs/*:refs/* could have been useful before --mirror, but refs/*\n\nIt still is, unless there is a way to specify --mirror in your config\nfile.\n\n-Peff\n"}]}