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

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 13, 2008, 03:18 UTC
Message-ID: <7vskpwia91.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <1226544602-1839-1-git-send-email-ian.hilt@gmx.com>

Ian Hilt <ian.hilt@gmx.com> writes:

Show 7 quoted lines

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

Show 24 quoted lines

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

Show 6 quoted lines

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