Re: [PATCH 05/15] t/Makefile: optimize chainlint self-test

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Dec 13, 2021, 17:05 UTC
Message-ID: <CAPig+cR5Q2q=wdXuVBeP52=pAfvh6_4z__g-0JUGWq_7uor_Hg@mail.gmail.com>
In-Reply-To: <211213.86tufc8oop.gmgdl@evledraar.gmail.com>

On Mon, Dec 13, 2021 at 11:17 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

Show 16 quoted lines

> On Mon, Dec 13 2021, Eric Sunshine wrote:
> > On Mon, Dec 13, 2021 at 10:43 AM Fabian Stelzer <fs@gigacodes.de> wrote:
> >> I have no idea what is going on here: `<$< |` ?
> >
> > Ya, that line-noise is an unfortunate combination of shell and
> > Makefile gobbledygook. The `$<` is effectively the source file (the
> > file being passed into chainlint.sed), and the rest of it is just
> > normal shell. `<` is redirection (using the source file `$<` as
> > stdin), and `|` is the pipe operator (sending the output of
> > chainlint.sed to another `sed`), and capturing it all via shell `>`
> > redirection in `$@` which is the Makefile variable for the target
> > file.
>
> To add to that;
> https://www.gnu.org/software/make/manual/html_node/Rules.html#Rules and
> other relevant parts of the GNU make manual are very helpful here.

And the Makefile variables $< and $@, in particular, are documented here: https://www.gnu.org/software/make/manual/html_node/Automatic-Variables.html

Show 17 quoted lines

> I don't really care about the details of whether it's invoked once or N
> times, although I think the N times with proper dependencies tends to
> give you better error messages, but maybe you'll be changing it
> significantly enough that the current map between chainlint files and
> approximately what sort of thing they check won't be there anymore.
>
> In any case, I'd think that a rule that used $< now (i.e. 1=1 file->out
> prereq) would be better for the current state, and could just be changed
> to use one of $^ or $+ later.
>
> I.e. you can declare a "check.done" that depends on {1..10}.todo, and
> get a list of all of those {1..10}.todo files if one changes, or just
> the ones whose mtime is newer than a "check.done".
>
> The reason I looked at this to begin with is that it takes it ~100-150ms
> to run now, which adds up if you're e.g. using "make test T=<glob>" in
> "git rebase -i --exec".

Regarding this last point, one idea I strongly considered (and have not rejected) is to stop making `check-chainlin` a dependency of `test` and `prove`. Unlike most of the test suite, in which a change to any part of the Git source code could potentially cause any test to fail -- thus, it is important to run the full test suite for any source code change -- the `check-chainlint` target is completely isolated from everything else; it only checks whether `chainlint` itself functions correctly. The only time it really makes sense to run `check-chainlint` is when chainlint itself is changed in order to verify that it still functions as expected. Considering how infrequently (i.e. never) chainlint is modified, it seems somewhat silly for every `make test` or `make prove` invoked by anybody anywhere to repeatedly and forever validate chainlint[*]. Instead, it could be the responsibility of the person modifying chainlint to run the `check-chainlint` self-tests.

[*]: There is at least one exception. Various implementations of `sed` could behave differently, thus impacting the behavior of chainlint.sed. This is not just a theoretical concern. I did all the development of this series on macOS, where everything worked as intended. Shortly before sending the series to the list, I subjected it to other platforms via CI and found that it failed on Linux due to minor behavioral differences in `sed` on Linux (though, very oddly, it worked just fine on Windows). I might not have caught this problem if `check-chainlint` had not been run automatically by `make test`.