Re: [PATCH 4/6] test-lib: simplify leak-log checking

From: Jeff King <peff@peff.net>
Date: Jan 3, 2025, 20:24 UTC
Message-ID: <20250103202410.GC3212696@coredump.intra.peff.net>
In-Reply-To: <Z3fSmSYoWs-a6afW@pks.im>

On Fri, Jan 03, 2025 at 01:05:45PM +0100, Patrick Steinhardt wrote:

Show 19 quoted lines

> On Wed, Jan 01, 2025 at 03:17:21PM -0500, Jeff King wrote:
> > @@ -1181,8 +1170,14 @@ test_atexit_handler () {
> >  }
> >  
> >  check_test_results_san_file_empty_ () {
> > -	test -z "$TEST_RESULTS_SAN_FILE" ||
> > -	test "$(nr_san_dir_leaks_)" = 0
> > +	test -z "$TEST_RESULTS_SAN_FILE" && return 0
> > +
> > +	# stderr piped to /dev/null because the directory may have
> > +	# been "rmdir"'d already.
> > +	! find "$TEST_RESULTS_SAN_DIR" \
> > +		-type f \
> > +		-name "$TEST_RESULTS_SAN_FILE_PFX.*" 2>/dev/null |
> > +	xargs grep -qv "Unable to get registers from thread"
> 
> Can't we use `-exec grep -qv "Unable to get registers from thread" {}
> \+` instead of using xargs? Or is that unportable? Might make it a bit
> easier to reason about the `!` in the presence of a pipe.

I don't think that saves us from negating, though. The "grep" will tell us if it matched any "real" lines, but we want to report that we found no real lines.

Plus I don't think "find" propagates the exit code from -exec anyway. I think you can check the exit status with more find logic, so you'd then use a conditional -print for each file like:

  find ... \
    -exec grep -qv "Unable to get registers from thread" \{} \; \
    -print

and you have to check whether the output is empty. The easiest way to do that is with another grep! Which also needs negated. ;)

I think if we really want to drop the negation, we'd be best to flip the function's return, like:

  have_leaks() {
	# not leak-checking
	test -z "$TEST_RESULTS_SAN_FILE" && return 1

	find "$TEST_RESULTS_SAN_DIR" \
		-type f \
		-name "$TEST_RESULTS_SAN_FILE_PFX.*" 2>/dev/null |
	xargs grep ^DEDUP_TOKEN |
	grep -qv sanitizer::GetThreadStackTopAndBottom
  }

And then you could switch the initial "grep" to -exec if you want, but there's no negation to get rid of, so it is only a preference of -exec versus xargs.

-Peff