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

From: Eric Wong <e@80x24.org>
Date: May 28, 2016, 23:33 UTC
Message-ID: <20160528233329.GA1132@dcvr.yhbt.net>
In-Reply-To: <vpqeg8mi4wm.fsf@anie.imag.fr>

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> wrote:

Show 13 quoted lines

> Samuel GROOT <samuel.groot@grenoble-inp.org> writes:
> 
> > 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 ?

That might be overkill and increase installation/maintenance burden. Bundling it would probably be problematic to distros, too.

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

Sounds good, Git.pm already has parse_mailboxes

Show 24 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"?
> 
> > +	while (<$fh>) {
> > +		push @body, $_;
> > +	}

I'd rather avoid the loops entirely and do this:

	local $/ = "\n"; # in case caller clobbers $/
	@body = (<$fh>);

> > +	return (@header, @body);
> > +}

Show 8 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");

Right. Reading this, it would've been easier to parse headers into a hash (normalized keys to lowercase) up front inside parse_email.