[PATCH] send-email: remove documented requirement for Net::SMTP::SSL

[PATCH] send-email: remove documented requirement for Net::SMTP::SSL

[Chris Mayo]

git-send-email uses the TLS support in the Net::SMTP core module from
recent versions of Perl. Documenting the minimum version is complex
because of separate numbering for Perl (5.21.5~169), Net:SMTP (2.34)
and libnet (3.01). Version numbers from commit:
bfbfc9a953 ("send-email: Net::SMTP::starttls was introduced in v2.34",
2017-05-31)

Users of older Perl versions without Net::SMTP::SSL installed will get a
clear error message.

Signed-off-by: Chris Mayo <aklhfex@gmail.com>
---
 Documentation/git-send-email.txt | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt
index 1afe9fc858..a2acdaffa3 100644
--- a/Documentation/git-send-email.txt
+++ b/Documentation/git-send-email.txt
@@ -500,8 +500,8 @@ app-specific or your regular password as appropriate.  If you have credential
 helper configured (see linkgit:git-credential[1]), the password will be saved in
 the credential store so you won't have to type it the next time.
 
-Note: the following perl modules are required
-      Net::SMTP::SSL, MIME::Base64 and Authen::SASL
+Note: the following Perl modules are required
+      MIME::Base64 and Authen::SASL
 
 SEE ALSO
 --------
-- 
2.21.0

Re: [PATCH] send-email: remove documented requirement for Net::SMTP::SSL

[Eric Wong]

Chris Mayo <aklhfex@gmail.com> wrote:
> git-send-email uses the TLS support in the Net::SMTP core module from
> recent versions of Perl. Documenting the minimum version is complex
> because of separate numbering for Perl (5.21.5~169), Net:SMTP (2.34)
> and libnet (3.01). Version numbers from commit:
> bfbfc9a953 ("send-email: Net::SMTP::starttls was introduced in v2.34",
> 2017-05-31)

No disagreement for removing the doc requirement for Net::SMTP::SSL.

But core modules can be split out by OS packagers.  For
Fedora/RH-based systems, the trend tends to be increasing
granularity and having more optional packages.

So I think documenting Net::SMTP (and Net::Domain) as
requirements would still be good, perhaps with a note stating
they're typically installed with Perl.

Fwiw, I recently ran into some issues where core modules such as
Devel::Peek, Encode, and autodie were separate packages on CentOS 7.

Re: [PATCH] send-email: remove documented requirement for Net::SMTP::SSL

[Ævar Arnfjörð Bjarmason]

On Mon, May 27 2019, Eric Wong wrote:

> Chris Mayo <aklhfex@gmail.com> wrote:
>> git-send-email uses the TLS support in the Net::SMTP core module from
>> recent versions of Perl. Documenting the minimum version is complex
>> because of separate numbering for Perl (5.21.5~169), Net:SMTP (2.34)
>> and libnet (3.01). Version numbers from commit:
>> bfbfc9a953 ("send-email: Net::SMTP::starttls was introduced in v2.34",
>> 2017-05-31)
>
> No disagreement for removing the doc requirement for Net::SMTP::SSL.
>
> But core modules can be split out by OS packagers.  For
> Fedora/RH-based systems, the trend tends to be increasing
> granularity and having more optional packages.
>
> So I think documenting Net::SMTP (and Net::Domain) as
> requirements would still be good, perhaps with a note stating
> they're typically installed with Perl.
>
> Fwiw, I recently ran into some issues where core modules such as
> Devel::Peek, Encode, and autodie were separate packages on CentOS 7.

I've done enough git-send-email patching in anger for a year at least
with what's sitting in "next" so I'm not working on this, but just my
0.02:

I wonder if we shouldn't just be much more aggressive about version
requirements for something like git-send-email.

Do we really have git users who want a new git *and* have an old perl
*and* aren't just getting it from an OS package where the module is
dual-life, so the distributor can just package up the newer version if
we were to require it?

I.e. couldn't we just remove the fallback code added in 0ead000c3a
("send-email: Net::SMTP::SSL is obsolete, use only when necessary",
2017-03-24) and do away with this version detection (which b.t.w. should
just do a $obj->can("starttls") check instead).

For shipping a newer Net::SMTP we aren't talking about upgrading
/usr/bin/perl, just that module, and anyone who's packaging git
(e.g. Debian) who cares about minimal dependencies is likely splitting
out git-send-email.perl anyway.

We could then just add some flag similar to NO_PERL_CPAN_FALLBACKS so
we'd error out by default unless these modules were there when git was
built, packagers could then still set some "no I can't be bothered with
send-email at all" or "no I can't be bothered with its SSL support", in
the latter case git-send-email would work except for the SSL parts.

That would take care of the communication about module dependencies via
manpage problem since we'd error by default. When I package things I
much prefer that error mode to "parts of package silently don't work
because we check at runtime and I didn't religiously scour the
docs/release notes".

I wouldn't say the same thing about git-add--interactive.perl due to
more common its use is.

Re: [PATCH] send-email: remove documented requirement for Net::SMTP::SSL

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 2713 bytes --]

On 2019-05-27 at 20:36:36, Ævar Arnfjörð Bjarmason wrote:
> I've done enough git-send-email patching in anger for a year at least
> with what's sitting in "next" so I'm not working on this, but just my
> 0.02:
> 
> I wonder if we shouldn't just be much more aggressive about version
> requirements for something like git-send-email.
> 
> Do we really have git users who want a new git *and* have an old perl
> *and* aren't just getting it from an OS package where the module is
> dual-life, so the distributor can just package up the newer version if
> we were to require it?

In my experience, shipping newer versions of packages shipped with the
OS is a no-no. That's a great way to break unrelated software on the
system, and if you're the distributor, to get users angry at you about
breaking stuff on their systems.

We do indeed have users who want a newer Git on those systems and are
using the system Perl. The Git shipped with CentOS 7 (not to mention
CentOS 6) is positively ancient and doesn't support useful features like
worktrees, so it makes sense to upgrade it. But if you're not a Perl
shop, nobody cares about the version of Perl on the system and fussing
with it doesn't make sense.

> I.e. couldn't we just remove the fallback code added in 0ead000c3a
> ("send-email: Net::SMTP::SSL is obsolete, use only when necessary",
> 2017-03-24) and do away with this version detection (which b.t.w. should
> just do a $obj->can("starttls") check instead).
> 
> For shipping a newer Net::SMTP we aren't talking about upgrading
> /usr/bin/perl, just that module, and anyone who's packaging git
> (e.g. Debian) who cares about minimal dependencies is likely splitting
> out git-send-email.perl anyway.
> 
> We could then just add some flag similar to NO_PERL_CPAN_FALLBACKS so
> we'd error out by default unless these modules were there when git was
> built, packagers could then still set some "no I can't be bothered with
> send-email at all" or "no I can't be bothered with its SSL support", in
> the latter case git-send-email would work except for the SSL parts.

We had a problem with Homebrew sometime back where they stopped shipping
git-send-email because it required Perl modules and there was a big
uproar and a request for us to allow dynamic Perl support. I would like
to discourage distributors from simply refusing to ship core pieces of
Git because it tends to cause problems for users and for us down the
line.

I understand and am fine with splitting components out into multiple
packages or omitting parts interfacing with other systems (e.g.
git-svn).
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]

Re: [PATCH] send-email: remove documented requirement for Net::SMTP::SSL

[Todd Zullinger]

Eric Wong wrote:
> Chris Mayo <aklhfex@gmail.com> wrote:
>> git-send-email uses the TLS support in the Net::SMTP core module from
>> recent versions of Perl. Documenting the minimum version is complex
>> because of separate numbering for Perl (5.21.5~169), Net:SMTP (2.34)
>> and libnet (3.01). Version numbers from commit:
>> bfbfc9a953 ("send-email: Net::SMTP::starttls was introduced in v2.34",
>> 2017-05-31)
> 
> No disagreement for removing the doc requirement for Net::SMTP::SSL.
> 
> But core modules can be split out by OS packagers.  For
> Fedora/RH-based systems, the trend tends to be increasing
> granularity and having more optional packages.
> 
> So I think documenting Net::SMTP (and Net::Domain) as
> requirements would still be good, perhaps with a note stating
> they're typically installed with Perl.

I didn't know that git-send-email.perl could take advantage
of Net::SMTP::starttls until I read this.

[Adding Dennis and Jonathan as the authors of 0ead000c3a
("send-email: Net::SMTP::SSL is obsolete, use only when
necessary", 2017-03-24) bfbfc9a953 ("send-email:
Net::SMTP::starttls was introduced in v2.34", 2017-05-31),
respectively.]

The current Fedora and Red Hat package have a requirement on
Net::SMTP::SSL from long, long ago¹.  As I looked at whether
I could remove that (or more accurately, replace it with
IO::Socket::SSL which is needed for Net::SMTP to handle
starttls), I noticed that on RHEL7 the Net::SMTP version was
2.31, but starttls support has been backported².

I wonder if it's (separately from this change) worth
adjusting the conditional which sets $use_net_smtp_ssl to
use "Net::SMTP->can('starttls')" rather than a strict
version check?  (It might not be if using 'can' is too
fragile or would only benefit the Red Hat 7 packages which
likely won't officially be updated to a newer git with such
a change.)

Something like:

diff --git i/git-send-email.perl w/git-send-email.perl
index 24859a7bc3..84ac03994d 100755
--- i/git-send-email.perl
+++ w/git-send-email.perl
@@ -1465,7 +1465,7 @@ sub send_message {
 		}
 
 		require Net::SMTP;
-		my $use_net_smtp_ssl = version->parse($Net::SMTP::VERSION) < version->parse("2.34");
+		my $use_net_smtp_ssl = Net::SMTP->can('starttls') ? 0 : 1;
 		$smtp_domain ||= maildomain();
 
 		if ($smtp_encryption eq 'ssl') {

¹ https://bugzilla.redhat.com/443615
² https://bugzilla.redhat.com/1557574
  https://git.centos.org/rpms/perl/c/13dfe3?branch=c7

-- 
Todd

Re: [PATCH] send-email: remove documented requirement for Net::SMTP::SSL

[Eric Wong]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
> On Mon, May 27 2019, Eric Wong wrote:
> > Chris Mayo <aklhfex@gmail.com> wrote:
> >> git-send-email uses the TLS support in the Net::SMTP core module from
> >> recent versions of Perl. Documenting the minimum version is complex
> >> because of separate numbering for Perl (5.21.5~169), Net:SMTP (2.34)
> >> and libnet (3.01). Version numbers from commit:
> >> bfbfc9a953 ("send-email: Net::SMTP::starttls was introduced in v2.34",
> >> 2017-05-31)
> >
> > No disagreement for removing the doc requirement for Net::SMTP::SSL.
> >
> > But core modules can be split out by OS packagers.  For
> > Fedora/RH-based systems, the trend tends to be increasing
> > granularity and having more optional packages.
> >
> > So I think documenting Net::SMTP (and Net::Domain) as
> > requirements would still be good, perhaps with a note stating
> > they're typically installed with Perl.
> >
> > Fwiw, I recently ran into some issues where core modules such as
> > Devel::Peek, Encode, and autodie were separate packages on CentOS 7.
> 
> I've done enough git-send-email patching in anger for a year at least
> with what's sitting in "next" so I'm not working on this, but just my
> 0.02:
> 
> I wonder if we shouldn't just be much more aggressive about version
> requirements for something like git-send-email.
> 
> Do we really have git users who want a new git *and* have an old perl
> *and* aren't just getting it from an OS package where the module is
> dual-life, so the distributor can just package up the newer version if
> we were to require it?

I started writing this earlier, but dropped my connection.
And brian said the same thing I was going to say.

If OS packagers were to start making Net::SMTP/Net::Domain
optional (which I sorta expect...), we should make them
optional, too (and add descriptive error messages and manpage
updates).

I have no need for Net::SMTP with sendemail.smtpserver=/usr/bin/msmtp

> I.e. couldn't we just remove the fallback code added in 0ead000c3a
> ("send-email: Net::SMTP::SSL is obsolete, use only when necessary",
> 2017-03-24) and do away with this version detection (which b.t.w. should
> just do a $obj->can("starttls") check instead).

Too soon for the fallback code removal; maybe when CentOS 6 is
EOL.... However, the "->can" check is nicer, yes.

> For shipping a newer Net::SMTP we aren't talking about upgrading
> /usr/bin/perl, just that module, and anyone who's packaging git
> (e.g. Debian) who cares about minimal dependencies is likely splitting
> out git-send-email.perl anyway.
> 
> We could then just add some flag similar to NO_PERL_CPAN_FALLBACKS so
> we'd error out by default unless these modules were there when git was
> built, packagers could then still set some "no I can't be bothered with
> send-email at all" or "no I can't be bothered with its SSL support", in
> the latter case git-send-email would work except for the SSL parts.
> 
> That would take care of the communication about module dependencies via
> manpage problem since we'd error by default. When I package things I
> much prefer that error mode to "parts of package silently don't work
> because we check at runtime and I didn't religiously scour the
> docs/release notes".

I prefer we check options passed to the program and decide
which dependencies to require based on that.  The manpage and
--help could be listing the modules required for certain option
groups, even.

> I wouldn't say the same thing about git-add--interactive.perl due to
> more common its use is.

I wish to see git-send-email (and vendor-neutral messaging in
general) gain more traction.  IMHO, send-email should reach the
commonality of other parts of git.



(*) if/when I find the time, I'd make "git svn find-rev"
    work w/o SVN::Perl, too.

Re: [PATCH] send-email: remove documented requirement for Net::SMTP::SSL

[Eric Wong]

Todd Zullinger <tmz@pobox.com> wrote:
> I wonder if it's (separately from this change) worth
> adjusting the conditional which sets $use_net_smtp_ssl to
> use "Net::SMTP->can('starttls')" rather than a strict
> version check?  (It might not be if using 'can' is too
> fragile or would only benefit the Red Hat 7 packages which
> likely won't officially be updated to a newer git with such
> a change.)
> 
> Something like:
> 
> diff --git i/git-send-email.perl w/git-send-email.perl
> index 24859a7bc3..84ac03994d 100755
> --- i/git-send-email.perl
> +++ w/git-send-email.perl
> @@ -1465,7 +1465,7 @@ sub send_message {
>  		}
>  
>  		require Net::SMTP;
> -		my $use_net_smtp_ssl = version->parse($Net::SMTP::VERSION) < version->parse("2.34");
> +		my $use_net_smtp_ssl = Net::SMTP->can('starttls') ? 0 : 1;
>  		$smtp_domain ||= maildomain();
>  
>  		if ($smtp_encryption eq 'ssl') {

Looks much better to me.

Re: [PATCH] send-email: remove documented requirement for Net::SMTP::SSL

[Ævar Arnfjörð Bjarmason]

On Tue, May 28 2019, Eric Wong wrote:

> Todd Zullinger <tmz@pobox.com> wrote:
>> I wonder if it's (separately from this change) worth
>> adjusting the conditional which sets $use_net_smtp_ssl to
>> use "Net::SMTP->can('starttls')" rather than a strict
>> version check?  (It might not be if using 'can' is too
>> fragile or would only benefit the Red Hat 7 packages which
>> likely won't officially be updated to a newer git with such
>> a change.)
>>
>> Something like:
>>
>> diff --git i/git-send-email.perl w/git-send-email.perl
>> index 24859a7bc3..84ac03994d 100755
>> --- i/git-send-email.perl
>> +++ w/git-send-email.perl
>> @@ -1465,7 +1465,7 @@ sub send_message {
>>  		}
>>
>>  		require Net::SMTP;
>> -		my $use_net_smtp_ssl = version->parse($Net::SMTP::VERSION) < version->parse("2.34");
>> +		my $use_net_smtp_ssl = Net::SMTP->can('starttls') ? 0 : 1;
>>  		$smtp_domain ||= maildomain();
>>
>>  		if ($smtp_encryption eq 'ssl') {
>
> Looks much better to me.

Same, but to bikeshed a bit, at this point we can just do:
    
    diff --git a/git-send-email.perl b/git-send-email.perl
    index 24859a7bc3..4ad2091a49 100755
    --- a/git-send-email.perl
    +++ b/git-send-email.perl
    @@ -1468 +1467,0 @@ sub send_message {
    -               my $use_net_smtp_ssl = version->parse($Net::SMTP::VERSION) < version->parse("2.34");
    @@ -1485 +1484 @@ sub send_message {
    -                       if ($use_net_smtp_ssl) {
    +                       if (Net::SMTP->can('starttls')) {
    @@ -1507 +1506 @@ sub send_message {
    -                               if ($use_net_smtp_ssl) {
    +                               if (Net::SMTP->can('starttls')) {

Re: [PATCH] send-email: remove documented requirement for Net::SMTP::SSL

[Todd Zullinger]

Hi,

Sorry I missed your earlier reply which also mentioned using
$obj->can() Ævar.  That's what I get for typing a reply and
then walking away for a few hours before hitting send. ;)

Ævar Arnfjörð Bjarmason wrote:
> Same, but to bikeshed a bit, at this point we can just do:
>     
>     diff --git a/git-send-email.perl b/git-send-email.perl
>     index 24859a7bc3..4ad2091a49 100755
>     --- a/git-send-email.perl
>     +++ b/git-send-email.perl
>     @@ -1468 +1467,0 @@ sub send_message {
>     -               my $use_net_smtp_ssl = version->parse($Net::SMTP::VERSION) < version->parse("2.34");
>     @@ -1485 +1484 @@ sub send_message {
>     -                       if ($use_net_smtp_ssl) {
>     +                       if (Net::SMTP->can('starttls')) {
>     @@ -1507 +1506 @@ sub send_message {
>     -                               if ($use_net_smtp_ssl) {
>     +                               if (Net::SMTP->can('starttls')) {
> 

I think we'd need to use 'if ! ...' there, or more likely,
switch the blocks which follow because the code following
'if ($use_net_smtp_ssl)' is for Net::SMTP::SSL with the
'else' block handling the case where Net::SMTP has ssl/tls
support.  Right?

I know I read the $use_net_smtp_ssl bit backwards the first
time or two as well.

-- 
Todd

Re: [PATCH] send-email: remove documented requirement for Net::SMTP::SSL

[Eric Wong]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
> On Tue, May 28 2019, Eric Wong wrote:
> > Todd Zullinger <tmz@pobox.com> wrote:
> >> Something like:
> >>
> >> diff --git i/git-send-email.perl w/git-send-email.perl
> >> index 24859a7bc3..84ac03994d 100755
> >> --- i/git-send-email.perl
> >> +++ w/git-send-email.perl
> >> @@ -1465,7 +1465,7 @@ sub send_message {
> >>  		}
> >>
> >>  		require Net::SMTP;
> >> -		my $use_net_smtp_ssl = version->parse($Net::SMTP::VERSION) < version->parse("2.34");
> >> +		my $use_net_smtp_ssl = Net::SMTP->can('starttls') ? 0 : 1;
> >>  		$smtp_domain ||= maildomain();
> >>
> >>  		if ($smtp_encryption eq 'ssl') {
> >
> > Looks much better to me.
> 
> Same, but to bikeshed a bit, at this point we can just do:
>     
>     diff --git a/git-send-email.perl b/git-send-email.perl
>     index 24859a7bc3..4ad2091a49 100755
>     --- a/git-send-email.perl
>     +++ b/git-send-email.perl
>     @@ -1468 +1467,0 @@ sub send_message {
>     -               my $use_net_smtp_ssl = version->parse($Net::SMTP::VERSION) < version->parse("2.34");
>     @@ -1485 +1484 @@ sub send_message {
>     -                       if ($use_net_smtp_ssl) {
>     +                       if (Net::SMTP->can('starttls')) {
>     @@ -1507 +1506 @@ sub send_message {
>     -                               if ($use_net_smtp_ssl) {
>     +                               if (Net::SMTP->can('starttls')) {

Higher probability of a typo slipping through the cracks, though :)

(I was just bit by a similar case with hashes that would've been
 caught by "use fields")

Re: [PATCH] send-email: remove documented requirement for Net::SMTP::SSL

[Ævar Arnfjörð Bjarmason]

On Tue, May 28 2019, brian m. carlson wrote:

> On 2019-05-27 at 20:36:36, Ævar Arnfjörð Bjarmason wrote:
>> I've done enough git-send-email patching in anger for a year at least
>> with what's sitting in "next" so I'm not working on this, but just my
>> 0.02:
>>
>> I wonder if we shouldn't just be much more aggressive about version
>> requirements for something like git-send-email.
>>
>> Do we really have git users who want a new git *and* have an old perl
>> *and* aren't just getting it from an OS package where the module is
>> dual-life, so the distributor can just package up the newer version if
>> we were to require it?
>
> In my experience, shipping newer versions of packages shipped with the
> OS is a no-no. That's a great way to break unrelated software on the
> system, and if you're the distributor, to get users angry at you about
> breaking stuff on their systems.

Just because a package X needs dependency Y at version Z doesn't mean
you need to package it up in such a way that everything that needs any
version of Y must use it at version Z, you put the two versions in
different places on the FS.

So in this case the packager would grumble a bit and package up a
Net::SMTP installed at another path. I do that myself to get some more
modern versions of dependencies on CentOS 6 without breaking the base
system.

Does that mean we should do it in this case? No, and given the feedback
in this thread about this in particular I think we should just keep the
old code.

But I think is important to clarify in general, i.e. just because you're
on CentOS 6 and don't want to update the main perl-Net-SMTP you can
still package up a for-git-perl-Net-SMTP or whatever.

We had a similar discussion about whether to depend on a more recent
libcurl in the past:

https://public-inbox.org/git/CACBZZX78oKU5HuBEqb9qLy7--wcwhC_mW6x7Q+tB4suxohSCsQ@mail.gmail.com/

> We do indeed have users who want a newer Git on those systems and are
> using the system Perl. The Git shipped with CentOS 7 (not to mention
> CentOS 6) is positively ancient and doesn't support useful features like
> worktrees, so it makes sense to upgrade it. But if you're not a Perl
> shop, nobody cares about the version of Perl on the system and fussing
> with it doesn't make sense.

What I was suggesting was whether it was worth it to ask distributors to
drop in a few *.pm files, and perhaps a *.so or two compiled against
openssl, they wouldn't need to change the base /usr/bin/perl.

In the case of the *.pm stuff we could drop fallbacks into
perl/Git/LoadCPAN if we wanted things to work out of the box.

>> I.e. couldn't we just remove the fallback code added in 0ead000c3a
>> ("send-email: Net::SMTP::SSL is obsolete, use only when necessary",
>> 2017-03-24) and do away with this version detection (which b.t.w. should
>> just do a $obj->can("starttls") check instead).
>>
>> For shipping a newer Net::SMTP we aren't talking about upgrading
>> /usr/bin/perl, just that module, and anyone who's packaging git
>> (e.g. Debian) who cares about minimal dependencies is likely splitting
>> out git-send-email.perl anyway.
>>
>> We could then just add some flag similar to NO_PERL_CPAN_FALLBACKS so
>> we'd error out by default unless these modules were there when git was
>> built, packagers could then still set some "no I can't be bothered with
>> send-email at all" or "no I can't be bothered with its SSL support", in
>> the latter case git-send-email would work except for the SSL parts.
>
> We had a problem with Homebrew sometime back where they stopped shipping
> git-send-email because it required Perl modules and there was a big
> uproar and a request for us to allow dynamic Perl support. I would like
> to discourage distributors from simply refusing to ship core pieces of
> Git because it tends to cause problems for users and for us down the
> line.

I'm sympathetic to packagers that do that, particularly when "add -i"
and friends in s/Perl/C/ land, at which point we'd be asking some
distributors who otherwise wouldn't have perl at all to install it just
for us.

It seems packagers mainly had issues with the Perl-SSL stuff. We already
support SSL for https via libcurl usually, perhaps we'd be better off
having some wrapper using that
(https://curl.haxx.se/libcurl/c/smtp-tls.html), or having
git-send-email.perl support something simpler like say stunnel.

> I understand and am fine with splitting components out into multiple
> packages or omitting parts interfacing with other systems (e.g.
> git-svn).

I tend to think of SVN as a system more closely related to Git than Git
is related to E-Mail :)

It's also something that's by definition part of an "egress" workflow,
so it tends to be much easier to work around it being missing than say
"add -i" or something you truly need locally not being there.