git/list[1] front-page[2] threads[3] people[4] search[5] about
 

[PATCH v2 26/27] test-lib: unconditionally enable leak checking

From
Patrick Steinhardt <ps@pks.im>
Date
Nov 11, 2024, 10:38 UTC
Message-ID
<20241111-b4-pks-leak-fixes-pt10-v2-26-6154bf91f0b0@pks.im>
In-Reply-To
<20241111-b4-pks-leak-fixes-pt10-v2-0-6154bf91f0b0@pks.im>

Over the last two releases we have plugged a couple hundred of memory leaks exposed by the Git test suite. With the preceding commits we have finally fixed the last leak exposed by our test suite, which means that we are now basically leak free wherever we have branch coverage.

From hereon, the Git test suite should ideally stay free of memory leaks. Most importantly, any test suite that is being added should automatically be subject to the leak checker, and if that test does not pass it is a strong signal that the added code introduced new memory leaks and should not be accepted without further changes.

Drop the infrastructure around TEST_PASSES_SANITIZE_LEAK to reflect this new requirement. Like this, all test suites will be subject to the leak checker by default.

This is being intentionally strict, but we still have an escape hatch: the SANITIZE_LEAK prerequisite. There is one known case in t5601 where the leak sanitizer itself is buggy, so adding this prereq in such cases is acceptable. Another acceptable situation is when a newly added test uncovers preexisting memory leaks: when fixing that memory leak would be sufficiently complicated it is fine to annotate and document the leak accordingly. But in any case, the burden is now on the patch author to explain why exactly they have to add the SANITIZE_LEAK prerequisite.

The TEST_PASSES_SANITIZE_LEAK annotations will be dropped in the next patch.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 ci/lib.sh        |  1 -
 t/README         | 21 -----------------
 t/lib-git-svn.sh |  4 ----
 t/test-lib.sh    | 72 +-------------------------------------------------------
 4 files changed, 1 insertion(+), 97 deletions(-)
diff --git a/ci/lib.sh b/ci/lib.sh
index 246072a0932621a64562d94f5a50a1ba880bd48b..930f98d7228166c37c236beb062b14675fb68ef3 100755
--- a/ci/lib.sh
+++ b/ci/lib.sh
@@ -384,7 +384,6 @@ linux-musl)
 	;;
 linux-leaks|linux-reftable-leaks)
 	export SANITIZE=leak
-	export GIT_TEST_PASSING_SANITIZE_LEAK=true
 	;;
 linux-asan-ubsan)
 	export SANITIZE=address,undefined
diff --git a/t/README b/t/README
index 8c0319b58e5c8333a13f4d07a47519fa8f137709..e84824dc002932102d0021e96c80f70354d4994c 100644
--- a/t/README
+++ b/t/README
@@ -368,27 +368,6 @@ excluded as so much relies on it, but this might change in the future.
 GIT_TEST_SPLIT_INDEX=<boolean> forces split-index mode on the whole
 test suite. Accept any boolean values that are accepted by git-config.
 
-GIT_TEST_PASSING_SANITIZE_LEAK=true skips those tests that haven't
-declared themselves as leak-free by setting
-"TEST_PASSES_SANITIZE_LEAK=true" before sourcing "test-lib.sh". This
-test mode is used by the "linux-leaks" CI target.
-
-GIT_TEST_PASSING_SANITIZE_LEAK=check checks that our
-"TEST_PASSES_SANITIZE_LEAK=true" markings are current. Rather than
-skipping those tests that haven't set "TEST_PASSES_SANITIZE_LEAK=true"
-before sourcing "test-lib.sh" this mode runs them with
-"--invert-exit-code". This is used to check that there's a one-to-one
-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_PASSING_SANITIZE_LEAK=check when combined with "--immediate"
-will run to completion faster, and result in the same failing
-tests.
-
-GIT_TEST_PASSING_SANITIZE_LEAK=check-failing behaves the same as "check",
-but skips all tests which are already marked as leak-free.
-
 GIT_TEST_PROTOCOL_VERSION=<n>, when set, makes 'protocol.version'
 default to n.
 
diff --git a/t/lib-git-svn.sh b/t/lib-git-svn.sh
index ea28971e8ee6abfcd3c0394c6d8b143c75ac12dd..2fde2353fd38356548fd40e4808984618b0d9585 100644
--- a/t/lib-git-svn.sh
+++ b/t/lib-git-svn.sh
@@ -1,7 +1,3 @@
-if test -z "$TEST_FAILS_SANITIZE_LEAK"
-then
-	TEST_PASSES_SANITIZE_LEAK=true
-fi
 . ./test-lib.sh
 
 if test -n "$NO_SVN_TESTS"
diff --git a/t/test-lib.sh b/t/test-lib.sh
index a278181a0568a2422ab1e7f007bc016b95a58e63..508b5fe1f550db690ba769c9c57f5ca8ce3d3102 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -1227,23 +1227,7 @@ check_test_results_san_file_ () {
 	fi &&
 	say_color error "$(cat "$TEST_RESULTS_SAN_FILE".*)" &&
 
-	if test -n "$passes_sanitize_leak" && test "$test_failure" = 0
-	then
-		say "As TEST_PASSES_SANITIZE_LEAK=true and our logs show we're leaking, exit non-zero!" &&
-		invert_exit_code=t
-	elif test -n "$passes_sanitize_leak"
-	then
-		say "As TEST_PASSES_SANITIZE_LEAK=true and our logs show we're leaking, and we're failing for other reasons too..." &&
-		invert_exit_code=
-	elif test -n "$sanitize_leak_check" && test "$test_failure" = 0
-	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=
-	elif test -n "$sanitize_leak_check"
-	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
-	elif test "$test_failure" = 0
+	if test "$test_failure" = 0
 	then
 		say "Our logs revealed a memory leak, exit non-zero!" &&
 		invert_exit_code=t
@@ -1274,11 +1258,6 @@ test_done () {
 		EOF
 	fi
 
-	if test -z "$passes_sanitize_leak" && test_bool_env TEST_PASSES_SANITIZE_LEAK false
-	then
-		BAIL_OUT "Please, set TEST_PASSES_SANITIZE_LEAK before sourcing test-lib.sh"
-	fi
-
 	if test "$test_fixed" != 0
 	then
 		say_color error "# $test_fixed known breakage(s) vanished; please update test(s)"
@@ -1515,51 +1494,8 @@ then
 	test_done
 fi
 
-BAIL_OUT_ENV_NEEDS_SANITIZE_LEAK () {
-	BAIL_OUT "$1 has no effect except when compiled with SANITIZE=leak"
-}
-
 if test -n "$SANITIZE_LEAK"
 then
-	# Normalize with test_bool_env
-	passes_sanitize_leak=
-
-	# We need to see TEST_PASSES_SANITIZE_LEAK in "test-tool
-	# env-helper" (via test_bool_env)
-	export TEST_PASSES_SANITIZE_LEAK
-	if test_bool_env TEST_PASSES_SANITIZE_LEAK false
-	then
-		passes_sanitize_leak=t
-	fi
-
-	if test "$GIT_TEST_PASSING_SANITIZE_LEAK" = "check" ||
-	   test "$GIT_TEST_PASSING_SANITIZE_LEAK" = "check-failing"
-	then
-		if test "$GIT_TEST_PASSING_SANITIZE_LEAK" = "check-failing" &&
-		   test -n "$passes_sanitize_leak"
-		then
-			skip_all="skipping leak-free $this_test under GIT_TEST_PASSING_SANITIZE_LEAK=check-failing"
-			test_done
-		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
-		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
-
 	rm -rf "$TEST_RESULTS_SAN_DIR"
 	if ! mkdir -p "$TEST_RESULTS_SAN_DIR"
 	then
@@ -1574,12 +1510,6 @@ then
 	prepend_var LSAN_OPTIONS : log_exe_name=1
 	prepend_var LSAN_OPTIONS : log_path="'$TEST_RESULTS_SAN_FILE'"
 	export LSAN_OPTIONS
-
-elif test "$GIT_TEST_PASSING_SANITIZE_LEAK" = "check" ||
-     test "$GIT_TEST_PASSING_SANITIZE_LEAK" = "check-failing" ||
-     test_bool_env GIT_TEST_PASSING_SANITIZE_LEAK false
-then
-	BAIL_OUT_ENV_NEEDS_SANITIZE_LEAK "GIT_TEST_PASSING_SANITIZE_LEAK=true"
 fi
 
 if test "${GIT_TEST_CHAIN_LINT:-1}" != 0 &&

-- 
2.47.0.229.g8f8d6eee53.dirty
Previous: Patrick SteinhardtNext: Patrick Steinhardt
Message 40 of 45 in “Memory leak fixes (pt.10, final)”
  1. 00/27 Memory leak fixes (pt.10, final)Patrick Steinhardt, Nov 11, 2024
  2. 01/27 builtin/blame: fix leaking blame entries with `--incremental`Patrick Steinhardt, Nov 11, 2024
  3. 02/27 bisect: fix leaking good/bad terms when reading multipe timesPatrick Steinhardt, Nov 11, 2024
  4. 03/27 bisect: fix leaking string in `handle_bad_merge_base()`Patrick Steinhardt, Nov 11, 2024
  5. 04/27 bisect: fix leaking `current_bad_oid`Patrick Steinhardt, Nov 11, 2024
  6. 05/27 bisect: fix multiple leaks in `bisect_next_all()`Patrick Steinhardt, Nov 11, 2024
  7. 06/27 bisect: fix leaking commit list items in `check_merge_base()`Patrick Steinhardt, Nov 11, 2024
  8. 07/27 bisect: fix various cases where we leak commit list itemsPatrick Steinhardt, Nov 11, 2024
  9. Toon ClaesNov 20, 2024
  10. Patrick SteinhardtNov 20, 2024
  11. 08/27 line-log: fix leak when rewriting commit parentsPatrick Steinhardt, Nov 11, 2024
  12. 09/27 strvec: introduce new `strvec_splice()` functionPatrick Steinhardt, Nov 11, 2024
  13. Toon ClaesNov 20, 2024
  14. Patrick SteinhardtNov 20, 2024
  15. Junio C HamanoNov 20, 2024
  16. Jeff KingNov 21, 2024
  17. Jeff KingNov 21, 2024
  18. Doxygen-styled comments [was: Re: [PATCH v2 09/27] strvec: introduce new `strvec_splice()` function]Toon Claes, Nov 21, 2024
  19. Jeff KingNov 21, 2024
  20. 10/27 git: refactor alias handling to use a `struct strvec`Patrick Steinhardt, Nov 11, 2024
  21. 11/27 git: refactor builtin handling to use a `struct strvec`Patrick Steinhardt, Nov 11, 2024
  22. Toon ClaesNov 20, 2024
  23. 12/27 split-index: fix memory leak in `move_cache_to_base_index()`Patrick Steinhardt, Nov 11, 2024
  24. 13/27 builtin/sparse-checkout: fix leaking sanitized patternsPatrick Steinhardt, Nov 11, 2024
  25. 14/27 help: refactor to not use globals for reading configPatrick Steinhardt, Nov 11, 2024
  26. 15/27 help: fix leaking `struct cmdnames`Patrick Steinhardt, Nov 11, 2024
  27. 16/27 help: fix leaking return value from `help_unknown_cmd()`Patrick Steinhardt, Nov 11, 2024
  28. 17/27 builtin/help: fix leaks in `check_git_cmd()`Patrick Steinhardt, Nov 11, 2024
  29. 18/27 builtin/init-db: fix leaking directory pathsPatrick Steinhardt, Nov 11, 2024
  30. 19/27 builtin/branch: fix leaking sorting optionsPatrick Steinhardt, Nov 11, 2024
  31. 20/27 t/helper: fix leaking commit graph in "read-graph" subcommandPatrick Steinhardt, Nov 11, 2024
  32. 21/27 global: drop `UNLEAK()` annotationPatrick Steinhardt, Nov 11, 2024
  33. Jeff KingNov 12, 2024
  34. Patrick SteinhardtNov 12, 2024
  35. Jeff KingNov 12, 2024
  36. 22/27 git-compat-util: drop now-unused `UNLEAK()` macroPatrick Steinhardt, Nov 11, 2024
  37. 23/27 t5601: work around leak sanitizer issuePatrick Steinhardt, Nov 11, 2024
  38. 24/27 t: mark some tests as leak freePatrick Steinhardt, Nov 11, 2024
  39. 25/27 t: remove unneeded !SANITIZE_LEAK prerequisitesPatrick Steinhardt, Nov 11, 2024
  40. 26/27 test-lib: unconditionally enable leak checkingPatrick Steinhardt, Nov 11, 2024
  41. 27/27 t: remove TEST_PASSES_SANITIZE_LEAK annotationsPatrick Steinhardt, Nov 11, 2024
  42. Toon ClaesNov 20, 2024
  43. Patrick SteinhardtNov 20, 2024
  44. Rubén JustoNov 11, 2024
  45. Rubén JustoNov 12, 2024

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.