{"thread":{"id":"55680","subject":"[PATCH] t: avoid sed-based chain-linting in some expensive cases","startedAt":"2021-05-13T06:25:56Z","lastAt":"2021-05-15T09:19:25Z","messageCount":7,"participants":["Jeff King","Junio C Hamano","Martin Ågren"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"424423","messageId":"YJzGcZpZ+E9R0gYd@coredump.intra.peff.net","threadId":"55680","inReplyTo":null,"subject":"[PATCH] t: avoid sed-based chain-linting in some expensive cases","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-05-13T06:25:53Z","receivedAt":"2021-05-13T06:25:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Commit 878f988350 (t/test-lib: teach --chain-lint to detect broken\n&&-chains in subshells, 2018-07-11) introduced additional chain-lint\ntests which add an extra \"sed\" pipeline to each test we run. This has a\nmeasurable impact on runtime. Here are timings with and without a new\nenvironment variable (added by this patch) that lets you disable just\nthe additional sed-based chain-lint tests:\n\n  Benchmark #1: GIT_TEST_CHAIN_LINT_HARDER=1 make test\n    Time (mean ± σ):     64.202 s ±  1.030 s    [User: 622.469 s, System: 301.402 s]\n    Range (min … max):   61.571 s … 65.662 s    10 runs\n\n  Benchmark #2: GIT_TEST_CHAIN_LINT_HARDER=0 make test\n    Time (mean ± σ):     57.591 s ±  0.333 s    [User: 529.368 s, System: 270.618 s]\n    Range (min … max):   57.143 s … 58.309 s    10 runs\n\n  Summary\n    'GIT_TEST_CHAIN_LINT_HARDER=0 make test' ran\n      1.11 ± 0.02 times faster than 'GIT_TEST_CHAIN_LINT_HARDER=1 make test'\n\nOf course those extra lint checks are doing something useful, so paying\na few extra seconds (at least on Linux) isn't so bad (though note the\nCPU time; we're bounded in our parallel run here by the slowest test, so\nit really is ~120s of CPU improvement).\n\nBut we can observe that there are some test scripts where they produce a\nmuch stronger effect, and provide less value. In t0027 and t3070 we run\na very large number of small tests, all driven by a series of\nfunctions/loops which are filling in the test bodies. There we get much\nless bang for our buck in terms of bug-finding versus CPU cost.\n\nThis patch introduces a mechanism for controlling when those extra\nlint checks are run, at two levels:\n\n  - a user can ask to disable or to force-enable the checks by setting\n    GIT_TEST_CHAIN_LINT_HARDER\n\n  - if the user hasn't specified a preference, individual scripts can\n    disable the checks by setting GIT_TEST_CHAIN_LINT_HARDER_DEFAULT;\n    scripts which don't set that get the current behavior of enabling\n    them.\n\nIn addition, this patch flips the default for t0027 and t3070's\nmass-generated sections to disable the extra checks. Here are the timing\nresults for t0027:\n\n  Benchmark #1: GIT_TEST_CHAIN_LINT_HARDER=1 ./t0027-auto-crlf.sh\n    Time (mean ± σ):     17.078 s ±  0.848 s    [User: 14.878 s, System: 7.075 s]\n    Range (min … max):   15.952 s … 18.421 s    10 runs\n\n  Benchmark #2: GIT_TEST_CHAIN_LINT_HARDER=0 ./t0027-auto-crlf.sh\n    Time (mean ± σ):      9.063 s ±  0.759 s    [User: 7.890 s, System: 3.362 s]\n    Range (min … max):    7.747 s … 10.619 s    10 runs\n\n  Benchmark #3: ./t0027-auto-crlf.sh\n    Time (mean ± σ):      9.186 s ±  0.881 s    [User: 7.957 s, System: 3.427 s]\n    Range (min … max):    7.796 s … 10.498 s    10 runs\n\n  Summary\n    'GIT_TEST_CHAIN_LINT_HARDER=0 ./t0027-auto-crlf.sh' ran\n      1.01 ± 0.13 times faster than './t0027-auto-crlf.sh'\n      1.88 ± 0.18 times faster than 'GIT_TEST_CHAIN_LINT_HARDER=1 ./t0027-auto-crlf.sh'\n\nWe can see that disabling the checks for the whole script buys us an\nalmost 2x speedup. But the new default behavior, disabling them only for\nthe mass-generated part, gets us most of that speedup (but still leaves\nthe checks on for further manual tests people might write).\n\n  As a side note, I'd caution about comparing runtimes and CPU seconds\n  between this timing and the earlier \"make test\" one. In \"make test\",\n  we're running a lot of scripts in parallel, so the CPU is throttling\n  down (and thus a CPU second saved here would count for more during a\n  parallel run; the same work takes more CPU seconds there).\n\nWe get similar results for t3070:\n\n  Benchmark #1: GIT_TEST_CHAIN_LINT_HARDER=1 ./t3070-wildmatch.sh\n    Time (mean ± σ):     20.054 s ±  3.967 s    [User: 16.003 s, System: 8.286 s]\n    Range (min … max):   11.891 s … 23.671 s    10 runs\n\n  Benchmark #2: GIT_TEST_CHAIN_LINT_HARDER=0 ./t3070-wildmatch.sh\n    Time (mean ± σ):     12.399 s ±  2.256 s    [User: 7.542 s, System: 5.342 s]\n    Range (min … max):    9.606 s … 15.727 s    10 runs\n\n  Benchmark #3: ./t3070-wildmatch.sh\n    Time (mean ± σ):     10.726 s ±  3.476 s    [User: 6.790 s, System: 4.365 s]\n    Range (min … max):    5.444 s … 15.376 s    10 runs\n\n  Summary\n    './t3070-wildmatch.sh' ran\n      1.16 ± 0.43 times faster than 'GIT_TEST_CHAIN_LINT_HARDER=0 ./t3070-wildmatch.sh'\n      1.87 ± 0.71 times faster than 'GIT_TEST_CHAIN_LINT_HARDER=1 ./t3070-wildmatch.sh'\n\nAgain, we get almost a 2x speedup disabling these. In this case, there\nare no tests not covered by the script's \"default to disable\" behavior,\nso the second two benchmarks should be the same (and while they do\ndiffer, you can see the variance is quite high but they're within one\nstandard deviation).\n\nSo it seems like for these two scripts, at least, disabling the extra\nchecks is a reasonable tradeoff. Sadly, the overall runtime of \"make\ntest\" on my system doesn't get much faster. But that's because we're\nmostly limited by the cost of the single biggest test. Here are the\ntop-5 tests by wall-clock time from a parallel run, before my patch:\n\n  57.9192368984222 t9001-send-email.sh\n  45.6329638957977 t0027-auto-crlf.sh\n  32.5278220176697 t3070-wildmatch.sh\n  22.2701289653778 t7610-mergetool.sh\n  20.8635759353638 t1701-racy-split-index.sh\n\nAnd after:\n\n  57.1476998329163 t9001-send-email.sh\n  33.776211977005 t0027-auto-crlf.sh\n  21.3116669654846 t7610-mergetool.sh\n  20.7748689651489 t1701-racy-split-index.sh\n  19.6957249641418 t7112-reset-submodule.sh\n\nWe dropped 12s from t0027, and t3070 dropped off our list entirely at\naround 16s. In both cases we're bound by t9001, but its slowness is\ndue to the actual tests, so we'll have to deal with it in a different\nway. But this reduces overall CPU, and means that dealing with t9001 (by\nimproving the speed of send-email or splitting it apart) will let us\nreduce our overall runtime even on multi-core machines.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nThis is one of the speedups I mentioned earlier in:\n\n  https://lore.kernel.org/git/YJxmHbHAn4x93xsr@coredump.intra.peff.net/\n\nbut it's really independent of Ævar's your work on send-email (of course\nonce he makes t9001 faster, then these two scripts become the most\nexpensive :) ).\n\nI suspect t3070 could be made faster still by consolidating some of its\n1890 test cases (e.g., testing multiple related variants in a single\ninvocation of git, reducing the number of processes needed). But it\nlooks non-trivial, and I think this extra chain-lint knob is a good tool\nto have in our belt anyway.\n\n t/README             | 5 +++++\n t/t0027-auto-crlf.sh | 7 ++++++-\n t/t3070-wildmatch.sh | 5 +++++\n t/test-lib.sh        | 7 +++++--\n 4 files changed, 21 insertions(+), 3 deletions(-)\n\ndiff --git a/t/README b/t/README\nindex 8eb9e46b1d..6f28819406 100644\n--- a/t/README\n+++ b/t/README\n@@ -196,6 +196,11 @@ 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+\tA few test scripts disable some of the more advanced\n+\tchain-linting detection in the name of efficiency. You can\n+\toverride this by setting the GIT_TEST_CHAIN_LINT_HARDER\n+\tenvironment variable to \"1\".\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/t0027-auto-crlf.sh b/t/t0027-auto-crlf.sh\nindex d24d5acfbc..4a5c5c602c 100755\n--- a/t/t0027-auto-crlf.sh\n+++ b/t/t0027-auto-crlf.sh\n@@ -386,7 +386,9 @@ test_expect_success 'setup main' '\n \ttest_tick\n '\n \n-\n+# Disable extra chain-linting for the next set of tests. There are many\n+# auto-generated ones that are not worth checking over and over.\n+GIT_TEST_CHAIN_LINT_HARDER_DEFAULT=0\n \n warn_LF_CRLF=\"LF will be replaced by CRLF\"\n warn_CRLF_LF=\"CRLF will be replaced by LF\"\n@@ -597,6 +599,9 @@ do\n \tcheckout_files     auto  \"$id\" \"\"     false   native   $NL   CRLF  CRLF_mix_LF  LF_mix_CR    LF_nul\n done\n \n+# The rest of the tests are unique; do the usual linting.\n+unset GIT_TEST_CHAIN_LINT_HARDER_DEFAULT\n+\n # Should be the last test case: remove some files from the worktree\n test_expect_success 'ls-files --eol -d -z' '\n \trm crlf_false_attr__CRLF.txt crlf_false_attr__CRLF_mix_LF.txt crlf_false_attr__LF.txt .gitattributes &&\ndiff --git a/t/t3070-wildmatch.sh b/t/t3070-wildmatch.sh\nindex 891d4d7cb9..56ea4bda13 100755\n--- a/t/t3070-wildmatch.sh\n+++ b/t/t3070-wildmatch.sh\n@@ -4,6 +4,11 @@ test_description='wildmatch tests'\n \n . ./test-lib.sh\n \n+# Disable expensive chain-lint tests; all of the tests in this script\n+# are variants of a few trivial test-tool invocations, and there are a lot of\n+# them.\n+GIT_TEST_CHAIN_LINT_HARDER_DEFAULT=0\n+\n should_create_test_file() {\n \tfile=$1\n \ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex adaa2db601..adaf03543e 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -947,8 +947,11 @@ 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 $(printf '%s\\n' \"$1\" | sed -f \"$GIT_BUILD_DIR/t/chainlint.sed\" | grep -q '?![A-Z][A-Z]*?!') ||\n-\t\t\ttest \"OK-117\" != \"$(test_eval_ \"(exit 117) && $1${LF}${LF}echo OK-\\$?\" 3>&1)\"\n+\t\tif test \"OK-117\" != \"$(test_eval_ \"(exit 117) && $1${LF}${LF}echo OK-\\$?\" 3>&1)\" ||\n+\t\t   {\n+\t\t\ttest \"${GIT_TEST_CHAIN_LINT_HARDER:-${GIT_TEST_CHAIN_LINT_HARDER_DEFAULT:-1}}\" != 0 &&\n+\t\t\t$(printf '%s\\n' \"$1\" | sed -f \"$GIT_BUILD_DIR/t/chainlint.sed\" | grep -q '?![A-Z][A-Z]*?!')\n+\t\t   }\n \t\tthen\n \t\t\tBUG \"broken &&-chain or run-away HERE-DOC: $1\"\n \t\tfi\n-- \n2.31.1.1030.g25c0337765\n"},{"id":"424428","messageId":"xmqqim3nb01b.fsf@gitster.g","threadId":"55680","inReplyTo":"YJzGcZpZ+E9R0gYd@coredump.intra.peff.net","subject":"Re: [PATCH] t: avoid sed-based chain-linting in some expensive cases","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-05-13T06:49:52Z","receivedAt":"2021-05-13T06:50:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Of course those extra lint checks are doing something useful, so paying\n> a few extra seconds (at least on Linux) isn't so bad (though note the\n> CPU time; we're bounded in our parallel run here by the slowest test, so\n> it really is ~120s of CPU improvement).\n\nNice.\n\n> diff --git a/t/test-lib.sh b/t/test-lib.sh\n> index adaa2db601..adaf03543e 100644\n> --- a/t/test-lib.sh\n> +++ b/t/test-lib.sh\n> @@ -947,8 +947,11 @@ 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 $(printf '%s\\n' \"$1\" | sed -f \"$GIT_BUILD_DIR/t/chainlint.sed\" | grep -q '?![A-Z][A-Z]*?!') ||\n> -\t\t\ttest \"OK-117\" != \"$(test_eval_ \"(exit 117) && $1${LF}${LF}echo OK-\\$?\" 3>&1)\"\n> +\t\tif test \"OK-117\" != \"$(test_eval_ \"(exit 117) && $1${LF}${LF}echo OK-\\$?\" 3>&1)\" ||\n> +\t\t   {\n> +\t\t\ttest \"${GIT_TEST_CHAIN_LINT_HARDER:-${GIT_TEST_CHAIN_LINT_HARDER_DEFAULT:-1}}\" != 0 &&\n> +\t\t\t$(printf '%s\\n' \"$1\" | sed -f \"$GIT_BUILD_DIR/t/chainlint.sed\" | grep -q '?![A-Z][A-Z]*?!')\n> +\t\t   }\n\nWe have been doing the more expensive one first, but we now\noptionally skip it while retaining the one that uses the shell.\nOK.\n\nNicely done.\n\n>  \t\tthen\n>  \t\t\tBUG \"broken &&-chain or run-away HERE-DOC: $1\"\n>  \t\tfi\n"},{"id":"424433","messageId":"YJzT6dUBm82L1qZa@coredump.intra.peff.net","threadId":"55680","inReplyTo":"xmqqim3nb01b.fsf@gitster.g","subject":"Re: [PATCH] t: avoid sed-based chain-linting in some expensive cases","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-05-13T07:23:21Z","receivedAt":"2021-05-13T07:23:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 13, 2021 at 03:49:52PM +0900, Junio C Hamano wrote:\n\n> > diff --git a/t/test-lib.sh b/t/test-lib.sh\n> > index adaa2db601..adaf03543e 100644\n> > --- a/t/test-lib.sh\n> > +++ b/t/test-lib.sh\n> > @@ -947,8 +947,11 @@ 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 $(printf '%s\\n' \"$1\" | sed -f \"$GIT_BUILD_DIR/t/chainlint.sed\" | grep -q '?![A-Z][A-Z]*?!') ||\n> > -\t\t\ttest \"OK-117\" != \"$(test_eval_ \"(exit 117) && $1${LF}${LF}echo OK-\\$?\" 3>&1)\"\n> > +\t\tif test \"OK-117\" != \"$(test_eval_ \"(exit 117) && $1${LF}${LF}echo OK-\\$?\" 3>&1)\" ||\n> > +\t\t   {\n> > +\t\t\ttest \"${GIT_TEST_CHAIN_LINT_HARDER:-${GIT_TEST_CHAIN_LINT_HARDER_DEFAULT:-1}}\" != 0 &&\n> > +\t\t\t$(printf '%s\\n' \"$1\" | sed -f \"$GIT_BUILD_DIR/t/chainlint.sed\" | grep -q '?![A-Z][A-Z]*?!')\n> > +\t\t   }\n> \n> We have been doing the more expensive one first, but we now\n> optionally skip it while retaining the one that uses the shell.\n> OK.\n\nOh yeah, I meant to call that out. I flipped them mostly because it made\nthe conditional easier to read (though as you can see, it's already\nquite a mouthful).\n\nIn practice the short-circuit doesn't help us much, though. Unless a\ntest is buggy, we end up having to run both tests either way, no matter\nthe order.\n\n-Peff\n"},{"id":"424460","messageId":"CAN0heSp3mXQeqeC_Zd==bBoJCCWe-NzJsomuUf6MTxy7+WZ1wA@mail.gmail.com","threadId":"55680","inReplyTo":"YJzGcZpZ+E9R0gYd@coredump.intra.peff.net","subject":"Re: [PATCH] t: avoid sed-based chain-linting in some expensive cases","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2021-05-13T11:05:28Z","receivedAt":"2021-05-13T11:06:39Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On Thu, 13 May 2021 at 08:29, Jeff King <peff@peff.net> wrote:\n> This patch introduces a mechanism for controlling when those extra\n> lint checks are run, at two levels:\n>\n>   - a user can ask to disable or to force-enable the checks by setting\n>     GIT_TEST_CHAIN_LINT_HARDER\n>\n>   - if the user hasn't specified a preference, individual scripts can\n>     disable the checks by setting GIT_TEST_CHAIN_LINT_HARDER_DEFAULT;\n>     scripts which don't set that get the current behavior of enabling\n>     them.\n\n> +# Disable expensive chain-lint tests; all of the tests in this script\n> +# are variants of a few trivial test-tool invocations, and there are a lot of\n> +# them.\n> +GIT_TEST_CHAIN_LINT_HARDER_DEFAULT=0\n\nDevil's advocate: Who do we expect to turn GIT_TEST_CHAIN_LINT_HARDER\non, and when?  If no one ever does it then we might as well drop the\n\"default\" thing and just go \"we won't bother linting these particular\ntests, ever\". But as long as \"someone\" does it \"sometimes\", it's not\nlike it's a very complex mechanism to carry around.\n\nI seem to have 140 tests that haven't changed on disk since I did this\nparticular clone in 2017. 235 haven't changed this calendar year. Maybe\nwe could skip linting those tests that haven't been modified for several\nweeks on the basis that they can't reasonably have newly-introduced\nsyntax mistakes. I guess it gets tricky where the t????-*.sh file\ndoesn't change in a long time, but it sources tests from other places,\nsuch as a lib-foo.sh helper. We'd have to be a bit more clever there.\nThat's all just thinking out loud, and definitely not something that\nshould hold up your patch.\n\n\nMartin\n"},{"id":"424544","messageId":"YJ4PHbVoQ8+ubfBK@coredump.intra.peff.net","threadId":"55680","inReplyTo":"CAN0heSp3mXQeqeC_Zd==bBoJCCWe-NzJsomuUf6MTxy7+WZ1wA@mail.gmail.com","subject":"Re: [PATCH] t: avoid sed-based chain-linting in some expensive cases","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-05-14T05:48:13Z","receivedAt":"2021-05-14T05:48:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 13, 2021 at 01:05:28PM +0200, Martin Ågren wrote:\n\n> > +# Disable expensive chain-lint tests; all of the tests in this script\n> > +# are variants of a few trivial test-tool invocations, and there are a lot of\n> > +# them.\n> > +GIT_TEST_CHAIN_LINT_HARDER_DEFAULT=0\n> \n> Devil's advocate: Who do we expect to turn GIT_TEST_CHAIN_LINT_HARDER\n> on, and when?  If no one ever does it then we might as well drop the\n> \"default\" thing and just go \"we won't bother linting these particular\n> tests, ever\". But as long as \"someone\" does it \"sometimes\", it's not\n> like it's a very complex mechanism to carry around.\n\nThe answer is probably: people who suspect something is broken. We could\nperhaps also turn it on for CI to be more complete there (and where 30\nseconds of CPU time is relatively much smaller). It was also handy to\nhave while timing the impact, of course.\n\nI'm not opposed to having it be less flexible, and in fact I wrote it\nthat way originally. But it doesn't actually make the code much simpler.\nThe assignments to _DEFAULT in the scripts become GIT_TEST_CHAIN_LINT_HARDER\nand the read side has one less level of defaulting:\n\n-test \"${GIT_TEST_CHAIN_LINT_HARDER:-${GIT_TEST_CHAIN_LINT_HARDER_DEFAULT:-1}}\" != 0 &&\n+test \"${GIT_TEST_CHAIN_LINT_HARDER:-1}\" != 0 &&\n\nI guess it's conceptually a little simpler, though. I dunno. I sort of\nassumed it would just work and nobody would need to ever look at or\nconfigure it either way. :)\n\n> I seem to have 140 tests that haven't changed on disk since I did this\n> particular clone in 2017. 235 haven't changed this calendar year. Maybe\n> we could skip linting those tests that haven't been modified for several\n> weeks on the basis that they can't reasonably have newly-introduced\n> syntax mistakes. I guess it gets tricky where the t????-*.sh file\n> doesn't change in a long time, but it sources tests from other places,\n> such as a lib-foo.sh helper. We'd have to be a bit more clever there.\n> That's all just thinking out loud, and definitely not something that\n> should hold up your patch.\n\nYeah, I suspect that would work in general. But it seems like even more\ncomplexity (now you have a cache of \"I linted this script at time X and\nit was good\" that has to be written). It does increase the possible\nsavings though (up to perhaps 100 or so seconds of parallel CPU in my\ncase).\n\nI think a bigger and better version of that is to actually see which\ncode paths are run by which scripts, and not even bother running scripts\nthat don't touch code which has changed. But that's a _lot_ more\ncomplicated, and writing such a tool is probably at least worth a thesis\nproject. ;)\n\n-Peff\n"},{"id":"424552","messageId":"CAN0heSrjPJG_OGyV2dt9bOfkd07He=kBUKQ-+w5JXNae+YH7qg@mail.gmail.com","threadId":"55680","inReplyTo":"YJ4PHbVoQ8+ubfBK@coredump.intra.peff.net","subject":"Re: [PATCH] t: avoid sed-based chain-linting in some expensive cases","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2021-05-14T08:52:35Z","receivedAt":"2021-05-14T08:52:50Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On Fri, 14 May 2021 at 07:48, Jeff King <peff@peff.net> wrote:\n>\n> On Thu, May 13, 2021 at 01:05:28PM +0200, Martin Ågren wrote:\n>\n> > Devil's advocate: Who do we expect to turn GIT_TEST_CHAIN_LINT_HARDER\n> > on, and when?  If no one ever does it then we might as well drop the\n> > \"default\" thing and just go \"we won't bother linting these particular\n> > tests, ever\". But as long as \"someone\" does it \"sometimes\", it's not\n> > like it's a very complex mechanism to carry around.\n>\n> The answer is probably: people who suspect something is broken. We could\n> perhaps also turn it on for CI to be more complete there (and where 30\n> seconds of CPU time is relatively much smaller). It was also handy to\n> have while timing the impact, of course.\n>\n> I'm not opposed to having it be less flexible, and in fact I wrote it\n\nAnd to be clear, I'm not opposed to having it more flexible. :)\n\n> that way originally. But it doesn't actually make the code much simpler.\n> The assignments to _DEFAULT in the scripts become GIT_TEST_CHAIN_LINT_HARDER\n> and the read side has one less level of defaulting:\n>\n> -test \"${GIT_TEST_CHAIN_LINT_HARDER:-${GIT_TEST_CHAIN_LINT_HARDER_DEFAULT:-1}}\" != 0 &&\n> +test \"${GIT_TEST_CHAIN_LINT_HARDER:-1}\" != 0 &&\n>\n> I guess it's conceptually a little simpler, though. I dunno. I sort of\n> assumed it would just work and nobody would need to ever look at or\n> configure it either way. :)\n\n:)\n\n> > I seem to have 140 tests that haven't changed on disk since I did this\n> > particular clone in 2017. 235 haven't changed this calendar year. Maybe\n> > we could skip linting those tests that haven't been modified for several\n> > weeks on the basis that they can't reasonably have newly-introduced\n> > syntax mistakes. I guess it gets tricky where the t????-*.sh file\n> > doesn't change in a long time, but it sources tests from other places,\n> > such as a lib-foo.sh helper. We'd have to be a bit more clever there.\n> > That's all just thinking out loud, and definitely not something that\n> > should hold up your patch.\n>\n> Yeah, I suspect that would work in general. But it seems like even more\n> complexity (now you have a cache of \"I linted this script at time X and\n> it was good\" that has to be written). It does increase the possible\n> savings though (up to perhaps 100 or so seconds of parallel CPU in my\n> case).\n\nYeah, I thought about the cache. I guess it would be a list of\nknown-good test script hashes / blob IDs. But what I actually meant was\nto check whether the mtime was way back in the past. It's not fool-proof\nthough. You could have a network-mounted disk where the date is way off,\nor you could hack up the test script, wait for several weeks and *then*\nrun it. ;)\n\n> I think a bigger and better version of that is to actually see which\n> code paths are run by which scripts, and not even bother running scripts\n> that don't touch code which has changed. But that's a _lot_ more\n> complicated, and writing such a tool is probably at least worth a thesis\n> project. ;)\n\nAh yes, it should be safe to punt on that for now.\n\nMartin\n"},{"id":"424633","messageId":"YJ+SGfD7kPEW6ysn@coredump.intra.peff.net","threadId":"55680","inReplyTo":"CAN0heSrjPJG_OGyV2dt9bOfkd07He=kBUKQ-+w5JXNae+YH7qg@mail.gmail.com","subject":"Re: [PATCH] t: avoid sed-based chain-linting in some expensive cases","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-05-15T09:19:21Z","receivedAt":"2021-05-15T09:19:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, May 14, 2021 at 10:52:35AM +0200, Martin Ågren wrote:\n\n> > Yeah, I suspect that would work in general. But it seems like even more\n> > complexity (now you have a cache of \"I linted this script at time X and\n> > it was good\" that has to be written). It does increase the possible\n> > savings though (up to perhaps 100 or so seconds of parallel CPU in my\n> > case).\n> \n> Yeah, I thought about the cache. I guess it would be a list of\n> known-good test script hashes / blob IDs. But what I actually meant was\n> to check whether the mtime was way back in the past. It's not fool-proof\n> though. You could have a network-mounted disk where the date is way off,\n> or you could hack up the test script, wait for several weeks and *then*\n> run it. ;)\n\nAh, I see. You'd set some date for \"this is so old you've probably run\nthe linting on it by now\". Yeah, that is less complex to implement. It\ndoes feel awfully magical, though.\n\n-Peff\n"}]}