threads / patch / 54930

patchRe: [PATCH] p7519: allow running without watchman prereq

Subject: Re: [PATCH] p7519: allow running without watchman prereq

## tl;dr

2 messages between Jan 4, 2021 and Jan 5, 2021. Diffs are folded; open one to read it.

replies: 1people: 2as markdown or json

Taylor Blau· Jan 4, 2021, 21:35 UTC · lore

[PATCH] p7519: allow running without watchman prereq

p7519 measures the performance of the fsmonitor code. To do this, it uses the installed copy of Watchman. If Watchman isn't installed, a noop integration script is installed in its place.

When in the latter mode, it is expected that the script should not write a "last update token": in fact, it doesn't write anything at all since the script is blank.

Commit 33226af42b (t/perf/fsmonitor: improve error message if typoing hook name, 2020-10-26) made sure that running 'git update-index --fsmonitor' did not write anything to stderr, but this is not the case when using the empty Watchman script, since Git will complain that:

    $ which watchman
    watchman not found
    $ cat .git/hooks/fsmonitor-empty
    $ git -c core.fsmonitor=.git/hooks/fsmonitor-empty update-index --fsmonitor
    warning: Empty last update token.

Prior to 33226af42b, the output wasn't checked at all, which allowed this noop mode to work. But, 33226af42b breaks p7519 when running it without a 'watchman(1)' on your system.

Handle this by only checking that the stderr is empty only when running with a real watchman executable. Otherwise, assert that the error message is the expected one when running in the noop mode.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
Noticed while running the perf tests on v2.30.0 on a system that happens
to not have Watchman installed.

I could do without the 'else' block here, but it seems straightforward enough. Likewise, the "ensure no silent error" comment could certainly be removed (since 'test_must_be_empty error' makes what is happening quite clear), but the fix pursued here is minimal.

 t/perf/p7519-fsmonitor.sh | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)
Show changes to t/perf/p7519-fsmonitor.sh +6 −2
diff --git a/t/perf/p7519-fsmonitor.sh b/t/perf/p7519-fsmonitor.sh
index 9b43342806..1e20a184c7 100755
--- a/t/perf/p7519-fsmonitor.sh
+++ b/t/perf/p7519-fsmonitor.sh
@@ -129,7 +129,12 @@ setup_for_fsmonitor() {

 	git config core.fsmonitor "$INTEGRATION_SCRIPT" &&
 	git update-index --fsmonitor 2>error &&
-	test_must_be_empty error  # ensure no silent error
+	if test_have_prereq WATCHMAN
+	then
+		test_must_be_empty error  # ensure no silent error
+	else
+		grep "Empty last update token" error
+	fi
 }

 test_perf_w_drop_caches () {
--
2.29.2.533.g07db1f5344
Nipunn Koorapati· Jan 4, 2021, 23:56 UTC · re: Taylor Blau · lore
> I could do without the 'else' block here, but it seems straightforward
> enough. Likewise, the "ensure no silent error" comment could certainly
> be removed (since 'test_must_be_empty error' makes what is happening
> quite clear), but the fix pursued here is minimal.

This looks great to me. Thanks for fixing - and appreciate the else clause here for the reader.

--Nipunn

← back to recent threads