Re: [PATCH] send-email: extract email-parsing code into a subroutine

[Matthieu Moy]

PAYRE NATHAN p1508475  writes:

>>> +sub parse_header_line {
>>> + my $lines = shift;
>>> + my $parsed_line = shift;
>>> + my $pattern = join "|", qw(To Cc Bcc);
>>
>> Nit: you may want to rename it to something more explicit, like
>> $addr_headers_pat.
>
> I find "$addr_headers_pat" too long that's why I've choose rename it
> into "$addr_pat", in addition to that, because the variable is in the
> subroutine "parse_header_line" it does not require to include
> "headers" in the variable name.

I suggested this name because $addr_pat seems to imply that this matches
an address, while it matches the _name of headers_ containing address.
But that's not terribly important, the meaning is clear by the context
anyway.

All my previous remarks have been taken into account. This is now

Reviewed-by: Matthieu Moy 

Thanks,

-- 
Matthieu Moy
https://matthieu-moy.fr/

[PATCH] send-email: extract email-parsing code into a subroutine

[Nathan Payre]

The existing code mixes parsing of email header with regular
expression and actual code. Extract the parsing code into a new
subroutine "parse_header_line()". This improves the code readability
and make parse_header_line reusable in other place.

"parsed_header_line()" and "filter_body()" could be used for
refactoring the part of code which parses the header to prepare the
email and send it.

In contrast to the previous version it doesn't keep the header order
and strip duplicate headers.

Signed-off-by: Nathan Payre 
Signed-off-by: Matthieu Moy 
Signed-off-by: Timothee Albertin 
Signed-off-by: Daniel Bensoussan 
Signed-off-by: Junio C Hamano 
Thanks-to: Ævar Arnfjörð Bjarmason 
---

>> +sub parse_header_line {
>> + my $lines = shift;
>> + my $parsed_line = shift;
>> + my $pattern = join "|", qw(To Cc Bcc);
>
> Nit: you may want to rename it to something more explicit, like
> $addr_headers_pat.

I find "$addr_headers_pat" too long that's why I've choose rename it
into "$addr_pat", in addition to that, because the variable is in the
subroutine "parse_header_line" it does not require to include
"headers" in the variable name.

 git-send-email.perl | 115 +++-
 1 file changed, 77 insertions(+), 38 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index 2208dcc21..e6e813041 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -703,57 +703,70 @@ EOT3
do_edit($compose_filename);
}
 
-   open my $c2, ">", $compose_filename . ".final"
-   or die sprintf(__("Failed to open %s.final: %s"), 
$compose_filename, $!);
-
open $c, "<", $compose_filename
or die sprintf(__("Failed to open %s: %s"), $compose_filename, 
$!);
 
-   my $need_8bit_cte = file_has_nonascii($compose_filename);
-   my $in_body = 0;
-   my $summary_empty = 1;
if (!defined $compose_encoding) {
$compose_encoding = "UTF-8";
}
-   while(<$c>) {
-   next if m/^GIT:/;
-   if ($in_body) {
-   $summary_empty = 0 unless (/^\n$/);
-   } elsif (/^\n$/) {
-   $in_body = 1;
-   if ($need_8bit_cte) {
-   print $c2 "MIME-Version: 1.0\n",
-"Content-Type: text/plain; ",
-  "charset=$compose_encoding\n",
-"Content-Transfer-Encoding: 8bit\n";
-   }
-   } elsif (/^MIME-Version:/i) {
-   $need_8bit_cte = 0;
-   } elsif (/^Subject:\s*(.+)\s*$/i) {
-   $initial_subject = $1;
-   my $subject = $initial_subject;
-   $_ = "Subject: " .
-   quote_subject($subject, $compose_encoding) .
-   "\n";
-   } elsif (/^In-Reply-To:\s*(.+)\s*$/i) {
-   $initial_reply_to = $1;
-   next;
-   } elsif (/^From:\s*(.+)\s*$/i) {
-   $sender = $1;
-   next;
-   } elsif (/^(?:To|Cc|Bcc):/i) {
-   print __("To/Cc/Bcc fields are not interpreted yet, 
they have been ignored\n");
-   next;
+
+   my %parsed_email;
+   while (my $line = <$c>) {
+   next if $line =~ m/^GIT:/;
+   parse_header_line($line, \%parsed_email);
+   if ($line =~ /^$/) {
+   $parsed_email{'body'} = filter_body($c);
}
-   print $c2 $_;
}
close $c;
-   close $c2;
 
-   if ($summary_empty) {
+   open my $c2, ">", $compose_filename . ".final"
+   or die sprintf(__("Failed to open %s.final: %s"), $compose_filename, 
$!);
+
+
+   if ($parsed_email{'From'}) {
+   $sender = delete($parsed_email{'From'});
+   }
+   if ($parsed_email{'In-Reply-To'}) {
+   $initial_reply_to = delete($parsed_email{'In-Reply-To'});
+   }
+   if ($parsed_email{'Subject'}) {
+   $initial_subject = delete($parsed_email{'Subject'});
+   print $c2 "Subject: " .
+   quote_subject($initial_subject, $compose_encoding) .
+   "\n";
+   }
+
+   if ($parsed_email{'MIME-Version'}) {
+   print $c2 "MIME-Version: $parsed_email{'MIME-Version'}\n",
+   "Content-Type: 
$parsed_email{'Content-Type'};\n",
+   "Content-Transfer-Encoding: 
$parsed_email{'Content-Transfer-Encoding'}\n";
+   delete($parsed_email{'MIME-Version'});

Re: [PATCH] send-email: extract email-parsing code into a subroutine

[Matthieu Moy]

"PAYRE NATHAN p1508475"  wrote:

> - print $c2 $_;
>   }
> +
>   close $c;


Nit: this added newline does not seem necessary to me. Nothing
serious, but this kind of thing tend to distract the reader when
reviewing the patch.

> + foreach my $key (keys %parsed_email) {
> + next if $key == 'body';
> + print $c2 "$key: $parsed_email{$key}";
> + }

I'd add a comment like

# Preserve unknown headers

at the top of the loop to make it clear what we're doing.

On a side note: there's no comment in the code you're adding. This is
not necessarily a bad thing (beautifully written code does not need
comments to be readable), but you may re-read your code with the
question "did I explain everything well-enough?" in mind. The loop
above is a case where IMHO a short and sweet comment helps the reader.

Two potential issues not mentionned in your message but that we
discussed offlist is that 1) this doesn't preserve the order, and 2)
this strips duplicate headers. I believe this is not a problem here,
and trying to solve these points would make the code overkill, but
this would really deserve being mentionned in the commit message.
First, so that people reviewing your patch now can confirm (or not)
that you are taking the right decision by doing this, and also for
people in the future examining your patch (e.g. after a bisect).

> +sub parse_header_line {
> + my $lines = shift;
> + my $parsed_line = shift;
> + my $pattern = join "|", qw(To Cc Bcc);

Nit: you may want to rename it to something more explicit, like
$addr_headers_pat.

None of my nit should block the patch inclusion, but I think the
commit message should be expanded to include a mention of the
"duplicate headers"/"header order" potential issue.

-- 
Matthieu Moy
https://matthieu-moy.fr/

[PATCH] send-email: extract email-parsing code into a subroutine

[Nathan Payre]

The existing code mixes parsing of email header with regular
expression and actual code. Extract the parsing code into a new
subroutine "parse_header_line()". This improves the code readability
and make parse_header_line reusable in other place.

"parsed_header_line()" and "filter_body()" could be used for
refactoring the part of code which parses the header to prepare the
email and send it.

Signed-off-by: Nathan Payre 
Signed-off-by: Matthieu Moy 
Signed-off-by: Timothee Albertin 
Signed-off-by: Daniel Bensoussan 
Signed-off-by: Junio C Hamano 
Thanks-to: Ævar Arnfjörð Bjarmason 
---

>> "PAYRE NATHAN p1508475"  wrote:
>>> + my %parsed_email;
>>> + $parsed_email{'body'} = '';
>>> + while (my $line = <$c>) {
>>> + next if $line =~ m/^GIT:/;
>>> + parse_header_line($line, \%parsed_email);
>>> + if ($line =~ /^$/) {
>>> + $parsed_email{'body'} = filter_body($c);
>>>   }
>>> - print $c2 $_;
>>
>> I didn't notice this at first, but you're modifying the behavior here:
>> the old code used to print to $c2 anything that didn't match any of
>> the if/else if branches.
>>
>> To keep this behavior, you need to keep all these extra headers in
>> $parsed_email (you do, in this version) and print them after taking
>> care of all the known headers (AFAICT, you don't).
>
> This case is not that easy to correct because:
> - It's could weigh the code.
> - The refactoring may not be legitimate anymore.
> 
> I've found two way to resolve this:
> .1) After every if($parsed_email{'key'}) remove the corresponding key
> and just before closing $c2 create a new loop which add all the
> remaining parts.
>
> .2) Making a mix between the old and new code. Some parts of
> my patch can improve the old code (like the removing of
> $need_8bit_cte) then it will be kept and the while loop will be
> similar the old code
>
> I think that the first version will look like better than the second
> one, easy to read, but it will change the order of the email header.

This is how I see the first choice of the two I've proposed in my last
email.

 git-send-email.perl | 116
 +++- 1 file changed,
 78 insertions(+), 38 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index 2208dcc21..f942fc2a5 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -703,57 +703,71 @@ EOT3
do_edit($compose_filename);
}
 
-   open my $c2, ">", $compose_filename . ".final"
-   or die sprintf(__("Failed to open %s.final: %s"), 
$compose_filename, $!);
-
open $c, "<", $compose_filename
or die sprintf(__("Failed to open %s: %s"), $compose_filename, 
$!);
 
-   my $need_8bit_cte = file_has_nonascii($compose_filename);
-   my $in_body = 0;
-   my $summary_empty = 1;
if (!defined $compose_encoding) {
$compose_encoding = "UTF-8";
}
-   while(<$c>) {
-   next if m/^GIT:/;
-   if ($in_body) {
-   $summary_empty = 0 unless (/^\n$/);
-   } elsif (/^\n$/) {
-   $in_body = 1;
-   if ($need_8bit_cte) {
-   print $c2 "MIME-Version: 1.0\n",
-"Content-Type: text/plain; ",
-  "charset=$compose_encoding\n",
-"Content-Transfer-Encoding: 8bit\n";
-   }
-   } elsif (/^MIME-Version:/i) {
-   $need_8bit_cte = 0;
-   } elsif (/^Subject:\s*(.+)\s*$/i) {
-   $initial_subject = $1;
-   my $subject = $initial_subject;
-   $_ = "Subject: " .
-   quote_subject($subject, $compose_encoding) .
-   "\n";
-   } elsif (/^In-Reply-To:\s*(.+)\s*$/i) {
-   $initial_reply_to = $1;
-   next;
-   } elsif (/^From:\s*(.+)\s*$/i) {
-   $sender = $1;
-   next;
-   } elsif (/^(?:To|Cc|Bcc):/i) {
-   print __("To/Cc/Bcc fields are not interpreted yet, 
they have been ignored\n");
-   next;
+
+   my %parsed_email;
+   while (my $line = <$c>) {
+   next if $line =~ m/^GIT:/;
+   parse_header_line($line, \%parsed_email);
+   if ($line =~ /^$/) {
+   $parsed_email{'body'} = filter_body($c);
}
-   print $c2 $_;
}
+
close $c;
-   close $c2;
 
-   if 

Re: [PATCH] send-email: extract email-parsing code into a subroutine

[Nathan PAYRE]

2017-12-11 22:12 GMT+01:00 Matthieu Moy :

> "PAYRE NATHAN p1508475"  wrote:
>> + my %parsed_email;
>> + $parsed_email{'body'} = '';
>> + while (my $line = <$c>) {
>> + next if $line =~ m/^GIT:/;
>> + parse_header_line($line, \%parsed_email);
>> + if ($line =~ /^$/) {
>> + $parsed_email{'body'} = filter_body($c);
>>   }
>> - print $c2 $_;
>
> I didn't notice this at first, but you're modifying the behavior here:
> the old code used to print to $c2 anything that didn't match any of
> the if/else if branches.
>
> To keep this behavior, you need to keep all these extra headers in
> $parsed_email (you do, in this version) and print them after taking
> care of all the known headers (AFAICT, you don't).

This case is not that easy to correct because:
- It's could weigh the code.
- The refactoring may not be legitimate anymore.

I've found two way to resolve this:
.1) After every if($parsed_email{'key'}) remove the corresponding key
and just before closing $c2 create a new loop which add all the
remaining parts.

.2) Making a mix between the old and new code. Some parts of
my patch can improve the old code (like the removing of
$need_8bit_cte) then it will be kept and the while loop will be
similar the old code

I think that the first version will look like better than the second
one, easy to read, but it will change the order of the email header.

Re: [PATCH] send-email: extract email-parsing code into a subroutine

[Matthieu Moy]

"PAYRE NATHAN p1508475"  wrote:

> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -685,7 +685,7 @@ Lines beginning in "GIT:" will be removed.
>  Consider including an overall diffstat or table of contents
>  for the patch you are writing.
>  
> -Clear the body content if you don't wish to send a summary.
> +Clear the body content if you dont wish to send a summary.

This is not part of your patch. Use "git add -p" to specify
exactly which hunks should go into the patch and don't let this
kind of change end up in the version you send.

> + my %parsed_email;
> + $parsed_email{'body'} = '';
> + while (my $line = <$c>) {
> + next if $line =~ m/^GIT:/;
> + parse_header_line($line, \%parsed_email);
> + if ($line =~ /^$/) {
> + $parsed_email{'body'} = filter_body($c);
>   }
> - print $c2 $_;

I didn't notice this at first, but you're modifying the behavior here:
the old code used to print to $c2 anything that didn't match any of
the if/else if branches.

To keep this behavior, you need to keep all these extra headers in
$parsed_email (you do, in this version) and print them after taking
care of all the known headers (AFAICT, you don't).

>   }
> - close $c;
> - close $c2;

You'll still need $c2, but you don't need $c anymore, so I'd keep the
"close $c" here. OTOH, $c2 is not needed before this point (actually a
bit later), so it would make sense to move the "open" down a little.
This would materialize the "read input, then write output" scheme (as
opposed to "write output while reading input" in the previous code).
It's not a new issue in your patch, but giving variables meaningful
names (i.e. not $c and $c2) would help, too.

> + if ($parsed_email{'mime-version'}) {
> + print $c2 "MIME-Version: $parsed_email{'mime-version'}\n",
> + "Content-Type: 
> $parsed_email{'content-type'};\n",
> + "Content-Transfer-Encoding: 
> $parsed_email{'content-transfer-encoding'}\n";
> + }
> +
> + if ($parsed_email{'content-type'}) {
> + print $c2 "MIME-Version: 1.0\n",
> +  "Content-Type: $parsed_email{'content-type'};\n",
> +  "Content-Transfer-Encoding: 8bit\n";

This "if ($parsed_email{'content-type'})" does not correspond to
anything in the old code, and ...

> + } elsif (file_has_nonascii($compose_filename)) {
> +my $content_type = ($parsed_email{'content-type'} or
> +"text/plain; charset=$compose_encoding");

Here, your're dealing explicitly with $parsed_email{'content-type'} !=
false (you're in the 'else' branch where it can only be false).

I think you just meant to drop the "if
($parsed_email{'content-type'})" part, and plug the "elseif" directly
after the "if ($parsed_email{'mime-version'})". That's what I
suggested in my earlier email.

> +my $content_type =3D ($parsed_email{'content-type'} or
> +"text/plain; charset=3D$compose_encoding");
> +print $c2 "MIME-Version: 1.0\n",
> +  "Content-Type: $content_type\n",
> +  "Content-Transfer-Encoding: 8bit\n";
> +}

This part is indented with spaces, please use tabs.

-- 
Matthieu Moy
https://matthieu-moy.fr/

[PATCH] send-email: extract email-parsing code into a subroutine

[Nathan Payre]

The existing code mixes parsing of email header with regular
expression and actual code. Extract the parsing code into a new
subroutine "parse_header_line()". This improves the code readability
and make parse_header_line reusable in other place.

"parsed_header_line()" and "filter_body()" could be used for
refactoring the part of code which parses the header to prepare the
email and send it.

Signed-off-by: Nathan Payre 
Signed-off-by: Matthieu Moy 
Signed-off-by: Timothee Albertin 
Signed-off-by: Daniel Bensoussan 
Signed-off-by: Junio C Hamano 
Thanks-to: Ævar Arnfjörð Bjarmason 
---

I fixed the last reported problems and removed some other old
variable as $need_8bit_cte.

 git-send-email.perl | 110 ++--
 1 file changed, 73 insertions(+), 37 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index 2208dcc21..ac36c6aac 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -685,7 +685,7 @@ Lines beginning in "GIT:" will be removed.
 Consider including an overall diffstat or table of contents
 for the patch you are writing.
 
-Clear the body content if you don't wish to send a summary.
+Clear the body content if you dont wish to send a summary.
 EOT2
 From: $tpl_sender
 Subject: $tpl_subject
@@ -709,51 +709,61 @@ EOT3
open $c, "<", $compose_filename
or die sprintf(__("Failed to open %s: %s"), $compose_filename, 
$!);
 
-   my $need_8bit_cte = file_has_nonascii($compose_filename);
-   my $in_body = 0;
-   my $summary_empty = 1;
if (!defined $compose_encoding) {
$compose_encoding = "UTF-8";
}
-   while(<$c>) {
-   next if m/^GIT:/;
-   if ($in_body) {
-   $summary_empty = 0 unless (/^\n$/);
-   } elsif (/^\n$/) {
-   $in_body = 1;
-   if ($need_8bit_cte) {
-   print $c2 "MIME-Version: 1.0\n",
-"Content-Type: text/plain; ",
-  "charset=$compose_encoding\n",
-"Content-Transfer-Encoding: 8bit\n";
-   }
-   } elsif (/^MIME-Version:/i) {
-   $need_8bit_cte = 0;
-   } elsif (/^Subject:\s*(.+)\s*$/i) {
-   $initial_subject = $1;
-   my $subject = $initial_subject;
-   $_ = "Subject: " .
-   quote_subject($subject, $compose_encoding) .
-   "\n";
-   } elsif (/^In-Reply-To:\s*(.+)\s*$/i) {
-   $initial_reply_to = $1;
-   next;
-   } elsif (/^From:\s*(.+)\s*$/i) {
-   $sender = $1;
-   next;
-   } elsif (/^(?:To|Cc|Bcc):/i) {
-   print __("To/Cc/Bcc fields are not interpreted yet, 
they have been ignored\n");
-   next;
+
+
+   my %parsed_email;
+   $parsed_email{'body'} = '';
+   while (my $line = <$c>) {
+   next if $line =~ m/^GIT:/;
+   parse_header_line($line, \%parsed_email);
+   if ($line =~ /^$/) {
+   $parsed_email{'body'} = filter_body($c);
}
-   print $c2 $_;
}
-   close $c;
-   close $c2;
 
-   if ($summary_empty) {
+   if ($parsed_email{'from'}) {
+   $sender = $parsed_email{'from'};
+   }
+   if ($parsed_email{'in-reply-to'}) {
+   $initial_reply_to = $parsed_email{'in-reply-to'};
+   }
+   if ($parsed_email{'subject'}) {
+   $initial_subject = $parsed_email{'subject'};
+   print $c2 "Subject: " .
+   quote_subject($parsed_email{'subject'}, 
$compose_encoding) .
+   "\n";
+   }
+   if ($parsed_email{'mime-version'}) {
+   print $c2 "MIME-Version: $parsed_email{'mime-version'}\n",
+   "Content-Type: 
$parsed_email{'content-type'};\n",
+   "Content-Transfer-Encoding: 
$parsed_email{'content-transfer-encoding'}\n";
+   }
+
+   if ($parsed_email{'content-type'}) {
+   print $c2 "MIME-Version: 1.0\n",
+"Content-Type: $parsed_email{'content-type'};\n",
+"Content-Transfer-Encoding: 8bit\n";
+   } elsif (file_has_nonascii($compose_filename)) {
+my $content_type = ($parsed_email{'content-type'} or
+"text/plain; charset=$compose_encoding");
+print $c2 "MIME-Version: 1.0\n",
+  

Re: [PATCH] send-email: extract email-parsing code into a subroutine

[Junio C Hamano]

Nathan PAYRE  writes:

> I've tested your code, and after few changes it's works perfectly!
> The code looks better now.
> Thanks a lot for your review.

Thanks, both of you.  

Could you send in the final version later so that I can pick it up?
I agree with Matthieu's suggestion on what address to use on your
From: (authorship identity) and S-o-b:; as you are showing your work
done as a uni student, the authorship and sign-off should be done as
such.

Re: [PATCH] send-email: extract email-parsing code into a subroutine

[Nathan PAYRE]

I've tested your code, and after few changes it's works perfectly!
The code looks better now.
Thanks a lot for your review.

2017-12-03 23:00 GMT+01:00 Ævar Arnfjörð Bjarmason :
>
> On Sat, Dec 02 2017, Payre Nathan jotted:
>
>> From: Nathan Payre 
>>
>> The existing code mixes parsing of email header with regular
>> expression and actual code. Extract the parsing code into a new
>> subroutine 'parse_header_line()'. This improves the code readability
>> and make parse_header_line reusable in other place.
>>
>> Signed-off-by: Nathan Payre 
>> Signed-off-by: Matthieu Moy 
>> Signed-off-by: Timothee Albertin 
>> Signed-off-by: Daniel Bensoussan 
>> ---
>>
>> This patch is a first step to implement a new feature.
>> See new feature discussion here: 
>> https://public-inbox.org/git/20171030223444.5052-1-nathan.pa...@etu.univ-lyon1.fr/
>>
>>  git-send-email.perl | 106 
>> +++-
>>  1 file changed, 80 insertions(+), 26 deletions(-)
>>
>> diff --git a/git-send-email.perl b/git-send-email.perl
>> index 2208dcc21..98c2e461c 100755
>> --- a/git-send-email.perl
>> +++ b/git-send-email.perl
>> @@ -715,41 +715,64 @@ EOT3
>>   if (!defined $compose_encoding) {
>>   $compose_encoding = "UTF-8";
>>   }
>> - while(<$c>) {
>> +
>> + my %parsed_email;
>> + while (<$c>) {
>>   next if m/^GIT:/;
>> - if ($in_body) {
>> - $summary_empty = 0 unless (/^\n$/);
>> - } elsif (/^\n$/) {
>> - $in_body = 1;
>> - if ($need_8bit_cte) {
>> + parse_header_line($_, \%parsed_email);
>> + if (/^\n$/i) {
>> + while (my $row = <$c>) {
>> + if (!($row =~ m/^GIT:/)) {
>> + $parsed_email{'body'} = 
>> $parsed_email{'body'} . $row;
>> + }
>> + }
>> + }
>> + }
>> + if ($parsed_email{'from'}) {
>> + $sender = $parsed_email{'from'};
>> + }
>> + if ($parsed_email{'in_reply_to'}) {
>> + $initial_reply_to = $parsed_email{'in_reply_to'};
>> + }
>> + if ($parsed_email{'subject'}) {
>> + $initial_subject = $parsed_email{'subject'};
>> + print $c2 "Subject: " .
>> + quote_subject($parsed_email{'subject'}, 
>> $compose_encoding) .
>> + "\n";
>> + }
>> + if ($parsed_email{'mime-version'}) {
>> + $need_8bit_cte = 0;
>> + }
>> + if ($need_8bit_cte) {
>> + if ($parsed_email{'content-type'}) {
>> + print $c2 "MIME-Version: 1.0\n",
>> +  "Content-Type: 
>> $parsed_email{'content-type'};",
>> +  "Content-Transfer-Encoding: 8bit\n";
>> + } else {
>>   print $c2 "MIME-Version: 1.0\n",
>>"Content-Type: text/plain; ",
>> -"charset=$compose_encoding\n",
>> +  "charset=$compose_encoding\n",
>>"Content-Transfer-Encoding: 8bit\n";
>>   }
>> - } elsif (/^MIME-Version:/i) {
>> - $need_8bit_cte = 0;
>> - } elsif (/^Subject:\s*(.+)\s*$/i) {
>> - $initial_subject = $1;
>> - my $subject = $initial_subject;
>> - $_ = "Subject: " .
>> - quote_subject($subject, $compose_encoding) .
>> - "\n";
>> - } elsif (/^In-Reply-To:\s*(.+)\s*$/i) {
>> - $initial_reply_to = $1;
>> - next;
>> - } elsif (/^From:\s*(.+)\s*$/i) {
>> - $sender = $1;
>> - next;
>> - } elsif (/^(?:To|Cc|Bcc):/i) {
>> - print __("To/Cc/Bcc fields are not interpreted yet, 
>> they have been ignored\n");
>> - next;
>> - }
>> - print $c2 $_;
>>   }
>> + if ($parsed_email{'body'}) {
>> + $summary_empty = 0;
>> + print $c2 "\n$parsed_email{'body'}\n";
>> + }
>> +
>>   close $c;
>>   close $c2;
>>
>> + open $c2, "<", $compose_filename . ".final"
>> + or die sprintf(__("Failed to open %s.final: %s"), 
>> $compose_filename, $!);
>> +
>> + print "affichage : \n";
>> + while (<$c2>) {
>> + print $_;
>> + }
>> +
>> + close $c2;
>> +
>>   if ($summary_empty) {
>>   print __("Summary email is empty, skipping it\n");

Re: [PATCH] send-email: extract email-parsing code into a subroutine

[Ævar Arnfjörð Bjarmason]

On Sat, Dec 02 2017, Payre Nathan jotted:

> From: Nathan Payre 
>
> The existing code mixes parsing of email header with regular
> expression and actual code. Extract the parsing code into a new
> subroutine 'parse_header_line()'. This improves the code readability
> and make parse_header_line reusable in other place.
>
> Signed-off-by: Nathan Payre 
> Signed-off-by: Matthieu Moy 
> Signed-off-by: Timothee Albertin 
> Signed-off-by: Daniel Bensoussan 
> ---
>
> This patch is a first step to implement a new feature.
> See new feature discussion here: 
> https://public-inbox.org/git/20171030223444.5052-1-nathan.pa...@etu.univ-lyon1.fr/
>
>  git-send-email.perl | 106 
> +++-
>  1 file changed, 80 insertions(+), 26 deletions(-)
>
> diff --git a/git-send-email.perl b/git-send-email.perl
> index 2208dcc21..98c2e461c 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -715,41 +715,64 @@ EOT3
>   if (!defined $compose_encoding) {
>   $compose_encoding = "UTF-8";
>   }
> - while(<$c>) {
> +
> + my %parsed_email;
> + while (<$c>) {
>   next if m/^GIT:/;
> - if ($in_body) {
> - $summary_empty = 0 unless (/^\n$/);
> - } elsif (/^\n$/) {
> - $in_body = 1;
> - if ($need_8bit_cte) {
> + parse_header_line($_, \%parsed_email);
> + if (/^\n$/i) {
> + while (my $row = <$c>) {
> + if (!($row =~ m/^GIT:/)) {
> + $parsed_email{'body'} = 
> $parsed_email{'body'} . $row;
> + }
> + }
> + }
> + }
> + if ($parsed_email{'from'}) {
> + $sender = $parsed_email{'from'};
> + }
> + if ($parsed_email{'in_reply_to'}) {
> + $initial_reply_to = $parsed_email{'in_reply_to'};
> + }
> + if ($parsed_email{'subject'}) {
> + $initial_subject = $parsed_email{'subject'};
> + print $c2 "Subject: " .
> + quote_subject($parsed_email{'subject'}, 
> $compose_encoding) .
> + "\n";
> + }
> + if ($parsed_email{'mime-version'}) {
> + $need_8bit_cte = 0;
> + }
> + if ($need_8bit_cte) {
> + if ($parsed_email{'content-type'}) {
> + print $c2 "MIME-Version: 1.0\n",
> +  "Content-Type: 
> $parsed_email{'content-type'};",
> +  "Content-Transfer-Encoding: 8bit\n";
> + } else {
>   print $c2 "MIME-Version: 1.0\n",
>"Content-Type: text/plain; ",
> -"charset=$compose_encoding\n",
> +  "charset=$compose_encoding\n",
>"Content-Transfer-Encoding: 8bit\n";
>   }
> - } elsif (/^MIME-Version:/i) {
> - $need_8bit_cte = 0;
> - } elsif (/^Subject:\s*(.+)\s*$/i) {
> - $initial_subject = $1;
> - my $subject = $initial_subject;
> - $_ = "Subject: " .
> - quote_subject($subject, $compose_encoding) .
> - "\n";
> - } elsif (/^In-Reply-To:\s*(.+)\s*$/i) {
> - $initial_reply_to = $1;
> - next;
> - } elsif (/^From:\s*(.+)\s*$/i) {
> - $sender = $1;
> - next;
> - } elsif (/^(?:To|Cc|Bcc):/i) {
> - print __("To/Cc/Bcc fields are not interpreted yet, 
> they have been ignored\n");
> - next;
> - }
> - print $c2 $_;
>   }
> + if ($parsed_email{'body'}) {
> + $summary_empty = 0;
> + print $c2 "\n$parsed_email{'body'}\n";
> + }
> +
>   close $c;
>   close $c2;
>
> + open $c2, "<", $compose_filename . ".final"
> + or die sprintf(__("Failed to open %s.final: %s"), 
> $compose_filename, $!);
> +
> + print "affichage : \n";
> + while (<$c2>) {
> + print $_;
> + }
> +
> + close $c2;
> +
>   if ($summary_empty) {
>   print __("Summary email is empty, skipping it\n");
>   $compose = -1;
> @@ -792,6 +815,37 @@ sub ask {
>   return;
>  }
>
> +sub parse_header_line {
> + my $lines = shift;
> + my $parsed_line = shift;
> +
> + foreach (split(/\n/, $lines)) {
> + if (/^From:\s*(.+)$/i) {
> + $parsed_line->{'from'} = $1;
> + 

Re: [PATCH] send-email: extract email-parsing code into a subroutine

[Matthieu Moy]

Nathan PAYRE  writes:

> I found a mistake in my signed-off-by, please replace
>  by 

I think you want exactly the opposite of this. You're contributing as a
Lyon 1 student, hence your identity is @etu.univ-lyon1.fr. Your Gmail
adress is used only for technical reasons.

OTOH, you are missing the first line From: ... @..univ-lyon1.fr in your
message.

See how you did it:

https://public-inbox.org/git/20171012091727.30759-1-second.pa...@gmail.com/

(The sign-off was wrong in this one, but the From was OK)

-- 
Matthieu Moy
https://matthieu-moy.fr/

Re: [PATCH] send-email: extract email-parsing code into a subroutine

[Nathan PAYRE]

I found a mistake in my signed-off-by, please replace
 by 

Excuse me.

2017-12-02 18:02 GMT+01:00 Payre Nathan :
> From: Nathan Payre 
>
> The existing code mixes parsing of email header with regular
> expression and actual code. Extract the parsing code into a new
> subroutine 'parse_header_line()'. This improves the code readability
> and make parse_header_line reusable in other place.
>
> Signed-off-by: Nathan Payre 
> Signed-off-by: Matthieu Moy 
> Signed-off-by: Timothee Albertin 
> Signed-off-by: Daniel Bensoussan 
> ---
>
> This patch is a first step to implement a new feature.
> See new feature discussion here: 
> https://public-inbox.org/git/20171030223444.5052-1-nathan.pa...@etu.univ-lyon1.fr/
>
>  git-send-email.perl | 106 
> +++-
>  1 file changed, 80 insertions(+), 26 deletions(-)
>
> diff --git a/git-send-email.perl b/git-send-email.perl
> index 2208dcc21..98c2e461c 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -715,41 +715,64 @@ EOT3
> if (!defined $compose_encoding) {
> $compose_encoding = "UTF-8";
> }
> -   while(<$c>) {
> +
> +   my %parsed_email;
> +   while (<$c>) {
> next if m/^GIT:/;
> -   if ($in_body) {
> -   $summary_empty = 0 unless (/^\n$/);
> -   } elsif (/^\n$/) {
> -   $in_body = 1;
> -   if ($need_8bit_cte) {
> +   parse_header_line($_, \%parsed_email);
> +   if (/^\n$/i) {
> +   while (my $row = <$c>) {
> +   if (!($row =~ m/^GIT:/)) {
> +   $parsed_email{'body'} = 
> $parsed_email{'body'} . $row;
> +   }
> +   }
> +   }
> +   }
> +   if ($parsed_email{'from'}) {
> +   $sender = $parsed_email{'from'};
> +   }
> +   if ($parsed_email{'in_reply_to'}) {
> +   $initial_reply_to = $parsed_email{'in_reply_to'};
> +   }
> +   if ($parsed_email{'subject'}) {
> +   $initial_subject = $parsed_email{'subject'};
> +   print $c2 "Subject: " .
> +   quote_subject($parsed_email{'subject'}, 
> $compose_encoding) .
> +   "\n";
> +   }
> +   if ($parsed_email{'mime-version'}) {
> +   $need_8bit_cte = 0;
> +   }
> +   if ($need_8bit_cte) {
> +   if ($parsed_email{'content-type'}) {
> +   print $c2 "MIME-Version: 1.0\n",
> +"Content-Type: 
> $parsed_email{'content-type'};",
> +"Content-Transfer-Encoding: 8bit\n";
> +   } else {
> print $c2 "MIME-Version: 1.0\n",
>  "Content-Type: text/plain; ",
> -  "charset=$compose_encoding\n",
> +"charset=$compose_encoding\n",
>  "Content-Transfer-Encoding: 8bit\n";
> }
> -   } elsif (/^MIME-Version:/i) {
> -   $need_8bit_cte = 0;
> -   } elsif (/^Subject:\s*(.+)\s*$/i) {
> -   $initial_subject = $1;
> -   my $subject = $initial_subject;
> -   $_ = "Subject: " .
> -   quote_subject($subject, $compose_encoding) .
> -   "\n";
> -   } elsif (/^In-Reply-To:\s*(.+)\s*$/i) {
> -   $initial_reply_to = $1;
> -   next;
> -   } elsif (/^From:\s*(.+)\s*$/i) {
> -   $sender = $1;
> -   next;
> -   } elsif (/^(?:To|Cc|Bcc):/i) {
> -   print __("To/Cc/Bcc fields are not interpreted yet, 
> they have been ignored\n");
> -   next;
> -   }
> -   print $c2 $_;
> }
> +   if ($parsed_email{'body'}) {
> +   $summary_empty = 0;
> +   print $c2 "\n$parsed_email{'body'}\n";
> +   }
> +
> close $c;
> close $c2;
>
> +   open $c2, "<", $compose_filename . ".final"
> +   or die sprintf(__("Failed to open %s.final: %s"), 
> $compose_filename, $!);
> +
> +   print "affichage : \n";
> +   while (<$c2>) {
> +   print $_;
> +   }
> +
> +   close $c2;
> +
> if ($summary_empty) {
> print __("Summary email is empty, skipping it\n");
>  

[PATCH] send-email: extract email-parsing code into a subroutine

[Payre Nathan]

From: Nathan Payre 

The existing code mixes parsing of email header with regular
expression and actual code. Extract the parsing code into a new
subroutine 'parse_header_line()'. This improves the code readability
and make parse_header_line reusable in other place.

Signed-off-by: Nathan Payre 
Signed-off-by: Matthieu Moy 
Signed-off-by: Timothee Albertin 
Signed-off-by: Daniel Bensoussan 
---

This patch is a first step to implement a new feature.
See new feature discussion here: 
https://public-inbox.org/git/20171030223444.5052-1-nathan.pa...@etu.univ-lyon1.fr/

 git-send-email.perl | 106 +++-
 1 file changed, 80 insertions(+), 26 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index 2208dcc21..98c2e461c 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -715,41 +715,64 @@ EOT3
if (!defined $compose_encoding) {
$compose_encoding = "UTF-8";
}
-   while(<$c>) {
+
+   my %parsed_email;
+   while (<$c>) {
next if m/^GIT:/;
-   if ($in_body) {
-   $summary_empty = 0 unless (/^\n$/);
-   } elsif (/^\n$/) {
-   $in_body = 1;
-   if ($need_8bit_cte) {
+   parse_header_line($_, \%parsed_email);
+   if (/^\n$/i) {
+   while (my $row = <$c>) {
+   if (!($row =~ m/^GIT:/)) {
+   $parsed_email{'body'} = 
$parsed_email{'body'} . $row;
+   }
+   }
+   }
+   }
+   if ($parsed_email{'from'}) {
+   $sender = $parsed_email{'from'};
+   }
+   if ($parsed_email{'in_reply_to'}) {
+   $initial_reply_to = $parsed_email{'in_reply_to'};
+   }
+   if ($parsed_email{'subject'}) {
+   $initial_subject = $parsed_email{'subject'};
+   print $c2 "Subject: " .
+   quote_subject($parsed_email{'subject'}, 
$compose_encoding) .
+   "\n";
+   }
+   if ($parsed_email{'mime-version'}) {
+   $need_8bit_cte = 0;
+   }
+   if ($need_8bit_cte) {
+   if ($parsed_email{'content-type'}) {
+   print $c2 "MIME-Version: 1.0\n",
+"Content-Type: 
$parsed_email{'content-type'};",
+"Content-Transfer-Encoding: 8bit\n";
+   } else {
print $c2 "MIME-Version: 1.0\n",
 "Content-Type: text/plain; ",
-  "charset=$compose_encoding\n",
+"charset=$compose_encoding\n",
 "Content-Transfer-Encoding: 8bit\n";
}
-   } elsif (/^MIME-Version:/i) {
-   $need_8bit_cte = 0;
-   } elsif (/^Subject:\s*(.+)\s*$/i) {
-   $initial_subject = $1;
-   my $subject = $initial_subject;
-   $_ = "Subject: " .
-   quote_subject($subject, $compose_encoding) .
-   "\n";
-   } elsif (/^In-Reply-To:\s*(.+)\s*$/i) {
-   $initial_reply_to = $1;
-   next;
-   } elsif (/^From:\s*(.+)\s*$/i) {
-   $sender = $1;
-   next;
-   } elsif (/^(?:To|Cc|Bcc):/i) {
-   print __("To/Cc/Bcc fields are not interpreted yet, 
they have been ignored\n");
-   next;
-   }
-   print $c2 $_;
}
+   if ($parsed_email{'body'}) {
+   $summary_empty = 0;
+   print $c2 "\n$parsed_email{'body'}\n";
+   }
+
close $c;
close $c2;
 
+   open $c2, "<", $compose_filename . ".final"
+   or die sprintf(__("Failed to open %s.final: %s"), 
$compose_filename, $!);
+
+   print "affichage : \n";
+   while (<$c2>) {
+   print $_;
+   }
+
+   close $c2;
+
if ($summary_empty) {
print __("Summary email is empty, skipping it\n");
$compose = -1;
@@ -792,6 +815,37 @@ sub ask {
return;
 }
 
+sub parse_header_line {
+   my $lines = shift;
+   my $parsed_line = shift;
+
+   foreach (split(/\n/, $lines)) {
+   if (/^From:\s*(.+)$/i) {
+   $parsed_line->{'from'} = $1;
+   } elsif (/^To:\s*(.+)$/i) {
+   $parsed_line->{'to'} = [ parse_address_line($1) ];
+