Re: [PATCH 1/2] test-lib: prevent misuses of --invert-exit-code

From: Jeff King <peff@peff.net>
Date: Sep 12, 2023, 08:35 UTC
Message-ID: <20230912083528.GC1630538@coredump.intra.peff.net>
In-Reply-To: <1a60a1ca-0ef0-ecf5-d0aa-a28d7c148a82@gmail.com>

On Sun, Sep 10, 2023 at 01:08:11AM +0200, Rubén Justo wrote:

Show 10 quoted lines

> GIT_TEST_PASSING_SANITIZE_LEAK=true and GIT_TEST_SANITIZE_LEAK_LOG=true
> use internnlly the --invert-exit-code machinery.  Therefore if the user
> wants to use --invert-exit-code in combination with them, the result
> will be confusing.
> 
> For the same reason, we are already using BAIL_OUT if the user tries to
> combine GIT_TEST_PASSING_SANITIZE_LEAK=check with --invert-exit-code.
> 
> Let's do the same for GIT_TEST_PASSING_SANITIZE_LEAK=true and
> GIT_TEST_SANITIZE_LEAK_LOG=true.

OK, so we are trying to find a case where the user is triggering --invert-exit-code themselves and complaining. But in the code...

Show 19 quoted lines

> @@ -1557,15 +1557,25 @@ then
>  			say "in GIT_TEST_PASSING_SANITIZE_LEAK=check mode, setting --invert-exit-code for TEST_PASSES_SANITIZE_LEAK != true"
>  			invert_exit_code=t
>  		fi
> -	elif test -z "$passes_sanitize_leak" &&
> -	     test_bool_env GIT_TEST_PASSING_SANITIZE_LEAK false
> +	elif test_bool_env GIT_TEST_PASSING_SANITIZE_LEAK false
>  	then
> -		skip_all="skipping $this_test under GIT_TEST_PASSING_SANITIZE_LEAK=true"
> -		test_done
> +		if test -n "$invert_exit_code"
> +		then
> +			BAIL_OUT "cannot use --invert-exit-code under GIT_TEST_PASSING_SANITIZE_LEAK=true"
> +		elif test -z "$passes_sanitize_leak"
> +		then
> +			skip_all="skipping $this_test under GIT_TEST_PASSING_SANITIZE_LEAK=true"
> +			test_done
> +		fi
>  	fi

You can see at the top of the context that we will set invert_exit_code=t ourselves, which will then complain here:

Show 10 quoted lines

>  	if test_bool_env GIT_TEST_SANITIZE_LEAK_LOG false
>  	then
> +		if test -n "$invert_exit_code"
> +		then
> +			BAIL_OUT "cannot use --invert-exit-code and GIT_TEST_SANITIZE_LEAK_LOG=true"
> +		fi
> +
>  		if ! mkdir -p "$TEST_RESULTS_SAN_DIR"
>  		then
>  			BAIL_OUT "cannot create $TEST_RESULTS_SAN_DIR"

That varible-set in the earlier context is from running in "check" mode. So:

  make GIT_TEST_PASSING_SANITIZE_LEAK=check GIT_TEST_SANITIZE_LEAK_LOG=true

will now always fail. But this is the main way you'd want to run it (enabling the leak log catches more stuff, and the log-check function you touch in patch 2 already covers check mode).

So I think you'd have to hoist your check above the if/else for setting up PASSING_SANITIZE_LEAK modes.

-Peff