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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Dec 13, 2021, 19:37 UTC
Message-ID: <211213.86v8zs70mo.gmgdl@evledraar.gmail.com>
In-Reply-To: <CAPig+cTDu+gXOTeBHALCuS9ZqvuqxQW-gXdYUsyAxTOT=gHEnA@mail.gmail.com>

On Mon, Dec 13 2021, Eric Sunshine wrote:

Show 20 quoted lines

> On Mon, Dec 13, 2021 at 11:29 AM Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
>> On Mon, Dec 13 2021, Eric Sunshine wrote:
>> > It's not clear whether the intention is to maintain the &&-chain in
>> > this function...
>> > ... or not care about it since it's broken here before `shift`...
>>
>> Thanks, those should all use &&-chaining. Will fix.
>
> 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.

E.g. if you run the tests with SANITIZE=leak and GIT_TEST_PASSING_SANITIZE_LEAK=true in "next" now you'll find it passes, but we'll still (especially if you log them or --verbose-log) have memory leaks in various places still.

Those aren't new issues in anything I've done in the leak testing mode, although I'm hoping to eventually getting around to fixing them. They're just cases where tests pass because some function is lacking a &&.

Although to be fair the SANITIZE=leak default is to die at the very end, so if the program had something useful to do it probably got around to doing it, but that doesn't apply when we invoke ourselves via run-command.c (as that invocation will fail, and we'll usually abort).

But it would have been nice to have had those failures cascade up from the functions up to the top-level.

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