{"thread":{"id":"47366","subject":"[PATCH] send-email: extract email-parsing code into a subroutine","startedAt":"2017-12-02T17:03:10Z","lastAt":"2017-12-15T15:45:20Z","messageCount":25,"participants":["Payre Nathan","Nathan PAYRE","Matthieu Moy","Ævar Arnfjörð Bjarmason","Junio C Hamano","Nathan Payre","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"333959","messageId":"20171202170220.10073-1-second.payre@gmail.com","threadId":"47366","inReplyTo":null,"subject":"[PATCH] send-email: extract email-parsing code into a subroutine","fromName":"Payre Nathan","fromEmail":"second.payre@gmail.com","sentAt":"2017-12-02T17:02:20Z","receivedAt":"2017-12-02T17:03:10Z","isPatch":true,"sender":{"key":"second.payre@gmail.com","avatar":null},"body":"From: Nathan Payre \n\nThe existing code mixes parsing of email header with regular\nexpression and actual code. Extract the parsing code into a new\nsubroutine 'parse_header_line()'. This improves the code readability\nand make parse_header_line reusable in other place.\n\nSigned-off-by: Nathan Payre \nSigned-off-by: Matthieu Moy \nSigned-off-by: Timothee Albertin \nSigned-off-by: Daniel Bensoussan \n---\n\nThis patch is a first step to implement a new feature.\nSee new feature discussion here: https://public-inbox.org/git/20171030223444.5052-1-nathan.payre@etu.univ-lyon1.fr/\n\n git-send-email.perl | 106 +++++++++++++++++++++++++++++++++++++++-------------\n 1 file changed, 80 insertions(+), 26 deletions(-)\n\ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex 2208dcc21..98c2e461c 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -715,41 +715,64 @@ EOT3\n \tif (!defined $compose_encoding) {\n \t\t$compose_encoding = \"UTF-8\";\n \t}\n-\twhile(<$c>) {\n+\n+\tmy %parsed_email;\n+\twhile (<$c>) {\n \t\tnext if m/^GIT:/;\n-\t\tif ($in_body) {\n-\t\t\t$summary_empty = 0 unless (/^\\n$/);\n-\t\t} elsif (/^\\n$/) {\n-\t\t\t$in_body = 1;\n-\t\t\tif ($need_8bit_cte) {\n+\t\tparse_header_line($_, \\%parsed_email);\n+\t\tif (/^\\n$/i) {\n+\t\t\twhile (my $row = <$c>) {\n+\t\t\t\tif (!($row =~ m/^GIT:/)) {\n+\t\t\t\t\t$parsed_email{'body'} = $parsed_email{'body'} . $row;\n+\t\t\t\t}\n+\t\t\t}\n+\t\t}\n+\t}\n+\tif ($parsed_email{'from'}) {\n+\t\t$sender = $parsed_email{'from'};\n+\t}\n+\tif ($parsed_email{'in_reply_to'}) {\n+\t\t$initial_reply_to = $parsed_email{'in_reply_to'};\n+\t}\n+\tif ($parsed_email{'subject'}) {\n+\t\t$initial_subject = $parsed_email{'subject'};\n+\t\tprint $c2 \"Subject: \" .\n+\t\t\tquote_subject($parsed_email{'subject'}, $compose_encoding) .\n+\t\t\t\"\\n\";\n+\t}\n+\tif ($parsed_email{'mime-version'}) {\n+\t\t$need_8bit_cte = 0;\n+\t}\n+\tif ($need_8bit_cte) {\n+\t\tif ($parsed_email{'content-type'}) {\n+\t\t\t\tprint $c2 \"MIME-Version: 1.0\\n\",\n+\t\t\t\t\t \"Content-Type: $parsed_email{'content-type'};\",\n+\t\t\t\t\t \"Content-Transfer-Encoding: 8bit\\n\";\n+\t\t\t} else {\n \t\t\t\tprint $c2 \"MIME-Version: 1.0\\n\",\n \t\t\t\t\t \"Content-Type: text/plain; \",\n-\t\t\t\t\t \"charset=$compose_encoding\\n\",\n+\t\t\t\t\t \"charset=$compose_encoding\\n\",\n \t\t\t\t\t \"Content-Transfer-Encoding: 8bit\\n\";\n \t\t\t}\n-\t\t} elsif (/^MIME-Version:/i) {\n-\t\t\t$need_8bit_cte = 0;\n-\t\t} elsif (/^Subject:\\s*(.+)\\s*$/i) {\n-\t\t\t$initial_subject = $1;\n-\t\t\tmy $subject = $initial_subject;\n-\t\t\t$_ = \"Subject: \" .\n-\t\t\t\tquote_subject($subject, $compose_encoding) .\n-\t\t\t\t\"\\n\";\n-\t\t} elsif (/^In-Reply-To:\\s*(.+)\\s*$/i) {\n-\t\t\t$initial_reply_to = $1;\n-\t\t\tnext;\n-\t\t} elsif (/^From:\\s*(.+)\\s*$/i) {\n-\t\t\t$sender = $1;\n-\t\t\tnext;\n-\t\t} elsif (/^(?:To|Cc|Bcc):/i) {\n-\t\t\tprint __(\"To/Cc/Bcc fields are not interpreted yet, they have been ignored\\n\");\n-\t\t\tnext;\n-\t\t}\n-\t\tprint $c2 $_;\n \t}\n+\tif ($parsed_email{'body'}) {\n+\t\t$summary_empty = 0;\n+\t\tprint $c2 \"\\n$parsed_email{'body'}\\n\";\n+\t}\n+\n \tclose $c;\n \tclose $c2;\n \n+\topen $c2, \"<\", $compose_filename . \".final\"\n+\t\tor die sprintf(__(\"Failed to open %s.final: %s\"), $compose_filename, $!);\n+\n+\tprint \"affichage : \\n\";\n+\twhile (<$c2>) {\n+\t\tprint $_;\n+\t}\n+\n+\tclose $c2;\n+\n \tif ($summary_empty) {\n \t\tprint __(\"Summary email is empty, skipping it\\n\");\n \t\t$compose = -1;\n@@ -792,6 +815,37 @@ sub ask {\n \treturn;\n }\n \n+sub parse_header_line {\n+\tmy $lines = shift;\n+\tmy $parsed_line = shift;\n+\n+\tforeach (split(/\\n/, $lines)) {\n+\t\tif (/^From:\\s*(.+)$/i) {\n+\t\t\t$parsed_line->{'from'} = $1;\n+\t\t} elsif (/^To:\\s*(.+)$/i) {\n+\t\t\t$parsed_line->{'to'} = [ parse_address_line($1) ];\n+\t\t} elsif (/^Cc:\\s*(.+)$/i) {\n+\t\t\t$parsed_line->{'cc'} = [ parse_address_line($1) ];\n+\t\t} elsif (/^Bcc:\\s*(.+)$/i) {\n+\t\t\t$parsed_line->{'bcc'} = [ parse_address_line($1) ];\n+\t\t} elsif (/^Subject:\\s*(.+)\\s*$/i) {\n+\t\t\t$parsed_line->{'subject'} = $1;\n+\t\t} elsif (/^Date: (.*)/i) {\n+\t\t\t$parsed_line->{'date'} = $1;\n+\t\t} elsif (/^In-Reply-To:\\s*(.+)\\s*$/i) {\n+\t\t\t$parsed_line->{'in_reply_to'} = $1;\n+\t\t} elsif (/^Message-ID: (.*)$/i) {\n+\t\t\t$parsed_line->{'message_id'} = $1;\n+\t\t} elsif (/^MIME-Version:$/i) {\n+\t\t\t$parsed_line->{'mime-version'} = $1;\n+\t\t} elsif (/^Content-Type:\\s+(.*)\\s*$/i) {\n+\t\t\t$parsed_line->{'content-type'} = $1;\n+\t\t} elsif (/^References:\\s+(.*)/i) {\n+\t\t\t$parsed_line->{'references'} = $1;\n+\t\t}\n+\t}\n+}\n+\n my %broken_encoding;\n \n sub file_declares_8bit_cte {\n-- \n2.15.1\n\n"},{"id":"333960","messageId":"CAGb4CBUiOGUio=DY4Atm+4E=FqMn-aV3_vRCUKV_enjkUk4A_w@mail.gmail.com","threadId":"47366","inReplyTo":"20171202170220.10073-1-second.payre@gmail.com","subject":"Re: [PATCH] send-email: extract email-parsing code into a subroutine","fromName":"Nathan PAYRE","fromEmail":"second.payre@gmail.com","sentAt":"2017-12-02T17:11:45Z","receivedAt":"2017-12-02T17:11:53Z","isPatch":true,"sender":{"key":"second.payre@gmail.com","avatar":null},"body":"I found a mistake in my signed-off-by, please replace\n by \n\nExcuse me.\n\n2017-12-02 18:02 GMT+01:00 Payre Nathan :\n> From: Nathan Payre \n>\n> The existing code mixes parsing of email header with regular\n> expression and actual code. Extract the parsing code into a new\n> subroutine 'parse_header_line()'. This improves the code readability\n> and make parse_header_line reusable in other place.\n>\n> Signed-off-by: Nathan Payre \n> Signed-off-by: Matthieu Moy \n> Signed-off-by: Timothee Albertin \n> Signed-off-by: Daniel Bensoussan \n> ---\n>\n> This patch is a first step to implement a new feature.\n> See new feature discussion here: https://public-inbox.org/git/20171030223444.5052-1-nathan.payre@etu.univ-lyon1.fr/\n>\n> git-send-email.perl | 106 +++++++++++++++++++++++++++++++++++++++-------------\n> 1 file changed, 80 insertions(+), 26 deletions(-)\n>\n> diff --git a/git-send-email.perl b/git-send-email.perl\n> index 2208dcc21..98c2e461c 100755\n> --- a/git-send-email.perl\n> +++ b/git-send-email.perl\n> @@ -715,41 +715,64 @@ EOT3\n> if (!defined $compose_encoding) {\n> $compose_encoding = \"UTF-8\";\n> }\n> - while(<$c>) {\n> +\n> + my %parsed_email;\n> + while (<$c>) {\n> next if m/^GIT:/;\n> - if ($in_body) {\n> - $summary_empty = 0 unless (/^\\n$/);\n> - } elsif (/^\\n$/) {\n> - $in_body = 1;\n> - if ($need_8bit_cte) {\n> + parse_header_line($_, \\%parsed_email);\n> + if (/^\\n$/i) {\n> + while (my $row = <$c>) {\n> + if (!($row =~ m/^GIT:/)) {\n> + $parsed_email{'body'} = $parsed_email{'body'} . $row;\n> + }\n> + }\n> + }\n> + }\n> + if ($parsed_email{'from'}) {\n> + $sender = $parsed_email{'from'};\n> + }\n> + if ($parsed_email{'in_reply_to'}) {\n> + $initial_reply_to = $parsed_email{'in_reply_to'};\n> + }\n> + if ($parsed_email{'subject'}) {\n> + $initial_subject = $parsed_email{'subject'};\n> + print $c2 \"Subject: \" .\n> + quote_subject($parsed_email{'subject'}, $compose_encoding) .\n> + \"\\n\";\n> + }\n> + if ($parsed_email{'mime-version'}) {\n> + $need_8bit_cte = 0;\n> + }\n> + if ($need_8bit_cte) {\n> + if ($parsed_email{'content-type'}) {\n> + print $c2 \"MIME-Version: 1.0\\n\",\n> + \"Content-Type: $parsed_email{'content-type'};\",\n> + \"Content-Transfer-Encoding: 8bit\\n\";\n> + } else {\n> print $c2 \"MIME-Version: 1.0\\n\",\n> \"Content-Type: text/plain; \",\n> - \"charset=$compose_encoding\\n\",\n> + \"charset=$compose_encoding\\n\",\n> \"Content-Transfer-Encoding: 8bit\\n\";\n> }\n> - } elsif (/^MIME-Version:/i) {\n> - $need_8bit_cte = 0;\n> - } elsif (/^Subject:\\s*(.+)\\s*$/i) {\n> - $initial_subject = $1;\n> - my $subject = $initial_subject;\n> - $_ = \"Subject: \" .\n> - quote_subject($subject, $compose_encoding) .\n> - \"\\n\";\n> - } elsif (/^In-Reply-To:\\s*(.+)\\s*$/i) {\n> - $initial_reply_to = $1;\n> - next;\n> - } elsif (/^From:\\s*(.+)\\s*$/i) {\n> - $sender = $1;\n> - next;\n> - } elsif (/^(?:To|Cc|Bcc):/i) {\n> - print __(\"To/Cc/Bcc fields are not interpreted yet, they have been ignored\\n\");\n> - next;\n> - }\n> - print $c2 $_;\n> }\n> + if ($parsed_email{'body'}) {\n> + $summary_empty = 0;\n> + print $c2 \"\\n$parsed_email{'body'}\\n\";\n> + }\n> +\n> close $c;\n> close $c2;\n>\n> + open $c2, \"<\", $compose_filename . \".final\"\n> + or die sprintf(__(\"Failed to open %s.final: %s\"), $compose_filename, $!);\n> +\n> + print \"affichage : \\n\";\n> + while (<$c2>) {\n> + print $_;\n> + }\n> +\n> + close $c2;\n> +\n> if ($summary_empty) {\n> print __(\"Summary email is empty, skipping it\\n\");\n> $compose = -1;\n> @@ -792,6 +815,37 @@ sub ask {\n> return;\n> }\n>\n> +sub parse_header_line {\n> + my $lines = shift;\n> + my $parsed_line = shift;\n> +\n> + foreach (split(/\\n/, $lines)) {\n> + if (/^From:\\s*(.+)$/i) {\n> + $parsed_line->{'from'} = $1;\n> + } elsif (/^To:\\s*(.+)$/i) {\n> + $parsed_line->{'to'} = [ parse_address_line($1) ];\n> + } elsif (/^Cc:\\s*(.+)$/i) {\n> + $parsed_line->{'cc'} = [ parse_address_line($1) ];\n> + } elsif (/^Bcc:\\s*(.+)$/i) {\n> + $parsed_line->{'bcc'} = [ parse_address_line($1) ];\n> + } elsif (/^Subject:\\s*(.+)\\s*$/i) {\n> + $parsed_line->{'subject'} = $1;\n> + } elsif (/^Date: (.*)/i) {\n> + $parsed_line->{'date'} = $1;\n> + } elsif (/^In-Reply-To:\\s*(.+)\\s*$/i) {\n> + $parsed_line->{'in_reply_to'} = $1;\n> + } elsif (/^Message-ID: (.*)$/i) {\n> + $parsed_line->{'message_id'} = $1;\n> + } elsif (/^MIME-Version:$/i) {\n> + $parsed_line->{'mime-version'} = $1;\n> + } elsif (/^Content-Type:\\s+(.*)\\s*$/i) {\n> + $parsed_line->{'content-type'} = $1;\n> + } elsif (/^References:\\s+(.*)/i) {\n> + $parsed_line->{'references'} = $1;\n> + }\n> + }\n> +}\n> +\n> my %broken_encoding;\n>\n> sub file_declares_8bit_cte {\n> --\n> 2.15.1\n>\n"},{"id":"334005","messageId":"q7h9zi6zpkzj.fsf@orange.lip.ens-lyon.fr","threadId":"47366","inReplyTo":"445d0838cf2a4107bad95d5cc2d38a05@BPMBX2013-01.univ-lyon1.fr","subject":"Re: [PATCH] send-email: extract email-parsing code into a subroutine","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@univ-lyon1.fr","sentAt":"2017-12-03T21:20:00Z","receivedAt":"2017-12-03T21:20:17Z","isPatch":true,"sender":{"key":"matthieu.moy@univ-lyon1.fr","avatar":"https://gravatar.com/avatar/8ab83b763226bd297b59ddd1463a8bfc852924577191233f73a953b86888fe0c?d=mp&s=160"},"body":"Nathan PAYRE writes:\n\n> I found a mistake in my signed-off-by, please replace\n> by \n\nI think you want exactly the opposite of this. You're contributing as a\nLyon 1 student, hence your identity is @etu.univ-lyon1.fr. Your Gmail\nadress is used only for technical reasons.\n\nOTOH, you are missing the first line From: ... @..univ-lyon1.fr in your\nmessage.\n\nSee how you did it:\n\nhttps://public-inbox.org/git/20171012091727.30759-1-second.payre@gmail.com/\n\n(The sign-off was wrong in this one, but the From was OK)\n\n-- \nMatthieu Moy\nhttps://matthieu-moy.fr/\n"},{"id":"334013","messageId":"874lp7v5du.fsf@evledraar.booking.com","threadId":"47366","inReplyTo":"20171202170220.10073-1-second.payre@gmail.com","subject":"Re: [PATCH] send-email: extract email-parsing code into a subroutine","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-12-03T22:00:29Z","receivedAt":"2017-12-03T22:00:39Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sat, Dec 02 2017, Payre Nathan jotted:\n\n> From: Nathan Payre \n>\n> The existing code mixes parsing of email header with regular\n> expression and actual code. Extract the parsing code into a new\n> subroutine 'parse_header_line()'. This improves the code readability\n> and make parse_header_line reusable in other place.\n>\n> Signed-off-by: Nathan Payre \n> Signed-off-by: Matthieu Moy \n> Signed-off-by: Timothee Albertin \n> Signed-off-by: Daniel Bensoussan \n> ---\n>\n> This patch is a first step to implement a new feature.\n> See new feature discussion here: https://public-inbox.org/git/20171030223444.5052-1-nathan.payre@etu.univ-lyon1.fr/\n>\n> git-send-email.perl | 106 +++++++++++++++++++++++++++++++++++++++-------------\n> 1 file changed, 80 insertions(+), 26 deletions(-)\n>\n> diff --git a/git-send-email.perl b/git-send-email.perl\n> index 2208dcc21..98c2e461c 100755\n> --- a/git-send-email.perl\n> +++ b/git-send-email.perl\n> @@ -715,41 +715,64 @@ EOT3\n> \tif (!defined $compose_encoding) {\n> \t\t$compose_encoding = \"UTF-8\";\n> \t}\n> -\twhile(<$c>) {\n> +\n> +\tmy %parsed_email;\n> +\twhile (<$c>) {\n> \t\tnext if m/^GIT:/;\n> -\t\tif ($in_body) {\n> -\t\t\t$summary_empty = 0 unless (/^\\n$/);\n> -\t\t} elsif (/^\\n$/) {\n> -\t\t\t$in_body = 1;\n> -\t\t\tif ($need_8bit_cte) {\n> +\t\tparse_header_line($_, \\%parsed_email);\n> +\t\tif (/^\\n$/i) {\n> +\t\t\twhile (my $row = <$c>) {\n> +\t\t\t\tif (!($row =~ m/^GIT:/)) {\n> +\t\t\t\t\t$parsed_email{'body'} = $parsed_email{'body'} . $row;\n> +\t\t\t\t}\n> +\t\t\t}\n> +\t\t}\n> +\t}\n> +\tif ($parsed_email{'from'}) {\n> +\t\t$sender = $parsed_email{'from'};\n> +\t}\n> +\tif ($parsed_email{'in_reply_to'}) {\n> +\t\t$initial_reply_to = $parsed_email{'in_reply_to'};\n> +\t}\n> +\tif ($parsed_email{'subject'}) {\n> +\t\t$initial_subject = $parsed_email{'subject'};\n> +\t\tprint $c2 \"Subject: \" .\n> +\t\t\tquote_subject($parsed_email{'subject'}, $compose_encoding) .\n> +\t\t\t\"\\n\";\n> +\t}\n> +\tif ($parsed_email{'mime-version'}) {\n> +\t\t$need_8bit_cte = 0;\n> +\t}\n> +\tif ($need_8bit_cte) {\n> +\t\tif ($parsed_email{'content-type'}) {\n> +\t\t\t\tprint $c2 \"MIME-Version: 1.0\\n\",\n> +\t\t\t\t\t \"Content-Type: $parsed_email{'content-type'};\",\n> +\t\t\t\t\t \"Content-Transfer-Encoding: 8bit\\n\";\n> +\t\t\t} else {\n> \t\t\t\tprint $c2 \"MIME-Version: 1.0\\n\",\n> \t\t\t\t\t \"Content-Type: text/plain; \",\n> -\t\t\t\t\t \"charset=$compose_encoding\\n\",\n> +\t\t\t\t\t \"charset=$compose_encoding\\n\",\n> \t\t\t\t\t \"Content-Transfer-Encoding: 8bit\\n\";\n> \t\t\t}\n> -\t\t} elsif (/^MIME-Version:/i) {\n> -\t\t\t$need_8bit_cte = 0;\n> -\t\t} elsif (/^Subject:\\s*(.+)\\s*$/i) {\n> -\t\t\t$initial_subject = $1;\n> -\t\t\tmy $subject = $initial_subject;\n> -\t\t\t$_ = \"Subject: \" .\n> -\t\t\t\tquote_subject($subject, $compose_encoding) .\n> -\t\t\t\t\"\\n\";\n> -\t\t} elsif (/^In-Reply-To:\\s*(.+)\\s*$/i) {\n> -\t\t\t$initial_reply_to = $1;\n> -\t\t\tnext;\n> -\t\t} elsif (/^From:\\s*(.+)\\s*$/i) {\n> -\t\t\t$sender = $1;\n> -\t\t\tnext;\n> -\t\t} elsif (/^(?:To|Cc|Bcc):/i) {\n> -\t\t\tprint __(\"To/Cc/Bcc fields are not interpreted yet, they have been ignored\\n\");\n> -\t\t\tnext;\n> -\t\t}\n> -\t\tprint $c2 $_;\n> \t}\n> +\tif ($parsed_email{'body'}) {\n> +\t\t$summary_empty = 0;\n> +\t\tprint $c2 \"\\n$parsed_email{'body'}\\n\";\n> +\t}\n> +\n> \tclose $c;\n> \tclose $c2;\n>\n> +\topen $c2, \"<\", $compose_filename . \".final\"\n> +\t\tor die sprintf(__(\"Failed to open %s.final: %s\"), $compose_filename, $!);\n> +\n> +\tprint \"affichage : \\n\";\n> +\twhile (<$c2>) {\n> +\t\tprint $_;\n> +\t}\n> +\n> +\tclose $c2;\n> +\n> \tif ($summary_empty) {\n> \t\tprint __(\"Summary email is empty, skipping it\\n\");\n> \t\t$compose = -1;\n> @@ -792,6 +815,37 @@ sub ask {\n> \treturn;\n> }\n>\n> +sub parse_header_line {\n> +\tmy $lines = shift;\n> +\tmy $parsed_line = shift;\n> +\n> +\tforeach (split(/\\n/, $lines)) {\n> +\t\tif (/^From:\\s*(.+)$/i) {\n> +\t\t\t$parsed_line->{'from'} = $1;\n> +\t\t} elsif (/^To:\\s*(.+)$/i) {\n> +\t\t\t$parsed_line->{'to'} = [ parse_address_line($1) ];\n> +\t\t} elsif (/^Cc:\\s*(.+)$/i) {\n> +\t\t\t$parsed_line->{'cc'} = [ parse_address_line($1) ];\n> +\t\t} elsif (/^Bcc:\\s*(.+)$/i) {\n> +\t\t\t$parsed_line->{'bcc'} = [ parse_address_line($1) ];\n> +\t\t} elsif (/^Subject:\\s*(.+)\\s*$/i) {\n> +\t\t\t$parsed_line->{'subject'} = $1;\n> +\t\t} elsif (/^Date: (.*)/i) {\n> +\t\t\t$parsed_line->{'date'} = $1;\n> +\t\t} elsif (/^In-Reply-To:\\s*(.+)\\s*$/i) {\n> +\t\t\t$parsed_line->{'in_reply_to'} = $1;\n> +\t\t} elsif (/^Message-ID: (.*)$/i) {\n> +\t\t\t$parsed_line->{'message_id'} = $1;\n> +\t\t} elsif (/^MIME-Version:$/i) {\n> +\t\t\t$parsed_line->{'mime-version'} = $1;\n> +\t\t} elsif (/^Content-Type:\\s+(.*)\\s*$/i) {\n> +\t\t\t$parsed_line->{'content-type'} = $1;\n> +\t\t} elsif (/^References:\\s+(.*)/i) {\n> +\t\t\t$parsed_line->{'references'} = $1;\n> +\t\t}\n> +\t}\n> +}\n> +\n> my %broken_encoding;\n>\n> sub file_declares_8bit_cte {\n\nI haven't read the patches that follow. Completely untested, But just a\ndiff on top I came up with while reading this:\n\nRationale:\n\n * Once you start passing $_ to functions you should probably just give\n it a name.\n\n * !($x =~ m//) you can just write as $x !~ m//\n\n * There's a lot of copy/paste programming in parse_header_line() and an\n inconsistency between you seeing A-Header and turning it into either\n a_header or a-header. If you just stick with a-header and use dash\n you end up with just two cases.\n\n The resulting line is quite long, so it's worth doing:\n\n my $header_parsed = join \"|\", qw(To CC ...);\n my $header_unparsed = join \"|\", qw(From Subject Message-ID ...);\n [...]\n if ($str =~ /^($header_unparsed)\n \n\ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex 98c2e461cf..3696cad456 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -717,12 +717,12 @@ EOT3\n \t}\n\n \tmy %parsed_email;\n-\twhile (<$c>) {\n-\t\tnext if m/^GIT:/;\n-\t\tparse_header_line($_, \\%parsed_email);\n-\t\tif (/^\\n$/i) {\n+\twhile (my $line = <$c>) {\n+\t\tnext if $line =~ m/^GIT:/;\n+\t\tparse_header_line($line, \\%parsed_email);\n+\t\tif ($line =~ /^\\n$/i) {\n \t\t\twhile (my $row = <$c>) {\n-\t\t\t\tif (!($row =~ m/^GIT:/)) {\n+\t\t\t\tif ($row !~ m/^GIT:/) {\n \t\t\t\t\t$parsed_email{'body'} = $parsed_email{'body'} . $row;\n \t\t\t\t}\n \t\t\t}\n@@ -731,7 +731,7 @@ EOT3\n \tif ($parsed_email{'from'}) {\n \t\t$sender = $parsed_email{'from'};\n \t}\n-\tif ($parsed_email{'in_reply_to'}) {\n+\tif ($parsed_email{'in-reply-to'}) {\n \t\t$initial_reply_to = $parsed_email{'in_reply_to'};\n \t}\n \tif ($parsed_email{'subject'}) {\n@@ -820,28 +820,10 @@ sub parse_header_line {\n \tmy $parsed_line = shift;\n\n \tforeach (split(/\\n/, $lines)) {\n-\t\tif (/^From:\\s*(.+)$/i) {\n-\t\t\t$parsed_line->{'from'} = $1;\n-\t\t} elsif (/^To:\\s*(.+)$/i) {\n-\t\t\t$parsed_line->{'to'} = [ parse_address_line($1) ];\n-\t\t} elsif (/^Cc:\\s*(.+)$/i) {\n-\t\t\t$parsed_line->{'cc'} = [ parse_address_line($1) ];\n-\t\t} elsif (/^Bcc:\\s*(.+)$/i) {\n-\t\t\t$parsed_line->{'bcc'} = [ parse_address_line($1) ];\n-\t\t} elsif (/^Subject:\\s*(.+)\\s*$/i) {\n-\t\t\t$parsed_line->{'subject'} = $1;\n-\t\t} elsif (/^Date: (.*)/i) {\n-\t\t\t$parsed_line->{'date'} = $1;\n-\t\t} elsif (/^In-Reply-To:\\s*(.+)\\s*$/i) {\n-\t\t\t$parsed_line->{'in_reply_to'} = $1;\n-\t\t} elsif (/^Message-ID: (.*)$/i) {\n-\t\t\t$parsed_line->{'message_id'} = $1;\n-\t\t} elsif (/^MIME-Version:$/i) {\n-\t\t\t$parsed_line->{'mime-version'} = $1;\n-\t\t} elsif (/^Content-Type:\\s+(.*)\\s*$/i) {\n-\t\t\t$parsed_line->{'content-type'} = $1;\n-\t\t} elsif (/^References:\\s+(.*)/i) {\n-\t\t\t$parsed_line->{'references'} = $1;\n+\t\tif (/^(To|Cc|Bcc):\\s*(.+)$/i) {\n+\t\t\t$parsed_line->{lc $1} = [ parse_address_line($2) ];\n+\t\t} elsif (/^(From|Subject|Date|In-Reply-To|Message-ID|MIME-Version|Content-Type|References):\\s*(.+)\\s*$/i) {\n+\t\t\t$parsed_line->{lc $1} = $2;\n \t\t}\n \t}\n }\n"},{"id":"334028","messageId":"CAGb4CBUY0QVOHqAFDi6kSVmK48PzKKTuXR2F_Nr+6cHHUhxBzQ@mail.gmail.com","threadId":"47366","inReplyTo":"874lp7v5du.fsf@evledraar.booking.com","subject":"Re: [PATCH] send-email: extract email-parsing code into a subroutine","fromName":"Nathan PAYRE","fromEmail":"second.payre@gmail.com","sentAt":"2017-12-03T23:41:04Z","receivedAt":"2017-12-03T23:41:11Z","isPatch":true,"sender":{"key":"second.payre@gmail.com","avatar":null},"body":"I've tested your code, and after few changes it's works perfectly!\nThe code looks better now.\nThanks a lot for your review.\n\n2017-12-03 23:00 GMT+01:00 Ævar Arnfjörð Bjarmason :\n>\n> On Sat, Dec 02 2017, Payre Nathan jotted:\n>\n>> From: Nathan Payre \n>>\n>> The existing code mixes parsing of email header with regular\n>> expression and actual code. Extract the parsing code into a new\n>> subroutine 'parse_header_line()'. This improves the code readability\n>> and make parse_header_line reusable in other place.\n>>\n>> Signed-off-by: Nathan Payre \n>> Signed-off-by: Matthieu Moy \n>> Signed-off-by: Timothee Albertin \n>> Signed-off-by: Daniel Bensoussan \n>> ---\n>>\n>> This patch is a first step to implement a new feature.\n>> See new feature discussion here: https://public-inbox.org/git/20171030223444.5052-1-nathan.payre@etu.univ-lyon1.fr/\n>>\n>> git-send-email.perl | 106 +++++++++++++++++++++++++++++++++++++++-------------\n>> 1 file changed, 80 insertions(+), 26 deletions(-)\n>>\n>> diff --git a/git-send-email.perl b/git-send-email.perl\n>> index 2208dcc21..98c2e461c 100755\n>> --- a/git-send-email.perl\n>> +++ b/git-send-email.perl\n>> @@ -715,41 +715,64 @@ EOT3\n>> if (!defined $compose_encoding) {\n>> $compose_encoding = \"UTF-8\";\n>> }\n>> - while(<$c>) {\n>> +\n>> + my %parsed_email;\n>> + while (<$c>) {\n>> next if m/^GIT:/;\n>> - if ($in_body) {\n>> - $summary_empty = 0 unless (/^\\n$/);\n>> - } elsif (/^\\n$/) {\n>> - $in_body = 1;\n>> - if ($need_8bit_cte) {\n>> + parse_header_line($_, \\%parsed_email);\n>> + if (/^\\n$/i) {\n>> + while (my $row = <$c>) {\n>> + if (!($row =~ m/^GIT:/)) {\n>> + $parsed_email{'body'} = $parsed_email{'body'} . $row;\n>> + }\n>> + }\n>> + }\n>> + }\n>> + if ($parsed_email{'from'}) {\n>> + $sender = $parsed_email{'from'};\n>> + }\n>> + if ($parsed_email{'in_reply_to'}) {\n>> + $initial_reply_to = $parsed_email{'in_reply_to'};\n>> + }\n>> + if ($parsed_email{'subject'}) {\n>> + $initial_subject = $parsed_email{'subject'};\n>> + print $c2 \"Subject: \" .\n>> + quote_subject($parsed_email{'subject'}, $compose_encoding) .\n>> + \"\\n\";\n>> + }\n>> + if ($parsed_email{'mime-version'}) {\n>> + $need_8bit_cte = 0;\n>> + }\n>> + if ($need_8bit_cte) {\n>> + if ($parsed_email{'content-type'}) {\n>> + print $c2 \"MIME-Version: 1.0\\n\",\n>> + \"Content-Type: $parsed_email{'content-type'};\",\n>> + \"Content-Transfer-Encoding: 8bit\\n\";\n>> + } else {\n>> print $c2 \"MIME-Version: 1.0\\n\",\n>> \"Content-Type: text/plain; \",\n>> - \"charset=$compose_encoding\\n\",\n>> + \"charset=$compose_encoding\\n\",\n>> \"Content-Transfer-Encoding: 8bit\\n\";\n>> }\n>> - } elsif (/^MIME-Version:/i) {\n>> - $need_8bit_cte = 0;\n>> - } elsif (/^Subject:\\s*(.+)\\s*$/i) {\n>> - $initial_subject = $1;\n>> - my $subject = $initial_subject;\n>> - $_ = \"Subject: \" .\n>> - quote_subject($subject, $compose_encoding) .\n>> - \"\\n\";\n>> - } elsif (/^In-Reply-To:\\s*(.+)\\s*$/i) {\n>> - $initial_reply_to = $1;\n>> - next;\n>> - } elsif (/^From:\\s*(.+)\\s*$/i) {\n>> - $sender = $1;\n>> - next;\n>> - } elsif (/^(?:To|Cc|Bcc):/i) {\n>> - print __(\"To/Cc/Bcc fields are not interpreted yet, they have been ignored\\n\");\n>> - next;\n>> - }\n>> - print $c2 $_;\n>> }\n>> + if ($parsed_email{'body'}) {\n>> + $summary_empty = 0;\n>> + print $c2 \"\\n$parsed_email{'body'}\\n\";\n>> + }\n>> +\n>> close $c;\n>> close $c2;\n>>\n>> + open $c2, \"<\", $compose_filename . \".final\"\n>> + or die sprintf(__(\"Failed to open %s.final: %s\"), $compose_filename, $!);\n>> +\n>> + print \"affichage : \\n\";\n>> + while (<$c2>) {\n>> + print $_;\n>> + }\n>> +\n>> + close $c2;\n>> +\n>> if ($summary_empty) {\n>> print __(\"Summary email is empty, skipping it\\n\");\n>> $compose = -1;\n>> @@ -792,6 +815,37 @@ sub ask {\n>> return;\n>> }\n>>\n>> +sub parse_header_line {\n>> + my $lines = shift;\n>> + my $parsed_line = shift;\n>> +\n>> + foreach (split(/\\n/, $lines)) {\n>> + if (/^From:\\s*(.+)$/i) {\n>> + $parsed_line->{'from'} = $1;\n>> + } elsif (/^To:\\s*(.+)$/i) {\n>> + $parsed_line->{'to'} = [ parse_address_line($1) ];\n>> + } elsif (/^Cc:\\s*(.+)$/i) {\n>> + $parsed_line->{'cc'} = [ parse_address_line($1) ];\n>> + } elsif (/^Bcc:\\s*(.+)$/i) {\n>> + $parsed_line->{'bcc'} = [ parse_address_line($1) ];\n>> + } elsif (/^Subject:\\s*(.+)\\s*$/i) {\n>> + $parsed_line->{'subject'} = $1;\n>> + } elsif (/^Date: (.*)/i) {\n>> + $parsed_line->{'date'} = $1;\n>> + } elsif (/^In-Reply-To:\\s*(.+)\\s*$/i) {\n>> + $parsed_line->{'in_reply_to'} = $1;\n>> + } elsif (/^Message-ID: (.*)$/i) {\n>> + $parsed_line->{'message_id'} = $1;\n>> + } elsif (/^MIME-Version:$/i) {\n>> + $parsed_line->{'mime-version'} = $1;\n>> + } elsif (/^Content-Type:\\s+(.*)\\s*$/i) {\n>> + $parsed_line->{'content-type'} = $1;\n>> + } elsif (/^References:\\s+(.*)/i) {\n>> + $parsed_line->{'references'} = $1;\n>> + }\n>> + }\n>> +}\n>> +\n>> my %broken_encoding;\n>>\n>> sub file_declares_8bit_cte {\n>\n> I haven't read the patches that follow. Completely untested, But just a\n> diff on top I came up with while reading this:\n>\n> Rationale:\n>\n> * Once you start passing $_ to functions you should probably just give\n> it a name.\n>\n> * !($x =~ m//) you can just write as $x !~ m//\n>\n> * There's a lot of copy/paste programming in parse_header_line() and an\n> inconsistency between you seeing A-Header and turning it into either\n> a_header or a-header. If you just stick with a-header and use dash\n> you end up with just two cases.\n>\n> The resulting line is quite long, so it's worth doing:\n>\n> my $header_parsed = join \"|\", qw(To CC ...);\n> my $header_unparsed = join \"|\", qw(From Subject Message-ID ...);\n> [...]\n> if ($str =~ /^($header_unparsed)\n>\n>\n> diff --git a/git-send-email.perl b/git-send-email.perl\n> index 98c2e461cf..3696cad456 100755\n> --- a/git-send-email.perl\n> +++ b/git-send-email.perl\n> @@ -717,12 +717,12 @@ EOT3\n> }\n>\n> my %parsed_email;\n> - while (<$c>) {\n> - next if m/^GIT:/;\n> - parse_header_line($_, \\%parsed_email);\n> - if (/^\\n$/i) {\n> + while (my $line = <$c>) {\n> + next if $line =~ m/^GIT:/;\n> + parse_header_line($line, \\%parsed_email);\n> + if ($line =~ /^\\n$/i) {\n> while (my $row = <$c>) {\n> - if (!($row =~ m/^GIT:/)) {\n> + if ($row !~ m/^GIT:/) {\n> $parsed_email{'body'} = $parsed_email{'body'} . $row;\n> }\n> }\n> @@ -731,7 +731,7 @@ EOT3\n> if ($parsed_email{'from'}) {\n> $sender = $parsed_email{'from'};\n> }\n> - if ($parsed_email{'in_reply_to'}) {\n> + if ($parsed_email{'in-reply-to'}) {\n> $initial_reply_to = $parsed_email{'in_reply_to'};\n> }\n> if ($parsed_email{'subject'}) {\n> @@ -820,28 +820,10 @@ sub parse_header_line {\n> my $parsed_line = shift;\n>\n> foreach (split(/\\n/, $lines)) {\n> - if (/^From:\\s*(.+)$/i) {\n> - $parsed_line->{'from'} = $1;\n> - } elsif (/^To:\\s*(.+)$/i) {\n> - $parsed_line->{'to'} = [ parse_address_line($1) ];\n> - } elsif (/^Cc:\\s*(.+)$/i) {\n> - $parsed_line->{'cc'} = [ parse_address_line($1) ];\n> - } elsif (/^Bcc:\\s*(.+)$/i) {\n> - $parsed_line->{'bcc'} = [ parse_address_line($1) ];\n> - } elsif (/^Subject:\\s*(.+)\\s*$/i) {\n> - $parsed_line->{'subject'} = $1;\n> - } elsif (/^Date: (.*)/i) {\n> - $parsed_line->{'date'} = $1;\n> - } elsif (/^In-Reply-To:\\s*(.+)\\s*$/i) {\n> - $parsed_line->{'in_reply_to'} = $1;\n> - } elsif (/^Message-ID: (.*)$/i) {\n> - $parsed_line->{'message_id'} = $1;\n> - } elsif (/^MIME-Version:$/i) {\n> - $parsed_line->{'mime-version'} = $1;\n> - } elsif (/^Content-Type:\\s+(.*)\\s*$/i) {\n> - $parsed_line->{'content-type'} = $1;\n> - } elsif (/^References:\\s+(.*)/i) {\n> - $parsed_line->{'references'} = $1;\n> + if (/^(To|Cc|Bcc):\\s*(.+)$/i) {\n> + $parsed_line->{lc $1} = [ parse_address_line($2) ];\n> + } elsif (/^(From|Subject|Date|In-Reply-To|Message-ID|MIME-Version|Content-Type|References):\\s*(.+)\\s*$/i) {\n> + $parsed_line->{lc $1} = $2;\n> }\n> }\n> }\n"},{"id":"334046","messageId":"xmqqlgiiobcy.fsf@gitster.mtv.corp.google.com","threadId":"47366","inReplyTo":"CAGb4CBUY0QVOHqAFDi6kSVmK48PzKKTuXR2F_Nr+6cHHUhxBzQ@mail.gmail.com","subject":"Re: [PATCH] send-email: extract email-parsing code into a subroutine","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-12-04T13:45:33Z","receivedAt":"2017-12-04T13:45:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nathan PAYRE writes:\n\n> I've tested your code, and after few changes it's works perfectly!\n> The code looks better now.\n> Thanks a lot for your review.\n\nThanks, both of you. \n\nCould you send in the final version later so that I can pick it up?\nI agree with Matthieu's suggestion on what address to use on your\nFrom: (authorship identity) and S-o-b:; as you are showing your work\ndone as a uni student, the authorship and sign-off should be done as\nsuch.\n"},{"id":"334248","messageId":"20171206153821.24435-1-nathan.payre@etu.univ-lyon1.fr","threadId":"47366","inReplyTo":"xmqqlgiiobcy.fsf@gitster.mtv.corp.google.com","subject":"[PATCH v2] send-email: extract email-parsing code into a subroutine","fromName":"Nathan Payre","fromEmail":"nathan.payre@etu.univ-lyon1.fr","sentAt":"2017-12-06T15:38:21Z","receivedAt":"2017-12-06T15:38:49Z","isPatch":true,"sender":{"key":"nathan.payre@etu.univ-lyon1.fr","avatar":null},"body":"The existing code mixes parsing of email header with regular\nexpression and actual code. Extract the parsing code into a new\nsubroutine 'parse_header_line()'. This improves the code readability\nand make parse_header_line reusable in other place.\n\nSigned-off-by: Nathan Payre \nSigned-off-by: Matthieu Moy \nSigned-off-by: Timothee Albertin \nSigned-off-by: Daniel Bensoussan \nThanks-to: Ævar Arnfjörð Bjarmason \n---\n git-send-email.perl | 100 ++++++++++++++++++++++++++++++++++++++--------------\n 1 file changed, 73 insertions(+), 27 deletions(-)\n\ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex 2208dcc21..db16e4dec 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -715,41 +715,73 @@ EOT3\n \tif (!defined $compose_encoding) {\n \t\t$compose_encoding = \"UTF-8\";\n \t}\n-\twhile(<$c>) {\n-\t\tnext if m/^GIT:/;\n-\t\tif ($in_body) {\n-\t\t\t$summary_empty = 0 unless (/^\\n$/);\n-\t\t} elsif (/^\\n$/) {\n-\t\t\t$in_body = 1;\n-\t\t\tif ($need_8bit_cte) {\n+\n+ my %parsed_email;\n+\t$parsed_email{'body'} = '';\n+ while (my $line = <$c>) {\n+\t next if $line =~ m/^GIT:/;\n+\t parse_header_line($line, \\%parsed_email);\n+\t if ($line =~ /^\\n$/i) {\n+\t while (my $body_line = <$c>) {\n+ if ($body_line !~ m/^GIT:/) {\n+ $parsed_email{'body'} = $parsed_email{'body'} . $body_line;\n+ }\n+\t }\n+\t\t}\n+\t\tprint \"la : $line\\n\";\n+\t}\n+\n+\tif ($parsed_email{'from'}) {\n+\t\t$sender = $parsed_email{'from'};\n+\t}\n+\tif ($parsed_email{'in-reply-to'}) {\n+\t\t$initial_reply_to = $parsed_email{'in-reply-to'};\n+\t}\n+\tif ($parsed_email{'subject'}) {\n+\t\t$initial_subject = $parsed_email{'subject'};\n+\t\tprint $c2 \"Subject: \" .\n+\t\t\tquote_subject($parsed_email{'subject'}, $compose_encoding) .\n+\t\t\t\"\\n\";\n+\t}\n+\tif ($parsed_email{'mime-version'}) {\n+\t\tprint \"CASE 0\\n\";\n+\t\t$need_8bit_cte = 0;\n+\t\tprint $c2 \"MIME-Version: $parsed_email{'mime-version'}\\n\",\n+\t\t\t\t\t\"Content-Type: $parsed_email{'content-type'};\\n\",\n+\t\t\t\t\t\"Content-Transfer-Encoding: $parsed_email{'content-transfer-encoding'}\\n\";\n+\t}\n+\tif ($need_8bit_cte) {\n+\t\tif ($parsed_email{'content-type'}) {\n+\t\t\t\tprint \"CASE 1\\n\";\n+\t\t\t\tprint $c2 \"MIME-Version: 1.0\\n\",\n+\t\t\t\t\t \"Content-Type: $parsed_email{'content-type'};\",\n+\t\t\t\t\t \"Content-Transfer-Encoding: 8bit\\n\";\n+\t\t\t} else {\n+\t\t\t\tprint \"CASE 2\\n\";\n \t\t\t\tprint $c2 \"MIME-Version: 1.0\\n\",\n \t\t\t\t\t \"Content-Type: text/plain; \",\n-\t\t\t\t\t \"charset=$compose_encoding\\n\",\n+\t\t\t\t\t \"charset=$compose_encoding\\n\",\n \t\t\t\t\t \"Content-Transfer-Encoding: 8bit\\n\";\n \t\t\t}\n-\t\t} elsif (/^MIME-Version:/i) {\n-\t\t\t$need_8bit_cte = 0;\n-\t\t} elsif (/^Subject:\\s*(.+)\\s*$/i) {\n-\t\t\t$initial_subject = $1;\n-\t\t\tmy $subject = $initial_subject;\n-\t\t\t$_ = \"Subject: \" .\n-\t\t\t\tquote_subject($subject, $compose_encoding) .\n-\t\t\t\t\"\\n\";\n-\t\t} elsif (/^In-Reply-To:\\s*(.+)\\s*$/i) {\n-\t\t\t$initial_reply_to = $1;\n-\t\t\tnext;\n-\t\t} elsif (/^From:\\s*(.+)\\s*$/i) {\n-\t\t\t$sender = $1;\n-\t\t\tnext;\n-\t\t} elsif (/^(?:To|Cc|Bcc):/i) {\n-\t\t\tprint __(\"To/Cc/Bcc fields are not interpreted yet, they have been ignored\\n\");\n-\t\t\tnext;\n-\t\t}\n-\t\tprint $c2 $_;\n \t}\n+\tif ($parsed_email{'body'}) {\n+\t\t$summary_empty = 0;\n+\t\tprint $c2 \"\\n$parsed_email{'body'}\\n\";\n+\t}\n+\n \tclose $c;\n \tclose $c2;\n \n+\topen $c2, \"<\", $compose_filename . \".final\"\n+\t\tor die sprintf(__(\"Failed to open %s.final: %s\"), $compose_filename, $!);\n+\n+\tprint \"affichage : \\n\";\n+\twhile (<$c2>) {\n+\t\tprint $_;\n+\t}\n+\n+\tclose $c2;\n+\n \tif ($summary_empty) {\n \t\tprint __(\"Summary email is empty, skipping it\\n\");\n \t\t$compose = -1;\n@@ -792,6 +824,20 @@ sub ask {\n \treturn;\n }\n \n+sub parse_header_line {\n+\tmy $lines = shift;\n+\tmy $parsed_line = shift;\n+\n+\tforeach (split(/\\n/, $lines)) {\n+\t\tif (/^(To|Cc|Bcc):\\s*(.+)$/i) {\n+\t\t $parsed_line->{lc $1} = [ parse_address_line($2) ];\n+\t\t} elsif (/^(From|Subject|Date|In-Reply-To|Message-ID|MIME-Version|Content-Type|Content-Transfer-Encoding|References):\\s*(.+)\\s*$/i) {\n+\t\t $parsed_line->{lc $1} = $2;\n+\t\t}\n+\t}\n+}\n+\n+\n my %broken_encoding;\n \n sub file_declares_8bit_cte {\n-- \n2.15.1\n\n"},{"id":"334253","messageId":"20171206153223.24102-1-nathan.payre@etu.univ-lyon1.fr","threadId":"47366","inReplyTo":"CAGb4CBUY0QVOHqAFDi6kSVmK48PzKKTuXR2F_Nr+6cHHUhxBzQ@mail.gmail.com","subject":"[PATCH v2] send-email: extract email-parsing code into a subroutine","fromName":"Nathan Payre","fromEmail":"nathan.payre@etu.univ-lyon1.fr","sentAt":"2017-12-06T15:32:23Z","receivedAt":"2017-12-06T16:05:10Z","isPatch":true,"sender":{"key":"nathan.payre@etu.univ-lyon1.fr","avatar":null},"body":"The existing code mixes parsing of email header with regular\nexpression and actual code. Extract the parsing code into a new\nsubroutine 'parse_header_line()'. This improves the code readability\nand make parse_header_line reusable in other place.\n\nSigned-off-by: Nathan Payre \nSigned-off-by: Matthieu Moy \nSigned-off-by: Timothee Albertin \nSigned-off-by: Daniel Bensoussan \nThanks-to: Ævar Arnfjörð Bjarmason \n---\n git-send-email.perl | 100 ++++++++++++++++++++++++++++++++++++++--------------\n 1 file changed, 73 insertions(+), 27 deletions(-)\n\ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex 2208dcc21..db16e4dec 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -715,41 +715,73 @@ EOT3\n \tif (!defined $compose_encoding) {\n \t\t$compose_encoding = \"UTF-8\";\n \t}\n-\twhile(<$c>) {\n-\t\tnext if m/^GIT:/;\n-\t\tif ($in_body) {\n-\t\t\t$summary_empty = 0 unless (/^\\n$/);\n-\t\t} elsif (/^\\n$/) {\n-\t\t\t$in_body = 1;\n-\t\t\tif ($need_8bit_cte) {\n+\n+ my %parsed_email;\n+\t$parsed_email{'body'} = '';\n+ while (my $line = <$c>) {\n+\t next if $line =~ m/^GIT:/;\n+\t parse_header_line($line, \\%parsed_email);\n+\t if ($line =~ /^\\n$/i) {\n+\t while (my $body_line = <$c>) {\n+ if ($body_line !~ m/^GIT:/) {\n+ $parsed_email{'body'} = $parsed_email{'body'} . $body_line;\n+ }\n+\t }\n+\t\t}\n+\t\tprint \"la : $line\\n\";\n+\t}\n+\n+\tif ($parsed_email{'from'}) {\n+\t\t$sender = $parsed_email{'from'};\n+\t}\n+\tif ($parsed_email{'in-reply-to'}) {\n+\t\t$initial_reply_to = $parsed_email{'in-reply-to'};\n+\t}\n+\tif ($parsed_email{'subject'}) {\n+\t\t$initial_subject = $parsed_email{'subject'};\n+\t\tprint $c2 \"Subject: \" .\n+\t\t\tquote_subject($parsed_email{'subject'}, $compose_encoding) .\n+\t\t\t\"\\n\";\n+\t}\n+\tif ($parsed_email{'mime-version'}) {\n+\t\tprint \"CASE 0\\n\";\n+\t\t$need_8bit_cte = 0;\n+\t\tprint $c2 \"MIME-Version: $parsed_email{'mime-version'}\\n\",\n+\t\t\t\t\t\"Content-Type: $parsed_email{'content-type'};\\n\",\n+\t\t\t\t\t\"Content-Transfer-Encoding: $parsed_email{'content-transfer-encoding'}\\n\";\n+\t}\n+\tif ($need_8bit_cte) {\n+\t\tif ($parsed_email{'content-type'}) {\n+\t\t\t\tprint \"CASE 1\\n\";\n+\t\t\t\tprint $c2 \"MIME-Version: 1.0\\n\",\n+\t\t\t\t\t \"Content-Type: $parsed_email{'content-type'};\",\n+\t\t\t\t\t \"Content-Transfer-Encoding: 8bit\\n\";\n+\t\t\t} else {\n+\t\t\t\tprint \"CASE 2\\n\";\n \t\t\t\tprint $c2 \"MIME-Version: 1.0\\n\",\n \t\t\t\t\t \"Content-Type: text/plain; \",\n-\t\t\t\t\t \"charset=$compose_encoding\\n\",\n+\t\t\t\t\t \"charset=$compose_encoding\\n\",\n \t\t\t\t\t \"Content-Transfer-Encoding: 8bit\\n\";\n \t\t\t}\n-\t\t} elsif (/^MIME-Version:/i) {\n-\t\t\t$need_8bit_cte = 0;\n-\t\t} elsif (/^Subject:\\s*(.+)\\s*$/i) {\n-\t\t\t$initial_subject = $1;\n-\t\t\tmy $subject = $initial_subject;\n-\t\t\t$_ = \"Subject: \" .\n-\t\t\t\tquote_subject($subject, $compose_encoding) .\n-\t\t\t\t\"\\n\";\n-\t\t} elsif (/^In-Reply-To:\\s*(.+)\\s*$/i) {\n-\t\t\t$initial_reply_to = $1;\n-\t\t\tnext;\n-\t\t} elsif (/^From:\\s*(.+)\\s*$/i) {\n-\t\t\t$sender = $1;\n-\t\t\tnext;\n-\t\t} elsif (/^(?:To|Cc|Bcc):/i) {\n-\t\t\tprint __(\"To/Cc/Bcc fields are not interpreted yet, they have been ignored\\n\");\n-\t\t\tnext;\n-\t\t}\n-\t\tprint $c2 $_;\n \t}\n+\tif ($parsed_email{'body'}) {\n+\t\t$summary_empty = 0;\n+\t\tprint $c2 \"\\n$parsed_email{'body'}\\n\";\n+\t}\n+\n \tclose $c;\n \tclose $c2;\n \n+\topen $c2, \"<\", $compose_filename . \".final\"\n+\t\tor die sprintf(__(\"Failed to open %s.final: %s\"), $compose_filename, $!);\n+\n+\tprint \"affichage : \\n\";\n+\twhile (<$c2>) {\n+\t\tprint $_;\n+\t}\n+\n+\tclose $c2;\n+\n \tif ($summary_empty) {\n \t\tprint __(\"Summary email is empty, skipping it\\n\");\n \t\t$compose = -1;\n@@ -792,6 +824,20 @@ sub ask {\n \treturn;\n }\n \n+sub parse_header_line {\n+\tmy $lines = shift;\n+\tmy $parsed_line = shift;\n+\n+\tforeach (split(/\\n/, $lines)) {\n+\t\tif (/^(To|Cc|Bcc):\\s*(.+)$/i) {\n+\t\t $parsed_line->{lc $1} = [ parse_address_line($2) ];\n+\t\t} elsif (/^(From|Subject|Date|In-Reply-To|Message-ID|MIME-Version|Content-Type|Content-Transfer-Encoding|References):\\s*(.+)\\s*$/i) {\n+\t\t $parsed_line->{lc $1} = $2;\n+\t\t}\n+\t}\n+}\n+\n+\n my %broken_encoding;\n \n sub file_declares_8bit_cte {\n-- \n2.15.1\n\n"},{"id":"334273","messageId":"xmqqvahjfsdx.fsf@gitster.mtv.corp.google.com","threadId":"47366","inReplyTo":"20171206153821.24435-1-nathan.payre@etu.univ-lyon1.fr","subject":"Re: [PATCH v2] send-email: extract email-parsing code into a subroutine","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-12-06T21:39:22Z","receivedAt":"2017-12-06T21:39:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\nNathan Payre writes:\n\n> The existing code mixes parsing of email header with regular\n> expression and actual code. Extract the parsing code into a new\n> subroutine 'parse_header_line()'. This improves the code readability\n> and make parse_header_line reusable in other place.\n>\n> Signed-off-by: Nathan Payre \n> Signed-off-by: Matthieu Moy \n> Signed-off-by: Timothee Albertin \n> Signed-off-by: Daniel Bensoussan \n> Thanks-to: Ævar Arnfjörð Bjarmason \n\nThanks, but... \n\n> + my %parsed_email;\n> +\t$parsed_email{'body'} = '';\n> + while (my $line = <$c>) {\n> +\t next if $line =~ m/^GIT:/;\n> +\t parse_header_line($line, \\%parsed_email);\n> +\t if ($line =~ /^\\n$/i) {\n> +\t while (my $body_line = <$c>) {\n> + if ($body_line !~ m/^GIT:/) {\n> + $parsed_email{'body'} = $parsed_email{'body'} . $body_line;\n> + }\n> +\t }\n> +\t\t}\n> +\t\tprint \"la : $line\\n\";\n> +\t}\n\n... throughout this patch, not limited to this section, indentation\nis strange and there seem to be many \"print\" that show messages that\ndo not seem to be meant for end-user consumption. I can see that\nthis aspires to improve the readability, but not quite yet ;-).\n\nAlso \"reusable in other place\" is by itself not an unconditional\nplus, until readers can be convinced that that 'other place' really\nwants to be able to call this function. Is there some untold\nmotivation behind this change---such as a planned update to actually\nuse this helper subroutine?\n"},{"id":"334284","messageId":"CAGb4CBWZciqxdfpSkK1vezhiuSYX5Yy-xSq=Uj4h+vhRo9uyoQ@mail.gmail.com","threadId":"47366","inReplyTo":"xmqqvahjfsdx.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2] send-email: extract email-parsing code into a subroutine","fromName":"Nathan PAYRE","fromEmail":"second.payre@gmail.com","sentAt":"2017-12-06T22:55:09Z","receivedAt":"2017-12-06T22:55:20Z","isPatch":true,"sender":{"key":"second.payre@gmail.com","avatar":null},"body":"Junio C Hamano : writes:\n\n> ... throughout this patch, not limited to this section, indentation\n> is strange and there seem to be many \"print\" that show messages that\n> do not seem to be meant for end-user consumption. I can see that\n> this aspires to improve the readability, but not quite yet ;-).\n\nHmmm I'm wondering who place thoses print in my code !\nI will fix it fast. :-)\n\n> Also \"reusable in other place\" is by itself not an unconditional\n> plus, until readers can be convinced that that 'other place' really\n> wants to be able to call this function. Is there some untold\n> motivation behind this change---such as a planned update to actually\n> use this helper subroutine?\n\nThis subroutine will be used to implement, initially a new option called\n\"--quote-email\", but became \"--cite\" added after \"--in-reply-to\".\nThis will permit to the user to cite a mail and reply with a patch and keep\nCc, To ...\nSee discussion :\nhttps://public-inbox.org/git/20171030223444.5052-1-nathan.payre@etu.univ-lyon1.fr/\n\nAnd Daniel Timothee and I wanted to refactor an other part of the file\nusing parse_header_line(). Near Line 1570.\n"},{"id":"334285","messageId":"20171206230225.18873-1-nathan.payre@etu.univ-lyon1.fr","threadId":"47366","inReplyTo":"CAGb4CBWZciqxdfpSkK1vezhiuSYX5Yy-xSq=Uj4h+vhRo9uyoQ@mail.gmail.com","subject":"[PATCH v3] send-email: extract email-parsing code into a subroutine","fromName":"Nathan Payre","fromEmail":"nathan.payre@etu.univ-lyon1.fr","sentAt":"2017-12-06T23:02:25Z","receivedAt":"2017-12-06T23:02:48Z","isPatch":true,"sender":{"key":"nathan.payre@etu.univ-lyon1.fr","avatar":null},"body":"The existing code mixes parsing of email header with regular\nexpression and actual code. Extract the parsing code into a new\nsubroutine 'parse_header_line()'. This improves the code readability\nand make parse_header_line reusable in other place.\n\nSigned-off-by: Nathan Payre \nSigned-off-by: Matthieu Moy \nSigned-off-by: Timothee Albertin \nSigned-off-by: Daniel Bensoussan \nThanks-to: Ævar Arnfjörð Bjarmason \n---\n\nWithout the \"print\" used for testing. \n\n git-send-email.perl | 90 +++++++++++++++++++++++++++++++++++++----------------\n 1 file changed, 63 insertions(+), 27 deletions(-)\n\ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex 2208dcc21..a10574a56 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -715,41 +715,63 @@ EOT3\n \tif (!defined $compose_encoding) {\n \t\t$compose_encoding = \"UTF-8\";\n \t}\n-\twhile(<$c>) {\n-\t\tnext if m/^GIT:/;\n-\t\tif ($in_body) {\n-\t\t\t$summary_empty = 0 unless (/^\\n$/);\n-\t\t} elsif (/^\\n$/) {\n-\t\t\t$in_body = 1;\n-\t\t\tif ($need_8bit_cte) {\n+\n+ my %parsed_email;\n+\t$parsed_email{'body'} = '';\n+ while (my $line = <$c>) {\n+\t next if $line =~ m/^GIT:/;\n+\t parse_header_line($line, \\%parsed_email);\n+\t if ($line =~ /^\\n$/i) {\n+\t while (my $body_line = <$c>) {\n+ if ($body_line !~ m/^GIT:/) {\n+ $parsed_email{'body'} = $parsed_email{'body'} . $body_line;\n+ }\n+\t }\n+\t\t}\n+\t}\n+\n+\tif ($parsed_email{'from'}) {\n+\t\t$sender = $parsed_email{'from'};\n+\t}\n+\tif ($parsed_email{'in-reply-to'}) {\n+\t\t$initial_reply_to = $parsed_email{'in-reply-to'};\n+\t}\n+\tif ($parsed_email{'subject'}) {\n+\t\t$initial_subject = $parsed_email{'subject'};\n+\t\tprint $c2 \"Subject: \" .\n+\t\t\tquote_subject($parsed_email{'subject'}, $compose_encoding) .\n+\t\t\t\"\\n\";\n+\t}\n+\tif ($parsed_email{'mime-version'}) {\n+\t\t$need_8bit_cte = 0;\n+\t\tprint $c2 \"MIME-Version: $parsed_email{'mime-version'}\\n\",\n+\t\t\t\t\t\"Content-Type: $parsed_email{'content-type'};\\n\",\n+\t\t\t\t\t\"Content-Transfer-Encoding: $parsed_email{'content-transfer-encoding'}\\n\";\n+\t}\n+\tif ($need_8bit_cte) {\n+\t\tif ($parsed_email{'content-type'}) {\n+\t\t\t\tprint $c2 \"MIME-Version: 1.0\\n\",\n+\t\t\t\t\t \"Content-Type: $parsed_email{'content-type'};\",\n+\t\t\t\t\t \"Content-Transfer-Encoding: 8bit\\n\";\n+\t\t\t} else {\n \t\t\t\tprint $c2 \"MIME-Version: 1.0\\n\",\n \t\t\t\t\t \"Content-Type: text/plain; \",\n-\t\t\t\t\t \"charset=$compose_encoding\\n\",\n+\t\t\t\t\t \"charset=$compose_encoding\\n\",\n \t\t\t\t\t \"Content-Transfer-Encoding: 8bit\\n\";\n \t\t\t}\n-\t\t} elsif (/^MIME-Version:/i) {\n-\t\t\t$need_8bit_cte = 0;\n-\t\t} elsif (/^Subject:\\s*(.+)\\s*$/i) {\n-\t\t\t$initial_subject = $1;\n-\t\t\tmy $subject = $initial_subject;\n-\t\t\t$_ = \"Subject: \" .\n-\t\t\t\tquote_subject($subject, $compose_encoding) .\n-\t\t\t\t\"\\n\";\n-\t\t} elsif (/^In-Reply-To:\\s*(.+)\\s*$/i) {\n-\t\t\t$initial_reply_to = $1;\n-\t\t\tnext;\n-\t\t} elsif (/^From:\\s*(.+)\\s*$/i) {\n-\t\t\t$sender = $1;\n-\t\t\tnext;\n-\t\t} elsif (/^(?:To|Cc|Bcc):/i) {\n-\t\t\tprint __(\"To/Cc/Bcc fields are not interpreted yet, they have been ignored\\n\");\n-\t\t\tnext;\n-\t\t}\n-\t\tprint $c2 $_;\n \t}\n+\tif ($parsed_email{'body'}) {\n+\t\t$summary_empty = 0;\n+\t\tprint $c2 \"\\n$parsed_email{'body'}\\n\";\n+\t}\n+\n \tclose $c;\n \tclose $c2;\n \n+\topen $c2, \"<\", $compose_filename . \".final\"\n+\t\tor die sprintf(__(\"Failed to open %s.final: %s\"), $compose_filename, $!);\n+\tclose $c2;\n+\n \tif ($summary_empty) {\n \t\tprint __(\"Summary email is empty, skipping it\\n\");\n \t\t$compose = -1;\n@@ -792,6 +814,20 @@ sub ask {\n \treturn;\n }\n \n+sub parse_header_line {\n+\tmy $lines = shift;\n+\tmy $parsed_line = shift;\n+\n+\tforeach (split(/\\n/, $lines)) {\n+\t\tif (/^(To|Cc|Bcc):\\s*(.+)$/i) {\n+\t\t $parsed_line->{lc $1} = [ parse_address_line($2) ];\n+\t\t} elsif (/^(From|Subject|Date|In-Reply-To|Message-ID|MIME-Version|Content-Type|Content-Transfer-Encoding|References):\\s*(.+)\\s*$/i) {\n+\t\t $parsed_line->{lc $1} = $2;\n+\t\t}\n+\t}\n+}\n+\n+\n my %broken_encoding;\n \n sub file_declares_8bit_cte {\n-- \n2.15.1\n\n"},{"id":"334286","messageId":"xmqqd13rfock.fsf@gitster.mtv.corp.google.com","threadId":"47366","inReplyTo":"CAGb4CBWZciqxdfpSkK1vezhiuSYX5Yy-xSq=Uj4h+vhRo9uyoQ@mail.gmail.com","subject":"Re: [PATCH v2] send-email: extract email-parsing code into a subroutine","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-12-06T23:06:35Z","receivedAt":"2017-12-06T23:06:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nathan PAYRE writes:\n\n> Junio C Hamano : writes:\n>\n>> ... throughout this patch, not limited to this section, indentation\n>> is strange and there seem to be many \"print\" that show messages that\n>> do not seem to be meant for end-user consumption. I can see that\n>> this aspires to improve the readability, but not quite yet ;-).\n>\n> Hmmm I'm wondering who place thoses print in my code !\n> I will fix it fast. :-)\n\nDon't make waste by being hasty, though. The print statements were\nbad, but funny indentation was more distracting and will be worse\nhindrance from the maintainabaility's point of view.\n\nThanks.\n"},{"id":"334287","messageId":"CACBZZX50TAPcoqUU_oREC=4T2uXVrKSFTjR+g-pvqusaX3FURw@mail.gmail.com","threadId":"47366","inReplyTo":"20171206230225.18873-1-nathan.payre@etu.univ-lyon1.fr","subject":"Re: [PATCH v3] send-email: extract email-parsing code into a subroutine","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-12-06T23:12:45Z","receivedAt":"2017-12-06T23:13:12Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Thu, Dec 7, 2017 at 12:02 AM, Nathan Payre\n wrote:\n\n> +sub parse_header_line {\n> + my $lines = shift;\n> + my $parsed_line = shift;\n> +\n> + foreach (split(/\\n/, $lines)) {\n> + if (/^(To|Cc|Bcc):\\s*(.+)$/i) {\n> + $parsed_line->{lc $1} = [ parse_address_line($2) ];\n> + } elsif (/^(From|Subject|Date|In-Reply-To|Message-ID|MIME-Version|Content-Type|Content-Transfer-Encoding|References):\\s*(.+)\\s*$/i) {\n> + $parsed_line->{lc $1} = $2;\n> + }\n> + }\n> +}\n\nNit: As noted in my earlier review this results in very long lines,\nI'm just typing this pseudocode into an E-Mail client so not tested at\nall, but this would be better:\n\n my $header_parsed = join \"|\", qw(To Cc Bcc);\n my $header_unparsed = join \"|\", qw(From Subject Message-ID ...); #\nline wrap this at some point\n foreach [...]\n if (/^($header_parsed)[...]\n } elsif (^/($header_unparsed)[...].\n"},{"id":"334306","messageId":"CAPig+cSmdZds=9P91v4YhHkPyLPaUFcnV1ynMswhKKKauMhRWw@mail.gmail.com","threadId":"47366","inReplyTo":"CAGb4CBWZciqxdfpSkK1vezhiuSYX5Yy-xSq=Uj4h+vhRo9uyoQ@mail.gmail.com","subject":"Re: [PATCH v2] send-email: extract email-parsing code into a subroutine","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2017-12-07T07:32:10Z","receivedAt":"2017-12-07T07:32:18Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Dec 6, 2017 at 5:55 PM, Nathan PAYRE wrote:\n> Junio C Hamano : writes:\n>> Also \"reusable in other place\" is by itself not an unconditional\n>> plus, until readers can be convinced that that 'other place' really\n>> wants to be able to call this function. Is there some untold\n>> motivation behind this change---such as a planned update to actually\n>> use this helper subroutine?\n>\n> This subroutine will be used to implement, initially a new option called\n> \"--quote-email\", but became \"--cite\" added after \"--in-reply-to\".\n> This will permit to the user to cite a mail and reply with a patch and keep\n> Cc, To ...\n> See discussion :\n> https://public-inbox.org/git/20171030223444.5052-1-nathan.payre@etu.univ-lyon1.fr/\n\nAlthough he didn't state so explicitly, please take Junio's question\nas a strong hint that you should update the commit message to include\nthe above rationale/explanation about why you consider this a good\nchange. The reason for including the motivation in the commit message\nis that you want, not only to convince Junio now that this is a useful\nchange, but to convince future readers of the project history that\nthis change is desirable.\n"},{"id":"334307","messageId":"q7h9609jotqd.fsf@orange.lip.ens-lyon.fr","threadId":"47366","inReplyTo":"2b59497271cd4fada4ff590a001446cf@BPMBX2013-01.univ-lyon1.fr","subject":"Re: [PATCH v3] send-email: extract email-parsing code into a subroutine","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@univ-lyon1.fr","sentAt":"2017-12-07T07:57:46Z","receivedAt":"2017-12-07T07:57:59Z","isPatch":true,"sender":{"key":"matthieu.moy@univ-lyon1.fr","avatar":"https://gravatar.com/avatar/8ab83b763226bd297b59ddd1463a8bfc852924577191233f73a953b86888fe0c?d=mp&s=160"},"body":"PAYRE NATHAN p1508475 writes:\n\n> Without the \"print\" used for testing. \n\nBut still smoe broken indentation:\n\n> git-send-email.perl | 90 +++++++++++++++++++++++++++++++++++++----------------\n> 1 file changed, 63 insertions(+), 27 deletions(-)\n>\n> diff --git a/git-send-email.perl b/git-send-email.perl\n> index 2208dcc21..a10574a56 100755\n> --- a/git-send-email.perl\n> +++ b/git-send-email.perl\n> @@ -715,41 +715,63 @@ EOT3\n> \tif (!defined $compose_encoding) {\n> \t\t$compose_encoding = \"UTF-8\";\n> \t}\n> -\twhile(<$c>) {\n> -\t\tnext if m/^GIT:/;\n> -\t\tif ($in_body) {\n> -\t\t\t$summary_empty = 0 unless (/^\\n$/);\n> -\t\t} elsif (/^\\n$/) {\n> -\t\t\t$in_body = 1;\n> -\t\t\tif ($need_8bit_cte) {\n> +\n> + my %parsed_email;\n> +\t$parsed_email{'body'} = '';\n> + while (my $line = <$c>) {\n> +\t next if $line =~ m/^GIT:/;\n> +\t parse_header_line($line, \\%parsed_email);\n> +\t if ($line =~ /^\\n$/i) {\n> +\t while (my $body_line = <$c>) {\n> + if ($body_line !~ m/^GIT:/) {\n> + $parsed_email{'body'} = $parsed_email{'body'} . $body_line;\n> + }\n> +\t }\n> +\t\t}\n> +\t}\n\nThis may display properly in your text editor with your setting, but\nappears broken at least with tab-width=8. Don't mix tabs and spaces. The\nGit coding style is to indent with tabs.\n\nTo see what I mean, open the script in Emacs and type M-x\nwhitespace-mode RET.\n\n-- \nMatthieu Moy\nhttps://matthieu-moy.fr/\n"},{"id":"334315","messageId":"20171207102857.28272-1-nathan.payre@etu.univ-lyon1.fr","threadId":"47366","inReplyTo":"20171206230225.18873-1-nathan.payre@etu.univ-lyon1.fr","subject":"[PATCH v4] send-email: extract email-parsing code into a subroutine","fromName":"Nathan Payre","fromEmail":"nathan.payre@etu.univ-lyon1.fr","sentAt":"2017-12-07T10:28:57Z","receivedAt":"2017-12-07T10:29:14Z","isPatch":true,"sender":{"key":"nathan.payre@etu.univ-lyon1.fr","avatar":null},"body":"The existing code mixes parsing of email header with regular\nexpression and actual code. Extract the parsing code into a new\nsubroutine \"parse_header_line()\". This improves the code readability\nand make parse_header_line reusable in other place.\n\n\"parsed_header_line()\" and \"filter_body()\" could be used for refactoring\nthe part of code which parses the header a last time to prepare the\nemail and send it.\n\nSigned-off-by: Nathan Payre \nSigned-off-by: Matthieu Moy \nSigned-off-by: Timothee Albertin \nSigned-off-by: Daniel Bensoussan \nSigned-off-by: Junio C Hamano \nThanks-to: Ævar Arnfjörð Bjarmason \n---\n\nThis patch fixes the indentation problem and reduce lines over 80 characters.\n\n git-send-email.perl | 102 ++++++++++++++++++++++++++++++++++++++--------------\n 1 file changed, 75 insertions(+), 27 deletions(-)\n\ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex 2208dcc21..b64f8872d 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -715,41 +715,60 @@ EOT3\n \tif (!defined $compose_encoding) {\n \t\t$compose_encoding = \"UTF-8\";\n \t}\n-\twhile(<$c>) {\n-\t\tnext if m/^GIT:/;\n-\t\tif ($in_body) {\n-\t\t\t$summary_empty = 0 unless (/^\\n$/);\n-\t\t} elsif (/^\\n$/) {\n-\t\t\t$in_body = 1;\n-\t\t\tif ($need_8bit_cte) {\n+\n+\n+\tmy %parsed_email;\n+\t$parsed_email{'body'} = '';\n+\twhile (my $line = <$c>) {\n+\t\tnext if $line =~ m/^GIT:/;\n+\t\tparse_header_line($line, \\%parsed_email);\n+\t\tif ($line =~ /^\\n$/i) {\n+\t\t\t$parsed_email{'body'} = filter_body($c);\n+\t\t}\n+\t}\n+\n+\tif ($parsed_email{'from'}) {\n+\t\t$sender = $parsed_email{'from'};\n+\t}\n+\tif ($parsed_email{'in-reply-to'}) {\n+\t\t$initial_reply_to = $parsed_email{'in-reply-to'};\n+\t}\n+\tif ($parsed_email{'subject'}) {\n+\t\t$initial_subject = $parsed_email{'subject'};\n+\t\tprint $c2 \"Subject: \" .\n+\t\t\tquote_subject($parsed_email{'subject'}, $compose_encoding) .\n+\t\t\t\"\\n\";\n+\t}\n+\tif ($parsed_email{'mime-version'}) {\n+\t\t$need_8bit_cte = 0;\n+\t\tprint $c2 \"MIME-Version: $parsed_email{'mime-version'}\\n\",\n+\t\t\t\t\"Content-Type: $parsed_email{'content-type'};\\n\",\n+\t\t\t\t\"Content-Transfer-Encoding: $parsed_email{'content-transfer-encoding'}\\n\";\n+\t}\n+\tif ($need_8bit_cte) {\n+\t\tif ($parsed_email{'content-type'}) {\n+\t\t\t\tprint $c2 \"MIME-Version: 1.0\\n\",\n+\t\t\t\t\t \"Content-Type: $parsed_email{'content-type'};\",\n+\t\t\t\t\t \"Content-Transfer-Encoding: 8bit\\n\";\n+\t\t\t} else {\n \t\t\t\tprint $c2 \"MIME-Version: 1.0\\n\",\n \t\t\t\t\t \"Content-Type: text/plain; \",\n-\t\t\t\t\t \"charset=$compose_encoding\\n\",\n+\t\t\t\t\t \"charset=$compose_encoding\\n\",\n \t\t\t\t\t \"Content-Transfer-Encoding: 8bit\\n\";\n \t\t\t}\n-\t\t} elsif (/^MIME-Version:/i) {\n-\t\t\t$need_8bit_cte = 0;\n-\t\t} elsif (/^Subject:\\s*(.+)\\s*$/i) {\n-\t\t\t$initial_subject = $1;\n-\t\t\tmy $subject = $initial_subject;\n-\t\t\t$_ = \"Subject: \" .\n-\t\t\t\tquote_subject($subject, $compose_encoding) .\n-\t\t\t\t\"\\n\";\n-\t\t} elsif (/^In-Reply-To:\\s*(.+)\\s*$/i) {\n-\t\t\t$initial_reply_to = $1;\n-\t\t\tnext;\n-\t\t} elsif (/^From:\\s*(.+)\\s*$/i) {\n-\t\t\t$sender = $1;\n-\t\t\tnext;\n-\t\t} elsif (/^(?:To|Cc|Bcc):/i) {\n-\t\t\tprint __(\"To/Cc/Bcc fields are not interpreted yet, they have been ignored\\n\");\n-\t\t\tnext;\n-\t\t}\n-\t\tprint $c2 $_;\n \t}\n+\tif ($parsed_email{'body'}) {\n+\t\t$summary_empty = 0;\n+\t\tprint $c2 \"\\n$parsed_email{'body'}\\n\";\n+\t}\n+\n \tclose $c;\n \tclose $c2;\n \n+\topen $c2, \"<\", $compose_filename . \".final\"\n+\t\tor die sprintf(__(\"Failed to open %s.final: %s\"), $compose_filename, $!);\n+\tclose $c2;\n+\n \tif ($summary_empty) {\n \t\tprint __(\"Summary email is empty, skipping it\\n\");\n \t\t$compose = -1;\n@@ -792,6 +811,35 @@ sub ask {\n \treturn;\n }\n \n+sub parse_header_line {\n+\tmy $lines = shift;\n+\tmy $parsed_line = shift;\n+\tmy $pattern1 = join \"|\", qw(To Cc Bcc);\n+\tmy $pattern2 = join \"|\",\n+\t\tqw(From Subject Date In-Reply-To Message-ID MIME-Version \n+\t\t\tContent-Type Content-Transfer-Encoding References);\n+\t\n+\tforeach (split(/\\n/, $lines)) {\n+\t\tif (/^($pattern1):\\s*(.+)$/i) {\n+\t\t $parsed_line->{lc $1} = [ parse_address_line($2) ];\n+\t\t} elsif (/^($pattern2):\\s*(.+)\\s*$/i) {\n+\t\t $parsed_line->{lc $1} = $2;\n+\t\t}\n+\t}\n+}\n+\n+sub filter_body {\n+\tmy $c = shift;\n+\tmy $body = \"\";\n+\twhile (my $body_line = <$c>) {\n+\t\tif ($body_line !~ m/^GIT:/) {\n+\t\t\t$body = $body . $body_line;\n+\t\t}\n+\t}\n+\treturn $body;\n+}\n+\n+\n my %broken_encoding;\n \n sub file_declares_8bit_cte {\n-- \n2.15.1\n\n"},{"id":"334317","messageId":"q7h94lp2oepu.fsf@orange.lip.ens-lyon.fr","threadId":"47366","inReplyTo":"ff9066a7209b4e21867d933542f8eece@BPMBX2013-01.univ-lyon1.fr","subject":"Re: [PATCH v4] send-email: extract email-parsing code into a subroutine","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@univ-lyon1.fr","sentAt":"2017-12-07T13:22:05Z","receivedAt":"2017-12-07T13:22:13Z","isPatch":true,"sender":{"key":"matthieu.moy@univ-lyon1.fr","avatar":"https://gravatar.com/avatar/8ab83b763226bd297b59ddd1463a8bfc852924577191233f73a953b86888fe0c?d=mp&s=160"},"body":"Not terribly important, but your patch has trailing newlines. \"git diff\n--staged --check\" to see them. More below.\n\nPAYRE NATHAN p1508475 writes:\n\n> the part of code which parses the header a last time to prepare the\n> email and send it.\n\nThe important point is not that it's the last time the code parses\nheaders, so I'd drop the \"a last time\".\n\n> +\tmy %parsed_email;\n> +\t$parsed_email{'body'} = '';\n> +\twhile (my $line = <$c>) {\n> +\t\tnext if $line =~ m/^GIT:/;\n> +\t\tparse_header_line($line, \\%parsed_email);\n> +\t\tif ($line =~ /^\\n$/i) {\n\nYou don't need the /i (case-Insensitive) here, there are no letters to\nmatch.\n\n> +\tif ($parsed_email{'mime-version'}) {\n> +\t\t$need_8bit_cte = 0;\n\nThis $need_8bit_cte is a leftover of the old code, which processed the\nheaders in the order it found them in the message and had to remember\nthe content of MIME-Version while parsing Content-Type.\n\nI believe you can apply this on top of your patch:\n\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -709,7 +709,6 @@ EOT3\n open $c, \"<\", $compose_filename\n or die sprintf(__(\"Failed to open %s: %s\"), $compose_filename, $!);\n \n- my $need_8bit_cte = file_has_nonascii($compose_filename);\n my $in_body = 0;\n my $summary_empty = 1;\n if (!defined $compose_encoding) {\n@@ -740,12 +739,10 @@ EOT3\n \"\\n\";\n }\n if ($parsed_email{'mime-version'}) {\n- $need_8bit_cte = 0;\n print $c2 \"MIME-Version: $parsed_email{'mime-version'}\\n\",\n \"Content-Type: $parsed_email{'content-type'};\\n\",\n \"Content-Transfer-Encoding: $parsed_email{'content-transfer-encoding'}\\n\";\n- }\n- if ($need_8bit_cte) {\n+ } else if (file_has_nonascii($compose_filename)) {\n if ($parsed_email{'content-type'}) {\n print $c2 \"MIME-Version: 1.0\\n\",\n \"Content-Type: $parsed_email{'content-type'};\",\n\nIt reads much better: \"If the original message already had a\nMIME-Version header, then use that, else see if the file has non-ascii\ncharacters and if so, use MIME-Version: 1.0\".\n\nActually, you can even simplify further by factoring the if/else below:\n\n> +\t\tif ($parsed_email{'content-type'}) {\n> +\t\t\t\tprint $c2 \"MIME-Version: 1.0\\n\",\n> +\t\t\t\t\t \"Content-Type: $parsed_email{'content-type'};\",\n\n(Suspicious \";\", and suspicious absence of \"\\n\" here, I don't think it's\nintentional and I'm fixing it below, but correct me if I'm wrong)\n\n> +\t\t\t\t\t \"Content-Transfer-Encoding: 8bit\\n\";\n> +\t\t\t} else {\n\n(Broken indentation, this is not aligned with the \"if\" above)\n\n> \t\t\t\tprint $c2 \"MIME-Version: 1.0\\n\",\n> \t\t\t\t\t \"Content-Type: text/plain; \",\n> -\t\t\t\t\t \"charset=$compose_encoding\\n\",\n> +\t\t\t\t\t \"charset=$compose_encoding\\n\",\n> \t\t\t\t\t \"Content-Transfer-Encoding: 8bit\\n\";\n> \t\t\t}\n\nThis could become stg like (untested):\n\n\t} else if (file_has_nonascii($compose_filename)) {\n \tmy $content_type = ($parsed_email{'content-type'} or\n \t\"text/plain; charset=$compose_encoding\");\n\t\tprint $c2 \"MIME-Version: 1.0\\n\",\n\t\t\t \"Content-Type: $content_type\\n\",\n\t\t\t \"Content-Transfer-Encoding: 8bit\\n\";\n\t}\n\n> +\topen $c2, \"<\", $compose_filename . \".final\"\n> +\t\tor die sprintf(__(\"Failed to open %s.final: %s\"), $compose_filename, $!);\n> +\tclose $c2;\n\nWhat is this? Cut-and-paste mistake?\n\n> +sub parse_header_line {\n> +\tmy $lines = shift;\n> +\tmy $parsed_line = shift;\n> +\tmy $pattern1 = join \"|\", qw(To Cc Bcc);\n> +\tmy $pattern2 = join \"|\",\n> +\t\tqw(From Subject Date In-Reply-To Message-ID MIME-Version \n> +\t\t\tContent-Type Content-Transfer-Encoding References);\n> +\t\n> +\tforeach (split(/\\n/, $lines)) {\n> +\t\tif (/^($pattern1):\\s*(.+)$/i) {\n> +\t\t $parsed_line->{lc $1} = [ parse_address_line($2) ];\n> +\t\t} elsif (/^($pattern2):\\s*(.+)\\s*$/i) {\n> +\t\t $parsed_line->{lc $1} = $2;\n> +\t\t}\n\nI don't think you need to list the possibilities in the \"else\" branch.\nJust matching /^([^:]*):\\s*(.+)\\s*$/i should do the trick.\n\n> +\t\t\t$body = $body . $body_line;\n\nOr just: $body .= $body_line;\n\n-- \nMatthieu Moy\nhttps://matthieu-moy.fr/\n"},{"id":"334318","messageId":"87po7qzktt.fsf@evledraar.gmail.com","threadId":"47366","inReplyTo":"q7h94lp2oepu.fsf@orange.lip.ens-lyon.fr","subject":"Re: [PATCH v4] send-email: extract email-parsing code into a subroutine","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-12-07T14:14:38Z","receivedAt":"2017-12-07T14:14:48Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Dec 07 2017, Matthieu Moy jotted:\n\n> Not terribly important, but your patch has trailing newlines. \"git diff\n> --staged --check\" to see them. More below.\n>\n> PAYRE NATHAN p1508475 writes:\n>\n>> the part of code which parses the header a last time to prepare the\n>> email and send it.\n>\n> The important point is not that it's the last time the code parses\n> headers, so I'd drop the \"a last time\".\n>\n>> +\tmy %parsed_email;\n>> +\t$parsed_email{'body'} = '';\n>> +\twhile (my $line = <$c>) {\n>> +\t\tnext if $line =~ m/^GIT:/;\n>> +\t\tparse_header_line($line, \\%parsed_email);\n>> +\t\tif ($line =~ /^\\n$/i) {\n>\n> You don't need the /i (case-Insensitive) here, there are no letters to\n> match.\n\nGood catch, actually this can just be: /^$/. The $ syntax already\nmatches the ending newline, no need for /^\\n$/.\n\n>> +sub parse_header_line {\n>> +\tmy $lines = shift;\n>> +\tmy $parsed_line = shift;\n>> +\tmy $pattern1 = join \"|\", qw(To Cc Bcc);\n>> +\tmy $pattern2 = join \"|\",\n>> +\t\tqw(From Subject Date In-Reply-To Message-ID MIME-Version\n>> +\t\t\tContent-Type Content-Transfer-Encoding References);\n>> +\n>> +\tforeach (split(/\\n/, $lines)) {\n>> +\t\tif (/^($pattern1):\\s*(.+)$/i) {\n>> +\t\t $parsed_line->{lc $1} = [ parse_address_line($2) ];\n>> +\t\t} elsif (/^($pattern2):\\s*(.+)\\s*$/i) {\n>> +\t\t $parsed_line->{lc $1} = $2;\n>> +\t\t}\n>\n> I don't think you need to list the possibilities in the \"else\" branch.\n> Just matching /^([^:]*):\\s*(.+)\\s*$/i should do the trick.\n\nAlthough you'll end up with a lot of stuff in the $parsed_line hash you\ndon't need, which makes dumping it for debugging verbose.\n\nI also wonder about multi-line headers, but then again that probably\nbreaks already on e.g. Message-ID and Refererences, but that's an\nexisting bug unrelated to this patch...\n\n>> +\t\t\t$body = $body . $body_line;\n>\n> Or just: $body .= $body_line;\n"},{"id":"334543","messageId":"20171209153727.30113-1-nathan.payre@etu.univ-lyon1.fr","threadId":"47366","inReplyTo":"CAGb4CBWZciqxdfpSkK1vezhiuSYX5Yy-xSq=Uj4h+vhRo9uyoQ@mail.gmail.com","subject":"[PATCH] send-email: extract email-parsing code into a subroutine","fromName":"Nathan Payre","fromEmail":"nathan.payre@etu.univ-lyon1.fr","sentAt":"2017-12-09T15:37:27Z","receivedAt":"2017-12-09T15:37:56Z","isPatch":true,"sender":{"key":"nathan.payre@etu.univ-lyon1.fr","avatar":null},"body":"The existing code mixes parsing of email header with regular\nexpression and actual code. Extract the parsing code into a new\nsubroutine \"parse_header_line()\". This improves the code readability\nand make parse_header_line reusable in other place.\n\n\"parsed_header_line()\" and \"filter_body()\" could be used for\nrefactoring the part of code which parses the header to prepare the\nemail and send it.\n\nSigned-off-by: Nathan Payre \nSigned-off-by: Matthieu Moy \nSigned-off-by: Timothee Albertin \nSigned-off-by: Daniel Bensoussan \nSigned-off-by: Junio C Hamano \nThanks-to: Ævar Arnfjörð Bjarmason \n---\n\nI fixed the last reported problems and removed some other old\nvariable as $need_8bit_cte.\n\n git-send-email.perl | 110 ++++++++++++++++++++++++++++++++++------------------\n 1 file changed, 73 insertions(+), 37 deletions(-)\n\ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex 2208dcc21..ac36c6aac 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -685,7 +685,7 @@ Lines beginning in \"GIT:\" will be removed.\n Consider including an overall diffstat or table of contents\n for the patch you are writing.\n \n-Clear the body content if you don't wish to send a summary.\n+Clear the body content if you dont wish to send a summary.\n EOT2\n From: $tpl_sender\n Subject: $tpl_subject\n@@ -709,51 +709,61 @@ EOT3\n \topen $c, \"<\", $compose_filename\n \t\tor die sprintf(__(\"Failed to open %s: %s\"), $compose_filename, $!);\n \n-\tmy $need_8bit_cte = file_has_nonascii($compose_filename);\n-\tmy $in_body = 0;\n-\tmy $summary_empty = 1;\n \tif (!defined $compose_encoding) {\n \t\t$compose_encoding = \"UTF-8\";\n \t}\n-\twhile(<$c>) {\n-\t\tnext if m/^GIT:/;\n-\t\tif ($in_body) {\n-\t\t\t$summary_empty = 0 unless (/^\\n$/);\n-\t\t} elsif (/^\\n$/) {\n-\t\t\t$in_body = 1;\n-\t\t\tif ($need_8bit_cte) {\n-\t\t\t\tprint $c2 \"MIME-Version: 1.0\\n\",\n-\t\t\t\t\t \"Content-Type: text/plain; \",\n-\t\t\t\t\t \"charset=$compose_encoding\\n\",\n-\t\t\t\t\t \"Content-Transfer-Encoding: 8bit\\n\";\n-\t\t\t}\n-\t\t} elsif (/^MIME-Version:/i) {\n-\t\t\t$need_8bit_cte = 0;\n-\t\t} elsif (/^Subject:\\s*(.+)\\s*$/i) {\n-\t\t\t$initial_subject = $1;\n-\t\t\tmy $subject = $initial_subject;\n-\t\t\t$_ = \"Subject: \" .\n-\t\t\t\tquote_subject($subject, $compose_encoding) .\n-\t\t\t\t\"\\n\";\n-\t\t} elsif (/^In-Reply-To:\\s*(.+)\\s*$/i) {\n-\t\t\t$initial_reply_to = $1;\n-\t\t\tnext;\n-\t\t} elsif (/^From:\\s*(.+)\\s*$/i) {\n-\t\t\t$sender = $1;\n-\t\t\tnext;\n-\t\t} elsif (/^(?:To|Cc|Bcc):/i) {\n-\t\t\tprint __(\"To/Cc/Bcc fields are not interpreted yet, they have been ignored\\n\");\n-\t\t\tnext;\n+\n+\n+\tmy %parsed_email;\n+\t$parsed_email{'body'} = '';\n+\twhile (my $line = <$c>) {\n+\t\tnext if $line =~ m/^GIT:/;\n+\t\tparse_header_line($line, \\%parsed_email);\n+\t\tif ($line =~ /^$/) {\n+\t\t\t$parsed_email{'body'} = filter_body($c);\n \t\t}\n-\t\tprint $c2 $_;\n \t}\n-\tclose $c;\n-\tclose $c2;\n \n-\tif ($summary_empty) {\n+\tif ($parsed_email{'from'}) {\n+\t\t$sender = $parsed_email{'from'};\n+\t}\n+\tif ($parsed_email{'in-reply-to'}) {\n+\t\t$initial_reply_to = $parsed_email{'in-reply-to'};\n+\t}\n+\tif ($parsed_email{'subject'}) {\n+\t\t$initial_subject = $parsed_email{'subject'};\n+\t\tprint $c2 \"Subject: \" .\n+\t\t\tquote_subject($parsed_email{'subject'}, $compose_encoding) .\n+\t\t\t\"\\n\";\n+\t}\n+\tif ($parsed_email{'mime-version'}) {\n+\t\tprint $c2 \"MIME-Version: $parsed_email{'mime-version'}\\n\",\n+\t\t\t\t\"Content-Type: $parsed_email{'content-type'};\\n\",\n+\t\t\t\t\"Content-Transfer-Encoding: $parsed_email{'content-transfer-encoding'}\\n\";\n+\t}\n+\n+\tif ($parsed_email{'content-type'}) {\n+\t\tprint $c2 \"MIME-Version: 1.0\\n\",\n+\t\t\t \"Content-Type: $parsed_email{'content-type'};\\n\",\n+\t\t\t \"Content-Transfer-Encoding: 8bit\\n\";\n+\t} elsif (file_has_nonascii($compose_filename)) {\n+ my $content_type = ($parsed_email{'content-type'} or\n+ \"text/plain; charset=$compose_encoding\");\n+ print $c2 \"MIME-Version: 1.0\\n\",\n+ \"Content-Type: $content_type\\n\",\n+ \"Content-Transfer-Encoding: 8bit\\n\";\n+ }\n+\n+\tif ($parsed_email{'body'}) {\n+\t\tprint $c2 \"\\n$parsed_email{'body'}\\n\";\n+\t} else {\n \t\tprint __(\"Summary email is empty, skipping it\\n\");\n \t\t$compose = -1;\n \t}\n+\n+\tclose $c;\n+\tclose $c2;\n+\n } elsif ($annotate) {\n \tdo_edit(@files);\n }\n@@ -792,6 +802,32 @@ sub ask {\n \treturn;\n }\n \n+sub parse_header_line {\n+\tmy $lines = shift;\n+\tmy $parsed_line = shift;\n+\tmy $pattern = join \"|\", qw(To Cc Bcc);\n+\t\n+\tforeach (split(/\\n/, $lines)) {\n+\t\tif (/^($pattern):\\s*(.+)$/i) {\n+\t\t $parsed_line->{lc $1} = [ parse_address_line($2) ];\n+\t\t} elsif (/^([^:]*):\\s*(.+)\\s*$/i) {\n+\t\t $parsed_line->{lc $1} = $2;\n+\t\t}\n+\t}\n+}\n+\n+sub filter_body {\n+\tmy $c = shift;\n+\tmy $body = \"\";\n+\twhile (my $body_line = <$c>) {\n+\t\tif ($body_line !~ m/^GIT:/) {\n+\t\t\t$body .= $body_line;\n+\t\t}\n+\t}\n+\treturn $body;\n+}\n+\n+\n my %broken_encoding;\n \n sub file_declares_8bit_cte {\n-- \n2.15.1\n\n"},{"id":"334632","messageId":"1718292310.23922425.1513026746802.JavaMail.zimbra@inria.fr","threadId":"47366","inReplyTo":"34c53164f4054ee88354f19fc38ae0c4@BPMBX2013-01.univ-lyon1.fr","subject":"Re: [PATCH] send-email: extract email-parsing code into a subroutine","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@univ-lyon1.fr","sentAt":"2017-12-11T21:12:26Z","receivedAt":"2017-12-11T21:12:34Z","isPatch":true,"sender":{"key":"matthieu.moy@univ-lyon1.fr","avatar":"https://gravatar.com/avatar/8ab83b763226bd297b59ddd1463a8bfc852924577191233f73a953b86888fe0c?d=mp&s=160"},"body":"\"PAYRE NATHAN p1508475\" wrote:\n\n> --- a/git-send-email.perl\n> +++ b/git-send-email.perl\n> @@ -685,7 +685,7 @@ Lines beginning in \"GIT:\" will be removed.\n> Consider including an overall diffstat or table of contents\n> for the patch you are writing.\n> \n> -Clear the body content if you don't wish to send a summary.\n> +Clear the body content if you dont wish to send a summary.\n\nThis is not part of your patch. Use \"git add -p\" to specify\nexactly which hunks should go into the patch and don't let this\nkind of change end up in the version you send.\n\n> +\tmy %parsed_email;\n> +\t$parsed_email{'body'} = '';\n> +\twhile (my $line = <$c>) {\n> +\t\tnext if $line =~ m/^GIT:/;\n> +\t\tparse_header_line($line, \\%parsed_email);\n> +\t\tif ($line =~ /^$/) {\n> +\t\t\t$parsed_email{'body'} = filter_body($c);\n> \t\t}\n> -\t\tprint $c2 $_;\n\nI didn't notice this at first, but you're modifying the behavior here:\nthe old code used to print to $c2 anything that didn't match any of\nthe if/else if branches.\n\nTo keep this behavior, you need to keep all these extra headers in\n$parsed_email (you do, in this version) and print them after taking\ncare of all the known headers (AFAICT, you don't).\n\n> \t}\n> -\tclose $c;\n> -\tclose $c2;\n\nYou'll still need $c2, but you don't need $c anymore, so I'd keep the\n\"close $c\" here. OTOH, $c2 is not needed before this point (actually a\nbit later), so it would make sense to move the \"open\" down a little.\nThis would materialize the \"read input, then write output\" scheme (as\nopposed to \"write output while reading input\" in the previous code).\nIt's not a new issue in your patch, but giving variables meaningful\nnames (i.e. not $c and $c2) would help, too.\n\n> +\tif ($parsed_email{'mime-version'}) {\n> +\t\tprint $c2 \"MIME-Version: $parsed_email{'mime-version'}\\n\",\n> +\t\t\t\t\"Content-Type: $parsed_email{'content-type'};\\n\",\n> +\t\t\t\t\"Content-Transfer-Encoding: $parsed_email{'content-transfer-encoding'}\\n\";\n> +\t}\n> +\n> +\tif ($parsed_email{'content-type'}) {\n> +\t\tprint $c2 \"MIME-Version: 1.0\\n\",\n> +\t\t\t \"Content-Type: $parsed_email{'content-type'};\\n\",\n> +\t\t\t \"Content-Transfer-Encoding: 8bit\\n\";\n\nThis \"if ($parsed_email{'content-type'})\" does not correspond to\nanything in the old code, and ...\n\n> +\t} elsif (file_has_nonascii($compose_filename)) {\n> + my $content_type = ($parsed_email{'content-type'} or\n> + \"text/plain; charset=$compose_encoding\");\n\nHere, your're dealing explicitly with $parsed_email{'content-type'} !=\nfalse (you're in the 'else' branch where it can only be false).\n\nI think you just meant to drop the \"if\n($parsed_email{'content-type'})\" part, and plug the \"elseif\" directly\nafter the \"if ($parsed_email{'mime-version'})\". That's what I\nsuggested in my earlier email.\n\n> + my $content_type =3D ($parsed_email{'content-type'} or\n> + \"text/plain; charset=3D$compose_encoding\");\n> + print $c2 \"MIME-Version: 1.0\\n\",\n> + \"Content-Type: $content_type\\n\",\n> + \"Content-Transfer-Encoding: 8bit\\n\";\n> + }\n\nThis part is indented with spaces, please use tabs.\n\n-- \nMatthieu Moy\nhttps://matthieu-moy.fr/\n"},{"id":"334798","messageId":"CAGb4CBVcUv111dUy9waScAL2WATkk0LVqJQ55g3-XbH1H228YQ@mail.gmail.com","threadId":"47366","inReplyTo":"1718292310.23922425.1513026746802.JavaMail.zimbra@inria.fr","subject":"Re: [PATCH] send-email: extract email-parsing code into a subroutine","fromName":"Nathan PAYRE","fromEmail":"second.payre@gmail.com","sentAt":"2017-12-14T10:06:08Z","receivedAt":"2017-12-14T10:06:16Z","isPatch":true,"sender":{"key":"second.payre@gmail.com","avatar":null},"body":"2017-12-11 22:12 GMT+01:00 Matthieu Moy :\n\n> \"PAYRE NATHAN p1508475\" wrote:\n>> + my %parsed_email;\n>> + $parsed_email{'body'} = '';\n>> + while (my $line = <$c>) {\n>> + next if $line =~ m/^GIT:/;\n>> + parse_header_line($line, \\%parsed_email);\n>> + if ($line =~ /^$/) {\n>> + $parsed_email{'body'} = filter_body($c);\n>> }\n>> - print $c2 $_;\n>\n> I didn't notice this at first, but you're modifying the behavior here:\n> the old code used to print to $c2 anything that didn't match any of\n> the if/else if branches.\n>\n> To keep this behavior, you need to keep all these extra headers in\n> $parsed_email (you do, in this version) and print them after taking\n> care of all the known headers (AFAICT, you don't).\n\nThis case is not that easy to correct because:\n- It's could weigh the code.\n- The refactoring may not be legitimate anymore.\n\nI've found two way to resolve this:\n.1) After every if($parsed_email{'key'}) remove the corresponding key\nand just before closing $c2 create a new loop which add all the\nremaining parts.\n\n.2) Making a mix between the old and new code. Some parts of\nmy patch can improve the old code (like the removing of\n$need_8bit_cte) then it will be kept and the while loop will be\nsimilar the old code\n\nI think that the first version will look like better than the second\none, easy to read, but it will change the order of the email header.\n"},{"id":"334801","messageId":"20171214111243.13349-1-nathan.payre@etu.univ-lyon1.fr","threadId":"47366","inReplyTo":"CAGb4CBVcUv111dUy9waScAL2WATkk0LVqJQ55g3-XbH1H228YQ@mail.gmail.com","subject":"[PATCH] send-email: extract email-parsing code into a subroutine","fromName":"Nathan Payre","fromEmail":"nathan.payre@etu.univ-lyon1.fr","sentAt":"2017-12-14T11:12:43Z","receivedAt":"2017-12-14T11:13:25Z","isPatch":true,"sender":{"key":"nathan.payre@etu.univ-lyon1.fr","avatar":null},"body":"The existing code mixes parsing of email header with regular\nexpression and actual code. Extract the parsing code into a new\nsubroutine \"parse_header_line()\". This improves the code readability\nand make parse_header_line reusable in other place.\n\n\"parsed_header_line()\" and \"filter_body()\" could be used for\nrefactoring the part of code which parses the header to prepare the\nemail and send it.\n\nSigned-off-by: Nathan Payre \nSigned-off-by: Matthieu Moy \nSigned-off-by: Timothee Albertin \nSigned-off-by: Daniel Bensoussan \nSigned-off-by: Junio C Hamano \nThanks-to: Ævar Arnfjörð Bjarmason \n---\n\n>> \"PAYRE NATHAN p1508475\" wrote:\n>>> + my %parsed_email;\n>>> + $parsed_email{'body'} = '';\n>>> + while (my $line = <$c>) {\n>>> + next if $line =~ m/^GIT:/;\n>>> + parse_header_line($line, \\%parsed_email);\n>>> + if ($line =~ /^$/) {\n>>> + $parsed_email{'body'} = filter_body($c);\n>>> }\n>>> - print $c2 $_;\n>>\n>> I didn't notice this at first, but you're modifying the behavior here:\n>> the old code used to print to $c2 anything that didn't match any of\n>> the if/else if branches.\n>>\n>> To keep this behavior, you need to keep all these extra headers in\n>> $parsed_email (you do, in this version) and print them after taking\n>> care of all the known headers (AFAICT, you don't).\n>\n> This case is not that easy to correct because:\n> - It's could weigh the code.\n> - The refactoring may not be legitimate anymore.\n> \n> I've found two way to resolve this:\n> .1) After every if($parsed_email{'key'}) remove the corresponding key\n> and just before closing $c2 create a new loop which add all the\n> remaining parts.\n>\n> .2) Making a mix between the old and new code. Some parts of\n> my patch can improve the old code (like the removing of\n> $need_8bit_cte) then it will be kept and the while loop will be\n> similar the old code\n>\n> I think that the first version will look like better than the second\n> one, easy to read, but it will change the order of the email header.\n\nThis is how I see the first choice of the two I've proposed in my last\nemail.\n\n git-send-email.perl | 116\n +++++++++++++++++++++++++++++++++++----------------- 1 file changed,\n 78 insertions(+), 38 deletions(-)\n\ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex 2208dcc21..f942fc2a5 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -703,57 +703,71 @@ EOT3\n \t\tdo_edit($compose_filename);\n \t}\n \n-\topen my $c2, \">\", $compose_filename . \".final\"\n-\t\tor die sprintf(__(\"Failed to open %s.final: %s\"), $compose_filename, $!);\n-\n \topen $c, \"<\", $compose_filename\n \t\tor die sprintf(__(\"Failed to open %s: %s\"), $compose_filename, $!);\n \n-\tmy $need_8bit_cte = file_has_nonascii($compose_filename);\n-\tmy $in_body = 0;\n-\tmy $summary_empty = 1;\n \tif (!defined $compose_encoding) {\n \t\t$compose_encoding = \"UTF-8\";\n \t}\n-\twhile(<$c>) {\n-\t\tnext if m/^GIT:/;\n-\t\tif ($in_body) {\n-\t\t\t$summary_empty = 0 unless (/^\\n$/);\n-\t\t} elsif (/^\\n$/) {\n-\t\t\t$in_body = 1;\n-\t\t\tif ($need_8bit_cte) {\n-\t\t\t\tprint $c2 \"MIME-Version: 1.0\\n\",\n-\t\t\t\t\t \"Content-Type: text/plain; \",\n-\t\t\t\t\t \"charset=$compose_encoding\\n\",\n-\t\t\t\t\t \"Content-Transfer-Encoding: 8bit\\n\";\n-\t\t\t}\n-\t\t} elsif (/^MIME-Version:/i) {\n-\t\t\t$need_8bit_cte = 0;\n-\t\t} elsif (/^Subject:\\s*(.+)\\s*$/i) {\n-\t\t\t$initial_subject = $1;\n-\t\t\tmy $subject = $initial_subject;\n-\t\t\t$_ = \"Subject: \" .\n-\t\t\t\tquote_subject($subject, $compose_encoding) .\n-\t\t\t\t\"\\n\";\n-\t\t} elsif (/^In-Reply-To:\\s*(.+)\\s*$/i) {\n-\t\t\t$initial_reply_to = $1;\n-\t\t\tnext;\n-\t\t} elsif (/^From:\\s*(.+)\\s*$/i) {\n-\t\t\t$sender = $1;\n-\t\t\tnext;\n-\t\t} elsif (/^(?:To|Cc|Bcc):/i) {\n-\t\t\tprint __(\"To/Cc/Bcc fields are not interpreted yet, they have been ignored\\n\");\n-\t\t\tnext;\n+\n+\tmy %parsed_email;\n+\twhile (my $line = <$c>) {\n+\t\tnext if $line =~ m/^GIT:/;\n+\t\tparse_header_line($line, \\%parsed_email);\n+\t\tif ($line =~ /^$/) {\n+\t\t\t$parsed_email{'body'} = filter_body($c);\n \t\t}\n-\t\tprint $c2 $_;\n \t}\n+\n \tclose $c;\n-\tclose $c2;\n \n-\tif ($summary_empty) {\n+\topen my $c2, \">\", $compose_filename . \".final\"\n+\tor die sprintf(__(\"Failed to open %s.final: %s\"), $compose_filename, $!);\n+\n+\n+\tif ($parsed_email{'From'}) {\n+\t\t$sender = delete($parsed_email{'From'});\n+\t}\n+\tif ($parsed_email{'In-Reply-To'}) {\n+\t\t$initial_reply_to = delete($parsed_email{'In-Reply-To'});\n+\t}\n+\tif ($parsed_email{'Subject'}) {\n+\t\t$initial_subject = delete($parsed_email{'Subject'});\n+\t\tprint $c2 \"Subject: \" .\n+\t\t\tquote_subject($initial_subject, $compose_encoding) .\n+\t\t\t\"\\n\";\n+\t}\n+\n+\tif ($parsed_email{'MIME-Version'}) {\n+\t\tprint $c2 \"MIME-Version: $parsed_email{'MIME-Version'}\\n\",\n+\t\t\t\t\"Content-Type: $parsed_email{'Content-Type'};\\n\",\n+\t\t\t\t\"Content-Transfer-Encoding: $parsed_email{'Content-Transfer-Encoding'}\\n\";\n+\t\tdelete($parsed_email{'MIME-Version'});\n+\t\tdelete($parsed_email{'Content-Type'});\n+\t\tdelete($parsed_email{'Content-Transfer-Encoding'});\n+\t} elsif (file_has_nonascii($compose_filename)) {\n+\t\tmy $content_type = (delete($parsed_email{'Content-Type'}) or\n+\t\t\t\"text/plain; charset=$compose_encoding\");\n+\t\tprint $c2 \"MIME-Version: 1.0\\n\",\n+\t\t\t\"Content-Type: $content_type\\n\",\n+\t\t\t\"Content-Transfer-Encoding: 8bit\\n\";\n+\t}\n+\n+\tforeach my $key (keys %parsed_email) {\n+\t\tnext if $key == 'body';\n+\t\tprint $c2 \"$key: $parsed_email{$key}\";\n+\t}\n+\n+\tif ($parsed_email{'body'}) {\n+\t\tprint $c2 \"\\n$parsed_email{'body'}\\n\";\n+\t\tdelete($parsed_email{'body'});\n+\t} else {\n \t\tprint __(\"Summary email is empty, skipping it\\n\");\n \t\t$compose = -1;\n \t}\n+\n+\tclose $c2;\n+\n } elsif ($annotate) {\n \tdo_edit(@files);\n }\n@@ -792,6 +806,32 @@ sub ask {\n \treturn;\n }\n \n+sub parse_header_line {\n+\tmy $lines = shift;\n+\tmy $parsed_line = shift;\n+\tmy $pattern = join \"|\", qw(To Cc Bcc);\n+\t\n+\tforeach (split(/\\n/, $lines)) {\n+\t\tif (/^($pattern):\\s*(.+)$/i) {\n+\t\t $parsed_line->{$1} = [ parse_address_line($2) ];\n+\t\t} elsif (/^([^:]*):\\s*(.+)\\s*$/i) {\n+\t\t $parsed_line->{$1} = $2;\n+\t\t}\n+\t}\n+}\n+\n+sub filter_body {\n+\tmy $c = shift;\n+\tmy $body = \"\";\n+\twhile (my $body_line = <$c>) {\n+\t\tif ($body_line !~ m/^GIT:/) {\n+\t\t\t$body .= $body_line;\n+\t\t}\n+\t}\n+\treturn $body;\n+}\n+\n+\n my %broken_encoding;\n \n sub file_declares_8bit_cte {\n-- \n2.15.1\n\n"},{"id":"334805","messageId":"285414621.1144481.1513260333115.JavaMail.zimbra@inria.fr","threadId":"47366","inReplyTo":"ce70816f94c24754bea9bc8175de4bc4@BPMBX2013-01.univ-lyon1.fr","subject":"Re: [PATCH] send-email: extract email-parsing code into a subroutine","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@univ-lyon1.fr","sentAt":"2017-12-14T14:05:33Z","receivedAt":"2017-12-14T14:05:44Z","isPatch":true,"sender":{"key":"matthieu.moy@univ-lyon1.fr","avatar":"https://gravatar.com/avatar/8ab83b763226bd297b59ddd1463a8bfc852924577191233f73a953b86888fe0c?d=mp&s=160"},"body":"\"PAYRE NATHAN p1508475\" wrote:\n\n> -\t\tprint $c2 $_;\n> \t}\n> +\n> \tclose $c;\n\n\nNit: this added newline does not seem necessary to me. Nothing\nserious, but this kind of thing tend to distract the reader when\nreviewing the patch.\n\n> +\tforeach my $key (keys %parsed_email) {\n> +\t\tnext if $key == 'body';\n> +\t\tprint $c2 \"$key: $parsed_email{$key}\";\n> +\t}\n\nI'd add a comment like\n\n\t# Preserve unknown headers\n\nat the top of the loop to make it clear what we're doing.\n\nOn a side note: there's no comment in the code you're adding. This is\nnot necessarily a bad thing (beautifully written code does not need\ncomments to be readable), but you may re-read your code with the\nquestion \"did I explain everything well-enough?\" in mind. The loop\nabove is a case where IMHO a short and sweet comment helps the reader.\n\nTwo potential issues not mentionned in your message but that we\ndiscussed offlist is that 1) this doesn't preserve the order, and 2)\nthis strips duplicate headers. I believe this is not a problem here,\nand trying to solve these points would make the code overkill, but\nthis would really deserve being mentionned in the commit message.\nFirst, so that people reviewing your patch now can confirm (or not)\nthat you are taking the right decision by doing this, and also for\npeople in the future examining your patch (e.g. after a bisect).\n\n> +sub parse_header_line {\n> +\tmy $lines = shift;\n> +\tmy $parsed_line = shift;\n> +\tmy $pattern = join \"|\", qw(To Cc Bcc);\n\nNit: you may want to rename it to something more explicit, like\n$addr_headers_pat.\n\nNone of my nit should block the patch inclusion, but I think the\ncommit message should be expanded to include a mention of the\n\"duplicate headers\"/\"header order\" potential issue.\n\n-- \nMatthieu Moy\nhttps://matthieu-moy.fr/\n"},{"id":"334886","messageId":"20171215153339.12368-1-nathan.payre@etu.univ-lyon1.fr","threadId":"47366","inReplyTo":"285414621.1144481.1513260333115.JavaMail.zimbra@inria.fr","subject":"[PATCH] send-email: extract email-parsing code into a subroutine","fromName":"Nathan Payre","fromEmail":"nathan.payre@etu.univ-lyon1.fr","sentAt":"2017-12-15T15:33:39Z","receivedAt":"2017-12-15T15:34:11Z","isPatch":true,"sender":{"key":"nathan.payre@etu.univ-lyon1.fr","avatar":null},"body":"The existing code mixes parsing of email header with regular\nexpression and actual code. Extract the parsing code into a new\nsubroutine \"parse_header_line()\". This improves the code readability\nand make parse_header_line reusable in other place.\n\n\"parsed_header_line()\" and \"filter_body()\" could be used for\nrefactoring the part of code which parses the header to prepare the\nemail and send it.\n\nIn contrast to the previous version it doesn't keep the header order\nand strip duplicate headers.\n\nSigned-off-by: Nathan Payre \nSigned-off-by: Matthieu Moy \nSigned-off-by: Timothee Albertin \nSigned-off-by: Daniel Bensoussan \nSigned-off-by: Junio C Hamano \nThanks-to: Ævar Arnfjörð Bjarmason \n---\n\n>> +sub parse_header_line {\n>> + my $lines = shift;\n>> + my $parsed_line = shift;\n>> + my $pattern = join \"|\", qw(To Cc Bcc);\n>\n> Nit: you may want to rename it to something more explicit, like\n> $addr_headers_pat.\n\nI find \"$addr_headers_pat\" too long that's why I've choose rename it\ninto \"$addr_pat\", in addition to that, because the variable is in the\nsubroutine \"parse_header_line\" it does not require to include\n\"headers\" in the variable name.\n\n git-send-email.perl | 115 +++++++++++++++++++++++++++++++++++-----------------\n 1 file changed, 77 insertions(+), 38 deletions(-)\n\ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex 2208dcc21..e6e813041 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -703,57 +703,70 @@ EOT3\n \t\tdo_edit($compose_filename);\n \t}\n \n-\topen my $c2, \">\", $compose_filename . \".final\"\n-\t\tor die sprintf(__(\"Failed to open %s.final: %s\"), $compose_filename, $!);\n-\n \topen $c, \"<\", $compose_filename\n \t\tor die sprintf(__(\"Failed to open %s: %s\"), $compose_filename, $!);\n \n-\tmy $need_8bit_cte = file_has_nonascii($compose_filename);\n-\tmy $in_body = 0;\n-\tmy $summary_empty = 1;\n \tif (!defined $compose_encoding) {\n \t\t$compose_encoding = \"UTF-8\";\n \t}\n-\twhile(<$c>) {\n-\t\tnext if m/^GIT:/;\n-\t\tif ($in_body) {\n-\t\t\t$summary_empty = 0 unless (/^\\n$/);\n-\t\t} elsif (/^\\n$/) {\n-\t\t\t$in_body = 1;\n-\t\t\tif ($need_8bit_cte) {\n-\t\t\t\tprint $c2 \"MIME-Version: 1.0\\n\",\n-\t\t\t\t\t \"Content-Type: text/plain; \",\n-\t\t\t\t\t \"charset=$compose_encoding\\n\",\n-\t\t\t\t\t \"Content-Transfer-Encoding: 8bit\\n\";\n-\t\t\t}\n-\t\t} elsif (/^MIME-Version:/i) {\n-\t\t\t$need_8bit_cte = 0;\n-\t\t} elsif (/^Subject:\\s*(.+)\\s*$/i) {\n-\t\t\t$initial_subject = $1;\n-\t\t\tmy $subject = $initial_subject;\n-\t\t\t$_ = \"Subject: \" .\n-\t\t\t\tquote_subject($subject, $compose_encoding) .\n-\t\t\t\t\"\\n\";\n-\t\t} elsif (/^In-Reply-To:\\s*(.+)\\s*$/i) {\n-\t\t\t$initial_reply_to = $1;\n-\t\t\tnext;\n-\t\t} elsif (/^From:\\s*(.+)\\s*$/i) {\n-\t\t\t$sender = $1;\n-\t\t\tnext;\n-\t\t} elsif (/^(?:To|Cc|Bcc):/i) {\n-\t\t\tprint __(\"To/Cc/Bcc fields are not interpreted yet, they have been ignored\\n\");\n-\t\t\tnext;\n+\n+\tmy %parsed_email;\n+\twhile (my $line = <$c>) {\n+\t\tnext if $line =~ m/^GIT:/;\n+\t\tparse_header_line($line, \\%parsed_email);\n+\t\tif ($line =~ /^$/) {\n+\t\t\t$parsed_email{'body'} = filter_body($c);\n \t\t}\n-\t\tprint $c2 $_;\n \t}\n \tclose $c;\n-\tclose $c2;\n \n-\tif ($summary_empty) {\n+\topen my $c2, \">\", $compose_filename . \".final\"\n+\tor die sprintf(__(\"Failed to open %s.final: %s\"), $compose_filename, $!);\n+\n+\n+\tif ($parsed_email{'From'}) {\n+\t\t$sender = delete($parsed_email{'From'});\n+\t}\n+\tif ($parsed_email{'In-Reply-To'}) {\n+\t\t$initial_reply_to = delete($parsed_email{'In-Reply-To'});\n+\t}\n+\tif ($parsed_email{'Subject'}) {\n+\t\t$initial_subject = delete($parsed_email{'Subject'});\n+\t\tprint $c2 \"Subject: \" .\n+\t\t\tquote_subject($initial_subject, $compose_encoding) .\n+\t\t\t\"\\n\";\n+\t}\n+\n+\tif ($parsed_email{'MIME-Version'}) {\n+\t\tprint $c2 \"MIME-Version: $parsed_email{'MIME-Version'}\\n\",\n+\t\t\t\t\"Content-Type: $parsed_email{'Content-Type'};\\n\",\n+\t\t\t\t\"Content-Transfer-Encoding: $parsed_email{'Content-Transfer-Encoding'}\\n\";\n+\t\tdelete($parsed_email{'MIME-Version'});\n+\t\tdelete($parsed_email{'Content-Type'});\n+\t\tdelete($parsed_email{'Content-Transfer-Encoding'});\n+\t} elsif (file_has_nonascii($compose_filename)) {\n+\t\tmy $content_type = (delete($parsed_email{'Content-Type'}) or\n+\t\t\t\"text/plain; charset=$compose_encoding\");\n+\t\tprint $c2 \"MIME-Version: 1.0\\n\",\n+\t\t\t\"Content-Type: $content_type\\n\",\n+\t\t\t\"Content-Transfer-Encoding: 8bit\\n\";\n+\t}\n+\t# Preserve unknown headers\n+\tforeach my $key (keys %parsed_email) {\n+\t\tnext if $key eq 'body';\n+\t\tprint $c2 \"$key: $parsed_email{$key}\";\n+\t}\n+\n+\tif ($parsed_email{'body'}) {\n+\t\tprint $c2 \"\\n$parsed_email{'body'}\\n\";\n+\t\tdelete($parsed_email{'body'});\n+\t} else {\n \t\tprint __(\"Summary email is empty, skipping it\\n\");\n \t\t$compose = -1;\n \t}\n+\n+\tclose $c2;\n+\n } elsif ($annotate) {\n \tdo_edit(@files);\n }\n@@ -792,6 +805,32 @@ sub ask {\n \treturn;\n }\n \n+sub parse_header_line {\n+\tmy $lines = shift;\n+\tmy $parsed_line = shift;\n+\tmy $addr_pat = join \"|\", qw(To Cc Bcc);\n+\t\n+\tforeach (split(/\\n/, $lines)) {\n+\t\tif (/^($addr_pat):\\s*(.+)$/i) {\n+\t\t $parsed_line->{$1} = [ parse_address_line($2) ];\n+\t\t} elsif (/^([^:]*):\\s*(.+)\\s*$/i) {\n+\t\t $parsed_line->{$1} = $2;\n+\t\t}\n+\t}\n+}\n+\n+sub filter_body {\n+\tmy $c = shift;\n+\tmy $body = \"\";\n+\twhile (my $body_line = <$c>) {\n+\t\tif ($body_line !~ m/^GIT:/) {\n+\t\t\t$body .= $body_line;\n+\t\t}\n+\t}\n+\treturn $body;\n+}\n+\n+\n my %broken_encoding;\n \n sub file_declares_8bit_cte {\n-- \n2.15.1\n\n"},{"id":"334888","messageId":"q7h9lgi4m1w1.fsf@orange.lip.ens-lyon.fr","threadId":"47366","inReplyTo":"3cafddfe825a4fb4a554f02aa3c025a3@BPMBX2013-01.univ-lyon1.fr","subject":"Re: [PATCH] send-email: extract email-parsing code into a subroutine","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@univ-lyon1.fr","sentAt":"2017-12-15T15:44:46Z","receivedAt":"2017-12-15T15:45:20Z","isPatch":true,"sender":{"key":"matthieu.moy@univ-lyon1.fr","avatar":"https://gravatar.com/avatar/8ab83b763226bd297b59ddd1463a8bfc852924577191233f73a953b86888fe0c?d=mp&s=160"},"body":"PAYRE NATHAN p1508475 writes:\n\n>>> +sub parse_header_line {\n>>> + my $lines = shift;\n>>> + my $parsed_line = shift;\n>>> + my $pattern = join \"|\", qw(To Cc Bcc);\n>>\n>> Nit: you may want to rename it to something more explicit, like\n>> $addr_headers_pat.\n>\n> I find \"$addr_headers_pat\" too long that's why I've choose rename it\n> into \"$addr_pat\", in addition to that, because the variable is in the\n> subroutine \"parse_header_line\" it does not require to include\n> \"headers\" in the variable name.\n\nI suggested this name because $addr_pat seems to imply that this matches\nan address, while it matches the _name of headers_ containing address.\nBut that's not terribly important, the meaning is clear by the context\nanyway.\n\nAll my previous remarks have been taken into account. This is now\n\nReviewed-by: Matthieu Moy \n\nThanks,\n\n-- \nMatthieu Moy\nhttps://matthieu-moy.fr/\n"}]}