Re: [PATCH 2/2] test-lib: fix GIT_TEST_SANITIZE_LEAK_LOG

[Jeff King <peff@peff.net>]

On Fri, Sep 15, 2023 at 02:28:15AM +0200, Rubén Justo wrote:

> > And the problem is in (3). You switch it to trigger only if we have no
> > failures (fixing the inversion). But should we have the same a/b split
> > for this case? I.e.:
> > 
> >   3a. if we saw no test failures, invert to cause a failure
> >   3b. we saw other failures; do not invert, but _do_ mention that the
> >       log found extra leaks
> > 
> > In 3b we are explaining to the user what happened. Though maybe it is
> > not super important, because I think we'd have dumped the log contents
> > anyway?
> 
> I think so too.  At that point we've already dumped the contents of the
> $TEST_RESULTS_SAN_FILE file.
> 
> IMO, when $test_failure is zero (the "if" I'm touching), the message
> makes sense not so much to say that a leak has been found, but rather
> because we're forcing the non-zero exit.
> 
> But when $test_failure is not zero, after we've already dumped the
> log, maybe this is somewhat redundant:
> 
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 87cfea9e9a..b160ae3f7a 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -1267,6 +1267,8 @@ check_test_results_san_file_ () {
>         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
>  }
> 
> However, if you or anyone else thinks it adds value, I have no objection
> to re-roll with it.

I'm on the fence. It is probably not a big deal, and my biggest issue is
just that I had to walk through the explanation in my previous mail to
convince myself the change was not missing an important case.

But having done so, the main value in re-rolling would be preventing
somebody else from reading the code and having the same question. But
this discussion in the archive is probably sufficient.

-Peff