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

Ramkumar Ramachandra [off-list ref] 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.

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, after
carefully considering the consequences.

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.  We
have verified that the callers of the subroutines are alright with the
change.

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 <redacted>
---
 Junio: In future, please tell me explicitly that you're expecting a
 re-roll with an updated commit message.  It wasn't obvious to me at
 all.

 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 c3501d9..633f447 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);

@@ -1453,7 +1453,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;

@@ -1499,7 +1499,7 @@ sub validate_patch {
 			return "$.: patch contains a line longer than 998 characters";
 		}
 	}
-	return undef;
+	return;
 }
 
 sub file_has_nonascii {

-- 
1.8.2.141.g07926c6

Ramkumar Ramachandra wrote:

 Junio: In future, please tell me explicitly that you're expecting a
 re-roll with an updated commit message.  It wasn't obvious to me at
 all.

When there are questions in response to a patch, there are two
possibilities:

 * temporary brainfart --- sorry for the bother
 * the clarity of the patch or commit message has room for improvement

This wasn't a case of the former, so a seasoned contributor could
safely assume that it was definitely a case of the latter.

The explanation in Linux kernel's Documentation/SubmittingPatches
item 10 ("Don't get discouraged.  Re-submit") has some fitting advice.

Hope that helps,
Jonathan