Re: Re* [PATCH v8 2/2] tests: add a test mode for SANITIZE=leak, run it in CI

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Wed, Nov 03 2021, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> The CI target uses a new GIT_TEST_PASSING_SANITIZE_LEAK=true test
>> mode. When running in that mode, we'll assert that we were compiled
>> with SANITIZE=leak. We'll then skip all tests, except those that we've
>> opted-in by setting "TEST_PASSES_SANITIZE_LEAK=true".
>> ...
>> This is how tests that don't set "TEST_PASSES_SANITIZE_LEAK=true" will
>> be skipped under GIT_TEST_PASSING_SANITIZE_LEAK=true:
>
> I've been playing with this locally, but cannot shake the nagging
> feeling that GIT_TEST_PASSING_SANITIZE_LEAK must default to true.
> Otherwise, it is one more thing they need to find out and set when
> they do
>
>     make SANITYZE=leak test
>
> because they want to be a good developer and to ensure that they did
> not introduce new leaks.
>
> If we want to encourage folks to locally run the leak checks before
> declaring their own work "done", that is.
>
> Those who are hunting for and cleaning up existing leaks can and
> should set it to false, no?

I agree that that would make a lot more sense and be more useful :)

That was the behavior of the patch I originally suggested for
integrating this SANITIZE=leak[1], but due to feedback on it I ended up
keeping the pre-image behavior of how SANITIZE=leak worked, unless there
were any opt-in test modes etc. in play:
https://lore.kernel.org/git/patch-1.4-a61a294132-20210714T001007Z-avarab@gmail.com/

I think at this point it's probably better to just keep it as it is...

> in any case, here is a small fallout out of my adventure into this
> corner.
>
> ----- >8 --------- >8 --------- >8 --------- >8 -----
> Subject: t0006: date_mode can leak .strftime_fmt member
>
> As there is no date_mode_release() API function, and given the
> set of current callers it probably is not worth adding one, let's
> release the .strftime_fmt member that is obtained from strdup()
> before the caller of show_date() is done with it.
>
> This allows us to mark t0006 as passing under the leak sanitizer.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  t/helper/test-date.c | 2 ++
>  t/t0006-date.sh      | 2 ++
>  2 files changed, 4 insertions(+)
>
> diff --git c/t/helper/test-date.c w/t/helper/test-date.c
> index 099eff4f0f..e15ea02626 100644
> --- c/t/helper/test-date.c
> +++ w/t/helper/test-date.c
> @@ -53,6 +53,8 @@ static void show_dates(const char **argv, const char *format)
>  
>  		printf("%s -> %s\n", *argv, show_date(t, tz, &mode));
>  	}
> +
> +	free((void *)mode.strftime_fmt);
>  }

I'd notice that failure before, but hadn't looked into it. That was
easier to fix than I thought.

This fix looks good to me, except that you also need to change this at
the top:

diff --git a/t/helper/test-date.c b/t/helper/test-date.c
index e15ea026267..9defeb57360 100644
--- a/t/helper/test-date.c
+++ b/t/helper/test-date.c
@@ -34,7 +34,7 @@ static void show_human_dates(const char **argv)
 
 static void show_dates(const char **argv, const char *format)
 {
-       struct date_mode mode;
+       struct date_mode mode = { 0 };
 
        parse_date_format(format, &mode);
        for (; *argv; argv++) {

I.e. this makes this specific thing pass, but in other tests we'd end up
freeing a non-NULL and randomly initialized pointer unless we init it to
zero.

>  
>  static void parse_dates(const char **argv)
> diff --git c/t/t0006-date.sh w/t/t0006-date.sh
> index 6b757d7169..5d01f57b27 100755
> --- c/t/t0006-date.sh
> +++ w/t/t0006-date.sh
> @@ -1,6 +1,8 @@
>  #!/bin/sh
>  
>  test_description='test date parsing and printing'
> +
> +TEST_PASSES_SANITIZE_LEAK=true
>  . ./test-lib.sh
>  
>  # arbitrary reference time: 2009-08-30 19:20:00

And yeah, that's all that's needed in the test file then.