[PATCH] test-lib: GIT_TEST_SANITIZE_LEAK_LOG enabled by default

[PATCH] test-lib: GIT_TEST_SANITIZE_LEAK_LOG enabled by default

[Rubén Justo]

As we describe in t/README, it can happen that:

     Some tests run "git" (or "test-tool" etc.) without properly checking
     the exit code, or git will invoke itself and fail to ferry the
     abort() exit code to the original caller.

Therefore, GIT_TEST_SANITIZE_LEAK_LOG must be set to true to capture all
memory leaks triggered by the tests when SANITIZE=leak.

Set it to true by default, and stop worrying about someone checking for
leaks who isn't aware of this option and might be missing some leaks.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
  ci/lib.sh     | 1 -
  t/README      | 4 ++--
  t/test-lib.sh | 2 +-
  3 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/ci/lib.sh b/ci/lib.sh
index ff66ad356b..fe52954828 100755
--- a/ci/lib.sh
+++ b/ci/lib.sh
@@ -374,7 +374,6 @@ linux-musl)
  linux-leaks|linux-reftable-leaks)
  	export SANITIZE=leak
  	export GIT_TEST_PASSING_SANITIZE_LEAK=true
-	export GIT_TEST_SANITIZE_LEAK_LOG=true
  	;;
  linux-asan-ubsan)
  	export SANITIZE=address,undefined
diff --git a/t/README b/t/README
index d9e0e07506..1c97bc3331 100644
--- a/t/README
+++ b/t/README
@@ -382,10 +382,10 @@ mapping between "TEST_PASSES_SANITIZE_LEAK=true" 
and those tests that
  pass under "SANITIZE=leak". This is especially useful when testing a
  series that fixes various memory leaks with "git rebase -x".

-GIT_TEST_SANITIZE_LEAK_LOG=true will log memory leaks to
+GIT_TEST_SANITIZE_LEAK_LOG=<boolean> controls logging of memory leaks to
  "test-results/$TEST_NAME.leak/trace.*" files. The logs include a
  "dedup_token" (see +"ASAN_OPTIONS=help=1 ./git") and other options to
-make logs +machine-readable.
+make logs +machine-readable.  Defaults to "true" when SANITIZE=leak.

  With GIT_TEST_SANITIZE_LEAK_LOG=true we'll look at the leak logs
  before exiting and exit on failure if the logs showed that we had a
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 7ed6d3fc47..1dd2ea4e07 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -1578,7 +1578,7 @@ then
  		test_done
  	fi

-	if test_bool_env GIT_TEST_SANITIZE_LEAK_LOG false
+	if test_bool_env GIT_TEST_SANITIZE_LEAK_LOG true
  	then
  		if ! mkdir -p "$TEST_RESULTS_SAN_DIR"
  		then
-- 
2.45.1

Re: [PATCH] test-lib: GIT_TEST_SANITIZE_LEAK_LOG enabled by default

[Jeff King]

On Wed, Jul 10, 2024 at 02:51:58AM +0200, Rubén Justo wrote:

> As we describe in t/README, it can happen that:
> 
>     Some tests run "git" (or "test-tool" etc.) without properly checking
>     the exit code, or git will invoke itself and fail to ferry the
>     abort() exit code to the original caller.
> 
> Therefore, GIT_TEST_SANITIZE_LEAK_LOG must be set to true to capture all
> memory leaks triggered by the tests when SANITIZE=leak.
> 
> Set it to true by default, and stop worrying about someone checking for
> leaks who isn't aware of this option and might be missing some leaks.

I'm obviously in favor of this direction, but...why stop here? Do we
expect somebody to set it to false? If not, then can't we just get rid
of it entirely?

-Peff

[PATCH v2] test-lib: GIT_TEST_SANITIZE_LEAK_LOG enabled by default

[Rubén Justo]

As we currently describe in t/README, it can happen that:

     Some tests run "git" (or "test-tool" etc.) without properly checking
     the exit code, or git will invoke itself and fail to ferry the
     abort() exit code to the original caller.

Therefore, GIT_TEST_SANITIZE_LEAK_LOG=true is needed to be set to
capture all memory leaks triggered by our tests.

It seems unnecessary to force users to remember this option, as
forgetting it could lead to missed memory leaks.

We could solve the problem by setting GIT_TEST_SANITIZE_LEAK_LOG to
"true" by default, but that might suggest we think "false" makes sense,
which isn't the case.

Therefore, the best approach is to remove the option entirely while
maintaining the capability to detect memory leaks in blind spots of our
tests.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---

@Peff: I thought your preference was to keep the option.  I agree to
remove it completely.  This, v2, address that.

I'm traveling, and while I think the change doesn't break anything, I'd
appreciate a double check ;-)

By the way, I used, more or less, the text for the message that Junio 
suggested.

  ci/lib.sh     |  1 -
  t/README      | 24 ------------------------
  t/test-lib.sh | 41 +++++++++++++++++++----------------------
  3 files changed, 19 insertions(+), 47 deletions(-)

diff --git a/ci/lib.sh b/ci/lib.sh
index 814578ffc6..51f8f59a29 100755
--- a/ci/lib.sh
+++ b/ci/lib.sh
@@ -370,7 +370,6 @@ linux-musl)
  linux-leaks|linux-reftable-leaks)
  	export SANITIZE=leak
  	export GIT_TEST_PASSING_SANITIZE_LEAK=true
-	export GIT_TEST_SANITIZE_LEAK_LOG=true
  	;;
  linux-asan-ubsan)
  	export SANITIZE=address,undefined
diff --git a/t/README b/t/README
index d9e0e07506..c2a732d59e 100644
--- a/t/README
+++ b/t/README
@@ -382,33 +382,9 @@ mapping between "TEST_PASSES_SANITIZE_LEAK=true" 
and those tests that
  pass under "SANITIZE=leak". This is especially useful when testing a
  series that fixes various memory leaks with "git rebase -x".

-GIT_TEST_SANITIZE_LEAK_LOG=true will log memory leaks to
-"test-results/$TEST_NAME.leak/trace.*" files. The logs include a
-"dedup_token" (see +"ASAN_OPTIONS=help=1 ./git") and other options to
-make logs +machine-readable.
-
-With GIT_TEST_SANITIZE_LEAK_LOG=true we'll look at the leak logs
-before exiting and exit on failure if the logs showed that we had a
-memory leak, even if the test itself would have otherwise passed. This
-allows us to catch e.g. missing &&-chaining. This is especially useful
-when combined with "GIT_TEST_PASSING_SANITIZE_LEAK", see below.
-
  GIT_TEST_PASSING_SANITIZE_LEAK=check when combined with "--immediate"
  will run to completion faster, and result in the same failing
  tests. The only practical reason to run
-GIT_TEST_PASSING_SANITIZE_LEAK=check without "--immediate" is to
-combine it with "GIT_TEST_SANITIZE_LEAK_LOG=true". If we stop at the
-first failing test case our leak logs won't show subsequent leaks we
-might have run into.
-
-GIT_TEST_PASSING_SANITIZE_LEAK=(true|check) will not catch all memory
-leaks unless combined with GIT_TEST_SANITIZE_LEAK_LOG=true. Some tests
-run "git" (or "test-tool" etc.) without properly checking the exit
-code, or git will invoke itself and fail to ferry the abort() exit
-code to the original caller. When the two modes are combined we'll
-look at the "test-results/$TEST_NAME.leak/trace.*" files at the end of
-the test run to see if had memory leaks which the test itself didn't
-catch.

  GIT_TEST_PROTOCOL_VERSION=<n>, when set, makes 'protocol.version'
  default to n.
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 79d3e0e7d9..942828c55d 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -1270,8 +1270,8 @@ check_test_results_san_file_ () {
  		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
-		say "With GIT_TEST_SANITIZE_LEAK_LOG=true our logs revealed a memory 
leak, exit non-zero!" &&
-		invert_exit_code=t
+		say "Our logs revealed a leak!" &&
+		test "$test_failure" != 0 || invert_exit_code=t
  	fi
  }

@@ -1555,28 +1555,28 @@ then
  		passes_sanitize_leak=t
  	fi

-	if test "$GIT_TEST_PASSING_SANITIZE_LEAK" = "check"
+	if test -z "$passes_sanitize_leak" &&
+	   ! test "$GIT_TEST_PASSING_SANITIZE_LEAK" = "check" &&
+	   test_bool_env GIT_TEST_PASSING_SANITIZE_LEAK false
  	then
-		sanitize_leak_check=t
-		if test -n "$invert_exit_code"
+		skip_all="skipping $this_test under GIT_TEST_PASSING_SANITIZE_LEAK=true"
+		test_done
+	else
+		if test "$GIT_TEST_PASSING_SANITIZE_LEAK" = "check"
  		then
-			BAIL_OUT "cannot use --invert-exit-code under 
GIT_TEST_PASSING_SANITIZE_LEAK=check"
-		fi
+			sanitize_leak_check=t
+			if test -n "$invert_exit_code"
+			then
+				BAIL_OUT "cannot use --invert-exit-code under 
GIT_TEST_PASSING_SANITIZE_LEAK=check"
+			fi

-		if test -z "$passes_sanitize_leak"
-		then
-			say "in GIT_TEST_PASSING_SANITIZE_LEAK=check mode, setting 
--invert-exit-code for TEST_PASSES_SANITIZE_LEAK != true"
-			invert_exit_code=t
+			if test -z "$passes_sanitize_leak"
+			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
  		fi
-	elif test -z "$passes_sanitize_leak" &&
-	     test_bool_env GIT_TEST_PASSING_SANITIZE_LEAK false
-	then
-		skip_all="skipping $this_test under GIT_TEST_PASSING_SANITIZE_LEAK=true"
-		test_done
-	fi

-	if test_bool_env GIT_TEST_SANITIZE_LEAK_LOG false
-	then
  		if ! mkdir -p "$TEST_RESULTS_SAN_DIR"
  		then
  			BAIL_OUT "cannot create $TEST_RESULTS_SAN_DIR"
@@ -1599,9 +1599,6 @@ elif test "$GIT_TEST_PASSING_SANITIZE_LEAK" = 
"check" ||
       test_bool_env GIT_TEST_PASSING_SANITIZE_LEAK false
  then
  	BAIL_OUT_ENV_NEEDS_SANITIZE_LEAK "GIT_TEST_PASSING_SANITIZE_LEAK=true"
-elif test_bool_env GIT_TEST_SANITIZE_LEAK_LOG false
-then
-	BAIL_OUT_ENV_NEEDS_SANITIZE_LEAK "GIT_TEST_SANITIZE_LEAK_LOG=true"
  fi

  if test "${GIT_TEST_CHAIN_LINT:-1}" != 0 &&
-- 
2.45.1

Re: [PATCH v2] test-lib: GIT_TEST_SANITIZE_LEAK_LOG enabled by default

[Junio C Hamano]

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

> As we currently describe in t/README, it can happen that:
>
>     Some tests run "git" (or "test-tool" etc.) without properly checking
>     the exit code, or git will invoke itself and fail to ferry the
>     abort() exit code to the original caller.
>
> Therefore, GIT_TEST_SANITIZE_LEAK_LOG=true is needed to be set to
> capture all memory leaks triggered by our tests.
>
> It seems unnecessary to force users to remember this option, as
> forgetting it could lead to missed memory leaks.
>
> We could solve the problem by setting GIT_TEST_SANITIZE_LEAK_LOG to
> "true" by default, but that might suggest we think "false" makes sense,
> which isn't the case.
>
> Therefore, the best approach is to remove the option entirely while
> maintaining the capability to detect memory leaks in blind spots of our
> tests.
>
> Signed-off-by: Rubén Justo <rjusto@gmail.com>
> ---
>
> @Peff: I thought your preference was to keep the option.  I agree to
> remove it completely.  This, v2, address that.
>
> I'm traveling, and while I think the change doesn't break anything, I'd
> appreciate a double check ;-)
>
> By the way, I used, more or less, the text for the message that Junio
> suggested.

Hmph, the above doesn't look like any, but as a standalone patch it
is very much readable.

>  ci/lib.sh     |  1 -
>  t/README      | 24 ------------------------
>  t/test-lib.sh | 41 +++++++++++++++++++----------------------
>  3 files changed, 19 insertions(+), 47 deletions(-)

I won't be able to apply this to my tree, with

    warning: Patch sent with format=flowed; space at the end of lines might be lost.
    error: corrupt patch at line 22

until these get fixed, but is this meant to apply on top of 47c6d4da
(test-lib: fix GIT_TEST_SANITIZE_LEAK_LOG, 2024-06-30) which is
cooking in 'next'?


Thanks.


> diff --git a/ci/lib.sh b/ci/lib.sh
> index 814578ffc6..51f8f59a29 100755
> --- a/ci/lib.sh
> +++ b/ci/lib.sh
> @@ -370,7 +370,6 @@ linux-musl)
>  linux-leaks|linux-reftable-leaks)
>  	export SANITIZE=leak
>  	export GIT_TEST_PASSING_SANITIZE_LEAK=true
> -	export GIT_TEST_SANITIZE_LEAK_LOG=true
>  	;;
>  linux-asan-ubsan)
>  	export SANITIZE=address,undefined
> diff --git a/t/README b/t/README
> index d9e0e07506..c2a732d59e 100644
> --- a/t/README
> +++ b/t/README
> @@ -382,33 +382,9 @@ mapping between "TEST_PASSES_SANITIZE_LEAK=true"
> and those tests that
>  pass under "SANITIZE=leak". This is especially useful when testing a
>  series that fixes various memory leaks with "git rebase -x".
>
> -GIT_TEST_SANITIZE_LEAK_LOG=true will log memory leaks to
> -"test-results/$TEST_NAME.leak/trace.*" files. The logs include a
> -"dedup_token" (see +"ASAN_OPTIONS=help=1 ./git") and other options to
> -make logs +machine-readable.
> -
> -With GIT_TEST_SANITIZE_LEAK_LOG=true we'll look at the leak logs
> -before exiting and exit on failure if the logs showed that we had a
> -memory leak, even if the test itself would have otherwise passed. This
> -allows us to catch e.g. missing &&-chaining. This is especially useful
> -when combined with "GIT_TEST_PASSING_SANITIZE_LEAK", see below.
> -
>  GIT_TEST_PASSING_SANITIZE_LEAK=check when combined with "--immediate"
>  will run to completion faster, and result in the same failing
>  tests. The only practical reason to run
> -GIT_TEST_PASSING_SANITIZE_LEAK=check without "--immediate" is to
> -combine it with "GIT_TEST_SANITIZE_LEAK_LOG=true". If we stop at the
> -first failing test case our leak logs won't show subsequent leaks we
> -might have run into.
> -
> -GIT_TEST_PASSING_SANITIZE_LEAK=(true|check) will not catch all memory
> -leaks unless combined with GIT_TEST_SANITIZE_LEAK_LOG=true. Some tests
> -run "git" (or "test-tool" etc.) without properly checking the exit
> -code, or git will invoke itself and fail to ferry the abort() exit
> -code to the original caller. When the two modes are combined we'll
> -look at the "test-results/$TEST_NAME.leak/trace.*" files at the end of
> -the test run to see if had memory leaks which the test itself didn't
> -catch.
>
>  GIT_TEST_PROTOCOL_VERSION=<n>, when set, makes 'protocol.version'
>  default to n.
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 79d3e0e7d9..942828c55d 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -1270,8 +1270,8 @@ check_test_results_san_file_ () {
>  		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
> -		say "With GIT_TEST_SANITIZE_LEAK_LOG=true our logs
>                  revealed a memory leak, exit non-zero!" &&
> -		invert_exit_code=t
> +		say "Our logs revealed a leak!" &&
> +		test "$test_failure" != 0 || invert_exit_code=t
>  	fi
>  }
>
> @@ -1555,28 +1555,28 @@ then
>  		passes_sanitize_leak=t
>  	fi
>
> -	if test "$GIT_TEST_PASSING_SANITIZE_LEAK" = "check"
> +	if test -z "$passes_sanitize_leak" &&
> +	   ! test "$GIT_TEST_PASSING_SANITIZE_LEAK" = "check" &&
> +	   test_bool_env GIT_TEST_PASSING_SANITIZE_LEAK false
>  	then
> -		sanitize_leak_check=t
> -		if test -n "$invert_exit_code"
> +		skip_all="skipping $this_test under GIT_TEST_PASSING_SANITIZE_LEAK=true"
> +		test_done
> +	else
> +		if test "$GIT_TEST_PASSING_SANITIZE_LEAK" = "check"
>  		then
> -			BAIL_OUT "cannot use --invert-exit-code under
>                          GIT_TEST_PASSING_SANITIZE_LEAK=check"
> -		fi
> +			sanitize_leak_check=t
> +			if test -n "$invert_exit_code"
> +			then
> +				BAIL_OUT "cannot use
> --invert-exit-code under GIT_TEST_PASSING_SANITIZE_LEAK=check"
> +			fi
>
> -		if test -z "$passes_sanitize_leak"
> -		then
> -			say "in GIT_TEST_PASSING_SANITIZE_LEAK=check
>                          mode, setting --invert-exit-code for
>                         TEST_PASSES_SANITIZE_LEAK != true"
> -			invert_exit_code=t
> +			if test -z "$passes_sanitize_leak"
> +			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
>  		fi
> -	elif test -z "$passes_sanitize_leak" &&
> -	     test_bool_env GIT_TEST_PASSING_SANITIZE_LEAK false
> -	then
> -		skip_all="skipping $this_test under GIT_TEST_PASSING_SANITIZE_LEAK=true"
> -		test_done
> -	fi
>
> -	if test_bool_env GIT_TEST_SANITIZE_LEAK_LOG false
> -	then
>  		if ! mkdir -p "$TEST_RESULTS_SAN_DIR"
>  		then
>  			BAIL_OUT "cannot create $TEST_RESULTS_SAN_DIR"
> @@ -1599,9 +1599,6 @@ elif test "$GIT_TEST_PASSING_SANITIZE_LEAK" =
> "check" ||
>       test_bool_env GIT_TEST_PASSING_SANITIZE_LEAK false
>  then
>  	BAIL_OUT_ENV_NEEDS_SANITIZE_LEAK "GIT_TEST_PASSING_SANITIZE_LEAK=true"
> -elif test_bool_env GIT_TEST_SANITIZE_LEAK_LOG false
> -then
> -	BAIL_OUT_ENV_NEEDS_SANITIZE_LEAK "GIT_TEST_SANITIZE_LEAK_LOG=true"
>  fi
>
>  if test "${GIT_TEST_CHAIN_LINT:-1}" != 0 &&

Re: [PATCH v2] test-lib: GIT_TEST_SANITIZE_LEAK_LOG enabled by default

[Rubén Justo]

On Tue, Jul 09, 2024 at 08:58:56PM -0700, Junio C Hamano wrote:

 > > I used, more or less, the text for the message that Junio
 > > suggested.
 >
 > Hmph, the above doesn't look like any, but as a standalone patch it
 > is very much readable.

Ok.  I'll apply the simplification as a separate patch and then remove
the reference to GIT_TEST_SANITIZE_LEAK_LOG.

 > I won't be able to apply this to my tree, with
 >
 >     warning: Patch sent with format=flowed; space at the end of lines 
might be lost.
 >     error: corrupt patch at line 22
 >
 > until these get fixed,

Ouch.  My email client is set up incorrectly.  I'll fix it.
Sorry.

 > but is this meant to apply on top of 47c6d4da
 > (test-lib: fix GIT_TEST_SANITIZE_LEAK_LOG, 2024-06-30) which is
 > cooking in 'next'?

It is.  I'll use "git format-patch --base=47c6d4da" to reroll.

However, will wait a bit for confirmation that this patch is heading
in the expected direction.

Thanks.

Re: [PATCH v2] test-lib: GIT_TEST_SANITIZE_LEAK_LOG enabled by default

[Jeff King]

On Wed, Jul 10, 2024 at 11:30:13AM +0900, Rubén Justo wrote:

> As we currently describe in t/README, it can happen that:
> 
>     Some tests run "git" (or "test-tool" etc.) without properly checking
>     the exit code, or git will invoke itself and fail to ferry the
>     abort() exit code to the original caller.
> 
> Therefore, GIT_TEST_SANITIZE_LEAK_LOG=true is needed to be set to
> capture all memory leaks triggered by our tests.
> 
> It seems unnecessary to force users to remember this option, as
> forgetting it could lead to missed memory leaks.
> 
> We could solve the problem by setting GIT_TEST_SANITIZE_LEAK_LOG to
> "true" by default, but that might suggest we think "false" makes sense,
> which isn't the case.
> 
> Therefore, the best approach is to remove the option entirely while
> maintaining the capability to detect memory leaks in blind spots of our
> tests.

Yeah, I think that reasoning makes sense.

> diff --git a/ci/lib.sh b/ci/lib.sh
> index 814578ffc6..51f8f59a29 100755
> --- a/ci/lib.sh
> +++ b/ci/lib.sh
> @@ -370,7 +370,6 @@ linux-musl)
>  linux-leaks|linux-reftable-leaks)
>  	export SANITIZE=leak
>  	export GIT_TEST_PASSING_SANITIZE_LEAK=true
> -	export GIT_TEST_SANITIZE_LEAK_LOG=true
>  	;;

OK, we can drop this line snice it's now the default. Good.

> diff --git a/t/README b/t/README
> index d9e0e07506..c2a732d59e 100644
> --- a/t/README
> +++ b/t/README
> @@ -382,33 +382,9 @@ mapping between "TEST_PASSES_SANITIZE_LEAK=true" and
> those tests that
>  pass under "SANITIZE=leak". This is especially useful when testing a
>  series that fixes various memory leaks with "git rebase -x".
> 
> -GIT_TEST_SANITIZE_LEAK_LOG=true will log memory leaks to
> -"test-results/$TEST_NAME.leak/trace.*" files. The logs include a
> -"dedup_token" (see +"ASAN_OPTIONS=help=1 ./git") and other options to
> -make logs +machine-readable.
> -
> -With GIT_TEST_SANITIZE_LEAK_LOG=true we'll look at the leak logs
> -before exiting and exit on failure if the logs showed that we had a
> -memory leak, even if the test itself would have otherwise passed. This
> -allows us to catch e.g. missing &&-chaining. This is especially useful
> -when combined with "GIT_TEST_PASSING_SANITIZE_LEAK", see below.
> -
>  GIT_TEST_PASSING_SANITIZE_LEAK=check when combined with "--immediate"
>  will run to completion faster, and result in the same failing
>  tests. The only practical reason to run
> -GIT_TEST_PASSING_SANITIZE_LEAK=check without "--immediate" is to
> -combine it with "GIT_TEST_SANITIZE_LEAK_LOG=true". If we stop at the
> -first failing test case our leak logs won't show subsequent leaks we
> -might have run into.
> -
> -GIT_TEST_PASSING_SANITIZE_LEAK=(true|check) will not catch all memory
> -leaks unless combined with GIT_TEST_SANITIZE_LEAK_LOG=true. Some tests
> -run "git" (or "test-tool" etc.) without properly checking the exit
> -code, or git will invoke itself and fail to ferry the abort() exit
> -code to the original caller. When the two modes are combined we'll
> -look at the "test-results/$TEST_NAME.leak/trace.*" files at the end of
> -the test run to see if had memory leaks which the test itself didn't
> -catch.

After this patch, the documentation seems to end abruptly with "The only
practical reason to run". I think we need to either delete those lines,
too, or complete the thought.

I do think they are saying something useful, which is: in "check" mode,
you should always use "--immediate" since the point is just to find
scripts which aren't labeled correctly. But I think that is true whether
you are using the leak log or not. Your log will be incomplete, of
course, if you used "--immediate", but the point is to see whether we
find even one.

>  GIT_TEST_PROTOCOL_VERSION=<n>, when set, makes 'protocol.version'
>  default to n.
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 79d3e0e7d9..942828c55d 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -1270,8 +1270,8 @@ check_test_results_san_file_ () {
>  		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
> -		say "With GIT_TEST_SANITIZE_LEAK_LOG=true our logs revealed a memory
> leak, exit non-zero!" &&
> -		invert_exit_code=t
> +		say "Our logs revealed a leak!" &&
> +		test "$test_failure" != 0 || invert_exit_code=t
>  	fi
>  }

This adds back in the test_failure fix from 47c6d4dad2 (test-lib: fix
GIT_TEST_SANITIZE_LEAK_LOG, 2024-06-30), but in a different way. I think
we'd want to build on top, and then you just need to update the messages
on either side of that final elif/else.

> @@ -1555,28 +1555,28 @@ then
>  		passes_sanitize_leak=t
>  	fi
> 
> -	if test "$GIT_TEST_PASSING_SANITIZE_LEAK" = "check"
> +	if test -z "$passes_sanitize_leak" &&
> +	   ! test "$GIT_TEST_PASSING_SANITIZE_LEAK" = "check" &&
> +	   test_bool_env GIT_TEST_PASSING_SANITIZE_LEAK false
>  	then
> -		sanitize_leak_check=t
> -		if test -n "$invert_exit_code"
> +		skip_all="skipping $this_test under GIT_TEST_PASSING_SANITIZE_LEAK=true"
> +		test_done
> +	else
> +		if test "$GIT_TEST_PASSING_SANITIZE_LEAK" = "check"
>  		then
> [...]

I'm not sure why we need to touch this block. The "if
GIT_TEST_SANITIZE_LEAK_LOG" just below it I assumed would go away. But
all of this has to do with "check" versus "true", etc? There might be
new refactoring / simplification opportunities opened up by getting rid
of the LEAK_LOG variable, but we should do those on top.

I guess what's happening is that you've rearranged it so that:

> -	if test_bool_env GIT_TEST_SANITIZE_LEAK_LOG false
> -	then
>  		if ! mkdir -p "$TEST_RESULTS_SAN_DIR"
>  		then
>  			BAIL_OUT "cannot create $TEST_RESULTS_SAN_DIR"

...when this conditional goes away, the existing body is still in the
"else". But even though it would make the diff noisy to reindent, I
think we are better off doing so to make it clear what the actual change
is.

> @@ -1599,9 +1599,6 @@ elif test "$GIT_TEST_PASSING_SANITIZE_LEAK" = "check"
> ||
>       test_bool_env GIT_TEST_PASSING_SANITIZE_LEAK false
>  then
>  	BAIL_OUT_ENV_NEEDS_SANITIZE_LEAK "GIT_TEST_PASSING_SANITIZE_LEAK=true"
> -elif test_bool_env GIT_TEST_SANITIZE_LEAK_LOG false
> -then
> -	BAIL_OUT_ENV_NEEDS_SANITIZE_LEAK "GIT_TEST_SANITIZE_LEAK_LOG=true"
>  fi

OK, this final elif is responsible for complaining when you set LEAK_LOG
but don't have an actual leak-checking build. But once it goes away,
there's no need to complain. Makes sense.

-Peff

Re: [PATCH v2] test-lib: GIT_TEST_SANITIZE_LEAK_LOG enabled by default

[Rubén Justo]

On Wed, Jul 10, 2024 at 03:16:21AM -0400, Jeff King wrote:
> On Wed, Jul 10, 2024 at 11:30:13AM +0900, Rubén Justo wrote:
> 
> > As we currently describe in t/README, it can happen that:
> > 
> >     Some tests run "git" (or "test-tool" etc.) without properly checking
> >     the exit code, or git will invoke itself and fail to ferry the
> >     abort() exit code to the original caller.
> > 
> > Therefore, GIT_TEST_SANITIZE_LEAK_LOG=true is needed to be set to
> > capture all memory leaks triggered by our tests.
> > 
> > It seems unnecessary to force users to remember this option, as
> > forgetting it could lead to missed memory leaks.
> > 
> > We could solve the problem by setting GIT_TEST_SANITIZE_LEAK_LOG to
> > "true" by default, but that might suggest we think "false" makes sense,
> > which isn't the case.
> > 
> > Therefore, the best approach is to remove the option entirely while
> > maintaining the capability to detect memory leaks in blind spots of our
> > tests.
> 
> Yeah, I think that reasoning makes sense.
> 
> > diff --git a/ci/lib.sh b/ci/lib.sh
> > index 814578ffc6..51f8f59a29 100755
> > --- a/ci/lib.sh
> > +++ b/ci/lib.sh
> > @@ -370,7 +370,6 @@ linux-musl)
> >  linux-leaks|linux-reftable-leaks)
> >  	export SANITIZE=leak
> >  	export GIT_TEST_PASSING_SANITIZE_LEAK=true
> > -	export GIT_TEST_SANITIZE_LEAK_LOG=true
> >  	;;
> 
> OK, we can drop this line snice it's now the default. Good.
> 
> > diff --git a/t/README b/t/README
> > index d9e0e07506..c2a732d59e 100644
> > --- a/t/README
> > +++ b/t/README
> > @@ -382,33 +382,9 @@ mapping between "TEST_PASSES_SANITIZE_LEAK=true" and
> > those tests that
> >  pass under "SANITIZE=leak". This is especially useful when testing a
> >  series that fixes various memory leaks with "git rebase -x".
> > 
> > -GIT_TEST_SANITIZE_LEAK_LOG=true will log memory leaks to
> > -"test-results/$TEST_NAME.leak/trace.*" files. The logs include a
> > -"dedup_token" (see +"ASAN_OPTIONS=help=1 ./git") and other options to
> > -make logs +machine-readable.
> > -
> > -With GIT_TEST_SANITIZE_LEAK_LOG=true we'll look at the leak logs
> > -before exiting and exit on failure if the logs showed that we had a
> > -memory leak, even if the test itself would have otherwise passed. This
> > -allows us to catch e.g. missing &&-chaining. This is especially useful
> > -when combined with "GIT_TEST_PASSING_SANITIZE_LEAK", see below.
> > -
> >  GIT_TEST_PASSING_SANITIZE_LEAK=check when combined with "--immediate"
> >  will run to completion faster, and result in the same failing
> >  tests. The only practical reason to run
> > -GIT_TEST_PASSING_SANITIZE_LEAK=check without "--immediate" is to
> > -combine it with "GIT_TEST_SANITIZE_LEAK_LOG=true". If we stop at the
> > -first failing test case our leak logs won't show subsequent leaks we
> > -might have run into.
> > -
> > -GIT_TEST_PASSING_SANITIZE_LEAK=(true|check) will not catch all memory
> > -leaks unless combined with GIT_TEST_SANITIZE_LEAK_LOG=true. Some tests
> > -run "git" (or "test-tool" etc.) without properly checking the exit
> > -code, or git will invoke itself and fail to ferry the abort() exit
> > -code to the original caller. When the two modes are combined we'll
> > -look at the "test-results/$TEST_NAME.leak/trace.*" files at the end of
> > -the test run to see if had memory leaks which the test itself didn't
> > -catch.
> 
> After this patch, the documentation seems to end abruptly with "The only
> practical reason to run". I think we need to either delete those lines,
> too, or complete the thought.

I accidentally left that line unfinished.  I'll fix it. 

> 
> I do think they are saying something useful, which is: in "check" mode,
> you should always use "--immediate" since the point is just to find
> scripts which aren't labeled correctly. But I think that is true whether
> you are using the leak log or not. Your log will be incomplete, of
> course, if you used "--immediate", but the point is to see whether we
> find even one.
> 
> >  GIT_TEST_PROTOCOL_VERSION=<n>, when set, makes 'protocol.version'
> >  default to n.
> > diff --git a/t/test-lib.sh b/t/test-lib.sh
> > index 79d3e0e7d9..942828c55d 100644
> > --- a/t/test-lib.sh
> > +++ b/t/test-lib.sh
> > @@ -1270,8 +1270,8 @@ check_test_results_san_file_ () {
> >  		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
> > -		say "With GIT_TEST_SANITIZE_LEAK_LOG=true our logs revealed a memory
> > leak, exit non-zero!" &&
> > -		invert_exit_code=t
> > +		say "Our logs revealed a leak!" &&
> > +		test "$test_failure" != 0 || invert_exit_code=t
> >  	fi
> >  }
> 
> This adds back in the test_failure fix from 47c6d4dad2 (test-lib: fix
> GIT_TEST_SANITIZE_LEAK_LOG, 2024-06-30), but in a different way. I think
> we'd want to build on top, and then you just need to update the messages
> on either side of that final elif/else.

OK.  I think simplifying those lines introduced unnecessary noise.  I'll
discard it and just adjust the messages. 

> 
> > @@ -1555,28 +1555,28 @@ then
> >  		passes_sanitize_leak=t
> >  	fi
> > 
> > -	if test "$GIT_TEST_PASSING_SANITIZE_LEAK" = "check"
> > +	if test -z "$passes_sanitize_leak" &&
> > +	   ! test "$GIT_TEST_PASSING_SANITIZE_LEAK" = "check" &&
> > +	   test_bool_env GIT_TEST_PASSING_SANITIZE_LEAK false
> >  	then
> > -		sanitize_leak_check=t
> > -		if test -n "$invert_exit_code"
> > +		skip_all="skipping $this_test under GIT_TEST_PASSING_SANITIZE_LEAK=true"
> > +		test_done
> > +	else
> > +		if test "$GIT_TEST_PASSING_SANITIZE_LEAK" = "check"
> >  		then
> > [...]
> 
> I'm not sure why we need to touch this block. The "if
> GIT_TEST_SANITIZE_LEAK_LOG" just below it I assumed would go away. But
> all of this has to do with "check" versus "true", etc? There might be
> new refactoring / simplification opportunities opened up by getting rid
> of the LEAK_LOG variable, but we should do those on top.
> 
> I guess what's happening is that you've rearranged it so that:
> 
> > -	if test_bool_env GIT_TEST_SANITIZE_LEAK_LOG false
> > -	then
> >  		if ! mkdir -p "$TEST_RESULTS_SAN_DIR"
> >  		then
> >  			BAIL_OUT "cannot create $TEST_RESULTS_SAN_DIR"
> 
> ...when this conditional goes away, the existing body is still in the
> "else". But even though it would make the diff noisy to reindent, I
> think we are better off doing so to make it clear what the actual change
> is.

OK.

> 
> > @@ -1599,9 +1599,6 @@ elif test "$GIT_TEST_PASSING_SANITIZE_LEAK" = "check"
> > ||
> >       test_bool_env GIT_TEST_PASSING_SANITIZE_LEAK false
> >  then
> >  	BAIL_OUT_ENV_NEEDS_SANITIZE_LEAK "GIT_TEST_PASSING_SANITIZE_LEAK=true"
> > -elif test_bool_env GIT_TEST_SANITIZE_LEAK_LOG false
> > -then
> > -	BAIL_OUT_ENV_NEEDS_SANITIZE_LEAK "GIT_TEST_SANITIZE_LEAK_LOG=true"
> >  fi
> 
> OK, this final elif is responsible for complaining when you set LEAK_LOG
> but don't have an actual leak-checking build. But once it goes away,
> there's no need to complain. Makes sense.
> 

Thanks for reviewing the patch.

[PATCH v3] test-lib: GIT_TEST_SANITIZE_LEAK_LOG enabled by default

[Rubén Justo]

As we currently describe in t/README, it can happen that:

    Some tests run "git" (or "test-tool" etc.) without properly checking
    the exit code, or git will invoke itself and fail to ferry the
    abort() exit code to the original caller.

Therefore, GIT_TEST_SANITIZE_LEAK_LOG=true is needed to be set to
capture all memory leaks triggered by our tests.

It seems unnecessary to force users to remember this option, as
forgetting it could lead to missed memory leaks.

We could solve the problem by making it "true" by default, but that
might suggest we think "false" makes sense, which isn't the case.

Therefore, the best approach is to remove the option entirely while
maintaining the capability to detect memory leaks in blind spots of our
tests.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---

This iteration mainly reduces the noise introduced in the previous
iteration. 

 ci/lib.sh     |  1 -
 t/README      | 26 +-------------------------
 t/test-lib.sh | 37 ++++++++++++++++---------------------
 3 files changed, 17 insertions(+), 47 deletions(-)

diff --git a/ci/lib.sh b/ci/lib.sh
index ff66ad356b..fe52954828 100755
--- a/ci/lib.sh
+++ b/ci/lib.sh
@@ -374,7 +374,6 @@ linux-musl)
 linux-leaks|linux-reftable-leaks)
 	export SANITIZE=leak
 	export GIT_TEST_PASSING_SANITIZE_LEAK=true
-	export GIT_TEST_SANITIZE_LEAK_LOG=true
 	;;
 linux-asan-ubsan)
 	export SANITIZE=address,undefined
diff --git a/t/README b/t/README
index d9e0e07506..ea620de17e 100644
--- a/t/README
+++ b/t/README
@@ -382,33 +382,9 @@ mapping between "TEST_PASSES_SANITIZE_LEAK=true" and those tests that
 pass under "SANITIZE=leak". This is especially useful when testing a
 series that fixes various memory leaks with "git rebase -x".
 
-GIT_TEST_SANITIZE_LEAK_LOG=true will log memory leaks to
-"test-results/$TEST_NAME.leak/trace.*" files. The logs include a
-"dedup_token" (see +"ASAN_OPTIONS=help=1 ./git") and other options to
-make logs +machine-readable.
-
-With GIT_TEST_SANITIZE_LEAK_LOG=true we'll look at the leak logs
-before exiting and exit on failure if the logs showed that we had a
-memory leak, even if the test itself would have otherwise passed. This
-allows us to catch e.g. missing &&-chaining. This is especially useful
-when combined with "GIT_TEST_PASSING_SANITIZE_LEAK", see below.
-
 GIT_TEST_PASSING_SANITIZE_LEAK=check when combined with "--immediate"
 will run to completion faster, and result in the same failing
-tests. The only practical reason to run
-GIT_TEST_PASSING_SANITIZE_LEAK=check without "--immediate" is to
-combine it with "GIT_TEST_SANITIZE_LEAK_LOG=true". If we stop at the
-first failing test case our leak logs won't show subsequent leaks we
-might have run into.
-
-GIT_TEST_PASSING_SANITIZE_LEAK=(true|check) will not catch all memory
-leaks unless combined with GIT_TEST_SANITIZE_LEAK_LOG=true. Some tests
-run "git" (or "test-tool" etc.) without properly checking the exit
-code, or git will invoke itself and fail to ferry the abort() exit
-code to the original caller. When the two modes are combined we'll
-look at the "test-results/$TEST_NAME.leak/trace.*" files at the end of
-the test run to see if had memory leaks which the test itself didn't
-catch.
+tests.
 
 GIT_TEST_PROTOCOL_VERSION=<n>, when set, makes 'protocol.version'
 default to n.
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 7ed6d3fc47..54247604cb 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -1271,10 +1271,10 @@ check_test_results_san_file_ () {
 		invert_exit_code=t
 	elif test "$test_failure" = 0
 	then
-		say "With GIT_TEST_SANITIZE_LEAK_LOG=true our logs revealed a memory leak, exit non-zero!" &&
+		say "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..."
+		say "Our logs revealed a memory leak..."
 	fi
 }
 
@@ -1578,33 +1578,28 @@ then
 		test_done
 	fi
 
-	if test_bool_env GIT_TEST_SANITIZE_LEAK_LOG false
+	if ! mkdir -p "$TEST_RESULTS_SAN_DIR"
 	then
-		if ! mkdir -p "$TEST_RESULTS_SAN_DIR"
-		then
-			BAIL_OUT "cannot create $TEST_RESULTS_SAN_DIR"
-		fi &&
-		TEST_RESULTS_SAN_FILE="$TEST_RESULTS_SAN_DIR/$TEST_RESULTS_SAN_FILE_PFX"
+		BAIL_OUT "cannot create $TEST_RESULTS_SAN_DIR"
+	fi &&
+	TEST_RESULTS_SAN_FILE="$TEST_RESULTS_SAN_DIR/$TEST_RESULTS_SAN_FILE_PFX"
 
-		# In case "test-results" is left over from a previous
-		# run: Only report if new leaks show up.
-		TEST_RESULTS_SAN_DIR_NR_LEAKS_STARTUP=$(nr_san_dir_leaks_)
+	# In case "test-results" is left over from a previous
+	# run: Only report if new leaks show up.
+	TEST_RESULTS_SAN_DIR_NR_LEAKS_STARTUP=$(nr_san_dir_leaks_)
 
-		# Don't litter *.leak dirs if there was nothing to report
-		test_atexit "rmdir \"$TEST_RESULTS_SAN_DIR\" 2>/dev/null || :"
+	# Don't litter *.leak dirs if there was nothing to report
+	test_atexit "rmdir \"$TEST_RESULTS_SAN_DIR\" 2>/dev/null || :"
+
+	prepend_var LSAN_OPTIONS : dedup_token_length=9999
+	prepend_var LSAN_OPTIONS : log_exe_name=1
+	prepend_var LSAN_OPTIONS : log_path=\"$TEST_RESULTS_SAN_FILE\"
+	export LSAN_OPTIONS
 
-		prepend_var LSAN_OPTIONS : dedup_token_length=9999
-		prepend_var LSAN_OPTIONS : log_exe_name=1
-		prepend_var LSAN_OPTIONS : log_path=\"$TEST_RESULTS_SAN_FILE\"
-		export LSAN_OPTIONS
-	fi
 elif test "$GIT_TEST_PASSING_SANITIZE_LEAK" = "check" ||
      test_bool_env GIT_TEST_PASSING_SANITIZE_LEAK false
 then
 	BAIL_OUT_ENV_NEEDS_SANITIZE_LEAK "GIT_TEST_PASSING_SANITIZE_LEAK=true"
-elif test_bool_env GIT_TEST_SANITIZE_LEAK_LOG false
-then
-	BAIL_OUT_ENV_NEEDS_SANITIZE_LEAK "GIT_TEST_SANITIZE_LEAK_LOG=true"
 fi
 
 if test "${GIT_TEST_CHAIN_LINT:-1}" != 0 &&

base-commit: 47c6d4dad22a751068a4975f1c4177cc6c0c41d2
-- 
2.45.1

Re: [PATCH v3] test-lib: GIT_TEST_SANITIZE_LEAK_LOG enabled by default

[Jeff King]

On Thu, Jul 11, 2024 at 11:10:51PM +0900, Rubén Justo wrote:

> As we currently describe in t/README, it can happen that:
> 
>     Some tests run "git" (or "test-tool" etc.) without properly checking
>     the exit code, or git will invoke itself and fail to ferry the
>     abort() exit code to the original caller.
> 
> Therefore, GIT_TEST_SANITIZE_LEAK_LOG=true is needed to be set to
> capture all memory leaks triggered by our tests.
> 
> It seems unnecessary to force users to remember this option, as
> forgetting it could lead to missed memory leaks.
> 
> We could solve the problem by making it "true" by default, but that
> might suggest we think "false" makes sense, which isn't the case.
> 
> Therefore, the best approach is to remove the option entirely while
> maintaining the capability to detect memory leaks in blind spots of our
> tests.
> 
> Signed-off-by: Rubén Justo <rjusto@gmail.com>
> ---
> 
> This iteration mainly reduces the noise introduced in the previous
> iteration.

Thanks, this one looks good to me.

-Peff

Re: [PATCH] test-lib: GIT_TEST_SANITIZE_LEAK_LOG enabled by default

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1082 bytes --]

On Tue, Jul 09, 2024 at 09:12:06PM -0400, Jeff King wrote:
> On Wed, Jul 10, 2024 at 02:51:58AM +0200, Rubén Justo wrote:
> 
> > As we describe in t/README, it can happen that:
> > 
> >     Some tests run "git" (or "test-tool" etc.) without properly checking
> >     the exit code, or git will invoke itself and fail to ferry the
> >     abort() exit code to the original caller.
> > 
> > Therefore, GIT_TEST_SANITIZE_LEAK_LOG must be set to true to capture all
> > memory leaks triggered by the tests when SANITIZE=leak.
> > 
> > Set it to true by default, and stop worrying about someone checking for
> > leaks who isn't aware of this option and might be missing some leaks.
> 
> I'm obviously in favor of this direction, but...why stop here? Do we
> expect somebody to set it to false? If not, then can't we just get rid
> of it entirely?

I'd also be strongly in favor of just removing this variable altogether.
I found it quite tedious to remember setting it when working on the
memory leak fixes recently, and I'm not aware of any downsides.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]