Re: [PATCH] http: Add Accept-Language header if possible

[Eric Sunshine <sunshine@sunshineco.com>]

On Tue, Jul 8, 2014 at 11:54 AM, Yi EungJun <semtlenori@gmail.com> wrote:
> From: Yi EungJun <eungjun.yi@navercorp.com>
>
> 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