Re: [PATCH v2] Edit recipient addresses with the --compose flag

[Junio C Hamano <gitster@pobox.com>]

Ian Hilt <ian.hilt@gmx.com> 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@gmx.com>
> ---
> 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.