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

[Matthieu Moy <matthieu.moy@univ-lyon1.fr>]

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

> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -685,7 +685,7 @@ Lines beginning in "GIT:" will be removed.
>  Consider including an overall diffstat or table of contents
>  for the patch you are writing.
>  
> -Clear the body content if you don't wish to send a summary.
> +Clear the body content if you dont wish to send a summary.

This is not part of your patch. Use "git add -p" to specify
exactly which hunks should go into the patch and don't let this
kind of change end up in the version you send.

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

I didn't notice this at first, but you're modifying the behavior here:
the old code used to print to $c2 anything that didn't match any of
the if/else if branches.

To keep this behavior, you need to keep all these extra headers in
$parsed_email (you do, in this version) and print them after taking
care of all the known headers (AFAICT, you don't).

>  	}
> -	close $c;
> -	close $c2;

You'll still need $c2, but you don't need $c anymore, so I'd keep the
"close $c" here. OTOH, $c2 is not needed before this point (actually a
bit later), so it would make sense to move the "open" down a little.
This would materialize the "read input, then write output" scheme (as
opposed to "write output while reading input" in the previous code).
It's not a new issue in your patch, but giving variables meaningful
names (i.e. not $c and $c2) would help, too.

> +	if ($parsed_email{'mime-version'}) {
> +		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 ($parsed_email{'content-type'}) {
> +		print $c2 "MIME-Version: 1.0\n",
> +			 "Content-Type: $parsed_email{'content-type'};\n",
> +			 "Content-Transfer-Encoding: 8bit\n";

This "if ($parsed_email{'content-type'})" does not correspond to
anything in the old code, and ...

> +	} elsif (file_has_nonascii($compose_filename)) {
> +                my $content_type = ($parsed_email{'content-type'} or
> +                        "text/plain; charset=$compose_encoding");

Here, your're dealing explicitly with $parsed_email{'content-type'} !=
false (you're in the 'else' branch where it can only be false).

I think you just meant to drop the "if
($parsed_email{'content-type'})" part, and plug the "elseif" directly
after the "if ($parsed_email{'mime-version'})". That's what I
suggested in my earlier email.

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

This part is indented with spaces, please use tabs.

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