Re: [RFC PATCH 0/2] contrib/credential/netrc Makefile & test improvements

[Todd Zullinger <tmz@pobox.com>]

Hi Luis,

Luis Marsano wrote:
> Thanks for looking into this and addressing these issues.

And thank you for digging further. :)

> On Thu, Jun 7, 2018 at 1:20 AM Todd Zullinger <tmz@pobox.com> wrote:
>> I noticed failures from the contrib/credential/netrc tests
>> while building 2.18.0 release candidates.  I was surprised
>> to see the tests being run when called with a simple 'make'
>> command.
>>
>> The first patch in the series adds an empty 'all::' make
>> target to match most of our other Makefiles and avoid the
>> surprise of running tests by default.  (When the netrc
>> helper was added to the fedora builds, it copied the same
>> 'make -C contrib/credential/...' pattern from other
>> credential helpers -- despite the lack of anything to
>> build.)
> 
> I think this is a good idea.
> 
>> The actual test failures were initially due to my build
>> environment lacking the perl autodie module, which was added
>> in 786ef50a23 ("git-credential-netrc: accept gpg option",
>> 2018-05-12).
> 
> I added 'use autodie;' without realizing it had external dependencies.
> According to the documentation
> http://perldoc.perl.org/autodie.html
> it's a pragma since perl 5.10.1
> Removing 'use autodie;' should be fine: it's not critical.

I should clarify that part of why autodie isn't in my build
environment is that the Fedora and RHEL7+ perl packages
split out many modules which are shipped as part of the core
perl tarball.  So while all the platforms I care about have
perl >= 5.10.1, the Fedora and newer RHEL systems have the
autodie module in a separate package.

That said, the INSTALL docs still suggest that we only
require perl >= 5.8, so if autodie is easily removed, that
would probably be a good plan.

Ævar brought up bumping the minimum supported perl to 5.10.0
last December, in <20171223174400.26668-1-avarab@gmail.com>
(https://public-inbox.org/git/20171223174400.26668-1-avarab@gmail.com/).
The general consensus seemed positive, but I don't think
it's happened.  Even so, that was 5.10.0, not the 5.10.1
which added autodie.

>> After installing the autodie module, the failures were due
>> to the build environment lacking a git install (specifically
>> the perl Git module).  The tests needing a pre-installed
>> perl Git seemed odd and worth fixing.
> 
> I mistakenly thought 'use lib (split(/:/, $ENV{GITPERLLIB}));' in
> test.pl handled this.
> Since it doesn't, and I was only following an example from
> t/t9700/test.pl that doesn't fit, this line should be removed and it
> might make more sense to set the environment from
> t-git-credential-netrc.sh near '. ./test-lib.sh', which also sets the
> environment.
> Something like
> 
> diff --git a/contrib/credential/netrc/t-git-credential-netrc.sh
> b/contrib/credential/netrc/t-git-credential-netrc.sh
> index 58191a6..9e18611 100755
> --- a/contrib/credential/netrc/t-git-credential-netrc.sh
> +++ b/contrib/credential/netrc/t-git-credential-netrc.sh
> @@ -23,5 +23,6 @@
>         # The external test will outputs its own plan
>         test_external_has_tap=1
> 
> +       export PERL5LIB="$GITPERLLIB"
>         test_external \
>      'git-credential-netrc' \
> 
> Your solution, however, is reasonable, and I don't know which is preferred.

I think your placement is better.  As you say, it could also
be placed closer to '. ./test-lib.sh'.

It doesn't come up very often, but I wonder if there's any
downside to having test-lib.sh export PERL5LIB?

> It looks like you found an issue with t/t9700/test.pl, too.
> When altered to fail, it first reports ok (then reports failed and
> returns non-0).
> 
> not ok 46 - unquote simple quoted path
> not ok 47 - unquote escape sequences
> 1..47
> # test_external test Perl API was ok
> # test_external_without_stderr test no stderr: Perl API failed: perl
> /home/luism/project/git/t/t9700/test.pl:
> $ echo $?
> 1

Oops.  Nice catch.  At least that does exit non-zero I
guess.

> To make git-credential-netrc tests behave correctly, I ended up making
> the changes below.
> They might be okay, unless someone knows better.
> 
> diff --git a/contrib/credential/netrc/t-git-credential-netrc.sh
> b/contrib/credential/netrc/t-git-credential-netrc.sh
> index 58191a6..9e18611 100755
> --- a/contrib/credential/netrc/t-git-credential-netrc.sh
> +++ b/contrib/credential/netrc/t-git-credential-netrc.sh
> @@ -23,9 +23,10 @@
>         # The external test will outputs its own plan
>         test_external_has_tap=1
> 
> +       export PERL5LIB="$GITPERLLIB"
>         test_external \
>      'git-credential-netrc' \
> -    perl "$TEST_DIRECTORY"/../contrib/credential/netrc/test.pl
> +    perl "$GIT_BUILD_DIR"/contrib/credential/netrc/test.pl
> 
>         test_done
>  )

This reminds me, while we're here it might be worth adding a
whitespace cleanup commit to indent the lines following
test_expect_success and test_external with 2 tabs.

> diff --git a/contrib/credential/netrc/test.pl b/contrib/credential/netrc/test.pl
> index 1e10010..abc9081 100755
> --- a/contrib/credential/netrc/test.pl
> +++ b/contrib/credential/netrc/test.pl
> @@ -1,6 +1,4 @@
>  #!/usr/bin/perl
> -use lib (split(/:/, $ENV{GITPERLLIB}));
> -
>  use warnings;
>  use strict;
>  use Test::More qw(no_plan);
> @@ -12,7 +10,6 @@ BEGIN
>         # t-git-credential-netrc.sh kicks off our testing, so we have to go
>         # from there.
>         Test::More->builder->current_test(1);
> -       Test::More->builder->no_ending(1);
>  }
> 
>  my @global_credential_args = @ARGV;
> @@ -104,6 +101,9 @@ BEGIN
> 
>  ok(scalar keys %$cred == 2, 'Got keys decrypted by command option');
> 
> +my $is_passing = eval { Test::More->is_passing };
> +exit($is_passing ? 0 : 1) unless $@ =~ /Can't locate object method/;
> +
>  sub run_credential
>  {
>         my $args = shift @_;

I tested this and it worked well for my builds with and
without the autodie module (passing and failing
appropriately).

Thanks,

-- 
Todd
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
A common mistake people make when trying to design something
completely foolproof is to underestimate the ingenuity of complete
fools.
    -- Douglas Adams