{"thread":{"id":"36721","subject":"[PATCH 0/9] handle alternate charsets for remote http errors","startedAt":"2014-05-21T10:25:24Z","lastAt":"2014-05-23T22:34:01Z","messageCount":42,"participants":["Jeff King","Kyle J. McKay","Peter Krefting","Junio C Hamano","brian m. carlson"],"isPatch":true,"patchVersion":1,"patchTotal":9},"messages":[{"id":"242361","messageId":"20140521102524.GA30301@sigill.intra.peff.net","threadId":"36721","inReplyTo":null,"subject":"[PATCH 0/9] handle alternate charsets for remote http errors","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-05-21T10:25:24Z","receivedAt":"2014-05-21T10:25:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"As of commit 426e70d (remote-curl: show server content on http errors,\n2013-04-05), we relay any text/plain errors shown by the remote http\nserver to the user. However, we were lazy back then and left this TODO\nin place:\n\n /*\n * We only show text/plain parts, as other types are likely\n * to be ugly to look at on the user's terminal.\n *\n * TODO should handle \"; charset=XXX\", and re-encode into\n * logoutputencoding\n */\n\nThis series actually implements that, along with a few other cleanups.\n\n [1/9]: test-lib: preserve GIT_CURL_VERBOSE from the environment\n [2/9]: strbuf: add strbuf_tolower function\n [3/9]: daemon/config: factor out duplicate xstrdup_tolower\n [4/9]: http: normalize case of returned content-type\n [5/9]: t/lib-httpd: use write_script to copy CGI scripts\n [6/9]: t5550: test display of remote http error messages\n [7/9]: remote-curl: recognize text/plain with a charset parameter\n [8/9]: strbuf: add strbuf_reencode helper\n [9/9]: remote-curl: reencode http error messages\n\n-Peff\n"},{"id":"242362","messageId":"20140521102710.GA30464@sigill.intra.peff.net","threadId":"36721","inReplyTo":"20140521102524.GA30301@sigill.intra.peff.net","subject":"[PATCH 1/9] test-lib: preserve GIT_CURL_VERBOSE from the environment","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-05-21T10:27:11Z","receivedAt":"2014-05-21T10:27:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Turning on this variable can be useful when debugging http\ntests. It does break a few tests in t5541, but it is not\na variable that the user is likely to have enabled\naccidentally.\n\nSigned-off-by: Jeff King \n---\nNot necessary for this series, but I found it helpful.\n\nI took a look at making the tests in t5541 work with it, but it's a\nlittle nasty (we have to separate the curl output from the regular\nstderr). Unsetting GIT_CURL_VERBOSE for those tests would also be an\noption. Or just dropping this patch is OK with me, too; it's really only\nfor debugging.\n\n t/test-lib.sh | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex c081668..f7e55b1 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -91,6 +91,7 @@ unset VISUAL EMAIL LANGUAGE COLUMNS $(\"$PERL_PATH\" -e '\n \t\tVALGRIND\n \t\tUNZIP\n \t\tPERF_\n+\t\tCURL_VERBOSE\n \t));\n \tmy @vars = grep(/^GIT_/ && !/^GIT_($ok)/o, @env);\n \tprint join(\"\\n\", @vars);\n-- \n2.0.0.rc1.436.g03cb729\n"},{"id":"242363","messageId":"20140521102742.GB30464@sigill.intra.peff.net","threadId":"36721","inReplyTo":"20140521102524.GA30301@sigill.intra.peff.net","subject":"[PATCH 2/9] strbuf: add strbuf_tolower function","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-05-21T10:27:42Z","receivedAt":"2014-05-21T10:27:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This makes config's lowercase() function public.\n\nNote that we could continue to offer a pure-string\nlowercase, but there would be no callers (in most\npure-string cases, we actually duplicate and lowercase the\nduplicate).\n\nSigned-off-by: Jeff King \n---\nThis ones gets used later on...\n\n Documentation/technical/api-strbuf.txt | 4 ++++\n config.c | 8 +-------\n strbuf.c | 7 +++++++\n strbuf.h | 1 +\n 4 files changed, 13 insertions(+), 7 deletions(-)\n\ndiff --git a/Documentation/technical/api-strbuf.txt b/Documentation/technical/api-strbuf.txt\nindex 3350d97..8480f89 100644\n--- a/Documentation/technical/api-strbuf.txt\n+++ b/Documentation/technical/api-strbuf.txt\n@@ -125,6 +125,10 @@ Functions\n \n \tStrip whitespace from the end of a string.\n \n+`strbuf_tolower`::\n+\n+\tLowercase each character in the buffer using `tolower`.\n+\n `strbuf_cmp`::\n \n \tCompare two buffers. Returns an integer less than, equal to, or greater\ndiff --git a/config.c b/config.c\nindex a30cb5c..03ce5c6 100644\n--- a/config.c\n+++ b/config.c\n@@ -147,12 +147,6 @@ int git_config_include(const char *var, const char *value, void *data)\n \treturn ret;\n }\n \n-static void lowercase(char *p)\n-{\n-\tfor (; *p; p++)\n-\t\t*p = tolower(*p);\n-}\n-\n void git_config_push_parameter(const char *text)\n {\n \tstruct strbuf env = STRBUF_INIT;\n@@ -180,7 +174,7 @@ int git_config_parse_parameter(const char *text,\n \t\tstrbuf_list_free(pair);\n \t\treturn error(\"bogus config parameter: %s\", text);\n \t}\n-\tlowercase(pair[0]->buf);\n+\tstrbuf_tolower(pair[0]);\n \tif (fn(pair[0]->buf, pair[1] ? pair[1]->buf : NULL, data) < 0) {\n \t\tstrbuf_list_free(pair);\n \t\treturn -1;\ndiff --git a/strbuf.c b/strbuf.c\nindex ee96dcf..a1b8a47 100644\n--- a/strbuf.c\n+++ b/strbuf.c\n@@ -106,6 +106,13 @@ void strbuf_ltrim(struct strbuf *sb)\n \tsb->buf[sb->len] = '\\0';\n }\n \n+void strbuf_tolower(struct strbuf *sb)\n+{\n+\tsize_t i;\n+\tfor (i = 0; i < sb->len; i++)\n+\t\tsb->buf[i] = tolower(sb->buf[i]);\n+}\n+\n struct strbuf **strbuf_split_buf(const char *str, size_t slen,\n \t\t\t\t int terminator, int max)\n {\ndiff --git a/strbuf.h b/strbuf.h\nindex 39c14cf..6b6f745 100644\n--- a/strbuf.h\n+++ b/strbuf.h\n@@ -45,6 +45,7 @@ static inline void strbuf_setlen(struct strbuf *sb, size_t len)\n extern void strbuf_trim(struct strbuf *);\n extern void strbuf_rtrim(struct strbuf *);\n extern void strbuf_ltrim(struct strbuf *);\n+extern void strbuf_tolower(struct strbuf *sb);\n extern int strbuf_cmp(const struct strbuf *, const struct strbuf *);\n \n /*\n-- \n2.0.0.rc1.436.g03cb729\n"},{"id":"242364","messageId":"20140521102822.GC30464@sigill.intra.peff.net","threadId":"36721","inReplyTo":"20140521102524.GA30301@sigill.intra.peff.net","subject":"[PATCH 3/9] daemon/config: factor out duplicate xstrdup_tolower","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-05-21T10:28:22Z","receivedAt":"2014-05-21T10:28:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We have two implementations of the same function; let's drop\nthat to one. We take the name from daemon.c, but the\nimplementation (which is just slightly more efficient) from\nthe config code.\n\nSigned-off-by: Jeff King \n---\nUnlike the last patch, this one does _not_ get used elsewhere in this\nseries. It's just a cleanup I happened to notice while writing the other\none. It could be omitted, or applied separately.\n\n builtin/config.c | 15 +--------------\n daemon.c | 8 --------\n strbuf.c | 13 +++++++++++++\n strbuf.h | 2 ++\n 4 files changed, 16 insertions(+), 22 deletions(-)\n\ndiff --git a/builtin/config.c b/builtin/config.c\nindex 5677c94..fcd8474 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -395,19 +395,6 @@ static int urlmatch_collect_fn(const char *var, const char *value, void *cb)\n \treturn 0;\n }\n \n-static char *dup_downcase(const char *string)\n-{\n-\tchar *result;\n-\tsize_t len, i;\n-\n-\tlen = strlen(string);\n-\tresult = xmalloc(len + 1);\n-\tfor (i = 0; i < len; i++)\n-\t\tresult[i] = tolower(string[i]);\n-\tresult[i] = '\\0';\n-\treturn result;\n-}\n-\n static int get_urlmatch(const char *var, const char *url)\n {\n \tchar *section_tail;\n@@ -422,7 +409,7 @@ static int get_urlmatch(const char *var, const char *url)\n \tif (!url_normalize(url, &config.url))\n \t\tdie(\"%s\", config.url.err);\n \n-\tconfig.section = dup_downcase(var);\n+\tconfig.section = xstrdup_tolower(var);\n \tsection_tail = strchr(config.section, '.');\n \tif (section_tail) {\n \t\t*section_tail = '\\0';\ndiff --git a/daemon.c b/daemon.c\nindex eba1255..f9c63e9 100644\n--- a/daemon.c\n+++ b/daemon.c\n@@ -475,14 +475,6 @@ static void make_service_overridable(const char *name, int ena)\n \tdie(\"No such service %s\", name);\n }\n \n-static char *xstrdup_tolower(const char *str)\n-{\n-\tchar *p, *dup = xstrdup(str);\n-\tfor (p = dup; *p; p++)\n-\t\t*p = tolower(*p);\n-\treturn dup;\n-}\n-\n static void parse_host_and_port(char *hostport, char **host,\n \tchar **port)\n {\ndiff --git a/strbuf.c b/strbuf.c\nindex a1b8a47..d289d1a 100644\n--- a/strbuf.c\n+++ b/strbuf.c\n@@ -577,3 +577,16 @@ int fprintf_ln(FILE *fp, const char *fmt, ...)\n \t\treturn -1;\n \treturn ret + 1;\n }\n+\n+char *xstrdup_tolower(const char *string)\n+{\n+\tchar *result;\n+\tsize_t len, i;\n+\n+\tlen = strlen(string);\n+\tresult = xmalloc(len + 1);\n+\tfor (i = 0; i < len; i++)\n+\t\tresult[i] = tolower(string[i]);\n+\tresult[i] = '\\0';\n+\treturn result;\n+}\ndiff --git a/strbuf.h b/strbuf.h\nindex 6b6f745..25328b9 100644\n--- a/strbuf.h\n+++ b/strbuf.h\n@@ -184,4 +184,6 @@ extern int printf_ln(const char *fmt, ...);\n __attribute__((format (printf,2,3)))\n extern int fprintf_ln(FILE *fp, const char *fmt, ...);\n \n+char *xstrdup_tolower(const char *);\n+\n #endif /* STRBUF_H */\n-- \n2.0.0.rc1.436.g03cb729\n"},{"id":"242365","messageId":"20140521102917.GD30464@sigill.intra.peff.net","threadId":"36721","inReplyTo":"20140521102524.GA30301@sigill.intra.peff.net","subject":"[PATCH 4/9] http: normalize case of returned content-type","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-05-21T10:29:17Z","receivedAt":"2014-05-21T10:29:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The content-type string curl hands us comes straight from\nthe server, and may have odd capitalization. RFC 2616 states\nthat content-types are case-insensitive. We already handle\nthis when checking for text/plain (by using strcasecmp), but\ndo not when checking for a smart-http content-type.\n\nWe could simply convert the latter to use strcasecmp, but as\nwe add more parsing of the header, having it normalized will\nsimplify our parsing code.\n\nNote that there is one caveat. RFC 2616 notes that the type\nitself is case insensitive, as are parameter names. However,\nparameter valuse may be case-sensitive, depending on the\nindividual parameter. In practice, we are OK, though. We\ncurrently only look at the type itself. In the future we\nwill start looking at charset parameters, but those are also\ncase-insensitive. And it doesn't seem likely that we would\nlook at any other parameters.\n\nSigned-off-by: Jeff King \n---\nI think this is fine. If not, we can either:\n\n 1. Use strcasecmp and friends more consistently when\n parsing/comparing (later bits of the series will need to be\n adjusted).\n\n 2. Downcase here in a more context-aware way.\n\n http.c | 4 +++-\n remote-curl.c | 2 +-\n 2 files changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/http.c b/http.c\nindex 94e1afd..cd6c328 100644\n--- a/http.c\n+++ b/http.c\n@@ -957,9 +957,11 @@ static int http_request(const char *url,\n \n \tret = run_one_slot(slot, &results);\n \n-\tif (options && options->content_type)\n+\tif (options && options->content_type) {\n \t\tcurlinfo_strbuf(slot->curl, CURLINFO_CONTENT_TYPE,\n \t\t\t\toptions->content_type);\n+\t\tstrbuf_tolower(options->content_type);\n+\t}\n \n \tif (options && options->effective_url)\n \t\tcurlinfo_strbuf(slot->curl, CURLINFO_EFFECTIVE_URL,\ndiff --git a/remote-curl.c b/remote-curl.c\nindex 52c2d96..a5ab977 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -205,7 +205,7 @@ static int show_http_message(struct strbuf *type, struct strbuf *msg)\n \t * TODO should handle \"; charset=XXX\", and re-encode into\n \t * logoutputencoding\n \t */\n-\tif (strcasecmp(type->buf, \"text/plain\"))\n+\tif (strcmp(type->buf, \"text/plain\"))\n \t\treturn -1;\n \n \tstrbuf_trim(msg);\n-- \n2.0.0.rc1.436.g03cb729\n"},{"id":"242366","messageId":"20140521102941.GE30464@sigill.intra.peff.net","threadId":"36721","inReplyTo":"20140521102524.GA30301@sigill.intra.peff.net","subject":"[PATCH 5/9] t/lib-httpd: use write_script to copy CGI scripts","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-05-21T10:29:42Z","receivedAt":"2014-05-21T10:29:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Using write_script will set our shebang line appropriately\nwith $SHELL_PATH. The script that is there now is quite\nsimple and likely to succeed even with a non-POSIX /bin/sh,\nbut it does not hurt to be defensive.\n\nSigned-off-by: Jeff King \n---\n t/lib-httpd.sh | 6 +++++-\n t/lib-httpd/broken-smart-http.sh | 1 -\n 2 files changed, 5 insertions(+), 2 deletions(-)\n mode change 100755 => 100644 t/lib-httpd/broken-smart-http.sh\n\ndiff --git a/t/lib-httpd.sh b/t/lib-httpd.sh\nindex 252cbf1..8e680ef 100644\n--- a/t/lib-httpd.sh\n+++ b/t/lib-httpd.sh\n@@ -105,10 +105,14 @@ else\n \t\t\"Could not identify web server at '$LIB_HTTPD_PATH'\"\n fi\n \n+install_script () {\n+\twrite_script \"$HTTPD_ROOT_PATH/$1\" <\"$TEST_PATH/$1\"\n+}\n+\n prepare_httpd() {\n \tmkdir -p \"$HTTPD_DOCUMENT_ROOT_PATH\"\n \tcp \"$TEST_PATH\"/passwd \"$HTTPD_ROOT_PATH\"\n-\tcp \"$TEST_PATH\"/broken-smart-http.sh \"$HTTPD_ROOT_PATH\"\n+\tinstall_script broken-smart-http.sh\n \n \tln -s \"$LIB_HTTPD_MODULE_PATH\" \"$HTTPD_ROOT_PATH/modules\"\n \ndiff --git a/t/lib-httpd/broken-smart-http.sh b/t/lib-httpd/broken-smart-http.sh\nold mode 100755\nnew mode 100644\nindex f7ebfff..82cc610\n--- a/t/lib-httpd/broken-smart-http.sh\n+++ b/t/lib-httpd/broken-smart-http.sh\n@@ -1,4 +1,3 @@\n-#!/bin/sh\n printf \"Content-Type: text/%s\\n\" \"html\"\n echo\n printf \"%s\\n\" \"001e# service=git-upload-pack\"\n-- \n2.0.0.rc1.436.g03cb729\n"},{"id":"242367","messageId":"20140521102955.GF30464@sigill.intra.peff.net","threadId":"36721","inReplyTo":"20140521102524.GA30301@sigill.intra.peff.net","subject":"[PATCH 6/9] t5550: test display of remote http error messages","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-05-21T10:29:55Z","receivedAt":"2014-05-21T10:29:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Since commit 426e70d (remote-curl: show server content on\nhttp errors, 2013-04-05), we relay any text/plain error\nmessages from the remote server to the user. However, we\nnever tested it.\n\nSigned-off-by: Jeff King \n---\n t/lib-httpd.sh | 1 +\n t/lib-httpd/apache.conf | 4 ++++\n t/lib-httpd/error.sh | 17 +++++++++++++++++\n t/t5550-http-fetch-dumb.sh | 10 ++++++++++\n 4 files changed, 32 insertions(+)\n create mode 100755 t/lib-httpd/error.sh\n\ndiff --git a/t/lib-httpd.sh b/t/lib-httpd.sh\nindex 8e680ef..f7640be 100644\n--- a/t/lib-httpd.sh\n+++ b/t/lib-httpd.sh\n@@ -113,6 +113,7 @@ prepare_httpd() {\n \tmkdir -p \"$HTTPD_DOCUMENT_ROOT_PATH\"\n \tcp \"$TEST_PATH\"/passwd \"$HTTPD_ROOT_PATH\"\n \tinstall_script broken-smart-http.sh\n+\tinstall_script error.sh\n \n \tln -s \"$LIB_HTTPD_MODULE_PATH\" \"$HTTPD_ROOT_PATH/modules\"\n \ndiff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf\nindex 3a03e82..b384d79 100644\n--- a/t/lib-httpd/apache.conf\n+++ b/t/lib-httpd/apache.conf\n@@ -97,12 +97,16 @@ Alias /auth/dumb/ www/auth/dumb/\n \n ScriptAliasMatch /smart_*[^/]*/(.*) ${GIT_EXEC_PATH}/git-http-backend/$1\n ScriptAlias /broken_smart/ broken-smart-http.sh/\n+ScriptAlias /error/ error.sh/\n \n \tOptions FollowSymlinks\n \n \n \tOptions ExecCGI\n \n+\n+ Options ExecCGI\n+\n \n \tOptions ExecCGI\n \ndiff --git a/t/lib-httpd/error.sh b/t/lib-httpd/error.sh\nnew file mode 100755\nindex 0000000..786f281\n--- /dev/null\n+++ b/t/lib-httpd/error.sh\n@@ -0,0 +1,17 @@\n+#!/bin/sh\n+\n+printf \"Status: 500 Intentional Breakage\\n\"\n+\n+printf \"Content-Type: \"\n+case \"$PATH_INFO\" in\n+*html*)\n+\tprintf \"text/html\"\n+\t;;\n+*text*)\n+\tprintf \"text/plain\"\n+\t;;\n+esac\n+printf \"\\n\"\n+\n+printf \"\\n\"\n+printf \"this is the error message\\n\"\ndiff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh\nindex 1a3a2b6..13defd3 100755\n--- a/t/t5550-http-fetch-dumb.sh\n+++ b/t/t5550-http-fetch-dumb.sh\n@@ -171,5 +171,15 @@ test_expect_success 'did not use upload-pack service' '\n \ttest_cmp exp act\n '\n \n+test_expect_success 'git client shows text/plain errors' '\n+\ttest_must_fail git clone \"$HTTPD_URL/error/text\" 2>stderr &&\n+\tgrep \"this is the error message\" stderr\n+'\n+\n+test_expect_success 'git client does not show html errors' '\n+\ttest_must_fail git clone \"$HTTPD_URL/error/html\" 2>stderr &&\n+\t! grep \"this is the error message\" stderr\n+'\n+\n stop_httpd\n test_done\n-- \n2.0.0.rc1.436.g03cb729\n"},{"id":"242369","messageId":"20140521103301.GG30464@sigill.intra.peff.net","threadId":"36721","inReplyTo":"20140521102524.GA30301@sigill.intra.peff.net","subject":"[PATCH 7/9] remote-curl: recognize text/plain with a charset parameter","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-05-21T10:33:01Z","receivedAt":"2014-05-21T10:33:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Commit 426e70d (remote-curl: show server content on http\nerrors, 2013-04-05) tried to recognize text/plain content\ntypes, but fails to do so if they have any parameters.\n\nThis patches makes our parsing more liberal, though we still\ndo not do anything useful with a charset parameter.\n\nSigned-off-by: Jeff King \n---\n remote-curl.c | 26 ++++++++++++++++++--------\n t/lib-httpd/error.sh | 8 +++++++-\n t/t5550-http-fetch-dumb.sh | 5 +++++\n 3 files changed, 30 insertions(+), 9 deletions(-)\n\ndiff --git a/remote-curl.c b/remote-curl.c\nindex a5ab977..6d1b206 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -194,20 +194,30 @@ static void free_discovery(struct discovery *d)\n \t}\n }\n \n+/*\n+ * We only show text/plain parts, as other types are likely\n+ * to be ugly to look at on the user's terminal.\n+ */\n+static int content_type_is_terminal_friendly(struct strbuf *type)\n+{\n+\tconst char *p;\n+\n+\tp = skip_prefix(type->buf, \"text/plain\");\n+\tif (!p || (*p && *p != ';'))\n+\t\treturn 0;\n+\n+\treturn 1;\n+}\n+\n static int show_http_message(struct strbuf *type, struct strbuf *msg)\n {\n \tconst char *p, *eol;\n \n-\t/*\n-\t * We only show text/plain parts, as other types are likely\n-\t * to be ugly to look at on the user's terminal.\n-\t *\n-\t * TODO should handle \"; charset=XXX\", and re-encode into\n-\t * logoutputencoding\n-\t */\n-\tif (strcmp(type->buf, \"text/plain\"))\n+\tif (!content_type_is_terminal_friendly(type))\n \t\treturn -1;\n \n+\t/* TODO should record charset and reencode msg to logOutputEncoding */\n+\n \tstrbuf_trim(msg);\n \tif (!msg->len)\n \t\treturn -1;\ndiff --git a/t/lib-httpd/error.sh b/t/lib-httpd/error.sh\nindex 786f281..02e80b3 100755\n--- a/t/lib-httpd/error.sh\n+++ b/t/lib-httpd/error.sh\n@@ -3,6 +3,7 @@\n printf \"Status: 500 Intentional Breakage\\n\"\n \n printf \"Content-Type: \"\n+charset=iso8859-1\n case \"$PATH_INFO\" in\n *html*)\n \tprintf \"text/html\"\n@@ -10,8 +11,13 @@ case \"$PATH_INFO\" in\n *text*)\n \tprintf \"text/plain\"\n \t;;\n+*charset*)\n+\tprintf \"text/plain; charset=utf-8\"\n+\tcharset=utf-8\n+\t;;\n esac\n printf \"\\n\"\n \n printf \"\\n\"\n-printf \"this is the error message\\n\"\n+printf \"this is the error message\\n\" |\n+iconv -f us-ascii -t $charset\ndiff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh\nindex 13defd3..b35b261 100755\n--- a/t/t5550-http-fetch-dumb.sh\n+++ b/t/t5550-http-fetch-dumb.sh\n@@ -181,5 +181,10 @@ test_expect_success 'git client does not show html errors' '\n \t! grep \"this is the error message\" stderr\n '\n \n+test_expect_success 'git client shows text/plain with a charset' '\n+\ttest_must_fail git clone \"$HTTPD_URL/error/charset\" 2>stderr &&\n+\tgrep \"this is the error message\" stderr\n+'\n+\n stop_httpd\n test_done\n-- \n2.0.0.rc1.436.g03cb729\n"},{"id":"242370","messageId":"20140521103324.GH30464@sigill.intra.peff.net","threadId":"36721","inReplyTo":"20140521102524.GA30301@sigill.intra.peff.net","subject":"[PATCH 8/9] strbuf: add strbuf_reencode helper","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-05-21T10:33:24Z","receivedAt":"2014-05-21T10:33:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This is a convenience wrapper around `reencode_string_len`\nand `strbuf_attach`.\n\nSigned-off-by: Jeff King \n---\n Documentation/technical/api-strbuf.txt | 5 +++++\n strbuf.c | 17 +++++++++++++++++\n strbuf.h | 1 +\n 3 files changed, 23 insertions(+)\n\ndiff --git a/Documentation/technical/api-strbuf.txt b/Documentation/technical/api-strbuf.txt\nindex 8480f89..a468816 100644\n--- a/Documentation/technical/api-strbuf.txt\n+++ b/Documentation/technical/api-strbuf.txt\n@@ -129,6 +129,11 @@ Functions\n \n \tLowercase each character in the buffer using `tolower`.\n \n+`strbuf_reencode`::\n+\n+\tReplace the contents of the strbuf with a reencoded form. Returns -1\n+\ton error, 0 on success.\n+\n `strbuf_cmp`::\n \n \tCompare two buffers. Returns an integer less than, equal to, or greater\ndiff --git a/strbuf.c b/strbuf.c\nindex d289d1a..a717d9b 100644\n--- a/strbuf.c\n+++ b/strbuf.c\n@@ -1,5 +1,6 @@\n #include \"cache.h\"\n #include \"refs.h\"\n+#include \"utf8.h\"\n \n int starts_with(const char *str, const char *prefix)\n {\n@@ -113,6 +114,22 @@ void strbuf_tolower(struct strbuf *sb)\n \t\tsb->buf[i] = tolower(sb->buf[i]);\n }\n \n+int strbuf_reencode(struct strbuf *sb, const char *from, const char *to)\n+{\n+\tchar *out;\n+\tint len;\n+\n+\tif (same_encoding(from, to))\n+\t\treturn 0;\n+\n+\tout = reencode_string_len(sb->buf, sb->len, to, from, &len);\n+\tif (!out)\n+\t\treturn -1;\n+\n+\tstrbuf_attach(sb, out, len, len);\n+\treturn 0;\n+}\n+\n struct strbuf **strbuf_split_buf(const char *str, size_t slen,\n \t\t\t\t int terminator, int max)\n {\ndiff --git a/strbuf.h b/strbuf.h\nindex 25328b9..a068dd6 100644\n--- a/strbuf.h\n+++ b/strbuf.h\n@@ -46,6 +46,7 @@ extern void strbuf_trim(struct strbuf *);\n extern void strbuf_rtrim(struct strbuf *);\n extern void strbuf_ltrim(struct strbuf *);\n extern void strbuf_tolower(struct strbuf *sb);\n+extern int strbuf_reencode(struct strbuf *sb, const char *from, const char *to);\n extern int strbuf_cmp(const struct strbuf *, const struct strbuf *);\n \n /*\n-- \n2.0.0.rc1.436.g03cb729\n"},{"id":"242371","messageId":"20140521103353.GI30464@sigill.intra.peff.net","threadId":"36721","inReplyTo":"20140521102524.GA30301@sigill.intra.peff.net","subject":"[PATCH 9/9] remote-curl: reencode http error messages","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-05-21T10:33:53Z","receivedAt":"2014-05-21T10:33:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"As of the last commit, we now recognize an error message\nwith a content-type \"text/plain; charset=utf-16\" as text,\nbut we ignore the charset parameter entirely. Let's encode\nit to log_output_encoding, which is presumably something the\nuser's terminal can handle.\n\nSigned-off-by: Jeff King \n---\n remote-curl.c | 37 +++++++++++++++++++++++++++++++++----\n t/lib-httpd/error.sh | 4 ++++\n t/t5550-http-fetch-dumb.sh | 5 +++++\n 3 files changed, 42 insertions(+), 4 deletions(-)\n\ndiff --git a/remote-curl.c b/remote-curl.c\nindex 6d1b206..1dc90d7 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -194,11 +194,34 @@ static void free_discovery(struct discovery *d)\n \t}\n }\n \n+static char *find_param(const char *str, const char *name)\n+{\n+\tint len = strlen(name);\n+\n+\tfor (; *str; str++) {\n+\t\tconst char *p;\n+\n+\t\tif (*p++ != ' ')\n+\t\t\tcontinue;\n+\n+\t\tif (strncmp(p, name, len))\n+\t\t\tcontinue;\n+\t\tp += len;\n+\n+\t\tif (*p++ != '=')\n+\t\t\tcontinue;\n+\n+\t\treturn xstrndup(p, strchrnul(p, ' ') - p);\n+\t}\n+\n+\treturn NULL;\n+}\n+\n /*\n * We only show text/plain parts, as other types are likely\n * to be ugly to look at on the user's terminal.\n */\n-static int content_type_is_terminal_friendly(struct strbuf *type)\n+static int content_type_is_terminal_friendly(struct strbuf *type, char **charset)\n {\n \tconst char *p;\n \n@@ -206,17 +229,23 @@ static int content_type_is_terminal_friendly(struct strbuf *type)\n \tif (!p || (*p && *p != ';'))\n \t\treturn 0;\n \n+\t*charset = find_param(p, \"charset\");\n+\t/* default charset from rfc2616 */\n+\tif (!*charset)\n+\t\t*charset = xstrdup(\"iso8859-1\");\n+\n \treturn 1;\n }\n \n static int show_http_message(struct strbuf *type, struct strbuf *msg)\n {\n \tconst char *p, *eol;\n+\tchar *charset;\n \n-\tif (!content_type_is_terminal_friendly(type))\n+\tif (!content_type_is_terminal_friendly(type, &charset))\n \t\treturn -1;\n-\n-\t/* TODO should record charset and reencode msg to logOutputEncoding */\n+\tstrbuf_reencode(msg, charset, get_log_output_encoding());\n+\tfree(charset);\n \n \tstrbuf_trim(msg);\n \tif (!msg->len)\ndiff --git a/t/lib-httpd/error.sh b/t/lib-httpd/error.sh\nindex 02e80b3..4efbce7 100755\n--- a/t/lib-httpd/error.sh\n+++ b/t/lib-httpd/error.sh\n@@ -15,6 +15,10 @@ case \"$PATH_INFO\" in\n \tprintf \"text/plain; charset=utf-8\"\n \tcharset=utf-8\n \t;;\n+*utf16*)\n+\tprintf \"text/plain; charset=utf-16\"\n+\tcharset=utf-16\n+\t;;\n esac\n printf \"\\n\"\n \ndiff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh\nindex b35b261..01b8aae 100755\n--- a/t/t5550-http-fetch-dumb.sh\n+++ b/t/t5550-http-fetch-dumb.sh\n@@ -186,5 +186,10 @@ test_expect_success 'git client shows text/plain with a charset' '\n \tgrep \"this is the error message\" stderr\n '\n \n+test_expect_success 'http error messages are reencoded' '\n+\ttest_must_fail git clone \"$HTTPD_URL/error/utf16\" 2>stderr &&\n+\tgrep \"this is the error message\" stderr\n+'\n+\n stop_httpd\n test_done\n-- \n2.0.0.rc1.436.g03cb729\n"},{"id":"242444","messageId":"AD42BA87-3C73-4EB2-AF88-96C4F7E6FA0C@gmail.com","threadId":"36721","inReplyTo":"20140521102742.GB30464@sigill.intra.peff.net","subject":"Re: [PATCH 2/9] strbuf: add strbuf_tolower function","fromName":"Kyle J. McKay","fromEmail":"mackyle@gmail.com","sentAt":"2014-05-22T00:07:36Z","receivedAt":"2014-05-22T00:07:36Z","isPatch":true,"sender":{"key":"mackyle@gmail.com","avatar":"https://avatars.githubusercontent.com/u/813346?v=4"},"body":"On May 21, 2014, at 03:27, Jeff King wrote:\n\n> This makes config's lowercase() function public.\n>\n> Note that we could continue to offer a pure-string\n> lowercase, but there would be no callers (in most\n> pure-string cases, we actually duplicate and lowercase the\n> duplicate).\n>\n> Signed-off-by: Jeff King \n> ---\n> This ones gets used later on...\n>\n> Documentation/technical/api-strbuf.txt | 4 ++++\n> config.c | 8 +-------\n> strbuf.c | 7 +++++++\n> strbuf.h | 1 +\n> 4 files changed, 13 insertions(+), 7 deletions(-)\n>\n> diff --git a/Documentation/technical/api-strbuf.txt b/Documentation/ \n> technical/api-strbuf.txt\n> index 3350d97..8480f89 100644\n> --- a/Documentation/technical/api-strbuf.txt\n> +++ b/Documentation/technical/api-strbuf.txt\n> @@ -125,6 +125,10 @@ Functions\n>\n> \tStrip whitespace from the end of a string.\n>\n> +`strbuf_tolower`::\n> +\n> +\tLowercase each character in the buffer using `tolower`.\n> +\n> `strbuf_cmp`::\n>\n> \tCompare two buffers. Returns an integer less than, equal to, or \n> greater\n> diff --git a/config.c b/config.c\n> index a30cb5c..03ce5c6 100644\n> --- a/config.c\n> +++ b/config.c\n> @@ -147,12 +147,6 @@ int git_config_include(const char *var, const \n> char *value, void *data)\n> \treturn ret;\n> }\n>\n> -static void lowercase(char *p)\n> -{\n> -\tfor (; *p; p++)\n> -\t\t*p = tolower(*p);\n> -}\n> -\n> void git_config_push_parameter(const char *text)\n> {\n> \tstruct strbuf env = STRBUF_INIT;\n> @@ -180,7 +174,7 @@ int git_config_parse_parameter(const char *text,\n> \t\tstrbuf_list_free(pair);\n> \t\treturn error(\"bogus config parameter: %s\", text);\n> \t}\n> -\tlowercase(pair[0]->buf);\n> +\tstrbuf_tolower(pair[0]);\n> \tif (fn(pair[0]->buf, pair[1] ? pair[1]->buf : NULL, data) < 0) {\n> \t\tstrbuf_list_free(pair);\n> \t\treturn -1;\n> diff --git a/strbuf.c b/strbuf.c\n> index ee96dcf..a1b8a47 100644\n> --- a/strbuf.c\n> +++ b/strbuf.c\n> @@ -106,6 +106,13 @@ void strbuf_ltrim(struct strbuf *sb)\n> \tsb->buf[sb->len] = '\\0';\n> }\n>\n> +void strbuf_tolower(struct strbuf *sb)\n> +{\n> +\tsize_t i;\n> +\tfor (i = 0; i < sb->len; i++)\n> +\t\tsb->buf[i] = tolower(sb->buf[i]);\n> +}\n> +\n\nWouldn't a direct transfer of the lowercase function be something more \nlike:\n\n\nvoid strbuf_tolower(struct strbuf *sb)\n{\n\tchar *p = sb->buf;\n\tfor (; *p; p++)\n\t\t*p = tolower(*p);\n}\n\nThat seems to me to be a bit more efficient. According to the \ncomments in strbuf.c, \"people can always assume buf is non NULL and - \n >buf is NUL terminated even for a freshly initialized strbuf.\"\n\n> struct strbuf **strbuf_split_buf(const char *str, size_t slen,\n> \t\t\t\t int terminator, int max)\n> {\n> diff --git a/strbuf.h b/strbuf.h\n> index 39c14cf..6b6f745 100644\n> --- a/strbuf.h\n> +++ b/strbuf.h\n> @@ -45,6 +45,7 @@ static inline void strbuf_setlen(struct strbuf \n> *sb, size_t len)\n> extern void strbuf_trim(struct strbuf *);\n> extern void strbuf_rtrim(struct strbuf *);\n> extern void strbuf_ltrim(struct strbuf *);\n> +extern void strbuf_tolower(struct strbuf *sb);\n> extern int strbuf_cmp(const struct strbuf *, const struct strbuf *);\n>\n> /*\n> -- \n> 2.0.0.rc1.436.g03cb729\n>\n"},{"id":"242445","messageId":"90CE006F-EE74-40D2-8847-507E37021D84@gmail.com","threadId":"36721","inReplyTo":"20140521103301.GG30464@sigill.intra.peff.net","subject":"Re: [PATCH 7/9] remote-curl: recognize text/plain with a charset parameter","fromName":"Kyle J. McKay","fromEmail":"mackyle@gmail.com","sentAt":"2014-05-22T00:07:38Z","receivedAt":"2014-05-22T00:07:38Z","isPatch":true,"sender":{"key":"mackyle@gmail.com","avatar":"https://avatars.githubusercontent.com/u/813346?v=4"},"body":"On May 21, 2014, at 03:33, Jeff King wrote:\n\n> Commit 426e70d (remote-curl: show server content on http\n> errors, 2013-04-05) tried to recognize text/plain content\n> types, but fails to do so if they have any parameters.\n>\n> This patches makes our parsing more liberal, though we still\n> do not do anything useful with a charset parameter.\n>\n> Signed-off-by: Jeff King \n> ---\n> remote-curl.c | 26 ++++++++++++++++++--------\n> t/lib-httpd/error.sh | 8 +++++++-\n> t/t5550-http-fetch-dumb.sh | 5 +++++\n> 3 files changed, 30 insertions(+), 9 deletions(-)\n>\n> diff --git a/remote-curl.c b/remote-curl.c\n> index a5ab977..6d1b206 100644\n> --- a/remote-curl.c\n> +++ b/remote-curl.c\n> @@ -194,20 +194,30 @@ static void free_discovery(struct discovery *d)\n> \t}\n> }\n>\n> +/*\n> + * We only show text/plain parts, as other types are likely\n> + * to be ugly to look at on the user's terminal.\n> + */\n> +static int content_type_is_terminal_friendly(struct strbuf *type)\n> +{\n> +\tconst char *p;\n> +\n> +\tp = skip_prefix(type->buf, \"text/plain\");\n> +\tif (!p || (*p && *p != ';'))\n> +\t\treturn 0;\n> +\n> +\treturn 1;\n> +}\n> +\n\nI think that a strict reading of RFC 2616 allows \"text/plain ; \ncharset=utf-8\" as well as \"text/plain;charset=utf-8\" and \"text/plain; \ncharset=utf-8\". So perhaps this if line instead:\n\n+\tif (!p || (*p && *p != ';' && *p != ' ' && *p != '\\t'))\n\nSee RFC 2616 section 2.2 for the definition of linear white space \n(LWS) and the end of section 2.1 for the \"implied *LWS\" rule that \nallows it.\n\n> static int show_http_message(struct strbuf *type, struct strbuf *msg)\n> {\n> \tconst char *p, *eol;\n>\n> -\t/*\n> -\t * We only show text/plain parts, as other types are likely\n> -\t * to be ugly to look at on the user's terminal.\n> -\t *\n> -\t * TODO should handle \"; charset=XXX\", and re-encode into\n> -\t * logoutputencoding\n> -\t */\n> -\tif (strcmp(type->buf, \"text/plain\"))\n> +\tif (!content_type_is_terminal_friendly(type))\n> \t\treturn -1;\n>\n> +\t/* TODO should record charset and reencode msg to \n> logOutputEncoding */\n> +\n> \tstrbuf_trim(msg);\n> \tif (!msg->len)\n> \t\treturn -1;\n> diff --git a/t/lib-httpd/error.sh b/t/lib-httpd/error.sh\n> index 786f281..02e80b3 100755\n> --- a/t/lib-httpd/error.sh\n> +++ b/t/lib-httpd/error.sh\n> @@ -3,6 +3,7 @@\n> printf \"Status: 500 Intentional Breakage\\n\"\n>\n> printf \"Content-Type: \"\n> +charset=iso8859-1\n> case \"$PATH_INFO\" in\n> *html*)\n> \tprintf \"text/html\"\n> @@ -10,8 +11,13 @@ case \"$PATH_INFO\" in\n> *text*)\n> \tprintf \"text/plain\"\n> \t;;\n> +*charset*)\n> +\tprintf \"text/plain; charset=utf-8\"\n> +\tcharset=utf-8\n> +\t;;\n> esac\n> printf \"\\n\"\n>\n> printf \"\\n\"\n> -printf \"this is the error message\\n\"\n> +printf \"this is the error message\\n\" |\n> +iconv -f us-ascii -t $charset\n> diff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh\n> index 13defd3..b35b261 100755\n> --- a/t/t5550-http-fetch-dumb.sh\n> +++ b/t/t5550-http-fetch-dumb.sh\n> @@ -181,5 +181,10 @@ test_expect_success 'git client does not show \n> html errors' '\n> \t! grep \"this is the error message\" stderr\n> '\n>\n> +test_expect_success 'git client shows text/plain with a charset' '\n> +\ttest_must_fail git clone \"$HTTPD_URL/error/charset\" 2>stderr &&\n> +\tgrep \"this is the error message\" stderr\n> +'\n> +\n> stop_httpd\n> test_done\n> -- \n> 2.0.0.rc1.436.g03cb729\n>\n"},{"id":"242446","messageId":"EF689296-BB9E-4734-A044-25DEF1874884@gmail.com","threadId":"36721","inReplyTo":"20140521103353.GI30464@sigill.intra.peff.net","subject":"Re: [PATCH 9/9] remote-curl: reencode http error messages","fromName":"Kyle J. McKay","fromEmail":"mackyle@gmail.com","sentAt":"2014-05-22T00:07:40Z","receivedAt":"2014-05-22T00:07:40Z","isPatch":true,"sender":{"key":"mackyle@gmail.com","avatar":"https://avatars.githubusercontent.com/u/813346?v=4"},"body":"On May 21, 2014, at 03:33, Jeff King wrote:\n\n> As of the last commit, we now recognize an error message\n> with a content-type \"text/plain; charset=utf-16\" as text,\n> but we ignore the charset parameter entirely. Let's encode\n> it to log_output_encoding, which is presumably something the\n> user's terminal can handle.\n>\n> Signed-off-by: Jeff King \n> ---\n> remote-curl.c | 37 +++++++++++++++++++++++++++++++++----\n> t/lib-httpd/error.sh | 4 ++++\n> t/t5550-http-fetch-dumb.sh | 5 +++++\n> 3 files changed, 42 insertions(+), 4 deletions(-)\n>\n> diff --git a/remote-curl.c b/remote-curl.c\n> index 6d1b206..1dc90d7 100644\n> --- a/remote-curl.c\n> +++ b/remote-curl.c\n> @@ -194,11 +194,34 @@ static void free_discovery(struct discovery *d)\n> \t}\n> }\n>\n> +static char *find_param(const char *str, const char *name)\n> +{\n> +\tint len = strlen(name);\n> +\n> +\tfor (; *str; str++) {\n> +\t\tconst char *p;\n> +\n> +\t\tif (*p++ != ' ')\n> +\t\t\tcontinue;\n> +\n> +\t\tif (strncmp(p, name, len))\n> +\t\t\tcontinue;\n> +\t\tp += len;\n> +\n> +\t\tif (*p++ != '=')\n> +\t\t\tcontinue;\n> +\n> +\t\treturn xstrndup(p, strchrnul(p, ' ') - p);\n> +\t}\n> +\n> +\treturn NULL;\n> +}\n> +\n> /*\n> * We only show text/plain parts, as other types are likely\n> * to be ugly to look at on the user's terminal.\n> */\n> -static int content_type_is_terminal_friendly(struct strbuf *type)\n> +static int content_type_is_terminal_friendly(struct strbuf *type, \n> char **charset)\n> {\n> \tconst char *p;\n>\n> @@ -206,17 +229,23 @@ static int \n> content_type_is_terminal_friendly(struct strbuf *type)\n> \tif (!p || (*p && *p != ';'))\n> \t\treturn 0;\n>\n> +\t*charset = find_param(p, \"charset\");\n> +\t/* default charset from rfc2616 */\n> +\tif (!*charset)\n> +\t\t*charset = xstrdup(\"iso8859-1\");\n\nActually the name should be \"ISO-8859-1\". See RFC 2616 section \n3.7.1. Since it's case insensitive \"iso-8859-1\" would be fine too.\n\n> +\n> \treturn 1;\n> }\n>\n> static int show_http_message(struct strbuf *type, struct strbuf *msg)\n> {\n> \tconst char *p, *eol;\n> +\tchar *charset;\n>\n> -\tif (!content_type_is_terminal_friendly(type))\n> +\tif (!content_type_is_terminal_friendly(type, &charset))\n> \t\treturn -1;\n> -\n> -\t/* TODO should record charset and reencode msg to \n> logOutputEncoding */\n> +\tstrbuf_reencode(msg, charset, get_log_output_encoding());\n> +\tfree(charset);\n>\n> \tstrbuf_trim(msg);\n> \tif (!msg->len)\n> diff --git a/t/lib-httpd/error.sh b/t/lib-httpd/error.sh\n> index 02e80b3..4efbce7 100755\n> --- a/t/lib-httpd/error.sh\n> +++ b/t/lib-httpd/error.sh\n> @@ -15,6 +15,10 @@ case \"$PATH_INFO\" in\n> \tprintf \"text/plain; charset=utf-8\"\n> \tcharset=utf-8\n> \t;;\n> +*utf16*)\n> +\tprintf \"text/plain; charset=utf-16\"\n> +\tcharset=utf-16\n> +\t;;\n> esac\n> printf \"\\n\"\n>\n> diff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh\n> index b35b261..01b8aae 100755\n> --- a/t/t5550-http-fetch-dumb.sh\n> +++ b/t/t5550-http-fetch-dumb.sh\n> @@ -186,5 +186,10 @@ test_expect_success 'git client shows text/ \n> plain with a charset' '\n> \tgrep \"this is the error message\" stderr\n> '\n>\n> +test_expect_success 'http error messages are reencoded' '\n> +\ttest_must_fail git clone \"$HTTPD_URL/error/utf16\" 2>stderr &&\n> +\tgrep \"this is the error message\" stderr\n> +'\n> +\n> stop_httpd\n> test_done\n> -- \n> 2.0.0.rc1.436.g03cb729\n"},{"id":"242455","messageId":"20140522055852.GA16587@sigill.intra.peff.net","threadId":"36721","inReplyTo":"AD42BA87-3C73-4EB2-AF88-96C4F7E6FA0C@gmail.com","subject":"Re: [PATCH 2/9] strbuf: add strbuf_tolower function","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-05-22T05:58:53Z","receivedAt":"2014-05-22T05:58:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 21, 2014 at 05:07:36PM -0700, Kyle J. McKay wrote:\n\n> >+void strbuf_tolower(struct strbuf *sb)\n> >+{\n> >+\tsize_t i;\n> >+\tfor (i = 0; i < sb->len; i++)\n> >+\t\tsb->buf[i] = tolower(sb->buf[i]);\n> >+}\n> >+\n> \n> Wouldn't a direct transfer of the lowercase function be something more like:\n> \n> \n> void strbuf_tolower(struct strbuf *sb)\n> {\n> \tchar *p = sb->buf;\n> \tfor (; *p; p++)\n> \t\t*p = tolower(*p);\n> }\n> \n> That seems to me to be a bit more efficient. According to the comments in\n> strbuf.c, \"people can always assume buf is non NULL and ->buf is NUL\n> terminated even for a freshly initialized strbuf.\"\n\nYes, and that would be fine with me (I actually wrote strbuf_tolower for\nmy own use, and _then_ realized that we already had such a thing that\ncould be replaced).\n\n-Peff\n"},{"id":"242456","messageId":"20140522060516.GB16587@sigill.intra.peff.net","threadId":"36721","inReplyTo":"90CE006F-EE74-40D2-8847-507E37021D84@gmail.com","subject":"Re: [PATCH 7/9] remote-curl: recognize text/plain with a charset parameter","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-05-22T06:05:16Z","receivedAt":"2014-05-22T06:05:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 21, 2014 at 05:07:38PM -0700, Kyle J. McKay wrote:\n\n> >+\tp = skip_prefix(type->buf, \"text/plain\");\n> >+\tif (!p || (*p && *p != ';'))\n> >+\t\treturn 0;\n> >+\n> >+\treturn 1;\n> >+}\n> >+\n> \n> I think that a strict reading of RFC 2616 allows \"text/plain ;\n> charset=utf-8\" as well as \"text/plain;charset=utf-8\" and \"text/plain;\n> charset=utf-8\". So perhaps this if line instead:\n> \n> +\tif (!p || (*p && *p != ';' && *p != ' ' && *p != '\\t'))\n> \n> See RFC 2616 section 2.2 for the definition of linear white space (LWS) and\n> the end of section 2.1 for the \"implied *LWS\" rule that allows it.\n\nYou're right. There are a few exceptions in 3.7:\n\n Linear white space (LWS) MUST NOT be used between the type and\n subtype, nor between an attribute and its value.\n\nbut it does not include between the subtype and parameter. So the\n\"find_parameter\" also needs to accept the collapsed whitespace, too, I\nguess.\n\n-Peff\n"},{"id":"242457","messageId":"20140522060559.GC16587@sigill.intra.peff.net","threadId":"36721","inReplyTo":"EF689296-BB9E-4734-A044-25DEF1874884@gmail.com","subject":"Re: [PATCH 9/9] remote-curl: reencode http error messages","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-05-22T06:05:59Z","receivedAt":"2014-05-22T06:05:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 21, 2014 at 05:07:40PM -0700, Kyle J. McKay wrote:\n\n> >+\t/* default charset from rfc2616 */\n> >+\tif (!*charset)\n> >+\t\t*charset = xstrdup(\"iso8859-1\");\n> \n> Actually the name should be \"ISO-8859-1\". See RFC 2616 section 3.7.1.\n> Since it's case insensitive \"iso-8859-1\" would be fine too.\n\nThanks, will fix.\n\n-Peff\n"},{"id":"242459","messageId":"alpine.DEB.2.00.1405220809370.10958@ds9.cixit.se","threadId":"36721","inReplyTo":"90CE006F-EE74-40D2-8847-507E37021D84@gmail.com","subject":"Re: [PATCH 7/9] remote-curl: recognize text/plain with a charset parameter","fromName":"Peter Krefting","fromEmail":"peter@softwolves.pp.se","sentAt":"2014-05-22T07:12:58Z","receivedAt":"2014-05-22T07:12:58Z","isPatch":true,"sender":{"key":"peter@softwolves.pp.se","avatar":"https://avatars.githubusercontent.com/u/990764?v=4"},"body":"Kyle J. McKay:\n\n> I think that a strict reading of RFC 2616 allows \"text/plain ; charset=utf-8\" \n> as well as \"text/plain;charset=utf-8\" and \"text/plain; charset=utf-8\".\n\nIt does indeed, and I have seen servers send both variants, so they do \nneed to be catered for.\n\nThe number of servers that would actually send the charset attribute \nhere (for error messages) are probably not that many. It is probably a \ngood idea to make the default user-configurable (I know the specs \nstate that anything undeclared is iso-8859-1, but the real world \ndoesn't agree to that).\n\n-- \n\\\\// Peter - http://www.softwolves.pp.se/\n"},{"id":"242460","messageId":"alpine.DEB.2.00.1405220816050.10958@ds9.cixit.se","threadId":"36721","inReplyTo":"EF689296-BB9E-4734-A044-25DEF1874884@gmail.com","subject":"Re: [PATCH 9/9] remote-curl: reencode http error messages","fromName":"Peter Krefting","fromEmail":"peter@softwolves.pp.se","sentAt":"2014-05-22T07:26:17Z","receivedAt":"2014-05-22T07:26:17Z","isPatch":true,"sender":{"key":"peter@softwolves.pp.se","avatar":"https://avatars.githubusercontent.com/u/990764?v=4"},"body":"Kyle J. McKay:\n\n>> +\tif (!*charset)\n>> +\t\t*charset = xstrdup(\"iso8859-1\");\n>\n> Actually the name should be \"ISO-8859-1\". See RFC 2616 section 3.7.1. Since \n> it's case insensitive \"iso-8859-1\" would be fine too.\n\nYou'd be amazed at what you see in the wild... I'd recommend going \nwith the recommended algorithm from WHATWG's Encoding Standard, if you \nwant to make this robust: \n.\n\nThe spec is partly based on a lot of research I made in my previous \n$DAYJOB, with a lot of research added by the spec writer.\n\nThere is also Unicode's attempt at it, but it does unfortunately \nproduce too many false positives: \n\n-- \n\\\\// Peter - http://www.softwolves.pp.se/\n"},{"id":"242461","messageId":"21F11C1A-4C69-4633-9DC7-E9134D38856E@gmail.com","threadId":"36721","inReplyTo":"20140522060516.GB16587@sigill.intra.peff.net","subject":"Re: [PATCH 7/9] remote-curl: recognize text/plain with a charset parameter","fromName":"Kyle J. McKay","fromEmail":"mackyle@gmail.com","sentAt":"2014-05-22T07:27:38Z","receivedAt":"2014-05-22T07:27:38Z","isPatch":true,"sender":{"key":"mackyle@gmail.com","avatar":"https://avatars.githubusercontent.com/u/813346?v=4"},"body":"On May 21, 2014, at 23:05, Jeff King wrote:\n\n> On Wed, May 21, 2014 at 05:07:38PM -0700, Kyle J. McKay wrote:\n>\n>>> +\tp = skip_prefix(type->buf, \"text/plain\");\n>>> +\tif (!p || (*p && *p != ';'))\n>>> +\t\treturn 0;\n>>> +\n>>> +\treturn 1;\n>>> +}\n>>> +\n>>\n>> I think that a strict reading of RFC 2616 allows \"text/plain ;\n>> charset=utf-8\" as well as \"text/plain;charset=utf-8\" and \"text/plain;\n>> charset=utf-8\". So perhaps this if line instead:\n>>\n>> +\tif (!p || (*p && *p != ';' && *p != ' ' && *p != '\\t'))\n>>\n>> See RFC 2616 section 2.2 for the definition of linear white space \n>> (LWS) and\n>> the end of section 2.1 for the \"implied *LWS\" rule that allows it.\n>\n> You're right. There are a few exceptions in 3.7:\n>\n> Linear white space (LWS) MUST NOT be used between the type and\n> subtype, nor between an attribute and its value.\n>\n> but it does not include between the subtype and parameter. So the\n> \"find_parameter\" also needs to accept the collapsed whitespace, too, I\n> guess.\n\nYeah I think so too. It's probably enough though just to just strip \nall \" \" and \"\\t\" characters at the same time the content type is \nlowercased. While that would cause invalid content types such as \n\"text / plain\" to be recognized it would keep the rest of the code \nsimpler. Since a producer of an invalid content type shouldn't really \nbe depending on any particular behavior by a receiver of an invalid \ncontent type it's probably not an issue.\n\n--Kyle\n"},{"id":"242463","messageId":"20140522090259.GA29669@sigill.intra.peff.net","threadId":"36721","inReplyTo":"21F11C1A-4C69-4633-9DC7-E9134D38856E@gmail.com","subject":"Re: [PATCH 7/9] remote-curl: recognize text/plain with a charset parameter","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-05-22T09:02:59Z","receivedAt":"2014-05-22T09:02:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 22, 2014 at 12:27:38AM -0700, Kyle J. McKay wrote:\n\n> Yeah I think so too. It's probably enough though just to just strip all \" \"\n> and \"\\t\" characters at the same time the content type is lowercased. While\n> that would cause invalid content types such as \"text / plain\" to be\n> recognized it would keep the rest of the code simpler. Since a producer of\n> an invalid content type shouldn't really be depending on any particular\n> behavior by a receiver of an invalid content type it's probably not an\n> issue.\n\nI think you have to retain whitespace between parameters. Not that I\nexpect there to be multiple parameters in a text/plain, but it's\nallowed.\n\nI started doing this path of trying to produce a normalized version, but\nfound that it was just as much code as parsing at all (and it still\nleaves the calling code to do its own parse). Instead, what I ended up\nwith was just doing the parsing in http.c into nice, normalized chunks,\nand then the calling code can compare them with simple strcmps.\n\nI'll post the patches in a few minutes.\n\n-Peff\n"},{"id":"242464","messageId":"20140522090557.GB29669@sigill.intra.peff.net","threadId":"36721","inReplyTo":"alpine.DEB.2.00.1405220809370.10958@ds9.cixit.se","subject":"Re: [PATCH 7/9] remote-curl: recognize text/plain with a charset parameter","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-05-22T09:05:57Z","receivedAt":"2014-05-22T09:05:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 22, 2014 at 08:12:58AM +0100, Peter Krefting wrote:\n\n> Kyle J. McKay:\n> \n> >I think that a strict reading of RFC 2616 allows \"text/plain ;\n> >charset=utf-8\" as well as \"text/plain;charset=utf-8\" and \"text/plain;\n> >charset=utf-8\".\n> \n> It does indeed, and I have seen servers send both variants, so they do need\n> to be catered for.\n> \n> The number of servers that would actually send the charset attribute here\n> (for error messages) are probably not that many. It is probably a good idea\n> to make the default user-configurable (I know the specs state that anything\n> undeclared is iso-8859-1, but the real world doesn't agree to that).\n\nI was really hoping to avoid getting into all of the real-world\nmessiness that a real http client needs to deal with (as opposed to just\nfollowing the standards). This is only used for an optional relay of\nerror messages from the server. Most servers will send back text/html by\ndefault, and only those which specifically configure text/plain will\nhave their messages shown. IOW, I expect this to be configured\nspecifically for git messages, and the server admin can make sure they\nare following the standard and that it works with git.\n\n-Peff\n"},{"id":"242466","messageId":"20140522092824.GA14530@sigill.intra.peff.net","threadId":"36721","inReplyTo":"20140521102524.GA30301@sigill.intra.peff.net","subject":"[PATCH v2 0/9] handle alternate charsets for remote http errors","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-05-22T09:28:25Z","receivedAt":"2014-05-22T09:28:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 21, 2014 at 06:25:24AM -0400, Jeff King wrote:\n\n> As of commit 426e70d (remote-curl: show server content on http errors,\n> 2013-04-05), we relay any text/plain errors shown by the remote http\n> server to the user. However, we were lazy back then and left this TODO\n> in place:\n> \n> /*\n> * We only show text/plain parts, as other types are likely\n> * to be ugly to look at on the user's terminal.\n> *\n> * TODO should handle \"; charset=XXX\", and re-encode into\n> * logoutputencoding\n> */\n> \n> This series actually implements that, along with a few other cleanups.\n\nHere's a second version based on feedback from Kyle and Peter. Thanks\nboth for your comments.\n\nIt drops the \"tolower\" patches, which are not used anymore, and makes\nthe parsing of content-types and their parameters a bit more robust.\n\n [1/8]: test-lib: preserve GIT_CURL_VERBOSE from the environment\n [2/8]: t/lib-httpd: use write_script to copy CGI scripts\n [3/8]: t5550: test display of remote http error messages\n\n These three are the same as before.\n\n [4/8]: http: extract type/subtype portion of content-type\n\n Make our content-type matching more robust, both for the errors and\n for matching smart-http types.\n\n [5/8]: http: optionally extract charset parameter from content-type\n\n Feature work to support 7/8.\n\n [6/8]: strbuf: add strbuf_reencode helper\n\n Same as before (feature work to support 7/8).\n\n [7/8]: remote-curl: reencode http error messages\n\n The actual fix.\n\n [8/8]: http: default text charset to iso-8859-1\n\n This could be part of 5/8, but I floated it to the top of the heap\n to make it easier to discuss/adjust it.\n\n-Peff\n"},{"id":"242467","messageId":"20140522092849.GA15032@sigill.intra.peff.net","threadId":"36721","inReplyTo":"20140522092824.GA14530@sigill.intra.peff.net","subject":"[PATCH v2 1/8] test-lib: preserve GIT_CURL_VERBOSE from the environment","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-05-22T09:28:50Z","receivedAt":"2014-05-22T09:28:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Turning on this variable can be useful when debugging http\ntests. It does break a few tests in t5541, but it is not\na variable that the user is likely to have enabled\naccidentally.\n\nSigned-off-by: Jeff King \n---\n t/test-lib.sh | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex c081668..f7e55b1 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -91,6 +91,7 @@ unset VISUAL EMAIL LANGUAGE COLUMNS $(\"$PERL_PATH\" -e '\n \t\tVALGRIND\n \t\tUNZIP\n \t\tPERF_\n+\t\tCURL_VERBOSE\n \t));\n \tmy @vars = grep(/^GIT_/ && !/^GIT_($ok)/o, @env);\n \tprint join(\"\\n\", @vars);\n-- \n2.0.0.rc1.436.g03cb729\n"},{"id":"242468","messageId":"20140522092856.GB15032@sigill.intra.peff.net","threadId":"36721","inReplyTo":"20140522092824.GA14530@sigill.intra.peff.net","subject":"[PATCH v2 2/8] t/lib-httpd: use write_script to copy CGI scripts","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-05-22T09:28:56Z","receivedAt":"2014-05-22T09:28:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Using write_script will set our shebang line appropriately\nwith $SHELL_PATH. The script that is there now is quite\nsimple and likely to succeed even with a non-POSIX /bin/sh,\nbut it does not hurt to be defensive.\n\nSigned-off-by: Jeff King \n---\n t/lib-httpd.sh | 6 +++++-\n t/lib-httpd/broken-smart-http.sh | 1 -\n 2 files changed, 5 insertions(+), 2 deletions(-)\n mode change 100755 => 100644 t/lib-httpd/broken-smart-http.sh\n\ndiff --git a/t/lib-httpd.sh b/t/lib-httpd.sh\nindex 252cbf1..8e680ef 100644\n--- a/t/lib-httpd.sh\n+++ b/t/lib-httpd.sh\n@@ -105,10 +105,14 @@ else\n \t\t\"Could not identify web server at '$LIB_HTTPD_PATH'\"\n fi\n \n+install_script () {\n+\twrite_script \"$HTTPD_ROOT_PATH/$1\" <\"$TEST_PATH/$1\"\n+}\n+\n prepare_httpd() {\n \tmkdir -p \"$HTTPD_DOCUMENT_ROOT_PATH\"\n \tcp \"$TEST_PATH\"/passwd \"$HTTPD_ROOT_PATH\"\n-\tcp \"$TEST_PATH\"/broken-smart-http.sh \"$HTTPD_ROOT_PATH\"\n+\tinstall_script broken-smart-http.sh\n \n \tln -s \"$LIB_HTTPD_MODULE_PATH\" \"$HTTPD_ROOT_PATH/modules\"\n \ndiff --git a/t/lib-httpd/broken-smart-http.sh b/t/lib-httpd/broken-smart-http.sh\nold mode 100755\nnew mode 100644\nindex f7ebfff..82cc610\n--- a/t/lib-httpd/broken-smart-http.sh\n+++ b/t/lib-httpd/broken-smart-http.sh\n@@ -1,4 +1,3 @@\n-#!/bin/sh\n printf \"Content-Type: text/%s\\n\" \"html\"\n echo\n printf \"%s\\n\" \"001e# service=git-upload-pack\"\n-- \n2.0.0.rc1.436.g03cb729\n"},{"id":"242469","messageId":"20140522092902.GC15032@sigill.intra.peff.net","threadId":"36721","inReplyTo":"20140522092824.GA14530@sigill.intra.peff.net","subject":"[PATCH v2 3/8] t5550: test display of remote http error messages","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-05-22T09:29:03Z","receivedAt":"2014-05-22T09:29:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Since commit 426e70d (remote-curl: show server content on\nhttp errors, 2013-04-05), we relay any text/plain error\nmessages from the remote server to the user. However, we\nnever tested it.\n\nSigned-off-by: Jeff King \n---\n t/lib-httpd.sh | 1 +\n t/lib-httpd/apache.conf | 4 ++++\n t/lib-httpd/error.sh | 17 +++++++++++++++++\n t/t5550-http-fetch-dumb.sh | 10 ++++++++++\n 4 files changed, 32 insertions(+)\n create mode 100755 t/lib-httpd/error.sh\n\ndiff --git a/t/lib-httpd.sh b/t/lib-httpd.sh\nindex 8e680ef..f7640be 100644\n--- a/t/lib-httpd.sh\n+++ b/t/lib-httpd.sh\n@@ -113,6 +113,7 @@ prepare_httpd() {\n \tmkdir -p \"$HTTPD_DOCUMENT_ROOT_PATH\"\n \tcp \"$TEST_PATH\"/passwd \"$HTTPD_ROOT_PATH\"\n \tinstall_script broken-smart-http.sh\n+\tinstall_script error.sh\n \n \tln -s \"$LIB_HTTPD_MODULE_PATH\" \"$HTTPD_ROOT_PATH/modules\"\n \ndiff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf\nindex 3a03e82..b384d79 100644\n--- a/t/lib-httpd/apache.conf\n+++ b/t/lib-httpd/apache.conf\n@@ -97,12 +97,16 @@ Alias /auth/dumb/ www/auth/dumb/\n \n ScriptAliasMatch /smart_*[^/]*/(.*) ${GIT_EXEC_PATH}/git-http-backend/$1\n ScriptAlias /broken_smart/ broken-smart-http.sh/\n+ScriptAlias /error/ error.sh/\n \n \tOptions FollowSymlinks\n \n \n \tOptions ExecCGI\n \n+\n+ Options ExecCGI\n+\n \n \tOptions ExecCGI\n \ndiff --git a/t/lib-httpd/error.sh b/t/lib-httpd/error.sh\nnew file mode 100755\nindex 0000000..786f281\n--- /dev/null\n+++ b/t/lib-httpd/error.sh\n@@ -0,0 +1,17 @@\n+#!/bin/sh\n+\n+printf \"Status: 500 Intentional Breakage\\n\"\n+\n+printf \"Content-Type: \"\n+case \"$PATH_INFO\" in\n+*html*)\n+\tprintf \"text/html\"\n+\t;;\n+*text*)\n+\tprintf \"text/plain\"\n+\t;;\n+esac\n+printf \"\\n\"\n+\n+printf \"\\n\"\n+printf \"this is the error message\\n\"\ndiff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh\nindex 1a3a2b6..13defd3 100755\n--- a/t/t5550-http-fetch-dumb.sh\n+++ b/t/t5550-http-fetch-dumb.sh\n@@ -171,5 +171,15 @@ test_expect_success 'did not use upload-pack service' '\n \ttest_cmp exp act\n '\n \n+test_expect_success 'git client shows text/plain errors' '\n+\ttest_must_fail git clone \"$HTTPD_URL/error/text\" 2>stderr &&\n+\tgrep \"this is the error message\" stderr\n+'\n+\n+test_expect_success 'git client does not show html errors' '\n+\ttest_must_fail git clone \"$HTTPD_URL/error/html\" 2>stderr &&\n+\t! grep \"this is the error message\" stderr\n+'\n+\n stop_httpd\n test_done\n-- \n2.0.0.rc1.436.g03cb729\n"},{"id":"242470","messageId":"20140522092947.GD15032@sigill.intra.peff.net","threadId":"36721","inReplyTo":"20140522092824.GA14530@sigill.intra.peff.net","subject":"[PATCH v2 4/8] http: extract type/subtype portion of content-type","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-05-22T09:29:47Z","receivedAt":"2014-05-22T09:29:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we get a content-type from curl, we get the whole\nheader line, including any parameters, and without any\nnormalization (like downcasing or whitespace) applied.\nIf we later try to match it with strcmp() or even\nstrcasecmp(), we may get false negatives.\n\nThis could cause two visible behaviors:\n\n 1. We might fail to recognize a smart-http server by its\n content-type.\n\n 2. We might fail to relay text/plain error messages to\n users (especially if they contain a charset parameter).\n\nThis patch teaches the http code to extract and normalize\njust the type/subtype portion of the string. This is\ntechnically passing out less information to the callers, who\ncan no longer see the parameters. But none of the current\ncallers cares, and a future patch will add back an\neasier-to-use method for accessing those parameters.\n\nSigned-off-by: Jeff King \n---\n http.c | 32 +++++++++++++++++++++++++++++---\n remote-curl.c | 2 +-\n t/lib-httpd/error.sh | 8 +++++++-\n t/t5550-http-fetch-dumb.sh | 5 +++++\n 4 files changed, 42 insertions(+), 5 deletions(-)\n\ndiff --git a/http.c b/http.c\nindex 94e1afd..4edf5b9 100644\n--- a/http.c\n+++ b/http.c\n@@ -906,6 +906,29 @@ static CURLcode curlinfo_strbuf(CURL *curl, CURLINFO info, struct strbuf *buf)\n \treturn ret;\n }\n \n+/*\n+ * Extract a normalized version of the content type, with any\n+ * spaces suppressed, all letters lowercased, and no trailing \";\"\n+ * or parameters.\n+ *\n+ * Example:\n+ * \"TEXT/PLAIN; charset=utf-8\" -> \"text/plain\"\n+ */\n+static void extract_content_type(struct strbuf *raw, struct strbuf *type)\n+{\n+\tconst char *p;\n+\n+\tstrbuf_reset(type);\n+\tstrbuf_grow(type, raw->len);\n+\tfor (p = raw->buf; *p; p++) {\n+\t\tif (isspace(*p))\n+\t\t\tcontinue;\n+\t\tif (*p == ';')\n+\t\t\tbreak;\n+\t\tstrbuf_addch(type, tolower(*p));\n+\t}\n+}\n+\n /* http_request() targets */\n #define HTTP_REQUEST_STRBUF\t0\n #define HTTP_REQUEST_FILE\t1\n@@ -957,9 +980,12 @@ static int http_request(const char *url,\n \n \tret = run_one_slot(slot, &results);\n \n-\tif (options && options->content_type)\n-\t\tcurlinfo_strbuf(slot->curl, CURLINFO_CONTENT_TYPE,\n-\t\t\t\toptions->content_type);\n+\tif (options && options->content_type) {\n+\t\tstruct strbuf raw = STRBUF_INIT;\n+\t\tcurlinfo_strbuf(slot->curl, CURLINFO_CONTENT_TYPE, &raw);\n+\t\textract_content_type(&raw, options->content_type);\n+\t\tstrbuf_release(&raw);\n+\t}\n \n \tif (options && options->effective_url)\n \t\tcurlinfo_strbuf(slot->curl, CURLINFO_EFFECTIVE_URL,\ndiff --git a/remote-curl.c b/remote-curl.c\nindex 52c2d96..a5ab977 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -205,7 +205,7 @@ static int show_http_message(struct strbuf *type, struct strbuf *msg)\n \t * TODO should handle \"; charset=XXX\", and re-encode into\n \t * logoutputencoding\n \t */\n-\tif (strcasecmp(type->buf, \"text/plain\"))\n+\tif (strcmp(type->buf, \"text/plain\"))\n \t\treturn -1;\n \n \tstrbuf_trim(msg);\ndiff --git a/t/lib-httpd/error.sh b/t/lib-httpd/error.sh\nindex 786f281..23cec97 100755\n--- a/t/lib-httpd/error.sh\n+++ b/t/lib-httpd/error.sh\n@@ -3,6 +3,7 @@\n printf \"Status: 500 Intentional Breakage\\n\"\n \n printf \"Content-Type: \"\n+charset=iso-8859-1\n case \"$PATH_INFO\" in\n *html*)\n \tprintf \"text/html\"\n@@ -10,8 +11,13 @@ case \"$PATH_INFO\" in\n *text*)\n \tprintf \"text/plain\"\n \t;;\n+*charset*)\n+\tprintf \"text/plain; charset=utf-8\"\n+\tcharset=utf-8\n+\t;;\n esac\n printf \"\\n\"\n \n printf \"\\n\"\n-printf \"this is the error message\\n\"\n+printf \"this is the error message\\n\" |\n+iconv -f us-ascii -t $charset\ndiff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh\nindex 13defd3..b35b261 100755\n--- a/t/t5550-http-fetch-dumb.sh\n+++ b/t/t5550-http-fetch-dumb.sh\n@@ -181,5 +181,10 @@ test_expect_success 'git client does not show html errors' '\n \t! grep \"this is the error message\" stderr\n '\n \n+test_expect_success 'git client shows text/plain with a charset' '\n+\ttest_must_fail git clone \"$HTTPD_URL/error/charset\" 2>stderr &&\n+\tgrep \"this is the error message\" stderr\n+'\n+\n stop_httpd\n test_done\n-- \n2.0.0.rc1.436.g03cb729\n"},{"id":"242471","messageId":"20140522093005.GE15032@sigill.intra.peff.net","threadId":"36721","inReplyTo":"20140522092824.GA14530@sigill.intra.peff.net","subject":"[PATCH v2 5/8] http: optionally extract charset parameter from content-type","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-05-22T09:30:05Z","receivedAt":"2014-05-22T09:30:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Since the previous commit, we now give a sanitized,\nshortened version of the content-type header to any callers\nwho ask for it.\n\nThis patch adds back a way for them to cleanly access\nspecific parameters to the type. We could easily extract all\nparameters and make them available via a string_list, but:\n\n 1. That complicates the interface and memory management.\n\n 2. In practice, no planned callers care about anything\n except the charset.\n\nThis patch therefore goes with the simplest thing, and we\ncan expand or change the interface later if it becomes\nnecessary.\n\nSigned-off-by: Jeff King \n---\n http.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++----\n http.h | 7 +++++++\n 2 files changed, 57 insertions(+), 4 deletions(-)\n\ndiff --git a/http.c b/http.c\nindex 4edf5b9..e26ee8b 100644\n--- a/http.c\n+++ b/http.c\n@@ -907,14 +907,44 @@ static CURLcode curlinfo_strbuf(CURL *curl, CURLINFO info, struct strbuf *buf)\n }\n \n /*\n+ * Check for and extract a content-type parameter. \"raw\"\n+ * should be positioned at the start of the potential\n+ * parameter, with any whitespace already removed.\n+ *\n+ * \"name\" is the name of the parameter. The value is appended\n+ * to \"out\".\n+ */\n+static int extract_param(const char *raw, const char *name,\n+\t\t\t struct strbuf *out)\n+{\n+\tsize_t len = strlen(name);\n+\n+\tif (strncasecmp(raw, name, len))\n+\t\treturn -1;\n+\traw += len;\n+\n+\tif (*raw != '=')\n+\t\treturn -1;\n+\traw++;\n+\n+\twhile (*raw && !isspace(*raw))\n+\t\tstrbuf_addch(out, *raw++);\n+\treturn 0;\n+}\n+\n+/*\n * Extract a normalized version of the content type, with any\n * spaces suppressed, all letters lowercased, and no trailing \";\"\n * or parameters.\n *\n+ * If the \"charset\" argument is not NULL, store the value of any\n+ * charset parameter there.\n+ *\n * Example:\n- * \"TEXT/PLAIN; charset=utf-8\" -> \"text/plain\"\n+ * \"TEXT/PLAIN; charset=utf-8\" -> \"text/plain\", \"utf-8\"\n */\n-static void extract_content_type(struct strbuf *raw, struct strbuf *type)\n+static void extract_content_type(struct strbuf *raw, struct strbuf *type,\n+\t\t\t\t struct strbuf *charset)\n {\n \tconst char *p;\n \n@@ -923,10 +953,25 @@ static void extract_content_type(struct strbuf *raw, struct strbuf *type)\n \tfor (p = raw->buf; *p; p++) {\n \t\tif (isspace(*p))\n \t\t\tcontinue;\n-\t\tif (*p == ';')\n+\t\tif (*p == ';') {\n+\t\t\tp++;\n \t\t\tbreak;\n+\t\t}\n \t\tstrbuf_addch(type, tolower(*p));\n \t}\n+\n+\tif (!charset)\n+\t\treturn;\n+\n+\tstrbuf_reset(charset);\n+\twhile (*p) {\n+\t\twhile (isspace(*p))\n+\t\t\tp++;\n+\t\tif (!extract_param(p, \"charset\", charset))\n+\t\t\treturn;\n+\t\twhile (*p && !isspace(*p))\n+\t\t\tp++;\n+\t}\n }\n \n /* http_request() targets */\n@@ -983,7 +1028,8 @@ static int http_request(const char *url,\n \tif (options && options->content_type) {\n \t\tstruct strbuf raw = STRBUF_INIT;\n \t\tcurlinfo_strbuf(slot->curl, CURLINFO_CONTENT_TYPE, &raw);\n-\t\textract_content_type(&raw, options->content_type);\n+\t\textract_content_type(&raw, options->content_type,\n+\t\t\t\t options->charset);\n \t\tstrbuf_release(&raw);\n \t}\n \ndiff --git a/http.h b/http.h\nindex e64084f..473179b 100644\n--- a/http.h\n+++ b/http.h\n@@ -144,6 +144,13 @@ struct http_get_options {\n \tstruct strbuf *content_type;\n \n \t/*\n+\t * If non-NULL, and content_type above is non-NULL, returns\n+\t * the charset parameter from the content-type. If none is\n+\t * present, returns an empty string.\n+\t */\n+\tstruct strbuf *charset;\n+\n+\t/*\n \t * If non-NULL, returns the URL we ended up at, including any\n \t * redirects we followed.\n \t */\n-- \n2.0.0.rc1.436.g03cb729\n"},{"id":"242472","messageId":"20140522093014.GF15032@sigill.intra.peff.net","threadId":"36721","inReplyTo":"20140522092824.GA14530@sigill.intra.peff.net","subject":"[PATCH v2 6/8] strbuf: add strbuf_reencode helper","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-05-22T09:30:14Z","receivedAt":"2014-05-22T09:30:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This is a convenience wrapper around `reencode_string_len`\nand `strbuf_attach`.\n\nSigned-off-by: Jeff King \n---\n Documentation/technical/api-strbuf.txt | 5 +++++\n strbuf.c | 17 +++++++++++++++++\n strbuf.h | 1 +\n 3 files changed, 23 insertions(+)\n\ndiff --git a/Documentation/technical/api-strbuf.txt b/Documentation/technical/api-strbuf.txt\nindex 3350d97..9d28b03 100644\n--- a/Documentation/technical/api-strbuf.txt\n+++ b/Documentation/technical/api-strbuf.txt\n@@ -125,6 +125,11 @@ Functions\n \n \tStrip whitespace from the end of a string.\n \n+`strbuf_reencode`::\n+\n+\tReplace the contents of the strbuf with a reencoded form. Returns -1\n+\ton error, 0 on success.\n+\n `strbuf_cmp`::\n \n \tCompare two buffers. Returns an integer less than, equal to, or greater\ndiff --git a/strbuf.c b/strbuf.c\nindex ee96dcf..fc7290f 100644\n--- a/strbuf.c\n+++ b/strbuf.c\n@@ -1,5 +1,6 @@\n #include \"cache.h\"\n #include \"refs.h\"\n+#include \"utf8.h\"\n \n int starts_with(const char *str, const char *prefix)\n {\n@@ -106,6 +107,22 @@ void strbuf_ltrim(struct strbuf *sb)\n \tsb->buf[sb->len] = '\\0';\n }\n \n+int strbuf_reencode(struct strbuf *sb, const char *from, const char *to)\n+{\n+\tchar *out;\n+\tint len;\n+\n+\tif (same_encoding(from, to))\n+\t\treturn 0;\n+\n+\tout = reencode_string_len(sb->buf, sb->len, to, from, &len);\n+\tif (!out)\n+\t\treturn -1;\n+\n+\tstrbuf_attach(sb, out, len, len);\n+\treturn 0;\n+}\n+\n struct strbuf **strbuf_split_buf(const char *str, size_t slen,\n \t\t\t\t int terminator, int max)\n {\ndiff --git a/strbuf.h b/strbuf.h\nindex 39c14cf..4e9a2f8 100644\n--- a/strbuf.h\n+++ b/strbuf.h\n@@ -45,6 +45,7 @@ static inline void strbuf_setlen(struct strbuf *sb, size_t len)\n extern void strbuf_trim(struct strbuf *);\n extern void strbuf_rtrim(struct strbuf *);\n extern void strbuf_ltrim(struct strbuf *);\n+extern int strbuf_reencode(struct strbuf *sb, const char *from, const char *to);\n extern int strbuf_cmp(const struct strbuf *, const struct strbuf *);\n \n /*\n-- \n2.0.0.rc1.436.g03cb729\n"},{"id":"242473","messageId":"20140522093029.GG15032@sigill.intra.peff.net","threadId":"36721","inReplyTo":"20140522092824.GA14530@sigill.intra.peff.net","subject":"[PATCH v2 7/8] remote-curl: reencode http error messages","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-05-22T09:30:29Z","receivedAt":"2014-05-22T09:30:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We currently recognize an error message with a content-type\n\"text/plain; charset=utf-16\" as text, but we ignore the\ncharset parameter entirely. Let's encode it to\nlog_output_encoding, which is presumably something the\nuser's terminal can handle.\n\nSigned-off-by: Jeff King \n---\n remote-curl.c | 17 ++++++++++-------\n t/lib-httpd/error.sh | 4 ++++\n t/t5550-http-fetch-dumb.sh | 5 +++++\n 3 files changed, 19 insertions(+), 7 deletions(-)\n\ndiff --git a/remote-curl.c b/remote-curl.c\nindex a5ab977..4493b38 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -194,19 +194,19 @@ static void free_discovery(struct discovery *d)\n \t}\n }\n \n-static int show_http_message(struct strbuf *type, struct strbuf *msg)\n+static int show_http_message(struct strbuf *type, struct strbuf *charset,\n+\t\t\t struct strbuf *msg)\n {\n \tconst char *p, *eol;\n \n \t/*\n \t * We only show text/plain parts, as other types are likely\n \t * to be ugly to look at on the user's terminal.\n-\t *\n-\t * TODO should handle \"; charset=XXX\", and re-encode into\n-\t * logoutputencoding\n \t */\n \tif (strcmp(type->buf, \"text/plain\"))\n \t\treturn -1;\n+\tif (charset->len)\n+\t\tstrbuf_reencode(msg, charset->buf, get_log_output_encoding());\n \n \tstrbuf_trim(msg);\n \tif (!msg->len)\n@@ -225,6 +225,7 @@ static struct discovery* discover_refs(const char *service, int for_push)\n {\n \tstruct strbuf exp = STRBUF_INIT;\n \tstruct strbuf type = STRBUF_INIT;\n+\tstruct strbuf charset = STRBUF_INIT;\n \tstruct strbuf buffer = STRBUF_INIT;\n \tstruct strbuf refs_url = STRBUF_INIT;\n \tstruct strbuf effective_url = STRBUF_INIT;\n@@ -249,6 +250,7 @@ static struct discovery* discover_refs(const char *service, int for_push)\n \n \tmemset(&options, 0, sizeof(options));\n \toptions.content_type = &type;\n+\toptions.charset = &charset;\n \toptions.effective_url = &effective_url;\n \toptions.base_url = &url;\n \toptions.no_cache = 1;\n@@ -259,13 +261,13 @@ static struct discovery* discover_refs(const char *service, int for_push)\n \tcase HTTP_OK:\n \t\tbreak;\n \tcase HTTP_MISSING_TARGET:\n-\t\tshow_http_message(&type, &buffer);\n+\t\tshow_http_message(&type, &charset, &buffer);\n \t\tdie(\"repository '%s' not found\", url.buf);\n \tcase HTTP_NOAUTH:\n-\t\tshow_http_message(&type, &buffer);\n+\t\tshow_http_message(&type, &charset, &buffer);\n \t\tdie(\"Authentication failed for '%s'\", url.buf);\n \tdefault:\n-\t\tshow_http_message(&type, &buffer);\n+\t\tshow_http_message(&type, &charset, &buffer);\n \t\tdie(\"unable to access '%s': %s\", url.buf, curl_errorstr);\n \t}\n \n@@ -310,6 +312,7 @@ static struct discovery* discover_refs(const char *service, int for_push)\n \tstrbuf_release(&refs_url);\n \tstrbuf_release(&exp);\n \tstrbuf_release(&type);\n+\tstrbuf_release(&charset);\n \tstrbuf_release(&effective_url);\n \tstrbuf_release(&buffer);\n \tlast_discovery = last;\ndiff --git a/t/lib-httpd/error.sh b/t/lib-httpd/error.sh\nindex 23cec97..eafc9d2 100755\n--- a/t/lib-httpd/error.sh\n+++ b/t/lib-httpd/error.sh\n@@ -15,6 +15,10 @@ case \"$PATH_INFO\" in\n \tprintf \"text/plain; charset=utf-8\"\n \tcharset=utf-8\n \t;;\n+*utf16*)\n+\tprintf \"text/plain; charset=utf-16\"\n+\tcharset=utf-16\n+\t;;\n esac\n printf \"\\n\"\n \ndiff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh\nindex b35b261..01b8aae 100755\n--- a/t/t5550-http-fetch-dumb.sh\n+++ b/t/t5550-http-fetch-dumb.sh\n@@ -186,5 +186,10 @@ test_expect_success 'git client shows text/plain with a charset' '\n \tgrep \"this is the error message\" stderr\n '\n \n+test_expect_success 'http error messages are reencoded' '\n+\ttest_must_fail git clone \"$HTTPD_URL/error/utf16\" 2>stderr &&\n+\tgrep \"this is the error message\" stderr\n+'\n+\n stop_httpd\n test_done\n-- \n2.0.0.rc1.436.g03cb729\n"},{"id":"242474","messageId":"20140522093612.GH15032@sigill.intra.peff.net","threadId":"36721","inReplyTo":"20140522092824.GA14530@sigill.intra.peff.net","subject":"[PATCH v2 8/8] http: default text charset to iso-8859-1","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-05-22T09:36:12Z","receivedAt":"2014-05-22T09:36:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This is specified by RFC 2616 as the default if no \"charset\"\nparameter is given.\n\nSigned-off-by: Jeff King \n---\nI'd prefer to do this simple, standard thing, and see how it works in\nthe real world. We'll hand whatever we get off to iconv, and if it\nchokes, we'll pass through the data as-is. That should be enough for\nmost ascii messages to make it through readable, even if we get the\nencoding wrong.\n\nIf we do want to do magic like \"latin1 is really iso-8859-1\", that seems\nlike the domain of iconv to me. If iconv doesn't handle it itself, I'd\nrather have a wrapper there. Putting it at that layer keeps the code\ncleaner, and it means the wrapper would benefit the regular commit-log\nreencoding code.\n\nIf anybody wants to go further in that direction, be my guest, but please\nmake your suggestions in the form of patches which apply on top. :)\n\n http.c | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/http.c b/http.c\nindex e26ee8b..a37e84e 100644\n--- a/http.c\n+++ b/http.c\n@@ -972,6 +972,9 @@ static void extract_content_type(struct strbuf *raw, struct strbuf *type,\n \t\twhile (*p && !isspace(*p))\n \t\t\tp++;\n \t}\n+\n+\tif (!charset->len && starts_with(type->buf, \"text/\"))\n+\t\tstrbuf_addstr(charset, \"ISO-8859-1\");\n }\n \n /* http_request() targets */\n-- \n2.0.0.rc1.436.g03cb729\n"},{"id":"242479","messageId":"alpine.DEB.2.00.1405221118150.10958@ds9.cixit.se","threadId":"36721","inReplyTo":"20140522090557.GB29669@sigill.intra.peff.net","subject":"Re: [PATCH 7/9] remote-curl: recognize text/plain with a charset parameter","fromName":"Peter Krefting","fromEmail":"peter@softwolves.pp.se","sentAt":"2014-05-22T10:19:57Z","receivedAt":"2014-05-22T10:19:57Z","isPatch":true,"sender":{"key":"peter@softwolves.pp.se","avatar":"https://avatars.githubusercontent.com/u/990764?v=4"},"body":"Jeff King:\n\n> I was really hoping to avoid getting into all of the real-world\n> messiness that a real http client needs to deal with (as opposed to just\n> following the standards).\n\nYeah, I agree, you're probably fine without all this detail in over \n99% of the cases where this code would ever be exposed. I'm a bit \ndamaged after 10+ years as a web browser developer, responsible for \nexactly this kind of stuff... :-)\n\n-- \n\\\\// Peter - http://www.softwolves.pp.se/\n"},{"id":"242511","messageId":"xmqq38g1oe2y.fsf@gitster.dls.corp.google.com","threadId":"36721","inReplyTo":"20140522055852.GA16587@sigill.intra.peff.net","subject":"Re: [PATCH 2/9] strbuf: add strbuf_tolower function","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-05-22T18:36:37Z","receivedAt":"2014-05-22T18:36:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Wed, May 21, 2014 at 05:07:36PM -0700, Kyle J. McKay wrote:\n>\n>> >+void strbuf_tolower(struct strbuf *sb)\n>> >+{\n>> >+\tsize_t i;\n>> >+\tfor (i = 0; i < sb->len; i++)\n>> >+\t\tsb->buf[i] = tolower(sb->buf[i]);\n>> >+}\n>> >+\n>> \n>> Wouldn't a direct transfer of the lowercase function be something more like:\n>> \n>> \n>> void strbuf_tolower(struct strbuf *sb)\n>> {\n>> \tchar *p = sb->buf;\n>> \tfor (; *p; p++)\n>> \t\t*p = tolower(*p);\n>> }\n>> \n>> That seems to me to be a bit more efficient. According to the comments in\n>> strbuf.c, \"people can always assume buf is non NULL and ->buf is NUL\n>> terminated even for a freshly initialized strbuf.\"\n>\n> Yes, and that would be fine with me (I actually wrote strbuf_tolower for\n> my own use, and _then_ realized that we already had such a thing that\n> could be replaced).\n\nDo we forbid that sb->buf[x] for some x < sb->len to be NUL, and if\nthere is such a byte we stop running tolower() on the remainder?\n"},{"id":"242513","messageId":"20140522184113.GC1167@sigill.intra.peff.net","threadId":"36721","inReplyTo":"xmqq38g1oe2y.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 2/9] strbuf: add strbuf_tolower function","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-05-22T18:41:14Z","receivedAt":"2014-05-22T18:41:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 22, 2014 at 11:36:37AM -0700, Junio C Hamano wrote:\n\n> > Yes, and that would be fine with me (I actually wrote strbuf_tolower for\n> > my own use, and _then_ realized that we already had such a thing that\n> > could be replaced).\n> \n> Do we forbid that sb->buf[x] for some x < sb->len to be NUL, and if\n> there is such a byte we stop running tolower() on the remainder?\n\nChristian brought this up elsewhere, and I agree it's probably better to\nwork over the whole buffer, NULs included. I'm happy to re-roll (or you\ncan just pick up the version of the patch in this thread), but I think\nthe bigger question is: is this refactor worth doing, since there is\nonly one caller?\n\n-Peff\n"},{"id":"242526","messageId":"xmqqppj5msob.fsf@gitster.dls.corp.google.com","threadId":"36721","inReplyTo":"20140522184113.GC1167@sigill.intra.peff.net","subject":"Re: [PATCH 2/9] strbuf: add strbuf_tolower function","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-05-22T21:04:20Z","receivedAt":"2014-05-22T21:04:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> > Yes, and that would be fine with me (I actually wrote strbuf_tolower for\n>> > my own use, and _then_ realized that we already had such a thing that\n>> > could be replaced).\n>> ...\n> ... I think\n> the bigger question is: is this refactor worth doing, since there is\n> only one caller?\n\nIf you wrote it for your own use and then realized that it is\napplicable to this codepath, wouldn't that say that there are\nmultiple potential callers that would benefit from having it?\n"},{"id":"242535","messageId":"0E298290-243D-46E5-A177-C6D3553D7153@gmail.com","threadId":"36721","inReplyTo":"20140522184113.GC1167@sigill.intra.peff.net","subject":"Re: [PATCH 2/9] strbuf: add strbuf_tolower function","fromName":"Kyle J. McKay","fromEmail":"mackyle@gmail.com","sentAt":"2014-05-22T22:52:08Z","receivedAt":"2014-05-22T22:52:08Z","isPatch":true,"sender":{"key":"mackyle@gmail.com","avatar":"https://avatars.githubusercontent.com/u/813346?v=4"},"body":"On May 22, 2014, at 11:41, Jeff King wrote:\n\n> On Thu, May 22, 2014 at 11:36:37AM -0700, Junio C Hamano wrote:\n>\n>>> Yes, and that would be fine with me (I actually wrote \n>>> strbuf_tolower for\n>>> my own use, and _then_ realized that we already had such a thing \n>>> that\n>>> could be replaced).\n>>\n>> Do we forbid that sb->buf[x] for some x < sb->len to be NUL, and if\n>> there is such a byte we stop running tolower() on the remainder?\n>\n> Christian brought this up elsewhere, and I agree it's probably \n> better to\n> work over the whole buffer, NULs included. I'm happy to re-roll (or \n> you\n> can just pick up the version of the patch in this thread),\n\nThe only reason I brought up the code difference in the first place \nwas that the comment was \"This makes config's lowercase() function \npublic\" which made me expect to see basically the equivalent of \nreplacing a \"static\" with an \"extern\", but actually the function being \nmade public was a different implementation than config's lowercase() \nfunction. So it looks like the original PATCH 2/9 version of the code \nis best, but perhaps the comment can be tweaked a bit to not convey \nthe same impression I got. Maybe something like \"Replace config's \nlowercase() function with a public one\".\n\n--Kyle\n"},{"id":"242536","messageId":"567F86A0-98E4-49AF-81C8-8D9E1AEC3C5E@gmail.com","threadId":"36721","inReplyTo":"20140522092947.GD15032@sigill.intra.peff.net","subject":"Re: [PATCH v2 4/8] http: extract type/subtype portion of content-type","fromName":"Kyle J. McKay","fromEmail":"mackyle@gmail.com","sentAt":"2014-05-22T22:52:21Z","receivedAt":"2014-05-22T22:52:21Z","isPatch":true,"sender":{"key":"mackyle@gmail.com","avatar":"https://avatars.githubusercontent.com/u/813346?v=4"},"body":"\nOn May 22, 2014, at 02:29, Jeff King wrote:\n\n> When we get a content-type from curl, we get the whole\n> header line, including any parameters, and without any\n> normalization (like downcasing or whitespace) applied.\n> If we later try to match it with strcmp() or even\n> strcasecmp(), we may get false negatives.\n>\n> This could cause two visible behaviors:\n>\n> 1. We might fail to recognize a smart-http server by its\n> content-type.\n>\n> 2. We might fail to relay text/plain error messages to\n> users (especially if they contain a charset parameter).\n>\n> This patch teaches the http code to extract and normalize\n> just the type/subtype portion of the string. This is\n> technically passing out less information to the callers, who\n> can no longer see the parameters. But none of the current\n> callers cares, and a future patch will add back an\n> easier-to-use method for accessing those parameters.\n>\n> Signed-off-by: Jeff King \n> ---\n> http.c | 32 +++++++++++++++++++++++++++++---\n> remote-curl.c | 2 +-\n> t/lib-httpd/error.sh | 8 +++++++-\n> t/t5550-http-fetch-dumb.sh | 5 +++++\n> 4 files changed, 42 insertions(+), 5 deletions(-)\n>\n> diff --git a/http.c b/http.c\n> index 94e1afd..4edf5b9 100644\n> --- a/http.c\n> +++ b/http.c\n> @@ -906,6 +906,29 @@ static CURLcode curlinfo_strbuf(CURL *curl, \n> CURLINFO info, struct strbuf *buf)\n> \treturn ret;\n> }\n>\n> +/*\n> + * Extract a normalized version of the content type, with any\n> + * spaces suppressed, all letters lowercased, and no trailing \";\"\n> + * or parameters.\n> + *\n> + * Example:\n> + * \"TEXT/PLAIN; charset=utf-8\" -> \"text/plain\"\n> + */\n> +static void extract_content_type(struct strbuf *raw, struct strbuf \n> *type)\n> +{\n> +\tconst char *p;\n> +\n> +\tstrbuf_reset(type);\n> +\tstrbuf_grow(type, raw->len);\n> +\tfor (p = raw->buf; *p; p++) {\n> +\t\tif (isspace(*p))\n> +\t\t\tcontinue;\n> +\t\tif (*p == ';')\n> +\t\t\tbreak;\n> +\t\tstrbuf_addch(type, tolower(*p));\n> +\t}\n> +}\n> +\n\nThis will parse invalid content types as valid. Probably not \nimportant since the producer of an invalid content type shouldn't be \ndepending on any particular behavior by the consumer of such a type, \nbut I think it warrants a note in the comment block, perhaps something \nlike:\n\n * Note that an invalid content-type may be converted to a valid one\n\nor some such.\n\n--Kyle\n"},{"id":"242550","messageId":"20140523020224.GB284865@vauxhall.crustytoothpaste.net","threadId":"36721","inReplyTo":"20140522093612.GH15032@sigill.intra.peff.net","subject":"Re: [PATCH v2 8/8] http: default text charset to iso-8859-1","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2014-05-23T02:02:24Z","receivedAt":"2014-05-23T02:02:24Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Thu, May 22, 2014 at 05:36:12AM -0400, Jeff King wrote:\n> If we do want to do magic like \"latin1 is really iso-8859-1\", that seems\n> like the domain of iconv to me. If iconv doesn't handle it itself, I'd\n> rather have a wrapper there. Putting it at that layer keeps the code\n> cleaner, and it means the wrapper would benefit the regular commit-log\n> reencoding code.\n\nI think being a little stricter in our character encoding actually\nbenefits users. If someone claims that all their commit messages are in\nUS-ASCII or ISO-8859-1, and then stuffs Windows-1252 in there, that's\ngoing to break a lot of stuff, especially if someone assumes US-ASCII\nmeans it's okay to use it where UTF-8 is required.\n\nIt's much better to let people not insert broken stuff in the first\nplace rather than deal with it afterwards.\n\n-- \nbrian m. carlson / brian with sandals: Houston, Texas, US\n+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only\nOpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187\n"},{"id":"242601","messageId":"20140523200347.GC19088@sigill.intra.peff.net","threadId":"36721","inReplyTo":"xmqqppj5msob.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 2/9] strbuf: add strbuf_tolower function","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-05-23T20:03:47Z","receivedAt":"2014-05-23T20:03:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 22, 2014 at 02:04:20PM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> >> > Yes, and that would be fine with me (I actually wrote strbuf_tolower for\n> >> > my own use, and _then_ realized that we already had such a thing that\n> >> > could be replaced).\n> >> ...\n> > ... I think\n> > the bigger question is: is this refactor worth doing, since there is\n> > only one caller?\n> \n> If you wrote it for your own use and then realized that it is\n> applicable to this codepath, wouldn't that say that there are\n> multiple potential callers that would benefit from having it?\n\nSure, and that's why I posted it. But I know our standard for adding\ncode is often a bit higher. We don't generally add code that has no\ncallers. In this case, there is one caller, but it is still only\ntheoretical that there will be another. I.e., for the same reason that I\ndid not end up using it in my patch (namely, that we often want to\ndowncase _and_ do other things while we traverse the string), other\nplaces may not.\n\nThat's why I asked: it is a judgement call on \"does this seem like it is\nlikely to be a useful thing in the future?\". Probably, but I wouldn't be\nsad if people disagreed and we dropped it.\n\nAnyway, here is an updated patch. It uses pointer arithmetic (though I\nam curious whether hand-optimizing \"*p\" is actually any faster than\n\"sb->buf[i]\" with modern compilers), but traverses past NULs. I also\nupdated the commit message to reflect the discussion.\n\n-- >8 --\nSubject: strbuf: add strbuf_tolower function\n\nThis is a convenience wrapper to call tolower on each\ncharacter of the string.\n\nThis makes config's lowercase() function obsolete, though\nnote that because we have a strbuf, we are careful to\noperate over the whole strbuf, rather than assuming that a\nNUL is the end-of-string.\n\nWe could continue to offer a pure-string lowercase, but\nthere would be no callers (in most pure-string cases, we\nactually duplicate and lowercase the duplicate, for which we\nhave the xstrdup_tolower wrapper).\n\nSigned-off-by: Jeff King \n---\n Documentation/technical/api-strbuf.txt | 4 ++++\n config.c | 8 +-------\n strbuf.c | 7 +++++++\n strbuf.h | 1 +\n 4 files changed, 13 insertions(+), 7 deletions(-)\n\ndiff --git a/Documentation/technical/api-strbuf.txt b/Documentation/technical/api-strbuf.txt\nindex 3350d97..8480f89 100644\n--- a/Documentation/technical/api-strbuf.txt\n+++ b/Documentation/technical/api-strbuf.txt\n@@ -125,6 +125,10 @@ Functions\n \n \tStrip whitespace from the end of a string.\n \n+`strbuf_tolower`::\n+\n+\tLowercase each character in the buffer using `tolower`.\n+\n `strbuf_cmp`::\n \n \tCompare two buffers. Returns an integer less than, equal to, or greater\ndiff --git a/config.c b/config.c\nindex a30cb5c..03ce5c6 100644\n--- a/config.c\n+++ b/config.c\n@@ -147,12 +147,6 @@ int git_config_include(const char *var, const char *value, void *data)\n \treturn ret;\n }\n \n-static void lowercase(char *p)\n-{\n-\tfor (; *p; p++)\n-\t\t*p = tolower(*p);\n-}\n-\n void git_config_push_parameter(const char *text)\n {\n \tstruct strbuf env = STRBUF_INIT;\n@@ -180,7 +174,7 @@ int git_config_parse_parameter(const char *text,\n \t\tstrbuf_list_free(pair);\n \t\treturn error(\"bogus config parameter: %s\", text);\n \t}\n-\tlowercase(pair[0]->buf);\n+\tstrbuf_tolower(pair[0]);\n \tif (fn(pair[0]->buf, pair[1] ? pair[1]->buf : NULL, data) < 0) {\n \t\tstrbuf_list_free(pair);\n \t\treturn -1;\ndiff --git a/strbuf.c b/strbuf.c\nindex 854c725..2d059b9 100644\n--- a/strbuf.c\n+++ b/strbuf.c\n@@ -106,6 +106,13 @@ void strbuf_ltrim(struct strbuf *sb)\n \tsb->buf[sb->len] = '\\0';\n }\n \n+void strbuf_tolower(struct strbuf *sb)\n+{\n+\tchar *p = sb->buf, *end = sb->buf + sb->len;\n+\tfor (; p < end; p++)\n+\t\t*p = tolower(*p);\n+}\n+\n struct strbuf **strbuf_split_buf(const char *str, size_t slen,\n \t\t\t\t int terminator, int max)\n {\ndiff --git a/strbuf.h b/strbuf.h\nindex 4de7531..25328b9 100644\n--- a/strbuf.h\n+++ b/strbuf.h\n@@ -45,6 +45,7 @@ static inline void strbuf_setlen(struct strbuf *sb, size_t len)\n extern void strbuf_trim(struct strbuf *);\n extern void strbuf_rtrim(struct strbuf *);\n extern void strbuf_ltrim(struct strbuf *);\n+extern void strbuf_tolower(struct strbuf *sb);\n extern int strbuf_cmp(const struct strbuf *, const struct strbuf *);\n \n /*\n-- \n2.0.0.rc1.436.g03cb729\n"},{"id":"242602","messageId":"20140523200534.GD19088@sigill.intra.peff.net","threadId":"36721","inReplyTo":"0E298290-243D-46E5-A177-C6D3553D7153@gmail.com","subject":"Re: [PATCH 2/9] strbuf: add strbuf_tolower function","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-05-23T20:05:34Z","receivedAt":"2014-05-23T20:05:34Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 22, 2014 at 03:52:08PM -0700, Kyle J. McKay wrote:\n\n> >Christian brought this up elsewhere, and I agree it's probably better to\n> >work over the whole buffer, NULs included. I'm happy to re-roll (or you\n> >can just pick up the version of the patch in this thread),\n> \n> The only reason I brought up the code difference in the first place was that\n> the comment was \"This makes config's lowercase() function public\" which made\n> me expect to see basically the equivalent of replacing a \"static\" with an\n> \"extern\", but actually the function being made public was a different\n> implementation than config's lowercase() function. So it looks like the\n> original PATCH 2/9 version of the code is best, but perhaps the comment can\n> be tweaked a bit to not convey the same impression I got. Maybe something\n> like \"Replace config's lowercase() function with a public one\".\n\nYeah, sorry if it sounded like I was complaining about your review\nelsewhere. I mostly found it amusing that I got two opposite-direction\nreviews.\n\nI agree that clarifying the situation in the commit message is best, and\nI've done that in the version I just posted.\n\n-Peff\n"},{"id":"242604","messageId":"20140523201245.GE19088@sigill.intra.peff.net","threadId":"36721","inReplyTo":"567F86A0-98E4-49AF-81C8-8D9E1AEC3C5E@gmail.com","subject":"Re: [PATCH v2 4/8] http: extract type/subtype portion of content-type","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-05-23T20:12:45Z","receivedAt":"2014-05-23T20:12:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 22, 2014 at 03:52:21PM -0700, Kyle J. McKay wrote:\n\n> >+static void extract_content_type(struct strbuf *raw, struct strbuf *type)\n> >+{\n> >+\tconst char *p;\n> >+\n> >+\tstrbuf_reset(type);\n> >+\tstrbuf_grow(type, raw->len);\n> >+\tfor (p = raw->buf; *p; p++) {\n> >+\t\tif (isspace(*p))\n> >+\t\t\tcontinue;\n> >+\t\tif (*p == ';')\n> >+\t\t\tbreak;\n> >+\t\tstrbuf_addch(type, tolower(*p));\n> >+\t}\n> >+}\n> >+\n> \n> This will parse invalid content types as valid. Probably not important\n> since the producer of an invalid content type shouldn't be depending on any\n> particular behavior by the consumer of such a type, but I think it warrants\n> a note in the comment block, perhaps something like:\n> \n> * Note that an invalid content-type may be converted to a valid one\n> \n> or some such.\n\nYeah, that is intentional based on our earlier discussion (this function\nstarted as \"normalize_content_type\" :) ). I think it's not a big deal,\nbut agree it's worth a comment. Like:\n\ndiff --git a/http.c b/http.c\nindex 4edf5b9..6bfd093 100644\n--- a/http.c\n+++ b/http.c\n@@ -911,8 +911,14 @@ static CURLcode curlinfo_strbuf(CURL *curl, CURLINFO info, struct strbuf *buf)\n * spaces suppressed, all letters lowercased, and no trailing \";\"\n * or parameters.\n *\n+ * Note that we will silently remove even invalid whitespace. For\n+ * example, \"text / plain\" is specifically forbidden by RFC 2616,\n+ * but \"text/plain\" is the only reasonable output, and this keeps\n+ * our code simple.\n+ *\n * Example:\n * \"TEXT/PLAIN; charset=utf-8\" -> \"text/plain\"\n+ * \"text / plain\" -> \"text/plain\"\n */\n static void extract_content_type(struct strbuf *raw, struct strbuf *type)\n {\n\n-Peff\n"},{"id":"242619","messageId":"D9EE35E5-A76C-4C3C-9A50-42822165964F@gmail.com","threadId":"36721","inReplyTo":"20140523201245.GE19088@sigill.intra.peff.net","subject":"Re: [PATCH v2 4/8] http: extract type/subtype portion of content-type","fromName":"Kyle J. McKay","fromEmail":"mackyle@gmail.com","sentAt":"2014-05-23T22:00:43Z","receivedAt":"2014-05-23T22:00:43Z","isPatch":true,"sender":{"key":"mackyle@gmail.com","avatar":"https://avatars.githubusercontent.com/u/813346?v=4"},"body":"On May 23, 2014, at 13:12, Jeff King wrote:\n> On Thu, May 22, 2014 at 03:52:21PM -0700, Kyle J. McKay wrote:\n>\n>>> +static void extract_content_type(struct strbuf *raw, struct \n>>> strbuf *type)\n>>> +{\n>>> +\tconst char *p;\n>>> +\n>>> +\tstrbuf_reset(type);\n>>> +\tstrbuf_grow(type, raw->len);\n>>> +\tfor (p = raw->buf; *p; p++) {\n>>> +\t\tif (isspace(*p))\n>>> +\t\t\tcontinue;\n>>> +\t\tif (*p == ';')\n>>> +\t\t\tbreak;\n>>> +\t\tstrbuf_addch(type, tolower(*p));\n>>> +\t}\n>>> +}\n>>> +\n>>\n>> This will parse invalid content types as valid. Probably not \n>> important\n>> since the producer of an invalid content type shouldn't be \n>> depending on any\n>> particular behavior by the consumer of such a type, but I think it \n>> warrants\n>> a note in the comment block, perhaps something like:\n>>\n>> * Note that an invalid content-type may be converted to a valid one\n>>\n>> or some such.\n>\n> Yeah, that is intentional based on our earlier discussion (this \n> function\n> started as \"normalize_content_type\" :) ). I think it's not a big deal,\n> but agree it's worth a comment. Like:\n>\n> diff --git a/http.c b/http.c\n> index 4edf5b9..6bfd093 100644\n> --- a/http.c\n> +++ b/http.c\n> @@ -911,8 +911,14 @@ static CURLcode curlinfo_strbuf(CURL *curl, \n> CURLINFO info, struct strbuf *buf)\n> * spaces suppressed, all letters lowercased, and no trailing \";\"\n> * or parameters.\n> *\n> + * Note that we will silently remove even invalid whitespace. For\n> + * example, \"text / plain\" is specifically forbidden by RFC 2616,\n> + * but \"text/plain\" is the only reasonable output, and this keeps\n> + * our code simple.\n\nVery nice. :)\n\n> + *\n> * Example:\n> * \"TEXT/PLAIN; charset=utf-8\" -> \"text/plain\"\n> + * \"text / plain\" -> \"text/plain\"\n> */\n> static void extract_content_type(struct strbuf *raw, struct strbuf \n> *type)\n> {\n>\n> -Peff\n"},{"id":"242621","messageId":"6057662E-B230-40FB-AB98-3E94BA70C7C1@gmail.com","threadId":"36721","inReplyTo":"20140523200534.GD19088@sigill.intra.peff.net","subject":"Re: [PATCH 2/9] strbuf: add strbuf_tolower function","fromName":"Kyle J. McKay","fromEmail":"mackyle@gmail.com","sentAt":"2014-05-23T22:34:01Z","receivedAt":"2014-05-23T22:34:01Z","isPatch":true,"sender":{"key":"mackyle@gmail.com","avatar":"https://avatars.githubusercontent.com/u/813346?v=4"},"body":"On May 23, 2014, at 13:05, Jeff King wrote:\n> On Thu, May 22, 2014 at 03:52:08PM -0700, Kyle J. McKay wrote:\n>\n>>> Christian brought this up elsewhere, and I agree it's probably \n>>> better to\n>>> work over the whole buffer, NULs included. I'm happy to re-roll \n>>> (or you\n>>> can just pick up the version of the patch in this thread),\n>>\n>> The only reason I brought up the code difference in the first place \n>> was that\n>> the comment was \"This makes config's lowercase() function public\" \n>> which made\n>> me expect to see basically the equivalent of replacing a \"static\" \n>> with an\n>> \"extern\", but actually the function being made public was a different\n>> implementation than config's lowercase() function.\n>\n> Yeah, sorry if it sounded like I was complaining about your review\n> elsewhere. I mostly found it amusing that I got two opposite-direction\n> reviews.\n\nI didn't seem like complaining to me. I also was amused. :)\n\n> I agree that clarifying the situation in the commit message is best, \n> and\n> I've done that in the version I just posted.\n\nIt looks great. And I suspect you're correct that a modern compiler \nwould optimize the index-based version to be as efficient as the \npointer arithmetic version.\n\n--Kyle\n"}]}