Skip to content

Commit

Permalink
Merge branch 'rr/send-email-perl-critique' into jch
Browse files Browse the repository at this point in the history
Broken down from an earlier discussion to pick up reasonable bits
with explanation, to demonstrate how it should be done.

* rr/send-email-perl-critique:
  send-email: use the three-arg form of open in recipients_cmd
  send-email: drop misleading function prototype
  send-email: use "return;" not "return undef;" on error codepaths
  • Loading branch information
gitster committed Apr 4, 2013
2 parents 3d020b3 + a47eab0 commit 1a64761
Showing 1 changed file with 10 additions and 8 deletions.
18 changes: 10 additions & 8 deletions git-send-email.perl
Original file line number Original file line Diff line number Diff line change
Expand Up @@ -512,8 +512,9 @@ sub split_addrs {


($sender) = expand_aliases($sender) if defined $sender; ($sender) = expand_aliases($sender) if defined $sender;


# returns 1 if the conflict must be solved using it as a format-patch argument # is_format_patch_arg($f) returns 0 if $f names a patch, or 1 if
sub check_file_rev_conflict($) { # $f is a revision list specification to be passed to format-patch.
sub is_format_patch_arg {
return unless $repo; return unless $repo;
my $f = shift; my $f = shift;
try { try {
Expand All @@ -529,6 +530,7 @@ ($)
* Giving --format-patch option if you mean a range. * Giving --format-patch option if you mean a range.
EOF EOF
} catch Git::Error::Command with { } catch Git::Error::Command with {
# Not a valid revision. Treat it as a filename.
return 0; return 0;
} }
} }
Expand All @@ -540,14 +542,14 @@ ($)
if ($f eq "--") { if ($f eq "--") {
push @rev_list_opts, "--", @ARGV; push @rev_list_opts, "--", @ARGV;
@ARGV = (); @ARGV = ();
} elsif (-d $f and !check_file_rev_conflict($f)) { } elsif (-d $f and !is_format_patch_arg($f)) {
opendir my $dh, $f opendir my $dh, $f
or die "Failed to opendir $f: $!"; or die "Failed to opendir $f: $!";


push @files, grep { -f $_ } map { catfile($f, $_) } push @files, grep { -f $_ } map { catfile($f, $_) }
sort readdir $dh; sort readdir $dh;
closedir $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; push @files, $f;
} else { } else {
push @rev_list_opts, $f; push @rev_list_opts, $f;
Expand Down Expand Up @@ -711,7 +713,7 @@ sub ask {
} }
} }
} }
return undef; return;
} }


my %broken_encoding; my %broken_encoding;
Expand Down Expand Up @@ -833,7 +835,7 @@ sub extract_valid_address {
# less robust/correct than the monster regexp in Email::Valid, # less robust/correct than the monster regexp in Email::Valid,
# but still does a 99% job, and one less dependency # but still does a 99% job, and one less dependency
return $1 if $address =~ /($local_part_regexp\@$domain_regexp)/; return $1 if $address =~ /($local_part_regexp\@$domain_regexp)/;
return undef; return;
} }


sub extract_valid_address_or_die { sub extract_valid_address_or_die {
Expand Down Expand Up @@ -1453,7 +1455,7 @@ sub recipients_cmd {


my $sanitized_sender = sanitize_address($sender); my $sanitized_sender = sanitize_address($sender);
my @addresses = (); my @addresses = ();
open my $fh, "$cmd \Q$file\E |" open my $fh, "-|", "$cmd \Q$file\E"
or die "($prefix) Could not execute '$cmd'"; or die "($prefix) Could not execute '$cmd'";
while (my $address = <$fh>) { while (my $address = <$fh>) {
$address =~ s/^\s*//g; $address =~ s/^\s*//g;
Expand Down Expand Up @@ -1499,7 +1501,7 @@ sub validate_patch {
return "$.: patch contains a line longer than 998 characters"; return "$.: patch contains a line longer than 998 characters";
} }
} }
return undef; return;
} }


sub file_has_nonascii { sub file_has_nonascii {
Expand Down

0 comments on commit 1a64761

Please sign in to comment.