Re: [PATCH 2/2] tests: fix non-portable iconv invocation

2018-08-28 Thread Jonathan Nieder
Hi,

Ævar Arnfjörð Bjarmason wrote:

> The iconv that comes with a FreeBSD 11.2-RELEASE-p2 box I have access
> to doesn't support the SHIFT-JIS encoding. Guard a test added in
> e92d62253 ("convert: add round trip check based on
> 'core.checkRoundtripEncoding'", 2018-04-15) first released with Git
> v2.18.0 with a prerequisite that checks for its availability.
>
> The iconv command is in POSIX, and we have numerous tests
> unconditionally relying on its ability to convert ASCII, UTF-8 and
> UTF-16, but unconditionally relying on the presence of more obscure
> encodings isn't portable.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason 
> ---
>  t/t0028-working-tree-encoding.sh | 6 +-
>  1 file changed, 5 insertions(+), 1 deletion(-)

One nit:

[...]
> diff --git a/t/t0028-working-tree-encoding.sh 
> b/t/t0028-working-tree-encoding.sh
> index 12b8eb963a..b0f4490e8e 100755
> --- a/t/t0028-working-tree-encoding.sh
> +++ b/t/t0028-working-tree-encoding.sh
> @@ -203,7 +203,11 @@ test_expect_success 'error if encoding garbage is 
> already in Git' '
>   test_i18ngrep "error: BOM is required" err.out
>  '
>  
> -test_expect_success 'check roundtrip encoding' '
> +test_lazy_prereq ICONV_SHIFT_JIS '
> + iconv -f UTF-8 -t SHIFT-JIS /dev/null

Is this 2>/dev/null needed?  Leaving it out should make the test
easier to debug.

If I'm reading it correctly, test_run_lazy_prereq_ appears to respect
the normal --verbose etc settings, which means that the 2>/dev/null
could be left out.  A quick check[*] with and without -v seems to bear
this out.

> +'
> +
> +test_expect_success ICONV_SHIFT_JIS 'check roundtrip encoding' '
>   test_when_finished "rm -f roundtrip.shift roundtrip.utf16" &&
>   test_when_finished "git reset --hard HEAD" &&

With that tweak (removal of 2>/dev/null), this is
Reviewed-by: Jonathan Nieder 

Thanks.

[*]
 diff --git i/t/t-basic.sh w/t/t-basic.sh
 index 850f651e4e..879f63118e 100755
 --- i/t/t-basic.sh
 +++ w/t/t-basic.sh
 @@ -25,6 +25,10 @@ try_local_x () {
echo "$x"
  }
  
 +test_lazy_prereq NOISY_TEST '
 +  echo >&2 "PAAARTY!"
 +'
 +
  # This test is an experiment to check whether any Git users are using
  # Shells that don't support the "local" keyword. "local" is not
  # POSIX-standard, but it is very widely supported by POSIX-compliant
 @@ -35,7 +39,7 @@ try_local_x () {
  # fails this test, you can ignore the failure, but please report the
  # problem to the Git mailing list , as it might
  # convince us to continue avoiding the use of "local".
 -test_expect_success 'verify that the running shell supports "local"' '
 +test_expect_success NOISY_TEST 'verify that the running shell supports 
"local"' '
x="notlocal" &&
echo "local" >expected1 &&
try_local_x >actual1 &&


[PATCH 2/2] tests: fix non-portable iconv invocation

2018-08-28 Thread Ævar Arnfjörð Bjarmason
The iconv that comes with a FreeBSD 11.2-RELEASE-p2 box I have access
to doesn't support the SHIFT-JIS encoding. Guard a test added in
e92d62253 ("convert: add round trip check based on
'core.checkRoundtripEncoding'", 2018-04-15) first released with Git
v2.18.0 with a prerequisite that checks for its availability.

The iconv command is in POSIX, and we have numerous tests
unconditionally relying on its ability to convert ASCII, UTF-8 and
UTF-16, but unconditionally relying on the presence of more obscure
encodings isn't portable.

Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 t/t0028-working-tree-encoding.sh | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/t/t0028-working-tree-encoding.sh b/t/t0028-working-tree-encoding.sh
index 12b8eb963a..b0f4490e8e 100755
--- a/t/t0028-working-tree-encoding.sh
+++ b/t/t0028-working-tree-encoding.sh
@@ -203,7 +203,11 @@ test_expect_success 'error if encoding garbage is already 
in Git' '
test_i18ngrep "error: BOM is required" err.out
 '
 
-test_expect_success 'check roundtrip encoding' '
+test_lazy_prereq ICONV_SHIFT_JIS '
+   iconv -f UTF-8 -t SHIFT-JIS /dev/null
+'
+
+test_expect_success ICONV_SHIFT_JIS 'check roundtrip encoding' '
test_when_finished "rm -f roundtrip.shift roundtrip.utf16" &&
test_when_finished "git reset --hard HEAD" &&
 
-- 
2.19.0.rc0.228.g281dcd1b4d0