RFC patch, 2 partsgit-credential-netrc: make "all" default target of Makefile

Hi,

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.)

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).

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.

The second patch in the series aims to fix this. I'm not sure if there's a better or more preferable way to fix this, which is one of the reasons for the RFC tag. (It's also why I added you to the Cc Ævar, as you're one of the knowledgeable perl folks here.)

The other reason for the RFC tag is that I'm unsure of how to fix the last issue I found. The tests exit cleanly even when there are failures, which seems undesirable. I'm not familiar with the perl test_external framework to suggest a fix in patch form. It might be a matter of adding something like this, from t/t9700/test.pl:

    my $is_passing = eval { Test::More->is_passing };
    exit($is_passing ? 0 : 1) unless $@ =~ /Can't locate object method/;

?  But that's a wild guess which I haven't tested.

Here's the output from 'make test' showing that most tests fail and we still get a clean exit status:

$ make -C contrib/credential/netrc test ; echo "netrc test exit status: $?" make: Entering directory '/builddir/build/BUILD/git-2.18.0.rc1/contrib/credential/netrc' ./t-git-credential-netrc.sh ok 1 - set up test repository # run 1: git-credential-netrc (perl /builddir/build/BUILD/git-2.18.0.rc1/t/../contrib/credential/netrc/test.pl) ok 2 - Got 0 keys from insecure file ok 3 - Got 0 keys from missing file not ok 4 - Got first found keys with bad data ok 5 - Got no corovamilkbar keys not ok 6 - Got 2 Github keys not ok 7 - Got correct Github password not ok 8 - Got correct Github username not ok 9 - Got 2 username-specific keys not ok 10 - Got correct user-specific password not ok 11 - Got correct user-specific protocol not ok 12 - Got 2 host:port-specific keys not ok 13 - Got correct host:port-specific password not ok 14 - Got correct host:port-specific username not ok 15 - Got 2 'host:port kills host' keys not ok 16 - Got correct 'host:port kills host' password not ok 17 - Got correct 'host:port kills host' username not ok 18 - Got keys decrypted by git config option not ok 19 - Got keys decrypted by command option # test_external test git-credential-netrc was ok make: Leaving directory '/builddir/build/BUILD/git-2.18.0.rc1/contrib/credential/netrc' netrc test exit status: 0

Todd Zullinger (2):
  git-credential-netrc: make "all" default target of Makefile
  git-credential-netrc: use in-tree Git.pm for tests

 contrib/credential/netrc/Makefile | 3 +++
 contrib/credential/netrc/test.pl  | 3 +++
 2 files changed, 6 insertions(+)

Thanks all.

Running "make" in contrib/credential/netrc should run the "all" target rather than the "test" target. Add an empty "all::" target like most of our other Makefiles.

Signed-off-by: Todd Zullinger <tmz@pobox.com>
---
 contrib/credential/netrc/Makefile | 3 +++
 1 file changed, 3 insertions(+)

The netrc test.pl script calls git-credential-netrc which imports the Git module. Pass GITPERLLIB to git-credential-netrc via PERL5LIB to ensure the in-tree Git module is used for testing.

Signed-off-by: Todd Zullinger <tmz@pobox.com>
---
 contrib/credential/netrc/test.pl | 3 +++
 1 file changed, 3 insertions(+)

Thanks for looking into this and addressing these issues.

On Thu, Jun 7, 2018 at 1:20 AM Todd Zullinger <tmz@pobox.com> wrote:

I think this is a good idea.

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

Hi Luis,

Luis Marsano wrote:

And thank you for digging further. :)

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.

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?

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

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.

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

Thanks,

On Sun, Jun 10 2018, Todd Zullinger wrote:

The intent of those docs was and still is to say "5.8 and the modules it ships with".

This was discussed when 2.17.0 was released with my changes to make us unconditionally use Digest::MD5: https://public-inbox.org/git/87fu50e0ht.fsf@evledraar.gmail.com/

As noted there that's not some dogmatic "RedHat altered perl so we don't care about them", but rather that in practice this doesn't impact users negatively, so I think it's fine.

Right, this doesn't apply to autodie. Looking at https://www.cpan.org/ports/binaries.html there were a lot of releases that had 5.10.0, not *.1.

Also git-credential-netrc is in contrib, I don't know if that warrants treating it differently, I don't use it myself, and don't know how much it's "not really contrib" in practice (e.g. like the bash completion which is installed everywhere...)>

But yeah, skimming the code it would be easy to remove the dependency.

Hi,

Ævar Arnfjörð Bjarmason wrote:

Yeah, that was my understanding. I only included the information on why it was missing from my build environment despite having perl-5.10.1 was due to the Fedora/Red Hat packaging. I agree that any issues which fall out of those packaging differences are on Fedora/Red Hat packagers to fix.

(Which should all be relatively trivial, as the perl modules contain a 'Provides: perl($module)'. That allows dnf|yum install 'perl(autodie)' to easily pull in the right package. And the rpm perl dep generator creates a 'Requires: perl(autodie)' when is sees 'use autodie'.)

Sorry if I muddied the conversation with that tangential info. :)

Indeed, that's a fine question. All the platforms I care about have 5.10.1 or newer, so either way works for me.

I wrapped up the changes from Luis which replace my 2/2 "use in-tree Git.pm for tests" and to ensure the tests exit properly on failures. I'll send those out now in the hope that it saves a little effort in moving these minor fixes forward.

As far as removing the autodie dep, is there anything more to that than dropping the 'use autodie' line? It looks like doing so leaves us no worse than we were before, but I haven't written any perl in a long time.

Thanks,

On Tue, Jun 12, 2018 at 11:08 PM Todd Zullinger <tmz@pobox.com> wrote:

Erasing that line should remove the dependency. I added it as a gratuitous safeguard. I think it's fine to simply remove it. If anyone knows better, of course, I hope they'll say.

The "autodie" module was added in Perl 5.10.1, but our INSTALL document says "version 5.8 or later is needed".

As discussed in <87efhfvxzu.fsf@evledraar.gmail.com> this script is in contrib/, so we might not want to apply that policy, however in this case "autodie" was recently added as a "gratuitous safeguard" in 786ef50a23 ("git-credential-netrc: accept gpg option", 2018-05-12) (see <CAHqJXRE8OKSKcck1APHAHccLZhox+tZi8nNu2RA74RErX8s3Pg@mail.gmail.com>).

Looking at it more carefully the addition of "autodie" inadvertently introduced a logic error, since having it is equivalent to this patch:

    @@ -245,10 +244,10 @@ sub load_netrc {
     	if ($gpgmode) {
     		my @cmd = ($options{'gpg'}, qw(--decrypt), $file);
     		log_verbose("Using GPG to open $file: [@cmd]");
    -		open $io, "-|", @cmd;
    +		open $io, "-|", @cmd or die "@cmd: $!";
     	} else {
     		log_verbose("Opening $file...");
    -		open $io, '<', $file;
    +		open $io, '<', $file or die "$file: $!$!;
     	}

     	# nothing to do if the open failed (we log the error later)

As shown in the context the intent of that code is not do die but to log the error later.

Per my reading of the file this was the only thing autodie was doing in this file (there was no other code it altered). So let's remove it, both to fix the logic error and to get rid of the dependency.

1. <87efhfvxzu.fsf@evledraar.gmail.com>
   (https://public-inbox.org/git/87efhfvxzu.fsf@evledraar.gmail.com/)
2. <CAHqJXRE8OKSKcck1APHAHccLZhox+tZi8nNu2RA74RErX8s3Pg@mail.gmail.com>
   (https://public-inbox.org/git/CAHqJXRE8OKSKcck1APHAHccLZhox+tZi8nNu2RA74RErX8s3Pg@mail.gmail.com/)

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 contrib/credential/netrc/git-credential-netrc | 1 -
 1 file changed, 1 deletion(-)

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

Even though this may not be all that release-critical, let's queue it so that we do not have to remember to dig it up later ;-)

Thank you very much to all of you involved in the thread.

Hi,

Junio C Hamano wrote:

Should I resend the RFC v2 series as well? The only change I'd make would be to add my signoff on the patches which are 'From: Luis' per suggestion.

If you think that's worth adding, I can resend or you may add it while queueing. Whichever is easier for you works for me.

Thanks,

This replaces my 2/2 "use in-tree Git.pm for tests" with Luis's improved version. It also adds Luis's fix to ensure the proper exit status on test failures and a minor whitespace cleanup.

Is it alright to forge your signoff Luis?

Luis Marsano (2):
  git-credential-netrc: use in-tree Git.pm for tests
  git-credential-netrc: fix exit status when tests fail

Todd Zullinger (2):
  git-credential-netrc: make "all" default target of Makefile
  git-credential-netrc: minor whitespace cleanup in test script

 contrib/credential/netrc/Makefile                  | 3 +++
 contrib/credential/netrc/t-git-credential-netrc.sh | 9 +++++----
 contrib/credential/netrc/test.pl                   | 5 +++--
 3 files changed, 11 insertions(+), 6 deletions(-)

On Tue, Jun 12, 2018 at 11:10 PM Todd Zullinger <tmz@pobox.com> wrote:

Thanks, that's fine. You should sign-off on those, too: you identified the problems, told me the solutions, tested them, and deserve the credit. Moreover, Documentation/SubmittingPatches encourages it.

When the patches are ready, I think we'll need the primary author (Ted) to acknowledge before the maintainer (Junio) can approve.

Running "make" in contrib/credential/netrc should run the "all" target rather than the "test" target. Add an empty "all::" target like most of our other Makefiles.

Signed-off-by: Todd Zullinger <tmz@pobox.com>
---
 contrib/credential/netrc/Makefile | 3 +++
 1 file changed, 3 insertions(+)

Signed-off-by: Todd Zullinger <tmz@pobox.com>
---
 contrib/credential/netrc/t-git-credential-netrc.sh | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

On Tue, Jun 12, 2018 at 11:10 PM, Todd Zullinger <tmz@pobox.com> wrote:

Since you're touching all the tests in this script anyhow, perhaps modernize them so the title and opening quote of the test body are on the same line as test_expect_success, and the closing body quote is on a line of its own?

    test_expect_sucess 'setup test repository' '
        ...test body...
    '

I also changed "set up" to "setup" to follow existing practice.

(Not necessarily worth a re-roll.)

Eric Sunshine wrote:

These tests were based on similar test_external tests which use perl. like t0202 & t9700. Both examples use the same formatting (and use of 'set up'). Perhaps a later clean up can adjust all three tests?

On Wed, Jun 13, 2018 at 1:21 PM Todd Zullinger <tmz@pobox.com> wrote:

Whichever course of action works for you and Junio is fine. In this case, it's such a minor bit of additional work to modernize the two tests in this script that it would make sense to do so in this patch if you happen to re-roll (and if you agree with me), but is itself probably not worth a re-roll (as mentioned above).

From: Luis Marsano <luis.marsano@gmail.com>

The netrc test.pl script calls git-credential-netrc which imports the Git module. Pass GITPERLLIB to git-credential-netrc via PERL5LIB to ensure the in-tree Git module is used for testing.

Signed-off-by: Luis Marsano <luis.marsano@gmail.com>
---
 contrib/credential/netrc/t-git-credential-netrc.sh | 3 ++-
 contrib/credential/netrc/test.pl                   | 1 -
 2 files changed, 2 insertions(+), 2 deletions(-)

From: Luis Marsano <luis.marsano@gmail.com>

Signed-off-by: Luis Marsano <luis.marsano@gmail.com>
---
 contrib/credential/netrc/test.pl | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)