On Mon, Dec 13 2021, Eric Sunshine wrote:
> On Mon, Dec 13, 2021 at 11:29 AM Ævar Arnfjörð Bjarmason
> <avarab@xxxxxxxxx> 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".
