So on a second reading.
So first, I've been sitting on some fairly extensive send-email patches locally, but have been trying to focus on re-rolling some of my outstanding stuff.
But I just sent two patches directly relevant to this series as https://lore.kernel.org/git/cover-0.2-00000000000-20210411T144128Z-avarab@gmail.com/
Something felt a bit wrong about the approach in your series, I wasn't quite sure what initially, but here it is;
So, the only reason we have that "encryption is none -- this only prevents a warning" so late in the file (as opposed to setting it to '' when we declare the variable) was because of the smtp.{smtpssl,smtpencryption} interaction, i.e. we relied on it being undef to see if we needed to parse the secondary variable.
But with it gone with my small series (it already didn't work) we can get rid of that special case.
But, on the specifics of the "felt funny":
1. Your 2/3 changes a long standing existing "any other value = no
encryption" to "die on unrecognized". I happen to think this is
probably a good idea, but let's be explicit in the commit message,
e.g.: We don't think it's a good idea to silently degrade to
non-encrypted as we've been promising just because your version
doesn't support something, let's die instead. 2. If we're breaking the "any other value" we should not be documenting
the "or nothing", the distinction between "" and undef on the
Perl-level was just a leaky implementation detail. But let's not conflate that with how we present something to the
user. It's not the same to not set a variable v.s. setting it to the
empty string. With my 2-part series it's even more trivial to detect that, but
even on top of master you just move your check above the "set to
empty unless defined". 3. While I'm very much leaning to #1 being a good idea, I'm very much
leaning towards introducing this "starttls" alias being a bad idea
for the same reason.
I.e. let's not create a new 'starttls' if we can avoid it explicitly
because we used to have the long-standing "anything unrecognized is
empty == no encryption" behavior. A lot of users read documentation for the latest version online, but
may have an older version installed. To the extent that anyone cares about the transport security of
git-send-email (I'm kind of "meh" on it, but if we're making
sendemail.smtpEncryption parsing strict you probably disagree), then
such silent downgrading seems worse than just not accepting starttls
at all. I.e. to have a new behavior of something like: if (defined $smtp_encryption) {
die "we call 'starttls' 'tls' for historical reasons, sorry!" if $smtp_encryption eq 'starttls';
die "unknown mode '$smtp_encryption'" unless $smtp_encryption =~ /^(?:ssl|tls)$/s;
} else {
$smtp_encryption = '';
} I.e. I get that it's confusing, but isn't it enough to address the
TLS v.s. STARTTLS confusion in the docs, as opposed to supporting it
in the config format, which as noted will silently downgrade on
older versions?
Drew DeVault
