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"?
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.
From: Eric Wong <hidden> Date: 2016-06-16 02:19:38
Matthieu Moy [off-list ref] wrote:
Samuel GROOT [off-list ref] writes:
quoted
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
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
quoted
+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"?
quoted
+ while (<$fh>) {
+ push @body, $_;
+ }
I'd rather avoid the loops entirely and do this:
local $/ = "\n"; # in case caller clobbers $/
@body = (<$fh>);
From: Samuel GROOT <hidden> Date: 2016-06-16 02:19:38
On 05/29/2016 01:33 AM, Eric Wong wrote:
Matthieu Moy [off-list ref] wrote:
quoted
Samuel GROOT [off-list ref] writes:
quoted
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
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.
quoted
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.
quoted
quoted
+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"?
quoted
+ 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!
From: Samuel GROOT <hidden> Date: 2016-06-16 02:19:42
On 05/29/2016 01:33 AM, Eric Wong wrote:
Matthieu Moy [off-list ref] wrote:
quoted
Samuel GROOT [off-list ref] writes:
quoted
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
That might be overkill and increase installation/maintenance
burden. Bundling it would probably be problematic to distros,
too.
We have 5 solutions here:
1. Make a new dependence to Email::Simple.
2. Bundle Email::Simple in Git's source tree.
3. Use Email::Simple if installed, else use our library.
4. Making our own email parser library.
5. Duplicate parser loop as we did for our patch to implement
`--quote-email` as proposed in $gmane/295772 .
Obviously, option (5) is the easiest one for us, but it leaves
refactoring for later, and option (1) is also easier but adds a new
dependence which is not that good.
Since our project ends next week, we might not have enough time to
finish developing a custom parser API so (4) is not a viable option for
now but could be done in the future.
We could consider bundling Email::Simple as the best option, as it's
developed since 2003 and might be safer to use than anything we could
write in several weeks.
From: Eric Wong <hidden> Date: 2016-06-16 02:19:42
Samuel GROOT [off-list ref] wrote:
On 05/29/2016 01:33 AM, Eric Wong wrote:
quoted
Matthieu Moy [off-list ref] wrote:
quoted
Samuel GROOT [off-list ref] writes:
quoted
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
That might be overkill and increase installation/maintenance
burden. Bundling it would probably be problematic to distros,
too.
We have 5 solutions here:
1. Make a new dependence to Email::Simple.
2. Bundle Email::Simple in Git's source tree.
3. Use Email::Simple if installed, else use our library.
4. Making our own email parser library.
5. Duplicate parser loop as we did for our patch to implement
`--quote-email` as proposed in $gmane/295772 .
Obviously, option (5) is the easiest one for us, but it leaves refactoring
for later, and option (1) is also easier but adds a new dependence which is
not that good.
I would go with (5) for now and leave (4) for later (which
might just be moving the function to a new file).
Since our project ends next week, we might not have enough time to finish
developing a custom parser API so (4) is not a viable option for now but
could be done in the future.
We could consider bundling Email::Simple as the best option, as it's
developed since 2003 and might be safer to use than anything we could write
in several weeks.
In an ideal world, (1) would be nice. But (IMHO) git-send-email
should remain installable on non-ideal systems which do not
provide Email::Simple as a package.
(2) would probably be non-ideal for distro maintainers
(+Cc: Jonathan for opinions), and (3) is the most complex
and difficult-to-support.