Re: [PATCH v2] Edit recipient addresses with the --compose flag
From: Junio C Hamano <hidden>
Date: 2016-06-15 22:45:38
Ian Hilt [off-list ref] writes:
Sometimes specifying the recipient addresses can be tedious on the command-line. This commit allows the user to edit the recipient addresses in their editor of choice. Signed-off-by: Ian Hilt <redacted> --- Here's an updated commit with improved regex's from Junio and Francis.
This heavily depends on Pierre's patch, so I am CC'ing him for comments. Until his series settles down, I cannot apply this anyway.
quoted hunk
@@ -489,6 +492,9 @@ GIT: for the patch you are writing. GIT: GIT: Clear the body content if you don't wish to send a summary. From: $tpl_sender +To: $tpl_to +Cc: $tpl_cc +Bcc: $tpl_bcc Subject: $tpl_subject In-Reply-To: $tpl_reply_to@@ -512,9 +518,31 @@ EOT open(C,"<",$compose_filename) or die "Failed to open $compose_filename : " . $!; + local $/; + my $c_file = <C>; + $/ = "\n"; + close(C); + + my (@tmp_to, @tmp_cc, @tmp_bcc); + + if ($c_file =~ /^To:\s*(\S.+?)\s*\nCc:/ism) { + @tmp_to = get_recipients($1); + }
Why "\S.+?" and not "\S.*?"? A local user whose login name is 'q' is
disallowed?
Why does the user must keep "Cc:" in order for this new code to pick up
the list of recipients? In other words, you are forbidding the user from
removing the entire "Cc:" line, even when the message should not be Cc'ed
to anywhere. Instead there has to remain an empty Cc: line. Worse yet,
such an empty "Cc:" line is printed to C2 with your patch and eventually
fed to sendmail. I think it is a violation of 2822 to have Cc: that is
empty, as the format is specified as:
cc = "Cc:" address-list CRLF
bcc = "Bcc:" (address-list / [CFWS]) CRLF
address-list = (address *("," address)) / obs-addr-list
+ if ($c_file =~ /^Cc:\s*(\S.+?)\s*\nBcc:/ism) {
+ @tmp_cc = get_recipients($1);
+ }
+ if ($c_file =~ /^Bcc:\s*(\S.+?)\s*\nSubject:/ism) {
+ @tmp_bcc = get_recipients($1);
+ }Exactly the same comment applies to Bcc and Subject part of the parsing. I think the parsing code you introduced simply suck. Why isn't it done as a part of the main loop to read the same file that already exists? Unlike your additional code above that reads the whole file into a scalar only to discard, the existing main loop processes one line at a file (which should be more memory efficient), and you are not handling the header continuation line anyway, processing one line at a time would make your code much simpler (for one thing, you do not have to do /sm at all). Also it won't be confused as your version would if the message happens to have "To:" or "Cc:" in the message part, thanks to $in_body variable check that is already in the code.