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
