Junio C Hamano wrote:
> Ramkumar Ramachandra <artag...@gmail.com> 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

Reply via email to