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

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 17, 2022, 15:54 UTC
Message-ID: <xmqq5yiqzx01.fsf@gitster.g>
In-Reply-To: <f5388e4d-7eb7-9333-6a8e-86ce449aced0@github.com>

Derrick Stolee <derrickstolee@github.com> writes:

Show 23 quoted lines

> 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?

Ah, I didn't consider the possibility where the user uses the configuration to say "enable it if you are able, but it is OK if you cannot". Whether the "is supported" is dynamic or compiled-in, that may be a valid issue to consider. An easy way out may be to declare that the value "true" for "core.fsmonitor" variable means exactly that, i.e. the user asks to run it, but it is not an error if it cannot run.

A variant that may need slightly more work would be to introduce a separate value (perhaps "when-able") that means that, while keeping the "true" to mean "run the built-in one, or error out to let me know otherwise" as before.

Thanks.