Re: [PATCH] t: avoid sed-based chain-linting in some expensive cases

From: Jeff King <peff@peff.net>
Date: May 14, 2021, 05:48 UTC
Message-ID: <YJ4PHbVoQ8+ubfBK@coredump.intra.peff.net>
In-Reply-To: <CAN0heSp3mXQeqeC_Zd==bBoJCCWe-NzJsomuUf6MTxy7+WZ1wA@mail.gmail.com>

On Thu, May 13, 2021 at 01:05:28PM +0200, Martin Ågren wrote:

Show 10 quoted lines

> > +# Disable expensive chain-lint tests; all of the tests in this script
> > +# are variants of a few trivial test-tool invocations, and there are a lot of
> > +# them.
> > +GIT_TEST_CHAIN_LINT_HARDER_DEFAULT=0
> 
> Devil's advocate: Who do we expect to turn GIT_TEST_CHAIN_LINT_HARDER
> on, and when?  If no one ever does it then we might as well drop the
> "default" thing and just go "we won't bother linting these particular
> tests, ever". But as long as "someone" does it "sometimes", it's not
> like it's a very complex mechanism to carry around.

The answer is probably: people who suspect something is broken. We could perhaps also turn it on for CI to be more complete there (and where 30 seconds of CPU time is relatively much smaller). It was also handy to have while timing the impact, of course.

I'm not opposed to having it be less flexible, and in fact I wrote it that way originally. But it doesn't actually make the code much simpler. The assignments to _DEFAULT in the scripts become GIT_TEST_CHAIN_LINT_HARDER and the read side has one less level of defaulting:

-test "${GIT_TEST_CHAIN_LINT_HARDER:-${GIT_TEST_CHAIN_LINT_HARDER_DEFAULT:-1}}" != 0 && +test "${GIT_TEST_CHAIN_LINT_HARDER:-1}" != 0 &&

I guess it's conceptually a little simpler, though. I dunno. I sort of assumed it would just work and nobody would need to ever look at or configure it either way. :)

Show 9 quoted lines

> I seem to have 140 tests that haven't changed on disk since I did this
> particular clone in 2017. 235 haven't changed this calendar year. Maybe
> we could skip linting those tests that haven't been modified for several
> weeks on the basis that they can't reasonably have newly-introduced
> syntax mistakes. I guess it gets tricky where the t????-*.sh file
> doesn't change in a long time, but it sources tests from other places,
> such as a lib-foo.sh helper. We'd have to be a bit more clever there.
> That's all just thinking out loud, and definitely not something that
> should hold up your patch.

Yeah, I suspect that would work in general. But it seems like even more complexity (now you have a cache of "I linted this script at time X and it was good" that has to be written). It does increase the possible savings though (up to perhaps 100 or so seconds of parallel CPU in my case).

I think a bigger and better version of that is to actually see which code paths are run by which scripts, and not even bother running scripts that don't touch code which has changed. But that's a _lot_ more complicated, and writing such a tool is probably at least worth a thesis project. ;)

-Peff