Re: threaded patch series

Re: threaded patch series

[Joe Perches]

On Thu, 2010-09-23 at 10:55 +0200, Julia Lawall wrote:
> I made some changes to git-send-email to get it to send mail to different 
> people, ie a different set of addresses for each patch.  Is that now 
> possible with the standard version?  If not I can submit a patch with my 
> changes at some point.

I believe it's not currently possible to have
different "to:" addresses in a git send-email
patch series and that could be a useful patch
to submit.

[RFC PATCH] sit-send-email.pl: Add --to-cmd

[Joe Perches]

On Thu, 2010-09-23 at 17:58 +0200, Julia Lawall wrote:
> On Thu, 23 Sep 2010, Joe Perches wrote:
> > On Thu, 2010-09-23 at 16:00 +0400, Vasiliy Kulikov wrote:
> > > On Thu, Sep 23, 2010 at 13:09 +0400, Vasiliy Kulikov wrote:
> > > > On Thu, Sep 23, 2010 at 10:55 +0200, Julia Lawall wrote:
> > > > > I made some changes to git-send-email to get it to send mail to different 
> > > > > people, ie a different set of addresses for each patch.  Is that now 
> > > > > possible with the standard version?  If not I can submit a patch with my 
> > > > > changes at some point.
> > > > I use git-send-email --cc-cmd=script_to_form_cc_list.
> > I believe that Julia means some mechanism to vary the
> > "to" addresses for each patch, ie: some "--to-cmd=cmd".
> Yes, sort of.  I took the strategy of precomputing the To addresses, so I 
> just have a collection of files that have different To and Cc addresses.  
> But a --to-cmd option seems like a good idea too.

Perhaps something like this?

Lightly tested only.

I know there's a test harness in git, but
I don't know how to wire up the new options.

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

diff --git a/git-send-email.perl b/git-send-email.perl
index 6dab3bf..8e8e4c4 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -70,6 +70,7 @@ git send-email [options] <file | directory | rev-list options >
 
   Automating:
     --identity              <str>  * Use the sendemail.<id> options.
+    --to-cmd                <str>  * Email To: via `<str> \$patch_path`
     --cc-cmd                <str>  * Email Cc: via `<str> \$patch_path`
     --suppress-cc           <str>  * author, self, sob, cc, cccmd, body, bodycc, all.
     --[no-]signed-off-by-cc        * Send to Signed-off-by: addresses. Default on.
@@ -187,7 +188,8 @@ sub do_edit {
 }
 
 # Variables with corresponding config settings
-my ($thread, $chain_reply_to, $suppress_from, $signed_off_by_cc, $cc_cmd);
+my ($thread, $chain_reply_to, $suppress_from, $signed_off_by_cc);
+my ($to_cmd, $cc_cmd);
 my ($smtp_server, $smtp_server_port, $smtp_authuser, $smtp_encryption);
 my ($identity, $aliasfiletype, @alias_files, @smtp_host_parts, $smtp_domain);
 my ($validate, $confirm);
@@ -214,6 +216,7 @@ my %config_settings = (
     "smtppass" => \$smtp_authpass,
 	"smtpdomain" => \$smtp_domain,
     "to" => \@to,
+    "tocmd" => \$to_cmd,
     "cc" => \@initial_cc,
     "cccmd" => \$cc_cmd,
     "aliasfiletype" => \$aliasfiletype,
@@ -272,6 +275,7 @@ my $rc = GetOptions("sender|from=s" => \$sender,
                     "in-reply-to=s" => \$initial_reply_to,
 		    "subject=s" => \$initial_subject,
 		    "to=s" => \@to,
+		    "to-cmd=s" => \$to_cmd,
 		    "no-to" => \$no_to,
 		    "cc=s" => \@initial_cc,
 		    "no-cc" => \$no_cc,
@@ -711,7 +715,7 @@ if (!defined $sender) {
 	$prompting++;
 }
 
-if (!@to) {
+if (!@to && $to_cmd eq "") {
 	my $to = ask("Who should the emails be sent to? ");
 	push @to, parse_address_line($to) if defined $to; # sanitized/validated later
 	$prompting++;
@@ -1238,6 +1242,23 @@ foreach my $t (@files) {
 	}
 	close F;
 
+	if (defined $to_cmd) {
+		open(F, "$to_cmd \Q$t\E |")
+			or die "(to-cmd) Could not execute '$to_cmd'";
+		while(<F>) {
+			my $t = $_;
+			$t =~ s/^\s*//g;
+			$t =~ s/\n$//g;
+			next if ($t eq $sender and $suppress_from);
+			push @to, parse_address_line($t)
+			    if defined $t; # sanitized/validated later
+			printf("(to-cmd) Adding To: %s from: '%s'\n",
+				$t, $to_cmd) unless $quiet;
+		}
+		close F
+			or die "(to-cmd) failed to close pipe to '$to_cmd'";
+	}
+
 	if (defined $cc_cmd && !$suppress_cc{'cccmd'}) {
 		open(F, "$cc_cmd \Q$t\E |")
 			or die "(cc-cmd) Could not execute '$cc_cmd'";

Re: [RFC PATCH] sit-send-email.pl: Add --to-cmd

[Ævar Arnfjörð Bjarmason]

On Thu, Sep 23, 2010 at 17:17, Joe Perches <joe@perches.com> wrote:
> On Thu, 2010-09-23 at 17:58 +0200, Julia Lawall wrote:
>> On Thu, 23 Sep 2010, Joe Perches wrote:
>> > On Thu, 2010-09-23 at 16:00 +0400, Vasiliy Kulikov wrote:
>> > > On Thu, Sep 23, 2010 at 13:09 +0400, Vasiliy Kulikov wrote:
>> > > > On Thu, Sep 23, 2010 at 10:55 +0200, Julia Lawall wrote:
>> > > > > I made some changes to git-send-email to get it to send mail to different
>> > > > > people, ie a different set of addresses for each patch.  Is that now
>> > > > > possible with the standard version?  If not I can submit a patch with my
>> > > > > changes at some point.
>> > > > I use git-send-email --cc-cmd=script_to_form_cc_list.
>> > I believe that Julia means some mechanism to vary the
>> > "to" addresses for each patch, ie: some "--to-cmd=cmd".
>> Yes, sort of.  I took the strategy of precomputing the To addresses, so I
>> just have a collection of files that have different To and Cc addresses.
>> But a --to-cmd option seems like a good idea too.
>
> Perhaps something like this?
>
> Lightly tested only.
>
> I know there's a test harness in git, but
> I don't know how to wire up the new options.

You'd add the tests to t9001-send-email.sh and --tocmd out to some
program you create. Is there anything in particular you need help
with?

> Signed-off-by: Joe Perches <joe@perches.com>
> ---
>  git-send-email.perl |   25 +++++++++++++++++++++++--
>  1 files changed, 23 insertions(+), 2 deletions(-)
>
> diff --git a/git-send-email.perl b/git-send-email.perl
> index 6dab3bf..8e8e4c4 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -70,6 +70,7 @@ git send-email [options] <file | directory | rev-list options >
>
>   Automating:
>     --identity              <str>  * Use the sendemail.<id> options.
> +    --to-cmd                <str>  * Email To: via `<str> \$patch_path`
>     --cc-cmd                <str>  * Email Cc: via `<str> \$patch_path`
>     --suppress-cc           <str>  * author, self, sob, cc, cccmd, body, bodycc, all.
>     --[no-]signed-off-by-cc        * Send to Signed-off-by: addresses. Default on.
> @@ -187,7 +188,8 @@ sub do_edit {
>  }
>
>  # Variables with corresponding config settings
> -my ($thread, $chain_reply_to, $suppress_from, $signed_off_by_cc, $cc_cmd);
> +my ($thread, $chain_reply_to, $suppress_from, $signed_off_by_cc);
> +my ($to_cmd, $cc_cmd);
>  my ($smtp_server, $smtp_server_port, $smtp_authuser, $smtp_encryption);
>  my ($identity, $aliasfiletype, @alias_files, @smtp_host_parts, $smtp_domain);
>  my ($validate, $confirm);
> @@ -214,6 +216,7 @@ my %config_settings = (
>     "smtppass" => \$smtp_authpass,
>        "smtpdomain" => \$smtp_domain,
>     "to" => \@to,
> +    "tocmd" => \$to_cmd,
>     "cc" => \@initial_cc,
>     "cccmd" => \$cc_cmd,
>     "aliasfiletype" => \$aliasfiletype,
> @@ -272,6 +275,7 @@ my $rc = GetOptions("sender|from=s" => \$sender,
>                     "in-reply-to=s" => \$initial_reply_to,
>                    "subject=s" => \$initial_subject,
>                    "to=s" => \@to,
> +                   "to-cmd=s" => \$to_cmd,
>                    "no-to" => \$no_to,
>                    "cc=s" => \@initial_cc,
>                    "no-cc" => \$no_cc,
> @@ -711,7 +715,7 @@ if (!defined $sender) {
>        $prompting++;
>  }
>
> -if (!@to) {
> +if (!@to && $to_cmd eq "") {

Why compare $to_cmd to "" instead of checking definedness?

>        my $to = ask("Who should the emails be sent to? ");
>        push @to, parse_address_line($to) if defined $to; # sanitized/validated later
>        $prompting++;
> @@ -1238,6 +1242,23 @@ foreach my $t (@files) {
>        }
>        close F;
>
> +       if (defined $to_cmd) {
> +               open(F, "$to_cmd \Q$t\E |")

quotemeta() is for escaping regexes, not shell syntax. You probably
want IPC::Open2 or PC::Open3's functions which'll escape arguments for
you.

Also "open my $f" is better, but I see the existing code uses glob
filehandles (urghl).

> +                       or die "(to-cmd) Could not execute '$to_cmd'";
> +               while(<F>) {
> +                       my $t = $_;
> +                       $t =~ s/^\s*//g;
> +                       $t =~ s/\n$//g;

Shouldn't this just be:

    while (my $address = <$f>) {
        chomp $address;
        ...

I.e. do you need to strip whitespace from the beginning of the string?

> +                       next if ($t eq $sender and $suppress_from);
> +                       push @to, parse_address_line($t)
> +                           if defined $t; # sanitized/validated later
> +                       printf("(to-cmd) Adding To: %s from: '%s'\n",
> +                               $t, $to_cmd) unless $quiet;
> +               }
> +               close F
> +                       or die "(to-cmd) failed to close pipe to '$to_cmd'";
> +       }

close F could be skipped if we used lexical handes, but see urghl
above.

Re: [RFC PATCH] sit-send-email.pl: Add --to-cmd

[Joe Perches]

On Thu, 2010-09-23 at 17:29 +0000, Ævar Arnfjörð Bjarmason wrote:
> On Thu, Sep 23, 2010 at 17:17, Joe Perches <joe@perches.com> wrote:
> > I know there's a test harness in git, but
> > I don't know how to wire up the new options.
> You'd add the tests to t9001-send-email.sh and --tocmd out to some
> program you create. Is there anything in particular you need help
> with?

Just the doing.  I was (am) being lazy.

> > -if (!@to) {
> > +if (!@to && $to_cmd eq "") {
> 
> Why compare $to_cmd to "" instead of checking definedness?

No real reason.  Using define is the style used in the rest of
the file and it should be changed.

> > @@ -1238,6 +1242,23 @@ foreach my $t (@files) {
> >        }
> >        close F;
> >
> > +       if (defined $to_cmd) {
> > +               open(F, "$to_cmd \Q$t\E |")
> 
> quotemeta() is for escaping regexes, not shell syntax. You probably
> want IPC::Open2 or PC::Open3's functions which'll escape arguments for
> you.

I just copied the style from the equivalent cc_cmd section below,
so if it's necessary, it should be changed there too.

> I.e. do you need to strip whitespace from the beginning of the string?

I think so.

Re: [RFC PATCH] sit-send-email.pl: Add --to-cmd

[Ævar Arnfjörð Bjarmason]

On Thu, Sep 23, 2010 at 17:46, Joe Perches <joe@perches.com> wrote:
> On Thu, 2010-09-23 at 17:29 +0000, Ævar Arnfjörð Bjarmason wrote:
>> On Thu, Sep 23, 2010 at 17:17, Joe Perches <joe@perches.com> wrote:
>> > I know there's a test harness in git, but
>> > I don't know how to wire up the new options.
>> You'd add the tests to t9001-send-email.sh and --tocmd out to some
>> program you create. Is there anything in particular you need help
>> with?
>
> Just the doing.  I was (am) being lazy.
>
>> > -if (!@to) {
>> > +if (!@to && $to_cmd eq "") {
>>
>> Why compare $to_cmd to "" instead of checking definedness?
>
> No real reason.  Using define is the style used in the rest of
> the file and it should be changed.
>
>> > @@ -1238,6 +1242,23 @@ foreach my $t (@files) {
>> >        }
>> >        close F;
>> >
>> > +       if (defined $to_cmd) {
>> > +               open(F, "$to_cmd \Q$t\E |")
>>
>> quotemeta() is for escaping regexes, not shell syntax. You probably
>> want IPC::Open2 or PC::Open3's functions which'll escape arguments for
>> you.
>
> I just copied the style from the equivalent cc_cmd section below,
> so if it's necessary, it should be changed there too.
>
>> I.e. do you need to strip whitespace from the beginning of the string?
>
> I think so.

This all sounds reasonable, but I really need to go through
git-send-email.perl and fix all these bugs at some point...

[PATCH V2] git-send-email.perl: Add --to-cmd

[Joe Perches]

Add the ability to use a command line --to-cmd=cmd
to create the list of "To:" addresses.

Signed-off-by: Joe Perches <joe@perches.com>
---
 Documentation/git-send-email.txt |    8 +++++++-
 git-send-email.perl              |   25 +++++++++++++++++++++++--
 t/t9001-send-email.sh            |   18 ++++++++++++++++++
 3 files changed, 48 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt
index c283084..fff97a3 100644
--- a/Documentation/git-send-email.txt
+++ b/Documentation/git-send-email.txt
@@ -97,7 +97,7 @@ See the CONFIGURATION section for 'sendemail.multiedit'.
 	Specify the primary recipient of the emails generated. Generally, this
 	will be the upstream maintainer of the project involved. Default is the
 	value of the 'sendemail.to' configuration value; if that is unspecified,
-	this will be prompted for.
+	and --to-cmd is not specified, this will be prompted for.
 +
 The --to option must be repeated for each user you want on the to list.
 
@@ -177,6 +177,12 @@ user is prompted for a password while the input is masked for privacy.
 Automating
 ~~~~~~~~~~
 
+--to-cmd=<command>::
+	Specify a command to execute once per patch file which
+	should generate patch file specific "To:" entries.
+	Output of this command must be single email address per line.
+	Default is the value of 'sendemail.tocmd' configuration value.
+
 --cc-cmd=<command>::
 	Specify a command to execute once per patch file which
 	should generate patch file specific "Cc:" entries.
diff --git a/git-send-email.perl b/git-send-email.perl
index 6dab3bf..fa8da8b 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -70,6 +70,7 @@ git send-email [options] <file | directory | rev-list options >
 
   Automating:
     --identity              <str>  * Use the sendemail.<id> options.
+    --to-cmd                <str>  * Email To: via `<str> \$patch_path`
     --cc-cmd                <str>  * Email Cc: via `<str> \$patch_path`
     --suppress-cc           <str>  * author, self, sob, cc, cccmd, body, bodycc, all.
     --[no-]signed-off-by-cc        * Send to Signed-off-by: addresses. Default on.
@@ -187,7 +188,8 @@ sub do_edit {
 }
 
 # Variables with corresponding config settings
-my ($thread, $chain_reply_to, $suppress_from, $signed_off_by_cc, $cc_cmd);
+my ($thread, $chain_reply_to, $suppress_from, $signed_off_by_cc);
+my ($to_cmd, $cc_cmd);
 my ($smtp_server, $smtp_server_port, $smtp_authuser, $smtp_encryption);
 my ($identity, $aliasfiletype, @alias_files, @smtp_host_parts, $smtp_domain);
 my ($validate, $confirm);
@@ -214,6 +216,7 @@ my %config_settings = (
     "smtppass" => \$smtp_authpass,
 	"smtpdomain" => \$smtp_domain,
     "to" => \@to,
+    "tocmd" => \$to_cmd,
     "cc" => \@initial_cc,
     "cccmd" => \$cc_cmd,
     "aliasfiletype" => \$aliasfiletype,
@@ -272,6 +275,7 @@ my $rc = GetOptions("sender|from=s" => \$sender,
                     "in-reply-to=s" => \$initial_reply_to,
 		    "subject=s" => \$initial_subject,
 		    "to=s" => \@to,
+		    "to-cmd=s" => \$to_cmd,
 		    "no-to" => \$no_to,
 		    "cc=s" => \@initial_cc,
 		    "no-cc" => \$no_cc,
@@ -711,7 +715,7 @@ if (!defined $sender) {
 	$prompting++;
 }
 
-if (!@to) {
+if (!@to && !defined $to_cmd) {
 	my $to = ask("Who should the emails be sent to? ");
 	push @to, parse_address_line($to) if defined $to; # sanitized/validated later
 	$prompting++;
@@ -1238,6 +1242,23 @@ foreach my $t (@files) {
 	}
 	close F;
 
+	if (defined $to_cmd) {
+		open(F, "$to_cmd \Q$t\E |")
+			or die "(to-cmd) Could not execute '$to_cmd'";
+		while(<F>) {
+			my $t = $_;
+			$t =~ s/^\s*//g;
+			$t =~ s/\n$//g;
+			next if ($t eq $sender and $suppress_from);
+			push @to, parse_address_line($t)
+			    if defined $t; # sanitized/validated later
+			printf("(to-cmd) Adding To: %s from: '%s'\n",
+				$t, $to_cmd) unless $quiet;
+		}
+		close F
+			or die "(to-cmd) failed to close pipe to '$to_cmd'";
+	}
+
 	if (defined $cc_cmd && !$suppress_cc{'cccmd'}) {
 		open(F, "$cc_cmd \Q$t\E |")
 			or die "(cc-cmd) Could not execute '$cc_cmd'";
diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index 71b3df9..36cf421 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -201,6 +201,24 @@ test_expect_success $PREREQ 'Prompting works' '
 		grep "^To: to@example.com\$" msgtxt1
 '
 
+test_expect_success $PREREQ 'tocmd works' '
+	clean_fake_sendmail &&
+	cp $patches tocmd.patch &&
+	echo tocmd--tocmd@example.com >>tocmd.patch &&
+	{
+	  echo "#!$SHELL_PATH"
+	  echo sed -n -e s/^tocmd--//p \"\$1\"
+	} > tocmd-sed &&
+	chmod +x tocmd-sed &&
+	git send-email \
+		--from="Example <nobody@example.com>" \
+		--to-cmd=./tocmd-sed \
+		--smtp-server="$(pwd)/fake.sendmail" \
+		tocmd.patch \
+		&&
+	grep "^To: tocmd@example.com" msgtxt1
+'
+
 test_expect_success $PREREQ 'cccmd works' '
 	clean_fake_sendmail &&
 	cp $patches cccmd.patch &&

Re: [PATCH V2] git-send-email.perl: Add --to-cmd

[matt mooney]

On Thu, Sep 23, 2010 at 11:45 AM, Joe Perches <joe@perches.com> wrote:
> Add the ability to use a command line --to-cmd=cmd
> to create the list of "To:" addresses.
>
> Signed-off-by: Joe Perches <joe@perches.com>
> ---
>  Documentation/git-send-email.txt |    8 +++++++-
>  git-send-email.perl              |   25 +++++++++++++++++++++++--
>  t/t9001-send-email.sh            |   18 ++++++++++++++++++
>  3 files changed, 48 insertions(+), 3 deletions(-)

Looks reasonable to me. Nice work getting on this quickly ;)

-mfm

-- 
GPG-Key: 9AFE00EA

Re: [PATCH V2] git-send-email.perl: Add --to-cmd

[Junio C Hamano]

Joe Perches <joe@perches.com> writes:

> +	if (defined $to_cmd) {
> +		open(F, "$to_cmd \Q$t\E |")
> +			or die "(to-cmd) Could not execute '$to_cmd'";
> +		while(<F>) {
> +			my $t = $_;

"my $t" masks another $t in the outer scope; technically not a bug, but
questionable as a style.

> +			$t =~ s/^\s*//g;
> +			$t =~ s/\n$//g;
> +			next if ($t eq $sender and $suppress_from);
> +			push @to, parse_address_line($t)
> +			    if defined $t; # sanitized/validated later

This "if defined $t" makes my head hurt.  Why?

 * The "while (<F>)" loop wouldn't have given you an undef in $t in the
   first place;

 * You would have got "Use of uninitialized value" warning at these two
   s/// statements if $t were undef; and

 * Even if $t were undef, these two s/// statements would have made $t a
   defined, empty string.

> +			printf("(to-cmd) Adding To: %s from: '%s'\n",
> +				$t, $to_cmd) unless $quiet;
> +		}
> +		close F
> +			or die "(to-cmd) failed to close pipe to '$to_cmd'";
> +	}

In any case, this whole codeblock obviously is a copy-and-paste from
corresponding $cc_cmd codepath, and I wonder if you can refactor the
original into a common helper function first and then use it to make the
addition of this feature a smaller patch.

	if (defined $cc_cmd) {
        	push @cc, recipients_cmd($cc_cmd, 'cc');
	}
        if (defined $to_cmd) {
	        push @to, recipients_cmd($to_cmd, 'to');
	}

If you did so, the first patch that refactors to create a helper function
can address issues Ævar raised in the review to clean things up, no?

I notice that you use parse_address_line() while $cc_cmd codepath doesn't.
I haven't studied other codepaths deeply, but my gut feeling is that the
reason why the $cc_cmd codepath does not call parse_address_line() before
pushing the result to @cc is _not_ because strings on @cc shouldn't be
sanitized (the codepath to parse "Cc: " calls parse_address_line and
pushes the result to @cc), but because the code is simply sloppy.  So I
suspect that it would be Ok for recipients_cmd to call parse_address_line
unconditionally.

Hmm?

Re: [PATCH V2] git-send-email.perl: Add --to-cmd

[Ævar Arnfjörð Bjarmason]

On Thu, Sep 23, 2010 at 22:37, Junio C Hamano <gitster@pobox.com> wrote:
> Joe Perches <joe@perches.com> writes:
>
>> +     if (defined $to_cmd) {
>> +             open(F, "$to_cmd \Q$t\E |")
>> +                     or die "(to-cmd) Could not execute '$to_cmd'";
>> +             while(<F>) {
>> +                     my $t = $_;
>
> "my $t" masks another $t in the outer scope; technically not a bug, but
> questionable as a style.
>
>> +                     $t =~ s/^\s*//g;
>> +                     $t =~ s/\n$//g;
>> +                     next if ($t eq $sender and $suppress_from);
>> +                     push @to, parse_address_line($t)
>> +                         if defined $t; # sanitized/validated later
>
> This "if defined $t" makes my head hurt.  Why?
>
>  * The "while (<F>)" loop wouldn't have given you an undef in $t in the
>   first place;
>
>  * You would have got "Use of uninitialized value" warning at these two
>   s/// statements if $t were undef; and
>
>  * Even if $t were undef, these two s/// statements would have made $t a
>   defined, empty string.

Well spotted. Also it *can't* be undef here by definition. Since $t is
taken from the $_ value which comes from the <> operator. That just
wraps readline(), which will exit the loop when it hit EOF (at which
point readline() *would* return undef).

So this whole business of checking for the definedness of $t doesn't
make any sense.

[PATCH V3] git-send-email.perl: Add --to-cmd

[Joe Perches]

Add the ability to use a command line --to-cmd=cmd
to create the list of "To:" addresses.

Used a shared routine for --cc-cmd and --to-cmd.

Did not use IPC::Open2, leaving that for Ævar if
ever he decides to fix the other bugs he might find.

Signed-off-by: Joe Perches <joe@perches.com>
---
 Documentation/git-send-email.txt |    8 +++++-
 git-send-email.perl              |   51 +++++++++++++++++++++++++------------
 t/t9001-send-email.sh            |   18 +++++++++++++
 3 files changed, 59 insertions(+), 18 deletions(-)

diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt
index c283084..fff97a3 100644
--- a/Documentation/git-send-email.txt
+++ b/Documentation/git-send-email.txt
@@ -97,7 +97,7 @@ See the CONFIGURATION section for 'sendemail.multiedit'.
 	Specify the primary recipient of the emails generated. Generally, this
 	will be the upstream maintainer of the project involved. Default is the
 	value of the 'sendemail.to' configuration value; if that is unspecified,
-	this will be prompted for.
+	and --to-cmd is not specified, this will be prompted for.
 +
 The --to option must be repeated for each user you want on the to list.
 
@@ -177,6 +177,12 @@ user is prompted for a password while the input is masked for privacy.
 Automating
 ~~~~~~~~~~
 
+--to-cmd=<command>::
+	Specify a command to execute once per patch file which
+	should generate patch file specific "To:" entries.
+	Output of this command must be single email address per line.
+	Default is the value of 'sendemail.tocmd' configuration value.
+
 --cc-cmd=<command>::
 	Specify a command to execute once per patch file which
 	should generate patch file specific "Cc:" entries.
diff --git a/git-send-email.perl b/git-send-email.perl
index 6dab3bf..e148269 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -70,6 +70,7 @@ git send-email [options] <file | directory | rev-list options >
 
   Automating:
     --identity              <str>  * Use the sendemail.<id> options.
+    --to-cmd                <str>  * Email To: via `<str> \$patch_path`
     --cc-cmd                <str>  * Email Cc: via `<str> \$patch_path`
     --suppress-cc           <str>  * author, self, sob, cc, cccmd, body, bodycc, all.
     --[no-]signed-off-by-cc        * Send to Signed-off-by: addresses. Default on.
@@ -187,7 +188,8 @@ sub do_edit {
 }
 
 # Variables with corresponding config settings
-my ($thread, $chain_reply_to, $suppress_from, $signed_off_by_cc, $cc_cmd);
+my ($thread, $chain_reply_to, $suppress_from, $signed_off_by_cc);
+my ($to_cmd, $cc_cmd);
 my ($smtp_server, $smtp_server_port, $smtp_authuser, $smtp_encryption);
 my ($identity, $aliasfiletype, @alias_files, @smtp_host_parts, $smtp_domain);
 my ($validate, $confirm);
@@ -214,6 +216,7 @@ my %config_settings = (
     "smtppass" => \$smtp_authpass,
 	"smtpdomain" => \$smtp_domain,
     "to" => \@to,
+    "tocmd" => \$to_cmd,
     "cc" => \@initial_cc,
     "cccmd" => \$cc_cmd,
     "aliasfiletype" => \$aliasfiletype,
@@ -272,6 +275,7 @@ my $rc = GetOptions("sender|from=s" => \$sender,
                     "in-reply-to=s" => \$initial_reply_to,
 		    "subject=s" => \$initial_subject,
 		    "to=s" => \@to,
+		    "to-cmd=s" => \$to_cmd,
 		    "no-to" => \$no_to,
 		    "cc=s" => \@initial_cc,
 		    "no-cc" => \$no_cc,
@@ -711,7 +715,7 @@ if (!defined $sender) {
 	$prompting++;
 }
 
-if (!@to) {
+if (!@to && !defined $to_cmd) {
 	my $to = ask("Who should the emails be sent to? ");
 	push @to, parse_address_line($to) if defined $to; # sanitized/validated later
 	$prompting++;
@@ -1238,21 +1242,10 @@ foreach my $t (@files) {
 	}
 	close F;
 
-	if (defined $cc_cmd && !$suppress_cc{'cccmd'}) {
-		open(F, "$cc_cmd \Q$t\E |")
-			or die "(cc-cmd) Could not execute '$cc_cmd'";
-		while(<F>) {
-			my $c = $_;
-			$c =~ s/^\s*//g;
-			$c =~ s/\n$//g;
-			next if ($c eq $sender and $suppress_from);
-			push @cc, $c;
-			printf("(cc-cmd) Adding cc: %s from: '%s'\n",
-				$c, $cc_cmd) unless $quiet;
-		}
-		close F
-			or die "(cc-cmd) failed to close pipe to '$cc_cmd'";
-	}
+	push @to, recipients_cmd("to-cmd", "to", $to_cmd, $t)
+		if defined $to_cmd;
+	push @cc, recipients_cmd("cc-cmd", "cc", $cc_cmd, $t)
+		if defined $cc_cmd && !$suppress_cc{'cccmd'};
 
 	if ($broken_encoding{$t} && !$has_content_type) {
 		$has_content_type = 1;
@@ -1310,6 +1303,30 @@ foreach my $t (@files) {
 	$message_id = undef;
 }
 
+# Execute a command (ie: $to_cmd) to get a list of email addresses
+# and return a results array
+sub recipients_cmd(@) {
+	my ($prefix, $what, $cmd, $file) = @_;
+
+	my $sanitized_sender = sanitize_address($sender);
+	my @addresses = ();
+	open(F, "$cmd \Q$file\E |")
+	    or die "($prefix) Could not execute '$cmd'";
+	while(<F>) {
+		my $address = $_;
+		$address =~ s/^\s*//g;
+		$address =~ s/\n$//g;
+		$address = sanitize_address($address);
+		next if ($address eq $sanitized_sender and $suppress_from);
+		push @addresses, $address;
+		printf("($prefix) Adding %s: %s from: '%s'\n",
+		       $what, $address, $cmd) unless $quiet;
+		}
+	close F
+	    or die "($prefix) failed to close pipe to '$cmd'";
+	return @addresses;
+}
+
 cleanup_compose_files();
 
 sub cleanup_compose_files() {
diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index 71b3df9..36cf421 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -201,6 +201,24 @@ test_expect_success $PREREQ 'Prompting works' '
 		grep "^To: to@example.com\$" msgtxt1
 '
 
+test_expect_success $PREREQ 'tocmd works' '
+	clean_fake_sendmail &&
+	cp $patches tocmd.patch &&
+	echo tocmd--tocmd@example.com >>tocmd.patch &&
+	{
+	  echo "#!$SHELL_PATH"
+	  echo sed -n -e s/^tocmd--//p \"\$1\"
+	} > tocmd-sed &&
+	chmod +x tocmd-sed &&
+	git send-email \
+		--from="Example <nobody@example.com>" \
+		--to-cmd=./tocmd-sed \
+		--smtp-server="$(pwd)/fake.sendmail" \
+		tocmd.patch \
+		&&
+	grep "^To: tocmd@example.com" msgtxt1
+'
+
 test_expect_success $PREREQ 'cccmd works' '
 	clean_fake_sendmail &&
 	cp $patches cccmd.patch &&

Re: [PATCH V3] git-send-email.perl: Add --to-cmd

[Jakub Narebski]

Joe Perches <joe@perches.com> writes:

> +# Execute a command (ie: $to_cmd) to get a list of email addresses
> +# and return a results array
> +sub recipients_cmd(@) {

Do not use subroutine prototypes: they do not do what you think they
do.  In this case using prototype is unnecessary and can be dangerous.

> +	my ($prefix, $what, $cmd, $file) = @_;
> +
> +	my $sanitized_sender = sanitize_address($sender);
> +	my @addresses = ();
> +	open(F, "$cmd \Q$file\E |")

For the future: use lexical filehandles instead of globals

  +	open(my $fh, "$cmd \Q$file\E |")


> +	    or die "($prefix) Could not execute '$cmd'";

You should use quote_command from gitweb/gitweb.perl (should probably
make it into Git.pm):

  # quote the given arguments for passing them to the shell
  # quote_command("command", "arg 1", "arg with ' and ! characters")
  # => "'command' 'arg 1' 'arg with '\'' and '\!' characters'"
  # Try to avoid using this function wherever possible.
  sub quote_command {
  	return join(' ',
  		map { my $a = $_; $a =~ s/(['!])/'\\$1'/g; "'$a'" } @_ );
  }

Or use String::ShellQuote :-)

But that is for a cleanup patch; you are using what it is already
there.

> +	while(<F>) {
> +		my $address = $_;
> +		$address =~ s/^\s*//g;
> +		$address =~ s/\n$//g;

Hmmm... why does it remove leading, but not trailing whitespace?

> +		$address = sanitize_address($address);
> +		next if ($address eq $sanitized_sender and $suppress_from);
> +		push @addresses, $address;
> +		printf("($prefix) Adding %s: %s from: '%s'\n",
> +		       $what, $address, $cmd) unless $quiet;
> +		}
> +	close F
> +	    or die "($prefix) failed to close pipe to '$cmd'";
> +	return @addresses;
> +}

-- 
Jakub Narebski
Poland
ShadeHawk on #git

Re: [PATCH V3] git-send-email.perl: Add --to-cmd

[Joe Perches]

On Fri, 2010-09-24 at 08:32 -0700, Jakub Narebski wrote:
> Joe Perches <joe@perches.com> writes:
> > +# Execute a command (ie: $to_cmd) to get a list of email addresses
> > +# and return a results array
> > +sub recipients_cmd(@) {
> Do not use subroutine prototypes: they do not do what you think they
> do.  In this case using prototype is unnecessary and can be dangerous.
 
It can be removed.  I was following the form of the
other returned array in the code.

sub unique_email_list(@) {

> > +	while(<F>) {
> > +		my $address = $_;
> > +		$address =~ s/^\s*//g;
> > +		$address =~ s/\n$//g;
> Hmmm... why does it remove leading, but not trailing whitespace?
 
Unmodified from the current.  I agree it should do both.

Re: [PATCH V3] git-send-email.perl: Add --to-cmd

[Ævar Arnfjörð Bjarmason]

On Fri, Sep 24, 2010 at 16:06, Joe Perches <joe@perches.com> wrote:
> On Fri, 2010-09-24 at 08:32 -0700, Jakub Narebski wrote:
>> Joe Perches <joe@perches.com> writes:
>> > +# Execute a command (ie: $to_cmd) to get a list of email addresses
>> > +# and return a results array
>> > +sub recipients_cmd(@) {
>> Do not use subroutine prototypes: they do not do what you think they
>> do.  In this case using prototype is unnecessary and can be dangerous.
>
> It can be removed.  I was following the form of the
> other returned array in the code.

While we generally follow the rule that you should use the style of
the existing code, I think resaonable to discard that when the
surrounding code with less than wizard-level understanding of the
language.

In any case most of git-send-email.perl doesn't use prototypes, it's
just 2-3 subs out of ~30.

[PATCH V4] git-send-email.perl: Add --to-cmd

[Joe Perches]

Add the ability to use a command line --to-cmd=cmd
to create the list of "To:" addresses.

Used a shared routine for --cc-cmd and --to-cmd.

Did not use IPC::Open2, leaving that for Ævar if
ever he decides to fix the other bugs he might find.

Signed-off-by: Joe Perches <joe@perches.com>
---
 Documentation/git-send-email.txt |    8 +++++-
 git-send-email.perl              |   51 +++++++++++++++++++++++++------------
 t/t9001-send-email.sh            |   18 +++++++++++++
 3 files changed, 59 insertions(+), 18 deletions(-)

diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt
index c283084..fff97a3 100644
--- a/Documentation/git-send-email.txt
+++ b/Documentation/git-send-email.txt
@@ -97,7 +97,7 @@ See the CONFIGURATION section for 'sendemail.multiedit'.
 	Specify the primary recipient of the emails generated. Generally, this
 	will be the upstream maintainer of the project involved. Default is the
 	value of the 'sendemail.to' configuration value; if that is unspecified,
-	this will be prompted for.
+	and --to-cmd is not specified, this will be prompted for.
 +
 The --to option must be repeated for each user you want on the to list.
 
@@ -177,6 +177,12 @@ user is prompted for a password while the input is masked for privacy.
 Automating
 ~~~~~~~~~~
 
+--to-cmd=<command>::
+	Specify a command to execute once per patch file which
+	should generate patch file specific "To:" entries.
+	Output of this command must be single email address per line.
+	Default is the value of 'sendemail.tocmd' configuration value.
+
 --cc-cmd=<command>::
 	Specify a command to execute once per patch file which
 	should generate patch file specific "Cc:" entries.
diff --git a/git-send-email.perl b/git-send-email.perl
index 6dab3bf..3acfdc2 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -70,6 +70,7 @@ git send-email [options] <file | directory | rev-list options >
 
   Automating:
     --identity              <str>  * Use the sendemail.<id> options.
+    --to-cmd                <str>  * Email To: via `<str> \$patch_path`
     --cc-cmd                <str>  * Email Cc: via `<str> \$patch_path`
     --suppress-cc           <str>  * author, self, sob, cc, cccmd, body, bodycc, all.
     --[no-]signed-off-by-cc        * Send to Signed-off-by: addresses. Default on.
@@ -187,7 +188,8 @@ sub do_edit {
 }
 
 # Variables with corresponding config settings
-my ($thread, $chain_reply_to, $suppress_from, $signed_off_by_cc, $cc_cmd);
+my ($thread, $chain_reply_to, $suppress_from, $signed_off_by_cc);
+my ($to_cmd, $cc_cmd);
 my ($smtp_server, $smtp_server_port, $smtp_authuser, $smtp_encryption);
 my ($identity, $aliasfiletype, @alias_files, @smtp_host_parts, $smtp_domain);
 my ($validate, $confirm);
@@ -214,6 +216,7 @@ my %config_settings = (
     "smtppass" => \$smtp_authpass,
 	"smtpdomain" => \$smtp_domain,
     "to" => \@to,
+    "tocmd" => \$to_cmd,
     "cc" => \@initial_cc,
     "cccmd" => \$cc_cmd,
     "aliasfiletype" => \$aliasfiletype,
@@ -272,6 +275,7 @@ my $rc = GetOptions("sender|from=s" => \$sender,
                     "in-reply-to=s" => \$initial_reply_to,
 		    "subject=s" => \$initial_subject,
 		    "to=s" => \@to,
+		    "to-cmd=s" => \$to_cmd,
 		    "no-to" => \$no_to,
 		    "cc=s" => \@initial_cc,
 		    "no-cc" => \$no_cc,
@@ -711,7 +715,7 @@ if (!defined $sender) {
 	$prompting++;
 }
 
-if (!@to) {
+if (!@to && !defined $to_cmd) {
 	my $to = ask("Who should the emails be sent to? ");
 	push @to, parse_address_line($to) if defined $to; # sanitized/validated later
 	$prompting++;
@@ -1238,21 +1242,10 @@ foreach my $t (@files) {
 	}
 	close F;
 
-	if (defined $cc_cmd && !$suppress_cc{'cccmd'}) {
-		open(F, "$cc_cmd \Q$t\E |")
-			or die "(cc-cmd) Could not execute '$cc_cmd'";
-		while(<F>) {
-			my $c = $_;
-			$c =~ s/^\s*//g;
-			$c =~ s/\n$//g;
-			next if ($c eq $sender and $suppress_from);
-			push @cc, $c;
-			printf("(cc-cmd) Adding cc: %s from: '%s'\n",
-				$c, $cc_cmd) unless $quiet;
-		}
-		close F
-			or die "(cc-cmd) failed to close pipe to '$cc_cmd'";
-	}
+	push @to, recipients_cmd("to-cmd", "to", $to_cmd, $t)
+		if defined $to_cmd;
+	push @cc, recipients_cmd("cc-cmd", "cc", $cc_cmd, $t)
+		if defined $cc_cmd && !$suppress_cc{'cccmd'};
 
 	if ($broken_encoding{$t} && !$has_content_type) {
 		$has_content_type = 1;
@@ -1310,6 +1303,30 @@ foreach my $t (@files) {
 	$message_id = undef;
 }
 
+# Execute a command (ie: $to_cmd) to get a list of email addresses
+# and return a results array
+sub recipients_cmd {
+	my ($prefix, $what, $cmd, $file) = @_;
+
+	my $sanitized_sender = sanitize_address($sender);
+	my @addresses = ();
+	open(F, "$cmd \Q$file\E |")
+	    or die "($prefix) Could not execute '$cmd'";
+	while(<F>) {
+		my $address = $_;
+		$address =~ s/^\s*//g;
+		$address =~ s/\s*$//g;
+		$address = sanitize_address($address);
+		next if ($address eq $sanitized_sender and $suppress_from);
+		push @addresses, $address;
+		printf("($prefix) Adding %s: %s from: '%s'\n",
+		       $what, $address, $cmd) unless $quiet;
+		}
+	close F
+	    or die "($prefix) failed to close pipe to '$cmd'";
+	return @addresses;
+}
+
 cleanup_compose_files();
 
 sub cleanup_compose_files() {
diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index 71b3df9..36cf421 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -201,6 +201,24 @@ test_expect_success $PREREQ 'Prompting works' '
 		grep "^To: to@example.com\$" msgtxt1
 '
 
+test_expect_success $PREREQ 'tocmd works' '
+	clean_fake_sendmail &&
+	cp $patches tocmd.patch &&
+	echo tocmd--tocmd@example.com >>tocmd.patch &&
+	{
+	  echo "#!$SHELL_PATH"
+	  echo sed -n -e s/^tocmd--//p \"\$1\"
+	} > tocmd-sed &&
+	chmod +x tocmd-sed &&
+	git send-email \
+		--from="Example <nobody@example.com>" \
+		--to-cmd=./tocmd-sed \
+		--smtp-server="$(pwd)/fake.sendmail" \
+		tocmd.patch \
+		&&
+	grep "^To: tocmd@example.com" msgtxt1
+'
+
 test_expect_success $PREREQ 'cccmd works' '
 	clean_fake_sendmail &&
 	cp $patches cccmd.patch &&

Re: [PATCH V2] git-send-email.perl: Add --to-cmd

[Joe Perches]

On Thu, 2010-09-23 at 15:37 -0700, Junio C Hamano wrote:
> Joe Perches <joe@perches.com> writes:
> 
> > +	if (defined $to_cmd) {
> > +		open(F, "$to_cmd \Q$t\E |")
> > +			or die "(to-cmd) Could not execute '$to_cmd'";
> > +		while(<F>) {
> > +			my $t = $_;
> 
> "my $t" masks another $t in the outer scope; technically not a bug, but
> questionable as a style.
> 
> > +			$t =~ s/^\s*//g;
> > +			$t =~ s/\n$//g;
> > +			next if ($t eq $sender and $suppress_from);
> > +			push @to, parse_address_line($t)
> > +			    if defined $t; # sanitized/validated later
> 
> This "if defined $t" makes my head hurt.  Why?
> 
>  * The "while (<F>)" loop wouldn't have given you an undef in $t in the
>    first place;
> 
>  * You would have got "Use of uninitialized value" warning at these two
>    s/// statements if $t were undef; and
> 
>  * Even if $t were undef, these two s/// statements would have made $t a
>    defined, empty string.

all true.

> > +			printf("(to-cmd) Adding To: %s from: '%s'\n",
> > +				$t, $to_cmd) unless $quiet;
> > +		}
> > +		close F
> > +			or die "(to-cmd) failed to close pipe to '$to_cmd'";
> > +	}
> 
> In any case, this whole codeblock obviously is a copy-and-paste from
> corresponding $cc_cmd codepath, and I wonder if you can refactor the
> original into a common helper function first and then use it to make the
> addition of this feature a smaller patch.
> 
> 	if (defined $cc_cmd) {
>         	push @cc, recipients_cmd($cc_cmd, 'cc');
> 	}
>         if (defined $to_cmd) {
> 	        push @to, recipients_cmd($to_cmd, 'to');
> 	}

Overall, I believe it'll be more code, but all right.

> If you did so, the first patch that refactors to create a helper function
> can address issues Ævar raised in the review to clean things up, no?

> I notice that you use parse_address_line() while $cc_cmd codepath doesn't.
> I haven't studied other codepaths deeply, but my gut feeling is that the
> reason why the $cc_cmd codepath does not call parse_address_line() before
> pushing the result to @cc is _not_ because strings on @cc shouldn't be
> sanitized (the codepath to parse "Cc: " calls parse_address_line and
> pushes the result to @cc), but because the code is simply sloppy.

Probably, I wrote some of those lines...