Joe PerchesOn 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.
Joe Perches[RFC PATCH] sit-send-email.pl: Add --to-cmd
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'";
Ævar Arnfjörð BjarmasonRe: [RFC PATCH] sit-send-email.pl: Add --to-cmd
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.
Joe PerchesRe: [RFC PATCH] sit-send-email.pl: Add --to-cmd
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.
Ævar Arnfjörð BjarmasonRe: [RFC PATCH] sit-send-email.pl: Add --to-cmd
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...
Joe Perches[PATCH V2] git-send-email.perl: Add --to-cmd
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
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
Junio C HamanoRe: [PATCH V2] git-send-email.perl: Add --to-cmd
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?
Ævar Arnfjörð BjarmasonRe: [PATCH V2] git-send-email.perl: Add --to-cmd
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.
Joe Perches[PATCH V3] git-send-email.perl: Add --to-cmd
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 &&
Jakub NarebskiRe: [PATCH V3] git-send-email.perl: Add --to-cmd
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
Joe PerchesRe: [PATCH V3] git-send-email.perl: Add --to-cmd
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.
Ævar Arnfjörð BjarmasonRe: [PATCH V3] git-send-email.perl: Add --to-cmd
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.
Joe Perches[PATCH V4] git-send-email.perl: Add --to-cmd
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 &&
Joe PerchesRe: [PATCH V2] git-send-email.perl: Add --to-cmd
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...