git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
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

  reply	other threads:[~2021-05-14  5:48 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-05-13  6:25 Jeff King
2021-05-13  6:49 ` Junio C Hamano
2021-05-13  7:23   ` Jeff King
2021-05-13 11:05 ` Martin Ågren
2021-05-14  5:48   ` Jeff King [this message]
2021-05-14  8:52     ` Martin Ågren
2021-05-15  9:19       ` Jeff King

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YJ4PHbVoQ8+ubfBK@coredump.intra.peff.net \
    --to=peff@peff.net \
    --cc=avarab@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=martin.agren@gmail.com \
    --cc=sunshine@sunshineco.com \
    --subject='Re: [PATCH] t: avoid sed-based chain-linting in some expensive cases' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).