{"thread":{"id":"27097","subject":"[BUG] format-patch does not wrap From-field after author name","startedAt":"2011-04-14T17:01:41Z","lastAt":"2011-04-16T01:45:25Z","messageCount":15,"participants":["Erik Faye-Lund","Junio C Hamano","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"165816","messageId":"BANLkTimruwojkq_HNMZeCDBV6K8_aFc_XQ@mail.gmail.com","threadId":"27097","inReplyTo":null,"subject":"[BUG] format-patch does not wrap From-field after author name","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2011-04-14T17:01:41Z","receivedAt":"2011-04-14T17:01:41Z","isPatch":false,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"With certain combinations of author name and email lengths, git\nformat-patch does not correctly wrap the From line to be below 78\ncharacters, violating rfc2047.\n\nThis happens because pp_user_info in pretty.c use add_rfc2047 (which\nwraps) for the name, but strbuf_add (which does not wrap) for the\nemail part.\n\nHere's a test that illustrate the problem:\n\ndiff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh\nindex 9c66367..a4b8b59 100755\n--- a/t/t4014-format-patch.sh\n+++ b/t/t4014-format-patch.sh\n@@ -793,4 +793,19 @@ test_expect_success 'format-patch wraps extremely\nlong headers (rfc2047)' '\n \ttest_cmp expect subject\n '\n\n+M8=\"foo_bar_\"\n+M64=$M8$M8$M8$M8$M8$M8$M8$M8\n+cat >expect <<'EOF'\n+From: foo_bar_foo_bar_foo_bar_foo_bar_foo_bar_foo_bar_foo_bar_foo_bar\n+ \n+EOF\n+test_expect_success 'format-patch wraps non-quotable headers' '\n+\trm -rf patches/ &&\n+\techo content >>file &&\n+\tgit add file &&\n+\tgit commit -mfoo --author \"$M64 \" &&\n+\tgit format-patch --stdout -1 >patch &&\n+\tsed -n \"/^From: /p; /^ /p; /^$/q\" from &&\n+\ttest_cmp expect from\n+'\n test_done\n"},{"id":"165818","messageId":"7v39lkiwoj.fsf@alter.siamese.dyndns.org","threadId":"27097","inReplyTo":"BANLkTimruwojkq_HNMZeCDBV6K8_aFc_XQ@mail.gmail.com","subject":"Re: [BUG] format-patch does not wrap From-field after author name","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-04-14T17:12:44Z","receivedAt":"2011-04-14T17:12:44Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Erik Faye-Lund writes:\n\n> With certain combinations of author name and email lengths, git\n> format-patch does not correctly wrap the From line to be below 78\n> characters, violating rfc2047.\n\nDidn't we have this fixed quite a while ago with the series that ends at\nc22e7de (format-patch: rfc2047-encode newlines in headers, 2011-02-23)?\n\nWhat version of git do you use?\n"},{"id":"165820","messageId":"20110414175034.GA23342@sigill.intra.peff.net","threadId":"27097","inReplyTo":"7v39lkiwoj.fsf@alter.siamese.dyndns.org","subject":"Re: [BUG] format-patch does not wrap From-field after author name","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-04-14T17:50:34Z","receivedAt":"2011-04-14T17:50:34Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 14, 2011 at 10:12:44AM -0700, Junio C Hamano wrote:\n\n> Erik Faye-Lund writes:\n> \n> > With certain combinations of author name and email lengths, git\n> > format-patch does not correctly wrap the From line to be below 78\n> > characters, violating rfc2047.\n> \n> Didn't we have this fixed quite a while ago with the series that ends at\n> c22e7de (format-patch: rfc2047-encode newlines in headers, 2011-02-23)?\n> \n> What version of git do you use?\n\nNo, that doesn't fix it. The problem is a short name and a long email\naddress. We rfc2047-encode and wrap the name, but then tack the email\nonto the end without caring about line length.\n\nAccording to rfc2047, we are specifically forbidden to use an\nencoded-word in an addr-spec. So it would not make sense to assemble the\nheader first and then feed the whole thing to add_rfc2047.\n\nFortunately, this is an easy thing to wrap, since we can't actually\nbreak the addr-spec across lines. We can just check if the line is long,\nand if so, wrap the whole address to the next line, which is the best we\ncan do (and presumably nobody has an address portion over 78\ncharacters). That patch looks something like:\n\ndiff --git a/pretty.c b/pretty.c\nindex e1d8a8f..63a7c2f 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -287,6 +287,7 @@ void pp_user_info(const char *what, enum cmit_fmt fmt, struct strbuf *sb,\n \tif (fmt == CMIT_FMT_EMAIL) {\n \t\tchar *name_tail = strchr(line, '<');\n \t\tint display_name_length;\n+\t\tint final_line;\n \t\tif (!name_tail)\n \t\t\treturn;\n \t\twhile (line < name_tail && isspace(name_tail[-1]))\n@@ -294,6 +295,11 @@ void pp_user_info(const char *what, enum cmit_fmt fmt, struct strbuf *sb,\n \t\tdisplay_name_length = name_tail - line;\n \t\tstrbuf_addstr(sb, \"From: \");\n \t\tadd_rfc2047(sb, line, display_name_length, encoding);\n+\t\tfor (final_line = 0; final_line < sb->len; final_line++)\n+\t\t\tif (sb->buf[sb->len - final_line - 1] == '\\n')\n+\t\t\t\tbreak;\n+\t\tif (namelen - display_name_length + final_line > 78)\n+\t\t\tstrbuf_addch(sb, '\\n');\n \t\tstrbuf_add(sb, name_tail, namelen - display_name_length);\n \t\tstrbuf_addch(sb, '\\n');\n \t} else {\n\nNote that it relies on the commit header having a space before the \"<\",\nwhich forms the continuation whitespace for the header. This is probably\nreasonable, but we could double-check if we want to handle malformed\ncommit headers better.\n\nOr we could just ignore it. AFAICS, this doesn't actually violate\nrfc2047, nor rfc5322. The 78-character limit is simply a SHOULD, and\nwe have up to 998 for MUST. For a single-address header[1], this seems\nkind of unlikely to me.\n\n-Peff\n\n[1] For multi-address headers like \"format-patch --cc=foo --cc=bar\", it\nlooks like we already break them across lines. No idea if \"send-email\"\nis sensible in this area or not.\n"},{"id":"165821","messageId":"20110414175212.GB23342@sigill.intra.peff.net","threadId":"27097","inReplyTo":"BANLkTimruwojkq_HNMZeCDBV6K8_aFc_XQ@mail.gmail.com","subject":"Re: [BUG] format-patch does not wrap From-field after author name","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-04-14T17:52:12Z","receivedAt":"2011-04-14T17:52:12Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 14, 2011 at 07:01:41PM +0200, Erik Faye-Lund wrote:\n\n> diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh\n> index 9c66367..a4b8b59 100755\n> --- a/t/t4014-format-patch.sh\n> +++ b/t/t4014-format-patch.sh\n> @@ -793,4 +793,19 @@ test_expect_success 'format-patch wraps extremely\n> long headers (rfc2047)' '\n\nSpeaking of wrapping, your MUA seems to have mangled the patch.\n\n> +M8=\"foo_bar_\"\n> +M64=$M8$M8$M8$M8$M8$M8$M8$M8\n> +cat >expect <<'EOF'\n> +From: foo_bar_foo_bar_foo_bar_foo_bar_foo_bar_foo_bar_foo_bar_foo_bar\n> + \n> +EOF\n\nYour expect data is missing the trailing \"_\". You could probably just\ndo:\n\n cat >expect <\n EOF\n\nwhich is even simpler.\n\n-Peff\n"},{"id":"165845","messageId":"BANLkTikzcSuDQyHsd5Oepk_N5WhF_JVCPw@mail.gmail.com","threadId":"27097","inReplyTo":"20110414175212.GB23342@sigill.intra.peff.net","subject":"Re: [BUG] format-patch does not wrap From-field after author name","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2011-04-14T21:06:36Z","receivedAt":"2011-04-14T21:06:36Z","isPatch":false,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Thu, Apr 14, 2011 at 7:52 PM, Jeff King wrote:\n> On Thu, Apr 14, 2011 at 07:01:41PM +0200, Erik Faye-Lund wrote:\n>\n>> diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh\n>> index 9c66367..a4b8b59 100755\n>> --- a/t/t4014-format-patch.sh\n>> +++ b/t/t4014-format-patch.sh\n>> @@ -793,4 +793,19 @@ test_expect_success 'format-patch wraps extremely\n>> long headers (rfc2047)' '\n>\n> Speaking of wrapping, your MUA seems to have mangled the patch.\n>\n>> +M8=\"foo_bar_\"\n>> +M64=$M8$M8$M8$M8$M8$M8$M8$M8\n>> +cat >expect <<'EOF'\n>> +From: foo_bar_foo_bar_foo_bar_foo_bar_foo_bar_foo_bar_foo_bar_foo_bar\n>> + \n>> +EOF\n>\n> Your expect data is missing the trailing \"_\". You could probably just\n> do:\n>\n>  cat >expect <  From: $M64\n>   \n>  EOF\n>\n> which is even simpler.\n>\n> -Peff\n>\n"},{"id":"165847","messageId":"BANLkTi=Oq_jNYb+CSa9nwtoWtEdbF_QcgA@mail.gmail.com","threadId":"27097","inReplyTo":"20110414175212.GB23342@sigill.intra.peff.net","subject":"Re: [BUG] format-patch does not wrap From-field after author name","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2011-04-14T21:07:41Z","receivedAt":"2011-04-14T21:07:41Z","isPatch":false,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Thu, Apr 14, 2011 at 7:52 PM, Jeff King wrote:\n> On Thu, Apr 14, 2011 at 07:01:41PM +0200, Erik Faye-Lund wrote:\n>\n>> diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh\n>> index 9c66367..a4b8b59 100755\n>> --- a/t/t4014-format-patch.sh\n>> +++ b/t/t4014-format-patch.sh\n>> @@ -793,4 +793,19 @@ test_expect_success 'format-patch wraps extremely\n>> long headers (rfc2047)' '\n>\n> Speaking of wrapping, your MUA seems to have mangled the patch.\n>\n\nHehe, yes. How ironic :)\n\nWhat can I say, silly GMail? :P\n\n>> +M8=\"foo_bar_\"\n>> +M64=$M8$M8$M8$M8$M8$M8$M8$M8\n>> +cat >expect <<'EOF'\n>> +From: foo_bar_foo_bar_foo_bar_foo_bar_foo_bar_foo_bar_foo_bar_foo_bar\n>> + \n>> +EOF\n>\n> Your expect data is missing the trailing \"_\". You could probably just\n> do:\n>\n>  cat >expect <  From: $M64\n>   \n>  EOF\n>\n> which is even simpler.\n>\n\nYes. Much better, thanks.\n"},{"id":"165850","messageId":"BANLkTin+K46_RSDsYWHso3v7Gpe_k+0m8Q@mail.gmail.com","threadId":"27097","inReplyTo":"20110414175034.GA23342@sigill.intra.peff.net","subject":"Re: [BUG] format-patch does not wrap From-field after author name","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2011-04-14T21:19:09Z","receivedAt":"2011-04-14T21:19:09Z","isPatch":false,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Thu, Apr 14, 2011 at 7:50 PM, Jeff King wrote:\n> On Thu, Apr 14, 2011 at 10:12:44AM -0700, Junio C Hamano wrote:\n>\n>> Erik Faye-Lund writes:\n>>\n>> > With certain combinations of author name and email lengths, git\n>> > format-patch does not correctly wrap the From line to be below 78\n>> > characters, violating rfc2047.\n>>\n>> Didn't we have this fixed quite a while ago with the series that ends at\n>> c22e7de (format-patch: rfc2047-encode newlines in headers, 2011-02-23)?\n>>\n>> What version of git do you use?\n\nI believe it was straight from todays master, but I'm not still at that machine.\n\n> No, that doesn't fix it. The problem is a short name and a long email\n> address. We rfc2047-encode and wrap the name, but then tack the email\n> onto the end without caring about line length.\n>\n> According to rfc2047, we are specifically forbidden to use an\n> encoded-word in an addr-spec. So it would not make sense to assemble the\n> header first and then feed the whole thing to add_rfc2047.\n>\n> Fortunately, this is an easy thing to wrap, since we can't actually\n> break the addr-spec across lines. We can just check if the line is long,\n> and if so, wrap the whole address to the next line, which is the best we\n> can do (and presumably nobody has an address portion over 78\n> characters). That patch looks something like:\n>\n> diff --git a/pretty.c b/pretty.c\n> index e1d8a8f..63a7c2f 100644\n> --- a/pretty.c\n> +++ b/pretty.c\n> @@ -287,6 +287,7 @@ void pp_user_info(const char *what, enum cmit_fmt fmt, struct strbuf *sb,\n>        if (fmt == CMIT_FMT_EMAIL) {\n>                char *name_tail = strchr(line, '<');\n>                int display_name_length;\n> +               int final_line;\n>                if (!name_tail)\n>                        return;\n>                while (line < name_tail && isspace(name_tail[-1]))\n> @@ -294,6 +295,11 @@ void pp_user_info(const char *what, enum cmit_fmt fmt, struct strbuf *sb,\n>                display_name_length = name_tail - line;\n>                strbuf_addstr(sb, \"From: \");\n>                add_rfc2047(sb, line, display_name_length, encoding);\n> +               for (final_line = 0; final_line < sb->len; final_line++)\n> +                       if (sb->buf[sb->len - final_line - 1] == '\\n')\n> +                               break;\n> +               if (namelen - display_name_length + final_line > 78)\n> +                       strbuf_addch(sb, '\\n');\n>                strbuf_add(sb, name_tail, namelen - display_name_length);\n>                strbuf_addch(sb, '\\n');\n>        } else {\n>\n\nYes, this is the reasonable fix. Basically copying the wrapping logic\nfrom add_rfc2047...\n\n> Note that it relies on the commit header having a space before the \"<\",\n> which forms the continuation whitespace for the header. This is probably\n> reasonable, but we could double-check if we want to handle malformed\n> commit headers better.\n\nI think that's a reasonable assumption; this field comes straight from\nthe commit. It should already be well-formed, no?\n\n> Or we could just ignore it. AFAICS, this doesn't actually violate\n> rfc2047, nor rfc5322. The 78-character limit is simply a SHOULD, and\n> we have up to 998 for MUST. For a single-address header[1], this seems\n> kind of unlikely to me.\n\nTrue. But since the fix is as simple as it is, perhaps it's worth it\njust for the clean conscience?\n\nDon't get me wrong, this was just something I stumbled over. I don't\nhave any real-world case where it breaks.\n\n> [1] For multi-address headers like \"format-patch --cc=foo --cc=bar\", it\n> looks like we already break them across lines.\n\nYes, but this is even worse: these fields don't get encoded at all!\n\n> No idea if \"send-email\"\n> is sensible in this area or not.\n\n...but luckily send-email is sensible. The first thing it does is to\nunfold each header line. Addresses are rfc2047-decoded and recoded (if\nneeded, utf-8 is assumed as the encoding if none is found), and To/Cc\nheaders are laid out one address per line.\n\nThis still produce invalid To/Cc headers if send-email isn't used, though.\n"},{"id":"165855","messageId":"20110414214230.GB7709@sigill.intra.peff.net","threadId":"27097","inReplyTo":"BANLkTin+K46_RSDsYWHso3v7Gpe_k+0m8Q@mail.gmail.com","subject":"Re: [BUG] format-patch does not wrap From-field after author name","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-04-14T21:42:30Z","receivedAt":"2011-04-14T21:42:30Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 14, 2011 at 11:19:09PM +0200, Erik Faye-Lund wrote:\n\n> > Note that it relies on the commit header having a space before the \"<\",\n> > which forms the continuation whitespace for the header. This is probably\n> > reasonable, but we could double-check if we want to handle malformed\n> > commit headers better.\n> \n> I think that's a reasonable assumption; this field comes straight from\n> the commit. It should already be well-formed, no?\n\nProbably; it was just something I noted while writing the patch, and I\nlike to be paranoid about assumptions. :)\n\n> > Or we could just ignore it. AFAICS, this doesn't actually violate\n> > rfc2047, nor rfc5322. The 78-character limit is simply a SHOULD, and\n> > we have up to 998 for MUST. For a single-address header[1], this seems\n> > kind of unlikely to me.\n> \n> True. But since the fix is as simple as it is, perhaps it's worth it\n> just for the clean conscience?\n\nFair enough. Patch to follow.\n\n> > [1] For multi-address headers like \"format-patch --cc=foo --cc=bar\", it\n> > looks like we already break them across lines.\n> \n> Yes, but this is even worse: these fields don't get encoded at all!\n\nUgh, you're right. That is a totally separate issue, and one I really\ndon't want to get into. Because it means we have to _parse_ those\nheaders and understand which part is a name and which is an address.\n\nPeople who use \"--cc\" or format.headers will have to deal with that\nthemselves. I consider both to be somewhat useless, since you can\npost-process the mbox after format-patch is run (or in your MUA).\nWhereas quoting and encoding fields in format-patch is necessary to give\nunambiguous input to the MUA (be it send-email or whatever).\n\n-Peff\n"},{"id":"165857","messageId":"20110414221809.GA19222@sigill.intra.peff.net","threadId":"27097","inReplyTo":"20110414214230.GB7709@sigill.intra.peff.net","subject":"Re: [BUG] format-patch does not wrap From-field after author name","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-04-14T22:18:09Z","receivedAt":"2011-04-14T22:18:09Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 14, 2011 at 05:42:30PM -0400, Jeff King wrote:\n\n> > > Or we could just ignore it. AFAICS, this doesn't actually violate\n> > > rfc2047, nor rfc5322. The 78-character limit is simply a SHOULD, and\n> > > we have up to 998 for MUST. For a single-address header[1], this seems\n> > > kind of unlikely to me.\n> > \n> > True. But since the fix is as simple as it is, perhaps it's worth it\n> > just for the clean conscience?\n> \n> Fair enough. Patch to follow.\n\nHere it is.\n\n-- >8 --\nSubject: [PATCH] format-patch: wrap email addresses after long names\n\nWe already wrap names in \"from\" headers, which tend to be\nthe long part of an address. But it's also possible for a\nlong name to not be wrapped, but to make us want to wrap the\nemail address. For example (imagine for the sake of\nreadability we want to wrap at 50 characters instead of 78):\n\n From: this is my really long git name \n\nThe name does not overflow the line, but the name and email\ntogether do. So we would rather see:\n\n From: this is my really long git name\n \n\nBecause we wrap the name separately during add_rfc2047, we\nneglected this case. Instead, we should see how long the\nfinal line of the wrapped name ended up, and decide whether\nor not to wrap based on that. We can't break the address\ninto multiple parts, so we either leave it with the name, or\nput it by itself on a line.\n\nTest by Erik Faye-Lund.\n\nSigned-off-by: Jeff King \n---\n pretty.c | 9 +++++++++\n t/t4014-format-patch.sh | 15 +++++++++++++++\n 2 files changed, 24 insertions(+), 0 deletions(-)\n\ndiff --git a/pretty.c b/pretty.c\nindex e1d8a8f..ba95de9 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -287,6 +287,7 @@ void pp_user_info(const char *what, enum cmit_fmt fmt, struct strbuf *sb,\n \tif (fmt == CMIT_FMT_EMAIL) {\n \t\tchar *name_tail = strchr(line, '<');\n \t\tint display_name_length;\n+\t\tint final_line;\n \t\tif (!name_tail)\n \t\t\treturn;\n \t\twhile (line < name_tail && isspace(name_tail[-1]))\n@@ -294,6 +295,14 @@ void pp_user_info(const char *what, enum cmit_fmt fmt, struct strbuf *sb,\n \t\tdisplay_name_length = name_tail - line;\n \t\tstrbuf_addstr(sb, \"From: \");\n \t\tadd_rfc2047(sb, line, display_name_length, encoding);\n+\t\tfor (final_line = 0; final_line < sb->len; final_line++)\n+\t\t\tif (sb->buf[sb->len - final_line - 1] == '\\n')\n+\t\t\t\tbreak;\n+\t\tif (namelen - display_name_length + final_line > 78) {\n+\t\t\tstrbuf_addch(sb, '\\n');\n+\t\t\tif (!isspace(name_tail[0]))\n+\t\t\t\tstrbuf_addch(sb, ' ');\n+\t\t}\n \t\tstrbuf_add(sb, name_tail, namelen - display_name_length);\n \t\tstrbuf_addch(sb, '\\n');\n \t} else {\ndiff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh\nindex c3cdb52..7d06716 100755\n--- a/t/t4014-format-patch.sh\n+++ b/t/t4014-format-patch.sh\n@@ -793,4 +793,19 @@ test_expect_success 'format-patch wraps extremely long headers (rfc2047)' '\n \ttest_cmp expect subject\n '\n \n+M8=\"foo_bar_\"\n+M64=$M8$M8$M8$M8$M8$M8$M8$M8\n+cat >expect <\n+EOF\n+test_expect_success 'format-patch wraps non-quotable headers' '\n+\trm -rf patches/ &&\n+\techo content >>file &&\n+\tgit add file &&\n+\tgit commit -mfoo --author \"$M64 \" &&\n+\tgit format-patch --stdout -1 >patch &&\n+\tsed -n \"/^From: /p; /^ /p; /^$/q\" from &&\n+\ttest_cmp expect from\n+'\n test_done\n-- \n1.7.5.rc1.3.ga78bf\n"},{"id":"165859","messageId":"BANLkTikgZH8135=o5ODcA=780-1D7YFngw@mail.gmail.com","threadId":"27097","inReplyTo":"20110414214230.GB7709@sigill.intra.peff.net","subject":"Re: [BUG] format-patch does not wrap From-field after author name","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2011-04-14T22:21:24Z","receivedAt":"2011-04-14T22:21:24Z","isPatch":false,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Thu, Apr 14, 2011 at 11:42 PM, Jeff King wrote:\n> On Thu, Apr 14, 2011 at 11:19:09PM +0200, Erik Faye-Lund wrote:\n>> > Or we could just ignore it. AFAICS, this doesn't actually violate\n>> > rfc2047, nor rfc5322. The 78-character limit is simply a SHOULD, and\n>> > we have up to 998 for MUST. For a single-address header[1], this seems\n>> > kind of unlikely to me.\n>>\n>> True. But since the fix is as simple as it is, perhaps it's worth it\n>> just for the clean conscience?\n>\n> Fair enough. Patch to follow.\n>\n\nThinking about it a bit more, I'm getting a bit more unsure:\n- The 78-limit is about user-interfaces, not protocol robustness.\n- Since send-email unwraps the line and does not re-wrap it, even if\nwe have a name like this it's likely that the work gets undone right\naway.\n- So that means that send-email should probably also be fixed. But now\nI'm wondering if we've crossed the point where this will just lead to\nless obvious code for very little gain.\n\nSo I think I might have over-though this.\n\n>> > [1] For multi-address headers like \"format-patch --cc=foo --cc=bar\", it\n>> > looks like we already break them across lines.\n>>\n>> Yes, but this is even worse: these fields don't get encoded at all!\n>\n> Ugh, you're right. That is a totally separate issue, and one I really\n> don't want to get into.\n\nIndeed. I have an itch around this area (I've been playing with\nporting send-email to C), so I might look at it at some point soon.\n\n> Because it means we have to _parse_ those\n> headers and understand which part is a name and which is an address.\n>\n\nThat part is surprisingly easy: If it contains a '<', then it's on the form\n\"Foo Bar Baz \". If not, it's \"foo@bar.baz\" (assuming it's\nUTF-8 encoded rfc5322 mailbox'es we assume, which would make the most\nsense to me)\n\n> People who use \"--cc\" or format.headers will have to deal with that\n> themselves. I consider both to be somewhat useless, since you can\n> post-process the mbox after format-patch is run (or in your MUA).\n> Whereas quoting and encoding fields in format-patch is necessary to give\n> unambiguous input to the MUA (be it send-email or whatever).\n>\n\nI agree. I'm actually a tad surprised we support it in the first place.\n"},{"id":"165861","messageId":"20110414222940.GA19389@sigill.intra.peff.net","threadId":"27097","inReplyTo":"BANLkTikgZH8135=o5ODcA=780-1D7YFngw@mail.gmail.com","subject":"Re: [BUG] format-patch does not wrap From-field after author name","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-04-14T22:29:40Z","receivedAt":"2011-04-14T22:29:40Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Apr 15, 2011 at 12:21:24AM +0200, Erik Faye-Lund wrote:\n\n> >> True. But since the fix is as simple as it is, perhaps it's worth it\n> >> just for the clean conscience?\n> >\n> > Fair enough. Patch to follow.\n> >\n> \n> Thinking about it a bit more, I'm getting a bit more unsure:\n> - The 78-limit is about user-interfaces, not protocol robustness.\n\nTrue. In theory we should also be limiting to avoid the 998-character\nhard protocol limit, but that is getting ridiculously unlikely.\n\n> - Since send-email unwraps the line and does not re-wrap it, even if\n> we have a name like this it's likely that the work gets undone right\n> away.\n\nNot everybody uses send-email. So you are also helping MUAs which\nconsume the output of format-patch.\n\nThat being said, I doubt that this will make a difference to anybody.\nThe real reason that we put wrapping into add_rfc2047 was for subjects,\nwhich _do_ get long.\n\n> - So that means that send-email should probably also be fixed. But now\n> I'm wondering if we've crossed the point where this will just lead to\n> less obvious code for very little gain.\n\nIt is ugly code.\n\nI'm just as happy if we drop it.\n\n> > Because it means we have to _parse_ those\n> > headers and understand which part is a name and which is an address.\n> \n> That part is surprisingly easy: If it contains a '<', then it's on the form\n> \"Foo Bar Baz \". If not, it's \"foo@bar.baz\" (assuming it's\n> UTF-8 encoded rfc5322 mailbox'es we assume, which would make the most\n> sense to me)\n\nWhat about:\n\n \"Foo \\\"The Bar\\\" Baz\" \n\nor\n\n Foo \"The Bar\" Baz \n\nor\n\n Foo (The Bar) Baz \n\nI.e., are we taking rfc822-style addresses, or are we taking something\nthat looks vaguely like an email address, and just treating everything\nleft of \"<\" as literal?\n\n-Peff\n"},{"id":"165862","messageId":"BANLkTim0Q+RaExss+U3m-=CHT6Jd1jc56Q@mail.gmail.com","threadId":"27097","inReplyTo":"20110414222940.GA19389@sigill.intra.peff.net","subject":"Re: [BUG] format-patch does not wrap From-field after author name","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2011-04-14T22:43:08Z","receivedAt":"2011-04-14T22:43:08Z","isPatch":false,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Fri, Apr 15, 2011 at 12:29 AM, Jeff King wrote:\n> On Fri, Apr 15, 2011 at 12:21:24AM +0200, Erik Faye-Lund wrote:\n>\n>> >> True. But since the fix is as simple as it is, perhaps it's worth it\n>> >> just for the clean conscience?\n>> >\n>> > Fair enough. Patch to follow.\n>> >\n>>\n>> Thinking about it a bit more, I'm getting a bit more unsure:\n>> - The 78-limit is about user-interfaces, not protocol robustness.\n>\n> True. In theory we should also be limiting to avoid the 998-character\n> hard protocol limit, but that is getting ridiculously unlikely.\n\nI think that's over in my definition of \"insanity land\", yeah :)\n\n>> - Since send-email unwraps the line and does not re-wrap it, even if\n>> we have a name like this it's likely that the work gets undone right\n>> away.\n>\n> Not everybody uses send-email. So you are also helping MUAs which\n> consume the output of format-patch.\n\nGood point.\n\n> That being said, I doubt that this will make a difference to anybody.\n> The real reason that we put wrapping into add_rfc2047 was for subjects,\n> which _do_ get long.\n\nAbsolutely.\n\n>> - So that means that send-email should probably also be fixed. But now\n>> I'm wondering if we've crossed the point where this will just lead to\n>> less obvious code for very little gain.\n>\n> It is ugly code.\n>\n> I'm just as happy if we drop it.\n\nOK, then I'll try to forget about this issue for now. Sorry for troubling you.\n\n>> > Because it means we have to _parse_ those\n>> > headers and understand which part is a name and which is an address.\n>>\n>> That part is surprisingly easy: If it contains a '<', then it's on the form\n>> \"Foo Bar Baz \". If not, it's \"foo@bar.baz\" (assuming it's\n>> UTF-8 encoded rfc5322 mailbox'es we assume, which would make the most\n>> sense to me)\n>\n> What about:\n>\n>  \"Foo \\\"The Bar\\\" Baz\" \n>\n> or\n>\n>  Foo \"The Bar\" Baz \n>\n> or\n>\n>  Foo (The Bar) Baz \n>\n> I.e., are we taking rfc822-style addresses, or are we taking something\n> that looks vaguely like an email address, and just treating everything\n> left of \"<\" as literal?\n\nI was just thinking of interpreting everything left of '<' literally\nand encode it (if needed). Currently, we interpret the entire string\nliterally, encoding the name would an improvement.\n"},{"id":"165876","messageId":"20110415033003.GB19621@sigill.intra.peff.net","threadId":"27097","inReplyTo":"BANLkTim0Q+RaExss+U3m-=CHT6Jd1jc56Q@mail.gmail.com","subject":"Re: [BUG] format-patch does not wrap From-field after author name","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-04-15T03:30:03Z","receivedAt":"2011-04-15T03:30:03Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Apr 15, 2011 at 12:43:08AM +0200, Erik Faye-Lund wrote:\n\n> >> That part is surprisingly easy: If it contains a '<', then it's on the form\n> >> \"Foo Bar Baz \". If not, it's \"foo@bar.baz\" (assuming it's\n> >> UTF-8 encoded rfc5322 mailbox'es we assume, which would make the most\n> >> sense to me)\n> >\n> > What about:\n> >\n> >  \"Foo \\\"The Bar\\\" Baz\" \n> >\n> > or\n> >\n> >  Foo \"The Bar\" Baz \n> >\n> > or\n> >\n> >  Foo (The Bar) Baz \n> >\n> > I.e., are we taking rfc822-style addresses, or are we taking something\n> > that looks vaguely like an email address, and just treating everything\n> > left of \"<\" as literal?\n> \n> I was just thinking of interpreting everything left of '<' literally\n> and encode it (if needed). Currently, we interpret the entire string\n> literally, encoding the name would an improvement.\n\nWon't that be a regression for people who already know that we take\nthings literally and are manually quoting and/or rfc2047-encoding the\ncontents?\n\n-Peff\n"},{"id":"165887","messageId":"BANLkTinifkWJJCNGYo1gHKJGMVmRX7QfmA@mail.gmail.com","threadId":"27097","inReplyTo":"20110415033003.GB19621@sigill.intra.peff.net","subject":"Re: [BUG] format-patch does not wrap From-field after author name","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2011-04-15T08:32:27Z","receivedAt":"2011-04-15T08:32:27Z","isPatch":false,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Fri, Apr 15, 2011 at 5:30 AM, Jeff King wrote:\n> On Fri, Apr 15, 2011 at 12:43:08AM +0200, Erik Faye-Lund wrote:\n>\n>> >> That part is surprisingly easy: If it contains a '<', then it's on the form\n>> >> \"Foo Bar Baz \". If not, it's \"foo@bar.baz\" (assuming it's\n>> >> UTF-8 encoded rfc5322 mailbox'es we assume, which would make the most\n>> >> sense to me)\n>> >\n>> > What about:\n>> >\n>> >  \"Foo \\\"The Bar\\\" Baz\" \n>> >\n>> > or\n>> >\n>> >  Foo \"The Bar\" Baz \n>> >\n>> > or\n>> >\n>> >  Foo (The Bar) Baz \n>> >\n>> > I.e., are we taking rfc822-style addresses, or are we taking something\n>> > that looks vaguely like an email address, and just treating everything\n>> > left of \"<\" as literal?\n>>\n>> I was just thinking of interpreting everything left of '<' literally\n>> and encode it (if needed). Currently, we interpret the entire string\n>> literally, encoding the name would an improvement.\n>\n> Won't that be a regression for people who already know that we take\n> things literally and are manually quoting and/or rfc2047-encoding the\n> contents?\n\nYes. But won't that always be the case when someone depends on buggy behavior?\n\nBesides, send-email takes interprets it's --to and --cc arguments as\nwell as sendemail.to and sendemail.cc config options literally (i.e\nquoting if needed without any attempts on unquoting first). IMO having\ntwo closely related programs with similar options that behave\ndifferent in border-cases is pretty ugly. ESPECIALLY when one of them\nhas a habit of forwarding unknown options to the other, like\nsend-email does...\n"},{"id":"165944","messageId":"20110416014525.GA23306@sigill.intra.peff.net","threadId":"27097","inReplyTo":"BANLkTinifkWJJCNGYo1gHKJGMVmRX7QfmA@mail.gmail.com","subject":"Re: [BUG] format-patch does not wrap From-field after author name","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-04-16T01:45:25Z","receivedAt":"2011-04-16T01:45:25Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Apr 15, 2011 at 10:32:27AM +0200, Erik Faye-Lund wrote:\n\n> >> I was just thinking of interpreting everything left of '<' literally\n> >> and encode it (if needed). Currently, we interpret the entire string\n> >> literally, encoding the name would an improvement.\n> >\n> > Won't that be a regression for people who already know that we take\n> > things literally and are manually quoting and/or rfc2047-encoding the\n> > contents?\n> \n> Yes. But won't that always be the case when someone depends on buggy behavior?\n\nI guess I don't see the current behavior as necessarily buggy, just\nsub-optimal. I can imagine people have worked around it by embedding\nrfc2047-encoded content manually.\n\nBut I admit I don't really care that much, and I don't know what common\nuse is. Grepping the list archives didn't turn up anything useful.\n\n> Besides, send-email takes interprets it's --to and --cc arguments as\n> well as sendemail.to and sendemail.cc config options literally (i.e\n> quoting if needed without any attempts on unquoting first). IMO having\n> two closely related programs with similar options that behave\n> different in border-cases is pretty ugly. ESPECIALLY when one of them\n> has a habit of forwarding unknown options to the other, like\n> send-email does...\n\nYeah, it would be nice to resolve that inconsistency.\n\n-Peff\n"}]}