[PATCH] send-email: improve recipients_cmd()

[PATCH] send-email: improve recipients_cmd()

[Felipe Contreras]

We don't need to quote the filename to pass to the command, we can use
an array of all the arguments to pass to the command, which is safer,
and more extensible.

Commit a47eab0 (send-email: use the three-arg form of open in
recipients_cmd) stated we couldn't pass $file directly, but in fact, we
can, the multi-word string is passed as is, and we can pass an array
too.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 git-send-email.perl | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index bd13cc8..7880d12 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -1452,11 +1452,11 @@ foreach my $t (@files) {
 # Execute a command (e.g. $to_cmd) to get a list of email addresses
 # and return a results array
 sub recipients_cmd {
-	my ($prefix, $what, $cmd, $file) = @_;
+	my ($prefix, $what, $cmd, @args) = @_;
 
 	my $sanitized_sender = sanitize_address($sender);
 	my @addresses = ();
-	open my $fh, "-|", "$cmd \Q$file\E"
+	open my $fh, "-|", $cmd, @args
 	    or die "($prefix) Could not execute '$cmd'";
 	while (my $address = <$fh>) {
 		$address =~ s/^\s*//g;
-- 
1.8.2.1.1031.g2ee5873

Re: [PATCH] send-email: improve recipients_cmd()

[Junio C Hamano]

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

> We don't need to quote the filename to pass to the command, we can use
> an array of all the arguments to pass to the command, which is safer,
> and more extensible.
>
> Commit a47eab0 (send-email: use the three-arg form of open in
> recipients_cmd) stated we couldn't pass $file directly, but in fact, we
> can, the multi-word string is passed as is, and we can pass an array
> too.

I think the comment is not about passing $file directly, but is
about passing $cmd that could be multi-word string directly.  The
caller expects it be split into command and its earlier part of
command line parameters, so that you can say

    $cmd = "cccmd --frotz --nitfol"

but the non-string form of open would not give you that, unless you
rewrite it to

	open $fh, "-|", qw(sh -c), $cmd, @args

or something, no?

> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  git-send-email.perl | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/git-send-email.perl b/git-send-email.perl
> index bd13cc8..7880d12 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -1452,11 +1452,11 @@ foreach my $t (@files) {
>  # Execute a command (e.g. $to_cmd) to get a list of email addresses
>  # and return a results array
>  sub recipients_cmd {
> -	my ($prefix, $what, $cmd, $file) = @_;
> +	my ($prefix, $what, $cmd, @args) = @_;
>  
>  	my $sanitized_sender = sanitize_address($sender);
>  	my @addresses = ();
> -	open my $fh, "-|", "$cmd \Q$file\E"
> +	open my $fh, "-|", $cmd, @args
>  	    or die "($prefix) Could not execute '$cmd'";
>  	while (my $address = <$fh>) {
>  		$address =~ s/^\s*//g;

Re: [PATCH] send-email: improve recipients_cmd()

[Felipe Contreras]

On Sun, Apr 28, 2013 at 2:18 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> We don't need to quote the filename to pass to the command, we can use
>> an array of all the arguments to pass to the command, which is safer,
>> and more extensible.
>>
>> Commit a47eab0 (send-email: use the three-arg form of open in
>> recipients_cmd) stated we couldn't pass $file directly, but in fact, we
>> can, the multi-word string is passed as is, and we can pass an array
>> too.
>
> I think the comment is not about passing $file directly, but is
> about passing $cmd that could be multi-word string directly.  The
> caller expects it be split into command and its earlier part of
> command line parameters, so that you can say
>
>     $cmd = "cccmd --frotz --nitfol"

I see.

> but the non-string form of open would not give you that, unless you
> rewrite it to
>
>         open $fh, "-|", qw(sh -c), $cmd, @args

That doesn't seem to work for me.

It would have to be:

  open $fh, "-|", qw(sh -c), "$cmd \Q$args\E"

So we end up in the same place.

-- 
Felipe Contreras

Re: [PATCH] send-email: improve recipients_cmd()

[Junio C Hamano]

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

>>         open $fh, "-|", qw(sh -c), $cmd, @args
>
> That doesn't seem to work for me.

My fault.  It needs to form a command line like this:

	sh -c 'cccmd --frotz --nitfol "$@"' - a r g s

[jc: goes and tries
	$ sh -c 'echo X Y "$@"' - a r g s
	X Y a r g s
]

so if we want to get rid of \Q\E, it would be:

	open $fh, '-|', qw(sh -c), "$cmd " . '"$@"', '-', @args

Re: [PATCH] send-email: improve recipients_cmd()

[Felipe Contreras]

On Mon, Apr 29, 2013 at 12:24 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>>>         open $fh, "-|", qw(sh -c), $cmd, @args
>>
>> That doesn't seem to work for me.
>
> My fault.  It needs to form a command line like this:
>
>         sh -c 'cccmd --frotz --nitfol "$@"' - a r g s
>
> [jc: goes and tries
>         $ sh -c 'echo X Y "$@"' - a r g s
>         X Y a r g s
> ]
>
> so if we want to get rid of \Q\E, it would be:
>
>         open $fh, '-|', qw(sh -c), "$cmd " . '"$@"', '-', @args
>

I don't know if that would be better, or converting @args to a list of
quoted strings, essentially keeping the current behavior.


-- 
Felipe Contreras

Re: [PATCH] send-email: improve recipients_cmd()

[Junio C Hamano]

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

>> so if we want to get rid of \Q\E, it would be:
>>
>>         open $fh, '-|', qw(sh -c), "$cmd " . '"$@"', '-', @args
>>
>
> I don't know if that would be better, or converting @args to a list of
> quoted strings, essentially keeping the current behavior.

I tend to agree, so let's drop it for now.