{"thread":{"id":"16300","subject":"[PATCH v2] Edit recipient addresses with the --compose flag","startedAt":"2008-11-13T02:50:02Z","lastAt":"2008-11-14T16:58:42Z","messageCount":5,"participants":["Ian Hilt","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"95659","messageId":"1226544602-1839-1-git-send-email-ian.hilt@gmx.com","threadId":"16300","inReplyTo":null,"subject":"[PATCH v2] Edit recipient addresses with the --compose flag","fromName":"Ian Hilt","fromEmail":"ian.hilt@gmx.com","sentAt":"2008-11-13T02:50:02Z","receivedAt":"2008-11-13T02:50:02Z","isPatch":true,"sender":{"key":"ian.hilt@gmx.com","avatar":null},"body":"Sometimes specifying the recipient addresses can be tedious on the\ncommand-line. This commit allows the user to edit the recipient\naddresses in their editor of choice.\n\nSigned-off-by: Ian Hilt \n---\nHere's an updated commit with improved regex's from Junio and Francis.\n\n git-send-email.perl | 47 ++++++++++++++++++++++++++++++++++++++++++++---\n 1 files changed, 44 insertions(+), 3 deletions(-)\n\ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex 9039cfd..4d8aaa7 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -480,6 +480,9 @@ if ($compose) {\n \tmy $tpl_sender = $sender || $repoauthor || $repocommitter || '';\n \tmy $tpl_subject = $initial_subject || '';\n \tmy $tpl_reply_to = $initial_reply_to || '';\n+\tmy $tpl_to = join(', ', @to);\n+\tmy $tpl_cc = join(', ', @initial_cc);\n+\tmy $tpl_bcc = join(', ', @bcclist);\n \n \tprint C <;\n+\t$/ = \"\\n\";\n+\tclose(C);\n+\n+\tmy (@tmp_to, @tmp_cc, @tmp_bcc);\n+\n+\tif ($c_file =~ /^To:\\s*(\\S.+?)\\s*\\nCc:/ism) {\n+\t\t@tmp_to = get_recipients($1);\n+\t}\n+\tif ($c_file =~ /^Cc:\\s*(\\S.+?)\\s*\\nBcc:/ism) {\n+\t\t@tmp_cc = get_recipients($1);\n+\t}\n+\tif ($c_file =~ /^Bcc:\\s*(\\S.+?)\\s*\\nSubject:/ism) {\n+\t\t@tmp_bcc = get_recipients($1);\n+\t}\n+\n+\n \tmy $need_8bit_cte = file_has_nonascii($compose_filename);\n \tmy $in_body = 0;\n \tmy $summary_empty = 1;\n+\n+\topen(C,\"<\",$compose_filename)\n+\t\tor die \"Failed to open $compose_filename : \" . $!;\n+\n \twhile() {\n \t\tnext if m/^GIT: /;\n \t\tif ($in_body) {\n@@ -543,15 +571,21 @@ EOT\n \t\t} elsif (/^From:\\s*(.+)\\s*$/i) {\n \t\t\t$sender = $1;\n \t\t\tnext;\n-\t\t} elsif (/^(?:To|Cc|Bcc):/i) {\n-\t\t\tprint \"To/Cc/Bcc fields are not interpreted yet, they have been ignored\\n\";\n-\t\t\tnext;\n \t\t}\n \t\tprint C2 $_;\n \t}\n \tclose(C);\n \tclose(C2);\n \n+\tif (@tmp_to) {\n+\t\t@to = @tmp_to;\n+\t}\n+\tif (@tmp_cc) {\n+\t\t@initial_cc = @tmp_cc;\n+\t}\n+\tif (@tmp_bcc) {\n+\t\t@bcclist = @tmp_bcc;\n+\t}\n \tif ($summary_empty) {\n \t\tprint \"Summary email is empty, skipping it\\n\";\n \t\t$compose = -1;\n@@ -1095,3 +1129,10 @@ sub file_has_nonascii {\n \t}\n \treturn 0;\n }\n+\n+sub get_recipients {\n+\tmy $match = shift(@_);\n+\tmy @recipients = split(/\\s*,\\s*(?![^\"]+(?:\\\"[^*]*)*\")/, $match);\n+\n+\treturn @recipients;\n+}\n-- \n1.6.0.3.523.g304d0\n"},{"id":"95660","messageId":"7vskpwia91.fsf@gitster.siamese.dyndns.org","threadId":"16300","inReplyTo":"1226544602-1839-1-git-send-email-ian.hilt@gmx.com","subject":"Re: [PATCH v2] Edit recipient addresses with the --compose flag","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-11-13T03:18:50Z","receivedAt":"2008-11-13T03:18:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ian Hilt writes:\n\n> Sometimes specifying the recipient addresses can be tedious on the\n> command-line. This commit allows the user to edit the recipient\n> addresses in their editor of choice.\n>\n> Signed-off-by: Ian Hilt \n> ---\n> Here's an updated commit with improved regex's from Junio and Francis.\n\nThis heavily depends on Pierre's patch, so I am CC'ing him for comments.\nUntil his series settles down, I cannot apply this anyway.\n\n> @@ -489,6 +492,9 @@ GIT: for the patch you are writing.\n> GIT:\n> GIT: Clear the body content if you don't wish to send a summary.\n> From: $tpl_sender\n> +To: $tpl_to\n> +Cc: $tpl_cc\n> +Bcc: $tpl_bcc\n> Subject: $tpl_subject\n> In-Reply-To: $tpl_reply_to\n> \n> @@ -512,9 +518,31 @@ EOT\n> \topen(C,\"<\",$compose_filename)\n> \t\tor die \"Failed to open $compose_filename : \" . $!;\n> \n> +\tlocal $/;\n> +\tmy $c_file = ;\n> +\t$/ = \"\\n\";\n> +\tclose(C);\n> +\n> +\tmy (@tmp_to, @tmp_cc, @tmp_bcc);\n> +\n> +\tif ($c_file =~ /^To:\\s*(\\S.+?)\\s*\\nCc:/ism) {\n> +\t\t@tmp_to = get_recipients($1);\n> +\t}\n\nWhy \"\\S.+?\" and not \"\\S.*?\"? A local user whose login name is 'q' is\ndisallowed?\n\nWhy does the user must keep \"Cc:\" in order for this new code to pick up\nthe list of recipients? In other words, you are forbidding the user from\nremoving the entire \"Cc:\" line, even when the message should not be Cc'ed\nto anywhere. Instead there has to remain an empty Cc: line. Worse yet,\nsuch an empty \"Cc:\" line is printed to C2 with your patch and eventually\nfed to sendmail. I think it is a violation of 2822 to have Cc: that is\nempty, as the format is specified as:\n\n cc = \"Cc:\" address-list CRLF\n bcc = \"Bcc:\" (address-list / [CFWS]) CRLF\n address-list = (address *(\",\" address)) / obs-addr-list\n\n> +\tif ($c_file =~ /^Cc:\\s*(\\S.+?)\\s*\\nBcc:/ism) {\n> +\t\t@tmp_cc = get_recipients($1);\n> +\t}\n> +\tif ($c_file =~ /^Bcc:\\s*(\\S.+?)\\s*\\nSubject:/ism) {\n> +\t\t@tmp_bcc = get_recipients($1);\n> +\t}\n\nExactly the same comment applies to Bcc and Subject part of the parsing.\n\nI think the parsing code you introduced simply suck. Why isn't it done as\na part of the main loop to read the same file that already exists?\n\nUnlike your additional code above that reads the whole file into a scalar\nonly to discard, the existing main loop processes one line at a file\n(which should be more memory efficient), and you are not handling the\nheader continuation line anyway, processing one line at a time would make\nyour code much simpler (for one thing, you do not have to do /sm at all).\nAlso it won't be confused as your version would if the message happens to\nhave \"To:\" or \"Cc:\" in the message part, thanks to $in_body variable check\nthat is already in the code.\n"},{"id":"95758","messageId":"alpine.LFD.2.00.0811132013530.6125@sys-0.hiltweb.site","threadId":"16300","inReplyTo":"7vskpwia91.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH v2] Edit recipient addresses with the --compose flag","fromName":"Ian Hilt","fromEmail":"ian.hilt@gmx.com","sentAt":"2008-11-14T02:10:43Z","receivedAt":"2008-11-14T02:10:43Z","isPatch":true,"sender":{"key":"ian.hilt@gmx.com","avatar":null},"body":"On Wed, 12 Nov 2008, Junio C Hamano wrote:\n> Ian Hilt writes:\n> \n> > Sometimes specifying the recipient addresses can be tedious on the\n> > command-line. This commit allows the user to edit the recipient\n> > addresses in their editor of choice.\n> >\n> > Signed-off-by: Ian Hilt \n> > ---\n> > Here's an updated commit with improved regex's from Junio and Francis.\n> \n> This heavily depends on Pierre's patch, so I am CC'ing him for comments.\n> Until his series settles down, I cannot apply this anyway.\n\nI didn't realize this was such a bad time to submit this patch.\n\n> > @@ -489,6 +492,9 @@ GIT: for the patch you are writing.\n> > GIT:\n> > GIT: Clear the body content if you don't wish to send a summary.\n> > From: $tpl_sender\n> > +To: $tpl_to\n> > +Cc: $tpl_cc\n> > +Bcc: $tpl_bcc\n> > Subject: $tpl_subject\n> > In-Reply-To: $tpl_reply_to\n> > \n> > @@ -512,9 +518,31 @@ EOT\n> > \topen(C,\"<\",$compose_filename)\n> > \t\tor die \"Failed to open $compose_filename : \" . $!;\n> > \n> > +\tlocal $/;\n> > +\tmy $c_file = ;\n> > +\t$/ = \"\\n\";\n> > +\tclose(C);\n> > +\n> > +\tmy (@tmp_to, @tmp_cc, @tmp_bcc);\n> > +\n> > +\tif ($c_file =~ /^To:\\s*(\\S.+?)\\s*\\nCc:/ism) {\n> > +\t\t@tmp_to = get_recipients($1);\n> > +\t}\n> \n> Why \"\\S.+?\" and not \"\\S.*?\"? A local user whose login name is 'q' is\n> disallowed?\n\nTrue. Thanks for pointing this out.\n\n> Why does the user must keep \"Cc:\" in order for this new code to pick up\n> the list of recipients? In other words, you are forbidding the user from\n> removing the entire \"Cc:\" line, even when the message should not be Cc'ed\n> to anywhere. Instead there has to remain an empty Cc: line. Worse yet,\n> such an empty \"Cc:\" line is printed to C2 with your patch and eventually\n> fed to sendmail. I think it is a violation of 2822 to have Cc: that is\n> empty, as the format is specified as:\n> \n> cc = \"Cc:\" address-list CRLF\n> bcc = \"Bcc:\" (address-list / [CFWS]) CRLF\n> address-list = (address *(\",\" address)) / obs-addr-list\n\nI think you're mistaken here. It is entirely possible to delete the Cc\nand Bcc lines with no ill effect. It is also possible to leave them in\nand not add any addresses and the code won't feed these empty lines to\nsendmail. In the subroutine send_message, I believe a check is made to\ndetermine if $cc is equal to ''. If it's not, then it will use it. The\nBcc list is even simpler. It is gathered into @recipients via\nunique_email_list(). If it's empty, nothing happens.\n\n> > +\tif ($c_file =~ /^Cc:\\s*(\\S.+?)\\s*\\nBcc:/ism) {\n> > +\t\t@tmp_cc = get_recipients($1);\n> > +\t}\n> > +\tif ($c_file =~ /^Bcc:\\s*(\\S.+?)\\s*\\nSubject:/ism) {\n> > +\t\t@tmp_bcc = get_recipients($1);\n> > +\t}\n> \n> Exactly the same comment applies to Bcc and Subject part of the parsing.\n> \n> I think the parsing code you introduced simply suck. Why isn't it done as\n> a part of the main loop to read the same file that already exists?\n\nMultiline recipient fields.\n\nI know rfc 2822 that you cited above specifies that the address list not\ncontain a CRLF but as a terminator. However, I thought that for\nreadability's sake it would be good to enable the user to introduce a\nline break into the recipient field. This is why I used the regex's the\nway I did and slurp'd the file rather than worked on it line-by-line.\n\n> Unlike your additional code above that reads the whole file into a scalar\n> only to discard, the existing main loop processes one line at a file\n> (which should be more memory efficient), and you are not handling the\n> header continuation line anyway, processing one line at a time would make\n> your code much simpler (for one thing, you do not have to do /sm at all).\n> Also it won't be confused as your version would if the message happens to\n> have \"To:\" or \"Cc:\" in the message part, thanks to $in_body variable check\n> that is already in the code.\n\nI definitely agree. I started out coding exactly this way but then\nthought about Pierre's comment about bloated To and Cc fields. This is\nwhy I wanted to allow the user to introduce line breaks into the\nrecipient fields. It's a lot easier to read a nicely formatted email\nlist if there are a lot of addresses.\n\nAs a second thought, I probably should have put RFC into the subject of\nthis patch.\n\nAnd as far as my code's recipient regex matching the body of the\nmessage, I'll try to find a solution. It may not be possible to do\nmultiline recipient fields without this problem. Thanks for reviewing\nthis.\n"},{"id":"95760","messageId":"7v7i77f0f8.fsf@gitster.siamese.dyndns.org","threadId":"16300","inReplyTo":"alpine.LFD.2.00.0811132013530.6125@sys-0.hiltweb.site","subject":"Re: [PATCH v2] Edit recipient addresses with the --compose flag","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-11-14T03:31:39Z","receivedAt":"2008-11-14T03:31:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ian Hilt writes:\n\n> On Wed, 12 Nov 2008, Junio C Hamano wrote:\n>> Ian Hilt writes:\n>> \n>> > Sometimes specifying the recipient addresses can be tedious on the\n>> > command-line. This commit allows the user to edit the recipient\n>> > addresses in their editor of choice.\n>> >\n>> > Signed-off-by: Ian Hilt \n>> > ---\n>> > Here's an updated commit with improved regex's from Junio and Francis.\n>> \n>> This heavily depends on Pierre's patch, so I am CC'ing him for comments.\n>> Until his series settles down, I cannot apply this anyway.\n>\n> I didn't realize this was such a bad time to submit this patch.\n\nIt is not a bad time. I just won't be able to apply it right away, but\npeople (like Pierre) who are interested in send-email enhancement can help\nimproving your patch by reviewing.\n\n>> Why does the user must keep \"Cc:\" in order for this new code to pick up\n>> the list of recipients? ...\n>> \n>> cc = \"Cc:\" address-list CRLF\n>> bcc = \"Bcc:\" (address-list / [CFWS]) CRLF\n>> address-list = (address *(\",\" address)) / obs-addr-list\n>\n> I think you're mistaken here. It is entirely possible to delete the Cc\n> and Bcc lines with no ill effect.\n\nYou have this piece of code\n\n>> > +\tif ($c_file =~ /^To:\\s*(\\S.+?)\\s*\\nCc:/ism) {\n>> > +\t\t@tmp_to = get_recipients($1);\n>> > +\t}\n\nto pick up the \"To: \" addressees. If your user deletes Cc: line, would\nthat regexp still capture them in @tmp_to? How?\n\n> determine if $cc is equal to ''. If it's not, then it will use it.\n\nAh, somehow I thought C2 you are writing into (message.final) was used as\nthe final payload, but you are right. The foreach () loop at the toplevel\nreads them and interprets them.\n\n>> I think the parsing code you introduced simply suck. Why isn't it done as\n>> a part of the main loop to read the same file that already exists?\n>\n> Multiline recipient fields.\n\nSo you were trying to handle folded headers after all, I see.\n\nBut if you were to go that route, I think you are much better off doing so\nby enabling the header folding for all the header lines in the while ()\nloop that currently reads one line at a time.\n\nI however hove to wonder why we are not using any canned e-mail header\nparser for this part of the code. Surely there must be a widely used one\nthat everybody who writes Perl uses???\n"},{"id":"95799","messageId":"alpine.LFD.2.00.0811141143360.2651@maintenance05.msc.mcgregor-surmount.com","threadId":"16300","inReplyTo":"7v7i77f0f8.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH v2] Edit recipient addresses with the --compose flag","fromName":"Ian Hilt","fromEmail":"ian.hilt@gmx.com","sentAt":"2008-11-14T16:58:42Z","receivedAt":"2008-11-14T16:58:42Z","isPatch":true,"sender":{"key":"ian.hilt@gmx.com","avatar":null},"body":"On Thu, 13 Nov 2008, Junio C Hamano wrote:\n> Ian Hilt writes:\n> > On Wed, 12 Nov 2008, Junio C Hamano wrote:\n> >> Ian Hilt writes:\n> >> \n> >> > Sometimes specifying the recipient addresses can be tedious on the\n> >> > command-line. This commit allows the user to edit the recipient\n> >> > addresses in their editor of choice.\n> >> >\n> >> > Signed-off-by: Ian Hilt \n> >> > ---\n> >> > Here's an updated commit with improved regex's from Junio and Francis.\n> >> \n> >> This heavily depends on Pierre's patch, so I am CC'ing him for comments.\n> >> Until his series settles down, I cannot apply this anyway.\n> >\n> > I didn't realize this was such a bad time to submit this patch.\n> \n> It is not a bad time. I just won't be able to apply it right away, but\n> people (like Pierre) who are interested in send-email enhancement can help\n> improving your patch by reviewing.\n\nAnd improvement it needs.\n\n> >> Why does the user must keep \"Cc:\" in order for this new code to pick up\n> >> the list of recipients? ...\n> >> \n> >> cc = \"Cc:\" address-list CRLF\n> >> bcc = \"Bcc:\" (address-list / [CFWS]) CRLF\n> >> address-list = (address *(\",\" address)) / obs-addr-list\n> >\n> > I think you're mistaken here. It is entirely possible to delete the Cc\n> > and Bcc lines with no ill effect.\n> \n> You have this piece of code\n> \n> >> > +\tif ($c_file =~ /^To:\\s*(\\S.+?)\\s*\\nCc:/ism) {\n> >> > +\t\t@tmp_to = get_recipients($1);\n> >> > +\t}\n> \n> to pick up the \"To: \" addressees. If your user deletes Cc: line, would\n> that regexp still capture them in @tmp_to? How?\n\nWow. I don't know why I didn't catch that. You're right.\n\n> > determine if $cc is equal to ''. If it's not, then it will use it.\n> \n> Ah, somehow I thought C2 you are writing into (message.final) was used as\n> the final payload, but you are right. The foreach () loop at the toplevel\n> reads them and interprets them.\n> \n> >> I think the parsing code you introduced simply suck. Why isn't it done as\n> >> a part of the main loop to read the same file that already exists?\n> >\n> > Multiline recipient fields.\n> \n> So you were trying to handle folded headers after all, I see.\n> \n> But if you were to go that route, I think you are much better off doing so\n> by enabling the header folding for all the header lines in the while ()\n> loop that currently reads one line at a time.\n> \n> I however hove to wonder why we are not using any canned e-mail header\n> parser for this part of the code. Surely there must be a widely used one\n> that everybody who writes Perl uses???\n\nI thought about this, but I didn't want to introduce another dependency.\nI'm sure there's an easy way to do this stuff but I just don't have\nenough perl know-how yet.\n\nIn any case, I think this concept needs serious work considering the\npoints that have been raised. I don't have a lot of time to devote to\nthis however much I would like to complete it sooner than later. So for\nnow, unless someone else wants to take up the mantle, I think it would\nbe better to put this patch aside.\n"}]}