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.
> @@ -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.
--
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
