On Sat, Mar 25, 2023 at 05:09:15AM -0400, Eric Sunshine wrote:
> > 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).
> 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
