Re: [PATCH] t3070: make chain lint tester happy

From: Jeff King <peff@peff.net>
Date: Mar 25, 2023, 19:47 UTC
Message-ID: <20230325194700.GA186113@coredump.intra.peff.net>
In-Reply-To: <CAPig+cQtLFX4PgXyyK_AAkCvg4Aw2RAC5MmLbib-aHHgTBcDuw@mail.gmail.com>

On Sat, Mar 25, 2023 at 05:09:15AM -0400, Eric Sunshine wrote:

Show 21 quoted lines

> > Hmm, actually chainlint.pl does not seem to catch this:
> >
> > -- >8 --
> > test_expect_success 'ok, first line cannot break &&-chain' '
> >         true &
> >         pid=$!
> > '
> >
> > test_expect_success 'not ok, failure is lost' '
> >         false &&
> >         true &
> >         pid=$!
> > '
> > -- >8 --
> 
> Right, that's one of the "special cases" I mentioned earlier; an
> intentional simplification of implementation to keep the complexity
> level down. Although the linter is genuinely parsing the shell code,
> it doesn't really understand or check shell semantics, and is just
> using simple heuristics to detect the common types of &&-breakage and
> missing `return 1`.

Ah, OK. I thought the special case was ignoring the first one (which you probably need to do to make "{ cmd & pid=$!; }" work inside braces), not the second.

> This particular simplification is that if it encounters one of the
> special cases in which some construct (such as "&") should not be
> considered as a break in the &&-chain, it clears all "??AMP??" flags
> which come before that point in the current parse context.

That makes sense, though it does mean that an easy typo ("&" for "&&") wouldn't get caught. I don't recall this case happening a lot, though.

It does make me inclined to keep the built-in checker, just because we know it can catch this case (at least at the top-level of the snippet).

Show 11 quoted lines

> More specifically, it's not even building a parse tree; it's just
> trying to detect problems on-the-fly as it parses, so when it finds
> something like "&" which is _not_ a breakage, it can't easily go back
> and recheck which earlier "??AMP??" annotations are still needed. So,
> it just clears the earlier ones unconditionally with the hope of not
> letting too many false-negatives through. It would certainly be
> possible to make it do a better job of detection, but doing so would
> complicate the code quite a bit. (Eventually, I think it would be best
> to build a parse tree, which would make it easier to incorporate other
> linting ideas I have in mind, but I don't have any immediate plans to
> do so.)

Yeah, I certainly don't think this case merits all that effort. I'm thinking it is worth tweaking CHAINLINTSUPPRESS so that the internal one is run by "make test", though.

-Peff