On Sat, Mar 25, 2023 at 4:04 AM Jeff King [off-list ref] wrote:

On Sat, Mar 25, 2023 at 03:58:32AM -0400, Jeff King wrote:

quoted

It's not your chain-lint script, but rather the builtin one that sticks
"(exit 117) &&" in front of the snippet and evals it. So it creates the
exact "foo && bar &" situation by prepending a line to the snippet.

And btw, I think that is the answer to "how did Phillip not notice it?".
When running "make test" these days, we rely on chainlint.pl to detect
any problems, and then set GIT_TEST_CHAIN_LINT=0 so that the scripts do
not invoke it again. But that variable also suppresses the internal
linter, and thus "make test" passes, but running the script individually
does not.

Indeed, that would explain it.

It does seem like a recipe for confusion if the two linters are not in
agreement. I think we might want to either:

  1. Say that the internal linter still has value, and tweak the
     suppression so it only turns off the extra per-script run of
     chainlint.pl, and not the internal one (which is cheap-ish to run).

This is appealing since the internal linter is nearly zero-cost,
though doing this would not fully address the "recipe for confusion"
since the two linters would still not be in agreement. This approach
does have the benefit that it gives at least _some_ protection (minus
caveats mentioned below) on platforms where it may be common to
disable chainlint.pl due to slowness, such as Windows.

  2. Say that the internal linter does not have value, and we should
     rely on chainlint.pl. In which case we might as well ditch the
     internal one completely.

The value of the internal linter is fairly limited in that it only
checks top-level &&-chain; it doesn't check inside subprocesses,
blocks, or any compound statement (case/esac, if/fi, while/done,
etc.).

     I'm OK with this direction, if we're comfortable that there are no
     real problems that would be caught by the internal one but not the
     script.

I retained the internal linter in place "just in case" (i.e. in the
event the script missed something legitimate), but I don't feel
strongly about it.