[RFC/PATCH 0/9] friendlier http error messages

[RFC/PATCH 0/9] friendlier http error messages

[Jeff King]

The error messages that git generates for routine http problems can
sometimes be a bit verbose or confusing. They also provide no
opportunity for the server to communicate any free-form text, even
though the server knows much better than the git client the reason for
the error or what the next step to suggest to the user might be.

This series provides a channel for those messages, and does some general
cleanup and reformatting of the error messages that git itself produces.

Here are some before-and-after examples with this series (apologies for
the long lines, but they are part of the ugliness I want to address, so
I am leaving them in).

The "remote:" bits are simulated in the output below, as I haven't yet
updated GitHub's server side to produce more useful messages. So you can
repeat the tests, but note that the text you get from the server will
not be the same (e.g., our 404 currently just says "Repository not
found" which is not all that helpful).

  [before]
  $ git ls-remote https://github.com/non/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?

  [after]
  $ git ls-remote https://github.com/non/existent
  remote: The remote repository was not found, or you do not have
  remote: permission to access it.
  fatal: repository 'https://github.com/non/existent/' not found

  [before]
  $ GIT_SMART_HTTP=0 git ls-remote https://github.com/git/git.git
  error: The requested URL returned error: 403 Forbidden while accessing https://github.com/git/git.git/info/refs
  fatal: HTTP request failed

  [after]
  $ GIT_SMART_HTTP=0 git ls-remote https://github.com/git/git.git
  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.
  fatal: unable to access 'https://github.com/git/git.git/': The requested URL returned error: 403

I still really hate the length of the generic http message (which you
can see in the final 403 example). The text "The requested URL returned
error:" comes from curl, though there is actually an opportunity to
munge it, as you will see in the patches. However, I was unable to come
up with a shorter text that sounded any better.

Another option would be to just split it across lines with some
indentation, like:

  fatal: The requested URL returned error: 403
    while accessing https://github.com/git/git.git

I'm open to suggestions.

There are a lot of little patches, as I tried to explain the rationale
for each individual change (and it makes it easy to take or reject
individual patches). If we do take them all, it may make sense to just
squash patches 3-5.

  [1/9]: http: add HTTP_KEEP_ERROR option
  [2/9]: remote-curl: show server content on http errors
  [3/9]: remote-curl: let servers override http 404 advice
  [4/9]: remote-curl: always show friendlier 404 message
  [5/9]: remote-curl: consistently report repo url for http errors
  [6/9]: http: simplify http_error helper function
  [7/9]: http: re-word http error message
  [8/9]: remote-curl: die directly with http error messages
  [9/9]: http: drop http_error function

-Peff

[PATCH 1/9] http: add HTTP_KEEP_ERROR option

[Jeff King]

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 <peff@peff.net>
---
 http.c | 37 +++++++++++++++++++++++++++++++++++++
 http.h |  1 +
 2 files changed, 38 insertions(+)

diff --git a/http.c b/http.c
index 8803c70..45cc7c7 100644
--- a/http.c
+++ b/http.c
@@ -761,6 +761,25 @@ int handle_curl_result(struct slot_results *results)
 
 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 25d1931..0fe54f4 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
-- 
1.8.2.rc0.33.gd915649

[PATCH 2/9] remote-curl: show server content on http errors

[Jeff King]

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 <peff@peff.net>
---
I took Jonathan's suggestion of just respecting text/plain to signify a
message that git can show on the terminal. It is the most sensible
content-type to use, I think, but I'm a little worried that
random servers may produce totally uninteresting text/plain (e.g., many
servers ship with 404 pages that just say "404" again with more useless
text). But such pages tend to be text/html, so we may be able to get
away with assuming text/plain will be useful.

 remote-curl.c | 33 ++++++++++++++++++++++++++++++++-
 1 file changed, 32 insertions(+), 1 deletion(-)

diff --git a/remote-curl.c b/remote-curl.c
index 93a09a6..4fea94f 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");
 	}
-- 
1.8.2.rc0.33.gd915649

[PATCH 3/9] remote-curl: let servers override http 404 advice

[Jeff King]

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 <peff@peff.net>
---
 remote-curl.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/remote-curl.c b/remote-curl.c
index 4fea94f..083efdf 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:
-- 
1.8.2.rc0.33.gd915649

[PATCH 4/9] remote-curl: always show friendlier 404 message

[Jeff King]

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 <peff@peff.net>
---
This is obviously a more stringent version of the last patch, and could
just replace it. I think the last one is a no-brainer, because it lets
well-configured sites replace these messages. This one is less obvious,
because we may be hitting some random poorly configured server that
somebody is trying to set up.

Still, if you think about the number of people fetching (against any
server) versus the number of people configuring new servers, I think the
advice to run update-server-info has a vanishingly small chance of being
useful (and IMHO is misleading if you do not know how dumb-http works,
as it makes you think the server is misconfigured, when it is much more
likely to be a user error).

 remote-curl.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/remote-curl.c b/remote-curl.c
index 083efdf..5d9f961 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");
-- 
1.8.2.rc0.33.gd915649

[PATCH 5/9] remote-curl: consistently report repo url for http errors

[Jeff King]

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 <peff@peff.net>
---
This is the same rationale as the latter half of the last patch; if we
take them all, I'd be happy to see them squashed together.

 remote-curl.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/remote-curl.c b/remote-curl.c
index 5d9f961..6c6714b 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");
 	}
 
-- 
1.8.2.rc0.33.gd915649

[PATCH 6/9] http: simplify http_error helper function

[Jeff King]

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 <peff@peff.net>
---
 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 bd66f6a..439a555 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 45cc7c7..5e6f67d 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 @@ int http_error(const char *url, int ret)
 	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 0fe54f4..fa65128 100644
--- a/http.h
+++ b/http.h
@@ -136,10 +136,9 @@ int http_get_strbuf(const char *url, struct strbuf *content_type, struct strbuf
 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 6c6714b..9abe4b7 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");
 	}
 
-- 
1.8.2.rc0.33.gd915649

[PATCH 7/9] http: re-word http error message

[Jeff King]

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 <peff@peff.net>
---
As I mentioned in the cover letter, I'm still not that excited about the
"after" result here. Suggestions welcome.

 http.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/http.c b/http.c
index 5e6f67d..64068a2 100644
--- a/http.c
+++ b/http.c
@@ -943,7 +943,7 @@ void http_error(const char *url)
 
 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)
-- 
1.8.2.rc0.33.gd915649

[PATCH 8/9] remote-curl: die directly with http error messages

[Jeff King]

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 <peff@peff.net>
---
 remote-curl.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/remote-curl.c b/remote-curl.c
index 9abe4b7..60eda63 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));
-- 
1.8.2.rc0.33.gd915649

[PATCH 9/9] http: drop http_error function

[Jeff King]

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 <peff@peff.net>
---
This is a cleanup made possible by the previous patch.

 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 439a555..395a8cf 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 64068a2..58c063c 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 fa65128..4dc5353 100644
--- a/http.h
+++ b/http.h
@@ -135,11 +135,6 @@ int http_get_strbuf(const char *url, struct strbuf *content_type, struct strbuf
  */
 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 */
-- 
1.8.2.rc0.33.gd915649