Re: [BUG] format-patch does not wrap From-field after author name
On Thu, Apr 14, 2011 at 7:50 PM, Jeff King <peff@peff.net> wrote:
Show 12 quoted lines
> On Thu, Apr 14, 2011 at 10:12:44AM -0700, Junio C Hamano wrote:
>
>> Erik Faye-Lund <kusmabite@gmail.com> writes:
>>
>> > With certain combinations of author name and email lengths, git
>> > format-patch does not correctly wrap the From line to be below 78
>> > characters, violating rfc2047.
>>
>> Didn't we have this fixed quite a while ago with the series that ends at
>> c22e7de (format-patch: rfc2047-encode newlines in headers, 2011-02-23)?
>>
>> What version of git do you use?
I believe it was straight from todays master, but I'm not still at that machine.
Show 39 quoted lines
> No, that doesn't fix it. The problem is a short name and a long email
> address. We rfc2047-encode and wrap the name, but then tack the email
> onto the end without caring about line length.
>
> According to rfc2047, we are specifically forbidden to use an
> encoded-word in an addr-spec. So it would not make sense to assemble the
> header first and then feed the whole thing to add_rfc2047.
>
> Fortunately, this is an easy thing to wrap, since we can't actually
> break the addr-spec across lines. We can just check if the line is long,
> and if so, wrap the whole address to the next line, which is the best we
> can do (and presumably nobody has an address portion over 78
> characters). That patch looks something like:
>
> diff --git a/pretty.c b/pretty.c
> index e1d8a8f..63a7c2f 100644
> --- a/pretty.c
> +++ b/pretty.c
> @@ -287,6 +287,7 @@ void pp_user_info(const char *what, enum cmit_fmt fmt, struct strbuf *sb,
> if (fmt == CMIT_FMT_EMAIL) {
> char *name_tail = strchr(line, '<');
> int display_name_length;
> + int final_line;
> if (!name_tail)
> return;
> while (line < name_tail && isspace(name_tail[-1]))
> @@ -294,6 +295,11 @@ void pp_user_info(const char *what, enum cmit_fmt fmt, struct strbuf *sb,
> display_name_length = name_tail - line;
> strbuf_addstr(sb, "From: ");
> add_rfc2047(sb, line, display_name_length, encoding);
> + for (final_line = 0; final_line < sb->len; final_line++)
> + if (sb->buf[sb->len - final_line - 1] == '\n')
> + break;
> + if (namelen - display_name_length + final_line > 78)
> + strbuf_addch(sb, '\n');
> strbuf_add(sb, name_tail, namelen - display_name_length);
> strbuf_addch(sb, '\n');
> } else {
>Yes, this is the reasonable fix. Basically copying the wrapping logic from add_rfc2047...
> Note that it relies on the commit header having a space before the "<",
> which forms the continuation whitespace for the header. This is probably
> reasonable, but we could double-check if we want to handle malformed
> commit headers better.
I think that's a reasonable assumption; this field comes straight from the commit. It should already be well-formed, no?
> Or we could just ignore it. AFAICS, this doesn't actually violate
> rfc2047, nor rfc5322. The 78-character limit is simply a SHOULD, and
> we have up to 998 for MUST. For a single-address header[1], this seems
> kind of unlikely to me.
True. But since the fix is as simple as it is, perhaps it's worth it just for the clean conscience?
Don't get me wrong, this was just something I stumbled over. I don't have any real-world case where it breaks.
> [1] For multi-address headers like "format-patch --cc=foo --cc=bar", it
> looks like we already break them across lines.
Yes, but this is even worse: these fields don't get encoded at all!
> No idea if "send-email"
> is sensible in this area or not.
...but luckily send-email is sensible. The first thing it does is to unfold each header line. Addresses are rfc2047-decoded and recoded (if needed, utf-8 is assumed as the encoding if none is found), and To/Cc headers are laid out one address per line.
This still produce invalid To/Cc headers if send-email isn't used, though.
Erik Faye-Lund
