# [PATCH] http: Add Accept-Language header if possible 12 messages from 2014-07-08 to 2014-07-12. Participants: Yi EungJun, Eric Sunshine, Jeff King, Yi, EungJun, Peter Krefting. Thread: https://gitlist.dev/t/37077 ## Yi EungJun, 2014-07-08 15:54 Subject: [PATCH] http: Add Accept-Language header if possible Message-ID: <1404834846-11812-1-git-send-email-eungjun.yi@navercorp.com> URL: https://gitlist.dev/e/1404834846-11812-1-git-send-email-eungjun.yi%40navercorp.com ``` From: Yi EungJun Add an Accept-Language header which indicates the user's preferred languages defined by 'LANGUAGE' environment variable if the variable is not empty. Example: LANGUAGE= -> "" LANGUAGE=ko -> "Accept-Language: ko; q=1.000, *; q=0.001" LANGUAGE=ko:en -> "Accept-Language: ko; q=1.000, en; q=0.999, *; q=0.001" This gives git servers a chance to display remote error messages in the user's preferred language. --- http.c | 43 +++++++++++++++++++++++++++++++++++++++++++ t/t5550-http-fetch-dumb.sh | 10 ++++++++++ 2 files changed, 53 insertions(+) diff --git a/http.c b/http.c index 3a28b21..c345616 100644 --- a/http.c +++ b/http.c @@ -983,6 +983,47 @@ static void extract_content_type(struct strbuf *raw, struct strbuf *type, strbuf_addstr(charset, "ISO-8859-1"); } +/* + * Add an Accept-Language header which indicates user's preferred languages + * defined by 'LANGUAGE' environment variable if the variable is not empty. + * + * Example: + * LANGUAGE= -> "" + * LANGUAGE=ko -> "Accept-Language: ko; q=1.000, *; q=0.001" + * LANGUAGE=ko:en -> "Accept-Language: ko; q=1.000, en; q=0.999, *; q=0.001" + */ +static void add_accept_language(struct strbuf *buf) +{ + const char *p1, *p2; + float q = 1.000; + + p1 = getenv("LANGUAGE"); + + if (p1 != NULL && p1[0] != '\0') { + strbuf_reset(buf); + strbuf_addstr(buf, "Accept-Language: "); + for (p2 = p1; q > 0.001; p2++) { + if ((*p2 == ':' || *p2 == '\0') && p1 != p2) { + if (q < 1.0) { + strbuf_addstr(buf, ", "); + } + strbuf_add(buf, p1, p2 - p1); + strbuf_addf(buf, "; q=%.3f", q); + q -= 0.001; + p1 = p2 + 1; + + if (*p2 == '\0') { + break; + } + } + } + if (q < 1.0) { + strbuf_addstr(buf, ", "); + } + strbuf_addstr(buf, "*; q=0.001\r\n"); + } +} + /* http_request() targets */ #define HTTP_REQUEST_STRBUF 0 #define HTTP_REQUEST_FILE 1 @@ -1020,6 +1061,8 @@ static int http_request(const char *url, fwrite_buffer); } + add_accept_language(&buf); + strbuf_addstr(&buf, "Pragma:"); if (options && options->no_cache) strbuf_addstr(&buf, " no-cache"); diff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh index ac71418..ea15158 100755 --- a/t/t5550-http-fetch-dumb.sh +++ b/t/t5550-http-fetch-dumb.sh @@ -196,5 +196,15 @@ test_expect_success 'reencoding is robust to whitespace oddities' ' grep "this is the error message" stderr ' +test_expect_success 'git client sends Accept-Language' ' + GIT_CURL_VERBOSE=1 LANGUAGE=ko:en git clone "$HTTPD_URL/accept/language" 2>actual + grep "^Accept-Language: ko; q=1.000, en; q=0.999, \*; q=0.001" actual +' + +test_expect_success 'git client does not send Accept-Language' ' + GIT_CURL_VERBOSE=1 LANGUAGE= git clone "$HTTPD_URL/accept/language" 2>actual + test_must_fail grep "^Accept-Language:" actual +' + stop_httpd test_done -- 2.0.1.473.gafdefd9.dirty ``` ## Eric Sunshine, 2014-07-08 21:52 Subject: Re: [PATCH] http: Add Accept-Language header if possible Message-ID: URL: https://gitlist.dev/e/CAPig%2BcTbhUKF7OwYJY2_1m9j%3DkhPRaLM2kd5KqDGD%3DvjRz0qHg%40mail.gmail.com In-Reply-To: <1404834846-11812-1-git-send-email-eungjun.yi@navercorp.com> ``` On Tue, Jul 8, 2014 at 11:54 AM, Yi EungJun wrote: > From: Yi EungJun > > Add an Accept-Language header which indicates the user's preferred > languages defined by 'LANGUAGE' environment variable if the variable is > not empty. > > Example: > LANGUAGE= -> "" > LANGUAGE=ko -> "Accept-Language: ko; q=1.000, *; q=0.001" > LANGUAGE=ko:en -> "Accept-Language: ko; q=1.000, en; q=0.999, *; q=0.001" > > This gives git servers a chance to display remote error messages in > the user's preferred language. > --- > http.c | 43 +++++++++++++++++++++++++++++++++++++++++++ > t/t5550-http-fetch-dumb.sh | 10 ++++++++++ > 2 files changed, 53 insertions(+) > > diff --git a/http.c b/http.c > index 3a28b21..c345616 100644 > --- a/http.c > +++ b/http.c > @@ -983,6 +983,47 @@ static void extract_content_type(struct strbuf *raw, struct strbuf *type, > strbuf_addstr(charset, "ISO-8859-1"); > } > > +/* > + * Add an Accept-Language header which indicates user's preferred languages > + * defined by 'LANGUAGE' environment variable if the variable is not empty. > + * > + * Example: > + * LANGUAGE= -> "" > + * LANGUAGE=ko -> "Accept-Language: ko; q=1.000, *; q=0.001" > + * LANGUAGE=ko:en -> "Accept-Language: ko; q=1.000, en; q=0.999, *; q=0.001" > + */ > +static void add_accept_language(struct strbuf *buf) > +{ > + const char *p1, *p2; > + float q = 1.000; > + > + p1 = getenv("LANGUAGE"); > + > + if (p1 != NULL && p1[0] != '\0') { > + strbuf_reset(buf); It seems wrong to clear 'buf' in a function named add_accept_language(). > + strbuf_addstr(buf, "Accept-Language: "); > + for (p2 = p1; q > 0.001; p2++) { > + if ((*p2 == ':' || *p2 == '\0') && p1 != p2) { > + if (q < 1.0) { > + strbuf_addstr(buf, ", "); > + } > + strbuf_add(buf, p1, p2 - p1); > + strbuf_addf(buf, "; q=%.3f", q); > + q -= 0.001; > + p1 = p2 + 1; > + > + if (*p2 == '\0') { > + break; > + } > + } > + } > + if (q < 1.0) { > + strbuf_addstr(buf, ", "); > + } > + strbuf_addstr(buf, "*; q=0.001\r\n"); Manually adding "\r\n" is contraindicated. Headers passed to curl_easy_setopt(c, CURLOPT_HTTPHEADER, headers) must not have "\r\n", since curl will add terminators itself [1]. [1]: http://curl.haxx.se/libcurl/c/CURLOPT_HTTPHEADER.html > + } > +} > + > /* http_request() targets */ > #define HTTP_REQUEST_STRBUF 0 > #define HTTP_REQUEST_FILE 1 > @@ -1020,6 +1061,8 @@ static int http_request(const char *url, > fwrite_buffer); > } > > + add_accept_language(&buf); This is inconsistent with how other headers are handled by this function. The existing idiom is: strbuf_add(&buf, ...); /* construct header */ headers = curl_slist_apend(headers, buf.buf); strbuf_reset(&buf); > + > strbuf_addstr(&buf, "Pragma:"); > if (options && options->no_cache) > strbuf_addstr(&buf, " no-cache"); > diff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh > index ac71418..ea15158 100755 > --- a/t/t5550-http-fetch-dumb.sh > +++ b/t/t5550-http-fetch-dumb.sh > @@ -196,5 +196,15 @@ test_expect_success 'reencoding is robust to whitespace oddities' ' > grep "this is the error message" stderr > ' > > +test_expect_success 'git client sends Accept-Language' ' > + GIT_CURL_VERBOSE=1 LANGUAGE=ko:en git clone "$HTTPD_URL/accept/language" 2>actual Broken &&-chain. > + grep "^Accept-Language: ko; q=1.000, en; q=0.999, \*; q=0.001" actual Do you want to \-escape the periods? (Or maybe use 'grep -F'?) > +' > + > +test_expect_success 'git client does not send Accept-Language' ' > + GIT_CURL_VERBOSE=1 LANGUAGE= git clone "$HTTPD_URL/accept/language" 2>actual Broken &&-chain. > + test_must_fail grep "^Accept-Language:" actual > +' > + > stop_httpd > test_done > -- > 2.0.1.473.gafdefd9.dirty ``` ## Jeff King, 2014-07-09 05:10 Subject: Re: [PATCH] http: Add Accept-Language header if possible Message-ID: <20140709051040.GB2318@sigill.intra.peff.net> URL: https://gitlist.dev/e/20140709051040.GB2318%40sigill.intra.peff.net In-Reply-To: <1404834846-11812-1-git-send-email-eungjun.yi@navercorp.com> ``` On Wed, Jul 09, 2014 at 12:54:06AM +0900, Yi EungJun wrote: > From: Yi EungJun > > Add an Accept-Language header which indicates the user's preferred > languages defined by 'LANGUAGE' environment variable if the variable is > not empty. > > Example: > LANGUAGE= -> "" > LANGUAGE=ko -> "Accept-Language: ko; q=1.000, *; q=0.001" > LANGUAGE=ko:en -> "Accept-Language: ko; q=1.000, en; q=0.999, *; q=0.001" > > This gives git servers a chance to display remote error messages in > the user's preferred language. Should this also take into account other language-related variables? I'd think $LC_ALL, $LC_MESSAGES, and $LANG would affect it, too. Are colon-separated values a standard in $LANGUAGE? I have never seen them, but I admit I am not very knowledgeable about localization issues. Also, we do we need to do more parsing? My $LANG is set to en_US.UTF-8. The encoding part is presumably uninteresting to the remote server. I also wonder if there are support functions in libc or as part of gettext that can help us get these values. -Peff ``` ## Yi, EungJun, 2014-07-09 05:46 Subject: Re: [PATCH] http: Add Accept-Language header if possible Message-ID: URL: https://gitlist.dev/e/CAFT%2BTg8HZTsbWK2WHHg_q04LY5Vm7cjfNkfHGBEdbKjf1%3D%3DrKw%40mail.gmail.com In-Reply-To: <20140709051040.GB2318@sigill.intra.peff.net> ``` 2014-07-09 14:10 GMT+09:00 Jeff King : > On Wed, Jul 09, 2014 at 12:54:06AM +0900, Yi EungJun wrote: > >> From: Yi EungJun >> >> Add an Accept-Language header which indicates the user's preferred >> languages defined by 'LANGUAGE' environment variable if the variable is >> not empty. >> >> Example: >> LANGUAGE= -> "" >> LANGUAGE=ko -> "Accept-Language: ko; q=1.000, *; q=0.001" >> LANGUAGE=ko:en -> "Accept-Language: ko; q=1.000, en; q=0.999, *; q=0.001" >> >> This gives git servers a chance to display remote error messages in >> the user's preferred language. > > Should this also take into account other language-related variables? I'd > think $LC_ALL, $LC_MESSAGES, and $LANG would affect it, too. Are > colon-separated values a standard in $LANGUAGE? I have never seen them, > but I admit I am not very knowledgeable about localization issues. > > Also, we do we need to do more parsing? My $LANG is set to en_US.UTF-8. > The encoding part is presumably uninteresting to the remote server. I > also wonder if there are support functions in libc or as part of gettext > that can help us get these values. > > -Peff I agree with you. In fact, I tried to get user's preferred language in the same way as gettext. It has guess_category_value() to do that and the function is good enough because it considers $LANGUAGE, $LC_ALL, $LANG, and also system-dependent preferences. But the function does not seem a public API and I don't know how can I use the function in Git. So I chose to use $LANGUAGE only. ``` ## Jeff King, 2014-07-09 06:18 Subject: Re: [PATCH] http: Add Accept-Language header if possible Message-ID: <20140709061853.GA6731@sigill.intra.peff.net> URL: https://gitlist.dev/e/20140709061853.GA6731%40sigill.intra.peff.net In-Reply-To: ``` On Wed, Jul 09, 2014 at 02:46:35PM +0900, Yi, EungJun wrote: > I agree with you. In fact, I tried to get user's preferred language in > the same way as gettext. It has guess_category_value() to do that and > the function is good enough because it considers $LANGUAGE, $LC_ALL, > $LANG, and also system-dependent preferences. But the function does > not seem a public API and I don't know how can I use the function in > Git. So I chose to use $LANGUAGE only. I did some digging, and I think the public API is setlocale with a NULL parameter, like: printf("%s\n", setlocale(LC_MESSAGES, NULL)); That still will end up like "en_US.UTF-8", though; I couldn't find any standard functions for parsing that. It seems like it would be pretty straightforward to do so, though. >From my brief reading of rfc2616, that should probably become "en-us", and any time we add "x-y", we may want to add "x" as a fallback (that is certainly true for English; I don't know about other languages with dialects). -Peff ``` ## Peter Krefting, 2014-07-09 10:40 Subject: Re: [PATCH] http: Add Accept-Language header if possible Message-ID: URL: https://gitlist.dev/e/alpine.DEB.2.00.1407091127490.22132%40ds9.cixit.se In-Reply-To: <1404834846-11812-1-git-send-email-eungjun.yi@navercorp.com> ``` Yi EungJun: > Example: > LANGUAGE= -> "" > LANGUAGE=ko -> "Accept-Language: ko; q=1.000, *; q=0.001" > LANGUAGE=ko:en -> "Accept-Language: ko; q=1.000, en; q=0.999, *; q=0.001" Avoid adding "q=1.000". It is redundant (the default for any unqualified language names is 1.0, and additionally there has historically been some buggy servers that failed if it was included. > + p1 = getenv("LANGUAGE"); You need a fallback mechanism here to parse all the possible language variables. I would use the first one I find of these: 1. LANGUAGE 2. LC_ALL 3. LC_MESSAGES 4. LANG Only "LANGUAGE" holds a colon-separated list, but the same code can parse all of them, just yielding a single entry for the others. > + strbuf_add(buf, p1, p2 - p1); The tokens are on the form language_COUNTRY.encoding@identifier, whereas Accept-Language wants language-COUNTRY, so you need to a) replace "_" with "-", and b) chop off anything following a "." or "@". > + strbuf_addf(buf, "; q=%.3f", q); > + q -= 0.001; Three decimals seems a bit overkill, but some experimentation might be necessary. > + strbuf_addstr(buf, "*; q=0.001\r\n"); You should probably also add an explicit "en" here, if none was already included. I've seen some servers break horribly if "en" isn't included. For reference, I have my LANGUAGE variable set to "sv_SE.utf8:sv:nb_NO.utf8:nb:da_DK.utf8:da:nn_NO.utf8:nn:en_GB.utf8:en_US.utf8:en" -- \\// Peter - http://www.softwolves.pp.se/ ``` ## Peter Krefting, 2014-07-09 10:46 Subject: Re: [PATCH] http: Add Accept-Language header if possible Message-ID: URL: https://gitlist.dev/e/alpine.DEB.2.00.1407091142030.22132%40ds9.cixit.se In-Reply-To: <20140709061853.GA6731@sigill.intra.peff.net> ``` Jeff King: > I did some digging, and I think the public API is setlocale with a > NULL parameter, like: > > printf("%s\n", setlocale(LC_MESSAGES, NULL)); > > That still will end up like "en_US.UTF-8", though; And it only yields the highest-priority language, I think. > I couldn't find any standard functions for parsing that. It seems > like it would be pretty straightforward to do so, though. RFC 5646 is the current specification on language tags, btw. > From my brief reading of rfc2616, that should probably become > "en-us", and any time we add "x-y", we may want to add "x" as a > fallback (that is certainly true for English; I don't know about > other languages with dialects). Yes, adding the generic fallback is necessary, as "en-US" on the server matches a client's "en", but not vice versa. So if you request "en-US" and "de" and the server only has "en-GB" and "de", you'd get the "de" version. Debian's website has a nice writeup on the subject: http://www.debian.org/intro/cn#howtoset -- \\// Peter - http://www.softwolves.pp.se/ ``` ## Jeff King, 2014-07-10 20:10 Subject: Re: [PATCH] http: Add Accept-Language header if possible Message-ID: <20140710201034.GB15615@sigill.intra.peff.net> URL: https://gitlist.dev/e/20140710201034.GB15615%40sigill.intra.peff.net In-Reply-To: ``` On Wed, Jul 09, 2014 at 11:46:14AM +0100, Peter Krefting wrote: > Jeff King: > > >I did some digging, and I think the public API is setlocale with a NULL > >parameter, like: > > > > printf("%s\n", setlocale(LC_MESSAGES, NULL)); > > > >That still will end up like "en_US.UTF-8", though; > > And it only yields the highest-priority language, I think. I wasn't clear on whether POSIX locale variables actually supported multiple languages with priorities. I have never seen that, though the original commit message indicated that LANGUAGE=x:y was a thing (I wasn't sure if that was a made-up thing, or something that libc actually supported). > Debian's website has a nice writeup on the subject: > http://www.debian.org/intro/cn#howtoset That seems to be about language settings in browsers, which are a much richer set of preferences than POSIX locales (I think). It would not be wrong to have that level of configuration for git's http requests, but I do not know if it is worth the effort. Mapping the user's gettext locale into an accept-language header seems like a straightforward way to communicate to the other side what the client is using to show errors (so that errors coming from the server can match). If that is the case, though, I wonder if we should actually be adding it as a git-protocol header so that all transports can benefit (i.e., we could be localizing human-readable error messages in upload-pack, receive-pack, etc). -Peff ``` ## Yi, EungJun, 2014-07-11 09:05 Subject: Re: [PATCH] http: Add Accept-Language header if possible Message-ID: URL: https://gitlist.dev/e/CAFT%2BTg8Z5z%3DvTcCTbwKMQeTAaqpTYzr5OzdfedV0q8J5KzG-Yg%40mail.gmail.com In-Reply-To: ``` 2014-07-09 19:40 GMT+09:00 Peter Krefting : > Yi EungJun: > > >> Example: >> LANGUAGE= -> "" >> LANGUAGE=ko -> "Accept-Language: ko; q=1.000, *; q=0.001" >> LANGUAGE=ko:en -> "Accept-Language: ko; q=1.000, en; q=0.999, *; q=0.001" > > > Avoid adding "q=1.000". It is redundant (the default for any unqualified > language names is 1.0, and additionally there has historically been some > buggy servers that failed if it was included. Ok, I'll fix it. > > >> + p1 = getenv("LANGUAGE"); > > > You need a fallback mechanism here to parse all the possible language > variables. I would use the first one I find of these: > > 1. LANGUAGE > 2. LC_ALL > 3. LC_MESSAGES > 4. LANG > > Only "LANGUAGE" holds a colon-separated list, but the same code can parse > all of them, just yielding a single entry for the others. I'll use setlocale(LC_MESSAGES, NULL) as well as getenv("LANGUAGE"). > > >> + strbuf_add(buf, p1, p2 - p1); > > > The tokens are on the form language_COUNTRY.encoding@identifier, whereas > Accept-Language wants language-COUNTRY, so you need to a) replace "_" with > "-", and b) chop off anything following a "." or "@". > > >> + strbuf_addf(buf, "; q=%.3f", q); >> + q -= 0.001; > > > Three decimals seems a bit overkill, but some experimentation might be > necessary. I'll use three decimals only if there are 100 or more preferred languages. > > >> + strbuf_addstr(buf, "*; q=0.001\r\n"); > > > You should probably also add an explicit "en" here, if none was already > included. I've seen some servers break horribly if "en" isn't included. I'll send Accept-Language only if there is at least one preferred language. Is it enough? Thanks for your review. ``` ## Yi, EungJun, 2014-07-11 09:43 Subject: Re: [PATCH] http: Add Accept-Language header if possible Message-ID: URL: https://gitlist.dev/e/CAFT%2BTg_Zkim8DJzdKHa5d7ibN3Za-fftd9m-cZDo3rizLdQ%3Dxg%40mail.gmail.com In-Reply-To: <20140710201034.GB15615@sigill.intra.peff.net> ``` 2014-07-11 5:10 GMT+09:00 Jeff King : > On Wed, Jul 09, 2014 at 11:46:14AM +0100, Peter Krefting wrote: > >> Jeff King: >> >> >I did some digging, and I think the public API is setlocale with a NULL >> >parameter, like: >> > >> > printf("%s\n", setlocale(LC_MESSAGES, NULL)); >> > >> >That still will end up like "en_US.UTF-8", though; >> >> And it only yields the highest-priority language, I think. > > I wasn't clear on whether POSIX locale variables actually supported > multiple languages with priorities. I have never seen that, though the > original commit message indicated that LANGUAGE=x:y was a thing (I > wasn't sure if that was a made-up thing, or something that libc actually > supported). > >> Debian's website has a nice writeup on the subject: >> http://www.debian.org/intro/cn#howtoset > > That seems to be about language settings in browsers, which are a much > richer set of preferences than POSIX locales (I think). > > It would not be wrong to have that level of configuration for git's http > requests, but I do not know if it is worth the effort. Mapping the > user's gettext locale into an accept-language header seems like a > straightforward way to communicate to the other side what the client is > using to show errors (so that errors coming from the server can match). Thanks for you advice. I'll write a path to use both of setlocale(LC_MESSAGES, NULL) and getenv("LANGUAGE") to get the user's preferred language. setlocale(LC_MESSAGES, NULL) is quite nice way because it takes LC_ALL, LC_MESSAGES and LANG into account, but not LANGUAGE. I think we should take also LANGUAGE into account as gettext does. [1] [1]: http://www.gnu.org/software/gettext/manual/gettext.html#Locale-Environment-Variables ``` ## Yi, EungJun, 2014-07-11 16:35 Subject: Re: [PATCH] http: Add Accept-Language header if possible Message-ID: URL: https://gitlist.dev/e/CAFT%2BTg9vLE-xr9dBV8uD%3Df0xu%2BU4_XOqdRC5ZKd95P0vLqfdbw%40mail.gmail.com In-Reply-To: ``` 2014-07-12 1:24 GMT+09:00 Eric Sunshine : > On Fri, Jul 11, 2014 at 5:22 AM, Yi, EungJun wrote: >> 2014-07-09 6:52 GMT+09:00 Eric Sunshine : >>>> + grep "^Accept-Language: ko; q=1.000, en; q=0.999, \*; q=0.001" actual >>> >>> Do you want to \-escape the periods? (Or maybe use 'grep -F'?) >> >> I just want to match '*' character. I tried 'grep -F' but it does not help. > > I meant that the periods in your grep pattern are matching any > character. If you want to be very strict, so that they match only > period, then you should \-escape them. Oops, I misunderstood you. You are right. I'll \- escape them. ``` ## Peter Krefting, 2014-07-12 10:11 Subject: Re: [PATCH] http: Add Accept-Language header if possible Message-ID: URL: https://gitlist.dev/e/alpine.DEB.2.00.1407121110070.18850%40ds9.cixit.se In-Reply-To: <20140710201034.GB15615@sigill.intra.peff.net> ``` Jeff King: > If that is the case, though, I wonder if we should actually be adding it > as a git-protocol header so that all transports can benefit (i.e., we > could be localizing human-readable error messages in upload-pack, > receive-pack, etc). That would be very nice, thre is a lot of language mixing going on at the moment. -- \\// Peter - http://www.softwolves.pp.se/ ```