send-email: extract email-parsing code into a subroutine
PATCH 1/1 send-email: extract email-parsing code into a subroutine
From: Nathan Payre <[email protected]>
The existing code mixes parsing of email header with regular expression and actual code. Extract the parsing code into a new subroutine 'parse_header_line()'. This improves the code readability and make parse_header_line reusable in other place.
---
This patch is a first step to implement a new feature.
See new feature discussion here: https://public-inbox.org/git/[email protected]/
git-send-email.perl | 106 +++++++++++++++++++++++++++++++++++++++------------- 1 file changed, 80 insertions(+), 26 deletions(-)
git-send-email.perl +80 -26
@@ -715,41 +715,64 @@ EOT3 if (!defined $compose_encoding) { $compose_encoding = "UTF-8"; } - while(<$c>) { + + my %parsed_email; + while (<$c>) { next if m/^GIT:/; - if ($in_body) { - $summary_empty = 0 unless (/^\n$/); - } elsif (/^\n$/) { - $in_body = 1; - if ($need_8bit_cte) { + parse_header_line($_, \%parsed_email); + if (/^\n$/i) { + while (my $row = <$c>) { + if (!($row =~ m/^GIT:/)) { + $parsed_email{'body'} = $parsed_email{'body'} . $row; + } + } + } + } + if ($parsed_email{'from'}) { + $sender = $parsed_email{'from'}; + } + if ($parsed_email{'in_reply_to'}) { + $initial_reply_to = $parsed_email{'in_reply_to'}; + } + if ($parsed_email{'subject'}) { + $initial_subject = $parsed_email{'subject'}; + print $c2 "Subject: " . + quote_subject($parsed_email{'subject'}, $compose_encoding) . + "\n"; + } + if ($parsed_email{'mime-version'}) { + $need_8bit_cte = 0; + } + if ($need_8bit_cte) { + if ($parsed_email{'content-type'}) { + print $c2 "MIME-Version: 1.0\n", + "Content-Type: $parsed_email{'content-type'};", + "Content-Transfer-Encoding: 8bit\n"; + } else { print $c2 "MIME-Version: 1.0\n", "Content-Type: text/plain; ", - "charset=$compose_encoding\n", + "charset=$compose_encoding\n", "Content-Transfer-Encoding: 8bit\n"; } - } elsif (/^MIME-Version:/i) { - $need_8bit_cte = 0; - } elsif (/^Subject:\s*(.+)\s*$/i) { - $initial_subject = $1; - my $subject = $initial_subject; - $_ = "Subject: " . - quote_subject($subject, $compose_encoding) . - "\n"; - } elsif (/^In-Reply-To:\s*(.+)\s*$/i) { - $initial_reply_to = $1; - next; - } elsif (/^From:\s*(.+)\s*$/i) { - $sender = $1; - next; - } elsif (/^(?:To|Cc|Bcc):/i) { - print __("To/Cc/Bcc fields are not interpreted yet, they have been ignored\n"); - next; - } - print $c2 $_; } + if ($parsed_email{'body'}) { + $summary_empty = 0; + print $c2 "\n$parsed_email{'body'}\n"; + } + close $c; close $c2; + open $c2, "<", $compose_filename . ".final" + or die sprintf(__("Failed to open %s.final: %s"), $compose_filename, $!); + + print "affichage : \n"; + while (<$c2>) { + print $_; + } + + close $c2; + if ($summary_empty) { print __("Summary email is empty, skipping it\n"); $compose = -1;
@@ -792,6 +815,37 @@ sub ask { return; } +sub parse_header_line { + my $lines = shift; + my $parsed_line = shift; + + foreach (split(/\n/, $lines)) { + if (/^From:\s*(.+)$/i) { + $parsed_line->{'from'} = $1; + } elsif (/^To:\s*(.+)$/i) { + $parsed_line->{'to'} = [ parse_address_line($1) ]; + } elsif (/^Cc:\s*(.+)$/i) { + $parsed_line->{'cc'} = [ parse_address_line($1) ]; + } elsif (/^Bcc:\s*(.+)$/i) { + $parsed_line->{'bcc'} = [ parse_address_line($1) ]; + } elsif (/^Subject:\s*(.+)\s*$/i) { + $parsed_line->{'subject'} = $1; + } elsif (/^Date: (.*)/i) { + $parsed_line->{'date'} = $1; + } elsif (/^In-Reply-To:\s*(.+)\s*$/i) { + $parsed_line->{'in_reply_to'} = $1; + } elsif (/^Message-ID: (.*)$/i) { + $parsed_line->{'message_id'} = $1; + } elsif (/^MIME-Version:$/i) { + $parsed_line->{'mime-version'} = $1; + } elsif (/^Content-Type:\s+(.*)\s*$/i) { + $parsed_line->{'content-type'} = $1; + } elsif (/^References:\s+(.*)/i) { + $parsed_line->{'references'} = $1; + } + } +} + my %broken_encoding; sub file_declares_8bit_cte {
Signature · 1 line
2.15.1
I found a mistake in my signed-off-by, please replace <[email protected].> by <[email protected]>
Excuse me.
2017-12-02 18:02 GMT+01:00 Payre Nathan <[email protected]>:
Quoted 155 lines
From: Nathan Payre <[email protected]>
The existing code mixes parsing of email header with regular expression and actual code. Extract the parsing code into a new subroutine '
parse_header_line()'. This improves the code readability and makeparse_header_linereusable in other place.Signed-off-by Nathan Payre <[email protected]>Signed-off-by Matthieu Moy <[email protected]>Signed-off-by Timothee Albertin <[email protected]>Signed-off-by Daniel Bensoussan <[email protected]>---
This patch is a first step to implement a new feature.
See new feature discussion here: https://public-inbox.org/git/[email protected]/git-send-email.perl | 106 +++++++++++++++++++++++++++++++++++++++------------- 1 file changed, 80 insertions(+), 26 deletions(-)git-send-email.perl +80
-27@@ -715,41 +715,64 @@ EOT3 if (!defined $compose_encoding) { $compose_encoding = "UTF-8"; } - while(<$c>) { + + my %parsed_email; + while (<$c>) { next if m/^GIT:/; - if ($in_body) { - $summary_empty = 0 unless (/^\n$/); - } elsif (/^\n$/) { - $in_body = 1; - if ($need_8bit_cte) { + parse_header_line($_, \%parsed_email); + if (/^\n$/i) { + while (my $row = <$c>) { + if (!($row =~ m/^GIT:/)) { + $parsed_email{'body'} = $parsed_email{'body'} . $row; + } + } + } + } + if ($parsed_email{'from'}) { + $sender = $parsed_email{'from'}; + } + if ($parsed_email{'in_reply_to'}) { + $initial_reply_to = $parsed_email{'in_reply_to'}; + } + if ($parsed_email{'subject'}) { + $initial_subject = $parsed_email{'subject'}; + print $c2 "Subject: " . + quote_subject($parsed_email{'subject'}, $compose_encoding) . + "\n"; + } + if ($parsed_email{'mime-version'}) { + $need_8bit_cte = 0; + } + if ($need_8bit_cte) { + if ($parsed_email{'content-type'}) { + print $c2 "MIME-Version: 1.0\n", + "Content-Type: $parsed_email{'content-type'};", + "Content-Transfer-Encoding: 8bit\n"; + } else { print $c2 "MIME-Version: 1.0\n", "Content-Type: text/plain; ", - "charset=$compose_encoding\n", + "charset=$compose_encoding\n", "Content-Transfer-Encoding: 8bit\n"; } - } elsif (/^MIME-Version:/i) { - $need_8bit_cte = 0; - } elsif (/^Subject:\s*(.+)\s*$/i) { - $initial_subject = $1; - my $subject = $initial_subject; - $_ = "Subject: " . - quote_subject($subject, $compose_encoding) . - "\n"; - } elsif (/^In-Reply-To:\s*(.+)\s*$/i) { - $initial_reply_to = $1; - next; - } elsif (/^From:\s*(.+)\s*$/i) { - $sender = $1; - next; - } elsif (/^(?:To|Cc|Bcc):/i) { - print __("To/Cc/Bcc fields are not interpreted yet, they have been ignored\n"); - next; - } - print $c2 $_; } + if ($parsed_email{'body'}) { + $summary_empty = 0; + print $c2 "\n$parsed_email{'body'}\n"; + } + close $c; close $c2; + open $c2, "<", $compose_filename . ".final" + or die sprintf(__("Failed to open %s.final: %s"), $compose_filename, $!); + + print "affichage : \n"; + while (<$c2>) { + print $_; + } + + close $c2; + if ($summary_empty) { print __("Summary email is empty, skipping it\n"); $compose = -1;@@ -792,6 +815,37 @@ sub ask { return; } +sub parse_header_line { + my $lines = shift; + my $parsed_line = shift; + + foreach (split(/\n/, $lines)) { + if (/^From:\s*(.+)$/i) { + $parsed_line->{'from'} = $1; + } elsif (/^To:\s*(.+)$/i) { + $parsed_line->{'to'} = [ parse_address_line($1) ]; + } elsif (/^Cc:\s*(.+)$/i) { + $parsed_line->{'cc'} = [ parse_address_line($1) ]; + } elsif (/^Bcc:\s*(.+)$/i) { + $parsed_line->{'bcc'} = [ parse_address_line($1) ]; + } elsif (/^Subject:\s*(.+)\s*$/i) { + $parsed_line->{'subject'} = $1; + } elsif (/^Date: (.*)/i) { + $parsed_line->{'date'} = $1; + } elsif (/^In-Reply-To:\s*(.+)\s*$/i) { + $parsed_line->{'in_reply_to'} = $1; + } elsif (/^Message-ID: (.*)$/i) { + $parsed_line->{'message_id'} = $1; + } elsif (/^MIME-Version:$/i) { + $parsed_line->{'mime-version'} = $1; + } elsif (/^Content-Type:\s+(.*)\s*$/i) { + $parsed_line->{'content-type'} = $1; + } elsif (/^References:\s+(.*)/i) { + $parsed_line->{'references'} = $1; + } + } +} + my %broken_encoding; sub file_declares_8bit_cte { --2.15.1
A message that is not in the archive (sent off-list or not yet received): [email protected]
Quoted 2 lines · Nathan PAYRE
I found a mistake in my signed-off-by, please replace <[email protected].> by <[email protected]>
I think you want exactly the opposite of this. You're contributing as a Lyon 1 student, hence your identity is @etu.univ-lyon1.fr. Your Gmail adress is used only for technical reasons.
OTOH, you are missing the first line From: ... @..univ-lyon1.fr in your message.
See how you did it:
https://public-inbox.org/git/[email protected]/
(The sign-off was wrong in this one, but the From was OK)
Signature · 2 lines
Matthieu Moy https://matthieu-moy.fr/
On Sat, Dec 02 2017, Payre Nathan jotted:
Quoted 142 lines
From: Nathan Payre <[email protected]>
The existing code mixes parsing of email header with regular expression and actual code. Extract the parsing code into a new subroutine '
parse_header_line()'. This improves the code readability and makeparse_header_linereusable in other place.Signed-off-by Nathan Payre <[email protected]>Signed-off-by Matthieu Moy <[email protected]>Signed-off-by Timothee Albertin <[email protected]>Signed-off-by Daniel Bensoussan <[email protected]>---
This patch is a first step to implement a new feature.
See new feature discussion here: https://public-inbox.org/git/[email protected]/git-send-email.perl | 106 +++++++++++++++++++++++++++++++++++++++------------- 1 file changed, 80 insertions(+), 26 deletions(-)git-send-email.perl +73
-26@@ -715,41 +715,64 @@ EOT3 if (!defined $compose_encoding) { $compose_encoding = "UTF-8"; } - while(<$c>) { + + my %parsed_email; + while (<$c>) { next if m/^GIT:/; - if ($in_body) { - $summary_empty = 0 unless (/^\n$/); - } elsif (/^\n$/) { - $in_body = 1; - if ($need_8bit_cte) { + parse_header_line($_, \%parsed_email); + if (/^\n$/i) { + while (my $row = <$c>) { + if (!($row =~ m/^GIT:/)) { + $parsed_email{'body'} = $parsed_email{'body'} . $row; + } + } + } + } + if ($parsed_email{'from'}) { + $sender = $parsed_email{'from'}; + } + if ($parsed_email{'in_reply_to'}) { + $initial_reply_to = $parsed_email{'in_reply_to'}; + } + if ($parsed_email{'subject'}) { + $initial_subject = $parsed_email{'subject'}; + print $c2 "Subject: " . + quote_subject($parsed_email{'subject'}, $compose_encoding) . + "\n"; + } + if ($parsed_email{'mime-version'}) { + $need_8bit_cte = 0; + } + if ($need_8bit_cte) { + if ($parsed_email{'content-type'}) { + print $c2 "MIME-Version: 1.0\n", + "Content-Type: $parsed_email{'content-type'};", + "Content-Transfer-Encoding: 8bit\n"; + } else { print $c2 "MIME-Version: 1.0\n", "Content-Type: text/plain; ", - "charset=$compose_encoding\n", + "charset=$compose_encoding\n", "Content-Transfer-Encoding: 8bit\n"; } - } elsif (/^MIME-Version:/i) { - $need_8bit_cte = 0; - } elsif (/^Subject:\s*(.+)\s*$/i) { - $initial_subject = $1; - my $subject = $initial_subject; - $_ = "Subject: " . - quote_subject($subject, $compose_encoding) . - "\n"; - } elsif (/^In-Reply-To:\s*(.+)\s*$/i) { - $initial_reply_to = $1; - next; - } elsif (/^From:\s*(.+)\s*$/i) { - $sender = $1; - next; - } elsif (/^(?:To|Cc|Bcc):/i) { - print __("To/Cc/Bcc fields are not interpreted yet, they have been ignored\n"); - next; - } - print $c2 $_; } + if ($parsed_email{'body'}) { + $summary_empty = 0; + print $c2 "\n$parsed_email{'body'}\n"; + } + close $c; close $c2; + open $c2, "<", $compose_filename . ".final" + or die sprintf(__("Failed to open %s.final: %s"), $compose_filename, $!); + + print "affichage : \n"; + while (<$c2>) { + print $_; + } + + close $c2; + if ($summary_empty) { print __("Summary email is empty, skipping it\n"); $compose = -1;@@ -792,6 +815,37 @@ sub ask { return; } +sub parse_header_line { + my $lines = shift; + my $parsed_line = shift; + + foreach (split(/\n/, $lines)) { + if (/^From:\s*(.+)$/i) { + $parsed_line->{'from'} = $1; + } elsif (/^To:\s*(.+)$/i) { + $parsed_line->{'to'} = [ parse_address_line($1) ]; + } elsif (/^Cc:\s*(.+)$/i) { + $parsed_line->{'cc'} = [ parse_address_line($1) ]; + } elsif (/^Bcc:\s*(.+)$/i) { + $parsed_line->{'bcc'} = [ parse_address_line($1) ]; + } elsif (/^Subject:\s*(.+)\s*$/i) { + $parsed_line->{'subject'} = $1; + } elsif (/^Date: (.*)/i) { + $parsed_line->{'date'} = $1; + } elsif (/^In-Reply-To:\s*(.+)\s*$/i) { + $parsed_line->{'in_reply_to'} = $1; + } elsif (/^Message-ID: (.*)$/i) { + $parsed_line->{'message_id'} = $1; + } elsif (/^MIME-Version:$/i) { + $parsed_line->{'mime-version'} = $1; + } elsif (/^Content-Type:\s+(.*)\s*$/i) {
git-send-email.perl sub ask { · line 851
Quoted 10 lines
git-send-email.perl +7
-0+ $parsed_line->{'content-type'} = $1; + } elsif (/^References:\s+(.*)/i) { + $parsed_line->{'references'} = $1; + } + } +} + my %broken_encoding; sub file_declares_8bit_cte {
I haven't read the patches that follow. Completely untested, But just a diff on top I came up with while reading this:
Rationale:
* Once you start passing $_ to functions you should probably just give it a name.
* !($x =~ m//) you can just write as $x !~ m//
* There's a lot of copy/paste programming in parse_header_line() and an inconsistency between you seeing A-Header and turning it into either a_header or a-header. If you just stick with a-header and use dash you end up with just two cases.
The resulting line is quite long, so it's worth doing:
my $header_parsed = join "|", qw(To CC ...); my $header_unparsed = join "|", qw(From Subject Message-ID ...); [...] if ($str =~ /^($header_unparsed)
git-send-email.perl +10 -28
@@ -717,12 +717,12 @@ EOT3 } my %parsed_email; - while (<$c>) { - next if m/^GIT:/; - parse_header_line($_, \%parsed_email); - if (/^\n$/i) { + while (my $line = <$c>) { + next if $line =~ m/^GIT:/; + parse_header_line($line, \%parsed_email); + if ($line =~ /^\n$/i) { while (my $row = <$c>) { - if (!($row =~ m/^GIT:/)) { + if ($row !~ m/^GIT:/) { $parsed_email{'body'} = $parsed_email{'body'} . $row; } }
@@ -731,7 +731,7 @@ EOT3 if ($parsed_email{'from'}) { $sender = $parsed_email{'from'}; } - if ($parsed_email{'in_reply_to'}) { + if ($parsed_email{'in-reply-to'}) { $initial_reply_to = $parsed_email{'in_reply_to'}; } if ($parsed_email{'subject'}) {
@@ -820,28 +820,10 @@ sub parse_header_line { my $parsed_line = shift; foreach (split(/\n/, $lines)) { - if (/^From:\s*(.+)$/i) { - $parsed_line->{'from'} = $1; - } elsif (/^To:\s*(.+)$/i) { - $parsed_line->{'to'} = [ parse_address_line($1) ]; - } elsif (/^Cc:\s*(.+)$/i) { - $parsed_line->{'cc'} = [ parse_address_line($1) ]; - } elsif (/^Bcc:\s*(.+)$/i) { - $parsed_line->{'bcc'} = [ parse_address_line($1) ]; - } elsif (/^Subject:\s*(.+)\s*$/i) { - $parsed_line->{'subject'} = $1; - } elsif (/^Date: (.*)/i) { - $parsed_line->{'date'} = $1; - } elsif (/^In-Reply-To:\s*(.+)\s*$/i) { - $parsed_line->{'in_reply_to'} = $1; - } elsif (/^Message-ID: (.*)$/i) { - $parsed_line->{'message_id'} = $1; - } elsif (/^MIME-Version:$/i) { - $parsed_line->{'mime-version'} = $1; - } elsif (/^Content-Type:\s+(.*)\s*$/i) { - $parsed_line->{'content-type'} = $1; - } elsif (/^References:\s+(.*)/i) { - $parsed_line->{'references'} = $1; + if (/^(To|Cc|Bcc):\s*(.+)$/i) { + $parsed_line->{lc $1} = [ parse_address_line($2) ]; + } elsif (/^(From|Subject|Date|In-Reply-To|Message-ID|MIME-Version|Content-Type|References):\s*(.+)\s*$/i) { + $parsed_line->{lc $1} = $2; } } }
I've tested your code, and after few changes it's works perfectly! The code looks better now.
Thanks a lot for your review.
2017-12-03 23:00 GMT+01:00 Ævar Arnfjörð Bjarmason <[email protected]>:
Quoted 243 lines
On Sat, Dec 02 2017, Payre Nathan jotted:
Quoted 142 lines
From: Nathan Payre <[email protected]>
The existing code mixes parsing of email header with regular expression and actual code. Extract the parsing code into a new subroutine '
parse_header_line()'. This improves the code readability and makeparse_header_linereusable in other place.Signed-off-by Nathan Payre <[email protected]>Signed-off-by Matthieu Moy <[email protected]>Signed-off-by Timothee Albertin <[email protected]>Signed-off-by Daniel Bensoussan <[email protected]>---
This patch is a first step to implement a new feature.
See new feature discussion here: https://public-inbox.org/git/[email protected]/git-send-email.perl | 106 +++++++++++++++++++++++++++++++++++++++------------- 1 file changed, 80 insertions(+), 26 deletions(-)git-send-email.perl +73
-26@@ -715,41 +715,64 @@ EOT3 if (!defined $compose_encoding) { $compose_encoding = "UTF-8"; } - while(<$c>) { + + my %parsed_email; + while (<$c>) { next if m/^GIT:/; - if ($in_body) { - $summary_empty = 0 unless (/^\n$/); - } elsif (/^\n$/) { - $in_body = 1; - if ($need_8bit_cte) { + parse_header_line($_, \%parsed_email); + if (/^\n$/i) { + while (my $row = <$c>) { + if (!($row =~ m/^GIT:/)) { + $parsed_email{'body'} = $parsed_email{'body'} . $row; + } + } + } + } + if ($parsed_email{'from'}) { + $sender = $parsed_email{'from'}; + } + if ($parsed_email{'in_reply_to'}) { + $initial_reply_to = $parsed_email{'in_reply_to'}; + } + if ($parsed_email{'subject'}) { + $initial_subject = $parsed_email{'subject'}; + print $c2 "Subject: " . + quote_subject($parsed_email{'subject'}, $compose_encoding) . + "\n"; + } + if ($parsed_email{'mime-version'}) { + $need_8bit_cte = 0; + } + if ($need_8bit_cte) { + if ($parsed_email{'content-type'}) { + print $c2 "MIME-Version: 1.0\n", + "Content-Type: $parsed_email{'content-type'};", + "Content-Transfer-Encoding: 8bit\n"; + } else { print $c2 "MIME-Version: 1.0\n", "Content-Type: text/plain; ", - "charset=$compose_encoding\n", + "charset=$compose_encoding\n", "Content-Transfer-Encoding: 8bit\n"; } - } elsif (/^MIME-Version:/i) { - $need_8bit_cte = 0; - } elsif (/^Subject:\s*(.+)\s*$/i) { - $initial_subject = $1; - my $subject = $initial_subject; - $_ = "Subject: " . - quote_subject($subject, $compose_encoding) . - "\n"; - } elsif (/^In-Reply-To:\s*(.+)\s*$/i) { - $initial_reply_to = $1; - next; - } elsif (/^From:\s*(.+)\s*$/i) { - $sender = $1; - next; - } elsif (/^(?:To|Cc|Bcc):/i) { - print __("To/Cc/Bcc fields are not interpreted yet, they have been ignored\n"); - next; - } - print $c2 $_; } + if ($parsed_email{'body'}) { + $summary_empty = 0; + print $c2 "\n$parsed_email{'body'}\n"; + } + close $c; close $c2; + open $c2, "<", $compose_filename . ".final" + or die sprintf(__("Failed to open %s.final: %s"), $compose_filename, $!); + + print "affichage : \n"; + while (<$c2>) { + print $_; + } + + close $c2; + if ($summary_empty) { print __("Summary email is empty, skipping it\n"); $compose = -1;@@ -792,6 +815,37 @@ sub ask { return; } +sub parse_header_line { + my $lines = shift; + my $parsed_line = shift; + + foreach (split(/\n/, $lines)) { + if (/^From:\s*(.+)$/i) { + $parsed_line->{'from'} = $1; + } elsif (/^To:\s*(.+)$/i) { + $parsed_line->{'to'} = [ parse_address_line($1) ]; + } elsif (/^Cc:\s*(.+)$/i) { + $parsed_line->{'cc'} = [ parse_address_line($1) ]; + } elsif (/^Bcc:\s*(.+)$/i) { + $parsed_line->{'bcc'} = [ parse_address_line($1) ]; + } elsif (/^Subject:\s*(.+)\s*$/i) { + $parsed_line->{'subject'} = $1; + } elsif (/^Date: (.*)/i) { + $parsed_line->{'date'} = $1; + } elsif (/^In-Reply-To:\s*(.+)\s*$/i) { + $parsed_line->{'in_reply_to'} = $1; + } elsif (/^Message-ID: (.*)$/i) { + $parsed_line->{'message_id'} = $1; + } elsif (/^MIME-Version:$/i) { + $parsed_line->{'mime-version'} = $1; + } elsif (/^Content-Type:\s+(.*)\s*$/i) {git-send-email.perl sub ask { · line 851
Quoted 10 lines
git-send-email.perl +7
-0+ $parsed_line->{'content-type'} = $1; + } elsif (/^References:\s+(.*)/i) { + $parsed_line->{'references'} = $1; + } + } +} + my %broken_encoding; sub file_declares_8bit_cte {I haven't read the patches that follow. Completely untested, But just a diff on top I came up with while reading this:
Rationale:
* Once you start passing $_ to functions you should probably just give it a name.
* !($x =~ m//) you can just write as $x !~ m//
* There's a lot of copy/paste programming in
parse_header_line()and an inconsistency between you seeing A-Header and turning it into eithera_headeror a-header. If you just stick with a-header and use dash you end up with just two cases.The resulting line is quite long, so it's worth doing: my $header_parsed = join "|", qw(To CC ...); my $header_unparsed = join "|", qw(From Subject Message-ID ...); [...] if ($str =~ /^($header_unparsed)git-send-email.perl +10
-28@@ -717,12 +717,12 @@ EOT3 } my %parsed_email; - while (<$c>) { - next if m/^GIT:/; - parse_header_line($_, \%parsed_email); - if (/^\n$/i) { + while (my $line = <$c>) { + next if $line =~ m/^GIT:/; + parse_header_line($line, \%parsed_email); + if ($line =~ /^\n$/i) { while (my $row = <$c>) { - if (!($row =~ m/^GIT:/)) { + if ($row !~ m/^GIT:/) { $parsed_email{'body'} = $parsed_email{'body'} . $row; } }@@ -731,7 +731,7 @@ EOT3 if ($parsed_email{'from'}) { $sender = $parsed_email{'from'}; } - if ($parsed_email{'in_reply_to'}) { + if ($parsed_email{'in-reply-to'}) { $initial_reply_to = $parsed_email{'in_reply_to'}; } if ($parsed_email{'subject'}) {@@ -820,28 +820,10 @@ sub parse_header_line { my $parsed_line = shift; foreach (split(/\n/, $lines)) { - if (/^From:\s*(.+)$/i) { - $parsed_line->{'from'} = $1; - } elsif (/^To:\s*(.+)$/i) { - $parsed_line->{'to'} = [ parse_address_line($1) ]; - } elsif (/^Cc:\s*(.+)$/i) { - $parsed_line->{'cc'} = [ parse_address_line($1) ]; - } elsif (/^Bcc:\s*(.+)$/i) { - $parsed_line->{'bcc'} = [ parse_address_line($1) ]; - } elsif (/^Subject:\s*(.+)\s*$/i) { - $parsed_line->{'subject'} = $1; - } elsif (/^Date: (.*)/i) { - $parsed_line->{'date'} = $1; - } elsif (/^In-Reply-To:\s*(.+)\s*$/i) { - $parsed_line->{'in_reply_to'} = $1; - } elsif (/^Message-ID: (.*)$/i) { - $parsed_line->{'message_id'} = $1; - } elsif (/^MIME-Version:$/i) { - $parsed_line->{'mime-version'} = $1; - } elsif (/^Content-Type:\s+(.*)\s*$/i) { - $parsed_line->{'content-type'} = $1; - } elsif (/^References:\s+(.*)/i) { - $parsed_line->{'references'} = $1; + if (/^(To|Cc|Bcc):\s*(.+)$/i) { + $parsed_line->{lc $1} = [ parse_address_line($2) ]; + } elsif (/^(From|Subject|Date|In-Reply-To|Message-ID|MIME-Version|Content-Type|References):\s*(.+)\s*$/i) { + $parsed_line->{lc $1} = $2; } } }
Quoted 3 lines · Nathan PAYRE
I've tested your code, and after few changes it's works perfectly! The code looks better now.
Thanks a lot for your review.
Thanks, both of you.
Could you send in the final version later so that I can pick it up? I agree with Matthieu's suggestion on what address to use on your
From: (authorship identity) and S-o-b:; as you are showing your work done as a uni student, the authorship and sign-off should be done as such.
PATCH v2 1/1 send-email: extract email-parsing code into a subroutine
The existing code mixes parsing of email header with regular expression and actual code. Extract the parsing code into a new subroutine 'parse_header_line()'. This improves the code readability and make parse_header_line reusable in other place.
Thanks-to: Ævar Arnfjörð Bjarmason <[email protected]>
--- git-send-email.perl | 100 ++++++++++++++++++++++++++++++++++++++-------------- 1 file changed, 73 insertions(+), 27 deletions(-)
git-send-email.perl +73 -27
@@ -715,41 +715,73 @@ EOT3 if (!defined $compose_encoding) { $compose_encoding = "UTF-8"; } - while(<$c>) { - next if m/^GIT:/; - if ($in_body) { - $summary_empty = 0 unless (/^\n$/); - } elsif (/^\n$/) { - $in_body = 1; - if ($need_8bit_cte) { + + my %parsed_email; + $parsed_email{'body'} = ''; + while (my $line = <$c>) { + next if $line =~ m/^GIT:/; + parse_header_line($line, \%parsed_email); + if ($line =~ /^\n$/i) { + while (my $body_line = <$c>) { + if ($body_line !~ m/^GIT:/) { + $parsed_email{'body'} = $parsed_email{'body'} . $body_line; + } + } + } + print "la : $line\n"; + } + + if ($parsed_email{'from'}) { + $sender = $parsed_email{'from'}; + } + if ($parsed_email{'in-reply-to'}) { + $initial_reply_to = $parsed_email{'in-reply-to'}; + } + if ($parsed_email{'subject'}) { + $initial_subject = $parsed_email{'subject'}; + print $c2 "Subject: " . + quote_subject($parsed_email{'subject'}, $compose_encoding) . + "\n"; + } + if ($parsed_email{'mime-version'}) { + print "CASE 0\n"; + $need_8bit_cte = 0; + print $c2 "MIME-Version: $parsed_email{'mime-version'}\n", + "Content-Type: $parsed_email{'content-type'};\n", + "Content-Transfer-Encoding: $parsed_email{'content-transfer-encoding'}\n"; + } + if ($need_8bit_cte) { + if ($parsed_email{'content-type'}) { + print "CASE 1\n"; + print $c2 "MIME-Version: 1.0\n", + "Content-Type: $parsed_email{'content-type'};", + "Content-Transfer-Encoding: 8bit\n"; + } else { + print "CASE 2\n"; print $c2 "MIME-Version: 1.0\n", "Content-Type: text/plain; ", - "charset=$compose_encoding\n", + "charset=$compose_encoding\n", "Content-Transfer-Encoding: 8bit\n"; } - } elsif (/^MIME-Version:/i) { - $need_8bit_cte = 0; - } elsif (/^Subject:\s*(.+)\s*$/i) { - $initial_subject = $1; - my $subject = $initial_subject; - $_ = "Subject: " . - quote_subject($subject, $compose_encoding) . - "\n"; - } elsif (/^In-Reply-To:\s*(.+)\s*$/i) { - $initial_reply_to = $1; - next; - } elsif (/^From:\s*(.+)\s*$/i) { - $sender = $1; - next; - } elsif (/^(?:To|Cc|Bcc):/i) { - print __("To/Cc/Bcc fields are not interpreted yet, they have been ignored\n"); - next; - } - print $c2 $_; } + if ($parsed_email{'body'}) { + $summary_empty = 0; + print $c2 "\n$parsed_email{'body'}\n"; + } + close $c; close $c2; + open $c2, "<", $compose_filename . ".final" + or die sprintf(__("Failed to open %s.final: %s"), $compose_filename, $!); + + print "affichage : \n"; + while (<$c2>) { + print $_; + } + + close $c2; + if ($summary_empty) { print __("Summary email is empty, skipping it\n"); $compose = -1;
@@ -792,6 +824,20 @@ sub ask { return; } +sub parse_header_line { + my $lines = shift; + my $parsed_line = shift; + + foreach (split(/\n/, $lines)) { + if (/^(To|Cc|Bcc):\s*(.+)$/i) { + $parsed_line->{lc $1} = [ parse_address_line($2) ]; + } elsif (/^(From|Subject|Date|In-Reply-To|Message-ID|MIME-Version|Content-Type|Content-Transfer-Encoding|References):\s*(.+)\s*$/i) { + $parsed_line->{lc $1} = $2; + } + } +} + + my %broken_encoding; sub file_declares_8bit_cte {
Signature · 1 line
2.15.1
Quoted 10 lines · Nathan Payre
The existing code mixes parsing of email header with regular expression and actual code. Extract the parsing code into a new subroutine '
parse_header_line()'. This improves the code readability and makeparse_header_linereusable in other place.Signed-off-by Nathan Payre <[email protected]>Signed-off-by Matthieu Moy <[email protected]>Signed-off-by Timothee Albertin <[email protected]>Signed-off-by Daniel Bensoussan <[email protected]>Thanks-to: Ævar Arnfjörð Bjarmason <[email protected]>
Thanks, but...
Quoted 4 lines
git-send-email.perl +4
-0+ my %parsed_email; + $parsed_email{'body'} = ''; + while (my $line = <$c>) { + next if $line =~ m/^GIT:/;
git-send-email.perl EOT3 · line 732
Quoted 10 lines
git-send-email.perl +10
-0+ parse_header_line($line, \%parsed_email); + if ($line =~ /^\n$/i) { + while (my $body_line = <$c>) { + if ($body_line !~ m/^GIT:/) { + $parsed_email{'body'} = $parsed_email{'body'} . $body_line; + } + } + } + print "la : $line\n"; + }
... throughout this patch, not limited to this section, indentation is strange and there seem to be many "print" that show messages that do not seem to be meant for end-user consumption. I can see that this aspires to improve the readability, but not quite yet ;-).
Also "reusable in other place" is by itself not an unconditional plus, until readers can be convinced that that 'other place' really wants to be able to call this function. Is there some untold motivation behind this change---such as a planned update to actually use this helper subroutine?
Quoted 4 lines · Junio C Hamano
... throughout this patch, not limited to this section, indentation is strange and there seem to be many "print" that show messages that do not seem to be meant for end-user consumption. I can see that this aspires to improve the readability, but not quite yet ;-).
Hmmm I'm wondering who place thoses print in my code ! I will fix it fast. :-)
Quoted 5 lines
Also "reusable in other place" is by itself not an unconditional plus, until readers can be convinced that that 'other place' really wants to be able to call this function. Is there some untold motivation behind this change---such as a planned update to actually use this helper subroutine?
This subroutine will be used to implement, initially a new option called "--quote-email", but became "--cite" added after "--in-reply-to". This will permit to the user to cite a mail and reply with a patch and keep Cc, To ...
See discussion :
https://public-inbox.org/git/[email protected]/
And Daniel Timothee and I wanted to refactor an other part of the file using parse_header_line(). Near Line 1570.
PATCH v3 1/1 send-email: extract email-parsing code into a subroutine
The existing code mixes parsing of email header with regular expression and actual code. Extract the parsing code into a new subroutine 'parse_header_line()'. This improves the code readability and make parse_header_line reusable in other place.
Thanks-to: Ævar Arnfjörð Bjarmason <[email protected]>
---
Without the "print" used for testing.
git-send-email.perl | 90 +++++++++++++++++++++++++++++++++++++---------------- 1 file changed, 63 insertions(+), 27 deletions(-)
git-send-email.perl +63 -27
@@ -715,41 +715,63 @@ EOT3 if (!defined $compose_encoding) { $compose_encoding = "UTF-8"; } - while(<$c>) { - next if m/^GIT:/; - if ($in_body) { - $summary_empty = 0 unless (/^\n$/); - } elsif (/^\n$/) { - $in_body = 1; - if ($need_8bit_cte) { + + my %parsed_email; + $parsed_email{'body'} = ''; + while (my $line = <$c>) { + next if $line =~ m/^GIT:/; + parse_header_line($line, \%parsed_email); + if ($line =~ /^\n$/i) { + while (my $body_line = <$c>) { + if ($body_line !~ m/^GIT:/) { + $parsed_email{'body'} = $parsed_email{'body'} . $body_line; + } + } + } + } + + if ($parsed_email{'from'}) { + $sender = $parsed_email{'from'}; + } + if ($parsed_email{'in-reply-to'}) { + $initial_reply_to = $parsed_email{'in-reply-to'}; + } + if ($parsed_email{'subject'}) { + $initial_subject = $parsed_email{'subject'}; + print $c2 "Subject: " . + quote_subject($parsed_email{'subject'}, $compose_encoding) . + "\n"; + } + if ($parsed_email{'mime-version'}) { + $need_8bit_cte = 0; + print $c2 "MIME-Version: $parsed_email{'mime-version'}\n", + "Content-Type: $parsed_email{'content-type'};\n", + "Content-Transfer-Encoding: $parsed_email{'content-transfer-encoding'}\n"; + } + if ($need_8bit_cte) { + if ($parsed_email{'content-type'}) { + print $c2 "MIME-Version: 1.0\n", + "Content-Type: $parsed_email{'content-type'};", + "Content-Transfer-Encoding: 8bit\n"; + } else { print $c2 "MIME-Version: 1.0\n", "Content-Type: text/plain; ", - "charset=$compose_encoding\n", + "charset=$compose_encoding\n", "Content-Transfer-Encoding: 8bit\n"; } - } elsif (/^MIME-Version:/i) { - $need_8bit_cte = 0; - } elsif (/^Subject:\s*(.+)\s*$/i) { - $initial_subject = $1; - my $subject = $initial_subject; - $_ = "Subject: " . - quote_subject($subject, $compose_encoding) . - "\n"; - } elsif (/^In-Reply-To:\s*(.+)\s*$/i) { - $initial_reply_to = $1; - next; - } elsif (/^From:\s*(.+)\s*$/i) { - $sender = $1; - next; - } elsif (/^(?:To|Cc|Bcc):/i) { - print __("To/Cc/Bcc fields are not interpreted yet, they have been ignored\n"); - next; - } - print $c2 $_; } + if ($parsed_email{'body'}) { + $summary_empty = 0; + print $c2 "\n$parsed_email{'body'}\n"; + } + close $c; close $c2; + open $c2, "<", $compose_filename . ".final" + or die sprintf(__("Failed to open %s.final: %s"), $compose_filename, $!); + close $c2; + if ($summary_empty) { print __("Summary email is empty, skipping it\n"); $compose = -1;
@@ -792,6 +814,20 @@ sub ask { return; } +sub parse_header_line { + my $lines = shift; + my $parsed_line = shift; + + foreach (split(/\n/, $lines)) { + if (/^(To|Cc|Bcc):\s*(.+)$/i) { + $parsed_line->{lc $1} = [ parse_address_line($2) ]; + } elsif (/^(From|Subject|Date|In-Reply-To|Message-ID|MIME-Version|Content-Type|Content-Transfer-Encoding|References):\s*(.+)\s*$/i) { + $parsed_line->{lc $1} = $2; + } + } +} + + my %broken_encoding; sub file_declares_8bit_cte {
Signature · 1 line
2.15.1
git-send-email.perl sub ask { · line 828
Quoted 12 lines · Nathan Payre
git-send-email.perl +12
-0+sub parse_header_line { + my $lines = shift; + my $parsed_line = shift; + + foreach (split(/\n/, $lines)) { + if (/^(To|Cc|Bcc):\s*(.+)$/i) { + $parsed_line->{lc $1} = [ parse_address_line($2) ]; + } elsif (/^(From|Subject|Date|In-Reply-To|Message-ID|MIME-Version|Content-Type|Content-Transfer-Encoding|References):\s*(.+)\s*$/i) { + $parsed_line->{lc $1} = $2; + } + } +}
Nit: As noted in my earlier review this results in very long lines, I'm just typing this pseudocode into an E-Mail client so not tested at all, but this would be better:
my $header_parsed = join "|", qw(To Cc Bcc);
my $header_unparsed = join "|", qw(From Subject Message-ID ...); #
line wrap this at some point
foreach [...]
if (/^($header_parsed)[...]
} elsif (^/($header_unparsed)[...].
PATCH v4 1/1 send-email: extract email-parsing code into a subroutine
In reply to Nathan PayreThe existing code mixes parsing of email header with regular expression and actual code. Extract the parsing code into a new subroutine "parse_header_line()". This improves the code readability and make parse_header_line reusable in other place.
"parsed_header_line()" and "filter_body()" could be used for refactoring the part of code which parses the header a last time to prepare the email and send it.
Thanks-to: Ævar Arnfjörð Bjarmason <[email protected]>
---
This patch fixes the indentation problem and reduce lines over 80 characters.
git-send-email.perl | 102 ++++++++++++++++++++++++++++++++++++++-------------- 1 file changed, 75 insertions(+), 27 deletions(-)
git-send-email.perl +75 -27
@@ -715,41 +715,60 @@ EOT3 if (!defined $compose_encoding) { $compose_encoding = "UTF-8"; } - while(<$c>) { - next if m/^GIT:/; - if ($in_body) { - $summary_empty = 0 unless (/^\n$/); - } elsif (/^\n$/) { - $in_body = 1; - if ($need_8bit_cte) { + + + my %parsed_email; + $parsed_email{'body'} = ''; + while (my $line = <$c>) { + next if $line =~ m/^GIT:/; + parse_header_line($line, \%parsed_email); + if ($line =~ /^\n$/i) { + $parsed_email{'body'} = filter_body($c); + } + } + + if ($parsed_email{'from'}) { + $sender = $parsed_email{'from'}; + } + if ($parsed_email{'in-reply-to'}) { + $initial_reply_to = $parsed_email{'in-reply-to'}; + } + if ($parsed_email{'subject'}) { + $initial_subject = $parsed_email{'subject'}; + print $c2 "Subject: " . + quote_subject($parsed_email{'subject'}, $compose_encoding) . + "\n"; + } + if ($parsed_email{'mime-version'}) { + $need_8bit_cte = 0; + print $c2 "MIME-Version: $parsed_email{'mime-version'}\n", + "Content-Type: $parsed_email{'content-type'};\n", + "Content-Transfer-Encoding: $parsed_email{'content-transfer-encoding'}\n"; + } + if ($need_8bit_cte) { + if ($parsed_email{'content-type'}) { + print $c2 "MIME-Version: 1.0\n", + "Content-Type: $parsed_email{'content-type'};", + "Content-Transfer-Encoding: 8bit\n"; + } else { print $c2 "MIME-Version: 1.0\n", "Content-Type: text/plain; ", - "charset=$compose_encoding\n", + "charset=$compose_encoding\n", "Content-Transfer-Encoding: 8bit\n"; } - } elsif (/^MIME-Version:/i) { - $need_8bit_cte = 0; - } elsif (/^Subject:\s*(.+)\s*$/i) { - $initial_subject = $1; - my $subject = $initial_subject; - $_ = "Subject: " . - quote_subject($subject, $compose_encoding) . - "\n"; - } elsif (/^In-Reply-To:\s*(.+)\s*$/i) { - $initial_reply_to = $1; - next; - } elsif (/^From:\s*(.+)\s*$/i) { - $sender = $1; - next; - } elsif (/^(?:To|Cc|Bcc):/i) { - print __("To/Cc/Bcc fields are not interpreted yet, they have been ignored\n"); - next; - } - print $c2 $_; } + if ($parsed_email{'body'}) { + $summary_empty = 0; + print $c2 "\n$parsed_email{'body'}\n"; + } + close $c; close $c2; + open $c2, "<", $compose_filename . ".final" + or die sprintf(__("Failed to open %s.final: %s"), $compose_filename, $!); + close $c2; + if ($summary_empty) { print __("Summary email is empty, skipping it\n"); $compose = -1;
@@ -792,6 +811,35 @@ sub ask { return; } +sub parse_header_line { + my $lines = shift; + my $parsed_line = shift; + my $pattern1 = join "|", qw(To Cc Bcc); + my $pattern2 = join "|", + qw(From Subject Date In-Reply-To Message-ID MIME-Version + Content-Type Content-Transfer-Encoding References); + + foreach (split(/\n/, $lines)) { + if (/^($pattern1):\s*(.+)$/i) { + $parsed_line->{lc $1} = [ parse_address_line($2) ]; + } elsif (/^($pattern2):\s*(.+)\s*$/i) { + $parsed_line->{lc $1} = $2; + } + } +} + +sub filter_body { + my $c = shift; + my $body = ""; + while (my $body_line = <$c>) { + if ($body_line !~ m/^GIT:/) { + $body = $body . $body_line; + } + } + return $body; +} + + my %broken_encoding; sub file_declares_8bit_cte {
Signature · 1 line
2.15.1
A message that is not in the archive (sent off-list or not yet received): [email protected]
Not terribly important, but your patch has trailing newlines. "git diff --staged --check" to see them. More below.
Quoted 2 lines · PAYRE NATHAN p1508475 <[email protected]> writes:
the part of code which parses the header a last time to prepare the email and send it.
The important point is not that it's the last time the code parses headers, so I'd drop the "a last time".
Review
Quoted 6 lines
(quoted diff) +6
-0+ my %parsed_email; + $parsed_email{'body'} = ''; + while (my $line = <$c>) { + next if $line =~ m/^GIT:/; + parse_header_line($line, \%parsed_email); + if ($line =~ /^\n$/i) {
You don't need the /i (case-Insensitive) here, there are no letters to match.
Review
Quoted 2 lines
(quoted diff) +2
-0+ if ($parsed_email{'mime-version'}) { + $need_8bit_cte = 0;
This $need_8bit_cte is a leftover of the old code, which processed the headers in the order it found them in the message and had to remember the content of MIME-Version while parsing Content-Type.
I believe you can apply this on top of your patch:
git-send-email.perl +1 -4
@@ -709,7 +709,6 @@ EOT3 open $c, "<", $compose_filename or die sprintf(__("Failed to open %s: %s"), $compose_filename, $!); - my $need_8bit_cte = file_has_nonascii($compose_filename); my $in_body = 0; my $summary_empty = 1; if (!defined $compose_encoding) {
@@ -740,12 +739,10 @@ EOT3 "\n"; } if ($parsed_email{'mime-version'}) { - $need_8bit_cte = 0; print $c2 "MIME-Version: $parsed_email{'mime-version'}\n", "Content-Type: $parsed_email{'content-type'};\n", "Content-Transfer-Encoding: $parsed_email{'content-transfer-encoding'}\n"; - } - if ($need_8bit_cte) { + } else if (file_has_nonascii($compose_filename)) { if ($parsed_email{'content-type'}) { print $c2 "MIME-Version: 1.0\n", "Content-Type: $parsed_email{'content-type'};",
It reads much better: "If the original message already had a MIME-Version header, then use that, else see if the file has non-ascii characters and if so, use MIME-Version: 1.0".
Actually, you can even simplify further by factoring the if/else below:
Review
Quoted 3 lines
(quoted diff) +3
-0+ if ($parsed_email{'content-type'}) { + print $c2 "MIME-Version: 1.0\n", + "Content-Type: $parsed_email{'content-type'};",
(Suspicious ";", and suspicious absence of "\n" here, I don't think it's intentional and I'm fixing it below, but correct me if I'm wrong)
Review
Quoted 2 lines
(quoted diff) +2
-0+ "Content-Transfer-Encoding: 8bit\n"; + } else {
(Broken indentation, this is not aligned with the "if" above)
Review
Quoted 6 lines
(quoted diff) +1
-1print $c2 "MIME-Version: 1.0\n", "Content-Type: text/plain; ", - "charset=$compose_encoding\n", + "charset=$compose_encoding\n", "Content-Transfer-Encoding: 8bit\n"; }
This could become stg like (untested):
} else if (file_has_nonascii($compose_filename)) {
my $content_type = ($parsed_email{'content-type'} or
"text/plain; charset=$compose_encoding");
print $c2 "MIME-Version: 1.0\n",
"Content-Type: $content_type\n",
"Content-Transfer-Encoding: 8bit\n";
}Review
Quoted 3 lines
(quoted diff) +3
-0+ open $c2, "<", $compose_filename . ".final" + or die sprintf(__("Failed to open %s.final: %s"), $compose_filename, $!); + close $c2;
What is this? Cut-and-paste mistake?
Quoted 4 lines
(quoted diff) +4
-0+sub parse_header_line { + my $lines = shift; + my $parsed_line = shift; + my $pattern1 = join "|", qw(To Cc Bcc);
Review
Quoted 10 lines
(quoted diff) +10
-0+ my $pattern2 = join "|", + qw(From Subject Date In-Reply-To Message-ID MIME-Version + Content-Type Content-Transfer-Encoding References); + + foreach (split(/\n/, $lines)) { + if (/^($pattern1):\s*(.+)$/i) { + $parsed_line->{lc $1} = [ parse_address_line($2) ]; + } elsif (/^($pattern2):\s*(.+)\s*$/i) { + $parsed_line->{lc $1} = $2; + }
I don't think you need to list the possibilities in the "else" branch. Just matching /^([^:]*):\s*(.+)\s*$/i should do the trick.
Review
Quoted 1 line
(quoted diff) +1
-0+ $body = $body . $body_line;
Or just: $body .= $body_line;
Signature · 2 lines
Matthieu Moy https://matthieu-moy.fr/
On Thu, Dec 07 2017, Matthieu Moy jotted:
Quoted 11 lines
Not terribly important, but your patch has trailing newlines. "git diff --staged --check" to see them. More below.
Quoted 2 lines · PAYRE NATHAN p1508475 <[email protected]> writes:
the part of code which parses the header a last time to prepare the email and send it.
The important point is not that it's the last time the code parses headers, so I'd drop the "a last time".
Quoted 9 lines
Review
Quoted 6 lines
(quoted diff) +6
-0+ my %parsed_email; + $parsed_email{'body'} = ''; + while (my $line = <$c>) { + next if $line =~ m/^GIT:/; + parse_header_line($line, \%parsed_email); + if ($line =~ /^\n$/i) {You don't need the /i (case-Insensitive) here, there are no letters to match.
Good catch, actually this can just be: /^$/. The $ syntax already matches the ending newline, no need for /^\n$/.
Quoted 9 lines
Quoted 9 lines
(quoted diff) +9
-0+sub parse_header_line { + my $lines = shift; + my $parsed_line = shift; + my $pattern1 = join "|", qw(To Cc Bcc); + my $pattern2 = join "|", + qw(From Subject Date In-Reply-To Message-ID MIME-Version + Content-Type Content-Transfer-Encoding References); + + foreach (split(/\n/, $lines)) {
Quoted 8 lines
Review
Quoted 5 lines
(quoted diff) +5
-0+ if (/^($pattern1):\s*(.+)$/i) { + $parsed_line->{lc $1} = [ parse_address_line($2) ]; + } elsif (/^($pattern2):\s*(.+)\s*$/i) { + $parsed_line->{lc $1} = $2; + }I don't think you need to list the possibilities in the "else" branch. Just matching /^([^:]*):\s*(.+)\s*$/i should do the trick.
Although you'll end up with a lot of stuff in the $parsed_line hash you don't need, which makes dumping it for debugging verbose.
I also wonder about multi-line headers, but then again that probably breaks already on e.g. Message-ID and Refererences, but that's an existing bug unrelated to this patch...
Quoted 3 lines
Review
Quoted 1 line
(quoted diff) +1
-0+ $body = $body . $body_line;Or just: $body .= $body_line;
Quoted 9 lines · Nathan PAYRE
Quoted 4 lines · Junio C Hamano
... throughout this patch, not limited to this section, indentation is strange and there seem to be many "print" that show messages that do not seem to be meant for end-user consumption. I can see that this aspires to improve the readability, but not quite yet ;-).
Hmmm I'm wondering who place thoses print in my code ! I will fix it fast. :-)
Don't make waste by being hasty, though. The print statements were bad, but funny indentation was more distracting and will be worse hindrance from the maintainabaility's point of view.
Thanks.
Quoted 7 lines · Nathan PAYRE
Quoted 5 lines · Junio C Hamano
Also "reusable in other place" is by itself not an unconditional plus, until readers can be convinced that that 'other place' really wants to be able to call this function. Is there some untold motivation behind this change---such as a planned update to actually use this helper subroutine?
Quoted 6 lines
This subroutine will be used to implement, initially a new option called "--quote-email", but became "--cite" added after "--in-reply-to". This will permit to the user to cite a mail and reply with a patch and keep Cc, To ...
See discussion :
https://public-inbox.org/git/[email protected]/
Although he didn't state so explicitly, please take Junio's question as a strong hint that you should update the commit message to include the above rationale/explanation about why you consider this a good change. The reason for including the motivation in the commit message is that you want, not only to convince Junio now that this is a useful change, but to convince future readers of the project history that this change is desirable.
A message that is not in the archive (sent off-list or not yet received): [email protected]
Quoted 1 line · PAYRE NATHAN p1508475 <[email protected]> writes:
Without the "print" used for testing.
But still smoe broken indentation:
Quoted 22 lines
git-send-email.perl | 90 +++++++++++++++++++++++++++++++++++++---------------- 1 file changed, 63 insertions(+), 27 deletions(-)git-send-email.perl +4
-7@@ -715,41 +715,63 @@ EOT3 if (!defined $compose_encoding) { $compose_encoding = "UTF-8"; } - while(<$c>) { - next if m/^GIT:/; - if ($in_body) { - $summary_empty = 0 unless (/^\n$/); - } elsif (/^\n$/) { - $in_body = 1; - if ($need_8bit_cte) { + + my %parsed_email; + $parsed_email{'body'} = ''; + while (my $line = <$c>) {
git-send-email.perl EOT3 · line 731
Quoted 10 lines
git-send-email.perl +10
-0+ next if $line =~ m/^GIT:/; + parse_header_line($line, \%parsed_email); + if ($line =~ /^\n$/i) { + while (my $body_line = <$c>) { + if ($body_line !~ m/^GIT:/) { + $parsed_email{'body'} = $parsed_email{'body'} . $body_line; + } + } + } + }
This may display properly in your text editor with your setting, but appears broken at least with tab-width=8. Don't mix tabs and spaces. The Git coding style is to indent with tabs.
To see what I mean, open the script in Emacs and type M-x whitespace-mode RET.
Signature · 2 lines
Matthieu Moy https://matthieu-moy.fr/
PATCH 1/1 send-email: extract email-parsing code into a subroutine
In reply to Nathan PAYREThe existing code mixes parsing of email header with regular expression and actual code. Extract the parsing code into a new subroutine "parse_header_line()". This improves the code readability and make parse_header_line reusable in other place.
"parsed_header_line()" and "filter_body()" could be used for refactoring the part of code which parses the header to prepare the email and send it.
Thanks-to: Ævar Arnfjörð Bjarmason <[email protected]>
---
I fixed the last reported problems and removed some other old variable as $need_8bit_cte.
git-send-email.perl | 110 ++++++++++++++++++++++++++++++++++------------------ 1 file changed, 73 insertions(+), 37 deletions(-)
git-send-email.perl +73 -37
@@ -685,7 +685,7 @@ Lines beginning in "GIT:" will be removed. Consider including an overall diffstat or table of contents for the patch you are writing. -Clear the body content if you don't wish to send a summary. +Clear the body content if you dont wish to send a summary. EOT2 From: $tpl_sender Subject: $tpl_subject
@@ -709,51 +709,61 @@ EOT3 open $c, "<", $compose_filename or die sprintf(__("Failed to open %s: %s"), $compose_filename, $!); - my $need_8bit_cte = file_has_nonascii($compose_filename); - my $in_body = 0; - my $summary_empty = 1; if (!defined $compose_encoding) { $compose_encoding = "UTF-8"; } - while(<$c>) { - next if m/^GIT:/; - if ($in_body) { - $summary_empty = 0 unless (/^\n$/); - } elsif (/^\n$/) { - $in_body = 1; - if ($need_8bit_cte) { - print $c2 "MIME-Version: 1.0\n", - "Content-Type: text/plain; ", - "charset=$compose_encoding\n", - "Content-Transfer-Encoding: 8bit\n"; - } - } elsif (/^MIME-Version:/i) { - $need_8bit_cte = 0; - } elsif (/^Subject:\s*(.+)\s*$/i) { - $initial_subject = $1; - my $subject = $initial_subject; - $_ = "Subject: " . - quote_subject($subject, $compose_encoding) . - "\n"; - } elsif (/^In-Reply-To:\s*(.+)\s*$/i) { - $initial_reply_to = $1; - next; - } elsif (/^From:\s*(.+)\s*$/i) { - $sender = $1; - next; - } elsif (/^(?:To|Cc|Bcc):/i) { - print __("To/Cc/Bcc fields are not interpreted yet, they have been ignored\n"); - next; + + + my %parsed_email; + $parsed_email{'body'} = ''; + while (my $line = <$c>) { + next if $line =~ m/^GIT:/; + parse_header_line($line, \%parsed_email); + if ($line =~ /^$/) { + $parsed_email{'body'} = filter_body($c); } - print $c2 $_; } - close $c; - close $c2; - if ($summary_empty) { + if ($parsed_email{'from'}) { + $sender = $parsed_email{'from'}; + } + if ($parsed_email{'in-reply-to'}) { + $initial_reply_to = $parsed_email{'in-reply-to'}; + } + if ($parsed_email{'subject'}) { + $initial_subject = $parsed_email{'subject'}; + print $c2 "Subject: " . + quote_subject($parsed_email{'subject'}, $compose_encoding) . + "\n"; + } + if ($parsed_email{'mime-version'}) { + print $c2 "MIME-Version: $parsed_email{'mime-version'}\n", + "Content-Type: $parsed_email{'content-type'};\n", + "Content-Transfer-Encoding: $parsed_email{'content-transfer-encoding'}\n"; + } + + if ($parsed_email{'content-type'}) { + print $c2 "MIME-Version: 1.0\n", + "Content-Type: $parsed_email{'content-type'};\n", + "Content-Transfer-Encoding: 8bit\n"; + } elsif (file_has_nonascii($compose_filename)) { + my $content_type = ($parsed_email{'content-type'} or + "text/plain; charset=$compose_encoding"); + print $c2 "MIME-Version: 1.0\n", + "Content-Type: $content_type\n", + "Content-Transfer-Encoding: 8bit\n"; + } + + if ($parsed_email{'body'}) { + print $c2 "\n$parsed_email{'body'}\n"; + } else { print __("Summary email is empty, skipping it\n"); $compose = -1; } + + close $c; + close $c2; + } elsif ($annotate) { do_edit(@files); }
@@ -792,6 +802,32 @@ sub ask { return; } +sub parse_header_line { + my $lines = shift; + my $parsed_line = shift; + my $pattern = join "|", qw(To Cc Bcc); + + foreach (split(/\n/, $lines)) { + if (/^($pattern):\s*(.+)$/i) { + $parsed_line->{lc $1} = [ parse_address_line($2) ]; + } elsif (/^([^:]*):\s*(.+)\s*$/i) { + $parsed_line->{lc $1} = $2; + } + } +} + +sub filter_body { + my $c = shift; + my $body = ""; + while (my $body_line = <$c>) { + if ($body_line !~ m/^GIT:/) { + $body .= $body_line; + } + } + return $body; +} + + my %broken_encoding; sub file_declares_8bit_cte {
Signature · 1 line
2.15.1
A message that is not in the archive (sent off-list or not yet received): [email protected]
git-send-email.perl Lines beginning in "GIT:" will be removed. · line 688
Quoted 8 lines · "PAYRE NATHAN p1508475" <[email protected]> wrote:
git-send-email.perl +1
-1@@ -685,7 +685,7 @@ Lines beginning in "GIT:" will be removed. Consider including an overall diffstat or table of contents for the patch you are writing. -Clear the body content if you don't wish to send a summary. +Clear the body content if you dont wish to send a summary.
This is not part of your patch. Use "git add -p" to specify exactly which hunks should go into the patch and don't let this kind of change end up in the version you send.
git-send-email.perl EOT3 · line 748
Quoted 9 lines
git-send-email.perl +7
-1+ my %parsed_email; + $parsed_email{'body'} = ''; + while (my $line = <$c>) { + next if $line =~ m/^GIT:/; + parse_header_line($line, \%parsed_email); + if ($line =~ /^$/) { + $parsed_email{'body'} = filter_body($c); } - print $c2 $_;
I didn't notice this at first, but you're modifying the behavior here:
the old code used to print to $c2 anything that didn't match any of the if/else if branches.
To keep this behavior, you need to keep all these extra headers in $parsed_email (you do, in this version) and print them after taking care of all the known headers (AFAICT, you don't).
git-send-email.perl
Quoted 3 lines
git-send-email.perl +0
-2} - close $c; - close $c2;
You'll still need $c2, but you don't need $c anymore, so I'd keep the "close $c" here. OTOH, $c2 is not needed before this point (actually a bit later), so it would make sense to move the "open" down a little. This would materialize the "read input, then write output" scheme (as opposed to "write output while reading input" in the previous code). It's not a new issue in your patch, but giving variables meaningful names (i.e. not $c and $c2) would help, too.
git-send-email.perl EOT3
Quoted 10 lines
git-send-email.perl +10
-0+ if ($parsed_email{'mime-version'}) { + print $c2 "MIME-Version: $parsed_email{'mime-version'}\n", + "Content-Type: $parsed_email{'content-type'};\n", + "Content-Transfer-Encoding: $parsed_email{'content-transfer-encoding'}\n"; + } + + if ($parsed_email{'content-type'}) { + print $c2 "MIME-Version: 1.0\n", + "Content-Type: $parsed_email{'content-type'};\n", + "Content-Transfer-Encoding: 8bit\n";
This "if ($parsed_email{'content-type'})" does not correspond to anything in the old code, and ...
git-send-email.perl
Quoted 3 lines
git-send-email.perl +3
-0+ } elsif (file_has_nonascii($compose_filename)) { + my $content_type = ($parsed_email{'content-type'} or + "text/plain; charset=$compose_encoding");
Here, your're dealing explicitly with $parsed_email{'content-type'} != false (you're in the 'else' branch where it can only be false).
I think you just meant to drop the "if ($parsed_email{'content-type'})" part, and plug the "elseif" directly after the "if ($parsed_email{'mime-version'})". That's what I suggested in my earlier email.
git-send-email.perl EOT3 · line 770
Quoted 6 lines
git-send-email.perl +6
-0+ my $content_type =3D ($parsed_email{'content-type'} or + "text/plain; charset=3D$compose_encoding"); + print $c2 "MIME-Version: 1.0\n", + "Content-Type: $content_type\n", + "Content-Transfer-Encoding: 8bit\n"; + }
This part is indented with spaces, please use tabs.
Signature · 2 lines
Matthieu Moy https://matthieu-moy.fr/
2017-12-11 22:12 GMT+01:00 Matthieu Moy <[email protected]>:
Quoted 11 lines
git-send-email.perl EOT3 · line 748
Quoted 9 lines · "PAYRE NATHAN p1508475" <[email protected]> wrote:
git-send-email.perl +7
-1+ my %parsed_email; + $parsed_email{'body'} = ''; + while (my $line = <$c>) { + next if $line =~ m/^GIT:/; + parse_header_line($line, \%parsed_email); + if ($line =~ /^$/) { + $parsed_email{'body'} = filter_body($c); } - print $c2 $_;
Quoted 7 lines
I didn't notice this at first, but you're modifying the behavior here:
the old code used to print to $c2 anything that didn't match any of the if/else if branches.To keep this behavior, you need to keep all these extra headers in $parsed_email (you do, in this version) and print them after taking care of all the known headers (AFAICT, you don't).
This case is not that easy to correct because:
- It's could weigh the code.
- The refactoring may not be legitimate anymore.
I've found two way to resolve this:
.1) After every if($parsed_email{'key'}) remove the corresponding key and just before closing $c2 create a new loop which add all the remaining parts.
.2) Making a mix between the old and new code. Some parts of my patch can improve the old code (like the removing of $need_8bit_cte) then it will be kept and the while loop will be similar the old code
I think that the first version will look like better than the second one, easy to read, but it will change the order of the email header.
PATCH 1/1 send-email: extract email-parsing code into a subroutine
The existing code mixes parsing of email header with regular expression and actual code. Extract the parsing code into a new subroutine "parse_header_line()". This improves the code readability and make parse_header_line reusable in other place.
"parsed_header_line()" and "filter_body()" could be used for refactoring the part of code which parses the header to prepare the email and send it.
Thanks-to: Ævar Arnfjörð Bjarmason <[email protected]>
---
Quoted 28 lines
Quoted 11 lines
Review
Quoted 9 lines · "PAYRE NATHAN p1508475" <[email protected]> wrote:
(quoted diff) +7
-1+ my %parsed_email; + $parsed_email{'body'} = ''; + while (my $line = <$c>) { + next if $line =~ m/^GIT:/; + parse_header_line($line, \%parsed_email); + if ($line =~ /^$/) { + $parsed_email{'body'} = filter_body($c); } - print $c2 $_;Quoted 7 lines
I didn't notice this at first, but you're modifying the behavior here:
the old code used to print to $c2 anything that didn't match any of the if/else if branches.To keep this behavior, you need to keep all these extra headers in $parsed_email (you do, in this version) and print them after taking care of all the known headers (AFAICT, you don't).
This case is not that easy to correct because:
- It's could weigh the code.
- The refactoring may not be legitimate anymore.I've found two way to resolve this:
.1) After everyif($parsed_email{'key'})remove the corresponding key and just before closing $c2 create a new loop which add all the remaining parts.
Quoted 7 lines
.2) Making a mix between the old and new code. Some parts of my patch can improve the old code (like the removing of $need_8bit_cte) then it will be kept and the while loop will be similar the old code
I think that the first version will look like better than the second one, easy to read, but it will change the order of the email header.
This is how I see the first choice of the two I've proposed in my last email.
git-send-email.perl | 116
+++++++++++++++++++++++++++++++++++----------------- 1 file changed, 78 insertions(+), 38 deletions(-)
git-send-email.perl +78 -38
@@ -703,57 +703,71 @@ EOT3 do_edit($compose_filename); } - open my $c2, ">", $compose_filename . ".final" - or die sprintf(__("Failed to open %s.final: %s"), $compose_filename, $!); - open $c, "<", $compose_filename or die sprintf(__("Failed to open %s: %s"), $compose_filename, $!); - my $need_8bit_cte = file_has_nonascii($compose_filename); - my $in_body = 0; - my $summary_empty = 1; if (!defined $compose_encoding) { $compose_encoding = "UTF-8"; } - while(<$c>) { - next if m/^GIT:/; - if ($in_body) { - $summary_empty = 0 unless (/^\n$/); - } elsif (/^\n$/) { - $in_body = 1; - if ($need_8bit_cte) { - print $c2 "MIME-Version: 1.0\n", - "Content-Type: text/plain; ", - "charset=$compose_encoding\n", - "Content-Transfer-Encoding: 8bit\n"; - } - } elsif (/^MIME-Version:/i) { - $need_8bit_cte = 0; - } elsif (/^Subject:\s*(.+)\s*$/i) { - $initial_subject = $1; - my $subject = $initial_subject; - $_ = "Subject: " . - quote_subject($subject, $compose_encoding) . - "\n"; - } elsif (/^In-Reply-To:\s*(.+)\s*$/i) { - $initial_reply_to = $1; - next; - } elsif (/^From:\s*(.+)\s*$/i) { - $sender = $1; - next; - } elsif (/^(?:To|Cc|Bcc):/i) { - print __("To/Cc/Bcc fields are not interpreted yet, they have been ignored\n"); - next; + + my %parsed_email; + while (my $line = <$c>) { + next if $line =~ m/^GIT:/; + parse_header_line($line, \%parsed_email); + if ($line =~ /^$/) { + $parsed_email{'body'} = filter_body($c); } - print $c2 $_; } + close $c; - close $c2; - if ($summary_empty) { + open my $c2, ">", $compose_filename . ".final" + or die sprintf(__("Failed to open %s.final: %s"), $compose_filename, $!); + + + if ($parsed_email{'From'}) { + $sender = delete($parsed_email{'From'}); + } + if ($parsed_email{'In-Reply-To'}) { + $initial_reply_to = delete($parsed_email{'In-Reply-To'}); + } + if ($parsed_email{'Subject'}) { + $initial_subject = delete($parsed_email{'Subject'}); + print $c2 "Subject: " . + quote_subject($initial_subject, $compose_encoding) . + "\n"; + } + + if ($parsed_email{'MIME-Version'}) { + print $c2 "MIME-Version: $parsed_email{'MIME-Version'}\n", + "Content-Type: $parsed_email{'Content-Type'};\n", + "Content-Transfer-Encoding: $parsed_email{'Content-Transfer-Encoding'}\n"; + delete($parsed_email{'MIME-Version'}); + delete($parsed_email{'Content-Type'}); + delete($parsed_email{'Content-Transfer-Encoding'}); + } elsif (file_has_nonascii($compose_filename)) { + my $content_type = (delete($parsed_email{'Content-Type'}) or + "text/plain; charset=$compose_encoding"); + print $c2 "MIME-Version: 1.0\n", + "Content-Type: $content_type\n", + "Content-Transfer-Encoding: 8bit\n"; + } + + foreach my $key (keys %parsed_email) { + next if $key == 'body'; + print $c2 "$key: $parsed_email{$key}"; + } + + if ($parsed_email{'body'}) { + print $c2 "\n$parsed_email{'body'}\n"; + delete($parsed_email{'body'}); + } else { print __("Summary email is empty, skipping it\n"); $compose = -1; } + + close $c2; + } elsif ($annotate) { do_edit(@files); }
@@ -792,6 +806,32 @@ sub ask { return; } +sub parse_header_line { + my $lines = shift; + my $parsed_line = shift; + my $pattern = join "|", qw(To Cc Bcc); + + foreach (split(/\n/, $lines)) { + if (/^($pattern):\s*(.+)$/i) { + $parsed_line->{$1} = [ parse_address_line($2) ]; + } elsif (/^([^:]*):\s*(.+)\s*$/i) { + $parsed_line->{$1} = $2; + } + } +} + +sub filter_body { + my $c = shift; + my $body = ""; + while (my $body_line = <$c>) { + if ($body_line !~ m/^GIT:/) { + $body .= $body_line; + } + } + return $body; +} + + my %broken_encoding; sub file_declares_8bit_cte {
Signature · 1 line
2.15.1
A message that is not in the archive (sent off-list or not yet received): [email protected]
Review
Quoted 4 lines · "PAYRE NATHAN p1508475" <[email protected]> wrote:
(quoted diff) +1
-1- print $c2 $_; } + close $c;
Nit: this added newline does not seem necessary to me. Nothing serious, but this kind of thing tend to distract the reader when reviewing the patch.
Review
Quoted 4 lines
(quoted diff) +4
-0+ foreach my $key (keys %parsed_email) { + next if $key == 'body'; + print $c2 "$key: $parsed_email{$key}"; + }
I'd add a comment like
# Preserve unknown headers
at the top of the loop to make it clear what we're doing.
On a side note: there's no comment in the code you're adding. This is not necessarily a bad thing (beautifully written code does not need comments to be readable), but you may re-read your code with the question "did I explain everything well-enough?" in mind. The loop above is a case where IMHO a short and sweet comment helps the reader.
Two potential issues not mentionned in your message but that we discussed offlist is that 1) this doesn't preserve the order, and 2) this strips duplicate headers. I believe this is not a problem here, and trying to solve these points would make the code overkill, but this would really deserve being mentionned in the commit message. First, so that people reviewing your patch now can confirm (or not) that you are taking the right decision by doing this, and also for people in the future examining your patch (e.g. after a bisect).
Review
Quoted 4 lines
(quoted diff) +4
-0+sub parse_header_line { + my $lines = shift; + my $parsed_line = shift; + my $pattern = join "|", qw(To Cc Bcc);
Nit: you may want to rename it to something more explicit, like $addr_headers_pat.
None of my nit should block the patch inclusion, but I think the commit message should be expanded to include a mention of the "duplicate headers"/"header order" potential issue.
Signature · 2 lines
Matthieu Moy https://matthieu-moy.fr/
PATCH 1/1 send-email: extract email-parsing code into a subroutine
The existing code mixes parsing of email header with regular expression and actual code. Extract the parsing code into a new subroutine "parse_header_line()". This improves the code readability and make parse_header_line reusable in other place.
"parsed_header_line()" and "filter_body()" could be used for refactoring the part of code which parses the header to prepare the email and send it.
In contrast to the previous version it doesn't keep the header order and strip duplicate headers.
Thanks-to: Ævar Arnfjörð Bjarmason <[email protected]>
---
Quoted 7 lines
Review
Quoted 4 lines
(quoted diff) +4
-0+sub parse_header_line { + my $lines = shift; + my $parsed_line = shift; + my $pattern = join "|", qw(To Cc Bcc);Nit: you may want to rename it to something more explicit, like $addr_headers_pat.
I find "$addr_headers_pat" too long that's why I've choose rename it into "$addr_pat", in addition to that, because the variable is in the subroutine "parse_header_line" it does not require to include "headers" in the variable name.
git-send-email.perl | 115 +++++++++++++++++++++++++++++++++++----------------- 1 file changed, 77 insertions(+), 38 deletions(-)
git-send-email.perl +77 -38
@@ -703,57 +703,70 @@ EOT3 do_edit($compose_filename); } - open my $c2, ">", $compose_filename . ".final" - or die sprintf(__("Failed to open %s.final: %s"), $compose_filename, $!); - open $c, "<", $compose_filename or die sprintf(__("Failed to open %s: %s"), $compose_filename, $!); - my $need_8bit_cte = file_has_nonascii($compose_filename); - my $in_body = 0; - my $summary_empty = 1; if (!defined $compose_encoding) { $compose_encoding = "UTF-8"; } - while(<$c>) { - next if m/^GIT:/; - if ($in_body) { - $summary_empty = 0 unless (/^\n$/); - } elsif (/^\n$/) { - $in_body = 1; - if ($need_8bit_cte) { - print $c2 "MIME-Version: 1.0\n", - "Content-Type: text/plain; ", - "charset=$compose_encoding\n", - "Content-Transfer-Encoding: 8bit\n"; - } - } elsif (/^MIME-Version:/i) { - $need_8bit_cte = 0; - } elsif (/^Subject:\s*(.+)\s*$/i) { - $initial_subject = $1; - my $subject = $initial_subject; - $_ = "Subject: " . - quote_subject($subject, $compose_encoding) . - "\n"; - } elsif (/^In-Reply-To:\s*(.+)\s*$/i) { - $initial_reply_to = $1; - next; - } elsif (/^From:\s*(.+)\s*$/i) { - $sender = $1; - next; - } elsif (/^(?:To|Cc|Bcc):/i) { - print __("To/Cc/Bcc fields are not interpreted yet, they have been ignored\n"); - next; + + my %parsed_email; + while (my $line = <$c>) { + next if $line =~ m/^GIT:/; + parse_header_line($line, \%parsed_email); + if ($line =~ /^$/) { + $parsed_email{'body'} = filter_body($c); } - print $c2 $_; } close $c; - close $c2; - if ($summary_empty) { + open my $c2, ">", $compose_filename . ".final" + or die sprintf(__("Failed to open %s.final: %s"), $compose_filename, $!); + + + if ($parsed_email{'From'}) { + $sender = delete($parsed_email{'From'}); + } + if ($parsed_email{'In-Reply-To'}) { + $initial_reply_to = delete($parsed_email{'In-Reply-To'}); + } + if ($parsed_email{'Subject'}) { + $initial_subject = delete($parsed_email{'Subject'}); + print $c2 "Subject: " . + quote_subject($initial_subject, $compose_encoding) . + "\n"; + } + + if ($parsed_email{'MIME-Version'}) { + print $c2 "MIME-Version: $parsed_email{'MIME-Version'}\n", + "Content-Type: $parsed_email{'Content-Type'};\n", + "Content-Transfer-Encoding: $parsed_email{'Content-Transfer-Encoding'}\n"; + delete($parsed_email{'MIME-Version'}); + delete($parsed_email{'Content-Type'}); + delete($parsed_email{'Content-Transfer-Encoding'}); + } elsif (file_has_nonascii($compose_filename)) { + my $content_type = (delete($parsed_email{'Content-Type'}) or + "text/plain; charset=$compose_encoding"); + print $c2 "MIME-Version: 1.0\n", + "Content-Type: $content_type\n", + "Content-Transfer-Encoding: 8bit\n"; + } + # Preserve unknown headers + foreach my $key (keys %parsed_email) { + next if $key eq 'body'; + print $c2 "$key: $parsed_email{$key}"; + } + + if ($parsed_email{'body'}) { + print $c2 "\n$parsed_email{'body'}\n"; + delete($parsed_email{'body'}); + } else { print __("Summary email is empty, skipping it\n"); $compose = -1; } + + close $c2; + } elsif ($annotate) { do_edit(@files); }
@@ -792,6 +805,32 @@ sub ask { return; } +sub parse_header_line { + my $lines = shift; + my $parsed_line = shift; + my $addr_pat = join "|", qw(To Cc Bcc); + + foreach (split(/\n/, $lines)) { + if (/^($addr_pat):\s*(.+)$/i) { + $parsed_line->{$1} = [ parse_address_line($2) ]; + } elsif (/^([^:]*):\s*(.+)\s*$/i) { + $parsed_line->{$1} = $2; + } + } +} + +sub filter_body { + my $c = shift; + my $body = ""; + while (my $body_line = <$c>) { + if ($body_line !~ m/^GIT:/) { + $body .= $body_line; + } + } + return $body; +} + + my %broken_encoding; sub file_declares_8bit_cte {
Signature · 1 line
2.15.1
A message that is not in the archive (sent off-list or not yet received): [email protected]
Matthieu Moy
Reviewed-byQuoted 5 lines · PAYRE NATHAN p1508475 <[email protected]> writes:
Quoted 5 lines
Quoted 4 lines
(quoted diff) +4
-0+sub parse_header_line { + my $lines = shift; + my $parsed_line = shift; + my $pattern = join "|", qw(To Cc Bcc);
Quoted 7 lines
Quoted 2 lines
Nit: you may want to rename it to something more explicit, like $addr_headers_pat.
I find "$addr_headers_pat" too long that's why I've choose rename it into "$addr_pat", in addition to that, because the variable is in the subroutine "
parse_header_line" it does not require to include "headers" in the variable name.
I suggested this name because $addr_pat seems to imply that this matches an address, while it matches the _name of headers_ containing address. But that's not terribly important, the meaning is clear by the context anyway.
All my previous remarks have been taken into account. This is now
Thanks,
Signature · 2 lines
Matthieu Moy https://matthieu-moy.fr/
PATCH v2 1/1 send-email: extract email-parsing code into a subroutine
In reply to Nathan PAYREThe existing code mixes parsing of email header with regular expression and actual code. Extract the parsing code into a new subroutine 'parse_header_line()'. This improves the code readability and make parse_header_line reusable in other place.
Thanks-to: Ævar Arnfjörð Bjarmason <[email protected]>
--- git-send-email.perl | 100 ++++++++++++++++++++++++++++++++++++++-------------- 1 file changed, 73 insertions(+), 27 deletions(-)
git-send-email.perl +73 -27
@@ -715,41 +715,73 @@ EOT3 if (!defined $compose_encoding) { $compose_encoding = "UTF-8"; } - while(<$c>) { - next if m/^GIT:/; - if ($in_body) { - $summary_empty = 0 unless (/^\n$/); - } elsif (/^\n$/) { - $in_body = 1; - if ($need_8bit_cte) { + + my %parsed_email; + $parsed_email{'body'} = ''; + while (my $line = <$c>) { + next if $line =~ m/^GIT:/; + parse_header_line($line, \%parsed_email); + if ($line =~ /^\n$/i) { + while (my $body_line = <$c>) { + if ($body_line !~ m/^GIT:/) { + $parsed_email{'body'} = $parsed_email{'body'} . $body_line; + } + } + } + print "la : $line\n"; + } + + if ($parsed_email{'from'}) { + $sender = $parsed_email{'from'}; + } + if ($parsed_email{'in-reply-to'}) { + $initial_reply_to = $parsed_email{'in-reply-to'}; + } + if ($parsed_email{'subject'}) { + $initial_subject = $parsed_email{'subject'}; + print $c2 "Subject: " . + quote_subject($parsed_email{'subject'}, $compose_encoding) . + "\n"; + } + if ($parsed_email{'mime-version'}) { + print "CASE 0\n"; + $need_8bit_cte = 0; + print $c2 "MIME-Version: $parsed_email{'mime-version'}\n", + "Content-Type: $parsed_email{'content-type'};\n", + "Content-Transfer-Encoding: $parsed_email{'content-transfer-encoding'}\n"; + } + if ($need_8bit_cte) { + if ($parsed_email{'content-type'}) { + print "CASE 1\n"; + print $c2 "MIME-Version: 1.0\n", + "Content-Type: $parsed_email{'content-type'};", + "Content-Transfer-Encoding: 8bit\n"; + } else { + print "CASE 2\n"; print $c2 "MIME-Version: 1.0\n", "Content-Type: text/plain; ", - "charset=$compose_encoding\n", + "charset=$compose_encoding\n", "Content-Transfer-Encoding: 8bit\n"; } - } elsif (/^MIME-Version:/i) { - $need_8bit_cte = 0; - } elsif (/^Subject:\s*(.+)\s*$/i) { - $initial_subject = $1; - my $subject = $initial_subject; - $_ = "Subject: " . - quote_subject($subject, $compose_encoding) . - "\n"; - } elsif (/^In-Reply-To:\s*(.+)\s*$/i) { - $initial_reply_to = $1; - next; - } elsif (/^From:\s*(.+)\s*$/i) { - $sender = $1; - next; - } elsif (/^(?:To|Cc|Bcc):/i) { - print __("To/Cc/Bcc fields are not interpreted yet, they have been ignored\n"); - next; - } - print $c2 $_; } + if ($parsed_email{'body'}) { + $summary_empty = 0; + print $c2 "\n$parsed_email{'body'}\n"; + } + close $c; close $c2; + open $c2, "<", $compose_filename . ".final" + or die sprintf(__("Failed to open %s.final: %s"), $compose_filename, $!); + + print "affichage : \n"; + while (<$c2>) { + print $_; + } + + close $c2; + if ($summary_empty) { print __("Summary email is empty, skipping it\n"); $compose = -1;
@@ -792,6 +824,20 @@ sub ask { return; } +sub parse_header_line { + my $lines = shift; + my $parsed_line = shift; + + foreach (split(/\n/, $lines)) { + if (/^(To|Cc|Bcc):\s*(.+)$/i) { + $parsed_line->{lc $1} = [ parse_address_line($2) ]; + } elsif (/^(From|Subject|Date|In-Reply-To|Message-ID|MIME-Version|Content-Type|Content-Transfer-Encoding|References):\s*(.+)\s*$/i) { + $parsed_line->{lc $1} = $2; + } + } +} + + my %broken_encoding; sub file_declares_8bit_cte {
Signature · 1 line
2.15.1