From 8dd2e88a923bd24775182f0a507c993e06a0aacb Mon Sep 17 00:00:00 2001 From: Masaya Suzuki Date: Thu, 10 Jan 2019 11:33:46 -0800 Subject: http: support file handles for HTTP_KEEP_ERROR HTTP_KEEP_ERROR makes it easy to debug HTTP transport errors. In order to make HTTP_KEEP_ERROR enabled for all requests, file handles need to be supported. Signed-off-by: Masaya Suzuki Signed-off-by: Junio C Hamano --- http.c | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/http.c b/http.c index eacc2a75e..ab4279a1c 100644 --- a/http.c +++ b/http.c @@ -1952,16 +1952,26 @@ static int http_request_reauth(const char *url, /* * If we are using KEEP_ERROR, the previous request may have * put cruft into our output stream; we should clear it out before - * making our next request. We only know how to do this for - * the strbuf case, but that is enough to satisfy current callers. + * making our next request. */ if (options && options->keep_error) { switch (target) { case HTTP_REQUEST_STRBUF: strbuf_reset(result); break; + case HTTP_REQUEST_FILE: + if (fflush(result)) { + error_errno("unable to flush a file"); + return HTTP_START_FAILED; + } + rewind(result); + if (ftruncate(fileno(result), 0) < 0) { + error_errno("unable to truncate a file"); + return HTTP_START_FAILED; + } + break; default: - BUG("HTTP_KEEP_ERROR is only supported with strbufs"); + BUG("Unknown http_request target"); } } -- cgit v1.2.3 From e6cf87b12d3b85b31637c865bbfaed62c3e59e94 Mon Sep 17 00:00:00 2001 From: Masaya Suzuki Date: Thu, 10 Jan 2019 11:33:47 -0800 Subject: http: enable keep_error for HTTP requests curl stops parsing a response when it sees a bad HTTP status code and it has CURLOPT_FAILONERROR set. This prevents GIT_CURL_VERBOSE to show HTTP headers on error. keep_error is an option to receive the HTTP response body for those error responses. By enabling this option, curl will process the HTTP response headers, and they're shown if GIT_CURL_VERBOSE is set. Signed-off-by: Masaya Suzuki Signed-off-by: Junio C Hamano --- http.c | 42 +++++++++++++++++++----------------------- http.h | 1 - remote-curl.c | 1 - 3 files changed, 19 insertions(+), 25 deletions(-) diff --git a/http.c b/http.c index ab4279a1c..b39921636 100644 --- a/http.c +++ b/http.c @@ -1837,8 +1837,6 @@ static int http_request(const char *url, strbuf_addstr(&buf, "Pragma:"); if (options && options->no_cache) strbuf_addstr(&buf, " no-cache"); - if (options && options->keep_error) - curl_easy_setopt(slot->curl, CURLOPT_FAILONERROR, 0); if (options && options->initial_request && http_follow_config == HTTP_FOLLOW_INITIAL) curl_easy_setopt(slot->curl, CURLOPT_FOLLOWLOCATION, 1); @@ -1856,6 +1854,7 @@ static int http_request(const char *url, curl_easy_setopt(slot->curl, CURLOPT_URL, url); curl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, headers); curl_easy_setopt(slot->curl, CURLOPT_ENCODING, ""); + curl_easy_setopt(slot->curl, CURLOPT_FAILONERROR, 0); ret = run_one_slot(slot, &results); @@ -1950,29 +1949,26 @@ static int http_request_reauth(const char *url, return ret; /* - * If we are using KEEP_ERROR, the previous request may have - * put cruft into our output stream; we should clear it out before - * making our next request. + * The previous request may have put cruft into our output stream; we + * should clear it out before making our next request. */ - if (options && options->keep_error) { - switch (target) { - case HTTP_REQUEST_STRBUF: - strbuf_reset(result); - break; - case HTTP_REQUEST_FILE: - if (fflush(result)) { - error_errno("unable to flush a file"); - return HTTP_START_FAILED; - } - rewind(result); - if (ftruncate(fileno(result), 0) < 0) { - error_errno("unable to truncate a file"); - return HTTP_START_FAILED; - } - break; - default: - BUG("Unknown http_request target"); + switch (target) { + case HTTP_REQUEST_STRBUF: + strbuf_reset(result); + break; + case HTTP_REQUEST_FILE: + if (fflush(result)) { + error_errno("unable to flush a file"); + return HTTP_START_FAILED; + } + rewind(result); + if (ftruncate(fileno(result), 0) < 0) { + error_errno("unable to truncate a file"); + return HTTP_START_FAILED; } + break; + default: + BUG("Unknown http_request target"); } credential_fill(&http_auth); diff --git a/http.h b/http.h index d305ca1dc..eebf40688 100644 --- a/http.h +++ b/http.h @@ -146,7 +146,6 @@ extern char *get_remote_object_url(const char *url, const char *hex, /* Options for http_get_*() */ struct http_get_options { unsigned no_cache:1, - keep_error:1, initial_request:1; /* If non-NULL, returns the content-type of the response. */ diff --git a/remote-curl.c b/remote-curl.c index 1220dffcd..d8eda2380 100644 --- a/remote-curl.c +++ b/remote-curl.c @@ -380,7 +380,6 @@ static struct discovery *discover_refs(const char *service, int for_push) http_options.extra_headers = &extra_headers; http_options.initial_request = 1; http_options.no_cache = 1; - http_options.keep_error = 1; http_ret = http_get_strbuf(refs_url.buf, &buffer, &http_options); switch (http_ret) { -- cgit v1.2.3 From cf2fb92b00df0594aac2176b524db45b62795a38 Mon Sep 17 00:00:00 2001 From: Masaya Suzuki Date: Thu, 10 Jan 2019 11:33:48 -0800 Subject: remote-curl: define struct for CURLOPT_WRITEFUNCTION In order to pass more values for rpc_in, define a struct and pass it as an additional value. Signed-off-by: Masaya Suzuki Signed-off-by: Junio C Hamano --- remote-curl.c | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/remote-curl.c b/remote-curl.c index d8eda2380..d4673b6e8 100644 --- a/remote-curl.c +++ b/remote-curl.c @@ -545,14 +545,22 @@ static curlioerr rpc_ioctl(CURL *handle, int cmd, void *clientp) } #endif +struct rpc_in_data { + struct rpc_state *rpc; +}; + +/* + * A callback for CURLOPT_WRITEFUNCTION. The return value is the bytes consumed + * from ptr. + */ static size_t rpc_in(char *ptr, size_t eltsize, size_t nmemb, void *buffer_) { size_t size = eltsize * nmemb; - struct rpc_state *rpc = buffer_; + struct rpc_in_data *data = buffer_; if (size) - rpc->any_written = 1; - write_or_die(rpc->in, ptr, size); + data->rpc->any_written = 1; + write_or_die(data->rpc->in, ptr, size); return size; } @@ -632,6 +640,7 @@ static int post_rpc(struct rpc_state *rpc) size_t gzip_size = 0; int err, large_request = 0; int needs_100_continue = 0; + struct rpc_in_data rpc_in_data; /* Try to load the entire request, if we can fit it into the * allocated buffer space we can use HTTP/1.0 and avoid the @@ -764,7 +773,8 @@ retry: curl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, headers); curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, rpc_in); - curl_easy_setopt(slot->curl, CURLOPT_FILE, rpc); + rpc_in_data.rpc = rpc; + curl_easy_setopt(slot->curl, CURLOPT_FILE, &rpc_in_data); rpc->any_written = 0; -- cgit v1.2.3 From b79bdd8c1208103f106e9cf7a535b625521b21c9 Mon Sep 17 00:00:00 2001 From: Masaya Suzuki Date: Thu, 10 Jan 2019 11:33:49 -0800 Subject: remote-curl: unset CURLOPT_FAILONERROR By not setting CURLOPT_FAILONERROR, curl parses the HTTP response headers even if the response is an error. This makes GIT_CURL_VERBOSE to show the HTTP headers, which is useful for debugging. Signed-off-by: Masaya Suzuki Signed-off-by: Junio C Hamano --- remote-curl.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/remote-curl.c b/remote-curl.c index d4673b6e8..91b39ca09 100644 --- a/remote-curl.c +++ b/remote-curl.c @@ -547,6 +547,7 @@ static curlioerr rpc_ioctl(CURL *handle, int cmd, void *clientp) struct rpc_in_data { struct rpc_state *rpc; + struct active_request_slot *slot; }; /* @@ -558,6 +559,13 @@ static size_t rpc_in(char *ptr, size_t eltsize, { size_t size = eltsize * nmemb; struct rpc_in_data *data = buffer_; + long response_code; + + if (curl_easy_getinfo(data->slot->curl, CURLINFO_RESPONSE_CODE, + &response_code) != CURLE_OK) + return size; + if (response_code >= 300) + return size; if (size) data->rpc->any_written = 1; write_or_die(data->rpc->in, ptr, size); @@ -774,7 +782,9 @@ retry: curl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, headers); curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, rpc_in); rpc_in_data.rpc = rpc; + rpc_in_data.slot = slot; curl_easy_setopt(slot->curl, CURLOPT_FILE, &rpc_in_data); + curl_easy_setopt(slot->curl, CURLOPT_FAILONERROR, 0); rpc->any_written = 0; -- cgit v1.2.3 From e4871cd50c87610469a08b7fa8ef46677d2b8b7b Mon Sep 17 00:00:00 2001 From: Masaya Suzuki Date: Thu, 10 Jan 2019 11:33:50 -0800 Subject: test: test GIT_CURL_VERBOSE=1 shows an error This tests GIT_CURL_VERBOSE shows an error when an URL returns 500. This exercises the code in remote_curl. Signed-off-by: Masaya Suzuki Signed-off-by: Junio C Hamano --- t/lib-httpd/apache.conf | 1 + t/t5581-http-curl-verbose.sh | 28 ++++++++++++++++++++++++++++ 2 files changed, 29 insertions(+) create mode 100755 t/t5581-http-curl-verbose.sh diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf index 581c010d8..cc4b87507 100644 --- a/t/lib-httpd/apache.conf +++ b/t/lib-httpd/apache.conf @@ -115,6 +115,7 @@ Alias /auth/dumb/ www/auth/dumb/ SetEnv GIT_EXEC_PATH ${GIT_EXEC_PATH} SetEnv GIT_HTTP_EXPORT_ALL +ScriptAliasMatch /error_git_upload_pack/(.*)/git-upload-pack error.sh/ ScriptAliasMatch /smart_*[^/]*/(.*) ${GIT_EXEC_PATH}/git-http-backend/$1 ScriptAlias /broken_smart/ broken-smart-http.sh/ ScriptAlias /error/ error.sh/ diff --git a/t/t5581-http-curl-verbose.sh b/t/t5581-http-curl-verbose.sh new file mode 100755 index 000000000..cd9283eee --- /dev/null +++ b/t/t5581-http-curl-verbose.sh @@ -0,0 +1,28 @@ +#!/bin/sh + +test_description='test GIT_CURL_VERBOSE' +. ./test-lib.sh +. "$TEST_DIRECTORY"/lib-httpd.sh +start_httpd + +test_expect_success 'setup repository' ' + mkdir "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" && + git -C "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" --bare init && + git config push.default matching && + echo content >file && + git add file && + git commit -m one && + git remote add public "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" && + git push public master:master +' + +test_expect_success 'failure in git-upload-pack is shown' ' + test_might_fail env GIT_CURL_VERBOSE=1 \ + git clone "$HTTPD_URL/error_git_upload_pack/smart/repo.git" \ + 2>curl_log && + grep "< HTTP/1.1 500 Intentional Breakage" curl_log +' + +stop_httpd + +test_done -- cgit v1.2.3