Samuel GROOT <samuel.groot@xxxxxxxxxxxxxxxx> 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 ? 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. > +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, $_; > + } > + > + 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 (. > + 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/ -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html