{"thread":{"id":"60217","subject":"[PATCH 0/2] fix GIT_TEST_SANITIZE_LEAK_LOG=true","startedAt":"2023-09-09T23:03:10Z","lastAt":"2023-09-23T08:11:52Z","messageCount":16,"participants":["Rubén Justo","Eric Sunshine","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"481636","messageId":"68522960-edda-26d3-ddca-cee63f2d859e@gmail.com","threadId":"60217","inReplyTo":null,"subject":"[PATCH 0/2] fix GIT_TEST_SANITIZE_LEAK_LOG=true","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2023-09-09T23:03:04Z","receivedAt":"2023-09-09T23:03:10Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"While using \"make test\" I noticed that invert_exit_code used by\nGIT_TEST_SANITIZE_LEAK_LOG=true produces unexpected results.\n\n   $ git checkout v2.40.1 \n   $ make test SANITIZE=leak T=t3200-branch.sh # fails\n   $ make test SANITIZE=leak GIT_TEST_SANITIZE_LEAK_LOG=true T=t3200-branch.sh # succeeds\n\n\nRubén Justo (2):\n  test-lib: prevent misuses of --invert-exit-code\n  test-lib: fix GIT_TEST_SANITIZE_LEAK_LOG\n\n t/test-lib.sh | 21 ++++++++++++++++-----\n 1 file changed, 16 insertions(+), 5 deletions(-)\n\n-- \n2.40.1\n\n"},{"id":"481637","messageId":"1a60a1ca-0ef0-ecf5-d0aa-a28d7c148a82@gmail.com","threadId":"60217","inReplyTo":"68522960-edda-26d3-ddca-cee63f2d859e@gmail.com","subject":"[PATCH 1/2] test-lib: prevent misuses of --invert-exit-code","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2023-09-09T23:08:11Z","receivedAt":"2023-09-09T23:08:19Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"GIT_TEST_PASSING_SANITIZE_LEAK=true and GIT_TEST_SANITIZE_LEAK_LOG=true\nuse internnlly the --invert-exit-code machinery.  Therefore if the user\nwants to use --invert-exit-code in combination with them, the result\nwill be confusing.\n\nFor the same reason, we are already using BAIL_OUT if the user tries to\ncombine GIT_TEST_PASSING_SANITIZE_LEAK=check with --invert-exit-code.\n\nLet's do the same for GIT_TEST_PASSING_SANITIZE_LEAK=true and\nGIT_TEST_SANITIZE_LEAK_LOG=true.\n\nSigned-off-by: Rubén Justo <rjusto@gmail.com>\n---\n t/test-lib.sh | 18 ++++++++++++++----\n 1 file changed, 14 insertions(+), 4 deletions(-)\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 293caf0f20..88a34fba67 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -1557,15 +1557,25 @@ then\n \t\t\tsay \"in GIT_TEST_PASSING_SANITIZE_LEAK=check mode, setting --invert-exit-code for TEST_PASSES_SANITIZE_LEAK != true\"\n \t\t\tinvert_exit_code=t\n \t\tfi\n-\telif test -z \"$passes_sanitize_leak\" &&\n-\t     test_bool_env GIT_TEST_PASSING_SANITIZE_LEAK false\n+\telif test_bool_env GIT_TEST_PASSING_SANITIZE_LEAK false\n \tthen\n-\t\tskip_all=\"skipping $this_test under GIT_TEST_PASSING_SANITIZE_LEAK=true\"\n-\t\ttest_done\n+\t\tif test -n \"$invert_exit_code\"\n+\t\tthen\n+\t\t\tBAIL_OUT \"cannot use --invert-exit-code under GIT_TEST_PASSING_SANITIZE_LEAK=true\"\n+\t\telif test -z \"$passes_sanitize_leak\"\n+\t\tthen\n+\t\t\tskip_all=\"skipping $this_test under GIT_TEST_PASSING_SANITIZE_LEAK=true\"\n+\t\t\ttest_done\n+\t\tfi\n \tfi\n \n \tif test_bool_env GIT_TEST_SANITIZE_LEAK_LOG false\n \tthen\n+\t\tif test -n \"$invert_exit_code\"\n+\t\tthen\n+\t\t\tBAIL_OUT \"cannot use --invert-exit-code and GIT_TEST_SANITIZE_LEAK_LOG=true\"\n+\t\tfi\n+\n \t\tif ! mkdir -p \"$TEST_RESULTS_SAN_DIR\"\n \t\tthen\n \t\t\tBAIL_OUT \"cannot create $TEST_RESULTS_SAN_DIR\"\n-- \n2.40.1\n\n"},{"id":"481638","messageId":"64825651-00d2-d2c5-2da2-91a9917bef21@gmail.com","threadId":"60217","inReplyTo":"68522960-edda-26d3-ddca-cee63f2d859e@gmail.com","subject":"[PATCH 2/2] test-lib: fix GIT_TEST_SANITIZE_LEAK_LOG","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2023-09-09T23:09:52Z","receivedAt":"2023-09-09T23:09:58Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"GIT_TEST_SANITIZE_LEAK_LOG=true with a test that leaks, will make the\ntest return zero unintentionally:\n\n  $ git checkout v2.40.1\n  $ make SANITIZE=leak\n  $ make -C t GIT_TEST_SANITIZE_LEAK_LOG=true t3200-branch.sh\n  ...\n  With GIT_TEST_SANITIZE_LEAK_LOG=true our logs revealed a memory leak, exit non-zero!\n  # faked up failures as TODO & now exiting with 0 due to --invert-exit-code\n\nLet's use invert_exit_code only if needed.\n\nSigned-off-by: Rubén Justo <rjusto@gmail.com>\n---\n t/test-lib.sh | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 88a34fba67..87cfea9e9a 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -1263,7 +1263,8 @@ check_test_results_san_file_ () {\n \tthen\n \t\tsay \"As TEST_PASSES_SANITIZE_LEAK=true isn't set the above leak is 'ok' with GIT_TEST_PASSING_SANITIZE_LEAK=check\" &&\n \t\tinvert_exit_code=t\n-\telse\n+\telif test \"$test_failure\" = 0\n+\tthen\n \t\tsay \"With GIT_TEST_SANITIZE_LEAK_LOG=true our logs revealed a memory leak, exit non-zero!\" &&\n \t\tinvert_exit_code=t\n \tfi\n-- \n2.40.1\n\n"},{"id":"481639","messageId":"CAPig+cR+rZ4+Qyfz5YQVMixW0oC65iOzxymwOaUnPqSTVScW7g@mail.gmail.com","threadId":"60217","inReplyTo":"1a60a1ca-0ef0-ecf5-d0aa-a28d7c148a82@gmail.com","subject":"Re: [PATCH 1/2] test-lib: prevent misuses of --invert-exit-code","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2023-09-10T01:59:37Z","receivedAt":"2023-09-10T01:59:53Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sat, Sep 9, 2023 at 7:08 PM Rubén Justo <rjusto@gmail.com> wrote:\n> GIT_TEST_PASSING_SANITIZE_LEAK=true and GIT_TEST_SANITIZE_LEAK_LOG=true\n> use internnlly the --invert-exit-code machinery.  Therefore if the user\n\ns/internnlly/internally/\n\n> wants to use --invert-exit-code in combination with them, the result\n> will be confusing.\n>\n> For the same reason, we are already using BAIL_OUT if the user tries to\n> combine GIT_TEST_PASSING_SANITIZE_LEAK=check with --invert-exit-code.\n>\n> Let's do the same for GIT_TEST_PASSING_SANITIZE_LEAK=true and\n> GIT_TEST_SANITIZE_LEAK_LOG=true.\n>\n> Signed-off-by: Rubén Justo <rjusto@gmail.com>\n"},{"id":"481662","messageId":"577cc92e-9f21-55a8-7389-5f94b1ae9a98@gmail.com","threadId":"60217","inReplyTo":"CAPig+cR+rZ4+Qyfz5YQVMixW0oC65iOzxymwOaUnPqSTVScW7g@mail.gmail.com","subject":"Re: [PATCH 1/2] test-lib: prevent misuses of --invert-exit-code","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2023-09-10T22:58:31Z","receivedAt":"2023-09-10T22:58:44Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On 10/9/23 3:59, Eric Sunshine wrote:\n> On Sat, Sep 9, 2023 at 7:08 PM Rubén Justo <rjusto@gmail.com> wrote:\n>> GIT_TEST_PASSING_SANITIZE_LEAK=true and GIT_TEST_SANITIZE_LEAK_LOG=true\n>> use internnlly the --invert-exit-code machinery.  Therefore if the user\n> \n> s/internnlly/internally/\n\nThanks!\n\n> \n>> wants to use --invert-exit-code in combination with them, the result\n>> will be confusing.\n>>\n>> For the same reason, we are already using BAIL_OUT if the user tries to\n>> combine GIT_TEST_PASSING_SANITIZE_LEAK=check with --invert-exit-code.\n>>\n>> Let's do the same for GIT_TEST_PASSING_SANITIZE_LEAK=true and\n>> GIT_TEST_SANITIZE_LEAK_LOG=true.\n>>\n>> Signed-off-by: Rubén Justo <rjusto@gmail.com>\n"},{"id":"481748","messageId":"20230912082742.GB1630538@coredump.intra.peff.net","threadId":"60217","inReplyTo":"64825651-00d2-d2c5-2da2-91a9917bef21@gmail.com","subject":"Re: [PATCH 2/2] test-lib: fix GIT_TEST_SANITIZE_LEAK_LOG","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-09-12T08:27:42Z","receivedAt":"2023-09-12T08:27:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Sep 10, 2023 at 01:09:52AM +0200, Rubén Justo wrote:\n\n> GIT_TEST_SANITIZE_LEAK_LOG=true with a test that leaks, will make the\n> test return zero unintentionally:\n> \n>   $ git checkout v2.40.1\n>   $ make SANITIZE=leak\n>   $ make -C t GIT_TEST_SANITIZE_LEAK_LOG=true t3200-branch.sh\n>   ...\n>   With GIT_TEST_SANITIZE_LEAK_LOG=true our logs revealed a memory leak, exit non-zero!\n>   # faked up failures as TODO & now exiting with 0 due to --invert-exit-code\n> \n> Let's use invert_exit_code only if needed.\n\nHmm, OK. So we saw some actual test errors (maybe from leaks or maybe\nnot), but then we _also_ saw entries in the leak-log. So the inversion\ncancels out, and we accidentally say everything is OK, which is wrong.\n\nI'm not quite sure of your fix, though. In the if-else chain you're\ntouching, we know going in that we found a leak in the log. And then we\ncover these 5 cases:\n\n  1. if the test is marked as passing-leak\n    a. if we saw no test failures, invert (and mention the leaking log)\n    b. otherwise, do not invert (and mention the log)\n  2. else if we are in \"check\" mode\n    a. if we saw no test failures, do not invert (we do have a leak,\n       which is equivalent to a test failure). Mention the log.\n    b. otherwise, invert (to switch back to \"success\", since we are\n       looking for leaks), but still mention the log.\n  3. invert to trigger failure (and mention the log)\n\nAnd the problem is in (3). You switch it to trigger only if we have no\nfailures (fixing the inversion). But should we have the same a/b split\nfor this case? I.e.:\n\n  3a. if we saw no test failures, invert to cause a failure\n  3b. we saw other failures; do not invert, but _do_ mention that the\n      log found extra leaks\n\nIn 3b we are explaining to the user what happened. Though maybe it is\nnot super important, because I think we'd have dumped the log contents\nanyway?\n\nOther than that, I think the patch is correct. I wondered when we ran\nthis \"check_test_results_san_file_\" code, but it is only at the end of\nthe script. So we are OK to make a definitive call on the zero/non-zero\ncount of failed tests.\n\n-Peff\n"},{"id":"481749","messageId":"20230912083528.GC1630538@coredump.intra.peff.net","threadId":"60217","inReplyTo":"1a60a1ca-0ef0-ecf5-d0aa-a28d7c148a82@gmail.com","subject":"Re: [PATCH 1/2] test-lib: prevent misuses of --invert-exit-code","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-09-12T08:35:28Z","receivedAt":"2023-09-12T08:36:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Sep 10, 2023 at 01:08:11AM +0200, Rubén Justo wrote:\n\n> GIT_TEST_PASSING_SANITIZE_LEAK=true and GIT_TEST_SANITIZE_LEAK_LOG=true\n> use internnlly the --invert-exit-code machinery.  Therefore if the user\n> wants to use --invert-exit-code in combination with them, the result\n> will be confusing.\n> \n> For the same reason, we are already using BAIL_OUT if the user tries to\n> combine GIT_TEST_PASSING_SANITIZE_LEAK=check with --invert-exit-code.\n> \n> Let's do the same for GIT_TEST_PASSING_SANITIZE_LEAK=true and\n> GIT_TEST_SANITIZE_LEAK_LOG=true.\n\nOK, so we are trying to find a case where the user is triggering\n--invert-exit-code themselves and complaining. But in the code...\n\n> @@ -1557,15 +1557,25 @@ then\n>  \t\t\tsay \"in GIT_TEST_PASSING_SANITIZE_LEAK=check mode, setting --invert-exit-code for TEST_PASSES_SANITIZE_LEAK != true\"\n>  \t\t\tinvert_exit_code=t\n>  \t\tfi\n> -\telif test -z \"$passes_sanitize_leak\" &&\n> -\t     test_bool_env GIT_TEST_PASSING_SANITIZE_LEAK false\n> +\telif test_bool_env GIT_TEST_PASSING_SANITIZE_LEAK false\n>  \tthen\n> -\t\tskip_all=\"skipping $this_test under GIT_TEST_PASSING_SANITIZE_LEAK=true\"\n> -\t\ttest_done\n> +\t\tif test -n \"$invert_exit_code\"\n> +\t\tthen\n> +\t\t\tBAIL_OUT \"cannot use --invert-exit-code under GIT_TEST_PASSING_SANITIZE_LEAK=true\"\n> +\t\telif test -z \"$passes_sanitize_leak\"\n> +\t\tthen\n> +\t\t\tskip_all=\"skipping $this_test under GIT_TEST_PASSING_SANITIZE_LEAK=true\"\n> +\t\t\ttest_done\n> +\t\tfi\n>  \tfi\n\nYou can see at the top of the context that we will set\ninvert_exit_code=t ourselves, which will then complain here:\n\n>  \tif test_bool_env GIT_TEST_SANITIZE_LEAK_LOG false\n>  \tthen\n> +\t\tif test -n \"$invert_exit_code\"\n> +\t\tthen\n> +\t\t\tBAIL_OUT \"cannot use --invert-exit-code and GIT_TEST_SANITIZE_LEAK_LOG=true\"\n> +\t\tfi\n> +\n>  \t\tif ! mkdir -p \"$TEST_RESULTS_SAN_DIR\"\n>  \t\tthen\n>  \t\t\tBAIL_OUT \"cannot create $TEST_RESULTS_SAN_DIR\"\n\nThat varible-set in the earlier context is from running in \"check\" mode.\nSo:\n\n  make GIT_TEST_PASSING_SANITIZE_LEAK=check GIT_TEST_SANITIZE_LEAK_LOG=true\n\nwill now always fail. But this is the main way you'd want to run it\n(enabling the leak log catches more stuff, and the log-check function\nyou touch in patch 2 already covers check mode).\n\nSo I think you'd have to hoist your check above the if/else for setting\nup PASSING_SANITIZE_LEAK modes.\n\n-Peff\n"},{"id":"481862","messageId":"0d0e903f-aa3e-964d-7935-ceaae04a413e@gmail.com","threadId":"60217","inReplyTo":"20230912083528.GC1630538@coredump.intra.peff.net","subject":"Re: [PATCH 1/2] test-lib: prevent misuses of --invert-exit-code","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2023-09-15T00:10:12Z","receivedAt":"2023-09-15T00:10:21Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On 12-sep-2023 04:35:28, Jeff King wrote:\n> On Sun, Sep 10, 2023 at 01:08:11AM +0200, Rubén Justo wrote:\n> \n> > GIT_TEST_PASSING_SANITIZE_LEAK=true and GIT_TEST_SANITIZE_LEAK_LOG=true\n> > use internnlly the --invert-exit-code machinery.  Therefore if the user\n> > wants to use --invert-exit-code in combination with them, the result\n> > will be confusing.\n> > \n> > For the same reason, we are already using BAIL_OUT if the user tries to\n> > combine GIT_TEST_PASSING_SANITIZE_LEAK=check with --invert-exit-code.\n> > \n> > Let's do the same for GIT_TEST_PASSING_SANITIZE_LEAK=true and\n> > GIT_TEST_SANITIZE_LEAK_LOG=true.\n> \n> OK, so we are trying to find a case where the user is triggering\n> --invert-exit-code themselves and complaining. But in the code...\n> \n> > @@ -1557,15 +1557,25 @@ then\n> >  \t\t\tsay \"in GIT_TEST_PASSING_SANITIZE_LEAK=check mode, setting --invert-exit-code for TEST_PASSES_SANITIZE_LEAK != true\"\n> >  \t\t\tinvert_exit_code=t\n> >  \t\tfi\n> > -\telif test -z \"$passes_sanitize_leak\" &&\n> > -\t     test_bool_env GIT_TEST_PASSING_SANITIZE_LEAK false\n> > +\telif test_bool_env GIT_TEST_PASSING_SANITIZE_LEAK false\n> >  \tthen\n> > -\t\tskip_all=\"skipping $this_test under GIT_TEST_PASSING_SANITIZE_LEAK=true\"\n> > -\t\ttest_done\n> > +\t\tif test -n \"$invert_exit_code\"\n> > +\t\tthen\n> > +\t\t\tBAIL_OUT \"cannot use --invert-exit-code under GIT_TEST_PASSING_SANITIZE_LEAK=true\"\n> > +\t\telif test -z \"$passes_sanitize_leak\"\n> > +\t\tthen\n> > +\t\t\tskip_all=\"skipping $this_test under GIT_TEST_PASSING_SANITIZE_LEAK=true\"\n> > +\t\t\ttest_done\n> > +\t\tfi\n> >  \tfi\n> \n> You can see at the top of the context that we will set\n> invert_exit_code=t ourselves, which will then complain here:\n> \n> >  \tif test_bool_env GIT_TEST_SANITIZE_LEAK_LOG false\n> >  \tthen\n> > +\t\tif test -n \"$invert_exit_code\"\n> > +\t\tthen\n> > +\t\t\tBAIL_OUT \"cannot use --invert-exit-code and GIT_TEST_SANITIZE_LEAK_LOG=true\"\n> > +\t\tfi\n> > +\n> >  \t\tif ! mkdir -p \"$TEST_RESULTS_SAN_DIR\"\n> >  \t\tthen\n> >  \t\t\tBAIL_OUT \"cannot create $TEST_RESULTS_SAN_DIR\"\n> \n> That varible-set in the earlier context is from running in \"check\" mode.\n> So:\n> \n>   make GIT_TEST_PASSING_SANITIZE_LEAK=check GIT_TEST_SANITIZE_LEAK_LOG=true\n> \n> will now always fail. But this is the main way you'd want to run it\n> (enabling the leak log catches more stuff, and the log-check function\n> you touch in patch 2 already covers check mode).\n> \n> So I think you'd have to hoist your check above the if/else for setting\n> up PASSING_SANITIZE_LEAK modes.\n\nArrg, sorry.  You're right.\n\nThis was part of the series by mistake.  Please, discard it.\n\nIn my tree, I have a previous commit with:\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 87cfea9e9a..46b8a76e9c 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -1556,7 +1556,6 @@ then\n                if test -z \"$passes_sanitize_leak\"\n                then\n                        say \"in GIT_TEST_PASSING_SANITIZE_LEAK=check mode, setting --invert-exit-code for TEST_PASSES_SANITIZE_LEAK != true\"\n-                       invert_exit_code=t\n                fi\n        elif test_bool_env GIT_TEST_PASSING_SANITIZE_LEAK false\n        then\n\nthat is part of an unsent attempt to make:\n\n  $ make SANITIZE=leak T=t7510-signed-commit.sh GIT_TEST_PASSING_SANITIZE_LEAK=check test\n \nnot to suggest, when GPG is missing, that t7510-signed-commit.sh is a\ncandidate to be marked as leak-free.  Which is distracting to me.\n\nHowever I was not satisfied with the solution and discarded it.  But\nunfortunately not entirely.  Sorry. \n\n> \n> -Peff\n"},{"id":"481864","messageId":"2890b210-c42f-41cf-e676-0b1c56310f73@gmail.com","threadId":"60217","inReplyTo":"20230912082742.GB1630538@coredump.intra.peff.net","subject":"Re: [PATCH 2/2] test-lib: fix GIT_TEST_SANITIZE_LEAK_LOG","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2023-09-15T00:28:15Z","receivedAt":"2023-09-15T00:28:21Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On 12-sep-2023 04:27:42, Jeff King wrote:\n> On Sun, Sep 10, 2023 at 01:09:52AM +0200, Rubén Justo wrote:\n> \n> > GIT_TEST_SANITIZE_LEAK_LOG=true with a test that leaks, will make the\n> > test return zero unintentionally:\n> > \n> >   $ git checkout v2.40.1\n> >   $ make SANITIZE=leak\n> >   $ make -C t GIT_TEST_SANITIZE_LEAK_LOG=true t3200-branch.sh\n> >   ...\n> >   With GIT_TEST_SANITIZE_LEAK_LOG=true our logs revealed a memory leak, exit non-zero!\n> >   # faked up failures as TODO & now exiting with 0 due to --invert-exit-code\n> > \n> > Let's use invert_exit_code only if needed.\n> \n> Hmm, OK. So we saw some actual test errors (maybe from leaks or maybe\n> not), but then we _also_ saw entries in the leak-log. So the inversion\n> cancels out, and we accidentally say everything is OK, which is wrong.\n> \n> I'm not quite sure of your fix, though. In the if-else chain you're\n> touching, we know going in that we found a leak in the log. And then we\n> cover these 5 cases:\n> \n>   1. if the test is marked as passing-leak\n>     a. if we saw no test failures, invert (and mention the leaking log)\n>     b. otherwise, do not invert (and mention the log)\n>   2. else if we are in \"check\" mode\n>     a. if we saw no test failures, do not invert (we do have a leak,\n>        which is equivalent to a test failure). Mention the log.\n>     b. otherwise, invert (to switch back to \"success\", since we are\n>        looking for leaks), but still mention the log.\n>   3. invert to trigger failure (and mention the log)\n> \n> And the problem is in (3). You switch it to trigger only if we have no\n> failures (fixing the inversion). But should we have the same a/b split\n> for this case? I.e.:\n> \n>   3a. if we saw no test failures, invert to cause a failure\n>   3b. we saw other failures; do not invert, but _do_ mention that the\n>       log found extra leaks\n> \n> In 3b we are explaining to the user what happened. Though maybe it is\n> not super important, because I think we'd have dumped the log contents\n> anyway?\n\nI think so too.  At that point we've already dumped the contents of the\n$TEST_RESULTS_SAN_FILE file.\n\nIMO, when $test_failure is zero (the \"if\" I'm touching), the message\nmakes sense not so much to say that a leak has been found, but rather\nbecause we're forcing the non-zero exit.\n\nBut when $test_failure is not zero, after we've already dumped the\nlog, maybe this is somewhat redundant:\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 87cfea9e9a..b160ae3f7a 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -1267,6 +1267,8 @@ check_test_results_san_file_ () {\n        then\n                say \"With GIT_TEST_SANITIZE_LEAK_LOG=true our logs revealed a memory leak, exit non-zero!\" &&\n                invert_exit_code=t\n+       else\n+               say \"With GIT_TEST_SANITIZE_LEAK_LOG=true our logs revealed a memory leak\"\n        fi\n }\n\nHowever, if you or anyone else thinks it adds value, I have no objection\nto re-roll with it.\n\n> Other than that, I think the patch is correct. I wondered when we ran\n> this \"check_test_results_san_file_\" code, but it is only at the end of\n> the script. So we are OK to make a definitive call on the zero/non-zero\n> count of failed tests.\n> \n> -Peff\n\nThank you for taking the time to review these series.\n"},{"id":"481870","messageId":"20230915112906.GA3125427@coredump.intra.peff.net","threadId":"60217","inReplyTo":"2890b210-c42f-41cf-e676-0b1c56310f73@gmail.com","subject":"Re: [PATCH 2/2] test-lib: fix GIT_TEST_SANITIZE_LEAK_LOG","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-09-15T11:29:06Z","receivedAt":"2023-09-15T11:29:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 15, 2023 at 02:28:15AM +0200, Rubén Justo wrote:\n\n> > And the problem is in (3). You switch it to trigger only if we have no\n> > failures (fixing the inversion). But should we have the same a/b split\n> > for this case? I.e.:\n> > \n> >   3a. if we saw no test failures, invert to cause a failure\n> >   3b. we saw other failures; do not invert, but _do_ mention that the\n> >       log found extra leaks\n> > \n> > In 3b we are explaining to the user what happened. Though maybe it is\n> > not super important, because I think we'd have dumped the log contents\n> > anyway?\n> \n> I think so too.  At that point we've already dumped the contents of the\n> $TEST_RESULTS_SAN_FILE file.\n> \n> IMO, when $test_failure is zero (the \"if\" I'm touching), the message\n> makes sense not so much to say that a leak has been found, but rather\n> because we're forcing the non-zero exit.\n> \n> But when $test_failure is not zero, after we've already dumped the\n> log, maybe this is somewhat redundant:\n> \n> diff --git a/t/test-lib.sh b/t/test-lib.sh\n> index 87cfea9e9a..b160ae3f7a 100644\n> --- a/t/test-lib.sh\n> +++ b/t/test-lib.sh\n> @@ -1267,6 +1267,8 @@ check_test_results_san_file_ () {\n>         then\n>                 say \"With GIT_TEST_SANITIZE_LEAK_LOG=true our logs revealed a memory leak, exit non-zero!\" &&\n>                 invert_exit_code=t\n> +       else\n> +               say \"With GIT_TEST_SANITIZE_LEAK_LOG=true our logs revealed a memory leak\"\n>         fi\n>  }\n> \n> However, if you or anyone else thinks it adds value, I have no objection\n> to re-roll with it.\n\nI'm on the fence. It is probably not a big deal, and my biggest issue is\njust that I had to walk through the explanation in my previous mail to\nconvince myself the change was not missing an important case.\n\nBut having done so, the main value in re-rolling would be preventing\nsomebody else from reading the code and having the same question. But\nthis discussion in the archive is probably sufficient.\n\n-Peff\n"},{"id":"481880","messageId":"xmqqcyyjfht2.fsf@gitster.g","threadId":"60217","inReplyTo":"2890b210-c42f-41cf-e676-0b1c56310f73@gmail.com","subject":"Re: [PATCH 2/2] test-lib: fix GIT_TEST_SANITIZE_LEAK_LOG","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-09-15T17:49:13Z","receivedAt":"2023-09-15T17:50:00Z","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>> And the problem is in (3). You switch it to trigger only if we have no\n>> failures (fixing the inversion). But should we have the same a/b split\n>> for this case? I.e.:\n>> \n>>   3a. if we saw no test failures, invert to cause a failure\n>>   3b. we saw other failures; do not invert, but _do_ mention that the\n>>       log found extra leaks\n>> \n>> In 3b we are explaining to the user what happened. Though maybe it is\n>> not super important, because I think we'd have dumped the log contents\n>> anyway?\n>\n> I think so too.  At that point we've already dumped the contents of the\n> $TEST_RESULTS_SAN_FILE file.\n> ...\n> However, if you or anyone else thinks it adds value, I have no objection\n> to re-roll with it.\n\nI do not know offhand if we need the code update to implement what\nPeff called \"maybe it is not super important\", but if we decide not\nto, at least it would help future developers to document the fact\nthat we were aware of the issue when the code was developed, and why\nwe decided not to address it (in other words, describe why we\ndecided it is not super important).\n\nThanks both for polishing the series and making it better.\n"},{"id":"481881","messageId":"xmqq5y4bfhpm.fsf@gitster.g","threadId":"60217","inReplyTo":"20230915112906.GA3125427@coredump.intra.peff.net","subject":"Re: [PATCH 2/2] test-lib: fix GIT_TEST_SANITIZE_LEAK_LOG","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-09-15T17:51:17Z","receivedAt":"2023-09-15T17:52:40Z","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> But having done so, the main value in re-rolling would be preventing\n> somebody else from reading the code and having the same question.\n\nIndeed.  It would be valuable to help future developers not to waste\ntime on wondering what we already know they may do so on.\n"},{"id":"481917","messageId":"20230916053257.GC13092@coredump.intra.peff.net","threadId":"60217","inReplyTo":"xmqq5y4bfhpm.fsf@gitster.g","subject":"Re: [PATCH 2/2] test-lib: fix GIT_TEST_SANITIZE_LEAK_LOG","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-09-16T05:32:57Z","receivedAt":"2023-09-16T05:33:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 15, 2023 at 10:51:17AM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > But having done so, the main value in re-rolling would be preventing\n> > somebody else from reading the code and having the same question.\n> \n> Indeed.  It would be valuable to help future developers not to waste\n> time on wondering what we already know they may do so on.\n\nI guess I was thinking \"maybe that is not a good reason to change what\nthe code does\" when I wrote my other email. But it is probably a good\nreason to make a note in the commit message. :)\n\n-Peff\n"},{"id":"482182","messageId":"54253e98-10d5-55ef-a3ac-1f1a8cfcdec9@gmail.com","threadId":"60217","inReplyTo":"68522960-edda-26d3-ddca-cee63f2d859e@gmail.com","subject":"[PATCH v2] test-lib: fix GIT_TEST_SANITIZE_LEAK_LOG","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2023-09-22T20:38:06Z","receivedAt":"2023-09-22T20:38:15Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"In the if-elses chain we have in \"check_test_results_san_file_\", we\nconsider three variables: $passes_sanitize_leak, $sanitize_leak_check\nand lastly, implicitly, GIT_TEST_SANITIZE_LEAK_LOG (always set to \"true\"\nat that point).\n\nFor the first two variables we have different considerations depending\non the value of $test_failure, which make sense.  However, for the\nthird, GIT_TEST_SANITIZE_LEAK_LOG, we don't;  regardless of\n$test_failure, we use \"invert_exit_code=t\" to produce a non-zero\nreturn value.\n\nThat assumes \"$test_failure = 0\" is always true at that point.  But it\nis not:\n\n   $ git checkout v2.40.1\n   $ make SANITIZE=leak\n   $ make -C t GIT_TEST_SANITIZE_LEAK_LOG=true t3200-branch.sh\n   ...\n   With GIT_TEST_SANITIZE_LEAK_LOG=true, our logs revealed a memory leak, exiting with a non-zero status!\n   # Simulated failures as TODO & now exiting with 0 due to --invert-exit-code\n\nWe need to use \"invert_exit_code=t\" only when \"$test_failure = 0\".\n\nProbably we have missed this because GIT_TEST_SANITIZE_LEAK_LOG is\ncommonly used in combination with GIT_TEST_PASSING_SANITIZE_LEAK=check,\nthe second variable noted above, for which we have, as already said,\ndifferent considerations depending on the value of $test_failure.\n\nHowever GIT_TEST_SANITIZE_LEAK_LOG has value used on its own, as it was\ndocumented in faececa53f (test-lib: have the \"check\" mode for\nSANITIZE=leak consider leak logs, 2022-07-28).\n\nLet's add the missing conditions in the if-elses chain to make it work\nas expected.\n\nNote that we're not resetting the value of \"invert_exit_code\" when\n$test_failure is non-zero.  Doing that is only needed when\nGIT_TEST_PASSING_SANITIZE_LEAK=check, the second variable noted above.\n\nHelped-by: Eric Sunshine <sunshine@sunshineco.com>\nHelped-by: Jeff King <peff@peff.net>\nSigned-off-by: Rubén Justo <rjusto@gmail.com>\n---\n\nChanges since v1:\n- dropped 1/1 as it was erroneously included\n- reworded the message to better capture the situation and the goal\n- show a message also when $test_failure is non-zero\n\n\nIt is probably worth revisiting the use of \"invert_exit_code=t\"\nin this SANITIZE_LEAK machinery.  But I'll stop here.\n\n---\n t/test-lib.sh | 5 ++++-\n 1 file changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 5ea5d1d62a..945ede50ac 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -1264,9 +1264,12 @@ check_test_results_san_file_ () {\n \tthen\n \t\tsay \"As TEST_PASSES_SANITIZE_LEAK=true isn't set the above leak is 'ok' with GIT_TEST_PASSING_SANITIZE_LEAK=check\" &&\n \t\tinvert_exit_code=t\n-\telse\n+\telif test \"$test_failure\" = 0\n+\tthen\n \t\tsay \"With GIT_TEST_SANITIZE_LEAK_LOG=true our logs revealed a memory leak, exit non-zero!\" &&\n \t\tinvert_exit_code=t\n+\telse\n+\t\tsay \"With GIT_TEST_SANITIZE_LEAK_LOG=true our logs revealed a memory leak...\"\n \tfi\n }\n \n-- \n2.34.1\n"},{"id":"482199","messageId":"20230923062415.GA1469941@coredump.intra.peff.net","threadId":"60217","inReplyTo":"54253e98-10d5-55ef-a3ac-1f1a8cfcdec9@gmail.com","subject":"Re: [PATCH v2] test-lib: fix GIT_TEST_SANITIZE_LEAK_LOG","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-09-23T06:24:15Z","receivedAt":"2023-09-23T06:24:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 22, 2023 at 10:38:06PM +0200, Rubén Justo wrote:\n\n> [...]\n> Let's add the missing conditions in the if-elses chain to make it work\n> as expected.\n\nThanks, I think this explains the situation better than the original.\n\n> +\telif test \"$test_failure\" = 0\n> +\tthen\n>  \t\tsay \"With GIT_TEST_SANITIZE_LEAK_LOG=true our logs revealed a memory leak, exit non-zero!\" &&\n>  \t\tinvert_exit_code=t\n> +\telse\n> +\t\tsay \"With GIT_TEST_SANITIZE_LEAK_LOG=true our logs revealed a memory leak...\"\n>  \tfi\n\nOK, so you did add in the \"else\" here. :) I am obviously fine with that,\nand the patch overall looks good to me.\n\n-Peff\n"},{"id":"482207","messageId":"f33cbdf5-9887-e9c4-504e-f7d96e630bc4@gmail.com","threadId":"60217","inReplyTo":"20230923062415.GA1469941@coredump.intra.peff.net","subject":"Re: [PATCH v2] test-lib: fix GIT_TEST_SANITIZE_LEAK_LOG","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2023-09-23T08:11:42Z","receivedAt":"2023-09-23T08:11:52Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On 23-sep-2023 02:24:15, Jeff King wrote:\n> On Fri, Sep 22, 2023 at 10:38:06PM +0200, Rubén Justo wrote:\n> \n> > [...]\n> > Let's add the missing conditions in the if-elses chain to make it work\n> > as expected.\n> \n> Thanks, I think this explains the situation better than the original.\n> \n> > +\telif test \"$test_failure\" = 0\n> > +\tthen\n> >  \t\tsay \"With GIT_TEST_SANITIZE_LEAK_LOG=true our logs revealed a memory leak, exit non-zero!\" &&\n> >  \t\tinvert_exit_code=t\n> > +\telse\n> > +\t\tsay \"With GIT_TEST_SANITIZE_LEAK_LOG=true our logs revealed a memory leak...\"\n> >  \tfi\n> \n> OK, so you did add in the \"else\" here. :)\n\nYes, easier to explain XD\n\nI had last minute doubts about the missing \"invert_exit_code=\", but\ndecided not to include it.  This way, \"--invert-exit-code\" works as\nexpected with \"failing\" tests.\n\nMaybe we can make \"--invert-exit-code\" work in the other cases as well.\n\nBut let's let the dust settle, before scratching that itch :)\n\n> I am obviously fine with that,\n> and the patch overall looks good to me.\n\nThank you for keeping an eye on this.\n\n> \n> -Peff\n"}]}