| Message ID | 20250320-b4-pks-t-perlless-v1-13-b1eefe27ac55@pks.im (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | t: drop Perl as a mandatory prerequisite | expand |
On Thu, Mar 20, 2025 at 5:37 AM Patrick Steinhardt <ps@pks.im> 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. > > Signed-off-by: Patrick Steinhardt <ps@pks.im> > --- > -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 > ' This change seems unrelated to the stated purpose (`textconv`) of this patch(?).
On Thu, Mar 20, 2025 at 03:37:08PM -0400, Eric Sunshine wrote: > On Thu, Mar 20, 2025 at 5:37 AM Patrick Steinhardt <ps@pks.im> 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. > > > > Signed-off-by: Patrick Steinhardt <ps@pks.im> > > --- > > -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 > > ' > > This change seems unrelated to the stated purpose (`textconv`) of this patch(?). Not quite. The test previously didn't run because it depends on the Perl-based textconv script. Now that this textconv script was adapted to use shell scripting instead it can run, but as explained in the commit message the output of the textconv script changed. We don't really care for the exact output at all, we only care that textconv did its thing. But we do have to adapt the test accordingly. Patrick
On Mon, Mar 24, 2025 at 8:46 AM Patrick Steinhardt <ps@pks.im> wrote: > On Thu, Mar 20, 2025 at 03:37:08PM -0400, Eric Sunshine wrote: > > On Thu, Mar 20, 2025 at 5:37 AM Patrick Steinhardt <ps@pks.im> wrote: > > > -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 > > > ' > > > > This change seems unrelated to the stated purpose (`textconv`) of this patch(?). > > Not quite. The test previously didn't run because it depends on the > Perl-based textconv script. Now that this textconv script was adapted > to use shell scripting instead it can run, but as explained in the > commit message the output of the textconv script changed. We don't > really care for the exact output at all, we only care that textconv did > its thing. But we do have to adapt the test accordingly. Okay, I see that now that I have read your response and examined the change more closely. The unrelated `grep` to `test_grep` change visually overwhelms the diff, so much so that I overlooked the other smaller necessary changes. Perhaps it would make sense to mention the unrelated change in the commit message but is not itself worth a reroll.
Hi Patrick On 20/03/2025 09:35, 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. > > -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" I guess it is fine just to handle the characters we expect at the moment (is that what the second paragraph of the commit message is referring to?), but the script it was more tolerant of future changes to the test data. We could always just use 'test-tool hexdump' here like the tests below but it is probably not worth a re-roll on its own. Overall this series looks like a useful improvement. Thanks Phillip > + EOF > 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 > 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 > ' > > 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 > ' >
On Mon, Mar 24, 2025 at 12:07:52PM -0400, Eric Sunshine wrote: > On Mon, Mar 24, 2025 at 8:46 AM Patrick Steinhardt <ps@pks.im> wrote: > > On Thu, Mar 20, 2025 at 03:37:08PM -0400, Eric Sunshine wrote: > > > On Thu, Mar 20, 2025 at 5:37 AM Patrick Steinhardt <ps@pks.im> wrote: > > > > -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 > > > > ' > > > > > > This change seems unrelated to the stated purpose (`textconv`) of this patch(?). > > > > Not quite. The test previously didn't run because it depends on the > > Perl-based textconv script. Now that this textconv script was adapted > > to use shell scripting instead it can run, but as explained in the > > commit message the output of the textconv script changed. We don't > > really care for the exact output at all, we only care that textconv did > > its thing. But we do have to adapt the test accordingly. > > Okay, I see that now that I have read your response and examined the > change more closely. The unrelated `grep` to `test_grep` change > visually overwhelms the diff, so much so that I overlooked the other > smaller necessary changes. Perhaps it would make sense to mention the > unrelated change in the commit message but is not itself worth a > reroll. I already tried to describe this in the commit message, but obviously I seem to have failed :) I'll add another sentence to mention that tests have to be adapted accordingly. Patrick
On Mon, Mar 24, 2025 at 04:16:22PM +0000, Phillip Wood wrote: > Hi Patrick > > On 20/03/2025 09:35, 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. > > > -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" > I guess it is fine just to handle the characters we expect at the moment (is > that what the second paragraph of the commit message is referring to?), but > the script it was more tolerant of future changes to the test data. We could > always just use 'test-tool hexdump' here like the tests below but it is > probably not worth a re-roll on its own. That was my first version, but it required a bunch of changes to tests all over the place. So I decided to simplify this test suite to do the bare minimum. Patrick
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 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 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 ' 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 '
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. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- 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(-)