git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Nathan PAYRE <second.payre@gmail.com>
To: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Cc: Git Mailing List <git@vger.kernel.org>,
	Ryan Anderson <ryan@michonline.com>,
	e@80x24.org, Junio C Hamano <gitster@pobox.com>,
	Nathan Payre <nathan.payre@etu.univ-lyon1.fr>,
	Matthieu Moy <matthieu.moy@univ-lyon1.fr>,
	Timothee Albertin <timothee.albertin@etu.univ-lyon1.fr>,
	Daniel Bensoussan <daniel.bensoussan--bohm@etu.univ-lyon1.fr>
Subject: Re: [PATCH] send-email: extract email-parsing code into a subroutine
Date: Mon, 4 Dec 2017 00:41:04 +0100	[thread overview]
Message-ID: <CAGb4CBUY0QVOHqAFDi6kSVmK48PzKKTuXR2F_Nr+6cHHUhxBzQ@mail.gmail.com> (raw)
In-Reply-To: <874lp7v5du.fsf@evledraar.booking.com>

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 <avarab@gmail.com>:
>
> On Sat, Dec 02 2017, Payre Nathan jotted:
>
>> From: Nathan Payre <second.payre@gmail.com>
>>
>> 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 <nathan.payre@etu.univ-lyon1.fr>
>> Signed-off-by: Matthieu Moy <matthieu.moy@univ-lyon1.fr>
>> Signed-off-by: Timothee Albertin <timothee.albertin@etu.univ-lyon1.fr>
>> Signed-off-by: Daniel Bensoussan <daniel.bensoussan--bohm@etu.univ-lyon1.fr>
>> ---
>>
>> 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.payre@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) ];
>> +             } elsif (/^Cc:\s*(.+)$/i) {
>> +                     $parsed_line->{'cc'} = [ parse_address_line($1) ];
>> +             } elsif (/^Bcc:\s*(.+)$/i) {
>> +                     $parsed_line->{'bcc'} = [ parse_address_line($1) ];
>> +             } elsif (/^Subject:\s*(.+)\s*$/i) {
>> +                     $parsed_line->{'subject'} = $1;
>> +             } elsif (/^Date: (.*)/i) {
>> +                     $parsed_line->{'date'} = $1;
>> +             } elsif (/^In-Reply-To:\s*(.+)\s*$/i) {
>> +                     $parsed_line->{'in_reply_to'} = $1;
>> +             } elsif (/^Message-ID: (.*)$/i) {
>> +                     $parsed_line->{'message_id'} = $1;
>> +             } elsif (/^MIME-Version:$/i) {
>> +                     $parsed_line->{'mime-version'} = $1;
>> +             } elsif (/^Content-Type:\s+(.*)\s*$/i) {
>> +                     $parsed_line->{'content-type'} = $1;
>> +             } elsif (/^References:\s+(.*)/i) {
>> +                     $parsed_line->{'references'} = $1;
>> +             }
>> +     }
>> +}
>> +
>>  my %broken_encoding;
>>
>>  sub file_declares_8bit_cte {
>
> I haven't read the patches that follow. Completely untested, But just a
> diff on top I came up with while reading this:
>
> Rationale:
>
>  * Once you start passing $_ to functions you should probably just give
>    it a name.
>
>  * !($x =~ m//) you can just write as $x !~ m//
>
>  * There's a lot of copy/paste programming in parse_header_line() and an
>    inconsistency between you seeing A-Header and turning it into either
>    a_header or a-header. If you just stick with a-header and use dash
>    you end up with just two cases.
>
>    The resulting line is quite long, so it's worth doing:
>
>    my $header_parsed   = join "|", qw(To CC ...);
>    my $header_unparsed = join "|", qw(From Subject Message-ID ...);
>    [...]
>    if ($str =~ /^($header_unparsed)
>
>
> diff --git a/git-send-email.perl b/git-send-email.perl
> index 98c2e461cf..3696cad456 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -717,12 +717,12 @@ EOT3
>         }
>
>         my %parsed_email;
> -       while (<$c>) {
> -               next if m/^GIT:/;
> -               parse_header_line($_, \%parsed_email);
> -               if (/^\n$/i) {
> +       while (my $line = <$c>) {
> +               next if $line =~ m/^GIT:/;
> +               parse_header_line($line, \%parsed_email);
> +               if ($line =~ /^\n$/i) {
>                         while (my $row = <$c>) {
> -                               if (!($row =~ m/^GIT:/)) {
> +                               if ($row !~ m/^GIT:/) {
>                                         $parsed_email{'body'} = $parsed_email{'body'} . $row;
>                                 }
>                         }
> @@ -731,7 +731,7 @@ EOT3
>         if ($parsed_email{'from'}) {
>                 $sender = $parsed_email{'from'};
>         }
> -       if ($parsed_email{'in_reply_to'}) {
> +       if ($parsed_email{'in-reply-to'}) {
>                 $initial_reply_to = $parsed_email{'in_reply_to'};
>         }
>         if ($parsed_email{'subject'}) {
> @@ -820,28 +820,10 @@ sub parse_header_line {
>         my $parsed_line = shift;
>
>         foreach (split(/\n/, $lines)) {
> -               if (/^From:\s*(.+)$/i) {
> -                       $parsed_line->{'from'} = $1;
> -               } elsif (/^To:\s*(.+)$/i) {
> -                       $parsed_line->{'to'} = [ parse_address_line($1) ];
> -               } elsif (/^Cc:\s*(.+)$/i) {
> -                       $parsed_line->{'cc'} = [ parse_address_line($1) ];
> -               } elsif (/^Bcc:\s*(.+)$/i) {
> -                       $parsed_line->{'bcc'} = [ parse_address_line($1) ];
> -               } elsif (/^Subject:\s*(.+)\s*$/i) {
> -                       $parsed_line->{'subject'} = $1;
> -               } elsif (/^Date: (.*)/i) {
> -                       $parsed_line->{'date'} = $1;
> -               } elsif (/^In-Reply-To:\s*(.+)\s*$/i) {
> -                       $parsed_line->{'in_reply_to'} = $1;
> -               } elsif (/^Message-ID: (.*)$/i) {
> -                       $parsed_line->{'message_id'} = $1;
> -               } elsif (/^MIME-Version:$/i) {
> -                       $parsed_line->{'mime-version'} = $1;
> -               } elsif (/^Content-Type:\s+(.*)\s*$/i) {
> -                       $parsed_line->{'content-type'} = $1;
> -               } elsif (/^References:\s+(.*)/i) {
> -                       $parsed_line->{'references'} = $1;
> +               if (/^(To|Cc|Bcc):\s*(.+)$/i) {
> +                       $parsed_line->{lc $1} = [ parse_address_line($2) ];
> +               } elsif (/^(From|Subject|Date|In-Reply-To|Message-ID|MIME-Version|Content-Type|References):\s*(.+)\s*$/i) {
> +                       $parsed_line->{lc $1} = $2;
>                 }
>         }
>  }

  reply	other threads:[~2017-12-03 23:41 UTC|newest]

Thread overview: 25+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-12-02 17:02 [PATCH] send-email: extract email-parsing code into a subroutine Payre Nathan
2017-12-02 17:11 ` Nathan PAYRE
     [not found] ` <445d0838cf2a4107bad95d5cc2d38a05@BPMBX2013-01.univ-lyon1.fr>
2017-12-03 21:20   ` Matthieu Moy
2017-12-03 22:00 ` Ævar Arnfjörð Bjarmason
2017-12-03 23:41   ` Nathan PAYRE [this message]
2017-12-04 13:45     ` Junio C Hamano
2017-12-06 15:38       ` [PATCH v2] " Nathan Payre
2017-12-06 21:39         ` Junio C Hamano
2017-12-06 22:55           ` Nathan PAYRE
2017-12-06 23:02             ` [PATCH v3] " Nathan Payre
2017-12-06 23:12               ` Ævar Arnfjörð Bjarmason
2017-12-07 10:28               ` [PATCH v4] " Nathan Payre
     [not found]               ` <ff9066a7209b4e21867d933542f8eece@BPMBX2013-01.univ-lyon1.fr>
2017-12-07 13:22                 ` Matthieu Moy
2017-12-07 14:14                   ` Ævar Arnfjörð Bjarmason
2017-12-06 23:06             ` [PATCH v2] " Junio C Hamano
2017-12-07  7:32             ` Eric Sunshine
     [not found]             ` <2b59497271cd4fada4ff590a001446cf@BPMBX2013-01.univ-lyon1.fr>
2017-12-07  7:57               ` [PATCH v3] " Matthieu Moy
2017-12-09 15:37             ` [PATCH] " Nathan Payre
     [not found]             ` <34c53164f4054ee88354f19fc38ae0c4@BPMBX2013-01.univ-lyon1.fr>
2017-12-11 21:12               ` Matthieu Moy
2017-12-14 10:06                 ` Nathan PAYRE
2017-12-14 11:12                   ` Nathan Payre
     [not found]                   ` <ce70816f94c24754bea9bc8175de4bc4@BPMBX2013-01.univ-lyon1.fr>
2017-12-14 14:05                     ` Matthieu Moy
2017-12-15 15:33                       ` Nathan Payre
     [not found]                       ` <3cafddfe825a4fb4a554f02aa3c025a3@BPMBX2013-01.univ-lyon1.fr>
2017-12-15 15:44                         ` Matthieu Moy
2017-12-06 15:32     ` [PATCH v2] " Nathan Payre

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAGb4CBUY0QVOHqAFDi6kSVmK48PzKKTuXR2F_Nr+6cHHUhxBzQ@mail.gmail.com \
    --to=second.payre@gmail.com \
    --cc=avarab@gmail.com \
    --cc=daniel.bensoussan--bohm@etu.univ-lyon1.fr \
    --cc=e@80x24.org \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=matthieu.moy@univ-lyon1.fr \
    --cc=nathan.payre@etu.univ-lyon1.fr \
    --cc=ryan@michonline.com \
    --cc=timothee.albertin@etu.univ-lyon1.fr \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).