Re: [RFC PATCH 1/2] send-email: fix garbage removal after address

From: Matthieu Moy <git@matthieu-moy.fr>
Date: Aug 25, 2017, 09:11 UTC
Message-ID: <vpq378g107m.fsf@anie.imag.fr>
In-Reply-To: <xmqqk21svh9o.fsf@gitster.mtv.corp.google.com>

Junio C Hamano <gitster@pobox.com> writes:

Show 17 quoted lines

> Matthieu Moy <git@matthieu-moy.fr> writes:
>
>> +sub strip_garbage_one_address {
>> +	my ($addr) = @_;
>> +	chomp $addr;
>> +	if ($addr =~ /^(("[^"]*"|[^"<]*)? *<[^>]*>).*/) {
>> +		# "Foo Bar" <foobar@example.com> [possibly garbage here]
>> +		# Foo Bar <foobar@example.com> [possibly garbage here]
>> +		return $1;
>> +	}
>> +	if ($addr =~ /^(<[^>]*>).*/) {
>> +		# <foo@example.com> [possibly garbage here]
>> +		# if garbage contains other addresses, they are ignored.
>> +		return $1;
>> +	}
>
> Isn't this already covered by the first one,

Oops, indeed. I just removed the second "if" (and added the appropriate comment to the first):

+       if ($addr =~ /^(("[^"]*"|[^"<]*)? *<[^>]*>).*/) {
+               # "Foo Bar" <foobar@example.com> [possibly garbage here]
+               # Foo Bar <foobar@example.com> [possibly garbage here]
+               # <foo@example.com> [possibly garbage here]
+               return $1;
+       }

Show 5 quoted lines

> By the way, these three regexps smell like they were written
> specifically to cover three cases you care about (perhaps the ones
> in your proposed log message), but what will be our response when
> somebody else comes next time to us and says that their favourite
> formatting of "Cc:" line is not covered by these rules?

Well, actually the last one covers essentially everything. Just stop at the first space, #, ',' or '"'. The first case is here to allow putting a name in front of the address, which is something we've already allowed and sounds reasonable from the user point of view.

OTOH, I didn't bother with real corner-cases like

  Cc: "Foo \"bar\"" <foobar@example.com>

Show 7 quoted lines

> So, from that point of view, I, with devil's advocate hat on, wonder
> why we are not saying
>
> 	"Cc: s@k.org # cruft"?  Use "Cc: <s@k.org> # cruft" instead
> 	and you'd be fine.
>
> right now, without this patch.

I would agree if the broken case were an exotic one. But a plain adress is really the simplest use-case I can think of, so it's hard to say "don't do that" when we should say "sorry, we should obviously have thought about this use-case".

-- 
Matthieu Moy
https://matthieu-moy.fr/