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