[PATCH] Remove dependency on deprecated Net::SMTP::SSL

git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed

* [PATCH] Remove dependency on deprecated Net::SMTP::SSL
@ 2016-11-20 21:18 Mike Fisher
  2016-11-20 21:53 ` brian m. carlson
                   ` (2 more replies)
  0 siblings, 3 replies; 16+ messages in thread
From: Mike Fisher @ 2016-11-20 21:18 UTC (permalink / raw)
  To: git

Refactor send_message() to remove dependency on deprecated
Net::SMTP::SSL:

<http://search.cpan.org/~rjbs/Net-SMTP-SSL-1.04/lib/Net/SMTP/SSL.pm#DEPRECATED>

Signed-off-by: Mike Fisher <mfisher@csh.rit.edu>
---
  git-send-email.perl | 54 
+++++++++++++++++++++++++----------------------------
  1 file changed, 25 insertions(+), 29 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index da81be4..fc166c5 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -1330,15 +1330,17 @@ Message-Id: $message_id
  		print $sm "$header\n$message";
  		close $sm or die $!;
  	} else {
-
  		if (!defined $smtp_server) {
  			die "The required SMTP server is not properly defined."
  		}

+		require Net::SMTP;
+		$smtp_domain ||= maildomain();
+		my $smtp_ssl = 0;
+
  		if ($smtp_encryption eq 'ssl') {
  			$smtp_server_port ||= 465; # ssmtp
-			require Net::SMTP::SSL;
-			$smtp_domain ||= maildomain();
+			$smtp_ssl = 1;
  			require IO::Socket::SSL;

  			# Suppress "variable accessed once" warning.
@@ -1347,37 +1349,31 @@ Message-Id: $message_id
  				$IO::Socket::SSL::DEBUG = 1;
  			}

-			# Net::SMTP::SSL->new() does not forward any SSL options
  			IO::Socket::SSL::set_client_defaults(
  				ssl_verify_params());
-			$smtp ||= Net::SMTP::SSL->new($smtp_server,
-						      Hello => $smtp_domain,
-						      Port => $smtp_server_port,
-						      Debug => $debug_net_smtp);
  		}
  		else {
-			require Net::SMTP;
-			$smtp_domain ||= maildomain();
  			$smtp_server_port ||= 25;
-			$smtp ||= Net::SMTP->new($smtp_server,
-						 Hello => $smtp_domain,
-						 Debug => $debug_net_smtp,
-						 Port => $smtp_server_port);
-			if ($smtp_encryption eq 'tls' && $smtp) {
-				require Net::SMTP::SSL;
-				$smtp->command('STARTTLS');
-				$smtp->response();
-				if ($smtp->code == 220) {
-					$smtp = Net::SMTP::SSL->start_SSL($smtp,
-									  ssl_verify_params())
-						or die "STARTTLS failed! ".IO::Socket::SSL::errstr();
-					$smtp_encryption = '';
-					# Send EHLO again to receive fresh
-					# supported commands
-					$smtp->hello($smtp_domain);
-				} else {
-					die "Server does not support STARTTLS! ".$smtp->message;
-				}
+		}
+
+		$smtp ||= Net::SMTP->new($smtp_server,
+					 Hello => $smtp_domain,
+					 Port => $smtp_server_port,
+					 Debug => $debug_net_smtp,
+					 SSL => $smtp_ssl);
+
+		if ($smtp_encryption eq 'tls' && $smtp) {
+			$smtp->command('STARTTLS');
+			$smtp->response();
+			if ($smtp->code == 220) {
+				$smtp->starttls(ssl_verify_params())
+					or die "STARTTLS failed! ".IO::Socket::SSL::errstr();
+				$smtp_encryption = '';
+				# Send EHLO again to receive fresh
+				# supported commands
+				$smtp->hello($smtp_domain);
+			} else {
+				die "Server does not support STARTTLS! ".$smtp->message;
  			}
  		}

-- 
2.9.3 (Apple Git-75)


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH] Remove dependency on deprecated Net::SMTP::SSL
  2016-11-20 21:18 [PATCH] Remove dependency on deprecated Net::SMTP::SSL Mike Fisher
@ 2016-11-20 21:53 ` brian m. carlson
  2017-01-13 14:59   ` Renato Botelho
  2016-11-21  5:37 ` Torsten Bögershausen
  2017-03-18 22:23 ` [PATCH] send-email: Net::SMTP::SSL is obsolete, use only when necessary Dennis Kaarsemaker
  2 siblings, 1 reply; 16+ messages in thread
From: brian m. carlson @ 2016-11-20 21:53 UTC (permalink / raw)
  To: Mike Fisher; +Cc: git

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

On Sun, Nov 20, 2016 at 04:18:16PM -0500, Mike Fisher wrote:
> Refactor send_message() to remove dependency on deprecated
> Net::SMTP::SSL:
> 
> <http://search.cpan.org/~rjbs/Net-SMTP-SSL-1.04/lib/Net/SMTP/SSL.pm#DEPRECATED>

As much as I hate to say this, I think this is going to cause
compatibility problems.  Net::SMTP is part of core Perl (as of v5.7.3),
but the version you want to rely on (which you did not provide an
explicit dependency on) is from October 2014.

That basically means that no Perl on a Red Hat or CentOS system is going
to provide that support, since RHEL 7 was released in June 2014.
Providing an updated Git on those platforms would require replacing the
system Perl or parts of it, which would be undesirable.  This would
affect Debian 7 as well.

We currently support Perl 5.8 [0], so if you want to remove support for
Net::SMTP::SSL, I'd recommend a solution that works with that version.

[0] I personally believe we should drop support for Perl older than
5.10.1 (if not newer), but that's my opinion and it isn't shared by
other list regulars.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] Remove dependency on deprecated Net::SMTP::SSL
  2016-11-20 21:53 ` brian m. carlson
@ 2017-01-13 14:59   ` Renato Botelho
  0 siblings, 0 replies; 16+ messages in thread
From: Renato Botelho @ 2017-01-13 14:59 UTC (permalink / raw)
  To: brian m. carlson; +Cc: Mike Fisher, git

> On 20 Nov 2016, at 19:53, brian m. carlson <sandals@crustytoothpaste.net> wrote:
> 
> On Sun, Nov 20, 2016 at 04:18:16PM -0500, Mike Fisher wrote:
>> Refactor send_message() to remove dependency on deprecated
>> Net::SMTP::SSL:
>> 
>> <http://search.cpan.org/~rjbs/Net-SMTP-SSL-1.04/lib/Net/SMTP/SSL.pm#DEPRECATED>
> 
> As much as I hate to say this, I think this is going to cause
> compatibility problems.  Net::SMTP is part of core Perl (as of v5.7.3),
> but the version you want to rely on (which you did not provide an
> explicit dependency on) is from October 2014.
> 
> That basically means that no Perl on a Red Hat or CentOS system is going
> to provide that support, since RHEL 7 was released in June 2014.
> Providing an updated Git on those platforms would require replacing the
> system Perl or parts of it, which would be undesirable.  This would
> affect Debian 7 as well.
> 
> We currently support Perl 5.8 [0], so if you want to remove support for
> Net::SMTP::SSL, I'd recommend a solution that works with that version.
> 
> [0] I personally believe we should drop support for Perl older than
> 5.10.1 (if not newer), but that's my opinion and it isn't shared by
> other list regulars.
> -- 
> brian m. carlson / brian with sandals: Houston, Texas, US
> +1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
> OpenPGP: https://keybase.io/bk2204

Net::SMTP::SSL is marked as DEPRECATED on FreeBSD ports tree and will be removed in 2017-03-31. When it happens users will not be able to run git-send-email anymore. I’m considering to add Mike’s patch to FreeBSD ports tree as an alternative but it would be good to have a official solution for this problem.

FreeBSD bug report can be found at https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=214335

--
Renato Botelho


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] Remove dependency on deprecated Net::SMTP::SSL
  2016-11-20 21:18 [PATCH] Remove dependency on deprecated Net::SMTP::SSL Mike Fisher
  2016-11-20 21:53 ` brian m. carlson
@ 2016-11-21  5:37 ` Torsten Bögershausen
  2017-03-18 22:23 ` [PATCH] send-email: Net::SMTP::SSL is obsolete, use only when necessary Dennis Kaarsemaker
  2 siblings, 0 replies; 16+ messages in thread
From: Torsten Bögershausen @ 2016-11-21  5:37 UTC (permalink / raw)
  To: Mike Fisher, git



>On 20/11/16 22:18, Mike Fisher  wrote:

Thanks for contributing to Git.
One comment on the head line:
 >Refactor send_message() to remove dependency on deprecated
Net::SMTP::SSL
The word "refactor" may be used in other way: Re-structure the code,
and use the same API.


"Remove dependency on deprecated Net::SMTP::SSL"

> Refactor send_message() to remove dependency on deprecated
> Net::SMTP::SSL:
Is there a security risk with require Net::SMTP::SSL ?
If yes, the commit message should state this.
If no:
Even if it is deprecated, is it still in use somewhere ?
Does it hurt someone, is there any OS release where the old code doesn't work 
anymore ?
Or is it "only" nice to have ?
Since when does Net::SMTP include Net::SMTP::SSL ?
On which system has the change been tested ?

I think the commit message could and should give more information like this.

My comments may be over-critical.
Lets see if other people from the list know more than me.

>
> <http://search.cpan.org/~rjbs/Net-SMTP-SSL-1.04/lib/Net/SMTP/SSL.pm#DEPRECATED>
>
> Signed-off-by: Mike Fisher <mfisher@csh.rit.edu>
> ---
>  git-send-email.perl | 54 +++++++++++++++++++++++++----------------------------
>  1 file changed, 25 insertions(+), 29 deletions(-)
>
> diff --git a/git-send-email.perl b/git-send-email.perl
> index da81be4..fc166c5 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -1330,15 +1330,17 @@ Message-Id: $message_id
>          print $sm "$header\n$message";
>          close $sm or die $!;
>      } else {
> -
I can see one refactoring, that is the removal of an empty line.

>
>          if (!defined $smtp_server) {
>              die "The required SMTP server is not properly defined."
>          }
>
> +        require Net::SMTP;
> +        $smtp_domain ||= maildomain();
> +        my $smtp_ssl = 0;
> +
>          if ($smtp_encryption eq 'ssl') {
>              $smtp_server_port ||= 465; # ssmtp
> -            require Net::SMTP::SSL;
> -            $smtp_domain ||= maildomain();
> +            $smtp_ssl = 1;
>              require IO::Socket::SSL;
>
>              # Suppress "variable accessed once" warning.
> @@ -1347,37 +1349,31 @@ Message-Id: $message_id
>                  $IO::Socket::SSL::DEBUG = 1;
>              }
>
> -            # Net::SMTP::SSL->new() does not forward any SSL options
>              IO::Socket::SSL::set_client_defaults(
>                  ssl_verify_params());
> -            $smtp ||= Net::SMTP::SSL->new($smtp_server,
> -                              Hello => $smtp_domain,
> -                              Port => $smtp_server_port,
> -                              Debug => $debug_net_smtp);
>          }
>          else {
> -            require Net::SMTP;
> -            $smtp_domain ||= maildomain();
>              $smtp_server_port ||= 25;
> -            $smtp ||= Net::SMTP->new($smtp_server,
> -                         Hello => $smtp_domain,
> -                         Debug => $debug_net_smtp,
> -                         Port => $smtp_server_port);
> -            if ($smtp_encryption eq 'tls' && $smtp) {
> -                require Net::SMTP::SSL;
> -                $smtp->command('STARTTLS');
> -                $smtp->response();
> -                if ($smtp->code == 220) {
> -                    $smtp = Net::SMTP::SSL->start_SSL($smtp,
> -                                      ssl_verify_params())
> -                        or die "STARTTLS failed! ".IO::Socket::SSL::errstr();
> -                    $smtp_encryption = '';
> -                    # Send EHLO again to receive fresh
> -                    # supported commands
> -                    $smtp->hello($smtp_domain);
> -                } else {
> -                    die "Server does not support STARTTLS! ".$smtp->message;
> -                }
> +        }
> +
> +        $smtp ||= Net::SMTP->new($smtp_server,
> +                     Hello => $smtp_domain,
> +                     Port => $smtp_server_port,
> +                     Debug => $debug_net_smtp,
> +                     SSL => $smtp_ssl);
> +
> +        if ($smtp_encryption eq 'tls' && $smtp) {
> +            $smtp->command('STARTTLS');
> +            $smtp->response();
> +            if ($smtp->code == 220) {
> +                $smtp->starttls(ssl_verify_params())
> +                    or die "STARTTLS failed! ".IO::Socket::SSL::errstr();
> +                $smtp_encryption = '';
> +                # Send EHLO again to receive fresh
> +                # supported commands
> +                $smtp->hello($smtp_domain);
> +            } else {
> +                die "Server does not support STARTTLS! ".$smtp->message;
>              }
>          }
>


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH] send-email: Net::SMTP::SSL is obsolete, use only when necessary
  2016-11-20 21:18 [PATCH] Remove dependency on deprecated Net::SMTP::SSL Mike Fisher
  2016-11-20 21:53 ` brian m. carlson
  2016-11-21  5:37 ` Torsten Bögershausen
@ 2017-03-18 22:23 ` Dennis Kaarsemaker
  2017-03-18 22:47   ` Ævar Arnfjörð Bjarmason
  2 siblings, 1 reply; 16+ messages in thread
From: Dennis Kaarsemaker @ 2017-03-18 22:23 UTC (permalink / raw)
  To: git; +Cc: Dennis Kaarsemaker

Net::SMTP itself can do the necessary SSL and STARTTLS bits just fine
since version 1.28, and Net::SMTP::SSL is now deprecated. Since 1.28
isn't that old yet, keep the old code in place and use it when
necessary.

Signed-off-by: Dennis Kaarsemaker <dennis@kaarsemaker.net>
---
 Note: I've only been able to test the starttls bits. None of the smtp servers
 I use actually use ssl, only starttls.

 git-send-email.perl | 52 ++++++++++++++++++++++++++++++++++------------------
 1 file changed, 34 insertions(+), 18 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index eea0a517f7..e247ea39dd 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -1353,10 +1353,12 @@ EOF
 			die __("The required SMTP server is not properly defined.")
 		}
 
+		require Net::SMTP;
+		my $use_net_smtp_ssl = $Net::SMTP::VERSION lt "1.28";
+		$smtp_domain ||= maildomain();
+
 		if ($smtp_encryption eq 'ssl') {
 			$smtp_server_port ||= 465; # ssmtp
-			require Net::SMTP::SSL;
-			$smtp_domain ||= maildomain();
 			require IO::Socket::SSL;
 
 			# Suppress "variable accessed once" warning.
@@ -1368,34 +1370,48 @@ EOF
 			# Net::SMTP::SSL->new() does not forward any SSL options
 			IO::Socket::SSL::set_client_defaults(
 				ssl_verify_params());
-			$smtp ||= Net::SMTP::SSL->new($smtp_server,
-						      Hello => $smtp_domain,
-						      Port => $smtp_server_port,
-						      Debug => $debug_net_smtp);
+
+			if ($use_net_smtp_ssl) {
+				require Net::SMTP::SSL;
+				$smtp ||= Net::SMTP::SSL->new($smtp_server,
+							      Hello => $smtp_domain,
+							      Port => $smtp_server_port,
+							      Debug => $debug_net_smtp);
+			}
+			else {
+				$smtp ||= Net::SMTP->new($smtp_server,
+							 Hello => $smtp_domain,
+							 Port => $smtp_server_port,
+							 Debug => $debug_net_smtp,
+							 SSL => 1);
+			}
 		}
 		else {
-			require Net::SMTP;
-			$smtp_domain ||= maildomain();
 			$smtp_server_port ||= 25;
 			$smtp ||= Net::SMTP->new($smtp_server,
 						 Hello => $smtp_domain,
 						 Debug => $debug_net_smtp,
 						 Port => $smtp_server_port);
 			if ($smtp_encryption eq 'tls' && $smtp) {
-				require Net::SMTP::SSL;
-				$smtp->command('STARTTLS');
-				$smtp->response();
-				if ($smtp->code == 220) {
+				if ($use_net_smtp_ssl) {
+					$smtp->command('STARTTLS');
+					$smtp->response();
+					if ($smtp->code != 220) {
+						die sprintf(__("Server does not support STARTTLS! %s"), $smtp->message);
+					}
+					require Net::SMTP::SSL;
 					$smtp = Net::SMTP::SSL->start_SSL($smtp,
 									  ssl_verify_params())
 						or die "STARTTLS failed! ".IO::Socket::SSL::errstr();
-					$smtp_encryption = '';
-					# Send EHLO again to receive fresh
-					# supported commands
-					$smtp->hello($smtp_domain);
-				} else {
-					die sprintf(__("Server does not support STARTTLS! %s"), $smtp->message);
 				}
+				else {
+					$smtp->starttls(ssl_verify_params())
+						or die "STARTTLS failed! ".IO::Socket::SSL::errstr();
+				}
+				$smtp_encryption = '';
+				# Send EHLO again to receive fresh
+				# supported commands
+				$smtp->hello($smtp_domain);
 			}
 		}
 
-- 
2.12.0-437-g0cc2799


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH] send-email: Net::SMTP::SSL is obsolete, use only when necessary
  2017-03-18 22:23 ` [PATCH] send-email: Net::SMTP::SSL is obsolete, use only when necessary Dennis Kaarsemaker
@ 2017-03-18 22:47   ` Ævar Arnfjörð Bjarmason
  2017-03-18 23:14     ` Dennis Kaarsemaker
  2017-03-24 21:37     ` [PATCH v2] " Dennis Kaarsemaker
  0 siblings, 2 replies; 16+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2017-03-18 22:47 UTC (permalink / raw)
  To: Dennis Kaarsemaker; +Cc: Git Mailing List

On Sat, Mar 18, 2017 at 11:23 PM, Dennis Kaarsemaker
<dennis@kaarsemaker.net> wrote:
> Net::SMTP itself can do the necessary SSL and STARTTLS bits just fine
> since version 1.28, and Net::SMTP::SSL is now deprecated. Since 1.28
> isn't that old yet, keep the old code in place and use it when
> necessary.
>
> Signed-off-by: Dennis Kaarsemaker <dennis@kaarsemaker.net>
> ---
>  Note: I've only been able to test the starttls bits. None of the smtp servers
>  I use actually use ssl, only starttls.
>
>  git-send-email.perl | 52 ++++++++++++++++++++++++++++++++++------------------
>  1 file changed, 34 insertions(+), 18 deletions(-)
>
> diff --git a/git-send-email.perl b/git-send-email.perl
> index eea0a517f7..e247ea39dd 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -1353,10 +1353,12 @@ EOF
>                         die __("The required SMTP server is not properly defined.")
>                 }
>
> +               require Net::SMTP;
> +               my $use_net_smtp_ssl = $Net::SMTP::VERSION lt "1.28";
> +               $smtp_domain ||= maildomain();
> +

While Net::SMTP is unlikely to change its versioning scheme, let's use
comparisons via the version module here in case they do change it to
something silly, and this ends up introducing a bug.

E.g. 04.00 would be considered a higher version by CPAN than 1.28, but
not by this code:

    $ perl -wE 'my ($x, $y) = @ARGV; my ($vx, $vy) = map {
version->parse($_) } ($x, $y); say $vx < $vy ? "vlower" : "vhigher";
say $x lt $y ? "slower" : "shigher"' 04.00 1.28
    vhigher
    slower

If we grep ::VERSION we can find other cases where we've gotten this
wrong, unlikely to bite us in practice, but version.pm is in core (so
core that you don't even need to use/require it), so let's do this
better for new code.


>[...]
> +                                       if ($smtp->code != 220) {
> +                                               die sprintf(__("Server does not support STARTTLS! %s"), $smtp->message);

Here a new message you're adding gets __(), makes sense.

> +                                       }
> +                                       require Net::SMTP::SSL;
>                                         $smtp = Net::SMTP::SSL->start_SSL($smtp,
>                                                                           ssl_verify_params())
>                                                 or die "STARTTLS failed! ".IO::Socket::SSL::errstr();
> -                                       $smtp_encryption = '';
> -                                       # Send EHLO again to receive fresh
> -                                       # supported commands
> -                                       $smtp->hello($smtp_domain);
> -                               } else {
> -                                       die sprintf(__("Server does not support STARTTLS! %s"), $smtp->message);
>                                 }
> +                               else {
> +                                       $smtp->starttls(ssl_verify_params())
> +                                               or die "STARTTLS failed! ".IO::Socket::SSL::errstr();
> +                               }

I see you just copied that from above but I wonder if it makes sense
to just mark both occurrences with __() too while we're at it.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] send-email: Net::SMTP::SSL is obsolete, use only when necessary
  2017-03-18 22:47   ` Ævar Arnfjörð Bjarmason
@ 2017-03-18 23:14     ` Dennis Kaarsemaker
  2017-03-24 21:37     ` [PATCH v2] " Dennis Kaarsemaker
  1 sibling, 0 replies; 16+ messages in thread
From: Dennis Kaarsemaker @ 2017-03-18 23:14 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason; +Cc: Git Mailing List

On Sat, 2017-03-18 at 23:47 +0100, Ævar Arnfjörð Bjarmason wrote:

> On Sat, Mar 18, 2017 at 11:23 PM, Dennis Kaarsemaker
> <dennis@kaarsemaker.net> wrote:
>
> > +               require Net::SMTP;
> > +               my $use_net_smtp_ssl = $Net::SMTP::VERSION lt "1.28";
> > +               $smtp_domain ||= maildomain();
> > +
> 
> While Net::SMTP is unlikely to change its versioning scheme, let's use
> comparisons via the version module here in case they do change it to
> something silly, and this ends up introducing a bug.

ok.

> > [...]
> > +                                       if ($smtp->code != 220) {
> > +                                               die sprintf(__("Server does not support STARTTLS! %s"), $smtp->message);
> 
> Here a new message you're adding gets __(), makes sense.

Didn't add it, it just moved from a bit further below :)

> > +                               else {
> > +                                       $smtp->starttls(ssl_verify_params())
> > +                                               or die "STARTTLS failed! ".IO::Socket::SSL::errstr();
> > +                               }
> 
> I see you just copied that from above but I wonder if it makes sense
> to just mark both occurrences with __() too while we're at it.

ok.

D.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH v2] send-email: Net::SMTP::SSL is obsolete, use only when necessary
  2017-03-18 22:47   ` Ævar Arnfjörð Bjarmason
  2017-03-18 23:14     ` Dennis Kaarsemaker
@ 2017-03-24 21:37     ` Dennis Kaarsemaker
  2017-05-04  7:01       ` Dennis Kaarsemaker
  2017-05-31 21:46       ` Jonathan Nieder
  1 sibling, 2 replies; 16+ messages in thread
From: Dennis Kaarsemaker @ 2017-03-24 21:37 UTC (permalink / raw)
  To: git; +Cc: Dennis Kaarsemaker

Net::SMTP itself can do the necessary SSL and STARTTLS bits just fine
since version 1.28, and Net::SMTP::SSL is now deprecated. Since 1.28
isn't that old yet, keep the old code in place and use it when
necessary.

While we're in the area, mark some messages for translation that were
not yet marked as such.

Signed-off-by: Dennis Kaarsemaker <dennis@kaarsemaker.net>
---
 git-send-email.perl | 54 ++++++++++++++++++++++++++++++++++-------------------
 1 file changed, 35 insertions(+), 19 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index eea0a517f7..0d90439d9a 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -1353,10 +1353,12 @@ EOF
 			die __("The required SMTP server is not properly defined.")
 		}
 
+		require Net::SMTP;
+		my $use_net_smtp_ssl = version->parse($Net::SMTP::VERSION) < version->parse("1.28");
+		$smtp_domain ||= maildomain();
+
 		if ($smtp_encryption eq 'ssl') {
 			$smtp_server_port ||= 465; # ssmtp
-			require Net::SMTP::SSL;
-			$smtp_domain ||= maildomain();
 			require IO::Socket::SSL;
 
 			# Suppress "variable accessed once" warning.
@@ -1368,34 +1370,48 @@ EOF
 			# Net::SMTP::SSL->new() does not forward any SSL options
 			IO::Socket::SSL::set_client_defaults(
 				ssl_verify_params());
-			$smtp ||= Net::SMTP::SSL->new($smtp_server,
-						      Hello => $smtp_domain,
-						      Port => $smtp_server_port,
-						      Debug => $debug_net_smtp);
+
+			if ($use_net_smtp_ssl) {
+				require Net::SMTP::SSL;
+				$smtp ||= Net::SMTP::SSL->new($smtp_server,
+							      Hello => $smtp_domain,
+							      Port => $smtp_server_port,
+							      Debug => $debug_net_smtp);
+			}
+			else {
+				$smtp ||= Net::SMTP->new($smtp_server,
+							 Hello => $smtp_domain,
+							 Port => $smtp_server_port,
+							 Debug => $debug_net_smtp,
+							 SSL => 1);
+			}
 		}
 		else {
-			require Net::SMTP;
-			$smtp_domain ||= maildomain();
 			$smtp_server_port ||= 25;
 			$smtp ||= Net::SMTP->new($smtp_server,
 						 Hello => $smtp_domain,
 						 Debug => $debug_net_smtp,
 						 Port => $smtp_server_port);
 			if ($smtp_encryption eq 'tls' && $smtp) {
-				require Net::SMTP::SSL;
-				$smtp->command('STARTTLS');
-				$smtp->response();
-				if ($smtp->code == 220) {
+				if ($use_net_smtp_ssl) {
+					$smtp->command('STARTTLS');
+					$smtp->response();
+					if ($smtp->code != 220) {
+						die sprintf(__("Server does not support STARTTLS! %s"), $smtp->message);
+					}
+					require Net::SMTP::SSL;
 					$smtp = Net::SMTP::SSL->start_SSL($smtp,
 									  ssl_verify_params())
-						or die "STARTTLS failed! ".IO::Socket::SSL::errstr();
-					$smtp_encryption = '';
-					# Send EHLO again to receive fresh
-					# supported commands
-					$smtp->hello($smtp_domain);
-				} else {
-					die sprintf(__("Server does not support STARTTLS! %s"), $smtp->message);
+						or die sprintf(__("STARTTLS failed! %s"), IO::Socket::SSL::errstr());
+				}
+				else {
+					$smtp->starttls(ssl_verify_params())
+						or die sprintf(__("STARTTLS failed! %s"), IO::Socket::SSL::errstr());
 				}
+				$smtp_encryption = '';
+				# Send EHLO again to receive fresh
+				# supported commands
+				$smtp->hello($smtp_domain);
 			}
 		}
 
-- 
2.12.0-488-gd3584ba


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH v2] send-email: Net::SMTP::SSL is obsolete, use only when necessary
  2017-03-24 21:37     ` [PATCH v2] " Dennis Kaarsemaker
@ 2017-05-04  7:01       ` Dennis Kaarsemaker
  2017-05-19 20:54         ` Dennis Kaarsemaker
  2017-05-31 21:46       ` Jonathan Nieder
  1 sibling, 1 reply; 16+ messages in thread
From: Dennis Kaarsemaker @ 2017-05-04  7:01 UTC (permalink / raw)
  To: git

Ping. It's a little over a month since I sent this, but I haven't seen
any comments. Is this commit good to go?

On Fri, 2017-03-24 at 22:37 +0100, Dennis Kaarsemaker wrote:
> Net::SMTP itself can do the necessary SSL and STARTTLS bits just fine
> since version 1.28, and Net::SMTP::SSL is now deprecated. Since 1.28
> isn't that old yet, keep the old code in place and use it when
> necessary.
> 
> While we're in the area, mark some messages for translation that were
> not yet marked as such.
> 
> Signed-off-by: Dennis Kaarsemaker <dennis@kaarsemaker.net>
> ---
>  git-send-email.perl | 54 ++++++++++++++++++++++++++++++++++-------------------
>  1 file changed, 35 insertions(+), 19 deletions(-)
> 
> diff --git a/git-send-email.perl b/git-send-email.perl
> index eea0a517f7..0d90439d9a 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -1353,10 +1353,12 @@ EOF
>  			die __("The required SMTP server is not properly defined.")
>  		}
>  
> +		require Net::SMTP;
> +		my $use_net_smtp_ssl = version->parse($Net::SMTP::VERSION) < version->parse("1.28");
> +		$smtp_domain ||= maildomain();
> +
>  		if ($smtp_encryption eq 'ssl') {
>  			$smtp_server_port ||= 465; # ssmtp
> -			require Net::SMTP::SSL;
> -			$smtp_domain ||= maildomain();
>  			require IO::Socket::SSL;
>  
>  			# Suppress "variable accessed once" warning.
> @@ -1368,34 +1370,48 @@ EOF
>  			# Net::SMTP::SSL->new() does not forward any SSL options
>  			IO::Socket::SSL::set_client_defaults(
>  				ssl_verify_params());
> -			$smtp ||= Net::SMTP::SSL->new($smtp_server,
> -						      Hello => $smtp_domain,
> -						      Port => $smtp_server_port,
> -						      Debug => $debug_net_smtp);
> +
> +			if ($use_net_smtp_ssl) {
> +				require Net::SMTP::SSL;
> +				$smtp ||= Net::SMTP::SSL->new($smtp_server,
> +							      Hello => $smtp_domain,
> +							      Port => $smtp_server_port,
> +							      Debug => $debug_net_smtp);
> +			}
> +			else {
> +				$smtp ||= Net::SMTP->new($smtp_server,
> +							 Hello => $smtp_domain,
> +							 Port => $smtp_server_port,
> +							 Debug => $debug_net_smtp,
> +							 SSL => 1);
> +			}
>  		}
>  		else {
> -			require Net::SMTP;
> -			$smtp_domain ||= maildomain();
>  			$smtp_server_port ||= 25;
>  			$smtp ||= Net::SMTP->new($smtp_server,
>  						 Hello => $smtp_domain,
>  						 Debug => $debug_net_smtp,
>  						 Port => $smtp_server_port);
>  			if ($smtp_encryption eq 'tls' && $smtp) {
> -				require Net::SMTP::SSL;
> -				$smtp->command('STARTTLS');
> -				$smtp->response();
> -				if ($smtp->code == 220) {
> +				if ($use_net_smtp_ssl) {
> +					$smtp->command('STARTTLS');
> +					$smtp->response();
> +					if ($smtp->code != 220) {
> +						die sprintf(__("Server does not support STARTTLS! %s"), $smtp->message);
> +					}
> +					require Net::SMTP::SSL;
>  					$smtp = Net::SMTP::SSL->start_SSL($smtp,
>  									  ssl_verify_params())
> -						or die "STARTTLS failed! ".IO::Socket::SSL::errstr();
> -					$smtp_encryption = '';
> -					# Send EHLO again to receive fresh
> -					# supported commands
> -					$smtp->hello($smtp_domain);
> -				} else {
> -					die sprintf(__("Server does not support STARTTLS! %s"), $smtp->message);
> +						or die sprintf(__("STARTTLS failed! %s"), IO::Socket::SSL::errstr());
> +				}
> +				else {
> +					$smtp->starttls(ssl_verify_params())
> +						or die sprintf(__("STARTTLS failed! %s"), IO::Socket::SSL::errstr());
>  				}
> +				$smtp_encryption = '';
> +				# Send EHLO again to receive fresh
> +				# supported commands
> +				$smtp->hello($smtp_domain);
>  			}
>  		}
>  

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2] send-email: Net::SMTP::SSL is obsolete, use only when necessary
  2017-05-04  7:01       ` Dennis Kaarsemaker
@ 2017-05-19 20:54         ` Dennis Kaarsemaker
  2017-05-20  7:56           ` Ævar Arnfjörð Bjarmason
  2017-05-31 22:50           ` Junio C Hamano
  0 siblings, 2 replies; 16+ messages in thread
From: Dennis Kaarsemaker @ 2017-05-19 20:54 UTC (permalink / raw)
  To: git, avarab, gitster

Second ping. This problem is not going away, so if this solution is not
acceptable, I'd like to know what needs to be improved.

On Thu, 2017-05-04 at 09:01 +0200, Dennis Kaarsemaker wrote:
> Ping. It's a little over a month since I sent this, but I haven't seen
> any comments. Is this commit good to go?
> 
> On Fri, 2017-03-24 at 22:37 +0100, Dennis Kaarsemaker wrote:
> > Net::SMTP itself can do the necessary SSL and STARTTLS bits just fine
> > since version 1.28, and Net::SMTP::SSL is now deprecated. Since 1.28
> > isn't that old yet, keep the old code in place and use it when
> > necessary.
> > 
> > While we're in the area, mark some messages for translation that were
> > not yet marked as such.
> > 
> > Signed-off-by: Dennis Kaarsemaker <dennis@kaarsemaker.net>
> > ---
> >  git-send-email.perl | 54 ++++++++++++++++++++++++++++++++++-------------------
> >  1 file changed, 35 insertions(+), 19 deletions(-)
> > 
> > diff --git a/git-send-email.perl b/git-send-email.perl
> > index eea0a517f7..0d90439d9a 100755
> > --- a/git-send-email.perl
> > +++ b/git-send-email.perl
> > @@ -1353,10 +1353,12 @@ EOF
> >  			die __("The required SMTP server is not properly defined.")
> >  		}
> >  
> > +		require Net::SMTP;
> > +		my $use_net_smtp_ssl = version->parse($Net::SMTP::VERSION) < version->parse("1.28");
> > +		$smtp_domain ||= maildomain();
> > +
> >  		if ($smtp_encryption eq 'ssl') {
> >  			$smtp_server_port ||= 465; # ssmtp
> > -			require Net::SMTP::SSL;
> > -			$smtp_domain ||= maildomain();
> >  			require IO::Socket::SSL;
> >  
> >  			# Suppress "variable accessed once" warning.
> > @@ -1368,34 +1370,48 @@ EOF
> >  			# Net::SMTP::SSL->new() does not forward any SSL options
> >  			IO::Socket::SSL::set_client_defaults(
> >  				ssl_verify_params());
> > -			$smtp ||= Net::SMTP::SSL->new($smtp_server,
> > -						      Hello => $smtp_domain,
> > -						      Port => $smtp_server_port,
> > -						      Debug => $debug_net_smtp);
> > +
> > +			if ($use_net_smtp_ssl) {
> > +				require Net::SMTP::SSL;
> > +				$smtp ||= Net::SMTP::SSL->new($smtp_server,
> > +							      Hello => $smtp_domain,
> > +							      Port => $smtp_server_port,
> > +							      Debug => $debug_net_smtp);
> > +			}
> > +			else {
> > +				$smtp ||= Net::SMTP->new($smtp_server,
> > +							 Hello => $smtp_domain,
> > +							 Port => $smtp_server_port,
> > +							 Debug => $debug_net_smtp,
> > +							 SSL => 1);
> > +			}
> >  		}
> >  		else {
> > -			require Net::SMTP;
> > -			$smtp_domain ||= maildomain();
> >  			$smtp_server_port ||= 25;
> >  			$smtp ||= Net::SMTP->new($smtp_server,
> >  						 Hello => $smtp_domain,
> >  						 Debug => $debug_net_smtp,
> >  						 Port => $smtp_server_port);
> >  			if ($smtp_encryption eq 'tls' && $smtp) {
> > -				require Net::SMTP::SSL;
> > -				$smtp->command('STARTTLS');
> > -				$smtp->response();
> > -				if ($smtp->code == 220) {
> > +				if ($use_net_smtp_ssl) {
> > +					$smtp->command('STARTTLS');
> > +					$smtp->response();
> > +					if ($smtp->code != 220) {
> > +						die sprintf(__("Server does not support STARTTLS! %s"), $smtp->message);
> > +					}
> > +					require Net::SMTP::SSL;
> >  					$smtp = Net::SMTP::SSL->start_SSL($smtp,
> >  									  ssl_verify_params())
> > -						or die "STARTTLS failed! ".IO::Socket::SSL::errstr();
> > -					$smtp_encryption = '';
> > -					# Send EHLO again to receive fresh
> > -					# supported commands
> > -					$smtp->hello($smtp_domain);
> > -				} else {
> > -					die sprintf(__("Server does not support STARTTLS! %s"), $smtp->message);
> > +						or die sprintf(__("STARTTLS failed! %s"), IO::Socket::SSL::errstr());
> > +				}
> > +				else {
> > +					$smtp->starttls(ssl_verify_params())
> > +						or die sprintf(__("STARTTLS failed! %s"), IO::Socket::SSL::errstr());
> >  				}
> > +				$smtp_encryption = '';
> > +				# Send EHLO again to receive fresh
> > +				# supported commands
> > +				$smtp->hello($smtp_domain);
> >  			}
> >  		}
> >  

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2] send-email: Net::SMTP::SSL is obsolete, use only when necessary
  2017-05-19 20:54         ` Dennis Kaarsemaker
@ 2017-05-20  7:56           ` Ævar Arnfjörð Bjarmason
  2017-05-31 22:50           ` Junio C Hamano
  1 sibling, 0 replies; 16+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2017-05-20  7:56 UTC (permalink / raw)
  To: Dennis Kaarsemaker; +Cc: Git Mailing List, Junio C Hamano

On Fri, May 19, 2017 at 10:54 PM, Dennis Kaarsemaker
<dennis@kaarsemaker.net> wrote:
> Second ping. This problem is not going away, so if this solution is not
> acceptable, I'd like to know what needs to be improved.

FWIW:

Reviewed-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>

> On Thu, 2017-05-04 at 09:01 +0200, Dennis Kaarsemaker wrote:
>> Ping. It's a little over a month since I sent this, but I haven't seen
>> any comments. Is this commit good to go?
>>
>> On Fri, 2017-03-24 at 22:37 +0100, Dennis Kaarsemaker wrote:
>> > Net::SMTP itself can do the necessary SSL and STARTTLS bits just fine
>> > since version 1.28, and Net::SMTP::SSL is now deprecated. Since 1.28
>> > isn't that old yet, keep the old code in place and use it when
>> > necessary.
>> >
>> > While we're in the area, mark some messages for translation that were
>> > not yet marked as such.
>> >
>> > Signed-off-by: Dennis Kaarsemaker <dennis@kaarsemaker.net>
>> > ---
>> >  git-send-email.perl | 54 ++++++++++++++++++++++++++++++++++-------------------
>> >  1 file changed, 35 insertions(+), 19 deletions(-)
>> >
>> > diff --git a/git-send-email.perl b/git-send-email.perl
>> > index eea0a517f7..0d90439d9a 100755
>> > --- a/git-send-email.perl
>> > +++ b/git-send-email.perl
>> > @@ -1353,10 +1353,12 @@ EOF
>> >                     die __("The required SMTP server is not properly defined.")
>> >             }
>> >
>> > +           require Net::SMTP;
>> > +           my $use_net_smtp_ssl = version->parse($Net::SMTP::VERSION) < version->parse("1.28");
>> > +           $smtp_domain ||= maildomain();
>> > +
>> >             if ($smtp_encryption eq 'ssl') {
>> >                     $smtp_server_port ||= 465; # ssmtp
>> > -                   require Net::SMTP::SSL;
>> > -                   $smtp_domain ||= maildomain();
>> >                     require IO::Socket::SSL;
>> >
>> >                     # Suppress "variable accessed once" warning.
>> > @@ -1368,34 +1370,48 @@ EOF
>> >                     # Net::SMTP::SSL->new() does not forward any SSL options
>> >                     IO::Socket::SSL::set_client_defaults(
>> >                             ssl_verify_params());
>> > -                   $smtp ||= Net::SMTP::SSL->new($smtp_server,
>> > -                                                 Hello => $smtp_domain,
>> > -                                                 Port => $smtp_server_port,
>> > -                                                 Debug => $debug_net_smtp);
>> > +
>> > +                   if ($use_net_smtp_ssl) {
>> > +                           require Net::SMTP::SSL;
>> > +                           $smtp ||= Net::SMTP::SSL->new($smtp_server,
>> > +                                                         Hello => $smtp_domain,
>> > +                                                         Port => $smtp_server_port,
>> > +                                                         Debug => $debug_net_smtp);
>> > +                   }
>> > +                   else {
>> > +                           $smtp ||= Net::SMTP->new($smtp_server,
>> > +                                                    Hello => $smtp_domain,
>> > +                                                    Port => $smtp_server_port,
>> > +                                                    Debug => $debug_net_smtp,
>> > +                                                    SSL => 1);
>> > +                   }
>> >             }
>> >             else {
>> > -                   require Net::SMTP;
>> > -                   $smtp_domain ||= maildomain();
>> >                     $smtp_server_port ||= 25;
>> >                     $smtp ||= Net::SMTP->new($smtp_server,
>> >                                              Hello => $smtp_domain,
>> >                                              Debug => $debug_net_smtp,
>> >                                              Port => $smtp_server_port);
>> >                     if ($smtp_encryption eq 'tls' && $smtp) {
>> > -                           require Net::SMTP::SSL;
>> > -                           $smtp->command('STARTTLS');
>> > -                           $smtp->response();
>> > -                           if ($smtp->code == 220) {
>> > +                           if ($use_net_smtp_ssl) {
>> > +                                   $smtp->command('STARTTLS');
>> > +                                   $smtp->response();
>> > +                                   if ($smtp->code != 220) {
>> > +                                           die sprintf(__("Server does not support STARTTLS! %s"), $smtp->message);
>> > +                                   }
>> > +                                   require Net::SMTP::SSL;
>> >                                     $smtp = Net::SMTP::SSL->start_SSL($smtp,
>> >                                                                       ssl_verify_params())
>> > -                                           or die "STARTTLS failed! ".IO::Socket::SSL::errstr();
>> > -                                   $smtp_encryption = '';
>> > -                                   # Send EHLO again to receive fresh
>> > -                                   # supported commands
>> > -                                   $smtp->hello($smtp_domain);
>> > -                           } else {
>> > -                                   die sprintf(__("Server does not support STARTTLS! %s"), $smtp->message);
>> > +                                           or die sprintf(__("STARTTLS failed! %s"), IO::Socket::SSL::errstr());
>> > +                           }
>> > +                           else {
>> > +                                   $smtp->starttls(ssl_verify_params())
>> > +                                           or die sprintf(__("STARTTLS failed! %s"), IO::Socket::SSL::errstr());
>> >                             }
>> > +                           $smtp_encryption = '';
>> > +                           # Send EHLO again to receive fresh
>> > +                           # supported commands
>> > +                           $smtp->hello($smtp_domain);
>> >                     }
>> >             }
>> >

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2] send-email: Net::SMTP::SSL is obsolete, use only when necessary
  2017-05-19 20:54         ` Dennis Kaarsemaker
  2017-05-20  7:56           ` Ævar Arnfjörð Bjarmason
@ 2017-05-31 22:50           ` Junio C Hamano
  2017-06-01 19:42             ` Dennis Kaarsemaker
  1 sibling, 1 reply; 16+ messages in thread
From: Junio C Hamano @ 2017-05-31 22:50 UTC (permalink / raw)
  To: Dennis Kaarsemaker; +Cc: git, avarab, Eric Biggers

Dennis Kaarsemaker <dennis@kaarsemaker.net> writes:

> Second ping. This problem is not going away, so if this solution is not
> acceptable, I'd like to know what needs to be improved.

Perhaps you needed to actually test with older installation that
people have, it seems, between pings.  Immediately after this was
merged to 'master', we start getting bug reports X-<.

Eric Biggers' message 

    https://public-inbox.org/git/<20170531222455.GD72735@gmail.com>

seems to indicate that we should cut off at 3.01 not 1.28?

Thanks.

> On Thu, 2017-05-04 at 09:01 +0200, Dennis Kaarsemaker wrote:
>> Ping. It's a little over a month since I sent this, but I haven't seen
>> any comments. Is this commit good to go?
>> 
>> On Fri, 2017-03-24 at 22:37 +0100, Dennis Kaarsemaker wrote:
>> > Net::SMTP itself can do the necessary SSL and STARTTLS bits just fine
>> > since version 1.28, and Net::SMTP::SSL is now deprecated. Since 1.28
>> > isn't that old yet, keep the old code in place and use it when
>> > necessary.
>> > ...
>> > diff --git a/git-send-email.perl b/git-send-email.perl
>> > index eea0a517f7..0d90439d9a 100755
>> > --- a/git-send-email.perl
>> > +++ b/git-send-email.perl
>> > @@ -1353,10 +1353,12 @@ EOF
>> >  			die __("The required SMTP server is not properly defined.")
>> >  		}
>> >  
>> > +		require Net::SMTP;
>> > +		my $use_net_smtp_ssl = version->parse($Net::SMTP::VERSION) < version->parse("1.28");

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2] send-email: Net::SMTP::SSL is obsolete, use only when necessary
  2017-05-31 22:50           ` Junio C Hamano
@ 2017-06-01 19:42             ` Dennis Kaarsemaker
  0 siblings, 0 replies; 16+ messages in thread
From: Dennis Kaarsemaker @ 2017-06-01 19:42 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, avarab, Eric Biggers

On Thu, 2017-06-01 at 07:50 +0900, Junio C Hamano wrote:
> Dennis Kaarsemaker <dennis@kaarsemaker.net> writes:
> 
> > Second ping. This problem is not going away, so if this solution is not
> > acceptable, I'd like to know what needs to be improved.
> 
> Perhaps you needed to actually test with older installation that
> people have, it seems, between pings.  Immediately after this was
> merged to 'master', we start getting bug reports X-<.
> 
> Eric Biggers' message 
> 
>     https://public-inbox.org/git/<20170531222455.GD72735@gmail.com>;
> 
> seems to indicate that we should cut off at 3.01 not 1.28?
> 
> Thanks.

Apologies for that. The version numbering of libnet is *weird*. The
libnet version this was fixed in *is* 1.28, but the Net::SMTP module in
 libnet version 1.28 has version 2.33.

I did test with some older perl versions, but not far enough back it
seems :(

D.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2] send-email: Net::SMTP::SSL is obsolete, use only when necessary
  2017-03-24 21:37     ` [PATCH v2] " Dennis Kaarsemaker
  2017-05-04  7:01       ` Dennis Kaarsemaker
@ 2017-05-31 21:46       ` Jonathan Nieder
  2017-05-31 22:39         ` Junio C Hamano
  1 sibling, 1 reply; 16+ messages in thread
From: Jonathan Nieder @ 2017-05-31 21:46 UTC (permalink / raw)
  To: Dennis Kaarsemaker
  Cc: git, Brandon Williams, Ævar Arnfjörð Bjarmason,
	Junio C Hamano

Hi,

Dennis Kaarsemaker wrote:

> Net::SMTP itself can do the necessary SSL and STARTTLS bits just fine
> since version 1.28, and Net::SMTP::SSL is now deprecated. Since 1.28
> isn't that old yet, keep the old code in place and use it when
> necessary.

This broke git send-email for me.  The error message is

  Can't locate object method "starttls" via package "Net::SMTP" at /usr/lib/git-core/git-send-email line 1410.

Is 1.28 the right minimum version?

  $ perl -e 'require Net::SMTP; print version->parse($Net::SMTP::VERSION); print "\n"'
  2.31
  $ grep VERSION /usr/share/perl/5.18.2/Net/SMTP.pm
  use vars qw($VERSION @ISA);
  $VERSION = "2.31";
  $ grep starttls /usr/share/perl/5.18.2/Net/SMTP.pm
  $ dpkg-query -W perl
  perl    5.18.2-2ubuntu1.1

Patch left unsnipped for reference.

Thanks,
Jonathan

> While we're in the area, mark some messages for translation that were
> not yet marked as such.
>
> Signed-off-by: Dennis Kaarsemaker <dennis@kaarsemaker.net>
> ---
>  git-send-email.perl | 54 ++++++++++++++++++++++++++++++++++-------------------
>  1 file changed, 35 insertions(+), 19 deletions(-)
> 
> diff --git a/git-send-email.perl b/git-send-email.perl
> index eea0a517f7..0d90439d9a 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -1353,10 +1353,12 @@ EOF
>  			die __("The required SMTP server is not properly defined.")
>  		}
>  
> +		require Net::SMTP;
> +		my $use_net_smtp_ssl = version->parse($Net::SMTP::VERSION) < version->parse("1.28");
> +		$smtp_domain ||= maildomain();
> +
>  		if ($smtp_encryption eq 'ssl') {
>  			$smtp_server_port ||= 465; # ssmtp
> -			require Net::SMTP::SSL;
> -			$smtp_domain ||= maildomain();
>  			require IO::Socket::SSL;
>  
>  			# Suppress "variable accessed once" warning.
> @@ -1368,34 +1370,48 @@ EOF
>  			# Net::SMTP::SSL->new() does not forward any SSL options
>  			IO::Socket::SSL::set_client_defaults(
>  				ssl_verify_params());
> -			$smtp ||= Net::SMTP::SSL->new($smtp_server,
> -						      Hello => $smtp_domain,
> -						      Port => $smtp_server_port,
> -						      Debug => $debug_net_smtp);
> +
> +			if ($use_net_smtp_ssl) {
> +				require Net::SMTP::SSL;
> +				$smtp ||= Net::SMTP::SSL->new($smtp_server,
> +							      Hello => $smtp_domain,
> +							      Port => $smtp_server_port,
> +							      Debug => $debug_net_smtp);
> +			}
> +			else {
> +				$smtp ||= Net::SMTP->new($smtp_server,
> +							 Hello => $smtp_domain,
> +							 Port => $smtp_server_port,
> +							 Debug => $debug_net_smtp,
> +							 SSL => 1);
> +			}
>  		}
>  		else {
> -			require Net::SMTP;
> -			$smtp_domain ||= maildomain();
>  			$smtp_server_port ||= 25;
>  			$smtp ||= Net::SMTP->new($smtp_server,
>  						 Hello => $smtp_domain,
>  						 Debug => $debug_net_smtp,
>  						 Port => $smtp_server_port);
>  			if ($smtp_encryption eq 'tls' && $smtp) {
> -				require Net::SMTP::SSL;
> -				$smtp->command('STARTTLS');
> -				$smtp->response();
> -				if ($smtp->code == 220) {
> +				if ($use_net_smtp_ssl) {
> +					$smtp->command('STARTTLS');
> +					$smtp->response();
> +					if ($smtp->code != 220) {
> +						die sprintf(__("Server does not support STARTTLS! %s"), $smtp->message);
> +					}
> +					require Net::SMTP::SSL;
>  					$smtp = Net::SMTP::SSL->start_SSL($smtp,
>  									  ssl_verify_params())
> -						or die "STARTTLS failed! ".IO::Socket::SSL::errstr();
> -					$smtp_encryption = '';
> -					# Send EHLO again to receive fresh
> -					# supported commands
> -					$smtp->hello($smtp_domain);
> -				} else {
> -					die sprintf(__("Server does not support STARTTLS! %s"), $smtp->message);
> +						or die sprintf(__("STARTTLS failed! %s"), IO::Socket::SSL::errstr());
> +				}
> +				else {
> +					$smtp->starttls(ssl_verify_params())
> +						or die sprintf(__("STARTTLS failed! %s"), IO::Socket::SSL::errstr());
>  				}
> +				$smtp_encryption = '';
> +				# Send EHLO again to receive fresh
> +				# supported commands
> +				$smtp->hello($smtp_domain);
>  			}
>  		}
>  
> -- 
> 2.12.0-488-gd3584ba
> 

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2] send-email: Net::SMTP::SSL is obsolete, use only when necessary
  2017-05-31 21:46       ` Jonathan Nieder
@ 2017-05-31 22:39         ` Junio C Hamano
  2017-05-31 22:53           ` Jonathan Nieder
  0 siblings, 1 reply; 16+ messages in thread
From: Junio C Hamano @ 2017-05-31 22:39 UTC (permalink / raw)
  To: Jonathan Nieder
  Cc: Dennis Kaarsemaker, git, Brandon Williams,
	Ævar Arnfjörð Bjarmason

Jonathan Nieder <jrnieder@gmail.com> writes:

> Dennis Kaarsemaker wrote:
>
>> Net::SMTP itself can do the necessary SSL and STARTTLS bits just fine
>> since version 1.28, and Net::SMTP::SSL is now deprecated. Since 1.28
>> isn't that old yet, keep the old code in place and use it when
>> necessary.
>
> This broke git send-email for me.  The error message is
>
>   Can't locate object method "starttls" via package "Net::SMTP" at /usr/lib/git-core/git-send-email line 1410.
>
> Is 1.28 the right minimum version?
>
>   $ perl -e 'require Net::SMTP; print version->parse($Net::SMTP::VERSION); print "\n"'
>   2.31
>   $ grep VERSION /usr/share/perl/5.18.2/Net/SMTP.pm
>   use vars qw($VERSION @ISA);
>   $VERSION = "2.31";
>   $ grep starttls /usr/share/perl/5.18.2/Net/SMTP.pm
>   $ dpkg-query -W perl
>   perl    5.18.2-2ubuntu1.1
>
Thanks.  

Let's revert the merge for now until we know (this time for certain)
what the minimum version is.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2] send-email: Net::SMTP::SSL is obsolete, use only when necessary
  2017-05-31 22:39         ` Junio C Hamano
@ 2017-05-31 22:53           ` Jonathan Nieder
  0 siblings, 0 replies; 16+ messages in thread
From: Jonathan Nieder @ 2017-05-31 22:53 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Dennis Kaarsemaker, git, Brandon Williams,
	Ævar Arnfjörð Bjarmason

Hi,

Jun 01, 2017 at 07:39:16AM +0900, Junio C Hamano wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:

>> This broke git send-email for me.  The error message is
>>
>>   Can't locate object method "starttls" via package "Net::SMTP" at /usr/lib/git-core/git-send-email line 1410.
>>
>> Is 1.28 the right minimum version?
>>
>>   $ perl -e 'require Net::SMTP; print version->parse($Net::SMTP::VERSION); print "\n"'
[...]
>>   $ grep starttls /usr/share/perl/5.18.2/Net/SMTP.pm
>>   $ dpkg-query -W perl
>>   perl    5.18.2-2ubuntu1.1
>
> Thanks.
>
> Let's revert the merge for now until we know (this time for certain)
> what the minimum version is.

Thanks.  I just sent
http://public-inbox.org/git/20170531224415.GC81679@aiede.mtv.corp.google.com
in response to another thread.  That uses 3.01 as minimum version,
since it is the minimum version imported to core perl with starttls
support.

I haven't tried testing with historical Net::SMTP versions, though.
Is there a git repository available with all Net::SMTP versions from
CPAN, or is https://perl5.git.perl.org/perl.git as good as it gets?

Regards,
Jonathan

^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2017-06-01 19:42 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-11-20 21:18 [PATCH] Remove dependency on deprecated Net::SMTP::SSL Mike Fisher
2016-11-20 21:53 ` brian m. carlson
2017-01-13 14:59   ` Renato Botelho
2016-11-21  5:37 ` Torsten Bögershausen
2017-03-18 22:23 ` [PATCH] send-email: Net::SMTP::SSL is obsolete, use only when necessary Dennis Kaarsemaker
2017-03-18 22:47   ` Ævar Arnfjörð Bjarmason
2017-03-18 23:14     ` Dennis Kaarsemaker
2017-03-24 21:37     ` [PATCH v2] " Dennis Kaarsemaker
2017-05-04  7:01       ` Dennis Kaarsemaker
2017-05-19 20:54         ` Dennis Kaarsemaker
2017-05-20  7:56           ` Ævar Arnfjörð Bjarmason
2017-05-31 22:50           ` Junio C Hamano
2017-06-01 19:42             ` Dennis Kaarsemaker
2017-05-31 21:46       ` Jonathan Nieder
2017-05-31 22:39         ` Junio C Hamano
2017-05-31 22:53           ` Jonathan Nieder

Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).