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
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
[PATCH] git-send-email.perl: implement suggestions made by perlcritic
Running perlcritic with gentle severity reports six problems. The
following lists the line numbers on which the problems occur, along
with a description of the problem. This patch fixes them all.
516: Contrary to common belief, subroutine prototypes do not enable
compile-time checks for proper arguments. They serve the singular
purpose of defining functions that behave like built-in functions, but
check_file_rev_conflict was never intended to behave like one.
714, 836, 855, 1487: Returning `undef' upon failure from a subroutine
is pretty common. But if the subroutine is called in list context, an
explicit `return undef;' statement will return a one-element list
containing `(undef)'. Now if that list is subsequently put in a
boolean context to test for failure, then it evaluates to true. But
you probably wanted it to be false. The solution is to just use a
bare `return' statement whenever you want to return failure. In list
context, Perl will then give you an empty list (which is false), and
`undef' in scalar context (which is also false).
1441: The three-argument form of `open' (introduced in Perl 5.6)
prevents subtle bugs that occur when the filename starts with funny
characters like '' or ''. It's also more explicitly clear to define
the input mode of the file. This policy will not complain if the file
explicitly states that it is compatible with a version of Perl prior
to 5.6 via an include statement (see 'use 5.008' near the top of the
file).
Signed-off-by: Ramkumar Ramachandra artag...@gmail.com
---
I was looking at git-send-email.perl after I posted that bug report
about SSL_verify. What better way to get started than Perl Best
Practices? Here's a nice patch to fix some obvious style issues. I
didn't run perlcritic on a higher severity, because I don't know
enough Perl to have a well-formed opinion of my own; I'm just fixing
the glaringly obvious problems.
git-send-email.perl | 12 ++--
1 file changed, 6 insertions(+), 6 deletions(-)
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 {
return unless $repo;
my $f = shift;
try {
@@ -711,7 +711,7 @@ sub ask {
}
}
}
- return undef;
+ return;
}
my %broken_encoding;
@@ -833,7 +833,7 @@ sub extract_valid_address {
# less robust/correct than the monster regexp in Email::Valid,
# but still does a 99% job, and one less dependency
return $1 if $address =~ /($local_part_regexp\@$domain_regexp)/;
- return undef;
+ return;
}
sub extract_valid_address_or_die {
@@ -852,7 +852,7 @@ sub validate_address {
valid_re = qr/^(?:quit|q|drop|d|edit|e)/i,
default = 'q');
if (/^d/i) {
- return undef;
+ return;
} elsif (/^q/i) {
cleanup_compose_files();
exit(0);
@@ -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
or die ($prefix) Could not execute '$cmd';
while (my $address = $fh) {
$address =~ s/^\s*//g;
@@ -1484,7 +1484,7 @@ sub validate_patch {
return $.: patch contains a line longer than 998
characters;
}
}
- return undef;
+ return;
}
sub file_has_nonascii {
--
1.8.2.62.ga35d936.dirty
--
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 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
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
