Re: [PATCH 1/4] t0080: mark as leak-free

From: Jeff King <peff@peff.net>
Date: Jan 30, 2024, 05:53 UTC
Message-ID: <20240130055333.GB166761@coredump.intra.peff.net>
In-Reply-To: <xmqqa5onhjm4.fsf@gitster.g>

On Mon, Jan 29, 2024 at 02:15:15PM -0800, Junio C Hamano wrote:

Show 15 quoted lines

> > Let's mark it as leak-free to make sure it stays that way (and to reduce
> > noise when looking for other leak-free scripts after we fix some leaks).
> 
> For other tests in this series, that rationale is a very sensible
> thing, but does it apply to this one?
> 
> The point of the t-basic tests is to ensure the lightweight unit
> test framework that requires nothing from Git behaves (and keeps
> behaving) sensibly.  The point of running t[0-9][0-9][0-9][0-9]
> tests under leak sanitizer is to exercise production Git code to
> catch leaks in Git code.
> 
> So it is not quite clear if we even want to run this t0080 under
> leak sanitizer to begin with.  t0080 is a relatively tiny test, but
> do we even want to spend leak sanitizer cycles on it?  I dunno.

I think you are right that we do not particularly care about leaks in the t-basic code. That is also true of other test harness code (other unit-tests, but also stuff in t/helper). But IMHO it is less work to just keep that code leak-free than it is to try to distinguish between production and test code.

Right now, it is not that hard to simply leave the PASSES_SANITIZE_LEAK flag off of t0080, and then it won't be run in the leak-checking CI job. But I think the end-game of all of this leak-checking stuff is that eventually _everything_ will be leak-free, and we will discard the whole PASSES_SANITIZE_LEAK mechanism entirely. And in that end-game, it is simpler for everything, including t-basic, to just be leak-free and checked under the same regime.

Setting the flag now just makes sure we continue correctly on that path, rather than getting surprised near the end of the road that t-basic has some dumb leak. Plus it avoids the script popping up as a false positive when checking for scripts which can be marked.

-Peff