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

From: Derrick Stolee <derrickstolee@github.com>
Date: Aug 17, 2022, 14:34 UTC
Message-ID: <f5388e4d-7eb7-9333-6a8e-86ce449aced0@github.com>
In-Reply-To: <5fdf8337972d7092aba06a9c750f42cd5868e630.1660694290.git.gitgitgadget@gmail.com>

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

Show 9 quoted lines

> +#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