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

Ramkumar Ramachandra [off-list ref] writes:

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.

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.

Junio C Hamano wrote:

Ramkumar Ramachandra [off-list ref] writes:

quoted

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.

quoted

@@ -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.

Junio,

I don't see this queued in 'pu'.  Do you have any objections to the
patch, or did you just forget?

Thanks.