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

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 15, 2023, 17:49 UTC
Message-ID: <xmqqcyyjfht2.fsf@gitster.g>
In-Reply-To: <2890b210-c42f-41cf-e676-0b1c56310f73@gmail.com>

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

Show 17 quoted lines

>> 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.