{"thread":{"id":"60810","subject":"[PATCH 0/4] mark tests as leak-free","startedAt":"2024-01-29T21:04:14Z","lastAt":"2024-01-30T20:00:33Z","messageCount":14,"participants":["Rubén Justo","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"487550","messageId":"45eb0748-6415-4e52-a54f-8d4e5ad57dde@gmail.com","threadId":"60810","inReplyTo":null,"subject":"[PATCH 0/4] mark tests as leak-free","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-01-29T21:04:10Z","receivedAt":"2024-01-29T21:04:14Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"The tests: t0080, t5332 and t6113 can be annotated as leak-free.\n\nI used:\n  $ make SANITIZE=leak GIT_TEST_PASSING_SANITIZE_LEAK=check GIT_TEST_SANITIZE_LEAK_LOG=true test\n\nRubén Justo (4):\n  t0080: mark as leak-free\n  t5332: mark as leak-free\n  t6113: mark as leak-free\n  test-lib: check for TEST_PASSES_SANITIZE_LEAK\n\n t/t0080-unit-test-output.sh        | 1 +\n t/t5332-multi-pack-reuse.sh        | 1 +\n t/t6113-rev-list-bitmap-filters.sh | 3 ++-\n t/test-lib.sh                      | 5 +++++\n 4 files changed, 9 insertions(+), 1 deletion(-)\n\n-- \n2.43.0\n"},{"id":"487551","messageId":"4adfcba4-0f2b-44f5-a312-97f00f979435@gmail.com","threadId":"60810","inReplyTo":"45eb0748-6415-4e52-a54f-8d4e5ad57dde@gmail.com","subject":"[PATCH 1/4] t0080: mark as leak-free","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-01-29T21:08:23Z","receivedAt":"2024-01-29T21:08:26Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"This test is leak-free since it was added in e137fe3b29 (unit tests: add\nTAP unit test framework, 2023-11-09)\n\nLet's mark it as leak-free to make sure it stays that way (and to reduce\nnoise when looking for other leak-free scripts after we fix some leaks).\n\nSigned-off-by: Rubén Justo <rjusto@gmail.com>\n---\n t/t0080-unit-test-output.sh | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/t/t0080-unit-test-output.sh b/t/t0080-unit-test-output.sh\nindex 961b54b06c..6657c114a3 100755\n--- a/t/t0080-unit-test-output.sh\n+++ b/t/t0080-unit-test-output.sh\n@@ -2,6 +2,7 @@\n \n test_description='Test the output of the unit test framework'\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n test_expect_success 'TAP output from unit tests' '\n-- \n2.43.0\n"},{"id":"487552","messageId":"c5b2f1ba-6a10-40ae-a2f4-07bd9060d42f@gmail.com","threadId":"60810","inReplyTo":"45eb0748-6415-4e52-a54f-8d4e5ad57dde@gmail.com","subject":"[PATCH 2/4] t5332: mark as leak-free","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-01-29T21:08:27Z","receivedAt":"2024-01-29T21:08:30Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"This test is leak-free since it was added in af626ac0e0 (pack-bitmap:\nenable reuse from all bitmapped packs, 2023-12-14).\n\nLet's mark it as leak-free to make sure it stays that way (and to reduce\nnoise when looking for other leak-free scripts after we fix some leaks).\n\nSigned-off-by: Rubén Justo <rjusto@gmail.com>\n---\n t/t5332-multi-pack-reuse.sh | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/t/t5332-multi-pack-reuse.sh b/t/t5332-multi-pack-reuse.sh\nindex 2ba788b042..99145327a6 100755\n--- a/t/t5332-multi-pack-reuse.sh\n+++ b/t/t5332-multi-pack-reuse.sh\n@@ -2,6 +2,7 @@\n \n test_description='pack-objects multi-pack reuse'\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n . \"$TEST_DIRECTORY\"/lib-bitmap.sh\n \n-- \n2.43.0\n"},{"id":"487553","messageId":"dc7e07fa-a1c9-4515-a903-380ce9fb81c7@gmail.com","threadId":"60810","inReplyTo":"45eb0748-6415-4e52-a54f-8d4e5ad57dde@gmail.com","subject":"[PATCH 3/4] t6113: mark as leak-free","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-01-29T21:08:34Z","receivedAt":"2024-01-29T21:08:36Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"This test does not leak since a96015a517 (pack-bitmap: plug leak in\nfind_objects(), 2023-12-14) when the annotation\nTEST_PASSES_SANITIZE_LEAK=true was also added.\n\nUnfortunately it was added after test-lib.sh is sourced, which makes\nGIT_TEST_PASSING_SANITIZE_LEAK=check error:\n\n   $ make SANITIZE=leak GIT_TEST_PASSING_SANITIZE_LEAK=check test T=t6113-rev-list-bitmap-filters.sh\n   ...\n   make[2]: Entering directory '/tmp/git/git/t'\n   *** t6113-rev-list-bitmap-filters.sh ***\n   in GIT_TEST_PASSING_SANITIZE_LEAK=check mode, setting --invert-exit-code for TEST_PASSES_SANITIZE_LEAK != true\n   ok 1 - set up bitmapped repo\n   ok 2 - filters fallback to non-bitmap traversal\n   ok 3 - blob:none filter\n   ok 4 - blob:none filter with specified blob\n   ok 5 - blob:limit filter\n   ok 6 - blob:limit filter with specified blob\n   ok 7 - tree:0 filter\n   ok 8 - tree:0 filter with specified blob, tree\n   ok 9 - tree:1 filter\n   ok 10 - object:type filter\n   ok 11 - object:type filter with --filter-provided-objects\n   ok 12 - combine filter\n   ok 13 - combine filter with --filter-provided-objects\n   ok 14 - bitmap traversal with --unpacked\n   # passed all 14 test(s)\n   1..14\n   # faking up non-zero exit with --invert-exit-code\n   make[2]: *** [Makefile:68: t6113-rev-list-bitmap-filters.sh] Error 1\n   make[2]: Leaving directory '/tmp/git/git/t'\n   make[1]: *** [Makefile:55: test] Error 2\n   make[1]: Leaving directory '/tmp/git/git/t'\n   make: *** [Makefile:3212: test] Error 2\n\nLet's move the annotation before sourcing test-lib.sh, to make\nGIT_TEST_PASSING_SANITIZE_LEAK=check happy.\n\nSigned-off-by: Rubén Justo <rjusto@gmail.com>\n---\n t/t6113-rev-list-bitmap-filters.sh | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t6113-rev-list-bitmap-filters.sh b/t/t6113-rev-list-bitmap-filters.sh\nindex 459f0d7412..a9656a1ec8 100755\n--- a/t/t6113-rev-list-bitmap-filters.sh\n+++ b/t/t6113-rev-list-bitmap-filters.sh\n@@ -1,10 +1,11 @@\n #!/bin/sh\n \n test_description='rev-list combining bitmaps and filters'\n+\n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n . \"$TEST_DIRECTORY\"/lib-bitmap.sh\n \n-TEST_PASSES_SANITIZE_LEAK=true\n \n test_expect_success 'set up bitmapped repo' '\n \t# one commit will have bitmaps, the other will not\n-- \n2.43.0\n"},{"id":"487554","messageId":"6447b11b-f916-4079-8936-8d4f6c480f57@gmail.com","threadId":"60810","inReplyTo":"45eb0748-6415-4e52-a54f-8d4e5ad57dde@gmail.com","subject":"[PATCH 4/4] test-lib: check for TEST_PASSES_SANITIZE_LEAK","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-01-29T21:08:38Z","receivedAt":"2024-01-29T21:08:40Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"TEST_PASSES_SANITIZE_LEAK must be set before sourcing test-lib.sh, as we\nsay in t/README:\n\n   GIT_TEST_PASSING_SANITIZE_LEAK=true skips those tests that haven't\n   declared themselves as leak-free by setting\n   \"TEST_PASSES_SANITIZE_LEAK=true\" before sourcing \"test-lib.sh\". This\n   test mode is used by the \"linux-leaks\" CI target.\n\n   GIT_TEST_PASSING_SANITIZE_LEAK=check checks that our\n   \"TEST_PASSES_SANITIZE_LEAK=true\" markings are current. Rather than\n   skipping those tests that haven't set \"TEST_PASSES_SANITIZE_LEAK=true\"\n   before sourcing \"test-lib.sh\" this mode runs them with\n   \"--invert-exit-code\". This is used to check that there's a one-to-one\n   mapping between \"TEST_PASSES_SANITIZE_LEAK=true\" and those tests that\n   pass under \"SANITIZE=leak\". This is especially useful when testing a\n   series that fixes various memory leaks with \"git rebase -x\".\n\nIn a recent commit we fixed a test where it was set after sourcing\ntest-lib.sh, leading to confusing results.\n\nTo prevent future oversights, let's add a simple check to ensure the\nvalue for TEST_PASSES_SANITIZE_LEAK remains unchanged at test_done().\n\nSigned-off-by: Rubén Justo <rjusto@gmail.com>\n---\n t/test-lib.sh | 5 +++++\n 1 file changed, 5 insertions(+)\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex fc93aa57e6..042f557a6f 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -1297,6 +1297,11 @@ test_done () {\n \t\tEOF\n \tfi\n \n+\tif test -z \"$passes_sanitize_leak\" && test_bool_env TEST_PASSES_SANITIZE_LEAK false\n+\tthen\n+\t\tBAIL_OUT \"Please, set TEST_PASSES_SANITIZE_LEAK before sourcing test-lib.sh\"\n+\tfi\n+\n \tif test \"$test_fixed\" != 0\n \tthen\n \t\tsay_color error \"# $test_fixed known breakage(s) vanished; please update test(s)\"\n-- \n2.43.0\n"},{"id":"487560","messageId":"xmqqa5onhjm4.fsf@gitster.g","threadId":"60810","inReplyTo":"4adfcba4-0f2b-44f5-a312-97f00f979435@gmail.com","subject":"Re: [PATCH 1/4] t0080: mark as leak-free","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-29T22:15:15Z","receivedAt":"2024-01-29T22:15:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rubén Justo <rjusto@gmail.com> writes:\n\n> This test is leak-free since it was added in e137fe3b29 (unit tests: add\n> TAP unit test framework, 2023-11-09)\n>\n> Let's mark it as leak-free to make sure it stays that way (and to reduce\n> noise when looking for other leak-free scripts after we fix some leaks).\n\nFor other tests in this series, that rationale is a very sensible\nthing, but does it apply to this one?\n\nThe point of the t-basic tests is to ensure the lightweight unit\ntest framework that requires nothing from Git behaves (and keeps\nbehaving) sensibly.  The point of running t[0-9][0-9][0-9][0-9]\ntests under leak sanitizer is to exercise production Git code to\ncatch leaks in Git code.\n\nSo it is not quite clear if we even want to run this t0080 under\nleak sanitizer to begin with.  t0080 is a relatively tiny test, but\ndo we even want to spend leak sanitizer cycles on it?  I dunno.\n\n> Signed-off-by: Rubén Justo <rjusto@gmail.com>\n> ---\n>  t/t0080-unit-test-output.sh | 1 +\n>  1 file changed, 1 insertion(+)\n>\n> diff --git a/t/t0080-unit-test-output.sh b/t/t0080-unit-test-output.sh\n> index 961b54b06c..6657c114a3 100755\n> --- a/t/t0080-unit-test-output.sh\n> +++ b/t/t0080-unit-test-output.sh\n> @@ -2,6 +2,7 @@\n>  \n>  test_description='Test the output of the unit test framework'\n>  \n> +TEST_PASSES_SANITIZE_LEAK=true\n>  . ./test-lib.sh\n>  \n>  test_expect_success 'TAP output from unit tests' '\n"},{"id":"487561","messageId":"xmqq5xzbhj4j.fsf@gitster.g","threadId":"60810","inReplyTo":"6447b11b-f916-4079-8936-8d4f6c480f57@gmail.com","subject":"Re: [PATCH 4/4] test-lib: check for TEST_PASSES_SANITIZE_LEAK","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-29T22:25:48Z","receivedAt":"2024-01-29T22:25:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rubén Justo <rjusto@gmail.com> writes:\n\n> In a recent commit we fixed a test where it was set after sourcing\n> test-lib.sh, leading to confusing results.\n\nOK, the reference is to the previous step.  Makes sense.\n\n>\n> To prevent future oversights, let's add a simple check to ensure the\n> value for TEST_PASSES_SANITIZE_LEAK remains unchanged at test_done().\n>\n> Signed-off-by: Rubén Justo <rjusto@gmail.com>\n> ---\n>  t/test-lib.sh | 5 +++++\n>  1 file changed, 5 insertions(+)\n>\n> diff --git a/t/test-lib.sh b/t/test-lib.sh\n> index fc93aa57e6..042f557a6f 100644\n> --- a/t/test-lib.sh\n> +++ b/t/test-lib.sh\n> @@ -1297,6 +1297,11 @@ test_done () {\n>  \t\tEOF\n>  \tfi\n>  \n> +\tif test -z \"$passes_sanitize_leak\" && test_bool_env TEST_PASSES_SANITIZE_LEAK false\n> +\tthen\n> +\t\tBAIL_OUT \"Please, set TEST_PASSES_SANITIZE_LEAK before sourcing test-lib.sh\"\n> +\tfi\n> +\n>  \tif test \"$test_fixed\" != 0\n>  \tthen\n>  \t\tsay_color error \"# $test_fixed known breakage(s) vanished; please update test(s)\"\n"},{"id":"487562","messageId":"c932fbfc-f14f-4403-bfc5-cf1d616b22de@gmail.com","threadId":"60810","inReplyTo":"xmqqa5onhjm4.fsf@gitster.g","subject":"Re: [PATCH 1/4] t0080: mark as leak-free","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-01-29T23:20:22Z","receivedAt":"2024-01-29T23:20:29Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On 29-ene-2024 14:15:15, Junio C Hamano wrote:\n> Rubén Justo <rjusto@gmail.com> writes:\n> \n> > This test is leak-free since it was added in e137fe3b29 (unit tests: add\n> > TAP unit test framework, 2023-11-09)\n> >\n> > Let's mark it as leak-free to make sure it stays that way (and to reduce\n> > noise when looking for other leak-free scripts after we fix some leaks).\n> \n> For other tests in this series, that rationale is a very sensible\n> thing, but does it apply to this one?\n> \n> The point of the t-basic tests is to ensure the lightweight unit\n> test framework that requires nothing from Git behaves (and keeps\n> behaving) sensibly.  The point of running t[0-9][0-9][0-9][0-9]\n> tests under leak sanitizer is to exercise production Git code to\n> catch leaks in Git code.\n> \n> So it is not quite clear if we even want to run this t0080 under\n> leak sanitizer to begin with.  t0080 is a relatively tiny test, but\n> do we even want to spend leak sanitizer cycles on it?  I dunno.\n\nIIUC, that would imply building test-tool with a different set of flags\nthan Git, new artifacts ...  or running test-tool with some LSAN_OPTIONS\noptions, to disable it ...  or both ... or ...\n\nAnd that is assuming that with test-tool we won't catch a leak in Git\nthat we're not seeing in the other tests ...\n\nMaybe this is tangential to this series but,  while a decision is being\nmade, annotating the test makes GIT_TEST_PASSING_SANITIZE_LEAK=check\npass, which is the objective in this series. \n\n> \n> > Signed-off-by: Rubén Justo <rjusto@gmail.com>\n> > ---\n> >  t/t0080-unit-test-output.sh | 1 +\n> >  1 file changed, 1 insertion(+)\n> >\n> > diff --git a/t/t0080-unit-test-output.sh b/t/t0080-unit-test-output.sh\n> > index 961b54b06c..6657c114a3 100755\n> > --- a/t/t0080-unit-test-output.sh\n> > +++ b/t/t0080-unit-test-output.sh\n> > @@ -2,6 +2,7 @@\n> >  \n> >  test_description='Test the output of the unit test framework'\n> >  \n> > +TEST_PASSES_SANITIZE_LEAK=true\n> >  . ./test-lib.sh\n> >  \n> >  test_expect_success 'TAP output from unit tests' '\n"},{"id":"487563","messageId":"xmqqwmrrg0l6.fsf@gitster.g","threadId":"60810","inReplyTo":"c932fbfc-f14f-4403-bfc5-cf1d616b22de@gmail.com","subject":"Re: [PATCH 1/4] t0080: mark as leak-free","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-29T23:51:33Z","receivedAt":"2024-01-29T23:51:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rubén Justo <rjusto@gmail.com> writes:\n\n>> The point of the t-basic tests is to ensure the lightweight unit\n>> test framework that requires nothing from Git behaves (and keeps\n>> behaving) sensibly.  The point of running t[0-9][0-9][0-9][0-9]\n>> tests under leak sanitizer is to exercise production Git code to\n>> catch leaks in Git code.\n>> \n>> So it is not quite clear if we even want to run this t0080 under\n>> leak sanitizer to begin with.  t0080 is a relatively tiny test, but\n>> do we even want to spend leak sanitizer cycles on it?  I dunno.\n>\n> IIUC, that would imply building test-tool with a different set of flags\n> than Git, new artifacts ...  or running test-tool with some LSAN_OPTIONS\n> options, to disable it ...  or both ... or ...\n>\n> And that is assuming that with test-tool we won't catch a leak in Git\n> that we're not seeing in the other tests ...\n\nBut t0080 does not even run test-tool, does it?  The t-basic unit\ntest is about testing the unit test framework and does not even\ntrigger any of the half-libified Git code.  So I am not sure why\nyou are bringing up test-tool into the picture.\n\n> Maybe this is tangential to this series but,  while a decision is being\n> made, annotating the test makes GIT_TEST_PASSING_SANITIZE_LEAK=check\n> pass, which is the objective in this series. \n\nOne major reason why we want to set TEST_PASSES_SANITIZE_LEAK to\ntrue is because that way the marked test will be run under the leak\nsanitizer in the CI.\n\nWhat do we expect to gain by running t0080, which is to run the\nt-basic unit test, under the leak sanitizer?  Unlike other\nt[0-9][0-9][0-9][0-9] tests that exercise Git production code, would\nwe care about a new leak found in t-basic run from t0080 in the\nfirst place?\n\nAnnotating with TEST_PASSES_SANITIZE_LEAK is not a goal by itself.\nAnnotating the tests that we want to run under the sanitizer and see\nthem passing with it is.  And obviously these tests that exercise\nGit production code are very good candidates for us to do so.  It is\nunclear if t0080 falls into the same category.  That is why I asked\nwhat we expect to gain by running it.\n\nThanks.\n"},{"id":"487585","messageId":"20240130055333.GB166761@coredump.intra.peff.net","threadId":"60810","inReplyTo":"xmqqa5onhjm4.fsf@gitster.g","subject":"Re: [PATCH 1/4] t0080: mark as leak-free","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-01-30T05:53:33Z","receivedAt":"2024-01-30T05:53:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jan 29, 2024 at 02:15:15PM -0800, Junio C Hamano wrote:\n\n> > Let's mark it as leak-free to make sure it stays that way (and to reduce\n> > noise when looking for other leak-free scripts after we fix some leaks).\n> \n> For other tests in this series, that rationale is a very sensible\n> thing, but does it apply to this one?\n> \n> The point of the t-basic tests is to ensure the lightweight unit\n> test framework that requires nothing from Git behaves (and keeps\n> behaving) sensibly.  The point of running t[0-9][0-9][0-9][0-9]\n> tests under leak sanitizer is to exercise production Git code to\n> catch leaks in Git code.\n> \n> So it is not quite clear if we even want to run this t0080 under\n> leak sanitizer to begin with.  t0080 is a relatively tiny test, but\n> do we even want to spend leak sanitizer cycles on it?  I dunno.\n\nI think you are right that we do not particularly care about leaks in\nthe t-basic code. That is also true of other test harness code (other\nunit-tests, but also stuff in t/helper). But IMHO it is less work to\njust keep that code leak-free than it is to try to distinguish between\nproduction and test code.\n\nRight now, it is not that hard to simply leave the PASSES_SANITIZE_LEAK\nflag off of t0080, and then it won't be run in the leak-checking CI job.\nBut I think the end-game of all of this leak-checking stuff is that\neventually _everything_ will be leak-free, and we will discard the whole\nPASSES_SANITIZE_LEAK mechanism entirely. And in that end-game, it is\nsimpler for everything, including t-basic, to just be leak-free and\nchecked under the same regime.\n\nSetting the flag now just makes sure we continue correctly on that path,\nrather than getting surprised near the end of the road that t-basic has\nsome dumb leak. Plus it avoids the script popping up as a false positive\nwhen checking for scripts which can be marked.\n\n-Peff\n"},{"id":"487587","messageId":"20240130055448.GC166761@coredump.intra.peff.net","threadId":"60810","inReplyTo":"45eb0748-6415-4e52-a54f-8d4e5ad57dde@gmail.com","subject":"Re: [PATCH 0/4] mark tests as leak-free","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-01-30T05:54:48Z","receivedAt":"2024-01-30T05:54:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jan 29, 2024 at 10:04:10PM +0100, Rubén Justo wrote:\n\n> The tests: t0080, t5332 and t6113 can be annotated as leak-free.\n> \n> I used:\n>   $ make SANITIZE=leak GIT_TEST_PASSING_SANITIZE_LEAK=check GIT_TEST_SANITIZE_LEAK_LOG=true test\n> \n> Rubén Justo (4):\n>   t0080: mark as leak-free\n>   t5332: mark as leak-free\n>   t6113: mark as leak-free\n>   test-lib: check for TEST_PASSES_SANITIZE_LEAK\n\nThese all looked reasonable to me. Thank you for not just fixing them,\nbut including the background for each case (e.g., leak-free as of commit\nXYZ, etc).\n\n-Peff\n"},{"id":"487599","messageId":"xmqqr0hydd3e.fsf@gitster.g","threadId":"60810","inReplyTo":"20240130055448.GC166761@coredump.intra.peff.net","subject":"Re: [PATCH 0/4] mark tests as leak-free","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-30T16:01:57Z","receivedAt":"2024-01-30T16:02:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Mon, Jan 29, 2024 at 10:04:10PM +0100, Rubén Justo wrote:\n>\n>> The tests: t0080, t5332 and t6113 can be annotated as leak-free.\n>> \n>> I used:\n>>   $ make SANITIZE=leak GIT_TEST_PASSING_SANITIZE_LEAK=check GIT_TEST_SANITIZE_LEAK_LOG=true test\n>> \n>> Rubén Justo (4):\n>>   t0080: mark as leak-free\n>>   t5332: mark as leak-free\n>>   t6113: mark as leak-free\n>>   test-lib: check for TEST_PASSES_SANITIZE_LEAK\n>\n> These all looked reasonable to me. Thank you for not just fixing them,\n> but including the background for each case (e.g., leak-free as of commit\n> XYZ, etc).\n\nYup, the background description was very useful to read.\n\nThanks.\n"},{"id":"487605","messageId":"f3a84780-de59-421a-b4a7-2e33b4fd2d97@gmail.com","threadId":"60810","inReplyTo":"xmqqwmrrg0l6.fsf@gitster.g","subject":"Re: [PATCH 1/4] t0080: mark as leak-free","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-01-30T18:14:12Z","receivedAt":"2024-01-30T18:14:16Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On 29-ene-2024 15:51:33, Junio C Hamano wrote:\n> Rubén Justo <rjusto@gmail.com> writes:\n> \n> >> The point of the t-basic tests is to ensure the lightweight unit\n> >> test framework that requires nothing from Git behaves (and keeps\n> >> behaving) sensibly.  The point of running t[0-9][0-9][0-9][0-9]\n> >> tests under leak sanitizer is to exercise production Git code to\n> >> catch leaks in Git code.\n> >> \n> >> So it is not quite clear if we even want to run this t0080 under\n> >> leak sanitizer to begin with.  t0080 is a relatively tiny test, but\n> >> do we even want to spend leak sanitizer cycles on it?  I dunno.\n> >\n> > IIUC, that would imply building test-tool with a different set of flags\n> > than Git, new artifacts ...  or running test-tool with some LSAN_OPTIONS\n> > options, to disable it ...  or both ... or ...\n> >\n> > And that is assuming that with test-tool we won't catch a leak in Git\n> > that we're not seeing in the other tests ...\n> \n> But t0080 does not even run test-tool, does it?  The t-basic unit\n> test is about testing the unit test framework and does not even\n> trigger any of the half-libified Git code.  So I am not sure why\n> you are bringing up test-tool into the picture.\n\nOf course, test-tool has nothing to do here.  I think I got distracted\nbecause:\n\n  $ ( cd t; ./t0080-unit-test-output.sh )\n  Bail out! You need to build test-tool; Run \"make t/helper/test-tool\" in the source (toplevel) directory\n\nMy reasoning was about t/unit-test/bin/t-basic (though also applies to\ntest-tool), due to:\n\n  $ make SANITIZE=leak -n t/unit-tests/bin/t-basic \n  ...\n  echo '   ' LINK t/unit-tests/bin/t-basic;cc   -g -O2 -Wall -I. \\\n  -DHAVE_SYSINFO -fsanitize=leak -fno-sanitize-recover=leak \\\n  -fno-omit-frame-pointer -DSUPPRESS_ANNOTATED_LEAKS -O0 \\\n  -DGIT_HOST_CPU=\"\\\"x86_64\\\"\" -DHAVE_ALLOCA_H  -DUSE_CURL_FOR_IMAP_SEND \\\n  -DSUPPORTS_SIMPLE_IPC -DSHA1_DC -DSHA1DC_NO_STANDARD_INCLUDES \\\n  -DSHA1DC_INIT_SAFE_HASH_DEFAULT=0 \\\n  -DSHA1DC_CUSTOM_INCLUDE_SHA1_C=\"\\\"git-compat-util.h\\\"\" \\\n  -DSHA1DC_CUSTOM_INCLUDE_UBC_CHECK_C=\"\\\"git-compat-util.h\\\"\" -DSHA256_BLK \\\n  -DHAVE_PATHS_H -DHAVE_DEV_TTY -DHAVE_CLOCK_GETTIME \\\n  -DHAVE_CLOCK_MONOTONIC -DHAVE_SYNC_FILE_RANGE -DHAVE_GETDELIM \\\n  '-DPROCFS_EXECUTABLE_PATH=\"/proc/self/exe\"' -DFREAD_READS_DIRECTORIES \\\n  -DNO_STRLCPY -DSHELL_PATH='\"/bin/sh\"' -o t/unit-tests/bin/t-basic   \\\n  t/unit-tests/t-basic.o t/unit-tests/test-lib.o common-main.o libgit.a \\\n  xdiff/lib.a reftable/libreftable.a libgit.a xdiff/lib.a \\\n  reftable/libreftable.a libgit.a -lz -lpthread -lrt\n\nNote that we inject this flags:\n  -fsanitize=leak -fno-sanitize-recover=leak -fno-omit-frame-pointer \\\n  -DSUPPRESS_ANNOTATED_LEAKS -O0\n\n> \n> > Maybe this is tangential to this series but,  while a decision is being\n> > made, annotating the test makes GIT_TEST_PASSING_SANITIZE_LEAK=check\n> > pass, which is the objective in this series. \n> \n> One major reason why we want to set TEST_PASSES_SANITIZE_LEAK to\n> true is because that way the marked test will be run under the leak\n> sanitizer in the CI.\n> \n> What do we expect to gain by running t0080, which is to run the\n> t-basic unit test, under the leak sanitizer?  Unlike other\n> t[0-9][0-9][0-9][0-9] tests that exercise Git production code, would\n> we care about a new leak found in t-basic run from t0080 in the\n> first place?\n> \n> Annotating with TEST_PASSES_SANITIZE_LEAK is not a goal by itself.\n\nIndeed.  It points to a horizon.\n\n> Annotating the tests that we want to run under the sanitizer and see\n> them passing with it is.\n\nMaybe this is also a horizon (not reachable by definition), and\nexpecting \"make test\" to be leak-free (including t0080) a good path\ntowards that horizon, IMHO.  But you are right, those leak sanitizer\ncycles may not be worth it.\n\n> And obviously these tests that exercise\n> Git production code are very good candidates for us to do so.  It is\n> unclear if t0080 falls into the same category.  That is why I asked\n> what we expect to gain by running it.\n> \n> Thanks.\n\nThank you for bringing up a good question.\n\nI see you queued this as 4/4.  OK.  I'll consider that if a re-roll for\nthis series is needed.\n"},{"id":"487608","messageId":"xmqqr0hya8x3.fsf@gitster.g","threadId":"60810","inReplyTo":"20240130055333.GB166761@coredump.intra.peff.net","subject":"Re: [PATCH 1/4] t0080: mark as leak-free","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-30T20:00:24Z","receivedAt":"2024-01-30T20:00:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Setting the flag now just makes sure we continue correctly on that path,\n> rather than getting surprised near the end of the road that t-basic has\n> some dumb leak. Plus it avoids the script popping up as a false positive\n> when checking for scripts which can be marked.\n\nAlright.  Any such \"dumb leak\" in the \"basic\" would hopefully be\ncaught by the real t-$other unit tests exercised under the leak\nsanitizer, I hope, but it is not worth our time wondering if it\nmakes sense to special case t0080 specifically.\n"}]}