On Mon, Jul 01, 2024 at 01:19:34PM -0700, Junio C Hamano wrote:
> Your patch from September 2023 [*] did mention it upfront:
>
> GIT_TEST_SANITIZE_LEAK_LOG=true with a test that leaks, will
> make the test return zero unintentionally.
>
> With that inserted in front of the proposed log message, the
> resulting explanation looks reasonable to me.
I see that you have already added this paragraph to what is already in
"seen". OK.
> > diff --git a/t/test-lib.sh b/t/test-lib.sh
> > index 79d3e0e7d9..7ed6d3fc47 100644
> > --- a/t/test-lib.sh
> > +++ b/t/test-lib.sh
> > @@ -1269,9 +1269,12 @@ check_test_results_san_file_ () {
> > then
> > say "As TEST_PASSES_SANITIZE_LEAK=true isn't set the above leak is 'ok' with GIT_TEST_PASSING_SANITIZE_LEAK=check" &&
> > invert_exit_code=t
> > - else
> > + elif test "$test_failure" = 0
> > + then
> > say "With GIT_TEST_SANITIZE_LEAK_LOG=true our logs revealed a memory leak, exit non-zero!" &&
> > invert_exit_code=t
> > + else
> > + say "With GIT_TEST_SANITIZE_LEAK_LOG=true our logs revealed a memory leak..."
> > fi
> > }
>
> This is outside the scope of this patch simply because it is
> inherited from the original, but does ", exit non-zero!" part of
> the message really add any value?
Explicitly indicating that the error is being forced due to
"GIT_TEST_SANITIZE_LEAK_LOG=true", for a test that doesn't fail when run
normally or even when run with just
"GIT_TEST_PASSING_SANITIZE_LEAK=yes", could save us some confusion.
So, I dunno.
Anyway, I agree that this can be addressed later.
Thanks.
