Commit 99a64e4b73c (tests: lint for run-away here-doc, 2017-03-22)
tweaked the chain-lint test to catch unclosed here-docs. It works by
adding an extra "echo" command after the test snippet, and checking that
it is run (if it gets swallowed by a here-doc, naturally it is not run).
The downside here is that we introduced an extra $() substitution, which
happens in a subshell. This has a measurable performance impact when
run for many tests.
The tradeoff in safety was undoubtedly worth it when 99a64e4b73c was
written. But these days, the external chainlint.pl does a pretty good
job of finding these (even though it's not something it specifically
tries to flag). For example, if you have a test like:
test_expect_success 'should fail linter' '
some_command >actual &&
cat >expect <<-\EOF &&
ok
# missing EOF line here
test_cmp expect actual
'
it will see that the here-doc isn't closed, treat it as not-a-here-doc,
and complain that the "ok" line does not have an "&&". So in practice we
should be catching these via that linter, although:
- the error message is not as good as it could be (the real problem is
the unclosed here-doc)
- it can be fooled if there are no lines in the here-doc:
cat >expect <<-\EOF &&
# missing EOF line here
or if every line in the here-doc has &&-chaining (weird, but
possible)
Those are sufficiently unlikely that they're not worth worrying too much
about. And by switching back to a simpler chain-lint, hyperfine reports
a measurable speedup on t3070 (which has 1800 tests):
'HEAD' ran
1.12 ± 0.01 times faster than 'HEAD~1'
Signed-off-by: Jeff King <peff@xxxxxxxx>
---
I didn't look at how hard it would be to teach chainlint.pl to complain
about the unclosed here-doc. I think it _might_ actually not be that
bad, just because it does already understand here-docs in general. If it
handled that, then all of the hand-waving in the second half of the
commit message could go away. ;)
I'd also be OK dropping this. 12% is nice, but this one test is an
outlier. Picking t4202 somewhat at random as a more realistic test, any
improvement seems to be mostly lost in the noise.
t/test-lib.sh | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/t/test-lib.sh b/t/test-lib.sh
index cfcbd899c5a..0048ec7b6f6 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -1101,9 +1101,10 @@ test_run_ () {
trace=
# 117 is magic because it is unlikely to match the exit
# code of other programs
- if test "OK-117" != "$(test_eval_ "fail_117 && $1${LF}${LF}echo OK-\$?" 3>&1)"
+ test_eval_ "fail_117 && $1"
+ if test $? != 117
then
- BUG "broken &&-chain or run-away HERE-DOC: $1"
+ BUG "broken &&-chain: $1"
fi
trace=$trace_tmp
fi
--
2.40.0.616.gf524ec75088
