problem when using --cc-cmd

problem when using --cc-cmd

[Thiago Farina]

Hi,

I'm trying to use the --cc-cmd to get the list of people who to copy
when sending a patch to linux kernel.

But when I run:

$ git send-email --to linux-kernel@vger.kernel.org --cc-cmd
scripts/get_maintainer.pl foo

I'm getting some lines like:
Use of uninitialized value $cc in string eq at
/home/tfarina/libexec/git-core/git-send-email line 964.

Any idea?

Thanks in advance.

Re: problem when using --cc-cmd

[Jonathan Nieder]

Hi,

Thiago Farina wrote:

> when I run:
>
> $ git send-email --to linux-kernel@vger.kernel.org --cc-cmd
> scripts/get_maintainer.pl foo
>
> I'm getting some lines like:
> Use of uninitialized value $cc in string eq at
> /home/tfarina/libexec/git-core/git-send-email line 964.

Yes, sounds like a bug.  Cc-ing some send-email people for tips.

On the other hand, using --cc-cmd=scripts/get_maintainer.pl does not
sound like a great idea to me.  On one hand the output of
get_maintainer.pl is not an unadorned address per line like --cc-cmd
expects.  On the other hand, at least some versions of
get_maintainer.pl returned more addresses than are likely to be
interested people (by using --git by default).

I think get_maintainer.pl is meant to be a starting point for tracking
down who might be interested in a patch and should be followed by
careful investigation.  (That means making sure that there is a
reasonable number of people and the reasons given by --roles ouput
make sense, and maybe even glancing at some messages by them from the
relevant mailing list to make sure the script has not gone haywire.)

Hope that helps,
Jonathan

Re: problem when using --cc-cmd

[Joe Perches]

On Tue, 2011-04-19 at 16:52 -0500, Jonathan Nieder wrote:
> Thiago Farina wrote:
> > when I run:
> > $ git send-email --to linux-kernel@vger.kernel.org --cc-cmd
> > scripts/get_maintainer.pl foo
> > I'm getting some lines like:
> > Use of uninitialized value $cc in string eq at
> > /home/tfarina/libexec/git-core/git-send-email line 964.
> Yes, sounds like a bug.  Cc-ing some send-email people for tips.

I haven't seen this.

What versions of ./scripts/get_maintainer.pl and git are
you using?

> On the other hand, using --cc-cmd=scripts/get_maintainer.pl does not
> sound like a great idea to me.  On one hand the output of
> get_maintainer.pl is not an unadorned address per line like --cc-cmd
> expects.  On the other hand, at least some versions of
> get_maintainer.pl returned more addresses than are likely to be
> interested people (by using --git by default).
> 
> I think get_maintainer.pl is meant to be a starting point for tracking
> down who might be interested in a patch and should be followed by
> careful investigation.  (That means making sure that there is a
> reasonable number of people and the reasons given by --roles ouput
> make sense, and maybe even glancing at some messages by them from the
> relevant mailing list to make sure the script has not gone haywire.)

Jonathan is basically correct in the what he writes above.

I also think git history isn't a very good mechanism to
rely on for determining MAINTAINERS, it should only be a
fallback to determine who should receive a copy of a patch.

That said, I use scripts/get_maintainer.pl to generate
to's and cc's.  I do not use --git or --git-fallback
and rely only on the MAINTAINERS file pattern matching.

Here are the settings I use:

$ cat ~/.gitconfig
[sendemail]
	chainreplyto = false
	thread = false
	suppresscc = self
	tocmd = ~/bin/to.sh
	cccmd = ~/bin/cc.sh

$ cat ~/bin/to.sh
#!/bin/bash

opts="--nogit --nogit-fallback --norolestats --pattern-depth=1"

if [[ $(basename $1) =~ ^0000- ]] ; then
    ./scripts/get_maintainer.pl --nom $opts $(dirname $1)/*
else
    maint=$(./scripts/get_maintainer.pl --nol $opts $1)

    if [ "$maint" == "" ] ; then
	echo "linux-kernel@vger.kernel.org"
    else
	echo "$maint"
    fi
fi

$ cat ~/bin/cc.sh
#!/bin/bash

opts="--nogit --nogit-fallback --norolestats"

if [[ $(basename $1) =~ ^0000- ]] ; then
    ./scripts/get_maintainer.pl --nom $opts $(dirname $1)/*
else
    ./scripts/get_maintainer.pl $opts $1
fi

Re: problem when using --cc-cmd

[Thiago Farina]

On Wed, Apr 20, 2011 at 12:03 AM, Joe Perches <joe@perches.com> wrote:
> On Tue, 2011-04-19 at 16:52 -0500, Jonathan Nieder wrote:
>> Thiago Farina wrote:
>> > when I run:
>> > $ git send-email --to linux-kernel@vger.kernel.org --cc-cmd
>> > scripts/get_maintainer.pl foo
>> > I'm getting some lines like:
>> > Use of uninitialized value $cc in string eq at
>> > /home/tfarina/libexec/git-core/git-send-email line 964.
>> Yes, sounds like a bug.  Cc-ing some send-email people for tips.
>
> I haven't seen this.
>
> What versions of ./scripts/get_maintainer.pl and git are
> you using?
>

$ scripts/get_maintainer.pl --version
scripts/get_maintainer.pl 0.26

$ git version
git version 1.7.5.rc2.5.g60e19

>> On the other hand, using --cc-cmd=scripts/get_maintainer.pl does not
>> sound like a great idea to me.  On one hand the output of
>> get_maintainer.pl is not an unadorned address per line like --cc-cmd
>> expects.  On the other hand, at least some versions of
>> get_maintainer.pl returned more addresses than are likely to be
>> interested people (by using --git by default).
>>
>> I think get_maintainer.pl is meant to be a starting point for tracking
>> down who might be interested in a patch and should be followed by
>> careful investigation.  (That means making sure that there is a
>> reasonable number of people and the reasons given by --roles ouput
>> make sense, and maybe even glancing at some messages by them from the
>> relevant mailing list to make sure the script has not gone haywire.)
>
> Jonathan is basically correct in the what he writes above.
>
> I also think git history isn't a very good mechanism to
> rely on for determining MAINTAINERS, it should only be a
> fallback to determine who should receive a copy of a patch.
>
> That said, I use scripts/get_maintainer.pl to generate
> to's and cc's.  I do not use --git or --git-fallback
> and rely only on the MAINTAINERS file pattern matching.
>
> Here are the settings I use:
>
Cool, thanks for sharing it. I'll add that to my config file.

> $ cat ~/.gitconfig
> [sendemail]
>        chainreplyto = false
>        thread = false
>        suppresscc = self
>        tocmd = ~/bin/to.sh
>        cccmd = ~/bin/cc.sh
>
> $ cat ~/bin/to.sh
> #!/bin/bash
>
> opts="--nogit --nogit-fallback --norolestats --pattern-depth=1"
>
> if [[ $(basename $1) =~ ^0000- ]] ; then
>    ./scripts/get_maintainer.pl --nom $opts $(dirname $1)/*
> else
>    maint=$(./scripts/get_maintainer.pl --nol $opts $1)
>
>    if [ "$maint" == "" ] ; then
>        echo "linux-kernel@vger.kernel.org"
>    else
>        echo "$maint"
>    fi
> fi
>
> $ cat ~/bin/cc.sh
> #!/bin/bash
>
> opts="--nogit --nogit-fallback --norolestats"
>
> if [[ $(basename $1) =~ ^0000- ]] ; then
>    ./scripts/get_maintainer.pl --nom $opts $(dirname $1)/*
> else
>    ./scripts/get_maintainer.pl $opts $1
> fi
>
>
>
>

Re: problem when using --cc-cmd

[Joe Perches]

On Wed, 2011-04-20 at 12:45 -0300, Thiago Farina wrote:
> On Wed, Apr 20, 2011 at 12:03 AM, Joe Perches <joe@perches.com> wrote:
> > On Tue, 2011-04-19 at 16:52 -0500, Jonathan Nieder wrote:
> >> Thiago Farina wrote:
> >> > when I run:
> >> > $ git send-email --to linux-kernel@vger.kernel.org --cc-cmd
> >> > scripts/get_maintainer.pl foo
> >> > I'm getting some lines like:
> >> > Use of uninitialized value $cc in string eq at
> >> > /home/tfarina/libexec/git-core/git-send-email line 964.
> >> Yes, sounds like a bug.  Cc-ing some send-email people for tips.
> > I haven't seen this.
> > What versions of ./scripts/get_maintainer.pl and git are
> > you using?
> $ scripts/get_maintainer.pl --version
> scripts/get_maintainer.pl 0.26
> $ git version
> git version 1.7.5.rc2.5.g60e19

To get this to work properly, the output of cc-cmd
(scripts/get_maintainer.pl) must be valid email addresses.

The git send-email --help for cc-cmd says:

       --cc-cmd=<command>
           Specify a command to execute once per patch file which should
           generate patch file specific "Cc:" entries. Output of this command
           must be single email address per line. Default is the value of
           sendemail.cccmd configuration value.

You'll need to add "--norolestats" to the cc-cmd if
you use scripts/get_maintainer.pl.

$ git send-email --to linux-kernel@vger.kernel.org \
	--cc-cmd "scripts/get_maintainer.pl --norolestats" foo

I suppose you could call it a defect that the
output of cc-cmd isn't screened for invalid
email addresses but I think it's not really a
problem.

[RFC PATCH] git-send-email: Validate recipient_cmd (to-cmd, cc-cmd) addresses

[Joe Perches]

On Wed, 2011-04-20 at 12:45 -0300, Thiago Farina wrote:
> On Wed, Apr 20, 2011 at 12:03 AM, Joe Perches <joe@perches.com> wrote:
> > On Tue, 2011-04-19 at 16:52 -0500, Jonathan Nieder wrote:
> >> Thiago Farina wrote:
> >> > when I run:
> >> > $ git send-email --to linux-kernel@vger.kernel.org --cc-cmd
> >> > scripts/get_maintainer.pl foo
> >> > I'm getting some lines like:
> >> > Use of uninitialized value $cc in string eq at
> >> > /home/tfarina/libexec/git-core/git-send-email line 964.
> >> Yes, sounds like a bug.  Cc-ing some send-email people for tips.

Perhaps some patch like this.

Validate the address(es) returned from recipient_cmd.
Die if the output contains an invalid address.

Signed-off-by: Joe Perches <joe@perches.com>
---
 git-send-email.perl |   18 ++++++++++++------
 1 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index 76565de..9273cf2 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -870,10 +870,14 @@ sub is_rfc2047_quoted {
 # use the simplest quoting being able to handle the recipient
 sub sanitize_address {
 	my ($recipient) = @_;
-	my ($recipient_name, $recipient_addr) = ($recipient =~ /^(.*?)\s*(<.*)/);
+	my ($recipient_name, $recipient_addr) = ($recipient =~ /^\s*(.*?)\s*(<[^>]+>)/);
 
 	if (not $recipient_name) {
-		return $recipient;
+		return $recipient_addr if ($recipient_addr);
+		if ($recipient =~ /^\s*(.+\@\S*).*$/) {
+			return $1;
+		}
+		return "";
 	}
 
 	# if recipient_name is already quoted, do nothing
@@ -1343,11 +1347,13 @@ sub recipients_cmd {
 	while (my $address = <$fh>) {
 		$address =~ s/^\s*//g;
 		$address =~ s/\s*$//g;
-		$address = sanitize_address($address);
-		next if ($address eq $sanitized_sender and $suppress_from);
-		push @addresses, $address;
+		my $sanitized_address = sanitize_address($address);
+		next if ($sanitized_address eq $sanitized_sender and $suppress_from);
+		die "($prefix) '$cmd' returned invalid address: '$address'\n"
+			if ($address =~ /.*${sanitized_address}.+/);
+		push @addresses, $sanitized_address;
 		printf("($prefix) Adding %s: %s from: '%s'\n",
-		       $what, $address, $cmd) unless $quiet;
+		       $what, $sanitized_address, $cmd) unless $quiet;
 		}
 	close $fh
 	    or die "($prefix) failed to close pipe to '$cmd'";

Re: [RFC PATCH] git-send-email: Validate recipient_cmd (to-cmd, cc-cmd) addresses

[Ævar Arnfjörð Bjarmason]

On Wed, Apr 20, 2011 at 23:50, Joe Perches <joe@perches.com> wrote:
> +       my ($recipient_name, $recipient_addr) = ($recipient =~ /^\s*(.*?)\s*(<[^>]+>)/);

In Perl you can write (<.*?>) instead of (<[^>]+>)

> +               if ($recipient =~ /^\s*(.+\@\S*).*$/) {

If this program doesn't have some extract_emails_from_string()
function already it probably should.

Re: [RFC PATCH] git-send-email: Validate recipient_cmd (to-cmd, cc-cmd) addresses

[Joe Perches]

On Thu, 2011-04-21 at 00:29 +0200, Ævar Arnfjörð Bjarmason wrote:
> On Wed, Apr 20, 2011 at 23:50, Joe Perches <joe@perches.com> wrote:
> > +       my ($recipient_name, $recipient_addr) = ($recipient =~ /^\s*(.*?)\s*(<[^>]+>)/);
> In Perl you can write (<.*?>) instead of (<[^>]+>)

Hey Ævar.  That matches <>.  Not a good email address.

This is what linux/scripts/get_maintainers.pl uses:

sub parse_email {
    my ($formatted_email) = @_;

    my $name = "";
    my $address = "";

    if ($formatted_email =~ /^([^<]+)<(.+\@.*)>.*$/) {
	$name = $1;
	$address = $2;
    } elsif ($formatted_email =~ /^\s*<(.+\@\S*)>.*$/) {
	$address = $1;
    } elsif ($formatted_email =~ /^(.+\@\S*).*$/) {
	$address = $1;
    }

    $name =~ s/^\s+|\s+$//g;
    $name =~ s/^\"|\"$//g;
    $address =~ s/^\s+|\s+$//g;

    if ($name =~ /[^\w \-]/i) {  	 ##has "must quote" chars
	$name =~ s/(?<!\\)"/\\"/g;       ##escape quotes
	$name = "\"$name\"";
    }

    return ($name, $address);
}

There's probably some weakness in that.

> If this program doesn't have some extract_emails_from_string()
> function already it probably should.

Maybe it does.  It currently uses "sanitize_address".

Re: [RFC PATCH] git-send-email: Validate recipient_cmd (to-cmd, cc-cmd) addresses

[Ævar Arnfjörð Bjarmason]

On Thu, Apr 21, 2011 at 00:45, Joe Perches <joe@perches.com> wrote:
> On Thu, 2011-04-21 at 00:29 +0200, Ævar Arnfjörð Bjarmason wrote:
>> On Wed, Apr 20, 2011 at 23:50, Joe Perches <joe@perches.com> wrote:
>> > +       my ($recipient_name, $recipient_addr) = ($recipient =~ /^\s*(.*?)\s*(<[^>]+>)/);
>> In Perl you can write (<.*?>) instead of (<[^>]+>)
>
> Hey Ævar.  That matches <>.  Not a good email address.

True, but you can use <.+?> instead.

I meant that you don't need to work around the lack of non-greedy
regex features in Perl.

Re: [RFC PATCH] git-send-email: Validate recipient_cmd (to-cmd, cc-cmd) addresses

[Joe Perches]

On Thu, 2011-04-21 at 00:50 +0200, Ævar Arnfjörð Bjarmason wrote:
> On Thu, Apr 21, 2011 at 00:45, Joe Perches <joe@perches.com> wrote:
> > On Thu, 2011-04-21 at 00:29 +0200, Ævar Arnfjörð Bjarmason wrote:
> >> On Wed, Apr 20, 2011 at 23:50, Joe Perches <joe@perches.com> wrote:
> >> > +       my ($recipient_name, $recipient_addr) = ($recipient =~ /^\s*(.*?)\s*(<[^>]+>)/);
> >> In Perl you can write (<.*?>) instead of (<[^>]+>)
> > Hey Ævar.  That matches <>.  Not a good email address.
> True, but you can use <.+?> instead.

Nope, that matches all of "<>>"
I want to terminate the match on the first >.