On 25/03/2023 08:04, Jeff King wrote:

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

quoted

quoted

quoted

This looks like the right solution. I do wonder how Phillip managed to
miss it, though, since the test script complains loudly.

I am unable to reproduce any linting errors when running this script
through chainlint, which is why I was more than a little confused by
this patch when I read it, and I was just about to ask for more
information, such as the actual error message.

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.

Ah, that explains it, I was wondering how the CI run had passed. Thanks 
to Michael for the patch and Peff and Eric for digging into cause of the 
problem

Best Wishes

Phillip

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

   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.

      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.

-Peff