Skip to content

Commit 637e894

Browse files
peffgitster
peff
authored and
gitster
committed
Revert "send-email: extract email-parsing code into a subroutine"
This reverts commit b604954. Prior to that commit, we read the results of the user editing the "--compose" message in a loop, picking out parts we cared about, and streaming the result out to a ".final" file. That commit split the reading/interpreting into two phases; we'd now read into a hash, and then pick things out of the hash. The goal was making the code more readable. And in some ways it did, because the ugly regexes are confined to the reading phase. But it also introduced several bugs, because now the two phases need to match each other. In particular: - we pick out headers like "Subject: foo" with a case-insensitive regex, and then use the user-provided header name as the key in a case-sensitive hash. So if the user wrote "subject: foo", we'd no longer recognize it as a subject. - the namespace for the hash keys conflates header names with meta information like "body". If you put "body: foo" in your message, it would be misinterpreted as the actual message body (nobody is likely to do that in practice, but it seems like an unnecessary danger). - the handling for to/cc/bcc is totally broken. The behavior before that commit is to recognize and skip those headers, with a note to the user that they are not yet handled. Not great, but OK. But after the patch, the reading side now splits the addresses into a perl array-ref. But the interpreting side doesn't handle this at all, and blindly prints the stringified array-ref value. This leads to garbage like: (mbox) Adding to: ARRAY (0x555b4345c428) from line 'To: ARRAY(0x555b4345c428)' error: unable to extract a valid address from: ARRAY (0x555b4345c428) What to do with this address? ([q]uit|[d]rop|[e]dit): Probably not a huge deal, since nobody should even try to use those headers in the first place (since they were not implemented). But the new behavior is worse, and indicative of the sorts of problems that come from having the two layers. The revert had a few conflicts, due to later work in this area from 15dc3b9 (send-email: rename variable for clarity, 2018-03-04) and d11c943 (send-email: support separate Reply-To address, 2018-03-04). I've ported the changes from those commits over as part of the conflict resolution. The new tests show the bugs. Note the use of GIT_SEND_EMAIL_NOTTY in the second one. Without it, the test is happy to reach outside the test harness to the developer's actual terminal (when run with the buggy state before this patch). Signed-off-by: Jeff King <[email protected]> Signed-off-by: Junio C Hamano <[email protected]>
1 parent e0c7e2c commit 637e894

File tree

2 files changed

+75
-80
lines changed

2 files changed

+75
-80
lines changed

‎git-send-email.perl

Lines changed: 40 additions & 80 deletions
Original file line numberDiff line numberDiff line change
@@ -888,73 +888,59 @@ sub get_patch_subject {
888888
do_edit($compose_filename);
889889
}
890890
891+
open my $c2, ">", $compose_filename . ".final"
892+
or die sprintf(__("Failed to open %s.final: %s"), $compose_filename, $!);
893+
891894
open $c, "<", $compose_filename
892895
or die sprintf(__("Failed to open %s: %s"), $compose_filename, $!);
893896
897+
my $need_8bit_cte = file_has_nonascii($compose_filename);
898+
my $in_body = 0;
899+
my $summary_empty = 1;
894900
if (!defined $compose_encoding) {
895901
$compose_encoding = "UTF-8";
896902
}
897-
898-
my %parsed_email;
899-
while (my $line = <$c>) {
900-
next if $line =~ m/^GIT:/;
901-
parse_header_line($line, \%parsed_email);
902-
if ($line =~ /^$/) {
903-
$parsed_email{'body'} = filter_body($c);
903+
while(<$c>) {
904+
next if m/^GIT:/;
905+
if ($in_body) {
906+
$summary_empty = 0 unless (/^\n$/);
907+
} elsif (/^\n$/) {
908+
$in_body = 1;
909+
if ($need_8bit_cte) {
910+
print $c2 "MIME-Version: 1.0\n",
911+
"Content-Type: text/plain; ",
912+
"charset=$compose_encoding\n",
913+
"Content-Transfer-Encoding: 8bit\n";
914+
}
915+
} elsif (/^MIME-Version:/i) {
916+
$need_8bit_cte = 0;
917+
} elsif (/^Subject:\s*(.+)\s*$/i) {
918+
$initial_subject = $1;
919+
my $subject = $initial_subject;
920+
$_ = "Subject: " .
921+
quote_subject($subject, $compose_encoding) .
922+
"\n";
923+
} elsif (/^In-Reply-To:\s*(.+)\s*$/i) {
924+
$initial_in_reply_to = $1;
925+
next;
926+
} elsif (/^Reply-To:\s*(.+)\s*$/i) {
927+
$reply_to = $1;
928+
} elsif (/^From:\s*(.+)\s*$/i) {
929+
$sender = $1;
930+
next;
931+
} elsif (/^(?:To|Cc|Bcc):/i) {
932+
print __("To/Cc/Bcc fields are not interpreted yet, they have been ignored\n");
933+
next;
904934
}
935+
print $c2 $_;
905936
}
906937
close $c;
938+
close $c2;
907939
908-
open my $c2, ">", $compose_filename . ".final"
909-
or die sprintf(__("Failed to open %s.final: %s"), $compose_filename, $!);
910-
911-
912-
if ($parsed_email{'From'}) {
913-
$sender = delete($parsed_email{'From'});
914-
}
915-
if ($parsed_email{'In-Reply-To'}) {
916-
$initial_in_reply_to = delete($parsed_email{'In-Reply-To'});
917-
}
918-
if ($parsed_email{'Reply-To'}) {
919-
$reply_to = delete($parsed_email{'Reply-To'});
920-
}
921-
if ($parsed_email{'Subject'}) {
922-
$initial_subject = delete($parsed_email{'Subject'});
923-
print $c2 "Subject: " .
924-
quote_subject($initial_subject, $compose_encoding) .
925-
"\n";
926-
}
927-
928-
if ($parsed_email{'MIME-Version'}) {
929-
print $c2 "MIME-Version: $parsed_email{'MIME-Version'}\n",
930-
"Content-Type: $parsed_email{'Content-Type'};\n",
931-
"Content-Transfer-Encoding: $parsed_email{'Content-Transfer-Encoding'}\n";
932-
delete($parsed_email{'MIME-Version'});
933-
delete($parsed_email{'Content-Type'});
934-
delete($parsed_email{'Content-Transfer-Encoding'});
935-
} elsif (file_has_nonascii($compose_filename)) {
936-
my $content_type = (delete($parsed_email{'Content-Type'}) or
937-
"text/plain; charset=$compose_encoding");
938-
print $c2 "MIME-Version: 1.0\n",
939-
"Content-Type: $content_type\n",
940-
"Content-Transfer-Encoding: 8bit\n";
941-
}
942-
# Preserve unknown headers
943-
foreach my $key (keys %parsed_email) {
944-
next if $key eq 'body';
945-
print $c2 "$key: $parsed_email{$key}";
946-
}
947-
948-
if ($parsed_email{'body'}) {
949-
print $c2 "\n$parsed_email{'body'}\n";
950-
delete($parsed_email{'body'});
951-
} else {
940+
if ($summary_empty) {
952941
print __("Summary email is empty, skipping it\n");
953942
$compose = -1;
954943
}
955-
956-
close $c2;
957-
958944
} elsif ($annotate) {
959945
do_edit(@files);
960946
}
@@ -1009,32 +995,6 @@ sub ask {
1009995
return;
1010996
}
1011997
1012-
sub parse_header_line {
1013-
my $lines = shift;
1014-
my $parsed_line = shift;
1015-
my $addr_pat = join "|", qw(To Cc Bcc);
1016-
1017-
foreach (split(/\n/, $lines)) {
1018-
if (/^($addr_pat):\s*(.+)$/i) {
1019-
$parsed_line->{$1} = [ parse_address_line($2) ];
1020-
} elsif (/^([^:]*):\s*(.+)\s*$/i) {
1021-
$parsed_line->{$1} = $2;
1022-
}
1023-
}
1024-
}
1025-
1026-
sub filter_body {
1027-
my $c = shift;
1028-
my $body = "";
1029-
while (my $body_line = <$c>) {
1030-
if ($body_line !~ m/^GIT:/) {
1031-
$body .= $body_line;
1032-
}
1033-
}
1034-
return $body;
1035-
}
1036-
1037-
1038998
my %broken_encoding;
1039999
10401000
sub file_declares_8bit_cte {

‎t/t9001-send-email.sh

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2505,4 +2505,39 @@ test_expect_success $PREREQ 'test forbidSendmailVariables behavior override' '
25052505
HEAD^
25062506
'
25072507

2508+
test_expect_success $PREREQ '--compose handles lowercase headers' '
2509+
write_script fake-editor <<-\EOF &&
2510+
sed "s/^From:.*/from: [email protected]/i" "$1" >"$1.tmp" &&
2511+
mv "$1.tmp" "$1"
2512+
EOF
2513+
clean_fake_sendmail &&
2514+
git send-email \
2515+
--compose \
2516+
--from="Example <[email protected]>" \
2517+
[email protected] \
2518+
--smtp-server="$(pwd)/fake.sendmail" \
2519+
HEAD^ &&
2520+
grep "From: [email protected]" msgtxt1
2521+
'
2522+
2523+
test_expect_success $PREREQ '--compose handles to headers' '
2524+
write_script fake-editor <<-\EOF &&
2525+
sed "s/^$/To: [email protected]\n/" <"$1" >"$1.tmp" &&
2526+
echo this is the body >>"$1.tmp" &&
2527+
mv "$1.tmp" "$1"
2528+
EOF
2529+
clean_fake_sendmail &&
2530+
GIT_SEND_EMAIL_NOTTY=1 \
2531+
git send-email \
2532+
--compose \
2533+
--from="Example <[email protected]>" \
2534+
[email protected] \
2535+
--smtp-server="$(pwd)/fake.sendmail" \
2536+
HEAD^ &&
2537+
# Ideally the "to" header we specified would be used,
2538+
# but the program explicitly warns that these are
2539+
# ignored. For now, just make sure we did not abort.
2540+
grep "To:" msgtxt1
2541+
'
2542+
25082543
test_done

0 commit comments

Comments
 (0)