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

[Patrick Steinhardt <ps@pks.im>]

On Fri, Jan 03, 2025 at 03:24:10PM -0500, Jeff King wrote:
> On Fri, Jan 03, 2025 at 01:05:45PM +0100, Patrick Steinhardt wrote:
> 
> > 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:

It should. Quoting find(1):

    If any invocation with the `+' form returns a non-zero value as exit
    status, then find returns a non-zero exit status.

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

Yup, I didn't mean to say that we can drop the negation, but that it
makes it easier to reason about what the negation applies to (the whole
pipe or just the find(1) command)).

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

Yup.

Patrick