Re: [PATCH v4 4/6] send-email: create email parser subroutine

[Junio C Hamano <gitster@pobox.com>]

Samuel GROOT <samuel.groot@grenoble-inp.org> writes:

> +sub parse_email {
> +	my %mail = ();
> +	my $fh = shift;
> +	my $last_header;

> +	# Unfold and parse multiline header fields
> +	while (<$fh>) {
> +		last if /^\s*$/;

You stop at the end of fields before the message body starts.

> +		s/\r\n|\n|\r//;

The pattern is not anchored at the right end of the string;
intended?  Is it worth worrying about a lone '\r'?

> +		if (/^([^\s:]+):[\s]+(.*)$/) {
> +			$last_header = lc($1);
> +			@{$mail{$last_header}} = ()
> +				unless defined $mail{$last_header};
> +			push @{$mail{$last_header}}, $2;

> +		} elsif (/^\s+\S/ and defined $last_header) {
> +			s/^\s+/ /;
> +			push @{$mail{$last_header}}, $_;

Even though the comment said "unfold", you do not really do the
unfolding here and the caller can (if it wants to) figure out where
one logical header was folded in the original into multiple physical
lines, because you are returning an arrayref.

However, that means the caller still cannot tell what the original
was if you are given:

	X-header: a b
            c
	X-header: d

as you would return { 'X-header' => ["a b", "c", "d")] }

In that sense, it may be better to do a real unfolding here, so that
it would return { 'X-header' => ["a b c", "d"] } from here instead?

I.e. instead of "push @{...}, $_", append $_ to the last element of
that array?

> +		} else {
> +			die("Mail format undefined!\n");

What does that mean?  It would probably help if you included the
line that the code did not understand in the message.


> +		}
> +	}
> +
> +	# Separate body from header
> +	$mail{"body"} = [(<$fh>)];
> +
> +	return \%mail;

The name of the local thing is not observable from the caller, but
because this is "parse-email-header" and returns "header fields"
without reading the "mail", perhaps call it %header instead?

> +}