Re: [PATCH 0/3] leak tests: mark remaining tests leak-free as such

On Thu, Aug 24, 2023 at 02:40:34PM -0400, Taylor Blau wrote:

While working on another topic that cleared up some leaks, I wanted to
see if any new tests became leak-free, so I ran:

    $ make SANITIZE=leak
    $ make GIT_TEST_PASSING_SANITIZE_LEAK=check GIT_TEST_OPTS=-i test

Is that exactly what you ran? Because I'd expect the second "make"
invocation to rebuild Git _without_ SANITIZE=leak enabled in that case.
(Though I would have then expected most of the scripts to complain
loudly about the mismatch; did you "cd t" in between the two?).

 t/t3321-notes-stripspace.sh | 1 +
 t/t5571-pre-push-hook.sh    | 1 +
 t/t5583-push-branches.sh    | 1 +
 t/t7516-commit-races.sh     | 2 ++
 4 files changed, 5 insertions(+)

If I run a single:

  make SANITIZE=leak GIT_TEST_PASSING_SANITIZE_LEAK=check GIT_TEST_OPTS=-i test

on v2.42.0, I get many hits. All of the ones you mentioned, plus:

  t7408 t5407 t7008 t5811 t3407 t6001 t4058 t2016

If I run a few by hand, I _do_ see leaks in them, but the exit codes are
hidden from the test suite (they are sub-programs of scripts, etc). I
guess you also have:

  GIT_TEST_SANITIZE_LEAK_LOG=true

set, which should find those (and which you mention in your first
commit). Turning that on eliminates some of them, but I'm left with:

  t5614 t5317 t5503

not in your list. Which is super weird, because t5614 is marked with
TEST_PASSES_SANITIZE_LEAK. Hrm. And if I run it again, I get a
_different_ set (t5614 again, along with your 4, but also t5303, t7701,
and t4050). I wonder if we have a race in the leak-log code or
something (I'm running under prove with -j32, naturally).

This series marks all leak-free tests as such, meaning that the above
"make test" invocation will pass after this series. The bulk of the
tests which are marked here in the first patch were always
leak-free[^1]. The remaining two patches address a couple of special
cases of tests which are also leak-free.

Hmm. If I check t5571, for example, by bisecting on:

  make SANITIZE=leak && (cd t && ./t5571-pre-push-hook.sh -v -i)

it shows that it was fixed by 861c56f6f9 (branch: fix a leak in
setup_tracking, 2023-06-11), which make sense. There are a bunch of leak
fixes in the same series, which makes me wonder if they're responsible
for most of these.

If the leaks are gone, I am happy that we are marking them. But it is
weird to me that we are getting different results.

-Peff