threads / patch / 49200

patchtests: fix and add lint for non-portable head -c N

Subject: [PATCH] tests: fix and add lint for non-portable head -c N

## tl;dr

33 messages between Aug 23, 2018 and Aug 28, 2018. Diffs are folded; open one to read it.

replies: 32people: 5as markdown or json

Ævar Arnfjörð Bjarmason· Aug 23, 2018, 09:14 UTC · lore

The "head -c BYTES" option is non-portable (not in POSIX[1]). Change such invocations to use the test_copy_bytes wrapper added in 48860819e8 ("t9300: factor out portable "head -c" replacement", 2016-06-30).

This fixes a test added in 9d2e330b17 ("ewah_read_mmap: bounds-check mmap reads", 2018-06-14), which has been breaking t5310-pack-bitmaps.sh on OpenBSD since 2.18.0. The OpenBSD ports already have a similar workaround after their upgrade to 2.18.0[2].

I have not tested this on IRIX, but according to 4de0bbd898 ("t9300: use perl "head -c" clone in place of "dd bs=1 count=16000" kluge", 2010-12-13) this invocation would have broken things there too.

Also, change a valgrind-specific codepath in test-lib.sh to use this wrapper. Given where valgrind runs I don't think this would ever become a portability issue in practice, but it's easier to just use the wrapper than introduce some exception for the "make test-lint" check being added here.

1. http://pubs.opengroup.org/onlinepubs/9699919799/utilities/head.html
2. https://github.com/openbsd/ports/commit/08d5d82eaefe5cf2f125ecc0c6a57df9cf91350c#diff-f7d3c4fabeed1691620d608f1534f5e5
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
Junio: Even though this isn't a 2.19.0-rc0 regression I think it makes
sense for 2.19.0. The fix is trivial, and it'll unbreak (at least some
of) the tests on stock git on OpenBSD.

OpenBSD guys: If you CC the git mailing list when you find you need to apply patches like these, we're happy to fix this more pro-actively. I just happened to be testing the upcoming 2.19.0 on OpenBSD and spotted this.

 t/check-non-portable-shell.pl | 1 +
 t/t5310-pack-bitmaps.sh       | 2 +-
 t/test-lib.sh                 | 4 ++--
 3 files changed, 4 insertions(+), 3 deletions(-)
Show changes to 3 files +4 −3

t/check-non-portable-shell.pl, t/t5310-pack-bitmaps.sh, t/test-lib.sh

diff --git a/t/check-non-portable-shell.pl b/t/check-non-portable-shell.pl
index d5823f71d8..94a7e6165e 100755
--- a/t/check-non-portable-shell.pl
+++ b/t/check-non-portable-shell.pl
@@ -35,6 +35,7 @@ sub err {
 		chomp;
 	}
 
+	/\bhead\s+-c\b/ and err 'head -c is not portable (use test_copy_bytes BYTES <file >out)';
 	/\bsed\s+-i/ and err 'sed -i is not portable';
 	/\becho\s+-[neE]/ and err 'echo with option is not portable (use printf)';
 	/^\s*declare\s+/ and err 'arrays/declare not portable';
diff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh
index 557bd0d0c0..7bff7923f2 100755
--- a/t/t5310-pack-bitmaps.sh
+++ b/t/t5310-pack-bitmaps.sh
@@ -335,7 +335,7 @@ test_expect_success 'truncated bitmap fails gracefully' '
 	git rev-list --use-bitmap-index --count --all >expect &&
 	bitmap=$(ls .git/objects/pack/*.bitmap) &&
 	test_when_finished "rm -f $bitmap" &&
-	head -c 512 <$bitmap >$bitmap.tmp &&
+	test_copy_bytes 512 <$bitmap >$bitmap.tmp &&
 	mv -f $bitmap.tmp $bitmap &&
 	git rev-list --use-bitmap-index --count --all >actual 2>stderr &&
 	test_cmp expect actual &&
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 8bb0f4348e..44288cbb59 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -867,7 +867,7 @@ then
 		# handle only executables, unless they are shell libraries that
 		# need to be in the exec-path.
 		test -x "$1" ||
-		test "# " = "$(head -c 2 <"$1")" ||
+		test "# " = "$(test_copy_bytes 2 <"$1")" ||
 		return;
 
 		base=$(basename "$1")
@@ -882,7 +882,7 @@ then
 		# do not override scripts
 		if test -x "$symlink_target" &&
 		    test ! -d "$symlink_target" &&
-		    test "#!" != "$(head -c 2 < "$symlink_target")"
+		    test "#!" != "$(test_copy_bytes 2 <"$symlink_target")"
 		then
 			symlink_target=../valgrind.sh
 		fi
-- 
2.18.0.865.gffc8e1a3cd6
Ævar Arnfjörð Bjarmason· Aug 23, 2018, 09:56 UTC · re: Ævar Arnfjörð Bjarmason · lore

Test failures on OpenBSD

This is on OpenBSD 6.2 amd64 with my "tests: fix and add lint for non-portable head -c N" patch (which fixes one failure).

    $ for t in t1305-config-include.sh t1308-config-set.sh t5004-archive-corner-cases.sh t5552-skipping-fetch-negotiator.sh; do ./$t --no-color -v -x 2>&1 | grep -B10 "^not ok"; done
    
    + cd bar
    + echo [includeIf "gitdir:bar/"]path=bar7
    + >> .git/config 
    + echo [test]seven=7
    + > .git/bar7 
    + echo 7
    + > expect 
    + git config test.seven
    + > actual 
    error: last command exited with $?=1
    not ok 27 - conditional include, gitdir matching symlink
    --
    + cd bar
    + echo [includeIf "gitdir/i:BAR/"]path=bar8
    + >> .git/config 
    + echo [test]eight=8
    + > .git/bar8 
    + echo 8
    + > expect 
    + git config test.eight
    + > actual 
    error: last command exited with $?=1
    not ok 28 - conditional include, gitdir matching symlink, icase
    	test_cmp expect actual
    
    + echo Error (-1) reading configuration file a-directory.
    + > expect 
    + mkdir a-directory
    + test_expect_code 2 test-tool config configset_get_value foo.bar a-directory
    + 2> output 
    Value not found for "foo.bar"
    test_expect_code: command exited with 1, we wanted 2 test-tool config configset_get_value foo.bar a-directory
    error: last command exited with $?=1
    not ok 23 - proper error on directory "files"
    + make_dir extract
    + tar xf subtree-all.tar -C extract
    tar: Cannot identify format. Searching...
    tar: End of archive volume 1 reached
    tar: Sorry, unable to determine archive format.
    error: last command exited with $?=1
    + rm -rf extract
    + exit 1
    + eval_ret=1
    + :
    not ok 9 - archive empty subtree with no pathspec
    --
    + make_dir extract
    + tar xf subtree-path.tar -C extract
    tar: Cannot identify format. Searching...
    tar: End of archive volume 1 reached
    tar: Sorry, unable to determine archive format.
    error: last command exited with $?=1
    + rm -rf extract
    + exit 1
    + eval_ret=1
    + :
    not ok 10 - archive empty subtree by direct pathspec
    'git <command> [<revision>...] -- [<file>...]'
    fatal: ambiguous argument 'c7': unknown revision or path not in the working tree.
    Use '--' to separate paths from revisions, like this:
    'git <command> [<revision>...] -- [<file>...]'
    No have c7 (c7)
    error: last command exited with $?=1
    + test_unconfig -C client fetch.negotiationalgorithm
    + exit 1
    + eval_ret=1
    + :
    not ok 1 - commits with no parents are sent regardless of skip distance
    --
     Author: A U Thor <author@example.com>
     1 file changed, 1 insertion(+)
     create mode 100644 to_fetch.t
    + git init client
    Initialized empty Git repository in /home/avar/g/git/t/trash directory.t5552-skipping-fetch-negotiator/client/.git/
    + seq 11
    ./t5552-skipping-fetch-negotiator.sh: seq: not found
    + git -C client checkout c5
    error: pathspec 'c5' did not match any file(s) known to git
    error: last command exited with $?=1
    not ok 3 - when two skips collide, favor the larger one
    --
    + git init client
    Initialized empty Git repository in /home/avar/g/git/t/trash directory.t5552-skipping-fetch-negotiator/client/.git/
    + seq 8
    ./t5552-skipping-fetch-negotiator.sh: seq: not found
    + seq 19
    ./t5552-skipping-fetch-negotiator.sh: seq: not found
    + pwd
    + git -C server fetch --no-tags /home/avar/g/git/t/trash directory.t5552-skipping-fetch-negotiator/client b1:refs/heads/b1
    fatal: Couldn't find remote ref b1
    error: last command exited with $?=128
    not ok 6 - do not send "have" with ancestors of commits that server ACKed
Full output at https://gitlab.com/snippets/1747801

Some of this, like the t5552-skipping-fetch-negotiator.sh failures are new in 2.19.0 (those go away with s/seq/test_seq). But some are older. E.g. the archive corner cases failure is becuse that test wants unzip & GNU tar, which OpenBSD upstream has patched already: https://github.com/openbsd/ports/blob/master/devel/git/patches/patch-t_test-lib_sh

Junio C Hamano· Aug 23, 2018, 15:53 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: Test failures on OpenBSD

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
Show 9 quoted lines
>     + echo Error (-1) reading configuration file a-directory.
>     + > expect 
>     + mkdir a-directory
>     + test_expect_code 2 test-tool config configset_get_value foo.bar a-directory
>     + 2> output 
>     Value not found for "foo.bar"
>     test_expect_code: command exited with 1, we wanted 2 test-tool config configset_get_value foo.bar a-directory
>     error: last command exited with $?=1
>     not ok 23 - proper error on directory "files"

A blind guess. is this "BSD allows opening a FILE* on a directory and reading bytes from it" aka FREAD_READS_DIRECTORIES?

I wonder why FreeBSD sets it but not OpenBSD in config.mak.uname.
Show 6 quoted lines
>     + make_dir extract
>     + tar xf subtree-all.tar -C extract
>     tar: Cannot identify format. Searching...
>     tar: End of archive volume 1 reached
>     tar: Sorry, unable to determine archive format.
>     error: last command exited with $?=1

Perhaps modern tar elements like extended headers are not supported on the platform-supplied "tar"? "make TAR=gnutar"?

>     + seq 11
>     ./t5552-skipping-fetch-negotiator.sh: seq: not found
This must use test_seq.
 t/t5552-skipping-fetch-negotiator.sh | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)
Show changes to t/t5552-skipping-fetch-negotiator.sh +6 −6
diff --git a/t/t5552-skipping-fetch-negotiator.sh b/t/t5552-skipping-fetch-negotiator.sh
index 5ad5bece55..30857b84a8 100755
--- a/t/t5552-skipping-fetch-negotiator.sh
+++ b/t/t5552-skipping-fetch-negotiator.sh
@@ -46,7 +46,7 @@ test_expect_success 'commits with no parents are sent regardless of skip distanc
 	test_commit -C server to_fetch &&
 
 	git init client &&
-	for i in $(seq 7)
+	for i in $(test_seq 7)
 	do
 		test_commit -C client c$i
 	done &&
@@ -89,7 +89,7 @@ test_expect_success 'when two skips collide, favor the larger one' '
 	test_commit -C server to_fetch &&
 
 	git init client &&
-	for i in $(seq 11)
+	for i in $(test_seq 11)
 	do
 		test_commit -C client c$i
 	done &&
@@ -168,14 +168,14 @@ test_expect_success 'do not send "have" with ancestors of commits that server AC
 	test_commit -C server to_fetch &&
 
 	git init client &&
-	for i in $(seq 8)
+	for i in $(test_seq 8)
 	do
 		git -C client checkout --orphan b$i &&
 		test_commit -C client b$i.c0
 	done &&
-	for j in $(seq 19)
+	for j in $(test_seq 19)
 	do
-		for i in $(seq 8)
+		for i in $(test_seq 8)
 		do
 			git -C client checkout b$i &&
 			test_commit -C client b$i.c$j
@@ -205,7 +205,7 @@ test_expect_success 'do not send "have" with ancestors of commits that server AC
 
 	# fetch-pack should thus not send any more commits in the b1 branch, but
 	# should still send the others (in this test, just check b2).
-	for i in $(seq 0 8)
+	for i in $(test_seq 0 8)
 	do
 		have_not_sent b1.c$i
 	done &&
Ævar Arnfjörð Bjarmason· Aug 23, 2018, 15:25 UTC · re: Ævar Arnfjörð Bjarmason · lore

[PATCH v2 1/2] tests: fix and add lint for non-portable head -c N

The "head -c BYTES" option is non-portable (not in POSIX[1]). Change such invocations to use the test_copy_bytes wrapper added in 48860819e8 ("t9300: factor out portable "head -c" replacement", 2016-06-30).

This fixes a test added in 9d2e330b17 ("ewah_read_mmap: bounds-check mmap reads", 2018-06-14), which has been breaking t5310-pack-bitmaps.sh on OpenBSD since 2.18.0. The OpenBSD ports already have a similar workaround after their upgrade to 2.18.0[2].

I have not tested this on IRIX, but according to 4de0bbd898 ("t9300: use perl "head -c" clone in place of "dd bs=1 count=16000" kluge", 2010-12-13) this invocation would have broken things there too.

Also, change a valgrind-specific codepath in test-lib.sh to use this wrapper. Given where valgrind runs I don't think this would ever become a portability issue in practice, but it's easier to just use the wrapper than introduce some exception for the "make test-lint" check being added here.

1. http://pubs.opengroup.org/onlinepubs/9699919799/utilities/head.html
2. https://github.com/openbsd/ports/commit/08d5d82eaefe5cf2f125ecc0c6a57df9cf91350c#diff-f7d3c4fabeed1691620d608f1534f5e5
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/check-non-portable-shell.pl | 1 +
 t/t5310-pack-bitmaps.sh       | 2 +-
 t/test-lib.sh                 | 4 ++--
 3 files changed, 4 insertions(+), 3 deletions(-)
Show changes to 3 files +4 −3

t/check-non-portable-shell.pl, t/t5310-pack-bitmaps.sh, t/test-lib.sh

diff --git a/t/check-non-portable-shell.pl b/t/check-non-portable-shell.pl
index d5823f71d8..c8f10d40a1 100755
--- a/t/check-non-portable-shell.pl
+++ b/t/check-non-portable-shell.pl
@@ -41,6 +41,7 @@ sub err {
 	/^\s*[^#]\s*which\s/ and err 'which is not portable (use type)';
 	/\btest\s+[^=]*==/ and err '"test a == b" is not portable (use =)';
 	/\bwc -l.*"\s*=/ and err '`"$(wc -l)"` is not portable (use test_line_count)';
+	/\bhead\s+-c\b/ and err 'head -c is not portable (use test_copy_bytes BYTES <file >out)';
 	/\bexport\s+[A-Za-z0-9_]*=/ and err '"export FOO=bar" is not portable (use FOO=bar && export FOO)';
 	/^\s*([A-Z0-9_]+=(\w+|(["']).*?\3)\s+)+(\w+)/ and exists($func{$4}) and
 		err '"FOO=bar shell_func" assignment extends beyond "shell_func"';
diff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh
index 557bd0d0c0..7bff7923f2 100755
--- a/t/t5310-pack-bitmaps.sh
+++ b/t/t5310-pack-bitmaps.sh
@@ -335,7 +335,7 @@ test_expect_success 'truncated bitmap fails gracefully' '
 	git rev-list --use-bitmap-index --count --all >expect &&
 	bitmap=$(ls .git/objects/pack/*.bitmap) &&
 	test_when_finished "rm -f $bitmap" &&
-	head -c 512 <$bitmap >$bitmap.tmp &&
+	test_copy_bytes 512 <$bitmap >$bitmap.tmp &&
 	mv -f $bitmap.tmp $bitmap &&
 	git rev-list --use-bitmap-index --count --all >actual 2>stderr &&
 	test_cmp expect actual &&
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 8bb0f4348e..44288cbb59 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -867,7 +867,7 @@ then
 		# handle only executables, unless they are shell libraries that
 		# need to be in the exec-path.
 		test -x "$1" ||
-		test "# " = "$(head -c 2 <"$1")" ||
+		test "# " = "$(test_copy_bytes 2 <"$1")" ||
 		return;
 
 		base=$(basename "$1")
@@ -882,7 +882,7 @@ then
 		# do not override scripts
 		if test -x "$symlink_target" &&
 		    test ! -d "$symlink_target" &&
-		    test "#!" != "$(head -c 2 < "$symlink_target")"
+		    test "#!" != "$(test_copy_bytes 2 <"$symlink_target")"
 		then
 			symlink_target=../valgrind.sh
 		fi
-- 
2.18.0.865.gffc8e1a3cd6
Jeff King· Aug 23, 2018, 16:11 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH v2 1/2] tests: fix and add lint for non-portable head -c N

On Thu, Aug 23, 2018 at 03:25:01PM +0000, Ævar Arnfjörð Bjarmason wrote:
Show 9 quoted lines
> The "head -c BYTES" option is non-portable (not in POSIX[1]). Change
> such invocations to use the test_copy_bytes wrapper added in
> 48860819e8 ("t9300: factor out portable "head -c" replacement",
> 2016-06-30).
> 
> This fixes a test added in 9d2e330b17 ("ewah_read_mmap: bounds-check
> mmap reads", 2018-06-14), which has been breaking
> t5310-pack-bitmaps.sh on OpenBSD since 2.18.0. The OpenBSD ports
> already have a similar workaround after their upgrade to 2.18.0[2].

Heh, I even considered using this when writing that test. But the reason I introduced test_copy_bytes is not because the target platform did not have "head -c" at all, but because some tests need very specific buffering guarantees when reading from a shared pipe.

That said, if OpenBSD's "head" doesn't have "-c" at all, I'm fine with this as a fix (and it sounds like we know that IRIX lacks it, too).

Show 5 quoted lines
> Also, change a valgrind-specific codepath in test-lib.sh to use this
> wrapper. Given where valgrind runs I don't think this would ever
> become a portability issue in practice, but it's easier to just use
> the wrapper than introduce some exception for the "make test-lint"
> check being added here.

When working on 9d2e330b17, I recall finding these other "head -c" invocations in the test suite when I did 9d2e330b17 and took them as evidence that it was OK to use in vanilla cases. So even if these sites don't affect any platforms in practice, I think it's worth it to ban "head -c" completely.

Show 6 quoted lines
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  t/check-non-portable-shell.pl | 1 +
>  t/t5310-pack-bitmaps.sh       | 2 +-
>  t/test-lib.sh                 | 4 ++--
>  3 files changed, 4 insertions(+), 3 deletions(-)
Patch itself looks good to me. Thanks.
-Peff
Ævar Arnfjörð Bjarmason· Aug 23, 2018, 15:25 UTC · re: Ævar Arnfjörð Bjarmason · lore

[PATCH v2 2/2] tests: fix and add lint for non-portable seq

GNU seq is not a POSIX command, and doesn't exist on e.g. OpenBSD. We've had the test_seq wrapper since d17cf5f3a3 ("tests: Introduce test_seq", 2012-08-04), but use of it keeps coming back, e.g. in the recently added "fetch negotiator" tests being added here.

So let's also add a check to "make test-lint". The regex is aiming to capture the likes of $(seq ..) and "seq" as a stand-alone command, without capturing some existing cases where we e.g. have files called "seq".

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---

Now with a fix & check in v2 for the seq issue mentioned in https://public-inbox.org/git/87a7pdfltp.fsf@evledraar.gmail.com/

 t/check-non-portable-shell.pl        |  1 +
 t/t5552-skipping-fetch-negotiator.sh | 12 ++++++------
 t/t5703-upload-pack-ref-in-want.sh   |  4 ++--
 3 files changed, 9 insertions(+), 8 deletions(-)
Show changes to 3 files +9 −8

t/check-non-portable-shell.pl, t/t5552-skipping-fetch-negotiator.sh, t/t5703-upload-pack-ref-in-want.sh

diff --git a/t/check-non-portable-shell.pl b/t/check-non-portable-shell.pl
index c8f10d40a1..75f38298d7 100755
--- a/t/check-non-portable-shell.pl
+++ b/t/check-non-portable-shell.pl
@@ -42,6 +42,7 @@ sub err {
 	/\btest\s+[^=]*==/ and err '"test a == b" is not portable (use =)';
 	/\bwc -l.*"\s*=/ and err '`"$(wc -l)"` is not portable (use test_line_count)';
 	/\bhead\s+-c\b/ and err 'head -c is not portable (use test_copy_bytes BYTES <file >out)';
+	/(?:\$\(seq|^\s*seq\b)/ and err 'seq is not portable (use test_seq)';
 	/\bexport\s+[A-Za-z0-9_]*=/ and err '"export FOO=bar" is not portable (use FOO=bar && export FOO)';
 	/^\s*([A-Z0-9_]+=(\w+|(["']).*?\3)\s+)+(\w+)/ and exists($func{$4}) and
 		err '"FOO=bar shell_func" assignment extends beyond "shell_func"';
diff --git a/t/t5552-skipping-fetch-negotiator.sh b/t/t5552-skipping-fetch-negotiator.sh
index 5ad5bece55..30857b84a8 100755
--- a/t/t5552-skipping-fetch-negotiator.sh
+++ b/t/t5552-skipping-fetch-negotiator.sh
@@ -46,7 +46,7 @@ test_expect_success 'commits with no parents are sent regardless of skip distanc
 	test_commit -C server to_fetch &&
 
 	git init client &&
-	for i in $(seq 7)
+	for i in $(test_seq 7)
 	do
 		test_commit -C client c$i
 	done &&
@@ -89,7 +89,7 @@ test_expect_success 'when two skips collide, favor the larger one' '
 	test_commit -C server to_fetch &&
 
 	git init client &&
-	for i in $(seq 11)
+	for i in $(test_seq 11)
 	do
 		test_commit -C client c$i
 	done &&
@@ -168,14 +168,14 @@ test_expect_success 'do not send "have" with ancestors of commits that server AC
 	test_commit -C server to_fetch &&
 
 	git init client &&
-	for i in $(seq 8)
+	for i in $(test_seq 8)
 	do
 		git -C client checkout --orphan b$i &&
 		test_commit -C client b$i.c0
 	done &&
-	for j in $(seq 19)
+	for j in $(test_seq 19)
 	do
-		for i in $(seq 8)
+		for i in $(test_seq 8)
 		do
 			git -C client checkout b$i &&
 			test_commit -C client b$i.c$j
@@ -205,7 +205,7 @@ test_expect_success 'do not send "have" with ancestors of commits that server AC
 
 	# fetch-pack should thus not send any more commits in the b1 branch, but
 	# should still send the others (in this test, just check b2).
-	for i in $(seq 0 8)
+	for i in $(test_seq 0 8)
 	do
 		have_not_sent b1.c$i
 	done &&
diff --git a/t/t5703-upload-pack-ref-in-want.sh b/t/t5703-upload-pack-ref-in-want.sh
index a73c55a47e..d1ccc22331 100755
--- a/t/t5703-upload-pack-ref-in-want.sh
+++ b/t/t5703-upload-pack-ref-in-want.sh
@@ -176,7 +176,7 @@ test_expect_success 'setup repos for change-while-negotiating test' '
 		git clone "http://127.0.0.1:$LIB_HTTPD_PORT/smart/repo" "$LOCAL_PRISTINE" &&
 		cd "$LOCAL_PRISTINE" &&
 		git checkout -b side &&
-		for i in $(seq 1 33); do test_commit s$i; done &&
+		for i in $(test_seq 1 33); do test_commit s$i; done &&
 
 		# Add novel commits to upstream
 		git checkout master &&
@@ -289,7 +289,7 @@ test_expect_success 'setup repos for fetching with ref-in-want tests' '
 		git clone "file://$REPO" "$LOCAL_PRISTINE" &&
 		cd "$LOCAL_PRISTINE" &&
 		git checkout -b side &&
-		for i in $(seq 1 33); do test_commit s$i; done &&
+		for i in $(test_seq 1 33); do test_commit s$i; done &&
 
 		# Add novel commits to upstream
 		git checkout master &&
-- 
2.18.0.865.gffc8e1a3cd6
Junio C Hamano· Aug 23, 2018, 16:08 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH v2 2/2] tests: fix and add lint for non-portable seq

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
> GNU seq is not a POSIX command, and doesn't exist on

s/GNU //; the command did not even originate there, but came from V8 and/or Plan9 IIRC.

Show 14 quoted lines
> e.g. OpenBSD. We've had the test_seq wrapper since d17cf5f3a3 ("tests:
> Introduce test_seq", 2012-08-04), but use of it keeps coming back,
> e.g. in the recently added "fetch negotiator" tests being added here.
>
> So let's also add a check to "make test-lint". The regex is aiming to
> capture the likes of $(seq ..) and "seq" as a stand-alone command,
> without capturing some existing cases where we e.g. have files called
> "seq".
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>
> Now with a fix & check in v2 for the seq issue mentioned in
> https://public-inbox.org/git/87a7pdfltp.fsf@evledraar.gmail.com/
Thanks.
Show 14 quoted lines
>  t/check-non-portable-shell.pl        |  1 +
>  t/t5552-skipping-fetch-negotiator.sh | 12 ++++++------
>  t/t5703-upload-pack-ref-in-want.sh   |  4 ++--
>  3 files changed, 9 insertions(+), 8 deletions(-)
>
> diff --git a/t/check-non-portable-shell.pl b/t/check-non-portable-shell.pl
> index c8f10d40a1..75f38298d7 100755
> --- a/t/check-non-portable-shell.pl
> +++ b/t/check-non-portable-shell.pl
> @@ -42,6 +42,7 @@ sub err {
>  	/\btest\s+[^=]*==/ and err '"test a == b" is not portable (use =)';
>  	/\bwc -l.*"\s*=/ and err '`"$(wc -l)"` is not portable (use test_line_count)';
>  	/\bhead\s+-c\b/ and err 'head -c is not portable (use test_copy_bytes BYTES <file >out)';
> +	/(?:\$\(seq|^\s*seq\b)/ and err 'seq is not portable (use test_seq)';

Looking at $(wc -l) thing a few lines above, I am not sure if this deviation is a good idea. Wouldn't /\bseq\s/ be sufficient?

Ævar Arnfjörð Bjarmason· Aug 23, 2018, 17:20 UTC · re: Junio C Hamano · lore

Re: [PATCH v2 2/2] tests: fix and add lint for non-portable seq

On Thu, Aug 23 2018, Junio C Hamano wrote:
Show 41 quoted lines
> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> GNU seq is not a POSIX command, and doesn't exist on
>
> s/GNU //; the command did not even originate there, but came from V8
> and/or Plan9 IIRC.
>
>> e.g. OpenBSD. We've had the test_seq wrapper since d17cf5f3a3 ("tests:
>> Introduce test_seq", 2012-08-04), but use of it keeps coming back,
>> e.g. in the recently added "fetch negotiator" tests being added here.
>>
>> So let's also add a check to "make test-lint". The regex is aiming to
>> capture the likes of $(seq ..) and "seq" as a stand-alone command,
>> without capturing some existing cases where we e.g. have files called
>> "seq".
>>
>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> ---
>>
>> Now with a fix & check in v2 for the seq issue mentioned in
>> https://public-inbox.org/git/87a7pdfltp.fsf@evledraar.gmail.com/
>
> Thanks.
>
>>  t/check-non-portable-shell.pl        |  1 +
>>  t/t5552-skipping-fetch-negotiator.sh | 12 ++++++------
>>  t/t5703-upload-pack-ref-in-want.sh   |  4 ++--
>>  3 files changed, 9 insertions(+), 8 deletions(-)
>>
>> diff --git a/t/check-non-portable-shell.pl b/t/check-non-portable-shell.pl
>> index c8f10d40a1..75f38298d7 100755
>> --- a/t/check-non-portable-shell.pl
>> +++ b/t/check-non-portable-shell.pl
>> @@ -42,6 +42,7 @@ sub err {
>>  	/\btest\s+[^=]*==/ and err '"test a == b" is not portable (use =)';
>>  	/\bwc -l.*"\s*=/ and err '`"$(wc -l)"` is not portable (use test_line_count)';
>>  	/\bhead\s+-c\b/ and err 'head -c is not portable (use test_copy_bytes BYTES <file >out)';
>> +	/(?:\$\(seq|^\s*seq\b)/ and err 'seq is not portable (use test_seq)';
>
> Looking at $(wc -l) thing a few lines above, I am not sure if this
> deviation is a good idea.  Wouldn't /\bseq\s/ be sufficient?
As alluded to in the commit message that will find cases like:
    $ git grep -P '\bseq\b' -- t | grep -P -v '(?:\$\(seq|^\s*seq\b)'
    t/perf/p3400-rebase.sh:16:              seq 1000 >>unrelated-file$i &&
    t/perf/p3400-rebase.sh:21:              seq 1000 | tac >>unrelated-file$i &&
    t/t3404-rebase-interactive.sh:1227:             test_seq 5 | sed "s/$double/&&/" >seq &&
    t/t3404-rebase-interactive.sh:1228:             git add seq &&
    t/t3404-rebase-interactive.sh:1230:             git commit -m seq-$double
    t/t3404-rebase-interactive.sh:1232:     git tag seq-onto &&
    t/t3404-rebase-interactive.sh:1234:     git cherry-pick seq-onto &&
    t/t3404-rebase-interactive.sh:1236:     test_must_fail env FAKE_LINES= git rebase -i seq-onto &&
    t/t3404-rebase-interactive.sh:1239:     git diff --exit-code seq-onto &&
    t/t4022-diff-rewrite.sh:69:     test_seq 1 99 >seq &&
    t/t4022-diff-rewrite.sh:70:     printf 100 >>seq &&
    t/t4022-diff-rewrite.sh:71:     git add seq &&
    t/t4022-diff-rewrite.sh:72:     git commit seq -m seq
    t/t4022-diff-rewrite.sh:76:     test_seq 1 5 >seq &&
    t/t4022-diff-rewrite.sh:77:     test_seq 9331 9420 >>seq &&
    t/t4022-diff-rewrite.sh:78:     test_seq 96 100 >>seq
    t/t4022-diff-rewrite.sh:82:     git diff -B seq >res &&
    t/t4022-diff-rewrite.sh:87:     git diff seq >res &&
    t/t4022-diff-rewrite.sh:93:     git diff -B seq >res &&
    t/t4150-am.sh:933:      git format-patch -2 --stdout >seq.patch &&
    t/t4150-am.sh:945:      test_must_fail git am -3 seq.patch &&
    t/t4150-am.sh:955:      test_must_fail git am -3 seq.patch &&
    t/t5520-pull.sh:295:    git checkout -b seq &&
    t/t5520-pull.sh:296:    test_seq 5 >seq.txt &&
    t/t5520-pull.sh:297:    git add seq.txt &&
    t/t5520-pull.sh:299:    git commit -m "Add seq.txt" &&
    t/t5520-pull.sh:300:    echo 6 >>seq.txt &&
    t/t5520-pull.sh:302:    git commit -m "Append to seq.txt" seq.txt &&
    t/t5520-pull.sh:304:    echo conflicting >>seq.txt &&
    t/t5520-pull.sh:306:    git commit -m "Create conflict" seq.txt &&
    t/t5520-pull.sh:307:    test_must_fail git pull --rebase . seq 2>err >out &&
Ævar Arnfjörð Bjarmason· Aug 23, 2018, 20:35 UTC · re: Ævar Arnfjörð Bjarmason · lore

[PATCH v3 0/5] OpenBSD & AIX etc. portability fixes

This grew a bit more. I'm going to stop poking at this for now. The tests are still broken on OpenBSD (3-5 broken) and on AIX something like 20-30 are broken, but this makes it slightly better.

Ævar Arnfjörð Bjarmason (5):
  tests: fix and add lint for non-portable head -c N
  tests: fix and add lint for non-portable seq
  tests: use shorter here-docs in chainlint.sed for AIX sed
  tests: fix version-specific portability issue in Perl JSON
  tests: fix and add lint for non-portable grep --file
 t/chainlint.sed                      |  8 ++++----
 t/check-non-portable-shell.pl        |  3 +++
 t/t0019/parse_json.perl              |  3 +++
 t/t5310-pack-bitmaps.sh              |  2 +-
 t/t5318-commit-graph.sh              |  2 +-
 t/t5552-skipping-fetch-negotiator.sh | 12 ++++++------
 t/t5703-upload-pack-ref-in-want.sh   |  4 ++--
 t/test-lib.sh                        |  4 ++--
 8 files changed, 22 insertions(+), 16 deletions(-)
-- 
2.18.0.865.gffc8e1a3cd6
Ævar Arnfjörð Bjarmason· Aug 23, 2018, 20:36 UTC · re: Ævar Arnfjörð Bjarmason · lore

[PATCH v3 1/5] tests: fix and add lint for non-portable head -c N

The "head -c BYTES" option is non-portable (not in POSIX[1]). Change such invocations to use the test_copy_bytes wrapper added in 48860819e8 ("t9300: factor out portable "head -c" replacement", 2016-06-30).

This fixes a test added in 9d2e330b17 ("ewah_read_mmap: bounds-check mmap reads", 2018-06-14), which has been breaking t5310-pack-bitmaps.sh on OpenBSD since 2.18.0. The OpenBSD ports already have a similar workaround after their upgrade to 2.18.0[2].

I have not tested this on IRIX, but according to 4de0bbd898 ("t9300: use perl "head -c" clone in place of "dd bs=1 count=16000" kluge", 2010-12-13) this invocation would have broken things there too.

Also, change a valgrind-specific codepath in test-lib.sh to use this wrapper. Given where valgrind runs I don't think this would ever become a portability issue in practice, but it's easier to just use the wrapper than introduce some exception for the "make test-lint" check being added here.

1. http://pubs.opengroup.org/onlinepubs/9699919799/utilities/head.html
2. https://github.com/openbsd/ports/commit/08d5d82eaefe5cf2f125ecc0c6a57df9cf91350c#diff-f7d3c4fabeed1691620d608f1534f5e5
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/check-non-portable-shell.pl | 1 +
 t/t5310-pack-bitmaps.sh       | 2 +-
 t/test-lib.sh                 | 4 ++--
 3 files changed, 4 insertions(+), 3 deletions(-)
Show changes to 3 files +4 −3

t/check-non-portable-shell.pl, t/t5310-pack-bitmaps.sh, t/test-lib.sh

diff --git a/t/check-non-portable-shell.pl b/t/check-non-portable-shell.pl
index d5823f71d8..c8f10d40a1 100755
--- a/t/check-non-portable-shell.pl
+++ b/t/check-non-portable-shell.pl
@@ -41,6 +41,7 @@ sub err {
 	/^\s*[^#]\s*which\s/ and err 'which is not portable (use type)';
 	/\btest\s+[^=]*==/ and err '"test a == b" is not portable (use =)';
 	/\bwc -l.*"\s*=/ and err '`"$(wc -l)"` is not portable (use test_line_count)';
+	/\bhead\s+-c\b/ and err 'head -c is not portable (use test_copy_bytes BYTES <file >out)';
 	/\bexport\s+[A-Za-z0-9_]*=/ and err '"export FOO=bar" is not portable (use FOO=bar && export FOO)';
 	/^\s*([A-Z0-9_]+=(\w+|(["']).*?\3)\s+)+(\w+)/ and exists($func{$4}) and
 		err '"FOO=bar shell_func" assignment extends beyond "shell_func"';
diff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh
index 557bd0d0c0..7bff7923f2 100755
--- a/t/t5310-pack-bitmaps.sh
+++ b/t/t5310-pack-bitmaps.sh
@@ -335,7 +335,7 @@ test_expect_success 'truncated bitmap fails gracefully' '
 	git rev-list --use-bitmap-index --count --all >expect &&
 	bitmap=$(ls .git/objects/pack/*.bitmap) &&
 	test_when_finished "rm -f $bitmap" &&
-	head -c 512 <$bitmap >$bitmap.tmp &&
+	test_copy_bytes 512 <$bitmap >$bitmap.tmp &&
 	mv -f $bitmap.tmp $bitmap &&
 	git rev-list --use-bitmap-index --count --all >actual 2>stderr &&
 	test_cmp expect actual &&
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 8bb0f4348e..44288cbb59 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -867,7 +867,7 @@ then
 		# handle only executables, unless they are shell libraries that
 		# need to be in the exec-path.
 		test -x "$1" ||
-		test "# " = "$(head -c 2 <"$1")" ||
+		test "# " = "$(test_copy_bytes 2 <"$1")" ||
 		return;
 
 		base=$(basename "$1")
@@ -882,7 +882,7 @@ then
 		# do not override scripts
 		if test -x "$symlink_target" &&
 		    test ! -d "$symlink_target" &&
-		    test "#!" != "$(head -c 2 < "$symlink_target")"
+		    test "#!" != "$(test_copy_bytes 2 <"$symlink_target")"
 		then
 			symlink_target=../valgrind.sh
 		fi
-- 
2.18.0.865.gffc8e1a3cd6
Ævar Arnfjörð Bjarmason· Aug 23, 2018, 20:36 UTC · re: Ævar Arnfjörð Bjarmason · lore

[PATCH v3 2/5] tests: fix and add lint for non-portable seq

The seq command is not in POSIX, and doesn't exist on e.g. OpenBSD. We've had the test_seq wrapper since d17cf5f3a3 ("tests: Introduce test_seq", 2012-08-04), but use of it keeps coming back, e.g. in the recently added "fetch negotiator" tests being added here.

So let's also add a check to "make test-lint". The regex is aiming to capture the likes of $(seq ..) and "seq" as a stand-alone command, without capturing some existing cases where we e.g. have files called "seq", as \bseq\b would do.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/check-non-portable-shell.pl        |  1 +
 t/t5552-skipping-fetch-negotiator.sh | 12 ++++++------
 t/t5703-upload-pack-ref-in-want.sh   |  4 ++--
 3 files changed, 9 insertions(+), 8 deletions(-)
Show changes to 3 files +9 −8

t/check-non-portable-shell.pl, t/t5552-skipping-fetch-negotiator.sh, t/t5703-upload-pack-ref-in-want.sh

diff --git a/t/check-non-portable-shell.pl b/t/check-non-portable-shell.pl
index c8f10d40a1..75f38298d7 100755
--- a/t/check-non-portable-shell.pl
+++ b/t/check-non-portable-shell.pl
@@ -42,6 +42,7 @@ sub err {
 	/\btest\s+[^=]*==/ and err '"test a == b" is not portable (use =)';
 	/\bwc -l.*"\s*=/ and err '`"$(wc -l)"` is not portable (use test_line_count)';
 	/\bhead\s+-c\b/ and err 'head -c is not portable (use test_copy_bytes BYTES <file >out)';
+	/(?:\$\(seq|^\s*seq\b)/ and err 'seq is not portable (use test_seq)';
 	/\bexport\s+[A-Za-z0-9_]*=/ and err '"export FOO=bar" is not portable (use FOO=bar && export FOO)';
 	/^\s*([A-Z0-9_]+=(\w+|(["']).*?\3)\s+)+(\w+)/ and exists($func{$4}) and
 		err '"FOO=bar shell_func" assignment extends beyond "shell_func"';
diff --git a/t/t5552-skipping-fetch-negotiator.sh b/t/t5552-skipping-fetch-negotiator.sh
index 5ad5bece55..30857b84a8 100755
--- a/t/t5552-skipping-fetch-negotiator.sh
+++ b/t/t5552-skipping-fetch-negotiator.sh
@@ -46,7 +46,7 @@ test_expect_success 'commits with no parents are sent regardless of skip distanc
 	test_commit -C server to_fetch &&
 
 	git init client &&
-	for i in $(seq 7)
+	for i in $(test_seq 7)
 	do
 		test_commit -C client c$i
 	done &&
@@ -89,7 +89,7 @@ test_expect_success 'when two skips collide, favor the larger one' '
 	test_commit -C server to_fetch &&
 
 	git init client &&
-	for i in $(seq 11)
+	for i in $(test_seq 11)
 	do
 		test_commit -C client c$i
 	done &&
@@ -168,14 +168,14 @@ test_expect_success 'do not send "have" with ancestors of commits that server AC
 	test_commit -C server to_fetch &&
 
 	git init client &&
-	for i in $(seq 8)
+	for i in $(test_seq 8)
 	do
 		git -C client checkout --orphan b$i &&
 		test_commit -C client b$i.c0
 	done &&
-	for j in $(seq 19)
+	for j in $(test_seq 19)
 	do
-		for i in $(seq 8)
+		for i in $(test_seq 8)
 		do
 			git -C client checkout b$i &&
 			test_commit -C client b$i.c$j
@@ -205,7 +205,7 @@ test_expect_success 'do not send "have" with ancestors of commits that server AC
 
 	# fetch-pack should thus not send any more commits in the b1 branch, but
 	# should still send the others (in this test, just check b2).
-	for i in $(seq 0 8)
+	for i in $(test_seq 0 8)
 	do
 		have_not_sent b1.c$i
 	done &&
diff --git a/t/t5703-upload-pack-ref-in-want.sh b/t/t5703-upload-pack-ref-in-want.sh
index a73c55a47e..d1ccc22331 100755
--- a/t/t5703-upload-pack-ref-in-want.sh
+++ b/t/t5703-upload-pack-ref-in-want.sh
@@ -176,7 +176,7 @@ test_expect_success 'setup repos for change-while-negotiating test' '
 		git clone "http://127.0.0.1:$LIB_HTTPD_PORT/smart/repo" "$LOCAL_PRISTINE" &&
 		cd "$LOCAL_PRISTINE" &&
 		git checkout -b side &&
-		for i in $(seq 1 33); do test_commit s$i; done &&
+		for i in $(test_seq 1 33); do test_commit s$i; done &&
 
 		# Add novel commits to upstream
 		git checkout master &&
@@ -289,7 +289,7 @@ test_expect_success 'setup repos for fetching with ref-in-want tests' '
 		git clone "file://$REPO" "$LOCAL_PRISTINE" &&
 		cd "$LOCAL_PRISTINE" &&
 		git checkout -b side &&
-		for i in $(seq 1 33); do test_commit s$i; done &&
+		for i in $(test_seq 1 33); do test_commit s$i; done &&
 
 		# Add novel commits to upstream
 		git checkout master &&
-- 
2.18.0.865.gffc8e1a3cd6
Ævar Arnfjörð Bjarmason· Aug 23, 2018, 20:36 UTC · re: Ævar Arnfjörð Bjarmason · lore

[PATCH v3 3/5] tests: use shorter here-docs in chainlint.sed for AIX sed

Improve the portability of chainlint by using shorter here-docs. On AIX sed will complain about:

    sed: 0602-417 The label :hereslurp is greater than eight
    characters

As noted in [1] there's still a remaining recently introduced portability issue also introduced in 878f988350 ("t/test-lib: teach --chain-lint to detect broken &&-chains in subshells", 2018-07-11), so under AIX the tests must be run with GIT_TEST_CHAIN_LINT=0.

I don't know how to solve the other issue, and this gets us some of the way to GIT_TEST_CHAIN_LINT=1 working again on AIX.

1. https://public-inbox.org/git/871sapezba.fsf@evledraar.gmail.com/
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/chainlint.sed | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
Show changes to t/chainlint.sed +4 −4
diff --git a/t/chainlint.sed b/t/chainlint.sed
index 8544df38df..2333705b27 100644
--- a/t/chainlint.sed
+++ b/t/chainlint.sed
@@ -97,11 +97,11 @@
 /<<[ 	]*[-\\']*[A-Za-z0-9_]/ {
 	s/^\(.*\)<<[ 	]*[-\\']*\([A-Za-z0-9_][A-Za-z0-9_]*\)'*/<\2>\1<</
 	s/[ 	]*<<//
-	:hereslurp
+	:hered
 	N
 	/^<\([^>]*\)>.*\n[ 	]*\1[ 	]*$/!{
 		s/\n.*$//
-		bhereslurp
+		bhered
 	}
 	s/^<[^>]*>//
 	s/\n.*$//
@@ -283,11 +283,11 @@ bfolded
 :heredoc
 s/^\(.*\)<<[ 	]*[-\\']*\([A-Za-z0-9_][A-Za-z0-9_]*\)'*/<\2>\1<</
 s/[ 	]*<<//
-:hereslurpsub
+:heredsub
 N
 /^<\([^>]*\)>.*\n[ 	]*\1[ 	]*$/!{
 	s/\n.*$//
-	bhereslurpsub
+	bheredsub
 }
 s/^<[^>]*>//
 s/\n.*$//
-- 
2.18.0.865.gffc8e1a3cd6
Junio C Hamano· Aug 23, 2018, 20:42 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH v3 3/5] tests: use shorter here-docs in chainlint.sed for AIX sed

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
Show 18 quoted lines
> Improve the portability of chainlint by using shorter here-docs. On
> AIX sed will complain about:
>
>     sed: 0602-417 The label :hereslurp is greater than eight
>     characters
>
> As noted in [1] there's still a remaining recently introduced
> portability issue also introduced in 878f988350 ("t/test-lib: teach
> --chain-lint to detect broken &&-chains in subshells", 2018-07-11), so
> under AIX the tests must be run with GIT_TEST_CHAIN_LINT=0.
>
> I don't know how to solve the other issue, and this gets us some of
> the way to GIT_TEST_CHAIN_LINT=1 working again on AIX.
>
> 1. https://public-inbox.org/git/871sapezba.fsf@evledraar.gmail.com/
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
I'll globally do s/here-doc/label/ while queueing.

POSIX says "The implementation shall support label arguments recognized as unique up to at least 8 bytes", so replacing these labels to shorter strings makes perfect sense.

Will queue; thanks.
Show 35 quoted lines
>  t/chainlint.sed | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/t/chainlint.sed b/t/chainlint.sed
> index 8544df38df..2333705b27 100644
> --- a/t/chainlint.sed
> +++ b/t/chainlint.sed
> @@ -97,11 +97,11 @@
>  /<<[ 	]*[-\\']*[A-Za-z0-9_]/ {
>  	s/^\(.*\)<<[ 	]*[-\\']*\([A-Za-z0-9_][A-Za-z0-9_]*\)'*/<\2>\1<</
>  	s/[ 	]*<<//
> -	:hereslurp
> +	:hered
>  	N
>  	/^<\([^>]*\)>.*\n[ 	]*\1[ 	]*$/!{
>  		s/\n.*$//
> -		bhereslurp
> +		bhered
>  	}
>  	s/^<[^>]*>//
>  	s/\n.*$//
> @@ -283,11 +283,11 @@ bfolded
>  :heredoc
>  s/^\(.*\)<<[ 	]*[-\\']*\([A-Za-z0-9_][A-Za-z0-9_]*\)'*/<\2>\1<</
>  s/[ 	]*<<//
> -:hereslurpsub
> +:heredsub
>  N
>  /^<\([^>]*\)>.*\n[ 	]*\1[ 	]*$/!{
>  	s/\n.*$//
> -	bhereslurpsub
> +	bheredsub
>  }
>  s/^<[^>]*>//
>  s/\n.*$//
Eric Sunshine· Aug 23, 2018, 20:56 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH v3 3/5] tests: use shorter here-docs in chainlint.sed for AIX sed

On Thu, Aug 23, 2018 at 4:36 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

Show 7 quoted lines
> As noted in [1] there's still a remaining recently introduced
> portability issue also introduced in 878f988350 ("t/test-lib: teach
> --chain-lint to detect broken &&-chains in subshells", 2018-07-11), so
> under AIX the tests must be run with GIT_TEST_CHAIN_LINT=0.
>
> I don't know how to solve the other issue, and this gets us some of
> the way to GIT_TEST_CHAIN_LINT=1 working again on AIX.

Does unindenting the comment, as I suggested in [1], fix the remaining problem for you?

[1]: https://public-inbox.org/git/CAPig+cTTbU5HFMKgNyrxTp3+kcK46-Fn=4ZH6zDt1oQChAc3KA@mail.gmail.com/
Ævar Arnfjörð Bjarmason· Aug 24, 2018, 15:20 UTC · re: Eric Sunshine · lore

[PATCH v4 0/6] OpenBSD & AIX etc. portability fixes

On Thu, Aug 23 2018, Eric Sunshine wrote:
Show 14 quoted lines
> On Thu, Aug 23, 2018 at 4:36 PM Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
>> As noted in [1] there's still a remaining recently introduced
>> portability issue also introduced in 878f988350 ("t/test-lib: teach
>> --chain-lint to detect broken &&-chains in subshells", 2018-07-11), so
>> under AIX the tests must be run with GIT_TEST_CHAIN_LINT=0.
>>
>> I don't know how to solve the other issue, and this gets us some of
>> the way to GIT_TEST_CHAIN_LINT=1 working again on AIX.
>
> Does unindenting the comment, as I suggested in [1], fix the remaining
> problem for you?
>
> [1]: https://public-inbox.org/git/CAPig+cTTbU5HFMKgNyrxTp3+kcK46-Fn=4ZH6zDt1oQChAc3KA@mail.gmail.com/

I didn't notice Eric's E-mail before I sent my v3, so going back and testing this revealed two things:

 1) Yes, his suggestion works
 2) AIX sed will complain about one thing at a time, so we had a lot
    more of these "labels too long" problems once I got past fixing
    that issue.

So here's a version, which as noted in 4/6 makes GIT_TEST_CHAIN_LINT=1 fully work on AIX again.

As an aside, the reason I have access to AIX is because I requested access to the GCC compile farm as suggested by someone on-list here the other day: https://cfarm.tetaneutral.net/

They accepted my account request on the basis that I was going to hack on git & perl on those boxes, so if anyone else here is interested in testing stuff for portability...

Ævar Arnfjörð Bjarmason (6):
  tests: fix and add lint for non-portable head -c N
  tests: fix and add lint for non-portable seq
  tests: fix comment syntax in chainlint.sed for AIX sed
  tests: use shorter here-docs in chainlint.sed for AIX sed
  tests: fix version-specific portability issue in Perl JSON
  tests: fix and add lint for non-portable grep --file
 t/chainlint.sed                      | 59 ++++++++++++++--------------
 t/check-non-portable-shell.pl        |  3 ++
 t/t0019/parse_json.perl              |  3 ++
 t/t5310-pack-bitmaps.sh              |  2 +-
 t/t5318-commit-graph.sh              |  2 +-
 t/t5552-skipping-fetch-negotiator.sh | 12 +++---
 t/t5703-upload-pack-ref-in-want.sh   |  4 +-
 t/test-lib.sh                        |  4 +-
 8 files changed, 47 insertions(+), 42 deletions(-)
-- 
2.18.0.865.gffc8e1a3cd6
Ævar Arnfjörð Bjarmason· Aug 24, 2018, 15:20 UTC · re: Eric Sunshine · lore

[PATCH v4 1/6] tests: fix and add lint for non-portable head -c N

The "head -c BYTES" option is non-portable (not in POSIX[1]). Change such invocations to use the test_copy_bytes wrapper added in 48860819e8 ("t9300: factor out portable "head -c" replacement", 2016-06-30).

This fixes a test added in 9d2e330b17 ("ewah_read_mmap: bounds-check mmap reads", 2018-06-14), which has been breaking t5310-pack-bitmaps.sh on OpenBSD since 2.18.0. The OpenBSD ports already have a similar workaround after their upgrade to 2.18.0[2].

I have not tested this on IRIX, but according to 4de0bbd898 ("t9300: use perl "head -c" clone in place of "dd bs=1 count=16000" kluge", 2010-12-13) this invocation would have broken things there too.

Also, change a valgrind-specific codepath in test-lib.sh to use this wrapper. Given where valgrind runs I don't think this would ever become a portability issue in practice, but it's easier to just use the wrapper than introduce some exception for the "make test-lint" check being added here.

1. http://pubs.opengroup.org/onlinepubs/9699919799/utilities/head.html
2. https://github.com/openbsd/ports/commit/08d5d82eaefe5cf2f125ecc0c6a57df9cf91350c#diff-f7d3c4fabeed1691620d608f1534f5e5
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/check-non-portable-shell.pl | 1 +
 t/t5310-pack-bitmaps.sh       | 2 +-
 t/test-lib.sh                 | 4 ++--
 3 files changed, 4 insertions(+), 3 deletions(-)
Show changes to 3 files +4 −3

t/check-non-portable-shell.pl, t/t5310-pack-bitmaps.sh, t/test-lib.sh

diff --git a/t/check-non-portable-shell.pl b/t/check-non-portable-shell.pl
index d5823f71d8..c8f10d40a1 100755
--- a/t/check-non-portable-shell.pl
+++ b/t/check-non-portable-shell.pl
@@ -41,6 +41,7 @@ sub err {
 	/^\s*[^#]\s*which\s/ and err 'which is not portable (use type)';
 	/\btest\s+[^=]*==/ and err '"test a == b" is not portable (use =)';
 	/\bwc -l.*"\s*=/ and err '`"$(wc -l)"` is not portable (use test_line_count)';
+	/\bhead\s+-c\b/ and err 'head -c is not portable (use test_copy_bytes BYTES <file >out)';
 	/\bexport\s+[A-Za-z0-9_]*=/ and err '"export FOO=bar" is not portable (use FOO=bar && export FOO)';
 	/^\s*([A-Z0-9_]+=(\w+|(["']).*?\3)\s+)+(\w+)/ and exists($func{$4}) and
 		err '"FOO=bar shell_func" assignment extends beyond "shell_func"';
diff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh
index 557bd0d0c0..7bff7923f2 100755
--- a/t/t5310-pack-bitmaps.sh
+++ b/t/t5310-pack-bitmaps.sh
@@ -335,7 +335,7 @@ test_expect_success 'truncated bitmap fails gracefully' '
 	git rev-list --use-bitmap-index --count --all >expect &&
 	bitmap=$(ls .git/objects/pack/*.bitmap) &&
 	test_when_finished "rm -f $bitmap" &&
-	head -c 512 <$bitmap >$bitmap.tmp &&
+	test_copy_bytes 512 <$bitmap >$bitmap.tmp &&
 	mv -f $bitmap.tmp $bitmap &&
 	git rev-list --use-bitmap-index --count --all >actual 2>stderr &&
 	test_cmp expect actual &&
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 8bb0f4348e..44288cbb59 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -867,7 +867,7 @@ then
 		# handle only executables, unless they are shell libraries that
 		# need to be in the exec-path.
 		test -x "$1" ||
-		test "# " = "$(head -c 2 <"$1")" ||
+		test "# " = "$(test_copy_bytes 2 <"$1")" ||
 		return;
 
 		base=$(basename "$1")
@@ -882,7 +882,7 @@ then
 		# do not override scripts
 		if test -x "$symlink_target" &&
 		    test ! -d "$symlink_target" &&
-		    test "#!" != "$(head -c 2 < "$symlink_target")"
+		    test "#!" != "$(test_copy_bytes 2 <"$symlink_target")"
 		then
 			symlink_target=../valgrind.sh
 		fi
-- 
2.18.0.865.gffc8e1a3cd6
Ævar Arnfjörð Bjarmason· Aug 24, 2018, 15:20 UTC · re: Eric Sunshine · lore

[PATCH v4 2/6] tests: fix and add lint for non-portable seq

The seq command is not in POSIX, and doesn't exist on e.g. OpenBSD. We've had the test_seq wrapper since d17cf5f3a3 ("tests: Introduce test_seq", 2012-08-04), but use of it keeps coming back, e.g. in the recently added "fetch negotiator" tests being added here.

So let's also add a check to "make test-lint". The regex is aiming to capture the likes of $(seq ..) and "seq" as a stand-alone command, without capturing some existing cases where we e.g. have files called "seq", as \bseq\b would do.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/check-non-portable-shell.pl        |  1 +
 t/t5552-skipping-fetch-negotiator.sh | 12 ++++++------
 t/t5703-upload-pack-ref-in-want.sh   |  4 ++--
 3 files changed, 9 insertions(+), 8 deletions(-)
Show changes to 3 files +9 −8

t/check-non-portable-shell.pl, t/t5552-skipping-fetch-negotiator.sh, t/t5703-upload-pack-ref-in-want.sh

diff --git a/t/check-non-portable-shell.pl b/t/check-non-portable-shell.pl
index c8f10d40a1..75f38298d7 100755
--- a/t/check-non-portable-shell.pl
+++ b/t/check-non-portable-shell.pl
@@ -42,6 +42,7 @@ sub err {
 	/\btest\s+[^=]*==/ and err '"test a == b" is not portable (use =)';
 	/\bwc -l.*"\s*=/ and err '`"$(wc -l)"` is not portable (use test_line_count)';
 	/\bhead\s+-c\b/ and err 'head -c is not portable (use test_copy_bytes BYTES <file >out)';
+	/(?:\$\(seq|^\s*seq\b)/ and err 'seq is not portable (use test_seq)';
 	/\bexport\s+[A-Za-z0-9_]*=/ and err '"export FOO=bar" is not portable (use FOO=bar && export FOO)';
 	/^\s*([A-Z0-9_]+=(\w+|(["']).*?\3)\s+)+(\w+)/ and exists($func{$4}) and
 		err '"FOO=bar shell_func" assignment extends beyond "shell_func"';
diff --git a/t/t5552-skipping-fetch-negotiator.sh b/t/t5552-skipping-fetch-negotiator.sh
index 5ad5bece55..30857b84a8 100755
--- a/t/t5552-skipping-fetch-negotiator.sh
+++ b/t/t5552-skipping-fetch-negotiator.sh
@@ -46,7 +46,7 @@ test_expect_success 'commits with no parents are sent regardless of skip distanc
 	test_commit -C server to_fetch &&
 
 	git init client &&
-	for i in $(seq 7)
+	for i in $(test_seq 7)
 	do
 		test_commit -C client c$i
 	done &&
@@ -89,7 +89,7 @@ test_expect_success 'when two skips collide, favor the larger one' '
 	test_commit -C server to_fetch &&
 
 	git init client &&
-	for i in $(seq 11)
+	for i in $(test_seq 11)
 	do
 		test_commit -C client c$i
 	done &&
@@ -168,14 +168,14 @@ test_expect_success 'do not send "have" with ancestors of commits that server AC
 	test_commit -C server to_fetch &&
 
 	git init client &&
-	for i in $(seq 8)
+	for i in $(test_seq 8)
 	do
 		git -C client checkout --orphan b$i &&
 		test_commit -C client b$i.c0
 	done &&
-	for j in $(seq 19)
+	for j in $(test_seq 19)
 	do
-		for i in $(seq 8)
+		for i in $(test_seq 8)
 		do
 			git -C client checkout b$i &&
 			test_commit -C client b$i.c$j
@@ -205,7 +205,7 @@ test_expect_success 'do not send "have" with ancestors of commits that server AC
 
 	# fetch-pack should thus not send any more commits in the b1 branch, but
 	# should still send the others (in this test, just check b2).
-	for i in $(seq 0 8)
+	for i in $(test_seq 0 8)
 	do
 		have_not_sent b1.c$i
 	done &&
diff --git a/t/t5703-upload-pack-ref-in-want.sh b/t/t5703-upload-pack-ref-in-want.sh
index a73c55a47e..d1ccc22331 100755
--- a/t/t5703-upload-pack-ref-in-want.sh
+++ b/t/t5703-upload-pack-ref-in-want.sh
@@ -176,7 +176,7 @@ test_expect_success 'setup repos for change-while-negotiating test' '
 		git clone "http://127.0.0.1:$LIB_HTTPD_PORT/smart/repo" "$LOCAL_PRISTINE" &&
 		cd "$LOCAL_PRISTINE" &&
 		git checkout -b side &&
-		for i in $(seq 1 33); do test_commit s$i; done &&
+		for i in $(test_seq 1 33); do test_commit s$i; done &&
 
 		# Add novel commits to upstream
 		git checkout master &&
@@ -289,7 +289,7 @@ test_expect_success 'setup repos for fetching with ref-in-want tests' '
 		git clone "file://$REPO" "$LOCAL_PRISTINE" &&
 		cd "$LOCAL_PRISTINE" &&
 		git checkout -b side &&
-		for i in $(seq 1 33); do test_commit s$i; done &&
+		for i in $(test_seq 1 33); do test_commit s$i; done &&
 
 		# Add novel commits to upstream
 		git checkout master &&
-- 
2.18.0.865.gffc8e1a3cd6
Ævar Arnfjörð Bjarmason· Aug 24, 2018, 15:20 UTC · re: Eric Sunshine · lore

[PATCH v4 3/6] tests: fix comment syntax in chainlint.sed for AIX sed

Change a comment in chainlint.sed to appease AIX sed, which would previously print this error:

    sed:    # stash for later printing is not a recognized function
1. https://public-inbox.org/git/CAPig+cTTbU5HFMKgNyrxTp3+kcK46-Fn=4ZH6zDt1oQChAc3KA@mail.gmail.com/
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/chainlint.sed | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
Show changes to t/chainlint.sed +1 −2
diff --git a/t/chainlint.sed b/t/chainlint.sed
index 8544df38df..dcb4b333ed 100644
--- a/t/chainlint.sed
+++ b/t/chainlint.sed
@@ -131,9 +131,8 @@ b
 b
 
 :subshell
-# bare "(" line?
+# bare "(" line? -- stash for later printing
 /^[ 	]*([	]*$/ {
-	# stash for later printing
 	h
 	bnextline
 }
-- 
2.18.0.865.gffc8e1a3cd6
Eric Sunshine· Aug 24, 2018, 20:52 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH v4 3/6] tests: fix comment syntax in chainlint.sed for AIX sed

On Fri, Aug 24, 2018 at 11:20 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

Show 6 quoted lines
> Change a comment in chainlint.sed to appease AIX sed, which would
> previously print this error:
>
>     sed:    # stash for later printing is not a recognized function
>
> 1. https://public-inbox.org/git/CAPig+cTTbU5HFMKgNyrxTp3+kcK46-Fn=4ZH6zDt1oQChAc3KA@mail.gmail.com/

It would have been nice for the commit message to explain that the problem was caused by the comment being indented, since it's otherwise not otherwise obvious to the reader without chasing the link to the email exchange, but probably not worth a re-roll.

> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
FWIW,
Acked-by: Eric Sunshine <sunshine@sunshineco.com>
Ævar Arnfjörð Bjarmason· Aug 24, 2018, 15:20 UTC · re: Eric Sunshine · lore

[PATCH v4 4/6] tests: use shorter here-docs in chainlint.sed for AIX sed

Improve the portability of chainlint by using shorter here-docs. On AIX sed will complain about:

    sed: 0602-417 The label :hereslurp is greater than eight
    characters

This, in combination with the previous fix to this file makes GIT_TEST_CHAIN_LINT=1 (which is the default) working again on AIX without issues, and the "gmake check-chainlint" test also passes.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/chainlint.sed | 56 ++++++++++++++++++++++++-------------------------
 1 file changed, 28 insertions(+), 28 deletions(-)
Show changes to t/chainlint.sed +28 −28
diff --git a/t/chainlint.sed b/t/chainlint.sed
index dcb4b333ed..c80d2fad7a 100644
--- a/t/chainlint.sed
+++ b/t/chainlint.sed
@@ -97,11 +97,11 @@
 /<<[ 	]*[-\\']*[A-Za-z0-9_]/ {
 	s/^\(.*\)<<[ 	]*[-\\']*\([A-Za-z0-9_][A-Za-z0-9_]*\)'*/<\2>\1<</
 	s/[ 	]*<<//
-	:hereslurp
+	:hered
 	N
 	/^<\([^>]*\)>.*\n[ 	]*\1[ 	]*$/!{
 		s/\n.*$//
-		bhereslurp
+		bhered
 	}
 	s/^<[^>]*>//
 	s/\n.*$//
@@ -149,7 +149,7 @@ s/.*\n//
 
 :slurp
 # incomplete line "...\"
-/\\$/bincomplete
+/\\$/bicmplte
 # multi-line quoted string "...\n..."?
 /"/bdqstring
 # multi-line quoted string '...\n...'? (but not contraction in string "it's")
@@ -171,7 +171,7 @@ s/.*\n//
 	/"[^"]*#[^"]*"/!s/[ 	]#.*$//
 }
 # one-liner "case ... esac"
-/^[ 	]*case[ 	]*..*esac/bcheckchain
+/^[ 	]*case[ 	]*..*esac/bchkchn
 # multi-line "case ... esac"
 /^[ 	]*case[ 	]..*[ 	]in/bcase
 # multi-line "for ... done" or "while ... done"
@@ -200,32 +200,32 @@ s/.*\n//
 /^[ 	]*fi[ 	]*[<>|]/bdone
 /^[ 	]*fi[ 	]*)/bdone
 # nested one-liner "(...) &&"
-/^[ 	]*(.*)[ 	]*&&[ 	]*$/bcheckchain
+/^[ 	]*(.*)[ 	]*&&[ 	]*$/bchkchn
 # nested one-liner "(...)"
-/^[ 	]*(.*)[ 	]*$/bcheckchain
+/^[ 	]*(.*)[ 	]*$/bchkchn
 # nested one-liner "(...) >x" (or "2>x" or "<x" or "|x")
-/^[ 	]*(.*)[ 	]*[0-9]*[<>|]/bcheckchain
+/^[ 	]*(.*)[ 	]*[0-9]*[<>|]/bchkchn
 # nested multi-line "(...\n...)"
 /^[ 	]*(/bnest
 # multi-line "{...\n...}"
 /^[ 	]*{/bblock
 # closing ")" on own line -- exit subshell
-/^[ 	]*)/bclosesolo
+/^[ 	]*)/bclssolo
 # "$((...))" -- arithmetic expansion; not closing ")"
-/\$(([^)][^)]*))[^)]*$/bcheckchain
+/\$(([^)][^)]*))[^)]*$/bchkchn
 # "$(...)" -- command substitution; not closing ")"
-/\$([^)][^)]*)[^)]*$/bcheckchain
+/\$([^)][^)]*)[^)]*$/bchkchn
 # multi-line "$(...\n...)" -- command substitution; treat as nested subshell
 /\$([^)]*$/bnest
 # "=(...)" -- Bash array assignment; not closing ")"
-/=(/bcheckchain
+/=(/bchkchn
 # closing "...) &&"
 /)[ 	]*&&[ 	]*$/bclose
 # closing "...)"
 /)[ 	]*$/bclose
 # closing "...) >x" (or "2>x" or "<x" or "|x")
 /)[ 	]*[<>|]/bclose
-:checkchain
+:chkchn
 # mark suspect if line uses ";" internally rather than "&&" (but not ";" in a
 # string and not ";;" in one-liner "case...esac")
 /;/{
@@ -244,7 +244,7 @@ n
 bslurp
 
 # found incomplete line "...\" -- slurp up next line
-:incomplete
+:icmplte
 N
 s/\\\n//
 bslurp
@@ -282,11 +282,11 @@ bfolded
 :heredoc
 s/^\(.*\)<<[ 	]*[-\\']*\([A-Za-z0-9_][A-Za-z0-9_]*\)'*/<\2>\1<</
 s/[ 	]*<<//
-:hereslurpsub
+:heredsub
 N
 /^<\([^>]*\)>.*\n[ 	]*\1[ 	]*$/!{
 	s/\n.*$//
-	bhereslurpsub
+	bheredsub
 }
 s/^<[^>]*>//
 s/\n.*$//
@@ -316,43 +316,43 @@ x
 # is 'done' or 'fi' cuddled with ")" to close subshell?
 /done.*)/bclose
 /fi.*)/bclose
-bcheckchain
+bchkchn
 
 # found nested multi-line "(...\n...)" -- pass through untouched
 :nest
 x
-:nestslurp
+:nstslurp
 n
 # closing ")" on own line -- stop nested slurp
-/^[ 	]*)/bnestclose
+/^[ 	]*)/bnstclose
 # comment -- not closing ")" if in comment
-/^[ 	]*#/bnestcontinue
+/^[ 	]*#/bnstcnt
 # "$((...))" -- arithmetic expansion; not closing ")"
-/\$(([^)][^)]*))[^)]*$/bnestcontinue
+/\$(([^)][^)]*))[^)]*$/bnstcnt
 # "$(...)" -- command substitution; not closing ")"
-/\$([^)][^)]*)[^)]*$/bnestcontinue
+/\$([^)][^)]*)[^)]*$/bnstcnt
 # closing "...)" -- stop nested slurp
-/)/bnestclose
-:nestcontinue
+/)/bnstclose
+:nstcnt
 x
-bnestslurp
-:nestclose
+bnstslurp
+:nstclose
 s/^/>>/
 # is it "))" which closes nested and parent subshells?
 /)[ 	]*)/bslurp
-bcheckchain
+bchkchn
 
 # found multi-line "{...\n...}" block -- pass through untouched
 :block
 x
 n
 # closing "}" -- stop block slurp
-/}/bcheckchain
+/}/bchkchn
 bblock
 
 # found closing ")" on own line -- drop "suspect" from final line of subshell
 # since that line legitimately lacks "&&" and exit subshell loop
-:closesolo
+:clssolo
 x
 s/?!AMP?!//
 p
-- 
2.18.0.865.gffc8e1a3cd6
Eric Sunshine· Aug 24, 2018, 21:29 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH v4 4/6] tests: use shorter here-docs in chainlint.sed for AIX sed

On Fri, Aug 24, 2018 at 11:20 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

Show 5 quoted lines
> Improve the portability of chainlint by using shorter here-docs. On
> AIX sed will complain about:
>
>     sed: 0602-417 The label :hereslurp is greater than eight
>     characters

Shortening the names makes them ugly and often unreadable. That's not a complaint with this patch; just a general observation regarding 8-byte limitation with this platform's "sed" (and POSIX). See a few suggested improvements below, but probably not worth a re-roll.

Show 5 quoted lines
> This, in combination with the previous fix to this file makes
> GIT_TEST_CHAIN_LINT=1 (which is the default) working again on AIX
> without issues, and the "gmake check-chainlint" test also passes.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
FWIW,
Acked-by: Eric Sunshine <sunshine@sunshineco.com>
> diff --git a/t/chainlint.sed b/t/chainlint.sed
> -:checkchain
> +:chkchn
":chkchain"
>  # found incomplete line "...\" -- slurp up next line
> -:incomplete
> +:icmplte
":fold" (for "fold out NL")
>  # found nested multi-line "(...\n...)" -- pass through untouched
> -:nestslurp
> +:nstslurp
":nesteat"
> -:nestcontinue
> +:nstcnt
":nestcont"
> -:nestclose
> +:nstclose
":nestend" or ":endnest"
>  # found closing ")" on own line -- drop "suspect" from final line of subshell
>  # since that line legitimately lacks "&&" and exit subshell loop
> -:closesolo
> +:clssolo
":endsolo"
Ævar Arnfjörð Bjarmason· Aug 28, 2018, 20:14 UTC · re: Eric Sunshine · lore

Re: [PATCH v4 4/6] tests: use shorter here-docs in chainlint.sed for AIX sed

On Fri, Aug 24 2018, Eric Sunshine wrote:
Show 56 quoted lines
> On Fri, Aug 24, 2018 at 11:20 AM Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
>> Improve the portability of chainlint by using shorter here-docs. On
>> AIX sed will complain about:
>>
>>     sed: 0602-417 The label :hereslurp is greater than eight
>>     characters
>
> Shortening the names makes them ugly and often unreadable. That's not
> a complaint with this patch; just a general observation regarding
> 8-byte limitation with this platform's "sed" (and POSIX). See a few
> suggested improvements below, but probably not worth a re-roll.
>
>> This, in combination with the previous fix to this file makes
>> GIT_TEST_CHAIN_LINT=1 (which is the default) working again on AIX
>> without issues, and the "gmake check-chainlint" test also passes.
>>
>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>
> FWIW,
> Acked-by: Eric Sunshine <sunshine@sunshineco.com>
>
>> diff --git a/t/chainlint.sed b/t/chainlint.sed
>> -:checkchain
>> +:chkchn
>
> ":chkchain"
>
>>  # found incomplete line "...\" -- slurp up next line
>> -:incomplete
>> +:icmplte
>
> ":fold" (for "fold out NL")
>
>>  # found nested multi-line "(...\n...)" -- pass through untouched
>> -:nestslurp
>> +:nstslurp
>
> ":nesteat"
>
>> -:nestcontinue
>> +:nstcnt
>
> ":nestcont"
>
>> -:nestclose
>> +:nstclose
>
> ":nestend" or ":endnest"
>
>>  # found closing ")" on own line -- drop "suspect" from final line of subshell
>>  # since that line legitimately lacks "&&" and exit subshell loop
>> -:closesolo
>> +:clssolo
>
> ":endsolo"

I was meaning to get to this with a re-roll, but since this is already in next & these label renames seem cosmetic, it seems better to do this after 2.19, i.e. the compiler doesn't care about the specific names, but shortening them to <=8 fixes the bug.

Eric Sunshine· Aug 28, 2018, 20:17 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH v4 4/6] tests: use shorter here-docs in chainlint.sed for AIX sed

On Tue, Aug 28, 2018 at 4:14 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

Show 12 quoted lines
> On Fri, Aug 24 2018, Eric Sunshine wrote:
> > Shortening the names makes them ugly and often unreadable. That's not
> > a complaint with this patch; just a general observation regarding
> > 8-byte limitation with this platform's "sed" (and POSIX). See a few
> > suggested improvements below, but probably not worth a re-roll.
> >> +:clssolo
> > ":endsolo"
>
> I was meaning to get to this with a re-roll, but since this is already
> in next & these label renames seem cosmetic, it seems better to do this
> after 2.19, i.e. the compiler doesn't care about the specific names, but
> shortening them to <=8 fixes the bug.
Sounds fine. There's no rush. Thanks.
Junio C Hamano· Aug 27, 2018, 19:36 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH v4 4/6] tests: use shorter here-docs in chainlint.sed for AIX sed

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
Show 5 quoted lines
> Improve the portability of chainlint by using shorter here-docs. On
> AIX sed will complain about:
>
>     sed: 0602-417 The label :hereslurp is greater than eight
>     characters

Remind me again not to forget doing s/here-doc/label/ on this patch before queueing. Other than that, looks good, together with 3/6 for the the indented comment issue.

Ævar Arnfjörð Bjarmason· Aug 24, 2018, 15:20 UTC · re: Eric Sunshine · lore

[PATCH v4 5/6] tests: fix version-specific portability issue in Perl JSON

The test guarded by PERLJSON added in 75459410ed ("json_writer: new routines to create JSON data", 2018-07-13) assumed that a JSON boolean value like "true" or "false" would be represented as "1" or "0" in Perl.

This behavior can't be relied upon, e.g. with JSON.pm 2.50 and JSON::PP A JSON::PP::Boolean object will be represented as "true" or "false". To work around this let's check if we have any refs left after we check for hashes and arrays, assume those are JSON objects, and coerce them to a known boolean value.

The behavior of this test still looks odd to me. Why implement our own ad-hoc encoder just for some one-off test, as opposed to say Perl's own Data::Dumper with Sortkeys et al? But with this change it works, so let's leave it be.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t0019/parse_json.perl | 3 +++
 1 file changed, 3 insertions(+)
Show changes to t/t0019/parse_json.perl +3 −0
diff --git a/t/t0019/parse_json.perl b/t/t0019/parse_json.perl
index ca4e5bfa78..fea87fb81b 100644
--- a/t/t0019/parse_json.perl
+++ b/t/t0019/parse_json.perl
@@ -34,6 +34,9 @@ sub dump_item {
     } elsif (ref($value) eq 'HASH') {
 	print "$label_in hash\n";
 	dump_hash($label_in, $value);
+    } elsif (ref $value) {
+	my $bool = $value ? 1 : 0;
+	print "$label_in $bool\n";
     } elsif (defined $value) {
 	print "$label_in $value\n";
     } else {
-- 
2.18.0.865.gffc8e1a3cd6
Eric Sunshine· Aug 24, 2018, 20:41 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH v4 5/6] tests: fix version-specific portability issue in Perl JSON

On Fri, Aug 24, 2018 at 11:20 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

Show 7 quoted lines
> The test guarded by PERLJSON added in 75459410ed ("json_writer: new
> routines to create JSON data", 2018-07-13) assumed that a JSON boolean
> value like "true" or "false" would be represented as "1" or "0" in
> Perl.
>
> This behavior can't be relied upon, e.g. with JSON.pm 2.50 and
> JSON::PP A JSON::PP::Boolean object will be represented as "true" or
s/PP A/PP. A/
(Not worth a re-roll.)
Show 10 quoted lines
> "false". To work around this let's check if we have any refs left
> after we check for hashes and arrays, assume those are JSON objects,
> and coerce them to a known boolean value.
>
> The behavior of this test still looks odd to me. Why implement our own
> ad-hoc encoder just for some one-off test, as opposed to say Perl's
> own Data::Dumper with Sortkeys et al? But with this change it works,
> so let's leave it be.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Ævar Arnfjörð Bjarmason· Aug 24, 2018, 15:20 UTC · re: Eric Sunshine · lore

[PATCH v4 6/6] tests: fix and add lint for non-portable grep --file

The --file option to grep isn't in POSIX[1], but -f is[1]. Let's check for that in the future, and fix the portability regression in f237c8b6fe ("commit-graph: implement git-commit-graph write", 2018-04-02) that broke e.g. AIX.

1. http://pubs.opengroup.org/onlinepubs/009695399/utilities/grep.html
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/check-non-portable-shell.pl | 1 +
 t/t5318-commit-graph.sh       | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)
Show changes to 2 files +2 −1

t/check-non-portable-shell.pl, t/t5318-commit-graph.sh

diff --git a/t/check-non-portable-shell.pl b/t/check-non-portable-shell.pl
index 75f38298d7..b45bdac688 100755
--- a/t/check-non-portable-shell.pl
+++ b/t/check-non-portable-shell.pl
@@ -43,6 +43,7 @@ sub err {
 	/\bwc -l.*"\s*=/ and err '`"$(wc -l)"` is not portable (use test_line_count)';
 	/\bhead\s+-c\b/ and err 'head -c is not portable (use test_copy_bytes BYTES <file >out)';
 	/(?:\$\(seq|^\s*seq\b)/ and err 'seq is not portable (use test_seq)';
+	/\bgrep\b.*--file\b/ and err 'grep --file FILE is not portable (use grep -f FILE)';
 	/\bexport\s+[A-Za-z0-9_]*=/ and err '"export FOO=bar" is not portable (use FOO=bar && export FOO)';
 	/^\s*([A-Z0-9_]+=(\w+|(["']).*?\3)\s+)+(\w+)/ and exists($func{$4}) and
 		err '"FOO=bar shell_func" assignment extends beyond "shell_func"';
diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
index 3c1ffad491..0c500f7ca2 100755
--- a/t/t5318-commit-graph.sh
+++ b/t/t5318-commit-graph.sh
@@ -134,7 +134,7 @@ test_expect_success 'Add one more commit' '
 	git branch commits/8 &&
 	ls $objdir/pack | grep idx >existing-idx &&
 	git repack &&
-	ls $objdir/pack| grep idx | grep -v --file=existing-idx >new-idx
+	ls $objdir/pack| grep idx | grep -v -f existing-idx >new-idx
 '
 
 # Current graph structure:
-- 
2.18.0.865.gffc8e1a3cd6
Ævar Arnfjörð Bjarmason· Aug 23, 2018, 20:36 UTC · re: Ævar Arnfjörð Bjarmason · lore

[PATCH v3 4/5] tests: fix version-specific portability issue in Perl JSON

The test guarded by PERLJSON added in 75459410ed ("json_writer: new routines to create JSON data", 2018-07-13) assumed that a JSON boolean value like "true" or "false" would be represented as "1" or "0" in Perl.

This behavior can't be relied upon, e.g. with JSON.pm 2.50 and JSON::PP A JSON::PP::Boolean object will be represented as "true" or "false". To work around this let's check if we have any refs left after we check for hashes and arrays, assume those are JSON objects, and coerce them to a known boolean value.

The behavior of this test still looks odd to me. Why implement our own ad-hoc encoder just for some one-off test, as opposed to say Perl's own Data::Dumper with Sortkeys et al? But with this change it works, so let's leave it be.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t0019/parse_json.perl | 3 +++
 1 file changed, 3 insertions(+)
Show changes to t/t0019/parse_json.perl +3 −0
diff --git a/t/t0019/parse_json.perl b/t/t0019/parse_json.perl
index ca4e5bfa78..fea87fb81b 100644
--- a/t/t0019/parse_json.perl
+++ b/t/t0019/parse_json.perl
@@ -34,6 +34,9 @@ sub dump_item {
     } elsif (ref($value) eq 'HASH') {
 	print "$label_in hash\n";
 	dump_hash($label_in, $value);
+    } elsif (ref $value) {
+	my $bool = $value ? 1 : 0;
+	print "$label_in $bool\n";
     } elsif (defined $value) {
 	print "$label_in $value\n";
     } else {
-- 
2.18.0.865.gffc8e1a3cd6
Ævar Arnfjörð Bjarmason· Aug 23, 2018, 20:36 UTC · re: Ævar Arnfjörð Bjarmason · lore

[PATCH v3 5/5] tests: fix and add lint for non-portable grep --file

The --file option to grep isn't in POSIX[1], but -f is[1]. Let's check for that in the future, and fix the portability regression in f237c8b6fe ("commit-graph: implement git-commit-graph write", 2018-04-02) that broke e.g. AIX.

1. http://pubs.opengroup.org/onlinepubs/009695399/utilities/grep.html
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/check-non-portable-shell.pl | 1 +
 t/t5318-commit-graph.sh       | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)
Show changes to 2 files +2 −1

t/check-non-portable-shell.pl, t/t5318-commit-graph.sh

diff --git a/t/check-non-portable-shell.pl b/t/check-non-portable-shell.pl
index 75f38298d7..b45bdac688 100755
--- a/t/check-non-portable-shell.pl
+++ b/t/check-non-portable-shell.pl
@@ -43,6 +43,7 @@ sub err {
 	/\bwc -l.*"\s*=/ and err '`"$(wc -l)"` is not portable (use test_line_count)';
 	/\bhead\s+-c\b/ and err 'head -c is not portable (use test_copy_bytes BYTES <file >out)';
 	/(?:\$\(seq|^\s*seq\b)/ and err 'seq is not portable (use test_seq)';
+	/\bgrep\b.*--file\b/ and err 'grep --file FILE is not portable (use grep -f FILE)';
 	/\bexport\s+[A-Za-z0-9_]*=/ and err '"export FOO=bar" is not portable (use FOO=bar && export FOO)';
 	/^\s*([A-Z0-9_]+=(\w+|(["']).*?\3)\s+)+(\w+)/ and exists($func{$4}) and
 		err '"FOO=bar shell_func" assignment extends beyond "shell_func"';
diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
index 3c1ffad491..0c500f7ca2 100755
--- a/t/t5318-commit-graph.sh
+++ b/t/t5318-commit-graph.sh
@@ -134,7 +134,7 @@ test_expect_success 'Add one more commit' '
 	git branch commits/8 &&
 	ls $objdir/pack | grep idx >existing-idx &&
 	git repack &&
-	ls $objdir/pack| grep idx | grep -v --file=existing-idx >new-idx
+	ls $objdir/pack| grep idx | grep -v -f existing-idx >new-idx
 '
 
 # Current graph structure:
-- 
2.18.0.865.gffc8e1a3cd6
Junio C Hamano· Aug 23, 2018, 20:44 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH v3 5/5] tests: fix and add lint for non-portable grep --file

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
Show 9 quoted lines
> The --file option to grep isn't in POSIX[1], but -f is[1]. Let's check
> for that in the future, and fix the portability regression in
> f237c8b6fe ("commit-graph: implement git-commit-graph write",
> 2018-04-02) that broke e.g. AIX.
>
> 1. http://pubs.opengroup.org/onlinepubs/009695399/utilities/grep.html
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
Thanks.
Show 29 quoted lines
>  t/check-non-portable-shell.pl | 1 +
>  t/t5318-commit-graph.sh       | 2 +-
>  2 files changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/t/check-non-portable-shell.pl b/t/check-non-portable-shell.pl
> index 75f38298d7..b45bdac688 100755
> --- a/t/check-non-portable-shell.pl
> +++ b/t/check-non-portable-shell.pl
> @@ -43,6 +43,7 @@ sub err {
>  	/\bwc -l.*"\s*=/ and err '`"$(wc -l)"` is not portable (use test_line_count)';
>  	/\bhead\s+-c\b/ and err 'head -c is not portable (use test_copy_bytes BYTES <file >out)';
>  	/(?:\$\(seq|^\s*seq\b)/ and err 'seq is not portable (use test_seq)';
> +	/\bgrep\b.*--file\b/ and err 'grep --file FILE is not portable (use grep -f FILE)';
>  	/\bexport\s+[A-Za-z0-9_]*=/ and err '"export FOO=bar" is not portable (use FOO=bar && export FOO)';
>  	/^\s*([A-Z0-9_]+=(\w+|(["']).*?\3)\s+)+(\w+)/ and exists($func{$4}) and
>  		err '"FOO=bar shell_func" assignment extends beyond "shell_func"';
> diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
> index 3c1ffad491..0c500f7ca2 100755
> --- a/t/t5318-commit-graph.sh
> +++ b/t/t5318-commit-graph.sh
> @@ -134,7 +134,7 @@ test_expect_success 'Add one more commit' '
>  	git branch commits/8 &&
>  	ls $objdir/pack | grep idx >existing-idx &&
>  	git repack &&
> -	ls $objdir/pack| grep idx | grep -v --file=existing-idx >new-idx
> +	ls $objdir/pack| grep idx | grep -v -f existing-idx >new-idx
>  '
>  
>  # Current graph structure:
Derrick Stolee· Aug 24, 2018, 13:49 UTC · re: Junio C Hamano · lore

Re: [PATCH v3 5/5] tests: fix and add lint for non-portable grep --file

On 8/23/2018 4:44 PM, Junio C Hamano wrote:
Show 12 quoted lines
> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> The --file option to grep isn't in POSIX[1], but -f is[1]. Let's check
>> for that in the future, and fix the portability regression in
>> f237c8b6fe ("commit-graph: implement git-commit-graph write",
>> 2018-04-02) that broke e.g. AIX.
>>
>> 1. http://pubs.opengroup.org/onlinepubs/009695399/utilities/grep.html
>>
>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> ---
> Thanks.
Yes, thanks! I'll keep this in mind for the future.
Show 29 quoted lines
>>   t/check-non-portable-shell.pl | 1 +
>>   t/t5318-commit-graph.sh       | 2 +-
>>   2 files changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/t/check-non-portable-shell.pl b/t/check-non-portable-shell.pl
>> index 75f38298d7..b45bdac688 100755
>> --- a/t/check-non-portable-shell.pl
>> +++ b/t/check-non-portable-shell.pl
>> @@ -43,6 +43,7 @@ sub err {
>>   	/\bwc -l.*"\s*=/ and err '`"$(wc -l)"` is not portable (use test_line_count)';
>>   	/\bhead\s+-c\b/ and err 'head -c is not portable (use test_copy_bytes BYTES <file >out)';
>>   	/(?:\$\(seq|^\s*seq\b)/ and err 'seq is not portable (use test_seq)';
>> +	/\bgrep\b.*--file\b/ and err 'grep --file FILE is not portable (use grep -f FILE)';
>>   	/\bexport\s+[A-Za-z0-9_]*=/ and err '"export FOO=bar" is not portable (use FOO=bar && export FOO)';
>>   	/^\s*([A-Z0-9_]+=(\w+|(["']).*?\3)\s+)+(\w+)/ and exists($func{$4}) and
>>   		err '"FOO=bar shell_func" assignment extends beyond "shell_func"';
>> diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
>> index 3c1ffad491..0c500f7ca2 100755
>> --- a/t/t5318-commit-graph.sh
>> +++ b/t/t5318-commit-graph.sh
>> @@ -134,7 +134,7 @@ test_expect_success 'Add one more commit' '
>>   	git branch commits/8 &&
>>   	ls $objdir/pack | grep idx >existing-idx &&
>>   	git repack &&
>> -	ls $objdir/pack| grep idx | grep -v --file=existing-idx >new-idx
>> +	ls $objdir/pack| grep idx | grep -v -f existing-idx >new-idx
>>   '
>>   
>>   # Current graph structure:
Junio C Hamano· Aug 23, 2018, 15:42 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH] tests: fix and add lint for non-portable head -c N

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
> Junio: Even though this isn't a 2.19.0-rc0 regression I think it makes
> sense for 2.19.0. The fix is trivial, and it'll unbreak (at least some
> of) the tests on stock git on OpenBSD.

I would have been a bit more sympathetic if this were recent regression (say, 2.18 or so), even if it is not new in this cycle, and the patch applied to the target track cleanly (say, maint or maint-2.17), but seeing that some of these are quite old, dating back to 2009, I am not sure I am enthused.

The changes certainly look trivial.  Let's apply.
Thanks.
Show 58 quoted lines
>
> OpenBSD guys: If you CC the git mailing list when you find you need to
> apply patches like these, we're happy to fix this more pro-actively. I
> just happened to be testing the upcoming 2.19.0 on OpenBSD and spotted
> this.
>
>  t/check-non-portable-shell.pl | 1 +
>  t/t5310-pack-bitmaps.sh       | 2 +-
>  t/test-lib.sh                 | 4 ++--
>  3 files changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/t/check-non-portable-shell.pl b/t/check-non-portable-shell.pl
> index d5823f71d8..94a7e6165e 100755
> --- a/t/check-non-portable-shell.pl
> +++ b/t/check-non-portable-shell.pl
> @@ -35,6 +35,7 @@ sub err {
>  		chomp;
>  	}
>  
> +	/\bhead\s+-c\b/ and err 'head -c is not portable (use test_copy_bytes BYTES <file >out)';
>  	/\bsed\s+-i/ and err 'sed -i is not portable';
>  	/\becho\s+-[neE]/ and err 'echo with option is not portable (use printf)';
>  	/^\s*declare\s+/ and err 'arrays/declare not portable';
> diff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh
> index 557bd0d0c0..7bff7923f2 100755
> --- a/t/t5310-pack-bitmaps.sh
> +++ b/t/t5310-pack-bitmaps.sh
> @@ -335,7 +335,7 @@ test_expect_success 'truncated bitmap fails gracefully' '
>  	git rev-list --use-bitmap-index --count --all >expect &&
>  	bitmap=$(ls .git/objects/pack/*.bitmap) &&
>  	test_when_finished "rm -f $bitmap" &&
> -	head -c 512 <$bitmap >$bitmap.tmp &&
> +	test_copy_bytes 512 <$bitmap >$bitmap.tmp &&
>  	mv -f $bitmap.tmp $bitmap &&
>  	git rev-list --use-bitmap-index --count --all >actual 2>stderr &&
>  	test_cmp expect actual &&
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 8bb0f4348e..44288cbb59 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -867,7 +867,7 @@ then
>  		# handle only executables, unless they are shell libraries that
>  		# need to be in the exec-path.
>  		test -x "$1" ||
> -		test "# " = "$(head -c 2 <"$1")" ||
> +		test "# " = "$(test_copy_bytes 2 <"$1")" ||
>  		return;
>  
>  		base=$(basename "$1")
> @@ -882,7 +882,7 @@ then
>  		# do not override scripts
>  		if test -x "$symlink_target" &&
>  		    test ! -d "$symlink_target" &&
> -		    test "#!" != "$(head -c 2 < "$symlink_target")"
> +		    test "#!" != "$(test_copy_bytes 2 <"$symlink_target")"
>  		then
>  			symlink_target=../valgrind.sh
>  		fi
Ævar Arnfjörð Bjarmason· Aug 23, 2018, 17:24 UTC · re: Junio C Hamano · lore

Re: [PATCH] tests: fix and add lint for non-portable head -c N

On Thu, Aug 23 2018, Junio C Hamano wrote:
Show 10 quoted lines
> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> Junio: Even though this isn't a 2.19.0-rc0 regression I think it makes
>> sense for 2.19.0. The fix is trivial, and it'll unbreak (at least some
>> of) the tests on stock git on OpenBSD.
>
> I would have been a bit more sympathetic if this were recent
> regression (say, 2.18 or so), even if it is not new in this cycle,
> and the patch applied to the target track cleanly (say, maint or
> maint-2.17),
Yeah it's not a big deal if it's not in 2.19.
> but seeing that some of these are quite old, dating back to 2009, I am
> not sure I am enthused.

Note that those parts are the "while I'm at it" valgrind-specific parts, i.e. parts of the code that likely never ran on anything except a GNU toolchain.

Show 62 quoted lines
> The changes certainly look trivial.  Let's apply.
>
> Thanks.
>
>>
>> OpenBSD guys: If you CC the git mailing list when you find you need to
>> apply patches like these, we're happy to fix this more pro-actively. I
>> just happened to be testing the upcoming 2.19.0 on OpenBSD and spotted
>> this.
>>
>>  t/check-non-portable-shell.pl | 1 +
>>  t/t5310-pack-bitmaps.sh       | 2 +-
>>  t/test-lib.sh                 | 4 ++--
>>  3 files changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/t/check-non-portable-shell.pl b/t/check-non-portable-shell.pl
>> index d5823f71d8..94a7e6165e 100755
>> --- a/t/check-non-portable-shell.pl
>> +++ b/t/check-non-portable-shell.pl
>> @@ -35,6 +35,7 @@ sub err {
>>  		chomp;
>>  	}
>>
>> +	/\bhead\s+-c\b/ and err 'head -c is not portable (use test_copy_bytes BYTES <file >out)';
>>  	/\bsed\s+-i/ and err 'sed -i is not portable';
>>  	/\becho\s+-[neE]/ and err 'echo with option is not portable (use printf)';
>>  	/^\s*declare\s+/ and err 'arrays/declare not portable';
>> diff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh
>> index 557bd0d0c0..7bff7923f2 100755
>> --- a/t/t5310-pack-bitmaps.sh
>> +++ b/t/t5310-pack-bitmaps.sh
>> @@ -335,7 +335,7 @@ test_expect_success 'truncated bitmap fails gracefully' '
>>  	git rev-list --use-bitmap-index --count --all >expect &&
>>  	bitmap=$(ls .git/objects/pack/*.bitmap) &&
>>  	test_when_finished "rm -f $bitmap" &&
>> -	head -c 512 <$bitmap >$bitmap.tmp &&
>> +	test_copy_bytes 512 <$bitmap >$bitmap.tmp &&
>>  	mv -f $bitmap.tmp $bitmap &&
>>  	git rev-list --use-bitmap-index --count --all >actual 2>stderr &&
>>  	test_cmp expect actual &&
>> diff --git a/t/test-lib.sh b/t/test-lib.sh
>> index 8bb0f4348e..44288cbb59 100644
>> --- a/t/test-lib.sh
>> +++ b/t/test-lib.sh
>> @@ -867,7 +867,7 @@ then
>>  		# handle only executables, unless they are shell libraries that
>>  		# need to be in the exec-path.
>>  		test -x "$1" ||
>> -		test "# " = "$(head -c 2 <"$1")" ||
>> +		test "# " = "$(test_copy_bytes 2 <"$1")" ||
>>  		return;
>>
>>  		base=$(basename "$1")
>> @@ -882,7 +882,7 @@ then
>>  		# do not override scripts
>>  		if test -x "$symlink_target" &&
>>  		    test ! -d "$symlink_target" &&
>> -		    test "#!" != "$(head -c 2 < "$symlink_target")"
>> +		    test "#!" != "$(test_copy_bytes 2 <"$symlink_target")"
>>  		then
>>  			symlink_target=../valgrind.sh
>>  		fi

← back to recent threads