Re: [PATCH] test-lib: fix GIT_TEST_SANITIZE_LEAK_LOG

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 1, 2024, 20:19 UTC
Message-ID: <xmqqv81ovp9l.fsf@gitster.g>
In-Reply-To: <f4ae6e2a-218a-419c-b6c4-59a08be247a0@gmail.com>

Rubén Justo <rjusto@gmail.com> writes:

> In the if-else's chain we have in "check_test_results_san_file_", we
> consider three variables: $passes_sanitize_leak, $sanitize_leak_check
> and, implicitly, GIT_TEST_SANITIZE_LEAK_LOG (always set to "true" at
> that point).

Before this paragraph, we'd probably want to say what problem we are fixing. Using the verb "fix" on the subject line without saying what broken behaviour you see around GIT_TEST_SANITIZE_LEAK_LOG does not help, either.

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.

Show 17 quoted lines

> 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? I am wondering if

	else
-		say "With GIT_TEST_SANITIZE_LEAK_LOG=true ..., exit non-zero!"
+		say "With GIT_TEST_SANITIZE_LEAK_LOG=true our logs revealed a leak."
+		test "$test_failure" != 0 || invert_exit_code=t
	fi

might be what we eventually want to end up with, after the dust settles from this fix.

Thanks.