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
