{"thread":{"id":"59491","subject":"[PATCH 0/4] some chainlint fixes and performance improvements","startedAt":"2023-03-28T20:20:48Z","lastAt":"2023-03-30T22:16:27Z","messageCount":32,"participants":["Jeff King","Junio C Hamano","Eric Sunshine","Ævar Arnfjörð Bjarmason"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"474362","messageId":"20230328202043.GA1241391@coredump.intra.peff.net","threadId":"59491","inReplyTo":null,"subject":"[PATCH 0/4] some chainlint fixes and performance improvements","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-03-28T20:20:43Z","receivedAt":"2023-03-28T20:20:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Here are a few fixes for chainlint. The first patch should avoid the\nconfusion we discussed in the subthread at:\n\n https://lore.kernel.org/git/3714ba2f6528c38eb9c438126316a08b0cefca7c.1679696180.git.git@grubix.eu/\n\nand the relevant folks are cc'd. The rest are some old performance\nimprovement ideas I had for the internal chain-linter. I doubt they make\na huge difference overall, but they can be measured in certain cases.\nThe first one to me looks like an obvious win. The second one is\ndebatable, as it involves some hand-waving. The third one turned out not\nto make anything faster, but makes the code a little simpler.\n\nSo I'm on the fence for patches 3 and 4 below, but the first two I think\nare strict improvements.\n\n [1/4]: tests: run internal chain-linter under \"make test\"\n [2/4]: tests: replace chainlint subshell with a function\n [3/4]: tests: drop here-doc check from internal chain-linter\n [4/4]: tests: skip test_eval_ in internal chain-lint\n\n t/Makefile | 2 +-\n t/test-lib.sh | 27 ++++++++++++++-------------\n 2 files changed, 15 insertions(+), 14 deletions(-)\n\n-Peff\n"},{"id":"474363","messageId":"20230328202207.GA1241631@coredump.intra.peff.net","threadId":"59491","inReplyTo":"20230328202043.GA1241391@coredump.intra.peff.net","subject":"[PATCH 1/4] tests: run internal chain-linter under \"make test\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-03-28T20:22:07Z","receivedAt":"2023-03-28T20:22:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Since 69b9924b875 (t/Makefile: teach `make test` and `make prove` to run\nchainlint.pl, 2022-09-01), we run a single chainlint.pl process for all\nscripts, and then instruct each individual script to run with the\nequivalent of --no-chain-lint, which tells them not to redundantly run\nthe chainlint script themselves.\n\nHowever, this also disables the internal linter run within the shell by\neval-ing \"(exit 117) && $1\" and confirming we get code 117. In theory\nthe external linter produces a superset of complaints, and we don't need\nthe internal one anymore. However, we know there is at least one case\nwhere they differ. A test like:\n\n\ttest_expect_success 'should fail linter' '\n\t\tfalse &&\n\t\tsleep 2 &\n\t\tpid=$! &&\n\t\tkill $pid\n\t'\n\nis buggy (it ignores the failure from \"false\", because it is\nbackgrounded along with the sleep). The internal linter catches this,\nbut the external one doesn't (and teaching it to do so is complicated[1]).\nSo not only does \"make test\" miss this problem, but it's doubly\nconfusing because running the script standalone does complain.\n\nLet's teach the suppression in the Makefile to only turn off the\nexternal linter (which we know is redundant, as it was already run) and\nleave the internal one intact.\n\nI've used a new environment variable to do this here, and intentionally\ndid not add a \"--no-ext-chain-lint\" option. This is an internal\noptimization used by the Makefile, and not something that ordinary users\nwould need to tweak.\n\n[1] For discussion of chainlint.pl and this case, see:\n https://lore.kernel.org/git/CAPig+cQtLFX4PgXyyK_AAkCvg4Aw2RAC5MmLbib-aHHgTBcDuw@mail.gmail.com/\n\nSigned-off-by: Jeff King \n---\n t/Makefile | 2 +-\n t/test-lib.sh | 3 ++-\n 2 files changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/t/Makefile b/t/Makefile\nindex 88fa5049573..10881affdd0 100644\n--- a/t/Makefile\n+++ b/t/Makefile\n@@ -45,7 +45,7 @@ CHAINLINT = '$(PERL_PATH_SQ)' chainlint.pl\n # `test-chainlint` (which is a dependency of `test-lint`, `test` and `prove`)\n # checks all tests in all scripts via a single invocation, so tell individual\n # scripts not to \"chainlint\" themselves\n-CHAINLINTSUPPRESS = GIT_TEST_CHAIN_LINT=0 && export GIT_TEST_CHAIN_LINT &&\n+CHAINLINTSUPPRESS = GIT_TEST_EXT_CHAIN_LINT=0 && export GIT_TEST_EXT_CHAIN_LINT &&\n \n all: $(DEFAULT_TEST_TARGET)\n \ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 62136caee5a..09789566374 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -1593,7 +1593,8 @@ then\n \tBAIL_OUT_ENV_NEEDS_SANITIZE_LEAK \"GIT_TEST_SANITIZE_LEAK_LOG=true\"\n fi\n \n-if test \"${GIT_TEST_CHAIN_LINT:-1}\" != 0\n+if test \"${GIT_TEST_CHAIN_LINT:-1}\" != 0 &&\n+ test \"${GIT_TEST_EXT_CHAIN_LINT:-1}\" != 0\n then\n \t\"$PERL_PATH\" \"$TEST_DIRECTORY/chainlint.pl\" \"$0\" ||\n \t\tBUG \"lint error (see '?!...!? annotations above)\"\n-- \n2.40.0.616.gf524ec75088\n\n"},{"id":"474364","messageId":"20230328202302.GB1241631@coredump.intra.peff.net","threadId":"59491","inReplyTo":"20230328202043.GA1241391@coredump.intra.peff.net","subject":"[PATCH 2/4] tests: replace chainlint subshell with a function","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-03-28T20:23:02Z","receivedAt":"2023-03-28T20:23:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"To test that we don't break the &&-chain, test-lib.sh does something\nlike:\n\n (exit 117) && $test_commands\n\nand checks that the result is exit code 117. We don't care what that\ninitial command is, as long as it exits with a unique code. Using \"exit\"\nworks and is simple, but is a bit expensive since it requires a subshell\n(to avoid exiting the whole script!). This isn't usually very\nnoticeable, but it can add up for scripts which have a large number of\ntests.\n\nUsing \"return\" naively won't work here, because we'd return from the\nfunction eval-ing the snippet (and it wouldn't find &&-chain breakages).\nBut if we further push that into its own function, it does exactly what\nwe want, without extra subshell overhead.\n\nAccording to hyperfine, this produces a measurable improvement when\nrunning t3070 (which has 1800 tests, all of them quite short):\n\n 'HEAD' ran\n 1.09 ± 0.01 times faster than 'HEAD~1'\n\nSigned-off-by: Jeff King \n---\n t/test-lib.sh | 6 +++++-\n 1 file changed, 5 insertions(+), 1 deletion(-)\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 09789566374..cfcbd899c5a 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -1086,6 +1086,10 @@ test_eval_ () {\n \treturn $test_eval_ret_\n }\n \n+fail_117 () {\n+\treturn 117\n+}\n+\n test_run_ () {\n \ttest_cleanup=:\n \texpecting_failure=$2\n@@ -1097,7 +1101,7 @@ test_run_ () {\n \t\ttrace=\n \t\t# 117 is magic because it is unlikely to match the exit\n \t\t# code of other programs\n-\t\tif test \"OK-117\" != \"$(test_eval_ \"(exit 117) && $1${LF}${LF}echo OK-\\$?\" 3>&1)\"\n+\t\tif test \"OK-117\" != \"$(test_eval_ \"fail_117 && $1${LF}${LF}echo OK-\\$?\" 3>&1)\"\n \t\tthen\n \t\t\tBUG \"broken &&-chain or run-away HERE-DOC: $1\"\n \t\tfi\n-- \n2.40.0.616.gf524ec75088\n\n"},{"id":"474366","messageId":"20230328202819.GC1241631@coredump.intra.peff.net","threadId":"59491","inReplyTo":"20230328202043.GA1241391@coredump.intra.peff.net","subject":"[PATCH 3/4] tests: drop here-doc check from internal chain-linter","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-03-28T20:28:19Z","receivedAt":"2023-03-28T20:28:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Commit 99a64e4b73c (tests: lint for run-away here-doc, 2017-03-22)\ntweaked the chain-lint test to catch unclosed here-docs. It works by\nadding an extra \"echo\" command after the test snippet, and checking that\nit is run (if it gets swallowed by a here-doc, naturally it is not run).\n\nThe downside here is that we introduced an extra $() substitution, which\nhappens in a subshell. This has a measurable performance impact when\nrun for many tests.\n\nThe tradeoff in safety was undoubtedly worth it when 99a64e4b73c was\nwritten. But these days, the external chainlint.pl does a pretty good\njob of finding these (even though it's not something it specifically\ntries to flag). For example, if you have a test like:\n\n\ttest_expect_success 'should fail linter' '\n\t some_command >actual &&\n\t cat >expect <<-\\EOF &&\n\t\tok\n\t\t# missing EOF line here\n\t test_cmp expect actual\n\t'\n\nit will see that the here-doc isn't closed, treat it as not-a-here-doc,\nand complain that the \"ok\" line does not have an \"&&\". So in practice we\nshould be catching these via that linter, although:\n\n - the error message is not as good as it could be (the real problem is\n the unclosed here-doc)\n\n - it can be fooled if there are no lines in the here-doc:\n\n cat >expect <<-\\EOF &&\n # missing EOF line here\n\n or if every line in the here-doc has &&-chaining (weird, but\n possible)\n\nThose are sufficiently unlikely that they're not worth worrying too much\nabout. And by switching back to a simpler chain-lint, hyperfine reports\na measurable speedup on t3070 (which has 1800 tests):\n\n 'HEAD' ran\n 1.12 ± 0.01 times faster than 'HEAD~1'\n\nSigned-off-by: Jeff King \n---\nI didn't look at how hard it would be to teach chainlint.pl to complain\nabout the unclosed here-doc. I think it _might_ actually not be that\nbad, just because it does already understand here-docs in general. If it\nhandled that, then all of the hand-waving in the second half of the\ncommit message could go away. ;)\n\nI'd also be OK dropping this. 12% is nice, but this one test is an\noutlier. Picking t4202 somewhat at random as a more realistic test, any\nimprovement seems to be mostly lost in the noise.\n\n t/test-lib.sh | 5 +++--\n 1 file changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex cfcbd899c5a..0048ec7b6f6 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -1101,9 +1101,10 @@ test_run_ () {\n \t\ttrace=\n \t\t# 117 is magic because it is unlikely to match the exit\n \t\t# code of other programs\n-\t\tif test \"OK-117\" != \"$(test_eval_ \"fail_117 && $1${LF}${LF}echo OK-\\$?\" 3>&1)\"\n+\t\ttest_eval_ \"fail_117 && $1\"\n+\t\tif test $? != 117\n \t\tthen\n-\t\t\tBUG \"broken &&-chain or run-away HERE-DOC: $1\"\n+\t\t\tBUG \"broken &&-chain: $1\"\n \t\tfi\n \t\ttrace=$trace_tmp\n \tfi\n-- \n2.40.0.616.gf524ec75088\n\n"},{"id":"474367","messageId":"20230328202839.GD1241631@coredump.intra.peff.net","threadId":"59491","inReplyTo":"20230328202043.GA1241391@coredump.intra.peff.net","subject":"[PATCH 4/4] tests: skip test_eval_ in internal chain-lint","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-03-28T20:28:39Z","receivedAt":"2023-03-28T20:28:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"To check for broken &&-chains, we run \"fail_117 && $1\" as a test\nsnippet, and check the exit code. We use test_eval_ to do so, because\nthat's the way we run the actual test.\n\nBut we don't need any of its niceties, like \"set -x\" tracing. In fact,\nthey hinder us, because we have to explicitly disable them. So let's\nskip that and use \"eval\" more directly, which is simpler. I had hoped it\nwould also be faster, but it doesn't seem to produce a measurable\nimprovement (probably because it's just running internal shell commands,\nwith no subshells or forks).\n\nNote that there is one gotcha: even though we don't intend to run any of\nthe commands if the &&-chain is intact, an error like this:\n\n test_expect_success 'broken' '\n\t# this next line breaks the &&-chain\n\ttrue\n\t# and then this one is executed even by the linter\n\treturn 1\n '\n\nmeans we'll \"return 1\" from the eval, and thus from test_run_(). We\nactually do notice this in test_expect_success, but only by saying \"hey,\nthis test didn't say it was OK, so it must have failed\", which is not\nright (it should say \"broken &&-chain\").\n\nWe can handle this by calling test_eval_inner_() instead, which is our\ntrick for wrapping \"return\" in a test snippet. But to do that, we have\nto push the trace code out of that inner function and into test_eval_().\nThis is arguably where it belonged in the first place, but it never\nmattered because the \"inner_\" function had only one caller.\n\nSigned-off-by: Jeff King \n---\n t/test-lib.sh | 17 ++++++-----------\n 1 file changed, 6 insertions(+), 11 deletions(-)\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 0048ec7b6f6..293caf0f20e 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -1041,10 +1041,7 @@ want_trace () {\n # (and we want to make sure we run any cleanup like\n # \"set +x\").\n test_eval_inner_ () {\n-\t# Do not add anything extra (including LF) after '$*'\n-\teval \"\n-\t\twant_trace && trace_level_=$(($trace_level_+1)) && set -x\n-\t\t$*\"\n+\teval \"$*\"\n }\n \n test_eval_ () {\n@@ -1069,7 +1066,10 @@ test_eval_ () {\n \t# be _inside_ the block to avoid polluting the \"set -x\" output\n \t#\n \n-\ttest_eval_inner_ \"$@\" &3 2>&4\n+\t# Do not add anything extra (including LF) after '$*'\n+\ttest_eval_inner_ &3 2>&4 \"\n+\t\twant_trace && trace_level_=$(($trace_level_+1)) && set -x\n+\t\t$*\"\n \t{\n \t\ttest_eval_ret_=$?\n \t\tif want_trace\n@@ -1095,18 +1095,13 @@ test_run_ () {\n \texpecting_failure=$2\n \n \tif test \"${GIT_TEST_CHAIN_LINT:-1}\" != 0; then\n-\t\t# turn off tracing for this test-eval, as it simply creates\n-\t\t# confusing noise in the \"-x\" output\n-\t\ttrace_tmp=$trace\n-\t\ttrace=\n \t\t# 117 is magic because it is unlikely to match the exit\n \t\t# code of other programs\n-\t\ttest_eval_ \"fail_117 && $1\"\n+\t\ttest_eval_inner_ \"fail_117 && $1\" &3 2>&4\n \t\tif test $? != 117\n \t\tthen\n \t\t\tBUG \"broken &&-chain: $1\"\n \t\tfi\n-\t\ttrace=$trace_tmp\n \tfi\n \n \tsetup_malloc_check\n-- \n2.40.0.616.gf524ec75088\n"},{"id":"474369","messageId":"xmqqjzz0ehiq.fsf@gitster.g","threadId":"59491","inReplyTo":"20230328202302.GB1241631@coredump.intra.peff.net","subject":"Re: [PATCH 2/4] tests: replace chainlint subshell with a function","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-28T20:40:45Z","receivedAt":"2023-03-28T20:40:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> To test that we don't break the &&-chain, test-lib.sh does something\n> like:\n>\n> (exit 117) && $test_commands\n>\n> and checks that the result is exit code 117. We don't care what that\n> initial command is, as long as it exits with a unique code. Using \"exit\"\n> works and is simple, but is a bit expensive since it requires a subshell\n> (to avoid exiting the whole script!). This isn't usually very\n> noticeable, but it can add up for scripts which have a large number of\n> tests.\n>\n> Using \"return\" naively won't work here, because we'd return from the\n> function eval-ing the snippet (and it wouldn't find &&-chain breakages).\n> But if we further push that into its own function, it does exactly what\n> we want, without extra subshell overhead.\n\nCute.\n\n> According to hyperfine, this produces a measurable improvement when\n> running t3070 (which has 1800 tests, all of them quite short):\n>\n> 'HEAD' ran\n> 1.09 ± 0.01 times faster than 'HEAD~1'\n\nThat is certainly a respectable improvement.\n\n> Signed-off-by: Jeff King \n> ---\n> t/test-lib.sh | 6 +++++-\n> 1 file changed, 5 insertions(+), 1 deletion(-)\n>\n> diff --git a/t/test-lib.sh b/t/test-lib.sh\n> index 09789566374..cfcbd899c5a 100644\n> --- a/t/test-lib.sh\n> +++ b/t/test-lib.sh\n> @@ -1086,6 +1086,10 @@ test_eval_ () {\n> \treturn $test_eval_ret_\n> }\n> \n> +fail_117 () {\n> +\treturn 117\n> +}\n> +\n> test_run_ () {\n> \ttest_cleanup=:\n> \texpecting_failure=$2\n> @@ -1097,7 +1101,7 @@ test_run_ () {\n> \t\ttrace=\n> \t\t# 117 is magic because it is unlikely to match the exit\n> \t\t# code of other programs\n> -\t\tif test \"OK-117\" != \"$(test_eval_ \"(exit 117) && $1${LF}${LF}echo OK-\\$?\" 3>&1)\"\n> +\t\tif test \"OK-117\" != \"$(test_eval_ \"fail_117 && $1${LF}${LF}echo OK-\\$?\" 3>&1)\"\n> \t\tthen\n> \t\t\tBUG \"broken &&-chain or run-away HERE-DOC: $1\"\n> \t\tfi\n"},{"id":"474380","messageId":"20230328210814.GA1754178@coredump.intra.peff.net","threadId":"59491","inReplyTo":"20230328202043.GA1241391@coredump.intra.peff.net","subject":"Re: [PATCH 0/4] some chainlint fixes and performance improvements","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-03-28T21:08:14Z","receivedAt":"2023-03-28T21:08:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 28, 2023 at 04:20:44PM -0400, Jeff King wrote:\n\n> The rest are some old performance improvement ideas I had for the\n> internal chain-linter. I doubt they make a huge difference overall,\n> but they can be measured in certain cases. The first one to me looks\n> like an obvious win. The second one is debatable, as it involves some\n> hand-waving. The third one turned out not to make anything faster, but\n> makes the code a little simpler.\n\nBTW, I noticed something really funky when timing t3070 for this series.\n\n $ time ./t3070-wildmatch.sh\n [a bunch of output]\n real\t0m4.750s\n user\t0m3.665s\n sys\t0m0.955s\n\n $ time ./t3070-wildmatch.sh >/dev/null\n real\t0m18.664s\n user\t0m9.185s\n sys\t0m9.495s\n\nEr, what? It gets way slower when redirected to /dev/null. I can't\nfigure out why. Replacing the actual tests with a simple command shows\nthe same behavior, so it's not a problem in the tested commands. I tried\nsimplifying what the test script was doing, but it really looks like the\nslowdown scales with the number of subshells/forks.\n\nThe problem is also independent of shell (bash vs dash). And here's an\neven weirder bit. If I pipe the output through cat, it's still fast:\n\n $ time ./t3070-wildmatch.sh | cat\n [lots of output]\n real\t0m4.719s\n user\t0m3.636s\n sys\t0m0.946s\n\nbut when cat goes to /dev/null, it's slow again!\n\n $ time ./t3070-wildmatch.sh | cat >/dev/null\n real\t0m18.383s\n user\t0m8.987s\n sys\t0m9.450s\n\nSo our scripts are seeing the same environment (a pipe), but somehow cat\non the other side is slowing things down. Which seems to me like it\ncould be a kernel problem, but it's hard to imagine what. I tried a few\nversions and couldn't find one that didn't exhibit it. This is a Debian\nunstable machine (so running kernel 6.1.20). Another machine running\nstable (so kernel 5.15) did not exhibit the problem, but there are many\ndifferent variables beyond kernel version there.\n\nSo this doesn't seem like a Git problem at all, but it's an interesting\nmystery, and I wondered if anybody else has run into it.\n\n-Peff\n"},{"id":"474382","messageId":"xmqqzg7wczx7.fsf@gitster.g","threadId":"59491","inReplyTo":"20230328202819.GC1241631@coredump.intra.peff.net","subject":"Re: [PATCH 3/4] tests: drop here-doc check from internal chain-linter","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-28T21:46:12Z","receivedAt":"2023-03-28T21:46:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I'd also be OK dropping this. 12% is nice, but this one test is an\n> outlier. Picking t4202 somewhat at random as a more realistic test, any\n> improvement seems to be mostly lost in the noise.\n>\n> t/test-lib.sh | 5 +++--\n> 1 file changed, 3 insertions(+), 2 deletions(-)\n>\n> diff --git a/t/test-lib.sh b/t/test-lib.sh\n> index cfcbd899c5a..0048ec7b6f6 100644\n> --- a/t/test-lib.sh\n> +++ b/t/test-lib.sh\n> @@ -1101,9 +1101,10 @@ test_run_ () {\n> \t\ttrace=\n> \t\t# 117 is magic because it is unlikely to match the exit\n> \t\t# code of other programs\n> -\t\tif test \"OK-117\" != \"$(test_eval_ \"fail_117 && $1${LF}${LF}echo OK-\\$?\" 3>&1)\"\n> +\t\ttest_eval_ \"fail_117 && $1\"\n> +\t\tif test $? != 117\n> \t\tthen\n> -\t\t\tBUG \"broken &&-chain or run-away HERE-DOC: $1\"\n> +\t\t\tBUG \"broken &&-chain: $1\"\n> \t\tfi\n\nThis \"here-doc\" linter is a cute trick. With seemingly so little\nextra code, it catches a breakage in such an unexpected way.\n\nEven with a very small code footprint, overhead of an extra process\nis still there, and it would be very hard to figure out what it does\n(once you are told what it does, you can probably figure out how it\nworks). These make it a \"cute trick\".\n\nWhile it is a bit sad to see it lost, the resulting code certainly\nis easier to follow, I would think. I do not offhand know how\nvaluable detecting unterminated here-doc is, compared to the\nincreased clarity of hte code.\n\nThanks.\n\n\n"},{"id":"474387","messageId":"20230329023702.GA1793752@coredump.intra.peff.net","threadId":"59491","inReplyTo":"xmqqzg7wczx7.fsf@gitster.g","subject":"Re: [PATCH 3/4] tests: drop here-doc check from internal chain-linter","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-03-29T02:37:02Z","receivedAt":"2023-03-29T02:37:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 28, 2023 at 02:46:12PM -0700, Junio C Hamano wrote:\n\n> > -\t\tif test \"OK-117\" != \"$(test_eval_ \"fail_117 && $1${LF}${LF}echo OK-\\$?\" 3>&1)\"\n> > +\t\ttest_eval_ \"fail_117 && $1\"\n> > +\t\tif test $? != 117\n> > \t\tthen\n> > -\t\t\tBUG \"broken &&-chain or run-away HERE-DOC: $1\"\n> > +\t\t\tBUG \"broken &&-chain: $1\"\n> > \t\tfi\n> \n> This \"here-doc\" linter is a cute trick. With seemingly so little\n> extra code, it catches a breakage in such an unexpected way.\n> \n> Even with a very small code footprint, overhead of an extra process\n> is still there, and it would be very hard to figure out what it does\n> (once you are told what it does, you can probably figure out how it\n> works). These make it a \"cute trick\".\n\nYes, I thought it was quite clever (and it is attributed to you), but I\nagree that I did not quite realize what it was doing until after running\ngit-blame. I only started with \"gee, why didn't we write this in the\nsimpler way?\", which is often a good starting point for archaeology. :)\n\n> While it is a bit sad to see it lost, the resulting code certainly\n> is easier to follow, I would think. I do not offhand know how\n> valuable detecting unterminated here-doc is, compared to the\n> increased clarity of hte code.\n\nI think the complexity is merited _if_ we think it is catching useful\ncases. And I do count unterminated here-doc as a useful case, because,\nas with broken &&-chains, the failure mode is so nasty (a test will\nreport success, even though part of the test was simply not run!).\n\nI just think chainlint.pl is doing a good enough job of catching it that\nwe can rely on it. I'll be curious if Eric has input there on whether it\ncan do even better, which would remove all of the caveats from the\ncommit message.\n\n-Peff\n"},{"id":"474388","messageId":"20230329030432.GA1801645@coredump.intra.peff.net","threadId":"59491","inReplyTo":"20230329023702.GA1793752@coredump.intra.peff.net","subject":"Re: [PATCH 3/4] tests: drop here-doc check from internal chain-linter","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-03-29T03:04:32Z","receivedAt":"2023-03-29T03:04:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 28, 2023 at 10:37:02PM -0400, Jeff King wrote:\n\n> I just think chainlint.pl is doing a good enough job of catching it that\n> we can rely on it. I'll be curious if Eric has input there on whether it\n> can do even better, which would remove all of the caveats from the\n> commit message.\n\nSo I _think_ it's something like this:\n\ndiff --git a/t/chainlint.pl b/t/chainlint.pl\nindex e966412999a..6b8c1de5208 100755\n--- a/t/chainlint.pl\n+++ b/t/chainlint.pl\n@@ -171,6 +171,9 @@ sub swallow_heredocs {\n \t\tmy $start = pos($$b);\n \t\tmy $indent = $tag =~ s/^\\t// ? '\\\\s*' : '';\n \t\t$$b =~ /(?:\\G|\\n)$indent\\Q$tag\\E(?:\\n|\\z)/gc;\n+\t\tif (pos($$b) == $start) {\n+\t\t\tdie \"oops, we did not find the end of the heredoc\";\n+\t\t}\n \t\tmy $body = substr($$b, $start, pos($$b) - $start);\n \t\t$self->{lineno} += () = $body =~ /\\n/sg;\n \t}\n\nBut I wasn't sure how to surface a clean error from here, since we're in\nthe Lexer. Maybe we just accumulate a \"problems\" array here, and then\nroll those up via the TestParser? I'm not very familiar with the\narrangement of that part of the script.\n\nAnd I say \"think\" because the thing I was worried about is that we'd do\nthis lexing at too high a level, and accidentally walk past the end of\nthe test. Which would mean getting fooled by;\n\n test_expect_success 'this one is broken' '\n\tcat >foo <<\\EOF\n\toops, we are missing our here-doc end\n '\n\n test_expect_success 'this one is ok' '\n\tcat >foo <<\\EOF\n\tthis one is OK, but we would not want to confuse\n\tits closing tag for the missing one\n\tEOF\n '\n\nBut it looks like Lexer::swallow_heredocs gets to see the individual\ntest snippets.\n\n-Peff\n"},{"id":"474389","messageId":"CAPig+cSMYt81fcU8fRNhN9qZ=95jvRNv9YGhkM6PxgdMywWnCA@mail.gmail.com","threadId":"59491","inReplyTo":"20230329023702.GA1793752@coredump.intra.peff.net","subject":"Re: [PATCH 3/4] tests: drop here-doc check from internal chain-linter","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2023-03-29T03:07:17Z","receivedAt":"2023-03-29T03:07:33Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Mar 28, 2023 at 10:37 PM Jeff King wrote:\n> On Tue, Mar 28, 2023 at 02:46:12PM -0700, Junio C Hamano wrote:\n> > > - if test \"OK-117\" != \"$(test_eval_ \"fail_117 && $1${LF}${LF}echo OK-\\$?\" 3>&1)\"\n> > > - BUG \"broken &&-chain or run-away HERE-DOC: $1\"\n> >\n> > This \"here-doc\" linter is a cute trick. With seemingly so little\n> > extra code, it catches a breakage in such an unexpected way.\n> >\n> > Even with a very small code footprint, overhead of an extra process\n> > is still there, and it would be very hard to figure out what it does\n> > (once you are told what it does, you can probably figure out how it\n> > works). These make it a \"cute trick\".\n>\n> Yes, I thought it was quite clever (and it is attributed to you), but I\n> agree that I did not quite realize what it was doing until after running\n> git-blame. I only started with \"gee, why didn't we write this in the\n> simpler way?\", which is often a good starting point for archaeology. :)\n\nI never realized what the \"OK-\" part of \"OK-117\" was doing either. I\nguess I should have gone spelunking through history to find out,\nthough it wasn't high-priority for me to know with regards to my work\non chainlint.pl, so I never got around to it. I suspect that the \"OK-\"\ntrick was discussed and added during the period I was off-list.\n\n> > While it is a bit sad to see it lost, the resulting code certainly\n> > is easier to follow, I would think. I do not offhand know how\n> > valuable detecting unterminated here-doc is, compared to the\n> > increased clarity of hte code.\n>\n> I think the complexity is merited _if_ we think it is catching useful\n> cases. And I do count unterminated here-doc as a useful case, because,\n> as with broken &&-chains, the failure mode is so nasty (a test will\n> report success, even though part of the test was simply not run!).\n>\n> I just think chainlint.pl is doing a good enough job of catching it that\n> we can rely on it. I'll be curious if Eric has input there on whether it\n> can do even better, which would remove all of the caveats from the\n> commit message.\n\nIt was an intentional design choice, for the sake of simplicity, _not_\nto make chainlint.pl a shell syntax checker, despite the fact that it\nis genuinely parsing shell code. After all, the shell itself -- by\nwhich test code will ultimately be executed -- is more than capable of\ndiagnosing syntax errors, so why burden chainlint.pl with all that\nextra baggage? Instead, chainlint.pl is focussed on detecting semantic\nproblems -- such as broken &&-chain and missing `return 1` -- which\nare of importance to _this_ project.\n\nSo, it was very much deliberate that chainlint.pl does not diagnose an\nunclosed here-doc. When making that design choice, though, I wasn't\naware of 99a64e4b73c (tests: lint for run-away here-doc, 2017-03-22),\nthus didn't know that our test framework had been allowing such\nproblems to slip through silently[1].\n\nThat said, it doesn't look too hard to make chainlint.pl diagnose an\nunclosed here-doc.\n\n[1]: Why is that, by the way? Is `eval` not complaining about the\nunclosed here-doc? Is that something that can be fixed more generally?\nAre there other syntactic problems that go unnoticed?\n"},{"id":"474390","messageId":"CAPig+cQexJ9s1f6tLLaq7baeVCKhazPBi_xQ6uybkQcJQ97geg@mail.gmail.com","threadId":"59491","inReplyTo":"20230329030432.GA1801645@coredump.intra.peff.net","subject":"Re: [PATCH 3/4] tests: drop here-doc check from internal chain-linter","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2023-03-29T03:13:08Z","receivedAt":"2023-03-29T03:13:32Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Mar 28, 2023 at 11:04 PM Jeff King wrote:\n> On Tue, Mar 28, 2023 at 10:37:02PM -0400, Jeff King wrote:\n> > I just think chainlint.pl is doing a good enough job of catching it that\n> > we can rely on it. I'll be curious if Eric has input there on whether it\n> > can do even better, which would remove all of the caveats from the\n> > commit message.\n>\n> So I _think_ it's something like this:\n>\n> @@ -171,6 +171,9 @@ sub swallow_heredocs {\n> my $start = pos($$b);\n> my $indent = $tag =~ s/^\\t// ? '\\\\s*' : '';\n> $$b =~ /(?:\\G|\\n)$indent\\Q$tag\\E(?:\\n|\\z)/gc;\n> + if (pos($$b) == $start) {\n> + die \"oops, we did not find the end of the heredoc\";\n> + }\n> my $body = substr($$b, $start, pos($$b) - $start);\n> $self->{lineno} += () = $body =~ /\\n/sg;\n> }\n>\n> But I wasn't sure how to surface a clean error from here, since we're in\n> the Lexer. Maybe we just accumulate a \"problems\" array here, and then\n> roll those up via the TestParser? I'm not very familiar with the\n> arrangement of that part of the script.\n\nYes, it would look something like that and you chose the correct spot\nto detect the problem, but to get a \"pretty\" error message properly\npositioned in the input, we need to capture the input stream position\nof the here-doc tag itself in scan_heredoc_tag(). It doesn't look too\ndifficult, and I even started writing a bit of code to do it, but I'm\nnot sure how soon I can get around to finishing the implementation.\n\n> And I say \"think\" because the thing I was worried about is that we'd do\n> this lexing at too high a level, and accidentally walk past the end of\n> the test. Which would mean getting fooled by;\n>\n> test_expect_success 'this one is broken' '\n> cat >foo <<\\EOF\n> oops, we are missing our here-doc end\n> '\n>\n> test_expect_success 'this one is ok' '\n> cat >foo <<\\EOF\n> this one is OK, but we would not want to confuse\n> its closing tag for the missing one\n> EOF\n> '\n>\n> But it looks like Lexer::swallow_heredocs gets to see the individual\n> test snippets.\n\nCorrect. ScriptParser scans the input files for\ntest_expect_{success,failure} invocations in order to extract the\nindividual test snippets, which then get passed to TestParser for\nsemantic analysis.\n"},{"id":"474391","messageId":"CAPig+cQiOGrDSUc34jHEBp87Rx-dnXNcPcF76bu0SJoOzD+1hw@mail.gmail.com","threadId":"59491","inReplyTo":"CAPig+cQexJ9s1f6tLLaq7baeVCKhazPBi_xQ6uybkQcJQ97geg@mail.gmail.com","subject":"Re: [PATCH 3/4] tests: drop here-doc check from internal chain-linter","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2023-03-29T03:46:37Z","receivedAt":"2023-03-29T03:46:53Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Mar 28, 2023 at 11:13 PM Eric Sunshine wrote:\n> On Tue, Mar 28, 2023 at 11:04 PM Jeff King wrote:\n> > So I _think_ it's something like this:\n> >\n> > But I wasn't sure how to surface a clean error from here, since we're in\n> > the Lexer. Maybe we just accumulate a \"problems\" array here, and then\n> > roll those up via the TestParser? I'm not very familiar with the\n> > arrangement of that part of the script.\n>\n> Yes, it would look something like that and you chose the correct spot\n> to detect the problem, but to get a \"pretty\" error message properly\n> positioned in the input, we need to capture the input stream position\n> of the here-doc tag itself in scan_heredoc_tag(). It doesn't look too\n> difficult, and I even started writing a bit of code to do it, but I'm\n> not sure how soon I can get around to finishing the implementation.\n\nThe attached patch seems to do the job. Apologies for Gmail messing up\nthe whitespace. It's also attached to the email.\n\nThis would probably make a good preparatory patch to your [3/4]. As\nmentioned earlier in the thread, the changes to scan_heredoc_tag ()\ncapture the input-stream position of the here-doc tag itself, which is\nnecessary since it would be too late to do so by the time the error is\ndetected by swallow_heredocs(). I don't now when I'll get time to send\nthis as a proper patch, so feel free to write a commit message and\nincorporate it into your series if you want to use it. And, of course,\nyou have my sign-off already in the patch. It should be easy to add a\ntest, as well, in t/chainlint, perhaps as\nunclosed-here-doc.{text,expect}.\n\n--- >8 ---\nFrom b7103da900dd843aabb17201bc0f9ef0b7a704ba Mon Sep 17 00:00:00 2001\nFrom: Eric Sunshine \nDate: Tue, 28 Mar 2023 23:35:33 -0400\nSubject: [PATCH] chainlint: diagnose unclosed here-doc\n\nSigned-off-by: Eric Sunshine \n---\n t/chainlint.pl | 15 ++++++++++++---\n 1 file changed, 12 insertions(+), 3 deletions(-)\n\ndiff --git a/t/chainlint.pl b/t/chainlint.pl\nindex e966412999..3dac033ace 100755\n--- a/t/chainlint.pl\n+++ b/t/chainlint.pl\n@@ -80,7 +80,8 @@ sub scan_heredoc_tag {\n return \"<<$indented\" unless $token;\n my $tag = $token->[0];\n $tag =~ s/['\"\\\\]//g;\n- push(@{$self->{heretags}}, $indented ? \"\\t$tag\" : \"$tag\");\n+ $$token[0] = \"\\t$tag\" if $indented;\n+ push(@{$self->{heretags}}, $token);\n return \"<<$indented$tag\";\n }\n\n@@ -169,10 +170,18 @@ sub swallow_heredocs {\n my $tags = $self->{heretags};\n while (my $tag = shift @$tags) {\n my $start = pos($$b);\n- my $indent = $tag =~ s/^\\t// ? '\\\\s*' : '';\n- $$b =~ /(?:\\G|\\n)$indent\\Q$tag\\E(?:\\n|\\z)/gc;\n+ my $indent = $$tag[0] =~ s/^\\t// ? '\\\\s*' : '';\n+ $$b =~ /(?:\\G|\\n)$indent\\Q$$tag[0]\\E(?:\\n|\\z)/gc;\n+ if (pos($$b) > $start) {\n+ my $body = substr($$b, $start, pos($$b) - $start);\n+ $self->{lineno} += () = $body =~ /\\n/sg;\n+ next;\n+ }\n+ push(@{$self->{parser}->{problems}}, ['HERE', $tag]);\n+ $$b =~ /(?:\\G|\\n).*\\z/gc; # consume rest of input\n my $body = substr($$b, $start, pos($$b) - $start);\n $self->{lineno} += () = $body =~ /\\n/sg;\n+ last;\n }\n }\n\n--\n2.40.0.460.g7fdda0a984\n--- >8 ---\n\n\nFrom b7103da900dd843aabb17201bc0f9ef0b7a704ba Mon Sep 17 00:00:00 2001\nFrom: Eric Sunshine \nDate: Tue, 28 Mar 2023 23:35:33 -0400\nSubject: [PATCH] chainlint: diagnose unclosed here-doc\n\nSigned-off-by: Eric Sunshine \n---\n t/chainlint.pl | 15 ++++++++++++---\n 1 file changed, 12 insertions(+), 3 deletions(-)\n\ndiff --git a/t/chainlint.pl b/t/chainlint.pl\nindex e966412999..3dac033ace 100755\n--- a/t/chainlint.pl\n+++ b/t/chainlint.pl\n@@ -80,7 +80,8 @@ sub scan_heredoc_tag {\n \treturn \"<<$indented\" unless $token;\n \tmy $tag = $token->[0];\n \t$tag =~ s/['\"\\\\]//g;\n-\tpush(@{$self->{heretags}}, $indented ? \"\\t$tag\" : \"$tag\");\n+\t$$token[0] = \"\\t$tag\" if $indented;\n+\tpush(@{$self->{heretags}}, $token);\n \treturn \"<<$indented$tag\";\n }\n \n@@ -169,10 +170,18 @@ sub swallow_heredocs {\n \tmy $tags = $self->{heretags};\n \twhile (my $tag = shift @$tags) {\n \t\tmy $start = pos($$b);\n-\t\tmy $indent = $tag =~ s/^\\t// ? '\\\\s*' : '';\n-\t\t$$b =~ /(?:\\G|\\n)$indent\\Q$tag\\E(?:\\n|\\z)/gc;\n+\t\tmy $indent = $$tag[0] =~ s/^\\t// ? '\\\\s*' : '';\n+\t\t$$b =~ /(?:\\G|\\n)$indent\\Q$$tag[0]\\E(?:\\n|\\z)/gc;\n+\t\tif (pos($$b) > $start) {\n+\t\t\tmy $body = substr($$b, $start, pos($$b) - $start);\n+\t\t\t$self->{lineno} += () = $body =~ /\\n/sg;\n+\t\t\tnext;\n+\t\t}\n+\t\tpush(@{$self->{parser}->{problems}}, ['HERE', $tag]);\n+\t\t$$b =~ /(?:\\G|\\n).*\\z/gc; # consume rest of input\n \t\tmy $body = substr($$b, $start, pos($$b) - $start);\n \t\t$self->{lineno} += () = $body =~ /\\n/sg;\n+\t\tlast;\n \t}\n }\n \n-- \n2.40.0.460.g7fdda0a984\n\n"},{"id":"474392","messageId":"CAPig+cRH8Ca+s9zpmEx4O2v4q2s2Mqr2Axm_ryMan742da-KmA@mail.gmail.com","threadId":"59491","inReplyTo":"CAPig+cQiOGrDSUc34jHEBp87Rx-dnXNcPcF76bu0SJoOzD+1hw@mail.gmail.com","subject":"Re: [PATCH 3/4] tests: drop here-doc check from internal chain-linter","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2023-03-29T04:02:31Z","receivedAt":"2023-03-29T04:02:46Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Mar 28, 2023 at 11:46 PM Eric Sunshine wrote:\n> The attached patch seems to do the job. Apologies for Gmail messing up\n> the whitespace. It's also attached to the email.\n>\n> $tag =~ s/['\"\\\\]//g;\n> - push(@{$self->{heretags}}, $indented ? \"\\t$tag\" : \"$tag\");\n> + $$token[0] = \"\\t$tag\" if $indented;\n> + push(@{$self->{heretags}}, $token);\n> return \"<<$indented$tag\";\n\nBah, the `$$token[0] = ...` line is incorrect. It should be:\n\n $$token[0] = $indented ? \"\\t$tag\" : \"$tag\";\n\nwhich more correctly matches the original code. Without this fix,\n$$token[0] only gets the cleaned tag name in the `<<-EOF` case but not\nin the plain `< > Yes, it would look something like that and you chose the correct spot\n> > to detect the problem, but to get a \"pretty\" error message properly\n> > positioned in the input, we need to capture the input stream position\n> > of the here-doc tag itself in scan_heredoc_tag(). It doesn't look too\n> > difficult, and I even started writing a bit of code to do it, but I'm\n> > not sure how soon I can get around to finishing the implementation.\n> \n> The attached patch seems to do the job. Apologies for Gmail messing up\n> the whitespace. It's also attached to the email.\n\nThanks! I was going to say \"please don't consider this urgent\", but I\nsee that my nerd-snipe was successful. ;)\n\n> This would probably make a good preparatory patch to your [3/4]. As\n> mentioned earlier in the thread, the changes to scan_heredoc_tag ()\n> capture the input-stream position of the here-doc tag itself, which is\n> necessary since it would be too late to do so by the time the error is\n> detected by swallow_heredocs(). I don't now when I'll get time to send\n> this as a proper patch, so feel free to write a commit message and\n> incorporate it into your series if you want to use it. And, of course,\n> you have my sign-off already in the patch. It should be easy to add a\n> test, as well, in t/chainlint, perhaps as\n> unclosed-here-doc.{text,expect}.\n\nThanks, I can take it from here (and I agree doing it as prep for 3/4 is\ngood, as I can then omit a bunch of explanations there). Here are the\ntests I'll squash in (along with your $indent fix):\n\ndiff --git a/t/chainlint/unclosed-here-doc-indent.expect b/t/chainlint/unclosed-here-doc-indent.expect\nnew file mode 100644\nindex 00000000000..6e17bb66336\n--- /dev/null\n+++ b/t/chainlint/unclosed-here-doc-indent.expect\n@@ -0,0 +1,4 @@\n+command_which_is_run &&\n+cat >expect <<-\\EOF ?!HERE?! &&\n+we forget to end the here-doc\n+command_which_is_gobbled\ndiff --git a/t/chainlint/unclosed-here-doc-indent.test b/t/chainlint/unclosed-here-doc-indent.test\nnew file mode 100644\nindex 00000000000..5c841a9dfd4\n--- /dev/null\n+++ b/t/chainlint/unclosed-here-doc-indent.test\n@@ -0,0 +1,4 @@\n+command_which_is_run &&\n+cat >expect <<-\\EOF &&\n+we forget to end the here-doc\n+command_which_is_gobbled\ndiff --git a/t/chainlint/unclosed-here-doc.expect b/t/chainlint/unclosed-here-doc.expect\nnew file mode 100644\nindex 00000000000..c53b6b794a7\n--- /dev/null\n+++ b/t/chainlint/unclosed-here-doc.expect\n@@ -0,0 +1,7 @@\n+command_which_is_run &&\n+cat >expect <<\\EOF ?!HERE?! &&\n+\twe try to end the here-doc below,\n+\tbut the indentation throws us off\n+\tsince the operator is not \"<<-\".\n+\tEOF\n+command_which_is_gobbled\ndiff --git a/t/chainlint/unclosed-here-doc.test b/t/chainlint/unclosed-here-doc.test\nnew file mode 100644\nindex 00000000000..69d3786c348\n--- /dev/null\n+++ b/t/chainlint/unclosed-here-doc.test\n@@ -0,0 +1,7 @@\n+command_which_is_run &&\n+cat >expect <<\\EOF &&\n+\twe try to end the here-doc below,\n+\tbut the indentation throws us off\n+\tsince the operator is not \"<<-\".\n+\tEOF\n+command_which_is_gobbled\n\n-Peff\n"},{"id":"474394","messageId":"20230329062843.GC1793752@coredump.intra.peff.net","threadId":"59491","inReplyTo":"CAPig+cSMYt81fcU8fRNhN9qZ=95jvRNv9YGhkM6PxgdMywWnCA@mail.gmail.com","subject":"Re: [PATCH 3/4] tests: drop here-doc check from internal chain-linter","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-03-29T06:28:43Z","receivedAt":"2023-03-29T06:28:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 28, 2023 at 11:07:17PM -0400, Eric Sunshine wrote:\n\n> > I just think chainlint.pl is doing a good enough job of catching it that\n> > we can rely on it. I'll be curious if Eric has input there on whether it\n> > can do even better, which would remove all of the caveats from the\n> > commit message.\n> \n> It was an intentional design choice, for the sake of simplicity, _not_\n> to make chainlint.pl a shell syntax checker, despite the fact that it\n> is genuinely parsing shell code. After all, the shell itself -- by\n> which test code will ultimately be executed -- is more than capable of\n> diagnosing syntax errors, so why burden chainlint.pl with all that\n> extra baggage? Instead, chainlint.pl is focussed on detecting semantic\n> problems -- such as broken &&-chain and missing `return 1` -- which\n> are of importance to _this_ project.\n\nYeah, I'm not too surprised to hear any of that, and I almost wrote off\nchainlint.pl for this purpose (hence the hand-waving in my commit\nmessage). But after realizing it has to find here-docs anyway to ignore\nthem, it seemed reasonable to bend it to my will. ;)\n\nThanks again for your patch.\n\n> [1]: Why is that, by the way? Is `eval` not complaining about the\n> unclosed here-doc? Is that something that can be fixed more generally?\n> Are there other syntactic problems that go unnoticed?\n\nThe behavior varies from shell to shell. Historically, I suspect it was\na deliberate decision to read until EOF, because it lets you stick\narbitrary data at the end of a script, like:\n\n $ cat foo.sh\n my_prog <<\\EOF\n 1 2 3 4\n 5 6 7 8\n [imagine kilobytes of data tables here]\n\nwithout having to worry about terminating it. I think I've seen it with\nsomething like:\n\n {\n echo \"uudecode <<\\EOF | tar tf -\"\n tar cf - Documentation | uuencode -\n } >foo.sh\n\nwhich makes foo.sh a sort of self-extracting tarball. (As an aside, I\nwas disappointed that I did not have uuencode installed by default on my\nsystem. How times have changed. ;) ).\n\nThese days bash will warn about it, but dash will not:\n\n $ bash foo.sh | wc -l\n foo.sh: line 129028: warning: here-document at line 1 delimited by end-of-file (wanted `EOF')\n 901\n\n $ dash foo.sh | wc -l\n 901\n\nBash still has a zero exit code, though, so aside from the stderr cruft\n(which is hidden unless you are digging in \"-v\" output), the tests would\npass. I can't remember when bash started warning, though \"git log -S\" in\nits repo suggests it was bash 4.0 in 2009.\n\n-Peff\n"},{"id":"474395","messageId":"CAPig+cRO7cDkn5XsRq+9OdkdGQL-UL12-i2Tm9jJFO0mbYJtEA@mail.gmail.com","threadId":"59491","inReplyTo":"20230329060709.GB1793752@coredump.intra.peff.net","subject":"Re: [PATCH 3/4] tests: drop here-doc check from internal chain-linter","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2023-03-29T06:28:54Z","receivedAt":"2023-03-29T06:29:16Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Mar 29, 2023 at 2:07 AM Jeff King wrote:\n> On Tue, Mar 28, 2023 at 11:46:37PM -0400, Eric Sunshine wrote:\n> > The attached patch seems to do the job. Apologies for Gmail messing up\n> > the whitespace. It's also attached to the email.\n>\n> Thanks! I was going to say \"please don't consider this urgent\", but I\n> see that my nerd-snipe was successful. ;)\n\nAs always. My nerd-snipe armor is in failure mode.\n\n> > This would probably make a good preparatory patch to your [3/4]. As\n> > mentioned earlier in the thread, the changes to scan_heredoc_tag ()\n> > capture the input-stream position of the here-doc tag itself, which is\n> > necessary since it would be too late to do so by the time the error is\n> > detected by swallow_heredocs(). I don't now when I'll get time to send\n> > this as a proper patch, so feel free to write a commit message and\n> > incorporate it into your series if you want to use it. And, of course,\n> > you have my sign-off already in the patch. It should be easy to add a\n> > test, as well, in t/chainlint, perhaps as\n> > unclosed-here-doc.{text,expect}.\n>\n> Thanks, I can take it from here (and I agree doing it as prep for 3/4 is\n> good, as I can then omit a bunch of explanations there). Here are the\n> tests I'll squash in (along with your $indent fix):\n\nThanks for picking this up.\n\n> diff --git a/t/chainlint/unclosed-here-doc-indent.expect b/t/chainlint/unclosed-here-doc-indent.expect\n> diff --git a/t/chainlint/unclosed-here-doc-indent.test b/t/chainlint/unclosed-here-doc-indent.test\n> diff --git a/t/chainlint/unclosed-here-doc.expect b/t/chainlint/unclosed-here-doc.expect\n> diff --git a/t/chainlint/unclosed-here-doc.test b/t/chainlint/unclosed-here-doc.test\n\nThe new tests look fine.\n"},{"id":"474401","messageId":"230329.86wn2zvo9u.gmgdl@evledraar.gmail.com","threadId":"59491","inReplyTo":"20230328202207.GA1241631@coredump.intra.peff.net","subject":"Re: [PATCH 1/4] tests: run internal chain-linter under \"make test\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2023-03-29T10:20:00Z","receivedAt":"2023-03-29T10:35:27Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Mar 28 2023, Jeff King wrote:\n\n> Since 69b9924b875 (t/Makefile: teach `make test` and `make prove` to run\n> chainlint.pl, 2022-09-01), we run a single chainlint.pl process for all\n> scripts, and then instruct each individual script to run with the\n> equivalent of --no-chain-lint, which tells them not to redundantly run\n> the chainlint script themselves.\n>\n> However, this also disables the internal linter run within the shell by\n> eval-ing \"(exit 117) && $1\" and confirming we get code 117. In theory\n> the external linter produces a superset of complaints, and we don't need\n> the internal one anymore. However, we know there is at least one case\n> where they differ. A test like:\n>\n> \ttest_expect_success 'should fail linter' '\n> \t\tfalse &&\n> \t\tsleep 2 &\n> \t\tpid=$! &&\n> \t\tkill $pid\n> \t'\n>\n> is buggy (it ignores the failure from \"false\", because it is\n> backgrounded along with the sleep). The internal linter catches this,\n> but the external one doesn't (and teaching it to do so is complicated[1]).\n> So not only does \"make test\" miss this problem, but it's doubly\n> confusing because running the script standalone does complain.\n>\n> Let's teach the suppression in the Makefile to only turn off the\n> external linter (which we know is redundant, as it was already run) and\n> leave the internal one intact.\n>\n> I've used a new environment variable to do this here, and intentionally\n> did not add a \"--no-ext-chain-lint\" option. This is an internal\n> optimization used by the Makefile, and not something that ordinary users\n> would need to tweak.\n>\n> [1] For discussion of chainlint.pl and this case, see:\n> https://lore.kernel.org/git/CAPig+cQtLFX4PgXyyK_AAkCvg4Aw2RAC5MmLbib-aHHgTBcDuw@mail.gmail.com/\n>\n> Signed-off-by: Jeff King \n> ---\n> t/Makefile | 2 +-\n> t/test-lib.sh | 3 ++-\n> 2 files changed, 3 insertions(+), 2 deletions(-)\n>\n> diff --git a/t/Makefile b/t/Makefile\n> index 88fa5049573..10881affdd0 100644\n> --- a/t/Makefile\n> +++ b/t/Makefile\n> @@ -45,7 +45,7 @@ CHAINLINT = '$(PERL_PATH_SQ)' chainlint.pl\n> # `test-chainlint` (which is a dependency of `test-lint`, `test` and `prove`)\n> # checks all tests in all scripts via a single invocation, so tell individual\n> # scripts not to \"chainlint\" themselves\n> -CHAINLINTSUPPRESS = GIT_TEST_CHAIN_LINT=0 && export GIT_TEST_CHAIN_LINT &&\n> +CHAINLINTSUPPRESS = GIT_TEST_EXT_CHAIN_LINT=0 && export GIT_TEST_EXT_CHAIN_LINT &&\n> \n> all: $(DEFAULT_TEST_TARGET)\n> \n> diff --git a/t/test-lib.sh b/t/test-lib.sh\n> index 62136caee5a..09789566374 100644\n> --- a/t/test-lib.sh\n> +++ b/t/test-lib.sh\n> @@ -1593,7 +1593,8 @@ then\n> \tBAIL_OUT_ENV_NEEDS_SANITIZE_LEAK \"GIT_TEST_SANITIZE_LEAK_LOG=true\"\n> fi\n> \n> -if test \"${GIT_TEST_CHAIN_LINT:-1}\" != 0\n> +if test \"${GIT_TEST_CHAIN_LINT:-1}\" != 0 &&\n> + test \"${GIT_TEST_EXT_CHAIN_LINT:-1}\" != 0\n> then\n> \t\"$PERL_PATH\" \"$TEST_DIRECTORY/chainlint.pl\" \"$0\" ||\n> \t\tBUG \"lint error (see '?!...!? annotations above)\"\n\nSo, we used to use the \"sed\" script to run against the *.sh, and do the\n\"eval 117\" check, but since chainlint.pl we've checked all the tests in\none go when invoked from the Makefile, and turned off the \"eval 117\".\n\nAnd, as this change points out, there's cases where chainlint.pl won't\ncatch everything, so we'd like to keep the \"eval\" bits by default, but\nnot the invoking of chainlint.pl by the script itself.\n\nAll of which makes sense.\n\nBut it seems to me that the findings in this change make the docs\noutdated, and we should update them, but maybe I'm missing something.\n\nNow the comment in the Makefile still says (as seen in the context):\n\n\t# `test-chainlint` (which is a dependency of `test-lint`, `test` and `prove`)\n\t# checks all tests in all scripts via a single invocation, so tell individual\n\t# scripts not to \"chainlint\" themselves\n\nBut as the commit message notes that's not accurate anymore. We're *not*\ntelling them to turn off chainlint in its entirety, we're telling them\nto only suppress the chainlint.pl part.\n\nSo if you invoke the Makefile with GIT_TEST_CHAIN_LINT=0 we'll still\nturn all of it off, but an invokation of chainlint.pl for the scripts as\na batch is no longer thought to make the \"eval 117\" trick redundant.\n\nI haven't looked too deeply, but it seems that we should at least adjust\nthat comment in the Makefile, or perhaps we should rename this \"eval\n117\" thing to be \"internal lint\" or something, and not \"chain lint\", to\navoid conflating it with chainlint.pl itself.\n\nSo maybe something like this, i.e. we'd just \"rebrand\" this to begin\nwith, and avoid editing the t/Makefile at all (untested)?\n\n t/README | 12 ++++++++++++\n t/test-lib.sh | 6 +++++-\n 2 files changed, 17 insertions(+), 1 deletion(-)\n\ndiff --git a/t/README b/t/README\nindex 29576c37488..83d58f2e680 100644\n--- a/t/README\n+++ b/t/README\n@@ -196,6 +196,18 @@ appropriately before running \"make\". Short options can be bundled, i.e.\n \tthis feature by setting the GIT_TEST_CHAIN_LINT environment\n \tvariable to \"1\" or \"0\", respectively.\n \n+--internal-lint::\n+--no-internal-lint::\n+\tIf --internal-lint is enabled, the test harness will try to\n+\tdetect missing \"&&\"-chains in edge cases that \"--chain-lint\"\n+\twouldn't notice, unlike \"--chain-lint\" this check is run from\n+\twithin \"test_expect_*\" itself, and thus requires the tests to\n+\trun.\n+\n+\tYou may also enable or disable this feature by setting the\n+\tGIT_TEST_INTERNAL_LINT environment variable to \"1\" or \"0\",\n+\trespectively.\n+\n --stress::\n \tRun the test script repeatedly in multiple parallel jobs until\n \tone of them fails. Useful for reproducing rare failures in\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 62136caee5a..792c68aa56a 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -205,6 +205,10 @@ parse_option () {\n \t\tGIT_TEST_CHAIN_LINT=1 ;;\n \t--no-chain-lint)\n \t\tGIT_TEST_CHAIN_LINT=0 ;;\n+\t--internal-lint)\n+\t\tGIT_TEST_INTERNAL_LINT=1 ;;\n+\t--no-internal-lint)\n+\t\tGIT_TEST_INTERNAL_LINT=0 ;;\n \t-x)\n \t\ttrace=t ;;\n \t-V|--verbose-log)\n@@ -1090,7 +1094,7 @@ test_run_ () {\n \ttest_cleanup=:\n \texpecting_failure=$2\n \n-\tif test \"${GIT_TEST_CHAIN_LINT:-1}\" != 0; then\n+\tif test \"${GIT_TEST_INTERNAL_LINT:-1}\" != 0; then\n \t\t# turn off tracing for this test-eval, as it simply creates\n \t\t# confusing noise in the \"-x\" output\n \t\ttrace_tmp=$trace\n"},{"id":"474411","messageId":"xmqq7cuzd0cs.fsf@gitster.g","threadId":"59491","inReplyTo":"230329.86wn2zvo9u.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 1/4] tests: run internal chain-linter under \"make test\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-29T15:49:07Z","receivedAt":"2023-03-29T15:49:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> Now the comment in the Makefile still says (as seen in the context):\n>\n> \t# `test-chainlint` (which is a dependency of `test-lint`, `test` and `prove`)\n> \t# checks all tests in all scripts via a single invocation, so tell individual\n> \t# scripts not to \"chainlint\" themselves\n>\n> But as the commit message notes that's not accurate anymore. We're *not*\n> telling them to turn off chainlint in its entirety, we're telling them\n> to only suppress the chainlint.pl part.\n\nCorrect. To avoid a confusion we saw in the thread that led to this\nseries, we need an explanation that clearly distinguishes the \"exit\n117\" one and the \"script that parses the shell\" one. If we consider\nthat the name \"chainlint\" refers to the latter, the script, and\nre-read the three lines with that in mind, I think they are OK. It\nwould become even clearer if we insert four words, like so:\n\n\t`test-chainlint' (which is ...) checks ... via a single\n\tinvocation of the \"chainlint\" script, so tell ...\n\n> So if you invoke the Makefile with GIT_TEST_CHAIN_LINT=0 we'll still\n> turn all of it off, but an invokation of chainlint.pl for the scripts as\n> a batch is no longer thought to make the \"eval 117\" trick redundant.\n\nYes, I think that is the idea reached by the discussion in the other\nthread that led to this series.\n\n> I haven't looked too deeply, but it seems that we should at least adjust\n> that comment in the Makefile, or perhaps we should rename this \"eval\n> 117\" thing to be \"internal lint\" or something, and not \"chain lint\", to\n> avoid conflating it with chainlint.pl itself.\n\nI agree that it would help to clarify that we mean by \"chainlint\"\nthe script that parses (or the act of running the script, when it is\nused as a verb), and the above may be a way to do so. I however do\nnot see the need to come up with a new name for the other one, until\nwe have a way to toggle it enabled/disabled, which is not something\nthe discussion in the other thread found necessary.\n\nThanks.\n"},{"id":"474436","messageId":"20230329232804.GC2314218@coredump.intra.peff.net","threadId":"59491","inReplyTo":"xmqq7cuzd0cs.fsf@gitster.g","subject":"Re: [PATCH 1/4] tests: run internal chain-linter under \"make test\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-03-29T23:28:04Z","receivedAt":"2023-03-29T23:28:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 29, 2023 at 08:49:07AM -0700, Junio C Hamano wrote:\n\n> Ævar Arnfjörð Bjarmason writes:\n> \n> > Now the comment in the Makefile still says (as seen in the context):\n> >\n> > \t# `test-chainlint` (which is a dependency of `test-lint`, `test` and `prove`)\n> > \t# checks all tests in all scripts via a single invocation, so tell individual\n> > \t# scripts not to \"chainlint\" themselves\n> >\n> > But as the commit message notes that's not accurate anymore. We're *not*\n> > telling them to turn off chainlint in its entirety, we're telling them\n> > to only suppress the chainlint.pl part.\n> \n> Correct. To avoid a confusion we saw in the thread that led to this\n> series, we need an explanation that clearly distinguishes the \"exit\n> 117\" one and the \"script that parses the shell\" one. If we consider\n> that the name \"chainlint\" refers to the latter, the script, and\n> re-read the three lines with that in mind, I think they are OK. It\n> would become even clearer if we insert four words, like so:\n> \n> \t`test-chainlint' (which is ...) checks ... via a single\n> \tinvocation of the \"chainlint\" script, so tell ...\n\nI had hoped that \"chainlint\" in that comment would remain sufficient, as\nthe context implies that we're disabling the script. But it's easy\nenough to expand. I squashed this in:\n\ndiff --git a/t/Makefile b/t/Makefile\nindex 10881affdd0..3e00cdd801d 100644\n--- a/t/Makefile\n+++ b/t/Makefile\n@@ -44,7 +44,7 @@ CHAINLINT = '$(PERL_PATH_SQ)' chainlint.pl\n \n # `test-chainlint` (which is a dependency of `test-lint`, `test` and `prove`)\n # checks all tests in all scripts via a single invocation, so tell individual\n-# scripts not to \"chainlint\" themselves\n+# scripts not to run the external \"chainlint.pl\" script themselves\n CHAINLINTSUPPRESS = GIT_TEST_EXT_CHAIN_LINT=0 && export GIT_TEST_EXT_CHAIN_LINT &&\n \n all: $(DEFAULT_TEST_TARGET)\n\n> > I haven't looked too deeply, but it seems that we should at least adjust\n> > that comment in the Makefile, or perhaps we should rename this \"eval\n> > 117\" thing to be \"internal lint\" or something, and not \"chain lint\", to\n> > avoid conflating it with chainlint.pl itself.\n> \n> I agree that it would help to clarify that we mean by \"chainlint\"\n> the script that parses (or the act of running the script, when it is\n> used as a verb), and the above may be a way to do so. I however do\n> not see the need to come up with a new name for the other one, until\n> we have a way to toggle it enabled/disabled, which is not something\n> the discussion in the other thread found necessary.\n\nYeah. This distinction is not something anybody who is running or\nwriting tests should have to worry about (and the fact that it did come\nup was a bug that this patch is fixing). So adding an option like\n\"--no-chain-lint-internal\" is just making things more confusing, and\nnobody would use it. You'd need it only if you are working on\nchainlint.pl itself (and comparing results with the internal linter or\nsomething), and there you are better off running \"perl chainlint.pl\nt1234-foo.sh\" itself.\n\n-Peff\n"},{"id":"474476","messageId":"xmqq8rfejcxz.fsf@gitster.g","threadId":"59491","inReplyTo":"20230329232804.GC2314218@coredump.intra.peff.net","subject":"Re: [PATCH 1/4] tests: run internal chain-linter under \"make test\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-30T18:45:12Z","receivedAt":"2023-03-30T18:45:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I had hoped that \"chainlint\" in that comment would remain sufficient, as\n> the context implies that we're disabling the script. But it's easy\n> enough to expand. I squashed this in:\n>\n> diff --git a/t/Makefile b/t/Makefile\n> index 10881affdd0..3e00cdd801d 100644\n> --- a/t/Makefile\n> +++ b/t/Makefile\n> @@ -44,7 +44,7 @@ CHAINLINT = '$(PERL_PATH_SQ)' chainlint.pl\n> \n> # `test-chainlint` (which is a dependency of `test-lint`, `test` and `prove`)\n> # checks all tests in all scripts via a single invocation, so tell individual\n> -# scripts not to \"chainlint\" themselves\n> +# scripts not to run the external \"chainlint.pl\" script themselves\n\nOK. I've taken it and did \"rebase -i\" on this end.\n\nThanks.\n"},{"id":"474481","messageId":"20230330192712.GA27719@coredump.intra.peff.net","threadId":"59491","inReplyTo":"20230328202043.GA1241391@coredump.intra.peff.net","subject":"[PATCH v2 0/5] some chainlint fixes and performance improvements","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-03-30T19:27:12Z","receivedAt":"2023-03-30T19:27:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 28, 2023 at 04:20:44PM -0400, Jeff King wrote:\n\n> Here are a few fixes for chainlint.\n\nAnd here's a re-roll.\n\nAs before, I think the first patch is the most important, and the rest\nare optimizations. But with Eric's patch to chainlint.pl in the middle,\nI think the argument for patch 4 (previously patch 3) is much stronger.\n\nPatch 5 remains mostly a cleanup, with no performance improvement. IMHO\nthe result is easier to follow, but I'm open to arguments to the\ncontrary.\n\n [1/5]: tests: run internal chain-linter under \"make test\"\n [2/5]: tests: replace chainlint subshell with a function\n [3/5]: tests: diagnose unclosed here-doc in chainlint.pl\n [4/5]: tests: drop here-doc check from internal chain-linter\n [5/5]: tests: skip test_eval_ in internal chain-lint\n\n t/Makefile | 4 +--\n t/chainlint.pl | 15 +++++++++---\n t/chainlint/unclosed-here-doc-indent.expect | 4 +++\n t/chainlint/unclosed-here-doc-indent.test | 4 +++\n t/chainlint/unclosed-here-doc.expect | 7 ++++++\n t/chainlint/unclosed-here-doc.test | 7 ++++++\n t/test-lib.sh | 27 +++++++++++----------\n 7 files changed, 50 insertions(+), 18 deletions(-)\n create mode 100644 t/chainlint/unclosed-here-doc-indent.expect\n create mode 100644 t/chainlint/unclosed-here-doc-indent.test\n create mode 100644 t/chainlint/unclosed-here-doc.expect\n create mode 100644 t/chainlint/unclosed-here-doc.test\n\nRange-diff:\n\n1: 19deb7195df ! 1: d536d3b9ec0 tests: run internal chain-linter under \"make test\"\n @@ Commit message\n \n ## t/Makefile ##\n @@ t/Makefile: CHAINLINT = '$(PERL_PATH_SQ)' chainlint.pl\n + \n # `test-chainlint` (which is a dependency of `test-lint`, `test` and `prove`)\n # checks all tests in all scripts via a single invocation, so tell individual\n - # scripts not to \"chainlint\" themselves\n +-# scripts not to \"chainlint\" themselves\n -CHAINLINTSUPPRESS = GIT_TEST_CHAIN_LINT=0 && export GIT_TEST_CHAIN_LINT &&\n ++# scripts not to run the external \"chainlint.pl\" script themselves\n +CHAINLINTSUPPRESS = GIT_TEST_EXT_CHAIN_LINT=0 && export GIT_TEST_EXT_CHAIN_LINT &&\n \n all: $(DEFAULT_TEST_TARGET)\n2: a05c440dde5 = 2: fa29c781fca tests: replace chainlint subshell with a function\n-: ----------- > 3: c1a3ec3619e tests: diagnose unclosed here-doc in chainlint.pl\n3: 46556678938 ! 4: b5dc3618c83 tests: drop here-doc check from internal chain-linter\n @@ Commit message\n run for many tests.\n \n The tradeoff in safety was undoubtedly worth it when 99a64e4b73c was\n - written. But these days, the external chainlint.pl does a pretty good\n - job of finding these (even though it's not something it specifically\n - tries to flag). For example, if you have a test like:\n -\n - test_expect_success 'should fail linter' '\n - some_command >actual &&\n - cat >expect <<-\\EOF &&\n - ok\n - # missing EOF line here\n - test_cmp expect actual\n - '\n -\n - it will see that the here-doc isn't closed, treat it as not-a-here-doc,\n - and complain that the \"ok\" line does not have an \"&&\". So in practice we\n - should be catching these via that linter, although:\n -\n - - the error message is not as good as it could be (the real problem is\n - the unclosed here-doc)\n -\n - - it can be fooled if there are no lines in the here-doc:\n -\n - cat >expect <<-\\EOF &&\n - # missing EOF line here\n -\n - or if every line in the here-doc has &&-chaining (weird, but\n - possible)\n -\n - Those are sufficiently unlikely that they're not worth worrying too much\n - about. And by switching back to a simpler chain-lint, hyperfine reports\n - a measurable speedup on t3070 (which has 1800 tests):\n + written. But since the external chainlint.pl learned to find these\n + recently, we can just rely on it. By switching back to a simpler\n + chain-lint, hyperfine reports a measurable speedup on t3070 (which has\n + 1800 tests):\n \n 'HEAD' ran\n 1.12 ± 0.01 times faster than 'HEAD~1'\n4: f810780d326 = 5: 0ebf1da8b93 tests: skip test_eval_ in internal chain-lint\n\n"},{"id":"474482","messageId":"20230330192738.GA27989@coredump.intra.peff.net","threadId":"59491","inReplyTo":"20230330192712.GA27719@coredump.intra.peff.net","subject":"[PATCH v2 1/5] tests: run internal chain-linter under \"make test\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-03-30T19:27:38Z","receivedAt":"2023-03-30T19:27:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Since 69b9924b875 (t/Makefile: teach `make test` and `make prove` to run\nchainlint.pl, 2022-09-01), we run a single chainlint.pl process for all\nscripts, and then instruct each individual script to run with the\nequivalent of --no-chain-lint, which tells them not to redundantly run\nthe chainlint script themselves.\n\nHowever, this also disables the internal linter run within the shell by\neval-ing \"(exit 117) && $1\" and confirming we get code 117. In theory\nthe external linter produces a superset of complaints, and we don't need\nthe internal one anymore. However, we know there is at least one case\nwhere they differ. A test like:\n\n\ttest_expect_success 'should fail linter' '\n\t\tfalse &&\n\t\tsleep 2 &\n\t\tpid=$! &&\n\t\tkill $pid\n\t'\n\nis buggy (it ignores the failure from \"false\", because it is\nbackgrounded along with the sleep). The internal linter catches this,\nbut the external one doesn't (and teaching it to do so is complicated[1]).\nSo not only does \"make test\" miss this problem, but it's doubly\nconfusing because running the script standalone does complain.\n\nLet's teach the suppression in the Makefile to only turn off the\nexternal linter (which we know is redundant, as it was already run) and\nleave the internal one intact.\n\nI've used a new environment variable to do this here, and intentionally\ndid not add a \"--no-ext-chain-lint\" option. This is an internal\noptimization used by the Makefile, and not something that ordinary users\nwould need to tweak.\n\n[1] For discussion of chainlint.pl and this case, see:\n https://lore.kernel.org/git/CAPig+cQtLFX4PgXyyK_AAkCvg4Aw2RAC5MmLbib-aHHgTBcDuw@mail.gmail.com/\n\nSigned-off-by: Jeff King \n---\nSame as before, but tweaking the Makefile comment.\n\n t/Makefile | 4 ++--\n t/test-lib.sh | 3 ++-\n 2 files changed, 4 insertions(+), 3 deletions(-)\n\ndiff --git a/t/Makefile b/t/Makefile\nindex 88fa5049573..3e00cdd801d 100644\n--- a/t/Makefile\n+++ b/t/Makefile\n@@ -44,8 +44,8 @@ CHAINLINT = '$(PERL_PATH_SQ)' chainlint.pl\n \n # `test-chainlint` (which is a dependency of `test-lint`, `test` and `prove`)\n # checks all tests in all scripts via a single invocation, so tell individual\n-# scripts not to \"chainlint\" themselves\n-CHAINLINTSUPPRESS = GIT_TEST_CHAIN_LINT=0 && export GIT_TEST_CHAIN_LINT &&\n+# scripts not to run the external \"chainlint.pl\" script themselves\n+CHAINLINTSUPPRESS = GIT_TEST_EXT_CHAIN_LINT=0 && export GIT_TEST_EXT_CHAIN_LINT &&\n \n all: $(DEFAULT_TEST_TARGET)\n \ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 62136caee5a..09789566374 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -1593,7 +1593,8 @@ then\n \tBAIL_OUT_ENV_NEEDS_SANITIZE_LEAK \"GIT_TEST_SANITIZE_LEAK_LOG=true\"\n fi\n \n-if test \"${GIT_TEST_CHAIN_LINT:-1}\" != 0\n+if test \"${GIT_TEST_CHAIN_LINT:-1}\" != 0 &&\n+ test \"${GIT_TEST_EXT_CHAIN_LINT:-1}\" != 0\n then\n \t\"$PERL_PATH\" \"$TEST_DIRECTORY/chainlint.pl\" \"$0\" ||\n \t\tBUG \"lint error (see '?!...!? annotations above)\"\n-- \n2.40.0.692.g7c4c956fc5c\n\n"},{"id":"474483","messageId":"20230330192749.GB27989@coredump.intra.peff.net","threadId":"59491","inReplyTo":"20230330192712.GA27719@coredump.intra.peff.net","subject":"[PATCH v2 2/5] tests: replace chainlint subshell with a function","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-03-30T19:27:49Z","receivedAt":"2023-03-30T19:28:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"To test that we don't break the &&-chain, test-lib.sh does something\nlike:\n\n (exit 117) && $test_commands\n\nand checks that the result is exit code 117. We don't care what that\ninitial command is, as long as it exits with a unique code. Using \"exit\"\nworks and is simple, but is a bit expensive since it requires a subshell\n(to avoid exiting the whole script!). This isn't usually very\nnoticeable, but it can add up for scripts which have a large number of\ntests.\n\nUsing \"return\" naively won't work here, because we'd return from the\nfunction eval-ing the snippet (and it wouldn't find &&-chain breakages).\nBut if we further push that into its own function, it does exactly what\nwe want, without extra subshell overhead.\n\nAccording to hyperfine, this produces a measurable improvement when\nrunning t3070 (which has 1800 tests, all of them quite short):\n\n 'HEAD' ran\n 1.09 ± 0.01 times faster than 'HEAD~1'\n\nSigned-off-by: Jeff King \n---\nSame as before.\n\n t/test-lib.sh | 6 +++++-\n 1 file changed, 5 insertions(+), 1 deletion(-)\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 09789566374..cfcbd899c5a 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -1086,6 +1086,10 @@ test_eval_ () {\n \treturn $test_eval_ret_\n }\n \n+fail_117 () {\n+\treturn 117\n+}\n+\n test_run_ () {\n \ttest_cleanup=:\n \texpecting_failure=$2\n@@ -1097,7 +1101,7 @@ test_run_ () {\n \t\ttrace=\n \t\t# 117 is magic because it is unlikely to match the exit\n \t\t# code of other programs\n-\t\tif test \"OK-117\" != \"$(test_eval_ \"(exit 117) && $1${LF}${LF}echo OK-\\$?\" 3>&1)\"\n+\t\tif test \"OK-117\" != \"$(test_eval_ \"fail_117 && $1${LF}${LF}echo OK-\\$?\" 3>&1)\"\n \t\tthen\n \t\t\tBUG \"broken &&-chain or run-away HERE-DOC: $1\"\n \t\tfi\n-- \n2.40.0.692.g7c4c956fc5c\n\n"},{"id":"474484","messageId":"20230330193031.GC27989@coredump.intra.peff.net","threadId":"59491","inReplyTo":"20230330192712.GA27719@coredump.intra.peff.net","subject":"[PATCH v2 3/5] tests: diagnose unclosed here-doc in chainlint.pl","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-03-30T19:30:31Z","receivedAt":"2023-03-30T19:30:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"From: Eric Sunshine \n\nAn unclosed here-doc in a test is a problem, because it silently gobbles\nup any remaining commands. Since 99a64e4b73c (tests: lint for run-away\nhere-doc, 2017-03-22) we detect this by piggy-backing on the internal\nchainlint checker in test-lib.sh.\n\nHowever, it would be nice to detect it in chainlint.pl, for a few\nreasons:\n\n - the output from chainlint.pl is much nicer; it can show the exact\n spot of the error, rather than a vague \"somewhere in this test you\n broke the &&-chain or had a bad here-doc\" message.\n\n - the implementation in test-lib.sh runs for each test snippet. And\n since it requires a subshell, the extra cost is small but not zero.\n If chainlint.pl can reliably find the problem, we can optimize the\n test-lib.sh code.\n\nThe chainlint.pl code never intended to find here-doc problems. But\nsince it has to parse them anyway (to avoid reporting problems inside\nhere-docs), most of what we need is already there. We can detect the\nproblem when we fail to find the missing end-tag in swallow_heredocs().\nThe extra change in scan_heredoc_tag() stores the location of the start\nof the here-doc, which lets us mark it as the source of the error in the\noutput (see the new tests for examples).\n\n[jk: added commit message and tests]\n\nSigned-off-by: Eric Sunshine \nSigned-off-by: Jeff King \n---\nNew in this iteration (thanks again, Eric!).\n\nI changed the error-tag from \"HERE\" to \"UNCLOSED-HEREDOC\". I don't think\nthere any syntactic limitations to worry about (it's just shown to the\nuser). And it needs to be descriptive enough not to confuse users.\n\"HERE\" is especially bad because my brain interprets it as \"HERE there\nis an error\", which makes me say \"Right. What error?\".\n\nSo \"HEREDOC\" would work, but there is no reason (aside from length) not\nto err on the side of being descriptive.\n\n t/chainlint.pl | 15 ++++++++++++---\n t/chainlint/unclosed-here-doc-indent.expect | 4 ++++\n t/chainlint/unclosed-here-doc-indent.test | 4 ++++\n t/chainlint/unclosed-here-doc.expect | 7 +++++++\n t/chainlint/unclosed-here-doc.test | 7 +++++++\n 5 files changed, 34 insertions(+), 3 deletions(-)\n create mode 100644 t/chainlint/unclosed-here-doc-indent.expect\n create mode 100644 t/chainlint/unclosed-here-doc-indent.test\n create mode 100644 t/chainlint/unclosed-here-doc.expect\n create mode 100644 t/chainlint/unclosed-here-doc.test\n\ndiff --git a/t/chainlint.pl b/t/chainlint.pl\nindex e966412999a..556ee91a15b 100755\n--- a/t/chainlint.pl\n+++ b/t/chainlint.pl\n@@ -80,7 +80,8 @@ sub scan_heredoc_tag {\n \treturn \"<<$indented\" unless $token;\n \tmy $tag = $token->[0];\n \t$tag =~ s/['\"\\\\]//g;\n-\tpush(@{$self->{heretags}}, $indented ? \"\\t$tag\" : \"$tag\");\n+\t$$token[0] = $indented ? \"\\t$tag\" : \"$tag\";\n+\tpush(@{$self->{heretags}}, $token);\n \treturn \"<<$indented$tag\";\n }\n \n@@ -169,10 +170,18 @@ sub swallow_heredocs {\n \tmy $tags = $self->{heretags};\n \twhile (my $tag = shift @$tags) {\n \t\tmy $start = pos($$b);\n-\t\tmy $indent = $tag =~ s/^\\t// ? '\\\\s*' : '';\n-\t\t$$b =~ /(?:\\G|\\n)$indent\\Q$tag\\E(?:\\n|\\z)/gc;\n+\t\tmy $indent = $$tag[0] =~ s/^\\t// ? '\\\\s*' : '';\n+\t\t$$b =~ /(?:\\G|\\n)$indent\\Q$$tag[0]\\E(?:\\n|\\z)/gc;\n+\t\tif (pos($$b) > $start) {\n+\t\t\tmy $body = substr($$b, $start, pos($$b) - $start);\n+\t\t\t$self->{lineno} += () = $body =~ /\\n/sg;\n+\t\t\tnext;\n+\t\t}\n+\t\tpush(@{$self->{parser}->{problems}}, ['UNCLOSED-HEREDOC', $tag]);\n+\t\t$$b =~ /(?:\\G|\\n).*\\z/gc; # consume rest of input\n \t\tmy $body = substr($$b, $start, pos($$b) - $start);\n \t\t$self->{lineno} += () = $body =~ /\\n/sg;\n+\t\tlast;\n \t}\n }\n \ndiff --git a/t/chainlint/unclosed-here-doc-indent.expect b/t/chainlint/unclosed-here-doc-indent.expect\nnew file mode 100644\nindex 00000000000..7c30a1a024e\n--- /dev/null\n+++ b/t/chainlint/unclosed-here-doc-indent.expect\n@@ -0,0 +1,4 @@\n+command_which_is_run &&\n+cat >expect <<-\\EOF ?!UNCLOSED-HEREDOC?! &&\n+we forget to end the here-doc\n+command_which_is_gobbled\ndiff --git a/t/chainlint/unclosed-here-doc-indent.test b/t/chainlint/unclosed-here-doc-indent.test\nnew file mode 100644\nindex 00000000000..5c841a9dfd4\n--- /dev/null\n+++ b/t/chainlint/unclosed-here-doc-indent.test\n@@ -0,0 +1,4 @@\n+command_which_is_run &&\n+cat >expect <<-\\EOF &&\n+we forget to end the here-doc\n+command_which_is_gobbled\ndiff --git a/t/chainlint/unclosed-here-doc.expect b/t/chainlint/unclosed-here-doc.expect\nnew file mode 100644\nindex 00000000000..d65e50f78d4\n--- /dev/null\n+++ b/t/chainlint/unclosed-here-doc.expect\n@@ -0,0 +1,7 @@\n+command_which_is_run &&\n+cat >expect <<\\EOF ?!UNCLOSED-HEREDOC?! &&\n+\twe try to end the here-doc below,\n+\tbut the indentation throws us off\n+\tsince the operator is not \"<<-\".\n+\tEOF\n+command_which_is_gobbled\ndiff --git a/t/chainlint/unclosed-here-doc.test b/t/chainlint/unclosed-here-doc.test\nnew file mode 100644\nindex 00000000000..69d3786c348\n--- /dev/null\n+++ b/t/chainlint/unclosed-here-doc.test\n@@ -0,0 +1,7 @@\n+command_which_is_run &&\n+cat >expect <<\\EOF &&\n+\twe try to end the here-doc below,\n+\tbut the indentation throws us off\n+\tsince the operator is not \"<<-\".\n+\tEOF\n+command_which_is_gobbled\n-- \n2.40.0.692.g7c4c956fc5c\n\n"},{"id":"474485","messageId":"20230330193047.GD27989@coredump.intra.peff.net","threadId":"59491","inReplyTo":"20230330192712.GA27719@coredump.intra.peff.net","subject":"[PATCH v2 4/5] tests: drop here-doc check from internal chain-linter","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-03-30T19:30:47Z","receivedAt":"2023-03-30T19:30:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Commit 99a64e4b73c (tests: lint for run-away here-doc, 2017-03-22)\ntweaked the chain-lint test to catch unclosed here-docs. It works by\nadding an extra \"echo\" command after the test snippet, and checking that\nit is run (if it gets swallowed by a here-doc, naturally it is not run).\n\nThe downside here is that we introduced an extra $() substitution, which\nhappens in a subshell. This has a measurable performance impact when\nrun for many tests.\n\nThe tradeoff in safety was undoubtedly worth it when 99a64e4b73c was\nwritten. But since the external chainlint.pl learned to find these\nrecently, we can just rely on it. By switching back to a simpler\nchain-lint, hyperfine reports a measurable speedup on t3070 (which has\n1800 tests):\n\n 'HEAD' ran\n 1.12 ± 0.01 times faster than 'HEAD~1'\n\nSigned-off-by: Jeff King \n---\nNow with 90% less hand-waving.\n\n t/test-lib.sh | 5 +++--\n 1 file changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex cfcbd899c5a..0048ec7b6f6 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -1101,9 +1101,10 @@ test_run_ () {\n \t\ttrace=\n \t\t# 117 is magic because it is unlikely to match the exit\n \t\t# code of other programs\n-\t\tif test \"OK-117\" != \"$(test_eval_ \"fail_117 && $1${LF}${LF}echo OK-\\$?\" 3>&1)\"\n+\t\ttest_eval_ \"fail_117 && $1\"\n+\t\tif test $? != 117\n \t\tthen\n-\t\t\tBUG \"broken &&-chain or run-away HERE-DOC: $1\"\n+\t\t\tBUG \"broken &&-chain: $1\"\n \t\tfi\n \t\ttrace=$trace_tmp\n \tfi\n-- \n2.40.0.692.g7c4c956fc5c\n\n"},{"id":"474486","messageId":"20230330193056.GE27989@coredump.intra.peff.net","threadId":"59491","inReplyTo":"20230330192712.GA27719@coredump.intra.peff.net","subject":"[PATCH v2 5/5] tests: skip test_eval_ in internal chain-lint","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-03-30T19:30:56Z","receivedAt":"2023-03-30T19:31:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"To check for broken &&-chains, we run \"fail_117 && $1\" as a test\nsnippet, and check the exit code. We use test_eval_ to do so, because\nthat's the way we run the actual test.\n\nBut we don't need any of its niceties, like \"set -x\" tracing. In fact,\nthey hinder us, because we have to explicitly disable them. So let's\nskip that and use \"eval\" more directly, which is simpler. I had hoped it\nwould also be faster, but it doesn't seem to produce a measurable\nimprovement (probably because it's just running internal shell commands,\nwith no subshells or forks).\n\nNote that there is one gotcha: even though we don't intend to run any of\nthe commands if the &&-chain is intact, an error like this:\n\n test_expect_success 'broken' '\n\t# this next line breaks the &&-chain\n\ttrue\n\t# and then this one is executed even by the linter\n\treturn 1\n '\n\nmeans we'll \"return 1\" from the eval, and thus from test_run_(). We\nactually do notice this in test_expect_success, but only by saying \"hey,\nthis test didn't say it was OK, so it must have failed\", which is not\nright (it should say \"broken &&-chain\").\n\nWe can handle this by calling test_eval_inner_() instead, which is our\ntrick for wrapping \"return\" in a test snippet. But to do that, we have\nto push the trace code out of that inner function and into test_eval_().\nThis is arguably where it belonged in the first place, but it never\nmattered because the \"inner_\" function had only one caller.\n\nSigned-off-by: Jeff King \n---\nSame as before.\n\n t/test-lib.sh | 17 ++++++-----------\n 1 file changed, 6 insertions(+), 11 deletions(-)\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 0048ec7b6f6..293caf0f20e 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -1041,10 +1041,7 @@ want_trace () {\n # (and we want to make sure we run any cleanup like\n # \"set +x\").\n test_eval_inner_ () {\n-\t# Do not add anything extra (including LF) after '$*'\n-\teval \"\n-\t\twant_trace && trace_level_=$(($trace_level_+1)) && set -x\n-\t\t$*\"\n+\teval \"$*\"\n }\n \n test_eval_ () {\n@@ -1069,7 +1066,10 @@ test_eval_ () {\n \t# be _inside_ the block to avoid polluting the \"set -x\" output\n \t#\n \n-\ttest_eval_inner_ \"$@\" &3 2>&4\n+\t# Do not add anything extra (including LF) after '$*'\n+\ttest_eval_inner_ &3 2>&4 \"\n+\t\twant_trace && trace_level_=$(($trace_level_+1)) && set -x\n+\t\t$*\"\n \t{\n \t\ttest_eval_ret_=$?\n \t\tif want_trace\n@@ -1095,18 +1095,13 @@ test_run_ () {\n \texpecting_failure=$2\n \n \tif test \"${GIT_TEST_CHAIN_LINT:-1}\" != 0; then\n-\t\t# turn off tracing for this test-eval, as it simply creates\n-\t\t# confusing noise in the \"-x\" output\n-\t\ttrace_tmp=$trace\n-\t\ttrace=\n \t\t# 117 is magic because it is unlikely to match the exit\n \t\t# code of other programs\n-\t\ttest_eval_ \"fail_117 && $1\"\n+\t\ttest_eval_inner_ \"fail_117 && $1\" &3 2>&4\n \t\tif test $? != 117\n \t\tthen\n \t\t\tBUG \"broken &&-chain: $1\"\n \t\tfi\n-\t\ttrace=$trace_tmp\n \tfi\n \n \tsetup_malloc_check\n-- \n2.40.0.692.g7c4c956fc5c\n"},{"id":"474487","messageId":"xmqqjzyyhtel.fsf@gitster.g","threadId":"59491","inReplyTo":"20230330192712.GA27719@coredump.intra.peff.net","subject":"Re: [PATCH v2 0/5] some chainlint fixes and performance improvements","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-30T20:32:34Z","receivedAt":"2023-03-30T20:32:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> As before, I think the first patch is the most important, and the rest\n> are optimizations. But with Eric's patch to chainlint.pl in the middle,\n> I think the argument for patch 4 (previously patch 3) is much stronger.\n>\n> Patch 5 remains mostly a cleanup, with no performance improvement. IMHO\n> the result is easier to follow, but I'm open to arguments to the\n> contrary.\n>\n> [1/5]: tests: run internal chain-linter under \"make test\"\n> [2/5]: tests: replace chainlint subshell with a function\n> [3/5]: tests: diagnose unclosed here-doc in chainlint.pl\n> [4/5]: tests: drop here-doc check from internal chain-linter\n> [5/5]: tests: skip test_eval_ in internal chain-lint\n\nThe new step [3/5] makes it easier to justify [4/5], indeed. Two\nprimary changes at the beginning are good as before. The last one\ndoes not make anything particularly easier to read, replacing one\ncryptic eval stuff with another, but it does not make it any worse,\nand the most importantly, it is clear to see that it does not change\nthe behaviour.\n\nWill queue. Thanks. Let's merge it down to 'next'.\n\n"},{"id":"474489","messageId":"CAPig+cRr=fvGKms5p3saWXm+38rBdn_jMK6p+mQCrS54krXagg@mail.gmail.com","threadId":"59491","inReplyTo":"20230330193031.GC27989@coredump.intra.peff.net","subject":"Re: [PATCH v2 3/5] tests: diagnose unclosed here-doc in chainlint.pl","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2023-03-30T21:26:53Z","receivedAt":"2023-03-30T21:27:09Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Mar 30, 2023 at 3:30 PM Jeff King wrote:\n> From: Eric Sunshine \n>\n> An unclosed here-doc in a test is a problem, because it silently gobbles\n> up any remaining commands. Since 99a64e4b73c (tests: lint for run-away\n> here-doc, 2017-03-22) we detect this by piggy-backing on the internal\n> chainlint checker in test-lib.sh.\n>\n> However, it would be nice to detect it in chainlint.pl, for a few\n> reasons:\n>\n> - the output from chainlint.pl is much nicer; it can show the exact\n> spot of the error, rather than a vague \"somewhere in this test you\n> broke the &&-chain or had a bad here-doc\" message.\n>\n> - the implementation in test-lib.sh runs for each test snippet. And\n> since it requires a subshell, the extra cost is small but not zero.\n> If chainlint.pl can reliably find the problem, we can optimize the\n> test-lib.sh code.\n>\n> The chainlint.pl code never intended to find here-doc problems. But\n> since it has to parse them anyway (to avoid reporting problems inside\n> here-docs), most of what we need is already there. We can detect the\n> problem when we fail to find the missing end-tag in swallow_heredocs().\n> The extra change in scan_heredoc_tag() stores the location of the start\n> of the here-doc, which lets us mark it as the source of the error in the\n> output (see the new tests for examples).\n>\n> [jk: added commit message and tests]\n\nThanks for packaging this up as a proper patch.\n\n> Signed-off-by: Eric Sunshine \n> Signed-off-by: Jeff King \n> ---\n> New in this iteration (thanks again, Eric!).\n>\n> I changed the error-tag from \"HERE\" to \"UNCLOSED-HEREDOC\". I don't think\n> there any syntactic limitations to worry about (it's just shown to the\n> user).\n\nThe error-tag is plucked by the script at a couple places using a\nregex such as /\\?![^?]+\\?!/ which won't be tripped up by the longer\nand hyphenated name, so no problem there.\n\n> And it needs to be descriptive enough not to confuse users.\n> \"HERE\" is especially bad because my brain interprets it as \"HERE there\n> is an error\", which makes me say \"Right. What error?\".\n>\n> So \"HEREDOC\" would work, but there is no reason (aside from length) not\n> to err on the side of being descriptive.\n\nI had the same thought about HERE being potentially misleading, thus\nwavered between HERE and HEREDOC, but ultimately chose the shorter for\nlength-similarity with the existing AMP and LOOP. That reminds me\nthat, for some time now, I've been thinking that t/README ought to\nhave a section explaining the meaning of AMP and LOOP (and HERE, if we\nwent with that) since newcomers may not intuitively understand what\nsuch terse complaints mean.\n\n> diff --git a/t/chainlint.pl b/t/chainlint.pl\n> @@ -80,7 +80,8 @@ sub scan_heredoc_tag {\n> - push(@{$self->{heretags}}, $indented ? \"\\t$tag\" : \"$tag\");\n> + $$token[0] = $indented ? \"\\t$tag\" : \"$tag\";\n> + push(@{$self->{heretags}}, $token);\n> return \"<<$indented$tag\";\n> @@ -169,10 +170,18 @@ sub swallow_heredocs {\n> - my $indent = $tag =~ s/^\\t// ? '\\\\s*' : '';\n> - $$b =~ /(?:\\G|\\n)$indent\\Q$tag\\E(?:\\n|\\z)/gc;\n> + my $indent = $$tag[0] =~ s/^\\t// ? '\\\\s*' : '';\n> + $$b =~ /(?:\\G|\\n)$indent\\Q$$tag[0]\\E(?:\\n|\\z)/gc;\n> + if (pos($$b) > $start) {\n> + my $body = substr($$b, $start, pos($$b) - $start);\n> + $self->{lineno} += () = $body =~ /\\n/sg;\n> + next;\n> + }\n> + push(@{$self->{parser}->{problems}}, ['UNCLOSED-HEREDOC', $tag]);\n> + $$b =~ /(?:\\G|\\n).*\\z/gc; # consume rest of input\n> my $body = substr($$b, $start, pos($$b) - $start);\n> $self->{lineno} += () = $body =~ /\\n/sg;\n> + last;\n\nOddly enough, these changes look fine to me.\n"},{"id":"474491","messageId":"20230330220823.GA130426@coredump.intra.peff.net","threadId":"59491","inReplyTo":"20230328210814.GA1754178@coredump.intra.peff.net","subject":"Re: [PATCH 0/4] some chainlint fixes and performance improvements","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-03-30T22:08:23Z","receivedAt":"2023-03-30T22:09:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 28, 2023 at 05:08:15PM -0400, Jeff King wrote:\n\n> BTW, I noticed something really funky when timing t3070 for this series.\n> \n> $ time ./t3070-wildmatch.sh\n> [a bunch of output]\n> real\t0m4.750s\n> user\t0m3.665s\n> sys\t0m0.955s\n> \n> $ time ./t3070-wildmatch.sh >/dev/null\n> real\t0m18.664s\n> user\t0m9.185s\n> sys\t0m9.495s\n> \n> Er, what? It gets way slower when redirected to /dev/null. I can't\n> figure out why.\n\nIn case anyone is curious (and I know you were all on the edge of your\nseats), I figured this out. The issue is that with the \"powersave\" CPU\ngovernor in place, we never ratchet up the CPU frequency. Perhaps\nbecause no process is pegging the CPU, but we just have tons of small\nprocesses that quickly exit (which seems like a blind spot in the\ngovernor, but at least makes some sense).\n\nWhen the output is going to the terminal, then the terminal is consuming\nCPU, and the frequency scales up. So it's faster when we show the\noutput, even though we're doing more work, because the CPU clock is\nfaster. Switching to the \"performance\" governor makes the problem go\naway.\n\nI cared for this series, of course, because I wanted to run t3070 under\nhyperfine, which behaves like the /dev/null case (unless you pass\n--show-output, which mangles the screen, and is why the hyperfine output\nI showed earlier was so terse). So with the performance governor in\nplace, here's the hyperfine output for the whole series (this is on the\n5-patch v2):\n\n $ hyperfine -P parent 0 5 -s 'git checkout jk/chainlint-fixes~{parent}' \\\n -n 't3070 on jk/chainlint-fixes~{parent}' ./t3070-wildmatch.sh\n\n Benchmark 1: t3070 on jk/chainlint-fixes~0\n Time (mean ± σ): 3.677 s ± 0.047 s [User: 2.893 s, System: 0.677 s]\n Range (min … max): 3.606 s … 3.725 s 10 runs\n \n Benchmark 2: t3070 on jk/chainlint-fixes~1\n Time (mean ± σ): 3.720 s ± 0.013 s [User: 2.941 s, System: 0.676 s]\n Range (min … max): 3.698 s … 3.738 s 10 runs\n \n Benchmark 3: t3070 on jk/chainlint-fixes~2\n Time (mean ± σ): 4.224 s ± 0.019 s [User: 3.291 s, System: 0.850 s]\n Range (min … max): 4.191 s … 4.254 s 10 runs\n \n Benchmark 4: t3070 on jk/chainlint-fixes~3\n Time (mean ± σ): 4.227 s ± 0.018 s [User: 3.293 s, System: 0.856 s]\n Range (min … max): 4.198 s … 4.252 s 10 runs\n \n Benchmark 5: t3070 on jk/chainlint-fixes~4\n Time (mean ± σ): 4.604 s ± 0.014 s [User: 3.599 s, System: 0.887 s]\n Range (min … max): 4.583 s … 4.629 s 10 runs\n \n Benchmark 6: t3070 on jk/chainlint-fixes~5\n Time (mean ± σ): 4.603 s ± 0.010 s [User: 3.578 s, System: 0.904 s]\n Range (min … max): 4.583 s … 4.617 s 10 runs\n \n Summary\n 't3070 on jk/chainlint-fixes~0' ran\n 1.01 ± 0.01 times faster than 't3070 on jk/chainlint-fixes~1'\n 1.15 ± 0.02 times faster than 't3070 on jk/chainlint-fixes~2'\n 1.15 ± 0.02 times faster than 't3070 on jk/chainlint-fixes~3'\n 1.25 ± 0.02 times faster than 't3070 on jk/chainlint-fixes~5'\n 1.25 ± 0.02 times faster than 't3070 on jk/chainlint-fixes~4'\n\nWhich is what we'd expect. We got about 1.25x faster, in two jumps at ~3\nand ~1, which were the patches removing subshells (marking commits by\ntheir parent number is rather confusing; I think it might be worth\nmaking a small hyperfine wrapper that feeds the commit summary to \"-n\").\n\nSo no effect on the series (good), but I didn't want to leave the\nmystery unsolved on the list. :)\n\n-Peff\n"},{"id":"474492","messageId":"20230330220915.GA1914330@coredump.intra.peff.net","threadId":"59491","inReplyTo":"xmqqjzyyhtel.fsf@gitster.g","subject":"Re: [PATCH v2 0/5] some chainlint fixes and performance improvements","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-03-30T22:09:15Z","receivedAt":"2023-03-30T22:09:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 30, 2023 at 01:32:34PM -0700, Junio C Hamano wrote:\n\n> > [5/5]: tests: skip test_eval_ in internal chain-lint\n> \n> The new step [3/5] makes it easier to justify [4/5], indeed. Two\n> primary changes at the beginning are good as before. The last one\n> does not make anything particularly easier to read, replacing one\n> cryptic eval stuff with another, but it does not make it any worse,\n> and the most importantly, it is clear to see that it does not change\n> the behaviour.\n\nYeah, the eval garbage is horrid no matter where it is. The improvement\nis that we no longer have to manually save-and-restore the \"trace\"\nvariable.\n\n> Will queue. Thanks. Let's merge it down to 'next'.\n\nGreat, thanks. :)\n\n-Peff\n"},{"id":"474493","messageId":"xmqqcz4pj362.fsf@gitster.g","threadId":"59491","inReplyTo":"20230330220823.GA130426@coredump.intra.peff.net","subject":"Re: [PATCH 0/4] some chainlint fixes and performance improvements","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-30T22:16:21Z","receivedAt":"2023-03-30T22:16:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> When the output is going to the terminal, then the terminal is consuming\n> CPU, and the frequency scales up. So it's faster when we show the\n> output, even though we're doing more work, because the CPU clock is\n> faster.\n\nThat's disgustingly sick and crazy ;-). But it does explain the symptom\nand it sounds like that \"powersave\" is a poor match for measurement.\n\n> Switching to the \"performance\" governor makes the problem go\n> away.\n> ...\n> So no effect on the series (good), but I didn't want to leave the\n> mystery unsolved on the list. :)\n\nThanks.\n"}]}