Merge branch 'rr/send-email-perl-critique'
authorJunio C Hamano <gitster@pobox.com>
Fri, 5 Apr 2013 21:14:48 +0000 (14:14 -0700)
committerJunio C Hamano <gitster@pobox.com>
Fri, 5 Apr 2013 21:14:49 +0000 (14:14 -0700)
Update "git send-email" for issues noticed by PerlCritic.

* 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

1  2 
git-send-email.perl
diff --combined git-send-email.perl
index c3501d987e66d330189f156590316d335b580aaf,70cad15ec46d0ed32f3b97c792e9d975ece53fd8..729747281864155bee7ef009de3ab92cc7643a89
@@@ -512,8 -512,9 +512,9 @@@ if (@alias_files and $aliasfiletype an
  
  ($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 +530,7 @@@ to produce patches for.  Please disambi
      * 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 +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;
@@@ -711,7 -713,7 +713,7 @@@ sub ask 
                        }
                }
        }
-       return undef;
+       return;
  }
  
  my %broken_encoding;
@@@ -833,7 -835,7 +835,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 {
@@@ -1045,47 -1047,6 +1047,47 @@@ sub maildomain 
        return maildomain_net() || maildomain_mta() || 'localhost.localdomain';
  }
  
 +sub smtp_host_string {
 +      if (defined $smtp_server_port) {
 +              return "$smtp_server:$smtp_server_port";
 +      } else {
 +              return $smtp_server;
 +      }
 +}
 +
 +# Returns 1 if authentication succeeded or was not necessary
 +# (smtp_user was not specified), and 0 otherwise.
 +
 +sub smtp_auth_maybe {
 +      if (!defined $smtp_authuser || $auth) {
 +              return 1;
 +      }
 +
 +      # Workaround AUTH PLAIN/LOGIN interaction defect
 +      # with Authen::SASL::Cyrus
 +      eval {
 +              require Authen::SASL;
 +              Authen::SASL->import(qw(Perl));
 +      };
 +
 +      # TODO: Authentication may fail not because credentials were
 +      # invalid but due to other reasons, in which we should not
 +      # reject credentials.
 +      $auth = Git::credential({
 +              'protocol' => 'smtp',
 +              'host' => smtp_host_string(),
 +              'username' => $smtp_authuser,
 +              # if there's no password, "git credential fill" will
 +              # give us one, otherwise it'll just pass this one.
 +              'password' => $smtp_authpass
 +      }, sub {
 +              my $cred = shift;
 +              return !!$smtp->auth($cred->{'username'}, $cred->{'password'});
 +      });
 +
 +      return $auth;
 +}
 +
  # Returns 1 if the message was sent, and 0 otherwise.
  # In actuality, the whole program dies when there
  # is an error sending a message.
@@@ -1196,7 -1157,9 +1198,7 @@@ X-Mailer: git-send-email $gitversio
                else {
                        require Net::SMTP;
                        $smtp_domain ||= maildomain();
 -                      $smtp ||= Net::SMTP->new((defined $smtp_server_port)
 -                                               ? "$smtp_server:$smtp_server_port"
 -                                               : $smtp_server,
 +                      $smtp ||= Net::SMTP->new(smtp_host_string(),
                                                 Hello => $smtp_domain,
                                                 Debug => $debug_net_smtp);
                        if ($smtp_encryption eq 'tls' && $smtp) {
                            defined $smtp_server_port ? " port=$smtp_server_port" : "";
                }
  
 -              if (defined $smtp_authuser) {
 -                      # Workaround AUTH PLAIN/LOGIN interaction defect
 -                      # with Authen::SASL::Cyrus
 -                      eval {
 -                              require Authen::SASL;
 -                              Authen::SASL->import(qw(Perl));
 -                      };
 -
 -                      if (!defined $smtp_authpass) {
 -
 -                              system "stty -echo";
 -
 -                              do {
 -                                      print "Password: ";
 -                                      $_ = <STDIN>;
 -                                      print "\n";
 -                              } while (!defined $_);
 -
 -                              chomp($smtp_authpass = $_);
 -
 -                              system "stty echo";
 -                      }
 -
 -                      $auth ||= $smtp->auth( $smtp_authuser, $smtp_authpass ) or die $smtp->message;
 -              }
 +              smtp_auth_maybe or die $smtp->message;
  
                $smtp->mail( $raw_from ) or die $smtp->message;
                $smtp->to( @recipients ) or die $smtp->message;
@@@ -1453,7 -1440,7 +1455,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;
@@@ -1499,7 -1486,7 +1501,7 @@@ sub validate_patch 
                        return "$.: patch contains a line longer than 998 characters";
                }
        }
-       return undef;
+       return;
  }
  
  sub file_has_nonascii {