Re: [PATCH v3 13/20] t: refactor tests depending on Perl for textconv scripts

[Patrick Steinhardt <ps@pks.im>]

On Tue, Apr 01, 2025 at 08:55:22PM +0200, Johannes Schindelin wrote:
> On Thu, 27 Mar 2025, Patrick Steinhardt wrote:
> > 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
> > @@ -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?

Phillip had the same comment, and I was trying to address that by
improving the commit message a bit. But seems like it still isn't clear
enough.

The reason why I decided against using `test-tool hexdump` is that it
would have a ripple effect. The output generated by that helper is not
the same as the output generated by the Perl script, so if we started to
use the hexdump helper I would have to adapt a bunch of tests in this
test file to update their expectations.

The result would look something like the appended patch, which I think
is quite awkward. On the one hand we have trailing whitespace in the
expectation, on the other hand we have weird seemingly-unrelated changes
in other tests. So I shied away from that and instead decided to use a
simpler variant of the textconv script.

Let me adapt the commit message once again and make it a bit more
concrete compared to the current fuzzy description.

Patrick

diff --git a/t/t4030-diff-textconv.sh b/t/t4030-diff-textconv.sh
index f904fc19f69..16d7fd4c5ca 100755
--- a/t/t4030-diff-textconv.sh
+++ b/t/t4030-diff-textconv.sh
@@ -15,14 +15,14 @@ EOF
 cat >expect.text <<'EOF'
 --- a/file
 +++ b/file
-@@ -1 +1,2 @@
- 0
-+1
+@@ -1 +1 @@
+-00 0a 
++00 0a 01 0a 
 EOF
 
 test_expect_success 'setup binary file with history' '
 	write_script hexdump <<-\EOF &&
-	tr "\000\001" "01" <"$1"
+	test-tool hexdump <"$1"
 	EOF
 	test_commit --printf one file "\\0\\n" &&
 	test_commit --printf --append two file "\\01\\n"
@@ -92,7 +92,7 @@ test_expect_success 'show blob produces binary' '
 
 test_expect_success 'show --textconv blob produces text' '
 	git show --textconv HEAD:file >actual &&
-	printf "0\\n1\\n" >expect &&
+	printf "00 0a 01 0a \n" >expect &&
 	test_cmp expect actual
 '
 
@@ -103,7 +103,7 @@ test_expect_success 'show --no-textconv blob produces binary' '
 '
 
 test_expect_success 'grep-diff (-G) operates on textconv data (add)' '
-	echo one >expect &&
+	printf "two\none\n" >expect &&
 	git log --root --format=%s -G0 >actual &&
 	test_cmp expect actual
 '
@@ -115,7 +115,7 @@ test_expect_success 'grep-diff (-G) operates on textconv data (modification)' '
 '
 
 test_expect_success 'pickaxe (-S) operates on textconv data (add)' '
-	echo one >expect &&
+	printf "two\none\n" >expect &&
 	git log --root --format=%s -S0 >actual &&
 	test_cmp expect actual
 '
@@ -146,9 +146,8 @@ symlink=$(git rev-parse --short $(printf frotz | git hash-object --stdin))
 cat >expect.typechange <<EOF
 --- a/file
 +++ /dev/null
-@@ -1,2 +0,0 @@
--0
--1
+@@ -1 +0,0 @@
+-00 0a 01 0a 
 diff --git a/file b/file
 new file mode 120000
 index 0000000..$symlink