* [PATCH 1/3] doc/send-email: mention handling of "reply-to" with --compose
2023-10-20 10:03 ` [PATCH 0/3] some send-email --compose fixes Jeff King
@ 2023-10-20 10:09 ` Jeff King
2023-10-20 10:13 ` [PATCH 2/3] Revert "send-email: extract email-parsing code into a subroutine" Jeff King
` (2 subsequent siblings)
3 siblings, 0 replies; 47+ messages in thread
From: Jeff King @ 2023-10-20 10:09 UTC (permalink / raw)
To: Michael Strawbridge; +Cc: Junio C Hamano, Bagas Sanjaya, Git Mailing List
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 <peff@peff.net>
---
Just something I noticed since a later commit touches the same list.
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 492a82323d..021276329c 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.
+
--
2.42.0.980.g8b5f6199be
^ permalink raw reply related [flat|nested] 47+ messages in thread* [PATCH 2/3] Revert "send-email: extract email-parsing code into a subroutine"
2023-10-20 10:03 ` [PATCH 0/3] some send-email --compose fixes Jeff King
2023-10-20 10:09 ` [PATCH 1/3] doc/send-email: mention handling of "reply-to" with --compose Jeff King
@ 2023-10-20 10:13 ` Jeff King
2023-10-20 10:45 ` Oswald Buddenhagen
2023-10-20 21:45 ` Junio C Hamano
2023-10-20 10:15 ` [PATCH 3/3] send-email: handle to/cc/bcc from --compose message Jeff King
2023-10-20 21:42 ` [PATCH 0/3] some send-email --compose fixes Junio C Hamano
3 siblings, 2 replies; 47+ messages in thread
From: Jeff King @ 2023-10-20 10:13 UTC (permalink / raw)
To: Michael Strawbridge; +Cc: Junio C Hamano, Bagas Sanjaya, Git Mailing List
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 <peff@peff.net>
---
I guess "readable" is up for debate here, but I find the inline handling
a lot easier to follow (and it's half as many lines; most of the
diffstat is the new tests).
But one thing that gives me pause is that the neither before or after
this patch do we handle continuation lines like:
Subject: this is the beginning
and this is more subject
And it would probably be a lot easier to add when storing the headers in
a hash (it's not impossible to do it the other way, but you basically
have to delay processing each line with a small state machine).
So another option is to just fix the individual bugs separately.
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 288ea1ae80..bbda2a931b 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 263db3ad17..9644ff5793 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 <from@example.com>" \
+ --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 <from@example.com>" \
+ --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
--
2.42.0.980.g8b5f6199be
^ permalink raw reply related [flat|nested] 47+ messages in thread* Re: [PATCH 2/3] Revert "send-email: extract email-parsing code into a subroutine"
2023-10-20 10:13 ` [PATCH 2/3] Revert "send-email: extract email-parsing code into a subroutine" Jeff King
@ 2023-10-20 10:45 ` Oswald Buddenhagen
2023-10-23 18:40 ` Jeff King
2023-10-20 21:45 ` Junio C Hamano
1 sibling, 1 reply; 47+ messages in thread
From: Oswald Buddenhagen @ 2023-10-20 10:45 UTC (permalink / raw)
To: Jeff King
Cc: Michael Strawbridge, Junio C Hamano, Bagas Sanjaya,
Git Mailing List
On Fri, Oct 20, 2023 at 06:13:10AM -0400, Jeff King wrote:
>But one thing that gives me pause is that the neither before or after
>this patch do we handle continuation lines like:
>
> Subject: this is the beginning
> and this is more subject
>
>And it would probably be a lot easier to add when storing the headers in
>a hash (it's not impossible to do it the other way, but you basically
>have to delay processing each line with a small state machine).
>
that seems like a rather significant point, doesn't it?
>So another option is to just fix the individual bugs separately.
>
... so that seems preferable to me, given that the necessary fixes seem
rather trivial.
> I guess "readable" is up for debate here, but I find the inline handling
> a lot easier to follow
>
any particular reason for that?
> (and it's half as many lines; most of the diffstat is the new tests).
>- if ($parsed_email{'From'}) {
>- $sender = delete($parsed_email{'From'});
>- }
this verbosity could be cut down somewhat using just
$sender = delete($parsed_email{'From'});
and if the value can be pre-set and needs to be preserved,
$sender = delete($parsed_email{'From'}) // $sender;
but this seems kind of counter-productive legibility-wise.
regards
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [PATCH 2/3] Revert "send-email: extract email-parsing code into a subroutine"
2023-10-20 10:45 ` Oswald Buddenhagen
@ 2023-10-23 18:40 ` Jeff King
2023-10-23 19:50 ` Oswald Buddenhagen
0 siblings, 1 reply; 47+ messages in thread
From: Jeff King @ 2023-10-23 18:40 UTC (permalink / raw)
To: Oswald Buddenhagen
Cc: Michael Strawbridge, Junio C Hamano, Bagas Sanjaya,
Git Mailing List
On Fri, Oct 20, 2023 at 12:45:43PM +0200, Oswald Buddenhagen wrote:
> On Fri, Oct 20, 2023 at 06:13:10AM -0400, Jeff King wrote:
> > But one thing that gives me pause is that the neither before or after
> > this patch do we handle continuation lines like:
> >
> > Subject: this is the beginning
> > and this is more subject
> >
> > And it would probably be a lot easier to add when storing the headers in
> > a hash (it's not impossible to do it the other way, but you basically
> > have to delay processing each line with a small state machine).
> >
> that seems like a rather significant point, doesn't it?
Maybe. It depends on whether anybody is interested in adding
continuation support. Nobody has in the previous 18 years, and nobody
has asked for it.
> > So another option is to just fix the individual bugs separately.
> >
> ... so that seems preferable to me, given that the necessary fixes seem
> rather trivial.
They're not too bad. Probably:
1. lc() the keys we put into the hash
2. match to/cc/bcc and dereference their arrays
3. maybe handle 'body' separately from headers to avoid confusion
But there may be other similar bugs lurking. One I didn't mention: the
hash-based version randomly reorders headers!
> > I guess "readable" is up for debate here, but I find the inline handling
> > a lot easier to follow
> >
> any particular reason for that?
For the reasons I gave in the commit message: namely that the matching
and logic is in one place and doesn't need to be duplicated (e.g., the
special handling of to/cc/bcc, which caused a bug here).
> > (and it's half as many lines; most of the diffstat is the new tests).
>
> > - if ($parsed_email{'From'}) {
> > - $sender = delete($parsed_email{'From'});
> > - }
>
> this verbosity could be cut down somewhat using just
>
> $sender = delete($parsed_email{'From'});
>
> and if the value can be pre-set and needs to be preserved,
>
> $sender = delete($parsed_email{'From'}) // $sender;
>
> but this seems kind of counter-productive legibility-wise.
We do need to avoid overwriting the pre-set value. The "//" one would
work, but we support perl versions old enough that they don't have it.
-Peff
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [PATCH 2/3] Revert "send-email: extract email-parsing code into a subroutine"
2023-10-23 18:40 ` Jeff King
@ 2023-10-23 19:50 ` Oswald Buddenhagen
2023-10-25 6:11 ` Jeff King
0 siblings, 1 reply; 47+ messages in thread
From: Oswald Buddenhagen @ 2023-10-23 19:50 UTC (permalink / raw)
To: Jeff King
Cc: Michael Strawbridge, Junio C Hamano, Bagas Sanjaya,
Git Mailing List
On Mon, Oct 23, 2023 at 02:40:10PM -0400, Jeff King wrote:
>On Fri, Oct 20, 2023 at 12:45:43PM +0200, Oswald Buddenhagen wrote:
>> that seems like a rather significant point, doesn't it?
>
>Maybe. It depends on whether anybody is interested in adding
>continuation support. Nobody has in the previous 18 years, and nobody
>has asked for it.
>
dunno, it seems like a bug to me. so if i cared at all about this
functionality, i'd fix it just because. so at least it doesn't seem nice
to make it harder for a potential volunteer.
>> > So another option is to just fix the individual bugs separately.
>> >
>> ... so that seems preferable to me, given that the necessary fixes
>> seem
>> rather trivial.
>
>They're not too bad. Probably:
>
> 1. lc() the keys we put into the hash
>
> 2. match to/cc/bcc and dereference their arrays
>
> 3. maybe handle 'body' separately from headers to avoid confusion
>
with the header keys lowercased, one could simply use BODY as the key
and be done with it.
>But there may be other similar bugs lurking.
>One I didn't mention: the
>hash-based version randomly reorders headers!
>
hmm, yeah, that would mean using Tie::IxHash if one wanted to do it
elegantly, at the cost of depending on another non-core module.
also, it means that another hash with non-lowercased versions of the
keys would have to be kept.
ok, that's stupid. it would be easier to just keep an additional array
of the original keys for iteration, and check the hash before emitting
them.
>> > I guess "readable" is up for debate here, but I find the inline handling
>> > a lot easier to follow
>> >
>> any particular reason for that?
>
>For the reasons I gave in the commit message: namely that the matching
>and logic is in one place and doesn't need to be duplicated (e.g., the
>special handling of to/cc/bcc, which caused a bug here).
>
from what i can see, there isn't really anything to "match", apart from
agreeing on the data structure (which the code partially failed to do,
but that's trivial enough). and layering/abstracting things is usually
considered a good thing, unless the cost/benefit ratio is completely
backwards.
>The "//" one would
>work, but we support perl versions old enough that they don't have it.
>
according to my grepping, that ship has sailed.
also, why _would_ you support such ancient perl versions? that makes
even less sense to me than supporting ancient c compilers.
regards
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 2/3] Revert "send-email: extract email-parsing code into a subroutine"
2023-10-23 19:50 ` Oswald Buddenhagen
@ 2023-10-25 6:11 ` Jeff King
2023-10-25 9:23 ` Oswald Buddenhagen
0 siblings, 1 reply; 47+ messages in thread
From: Jeff King @ 2023-10-25 6:11 UTC (permalink / raw)
To: Oswald Buddenhagen
Cc: Michael Strawbridge, Junio C Hamano, Bagas Sanjaya,
Git Mailing List
On Mon, Oct 23, 2023 at 09:50:54PM +0200, Oswald Buddenhagen wrote:
> > The "//" one would
> > work, but we support perl versions old enough that they don't have it.
> >
> according to my grepping, that ship has sailed.
> also, why _would_ you support such ancient perl versions? that makes even
> less sense to me than supporting ancient c compilers.
It may be reasonable to bump the default perl version for the script.
But that would require somebody digging into what tends to ship these
days (which can be sometimes be surprising; witness macos using old
versions of bash due to license issues), and then updating the "use
5.008" in the script.
The "//" operator was added in perl 5.10. I'm not sure what you found
that makes you think the ship has sailed. The only hits for "//" I see
look like the end of substitution regexes ("s/foo//" and similar). But
if we are not consistent with the "use" claim, that is worth fixing.
-Peff
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [PATCH 2/3] Revert "send-email: extract email-parsing code into a subroutine"
2023-10-25 6:11 ` Jeff King
@ 2023-10-25 9:23 ` Oswald Buddenhagen
2023-10-27 22:31 ` Junio C Hamano
2023-10-30 9:13 ` Jeff King
0 siblings, 2 replies; 47+ messages in thread
From: Oswald Buddenhagen @ 2023-10-25 9:23 UTC (permalink / raw)
To: Jeff King
Cc: Michael Strawbridge, Junio C Hamano, Bagas Sanjaya,
Git Mailing List
On Wed, Oct 25, 2023 at 02:11:20AM -0400, Jeff King wrote:
>The "//" operator was added in perl 5.10. I'm not sure what you found
>that makes you think the ship has sailed. The only hits for "//" I see
>look like the end of substitution regexes ("s/foo//" and similar).
>
grep with spaces around the operator, then you can see the instance in
git-credential-netrc.perl easily.
regards
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [PATCH 2/3] Revert "send-email: extract email-parsing code into a subroutine"
2023-10-25 9:23 ` Oswald Buddenhagen
@ 2023-10-27 22:31 ` Junio C Hamano
2023-10-30 9:13 ` Jeff King
1 sibling, 0 replies; 47+ messages in thread
From: Junio C Hamano @ 2023-10-27 22:31 UTC (permalink / raw)
To: Oswald Buddenhagen
Cc: Jeff King, Michael Strawbridge, Bagas Sanjaya, Git Mailing List
Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:
> On Wed, Oct 25, 2023 at 02:11:20AM -0400, Jeff King wrote:
>>The "//" operator was added in perl 5.10. I'm not sure what you found
>>that makes you think the ship has sailed. The only hits for "//" I see
>>look like the end of substitution regexes ("s/foo//" and similar).
>>
> grep with spaces around the operator, then you can see the instance in
> git-credential-netrc.perl easily.
Good find, but given the relative prevalence in use between netrc
helper and send-email, my conclusion is rather opposite. It seems
to indicate that avoiding "//" would still be prudent if the only
tool we can find find broken on 5.008 is the netrc helper.
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [PATCH 2/3] Revert "send-email: extract email-parsing code into a subroutine"
2023-10-25 9:23 ` Oswald Buddenhagen
2023-10-27 22:31 ` Junio C Hamano
@ 2023-10-30 9:13 ` Jeff King
1 sibling, 0 replies; 47+ messages in thread
From: Jeff King @ 2023-10-30 9:13 UTC (permalink / raw)
To: Oswald Buddenhagen
Cc: Michael Strawbridge, Junio C Hamano, Bagas Sanjaya,
Git Mailing List
On Wed, Oct 25, 2023 at 11:23:01AM +0200, Oswald Buddenhagen wrote:
> On Wed, Oct 25, 2023 at 02:11:20AM -0400, Jeff King wrote:
> > The "//" operator was added in perl 5.10. I'm not sure what you found
> > that makes you think the ship has sailed. The only hits for "//" I see
> > look like the end of substitution regexes ("s/foo//" and similar).
> >
> grep with spaces around the operator, then you can see the instance in
> git-credential-netrc.perl easily.
Ah, yeah, there is one instance there. That script does not have a "use"
marker, though, and we do not necessarily need or want to be as strict
with contrib/ scripts, which are quite optional compared to core
functionality like send-email.
That said, I do suspect that requiring 5.10 or later would not be too
burdensome these days. If we want to do so, then the first step would be
updating the text in INSTALL, along with the "use" directives in most
files. Probably d48b284183 (perl: bump the required Perl version to 5.8
from 5.6.[21], 2010-09-24) could serve as a template.
-Peff
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 2/3] Revert "send-email: extract email-parsing code into a subroutine"
2023-10-20 10:13 ` [PATCH 2/3] Revert "send-email: extract email-parsing code into a subroutine" Jeff King
2023-10-20 10:45 ` Oswald Buddenhagen
@ 2023-10-20 21:45 ` Junio C Hamano
2023-10-23 18:47 ` Jeff King
1 sibling, 1 reply; 47+ messages in thread
From: Junio C Hamano @ 2023-10-20 21:45 UTC (permalink / raw)
To: Jeff King; +Cc: Michael Strawbridge, Bagas Sanjaya, Git Mailing List
Jeff King <peff@peff.net> writes:
> - the handling for to/cc/bcc is totally broken.
It is good to see another evidence that "--compose" is probably not
as often as used as we thought. With enough bugs discovered,
perhaps someday we can declare "it cannot be that the feature is
used in the wild, without anybody getting hit by these bugs---let's
deprecate and eventually remove it" ;-)
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 2/3] Revert "send-email: extract email-parsing code into a subroutine"
2023-10-20 21:45 ` Junio C Hamano
@ 2023-10-23 18:47 ` Jeff King
0 siblings, 0 replies; 47+ messages in thread
From: Jeff King @ 2023-10-23 18:47 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Michael Strawbridge, Bagas Sanjaya, Git Mailing List
On Fri, Oct 20, 2023 at 02:45:30PM -0700, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > - the handling for to/cc/bcc is totally broken.
>
> It is good to see another evidence that "--compose" is probably not
> as often as used as we thought. With enough bugs discovered,
> perhaps someday we can declare "it cannot be that the feature is
> used in the wild, without anybody getting hit by these bugs---let's
> deprecate and eventually remove it" ;-)
I'm not sure if that is evidence or not. The to/cc/bcc feature was just
never implemented. The commit from 2017 made it more broken than saying
"not yet implemented", but that may only be an indication that nobody
wants it or tried to use it.
I dunno. As I noted, the same feature exists when reading the
cover-letter from a set of format-patch files. And of course it is
implemented using totally separate code (in pre_process_file). One
possible cleanup would be to unify those two, but I'm sure there would
be behavior changes. Some of them perhaps good (e.g., it looks like
pre_process_file is more careful about rfc2047 handling) and some of
them I'm not so sure of (e.g., support for --header-cmd in the --compose
letter).
I think an interested person could champion such changes, but I am not
that interested in send-email (I don't use it, and some of its code is
pretty ancient and gross). My goal was to fix the bug I saw with minimal
regression (I waffled even on my patch 2).
-Peff
^ permalink raw reply [flat|nested] 47+ messages in thread
* [PATCH 3/3] send-email: handle to/cc/bcc from --compose message
2023-10-20 10:03 ` [PATCH 0/3] some send-email --compose fixes Jeff King
2023-10-20 10:09 ` [PATCH 1/3] doc/send-email: mention handling of "reply-to" with --compose Jeff King
2023-10-20 10:13 ` [PATCH 2/3] Revert "send-email: extract email-parsing code into a subroutine" Jeff King
@ 2023-10-20 10:15 ` Jeff King
2023-10-20 17:30 ` Eric Sunshine
2023-10-20 21:42 ` [PATCH 0/3] some send-email --compose fixes Junio C Hamano
3 siblings, 1 reply; 47+ messages in thread
From: Jeff King @ 2023-10-20 10:15 UTC (permalink / raw)
To: Michael Strawbridge; +Cc: Junio C Hamano, Bagas Sanjaya, Git Mailing List
If the user writes a message via --compose, send-email will pick up
varius 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 <peff@peff.net>
---
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 021276329c..f4d7166275 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 bbda2a931b..9e21b0b3f4 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 <<EOT1, Git::prefix_lines("GIT: ", __(<<EOT2)), <<EOT3;
From $tpl_sender # This line is ignored.
@@ -872,6 +875,9 @@ sub get_patch_subject {
Clear the body content if you don't wish to send a summary.
EOT2
From: $tpl_sender
+To: $tpl_to
+Cc: $tpl_cc
+Bcc: $tpl_bcc
Reply-To: $tpl_reply_to
Subject: $tpl_subject
In-Reply-To: $tpl_in_reply_to
@@ -928,8 +934,14 @@ sub get_patch_subject {
} 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");
+ } elsif (/^To:\s*(.+)\s*$/i) {
+ @initial_to = parse_address_line($1);
+ next;
+ } elsif (/^Cc:\s*(.+)\s*$/i) {
+ @initial_cc = parse_address_line($1);
+ next;
+ } elsif (/^Bcc:/i) {
+ @initial_bcc = parse_address_line($1);
next;
}
print $c2 $_;
diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index 9644ff5793..2e8e8137fb 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -2522,7 +2522,7 @@ test_expect_success $PREREQ '--compose handles lowercase headers' '
test_expect_success $PREREQ '--compose handles to headers' '
write_script fake-editor <<-\EOF &&
- sed "s/^$/To: edited-to@example.com\n/" <"$1" >"$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
--
2.42.0.980.g8b5f6199be
^ permalink raw reply related [flat|nested] 47+ messages in thread* Re: [PATCH 3/3] send-email: handle to/cc/bcc from --compose message
2023-10-20 10:15 ` [PATCH 3/3] send-email: handle to/cc/bcc from --compose message Jeff King
@ 2023-10-20 17:30 ` Eric Sunshine
0 siblings, 0 replies; 47+ messages in thread
From: Eric Sunshine @ 2023-10-20 17:30 UTC (permalink / raw)
To: Jeff King
Cc: Michael Strawbridge, Junio C Hamano, Bagas Sanjaya,
Git Mailing List
On Fri, Oct 20, 2023 at 6:15 AM Jeff King <peff@peff.net> wrote:
> If the user writes a message via --compose, send-email will pick up
> varius headers like "From", "Subject", etc and use them for other
s/varius/various/
> 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 <peff@peff.net>
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 0/3] some send-email --compose fixes
2023-10-20 10:03 ` [PATCH 0/3] some send-email --compose fixes Jeff King
` (2 preceding siblings ...)
2023-10-20 10:15 ` [PATCH 3/3] send-email: handle to/cc/bcc from --compose message Jeff King
@ 2023-10-20 21:42 ` Junio C Hamano
2023-10-23 18:51 ` Jeff King
3 siblings, 1 reply; 47+ messages in thread
From: Junio C Hamano @ 2023-10-20 21:42 UTC (permalink / raw)
To: Jeff King; +Cc: Michael Strawbridge, Bagas Sanjaya, Git Mailing List
Jeff King <peff@peff.net> writes:
> [culling the rather large cc, as we moving off the original topic]
>
> On Fri, Oct 20, 2023 at 03:14:03AM -0400, Jeff King wrote:
>
>> and there's your perl array ref (from the square brackets, which are
>> necessary because we're sticking it in a hash value). But even before
>> your patch, this seems to end up as garbage. The code which reads
>> $parsed_line does not dereference the array.
>>
>> The patch to fix it is only a few lines (well, more than that with some
>> light editorializing in the comments):
>
> So here's the fix in a cleaned up form, guided by my own comments from
> earlier. ;) I think this is actually all orthogonal to the patch you are
> working on, so yours could either go on top or just be applied
> separately.
>
> [1/3]: doc/send-email: mention handling of "reply-to" with --compose
> [2/3]: Revert "send-email: extract email-parsing code into a subroutine"
> [3/3]: send-email: handle to/cc/bcc from --compose message
Nice.
With the approach suggested to move the validation down to where the
necessary addresses are already all defined, Michael observed "whoa,
why am I getting stringified array ref?". If that is the only issue
in the approach, queuing these three patches first and then have
Michael's fix on top of them sounds like the cleanest thing to do.
Will queue on top of v2.42.0 to help those who may want to backport
these to the maintenance track.
Thanks.
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [PATCH 0/3] some send-email --compose fixes
2023-10-20 21:42 ` [PATCH 0/3] some send-email --compose fixes Junio C Hamano
@ 2023-10-23 18:51 ` Jeff King
2023-10-24 20:12 ` Michael Strawbridge
0 siblings, 1 reply; 47+ messages in thread
From: Jeff King @ 2023-10-23 18:51 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Michael Strawbridge, Bagas Sanjaya, Git Mailing List
On Fri, Oct 20, 2023 at 02:42:13PM -0700, Junio C Hamano wrote:
> > So here's the fix in a cleaned up form, guided by my own comments from
> > earlier. ;) I think this is actually all orthogonal to the patch you are
> > working on, so yours could either go on top or just be applied
> > separately.
> >
> > [1/3]: doc/send-email: mention handling of "reply-to" with --compose
> > [2/3]: Revert "send-email: extract email-parsing code into a subroutine"
> > [3/3]: send-email: handle to/cc/bcc from --compose message
>
> Nice.
>
> With the approach suggested to move the validation down to where the
> necessary addresses are already all defined, Michael observed "whoa,
> why am I getting stringified array ref?". If that is the only issue
> in the approach, queuing these three patches first and then have
> Michael's fix on top of them sounds like the cleanest thing to do.
I don't think it is even an issue in Michael's approach. I'd have to see
his patch and how he tested it to be sure, but I suspect he was simply
being extra careful to test nearby behavior and stumbled upon the
ARRAY() bug. But the bug was there long before either of his patches.
> Will queue on top of v2.42.0 to help those who may want to backport
> these to the maintenance track.
So I think you could take my series on top of master (or 2.42.0), and
eventually target 'master'. The bug it fixes is from 2017, so not
urgent. The reading of "to" headers is a new feature.
But the fix to move the validation around should probably go directly
onto a8022c5f7b (send-email: expose header information to
git-send-email's sendemail-validate hook, 2023-04-19) for use on maint.
I guess maybe it is not that urgent anymore, as that regression is in
v2.41, and we would not release anything along that maint track anymore,
though.
-Peff
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH 0/3] some send-email --compose fixes
2023-10-23 18:51 ` Jeff King
@ 2023-10-24 20:12 ` Michael Strawbridge
2023-10-24 20:19 ` [PATCH] send-email: move validation code below process_address_list Michael Strawbridge
0 siblings, 1 reply; 47+ messages in thread
From: Michael Strawbridge @ 2023-10-24 20:12 UTC (permalink / raw)
To: Jeff King, Junio C Hamano; +Cc: Bagas Sanjaya, Git Mailing List
On 10/23/23 14:51, Jeff King wrote:
> On Fri, Oct 20, 2023 at 02:42:13PM -0700, Junio C Hamano wrote:
>
>>> So here's the fix in a cleaned up form, guided by my own comments from
>>> earlier. ;) I think this is actually all orthogonal to the patch you are
>>> working on, so yours could either go on top or just be applied
>>> separately.
>>>
>>> [1/3]: doc/send-email: mention handling of "reply-to" with --compose
>>> [2/3]: Revert "send-email: extract email-parsing code into a subroutine"
>>> [3/3]: send-email: handle to/cc/bcc from --compose message
>>
>> Nice.
>>
>> With the approach suggested to move the validation down to where the
>> necessary addresses are already all defined, Michael observed "whoa,
>> why am I getting stringified array ref?". If that is the only issue
>> in the approach, queuing these three patches first and then have
>> Michael's fix on top of them sounds like the cleanest thing to do.
Patch coming soon.
>
> I don't think it is even an issue in Michael's approach. I'd have to see
> his patch and how he tested it to be sure, but I suspect he was simply
> being extra careful to test nearby behavior and stumbled upon the
> ARRAY() bug. But the bug was there long before either of his patches.
>
Thank you for your patches Peff! I think it fixes the issue I was seeing.
I was trying to be extra careful with my testing. I had missed testing
--compose and also the multiple --to/cc/bcc examples before.
>> Will queue on top of v2.42.0 to help those who may want to backport
>> these to the maintenance track.
>
> So I think you could take my series on top of master (or 2.42.0), and
> eventually target 'master'. The bug it fixes is from 2017, so not
> urgent. The reading of "to" headers is a new feature.
>
> But the fix to move the validation around should probably go directly
> onto a8022c5f7b (send-email: expose header information to
> git-send-email's sendemail-validate hook, 2023-04-19) for use on maint.
> I guess maybe it is not that urgent anymore, as that regression is in
> v2.41, and we would not release anything along that maint track anymore,
> though.
>
> -Peff
^ permalink raw reply [flat|nested] 47+ messages in thread
* [PATCH] send-email: move validation code below process_address_list
2023-10-24 20:12 ` Michael Strawbridge
@ 2023-10-24 20:19 ` Michael Strawbridge
2023-10-24 21:55 ` Junio C Hamano
` (2 more replies)
0 siblings, 3 replies; 47+ messages in thread
From: Michael Strawbridge @ 2023-10-24 20:19 UTC (permalink / raw)
To: Jeff King, Junio C Hamano; +Cc: Bagas Sanjaya, Git Mailing List
From 09ea51d63cebdf9ff0c073ef86e21b4b09c268e5 Mon Sep 17 00:00:00 2001
From: Michael Strawbridge <michael.strawbridge@amd.com>
Date: Wed, 11 Oct 2023 16:13:13 -0400
Subject: [PATCH] send-email: move validation code below process_address_list
Move validation logic below processing of email address lists so that
email validation gets the proper email addresses.
This fixes email address validation errors when the optional
perl module Email::Valid is installed and multiple addresses are passed
in on a single to/cc argument like --to=foo@example.com,bar@example.com.
Reported-by: Bagas Sanjaya <bagasdotme@gmail.com>
Signed-off-by: Michael Strawbridge <michael.strawbridge@amd.com>
---
git-send-email.perl | 48 ++++++++++++++++++++++-----------------------
1 file changed, 24 insertions(+), 24 deletions(-)
diff --git a/git-send-email.perl b/git-send-email.perl
index 288ea1ae80..a898dbc76e 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -799,30 +799,6 @@ sub is_format_patch_arg {
$time = time - scalar $#files;
-if ($validate) {
- # FIFOs can only be read once, exclude them from validation.
- my @real_files = ();
- foreach my $f (@files) {
- unless (-p $f) {
- push(@real_files, $f);
- }
- }
-
- # Run the loop once again to avoid gaps in the counter due to FIFO
- # arguments provided by the user.
- my $num = 1;
- my $num_files = scalar @real_files;
- $ENV{GIT_SENDEMAIL_FILE_TOTAL} = "$num_files";
- foreach my $r (@real_files) {
- $ENV{GIT_SENDEMAIL_FILE_COUNTER} = "$num";
- pre_process_file($r, 1);
- validate_patch($r, $target_xfer_encoding);
- $num += 1;
- }
- delete $ENV{GIT_SENDEMAIL_FILE_COUNTER};
- delete $ENV{GIT_SENDEMAIL_FILE_TOTAL};
-}
-
@files = handle_backup_files(@files);
if (@files) {
@@ -2023,6 +1999,30 @@ sub process_file {
return 1;
}
+if ($validate) {
+ # FIFOs can only be read once, exclude them from validation.
+ my @real_files = ();
+ foreach my $f (@files) {
+ unless (-p $f) {
+ push(@real_files, $f);
+ }
+ }
+
+ # Run the loop once again to avoid gaps in the counter due to FIFO
+ # arguments provided by the user.
+ my $num = 1;
+ my $num_files = scalar @real_files;
+ $ENV{GIT_SENDEMAIL_FILE_TOTAL} = "$num_files";
+ foreach my $r (@real_files) {
+ $ENV{GIT_SENDEMAIL_FILE_COUNTER} = "$num";
+ pre_process_file($r, 1);
+ validate_patch($r, $target_xfer_encoding);
+ $num += 1;
+ }
+ delete $ENV{GIT_SENDEMAIL_FILE_COUNTER};
+ delete $ENV{GIT_SENDEMAIL_FILE_TOTAL};
+}
+
foreach my $t (@files) {
while (!process_file($t)) {
# user edited the file
--
2.42.0
^ permalink raw reply related [flat|nested] 47+ messages in thread* Re: [PATCH] send-email: move validation code below process_address_list
2023-10-24 20:19 ` [PATCH] send-email: move validation code below process_address_list Michael Strawbridge
@ 2023-10-24 21:55 ` Junio C Hamano
2023-10-24 22:03 ` Junio C Hamano
2023-10-25 6:50 ` [PATCH] " Jeff King
2023-10-25 7:43 ` Uwe Kleine-König
2 siblings, 1 reply; 47+ messages in thread
From: Junio C Hamano @ 2023-10-24 21:55 UTC (permalink / raw)
To: Michael Strawbridge; +Cc: Jeff King, Bagas Sanjaya, Git Mailing List
Michael Strawbridge <michael.strawbridge@amd.com> writes:
> Subject: [PATCH] send-email: move validation code below process_address_list
>
> Move validation logic below processing of email address lists so that
> email validation gets the proper email addresses.
Hmph, without this patch, the tip of 'seen' passes t9001 on my box,
but with it, it claims that it failed 58, 87, and 89.
Here is how #58 fails (the last part of "cd t && sh t9001-*.sh -i -v").
expecting success of 9001.58 'In-Reply-To without --chain-reply-to':
clean_fake_sendmail &&
echo "<unique-message-id@example.com>" >expect &&
git send-email \
--from="Example <nobody@example.com>" \
--to=nobody@example.com \
--no-chain-reply-to \
--in-reply-to="$(cat expect)" \
--smtp-server="$(pwd)/fake.sendmail" \
$patches $patches $patches \
2>errors &&
# The first message is a reply to --in-reply-to
sed -n -e "s/^In-Reply-To: *\(.*\)/\1/p" msgtxt1 >actual &&
test_cmp expect actual &&
# Second and subsequent messages are replies to the first one
sed -n -e "s/^Message-ID: *\(.*\)/\1/p" msgtxt1 >expect &&
sed -n -e "s/^In-Reply-To: *\(.*\)/\1/p" msgtxt2 >actual &&
test_cmp expect actual &&
sed -n -e "s/^In-Reply-To: *\(.*\)/\1/p" msgtxt3 >actual &&
test_cmp expect actual
0001-Second.patch
0001-Second.patch
0001-Second.patch
(mbox) Adding cc: A <author@example.com> from line 'From: A <author@example.com>'
(mbox) Adding cc: One <one@example.com> from line 'Cc: One <one@example.com>, two@example.com'
(mbox) Adding cc: two@example.com from line 'Cc: One <one@example.com>, two@example.com'
(body) Adding cc: C O Mitter <committer@example.com> from line 'Signed-off-by: C O Mitter <committer@example.com>'
OK. Log says:
Sendmail: /usr/local/google/home/jch/w/git.git/t/trash directory.t9001-send-email/fake.sendmail -i nobody@example.com author@example.com one@example.com two@example.com committer@example.com
From: Example <nobody@example.com>
To: nobody@example.com
Cc: A <author@example.com>,
One <one@example.com>,
two@example.com,
C O Mitter <committer@example.com>
Subject: [PATCH 1/1] Second.
Date: Tue, 24 Oct 2023 21:52:27 +0000
Message-ID: <20231024215229.1787922-1-nobody@example.com>
X-Mailer: git-send-email 2.42.0-705-g1a1f985ecc
In-Reply-To: <unique-message-id@example.com>
References: <unique-message-id@example.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Result: OK
(mbox) Adding cc: A <author@example.com> from line 'From: A <author@example.com>'
(mbox) Adding cc: One <one@example.com> from line 'Cc: One <one@example.com>, two@example.com'
(mbox) Adding cc: two@example.com from line 'Cc: One <one@example.com>, two@example.com'
(body) Adding cc: C O Mitter <committer@example.com> from line 'Signed-off-by: C O Mitter <committer@example.com>'
OK. Log says:
Sendmail: /usr/local/google/home/jch/w/git.git/t/trash directory.t9001-send-email/fake.sendmail -i nobody@example.com author@example.com one@example.com two@example.com committer@example.com
From: Example <nobody@example.com>
To: nobody@example.com
Cc: A <author@example.com>,
One <one@example.com>,
two@example.com,
C O Mitter <committer@example.com>
Subject: [PATCH 1/1] Second.
Date: Tue, 24 Oct 2023 21:52:28 +0000
Message-ID: <20231024215229.1787922-2-nobody@example.com>
X-Mailer: git-send-email 2.42.0-705-g1a1f985ecc
In-Reply-To: <unique-message-id@example.com>
References: <unique-message-id@example.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Result: OK
(mbox) Adding cc: A <author@example.com> from line 'From: A <author@example.com>'
(mbox) Adding cc: One <one@example.com> from line 'Cc: One <one@example.com>, two@example.com'
(mbox) Adding cc: two@example.com from line 'Cc: One <one@example.com>, two@example.com'
(body) Adding cc: C O Mitter <committer@example.com> from line 'Signed-off-by: C O Mitter <committer@example.com>'
OK. Log says:
Sendmail: /usr/local/google/home/jch/w/git.git/t/trash directory.t9001-send-email/fake.sendmail -i nobody@example.com author@example.com one@example.com two@example.com committer@example.com
From: Example <nobody@example.com>
To: nobody@example.com
Cc: A <author@example.com>,
One <one@example.com>,
two@example.com,
C O Mitter <committer@example.com>
Subject: [PATCH 1/1] Second.
Date: Tue, 24 Oct 2023 21:52:29 +0000
Message-ID: <20231024215229.1787922-3-nobody@example.com>
X-Mailer: git-send-email 2.42.0-705-g1a1f985ecc
In-Reply-To: <unique-message-id@example.com>
References: <unique-message-id@example.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Result: OK
--- expect 2023-10-24 21:52:29.115044899 +0000
+++ actual 2023-10-24 21:52:29.119045306 +0000
@@ -1 +1 @@
-<20231024215229.1787922-1-nobody@example.com>
+<unique-message-id@example.com>
not ok 58 - In-Reply-To without --chain-reply-to
#
# clean_fake_sendmail &&
# echo "<unique-message-id@example.com>" >expect &&
# git send-email \
# --from="Example <nobody@example.com>" \
# --to=nobody@example.com \
# --no-chain-reply-to \
# --in-reply-to="$(cat expect)" \
# --smtp-server="$(pwd)/fake.sendmail" \
# $patches $patches $patches \
# 2>errors &&
# # The first message is a reply to --in-reply-to
# sed -n -e "s/^In-Reply-To: *\(.*\)/\1/p" msgtxt1 >actual &&
# test_cmp expect actual &&
# # Second and subsequent messages are replies to the first one
# sed -n -e "s/^Message-ID: *\(.*\)/\1/p" msgtxt1 >expect &&
# sed -n -e "s/^In-Reply-To: *\(.*\)/\1/p" msgtxt2 >actual &&
# test_cmp expect actual &&
# sed -n -e "s/^In-Reply-To: *\(.*\)/\1/p" msgtxt3 >actual &&
# test_cmp expect actual
#
1..58
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [PATCH] send-email: move validation code below process_address_list
2023-10-24 21:55 ` Junio C Hamano
@ 2023-10-24 22:03 ` Junio C Hamano
2023-10-25 18:48 ` Michael Strawbridge
2023-10-25 18:51 ` [PATCH v2] " Michael Strawbridge
0 siblings, 2 replies; 47+ messages in thread
From: Junio C Hamano @ 2023-10-24 22:03 UTC (permalink / raw)
To: Michael Strawbridge; +Cc: Jeff King, Bagas Sanjaya, Git Mailing List
Junio C Hamano <gitster@pobox.com> writes:
> Michael Strawbridge <michael.strawbridge@amd.com> writes:
>
>> Subject: [PATCH] send-email: move validation code below process_address_list
>>
>> Move validation logic below processing of email address lists so that
>> email validation gets the proper email addresses.
>
> Hmph, without this patch, the tip of 'seen' passes t9001 on my box,
> but with it, it claims that it failed 58, 87, and 89.
FWIW, when this patch is used with 'master' (not 'seen'), t9001
claims the same three tests failed. The way #58 fails seems to be
identical to the way 'seen' with this patch failed, shown in the
message I am responding to.
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH] send-email: move validation code below process_address_list
2023-10-24 22:03 ` Junio C Hamano
@ 2023-10-25 18:48 ` Michael Strawbridge
2023-10-25 18:51 ` [PATCH v2] " Michael Strawbridge
1 sibling, 0 replies; 47+ messages in thread
From: Michael Strawbridge @ 2023-10-25 18:48 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, Bagas Sanjaya, Git Mailing List
On 10/24/23 18:03, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> Michael Strawbridge <michael.strawbridge@amd.com> writes:
>>
>>> Subject: [PATCH] send-email: move validation code below process_address_list
>>>
>>> Move validation logic below processing of email address lists so that
>>> email validation gets the proper email addresses.
>>
>> Hmph, without this patch, the tip of 'seen' passes t9001 on my box,
>> but with it, it claims that it failed 58, 87, and 89.
>
> FWIW, when this patch is used with 'master' (not 'seen'), t9001
> claims the same three tests failed. The way #58 fails seems to be
> identical to the way 'seen' with this patch failed, shown in the
> message I am responding to.
I'm sorry to have wasted your time with patch 1. I had done the other manual
tests but ended up forgetting the automated ones.
^ permalink raw reply [flat|nested] 47+ messages in thread
* [PATCH v2] send-email: move validation code below process_address_list
2023-10-24 22:03 ` Junio C Hamano
2023-10-25 18:48 ` Michael Strawbridge
@ 2023-10-25 18:51 ` Michael Strawbridge
2023-10-26 12:44 ` Junio C Hamano
1 sibling, 1 reply; 47+ messages in thread
From: Michael Strawbridge @ 2023-10-25 18:51 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, Bagas Sanjaya, Git Mailing List
From 67223238d9b1977d20b1286055d7f197e4d746e9 Mon Sep 17 00:00:00 2001
From: Michael Strawbridge <michael.strawbridge@amd.com>
Date: Wed, 11 Oct 2023 16:13:13 -0400
Subject: [PATCH v2] send-email: move validation code below
process_address_list
Move validation logic below processing of email address lists so that
email validation gets the proper email addresses. As a side effect,
some initialization needed to be moved down. In order for validation
and the actual email sending to have the same initial state, the
initialized variables that get modified by pre_process_file are
encapsulated in a new function.
This fixes email address validation errors when the optional
perl module Email::Valid is installed and multiple addresses are passed
in on a single to/cc argument like --to=foo@example.com,bar@example.com.
A new test was added to t9001 to expose failures with this case in the
future.
Fixes: a8022c5f7b67 ("send-email: expose header information to git-send-email's sendemail-validate hook")
Reported-by: Bagas Sanjaya <bagasdotme@gmail.com>
Signed-off-by: Michael Strawbridge <michael.strawbridge@amd.com>
---
git-send-email.perl | 60 +++++++++++++++++++++++--------------------
t/t9001-send-email.sh | 19 ++++++++++++++
2 files changed, 51 insertions(+), 28 deletions(-)
diff --git a/git-send-email.perl b/git-send-email.perl
index 288ea1ae80..ce22a5e06d 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -799,30 +799,6 @@ sub is_format_patch_arg {
$time = time - scalar $#files;
-if ($validate) {
- # FIFOs can only be read once, exclude them from validation.
- my @real_files = ();
- foreach my $f (@files) {
- unless (-p $f) {
- push(@real_files, $f);
- }
- }
-
- # Run the loop once again to avoid gaps in the counter due to FIFO
- # arguments provided by the user.
- my $num = 1;
- my $num_files = scalar @real_files;
- $ENV{GIT_SENDEMAIL_FILE_TOTAL} = "$num_files";
- foreach my $r (@real_files) {
- $ENV{GIT_SENDEMAIL_FILE_COUNTER} = "$num";
- pre_process_file($r, 1);
- validate_patch($r, $target_xfer_encoding);
- $num += 1;
- }
- delete $ENV{GIT_SENDEMAIL_FILE_COUNTER};
- delete $ENV{GIT_SENDEMAIL_FILE_TOTAL};
-}
-
@files = handle_backup_files(@files);
if (@files) {
@@ -1754,10 +1730,6 @@ sub send_message {
return 1;
}
-$in_reply_to = $initial_in_reply_to;
-$references = $initial_in_reply_to || '';
-$message_num = 0;
-
sub pre_process_file {
my ($t, $quiet) = @_;
@@ -2023,6 +1995,38 @@ sub process_file {
return 1;
}
+sub initialize_modified_loop_vars {
+ $in_reply_to = $initial_in_reply_to;
+ $references = $initial_in_reply_to || '';
+ $message_num = 0;
+}
+
+if ($validate) {
+ # FIFOs can only be read once, exclude them from validation.
+ my @real_files = ();
+ foreach my $f (@files) {
+ unless (-p $f) {
+ push(@real_files, $f);
+ }
+ }
+
+ # Run the loop once again to avoid gaps in the counter due to FIFO
+ # arguments provided by the user.
+ my $num = 1;
+ my $num_files = scalar @real_files;
+ $ENV{GIT_SENDEMAIL_FILE_TOTAL} = "$num_files";
+ initialize_modified_loop_vars();
+ foreach my $r (@real_files) {
+ $ENV{GIT_SENDEMAIL_FILE_COUNTER} = "$num";
+ pre_process_file($r, 1);
+ validate_patch($r, $target_xfer_encoding);
+ $num += 1;
+ }
+ delete $ENV{GIT_SENDEMAIL_FILE_COUNTER};
+ delete $ENV{GIT_SENDEMAIL_FILE_TOTAL};
+}
+
+initialize_modified_loop_vars();
foreach my $t (@files) {
while (!process_file($t)) {
# user edited the file
diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index 263db3ad17..ccff2ad647 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -633,6 +633,25 @@ test_expect_success $PREREQ "--validate respects absolute core.hooksPath path" '
test_cmp expect actual
'
+test_expect_success $PREREQ "--validate hook supports multiple addresses in arguments" '
+ hooks_path="$(pwd)/my-hooks" &&
+ test_config core.hooksPath "$hooks_path" &&
+ test_when_finished "rm my-hooks.ran" &&
+ test_must_fail git send-email \
+ --from="Example <nobody@example.com>" \
+ --to=nobody@example.com,abc@example.com \
+ --smtp-server="$(pwd)/fake.sendmail" \
+ --validate \
+ longline.patch 2>actual &&
+ test_path_is_file my-hooks.ran &&
+ cat >expect <<-EOF &&
+ fatal: longline.patch: rejected by sendemail-validate hook
+ fatal: command '"'"'git hook run --ignore-missing sendemail-validate -- <patch> <header>'"'"' died with exit code 1
+ warning: no patches were sent
+ EOF
+ test_cmp expect actual
+'
+
test_expect_success $PREREQ "--validate hook supports header argument" '
write_script my-hooks/sendemail-validate <<-\EOF &&
if test "$#" -ge 2
--
2.42.GIT
^ permalink raw reply related [flat|nested] 47+ messages in thread* Re: [PATCH v2] send-email: move validation code below process_address_list
2023-10-25 18:51 ` [PATCH v2] " Michael Strawbridge
@ 2023-10-26 12:44 ` Junio C Hamano
2023-10-26 13:11 ` Michael Strawbridge
0 siblings, 1 reply; 47+ messages in thread
From: Junio C Hamano @ 2023-10-26 12:44 UTC (permalink / raw)
To: Michael Strawbridge; +Cc: Jeff King, Bagas Sanjaya, Git Mailing List
Michael Strawbridge <michael.strawbridge@amd.com> writes:
> From 67223238d9b1977d20b1286055d7f197e4d746e9 Mon Sep 17 00:00:00 2001
> From: Michael Strawbridge <michael.strawbridge@amd.com>
> Date: Wed, 11 Oct 2023 16:13:13 -0400
> Subject: [PATCH v2] send-email: move validation code below
> process_address_list
Why do these in-body headers to lie about the author date?
By the way, the in-body header does seem to support the header line
folding (see the "subject" one here).
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v2] send-email: move validation code below process_address_list
2023-10-26 12:44 ` Junio C Hamano
@ 2023-10-26 13:11 ` Michael Strawbridge
0 siblings, 0 replies; 47+ messages in thread
From: Michael Strawbridge @ 2023-10-26 13:11 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, Bagas Sanjaya, Git Mailing List
On 10/26/23 08:44, Junio C Hamano wrote:
> Michael Strawbridge <michael.strawbridge@amd.com> writes:
>
>> From 67223238d9b1977d20b1286055d7f197e4d746e9 Mon Sep 17 00:00:00 2001
>> From: Michael Strawbridge <michael.strawbridge@amd.com>
>> Date: Wed, 11 Oct 2023 16:13:13 -0400
>> Subject: [PATCH v2] send-email: move validation code below
>> process_address_list
>
> Why do these in-body headers to lie about the author date?
Sorry. They weren't meant to. I have been amending my local commit
rather than making new ones for every version. It seems that when
I do that, the author date stays at the first time the commit was
created. I wasn't aware of that unintended side effect.
>
> By the way, the in-body header does seem to support the header line
> folding (see the "subject" one here).
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH] send-email: move validation code below process_address_list
2023-10-24 20:19 ` [PATCH] send-email: move validation code below process_address_list Michael Strawbridge
2023-10-24 21:55 ` Junio C Hamano
@ 2023-10-25 6:50 ` Jeff King
2023-10-25 18:47 ` Michael Strawbridge
2023-10-25 7:43 ` Uwe Kleine-König
2 siblings, 1 reply; 47+ messages in thread
From: Jeff King @ 2023-10-25 6:50 UTC (permalink / raw)
To: Michael Strawbridge; +Cc: Junio C Hamano, Bagas Sanjaya, Git Mailing List
On Tue, Oct 24, 2023 at 04:19:43PM -0400, Michael Strawbridge wrote:
> Move validation logic below processing of email address lists so that
> email validation gets the proper email addresses.
>
> This fixes email address validation errors when the optional
> perl module Email::Valid is installed and multiple addresses are passed
> in on a single to/cc argument like --to=foo@example.com,bar@example.com.
Is there a test we can include here?
> @@ -2023,6 +1999,30 @@ sub process_file {
> return 1;
> }
>
> +if ($validate) {
So the new spot is right before we call process_file() on each of the
input files. It is a little hard to follow because of the number of
functions defined inline in the middle of the script, but I think that
is a reasonable spot. It is after we have called process_address_list()
on to/cc/bcc, which I think fixes the regression. But it is also after
we sanitize $reply_to, etc, which seems like a good idea.
But I think putting it down that far is the source of the test failures.
The culprit seems not to be the call to validate_patch() in the loop you
moved, but rather pre_process_file(), which was added in your earlier
a8022c5f7b (send-email: expose header information to git-send-email's
sendemail-validate hook, 2023-04-19).
It looks like the issue is the global $message_num variable which is
incremented by pre_process_file(). On line 1755 (on the current tip of
master), we set it to 0. And your patch moves the validation across
there (from line ~799 to ~2023).
And that's why the extra increments didn't matter when you added the
calls to pre_process_file() in your earlier patch; they all happened
before we reset $message_num to 0. But now they happen after.
To be fair, this is absolutely horrific perl code. There's over a
thousand lines of function definitions, and then hidden in the middle
are some global variable assignments!
So we have a few options, I think:
1. Reset $message_num to 0 after validating (maybe we also need
to reset $in_reply_to, etc, set at the same time? I didn't check).
This feels like a hack.
2. Move the validation down, but not so far down. Like maybe right
after we set up the @files list with the $compose.final name. This
is the smallest diff, but feels like we haven't really made the
world a better place.
3. Move the $message_num, etc, initialization to right before we call
the process_file() loop, which is what expects to use them. Like
this (squashed into your patch):
diff --git a/git-send-email.perl b/git-send-email.perl
index a898dbc76e..d44db14223 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -1730,10 +1730,6 @@ sub send_message {
return 1;
}
-$in_reply_to = $initial_in_reply_to;
-$references = $initial_in_reply_to || '';
-$message_num = 0;
-
sub pre_process_file {
my ($t, $quiet) = @_;
@@ -2023,6 +2019,10 @@ sub process_file {
delete $ENV{GIT_SENDEMAIL_FILE_TOTAL};
}
+$in_reply_to = $initial_in_reply_to;
+$references = $initial_in_reply_to || '';
+$message_num = 0;
+
foreach my $t (@files) {
while (!process_file($t)) {
# user edited the file
That seems to make the test failures go away. It is still weird that the
validation code is calling pre_process_file(), which increments
$message_num, without us having set it up in any meaningful way. I'm not
sure if there are bugs lurking there or not. I'm not impressed by the
general quality of this code, and I'm kind of afraid to keep looking
deeper.
-Peff
^ permalink raw reply related [flat|nested] 47+ messages in thread* Re: [PATCH] send-email: move validation code below process_address_list
2023-10-25 6:50 ` [PATCH] " Jeff King
@ 2023-10-25 18:47 ` Michael Strawbridge
0 siblings, 0 replies; 47+ messages in thread
From: Michael Strawbridge @ 2023-10-25 18:47 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, Bagas Sanjaya, Git Mailing List
On 10/25/23 02:50, Jeff King wrote:
> On Tue, Oct 24, 2023 at 04:19:43PM -0400, Michael Strawbridge wrote:
>
>> Move validation logic below processing of email address lists so that
>> email validation gets the proper email addresses.
>>
>> This fixes email address validation errors when the optional
>> perl module Email::Valid is installed and multiple addresses are passed
>> in on a single to/cc argument like --to=foo@example.com,bar@example.com.
>
> Is there a test we can include here?
>
>> @@ -2023,6 +1999,30 @@ sub process_file {
>> return 1;
>> }
>>
>> +if ($validate) {
>
> So the new spot is right before we call process_file() on each of the
> input files. It is a little hard to follow because of the number of
> functions defined inline in the middle of the script, but I think that
> is a reasonable spot. It is after we have called process_address_list()
> on to/cc/bcc, which I think fixes the regression. But it is also after
> we sanitize $reply_to, etc, which seems like a good idea.
>
> But I think putting it down that far is the source of the test failures.
>
> The culprit seems not to be the call to validate_patch() in the loop you
> moved, but rather pre_process_file(), which was added in your earlier
> a8022c5f7b (send-email: expose header information to git-send-email's
> sendemail-validate hook, 2023-04-19).
>
> It looks like the issue is the global $message_num variable which is
> incremented by pre_process_file(). On line 1755 (on the current tip of
> master), we set it to 0. And your patch moves the validation across
> there (from line ~799 to ~2023).
>
> And that's why the extra increments didn't matter when you added the
> calls to pre_process_file() in your earlier patch; they all happened
> before we reset $message_num to 0. But now they happen after.
>
> To be fair, this is absolutely horrific perl code. There's over a
> thousand lines of function definitions, and then hidden in the middle
> are some global variable assignments!
I agree. Following where things are initialized seems to be especially troublesome.
>
> So we have a few options, I think:
>
> 1. Reset $message_num to 0 after validating (maybe we also need
> to reset $in_reply_to, etc, set at the same time? I didn't check).
> This feels like a hack.
>
> 2. Move the validation down, but not so far down. Like maybe right
> after we set up the @files list with the $compose.final name. This
> is the smallest diff, but feels like we haven't really made the
> world a better place.
>
> 3. Move the $message_num, etc, initialization to right before we call
> the process_file() loop, which is what expects to use them. Like
> this (squashed into your patch):
>
> diff --git a/git-send-email.perl b/git-send-email.perl
> index a898dbc76e..d44db14223 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -1730,10 +1730,6 @@ sub send_message {
> return 1;
> }
>
> -$in_reply_to = $initial_in_reply_to;
> -$references = $initial_in_reply_to || '';
> -$message_num = 0;
> -
> sub pre_process_file {
> my ($t, $quiet) = @_;
>
> @@ -2023,6 +2019,10 @@ sub process_file {
> delete $ENV{GIT_SENDEMAIL_FILE_TOTAL};
> }
>
> +$in_reply_to = $initial_in_reply_to;
> +$references = $initial_in_reply_to || '';
> +$message_num = 0;
> +
> foreach my $t (@files) {
> while (!process_file($t)) {
> # user edited the file
>
The above patch was a great place to start. Thank you! In order to address
the fact that validation and actually sending the emails should have the same
initial conditions I created a new function to set the variables and call it
instead.
> That seems to make the test failures go away. It is still weird that the
> validation code is calling pre_process_file(), which increments
> $message_num, without us having set it up in any meaningful way. I'm not
> sure if there are bugs lurking there or not. I'm not impressed by the
> general quality of this code, and I'm kind of afraid to keep looking
> deeper.
>
> -Peff
^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH] send-email: move validation code below process_address_list
2023-10-24 20:19 ` [PATCH] send-email: move validation code below process_address_list Michael Strawbridge
2023-10-24 21:55 ` Junio C Hamano
2023-10-25 6:50 ` [PATCH] " Jeff King
@ 2023-10-25 7:43 ` Uwe Kleine-König
2023-10-27 13:04 ` Junio C Hamano
2 siblings, 1 reply; 47+ messages in thread
From: Uwe Kleine-König @ 2023-10-25 7:43 UTC (permalink / raw)
To: Michael Strawbridge
Cc: Jeff King, Junio C Hamano, Bagas Sanjaya, Git Mailing List
[-- Attachment #1: Type: text/plain, Size: 1386 bytes --]
Hello,
On Tue, Oct 24, 2023 at 04:19:43PM -0400, Michael Strawbridge wrote:
> >From 09ea51d63cebdf9ff0c073ef86e21b4b09c268e5 Mon Sep 17 00:00:00 2001
> From: Michael Strawbridge <michael.strawbridge@amd.com>
> Date: Wed, 11 Oct 2023 16:13:13 -0400
> Subject: [PATCH] send-email: move validation code below process_address_list
>
> Move validation logic below processing of email address lists so that
> email validation gets the proper email addresses.
>
> This fixes email address validation errors when the optional
> perl module Email::Valid is installed and multiple addresses are passed
> in on a single to/cc argument like --to=foo@example.com,bar@example.com.
>
> Reported-by: Bagas Sanjaya <bagasdotme@gmail.com>
> Signed-off-by: Michael Strawbridge <michael.strawbridge@amd.com>
If you do Fixes: trailers as the kernel does, this could get:
Fixes: a8022c5f7b67 ("send-email: expose header information to git-send-email's sendemail-validate hook")
I tested this patch on top of main (2e8e77cbac8a) and it fixes the
regression I reported in a separate thread (where Jeff pointed out this
patch as fixing it).
Tested-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Thanks
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 47+ messages in thread* Re: [PATCH] send-email: move validation code below process_address_list
2023-10-25 7:43 ` Uwe Kleine-König
@ 2023-10-27 13:04 ` Junio C Hamano
0 siblings, 0 replies; 47+ messages in thread
From: Junio C Hamano @ 2023-10-27 13:04 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Michael Strawbridge, Jeff King, Bagas Sanjaya, Git Mailing List
Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes:
>> This fixes email address validation errors when the optional
>> perl module Email::Valid is installed and multiple addresses are passed
>> in on a single to/cc argument like --to=foo@example.com,bar@example.com.
>>
>> Reported-by: Bagas Sanjaya <bagasdotme@gmail.com>
>> Signed-off-by: Michael Strawbridge <michael.strawbridge@amd.com>
>
> If you do Fixes: trailers as the kernel does, this could get:
>
> Fixes: a8022c5f7b67 ("send-email: expose header information to git-send-email's sendemail-validate hook")
While referring to a concrete commit object name is great, we tend
not to use that particular trailer in this project; rather, we
prefer to see description on how and in what way the culprit change
was undesirable.
> I tested this patch on top of main (2e8e77cbac8a) and it fixes the
> regression I reported in a separate thread (where Jeff pointed out this
> patch as fixing it).
Great. Thanks.
^ permalink raw reply [flat|nested] 47+ messages in thread