Failure to extra stable@vger.kernel.org addresses

Failure to extra stable@vger.kernel.org addresses

[Felipe Balbi]

[-- Attachment #1.1: Type: text/plain, Size: 2492 bytes --]

Hi guys,

for whatever reason my git has started acting up with
stable@vger.kernel.org addresses. It doesn't manage to extract a valid
adress from the string:

 Cc: <stable@vger.kernel.org> # v3.4 v3.5 v3.6

Removing the comment at the end of the line makes things work again. I
do remember, however, seeing this working since few weeks back I sent a
mail to stable (in fact the same one I'm using to test), so this could
be related to some perl updates, who knows ?!?

Anyway, here's output of git-send-email:

> $ git send-email --to linux-usb@vger.kernel.org ../linux/0001-usb-dwc3-gadget-fix-endpoint-always-busy-bug.diff --dry-run
> ../linux/0001-usb-dwc3-gadget-fix-endpoint-always-busy-bug.diff
> (mbox) Adding cc: Felipe Balbi <balbi@ti.com> from line 'From: Felipe Balbi <balbi@ti.com>'
> (body) Adding cc: <stable@vger.kernel.org> #v3.4 v3.5 v3.6 from line 'Cc: <stable@vger.kernel.org> #v3.4 v3.5 v3.6'
> (body) Adding cc: Felipe Balbi <balbi@ti.com> from line 'Signed-off-by: Felipe Balbi <balbi@ti.com>'
> Use of uninitialized value $cc in string eq at /usr/libexec/git-core/git-send-email line 997.
> Use of uninitialized value $cc in quotemeta at /usr/libexec/git-core/git-send-email line 997.
> W: unable to extract a valid address from: <stable@vger.kernel.org> #v3.4 v3.5 v3.6
> W: unable to extract a valid address from: <stable@vger.kernel.org> #v3.4 v3.5 v3.6
> Dry-OK. Log says:
> Sendmail: /usr/bin/msmtp -i linux-usb@vger.kernel.org balbi@ti.com
> From: Felipe Balbi <balbi@ti.com>
> To: linux-usb@vger.kernel.org
> Cc: Felipe Balbi <balbi@ti.com>
> Subject: [PATCH] usb: dwc3: gadget: fix 'endpoint always busy' bug
> Date: Mon, 19 Nov 2012 11:54:16 +0200
> Message-Id: <1353318856-14987-1-git-send-email-balbi@ti.com>
> X-Mailer: git-send-email 1.8.0
> 
> Result: OK

$ perl --version

This is perl 5, version 14, subversion 2 (v5.14.2) built for x86_64-linux-gnu-thread-multi
(with 72 registered patches, see perl -V for more detail)

Copyright 1987-2011, Larry Wall

Perl may be copied only under the terms of either the Artistic License or the
GNU General Public License, which may be found in the Perl 5 source kit.

Complete documentation for Perl, including FAQ lists, should be found on
this system using "man perl" or "perldoc perl".  If you have access to the
Internet, point your browser at http://www.perl.org/, the Perl Home Page.

And attached you can find the patch file which I'm using

-- 
balbi

[-- Attachment #1.2: 0001-usb-dwc3-gadget-fix-endpoint-always-busy-bug.diff --]
[-- Type: text/x-diff, Size: 1363 bytes --]

From 041d81f493d90c940ec41f0ec98bc7c4f2fba431 Mon Sep 17 00:00:00 2001
From: Felipe Balbi <balbi@ti.com>
Date: Thu, 4 Oct 2012 11:58:00 +0300
Subject: [PATCH] usb: dwc3: gadget: fix 'endpoint always busy' bug

If a USB transfer has already been started, meaning
we have already issued StartTransfer command to that
particular endpoint, DWC3_EP_BUSY flag has also
already been set.

When we try to cancel this transfer which is already
in controller's cache, we will not receive XferComplete
event and we must clear DWC3_EP_BUSY in order to allow
subsequent requests to be properly started.

The best place to clear that flag is right after issuing
DWC3_DEPCMD_ENDTRANSFER.

Cc: <stable@vger.kernel.org> #v3.4 v3.5 v3.6
Reported-by: Moiz Sonasath <m-sonasath@ti.com>
Signed-off-by: Felipe Balbi <balbi@ti.com>
---
 drivers/usb/dwc3/gadget.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index c9e729a..7b7dedd 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -1904,7 +1904,7 @@ static void dwc3_stop_active_transfer(struct dwc3 *dwc, u32 epnum)
 	ret = dwc3_send_gadget_ep_cmd(dwc, dep->number, cmd, &params);
 	WARN_ON_ONCE(ret);
 	dep->resource_index = 0;
-
+	dep->flags &= ~DWC3_EP_BUSY;
 	udelay(100);
 }
 
-- 
1.8.0


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: Failure to extra stable@vger.kernel.org addresses

[Krzysztof Mazur]

On Mon, Nov 19, 2012 at 11:57:47AM +0200, Felipe Balbi wrote:
> Hi guys,
> 
> for whatever reason my git has started acting up with
> stable@vger.kernel.org addresses. It doesn't manage to extract a valid
> adress from the string:
> 
>  Cc: <stable@vger.kernel.org> # v3.4 v3.5 v3.6
> 
> Removing the comment at the end of the line makes things work again. I
> do remember, however, seeing this working since few weeks back I sent a
> mail to stable (in fact the same one I'm using to test), so this could
> be related to some perl updates, who knows ?!?

You probably just installed Email::Valid package.

The current git-send-email works a little better and just prints an error:

W: unable to extract a valid address from: <stable@vger.kernel.org> #v3.4 v3.5 v3.6


This patch should fix the problem, now after <email> any garbage is
removed while extracting address.

diff --git a/git-send-email.perl b/git-send-email.perl
index 5a7c29d..bb659da 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -828,7 +828,7 @@ sub extract_valid_address {
 	# check for a local address:
 	return $address if ($address =~ /^($local_part_regexp)$/);
 
-	$address =~ s/^\s*<(.*)>\s*$/$1/;
+	$address =~ s/^\s*<(.*)>.*$/$1/;
 	if ($have_email_valid) {
 		return scalar Email::Valid->address($address);
 	} else {

Krzysiek

Re: Failure to extra stable@vger.kernel.org addresses

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1152 bytes --]

Hi,

On Mon, Nov 19, 2012 at 04:18:45PM +0100, Krzysztof Mazur wrote:
> On Mon, Nov 19, 2012 at 11:57:47AM +0200, Felipe Balbi wrote:
> > Hi guys,
> > 
> > for whatever reason my git has started acting up with
> > stable@vger.kernel.org addresses. It doesn't manage to extract a valid
> > adress from the string:
> > 
> >  Cc: <stable@vger.kernel.org> # v3.4 v3.5 v3.6
> > 
> > Removing the comment at the end of the line makes things work again. I
> > do remember, however, seeing this working since few weeks back I sent a
> > mail to stable (in fact the same one I'm using to test), so this could
> > be related to some perl updates, who knows ?!?
> 
> You probably just installed Email::Valid package.
> 
> The current git-send-email works a little better and just prints an error:
> 
> W: unable to extract a valid address from: <stable@vger.kernel.org> #v3.4 v3.5 v3.6
> 
> 
> This patch should fix the problem, now after <email> any garbage is
> removed while extracting address.

worked like a charm. When you send as a proper patch, you can add my:

Tested-by: Felipe Balbi <balbi@ti.com>

thanks

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: Failure to extra stable@vger.kernel.org addresses

[Junio C Hamano]

Krzysztof Mazur <krzysiek@podlesie.net> writes:

> On Mon, Nov 19, 2012 at 11:57:47AM +0200, Felipe Balbi wrote:
>> Hi guys,
>> 
>> for whatever reason my git has started acting up with
>> stable@vger.kernel.org addresses. It doesn't manage to extract a valid
>> adress from the string:
>> 
>>  Cc: <stable@vger.kernel.org> # v3.4 v3.5 v3.6
>> 
>> Removing the comment at the end of the line makes things work again. I
>> do remember, however, seeing this working since few weeks back I sent a
>> mail to stable (in fact the same one I'm using to test), so this could
>> be related to some perl updates, who knows ?!?
>
> You probably just installed Email::Valid package.
>
> The current git-send-email works a little better and just prints an error:
>
> W: unable to extract a valid address from: <stable@vger.kernel.org> #v3.4 v3.5 v3.6
>
>
> This patch should fix the problem, now after <email> any garbage is
> removed while extracting address.
>
> diff --git a/git-send-email.perl b/git-send-email.perl
> index 5a7c29d..bb659da 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -828,7 +828,7 @@ sub extract_valid_address {
>  	# check for a local address:
>  	return $address if ($address =~ /^($local_part_regexp)$/);
>  
> -	$address =~ s/^\s*<(.*)>\s*$/$1/;
> +	$address =~ s/^\s*<(.*)>.*$/$1/;
>  	if ($have_email_valid) {
>  		return scalar Email::Valid->address($address);
>  	} else {

Given that the problematic line

	Stable Kernel Maintainance Track <stable@vger.kernel.org> # vX.Y

is not even a valid e-mail address, doesn't this new logic belong to
sanitize_address() conceptually?

Re: Failure to extra stable@vger.kernel.org addresses

[Felipe Contreras]

On Mon, Nov 19, 2012 at 8:27 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Krzysztof Mazur <krzysiek@podlesie.net> writes:
>
>> On Mon, Nov 19, 2012 at 11:57:47AM +0200, Felipe Balbi wrote:
>>> Hi guys,
>>>
>>> for whatever reason my git has started acting up with
>>> stable@vger.kernel.org addresses. It doesn't manage to extract a valid
>>> adress from the string:
>>>
>>>  Cc: <stable@vger.kernel.org> # v3.4 v3.5 v3.6
>>>
>>> Removing the comment at the end of the line makes things work again. I
>>> do remember, however, seeing this working since few weeks back I sent a
>>> mail to stable (in fact the same one I'm using to test), so this could
>>> be related to some perl updates, who knows ?!?
>>
>> You probably just installed Email::Valid package.
>>
>> The current git-send-email works a little better and just prints an error:
>>
>> W: unable to extract a valid address from: <stable@vger.kernel.org> #v3.4 v3.5 v3.6
>>
>>
>> This patch should fix the problem, now after <email> any garbage is
>> removed while extracting address.
>>
>> diff --git a/git-send-email.perl b/git-send-email.perl
>> index 5a7c29d..bb659da 100755
>> --- a/git-send-email.perl
>> +++ b/git-send-email.perl
>> @@ -828,7 +828,7 @@ sub extract_valid_address {
>>       # check for a local address:
>>       return $address if ($address =~ /^($local_part_regexp)$/);
>>
>> -     $address =~ s/^\s*<(.*)>\s*$/$1/;
>> +     $address =~ s/^\s*<(.*)>.*$/$1/;
>>       if ($have_email_valid) {
>>               return scalar Email::Valid->address($address);
>>       } else {
>
> Given that the problematic line
>
>         Stable Kernel Maintainance Track <stable@vger.kernel.org> # vX.Y
>
> is not even a valid e-mail address, doesn't this new logic belong to
> sanitize_address() conceptually?

That would be great, it would also help the cc-cmd stuff. The
get_maintainer.pl patch from the Linux kernel outputs something like:

David Airlie <airlied@linux.ie> (maintainer:DRM DRIVERS)
Ben Skeggs <bskeggs@redhat.com>
(commit_signer:17/19=89%,commit_signer:43/46=93%)
Maxim Levitsky <maximlevitsky@gmail.com> (commit_signer:3/19=16%)
Greg Kroah-Hartman <gregkh@linuxfoundation.org> (commit_signer:2/19=11%)
Dave Airlie <airlied@redhat.com> (commit_signer:2/19=11%,commit_signer:3/46=7%)
Alex Deucher <alexander.deucher@amd.com> (commit_signer:1/19=5%)
dri-devel@lists.freedesktop.org (open list:DRM DRIVERS)
linux-kernel@vger.kernel.org (open list)

-- 
Felipe Contreras

Re: Failure to extra stable@vger.kernel.org addresses

[Krzysztof Mazur]

On Mon, Nov 19, 2012 at 11:27:45AM -0800, Junio C Hamano wrote:
> Given that the problematic line
> 
> 	Stable Kernel Maintainance Track <stable@vger.kernel.org> # vX.Y
> 
> is not even a valid e-mail address, doesn't this new logic belong to
> sanitize_address() conceptually?

Yes, it's much better to do it in the sanitize_address().

Felipe, may you check it?

Krzysiek
-- >8 --
Subject: [PATCH] git-send-email: remove garbage after email address

In some cases it's very useful to add some additional information
after email in Cc-list, for instance:

"Cc: Stable kernel <stable@vger.kernel.org> #v3.4 v3.5 v3.6"

Currently the git refuses to add such invalid email to Cc-list,
when the Email::Valid perl module is available or just uses whole line
as the email address.

Now in sanitize_address() everything after the email address is
removed, so the resulting line is correct email address and Email::Valid
validates it correctly.

Signed-off-by: Krzysztof Mazur <krzysiek@podlesie.net>
---
 git-send-email.perl | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/git-send-email.perl b/git-send-email.perl
index 5a7c29d..9840d0a 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -924,6 +924,10 @@ sub quote_subject {
 # use the simplest quoting being able to handle the recipient
 sub sanitize_address {
 	my ($recipient) = @_;
+
+	# remove garbage after email address
+	$recipient =~ s/(.*>).*$/$1/;
+
 	my ($recipient_name, $recipient_addr) = ($recipient =~ /^(.*?)\s*(<.*)/);
 
 	if (not $recipient_name) {
-- 
1.8.0.283.gc57d856

Re: Failure to extra stable@vger.kernel.org addresses

[Felipe Contreras]

On Mon, Nov 19, 2012 at 11:58 PM, Krzysztof Mazur <krzysiek@podlesie.net> wrote:

> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -924,6 +924,10 @@ sub quote_subject {
>  # use the simplest quoting being able to handle the recipient
>  sub sanitize_address {
>         my ($recipient) = @_;
> +
> +       # remove garbage after email address
> +       $recipient =~ s/(.*>).*$/$1/;
> +

Looks fine, but I would do s/(.*?>)(.*)$/$1/, so that 'test
<foo@bar.com> <#comment>' gets the second comment removed.

Cheers.

-- 
Felipe Contreras

Re: Failure to extra stable@vger.kernel.org addresses

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> On Mon, Nov 19, 2012 at 11:58 PM, Krzysztof Mazur <krzysiek@podlesie.net> wrote:
>
>> --- a/git-send-email.perl
>> +++ b/git-send-email.perl
>> @@ -924,6 +924,10 @@ sub quote_subject {
>>  # use the simplest quoting being able to handle the recipient
>>  sub sanitize_address {
>>         my ($recipient) = @_;
>> +
>> +       # remove garbage after email address
>> +       $recipient =~ s/(.*>).*$/$1/;
>> +
>
> Looks fine, but I would do s/(.*?>)(.*)$/$1/, so that 'test
> <foo@bar.com> <#comment>' gets the second comment removed.

Yeah, but do you need to capture the second group?  IOW, like
"s/(.*?>).*$/$1/" perhaps?

Re: Failure to extra stable@vger.kernel.org addresses

[Junio C Hamano]

Krzysztof Mazur <krzysiek@podlesie.net> writes:

> On Mon, Nov 19, 2012 at 11:27:45AM -0800, Junio C Hamano wrote:
>> Given that the problematic line
>> 
>> 	Stable Kernel Maintainance Track <stable@vger.kernel.org> # vX.Y
>> 
>> is not even a valid e-mail address, doesn't this new logic belong to
>> sanitize_address() conceptually?
>
> Yes, it's much better to do it in the sanitize_address().

Note that I did not check that all the addresses that are handled by
extract-valid-address came through sanitize-address function, so
unlike your original patch, this change alone may still pass some
garbage to Email::Valid->address().  I tend to think that is a
progress; we should make sure all the addresses are sanitized before
using them for sending messages out.

Re: Failure to extra stable@vger.kernel.org addresses

[Krzysztof Mazur]

On Mon, Nov 19, 2012 at 04:00:09PM -0800, Junio C Hamano wrote:
> Krzysztof Mazur <krzysiek@podlesie.net> writes:
> 
> > On Mon, Nov 19, 2012 at 11:27:45AM -0800, Junio C Hamano wrote:
> >> Given that the problematic line
> >> 
> >> 	Stable Kernel Maintainance Track <stable@vger.kernel.org> # vX.Y
> >> 
> >> is not even a valid e-mail address, doesn't this new logic belong to
> >> sanitize_address() conceptually?
> >
> > Yes, it's much better to do it in the sanitize_address().
> 
> Note that I did not check that all the addresses that are handled by
> extract-valid-address came through sanitize-address function, so

Before sending that patch, I checked that and tested with and without
Email::Valid.

> unlike your original patch, this change alone may still pass some
> garbage to Email::Valid->address().  I tend to think that is a
> progress; we should make sure all the addresses are sanitized before
> using them for sending messages out.

I will try to check that.

Krzysiek

Re: Failure to extra stable@vger.kernel.org addresses

[Krzysztof Mazur]

On Mon, Nov 19, 2012 at 03:57:36PM -0800, Junio C Hamano wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
> 
> > On Mon, Nov 19, 2012 at 11:58 PM, Krzysztof Mazur <krzysiek@podlesie.net> wrote:
> >
> >> --- a/git-send-email.perl
> >> +++ b/git-send-email.perl
> >> @@ -924,6 +924,10 @@ sub quote_subject {
> >>  # use the simplest quoting being able to handle the recipient
> >>  sub sanitize_address {
> >>         my ($recipient) = @_;
> >> +
> >> +       # remove garbage after email address
> >> +       $recipient =~ s/(.*>).*$/$1/;
> >> +
> >
> > Looks fine, but I would do s/(.*?>)(.*)$/$1/, so that 'test
> > <foo@bar.com> <#comment>' gets the second comment removed.
> 
> Yeah, but do you need to capture the second group?  IOW, like
> "s/(.*?>).*$/$1/" perhaps?

I also thought about removing everything after first ">", but I will
not work for addresses like:

Cc: "foo >" <stable@vger.kernel.org> #v3.4 v3.5 v3.6

What about:

	$recipient =~ s/(.*<[^@]*@[^]]*>).*$/$1/;

or even

diff --git a/git-send-email.perl b/git-send-email.perl
index 9840d0a..b988c57 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -925,8 +925,11 @@ sub quote_subject {
 sub sanitize_address {
 	my ($recipient) = @_;
 
+	my $local_part_regexp = qr/[^<>"\s@]+/;
+	my $domain_regexp = qr/[^.<>"\s@]+(?:\.[^.<>"\s@]+)+/;
+
 	# remove garbage after email address
-	$recipient =~ s/(.*>).*$/$1/;
+	$recipient =~ s/(.*<$local_part_regexp\@$domain_regexp>).*$/$1/;
 
 	my ($recipient_name, $recipient_addr) = ($recipient =~ /^(.*?)\s*(<.*)/);

which uses regex used by 99% accurate version of extract_valid_address().

Krzysiek

Re: Failure to extra stable@vger.kernel.org addresses

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1837 bytes --]

On Mon, Nov 19, 2012 at 11:58:38PM +0100, Krzysztof Mazur wrote:
> On Mon, Nov 19, 2012 at 11:27:45AM -0800, Junio C Hamano wrote:
> > Given that the problematic line
> > 
> > 	Stable Kernel Maintainance Track <stable@vger.kernel.org> # vX.Y
> > 
> > is not even a valid e-mail address, doesn't this new logic belong to
> > sanitize_address() conceptually?
> 
> Yes, it's much better to do it in the sanitize_address().
> 
> Felipe, may you check it?
> 
> Krzysiek
> -- >8 --
> Subject: [PATCH] git-send-email: remove garbage after email address
> 
> In some cases it's very useful to add some additional information
> after email in Cc-list, for instance:
> 
> "Cc: Stable kernel <stable@vger.kernel.org> #v3.4 v3.5 v3.6"
> 
> Currently the git refuses to add such invalid email to Cc-list,
> when the Email::Valid perl module is available or just uses whole line
> as the email address.
> 
> Now in sanitize_address() everything after the email address is
> removed, so the resulting line is correct email address and Email::Valid
> validates it correctly.
> 
> Signed-off-by: Krzysztof Mazur <krzysiek@podlesie.net>

Tested-by: Felipe Balbi <balbi@ti.com>

> ---
>  git-send-email.perl | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/git-send-email.perl b/git-send-email.perl
> index 5a7c29d..9840d0a 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -924,6 +924,10 @@ sub quote_subject {
>  # use the simplest quoting being able to handle the recipient
>  sub sanitize_address {
>  	my ($recipient) = @_;
> +
> +	# remove garbage after email address
> +	$recipient =~ s/(.*>).*$/$1/;
> +
>  	my ($recipient_name, $recipient_addr) = ($recipient =~ /^(.*?)\s*(<.*)/);
>  
>  	if (not $recipient_name) {
> -- 
> 1.8.0.283.gc57d856
> 

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: Failure to extra stable@vger.kernel.org addresses

[Krzysztof Mazur]

On Tue, Nov 20, 2012 at 08:31:00AM +0100, Krzysztof Mazur wrote:
> On Mon, Nov 19, 2012 at 03:57:36PM -0800, Junio C Hamano wrote:
> > Felipe Contreras <felipe.contreras@gmail.com> writes:
> > 
> > > On Mon, Nov 19, 2012 at 11:58 PM, Krzysztof Mazur <krzysiek@podlesie.net> wrote:
> > >
> > >> --- a/git-send-email.perl
> > >> +++ b/git-send-email.perl
> > >> @@ -924,6 +924,10 @@ sub quote_subject {
> > >>  # use the simplest quoting being able to handle the recipient
> > >>  sub sanitize_address {
> > >>         my ($recipient) = @_;
> > >> +
> > >> +       # remove garbage after email address
> > >> +       $recipient =~ s/(.*>).*$/$1/;
> > >> +
> > >
> > > Looks fine, but I would do s/(.*?>)(.*)$/$1/, so that 'test
> > > <foo@bar.com> <#comment>' gets the second comment removed.
> > 
> > Yeah, but do you need to capture the second group?  IOW, like
> > "s/(.*?>).*$/$1/" perhaps?
> 
> I also thought about removing everything after first ">", but I will
> not work for addresses like:
> 
> Cc: "foo >" <stable@vger.kernel.org> #v3.4 v3.5 v3.6
> 
> What about:
> 
> 	$recipient =~ s/(.*<[^@]*@[^]]*>).*$/$1/;
> 
> or even
> 
> which uses regex used by 99% accurate version of extract_valid_address().
> 

Of course, as you suggested earier, only the first email address should
be used, so in both cases the first ".*" should be changed to ".*?".
The second version becomes:

diff --git a/git-send-email.perl b/git-send-email.perl
index 9840d0a..dbe520c 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -925,8 +925,11 @@ sub quote_subject {
 sub sanitize_address {
 	my ($recipient) = @_;
 
+	my $local_part_regexp = qr/[^<>"\s@]+/;
+	my $domain_regexp = qr/[^.<>"\s@]+(?:\.[^.<>"\s@]+)+/;
+
 	# remove garbage after email address
-	$recipient =~ s/(.*>).*$/$1/;
+	$recipient =~ s/^(.*?<$local_part_regexp\@$domain_regexp>).*/$1/;
 
 	my ($recipient_name, $recipient_addr) = ($recipient =~ /^(.*?)\s*(<.*)/);
 
Krzysiek

Re: Failure to extra stable@vger.kernel.org addresses

[Felipe Contreras]

On Tue, Nov 20, 2012 at 8:56 AM, Krzysztof Mazur <krzysiek@podlesie.net> wrote:

> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -925,8 +925,11 @@ sub quote_subject {
>  sub sanitize_address {
>         my ($recipient) = @_;
>
> +       my $local_part_regexp = qr/[^<>"\s@]+/;
> +       my $domain_regexp = qr/[^.<>"\s@]+(?:\.[^.<>"\s@]+)+/;
> +
>         # remove garbage after email address
> -       $recipient =~ s/(.*>).*$/$1/;
> +       $recipient =~ s/^(.*?<$local_part_regexp\@$domain_regexp>).*/$1/;

I don't think all that extra complexity is warranted, to me
s/(.*?>)(.*)$/$1/ is just fine.

Cheers.

-- 
Felipe Contreras

Re: Failure to extra stable@vger.kernel.org addresses

[Andreas Ericsson]

On 11/20/2012 11:28 AM, Felipe Contreras wrote:
> On Tue, Nov 20, 2012 at 8:56 AM, Krzysztof Mazur <krzysiek@podlesie.net> wrote:
> 
>> --- a/git-send-email.perl
>> +++ b/git-send-email.perl
>> @@ -925,8 +925,11 @@ sub quote_subject {
>>   sub sanitize_address {
>>          my ($recipient) = @_;
>>
>> +       my $local_part_regexp = qr/[^<>"\s@]+/;
>> +       my $domain_regexp = qr/[^.<>"\s@]+(?:\.[^.<>"\s@]+)+/;
>> +
>>          # remove garbage after email address
>> -       $recipient =~ s/(.*>).*$/$1/;
>> +       $recipient =~ s/^(.*?<$local_part_regexp\@$domain_regexp>).*/$1/;
> 
> I don't think all that extra complexity is warranted, to me
> s/(.*?>)(.*)$/$1/ is just fine.
> 

It's intentionally left without the at-sign so one can send mail to a
local account as well as remote ones. Very nifty when debugging, and
when one wants to preview outgoing emails.

-- 
Andreas Ericsson                   andreas.ericsson@op5.se
OP5 AB                             www.op5.se
Tel: +46 8-230225                  Fax: +46 8-230231

Considering the successes of the wars on alcohol, poverty, drugs and
terror, I think we should give some serious thought to declaring war
on peace.

Re: Failure to extra stable@vger.kernel.org addresses

[Krzysztof Mazur]

On Tue, Nov 20, 2012 at 11:28:39AM +0100, Felipe Contreras wrote:
> On Tue, Nov 20, 2012 at 8:56 AM, Krzysztof Mazur <krzysiek@podlesie.net> wrote:
> 
> > --- a/git-send-email.perl
> > +++ b/git-send-email.perl
> > @@ -925,8 +925,11 @@ sub quote_subject {
> >  sub sanitize_address {
> >         my ($recipient) = @_;
> >
> > +       my $local_part_regexp = qr/[^<>"\s@]+/;
> > +       my $domain_regexp = qr/[^.<>"\s@]+(?:\.[^.<>"\s@]+)+/;
> > +
> >         # remove garbage after email address
> > -       $recipient =~ s/(.*>).*$/$1/;
> > +       $recipient =~ s/^(.*?<$local_part_regexp\@$domain_regexp>).*/$1/;
> 
> I don't think all that extra complexity is warranted, to me
> s/(.*?>)(.*)$/$1/ is just fine.
> 

Yeah, it's a little bit too complex, but "s/(.*?>)(.*)$/$1/"
causes small regression - '>' character is no longer allowed
in "phrase" before "<email address>". Maybe the initial version,
that removes everything after last '>' is better? In this case '>'
is not allowed in garbage after email.

Krzysiek

Re: Failure to extra stable@vger.kernel.org addresses

[Andreas Schwab]

Krzysztof Mazur <krzysiek@podlesie.net> writes:

> On Tue, Nov 20, 2012 at 11:28:39AM +0100, Felipe Contreras wrote:
>> On Tue, Nov 20, 2012 at 8:56 AM, Krzysztof Mazur <krzysiek@podlesie.net> wrote:
>> 
>> > --- a/git-send-email.perl
>> > +++ b/git-send-email.perl
>> > @@ -925,8 +925,11 @@ sub quote_subject {
>> >  sub sanitize_address {
>> >         my ($recipient) = @_;
>> >
>> > +       my $local_part_regexp = qr/[^<>"\s@]+/;
>> > +       my $domain_regexp = qr/[^.<>"\s@]+(?:\.[^.<>"\s@]+)+/;
>> > +
>> >         # remove garbage after email address
>> > -       $recipient =~ s/(.*>).*$/$1/;
>> > +       $recipient =~ s/^(.*?<$local_part_regexp\@$domain_regexp>).*/$1/;
>> 
>> I don't think all that extra complexity is warranted, to me
>> s/(.*?>)(.*)$/$1/ is just fine.
>> 
>
> Yeah, it's a little bit too complex, but "s/(.*?>)(.*)$/$1/"

How about "s/(.*?<[^>]*>).*$/$1/"?  That will still fail on "<foo@bar>"
<foo@bar>, but you'll need a full rfc822 parser to handle the general
case anyway.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: Failure to extra stable@vger.kernel.org addresses

[Krzysztof Mazur]

On Tue, Nov 20, 2012 at 08:58:20PM +0100, Andreas Schwab wrote:
> How about "s/(.*?<[^>]*>).*$/$1/"?  That will still fail on "<foo@bar>"
> <foo@bar>, but you'll need a full rfc822 parser to handle the general
> case anyway.

That will fail also on "<something>" <foo@bar>.


I think it's good compromise between complexity and correctness.

Felipe, may you check, it again? This time the change is trivial.

Andreas, may I add you in Thanks-to?

Thanks,

Krzysiek

-- >8 --
Subject: [PATCH] git-send-email: remove garbage after email address

In some cases it's very useful to add some additional information
after email in Cc-list, for instance:

"Cc: Stable kernel <stable@vger.kernel.org> #v3.4 v3.5 v3.6"

Currently the git refuses to add such invalid email to Cc-list,
when the Email::Valid perl module is available or just uses whole line
as the email address.

Now in sanitize_address() everything after the email address is
removed, so the resulting line is correct email address and Email::Valid
validates it correctly.

To avoid unnecessary complexity this code assumes that in phrase before
email address '<something>' never exists.

Signed-off-by: Krzysztof Mazur <krzysiek@podlesie.net>
---
 git-send-email.perl | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/git-send-email.perl b/git-send-email.perl
index 5a7c29d..157eabc 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -924,6 +924,10 @@ sub quote_subject {
 # use the simplest quoting being able to handle the recipient
 sub sanitize_address {
 	my ($recipient) = @_;
+
+	# remove garbage after email address
+	$recipient =~ s/(.*?<[^>]*>).*$/$1/;
+
 	my ($recipient_name, $recipient_addr) = ($recipient =~ /^(.*?)\s*(<.*)/);
 
 	if (not $recipient_name) {
-- 
1.8.0.283.gc57d856

Re: Failure to extra stable@vger.kernel.org addresses

[Felipe Contreras]

On Tue, Nov 20, 2012 at 10:21 PM, Krzysztof Mazur <krzysiek@podlesie.net> wrote:

> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -924,6 +924,10 @@ sub quote_subject {
>  # use the simplest quoting being able to handle the recipient
>  sub sanitize_address {
>         my ($recipient) = @_;
> +
> +       # remove garbage after email address
> +       $recipient =~ s/(.*?<[^>]*>).*$/$1/;

That won't work for 'foo@bar.com # test'. I think we should abandon
hopes of properly parsing an email address and just do:

$recipient =~ s/(.*?) #.*$/$1/;

Cheers.

-- 
Felipe Contreras

Re: Failure to extra stable@vger.kernel.org addresses

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> On Tue, Nov 20, 2012 at 10:21 PM, Krzysztof Mazur <krzysiek@podlesie.net> wrote:
>
>> --- a/git-send-email.perl
>> +++ b/git-send-email.perl
>> @@ -924,6 +924,10 @@ sub quote_subject {
>>  # use the simplest quoting being able to handle the recipient
>>  sub sanitize_address {
>>         my ($recipient) = @_;
>> +
>> +       # remove garbage after email address
>> +       $recipient =~ s/(.*?<[^>]*>).*$/$1/;
>
> That won't work for 'foo@bar.com # test'. I think we should abandon
> hopes of properly parsing an email address and just do:
>
> $recipient =~ s/(.*?) #.*$/$1/;

We should probably fix the tools that generate these bogus
non-addresses first.  What's wrong with

	Cc: stable kernel (v3.5 v3.6 v3.7) <stable@vger.kernel.org>

which should be OK?

Also I suspect that this should be also deemed valid:

	Cc: stable@vger.kernel.org (Stable kernel - v3.5 v3.6 v3.7)

Re: Failure to extra stable@vger.kernel.org addresses

[Krzysztof Mazur]

On Tue, Nov 20, 2012 at 02:30:02PM -0800, Junio C Hamano wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
> 
> > On Tue, Nov 20, 2012 at 10:21 PM, Krzysztof Mazur <krzysiek@podlesie.net> wrote:
> >
> >> --- a/git-send-email.perl
> >> +++ b/git-send-email.perl
> >> @@ -924,6 +924,10 @@ sub quote_subject {
> >>  # use the simplest quoting being able to handle the recipient
> >>  sub sanitize_address {
> >>         my ($recipient) = @_;
> >> +
> >> +       # remove garbage after email address
> >> +       $recipient =~ s/(.*?<[^>]*>).*$/$1/;
> >
> > That won't work for 'foo@bar.com # test'. I think we should abandon
> > hopes of properly parsing an email address and just do:
> >
> > $recipient =~ s/(.*?) #.*$/$1/;
> 
> We should probably fix the tools that generate these bogus
> non-addresses first.  What's wrong with
> 
> 	Cc: stable kernel (v3.5 v3.6 v3.7) <stable@vger.kernel.org>
> 
> which should be OK?
> 
> Also I suspect that this should be also deemed valid:
> 
> 	Cc: stable@vger.kernel.org (Stable kernel - v3.5 v3.6 v3.7)

So maybe we should just use the original regex:

$recipient =~ s/(.*>).*$/$1/

which does not add regression for valid addresses, and just fails
in some rare cases when '>' is used in garbage. It was sufficient
for original issue reported by, and tested by Felipe.

The problem with '>' would be fixed in separate patch. The same
problem exits for invalid address generated by --cc-cmd
(see [PATCH] git-send-email: don't return undefined value in
extract_valid_address()). We would report an error in both cases,
as suggested by Junio.

Krzysiek

Re: Failure to extra stable@vger.kernel.org addresses

[Junio C Hamano]

Krzysztof Mazur <krzysiek@podlesie.net> writes:

> On Tue, Nov 20, 2012 at 02:30:02PM -0800, Junio C Hamano wrote:
>
>> We should probably fix the tools that generate these bogus
>> non-addresses first.  What's wrong with
>> 
>> 	Cc: stable kernel (v3.5 v3.6 v3.7) <stable@vger.kernel.org>
>> 
>> which should be OK?
>> 
>> Also I suspect that this should be also deemed valid:
>> 
>> 	Cc: stable@vger.kernel.org (Stable kernel - v3.5 v3.6 v3.7)
>
> So maybe we should just use the original regex:
>
> $recipient =~ s/(.*>).*$/$1/
>
> which does not add regression for valid addresses, and just fails
> in some rare cases when '>' is used in garbage. It was sufficient
> for original issue reported by, and tested by Felipe.
>
> The problem with '>' would be fixed in separate patch. The same
> problem exits for invalid address generated by --cc-cmd
> (see [PATCH] git-send-email: don't return undefined value in
> extract_valid_address()). We would report an error in both cases,
> as suggested by Junio.

OK, sounds like a plan.

[PATCH 1/5] git-send-email: remove garbage after email address

[Krzysztof Mazur]

In some cases it's very useful to add some additional information
after email in Cc-list, for instance:

"Cc: Stable kernel <stable@vger.kernel.org> #v3.4 v3.5 v3.6"

Currently the git refuses to add such invalid email to Cc-list,
when the Email::Valid perl module is available or just uses whole line
as the email address.

Now in sanitize_address() everything after the email address is
removed, so the resulting line is correct email address and Email::Valid
validates it correctly.

Signed-off-by: Krzysztof Mazur <krzysiek@podlesie.net>
Tested-by: Felipe Balbi <balbi@ti.com>
---
 git-send-email.perl | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/git-send-email.perl b/git-send-email.perl
index 5a7c29d..9840d0a 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -924,6 +924,10 @@ sub quote_subject {
 # use the simplest quoting being able to handle the recipient
 sub sanitize_address {
 	my ($recipient) = @_;
+
+	# remove garbage after email address
+	$recipient =~ s/(.*>).*$/$1/;
+
 	my ($recipient_name, $recipient_addr) = ($recipient =~ /^(.*?)\s*(<.*)/);
 
 	if (not $recipient_name) {
-- 
1.8.0.393.gcc9701d

[PATCH 2/5] git-send-email: fix fallback code in extract_valid_address()

[Krzysztof Mazur]

In the fallback check, used when Email::Valid is not available, the
extract_valid_address() uses $1 without checking for success of matching
regex. The $1 variable may still hold the result of previous match,
which is the address when email address was in '<>' or be undefined
otherwise.

Now if match fails undefined value is always returned to indicate error.
The same value is used by Email::Valid->address() in that case.

Previously 'foo@bar' address was rejected by Email::Valid and fallback,
but '<foo@bar>' was rejected by Email::Valid, but accepted by fallback.

Signed-off-by: Krzysztof Mazur <krzysiek@podlesie.net>
---
 git-send-email.perl | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index 9840d0a..356f99d 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -831,12 +831,12 @@ sub extract_valid_address {
 	$address =~ s/^\s*<(.*)>\s*$/$1/;
 	if ($have_email_valid) {
 		return scalar Email::Valid->address($address);
-	} else {
-		# less robust/correct than the monster regexp in Email::Valid,
-		# but still does a 99% job, and one less dependency
-		$address =~ /($local_part_regexp\@$domain_regexp)/;
-		return $1;
 	}
+
+	# less robust/correct than the monster regexp in Email::Valid,
+	# but still does a 99% job, and one less dependency
+	return $1 if $address =~ /($local_part_regexp\@$domain_regexp)/;
+	return undef;
 }
 
 # Usually don't need to change anything below here.
-- 
1.8.0.393.gcc9701d

[PATCH 3/5] git-send-email: remove invalid addresses earlier

[Krzysztof Mazur]

Some addresses are passed twice to unique_email_list() and invalid addresses
may be reported twice per send_message. Now we warn about them earlier
and we also remove invalid addresses.

This also removes using of undefined values for string comparison
for invalid addresses in cc list processing.

Signed-off-by: Krzysztof Mazur <krzysiek@podlesie.net>
---
 git-send-email.perl | 52 +++++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 39 insertions(+), 13 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index 356f99d..5056fdc 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -786,9 +786,11 @@ sub expand_one_alias {
 }
 
 @initial_to = expand_aliases(@initial_to);
-@initial_to = (map { sanitize_address($_) } @initial_to);
+@initial_to = validate_address_list(sanitize_address_list(@initial_to));
 @initial_cc = expand_aliases(@initial_cc);
+@initial_cc = validate_address_list(sanitize_address_list(@initial_cc));
 @bcclist = expand_aliases(@bcclist);
+@bcclist = validate_address_list(sanitize_address_list(@bcclist));
 
 if ($thread && !defined $initial_reply_to && $prompting) {
 	$initial_reply_to = ask(
@@ -839,6 +841,28 @@ sub extract_valid_address {
 	return undef;
 }
 
+sub extract_valid_address_or_die {
+	my $address = shift;
+	$address = extract_valid_address($address);
+	die "error: unable to extract a valid address from: $address\n"
+		if !$address;
+	return $address;
+}
+
+sub validate_address {
+	my $address = shift;
+	if (!extract_valid_address($address)) {
+		print STDERR "W: unable to extract a valid address from: $address\n";
+		return undef;
+	}
+	return $address;
+}
+
+sub validate_address_list {
+	return (grep { defined $_ }
+		map { validate_address($_) } @_);
+}
+
 # Usually don't need to change anything below here.
 
 # we make a "fake" message id by taking the current number
@@ -955,6 +979,10 @@ sub sanitize_address {
 
 }
 
+sub sanitize_address_list {
+	return (map { sanitize_address($_) } @_);
+}
+
 # Returns the local Fully Qualified Domain Name (FQDN) if available.
 #
 # Tightly configured MTAa require that a caller sends a real DNS
@@ -1017,14 +1045,13 @@ sub maildomain {
 
 sub send_message {
 	my @recipients = unique_email_list(@to);
-	@cc = (grep { my $cc = extract_valid_address($_);
+	@cc = (grep { my $cc = extract_valid_address_or_die($_);
 		      not grep { $cc eq $_ || $_ =~ /<\Q${cc}\E>$/ } @recipients
 		    }
-	       map { sanitize_address($_) }
 	       @cc);
 	my $to = join (",\n\t", @recipients);
 	@recipients = unique_email_list(@recipients,@cc,@bcclist);
-	@recipients = (map { extract_valid_address($_) } @recipients);
+	@recipients = (map { extract_valid_address_or_die($_) } @recipients);
 	my $date = format_2822_time($time++);
 	my $gitversion = '@@GIT_VERSION@@';
 	if ($gitversion =~ m/..GIT_VERSION../) {
@@ -1267,7 +1294,7 @@ foreach my $t (@files) {
 				foreach my $addr (parse_address_line($1)) {
 					printf("(mbox) Adding to: %s from line '%s'\n",
 						$addr, $_) unless $quiet;
-					push @to, sanitize_address($addr);
+					push @to, $addr;
 				}
 			}
 			elsif (/^Cc:\s+(.*)$/) {
@@ -1376,6 +1403,9 @@ foreach my $t (@files) {
 		($confirm =~ /^(?:auto|compose)$/ && $compose && $message_num == 1));
 	$needs_confirm = "inform" if ($needs_confirm && $confirm_unconfigured && @cc);
 
+	@to = validate_address_list(sanitize_address_list(@to));
+	@cc = validate_address_list(sanitize_address_list(@cc));
+
 	@to = (@initial_to, @to);
 	@cc = (@initial_cc, @cc);
 
@@ -1431,14 +1461,10 @@ sub unique_email_list {
 	my @emails;
 
 	foreach my $entry (@_) {
-		if (my $clean = extract_valid_address($entry)) {
-			$seen{$clean} ||= 0;
-			next if $seen{$clean}++;
-			push @emails, $entry;
-		} else {
-			print STDERR "W: unable to extract a valid address",
-					" from: $entry\n";
-		}
+		my $clean = extract_valid_address_or_die($entry))
+		$seen{$clean} ||= 0;
+		next if $seen{$clean}++;
+		push @emails, $entry;
 	}
 	return @emails;
 }
-- 
1.8.0.393.gcc9701d

[PATCH 4/5] git-send-email: ask what to do with an invalid email address

[Krzysztof Mazur]

We used to warn about invalid emails and just drop them. Such warnings
can be unnoticed by user or noticed after sending email when we are not
giving the "final sanity check [Y/n]?"

Now we quit by default.

Signed-off-by: Krzysztof Mazur <krzysiek@podlesie.net>
Suggested-by: Junio C Hamano <gitster@pobox.com>
---
 git-send-email.perl | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index 5056fdc..d42dca2 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -852,8 +852,16 @@ sub extract_valid_address_or_die {
 sub validate_address {
 	my $address = shift;
 	if (!extract_valid_address($address)) {
-		print STDERR "W: unable to extract a valid address from: $address\n";
-		return undef;
+		print STDERR "error: unable to extract a valid address from: $address\n";
+		$_ = ask("What to do with this address? ([q]uit|[d]rop): ",
+			valid_re => qr/^(?:quit|q|drop|d)/i,
+			default => 'q');
+		if (/^d/i) {
+			return undef;
+		} elsif (/^q/i) {
+			cleanup_compose_files();
+			exit(0);
+		}
 	}
 	return $address;
 }
-- 
1.8.0.393.gcc9701d

[PATCH 5/5] git-send-email: allow edit invalid email address

[Krzysztof Mazur]

In some cases the user may want to send email with "Cc:" line with
email address we cannot extract. Now we allow user to extract
such email address for us.

Signed-off-by: Krzysztof Mazur <krzysiek@podlesie.net>
---
 git-send-email.perl | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index d42dca2..9996735 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -851,10 +851,10 @@ sub extract_valid_address_or_die {
 
 sub validate_address {
 	my $address = shift;
-	if (!extract_valid_address($address)) {
+	while (!extract_valid_address($address)) {
 		print STDERR "error: unable to extract a valid address from: $address\n";
-		$_ = ask("What to do with this address? ([q]uit|[d]rop): ",
-			valid_re => qr/^(?:quit|q|drop|d)/i,
+		$_ = ask("What to do with this address? ([q]uit|[d]rop|[e]dit): ",
+			valid_re => qr/^(?:quit|q|drop|d|edit|e)/i,
 			default => 'q');
 		if (/^d/i) {
 			return undef;
@@ -862,6 +862,9 @@ sub validate_address {
 			cleanup_compose_files();
 			exit(0);
 		}
+		$address = ask("Who should the email be sent to (if any)? ",
+			default => "",
+			valid_re => qr/\@.*\./, confirm_only => 1);
 	}
 	return $address;
 }
-- 
1.8.0.393.gcc9701d

Re: [PATCH 3/5] git-send-email: remove invalid addresses earlier

[Junio C Hamano]

Krzysztof Mazur <krzysiek@podlesie.net> writes:

> Some addresses are passed twice to unique_email_list() and invalid addresses
> may be reported twice per send_message. Now we warn about them earlier
> and we also remove invalid addresses.
>
> This also removes using of undefined values for string comparison
> for invalid addresses in cc list processing.
>
> Signed-off-by: Krzysztof Mazur <krzysiek@podlesie.net>
> ---

I think there are three kinds of address-looking things we deal
with:

 * Possibly invalid but meant for human consumption, e.g.

     Cc: Stable Kernel <stable@k.org> # for v3.5 and upwards

   in the commit log message trailer.

 * Meant to be fed to our MSA, without losing the human readable
   part, e.g.

     Cc: Stable Kernel <stable@k.org>

   in the header of the outgoing message.

 * Without the human-readable part, e.g.

     stable@k.org

   that is returned by extract_valid_address.

My understanding is that our input typically comes from the first
kind and sanitize_address() is meant to massage it into the second
kind.  The last kind is to be used to drive the underlying sendmail
machinery and meant to go in the envelope (this includes message-id
generation).

I do not think send-email adds the first kind (invalid ones) in its
output, even though it reads them from its input and copy them to
its output in the e-mail body part of the payload, but I think it
adds new addresses to the e-mail header part of the payload (that is
what $from, @initial_to, @initial_cc and @bcclist are all about).
We would want to feed them in the third form (i.e. output from
extract-valid-address on them) when driving the underlying sendmail
machinery to place them in the envelope part, but they should be in
the second form when we place them on e-mail header lines.  As far
as I can tell, the resulting code looks correct in this regard.  The
addresses are sanitized into the second form upfront and validated
before they are placed in @initial_to and friends, and we carry the
second form around most of the time, until we call unique_email_list
in send_message to pass them through extract_valid_address to turn
them into the third form to drive the underlying sendmail.

I however found it a bit confusing while reading the callers of
validate_address{,_list} functions, which not just validate (and
warns) but return the ones that pass the test.  Perhaps we would
want a brief comment before validate_address, validate_address_list,
and extract_valid_address{,_or_die} to clarify what they are doing
(especially what they return)?

The result still feels somewhat yucky (the yuckiness comes primarily
from the current code, not from the patch but I am mostly focused on
the result after applying the patch), in that extract-valid-address
that has problem with invalid email addresses will still die when
fed an address that is not "sanitized" first, so any future patch
that adds a new address source may still have to suffer the same
problem the part that dealt with the Cc list suffered (which your
1/5 fixed earlier), but I do not offhand think of a good way to
reorganize them.  We could of course make validate_address() call
sanitize_address(), but that would be mostly redundant since the new
code sanitizes the input upfront.

So overall, looks good to me.  Thanks.

>  git-send-email.perl | 52 +++++++++++++++++++++++++++++++++++++++-------------
>  1 file changed, 39 insertions(+), 13 deletions(-)
>
> diff --git a/git-send-email.perl b/git-send-email.perl
> index 356f99d..5056fdc 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -786,9 +786,11 @@ sub expand_one_alias {
>  }
>  
>  @initial_to = expand_aliases(@initial_to);
> -@initial_to = (map { sanitize_address($_) } @initial_to);
> +@initial_to = validate_address_list(sanitize_address_list(@initial_to));
>  @initial_cc = expand_aliases(@initial_cc);
> +@initial_cc = validate_address_list(sanitize_address_list(@initial_cc));
>  @bcclist = expand_aliases(@bcclist);
> +@bcclist = validate_address_list(sanitize_address_list(@bcclist));

Re: [PATCH 5/5] git-send-email: allow edit invalid email address

[Junio C Hamano]

Krzysztof Mazur <krzysiek@podlesie.net> writes:

> In some cases the user may want to send email with "Cc:" line with
> email address we cannot extract. Now we allow user to extract
> such email address for us.
>
> Signed-off-by: Krzysztof Mazur <krzysiek@podlesie.net>
> ---
>  git-send-email.perl | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/git-send-email.perl b/git-send-email.perl
> index d42dca2..9996735 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -851,10 +851,10 @@ sub extract_valid_address_or_die {
>  
>  sub validate_address {
>  	my $address = shift;
> -	if (!extract_valid_address($address)) {
> +	while (!extract_valid_address($address)) {
>  		print STDERR "error: unable to extract a valid address from: $address\n";
> -		$_ = ask("What to do with this address? ([q]uit|[d]rop): ",
> -			valid_re => qr/^(?:quit|q|drop|d)/i,
> +		$_ = ask("What to do with this address? ([q]uit|[d]rop|[e]dit): ",
> +			valid_re => qr/^(?:quit|q|drop|d|edit|e)/i,
>  			default => 'q');
>  		if (/^d/i) {
>  			return undef;
> @@ -862,6 +862,9 @@ sub validate_address {
>  			cleanup_compose_files();
>  			exit(0);
>  		}
> +		$address = ask("Who should the email be sent to (if any)? ",
> +			default => "",
> +			valid_re => qr/\@.*\./, confirm_only => 1);

Not having this new code inside "elsif (/^e/) { }" feels somewhat
sloppy, even though it is not *too* bad.  Also do we know this
function will never be used for addresses other than recipients' (I
gave a cursory look to see what is done to the $sender and it does
not seem to go through this function, tho)?

Re: [PATCH 5/5] git-send-email: allow edit invalid email address

[Krzysztof Mazur]

On Mon, Nov 26, 2012 at 09:08:34AM -0800, Junio C Hamano wrote:
> Krzysztof Mazur <krzysiek@podlesie.net> writes:
> 
> > In some cases the user may want to send email with "Cc:" line with
> > email address we cannot extract. Now we allow user to extract
> > such email address for us.
> >
> > Signed-off-by: Krzysztof Mazur <krzysiek@podlesie.net>
> > ---
> >  git-send-email.perl | 9 ++++++---
> >  1 file changed, 6 insertions(+), 3 deletions(-)
> >
> > diff --git a/git-send-email.perl b/git-send-email.perl
> > index d42dca2..9996735 100755
> > --- a/git-send-email.perl
> > +++ b/git-send-email.perl
> > @@ -851,10 +851,10 @@ sub extract_valid_address_or_die {
> >  
> >  sub validate_address {
> >  	my $address = shift;
> > -	if (!extract_valid_address($address)) {
> > +	while (!extract_valid_address($address)) {
> >  		print STDERR "error: unable to extract a valid address from: $address\n";
> > -		$_ = ask("What to do with this address? ([q]uit|[d]rop): ",
> > -			valid_re => qr/^(?:quit|q|drop|d)/i,
> > +		$_ = ask("What to do with this address? ([q]uit|[d]rop|[e]dit): ",
> > +			valid_re => qr/^(?:quit|q|drop|d|edit|e)/i,
> >  			default => 'q');
> >  		if (/^d/i) {
> >  			return undef;
> > @@ -862,6 +862,9 @@ sub validate_address {
> >  			cleanup_compose_files();
> >  			exit(0);
> >  		}
> > +		$address = ask("Who should the email be sent to (if any)? ",
> > +			default => "",
> > +			valid_re => qr/\@.*\./, confirm_only => 1);
> 
> Not having this new code inside "elsif (/^e/) { }" feels somewhat
> sloppy, even though it is not *too* bad.  Also do we know this

ok, I will fix that.

> function will never be used for addresses other than recipients' (I
> gave a cursory look to see what is done to the $sender and it does
> not seem to go through this function, tho)?

Yes, this function is called only from validate_address_just()
to filter @initial_to, @initial_cc, @bcc_list as early as possible,
and filter @to and @cc added in each email.

Krzysiek

Re: [PATCH 5/5] git-send-email: allow edit invalid email address

[Junio C Hamano]

Krzysztof Mazur <krzysiek@podlesie.net> writes:

>> Not having this new code inside "elsif (/^e/) { }" feels somewhat
>> sloppy, even though it is not *too* bad.  Also do we know this
>
> ok, I will fix that.
>
>> function will never be used for addresses other than recipients' (I
>> gave a cursory look to see what is done to the $sender and it does
>> not seem to go through this function, tho)?
>
> Yes, this function is called only from validate_address_just()
> to filter @initial_to, @initial_cc, @bcc_list as early as possible,
> and filter @to and @cc added in each email.

Thanks; when merged to 'pu', this series seems to break t9001.  I'll
push the result out with breakages but could you take a look?


Test Summary Report
-------------------
t9001-send-email.sh                              (Wstat: 256 Tests: 102 Failed: 77)
  Failed tests:  4-7, 9-10, 12-13, 15, 17-21, 23-29, 31-33
                35, 37, 39, 41, 43, 45, 47, 49, 51-58, 61-88
                91, 93-95, 98-102
  Non-zero exit status: 1

Re: [PATCH 5/5] git-send-email: allow edit invalid email address

[Krzysztof Mazur]

On Mon, Nov 26, 2012 at 02:58:58PM -0800, Junio C Hamano wrote:
> Krzysztof Mazur <krzysiek@podlesie.net> writes:
> 
> >> Not having this new code inside "elsif (/^e/) { }" feels somewhat
> >> sloppy, even though it is not *too* bad.  Also do we know this
> >
> > ok, I will fix that.
> >
> >> function will never be used for addresses other than recipients' (I
> >> gave a cursory look to see what is done to the $sender and it does
> >> not seem to go through this function, tho)?
> >
> > Yes, this function is called only from validate_address_just()
> > to filter @initial_to, @initial_cc, @bcc_list as early as possible,
> > and filter @to and @cc added in each email.
> 
> Thanks; when merged to 'pu', this series seems to break t9001.  I'll
> push the result out with breakages but could you take a look?
> 

Sorry, I tested final version only on an ancient perl 5.8.8 and it really
worked there. The third patch is broken:

diff --git a/git-send-email.perl b/git-send-email.perl
index 9996735..f3bbc16 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -1472,7 +1472,7 @@ sub unique_email_list {
 	my @emails;
 
 	foreach my $entry (@_) {
-		my $clean = extract_valid_address_or_die($entry))
+		my $clean = extract_valid_address_or_die($entry);
 		$seen{$clean} ||= 0;
 		next if $seen{$clean}++;
 		push @emails, $entry;

Krzysiek

Re: [PATCH 5/5] git-send-email: allow edit invalid email address

[Junio C Hamano]

Krzysztof Mazur <krzysiek@podlesie.net> writes:

> On Mon, Nov 26, 2012 at 02:58:58PM -0800, Junio C Hamano wrote:
>> Krzysztof Mazur <krzysiek@podlesie.net> writes:
>> 
>> >> Not having this new code inside "elsif (/^e/) { }" feels somewhat
>> >> sloppy, even though it is not *too* bad.  Also do we know this
>> >
>> > ok, I will fix that.
>> >
>> >> function will never be used for addresses other than recipients' (I
>> >> gave a cursory look to see what is done to the $sender and it does
>> >> not seem to go through this function, tho)?
>> >
>> > Yes, this function is called only from validate_address_just()
>> > to filter @initial_to, @initial_cc, @bcc_list as early as possible,
>> > and filter @to and @cc added in each email.
>> 
>> Thanks; when merged to 'pu', this series seems to break t9001.  I'll
>> push the result out with breakages but could you take a look?
>> 
>
> Sorry, I tested final version only on an ancient perl 5.8.8 and it really
> worked there. The third patch is broken:
>
> diff --git a/git-send-email.perl b/git-send-email.perl
> index 9996735..f3bbc16 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -1472,7 +1472,7 @@ sub unique_email_list {
>  	my @emails;
>  
>  	foreach my $entry (@_) {
> -		my $clean = extract_valid_address_or_die($entry))
> +		my $clean = extract_valid_address_or_die($entry);

Ah, ok, I wasn't looking closely enough.  Thanks for a quick
turnaround.  Will requeue and push out.

>  		$seen{$clean} ||= 0;
>  		next if $seen{$clean}++;
>  		push @emails, $entry;
>
> Krzysiek

Re: [PATCH 5/5] git-send-email: allow edit invalid email address

[Krzysztof Mazur]

On Mon, Nov 26, 2012 at 03:50:30PM -0800, Junio C Hamano wrote:
> Krzysztof Mazur <krzysiek@podlesie.net> writes:
> 
> > On Mon, Nov 26, 2012 at 02:58:58PM -0800, Junio C Hamano wrote:
> >> Krzysztof Mazur <krzysiek@podlesie.net> writes:
> >> 
> >> >> Not having this new code inside "elsif (/^e/) { }" feels somewhat
> >> >> sloppy, even though it is not *too* bad.  Also do we know this
> >> >
> >> > ok, I will fix that.
> >> >
> >> >> function will never be used for addresses other than recipients' (I
> >> >> gave a cursory look to see what is done to the $sender and it does
> >> >> not seem to go through this function, tho)?
> >> >
> >> > Yes, this function is called only from validate_address_just()
> >> > to filter @initial_to, @initial_cc, @bcc_list as early as possible,
> >> > and filter @to and @cc added in each email.
> >> 
> >> Thanks; when merged to 'pu', this series seems to break t9001.  I'll
> >> push the result out with breakages but could you take a look?
> >> 
> >
> > Sorry, I tested final version only on an ancient perl 5.8.8 and it really
> > worked there. The third patch is broken:
> >
> > diff --git a/git-send-email.perl b/git-send-email.perl
> > index 9996735..f3bbc16 100755
> > --- a/git-send-email.perl
> > +++ b/git-send-email.perl
> > @@ -1472,7 +1472,7 @@ sub unique_email_list {
> >  	my @emails;
> >  
> >  	foreach my $entry (@_) {
> > -		my $clean = extract_valid_address_or_die($entry))
> > +		my $clean = extract_valid_address_or_die($entry);
> 
> Ah, ok, I wasn't looking closely enough.  Thanks for a quick
> turnaround.  Will requeue and push out.

I rechecked that and I've just sent some older broken version. The
patch that I've sent had date Date: Thu, 22 Nov 2012 19:00:25 +0100,
but on my tree I have commit from Date: Thu Nov 22 19:01:55 2012 +0100,
which is exactly the same as the fixed version in your tree.

Thanks,

Krzysiek