Re: [PATCH] git-send-email.perl: implement suggestions made by perlcritic
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Mar 21, 2013, 15:29 UTC
- Message-ID
- <7vd2usbwkn.fsf@alter.siamese.dyndns.org>
- In-Reply-To
- <1363869587-10462-1-git-send-email-artagnon@gmail.com>
Ramkumar Ramachandra <artagnon@gmail.com> writes:
Show 10 quoted lines
> 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.
Show 6 quoted lines
> @@ -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.