Re: [PATCH] Add ability to specify SMTP server port when using git-send-email.

[Junio C Hamano <gitster@pobox.com>]

I'm inclined to do this on top of yours.

 * As you mentioned, ssmtp plus undocumented "server:port"
   syntax never worked.  There is no point supporting the
   combination.  Also I do not think it is worth additional
   lines of code to even say "server:port" plus --server-port
   option are not meant to be used together.

 * People who used the undocumented "server:port" syntax did not
   use the new --smtp-server-port option anyway.

 * If somebody goes over plain smtp specifies server without
   port, we can let the Net::SMTP to handle the default port.
   No need for _us_ to say that the default is 25.

 * I do not see much point insisting that port to be numeric; I
   do not know what Net::SMTP accepts, but if it accepts
   my.isp.com:smtp instead of my.isp.com:25, that is fine. This
   has the side effect of keeping people's existing set-up
   working.

 * The indentation was horrible.  Maybe your tabstop is set
   incorrectly?

 * As I am inclined not to insist on numeric port numbers,
   the additional tests become pointless.

The result is much simpler, and I think it is more readable.

---

 git-send-email.perl   |   64 +++++++++++++-----------------------------------
 t/t9001-send-email.sh |   12 ---------
 2 files changed, 18 insertions(+), 58 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index 886f78f..62e1429 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -380,29 +380,6 @@ if (!defined $smtp_server) {
 	$smtp_server ||= 'localhost'; # could be 127.0.0.1, too... *shrug*
 }
 
-# don't allow BOTH forms of port definition to work since we can't guess which one is right.
-if (($smtp_server =~ /:\d+/) && (defined $smtp_server_port)) {
-  die "You must specify the port using either hostname:port OR --smtp-server-port but not both!"
-}
-
-# setup smtp_server var if it was passed in as host:port format
-if ( $smtp_server =~ /:\d+/) {
-  # if they do pass a host:port form then split it and use the parts
-  @smtp_host_parts = split(/:/, $smtp_server);
-  $smtp_server = $smtp_host_parts[0];
-  $smtp_server_port = $smtp_host_parts[1];
-}
-
-# setup reasonable defaults if neither host:port or --smtp-server-port were passed
-if ( !defined $smtp_server_port) {
-  if ($smtp_ssl) {
-    $smtp_server_port = 465  # SSL port
-  } else {
-    $smtp_server_port = 25  # Non-SSL port
-  }
-}
-
-
 if ($compose) {
 	# Note that this does not need to be secure, but we will make a small
 	# effort to have it be unique
@@ -632,39 +609,34 @@ X-Mailer: git-send-email $gitversion
 	} else {
 
 		if (!defined $smtp_server) {
-		  die "The required SMTP server is not properly defined."
-		}
-
-		if (!defined $smtp_server_port || !$smtp_server_port =~ /^\d+$/ ) {
-		  die "The required SMTP server port is not properly defined."
+			die "The required SMTP server is not properly defined."
 		}
 
 		if ($smtp_ssl) {
+			$smtp_server_port ||= 465; # ssmtp
 			require Net::SMTP::SSL;
-			$smtp ||= Net::SMTP::SSL->new( $smtp_server, Port => $smtp_server_port );
+			$smtp ||= Net::SMTP::SSL->new($smtp_server, Port => $smtp_server_port);
 		}
 		else {
 			require Net::SMTP;
-			$smtp ||= Net::SMTP->new($smtp_server . ":" . $smtp_server_port);
+			$smtp ||= Net::SMTP->new((defined $smtp_server_port)
+						 ? "$smtp_server:$smtp_server_port"
+						 : $smtp_server);
 		}
 
-    # we'll get an ugly error if $smtp was undefined above.
-    # If so we'll catch it and present something friendlier.
-    if (!$smtp) {
-      die "Unable to initialize SMTP properly.  Is there something wrong with your config?";
-    }
-
-    if ((defined $smtp_authuser) && (defined $smtp_authpass)) {
-      $smtp->auth( $smtp_authuser, $smtp_authpass ) or die $smtp->message;
-    }
-
-    $smtp->mail( $raw_from ) or die $smtp->message;
-    $smtp->to( @recipients ) or die $smtp->message;
-    $smtp->data or die $smtp->message;
-    $smtp->datasend("$header\n$message") or die $smtp->message;
-    $smtp->dataend() or die $smtp->message;
-    $smtp->ok or die "Failed to send $subject\n".$smtp->message;
+		if (!$smtp) {
+			die "Unable to initialize SMTP properly.  Is there something wrong with your config?";
+		}
 
+		if ((defined $smtp_authuser) && (defined $smtp_authpass)) {
+			$smtp->auth( $smtp_authuser, $smtp_authpass ) or die $smtp->message;
+		}
+		$smtp->mail( $raw_from ) or die $smtp->message;
+		$smtp->to( @recipients ) or die $smtp->message;
+		$smtp->data or die $smtp->message;
+		$smtp->datasend("$header\n$message") or die $smtp->message;
+		$smtp->dataend() or die $smtp->message;
+		$smtp->ok or die "Failed to send $subject\n".$smtp->message;
 	}
 	if ($quiet) {
 		printf (($dry_run ? "Dry-" : "")."Sent %s\n", $subject);
diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index d32907d..83f9470 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -41,16 +41,4 @@ test_expect_success \
     'Verify commandline' \
     'diff commandline expected'
 
-test_expect_failure 'Passing in both host:port form AND --smtp-server-port' '
-  git send-email --from="Example <nobody@example.com>" --to=nobody@example.com --smtp-server smtp.foo.com:66 --smtp-server-port 77" $patches 2>errors
-'
-
-test_expect_failure 'Passing in non-numeric server port with host:port form' '
-  git send-email --from="Example <nobody@example.com>" --to=nobody@example.com --smtp-server smtp.foo.com:bar" $patches 2>errors
-'
-
-test_expect_failure 'Passing in non-numeric server port with --smtp-server-port form' '
-  git send-email --from="Example <nobody@example.com>" --to=nobody@example.com --smtp-server smtp.foo.com --smtp-server-port bar" $patches 2>errors
-'
-
 test_done