[PATCH 0/4] mark tests as leak-free

[PATCH 0/4] mark tests as leak-free

[Rubén Justo]

The tests: t0080, t5332 and t6113 can be annotated as leak-free.

I used:
  $ make SANITIZE=leak GIT_TEST_PASSING_SANITIZE_LEAK=check GIT_TEST_SANITIZE_LEAK_LOG=true test

Rubén Justo (4):
  t0080: mark as leak-free
  t5332: mark as leak-free
  t6113: mark as leak-free
  test-lib: check for TEST_PASSES_SANITIZE_LEAK

 t/t0080-unit-test-output.sh        | 1 +
 t/t5332-multi-pack-reuse.sh        | 1 +
 t/t6113-rev-list-bitmap-filters.sh | 3 ++-
 t/test-lib.sh                      | 5 +++++
 4 files changed, 9 insertions(+), 1 deletion(-)

-- 
2.43.0

[PATCH 1/4] t0080: mark as leak-free

[Rubén Justo]

This test is leak-free since it was added in e137fe3b29 (unit tests: add
TAP unit test framework, 2023-11-09)

Let's mark it as leak-free to make sure it stays that way (and to reduce
noise when looking for other leak-free scripts after we fix some leaks).

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 t/t0080-unit-test-output.sh | 1 +
 1 file changed, 1 insertion(+)

diff --git a/t/t0080-unit-test-output.sh b/t/t0080-unit-test-output.sh
index 961b54b06c..6657c114a3 100755
--- a/t/t0080-unit-test-output.sh
+++ b/t/t0080-unit-test-output.sh
@@ -2,6 +2,7 @@
 
 test_description='Test the output of the unit test framework'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'TAP output from unit tests' '
-- 
2.43.0

Re: [PATCH 1/4] t0080: mark as leak-free

[Junio C Hamano]

Rubén Justo <rjusto@gmail.com> writes:

> This test is leak-free since it was added in e137fe3b29 (unit tests: add
> TAP unit test framework, 2023-11-09)
>
> Let's mark it as leak-free to make sure it stays that way (and to reduce
> noise when looking for other leak-free scripts after we fix some leaks).

For other tests in this series, that rationale is a very sensible
thing, but does it apply to this one?

The point of the t-basic tests is to ensure the lightweight unit
test framework that requires nothing from Git behaves (and keeps
behaving) sensibly.  The point of running t[0-9][0-9][0-9][0-9]
tests under leak sanitizer is to exercise production Git code to
catch leaks in Git code.

So it is not quite clear if we even want to run this t0080 under
leak sanitizer to begin with.  t0080 is a relatively tiny test, but
do we even want to spend leak sanitizer cycles on it?  I dunno.

> Signed-off-by: Rubén Justo <rjusto@gmail.com>
> ---
>  t/t0080-unit-test-output.sh | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/t/t0080-unit-test-output.sh b/t/t0080-unit-test-output.sh
> index 961b54b06c..6657c114a3 100755
> --- a/t/t0080-unit-test-output.sh
> +++ b/t/t0080-unit-test-output.sh
> @@ -2,6 +2,7 @@
>  
>  test_description='Test the output of the unit test framework'
>  
> +TEST_PASSES_SANITIZE_LEAK=true
>  . ./test-lib.sh
>  
>  test_expect_success 'TAP output from unit tests' '

Re: [PATCH 1/4] t0080: mark as leak-free

[Rubén Justo]

On 29-ene-2024 14:15:15, Junio C Hamano wrote:
> Rubén Justo <rjusto@gmail.com> writes:
> 
> > This test is leak-free since it was added in e137fe3b29 (unit tests: add
> > TAP unit test framework, 2023-11-09)
> >
> > Let's mark it as leak-free to make sure it stays that way (and to reduce
> > noise when looking for other leak-free scripts after we fix some leaks).
> 
> For other tests in this series, that rationale is a very sensible
> thing, but does it apply to this one?
> 
> The point of the t-basic tests is to ensure the lightweight unit
> test framework that requires nothing from Git behaves (and keeps
> behaving) sensibly.  The point of running t[0-9][0-9][0-9][0-9]
> tests under leak sanitizer is to exercise production Git code to
> catch leaks in Git code.
> 
> So it is not quite clear if we even want to run this t0080 under
> leak sanitizer to begin with.  t0080 is a relatively tiny test, but
> do we even want to spend leak sanitizer cycles on it?  I dunno.

IIUC, that would imply building test-tool with a different set of flags
than Git, new artifacts ...  or running test-tool with some LSAN_OPTIONS
options, to disable it ...  or both ... or ...

And that is assuming that with test-tool we won't catch a leak in Git
that we're not seeing in the other tests ...

Maybe this is tangential to this series but,  while a decision is being
made, annotating the test makes GIT_TEST_PASSING_SANITIZE_LEAK=check
pass, which is the objective in this series. 

> 
> > Signed-off-by: Rubén Justo <rjusto@gmail.com>
> > ---
> >  t/t0080-unit-test-output.sh | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/t/t0080-unit-test-output.sh b/t/t0080-unit-test-output.sh
> > index 961b54b06c..6657c114a3 100755
> > --- a/t/t0080-unit-test-output.sh
> > +++ b/t/t0080-unit-test-output.sh
> > @@ -2,6 +2,7 @@
> >  
> >  test_description='Test the output of the unit test framework'
> >  
> > +TEST_PASSES_SANITIZE_LEAK=true
> >  . ./test-lib.sh
> >  
> >  test_expect_success 'TAP output from unit tests' '

Re: [PATCH 1/4] t0080: mark as leak-free

[Junio C Hamano]

Rubén Justo <rjusto@gmail.com> writes:

>> The point of the t-basic tests is to ensure the lightweight unit
>> test framework that requires nothing from Git behaves (and keeps
>> behaving) sensibly.  The point of running t[0-9][0-9][0-9][0-9]
>> tests under leak sanitizer is to exercise production Git code to
>> catch leaks in Git code.
>> 
>> So it is not quite clear if we even want to run this t0080 under
>> leak sanitizer to begin with.  t0080 is a relatively tiny test, but
>> do we even want to spend leak sanitizer cycles on it?  I dunno.
>
> IIUC, that would imply building test-tool with a different set of flags
> than Git, new artifacts ...  or running test-tool with some LSAN_OPTIONS
> options, to disable it ...  or both ... or ...
>
> And that is assuming that with test-tool we won't catch a leak in Git
> that we're not seeing in the other tests ...

But t0080 does not even run test-tool, does it?  The t-basic unit
test is about testing the unit test framework and does not even
trigger any of the half-libified Git code.  So I am not sure why
you are bringing up test-tool into the picture.

> Maybe this is tangential to this series but,  while a decision is being
> made, annotating the test makes GIT_TEST_PASSING_SANITIZE_LEAK=check
> pass, which is the objective in this series. 

One major reason why we want to set TEST_PASSES_SANITIZE_LEAK to
true is because that way the marked test will be run under the leak
sanitizer in the CI.

What do we expect to gain by running t0080, which is to run the
t-basic unit test, under the leak sanitizer?  Unlike other
t[0-9][0-9][0-9][0-9] tests that exercise Git production code, would
we care about a new leak found in t-basic run from t0080 in the
first place?

Annotating with TEST_PASSES_SANITIZE_LEAK is not a goal by itself.
Annotating the tests that we want to run under the sanitizer and see
them passing with it is.  And obviously these tests that exercise
Git production code are very good candidates for us to do so.  It is
unclear if t0080 falls into the same category.  That is why I asked
what we expect to gain by running it.

Thanks.

Re: [PATCH 1/4] t0080: mark as leak-free

[Rubén Justo]

On 29-ene-2024 15:51:33, Junio C Hamano wrote:
> Rubén Justo <rjusto@gmail.com> writes:
> 
> >> The point of the t-basic tests is to ensure the lightweight unit
> >> test framework that requires nothing from Git behaves (and keeps
> >> behaving) sensibly.  The point of running t[0-9][0-9][0-9][0-9]
> >> tests under leak sanitizer is to exercise production Git code to
> >> catch leaks in Git code.
> >> 
> >> So it is not quite clear if we even want to run this t0080 under
> >> leak sanitizer to begin with.  t0080 is a relatively tiny test, but
> >> do we even want to spend leak sanitizer cycles on it?  I dunno.
> >
> > IIUC, that would imply building test-tool with a different set of flags
> > than Git, new artifacts ...  or running test-tool with some LSAN_OPTIONS
> > options, to disable it ...  or both ... or ...
> >
> > And that is assuming that with test-tool we won't catch a leak in Git
> > that we're not seeing in the other tests ...
> 
> But t0080 does not even run test-tool, does it?  The t-basic unit
> test is about testing the unit test framework and does not even
> trigger any of the half-libified Git code.  So I am not sure why
> you are bringing up test-tool into the picture.

Of course, test-tool has nothing to do here.  I think I got distracted
because:

  $ ( cd t; ./t0080-unit-test-output.sh )
  Bail out! You need to build test-tool; Run "make t/helper/test-tool" in the source (toplevel) directory

My reasoning was about t/unit-test/bin/t-basic (though also applies to
test-tool), due to:

  $ make SANITIZE=leak -n t/unit-tests/bin/t-basic 
  ...
  echo '   ' LINK t/unit-tests/bin/t-basic;cc   -g -O2 -Wall -I. \
  -DHAVE_SYSINFO -fsanitize=leak -fno-sanitize-recover=leak \
  -fno-omit-frame-pointer -DSUPPRESS_ANNOTATED_LEAKS -O0 \
  -DGIT_HOST_CPU="\"x86_64\"" -DHAVE_ALLOCA_H  -DUSE_CURL_FOR_IMAP_SEND \
  -DSUPPORTS_SIMPLE_IPC -DSHA1_DC -DSHA1DC_NO_STANDARD_INCLUDES \
  -DSHA1DC_INIT_SAFE_HASH_DEFAULT=0 \
  -DSHA1DC_CUSTOM_INCLUDE_SHA1_C="\"git-compat-util.h\"" \
  -DSHA1DC_CUSTOM_INCLUDE_UBC_CHECK_C="\"git-compat-util.h\"" -DSHA256_BLK \
  -DHAVE_PATHS_H -DHAVE_DEV_TTY -DHAVE_CLOCK_GETTIME \
  -DHAVE_CLOCK_MONOTONIC -DHAVE_SYNC_FILE_RANGE -DHAVE_GETDELIM \
  '-DPROCFS_EXECUTABLE_PATH="/proc/self/exe"' -DFREAD_READS_DIRECTORIES \
  -DNO_STRLCPY -DSHELL_PATH='"/bin/sh"' -o t/unit-tests/bin/t-basic   \
  t/unit-tests/t-basic.o t/unit-tests/test-lib.o common-main.o libgit.a \
  xdiff/lib.a reftable/libreftable.a libgit.a xdiff/lib.a \
  reftable/libreftable.a libgit.a -lz -lpthread -lrt

Note that we inject this flags:
  -fsanitize=leak -fno-sanitize-recover=leak -fno-omit-frame-pointer \
  -DSUPPRESS_ANNOTATED_LEAKS -O0

> 
> > Maybe this is tangential to this series but,  while a decision is being
> > made, annotating the test makes GIT_TEST_PASSING_SANITIZE_LEAK=check
> > pass, which is the objective in this series. 
> 
> One major reason why we want to set TEST_PASSES_SANITIZE_LEAK to
> true is because that way the marked test will be run under the leak
> sanitizer in the CI.
> 
> What do we expect to gain by running t0080, which is to run the
> t-basic unit test, under the leak sanitizer?  Unlike other
> t[0-9][0-9][0-9][0-9] tests that exercise Git production code, would
> we care about a new leak found in t-basic run from t0080 in the
> first place?
> 
> Annotating with TEST_PASSES_SANITIZE_LEAK is not a goal by itself.

Indeed.  It points to a horizon.

> Annotating the tests that we want to run under the sanitizer and see
> them passing with it is.

Maybe this is also a horizon (not reachable by definition), and
expecting "make test" to be leak-free (including t0080) a good path
towards that horizon, IMHO.  But you are right, those leak sanitizer
cycles may not be worth it.

> And obviously these tests that exercise
> Git production code are very good candidates for us to do so.  It is
> unclear if t0080 falls into the same category.  That is why I asked
> what we expect to gain by running it.
> 
> Thanks.

Thank you for bringing up a good question.

I see you queued this as 4/4.  OK.  I'll consider that if a re-roll for
this series is needed.

Re: [PATCH 1/4] t0080: mark as leak-free

[Jeff King]

On Mon, Jan 29, 2024 at 02:15:15PM -0800, Junio C Hamano wrote:

> > Let's mark it as leak-free to make sure it stays that way (and to reduce
> > noise when looking for other leak-free scripts after we fix some leaks).
> 
> For other tests in this series, that rationale is a very sensible
> thing, but does it apply to this one?
> 
> The point of the t-basic tests is to ensure the lightweight unit
> test framework that requires nothing from Git behaves (and keeps
> behaving) sensibly.  The point of running t[0-9][0-9][0-9][0-9]
> tests under leak sanitizer is to exercise production Git code to
> catch leaks in Git code.
> 
> So it is not quite clear if we even want to run this t0080 under
> leak sanitizer to begin with.  t0080 is a relatively tiny test, but
> do we even want to spend leak sanitizer cycles on it?  I dunno.

I think you are right that we do not particularly care about leaks in
the t-basic code. That is also true of other test harness code (other
unit-tests, but also stuff in t/helper). But IMHO it is less work to
just keep that code leak-free than it is to try to distinguish between
production and test code.

Right now, it is not that hard to simply leave the PASSES_SANITIZE_LEAK
flag off of t0080, and then it won't be run in the leak-checking CI job.
But I think the end-game of all of this leak-checking stuff is that
eventually _everything_ will be leak-free, and we will discard the whole
PASSES_SANITIZE_LEAK mechanism entirely. And in that end-game, it is
simpler for everything, including t-basic, to just be leak-free and
checked under the same regime.

Setting the flag now just makes sure we continue correctly on that path,
rather than getting surprised near the end of the road that t-basic has
some dumb leak. Plus it avoids the script popping up as a false positive
when checking for scripts which can be marked.

-Peff

Re: [PATCH 1/4] t0080: mark as leak-free

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Setting the flag now just makes sure we continue correctly on that path,
> rather than getting surprised near the end of the road that t-basic has
> some dumb leak. Plus it avoids the script popping up as a false positive
> when checking for scripts which can be marked.

Alright.  Any such "dumb leak" in the "basic" would hopefully be
caught by the real t-$other unit tests exercised under the leak
sanitizer, I hope, but it is not worth our time wondering if it
makes sense to special case t0080 specifically.

[PATCH 2/4] t5332: mark as leak-free

[Rubén Justo]

This test is leak-free since it was added in af626ac0e0 (pack-bitmap:
enable reuse from all bitmapped packs, 2023-12-14).

Let's mark it as leak-free to make sure it stays that way (and to reduce
noise when looking for other leak-free scripts after we fix some leaks).

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 t/t5332-multi-pack-reuse.sh | 1 +
 1 file changed, 1 insertion(+)

diff --git a/t/t5332-multi-pack-reuse.sh b/t/t5332-multi-pack-reuse.sh
index 2ba788b042..99145327a6 100755
--- a/t/t5332-multi-pack-reuse.sh
+++ b/t/t5332-multi-pack-reuse.sh
@@ -2,6 +2,7 @@
 
 test_description='pack-objects multi-pack reuse'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-bitmap.sh
 
-- 
2.43.0

[PATCH 3/4] t6113: mark as leak-free

[Rubén Justo]

This test does not leak since a96015a517 (pack-bitmap: plug leak in
find_objects(), 2023-12-14) when the annotation
TEST_PASSES_SANITIZE_LEAK=true was also added.

Unfortunately it was added after test-lib.sh is sourced, which makes
GIT_TEST_PASSING_SANITIZE_LEAK=check error:

   $ make SANITIZE=leak GIT_TEST_PASSING_SANITIZE_LEAK=check test T=t6113-rev-list-bitmap-filters.sh
   ...
   make[2]: Entering directory '/tmp/git/git/t'
   *** t6113-rev-list-bitmap-filters.sh ***
   in GIT_TEST_PASSING_SANITIZE_LEAK=check mode, setting --invert-exit-code for TEST_PASSES_SANITIZE_LEAK != true
   ok 1 - set up bitmapped repo
   ok 2 - filters fallback to non-bitmap traversal
   ok 3 - blob:none filter
   ok 4 - blob:none filter with specified blob
   ok 5 - blob:limit filter
   ok 6 - blob:limit filter with specified blob
   ok 7 - tree:0 filter
   ok 8 - tree:0 filter with specified blob, tree
   ok 9 - tree:1 filter
   ok 10 - object:type filter
   ok 11 - object:type filter with --filter-provided-objects
   ok 12 - combine filter
   ok 13 - combine filter with --filter-provided-objects
   ok 14 - bitmap traversal with --unpacked
   # passed all 14 test(s)
   1..14
   # faking up non-zero exit with --invert-exit-code
   make[2]: *** [Makefile:68: t6113-rev-list-bitmap-filters.sh] Error 1
   make[2]: Leaving directory '/tmp/git/git/t'
   make[1]: *** [Makefile:55: test] Error 2
   make[1]: Leaving directory '/tmp/git/git/t'
   make: *** [Makefile:3212: test] Error 2

Let's move the annotation before sourcing test-lib.sh, to make
GIT_TEST_PASSING_SANITIZE_LEAK=check happy.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 t/t6113-rev-list-bitmap-filters.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/t/t6113-rev-list-bitmap-filters.sh b/t/t6113-rev-list-bitmap-filters.sh
index 459f0d7412..a9656a1ec8 100755
--- a/t/t6113-rev-list-bitmap-filters.sh
+++ b/t/t6113-rev-list-bitmap-filters.sh
@@ -1,10 +1,11 @@
 #!/bin/sh
 
 test_description='rev-list combining bitmaps and filters'
+
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-bitmap.sh
 
-TEST_PASSES_SANITIZE_LEAK=true
 
 test_expect_success 'set up bitmapped repo' '
 	# one commit will have bitmaps, the other will not
-- 
2.43.0

[PATCH 4/4] test-lib: check for TEST_PASSES_SANITIZE_LEAK

[Rubén Justo]

TEST_PASSES_SANITIZE_LEAK must be set before sourcing test-lib.sh, as we
say in t/README:

   GIT_TEST_PASSING_SANITIZE_LEAK=true skips those tests that haven't
   declared themselves as leak-free by setting
   "TEST_PASSES_SANITIZE_LEAK=true" before sourcing "test-lib.sh". This
   test mode is used by the "linux-leaks" CI target.

   GIT_TEST_PASSING_SANITIZE_LEAK=check checks that our
   "TEST_PASSES_SANITIZE_LEAK=true" markings are current. Rather than
   skipping those tests that haven't set "TEST_PASSES_SANITIZE_LEAK=true"
   before sourcing "test-lib.sh" this mode runs them with
   "--invert-exit-code". This is used to check that there's a one-to-one
   mapping between "TEST_PASSES_SANITIZE_LEAK=true" and those tests that
   pass under "SANITIZE=leak". This is especially useful when testing a
   series that fixes various memory leaks with "git rebase -x".

In a recent commit we fixed a test where it was set after sourcing
test-lib.sh, leading to confusing results.

To prevent future oversights, let's add a simple check to ensure the
value for TEST_PASSES_SANITIZE_LEAK remains unchanged at test_done().

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 t/test-lib.sh | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index fc93aa57e6..042f557a6f 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -1297,6 +1297,11 @@ test_done () {
 		EOF
 	fi
 
+	if test -z "$passes_sanitize_leak" && test_bool_env TEST_PASSES_SANITIZE_LEAK false
+	then
+		BAIL_OUT "Please, set TEST_PASSES_SANITIZE_LEAK before sourcing test-lib.sh"
+	fi
+
 	if test "$test_fixed" != 0
 	then
 		say_color error "# $test_fixed known breakage(s) vanished; please update test(s)"
-- 
2.43.0

Re: [PATCH 4/4] test-lib: check for TEST_PASSES_SANITIZE_LEAK

[Junio C Hamano]

Rubén Justo <rjusto@gmail.com> writes:

> In a recent commit we fixed a test where it was set after sourcing
> test-lib.sh, leading to confusing results.

OK, the reference is to the previous step.  Makes sense.

>
> To prevent future oversights, let's add a simple check to ensure the
> value for TEST_PASSES_SANITIZE_LEAK remains unchanged at test_done().
>
> Signed-off-by: Rubén Justo <rjusto@gmail.com>
> ---
>  t/test-lib.sh | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index fc93aa57e6..042f557a6f 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -1297,6 +1297,11 @@ test_done () {
>  		EOF
>  	fi
>  
> +	if test -z "$passes_sanitize_leak" && test_bool_env TEST_PASSES_SANITIZE_LEAK false
> +	then
> +		BAIL_OUT "Please, set TEST_PASSES_SANITIZE_LEAK before sourcing test-lib.sh"
> +	fi
> +
>  	if test "$test_fixed" != 0
>  	then
>  		say_color error "# $test_fixed known breakage(s) vanished; please update test(s)"

Re: [PATCH 0/4] mark tests as leak-free

[Jeff King]

On Mon, Jan 29, 2024 at 10:04:10PM +0100, Rubén Justo wrote:

> The tests: t0080, t5332 and t6113 can be annotated as leak-free.
> 
> I used:
>   $ make SANITIZE=leak GIT_TEST_PASSING_SANITIZE_LEAK=check GIT_TEST_SANITIZE_LEAK_LOG=true test
> 
> Rubén Justo (4):
>   t0080: mark as leak-free
>   t5332: mark as leak-free
>   t6113: mark as leak-free
>   test-lib: check for TEST_PASSES_SANITIZE_LEAK

These all looked reasonable to me. Thank you for not just fixing them,
but including the background for each case (e.g., leak-free as of commit
XYZ, etc).

-Peff

Re: [PATCH 0/4] mark tests as leak-free

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Mon, Jan 29, 2024 at 10:04:10PM +0100, Rubén Justo wrote:
>
>> The tests: t0080, t5332 and t6113 can be annotated as leak-free.
>> 
>> I used:
>>   $ make SANITIZE=leak GIT_TEST_PASSING_SANITIZE_LEAK=check GIT_TEST_SANITIZE_LEAK_LOG=true test
>> 
>> Rubén Justo (4):
>>   t0080: mark as leak-free
>>   t5332: mark as leak-free
>>   t6113: mark as leak-free
>>   test-lib: check for TEST_PASSES_SANITIZE_LEAK
>
> These all looked reasonable to me. Thank you for not just fixing them,
> but including the background for each case (e.g., leak-free as of commit
> XYZ, etc).

Yup, the background description was very useful to read.

Thanks.