git@vger.kernel.org mailing list mirror (one of many)
 help / Atom feed
From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
To: Dennis Kaarsemaker <dennis@kaarsemaker.net>
Cc: Git Mailing List <git@vger.kernel.org>
Subject: Re: [PATCH] send-email: Net::SMTP::SSL is obsolete, use only when necessary
Date: Sat, 18 Mar 2017 23:47:35 +0100
Message-ID: <CACBZZX5j1dYk8aeRED7T7iJ=b32aFUpfUWPpMpmtofBL3QnVXQ@mail.gmail.com> (raw)
In-Reply-To: <20170318222311.9993-1-dennis@kaarsemaker.net>

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.

  reply index

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
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 [this message]
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

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CACBZZX5j1dYk8aeRED7T7iJ=b32aFUpfUWPpMpmtofBL3QnVXQ@mail.gmail.com' \
    --to=avarab@gmail.com \
    --cc=dennis@kaarsemaker.net \
    --cc=git@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

git@vger.kernel.org mailing list mirror (one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.org/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/
       or Tor2web: https://www.tor2web.org/

AGPL code for this site: git clone https://public-inbox.org/ public-inbox