Re: [PATCH v4] send-email: extract email-parsing code into a subroutine

From: Matthieu Moy <matthieu.moy@univ-lyon1.fr>
Date: Dec 7, 2017, 13:22 UTC
Message-ID: <q7h94lp2oepu.fsf@orange.lip.ens-lyon.fr>
In-Reply-To: <ff9066a7209b4e21867d933542f8eece@BPMBX2013-01.univ-lyon1.fr>

Not terribly important, but your patch has trailing newlines. "git diff --staged --check" to see them. More below.

PAYRE NATHAN p1508475 <nathan.payre@etu.univ-lyon1.fr> writes:

> the part of code which parses the header a last time to prepare the
> email and send it.

The important point is not that it's the last time the code parses headers, so I'd drop the "a last time".

Show 6 quoted lines

> +	my %parsed_email;
> +	$parsed_email{'body'} = '';
> +	while (my $line = <$c>) {
> +		next if $line =~ m/^GIT:/;
> +		parse_header_line($line, \%parsed_email);
> +		if ($line =~ /^\n$/i) {

You don't need the /i (case-Insensitive) here, there are no letters to match.

> +	if ($parsed_email{'mime-version'}) {
> +		$need_8bit_cte = 0;

This $need_8bit_cte is a leftover of the old code, which processed the headers in the order it found them in the message and had to remember the content of MIME-Version while parsing Content-Type.

I believe you can apply this on top of your patch:

--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -709,7 +709,6 @@ EOT3
        open $c, "<", $compose_filename
                or die sprintf(__("Failed to open %s: %s"), $compose_filename, $!);
 
-       my $need_8bit_cte = file_has_nonascii($compose_filename);
        my $in_body = 0;
        my $summary_empty = 1;
        if (!defined $compose_encoding) {
@@ -740,12 +739,10 @@ EOT3
                        "\n";
        }
        if ($parsed_email{'mime-version'}) {
-               $need_8bit_cte = 0;
                print $c2 "MIME-Version: $parsed_email{'mime-version'}\n",
                                "Content-Type: $parsed_email{'content-type'};\n",
                                "Content-Transfer-Encoding: $parsed_email{'content-transfer-encoding'}\n";
-       }
-       if ($need_8bit_cte) {
+       } else if (file_has_nonascii($compose_filename)) {
                if ($parsed_email{'content-type'}) {
                                print $c2 "MIME-Version: 1.0\n",
                                         "Content-Type: $parsed_email{'content-type'};",

It reads much better: "If the original message already had a
MIME-Version header, then use that, else see if the file has non-ascii
characters and if so, use MIME-Version: 1.0".

Actually, you can even simplify further by factoring the if/else below:

> +		if ($parsed_email{'content-type'}) {
> +				print $c2 "MIME-Version: 1.0\n",
> +					 "Content-Type: $parsed_email{'content-type'};",

(Suspicious ";", and suspicious absence of "\n" here, I don't think it's
intentional and I'm fixing it below, but correct me if I'm wrong)

> +					 "Content-Transfer-Encoding: 8bit\n";
> +			} else {

(Broken indentation, this is not aligned with the "if" above)

>  				print $c2 "MIME-Version: 1.0\n",
>  					 "Content-Type: text/plain; ",
> -					   "charset=$compose_encoding\n",
> +					 "charset=$compose_encoding\n",
>  					 "Content-Transfer-Encoding: 8bit\n";
>  			}

This could become stg like (untested):

	} else if (file_has_nonascii($compose_filename)) {
        	my $content_type = ($parsed_email{'content-type'} or
                	"text/plain; charset=$compose_encoding");
		print $c2 "MIME-Version: 1.0\n",
			  "Content-Type: $content_type\n",
			  "Content-Transfer-Encoding: 8bit\n";
	}

> +	open $c2, "<", $compose_filename . ".final"
> +		or die sprintf(__("Failed to open %s.final: %s"), $compose_filename, $!);
> +	close $c2;

What is this? Cut-and-paste mistake?

> +sub parse_header_line {
> +	my $lines = shift;
> +	my $parsed_line = shift;
> +	my $pattern1 = join "|", qw(To Cc Bcc);
> +	my $pattern2 = join "|",
> +		qw(From Subject Date In-Reply-To Message-ID MIME-Version 
> +			Content-Type Content-Transfer-Encoding References);
> +	
> +	foreach (split(/\n/, $lines)) {
> +		if (/^($pattern1):\s*(.+)$/i) {
> +		        $parsed_line->{lc $1} = [ parse_address_line($2) ];
> +		} elsif (/^($pattern2):\s*(.+)\s*$/i) {
> +		        $parsed_line->{lc $1} = $2;
> +		}

I don't think you need to list the possibilities in the "else" branch.
Just matching /^([^:]*):\s*(.+)\s*$/i should do the trick.

> +			$body = $body . $body_line;

Or just: $body .= $body_line;

-- 
Matthieu Moy
https://matthieu-moy.fr/