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

["lilinchao@oschina.cn" <lilinchao@oschina.cn>]

>"Li Linchao via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> From: Li Linchao <lilinchao@oschina.cn>
>>
>> Git server end's ability to accept Accept-Language header was introduced
>> in f18604bbf2(http: add Accept-Language header if possible), but this is
>
>Pleaes refer to the commit like so:
>
>    f18604bb (http: add Accept-Language header if possible, 2015-01-28)
>
>(cf. Documentation/SubmittingPatches::commit-reference)
>
>"git show -s --pretty=reference f18604bb" is one way to format a
>commit name in that format.
> 
OK, thanks for reminding.
>> only used by very early phase of the transfer, that's HTTP GET request to
>
>"that's" -> "which is", probably. 
OK.
>
>> discover references. For other phases, like POST request in the smart HTTP
>> the server side don't know what language the client speaks.
>
>"HTTP the server side don't" -> "HTTP, the server does not"
>
>>  http.c                      |  4 ++--
>>  http.h                      |  3 +++
>>  remote-curl.c               | 16 ++++++++++++++++
>>  t/t5541-http-push-smart.sh  | 19 +++++++++++++++++++
>>  t/t5550-http-fetch-dumb.sh  | 10 +++++-----
>>  t/t5551-http-fetch-smart.sh | 10 ++++++++--
>>  6 files changed, 53 insertions(+), 9 deletions(-)
>
>What is curious is that without any of changes to the *.[ch] files,
>updated test 5550 and 5551 pass already.
>
>In other words, these updated tests in 5550 and 5551 probably are
>not testing the behaviour the updated code intends to show.  Of
>course, if we revert the code that taught the Accept-Language to the
>GET requests in f18604bb, these tests will fail.  There is no reason
>to touch these two tests to "prove" that the code change in this
>patch does not break existing support, either. 
My bad, the updated test in t5550 can not test the updated code, but test 
the original code in f18604bb.
>
>> diff --git a/http.h b/http.h
>> index ba303cfb372..3c94c479100 100644
>> --- a/http.h
>> +++ b/http.h
>> @@ -178,6 +178,9 @@ int http_fetch_ref(const char *base, struct ref *ref);
>>  int http_get_info_packs(const char *base_url,
>>  struct packed_git **packs_head);
>> 
>> +/* Helper for getting Accept-Language header */
>> +const char *http_get_accept_language_header(void);
>
>OK.
>
>> @@ -932,6 +933,10 @@ static int post_rpc(struct rpc_state *rpc, int stateless_connect, int flush_rece
>>  headers = curl_slist_append(headers, needs_100_continue ?
>>  "Expect: 100-continue" : "Expect:");
>> 
>> +	/* Add Accept-Language header */
>> +	if (rpc->hdr_accept_language)
>> +	headers = curl_slist_append(headers, rpc->hdr_accept_language);
>
>curl_slist_append() makes a copy of .hdr_accept_language, so rpc
>struct is still responsible to release the resource used for the
>member when it goes out of scope.
>
>> +	accept_language = http_get_accept_language_header();
>> +	if (accept_language) {
>> +	strbuf_addstr(&buf, accept_language);
>> +	rpc->hdr_accept_language = strbuf_detach(&buf, NULL);
>
>That looks like a roundabout way to say xstrdup().  The whole thing
>can be done like so:
>
>	rpc->hdr_accept_language = xstrdup_or_null(http_get_accept_language_header());
>
>And by doing so we kill another bug.  "struct rpc" is allocated on
>the stack without any initialization, so the new code leaves the
>hdr_accept_language member uninitialized.  Rather, we want to
>explicitly set NULL to the member when the new header is not in use.
>
>> +	}
>> +
>
>The memory ownership model for this new .hdr_accept_language member
>in the RPC struct seems to be that the struct owns the resource of
>the member.
>
>>  strbuf_addf(&buf, "Content-Type: application/x-%s-request", svc);
>>  rpc->hdr_content_type = strbuf_detach(&buf, NULL);
>> 
>> @@ -1400,6 +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;
>> 
>>  /*
>>  * Run the info/refs request and see if the server supports protocol
>> @@ -1418,6 +1431,9 @@ static int stateless_connect(const char *service_name)
>>  printf("\n");
>>  fflush(stdout);
>>  }
>> +	accept_language = http_get_accept_language_header();
>> +	if (accept_language)
>> +	rpc.hdr_accept_language = xstrfmt("%s", accept_language);
>
>And this is in line with that memory ownership model.
>
>>  rpc.service_name = service_name;
>>  rpc.service_url = xstrfmt("%s%s", url.buf, rpc.service_name);
>
>I however do not see anybody that actually freeing when rpc is
>done.
>
>Are we adding a new memory leak?  Shouldn't we be releasing the
>resources held in rpc.hdr_accept_language when rpc goes out of
>scope? 
Right. we should free it in the end of the method, like so:
        free(rpc.service_url);
        free(rpc.hdr_content_type);
        free(rpc.hdr_accept);
+      free(rpc.hdr_accept_language);
        free(rpc.protocol_header);
        free(rpc.buf);
        strbuf_release(&buf);

>
>> diff --git a/t/t5541-http-push-smart.sh b/t/t5541-http-push-smart.sh
>> index 2f09ff4fac6..4288a279e9e 100755
>> --- a/t/t5541-http-push-smart.sh
>> +++ b/t/t5541-http-push-smart.sh
>> @@ -80,6 +80,25 @@ test_expect_success 'push to remote repository (standard)' '
>>  test $HEAD = $(git rev-parse --verify HEAD))
>>  '
>> 
>> +test_expect_success 'push to remote repository (standard) with sending Accept-Language' '
>> +	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
>
>As I already asked, do we need to use a language code that has never
>been used in our existing test to test this new codepath, or is it
>sufficient to reuse what we already know that will not cause problems
>in developers' testing environment, like those used in other
>existing tests, like ko_KR, en_US, etc.  If the latter, I strongly
>do not want to see a new language added to the test.  We are *not*
>in the business of testing the system locale support on the user's
>platform. 
OK. 
>
>> +	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 &&
>
>If this test, or existing tests in other scripts, do not actually
>require the LANGUAGE specified in the environment variable to be
>"installed" on the user's platform, then it might be an acceptable
>alternative to use a locale (like "tlh_AQ") that is implausible to
>exist on the user's system, but using what we already use in other
>tests would be the safest thing to do.
>
>Use ko_KR.UTF8 (and nothing else) like 5550 does with its first use
>of check_language helper.  Or using en_US is also fine, as that is
>also used over there. 
OK.
>
>> +	! grep "Expect: 100-continue" err &&
>> +
>> +	grep "=> Send header: Accept-Language:" err >err.language &&
>> +	test_cmp exp err.language
>> +'
>> +
>>  test_expect_success 'push already up-to-date' '
>>  git push
>>  '
>
>As I already said, I do not think changes to the following two tests
>are warranted.
>
>> diff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh
>> diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh 
Well, after I made some tests, the reason t5551 fail to test what we want is
"if test "$GIT_TEST_PROTOCOL_VERSION" = 2" this statement block the real
test.
>
>
>Thanks.