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:

quoted

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:

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