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

quoted

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