{"thread":{"id":"46655","subject":"git send-email Cc with cruft not working as expected","startedAt":"2017-08-22T23:16:21Z","lastAt":"2017-08-25T09:12:28Z","messageCount":13,"participants":["Jacob Keller","Stefan Beller","Matthieu Moy","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"326990","messageId":"CA+P7+xrtZYUjPcVMkA+x8B57w+LxjjU8YSKcE77DrWne7449rg@mail.gmail.com","threadId":"46655","inReplyTo":null,"subject":"git send-email Cc with cruft not working as expected","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2017-08-22T23:15:54Z","receivedAt":"2017-08-22T23:16:21Z","isPatch":false,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"Hi,\n\nI recently found an issue with git-send-email where it does not\nproperly remove the cruft of an email address when sending using a Cc:\nline.\n\nThe specific example is with a commit containing the following Cc line,\n\nCc: stable@vger.kernel.org # 4.10+\n\nwhich is the standard way Linux upstream expects the stable Ccs to be,\nand I saw several examples of this in the past.\n\nHowever, this gets converted into a cc of\n\"stable@vger.kernel.org#4.10+\" which isn't a valid address obviously.\n\nThis does work as expected if you remember to\n\nCc: # 4.10+\n\nI would have assumed that validate_address would kick in and let me\nknow that the address I'd given isn't valid, or something along those\nlines.\n\nI tried to come up with a test for this, but modifying t9001 seemed to\ncause other failures and I couldn't detangle exactly how the tests fit\ntogether.\n\nIs this simply expected behavior and I need to remember to use <>\naround the address?\n\nThanks,\nJake\n"},{"id":"326991","messageId":"CAGZ79kZW_+GEKyP4+8agZ7nyjGEZ9p5d3N99W6sC3GTY_4Cm-g@mail.gmail.com","threadId":"46655","inReplyTo":"CA+P7+xrtZYUjPcVMkA+x8B57w+LxjjU8YSKcE77DrWne7449rg@mail.gmail.com","subject":"Re: git send-email Cc with cruft not working as expected","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-08-22T23:18:46Z","receivedAt":"2017-08-22T23:18:52Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Aug 22, 2017 at 4:15 PM, Jacob Keller wrote:\n> Hi,\n>\n> I recently found an issue with git-send-email where it does not\n> properly remove the cruft of an email address when sending using a Cc:\n> line.\n>\n> The specific example is with a commit containing the following Cc line,\n>\n> Cc: stable@vger.kernel.org # 4.10+\n\nPlease see and discuss at\nhttps://public-inbox.org/git/20170216174924.GB2625@localhost/\n"},{"id":"326992","messageId":"CA+P7+xpCJ8jwBQp9Ze=J955CaxnbVPc69ThXht2e=6TUMBq_UQ@mail.gmail.com","threadId":"46655","inReplyTo":"CAGZ79kZW_+GEKyP4+8agZ7nyjGEZ9p5d3N99W6sC3GTY_4Cm-g@mail.gmail.com","subject":"Re: git send-email Cc with cruft not working as expected","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2017-08-22T23:30:44Z","receivedAt":"2017-08-22T23:31:11Z","isPatch":false,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Tue, Aug 22, 2017 at 4:18 PM, Stefan Beller wrote:\n> On Tue, Aug 22, 2017 at 4:15 PM, Jacob Keller wrote:\n>> Hi,\n>>\n>> I recently found an issue with git-send-email where it does not\n>> properly remove the cruft of an email address when sending using a Cc:\n>> line.\n>>\n>> The specific example is with a commit containing the following Cc line,\n>>\n>> Cc: stable@vger.kernel.org # 4.10+\n>\n> Please see and discuss at\n> https://public-inbox.org/git/20170216174924.GB2625@localhost/\n\nI read that thread, and it addressed the problem of\n\nCc: # 4.10+\n\nbut did not fix this case without the <> around the email address.\n\nAdditionally I just discovered that the behavior here changes pretty\ndrastically if you have Email::Validate installed, now it splits the\naddress into multiple things:\n\nstable@vger.kernel.org, #, 4.10+\n\nThanks,\nJake\n"},{"id":"326993","messageId":"CAGZ79kbYWJmru_o48+8iH4_MVEtODFuicRY=23+BM+_q2ZJsaw@mail.gmail.com","threadId":"46655","inReplyTo":"CA+P7+xpCJ8jwBQp9Ze=J955CaxnbVPc69ThXht2e=6TUMBq_UQ@mail.gmail.com","subject":"Re: git send-email Cc with cruft not working as expected","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-08-22T23:36:54Z","receivedAt":"2017-08-22T23:37:00Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"+cc people from that thread\n\nOn Tue, Aug 22, 2017 at 4:30 PM, Jacob Keller wrote:\n> On Tue, Aug 22, 2017 at 4:18 PM, Stefan Beller wrote:\n>> On Tue, Aug 22, 2017 at 4:15 PM, Jacob Keller wrote:\n>>> Hi,\n>>>\n>>> I recently found an issue with git-send-email where it does not\n>>> properly remove the cruft of an email address when sending using a Cc:\n>>> line.\n>>>\n>>> The specific example is with a commit containing the following Cc line,\n>>>\n>>> Cc: stable@vger.kernel.org # 4.10+\n>>\n>> Please see and discuss at\n>> https://public-inbox.org/git/20170216174924.GB2625@localhost/\n>\n> I read that thread, and it addressed the problem of\n>\n> Cc: # 4.10+\n>\n> but did not fix this case without the <> around the email address.\n>\n> Additionally I just discovered that the behavior here changes pretty\n> drastically if you have Email::Validate installed, now it splits the\n> address into multiple things:\n>\n> stable@vger.kernel.org, #, 4.10+\n>\n> Thanks,\n> Jake\n"},{"id":"327015","messageId":"vpqo9r6lhzq.fsf@anie.imag.fr","threadId":"46655","inReplyTo":"CAGZ79kbYWJmru_o48+8iH4_MVEtODFuicRY=23+BM+_q2ZJsaw@mail.gmail.com","subject":"Re: git send-email Cc with cruft not working as expected","fromName":"Matthieu Moy","fromEmail":"git@matthieu-moy.fr","sentAt":"2017-08-23T10:02:17Z","receivedAt":"2017-08-23T10:09:15Z","isPatch":false,"sender":{"key":"git@matthieu-moy.fr","avatar":"https://avatars.githubusercontent.com/u/14709?v=4"},"body":"Stefan Beller writes:\n\n> +cc people from that thread\n>\n> On Tue, Aug 22, 2017 at 4:30 PM, Jacob Keller wrote:\n>> On Tue, Aug 22, 2017 at 4:18 PM, Stefan Beller wrote:\n>>> On Tue, Aug 22, 2017 at 4:15 PM, Jacob Keller wrote:\n>>>> Hi,\n>>>>\n>>>> I recently found an issue with git-send-email where it does not\n>>>> properly remove the cruft of an email address when sending using a Cc:\n>>>> line.\n>>>>\n>>>> The specific example is with a commit containing the following Cc line,\n>>>>\n>>>> Cc: stable@vger.kernel.org # 4.10+\n>>>\n>>> Please see and discuss at\n>>> https://public-inbox.org/git/20170216174924.GB2625@localhost/\n>>\n>> I read that thread, and it addressed the problem of\n>>\n>> Cc: # 4.10+\n>>\n>> but did not fix this case without the <> around the email address.\n\nIndeed. It detects garbage as \"everything after >\".\n\nI feel really sorry that we need so many iterations to get back to a\ncorrect behavior :-(.\n\n>> Additionally I just discovered that the behavior here changes pretty\n>> drastically if you have Email::Validate installed, now it splits the\n>> address into multiple things:\n\n(I'm assuming you mean Email::Address, there's also Email::Valid but I\ndon't think it would modify the behavior)\n\nHmm, I think we reached the point where we should just stop using\nEmail::Address.\n\nPatch series follows and should address both points.\n\n-- \nMatthieu Moy\nhttp://matthieu-moy.fr\n"},{"id":"327016","messageId":"20170823102102.20120-1-git@matthieu-moy.fr","threadId":"46655","inReplyTo":"vpqo9r6lhzq.fsf@anie.imag.fr","subject":"[RFC PATCH 1/2] send-email: fix garbage removal after address","fromName":"Matthieu Moy","fromEmail":"git@matthieu-moy.fr","sentAt":"2017-08-23T10:21:01Z","receivedAt":"2017-08-23T10:29:10Z","isPatch":true,"sender":{"key":"git@matthieu-moy.fr","avatar":"https://avatars.githubusercontent.com/u/14709?v=4"},"body":"This is a followup over 9d33439 (send-email: only allow one address\nper body tag, 2017-02-20). The first iteration did allow writting\n\n Cc: # garbage\n\nbut did so by matching the regex ([^>]*>?), i.e. stop after the first\ninstance of '>'. However, it did not properly deal with\n\n Cc: foo@example.com # garbage\n\nFix this using a new function strip_garbage_one_address, which does\nessentially what the old ([^>]*>?) was doing, but dealing with more\ncorner-cases. Since we've allowed\n\n Cc: \"Foo # Bar\" \n\nin previous versions, it makes sense to continue allowing it (but we\nstill remove any garbage after it). OTOH, when an address is given\nwithout quoting, we just take the first word and ignore everything\nafter.\n\nSigned-off-by: Matthieu Moy \n---\nAlso available as: https://github.com/git/git/pull/398\n\n git-send-email.perl | 26 ++++++++++++++++++++++++--\n t/t9001-send-email.sh | 4 ++++\n 2 files changed, 28 insertions(+), 2 deletions(-)\n\ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex fa6526986e..33a69ffe5d 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -1089,6 +1089,26 @@ sub sanitize_address {\n \n }\n \n+sub strip_garbage_one_address {\n+\tmy ($addr) = @_;\n+\tchomp $addr;\n+\tif ($addr =~ /^((\"[^\"]*\"|[^\"<]*)? *<[^>]*>).*/) {\n+\t\t# \"Foo Bar\" [possibly garbage here]\n+\t\t# Foo Bar [possibly garbage here]\n+\t\treturn $1;\n+\t}\n+\tif ($addr =~ /^(<[^>]*>).*/) {\n+\t\t# [possibly garbage here]\n+\t\t# if garbage contains other addresses, they are ignored.\n+\t\treturn $1;\n+\t}\n+\tif ($addr =~ /^([^\"#,\\s]*)/) {\n+\t\t# address without quoting: remove anything after the address\n+\t\treturn $1;\n+\t}\n+\treturn $addr;\n+}\n+\n sub sanitize_address_list {\n \treturn (map { sanitize_address($_) } @_);\n }\n@@ -1590,10 +1610,12 @@ foreach my $t (@files) {\n \t# Now parse the message body\n \twhile(<$fh>) {\n \t\t$message .= $_;\n-\t\tif (/^(Signed-off-by|Cc): ([^>]*>?)/i) {\n+\t\tif (/^(Signed-off-by|Cc): (.*)/i) {\n \t\t\tchomp;\n \t\t\tmy ($what, $c) = ($1, $2);\n-\t\t\tchomp $c;\n+\t\t\t# strip garbage for the address we'll use:\n+\t\t\t$c = strip_garbage_one_address($c);\n+\t\t\t# sanitize a bit more to decide whether to suppress the address:\n \t\t\tmy $sc = sanitize_address($c);\n \t\t\tif ($sc eq $sender) {\n \t\t\t\tnext if ($suppress_cc{'self'});\ndiff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh\nindex d1e4e8ad19..f30980895c 100755\n--- a/t/t9001-send-email.sh\n+++ b/t/t9001-send-email.sh\n@@ -148,6 +148,8 @@ cat >expected-cc <<\\EOF\n !two@example.com!\n !three@example.com!\n !four@example.com!\n+!five@example.com!\n+!six@example.com!\n EOF\n \"\n \n@@ -161,6 +163,8 @@ test_expect_success $PREREQ 'cc trailer with various syntax' '\n \tCc: # trailing comments are ignored\n \tCc: , one address per line\n \tCc: \"Some # Body\" [ ]\n+\tCc: five@example.com # not.six@example.com\n+\tCc: six@example.com, not.seven@example.com\n \tEOF\n \tclean_fake_sendmail &&\n \tgit send-email -1 --to=recipient@example.com \\\n-- \n2.14.0.rc0.dirty\n\n"},{"id":"327017","messageId":"20170823102102.20120-2-git@matthieu-moy.fr","threadId":"46655","inReplyTo":"20170823102102.20120-1-git@matthieu-moy.fr","subject":"[RFC PATCH 2/2] send-email: don't use Mail::Address, even if available","fromName":"Matthieu Moy","fromEmail":"git@matthieu-moy.fr","sentAt":"2017-08-23T10:21:02Z","receivedAt":"2017-08-23T10:29:12Z","isPatch":true,"sender":{"key":"git@matthieu-moy.fr","avatar":"https://avatars.githubusercontent.com/u/14709?v=4"},"body":"Using Mail::Address made sense when we didn't have a proper parser. We\nnow have a reasonable address parser, and using Mail::Address\n_if available_ causes much more trouble than it gives benefits:\n\n* Developers typically test one version, not both.\n\n* Users may not be aware that installing Mail::Address will change the\n behavior. They may complain about the behavior in one case without\n knowing that Mail::Address is involved.\n\n* Having this optional Mail::Address makes it tempting to anwser \"please\n install Mail::Address\" to users instead of fixing our own code. We've\n reached the stage where bugs in our parser should be fixed, not worked\n around.\n\nSigned-off-by: Matthieu Moy \n---\n git-send-email.perl | 7 +------\n 1 file changed, 1 insertion(+), 6 deletions(-)\n\ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex 33a69ffe5d..2208dcc213 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -155,7 +155,6 @@ sub format_2822_time {\n }\n \n my $have_email_valid = eval { require Email::Valid; 1 };\n-my $have_mail_address = eval { require Mail::Address; 1 };\n my $smtp;\n my $auth;\n my $num_sent = 0;\n@@ -490,11 +489,7 @@ my ($repoauthor, $repocommitter);\n ($repocommitter) = Git::ident_person(@repo, 'committer');\n \n sub parse_address_line {\n-\tif ($have_mail_address) {\n-\t\treturn map { $_->format } Mail::Address->parse($_[0]);\n-\t} else {\n-\t\treturn Git::parse_mailboxes($_[0]);\n-\t}\n+\treturn Git::parse_mailboxes($_[0]);\n }\n \n sub split_addrs {\n-- \n2.14.0.rc0.dirty\n\n"},{"id":"327096","messageId":"CA+P7+xrUgC9M9VeZTKL4K=ro23arrWHL+F5YUUTj_ZU8t24kkg@mail.gmail.com","threadId":"46655","inReplyTo":"20170823102102.20120-1-git@matthieu-moy.fr","subject":"Re: [RFC PATCH 1/2] send-email: fix garbage removal after address","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2017-08-23T21:59:40Z","receivedAt":"2017-08-23T22:00:07Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Wed, Aug 23, 2017 at 3:21 AM, Matthieu Moy wrote:\n> This is a followup over 9d33439 (send-email: only allow one address\n> per body tag, 2017-02-20). The first iteration did allow writting\n>\n> Cc: # garbage\n>\n> but did so by matching the regex ([^>]*>?), i.e. stop after the first\n> instance of '>'. However, it did not properly deal with\n>\n> Cc: foo@example.com # garbage\n>\n> Fix this using a new function strip_garbage_one_address, which does\n> essentially what the old ([^>]*>?) was doing, but dealing with more\n> corner-cases. Since we've allowed\n>\n> Cc: \"Foo # Bar\" \n>\n> in previous versions, it makes sense to continue allowing it (but we\n> still remove any garbage after it). OTOH, when an address is given\n> without quoting, we just take the first word and ignore everything\n> after.\n>\n> Signed-off-by: Matthieu Moy \n> ---\n\nI pulled this and tested it for my issue, and it fixes the problem for\nme. I think the approach in the code was solid too, extracting out the\nlogic helps make the code more clear.\n\nThanks,\nJake\n"},{"id":"327102","messageId":"CA+P7+xp7Gr_zvsreXpLLkn7GsR2fh-0W-20ov4QonQ7c292utA@mail.gmail.com","threadId":"46655","inReplyTo":"vpqo9r6lhzq.fsf@anie.imag.fr","subject":"Re: git send-email Cc with cruft not working as expected","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2017-08-23T22:49:12Z","receivedAt":"2017-08-23T22:49:38Z","isPatch":false,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Wed, Aug 23, 2017 at 3:02 AM, Matthieu Moy wrote:\n>> On Tue, Aug 22, 2017 at 4:30 PM, Jacob Keller wrote:\n>>> Additionally I just discovered that the behavior here changes pretty\n>>> drastically if you have Email::Validate installed, now it splits the\n>>> address into multiple things:\n>\n> (I'm assuming you mean Email::Address, there's also Email::Valid but I\n> don't think it would modify the behavior)\n>\n\nNo I actually definitely meant Email::Valid. I already had\nMail::Address installed, and I then installed Email::Valid, and it\nchanged behavior to split the cruft into multiple addresses.\n\nI don't actually know why or how it did this, but I'm certain it was\npresence of Email::Valid that did it.\n\nHowever, your first patch addresses the issue since you remove the\ncruft well before passing it into Email::Valid anyways.\n\n> Hmm, I think we reached the point where we should just stop using\n> Email::Address.\n\nI do agree, I don't think we should use Mail::Address.\n\n>\n> Patch series follows and should address both points.\n>\n> --\n> Matthieu Moy\n> http://matthieu-moy.fr\n"},{"id":"327159","messageId":"xmqqk21svh9o.fsf@gitster.mtv.corp.google.com","threadId":"46655","inReplyTo":"20170823102102.20120-1-git@matthieu-moy.fr","subject":"Re: [RFC PATCH 1/2] send-email: fix garbage removal after address","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-08-24T20:32:19Z","receivedAt":"2017-08-24T20:32:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matthieu Moy writes:\n\n> This is a followup over 9d33439 (send-email: only allow one address\n> per body tag, 2017-02-20). The first iteration did allow writting\n>\n> Cc: # garbage\n>\n> but did so by matching the regex ([^>]*>?), i.e. stop after the first\n> instance of '>'. However, it did not properly deal with\n>\n> Cc: foo@example.com # garbage\n>\n> Fix this using a new function strip_garbage_one_address, which does\n> essentially what the old ([^>]*>?) was doing, but dealing with more\n> corner-cases. Since we've allowed\n>\n> Cc: \"Foo # Bar\" \n>\n> in previous versions, it makes sense to continue allowing it (but we\n> still remove any garbage after it). OTOH, when an address is given\n> without quoting, we just take the first word and ignore everything\n> after.\n>\n> Signed-off-by: Matthieu Moy \n> ---\n> Also available as: https://github.com/git/git/pull/398\n>\n> git-send-email.perl | 26 ++++++++++++++++++++++++--\n> t/t9001-send-email.sh | 4 ++++\n> 2 files changed, 28 insertions(+), 2 deletions(-)\n>\n> diff --git a/git-send-email.perl b/git-send-email.perl\n> index fa6526986e..33a69ffe5d 100755\n> --- a/git-send-email.perl\n> +++ b/git-send-email.perl\n> @@ -1089,6 +1089,26 @@ sub sanitize_address {\n> \n> }\n> \n> +sub strip_garbage_one_address {\n> +\tmy ($addr) = @_;\n> +\tchomp $addr;\n> +\tif ($addr =~ /^((\"[^\"]*\"|[^\"<]*)? *<[^>]*>).*/) {\n> +\t\t# \"Foo Bar\" [possibly garbage here]\n> +\t\t# Foo Bar [possibly garbage here]\n> +\t\treturn $1;\n> +\t}\n> +\tif ($addr =~ /^(<[^>]*>).*/) {\n> +\t\t# [possibly garbage here]\n> +\t\t# if garbage contains other addresses, they are ignored.\n> +\t\treturn $1;\n> +\t}\n\nIsn't this already covered by the first one, which allows an\noptional \"something\", followed by an optional run of SPs, in front\nof this exact pattern, so the case where the optional \"something\"\ndoes not appear and the number of optional SP is zero would exactly\nmatch the one this pattern is meant to cover.\n\n> +\tif ($addr =~ /^([^\"#,\\s]*)/) {\n> +\t\t# address without quoting: remove anything after the address\n> +\t\treturn $1;\n> +\t}\n> +\treturn $addr;\n> +}\n\nBy the way, these three regexps smell like they were written\nspecifically to cover three cases you care about (perhaps the ones\nin your proposed log message), but what will be our response when\nsomebody else comes next time to us and says that their favourite\nformatting of \"Cc:\" line is not covered by these rules? \n\nWill we add yet another pattern? Where will it end? There will be\na point where we instead start telling them to update the convention\nof their project so that it will be covered by one of the patterns\nwe have already developed, I would imagine.\n\nSo, from that point of view, I, with devil's advocate hat on, wonder\nwhy we are not saying\n\n\t\"Cc: s@k.org # cruft\"? Use \"Cc: # cruft\" instead\n\tand you'd be fine.\n\nright now, without this patch.\n\nI do not _mind_ us trying to be extra nice for a while, and I\ncertainly do not mind _this_ particular patch that gives us a single\nhelper function that future \"here is another way to spell cruft\"\nrules can go, but I feel that there should be some line that lets us\nsay that we've done enough.\n\nThanks.\n"},{"id":"327176","messageId":"vpq378g107m.fsf@anie.imag.fr","threadId":"46655","inReplyTo":"xmqqk21svh9o.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC PATCH 1/2] send-email: fix garbage removal after address","fromName":"Matthieu Moy","fromEmail":"git@matthieu-moy.fr","sentAt":"2017-08-25T09:11:09Z","receivedAt":"2017-08-25T09:11:15Z","isPatch":true,"sender":{"key":"git@matthieu-moy.fr","avatar":"https://avatars.githubusercontent.com/u/14709?v=4"},"body":"Junio C Hamano writes:\n\n> Matthieu Moy writes:\n>\n>> +sub strip_garbage_one_address {\n>> +\tmy ($addr) = @_;\n>> +\tchomp $addr;\n>> +\tif ($addr =~ /^((\"[^\"]*\"|[^\"<]*)? *<[^>]*>).*/) {\n>> +\t\t# \"Foo Bar\" [possibly garbage here]\n>> +\t\t# Foo Bar [possibly garbage here]\n>> +\t\treturn $1;\n>> +\t}\n>> +\tif ($addr =~ /^(<[^>]*>).*/) {\n>> +\t\t# [possibly garbage here]\n>> +\t\t# if garbage contains other addresses, they are ignored.\n>> +\t\treturn $1;\n>> +\t}\n>\n> Isn't this already covered by the first one,\n\nOops, indeed. I just removed the second \"if\" (and added the appropriate\ncomment to the first):\n\n+ if ($addr =~ /^((\"[^\"]*\"|[^\"<]*)? *<[^>]*>).*/) {\n+ # \"Foo Bar\" [possibly garbage here]\n+ # Foo Bar [possibly garbage here]\n+ # [possibly garbage here]\n+ return $1;\n+ }\n\n> By the way, these three regexps smell like they were written\n> specifically to cover three cases you care about (perhaps the ones\n> in your proposed log message), but what will be our response when\n> somebody else comes next time to us and says that their favourite\n> formatting of \"Cc:\" line is not covered by these rules?\n\nWell, actually the last one covers essentially everything. Just stop at\nthe first space, #, ',' or '\"'. The first case is here to allow putting\na name in front of the address, which is something we've already allowed\nand sounds reasonable from the user point of view.\n\nOTOH, I didn't bother with real corner-cases like\n\n Cc: \"Foo \\\"bar\\\"\" \n\n> So, from that point of view, I, with devil's advocate hat on, wonder\n> why we are not saying\n>\n> \t\"Cc: s@k.org # cruft\"? Use \"Cc: # cruft\" instead\n> \tand you'd be fine.\n>\n> right now, without this patch.\n\nI would agree if the broken case were an exotic one. But a plain adress\nis really the simplest use-case I can think of, so it's hard to say\n\"don't do that\" when we should say \"sorry, we should obviously have\nthought about this use-case\".\n\n-- \nMatthieu Moy\nhttps://matthieu-moy.fr/\n"},{"id":"327177","messageId":"20170825091200.13358-1-git@matthieu-moy.fr","threadId":"46655","inReplyTo":"vpq378g107m.fsf@anie.imag.fr","subject":"[PATCH v2 1/2] send-email: fix garbage removal after address","fromName":"Matthieu Moy","fromEmail":"git@matthieu-moy.fr","sentAt":"2017-08-25T09:11:59Z","receivedAt":"2017-08-25T09:12:21Z","isPatch":true,"sender":{"key":"git@matthieu-moy.fr","avatar":"https://avatars.githubusercontent.com/u/14709?v=4"},"body":"This is a followup over 9d33439 (send-email: only allow one address\nper body tag, 2017-02-20). The first iteration did allow writting\n\n Cc: # garbage\n\nbut did so by matching the regex ([^>]*>?), i.e. stop after the first\ninstance of '>'. However, it did not properly deal with\n\n Cc: foo@example.com # garbage\n\nFix this using a new function strip_garbage_one_address, which does\nessentially what the old ([^>]*>?) was doing, but dealing with more\ncorner-cases. Since we've allowed\n\n Cc: \"Foo # Bar\" \n\nin previous versions, it makes sense to continue allowing it (but we\nstill remove any garbage after it). OTOH, when an address is given\nwithout quoting, we just take the first word and ignore everything\nafter.\n\nSigned-off-by: Matthieu Moy \n---\nChange since v1: removed dead code as suggested by Junio.\n\n\n git-send-email.perl | 22 ++++++++++++++++++++--\n t/t9001-send-email.sh | 4 ++++\n 2 files changed, 24 insertions(+), 2 deletions(-)\n\ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex fa6526986e..dfd646ac5b 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -1089,6 +1089,22 @@ sub sanitize_address {\n \n }\n \n+sub strip_garbage_one_address {\n+\tmy ($addr) = @_;\n+\tchomp $addr;\n+\tif ($addr =~ /^((\"[^\"]*\"|[^\"<]*)? *<[^>]*>).*/) {\n+\t\t# \"Foo Bar\" [possibly garbage here]\n+\t\t# Foo Bar [possibly garbage here]\n+\t\t# [possibly garbage here]\n+\t\treturn $1;\n+\t}\n+\tif ($addr =~ /^([^\"#,\\s]*)/) {\n+\t\t# address without quoting: remove anything after the address\n+\t\treturn $1;\n+\t}\n+\treturn $addr;\n+}\n+\n sub sanitize_address_list {\n \treturn (map { sanitize_address($_) } @_);\n }\n@@ -1590,10 +1606,12 @@ foreach my $t (@files) {\n \t# Now parse the message body\n \twhile(<$fh>) {\n \t\t$message .= $_;\n-\t\tif (/^(Signed-off-by|Cc): ([^>]*>?)/i) {\n+\t\tif (/^(Signed-off-by|Cc): (.*)/i) {\n \t\t\tchomp;\n \t\t\tmy ($what, $c) = ($1, $2);\n-\t\t\tchomp $c;\n+\t\t\t# strip garbage for the address we'll use:\n+\t\t\t$c = strip_garbage_one_address($c);\n+\t\t\t# sanitize a bit more to decide whether to suppress the address:\n \t\t\tmy $sc = sanitize_address($c);\n \t\t\tif ($sc eq $sender) {\n \t\t\t\tnext if ($suppress_cc{'self'});\ndiff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh\nindex d1e4e8ad19..f30980895c 100755\n--- a/t/t9001-send-email.sh\n+++ b/t/t9001-send-email.sh\n@@ -148,6 +148,8 @@ cat >expected-cc <<\\EOF\n !two@example.com!\n !three@example.com!\n !four@example.com!\n+!five@example.com!\n+!six@example.com!\n EOF\n \"\n \n@@ -161,6 +163,8 @@ test_expect_success $PREREQ 'cc trailer with various syntax' '\n \tCc: # trailing comments are ignored\n \tCc: , one address per line\n \tCc: \"Some # Body\" [ ]\n+\tCc: five@example.com # not.six@example.com\n+\tCc: six@example.com, not.seven@example.com\n \tEOF\n \tclean_fake_sendmail &&\n \tgit send-email -1 --to=recipient@example.com \\\n-- \n2.14.0.rc0.dirty\n\n"},{"id":"327178","messageId":"20170825091200.13358-2-git@matthieu-moy.fr","threadId":"46655","inReplyTo":"20170825091200.13358-1-git@matthieu-moy.fr","subject":"[PATCH v2 2/2] send-email: don't use Mail::Address, even if available","fromName":"Matthieu Moy","fromEmail":"git@matthieu-moy.fr","sentAt":"2017-08-25T09:12:00Z","receivedAt":"2017-08-25T09:12:28Z","isPatch":true,"sender":{"key":"git@matthieu-moy.fr","avatar":"https://avatars.githubusercontent.com/u/14709?v=4"},"body":"Using Mail::Address made sense when we didn't have a proper parser. We\nnow have a reasonable address parser, and using Mail::Address\n_if available_ causes much more trouble than it gives benefits:\n\n* Developers typically test one version, not both.\n\n* Users may not be aware that installing Mail::Address will change the\n behavior. They may complain about the behavior in one case without\n knowing that Mail::Address is involved.\n\n* Having this optional Mail::Address makes it tempting to anwser \"please\n install Mail::Address\" to users instead of fixing our own code. We've\n reached the stage where bugs in our parser should be fixed, not worked\n around.\n\nSigned-off-by: Matthieu Moy \n---\n git-send-email.perl | 7 +------\n 1 file changed, 1 insertion(+), 6 deletions(-)\n\ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex dfd646ac5b..0061dbfab9 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -155,7 +155,6 @@ sub format_2822_time {\n }\n \n my $have_email_valid = eval { require Email::Valid; 1 };\n-my $have_mail_address = eval { require Mail::Address; 1 };\n my $smtp;\n my $auth;\n my $num_sent = 0;\n@@ -490,11 +489,7 @@ my ($repoauthor, $repocommitter);\n ($repocommitter) = Git::ident_person(@repo, 'committer');\n \n sub parse_address_line {\n-\tif ($have_mail_address) {\n-\t\treturn map { $_->format } Mail::Address->parse($_[0]);\n-\t} else {\n-\t\treturn Git::parse_mailboxes($_[0]);\n-\t}\n+\treturn Git::parse_mailboxes($_[0]);\n }\n \n sub split_addrs {\n-- \n2.14.0.rc0.dirty\n\n"}]}