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

From: Jeff King <peff@peff.net>
To: "Martin Ågren" <martin.agren@gmail.com>
Cc: "Git Mailing List" <git@vger.kernel.org>,
	"Eric Sunshine" <sunshine@sunshineco.com>,
	"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Subject: Re: [PATCH] t: avoid sed-based chain-linting in some expensive cases
Date: Fri, 14 May 2021 01:48:13 -0400	[thread overview]
Message-ID: <YJ4PHbVoQ8+ubfBK@coredump.intra.peff.net> (raw)
In-Reply-To: <CAN0heSp3mXQeqeC_Zd==bBoJCCWe-NzJsomuUf6MTxy7+WZ1wA@mail.gmail.com>

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

> > +# 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. :)

> 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