Re: [PATCH] git-send-email.perl: implement suggestions made by perlcritic

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Ramkumar Ramachandra <artagnon@xxxxxxxxx> writes:

> diff --git a/git-send-email.perl b/git-send-email.perl
> index be809e5..e974b11 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -513,7 +513,7 @@ if (@alias_files and $aliasfiletype and defined $parse_alias{$aliasfiletype}) {
>  ($sender) = expand_aliases($sender) if defined $sender;
>  
>  # returns 1 if the conflict must be solved using it as a format-patch argument
> -sub check_file_rev_conflict($) {
> +sub check_file_rev_conflict {

Have you verified that the callers of this sub are OK with this
change?  It used to force scalar context on its arguments but now it
does not.

I am not saying I know the callers will get broken.  I am trying to
make sure that this is *not* the result of blindly following
perlcritic output without understanding the ramifications of the
change.

> @@ -1438,7 +1438,7 @@ sub recipients_cmd {
>  
>  	my $sanitized_sender = sanitize_address($sender);
>  	my @addresses = ();
> -	open my $fh, "$cmd \Q$file\E |"
> +	open my $fh, q{-|}, "$cmd \Q$file\E"

Strange quoting (why not just say "-|"?) aside, if you are moving
away from the two-param form of open(), it would be a sane thing to
do to also stop concatenating the arguments to commands to avoid
shell metacharacter gotcha.  It will make the resulting code much
safer.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]