Re: [PATCH v2 2/3] git-send-email: die on invalid smtp_encryption

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Apr 11, 2021, 14:20 UTC
Message-ID: <87blakgaxr.fsf@evledraar.gmail.com>
In-Reply-To: <20210411125431.28971-3-sir@cmpwn.com>

On Sun, Apr 11 2021, Drew DeVault wrote:

Show 32 quoted lines

> Signed-off-by: Drew DeVault <sir@cmpwn.com>
> ---
>  Documentation/git-send-email.txt | 4 ++--
>  git-send-email.perl              | 3 +++
>  2 files changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt
> index c17c3b400a..520b355e50 100644
> --- a/Documentation/git-send-email.txt
> +++ b/Documentation/git-send-email.txt
> @@ -171,8 +171,8 @@ Sending
>  	Specify the encryption to use, either 'ssl' or 'tls'. 'ssl' enables
>  	generic SSL/TLS support and is typically used on port 465.  'tls'
>  	enables in-band STARTTLS support and is typically used on port 25 or
> -	587.  Use whichever option is recommended by your mail provider.  Any
> -	other value reverts to plain SMTP.  Default is the value of
> +	587.  Use whichever option is recommended by your mail provider.  Leave
> +	empty to disable encryption and use plain SMTP.  Default is the value of
>  	`sendemail.smtpEncryption`.
>  
>  --smtp-domain=<FQDN>::
> diff --git a/git-send-email.perl b/git-send-email.perl
> index f5bbf1647e..bda5211f0d 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -495,6 +495,9 @@ sub read_config {
>  
>  # 'default' encryption is none -- this only prevents a warning
>  $smtp_encryption = '' unless (defined $smtp_encryption);
> +if ($smtp_encryption ne "" && $smtp_encryption ne "ssl" && $smtp_encryption ne "tls") {
> +	die __("Invalid smtp_encryption configuration: expected 'ssl', 'tls', or nothing.\n");
> +}

Having not tested this but just eyeballed the code, I'm fairly sure you're adding a logic error here, or is $smtp_encryption guaranteed to be defined at this point?

We start with it as undef, then read the config, then the CLI options. If we've got neither it'll still be undef here, no?

Thus the string comparison will emit a warning.

Maybe I'm overly used to regexen in Perl, but I'd also find this more readable as:

    $smtp_encryption !~ /^(?:|ssl|tls)$/s

Or something like:

    my @valid_smtp_encryption = ('', qw(ssl tls));
    if (!grep { $_ eq $smtp_encryption } @valid_smtp_encryption) {
        ....
    }