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@xxxxxxxxxxx>" -- to=nobody@xxxxxxxxxxx --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@xxxxxxxxxxx>" -- to=nobody@xxxxxxxxxxx --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@xxxxxxxxxxx>" -- to=nobody@xxxxxxxxxxx --smtp-server smtp.foo.com --smtp-server-port bar" $patches 2>errors-' - test_done
-- Glenn Rempe glenn.@xxxxxxxx
<<attachment: smime.p7s>>
