From: Kyle J. McKay <mackyle@gmail.com>
Date: Thu, 22 May 2014 00:07:38 GMT
Subject: Re: [PATCH 7/9] remote-curl: recognize text/plain with a charset parameter
Message-ID: <90CE006F-EE74-40D2-8847-507E37021D84@gmail.com>
In-Reply-To: <20140521103301.GG30464@sigill.intra.peff.net>

On May 21, 2014, at 03:33, Jeff King wrote:

> Commit 426e70d (remote-curl: show server content on http
> errors, 2013-04-05) tried to recognize text/plain content
> types, but fails to do so if they have any parameters.
>
> This patches makes our parsing more liberal, though we still
> do not do anything useful with a charset parameter.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> remote-curl.c              | 26 ++++++++++++++++++--------
> t/lib-httpd/error.sh       |  8 +++++++-
> t/t5550-http-fetch-dumb.sh |  5 +++++
> 3 files changed, 30 insertions(+), 9 deletions(-)
>
> diff --git a/remote-curl.c b/remote-curl.c
> index a5ab977..6d1b206 100644
> --- a/remote-curl.c
> +++ b/remote-curl.c
> @@ -194,20 +194,30 @@ static void free_discovery(struct discovery *d)
> 	}
> }
>
> +/*
> + * We only show text/plain parts, as other types are likely
> + * to be ugly to look at on the user's terminal.
> + */
> +static int content_type_is_terminal_friendly(struct strbuf *type)
> +{
> +	const char *p;
> +
> +	p = skip_prefix(type->buf, "text/plain");
> +	if (!p || (*p && *p != ';'))
> +		return 0;
> +
> +	return 1;
> +}
> +

I think that a strict reading of RFC 2616 allows "text/plain ;  
charset=utf-8" as well as "text/plain;charset=utf-8" and "text/plain;  
charset=utf-8".  So perhaps this if line instead:

+	if (!p || (*p && *p != ';' && *p != ' ' && *p != '\t'))

See RFC 2616 section 2.2 for the definition of linear white space  
(LWS) and the end of section 2.1 for the "implied *LWS" rule that  
allows it.

> 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 (strcmp(type->buf, "text/plain"))
> +	if (!content_type_is_terminal_friendly(type))
> 		return -1;
>
> +	/* TODO should record charset and reencode msg to  
> logOutputEncoding */
> +
> 	strbuf_trim(msg);
> 	if (!msg->len)
> 		return -1;
> diff --git a/t/lib-httpd/error.sh b/t/lib-httpd/error.sh
> index 786f281..02e80b3 100755
> --- a/t/lib-httpd/error.sh
> +++ b/t/lib-httpd/error.sh
> @@ -3,6 +3,7 @@
> printf "Status: 500 Intentional Breakage\n"
>
> printf "Content-Type: "
> +charset=iso8859-1
> case "$PATH_INFO" in
> *html*)
> 	printf "text/html"
> @@ -10,8 +11,13 @@ case "$PATH_INFO" in
> *text*)
> 	printf "text/plain"
> 	;;
> +*charset*)
> +	printf "text/plain; charset=utf-8"
> +	charset=utf-8
> +	;;
> esac
> printf "\n"
>
> printf "\n"
> -printf "this is the error message\n"
> +printf "this is the error message\n" |
> +iconv -f us-ascii -t $charset
> diff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh
> index 13defd3..b35b261 100755
> --- a/t/t5550-http-fetch-dumb.sh
> +++ b/t/t5550-http-fetch-dumb.sh
> @@ -181,5 +181,10 @@ test_expect_success 'git client does not show  
> html errors' '
> 	! 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
> +'
> +
> stop_httpd
> test_done
> -- 
> 2.0.0.rc1.436.g03cb729
>