Re: [PATCH 2/5] format-patch: do not wrap rfc2047 encoded headers too late

From: Jan H. Schönherr <schnhrr@cs.tu-berlin.de>
Date: Oct 10, 2012, 09:31 UTC
Message-ID: <5075408F.8050502@cs.tu-berlin.de>
In-Reply-To: <7v7gqzfnpj.fsf@alter.siamese.dyndns.org>

Am 09.10.2012 21:30, schrieb Junio C Hamano:

> Jan H. Schönherr <schnhrr@cs.tu-berlin.de> writes:

...

Show 19 quoted lines

>>  static int is_rfc2047_special(char ch)
>>  {
>> +	/*
>> +	 * We encode ' ' using '=20' even though rfc2047
>> +	 * allows using '_' for readability.  Unfortunately,
>> +	 * many programs do not understand this and just
>> +	 * leave the underscore in place.
>> +	 */
> 
> The sentence break made me read the above three times to understand
> what it is trying to say.  "Unfortunately" refers to what happens if
> we were to use '_', but it initially appeared to be describing some
> bug due to our encoding ' ' as '=20'.  Perhaps like this?
> 
> 	/*
> 	 * rfc2047 allows '_' to encode ' ' for readability, but
> 	 * many programs do not understand ...; encode ' ' using
> 	 * '=20' instead to avoid the problem.
> 	 */

I was just moving that comment (and the following check) around, but I'll update the comment in the next version.

Show 6 quoted lines

>> +	if (ch == ' ' || ch == '\n')
>> +		return 1;
> 
> The comment justifies why this "if (ch == ' ')", which could be part
> of the "return" below, separately is done, but nothing explains why
> you add '\n' (and not other controls, e.g. '\t') to the mix.

The check for '\n' was introduced in commit c22e7de3 ("format-patch: rfc2047-encode newlines in headers").

The commit log was:

    These should generally never happen, as we already
    concatenate multiples in subjects into a single line. But
    let's be defensive, since not encoding them means we will
    output malformed headers.

Having again a look at RFC 2047, I see that we should be even more strict and not allow any non-printable character to be passed through unencoded. I guess that adds another patch to the series. Hmm... Maybe I can split patch 4 into two patches, one that mostly fixes is_rfc2047_special() and one that avoids 822 quoting when doing 2047 encoding.

Show 24 quoted lines

> 
>>  	return (non_ascii(ch) || (ch == '=') || (ch == '?') || (ch == '_'));
>>  }
>>  
>>  static void add_rfc2047(struct strbuf *sb, const char *line, int len,
>>  		       const char *encoding)
>>  {
>> -	static const int max_length = 78; /* per rfc2822 */
>> +	static const int max_length = 76; /* per rfc2047 */
>>  	int i;
>>  	int line_len;
>>  
>> @@ -286,7 +295,7 @@ static void add_rfc2047(struct strbuf *sb, const char *line, int len,
>>  		if ((i + 1 < len) && (ch == '=' && line[i+1] == '?'))
>>  			goto needquote;
>>  	}
>> -	strbuf_add_wrapped_bytes(sb, line, len, -line_len, 1, max_length+1);
>> +	strbuf_add_wrapped_bytes(sb, line, len, -line_len, 1, 78+1);
>>  	return;
> 
> Yuck.  If you do want to retain 78 for non-quoted output for
> backward compatibility, that is OK, but if that is the case, please
> introduce a new constant "max_quoted_length" or something to stand
> for 76 and use it in the "needquote:" part below.

Will do.

Regards Jan