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.
On Wed, 12 Nov 2008, Junio C Hamano wrote:
Ian Hilt [off-list ref] writes:
quoted
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.
I didn't realize this was such a bad time to submit this patch.
quoted
@@ -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?
True. Thanks for pointing this out.
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
I think you're mistaken here. It is entirely possible to delete the Cc
and Bcc lines with no ill effect. It is also possible to leave them in
and not add any addresses and the code won't feed these empty lines to
sendmail. In the subroutine send_message, I believe a check is made to
determine if $cc is equal to ''. If it's not, then it will use it. The
Bcc list is even simpler. It is gathered into @recipients via
unique_email_list(). If it's empty, nothing happens.
quoted
+ 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?
Multiline recipient fields.
I know rfc 2822 that you cited above specifies that the address list not
contain a CRLF but as a terminator. However, I thought that for
readability's sake it would be good to enable the user to introduce a
line break into the recipient field. This is why I used the regex's the
way I did and slurp'd the file rather than worked on it line-by-line.
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.
I definitely agree. I started out coding exactly this way but then
thought about Pierre's comment about bloated To and Cc fields. This is
why I wanted to allow the user to introduce line breaks into the
recipient fields. It's a lot easier to read a nicely formatted email
list if there are a lot of addresses.
As a second thought, I probably should have put RFC into the subject of
this patch.
And as far as my code's recipient regex matching the body of the
message, I'll try to find a solution. It may not be possible to do
multiline recipient fields without this problem. Thanks for reviewing
this.