From: Jeff King <peff@peff.net>
Date: Tue, 31 Jul 2018 12:50:27 GMT
Subject: Re: [PATCH v2 01/10] t/test-lib: teach --chain-lint to detect broken &&-chains in subshells
Message-ID: <20180731125026.GA8072@sigill.intra.peff.net>
In-Reply-To: <CAPig+cRTgh6DStUdmXqvhbL_7sQY6wu21h27rjq_i=kZ_d+LAw@mail.gmail.com>

On Mon, Jul 30, 2018 at 05:38:06PM -0400, Eric Sunshine wrote:

> > I wonder if it should look for something like [A-Z][A-Z_]* to catch
> > all of these.
> 
> I considered that, but it doesn't handle nested here-docs, which we
> actually have in the test suite. For instance, from t9300-fast-import:
> 
>     cat >input <<-INPUT_END &&
>     mark :2
>     data <<EOF
>     $file2_data
>     EOF
>     ...
>     INPUT_END
> 
> Nesting could be handled easily enough either by stashing away the
> opening tag and matching against it later _or_ by doing recursive
> here-doc folding, however, 'sed' isn't a proper programming language
> and can't be coerced into doing either of those. (And, it was tricky
> enough just getting it to handle the nested case with a limited set of
> recognized tag names, without having to explicitly handle every
> combination of those names nested inside one another.)

I hesitate to make any suggestion here, as I think we may have passed
a point of useful cost/benefit in sinking more time into this script.
But...is switching to awk or perl an option? Our test suite already
depends on having a vanilla perl, so I don't think it would be a new
dependency. And it would give you actual data structures.

But like I said, it may not be worth it. I'd be OK just adjusting the
false positive and moving on.

> I am, for a couple reasons, somewhat hesitant to tweak the heuristic.
> 
> First, each tweak has the potential of causing more false-positives or
> (perhaps worse) false-negatives. The linter's own test-suite is
> supposed to protect against that, but test suite coverage is never
> perfect.
> 
> Second, ideally, the linter should protect against new broken
> &&-chains from entering the codebase, so poorly coded historic tests
> such as these aren't necessarily good motivation for tweaking, _and_
> it is (hopefully) unlikely that we would allow this sort of ugly shell
> code to enter the codebase going forward. (The counterargument is that
> this false-positive doesn't help someone coding up a new test who
> hasn't yet submitted the patch to the mailing list where more seasoned
> eyes would suggest better coding style.)

Right, I think the real cost is somebody who adds "<<CUSTOM_END_TAG"
later and is confused when they see the breakage. I.e., I don't mind
saying "we have a couple of style rules that you must follow to appease
the linter". But if the error message is not clear, it can send somebody
down the wrong rabbit hole trying to figure out what is going on.

-Peff