Re: [PATCH v3] fsmonitor: option to allow fsmonitor to run against network-mounted repos

[Junio C Hamano <gitster@pobox.com>]

"Eric DeCosta via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Eric DeCosta <edecosta@mathworks.com>
>
> Though perhaps not common, there are uses cases where users have large,
> network-mounted repos. Having the ability to run fsmonitor against
> network paths would benefit those users.
>
> Most modern Samba-based filers have the necessary support to enable
> fsmonitor on network-mounted repos. As a first step towards enabling
> fsmonitor to work against network-mounted repos, introduce a
> configuration option, 'fsmonitor.allowRemote'. Setting this option to
> true will override the default behavior (erroring-out) when a
> network-mounted repo is detected by fsmonitor.
>
> Signed-off-by: Eric DeCosta <edecosta@mathworks.com>
> ---
> ...
>  compat/fsmonitor/fsm-settings-win32.c | 66 +++++++++++++++++++++++++++
>  1 file changed, 66 insertions(+)
>
> diff --git a/compat/fsmonitor/fsm-settings-win32.c b/compat/fsmonitor/fsm-settings-win32.c
> index 907655720bb..32c0695c6c1 100644
> --- a/compat/fsmonitor/fsm-settings-win32.c
> +++ b/compat/fsmonitor/fsm-settings-win32.c
> @@ -24,6 +24,60 @@ static enum fsmonitor_reason check_vfs4git(struct repository *r)
>  	return FSMONITOR_REASON_OK;
>  }
>  
> +/*
> + * Check if monitoring remote working directories is allowed.
> + *
> + * By default, monitoring remote working directories is
> + * disabled unless on a network filesystem that is known to
> + * behave well.  Users may override this behavior in enviroments where
> + * they have proper support.
> + */

After applying this patch, "unless on a network filesystem ..." part
is not exactly in effect yet; we could say that we start with no
known-to-behave-well network filesystems, but we can then update the
above comment when we start to know of at least one good one.

> +/*
> + * Check remote working directory protocol.
> + *
> + * Error if client machine cannot get remote protocol information.
> + */

Good, but void means that the caller of this function does not know
when we detected an error.  Perhaps return -1 on error, return 0 on
"not error", so that we can return 1 when we learn to recognize
"known to behave well" network filesystem to tell the caller?

That is,

> +static void check_remote_protocol(wchar_t *wpath)

"void" -> "int"

> +{
> +	HANDLE h;
> +	FILE_REMOTE_PROTOCOL_INFO proto_info;
> +
> +	h = CreateFileW(wpath, GENERIC_READ, FILE_SHARE_READ, NULL, OPEN_EXISTING,
> +			FILE_FLAG_BACKUP_SEMANTICS, NULL);
> +
> +	if (h == INVALID_HANDLE_VALUE) {
> +		error(_("[GLE %ld] unable to open for read '%ls'"),
> +		      GetLastError(), wpath);
> +		return;

"return" -> "return -1"

> +	}
> +
> +	if (!GetFileInformationByHandleEx(h, FileRemoteProtocolInfo,
> +		&proto_info, sizeof(proto_info))) {
> +		error(_("[GLE %ld] unable to get protocol information for '%ls'"),
> +		      GetLastError(), wpath);
> +		CloseHandle(h);
> +		return;

"return" -> "return -1"

> +	}
> +
> +	CloseHandle(h);
> +
> +	trace_printf_key(&trace_fsmonitor,
> +				"check_remote_protocol('%ls') remote protocol %#8.8lx",
> +				wpath, proto_info.Protocol);
> +
> +	return;

"return" -> "return 0" (or "-1")

> +}
> +
>  /*
>   * Remote working directories are problematic for FSMonitor.
>   *
> @@ -115,6 +169,18 @@ static enum fsmonitor_reason check_remote(struct repository *r)
>  		trace_printf_key(&trace_fsmonitor,
>  				 "check_remote('%s') true",
>  				 r->worktree);
> +
> +		check_remote_protocol(wfullpath);

And here

		ret = check_remote_protocol(wfullpath);
		if (ret < 0)
			/* definitely an error */
			return FSMONITOR_REASON_ERROR;

and then we can fall thru the non-error case below.  We'd of course
need to declare "int ret" at the beginning of the function.

> +		switch (check_config_allowremote(r)) {
> +		case 0: /* config overrides and disables */
> +			return FSMONITOR_REASON_REMOTE;
> +		case 1: /* config overrides and enables */
> +			return FSMONITOR_REASON_OK;
> +		default:
> +			break; /* config has no opinion */
> +		}
> +
>  		return FSMONITOR_REASON_REMOTE;
>  	}

In the future, when this "first step" graduates to the upcoming
release, we may want to have a follow-up enhancement patch that
changes the code like so:

 * we recognize ones like SMB in check_remote_protocol() as "known
   to be good", and return 1 from there

 * after the "switch" above determies that the configuration file
   does not have any opinion, instead of unconditionally returning
   REASON_REMOTE to refuse the request, pay attention to "ret", e.g.
   something like

-	return FSMONITOR_REASON_REMOTE;
+	if (!ret)
+		return FSMONITOR_REASON_REMOTE;
+	else /* known to be good ones */
+		return FSMONITOR_REASON_OK;

When we do so, we'd resurrect the "unless on a network filesystem
that is known to behave well" comment.  What this last part does is
exactly that.

Thanks.