diff mbox series

[1/2] send-email: remove non-working support for "sendemail.smtpssl"

Message ID patch-1.2-ee041188e55-20210411T144128Z-avarab@gmail.com (mailing list archive)
State New
Headers show
Series send-email: simplify smtp.{smtpssl,smtpencryption} parsing | expand

Commit Message

Ævar Arnfjörð Bjarmason April 11, 2021, 2:43 p.m. UTC
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(-)

Comments

Junio C Hamano April 11, 2021, 7:08 p.m. UTC | #1
Æ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?
Ævar Arnfjörð Bjarmason April 11, 2021, 7:51 p.m. UTC | #2
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?
Ævar Arnfjörð Bjarmason May 1, 2021, 9:15 a.m. UTC | #3
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 mbox series

Patch

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;
 	}
 }