[PATCH 0/2] fix GIT_TEST_SANITIZE_LEAK_LOG=true

[PATCH 0/2] fix GIT_TEST_SANITIZE_LEAK_LOG=true

[Rubén Justo]

While using "make test" I noticed that invert_exit_code used by
GIT_TEST_SANITIZE_LEAK_LOG=true produces unexpected results.

   $ git checkout v2.40.1 
   $ make test SANITIZE=leak T=t3200-branch.sh # fails
   $ make test SANITIZE=leak GIT_TEST_SANITIZE_LEAK_LOG=true T=t3200-branch.sh # succeeds


Rubén Justo (2):
  test-lib: prevent misuses of --invert-exit-code
  test-lib: fix GIT_TEST_SANITIZE_LEAK_LOG

 t/test-lib.sh | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

-- 
2.40.1

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

[Rubén Justo]

GIT_TEST_PASSING_SANITIZE_LEAK=true and GIT_TEST_SANITIZE_LEAK_LOG=true
use internnlly the --invert-exit-code machinery.  Therefore if the user
wants to use --invert-exit-code in combination with them, the result
will be confusing.

For the same reason, we are already using BAIL_OUT if the user tries to
combine GIT_TEST_PASSING_SANITIZE_LEAK=check with --invert-exit-code.

Let's do the same for GIT_TEST_PASSING_SANITIZE_LEAK=true and
GIT_TEST_SANITIZE_LEAK_LOG=true.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 t/test-lib.sh | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 293caf0f20..88a34fba67 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -1557,15 +1557,25 @@ then
 			say "in GIT_TEST_PASSING_SANITIZE_LEAK=check mode, setting --invert-exit-code for TEST_PASSES_SANITIZE_LEAK != true"
 			invert_exit_code=t
 		fi
-	elif test -z "$passes_sanitize_leak" &&
-	     test_bool_env GIT_TEST_PASSING_SANITIZE_LEAK false
+	elif test_bool_env GIT_TEST_PASSING_SANITIZE_LEAK false
 	then
-		skip_all="skipping $this_test under GIT_TEST_PASSING_SANITIZE_LEAK=true"
-		test_done
+		if test -n "$invert_exit_code"
+		then
+			BAIL_OUT "cannot use --invert-exit-code under GIT_TEST_PASSING_SANITIZE_LEAK=true"
+		elif test -z "$passes_sanitize_leak"
+		then
+			skip_all="skipping $this_test under GIT_TEST_PASSING_SANITIZE_LEAK=true"
+			test_done
+		fi
 	fi
 
 	if test_bool_env GIT_TEST_SANITIZE_LEAK_LOG false
 	then
+		if test -n "$invert_exit_code"
+		then
+			BAIL_OUT "cannot use --invert-exit-code and GIT_TEST_SANITIZE_LEAK_LOG=true"
+		fi
+
 		if ! mkdir -p "$TEST_RESULTS_SAN_DIR"
 		then
 			BAIL_OUT "cannot create $TEST_RESULTS_SAN_DIR"
-- 
2.40.1

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

[Eric Sunshine]

On Sat, Sep 9, 2023 at 7:08 PM Rubén Justo <rjusto@gmail.com> wrote:
> GIT_TEST_PASSING_SANITIZE_LEAK=true and GIT_TEST_SANITIZE_LEAK_LOG=true
> use internnlly the --invert-exit-code machinery.  Therefore if the user

s/internnlly/internally/

> wants to use --invert-exit-code in combination with them, the result
> will be confusing.
>
> For the same reason, we are already using BAIL_OUT if the user tries to
> combine GIT_TEST_PASSING_SANITIZE_LEAK=check with --invert-exit-code.
>
> Let's do the same for GIT_TEST_PASSING_SANITIZE_LEAK=true and
> GIT_TEST_SANITIZE_LEAK_LOG=true.
>
> Signed-off-by: Rubén Justo <rjusto@gmail.com>

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

[Rubén Justo]

On 10/9/23 3:59, Eric Sunshine wrote:
> On Sat, Sep 9, 2023 at 7:08 PM Rubén Justo <rjusto@gmail.com> wrote:
>> GIT_TEST_PASSING_SANITIZE_LEAK=true and GIT_TEST_SANITIZE_LEAK_LOG=true
>> use internnlly the --invert-exit-code machinery.  Therefore if the user
> 
> s/internnlly/internally/

Thanks!

> 
>> wants to use --invert-exit-code in combination with them, the result
>> will be confusing.
>>
>> For the same reason, we are already using BAIL_OUT if the user tries to
>> combine GIT_TEST_PASSING_SANITIZE_LEAK=check with --invert-exit-code.
>>
>> Let's do the same for GIT_TEST_PASSING_SANITIZE_LEAK=true and
>> GIT_TEST_SANITIZE_LEAK_LOG=true.
>>
>> Signed-off-by: Rubén Justo <rjusto@gmail.com>

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

[Jeff King]

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

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

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

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

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

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

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

  make GIT_TEST_PASSING_SANITIZE_LEAK=check GIT_TEST_SANITIZE_LEAK_LOG=true

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

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

-Peff

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

[Rubén Justo]

On 12-sep-2023 04:35:28, Jeff King wrote:
> On Sun, Sep 10, 2023 at 01:08:11AM +0200, Rubén Justo wrote:
> 
> > GIT_TEST_PASSING_SANITIZE_LEAK=true and GIT_TEST_SANITIZE_LEAK_LOG=true
> > use internnlly the --invert-exit-code machinery.  Therefore if the user
> > wants to use --invert-exit-code in combination with them, the result
> > will be confusing.
> > 
> > For the same reason, we are already using BAIL_OUT if the user tries to
> > combine GIT_TEST_PASSING_SANITIZE_LEAK=check with --invert-exit-code.
> > 
> > Let's do the same for GIT_TEST_PASSING_SANITIZE_LEAK=true and
> > GIT_TEST_SANITIZE_LEAK_LOG=true.
> 
> OK, so we are trying to find a case where the user is triggering
> --invert-exit-code themselves and complaining. But in the code...
> 
> > @@ -1557,15 +1557,25 @@ then
> >  			say "in GIT_TEST_PASSING_SANITIZE_LEAK=check mode, setting --invert-exit-code for TEST_PASSES_SANITIZE_LEAK != true"
> >  			invert_exit_code=t
> >  		fi
> > -	elif test -z "$passes_sanitize_leak" &&
> > -	     test_bool_env GIT_TEST_PASSING_SANITIZE_LEAK false
> > +	elif test_bool_env GIT_TEST_PASSING_SANITIZE_LEAK false
> >  	then
> > -		skip_all="skipping $this_test under GIT_TEST_PASSING_SANITIZE_LEAK=true"
> > -		test_done
> > +		if test -n "$invert_exit_code"
> > +		then
> > +			BAIL_OUT "cannot use --invert-exit-code under GIT_TEST_PASSING_SANITIZE_LEAK=true"
> > +		elif test -z "$passes_sanitize_leak"
> > +		then
> > +			skip_all="skipping $this_test under GIT_TEST_PASSING_SANITIZE_LEAK=true"
> > +			test_done
> > +		fi
> >  	fi
> 
> You can see at the top of the context that we will set
> invert_exit_code=t ourselves, which will then complain here:
> 
> >  	if test_bool_env GIT_TEST_SANITIZE_LEAK_LOG false
> >  	then
> > +		if test -n "$invert_exit_code"
> > +		then
> > +			BAIL_OUT "cannot use --invert-exit-code and GIT_TEST_SANITIZE_LEAK_LOG=true"
> > +		fi
> > +
> >  		if ! mkdir -p "$TEST_RESULTS_SAN_DIR"
> >  		then
> >  			BAIL_OUT "cannot create $TEST_RESULTS_SAN_DIR"
> 
> That varible-set in the earlier context is from running in "check" mode.
> So:
> 
>   make GIT_TEST_PASSING_SANITIZE_LEAK=check GIT_TEST_SANITIZE_LEAK_LOG=true
> 
> will now always fail. But this is the main way you'd want to run it
> (enabling the leak log catches more stuff, and the log-check function
> you touch in patch 2 already covers check mode).
> 
> So I think you'd have to hoist your check above the if/else for setting
> up PASSING_SANITIZE_LEAK modes.

Arrg, sorry.  You're right.

This was part of the series by mistake.  Please, discard it.

In my tree, I have a previous commit with:

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 87cfea9e9a..46b8a76e9c 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -1556,7 +1556,6 @@ then
                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_bool_env GIT_TEST_PASSING_SANITIZE_LEAK false
        then

that is part of an unsent attempt to make:

  $ make SANITIZE=leak T=t7510-signed-commit.sh GIT_TEST_PASSING_SANITIZE_LEAK=check test
 
not to suggest, when GPG is missing, that t7510-signed-commit.sh is a
candidate to be marked as leak-free.  Which is distracting to me.

However I was not satisfied with the solution and discarded it.  But
unfortunately not entirely.  Sorry. 

> 
> -Peff

[PATCH 2/2] test-lib: fix GIT_TEST_SANITIZE_LEAK_LOG

[Rubén Justo]

GIT_TEST_SANITIZE_LEAK_LOG=true with a test that leaks, will make the
test return zero unintentionally:

  $ git checkout v2.40.1
  $ make SANITIZE=leak
  $ make -C t GIT_TEST_SANITIZE_LEAK_LOG=true t3200-branch.sh
  ...
  With GIT_TEST_SANITIZE_LEAK_LOG=true our logs revealed a memory leak, exit non-zero!
  # faked up failures as TODO & now exiting with 0 due to --invert-exit-code

Let's use invert_exit_code only if needed.

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

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 88a34fba67..87cfea9e9a 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -1263,7 +1263,8 @@ check_test_results_san_file_ () {
 	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
-	else
+	elif test "$test_failure" = 0
+	then
 		say "With GIT_TEST_SANITIZE_LEAK_LOG=true our logs revealed a memory leak, exit non-zero!" &&
 		invert_exit_code=t
 	fi
-- 
2.40.1

Re: [PATCH 2/2] test-lib: fix GIT_TEST_SANITIZE_LEAK_LOG

[Jeff King]

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

> GIT_TEST_SANITIZE_LEAK_LOG=true with a test that leaks, will make the
> test return zero unintentionally:
> 
>   $ git checkout v2.40.1
>   $ make SANITIZE=leak
>   $ make -C t GIT_TEST_SANITIZE_LEAK_LOG=true t3200-branch.sh
>   ...
>   With GIT_TEST_SANITIZE_LEAK_LOG=true our logs revealed a memory leak, exit non-zero!
>   # faked up failures as TODO & now exiting with 0 due to --invert-exit-code
> 
> Let's use invert_exit_code only if needed.

Hmm, OK. So we saw some actual test errors (maybe from leaks or maybe
not), but then we _also_ saw entries in the leak-log. So the inversion
cancels out, and we accidentally say everything is OK, which is wrong.

I'm not quite sure of your fix, though. In the if-else chain you're
touching, we know going in that we found a leak in the log. And then we
cover these 5 cases:

  1. if the test is marked as passing-leak
    a. if we saw no test failures, invert (and mention the leaking log)
    b. otherwise, do not invert (and mention the log)
  2. else if we are in "check" mode
    a. if we saw no test failures, do not invert (we do have a leak,
       which is equivalent to a test failure). Mention the log.
    b. otherwise, invert (to switch back to "success", since we are
       looking for leaks), but still mention the log.
  3. invert to trigger failure (and mention the log)

And the problem is in (3). You switch it to trigger only if we have no
failures (fixing the inversion). But should we have the same a/b split
for this case? I.e.:

  3a. if we saw no test failures, invert to cause a failure
  3b. we saw other failures; do not invert, but _do_ mention that the
      log found extra leaks

In 3b we are explaining to the user what happened. Though maybe it is
not super important, because I think we'd have dumped the log contents
anyway?

Other than that, I think the patch is correct. I wondered when we ran
this "check_test_results_san_file_" code, but it is only at the end of
the script. So we are OK to make a definitive call on the zero/non-zero
count of failed tests.

-Peff

Re: [PATCH 2/2] test-lib: fix GIT_TEST_SANITIZE_LEAK_LOG

[Rubén Justo]

On 12-sep-2023 04:27:42, Jeff King wrote:
> On Sun, Sep 10, 2023 at 01:09:52AM +0200, Rubén Justo wrote:
> 
> > GIT_TEST_SANITIZE_LEAK_LOG=true with a test that leaks, will make the
> > test return zero unintentionally:
> > 
> >   $ git checkout v2.40.1
> >   $ make SANITIZE=leak
> >   $ make -C t GIT_TEST_SANITIZE_LEAK_LOG=true t3200-branch.sh
> >   ...
> >   With GIT_TEST_SANITIZE_LEAK_LOG=true our logs revealed a memory leak, exit non-zero!
> >   # faked up failures as TODO & now exiting with 0 due to --invert-exit-code
> > 
> > Let's use invert_exit_code only if needed.
> 
> Hmm, OK. So we saw some actual test errors (maybe from leaks or maybe
> not), but then we _also_ saw entries in the leak-log. So the inversion
> cancels out, and we accidentally say everything is OK, which is wrong.
> 
> I'm not quite sure of your fix, though. In the if-else chain you're
> touching, we know going in that we found a leak in the log. And then we
> cover these 5 cases:
> 
>   1. if the test is marked as passing-leak
>     a. if we saw no test failures, invert (and mention the leaking log)
>     b. otherwise, do not invert (and mention the log)
>   2. else if we are in "check" mode
>     a. if we saw no test failures, do not invert (we do have a leak,
>        which is equivalent to a test failure). Mention the log.
>     b. otherwise, invert (to switch back to "success", since we are
>        looking for leaks), but still mention the log.
>   3. invert to trigger failure (and mention the log)
> 
> And the problem is in (3). You switch it to trigger only if we have no
> failures (fixing the inversion). But should we have the same a/b split
> for this case? I.e.:
> 
>   3a. if we saw no test failures, invert to cause a failure
>   3b. we saw other failures; do not invert, but _do_ mention that the
>       log found extra leaks
> 
> In 3b we are explaining to the user what happened. Though maybe it is
> not super important, because I think we'd have dumped the log contents
> anyway?

I think so too.  At that point we've already dumped the contents of the
$TEST_RESULTS_SAN_FILE file.

IMO, when $test_failure is zero (the "if" I'm touching), the message
makes sense not so much to say that a leak has been found, but rather
because we're forcing the non-zero exit.

But when $test_failure is not zero, after we've already dumped the
log, maybe this is somewhat redundant:

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 87cfea9e9a..b160ae3f7a 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -1267,6 +1267,8 @@ check_test_results_san_file_ () {
        then
                say "With GIT_TEST_SANITIZE_LEAK_LOG=true 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"
        fi
 }

However, if you or anyone else thinks it adds value, I have no objection
to re-roll with it.

> Other than that, I think the patch is correct. I wondered when we ran
> this "check_test_results_san_file_" code, but it is only at the end of
> the script. So we are OK to make a definitive call on the zero/non-zero
> count of failed tests.
> 
> -Peff

Thank you for taking the time to review these series.

Re: [PATCH 2/2] test-lib: fix GIT_TEST_SANITIZE_LEAK_LOG

[Jeff King]

On Fri, Sep 15, 2023 at 02:28:15AM +0200, Rubén Justo wrote:

> > And the problem is in (3). You switch it to trigger only if we have no
> > failures (fixing the inversion). But should we have the same a/b split
> > for this case? I.e.:
> > 
> >   3a. if we saw no test failures, invert to cause a failure
> >   3b. we saw other failures; do not invert, but _do_ mention that the
> >       log found extra leaks
> > 
> > In 3b we are explaining to the user what happened. Though maybe it is
> > not super important, because I think we'd have dumped the log contents
> > anyway?
> 
> I think so too.  At that point we've already dumped the contents of the
> $TEST_RESULTS_SAN_FILE file.
> 
> IMO, when $test_failure is zero (the "if" I'm touching), the message
> makes sense not so much to say that a leak has been found, but rather
> because we're forcing the non-zero exit.
> 
> But when $test_failure is not zero, after we've already dumped the
> log, maybe this is somewhat redundant:
> 
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 87cfea9e9a..b160ae3f7a 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -1267,6 +1267,8 @@ check_test_results_san_file_ () {
>         then
>                 say "With GIT_TEST_SANITIZE_LEAK_LOG=true 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"
>         fi
>  }
> 
> However, if you or anyone else thinks it adds value, I have no objection
> to re-roll with it.

I'm on the fence. It is probably not a big deal, and my biggest issue is
just that I had to walk through the explanation in my previous mail to
convince myself the change was not missing an important case.

But having done so, the main value in re-rolling would be preventing
somebody else from reading the code and having the same question. But
this discussion in the archive is probably sufficient.

-Peff

Re: [PATCH 2/2] test-lib: fix GIT_TEST_SANITIZE_LEAK_LOG

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> But having done so, the main value in re-rolling would be preventing
> somebody else from reading the code and having the same question.

Indeed.  It would be valuable to help future developers not to waste
time on wondering what we already know they may do so on.

Re: [PATCH 2/2] test-lib: fix GIT_TEST_SANITIZE_LEAK_LOG

[Jeff King]

On Fri, Sep 15, 2023 at 10:51:17AM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > But having done so, the main value in re-rolling would be preventing
> > somebody else from reading the code and having the same question.
> 
> Indeed.  It would be valuable to help future developers not to waste
> time on wondering what we already know they may do so on.

I guess I was thinking "maybe that is not a good reason to change what
the code does" when I wrote my other email. But it is probably a good
reason to make a note in the commit message. :)

-Peff

Re: [PATCH 2/2] test-lib: fix GIT_TEST_SANITIZE_LEAK_LOG

[Junio C Hamano]

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

>> And the problem is in (3). You switch it to trigger only if we have no
>> failures (fixing the inversion). But should we have the same a/b split
>> for this case? I.e.:
>> 
>>   3a. if we saw no test failures, invert to cause a failure
>>   3b. we saw other failures; do not invert, but _do_ mention that the
>>       log found extra leaks
>> 
>> In 3b we are explaining to the user what happened. Though maybe it is
>> not super important, because I think we'd have dumped the log contents
>> anyway?
>
> I think so too.  At that point we've already dumped the contents of the
> $TEST_RESULTS_SAN_FILE file.
> ...
> However, if you or anyone else thinks it adds value, I have no objection
> to re-roll with it.

I do not know offhand if we need the code update to implement what
Peff called "maybe it is not super important", but if we decide not
to, at least it would help future developers to document the fact
that we were aware of the issue when the code was developed, and why
we decided not to address it (in other words, describe why we
decided it is not super important).

Thanks both for polishing the series and making it better.

[PATCH v2] test-lib: fix GIT_TEST_SANITIZE_LEAK_LOG

[Rubén Justo]

In the if-elses chain we have in "check_test_results_san_file_", we
consider three variables: $passes_sanitize_leak, $sanitize_leak_check
and lastly, implicitly, GIT_TEST_SANITIZE_LEAK_LOG (always set to "true"
at that point).

For the first two variables we have different considerations depending
on the value of $test_failure, which make sense.  However, for the
third, GIT_TEST_SANITIZE_LEAK_LOG, we don't;  regardless of
$test_failure, we use "invert_exit_code=t" to produce a non-zero
return value.

That assumes "$test_failure = 0" is always true at that point.  But it
is not:

   $ git checkout v2.40.1
   $ make SANITIZE=leak
   $ make -C t GIT_TEST_SANITIZE_LEAK_LOG=true t3200-branch.sh
   ...
   With GIT_TEST_SANITIZE_LEAK_LOG=true, our logs revealed a memory leak, exiting with a non-zero status!
   # Simulated failures as TODO & now exiting with 0 due to --invert-exit-code

We need to use "invert_exit_code=t" only when "$test_failure = 0".

Probably we have missed this because GIT_TEST_SANITIZE_LEAK_LOG is
commonly used in combination with GIT_TEST_PASSING_SANITIZE_LEAK=check,
the second variable noted above, for which we have, as already said,
different considerations depending on the value of $test_failure.

However GIT_TEST_SANITIZE_LEAK_LOG has value used on its own, as it was
documented in faececa53f (test-lib: have the "check" mode for
SANITIZE=leak consider leak logs, 2022-07-28).

Let's add the missing conditions in the if-elses chain to make it work
as expected.

Note that we're not resetting the value of "invert_exit_code" when
$test_failure is non-zero.  Doing that is only needed when
GIT_TEST_PASSING_SANITIZE_LEAK=check, the second variable noted above.

Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Rubén Justo <rjusto@gmail.com>
---

Changes since v1:
- dropped 1/1 as it was erroneously included
- reworded the message to better capture the situation and the goal
- show a message also when $test_failure is non-zero


It is probably worth revisiting the use of "invert_exit_code=t"
in this SANITIZE_LEAK machinery.  But I'll stop here.

---
 t/test-lib.sh | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 5ea5d1d62a..945ede50ac 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -1264,9 +1264,12 @@ check_test_results_san_file_ () {
 	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
-	else
+	elif test "$test_failure" = 0
+	then
 		say "With GIT_TEST_SANITIZE_LEAK_LOG=true 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..."
 	fi
 }
 
-- 
2.34.1

Re: [PATCH v2] test-lib: fix GIT_TEST_SANITIZE_LEAK_LOG

[Jeff King]

On Fri, Sep 22, 2023 at 10:38:06PM +0200, Rubén Justo wrote:

> [...]
> Let's add the missing conditions in the if-elses chain to make it work
> as expected.

Thanks, I think this explains the situation better than the original.

> +	elif test "$test_failure" = 0
> +	then
>  		say "With GIT_TEST_SANITIZE_LEAK_LOG=true 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..."
>  	fi

OK, so you did add in the "else" here. :) I am obviously fine with that,
and the patch overall looks good to me.

-Peff

Re: [PATCH v2] test-lib: fix GIT_TEST_SANITIZE_LEAK_LOG

[Rubén Justo]

On 23-sep-2023 02:24:15, Jeff King wrote:
> On Fri, Sep 22, 2023 at 10:38:06PM +0200, Rubén Justo wrote:
> 
> > [...]
> > Let's add the missing conditions in the if-elses chain to make it work
> > as expected.
> 
> Thanks, I think this explains the situation better than the original.
> 
> > +	elif test "$test_failure" = 0
> > +	then
> >  		say "With GIT_TEST_SANITIZE_LEAK_LOG=true 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..."
> >  	fi
> 
> OK, so you did add in the "else" here. :)

Yes, easier to explain XD

I had last minute doubts about the missing "invert_exit_code=", but
decided not to include it.  This way, "--invert-exit-code" works as
expected with "failing" tests.

Maybe we can make "--invert-exit-code" work in the other cases as well.

But let's let the dust settle, before scratching that itch :)

> I am obviously fine with that,
> and the patch overall looks good to me.

Thank you for keeping an eye on this.

> 
> -Peff