{"thread":{"id":"24169","subject":"[PATCH v4] Do not decode url protocol.","startedAt":"2010-06-22T20:16:57Z","lastAt":"2010-06-23T17:38:59Z","messageCount":5,"participants":["Pascal Obry","Matthieu Moy","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":4,"patchTotal":null},"messages":[{"id":"144050","messageId":"4C211A39.2080207@obry.net","threadId":"24169","inReplyTo":null,"subject":"[PATCH v4] Do not decode url protocol.","fromName":"Pascal Obry","fromEmail":"pascal.obry@gmail.com","sentAt":"2010-06-22T20:16:57Z","receivedAt":"2010-06-22T20:16:57Z","isPatch":true,"sender":{"key":"pascal@obry.net","avatar":"https://avatars.githubusercontent.com/u/467069?v=4"},"body":"\nWhen using the protocol git+ssh:// for example we do not want to\ndecode the '+' as a space. The url decoding must take place only\nfor the server name and parameters.\n\nThis fixes a regression introduced in 9d2e942.\n---\n url.c | 19 +++++++++++++++----\n 1 files changed, 15 insertions(+), 4 deletions(-)\n\nOk, so this is the fourth version of this patch. Thanks again Matthieu\nfor the review. I think this time I got the place for the message right :)\n\nAnyway, I think this time we properly skip the protocol decoding when\nneeded.\n\ndiff --git a/url.c b/url.c\nindex cd32b92..0eb7fb3 100644\n--- a/url.c\n+++ b/url.c\n@@ -67,12 +67,23 @@ static int url_decode_char(const char *q)\n return val;\n }\n\n-static char *url_decode_internal(const char **query, const char *stop_at)\n+static char *url_decode_internal(const char **query, const char *stop_at,\n+ int with_protocol)\n {\n const char *q = *query;\n+ const char *first_slash;\n struct strbuf out;\n\n strbuf_init(&out, 16);\n+\n+ /* Skip protocol if present. */\n+ if (with_protocol) {\n+ first_slash = strchr(*query, '/');\n+\n+ while (q < first_slash)\n+ strbuf_addch(&out, *q++);\n+ }\n+\n do {\n unsigned char c = *q;\n\n@@ -104,15 +115,15 @@ static char *url_decode_internal(const char\n**query, const char *stop_at)\n\n char *url_decode(const char *url)\n {\n- return url_decode_internal(&url, NULL);\n+ return url_decode_internal(&url, NULL, 1);\n }\n\n char *url_decode_parameter_name(const char **query)\n {\n- return url_decode_internal(query, \"&=\");\n+ return url_decode_internal(query, \"&=\", 0);\n }\n\n char *url_decode_parameter_value(const char **query)\n {\n- return url_decode_internal(query, \"&\");\n+ return url_decode_internal(query, \"&\", 0);\n }\n-- \n1.7.1.426.gb436.dirty\n\n-- \n\n--|------------------------------------------------------\n--| Pascal Obry Team-Ada Member\n--| 45, rue Gabriel Peri - 78114 Magny Les Hameaux FRANCE\n--|------------------------------------------------------\n--| http://www.obry.net - http://v2p.fr.eu.org\n--| \"The best way to travel is by means of imagination\"\n--|\n--| gpg --keyserver keys.gnupg.net --recv-key F949BD3B\n"},{"id":"144053","messageId":"vpqmxumu4pp.fsf@bauges.imag.fr","threadId":"24169","inReplyTo":"4C211A39.2080207@obry.net","subject":"Re: [PATCH v4] Do not decode url protocol.","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2010-06-22T21:39:46Z","receivedAt":"2010-06-22T21:39:46Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Pascal Obry writes:\n\n> When using the protocol git+ssh:// for example we do not want to\n> decode the '+' as a space. The url decoding must take place only\n> for the server name and parameters.\n>\n> This fixes a regression introduced in 9d2e942.\n> ---\n> url.c | 19 +++++++++++++++----\n> 1 files changed, 15 insertions(+), 4 deletions(-)\n>\n> Ok, so this is the fourth version of this patch. Thanks again Matthieu\n> for the review. I think this time I got the place for the message\n> right :)\n\nHmm, what's so unclear in \"between the --- (tripple) and the\ndiffstat.\" ;-) ? (especially the \"between\" part)\n\n> + /* Skip protocol if present. */\n> + if (with_protocol) {\n> + first_slash = strchr(*query, '/');\n> +\n> + while (q < first_slash)\n> + strbuf_addch(&out, *q++);\n> + }\n\nNothing personal, but you messed up indentation. Git indents with tabs\n(8 chars width), not 2 spaces.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"144055","messageId":"20100622224603.GA27423@coredump.intra.peff.net","threadId":"24169","inReplyTo":"4C211A39.2080207@obry.net","subject":"Re: [PATCH v4] Do not decode url protocol.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-06-22T22:46:04Z","receivedAt":"2010-06-22T22:46:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 22, 2010 at 10:16:57PM +0200, Pascal Obry wrote:\n\n> When using the protocol git+ssh:// for example we do not want to\n> decode the '+' as a space. The url decoding must take place only\n> for the server name and parameters.\n> \n> This fixes a regression introduced in 9d2e942.\n\nSorry, this is completely my fault. I obviously didn't test with\ngit+ssh. I looked at adding a new test case, but I don't think there's\nan easy way. The only way to trigger it is by using \"git+ssh\" or\n\"ssh+git\"; any other protocol (real or fake) will be handled by a custom\nprotocol handler and won't execute this broken code at all.\n\nThe patch looks reasonable (and I agree with all of Matthieu's comments\nthus far). With respect to the \"what if there is no slash\" issue\ndiscussed earlier, I prefer to be a bit defensive about possible inputs\nin library-ish functions like this. But:\n\n> +\n> + /* Skip protocol if present. */\n> + if (with_protocol) {\n> + first_slash = strchr(*query, '/');\n> +\n> + while (q < first_slash)\n> + strbuf_addch(&out, *q++);\n> + }\n> +\n\nI think this actually works OK, given that \"q < first_slash\" will be\nfalse if first_slash is NULL, assuming NULL is all-bytes-zero or some\nlow number (which is not guaranteed by the standard, but is a pretty\npractical assumption these days).\n\nHowever, you could make things a little more efficient by handing the\nwhole thing to strbuf at once:\n\n if (with_protocol) {\n const char *first_slash = strchr(q, '/');\n if (first_slash) {\n strbuf_add(&out, q, first_slash - q);\n q = first_slash;\n }\n }\n\nwhich of course needs first_slash to be checked explicitly. Not that\nthe efficiency probably matters in practice.\n\n-Peff\n"},{"id":"144079","messageId":"7v4ogtfylw.fsf@alter.siamese.dyndns.org","threadId":"24169","inReplyTo":"4C211A39.2080207@obry.net","subject":"Re: [PATCH v4] Do not decode url protocol.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-06-23T17:27:39Z","receivedAt":"2010-06-23T17:27:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Pascal Obry writes:\n\n> When using the protocol git+ssh:// for example we do not want to\n> decode the '+' as a space. The url decoding must take place only\n> for the server name and parameters.\n>\n> This fixes a regression introduced in 9d2e942.\n\nSign-off?\n\nAs the patch was whitespace-broken, I'm proposing to rewrite it like the\nfollowing.\n\n-- >8 -- \nurl.c: \"://\" part at the beginning should not be URL decoded\n\nWhen using the protocol git+ssh:// for example we do not want to\ndecode the '+' as a space. The url decoding must take place only\nfor the server name and parameters.\n\nThis fixes a regression introduced in 9d2e942.\n\nInitial-fix-by: Pascal Obry \nSigned-off-by: Junio C Hamano \n---\n url.c | 28 ++++++++++++++++++----------\n 1 files changed, 18 insertions(+), 10 deletions(-)\n\ndiff --git a/url.c b/url.c\nindex cd32b92..bf5bb9c 100644\n--- a/url.c\n+++ b/url.c\n@@ -67,12 +67,10 @@ static int url_decode_char(const char *q)\n \treturn val;\n }\n \n-static char *url_decode_internal(const char **query, const char *stop_at)\n+static char *url_decode_internal(const char **query, const char *stop_at, struct strbuf *out)\n {\n \tconst char *q = *query;\n-\tstruct strbuf out;\n \n-\tstrbuf_init(&out, 16);\n \tdo {\n \t\tunsigned char c = *q;\n \n@@ -86,33 +84,43 @@ static char *url_decode_internal(const char **query, const char *stop_at)\n \t\tif (c == '%') {\n \t\t\tint val = url_decode_char(q + 1);\n \t\t\tif (0 <= val) {\n-\t\t\t\tstrbuf_addch(&out, val);\n+\t\t\t\tstrbuf_addch(out, val);\n \t\t\t\tq += 3;\n \t\t\t\tcontinue;\n \t\t\t}\n \t\t}\n \n \t\tif (c == '+')\n-\t\t\tstrbuf_addch(&out, ' ');\n+\t\t\tstrbuf_addch(out, ' ');\n \t\telse\n-\t\t\tstrbuf_addch(&out, c);\n+\t\t\tstrbuf_addch(out, c);\n \t\tq++;\n \t} while (1);\n \t*query = q;\n-\treturn strbuf_detach(&out, NULL);\n+\treturn strbuf_detach(out, NULL);\n }\n \n char *url_decode(const char *url)\n {\n-\treturn url_decode_internal(&url, NULL);\n+\tstruct strbuf out = STRBUF_INIT;\n+\tconst char *slash = strchr(url, '/');\n+\n+\t/* Skip protocol part if present */\n+\tif (slash && url < slash) {\n+\t\tstrbuf_add(&out, url, slash - url);\n+\t\turl = slash;\n+\t}\n+\treturn url_decode_internal(&url, NULL, &out);\n }\n \n char *url_decode_parameter_name(const char **query)\n {\n-\treturn url_decode_internal(query, \"&=\");\n+\tstruct strbuf out = STRBUF_INIT;\n+\treturn url_decode_internal(query, \"&=\", &out);\n }\n \n char *url_decode_parameter_value(const char **query)\n {\n-\treturn url_decode_internal(query, \"&\");\n+\tstruct strbuf out = STRBUF_INIT;\n+\treturn url_decode_internal(query, \"&\", &out);\n }\n"},{"id":"144081","messageId":"20100623173859.GA16938@coredump.intra.peff.net","threadId":"24169","inReplyTo":"7v4ogtfylw.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v4] Do not decode url protocol.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-06-23T17:38:59Z","receivedAt":"2010-06-23T17:38:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jun 23, 2010 at 10:27:39AM -0700, Junio C Hamano wrote:\n\n> > When using the protocol git+ssh:// for example we do not want to\n> > decode the '+' as a space. The url decoding must take place only\n> > for the server name and parameters.\n> >\n> > This fixes a regression introduced in 9d2e942.\n> \n> Sign-off?\n> \n> As the patch was whitespace-broken, I'm proposing to rewrite it like the\n> following.\n> \n> -- >8 -- \n> url.c: \"://\" part at the beginning should not be URL decoded\n\nI think this is cleaner than Pascal's patch.\n\nAcked-by: Jeff King \n\nThanks all for fixing my bug. :)\n\n-Peff\n"}]}