body-CC-comment regression
Hi,
I recently noticed that after an upgrade, git-send-email (2.10.2) started aborting when trying to send patches that had a linux-kernel stable-tag in its body. For example,
Cc: <[email protected]> # 4.4
was now parsed as
"[email protected]#4.4"
which resulted in
Died at /usr/libexec/git-core/git-send-email line 1332, <FIN> line 1.
This tends to happen in the middle of a series after the cover letter had been sent just to make things worse...
I finally got around to looking into this today and discovered this thread ("Re: Formatting problem send_mail in version 2.10.0"):
https://marc.info/?l=git&m=147633706724793&w=2
The problem with the resulting fixes that are now in 2.11.1 is that git-send-email no longer discards the trailing comment but rather shoves it into the name after adding some random white space:
"# 3 . 3 . x : 1b9508f : sched : Rate-limit newidle" <[email protected]>"
This example is based on the example fromDocumentation/process/stable-kernel-rules.rst:
Cc: <[email protected]> # 3.3.x: 1b9508f: sched: Rate-limit newidle
and this format for stable-tags has been documented at least since 2009 and 8e9b9362266d ("Doc/stable rules: add new cherry-pick logic"), and has been supported by git since 2012 and 831a488b76e0 ("git-send-email:
remove garbage after email address") I believe.
Can we please revert to the old behaviour of simply discarding such comments (from body-CC:s) or at least make it configurable through a configuration option?
Thanks,
Johan
Quoted 10 lines · Johan Hovold
I recently noticed that after an upgrade, git-send-email (2.10.2) started aborting when trying to send patches that had a linux-kernel stable-tag in its body. For example,
Cc: <[email protected]> # 4.4was now parsed as
"[email protected]#4.4"...
It sounds like a fallout of this:
https://public-inbox.org/git/[email protected]/#t
and any change to "fix" you may break the other person.
Quoted 3 lines
Can we please revert to the old behaviour of simply discarding such comments (from body-CC:s) or at least make it configurable through a configuration option?
If I recall the old thread correctly, it was reported that using Mail::Address without forcing git-send-email fall back to its own non-parsing-but-paste-address-looking-things-together code would solve it, so can the "make it configurable" be just "install Mail::Address"?
Quoted 10 lines · Junio C Hamano
Quoted 8 lines · Johan Hovold
I recently noticed that after an upgrade, git-send-email (2.10.2) started aborting when trying to send patches that had a linux-kernel stable-tag in its body. For example,
Cc: <[email protected]> # 4.4was now parsed as
Quoted 8 lines
Quoted 2 lines
"[email protected]#4.4"...
It sounds like a fallout of this:
https://public-inbox.org/git/[email protected]/#t
and any change to "fix" you may break the other person.
Yes, that's the thread I was referring to as well, and the reported breakage is the same even if the reporter used a non-standard stable-tag format (e.g. a "[4.8+]" suffix).
What I'm wondering is whether the alternative fix proposed in that thread, to revert to the old behaviour of discarding trailing comments, should be considered instead of what was implemented.
Quoted 9 lines
Quoted 3 lines
Can we please revert to the old behaviour of simply discarding such comments (from body-CC:s) or at least make it configurable through a configuration option?
If I recall the old thread correctly, it was reported that using
Mail::Addresswithout forcing git-send-email fall back to its own non-parsing-but-paste-address-looking-things-together code would solve it, so can the "make it configurable" be just "installMail::Address"?
I believe git-send-email's parser was changed to mimic Mail::Address, and installing it does not seem to change the behaviour of including any trailing comments in the name.
Thanks,
Johan
Quoted 8 lines · Johan Hovold
Hi,
I recently noticed that after an upgrade, git-send-email (2.10.2) started aborting when trying to send patches that had a linux-kernel stable-tag in its body. For example,
Cc: <[email protected]> # 4.4
Quoted 7 lines
was now parsed as
"[email protected]#4.4"which resulted in
Died at
/usr/libexec/git-core/git-send-emailline 1332, <FIN> line 1.
This has changed in e3fdbcc8e1 (parse_mailboxes: accept extra text after <...> address, 2016-10-13), released v2.11.0 as you noticed:
Quoted 11 lines
The problem with the resulting fixes that are now in 2.11.1 is that git-send-email no longer discards the trailing comment but rather shoves it into the name after adding some random white space:
"# 3 . 3 . x : 1b9508f : sched : Rate-limit newidle" <[email protected]>"This example is based on the example from
Documentation/process/stable-kernel-rules.rst:Cc: <[email protected]> # 3.3.x: 1b9508f: sched: Rate-limit newidle
Quoted 8 lines
and this format for stable-tags has been documented at least since 2009 and
8e9b9362266d("Doc/stable rules: add new cherry-pick logic"), and has been supported by git since 2012 and831a488b76e0("git-send-email:
remove garbage after email address") I believe.Can we please revert to the old behaviour of simply discarding such comments (from body-CC:s) or at least make it configurable through a configuration option?
The problem is that we now accept list of emails instead of just one email, so it's hard to define what "comments after the email", for example
Is not accepted as two emails.
So, just stripping whatever comes after # before parsing the list of emails would change the behavior once more, and possibly break other user's flow. Dropping the garbage after the email while parsing is possible, but only when we use our in-house parser (and we currently use Perl's Mail::Address when available).
So, a proper fix is far from obvious, and unfortunately I won't have time to work on that, at least not before a while.
OTOH, the current behavior isn't that bad. It accepts the input, and extracts a valid email out of it. Just the display name is admitedly suboptimal ...
Signature · 2 lines
Matthieu Moy http://www-verimag.imag.fr/~moy/
Quoted 49 lines · Matthieu Moy
Quoted 8 lines · Johan Hovold
Hi,
I recently noticed that after an upgrade, git-send-email (2.10.2) started aborting when trying to send patches that had a linux-kernel stable-tag in its body. For example,
Cc: <[email protected]> # 4.4Quoted 7 lines
was now parsed as
"[email protected]#4.4"which resulted in
Died at
/usr/libexec/git-core/git-send-emailline 1332, <FIN> line 1.This has changed in
e3fdbcc8e1(parse_mailboxes: accept extra text after <...> address, 2016-10-13), released v2.11.0 as you noticed:Quoted 11 lines
The problem with the resulting fixes that are now in 2.11.1 is that git-send-email no longer discards the trailing comment but rather shoves it into the name after adding some random white space:
"# 3 . 3 . x : 1b9508f : sched : Rate-limit newidle" <[email protected]>"This example is based on the example from
Documentation/process/stable-kernel-rules.rst:Cc: <[email protected]> # 3.3.x: 1b9508f: sched: Rate-limit newidleQuoted 8 lines
and this format for stable-tags has been documented at least since 2009 and
8e9b9362266d("Doc/stable rules: add new cherry-pick logic"), and has been supported by git since 2012 and831a488b76e0("git-send-email:
remove garbage after email address") I believe.Can we please revert to the old behaviour of simply discarding such comments (from body-CC:s) or at least make it configurable through a configuration option?
The problem is that we now accept list of emails instead of just one email, so it's hard to define what "comments after the email", for example
Cc <[email protected]> # , <[email protected]>Is not accepted as two emails.
Quoted 8 lines
So, just stripping whatever comes after # before parsing the list of emails would change the behavior once more, and possibly break other user's flow. Dropping the garbage after the email while parsing is possible, but only when we use our in-house parser (and we currently use Perl's
Mail::Addresswhen available).So, a proper fix is far from obvious, and unfortunately I won't have time to work on that, at least not before a while.
There is another option, namely to only accept a single address for tags in the body. I understand that being able to copy a CC-header to either the header section or to the command line could be useful, but I don't really see the point in allowing this in the tags in the body (a SoB always has one address, and so should a CC-tag).
And since this is a regression for something that has been working for years that was introduced by a new feature, I also think it's reasonable to (partially) revert the feature.
Quoted 3 lines
OTOH, the current behavior isn't that bad. It accepts the input, and extracts a valid email out of it. Just the display name is admitedly suboptimal ...
Yeah, but the display name can end up with so much noise that auto-cc is effectively broken for people submitting kernel patches (with stable tags) as the only way to avoid it is to suppress all bodycc.
So what I'm proposing is to revert to the earlier behaviour of only allowing one address per body tag by simply discarding anything after the address.
Something like the below seems to do the trick.
Thanks,
Johan
From f551b4ca9926624dc7af6c286d7cf0f97af39541 Mon Sep 17 00:00:00 2001
From: Johan Hovold <[email protected]>
Date: Fri, 17 Feb 2017 11:55:47 +0100
Subject: [PATCH] send-email: only allow one address per body tag
Adding comments after a tag in the body is a common practise (e.g. in the Linux kernel) and git-send-email has been supporting this for years by removing any trailing cruft after the address.
After some recent changes, any trailing comment is now instead appended to the recipient name (with some random white space inserted) resulting in undesirable noise in the headers, for example:
Revert to the earlier behaviour of discarding anything after the (first) address in a tag while parsing the body.
Note that multiple addresses after are still allowed after a command-line switch (and in a CC-header).
and --bcc")
address")
--- git-send-email.perl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
git-send-email.perl +1 -1
@@ -1563,7 +1563,7 @@ foreach my $t (@files) { # Now parse the message body while(<$fh>) { $message .= $_; - if (/^(Signed-off-by|Cc): (.*)$/i) { + if (/^(Signed-off-by|Cc): ([^>]*>?)/i) { chomp; my ($what, $c) = ($1, $2); chomp $c;
Signature · 1 line
2.11.1
Quoted 5 lines · Johan Hovold
There is another option, namely to only accept a single address for tags in the body. I understand that being able to copy a CC-header to either the header section or to the command line could be useful, but I don't really see the point in allowing this in the tags in the body (a SoB always has one address, and so should a CC-tag).
I mostly agree for the SoB, but why should a Cc tag have only one email?
The "multiple emails per Cc: field" has been there for a while already (b1c8a11c8024 released in 2.6.0, sept 2015), some users may have got used to it. What you are proposing breaks their flow.
Quoted 3 lines
And since this is a regression for something that has been working for years that was introduced by a new feature, I also think it's reasonable to (partially) revert the feature.
I'd find it rather ironic to fix your case by breaking a feature that has been working for more than a year :-(. What would you answer to a contributor comming one year from now and proposing to revert your reversion because it breaks his flow?
All that said, I think another fix would be both satisfactory for everyone and rather simple:
1) Stop calling Mail::Address even if available. It used to make sense to do that when our in-house parser was really poor, but we now have something essentially as good as Mail::Address. We test our parser against Mail::Address and we do have a few known differences (see t9000), but they are really corner-cases and shouldn't matter.
A good consequence of this is that we stop depending on the way Perl is installed to parse emails. Regardless of the current issue, I think it is a good thing.
2) Modify our in-house parser to discard garbage after the >. The patch should look like (untested):
perl/Git.pm +5 -5
@@ -903,11 +903,11 @@ sub parse_mailboxes { my (@addr_list, @phrase, @address, @comment, @buffer) = (); foreach my $token (@tokens) { if ($token =~ /^[,;]$/) { - # if buffer still contains undeterminated strings - # append it at the end of @address or @phrase - if ($end_of_addr_seen) { - push @phrase, @buffer; - } else { + # if buffer still contains undeterminated + # strings append it at the end of @address, + # unless we already saw the closing >, in + # which case we discard it. + if (!$end_of_addr_seen) { push @address, @buffer; }
What do you think?
Signature · 2 lines
Matthieu Moy http://www-verimag.imag.fr/~moy/
Quoted 9 lines · Matthieu Moy
Quoted 5 lines · Johan Hovold
There is another option, namely to only accept a single address for tags in the body. I understand that being able to copy a CC-header to either the header section or to the command line could be useful, but I don't really see the point in allowing this in the tags in the body (a SoB always has one address, and so should a CC-tag).
I mostly agree for the SoB, but why should a Cc tag have only one email?
For symmetry (with SoB) and readability reasons (one tag per line). These are body tags, not mail headers, after all.
Quoted 3 lines
The "multiple emails per Cc: field" has been there for a while already (
b1c8a11c8024released in 2.6.0, sept 2015), some users may have got used to it. What you are proposing breaks their flow.
Note that that commit never mentions multiple addresses in either headers or body-tags -- it's all about being able to specify multiple entries on the command line.
There does not seem to be single commit in the kernel where multiple address are specified in a CC tag since after git-send-email started allowing it, but there are ten commits before (to my surprise), and that should be contrasted with at least 4178 commits with trailing comments including a # sign.
Quoted 8 lines
Quoted 3 lines
And since this is a regression for something that has been working for years that was introduced by a new feature, I also think it's reasonable to (partially) revert the feature.
I'd find it rather ironic to fix your case by breaking a feature that has been working for more than a year :-(. What would you answer to a contributor comming one year from now and proposing to revert your reversion because it breaks his flow?
Such conflicts are not uncommon when dealing with regressions introduced by new features, and need to be dealt with on a case-by-case basis. But the fact that trailing comments have been properly supported for more than four years should carry some weight.
Quoted 9 lines
All that said, I think another fix would be both satisfactory for everyone and rather simple:
1) Stop calling
Mail::Addresseven if available. It used to make sense to do that when our in-house parser was really poor, but we now have something essentially as good asMail::Address. We test our parser againstMail::Addressand we do have a few known differences (see t9000), but they are really corner-cases and shouldn't matter.
Quoted 3 lines
A good consequence of this is that we stop depending on the way Perl is installed to parse emails. Regardless of the current issue, I think it is a good thing.
Right, that sounds like the right thing to do regardless.
Quoted 22 lines
2) Modify our in-house parser to discard garbage after the >. The patch should look like (untested):
perl/Git.pm +5
-5@@ -903,11 +903,11 @@ sub parse_mailboxes { my (@addr_list, @phrase, @address, @comment, @buffer) = (); foreach my $token (@tokens) { if ($token =~ /^[,;]$/) { - # if buffer still contains undeterminated strings - # append it at the end of @address or @phrase - if ($end_of_addr_seen) { - push @phrase, @buffer; - } else { + # if buffer still contains undeterminated + # strings append it at the end of @address, + # unless we already saw the closing >, in + # which case we discard it. + if (!$end_of_addr_seen) { push @address, @buffer; }
Quoted 1 line
What do you think?
Sounds perfectly fine to me, and seems to work too after quick test.
Note however that there's another minor issue with using multiple addresses in a Cc-tag in that it breaks --suppress-cc=self, but I guess that can be fixed separately.
Thanks,
Johan
Quoted 10 lines · Matthieu Moy
Quoted 5 lines
Johan Hovold <[email protected]> writes:
The "multiple emails per Cc: field" has been there for a while already (
b1c8a11c8024released in 2.6.0, sept 2015), some users may have got used to it. What you are proposing breaks their flow.Note that that commit never mentions multiple addresses in either headers or body-tags -- it's all about being able to specify multiple entries on the command line.
Indeed. I'm not the author of the patch, but I was supervising the students who wrote it and "multiple addresses in Cc:" was not the goal, but a (IMHO positive) side effect we discovered after the fact.
If I had a time machine, I'd probably go back then and forbid multiple addresses there, but ...
Quoted 5 lines
There does not seem to be single commit in the kernel where multiple address are specified in a CC tag since after git-send-email started allowing it, but there are ten commits before (to my surprise), and that should be contrasted with at least 4178 commits with trailing comments including a # sign.
Hey, there's a life outside the kernel ;-).
Quoted 7 lines
Quoted 1 line
1) Stop calling
Mail::Addresseven if available.[...]Right, that sounds like the right thing to do regardless.
Quoted 1 line
2) Modify our in-house parser to discard garbage after the >. [...]
Sounds perfectly fine to me, and seems to work too after quick test.
OK, sounds like the way to go.
Do you want to work on a patch? If not, I should be able to do that myself. The code changes are straightforward, but we probably want a proper test for that.
Quoted 2 lines
addresses in a Cc-tag in that it breaks --suppress-cc=self, but I guess that can be fixed separately.
OK. If it's unrelated enough, please start a separate thread to explain the problem (and/or write a patch ;-) ).
Thanks,
Signature · 2 lines
Matthieu Moy http://www-verimag.imag.fr/~moy/
Quoted 7 lines · Matthieu Moy
Quoted 7 lines · Matthieu Moy
Quoted 5 lines
Johan Hovold <[email protected]> writes:
The "multiple emails per Cc: field" has been there for a while already (
b1c8a11c8024released in 2.6.0, sept 2015), some users may have got used to it. What you are proposing breaks their flow.
Quoted 7 lines
Quoted 3 lines
Note that that commit never mentions multiple addresses in either headers or body-tags -- it's all about being able to specify multiple entries on the command line.
Indeed. I'm not the author of the patch, but I was supervising the students who wrote it and "multiple addresses in Cc:" was not the goal, but a (IMHO positive) side effect we discovered after the fact.
Yeah, and the broken --suppress-cc=self I mention below is indicative of that too.
Quoted 10 lines
If I had a time machine, I'd probably go back then and forbid multiple addresses there, but ...
Quoted 5 lines
There does not seem to be single commit in the kernel where multiple address are specified in a CC tag since after git-send-email started allowing it, but there are ten commits before (to my surprise), and that should be contrasted with at least 4178 commits with trailing comments including a # sign.
Hey, there's a life outside the kernel ;-).
Sure, but it's the origin of git as well as the tags we're discussing (I believe).
My point of bringing it up was that the multiple addresses in a CC-tag was indeed an unintended (and undocumented) side-effect and I doubt many people have started using it given that it's sort of counter-intuitive (again, compare with SoB).
If either the trailing comments or multiple addresses in a CC-tag has to go, I think dropping the latter is clearly the best choice.
Quoted 6 lines
Quoted 6 lines
Quoted 1 line
1) Stop calling
Mail::Addresseven if available.[...]Right, that sounds like the right thing to do regardless.
Quoted 1 line
2) Modify our in-house parser to discard garbage after the >. [...]
Quoted 7 lines
Quoted 1 line
Sounds perfectly fine to me, and seems to work too after quick test.
OK, sounds like the way to go.
Do you want to work on a patch? If not, I should be able to do that myself. The code changes are straightforward, but we probably want a proper test for that.
Feel free to implement it this way if that's what people prefer. As long as trailing comments are supported and discarded, I don't really have a preference.
Quoted 5 lines
Quoted 2 lines
addresses in a Cc-tag in that it breaks --suppress-cc=self, but I guess that can be fixed separately.
OK. If it's unrelated enough, please start a separate thread to explain the problem (and/or write a patch ;-) ).
Well, it's related to the "offending" patch that added support for multiple addresses in tags. By disallowing that, as my fix does, the problem goes away.
# Now parse the message body
while(<$fh>) {
$message .= $_;
if (/^(Signed-off-by|Cc): (.*)$/i) {
chomp;
my ($what, $c) = ($1, $2);
chomp $c;
my $sc = sanitize_address($c);
if ($sc eq $sender) {
next if ($suppress_cc{'self'});The problem here is that $sc will never match $sender when there are more than one address in a tag. For example:
From: Johan Hovold <[email protected]>
...
results in
sc = alpha <[email protected]>, Johan Hovold <[email protected]> sender = Johan Hovold <[email protected]>
so that --suppress-cc=self is not honoured.
Thanks,
Johan
Quoted 2 lines · Matthieu Moy
I mostly agree for the SoB, but why should a Cc tag have only one email?
Because changing that clearly broke real and useful behavior.
The "multiple email addresses" thing is bogus and wrong. Just don't do it.
How would you even parse it sanely? Are the Cc: lines now SMTP-compliant with the whole escaping and all the usual "next line" rules?
For example, in email, the rule for "next line" is that if you're in a header block, and it starts with whitespace, then it's a continuation of the last line.
That's *not* how Cc: lines work in commit messages. They are all individual lines, and we have lots of tools (mainly just scripts with grepping) that simply depend on it.
So this notion that the bottom of the commit message is some email header crap is WRONG.
Stop it. It caused bugs. It's wrong. Don't do it.
Linus
Quoted 19 lines · Matthieu Moy
Quoted 1 line · Matthieu Moy
...
If I had a time machine, I'd probably go back then and forbid multiple addresses there, but ...Quoted 5 lines
There does not seem to be single commit in the kernel where multiple address are specified in a CC tag since after git-send-email started allowing it, but there are ten commits before (to my surprise), and that should be contrasted with at least 4178 commits with trailing comments including a # sign.
Hey, there's a life outside the kernel ;-).
...Quoted 6 lines
Quoted 1 line
1) Stop calling
Mail::Addresseven if available.[...]Right, that sounds like the right thing to do regardless.
Quoted 1 line
2) Modify our in-house parser to discard garbage after the >. [...]
Quoted 7 lines
Quoted 1 line
Sounds perfectly fine to me, and seems to work too after quick test.
OK, sounds like the way to go.
Do you want to work on a patch? If not, I should be able to do that myself. The code changes are straightforward, but we probably want a proper test for that.
The true headers and the things at the bottom seem to be handled in a separate loop in send-email, so treating Cc: found in the former and in the latter differently should be doable. I think it is OK to explicitly treat the latter as "these are not e-mail addresses, but just a single e-mail address possibly with non-address cruft", without losing the ability to have more than one addresses on a single CC: e-mail header.
Quoted 29 lines · Junio C Hamano
Quoted 19 lines · Matthieu Moy
Quoted 1 line · Matthieu Moy
...
If I had a time machine, I'd probably go back then and forbid multiple addresses there, but ...Quoted 5 lines
There does not seem to be single commit in the kernel where multiple address are specified in a CC tag since after git-send-email started allowing it, but there are ten commits before (to my surprise), and that should be contrasted with at least 4178 commits with trailing comments including a # sign.
Hey, there's a life outside the kernel ;-).
...Quoted 6 lines
Quoted 1 line
1) Stop calling
Mail::Addresseven if available.[...]Right, that sounds like the right thing to do regardless.
Quoted 1 line
2) Modify our in-house parser to discard garbage after the >. [...]
Quoted 7 lines
Quoted 1 line
Sounds perfectly fine to me, and seems to work too after quick test.
OK, sounds like the way to go.
Do you want to work on a patch? If not, I should be able to do that myself. The code changes are straightforward, but we probably want a proper test for that.
Quoted 7 lines
The true headers and the things at the bottom seem to be handled in a separate loop in send-email, so treating Cc: found in the former and in the latter differently should be doable. I think it is OK to explicitly treat the latter as "these are not e-mail addresses, but just a single e-mail address possibly with non-address cruft", without losing the ability to have more than one addresses on a single CC: e-mail header.
That's precisely what the patch I posted earlier in the thread did.
Thanks,
Johan
Quoted 1 line · Johan Hovold
That's precisely what the patch I posted earlier in the thread did.
That's good. I didn't see any patch yet but the message you are responding to is a response to Matthieu's message asking if you are planning to work on it, so I'd assume you are and and look forward to seeing a patch (or a series?) we can queue.
Thanks.
Quoted 5 lines · Junio C Hamano
Quoted 1 line · Johan Hovold
That's precisely what the patch I posted earlier in the thread did.
That's good. I didn't see any patch yet
It's here:
http://public-inbox.org/git/20170217110642.GD2625@localhost/
but as I explained, this removes a feature suported since several major releases and we have no idea how many users may use the "mupliple emails in one field". The approach I proposed does not suffer from this.
Signature · 2 lines
Matthieu Moy http://www-verimag.imag.fr/~moy/
Quoted 8 lines · Matthieu Moy
Quoted 5 lines · Junio C Hamano
Quoted 1 line · Johan Hovold
That's precisely what the patch I posted earlier in the thread did.
That's good. I didn't see any patch yet
Quoted 7 lines
It's here:
http://public-inbox.org/git/20170217110642.GD2625@localhost/
but as I explained, this removes a feature suported since several major releases and we have no idea how many users may use the "mupliple emails in one field". The approach I proposed does not suffer from this.
OK, so you are aiming higher to please both parties, i.e. those who place a single address but cruft at the end would get the cruft stripped when we grab a usable address from the field, and those who write two or more addresses would get all of these addresses?
That approach may still constrain what those in the former camp can write in the "cruft" part, like they cannot write comma or semicolon as part of the "cruft", no? If that does not pose a practical problem, then I can imagine it would work well for people in both camps.
Quoted 3 lines · Junio C Hamano
That approach may still constrain what those in the former camp can write in the "cruft" part, like they cannot write comma or semicolon as part of the "cruft", no?
Right. Indeed, this may be a problem since the use of "#" for stable seem to include commit message, and they may contain commas.
So, maybe Johan's patch is better indeed.
Signature · 2 lines
Matthieu Moy http://www-verimag.imag.fr/~moy/
Quoted 10 lines · Matthieu Moy
Quoted 3 lines · Junio C Hamano
That approach may still constrain what those in the former camp can write in the "cruft" part, like they cannot write comma or semicolon as part of the "cruft", no?
Right. Indeed, this may be a problem since the use of "#" for stable seem to include commit message, and they may contain commas.
So, maybe Johan's patch is better indeed.
OK, so I'll queue that one with your Ack for now so that we won't forget. I guess we still want a few tests?
Thanks.
Quoted 8 lines · Junio C Hamano
Quoted 6 lines · Matthieu Moy
Quoted 3 lines · Junio C Hamano
That approach may still constrain what those in the former camp can write in the "cruft" part, like they cannot write comma or semicolon as part of the "cruft", no?
Quoted 7 lines
Quoted 4 lines
Right. Indeed, this may be a problem since the use of "#" for stable seem to include commit message, and they may contain commas.
So, maybe Johan's patch is better indeed.
OK, so I'll queue that one with your Ack for now so that we won't forget. I guess we still want a few tests?
It seems that there is an expectation in one of the tests that needs to be adjusted.
PATCH v2 1/1 send-email: only allow one address per body tag
Adding comments after a tag in the body is a common practise (e.g. in the Linux kernel) and git-send-email has been supporting this for years by removing any trailing cruft after the address.
After some recent changes, any trailing comment is now instead appended to the recipient name (with some random white space inserted) resulting in undesirable noise in the headers, for example:
Revert to the earlier behaviour of discarding anything after the (first) address in a tag while parsing the body.
Note that multiple addresses after are still allowed after a command line switch (and in a CC header field).
Also note that --suppress-cc=self was never honoured when using multiple addresses in a tag.
and --bcc")
address")
---
v2:
- update the cc-trailer test
- amend commit message and mention the broken --suppress-cc=self
git-send-email.perl | 2 +- t/t9001-send-email.sh | 7 +++---- 2 files changed, 4 insertions(+), 5 deletions(-)
git-send-email.perl +1 -1
@@ -1563,7 +1563,7 @@ foreach my $t (@files) { # Now parse the message body while(<$fh>) { $message .= $_; - if (/^(Signed-off-by|Cc): (.*)$/i) { + if (/^(Signed-off-by|Cc): ([^>]*>?)/i) { chomp; my ($what, $c) = ($1, $2); chomp $c;
t/t9001-send-email.sh +3 -4
@@ -148,7 +148,6 @@ cat >expected-cc <<\EOF [email protected]! [email protected]! [email protected]! [email protected]! EOF "
@@ -159,9 +158,9 @@ test_expect_success $PREREQ 'cc trailer with various syntax' ' Test Cc: trailers. Cc: [email protected] - Cc: <[email protected]> # this is part of the name - Cc: <[email protected]>, <[email protected]> # [email protected] - Cc: "Some # Body" <[email protected]> [part.of.name.too] + Cc: <[email protected]> # trailing comments are ignored + Cc: <[email protected]>, <[email protected]> one address per line + Cc: "Some # Body" <[email protected]> [ <also.a.comment> ] EOF clean_fake_sendmail && git send-email -1 [email protected] \
Signature · 1 line
2.11.1
Re: [PATCH v2] send-email: only allow one address per body tag
git-send-email.perl foreach my $t (@files) { · line 1566
Quoted 8 lines · Johan Hovold
git-send-email.perl +1
-1@@ -1563,7 +1563,7 @@ foreach my $t (@files) { # Now parse the message body while(<$fh>) { $message .= $_; - if (/^(Signed-off-by|Cc): (.*)$/i) { + if (/^(Signed-off-by|Cc): ([^>]*>?)/i) {
I think this is acceptable, but this doesn't work with trailers like
A proper management of this kind of weird address should be doable by reusing the regexp parsing "..." in parse_mailbox:
my $re_quote = qr/"(?:[^\"\\]|\\.)*"/;
So the final regex would look like
if (/^(Signed-off-by|Cc): (([^>]*|"(?:[^\"\\]|\\.)*")>?)/i) {
I don't think that should block the patch inclusion, but it may be worth considering.
Anyway, thanks for the patch!
Signature · 2 lines
Matthieu Moy http://www-verimag.imag.fr/~moy/
Re: [PATCH v2] send-email: only allow one address per body tag
Quoted 20 lines · Matthieu Moy
git-send-email.perl foreach my $t (@files) { · line 1566
Quoted 8 lines · Johan Hovold
git-send-email.perl +1
-1@@ -1563,7 +1563,7 @@ foreach my $t (@files) { # Now parse the message body while(<$fh>) { $message .= $_; - if (/^(Signed-off-by|Cc): (.*)$/i) { + if (/^(Signed-off-by|Cc): ([^>]*>?)/i) {I think this is acceptable, but this doesn't work with trailers like
Cc "Some > Body" <[email protected]>A proper management of this kind of weird address should be doable by reusing the regexp parsing "..." in
parse_mailbox:my $re_quote = qr/"(?:[^\"\\]|\\.)*"/;
Quoted 8 lines
So the final regex would look like
if (/^(Signed-off-by|Cc): (([^>]*|"(?:[^\"\\]|\\.)*")>?)/i) {
I don't think that should block the patch inclusion, but it may be worth considering.
Anyway, thanks for the patch!
Somehow this fell off the radar. So your reviewed-by: and then we'll cook this in 'next' for a while?
Thanks.
Re: [PATCH v2] send-email: only allow one address per body tag
Quoted 26 lines · Junio C Hamano
Quoted 24 lines · Matthieu Moy
git-send-email.perl foreach my $t (@files) { · line 1566
Quoted 8 lines · Johan Hovold
git-send-email.perl +1
-1@@ -1563,7 +1563,7 @@ foreach my $t (@files) { # Now parse the message body while(<$fh>) { $message .= $_; - if (/^(Signed-off-by|Cc): (.*)$/i) { + if (/^(Signed-off-by|Cc): ([^>]*>?)/i) {I think this is acceptable, but this doesn't work with trailers like
Cc "Some > Body" <[email protected]>A proper management of this kind of weird address should be doable by reusing the regexp parsing "..." in
parse_mailbox:my $re_quote = qr/"(?:[^\"\\]|\\.)*"/;So the final regex would look like
if (/^(Signed-off-by|Cc): (([^>]*|"(?:[^\"\\]|\\.)*")>?)/i) {
Quoted 7 lines
Quoted 4 lines
I don't think that should block the patch inclusion, but it may be worth considering.
Anyway, thanks for the patch!
Somehow this fell off the radar. So your reviewed-by: and then we'll cook this in 'next' for a while?
OK.
Signature · 2 lines
Matthieu Moy http://www-verimag.imag.fr/~moy/