git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Eric Sunshine <sunshine@sunshineco.com>
To: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Cc: Git List <git@vger.kernel.org>,
	Junio C Hamano <gitster@pobox.com>, Jeff King <peff@peff.net>,
	Elijah Newren <newren@gmail.com>,
	Fabian Stelzer <fs@gigacodes.de>
Subject: Re: [RFC PATCH] t/Makefile: use dependency graph for "check-chainlint"
Date: Tue, 14 Dec 2021 02:44:25 -0500	[thread overview]
Message-ID: <CAPig+cSFtpt6ExbVDbcx3tZodrKFuM-r2GMW4TQ2tJmLvHBFtQ@mail.gmail.com> (raw)
In-Reply-To: <RFC-patch-1.1-bb3f1577829-20211213T095456Z-avarab@gmail.com>

On Mon, Dec 13, 2021 at 5:09 AM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> On Mon, Dec 13 2021, Eric Sunshine wrote:
> > Rather than running `chainlint` and `diff` once per self-test -- which
> > may become expensive as more tests are added -- instead run `chainlint`
> > a single time over all tests bodies collectively and compare the result
> > to the collective "expected" output.
>
> I think that "optimizing" things like this is an anti-pattern. I.e. we
> have N chainlint test files, and N potential outputs from that (ok or
> not, and with/without error). If one of the chainlint tests changes
> we'd like to re-run it, if not we can re-use an earlier run.

As mentioned in a reply elsewhere, the commit message sells this as an
optimization, but that's not the real reason for the change, which is
that the rewritten `check-chainlint` target for the upcoming new
chainlint really wants to have a composite file of the "test" input
and a composite of the "expect" output. I didn't know how to sell that
change in this preparatory patch series, so I weakly framed it as an
optimization. The reason for making this a preparatory step is that it
makes for a less noisy patch later on when the new chainlint is
actually plugged into the `check-chainlint` target. At least, it was
less noisy originally... in the final implementation, I think it ends
up being noisy anyhow. So, maybe it makes sense to drop this patch
altogether(?).

> This is something make's dependency logic is perfectly suited for, and
> will be faster than any optimization of turning a for-loop into a
> "sed" command we run every time, since we'll only need to "stat" a few
> things to see that there's nothing to do.
>
> +BUILT_CHAINLINTTESTS = $(patsubst %,.build/%.actual,$(CHAINLINTTESTS))
> +
> +.build/chainlint:
> +       mkdir -p $@
> +
> +$(BUILT_CHAINLINTTESTS): | .build/chainlint
> +$(BUILT_CHAINLINTTESTS): .build/%.actual: %
> +       $(CHAINLINT) <$< | \
> +       sed -e '/^# LINT: /d' >$@ && \
> +       diff -u $(basename $<).expect $@
> +
> +check-chainlint: $(BUILT_CHAINLINTTESTS)

This sort of optimization makes sense (I think) even with the new
chainlint preferring to see composite "test" and "expect" files rather
than the individual files. The individual files would be prerequisites
of the composite files, thus the composites would only be regenerated
if the individual files change. And the composite files would be
prerequisites of the `check-chainlint` target, so it would only run if
the composite files change (or if chainlint itself changes).

In fact, with the new chainlint checking all tests in all scripts at
once, this technique should apply nicely to it, as well, since the
names of test scripts (t????-*.sh) are fed to it as command-line
arguments. Thus, the t????-*.sh files could be prerequisites of the
chainlint rule which would use $? to check only test scripts which
have changed since the previous run.

Having said all that, I don't think think the changes in this series
or the upcoming new chainlint series make the situation any worse
(Makefile-wise) than its current state, and the sort of optimizations
discussed here can easily be made after those series land. (And, as my
Git time is rather limited these days, I'd really like to focus on
getting the core components landed.)

  reply	other threads:[~2021-12-14  7:44 UTC|newest]

Thread overview: 34+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-12-13  6:30 [PATCH 00/15] generalize chainlint self-tests Eric Sunshine
2021-12-13  6:30 ` [PATCH 01/15] t/chainlint/*.test: don't use invalid shell syntax Eric Sunshine
2021-12-13  6:30 ` [PATCH 02/15] t/chainlint/*.test: fix invalid test cases due to mixing quote types Eric Sunshine
2021-12-13  6:30 ` [PATCH 03/15] t/chainlint/*.test: generalize self-test commentary Eric Sunshine
2021-12-13  6:30 ` [PATCH 04/15] t/chainlint/one-liner: avoid overly intimate chainlint.sed knowledge Eric Sunshine
2021-12-13  6:30 ` [PATCH 05/15] t/Makefile: optimize chainlint self-test Eric Sunshine
2021-12-13 10:09   ` [RFC PATCH] t/Makefile: use dependency graph for "check-chainlint" Ævar Arnfjörð Bjarmason
2021-12-14  7:44     ` Eric Sunshine [this message]
2021-12-14 12:34       ` Ævar Arnfjörð Bjarmason
2021-12-13 10:22   ` [PATCH 05/15] t/Makefile: optimize chainlint self-test Fabian Stelzer
2021-12-13 14:27     ` Eric Sunshine
2021-12-13 15:43       ` Fabian Stelzer
2021-12-13 16:02         ` Eric Sunshine
2021-12-13 16:11           ` Ævar Arnfjörð Bjarmason
2021-12-13 17:05             ` Eric Sunshine
2021-12-13 17:25               ` Eric Sunshine
2021-12-13 19:33                 ` Ævar Arnfjörð Bjarmason
2021-12-13 21:37                   ` Eric Sunshine
2021-12-13 16:14           ` Fabian Stelzer
2021-12-16 13:17         ` Ævar Arnfjörð Bjarmason
2021-12-16 15:47           ` Eric Sunshine
2021-12-16 19:26             ` Ævar Arnfjörð Bjarmason
2021-12-13  6:30 ` [PATCH 06/15] chainlint.sed: improve ?!AMP?! placement accuracy Eric Sunshine
2021-12-13  6:30 ` [PATCH 07/15] chainlint.sed: improve ?!SEMI?! " Eric Sunshine
2021-12-13  6:30 ` [PATCH 08/15] chainlint.sed: tolerate harmless ";" at end of last line in block Eric Sunshine
2021-12-13  6:30 ` [PATCH 09/15] chainlint.sed: drop unnecessary distinction between ?!AMP?! and ?!SEMI?! Eric Sunshine
2021-12-13  6:30 ` [PATCH 10/15] chainlint.sed: drop subshell-closing ">" annotation Eric Sunshine
2021-12-13  6:30 ` [PATCH 11/15] chainlint.sed: make here-doc "<<-" operator recognition more POSIX-like Eric Sunshine
2021-12-13  6:30 ` [PATCH 12/15] chainlint.sed: don't mistake `<< word` in string as here-doc operator Eric Sunshine
2021-12-13  6:30 ` [PATCH 13/15] chainlint.sed: stop throwing away here-doc tags Eric Sunshine
2021-12-13  6:30 ` [PATCH 14/15] chainlint.sed: swallow comments consistently Eric Sunshine
2021-12-13  6:30 ` [PATCH 15/15] chainlint.sed: stop splitting "(..." into separate lines "(" and "..." Eric Sunshine
2021-12-15  0:00 ` [PATCH 00/15] generalize chainlint self-tests Elijah Newren
2021-12-15  3:15   ` Eric Sunshine

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=CAPig+cSFtpt6ExbVDbcx3tZodrKFuM-r2GMW4TQ2tJmLvHBFtQ@mail.gmail.com \
    --to=sunshine@sunshineco.com \
    --cc=avarab@gmail.com \
    --cc=fs@gigacodes.de \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=newren@gmail.com \
    --cc=peff@peff.net \
    /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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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).