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

2013-03-27 Thread Ramkumar Ramachandra 
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

2013-03-27 Thread Junio C Hamano 
Ramkumar Ramachandra artag...@gmail.com 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

2013-03-21 Thread Junio C Hamano 
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.

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

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

2013-03-21 Thread Ramkumar Ramachandra 
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