On Wed, Jul 03, 2024 at 11:35:48PM +0200, Rubén Justo wrote:
> 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.
Maybe what we should do is integrate "GIT_TEST_SANITIZE_LEAK_LOG" into
"GIT_TEST_PASSING_SANITIZE_LEAK" because I'm not sure what value we get
by keeping them separate (test performance?). But that's another topic,
even further out of scope of this patch :-)
