{"thread":{"id":"50122","subject":"[PATCH 1/2] Change how HTTP response body is returned","startedAt":"2018-12-28T01:47:45Z","lastAt":"2019-01-10T23:06:46Z","messageCount":29,"participants":["Masaya Suzuki","Eric Sunshine","Junio C Hamano","Jeff King","SZEDER Gábor"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"365895","messageId":"20181228014720.206443-1-masayasuzuki@google.com","threadId":"50122","inReplyTo":null,"subject":"[PATCH 1/2] Change how HTTP response body is returned","fromName":"Masaya Suzuki","fromEmail":"masayasuzuki@google.com","sentAt":"2018-12-28T01:47:19Z","receivedAt":"2018-12-28T01:47:45Z","isPatch":true,"sender":{"key":"masayasuzuki@google.com","avatar":"https://avatars.githubusercontent.com/u/21878?v=4"},"body":"This changes the way HTTP response body is returned in\nhttp_request_reauth and post_rpc.\n\n1. http_request_reauth makes up to two requests; one without a\n credential and one with a credential. The first request can fail if\n it needs a credential. When the keep_error option is specified, the\n response to the first request can be written to the HTTP response\n body destination. If the response body destination is a string\n buffer, it erases the buffer before making the second request. By\n introducing http_response_dest, it can handle the case that the\n destination is a file handle.\n2. post_rpc makes an HTTP request and the response body is directly\n written to a file descriptor. This makes it check the HTTP status\n code before writing it, and do not write the response body if it's an\n error response. It's ok without this check now because post_rpc makes\n a request with CURLOPT_FAILONERROR, and libcurl won't call the\n callback if the response has an error status code.\n\nSigned-off-by: Masaya Suzuki \n---\n http.c | 99 +++++++++++++++++++++++++++++----------------------\n remote-curl.c | 29 ++++++++++++---\n 2 files changed, 81 insertions(+), 47 deletions(-)\n\ndiff --git a/http.c b/http.c\nindex eacc2a75e..d23417670 100644\n--- a/http.c\n+++ b/http.c\n@@ -165,6 +165,19 @@ static int http_schannel_check_revoke = 1;\n */\n static int http_schannel_use_ssl_cainfo;\n \n+/*\n+ * Where to store the result of http_request.\n+ *\n+ * At most one of buffer or file can be non-NULL. The buffer and file are not\n+ * allocated by http_request, and the caller is responsible for releasing them.\n+ */\n+struct http_response_dest {\n+\tstruct strbuf *buffer;\n+\n+\tFILE *file;\n+\tconst char *filename;\n+};\n+\n size_t fread_buffer(char *ptr, size_t eltsize, size_t nmemb, void *buffer_)\n {\n \tsize_t size = eltsize * nmemb;\n@@ -1794,12 +1807,8 @@ static void http_opt_request_remainder(CURL *curl, off_t pos)\n \tcurl_easy_setopt(curl, CURLOPT_RANGE, buf);\n }\n \n-/* http_request() targets */\n-#define HTTP_REQUEST_STRBUF\t0\n-#define HTTP_REQUEST_FILE\t1\n-\n static int http_request(const char *url,\n-\t\t\tvoid *result, int target,\n+\t\t\tstruct http_response_dest *dest,\n \t\t\tconst struct http_get_options *options)\n {\n \tstruct active_request_slot *slot;\n@@ -1812,21 +1821,23 @@ static int http_request(const char *url,\n \tslot = get_active_slot();\n \tcurl_easy_setopt(slot->curl, CURLOPT_HTTPGET, 1);\n \n-\tif (result == NULL) {\n-\t\tcurl_easy_setopt(slot->curl, CURLOPT_NOBODY, 1);\n-\t} else {\n+\tif (dest->file) {\n+\t\toff_t posn = ftello(dest->file);\n \t\tcurl_easy_setopt(slot->curl, CURLOPT_NOBODY, 0);\n-\t\tcurl_easy_setopt(slot->curl, CURLOPT_FILE, result);\n-\n-\t\tif (target == HTTP_REQUEST_FILE) {\n-\t\t\toff_t posn = ftello(result);\n-\t\t\tcurl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION,\n-\t\t\t\t\t fwrite);\n-\t\t\tif (posn > 0)\n-\t\t\t\thttp_opt_request_remainder(slot->curl, posn);\n-\t\t} else\n-\t\t\tcurl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION,\n-\t\t\t\t\t fwrite_buffer);\n+\t\tcurl_easy_setopt(slot->curl, CURLOPT_FILE,\n+\t\t\t\t dest->file);\n+\t\tcurl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION,\n+\t\t\t\t fwrite);\n+\t\tif (posn > 0)\n+\t\t\thttp_opt_request_remainder(slot->curl, posn);\n+\t} else if (dest->buffer) {\n+\t\tcurl_easy_setopt(slot->curl, CURLOPT_NOBODY, 0);\n+\t\tcurl_easy_setopt(slot->curl, CURLOPT_FILE,\n+\t\t\t\t dest->buffer);\n+\t\tcurl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION,\n+\t\t\t\t fwrite_buffer);\n+\t} else {\n+\t\tcurl_easy_setopt(slot->curl, CURLOPT_NOBODY, 1);\n \t}\n \n \taccept_language = get_accept_language();\n@@ -1930,10 +1941,10 @@ static int update_url_from_redirect(struct strbuf *base,\n }\n \n static int http_request_reauth(const char *url,\n-\t\t\t void *result, int target,\n+\t\t\t struct http_response_dest *dest,\n \t\t\t struct http_get_options *options)\n {\n-\tint ret = http_request(url, result, target, options);\n+\tint ret = http_request(url, dest, options);\n \n \tif (ret != HTTP_OK && ret != HTTP_REAUTH)\n \t\treturn ret;\n@@ -1949,32 +1960,34 @@ static int http_request_reauth(const char *url,\n \tif (ret != HTTP_REAUTH)\n \t\treturn ret;\n \n-\t/*\n-\t * If we are using KEEP_ERROR, the previous request may have\n-\t * put cruft into our output stream; we should clear it out before\n-\t * making our next request. We only know how to do this for\n-\t * the strbuf case, but that is enough to satisfy current callers.\n-\t */\n-\tif (options && options->keep_error) {\n-\t\tswitch (target) {\n-\t\tcase HTTP_REQUEST_STRBUF:\n-\t\t\tstrbuf_reset(result);\n-\t\t\tbreak;\n-\t\tdefault:\n-\t\t\tBUG(\"HTTP_KEEP_ERROR is only supported with strbufs\");\n+\tif (dest->file) {\n+\t\t/*\n+\t\t * At this point, the file contains the response body of the\n+\t\t * previous request. We need to truncate the file.\n+\t\t */\n+\t\tFILE *new_file = freopen(dest->filename, \"w\", dest->file);\n+\t\tif (new_file == NULL) {\n+\t\t\terror(\"Unable to open local file %s\", dest->filename);\n+\t\t\treturn HTTP_ERROR;\n \t\t}\n+\t\tdest->file = new_file;\n+\t} else if (dest->buffer) {\n+\t\tstrbuf_reset(dest->buffer);\n \t}\n \n \tcredential_fill(&http_auth);\n \n-\treturn http_request(url, result, target, options);\n+\treturn http_request(url, dest, options);\n }\n \n int http_get_strbuf(const char *url,\n-\t\t struct strbuf *result,\n+\t\t struct strbuf *dest_buffer,\n \t\t struct http_get_options *options)\n {\n-\treturn http_request_reauth(url, result, HTTP_REQUEST_STRBUF, options);\n+\tstruct http_response_dest dest;\n+\tdest.file = NULL;\n+\tdest.buffer = dest_buffer;\n+\treturn http_request_reauth(url, &dest, options);\n }\n \n /*\n@@ -1988,18 +2001,20 @@ static int http_get_file(const char *url, const char *filename,\n {\n \tint ret;\n \tstruct strbuf tmpfile = STRBUF_INIT;\n-\tFILE *result;\n+\tstruct http_response_dest dest;\n \n \tstrbuf_addf(&tmpfile, \"%s.temp\", filename);\n-\tresult = fopen(tmpfile.buf, \"a\");\n-\tif (!result) {\n+\tdest.buffer = NULL;\n+\tdest.file = fopen(tmpfile.buf, \"a\");\n+\tif (!dest.file) {\n \t\terror(\"Unable to open local file %s\", tmpfile.buf);\n \t\tret = HTTP_ERROR;\n \t\tgoto cleanup;\n \t}\n+\tdest.filename = tmpfile.buf;\n \n-\tret = http_request_reauth(url, result, HTTP_REQUEST_FILE, options);\n-\tfclose(result);\n+\tret = http_request_reauth(url, &dest, options);\n+\tfclose(dest.file);\n \n \tif (ret == HTTP_OK && finalize_object_file(tmpfile.buf, filename))\n \t\tret = HTTP_ERROR;\ndiff --git a/remote-curl.c b/remote-curl.c\nindex 1220dffcd..48656bf18 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -546,14 +546,31 @@ static curlioerr rpc_ioctl(CURL *handle, int cmd, void *clientp)\n }\n #endif\n \n+struct rpc_in_data {\n+\tstruct rpc_state *rpc;\n+\tstruct active_request_slot *slot;\n+};\n+\n+/*\n+ * A callback for CURLOPT_WRITEFUNCTION. The return value is the bytes consumed\n+ * from ptr.\n+ */\n static size_t rpc_in(char *ptr, size_t eltsize,\n \t\tsize_t nmemb, void *buffer_)\n {\n \tsize_t size = eltsize * nmemb;\n-\tstruct rpc_state *rpc = buffer_;\n+\tstruct rpc_in_data *data = buffer_;\n+\tlong response_code;\n+\n+\tif (curl_easy_getinfo(data->slot->curl, CURLINFO_RESPONSE_CODE,\n+\t\t\t &response_code) != CURLE_OK)\n+\t\treturn size;\n+\tif (response_code != 200)\n+\t\treturn size;\n+\n \tif (size)\n-\t\trpc->any_written = 1;\n-\twrite_or_die(rpc->in, ptr, size);\n+\t\tdata->rpc->any_written = 1;\n+\twrite_or_die(data->rpc->in, ptr, size);\n \treturn size;\n }\n \n@@ -633,6 +650,7 @@ static int post_rpc(struct rpc_state *rpc)\n \tsize_t gzip_size = 0;\n \tint err, large_request = 0;\n \tint needs_100_continue = 0;\n+\tstruct rpc_in_data rpc_in_data;\n \n \t/* Try to load the entire request, if we can fit it into the\n \t * allocated buffer space we can use HTTP/1.0 and avoid the\n@@ -765,8 +783,9 @@ static int post_rpc(struct rpc_state *rpc)\n \n \tcurl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, headers);\n \tcurl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, rpc_in);\n-\tcurl_easy_setopt(slot->curl, CURLOPT_FILE, rpc);\n-\n+\trpc_in_data.rpc = rpc;\n+\trpc_in_data.slot = slot;\n+\tcurl_easy_setopt(slot->curl, CURLOPT_FILE, &rpc_in_data);\n \n \trpc->any_written = 0;\n \terr = run_slot(slot, NULL);\n-- \n2.20.1.415.g653613c723-goog\n\n"},{"id":"365896","messageId":"20181228014720.206443-2-masayasuzuki@google.com","threadId":"50122","inReplyTo":"20181228014720.206443-1-masayasuzuki@google.com","subject":"[PATCH 2/2] Unset CURLOPT_FAILONERROR","fromName":"Masaya Suzuki","fromEmail":"masayasuzuki@google.com","sentAt":"2018-12-28T01:47:20Z","receivedAt":"2018-12-28T01:47:53Z","isPatch":true,"sender":{"key":"masayasuzuki@google.com","avatar":"https://avatars.githubusercontent.com/u/21878?v=4"},"body":"When GIT_CURL_VERBOSE is set, libcurl produces request/response headers\nto stderr. However, if the response is an error response and\nCURLOPT_FAILONERROR is set, libcurl stops parsing the response, and it\nwon't ump the headers. Showing HTTP response headers is useful for\ndebugging, especially for non-OK responses.\n\nThis is substantially same as setting http_options.keep_error to all\nrequests. Hence, removing this option.\n\nSigned-off-by: Masaya Suzuki \n---\n http.c | 4 ----\n http.h | 1 -\n remote-curl.c | 1 -\n t/lib-httpd/apache.conf | 1 +\n t/t5581-http-curl-verbose.sh | 32 ++++++++++++++++++++++++++++++++\n 5 files changed, 33 insertions(+), 6 deletions(-)\n create mode 100755 t/t5581-http-curl-verbose.sh\n\ndiff --git a/http.c b/http.c\nindex d23417670..8f8101da3 100644\n--- a/http.c\n+++ b/http.c\n@@ -1269,7 +1269,6 @@ struct active_request_slot *get_active_slot(void)\n \tcurl_easy_setopt(slot->curl, CURLOPT_POSTFIELDS, NULL);\n \tcurl_easy_setopt(slot->curl, CURLOPT_UPLOAD, 0);\n \tcurl_easy_setopt(slot->curl, CURLOPT_HTTPGET, 1);\n-\tcurl_easy_setopt(slot->curl, CURLOPT_FAILONERROR, 1);\n \tcurl_easy_setopt(slot->curl, CURLOPT_RANGE, NULL);\n \n \t/*\n@@ -1848,8 +1847,6 @@ static int http_request(const char *url,\n \tstrbuf_addstr(&buf, \"Pragma:\");\n \tif (options && options->no_cache)\n \t\tstrbuf_addstr(&buf, \" no-cache\");\n-\tif (options && options->keep_error)\n-\t\tcurl_easy_setopt(slot->curl, CURLOPT_FAILONERROR, 0);\n \tif (options && options->initial_request &&\n \t http_follow_config == HTTP_FOLLOW_INITIAL)\n \t\tcurl_easy_setopt(slot->curl, CURLOPT_FOLLOWLOCATION, 1);\n@@ -2415,7 +2412,6 @@ struct http_object_request *new_http_object_request(const char *base_url,\n \tfreq->slot = get_active_slot();\n \n \tcurl_easy_setopt(freq->slot->curl, CURLOPT_FILE, freq);\n-\tcurl_easy_setopt(freq->slot->curl, CURLOPT_FAILONERROR, 0);\n \tcurl_easy_setopt(freq->slot->curl, CURLOPT_WRITEFUNCTION, fwrite_sha1_file);\n \tcurl_easy_setopt(freq->slot->curl, CURLOPT_ERRORBUFFER, freq->errorstr);\n \tcurl_easy_setopt(freq->slot->curl, CURLOPT_URL, freq->url);\ndiff --git a/http.h b/http.h\nindex d305ca1dc..eebf40688 100644\n--- a/http.h\n+++ b/http.h\n@@ -146,7 +146,6 @@ extern char *get_remote_object_url(const char *url, const char *hex,\n /* Options for http_get_*() */\n struct http_get_options {\n \tunsigned no_cache:1,\n-\t\t keep_error:1,\n \t\t initial_request:1;\n \n \t/* If non-NULL, returns the content-type of the response. */\ndiff --git a/remote-curl.c b/remote-curl.c\nindex 48656bf18..43e7a1d80 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -380,7 +380,6 @@ static struct discovery *discover_refs(const char *service, int for_push)\n \thttp_options.extra_headers = &extra_headers;\n \thttp_options.initial_request = 1;\n \thttp_options.no_cache = 1;\n-\thttp_options.keep_error = 1;\n \n \thttp_ret = http_get_strbuf(refs_url.buf, &buffer, &http_options);\n \tswitch (http_ret) {\ndiff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf\nindex 581c010d8..cc4b87507 100644\n--- a/t/lib-httpd/apache.conf\n+++ b/t/lib-httpd/apache.conf\n@@ -115,6 +115,7 @@ Alias /auth/dumb/ www/auth/dumb/\n \tSetEnv GIT_EXEC_PATH ${GIT_EXEC_PATH}\n \tSetEnv GIT_HTTP_EXPORT_ALL\n \n+ScriptAliasMatch /error_git_upload_pack/(.*)/git-upload-pack error.sh/\n ScriptAliasMatch /smart_*[^/]*/(.*) ${GIT_EXEC_PATH}/git-http-backend/$1\n ScriptAlias /broken_smart/ broken-smart-http.sh/\n ScriptAlias /error/ error.sh/\ndiff --git a/t/t5581-http-curl-verbose.sh b/t/t5581-http-curl-verbose.sh\nnew file mode 100755\nindex 000000000..c89e06e12\n--- /dev/null\n+++ b/t/t5581-http-curl-verbose.sh\n@@ -0,0 +1,32 @@\n+#!/bin/sh\n+\n+test_description='test GIT_CURL_VERBOSE'\n+. ./test-lib.sh\n+. \"$TEST_DIRECTORY\"/lib-httpd.sh\n+start_httpd\n+\n+test_expect_success 'setup repository' '\n+\tgit config push.default matching &&\n+\techo content >file &&\n+\tgit add file &&\n+\tgit commit -m one\n+'\n+\n+test_expect_success 'create http-accessible bare repository' '\n+\tmkdir \"$HTTPD_DOCUMENT_ROOT_PATH/repo.git\" &&\n+\t(cd \"$HTTPD_DOCUMENT_ROOT_PATH/repo.git\" &&\n+\t git --bare init\n+\t) &&\n+\tgit remote add public \"$HTTPD_DOCUMENT_ROOT_PATH/repo.git\" &&\n+\tgit push public master:master\n+'\n+\n+test_expect_success 'failure in git-upload-pack is shown' '\n+\t(GIT_CURL_VERBOSE=1 git clone --bare \"$HTTPD_URL/error_git_upload_pack/smart/repo.git\" 2>curl_log ||\n+\t true) &&\n+\tcat curl_log | grep \"< HTTP/1.1 500 Intentional Breakage\"\n+'\n+\n+stop_httpd\n+\n+test_done\n-- \n2.20.1.415.g653613c723-goog\n\n"},{"id":"365909","messageId":"CAPig+cQc6oF3b03XS2jmNdtUDOX_08tv=AMUpdd7D1eDto33=A@mail.gmail.com","threadId":"50122","inReplyTo":"20181228014720.206443-2-masayasuzuki@google.com","subject":"Re: [PATCH 2/2] Unset CURLOPT_FAILONERROR","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-12-28T19:36:52Z","receivedAt":"2018-12-28T19:37:06Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Dec 27, 2018 at 8:47 PM Masaya Suzuki wrote:\n> When GIT_CURL_VERBOSE is set, libcurl produces request/response headers\n> to stderr. However, if the response is an error response and\n> CURLOPT_FAILONERROR is set, libcurl stops parsing the response, and it\n> won't ump the headers. Showing HTTP response headers is useful for\n\ns/ump/dump/\n\n> debugging, especially for non-OK responses.\n>\n> This is substantially same as setting http_options.keep_error to all\n> requests. Hence, removing this option.\n\ns/removing/remove/\n\n> Signed-off-by: Masaya Suzuki \n> ---\n> diff --git a/t/t5581-http-curl-verbose.sh b/t/t5581-http-curl-verbose.sh\n> @@ -0,0 +1,32 @@\n> +test_expect_success 'setup repository' '\n> + ...\n> +'\n> +\n> +test_expect_success 'create http-accessible bare repository' '\n\nNot a big deal, but this seems like more setup, so it could be folded\ninto the \"setup\" test above it.\n\n> + mkdir \"$HTTPD_DOCUMENT_ROOT_PATH/repo.git\" &&\n> + (cd \"$HTTPD_DOCUMENT_ROOT_PATH/repo.git\" &&\n> + git --bare init\n> + ) &&\n\nSince this is a new test script, it makes sense to format the subshell\nin the modern style:\n\n (\n cd ... &&\n git ...\n ) &&\n\nAlternately, use -C and drop the subshell altogether:\n\n git -C $BLAH/repo.git --bare init &&\n\n> + git remote add public \"$HTTPD_DOCUMENT_ROOT_PATH/repo.git\" &&\n> + git push public master:master\n> +'\n> +\n> +test_expect_success 'failure in git-upload-pack is shown' '\n> + (GIT_CURL_VERBOSE=1 git clone --bare \"$HTTPD_URL/error_git_upload_pack/smart/repo.git\" 2>curl_log ||\n> + true) &&\n\nUsing test_might_fail() would allow you to drop the subshell and the \"|| true\":\n\n test_might_fail env GIT_CURL_VERBOSE=1 git clone ... &&\n\n> + cat curl_log | grep \"< HTTP/1.1 500 Intentional Breakage\"\n> +'\n"},{"id":"365912","messageId":"CAJB1erXq3JZMd9XwUZFHp80Hr2kRakrp3JJgrAugXPyjp1rxNg@mail.gmail.com","threadId":"50122","inReplyTo":"CAPig+cQc6oF3b03XS2jmNdtUDOX_08tv=AMUpdd7D1eDto33=A@mail.gmail.com","subject":"Re: [PATCH 2/2] Unset CURLOPT_FAILONERROR","fromName":"Masaya Suzuki","fromEmail":"masayasuzuki@google.com","sentAt":"2018-12-28T19:51:12Z","receivedAt":"2018-12-28T19:51:27Z","isPatch":true,"sender":{"key":"masayasuzuki@google.com","avatar":"https://avatars.githubusercontent.com/u/21878?v=4"},"body":"On Fri, Dec 28, 2018 at 11:37 AM Eric Sunshine wrote:\n>\n> On Thu, Dec 27, 2018 at 8:47 PM Masaya Suzuki wrote:\n> > When GIT_CURL_VERBOSE is set, libcurl produces request/response headers\n> > to stderr. However, if the response is an error response and\n> > CURLOPT_FAILONERROR is set, libcurl stops parsing the response, and it\n> > won't ump the headers. Showing HTTP response headers is useful for\n>\n> s/ump/dump/\n>\n> > debugging, especially for non-OK responses.\n> >\n> > This is substantially same as setting http_options.keep_error to all\n> > requests. Hence, removing this option.\n>\n> s/removing/remove/\n>\n> > Signed-off-by: Masaya Suzuki \n> > ---\n> > diff --git a/t/t5581-http-curl-verbose.sh b/t/t5581-http-curl-verbose.sh\n> > @@ -0,0 +1,32 @@\n> > +test_expect_success 'setup repository' '\n> > + ...\n> > +'\n> > +\n> > +test_expect_success 'create http-accessible bare repository' '\n>\n> Not a big deal, but this seems like more setup, so it could be folded\n> into the \"setup\" test above it.\n>\n> > + mkdir \"$HTTPD_DOCUMENT_ROOT_PATH/repo.git\" &&\n> > + (cd \"$HTTPD_DOCUMENT_ROOT_PATH/repo.git\" &&\n> > + git --bare init\n> > + ) &&\n>\n> Since this is a new test script, it makes sense to format the subshell\n> in the modern style:\n>\n> (\n> cd ... &&\n> git ...\n> ) &&\n>\n> Alternately, use -C and drop the subshell altogether:\n>\n> git -C $BLAH/repo.git --bare init &&\n>\n> > + git remote add public \"$HTTPD_DOCUMENT_ROOT_PATH/repo.git\" &&\n> > + git push public master:master\n> > +'\n> > +\n> > +test_expect_success 'failure in git-upload-pack is shown' '\n> > + (GIT_CURL_VERBOSE=1 git clone --bare \"$HTTPD_URL/error_git_upload_pack/smart/repo.git\" 2>curl_log ||\n> > + true) &&\n>\n> Using test_might_fail() would allow you to drop the subshell and the \"|| true\":\n>\n> test_might_fail env GIT_CURL_VERBOSE=1 git clone ... &&\n>\n> > + cat curl_log | grep \"< HTTP/1.1 500 Intentional Breakage\"\n> > +'\n\nThe test should success. This is a test that a log is produced after a\ngit command fails. The point of this test is \"cat curl_log | grep ...\"\npart that asserts the log.\n"},{"id":"365913","messageId":"CAPig+cTOK9XkGiPcHdVrWbV-L0a+Av0Kv=NcRVYsUHnb0j5-Zw@mail.gmail.com","threadId":"50122","inReplyTo":"CAJB1erXq3JZMd9XwUZFHp80Hr2kRakrp3JJgrAugXPyjp1rxNg@mail.gmail.com","subject":"Re: [PATCH 2/2] Unset CURLOPT_FAILONERROR","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-12-28T19:58:46Z","receivedAt":"2018-12-28T19:58:59Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Dec 28, 2018 at 2:51 PM Masaya Suzuki wrote:\n> On Fri, Dec 28, 2018 at 11:37 AM Eric Sunshine wrote:\n> > On Thu, Dec 27, 2018 at 8:47 PM Masaya Suzuki wrote:\n> > > +test_expect_success 'failure in git-upload-pack is shown' '\n> > > + (GIT_CURL_VERBOSE=1 git clone --bare \"$HTTPD_URL/error_git_upload_pack/smart/repo.git\" 2>curl_log ||\n> > > + true) &&\n> >\n> > Using test_might_fail() would allow you to drop the subshell and the \"|| true\":\n> >\n> > test_might_fail env GIT_CURL_VERBOSE=1 git clone ... &&\n> >\n> > > + cat curl_log | grep \"< HTTP/1.1 500 Intentional Breakage\"\n> > > +'\n>\n> The test should success. This is a test that a log is produced after a\n> git command fails. The point of this test is \"cat curl_log | grep ...\"\n> part that asserts the log.\n\nUnfortunately, the name \"test_might_fail\" is confusing. It is not\nsaying that the entire test might or might not fail. Rather, it is\nsaying that the one command might or might not fail (and that you\ndon't care if it does fail). The idiom:\n\n (some-git-command || true) &&\n\ncan be replaced with:\n\n test_might_fail some-git-command &&\n\nwithout changing its meaning, and without affecting the\nsuccess/failure status of the test overall.\n\nSo, this new test could be written like this:\n\n--- 8< ---\ntest_expect_success 'failure in git-upload-pack is shown' '\n test_might_fail env GIT_CURL_VERBOSE=1 git clone --bare\n\"$HTTPD_URL/error_git_upload_pack/smart/repo.git\" 2>curl_log &&\n cat curl_log | grep \"< HTTP/1.1 500 Intentional Breakage\"\n'\n--- 8< ---\n\nand have the same meaning.\n"},{"id":"365915","messageId":"CAJB1erUMPKUKz-x9+xdyUayO9FUmZVQ1MbewtrQQJxgsLZyqbA@mail.gmail.com","threadId":"50122","inReplyTo":"CAPig+cTOK9XkGiPcHdVrWbV-L0a+Av0Kv=NcRVYsUHnb0j5-Zw@mail.gmail.com","subject":"Re: [PATCH 2/2] Unset CURLOPT_FAILONERROR","fromName":"Masaya Suzuki","fromEmail":"masayasuzuki@google.com","sentAt":"2018-12-28T20:00:20Z","receivedAt":"2018-12-28T20:00:35Z","isPatch":true,"sender":{"key":"masayasuzuki@google.com","avatar":"https://avatars.githubusercontent.com/u/21878?v=4"},"body":"On Fri, Dec 28, 2018 at 11:58 AM Eric Sunshine wrote:\n>\n> On Fri, Dec 28, 2018 at 2:51 PM Masaya Suzuki wrote:\n> > On Fri, Dec 28, 2018 at 11:37 AM Eric Sunshine wrote:\n> > > On Thu, Dec 27, 2018 at 8:47 PM Masaya Suzuki wrote:\n> > > > +test_expect_success 'failure in git-upload-pack is shown' '\n> > > > + (GIT_CURL_VERBOSE=1 git clone --bare \"$HTTPD_URL/error_git_upload_pack/smart/repo.git\" 2>curl_log ||\n> > > > + true) &&\n> > >\n> > > Using test_might_fail() would allow you to drop the subshell and the \"|| true\":\n> > >\n> > > test_might_fail env GIT_CURL_VERBOSE=1 git clone ... &&\n> > >\n> > > > + cat curl_log | grep \"< HTTP/1.1 500 Intentional Breakage\"\n> > > > +'\n> >\n> > The test should success. This is a test that a log is produced after a\n> > git command fails. The point of this test is \"cat curl_log | grep ...\"\n> > part that asserts the log.\n>\n> Unfortunately, the name \"test_might_fail\" is confusing. It is not\n> saying that the entire test might or might not fail. Rather, it is\n> saying that the one command might or might not fail (and that you\n> don't care if it does fail). The idiom:\n>\n> (some-git-command || true) &&\n>\n> can be replaced with:\n>\n> test_might_fail some-git-command &&\n>\n> without changing its meaning, and without affecting the\n> success/failure status of the test overall.\n>\n> So, this new test could be written like this:\n>\n> --- 8< ---\n> test_expect_success 'failure in git-upload-pack is shown' '\n> test_might_fail env GIT_CURL_VERBOSE=1 git clone --bare\n> \"$HTTPD_URL/error_git_upload_pack/smart/repo.git\" 2>curl_log &&\n> cat curl_log | grep \"< HTTP/1.1 500 Intentional Breakage\"\n> '\n> --- 8< ---\n>\n> and have the same meaning.\n\nAh. I see. It's used inside the test. Thanks.\n"},{"id":"365968","messageId":"20181229194447.157763-1-masayasuzuki@google.com","threadId":"50122","inReplyTo":"20181228014720.206443-1-masayasuzuki@google.com","subject":"[PATCH v2 0/2] Show HTTP headers of failed requests with GIT_CURL_VERBOSE","fromName":"Masaya Suzuki","fromEmail":"masayasuzuki@google.com","sentAt":"2018-12-29T19:44:45Z","receivedAt":"2018-12-29T19:44:53Z","isPatch":true,"sender":{"key":"masayasuzuki@google.com","avatar":"https://avatars.githubusercontent.com/u/21878?v=4"},"body":"When GIT_CURL_VERBOSE is set, libcurl produces request/response headers\nto stderr. However, if the response is an error response and\nCURLOPT_FAILONERROR is set, libcurl stops parsing the response, and it\nwon't dump the headers. Showing HTTP response headers is useful for\ndebugging, especially for non-OK responses.\n\nTo this end, the caller of libcurl needs to handle HTTP request failures by\nthemselves. The first patch makes git prepared to handle those failures. The\nsecond patch actually unsets CURLOPT_FAILONERROR.\n\nMasaya Suzuki (2):\n Change how HTTP response body is returned\n Unset CURLOPT_FAILONERROR\n\n http.c | 103 +++++++++++++++++++----------------\n http.h | 1 -\n remote-curl.c | 30 ++++++++--\n t/lib-httpd/apache.conf | 1 +\n t/t5581-http-curl-verbose.sh | 28 ++++++++++\n 5 files changed, 110 insertions(+), 53 deletions(-)\n create mode 100755 t/t5581-http-curl-verbose.sh\n\n-- \n2.20.1.415.g653613c723-goog\n\n"},{"id":"365969","messageId":"20181229194447.157763-2-masayasuzuki@google.com","threadId":"50122","inReplyTo":"20181229194447.157763-1-masayasuzuki@google.com","subject":"[PATCH v2 1/2] Change how HTTP response body is returned","fromName":"Masaya Suzuki","fromEmail":"masayasuzuki@google.com","sentAt":"2018-12-29T19:44:46Z","receivedAt":"2018-12-29T19:44:57Z","isPatch":true,"sender":{"key":"masayasuzuki@google.com","avatar":"https://avatars.githubusercontent.com/u/21878?v=4"},"body":"This changes the way HTTP response body is returned in\nhttp_request_reauth and post_rpc.\n\n1. http_request_reauth makes up to two requests; one without a\n credential and one with a credential. The first request can fail if\n it needs a credential. When the keep_error option is specified, the\n response to the first request can be written to the HTTP response\n body destination. If the response body destination is a string\n buffer, it erases the buffer before making the second request. By\n introducing http_response_dest, it can handle the case that the\n destination is a file handle.\n2. post_rpc makes an HTTP request and the response body is directly\n written to a file descriptor. This makes it check the HTTP status\n code before writing it, and do not write the response body if it's an\n error response. It's ok without this check now because post_rpc makes\n a request with CURLOPT_FAILONERROR, and libcurl won't call the\n callback if the response has an error status code.\n\nSigned-off-by: Masaya Suzuki \n---\n http.c | 99 +++++++++++++++++++++++++++++----------------------\n remote-curl.c | 29 ++++++++++++---\n 2 files changed, 81 insertions(+), 47 deletions(-)\n\ndiff --git a/http.c b/http.c\nindex eacc2a75e..d23417670 100644\n--- a/http.c\n+++ b/http.c\n@@ -165,6 +165,19 @@ static int http_schannel_check_revoke = 1;\n */\n static int http_schannel_use_ssl_cainfo;\n \n+/*\n+ * Where to store the result of http_request.\n+ *\n+ * At most one of buffer or file can be non-NULL. The buffer and file are not\n+ * allocated by http_request, and the caller is responsible for releasing them.\n+ */\n+struct http_response_dest {\n+\tstruct strbuf *buffer;\n+\n+\tFILE *file;\n+\tconst char *filename;\n+};\n+\n size_t fread_buffer(char *ptr, size_t eltsize, size_t nmemb, void *buffer_)\n {\n \tsize_t size = eltsize * nmemb;\n@@ -1794,12 +1807,8 @@ static void http_opt_request_remainder(CURL *curl, off_t pos)\n \tcurl_easy_setopt(curl, CURLOPT_RANGE, buf);\n }\n \n-/* http_request() targets */\n-#define HTTP_REQUEST_STRBUF\t0\n-#define HTTP_REQUEST_FILE\t1\n-\n static int http_request(const char *url,\n-\t\t\tvoid *result, int target,\n+\t\t\tstruct http_response_dest *dest,\n \t\t\tconst struct http_get_options *options)\n {\n \tstruct active_request_slot *slot;\n@@ -1812,21 +1821,23 @@ static int http_request(const char *url,\n \tslot = get_active_slot();\n \tcurl_easy_setopt(slot->curl, CURLOPT_HTTPGET, 1);\n \n-\tif (result == NULL) {\n-\t\tcurl_easy_setopt(slot->curl, CURLOPT_NOBODY, 1);\n-\t} else {\n+\tif (dest->file) {\n+\t\toff_t posn = ftello(dest->file);\n \t\tcurl_easy_setopt(slot->curl, CURLOPT_NOBODY, 0);\n-\t\tcurl_easy_setopt(slot->curl, CURLOPT_FILE, result);\n-\n-\t\tif (target == HTTP_REQUEST_FILE) {\n-\t\t\toff_t posn = ftello(result);\n-\t\t\tcurl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION,\n-\t\t\t\t\t fwrite);\n-\t\t\tif (posn > 0)\n-\t\t\t\thttp_opt_request_remainder(slot->curl, posn);\n-\t\t} else\n-\t\t\tcurl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION,\n-\t\t\t\t\t fwrite_buffer);\n+\t\tcurl_easy_setopt(slot->curl, CURLOPT_FILE,\n+\t\t\t\t dest->file);\n+\t\tcurl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION,\n+\t\t\t\t fwrite);\n+\t\tif (posn > 0)\n+\t\t\thttp_opt_request_remainder(slot->curl, posn);\n+\t} else if (dest->buffer) {\n+\t\tcurl_easy_setopt(slot->curl, CURLOPT_NOBODY, 0);\n+\t\tcurl_easy_setopt(slot->curl, CURLOPT_FILE,\n+\t\t\t\t dest->buffer);\n+\t\tcurl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION,\n+\t\t\t\t fwrite_buffer);\n+\t} else {\n+\t\tcurl_easy_setopt(slot->curl, CURLOPT_NOBODY, 1);\n \t}\n \n \taccept_language = get_accept_language();\n@@ -1930,10 +1941,10 @@ static int update_url_from_redirect(struct strbuf *base,\n }\n \n static int http_request_reauth(const char *url,\n-\t\t\t void *result, int target,\n+\t\t\t struct http_response_dest *dest,\n \t\t\t struct http_get_options *options)\n {\n-\tint ret = http_request(url, result, target, options);\n+\tint ret = http_request(url, dest, options);\n \n \tif (ret != HTTP_OK && ret != HTTP_REAUTH)\n \t\treturn ret;\n@@ -1949,32 +1960,34 @@ static int http_request_reauth(const char *url,\n \tif (ret != HTTP_REAUTH)\n \t\treturn ret;\n \n-\t/*\n-\t * If we are using KEEP_ERROR, the previous request may have\n-\t * put cruft into our output stream; we should clear it out before\n-\t * making our next request. We only know how to do this for\n-\t * the strbuf case, but that is enough to satisfy current callers.\n-\t */\n-\tif (options && options->keep_error) {\n-\t\tswitch (target) {\n-\t\tcase HTTP_REQUEST_STRBUF:\n-\t\t\tstrbuf_reset(result);\n-\t\t\tbreak;\n-\t\tdefault:\n-\t\t\tBUG(\"HTTP_KEEP_ERROR is only supported with strbufs\");\n+\tif (dest->file) {\n+\t\t/*\n+\t\t * At this point, the file contains the response body of the\n+\t\t * previous request. We need to truncate the file.\n+\t\t */\n+\t\tFILE *new_file = freopen(dest->filename, \"w\", dest->file);\n+\t\tif (new_file == NULL) {\n+\t\t\terror(\"Unable to open local file %s\", dest->filename);\n+\t\t\treturn HTTP_ERROR;\n \t\t}\n+\t\tdest->file = new_file;\n+\t} else if (dest->buffer) {\n+\t\tstrbuf_reset(dest->buffer);\n \t}\n \n \tcredential_fill(&http_auth);\n \n-\treturn http_request(url, result, target, options);\n+\treturn http_request(url, dest, options);\n }\n \n int http_get_strbuf(const char *url,\n-\t\t struct strbuf *result,\n+\t\t struct strbuf *dest_buffer,\n \t\t struct http_get_options *options)\n {\n-\treturn http_request_reauth(url, result, HTTP_REQUEST_STRBUF, options);\n+\tstruct http_response_dest dest;\n+\tdest.file = NULL;\n+\tdest.buffer = dest_buffer;\n+\treturn http_request_reauth(url, &dest, options);\n }\n \n /*\n@@ -1988,18 +2001,20 @@ static int http_get_file(const char *url, const char *filename,\n {\n \tint ret;\n \tstruct strbuf tmpfile = STRBUF_INIT;\n-\tFILE *result;\n+\tstruct http_response_dest dest;\n \n \tstrbuf_addf(&tmpfile, \"%s.temp\", filename);\n-\tresult = fopen(tmpfile.buf, \"a\");\n-\tif (!result) {\n+\tdest.buffer = NULL;\n+\tdest.file = fopen(tmpfile.buf, \"a\");\n+\tif (!dest.file) {\n \t\terror(\"Unable to open local file %s\", tmpfile.buf);\n \t\tret = HTTP_ERROR;\n \t\tgoto cleanup;\n \t}\n+\tdest.filename = tmpfile.buf;\n \n-\tret = http_request_reauth(url, result, HTTP_REQUEST_FILE, options);\n-\tfclose(result);\n+\tret = http_request_reauth(url, &dest, options);\n+\tfclose(dest.file);\n \n \tif (ret == HTTP_OK && finalize_object_file(tmpfile.buf, filename))\n \t\tret = HTTP_ERROR;\ndiff --git a/remote-curl.c b/remote-curl.c\nindex 1220dffcd..48656bf18 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -546,14 +546,31 @@ static curlioerr rpc_ioctl(CURL *handle, int cmd, void *clientp)\n }\n #endif\n \n+struct rpc_in_data {\n+\tstruct rpc_state *rpc;\n+\tstruct active_request_slot *slot;\n+};\n+\n+/*\n+ * A callback for CURLOPT_WRITEFUNCTION. The return value is the bytes consumed\n+ * from ptr.\n+ */\n static size_t rpc_in(char *ptr, size_t eltsize,\n \t\tsize_t nmemb, void *buffer_)\n {\n \tsize_t size = eltsize * nmemb;\n-\tstruct rpc_state *rpc = buffer_;\n+\tstruct rpc_in_data *data = buffer_;\n+\tlong response_code;\n+\n+\tif (curl_easy_getinfo(data->slot->curl, CURLINFO_RESPONSE_CODE,\n+\t\t\t &response_code) != CURLE_OK)\n+\t\treturn size;\n+\tif (response_code != 200)\n+\t\treturn size;\n+\n \tif (size)\n-\t\trpc->any_written = 1;\n-\twrite_or_die(rpc->in, ptr, size);\n+\t\tdata->rpc->any_written = 1;\n+\twrite_or_die(data->rpc->in, ptr, size);\n \treturn size;\n }\n \n@@ -633,6 +650,7 @@ static int post_rpc(struct rpc_state *rpc)\n \tsize_t gzip_size = 0;\n \tint err, large_request = 0;\n \tint needs_100_continue = 0;\n+\tstruct rpc_in_data rpc_in_data;\n \n \t/* Try to load the entire request, if we can fit it into the\n \t * allocated buffer space we can use HTTP/1.0 and avoid the\n@@ -765,8 +783,9 @@ static int post_rpc(struct rpc_state *rpc)\n \n \tcurl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, headers);\n \tcurl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, rpc_in);\n-\tcurl_easy_setopt(slot->curl, CURLOPT_FILE, rpc);\n-\n+\trpc_in_data.rpc = rpc;\n+\trpc_in_data.slot = slot;\n+\tcurl_easy_setopt(slot->curl, CURLOPT_FILE, &rpc_in_data);\n \n \trpc->any_written = 0;\n \terr = run_slot(slot, NULL);\n-- \n2.20.1.415.g653613c723-goog\n\n"},{"id":"365970","messageId":"20181229194447.157763-3-masayasuzuki@google.com","threadId":"50122","inReplyTo":"20181229194447.157763-1-masayasuzuki@google.com","subject":"[PATCH v2 2/2] Unset CURLOPT_FAILONERROR","fromName":"Masaya Suzuki","fromEmail":"masayasuzuki@google.com","sentAt":"2018-12-29T19:44:47Z","receivedAt":"2018-12-29T19:44:59Z","isPatch":true,"sender":{"key":"masayasuzuki@google.com","avatar":"https://avatars.githubusercontent.com/u/21878?v=4"},"body":"When GIT_CURL_VERBOSE is set, libcurl produces request/response headers\nto stderr. However, if the response is an error response and\nCURLOPT_FAILONERROR is set, libcurl stops parsing the response, and it\nwon't dump the headers. Showing HTTP response headers is useful for\ndebugging, especially for non-OK responses.\n\nThis is substantially same as setting http_options.keep_error to all\nrequests. Hence, remove this option.\n\nSigned-off-by: Masaya Suzuki \n---\n http.c | 4 ----\n http.h | 1 -\n remote-curl.c | 1 -\n t/lib-httpd/apache.conf | 1 +\n t/t5581-http-curl-verbose.sh | 28 ++++++++++++++++++++++++++++\n 5 files changed, 29 insertions(+), 6 deletions(-)\n create mode 100755 t/t5581-http-curl-verbose.sh\n\ndiff --git a/http.c b/http.c\nindex d23417670..8f8101da3 100644\n--- a/http.c\n+++ b/http.c\n@@ -1269,7 +1269,6 @@ struct active_request_slot *get_active_slot(void)\n \tcurl_easy_setopt(slot->curl, CURLOPT_POSTFIELDS, NULL);\n \tcurl_easy_setopt(slot->curl, CURLOPT_UPLOAD, 0);\n \tcurl_easy_setopt(slot->curl, CURLOPT_HTTPGET, 1);\n-\tcurl_easy_setopt(slot->curl, CURLOPT_FAILONERROR, 1);\n \tcurl_easy_setopt(slot->curl, CURLOPT_RANGE, NULL);\n \n \t/*\n@@ -1848,8 +1847,6 @@ static int http_request(const char *url,\n \tstrbuf_addstr(&buf, \"Pragma:\");\n \tif (options && options->no_cache)\n \t\tstrbuf_addstr(&buf, \" no-cache\");\n-\tif (options && options->keep_error)\n-\t\tcurl_easy_setopt(slot->curl, CURLOPT_FAILONERROR, 0);\n \tif (options && options->initial_request &&\n \t http_follow_config == HTTP_FOLLOW_INITIAL)\n \t\tcurl_easy_setopt(slot->curl, CURLOPT_FOLLOWLOCATION, 1);\n@@ -2415,7 +2412,6 @@ struct http_object_request *new_http_object_request(const char *base_url,\n \tfreq->slot = get_active_slot();\n \n \tcurl_easy_setopt(freq->slot->curl, CURLOPT_FILE, freq);\n-\tcurl_easy_setopt(freq->slot->curl, CURLOPT_FAILONERROR, 0);\n \tcurl_easy_setopt(freq->slot->curl, CURLOPT_WRITEFUNCTION, fwrite_sha1_file);\n \tcurl_easy_setopt(freq->slot->curl, CURLOPT_ERRORBUFFER, freq->errorstr);\n \tcurl_easy_setopt(freq->slot->curl, CURLOPT_URL, freq->url);\ndiff --git a/http.h b/http.h\nindex d305ca1dc..eebf40688 100644\n--- a/http.h\n+++ b/http.h\n@@ -146,7 +146,6 @@ extern char *get_remote_object_url(const char *url, const char *hex,\n /* Options for http_get_*() */\n struct http_get_options {\n \tunsigned no_cache:1,\n-\t\t keep_error:1,\n \t\t initial_request:1;\n \n \t/* If non-NULL, returns the content-type of the response. */\ndiff --git a/remote-curl.c b/remote-curl.c\nindex 48656bf18..43e7a1d80 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -380,7 +380,6 @@ static struct discovery *discover_refs(const char *service, int for_push)\n \thttp_options.extra_headers = &extra_headers;\n \thttp_options.initial_request = 1;\n \thttp_options.no_cache = 1;\n-\thttp_options.keep_error = 1;\n \n \thttp_ret = http_get_strbuf(refs_url.buf, &buffer, &http_options);\n \tswitch (http_ret) {\ndiff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf\nindex 581c010d8..cc4b87507 100644\n--- a/t/lib-httpd/apache.conf\n+++ b/t/lib-httpd/apache.conf\n@@ -115,6 +115,7 @@ Alias /auth/dumb/ www/auth/dumb/\n \tSetEnv GIT_EXEC_PATH ${GIT_EXEC_PATH}\n \tSetEnv GIT_HTTP_EXPORT_ALL\n \n+ScriptAliasMatch /error_git_upload_pack/(.*)/git-upload-pack error.sh/\n ScriptAliasMatch /smart_*[^/]*/(.*) ${GIT_EXEC_PATH}/git-http-backend/$1\n ScriptAlias /broken_smart/ broken-smart-http.sh/\n ScriptAlias /error/ error.sh/\ndiff --git a/t/t5581-http-curl-verbose.sh b/t/t5581-http-curl-verbose.sh\nnew file mode 100755\nindex 000000000..73148eeba\n--- /dev/null\n+++ b/t/t5581-http-curl-verbose.sh\n@@ -0,0 +1,28 @@\n+#!/bin/sh\n+\n+test_description='test GIT_CURL_VERBOSE'\n+. ./test-lib.sh\n+. \"$TEST_DIRECTORY\"/lib-httpd.sh\n+start_httpd\n+\n+test_expect_success 'setup repository' '\n+\tmkdir \"$HTTPD_DOCUMENT_ROOT_PATH/repo.git\" &&\n+\tgit -C \"$HTTPD_DOCUMENT_ROOT_PATH/repo.git\" --bare init &&\n+\tgit config push.default matching &&\n+\techo content >file &&\n+\tgit add file &&\n+\tgit commit -m one &&\n+\tgit remote add public \"$HTTPD_DOCUMENT_ROOT_PATH/repo.git\" &&\n+\tgit push public master:master\n+'\n+\n+test_expect_success 'failure in git-upload-pack is shown' '\n+\ttest_might_fail env GIT_CURL_VERBOSE=1 \\\n+\t\tgit clone \"$HTTPD_URL/error_git_upload_pack/smart/repo.git\" \\\n+\t\t2>curl_log &&\n+\tcat curl_log | grep \"< HTTP/1.1 500 Intentional Breakage\"\n+'\n+\n+stop_httpd\n+\n+test_done\n-- \n2.20.1.415.g653613c723-goog\n\n"},{"id":"366097","messageId":"xmqqtviph835.fsf@gitster-ct.c.googlers.com","threadId":"50122","inReplyTo":"20181229194447.157763-2-masayasuzuki@google.com","subject":"Re: [PATCH v2 1/2] Change how HTTP response body is returned","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-01-03T19:09:02Z","receivedAt":"2019-01-03T19:09:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Masaya Suzuki writes:\n\n> Subject: Re: [PATCH v2 1/2] Change how HTTP response body is returned\n\nPerhaps:\n\n Subject: http: change the way response body is returned\n\nbut if we can state why we want to do so concisely, that would be\neven better.\n\n> This changes the way HTTP response body is returned in\n> http_request_reauth and post_rpc.\n>\n> 1. http_request_reauth makes up to two requests; one without a\n> credential and one with a credential. The first request can fail if\n> it needs a credential. When the keep_error option is specified, the\n> response to the first request can be written to the HTTP response\n> body destination. If the response body destination is a string\n> buffer, it erases the buffer before making the second request. By\n> introducing http_response_dest, it can handle the case that the\n> destination is a file handle.\n> 2. post_rpc makes an HTTP request and the response body is directly\n> written to a file descriptor. This makes it check the HTTP status\n> code before writing it, and do not write the response body if it's an\n> error response. It's ok without this check now because post_rpc makes\n> a request with CURLOPT_FAILONERROR, and libcurl won't call the\n> callback if the response has an error status code.\n\nThe above may be an accurate description of what the code will do\nwith this patch, but I cannot quite read the reason why we would\nwant the code to behave so in the first place.\n\n>\n> Signed-off-by: Masaya Suzuki \n> ---\n> http.c | 99 +++++++++++++++++++++++++++++----------------------\n> remote-curl.c | 29 ++++++++++++---\n> 2 files changed, 81 insertions(+), 47 deletions(-)\n>\n> diff --git a/http.c b/http.c\n> index eacc2a75e..d23417670 100644\n> --- a/http.c\n> +++ b/http.c\n> @@ -165,6 +165,19 @@ static int http_schannel_check_revoke = 1;\n> */\n> static int http_schannel_use_ssl_cainfo;\n> \n> +/*\n> + * Where to store the result of http_request.\n> + *\n> + * At most one of buffer or file can be non-NULL. The buffer and file are not\n> + * allocated by http_request, and the caller is responsible for releasing them.\n> + */\n> +struct http_response_dest {\n> +\tstruct strbuf *buffer;\n> +\n> +\tFILE *file;\n> +\tconst char *filename;\n> +};\n> +\n> size_t fread_buffer(char *ptr, size_t eltsize, size_t nmemb, void *buffer_)\n> {\n> \tsize_t size = eltsize * nmemb;\n> @@ -1794,12 +1807,8 @@ static void http_opt_request_remainder(CURL *curl, off_t pos)\n> \tcurl_easy_setopt(curl, CURLOPT_RANGE, buf);\n> }\n> \n> -/* http_request() targets */\n> -#define HTTP_REQUEST_STRBUF\t0\n> -#define HTTP_REQUEST_FILE\t1\n> -\n> static int http_request(const char *url,\n> -\t\t\tvoid *result, int target,\n> +\t\t\tstruct http_response_dest *dest,\n> \t\t\tconst struct http_get_options *options)\n> {\n> \tstruct active_request_slot *slot;\n> @@ -1812,21 +1821,23 @@ static int http_request(const char *url,\n> \tslot = get_active_slot();\n> \tcurl_easy_setopt(slot->curl, CURLOPT_HTTPGET, 1);\n> \n> -\tif (result == NULL) {\n> -\t\tcurl_easy_setopt(slot->curl, CURLOPT_NOBODY, 1);\n> -\t} else {\n> +\tif (dest->file) {\n> +\t\toff_t posn = ftello(dest->file);\n> \t\tcurl_easy_setopt(slot->curl, CURLOPT_NOBODY, 0);\n> -\t\tcurl_easy_setopt(slot->curl, CURLOPT_FILE, result);\n\nOK, so it used to be that we can either discard the result\n(i.e. NOBODY) or send the result to CURLOPT_FILE, and the latter has\ntwo options (sent to a file, or sent to in-core buffer). The way\nthese three choices are given were with the result pointer and the\ntarget enum.\n\nThat is replaced by a struct that allows the same three choices.\n\nMakes sense so far.\n\n> @@ -1930,10 +1941,10 @@ static int update_url_from_redirect(struct strbuf *base,\n> }\n> \n> static int http_request_reauth(const char *url,\n> -\t\t\t void *result, int target,\n> +\t\t\t struct http_response_dest *dest,\n> \t\t\t struct http_get_options *options)\n> {\n> -\tint ret = http_request(url, result, target, options);\n> +\tint ret = http_request(url, dest, options);\n\nJust adjusting the calling convention to the change we saw above.\n\n> @@ -1949,32 +1960,34 @@ static int http_request_reauth(const char *url,\n> \tif (ret != HTTP_REAUTH)\n> \t\treturn ret;\n> \n> -\t/*\n> -\t * If we are using KEEP_ERROR, the previous request may have\n> -\t * put cruft into our output stream; we should clear it out before\n> -\t * making our next request. We only know how to do this for\n> -\t * the strbuf case, but that is enough to satisfy current callers.\n> -\t */\n> -\tif (options && options->keep_error) {\n> -\t\tswitch (target) {\n> -\t\tcase HTTP_REQUEST_STRBUF:\n> -\t\t\tstrbuf_reset(result);\n> -\t\t\tbreak;\n> -\t\tdefault:\n> -\t\t\tBUG(\"HTTP_KEEP_ERROR is only supported with strbufs\");\n\nNow it gets interesting. We used to allow keep-error only when\nreceiving to in-core buffer.\n\n> +\tif (dest->file) {\n> +\t\t/*\n> +\t\t * At this point, the file contains the response body of the\n> +\t\t * previous request. We need to truncate the file.\n> +\t\t */\n> +\t\tFILE *new_file = freopen(dest->filename, \"w\", dest->file);\n\nNow freopen() lets us restart the file anew with a new \"FILE *\".\n\n> +\t\tif (new_file == NULL) {\n> +\t\t\terror(\"Unable to open local file %s\", dest->filename);\n\nerror_errno(), perhaps?\n\nAt this point, I presume that dest->file is closed by the failed\nfreopen(), but dest->file is still non-NULL and causes further calls\nto http_request() with this dest would be a disaster? As long as\nthe caller of this function reacts to HTTP_ERROR and kill the dest,\nit would be fine.\n\n> +\t\t\treturn HTTP_ERROR;\n> \t\t}\n> +\t\tdest->file = new_file;\n> +\t} else if (dest->buffer) {\n> +\t\tstrbuf_reset(dest->buffer);\n> \t}\n\nOK.\n\n> \tcredential_fill(&http_auth);\n> \n> -\treturn http_request(url, result, target, options);\n> +\treturn http_request(url, dest, options);\n> }\n\nSo far, I spotted one reason why this patch wants to exist: it used\nto be that keep_error was impossible when sending to a file. It\nsomehow wants to allow us to do so (even though it still is unclear\nwho that new caller that wants to use keep_error is, and for what\npurpose it wants to use that facility).\n\nPerhaps this step can be split into at least two steps? The first\none is to turn to without changing any other\nbehaviour, and then the second one implements keep_error handling\nfor the \"dest->file != NULL\" case.\n\nThere may be other things this patch does, in which case it may\ndeserve to become three or more steps, but we haven't seen enough to\ndecide if that is the case. Let's read on.\n\n> int http_get_strbuf(const char *url,\n> -\t\t struct strbuf *result,\n> +\t\t struct strbuf *dest_buffer,\n> \t\t struct http_get_options *options)\n> {\n> -\treturn http_request_reauth(url, result, HTTP_REQUEST_STRBUF, options);\n> +\tstruct http_response_dest dest;\n> +\tdest.file = NULL;\n> +\tdest.buffer = dest_buffer;\n> +\treturn http_request_reauth(url, &dest, options);\n\nThis is merely adjusting for the updated calling convention, which\nmakes sense.\n\n> }\n> \n> /*\n> @@ -1988,18 +2001,20 @@ static int http_get_file(const char *url, const char *filename,\n> {\n> \tint ret;\n> \tstruct strbuf tmpfile = STRBUF_INIT;\n> -\tFILE *result;\n> +\tstruct http_response_dest dest;\n> \n> \tstrbuf_addf(&tmpfile, \"%s.temp\", filename);\n> -\tresult = fopen(tmpfile.buf, \"a\");\n> -\tif (!result) {\n> +\tdest.buffer = NULL;\n> +\tdest.file = fopen(tmpfile.buf, \"a\");\n> +\tif (!dest.file) {\n> \t\terror(\"Unable to open local file %s\", tmpfile.buf);\n> \t\tret = HTTP_ERROR;\n> \t\tgoto cleanup;\n> \t}\n> +\tdest.filename = tmpfile.buf;\n\nHmph. I somehow expected that the presence of dest.filename field\nwould allow callers to just set it and let the fopen() call handled\nin http_request(), e.g. at the beginning of the function, it would\ndo something like\n\n\tif (!dest->file && dest->filename)\n\t\tdest->file = fopen(...);\n\nBut obviously that is not within the scope of this change. Leaving\nthe caller responsible for opening and reporting errors as before\nlike the above is preferrable.\n\n> diff --git a/remote-curl.c b/remote-curl.c\n> index 1220dffcd..48656bf18 100644\n> --- a/remote-curl.c\n> +++ b/remote-curl.c\n> @@ -546,14 +546,31 @@ static curlioerr rpc_ioctl(CURL *handle, int cmd, void *clientp)\n> }\n> #endif\n> \n> +struct rpc_in_data {\n> +\tstruct rpc_state *rpc;\n> +\tstruct active_request_slot *slot;\n> +};\n> +\n> +/*\n> + * A callback for CURLOPT_WRITEFUNCTION. The return value is the bytes consumed\n> + * from ptr.\n> + */\n> static size_t rpc_in(char *ptr, size_t eltsize,\n> \t\tsize_t nmemb, void *buffer_)\n> {\n> \tsize_t size = eltsize * nmemb;\n> -\tstruct rpc_state *rpc = buffer_;\n> +\tstruct rpc_in_data *data = buffer_;\n> +\tlong response_code;\n> +\n> +\tif (curl_easy_getinfo(data->slot->curl, CURLINFO_RESPONSE_CODE,\n> +\t\t\t &response_code) != CURLE_OK)\n> +\t\treturn size;\n> +\tif (response_code != 200)\n> +\t\treturn size;\n> +\n> \tif (size)\n> -\t\trpc->any_written = 1;\n> -\twrite_or_die(rpc->in, ptr, size);\n> +\t\tdata->rpc->any_written = 1;\n> +\twrite_or_die(data->rpc->in, ptr, size);\n> \treturn size;\n> }\n\nThis is not necessarily related to the change we saw in http.c but\nmaking it more careful in general? That is, we avoid writing the\npayload to the destination (by the way, rpc->IN being the target of\nwrite(2) is somewhat a brain-twister). It may deserve to become its\nown step in a patch series, with separate justification (i.e. \"when\nrpc channel receives an error from the cURL library, we used to\nwrite the data to the file anyway, and that caused such and such\nproblems, as demonstrated by the new test added by this patch. we\nfix it by checking for the error before writing to the file\").\n\n> @@ -633,6 +650,7 @@ static int post_rpc(struct rpc_state *rpc)\n> \tsize_t gzip_size = 0;\n> \tint err, large_request = 0;\n> \tint needs_100_continue = 0;\n> +\tstruct rpc_in_data rpc_in_data;\n> \n> \t/* Try to load the entire request, if we can fit it into the\n> \t * allocated buffer space we can use HTTP/1.0 and avoid the\n> @@ -765,8 +783,9 @@ static int post_rpc(struct rpc_state *rpc)\n> \n> \tcurl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, headers);\n> \tcurl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, rpc_in);\n> -\tcurl_easy_setopt(slot->curl, CURLOPT_FILE, rpc);\n> -\n> +\trpc_in_data.rpc = rpc;\n> +\trpc_in_data.slot = slot;\n> +\tcurl_easy_setopt(slot->curl, CURLOPT_FILE, &rpc_in_data);\n> \n> \trpc->any_written = 0;\n> \terr = run_slot(slot, NULL);\n\nThese two hunks look like mere adjustments for the new callback\ntype, which makes sense.\n\nThanks.\n"},{"id":"366137","messageId":"20190104101149.GA26185@sigill.intra.peff.net","threadId":"50122","inReplyTo":"xmqqtviph835.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2 1/2] Change how HTTP response body is returned","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-01-04T10:11:50Z","receivedAt":"2019-01-04T10:11:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 03, 2019 at 11:09:02AM -0800, Junio C Hamano wrote:\n\n> > +\tif (dest->file) {\n> > +\t\t/*\n> > +\t\t * At this point, the file contains the response body of the\n> > +\t\t * previous request. We need to truncate the file.\n> > +\t\t */\n> > +\t\tFILE *new_file = freopen(dest->filename, \"w\", dest->file);\n> \n> Now freopen() lets us restart the file anew with a new \"FILE *\".\n> \n> > +\t\tif (new_file == NULL) {\n> > +\t\t\terror(\"Unable to open local file %s\", dest->filename);\n> \n> error_errno(), perhaps?\n> \n> At this point, I presume that dest->file is closed by the failed\n> freopen(), but dest->file is still non-NULL and causes further calls\n> to http_request() with this dest would be a disaster? As long as\n> the caller of this function reacts to HTTP_ERROR and kill the dest,\n> it would be fine.\n\nI also wondered what timing guarantees freopen() gives us (i.e., is it\npossible for it to open and truncate the file, and then close the old\nhandle, flushing some in-memory buffer). C99 says:\n\n The freopen function first attempts to close any file that is\n associated with the specified stream. Failure to close the file is\n ignored. The error and end-of-file indicators for the stream are\n cleared.\n\nSo I think the order is OK for my concern, but not for yours. I.e., on\nan error, dest->file is now undefined.\n\nIt might be nice to set \"dest->file == NULL\" in that case. There's no\nguarantee that the caller did not hold onto its own copy of the handle,\nbut since this struct is only exposed internally within http.c, that's\nprobably OK.\n\nThe most robust thing would perhaps be:\n\n fflush(dest->file);\n ftruncate(fileno(dest->file), 0);\n\nwhich leaves the handle intact.\n\n(I agree with the rest of your review, especially that it would be\neasier to read if this were split into separate refactor and\nchange-behavior steps).\n\n-Peff\n"},{"id":"366138","messageId":"20190104103042.GB26185@sigill.intra.peff.net","threadId":"50122","inReplyTo":"20181229194447.157763-2-masayasuzuki@google.com","subject":"Re: [PATCH v2 1/2] Change how HTTP response body is returned","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-01-04T10:30:42Z","receivedAt":"2019-01-04T10:30:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Dec 29, 2018 at 11:44:46AM -0800, Masaya Suzuki wrote:\n\n> +/*\n> + * A callback for CURLOPT_WRITEFUNCTION. The return value is the bytes consumed\n> + * from ptr.\n> + */\n> static size_t rpc_in(char *ptr, size_t eltsize,\n> \t\tsize_t nmemb, void *buffer_)\n> {\n> \tsize_t size = eltsize * nmemb;\n> -\tstruct rpc_state *rpc = buffer_;\n> +\tstruct rpc_in_data *data = buffer_;\n> +\tlong response_code;\n> +\n> +\tif (curl_easy_getinfo(data->slot->curl, CURLINFO_RESPONSE_CODE,\n> +\t\t\t &response_code) != CURLE_OK)\n> +\t\treturn size;\n\nThis hunk was unexpected to me. The function here is just writing out\nthe data, and I expected we'd handle the error after the whole transfer\nis done. But we do that anyway eventually via run_slot() (which uses\nhandle_curl_result). I guess the goal here is to start throwing away\ndata when we see an error, rather than storing it?\n\nThat makes some sense, though I do wonder if there's any case where curl\nwould call our WRITEFUNCTION before it knows the HTTP status. That\nimplies a body before our header, which seems impossible, though.\n\n> +\tif (response_code != 200)\n> +\t\treturn size;\n\nThe current behavior with CURLOPT_FAILONERROR treats codes >= 400 as an\nerror. And in handle_curl_result(), we treat >= 300 as an error (since\nwe only see 3xx for a disabled redirect). I suppose it's unlikely for us\nto see any success code besides 200, but we probably ought to be\nfollowing the same rules here.\n\n-Peff\n"},{"id":"366139","messageId":"20190104104907.GC26185@sigill.intra.peff.net","threadId":"50122","inReplyTo":"20181229194447.157763-3-masayasuzuki@google.com","subject":"Re: [PATCH v2 2/2] Unset CURLOPT_FAILONERROR","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-01-04T10:49:07Z","receivedAt":"2019-01-04T10:49:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Dec 29, 2018 at 11:44:47AM -0800, Masaya Suzuki wrote:\n\n> When GIT_CURL_VERBOSE is set, libcurl produces request/response headers\n> to stderr. However, if the response is an error response and\n> CURLOPT_FAILONERROR is set, libcurl stops parsing the response, and it\n> won't dump the headers. Showing HTTP response headers is useful for\n> debugging, especially for non-OK responses.\n\nOut of curiosity, does GIT_TRACE_CURL do any better? Or is it simply\nthat curl closes the handle when it sees the bad response code, and\nnobody ever gets to see the rest of the data?\n\n> This is substantially same as setting http_options.keep_error to all\n> requests. Hence, remove this option.\n\nThe assumption here is that every code path using FAILONERROR is\nprepared to handle the failing http response codes itself (since we no\nlonger set it at all in get_active_slot()). Is that so?\n\nAnything that uses handle_curl_result() is OK. That means run_one_slot()\nis fine, which in turn covers run_slot() for RPCs, and http_request()\nfor normal one-at-a-time requests. But what about the parallel multiple\nrequests issued by the dumb-http walker code?\n\nThere I think we end up in step_active_slots(), which calls into\nfinish_active_slot() for completed requests. I think that\nunconditionally fetches the http code without bothering to look at\nwhether curl reported success or not.\n\nSo I _think_ that's probably all of the users of the curl handles\nprovided by get_active_slot(). Though given the tangled mess of our HTTP\ncode, I won't be surprised if there's a corner case I missed in that\nanalysis.\n\n-Peff\n"},{"id":"366158","messageId":"xmqqsgy8b2r7.fsf@gitster-ct.c.googlers.com","threadId":"50122","inReplyTo":"20190104101149.GA26185@sigill.intra.peff.net","subject":"Re: [PATCH v2 1/2] Change how HTTP response body is returned","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-01-04T20:13:00Z","receivedAt":"2019-01-04T20:13:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> The most robust thing would perhaps be:\n>\n> fflush(dest->file);\n> ftruncate(fileno(dest->file), 0);\n>\n> which leaves the handle intact.\n\nAn added benefit of that approach is that there is no need for\nthe filename field in the dest structure.\n\nHaving a separate filename field could be a positive flexibility (it\ncould be used to open a file, store its FILE* in dest->file, while\nstoring the name of another file in dest->filename), but also a\nnegative flexibility (dest->file possibly pointing to a file\ndifferent from dest->filename is a source of confusion). As I do\nnot think any current caller that wants such a flexibility, or\ncallers in any immediate future, it probably is an overall plus if\nwe do not have to add the dest->filename field.\n\n> (I agree with the rest of your review, especially that it would be\n> easier to read if this were split into separate refactor and\n> change-behavior steps).\n>\n> -Peff\n"},{"id":"366301","messageId":"CAJB1erVjLwnKqy5zSMTBNySSUrJXiiL8Xrpw9puh7o4CDaeA2Q@mail.gmail.com","threadId":"50122","inReplyTo":"20190104104907.GC26185@sigill.intra.peff.net","subject":"Re: [PATCH v2 2/2] Unset CURLOPT_FAILONERROR","fromName":"Masaya Suzuki","fromEmail":"masayasuzuki@google.com","sentAt":"2019-01-07T23:24:19Z","receivedAt":"2019-01-07T23:24:34Z","isPatch":true,"sender":{"key":"masayasuzuki@google.com","avatar":"https://avatars.githubusercontent.com/u/21878?v=4"},"body":"On Fri, Jan 4, 2019 at 2:49 AM Jeff King wrote:\n>\n> On Sat, Dec 29, 2018 at 11:44:47AM -0800, Masaya Suzuki wrote:\n>\n> > When GIT_CURL_VERBOSE is set, libcurl produces request/response headers\n> > to stderr. However, if the response is an error response and\n> > CURLOPT_FAILONERROR is set, libcurl stops parsing the response, and it\n> > won't dump the headers. Showing HTTP response headers is useful for\n> > debugging, especially for non-OK responses.\n>\n> Out of curiosity, does GIT_TRACE_CURL do any better? Or is it simply\n> that curl closes the handle when it sees the bad response code, and\n> nobody ever gets to see the rest of the data?\n\ncurl disregards the rest of the contents when it sees a bad response\ncode when CURLOPT_FAILONERROR is set\n(https://github.com/curl/curl/blob/dea3f94298ac0859464768959488938c4e104545/lib/http.c#L3691).\nSo nobody gets the rest of the data.\n\n>\n> > This is substantially same as setting http_options.keep_error to all\n> > requests. Hence, remove this option.\n>\n> The assumption here is that every code path using FAILONERROR is\n> prepared to handle the failing http response codes itself (since we no\n> longer set it at all in get_active_slot()). Is that so?\n\nI was thinking that I covered the all code paths using FAILONERROR,\nbut it seems it's not the case. http-walker.c and http-push.c also\ncalls get_active_slot(). I'll narrow down the scope on removing\nFAILONERROR.\n\n>\n> Anything that uses handle_curl_result() is OK. That means run_one_slot()\n> is fine, which in turn covers run_slot() for RPCs, and http_request()\n> for normal one-at-a-time requests. But what about the parallel multiple\n> requests issued by the dumb-http walker code?\n\nRight. I'll keep FAILONERROR in get_active_slot and remove it only for\nthe code paths that can handle HTTP errors.\n\n>\n> There I think we end up in step_active_slots(), which calls into\n> finish_active_slot() for completed requests. I think that\n> unconditionally fetches the http code without bothering to look at\n> whether curl reported success or not.\n>\n> So I _think_ that's probably all of the users of the curl handles\n> provided by get_active_slot(). Though given the tangled mess of our HTTP\n> code, I won't be surprised if there's a corner case I missed in that\n> analysis.\n>\n> -Peff\n"},{"id":"366310","messageId":"20190108024741.62176-1-masayasuzuki@google.com","threadId":"50122","inReplyTo":"20181229194447.157763-1-masayasuzuki@google.com","subject":"[PATCH v3 0/5] Show HTTP headers of failed requests with GIT_CURL_VERBOSE","fromName":"Masaya Suzuki","fromEmail":"masayasuzuki@google.com","sentAt":"2019-01-08T02:47:36Z","receivedAt":"2019-01-08T02:47:58Z","isPatch":true,"sender":{"key":"masayasuzuki@google.com","avatar":"https://avatars.githubusercontent.com/u/21878?v=4"},"body":"Diff from v2[1]:\n\n* Remove http_resonse_dest\n\n This was introduced to have a filename for freopen. Jeff King proposed [2]\n using fflush and ftruncate and this makes this struct not needed.\n\n* Unset CURLOPT_FAILONERROR only when it's necessary.\n\n Previously, CURLOPT_FAILONERROR was unset for everything. This patch series\n does so only when it's necessary. This is from the observation in [3] that\n pointed out there are other possible code paths that hit http.c.\n\n* Split the patches for easier review\n\n[1]: https://public-inbox.org/git/20181229194447.157763-1-masayasuzuki@google.com/\n[2]: https://public-inbox.org/git/20190104101149.GA26185@sigill.intra.peff.net/\n[3]: https://public-inbox.org/git/20190104104907.GC26185@sigill.intra.peff.net/\n\nMasaya Suzuki (5):\n http: support file handles for HTTP_KEEP_ERROR\n http: enable keep_error for HTTP requests\n remote-curl: define struct for CURLOPT_WRITEFUNCTION\n remote-curl: unset CURLOPT_FAILONERROR\n test: test GIT_CURL_VERBOSE=1 shows an error\n\n http.c | 27 +++++++++++++--------------\n http.h | 1 -\n remote-curl.c | 29 ++++++++++++++++++++++++-----\n t/lib-httpd/apache.conf | 1 +\n t/t5581-http-curl-verbose.sh | 28 ++++++++++++++++++++++++++++\n 5 files changed, 66 insertions(+), 20 deletions(-)\n create mode 100755 t/t5581-http-curl-verbose.sh\n\n-- \n2.20.1.97.g81188d93c3-goog\n\n"},{"id":"366311","messageId":"20190108024741.62176-2-masayasuzuki@google.com","threadId":"50122","inReplyTo":"20190108024741.62176-1-masayasuzuki@google.com","subject":"[PATCH v3 1/5] http: support file handles for HTTP_KEEP_ERROR","fromName":"Masaya Suzuki","fromEmail":"masayasuzuki@google.com","sentAt":"2019-01-08T02:47:37Z","receivedAt":"2019-01-08T02:48:01Z","isPatch":true,"sender":{"key":"masayasuzuki@google.com","avatar":"https://avatars.githubusercontent.com/u/21878?v=4"},"body":"HTTP_KEEP_ERROR makes it easy to debug HTTP transport errors. In order\nto make HTTP_KEEP_ERROR enabled for all requests, file handles need to\nbe supported.\n\nSigned-off-by: Masaya Suzuki \n---\n http.c | 9 ++++++---\n 1 file changed, 6 insertions(+), 3 deletions(-)\n\ndiff --git a/http.c b/http.c\nindex 0b6807cef9..06450da96e 100644\n--- a/http.c\n+++ b/http.c\n@@ -1991,16 +1991,19 @@ static int http_request_reauth(const char *url,\n \t/*\n \t * If we are using KEEP_ERROR, the previous request may have\n \t * put cruft into our output stream; we should clear it out before\n-\t * making our next request. We only know how to do this for\n-\t * the strbuf case, but that is enough to satisfy current callers.\n+\t * making our next request.\n \t */\n \tif (options && options->keep_error) {\n \t\tswitch (target) {\n \t\tcase HTTP_REQUEST_STRBUF:\n \t\t\tstrbuf_reset(result);\n \t\t\tbreak;\n+\t\tcase HTTP_REQUEST_FILE:\n+\t\t\tfflush(result);\n+\t\t\tftruncate(fileno(result), 0);\n+\t\t\tbreak;\n \t\tdefault:\n-\t\t\tBUG(\"HTTP_KEEP_ERROR is only supported with strbufs\");\n+\t\t\tBUG(\"Unknown http_request target\");\n \t\t}\n \t}\n \n-- \n2.20.1.97.g81188d93c3-goog\n\n"},{"id":"366312","messageId":"20190108024741.62176-3-masayasuzuki@google.com","threadId":"50122","inReplyTo":"20190108024741.62176-1-masayasuzuki@google.com","subject":"[PATCH v3 2/5] http: enable keep_error for HTTP requests","fromName":"Masaya Suzuki","fromEmail":"masayasuzuki@google.com","sentAt":"2019-01-08T02:47:38Z","receivedAt":"2019-01-08T02:48:04Z","isPatch":true,"sender":{"key":"masayasuzuki@google.com","avatar":"https://avatars.githubusercontent.com/u/21878?v=4"},"body":"curl stops parsing a response when it sees a bad HTTP status code and it\nhas CURLOPT_FAILONERROR set. This prevents GIT_CURL_VERBOSE to show HTTP\nheaders on error.\n\nkeep_error is an option to receive the HTTP response body for those\nerror responses. By enabling this option, curl will process the HTTP\nresponse headers, and they're shown if GIT_CURL_VERBOSE is set.\n\nSigned-off-by: Masaya Suzuki \n---\n http.c | 30 +++++++++++++-----------------\n http.h | 1 -\n remote-curl.c | 1 -\n 3 files changed, 13 insertions(+), 19 deletions(-)\n\ndiff --git a/http.c b/http.c\nindex 06450da96e..a72db87723 100644\n--- a/http.c\n+++ b/http.c\n@@ -1876,8 +1876,6 @@ static int http_request(const char *url,\n \tstrbuf_addstr(&buf, \"Pragma:\");\n \tif (options && options->no_cache)\n \t\tstrbuf_addstr(&buf, \" no-cache\");\n-\tif (options && options->keep_error)\n-\t\tcurl_easy_setopt(slot->curl, CURLOPT_FAILONERROR, 0);\n \tif (options && options->initial_request &&\n \t http_follow_config == HTTP_FOLLOW_INITIAL)\n \t\tcurl_easy_setopt(slot->curl, CURLOPT_FOLLOWLOCATION, 1);\n@@ -1895,6 +1893,7 @@ static int http_request(const char *url,\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, \"\");\n+\tcurl_easy_setopt(slot->curl, CURLOPT_FAILONERROR, 0);\n \n \tret = run_one_slot(slot, &results);\n \n@@ -1989,22 +1988,19 @@ static int http_request_reauth(const char *url,\n \t\treturn ret;\n \n \t/*\n-\t * If we are using KEEP_ERROR, the previous request may have\n-\t * put cruft into our output stream; we should clear it out before\n-\t * making our next request.\n+\t * The previous request may have put cruft into our output stream; we\n+\t * should clear it out before making our next request.\n \t */\n-\tif (options && options->keep_error) {\n-\t\tswitch (target) {\n-\t\tcase HTTP_REQUEST_STRBUF:\n-\t\t\tstrbuf_reset(result);\n-\t\t\tbreak;\n-\t\tcase HTTP_REQUEST_FILE:\n-\t\t\tfflush(result);\n-\t\t\tftruncate(fileno(result), 0);\n-\t\t\tbreak;\n-\t\tdefault:\n-\t\t\tBUG(\"Unknown http_request target\");\n-\t\t}\n+\tswitch (target) {\n+\tcase HTTP_REQUEST_STRBUF:\n+\t\tstrbuf_reset(result);\n+\t\tbreak;\n+\tcase HTTP_REQUEST_FILE:\n+\t\tfflush(result);\n+\t\tftruncate(fileno(result), 0);\n+\t\tbreak;\n+\tdefault:\n+\t\tBUG(\"Unknown http_request target\");\n \t}\n \n \tcredential_fill(&http_auth);\ndiff --git a/http.h b/http.h\nindex d305ca1dc7..eebf40688c 100644\n--- a/http.h\n+++ b/http.h\n@@ -146,7 +146,6 @@ extern char *get_remote_object_url(const char *url, const char *hex,\n /* Options for http_get_*() */\n struct http_get_options {\n \tunsigned no_cache:1,\n-\t\t keep_error:1,\n \t\t initial_request:1;\n \n \t/* If non-NULL, returns the content-type of the response. */\ndiff --git a/remote-curl.c b/remote-curl.c\nindex 1220dffcdc..d8eda2380a 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -380,7 +380,6 @@ static struct discovery *discover_refs(const char *service, int for_push)\n \thttp_options.extra_headers = &extra_headers;\n \thttp_options.initial_request = 1;\n \thttp_options.no_cache = 1;\n-\thttp_options.keep_error = 1;\n \n \thttp_ret = http_get_strbuf(refs_url.buf, &buffer, &http_options);\n \tswitch (http_ret) {\n-- \n2.20.1.97.g81188d93c3-goog\n\n"},{"id":"366313","messageId":"20190108024741.62176-4-masayasuzuki@google.com","threadId":"50122","inReplyTo":"20190108024741.62176-1-masayasuzuki@google.com","subject":"[PATCH v3 3/5] remote-curl: define struct for CURLOPT_WRITEFUNCTION","fromName":"Masaya Suzuki","fromEmail":"masayasuzuki@google.com","sentAt":"2019-01-08T02:47:39Z","receivedAt":"2019-01-08T02:48:07Z","isPatch":true,"sender":{"key":"masayasuzuki@google.com","avatar":"https://avatars.githubusercontent.com/u/21878?v=4"},"body":"In order to pass more values for rpc_in, define a struct and pass it as\nan additional value.\n\nSigned-off-by: Masaya Suzuki \n---\n remote-curl.c | 18 ++++++++++++++----\n 1 file changed, 14 insertions(+), 4 deletions(-)\n\ndiff --git a/remote-curl.c b/remote-curl.c\nindex d8eda2380a..d4673b6e8c 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -545,14 +545,22 @@ static curlioerr rpc_ioctl(CURL *handle, int cmd, void *clientp)\n }\n #endif\n \n+struct rpc_in_data {\n+\tstruct rpc_state *rpc;\n+};\n+\n+/*\n+ * A callback for CURLOPT_WRITEFUNCTION. The return value is the bytes consumed\n+ * from ptr.\n+ */\n static size_t rpc_in(char *ptr, size_t eltsize,\n \t\tsize_t nmemb, void *buffer_)\n {\n \tsize_t size = eltsize * nmemb;\n-\tstruct rpc_state *rpc = buffer_;\n+\tstruct rpc_in_data *data = buffer_;\n \tif (size)\n-\t\trpc->any_written = 1;\n-\twrite_or_die(rpc->in, ptr, size);\n+\t\tdata->rpc->any_written = 1;\n+\twrite_or_die(data->rpc->in, ptr, size);\n \treturn size;\n }\n \n@@ -632,6 +640,7 @@ static int post_rpc(struct rpc_state *rpc)\n \tsize_t gzip_size = 0;\n \tint err, large_request = 0;\n \tint needs_100_continue = 0;\n+\tstruct rpc_in_data rpc_in_data;\n \n \t/* Try to load the entire request, if we can fit it into the\n \t * allocated buffer space we can use HTTP/1.0 and avoid the\n@@ -764,7 +773,8 @@ static int post_rpc(struct rpc_state *rpc)\n \n \tcurl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, headers);\n \tcurl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, rpc_in);\n-\tcurl_easy_setopt(slot->curl, CURLOPT_FILE, rpc);\n+\trpc_in_data.rpc = rpc;\n+\tcurl_easy_setopt(slot->curl, CURLOPT_FILE, &rpc_in_data);\n \n \n \trpc->any_written = 0;\n-- \n2.20.1.97.g81188d93c3-goog\n\n"},{"id":"366314","messageId":"20190108024741.62176-5-masayasuzuki@google.com","threadId":"50122","inReplyTo":"20190108024741.62176-1-masayasuzuki@google.com","subject":"[PATCH v3 4/5] remote-curl: unset CURLOPT_FAILONERROR","fromName":"Masaya Suzuki","fromEmail":"masayasuzuki@google.com","sentAt":"2019-01-08T02:47:40Z","receivedAt":"2019-01-08T02:48:10Z","isPatch":true,"sender":{"key":"masayasuzuki@google.com","avatar":"https://avatars.githubusercontent.com/u/21878?v=4"},"body":"By not setting CURLOPT_FAILONERROR, curl parses the HTTP response\nheaders even if the response is an error. This makes GIT_CURL_VERBOSE to\nshow the HTTP headers, which is useful for debugging.\n\nSigned-off-by: Masaya Suzuki \n---\n remote-curl.c | 10 ++++++++++\n 1 file changed, 10 insertions(+)\n\ndiff --git a/remote-curl.c b/remote-curl.c\nindex d4673b6e8c..91b39ca098 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -547,6 +547,7 @@ static curlioerr rpc_ioctl(CURL *handle, int cmd, void *clientp)\n \n struct rpc_in_data {\n \tstruct rpc_state *rpc;\n+\tstruct active_request_slot *slot;\n };\n \n /*\n@@ -558,6 +559,13 @@ static size_t rpc_in(char *ptr, size_t eltsize,\n {\n \tsize_t size = eltsize * nmemb;\n \tstruct rpc_in_data *data = buffer_;\n+\tlong response_code;\n+\n+\tif (curl_easy_getinfo(data->slot->curl, CURLINFO_RESPONSE_CODE,\n+\t\t\t &response_code) != CURLE_OK)\n+\t\treturn size;\n+\tif (response_code >= 300)\n+\t\treturn size;\n \tif (size)\n \t\tdata->rpc->any_written = 1;\n \twrite_or_die(data->rpc->in, ptr, size);\n@@ -774,7 +782,9 @@ static int post_rpc(struct rpc_state *rpc)\n \tcurl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, headers);\n \tcurl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, rpc_in);\n \trpc_in_data.rpc = rpc;\n+\trpc_in_data.slot = slot;\n \tcurl_easy_setopt(slot->curl, CURLOPT_FILE, &rpc_in_data);\n+\tcurl_easy_setopt(slot->curl, CURLOPT_FAILONERROR, 0);\n \n \n \trpc->any_written = 0;\n-- \n2.20.1.97.g81188d93c3-goog\n\n"},{"id":"366315","messageId":"20190108024741.62176-6-masayasuzuki@google.com","threadId":"50122","inReplyTo":"20190108024741.62176-1-masayasuzuki@google.com","subject":"[PATCH v3 5/5] test: test GIT_CURL_VERBOSE=1 shows an error","fromName":"Masaya Suzuki","fromEmail":"masayasuzuki@google.com","sentAt":"2019-01-08T02:47:41Z","receivedAt":"2019-01-08T02:48:12Z","isPatch":true,"sender":{"key":"masayasuzuki@google.com","avatar":"https://avatars.githubusercontent.com/u/21878?v=4"},"body":"This tests GIT_CURL_VERBOSE shows an error when an URL returns 500. This\nexercises the code in remote_curl.\n\nSigned-off-by: Masaya Suzuki \n---\n t/lib-httpd/apache.conf | 1 +\n t/t5581-http-curl-verbose.sh | 28 ++++++++++++++++++++++++++++\n 2 files changed, 29 insertions(+)\n create mode 100755 t/t5581-http-curl-verbose.sh\n\ndiff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf\nindex 581c010d8f..cc4b87507e 100644\n--- a/t/lib-httpd/apache.conf\n+++ b/t/lib-httpd/apache.conf\n@@ -115,6 +115,7 @@ Alias /auth/dumb/ www/auth/dumb/\n \tSetEnv GIT_EXEC_PATH ${GIT_EXEC_PATH}\n \tSetEnv GIT_HTTP_EXPORT_ALL\n \n+ScriptAliasMatch /error_git_upload_pack/(.*)/git-upload-pack error.sh/\n ScriptAliasMatch /smart_*[^/]*/(.*) ${GIT_EXEC_PATH}/git-http-backend/$1\n ScriptAlias /broken_smart/ broken-smart-http.sh/\n ScriptAlias /error/ error.sh/\ndiff --git a/t/t5581-http-curl-verbose.sh b/t/t5581-http-curl-verbose.sh\nnew file mode 100755\nindex 0000000000..cd9283eeec\n--- /dev/null\n+++ b/t/t5581-http-curl-verbose.sh\n@@ -0,0 +1,28 @@\n+#!/bin/sh\n+\n+test_description='test GIT_CURL_VERBOSE'\n+. ./test-lib.sh\n+. \"$TEST_DIRECTORY\"/lib-httpd.sh\n+start_httpd\n+\n+test_expect_success 'setup repository' '\n+\tmkdir \"$HTTPD_DOCUMENT_ROOT_PATH/repo.git\" &&\n+\tgit -C \"$HTTPD_DOCUMENT_ROOT_PATH/repo.git\" --bare init &&\n+\tgit config push.default matching &&\n+\techo content >file &&\n+\tgit add file &&\n+\tgit commit -m one &&\n+\tgit remote add public \"$HTTPD_DOCUMENT_ROOT_PATH/repo.git\" &&\n+\tgit push public master:master\n+'\n+\n+test_expect_success 'failure in git-upload-pack is shown' '\n+\ttest_might_fail env GIT_CURL_VERBOSE=1 \\\n+\t\tgit clone \"$HTTPD_URL/error_git_upload_pack/smart/repo.git\" \\\n+\t\t2>curl_log &&\n+\tgrep \"< HTTP/1.1 500 Intentional Breakage\" curl_log\n+'\n+\n+stop_httpd\n+\n+test_done\n-- \n2.20.1.97.g81188d93c3-goog\n\n"},{"id":"366409","messageId":"20190109121555.GE4673@szeder.dev","threadId":"50122","inReplyTo":"20190108024741.62176-2-masayasuzuki@google.com","subject":"Re: [PATCH v3 1/5] http: support file handles for HTTP_KEEP_ERROR","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-01-09T12:15:55Z","receivedAt":"2019-01-09T12:16:02Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Mon, Jan 07, 2019 at 06:47:37PM -0800, Masaya Suzuki wrote:\n> HTTP_KEEP_ERROR makes it easy to debug HTTP transport errors. In order\n> to make HTTP_KEEP_ERROR enabled for all requests, file handles need to\n> be supported.\n> \n> Signed-off-by: Masaya Suzuki \n> ---\n> http.c | 9 ++++++---\n> 1 file changed, 6 insertions(+), 3 deletions(-)\n> \n> diff --git a/http.c b/http.c\n> index 0b6807cef9..06450da96e 100644\n> --- a/http.c\n> +++ b/http.c\n> @@ -1991,16 +1991,19 @@ static int http_request_reauth(const char *url,\n> \t/*\n> \t * If we are using KEEP_ERROR, the previous request may have\n> \t * put cruft into our output stream; we should clear it out before\n> -\t * making our next request. We only know how to do this for\n> -\t * the strbuf case, but that is enough to satisfy current callers.\n> +\t * making our next request.\n> \t */\n> \tif (options && options->keep_error) {\n> \t\tswitch (target) {\n> \t\tcase HTTP_REQUEST_STRBUF:\n> \t\t\tstrbuf_reset(result);\n> \t\t\tbreak;\n> +\t\tcase HTTP_REQUEST_FILE:\n> +\t\t\tfflush(result);\n> +\t\t\tftruncate(fileno(result), 0);\n\nSome GCC versions complain about the above line:\n\n http.c: In function ‘http_request_reauth’:\n http.c:1961:3: error: ignoring return value of ‘ftruncate’, declared with attribute warn_unused_result [-Werror=unused-result]\n ftruncate(fileno(result), 0);\n ^~~~~~~~~~~~~~~~~~~~~~~~~~~~\n CC shell.o\n CC remote-testsvn.o\n CC vcs-svn/line_buffer.o\n cc1: all warnings being treated as errors\n make: *** [http.o] Error 1\n make: *** Waiting for unfinished jobs....\n\n> +\t\t\tbreak;\n> \t\tdefault:\n> -\t\t\tBUG(\"HTTP_KEEP_ERROR is only supported with strbufs\");\n> +\t\t\tBUG(\"Unknown http_request target\");\n> \t\t}\n> \t}\n> \n> -- \n> 2.20.1.97.g81188d93c3-goog\n> \n"},{"id":"366521","messageId":"20190110193350.213327-1-masayasuzuki@google.com","threadId":"50122","inReplyTo":"20190108024741.62176-1-masayasuzuki@google.com","subject":"[PATCH v4 0/5] Show HTTP headers of failed requests with GIT_CURL_VERBOSE","fromName":"Masaya Suzuki","fromEmail":"masayasuzuki@google.com","sentAt":"2019-01-10T19:33:45Z","receivedAt":"2019-01-10T19:34:23Z","isPatch":true,"sender":{"key":"masayasuzuki@google.com","avatar":"https://avatars.githubusercontent.com/u/21878?v=4"},"body":"Diff from v3[1]:\n\n* Handle ftruncate and fflush return values\n* Call rewind to set the position back\n\n[1]: https://public-inbox.org/git/20190108024741.62176-1-masayasuzuki@google.com/\n\nMasaya Suzuki (5):\n http: support file handles for HTTP_KEEP_ERROR\n http: enable keep_error for HTTP requests\n remote-curl: define struct for CURLOPT_WRITEFUNCTION\n remote-curl: unset CURLOPT_FAILONERROR\n test: test GIT_CURL_VERBOSE=1 shows an error\n\n http.c | 32 +++++++++++++++++++-------------\n http.h | 1 -\n remote-curl.c | 29 ++++++++++++++++++++++++-----\n t/lib-httpd/apache.conf | 1 +\n t/t5581-http-curl-verbose.sh | 28 ++++++++++++++++++++++++++++\n 5 files changed, 72 insertions(+), 19 deletions(-)\n create mode 100755 t/t5581-http-curl-verbose.sh\n\n-- \n2.20.1.97.g81188d93c3-goog\n\n"},{"id":"366522","messageId":"20190110193350.213327-2-masayasuzuki@google.com","threadId":"50122","inReplyTo":"20190110193350.213327-1-masayasuzuki@google.com","subject":"[PATCH v4 1/5] http: support file handles for HTTP_KEEP_ERROR","fromName":"Masaya Suzuki","fromEmail":"masayasuzuki@google.com","sentAt":"2019-01-10T19:33:46Z","receivedAt":"2019-01-10T19:34:25Z","isPatch":true,"sender":{"key":"masayasuzuki@google.com","avatar":"https://avatars.githubusercontent.com/u/21878?v=4"},"body":"HTTP_KEEP_ERROR makes it easy to debug HTTP transport errors. In order\nto make HTTP_KEEP_ERROR enabled for all requests, file handles need to\nbe supported.\n\nSigned-off-by: Masaya Suzuki \n---\n http.c | 16 +++++++++++++---\n 1 file changed, 13 insertions(+), 3 deletions(-)\n\ndiff --git a/http.c b/http.c\nindex 0b6807cef9..4eccf4c5d8 100644\n--- a/http.c\n+++ b/http.c\n@@ -1991,16 +1991,26 @@ static int http_request_reauth(const char *url,\n \t/*\n \t * If we are using KEEP_ERROR, the previous request may have\n \t * put cruft into our output stream; we should clear it out before\n-\t * making our next request. We only know how to do this for\n-\t * the strbuf case, but that is enough to satisfy current callers.\n+\t * making our next request.\n \t */\n \tif (options && options->keep_error) {\n \t\tswitch (target) {\n \t\tcase HTTP_REQUEST_STRBUF:\n \t\t\tstrbuf_reset(result);\n \t\t\tbreak;\n+\t\tcase HTTP_REQUEST_FILE:\n+\t\t\tif (fflush(result)) {\n+\t\t\t\terror_errno(\"unable to flush a file\");\n+\t\t\t\treturn HTTP_START_FAILED;\n+\t\t\t}\n+\t\t\trewind(result);\n+\t\t\tif (ftruncate(fileno(result), 0) < 0) {\n+\t\t\t\terror_errno(\"unable to truncate a file\");\n+\t\t\t\treturn HTTP_START_FAILED;\n+\t\t\t}\n+\t\t\tbreak;\n \t\tdefault:\n-\t\t\tBUG(\"HTTP_KEEP_ERROR is only supported with strbufs\");\n+\t\t\tBUG(\"Unknown http_request target\");\n \t\t}\n \t}\n \n-- \n2.20.1.97.g81188d93c3-goog\n\n"},{"id":"366523","messageId":"20190110193350.213327-3-masayasuzuki@google.com","threadId":"50122","inReplyTo":"20190110193350.213327-1-masayasuzuki@google.com","subject":"[PATCH v4 2/5] http: enable keep_error for HTTP requests","fromName":"Masaya Suzuki","fromEmail":"masayasuzuki@google.com","sentAt":"2019-01-10T19:33:47Z","receivedAt":"2019-01-10T19:34:29Z","isPatch":true,"sender":{"key":"masayasuzuki@google.com","avatar":"https://avatars.githubusercontent.com/u/21878?v=4"},"body":"curl stops parsing a response when it sees a bad HTTP status code and it\nhas CURLOPT_FAILONERROR set. This prevents GIT_CURL_VERBOSE to show HTTP\nheaders on error.\n\nkeep_error is an option to receive the HTTP response body for those\nerror responses. By enabling this option, curl will process the HTTP\nresponse headers, and they're shown if GIT_CURL_VERBOSE is set.\n\nSigned-off-by: Masaya Suzuki \n---\n http.c | 42 +++++++++++++++++++-----------------------\n http.h | 1 -\n remote-curl.c | 1 -\n 3 files changed, 19 insertions(+), 25 deletions(-)\n\ndiff --git a/http.c b/http.c\nindex 4eccf4c5d8..954bebf684 100644\n--- a/http.c\n+++ b/http.c\n@@ -1876,8 +1876,6 @@ static int http_request(const char *url,\n \tstrbuf_addstr(&buf, \"Pragma:\");\n \tif (options && options->no_cache)\n \t\tstrbuf_addstr(&buf, \" no-cache\");\n-\tif (options && options->keep_error)\n-\t\tcurl_easy_setopt(slot->curl, CURLOPT_FAILONERROR, 0);\n \tif (options && options->initial_request &&\n \t http_follow_config == HTTP_FOLLOW_INITIAL)\n \t\tcurl_easy_setopt(slot->curl, CURLOPT_FOLLOWLOCATION, 1);\n@@ -1895,6 +1893,7 @@ static int http_request(const char *url,\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, \"\");\n+\tcurl_easy_setopt(slot->curl, CURLOPT_FAILONERROR, 0);\n \n \tret = run_one_slot(slot, &results);\n \n@@ -1989,29 +1988,26 @@ static int http_request_reauth(const char *url,\n \t\treturn ret;\n \n \t/*\n-\t * If we are using KEEP_ERROR, the previous request may have\n-\t * put cruft into our output stream; we should clear it out before\n-\t * making our next request.\n+\t * The previous request may have put cruft into our output stream; we\n+\t * should clear it out before making our next request.\n \t */\n-\tif (options && options->keep_error) {\n-\t\tswitch (target) {\n-\t\tcase HTTP_REQUEST_STRBUF:\n-\t\t\tstrbuf_reset(result);\n-\t\t\tbreak;\n-\t\tcase HTTP_REQUEST_FILE:\n-\t\t\tif (fflush(result)) {\n-\t\t\t\terror_errno(\"unable to flush a file\");\n-\t\t\t\treturn HTTP_START_FAILED;\n-\t\t\t}\n-\t\t\trewind(result);\n-\t\t\tif (ftruncate(fileno(result), 0) < 0) {\n-\t\t\t\terror_errno(\"unable to truncate a file\");\n-\t\t\t\treturn HTTP_START_FAILED;\n-\t\t\t}\n-\t\t\tbreak;\n-\t\tdefault:\n-\t\t\tBUG(\"Unknown http_request target\");\n+\tswitch (target) {\n+\tcase HTTP_REQUEST_STRBUF:\n+\t\tstrbuf_reset(result);\n+\t\tbreak;\n+\tcase HTTP_REQUEST_FILE:\n+\t\tif (fflush(result)) {\n+\t\t\terror_errno(\"unable to flush a file\");\n+\t\t\treturn HTTP_START_FAILED;\n \t\t}\n+\t\trewind(result);\n+\t\tif (ftruncate(fileno(result), 0) < 0) {\n+\t\t\terror_errno(\"unable to truncate a file\");\n+\t\t\treturn HTTP_START_FAILED;\n+\t\t}\n+\t\tbreak;\n+\tdefault:\n+\t\tBUG(\"Unknown http_request target\");\n \t}\n \n \tcredential_fill(&http_auth);\ndiff --git a/http.h b/http.h\nindex d305ca1dc7..eebf40688c 100644\n--- a/http.h\n+++ b/http.h\n@@ -146,7 +146,6 @@ extern char *get_remote_object_url(const char *url, const char *hex,\n /* Options for http_get_*() */\n struct http_get_options {\n \tunsigned no_cache:1,\n-\t\t keep_error:1,\n \t\t initial_request:1;\n \n \t/* If non-NULL, returns the content-type of the response. */\ndiff --git a/remote-curl.c b/remote-curl.c\nindex 1220dffcdc..d8eda2380a 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -380,7 +380,6 @@ static struct discovery *discover_refs(const char *service, int for_push)\n \thttp_options.extra_headers = &extra_headers;\n \thttp_options.initial_request = 1;\n \thttp_options.no_cache = 1;\n-\thttp_options.keep_error = 1;\n \n \thttp_ret = http_get_strbuf(refs_url.buf, &buffer, &http_options);\n \tswitch (http_ret) {\n-- \n2.20.1.97.g81188d93c3-goog\n\n"},{"id":"366524","messageId":"20190110193350.213327-4-masayasuzuki@google.com","threadId":"50122","inReplyTo":"20190110193350.213327-1-masayasuzuki@google.com","subject":"[PATCH v4 3/5] remote-curl: define struct for CURLOPT_WRITEFUNCTION","fromName":"Masaya Suzuki","fromEmail":"masayasuzuki@google.com","sentAt":"2019-01-10T19:33:48Z","receivedAt":"2019-01-10T19:34:31Z","isPatch":true,"sender":{"key":"masayasuzuki@google.com","avatar":"https://avatars.githubusercontent.com/u/21878?v=4"},"body":"In order to pass more values for rpc_in, define a struct and pass it as\nan additional value.\n\nSigned-off-by: Masaya Suzuki \n---\n remote-curl.c | 18 ++++++++++++++----\n 1 file changed, 14 insertions(+), 4 deletions(-)\n\ndiff --git a/remote-curl.c b/remote-curl.c\nindex d8eda2380a..d4673b6e8c 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -545,14 +545,22 @@ static curlioerr rpc_ioctl(CURL *handle, int cmd, void *clientp)\n }\n #endif\n \n+struct rpc_in_data {\n+\tstruct rpc_state *rpc;\n+};\n+\n+/*\n+ * A callback for CURLOPT_WRITEFUNCTION. The return value is the bytes consumed\n+ * from ptr.\n+ */\n static size_t rpc_in(char *ptr, size_t eltsize,\n \t\tsize_t nmemb, void *buffer_)\n {\n \tsize_t size = eltsize * nmemb;\n-\tstruct rpc_state *rpc = buffer_;\n+\tstruct rpc_in_data *data = buffer_;\n \tif (size)\n-\t\trpc->any_written = 1;\n-\twrite_or_die(rpc->in, ptr, size);\n+\t\tdata->rpc->any_written = 1;\n+\twrite_or_die(data->rpc->in, ptr, size);\n \treturn size;\n }\n \n@@ -632,6 +640,7 @@ static int post_rpc(struct rpc_state *rpc)\n \tsize_t gzip_size = 0;\n \tint err, large_request = 0;\n \tint needs_100_continue = 0;\n+\tstruct rpc_in_data rpc_in_data;\n \n \t/* Try to load the entire request, if we can fit it into the\n \t * allocated buffer space we can use HTTP/1.0 and avoid the\n@@ -764,7 +773,8 @@ static int post_rpc(struct rpc_state *rpc)\n \n \tcurl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, headers);\n \tcurl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, rpc_in);\n-\tcurl_easy_setopt(slot->curl, CURLOPT_FILE, rpc);\n+\trpc_in_data.rpc = rpc;\n+\tcurl_easy_setopt(slot->curl, CURLOPT_FILE, &rpc_in_data);\n \n \n \trpc->any_written = 0;\n-- \n2.20.1.97.g81188d93c3-goog\n\n"},{"id":"366525","messageId":"20190110193350.213327-5-masayasuzuki@google.com","threadId":"50122","inReplyTo":"20190110193350.213327-1-masayasuzuki@google.com","subject":"[PATCH v4 4/5] remote-curl: unset CURLOPT_FAILONERROR","fromName":"Masaya Suzuki","fromEmail":"masayasuzuki@google.com","sentAt":"2019-01-10T19:33:49Z","receivedAt":"2019-01-10T19:34:34Z","isPatch":true,"sender":{"key":"masayasuzuki@google.com","avatar":"https://avatars.githubusercontent.com/u/21878?v=4"},"body":"By not setting CURLOPT_FAILONERROR, curl parses the HTTP response\nheaders even if the response is an error. This makes GIT_CURL_VERBOSE to\nshow the HTTP headers, which is useful for debugging.\n\nSigned-off-by: Masaya Suzuki \n---\n remote-curl.c | 10 ++++++++++\n 1 file changed, 10 insertions(+)\n\ndiff --git a/remote-curl.c b/remote-curl.c\nindex d4673b6e8c..91b39ca098 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -547,6 +547,7 @@ static curlioerr rpc_ioctl(CURL *handle, int cmd, void *clientp)\n \n struct rpc_in_data {\n \tstruct rpc_state *rpc;\n+\tstruct active_request_slot *slot;\n };\n \n /*\n@@ -558,6 +559,13 @@ static size_t rpc_in(char *ptr, size_t eltsize,\n {\n \tsize_t size = eltsize * nmemb;\n \tstruct rpc_in_data *data = buffer_;\n+\tlong response_code;\n+\n+\tif (curl_easy_getinfo(data->slot->curl, CURLINFO_RESPONSE_CODE,\n+\t\t\t &response_code) != CURLE_OK)\n+\t\treturn size;\n+\tif (response_code >= 300)\n+\t\treturn size;\n \tif (size)\n \t\tdata->rpc->any_written = 1;\n \twrite_or_die(data->rpc->in, ptr, size);\n@@ -774,7 +782,9 @@ static int post_rpc(struct rpc_state *rpc)\n \tcurl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, headers);\n \tcurl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, rpc_in);\n \trpc_in_data.rpc = rpc;\n+\trpc_in_data.slot = slot;\n \tcurl_easy_setopt(slot->curl, CURLOPT_FILE, &rpc_in_data);\n+\tcurl_easy_setopt(slot->curl, CURLOPT_FAILONERROR, 0);\n \n \n \trpc->any_written = 0;\n-- \n2.20.1.97.g81188d93c3-goog\n\n"},{"id":"366526","messageId":"20190110193350.213327-6-masayasuzuki@google.com","threadId":"50122","inReplyTo":"20190110193350.213327-1-masayasuzuki@google.com","subject":"[PATCH v4 5/5] test: test GIT_CURL_VERBOSE=1 shows an error","fromName":"Masaya Suzuki","fromEmail":"masayasuzuki@google.com","sentAt":"2019-01-10T19:33:50Z","receivedAt":"2019-01-10T19:34:35Z","isPatch":true,"sender":{"key":"masayasuzuki@google.com","avatar":"https://avatars.githubusercontent.com/u/21878?v=4"},"body":"This tests GIT_CURL_VERBOSE shows an error when an URL returns 500. This\nexercises the code in remote_curl.\n\nSigned-off-by: Masaya Suzuki \n---\n t/lib-httpd/apache.conf | 1 +\n t/t5581-http-curl-verbose.sh | 28 ++++++++++++++++++++++++++++\n 2 files changed, 29 insertions(+)\n create mode 100755 t/t5581-http-curl-verbose.sh\n\ndiff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf\nindex 581c010d8f..cc4b87507e 100644\n--- a/t/lib-httpd/apache.conf\n+++ b/t/lib-httpd/apache.conf\n@@ -115,6 +115,7 @@ Alias /auth/dumb/ www/auth/dumb/\n \tSetEnv GIT_EXEC_PATH ${GIT_EXEC_PATH}\n \tSetEnv GIT_HTTP_EXPORT_ALL\n \n+ScriptAliasMatch /error_git_upload_pack/(.*)/git-upload-pack error.sh/\n ScriptAliasMatch /smart_*[^/]*/(.*) ${GIT_EXEC_PATH}/git-http-backend/$1\n ScriptAlias /broken_smart/ broken-smart-http.sh/\n ScriptAlias /error/ error.sh/\ndiff --git a/t/t5581-http-curl-verbose.sh b/t/t5581-http-curl-verbose.sh\nnew file mode 100755\nindex 0000000000..cd9283eeec\n--- /dev/null\n+++ b/t/t5581-http-curl-verbose.sh\n@@ -0,0 +1,28 @@\n+#!/bin/sh\n+\n+test_description='test GIT_CURL_VERBOSE'\n+. ./test-lib.sh\n+. \"$TEST_DIRECTORY\"/lib-httpd.sh\n+start_httpd\n+\n+test_expect_success 'setup repository' '\n+\tmkdir \"$HTTPD_DOCUMENT_ROOT_PATH/repo.git\" &&\n+\tgit -C \"$HTTPD_DOCUMENT_ROOT_PATH/repo.git\" --bare init &&\n+\tgit config push.default matching &&\n+\techo content >file &&\n+\tgit add file &&\n+\tgit commit -m one &&\n+\tgit remote add public \"$HTTPD_DOCUMENT_ROOT_PATH/repo.git\" &&\n+\tgit push public master:master\n+'\n+\n+test_expect_success 'failure in git-upload-pack is shown' '\n+\ttest_might_fail env GIT_CURL_VERBOSE=1 \\\n+\t\tgit clone \"$HTTPD_URL/error_git_upload_pack/smart/repo.git\" \\\n+\t\t2>curl_log &&\n+\tgrep \"< HTTP/1.1 500 Intentional Breakage\" curl_log\n+'\n+\n+stop_httpd\n+\n+test_done\n-- \n2.20.1.97.g81188d93c3-goog\n\n"},{"id":"366544","messageId":"xmqqpnt4yuwu.fsf@gitster-ct.c.googlers.com","threadId":"50122","inReplyTo":"20190110193350.213327-1-masayasuzuki@google.com","subject":"Re: [PATCH v4 0/5] Show HTTP headers of failed requests with GIT_CURL_VERBOSE","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-01-10T23:06:41Z","receivedAt":"2019-01-10T23:06:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Masaya Suzuki writes:\n\n> Diff from v3[1]:\n>\n> * Handle ftruncate and fflush return values\n> * Call rewind to set the position back\n>\n> [1]: https://public-inbox.org/git/20190108024741.62176-1-masayasuzuki@google.com/\n\nThanks. Adding the error checking even to 1/5 and moving the\nupdated code in 2/5 makes the range-diff a bit noisy, but the\nresulting series makes lot of sense.\n\nWill queue.\n\n>\n> Masaya Suzuki (5):\n> http: support file handles for HTTP_KEEP_ERROR\n> http: enable keep_error for HTTP requests\n> remote-curl: define struct for CURLOPT_WRITEFUNCTION\n> remote-curl: unset CURLOPT_FAILONERROR\n> test: test GIT_CURL_VERBOSE=1 shows an error\n>\n> http.c | 32 +++++++++++++++++++-------------\n> http.h | 1 -\n> remote-curl.c | 29 ++++++++++++++++++++++++-----\n> t/lib-httpd/apache.conf | 1 +\n> t/t5581-http-curl-verbose.sh | 28 ++++++++++++++++++++++++++++\n> 5 files changed, 72 insertions(+), 19 deletions(-)\n> create mode 100755 t/t5581-http-curl-verbose.sh\n"}]}