Skip to contentSkip to conversation

Avoid broken Solaris tr

PATCH In review Ben Walton 5 participants 24 messages Jun 15, 2016

lore
Share & tools
lore

Ben Walton

author
#0 lore raw

PATCH 1/1 Avoid broken Solaris tr

Solaris' tr (both /usr/bin/ and /usr/xpg4/bin) fail to handle the case where the first argument is a multi-character set and the second is a single null character. Use perl to perform these substitutions instead. Now that we're using perl for the transliteration, we might as well replace the sed invocations with it too.

We make this change globally in t0008-ignores instead of just for the cases where it matters in order to maintain consistency.

Signed-off-by Ben Walton <[email protected]>
---
 t/t0008-ignores.sh | 26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)
t/t0008-ignores.sh +14 -12
index a56db80..9e4987e 100755
@@ -552,12 +552,13 @@ cat <<-EOF >expected-verbose
 	$global_excludes:2:!globaltwo	b/globaltwo
 EOF
 
-sed -e 's/^"//' -e 's/\\//' -e 's/"$//' stdin | \
-	tr "\n" "\0" >stdin0
-sed -e 's/^"//' -e 's/\\//' -e 's/"$//' expected-default | \
-	tr "\n" "\0" >expected-default0
-sed -e 's/	"/	/' -e 's/\\//' -e 's/"$//' expected-verbose | \
-	tr ":\t\n" "\0" >expected-verbose0
+perl -pne 's/^"//; s/\\//; s/"$//; s/\n/\0/g' stdin >stdin0
+
+perl -pne 's/^"//; s/\\//; s/"$//; s/\n/\0/g' expected-default > \
+    expected-default0
+
+perl -pne 's/	"/	/; s/\\//; s/"$//; s/[:\t\n]/\0/g' expected-verbose > \
+    expected-verbose0
 
 test_expect_success '--stdin' '
 	expect_from_stdin <expected-default &&

@@ -638,12 +639,13 @@ EOF
 grep -v '^::	' expected-all >expected-verbose
 sed -e 's/.*	//' expected-verbose >expected-default
 
-sed -e 's/^"//' -e 's/\\//' -e 's/"$//' stdin | \
-	tr "\n" "\0" >stdin0
-sed -e 's/^"//' -e 's/\\//' -e 's/"$//' expected-default | \
-	tr "\n" "\0" >expected-default0
-sed -e 's/	"/	/' -e 's/\\//' -e 's/"$//' expected-verbose | \
-	tr ":\t\n" "\0" >expected-verbose0
+perl -pne 's/^"//; s/\\//; s/"$//; s/\n/\0/g' stdin >stdin0
+
+perl -pne 's/^"//; s/\\//; s/"$//; s/\n/\0/g' expected-default > \
+    expected-default0
+
+perl -pne 's/	"/	/; s/\\//; s/"$//; s/[:\t\n]/\0/g' expected-verbose > \
+    expected-verbose0
 
 test_expect_success '--stdin from subdirectory' '
 	expect_from_stdin <expected-default &&
Signature · 1 line
1.8.1.2
#1 lore raw
Quoted 3 lines · Ben Walton

Solaris' tr (both /usr/bin/ and /usr/xpg4/bin) fail to handle the case where the first argument is a multi-character set and the second is a single null character.

Almost all the tr invocations look like converting LF to NUL, except for two that squash a colon ':', HT and LF all to NUL. Is Solaris's tr fine with the former but not the latter?

Quoted 2 lines

We make this change globally in t0008-ignores instead of just for the cases where it matters in order to maintain consistency.

I am not suggesting to keep 'tr "\n" "\0"', but just wanted to make sure I am reading the first paragraph correctly. If we are rewriting, we should do so consistently.

Quoted 1 line

+perl -pne 's/^"//; s/\\//; s/"$//; s/\n/\0/g' stdin >stdin0

What is -pne? Is it the same as -pe?

tr/\n/\0/ (or y/\n/\0/) may be more faithful to the original.

Review

Quoted 2 lines
(quoted diff) +2 -0
+perl -pne 's/^"//; s/\\//; s/"$//; s/\n/\0/g' expected-default > \
+    expected-default0

Ditto. We may want to give the same script used in the above two (and twice again in the later hunk) more descriptive name, e.g.

	broken_c_unquote () {
		perl -pe '... that script ...' "$@"
	}

	broken_c_quote stdin >stdin0

Side note: the script is broken as a generic C-unquote function in multiple ways. It does not work if it has more than one backslash quoted characters, it does not understand \t, \b, \015, \\, etc. to name two.

But the breakage does not matter for the strings used in the test vector.

author
#2 lore raw

On Tue, Jun 18, 2013 at 11:31 PM, Junio C Hamano <[email protected]> wrote:

Sorry for the very slow reply. This got lost in my inbox and I forgot about it.

Quoted 9 lines
Quoted 3 lines · Ben Walton

Solaris' tr (both /usr/bin/ and /usr/xpg4/bin) fail to handle the case where the first argument is a multi-character set and the second is a single null character.

Almost all the tr invocations look like converting LF to NUL, except for two that squash a colon ':', HT and LF all to NUL. Is Solaris's tr fine with the former but not the latter?

In retrospect, this isn't brokenness, just a difference in System V vs BSD semantics for tr, both of which are allowed by POSIX since the behaviour in question is specifically unspecified by the standard. The System V behaviour is to require a 1:1 map between string1 and string2 transformations whereas BSD behaviour (when len(string2) < len(string1)) is to pad string2 with the last character in string2 until the lengths are equal.

Quoted 27 lines
Quoted 3 lines

We make this change globally in t0008-ignores instead of just for the cases where it matters in order to maintain consistency.

I am not suggesting to keep 'tr "\n" "\0"', but just wanted to make sure I am reading the first paragraph correctly. If we are rewriting, we should do so consistently.

Quoted 1 line

+perl -pne 's/^"//; s/\\//; s/"$//; s/\n/\0/g' stdin >stdin0

What is -pne? Is it the same as -pe?

tr/\n/\0/ (or y/\n/\0/) may be more faithful to the original.

Review

Quoted 2 lines
(quoted diff) +2 -0
+perl -pne 's/^"//; s/\\//; s/"$//; s/\n/\0/g' expected-default > \
+    expected-default0

Ditto. We may want to give the same script used in the above two (and twice again in the later hunk) more descriptive name, e.g.

        broken_c_unquote () {
                perl -pe '... that script ...' "$@"
        }

        broken_c_quote stdin >stdin0
Quoted 7 lines

Side note: the script is broken as a generic C-unquote function in multiple ways. It does not work if it has more than one backslash quoted characters, it does not understand \t, \b, \015, \\, etc. to name two.

But the breakage does not matter for the strings used in the test vector.

I've updated the patch and will forward it shortly.

Thanks
-Ben
--
---------------------------------------------------------------------------------------------------------------------------
Take the risk of thinking for yourself. Much more happiness, truth, beauty and wisdom will come to you that way.

-Christopher Hitchens
---------------------------------------------------------------------------------------------------------------------------

author
#3 lore raw

PATCH 1/1 Avoid difference in tr semantics between System V and BSD

Solaris' tr (both /usr/bin/ and /usr/xpg4/bin) uses the System V semantics for tr whereby string1's length is truncated to the length of string2 if string2 is shorter. The BSD semantics, as used by GNU tr see string2 padded to the length of string1 using the final character in string2. POSIX explicitly doesn't specify the correct behavior here, making both equally valid.

This difference means that Solaris' native tr implementations produce different results for tr ":\t\n" "\0" than GNU tr. This breaks a few tests in t0008-ignores.sh.

Possible fixes for this are to make string2 be "\0\0\0" or "[\0*]".

Instead, use perl to perform these transliterations which means we don't need to worry about the difference at all. Since we're replacing tr with perl, we also use perl to replace the sed invocations used to transform the files.

Replace four identical transforms with a function named broken_c_unquote. Replace the other two identical transforms with a fuction named broken_c_unquote_verbose.

Signed-off-by Ben Walton <[email protected]>
---
 t/t0008-ignores.sh | 30 ++++++++++++++++++------------
 1 file changed, 18 insertions(+), 12 deletions(-)
t/t0008-ignores.sh +18 -12
index 181513a..4ebda99 100755
@@ -37,6 +37,14 @@ test_stderr () {
 	test_cmp "$HOME/expected-stderr" "$HOME/stderr"
 }
 
+broken_c_unquote () {
+	perl -pe 's/^"//; s/\\//; s/"$//; tr/\n/\0/' "$@"
+}
+
+broken_c_unquote_verbose () {
+	perl -pe 's/	"/	/; s/\\//; s/"$//; tr/:\t\n/\0/' "$@"
+}
+
 stderr_contains () {
 	regexp="$1"
 	if grep "$regexp" "$HOME/stderr"

@@ -606,12 +614,11 @@ cat <<-EOF >expected-verbose
 	$global_excludes:2:!globaltwo	b/globaltwo
 EOF
 
-sed -e 's/^"//' -e 's/\\//' -e 's/"$//' stdin | \
-	tr "\n" "\0" >stdin0
-sed -e 's/^"//' -e 's/\\//' -e 's/"$//' expected-default | \
-	tr "\n" "\0" >expected-default0
-sed -e 's/	"/	/' -e 's/\\//' -e 's/"$//' expected-verbose | \
-	tr ":\t\n" "\0" >expected-verbose0
+broken_c_unquote stdin >stdin0
+
+broken_c_unquote expected-default >expected-default0
+
+broken_c_unquote_verbose expected-verbose >expected-verbose0
 
 test_expect_success '--stdin' '
 	expect_from_stdin <expected-default &&

@@ -692,12 +699,11 @@ EOF
 grep -v '^::	' expected-all >expected-verbose
 sed -e 's/.*	//' expected-verbose >expected-default
 
-sed -e 's/^"//' -e 's/\\//' -e 's/"$//' stdin | \
-	tr "\n" "\0" >stdin0
-sed -e 's/^"//' -e 's/\\//' -e 's/"$//' expected-default | \
-	tr "\n" "\0" >expected-default0
-sed -e 's/	"/	/' -e 's/\\//' -e 's/"$//' expected-verbose | \
-	tr ":\t\n" "\0" >expected-verbose0
+broken_c_unquote stdin >stdin0
+
+broken_c_unquote expected-default >expected-default0
+
+broken_c_unquote_verbose expected-verbose >expected-verbose0
 
 test_expect_success '--stdin from subdirectory' '
 	expect_from_stdin <expected-default &&
Signature · 1 line
1.8.1.2
#4 lore raw

Re: [PATCH] Avoid difference in tr semantics between System V and BSD

Am 28.10.2013 10:13, schrieb Ben Walton:

Quoted 11 lines

Solaris' tr (both /usr/bin/ and /usr/xpg4/bin) uses the System V semantics for tr whereby string1's length is truncated to the length of string2 if string2 is shorter. The BSD semantics, as used by GNU tr see string2 padded to the length of string1 using the final character in string2. POSIX explicitly doesn't specify the correct behavior here, making both equally valid.

This difference means that Solaris' native tr implementations produce different results for tr ":\t\n" "\0" than GNU tr. This breaks a few tests in t0008-ignores.sh.

Quoted 6 lines

Possible fixes for this are to make string2 be "\0\0\0" or "[\0*]".

Instead, use perl to perform these transliterations which means we don't need to worry about the difference at all. Since we're replacing tr with perl, we also use perl to replace the sed invocations used to transform the files.

In other tests, we check for prerequisite PERL, i.e., we are prepared that perl is not available. Shouldn't we do that here, too?

But OTOH, I think that we should skip as few test cases as possible in such a basic test as t0008.

Just some food for thought...
-- Hannes

#5 lore raw

Re: [PATCH] Avoid difference in tr semantics between System V and BSD

Quoted 2 lines · Johannes Sixt

In other tests, we check for prerequisite PERL, i.e., we are prepared that perl is not available. Shouldn't we do that here, too?

I think the tests assume there's a perl present even when the PERL prereq isn't present already. E.g.:

	nul_to_q () {
		"$PERL_PATH" -pe 'y/\000/Q/'
	}

So in practice the PERL prereq just means "NO_PERL wasn't set", or in other words, "commands that use perl work". Maybe something like the following would help?

Signed-off-by Jonathan Nieder <[email protected]>
t/README +13 -4
index 2167125..54cd064 100644
@@ -629,11 +629,20 @@ See the prereq argument to the test_* functions in the "Test harness
 library" section above and the "test_have_prereq" function for how to
 use these, and "test_set_prereq" for how to define your own.
 
- - PERL & PYTHON
+ - PYTHON
 
-   Git wasn't compiled with NO_PERL=YesPlease or
-   NO_PYTHON=YesPlease. Wrap any tests that need Perl or Python in
-   these.
+   Git wasn't compiled with NO_PYTHON=YesPlease. Wrap any tests that
+   need Python with this.
+
+ - PERL
+
+   Git wasn't compiled with NO_PERL=YesPlease.
+
+   Even without the PERL prerequisite, tests can assume there is a
+   usable perl interpreter at $PERL_PATH, though it need not be
+   particularly modern.
+
+   Wrap tests for commands implemented in Perl with this.
 
  - POSIXPERM
#6 lore raw

Re: [PATCH] Avoid difference in tr semantics between System V and BSD

Quoted 34 lines · Jonathan Nieder
Quoted 2 lines · Johannes Sixt

In other tests, we check for prerequisite PERL, i.e., we are prepared that perl is not available. Shouldn't we do that here, too?

I think the tests assume there's a perl present even when the PERL prereq isn't present already. E.g.:

	nul_to_q () {
		"$PERL_PATH" -pe 'y/\000/Q/'
	}

So in practice the PERL prereq just means "NO_PERL wasn't set", or in other words, "commands that use perl work". Maybe something like the following would help?

Signed-off-by Jonathan Nieder <[email protected]>
t/README +3 -4
index 2167125..54cd064 100644
@@ -629,11 +629,20 @@ See the prereq argument to the test_* functions in the "Test harness
 library" section above and the "test_have_prereq" function for how to
 use these, and "test_set_prereq" for how to define your own.
 
- - PERL & PYTHON
+ - PYTHON
 
-   Git wasn't compiled with NO_PERL=YesPlease or
-   NO_PYTHON=YesPlease. Wrap any tests that need Perl or Python in
-   these.
+   Git wasn't compiled with NO_PYTHON=YesPlease. Wrap any tests that
+   need Python with this.

t/README See the prereq argument to the test_* functions in the "Test harness · line 645

Quoted 10 lines
t/README +10 -0
+
+ - PERL
+
+   Git wasn't compiled with NO_PERL=YesPlease.
+
+   Even without the PERL prerequisite, tests can assume there is a
+   usable perl interpreter at $PERL_PATH, though it need not be
+   particularly modern.
+
+   Wrap tests for commands implemented in Perl with this.

Sounds like a sensible thing to address, but I first parsed it as

Wrap (tests for (commands implemented in Perl)) with this.

while I think the readers are expected to parse it as

Wrap ((tests for commands) implemented in Perl) with this.

#7 lore raw

PATCH 1/1 t/README: tests can use perl even with NO_PERL

The git build system supports a NO_PERL switch to avoid installing perl bindings or other features (like "git add --patch") that rely on perl on runtime, but even with NO_PERL it has not been possible for a long time to run tests without perl. Helpers such as

	nul_to_q () {
		"$PERL_PATH" -pe 'y/\000/Q/'
	}

use perl as a better tr or sed and are regularly used in tests without worrying to add a PERL prerequisite.

Perl is portable enough that it seems fine to keep relying on it for this kind of thing in tests (and more readable than the alternative of trying to find POSIXy equivalents). Update the test documentation to clarify this.

Reported-by Johannes Sixt <[email protected]>
Signed-off-by Jonathan Nieder <[email protected]>

---

Quoted 14 lines · Junio C Hamano

Review

Quoted 9 lines · Jonathan Nieder

+ - PERL

(quoted diff) +8 -0
+
+   Git wasn't compiled with NO_PERL=YesPlease.
+
+   Even without the PERL prerequisite, tests can assume there is a
+   usable perl interpreter at $PERL_PATH, though it need not be
+   particularly modern.
+
+   Wrap tests for commands implemented in Perl with this.

Sounds like a sensible thing to address, but I first parsed it as

Quoted 5 lines

Wrap (tests for (commands implemented in Perl)) with this.

while I think the readers are expected to parse it as

Wrap ((tests for commands) implemented in Perl) with this.

I meant the former --- that is, tests for commands like "git add -p" should be skipped in a NO_PERL build. Probably clearest to leave out the third paragraph entirely, like this:

 t/README | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)
t/README +11 -4
index 2167125..0a939eb 100644
@@ -629,11 +629,18 @@ See the prereq argument to the test_* functions in the "Test harness
 library" section above and the "test_have_prereq" function for how to
 use these, and "test_set_prereq" for how to define your own.
 
- - PERL & PYTHON
+ - PYTHON
 
-   Git wasn't compiled with NO_PERL=YesPlease or
-   NO_PYTHON=YesPlease. Wrap any tests that need Perl or Python in
-   these.
+   Git wasn't compiled with NO_PYTHON=YesPlease. Wrap any tests that
+   need Python with this.
+
+ - PERL
+
+   Git wasn't compiled with NO_PERL=YesPlease.
+
+   Even without the PERL prerequisite, tests can assume there is a
+   usable perl interpreter at $PERL_PATH, though it need not be
+   particularly modern.
 
  - POSIXPERM
Signature · 1 line
1.8.4.1
#8 lore raw

Re: [PATCH] t/README: tests can use perl even with NO_PERL

Quoted 12 lines · Jonathan Nieder

The git build system supports a NO_PERL switch to avoid installing perl bindings or other features (like "git add --patch") that rely on perl on runtime, but even with NO_PERL it has not been possible for a long time to run tests without perl. Helpers such as

	nul_to_q () {
		"$PERL_PATH" -pe 'y/\000/Q/'
	}

use perl as a better tr or sed and are regularly used in tests without worrying to add a PERL prerequisite.

Quoted 8 lines

Perl is portable enough that it seems fine to keep relying on it for this kind of thing in tests (and more readable than the alternative of trying to find POSIXy equivalents). Update the test documentation to clarify this.

Reported-by Johannes Sixt <[email protected]>
Signed-off-by Jonathan Nieder <[email protected]>

---

Yeah, I think this accurately the conclusions we've come to informally during review on the list (for a long time we did not even use $PERL_PATH for such "vanilla" cases, but some people have a broken perl in their PATH).

Your patch looks good, and I think Ben's patch does not need a PERL prerequisite. However, it is supposed to use $PERL_PATH, which it does not.

Speaking of which, is there any reason to use the ugly "$PERL_PATH" everywhere, and not simply do:

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

in test-lib.sh?

-Peff

#9 lore raw

Re: [PATCH] t/README: tests can use perl even with NO_PERL

Quoted 8 lines · Jeff King

Speaking of which, is there any reason to use the ugly "$PERL_PATH" everywhere, and not simply do:

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

in test-lib.sh?

Sounds like a nice potential improvement to me. :)

Thanks,
Jonathan

#10 lore raw

cover perl

Quoted 5 lines · Jonathan Nieder
Quoted 3 lines · Jeff King

Speaking of which, is there any reason to use the ugly "$PERL_PATH" everywhere, and not simply do:

Quoted 7 lines
Quoted 5 lines
  perl () {
    "$PERL_PATH" "$@"
  }

in test-lib.sh?

Sounds like a nice potential improvement to me. :)

One answer to "is there any reason..." is "it will loop infinitely if you set PERL_PATH=perl". :) However, we can work around that with "command".

It also may cause problems due to the way one-shot variables are treated when calling a function versus a command, but we do not seem to set any variables for invocations perl (and I do not envision it happening often).

And finally, the other reason I can think of is that we can't apply it consistently. It only helps where a shell function would activate, which makes the end result potentially more confusing (especially to somebody who does not really grok shells and subprocesses). Still, it does not introduce any _new_ cases that need it, but only helps with a subset of the cases. So in that sense it is a strict improvement, as we can let most uses go, but catch only the trickier cases in review.

So I'm on the fence on whether it is a good idea or not, but I wrote up the patches to play with it. I also noticed that we do not consistently use $PERL_PATH in some of the built scripts, so I included that fix, too.

Note that I do not have a system with a broken perl. I simulated a very broken perl, which is how I found all of the spots to fix. But whether they are actual bugs that would trigger due to a Windows perl that handles CRLF differently, I have no clue.

[1/3]: use @@PERL@@ in built scripts
[2/3]: t: provide a perl() function which uses $PERL_PATH [3/3]: t: use perl instead of "$PERL_PATH" where applicable

-Peff

#11 lore raw

PATCH 1/3 use @@PERL@@ in built scripts

Several of the built shell commands invoke a bare "perl" to perform some one-liners. This will use the first perl in the PATH rather than the one specified by the user's SHELL_PATH. We are not asking these perl invocations to do anything exotic, so typically any old system perl will do; however, in some cases the system perl may have unexpected behavior (e.g., by handling line endings differently). We should err on the side of using the perl the user pointed us to.

The downside of this is that on systems with a sane perl setup, we no longer find the perl at runtime, but instead point to a static perl (like /usr/bin/perl). That means we will not handle somebody moving perl without rebuilding git, whereas before we tracked it just fine. This is probably not a big deal, though, as the built perl scripts already suffered from this.

Signed-off-by Jeff King <[email protected]>
---
 git-am.sh           | 4 ++--
 git-instaweb.sh     | 2 +-
 git-request-pull.sh | 2 +-
 git-submodule.sh    | 2 +-
 4 files changed, 5 insertions(+), 5 deletions(-)
git-am.sh +2 -2
index 7ea40fe..bbea430 100755
@@ -302,7 +302,7 @@ split_patches () {
 			# not starting with Author, From or Date is the
 			# subject, and the body starts with the next nonempty
 			# line not starting with Author, From or Date
-			perl -ne 'BEGIN { $subject = 0 }
+			@@PERL@@ -ne 'BEGIN { $subject = 0 }
 				if ($subject > 1) { print ; }
 				elsif (/^\s+$/) { next ; }
 				elsif (/^Author:/) { s/Author/From/ ; print ;}

@@ -334,7 +334,7 @@ split_patches () {
 			# Since we cannot guarantee that the commit message is in
 			# git-friendly format, we put no Subject: line and just consume
 			# all of the message as the body
-			LANG=C LC_ALL=C perl -M'POSIX qw(strftime)' -ne 'BEGIN { $subject = 0 }
+			LANG=C LC_ALL=C @@PERL@@ -M'POSIX qw(strftime)' -ne 'BEGIN { $subject = 0 }
 				if ($subject) { print ; }
 				elsif (/^\# User /) { s/\# User/From:/ ; print ; }
 				elsif (/^\# Date /) {
git-instaweb.sh +1 -1
index 01a1b05..e93a238 100755
@@ -581,7 +581,7 @@ EOF
 
 gitweb_conf() {
 	cat > "$fqgitdir/gitweb/gitweb_config.perl" <<EOF
-#!/usr/bin/perl
+#!@@PERL@@
 our \$projectroot = "$(dirname "$fqgitdir")";
 our \$git_temp = "$fqgitdir/gitweb/tmp";
 our \$projects_list = \$projectroot;
git-request-pull.sh +1 -1
index ebf1269..fe21d5d 100755
@@ -106,7 +106,7 @@ find_matching_ref='
 	}
 '
 
-ref=$(git ls-remote "$url" | perl -e "$find_matching_ref" "$head" "$headrev" "$tag_name")
+ref=$(git ls-remote "$url" | @@PERL@@ -e "$find_matching_ref" "$head" "$headrev" "$tag_name")
 
 url=$(git ls-remote --get-url "$url")
git-submodule.sh +1 -1
index 896f1c9..20ebf2e 100755
@@ -156,7 +156,7 @@ module_list()
 		git ls-files -z --error-unmatch --stage -- "$@" ||
 		echo "unmatched pathspec exists"
 	) |
-	perl -e '
+	@@PERL@@ -e '
 	my %unmerged = ();
 	my ($null_sha1) = ("0" x 40);
 	my @out = ();
Signature · 1 line
1.8.4.1.898.g8bf8a41.dirty
#12 lore raw

Re: [PATCH 1/3] use @@PERL@@ in built scripts

Quoted 8 lines · Jeff King

Several of the built shell commands invoke a bare "perl" to perform some one-liners. This will use the first perl in the PATH rather than the one specified by the user's SHELL_PATH. We are not asking these perl invocations to do anything exotic, so typically any old system perl will do; however, in some cases the system perl may have unexpected behavior (e.g., by handling line endings differently). We should err on the side of using the perl the user pointed us to.

Thanks; this makes sense.

#13 lore raw

PATCH 2/3 t: provide a perl() function which uses $PERL_PATH

In reply to Jeff King

Once upon a time, we assumed that calling a bare "perl" in the test scripts was OK, because we would find the perl from the user's PATH, and we were only asking that perl to do basic operations that work even on old versions of perl.

Later, we found that some systems really prefer to use $PERL_PATH even for these basic cases, because the system perl misbehaves in some way (e.g., by handling line endings differently). We then switched "perl" invocations to "$PERL_PATH" to respect the user's choice.

Having to use "$PERL_PATH" is ugly and cumbersome, though. Instead, let's provide a perl() shell function that tests can use, which will transparently do the right thing.

Unfortunately, test writers still have to use $PERL_PATH in certain situations, so we still need to keep the advice in the README.

Note that this may fix test failures in t5004, t5503, t6002, t6003, t6300, t8001, and t8002, depending on your system's perl setup. All of these can be detected by running:

ln -s /bin/false bin-wrappers/perl
make test

which fails before this patch, and passes after.

Signed-off-by Jeff King <[email protected]>

---
We could always "pollute" bin-wrappers with a broken perl to catch errors like these, but I think that would also pollute people who put bin-wrappers into their $PATH. I think we'd need a separate "test-wrappers" directory, and then put both it and bin-wrappers into the PATH. I don't know if that is worth it or not.

 t/README                | 12 ++++++++----
 t/test-lib-functions.sh |  4 ++++
 2 files changed, 12 insertions(+), 4 deletions(-)
t/README +8 -4
index 2167125..06bc5ed 100644
@@ -340,7 +340,11 @@ Don't:
  - use perl without spelling it as "$PERL_PATH". This is to help our
    friends on Windows where the platform Perl often adds CR before
    the end of line, and they bundle Git with a version of Perl that
-   does not do so, whose path is specified with $PERL_PATH.
+   does not do so, whose path is specified with $PERL_PATH. Note that we
+   provide a "perl" function which uses $PERL_PATH under the hood, so
+   you do not need to worry when simply running perl in the test scripts
+   (but you do, for example, on a shebang line or in a sub script
+   created via "write_script").
 
  - use sh without spelling it as "$SHELL_PATH", when the script can
    be misinterpreted by broken platform shell (e.g. Solaris).

@@ -387,7 +391,7 @@ of the test_* functions (see the "Test harness library" section
 below), e.g.:
 
     test_expect_success PERL 'I need Perl' '
-        "$PERL_PATH" -e "hlagh() if unf_unf()"
+        perl -e "hlagh() if unf_unf()"
     '
 
 The advantage of skipping tests like this is that platforms that don't

@@ -520,7 +524,7 @@ library for your script to use.
 
 	test_external \
 	    'GitwebCache::*FileCache*' \
-	    "$PERL_PATH" "$TEST_DIRECTORY"/t9503/test_cache_interface.pl
+	    perl "$TEST_DIRECTORY"/t9503/test_cache_interface.pl
 
    If the test is outputting its own TAP you should set the
    test_external_has_tap variable somewhere before calling the first

@@ -536,7 +540,7 @@ library for your script to use.
 
 	test_external_without_stderr \
 	    'Perl API' \
-	    "$PERL_PATH" "$TEST_DIRECTORY"/t9700/test.pl
+	    perl "$TEST_DIRECTORY"/t9700/test.pl
 
  - test_expect_code <exit-code> <command>
t/test-lib-functions.sh +4 -0
index a7e9aac..53af452 100644
@@ -710,3 +710,7 @@ test_ln_s_add () {
 		git update-index --add --cacheinfo 120000 $ln_s_obj "$2"
 	fi
 }
+
+perl () {
+	command "$PERL_PATH" "$@"
+}
Signature · 1 line
1.8.4.1.898.g8bf8a41.dirty
#14 lore raw

PATCH 3/3 t: use perl instead of "$PERL_PATH" where applicable

In reply to Jeff King

As of the last commit, we can use "perl" instead of "$PERL_PATH" when running tests, as the former is now a function which uses the latter. As the shorter "perl" is easier on the eyes, let's switch to using it everywhere.

This is not quite a mechanical s/$PERL_PATH/perl/ replacement, though. There are some places where we invoke perl from a script we generate on the fly, and those scripts do not have access to our internal shell functions. The result can be double-checked by running:

ln -s /bin/false bin-wrappers/perl
make test

which continues to pass even after this patch.

Signed-off-by Jeff King <[email protected]>

---
This one is noisy and optional on top of 2/3. We could also just fix up cases as we see them going forward.

 t/gitweb-lib.sh                           | 2 +-
 t/lib-git-svn.sh                          | 4 ++--
 t/lib-terminal.sh                         | 4 ++--
 t/t0202-gettext-perl.sh                   | 4 ++--
 t/t1010-mktree.sh                         | 4 ++--
 t/t3300-funny-names.sh                    | 6 +++---
 t/t4014-format-patch.sh                   | 2 +-
 t/t4020-diff-external.sh                  | 2 +-
 t/t4029-diff-trailing-space.sh            | 2 +-
 t/t4103-apply-binary.sh                   | 4 ++--
 t/t4116-apply-reverse.sh                  | 4 ++--
 t/t4200-rerere.sh                         | 8 ++++----
 t/t5300-pack-object.sh                    | 8 ++++----
 t/t5303-pack-corruption-resilience.sh     | 4 ++--
 t/t5551-http-fetch.sh                     | 2 +-
 t/t6011-rev-list-with-bad-commit.sh       | 2 +-
 t/t6013-rev-list-reverse-parents.sh       | 4 ++--
 t/t7508-status.sh                         | 2 +-
 t/t9129-git-svn-i18n-commitencoding.sh    | 2 +-
 t/t9137-git-svn-dcommit-clobber-series.sh | 8 ++++----
 t/t9300-fast-import.sh                    | 2 +-
 t/t9350-fast-export.sh                    | 2 +-
 t/t9400-git-cvsserver-server.sh           | 2 +-
 t/t9401-git-cvsserver-crlf.sh             | 2 +-
 t/t9402-git-cvsserver-refs.sh             | 2 +-
 t/t9700-perl-git.sh                       | 4 ++--
 t/t9810-git-p4-rcs.sh                     | 2 +-
 t/test-lib-functions.sh                   | 6 +++---
 28 files changed, 50 insertions(+), 50 deletions(-)
t/gitweb-lib.sh +1 -1
index 9e381e0..8cf909a 100644
@@ -69,7 +69,7 @@ gitweb_run () {
 	# written to web server logs, so we are not interested in that:
 	# we are interested only in properly formatted errors/warnings
 	rm -f gitweb.log &&
-	"$PERL_PATH" -- "$SCRIPT_NAME" \
+	perl -- "$SCRIPT_NAME" \
 		>gitweb.output 2>gitweb.log &&
 	perl -w -e '
 		open O, ">gitweb.headers";
t/lib-git-svn.sh +2 -2
index c5e55b1..b0ec12f 100644
@@ -29,7 +29,7 @@ export svnrepo
 svnconf=$PWD/svnconf
 export svnconf
 
-"$PERL_PATH" -w -e "
+perl -w -e "
 use SVN::Core;
 use SVN::Repos;
 \$SVN::Core::VERSION gt '1.1.0' or exit(42);

@@ -146,7 +146,7 @@ stop_httpd () {
 }
 
 convert_to_rev_db () {
-	"$PERL_PATH" -w -- - "$@" <<\EOF
+	perl -w -- - "$@" <<\EOF
 use strict;
 @ARGV == 2 or die "usage: convert_to_rev_db <input> <output>";
 open my $wr, '+>', $ARGV[1] or die "$!: couldn't open: $ARGV[1]";
t/lib-terminal.sh +2 -2
index 58d911d..737df28 100644
@@ -19,7 +19,7 @@ test_expect_success PERL 'set up terminal for tests' '
 	then
 		:
 	elif
-		"$PERL_PATH" "$TEST_DIRECTORY"/test-terminal.perl \
+		perl "$TEST_DIRECTORY"/test-terminal.perl \
 			sh -c "test -t 1 && test -t 2"
 	then
 		test_set_prereq TTY &&

@@ -29,7 +29,7 @@ test_expect_success PERL 'set up terminal for tests' '
 				echo >&4 "test_terminal: need to declare TTY prerequisite"
 				return 127
 			fi
-			"$PERL_PATH" "$TEST_DIRECTORY"/test-terminal.perl "$@"
+			perl "$TEST_DIRECTORY"/test-terminal.perl "$@"
 		}
 	fi
 '
t/t0202-gettext-perl.sh +2 -2
index 428ebb0..a29d166 100755
@@ -12,7 +12,7 @@ if ! test_have_prereq PERL; then
 	test_done
 fi
 
-"$PERL_PATH" -MTest::More -e 0 2>/dev/null || {
+perl -MTest::More -e 0 2>/dev/null || {
 	skip_all="Perl Test::More unavailable, skipping test"
 	test_done
 }

@@ -22,6 +22,6 @@ test_external_has_tap=1
 
 test_external_without_stderr \
     'Perl Git::I18N API' \
-    "$PERL_PATH" "$TEST_DIRECTORY"/t0202/test.pl
+    perl "$TEST_DIRECTORY"/t0202/test.pl
 
 test_done
t/t1010-mktree.sh +2 -2
index df573c4..b946f87 100755
@@ -42,13 +42,13 @@ test_expect_success 'ls-tree piped to mktree (2)' '
 '
 
 test_expect_success 'ls-tree output in wrong order given to mktree (1)' '
-	"$PERL_PATH" -e "print reverse <>" <top |
+	perl -e "print reverse <>" <top |
 	git mktree >actual &&
 	test_cmp tree actual
 '
 
 test_expect_success 'ls-tree output in wrong order given to mktree (2)' '
-	"$PERL_PATH" -e "print reverse <>" <top.withsub |
+	perl -e "print reverse <>" <top.withsub |
 	git mktree >actual &&
 	test_cmp tree.withsub actual
 '
t/t3300-funny-names.sh +3 -3
index 7480d6e..9a146f1 100755
@@ -69,7 +69,7 @@ test_expect_success 'ls-files -z does not quote funny filename' '
 	tabs	," (dq) and spaces
 	EOF
 	git ls-files -z >ls-files.z &&
-	"$PERL_PATH" -pe "y/\000/\012/" <ls-files.z >current &&
+	perl -pe "y/\000/\012/" <ls-files.z >current &&
 	test_cmp expected current
 '
 

@@ -106,7 +106,7 @@ test_expect_success 'diff-index -z does not quote funny filename' '
 	tabs	," (dq) and spaces
 	EOF
 	git diff-index -z --name-status $t0 >diff-index.z &&
-	"$PERL_PATH" -pe "y/\000/\012/" <diff-index.z >current &&
+	perl -pe "y/\000/\012/" <diff-index.z >current &&
 	test_cmp expected current
 '
 

@@ -116,7 +116,7 @@ test_expect_success 'diff-tree -z does not quote funny filename' '
 	tabs	," (dq) and spaces
 	EOF
 	git diff-tree -z --name-status $t0 $t1 >diff-tree.z &&
-	"$PERL_PATH" -pe y/\\000/\\012/ <diff-tree.z >current &&
+	perl -pe y/\\000/\\012/ <diff-tree.z >current &&
 	test_cmp expected current
 '
t/t4014-format-patch.sh +1 -1
index 8f272bc..73194b2 100755
@@ -293,7 +293,7 @@ check_threading () {
 	(git format-patch --stdout "$@"; echo $? > status.out) |
 	# Prints everything between the Message-ID and In-Reply-To,
 	# and replaces all Message-ID-lookalikes by a sequence number
-	"$PERL_PATH" -ne '
+	perl -ne '
 		if (/^(message-id|references|in-reply-to)/i) {
 			$printing = 1;
 		} elsif (/^\S/) {
t/t4020-diff-external.sh +1 -1
index 2e7d73f..8a30979 100755
@@ -177,7 +177,7 @@ test_expect_success 'no diff with -diff' '
 	git diff | grep Binary
 '
 
-echo NULZbetweenZwords | "$PERL_PATH" -pe 'y/Z/\000/' > file
+echo NULZbetweenZwords | perl -pe 'y/Z/\000/' > file
 
 test_expect_success 'force diff with "diff"' '
 	echo >.gitattributes "file diff" &&
t/t4029-diff-trailing-space.sh +1 -1
index 36e2f07..3ccc237 100755
@@ -27,7 +27,7 @@ test_expect_success \
      git config --bool diff.suppressBlankEmpty true &&
      git diff f > actual &&
      test_cmp exp actual &&
-     "$PERL_PATH" -i.bak -p -e "s/^\$/ /" exp &&
+     perl -i.bak -p -e "s/^\$/ /" exp &&
      git config --bool diff.suppressBlankEmpty false &&
      git diff f > actual &&
      test_cmp exp actual &&
t/t4103-apply-binary.sh +2 -2
index b1b906b..1b420e3 100755
@@ -23,10 +23,10 @@ test_expect_success 'setup' '
 	git commit -m "Initial Version" 2>/dev/null &&
 
 	git checkout -b binary &&
-	"$PERL_PATH" -pe "y/x/\000/" <file1 >file3 &&
+	perl -pe "y/x/\000/" <file1 >file3 &&
 	cat file3 >file4 &&
 	git add file2 &&
-	"$PERL_PATH" -pe "y/\000/v/" <file3 >file1 &&
+	perl -pe "y/\000/v/" <file3 >file1 &&
 	rm -f file2 &&
 	git update-index --add --remove file1 file2 file3 file4 &&
 	git commit -m "Second Version" &&
t/t4116-apply-reverse.sh +2 -2
index fca8153..2298ece 100755
@@ -12,14 +12,14 @@ test_description='git apply in reverse
 test_expect_success setup '
 
 	for i in a b c d e f g h i j k l m n; do echo $i; done >file1 &&
-	"$PERL_PATH" -pe "y/ijk/\\000\\001\\002/" <file1 >file2 &&
+	perl -pe "y/ijk/\\000\\001\\002/" <file1 >file2 &&
 
 	git add file1 file2 &&
 	git commit -m initial &&
 	git tag initial &&
 
 	for i in a b c g h i J K L m o n p q; do echo $i; done >file1 &&
-	"$PERL_PATH" -pe "y/mon/\\000\\001\\002/" <file1 >file2 &&
+	perl -pe "y/mon/\\000\\001\\002/" <file1 >file2 &&
 
 	git commit -a -m second &&
 	git tag second &&
t/t4200-rerere.sh +4 -4
index 7f6666f..076e770 100755
@@ -78,7 +78,7 @@ test_expect_success 'activate rerere, old style (conflicting merge)' '
 	test_might_fail git config --unset rerere.enabled &&
 	test_must_fail git merge first &&
 
-	sha1=$("$PERL_PATH" -pe "s/	.*//" .git/MERGE_RR) &&
+	sha1=$(perl -pe "s/	.*//" .git/MERGE_RR) &&
 	rr=.git/rr-cache/$sha1 &&
 	grep "^=======\$" $rr/preimage &&
 	! test -f $rr/postimage &&

@@ -91,7 +91,7 @@ test_expect_success 'rerere.enabled works, too' '
 	git reset --hard &&
 	test_must_fail git merge first &&
 
-	sha1=$("$PERL_PATH" -pe "s/	.*//" .git/MERGE_RR) &&
+	sha1=$(perl -pe "s/	.*//" .git/MERGE_RR) &&
 	rr=.git/rr-cache/$sha1 &&
 	grep ^=======$ $rr/preimage
 '

@@ -101,7 +101,7 @@ test_expect_success 'set up rr-cache' '
 	git config rerere.enabled true &&
 	git reset --hard &&
 	test_must_fail git merge first &&
-	sha1=$("$PERL_PATH" -pe "s/	.*//" .git/MERGE_RR) &&
+	sha1=$(perl -pe "s/	.*//" .git/MERGE_RR) &&
 	rr=.git/rr-cache/$sha1
 '
 

@@ -185,7 +185,7 @@ test_expect_success 'rerere updates postimage timestamp' '
 
 test_expect_success 'rerere clear' '
 	rm $rr/postimage &&
-	echo "$sha1	a1" | "$PERL_PATH" -pe "y/\012/\000/" >.git/MERGE_RR &&
+	echo "$sha1	a1" | perl -pe "y/\012/\000/" >.git/MERGE_RR &&
 	git rerere clear &&
 	! test -d $rr
 '
t/t5300-pack-object.sh +4 -4
index a07c871..c755c09 100755
@@ -13,9 +13,9 @@ TRASH=`pwd`
 test_expect_success \
     'setup' \
     'rm -f .git/index* &&
-     "$PERL_PATH" -e "print \"a\" x 4096;" > a &&
-     "$PERL_PATH" -e "print \"b\" x 4096;" > b &&
-     "$PERL_PATH" -e "print \"c\" x 4096;" > c &&
+     perl -e "print \"a\" x 4096;" > a &&
+     perl -e "print \"b\" x 4096;" > b &&
+     perl -e "print \"c\" x 4096;" > c &&
      test-genrandom "seed a" 2097152 > a_big &&
      test-genrandom "seed b" 2097152 > b_big &&
      git update-index --add a a_big b b_big c &&

@@ -129,7 +129,7 @@ test_expect_success \
 cd "$TRASH"
 
 test_expect_success 'compare delta flavors' '
-	"$PERL_PATH" -e '\''
+	perl -e '\''
 		defined($_ = -s $_) or die for @ARGV;
 		exit 1 if $ARGV[0] <= $ARGV[1];
 	'\'' test-2-$packname_2.pack test-3-$packname_3.pack
t/t5303-pack-corruption-resilience.sh +2 -2
index 35926de..663b02b 100755
@@ -98,7 +98,7 @@ test_expect_success \
     'create_new_pack &&
      git prune-packed &&
      chmod +w ${pack}.pack &&
-     "$PERL_PATH" -i.bak -pe "s/ base /abcdef/" ${pack}.pack &&
+     perl -i.bak -pe "s/ base /abcdef/" ${pack}.pack &&
      test_must_fail git cat-file blob $blob_1 > /dev/null &&
      test_must_fail git cat-file blob $blob_2 > /dev/null &&
      test_must_fail git cat-file blob $blob_3 > /dev/null'

@@ -155,7 +155,7 @@ test_expect_success \
     'create_new_pack &&
      git prune-packed &&
      chmod +w ${pack}.pack &&
-     "$PERL_PATH" -i.bak -pe "s/ delta1 /abcdefgh/" ${pack}.pack &&
+     perl -i.bak -pe "s/ delta1 /abcdefgh/" ${pack}.pack &&
      git cat-file blob $blob_1 > /dev/null &&
      test_must_fail git cat-file blob $blob_2 > /dev/null &&
      test_must_fail git cat-file blob $blob_3 > /dev/null'
t/t5551-http-fetch.sh +1 -1
index 8196af1..f036392 100755
@@ -224,7 +224,7 @@ test_expect_success EXPENSIVE 'create 50,000 tags in the repo' '
 	done | git fast-import --export-marks=marks &&
 
 	# now assign tags to all the dangling commits we created above
-	tag=$("$PERL_PATH" -e "print \"bla\" x 30") &&
+	tag=$(perl -e "print \"bla\" x 30") &&
 	sed -e "s|^:\([^ ]*\) \(.*\)$|\2 refs/tags/$tag-\1|" <marks >>packed-refs
 	)
 '
t/t6011-rev-list-with-bad-commit.sh +1 -1
index bbb0581..e51eb41 100755
@@ -37,7 +37,7 @@ test_expect_success 'verify number of revisions' \
 
 test_expect_success 'corrupt second commit object' \
    '
-   "$PERL_PATH" -i.bak -pe "s/second commit/socond commit/" .git/objects/pack/*.pack &&
+   perl -i.bak -pe "s/second commit/socond commit/" .git/objects/pack/*.pack &&
    test_must_fail git fsck --full
    '
t/t6013-rev-list-reverse-parents.sh +2 -2
index 892a537..59fc2f0 100755
@@ -25,7 +25,7 @@ test_expect_success 'set up --reverse example' '
 
 test_expect_success '--reverse --parents --full-history combines correctly' '
 	git rev-list --parents --full-history master -- foo |
-		"$PERL_PATH" -e "print reverse <>" > expected &&
+		perl -e "print reverse <>" > expected &&
 	git rev-list --reverse --parents --full-history master -- foo \
 		> actual &&
 	test_cmp actual expected

@@ -33,7 +33,7 @@ test_expect_success '--reverse --parents --full-history combines correctly' '
 
 test_expect_success '--boundary does too' '
 	git rev-list --boundary --parents --full-history master ^root -- foo |
-		"$PERL_PATH" -e "print reverse <>" > expected &&
+		perl -e "print reverse <>" > expected &&
 	git rev-list --boundary --reverse --parents --full-history \
 		master ^root -- foo > actual &&
 	test_cmp actual expected
t/t7508-status.sh +1 -1
index 6fb59f3..c987b5e 100755
@@ -994,7 +994,7 @@ test_expect_success 'status -s submodule summary (clean submodule)' '
 
 test_expect_success 'status -z implies porcelain' '
 	git status --porcelain |
-	"$PERL_PATH" -pe "s/\012/\000/g" >expect &&
+	perl -pe "s/\012/\000/g" >expect &&
 	git status -z >output &&
 	test_cmp expect output
 '
t/t9129-git-svn-i18n-commitencoding.sh +1 -1
index 9a40f1e..8cfdfe7 100755
@@ -29,7 +29,7 @@ fi
 compare_svn_head_with () {
 	# extract just the log message and strip out committer info.
 	# don't use --limit here since svn 1.1.x doesn't have it,
-	LC_ALL="$a_utf8_locale" svn log `git svn info --url` | "$PERL_PATH" -w -e '
+	LC_ALL="$a_utf8_locale" svn log `git svn info --url` | perl -w -e '
 		use bytes;
 		$/ = ("-"x72) . "\n";
 		my @x = <STDIN>;
t/t9137-git-svn-dcommit-clobber-series.sh +4 -4
index c17aa31..d60da63 100755
@@ -20,8 +20,8 @@ test_expect_success '(supposedly) non-conflicting change from SVN' '
 	test x"`sed -n -e 61p < file`" = x61 &&
 	svn_cmd co "$svnrepo" tmp &&
 	(cd tmp &&
-		"$PERL_PATH" -i.bak -p -e "s/^58$/5588/" file &&
-		"$PERL_PATH" -i.bak -p -e "s/^61$/6611/" file &&
+		perl -i.bak -p -e "s/^58$/5588/" file &&
+		perl -i.bak -p -e "s/^61$/6611/" file &&
 		poke file &&
 		test x"`sed -n -e 58p < file`" = x5588 &&
 		test x"`sed -n -e 61p < file`" = x6611 &&

@@ -40,8 +40,8 @@ test_expect_success 'some unrelated changes to git' "
 test_expect_success 'change file but in unrelated area' "
 	test x\"\`sed -n -e 4p < file\`\" = x4 &&
 	test x\"\`sed -n -e 7p < file\`\" = x7 &&
-	"$PERL_PATH" -i.bak -p -e 's/^4\$/4444/' file &&
-	"$PERL_PATH" -i.bak -p -e 's/^7\$/7777/' file &&
+	perl -i.bak -p -e 's/^4\$/4444/' file &&
+	perl -i.bak -p -e 's/^7\$/7777/' file &&
 	test x\"\`sed -n -e 4p < file\`\" = x4444 &&
 	test x\"\`sed -n -e 7p < file\`\" = x7777 &&
 	git commit -m '4 => 4444, 7 => 7777' file &&
t/t9300-fast-import.sh +1 -1
index 88fc407..27263df 100755
@@ -12,7 +12,7 @@ test_description='test git fast-import utility'
 # This could be written as "head -c $1", but IRIX "head" does not
 # support the -c option.
 head_c () {
-	"$PERL_PATH" -e '
+	perl -e '
 		my $len = $ARGV[1];
 		while ($len > 0) {
 			my $s;
t/t9350-fast-export.sh +1 -1
index 34c2d8f..2312dec 100755
@@ -429,7 +429,7 @@ test_expect_success 'fast-export quotes pathnames' '
 		--cacheinfo 100644 $blob "path with \\backslash" \
 		--cacheinfo 100644 $blob "path with space" &&
 	 git commit -m addition &&
-	 git ls-files -z -s | "$PERL_PATH" -0pe "s{\\t}{$&subdir/}" >index &&
+	 git ls-files -z -s | perl -0pe "s{\\t}{$&subdir/}" >index &&
 	 git read-tree --empty &&
 	 git update-index -z --index-info <index &&
 	 git commit -m rename &&
t/t9400-git-cvsserver-server.sh +1 -1
index 0431386..3edc408 100755
@@ -20,7 +20,7 @@ then
     skip_all='skipping git-cvsserver tests, cvs not found'
     test_done
 fi
-"$PERL_PATH" -e 'use DBI; use DBD::SQLite' >/dev/null 2>&1 || {
+perl -e 'use DBI; use DBD::SQLite' >/dev/null 2>&1 || {
     skip_all='skipping git-cvsserver tests, Perl SQLite interface unavailable'
     test_done
 }
t/t9401-git-cvsserver-crlf.sh +1 -1
index 8c3db76..5a4ed28 100755
@@ -68,7 +68,7 @@ then
     skip_all='skipping git-cvsserver tests, perl not available'
     test_done
 fi
-"$PERL_PATH" -e 'use DBI; use DBD::SQLite' >/dev/null 2>&1 || {
+perl -e 'use DBI; use DBD::SQLite' >/dev/null 2>&1 || {
     skip_all='skipping git-cvsserver tests, Perl SQLite interface unavailable'
     test_done
 }
t/t9402-git-cvsserver-refs.sh +1 -1
index db69af2..1e266ef 100755
@@ -76,7 +76,7 @@ then
 	skip_all='skipping git-cvsserver tests, perl not available'
 	test_done
 fi
-"$PERL_PATH" -e 'use DBI; use DBD::SQLite' >/dev/null 2>&1 || {
+perl -e 'use DBI; use DBD::SQLite' >/dev/null 2>&1 || {
 	skip_all='skipping git-cvsserver tests, Perl SQLite interface unavailable'
 	test_done
 }
t/t9700-perl-git.sh +2 -2
index 435d896..102c133 100755
@@ -11,7 +11,7 @@ if ! test_have_prereq PERL; then
 	test_done
 fi
 
-"$PERL_PATH" -MTest::More -e 0 2>/dev/null || {
+perl -MTest::More -e 0 2>/dev/null || {
 	skip_all="Perl Test::More unavailable, skipping test"
 	test_done
 }

@@ -55,6 +55,6 @@ test_external_has_tap=1
 
 test_external_without_stderr \
     'Perl API' \
-    "$PERL_PATH" "$TEST_DIRECTORY"/t9700/test.pl
+    perl "$TEST_DIRECTORY"/t9700/test.pl
 
 test_done
t/t9810-git-p4-rcs.sh +1 -1
index 34fbc90..8134ab4 100755
@@ -263,7 +263,7 @@ test_expect_success 'cope with rcs keyword expansion damage' '
 		git config git-p4.attemptRCSCleanup true &&
 		(cd "$cli" && p4_append_to_file kwfile1.c) &&
 		old_lines=$(wc -l <kwfile1.c) &&
-		"$PERL_PATH" -n -i -e "print unless m/Revision:/" kwfile1.c &&
+		perl -n -i -e "print unless m/Revision:/" kwfile1.c &&
 		new_lines=$(wc -l <kwfile1.c) &&
 		test $new_lines = $(($old_lines - 1)) &&
t/test-lib-functions.sh +3 -3
index 53af452..bb878f3 100644
@@ -76,11 +76,11 @@ test_decode_color () {
 }
 
 nul_to_q () {
-	"$PERL_PATH" -pe 'y/\000/Q/'
+	perl -pe 'y/\000/Q/'
 }
 
 q_to_nul () {
-	"$PERL_PATH" -pe 'y/Q/\000/'
+	perl -pe 'y/Q/\000/'
 }
 
 q_to_cr () {

@@ -648,7 +648,7 @@ test_seq () {
 	2)	;;
 	*)	error "bug in the test script: not 1 or 2 parameters to test_seq" ;;
 	esac
-	"$PERL_PATH" -le 'print for $ARGV[0]..$ARGV[1]' -- "$@"
+	perl -le 'print for $ARGV[0]..$ARGV[1]' -- "$@"
 }
 
 # This function can be used to schedule some commands to be run
Signature · 1 line
1.8.4.1.898.g8bf8a41.dirty
author
#15 lore raw

Re: [PATCH] t/README: tests can use perl even with NO_PERL

In reply to Jonathan Nieder
Quoted 5 lines · Jonathan Nieder
Quoted 3 lines · Jeff King

Speaking of which, is there any reason to use the ugly "$PERL_PATH" everywhere, and not simply do:

Quoted 7 lines
Quoted 5 lines
  perl () {
    "$PERL_PATH" "$@"
  }

in test-lib.sh?

Sounds like a nice potential improvement to me. :)

+1.

-Ben

Signature · 6 lines
---------------------------------------------------------------------------------------------------------------------------
Take the risk of thinking for yourself.  Much more happiness,
truth, beauty and wisdom will come to you that way.

-Christopher Hitchens
---------------------------------------------------------------------------------------------------------------------------
#16 lore raw

Re: [PATCH] t/README: tests can use perl even with NO_PERL

In reply to Jonathan Nieder

Am 28.10.2013 20:22, schrieb Jonathan Nieder:

Quoted 9 lines

The git build system supports a NO_PERL switch to avoid installing perl bindings or other features (like "git add --patch") that rely on perl on runtime, but even with NO_PERL it has not been possible for a long time to run tests without perl. Helpers such as

	nul_to_q () {
		"$PERL_PATH" -pe 'y/\000/Q/'
	}
Quoted 7 lines

use perl as a better tr or sed and are regularly used in tests without worrying to add a PERL prerequisite.

Perl is portable enough that it seems fine to keep relying on it for this kind of thing in tests (and more readable than the alternative of trying to find POSIXy equivalents). Update the test documentation to clarify this.

Thank you for the clarification!

-- Hannes

author
#17 lore raw

Re: [PATCH] Avoid difference in tr semantics between System V and BSD

In reply to Junio C Hamano

I'm happy to defer to your judgement on this - If you'd like the tests wrapped, I'll do so.

Thanks
-Ben

Quoted 55 lines · Junio C Hamano
Quoted 34 lines · Jonathan Nieder
Quoted 2 lines · Johannes Sixt

In other tests, we check for prerequisite PERL, i.e., we are prepared that perl is not available. Shouldn't we do that here, too?

I think the tests assume there's a perl present even when the PERL prereq isn't present already. E.g.:

      nul_to_q () {
              "$PERL_PATH" -pe 'y/\000/Q/'
      }

So in practice the PERL prereq just means "NO_PERL wasn't set", or in other words, "commands that use perl work". Maybe something like the following would help?

Signed-off-by Jonathan Nieder <[email protected]>
t/README +3 -4
index 2167125..54cd064 100644
@@ -629,11 +629,20 @@ See the prereq argument to the test_* functions in the "Test harness
 library" section above and the "test_have_prereq" function for how to
 use these, and "test_set_prereq" for how to define your own.
 
- - PERL & PYTHON
+ - PYTHON
 
-   Git wasn't compiled with NO_PERL=YesPlease or
-   NO_PYTHON=YesPlease. Wrap any tests that need Perl or Python in
-   these.
+   Git wasn't compiled with NO_PYTHON=YesPlease. Wrap any tests that
+   need Python with this.

t/README See the prereq argument to the test_* functions in the "Test harness · line 645

Quoted 10 lines
t/README +10 -0
+
+ - PERL
+
+   Git wasn't compiled with NO_PERL=YesPlease.
+
+   Even without the PERL prerequisite, tests can assume there is a
+   usable perl interpreter at $PERL_PATH, though it need not be
+   particularly modern.
+
+   Wrap tests for commands implemented in Perl with this.

Sounds like a sensible thing to address, but I first parsed it as

Wrap (tests for (commands implemented in Perl)) with this.

while I think the readers are expected to parse it as

Wrap ((tests for commands) implemented in Perl) with this.

Signature · 6 lines
---------------------------------------------------------------------------------------------------------------------------
Take the risk of thinking for yourself.  Much more happiness,
truth, beauty and wisdom will come to you that way.

-Christopher Hitchens
---------------------------------------------------------------------------------------------------------------------------
author
#18 lore raw

Re: [PATCH] Avoid difference in tr semantics between System V and BSD

Quoted 62 lines · Ben Walton

I'm happy to defer to your judgement on this - If you'd like the tests wrapped, I'll do so.

Thanks
-Ben

Quoted 55 lines · Junio C Hamano
Quoted 34 lines · Jonathan Nieder
Quoted 2 lines · Johannes Sixt

In other tests, we check for prerequisite PERL, i.e., we are prepared that perl is not available. Shouldn't we do that here, too?

I think the tests assume there's a perl present even when the PERL prereq isn't present already. E.g.:

      nul_to_q () {
              "$PERL_PATH" -pe 'y/\000/Q/'
      }

So in practice the PERL prereq just means "NO_PERL wasn't set", or in other words, "commands that use perl work". Maybe something like the following would help?

Signed-off-by Jonathan Nieder <[email protected]>
t/README +3 -4
index 2167125..54cd064 100644
@@ -629,11 +629,20 @@ See the prereq argument to the test_* functions in the "Test harness
 library" section above and the "test_have_prereq" function for how to
 use these, and "test_set_prereq" for how to define your own.
 
- - PERL & PYTHON
+ - PYTHON
 
-   Git wasn't compiled with NO_PERL=YesPlease or
-   NO_PYTHON=YesPlease. Wrap any tests that need Perl or Python in
-   these.
+   Git wasn't compiled with NO_PYTHON=YesPlease. Wrap any tests that
+   need Python with this.

t/README See the prereq argument to the test_* functions in the "Test harness · line 645

Quoted 10 lines
t/README +10 -0
+
+ - PERL
+
+   Git wasn't compiled with NO_PERL=YesPlease.
+
+   Even without the PERL prerequisite, tests can assume there is a
+   usable perl interpreter at $PERL_PATH, though it need not be
+   particularly modern.
+
+   Wrap tests for commands implemented in Perl with this.

Sounds like a sensible thing to address, but I first parsed it as

Wrap (tests for (commands implemented in Perl)) with this.

while I think the readers are expected to parse it as

Wrap ((tests for commands) implemented in Perl) with this.

Per the other discussion about replacing all PERL_PATH with a shell function named perl, should I update this patch to use $PERL_PATH in the meantime so that it can be batch updated when the function is added in a separate patch?

Thanks
-Ben

Signature · 6 lines
---------------------------------------------------------------------------------------------------------------------------
Take the risk of thinking for yourself.  Much more happiness,
truth, beauty and wisdom will come to you that way.

-Christopher Hitchens
---------------------------------------------------------------------------------------------------------------------------
#19 lore raw

Re: [PATCH] Avoid difference in tr semantics between System V and BSD

Quoted 4 lines · Ben Walton

Per the other discussion about replacing all PERL_PATH with a shell function named perl, should I update this patch to use $PERL_PATH in the meantime so that it can be batch updated when the function is added in a separate patch?

Yeah, sounds like a good plan, and very much appreciated.

Thanks.

author
#20 lore raw

PATCH 1/1 Avoid difference in tr semantics between System V and BSD

Solaris' tr (both /usr/bin/ and /usr/xpg4/bin) uses the System V semantics for tr whereby string1's length is truncated to the length of string2 if string2 is shorter. The BSD semantics, as used by GNU tr see string2 padded to the length of string1 using the final character in string2. POSIX explicitly doesn't specify the correct behavior here, making both equally valid.

This difference means that Solaris' native tr implementations produce different results for tr ":\t\n" "\0" than GNU tr. This breaks a few tests in t0008-ignores.sh.

Possible fixes for this are to make string2 be "\0\0\0" or "[\0*]".

Instead, use perl to perform these transliterations which means we don't need to worry about the difference at all. Since we're replacing tr with perl, we also use perl to replace the sed invocations used to transform the files.

Replace four identical transforms with a function named broken_c_unquote. Replace the other two identical transforms with a fuction named broken_c_unquote_verbose.

Signed-off-by Ben Walton <[email protected]>
---
 t/t0008-ignores.sh | 30 ++++++++++++++++++------------
 1 file changed, 18 insertions(+), 12 deletions(-)
t/t0008-ignores.sh +18 -12
index 181513a..45f9396 100755
@@ -37,6 +37,14 @@ test_stderr () {
 	test_cmp "$HOME/expected-stderr" "$HOME/stderr"
 }
 
+broken_c_unquote () {
+	$PERL_PATH -pe 's/^"//; s/\\//; s/"$//; tr/\n/\0/' "$@"
+}
+
+broken_c_unquote_verbose () {
+	$PERL_PATH -pe 's/	"/	/; s/\\//; s/"$//; tr/:\t\n/\0/' "$@"
+}
+
 stderr_contains () {
 	regexp="$1"
 	if grep "$regexp" "$HOME/stderr"

@@ -606,12 +614,11 @@ cat <<-EOF >expected-verbose
 	$global_excludes:2:!globaltwo	b/globaltwo
 EOF
 
-sed -e 's/^"//' -e 's/\\//' -e 's/"$//' stdin | \
-	tr "\n" "\0" >stdin0
-sed -e 's/^"//' -e 's/\\//' -e 's/"$//' expected-default | \
-	tr "\n" "\0" >expected-default0
-sed -e 's/	"/	/' -e 's/\\//' -e 's/"$//' expected-verbose | \
-	tr ":\t\n" "\0" >expected-verbose0
+broken_c_unquote stdin >stdin0
+
+broken_c_unquote expected-default >expected-default0
+
+broken_c_unquote_verbose expected-verbose >expected-verbose0
 
 test_expect_success '--stdin' '
 	expect_from_stdin <expected-default &&

@@ -692,12 +699,11 @@ EOF
 grep -v '^::	' expected-all >expected-verbose
 sed -e 's/.*	//' expected-verbose >expected-default
 
-sed -e 's/^"//' -e 's/\\//' -e 's/"$//' stdin | \
-	tr "\n" "\0" >stdin0
-sed -e 's/^"//' -e 's/\\//' -e 's/"$//' expected-default | \
-	tr "\n" "\0" >expected-default0
-sed -e 's/	"/	/' -e 's/\\//' -e 's/"$//' expected-verbose | \
-	tr ":\t\n" "\0" >expected-verbose0
+broken_c_unquote stdin >stdin0
+
+broken_c_unquote expected-default >expected-default0
+
+broken_c_unquote_verbose expected-verbose >expected-verbose0
 
 test_expect_success '--stdin from subdirectory' '
 	expect_from_stdin <expected-default &&
Signature · 1 line
1.8.1.2
author
#21 lore raw

Re: [PATCH] Avoid difference in tr semantics between System V and BSD

Ignore this version. The immediate followup quotes PERL_PATH.

Quoted 85 lines · Ben Walton

Solaris' tr (both /usr/bin/ and /usr/xpg4/bin) uses the System V semantics for tr whereby string1's length is truncated to the length of string2 if string2 is shorter. The BSD semantics, as used by GNU tr see string2 padded to the length of string1 using the final character in string2. POSIX explicitly doesn't specify the correct behavior here, making both equally valid.

This difference means that Solaris' native tr implementations produce different results for tr ":\t\n" "\0" than GNU tr. This breaks a few tests in t0008-ignores.sh.

Possible fixes for this are to make string2 be "\0\0\0" or "[\0*]".

Instead, use perl to perform these transliterations which means we don't need to worry about the difference at all. Since we're replacing tr with perl, we also use perl to replace the sed invocations used to transform the files.

Replace four identical transforms with a function named broken_c_unquote. Replace the other two identical transforms with a fuction named broken_c_unquote_verbose.

Signed-off-by Ben Walton <[email protected]>
---
 t/t0008-ignores.sh | 30 ++++++++++++++++++------------
 1 file changed, 18 insertions(+), 12 deletions(-)
t/t0008-ignores.sh +18 -13
index 181513a..45f9396 100755
@@ -37,6 +37,14 @@ test_stderr () {
        test_cmp "$HOME/expected-stderr" "$HOME/stderr"
 }
 
+broken_c_unquote () {
+       $PERL_PATH -pe 's/^"//; s/\\//; s/"$//; tr/\n/\0/' "$@"
+}
+
+broken_c_unquote_verbose () {
+       $PERL_PATH -pe 's/      "/      /; s/\\//; s/"$//; tr/:\t\n/\0/' "$@"
+}
+
 stderr_contains () {
        regexp="$1"
        if grep "$regexp" "$HOME/stderr"

@@ -606,12 +614,11 @@ cat <<-EOF >expected-verbose
        $global_excludes:2:!globaltwo   b/globaltwo
 EOF
 
-sed -e 's/^"//' -e 's/\\//' -e 's/"$//' stdin | \
-       tr "\n" "\0" >stdin0
-sed -e 's/^"//' -e 's/\\//' -e 's/"$//' expected-default | \
-       tr "\n" "\0" >expected-default0
-sed -e 's/     "/      /' -e 's/\\//' -e 's/"$//' expected-verbose | \
-       tr ":\t\n" "\0" >expected-verbose0
+broken_c_unquote stdin >stdin0
+
+broken_c_unquote expected-default >expected-default0
+
+broken_c_unquote_verbose expected-verbose >expected-verbose0
 
 test_expect_success '--stdin' '
        expect_from_stdin <expected-default &&

@@ -692,12 +699,11 @@ EOF
 grep -v '^::   ' expected-all >expected-verbose
 sed -e 's/.*   //' expected-verbose >expected-default
 
-sed -e 's/^"//' -e 's/\\//' -e 's/"$//' stdin | \
-       tr "\n" "\0" >stdin0
-sed -e 's/^"//' -e 's/\\//' -e 's/"$//' expected-default | \
-       tr "\n" "\0" >expected-default0
-sed -e 's/     "/      /' -e 's/\\//' -e 's/"$//' expected-verbose | \
-       tr ":\t\n" "\0" >expected-verbose0
+broken_c_unquote stdin >stdin0
+
+broken_c_unquote expected-default >expected-default0
+
+broken_c_unquote_verbose expected-verbose >expected-verbose0
 
 test_expect_success '--stdin from subdirectory' '
        expect_from_stdin <expected-default &&
--

1.8.1.2

Signature · 6 lines
---------------------------------------------------------------------------------------------------------------------------
Take the risk of thinking for yourself.  Much more happiness,
truth, beauty and wisdom will come to you that way.

-Christopher Hitchens
---------------------------------------------------------------------------------------------------------------------------
author
#22 lore raw

PATCH 1/1 Avoid difference in tr semantics between System V and BSD

In reply to Junio C Hamano

Solaris' tr (both /usr/bin/ and /usr/xpg4/bin) uses the System V semantics for tr whereby string1's length is truncated to the length of string2 if string2 is shorter. The BSD semantics, as used by GNU tr see string2 padded to the length of string1 using the final character in string2. POSIX explicitly doesn't specify the correct behavior here, making both equally valid.

This difference means that Solaris' native tr implementations produce different results for tr ":\t\n" "\0" than GNU tr. This breaks a few tests in t0008-ignores.sh.

Possible fixes for this are to make string2 be "\0\0\0" or "[\0*]".

Instead, use perl to perform these transliterations which means we don't need to worry about the difference at all. Since we're replacing tr with perl, we also use perl to replace the sed invocations used to transform the files.

Replace four identical transforms with a function named broken_c_unquote. Replace the other two identical transforms with a fuction named broken_c_unquote_verbose.

Signed-off-by Ben Walton <[email protected]>

---

...Forgot to quote PERL_PATH in the previous iteration.

 t/t0008-ignores.sh | 30 ++++++++++++++++++------------
 1 file changed, 18 insertions(+), 12 deletions(-)
t/t0008-ignores.sh +18 -12
index 181513a..b4d98e6 100755
@@ -37,6 +37,14 @@ test_stderr () {
 	test_cmp "$HOME/expected-stderr" "$HOME/stderr"
 }
 
+broken_c_unquote () {
+	"$PERL_PATH" -pe 's/^"//; s/\\//; s/"$//; tr/\n/\0/' "$@"
+}
+
+broken_c_unquote_verbose () {
+	"$PERL_PATH" -pe 's/	"/	/; s/\\//; s/"$//; tr/:\t\n/\0/' "$@"
+}
+
 stderr_contains () {
 	regexp="$1"
 	if grep "$regexp" "$HOME/stderr"

@@ -606,12 +614,11 @@ cat <<-EOF >expected-verbose
 	$global_excludes:2:!globaltwo	b/globaltwo
 EOF
 
-sed -e 's/^"//' -e 's/\\//' -e 's/"$//' stdin | \
-	tr "\n" "\0" >stdin0
-sed -e 's/^"//' -e 's/\\//' -e 's/"$//' expected-default | \
-	tr "\n" "\0" >expected-default0
-sed -e 's/	"/	/' -e 's/\\//' -e 's/"$//' expected-verbose | \
-	tr ":\t\n" "\0" >expected-verbose0
+broken_c_unquote stdin >stdin0
+
+broken_c_unquote expected-default >expected-default0
+
+broken_c_unquote_verbose expected-verbose >expected-verbose0
 
 test_expect_success '--stdin' '
 	expect_from_stdin <expected-default &&

@@ -692,12 +699,11 @@ EOF
 grep -v '^::	' expected-all >expected-verbose
 sed -e 's/.*	//' expected-verbose >expected-default
 
-sed -e 's/^"//' -e 's/\\//' -e 's/"$//' stdin | \
-	tr "\n" "\0" >stdin0
-sed -e 's/^"//' -e 's/\\//' -e 's/"$//' expected-default | \
-	tr "\n" "\0" >expected-default0
-sed -e 's/	"/	/' -e 's/\\//' -e 's/"$//' expected-verbose | \
-	tr ":\t\n" "\0" >expected-verbose0
+broken_c_unquote stdin >stdin0
+
+broken_c_unquote expected-default >expected-default0
+
+broken_c_unquote_verbose expected-verbose >expected-verbose0
 
 test_expect_success '--stdin from subdirectory' '
 	expect_from_stdin <expected-default &&
Signature · 1 line
1.8.1.2
#23 lore raw

Re: [PATCH] Avoid difference in tr semantics between System V and BSD

Quoted 18 lines · Ben Walton

Solaris' tr (both /usr/bin/ and /usr/xpg4/bin) uses the System V semantics for tr whereby string1's length is truncated to the length of string2 if string2 is shorter. The BSD semantics, as used by GNU tr see string2 padded to the length of string1 using the final character in string2. POSIX explicitly doesn't specify the correct behavior here, making both equally valid.

This difference means that Solaris' native tr implementations produce different results for tr ":\t\n" "\0" than GNU tr. This breaks a few tests in t0008-ignores.sh.

Possible fixes for this are to make string2 be "\0\0\0" or "[\0*]".

Instead, use perl to perform these transliterations which means we don't need to worry about the difference at all. Since we're replacing tr with perl, we also use perl to replace the sed invocations used to transform the files.

Quoted 8 lines

Replace four identical transforms with a function named broken_c_unquote. Replace the other two identical transforms with a fuction named broken_c_unquote_verbose.

Signed-off-by Ben Walton <[email protected]>

---

...Forgot to quote PERL_PATH in the previous iteration.

Thanks; will requeue.