{"thread":{"id":"48540","subject":"[PATCH 1/2] remote-curl: accept all encoding supported by curl","startedAt":"2018-05-21T23:40:12Z","lastAt":"2018-05-26T11:08:37Z","messageCount":15,"participants":["Brandon Williams","Jonathan Nieder","Stefan Beller","Daniel Stenberg","Junio C Hamano","brian m. carlson","anton.golubev@gmail.com"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"348262","messageId":"20180521234004.142548-1-bmwill@google.com","threadId":"48540","inReplyTo":null,"subject":"[PATCH 1/2] remote-curl: accept all encoding supported by curl","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2018-05-21T23:40:03Z","receivedAt":"2018-05-21T23:40:12Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"Configure curl to accept all encoding which curl supports instead of\nonly accepting gzip responses.\n\nThis is necessary to fix a bug when using an installation of curl which\ndoesn't support gzip.  Since curl doesn't do any checking to verify that\nit supports the encoding set when calling 'curl_easy_setopt()', curl can\nend up sending an \"Accept-Encoding\" header indicating that it supports\na particular encoding when in fact it doesn't.  Instead when the empty\nstring \"\" is used when setting `CURLOPT_ENCODING`, curl will send an\n\"Accept-Encoding\" header containing only the encoding methods curl\nsupports.\n\nSigned-off-by: Brandon Williams <bmwill@google.com>\n---\n http.c                      | 2 +-\n remote-curl.c               | 2 +-\n t/t5551-http-fetch-smart.sh | 4 ++--\n 3 files changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/http.c b/http.c\nindex fed13b216..709150fc7 100644\n--- a/http.c\n+++ b/http.c\n@@ -1788,7 +1788,7 @@ static int http_request(const char *url,\n \n \tcurl_easy_setopt(slot->curl, CURLOPT_URL, url);\n \tcurl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, headers);\n-\tcurl_easy_setopt(slot->curl, CURLOPT_ENCODING, \"gzip\");\n+\tcurl_easy_setopt(slot->curl, CURLOPT_ENCODING, \"\");\n \n \tret = run_one_slot(slot, &results);\n \ndiff --git a/remote-curl.c b/remote-curl.c\nindex ceb05347b..565bba104 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -684,7 +684,7 @@ static int post_rpc(struct rpc_state *rpc)\n \tcurl_easy_setopt(slot->curl, CURLOPT_NOBODY, 0);\n \tcurl_easy_setopt(slot->curl, CURLOPT_POST, 1);\n \tcurl_easy_setopt(slot->curl, CURLOPT_URL, rpc->service_url);\n-\tcurl_easy_setopt(slot->curl, CURLOPT_ENCODING, \"gzip\");\n+\tcurl_easy_setopt(slot->curl, CURLOPT_ENCODING, \"\");\n \n \tif (large_request) {\n \t\t/* The request body is large and the size cannot be predicted.\ndiff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh\nindex f5721b4a5..39c65482c 100755\n--- a/t/t5551-http-fetch-smart.sh\n+++ b/t/t5551-http-fetch-smart.sh\n@@ -26,14 +26,14 @@ setup_askpass_helper\n cat >exp <<EOF\n > GET /smart/repo.git/info/refs?service=git-upload-pack HTTP/1.1\n > Accept: */*\n-> Accept-Encoding: gzip\n+> Accept-Encoding: deflate, gzip\n > Pragma: no-cache\n < HTTP/1.1 200 OK\n < Pragma: no-cache\n < Cache-Control: no-cache, max-age=0, must-revalidate\n < Content-Type: application/x-git-upload-pack-advertisement\n > POST /smart/repo.git/git-upload-pack HTTP/1.1\n-> Accept-Encoding: gzip\n+> Accept-Encoding: deflate, gzip\n > Content-Type: application/x-git-upload-pack-request\n > Accept: application/x-git-upload-pack-result\n > Content-Length: xxx\n-- \n2.17.0.441.gb46fe60e1d-goog\n\n"},{"id":"348263","messageId":"20180521234004.142548-2-bmwill@google.com","threadId":"48540","inReplyTo":"20180521234004.142548-1-bmwill@google.com","subject":"[PATCH 2/2] remote-curl: accept compressed responses with protocol v2","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2018-05-21T23:40:04Z","receivedAt":"2018-05-21T23:40:15Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"Configure curl to accept compressed responses when using protocol v2 by\nsetting `CURLOPT_ENCODING` to \"\", which indicates that curl should send\nan \"Accept-Encoding\" header with all supported compression encodings.\n\nSigned-off-by: Brandon Williams <bmwill@google.com>\n---\n remote-curl.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/remote-curl.c b/remote-curl.c\nindex 565bba104..99b0bedc6 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -1259,6 +1259,7 @@ static int proxy_request(struct proxy_state *p)\n \n \tslot = get_active_slot();\n \n+\tcurl_easy_setopt(slot->curl, CURLOPT_ENCODING, \"\");\n \tcurl_easy_setopt(slot->curl, CURLOPT_NOBODY, 0);\n \tcurl_easy_setopt(slot->curl, CURLOPT_POST, 1);\n \tcurl_easy_setopt(slot->curl, CURLOPT_URL, p->service_url);\n-- \n2.17.0.441.gb46fe60e1d-goog\n\n"},{"id":"348266","messageId":"20180522000129.GG10623@aiede.svl.corp.google.com","threadId":"48540","inReplyTo":"20180521234004.142548-1-bmwill@google.com","subject":"Re: [PATCH 1/2] remote-curl: accept all encoding supported by curl","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2018-05-22T00:01:29Z","receivedAt":"2018-05-22T00:01:36Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nBrandon Williams wrote:\n\n> Subject: remote-curl: accept all encoding supported by curl\n\nnit: s/encoding/encodings\n\n> Configure curl to accept all encoding which curl supports instead of\n> only accepting gzip responses.\n\nLikewise.\n\n> This is necessary to fix a bug when using an installation of curl which\n> doesn't support gzip.  Since curl doesn't do any checking to verify that\n> it supports the encoding set when calling 'curl_easy_setopt()', curl can\n> end up sending an \"Accept-Encoding\" header indicating that it supports\n> a particular encoding when in fact it doesn't.  Instead when the empty\n> string \"\" is used when setting `CURLOPT_ENCODING`, curl will send an\n> \"Accept-Encoding\" header containing only the encoding methods curl\n> supports.\n>\n> Signed-off-by: Brandon Williams <bmwill@google.com>\n\nThanks for the analysis and fix.\n\nReported-by: Anton Golubev <anton.golubev@gmail.com>\n\nAlso ccing the reporter so we can hopefully get a tested-by.  Anton,\ncan you test this patch and let us know how it goes?  You can apply it\nas follows:\n\n  curl \\\n    https://public-inbox.org/git/20180521234004.142548-1-bmwill@google.com/raw \\\n    >patch.txt\n  git am -3 patch.txt\n\nBrandon, can the commit message also say a little more about the\nmotivating context and symptoms?\n\n  $ curl --version\n  curl 7.52.1 (arm-openwrt-linux-gnu) libcurl/7.52.1 mbedTLS/2.6.0\n  Protocols: file ftp ftps http https\n  Features: IPv6 Largefile SSL\n\nThe issue is that when curl is built without the \"zlib\" feature, since\nv1.8.0-rc0~14^2 (Enable info/refs gzip decompression in HTTP client,\n2012-09-19) we end up requesting \"gzip\" encoding anyway despite\nlibcurl not being able to decode it.  Worse, instead of getting a\nclear error message indicating so, we end up falling back to \"dumb\"\nhttp, producing a confusing and difficult to debug result.\n\n> ---\n>  http.c                      | 2 +-\n>  remote-curl.c               | 2 +-\n>  t/t5551-http-fetch-smart.sh | 4 ++--\n>  3 files changed, 4 insertions(+), 4 deletions(-)\n>\n> diff --git a/http.c b/http.c\n> index fed13b216..709150fc7 100644\n> --- a/http.c\n> +++ b/http.c\n> @@ -1788,7 +1788,7 @@ static int http_request(const char *url,\n>  \n>  \tcurl_easy_setopt(slot->curl, CURLOPT_URL, url);\n>  \tcurl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, headers);\n> -\tcurl_easy_setopt(slot->curl, CURLOPT_ENCODING, \"gzip\");\n> +\tcurl_easy_setopt(slot->curl, CURLOPT_ENCODING, \"\");\n>  \n>  \tret = run_one_slot(slot, &results);\n>  \n> diff --git a/remote-curl.c b/remote-curl.c\n> index ceb05347b..565bba104 100644\n> --- a/remote-curl.c\n> +++ b/remote-curl.c\n> @@ -684,7 +684,7 @@ static int post_rpc(struct rpc_state *rpc)\n>  \tcurl_easy_setopt(slot->curl, CURLOPT_NOBODY, 0);\n>  \tcurl_easy_setopt(slot->curl, CURLOPT_POST, 1);\n>  \tcurl_easy_setopt(slot->curl, CURLOPT_URL, rpc->service_url);\n> -\tcurl_easy_setopt(slot->curl, CURLOPT_ENCODING, \"gzip\");\n> +\tcurl_easy_setopt(slot->curl, CURLOPT_ENCODING, \"\");\n>  \n>  \tif (large_request) {\n>  \t\t/* The request body is large and the size cannot be predicted.\n\nMakes sense.\n\n> diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh\n> index f5721b4a5..39c65482c 100755\n> --- a/t/t5551-http-fetch-smart.sh\n> +++ b/t/t5551-http-fetch-smart.sh\n> @@ -26,14 +26,14 @@ setup_askpass_helper\n>  cat >exp <<EOF\n>  > GET /smart/repo.git/info/refs?service=git-upload-pack HTTP/1.1\n>  > Accept: */*\n> -> Accept-Encoding: gzip\n> +> Accept-Encoding: deflate, gzip\n>  > Pragma: no-cache\n>  < HTTP/1.1 200 OK\n>  < Pragma: no-cache\n>  < Cache-Control: no-cache, max-age=0, must-revalidate\n>  < Content-Type: application/x-git-upload-pack-advertisement\n>  > POST /smart/repo.git/git-upload-pack HTTP/1.1\n> -> Accept-Encoding: gzip\n> +> Accept-Encoding: deflate, gzip\n>  > Content-Type: application/x-git-upload-pack-request\n>  > Accept: application/x-git-upload-pack-result\n>  > Content-Length: xxx\n\nIf libcurl gains support for another encoding in the future, this test\nwould start failing.  Can we make the matching less strict?  For\nexample, how about something like the following for squashing in?\n\nThanks,\nJonathan\n\ndiff --git i/t/t5551-http-fetch-smart.sh w/t/t5551-http-fetch-smart.sh\nindex 39c65482ce..913089b144 100755\n--- i/t/t5551-http-fetch-smart.sh\n+++ w/t/t5551-http-fetch-smart.sh\n@@ -26,14 +26,14 @@ setup_askpass_helper\n cat >exp <<EOF\n > GET /smart/repo.git/info/refs?service=git-upload-pack HTTP/1.1\n > Accept: */*\n-> Accept-Encoding: deflate, gzip\n+> Accept-Encoding: ENCODINGS\n > Pragma: no-cache\n < HTTP/1.1 200 OK\n < Pragma: no-cache\n < Cache-Control: no-cache, max-age=0, must-revalidate\n < Content-Type: application/x-git-upload-pack-advertisement\n > POST /smart/repo.git/git-upload-pack HTTP/1.1\n-> Accept-Encoding: deflate, gzip\n+> Accept-Encoding: ENCODINGS\n > Content-Type: application/x-git-upload-pack-request\n > Accept: application/x-git-upload-pack-result\n > Content-Length: xxx\n@@ -79,8 +79,13 @@ test_expect_success 'clone http repository' '\n \t\t/^< Date: /d\n \t\t/^< Content-Length: /d\n \t\t/^< Transfer-Encoding: /d\n-\t\" >act &&\n-\ttest_cmp exp act\n+\t\" >actual &&\n+\tsed -e \"s/^> Accept-Encoding: .*/> Accept-Encoding: ENCODINGS/\" \\\n+\t\t\tactual >actual.smudged &&\n+\ttest_cmp exp actual.smudged &&\n+\n+\tgrep \"Accept-Encoding:.*gzip\" actual >actual.gzip &&\n+\ttest_line_count = 2 actual.gzip\n '\n \n test_expect_success 'fetch changes via http' '\n"},{"id":"348267","messageId":"CAGZ79kZiyi_1nxvfLttD6HPyV66Wz3pLnuAe=L7FB9ak05dGAQ@mail.gmail.com","threadId":"48540","inReplyTo":"20180521234004.142548-1-bmwill@google.com","subject":"Re: [PATCH 1/2] remote-curl: accept all encoding supported by curl","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-05-22T00:02:13Z","receivedAt":"2018-05-22T00:02:17Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, May 21, 2018 at 4:40 PM, Brandon Williams <bmwill@google.com> wrote:\n> Configure curl to accept all encoding which curl supports instead of\n> only accepting gzip responses.\n\nThis partially reverts aa90b9697f9 (Enable info/refs gzip decompression\nin HTTP client, 2012-09-19), as that specifically called out deflate not being\na good option. Is that worth mentioning in the commit message?\n\n> This is necessary to fix a bug when using an installation of curl which\n> doesn't support gzip.  Since curl doesn't do any checking to verify that\n> it supports the encoding set when calling 'curl_easy_setopt()', curl can\n> end up sending an \"Accept-Encoding\" header indicating that it supports\n> a particular encoding when in fact it doesn't.  Instead when the empty\n> string \"\" is used when setting `CURLOPT_ENCODING`, curl will send an\n> \"Accept-Encoding\" header containing only the encoding methods curl\n> supports.\n>\n> Signed-off-by: Brandon Williams <bmwill@google.com>\n> ---\n>  http.c                      | 2 +-\n>  remote-curl.c               | 2 +-\n>  t/t5551-http-fetch-smart.sh | 4 ++--\n>  3 files changed, 4 insertions(+), 4 deletions(-)\n>\n> diff --git a/http.c b/http.c\n> index fed13b216..709150fc7 100644\n> --- a/http.c\n> +++ b/http.c\n> @@ -1788,7 +1788,7 @@ static int http_request(const char *url,\n>\n>         curl_easy_setopt(slot->curl, CURLOPT_URL, url);\n>         curl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, headers);\n> -       curl_easy_setopt(slot->curl, CURLOPT_ENCODING, \"gzip\");\n> +       curl_easy_setopt(slot->curl, CURLOPT_ENCODING, \"\");\n>\n>         ret = run_one_slot(slot, &results);\n>\n> diff --git a/remote-curl.c b/remote-curl.c\n> index ceb05347b..565bba104 100644\n> --- a/remote-curl.c\n> +++ b/remote-curl.c\n> @@ -684,7 +684,7 @@ static int post_rpc(struct rpc_state *rpc)\n>         curl_easy_setopt(slot->curl, CURLOPT_NOBODY, 0);\n>         curl_easy_setopt(slot->curl, CURLOPT_POST, 1);\n>         curl_easy_setopt(slot->curl, CURLOPT_URL, rpc->service_url);\n> -       curl_easy_setopt(slot->curl, CURLOPT_ENCODING, \"gzip\");\n> +       curl_easy_setopt(slot->curl, CURLOPT_ENCODING, \"\");\n\nLooking at the code here, this succeeds if enough memory is available.\nThere is no check if the given parameter is part of\nCurl_all_content_encodings();\nhttps://github.com/curl/curl/blob/e66cca046cef20d00fba89260dfa6b4a3997233d/lib/setopt.c#L429\nhttps://github.com/curl/curl/blob/c675c40295045d4988eeb6291c54eb48f138822f/lib/content_encoding.c#L686\n\nwhich may be worth checking first?\n\n>\n>         if (large_request) {\n>                 /* The request body is large and the size cannot be predicted.\n> diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh\n> index f5721b4a5..39c65482c 100755\n> --- a/t/t5551-http-fetch-smart.sh\n> +++ b/t/t5551-http-fetch-smart.sh\n> @@ -26,14 +26,14 @@ setup_askpass_helper\n>  cat >exp <<EOF\n>  > GET /smart/repo.git/info/refs?service=git-upload-pack HTTP/1.1\n>  > Accept: */*\n> -> Accept-Encoding: gzip\n> +> Accept-Encoding: deflate, gzip\n>  > Pragma: no-cache\n>  < HTTP/1.1 200 OK\n>  < Pragma: no-cache\n>  < Cache-Control: no-cache, max-age=0, must-revalidate\n>  < Content-Type: application/x-git-upload-pack-advertisement\n>  > POST /smart/repo.git/git-upload-pack HTTP/1.1\n> -> Accept-Encoding: gzip\n> +> Accept-Encoding: deflate, gzip\n>  > Content-Type: application/x-git-upload-pack-request\n>  > Accept: application/x-git-upload-pack-result\n>  > Content-Length: xxx\n> --\n> 2.17.0.441.gb46fe60e1d-goog\n>\n"},{"id":"348275","messageId":"20180522010008.GI10623@aiede.svl.corp.google.com","threadId":"48540","inReplyTo":"CAGZ79kZiyi_1nxvfLttD6HPyV66Wz3pLnuAe=L7FB9ak05dGAQ@mail.gmail.com","subject":"Re: [PATCH 1/2] remote-curl: accept all encoding supported by curl","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2018-05-22T01:00:08Z","receivedAt":"2018-05-22T01:00:14Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nStefan Beller wrote:\n> On Mon, May 21, 2018 at 4:40 PM, Brandon Williams <bmwill@google.com> wrote:\n\n>> Configure curl to accept all encoding which curl supports instead of\n>> only accepting gzip responses.\n>\n> This partially reverts aa90b9697f9 (Enable info/refs gzip decompression\n> in HTTP client, 2012-09-19), as that specifically called out deflate not being\n> a good option. Is that worth mentioning in the commit message?\n\nMore specifically, it mentions the wasted 9 extra bytes from including\n\"deflate, \" on the Accept-Encoding line.  I think the extra bandwidth\nusage will be okay. :)\n\n[...]\n>> -       curl_easy_setopt(slot->curl, CURLOPT_ENCODING, \"gzip\");\n>> +       curl_easy_setopt(slot->curl, CURLOPT_ENCODING, \"\");\n>\n> Looking at the code here, this succeeds if enough memory is available.\n> There is no check if the given parameter is part of\n> Curl_all_content_encodings();\n> https://github.com/curl/curl/blob/e66cca046cef20d00fba89260dfa6b4a3997233d/lib/setopt.c#L429\n> https://github.com/curl/curl/blob/c675c40295045d4988eeb6291c54eb48f138822f/lib/content_encoding.c#L686\n>\n> which may be worth checking first?\n\nBy \"this\" are you referring to the preimage or the postimage?  Are you\nsuggesting a change in git or in libcurl?\n\nCurl_all_content_encodings() is an internal function in libcurl, so\nI'm assuming the latter.\n\nThanks,\nJonathan\n"},{"id":"348284","messageId":"alpine.DEB.2.20.1805220824440.6210@tvnag.unkk.fr","threadId":"48540","inReplyTo":"20180522010008.GI10623@aiede.svl.corp.google.com","subject":"Re: [PATCH 1/2] remote-curl: accept all encoding supported by curl","fromName":"Daniel Stenberg","fromEmail":"daniel@haxx.se","sentAt":"2018-05-22T06:32:43Z","receivedAt":"2018-05-22T06:42:09Z","isPatch":true,"sender":{"key":"daniel@haxx.se","avatar":"https://gravatar.com/avatar/69fdca87edd17cee21ca2e79fc2ff671d644603c3dc27167430f3cd3dbab7ba8?d=mp&s=160"},"body":"On Mon, 21 May 2018, Jonathan Nieder wrote:\n\n>> Looking at the code here, this succeeds if enough memory is available.\n>> There is no check if the given parameter is part of\n>> Curl_all_content_encodings();\n>\n> By \"this\" are you referring to the preimage or the postimage?  Are you \n> suggesting a change in git or in libcurl?\n>\n> Curl_all_content_encodings() is an internal function in libcurl, so I'm \n> assuming the latter.\n\nAck, that certainly isn't the most wonderful API for selecting a compression \nmethod. In reality, almost everyone sticks to passing on a \"\" to that option \nto let libcurl pick and ask for the compression algos it knows since both gzip \nand brotli are present only conditionally depending on build options.\n\nI would agree that the libcurl setopt call should probably be made to fail if \nasked to use a compression method not built-in/supported. Then an application \ncould in fact try different algos in order until one works or ask to disable \ncompression completely.\n\nIn the generic HTTP case, it usually makes sense to ask for more than one \nalgorthim though, since this is asking the server for a compressed version and \ntypically a HTTP client doesn't know which compression methods the server \noffers. Not sure this is actually true to the same extent for git.\n\n-- \n\n  / daniel.haxx.se\n"},{"id":"348319","messageId":"20180522184007.GA177559@google.com","threadId":"48540","inReplyTo":"alpine.DEB.2.20.1805220824440.6210@tvnag.unkk.fr","subject":"Re: [PATCH 1/2] remote-curl: accept all encoding supported by curl","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2018-05-22T18:40:07Z","receivedAt":"2018-05-22T18:40:14Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 05/22, Daniel Stenberg wrote:\n> On Mon, 21 May 2018, Jonathan Nieder wrote:\n> \n> > > Looking at the code here, this succeeds if enough memory is available.\n> > > There is no check if the given parameter is part of\n> > > Curl_all_content_encodings();\n> > \n> > By \"this\" are you referring to the preimage or the postimage?  Are you\n> > suggesting a change in git or in libcurl?\n> > \n> > Curl_all_content_encodings() is an internal function in libcurl, so I'm\n> > assuming the latter.\n> \n> Ack, that certainly isn't the most wonderful API for selecting a compression\n> method. In reality, almost everyone sticks to passing on a \"\" to that option\n> to let libcurl pick and ask for the compression algos it knows since both\n> gzip and brotli are present only conditionally depending on build options.\n\nThanks for the clarification.  Sounds like the best option is to\ncontinue with this patch and let curl decide using \"\".\n\n> \n> I would agree that the libcurl setopt call should probably be made to fail\n> if asked to use a compression method not built-in/supported. Then an\n> application could in fact try different algos in order until one works or\n> ask to disable compression completely.\n> \n> In the generic HTTP case, it usually makes sense to ask for more than one\n> algorthim though, since this is asking the server for a compressed version\n> and typically a HTTP client doesn't know which compression methods the\n> server offers. Not sure this is actually true to the same extent for git.\n> \n> -- \n> \n>  / daniel.haxx.se\n\n-- \nBrandon Williams\n"},{"id":"348320","messageId":"20180522184204.47332-1-bmwill@google.com","threadId":"48540","inReplyTo":"20180521234004.142548-1-bmwill@google.com","subject":"[PATCH v2 1/2] remote-curl: accept all encodings supported by curl","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2018-05-22T18:42:03Z","receivedAt":"2018-05-22T18:42:13Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"Configure curl to accept all encodings which curl supports instead of\nonly accepting gzip responses.\n\nThis fixes an issue when using an installation of curl which is built\nwithout the \"zlib\" feature. Since aa90b9697 (Enable info/refs gzip\ndecompression in HTTP client, 2012-09-19) we end up requesting \"gzip\"\nencoding anyway despite libcurl not being able to decode it.  Worse,\ninstead of getting a clear error message indicating so, we end up\nfalling back to \"dumb\" http, producing a confusing and difficult to\ndebug result.\n\nSince curl doesn't do any checking to verify that it supports the a\nrequested encoding, instead set the curl option `CURLOPT_ENCODING` with\nan empty string indicating that curl should send an \"Accept-Encoding\"\nheader containing only the encodings supported by curl.\n\nReported-by: Anton Golubev <anton.golubev@gmail.com>\nSigned-off-by: Brandon Williams <bmwill@google.com>\n---\n\nVersion 2 of this series just tweaks the commit message and the test per\nJonathan's suggestion.\n\n http.c                      |  2 +-\n remote-curl.c               |  2 +-\n t/t5551-http-fetch-smart.sh | 13 +++++++++----\n 3 files changed, 11 insertions(+), 6 deletions(-)\n\ndiff --git a/http.c b/http.c\nindex fed13b216..709150fc7 100644\n--- a/http.c\n+++ b/http.c\n@@ -1788,7 +1788,7 @@ static int http_request(const char *url,\n \n \tcurl_easy_setopt(slot->curl, CURLOPT_URL, url);\n \tcurl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, headers);\n-\tcurl_easy_setopt(slot->curl, CURLOPT_ENCODING, \"gzip\");\n+\tcurl_easy_setopt(slot->curl, CURLOPT_ENCODING, \"\");\n \n \tret = run_one_slot(slot, &results);\n \ndiff --git a/remote-curl.c b/remote-curl.c\nindex ceb05347b..565bba104 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -684,7 +684,7 @@ static int post_rpc(struct rpc_state *rpc)\n \tcurl_easy_setopt(slot->curl, CURLOPT_NOBODY, 0);\n \tcurl_easy_setopt(slot->curl, CURLOPT_POST, 1);\n \tcurl_easy_setopt(slot->curl, CURLOPT_URL, rpc->service_url);\n-\tcurl_easy_setopt(slot->curl, CURLOPT_ENCODING, \"gzip\");\n+\tcurl_easy_setopt(slot->curl, CURLOPT_ENCODING, \"\");\n \n \tif (large_request) {\n \t\t/* The request body is large and the size cannot be predicted.\ndiff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh\nindex f5721b4a5..913089b14 100755\n--- a/t/t5551-http-fetch-smart.sh\n+++ b/t/t5551-http-fetch-smart.sh\n@@ -26,14 +26,14 @@ setup_askpass_helper\n cat >exp <<EOF\n > GET /smart/repo.git/info/refs?service=git-upload-pack HTTP/1.1\n > Accept: */*\n-> Accept-Encoding: gzip\n+> Accept-Encoding: ENCODINGS\n > Pragma: no-cache\n < HTTP/1.1 200 OK\n < Pragma: no-cache\n < Cache-Control: no-cache, max-age=0, must-revalidate\n < Content-Type: application/x-git-upload-pack-advertisement\n > POST /smart/repo.git/git-upload-pack HTTP/1.1\n-> Accept-Encoding: gzip\n+> Accept-Encoding: ENCODINGS\n > Content-Type: application/x-git-upload-pack-request\n > Accept: application/x-git-upload-pack-result\n > Content-Length: xxx\n@@ -79,8 +79,13 @@ test_expect_success 'clone http repository' '\n \t\t/^< Date: /d\n \t\t/^< Content-Length: /d\n \t\t/^< Transfer-Encoding: /d\n-\t\" >act &&\n-\ttest_cmp exp act\n+\t\" >actual &&\n+\tsed -e \"s/^> Accept-Encoding: .*/> Accept-Encoding: ENCODINGS/\" \\\n+\t\t\tactual >actual.smudged &&\n+\ttest_cmp exp actual.smudged &&\n+\n+\tgrep \"Accept-Encoding:.*gzip\" actual >actual.gzip &&\n+\ttest_line_count = 2 actual.gzip\n '\n \n test_expect_success 'fetch changes via http' '\n-- \n2.17.0.441.gb46fe60e1d-goog\n\n"},{"id":"348321","messageId":"20180522184204.47332-2-bmwill@google.com","threadId":"48540","inReplyTo":"20180522184204.47332-1-bmwill@google.com","subject":"[PATCH v2 2/2] remote-curl: accept compressed responses with protocol v2","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2018-05-22T18:42:04Z","receivedAt":"2018-05-22T18:42:15Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"Configure curl to accept compressed responses when using protocol v2 by\nsetting `CURLOPT_ENCODING` to \"\", which indicates that curl should send\nan \"Accept-Encoding\" header with all supported compression encodings.\n\nSigned-off-by: Brandon Williams <bmwill@google.com>\n---\n remote-curl.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/remote-curl.c b/remote-curl.c\nindex 565bba104..99b0bedc6 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -1259,6 +1259,7 @@ static int proxy_request(struct proxy_state *p)\n \n \tslot = get_active_slot();\n \n+\tcurl_easy_setopt(slot->curl, CURLOPT_ENCODING, \"\");\n \tcurl_easy_setopt(slot->curl, CURLOPT_NOBODY, 0);\n \tcurl_easy_setopt(slot->curl, CURLOPT_POST, 1);\n \tcurl_easy_setopt(slot->curl, CURLOPT_URL, p->service_url);\n-- \n2.17.0.441.gb46fe60e1d-goog\n\n"},{"id":"348323","messageId":"20180522184821.GL10623@aiede.svl.corp.google.com","threadId":"48540","inReplyTo":"20180522184204.47332-1-bmwill@google.com","subject":"Re: [PATCH v2 1/2] remote-curl: accept all encodings supported by curl","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2018-05-22T18:48:21Z","receivedAt":"2018-05-22T18:48:27Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Brandon Williams wrote:\n\n> Configure curl to accept all encodings which curl supports instead of\n> only accepting gzip responses.\n>\n> This fixes an issue when using an installation of curl which is built\n> without the \"zlib\" feature. Since aa90b9697 (Enable info/refs gzip\n> decompression in HTTP client, 2012-09-19) we end up requesting \"gzip\"\n> encoding anyway despite libcurl not being able to decode it.  Worse,\n> instead of getting a clear error message indicating so, we end up\n> falling back to \"dumb\" http, producing a confusing and difficult to\n> debug result.\n>\n> Since curl doesn't do any checking to verify that it supports the a\n> requested encoding, instead set the curl option `CURLOPT_ENCODING` with\n> an empty string indicating that curl should send an \"Accept-Encoding\"\n> header containing only the encodings supported by curl.\n\nEven better, this means we get the benefit of future of even better\ncompression algorithms once libcurl learns them.\n\n> Reported-by: Anton Golubev <anton.golubev@gmail.com>\n> Signed-off-by: Brandon Williams <bmwill@google.com>\n> ---\n> Version 2 of this series just tweaks the commit message and the test per\n> Jonathan's suggestion.\n>\n>  http.c                      |  2 +-\n>  remote-curl.c               |  2 +-\n>  t/t5551-http-fetch-smart.sh | 13 +++++++++----\n>  3 files changed, 11 insertions(+), 6 deletions(-)\n\nReviewed-by: Jonathan Nieder <jrnieder@gmail.com>\n\nThanks for fixing it.\n\nPatch left unsnipped for reference.\n\n> --- a/http.c\n> +++ b/http.c\n> @@ -1788,7 +1788,7 @@ static int http_request(const char *url,\n>  \n>  \tcurl_easy_setopt(slot->curl, CURLOPT_URL, url);\n>  \tcurl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, headers);\n> -\tcurl_easy_setopt(slot->curl, CURLOPT_ENCODING, \"gzip\");\n> +\tcurl_easy_setopt(slot->curl, CURLOPT_ENCODING, \"\");\n>  \n>  \tret = run_one_slot(slot, &results);\n>  \n> diff --git a/remote-curl.c b/remote-curl.c\n> index ceb05347b..565bba104 100644\n> --- a/remote-curl.c\n> +++ b/remote-curl.c\n> @@ -684,7 +684,7 @@ static int post_rpc(struct rpc_state *rpc)\n>  \tcurl_easy_setopt(slot->curl, CURLOPT_NOBODY, 0);\n>  \tcurl_easy_setopt(slot->curl, CURLOPT_POST, 1);\n>  \tcurl_easy_setopt(slot->curl, CURLOPT_URL, rpc->service_url);\n> -\tcurl_easy_setopt(slot->curl, CURLOPT_ENCODING, \"gzip\");\n> +\tcurl_easy_setopt(slot->curl, CURLOPT_ENCODING, \"\");\n>  \n>  \tif (large_request) {\n>  \t\t/* The request body is large and the size cannot be predicted.\n> diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh\n> index f5721b4a5..913089b14 100755\n> --- a/t/t5551-http-fetch-smart.sh\n> +++ b/t/t5551-http-fetch-smart.sh\n> @@ -26,14 +26,14 @@ setup_askpass_helper\n>  cat >exp <<EOF\n>  > GET /smart/repo.git/info/refs?service=git-upload-pack HTTP/1.1\n>  > Accept: */*\n> -> Accept-Encoding: gzip\n> +> Accept-Encoding: ENCODINGS\n>  > Pragma: no-cache\n>  < HTTP/1.1 200 OK\n>  < Pragma: no-cache\n>  < Cache-Control: no-cache, max-age=0, must-revalidate\n>  < Content-Type: application/x-git-upload-pack-advertisement\n>  > POST /smart/repo.git/git-upload-pack HTTP/1.1\n> -> Accept-Encoding: gzip\n> +> Accept-Encoding: ENCODINGS\n>  > Content-Type: application/x-git-upload-pack-request\n>  > Accept: application/x-git-upload-pack-result\n>  > Content-Length: xxx\n> @@ -79,8 +79,13 @@ test_expect_success 'clone http repository' '\n>  \t\t/^< Date: /d\n>  \t\t/^< Content-Length: /d\n>  \t\t/^< Transfer-Encoding: /d\n> -\t\" >act &&\n> -\ttest_cmp exp act\n> +\t\" >actual &&\n> +\tsed -e \"s/^> Accept-Encoding: .*/> Accept-Encoding: ENCODINGS/\" \\\n> +\t\t\tactual >actual.smudged &&\n> +\ttest_cmp exp actual.smudged &&\n> +\n> +\tgrep \"Accept-Encoding:.*gzip\" actual >actual.gzip &&\n> +\ttest_line_count = 2 actual.gzip\n>  '\n>  \n>  test_expect_success 'fetch changes via http' '\n"},{"id":"348324","messageId":"20180522185524.GM10623@aiede.svl.corp.google.com","threadId":"48540","inReplyTo":"20180522184204.47332-2-bmwill@google.com","subject":"Re: [PATCH v2 2/2] remote-curl: accept compressed responses with protocol v2","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2018-05-22T18:55:24Z","receivedAt":"2018-05-22T18:55:30Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Brandon Williams wrote:\n\n> Configure curl to accept compressed responses when using protocol v2 by\n> setting `CURLOPT_ENCODING` to \"\", which indicates that curl should send\n> an \"Accept-Encoding\" header with all supported compression encodings.\n>\n> Signed-off-by: Brandon Williams <bmwill@google.com>\n> ---\n>  remote-curl.c | 1 +\n>  1 file changed, 1 insertion(+)\n\nYay!\n\n> diff --git a/remote-curl.c b/remote-curl.c\n> index 565bba104..99b0bedc6 100644\n> --- a/remote-curl.c\n> +++ b/remote-curl.c\n> @@ -1259,6 +1259,7 @@ static int proxy_request(struct proxy_state *p)\n>  \n>  \tslot = get_active_slot();\n>  \n> +\tcurl_easy_setopt(slot->curl, CURLOPT_ENCODING, \"\");\n>  \tcurl_easy_setopt(slot->curl, CURLOPT_NOBODY, 0);\n>  \tcurl_easy_setopt(slot->curl, CURLOPT_POST, 1);\n>  \tcurl_easy_setopt(slot->curl, CURLOPT_URL, p->service_url);\n\nCan this get a test?\n\nI'm particularly interested in it since it's easy to accidentally\napply this patch to the wrong duplicated place (luckily 'p' is a\ndifferent variable name than 'rpc' but it's an easy mistake to make if\napplying the patch manually).\n\nWith or without such a test,\nReviewed-by: Jonathan Nieder <jrnieder@gmail.com>\n"},{"id":"348341","messageId":"xmqqwovvw4fl.fsf@gitster-ct.c.googlers.com","threadId":"48540","inReplyTo":"20180522184204.47332-1-bmwill@google.com","subject":"Re: [PATCH v2 1/2] remote-curl: accept all encodings supported by curl","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-05-23T01:23:26Z","receivedAt":"2018-05-23T01:23:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brandon Williams <bmwill@google.com> writes:\n\n> diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh\n> index f5721b4a5..913089b14 100755\n> --- a/t/t5551-http-fetch-smart.sh\n> +++ b/t/t5551-http-fetch-smart.sh\n> @@ -26,14 +26,14 @@ setup_askpass_helper\n>  cat >exp <<EOF\n>  > GET /smart/repo.git/info/refs?service=git-upload-pack HTTP/1.1\n>  > Accept: */*\n> -> Accept-Encoding: gzip\n> +> Accept-Encoding: ENCODINGS\n>  > Pragma: no-cache\n\nIs the ordering of these headers determined by the user of cURL\nlibrary (i.e. Git), or whatever the version of cURL we happened to\nlink with happens to produce?\n\nThe point is whether the order is expected to be stable, or we are\nbetter off sorting the actual log before comparing.\n\n>  < HTTP/1.1 200 OK\n>  < Pragma: no-cache\n>  < Cache-Control: no-cache, max-age=0, must-revalidate\n>  < Content-Type: application/x-git-upload-pack-advertisement\n\nA similar question for this response.\n\n>  > POST /smart/repo.git/git-upload-pack HTTP/1.1\n> -> Accept-Encoding: gzip\n> +> Accept-Encoding: ENCODINGS\n>  > Content-Type: application/x-git-upload-pack-request\n>  > Accept: application/x-git-upload-pack-result\n>  > Content-Length: xxx\n\nDitto for this request.\n\n> @@ -79,8 +79,13 @@ test_expect_success 'clone http repository' '\n>  \t\t/^< Date: /d\n>  \t\t/^< Content-Length: /d\n>  \t\t/^< Transfer-Encoding: /d\n> -\t\" >act &&\n> -\ttest_cmp exp act\n> +\t\" >actual &&\n> +\tsed -e \"s/^> Accept-Encoding: .*/> Accept-Encoding: ENCODINGS/\" \\\n> +\t\t\tactual >actual.smudged &&\n> +\ttest_cmp exp actual.smudged &&\n> +\n> +\tgrep \"Accept-Encoding:.*gzip\" actual >actual.gzip &&\n> +\ttest_line_count = 2 actual.gzip\n>  '\n\nSimilarly, how much control do we have to ensure that the test HTTPD\nserver (1) supports gzip and (2) does not support encoding algos\nwith confusing names e.g. \"funnygzipalgo\" that may accidentally\nmatch that pattern?\n\nThanks.  Not a new issue with this patch, but just being curious if\nyou or anybody thought about it as a possible issue.\n\n\n"},{"id":"348345","messageId":"20180523021727.GJ652292@genre.crustytoothpaste.net","threadId":"48540","inReplyTo":"xmqqwovvw4fl.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2 1/2] remote-curl: accept all encodings supported by curl","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2018-05-23T02:17:27Z","receivedAt":"2018-05-23T02:17:38Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Wed, May 23, 2018 at 10:23:26AM +0900, Junio C Hamano wrote:\n> Similarly, how much control do we have to ensure that the test HTTPD\n> server (1) supports gzip and (2) does not support encoding algos\n> with confusing names e.g. \"funnygzipalgo\" that may accidentally\n> match that pattern?\n\nI feel it's quite likely indeed that pretty much any Apache instance is\ngoing to have the gzip encoding.  Every distributor I know supports it.\n\nAs for whether there are confusing alternate algorithms, I think it's\nbest to just look at the IANA registration[0] to see what people are\nusing.  Potential matches include gzip, x-gzip (a deprecated alias that\nversions of Apache we can use are not likely to support), and\npack200-gzip (a format for Java archives, which we hope the remote side\nwill not be sending).\n\nOverall, I think this is not likely to be a problem, but if necessary in\nthe future, we can add a prerequisite that looks in the module directory\nfor the appropriate module.  We haven't seen an issue with it yet,\nthough, TTBOMK.\n\n[0] https://www.iana.org/assignments/http-parameters/http-parameters.xml#content-coding\n-- \nbrian m. carlson: Houston, Texas, US\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"348355","messageId":"alpine.DEB.2.20.1805230751370.6210@tvnag.unkk.fr","threadId":"48540","inReplyTo":"xmqqwovvw4fl.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2 1/2] remote-curl: accept all encodings supported by curl","fromName":"Daniel Stenberg","fromEmail":"daniel@haxx.se","sentAt":"2018-05-23T05:55:19Z","receivedAt":"2018-05-23T05:55:28Z","isPatch":true,"sender":{"key":"daniel@haxx.se","avatar":"https://gravatar.com/avatar/69fdca87edd17cee21ca2e79fc2ff671d644603c3dc27167430f3cd3dbab7ba8?d=mp&s=160"},"body":"On Wed, 23 May 2018, Junio C Hamano wrote:\n\n>> -> Accept-Encoding: gzip\n>> +> Accept-Encoding: ENCODINGS\n>\n> Is the ordering of these headers determined by the user of cURL library \n> (i.e. Git), or whatever the version of cURL we happened to link with happens \n> to produce?\n>\n> The point is whether the order is expected to be stable, or we are better \n> off sorting the actual log before comparing.\n\nThe order is not guaranteed by libcurl to be fixed, but it is likely to remain \nstable since we too have test cases and compare outputs with expected outputs! \n=)\n\nGoing forward, brotli (br) is going to become more commonly present in that \nheader.\n\n-- \n\n  / daniel.haxx.se\n"},{"id":"348582","messageId":"015501d3f4e1$e0b47c60$a21d7520$@gmail.com","threadId":"48540","inReplyTo":"20180522000129.GG10623@aiede.svl.corp.google.com","subject":"RE: [PATCH 1/2] remote-curl: accept all encoding supported by curl","fromName":"","fromEmail":"anton.golubev@gmail.com","sentAt":"2018-05-26T11:08:29Z","receivedAt":"2018-05-26T11:08:37Z","isPatch":true,"sender":{"key":"anton.golubev@gmail.com","avatar":null},"body":"Hi Jonathan,\n\nI'd like to confirm, that your patch fixes my problem: I can sync with\ngoogle repository now using git and curl version without gzip support. \nDo you know how this patch is going to be released? Just HEAD now and GA in\nthe next planned release?\n\nCommunication looks like follow now:\n\nroot@bsb:~# GIT_TRACE_PACKET=1 GIT_CURL_VERBOSE=1 git ls-remote\nhttps://source.developers.google.co m/p/wired-balm-187912/r/dotfiles 2>&1 |\nsed -e 's/\\(git-[^=]*\\)=.*/\\1=REDACTED/' -e 's/Authorizatio n:\n.*/Authorization: REDACTED/'\n> GET /p/wired-balm-187912/r/dotfiles/info/refs?service=git-upload-pack\nHTTP/1.1\nHost: source.developers.google.com\nUser-Agent: git/2.17.0\nAccept: */*\nAccept-Encoding: identity\nCookie: o=git-anton.golubev.gmail.com=REDACTED\nPragma: no-cache\n\n< HTTP/1.1 200 OK\n< Cache-Control: no-cache, max-age=0, must-revalidate\n< Content-Length: 374\n< Content-Type: application/x-git-upload-pack-advertisement\n< Expires: Fri, 01 Jan 1980 00:00:00 GMT\n< Pragma: no-cache\n< X-Content-Type-Options: nosniff\n< X-Frame-Options: SAMEORIGIN\n< X-Xss-Protection: 1; mode=block\n< Date: Sat, 26 May 2018 11:04:41 GMT\n< Alt-Svc: hq=\":443\"; ma=2592000; quic=51303433; quic=51303432;\nquic=51303431; quic=51303339; quic=51303335,quic=\":443\"; ma=2592000;\nv=\"43,42,41,39,35\"\n<\n13:04:41.274561 pkt-line.c:80           packet:          git< #\nservice=git-upload-pack\n13:04:41.274634 pkt-line.c:80           packet:          git< 0000\n13:04:41.274693 pkt-line.c:80           packet:          git<\n45e2c99dd1790529cc4b7e029b1e9dfcc817d18e HEAD\\0 include-tag\nmulti_ack_detailed multi_ack ofs-delta side-band side-band-64k thin-pack\nno-progress shallow no-done allow-tip-sha1-in-want\nallow-reachable-sha1-in-want agent=JGit/4-google filter\nsymref=HEAD:refs/heads/master\n13:04:41.274739 pkt-line.c:80           packet:          git<\n45e2c99dd1790529cc4b7e029b1e9dfcc817d18e refs/heads/master\n13:04:41.274777 pkt-line.c:80           packet:          git< 0000\n45e2c99dd1790529cc4b7e029b1e9dfcc817d18e        HEAD\n45e2c99dd1790529cc4b7e029b1e9dfcc817d18e        refs/heads/master\n\nKind regards,\nAnton Golubev\n\n\n\n-----Original Message-----\nFrom: Jonathan Nieder [mailto:jrnieder@gmail.com] \nSent: Dienstag, 22. Mai 2018 02:01\nTo: Brandon Williams <bmwill@google.com>\nCc: git@vger.kernel.org; Anton Golubev <anton.golubev@gmail.com>\nSubject: Re: [PATCH 1/2] remote-curl: accept all encoding supported by curl\n\nHi,\n\nBrandon Williams wrote:\n\n> Subject: remote-curl: accept all encoding supported by curl\n\nnit: s/encoding/encodings\n\n> Configure curl to accept all encoding which curl supports instead of \n> only accepting gzip responses.\n\nLikewise.\n\n> This is necessary to fix a bug when using an installation of curl \n> which doesn't support gzip.  Since curl doesn't do any checking to \n> verify that it supports the encoding set when calling \n> 'curl_easy_setopt()', curl can end up sending an \"Accept-Encoding\" \n> header indicating that it supports a particular encoding when in fact \n> it doesn't.  Instead when the empty string \"\" is used when setting \n> `CURLOPT_ENCODING`, curl will send an \"Accept-Encoding\" header \n> containing only the encoding methods curl supports.\n>\n> Signed-off-by: Brandon Williams <bmwill@google.com>\n\nThanks for the analysis and fix.\n\nReported-by: Anton Golubev <anton.golubev@gmail.com>\n\nAlso ccing the reporter so we can hopefully get a tested-by.  Anton, can you\ntest this patch and let us know how it goes?  You can apply it as follows:\n\n  curl \\\n \nhttps://public-inbox.org/git/20180521234004.142548-1-bmwill@google.com/raw \\\n    >patch.txt\n  git am -3 patch.txt\n\nBrandon, can the commit message also say a little more about the motivating\ncontext and symptoms?\n\n  $ curl --version\n  curl 7.52.1 (arm-openwrt-linux-gnu) libcurl/7.52.1 mbedTLS/2.6.0\n  Protocols: file ftp ftps http https\n  Features: IPv6 Largefile SSL\n\nThe issue is that when curl is built without the \"zlib\" feature, since\nv1.8.0-rc0~14^2 (Enable info/refs gzip decompression in HTTP client,\n2012-09-19) we end up requesting \"gzip\" encoding anyway despite libcurl not\nbeing able to decode it.  Worse, instead of getting a clear error message\nindicating so, we end up falling back to \"dumb\"\nhttp, producing a confusing and difficult to debug result.\n\n> ---\n>  http.c                      | 2 +-\n>  remote-curl.c               | 2 +-\n>  t/t5551-http-fetch-smart.sh | 4 ++--\n>  3 files changed, 4 insertions(+), 4 deletions(-)\n>\n> diff --git a/http.c b/http.c\n> index fed13b216..709150fc7 100644\n> --- a/http.c\n> +++ b/http.c\n> @@ -1788,7 +1788,7 @@ static int http_request(const char *url,\n>  \n>  \tcurl_easy_setopt(slot->curl, CURLOPT_URL, url);\n>  \tcurl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, headers);\n> -\tcurl_easy_setopt(slot->curl, CURLOPT_ENCODING, \"gzip\");\n> +\tcurl_easy_setopt(slot->curl, CURLOPT_ENCODING, \"\");\n>  \n>  \tret = run_one_slot(slot, &results);\n>  \n> diff --git a/remote-curl.c b/remote-curl.c index ceb05347b..565bba104 \n> 100644\n> --- a/remote-curl.c\n> +++ b/remote-curl.c\n> @@ -684,7 +684,7 @@ static int post_rpc(struct rpc_state *rpc)\n>  \tcurl_easy_setopt(slot->curl, CURLOPT_NOBODY, 0);\n>  \tcurl_easy_setopt(slot->curl, CURLOPT_POST, 1);\n>  \tcurl_easy_setopt(slot->curl, CURLOPT_URL, rpc->service_url);\n> -\tcurl_easy_setopt(slot->curl, CURLOPT_ENCODING, \"gzip\");\n> +\tcurl_easy_setopt(slot->curl, CURLOPT_ENCODING, \"\");\n>  \n>  \tif (large_request) {\n>  \t\t/* The request body is large and the size cannot be\npredicted.\n\nMakes sense.\n\n> diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh \n> index f5721b4a5..39c65482c 100755\n> --- a/t/t5551-http-fetch-smart.sh\n> +++ b/t/t5551-http-fetch-smart.sh\n> @@ -26,14 +26,14 @@ setup_askpass_helper  cat >exp <<EOF  > GET \n> /smart/repo.git/info/refs?service=git-upload-pack HTTP/1.1  > Accept: \n> */*\n> -> Accept-Encoding: gzip\n> +> Accept-Encoding: deflate, gzip\n>  > Pragma: no-cache\n>  < HTTP/1.1 200 OK\n>  < Pragma: no-cache\n>  < Cache-Control: no-cache, max-age=0, must-revalidate  < \n> Content-Type: application/x-git-upload-pack-advertisement\n>  > POST /smart/repo.git/git-upload-pack HTTP/1.1\n> -> Accept-Encoding: gzip\n> +> Accept-Encoding: deflate, gzip\n>  > Content-Type: application/x-git-upload-pack-request\n>  > Accept: application/x-git-upload-pack-result\n>  > Content-Length: xxx\n\nIf libcurl gains support for another encoding in the future, this test would\nstart failing.  Can we make the matching less strict?  For example, how\nabout something like the following for squashing in?\n\nThanks,\nJonathan\n\ndiff --git i/t/t5551-http-fetch-smart.sh w/t/t5551-http-fetch-smart.sh index\n39c65482ce..913089b144 100755\n--- i/t/t5551-http-fetch-smart.sh\n+++ w/t/t5551-http-fetch-smart.sh\n@@ -26,14 +26,14 @@ setup_askpass_helper  cat >exp <<EOF  > GET\n/smart/repo.git/info/refs?service=git-upload-pack HTTP/1.1  > Accept: */*\n-> Accept-Encoding: deflate, gzip\n+> Accept-Encoding: ENCODINGS\n > Pragma: no-cache\n < HTTP/1.1 200 OK\n < Pragma: no-cache\n < Cache-Control: no-cache, max-age=0, must-revalidate  < Content-Type:\napplication/x-git-upload-pack-advertisement\n > POST /smart/repo.git/git-upload-pack HTTP/1.1\n-> Accept-Encoding: deflate, gzip\n+> Accept-Encoding: ENCODINGS\n > Content-Type: application/x-git-upload-pack-request\n > Accept: application/x-git-upload-pack-result\n > Content-Length: xxx\n@@ -79,8 +79,13 @@ test_expect_success 'clone http repository' '\n \t\t/^< Date: /d\n \t\t/^< Content-Length: /d\n \t\t/^< Transfer-Encoding: /d\n-\t\" >act &&\n-\ttest_cmp exp act\n+\t\" >actual &&\n+\tsed -e \"s/^> Accept-Encoding: .*/> Accept-Encoding: ENCODINGS/\" \\\n+\t\t\tactual >actual.smudged &&\n+\ttest_cmp exp actual.smudged &&\n+\n+\tgrep \"Accept-Encoding:.*gzip\" actual >actual.gzip &&\n+\ttest_line_count = 2 actual.gzip\n '\n \n test_expect_success 'fetch changes via http' '\n\n"}]}