Re: body-CC-comment regression

From: Johan Hovold <johan@kernel.org>
Date: Feb 17, 2017, 17:30 UTC
Message-ID: <20170217173004.GF2625@localhost>
In-Reply-To: <vpq4lzs7o0s.fsf@anie.imag.fr>

On Fri, Feb 17, 2017 at 05:58:11PM +0100, Matthieu Moy wrote:

Show 14 quoted lines

> > On Fri, Feb 17, 2017 at 02:16:42PM +0100, Matthieu Moy wrote:
> >> Johan Hovold <johan@kernel.org> writes:
> >
> >> 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.
> 
> 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.

Show 10 quoted lines

> If I had a time machine, I'd probably go back then and forbid multiple
> addresses there, but ...
> 
> > 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.

Show 13 quoted lines

> >> 1) Stop calling Mail::Address even if available.[...]
> >
> > Right, that sounds like the right thing to do regardless.
> >
> >> 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.

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.

Show 5 quoted 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 <johan@kernel.org>
	...

	Cc: alpha <test1@a.com>, Johan Hovold <johan@kernel.org>

results in

	sc = alpha <test1@a.com>, Johan Hovold <johan@kernel.org>
	sender = Johan Hovold <johan@kernel.org>

so that --suppress-cc=self is not honoured.

Thanks, Johan