Re: [PATCH 00/18] make test "linting" more comprehensive

[Jeff King <peff@peff.net>]

On Thu, Sep 01, 2022 at 12:29:38AM +0000, Eric Sunshine via GitGitGadget wrote:

> A while back, Peff successfully nerd-sniped[1] me into tackling a
> long-brewing idea I had about (possibly) improving "chainlint" performance

Oops, sorry. :)

I gave this a read-through, and it looks sensible overall. I have to
admit that I did not carefully check all of your regexes. Given the
relatively low stakes of the code (as an internal build-time tool only)
and the set of tests accompanying it, I'm willing to assume it's good
enough until we see counter-examples.

I posted some timings and thoughts on the use of threads elsewhere. But
in the end the timings are close enough that I don't care that much
either way.

I'd also note that I got some first-hand experience with the script as I
merged it with all of my other long-brewing topics, and it found a half
dozen spots, mostly LOOP annotations. At least one was a real "oops,
we'd miss a bug in Git here" spot. Several were "we'd probably notice
the problem because the loop output wouldn't be as expected". One was a
"we're on the left-hand of a pipe, so the exit code doesn't matter
anyway" case, but I am more than happy to fix those if it lets us be
linter-clean.

The output took me a minute to adjust to, just because it feels pretty
jumbled when there are several cases. Mostly this is because the
script eats indentation. So it's hard to see the "# chainlint:" comment
starts, let alone the ?! annotations. Here's an example:

-- >8 --
# chainlint: t4070-diff-pairs.sh
# chainlint: split input across multiple diff-pairs
write_script split-raw-diff "$PERL_PATH" <<-EOF &&

git diff-tree -p -M -C -C base new > expect &&

git diff-tree -r -z -M -C -C base new |
./split-raw-diff &&
for i in diff* ; do
git diff-pairs -p < $i ?!LOOP?!
done > actual &&
test_cmp expect actual
# chainlint: perf/p5305-pack-limits.sh
# chainlint: set up delta islands
head=$(git rev-parse HEAD) &&
git for-each-ref --format="delete %(refname)" |
git update-ref --no-deref --stdin &&

n=0 &&
fork=0 &&
git rev-list --first-parent $head |
while read commit ; do
n=$((n+1)) ?!AMP?!
if test "$n" = 100 ; then
echo "create refs/forks/$fork/master $commit" ?!AMP?!
fork=$((fork+1)) ?!AMP?!
n=0
fi ?!LOOP?!
done |
git update-ref --stdin &&

git config pack.island "refs/forks/([0-9]*)/"
-- 8< --

It wasn't too bad once I got the hang of it, but I wonder if a user
writing a single test for the first time may get a bit overwhelmed.  I
assume that the indentation is removed as part of the normalization (I
notice extra whitespace around "<", too). That might be hard to address.

I wonder if color output for "# chainlint" and "?!" annotations would
help, too. It looks like that may be tricky, though, because the
annotations re-parsed internally in some cases.

-Peff