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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Mar 18, 2017, 22:47 UTC
Message-ID: <CACBZZX5j1dYk8aeRED7T7iJ=b32aFUpfUWPpMpmtofBL3QnVXQ@mail.gmail.com>
In-Reply-To: <20170318222311.9993-1-dennis@kaarsemaker.net>

On Sat, Mar 18, 2017 at 11:23 PM, Dennis Kaarsemaker <dennis@kaarsemaker.net> wrote:

Show 25 quoted lines

> 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.

Show 16 quoted lines

> +                                       }
> +                                       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.