Re: [WIP-PATCH 1/2] send-email: create email parser subroutine

From: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Date: May 28, 2016, 15:22 UTC
Message-ID: <vpqeg8mi4wm.fsf@anie.imag.fr>
In-Reply-To: <20160527140104.11192-2-samuel.groot@grenoble-inp.org>

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

Show 5 quoted lines

> Parsing and processing in send-email is done in the same loop.
>
> To make the code more maintainable, we create two subroutines:
> - `parse_email` to separate header and body
> - `parse_header` to retrieve data from header

These routines are not specific to git send-email, nor to Git.

Does it make sense to use an external library, like http://search.cpan.org/~rjbs/Email-Simple-2.210/lib/Email/Simple.pm , either by depending on it, or by copying it in Git's source tree ?

If not, I think it would be better to introduce an email parsing library in a dedicated Perl module in perl/ in our source tree, to keep git-send-email.perl more focused on the "send-email" logic.

Show 18 quoted lines

> +sub parse_email {
> +	my @header = ();
> +	my @body = ();
> +	my $fh = shift;
> +
> +	# First unfold multiline header fields
> +	while (<$fh>) {
> +		last if /^\s*$/;
> +		if (/^\s+\S/ and @header) {
> +			chomp($header[$#header]);
> +			s/^\s+/ /;
> +			$header[$#header] .= $_;
> +		} else {
> +			push(@header, $_);
> +		}
> +	}
> +
> +	# Now unfold the message body

Why "unfold"? Don't you mean "split message body into a list of lines"?

Show 6 quoted lines

> +	while (<$fh>) {
> +		push @body, $_;
> +	}
> +
> +	return (@header, @body);
> +}

Please document your functions. See e.g. perl/Git.pm for an example of what perldoc allows you to do.

This also lacks tests. One advantage of having a clean API is that it also makes it simpler to do unit-testing. Grep "Test::More" in t/ to see some existing unit-tests in Perl.

> +	foreach(@_) {

Style: space before (.

Show 5 quoted lines

> +		if (defined $input_format && $input_format eq 'mbox') {
> +			if (/^Subject:\s+(.*)$/i) {
> +				$subject = $1;
> +			} elsif (/^From:\s+(.*)$/i) {
> +				$from = $1;

Not sure we need thes if/elsif/ for generic headers. Email::Simple's API seems much simpler and general: $email->header("From");

> +				foreach my $addr (parse_address_line($1)) {
> +					push @to, $addr;
> +				}

3 lines for an array concatenation in a high-level language. It looks like 2 more than needed ;-).

> +			}
> +
> +		} else {

Useless blank line.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/