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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Jun 8, 2016, 18:12 UTC
Message-ID: <CAPig+cTO+-aATxyNBt2HtctH_ofgqEc8ik3OLSN+THVgu6dhKQ@mail.gmail.com>
In-Reply-To: <xmqq8tyfmuk4.fsf@gitster.mtv.corp.google.com>

On Wed, Jun 8, 2016 at 1:58 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 16 quoted lines

> 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'?

Thanks, I think you covered pretty much everything I was going to say. I'd just add that if the matching is going to be kept loose like this (rather than anchoring it), then s/[\r\n]+//g might be easier to read, but it's a minor point.

Show 14 quoted lines

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

Also, the comment about folding lines should be moved down the part of the code which is actually (supposed to be) doing the folding rather than having the comment at the top of the loop.

Show 14 quoted lines

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

Right.

Show 8 quoted lines

>> +     # 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?

If there is (for some reason) a mail header named 'body', then this assignment of the body portion of the message will overwrite it. Perhaps this function should instead return multiple values: the header hash, and the message body.