On Wed, 12 Nov 2008, Junio C Hamano wrote:
> Ian Hilt <ian.hilt@xxxxxxx> 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 <ian.hilt@xxxxxxx>
> > ---
> > 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.
> > @@ -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.
> > + 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
