Merge branch 'jk/http-errors'

Propagate the error messages from the webserver better to the
client coming over the HTTP transport.

* jk/http-errors:
  http: default text charset to iso-8859-1
  remote-curl: reencode http error messages
  strbuf: add strbuf_reencode helper
  http: optionally extract charset parameter from content-type
  http: extract type/subtype portion of content-type
  t5550: test display of remote http error messages
  t/lib-httpd: use write_script to copy CGI scripts
  test-lib: preserve GIT_CURL_VERBOSE from the environment

Documentation/technical/api-strbuf.txt

@@ -134,6 +134,11 @@ Functions
 
 	Strip whitespace from the beginning of a string.
 
+`strbuf_reencode`::
+
+	Replace the contents of the strbuf with a reencoded form.  Returns -1
+	on error, 0 on success.
+
 `strbuf_tolower`::
 
 	Lowercase each character in the buffer using `tolower`.

http.c

@@ -906,6 +906,83 @@ static CURLcode curlinfo_strbuf(CURL *curl, CURLINFO info, struct strbuf *buf)
 	return ret;
 }
 
+/*
+ * Check for and extract a content-type parameter. "raw"
+ * should be positioned at the start of the potential
+ * parameter, with any whitespace already removed.
+ *
+ * "name" is the name of the parameter. The value is appended
+ * to "out".
+ */
+static int extract_param(const char *raw, const char *name,
+			 struct strbuf *out)
+{
+	size_t len = strlen(name);
+
+	if (strncasecmp(raw, name, len))
+		return -1;
+	raw += len;
+
+	if (*raw != '=')
+		return -1;
+	raw++;
+
+	while (*raw && !isspace(*raw))
+		strbuf_addch(out, *raw++);
+	return 0;
+}
+
+/*
+ * Extract a normalized version of the content type, with any
+ * spaces suppressed, all letters lowercased, and no trailing ";"
+ * or parameters.
+ *
+ * Note that we will silently remove even invalid whitespace. For
+ * example, "text / plain" is specifically forbidden by RFC 2616,
+ * but "text/plain" is the only reasonable output, and this keeps
+ * our code simple.
+ *
+ * If the "charset" argument is not NULL, store the value of any
+ * charset parameter there.
+ *
+ * Example:
+ *   "TEXT/PLAIN; charset=utf-8" -> "text/plain", "utf-8"
+ *   "text / plain" -> "text/plain"
+ */
+static void extract_content_type(struct strbuf *raw, struct strbuf *type,
+				 struct strbuf *charset)
+{
+	const char *p;
+
+	strbuf_reset(type);
+	strbuf_grow(type, raw->len);
+	for (p = raw->buf; *p; p++) {
+		if (isspace(*p))
+			continue;
+		if (*p == ';') {
+			p++;
+			break;
+		}
+		strbuf_addch(type, tolower(*p));
+	}
+
+	if (!charset)
+		return;
+
+	strbuf_reset(charset);
+	while (*p) {
+		while (isspace(*p))
+			p++;
+		if (!extract_param(p, "charset", charset))
+			return;
+		while (*p && !isspace(*p))
+			p++;
+	}
+
+	if (!charset->len && starts_with(type->buf, "text/"))
+		strbuf_addstr(charset, "ISO-8859-1");
+}
+
 /* http_request() targets */
 #define HTTP_REQUEST_STRBUF	0
 #define HTTP_REQUEST_FILE	1
@@ -957,9 +1034,13 @@ static int http_request(const char *url,
 
 	ret = run_one_slot(slot, &results);
 
-	if (options && options->content_type)
-		curlinfo_strbuf(slot->curl, CURLINFO_CONTENT_TYPE,
-				options->content_type);
+	if (options && options->content_type) {
+		struct strbuf raw = STRBUF_INIT;
+		curlinfo_strbuf(slot->curl, CURLINFO_CONTENT_TYPE, &raw);
+		extract_content_type(&raw, options->content_type,
+				     options->charset);
+		strbuf_release(&raw);
+	}
 
 	if (options && options->effective_url)
 		curlinfo_strbuf(slot->curl, CURLINFO_EFFECTIVE_URL,

http.h

@@ -143,6 +143,13 @@ struct http_get_options {
 	/* If non-NULL, returns the content-type of the response. */
 	struct strbuf *content_type;
 
+	/*
+	 * If non-NULL, and content_type above is non-NULL, returns
+	 * the charset parameter from the content-type. If none is
+	 * present, returns an empty string.
+	 */
+	struct strbuf *charset;
+
 	/*
 	 * If non-NULL, returns the URL we ended up at, including any
 	 * redirects we followed.

remote-curl.c

@@ -194,19 +194,19 @@ static void free_discovery(struct discovery *d)
 	}
 }
 
-static int show_http_message(struct strbuf *type, struct strbuf *msg)
+static int show_http_message(struct strbuf *type, struct strbuf *charset,
+			     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"))
+	if (strcmp(type->buf, "text/plain"))
 		return -1;
+	if (charset->len)
+		strbuf_reencode(msg, charset->buf, get_log_output_encoding());
 
 	strbuf_trim(msg);
 	if (!msg->len)
@@ -225,6 +225,7 @@ static struct discovery* discover_refs(const char *service, int for_push)
 {
 	struct strbuf exp = STRBUF_INIT;
 	struct strbuf type = STRBUF_INIT;
+	struct strbuf charset = STRBUF_INIT;
 	struct strbuf buffer = STRBUF_INIT;
 	struct strbuf refs_url = STRBUF_INIT;
 	struct strbuf effective_url = STRBUF_INIT;
@@ -249,6 +250,7 @@ static struct discovery* discover_refs(const char *service, int for_push)
 
 	memset(&options, 0, sizeof(options));
 	options.content_type = &type;
+	options.charset = &charset;
 	options.effective_url = &effective_url;
 	options.base_url = &url;
 	options.no_cache = 1;
@@ -259,13 +261,13 @@ static struct discovery* discover_refs(const char *service, int for_push)
 	case HTTP_OK:
 		break;
 	case HTTP_MISSING_TARGET:
-		show_http_message(&type, &buffer);
+		show_http_message(&type, &charset, &buffer);
 		die("repository '%s' not found", url.buf);
 	case HTTP_NOAUTH:
-		show_http_message(&type, &buffer);
+		show_http_message(&type, &charset, &buffer);
 		die("Authentication failed for '%s'", url.buf);
 	default:
-		show_http_message(&type, &buffer);
+		show_http_message(&type, &charset, &buffer);
 		die("unable to access '%s': %s", url.buf, curl_errorstr);
 	}
 
@@ -310,6 +312,7 @@ static struct discovery* discover_refs(const char *service, int for_push)
 	strbuf_release(&refs_url);
 	strbuf_release(&exp);
 	strbuf_release(&type);
+	strbuf_release(&charset);
 	strbuf_release(&effective_url);
 	strbuf_release(&buffer);
 	last_discovery = last;

strbuf.c

@@ -1,5 +1,6 @@
 #include "cache.h"
 #include "refs.h"
+#include "utf8.h"
 
 int starts_with(const char *str, const char *prefix)
 {
@@ -99,6 +100,22 @@ void strbuf_ltrim(struct strbuf *sb)
 	sb->buf[sb->len] = '\0';
 }
 
+int strbuf_reencode(struct strbuf *sb, const char *from, const char *to)
+{
+	char *out;
+	int len;
+
+	if (same_encoding(from, to))
+		return 0;
+
+	out = reencode_string_len(sb->buf, sb->len, to, from, &len);
+	if (!out)
+		return -1;
+
+	strbuf_attach(sb, out, len, len);
+	return 0;
+}
+
 void strbuf_tolower(struct strbuf *sb)
 {
 	char *p = sb->buf, *end = sb->buf + sb->len;

strbuf.h

@@ -45,6 +45,7 @@ static inline void strbuf_setlen(struct strbuf *sb, size_t len)
 extern void strbuf_trim(struct strbuf *);
 extern void strbuf_rtrim(struct strbuf *);
 extern void strbuf_ltrim(struct strbuf *);
+extern int strbuf_reencode(struct strbuf *sb, const char *from, const char *to);
 extern void strbuf_tolower(struct strbuf *sb);
 extern int strbuf_cmp(const struct strbuf *, const struct strbuf *);
 

t/lib-httpd.sh

@@ -110,10 +110,15 @@ else
 		"Could not identify web server at '$LIB_HTTPD_PATH'"
 fi
 
+install_script () {
+	write_script "$HTTPD_ROOT_PATH/$1" <"$TEST_PATH/$1"
+}
+
 prepare_httpd() {
 	mkdir -p "$HTTPD_DOCUMENT_ROOT_PATH"
 	cp "$TEST_PATH"/passwd "$HTTPD_ROOT_PATH"
-	cp "$TEST_PATH"/broken-smart-http.sh "$HTTPD_ROOT_PATH"
+	install_script broken-smart-http.sh
+	install_script error.sh
 
 	ln -s "$LIB_HTTPD_MODULE_PATH" "$HTTPD_ROOT_PATH/modules"
 

t/lib-httpd/apache.conf

@@ -97,12 +97,16 @@ Alias /auth/dumb/ www/auth/dumb/
 </LocationMatch>
 ScriptAliasMatch /smart_*[^/]*/(.*) ${GIT_EXEC_PATH}/git-http-backend/$1
 ScriptAlias /broken_smart/ broken-smart-http.sh/
+ScriptAlias /error/ error.sh/
 <Directory ${GIT_EXEC_PATH}>
 	Options FollowSymlinks
 </Directory>
 <Files broken-smart-http.sh>
 	Options ExecCGI
 </Files>
+<Files error.sh>
+  Options ExecCGI
+</Files>
 <Files ${GIT_EXEC_PATH}/git-http-backend>
 	Options ExecCGI
 </Files>

t/lib-httpd/broken-smart-http.sh

@@ -1,4 +1,3 @@
-#!/bin/sh
 printf "Content-Type: text/%s\n" "html"
 echo
 printf "%s\n" "001e# service=git-upload-pack"

t/lib-httpd/error.sh

@@ -0,0 +1,27 @@
+#!/bin/sh
+
+printf "Status: 500 Intentional Breakage\n"
+
+printf "Content-Type: "
+charset=iso-8859-1
+case "$PATH_INFO" in
+*html*)
+	printf "text/html"
+	;;
+*text*)
+	printf "text/plain"
+	;;
+*charset*)
+	printf "text/plain; charset=utf-8"
+	charset=utf-8
+	;;
+*utf16*)
+	printf "text/plain; charset=utf-16"
+	charset=utf-16
+	;;
+esac
+printf "\n"
+
+printf "\n"
+printf "this is the error message\n" |
+iconv -f us-ascii -t $charset

t/t5550-http-fetch-dumb.sh

@@ -171,5 +171,25 @@ test_expect_success 'did not use upload-pack service' '
 	test_cmp exp act
 '
 
+test_expect_success 'git client shows text/plain errors' '
+	test_must_fail git clone "$HTTPD_URL/error/text" 2>stderr &&
+	grep "this is the error message" stderr
+'
+
+test_expect_success 'git client does not show html errors' '
+	test_must_fail git clone "$HTTPD_URL/error/html" 2>stderr &&
+	! grep "this is the error message" stderr
+'
+
+test_expect_success 'git client shows text/plain with a charset' '
+	test_must_fail git clone "$HTTPD_URL/error/charset" 2>stderr &&
+	grep "this is the error message" stderr
+'
+
+test_expect_success 'http error messages are reencoded' '
+	test_must_fail git clone "$HTTPD_URL/error/utf16" 2>stderr &&
+	grep "this is the error message" stderr
+'
+
 stop_httpd
 test_done

t/test-lib.sh

@@ -91,6 +91,7 @@ unset VISUAL EMAIL LANGUAGE COLUMNS $("$PERL_PATH" -e '
 		VALGRIND
 		UNZIP
 		PERF_
+		CURL_VERBOSE
 	));
 	my @vars = grep(/^GIT_/ && !/^GIT_($ok)/o, @env);
 	print join("\n", @vars);