Re: [WIP-PATCH 1/2] send-email: create email parser subroutine
- From
- Samuel GROOT <samuel.groot@grenoble-inp.org>
- Date
- May 29, 2016, 17:15 UTC
- Message-ID
- <8904f487-985c-bd2d-a8d1-4a712c6ef558@grenoble-inp.org>
- In-Reply-To
- <20160528233329.GA1132@dcvr.yhbt.net>
On 05/29/2016 01:33 AM, Eric Wong wrote:
Show 18 quoted lines
> Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> wrote: >> 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.
I have no opinion on that topic, but it could be interesting to have other opinions. For the first patch I thought it would be easier and quicker to use code already written, and maybe use another method in the next iteration.
Email::Simple is licensed under Perl's Artistic License or GPL (v1 or any later version), so it's fine to bundle it.
Show 5 quoted lines
>> 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
I agree, I will look into that.
Show 29 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>);I didn't know this method before, thanks for suggesting it!
Show 15 quoted lines
>>> + return (@header, @body);
>>> +}
>
>
>>> + 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.So should we merge parse_email and parse_header in one unique subroutine?