Hi Patrick,
On Thu, 27 Mar 2025, Patrick Steinhardt wrote:
> We have a couple of tests that depend on Perl for textconv scripts.
> Refactor these tests to instead be implemented via shell utilities so
> that we can drop a couple of PERL_TEST_HELPERS prerequisites.
>
> Note that not all of the conversions are a one-to-one equivalent to the
> previous textconv scripts. But that's not really needed in the first
> place: we only care that the textconv script does something, and that
> can be verified trivially without having a full-blown invocation of
> hexdump. So at times, the implementation of the textconv scripts is
> reduced to their bare minimum and the expectations of those tests are
> adapted accordingly.
Hmm. I am having a harder time with this patch than with the others. See
below.
>
> Signed-off-by: Patrick Steinhardt <ps@xxxxxx>
> ---
> t/t4030-diff-textconv.sh | 15 +++------------
> t/t4031-diff-rewrite-binary.sh | 19 +++++++------------
> t/t7815-grep-binary.sh | 15 +++------------
> 3 files changed, 13 insertions(+), 36 deletions(-)
>
> diff --git a/t/t4030-diff-textconv.sh b/t/t4030-diff-textconv.sh
> index c7d8eb12453..f904fc19f69 100755
> --- a/t/t4030-diff-textconv.sh
> +++ b/t/t4030-diff-textconv.sh
> @@ -4,12 +4,6 @@ test_description='diff.*.textconv tests'
>
> . ./test-lib.sh
>
> -if ! test_have_prereq PERL_TEST_HELPERS
> -then
> - skip_all='skipping diff textconv tests; Perl not available'
> - test_done
> -fi
> -
> find_diff() {
> sed '1,/^index /d' | sed '/^-- $/,$d'
> }
> @@ -26,13 +20,10 @@ cat >expect.text <<'EOF'
> +1
> EOF
>
> -cat >hexdump <<'EOF'
> -#!/bin/sh
> -"$PERL_PATH" -e '$/ = undef; $_ = <>; s/./ord($&)/ge; print $_' < "$1"
> -EOF
> -chmod +x hexdump
> -
> test_expect_success 'setup binary file with history' '
> + write_script hexdump <<-\EOF &&
> + tr "\000\001" "01" <"$1"
> + EOF
So here the `hexdump` script is written, basically replacing NUL and SOH
with the digits zero and one, respectively. I wonder why the script does
not call `test-tool hexdump` instead? And I wonder even more why no test
case has to be adapted below this change in the same file. I _guess_ that
the reason is that the file named, creatively, "file" is initialized with
a NUL and a newline, committed, then a line is appended that contains SOH
and a newline, and then the test cases verify the hunk _headers_ only?
If using `test-tool hexdump <"$1"` would work here, too, I'd actually have
preferred that over the `tr` invocation, even if would still not be
recapitulating the functionality of that Perl script (which, contrary to
its name, seemed never to have output hexadecimal values...).
To be clear: I do not suggest to change the patch, I am merely puzzled why
the more obvious `test-tool hexdump <"$1"` was not used here?
> test_commit --printf one file "\\0\\n" &&
> test_commit --printf --append two file "\\01\\n"
> '
> diff --git a/t/t4031-diff-rewrite-binary.sh b/t/t4031-diff-rewrite-binary.sh
> index cbe50b15772..15e012ccc7c 100755
> --- a/t/t4031-diff-rewrite-binary.sh
> +++ b/t/t4031-diff-rewrite-binary.sh
> @@ -57,24 +57,19 @@ test_expect_success 'diff --stat counts binary rewrite as 0 lines' '
> grep " rewrite file" diff
> '
>
> -{
> - echo "#!$SHELL_PATH"
> - cat <<'EOF'
> -"$PERL_PATH" -e '$/ = undef; $_ = <>; s/./ord($&)/ge; print $_' < "$1"
> -EOF
> -} >dump
> -chmod +x dump
> -
> test_expect_success 'setup textconv' '
> + write_script dump <<-\EOF &&
> + test-tool hexdump <"$1"
> + EOF
So this looks much more like what I would have expected also in t4030,
and...
> echo file diff=foo >.gitattributes &&
> git config diff.foo.textconv "\"$(pwd)\""/dump
> '
>
> -test_expect_success PERL_TEST_HELPERS 'rewrite diff respects textconv' '
> +test_expect_success 'rewrite diff respects textconv' '
> git diff -B >diff &&
> - grep "dissimilarity index" diff &&
> - grep "^-61" diff &&
> - grep "^-0" diff
> + test_grep "dissimilarity index" diff &&
> + test_grep "^-3d 0a 00" diff &&
> + test_grep "^+3d 0a 01" diff
> '
... the adjustment of the expectations is actually going above and beyond,
the original test was not half as stringent as the new test is.
The rest of the patch looks good to me.
Ciao,
Johannes
>
> test_done
> diff --git a/t/t7815-grep-binary.sh b/t/t7815-grep-binary.sh
> index b2730d200c8..3bd91da9707 100755
> --- a/t/t7815-grep-binary.sh
> +++ b/t/t7815-grep-binary.sh
> @@ -4,12 +4,6 @@ test_description='git grep in binary files'
>
> . ./test-lib.sh
>
> -if ! test_have_prereq PERL_TEST_HELPERS
> -then
> - skip_all='skipping grep binary tests; Perl not available'
> - test_done
> -fi
> -
> test_expect_success 'setup' "
> echo 'binaryQfileQm[*]cQ*æQð' | q_to_nul >a &&
> git add a &&
> @@ -120,13 +114,10 @@ test_expect_success 'grep respects not-binary diff attribute' '
> test_cmp expect actual
> '
>
> -cat >nul_to_q_textconv <<'EOF'
> -#!/bin/sh
> -"$PERL_PATH" -pe 'y/\000/Q/' < "$1"
> -EOF
> -chmod +x nul_to_q_textconv
> -
> test_expect_success 'setup textconv filters' '
> + write_script nul_to_q_textconv <<-\EOF &&
> + tr "\000" "Q" <"$1"
> + EOF
> echo a diff=foo >.gitattributes &&
> git config diff.foo.textconv "\"$(pwd)\""/nul_to_q_textconv
> '
>
> --
> 2.49.0.472.ge94155a9ec.dirty
>
>
