Re: [PATCH 2/2] tests: fix non-portable iconv invocation
- From
Jonathan Nieder <jrnieder@gmail.com>
- Date
- Aug 28, 2018, 22:55 UTC
- Message-ID
- <20180828225512.GB74687@aiede.svl.corp.google.com>
- In-Reply-To
- <20180828193827.8648-3-avarab@gmail.com>
Hi,
Ævar Arnfjörð Bjarmason wrote:
Show 15 quoted lines
> 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 <avarab@gmail.com>
> ---
> t/t0028-working-tree-encoding.sh | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)One nit:
[...]
Show 11 quoted lines
> 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 2>/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.
Show 5 quoted lines
> +' > + > +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 <jrnieder@gmail.com>
Thanks.
[*]
diff --git i/t/t0000-basic.sh w/t/t0000-basic.sh
index 850f651e4e..879f63118e 100755
--- i/t/t0000-basic.sh
+++ w/t/t0000-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 <git@vger.kernel.org>, 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 &&