Junio C Hamano <gitster@xxxxxxxxx> writes:
> Matthieu Moy <git@xxxxxxxxxxxxxxx> writes:
>
>> +sub strip_garbage_one_address {
>> + my ($addr) = @_;
>> + chomp $addr;
>> + if ($addr =~ /^(("[^"]*"|[^"<]*)? *<[^>]*>).*/) {
>> + # "Foo Bar" <foobar@xxxxxxxxxxx> [possibly garbage here]
>> + # Foo Bar <foobar@xxxxxxxxxxx> [possibly garbage here]
>> + return $1;
>> + }
>> + if ($addr =~ /^(<[^>]*>).*/) {
>> + # <foo@xxxxxxxxxxx> [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@xxxxxxxxxxx> [possibly garbage here]
+ # Foo Bar <foobar@xxxxxxxxxxx> [possibly garbage here]
+ # <foo@xxxxxxxxxxx> [possibly garbage here]
+ return $1;
+ }
> 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@xxxxxxxxxxx>
> So, from that point of view, I, with devil's advocate hat on, wonder
> why we are not saying
>
> "Cc: s@xxxxx # cruft"? Use "Cc: <s@xxxxx> # 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/
