remote-curl: accept all encoding supported by curl
PATCH 1/2 remote-curl: accept all encoding supported by curl
Configure curl to accept all encoding which curl supports instead of only accepting gzip responses.
This is necessary to fix a bug when using an installation of curl which doesn't support gzip. Since curl doesn't do any checking to verify that it supports the encoding set when calling 'curl_easy_setopt()', curl can end up sending an "Accept-Encoding" header indicating that it supports a particular encoding when in fact it doesn't. Instead when the empty string "" is used when setting CURLOPT_ENCODING, curl will send an "Accept-Encoding" header containing only the encoding methods curl supports.
--- http.c | 2 +- remote-curl.c | 2 +- t/t5551-http-fetch-smart.sh | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-)
http.c +1 -1
@@ -1788,7 +1788,7 @@ static int http_request(const char *url,curl_easy_setopt (slot->curl,CURLOPT_URL , url);curl_easy_setopt (slot->curl,CURLOPT_HTTPHEADER , headers); -curl_easy_setopt (slot->curl,CURLOPT_ENCODING ,"gzip" ); +curl_easy_setopt (slot->curl,CURLOPT_ENCODING ,"" ); ret =run_one_slot (slot, &results);
remote-curl.c +1 -1
@@ -684,7 +684,7 @@ static int post_rpc(struct rpc_state *rpc)curl_easy_setopt (slot->curl,CURLOPT_NOBODY ,0 );curl_easy_setopt (slot->curl,CURLOPT_POST ,1 );curl_easy_setopt (slot->curl,CURLOPT_URL , rpc->service_url); -curl_easy_setopt (slot->curl,CURLOPT_ENCODING ,"gzip" ); +curl_easy_setopt (slot->curl,CURLOPT_ENCODING ,"" );if (large_request) {/* The request body is large and the size cannot be predicted.
t/t5551-http-fetch-smart.sh +2 -2
@@ -26,14 +26,14 @@ setup_askpass_helper cat >exp <<EOF > GET /smart/repo.git/info/refs?service=git-upload-pack HTTP/1.1 > Accept: */* -> Accept-Encoding: gzip +> Accept-Encoding: deflate, gzip > Pragma: no-cache < HTTP/1.1 200 OK < Pragma: no-cache < Cache-Control: no-cache, max-age=0, must-revalidate < Content-Type: application/x-git-upload-pack-advertisement > POST /smart/repo.git/git-upload-pack HTTP/1.1 -> Accept-Encoding: gzip +> Accept-Encoding: deflate, gzip > Content-Type: application/x-git-upload-pack-request > Accept: application/x-git-upload-pack-result > Content-Length: xxx
Signature · 1 line
2.17.0.441.gb46fe60e1d-goog
PATCH 2/2 remote-curl: accept compressed responses with protocol v2
Configure curl to accept compressed responses when using protocol v2 by setting CURLOPT_ENCODING to "", which indicates that curl should send an "Accept-Encoding" header with all supported compression encodings.
--- remote-curl.c | 1 + 1 file changed, 1 insertion(+)
remote-curl.c +1 -0
@@ -1259,6 +1259,7 @@ static int proxy_request(struct proxy_state *p) slot =get_active_slot (); +curl_easy_setopt (slot->curl,CURLOPT_ENCODING ,"" );curl_easy_setopt (slot->curl,CURLOPT_NOBODY ,0 );curl_easy_setopt (slot->curl,CURLOPT_POST ,1 );curl_easy_setopt (slot->curl,CURLOPT_URL , p->service_url);
Signature · 1 line
2.17.0.441.gb46fe60e1d-goog
Hi,
Quoted 1 line · Brandon Williams
Subject: remote-curl: accept all encoding supported by curl
nit: s/encoding/encodings
Quoted 2 lines
Configure curl to accept all encoding which curl supports instead of only accepting gzip responses.
Likewise.
Quoted 10 lines
This is necessary to fix a bug when using an installation of curl which doesn't support gzip. Since curl doesn't do any checking to verify that it supports the encoding set when calling '
curl_easy_setopt()', curl can end up sending an "Accept-Encoding" header indicating that it supports a particular encoding when in fact it doesn't. Instead when the empty string "" is used when settingCURLOPT_ENCODING, curl will send an "Accept-Encoding" header containing only the encoding methods curl supports.Signed-off-by Brandon Williams <[email protected]>
Thanks for the analysis and fix.
Also ccing the reporter so we can hopefully get a tested-by. Anton, can you test this patch and let us know how it goes? You can apply it as follows:
curl \
https://public-inbox.org/git/[email protected]/raw \
>patch.txt
git am -3 patch.txtBrandon, can the commit message also say a little more about the motivating context and symptoms?
$ curl --version curl 7.52.1 (arm-openwrt-linux-gnu) libcurl/7.52.1 mbedTLS/2.6.0 Protocols: file ftp ftps http https Features: IPv6 Largefile SSL
The issue is that when curl is built without the "zlib" feature, since v1.8.0-rc0~14^2 (Enable info/refs gzip decompression in HTTP client, 2012-09-19) we end up requesting "gzip" encoding anyway despite libcurl not being able to decode it. Worse, instead of getting a clear error message indicating so, we end up falling back to "dumb" http, producing a confusing and difficult to debug result.
Quoted 24 lines
--- http.c | 2 +- remote-curl.c | 2 +- t/t5551-http-fetch-smart.sh | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-)http.c +1
-1@@ -1788,7 +1788,7 @@ static int http_request(const char *url,curl_easy_setopt (slot->curl,CURLOPT_URL , url);curl_easy_setopt (slot->curl,CURLOPT_HTTPHEADER , headers); -curl_easy_setopt (slot->curl,CURLOPT_ENCODING ,"gzip" ); +curl_easy_setopt (slot->curl,CURLOPT_ENCODING ,"" ); ret =run_one_slot (slot, &results);remote-curl.c +0
-0
remote-curl.c static int post_rpc(struct rpc_state *rpc) · line 690
Quoted 8 lines
remote-curl.c +1
-1@@ -684,7 +684,7 @@ static int post_rpc(struct rpc_state *rpc)curl_easy_setopt (slot->curl,CURLOPT_NOBODY ,0 );curl_easy_setopt (slot->curl,CURLOPT_POST ,1 );curl_easy_setopt (slot->curl,CURLOPT_URL , rpc->service_url); -curl_easy_setopt (slot->curl,CURLOPT_ENCODING ,"gzip" ); +curl_easy_setopt (slot->curl,CURLOPT_ENCODING ,"" );if (large_request) {/* The request body is large and the size cannot be predicted.
Makes sense.
Quoted 11 lines
t/t5551-http-fetch-smart.sh +1
-1@@ -26,14 +26,14 @@ setup_askpass_helper cat >exp <<EOF > GET /smart/repo.git/info/refs?service=git-upload-pack HTTP/1.1 > Accept: */* -> Accept-Encoding: gzip +> Accept-Encoding: deflate, gzip > Pragma: no-cache
t/t5551-http-fetch-smart.sh setup_askpass_helper · line 39
Quoted 10 lines
t/t5551-http-fetch-smart.sh +1
-1< HTTP/1.1 200 OK < Pragma: no-cache < Cache-Control: no-cache, max-age=0, must-revalidate < Content-Type: application/x-git-upload-pack-advertisement > POST /smart/repo.git/git-upload-pack HTTP/1.1 -> Accept-Encoding: gzip +> Accept-Encoding: deflate, gzip > Content-Type: application/x-git-upload-pack-request > Accept: application/x-git-upload-pack-result > Content-Length: xxx
If libcurl gains support for another encoding in the future, this test would start failing. Can we make the matching less strict? For example, how about something like the following for squashing in?
Thanks,
Jonathan
t/t5551-http-fetch-smart.sh +9 -4
@@ -26,14 +26,14 @@ setup_askpass_helper cat >exp <<EOF > GET /smart/repo.git/info/refs?service=git-upload-pack HTTP/1.1 > Accept: */* -> Accept-Encoding: deflate, gzip +> Accept-Encoding: ENCODINGS > Pragma: no-cache < HTTP/1.1 200 OK < Pragma: no-cache < Cache-Control: no-cache, max-age=0, must-revalidate < Content-Type: application/x-git-upload-pack-advertisement > POST /smart/repo.git/git-upload-pack HTTP/1.1 -> Accept-Encoding: deflate, gzip +> Accept-Encoding: ENCODINGS > Content-Type: application/x-git-upload-pack-request > Accept: application/x-git-upload-pack-result > Content-Length: xxx
@@ -79,8 +79,13 @@ test_expect_success 'clone http repository' ' /^< Date: /d /^< Content-Length: /d /^< Transfer-Encoding: /d - " >act && - test_cmp exp act + " >actual && + sed -e "s/^> Accept-Encoding: .*/> Accept-Encoding: ENCODINGS/" \ + actual >actual.smudged && + test_cmp exp actual.smudged && + + grep "Accept-Encoding:.*gzip" actual >actual.gzip && + test_line_count = 2 actual.gzip ' test_expect_success 'fetch changes via http' '
Hi Jonathan,
I'd like to confirm, that your patch fixes my problem: I can sync with google repository now using git and curl version without gzip support. Do you know how this patch is going to be released? Just HEAD now and GA in the next planned release?
Communication looks like follow now:
root@bsb:~# GIT_TRACE_PACKET=1 GIT_CURL_VERBOSE=1 git ls-remote https://source.developers.google.co m/p/wired-balm-187912/r/dotfiles 2>&1 | sed -e 's/\(git-[^=]*\)=.*/\1=REDACTED/' -e 's/Authorizatio n: .*/Authorization: REDACTED/'
Quoted 1 line
GET /p/wired-balm-187912/r/dotfiles/info/refs?service=git-upload-pack
HTTP/1.1
Host: source.developers.google.com
User-Agent: git/2.17.0
Accept: */*
Accept-Encoding: identity
Cookie: o=git-anton.golubev.gmail.com=REDACTED
Pragma: no-cache
< HTTP/1.1 200 OK
< Cache-Control: no-cache, max-age=0, must-revalidate
< Content-Length: 374
< Content-Type: application/x-git-upload-pack-advertisement < Expires: Fri, 01 Jan 1980 00:00:00 GMT
< Pragma: no-cache
< X-Content-Type-Options: nosniff
< X-Frame-Options: SAMEORIGIN
< X-Xss-Protection: 1; mode=block
< Date: Sat, 26 May 2018 11:04:41 GMT
< Alt-Svc: hq=":443"; ma=2592000; quic=51303433; quic=51303432; quic=51303431; quic=51303339; quic=51303335,quic=":443"; ma=2592000; v="43,42,41,39,35"
<
13:04:41.274561 pkt-line.c:80 packet: git< #
service=git-upload-pack
13:04:41.274634 pkt-line.c:80 packet: git< 0000 13:04:41.274693 pkt-line.c:80 packet: git<
45e2c99dd1790529cc4b7e029b1e9dfcc817d18e HEAD\0 include-tag multi_ack_detailed multi_ack ofs-delta side-band side-band-64k thin-pack no-progress shallow no-done allow-tip-sha1-in-want allow-reachable-sha1-in-want agent=JGit/4-google filter symref=HEAD:refs/heads/master
13:04:41.274739 pkt-line.c:80 packet: git<45e2c99dd1790529cc4b7e029b1e9dfcc817d18e refs/heads/master
13:04:41.274777 pkt-line.c:80 packet: git< 000045e2c99dd1790529cc4b7e029b1e9dfcc817d18e HEAD45e2c99dd1790529cc4b7e029b1e9dfcc817d18e refs/heads/master
Kind regards,
Anton Golubev
-----Original Message-----
From: Jonathan Nieder [mailto:[email protected]]
Sent: Dienstag, 22. Mai 2018 02:01
To: Brandon Williams <[email protected]>
Cc: [email protected]; Anton Golubev <[email protected]>
Subject: Re: [PATCH 1/2] remote-curl: accept all encoding supported by curl
Hi,
Quoted 1 line · Brandon Williams
Subject: remote-curl: accept all encoding supported by curl
nit: s/encoding/encodings
Quoted 2 lines
Configure curl to accept all encoding which curl supports instead of only accepting gzip responses.
Likewise.
Quoted 10 lines
This is necessary to fix a bug when using an installation of curl which doesn't support gzip. Since curl doesn't do any checking to verify that it supports the encoding set when calling '
curl_easy_setopt()', curl can end up sending an "Accept-Encoding" header indicating that it supports a particular encoding when in fact it doesn't. Instead when the empty string "" is used when settingCURLOPT_ENCODING, curl will send an "Accept-Encoding" header containing only the encoding methods curl supports.Signed-off-by Brandon Williams <[email protected]>
Thanks for the analysis and fix.
Also ccing the reporter so we can hopefully get a tested-by. Anton, can you test this patch and let us know how it goes? You can apply it as follows:
curl \
https://public-inbox.org/git/[email protected]/raw \
Quoted 1 line
patch.txt
git am -3 patch.txt
Brandon, can the commit message also say a little more about the motivating context and symptoms?
$ curl --version curl 7.52.1 (arm-openwrt-linux-gnu) libcurl/7.52.1 mbedTLS/2.6.0 Protocols: file ftp ftps http https Features: IPv6 Largefile SSL
The issue is that when curl is built without the "zlib" feature, since v1.8.0-rc0~14^2 (Enable info/refs gzip decompression in HTTP client, 2012-09-19) we end up requesting "gzip" encoding anyway despite libcurl not being able to decode it. Worse, instead of getting a clear error message indicating so, we end up falling back to "dumb"
http, producing a confusing and difficult to debug result.
Quoted 24 lines
--- http.c | 2 +- remote-curl.c | 2 +- t/t5551-http-fetch-smart.sh | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-)http.c +1
-1@@ -1788,7 +1788,7 @@ static int http_request(const char *url,curl_easy_setopt (slot->curl,CURLOPT_URL , url);curl_easy_setopt (slot->curl,CURLOPT_HTTPHEADER , headers); -curl_easy_setopt (slot->curl,CURLOPT_ENCODING ,"gzip" ); +curl_easy_setopt (slot->curl,CURLOPT_ENCODING ,"" ); ret =run_one_slot (slot, &results);remote-curl.c index ceb05347b..565bba104 renamed from remote-curl.c +0
-0100644
remote-curl.c static int post_rpc(struct rpc_state *rpc) · line 690
Quoted 8 lines
remote-curl.c +1
-1@@ -684,7 +684,7 @@ static int post_rpc(struct rpc_state *rpc)curl_easy_setopt (slot->curl,CURLOPT_NOBODY ,0 );curl_easy_setopt (slot->curl,CURLOPT_POST ,1 );curl_easy_setopt (slot->curl,CURLOPT_URL , rpc->service_url); -curl_easy_setopt (slot->curl,CURLOPT_ENCODING ,"gzip" ); +curl_easy_setopt (slot->curl,CURLOPT_ENCODING ,"" );if (large_request) {/* The request body is large and the size cannot be
predicted.
Makes sense.
Quoted 9 lines
t/t5551-http-fetch-smart.sh +0
-0@@ -26,14 +26,14 @@ setup_askpass_helper cat >exp <<EOF > GET/smart/repo.git/info/refs?service=git-upload-pack HTTP/1.1 > Accept:
*/*
-> Accept-Encoding: gzip
+> Accept-Encoding: deflate, gzip
Quoted 11 lines
Quoted 1 line
Pragma: no-cache
< HTTP/1.1 200 OK
< Pragma: no-cache
< Cache-Control: no-cache, max-age=0, must-revalidate <
Content-Type: application/x-git-upload-pack-advertisementQuoted 1 line
POST /smart/repo.git/git-upload-pack HTTP/1.1
-> Accept-Encoding: gzip
+> Accept-Encoding: deflate, gzipQuoted 3 lines
Content-Type: application/x-git-upload-pack-request
Accept: application/x-git-upload-pack-result
Content-Length: xxx
If libcurl gains support for another encoding in the future, this test would start failing. Can we make the matching less strict? For example, how about something like the following for squashing in?
Thanks,
Jonathan
w/t/t5551-http-fetch-smart.sh index renamed from i/t/t5551-http-fetch-smart.sh +0 -0
39c65482ce..913089b144 100755
t/t5551-http-fetch-smart.sh +0 -0
@@ -26,14 +26,14 @@ setup_askpass_helper cat >exp <<EOF > GET
/smart/repo.git/info/refs?service=git-upload-pack HTTP/1.1 > Accept: */* -> Accept-Encoding: deflate, gzip
+> Accept-Encoding: ENCODINGS
Quoted 1 line
Pragma: no-cache
< HTTP/1.1 200 OK
< Pragma: no-cache
< Cache-Control: no-cache, max-age=0, must-revalidate < Content-Type:
application/x-git-upload-pack-advertisement
Quoted 1 line
POST /smart/repo.git/git-upload-pack HTTP/1.1
-> Accept-Encoding: deflate, gzip
+> Accept-Encoding: ENCODINGS
Quoted 3 lines
Content-Type: application/x-git-upload-pack-request
Accept: application/x-git-upload-pack-result
Content-Length: xxx
@@ -79,8 +79,13 @@ test_expect_success 'clone http repository' '
/^< Date: /d
/^< Content-Length: /d
/^< Transfer-Encoding: /d
- " >act &&
- test_cmp exp act
+ " >actual &&
+ sed -e "s/^> Accept-Encoding: .*/> Accept-Encoding: ENCODINGS/" \
+ actual >actual.smudged &&
+ test_cmp exp actual.smudged &&
+
+ grep "Accept-Encoding:.*gzip" actual >actual.gzip &&
+ test_line_count = 2 actual.gzip
'
test_expect_success 'fetch changes via http' '
Quoted 2 lines · Brandon Williams
Configure curl to accept all encoding which curl supports instead of only accepting gzip responses.
This partially reverts aa90b9697f9 (Enable info/refs gzip decompression in HTTP client, 2012-09-19), as that specifically called out deflate not being a good option. Is that worth mentioning in the commit message?
Quoted 34 lines
This is necessary to fix a bug when using an installation of curl which doesn't support gzip. Since curl doesn't do any checking to verify that it supports the encoding set when calling '
curl_easy_setopt()', curl can end up sending an "Accept-Encoding" header indicating that it supports a particular encoding when in fact it doesn't. Instead when the empty string "" is used when settingCURLOPT_ENCODING, curl will send an "Accept-Encoding" header containing only the encoding methods curl supports.Signed-off-by Brandon Williams <[email protected]>--- http.c | 2 +- remote-curl.c | 2 +- t/t5551-http-fetch-smart.sh | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-)http.c +1
-1@@ -1788,7 +1788,7 @@ static int http_request(const char *url,curl_easy_setopt (slot->curl,CURLOPT_URL , url);curl_easy_setopt (slot->curl,CURLOPT_HTTPHEADER , headers); -curl_easy_setopt (slot->curl,CURLOPT_ENCODING ,"gzip" ); +curl_easy_setopt (slot->curl,CURLOPT_ENCODING ,"" ); ret =run_one_slot (slot, &results);remote-curl.c +0
-0
remote-curl.c static int post_rpc(struct rpc_state *rpc) · line 687
Quoted 5 lines
remote-curl.c +1
-1@@ -684,7 +684,7 @@ static int post_rpc(struct rpc_state *rpc)curl_easy_setopt (slot->curl,CURLOPT_NOBODY ,0 );curl_easy_setopt (slot->curl,CURLOPT_POST ,1 );curl_easy_setopt (slot->curl,CURLOPT_URL , rpc->service_url); -curl_easy_setopt (slot->curl,CURLOPT_ENCODING ,"gzip" ); +curl_easy_setopt (slot->curl,CURLOPT_ENCODING ,"" );
Looking at the code here, this succeeds if enough memory is available. There is no check if the given parameter is part of Curl_all_content_encodings();
https://github.com/curl/curl/blob/e66cca046cef20d00fba89260dfa6b4a3997233d/lib/setopt.c#L429
https://github.com/curl/curl/blob/c675c40295045d4988eeb6291c54eb48f138822f/lib/content_encoding.c#L686
which may be worth checking first?
Quoted 27 lines
remote-curl.c +0
-0if (large_request) {/* The request body is large and the size cannot be predicted. t/t5551-http-fetch-smart.sh +2
-3@@ -26,14 +26,14 @@ setup_askpass_helper cat >exp <<EOF > GET /smart/repo.git/info/refs?service=git-upload-pack HTTP/1.1 > Accept: */* -> Accept-Encoding: gzip +> Accept-Encoding: deflate, gzip > Pragma: no-cache < HTTP/1.1 200 OK < Pragma: no-cache < Cache-Control: no-cache, max-age=0, must-revalidate < Content-Type: application/x-git-upload-pack-advertisement > POST /smart/repo.git/git-upload-pack HTTP/1.1 -> Accept-Encoding: gzip +> Accept-Encoding: deflate, gzip > Content-Type: application/x-git-upload-pack-request > Accept: application/x-git-upload-pack-result > Content-Length: xxx --2.17.0.441.gb46fe60e1d-goog
Hi,
Quoted 8 lines · Stefan Beller
Quoted 2 lines · Brandon Williams
Configure curl to accept all encoding which curl supports instead of only accepting gzip responses.
This partially reverts
aa90b9697f9(Enable info/refs gzip decompression in HTTP client, 2012-09-19), as that specifically called out deflate not being a good option. Is that worth mentioning in the commit message?
More specifically, it mentions the wasted 9 extra bytes from including "deflate, " on the Accept-Encoding line. I think the extra bandwidth usage will be okay. :)
[...]
Quoted 10 lines
Review
Quoted 2 lines
(quoted diff) +1
-1- curl_easy_setopt(slot->curl, CURLOPT_ENCODING, "gzip"); + curl_easy_setopt(slot->curl, CURLOPT_ENCODING, "");Looking at the code here, this succeeds if enough memory is available. There is no check if the given parameter is part of
Curl_all_content_encodings();
https://github.com/curl/curl/blob/e66cca046cef20d00fba89260dfa6b4a3997233d/lib/setopt.c#L429
https://github.com/curl/curl/blob/c675c40295045d4988eeb6291c54eb48f138822f/lib/content_encoding.c#L686which may be worth checking first?
By "this" are you referring to the preimage or the postimage? Are you suggesting a change in git or in libcurl?
Curl_all_content_encodings() is an internal function in libcurl, so I'm assuming the latter.
Thanks,
Jonathan
Quoted 7 lines · Jonathan Nieder
Quoted 3 lines
Looking at the code here, this succeeds if enough memory is available. There is no check if the given parameter is part of
Curl_all_content_encodings();By "this" are you referring to the preimage or the postimage? Are you suggesting a change in git or in libcurl?
Curl_all_content_encodings()is an internal function in libcurl, so I'm assuming the latter.
Ack, that certainly isn't the most wonderful API for selecting a compression method. In reality, almost everyone sticks to passing on a "" to that option to let libcurl pick and ask for the compression algos it knows since both gzip and brotli are present only conditionally depending on build options.
I would agree that the libcurl setopt call should probably be made to fail if asked to use a compression method not built-in/supported. Then an application could in fact try different algos in order until one works or ask to disable compression completely.
In the generic HTTP case, it usually makes sense to ask for more than one algorthim though, since this is asking the server for a compressed version and typically a HTTP client doesn't know which compression methods the server offers. Not sure this is actually true to the same extent for git.
Signature · 1 line
/ daniel.haxx.se
Quoted 9 lines · Daniel Stenberg
Quoted 7 lines · Jonathan Nieder
Quoted 3 lines
Looking at the code here, this succeeds if enough memory is available. There is no check if the given parameter is part of
Curl_all_content_encodings();By "this" are you referring to the preimage or the postimage? Are you suggesting a change in git or in libcurl?
Quoted 7 lines
Quoted 2 lines
Curl_all_content_encodings()is an internal function in libcurl, so I'm assuming the latter.Ack, that certainly isn't the most wonderful API for selecting a compression method. In reality, almost everyone sticks to passing on a "" to that option to let libcurl pick and ask for the compression algos it knows since both gzip and brotli are present only conditionally depending on build options.
Thanks for the clarification. Sounds like the best option is to continue with this patch and let curl decide using "".
Quoted 14 lines
I would agree that the libcurl setopt call should probably be made to fail if asked to use a compression method not built-in/supported. Then an application could in fact try different algos in order until one works or ask to disable compression completely.
In the generic HTTP case, it usually makes sense to ask for more than one algorthim though, since this is asking the server for a compressed version and typically a HTTP client doesn't know which compression methods the server offers. Not sure this is actually true to the same extent for git.
Signature · 1 line
/ daniel.haxx.se
Signature · 1 line
Brandon Williams
PATCH v2 1/2 remote-curl: accept all encodings supported by curl
In reply to Brandon WilliamsConfigure curl to accept all encodings which curl supports instead of only accepting gzip responses.
This fixes an issue when using an installation of curl which is built without the "zlib" feature. Since aa90b9697 (Enable info/refs gzip decompression in HTTP client, 2012-09-19) we end up requesting "gzip" encoding anyway despite libcurl not being able to decode it. Worse, instead of getting a clear error message indicating so, we end up falling back to "dumb" http, producing a confusing and difficult to debug result.
Since curl doesn't do any checking to verify that it supports the a requested encoding, instead set the curl option CURLOPT_ENCODING with an empty string indicating that curl should send an "Accept-Encoding" header containing only the encodings supported by curl.
---
Version 2 of this series just tweaks the commit message and the test per Jonathan's suggestion.
http.c | 2 +- remote-curl.c | 2 +- t/t5551-http-fetch-smart.sh | 13 +++++++++---- 3 files changed, 11 insertions(+), 6 deletions(-)
http.c +1 -1
@@ -1788,7 +1788,7 @@ static int http_request(const char *url,curl_easy_setopt (slot->curl,CURLOPT_URL , url);curl_easy_setopt (slot->curl,CURLOPT_HTTPHEADER , headers); -curl_easy_setopt (slot->curl,CURLOPT_ENCODING ,"gzip" ); +curl_easy_setopt (slot->curl,CURLOPT_ENCODING ,"" ); ret =run_one_slot (slot, &results);
remote-curl.c +1 -1
@@ -684,7 +684,7 @@ static int post_rpc(struct rpc_state *rpc)curl_easy_setopt (slot->curl,CURLOPT_NOBODY ,0 );curl_easy_setopt (slot->curl,CURLOPT_POST ,1 );curl_easy_setopt (slot->curl,CURLOPT_URL , rpc->service_url); -curl_easy_setopt (slot->curl,CURLOPT_ENCODING ,"gzip" ); +curl_easy_setopt (slot->curl,CURLOPT_ENCODING ,"" );if (large_request) {/* The request body is large and the size cannot be predicted.
t/t5551-http-fetch-smart.sh +9 -4
@@ -26,14 +26,14 @@ setup_askpass_helper cat >exp <<EOF > GET /smart/repo.git/info/refs?service=git-upload-pack HTTP/1.1 > Accept: */* -> Accept-Encoding: gzip +> Accept-Encoding: ENCODINGS > Pragma: no-cache < HTTP/1.1 200 OK < Pragma: no-cache < Cache-Control: no-cache, max-age=0, must-revalidate < Content-Type: application/x-git-upload-pack-advertisement > POST /smart/repo.git/git-upload-pack HTTP/1.1 -> Accept-Encoding: gzip +> Accept-Encoding: ENCODINGS > Content-Type: application/x-git-upload-pack-request > Accept: application/x-git-upload-pack-result > Content-Length: xxx
@@ -79,8 +79,13 @@ test_expect_success 'clone http repository' ' /^< Date: /d /^< Content-Length: /d /^< Transfer-Encoding: /d - " >act && - test_cmp exp act + " >actual && + sed -e "s/^> Accept-Encoding: .*/> Accept-Encoding: ENCODINGS/" \ + actual >actual.smudged && + test_cmp exp actual.smudged && + + grep "Accept-Encoding:.*gzip" actual >actual.gzip && + test_line_count = 2 actual.gzip ' test_expect_success 'fetch changes via http' '
Signature · 1 line
2.17.0.441.gb46fe60e1d-goog
PATCH v2 2/2 remote-curl: accept compressed responses with protocol v2
Configure curl to accept compressed responses when using protocol v2 by setting CURLOPT_ENCODING to "", which indicates that curl should send an "Accept-Encoding" header with all supported compression encodings.
--- remote-curl.c | 1 + 1 file changed, 1 insertion(+)
remote-curl.c +1 -0
@@ -1259,6 +1259,7 @@ static int proxy_request(struct proxy_state *p) slot =get_active_slot (); +curl_easy_setopt (slot->curl,CURLOPT_ENCODING ,"" );curl_easy_setopt (slot->curl,CURLOPT_NOBODY ,0 );curl_easy_setopt (slot->curl,CURLOPT_POST ,1 );curl_easy_setopt (slot->curl,CURLOPT_URL , p->service_url);
Signature · 1 line
2.17.0.441.gb46fe60e1d-goog
Re: [PATCH v2 2/2] remote-curl: accept compressed responses with protocol v2
Quoted 8 lines · Brandon Williams
Configure curl to accept compressed responses when using protocol v2 by setting
CURLOPT_ENCODINGto "", which indicates that curl should send an "Accept-Encoding" header with all supported compression encodings.Signed-off-by Brandon Williams <[email protected]>--- remote-curl.c | 1 + 1 file changed, 1 insertion(+)
Yay!
remote-curl.c static int proxy_request(struct proxy_state *p) · line 1265
Quoted 12 lines
remote-curl.c +1
-0@@ -1259,6 +1259,7 @@ static int proxy_request(struct proxy_state *p) slot =get_active_slot (); +curl_easy_setopt (slot->curl,CURLOPT_ENCODING ,"" );curl_easy_setopt (slot->curl,CURLOPT_NOBODY ,0 );curl_easy_setopt (slot->curl,CURLOPT_POST ,1 );curl_easy_setopt (slot->curl,CURLOPT_URL , p->service_url);
Can this get a test?
I'm particularly interested in it since it's easy to accidentally apply this patch to the wrong duplicated place (luckily 'p' is a different variable name than 'rpc' but it's an easy mistake to make if applying the patch manually).
With or without such a test,
Re: [PATCH v2 1/2] remote-curl: accept all encodings supported by curl
In reply to Brandon WilliamsQuoted 11 lines · Brandon Williams
Configure curl to accept all encodings which curl supports instead of only accepting gzip responses.
This fixes an issue when using an installation of curl which is built without the "zlib" feature. Since
aa90b9697(Enable info/refs gzip decompression in HTTP client, 2012-09-19) we end up requesting "gzip" encoding anyway despite libcurl not being able to decode it. Worse, instead of getting a clear error message indicating so, we end up falling back to "dumb" http, producing a confusing and difficult to debug result.
Quoted 4 lines
Since curl doesn't do any checking to verify that it supports the a requested encoding, instead set the curl option
CURLOPT_ENCODINGwith an empty string indicating that curl should send an "Accept-Encoding" header containing only the encodings supported by curl.
Even better, this means we get the benefit of future of even better compression algorithms once libcurl learns them.
Quoted 10 lines
Reported-by Anton Golubev <[email protected]>Signed-off-by Brandon Williams <[email protected]>---
Version 2 of this series just tweaks the commit message and the test per Jonathan's suggestion.http.c | 2 +- remote-curl.c | 2 +- t/t5551-http-fetch-smart.sh | 13 +++++++++---- 3 files changed, 11 insertions(+), 6 deletions(-)
Thanks for fixing it.
Patch left unsnipped for reference.
Quoted 61 lines
http.c +1
-1@@ -1788,7 +1788,7 @@ static int http_request(const char *url,curl_easy_setopt (slot->curl,CURLOPT_URL , url);curl_easy_setopt (slot->curl,CURLOPT_HTTPHEADER , headers); -curl_easy_setopt (slot->curl,CURLOPT_ENCODING ,"gzip" ); +curl_easy_setopt (slot->curl,CURLOPT_ENCODING ,"" ); ret =run_one_slot (slot, &results);remote-curl.c +1
-1@@ -684,7 +684,7 @@ static int post_rpc(struct rpc_state *rpc)curl_easy_setopt (slot->curl,CURLOPT_NOBODY ,0 );curl_easy_setopt (slot->curl,CURLOPT_POST ,1 );curl_easy_setopt (slot->curl,CURLOPT_URL , rpc->service_url); -curl_easy_setopt (slot->curl,CURLOPT_ENCODING ,"gzip" ); +curl_easy_setopt (slot->curl,CURLOPT_ENCODING ,"" );if (large_request) {/* The request body is large and the size cannot be predicted. t/t5551-http-fetch-smart.sh +9
-4@@ -26,14 +26,14 @@ setup_askpass_helper cat >exp <<EOF > GET /smart/repo.git/info/refs?service=git-upload-pack HTTP/1.1 > Accept: */* -> Accept-Encoding: gzip +> Accept-Encoding: ENCODINGS > Pragma: no-cache < HTTP/1.1 200 OK < Pragma: no-cache < Cache-Control: no-cache, max-age=0, must-revalidate < Content-Type: application/x-git-upload-pack-advertisement > POST /smart/repo.git/git-upload-pack HTTP/1.1 -> Accept-Encoding: gzip +> Accept-Encoding: ENCODINGS > Content-Type: application/x-git-upload-pack-request > Accept: application/x-git-upload-pack-result > Content-Length: xxx@@ -79,8 +79,13 @@ test_expect_success 'clone http repository' ' /^< Date: /d /^< Content-Length: /d /^< Transfer-Encoding: /d - " >act && - test_cmp exp act + " >actual && + sed -e "s/^> Accept-Encoding: .*/> Accept-Encoding: ENCODINGS/" \ + actual >actual.smudged && + test_cmp exp actual.smudged && + + grep "Accept-Encoding:.*gzip" actual >actual.gzip && + test_line_count = 2 actual.gzip ' test_expect_success 'fetch changes via http' '
Re: [PATCH v2 1/2] remote-curl: accept all encodings supported by curl
In reply to Brandon Williamst/t5551-http-fetch-smart.sh setup_askpass_helper · line 30
Quoted 11 lines · Brandon Williams
t/t5551-http-fetch-smart.sh +1
-1@@ -26,14 +26,14 @@ setup_askpass_helper cat >exp <<EOF > GET /smart/repo.git/info/refs?service=git-upload-pack HTTP/1.1 > Accept: */* -> Accept-Encoding: gzip +> Accept-Encoding: ENCODINGS > Pragma: no-cache
Is the ordering of these headers determined by the user of cURL library (i.e. Git), or whatever the version of cURL we happened to link with happens to produce?
The point is whether the order is expected to be stable, or we are better off sorting the actual log before comparing.
t/t5551-http-fetch-smart.sh setup_askpass_helper · line 34
Quoted 4 lines
t/t5551-http-fetch-smart.sh +0
-0< HTTP/1.1 200 OK < Pragma: no-cache < Cache-Control: no-cache, max-age=0, must-revalidate < Content-Type: application/x-git-upload-pack-advertisement
A similar question for this response.
t/t5551-http-fetch-smart.sh setup_askpass_helper · line 39
Quoted 6 lines
t/t5551-http-fetch-smart.sh +1
-1> POST /smart/repo.git/git-upload-pack HTTP/1.1 -> Accept-Encoding: gzip +> Accept-Encoding: ENCODINGS > Content-Type: application/x-git-upload-pack-request > Accept: application/x-git-upload-pack-result > Content-Length: xxx
Ditto for this request.
Quoted 4 lines
t/t5551-http-fetch-smart.sh +0
-0@@ -79,8 +79,13 @@ test_expect_success 'clone http repository' ' /^< Date: /d /^< Content-Length: /d /^< Transfer-Encoding: /d
t/t5551-http-fetch-smart.sh test_expect_success 'clone http repository' ' · line 89
Quoted 10 lines
t/t5551-http-fetch-smart.sh +7
-2- " >act && - test_cmp exp act + " >actual && + sed -e "s/^> Accept-Encoding: .*/> Accept-Encoding: ENCODINGS/" \ + actual >actual.smudged && + test_cmp exp actual.smudged && + + grep "Accept-Encoding:.*gzip" actual >actual.gzip && + test_line_count = 2 actual.gzip '
Similarly, how much control do we have to ensure that the test HTTPD server (1) supports gzip and (2) does not support encoding algos with confusing names e.g. "funnygzipalgo" that may accidentally match that pattern?
Thanks. Not a new issue with this patch, but just being curious if you or anybody thought about it as a possible issue.
Re: [PATCH v2 1/2] remote-curl: accept all encodings supported by curl
Quoted 4 lines · Junio C Hamano
Similarly, how much control do we have to ensure that the test HTTPD server (1) supports gzip and (2) does not support encoding algos with confusing names e.g. "funnygzipalgo" that may accidentally match that pattern?
I feel it's quite likely indeed that pretty much any Apache instance is going to have the gzip encoding. Every distributor I know supports it.
As for whether there are confusing alternate algorithms, I think it's best to just look at the IANA registration[0] to see what people are using. Potential matches include gzip, x-gzip (a deprecated alias that versions of Apache we can use are not likely to support), and pack200-gzip (a format for Java archives, which we hope the remote side will not be sending).
Overall, I think this is not likely to be a problem, but if necessary in the future, we can add a prerequisite that looks in the module directory for the appropriate module. We haven't seen an issue with it yet, though, TTBOMK.
[0] https://www.iana.org/assignments/http-parameters/http-parameters.xml#content-coding
Signature · 2 lines
brian m. carlson: Houston, Texas, US OpenPGP: https://keybase.io/bk2204
Re: [PATCH v2 1/2] remote-curl: accept all encodings supported by curl
In reply to Junio C HamanoQuoted 6 lines · Junio C Hamano
Quoted 2 lines
-> Accept-Encoding: gzip
+> Accept-Encoding: ENCODINGSIs the ordering of these headers determined by the user of cURL library (i.e. Git), or whatever the version of cURL we happened to link with happens to produce?
The point is whether the order is expected to be stable, or we are better off sorting the actual log before comparing.
The order is not guaranteed by libcurl to be fixed, but it is likely to remain stable since we too have test cases and compare outputs with expected outputs! =)
Going forward, brotli (br) is going to become more commonly present in that header.
Signature · 1 line
/ daniel.haxx.se