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

[Eric Sunshine <sunshine@sunshineco.com>]

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

> 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`.