From e0c7e2c3261fa49f9c99638fb2adf526e3833927 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 20 Oct 2023 06:09:01 -0400 Subject: [PATCH 1/3] doc/send-email: mention handling of "reply-to" with --compose The documentation for git-send-email lists the headers handled specially by --compose in a way that implies that this is the complete set of headers that are special. But one more was added by d11c943c78 (send-email: support separate Reply-To address, 2018-03-04) and never documented. Let's add it, and reword the documentation slightly to avoid having to specify the list of headers twice (as it is growing and will continue to do so as we add new features). If you read the code, you may notice that we also handle MIME-Version specially, in that we'll avoid over-writing user-provided MIME headers. I don't think this is worth mentioning, as it's what you'd expect to happen (as opposed to the other headers, which are picked up to be used in later emails). And certainly this feature existed when the documentation was expanded in 01d3861217 (git-send-email.txt: describe --compose better, 2009-03-16), and we chose not to mention it then. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- Documentation/git-send-email.txt | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt index 492a82323dab8e..021276329c63dd 100644 --- a/Documentation/git-send-email.txt +++ b/Documentation/git-send-email.txt @@ -68,11 +68,11 @@ This option may be specified multiple times. Invoke a text editor (see GIT_EDITOR in linkgit:git-var[1]) to edit an introductory message for the patch series. + -When `--compose` is used, git send-email will use the From, Subject, and -In-Reply-To headers specified in the message. If the body of the message -(what you type after the headers and a blank line) only contains blank -(or Git: prefixed) lines, the summary won't be sent, but From, Subject, -and In-Reply-To headers will be used unless they are removed. +When `--compose` is used, git send-email will use the From, Subject, +Reply-To, and In-Reply-To headers specified in the message. If the body +of the message (what you type after the headers and a blank line) only +contains blank (or Git: prefixed) lines, the summary won't be sent, but +the headers mentioned above will be used unless they are removed. + Missing From or In-Reply-To headers will be prompted for. + From 637e8944a13af5eae2dcaef99d4d84645f2b60ac Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 20 Oct 2023 06:13:10 -0400 Subject: [PATCH 2/3] Revert "send-email: extract email-parsing code into a subroutine" This reverts commit b6049542b97e7b135e0e82bf996084d461224d32. 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 15dc3b9161 (send-email: rename variable for clarity, 2018-03-04) and d11c943c78 (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 Signed-off-by: Junio C Hamano --- git-send-email.perl | 120 ++++++++++++++---------------------------- t/t9001-send-email.sh | 35 ++++++++++++ 2 files changed, 75 insertions(+), 80 deletions(-) diff --git a/git-send-email.perl b/git-send-email.perl index 897cea6564fb50..2adaa359386c3b 100755 --- a/git-send-email.perl +++ b/git-send-email.perl @@ -888,73 +888,59 @@ sub get_patch_subject { 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"; } - - 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); + 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_in_reply_to = $1; + next; + } elsif (/^Reply-To:\s*(.+)\s*$/i) { + $reply_to = $1; + } 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 $_; } close $c; + close $c2; - 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_in_reply_to = delete($parsed_email{'In-Reply-To'}); - } - if ($parsed_email{'Reply-To'}) { - $reply_to = delete($parsed_email{'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'}); - delete($parsed_email{'Content-Type'}); - delete($parsed_email{'Content-Transfer-Encoding'}); - } elsif (file_has_nonascii($compose_filename)) { - my $content_type = (delete($parsed_email{'Content-Type'}) or - "text/plain; charset=$compose_encoding"); - print $c2 "MIME-Version: 1.0\n", - "Content-Type: $content_type\n", - "Content-Transfer-Encoding: 8bit\n"; - } - # Preserve unknown headers - foreach my $key (keys %parsed_email) { - next if $key eq 'body'; - print $c2 "$key: $parsed_email{$key}"; - } - - if ($parsed_email{'body'}) { - print $c2 "\n$parsed_email{'body'}\n"; - delete($parsed_email{'body'}); - } else { + if ($summary_empty) { print __("Summary email is empty, skipping it\n"); $compose = -1; } - - close $c2; - } elsif ($annotate) { do_edit(@files); } @@ -1009,32 +995,6 @@ sub ask { return; } -sub parse_header_line { - my $lines = shift; - my $parsed_line = shift; - my $addr_pat = join "|", qw(To Cc Bcc); - - foreach (split(/\n/, $lines)) { - if (/^($addr_pat):\s*(.+)$/i) { - $parsed_line->{$1} = [ parse_address_line($2) ]; - } elsif (/^([^:]*):\s*(.+)\s*$/i) { - $parsed_line->{$1} = $2; - } - } -} - -sub filter_body { - my $c = shift; - my $body = ""; - while (my $body_line = <$c>) { - if ($body_line !~ m/^GIT:/) { - $body .= $body_line; - } - } - return $body; -} - - my %broken_encoding; sub file_declares_8bit_cte { diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh index a60b05ad3f09f0..c62f032056e29d 100755 --- a/t/t9001-send-email.sh +++ b/t/t9001-send-email.sh @@ -2505,4 +2505,39 @@ test_expect_success $PREREQ 'test forbidSendmailVariables behavior override' ' HEAD^ ' +test_expect_success $PREREQ '--compose handles lowercase headers' ' + write_script fake-editor <<-\EOF && + sed "s/^From:.*/from: edited-from@example.com/i" "$1" >"$1.tmp" && + mv "$1.tmp" "$1" + EOF + clean_fake_sendmail && + git send-email \ + --compose \ + --from="Example " \ + --to=nobody@example.com \ + --smtp-server="$(pwd)/fake.sendmail" \ + HEAD^ && + grep "From: edited-from@example.com" msgtxt1 +' + +test_expect_success $PREREQ '--compose handles to headers' ' + write_script fake-editor <<-\EOF && + sed "s/^$/To: edited-to@example.com\n/" <"$1" >"$1.tmp" && + echo this is the body >>"$1.tmp" && + mv "$1.tmp" "$1" + EOF + clean_fake_sendmail && + GIT_SEND_EMAIL_NOTTY=1 \ + git send-email \ + --compose \ + --from="Example " \ + --to=nobody@example.com \ + --smtp-server="$(pwd)/fake.sendmail" \ + HEAD^ && + # Ideally the "to" header we specified would be used, + # but the program explicitly warns that these are + # ignored. For now, just make sure we did not abort. + grep "To:" msgtxt1 +' + test_done From 3ec6167567d0e1e03a728a64efa9848310d172ab Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 20 Oct 2023 06:15:24 -0400 Subject: [PATCH 3/3] send-email: handle to/cc/bcc from --compose message If the user writes a message via --compose, send-email will pick up various headers like "From", "Subject", etc and use them for other patches as if they were specified on the command-line. But we don't handle "To", "Cc", or "Bcc" this way; we just tell the user "those aren't interpeted yet" and ignore them. But it seems like an obvious thing to want, especially as the same feature exists when the cover letter is generated separately by format-patch. There it is gated behind the --to-cover option, but I don't think we'd need the same control here; since we generate the --compose template ourselves based on the existing input, if the user leaves the lines unchanged then the behavior remains the same. So let's fill in the implementation; like those other headers we already handle, we just need to assign to the initial_* variables. The only difference in this case is that they are arrays, so we'll feed them through parse_address_line() to split them (just like we would when reading a single string via prompting). Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- Documentation/git-send-email.txt | 11 ++++++----- git-send-email.perl | 16 ++++++++++++++-- t/t9001-send-email.sh | 16 +++++++++++----- 3 files changed, 31 insertions(+), 12 deletions(-) diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt index 021276329c63dd..f4d7166275ab7c 100644 --- a/Documentation/git-send-email.txt +++ b/Documentation/git-send-email.txt @@ -68,11 +68,12 @@ This option may be specified multiple times. Invoke a text editor (see GIT_EDITOR in linkgit:git-var[1]) to edit an introductory message for the patch series. + -When `--compose` is used, git send-email will use the From, Subject, -Reply-To, and In-Reply-To headers specified in the message. If the body -of the message (what you type after the headers and a blank line) only -contains blank (or Git: prefixed) lines, the summary won't be sent, but -the headers mentioned above will be used unless they are removed. +When `--compose` is used, git send-email will use the From, To, Cc, Bcc, +Subject, Reply-To, and In-Reply-To headers specified in the message. If +the body of the message (what you type after the headers and a blank +line) only contains blank (or Git: prefixed) lines, the summary won't be +sent, but the headers mentioned above will be used unless they are +removed. + Missing From or In-Reply-To headers will be prompted for. + diff --git a/git-send-email.perl b/git-send-email.perl index 2adaa359386c3b..526f2dd712382e 100755 --- a/git-send-email.perl +++ b/git-send-email.perl @@ -861,6 +861,9 @@ sub get_patch_subject { my $tpl_subject = $initial_subject || ''; my $tpl_in_reply_to = $initial_in_reply_to || ''; my $tpl_reply_to = $reply_to || ''; + my $tpl_to = join(',', @initial_to); + my $tpl_cc = join(',', @initial_cc); + my $tpl_bcc = join(', ', @initial_bcc); print $c <"$1.tmp" && + sed "s/^To: .*/&, edited-to@example.com/" <"$1" >"$1.tmp" && echo this is the body >>"$1.tmp" && mv "$1.tmp" "$1" EOF @@ -2534,10 +2534,16 @@ test_expect_success $PREREQ '--compose handles to headers' ' --to=nobody@example.com \ --smtp-server="$(pwd)/fake.sendmail" \ HEAD^ && - # Ideally the "to" header we specified would be used, - # but the program explicitly warns that these are - # ignored. For now, just make sure we did not abort. - grep "To:" msgtxt1 + # Check both that the cover letter used our modified "to" line, + # but also that it was picked up for the patch. + q_to_tab >expect <<-\EOF && + To: nobody@example.com, + Qedited-to@example.com + EOF + grep -A1 "^To:" msgtxt1 >msgtxt1.to && + test_cmp expect msgtxt1.to && + grep -A1 "^To:" msgtxt2 >msgtxt2.to && + test_cmp expect msgtxt2.to ' test_done