Re: [PATCH 22/23] p7519: add fsmonitor--daemon

[Derrick Stolee <stolee@gmail.com>]

On 4/1/2021 11:41 AM, Jeff Hostetler via GitGitGadget wrote:
> From: Jeff Hostetler <jeffhost@microsoft.com>
> 
> Repeat all of the fsmonitor perf tests using `git fsmonitor--daemon` and
> the "Simple IPC" interface.

It would be nice to see some numbers for how this test performs
on some standard Git repositories across Windows and macOS.

> Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
> ---
>  t/perf/p7519-fsmonitor.sh | 37 +++++++++++++++++++++++++++++++++++--
>  1 file changed, 35 insertions(+), 2 deletions(-)
> 
> diff --git a/t/perf/p7519-fsmonitor.sh b/t/perf/p7519-fsmonitor.sh
> index 5eb5044a103c..2d018bc7d589 100755
> --- a/t/perf/p7519-fsmonitor.sh
> +++ b/t/perf/p7519-fsmonitor.sh
> @@ -24,7 +24,8 @@ test_description="Test core.fsmonitor"
>  # GIT_PERF_7519_SPLIT_INDEX: used to configure core.splitIndex
>  # GIT_PERF_7519_FSMONITOR: used to configure core.fsMonitor. May be an
>  #   absolute path to an integration. May be a space delimited list of
> -#   absolute paths to integrations.
> +#   absolute paths to integrations.  (This hook or list of hooks does not
> +#   include the built-in fsmonitor--daemon.)
>  #
>  # The big win for using fsmonitor is the elimination of the need to scan the
>  # working directory looking for changed and untracked files. If the file
> @@ -135,10 +136,16 @@ test_expect_success "one time repo setup" '
>  
>  setup_for_fsmonitor() {
>  	# set INTEGRATION_SCRIPT depending on the environment
> -	if test -n "$INTEGRATION_PATH"
> +	if test -n "$USE_FSMONITOR_DAEMON"
>  	then
> +		git config core.useBuiltinFSMonitor true &&
> +		INTEGRATION_SCRIPT=false
> +	elif test -n "$INTEGRATION_PATH"
> +	then
> +		git config core.useBuiltinFSMonitor false &&
>  		INTEGRATION_SCRIPT="$INTEGRATION_PATH"
>  	else
> +		git config core.useBuiltinFSMonitor false &&
>  		#
>  		# Choose integration script based on existence of Watchman.
>  		# Fall back to an empty integration script.
> @@ -285,4 +292,30 @@ test_expect_success "setup without fsmonitor" '
>  test_fsmonitor_suite
>  trace_stop
>  
> +#
> +# Run a full set of perf tests using the built-in fsmonitor--daemon.
> +# It does not use the Hook API, so it has a different setup.
> +# Explicitly start the daemon here and before we start client commands
> +# so that we can later add custom tracing.
> +#
> +
> +test_lazy_prereq HAVE_FSMONITOR_DAEMON '
> +	git version --build-options | grep "feature:" | grep "fsmonitor--daemon"
> +'

Here you do create the prereq. Let's put this into t/test-lib.sh
or t/test-lib-functions.sh, whichever is more appropriate.

> +
> +if test_have_prereq HAVE_FSMONITOR_DAEMON
> +then
> +	USE_FSMONITOR_DAEMON=t
> +
> +	trace_start fsmonitor--daemon--server
> +	git fsmonitor--daemon --start
> +
> +	trace_start fsmonitor--daemon--client
> +	test_expect_success "setup for fsmonitor--daemon" 'setup_for_fsmonitor'

Maybe this is copied from the rest of the file, but we should probably
use the standard layout for tests here:

	test_expect_success 'setup for fsmonitor--daemon' '
		setup_for_fsmonitor
	'

> +	test_fsmonitor_suite
> +
> +	git fsmonitor--daemon --stop
> +	trace_stop
> +fi
> +

Thanks,
-Stolee