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

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

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