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

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

[Glenn Rempe]

Add ability to specify custom SMTP server port using
smtpserverport config value or --smtp-server-port command
line option.

Will default to port 25 if smtpssl config is set
to false or --smtp-ssl command line is unset.

Will default to port 465 if smtpssl config is set
to true or --smtp-ssl is set.

User can specify any arbitrary port number for standard or
SSL connections.

Users should be aware that sending auth info over non-ssl
connections may be unsafe.

Signed-off-by: Glenn Rempe <glenn@rempe.us>
---
 git-send-email.perl |   48 +++++++++++++++++++++++++++++++++++++-----------
 1 files changed, 37 insertions(+), 11 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index 4031e86..7c9c302 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -79,6 +79,10 @@ Options:
    --smtp-server  If set, specifies the outgoing SMTP server to use.
                   Defaults to localhost.
 
+   --smtp-server-port If set, specifies the port on the outgoing SMTP
+   server to use. Defaults to port 25 unless --smtp-ssl is set in
+   which case it will default to port 465.
+
    --smtp-user    The username for SMTP-AUTH.
 
    --smtp-pass    The password for SMTP-AUTH.
@@ -172,7 +176,7 @@ my ($quiet, $dry_run) = (0, 0);
 
 # Variables with corresponding config settings
 my ($thread, $chain_reply_to, $suppress_from, $signed_off_cc, $cc_cmd);
-my ($smtp_server, $smtp_authuser, $smtp_authpass, $smtp_ssl);
+my ($smtp_server, $smtp_server_port, $smtp_authuser, $smtp_authpass, $smtp_ssl);
 my ($identity, $aliasfiletype, @alias_files);
 
 my %config_bool_settings = (
@@ -185,6 +189,7 @@ my %config_bool_settings = (
 
 my %config_settings = (
     "smtpserver" => \$smtp_server,
+    "smtpserverport" => \$smtp_server_port,
     "smtpuser" => \$smtp_authuser,
     "smtppass" => \$smtp_authpass,
     "cccmd" => \$cc_cmd,
@@ -204,6 +209,7 @@ my $rc = GetOptions("sender|from=s" => \$sender,
 		    "bcc=s" => \@bcclist,
 		    "chain-reply-to!" => \$chain_reply_to,
 		    "smtp-server=s" => \$smtp_server,
+		    "smtp-server-port=s" => \$smtp_server_port,
 		    "smtp-user=s" => \$smtp_authuser,
 		    "smtp-pass=s" => \$smtp_authpass,
 		    "smtp-ssl!" => \$smtp_ssl,
@@ -375,6 +381,14 @@ if (!defined $smtp_server) {
 	$smtp_server ||= 'localhost'; # could be 127.0.0.1, too... *shrug*
 }
 
+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
@@ -604,20 +618,32 @@ X-Mailer: git-send-email $gitversion
 	} else {
 		if ($smtp_ssl) {
 			require Net::SMTP::SSL;
-			$smtp ||= Net::SMTP::SSL->new( $smtp_server, Port => 465 );
+			$smtp ||= Net::SMTP::SSL->new( $smtp_server, Port => $smtp_server_port );
 		}
 		else {
 			require Net::SMTP;
-			$smtp ||= Net::SMTP->new( $smtp_server );
+			$smtp ||= Net::SMTP->new( $smtp_server . ":" . $smtp_server_port );
 		}
-		$smtp->auth( $smtp_authuser, $smtp_authpass )
-			or die $smtp->message if (defined $smtp_authuser);
-		$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;
+    
+    # we'll get an ugly error if $smtp was undefined above.
+    # If so we'll catch it and present something friendlier.
+    if ($smtp) {
+
+      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;
+      
+    } else {
+      die "Unable to initialize SMTP properly.  Is there something wrong with your config?";
+    }
+
 	}
 	if ($quiet) {
 		printf (($dry_run ? "Dry-" : "")."Sent %s\n", $subject);
-- 
1.5.3.2.83.gc6869

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

[Junio C Hamano]

Glenn Rempe <glenn@rempe.us> writes:

>  git-send-email.perl |   48 +++++++++++++++++++++++++++++++++++++-----------
>  1 files changed, 37 insertions(+), 11 deletions(-)
>
> diff --git a/git-send-email.perl b/git-send-email.perl
> index 4031e86..7c9c302 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -79,6 +79,10 @@ Options:
>     --smtp-server  If set, specifies the outgoing SMTP server to use.
>                    Defaults to localhost.
>  
> +   --smtp-server-port If set, specifies the port on the outgoing SMTP
> +   server to use. Defaults to port 25 unless --smtp-ssl is set in
> +   which case it will default to port 465.
> +

This paragraph look inconsistent with different indentation
for second and subsequent lines.

> @@ -375,6 +381,14 @@ if (!defined $smtp_server) {
>  	$smtp_server ||= 'localhost'; # could be 127.0.0.1, too... *shrug*
>  }
>  
> +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
> @@ -604,20 +618,32 @@ X-Mailer: git-send-email $gitversion
>  	} else {
>  		if ($smtp_ssl) {
>  			require Net::SMTP::SSL;
> -			$smtp ||= Net::SMTP::SSL->new( $smtp_server, Port => 465 );
> +			$smtp ||= Net::SMTP::SSL->new( $smtp_server, Port => $smtp_server_port );
>  		}
>  		else {
>  			require Net::SMTP;
> -			$smtp ||= Net::SMTP->new( $smtp_server );
> +			$smtp ||= Net::SMTP->new( $smtp_server . ":" . $smtp_server_port );
>  		}

This change suggests that, although undocumented, existing users
could have already been using

	--smtp-server=smtp.myisp.com:26

to specify a nonstandard port, and this patch, while bringing in
the support for a nonstandard port as an official feature, would
break such a setup.  I wonder how real the issue is, and if we
can work it around easily.  For example,

 (1) drop the "default to 25 for smtp if undefined" part we saw
     earlier;

 (2) redo this part as

	if ($smtp_ssl) {
		... as you have it ...
	} else {
		require Net::SMTP;
                $smtp ||= Net::SMTP->new((defined $smtp_server_port) ?
					 "$smtp_server:$smtp_server_port" :
                                         $smtp_server);
	}

> -		$smtp->auth( $smtp_authuser, $smtp_authpass )
> -			or die $smtp->message if (defined $smtp_authuser);
> ...
> +    
> +    # we'll get an ugly error if $smtp was undefined above.
> +    # If so we'll catch it and present something friendlier.
> +    if ($smtp) {
> +
> +      if ((defined $smtp_authuser) && (defined $smtp_authpass)) {
> +        $smtp->auth( $smtp_authuser, $smtp_authpass ) or die $smtp->message;
> +      }
> +    ...
> +    } else {
> +      die "Unable to initialize SMTP properly.  Is there something wrong with your config?";
> +    }


I'd prefer the check done the opposite order, like

	if (!$smtp) {
        	die ...
	}

without an else clause.

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

[Glenn Rempe]

Add ability to specify custom SMTP server port using
smtpserverport config value or --smtp-server-port command
line option.

Ability to specify custom SMTP server port using
--smtp-server host:port syntax.

Will default to port 25 if smtpssl config is set
to false or --smtp-ssl command line is unset and port
is not explicitly defined.

Will default to port 465 if smtpssl config is set
to true or --smtp-ssl is set and port is not explicity
defined.

Users should be aware that sending auth info over non-ssl
connections may be unsafe or just may not work at all
depending on SMTP server config.

Added some negative test cases.

Signed-off-by: Glenn Rempe <glenn@rempe.us>
---
 git-send-email.perl   |   73 ++++++++++++++++++++++++++++++++++++++++---------
 t/t9001-send-email.sh |   12 ++++++++
 2 files changed, 72 insertions(+), 13 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index 4031e86..969cb39 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -77,7 +77,10 @@ Options:
                   the default section.
 
    --smtp-server  If set, specifies the outgoing SMTP server to use.
-                  Defaults to localhost.
+                  Defaults to localhost.  Port number can be specified here with
+                  hostname:port format or by using --smtp-server-port option.
+
+   --smtp-server-port Specify a port on the outgoing SMTP server to connect to.
 
    --smtp-user    The username for SMTP-AUTH.
 
@@ -172,8 +175,8 @@ my ($quiet, $dry_run) = (0, 0);
 
 # Variables with corresponding config settings
 my ($thread, $chain_reply_to, $suppress_from, $signed_off_cc, $cc_cmd);
-my ($smtp_server, $smtp_authuser, $smtp_authpass, $smtp_ssl);
-my ($identity, $aliasfiletype, @alias_files);
+my ($smtp_server, $smtp_server_port, $smtp_authuser, $smtp_authpass, $smtp_ssl);
+my ($identity, $aliasfiletype, @alias_files, @smtp_host_parts);
 
 my %config_bool_settings = (
     "thread" => [\$thread, 1],
@@ -185,6 +188,7 @@ my %config_bool_settings = (
 
 my %config_settings = (
     "smtpserver" => \$smtp_server,
+    "smtpserverport" => \$smtp_server_port,
     "smtpuser" => \$smtp_authuser,
     "smtppass" => \$smtp_authpass,
     "cccmd" => \$cc_cmd,
@@ -204,6 +208,7 @@ my $rc = GetOptions("sender|from=s" => \$sender,
 		    "bcc=s" => \@bcclist,
 		    "chain-reply-to!" => \$chain_reply_to,
 		    "smtp-server=s" => \$smtp_server,
+		    "smtp-server-port=s" => \$smtp_server_port,
 		    "smtp-user=s" => \$smtp_authuser,
 		    "smtp-pass=s" => \$smtp_authpass,
 		    "smtp-ssl!" => \$smtp_ssl,
@@ -375,6 +380,29 @@ 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
@@ -602,22 +630,41 @@ X-Mailer: git-send-email $gitversion
 		print $sm "$header\n$message";
 		close $sm or die $?;
 	} 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."
+		}
+
 		if ($smtp_ssl) {
 			require Net::SMTP::SSL;
-			$smtp ||= Net::SMTP::SSL->new( $smtp_server, Port => 465 );
+			$smtp ||= Net::SMTP::SSL->new( $smtp_server, Port => $smtp_server_port );
 		}
 		else {
 			require Net::SMTP;
-			$smtp ||= Net::SMTP->new( $smtp_server );
+			$smtp ||= Net::SMTP->new($smtp_server . ":" . $smtp_server_port);
 		}
-		$smtp->auth( $smtp_authuser, $smtp_authpass )
-			or die $smtp->message if (defined $smtp_authuser);
-		$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;
+    
+    # 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 ($quiet) {
 		printf (($dry_run ? "Dry-" : "")."Sent %s\n", $subject);
diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index 83f9470..d32907d 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -41,4 +41,16 @@ 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
-- 
1.5.3.2.105.g23fe

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

[Johannes Schindelin]

Hi,

On Tue, 25 Sep 2007, Glenn Rempe wrote:

> +if (($smtp_server =~ /:\d+/) && (defined $smtp_server_port)) {

Not that I want to be a PITA, but this breaks down with IPv6, right?

Ciao,
Dscho

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

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> On Tue, 25 Sep 2007, Glenn Rempe wrote:
>
>> +if (($smtp_server =~ /:\d+/) && (defined $smtp_server_port)) {
>
> Not that I want to be a PITA, but this breaks down with IPv6, right?

Right.  Do we care about symbolic "server.addre.ss:smtp"
notation as well, I wonder?

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
>> On Tue, 25 Sep 2007, Glenn Rempe wrote:
>>
>>> +if (($smtp_server =~ /:\d+/) && (defined $smtp_server_port)) {
>>
>> Not that I want to be a PITA, but this breaks down with IPv6, right?
>
> Right.  Do we care about symbolic "server.addre.ss:smtp"
> notation as well, I wonder?

Well, does it break?

BTW, I do not think we care about ":smtp"; it was a
tongue-in-cheek comment.

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

[Glenn Rempe]

On Sep 25, 2007, at 4:25 PM, Junio C Hamano wrote:

> Junio C Hamano <gitster@pobox.com> writes:
>
>> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>>
>>> On Tue, 25 Sep 2007, Glenn Rempe wrote:
>>>
>>>> +if (($smtp_server =~ /:\d+/) && (defined $smtp_server_port)) {
>>>
>>> Not that I want to be a PITA, but this breaks down with IPv6, right?
>>
>> Right.  Do we care about symbolic "server.addre.ss:smtp"
>> notation as well, I wonder?
>
> Well, does it break?
>
> BTW, I do not think we care about ":smtp"; it was a
> tongue-in-cheek comment.

Unfortunately, I know little about IPv6 and whether this breaks IPv6  
addressing or not.  So I'll leave that question for others. Does the  
unpatched code work with IPv6?  Does anyone currently use it that way?

Junio, are you suggesting that I should remove the host:port form  
support entirely and leave only the --smtp-server port option as  
valid?  I kind of liked that this new method allows both forms of  
specifying port as equal citizens.  :-)  If you are suggesting that  
it be removed, I think we would have to reject the host:port form  
smtp-server addresses so we don't break when both --smtp- 
server=host:port and -smtp-ssl are provided. (which brings us back to  
the ipv6 question).  No?

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

[Junio C Hamano]

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

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

[Glenn Rempe]

[-- Attachment #1: Type: text/plain, Size: 8424 bytes --]

ok, you're the boss! :-)

Comments:

>  * 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.

ok.  I'm new to Git and the project's coding standards.  I just  
thought that the additional check made the experience friendlier for  
the end user in the case of passing ambiguous arguments, even at the  
expense of a few lines of code.

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

Likely true.

>
>  * 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.

ok.

>  * 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.

ok.  Again just trying to protect against invalid arguments.

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

Can you be more detailed on the definition of 'horrible'? :-) I am  
using Textmate on OS X with soft tab stops (2 spaces).  What should  
it be to look less horrible on your end?  Or is the issue that I  
indent fewer tabstops than you expect? If so, sorry since perl is not  
my usual language and Ruby 2 space (soft tab) indentation looks right  
to my eye.

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

ok.  Assuming you want to remove the check on port numbers.

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

Back to my first comment.  I agree its more readable with less code.   
Just weighing the trade off with user experience.  The tool is a bit  
'sharper' in the hands of the end user now IMHO.

I hope this was helpful. :-)  Its been useful for me in getting to  
know Git and the community a bit better.  I'll assume you don't need  
me to do anything else on this issue?  If thats not correct please  
let me know.

Glenn


On Sep 25, 2007, at 5:27 PM, Junio C Hamano wrote:

> 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

--
Glenn Rempe
glenn.@rempe.us




[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 2411 bytes --]

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

[Johannes Schindelin]

Hi,

On Tue, 25 Sep 2007, Glenn Rempe wrote:

> > * The indentation was horrible.  Maybe your tabstop is set
> >   incorrectly?
> 
> Can you be more detailed on the definition of 'horrible'? :-) I am using 
> Textmate on OS X with soft tab stops (2 spaces).  What should it be to 
> look less horrible on your end?  Or is the issue that I indent fewer 
> tabstops than you expect? If so, sorry since perl is not my usual 
> language and Ruby 2 space (soft tab) indentation looks right to my eye.

We use soft tabs, with the standard up-to 8 spaces, so that short sighted 
people like me can still see that the line is actually indented.

In related news, I just saw that we never mention the 80 characters per 
line convention in code, or the 76 characters per line for commit messages 
(because of the 4 space indent git-log does -- ouch ;-)

Ciao,
Dscho

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

[Andreas Ericsson]

Johannes Schindelin wrote:
> Hi,
> 
> On Tue, 25 Sep 2007, Glenn Rempe wrote:
> 
>>> * The indentation was horrible.  Maybe your tabstop is set
>>>   incorrectly?
>> Can you be more detailed on the definition of 'horrible'? :-) I am using 
>> Textmate on OS X with soft tab stops (2 spaces).  What should it be to 
>> look less horrible on your end?  Or is the issue that I indent fewer 
>> tabstops than you expect? If so, sorry since perl is not my usual 
>> language and Ruby 2 space (soft tab) indentation looks right to my eye.
> 
> We use soft tabs, with the standard up-to 8 spaces,

Actually, the rest of git's perl-scripts use hard tabs, just as the C-code
and the shell-code, with tabs for indentation and spaces for alignment.

-- 
Andreas Ericsson                   andreas.ericsson@op5.se
OP5 AB                             www.op5.se
Tel: +46 8-230225                  Fax: +46 8-230231

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

[Johannes Schindelin]

Hi,

On Wed, 26 Sep 2007, Andreas Ericsson wrote:

> Actually, the rest of git's perl-scripts use hard tabs, just as the 
> C-code and the shell-code, with tabs for indentation and spaces for 
> alignment.

D'oh.  I meant to say hard tabs.

Thanks,
Dscho