Johan Hovold [off-list ref] writes:
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.
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):
--- a/perl/Git.pm
+++ b/perl/Git.pm
@@ -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?
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
On Fri, Feb 17, 2017 at 02:16:42PM +0100, Matthieu Moy wrote:
Johan Hovold [off-list ref] writes:
quoted
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.
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.
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
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.
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.
Right, that sounds like the right thing to do regardless.
quoted hunk
2) Modify our in-house parser to discard garbage after the >. The patch
should look like (untested):
--- a/perl/Git.pm
+++ b/perl/Git.pm
@@ -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?
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
On Fri, Feb 17, 2017 at 5:16 AM, Matthieu Moy
[off-list ref] wrote:
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