Re: [PATCH] format-patch: teach --no-encode-headers

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 6, 2020, 03:29 UTC
Message-ID: <xmqqd08lwatm.fsf@gitster.c.googlers.com>
In-Reply-To: <20200405231109.8249-1-me@pluvano.com>

Emma Brooks <me@pluvano.com> writes:

Show 10 quoted lines

> When commit subjects or authors have non-ASCII characters, git
> format-patch Q-encodes them so they can be safely sent over email.
> However, if the patch transfer method is something other than email (web
> review tools, sneakernet), this only serves to make the patch metadata
> harder to read without first applying it (unless you can decode RFC 2047
> in your head). git am as well as some email software supports
> non-Q-encoded mail as described in RFC 6531.
>
> Add --[no-]encode-headers and format.encodeHeaders to let the user
> control this behavior.

This would be immensely useful. I often find the in-body headers that are Q-encoded too ugly to live.

Is the RFC2047 the only thing we do to message headers? What I am trying is to figure out if "encode-headers - yes/no?" would be a stable (iow, would we be gaining other kinds of encoding over time?) and well-defined (iow, is there a case where one kind of 'encoding' is still desirable while disabling other kinds of 'encoding' is wanted?) question. If there is any doubt in your answers to the above question, we may have to make sure the name of the option makes it clear to users what kind of encoding we're talking about.

> +format.encodeHeaders::
> +	Encode email headers that have non-ASCII characters with
> +	"Q-encoding" for email transmission. Defaults to true.

OK.

>  
> +--[no-]encode-headers::

I think we'd want to standadize on writing these out, i.e.

        --encode-headers::
        --no-encode-headers::

so let's follow that when adding a new option.

> +	Encode email headers that have non-ASCII characters with
> +	"Q-encoding", instead of outputting the headers verbatim. The

I wonder if calling RFC2047 out helps readers here, when they wonder what Q is and how they can decipher it.

> +	default is `--encode-headers` unless the `format.encodeHeaders`
> +	configuration variable is set.

I am wondering if we can go even shorter, e.g.

	The default is set to the value of `format.encodeHeaders`
	configuration variable.

> -		if (needs_rfc2047_encoding(namebuf, namelen)) {
> +		if (pp->encode_headers &&
> +				needs_rfc2047_encoding(namebuf, namelen)) {

Don't overly indent the second line like this. The same comment applies to the next hunk (not quoted).

Thanks.