{"thread":{"id":"49242","subject":"[PATCH 0/2] FreeBSD & AIX test portability fixes","startedAt":"2018-08-28T19:38:41Z","lastAt":"2018-08-29T20:23:05Z","messageCount":6,"participants":["Ævar Arnfjörð Bjarmason","Junio C Hamano","Jonathan Nieder","Ann T Ropea"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"356724","messageId":"20180828193827.8648-1-avarab@gmail.com","threadId":"49242","inReplyTo":null,"subject":"[PATCH 0/2] FreeBSD & AIX test portability fixes","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-08-28T19:38:25Z","receivedAt":"2018-08-28T19:38:41Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"This makes the vanilla test suite pass without errors on the FreeBSD\nsystem noted in the commit messages.\n\nOn AIX things are still horribly broken, but this fixes one more issue\nthat also affects that system.\n\nÆvar Arnfjörð Bjarmason (2):\n  tests: fix non-portable \"${var:-\"str\"}\" construct\n  tests: fix non-portable iconv invocation\n\n t/t0028-working-tree-encoding.sh | 6 +++++-\n t/t4013-diff-various.sh          | 2 +-\n 2 files changed, 6 insertions(+), 2 deletions(-)\n\n-- \n2.19.0.rc0.228.g281dcd1b4d0\n\n"},{"id":"356725","messageId":"20180828193827.8648-2-avarab@gmail.com","threadId":"49242","inReplyTo":"20180828193827.8648-1-avarab@gmail.com","subject":"[PATCH 1/2] tests: fix non-portable \"${var:-\"str\"}\" construct","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-08-28T19:38:26Z","receivedAt":"2018-08-28T19:38:42Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On both AIX 7200-00-01-1543 and FreeBSD 11.2-RELEASE-p2 the\n\"${var:-\"str\"}\" syntax means something different than what it does\nunder the bash or dash shells.\n\nBoth will consider the start of the new unescaped quotes to be a new\nargument to test_expect_success, resulting in the following error:\n\n    error: bug in the test script: 'git diff-tree initial # magic\n    is (not' does not look like a prereq\n\nFix this by removing the redundant quotes. There's no need for them,\nand the resulting code works under all the aforementioned shells. This\nfixes a regression in c2f1d3989 (\"t4013: test new output from diff\n--abbrev --raw\", 2017-12-03) first released with Git v2.16.0.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n t/t4013-diff-various.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh\nindex f8d853595b..73f7038253 100755\n--- a/t/t4013-diff-various.sh\n+++ b/t/t4013-diff-various.sh\n@@ -140,7 +140,7 @@ do\n \texpect=\"$TEST_DIRECTORY/t4013/diff.$test\"\n \tactual=\"$pfx-diff.$test\"\n \n-\ttest_expect_success \"git $cmd # magic is ${magic:-\"(not used)\"}\" '\n+\ttest_expect_success \"git $cmd # magic is ${magic:-(not used)}\" '\n \t\t{\n \t\t\techo \"$ git $cmd\"\n \t\t\tcase \"$magic\" in\n-- \n2.19.0.rc0.228.g281dcd1b4d0\n\n"},{"id":"356726","messageId":"20180828193827.8648-3-avarab@gmail.com","threadId":"49242","inReplyTo":"20180828193827.8648-1-avarab@gmail.com","subject":"[PATCH 2/2] tests: fix non-portable iconv invocation","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-08-28T19:38:27Z","receivedAt":"2018-08-28T19:38:44Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"The iconv that comes with a FreeBSD 11.2-RELEASE-p2 box I have access\nto doesn't support the SHIFT-JIS encoding. Guard a test added in\ne92d62253 (\"convert: add round trip check based on\n'core.checkRoundtripEncoding'\", 2018-04-15) first released with Git\nv2.18.0 with a prerequisite that checks for its availability.\n\nThe iconv command is in POSIX, and we have numerous tests\nunconditionally relying on its ability to convert ASCII, UTF-8 and\nUTF-16, but unconditionally relying on the presence of more obscure\nencodings isn't portable.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n t/t0028-working-tree-encoding.sh | 6 +++++-\n 1 file changed, 5 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t0028-working-tree-encoding.sh b/t/t0028-working-tree-encoding.sh\nindex 12b8eb963a..b0f4490e8e 100755\n--- a/t/t0028-working-tree-encoding.sh\n+++ b/t/t0028-working-tree-encoding.sh\n@@ -203,7 +203,11 @@ test_expect_success 'error if encoding garbage is already in Git' '\n \ttest_i18ngrep \"error: BOM is required\" err.out\n '\n \n-test_expect_success 'check roundtrip encoding' '\n+test_lazy_prereq ICONV_SHIFT_JIS '\n+\ticonv -f UTF-8 -t SHIFT-JIS </dev/null 2>/dev/null\n+'\n+\n+test_expect_success ICONV_SHIFT_JIS 'check roundtrip encoding' '\n \ttest_when_finished \"rm -f roundtrip.shift roundtrip.utf16\" &&\n \ttest_when_finished \"git reset --hard HEAD\" &&\n \n-- \n2.19.0.rc0.228.g281dcd1b4d0\n\n"},{"id":"356728","messageId":"xmqqlg8qff24.fsf@gitster-ct.c.googlers.com","threadId":"49242","inReplyTo":"20180828193827.8648-2-avarab@gmail.com","subject":"Re: [PATCH 1/2] tests: fix non-portable \"${var:-\"str\"}\" construct","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-28T19:48:19Z","receivedAt":"2018-08-28T19:48:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:\n\n> On both AIX 7200-00-01-1543 and FreeBSD 11.2-RELEASE-p2 the\n> \"${var:-\"str\"}\" syntax means something different than what it does\n> under the bash or dash shells.\n>\n> Both will consider the start of the new unescaped quotes to be a new\n> argument to test_expect_success, resulting in the following error:\n>\n>     error: bug in the test script: 'git diff-tree initial # magic\n>     is (not' does not look like a prereq\n>\n> Fix this by removing the redundant quotes. There's no need for them,\n> and the resulting code works under all the aforementioned shells.\n\nYup, there is no need for that inner dq pair in this particular case.\n\nI was more worried about scripted Porcelains, like \n\n   : \"${GIT_OBJECT_DIRECTORY=\"$(git rev-parse --git-path objects)\"}\"\n\nwhich I think can safely lose the outer dq pair.  In t/ directory,\nthere is this one in test-lib-functions.sh which I do not offhand\nknow how these problematic shells would handle.\n\n\techo \"#!${2-\"$SHELL_PATH\"}\" &&\n\nOther than the presence of these two that are not covered by this\npatch, the patch itself looks good.  Thanks.\n\n> fixes a regression in c2f1d3989 (\"t4013: test new output from diff\n> --abbrev --raw\", 2017-12-03) first released with Git v2.16.0.\n>\n> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n> ---\n>  t/t4013-diff-various.sh | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh\n> index f8d853595b..73f7038253 100755\n> --- a/t/t4013-diff-various.sh\n> +++ b/t/t4013-diff-various.sh\n> @@ -140,7 +140,7 @@ do\n>  \texpect=\"$TEST_DIRECTORY/t4013/diff.$test\"\n>  \tactual=\"$pfx-diff.$test\"\n>  \n> -\ttest_expect_success \"git $cmd # magic is ${magic:-\"(not used)\"}\" '\n> +\ttest_expect_success \"git $cmd # magic is ${magic:-(not used)}\" '\n>  \t\t{\n>  \t\t\techo \"$ git $cmd\"\n>  \t\t\tcase \"$magic\" in\n"},{"id":"356778","messageId":"20180828225512.GB74687@aiede.svl.corp.google.com","threadId":"49242","inReplyTo":"20180828193827.8648-3-avarab@gmail.com","subject":"Re: [PATCH 2/2] tests: fix non-portable iconv invocation","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2018-08-28T22:55:12Z","receivedAt":"2018-08-28T22:55:17Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nÆvar Arnfjörð Bjarmason wrote:\n\n> The iconv that comes with a FreeBSD 11.2-RELEASE-p2 box I have access\n> to doesn't support the SHIFT-JIS encoding. Guard a test added in\n> e92d62253 (\"convert: add round trip check based on\n> 'core.checkRoundtripEncoding'\", 2018-04-15) first released with Git\n> v2.18.0 with a prerequisite that checks for its availability.\n>\n> The iconv command is in POSIX, and we have numerous tests\n> unconditionally relying on its ability to convert ASCII, UTF-8 and\n> UTF-16, but unconditionally relying on the presence of more obscure\n> encodings isn't portable.\n>\n> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n> ---\n>  t/t0028-working-tree-encoding.sh | 6 +++++-\n>  1 file changed, 5 insertions(+), 1 deletion(-)\n\nOne nit:\n\n[...]\n> diff --git a/t/t0028-working-tree-encoding.sh b/t/t0028-working-tree-encoding.sh\n> index 12b8eb963a..b0f4490e8e 100755\n> --- a/t/t0028-working-tree-encoding.sh\n> +++ b/t/t0028-working-tree-encoding.sh\n> @@ -203,7 +203,11 @@ test_expect_success 'error if encoding garbage is already in Git' '\n>  \ttest_i18ngrep \"error: BOM is required\" err.out\n>  '\n>  \n> -test_expect_success 'check roundtrip encoding' '\n> +test_lazy_prereq ICONV_SHIFT_JIS '\n> +\ticonv -f UTF-8 -t SHIFT-JIS </dev/null 2>/dev/null\n\nIs this 2>/dev/null needed?  Leaving it out should make the test\neasier to debug.\n\nIf I'm reading it correctly, test_run_lazy_prereq_ appears to respect\nthe normal --verbose etc settings, which means that the 2>/dev/null\ncould be left out.  A quick check[*] with and without -v seems to bear\nthis out.\n\n> +'\n> +\n> +test_expect_success ICONV_SHIFT_JIS 'check roundtrip encoding' '\n>  \ttest_when_finished \"rm -f roundtrip.shift roundtrip.utf16\" &&\n>  \ttest_when_finished \"git reset --hard HEAD\" &&\n\nWith that tweak (removal of 2>/dev/null), this is\nReviewed-by: Jonathan Nieder <jrnieder@gmail.com>\n\nThanks.\n\n[*]\n diff --git i/t/t0000-basic.sh w/t/t0000-basic.sh\n index 850f651e4e..879f63118e 100755\n --- i/t/t0000-basic.sh\n +++ w/t/t0000-basic.sh\n @@ -25,6 +25,10 @@ try_local_x () {\n  \techo \"$x\"\n  }\n  \n +test_lazy_prereq NOISY_TEST '\n +\techo >&2 \"PAAARTY!\"\n +'\n +\n  # This test is an experiment to check whether any Git users are using\n  # Shells that don't support the \"local\" keyword. \"local\" is not\n  # POSIX-standard, but it is very widely supported by POSIX-compliant\n @@ -35,7 +39,7 @@ try_local_x () {\n  # fails this test, you can ignore the failure, but please report the\n  # problem to the Git mailing list <git@vger.kernel.org>, as it might\n  # convince us to continue avoiding the use of \"local\".\n -test_expect_success 'verify that the running shell supports \"local\"' '\n +test_expect_success NOISY_TEST 'verify that the running shell supports \"local\"' '\n  \tx=\"notlocal\" &&\n  \techo \"local\" >expected1 &&\n  \ttry_local_x >actual1 &&\n"},{"id":"356875","messageId":"20180829200908.13461-1-bedhanger@gmx.de","threadId":"49242","inReplyTo":"20180828193827.8648-2-avarab@gmail.com","subject":"Re: [PATCH 1/2] tests: fix non-portable \"${var:-\"str\"}\" construct","fromName":"Ann T Ropea","fromEmail":"bedhanger@gmx.de","sentAt":"2018-08-29T20:09:08Z","receivedAt":"2018-08-29T20:23:05Z","isPatch":true,"sender":{"key":"bedhanger@gmx.de","avatar":"https://avatars.githubusercontent.com/u/21041143?v=4"},"body":"Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n\n> Fix this by removing the redundant quotes. There's no need for them,\n> and the resulting code works under all the aforementioned shells. This\n> fixes a regression in c2f1d3989 (\"t4013: test new output from diff\n> --abbrev --raw\", 2017-12-03) first released with Git v2.16.0.\n\nAcked-by: Ann T Ropea <bedhanger@gmx.de>\n\nThanks.\n"}]}