| Message ID | patch-1.2-ee041188e55-20210411T144128Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 671818ab0b8206481d38054477059fee46db0ba3 |
| Headers | show |
| Series | send-email: simplify smtp.{smtpssl,smtpencryption} parsing | expand |
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > So let's just remove it instead of fixing the bug, clearly nobody's > cared enough to complain. Hmph, is that a safe assumption? They may have just assumed that you did not break it and kept using plaintext without knowing? If we do not give a warning when sending over an unencrypted channel in red flashing letters, that is more likely explanation than nobody caring that we saw no breakage reports, no?
On Sun, Apr 11 2021, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > >> So let's just remove it instead of fixing the bug, clearly nobody's >> cared enough to complain. > > Hmph, is that a safe assumption? They may have just assumed that > you did not break it and kept using plaintext without knowing? If > we do not give a warning when sending over an unencrypted channel in > red flashing letters, that is more likely explanation than nobody > caring that we saw no breakage reports, no? Maybe, I think in either case this patch series makes senes. We were already 11 years into a stated deprecation period of that variable, now it's 13. If we're going to e.g. emit some notice about it I think the parsing simplification this series gives us makes sense, we can always add a trivial patch on top to make it die if it sees the old variable. I don't think that's needed, do you?
On Sun, Apr 11 2021, Ævar Arnfjörð Bjarmason wrote: > On Sun, Apr 11 2021, Junio C Hamano wrote: > >> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: >> >>> So let's just remove it instead of fixing the bug, clearly nobody's >>> cared enough to complain. >> >> Hmph, is that a safe assumption? They may have just assumed that >> you did not break it and kept using plaintext without knowing? If >> we do not give a warning when sending over an unencrypted channel in >> red flashing letters, that is more likely explanation than nobody >> caring that we saw no breakage reports, no? > > Maybe, I think in either case this patch series makes senes. We were > already 11 years into a stated deprecation period of that variable, now > it's 13. > > If we're going to e.g. emit some notice about it I think the parsing > simplification this series gives us makes sense, we can always add a > trivial patch on top to make it die if it sees the old variable. > > I don't think that's needed, do you? Junio: *Poke*. Was going thorugh my outstanding patches, I still think it makes sense to just pick this up. Especially with the related discussion later about how common in-the-wild service providers would just not support AUTH non-encrypted, so in practice I think it's even less likely that anyone saw silent breakage as a result of this already-deprecated variable being ignored.
diff --git a/Documentation/config/sendemail.txt b/Documentation/config/sendemail.txt index cbc5af42fdf..50baa5d6bfb 100644 --- a/Documentation/config/sendemail.txt +++ b/Documentation/config/sendemail.txt @@ -8,9 +8,6 @@ sendemail.smtpEncryption:: See linkgit:git-send-email[1] for description. Note that this setting is not subject to the 'identity' mechanism. -sendemail.smtpssl (deprecated):: - Deprecated alias for 'sendemail.smtpEncryption = ssl'. - sendemail.smtpsslcertpath:: Path to ca-certificates (either a directory or a single file). Set it to an empty string to disable certificate verification. diff --git a/git-send-email.perl b/git-send-email.perl index f5bbf1647e3..877c7dd1a21 100755 --- a/git-send-email.perl +++ b/git-send-email.perl @@ -374,11 +374,7 @@ sub read_config { my $enc = Git::config(@repo, $setting); return unless defined $enc; return if $configured->{$setting}++; - if (defined $enc) { - $smtp_encryption = $enc; - } elsif (Git::config_bool(@repo, "$prefix.smtpssl")) { - $smtp_encryption = 'ssl'; - } + $smtp_encryption = $enc; } }
Remove the already dead code to support "sendemail.smtssl" by finally removing the dead code supporting the configuration option. In f6bebd121ac (git-send-email: add support for TLS via Net::SMTP::SSL, 2008-06-25) the --smtp-ssl command-line option was documented as deprecated, later in 65180c66186 (List send-email config options in config.txt., 2009-07-22) the "sendemail.smtpssl" configuration option was also documented as such. Then in in 3ff15040e22 (send-email: fix regression in sendemail.identity parsing, 2019-05-17) I unintentionally removed support for it by introducing a bug in read_config(). As can be seen from the diff context we've already returned unless $enc i defined, so it's not possible for us to reach the "elsif" branch here. This code was therefore already dead since Git v2.23.0. So let's just remove it instead of fixing the bug, clearly nobody's cared enough to complain. The --smtp-ssl option is still deprecated, if someone cares they can follow-up and remove that too, but unlike the config option that one could still be in use in the wild. I'm just removing this code that's provably unused already. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- Documentation/config/sendemail.txt | 3 --- git-send-email.perl | 6 +----- 2 files changed, 1 insertion(+), 8 deletions(-)