Re: [PATCH 10/13] test-lib-functions: add and use a "write_hook" wrapper

[Eric Sunshine <sunshine@sunshineco.com>]

On Mon, Dec 13, 2021 at 2:42 PM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> On Mon, Dec 13 2021, Eric Sunshine wrote:
> > 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.