Merge branch 'jk/http-error-messages'

Improve error reporting from the http transfer clients.

* jk/http-error-messages:
  http: drop http_error function
  remote-curl: die directly with http error messages
  http: re-word http error message
  http: simplify http_error helper function
  remote-curl: consistently report repo url for http errors
  remote-curl: always show friendlier 404 message
  remote-curl: let servers override http 404 advice
  remote-curl: show server content on http errors
  http: add HTTP_KEEP_ERROR option

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);
+		error("unable to access '%s': %s", url, curl_errorstr);
 	default:
 		ret = -1;
 	}

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);
 
@@ -836,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;
 	}
 
@@ -862,6 +884,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);
 }
 
@@ -903,15 +941,6 @@ cleanup:
 	return ret;
 }
 
-int http_error(const char *url, int ret)
-{
-	/* http_request has already handled HTTP_START_FAILED. */
-	if (ret != HTTP_START_FAILED)
-		error("%s while accessing %s", curl_errorstr, url);
-
-	return ret;
-}
-
 int http_fetch_ref(const char *base, struct ref *ref)
 {
 	char *url;

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
@@ -134,12 +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,
- * and returns ret.
- */
-int http_error(const char *url, int ret);
-
 extern int http_fetch_ref(const char *base, struct ref *ref);
 
 /* Helpers for fetching packs */

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,18 +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:
-		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:
-		die("Authentication failed");
+		show_http_message(&type, &buffer);
+		die("Authentication failed for '%s'", url);
 	default:
-		http_error(refs_url, http_ret);
-		die("HTTP request failed");
+		show_http_message(&type, &buffer);
+		die("unable to access '%s': %s", url, curl_errorstr);
 	}
 
 	last= xcalloc(1, sizeof(*last_discovery));