t9700-perl-git.sh is broken on some configurations

t9700-perl-git.sh is broken on some configurations

[Christian Couder]

Hi,

It looks like t9700-perl-git.sh is broken on one machine I use but not
on my laptop since commit d53c2c67380f769f91fd45cc8c63a5883245ccca
(mingw: fix t9700's assumption about directory separators, Jan 27
17:19:56 2016).

I get:

------------------------
Initialized empty Git repository in /home/ccouder/git/git/t/trash
directory.t9700-perl-git/.git/
expecting success: echo "test file 1" > file1 &&
     echo "test file 2" > file2 &&
     mkdir directory1 &&
     echo "in directory1" >> directory1/file &&
     mkdir directory2 &&
     echo "in directory2" >> directory2/file &&
     git add . &&
     git commit -m "first commit" &&

     echo "new file in subdir 2" > directory2/file2 &&
     git add . &&
     git commit -m "commit in directory2" &&

     echo "changed file 1" > file1 &&
     git commit -a -m "second commit" &&

     git config --add color.test.slot1 green &&
     git config --add test.string value &&
     git config --add test.dupstring value1 &&
     git config --add test.dupstring value2 &&
     git config --add test.booltrue true &&
     git config --add test.boolfalse no &&
     git config --add test.boolother other &&
     git config --add test.int 2k &&
     git config --add test.path "~/foo" &&
     git config --add test.pathexpanded "$HOME/foo" &&
     git config --add test.pathmulti foo &&
     git config --add test.pathmulti bar

[master (root-commit) fc41470] first commit
 Author: A U Thor <author@example.com>
 4 files changed, 4 insertions(+)
 create mode 100644 directory1/file
 create mode 100644 directory2/file
 create mode 100644 file1
 create mode 100644 file2
[master 6a30dee] commit in directory2
 Author: A U Thor <author@example.com>
 1 file changed, 1 insertion(+)
 create mode 100644 directory2/file2
[master 33414b1] second commit
 Author: A U Thor <author@example.com>
 1 file changed, 1 insertion(+), 1 deletion(-)
ok 1 - set up test repository

# run 1: Perl API (perl /home/ccouder/git/git/t/t9700/test.pl)
ok 2 - use Git;
# test_external test Perl API failed: perl /home/ccouder/git/git/t/t9700/test.pl
# expecting no stderr from previous command
# test_external_without_stderr test no stderr: Perl API failed: perl
/home/ccouder/git/git/t/t9700/test.pl:
# Stderr is:
Bareword found where operator expected at
/home/ccouder/git/git/t/t9700/test.pl line 36, near "s/\\/\//gr"
syntax error at /home/ccouder/git/git/t/t9700/test.pl line 36, near "s/\\/\//gr"
Execution of /home/ccouder/git/git/t/t9700/test.pl aborted due to
compilation errors.
------------------------

Indeed on the command line I get:

------------------------
$ t/t9700/test.pl
ok 2 - use Git;
Bareword found where operator expected at t/t9700/test.pl line 36,
near "s/\\/\//gr"
syntax error at t/t9700/test.pl line 36, near "s/\\/\//gr"
Execution of t/t9700/test.pl aborted due to compilation errors.
------------------------

A quick look at t/t9700/test.pl line 36 was not enough for me to spot
the problem.

Perl version is: perl 5, version 18, subversion 2 (v5.18.2) built for
x86_64-linux

The machine is running CentOS 6.5.

Thanks,
Christian.

Re: t9700-perl-git.sh is broken on some configurations

[Jeff King]

On Fri, Mar 04, 2016 at 09:13:51AM +0100, Christian Couder wrote:

> Indeed on the command line I get:
> 
> ------------------------
> $ t/t9700/test.pl
> ok 2 - use Git;
> Bareword found where operator expected at t/t9700/test.pl line 36,
> near "s/\\/\//gr"
> syntax error at t/t9700/test.pl line 36, near "s/\\/\//gr"
> Execution of t/t9700/test.pl aborted due to compilation errors.
> ------------------------
> 
> A quick look at t/t9700/test.pl line 36 was not enough for me to spot
> the problem.
> 
> Perl version is: perl 5, version 18, subversion 2 (v5.18.2) built for
> x86_64-linux
> 
> The machine is running CentOS 6.5.

I can't reproduce on any of the machines I have handy (perl 5.14, 5.20,
and 5.22). I don't have 5.18 handy. The line in question looks fine to
me, so perhaps it is a temporary regression in 5.18.

Does it help to wrap it in parentheses, like:

diff --git a/t/t9700/test.pl b/t/t9700/test.pl
index 7e8c40b..edeeb0e 100755
--- a/t/t9700/test.pl
+++ b/t/t9700/test.pl
@@ -33,7 +33,7 @@ is($r->config_int("test.int"), 2048, "config_int: integer");
 is($r->config_int("test.nonexistent"), undef, "config_int: nonexistent");
 ok($r->config_bool("test.booltrue"), "config_bool: true");
 ok(!$r->config_bool("test.boolfalse"), "config_bool: false");
-is($r->config_path("test.path") =~ s/\\/\//gr, $r->config("test.pathexpanded"),
+is(($r->config_path("test.path") =~ s/\\/\//gr), $r->config("test.pathexpanded"),
    "config_path: ~/foo expansion");
 is_deeply([$r->config_path("test.pathmulti")], ["foo", "bar"],
    "config_path: multiple values");

or even write it out longhand without "/r":

diff --git a/t/t9700/test.pl b/t/t9700/test.pl
index 7e8c40b..52471cf 100755
--- a/t/t9700/test.pl
+++ b/t/t9700/test.pl
@@ -33,7 +33,9 @@ is($r->config_int("test.int"), 2048, "config_int: integer");
 is($r->config_int("test.nonexistent"), undef, "config_int: nonexistent");
 ok($r->config_bool("test.booltrue"), "config_bool: true");
 ok(!$r->config_bool("test.boolfalse"), "config_bool: false");
-is($r->config_path("test.path") =~ s/\\/\//gr, $r->config("test.pathexpanded"),
+my $test_path = $r->config_path("test.path");
+$test_path =~ s/\\/\//g;
+is($test_path, $r->config("test.pathexpanded"),
    "config_path: ~/foo expansion");
 is_deeply([$r->config_path("test.pathmulti")], ["foo", "bar"],
    "config_path: multiple values");

? Those are just guesses, but if we are tickling a bug in perl's parser,
this might avoid them. I also wondered when "/r" appeared. It was in
5.14, so you're presumably good there. The "use" statement at the top of
the script says "5.008", so perhaps we should be writing it out longhand
anyway (that version is "only" 5 years old, so I suspect there are still
systems around with 5.12 or older).

-Peff

Re: t9700-perl-git.sh is broken on some configurations

[Christian Couder]

On Fri, Mar 4, 2016 at 9:56 AM, Jeff King <peff@peff.net> wrote:
> On Fri, Mar 04, 2016 at 09:13:51AM +0100, Christian Couder wrote:
>
>> Indeed on the command line I get:
>>
>> ------------------------
>> $ t/t9700/test.pl
>> ok 2 - use Git;
>> Bareword found where operator expected at t/t9700/test.pl line 36,
>> near "s/\\/\//gr"
>> syntax error at t/t9700/test.pl line 36, near "s/\\/\//gr"
>> Execution of t/t9700/test.pl aborted due to compilation errors.
>> ------------------------
>>
>> A quick look at t/t9700/test.pl line 36 was not enough for me to spot
>> the problem.
>>
>> Perl version is: perl 5, version 18, subversion 2 (v5.18.2) built for
>> x86_64-linux
>>
>> The machine is running CentOS 6.5.
>
> I can't reproduce on any of the machines I have handy (perl 5.14, 5.20,
> and 5.22). I don't have 5.18 handy. The line in question looks fine to
> me, so perhaps it is a temporary regression in 5.18.

It is strange because on the same machine there is also v5.10.1
installed and I get the same error with it.

> Does it help to wrap it in parentheses, like:
>
> diff --git a/t/t9700/test.pl b/t/t9700/test.pl
> index 7e8c40b..edeeb0e 100755
> --- a/t/t9700/test.pl
> +++ b/t/t9700/test.pl
> @@ -33,7 +33,7 @@ is($r->config_int("test.int"), 2048, "config_int: integer");
>  is($r->config_int("test.nonexistent"), undef, "config_int: nonexistent");
>  ok($r->config_bool("test.booltrue"), "config_bool: true");
>  ok(!$r->config_bool("test.boolfalse"), "config_bool: false");
> -is($r->config_path("test.path") =~ s/\\/\//gr, $r->config("test.pathexpanded"),
> +is(($r->config_path("test.path") =~ s/\\/\//gr), $r->config("test.pathexpanded"),
>     "config_path: ~/foo expansion");
>  is_deeply([$r->config_path("test.pathmulti")], ["foo", "bar"],
>     "config_path: multiple values");

No, parentheses don't help.

> or even write it out longhand without "/r":
>
> diff --git a/t/t9700/test.pl b/t/t9700/test.pl
> index 7e8c40b..52471cf 100755
> --- a/t/t9700/test.pl
> +++ b/t/t9700/test.pl
> @@ -33,7 +33,9 @@ is($r->config_int("test.int"), 2048, "config_int: integer");
>  is($r->config_int("test.nonexistent"), undef, "config_int: nonexistent");
>  ok($r->config_bool("test.booltrue"), "config_bool: true");
>  ok(!$r->config_bool("test.boolfalse"), "config_bool: false");
> -is($r->config_path("test.path") =~ s/\\/\//gr, $r->config("test.pathexpanded"),
> +my $test_path = $r->config_path("test.path");
> +$test_path =~ s/\\/\//g;
> +is($test_path, $r->config("test.pathexpanded"),
>     "config_path: ~/foo expansion");
>  is_deeply([$r->config_path("test.pathmulti")], ["foo", "bar"],
>     "config_path: multiple values");
>
> ?

Yeah, it works like the above with both perl versions.

> Those are just guesses, but if we are tickling a bug in perl's parser,
> this might avoid them. I also wondered when "/r" appeared. It was in
> 5.14, so you're presumably good there.

If I just remove the "r" at the end of "s/\\/\//gr", I get with both
Perl versions:

Can't modify non-lvalue subroutine call at t/t9700/test.pl line 36.

> The "use" statement at the top of
> the script says "5.008", so perhaps we should be writing it out longhand
> anyway (that version is "only" 5 years old, so I suspect there are still
> systems around with 5.12 or older).

Yeah, it would work.

Thanks,
Christian.

Re: t9700-perl-git.sh is broken on some configurations

[Jeff King]

On Fri, Mar 04, 2016 at 11:30:36AM +0100, Christian Couder wrote:

> > Those are just guesses, but if we are tickling a bug in perl's parser,
> > this might avoid them. I also wondered when "/r" appeared. It was in
> > 5.14, so you're presumably good there.
> 
> If I just remove the "r" at the end of "s/\\/\//gr", I get with both
> Perl versions:
> 
> Can't modify non-lvalue subroutine call at t/t9700/test.pl line 36.

Right, because the string being operated on is the return value of a
function, so we can't do substitution on it (unless with "r", whose
purpose is to allow exactly such a thing).

> > The "use" statement at the top of
> > the script says "5.008", so perhaps we should be writing it out longhand
> > anyway (that version is "only" 5 years old, so I suspect there are still
> > systems around with 5.12 or older).
> 
> Yeah, it would work.

Thanks for confirming the longhand does work; I think the patch I just
posted elsewhere in the thread should be good for you, then.

-Peff

Re: t9700-perl-git.sh is broken on some configurations

[Dennis Kaarsemaker]

On vr, 2016-03-04 at 03:56 -0500, Jeff King wrote:
> ? Those are just guesses, but if we are tickling a bug in perl's parser,
> this might avoid them. I also wondered when "/r" appeared. It was in
> 5.14, so you're presumably good there. The "use" statement at the top of
> the script says "5.008", so perhaps we should be writing it out longhand
> anyway (that version is "only" 5 years old, so I suspect there are still
> systems around with 5.12 or older).

Knowing the system Christian is testing on, I think the problem is that
the tests are actually being run against perl 5.10, which RHEL 6 ships
as system perl. As that's still a supported OS, writing tests in a form
compatible with it would be a good thing :)

-- 
Dennis Kaarsemaker
http://www.kaarsemaker.net

[PATCH] t9700: fix test for perl older than 5.14

[Jeff King]

On Fri, Mar 04, 2016 at 11:58:24AM +0100, Dennis Kaarsemaker wrote:

> On vr, 2016-03-04 at 03:56 -0500, Jeff King wrote:
> > ? Those are just guesses, but if we are tickling a bug in perl's parser,
> > this might avoid them. I also wondered when "/r" appeared. It was in
> > 5.14, so you're presumably good there. The "use" statement at the top of
> > the script says "5.008", so perhaps we should be writing it out longhand
> > anyway (that version is "only" 5 years old, so I suspect there are still
> > systems around with 5.12 or older).
> 
> Knowing the system Christian is testing on, I think the problem is that
> the tests are actually being run against perl 5.10, which RHEL 6 ships
> as system perl. As that's still a supported OS, writing tests in a form
> compatible with it would be a good thing :)

That would make sense. `perl` in t9700-perl-git.sh (and all of our
scripts) is actually a shell function:

  perl () {
          command "$PERL_PATH" "$@"
  }

to make sure we respect PERL_PATH everywhere. And that defaults in the
Makefile to /usr/bin/perl. Christian presumably has 5.14 in his $PATH,
but /usr/bin/perl is the system 5.10.

One workaround would therefore be for him to tweak PERL_PATH, but
obviously that does not help anyone else. I think we should do this:

-- >8 --
Subject: t9700: fix test for perl older than 5.14

Commit d53c2c6 (mingw: fix t9700's assumption about
directory separators, 2016-01-27) uses perl's "/r" regex
modifier to do a non-destructive replacement on a string,
leaving the original unmodified and returning the result.

This feature was introduced in perl 5.14, but systems with
older perl are still common (e.g., CentOS 6.5 still has perl
5.10). Let's work around it by providing a helper function
that does the same thing using older syntax.

While we're at it, let's switch to using an alternate regex
separator, which is slightly more readable.

Reported-by: Christian Couder <christian.couder@gmail.com>
Helped-by: Dennis Kaarsemaker <dennis@kaarsemaker.net>
Signed-off-by: Jeff King <peff@peff.net>
---
And yes, before anyone checks, the alternate separators are available
even in ancient versions of perl. :)

 t/t9700/test.pl | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/t/t9700/test.pl b/t/t9700/test.pl
index 7e8c40b..1b75c91 100755
--- a/t/t9700/test.pl
+++ b/t/t9700/test.pl
@@ -17,6 +17,12 @@ BEGIN {
 use Cwd;
 use File::Basename;
 
+sub adjust_dirsep {
+	my $path = shift;
+	$path =~ s{\\}{/}g;
+	return $path;
+}
+
 BEGIN { use_ok('Git') }
 
 # set up
@@ -33,7 +39,7 @@ is($r->config_int("test.int"), 2048, "config_int: integer");
 is($r->config_int("test.nonexistent"), undef, "config_int: nonexistent");
 ok($r->config_bool("test.booltrue"), "config_bool: true");
 ok(!$r->config_bool("test.boolfalse"), "config_bool: false");
-is($r->config_path("test.path") =~ s/\\/\//gr, $r->config("test.pathexpanded"),
+is(adjust_dirsep($r->config_path("test.path")), $r->config("test.pathexpanded"),
    "config_path: ~/foo expansion");
 is_deeply([$r->config_path("test.pathmulti")], ["foo", "bar"],
    "config_path: multiple values");
-- 
2.8.0.rc0.309.g6677de9

Re: [PATCH] t9700: fix test for perl older than 5.14

[Dennis Kaarsemaker]

On vr, 2016-03-04 at 06:43 -0500, Jeff King wrote:
> On Fri, Mar 04, 2016 at 11:58:24AM +0100, Dennis Kaarsemaker wrote:
> 
> > On vr, 2016-03-04 at 03:56 -0500, Jeff King wrote:
> > > ? Those are just guesses, but if we are tickling a bug in perl's parser,
> > > this might avoid them. I also wondered when "/r" appeared. It was in
> > > 5.14, so you're presumably good there. The "use" statement at the top of
> > > the script says "5.008", so perhaps we should be writing it out longhand
> > > anyway (that version is "only" 5 years old, so I suspect there are still
> > > systems around with 5.12 or older).
> > 
> > Knowing the system Christian is testing on, I think the problem is that
> > the tests are actually being run against perl 5.10, which RHEL 6 ships
> > as system perl. As that's still a supported OS, writing tests in a form
> > compatible with it would be a good thing :)
> 
> That would make sense. `perl` in t9700-perl-git.sh (and all of our
> scripts) is actually a shell function:
> 
>   perl () {
>           command "$PERL_PATH" "$@"
>   }
> 
> to make sure we respect PERL_PATH everywhere. And that defaults in the
> Makefile to /usr/bin/perl. Christian presumably has 5.14 in his $PATH,
> but /usr/bin/perl is the system 5.10.

Yeah, that's how our systems are set up.

> One workaround would therefore be for him to tweak PERL_PATH, but
> obviously that does not help anyone else. I think we should do this:

Tested against 5.10 and 5.18 and works with both. I also tested the /r
variant with 5.18 and that works as expected.

-- 
Dennis Kaarsemaker
http://www.kaarsemaker.net

Re: [PATCH] t9700: fix test for perl older than 5.14

[Christian Couder]

On Fri, Mar 4, 2016 at 1:21 PM, Dennis Kaarsemaker
<dennis@kaarsemaker.net> wrote:
> On vr, 2016-03-04 at 06:43 -0500, Jeff King wrote:
>> On Fri, Mar 04, 2016 at 11:58:24AM +0100, Dennis Kaarsemaker wrote:
>>
>> > On vr, 2016-03-04 at 03:56 -0500, Jeff King wrote:
>> > > ? Those are just guesses, but if we are tickling a bug in perl's parser,
>> > > this might avoid them. I also wondered when "/r" appeared. It was in
>> > > 5.14, so you're presumably good there. The "use" statement at the top of
>> > > the script says "5.008", so perhaps we should be writing it out longhand
>> > > anyway (that version is "only" 5 years old, so I suspect there are still
>> > > systems around with 5.12 or older).
>> >
>> > Knowing the system Christian is testing on, I think the problem is that
>> > the tests are actually being run against perl 5.10, which RHEL 6 ships
>> > as system perl. As that's still a supported OS, writing tests in a form
>> > compatible with it would be a good thing :)
>>
>> That would make sense. `perl` in t9700-perl-git.sh (and all of our
>> scripts) is actually a shell function:
>>
>>   perl () {
>>           command "$PERL_PATH" "$@"
>>   }
>>
>> to make sure we respect PERL_PATH everywhere. And that defaults in the
>> Makefile to /usr/bin/perl. Christian presumably has 5.14 in his $PATH,
>> but /usr/bin/perl is the system 5.10.
>
> Yeah, that's how our systems are set up.

Yeah, you are both right.

Thanks for debugging this,
Christian.

Re: [PATCH] t9700: fix test for perl older than 5.14

[Johannes Schindelin]

Hi Peff,

On Fri, 4 Mar 2016, Jeff King wrote:

> Subject: t9700: fix test for perl older than 5.14
> 
> Commit d53c2c6 (mingw: fix t9700's assumption about
> directory separators, 2016-01-27) uses perl's "/r" regex
> modifier to do a non-destructive replacement on a string,
> leaving the original unmodified and returning the result.
> 
> This feature was introduced in perl 5.14, but systems with
> older perl are still common (e.g., CentOS 6.5 still has perl
> 5.10). Let's work around it by providing a helper function
> that does the same thing using older syntax.
> 
> While we're at it, let's switch to using an alternate regex
> separator, which is slightly more readable.

My apologies! And thanks for cleaning up after me.

Ciao,
Dscho

Re: [PATCH] t9700: fix test for perl older than 5.14

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> One workaround would therefore be for him to tweak PERL_PATH, but
> obviously that does not help anyone else. I think we should do this:
>
> -- >8 --
> Subject: t9700: fix test for perl older than 5.14
>
> Commit d53c2c6 (mingw: fix t9700's assumption about
> directory separators, 2016-01-27) uses perl's "/r" regex
> modifier to do a non-destructive replacement on a string,
> leaving the original unmodified and returning the result.

Will apply on js/mingw-tests and merge to 'master' before -rc1.
Thanks.

>
> This feature was introduced in perl 5.14, but systems with
> older perl are still common (e.g., CentOS 6.5 still has perl
> 5.10). Let's work around it by providing a helper function
> that does the same thing using older syntax.
>
> While we're at it, let's switch to using an alternate regex
> separator, which is slightly more readable.
>
> Reported-by: Christian Couder <christian.couder@gmail.com>
> Helped-by: Dennis Kaarsemaker <dennis@kaarsemaker.net>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> And yes, before anyone checks, the alternate separators are available
> even in ancient versions of perl. :)
>
>  t/t9700/test.pl | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/t/t9700/test.pl b/t/t9700/test.pl
> index 7e8c40b..1b75c91 100755
> --- a/t/t9700/test.pl
> +++ b/t/t9700/test.pl
> @@ -17,6 +17,12 @@ BEGIN {
>  use Cwd;
>  use File::Basename;
>  
> +sub adjust_dirsep {
> +	my $path = shift;
> +	$path =~ s{\\}{/}g;
> +	return $path;
> +}
> +
>  BEGIN { use_ok('Git') }
>  
>  # set up
> @@ -33,7 +39,7 @@ is($r->config_int("test.int"), 2048, "config_int: integer");
>  is($r->config_int("test.nonexistent"), undef, "config_int: nonexistent");
>  ok($r->config_bool("test.booltrue"), "config_bool: true");
>  ok(!$r->config_bool("test.boolfalse"), "config_bool: false");
> -is($r->config_path("test.path") =~ s/\\/\//gr, $r->config("test.pathexpanded"),
> +is(adjust_dirsep($r->config_path("test.path")), $r->config("test.pathexpanded"),
>     "config_path: ~/foo expansion");
>  is_deeply([$r->config_path("test.pathmulti")], ["foo", "bar"],
>     "config_path: multiple values");