* [PATCH 0/2] fix GIT_TEST_SANITIZE_LEAK_LOG=true
@ 2023-09-09 23:03 Rubén Justo
2023-09-09 23:08 ` [PATCH 1/2] test-lib: prevent misuses of --invert-exit-code Rubén Justo
2023-09-09 23:09 ` [PATCH 2/2] test-lib: fix GIT_TEST_SANITIZE_LEAK_LOG Rubén Justo
0 siblings, 2 replies; 13+ messages in thread
From: Rubén Justo @ 2023-09-09 23:03 UTC (permalink / raw)
To: Git List; +Cc: Ævar Arnfjörð Bjarmason
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 1/2] test-lib: prevent misuses of --invert-exit-code
2023-09-09 23:03 [PATCH 0/2] fix GIT_TEST_SANITIZE_LEAK_LOG=true Rubén Justo
@ 2023-09-09 23:08 ` Rubén Justo
2023-09-10 1:59 ` Eric Sunshine
2023-09-12 8:35 ` Jeff King
2023-09-09 23:09 ` [PATCH 2/2] test-lib: fix GIT_TEST_SANITIZE_LEAK_LOG Rubén Justo
1 sibling, 2 replies; 13+ messages in thread
From: Rubén Justo @ 2023-09-09 23:08 UTC (permalink / raw)
To: Git List; +Cc: Ævar Arnfjörð Bjarmason
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
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 2/2] test-lib: fix GIT_TEST_SANITIZE_LEAK_LOG
2023-09-09 23:03 [PATCH 0/2] fix GIT_TEST_SANITIZE_LEAK_LOG=true Rubén Justo
2023-09-09 23:08 ` [PATCH 1/2] test-lib: prevent misuses of --invert-exit-code Rubén Justo
@ 2023-09-09 23:09 ` Rubén Justo
2023-09-12 8:27 ` Jeff King
1 sibling, 1 reply; 13+ messages in thread
From: Rubén Justo @ 2023-09-09 23:09 UTC (permalink / raw)
To: Git List; +Cc: Ævar Arnfjörð Bjarmason
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
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] test-lib: prevent misuses of --invert-exit-code
2023-09-09 23:08 ` [PATCH 1/2] test-lib: prevent misuses of --invert-exit-code Rubén Justo
@ 2023-09-10 1:59 ` Eric Sunshine
2023-09-10 22:58 ` Rubén Justo
2023-09-12 8:35 ` Jeff King
1 sibling, 1 reply; 13+ messages in thread
From: Eric Sunshine @ 2023-09-10 1:59 UTC (permalink / raw)
To: Rubén Justo; +Cc: Git List, Ævar Arnfjörð Bjarmason
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>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] test-lib: prevent misuses of --invert-exit-code
2023-09-10 1:59 ` Eric Sunshine
@ 2023-09-10 22:58 ` Rubén Justo
0 siblings, 0 replies; 13+ messages in thread
From: Rubén Justo @ 2023-09-10 22:58 UTC (permalink / raw)
To: Eric Sunshine; +Cc: Git List, Ævar Arnfjörð Bjarmason
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>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] test-lib: fix GIT_TEST_SANITIZE_LEAK_LOG
2023-09-09 23:09 ` [PATCH 2/2] test-lib: fix GIT_TEST_SANITIZE_LEAK_LOG Rubén Justo
@ 2023-09-12 8:27 ` Jeff King
2023-09-15 0:28 ` Rubén Justo
0 siblings, 1 reply; 13+ messages in thread
From: Jeff King @ 2023-09-12 8:27 UTC (permalink / raw)
To: Rubén Justo; +Cc: Git List, Ævar Arnfjörð Bjarmason
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] test-lib: prevent misuses of --invert-exit-code
2023-09-09 23:08 ` [PATCH 1/2] test-lib: prevent misuses of --invert-exit-code Rubén Justo
2023-09-10 1:59 ` Eric Sunshine
@ 2023-09-12 8:35 ` Jeff King
2023-09-15 0:10 ` Rubén Justo
1 sibling, 1 reply; 13+ messages in thread
From: Jeff King @ 2023-09-12 8:35 UTC (permalink / raw)
To: Rubén Justo; +Cc: Git List, Ævar Arnfjörð Bjarmason
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] test-lib: prevent misuses of --invert-exit-code
2023-09-12 8:35 ` Jeff King
@ 2023-09-15 0:10 ` Rubén Justo
0 siblings, 0 replies; 13+ messages in thread
From: Rubén Justo @ 2023-09-15 0:10 UTC (permalink / raw)
To: Jeff King; +Cc: Git List, Ævar Arnfjörð Bjarmason
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
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] test-lib: fix GIT_TEST_SANITIZE_LEAK_LOG
2023-09-12 8:27 ` Jeff King
@ 2023-09-15 0:28 ` Rubén Justo
2023-09-15 11:29 ` Jeff King
2023-09-15 17:49 ` Junio C Hamano
0 siblings, 2 replies; 13+ messages in thread
From: Rubén Justo @ 2023-09-15 0:28 UTC (permalink / raw)
To: Jeff King; +Cc: Git List, Ævar Arnfjörð Bjarmason
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.
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] test-lib: fix GIT_TEST_SANITIZE_LEAK_LOG
2023-09-15 0:28 ` Rubén Justo
@ 2023-09-15 11:29 ` Jeff King
2023-09-15 17:51 ` Junio C Hamano
2023-09-15 17:49 ` Junio C Hamano
1 sibling, 1 reply; 13+ messages in thread
From: Jeff King @ 2023-09-15 11:29 UTC (permalink / raw)
To: Rubén Justo; +Cc: Git List, Ævar Arnfjörð Bjarmason
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] test-lib: fix GIT_TEST_SANITIZE_LEAK_LOG
2023-09-15 0:28 ` Rubén Justo
2023-09-15 11:29 ` Jeff King
@ 2023-09-15 17:49 ` Junio C Hamano
1 sibling, 0 replies; 13+ messages in thread
From: Junio C Hamano @ 2023-09-15 17:49 UTC (permalink / raw)
To: Rubén Justo
Cc: Jeff King, Git List, Ævar Arnfjörð Bjarmason
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.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] test-lib: fix GIT_TEST_SANITIZE_LEAK_LOG
2023-09-15 11:29 ` Jeff King
@ 2023-09-15 17:51 ` Junio C Hamano
2023-09-16 5:32 ` Jeff King
0 siblings, 1 reply; 13+ messages in thread
From: Junio C Hamano @ 2023-09-15 17:51 UTC (permalink / raw)
To: Jeff King
Cc: Rubén Justo, Git List, Ævar Arnfjörð Bjarmason
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.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] test-lib: fix GIT_TEST_SANITIZE_LEAK_LOG
2023-09-15 17:51 ` Junio C Hamano
@ 2023-09-16 5:32 ` Jeff King
0 siblings, 0 replies; 13+ messages in thread
From: Jeff King @ 2023-09-16 5:32 UTC (permalink / raw)
To: Junio C Hamano
Cc: Rubén Justo, Git List, Ævar Arnfjörð Bjarmason
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
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2023-09-16 5:33 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-09-09 23:03 [PATCH 0/2] fix GIT_TEST_SANITIZE_LEAK_LOG=true Rubén Justo
2023-09-09 23:08 ` [PATCH 1/2] test-lib: prevent misuses of --invert-exit-code Rubén Justo
2023-09-10 1:59 ` Eric Sunshine
2023-09-10 22:58 ` Rubén Justo
2023-09-12 8:35 ` Jeff King
2023-09-15 0:10 ` Rubén Justo
2023-09-09 23:09 ` [PATCH 2/2] test-lib: fix GIT_TEST_SANITIZE_LEAK_LOG Rubén Justo
2023-09-12 8:27 ` Jeff King
2023-09-15 0:28 ` Rubén Justo
2023-09-15 11:29 ` Jeff King
2023-09-15 17:51 ` Junio C Hamano
2023-09-16 5:32 ` Jeff King
2023-09-15 17:49 ` Junio C Hamano
Code repositories for project(s) associated with this public inbox
https://80x24.org/pub/scm/git/git.git/
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).