Re: body-CC-comment regression

[Johan Hovold <johan@kernel.org>]

On Thu, Feb 16, 2017 at 07:16:57PM +0100, Matthieu Moy wrote:
> Johan Hovold <johan@kernel.org> writes:
> 
> > 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: <stable@vger.kernel.org>	# 4.4
> >
> > was now parsed as
> >
> > 	"stable@vger.kernel.org#4.4"
> >
> > which resulted in
> >
> > 	Died at /usr/libexec/git-core/git-send-email line 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:
> 
> > 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" <stable@vger.kernel.org>"
> >
> > This example is based on the example from
> > Documentation/process/stable-kernel-rules.rst:
> >
> > 	Cc: <stable@vger.kernel.org> # 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?
> 
> 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: <foo@example.com> # , <boz@example.com>
> 
> 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.

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.

> 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 <johan@kernel.org>
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:

CC: "# 3 . 3 . x : 1b9508f : sched : Rate-limit newidle" <stable@vger.kernel.org>

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).

Fixes: b1c8a11c8024 ("send-email: allow multiple emails using --cc, --to
and --bcc")
Fixes: e3fdbcc8e164 ("parse_mailboxes: accept extra text after <...>
address")
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 git-send-email.perl | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index 068d60b3e698..eea0a517f71b 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -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;
-- 
2.11.1