Re: [PATCH 6/6] test-lib: ignore leaks in the sanitizer's thread code

From: Patrick Steinhardt <ps@pks.im>
Date: Jan 6, 2025, 07:56 UTC
Message-ID: <Z3uMxScZGjBBTFUU@pks.im>
In-Reply-To: <20250103202645.GD3212696@coredump.intra.peff.net>

On Fri, Jan 03, 2025 at 03:26:45PM -0500, Jeff King wrote:

Show 22 quoted lines

> On Fri, Jan 03, 2025 at 01:05:48PM +0100, Patrick Steinhardt wrote:
> 
> > > diff --git a/t/test-lib.sh b/t/test-lib.sh
> > > index c9487d0805..d1f62adbf8 100644
> > > --- a/t/test-lib.sh
> > > +++ b/t/test-lib.sh
> > > @@ -1177,7 +1177,8 @@ check_test_results_san_file_empty_ () {
> > >  	! find "$TEST_RESULTS_SAN_DIR" \
> > >  		-type f \
> > >  		-name "$TEST_RESULTS_SAN_FILE_PFX.*" 2>/dev/null |
> > > -	xargs grep -q ^DEDUP_TOKEN
> > > +	xargs grep ^DEDUP_TOKEN |
> > > +	grep -qv sanitizer::GetThreadStackTopAndBottom
> > >  }
> > 
> > It would be nice to provide some more context here in the form of a
> > comment so that one doesn't have to blame the commit.
> 
> We can add that on top, but I'm not sure what it should say. Do you want
> something along the lines of "add false positives to ignore here..." or
> are an explanation of why we are ignoring this particular false
> positive?

The latter, ideally also with a reference to the upstream issue you have created. That makes it way easier to discover why this line exists and may prompt people to remove the line eventually if they discover that the issue has been fixed for a while.

It doesn't have to be a full paragraph, but a sentence or to go a long way sometimes. For more context people can still blame the commit message.

Patrick