Re: [PATCH 04/23] fsmonitor: introduce `core.useBuiltinFSMonitor` to call the daemon via IPC

From: Derrick Stolee <stolee@gmail.com>
Date: Apr 26, 2021, 14:56 UTC
Message-ID: <f77457d0-925d-2ea1-4ad4-b3315532b37f@gmail.com>
In-Reply-To: <7082528d8f7c1afa33e1146e3d274e044735f6a1.1617291666.git.gitgitgadget@gmail.com>

On 4/1/21 11:40 AM, Johannes Schindelin via GitGitGadget wrote:> @@ -2515,6 +2515,11 @@ int git_config_get_max_percent_split_change(void)

>  
>  int repo_config_get_fsmonitor(struct repository *r)
>  {
> +	if (r->settings.use_builtin_fsmonitor > 0) {

Don't forget to run prepare_repo_settings(r) first.

> +		core_fsmonitor = "(built-in daemon)";
> +		return 1;
> +	}
> +

I found this odd, assigning a string to core_fsmonitor that would definitely cause a problem trying to execute it as a hook. I wondered the need for it at all, but found that there are several places in the FS Monitor subsystem that use core_fsmonitor as if it was a boolean, indicating whether or not the feature is enabled at all.

A cleaner way to handle this would be to hide the data behind a helper method, say "fsmonitor_enabled()" that could then check a value on the repository (or index) and store the hook value as a separate value that is only used by the hook-based implementation.

It's probably a good idea to do that cleanup now, before we find on accident that we missed a gap and start trying to run this bogus string as a hook invocation.

Show 10 quoted lines

> -static int query_fsmonitor(int version, const char *last_update, struct strbuf *query_result)
> +static int query_fsmonitor(int version, struct index_state *istate, struct strbuf *query_result)
>  {
> +	struct repository *r = istate->repo ? istate->repo : the_repository;
> +	const char *last_update = istate->fsmonitor_last_update;
>  	struct child_process cp = CHILD_PROCESS_INIT;
>  	int result;
>  
>  	if (!core_fsmonitor)
>  		return -1;

Here is an example of it being used as a boolean.

Show 9 quoted lines

> +	if (r->settings.use_builtin_fsmonitor > 0) {
> +#ifdef HAVE_FSMONITOR_DAEMON_BACKEND
> +		return fsmonitor_ipc__send_query(last_update, query_result);
> +#else
> +		/* Fake a trivial response. */
> +		warning(_("fsmonitor--daemon unavailable; falling back"));
> +		strbuf_add(query_result, "/", 2);
> +		return 0;
> +#endif

This seems like a case where the helper fsmonitor_ipc__is_supported() could be used instead of compile-time macros.

(I think this is especially true when we consider the future of the feature on Linux and the possibility of the same compiled code needing to check run-time properties of the platform for compatibility.)

Show 9 quoted lines

> --- a/repo-settings.c
> +++ b/repo-settings.c
> @@ -58,6 +58,9 @@ void prepare_repo_settings(struct repository *r)
>  		r->settings.core_multi_pack_index = value;
>  	UPDATE_DEFAULT_BOOL(r->settings.core_multi_pack_index, 1);
>  
> +	if (!repo_config_get_bool(r, "core.usebuiltinfsmonitor", &value) && value)
> +		r->settings.use_builtin_fsmonitor = 1;
> +

Follows the patterns of repo settings. Good.