From 622bc9309155b05af1a0d85bfb643bf6280eba35 Mon Sep 17 00:00:00 2001 From: Ramkumar Ramachandra Date: Sun, 31 Mar 2013 18:40:40 -0700 Subject: send-email: use "return;" not "return undef;" on error codepaths All the callers of "ask", "extract_valid_address", and "validate_patch" subroutines assign the return values from them to a single scalar: $var = subr(...); and "return undef;" in these subroutine can safely be turned into a simpler "return;". Doing so will also future-proof a new caller that mistakenly does this: @foo = ask(...); if (@foo) { ... we got an answer ... } else { ... we did not ... } Note that we leave "return undef;" in validate_address on purpose, even though Perlcritic may complain. The primary "return" site of the function returns whatever is in the scalar variable $address, so it is pointless to change only the other "return undef;" to "return". The caller must be prepared to see an array with a single undef as the return value from this subroutine anyway. Signed-off-by: Ramkumar Ramachandra Signed-off-by: Junio C Hamano --- git-send-email.perl | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/git-send-email.perl b/git-send-email.perl index be809e5b59..79cc5bee97 100755 --- a/git-send-email.perl +++ b/git-send-email.perl @@ -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 { @@ -1484,7 +1484,7 @@ sub validate_patch { return "$.: patch contains a line longer than 998 characters"; } } - return undef; + return; } sub file_has_nonascii { -- cgit v1.2.3 From 9b39703920b2d64985abcc1348b169d8fa658c24 Mon Sep 17 00:00:00 2001 From: Ramkumar Ramachandra Date: Sun, 31 Mar 2013 18:40:41 -0700 Subject: send-email: drop misleading function prototype The subroutine check_file_rev_conflict() is called from two places, both of which expects to pass a single scalar variable and see if that can be interpreted as a pathname or a revision name. It is defined with a function prototype ($) to force a scalar context while evaluating the arguments at the calling site but it does not help the current calling sites. The only effect it has is to hurt future calling sites that may want to build an argument list in an array variable and call it as check_file_rev_confict(@args). Drop the misleading prototype, as Perlcritic suggests. While at it, rename the function to avoid new call sites unaware of this change arising and add a comment clarifying what this function is for. Signed-off-by: Ramkumar Ramachandra Signed-off-by: Jonathan Nieder Signed-off-by: Junio C Hamano --- git-send-email.perl | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/git-send-email.perl b/git-send-email.perl index 79cc5bee97..fd8bfff3b2 100755 --- a/git-send-email.perl +++ b/git-send-email.perl @@ -512,8 +512,9 @@ 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($) { +# is_format_patch_arg($f) returns 0 if $f names a patch, or 1 if +# $f is a revision list specification to be passed to format-patch. +sub is_format_patch_arg { return unless $repo; my $f = shift; try { @@ -529,6 +530,7 @@ to produce patches for. Please disambiguate by... * Giving --format-patch option if you mean a range. EOF } catch Git::Error::Command with { + # Not a valid revision. Treat it as a filename. return 0; } } @@ -540,14 +542,14 @@ while (defined(my $f = shift @ARGV)) { if ($f eq "--") { push @rev_list_opts, "--", @ARGV; @ARGV = (); - } elsif (-d $f and !check_file_rev_conflict($f)) { + } elsif (-d $f and !is_format_patch_arg($f)) { opendir my $dh, $f or die "Failed to opendir $f: $!"; push @files, grep { -f $_ } map { catfile($f, $_) } sort readdir $dh; closedir $dh; - } elsif ((-f $f or -p $f) and !check_file_rev_conflict($f)) { + } elsif ((-f $f or -p $f) and !is_format_patch_arg($f)) { push @files, $f; } else { push @rev_list_opts, $f; -- cgit v1.2.3 From a47eab03f613fa55b9e690d5354e95bc165dceee Mon Sep 17 00:00:00 2001 From: Ramkumar Ramachandra Date: Sun, 31 Mar 2013 18:40:42 -0700 Subject: send-email: use the three-arg form of open in recipients_cmd Perlcritic does not want to see the trailing pipe in the two-args form of open(), i.e. open my $fh, "$cmd \Q$file\E |"; If $cmd were a single-token command name, it would make a lot more sense to use four-or-more-args form "open FILEHANDLE,MODE,CMD,ARGS" to avoid shell from expanding metacharacters in $file, but we do expect multi-word string in $to_cmd and $cc_cmd to be expanded by the shell, so we cannot rewrite it to open my $fh, "-|", $cmd, $file; for extra safety. At least, by using this in the three-arg form: open my $fh, "-|", "$cmd \Q$file\E"; we can silence Perlcritic, even though we do not gain much safety by doing so. Signed-off-by: Ramkumar Ramachandra Signed-off-by: Junio C Hamano --- git-send-email.perl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/git-send-email.perl b/git-send-email.perl index fd8bfff3b2..70cad15ec4 100755 --- a/git-send-email.perl +++ b/git-send-email.perl @@ -1440,7 +1440,7 @@ sub recipients_cmd { my $sanitized_sender = sanitize_address($sender); my @addresses = (); - open my $fh, "$cmd \Q$file\E |" + open my $fh, "-|", "$cmd \Q$file\E" or die "($prefix) Could not execute '$cmd'"; while (my $address = <$fh>) { $address =~ s/^\s*//g; -- cgit v1.2.3