On Mon, Dec 13, 2021 at 2:42 PM Ævar Arnfjörð Bjarmason
[off-list ref] wrote:

On Mon, Dec 13 2021, Eric Sunshine wrote:

quoted

By the way, the new chainlint could be made to catch broken &&-chains
(and missing `|| return 1`) in test script functions, as well; it
doesn't have to limit its checks only to tests. The reason I haven't
done so yet is that it's not clear how much we care about &&-chains in
functions, especially since we have _so many_ functions which don't
maintain the &&-chain. In the long run, I think it might be beneficial
to extend chainlint to check shell functions too, but fixing the
&&-chains in functions probably have to be done incrementally, thus
would likely require some sort of whitelisting or blacklisting
mechanism until all functions have been fixed. Anyhow, it's food for
thought.

I think doing that & phasing it in would be very useful.

I forgot to mention another reason that I haven't really thought yet
about tackling the linting of shell functions, which is that some
shell functions drive tests:

    test_it () {
        foo=$1
        bar=$2
        test_expect_sucess "something $foo" '
            do_it "$bar"
        '
    }

in which an &&-chain within the function body isn't meaningful,
whereas other functions are called by tests:

    cmp_it () {
        a=$1 &&
        b=$2 &&
        test_cmp "$a" "$b"
    }

    test_expect_success 'something' '
        echo foo >foo &&
        echo bar >bar &&
        cmp_it foo bar
    '

in which the &&-chain within the function body is important.

It _may_ be possible to figure out automatically into which category a
function falls, but would probably be easier to have some sort of
annotation mechanism to distinguish one category of function from the
other, and only validate a function which falls into the latter
category.

We've also said we shouldn't use things like this, i.e. a pipe with git
on the LHS:

    git <cmd> | ... &&

But I've run into a few cases where a test succeeds, even if both
commands here die:

    test "$(git <cmd>)" = "$(git <cmd2>)"

Which, if we're adding more lints is maybe something to consider
too. I.e. it falls under the general umbrella of cases where we'd hide
failures in "git".

Ya, this is a nice example, among others, of questionable code which a
linter might be able to detect. In fact, while working on the new
chainlint, I noted that it would be possible to replace
t/check-non-portable-shell.pl by adding a few more rules to the new
linter. A primary benefit of doing so is that
check-non-portable-shell.pl takes about 2.5 seconds to run on my
(admittedly 10+ year old) machine. However, I'm also hesitant to do so
since there is value in having those checks reside in a standalone
script like that; it's such a simple script that anyone can add new
checks without having to spend a lot of time studying how to do so.