Re: body-CC-comment regression
From: Johan Hovold <johan@kernel.org>
Date: 2017-02-17 16:42:51
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 ↗ jump to 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