Re: [PATCH] send-email: extract email-parsing code into a subroutine
- From
Matthieu Moy <matthieu.moy@univ-lyon1.fr>
- Date
- Dec 11, 2017, 21:12 UTC
- Message-ID
- <1718292310.23922425.1513026746802.JavaMail.zimbra@inria.fr>
- In-Reply-To
- <34c53164f4054ee88354f19fc38ae0c4@BPMBX2013-01.univ-lyon1.fr>
"PAYRE NATHAN p1508475" <nathan.payre@etu.univ-lyon1.fr> wrote:
Show 8 quoted lines
> --- 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.
Show 9 quoted lines
> + 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.
Show 10 quoted lines
> + 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.
Show 6 quoted lines
> + 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/