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

[Junio C Hamano <gitster@pobox.com>]

Yi EungJun <semtlenori@gmail.com> writes:

> From: Yi EungJun <eungjun.yi@navercorp.com>
>
> Add an Accept-Language header which indicates the user's preferred
> languages defined by $LANGUAGE, $LC_ALL, $LC_MESSAGES and $LANG.
>
> Examples:
>   LANGUAGE= -> ""
>   LANGUAGE=ko:en -> "Accept-Language: ko, en;q=0.9, *;q=0.1"
>   LANGUAGE=ko LANG=en_US.UTF-8 -> "Accept-Language: ko, *;q=0.1"
>   LANGUAGE= LANG=en_US.UTF-8 -> "Accept-Language: en-US, *;q=0.1"
>
> This gives git servers a chance to display remote error messages in
> the user's preferred language.
>
> Limit the number of languages to 1,000 because q-value must not be
> smaller than 0.001, and limit the length of Accept-Language header to
> 4,000 bytes for some HTTP servers which cannot accept such long header.
>
> Signed-off-by: Yi EungJun <eungjun.yi@navercorp.com>
> ---
>  http.c                     | 154 +++++++++++++++++++++++++++++++++++++++++++++
>  remote-curl.c              |   2 +
>  t/t5550-http-fetch-dumb.sh |  31 +++++++++
>  3 files changed, 187 insertions(+)
>
> diff --git a/http.c b/http.c
> index 040f362..69624af 100644
> --- a/http.c
> +++ b/http.c
> @@ -68,6 +68,8 @@ static struct curl_slist *no_pragma_header;
>  
>  static struct active_request_slot *active_queue_head;
>  
> +static struct strbuf *cached_accept_language;
> +
>  size_t fread_buffer(char *ptr, size_t eltsize, size_t nmemb, void *buffer_)
>  {
>  	size_t size = eltsize * nmemb;
> @@ -515,6 +517,9 @@ void http_cleanup(void)
>  		cert_auth.password = NULL;
>  	}
>  	ssl_cert_password_required = 0;
> +
> +	if (cached_accept_language)
> +		strbuf_release(cached_accept_language);

Is this correct?

You still keep cached_accept_language pointer itself, so the next
call to get_accept_language() would say "Ah, cached_accept_language
is there, so its contents (which is empty because we released it
here) must be valid and to be reused".  Perhaps you want to free it,
too, i.e.

	if (cached_accept_language) {
		strbuf_release(cached_accept_language);
                free(cached_accept_language);
                cached_accept_language = NULL;
	}

or something?

> +static struct strbuf *get_accept_language(void)
> +{
> + ...
> +	if (cached_accept_language)
> +		return cached_accept_language;
> +
> +	cached_accept_language = xmalloc(sizeof(struct strbuf));
> + ...
> +	for (max_q = 1, decimal_places = 0;
> +		max_q < num_langs;
> +		decimal_places++, max_q *= 10);

Have that "empty statement" on its own separate line, i.e.

	for (a, counter = 0;
             b;
             c, counter++)
             ; /* just counting */

Alternatively, you can make it more obvious that the purpose of loop
is to count, i.e.

	for (a, counter = 0;
             b;
             c)
             counter++;

> +test_expect_success 'git client does not send Accept-Language' '
> +	test_must_fail env GIT_CURL_VERBOSE=1 LANGUAGE= git clone "$HTTPD_URL/accept/language" 2>stderr &&
> +	! grep "^Accept-Language:" stderr
> +'

Hmph, this test smells a bit brittle.  What is the reason you expect
"git clone" to fail?  Is it because there is no repository at the
named URL at "$HTTPD_URL/accept/language"?  Is that the only plausible
reason for a failure?

It might be better to use the URL to a repository that is expected
to be served by the server started in this test and expect success.
If it bothers you that "clone" creates a new copy that is otherwise
unused by this test, you can use something like "ls-remote" instead,
I would think.