Re: [PATCH v2 3/5] scalar: enable built-in FSMonitor on `register`

[Derrick Stolee <derrickstolee@github.com>]

On 8/16/2022 7:58 PM, Matthew John Cheetham via GitGitGadget wrote:

> +#ifdef HAVE_FSMONITOR_DAEMON_BACKEND
> +		/*
> +		 * Enable the built-in FSMonitor on supported platforms.
> +		 */
> +		{ "core.fsmonitor", "true" },
> +#endif
> +	if (fsmonitor_ipc__is_supported() && start_fsmonitor_daemon())
> +		return error(_("could not start the FSMonitor daemon"));
> +

I initially worried if fsmonitor_ipc__is_supported() could use some
run-time information to detect if FS Monitor is supported (say, existence
of a network share or something). However, that implementation is
currently defined as a constant depending on
HAVE_FSMONITOR_DAEMON_BACKEND.

The reason I was worried is that we could enable core.fsmonitor=true based
on the compile-time macro, but then avoid starting the daemon based on the
run-time results. If we get into this state, would the user's 'git status'
calls start complaining about the core.fsmonitor=true config because it is
not supported?

The most future-proof thing to do might be to move the config write out of
the set_recommended_config() and into start_fsmonitor_daemon(). Perhaps
rename it to enable_fsmonitor() so it can fail due to writing the config
_or_ for starting the daemon. The error message would change, then, too.

Or maybe I'm making a mountain out of a mole hill and what exists here is
perfectly fine.

> +test_lazy_prereq BUILTIN_FSMONITOR '
> +	git version --build-options | grep -q "feature:.*fsmonitor--daemon"
> +'

It looks like we already have a FSMONITOR_DAEMON prereq in test-lib.sh.
Should we use that instead?

Thanks,
-Stolee