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:

> -		print $c2 $_;
>  	}
> +
>  	close $c;


Nit: this added newline does not seem necessary to me. Nothing
serious, but this kind of thing tend to distract the reader when
reviewing the patch.

> +	foreach my $key (keys %parsed_email) {
> +		next if $key == 'body';
> +		print $c2 "$key: $parsed_email{$key}";
> +	}

I'd add a comment like

	# Preserve unknown headers

at the top of the loop to make it clear what we're doing.

On a side note: there's no comment in the code you're adding. This is
not necessarily a bad thing (beautifully written code does not need
comments to be readable), but you may re-read your code with the
question "did I explain everything well-enough?" in mind. The loop
above is a case where IMHO a short and sweet comment helps the reader.

Two potential issues not mentionned in your message but that we
discussed offlist is that 1) this doesn't preserve the order, and 2)
this strips duplicate headers. I believe this is not a problem here,
and trying to solve these points would make the code overkill, but
this would really deserve being mentionned in the commit message.
First, so that people reviewing your patch now can confirm (or not)
that you are taking the right decision by doing this, and also for
people in the future examining your patch (e.g. after a bisect).

> +sub parse_header_line {
> +	my $lines = shift;
> +	my $parsed_line = shift;
> +	my $pattern = join "|", qw(To Cc Bcc);

Nit: you may want to rename it to something more explicit, like
$addr_headers_pat.

None of my nit should block the patch inclusion, but I think the
commit message should be expanded to include a mention of the
"duplicate headers"/"header order" potential issue.

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