From 6d052d78d74e581dd93dd6328d3c214f469e34d7 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 5 Apr 2013 18:14:06 -0400 Subject: [PATCH 1/9] http: add HTTP_KEEP_ERROR option We currently set curl's FAILONERROR option, which means that any http failures are reported as curl errors, and the http body content from the server is thrown away. This patch introduces a new option to http_get_strbuf which specifies that the body content from a failed http response should be placed in the destination strbuf, where it can be accessed by the caller. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- http.c | 37 +++++++++++++++++++++++++++++++++++++ http.h | 1 + 2 files changed, 38 insertions(+) diff --git a/http.c b/http.c index 8803c70825..45cc7c7010 100644 --- a/http.c +++ b/http.c @@ -761,6 +761,25 @@ char *get_remote_object_url(const char *url, const char *hex, int handle_curl_result(struct slot_results *results) { + /* + * If we see a failing http code with CURLE_OK, we have turned off + * FAILONERROR (to keep the server's custom error response), and should + * translate the code into failure here. + */ + if (results->curl_result == CURLE_OK && + results->http_code >= 400) { + results->curl_result = CURLE_HTTP_RETURNED_ERROR; + /* + * Normally curl will already have put the "reason phrase" + * from the server into curl_errorstr; unfortunately without + * FAILONERROR it is lost, so we can give only the numeric + * status code. + */ + snprintf(curl_errorstr, sizeof(curl_errorstr), + "The requested URL returned error: %ld", + results->http_code); + } + if (results->curl_result == CURLE_OK) { credential_approve(&http_auth); return HTTP_OK; @@ -825,6 +844,8 @@ static int http_request(const char *url, struct strbuf *type, strbuf_addstr(&buf, "Pragma:"); if (options & HTTP_NO_CACHE) strbuf_addstr(&buf, " no-cache"); + if (options & HTTP_KEEP_ERROR) + curl_easy_setopt(slot->curl, CURLOPT_FAILONERROR, 0); headers = curl_slist_append(headers, buf.buf); @@ -862,6 +883,22 @@ static int http_request_reauth(const char *url, int ret = http_request(url, type, result, target, options); if (ret != HTTP_REAUTH) 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. We only know how to do this for + * the strbuf case, but that is enough to satisfy current callers. + */ + if (options & HTTP_KEEP_ERROR) { + switch (target) { + case HTTP_REQUEST_STRBUF: + strbuf_reset(result); + break; + default: + die("BUG: HTTP_KEEP_ERROR is only supported with strbufs"); + } + } return http_request(url, type, result, target, options); } diff --git a/http.h b/http.h index 25d1931398..0fe54f4131 100644 --- a/http.h +++ b/http.h @@ -118,6 +118,7 @@ extern char *get_remote_object_url(const char *url, const char *hex, /* Options for http_request_*() */ #define HTTP_NO_CACHE 1 +#define HTTP_KEEP_ERROR 2 /* Return values for http_request_*() */ #define HTTP_OK 0 From 426e70d4a11ce3b4f70636d57c6a0ab16ae08a00 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 5 Apr 2013 18:17:23 -0400 Subject: [PATCH 2/9] remote-curl: show server content on http errors If an http request to a remote git server fails, we show only the http response code, or sometimes a custom message for particular codes. This gives the server no opportunity to offer a more detailed explanation of the reason for the failure, or to give extra advice. This patch teaches remote-curl to record and display the body content of a failed http response. We only display such responses when the content-type is advertised as text/plain, as it is the most likely to look presentable on the user's terminal (and it is hoped to be a good indication that the message is intended for git clients, and not for a web browser). Each line of the new output is prepended with "remote:". Example output may look like this (assuming the server is configured to display such a helpful message): $ GIT_SMART_HTTP=0 git clone https://example.com/some/repo.git Cloning into 'repo'... remote: Sorry, fetching via dumb http is forbidden. remote: Please upgrade your git client to v1.6.6 or greater remote: and make sure that smart-http is enabled. error: The requested URL returned error: 403 while accessing http://localhost:5001/some/repo.git/info/refs fatal: HTTP request failed For the sake of simplicity, we only record and display these errors during the initial fetch of the ref list, as that is the initial contact with the server and where the most common, interesting errors happen (and there is already precedent, as that is the only place we currently massage http error codes into more helpful messages). Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- remote-curl.c | 33 ++++++++++++++++++++++++++++++++- 1 file changed, 32 insertions(+), 1 deletion(-) diff --git a/remote-curl.c b/remote-curl.c index 93a09a64c3..4fea94f138 100644 --- a/remote-curl.c +++ b/remote-curl.c @@ -151,6 +151,33 @@ static void free_discovery(struct discovery *d) } } +static int show_http_message(struct strbuf *type, struct strbuf *msg) +{ + const char *p, *eol; + + /* + * We only show text/plain parts, as other types are likely + * to be ugly to look at on the user's terminal. + * + * TODO should handle "; charset=XXX", and re-encode into + * logoutputencoding + */ + if (strcasecmp(type->buf, "text/plain")) + return -1; + + strbuf_trim(msg); + if (!msg->len) + return -1; + + p = msg->buf; + do { + eol = strchrnul(p, '\n'); + fprintf(stderr, "remote: %.*s\n", (int)(eol - p), p); + p = eol + 1; + } while(*eol); + return 0; +} + static struct discovery* discover_refs(const char *service, int for_push) { struct strbuf exp = STRBUF_INIT; @@ -176,16 +203,20 @@ static struct discovery* discover_refs(const char *service, int for_push) } refs_url = strbuf_detach(&buffer, NULL); - http_ret = http_get_strbuf(refs_url, &type, &buffer, HTTP_NO_CACHE); + http_ret = http_get_strbuf(refs_url, &type, &buffer, + HTTP_NO_CACHE | HTTP_KEEP_ERROR); switch (http_ret) { case HTTP_OK: break; case HTTP_MISSING_TARGET: + show_http_message(&type, &buffer); die("%s not found: did you run git update-server-info on the" " server?", refs_url); case HTTP_NOAUTH: + show_http_message(&type, &buffer); die("Authentication failed"); default: + show_http_message(&type, &buffer); http_error(refs_url, http_ret); die("HTTP request failed"); } From 110bcdc3d0ea9f0042f24a4c0319dede36dfe7f9 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 5 Apr 2013 18:17:40 -0400 Subject: [PATCH 3/9] remote-curl: let servers override http 404 advice When we get an http 404 trying to get the initial list of refs from the server, we try to be helpful and remind the user that update-server-info may need to be run. This looks like: $ git clone https://github.com/non/existent Cloning into 'existent'... fatal: https://github.com/non/existent/info/refs?service=git-upload-pack not found: did you run git update-server-info on the server? Suggesting update-server-info may be a good suggestion for users who are in control of the server repo and who are planning to set up dumb http. But for users of smart http, and especially users who are not in control of the server repo, the advice is useless and confusing. The previous patch taught remote-curl to show custom advice from the server when it is available. When we have shown messages from the server, we can also drop our custom advice; what the server has to say is likely to be more accurate and helpful. We not only drop the mention of update-server-info, but also show only the main repo URL, not the full "info/refs" and service parameter. These elements may be useful for debugging a broken server configuration, but again, anything the server has provided is likely to be more useful (and one can still use GIT_CURL_VERBOSE to get much more complete debugging information). Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- remote-curl.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/remote-curl.c b/remote-curl.c index 4fea94f138..083efdf776 100644 --- a/remote-curl.c +++ b/remote-curl.c @@ -209,7 +209,8 @@ static struct discovery* discover_refs(const char *service, int for_push) case HTTP_OK: break; case HTTP_MISSING_TARGET: - show_http_message(&type, &buffer); + if (!show_http_message(&type, &buffer)) + die("repository '%s' not found", url); die("%s not found: did you run git update-server-info on the" " server?", refs_url); case HTTP_NOAUTH: From cfa0f4040dd1885fbcdd3d306c1defe22d0fee00 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 5 Apr 2013 18:20:43 -0400 Subject: [PATCH 4/9] remote-curl: always show friendlier 404 message When we get an http 404 trying to get the initial list of refs from the server, we try to be helpful and remind the user that update-server-info may need to be run. This looks like: $ git clone https://github.com/non/existent Cloning into 'existent'... fatal: https://github.com/non/existent/info/refs?service=git-upload-pack not found: did you run git update-server-info on the server? Suggesting update-server-info may be a good suggestion for users who are in control of the server repo and who are planning to set up dumb http. But for users of smart http, and especially users who are not in control of the server repo, the advice is useless and confusing. Since most people are expected to use smart http these days, it does not make sense to keep the update-server-info hint. We not only drop the mention of update-server-info, but also show only the main repo URL, not the full "info/refs" and service parameter. These elements may be useful for debugging a broken server configuration, but in the majority of cases, users are not fetching from their own repositories, but rather from other people's repositories; they have neither the power nor interest to fix a broken configuration, and the extra components just make the message more confusing. Users who do want to debug can and should use GIT_CURL_VERBOSE to get more complete information on the actual URLs visited. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- remote-curl.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/remote-curl.c b/remote-curl.c index 083efdf776..5d9f9618b4 100644 --- a/remote-curl.c +++ b/remote-curl.c @@ -209,10 +209,8 @@ static struct discovery* discover_refs(const char *service, int for_push) case HTTP_OK: break; case HTTP_MISSING_TARGET: - if (!show_http_message(&type, &buffer)) - die("repository '%s' not found", url); - die("%s not found: did you run git update-server-info on the" - " server?", refs_url); + show_http_message(&type, &buffer); + die("repository '%s' not found", url); case HTTP_NOAUTH: show_http_message(&type, &buffer); die("Authentication failed"); From d5ccbe4dfb43d95d1d04252490fcd200e6aa5759 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 5 Apr 2013 18:21:14 -0400 Subject: [PATCH 5/9] remote-curl: consistently report repo url for http errors When we report http errors in fetching the initial ref advertisement, we show the full URL we attempted to use, including "info/refs?service=git-upload-pack". While this may be useful for debugging a broken server, it is unnecessarily verbose and confusing for most cases, in which the client user is not even the same person as the owner of the repository. Let's just show the repository URL; debugging can happen with GIT_CURL_VERBOSE, which shows way more useful information, anyway. At the same time, let's also make sure to mention the repository URL when we report failed authentication (previously we said only "Authentication failed"). Knowing the URL can help the user realize why authentication failed (e.g., they meant to push to remote A, not remote B). Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- remote-curl.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/remote-curl.c b/remote-curl.c index 5d9f9618b4..6c6714b00a 100644 --- a/remote-curl.c +++ b/remote-curl.c @@ -213,10 +213,10 @@ static struct discovery* discover_refs(const char *service, int for_push) die("repository '%s' not found", url); case HTTP_NOAUTH: show_http_message(&type, &buffer); - die("Authentication failed"); + die("Authentication failed for '%s'", url); default: show_http_message(&type, &buffer); - http_error(refs_url, http_ret); + http_error(url, http_ret); die("HTTP request failed"); } From 67d2a7b5c502496b0c5acd25a5e0ffa766b61745 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 5 Apr 2013 18:21:34 -0400 Subject: [PATCH 6/9] http: simplify http_error helper function This helper function should really be a one-liner that prints an error message, but it has ended up unnecessarily complicated: 1. We call error() directly when we fail to start the curl request, so we must later avoid printing a duplicate error in http_error(). It would be much simpler in this case to just stuff the error message into our usual curl_errorstr buffer rather than printing it ourselves. This means that http_error does not even have to care about curl's exit value (the interesting part is in the errorstr buffer already). 2. We return the "ret" value passed in to us, but none of the callers actually cares about our return value. We can just drop this entirely. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- http-push.c | 2 +- http.c | 11 ++++------- http.h | 5 ++--- remote-curl.c | 2 +- 4 files changed, 8 insertions(+), 12 deletions(-) diff --git a/http-push.c b/http-push.c index bd66f6ab6e..439a555a72 100644 --- a/http-push.c +++ b/http-push.c @@ -1551,7 +1551,7 @@ static int remote_exists(const char *path) ret = 0; break; case HTTP_ERROR: - http_error(url, HTTP_ERROR); + http_error(url); default: ret = -1; } diff --git a/http.c b/http.c index 45cc7c7010..5e6f67d004 100644 --- a/http.c +++ b/http.c @@ -857,7 +857,8 @@ static int http_request(const char *url, struct strbuf *type, run_active_slot(slot); ret = handle_curl_result(&results); } else { - error("Unable to start HTTP request for %s", url); + snprintf(curl_errorstr, sizeof(curl_errorstr), + "failed to start HTTP request"); ret = HTTP_START_FAILED; } @@ -940,13 +941,9 @@ cleanup: return ret; } -int http_error(const char *url, int ret) +void http_error(const char *url) { - /* http_request has already handled HTTP_START_FAILED. */ - if (ret != HTTP_START_FAILED) - error("%s while accessing %s", curl_errorstr, url); - - return ret; + error("%s while accessing %s", curl_errorstr, url); } int http_fetch_ref(const char *base, struct ref *ref) diff --git a/http.h b/http.h index 0fe54f4131..fa65128f9e 100644 --- a/http.h +++ b/http.h @@ -136,10 +136,9 @@ extern char *get_remote_object_url(const char *url, const char *hex, int http_get_strbuf(const char *url, struct strbuf *content_type, struct strbuf *result, int options); /* - * Prints an error message using error() containing url and curl_errorstr, - * and returns ret. + * Prints an error message using error() containing url and curl_errorstr. */ -int http_error(const char *url, int ret); +void http_error(const char *url); extern int http_fetch_ref(const char *base, struct ref *ref); diff --git a/remote-curl.c b/remote-curl.c index 6c6714b00a..9abe4b7116 100644 --- a/remote-curl.c +++ b/remote-curl.c @@ -216,7 +216,7 @@ static struct discovery* discover_refs(const char *service, int for_push) die("Authentication failed for '%s'", url); default: show_http_message(&type, &buffer); - http_error(url, http_ret); + http_error(url); die("HTTP request failed"); } From 39a570f26c953e8277a6a26c00da657b240dcdd4 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 5 Apr 2013 18:22:01 -0400 Subject: [PATCH 7/9] http: re-word http error message When we report an http error code, we say something like: error: The requested URL reported failure: 403 Forbidden while accessing http://example.com/repo.git Everything between "error:" and "while" is written by curl, and the resulting sentence is hard to read (especially because there is no punctuation between curl's sentence and the remainder of ours). Instead, let's re-order this to give better flow: error: unable to access 'http://example.com/repo.git: The requested URL reported failure: 403 Forbidden This is still annoyingly long, but at least reads more clearly left to right. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- http.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/http.c b/http.c index 5e6f67d004..64068a2fc7 100644 --- a/http.c +++ b/http.c @@ -943,7 +943,7 @@ cleanup: void http_error(const char *url) { - error("%s while accessing %s", curl_errorstr, url); + error("unable to access '%s': %s", url, curl_errorstr); } int http_fetch_ref(const char *base, struct ref *ref) From de89f0b25a42238948787421c0253228c006f7fa Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 5 Apr 2013 18:22:15 -0400 Subject: [PATCH 8/9] remote-curl: die directly with http error messages When we encounter an unknown http error (e.g., a 403), we hand the error code to http_error, which then prints it with error(). After that we die with the redundant message "HTTP request failed". Instead, let's just drop http_error entirely, which does nothing but pass arguments to error(), and instead die directly with a useful message. So before: $ git clone https://example.com/repo.git Cloning into 'repo'... error: unable to access 'https://example.com/repo.git': The requested URL returned error: 403 Forbidden fatal: HTTP request failed and after: $ git clone https://example.com/repo.git Cloning into 'repo'... fatal: unable to access 'https://example.com/repo.git': The requested URL returned error: 403 Forbidden Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- remote-curl.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/remote-curl.c b/remote-curl.c index 9abe4b7116..60eda63081 100644 --- a/remote-curl.c +++ b/remote-curl.c @@ -216,8 +216,7 @@ static struct discovery* discover_refs(const char *service, int for_push) die("Authentication failed for '%s'", url); default: show_http_message(&type, &buffer); - http_error(url); - die("HTTP request failed"); + die("unable to access '%s': %s", url, curl_errorstr); } last= xcalloc(1, sizeof(*last_discovery)); From 4df13f69e9facbd09a2b06478e8c40082cda7ce6 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 5 Apr 2013 18:22:31 -0400 Subject: [PATCH 9/9] http: drop http_error function This function is a single-liner and is only called from one place. Just inline it, which makes the code more obvious. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- http-push.c | 2 +- http.c | 5 ----- http.h | 5 ----- 3 files changed, 1 insertion(+), 11 deletions(-) diff --git a/http-push.c b/http-push.c index 439a555a72..395a8cfc10 100644 --- a/http-push.c +++ b/http-push.c @@ -1551,7 +1551,7 @@ static int remote_exists(const char *path) ret = 0; break; case HTTP_ERROR: - http_error(url); + error("unable to access '%s': %s", url, curl_errorstr); default: ret = -1; } diff --git a/http.c b/http.c index 64068a2fc7..58c063c91b 100644 --- a/http.c +++ b/http.c @@ -941,11 +941,6 @@ cleanup: return ret; } -void http_error(const char *url) -{ - error("unable to access '%s': %s", url, curl_errorstr); -} - int http_fetch_ref(const char *base, struct ref *ref) { char *url; diff --git a/http.h b/http.h index fa65128f9e..4dc53531ae 100644 --- a/http.h +++ b/http.h @@ -135,11 +135,6 @@ extern char *get_remote_object_url(const char *url, const char *hex, */ int http_get_strbuf(const char *url, struct strbuf *content_type, struct strbuf *result, int options); -/* - * Prints an error message using error() containing url and curl_errorstr. - */ -void http_error(const char *url); - extern int http_fetch_ref(const char *base, struct ref *ref); /* Helpers for fetching packs */