{"thread":{"id":"16794","subject":"[PATCH] git-send-email: handle email address with quoted comma","startedAt":"2008-12-19T03:40:12Z","lastAt":"2008-12-20T20:09:44Z","messageCount":6,"participants":["Wu Fengguang","Junio C Hamano","Matt Kraai"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"98328","messageId":"1229658012-9240-1-git-send-email-fengguang.wu@intel.com","threadId":"16794","inReplyTo":null,"subject":"[PATCH] git-send-email: handle email address with quoted comma","fromName":"Wu Fengguang","fromEmail":"fengguang.wu@intel.com","sentAt":"2008-12-19T03:40:12Z","receivedAt":"2008-12-19T03:40:12Z","isPatch":true,"sender":{"key":"fengguang.wu@intel.com","avatar":null},"body":"Correctly handle email addresses containing quoted commas, e.g.\n\n\t\"Zhu, Yi\" <yi.zhu@intel.com>, \"Li, Shaohua\" <shaohua.li@intel.com>\n\nHere the commas inside the double quotes are NOT email separators.\n\nSigned-off-by: Wu Fengguang <fengguang.wu@intel.com>\n---\n git-send-email.perl |   13 ++++++++++---\n 1 files changed, 10 insertions(+), 3 deletions(-)\n\ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex 3112f76..d44e99c 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -20,6 +20,7 @@ use strict;\n use warnings;\n use Term::ReadLine;\n use Getopt::Long;\n+use Text::ParseWords;\n use Data::Dumper;\n use Term::ANSIColor;\n use File::Temp qw/ tempdir /;\n@@ -359,6 +360,12 @@ foreach my $entry (@bcclist) {\n \tdie \"Comma in --bcclist entry: $entry'\\n\" unless $entry !~ m/,/;\n }\n \n+sub split_addrs($) {\n+\tmy ($addrs) = @_;\n+\n+\treturn &quotewords('\\s*,\\s*', 1, $addrs);\n+}\n+\n my %aliases;\n my %parse_alias = (\n \t# multiline formats can be supported in the future\n@@ -367,7 +374,7 @@ my %parse_alias = (\n \t\t\tmy ($alias, $addr) = ($1, $2);\n \t\t\t$addr =~ s/#.*$//; # mutt allows # comments\n \t\t\t # commas delimit multiple addresses\n-\t\t\t$aliases{$alias} = [ split(/\\s*,\\s*/, $addr) ];\n+\t\t\t$aliases{$alias} = [ split_addrs($addr) ];\n \t\t}}},\n \tmailrc => sub { my $fh = shift; while (<$fh>) {\n \t\tif (/^alias\\s+(\\S+)\\s+(.*)$/) {\n@@ -379,7 +386,7 @@ my %parse_alias = (\n \t\t\tchomp $x;\n \t\t        $x .= $1 while(defined($_ = <$fh>) && /^ +(.*)$/);\n \t\t\t$x =~ /^(\\S+)$f\\t\\(?([^\\t]+?)\\)?(:?$f){0,2}$/ or next;\n-\t\t\t$aliases{$1} = [ split(/\\s*,\\s*/, $2) ];\n+\t\t\t$aliases{$1} = [ split_addrs($2) ];\n \t\t}},\n \tgnus => sub { my $fh = shift; while (<$fh>) {\n \t\tif (/\\(define-mail-alias\\s+\"(\\S+?)\"\\s+\"(\\S+?)\"\\)/) {\n@@ -588,7 +595,7 @@ if (!@to) {\n \t}\n \n \tmy $to = $_;\n-\tpush @to, split /,\\s*/, $to;\n+\tpush @to, split_addrs($to);\n \t$prompting++;\n }\n \n-- \n1.6.0.4\n"},{"id":"98342","messageId":"7vej04d5wy.fsf@gitster.siamese.dyndns.org","threadId":"16794","inReplyTo":"1229658012-9240-1-git-send-email-fengguang.wu@intel.com","subject":"Re: [PATCH] git-send-email: handle email address with quoted comma","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-12-19T06:40:13Z","receivedAt":"2008-12-19T06:40:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Wu Fengguang <fengguang.wu@intel.com> writes:\n\n> Correctly handle email addresses containing quoted commas, e.g.\n>\n> \t\"Zhu, Yi\" <yi.zhu@intel.com>, \"Li, Shaohua\" <shaohua.li@intel.com>\n>\n> Here the commas inside the double quotes are NOT email separators.\n\nThanks.\n\n> @@ -359,6 +360,12 @@ foreach my $entry (@bcclist) {\n>  \tdie \"Comma in --bcclist entry: $entry'\\n\" unless $entry !~ m/,/;\n>  }\n>  \n> +sub split_addrs($) {\n> +\tmy ($addrs) = @_;\n> +\n> +\treturn &quotewords('\\s*,\\s*', 1, $addrs);\n> +}\n> +\n\nDoes it add real value (e.g. type safety, simplified interface to the\ncaller, etc.) to force scalar context to the callers?  It has been my\nexperience that use of prototypes (aka \"parameter context templates\") in\nPerl programs tend to make the code less readable and more error prone in\nlonger term.  I would further say that, even though you do not have any\nexisting caller of split_addrs sub that uses it for more than two values,\nnot using the prototype would be a better way to write this sub in this\nparticular case, because it would allow callers to say [*1*]:\n\n\t@addrs = split_addr(@list_of_addr_lines);\n\nIt also is a bit funny-looking to invoke &function() (it is Perl4 style,\nisn't it?)\n\nIOW, wouldn't this be a better alternative?\n\n\tsub split_addrs {\n        \treturn quotewords('\\s*,\\s*', 1, @_);\n\t}\n\n[Footnote]\n\n*1*  This program demonstrates why use of prototype in this case is more\nconfusing than it is worth.\n\n-- >8 --\n#!/usr/bin/perl -w\n\nuse Text::ParseWords;\n\nsub foo ($) { my ($addrs) = @_; return quotewords('\\s*,\\s*', 1, $addrs); }\nsub bar { return quotewords('\\s*,\\s*', 1, @_); }\nmy @addrs = ('Frotz, \"Xyzzy, Zork\", Nitfol', 'Yomin, Rezrov');\nmy @addr = ($addrs[0]);\nfor (foo($addrs[0])) {\n\tprint \"foo(\\$addrs[0]) <<$_>>\\n\";\n}\nfor (foo(@addr)) {\n\tprint \"foo(\\@addr) <<$_>>\\n\";\n}\nfor (bar($addrs[0])) {\n\tprint \"bar(\\$addrs[0]) <<$_>>\\n\";\n}\nfor (bar(@addr)) {\n\tprint \"bar(\\@addr) <<$_>>\\n\";\n}\n-- 8< --\n\nThe output from the above (the fourth one is the most interesting) looks\nlike this.\n\nfoo($addrs[0]) <<Frotz>>\nfoo($addrs[0]) <<\"Xyzzy, Zork\">>\nfoo($addrs[0]) <<Nitfol>>\nfoo(@addr) <<1>>\nbar($addrs[0]) <<Frotz>>\nbar($addrs[0]) <<\"Xyzzy, Zork\">>\nbar($addrs[0]) <<Nitfol>>\nbar(@addr) <<Frotz>>\nbar(@addr) <<\"Xyzzy, Zork\">>\nbar(@addr) <<Nitfol>>\n\n*2* A more detailed discussion on Perl's \"prototypes\" is found here:\n\nhttp://web.archive.org/web/20080210085941/http://library.n0i.net/programming/perl/articles/fm_prototypes/\n"},{"id":"98344","messageId":"20081219081010.GA12494@localhost","threadId":"16794","inReplyTo":"7vej04d5wy.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] git-send-email: handle email address with quoted comma","fromName":"Wu Fengguang","fromEmail":"fengguang.wu@intel.com","sentAt":"2008-12-19T08:10:10Z","receivedAt":"2008-12-19T08:10:10Z","isPatch":true,"sender":{"key":"fengguang.wu@intel.com","avatar":null},"body":"On Fri, Dec 19, 2008 at 08:40:13AM +0200, Junio C Hamano wrote:\n> Wu Fengguang <fengguang.wu@intel.com> writes:\n> \n> > Correctly handle email addresses containing quoted commas, e.g.\n> >\n> > \t\"Zhu, Yi\" <yi.zhu@intel.com>, \"Li, Shaohua\" <shaohua.li@intel.com>\n> >\n> > Here the commas inside the double quotes are NOT email separators.\n> \n> Thanks.\n> \n> > @@ -359,6 +360,12 @@ foreach my $entry (@bcclist) {\n> >  \tdie \"Comma in --bcclist entry: $entry'\\n\" unless $entry !~ m/,/;\n> >  }\n> >  \n> > +sub split_addrs($) {\n> > +\tmy ($addrs) = @_;\n> > +\n> > +\treturn &quotewords('\\s*,\\s*', 1, $addrs);\n> > +}\n> > +\n> \n> Does it add real value (e.g. type safety, simplified interface to the\n> caller, etc.) to force scalar context to the callers?  It has been my\n> experience that use of prototypes (aka \"parameter context templates\") in\n> Perl programs tend to make the code less readable and more error prone in\n> longer term.  I would further say that, even though you do not have any\n> existing caller of split_addrs sub that uses it for more than two values,\n> not using the prototype would be a better way to write this sub in this\n> particular case, because it would allow callers to say [*1*]:\n> \n> \t@addrs = split_addr(@list_of_addr_lines);\n> \n> It also is a bit funny-looking to invoke &function() (it is Perl4 style,\n> isn't it?)\n> \n> IOW, wouldn't this be a better alternative?\n> \n> \tsub split_addrs {\n>         \treturn quotewords('\\s*,\\s*', 1, @_);\n> \t}\n\nHi Junio and Matt, \n\nThank you for the helpful information. The patch is updated and tested\naccording to your comments.\n\nThanks,\nFengguang\n---\ngit-send-email: handle email address with quoted comma\n\nCorrectly handle email addresses containing quoted commas, e.g.\n\n\t\"Zhu, Yi\" <yi.zhu@intel.com>, \"Li, Shaohua\" <shaohua.li@intel.com>\n\nHere the commas inside the double quotes are NOT email separators.\n\nSigned-off-by: Wu Fengguang <fengguang.wu@intel.com>\n---\n git-send-email.perl |   11 ++++++++---\n 1 files changed, 8 insertions(+), 3 deletions(-)\n\ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex 3112f76..6114401 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -20,6 +20,7 @@ use strict;\n use warnings;\n use Term::ReadLine;\n use Getopt::Long;\n+use Text::ParseWords;\n use Data::Dumper;\n use Term::ANSIColor;\n use File::Temp qw/ tempdir /;\n@@ -359,6 +360,10 @@ foreach my $entry (@bcclist) {\n \tdie \"Comma in --bcclist entry: $entry'\\n\" unless $entry !~ m/,/;\n }\n \n+sub split_addrs {\n+\treturn parse_line('\\s*,\\s*', 1, @_);\n+}\n+\n my %aliases;\n my %parse_alias = (\n \t# multiline formats can be supported in the future\n@@ -367,7 +372,7 @@ my %parse_alias = (\n \t\t\tmy ($alias, $addr) = ($1, $2);\n \t\t\t$addr =~ s/#.*$//; # mutt allows # comments\n \t\t\t # commas delimit multiple addresses\n-\t\t\t$aliases{$alias} = [ split(/\\s*,\\s*/, $addr) ];\n+\t\t\t$aliases{$alias} = [ split_addrs($addr) ];\n \t\t}}},\n \tmailrc => sub { my $fh = shift; while (<$fh>) {\n \t\tif (/^alias\\s+(\\S+)\\s+(.*)$/) {\n@@ -379,7 +384,7 @@ my %parse_alias = (\n \t\t\tchomp $x;\n \t\t        $x .= $1 while(defined($_ = <$fh>) && /^ +(.*)$/);\n \t\t\t$x =~ /^(\\S+)$f\\t\\(?([^\\t]+?)\\)?(:?$f){0,2}$/ or next;\n-\t\t\t$aliases{$1} = [ split(/\\s*,\\s*/, $2) ];\n+\t\t\t$aliases{$1} = [ split_addrs($2) ];\n \t\t}},\n \tgnus => sub { my $fh = shift; while (<$fh>) {\n \t\tif (/\\(define-mail-alias\\s+\"(\\S+?)\"\\s+\"(\\S+?)\"\\)/) {\n@@ -588,7 +593,7 @@ if (!@to) {\n \t}\n \n \tmy $to = $_;\n-\tpush @to, split /,\\s*/, $to;\n+\tpush @to, split_addrs($to);\n \t$prompting++;\n }\n \n-- \n1.6.0.4\n"},{"id":"98423","messageId":"loom.20081219T162504-25@post.gmane.org","threadId":"16794","inReplyTo":"20081219081010.GA12494@localhost","subject":"Re: [PATCH] git-send-email: handle email address with quoted comma","fromName":"Matt Kraai","fromEmail":"kraai@ftbfs.org","sentAt":"2008-12-19T16:28:58Z","receivedAt":"2008-12-19T16:28:58Z","isPatch":true,"sender":{"key":"kraai@ftbfs.org","avatar":null},"body":"Howdy,\n\nWu Fengguang <fengguang.wu <at> intel.com> writes:\n> +sub split_addrs {\n> +\treturn parse_line('\\s*,\\s*', 1, @_);\n> +}\n> +\n\nI'm not sure it's still a good idea to use parse_line.  It should work OK for\nnow, since split_addrs is only passed one string.  If anyone ever tries to pass\nit a list of strings, however, parse_line will ignore all but the first.\n\n-- \nMatt\n"},{"id":"98415","messageId":"7vabar1gw3.fsf@gitster.siamese.dyndns.org","threadId":"16794","inReplyTo":"20081219081010.GA12494@localhost","subject":"Re: [PATCH] git-send-email: handle email address with quoted comma","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-12-20T06:48:28Z","receivedAt":"2008-12-20T06:48:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Wu Fengguang <fengguang.wu@intel.com> writes:\n\n> Thank you for the helpful information. The patch is updated and tested\n\nThanks.  I'll apply but it would be nice to have an additional test script\nsomewhere in t/t9001-send-email.sh to protect this change from regression\nby future changes.\n"},{"id":"98452","messageId":"7v3agiwquv.fsf@gitster.siamese.dyndns.org","threadId":"16794","inReplyTo":"loom.20081219T162504-25@post.gmane.org","subject":"Re: [PATCH] git-send-email: handle email address with quoted comma","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-12-20T20:09:44Z","receivedAt":"2008-12-20T20:09:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matt Kraai <kraai@ftbfs.org> writes:\n\n> Howdy,\n>\n> Wu Fengguang <fengguang.wu <at> intel.com> writes:\n>> +sub split_addrs {\n>> +\treturn parse_line('\\s*,\\s*', 1, @_);\n>> +}\n>> +\n>\n> I'm not sure it's still a good idea to use parse_line.  It should work OK for\n> now, since split_addrs is only passed one string.  If anyone ever tries to pass\n> it a list of strings, however, parse_line will ignore all but the first.\n\nYikes, I should have caught this.  As you point out, this is a breakage\nwaiting to happen until somebody restructures the callers.  We should\nfutureproof it by using quotewords() instead.\n"}]}