[PATCH] 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 <redacted>
---

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

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

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 [off-list ref]
     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

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

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

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

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

On Thu, Aug 23, 2018 at 03:25:01PM +0000, Ævar Arnfjörð Bjarmason wrote:

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

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.

Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
 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

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

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

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 <redacted>
---
 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)';
 	/\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

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 <redacted>
---
 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.*$//

-- 
2.18.0.865.gffc8e1a3cd6

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 <redacted>
---
 t/t0019/parse_json.perl | 3 +++
 1 file changed, 3 insertions(+)

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

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

-- 
2.18.0.865.gffc8e1a3cd6

On Thu, Aug 23, 2018 at 4:36 PM Ævar Arnfjörð Bjarmason
[off-list ref] 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/

On Thu, Aug 23 2018, Eric Sunshine wrote:

On Thu, Aug 23, 2018 at 4:36 PM Ævar Arnfjörð Bjarmason
[off-list ref] wrote:

quoted

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

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

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 <redacted>
---
 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)';
 	/\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

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 <redacted>
---
 t/chainlint.sed | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

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

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 <redacted>
---
 t/chainlint.sed | 56 ++++++++++++++++++++++++-------------------------
 1 file changed, 28 insertions(+), 28 deletions(-)

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

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 <redacted>
---
 t/t0019/parse_json.perl | 3 +++
 1 file changed, 3 insertions(+)

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

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

-- 
2.18.0.865.gffc8e1a3cd6

On Fri, Aug 24, 2018 at 11:20 AM Ævar Arnfjörð Bjarmason
[off-list ref] wrote:

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

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

On Fri, Aug 24, 2018 at 11:20 AM Ævar Arnfjörð Bjarmason
[off-list ref] wrote:

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

FWIW,
Acked-by: Eric Sunshine <redacted>

On Fri, Aug 24, 2018 at 11:20 AM Ævar Arnfjörð Bjarmason
[off-list ref] 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 <redacted>

FWIW,
Acked-by: Eric Sunshine <redacted>

":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  [off-list ref] writes:

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.

On Fri, Aug 24 2018, Eric Sunshine wrote:

On Fri, Aug 24, 2018 at 11:20 AM Ævar Arnfjörð Bjarmason
[off-list ref] wrote:

quoted

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.

quoted

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

FWIW,
Acked-by: Eric Sunshine <redacted>

quoted

diff --git a/t/chainlint.sed b/t/chainlint.sed
-:checkchain
+:chkchn

":chkchain"

quoted

 # found incomplete line "...\" -- slurp up next line
-:incomplete
+:icmplte

":fold" (for "fold out NL")

quoted

 # found nested multi-line "(...\n...)" -- pass through untouched
-:nestslurp
+:nstslurp

":nesteat"

quoted

-:nestcontinue
+:nstcnt

":nestcont"

quoted

-:nestclose
+:nstclose

":nestend" or ":endnest"

quoted

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

On Tue, Aug 28, 2018 at 4:14 PM Ævar Arnfjörð Bjarmason
[off-list ref] wrote:

On Fri, Aug 24 2018, Eric Sunshine wrote:

quoted

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.

quoted

+: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.