| Message ID | pull.1251.v2.git.1654756523475.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] remote-curl: send Accept-Language header to server | expand |
"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. > only used by very early phase of the transfer, that's HTTP GET request to "that's" -> "which is", probably. > 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. > 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? > 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. > + 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. > + ! 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 Thanks.
>"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.
>>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. Fix: '"$GIT_TEST_PROTOCOL_VERSION" = 0' >> >> >>Thanks.
diff --git a/http.c b/http.c index 11c6f69facd..33301d8d5d5 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 *http_get_accept_language_header(void) { if (!cached_accept_language) { struct strbuf buf = STRBUF_INIT; @@ -1829,7 +1829,7 @@ static int http_request(const char *url, fwrite_buffer); } - accept_language = get_accept_language(); + accept_language = http_get_accept_language_header(); if (accept_language) headers = curl_slist_append(headers, accept_language); 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); + struct http_pack_request { char *url; diff --git a/remote-curl.c b/remote-curl.c index 67f178b1120..504bbdedbda 100644 --- a/remote-curl.c +++ b/remote-curl.c @@ -580,6 +580,7 @@ struct rpc_state { char *service_url; char *hdr_content_type; char *hdr_accept; + char *hdr_accept_language; char *protocol_header; char *buf; size_t alloc; @@ -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); + /* Add the extra Git-Protocol header */ if (rpc->protocol_header) headers = curl_slist_append(headers, rpc->protocol_header); @@ -1058,6 +1063,7 @@ static int rpc_service(struct rpc_state *rpc, struct discovery *heads, struct strbuf *rpc_result) { const char *svc = rpc->service_name; + const char *accept_language; struct strbuf buf = STRBUF_INIT; struct child_process client = CHILD_PROCESS_INIT; int err = 0; @@ -1080,6 +1086,12 @@ static int rpc_service(struct rpc_state *rpc, struct discovery *heads, strbuf_addf(&buf, "%s%s", url.buf, svc); rpc->service_url = strbuf_detach(&buf, NULL); + accept_language = http_get_accept_language_header(); + if (accept_language) { + strbuf_addstr(&buf, accept_language); + rpc->hdr_accept_language = strbuf_detach(&buf, NULL); + } + 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); rpc.service_name = service_name; rpc.service_url = xstrfmt("%s%s", url.buf, rpc.service_name); 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 + + 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 && + ! 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 ' diff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh index f0d9cd584d3..42dd9fe2af7 100755 --- a/t/t5550-http-fetch-dumb.sh +++ b/t/t5550-http-fetch-dumb.sh @@ -358,18 +358,18 @@ test_expect_success 'git client sends Accept-Language correctly with unordinary check_language "ko-KR, en-US;q=0.9, *;q=0.8" "ko_KR::en_US" && check_language "ko-KR, *;q=0.9" ":::ko_KR" && check_language "ko-KR, en-US;q=0.9, *;q=0.8" "ko_KR!!:en_US" && - check_language "ko-KR, ja-JP;q=0.9, *;q=0.8" "ko_KR en_US:ja_JP"' + check_language "ko-KR, ja-JP;q=0.9, zh-CN;q=0.8, *;q=0.7" "ko_KR en_US:ja_JP:zh_CN"' test_expect_success 'git client sends Accept-Language with many preferred languages' ' - check_language "ko-KR, en-US;q=0.9, fr-CA;q=0.8, de;q=0.7, sr;q=0.6, \ -ja;q=0.5, zh;q=0.4, sv;q=0.3, pt;q=0.2, *;q=0.1" \ - ko_KR.EUC-KR:en_US.UTF-8:fr_CA:de.UTF-8@euro:sr@latin:ja:zh:sv:pt && + check_language "ko-KR, en-US;q=0.99, fr-CA;q=0.98, de;q=0.97, sr;q=0.96, \ +ja;q=0.95, zh;q=0.94, sv;q=0.93, pt;q=0.92, zh-CN;q=0.91, *;q=0.90" \ + ko_KR.EUC-KR:en_US.UTF-8:fr_CA:de.UTF-8@euro:sr@latin:ja:zh:sv:pt:zh_CN && check_language "ko-KR, en-US;q=0.99, fr-CA;q=0.98, de;q=0.97, sr;q=0.96, \ ja;q=0.95, zh;q=0.94, sv;q=0.93, pt;q=0.92, nb;q=0.91, *;q=0.90" \ ko_KR.EUC-KR:en_US.UTF-8:fr_CA:de.UTF-8@euro:sr@latin:ja:zh:sv:pt:nb ' -test_expect_success 'git client does not send an empty Accept-Language' ' +test_expect_success 'git client send an empty Accept-Language' ' GIT_TRACE_CURL=true LANGUAGE= git ls-remote "$HTTPD_URL/dumb/repo.git" 2>stderr && ! grep "^=> Send header: Accept-Language:" stderr ' diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh index b9351a732f6..6f65131a4e4 100755 --- a/t/t5551-http-fetch-smart.sh +++ b/t/t5551-http-fetch-smart.sh @@ -31,6 +31,7 @@ test_expect_success 'clone http repository' ' > GET /smart/repo.git/info/refs?service=git-upload-pack HTTP/1.1 > Accept: */* > Accept-Encoding: ENCODINGS + > Accept-Language: zh-CN, en;q=0.9, *;q=0.8 > Pragma: no-cache < HTTP/1.1 200 OK < Pragma: no-cache @@ -40,13 +41,15 @@ test_expect_success 'clone http repository' ' > Accept-Encoding: ENCODINGS > Content-Type: application/x-git-upload-pack-request > Accept: application/x-git-upload-pack-result + > Accept-Language: zh-CN, en;q=0.9, *;q=0.8 > Content-Length: xxx < HTTP/1.1 200 OK < Pragma: no-cache < Cache-Control: no-cache, max-age=0, must-revalidate < Content-Type: application/x-git-upload-pack-result EOF - GIT_TRACE_CURL=true GIT_TEST_PROTOCOL_VERSION=0 \ + + GIT_TRACE_CURL=true GIT_TEST_PROTOCOL_VERSION=0 LANGUAGE="zh_CN:en" \ git clone --quiet $HTTPD_URL/smart/repo.git clone 2>err && test_cmp file clone/file && tr '\''\015'\'' Q <err | @@ -94,7 +97,10 @@ test_expect_success 'clone http repository' ' test_cmp exp actual.smudged && grep "Accept-Encoding:.*gzip" actual >actual.gzip && - test_line_count = 2 actual.gzip + test_line_count = 2 actual.gzip && + + grep "Accept-Language: zh-CN, en" actual >actual.language && + test_line_count = 2 actual.language fi '