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