Re: [PATCH v2 1/8] log tests: don't use "exit 1" outside a sub-shell

[Eric Sunshine <sunshine@sunshineco.com>]

On Fri, Dec 2, 2022 at 5:08 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
> On Fri, Dec 02 2022, Eric Sunshine wrote:
> > It might make more sense to turn this on its head and make it a
> > linting issue and simply throw a "?!FOO?!" as is done for other
> > suspect shell code. In fact, I already have local chainlint.pl patches
> > which detect whether a subshell is active so that the linter can
> > complain if it sees `cd` outside of a subshell. I would think that
> > warning about misuse of `exit 1` outside a subshell (and perhaps
> > `return 1` inside a subshell) should be possible, though I haven't
> > thought through all the possibilities.
>
> That would be great. As a reminder I think (maybe it's not what you have
> in mind exactly?) that we had a brief discussion on this topic starting
> at [1]. I.e. I was hoping chainlint.pl could be extended to detect
> exactly these sort of "test...$(git" patterns (among other things).

I recall it but haven't really put any additional thought into it. I
did put a little bit of thought into how to upgrade chainlint.pl to
supplant check-non-portable-shell.pl, though that doesn't help much
with your wish list. More recently, I've been thinking that it might
make more sense for chainlint.pl to generate an AST rather than
working against the raw token stream, which may mesh better with the
sort of rewriting you mentioned, and may simplify (or complexify) some
of the linting heuristics. I haven't really thought it through yet,
and I'm always concerned about slowing down the script too much -- but
perhaps I'm worrying unnecessarily if your Makefile-based
parallelization patches land, or if we just rip out parallelization
altogether as Peff was suggesting.

> This topic doesn't get us there, but once we finally get rid of some of
> those patterns it would be nice to have assertions to ensure they don't
> come back.

Ya.