{"thread":{"id":"37174","subject":"[PATCH v4 0/1] http: Add Accept-Language header if possible","startedAt":"2014-07-19T17:58:49Z","lastAt":"2015-03-06T19:01:32Z","messageCount":46,"participants":["Yi EungJun","Junio C Hamano","Yi, EungJun","Eric Sunshine","Michael Blume","Torsten Bögershausen","Jeff King","Stefan Beller","Ævar Arnfjörð Bjarmason"],"isPatch":true,"patchVersion":4,"patchTotal":1},"messages":[{"id":"246390","messageId":"1405792730-13539-1-git-send-email-eungjun.yi@navercorp.com","threadId":"37174","inReplyTo":null,"subject":"[PATCH v4 0/1] http: Add Accept-Language header if possible","fromName":"Yi EungJun","fromEmail":"semtlenori@gmail.com","sentAt":"2014-07-19T17:58:49Z","receivedAt":"2014-07-19T17:58:49Z","isPatch":true,"sender":{"key":"semtlenori@gmail.com","avatar":"https://gravatar.com/avatar/8363435d2badb3450df0dd7c4ec2113f6e1d62c44dd3cf7dfe83c5a389b8a9bf?d=mp&s=160"},"body":"Changes since v3:\n\n* Fix styles and syntax. (Thanks to Jeff King and Eric Sunshine)\n* Cache Accept-Language header. (Thanks to Jeff King)\n* Remove floating point numbers. (Thanks to Junio C Hamano)\n* Make the for-loop to get the value of the header simpler.\n* Add more comments.\n\nYi EungJun (1):\n http: Add Accept-Language header if possible\n\n http.c | 134 +++++++++++++++++++++++++++++++++++++++++++++\n remote-curl.c | 2 +\n t/t5550-http-fetch-dumb.sh | 31 +++++++++++\n 3 files changed, 167 insertions(+)\n\n-- \n2.0.1.473.g731ddce.dirty\n"},{"id":"246391","messageId":"1405792730-13539-2-git-send-email-eungjun.yi@navercorp.com","threadId":"37174","inReplyTo":"1405792730-13539-1-git-send-email-eungjun.yi@navercorp.com","subject":"[PATCH v4 1/1] http: Add Accept-Language header if possible","fromName":"Yi EungJun","fromEmail":"semtlenori@gmail.com","sentAt":"2014-07-19T17:58:50Z","receivedAt":"2014-07-19T17:58:50Z","isPatch":true,"sender":{"key":"semtlenori@gmail.com","avatar":"https://gravatar.com/avatar/8363435d2badb3450df0dd7c4ec2113f6e1d62c44dd3cf7dfe83c5a389b8a9bf?d=mp&s=160"},"body":"From: Yi EungJun \n\nAdd an Accept-Language header which indicates the user's preferred\nlanguages defined by $LANGUAGE, $LC_ALL, $LC_MESSAGES and $LANG.\n\nExamples:\n LANGUAGE= -> \"\"\n LANGUAGE=ko:en -> \"Accept-Language: ko, en; q=0.9, *; q=0.1\"\n LANGUAGE=ko LANG=en_US.UTF-8 -> \"Accept-Language: ko, *; q=0.1\"\n LANGUAGE= LANG=en_US.UTF-8 -> \"Accept-Language: en-US, *; q=0.1\"\n\nThis gives git servers a chance to display remote error messages in\nthe user's preferred language.\n\nSigned-off-by: Yi EungJun \n---\n http.c | 134 +++++++++++++++++++++++++++++++++++++++++++++\n remote-curl.c | 2 +\n t/t5550-http-fetch-dumb.sh | 31 +++++++++++\n 3 files changed, 167 insertions(+)\n\ndiff --git a/http.c b/http.c\nindex 3a28b21..ed4e8e1 100644\n--- a/http.c\n+++ b/http.c\n@@ -67,6 +67,8 @@ static struct curl_slist *no_pragma_header;\n \n static struct active_request_slot *active_queue_head;\n \n+static struct strbuf *cached_accept_language = NULL;\n+\n size_t fread_buffer(char *ptr, size_t eltsize, size_t nmemb, void *buffer_)\n {\n \tsize_t size = eltsize * nmemb;\n@@ -512,6 +514,9 @@ void http_cleanup(void)\n \t\tcert_auth.password = NULL;\n \t}\n \tssl_cert_password_required = 0;\n+\n+\tif (cached_accept_language)\n+\t\tstrbuf_release(cached_accept_language);\n }\n \n struct active_request_slot *get_active_slot(void)\n@@ -983,6 +988,129 @@ static void extract_content_type(struct strbuf *raw, struct strbuf *type,\n \t\tstrbuf_addstr(charset, \"ISO-8859-1\");\n }\n \n+/*\n+ * Guess the user's preferred languages from the value in LANGUAGE environment\n+ * variable and LC_MESSAGES locale category.\n+ *\n+ * The result can be a colon-separated list like \"ko:ja:en\".\n+ */\n+static const char *get_preferred_languages(void)\n+{\n+\tconst char *retval;\n+\n+\tretval = getenv(\"LANGUAGE\");\n+\tif (retval && *retval)\n+\t\treturn retval;\n+\n+\tretval = setlocale(LC_MESSAGES, NULL);\n+\tif (retval && *retval &&\n+\t\tstrcmp(retval, \"C\") &&\n+\t\tstrcmp(retval, \"POSIX\"))\n+\t\treturn retval;\n+\n+\treturn NULL;\n+}\n+\n+/*\n+ * Get an Accept-Language header which indicates user's preferred languages.\n+ *\n+ * Examples:\n+ * LANGUAGE= -> \"\"\n+ * LANGUAGE=ko:en -> \"Accept-Language: ko, en; q=0.9, *; q=0.1\"\n+ * LANGUAGE=ko_KR.UTF-8:sr@latin -> \"Accept-Language: ko-KR, sr; q=0.9, *; q=0.1\"\n+ * LANGUAGE=ko LANG=en_US.UTF-8 -> \"Accept-Language: ko, *; q=0.1\"\n+ * LANGUAGE= LANG=en_US.UTF-8 -> \"Accept-Language: en-US, *; q=0.1\"\n+ * LANGUAGE= LANG=C -> \"\"\n+ */\n+static struct strbuf *get_accept_language(void)\n+{\n+\tconst char *lang_begin, *pos;\n+\tint q, max_q;\n+\tint num_langs;\n+\tint decimal_places;\n+\tint is_codeset_or_modifier = 0;\n+\tstatic struct strbuf buf = STRBUF_INIT;\n+\tstruct strbuf q_format_buf = STRBUF_INIT;\n+\tchar *q_format;\n+\n+\tif (cached_accept_language)\n+\t\treturn cached_accept_language;\n+\n+\tlang_begin = get_preferred_languages();\n+\n+\t/* Don't add Accept-Language header if no language is preferred. */\n+\tif (!(lang_begin && *lang_begin)) {\n+\t\tcached_accept_language = &buf;\n+\t\treturn cached_accept_language;\n+\t}\n+\n+\t/* Count number of preferred lang_begin to decide precision of q-factor */\n+\tfor (num_langs = 1, pos = lang_begin; *pos; pos++)\n+\t\tif (*pos == ':')\n+\t\t\tnum_langs++;\n+\n+\t/* Decide the precision for q-factor on number of preferred lang_begin. */\n+\tnum_langs += 1; /* for '*' */\n+\tdecimal_places = 1 + (num_langs > 10) + (num_langs > 100);\n+\tstrbuf_addf(&q_format_buf, \"; q=0.%%0%dd\", decimal_places);\n+\tq_format = strbuf_detach(&q_format_buf, NULL);\n+\tfor (max_q = 1; decimal_places-- > 0;) max_q *= 10;\n+\tq = max_q;\n+\n+\tstrbuf_addstr(&buf, \"Accept-Language: \");\n+\n+\t/*\n+\t * Convert a list of colon-separated locale values [1][2] to a list of\n+\t * comma-separated language tags [3] which can be used as a value of\n+\t * Accept-Language header.\n+\t *\n+\t * [1]: http://pubs.opengroup.org/onlinepubs/007908799/xbd/envvar.html\n+\t * [2]: http://www.gnu.org/software/libc/manual/html_node/Using-gettextized-software.html\n+\t * [3]: http://tools.ietf.org/html/rfc7231#section-5.3.5\n+\t */\n+\tfor (pos = lang_begin; ; pos++) {\n+\t\tif (*pos == ':' || !*pos) {\n+\t\t\t/* Ignore if this character is the first one. */\n+\t\t\tif (pos == lang_begin)\n+\t\t\t\tcontinue;\n+\n+\t\t\tis_codeset_or_modifier = 0;\n+\n+\t\t\t/* Put a q-factor only if it is less than 1.0. */\n+\t\t\tif (q < max_q)\n+\t\t\t\tstrbuf_addf(&buf, q_format, q);\n+\n+\t\t\tif (q > 1)\n+\t\t\t\tq--;\n+\n+\t\t\t/* NULL pos means this is the last language. */\n+\t\t\tif (*pos)\n+\t\t\t\tstrbuf_addstr(&buf, \", \");\n+\t\t\telse\n+\t\t\t\tbreak;\n+\n+\t\t} else if (is_codeset_or_modifier)\n+\t\t\tcontinue;\n+\t\telse if (*pos == '.' || *pos == '@') /* Remove .codeset and @modifier. */\n+\t\t\tis_codeset_or_modifier = 1;\n+\t\telse\n+\t\t\tstrbuf_addch(&buf, *pos == '_' ? '-' : *pos);\n+\t}\n+\n+\t/* Don't add Accept-Language header if no language is preferred. */\n+\tif (q >= max_q) {\n+\t\tcached_accept_language = &buf;\n+\t\treturn cached_accept_language;\n+\t}\n+\n+\t/* Add '*' with minimum q-factor greater than 0.0. */\n+\tstrbuf_addstr(&buf, \", *\");\n+\tstrbuf_addf(&buf, q_format, 1);\n+\n+\tcached_accept_language = &buf;\n+\treturn cached_accept_language;\n+}\n+\n /* http_request() targets */\n #define HTTP_REQUEST_STRBUF\t0\n #define HTTP_REQUEST_FILE\t1\n@@ -995,6 +1123,7 @@ static int http_request(const char *url,\n \tstruct slot_results results;\n \tstruct curl_slist *headers = NULL;\n \tstruct strbuf buf = STRBUF_INIT;\n+\tstruct strbuf* accept_language;\n \tint ret;\n \n \tslot = get_active_slot();\n@@ -1020,6 +1149,11 @@ static int http_request(const char *url,\n \t\t\t\t\t fwrite_buffer);\n \t}\n \n+\taccept_language = get_accept_language();\n+\n+\tif (accept_language && accept_language->len > 0)\n+\t\theaders = curl_slist_append(headers, accept_language->buf);\n+\n \tstrbuf_addstr(&buf, \"Pragma:\");\n \tif (options && options->no_cache)\n \t\tstrbuf_addstr(&buf, \" no-cache\");\ndiff --git a/remote-curl.c b/remote-curl.c\nindex 4493b38..07f2a5d 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -946,6 +946,8 @@ int main(int argc, const char **argv)\n \tstruct strbuf buf = STRBUF_INIT;\n \tint nongit;\n \n+\tgit_setup_gettext();\n+\n \tgit_extract_argv0_path(argv[0]);\n \tsetup_git_directory_gently(&nongit);\n \tif (argc < 2) {\ndiff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh\nindex ac71418..d2dac44 100755\n--- a/t/t5550-http-fetch-dumb.sh\n+++ b/t/t5550-http-fetch-dumb.sh\n@@ -196,5 +196,36 @@ test_expect_success 'reencoding is robust to whitespace oddities' '\n \tgrep \"this is the error message\" stderr\n '\n \n+check_language () {\n+\techo \"Accept-Language: $1\\r\" >expect &&\n+\ttest_must_fail env \\\n+\t\tGIT_CURL_VERBOSE=1 \\\n+\t\tLANGUAGE=$2 \\\n+\t\tLC_ALL=$3 \\\n+\t\tLC_MESSAGES=$4 \\\n+\t\tLANG=$5 \\\n+\t\tgit clone \"$HTTPD_URL/accept/language\" 2>stderr &&\n+\tgrep -i ^Accept-Language: stderr >actual &&\n+\ttest_cmp expect actual\n+}\n+\n+test_expect_success 'git client sends Accept-Language based on LANGUAGE, LC_ALL, LC_MESSAGES and LANG' '\n+\tcheck_language \"ko-KR, *; q=0.1\" ko_KR.UTF-8 de_DE.UTF-8 ja_JP.UTF-8 en_US.UTF-8 &&\n+\tcheck_language \"de-DE, *; q=0.1\" \"\" de_DE.UTF-8 ja_JP.UTF-8 en_US.UTF-8 &&\n+\tcheck_language \"ja-JP, *; q=0.1\" \"\" \"\" ja_JP.UTF-8 en_US.UTF-8 &&\n+\tcheck_language \"en-US, *; q=0.1\" \"\" \"\" \"\" en_US.UTF-8\n+'\n+\n+test_expect_success 'git client sends Accept-Language with many preferred languages' '\n+\tcheck_language \"ko-KR, en-US; q=0.99, fr-CA; q=0.98, de; q=0.97, sr; q=0.96, \\\n+ja; q=0.95, zh; q=0.94, sv; q=0.93, pt; q=0.92, nb; q=0.91, *; q=0.01\" \\\n+\t\tko_KR.EUC-KR:en_US.UTF-8:fr_CA:de.UTF-8@euro:sr@latin:ja:zh:sv:pt:nb\n+'\n+\n+test_expect_success 'git client does not send Accept-Language' '\n+\ttest_must_fail env GIT_CURL_VERBOSE=1 LANGUAGE= git clone \"$HTTPD_URL/accept/language\" 2>stderr &&\n+\t! grep \"^Accept-Language:\" stderr\n+'\n+\n stop_httpd\n test_done\n-- \n2.0.1.473.g731ddce.dirty\n"},{"id":"246474","messageId":"xmqqwqb6ilik.fsf@gitster.dls.corp.google.com","threadId":"37174","inReplyTo":"1405792730-13539-2-git-send-email-eungjun.yi@navercorp.com","subject":"Re: [PATCH v4 1/1] http: Add Accept-Language header if possible","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-07-21T19:01:39Z","receivedAt":"2014-07-21T19:01:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Yi EungJun writes:\n\n> From: Yi EungJun \n>\n> Add an Accept-Language header which indicates the user's preferred\n> languages defined by $LANGUAGE, $LC_ALL, $LC_MESSAGES and $LANG.\n>\n> Examples:\n> LANGUAGE= -> \"\"\n> LANGUAGE=ko:en -> \"Accept-Language: ko, en; q=0.9, *; q=0.1\"\n> LANGUAGE=ko LANG=en_US.UTF-8 -> \"Accept-Language: ko, *; q=0.1\"\n> LANGUAGE= LANG=en_US.UTF-8 -> \"Accept-Language: en-US, *; q=0.1\"\n>\n> This gives git servers a chance to display remote error messages in\n> the user's preferred language.\n>\n> Signed-off-by: Yi EungJun \n> ---\n> http.c | 134 +++++++++++++++++++++++++++++++++++++++++++++\n> remote-curl.c | 2 +\n> t/t5550-http-fetch-dumb.sh | 31 +++++++++++\n> 3 files changed, 167 insertions(+)\n>\n> diff --git a/http.c b/http.c\n> index 3a28b21..ed4e8e1 100644\n> --- a/http.c\n> +++ b/http.c\n> @@ -67,6 +67,8 @@ static struct curl_slist *no_pragma_header;\n> \n> static struct active_request_slot *active_queue_head;\n> \n> +static struct strbuf *cached_accept_language = NULL;\n\nPlease drop \" = NULL\" that is unnecessary for BSS.\n\n> @@ -512,6 +514,9 @@ void http_cleanup(void)\n> \t\tcert_auth.password = NULL;\n> \t}\n> \tssl_cert_password_required = 0;\n> +\n> +\tif (cached_accept_language)\n> +\t\tstrbuf_release(cached_accept_language);\n> }\n\n\n\n> @@ -983,6 +988,129 @@ static void extract_content_type(struct strbuf *raw, struct strbuf *type,\n> \t\tstrbuf_addstr(charset, \"ISO-8859-1\");\n> }\n> \n> +/*\n> + * Guess the user's preferred languages from the value in LANGUAGE environment\n> + * variable and LC_MESSAGES locale category.\n> + *\n> + * The result can be a colon-separated list like \"ko:ja:en\".\n> + */\n> +static const char *get_preferred_languages(void)\n> +{\n> +\tconst char *retval;\n> +\n> +\tretval = getenv(\"LANGUAGE\");\n> +\tif (retval && *retval)\n> +\t\treturn retval;\n> +\n> +\tretval = setlocale(LC_MESSAGES, NULL);\n> +\tif (retval && *retval &&\n> +\t\tstrcmp(retval, \"C\") &&\n> +\t\tstrcmp(retval, \"POSIX\"))\n> +\t\treturn retval;\n> +\n> +\treturn NULL;\n> +}\n> +\n> +/*\n> + * Get an Accept-Language header which indicates user's preferred languages.\n> + *\n> + * Examples:\n> + * LANGUAGE= -> \"\"\n> + * LANGUAGE=ko:en -> \"Accept-Language: ko, en; q=0.9, *; q=0.1\"\n> + * LANGUAGE=ko_KR.UTF-8:sr@latin -> \"Accept-Language: ko-KR, sr; q=0.9, *; q=0.1\"\n> + * LANGUAGE=ko LANG=en_US.UTF-8 -> \"Accept-Language: ko, *; q=0.1\"\n> + * LANGUAGE= LANG=en_US.UTF-8 -> \"Accept-Language: en-US, *; q=0.1\"\n> + * LANGUAGE= LANG=C -> \"\"\n> + */\n> +static struct strbuf *get_accept_language(void)\n> +{\n> +\tconst char *lang_begin, *pos;\n> +\tint q, max_q;\n> +\tint num_langs;\n> +\tint decimal_places;\n> +\tint is_codeset_or_modifier = 0;\n> +\tstatic struct strbuf buf = STRBUF_INIT;\n> +\tstruct strbuf q_format_buf = STRBUF_INIT;\n> +\tchar *q_format;\n> +\n> +\tif (cached_accept_language)\n> +\t\treturn cached_accept_language;\n> +\n> +\tlang_begin = get_preferred_languages();\n> +\n> +\t/* Don't add Accept-Language header if no language is preferred. */\n> +\tif (!(lang_begin && *lang_begin)) {\n\nIt is not wrong per-se, but given how hard get_preferred_languages()\ntries not to return a pointer to an empty string, this seems a bit\noverly defensive to me.\n\n> +\t\tcached_accept_language = &buf;\n> +\t\treturn cached_accept_language;\n\nIt is somewhat unconventional to have a static pointer outside to\npoint at a singleton and then have a singleton actually as a static\nstructure. I would have done without \"buf\" in this function and\ninstead started this function like so:\n\n\tif (cached_accept_language)\n \treturn cached_accept_language;\n\n\tcached_accept_language = xmalloc(sizeof(struct strbuf));\n strbuf_init(cached_accept_language, 0);\n lang_begin = get_preferred_languages();\n\tif (!lang_begin)\n\t\treturn cached_accept_language;\n\n> +\t}\n> +\n> +\t/* Count number of preferred lang_begin to decide precision of q-factor */\n> +\tfor (num_langs = 1, pos = lang_begin; *pos; pos++)\n> +\t\tif (*pos == ':')\n> +\t\t\tnum_langs++;\n> +\n> +\t/* Decide the precision for q-factor on number of preferred lang_begin. */\n> +\tnum_langs += 1; /* for '*' */\n\n\n> +\tdecimal_places = 1 + (num_langs > 10) + (num_langs > 100);\n\nWhat if you got 60000 languages ;-)? I do not think we want to bend\nbackwards and make the code list all 60000 of them, assigning a\nunique and decreasing q value to each of them, forming an overlong\nAccept-Language header, but at the same time, I do not think we want\nto show nonsense output because we compute the precision incorrectly\nhere.\n\n> +\tstrbuf_addf(&q_format_buf, \"; q=0.%%0%dd\", decimal_places);\n> +\tq_format = strbuf_detach(&q_format_buf, NULL);\n\nq_format_buf is an overkill use of strbuf, isn't it? Just\n\n\tchar q_format_buf[32];\n\tsprintf(q_format_buf, \";q=0.%%0%d\", decimal_places);\n\nor something should be more than sufficient, no?\n\n> +\tfor (max_q = 1; decimal_places-- > 0;) max_q *= 10;\n\nAs you have to do one loop like this that amounts to computing log10\nof num_langs, why not compute decimal_places the same way while at\nit? It may also make sense to cap the number of languages to avoid\nspitting out overly long Accept-Language header with practicaly\nuseless list of many languages. That is, something along the lines\nof ... (note that I may very well have off-by-one or off-by-ten\nerrors here you may need to tweak to get right):\n\n if (MAX_LANGS < num_langs)\n \tnum_langs = MAX_LANGS;\n for (max_q = 1, decimal_places = 1;\n max_q < num_langs;\n decimal_places++, max_q *= 10)\n ;\n\nIf you are to use the MAX_LANGS cap, the main loop would also need\nto pay attention to it by breaking out of the loop early before you\nreach the end of the string, of course.\n\n> +\tq = max_q;\n> +\n> +\tstrbuf_addstr(&buf, \"Accept-Language: \");\n> +\n> +\t/*\n> +\t * Convert a list of colon-separated locale values [1][2] to a list of\n> +\t * comma-separated language tags [3] which can be used as a value of\n> +\t * Accept-Language header.\n> +\t *\n> +\t * [1]: http://pubs.opengroup.org/onlinepubs/007908799/xbd/envvar.html\n> +\t * [2]: http://www.gnu.org/software/libc/manual/html_node/Using-gettextized-software.html\n> +\t * [3]: http://tools.ietf.org/html/rfc7231#section-5.3.5\n> +\t */\n> +\tfor (pos = lang_begin; ; pos++) {\n> +\t\tif (*pos == ':' || !*pos) {\n> +\t\t\t/* Ignore if this character is the first one. */\n> +\t\t\tif (pos == lang_begin)\n> +\t\t\t\tcontinue;\n\nBy doing this \"ignore empty\" here, but not doing the same when you\ncount num_langs, are you potentially miscounting num_langs?\n\n> +\t\t\tis_codeset_or_modifier = 0;\n> +\n> +\t\t\t/* Put a q-factor only if it is less than 1.0. */\n> +\t\t\tif (q < max_q)\n\n... is it the same thing as \"do not do this for the first round, but\ndo so for all the other round\"?\n\n> +\t\t\t\tstrbuf_addf(&buf, q_format, q);\n> +\n> +\t\t\tif (q > 1)\n\nHmm, I am puzzled. C this ever be an issue (unless you have\noff-by-one error or you add \"cap num_langs to MAX_LANGS\", that is)?\n\n> +\t\t\t\tq--;\n\n> +\t\t\t/* NULL pos means this is the last language. */\n> +\t\t\tif (*pos)\n> +\t\t\t\tstrbuf_addstr(&buf, \", \");\n> +\t\t\telse\n> +\t\t\t\tbreak;\n> +\n> +\t\t} else if (is_codeset_or_modifier)\n> +\t\t\tcontinue;\n> +\t\telse if (*pos == '.' || *pos == '@') /* Remove .codeset and @modifier. */\n> +\t\t\tis_codeset_or_modifier = 1;\n> +\t\telse\n> +\t\t\tstrbuf_addch(&buf, *pos == '_' ? '-' : *pos);\n> +\t}\n> +\n> +\t/* Don't add Accept-Language header if no language is preferred. */\n> +\tif (q >= max_q) {\n\nCan q go over max_q, or is it \"q may be max_q\"? In other words, is\nthis essentially saying \"if we did not find any language in the\npreferred languages list\"?\n\n> +\t\tcached_accept_language = &buf;\n> +\t\treturn cached_accept_language;\n> +\t}\n> +\n> +\t/* Add '*' with minimum q-factor greater than 0.0. */\n> +\tstrbuf_addstr(&buf, \", *\");\n> +\tstrbuf_addf(&buf, q_format, 1);\n> +\n> +\tcached_accept_language = &buf;\n> +\treturn cached_accept_language;\n> +}\n> +\n> /* http_request() targets */\n> #define HTTP_REQUEST_STRBUF\t0\n> #define HTTP_REQUEST_FILE\t1\n> @@ -995,6 +1123,7 @@ static int http_request(const char *url,\n> \tstruct slot_results results;\n> \tstruct curl_slist *headers = NULL;\n> \tstruct strbuf buf = STRBUF_INIT;\n> +\tstruct strbuf* accept_language;\n\nAs we write in C, not C++, our asterisks stick to the variable, not\nthe type.\n"},{"id":"247191","messageId":"CAFT+Tg-3jCEdpS1K5bGTG-Pv6ne+5q4rD5q2=+KWVjckfy5W8g@mail.gmail.com","threadId":"37174","inReplyTo":"xmqqwqb6ilik.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v4 1/1] http: Add Accept-Language header if possible","fromName":"Yi, EungJun","fromEmail":"semtlenori@gmail.com","sentAt":"2014-08-03T07:35:52Z","receivedAt":"2014-08-03T07:35:52Z","isPatch":true,"sender":{"key":"semtlenori@gmail.com","avatar":"https://gravatar.com/avatar/8363435d2badb3450df0dd7c4ec2113f6e1d62c44dd3cf7dfe83c5a389b8a9bf?d=mp&s=160"},"body":"Thanks very much for your detailed review and sorry for late reply.\n\n2014-07-22 4:01 GMT+09:00 Junio C Hamano :\n> Yi EungJun writes:\n>\n>> From: Yi EungJun \n>>\n>> Add an Accept-Language header which indicates the user's preferred\n>> languages defined by $LANGUAGE, $LC_ALL, $LC_MESSAGES and $LANG.\n>>\n>> Examples:\n>> LANGUAGE= -> \"\"\n>> LANGUAGE=ko:en -> \"Accept-Language: ko, en; q=0.9, *; q=0.1\"\n>> LANGUAGE=ko LANG=en_US.UTF-8 -> \"Accept-Language: ko, *; q=0.1\"\n>> LANGUAGE= LANG=en_US.UTF-8 -> \"Accept-Language: en-US, *; q=0.1\"\n>>\n>> This gives git servers a chance to display remote error messages in\n>> the user's preferred language.\n>>\n>> Signed-off-by: Yi EungJun \n>> ---\n>> http.c | 134 +++++++++++++++++++++++++++++++++++++++++++++\n>> remote-curl.c | 2 +\n>> t/t5550-http-fetch-dumb.sh | 31 +++++++++++\n>> 3 files changed, 167 insertions(+)\n>>\n>> diff --git a/http.c b/http.c\n>> index 3a28b21..ed4e8e1 100644\n>> --- a/http.c\n>> +++ b/http.c\n>> @@ -67,6 +67,8 @@ static struct curl_slist *no_pragma_header;\n>>\n>> static struct active_request_slot *active_queue_head;\n>>\n>> +static struct strbuf *cached_accept_language = NULL;\n>\n> Please drop \" = NULL\" that is unnecessary for BSS.\n\nThanks, I'll fix it.\n\n>\n>> @@ -512,6 +514,9 @@ void http_cleanup(void)\n>> cert_auth.password = NULL;\n>> }\n>> ssl_cert_password_required = 0;\n>> +\n>> + if (cached_accept_language)\n>> + strbuf_release(cached_accept_language);\n>> }\n>\n>\n>\n>> @@ -983,6 +988,129 @@ static void extract_content_type(struct strbuf *raw, struct strbuf *type,\n>> strbuf_addstr(charset, \"ISO-8859-1\");\n>> }\n>>\n>> +/*\n>> + * Guess the user's preferred languages from the value in LANGUAGE environment\n>> + * variable and LC_MESSAGES locale category.\n>> + *\n>> + * The result can be a colon-separated list like \"ko:ja:en\".\n>> + */\n>> +static const char *get_preferred_languages(void)\n>> +{\n>> + const char *retval;\n>> +\n>> + retval = getenv(\"LANGUAGE\");\n>> + if (retval && *retval)\n>> + return retval;\n>> +\n>> + retval = setlocale(LC_MESSAGES, NULL);\n>> + if (retval && *retval &&\n>> + strcmp(retval, \"C\") &&\n>> + strcmp(retval, \"POSIX\"))\n>> + return retval;\n>> +\n>> + return NULL;\n>> +}\n>> +\n>> +/*\n>> + * Get an Accept-Language header which indicates user's preferred languages.\n>> + *\n>> + * Examples:\n>> + * LANGUAGE= -> \"\"\n>> + * LANGUAGE=ko:en -> \"Accept-Language: ko, en; q=0.9, *; q=0.1\"\n>> + * LANGUAGE=ko_KR.UTF-8:sr@latin -> \"Accept-Language: ko-KR, sr; q=0.9, *; q=0.1\"\n>> + * LANGUAGE=ko LANG=en_US.UTF-8 -> \"Accept-Language: ko, *; q=0.1\"\n>> + * LANGUAGE= LANG=en_US.UTF-8 -> \"Accept-Language: en-US, *; q=0.1\"\n>> + * LANGUAGE= LANG=C -> \"\"\n>> + */\n>> +static struct strbuf *get_accept_language(void)\n>> +{\n>> + const char *lang_begin, *pos;\n>> + int q, max_q;\n>> + int num_langs;\n>> + int decimal_places;\n>> + int is_codeset_or_modifier = 0;\n>> + static struct strbuf buf = STRBUF_INIT;\n>> + struct strbuf q_format_buf = STRBUF_INIT;\n>> + char *q_format;\n>> +\n>> + if (cached_accept_language)\n>> + return cached_accept_language;\n>> +\n>> + lang_begin = get_preferred_languages();\n>> +\n>> + /* Don't add Accept-Language header if no language is preferred. */\n>> + if (!(lang_begin && *lang_begin)) {\n>\n> It is not wrong per-se, but given how hard get_preferred_languages()\n> tries not to return a pointer to an empty string, this seems a bit\n> overly defensive to me.\n\nThanks, I'll fix it.\n\n>\n>> + cached_accept_language = &buf;\n>> + return cached_accept_language;\n>\n> It is somewhat unconventional to have a static pointer outside to\n> point at a singleton and then have a singleton actually as a static\n> structure. I would have done without \"buf\" in this function and\n> instead started this function like so:\n>\n> if (cached_accept_language)\n> return cached_accept_language;\n>\n> cached_accept_language = xmalloc(sizeof(struct strbuf));\n> strbuf_init(cached_accept_language, 0);\n> lang_begin = get_preferred_languages();\n> if (!lang_begin)\n> return cached_accept_language;\n>\n\nThanks, I'll fix it as you suggest.\n\n>> + }\n>> +\n>> + /* Count number of preferred lang_begin to decide precision of q-factor */\n>> + for (num_langs = 1, pos = lang_begin; *pos; pos++)\n>> + if (*pos == ':')\n>> + num_langs++;\n>> +\n>> + /* Decide the precision for q-factor on number of preferred lang_begin. */\n>> + num_langs += 1; /* for '*' */\n>\n>\n>> + decimal_places = 1 + (num_langs > 10) + (num_langs > 100);\n>\n> What if you got 60000 languages ;-)? I do not think we want to bend\n> backwards and make the code list all 60000 of them, assigning a\n> unique and decreasing q value to each of them, forming an overlong\n> Accept-Language header, but at the same time, I do not think we want\n> to show nonsense output because we compute the precision incorrectly\n> here.\n\nIn that case, less-preferred 59,001 languages will have same q-value\nof 0.001. Unfortunately, there is no way to represent user's\npreferences of over 1,000 languages since the HTTP specification\nrestricts the range of q-value from 0.001 to 1.000 [1]. I think this\ncode reflects user's preferences better than Google chrome whose\nminimum q-value is 0.2 and Mozilla firefox whose minimum q-value is\n0.01 [2].\n\n>\n>> + strbuf_addf(&q_format_buf, \"; q=0.%%0%dd\", decimal_places);\n>> + q_format = strbuf_detach(&q_format_buf, NULL);\n>\n> q_format_buf is an overkill use of strbuf, isn't it? Just\n>\n> char q_format_buf[32];\n> sprintf(q_format_buf, \";q=0.%%0%d\", decimal_places);\n>\n> or something should be more than sufficient, no?\n\nThanks, I'll fix it as you suggest.\n\n>\n>> + for (max_q = 1; decimal_places-- > 0;) max_q *= 10;\n>\n> As you have to do one loop like this that amounts to computing log10\n> of num_langs, why not compute decimal_places the same way while at\n> it? It may also make sense to cap the number of languages to avoid\n> spitting out overly long Accept-Language header with practicaly\n> useless list of many languages. That is, something along the lines\n> of ... (note that I may very well have off-by-one or off-by-ten\n> errors here you may need to tweak to get right):\n>\n> if (MAX_LANGS < num_langs)\n> num_langs = MAX_LANGS;\n> for (max_q = 1, decimal_places = 1;\n> max_q < num_langs;\n> decimal_places++, max_q *= 10)\n> ;\n>\n> If you are to use the MAX_LANGS cap, the main loop would also need\n> to pay attention to it by breaking out of the loop early before you\n> reach the end of the string, of course.\n\nThanks for good point. As you said, we should limit the length of the\nvalue of Accept-Language header because some HTTP servers respond 4xx\nClient Error if any header's value is very long (4KB or more) [3].\n\nBut MAX_LANGS may not be enough because the header can be too long\neven if the number of languages does not exceed MAX_LANGS if language\ntag is too long. Many of language tags are 2-3 characters but some are\n11 characters; Even it is possible that a user has a very long custom\nlanguage tag.\n\nI think this problem can be solved by one of these solutions:\n\nA. Set MAX_LANGS conservatively (100 or less).\nB. Limit the length of Accept-Language header directly (4KB or less).\nC. Negotiate with server; Resend a request with shorter\nAccept-Language header if the server responds 4xx error.\n\n>\n>> + q = max_q;\n>> +\n>> + strbuf_addstr(&buf, \"Accept-Language: \");\n>> +\n>> + /*\n>> + * Convert a list of colon-separated locale values [1][2] to a list of\n>> + * comma-separated language tags [3] which can be used as a value of\n>> + * Accept-Language header.\n>> + *\n>> + * [1]: http://pubs.opengroup.org/onlinepubs/007908799/xbd/envvar.html\n>> + * [2]: http://www.gnu.org/software/libc/manual/html_node/Using-gettextized-software.html\n>> + * [3]: http://tools.ietf.org/html/rfc7231#section-5.3.5\n>> + */\n>> + for (pos = lang_begin; ; pos++) {\n>> + if (*pos == ':' || !*pos) {\n>> + /* Ignore if this character is the first one. */\n>> + if (pos == lang_begin)\n>> + continue;\n>\n> By doing this \"ignore empty\" here, but not doing the same when you\n> count num_langs, are you potentially miscounting num_langs?\n\nYes, num_langs can be larger than actual number of languages. For\nexample, if LANGUAGE=\"en:\" num_langs is 2. I wanted to make the logic\nto compute num_langs as simple as possible and thought num_langs does\nnot need to be very accurate because it is used only to compute the\nprecision of q-factor (max_q and decimal_places).\n\nDo we need to compute num_langs accurately?\n\n>\n>> + is_codeset_or_modifier = 0;\n>> +\n>> + /* Put a q-factor only if it is less than 1.0. */\n>> + if (q < max_q)\n>\n> ... is it the same thing as \"do not do this for the first round, but\n> do so for all the other round\"?\n\nYes, q-factor is q/max_q and q-factor of 1.0 is not necessary because:\n> if no \"q\" parameter is present, the default weight is 1.\n> -- http://tools.ietf.org/html/rfc7231#section-5.3.1\n\n>\n>> + strbuf_addf(&buf, q_format, q);\n>> +\n>> + if (q > 1)\n>\n> Hmm, I am puzzled. C this ever be an issue (unless you have\n> off-by-one error or you add \"cap num_langs to MAX_LANGS\", that is)?\n\nYes, it may be an issue if the number of languages is larger than the\nmax_q. max_q must not be larger than 1000 but the number of languages\nmay be.\n\nBut this if-statement will be not necessary if we limit the number of\nlanguages to 1,000 or less (by MAX_LANGS you suggest).\n\n>\n>> + q--;\n>\n>> + /* NULL pos means this is the last language. */\n>> + if (*pos)\n>> + strbuf_addstr(&buf, \", \");\n>> + else\n>> + break;\n>> +\n>> + } else if (is_codeset_or_modifier)\n>> + continue;\n>> + else if (*pos == '.' || *pos == '@') /* Remove .codeset and @modifier. */\n>> + is_codeset_or_modifier = 1;\n>> + else\n>> + strbuf_addch(&buf, *pos == '_' ? '-' : *pos);\n>> + }\n>> +\n>> + /* Don't add Accept-Language header if no language is preferred. */\n>> + if (q >= max_q) {\n>\n> Can q go over max_q, or is it \"q may be max_q\"? In other words, is\n> this essentially saying \"if we did not find any language in the\n> preferred languages list\"?\n\nYes. q may be max_q if we did not find any language in the preferred\nlanguages list. But there is no chance that q goes over max_q.\n\nBut it will be not necessary if num_langs is computed accurately.\n\n>\n>> + cached_accept_language = &buf;\n>> + return cached_accept_language;\n>> + }\n>> +\n>> + /* Add '*' with minimum q-factor greater than 0.0. */\n>> + strbuf_addstr(&buf, \", *\");\n>> + strbuf_addf(&buf, q_format, 1);\n>> +\n>> + cached_accept_language = &buf;\n>> + return cached_accept_language;\n>> +}\n>> +\n>> /* http_request() targets */\n>> #define HTTP_REQUEST_STRBUF 0\n>> #define HTTP_REQUEST_FILE 1\n>> @@ -995,6 +1123,7 @@ static int http_request(const char *url,\n>> struct slot_results results;\n>> struct curl_slist *headers = NULL;\n>> struct strbuf buf = STRBUF_INIT;\n>> + struct strbuf* accept_language;\n>\n> As we write in C, not C++, our asterisks stick to the variable, not\n> the type.\n\nThanks, I'll fix it.\n\n[1]: http://tools.ietf.org/html/rfc7231#section-5.3.1\n[2]: https://hg.mozilla.org/integration/mozilla-inbound/rev/1418f9ce6f8b\n[3]: http://stackoverflow.com/questions/686217/maximum-on-http-header-values/8623061#8623061\n"},{"id":"252855","messageId":"1417522356-24212-1-git-send-email-eungjun.yi@navercorp.com","threadId":"37174","inReplyTo":"1405792730-13539-1-git-send-email-eungjun.yi@navercorp.com","subject":"[PATCH v5 0/1] http: Add Accept-Language header if possible","fromName":"Yi EungJun","fromEmail":"semtlenori@gmail.com","sentAt":"2014-12-02T12:12:35Z","receivedAt":"2014-12-02T12:12:35Z","isPatch":true,"sender":{"key":"semtlenori@gmail.com","avatar":"https://gravatar.com/avatar/8363435d2badb3450df0dd7c4ec2113f6e1d62c44dd3cf7dfe83c5a389b8a9bf?d=mp&s=160"},"body":"Changes since v4\n\n* Fix styles as Junio C Hamano suggested.\n* Limit number of languages and length of Accept-Language header.\n\nYi EungJun (1):\n http: Add Accept-Language header if possible\n\n http.c | 154 +++++++++++++++++++++++++++++++++++++++++++++\n remote-curl.c | 2 +\n t/t5550-http-fetch-dumb.sh | 31 +++++++++\n 3 files changed, 187 insertions(+)\n\n-- \n2.2.0\n"},{"id":"252856","messageId":"1417522356-24212-2-git-send-email-eungjun.yi@navercorp.com","threadId":"37174","inReplyTo":"1417522356-24212-1-git-send-email-eungjun.yi@navercorp.com","subject":"[PATCH v5 1/1] http: Add Accept-Language header if possible","fromName":"Yi EungJun","fromEmail":"semtlenori@gmail.com","sentAt":"2014-12-02T12:12:36Z","receivedAt":"2014-12-02T12:12:36Z","isPatch":true,"sender":{"key":"semtlenori@gmail.com","avatar":"https://gravatar.com/avatar/8363435d2badb3450df0dd7c4ec2113f6e1d62c44dd3cf7dfe83c5a389b8a9bf?d=mp&s=160"},"body":"From: Yi EungJun \n\nAdd an Accept-Language header which indicates the user's preferred\nlanguages defined by $LANGUAGE, $LC_ALL, $LC_MESSAGES and $LANG.\n\nExamples:\n LANGUAGE= -> \"\"\n LANGUAGE=ko:en -> \"Accept-Language: ko, en;q=0.9, *;q=0.1\"\n LANGUAGE=ko LANG=en_US.UTF-8 -> \"Accept-Language: ko, *;q=0.1\"\n LANGUAGE= LANG=en_US.UTF-8 -> \"Accept-Language: en-US, *;q=0.1\"\n\nThis gives git servers a chance to display remote error messages in\nthe user's preferred language.\n\nLimit the number of languages to 1,000 because q-value must not be\nsmaller than 0.001, and limit the length of Accept-Language header to\n4,000 bytes for some HTTP servers which cannot accept such long header.\n\nSigned-off-by: Yi EungJun \n---\n http.c | 154 +++++++++++++++++++++++++++++++++++++++++++++\n remote-curl.c | 2 +\n t/t5550-http-fetch-dumb.sh | 31 +++++++++\n 3 files changed, 187 insertions(+)\n\ndiff --git a/http.c b/http.c\nindex 040f362..69624af 100644\n--- a/http.c\n+++ b/http.c\n@@ -68,6 +68,8 @@ static struct curl_slist *no_pragma_header;\n \n static struct active_request_slot *active_queue_head;\n \n+static struct strbuf *cached_accept_language;\n+\n size_t fread_buffer(char *ptr, size_t eltsize, size_t nmemb, void *buffer_)\n {\n \tsize_t size = eltsize * nmemb;\n@@ -515,6 +517,9 @@ void http_cleanup(void)\n \t\tcert_auth.password = NULL;\n \t}\n \tssl_cert_password_required = 0;\n+\n+\tif (cached_accept_language)\n+\t\tstrbuf_release(cached_accept_language);\n }\n \n struct active_request_slot *get_active_slot(void)\n@@ -986,6 +991,149 @@ static void extract_content_type(struct strbuf *raw, struct strbuf *type,\n \t\tstrbuf_addstr(charset, \"ISO-8859-1\");\n }\n \n+/*\n+ * Guess the user's preferred languages from the value in LANGUAGE environment\n+ * variable and LC_MESSAGES locale category.\n+ *\n+ * The result can be a colon-separated list like \"ko:ja:en\".\n+ */\n+static const char *get_preferred_languages(void)\n+{\n+\tconst char *retval;\n+\n+\tretval = getenv(\"LANGUAGE\");\n+\tif (retval && *retval)\n+\t\treturn retval;\n+\n+\tretval = setlocale(LC_MESSAGES, NULL);\n+\tif (retval && *retval &&\n+\t\tstrcmp(retval, \"C\") &&\n+\t\tstrcmp(retval, \"POSIX\"))\n+\t\treturn retval;\n+\n+\treturn NULL;\n+}\n+\n+/*\n+ * Get an Accept-Language header which indicates user's preferred languages.\n+ *\n+ * Examples:\n+ * LANGUAGE= -> \"\"\n+ * LANGUAGE=ko:en -> \"Accept-Language: ko, en; q=0.9, *; q=0.1\"\n+ * LANGUAGE=ko_KR.UTF-8:sr@latin -> \"Accept-Language: ko-KR, sr; q=0.9, *; q=0.1\"\n+ * LANGUAGE=ko LANG=en_US.UTF-8 -> \"Accept-Language: ko, *; q=0.1\"\n+ * LANGUAGE= LANG=en_US.UTF-8 -> \"Accept-Language: en-US, *; q=0.1\"\n+ * LANGUAGE= LANG=C -> \"\"\n+ */\n+static struct strbuf *get_accept_language(void)\n+{\n+\tconst char *lang_begin, *pos;\n+\tint q, max_q;\n+\tint num_langs;\n+\tint decimal_places;\n+\tint is_codeset_or_modifier = 0;\n+\tchar q_format[32];\n+\t/*\n+\t * MAX_LANGS must not be larger than 1,000. If it is larger than that,\n+\t * q-value will be smaller than 0.001, the minimum q-value the HTTP\n+\t * specification allows [1].\n+\t *\n+\t * [1]: http://tools.ietf.org/html/rfc7231#section-5.3.1\n+\t */\n+\tconst int MAX_LANGS = 1000;\n+\tconst int MAX_SIZE_OF_HEADER = 4000;\n+\tint last_size = 0;\n+\n+\tif (cached_accept_language)\n+\t\treturn cached_accept_language;\n+\n+\tcached_accept_language = xmalloc(sizeof(struct strbuf));\n+\tstrbuf_init(cached_accept_language, 0);\n+\tlang_begin = get_preferred_languages();\n+\n+\t/* Don't add Accept-Language header if no language is preferred. */\n+\tif (!lang_begin)\n+\t\treturn cached_accept_language;\n+\n+\t/* Count number of preferred lang_begin to decide precision of q-factor. */\n+\tfor (num_langs = 1, pos = lang_begin; *pos; pos++)\n+\t\tif (*pos == ':')\n+\t\t\tnum_langs++;\n+\n+\t/* Decide the precision for q-factor on number of preferred lang_begin. */\n+\tnum_langs += 1; /* for '*' */\n+\n+\tif (MAX_LANGS < num_langs)\n+\t\tnum_langs = MAX_LANGS;\n+\n+\tfor (max_q = 1, decimal_places = 0;\n+\t\tmax_q < num_langs;\n+\t\tdecimal_places++, max_q *= 10);\n+\n+\tsprintf(q_format, \";q=0.%%0%dd\", decimal_places);\n+\n+\tq = max_q;\n+\n+\tstrbuf_addstr(cached_accept_language, \"Accept-Language: \");\n+\n+\t/*\n+\t * Convert a list of colon-separated locale values [1][2] to a list of\n+\t * comma-separated language tags [3] which can be used as a value of\n+\t * Accept-Language header.\n+\t *\n+\t * [1]: http://pubs.opengroup.org/onlinepubs/007908799/xbd/envvar.html\n+\t * [2]: http://www.gnu.org/software/libc/manual/html_node/Using-gettextized-software.html\n+\t * [3]: http://tools.ietf.org/html/rfc7231#section-5.3.5\n+\t */\n+\tfor (pos = lang_begin; ; pos++) {\n+\t\tif (*pos == ':' || !*pos) {\n+\t\t\t/* Ignore if this character is the first one. */\n+\t\t\tif (pos == lang_begin)\n+\t\t\t\tcontinue;\n+\n+\t\t\tis_codeset_or_modifier = 0;\n+\n+\t\t\t/* Put a q-factor only if it is less than 1.0. */\n+\t\t\tif (q < max_q)\n+\t\t\t\tstrbuf_addf(cached_accept_language, q_format, q);\n+\n+\t\t\tif (q > 1)\n+\t\t\t\tq--;\n+\n+\t\t\tlast_size = cached_accept_language->len;\n+\n+\t\t\t/* NULL pos means this is the last language. */\n+\t\t\tif (*pos)\n+\t\t\t\tstrbuf_addstr(cached_accept_language, \", \");\n+\t\t\telse\n+\t\t\t\tbreak;\n+\n+\t\t} else if (is_codeset_or_modifier)\n+\t\t\tcontinue;\n+\t\telse if (*pos == '.' || *pos == '@') /* Remove .codeset and @modifier. */\n+\t\t\tis_codeset_or_modifier = 1;\n+\t\telse\n+\t\t\tstrbuf_addch(cached_accept_language, *pos == '_' ? '-' : *pos);\n+\n+\t\tif (cached_accept_language->len > MAX_SIZE_OF_HEADER) {\n+\t\t\tstrbuf_remove(cached_accept_language, last_size,\n+\t\t\t\t\tcached_accept_language->len - last_size);\n+\t\t\tbreak;\n+\t\t}\n+\t}\n+\n+\t/* Don't add Accept-Language header if no language is preferred. */\n+\tif (q >= max_q) {\n+\t\treturn cached_accept_language;\n+\t}\n+\n+\t/* Add '*' with minimum q-factor greater than 0.0. */\n+\tstrbuf_addstr(cached_accept_language, \", *\");\n+\tstrbuf_addf(cached_accept_language, q_format, 1);\n+\n+\treturn cached_accept_language;\n+}\n+\n /* http_request() targets */\n #define HTTP_REQUEST_STRBUF\t0\n #define HTTP_REQUEST_FILE\t1\n@@ -998,6 +1146,7 @@ static int http_request(const char *url,\n \tstruct slot_results results;\n \tstruct curl_slist *headers = NULL;\n \tstruct strbuf buf = STRBUF_INIT;\n+\tstruct strbuf *accept_language;\n \tint ret;\n \n \tslot = get_active_slot();\n@@ -1023,6 +1172,11 @@ static int http_request(const char *url,\n \t\t\t\t\t fwrite_buffer);\n \t}\n \n+\taccept_language = get_accept_language();\n+\n+\tif (accept_language && accept_language->len > 0)\n+\t\theaders = curl_slist_append(headers, accept_language->buf);\n+\n \tstrbuf_addstr(&buf, \"Pragma:\");\n \tif (options && options->no_cache)\n \t\tstrbuf_addstr(&buf, \" no-cache\");\ndiff --git a/remote-curl.c b/remote-curl.c\nindex dd63bc2..04989e5 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -962,6 +962,8 @@ int main(int argc, const char **argv)\n \tstruct strbuf buf = STRBUF_INIT;\n \tint nongit;\n \n+\tgit_setup_gettext();\n+\n \tgit_extract_argv0_path(argv[0]);\n \tsetup_git_directory_gently(&nongit);\n \tif (argc < 2) {\ndiff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh\nindex ac71418..197c361 100755\n--- a/t/t5550-http-fetch-dumb.sh\n+++ b/t/t5550-http-fetch-dumb.sh\n@@ -196,5 +196,36 @@ test_expect_success 'reencoding is robust to whitespace oddities' '\n \tgrep \"this is the error message\" stderr\n '\n \n+check_language () {\n+\techo \"Accept-Language: $1\\r\" >expect &&\n+\ttest_must_fail env \\\n+\t\tGIT_CURL_VERBOSE=1 \\\n+\t\tLANGUAGE=$2 \\\n+\t\tLC_ALL=$3 \\\n+\t\tLC_MESSAGES=$4 \\\n+\t\tLANG=$5 \\\n+\t\tgit clone \"$HTTPD_URL/accept/language\" 2>stderr &&\n+\tgrep -i ^Accept-Language: stderr >actual &&\n+\ttest_cmp expect actual\n+}\n+\n+test_expect_success 'git client sends Accept-Language based on LANGUAGE, LC_ALL, LC_MESSAGES and LANG' '\n+\tcheck_language \"ko-KR, *;q=0.1\" ko_KR.UTF-8 de_DE.UTF-8 ja_JP.UTF-8 en_US.UTF-8 &&\n+\tcheck_language \"de-DE, *;q=0.1\" \"\" de_DE.UTF-8 ja_JP.UTF-8 en_US.UTF-8 &&\n+\tcheck_language \"ja-JP, *;q=0.1\" \"\" \"\" ja_JP.UTF-8 en_US.UTF-8 &&\n+\tcheck_language \"en-US, *;q=0.1\" \"\" \"\" \"\" en_US.UTF-8\n+'\n+\n+test_expect_success 'git client sends Accept-Language with many preferred languages' '\n+\tcheck_language \"ko-KR, en-US;q=0.99, fr-CA;q=0.98, de;q=0.97, sr;q=0.96, \\\n+ja;q=0.95, zh;q=0.94, sv;q=0.93, pt;q=0.92, nb;q=0.91, *;q=0.01\" \\\n+\t\tko_KR.EUC-KR:en_US.UTF-8:fr_CA:de.UTF-8@euro:sr@latin:ja:zh:sv:pt:nb\n+'\n+\n+test_expect_success 'git client does not send Accept-Language' '\n+\ttest_must_fail env GIT_CURL_VERBOSE=1 LANGUAGE= git clone \"$HTTPD_URL/accept/language\" 2>stderr &&\n+\t! grep \"^Accept-Language:\" stderr\n+'\n+\n stop_httpd\n test_done\n-- \n2.2.0\n"},{"id":"252971","messageId":"xmqqfvcwiof7.fsf@gitster.dls.corp.google.com","threadId":"37174","inReplyTo":"1417522356-24212-2-git-send-email-eungjun.yi@navercorp.com","subject":"Re: [PATCH v5 1/1] http: Add Accept-Language header if possible","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-12-03T18:22:20Z","receivedAt":"2014-12-03T18:22:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Yi EungJun writes:\n\n> From: Yi EungJun \n>\n> Add an Accept-Language header which indicates the user's preferred\n> languages defined by $LANGUAGE, $LC_ALL, $LC_MESSAGES and $LANG.\n>\n> Examples:\n> LANGUAGE= -> \"\"\n> LANGUAGE=ko:en -> \"Accept-Language: ko, en;q=0.9, *;q=0.1\"\n> LANGUAGE=ko LANG=en_US.UTF-8 -> \"Accept-Language: ko, *;q=0.1\"\n> LANGUAGE= LANG=en_US.UTF-8 -> \"Accept-Language: en-US, *;q=0.1\"\n>\n> This gives git servers a chance to display remote error messages in\n> the user's preferred language.\n>\n> Limit the number of languages to 1,000 because q-value must not be\n> smaller than 0.001, and limit the length of Accept-Language header to\n> 4,000 bytes for some HTTP servers which cannot accept such long header.\n>\n> Signed-off-by: Yi EungJun \n> ---\n> http.c | 154 +++++++++++++++++++++++++++++++++++++++++++++\n> remote-curl.c | 2 +\n> t/t5550-http-fetch-dumb.sh | 31 +++++++++\n> 3 files changed, 187 insertions(+)\n>\n> diff --git a/http.c b/http.c\n> index 040f362..69624af 100644\n> --- a/http.c\n> +++ b/http.c\n> @@ -68,6 +68,8 @@ static struct curl_slist *no_pragma_header;\n> \n> static struct active_request_slot *active_queue_head;\n> \n> +static struct strbuf *cached_accept_language;\n> +\n> size_t fread_buffer(char *ptr, size_t eltsize, size_t nmemb, void *buffer_)\n> {\n> \tsize_t size = eltsize * nmemb;\n> @@ -515,6 +517,9 @@ void http_cleanup(void)\n> \t\tcert_auth.password = NULL;\n> \t}\n> \tssl_cert_password_required = 0;\n> +\n> +\tif (cached_accept_language)\n> +\t\tstrbuf_release(cached_accept_language);\n\nIs this correct?\n\nYou still keep cached_accept_language pointer itself, so the next\ncall to get_accept_language() would say \"Ah, cached_accept_language\nis there, so its contents (which is empty because we released it\nhere) must be valid and to be reused\". Perhaps you want to free it,\ntoo, i.e.\n\n\tif (cached_accept_language) {\n\t\tstrbuf_release(cached_accept_language);\n free(cached_accept_language);\n cached_accept_language = NULL;\n\t}\n\nor something?\n\n> +static struct strbuf *get_accept_language(void)\n> +{\n> + ...\n> +\tif (cached_accept_language)\n> +\t\treturn cached_accept_language;\n> +\n> +\tcached_accept_language = xmalloc(sizeof(struct strbuf));\n> + ...\n> +\tfor (max_q = 1, decimal_places = 0;\n> +\t\tmax_q < num_langs;\n> +\t\tdecimal_places++, max_q *= 10);\n\nHave that \"empty statement\" on its own separate line, i.e.\n\n\tfor (a, counter = 0;\n b;\n c, counter++)\n ; /* just counting */\n\nAlternatively, you can make it more obvious that the purpose of loop\nis to count, i.e.\n\n\tfor (a, counter = 0;\n b;\n c)\n counter++;\n\n> +test_expect_success 'git client does not send Accept-Language' '\n> +\ttest_must_fail env GIT_CURL_VERBOSE=1 LANGUAGE= git clone \"$HTTPD_URL/accept/language\" 2>stderr &&\n> +\t! grep \"^Accept-Language:\" stderr\n> +'\n\nHmph, this test smells a bit brittle. What is the reason you expect\n\"git clone\" to fail? Is it because there is no repository at the\nnamed URL at \"$HTTPD_URL/accept/language\"? Is that the only plausible\nreason for a failure?\n\nIt might be better to use the URL to a repository that is expected\nto be served by the server started in this test and expect success.\nIf it bothers you that \"clone\" creates a new copy that is otherwise\nunused by this test, you can use something like \"ls-remote\" instead,\nI would think.\n"},{"id":"252976","messageId":"CAPig+cTsULQPxoaSQ-ZvjWJ9Rgpdf3zG7ObPg4TnxFbXT9TwnA@mail.gmail.com","threadId":"37174","inReplyTo":"1417522356-24212-2-git-send-email-eungjun.yi@navercorp.com","subject":"Re: [PATCH v5 1/1] http: Add Accept-Language header if possible","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-12-03T19:31:40Z","receivedAt":"2014-12-03T19:31:40Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Dec 2, 2014 at 7:12 AM, Yi EungJun wrote:\n> From: Yi EungJun \n>\n> Add an Accept-Language header which indicates the user's preferred\n> languages defined by $LANGUAGE, $LC_ALL, $LC_MESSAGES and $LANG.\n>\n> Examples:\n> LANGUAGE= -> \"\"\n> LANGUAGE=ko:en -> \"Accept-Language: ko, en;q=0.9, *;q=0.1\"\n> LANGUAGE=ko LANG=en_US.UTF-8 -> \"Accept-Language: ko, *;q=0.1\"\n> LANGUAGE= LANG=en_US.UTF-8 -> \"Accept-Language: en-US, *;q=0.1\"\n>\n> This gives git servers a chance to display remote error messages in\n> the user's preferred language.\n>\n> Limit the number of languages to 1,000 because q-value must not be\n> smaller than 0.001, and limit the length of Accept-Language header to\n> 4,000 bytes for some HTTP servers which cannot accept such long header.\n>\n> Signed-off-by: Yi EungJun \n> ---\n> diff --git a/http.c b/http.c\n> index 040f362..69624af 100644\n> --- a/http.c\n> +++ b/http.c\n> @@ -68,6 +68,8 @@ static struct curl_slist *no_pragma_header;\n>\n> static struct active_request_slot *active_queue_head;\n>\n> +static struct strbuf *cached_accept_language;\n\nIs there a reason this needs to be a pointer to a strbuf rather than\njust a strbuf? That is wouldn't this work?\n\n static struct strbuf cached_accept_language = STRBUF_INIT;\n\n> size_t fread_buffer(char *ptr, size_t eltsize, size_t nmemb, void *buffer_)\n> {\n> size_t size = eltsize * nmemb;\n> @@ -515,6 +517,9 @@ void http_cleanup(void)\n> cert_auth.password = NULL;\n> }\n> ssl_cert_password_required = 0;\n> +\n> + if (cached_accept_language)\n> + strbuf_release(cached_accept_language);\n\nJunio already mentioned that this is leaking the memory of the strbuf\nstruct itself which was xmalloc()'d by get_accept_language().\n\n> }\n>\n> struct active_request_slot *get_active_slot(void)\n> @@ -986,6 +991,149 @@ static void extract_content_type(struct strbuf *raw, struct strbuf *type,\n> strbuf_addstr(charset, \"ISO-8859-1\");\n> }\n>\n> +/*\n> + * Guess the user's preferred languages from the value in LANGUAGE environment\n> + * variable and LC_MESSAGES locale category.\n> + *\n> + * The result can be a colon-separated list like \"ko:ja:en\".\n> + */\n> +static const char *get_preferred_languages(void)\n> +{\n> + const char *retval;\n> +\n> + retval = getenv(\"LANGUAGE\");\n> + if (retval && *retval)\n> + return retval;\n> +\n> + retval = setlocale(LC_MESSAGES, NULL);\n> + if (retval && *retval &&\n> + strcmp(retval, \"C\") &&\n> + strcmp(retval, \"POSIX\"))\n> + return retval;\n> +\n> + return NULL;\n\nMental note: This function will never return an empty string \"\", even\nif LANGUAGE or LC_MESSAGES is empty.\n\n> +}\n> +\n> +/*\n> + * Get an Accept-Language header which indicates user's preferred languages.\n> + *\n> + * Examples:\n> + * LANGUAGE= -> \"\"\n> + * LANGUAGE=ko:en -> \"Accept-Language: ko, en; q=0.9, *; q=0.1\"\n> + * LANGUAGE=ko_KR.UTF-8:sr@latin -> \"Accept-Language: ko-KR, sr; q=0.9, *; q=0.1\"\n> + * LANGUAGE=ko LANG=en_US.UTF-8 -> \"Accept-Language: ko, *; q=0.1\"\n> + * LANGUAGE= LANG=en_US.UTF-8 -> \"Accept-Language: en-US, *; q=0.1\"\n> + * LANGUAGE= LANG=C -> \"\"\n> + */\n> +static struct strbuf *get_accept_language(void)\n\nI find this API a bit strange. Use of strbuf to construct the returned\nstring is an implementation detail of this function. From the caller's\npoint of view, it should just be receiving a constant string: one\nwhich it needs neither to modify nor free. Also, if the caller were to\nmodify the returned strbuf for some reason, then that modification\nwould impact all future calls to get_accept_language() since the\nstrbuf is 'static' and not recomputed. Instead, I would expect the\ndeclaration to be:\n\n static const char *get_accept_language(void)\n\n> +{\n> + const char *lang_begin, *pos;\n> + int q, max_q;\n> + int num_langs;\n> + int decimal_places;\n> + int is_codeset_or_modifier = 0;\n> + char q_format[32];\n> + const int MAX_LANGS = 1000;\n> + const int MAX_SIZE_OF_HEADER = 4000;\n> + int last_size = 0;\n> +\n> + if (cached_accept_language)\n> + return cached_accept_language;\n> +\n> + cached_accept_language = xmalloc(sizeof(struct strbuf));\n> + strbuf_init(cached_accept_language, 0);\n> + lang_begin = get_preferred_languages();\n\nMental note: lang_begin will never be the empty string \"\".\n\n> + /* Don't add Accept-Language header if no language is preferred. */\n> + if (!lang_begin)\n> + return cached_accept_language;\n> +\n> + /* Count number of preferred lang_begin to decide precision of q-factor. */\n> + for (num_langs = 1, pos = lang_begin; *pos; pos++)\n> + if (*pos == ':')\n> + num_langs++;\n> +\n> + /* Decide the precision for q-factor on number of preferred lang_begin. */\n> + num_langs += 1; /* for '*' */\n> +\n> + if (MAX_LANGS < num_langs)\n> + num_langs = MAX_LANGS;\n> +\n> + for (max_q = 1, decimal_places = 0;\n> + max_q < num_langs;\n> + decimal_places++, max_q *= 10);\n> +\n> + sprintf(q_format, \";q=0.%%0%dd\", decimal_places);\n> +\n> + q = max_q;\n> +\n> + strbuf_addstr(cached_accept_language, \"Accept-Language: \");\n> +\n> + for (pos = lang_begin; ; pos++) {\n> + if (*pos == ':' || !*pos) {\n> + /* Ignore if this character is the first one. */\n> + if (pos == lang_begin)\n> + continue;\n\nIf lang_begin were ever to point at an empty string \"\", then this\nlogic would access memory beyond the end-of-string. Since\nget_preferred_languages() won't return an empty string, this case is\nsafe, but it's not obvious to the casual reader and some person\nmodifying the code in the future might not notice this restriction. I\npersonally would feel more comfortable if the no-empty-string\nassumption was documented formally with an assert(*lang_begin) or\ndie() before entering the loop; or if the code was rewritten to be\nless fragile.\n\n> + is_codeset_or_modifier = 0;\n> +\n> + /* Put a q-factor only if it is less than 1.0. */\n> + if (q < max_q)\n> + strbuf_addf(cached_accept_language, q_format, q);\n> +\n> + if (q > 1)\n> + q--;\n> +\n> + last_size = cached_accept_language->len;\n> +\n> + /* NULL pos means this is the last language. */\n\nNit: This feels somewhat backward. It sounds like the comment is\nexplaining the 'then' part of the 'if' rather than the 'else' part.\nPerhaps rephrase like this:\n\n /* non-NULL pos means more languages */\n\nor, better yet, just drop the comment since the code is self-explanatory.\n\n> + if (*pos)\n> + strbuf_addstr(cached_accept_language, \", \");\n> + else\n> + break;\n> +\n> + } else if (is_codeset_or_modifier)\n> + continue;\n> + else if (*pos == '.' || *pos == '@') /* Remove .codeset and @modifier. */\n> + is_codeset_or_modifier = 1;\n> + else\n> + strbuf_addch(cached_accept_language, *pos == '_' ? '-' : *pos);\n> +\n> + if (cached_accept_language->len > MAX_SIZE_OF_HEADER) {\n\nMental note: Here you respect MAX_SIZE_OF_HEADER.\n\n> + strbuf_remove(cached_accept_language, last_size,\n> + cached_accept_language->len - last_size);\n> + break;\n> + }\n> + }\n> +\n> + /* Don't add Accept-Language header if no language is preferred. */\n\nIs this comment correct? The \"Accept-Language:\" header was already\nadded to cached_accept_language much earlier in the function.\n\n> + if (q >= max_q) {\n> + return cached_accept_language;\n> + }\n\nStyle: Unnecessary braces.\n\n> + /* Add '*' with minimum q-factor greater than 0.0. */\n> + strbuf_addstr(cached_accept_language, \", *\");\n> + strbuf_addf(cached_accept_language, q_format, 1);\n\nHere you don't respect MAX_SIZE_OF_HEADER.\n\n> +\n> + return cached_accept_language;\n> +}\n> +\n> /* http_request() targets */\n> #define HTTP_REQUEST_STRBUF 0\n> #define HTTP_REQUEST_FILE 1\n> @@ -998,6 +1146,7 @@ static int http_request(const char *url,\n> struct slot_results results;\n> struct curl_slist *headers = NULL;\n> struct strbuf buf = STRBUF_INIT;\n> + struct strbuf *accept_language;\n> int ret;\n>\n> slot = get_active_slot();\n> @@ -1023,6 +1172,11 @@ static int http_request(const char *url,\n> fwrite_buffer);\n> }\n>\n> + accept_language = get_accept_language();\n> +\n> + if (accept_language && accept_language->len > 0)\n> + headers = curl_slist_append(headers, accept_language->buf);\n> +\n> strbuf_addstr(&buf, \"Pragma:\");\n> if (options && options->no_cache)\n> strbuf_addstr(&buf, \" no-cache\");\n"},{"id":"252989","messageId":"xmqqppc0wh33.fsf@gitster.dls.corp.google.com","threadId":"37174","inReplyTo":"CAPig+cTsULQPxoaSQ-ZvjWJ9Rgpdf3zG7ObPg4TnxFbXT9TwnA@mail.gmail.com","subject":"Re: [PATCH v5 1/1] http: Add Accept-Language header if possible","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-12-03T21:37:04Z","receivedAt":"2014-12-03T21:37:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n>> @@ -515,6 +517,9 @@ void http_cleanup(void)\n>> cert_auth.password = NULL;\n>> }\n>> ssl_cert_password_required = 0;\n>> +\n>> + if (cached_accept_language)\n>> + strbuf_release(cached_accept_language);\n>\n> Junio already mentioned that this is leaking the memory of the strbuf\n> struct itself which was xmalloc()'d by get_accept_language().\n\nI actually didn't ;-) A singleton cached_accept_language strbuf\nitself being kept around, with its reuse by get_accept_language(),\nis fine and is not a leak. But clearing the strbuf alone will\nintroduce correctness problem---the second HTTP connection will see\nan empty strbuf, get_accept_language() will say \"we've already\ncomputed and the header we must issue is an empty string\", which is\nnot correct.\n\nIn the fix-up \"SQUASH???\" commit I queued on top of this patch on\n'pu', I had to run \"sort -u\" on the output to the standard error\nstream, as there seemed to be two HTTP connections and the actual\noutput had two headers, even though the test expected only one in\nthe output. I suspect that it is a fallout from this bug that the\noriginal code passed that test that expects only one.\n\n>> +static struct strbuf *get_accept_language(void)\n>\n> I find this API a bit strange. Use of strbuf to construct the returned\n> string is an implementation detail of this function. From the caller's\n> point of view, it should just be receiving a constant string: one\n> which it needs neither to modify nor free. Also, if the caller were to\n> modify the returned strbuf for some reason, then that modification\n> would impact all future calls to get_accept_language() since the\n> strbuf is 'static' and not recomputed. Instead, I would expect the\n> declaration to be:\n>\n> static const char *get_accept_language(void)\n\nMakes sense to me.\n\n>> + /* Put a q-factor only if it is less than 1.0. */\n>> + if (q < max_q)\n>> + strbuf_addf(cached_accept_language, q_format, q);\n>> +\n>> + if (q > 1)\n>> + q--;\n\nI didn't mention this but if q ever goes below 1, wouldn't it mean\nthat there is no point continuing this loop?\n"},{"id":"252995","messageId":"CAO2U3QgDzDTt-zujw1yk51HFdp4oACusXeZ59h-CUgU41vgDHw@mail.gmail.com","threadId":"37174","inReplyTo":"xmqqppc0wh33.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v5 1/1] http: Add Accept-Language header if possible","fromName":"Michael Blume","fromEmail":"blume.mike@gmail.com","sentAt":"2014-12-03T22:00:17Z","receivedAt":"2014-12-03T22:00:17Z","isPatch":true,"sender":{"key":"blume.mike@gmail.com","avatar":"https://gravatar.com/avatar/1a7b440e1d942425ff4098ac7fc15b86b30cecaa56e1692a7ef8b5939ba25ea7?d=mp&s=160"},"body":"On Wed, Dec 3, 2014 at 1:37 PM, Junio C Hamano wrote:\n> Eric Sunshine writes:\n>\n>>> @@ -515,6 +517,9 @@ void http_cleanup(void)\n>>> cert_auth.password = NULL;\n>>> }\n>>> ssl_cert_password_required = 0;\n>>> +\n>>> + if (cached_accept_language)\n>>> + strbuf_release(cached_accept_language);\n>>\n>> Junio already mentioned that this is leaking the memory of the strbuf\n>> struct itself which was xmalloc()'d by get_accept_language().\n>\n> I actually didn't ;-) A singleton cached_accept_language strbuf\n> itself being kept around, with its reuse by get_accept_language(),\n> is fine and is not a leak. But clearing the strbuf alone will\n> introduce correctness problem---the second HTTP connection will see\n> an empty strbuf, get_accept_language() will say \"we've already\n> computed and the header we must issue is an empty string\", which is\n> not correct.\n>\n> In the fix-up \"SQUASH???\" commit I queued on top of this patch on\n> 'pu', I had to run \"sort -u\" on the output to the standard error\n> stream, as there seemed to be two HTTP connections and the actual\n> output had two headers, even though the test expected only one in\n> the output. I suspect that it is a fallout from this bug that the\n> original code passed that test that expects only one.\n>\n>>> +static struct strbuf *get_accept_language(void)\n>>\n>> I find this API a bit strange. Use of strbuf to construct the returned\n>> string is an implementation detail of this function. From the caller's\n>> point of view, it should just be receiving a constant string: one\n>> which it needs neither to modify nor free. Also, if the caller were to\n>> modify the returned strbuf for some reason, then that modification\n>> would impact all future calls to get_accept_language() since the\n>> strbuf is 'static' and not recomputed. Instead, I would expect the\n>> declaration to be:\n>>\n>> static const char *get_accept_language(void)\n>\n> Makes sense to me.\n>\n>>> + /* Put a q-factor only if it is less than 1.0. */\n>>> + if (q < max_q)\n>>> + strbuf_addf(cached_accept_language, q_format, q);\n>>> +\n>>> + if (q > 1)\n>>> + q--;\n>\n> I didn't mention this but if q ever goes below 1, wouldn't it mean\n> that there is no point continuing this loop?\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at http://vger.kernel.org/majordomo-info.html\nThis seems to be failing under Mac OS for me\n\nnot ok 25 - git client sends Accept-Language based on LANGUAGE,\nLC_ALL, LC_MESSAGES and LANG\n#\n# check_language \"ko-KR, *;q=0.1\" ko_KR.UTF-8 de_DE.UTF-8 ja_JP.UTF-8\nen_US.UTF-8 &&\n# check_language \"de-DE, *;q=0.1\" \"\" de_DE.UTF-8 ja_JP.UTF-8\nen_US.UTF-8 &&\n# check_language \"ja-JP, *;q=0.1\" \"\" \"\" ja_JP.UTF-8\nen_US.UTF-8 &&\n# check_language \"en-US, *;q=0.1\" \"\" \"\" \"\"\nen_US.UTF-8\n#\n"},{"id":"252996","messageId":"CAO2U3QjG2rUgUrM5odX0UOnHsENnYTfwaRLhHv8gka7qj4XWdw@mail.gmail.com","threadId":"37174","inReplyTo":"CAO2U3QgDzDTt-zujw1yk51HFdp4oACusXeZ59h-CUgU41vgDHw@mail.gmail.com","subject":"Re: [PATCH v5 1/1] http: Add Accept-Language header if possible","fromName":"Michael Blume","fromEmail":"blume.mike@gmail.com","sentAt":"2014-12-03T22:06:56Z","receivedAt":"2014-12-03T22:06:56Z","isPatch":true,"sender":{"key":"blume.mike@gmail.com","avatar":"https://gravatar.com/avatar/1a7b440e1d942425ff4098ac7fc15b86b30cecaa56e1692a7ef8b5939ba25ea7?d=mp&s=160"},"body":"On Wed, Dec 3, 2014 at 2:00 PM, Michael Blume wrote:\n> On Wed, Dec 3, 2014 at 1:37 PM, Junio C Hamano wrote:\n>> Eric Sunshine writes:\n>>\n>>>> @@ -515,6 +517,9 @@ void http_cleanup(void)\n>>>> cert_auth.password = NULL;\n>>>> }\n>>>> ssl_cert_password_required = 0;\n>>>> +\n>>>> + if (cached_accept_language)\n>>>> + strbuf_release(cached_accept_language);\n>>>\n>>> Junio already mentioned that this is leaking the memory of the strbuf\n>>> struct itself which was xmalloc()'d by get_accept_language().\n>>\n>> I actually didn't ;-) A singleton cached_accept_language strbuf\n>> itself being kept around, with its reuse by get_accept_language(),\n>> is fine and is not a leak. But clearing the strbuf alone will\n>> introduce correctness problem---the second HTTP connection will see\n>> an empty strbuf, get_accept_language() will say \"we've already\n>> computed and the header we must issue is an empty string\", which is\n>> not correct.\n>>\n>> In the fix-up \"SQUASH???\" commit I queued on top of this patch on\n>> 'pu', I had to run \"sort -u\" on the output to the standard error\n>> stream, as there seemed to be two HTTP connections and the actual\n>> output had two headers, even though the test expected only one in\n>> the output. I suspect that it is a fallout from this bug that the\n>> original code passed that test that expects only one.\n>>\n>>>> +static struct strbuf *get_accept_language(void)\n>>>\n>>> I find this API a bit strange. Use of strbuf to construct the returned\n>>> string is an implementation detail of this function. From the caller's\n>>> point of view, it should just be receiving a constant string: one\n>>> which it needs neither to modify nor free. Also, if the caller were to\n>>> modify the returned strbuf for some reason, then that modification\n>>> would impact all future calls to get_accept_language() since the\n>>> strbuf is 'static' and not recomputed. Instead, I would expect the\n>>> declaration to be:\n>>>\n>>> static const char *get_accept_language(void)\n>>\n>> Makes sense to me.\n>>\n>>>> + /* Put a q-factor only if it is less than 1.0. */\n>>>> + if (q < max_q)\n>>>> + strbuf_addf(cached_accept_language, q_format, q);\n>>>> +\n>>>> + if (q > 1)\n>>>> + q--;\n>>\n>> I didn't mention this but if q ever goes below 1, wouldn't it mean\n>> that there is no point continuing this loop?\n>> --\n>> To unsubscribe from this list: send the line \"unsubscribe git\" in\n>> the body of a message to majordomo@vger.kernel.org\n>> More majordomo info at http://vger.kernel.org/majordomo-info.html\n> This seems to be failing under Mac OS for me\n>\n> not ok 25 - git client sends Accept-Language based on LANGUAGE,\n> LC_ALL, LC_MESSAGES and LANG\n> #\n> # check_language \"ko-KR, *;q=0.1\" ko_KR.UTF-8 de_DE.UTF-8 ja_JP.UTF-8\n> en_US.UTF-8 &&\n> # check_language \"de-DE, *;q=0.1\" \"\" de_DE.UTF-8 ja_JP.UTF-8\n> en_US.UTF-8 &&\n> # check_language \"ja-JP, *;q=0.1\" \"\" \"\" ja_JP.UTF-8\n> en_US.UTF-8 &&\n> # check_language \"en-US, *;q=0.1\" \"\" \"\" \"\"\n> en_US.UTF-8\n> #\n\n\nverbose results\n\nInitialized empty Git repository in\n/Users/Shared/Jenkins/Home/jobs/git/workspace/t/trash\ndirectory.t5550-http-fetch-dumb/.git/\nexpecting success:\ngit config push.default matching &&\necho content1 >file &&\ngit add file &&\ngit commit -m one\necho content2 >file &&\ngit add file &&\ngit commit -m two\n\n[master (root-commit) f5983e9] one\n Author: A U Thor \n 1 file changed, 1 insertion(+)\n create mode 100644 file\n[master 2ff8a06] two\n Author: A U Thor \n 1 file changed, 1 insertion(+), 1 deletion(-)\nok 1 - setup repository\n\nexpecting success:\ncp -R .git \"$HTTPD_DOCUMENT_ROOT_PATH/repo.git\" &&\n(cd \"$HTTPD_DOCUMENT_ROOT_PATH/repo.git\" &&\ngit config core.bare true &&\nmkdir -p hooks &&\necho \"exec git update-server-info\" >hooks/post-update &&\nchmod +x hooks/post-update &&\nhooks/post-update\n) &&\ngit remote add public \"$HTTPD_DOCUMENT_ROOT_PATH/repo.git\" &&\ngit push public master:master\n\nEverything up-to-date\nok 2 - create http-accessible bare repository with loose objects\n\nexpecting success:\ngit clone $HTTPD_URL/dumb/repo.git clone-tmpl &&\ncp -R clone-tmpl clone &&\ntest_cmp file clone/file\n\nCloning into 'clone-tmpl'...\nok 3 - clone http repository\n\nexpecting success:\nmkdir -p \"$HTTPD_DOCUMENT_ROOT_PATH/auth/dumb/\" &&\ncp -Rf \"$HTTPD_DOCUMENT_ROOT_PATH/repo.git\" \\\n \"$HTTPD_DOCUMENT_ROOT_PATH/auth/dumb/repo.git\"\n\nok 4 - create password-protected repository\n\nexpecting success:\nwrite_script \"$TRASH_DIRECTORY/askpass\" <<-\\EOF &&\necho >>\"$TRASH_DIRECTORY/askpass-query\" \"askpass: $*\" &&\ncase \"$*\" in\n*Username*)\nwhat=user\n;;\n*Password*)\nwhat=pass\n;;\nesac &&\ncat \"$TRASH_DIRECTORY/askpass-$what\"\nEOF\nGIT_ASKPASS=\"$TRASH_DIRECTORY/askpass\" &&\nexport GIT_ASKPASS &&\nexport TRASH_DIRECTORY\n\nok 5 - setup askpass helper\n\nexpecting success:\nset_askpass wrong &&\ntest_must_fail git clone \"$HTTPD_URL/auth/dumb/repo.git\" clone-auth-fail &&\nexpect_askpass both wrong\n\nCloning into 'clone-auth-fail'...\nfatal: Authentication failed for 'http://127.0.0.1:5550/auth/dumb/repo.git/'\nok 6 - cloning password-protected repository can fail\n\nexpecting success:\nset_askpass wrong &&\ngit clone \"$HTTPD_URL_USER_PASS/auth/dumb/repo.git\" clone-auth-none &&\nexpect_askpass none\n\nCloning into 'clone-auth-none'...\nok 7 - http auth can use user/pass in URL\n\nexpecting success:\nset_askpass wrong pass@host &&\ngit clone \"$HTTPD_URL_USER/auth/dumb/repo.git\" clone-auth-pass &&\nexpect_askpass pass user@host\n\nCloning into 'clone-auth-pass'...\nok 8 - http auth can use just user in URL\n\nexpecting success:\nset_askpass user@host pass@host &&\ngit clone \"$HTTPD_URL/auth/dumb/repo.git\" clone-auth-both &&\nexpect_askpass both user@host\n\nCloning into 'clone-auth-both'...\nok 9 - http auth can request both user and pass\n\nexpecting success:\ntest_config_global credential.helper \"!f() {\ncat >/dev/null\necho username=user@host\necho password=pass@host\n}; f\" &&\nset_askpass wrong &&\ngit clone \"$HTTPD_URL/auth/dumb/repo.git\" clone-auth-helper &&\nexpect_askpass none\n\nCloning into 'clone-auth-helper'...\nok 10 - http auth respects credential helper config\n\nexpecting success:\ntest_config_global \"credential.$HTTPD_URL.username\" user@host &&\nset_askpass wrong pass@host &&\ngit clone \"$HTTPD_URL/auth/dumb/repo.git\" clone-auth-user &&\nexpect_askpass pass user@host\n\nCloning into 'clone-auth-user'...\nok 11 - http auth can get username from config\n\nexpecting success:\ntest_config_global \"credential.$HTTPD_URL.username\" wrong &&\nset_askpass wrong pass@host &&\ngit clone \"$HTTPD_URL_USER/auth/dumb/repo.git\" clone-auth-user2 &&\nexpect_askpass pass user@host\n\nCloning into 'clone-auth-user2'...\nok 12 - configured username does not override URL\n\nexpecting success:\necho content >>file &&\ngit commit -a -m two &&\ngit push public &&\n(cd clone && git pull) &&\ntest_cmp file clone/file\n\n[master d4af499] two\n Author: A U Thor \n 1 file changed, 1 insertion(+)\nTo /Users/Shared/Jenkins/Home/jobs/git/workspace/t/trash\ndirectory.t5550-http-fetch-dumb/httpd/www/repo.git\n 2ff8a06..d4af499 master -> master\nFrom http://127.0.0.1:5550/dumb/repo\n 2ff8a06..d4af499 master -> origin/master\nUpdating 2ff8a06..d4af499\nFast-forward\n file | 1 +\n 1 file changed, 1 insertion(+)\nok 13 - fetch changes via http\n\nexpecting success:\ncp -R clone-tmpl clone2 &&\n\nHEAD=$(git rev-parse --verify HEAD) &&\n(cd clone2 &&\ngit http-fetch -a -w heads/master-new $HEAD $(git config remote.origin.url) &&\ngit checkout master-new &&\ntest $HEAD = $(git rev-parse --verify HEAD)) &&\ntest_cmp file clone2/file\n\nSwitched to branch 'master-new'\nok 14 - fetch changes via manual http-fetch\n\nexpecting success:\ngit push public master:other &&\n(cd clone &&\ngit remote set-head origin -d &&\ngit remote set-head origin -a &&\ngit symbolic-ref refs/remotes/origin/HEAD > output &&\necho refs/remotes/origin/master > expect &&\ntest_cmp expect output\n)\n\nTo /Users/Shared/Jenkins/Home/jobs/git/workspace/t/trash\ndirectory.t5550-http-fetch-dumb/httpd/www/repo.git\n * [new branch] master -> other\norigin/HEAD set to master\nok 15 - http remote detects correct HEAD\n\nexpecting success:\ncp -R \"$HTTPD_DOCUMENT_ROOT_PATH\"/repo.git\n\"$HTTPD_DOCUMENT_ROOT_PATH\"/repo_pack.git &&\n(cd \"$HTTPD_DOCUMENT_ROOT_PATH\"/repo_pack.git &&\ngit --bare repack -a -d\n) &&\ngit clone $HTTPD_URL/dumb/repo_pack.git\n\nCloning into 'repo_pack'...\nok 16 - fetch packed objects\n\nexpecting success:\ncp -R \"$HTTPD_DOCUMENT_ROOT_PATH\"/repo_pack.git\n\"$HTTPD_DOCUMENT_ROOT_PATH\"/repo_bad1.git &&\n(cd \"$HTTPD_DOCUMENT_ROOT_PATH\"/repo_bad1.git &&\np=`ls objects/pack/pack-*.pack` &&\nchmod u+w $p &&\nprintf %0256d 0 | dd of=$p bs=256 count=1 seek=1 conv=notrunc\n) &&\nmkdir repo_bad1.git &&\n(cd repo_bad1.git &&\ngit --bare init &&\ntest_must_fail git --bare fetch $HTTPD_URL/dumb/repo_bad1.git &&\ntest 0 = `ls objects/pack/pack-*.pack | wc -l`\n)\n\n1+0 records in\n1+0 records out\n256 bytes transferred in 0.000014 secs (18512790 bytes/sec)\nInitialized empty Git repository in\n/Users/Shared/Jenkins/Home/jobs/git/workspace/t/trash\ndirectory.t5550-http-fetch-dumb/repo_bad1.git/\nfatal: pack has bad object at offset 168: inflate returned -5\nerror: Unable to find d4af499a00b28bc0ab78fa94cc6a449fae19b08d under\nhttp://127.0.0.1:5550/dumb/repo_bad1.git\nCannot obtain needed object d4af499a00b28bc0ab78fa94cc6a449fae19b08d\nerror: fetch failed.\nls: objects/pack/pack-*.pack: No such file or directory\nok 17 - fetch notices corrupt pack\n\nexpecting success:\ncp -R \"$HTTPD_DOCUMENT_ROOT_PATH\"/repo_pack.git\n\"$HTTPD_DOCUMENT_ROOT_PATH\"/repo_bad2.git &&\n(cd \"$HTTPD_DOCUMENT_ROOT_PATH\"/repo_bad2.git &&\np=`ls objects/pack/pack-*.idx` &&\nchmod u+w $p &&\nprintf %0256d 0 | dd of=$p bs=256 count=1 seek=1 conv=notrunc\n) &&\nmkdir repo_bad2.git &&\n(cd repo_bad2.git &&\ngit --bare init &&\ntest_must_fail git --bare fetch $HTTPD_URL/dumb/repo_bad2.git &&\ntest 0 = `ls objects/pack | wc -l`\n)\n\n1+0 records in\n1+0 records out\n256 bytes transferred in 0.000013 secs (19522579 bytes/sec)\nInitialized empty Git repository in\n/Users/Shared/Jenkins/Home/jobs/git/workspace/t/trash\ndirectory.t5550-http-fetch-dumb/repo_bad2.git/\nerror: non-monotonic index\n/Users/Shared/Jenkins/Home/jobs/git/workspace/t/trash\ndirectory.t5550-http-fetch-dumb/repo_bad2.git/objects/pack/pack-7244949d8d6e59a30923b3fff7801f159ef4ba5d.idx.temp\nerror: Unable to find d4af499a00b28bc0ab78fa94cc6a449fae19b08d under\nhttp://127.0.0.1:5550/dumb/repo_bad2.git\nCannot obtain needed object d4af499a00b28bc0ab78fa94cc6a449fae19b08d\nerror: fetch failed.\nok 18 - fetch notices corrupt idx\n\nexpecting success:\ngrep /git-upload-pack <\"$HTTPD_ROOT_PATH\"/access.log >act\n: >exp\ntest_cmp exp act\n\nok 19 - did not use upload-pack service\n\nexpecting success:\ntest_must_fail git clone \"$HTTPD_URL/error/text\" 2>stderr &&\ngrep \"this is the error message\" stderr\n\nremote: this is the error message\nok 20 - git client shows text/plain errors\n\nexpecting success:\ntest_must_fail git clone \"$HTTPD_URL/error/html\" 2>stderr &&\n! grep \"this is the error message\" stderr\n\nok 21 - git client does not show html errors\n\nexpecting success:\ntest_must_fail git clone \"$HTTPD_URL/error/charset\" 2>stderr &&\ngrep \"this is the error message\" stderr\n\nremote: this is the error message\nok 22 - git client shows text/plain with a charset\n\nexpecting success:\ntest_must_fail git clone \"$HTTPD_URL/error/utf16\" 2>stderr &&\ngrep \"this is the error message\" stderr\n\nremote: this is the error message\nok 23 - http error messages are reencoded\n\nexpecting success:\ntest_must_fail git clone \"$HTTPD_URL/error/odd-spacing\" 2>stderr &&\ngrep \"this is the error message\" stderr\n\nremote: this is the error message\nok 24 - reencoding is robust to whitespace oddities\n\nexpecting success:\ncheck_language \"ko-KR, *;q=0.1\" ko_KR.UTF-8 de_DE.UTF-8 ja_JP.UTF-8\nen_US.UTF-8 &&\ncheck_language \"de-DE, *;q=0.1\" \"\" de_DE.UTF-8 ja_JP.UTF-8\nen_US.UTF-8 &&\ncheck_language \"ja-JP, *;q=0.1\" \"\" \"\" ja_JP.UTF-8\nen_US.UTF-8 &&\ncheck_language \"en-US, *;q=0.1\" \"\" \"\" \"\" en_US.UTF-8\n\nnot ok 25 - git client sends Accept-Language based on LANGUAGE,\nLC_ALL, LC_MESSAGES and LANG\n#\n# check_language \"ko-KR, *;q=0.1\" ko_KR.UTF-8 de_DE.UTF-8 ja_JP.UTF-8\nen_US.UTF-8 &&\n# check_language \"de-DE, *;q=0.1\" \"\" de_DE.UTF-8 ja_JP.UTF-8\nen_US.UTF-8 &&\n# check_language \"ja-JP, *;q=0.1\" \"\" \"\" ja_JP.UTF-8\nen_US.UTF-8 &&\n# check_language \"en-US, *;q=0.1\" \"\" \"\" \"\"\nen_US.UTF-8\n#\n\nexpecting success:\ncheck_language \"ko-KR, en-US;q=0.99, fr-CA;q=0.98, de;q=0.97, sr;q=0.96, \\\nja;q=0.95, zh;q=0.94, sv;q=0.93, pt;q=0.92, nb;q=0.91, *;q=0.01\" \\\nko_KR.EUC-KR:en_US.UTF-8:fr_CA:de.UTF-8@euro:sr@latin:ja:zh:sv:pt:nb\n\nok 26 - git client sends Accept-Language with many preferred languages\n\nexpecting success:\nGIT_CURL_VERBOSE=1 LANGUAGE= git ls-remote \"$HTTPD_URL/dumb/repo.git\"\n2>stderr &&\n! grep \"^Accept-Language:\" stderr\n\nd4af499a00b28bc0ab78fa94cc6a449fae19b08d HEAD\nd4af499a00b28bc0ab78fa94cc6a449fae19b08d refs/heads/master\nd4af499a00b28bc0ab78fa94cc6a449fae19b08d refs/heads/other\nok 27 - git client does not send an empty Accept-Language\n\n# failed 1 among 27 test(s)\n1..27\n"},{"id":"253923","messageId":"1419266658-1180-1-git-send-email-eungjun.yi@navercorp.com","threadId":"37174","inReplyTo":"CAO2U3QjG2rUgUrM5odX0UOnHsENnYTfwaRLhHv8gka7qj4XWdw@mail.gmail.com","subject":"[PATCH v6 0/1] http: Add Accept-Language header if possible","fromName":"Yi EungJun","fromEmail":"semtlenori@gmail.com","sentAt":"2014-12-22T16:44:17Z","receivedAt":"2014-12-22T16:44:17Z","isPatch":true,"sender":{"key":"semtlenori@gmail.com","avatar":"https://gravatar.com/avatar/8363435d2badb3450df0dd7c4ec2113f6e1d62c44dd3cf7dfe83c5a389b8a9bf?d=mp&s=160"},"body":"Changes since v5\n\n>From Junio C Hamano's review:\n\n* The tests use `ls-remote` instead of `clone` for tests; I copied the test\n code from ba8e63dc30a80656fddc616f714fb217ad220c04.\n\n* Set cached_accept_langauge to NULL after free it.\n\n>From Eric Sunshine's review:\n\n* get_accept_language() returns a pointer to const char instead of strbuf; the\n type of cached_accept_language also has been changed to char* from strbuf*\n\n* write_accept_language(), which is extracted from get_accept_language(),\n respects MAX_SIZE_OF_HEADER.\n\n* The for-loop in write_accept_language() works correctly if lang_begin points\n an empty string.\n\n>From Jeff King's advice:\n\n* get_preferred_languages() considers LC_MESSAGES only if NO_GETTEXT is not\n defined.\n\n* Remove the tests for LC_MESSAGES, LANG and LC_ALL.\n\nYi EungJun (1):\n http: Add Accept-Language header if possible\n\n http.c | 173 +++++++++++++++++++++++++++++++++++++++++++++\n remote-curl.c | 2 +\n t/t5550-http-fetch-dumb.sh | 32 +++++++++\n 3 files changed, 207 insertions(+)\n\n-- \n2.2.0.375.gcd18ce6.dirty\n"},{"id":"253924","messageId":"1419266658-1180-2-git-send-email-eungjun.yi@navercorp.com","threadId":"37174","inReplyTo":"1419266658-1180-1-git-send-email-eungjun.yi@navercorp.com","subject":"[PATCH v6 1/1] http: Add Accept-Language header if possible","fromName":"Yi EungJun","fromEmail":"semtlenori@gmail.com","sentAt":"2014-12-22T16:44:18Z","receivedAt":"2014-12-22T16:44:18Z","isPatch":true,"sender":{"key":"semtlenori@gmail.com","avatar":"https://gravatar.com/avatar/8363435d2badb3450df0dd7c4ec2113f6e1d62c44dd3cf7dfe83c5a389b8a9bf?d=mp&s=160"},"body":"From: Yi EungJun \n\nAdd an Accept-Language header which indicates the user's preferred\nlanguages defined by $LANGUAGE, $LC_ALL, $LC_MESSAGES and $LANG.\n\nExamples:\n LANGUAGE= -> \"\"\n LANGUAGE=ko:en -> \"Accept-Language: ko, en;q=0.9, *;q=0.1\"\n LANGUAGE=ko LANG=en_US.UTF-8 -> \"Accept-Language: ko, *;q=0.1\"\n LANGUAGE= LANG=en_US.UTF-8 -> \"Accept-Language: en-US, *;q=0.1\"\n\nThis gives git servers a chance to display remote error messages in\nthe user's preferred language.\n\nLimit the number of languages to 1,000 because q-value must not be\nsmaller than 0.001, and limit the length of Accept-Language header to\n4,000 bytes for some HTTP servers which cannot accept such long header.\n\nSigned-off-by: Yi EungJun \n---\n http.c | 173 +++++++++++++++++++++++++++++++++++++++++++++\n remote-curl.c | 2 +\n t/t5550-http-fetch-dumb.sh | 32 +++++++++\n 3 files changed, 207 insertions(+)\n\ndiff --git a/http.c b/http.c\nindex 040f362..7a77708 100644\n--- a/http.c\n+++ b/http.c\n@@ -68,6 +68,8 @@ static struct curl_slist *no_pragma_header;\n \n static struct active_request_slot *active_queue_head;\n \n+static char *cached_accept_language;\n+\n size_t fread_buffer(char *ptr, size_t eltsize, size_t nmemb, void *buffer_)\n {\n \tsize_t size = eltsize * nmemb;\n@@ -515,6 +517,11 @@ void http_cleanup(void)\n \t\tcert_auth.password = NULL;\n \t}\n \tssl_cert_password_required = 0;\n+\n+\tif (cached_accept_language) {\n+\t\tfree(cached_accept_language);\n+\t\tcached_accept_language = NULL;\n+\t}\n }\n \n struct active_request_slot *get_active_slot(void)\n@@ -986,6 +993,166 @@ static void extract_content_type(struct strbuf *raw, struct strbuf *type,\n \t\tstrbuf_addstr(charset, \"ISO-8859-1\");\n }\n \n+/*\n+ * Guess the user's preferred languages from the value in LANGUAGE environment\n+ * variable and LC_MESSAGES locale category if NO_GETTEXT is not defined.\n+ *\n+ * The result can be a colon-separated list like \"ko:ja:en\".\n+ */\n+static const char *get_preferred_languages(void)\n+{\n+\tconst char *retval;\n+\n+\tretval = getenv(\"LANGUAGE\");\n+\tif (retval && *retval)\n+\t\treturn retval;\n+\n+#ifndef NO_GETTEXT\n+\tretval = setlocale(LC_MESSAGES, NULL);\n+\tif (retval && *retval &&\n+\t\tstrcmp(retval, \"C\") &&\n+\t\tstrcmp(retval, \"POSIX\"))\n+\t\treturn retval;\n+#endif\n+\n+\treturn NULL;\n+}\n+\n+static void write_accept_language(struct strbuf *buf)\n+{\n+\tconst char *lang_begin, *pos;\n+\tint q, max_q;\n+\tint num_langs;\n+\tint decimal_places;\n+\tconst int CODESET_OR_MODIFIER = 1;\n+\tconst int LANGUAGE_TAG = 2;\n+\tconst int SEPARATOR = 3;\n+\tint is_q_factor_required = 0;\n+\tint parse_state = 0;\n+\tchar q_format[32];\n+\t/*\n+\t * MAX_LANGS must not be larger than 1,000. If it is larger than that,\n+\t * q-value will be smaller than 0.001, the minimum q-value the HTTP\n+\t * specification allows [1].\n+\t *\n+\t * [1]: http://tools.ietf.org/html/rfc7231#section-5.3.1\n+\t */\n+\tconst int MAX_LANGS = 1000;\n+\tconst int MAX_SIZE_OF_HEADER = 4000;\n+\tconst int MAX_SIZE_OF_ASTERISK_ELEMENT = 11; /* for \", *;q=0.001\" */\n+\tint last_size = 0;\n+\n+\tlang_begin = get_preferred_languages();\n+\n+\t/* Don't add Accept-Language header if no language is preferred. */\n+\tif (!lang_begin)\n+\t\treturn;\n+\n+\t/* Count number of preferred lang_begin to decide precision of q-factor. */\n+\tfor (num_langs = 1, pos = lang_begin; *pos; pos++)\n+\t\tif (*pos == ':')\n+\t\t\tnum_langs++;\n+\n+\t/* Decide the precision for q-factor on number of preferred lang_begin. */\n+\tnum_langs += 1; /* for '*' */\n+\n+\tif (MAX_LANGS < num_langs)\n+\t\tnum_langs = MAX_LANGS;\n+\n+\tfor (max_q = 1, decimal_places = 0;\n+\t\tmax_q < num_langs;\n+\t\tdecimal_places++, max_q *= 10)\n+\t\t;\n+\n+\tsprintf(q_format, \";q=0.%%0%dd\", decimal_places);\n+\n+\tq = max_q;\n+\n+\tstrbuf_addstr(buf, \"Accept-Language: \");\n+\n+\t/*\n+\t * Convert a list of colon-separated locale values [1][2] to a list of\n+\t * comma-separated language tags [3] which can be used as a value of\n+\t * Accept-Language header.\n+\t *\n+\t * [1]: http://pubs.opengroup.org/onlinepubs/007908799/xbd/envvar.html\n+\t * [2]: http://www.gnu.org/software/libc/manual/html_node/Using-gettextized-software.html\n+\t * [3]: http://tools.ietf.org/html/rfc7231#section-5.3.5\n+\t */\n+\tfor (pos = lang_begin; ; pos++) {\n+\t\tif (!*pos || *pos == ':') {\n+\t\t\tif (is_q_factor_required) {\n+\t\t\t\t/* Put a q-factor only if it is less than 1.0. */\n+\t\t\t\tif (q < max_q)\n+\t\t\t\t\tstrbuf_addf(buf, q_format, q);\n+\n+\t\t\t\tif (q > 1)\n+\t\t\t\t\tq--;\n+\n+\t\t\t\tlast_size = buf->len;\n+\n+\t\t\t\tis_q_factor_required = 0;\n+\t\t\t}\n+\t\t\tparse_state = SEPARATOR;\n+\t\t} else if (parse_state == CODESET_OR_MODIFIER)\n+\t\t\tcontinue;\n+\t\telse if (*pos == ' ') /* Ignore whitespace character */\n+\t\t\tcontinue;\n+\t\telse if (*pos == '.' || *pos == '@') /* Remove .codeset and @modifier. */\n+\t\t\tparse_state = CODESET_OR_MODIFIER;\n+\t\telse {\n+\t\t\tif (parse_state != LANGUAGE_TAG && q < max_q)\n+\t\t\t\tstrbuf_addstr(buf, \", \");\n+\t\t\tstrbuf_addch(buf, *pos == '_' ? '-' : *pos);\n+\t\t\tis_q_factor_required = 1;\n+\t\t\tparse_state = LANGUAGE_TAG;\n+\t\t}\n+\n+\t\tif (buf->len > MAX_SIZE_OF_HEADER - MAX_SIZE_OF_ASTERISK_ELEMENT) {\n+\t\t\tstrbuf_remove(buf, last_size, buf->len - last_size);\n+\t\t\tbreak;\n+\t\t}\n+\n+\t\tif (!*pos)\n+\t\t\tbreak;\n+\t}\n+\n+\t/* Don't add Accept-Language header if no language is preferred. */\n+\tif (q >= max_q) {\n+\t\tstrbuf_reset(buf);\n+\t\treturn;\n+\t}\n+\n+\t/* Add '*' with minimum q-factor greater than 0.0. */\n+\tstrbuf_addstr(buf, \", *\");\n+\tstrbuf_addf(buf, q_format, 1);\n+}\n+\n+/*\n+ * Get an Accept-Language header which indicates user's preferred languages.\n+ *\n+ * This function always return non-NULL string as strbuf_detach() does.\n+ *\n+ * Examples:\n+ * LANGUAGE= -> \"\"\n+ * LANGUAGE=ko:en -> \"Accept-Language: ko, en; q=0.9, *; q=0.1\"\n+ * LANGUAGE=ko_KR.UTF-8:sr@latin -> \"Accept-Language: ko-KR, sr; q=0.9, *; q=0.1\"\n+ * LANGUAGE=ko LANG=en_US.UTF-8 -> \"Accept-Language: ko, *; q=0.1\"\n+ * LANGUAGE= LANG=en_US.UTF-8 -> \"Accept-Language: en-US, *; q=0.1\"\n+ * LANGUAGE= LANG=C -> \"\"\n+ */\n+static const char *get_accept_language(void)\n+{\n+\tif (!cached_accept_language) {\n+\t\tstruct strbuf buf = STRBUF_INIT;\n+\t\twrite_accept_language(&buf);\n+\t\tcached_accept_language = strbuf_detach(&buf, NULL);\n+\t\tstrbuf_release(&buf);\n+\t}\n+\n+\treturn cached_accept_language;\n+}\n+\n /* http_request() targets */\n #define HTTP_REQUEST_STRBUF\t0\n #define HTTP_REQUEST_FILE\t1\n@@ -998,6 +1165,7 @@ static int http_request(const char *url,\n \tstruct slot_results results;\n \tstruct curl_slist *headers = NULL;\n \tstruct strbuf buf = STRBUF_INIT;\n+\tconst char *accept_language;\n \tint ret;\n \n \tslot = get_active_slot();\n@@ -1023,6 +1191,11 @@ static int http_request(const char *url,\n \t\t\t\t\t fwrite_buffer);\n \t}\n \n+\taccept_language = get_accept_language();\n+\n+\tif (strlen(accept_language) > 0)\n+\t\theaders = curl_slist_append(headers, accept_language);\n+\n \tstrbuf_addstr(&buf, \"Pragma:\");\n \tif (options && options->no_cache)\n \t\tstrbuf_addstr(&buf, \" no-cache\");\ndiff --git a/remote-curl.c b/remote-curl.c\nindex dd63bc2..04989e5 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -962,6 +962,8 @@ int main(int argc, const char **argv)\n \tstruct strbuf buf = STRBUF_INIT;\n \tint nongit;\n \n+\tgit_setup_gettext();\n+\n \tgit_extract_argv0_path(argv[0]);\n \tsetup_git_directory_gently(&nongit);\n \tif (argc < 2) {\ndiff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh\nindex ac71418..1a58b97 100755\n--- a/t/t5550-http-fetch-dumb.sh\n+++ b/t/t5550-http-fetch-dumb.sh\n@@ -196,5 +196,37 @@ test_expect_success 'reencoding is robust to whitespace oddities' '\n \tgrep \"this is the error message\" stderr\n '\n \n+check_language () {\n+\techo \"Accept-Language: $1\" >expect &&\n+\tGIT_CURL_VERBOSE=1 \\\n+\tLANGUAGE=$2 \\\n+\tgit ls-remote \"$HTTPD_URL/dumb/repo.git\" 2>&1 |\n+\ttr -d '\\015' |\n+\tsort -u >stderr &&\n+\tgrep -i ^Accept-Language: stderr >actual &&\n+\ttest_cmp expect actual\n+}\n+\n+test_expect_success 'git client sends Accept-Language based on LANGUAGE' '\n+\tcheck_language \"ko-KR, *;q=0.1\" ko_KR.UTF-8'\n+\n+test_expect_success 'git client sends Accept-Language correctly with unordinary LANGUAGE' '\n+\tcheck_language \"ko-KR, *;q=0.1\" \"ko_KR:\" &&\n+\tcheck_language \"ko-KR, *;q=0.1\" \" ko_KR\" &&\n+\tcheck_language \"ko-KR, en-US;q=0.9, *;q=0.1\" \"ko_KR: en_US\" &&\n+\tcheck_language \"ko-KR, en-US;q=0.9, *;q=0.1\" \"ko_KR::en_US\" &&\n+\tcheck_language \"ko-KR, *;q=0.1\" \":::ko_KR\"'\n+\n+test_expect_success 'git client sends Accept-Language with many preferred languages' '\n+\tcheck_language \"ko-KR, en-US;q=0.99, fr-CA;q=0.98, de;q=0.97, sr;q=0.96, \\\n+ja;q=0.95, zh;q=0.94, sv;q=0.93, pt;q=0.92, nb;q=0.91, *;q=0.01\" \\\n+\t\tko_KR.EUC-KR:en_US.UTF-8:fr_CA:de.UTF-8@euro:sr@latin:ja:zh:sv:pt:nb\n+'\n+\n+test_expect_success 'git client does not send an empty Accept-Language' '\n+\tGIT_CURL_VERBOSE=1 LANGUAGE= git ls-remote \"$HTTPD_URL/dumb/repo.git\" 2>stderr &&\n+\t! grep \"^Accept-Language:\" stderr\n+'\n+\n stop_httpd\n test_done\n-- \n2.2.0.375.gcd18ce6.dirty\n"},{"id":"253943","messageId":"xmqqwq5j8onm.fsf@gitster.dls.corp.google.com","threadId":"37174","inReplyTo":"1419266658-1180-2-git-send-email-eungjun.yi@navercorp.com","subject":"Re: [PATCH v6 1/1] http: Add Accept-Language header if possible","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-12-22T19:34:05Z","receivedAt":"2014-12-22T19:34:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Yi EungJun writes:\n\n> From: Yi EungJun \n>\n> Add an Accept-Language header which indicates the user's preferred\n> languages defined by $LANGUAGE, $LC_ALL, $LC_MESSAGES and $LANG.\n>\n> Examples:\n> LANGUAGE= -> \"\"\n> LANGUAGE=ko:en -> \"Accept-Language: ko, en;q=0.9, *;q=0.1\"\n> LANGUAGE=ko LANG=en_US.UTF-8 -> \"Accept-Language: ko, *;q=0.1\"\n> LANGUAGE= LANG=en_US.UTF-8 -> \"Accept-Language: en-US, *;q=0.1\"\n>\n> This gives git servers a chance to display remote error messages in\n> the user's preferred language.\n>\n> Limit the number of languages to 1,000 because q-value must not be\n> smaller than 0.001, and limit the length of Accept-Language header to\n> 4,000 bytes for some HTTP servers which cannot accept such long header.\n>\n> Signed-off-by: Yi EungJun \n\nOverall, this one is a much more pleasant read than the previous\nrounds.\n\n> @@ -986,6 +993,166 @@ static void extract_content_type(struct strbuf *raw, struct strbuf *type,\n> \t\tstrbuf_addstr(charset, \"ISO-8859-1\");\n> }\n> \n> +/*\n> + * Guess the user's preferred languages from the value in LANGUAGE environment\n> + * variable and LC_MESSAGES locale category if NO_GETTEXT is not defined.\n> + *\n> + * The result can be a colon-separated list like \"ko:ja:en\".\n> + */\n> +static const char *get_preferred_languages(void)\n> +{\n> +\tconst char *retval;\n> +\n> +\tretval = getenv(\"LANGUAGE\");\n> +\tif (retval && *retval)\n> +\t\treturn retval;\n> +\n> +#ifndef NO_GETTEXT\n> +\tretval = setlocale(LC_MESSAGES, NULL);\n> +\tif (retval && *retval &&\n> +\t\tstrcmp(retval, \"C\") &&\n> +\t\tstrcmp(retval, \"POSIX\"))\n> +\t\treturn retval;\n> +#endif\n\nA tangent.\n\nI wonder if we want to have something silly like this:\n\n\t#ifndef NO_GETTEXT\n #define setlocale(x, y) NULL /* or \"C\"??? */\n #endif\n\nin a common header (e.g. gettext.h) to avoid sprinkling #ifdefs in\nour code. While I do not think we call setlocale() that often to\nwarrant such a trick, we already do something very similar to make\ngit_setup_gettext() a no-op in NO_GETTEXT builds in that header\nfile, and the change in this patch to remote-curl.c does take\nadvantage of it already, so...\n\n> +static void write_accept_language(struct strbuf *buf)\n> +{\n> +\tconst char *lang_begin, *pos;\n> +\tint q, max_q;\n> +\tint num_langs;\n> +\tint decimal_places;\n> +\tconst int CODESET_OR_MODIFIER = 1;\n> +\tconst int LANGUAGE_TAG = 2;\n> +\tconst int SEPARATOR = 3;\n\nAnother tangent, but I think we tend to use either #define or enum\nfor constants, not \"const int\", in our codebase, for symbolic\nconstants. In order to define a set of symbolic constants limited\nto a function scope, the \"const int\" way may be nicer than the other\ntwo methods we have traditionally used. Perhaps we should promote\nsuch use more widely, write our new code following this example, and\nmigrate existing ones over time? I dunno.\n\n> ...\n> +\t/* Decide the precision for q-factor on number of preferred lang_begin. */\n> +\tnum_langs += 1; /* for '*' */\n> +\n> +\tif (MAX_LANGS < num_langs)\n> +\t\tnum_langs = MAX_LANGS;\n> +\n> +\tfor (max_q = 1, decimal_places = 0;\n> +\t\tmax_q < num_langs;\n> +\t\tdecimal_places++, max_q *= 10)\n> +\t\t;\n\nSo, if we have 10 languages (num_langs == 10), decimal_places\nbecomes 1, max_q becomes 10 ...\n\n> +\tsprintf(q_format, \";q=0.%%0%dd\", decimal_places);\n\n... and we will use \"q=0.%01d\" as the format. This is OK because\nthe first one is given without q= so we use the format only nine\ntimes, counting down from 0.9 to 0.1 in 0.1 increments.\n\nSounds OK to me (I always miscount this kind of loop and need to\nthink aloud while doing a sanity check).\n\n> +\tfor (pos = lang_begin; ; pos++) {\n> +\t\tif (!*pos || *pos == ':') {\n> +\t\t\tif (is_q_factor_required) {\n> +\t\t\t\t/* Put a q-factor only if it is less than 1.0. */\n> +\t\t\t\tif (q < max_q)\n> +\t\t\t\t\tstrbuf_addf(buf, q_format, q);\n> +\n> +\t\t\t\tif (q > 1)\n> +\t\t\t\t\tq--;\n\nWhen does this \"if\" statement not trigger? It seems to me that it\nwill stop decrementing only if you have very many languages (e.g.\nnum_langs was clipped to MAX_LANGS), and at that point you would not\nwant to scan and add more languages---is there a reason why you keep\ngoing in such a case and not break out of the loop, i.e.\n\n\tif (q-- < 1)\n\t\tbreak;\n\nor something like that?\n\n> + ...\n> +\t\tif (buf->len > MAX_SIZE_OF_HEADER - MAX_SIZE_OF_ASTERISK_ELEMENT) {\n> +\t\t\tstrbuf_remove(buf, last_size, buf->len - last_size);\n> +\t\t\tbreak;\n> +\t\t}\n> +\n> +\t\tif (!*pos)\n> +\t\t\tbreak;\n\nAlternatively use one of these breaks when q goes below 1, perhaps?\n\n> +/*\n> + * Get an Accept-Language header which indicates user's preferred languages.\n> + *\n> + * This function always return non-NULL string as strbuf_detach() does.\n> + *\n> + * Examples:\n> + * LANGUAGE= -> \"\"\n> + * LANGUAGE=ko:en -> \"Accept-Language: ko, en; q=0.9, *; q=0.1\"\n> + * LANGUAGE=ko_KR.UTF-8:sr@latin -> \"Accept-Language: ko-KR, sr; q=0.9, *; q=0.1\"\n> + * LANGUAGE=ko LANG=en_US.UTF-8 -> \"Accept-Language: ko, *; q=0.1\"\n> + * LANGUAGE= LANG=en_US.UTF-8 -> \"Accept-Language: en-US, *; q=0.1\"\n> + * LANGUAGE= LANG=C -> \"\"\n> + */\n> +static const char *get_accept_language(void)\n> +{\n> +\tif (!cached_accept_language) {\n> +\t\tstruct strbuf buf = STRBUF_INIT;\n> +\t\twrite_accept_language(&buf);\n> +\t\tcached_accept_language = strbuf_detach(&buf, NULL);\n> +\t\tstrbuf_release(&buf);\n\nIf you detached the associated string from the strbuf, you have\nalready released the resource from it; no need to release it, I\nwould think.\n\n> diff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh\n> index ac71418..1a58b97 100755\n> --- a/t/t5550-http-fetch-dumb.sh\n> +++ b/t/t5550-http-fetch-dumb.sh\n> @@ -196,5 +196,37 @@ test_expect_success 'reencoding is robust to whitespace oddities' '\n> \tgrep \"this is the error message\" stderr\n> '\n> \n> +check_language () {\n> +\techo \"Accept-Language: $1\" >expect &&\n> +\tGIT_CURL_VERBOSE=1 \\\n> +\tLANGUAGE=$2 \\\n> +\tgit ls-remote \"$HTTPD_URL/dumb/repo.git\" 2>&1 |\n> +\ttr -d '\\015' |\n> +\tsort -u >stderr &&\n> +\tgrep -i ^Accept-Language: stderr >actual &&\n> +\ttest_cmp expect actual\n> +}\n\nThis makes it hard to test a case where no Accept-Language: header\nshould be issued in the request, because at that point we would be\nexpecting no matching string in the output.\n\n\tcase \"$2\" in\n '')\n \t>expect\n ;;\n\t?*)\n \techo \"Accept-Language: $1\" >expect\n ;;\n\tesac &&\n\tgit ls-remote \"$HTTPD_URL/dumb/repo.git\" >output 2>&1 &&\n\ttr -d '\\015' actual &&\n test_cmp expect actual\n\nor something like that, perhaps?\n\nAnd I can see below that we are not testing that \"negative\" case.\n\nAfter writing a new shiny feature, it always is tempting to show off\nthat it triggers when it is expected to and gives an expected\nresult, but it is equally important to have tests that make sure\nthat the feature does not trigger when it should not.\n\n\n> +test_expect_success 'git client sends Accept-Language based on LANGUAGE' '\n> +\tcheck_language \"ko-KR, *;q=0.1\" ko_KR.UTF-8'\n> +\n> +test_expect_success 'git client sends Accept-Language correctly with unordinary LANGUAGE' '\n> +\tcheck_language \"ko-KR, *;q=0.1\" \"ko_KR:\" &&\n> +\tcheck_language \"ko-KR, *;q=0.1\" \" ko_KR\" &&\n> +\tcheck_language \"ko-KR, en-US;q=0.9, *;q=0.1\" \"ko_KR: en_US\" &&\n> +\tcheck_language \"ko-KR, en-US;q=0.9, *;q=0.1\" \"ko_KR::en_US\" &&\n> +\tcheck_language \"ko-KR, *;q=0.1\" \":::ko_KR\"'\n> +\n> +test_expect_success 'git client sends Accept-Language with many preferred languages' '\n> +\tcheck_language \"ko-KR, en-US;q=0.99, fr-CA;q=0.98, de;q=0.97, sr;q=0.96, \\\n> +ja;q=0.95, zh;q=0.94, sv;q=0.93, pt;q=0.92, nb;q=0.91, *;q=0.01\" \\\n> +\t\tko_KR.EUC-KR:en_US.UTF-8:fr_CA:de.UTF-8@euro:sr@latin:ja:zh:sv:pt:nb\n> +'\n"},{"id":"254085","messageId":"CAPig+cQZG3gWEw8_HHHvP6EDLKkf-nMZLwkE4OF9hwNX72wgXw@mail.gmail.com","threadId":"37174","inReplyTo":"1419266658-1180-2-git-send-email-eungjun.yi@navercorp.com","subject":"Re: [PATCH v6 1/1] http: Add Accept-Language header if possible","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-12-24T20:35:09Z","receivedAt":"2014-12-24T20:35:09Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Dec 22, 2014 at 11:44 AM, Yi EungJun wrote:\n> From: Yi EungJun \n>\n> Add an Accept-Language header which indicates the user's preferred\n> languages defined by $LANGUAGE, $LC_ALL, $LC_MESSAGES and $LANG.\n>\n> Examples:\n> LANGUAGE= -> \"\"\n> LANGUAGE=ko:en -> \"Accept-Language: ko, en;q=0.9, *;q=0.1\"\n> LANGUAGE=ko LANG=en_US.UTF-8 -> \"Accept-Language: ko, *;q=0.1\"\n> LANGUAGE= LANG=en_US.UTF-8 -> \"Accept-Language: en-US, *;q=0.1\"\n>\n> This gives git servers a chance to display remote error messages in\n> the user's preferred language.\n>\n> Limit the number of languages to 1,000 because q-value must not be\n> smaller than 0.001, and limit the length of Accept-Language header to\n> 4,000 bytes for some HTTP servers which cannot accept such long header.\n\nJust a few comments and observations below. Alone, they are not\nnecessarily worth a re-roll, but if you happen to re-roll for some\nother reason, perhaps take them into consideration.\n\n> Signed-off-by: Yi EungJun \n> ---\n> diff --git a/http.c b/http.c\n> index 040f362..7a77708 100644\n> --- a/http.c\n> +++ b/http.c\n> @@ -986,6 +993,166 @@ static void extract_content_type(struct strbuf *raw, struct strbuf *type,\n> strbuf_addstr(charset, \"ISO-8859-1\");\n> }\n>\n> +static void write_accept_language(struct strbuf *buf)\n> +{\n> + [...]\n> + /*\n> + * MAX_LANGS must not be larger than 1,000. If it is larger than that,\n> + * q-value will be smaller than 0.001, the minimum q-value the HTTP\n> + * specification allows [1].\n> + *\n> + * [1]: http://tools.ietf.org/html/rfc7231#section-5.3.1\n> + */\n> + const int MAX_LANGS = 1000;\n> + const int MAX_SIZE_OF_HEADER = 4000;\n> + const int MAX_SIZE_OF_ASTERISK_ELEMENT = 11; /* for \", *;q=0.001\" */\n\nThese two MAX_SIZE_* constants are never used individually, but rather\nonly as (MAX_SIZE_OF_HEADER - MAX_SIZE_OF_ASTERISK_ELEMENT). It might\nbe a bit more readable to compute the final value here, with a\nsuitable comment, rather than at point-of-use. Perhaps something like:\n\n /* limit of some HTTP servers is 4000 - strlen(\", *;q=0.001\") */\n const int MAX_HEADER_SIZE = 4000 - 11;\n\nMore below.\n\n> + [...]\n> + /*\n> + * Convert a list of colon-separated locale values [1][2] to a list of\n> + * comma-separated language tags [3] which can be used as a value of\n> + * Accept-Language header.\n> + [...]\n> + */\n> + for (pos = lang_begin; ; pos++) {\n> + if (!*pos || *pos == ':') {\n> + if (is_q_factor_required) {\n> + /* Put a q-factor only if it is less than 1.0. */\n> + if (q < max_q)\n> + strbuf_addf(buf, q_format, q);\n> +\n> + if (q > 1)\n> + q--;\n> +\n> + last_size = buf->len;\n> +\n> + is_q_factor_required = 0;\n> + }\n> + parse_state = SEPARATOR;\n> + } else if (parse_state == CODESET_OR_MODIFIER)\n> + continue;\n> + else if (*pos == ' ') /* Ignore whitespace character */\n> + continue;\n> + else if (*pos == '.' || *pos == '@') /* Remove .codeset and @modifier. */\n> + parse_state = CODESET_OR_MODIFIER;\n> + else {\n> + if (parse_state != LANGUAGE_TAG && q < max_q)\n> + strbuf_addstr(buf, \", \");\n> + strbuf_addch(buf, *pos == '_' ? '-' : *pos);\n> + is_q_factor_required = 1;\n> + parse_state = LANGUAGE_TAG;\n> + }\n> +\n> + if (buf->len > MAX_SIZE_OF_HEADER - MAX_SIZE_OF_ASTERISK_ELEMENT) {\n> + strbuf_remove(buf, last_size, buf->len - last_size);\n> + break;\n> + }\n> +\n> + if (!*pos)\n> + break;\n> + }\n\nAlthough often suitable when parsing complex inputs, state machines\ndemand high cognitive load. The input you're parsing, on the other\nhand, is straightforward and can easily be processed with a simple\nsequential parser, which is easier to reason about and review for\ncorrectness. For instance, something like this:\n\n while (*s) {\n /* collect language tag */\n for (; *s && *s != '.' && *s != '@' && *s != ':'; s++)\n strbuf_addch(buf, *s == '_' ? '-' : *s);\n\n /* skip .codeset and @modifier */\n while (*s && *s != ':')\n s++;\n\n strbuf_addf(buf, q_format, q);\n ... other bookkeeping ...\n\n if (*s == ':')\n s++;\n }\n\nThis example is intentionally simplified but illustrates the general\nidea. It lacks comma insertion (left as an exercise for the reader)\nand empty language tag handling (\":en\", \"en::ko\"); and doesn't take\nwhitespace into consideration since it wasn't clear why your v6 parser\npays attention to embedded spaces, whereas your earlier versions did\nnot.\n\n> + /* Don't add Accept-Language header if no language is preferred. */\n> + if (q >= max_q) {\n> + strbuf_reset(buf);\n> + return;\n> + }\n> +\n> + /* Add '*' with minimum q-factor greater than 0.0. */\n> + strbuf_addstr(buf, \", *\");\n> + strbuf_addf(buf, q_format, 1);\n> +}\n> +\n> +/*\n> + * Get an Accept-Language header which indicates user's preferred languages.\n> + *\n> + * This function always return non-NULL string as strbuf_detach() does.\n\nA couple comments:\n\nIt's not necessary to explain the public API in terms of an\nimplementation detail. Callers of this function don't care and don't\nneed to know that the value was constructed via strbuf, nor that it is\nsomehow dependent upon the behavior of the underlying implementation\nof strbuf_detach().\n\nThis is a somewhat unusual contract. It's much more common and\nidiomatic in C to return NULL as an indication of \"no preference\" (or\n\"failure\") than to return an empty string.\n\n> + *\n> + * Examples:\n> + * LANGUAGE= -> \"\"\n> + * LANGUAGE=ko:en -> \"Accept-Language: ko, en; q=0.9, *; q=0.1\"\n> + * LANGUAGE=ko_KR.UTF-8:sr@latin -> \"Accept-Language: ko-KR, sr; q=0.9, *; q=0.1\"\n> + * LANGUAGE=ko LANG=en_US.UTF-8 -> \"Accept-Language: ko, *; q=0.1\"\n> + * LANGUAGE= LANG=en_US.UTF-8 -> \"Accept-Language: en-US, *; q=0.1\"\n> + * LANGUAGE= LANG=C -> \"\"\n> + */\n> +static const char *get_accept_language(void)\n> +{\n> + if (!cached_accept_language) {\n> + struct strbuf buf = STRBUF_INIT;\n> + write_accept_language(&buf);\n> + cached_accept_language = strbuf_detach(&buf, NULL);\n> + strbuf_release(&buf);\n\nJunio already mentioned that strbuf_release() is unnecessary following\nstrbuf_detach().\n\n> + }\n> +\n> + return cached_accept_language;\n> +}\n> +\n> /* http_request() targets */\n> #define HTTP_REQUEST_STRBUF 0\n> #define HTTP_REQUEST_FILE 1\n"},{"id":"254138","messageId":"xmqqegri1lbs.fsf@gitster.dls.corp.google.com","threadId":"37174","inReplyTo":"CAPig+cQZG3gWEw8_HHHvP6EDLKkf-nMZLwkE4OF9hwNX72wgXw@mail.gmail.com","subject":"Re: [PATCH v6 1/1] http: Add Accept-Language header if possible","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-12-29T16:18:15Z","receivedAt":"2014-12-29T16:18:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> Just a few comments and observations below. Alone, they are not\n> necessarily worth a re-roll, but if you happen to re-roll for some\n> other reason, perhaps take them into consideration.\n\nI actually think everything you said in this review makes sense and\nwill make the resulting code a lot better (especially the part on\nthe parsing loop).\n\nThanks, as usual, for a careful reading.\n"},{"id":"254863","messageId":"1421583806-3563-1-git-send-email-eungjun.yi@navercorp.com","threadId":"37174","inReplyTo":"xmqqegri1lbs.fsf@gitster.dls.corp.google.com","subject":"[PATCH v7 0/1] http: Add Accept-Language header if possible","fromName":"Yi EungJun","fromEmail":"semtlenori@gmail.com","sentAt":"2015-01-18T12:23:25Z","receivedAt":"2015-01-18T12:23:25Z","isPatch":true,"sender":{"key":"semtlenori@gmail.com","avatar":"https://gravatar.com/avatar/8363435d2badb3450df0dd7c4ec2113f6e1d62c44dd3cf7dfe83c5a389b8a9bf?d=mp&s=160"},"body":"From: Yi EungJun \n\nChanges since v6\n\n>From Junio C Hamano's review:\n\n* Fix check_language() in t5550-http-fetch-dumb.sh as his suggestion.\n\n>From Eric Sunshine's review:\n\n* Rewrite the parser without state.\n\nYi EungJun (1):\n http: Add Accept-Language header if possible\n\n http.c | 152 +++++++++++++++++++++++++++++++++++++++++++++\n remote-curl.c | 2 +\n t/t5550-http-fetch-dumb.sh | 42 +++++++++++++\n 3 files changed, 196 insertions(+)\n\n-- \n2.2.0.44.g37b3e56.dirty\n"},{"id":"254864","messageId":"1421583995-3663-1-git-send-email-eungjun.yi@navercorp.com","threadId":"37174","inReplyTo":"1421583806-3563-1-git-send-email-eungjun.yi@navercorp.com","subject":"[PATCH v7 1/1] http: Add Accept-Language header if possible","fromName":"Yi EungJun","fromEmail":"semtlenori@gmail.com","sentAt":"2015-01-18T12:26:35Z","receivedAt":"2015-01-18T12:26:35Z","isPatch":true,"sender":{"key":"semtlenori@gmail.com","avatar":"https://gravatar.com/avatar/8363435d2badb3450df0dd7c4ec2113f6e1d62c44dd3cf7dfe83c5a389b8a9bf?d=mp&s=160"},"body":"From: Yi EungJun \n\nAdd an Accept-Language header which indicates the user's preferred\nlanguages defined by $LANGUAGE, $LC_ALL, $LC_MESSAGES and $LANG.\n\nExamples:\n LANGUAGE= -> \"\"\n LANGUAGE=ko:en -> \"Accept-Language: ko, en;q=0.9, *;q=0.1\"\n LANGUAGE=ko LANG=en_US.UTF-8 -> \"Accept-Language: ko, *;q=0.1\"\n LANGUAGE= LANG=en_US.UTF-8 -> \"Accept-Language: en-US, *;q=0.1\"\n\nThis gives git servers a chance to display remote error messages in\nthe user's preferred language.\n\nLimit the number of languages to 1,000 because q-value must not be\nsmaller than 0.001, and limit the length of Accept-Language header to\n4,000 bytes for some HTTP servers which cannot accept such long header.\n\nSigned-off-by: Yi EungJun \n---\n http.c | 152 +++++++++++++++++++++++++++++++++++++++++++++\n remote-curl.c | 2 +\n t/t5550-http-fetch-dumb.sh | 42 +++++++++++++\n 3 files changed, 196 insertions(+)\n\ndiff --git a/http.c b/http.c\nindex 040f362..349b033 100644\n--- a/http.c\n+++ b/http.c\n@@ -68,6 +68,8 @@ static struct curl_slist *no_pragma_header;\n \n static struct active_request_slot *active_queue_head;\n \n+static char *cached_accept_language;\n+\n size_t fread_buffer(char *ptr, size_t eltsize, size_t nmemb, void *buffer_)\n {\n \tsize_t size = eltsize * nmemb;\n@@ -515,6 +517,11 @@ void http_cleanup(void)\n \t\tcert_auth.password = NULL;\n \t}\n \tssl_cert_password_required = 0;\n+\n+\tif (cached_accept_language) {\n+\t\tfree(cached_accept_language);\n+\t\tcached_accept_language = NULL;\n+\t}\n }\n \n struct active_request_slot *get_active_slot(void)\n@@ -986,6 +993,145 @@ static void extract_content_type(struct strbuf *raw, struct strbuf *type,\n \t\tstrbuf_addstr(charset, \"ISO-8859-1\");\n }\n \n+/*\n+ * Guess the user's preferred languages from the value in LANGUAGE environment\n+ * variable and LC_MESSAGES locale category if NO_GETTEXT is not defined.\n+ *\n+ * The result can be a colon-separated list like \"ko:ja:en\".\n+ */\n+static const char *get_preferred_languages(void)\n+{\n+\tconst char *retval;\n+\n+\tretval = getenv(\"LANGUAGE\");\n+\tif (retval && *retval)\n+\t\treturn retval;\n+\n+#ifndef NO_GETTEXT\n+\tretval = setlocale(LC_MESSAGES, NULL);\n+\tif (retval && *retval &&\n+\t\tstrcmp(retval, \"C\") &&\n+\t\tstrcmp(retval, \"POSIX\"))\n+\t\treturn retval;\n+#endif\n+\n+\treturn NULL;\n+}\n+\n+static void write_accept_language(struct strbuf *buf)\n+{\n+\t/*\n+\t * MAX_DECIMAL_PLACES must not be larger than 3. If it is larger than\n+\t * that, q-value will be smaller than 0.001, the minimum q-value the\n+\t * HTTP specification allows. See\n+\t * http://tools.ietf.org/html/rfc7231#section-5.3.1 for q-value.\n+\t */\n+\tconst int MAX_DECIMAL_PLACES = 3;\n+\tconst int MAX_LANGUAGE_TAGS = 1000;\n+\tconst int MAX_ACCEPT_LANGUAGE_HEADER_SIZE = 4000;\n+\tstruct strbuf *language_tags = NULL;\n+\tint num_langs;\n+\tconst char *s = get_preferred_languages();\n+\n+\t/* Don't add Accept-Language header if no language is preferred. */\n+\tif (!s)\n+\t\treturn;\n+\n+\t/*\n+\t * Split the colon-separated string of preferred languages into\n+\t * language_tags array.\n+\t */\n+\tdo {\n+\t\t/* increase language_tags array to add new language tag */\n+\t\tREALLOC_ARRAY(language_tags, num_langs + 1);\n+\t\tstrbuf_init(&language_tags[num_langs], 0);\n+\n+\t\t/* collect language tag */\n+\t\tfor (; *s && (isalnum(*s) || *s == '_'); s++)\n+\t\t\tstrbuf_addch(&language_tags[num_langs], *s == '_' ? '-' : *s);\n+\n+\t\t/* skip .codeset, @modifier and any other unnecessary parts */\n+\t\twhile (*s && *s != ':')\n+\t\t\ts++;\n+\n+\t\tif (language_tags[num_langs].len > 0) {\n+\t\t\tnum_langs++;\n+\t\t\tif (num_langs >= MAX_LANGUAGE_TAGS - 1) /* -1 for '*' */\n+\t\t\t\tbreak;\n+\t\t}\n+\t} while (*s++);\n+\n+\t/* write Accept-Language header into buf */\n+\tif (num_langs >= 1) {\n+\t\tint i;\n+\t\tint last_buf_len;\n+\t\tint max_q;\n+\t\tint decimal_places;\n+\t\tchar q_format[32];\n+\n+\t\t/* add '*' */\n+\t\tREALLOC_ARRAY(language_tags, num_langs + 1);\n+\t\tstrbuf_init(&language_tags[num_langs], 0);\n+\t\tstrbuf_addstr(&language_tags[num_langs++], \"*\");\n+\n+\t\t/* compute decimal_places */\n+\t\tfor (max_q = 1, decimal_places = 0;\n+\t\t\t\tmax_q < num_langs && decimal_places <= MAX_DECIMAL_PLACES;\n+\t\t\t\tdecimal_places++, max_q *= 10)\n+\t\t\t;\n+\n+\t\tsprintf(q_format, \";q=0.%%0%dd\", decimal_places);\n+\n+\t\tstrbuf_addstr(buf, \"Accept-Language: \");\n+\n+\t\tfor(i = 0; i < num_langs; i++) {\n+\t\t\tif (language_tags[i].len == 0)\n+\t\t\t\tcontinue;\n+\n+\t\t\tif (i > 0)\n+\t\t\t\tstrbuf_addstr(buf, \", \");\n+\n+\t\t\tstrbuf_addstr(buf, strbuf_detach(&language_tags[i], NULL));\n+\n+\t\t\tif (i > 0)\n+\t\t\t\tstrbuf_addf(buf, q_format, max_q - i);\n+\n+\t\t\tif (buf->len > MAX_ACCEPT_LANGUAGE_HEADER_SIZE) {\n+\t\t\t\tstrbuf_remove(buf, last_buf_len, buf->len - last_buf_len);\n+\t\t\t\tbreak;\n+\t\t\t}\n+\n+\t\t\tlast_buf_len = buf->len;\n+\t\t}\n+\t}\n+\n+\tfree(language_tags);\n+}\n+\n+/*\n+ * Get an Accept-Language header which indicates user's preferred languages.\n+ *\n+ * This function always return non-NULL string as strbuf_detach() does.\n+ *\n+ * Examples:\n+ * LANGUAGE= -> \"\"\n+ * LANGUAGE=ko:en -> \"Accept-Language: ko, en; q=0.9, *; q=0.1\"\n+ * LANGUAGE=ko_KR.UTF-8:sr@latin -> \"Accept-Language: ko-KR, sr; q=0.9, *; q=0.1\"\n+ * LANGUAGE=ko LANG=en_US.UTF-8 -> \"Accept-Language: ko, *; q=0.1\"\n+ * LANGUAGE= LANG=en_US.UTF-8 -> \"Accept-Language: en-US, *; q=0.1\"\n+ * LANGUAGE= LANG=C -> \"\"\n+ */\n+static const char *get_accept_language(void)\n+{\n+\tif (!cached_accept_language) {\n+\t\tstruct strbuf buf = STRBUF_INIT;\n+\t\twrite_accept_language(&buf);\n+\t\tcached_accept_language = strbuf_detach(&buf, NULL);\n+\t}\n+\n+\treturn cached_accept_language;\n+}\n+\n /* http_request() targets */\n #define HTTP_REQUEST_STRBUF\t0\n #define HTTP_REQUEST_FILE\t1\n@@ -998,6 +1144,7 @@ static int http_request(const char *url,\n \tstruct slot_results results;\n \tstruct curl_slist *headers = NULL;\n \tstruct strbuf buf = STRBUF_INIT;\n+\tconst char *accept_language;\n \tint ret;\n \n \tslot = get_active_slot();\n@@ -1023,6 +1170,11 @@ static int http_request(const char *url,\n \t\t\t\t\t fwrite_buffer);\n \t}\n \n+\taccept_language = get_accept_language();\n+\n+\tif (strlen(accept_language) > 0)\n+\t\theaders = curl_slist_append(headers, accept_language);\n+\n \tstrbuf_addstr(&buf, \"Pragma:\");\n \tif (options && options->no_cache)\n \t\tstrbuf_addstr(&buf, \" no-cache\");\ndiff --git a/remote-curl.c b/remote-curl.c\nindex dd63bc2..04989e5 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -962,6 +962,8 @@ int main(int argc, const char **argv)\n \tstruct strbuf buf = STRBUF_INIT;\n \tint nongit;\n \n+\tgit_setup_gettext();\n+\n \tgit_extract_argv0_path(argv[0]);\n \tsetup_git_directory_gently(&nongit);\n \tif (argc < 2) {\ndiff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh\nindex ac71418..e1e2938 100755\n--- a/t/t5550-http-fetch-dumb.sh\n+++ b/t/t5550-http-fetch-dumb.sh\n@@ -196,5 +196,47 @@ test_expect_success 'reencoding is robust to whitespace oddities' '\n \tgrep \"this is the error message\" stderr\n '\n \n+check_language () {\n+\tcase \"$2\" in\n+\t'')\n+\t\t>expect\n+\t\t;;\n+\t?*)\n+\t\techo \"Accept-Language: $1\" >expect\n+\t\t;;\n+\tesac &&\n+\tGIT_CURL_VERBOSE=1 \\\n+\tLANGUAGE=$2 \\\n+\tgit ls-remote \"$HTTPD_URL/dumb/repo.git\" >output 2>&1 &&\n+\ttr -d '\\015' actual &&\n+\ttest_cmp expect actual\n+}\n+\n+test_expect_success 'git client sends Accept-Language based on LANGUAGE' '\n+\tcheck_language \"ko-KR, *;q=0.9\" ko_KR.UTF-8'\n+\n+test_expect_success 'git client sends Accept-Language correctly with unordinary LANGUAGE' '\n+\tcheck_language \"ko-KR, *;q=0.9\" \"ko_KR:\" &&\n+\tcheck_language \"ko-KR, en-US;q=0.9, *;q=0.8\" \"ko_KR::en_US\" &&\n+\tcheck_language \"ko-KR, *;q=0.9\" \":::ko_KR\" &&\n+\tcheck_language \"ko-KR, en-US;q=0.9, *;q=0.8\" \"ko_KR!!:en_US\" &&\n+\tcheck_language \"ko-KR, ja-JP;q=0.9, *;q=0.8\" \"ko_KR en_US:ja_JP\"'\n+\n+test_expect_success 'git client sends Accept-Language with many preferred languages' '\n+\tcheck_language \"ko-KR, en-US;q=0.9, fr-CA;q=0.8, de;q=0.7, sr;q=0.6, \\\n+ja;q=0.5, zh;q=0.4, sv;q=0.3, pt;q=0.2, *;q=0.1\" \\\n+\t\tko_KR.EUC-KR:en_US.UTF-8:fr_CA:de.UTF-8@euro:sr@latin:ja:zh:sv:pt &&\n+\tcheck_language \"ko-KR, en-US;q=0.99, fr-CA;q=0.98, de;q=0.97, sr;q=0.96, \\\n+ja;q=0.95, zh;q=0.94, sv;q=0.93, pt;q=0.92, nb;q=0.91, *;q=0.90\" \\\n+\t\tko_KR.EUC-KR:en_US.UTF-8:fr_CA:de.UTF-8@euro:sr@latin:ja:zh:sv:pt:nb\n+'\n+\n+test_expect_success 'git client does not send an empty Accept-Language' '\n+\tGIT_CURL_VERBOSE=1 LANGUAGE= git ls-remote \"$HTTPD_URL/dumb/repo.git\" 2>stderr &&\n+\t! grep \"^Accept-Language:\" stderr\n+'\n+\n stop_httpd\n test_done\n-- \n2.2.0.44.g37b3e56.dirty\n"},{"id":"254867","messageId":"54BBCDBC.1010300@web.de","threadId":"37174","inReplyTo":"1421583995-3663-1-git-send-email-eungjun.yi@navercorp.com","subject":"Re: [PATCH v7 1/1] http: Add Accept-Language header if possible","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2015-01-18T15:14:04Z","receivedAt":"2015-01-18T15:14:04Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 18.01.15 13:26, Yi EungJun wrote:\n> From: Yi EungJun \n\n> diff --git a/http.c b/http.c\n> index 040f362..349b033 100644\n> --- a/http.c\n> +++ b/http.c\n> @@ -68,6 +68,8 @@ static struct curl_slist *no_pragma_header;\n> \n> static struct active_request_slot *active_queue_head;\n> \n> +static char *cached_accept_language;\n> +\n> size_t fread_buffer(char *ptr, size_t eltsize, size_t nmemb, void *buffer_)\n> {\n> \tsize_t size = eltsize * nmemb;\n> @@ -515,6 +517,11 @@ void http_cleanup(void)\n> \t\tcert_auth.password = NULL;\n> \t}\n> \tssl_cert_password_required = 0;\n> +\n> +\tif (cached_accept_language) {\n> +\t\tfree(cached_accept_language);\n> +\t\tcached_accept_language = NULL;\n> +\t}\n\nMinor remark:\nfree(NULL) is legal and does nothing.\nWe can simplify the code somewhat:\n\n \tssl_cert_password_required = 0;\n \n \tfree(cached_accept_language);\n \tcached_accept_language = NULL;\n\n\n\n\n> }\n> \n> struct active_request_slot *get_active_slot(void)\n> @@ -986,6 +993,145 @@ static void extract_content_type(struct strbuf *raw, struct strbuf *type,\n> \t\tstrbuf_addstr(charset, \"ISO-8859-1\");\n> }\n> \n> +/*\n> + * Guess the user's preferred languages from the value in LANGUAGE environment\n> + * variable and LC_MESSAGES locale category if NO_GETTEXT is not defined.\n> + *\n> + * The result can be a colon-separated list like \"ko:ja:en\".\n> + */\n> +static const char *get_preferred_languages(void)\n> +{\n> +\tconst char *retval;\n> +\n> +\tretval = getenv(\"LANGUAGE\");\n> +\tif (retval && *retval)\n> +\t\treturn retval;\n> +\n> +#ifndef NO_GETTEXT\n> +\tretval = setlocale(LC_MESSAGES, NULL);\n> +\tif (retval && *retval &&\n> +\t\tstrcmp(retval, \"C\") &&\n> +\t\tstrcmp(retval, \"POSIX\"))\n> +\t\treturn retval;\n> +#endif\n> +\n> +\treturn NULL;\n> +}\n> +\n> +static void write_accept_language(struct strbuf *buf)\n> +{\n> +\t/*\n> +\t * MAX_DECIMAL_PLACES must not be larger than 3. If it is larger than\n> +\t * that, q-value will be smaller than 0.001, the minimum q-value the\n> +\t * HTTP specification allows. See\n> +\t * http://tools.ietf.org/html/rfc7231#section-5.3.1 for q-value.\n> +\t */\n> +\tconst int MAX_DECIMAL_PLACES = 3;\n> +\tconst int MAX_LANGUAGE_TAGS = 1000;\n> +\tconst int MAX_ACCEPT_LANGUAGE_HEADER_SIZE = 4000;\n> +\tstruct strbuf *language_tags = NULL;\n> +\tint num_langs;\n> +\tconst char *s = get_preferred_languages();\n> +\n> +\t/* Don't add Accept-Language header if no language is preferred. */\n> +\tif (!s)\n> +\t\treturn;\n> +\n> +\t/*\n> +\t * Split the colon-separated string of preferred languages into\n> +\t * language_tags array.\n> +\t */\n> +\tdo {\n> +\t\t/* increase language_tags array to add new language tag */\n> +\t\tREALLOC_ARRAY(language_tags, num_langs + 1);\n> +\t\tstrbuf_init(&language_tags[num_langs], 0);\n> +\n> +\t\t/* collect language tag */\n> +\t\tfor (; *s && (isalnum(*s) || *s == '_'); s++)\n> +\t\t\tstrbuf_addch(&language_tags[num_langs], *s == '_' ? '-' : *s);\n> +\n> +\t\t/* skip .codeset, @modifier and any other unnecessary parts */\n> +\t\twhile (*s && *s != ':')\n> +\t\t\ts++;\n> +\n> +\t\tif (language_tags[num_langs].len > 0) {\n> +\t\t\tnum_langs++;\n> +\t\t\tif (num_langs >= MAX_LANGUAGE_TAGS - 1) /* -1 for '*' */\n> +\t\t\t\tbreak;\n> +\t\t}\n> +\t} while (*s++);\n> +\n> +\t/* write Accept-Language header into buf */\n> +\tif (num_langs >= 1) {\n> +\t\tint i;\n> +\t\tint last_buf_len;\n> +\t\tint max_q;\n> +\t\tint decimal_places;\n> +\t\tchar q_format[32];\n> +\n> +\t\t/* add '*' */\n> +\t\tREALLOC_ARRAY(language_tags, num_langs + 1);\n> +\t\tstrbuf_init(&language_tags[num_langs], 0);\n> +\t\tstrbuf_addstr(&language_tags[num_langs++], \"*\");\n> +\n> +\t\t/* compute decimal_places */\n> +\t\tfor (max_q = 1, decimal_places = 0;\n> +\t\t\t\tmax_q < num_langs && decimal_places <= MAX_DECIMAL_PLACES;\n> +\t\t\t\tdecimal_places++, max_q *= 10)\n> +\t\t\t;\n> +\n> +\t\tsprintf(q_format, \";q=0.%%0%dd\", decimal_places);\n> +\n> +\t\tstrbuf_addstr(buf, \"Accept-Language: \");\n> +\n> +\t\tfor(i = 0; i < num_langs; i++) {\n> +\t\t\tif (language_tags[i].len == 0)\n> +\t\t\t\tcontinue;\n> +\n> +\t\t\tif (i > 0)\n> +\t\t\t\tstrbuf_addstr(buf, \", \");\n> +\n> +\t\t\tstrbuf_addstr(buf, strbuf_detach(&language_tags[i], NULL));\n> +\n> +\t\t\tif (i > 0)\n> +\t\t\t\tstrbuf_addf(buf, q_format, max_q - i);\n> +\n> +\t\t\tif (buf->len > MAX_ACCEPT_LANGUAGE_HEADER_SIZE) {\n> +\t\t\t\tstrbuf_remove(buf, last_buf_len, buf->len - last_buf_len);\n> +\t\t\t\tbreak;\n> +\t\t\t}\n> +\n> +\t\t\tlast_buf_len = buf->len;\n> +\t\t}\n> +\t}\n> +\n> +\tfree(language_tags);\n> +}\n> +\n> +/*\n> + * Get an Accept-Language header which indicates user's preferred languages.\n> + *\n> + * This function always return non-NULL string as strbuf_detach() does.\n> + *\n> + * Examples:\n> + * LANGUAGE= -> \"\"\n> + * LANGUAGE=ko:en -> \"Accept-Language: ko, en; q=0.9, *; q=0.1\"\n> + * LANGUAGE=ko_KR.UTF-8:sr@latin -> \"Accept-Language: ko-KR, sr; q=0.9, *; q=0.1\"\n> + * LANGUAGE=ko LANG=en_US.UTF-8 -> \"Accept-Language: ko, *; q=0.1\"\n> + * LANGUAGE= LANG=en_US.UTF-8 -> \"Accept-Language: en-US, *; q=0.1\"\n> + * LANGUAGE= LANG=C -> \"\"\n> + */\n> +static const char *get_accept_language(void)\n> +{\n> +\tif (!cached_accept_language) {\n> +\t\tstruct strbuf buf = STRBUF_INIT;\n> +\t\twrite_accept_language(&buf);\n> +\t\tcached_accept_language = strbuf_detach(&buf, NULL);\n> +\t}\n> +\n> +\treturn cached_accept_language;\n> +}\n> +\n> /* http_request() targets */\n> #define HTTP_REQUEST_STRBUF\t0\n> #define HTTP_REQUEST_FILE\t1\n> @@ -998,6 +1144,7 @@ static int http_request(const char *url,\n> \tstruct slot_results results;\n> \tstruct curl_slist *headers = NULL;\n> \tstruct strbuf buf = STRBUF_INIT;\n> +\tconst char *accept_language;\n> \tint ret;\n> \n> \tslot = get_active_slot();\n> @@ -1023,6 +1170,11 @@ static int http_request(const char *url,\n> \t\t\t\t\t fwrite_buffer);\n> \t}\n> \n> +\taccept_language = get_accept_language();\n> +\n> +\tif (strlen(accept_language) > 0)\n> +\t\theaders = curl_slist_append(headers, accept_language);\n> +\n> \tstrbuf_addstr(&buf, \"Pragma:\");\n> \tif (options && options->no_cache)\n> \t\tstrbuf_addstr(&buf, \" no-cache\");\n> diff --git a/remote-curl.c b/remote-curl.c\n> index dd63bc2..04989e5 100644\n> --- a/remote-curl.c\n> +++ b/remote-curl.c\n> @@ -962,6 +962,8 @@ int main(int argc, const char **argv)\n> \tstruct strbuf buf = STRBUF_INIT;\n> \tint nongit;\n> \n> +\tgit_setup_gettext();\n> +\n> \tgit_extract_argv0_path(argv[0]);\n> \tsetup_git_directory_gently(&nongit);\n> \tif (argc < 2) {\n> diff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh\n> index ac71418..e1e2938 100755\n> --- a/t/t5550-http-fetch-dumb.sh\n> +++ b/t/t5550-http-fetch-dumb.sh\n> @@ -196,5 +196,47 @@ test_expect_success 'reencoding is robust to whitespace oddities' '\n> \tgrep \"this is the error message\" stderr\n> '\n> \n> +check_language () {\n> +\tcase \"$2\" in\n> +\t'')\n> +\t\t>expect\n> +\t\t;;\n> +\t?*)\n> +\t\techo \"Accept-Language: $1\" >expect\n> +\t\t;;\n> +\tesac &&\n> +\tGIT_CURL_VERBOSE=1 \\\n> +\tLANGUAGE=$2 \\\n> +\tgit ls-remote \"$HTTPD_URL/dumb/repo.git\" >output 2>&1 &&\n> +\ttr -d '\\015' +\tsort -u |\n> +\tsed -ne '/^Accept-Language:/ p' >actual &&\n> +\ttest_cmp expect actual\n> +}\n> +\n> +test_expect_success 'git client sends Accept-Language based on LANGUAGE' '\n> +\tcheck_language \"ko-KR, *;q=0.9\" ko_KR.UTF-8'\n> +\n> +test_expect_success 'git client sends Accept-Language correctly with unordinary LANGUAGE' '\n> +\tcheck_language \"ko-KR, *;q=0.9\" \"ko_KR:\" &&\n> +\tcheck_language \"ko-KR, en-US;q=0.9, *;q=0.8\" \"ko_KR::en_US\" &&\n> +\tcheck_language \"ko-KR, *;q=0.9\" \":::ko_KR\" &&\n> +\tcheck_language \"ko-KR, en-US;q=0.9, *;q=0.8\" \"ko_KR!!:en_US\" &&\n> +\tcheck_language \"ko-KR, ja-JP;q=0.9, *;q=0.8\" \"ko_KR en_US:ja_JP\"'\n> +\n> +test_expect_success 'git client sends Accept-Language with many preferred languages' '\n> +\tcheck_language \"ko-KR, en-US;q=0.9, fr-CA;q=0.8, de;q=0.7, sr;q=0.6, \\\n> +ja;q=0.5, zh;q=0.4, sv;q=0.3, pt;q=0.2, *;q=0.1\" \\\n> +\t\tko_KR.EUC-KR:en_US.UTF-8:fr_CA:de.UTF-8@euro:sr@latin:ja:zh:sv:pt &&\n> +\tcheck_language \"ko-KR, en-US;q=0.99, fr-CA;q=0.98, de;q=0.97, sr;q=0.96, \\\n> +ja;q=0.95, zh;q=0.94, sv;q=0.93, pt;q=0.92, nb;q=0.91, *;q=0.90\" \\\n> +\t\tko_KR.EUC-KR:en_US.UTF-8:fr_CA:de.UTF-8@euro:sr@latin:ja:zh:sv:pt:nb\n> +'\n> +\n> +test_expect_success 'git client does not send an empty Accept-Language' '\n> +\tGIT_CURL_VERBOSE=1 LANGUAGE= git ls-remote \"$HTTPD_URL/dumb/repo.git\" 2>stderr &&\n> +\t! grep \"^Accept-Language:\" stderr\n> +'\n> +\n> stop_httpd\n> test_done\n> \n"},{"id":"254907","messageId":"CAPig+cSrX=mpFWznRtYgQOsi7YU7ewEo6VqMmkq9OSiveG961Q@mail.gmail.com","threadId":"37174","inReplyTo":"1421583995-3663-1-git-send-email-eungjun.yi@navercorp.com","subject":"Re: [PATCH v6 0/1] http: Add Accept-Language header if possible","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-01-19T20:21:16Z","receivedAt":"2015-01-19T20:21:16Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sunday, January 18, 2015, Yi EungJun wrote:\n> Add an Accept-Language header which indicates the user's preferred\n> languages defined by $LANGUAGE, $LC_ALL, $LC_MESSAGES and $LANG.\n>\n> Examples:\n> LANGUAGE= -> \"\"\n> LANGUAGE=ko:en -> \"Accept-Language: ko, en;q=0.9, *;q=0.1\"\n> LANGUAGE=ko LANG=en_US.UTF-8 -> \"Accept-Language: ko, *;q=0.1\"\n> LANGUAGE= LANG=en_US.UTF-8 -> \"Accept-Language: en-US, *;q=0.1\"\n>\n> This gives git servers a chance to display remote error messages in\n> the user's preferred language.\n>\n> Limit the number of languages to 1,000 because q-value must not be\n> smaller than 0.001, and limit the length of Accept-Language header to\n> 4,000 bytes for some HTTP servers which cannot accept such long header.\n>\n> Signed-off-by: Yi EungJun \n> ---\n> diff --git a/http.c b/http.c\n> index 040f362..349b033 100644\n> --- a/http.c\n> +++ b/http.c\n> @@ -986,6 +993,145 @@ static void extract_content_type(struct strbuf *raw, struct strbuf *type,\n> strbuf_addstr(charset, \"ISO-8859-1\");\n> }\n>\n> +static void write_accept_language(struct strbuf *buf)\n> +{\n> + [...]\n> + const int MAX_DECIMAL_PLACES = 3;\n> + const int MAX_LANGUAGE_TAGS = 1000;\n> + const int MAX_ACCEPT_LANGUAGE_HEADER_SIZE = 4000;\n> + struct strbuf *language_tags = NULL;\n> + int num_langs;\n\nMental note: 'num_langs' is not initialized.\n\n> + const char *s = get_preferred_languages();\n> +\n> + /* Don't add Accept-Language header if no language is preferred. */\n> + if (!s)\n> + return;\n> +\n> + /*\n> + * Split the colon-separated string of preferred languages into\n> + * language_tags array.\n> + */\n> + do {\n> + /* increase language_tags array to add new language tag */\n> + REALLOC_ARRAY(language_tags, num_langs + 1);\n\n'num_langs' is used here uninitialized.\n\n> + strbuf_init(&language_tags[num_langs], 0);\n> +\n> + /* collect language tag */\n> + for (; *s && (isalnum(*s) || *s == '_'); s++)\n> + strbuf_addch(&language_tags[num_langs], *s == '_' ? '-' : *s);\n> +\n> + /* skip .codeset, @modifier and any other unnecessary parts */\n> + while (*s && *s != ':')\n> + s++;\n> +\n> + if (language_tags[num_langs].len > 0) {\n\nMental note: An empty (\"\") language tag is never allowed in language_tags[].\n\n > + num_langs++;\n\nThis is a little bit ugly. At the top of the loop, you allocate space\nin the array for a strbuf and initialize it. However, if the language\ntag is empty (\"\"), then 'num_langs' is never incremented, so the next\ntime through the loop, strbuf_init() is invoked on the same block of\nmemory (assuming the realloc was a no-op since the allocation size did\nnot change), overwriting whatever was there and possibly leaking\nmemory. In this particular case, by examining the parser code closely,\nwe can see that nothing was added to the strbuf, so nothing is being\nleaked the next time around, given the current implementation of\nstrbuf.\n\nHowever, this is potentially fragile. A change to the implementation\nof strbuf in the future (for instance, if strbuf_init() allocates\nmemory immediately) could result in a leak here. Moreover, this\nno-leak situation only holds true if no text at all has been added to\nthe strbuf after strbuf_init(). If someone changes the parser in the\nfuture to operate a bit differently so that some text is added and\nthen removed from the strbuf, even though the end result still has\nlength is 0, then it will start leaking.\n\nOne way to make this more robust would be to have a separate strbuf\nfor collecting the language tag. When you encounter a non-empty tag,\nonly then grow the array and initialize the new strbuf in the array.\nFinally, use strbuf_swap() to swap the collected language tag into the\nnew array position. Something like this:\n\n struct strbuf tag = STRBUF_INIT;\n do {\n for (; *s && (isalnum(*s) || *s == '_'); s++)\n strbuf_addch(&tag, *s == '_' ? '-' : *s);\n\n [...]\n\n if (tag.len) {\n num_langs++;\n REALLOC_ARRAY(language_tags, num_langs);\n strbuf_init(&language_tags[num_langs], 0);\n strbuf_swap(&tag, &language_tags[num_langs]);\n\n if (num_langs >= ...)\n break;\n }\n while (...);\n strbuf_release(&tag);\n\n> + if (num_langs >= MAX_LANGUAGE_TAGS - 1) /* -1 for '*' */\n> + break;\n> + }\n> + } while (*s++);\n> +\n> + /* write Accept-Language header into buf */\n> + if (num_langs >= 1) {\n> + int i;\n> + int last_buf_len;\n\nMental note: 'last_buf_len' is not initialized.\n\n> + int max_q;\n> + int decimal_places;\n> + char q_format[32];\n> +\n> + /* add '*' */\n> + REALLOC_ARRAY(language_tags, num_langs + 1);\n> + strbuf_init(&language_tags[num_langs], 0);\n> + strbuf_addstr(&language_tags[num_langs++], \"*\");\n> +\n> + /* compute decimal_places */\n> + for (max_q = 1, decimal_places = 0;\n> + max_q < num_langs && decimal_places <= MAX_DECIMAL_PLACES;\n> + decimal_places++, max_q *= 10)\n> + ;\n> +\n> + sprintf(q_format, \";q=0.%%0%dd\", decimal_places);\n> +\n> + strbuf_addstr(buf, \"Accept-Language: \");\n> +\n> + for(i = 0; i < num_langs; i++) {\n> + if (language_tags[i].len == 0)\n> + continue;\n\nThe parsing code does not allow empty tags (\"\") in language_tags[], so\nthis conditional is useless, isn't it?\n\n> +\n> + if (i > 0)\n> + strbuf_addstr(buf, \", \");\n> +\n> + strbuf_addstr(buf, strbuf_detach(&language_tags[i], NULL));\n\nThis leaks the string detached from 'language_tag[i]' since\nstrbuf_addstr() does not take ownership of it.\n\n> + if (i > 0)\n> + strbuf_addf(buf, q_format, max_q - i);\n> +\n> + if (buf->len > MAX_ACCEPT_LANGUAGE_HEADER_SIZE) {\n> + strbuf_remove(buf, last_buf_len, buf->len - last_buf_len);\n\n'last_buf_len' is (potentially) used here uninitialized the first time\nthrough loop.\n\n> + break;\n> + }\n> +\n> + last_buf_len = buf->len;\n> + }\n> + }\n> +\n> + free(language_tags);\n\nThis _seems_ to be okay since strbuf_detach() was invoked for each\nstrbuf in language_tags[], however, those strings were in fact leaked\n(as noted above), so it's not actually correct.\n\n> +}\n> +\n> +/*\n> + * Get an Accept-Language header which indicates user's preferred languages.\n> + *\n> + * This function always return non-NULL string as strbuf_detach() does.\n\nRepeating from [1]: It's not good form to describe the published API\nin terms of an implementation detail (strbuf_detach). Also, it would\nbe more idiomatic in C to return NULL rather than empty string.\n\n[1]: http://article.gmane.org/gmane.comp.version-control.git/261810/\n\n> + *\n> + * Examples:\n> + * LANGUAGE= -> \"\"\n> + * LANGUAGE=ko:en -> \"Accept-Language: ko, en; q=0.9, *; q=0.1\"\n> + * LANGUAGE=ko_KR.UTF-8:sr@latin -> \"Accept-Language: ko-KR, sr; q=0.9, *; q=0.1\"\n> + * LANGUAGE=ko LANG=en_US.UTF-8 -> \"Accept-Language: ko, *; q=0.1\"\n> + * LANGUAGE= LANG=en_US.UTF-8 -> \"Accept-Language: en-US, *; q=0.1\"\n> + * LANGUAGE= LANG=C -> \"\"\n> + */\n> +static const char *get_accept_language(void)\n> +{\n> + if (!cached_accept_language) {\n> + struct strbuf buf = STRBUF_INIT;\n> + write_accept_language(&buf);\n> + cached_accept_language = strbuf_detach(&buf, NULL);\n> + }\n> +\n> + return cached_accept_language;\n> +}\n> +\n"},{"id":"255067","messageId":"xmqq7fwfuu62.fsf@gitster.dls.corp.google.com","threadId":"37174","inReplyTo":"1421583995-3663-1-git-send-email-eungjun.yi@navercorp.com","subject":"Re: [PATCH v7 1/1] http: Add Accept-Language header if possible","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-01-22T07:54:45Z","receivedAt":"2015-01-22T07:54:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Yi EungJun writes:\n\n> +static void write_accept_language(struct strbuf *buf)\n> +{\n> +\t/*\n> +\t * MAX_DECIMAL_PLACES must not be larger than 3. If it is larger than\n> +\t * that, q-value will be smaller than 0.001, the minimum q-value the\n> +\t * HTTP specification allows. See\n> +\t * http://tools.ietf.org/html/rfc7231#section-5.3.1 for q-value.\n> +\t */\n> +\tconst int MAX_DECIMAL_PLACES = 3;\n> +\tconst int MAX_LANGUAGE_TAGS = 1000;\n> +\tconst int MAX_ACCEPT_LANGUAGE_HEADER_SIZE = 4000;\n> +\tstruct strbuf *language_tags = NULL;\n> +\tint num_langs;\n\nNo initial value given to this variable, but...\n\n> +\tconst char *s = get_preferred_languages();\n> +\n> +\t/* Don't add Accept-Language header if no language is preferred. */\n> +\tif (!s)\n> +\t\treturn;\n> +\n> +\t/*\n> +\t * Split the colon-separated string of preferred languages into\n> +\t * language_tags array.\n> +\t */\n> +\tdo {\n> +\t\t/* increase language_tags array to add new language tag */\n> +\t\tREALLOC_ARRAY(language_tags, num_langs + 1);\n\n... it is nevertheless used. I think it was meant to start at 0?\n\n> +\t/* write Accept-Language header into buf */\n> +\tif (num_langs >= 1) {\n> +\t\tint i;\n> +\t\tint last_buf_len;\n\nThis is uninitialized...\n\n> +\t\tint max_q;\n> +\t\tint decimal_places;\n> +\t\tchar q_format[32];\n> +\n> +...\n> +\t\t\tif (buf->len > MAX_ACCEPT_LANGUAGE_HEADER_SIZE) {\n> +\t\t\t\tstrbuf_remove(buf, last_buf_len, buf->len - last_buf_len);\n\n... and then it is used here.\n"},{"id":"255331","messageId":"1422373918-14132-1-git-send-email-eungjun.yi@navercorp.com","threadId":"37174","inReplyTo":"xmqq7fwfuu62.fsf@gitster.dls.corp.google.com","subject":"[PATCH v8 0/1] http: Add Accept-Language header if possible","fromName":"Yi EungJun","fromEmail":"semtlenori@gmail.com","sentAt":"2015-01-27T15:51:57Z","receivedAt":"2015-01-27T15:51:57Z","isPatch":true,"sender":{"key":"semtlenori@gmail.com","avatar":"https://gravatar.com/avatar/8363435d2badb3450df0dd7c4ec2113f6e1d62c44dd3cf7dfe83c5a389b8a9bf?d=mp&s=160"},"body":"From: Yi EungJun \n\nChange since v7\n\nFrom Torsten Bögershausen's review:\n\n\t* remove unnecessary if-statement\n\nFrom Eric Sunshine's review:\n\n\t* fix memory leaks and uninitialized variables\n\t* remove unnecessary if-statement\n\nFrom Junio C Hamano's review:\n\n\t* fix uninitialized variables\n\nYi EungJun (1):\n http: Add Accept-Language header if possible\n\n http.c | 151 +++++++++++++++++++++++++++++++++++++++++++++\n remote-curl.c | 2 +\n t/t5550-http-fetch-dumb.sh | 42 +++++++++++++\n 3 files changed, 195 insertions(+)\n\n-- \n2.3.0.rc1.32.ga3df1c7\n"},{"id":"255332","messageId":"1422373918-14132-2-git-send-email-eungjun.yi@navercorp.com","threadId":"37174","inReplyTo":"1422373918-14132-1-git-send-email-eungjun.yi@navercorp.com","subject":"[PATCH] http: Add Accept-Language header if possible","fromName":"Yi EungJun","fromEmail":"semtlenori@gmail.com","sentAt":"2015-01-27T15:51:58Z","receivedAt":"2015-01-27T15:51:58Z","isPatch":true,"sender":{"key":"semtlenori@gmail.com","avatar":"https://gravatar.com/avatar/8363435d2badb3450df0dd7c4ec2113f6e1d62c44dd3cf7dfe83c5a389b8a9bf?d=mp&s=160"},"body":"From: Yi EungJun \n\nAdd an Accept-Language header which indicates the user's preferred\nlanguages defined by $LANGUAGE, $LC_ALL, $LC_MESSAGES and $LANG.\n\nExamples:\n LANGUAGE= -> \"\"\n LANGUAGE=ko:en -> \"Accept-Language: ko, en;q=0.9, *;q=0.1\"\n LANGUAGE=ko LANG=en_US.UTF-8 -> \"Accept-Language: ko, *;q=0.1\"\n LANGUAGE= LANG=en_US.UTF-8 -> \"Accept-Language: en-US, *;q=0.1\"\n\nThis gives git servers a chance to display remote error messages in\nthe user's preferred language.\n\nLimit the number of languages to 1,000 because q-value must not be\nsmaller than 0.001, and limit the length of Accept-Language header to\n4,000 bytes for some HTTP servers which cannot accept such long header.\n\nSigned-off-by: Yi EungJun \n---\n http.c | 151 +++++++++++++++++++++++++++++++++++++++++++++\n remote-curl.c | 2 +\n t/t5550-http-fetch-dumb.sh | 42 +++++++++++++\n 3 files changed, 195 insertions(+)\n\ndiff --git a/http.c b/http.c\nindex 040f362..6111c6a 100644\n--- a/http.c\n+++ b/http.c\n@@ -68,6 +68,8 @@ static struct curl_slist *no_pragma_header;\n \n static struct active_request_slot *active_queue_head;\n \n+static char *cached_accept_language;\n+\n size_t fread_buffer(char *ptr, size_t eltsize, size_t nmemb, void *buffer_)\n {\n \tsize_t size = eltsize * nmemb;\n@@ -515,6 +517,9 @@ void http_cleanup(void)\n \t\tcert_auth.password = NULL;\n \t}\n \tssl_cert_password_required = 0;\n+\n+\tfree(cached_accept_language);\n+\tcached_accept_language = NULL;\n }\n \n struct active_request_slot *get_active_slot(void)\n@@ -986,6 +991,146 @@ static void extract_content_type(struct strbuf *raw, struct strbuf *type,\n \t\tstrbuf_addstr(charset, \"ISO-8859-1\");\n }\n \n+/*\n+ * Guess the user's preferred languages from the value in LANGUAGE environment\n+ * variable and LC_MESSAGES locale category if NO_GETTEXT is not defined.\n+ *\n+ * The result can be a colon-separated list like \"ko:ja:en\".\n+ */\n+static const char *get_preferred_languages(void)\n+{\n+\tconst char *retval;\n+\n+\tretval = getenv(\"LANGUAGE\");\n+\tif (retval && *retval)\n+\t\treturn retval;\n+\n+#ifndef NO_GETTEXT\n+\tretval = setlocale(LC_MESSAGES, NULL);\n+\tif (retval && *retval &&\n+\t\tstrcmp(retval, \"C\") &&\n+\t\tstrcmp(retval, \"POSIX\"))\n+\t\treturn retval;\n+#endif\n+\n+\treturn NULL;\n+}\n+\n+static void write_accept_language(struct strbuf *buf)\n+{\n+\t/*\n+\t * MAX_DECIMAL_PLACES must not be larger than 3. If it is larger than\n+\t * that, q-value will be smaller than 0.001, the minimum q-value the\n+\t * HTTP specification allows. See\n+\t * http://tools.ietf.org/html/rfc7231#section-5.3.1 for q-value.\n+\t */\n+\tconst int MAX_DECIMAL_PLACES = 3;\n+\tconst int MAX_LANGUAGE_TAGS = 1000;\n+\tconst int MAX_ACCEPT_LANGUAGE_HEADER_SIZE = 4000;\n+\tstruct strbuf *language_tags = NULL;\n+\tint num_langs = 0;\n+\tconst char *s = get_preferred_languages();\n+\tint i;\n+\tstruct strbuf tag = STRBUF_INIT;\n+\n+\t/* Don't add Accept-Language header if no language is preferred. */\n+\tif (!s)\n+\t\treturn;\n+\n+\t/*\n+\t * Split the colon-separated string of preferred languages into\n+\t * language_tags array.\n+\t */\n+\tdo {\n+\t\t/* collect language tag */\n+\t\tfor (; *s && (isalnum(*s) || *s == '_'); s++)\n+\t\t\tstrbuf_addch(&tag, *s == '_' ? '-' : *s);\n+\n+\t\t/* skip .codeset, @modifier and any other unnecessary parts */\n+\t\twhile (*s && *s != ':')\n+\t\t\ts++;\n+\n+\t\tif (tag.len) {\n+\t\t\tnum_langs++;\n+\t\t\tREALLOC_ARRAY(language_tags, num_langs);\n+\t\t\tstrbuf_init(&language_tags[num_langs - 1], 0);\n+\t\t\tstrbuf_swap(&tag, &language_tags[num_langs - 1]);\n+\n+\t\t\tif (num_langs >= MAX_LANGUAGE_TAGS - 1) /* -1 for '*' */\n+\t\t\t\tbreak;\n+\t\t}\n+\t} while (*s++);\n+\n+\t/* write Accept-Language header into buf */\n+\tif (num_langs >= 1) {\n+\t\tint last_buf_len = 0;\n+\t\tint max_q;\n+\t\tint decimal_places;\n+\t\tchar q_format[32];\n+\n+\t\t/* add '*' */\n+\t\tREALLOC_ARRAY(language_tags, num_langs + 1);\n+\t\tstrbuf_init(&language_tags[num_langs], 0);\n+\t\tstrbuf_addstr(&language_tags[num_langs++], \"*\");\n+\n+\t\t/* compute decimal_places */\n+\t\tfor (max_q = 1, decimal_places = 0;\n+\t\t\t\tmax_q < num_langs && decimal_places <= MAX_DECIMAL_PLACES;\n+\t\t\t\tdecimal_places++, max_q *= 10)\n+\t\t\t;\n+\n+\t\tsprintf(q_format, \";q=0.%%0%dd\", decimal_places);\n+\n+\t\tstrbuf_addstr(buf, \"Accept-Language: \");\n+\n+\t\tfor(i = 0; i < num_langs; i++) {\n+\t\t\tif (i > 0)\n+\t\t\t\tstrbuf_addstr(buf, \", \");\n+\n+\t\t\tstrbuf_addstr(buf, strbuf_detach(&language_tags[i], NULL));\n+\n+\t\t\tif (i > 0)\n+\t\t\t\tstrbuf_addf(buf, q_format, max_q - i);\n+\n+\t\t\tif (buf->len > MAX_ACCEPT_LANGUAGE_HEADER_SIZE) {\n+\t\t\t\tstrbuf_remove(buf, last_buf_len, buf->len - last_buf_len);\n+\t\t\t\tbreak;\n+\t\t\t}\n+\n+\t\t\tlast_buf_len = buf->len;\n+\t\t}\n+\t}\n+\n+\t/* free language tags */\n+\tfor(i = 0; i < num_langs; i++) {\n+\t\tstrbuf_release(&language_tags[i]);\n+\t}\n+\tfree(language_tags);\n+}\n+\n+/*\n+ * Get an Accept-Language header which indicates user's preferred languages.\n+ *\n+ * Examples:\n+ * LANGUAGE= -> \"\"\n+ * LANGUAGE=ko:en -> \"Accept-Language: ko, en; q=0.9, *; q=0.1\"\n+ * LANGUAGE=ko_KR.UTF-8:sr@latin -> \"Accept-Language: ko-KR, sr; q=0.9, *; q=0.1\"\n+ * LANGUAGE=ko LANG=en_US.UTF-8 -> \"Accept-Language: ko, *; q=0.1\"\n+ * LANGUAGE= LANG=en_US.UTF-8 -> \"Accept-Language: en-US, *; q=0.1\"\n+ * LANGUAGE= LANG=C -> \"\"\n+ */\n+static const char *get_accept_language(void)\n+{\n+\tif (!cached_accept_language) {\n+\t\tstruct strbuf buf = STRBUF_INIT;\n+\t\twrite_accept_language(&buf);\n+\t\tif (buf.len > 0)\n+\t\t\tcached_accept_language = strbuf_detach(&buf, NULL);\n+\t}\n+\n+\treturn cached_accept_language;\n+}\n+\n /* http_request() targets */\n #define HTTP_REQUEST_STRBUF\t0\n #define HTTP_REQUEST_FILE\t1\n@@ -998,6 +1143,7 @@ static int http_request(const char *url,\n \tstruct slot_results results;\n \tstruct curl_slist *headers = NULL;\n \tstruct strbuf buf = STRBUF_INIT;\n+\tconst char *accept_language;\n \tint ret;\n \n \tslot = get_active_slot();\n@@ -1023,6 +1169,11 @@ static int http_request(const char *url,\n \t\t\t\t\t fwrite_buffer);\n \t}\n \n+\taccept_language = get_accept_language();\n+\n+\tif (accept_language)\n+\t\theaders = curl_slist_append(headers, accept_language);\n+\n \tstrbuf_addstr(&buf, \"Pragma:\");\n \tif (options && options->no_cache)\n \t\tstrbuf_addstr(&buf, \" no-cache\");\ndiff --git a/remote-curl.c b/remote-curl.c\nindex dd63bc2..04989e5 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -962,6 +962,8 @@ int main(int argc, const char **argv)\n \tstruct strbuf buf = STRBUF_INIT;\n \tint nongit;\n \n+\tgit_setup_gettext();\n+\n \tgit_extract_argv0_path(argv[0]);\n \tsetup_git_directory_gently(&nongit);\n \tif (argc < 2) {\ndiff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh\nindex ac71418..e1e2938 100755\n--- a/t/t5550-http-fetch-dumb.sh\n+++ b/t/t5550-http-fetch-dumb.sh\n@@ -196,5 +196,47 @@ test_expect_success 'reencoding is robust to whitespace oddities' '\n \tgrep \"this is the error message\" stderr\n '\n \n+check_language () {\n+\tcase \"$2\" in\n+\t'')\n+\t\t>expect\n+\t\t;;\n+\t?*)\n+\t\techo \"Accept-Language: $1\" >expect\n+\t\t;;\n+\tesac &&\n+\tGIT_CURL_VERBOSE=1 \\\n+\tLANGUAGE=$2 \\\n+\tgit ls-remote \"$HTTPD_URL/dumb/repo.git\" >output 2>&1 &&\n+\ttr -d '\\015' actual &&\n+\ttest_cmp expect actual\n+}\n+\n+test_expect_success 'git client sends Accept-Language based on LANGUAGE' '\n+\tcheck_language \"ko-KR, *;q=0.9\" ko_KR.UTF-8'\n+\n+test_expect_success 'git client sends Accept-Language correctly with unordinary LANGUAGE' '\n+\tcheck_language \"ko-KR, *;q=0.9\" \"ko_KR:\" &&\n+\tcheck_language \"ko-KR, en-US;q=0.9, *;q=0.8\" \"ko_KR::en_US\" &&\n+\tcheck_language \"ko-KR, *;q=0.9\" \":::ko_KR\" &&\n+\tcheck_language \"ko-KR, en-US;q=0.9, *;q=0.8\" \"ko_KR!!:en_US\" &&\n+\tcheck_language \"ko-KR, ja-JP;q=0.9, *;q=0.8\" \"ko_KR en_US:ja_JP\"'\n+\n+test_expect_success 'git client sends Accept-Language with many preferred languages' '\n+\tcheck_language \"ko-KR, en-US;q=0.9, fr-CA;q=0.8, de;q=0.7, sr;q=0.6, \\\n+ja;q=0.5, zh;q=0.4, sv;q=0.3, pt;q=0.2, *;q=0.1\" \\\n+\t\tko_KR.EUC-KR:en_US.UTF-8:fr_CA:de.UTF-8@euro:sr@latin:ja:zh:sv:pt &&\n+\tcheck_language \"ko-KR, en-US;q=0.99, fr-CA;q=0.98, de;q=0.97, sr;q=0.96, \\\n+ja;q=0.95, zh;q=0.94, sv;q=0.93, pt;q=0.92, nb;q=0.91, *;q=0.90\" \\\n+\t\tko_KR.EUC-KR:en_US.UTF-8:fr_CA:de.UTF-8@euro:sr@latin:ja:zh:sv:pt:nb\n+'\n+\n+test_expect_success 'git client does not send an empty Accept-Language' '\n+\tGIT_CURL_VERBOSE=1 LANGUAGE= git ls-remote \"$HTTPD_URL/dumb/repo.git\" 2>stderr &&\n+\t! grep \"^Accept-Language:\" stderr\n+'\n+\n stop_httpd\n test_done\n-- \n2.3.0.rc1.32.ga3df1c7\n"},{"id":"255353","messageId":"xmqqwq47iyrn.fsf@gitster.dls.corp.google.com","threadId":"37174","inReplyTo":"1422373918-14132-2-git-send-email-eungjun.yi@navercorp.com","subject":"Re: [PATCH] http: Add Accept-Language header if possible","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-01-27T23:34:20Z","receivedAt":"2015-01-27T23:34:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Yi EungJun writes:\n\n> +static void write_accept_language(struct strbuf *buf)\n> +{\n> +\t/*\n> +\t * MAX_DECIMAL_PLACES must not be larger than 3. If it is larger than\n> +\t * that, q-value will be smaller than 0.001, the minimum q-value the\n> +\t * HTTP specification allows. See\n> +\t * http://tools.ietf.org/html/rfc7231#section-5.3.1 for q-value.\n> +\t */\n> +\tconst int MAX_DECIMAL_PLACES = 3;\n> +\tconst int MAX_LANGUAGE_TAGS = 1000;\n> +\tconst int MAX_ACCEPT_LANGUAGE_HEADER_SIZE = 4000;\n> +\tstruct strbuf *language_tags = NULL;\n> +\tint num_langs = 0;\n> +\tconst char *s = get_preferred_languages();\n> +\tint i;\n> +\tstruct strbuf tag = STRBUF_INIT;\n> +\n> +\t/* Don't add Accept-Language header if no language is preferred. */\n> +\tif (!s)\n> +\t\treturn;\n> +\n> +\t/*\n> +\t * Split the colon-separated string of preferred languages into\n> +\t * language_tags array.\n> +\t */\n> +\tdo {\n> +\t\t/* collect language tag */\n> +\t\tfor (; *s && (isalnum(*s) || *s == '_'); s++)\n> +\t\t\tstrbuf_addch(&tag, *s == '_' ? '-' : *s);\n> +\n> +\t\t/* skip .codeset, @modifier and any other unnecessary parts */\n> +\t\twhile (*s && *s != ':')\n> +\t\t\ts++;\n> +\n> +\t\tif (tag.len) {\n> +\t\t\tnum_langs++;\n> +\t\t\tREALLOC_ARRAY(language_tags, num_langs);\n> +\t\t\tstrbuf_init(&language_tags[num_langs - 1], 0);\n> +\t\t\tstrbuf_swap(&tag, &language_tags[num_langs - 1]);\n> +\n> +\t\t\tif (num_langs >= MAX_LANGUAGE_TAGS - 1) /* -1 for '*' */\n> +\t\t\t\tbreak;\n> +\t\t}\n> +\t} while (*s++);\n\nThe structure of this function is much easier to understand than any\nof the previous rounds. You collect up to the max you are going to\nsupport, and then you format up to the max you are going to send.\n\nVery straight-forward and simple.\n\n> +\t/* write Accept-Language header into buf */\n> +\tif (num_langs >= 1) {\n\nmicronit: should be OK to just say \"if (num_langs)\".\n\n> +\t\tint last_buf_len = 0;\n> +\t\tint max_q;\n> +\t\tint decimal_places;\n> +\t\tchar q_format[32];\n> +\n> +\t\t/* add '*' */\n> +\t\tREALLOC_ARRAY(language_tags, num_langs + 1);\n> +\t\tstrbuf_init(&language_tags[num_langs], 0);\n> +\t\tstrbuf_addstr(&language_tags[num_langs++], \"*\");\n> +\n> +\t\t/* compute decimal_places */\n> +\t\tfor (max_q = 1, decimal_places = 0;\n> +\t\t\t\tmax_q < num_langs && decimal_places <= MAX_DECIMAL_PLACES;\n> +\t\t\t\tdecimal_places++, max_q *= 10)\n> +\t\t\t;\n\nmicronit: the second and the third line are indented too deeply and\nmade me wonder if this has an overlong first line (i.e. the set-up\npart to enter the for-loop) split into multiple lines.\n\n> +\n> +\t\tsprintf(q_format, \";q=0.%%0%dd\", decimal_places);\n> +\n> +\t\tstrbuf_addstr(buf, \"Accept-Language: \");\n> +\n> +\t\tfor(i = 0; i < num_langs; i++) {\n> +\t\t\tif (i > 0)\n> +\t\t\t\tstrbuf_addstr(buf, \", \");\n> +\n> +\t\t\tstrbuf_addstr(buf, strbuf_detach(&language_tags[i], NULL));\n\nThis is not wrong per-se, but it looks somewhat convoluted to me.\n\nYou can just peek language_tags[i].buf here without detaching, as\nyou will free the strbufs after you exit the loop anyway. Your\nversion is not wrong and it does not result in double-freeing,\nbecause detach clears the strbuf, but at the same time, makes the\nresponsibility to free language_tags[] strbuf split into here (for\nelements up to the ones that are used to fill buf) and the cleanup\nloop (for elements that are not used in this loop).\n\n> +\t\t\tif (i > 0)\n> +\t\t\t\tstrbuf_addf(buf, q_format, max_q - i);\n> +\n> +\t\t\tif (buf->len > MAX_ACCEPT_LANGUAGE_HEADER_SIZE) {\n> +\t\t\t\tstrbuf_remove(buf, last_buf_len, buf->len - last_buf_len);\n> +\t\t\t\tbreak;\n> +\t\t\t}\n> +\n> +\t\t\tlast_buf_len = buf->len;\n> +\t\t}\n> +\t}\n> +\n> +\t/* free language tags */\n> +\tfor(i = 0; i < num_langs; i++) {\n> +\t\tstrbuf_release(&language_tags[i]);\n> +\t}\n> +\tfree(language_tags);\n> +}\n\nI am wondering if using strbuf for each of the language_tags[] is\neven necessary. How about doing it this way instead?\n\n http.c | 22 +++++++++-------------\n 1 file changed, 9 insertions(+), 13 deletions(-)\n\ndiff --git a/http.c b/http.c\nindex 6111c6a..db591b3 100644\n--- a/http.c\n+++ b/http.c\n@@ -1027,7 +1027,7 @@ static void write_accept_language(struct strbuf *buf)\n \tconst int MAX_DECIMAL_PLACES = 3;\n \tconst int MAX_LANGUAGE_TAGS = 1000;\n \tconst int MAX_ACCEPT_LANGUAGE_HEADER_SIZE = 4000;\n-\tstruct strbuf *language_tags = NULL;\n+\tchar **language_tags = NULL;\n \tint num_langs = 0;\n \tconst char *s = get_preferred_languages();\n \tint i;\n@@ -1053,9 +1053,7 @@ static void write_accept_language(struct strbuf *buf)\n \t\tif (tag.len) {\n \t\t\tnum_langs++;\n \t\t\tREALLOC_ARRAY(language_tags, num_langs);\n-\t\t\tstrbuf_init(&language_tags[num_langs - 1], 0);\n-\t\t\tstrbuf_swap(&tag, &language_tags[num_langs - 1]);\n-\n+\t\t\tlanguage_tags[num_langs - 1] = strbuf_detach(&tag, NULL);\n \t\t\tif (num_langs >= MAX_LANGUAGE_TAGS - 1) /* -1 for '*' */\n \t\t\t\tbreak;\n \t\t}\n@@ -1070,13 +1068,12 @@ static void write_accept_language(struct strbuf *buf)\n \n \t\t/* add '*' */\n \t\tREALLOC_ARRAY(language_tags, num_langs + 1);\n-\t\tstrbuf_init(&language_tags[num_langs], 0);\n-\t\tstrbuf_addstr(&language_tags[num_langs++], \"*\");\n+\t\tlanguage_tags[num_langs++] = \"*\"; /* it's OK; this won't be freed */\n \n \t\t/* compute decimal_places */\n \t\tfor (max_q = 1, decimal_places = 0;\n-\t\t\t\tmax_q < num_langs && decimal_places <= MAX_DECIMAL_PLACES;\n-\t\t\t\tdecimal_places++, max_q *= 10)\n+\t\t max_q < num_langs && decimal_places <= MAX_DECIMAL_PLACES;\n+\t\t decimal_places++, max_q *= 10)\n \t\t\t;\n \n \t\tsprintf(q_format, \";q=0.%%0%dd\", decimal_places);\n@@ -1087,7 +1084,7 @@ static void write_accept_language(struct strbuf *buf)\n \t\t\tif (i > 0)\n \t\t\t\tstrbuf_addstr(buf, \", \");\n \n-\t\t\tstrbuf_addstr(buf, strbuf_detach(&language_tags[i], NULL));\n+\t\t\tstrbuf_addstr(buf, language_tags[i]);\n \n \t\t\tif (i > 0)\n \t\t\t\tstrbuf_addf(buf, q_format, max_q - i);\n@@ -1101,10 +1098,9 @@ static void write_accept_language(struct strbuf *buf)\n \t\t}\n \t}\n \n-\t/* free language tags */\n-\tfor(i = 0; i < num_langs; i++) {\n-\t\tstrbuf_release(&language_tags[i]);\n-\t}\n+\t/* free language tags -- last one is a static '*' */\n+\tfor(i = 0; i < num_langs - 1; i++)\n+\t\tfree(language_tags[i]);\n \tfree(language_tags);\n }\n \n"},{"id":"255359","messageId":"CAPc5daXEFZ+3Qr8fg0g9Mi6V+3r5yNmAFpAwVXciaMTwK244kg@mail.gmail.com","threadId":"37174","inReplyTo":"xmqqwq47iyrn.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] http: Add Accept-Language header if possible","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-01-28T06:15:48Z","receivedAt":"2015-01-28T06:15:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"On Tue, Jan 27, 2015 at 3:34 PM, Junio C Hamano wrote:\n> Yi EungJun writes:\n>\n>> +\n>> + sprintf(q_format, \";q=0.%%0%dd\", decimal_places);\n>> +\n>> + strbuf_addstr(buf, \"Accept-Language: \");\n>> +\n>> + for(i = 0; i < num_langs; i++) {\n>> + if (i > 0)\n>> + strbuf_addstr(buf, \", \");\n>> +\n>> + strbuf_addstr(buf, strbuf_detach(&language_tags[i], NULL));\n>\n> This is not wrong per-se, but it looks somewhat convoluted to me.\n> ...\n\nActually, this is wrong, isn't it?\n\nstrbuf_detach() removes the language_tags[i].buf from the strbuf,\nand the caller now owns that piece of memory. Then strbuf_addstr()\nappends a copy of that string to buf, and the piece of memory\nthat was originally held by language_tags[i].buf is now lost forever.\n\nThis is leaking.\n\n>> + /* free language tags */\n>> + for(i = 0; i < num_langs; i++) {\n>> + strbuf_release(&language_tags[i]);\n>> + }\n\n... because this loop does not free memory for earlier parts of language_tags[].\n\n> I am wondering if using strbuf for each of the language_tags[] is\n> even necessary. How about doing it this way instead?\n\nAnd I think my counter-proposal does not leak (as it does not us strbuf for\nlanguage_tags[] anymore).\n\n>\n> http.c | 22 +++++++++-------------\n> 1 file changed, 9 insertions(+), 13 deletions(-)\n>\n> diff --git a/http.c b/http.c\n> index 6111c6a..db591b3 100644\n> --- a/http.c\n> +++ b/http.c\n> @@ -1027,7 +1027,7 @@ static void write_accept_language(struct strbuf *buf)\n> const int MAX_DECIMAL_PLACES = 3;\n> const int MAX_LANGUAGE_TAGS = 1000;\n> const int MAX_ACCEPT_LANGUAGE_HEADER_SIZE = 4000;\n> - struct strbuf *language_tags = NULL;\n> + char **language_tags = NULL;\n> int num_langs = 0;\n> const char *s = get_preferred_languages();\n> int i;\n> @@ -1053,9 +1053,7 @@ static void write_accept_language(struct strbuf *buf)\n> if (tag.len) {\n> num_langs++;\n> REALLOC_ARRAY(language_tags, num_langs);\n> - strbuf_init(&language_tags[num_langs - 1], 0);\n> - strbuf_swap(&tag, &language_tags[num_langs - 1]);\n> -\n> + language_tags[num_langs - 1] = strbuf_detach(&tag, NULL);\n> if (num_langs >= MAX_LANGUAGE_TAGS - 1) /* -1 for '*' */\n> break;\n> }\n> @@ -1070,13 +1068,12 @@ static void write_accept_language(struct strbuf *buf)\n>\n> /* add '*' */\n> REALLOC_ARRAY(language_tags, num_langs + 1);\n> - strbuf_init(&language_tags[num_langs], 0);\n> - strbuf_addstr(&language_tags[num_langs++], \"*\");\n> + language_tags[num_langs++] = \"*\"; /* it's OK; this won't be freed */\n>\n> /* compute decimal_places */\n> for (max_q = 1, decimal_places = 0;\n> - max_q < num_langs && decimal_places <= MAX_DECIMAL_PLACES;\n> - decimal_places++, max_q *= 10)\n> + max_q < num_langs && decimal_places <= MAX_DECIMAL_PLACES;\n> + decimal_places++, max_q *= 10)\n> ;\n>\n> sprintf(q_format, \";q=0.%%0%dd\", decimal_places);\n> @@ -1087,7 +1084,7 @@ static void write_accept_language(struct strbuf *buf)\n> if (i > 0)\n> strbuf_addstr(buf, \", \");\n>\n> - strbuf_addstr(buf, strbuf_detach(&language_tags[i], NULL));\n> + strbuf_addstr(buf, language_tags[i]);\n>\n> if (i > 0)\n> strbuf_addf(buf, q_format, max_q - i);\n> @@ -1101,10 +1098,9 @@ static void write_accept_language(struct strbuf *buf)\n> }\n> }\n>\n> - /* free language tags */\n> - for(i = 0; i < num_langs; i++) {\n> - strbuf_release(&language_tags[i]);\n> - }\n> + /* free language tags -- last one is a static '*' */\n> + for(i = 0; i < num_langs - 1; i++)\n> + free(language_tags[i]);\n> free(language_tags);\n> }\n>\n"},{"id":"255387","messageId":"CAFT+Tg9Pkv1m-Lq-gs-N1t5OEqqVACEj_LP=x8PkcJ6afQ3nYg@mail.gmail.com","threadId":"37174","inReplyTo":"CAPc5daXEFZ+3Qr8fg0g9Mi6V+3r5yNmAFpAwVXciaMTwK244kg@mail.gmail.com","subject":"Re: [PATCH] http: Add Accept-Language header if possible","fromName":"Yi, EungJun","fromEmail":"semtlenori@gmail.com","sentAt":"2015-01-28T11:59:28Z","receivedAt":"2015-01-28T11:59:28Z","isPatch":true,"sender":{"key":"semtlenori@gmail.com","avatar":"https://gravatar.com/avatar/8363435d2badb3450df0dd7c4ec2113f6e1d62c44dd3cf7dfe83c5a389b8a9bf?d=mp&s=160"},"body":"I agree that a list of char* is enough for language_tags.\n\nThanks for your review and patch. I'll apply your patch and send v9.\n\nOn Wed, Jan 28, 2015 at 3:15 PM, Junio C Hamano wrote:\n> On Tue, Jan 27, 2015 at 3:34 PM, Junio C Hamano wrote:\n>> Yi EungJun writes:\n>>\n>>> +\n>>> + sprintf(q_format, \";q=0.%%0%dd\", decimal_places);\n>>> +\n>>> + strbuf_addstr(buf, \"Accept-Language: \");\n>>> +\n>>> + for(i = 0; i < num_langs; i++) {\n>>> + if (i > 0)\n>>> + strbuf_addstr(buf, \", \");\n>>> +\n>>> + strbuf_addstr(buf, strbuf_detach(&language_tags[i], NULL));\n>>\n>> This is not wrong per-se, but it looks somewhat convoluted to me.\n>> ...\n>\n> Actually, this is wrong, isn't it?\n>\n> strbuf_detach() removes the language_tags[i].buf from the strbuf,\n> and the caller now owns that piece of memory. Then strbuf_addstr()\n> appends a copy of that string to buf, and the piece of memory\n> that was originally held by language_tags[i].buf is now lost forever.\n>\n> This is leaking.\n>\n>>> + /* free language tags */\n>>> + for(i = 0; i < num_langs; i++) {\n>>> + strbuf_release(&language_tags[i]);\n>>> + }\n>\n> ... because this loop does not free memory for earlier parts of language_tags[].\n>\n>> I am wondering if using strbuf for each of the language_tags[] is\n>> even necessary. How about doing it this way instead?\n>\n> And I think my counter-proposal does not leak (as it does not us strbuf for\n> language_tags[] anymore).\n>\n>>\n>> http.c | 22 +++++++++-------------\n>> 1 file changed, 9 insertions(+), 13 deletions(-)\n>>\n>> diff --git a/http.c b/http.c\n>> index 6111c6a..db591b3 100644\n>> --- a/http.c\n>> +++ b/http.c\n>> @@ -1027,7 +1027,7 @@ static void write_accept_language(struct strbuf *buf)\n>> const int MAX_DECIMAL_PLACES = 3;\n>> const int MAX_LANGUAGE_TAGS = 1000;\n>> const int MAX_ACCEPT_LANGUAGE_HEADER_SIZE = 4000;\n>> - struct strbuf *language_tags = NULL;\n>> + char **language_tags = NULL;\n>> int num_langs = 0;\n>> const char *s = get_preferred_languages();\n>> int i;\n>> @@ -1053,9 +1053,7 @@ static void write_accept_language(struct strbuf *buf)\n>> if (tag.len) {\n>> num_langs++;\n>> REALLOC_ARRAY(language_tags, num_langs);\n>> - strbuf_init(&language_tags[num_langs - 1], 0);\n>> - strbuf_swap(&tag, &language_tags[num_langs - 1]);\n>> -\n>> + language_tags[num_langs - 1] = strbuf_detach(&tag, NULL);\n>> if (num_langs >= MAX_LANGUAGE_TAGS - 1) /* -1 for '*' */\n>> break;\n>> }\n>> @@ -1070,13 +1068,12 @@ static void write_accept_language(struct strbuf *buf)\n>>\n>> /* add '*' */\n>> REALLOC_ARRAY(language_tags, num_langs + 1);\n>> - strbuf_init(&language_tags[num_langs], 0);\n>> - strbuf_addstr(&language_tags[num_langs++], \"*\");\n>> + language_tags[num_langs++] = \"*\"; /* it's OK; this won't be freed */\n>>\n>> /* compute decimal_places */\n>> for (max_q = 1, decimal_places = 0;\n>> - max_q < num_langs && decimal_places <= MAX_DECIMAL_PLACES;\n>> - decimal_places++, max_q *= 10)\n>> + max_q < num_langs && decimal_places <= MAX_DECIMAL_PLACES;\n>> + decimal_places++, max_q *= 10)\n>> ;\n>>\n>> sprintf(q_format, \";q=0.%%0%dd\", decimal_places);\n>> @@ -1087,7 +1084,7 @@ static void write_accept_language(struct strbuf *buf)\n>> if (i > 0)\n>> strbuf_addstr(buf, \", \");\n>>\n>> - strbuf_addstr(buf, strbuf_detach(&language_tags[i], NULL));\n>> + strbuf_addstr(buf, language_tags[i]);\n>>\n>> if (i > 0)\n>> strbuf_addf(buf, q_format, max_q - i);\n>> @@ -1101,10 +1098,9 @@ static void write_accept_language(struct strbuf *buf)\n>> }\n>> }\n>>\n>> - /* free language tags */\n>> - for(i = 0; i < num_langs; i++) {\n>> - strbuf_release(&language_tags[i]);\n>> - }\n>> + /* free language tags -- last one is a static '*' */\n>> + for(i = 0; i < num_langs - 1; i++)\n>> + free(language_tags[i]);\n>> free(language_tags);\n>> }\n>>\n"},{"id":"255388","messageId":"1422446677-8415-1-git-send-email-eungjun.yi@navercorp.com","threadId":"37174","inReplyTo":"CAPc5daXEFZ+3Qr8fg0g9Mi6V+3r5yNmAFpAwVXciaMTwK244kg@mail.gmail.com","subject":"[PATCH v9 0/1] http: Add Accept-Language header if possible","fromName":"Yi EungJun","fromEmail":"semtlenori@gmail.com","sentAt":"2015-01-28T12:04:36Z","receivedAt":"2015-01-28T12:04:36Z","isPatch":true,"sender":{"key":"semtlenori@gmail.com","avatar":"https://gravatar.com/avatar/8363435d2badb3450df0dd7c4ec2113f6e1d62c44dd3cf7dfe83c5a389b8a9bf?d=mp&s=160"},"body":"From: Yi EungJun \n\nChange since v8\n\nApply Junio's patch: Use an array of char* instead of strbuf for language_tags.\n\nYi EungJun (1):\n http: Add Accept-Language header if possible\n\n http.c | 147 +++++++++++++++++++++++++++++++++++++++++++++\n remote-curl.c | 2 +\n t/t5550-http-fetch-dumb.sh | 42 +++++++++++++\n 3 files changed, 191 insertions(+)\n\n-- \n2.3.0.rc1.32.g7a36c04\n"},{"id":"255379","messageId":"1422446677-8415-2-git-send-email-eungjun.yi@navercorp.com","threadId":"37174","inReplyTo":"1422446677-8415-1-git-send-email-eungjun.yi@navercorp.com","subject":"[PATCH v9 1/1] http: Add Accept-Language header if possible","fromName":"Yi EungJun","fromEmail":"semtlenori@gmail.com","sentAt":"2015-01-28T12:04:37Z","receivedAt":"2015-01-28T12:04:37Z","isPatch":true,"sender":{"key":"semtlenori@gmail.com","avatar":"https://gravatar.com/avatar/8363435d2badb3450df0dd7c4ec2113f6e1d62c44dd3cf7dfe83c5a389b8a9bf?d=mp&s=160"},"body":"From: Yi EungJun \n\nAdd an Accept-Language header which indicates the user's preferred\nlanguages defined by $LANGUAGE, $LC_ALL, $LC_MESSAGES and $LANG.\n\nExamples:\n LANGUAGE= -> \"\"\n LANGUAGE=ko:en -> \"Accept-Language: ko, en;q=0.9, *;q=0.1\"\n LANGUAGE=ko LANG=en_US.UTF-8 -> \"Accept-Language: ko, *;q=0.1\"\n LANGUAGE= LANG=en_US.UTF-8 -> \"Accept-Language: en-US, *;q=0.1\"\n\nThis gives git servers a chance to display remote error messages in\nthe user's preferred language.\n\nLimit the number of languages to 1,000 because q-value must not be\nsmaller than 0.001, and limit the length of Accept-Language header to\n4,000 bytes for some HTTP servers which cannot accept such long header.\n\nSigned-off-by: Yi EungJun \n---\n http.c | 147 +++++++++++++++++++++++++++++++++++++++++++++\n remote-curl.c | 2 +\n t/t5550-http-fetch-dumb.sh | 42 +++++++++++++\n 3 files changed, 191 insertions(+)\n\ndiff --git a/http.c b/http.c\nindex 040f362..b2ad2a8 100644\n--- a/http.c\n+++ b/http.c\n@@ -68,6 +68,8 @@ static struct curl_slist *no_pragma_header;\n \n static struct active_request_slot *active_queue_head;\n \n+static char *cached_accept_language;\n+\n size_t fread_buffer(char *ptr, size_t eltsize, size_t nmemb, void *buffer_)\n {\n \tsize_t size = eltsize * nmemb;\n@@ -515,6 +517,9 @@ void http_cleanup(void)\n \t\tcert_auth.password = NULL;\n \t}\n \tssl_cert_password_required = 0;\n+\n+\tfree(cached_accept_language);\n+\tcached_accept_language = NULL;\n }\n \n struct active_request_slot *get_active_slot(void)\n@@ -986,6 +991,142 @@ static void extract_content_type(struct strbuf *raw, struct strbuf *type,\n \t\tstrbuf_addstr(charset, \"ISO-8859-1\");\n }\n \n+/*\n+ * Guess the user's preferred languages from the value in LANGUAGE environment\n+ * variable and LC_MESSAGES locale category if NO_GETTEXT is not defined.\n+ *\n+ * The result can be a colon-separated list like \"ko:ja:en\".\n+ */\n+static const char *get_preferred_languages(void)\n+{\n+\tconst char *retval;\n+\n+\tretval = getenv(\"LANGUAGE\");\n+\tif (retval && *retval)\n+\t\treturn retval;\n+\n+#ifndef NO_GETTEXT\n+\tretval = setlocale(LC_MESSAGES, NULL);\n+\tif (retval && *retval &&\n+\t\tstrcmp(retval, \"C\") &&\n+\t\tstrcmp(retval, \"POSIX\"))\n+\t\treturn retval;\n+#endif\n+\n+\treturn NULL;\n+}\n+\n+static void write_accept_language(struct strbuf *buf)\n+{\n+\t/*\n+\t * MAX_DECIMAL_PLACES must not be larger than 3. If it is larger than\n+\t * that, q-value will be smaller than 0.001, the minimum q-value the\n+\t * HTTP specification allows. See\n+\t * http://tools.ietf.org/html/rfc7231#section-5.3.1 for q-value.\n+\t */\n+\tconst int MAX_DECIMAL_PLACES = 3;\n+\tconst int MAX_LANGUAGE_TAGS = 1000;\n+\tconst int MAX_ACCEPT_LANGUAGE_HEADER_SIZE = 4000;\n+\tchar **language_tags = NULL;\n+\tint num_langs = 0;\n+\tconst char *s = get_preferred_languages();\n+\tint i;\n+\tstruct strbuf tag = STRBUF_INIT;\n+\n+\t/* Don't add Accept-Language header if no language is preferred. */\n+\tif (!s)\n+\t\treturn;\n+\n+\t/*\n+\t * Split the colon-separated string of preferred languages into\n+\t * language_tags array.\n+\t */\n+\tdo {\n+\t\t/* collect language tag */\n+\t\tfor (; *s && (isalnum(*s) || *s == '_'); s++)\n+\t\t\tstrbuf_addch(&tag, *s == '_' ? '-' : *s);\n+\n+\t\t/* skip .codeset, @modifier and any other unnecessary parts */\n+\t\twhile (*s && *s != ':')\n+\t\t\ts++;\n+\n+\t\tif (tag.len) {\n+\t\t\tnum_langs++;\n+\t\t\tREALLOC_ARRAY(language_tags, num_langs);\n+\t\t\tlanguage_tags[num_langs - 1] = strbuf_detach(&tag, NULL);\n+\t\t\tif (num_langs >= MAX_LANGUAGE_TAGS - 1) /* -1 for '*' */\n+\t\t\t\tbreak;\n+\t\t}\n+\t} while (*s++);\n+\n+\t/* write Accept-Language header into buf */\n+\tif (num_langs) {\n+\t\tint last_buf_len = 0;\n+\t\tint max_q;\n+\t\tint decimal_places;\n+\t\tchar q_format[32];\n+\n+\t\t/* add '*' */\n+\t\tREALLOC_ARRAY(language_tags, num_langs + 1);\n+\t\tlanguage_tags[num_langs++] = \"*\"; /* it's OK; this won't be freed */\n+\n+\t\t/* compute decimal_places */\n+\t\tfor (max_q = 1, decimal_places = 0;\n+\t\t max_q < num_langs && decimal_places <= MAX_DECIMAL_PLACES;\n+\t\t decimal_places++, max_q *= 10)\n+\t\t\t;\n+\n+\t\tsprintf(q_format, \";q=0.%%0%dd\", decimal_places);\n+\n+\t\tstrbuf_addstr(buf, \"Accept-Language: \");\n+\n+\t\tfor(i = 0; i < num_langs; i++) {\n+\t\t\tif (i > 0)\n+\t\t\t\tstrbuf_addstr(buf, \", \");\n+\n+\t\t\tstrbuf_addstr(buf, language_tags[i]);\n+\n+\t\t\tif (i > 0)\n+\t\t\t\tstrbuf_addf(buf, q_format, max_q - i);\n+\n+\t\t\tif (buf->len > MAX_ACCEPT_LANGUAGE_HEADER_SIZE) {\n+\t\t\t\tstrbuf_remove(buf, last_buf_len, buf->len - last_buf_len);\n+\t\t\t\tbreak;\n+\t\t\t}\n+\n+\t\t\tlast_buf_len = buf->len;\n+\t\t}\n+\t}\n+\n+\t/* free language tags -- last one is a static '*' */\n+\tfor(i = 0; i < num_langs - 1; i++)\n+\t\tfree(language_tags[i]);\n+\tfree(language_tags);\n+}\n+\n+/*\n+ * Get an Accept-Language header which indicates user's preferred languages.\n+ *\n+ * Examples:\n+ * LANGUAGE= -> \"\"\n+ * LANGUAGE=ko:en -> \"Accept-Language: ko, en; q=0.9, *; q=0.1\"\n+ * LANGUAGE=ko_KR.UTF-8:sr@latin -> \"Accept-Language: ko-KR, sr; q=0.9, *; q=0.1\"\n+ * LANGUAGE=ko LANG=en_US.UTF-8 -> \"Accept-Language: ko, *; q=0.1\"\n+ * LANGUAGE= LANG=en_US.UTF-8 -> \"Accept-Language: en-US, *; q=0.1\"\n+ * LANGUAGE= LANG=C -> \"\"\n+ */\n+static const char *get_accept_language(void)\n+{\n+\tif (!cached_accept_language) {\n+\t\tstruct strbuf buf = STRBUF_INIT;\n+\t\twrite_accept_language(&buf);\n+\t\tif (buf.len > 0)\n+\t\t\tcached_accept_language = strbuf_detach(&buf, NULL);\n+\t}\n+\n+\treturn cached_accept_language;\n+}\n+\n /* http_request() targets */\n #define HTTP_REQUEST_STRBUF\t0\n #define HTTP_REQUEST_FILE\t1\n@@ -998,6 +1139,7 @@ static int http_request(const char *url,\n \tstruct slot_results results;\n \tstruct curl_slist *headers = NULL;\n \tstruct strbuf buf = STRBUF_INIT;\n+\tconst char *accept_language;\n \tint ret;\n \n \tslot = get_active_slot();\n@@ -1023,6 +1165,11 @@ static int http_request(const char *url,\n \t\t\t\t\t fwrite_buffer);\n \t}\n \n+\taccept_language = get_accept_language();\n+\n+\tif (accept_language)\n+\t\theaders = curl_slist_append(headers, accept_language);\n+\n \tstrbuf_addstr(&buf, \"Pragma:\");\n \tif (options && options->no_cache)\n \t\tstrbuf_addstr(&buf, \" no-cache\");\ndiff --git a/remote-curl.c b/remote-curl.c\nindex dd63bc2..04989e5 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -962,6 +962,8 @@ int main(int argc, const char **argv)\n \tstruct strbuf buf = STRBUF_INIT;\n \tint nongit;\n \n+\tgit_setup_gettext();\n+\n \tgit_extract_argv0_path(argv[0]);\n \tsetup_git_directory_gently(&nongit);\n \tif (argc < 2) {\ndiff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh\nindex ac71418..e1e2938 100755\n--- a/t/t5550-http-fetch-dumb.sh\n+++ b/t/t5550-http-fetch-dumb.sh\n@@ -196,5 +196,47 @@ test_expect_success 'reencoding is robust to whitespace oddities' '\n \tgrep \"this is the error message\" stderr\n '\n \n+check_language () {\n+\tcase \"$2\" in\n+\t'')\n+\t\t>expect\n+\t\t;;\n+\t?*)\n+\t\techo \"Accept-Language: $1\" >expect\n+\t\t;;\n+\tesac &&\n+\tGIT_CURL_VERBOSE=1 \\\n+\tLANGUAGE=$2 \\\n+\tgit ls-remote \"$HTTPD_URL/dumb/repo.git\" >output 2>&1 &&\n+\ttr -d '\\015' actual &&\n+\ttest_cmp expect actual\n+}\n+\n+test_expect_success 'git client sends Accept-Language based on LANGUAGE' '\n+\tcheck_language \"ko-KR, *;q=0.9\" ko_KR.UTF-8'\n+\n+test_expect_success 'git client sends Accept-Language correctly with unordinary LANGUAGE' '\n+\tcheck_language \"ko-KR, *;q=0.9\" \"ko_KR:\" &&\n+\tcheck_language \"ko-KR, en-US;q=0.9, *;q=0.8\" \"ko_KR::en_US\" &&\n+\tcheck_language \"ko-KR, *;q=0.9\" \":::ko_KR\" &&\n+\tcheck_language \"ko-KR, en-US;q=0.9, *;q=0.8\" \"ko_KR!!:en_US\" &&\n+\tcheck_language \"ko-KR, ja-JP;q=0.9, *;q=0.8\" \"ko_KR en_US:ja_JP\"'\n+\n+test_expect_success 'git client sends Accept-Language with many preferred languages' '\n+\tcheck_language \"ko-KR, en-US;q=0.9, fr-CA;q=0.8, de;q=0.7, sr;q=0.6, \\\n+ja;q=0.5, zh;q=0.4, sv;q=0.3, pt;q=0.2, *;q=0.1\" \\\n+\t\tko_KR.EUC-KR:en_US.UTF-8:fr_CA:de.UTF-8@euro:sr@latin:ja:zh:sv:pt &&\n+\tcheck_language \"ko-KR, en-US;q=0.99, fr-CA;q=0.98, de;q=0.97, sr;q=0.96, \\\n+ja;q=0.95, zh;q=0.94, sv;q=0.93, pt;q=0.92, nb;q=0.91, *;q=0.90\" \\\n+\t\tko_KR.EUC-KR:en_US.UTF-8:fr_CA:de.UTF-8@euro:sr@latin:ja:zh:sv:pt:nb\n+'\n+\n+test_expect_success 'git client does not send an empty Accept-Language' '\n+\tGIT_CURL_VERBOSE=1 LANGUAGE= git ls-remote \"$HTTPD_URL/dumb/repo.git\" 2>stderr &&\n+\t! grep \"^Accept-Language:\" stderr\n+'\n+\n stop_httpd\n test_done\n-- \n2.3.0.rc1.32.g7a36c04\n"},{"id":"255397","messageId":"xmqq7fw6f6s6.fsf@gitster.dls.corp.google.com","threadId":"37174","inReplyTo":"1422446677-8415-1-git-send-email-eungjun.yi@navercorp.com","subject":"Re: [PATCH v9 0/1] http: Add Accept-Language header if possible","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-01-29T06:19:21Z","receivedAt":"2015-01-29T06:19:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thanks; queued. Let's run with this and try to make it graduate\nearly next cycle.\n"},{"id":"255439","messageId":"CAFT+Tg_-Ka7qcPfXJd7QC+VwEY6Mgmq5TNvzv3GoLimrqfPnHQ@mail.gmail.com","threadId":"37174","inReplyTo":"xmqq7fw6f6s6.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v9 0/1] http: Add Accept-Language header if possible","fromName":"Yi, EungJun","fromEmail":"semtlenori@gmail.com","sentAt":"2015-01-30T17:23:32Z","receivedAt":"2015-01-30T17:23:32Z","isPatch":true,"sender":{"key":"semtlenori@gmail.com","avatar":"https://gravatar.com/avatar/8363435d2badb3450df0dd7c4ec2113f6e1d62c44dd3cf7dfe83c5a389b8a9bf?d=mp&s=160"},"body":"I'm very glad to hear that. Thanks to all reviewers!\n\nOn Thu, Jan 29, 2015 at 3:19 PM, Junio C Hamano wrote:\n> Thanks; queued. Let's run with this and try to make it graduate\n> early next cycle.\n"},{"id":"256667","messageId":"xmqqpp8xmwnp.fsf@gitster.dls.corp.google.com","threadId":"37174","inReplyTo":"1422446677-8415-2-git-send-email-eungjun.yi@navercorp.com","subject":"Re: [PATCH v9 1/1] http: Add Accept-Language header if possible","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-02-25T22:52:26Z","receivedAt":"2015-02-25T22:52:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Yi EungJun writes:\n\n> From: Yi EungJun \n>\n> Add an Accept-Language header which indicates the user's preferred\n> languages defined by $LANGUAGE, $LC_ALL, $LC_MESSAGES and $LANG.\n> ...\n> Signed-off-by: Yi EungJun \n> ---\n\nYikes.\n\nThis is now in 'master', but I wonder if people are getting\ncompilation errors because of this change. I do.\n\nIt introduces a call to setlocale() without causing to be\nincluded, and runs afoul of -Wimplicit-function-declaration.\n\nOther call sites of setlocale() are in gettext.c, which does include\nthe header at the beginning.\n\n> diff --git a/http.c b/http.c\n> index 040f362..b2ad2a8 100644\n> --- a/http.c\n> +++ b/http.c\n> ...\n> +#ifndef NO_GETTEXT\n> +\tretval = setlocale(LC_MESSAGES, NULL);\n> +\tif (retval && *retval &&\n> +\t\tstrcmp(retval, \"C\") &&\n> +\t\tstrcmp(retval, \"POSIX\"))\n> +\t\treturn retval;\n> +#endif\n\nI really do not like a conditional inclusion of system header files\ninside any *.c file, but here is a minimum emergency fix-up I am\nrunning with today. It should go to somewhere in git-compat-util.h.\n\nSomebody care to throw a tested fix-up patch at me?\n\nThanks.\n\n http.c | 4 ++++\n 1 file changed, 4 insertions(+)\n\ndiff --git a/http.c b/http.c\nindex efdab09..7e79cbd 100644\n--- a/http.c\n+++ b/http.c\n@@ -9,6 +9,10 @@\n #include \"version.h\"\n #include \"pkt-line.h\"\n \n+#ifndef NO_GETTEXT\n+#include \n+#endif\n+\n int active_requests;\n int http_is_verbose;\n size_t http_post_buffer = 16 * LARGE_PACKET_MAX;\n"},{"id":"256668","messageId":"20150226030416.GA6121@peff.net","threadId":"37174","inReplyTo":"xmqqpp8xmwnp.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v9 1/1] http: Add Accept-Language header if possible","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-02-26T03:04:16Z","receivedAt":"2015-02-26T03:04:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 25, 2015 at 02:52:26PM -0800, Junio C Hamano wrote:\n\n> This is now in 'master', but I wonder if people are getting\n> compilation errors because of this change. I do.\n\nI usually compile with NO_GETTEXT, but if I stop doing so, I see the\nproblem, too.\n\n> I really do not like a conditional inclusion of system header files\n> inside any *.c file, but here is a minimum emergency fix-up I am\n> running with today. It should go to somewhere in git-compat-util.h.\n\nPerhaps it would be less risky to stick get_preferred_languages() into\ngettext.c, like the patch below. Then we do not have to worry about\nlocale.h introducing other disruptive includes. The function is not\ntechnically about gettext, but it seems reasonable to me to stuff all of\nthe i18n code together.\n\nAnother variant of this would for gettext.c to provide a git_setlocale\nthat just wraps setlocale (and does nothing when NO_GETTEXT is given).\n\ndiff --git a/gettext.c b/gettext.c\nindex 8b2da46..7378ba2 100644\n--- a/gettext.c\n+++ b/gettext.c\n@@ -18,6 +18,31 @@\n #\tendif\n #endif\n \n+/*\n+ * Guess the user's preferred languages from the value in LANGUAGE environment\n+ * variable and LC_MESSAGES locale category if NO_GETTEXT is not defined.\n+ *\n+ * The result can be a colon-separated list like \"ko:ja:en\".\n+ */\n+const char *get_preferred_languages(void)\n+{\n+\tconst char *retval;\n+\n+\tretval = getenv(\"LANGUAGE\");\n+\tif (retval && *retval)\n+\t\treturn retval;\n+\n+#ifndef NO_GETTEXT\n+\tretval = setlocale(LC_MESSAGES, NULL);\n+\tif (retval && *retval &&\n+\t\tstrcmp(retval, \"C\") &&\n+\t\tstrcmp(retval, \"POSIX\"))\n+\t\treturn retval;\n+#endif\n+\n+\treturn NULL;\n+}\n+\n #ifdef GETTEXT_POISON\n int use_gettext_poison(void)\n {\ndiff --git a/gettext.h b/gettext.h\nindex dc1722d..5d8d2df 100644\n--- a/gettext.h\n+++ b/gettext.h\n@@ -89,4 +89,6 @@ const char *Q_(const char *msgid, const char *plu, unsigned long n)\n #define N_(msgid) (msgid)\n #endif\n \n+const char *get_preferred_languages();\n+\n #endif\ndiff --git a/http.c b/http.c\nindex 0153fb0..9c825af 100644\n--- a/http.c\n+++ b/http.c\n@@ -8,6 +8,7 @@\n #include \"credential.h\"\n #include \"version.h\"\n #include \"pkt-line.h\"\n+#include \"gettext.h\"\n \n int active_requests;\n int http_is_verbose;\n@@ -1002,32 +1003,6 @@ static void extract_content_type(struct strbuf *raw, struct strbuf *type,\n \t\tstrbuf_addstr(charset, \"ISO-8859-1\");\n }\n \n-\n-/*\n- * Guess the user's preferred languages from the value in LANGUAGE environment\n- * variable and LC_MESSAGES locale category if NO_GETTEXT is not defined.\n- *\n- * The result can be a colon-separated list like \"ko:ja:en\".\n- */\n-static const char *get_preferred_languages(void)\n-{\n-\tconst char *retval;\n-\n-\tretval = getenv(\"LANGUAGE\");\n-\tif (retval && *retval)\n-\t\treturn retval;\n-\n-#ifndef NO_GETTEXT\n-\tretval = setlocale(LC_MESSAGES, NULL);\n-\tif (retval && *retval &&\n-\t\tstrcmp(retval, \"C\") &&\n-\t\tstrcmp(retval, \"POSIX\"))\n-\t\treturn retval;\n-#endif\n-\n-\treturn NULL;\n-}\n-\n static void write_accept_language(struct strbuf *buf)\n {\n \t/*\n"},{"id":"256669","messageId":"20150226031029.GA20457@peff.net","threadId":"37174","inReplyTo":"20150226030416.GA6121@peff.net","subject":"Re: [PATCH v9 1/1] http: Add Accept-Language header if possible","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-02-26T03:10:29Z","receivedAt":"2015-02-26T03:10:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 25, 2015 at 10:04:16PM -0500, Jeff King wrote:\n\n> Another variant of this would for gettext.c to provide a git_setlocale\n> that just wraps setlocale (and does nothing when NO_GETTEXT is given).\n\nThis doesn't _quite_ work. In addition to the function, we have to have\nLC_MESSAGES defined. So we cannot provide a straight git_setlocale, but\nit has to be something like \"git_message_locale\". At which point we\nmight as well just provide get_preferred_languages() as the interface\nbetween gettext.c and the rest of the program.\n\n-Peff\n"},{"id":"256708","messageId":"xmqqmw40l777.fsf@gitster.dls.corp.google.com","threadId":"37174","inReplyTo":"20150226030416.GA6121@peff.net","subject":"Re: [PATCH v9 1/1] http: Add Accept-Language header if possible","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-02-26T20:59:56Z","receivedAt":"2015-02-26T20:59:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Perhaps it would be less risky to stick get_preferred_languages() into\n> gettext.c, like the patch below. Then we do not have to worry about\n> locale.h introducing other disruptive includes. The function is not\n> technically about gettext, but it seems reasonable to me to stuff all of\n> the i18n code together.\n\nYeah, I like that a lot better. Thanks.\n"},{"id":"256712","messageId":"20150226213356.GA14464@peff.net","threadId":"37174","inReplyTo":"xmqqmw40l777.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v9 1/1] http: Add Accept-Language header if possible","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-02-26T21:33:57Z","receivedAt":"2015-02-26T21:33:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Feb 26, 2015 at 12:59:56PM -0800, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > Perhaps it would be less risky to stick get_preferred_languages() into\n> > gettext.c, like the patch below. Then we do not have to worry about\n> > locale.h introducing other disruptive includes. The function is not\n> > technically about gettext, but it seems reasonable to me to stuff all of\n> > the i18n code together.\n> \n> Yeah, I like that a lot better. Thanks.\n\nAre you just using it for inspiration, or did you want me to wrap it up\nwith a commit message?\n\n-Peff\n"},{"id":"256715","messageId":"xmqqa900l57y.fsf@gitster.dls.corp.google.com","threadId":"37174","inReplyTo":"20150226213356.GA14464@peff.net","subject":"Re: [PATCH v9 1/1] http: Add Accept-Language header if possible","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-02-26T21:42:41Z","receivedAt":"2015-02-26T21:42:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Thu, Feb 26, 2015 at 12:59:56PM -0800, Junio C Hamano wrote:\n>\n>> Jeff King writes:\n>> \n>> > Perhaps it would be less risky to stick get_preferred_languages() into\n>> > gettext.c, like the patch below. Then we do not have to worry about\n>> > locale.h introducing other disruptive includes. The function is not\n>> > technically about gettext, but it seems reasonable to me to stuff all of\n>> > the i18n code together.\n>> \n>> Yeah, I like that a lot better. Thanks.\n>\n> Are you just using it for inspiration, or did you want me to wrap it up\n> with a commit message?\n\nHere is what I queued. Thanks.\n\n-- >8 --\nFrom: Jeff King \nDate: Wed, 25 Feb 2015 22:04:16 -0500\nSubject: [PATCH] gettext.c: move get_preferred_languages() from http.c\n\nCalling setlocale(LC_MESSAGES, ...) directly from http.c, without\nincluding , was causing compilation warnings. Move the\nhelper function to gettext.c that already includes the header and\nwhere locale-related issues are handled.\n\nSigned-off-by: Jeff King \nSigned-off-by: Junio C Hamano \n---\n gettext.c | 25 +++++++++++++++++++++++++\n gettext.h | 2 ++\n http.c | 1 +\n 3 files changed, 28 insertions(+)\n\ndiff --git a/gettext.c b/gettext.c\nindex 8b2da46..7378ba2 100644\n--- a/gettext.c\n+++ b/gettext.c\n@@ -18,6 +18,31 @@\n #\tendif\n #endif\n \n+/*\n+ * Guess the user's preferred languages from the value in LANGUAGE environment\n+ * variable and LC_MESSAGES locale category if NO_GETTEXT is not defined.\n+ *\n+ * The result can be a colon-separated list like \"ko:ja:en\".\n+ */\n+const char *get_preferred_languages(void)\n+{\n+\tconst char *retval;\n+\n+\tretval = getenv(\"LANGUAGE\");\n+\tif (retval && *retval)\n+\t\treturn retval;\n+\n+#ifndef NO_GETTEXT\n+\tretval = setlocale(LC_MESSAGES, NULL);\n+\tif (retval && *retval &&\n+\t\tstrcmp(retval, \"C\") &&\n+\t\tstrcmp(retval, \"POSIX\"))\n+\t\treturn retval;\n+#endif\n+\n+\treturn NULL;\n+}\n+\n #ifdef GETTEXT_POISON\n int use_gettext_poison(void)\n {\ndiff --git a/gettext.h b/gettext.h\nindex 7671d09..e539482 100644\n--- a/gettext.h\n+++ b/gettext.h\n@@ -65,4 +65,6 @@ const char *Q_(const char *msgid, const char *plu, unsigned long n)\n /* Mark msgid for translation but do not translate it. */\n #define N_(msgid) msgid\n \n+const char *get_preferred_languages(void);\n+\n #endif\ndiff --git a/http.c b/http.c\nindex 8b659b6..71ed418 100644\n--- a/http.c\n+++ b/http.c\n@@ -8,6 +8,7 @@\n #include \"credential.h\"\n #include \"version.h\"\n #include \"pkt-line.h\"\n+#include \"gettext.h\"\n \n int active_requests;\n int http_is_verbose;\n-- \n2.3.1-280-g2531f2d\n"},{"id":"256716","messageId":"CAGZ79kbUOhbs2DpM3CK=f+Gwj3v-q44Q7beiVgDHPPwm+rhEng@mail.gmail.com","threadId":"37174","inReplyTo":"xmqqa900l57y.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v9 1/1] http: Add Accept-Language header if possible","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-02-26T21:47:34Z","receivedAt":"2015-02-26T21:47:34Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Feb 26, 2015 at 1:42 PM, Junio C Hamano wrote:>\n> Here is what I queued. Thanks.\n\nI did not follow the thread if there are any intermediate patches,\nthough it applied cleanly.\n\nApplying this on top of f18604bbf2c391c689a41fca14cbaeff5e106255\n(http: add Accept-Language header if possible) still doesn't compile for me.\n\nhttp.c:1001:20: error: static declaration of 'get_preferred_languages'\nfollows non-static declaration\n static const char *get_preferred_languages(void)\n ^\nIn file included from cache.h:8:0,\n from http.h:4,\n from http.c:2:\ngettext.h:68:13: note: previous declaration of\n'get_preferred_languages' was here\n const char *get_preferred_languages(void);\n ^\nhttp.c: In function 'get_preferred_languages':\nhttp.c:1010:2: warning: implicit declaration of function 'setlocale'\n[-Wimplicit-function-declaration]\n retval = setlocale(LC_MESSAGES, NULL);\n ^\nhttp.c:1010:21: error: 'LC_MESSAGES' undeclared (first use in this function)\n retval = setlocale(LC_MESSAGES, NULL);\n ^\nhttp.c:1010:21: note: each undeclared identifier is reported only once\nfor each function it appears in\n\nRebasing this on top of current master (Post 2.3 cyle (batch #5)) also fails:\n\nhttp.c:1013:20: error: static declaration of 'get_preferred_languages'\nfollows non-static declaration\n static const char *get_preferred_languages(void)\n ^\nIn file included from cache.h:8:0,\n from http.h:4,\n from http.c:2:\ngettext.h:92:13: note: previous declaration of\n'get_preferred_languages' was here\n const char *get_preferred_languages(void);\n ^\nhttp.c: In function 'get_preferred_languages':\nhttp.c:1022:2: warning: implicit declaration of function 'setlocale'\n[-Wimplicit-function-declaration]\n retval = setlocale(LC_MESSAGES, NULL);\n ^\nhttp.c:1022:21: error: 'LC_MESSAGES' undeclared (first use in this function)\n retval = setlocale(LC_MESSAGES, NULL);\n ^\nhttp.c:1022:21: note: each undeclared identifier is reported only once\nfor each function it appears in\n\n\n\n\n>\n> -- >8 --\n> From: Jeff King \n> Date: Wed, 25 Feb 2015 22:04:16 -0500\n> Subject: [PATCH] gettext.c: move get_preferred_languages() from http.c\n>\n> Calling setlocale(LC_MESSAGES, ...) directly from http.c, without\n> including , was causing compilation warnings. Move the\n> helper function to gettext.c that already includes the header and\n> where locale-related issues are handled.\n>\n> Signed-off-by: Jeff King \n> Signed-off-by: Junio C Hamano \n> ---\n> gettext.c | 25 +++++++++++++++++++++++++\n> gettext.h | 2 ++\n> http.c | 1 +\n> 3 files changed, 28 insertions(+)\n>\n> diff --git a/gettext.c b/gettext.c\n> index 8b2da46..7378ba2 100644\n> --- a/gettext.c\n> +++ b/gettext.c\n> @@ -18,6 +18,31 @@\n> # endif\n> #endif\n>\n> +/*\n> + * Guess the user's preferred languages from the value in LANGUAGE environment\n> + * variable and LC_MESSAGES locale category if NO_GETTEXT is not defined.\n> + *\n> + * The result can be a colon-separated list like \"ko:ja:en\".\n> + */\n> +const char *get_preferred_languages(void)\n> +{\n> + const char *retval;\n> +\n> + retval = getenv(\"LANGUAGE\");\n> + if (retval && *retval)\n> + return retval;\n> +\n> +#ifndef NO_GETTEXT\n> + retval = setlocale(LC_MESSAGES, NULL);\n> + if (retval && *retval &&\n> + strcmp(retval, \"C\") &&\n> + strcmp(retval, \"POSIX\"))\n> + return retval;\n> +#endif\n> +\n> + return NULL;\n> +}\n> +\n> #ifdef GETTEXT_POISON\n> int use_gettext_poison(void)\n> {\n> diff --git a/gettext.h b/gettext.h\n> index 7671d09..e539482 100644\n> --- a/gettext.h\n> +++ b/gettext.h\n> @@ -65,4 +65,6 @@ const char *Q_(const char *msgid, const char *plu, unsigned long n)\n> /* Mark msgid for translation but do not translate it. */\n> #define N_(msgid) msgid\n>\n> +const char *get_preferred_languages(void);\n> +\n> #endif\n> diff --git a/http.c b/http.c\n> index 8b659b6..71ed418 100644\n> --- a/http.c\n> +++ b/http.c\n> @@ -8,6 +8,7 @@\n> #include \"credential.h\"\n> #include \"version.h\"\n> #include \"pkt-line.h\"\n> +#include \"gettext.h\"\n>\n> int active_requests;\n> int http_is_verbose;\n> --\n> 2.3.1-280-g2531f2d\n>\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at http://vger.kernel.org/majordomo-info.html\n"},{"id":"256717","messageId":"20150226220609.GA24663@peff.net","threadId":"37174","inReplyTo":"CAGZ79kbUOhbs2DpM3CK=f+Gwj3v-q44Q7beiVgDHPPwm+rhEng@mail.gmail.com","subject":"Re: [PATCH v9 1/1] http: Add Accept-Language header if possible","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-02-26T22:06:10Z","receivedAt":"2015-02-26T22:06:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Feb 26, 2015 at 01:47:34PM -0800, Stefan Beller wrote:\n\n> On Thu, Feb 26, 2015 at 1:42 PM, Junio C Hamano wrote:>\n> > Here is what I queued. Thanks.\n> \n> I did not follow the thread if there are any intermediate patches,\n> though it applied cleanly.\n\nWhat Junio posted is missing the hunk to drop the old static definition\nof get_preferred_languages from http.c.\n\n-Peff\n"},{"id":"256719","messageId":"20150226220759.GB24663@peff.net","threadId":"37174","inReplyTo":"20150226220609.GA24663@peff.net","subject":"Re: [PATCH v9 1/1] http: Add Accept-Language header if possible","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-02-26T22:07:59Z","receivedAt":"2015-02-26T22:07:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Feb 26, 2015 at 05:06:10PM -0500, Jeff King wrote:\n\n> On Thu, Feb 26, 2015 at 01:47:34PM -0800, Stefan Beller wrote:\n> \n> > On Thu, Feb 26, 2015 at 1:42 PM, Junio C Hamano wrote:>\n> > > Here is what I queued. Thanks.\n> > \n> > I did not follow the thread if there are any intermediate patches,\n> > though it applied cleanly.\n> \n> What Junio posted is missing the hunk to drop the old static definition\n> of get_preferred_languages from http.c.\n\nHere it is, with the commit message and the missing hunk. This works for\nme both with and without NO_GETTEXT defined.\n\n-- >8 --\nSubject: [PATCH] gettext.c: move get_preferred_languages() from http.c\n\nCalling setlocale(LC_MESSAGES, ...) directly from http.c,\nwithout including , was causing compilation\nwarnings. Move the helper function to gettext.c that\nalready includes the header and where locale-related issues\nare handled.\n\nSigned-off-by: Jeff King \n---\n gettext.c | 25 +++++++++++++++++++++++++\n gettext.h | 2 ++\n http.c | 27 +--------------------------\n 3 files changed, 28 insertions(+), 26 deletions(-)\n\ndiff --git a/gettext.c b/gettext.c\nindex 8b2da46..7378ba2 100644\n--- a/gettext.c\n+++ b/gettext.c\n@@ -18,6 +18,31 @@\n #\tendif\n #endif\n \n+/*\n+ * Guess the user's preferred languages from the value in LANGUAGE environment\n+ * variable and LC_MESSAGES locale category if NO_GETTEXT is not defined.\n+ *\n+ * The result can be a colon-separated list like \"ko:ja:en\".\n+ */\n+const char *get_preferred_languages(void)\n+{\n+\tconst char *retval;\n+\n+\tretval = getenv(\"LANGUAGE\");\n+\tif (retval && *retval)\n+\t\treturn retval;\n+\n+#ifndef NO_GETTEXT\n+\tretval = setlocale(LC_MESSAGES, NULL);\n+\tif (retval && *retval &&\n+\t\tstrcmp(retval, \"C\") &&\n+\t\tstrcmp(retval, \"POSIX\"))\n+\t\treturn retval;\n+#endif\n+\n+\treturn NULL;\n+}\n+\n #ifdef GETTEXT_POISON\n int use_gettext_poison(void)\n {\ndiff --git a/gettext.h b/gettext.h\nindex dc1722d..5d8d2df 100644\n--- a/gettext.h\n+++ b/gettext.h\n@@ -89,4 +89,6 @@ const char *Q_(const char *msgid, const char *plu, unsigned long n)\n #define N_(msgid) (msgid)\n #endif\n \n+const char *get_preferred_languages();\n+\n #endif\ndiff --git a/http.c b/http.c\nindex 0153fb0..9c825af 100644\n--- a/http.c\n+++ b/http.c\n@@ -8,6 +8,7 @@\n #include \"credential.h\"\n #include \"version.h\"\n #include \"pkt-line.h\"\n+#include \"gettext.h\"\n \n int active_requests;\n int http_is_verbose;\n@@ -1002,32 +1003,6 @@ static void extract_content_type(struct strbuf *raw, struct strbuf *type,\n \t\tstrbuf_addstr(charset, \"ISO-8859-1\");\n }\n \n-\n-/*\n- * Guess the user's preferred languages from the value in LANGUAGE environment\n- * variable and LC_MESSAGES locale category if NO_GETTEXT is not defined.\n- *\n- * The result can be a colon-separated list like \"ko:ja:en\".\n- */\n-static const char *get_preferred_languages(void)\n-{\n-\tconst char *retval;\n-\n-\tretval = getenv(\"LANGUAGE\");\n-\tif (retval && *retval)\n-\t\treturn retval;\n-\n-#ifndef NO_GETTEXT\n-\tretval = setlocale(LC_MESSAGES, NULL);\n-\tif (retval && *retval &&\n-\t\tstrcmp(retval, \"C\") &&\n-\t\tstrcmp(retval, \"POSIX\"))\n-\t\treturn retval;\n-#endif\n-\n-\treturn NULL;\n-}\n-\n static void write_accept_language(struct strbuf *buf)\n {\n \t/*\n-- \n2.3.0.449.g1690e78\n"},{"id":"256720","messageId":"xmqq1tlcl3sz.fsf@gitster.dls.corp.google.com","threadId":"37174","inReplyTo":"20150226220609.GA24663@peff.net","subject":"Re: [PATCH v9 1/1] http: Add Accept-Language header if possible","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-02-26T22:13:16Z","receivedAt":"2015-02-26T22:13:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Thu, Feb 26, 2015 at 01:47:34PM -0800, Stefan Beller wrote:\n>\n>> On Thu, Feb 26, 2015 at 1:42 PM, Junio C Hamano wrote:>\n>> > Here is what I queued. Thanks.\n>> \n>> I did not follow the thread if there are any intermediate patches,\n>> though it applied cleanly.\n>\n> What Junio posted is missing the hunk to drop the old static definition\n> of get_preferred_languages from http.c.\n\nI am still scratching my head to see how this happened, but I think\nwhen I did\n\n $ git checkout ye/http-accept-language\n $ git apply -3 $gmane/264422\n\nI took the wrong side of the confict in http.c\n\nThanks both for noticing. Now it is fixed up.\n"},{"id":"256721","messageId":"CAGZ79kar4Uf-mCwXpexPvNztwd_vfjdCoT_dDXULkOrCqUhG=A@mail.gmail.com","threadId":"37174","inReplyTo":"20150226220759.GB24663@peff.net","subject":"Re: [PATCH v9 1/1] http: Add Accept-Language header if possible","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-02-26T22:26:05Z","receivedAt":"2015-02-26T22:26:05Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Feb 26, 2015 at 2:07 PM, Jeff King wrote:\n>\n> Here it is, with the commit message and the missing hunk. This works for\n> me both with and without NO_GETTEXT defined.\n\nThis compiles here though a warning is spit:\nIn file included from cache.h:8:0,\n from userdiff.c:1:\ngettext.h:92:1: warning: function declaration isn't a prototype\n[-Wstrict-prototypes]\n const char *get_preferred_languages();\n ^\nso I guess I can still add a\nTested-by: Stefan Beller \n\n>\n> -- >8 --\n> Subject: [PATCH] gettext.c: move get_preferred_languages() from http.c\n>\n> Calling setlocale(LC_MESSAGES, ...) directly from http.c,\n> without including , was causing compilation\n> warnings. Move the helper function to gettext.c that\n> already includes the header and where locale-related issues\n> are handled.\n>\n> Signed-off-by: Jeff King \n> ---\n> gettext.c | 25 +++++++++++++++++++++++++\n> gettext.h | 2 ++\n> http.c | 27 +--------------------------\n> 3 files changed, 28 insertions(+), 26 deletions(-)\n>\n> diff --git a/gettext.c b/gettext.c\n> index 8b2da46..7378ba2 100644\n> --- a/gettext.c\n> +++ b/gettext.c\n> @@ -18,6 +18,31 @@\n> # endif\n> #endif\n>\n> +/*\n> + * Guess the user's preferred languages from the value in LANGUAGE environment\n> + * variable and LC_MESSAGES locale category if NO_GETTEXT is not defined.\n> + *\n> + * The result can be a colon-separated list like \"ko:ja:en\".\n> + */\n> +const char *get_preferred_languages(void)\n> +{\n> + const char *retval;\n> +\n> + retval = getenv(\"LANGUAGE\");\n> + if (retval && *retval)\n> + return retval;\n> +\n> +#ifndef NO_GETTEXT\n> + retval = setlocale(LC_MESSAGES, NULL);\n> + if (retval && *retval &&\n> + strcmp(retval, \"C\") &&\n> + strcmp(retval, \"POSIX\"))\n> + return retval;\n> +#endif\n> +\n> + return NULL;\n> +}\n> +\n> #ifdef GETTEXT_POISON\n> int use_gettext_poison(void)\n> {\n> diff --git a/gettext.h b/gettext.h\n> index dc1722d..5d8d2df 100644\n> --- a/gettext.h\n> +++ b/gettext.h\n> @@ -89,4 +89,6 @@ const char *Q_(const char *msgid, const char *plu, unsigned long n)\n> #define N_(msgid) (msgid)\n> #endif\n>\n> +const char *get_preferred_languages();\n> +\n> #endif\n> diff --git a/http.c b/http.c\n> index 0153fb0..9c825af 100644\n> --- a/http.c\n> +++ b/http.c\n> @@ -8,6 +8,7 @@\n> #include \"credential.h\"\n> #include \"version.h\"\n> #include \"pkt-line.h\"\n> +#include \"gettext.h\"\n>\n> int active_requests;\n> int http_is_verbose;\n> @@ -1002,32 +1003,6 @@ static void extract_content_type(struct strbuf *raw, struct strbuf *type,\n> strbuf_addstr(charset, \"ISO-8859-1\");\n> }\n>\n> -\n> -/*\n> - * Guess the user's preferred languages from the value in LANGUAGE environment\n> - * variable and LC_MESSAGES locale category if NO_GETTEXT is not defined.\n> - *\n> - * The result can be a colon-separated list like \"ko:ja:en\".\n> - */\n> -static const char *get_preferred_languages(void)\n> -{\n> - const char *retval;\n> -\n> - retval = getenv(\"LANGUAGE\");\n> - if (retval && *retval)\n> - return retval;\n> -\n> -#ifndef NO_GETTEXT\n> - retval = setlocale(LC_MESSAGES, NULL);\n> - if (retval && *retval &&\n> - strcmp(retval, \"C\") &&\n> - strcmp(retval, \"POSIX\"))\n> - return retval;\n> -#endif\n> -\n> - return NULL;\n> -}\n> -\n> static void write_accept_language(struct strbuf *buf)\n> {\n> /*\n> --\n> 2.3.0.449.g1690e78\n>\n"},{"id":"256722","messageId":"20150226223603.GA27946@peff.net","threadId":"37174","inReplyTo":"CAGZ79kar4Uf-mCwXpexPvNztwd_vfjdCoT_dDXULkOrCqUhG=A@mail.gmail.com","subject":"Re: [PATCH v9 1/1] http: Add Accept-Language header if possible","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-02-26T22:36:03Z","receivedAt":"2015-02-26T22:36:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Feb 26, 2015 at 02:26:05PM -0800, Stefan Beller wrote:\n\n> On Thu, Feb 26, 2015 at 2:07 PM, Jeff King wrote:\n> >\n> > Here it is, with the commit message and the missing hunk. This works for\n> > me both with and without NO_GETTEXT defined.\n> \n> This compiles here though a warning is spit:\n> In file included from cache.h:8:0,\n> from userdiff.c:1:\n> gettext.h:92:1: warning: function declaration isn't a prototype\n> [-Wstrict-prototypes]\n> const char *get_preferred_languages();\n> ^\n\nHmph. The compiler is right that it should be:\n\n const char *get_preferred_languages(void);\n\nbut my gcc (4.9.2, with -Wstrict_prototypes) does not seem to notice it!\nWeird.\n\n-Peff\n"},{"id":"256723","messageId":"20150226224558.GA10311@peff.net","threadId":"37174","inReplyTo":"20150226223603.GA27946@peff.net","subject":"Re: [PATCH v9 1/1] http: Add Accept-Language header if possible","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-02-26T22:45:58Z","receivedAt":"2015-02-26T22:45:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Feb 26, 2015 at 05:36:03PM -0500, Jeff King wrote:\n\n> > [-Wstrict-prototypes]\n> > const char *get_preferred_languages();\n> > ^\n> \n> Hmph. The compiler is right that it should be:\n> \n> const char *get_preferred_languages(void);\n> \n> but my gcc (4.9.2, with -Wstrict_prototypes) does not seem to notice it!\n> Weird.\n\nUgh. I have a snippet in my config.mak that relaxes the warnings on older\nversions of git, and it was accidentally triggering due to a typo. :(\n\nSo that explains that. Junio, do you mind squashing in:\n\ndiff --git a/gettext.h b/gettext.h\nindex 5d8d2df..33696a4 100644\n--- a/gettext.h\n+++ b/gettext.h\n@@ -89,6 +89,6 @@ const char *Q_(const char *msgid, const char *plu, unsigned long n)\n #define N_(msgid) (msgid)\n #endif\n \n-const char *get_preferred_languages();\n+const char *get_preferred_languages(void);\n \n #endif\n"},{"id":"256725","messageId":"xmqqtwy8jlp0.fsf@gitster.dls.corp.google.com","threadId":"37174","inReplyTo":"20150226224558.GA10311@peff.net","subject":"Re: [PATCH v9 1/1] http: Add Accept-Language header if possible","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-02-26T23:29:47Z","receivedAt":"2015-02-26T23:29:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Thu, Feb 26, 2015 at 05:36:03PM -0500, Jeff King wrote:\n>\n>> > [-Wstrict-prototypes]\n>> > const char *get_preferred_languages();\n>> > ^\n>> \n>> Hmph. The compiler is right that it should be:\n>> \n>> const char *get_preferred_languages(void);\n>> \n>> but my gcc (4.9.2, with -Wstrict_prototypes) does not seem to notice it!\n>> Weird.\n>\n> Ugh. I have a snippet in my config.mak that relaxes the warnings on older\n> versions of git, and it was accidentally triggering due to a typo. :(\n>\n> So that explains that. Junio, do you mind squashing in:\n\nYup, I already did when I got the first one.\n\nThanks.\n\n>\n> diff --git a/gettext.h b/gettext.h\n> index 5d8d2df..33696a4 100644\n> --- a/gettext.h\n> +++ b/gettext.h\n> @@ -89,6 +89,6 @@ const char *Q_(const char *msgid, const char *plu, unsigned long n)\n> #define N_(msgid) (msgid)\n> #endif\n> \n> -const char *get_preferred_languages();\n> +const char *get_preferred_languages(void);\n> \n> #endif\n"},{"id":"257170","messageId":"1425658438-1004-1-git-send-email-avarab@gmail.com","threadId":"37174","inReplyTo":"1405792730-13539-1-git-send-email-eungjun.yi@navercorp.com","subject":"[PATCH] http: Include locale.h when using setlocale()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2015-03-06T16:13:58Z","receivedAt":"2015-03-06T16:13:58Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Since v2.3.0-rc1-37-gf18604b we've been using setlocale() here without\nimporting locale.h. Oddly enough this only causes issues for me under\n-O0 on GCC & Clang. I.e. if I do:\n\n $ git clean -dxf; make -j 1 V=1 CFLAGS=\"-g -O0 -Wall\" http.o\n\nI'll get this on clang 3.5.0-6 & GCC 4.9.1-19 on Debian:\n\n http.c: In function ‘get_preferred_languages’:\n http.c:1021:2: warning: implicit declaration of function ‘setlocale’ [-Wimplicit-function-declaration]\n retval = setlocale(LC_MESSAGES, NULL);\n ^\n http.c:1021:21: error: ‘LC_MESSAGES’ undeclared (first use in this function)\n retval = setlocale(LC_MESSAGES, NULL);\n\nBut changing -O0 to -O1 or another optimization level makes the issue go\naway. Odd, but in any case we should be including this header if we're\ngoing to use the function, so just do that.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n http.c | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/http.c b/http.c\nindex 0153fb0..0606e6c 100644\n--- a/http.c\n+++ b/http.c\n@@ -8,6 +8,9 @@\n #include \"credential.h\"\n #include \"version.h\"\n #include \"pkt-line.h\"\n+#ifndef NO_GETTEXT\n+#\tinclude \n+#endif\n \n int active_requests;\n int http_is_verbose;\n-- \n2.1.3\n"},{"id":"257187","messageId":"xmqqa8zqympf.fsf@gitster.dls.corp.google.com","threadId":"37174","inReplyTo":"1425658438-1004-1-git-send-email-avarab@gmail.com","subject":"Re: [PATCH] http: Include locale.h when using setlocale()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-06T19:01:32Z","receivedAt":"2015-03-06T19:01:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> Since v2.3.0-rc1-37-gf18604b we've been using setlocale() here without\n> importing locale.h. Oddly enough this only causes issues for me under\n> -O0 on GCC & Clang.\n\nSorry for not making this entry in \"What's cooking\" report very\nprominent:\n\n * ye/http-accept-language (2015-02-26) 1 commit\n (merged to 'next' on 2015-03-03 at 58d195e)\n + gettext.c: move get_preferred_languages() from http.c\n\n Compilation fix for a recent topic in 'master'.\n\n Will merge to 'master'.\n\nThis has cooked on 'next' for a few days, and is eligible to\ngraduate to 'master'. Will be in the next update.\n\nThanks.\n"}]}