Re: [PATCH] remote-curl: send Accept-Language header to server

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 8, 2022, 23:32 UTC
Message-ID: <xmqqa6amohop.fsf@gitster.g>
In-Reply-To: <pull.1251.git.1654678407365.gitgitgadget@gmail.com>

"Li Linchao via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Cactusinhand <lilinchao@oschina.cn>

Huh?

Show 5 quoted lines

> Git server end's ability to accept Accept-Language header was introduced
> in f18604bbf2(http: add Accept-Language header if possible), but this is
> only used by very early phase of the transfer, that's HTTP GET request to
> discover references. For other phases, like POST request in the smart HTTP
> the server side don't know what language client speak.

"client speak" -> "the client speaks".

> This patch teaches git client to learn end-user's preferred language and

"This patch teaches" -> "Teach"

> throw accept-language header to server side. Once server get this header

"server side" -> "the server side". "server get" -> "the server gets" "header" -> "header,"

> it have ability to talk to end-user with language they understand, this

"it have ability" -> "it has the ability"

", this" -> ". This"

> would be very helpful for many non-English speakers.
>
> Signed-off-by: Li Linchao <lilinchao@oschina.cn>

Show 10 quoted lines

> diff --git a/http.c b/http.c
> index 11c6f69facd..0654e111d1d 100644
> --- a/http.c
> +++ b/http.c
> @@ -1775,7 +1775,7 @@ static void write_accept_language(struct strbuf *buf)
>   *   LANGUAGE= LANG=en_US.UTF-8 -> "Accept-Language: en-US, *; q=0.1"
>   *   LANGUAGE= LANG=C -> ""
>   */
> -static const char *get_accept_language(void)
> +const char *get_accept_language(void)

It was an understandable name for a file-scope static function, but is this name suitable to be a global without making it more narrow and specific to "HTTP" and "Header"?

Show 10 quoted lines

> diff --git a/remote-curl.c b/remote-curl.c
> index 67f178b1120..8acf506705c 100644
> --- a/remote-curl.c
> +++ b/remote-curl.c
> @@ -1400,7 +1412,7 @@ static int stateless_connect(const char *service_name)
>  	struct discovery *discover;
>  	struct rpc_state rpc;
>  	struct strbuf buf = STRBUF_INIT;
> -
> +	const char *accept_language;

Do not lose the blank line after the block with variable declarations. IOW, the patch around this line should read like this instead:

Show 7 quoted lines

>  	struct discovery *discover;
>  	struct rpc_state rpc;
>  	struct strbuf buf = STRBUF_INIT;
> +	const char *accept_language;
>
>  	/*
>  	 * Run the info/refs request and see if the server supports protocol

Show 8 quoted lines

> @@ -1418,6 +1430,10 @@ static int stateless_connect(const char *service_name)
>  		printf("\n");
>  		fflush(stdout);
>  	}
> +	accept_language = get_accept_language();
> +	if (accept_language) {
> +		rpc.hdr_accept_language = xstrfmt("%s", accept_language);
> +	}

Drop {} around a single-statement block.

Show 12 quoted lines

> +	cat >exp <<-\EOF &&
> +	=> Send header: Accept-Language: zh-CN, en;q=0.9, *;q=0.8
> +	=> Send header: Accept-Language: zh-CN, en;q=0.9, *;q=0.8
> +	EOF
> +
> +	cd "$ROOT_PATH"/test_repo_clone &&
> +	: >path_lang &&
> +	git add path_lang &&
> +	test_tick &&
> +	git commit -m path_lang &&
> +	HEAD=$(git rev-parse --verify HEAD) &&
> +	GIT_TRACE_CURL=true LANGUAGE="zh_CN:en" git push -v -v 2>err &&

A few comments.

 * In all gettext/locale tests we seem to set both LANGUAGE and
   LC_ALL environment variables.  Shouldn't we do the same for
   consistency?

 * In existing tests, we seem to use ko_KR, en_US, ja_JP, and random
   assortment of languages (e.g. t5550).  Can we safely add any new
   languages to the mix without any downside to the tester?  We
   should reuse what we already use, especially if this ends up
   forcing users and testers to install yet another "language pack"
   for zh_CN.

Thanks.