git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Glenn Rempe <glenn@rempe.us>
Date
Sep 26, 2007, 01:15 UTC
Message-ID
<1A0CAB9D-5C99-4FD7-B3AC-9B3161FD8663@rempe.us>
In-Reply-To
<7vabra2rv3.fsf@gitster.siamese.dyndns.org>
ok, you're the boss! :-)
Comments:
Show 5 quoted lines
>  * 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.
Show 5 quoted lines
>  * 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:
Show 168 quoted lines
> 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

Previous: Junio C HamanoNext: Johannes Schindelin
Message 7 of 10 in “Add ability to specify SMTP server port when using git-send-email.”
  1. Add ability to specify SMTP server port when using git-send-email.Glenn Rempe, Sep 25, 2007
  2. Johannes SchindelinSep 25, 2007
  3. Junio C HamanoSep 25, 2007
  4. Junio C HamanoSep 25, 2007
  5. Glenn RempeSep 26, 2007
  6. Junio C HamanoSep 26, 2007
  7. Glenn RempeSep 26, 2007
  8. Johannes SchindelinSep 26, 2007
  9. Andreas EricssonSep 26, 2007
  10. Johannes SchindelinSep 26, 2007

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.