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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Mar 25, 2023, 09:09 UTC
Message-ID: <CAPig+cQtLFX4PgXyyK_AAkCvg4Aw2RAC5MmLbib-aHHgTBcDuw@mail.gmail.com>
In-Reply-To: <20230325085127.GA26684@coredump.intra.peff.net>

On Sat, Mar 25, 2023 at 4:51 AM Jeff King <peff@peff.net> wrote:

Show 22 quoted lines

> On Sat, Mar 25, 2023 at 04:41:08AM -0400, Jeff King wrote:
> > Right, the chainlint.pl one is much more thorough. I just wondered if
> > there were any cases we were worried about it missing, that the internal
> > one catches. We found one in this thread, but as discussed, it is not a
> > problem (presumably chainlint.pl catches a "real" case where an
> > earlier line is hidden by the "&", but I wouldn't mind seeing it
> > complain here as a matter of style/future-proofing).
>
> 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`.

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.

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.)

Show 5 quoted lines

> It's a little funny, because we actually background the whole "false &&
> true" chain. So if you did "wait $pid" at the end, you would see the
> failure. But the test in this thread doesn't actually do that (it
> depends on kill after 2 seconds not finding the pid). Plus in general
> this seems like an accident that we should be flagging.

As above, it was a judgement call regarding linter implementation complexity versus letting a few potential breakages slip through undetected. As it stands, we get a pretty big pay off detecting >99.9% of real-world breakage without the additional complexity. (That is not an argument against improving its accuracy in the future, but rather an explanation of the current state of the linter.)