From: lilinchao@oschina.cn Date: Fri, 10 Jun 2022 03:49:44 GMT Subject: Re: Re: [PATCH v2] remote-curl: send Accept-Language header to server Message-ID: <2022061011484327929877@oschina.cn> In-Reply-To: >"Li Linchao via GitGitGadget" writes: > >> From: Li Linchao >> >> 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.