Re: [PATCH 0/4] chainlint: improve annotated output

From: Jeff King <peff@peff.net>
Date: Nov 9, 2022, 13:11 UTC
Message-ID: <Y2unEeio8cgmBWCX@coredump.intra.peff.net>
In-Reply-To: <Y2q78ofF8fsAX8XU@nand.local>

On Tue, Nov 08, 2022 at 03:28:34PM -0500, Taylor Blau wrote:

Show 13 quoted lines

> On Tue, Nov 08, 2022 at 07:08:26PM +0000, Eric Sunshine via GitGitGadget wrote:
> > This patch series further improves the output by instead making chainlint.pl
> > annotate the original test definition rather than the parsed token stream,
> > thus preserving indentation (and whitespace, in general), here-doc bodies,
> > etc., which should make it easier for a test author to relate each problem
> > back to the source.
> 
> Very nicely done. The changes all seemed reasonable to me (and, in fact,
> the approach is pretty straightforward -- the diffstat is misleading
> since many of changes are to chainlint's expected output).
> 
> So I'm happy with it, but let's hear from some other folks who are more
> familiar with this area before we start merging it down.

I don't claim to be _that_ familiar with the code itself, but all of the patches look reasonable to me. And most importantly, I dug out the state of my tree from early September (via the reflog) before I fixed all of the chainlint problems on my local topics. The improvement in the output with this series is night and day.

I was a little surprised that using a class in patch 3 would cause such a slowdown. But it's not that hard to believe that the workload is so heavy on string comparison and manipulation that the overloaded string and comparison functions introduce significant overhead. It has been a long time since I've optimized any perl, but I remember the rule of thumb being to minimize the number of lines of perl (because all of the builtin stuff is blazingly fast C, and all of the perl is byte-code).

At any rate, the result you came up with doesn't look too bad. The only risk is that you forgot to s/$token/$token->[0]/ somewhere, and I suspect we'd have found that in running the tests.

So it all seems like a step forward to me.

-Peff