Re: [PATCH v3] fsmonitor: option to allow fsmonitor to run against network-mounted repos
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Aug 11, 2022, 19:33 UTC
- Message-ID
- <xmqqh72iefsn.fsf@gitster.g>
- In-Reply-To
- <pull.1317.v3.git.1660242752495.gitgitgadget@gmail.com>
"Eric DeCosta via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 35 quoted lines
> 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.
Show 5 quoted lines
> +/* > + * 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"
Show 11 quoted lines
> +{
> + 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"
Show 8 quoted lines
> + }
> +
> + 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"
Show 9 quoted lines
> + }
> +
> + 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")
Show 11 quoted lines
> +}
> +
> /*
> * 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.
Show 11 quoted lines
> + 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.