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

On Mon, Dec 13, 2021 at 11:17 AM Ævar Arnfjörð Bjarmason
[off-list ref] wrote:

On Mon, Dec 13 2021, Eric Sunshine wrote:

quoted

On Mon, Dec 13, 2021 at 10:43 AM Fabian Stelzer [off-list ref] wrote:

quoted

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