Re: [PATCH] git-send-email.perl: implement suggestions made by perlcritic
Ramkumar Ramachandra writes: > I don't see this queued in 'pu'. Do you have any objections to the > patch, or did you just forget? I do not recall the details but I think I was expecting a re-roll updating the log message (if the original invited questions, there must have been some room for improvement to avoid such confusions, right?) for a few days since the exchange, and then forgot. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-send-email.perl: implement suggestions made by perlcritic
Junio, I don't see this queued in 'pu'. Do you have any objections to the patch, or did you just forget? Thanks. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-send-email.perl: implement suggestions made by perlcritic
Junio C Hamano wrote:
> Ramkumar Ramachandra 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.
No Junio, I haven't blindly followed the perlcritic output. I've
considered the ramifications, audited the callers, and run the tests
to make sure that nothing breaks.
>> @@ -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
Intentional. I thought it was a pretty way to differentiate between a
mode string (which can only be <, >, or -|) and a filename string. If
you don't share my taste, feel free to use quotes instead.
> , 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.
Yes, the file can be improved in many ways, but that is not the topic
of this series. This series just makes the changes suggested by
perlcritic gentle.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-send-email.perl: implement suggestions made by perlcritic
Ramkumar Ramachandra 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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
