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:

quoted

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