From: Glenn Rempe Date: Wed, 26 Sep 2007 01:15:09 GMT Subject: Re: [PATCH] Add ability to specify SMTP server port when using git-send-email. Message-ID: <1A0CAB9D-5C99-4FD7-B3AC-9B3161FD8663@rempe.us> In-Reply-To: <7vabra2rv3.fsf@gitster.siamese.dyndns.org> 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 " -- > 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 " -- > 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 " -- > to=nobody@example.com --smtp-server smtp.foo.com --smtp-server-port > bar" $patches 2>errors > -' > - > test_done -- Glenn Rempe glenn.@rempe.us