Re: [PATCH] send-email: Net::SMTP::SSL is obsolete, use only when necessary

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

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.