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

From: Jakub Narebski <jnareb@gmail.com>
Date: Sep 24, 2010, 15:32 UTC
Message-ID: <m3lj6rgnub.fsf@localhost.localdomain>
In-Reply-To: <1285291098.25928.220.camel@Joe-Laptop>

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.

Show 5 quoted lines

> +	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?

Show 10 quoted lines

> +		$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