| Message ID | 20181229194447.157763-3-masayasuzuki@google.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | Show HTTP headers of failed requests with GIT_CURL_VERBOSE | expand |
On Sat, Dec 29, 2018 at 11:44:47AM -0800, Masaya Suzuki wrote: > When GIT_CURL_VERBOSE is set, libcurl produces request/response headers > to stderr. However, if the response is an error response and > CURLOPT_FAILONERROR is set, libcurl stops parsing the response, and it > won't dump the headers. Showing HTTP response headers is useful for > debugging, especially for non-OK responses. Out of curiosity, does GIT_TRACE_CURL do any better? Or is it simply that curl closes the handle when it sees the bad response code, and nobody ever gets to see the rest of the data? > This is substantially same as setting http_options.keep_error to all > requests. Hence, remove this option. The assumption here is that every code path using FAILONERROR is prepared to handle the failing http response codes itself (since we no longer set it at all in get_active_slot()). Is that so? Anything that uses handle_curl_result() is OK. That means run_one_slot() is fine, which in turn covers run_slot() for RPCs, and http_request() for normal one-at-a-time requests. But what about the parallel multiple requests issued by the dumb-http walker code? There I think we end up in step_active_slots(), which calls into finish_active_slot() for completed requests. I think that unconditionally fetches the http code without bothering to look at whether curl reported success or not. So I _think_ that's probably all of the users of the curl handles provided by get_active_slot(). Though given the tangled mess of our HTTP code, I won't be surprised if there's a corner case I missed in that analysis. -Peff
On Fri, Jan 4, 2019 at 2:49 AM Jeff King <peff@peff.net> wrote: > > On Sat, Dec 29, 2018 at 11:44:47AM -0800, Masaya Suzuki wrote: > > > When GIT_CURL_VERBOSE is set, libcurl produces request/response headers > > to stderr. However, if the response is an error response and > > CURLOPT_FAILONERROR is set, libcurl stops parsing the response, and it > > won't dump the headers. Showing HTTP response headers is useful for > > debugging, especially for non-OK responses. > > Out of curiosity, does GIT_TRACE_CURL do any better? Or is it simply > that curl closes the handle when it sees the bad response code, and > nobody ever gets to see the rest of the data? curl disregards the rest of the contents when it sees a bad response code when CURLOPT_FAILONERROR is set (https://github.com/curl/curl/blob/dea3f94298ac0859464768959488938c4e104545/lib/http.c#L3691). So nobody gets the rest of the data. > > > This is substantially same as setting http_options.keep_error to all > > requests. Hence, remove this option. > > The assumption here is that every code path using FAILONERROR is > prepared to handle the failing http response codes itself (since we no > longer set it at all in get_active_slot()). Is that so? I was thinking that I covered the all code paths using FAILONERROR, but it seems it's not the case. http-walker.c and http-push.c also calls get_active_slot(). I'll narrow down the scope on removing FAILONERROR. > > Anything that uses handle_curl_result() is OK. That means run_one_slot() > is fine, which in turn covers run_slot() for RPCs, and http_request() > for normal one-at-a-time requests. But what about the parallel multiple > requests issued by the dumb-http walker code? Right. I'll keep FAILONERROR in get_active_slot and remove it only for the code paths that can handle HTTP errors. > > There I think we end up in step_active_slots(), which calls into > finish_active_slot() for completed requests. I think that > unconditionally fetches the http code without bothering to look at > whether curl reported success or not. > > So I _think_ that's probably all of the users of the curl handles > provided by get_active_slot(). Though given the tangled mess of our HTTP > code, I won't be surprised if there's a corner case I missed in that > analysis. > > -Peff
diff --git a/http.c b/http.c index d23417670..8f8101da3 100644 --- a/http.c +++ b/http.c @@ -1269,7 +1269,6 @@ struct active_request_slot *get_active_slot(void) curl_easy_setopt(slot->curl, CURLOPT_POSTFIELDS, NULL); curl_easy_setopt(slot->curl, CURLOPT_UPLOAD, 0); curl_easy_setopt(slot->curl, CURLOPT_HTTPGET, 1); - curl_easy_setopt(slot->curl, CURLOPT_FAILONERROR, 1); curl_easy_setopt(slot->curl, CURLOPT_RANGE, NULL); /* @@ -1848,8 +1847,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); @@ -2415,7 +2412,6 @@ struct http_object_request *new_http_object_request(const char *base_url, freq->slot = get_active_slot(); curl_easy_setopt(freq->slot->curl, CURLOPT_FILE, freq); - curl_easy_setopt(freq->slot->curl, CURLOPT_FAILONERROR, 0); curl_easy_setopt(freq->slot->curl, CURLOPT_WRITEFUNCTION, fwrite_sha1_file); curl_easy_setopt(freq->slot->curl, CURLOPT_ERRORBUFFER, freq->errorstr); curl_easy_setopt(freq->slot->curl, CURLOPT_URL, freq->url); 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 48656bf18..43e7a1d80 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) { 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 </LocationMatch> +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..73148eeba --- /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 && + cat curl_log | grep "< HTTP/1.1 500 Intentional Breakage" +' + +stop_httpd + +test_done
When GIT_CURL_VERBOSE is set, libcurl produces request/response headers to stderr. However, if the response is an error response and CURLOPT_FAILONERROR is set, libcurl stops parsing the response, and it won't dump the headers. Showing HTTP response headers is useful for debugging, especially for non-OK responses. This is substantially same as setting http_options.keep_error to all requests. Hence, remove this option. Signed-off-by: Masaya Suzuki <masayasuzuki@google.com> --- http.c | 4 ---- http.h | 1 - remote-curl.c | 1 - t/lib-httpd/apache.conf | 1 + t/t5581-http-curl-verbose.sh | 28 ++++++++++++++++++++++++++++ 5 files changed, 29 insertions(+), 6 deletions(-) create mode 100755 t/t5581-http-curl-verbose.sh