>"Li Linchao via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes:
>
>> From: Li Linchao <lilinchao@xxxxxxxxxx>
>>
>> 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.
