From: Junio C Hamano <gitster@pobox.com>
To: "Rubén Justo" <rjusto@gmail.com>
Cc: "Jeff King" <peff@peff.net>, "Git List" <git@vger.kernel.org>,
"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Subject: Re: [PATCH 2/2] test-lib: fix GIT_TEST_SANITIZE_LEAK_LOG
Date: Fri, 15 Sep 2023 10:49:13 -0700 [thread overview]
Message-ID: <xmqqcyyjfht2.fsf@gitster.g> (raw)
In-Reply-To: <2890b210-c42f-41cf-e676-0b1c56310f73@gmail.com> ("Rubén Justo"'s message of "Fri, 15 Sep 2023 02:28:15 +0200")
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.
next prev parent reply other threads:[~2023-09-15 17:50 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
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 [this message]
2023-09-22 20:38 ` [PATCH v2] " Rubén Justo
2023-09-23 6:24 ` Jeff King
2023-09-23 8:11 ` Rubén Justo
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqqcyyjfht2.fsf@gitster.g \
--to=gitster@pobox.com \
--cc=avarab@gmail.com \
--cc=git@vger.kernel.org \
--cc=peff@peff.net \
--cc=rjusto@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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 NNTP newsgroup(s).