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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]

  Powered by Linux