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

From: Emma Brooks <me@pluvano.com>
Date: Apr 7, 2020, 03:46 UTC
Message-ID: <20200407034622.GA42812@pluvano.com>
In-Reply-To: <xmqqd08lwatm.fsf@gitster.c.googlers.com>

On 2020-04-05 20:29:57-0700, Junio C Hamano wrote:

Show 8 quoted lines

> 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.

It's also too vague and it's not entirely clear from the option itself what sort of encoding it refers to. I will change it to --[no-]q-encode-headers and format.qEncodeHeaders in v2 unless there are other suggestions.

Show 8 quoted lines

> > +--[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.

OK.

Show 5 quoted lines

> > +	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.

I'll reference the RFC directly in v2.

Show 7 quoted lines

> > +	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.

OK, I'll go with that.

Show 8 quoted lines

> > -		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.

OK.