Re: [PATCH 06/23] fsmonitor-settings: stub in platform-specific incompatibility checking

From: Derrick Stolee <derrickstolee@github.com>
Date: Feb 24, 2022, 15:05 UTC
Message-ID: <b459111d-a536-b4d7-44d5-7f5aeb9ffb08@github.com>
In-Reply-To: <4715677f85ff3f47b437f82e2b82a8b5dd371c99.1644940773.git.gitgitgadget@gmail.com>

On 2/15/2022 10:59 AM, Jeff Hostetler via GitGitGadget wrote:

Show 11 quoted lines

> From: Jeff Hostetler <jeffhost@microsoft.com>
> 
> Extend generic incompatibility checkout with platform-specific
> mechanism.  Stub in Win32 version.
> 
> In the existing fsmonitor-settings code we have a way to mark
> types of repos as incompatible with fsmonitor (whether via the
> hook and ipc APIs).  For example, we do this for bare repos,
> since there are no files to watch.
> 
> Extend this exclusion mechanism for platfor-specific reasons.

s/platfor-specific/platform-specific/

> +# If your platform has os-specific ways to tell if a repo is incompatible with
> +# fsmonitor (whether the hook or ipc daemon version), set FSMONITOR_OS_SETTINGS
> +# to the "<name>" of the corresponding `compat/fsmonitor/fsm-settings-<name>.c`
> +# that implements the `fsm_os_settings__*()` routines.

nit: It's just a comment, but I think "OS" and "IPC" should probably be capitalized.

Show 6 quoted lines

> +#ifdef HAVE_FSMONITOR_OS_SETTINGS
> +	{
> +		enum fsmonitor_reason reason;
> +
> +		reason = fsm_os__incompatible(r);
> +		if (reason != FSMONITOR_REASON_ZERO) {

A naming nit about FSMONITOR_REASON_ZERO. It seems named ZERO on purpose so a non-zero reason signals incompatibility. That would mean you could use

		if (reason)

here. Alternatively, FSMONITOR_REASON_COMPATIBLE would signal the meaning better here:

		if (reason != FSMONITOR_REASON_COMPATIBLE)

> +			set_incompatible(r, reason);
> +			return 1;
> +		}

Thanks, -Stolee