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

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Thu, Dec 07 2017, Matthieu Moy jotted:

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

Good catch, actually this can just be: /^$/. The $ syntax already
matches the ending newline, no need for /^\n$/.

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

Although you'll end up with a lot of stuff in the $parsed_line hash you
don't need, which makes dumping it for debugging verbose.

I also wonder about multi-line headers, but then again that probably
breaks already on e.g. Message-ID and Refererences, but that's an
existing bug unrelated to this patch...

>> +			$body = $body . $body_line;
>
> Or just: $body .= $body_line;