"Eric DeCosta via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes: > Most modern Samba-based filers have the necessary support to enable > fsmonitor on network-mounted repos. My impression from the earlier discussion [*] was that having the necessary support and the support working well are two different things, and the network mounted directory being served via SMB was not enough sign of the latter. https://lore.kernel.org/git/16832f8a-c582-23bb-dda9-b7b2597a42eb@xxxxxxxxxxxxxxxxx/ I do not do Windows, so I'll leave it up to the experts, but if everybody who talks SMB behaves well, then the updated code that enables fsmonitor for SMB talkers and disables it for all other remotely mounted directories, with a configuration override, does sound like a good way to go. I think the new code in check_remote() is broken, though (see below). > +/* > + * Check if monitoring remote working directories is allowed. > + * > + * By default monitoring remote working directories is not allowed, > + * but users may override this behavior in enviroments where they > + * have proper support. > +*/ All existing multi-line comments in this file (and properly formatted ones for this project) ends with "*/" where the asterisk aligns with the asterisk on the previous line. The three-line design goal is well written, but as you are special casing SMB, perhaps By default, monitoring remote working directories is disabled unless on a network filesystem that is known to behave well. Users may override ... or something like that may be warranted. > +static enum fsmonitor_reason check_allow_remote(struct repository *r) > +{ > + int allow; > + > + if (repo_config_get_bool(r, "fsmonitor.allowremote", &allow) || !allow) > + return FSMONITOR_REASON_REMOTE; > + > + return FSMONITOR_REASON_OK; > +} This is not wrong per se, but it probably is easier to follow if you write them the other way around. if (!repo_config_get_bool(..., &allow) && allow) return FSMONITOR_REASON_OK; return FSMONITOR_REASON_REMOTE; After all, as the big comment before the function says, by default we deny and only on exceptions we allow. Actually, the above is not quite right. You'd probably want a "not set" or "undecided" bit. I.e. something like if (!repo_config_get_bool(..., &allow)) return allow ? FSMONITOR_REASON_OK : FSMONITOR_REASON_REMOTE; return FSMONITOR_REASON_UNDECIDED; /* invented... */ > +/* > + * Check if the remote working directory is mounted via SMB > + * > + * For now, remote working directories are only supported via SMB mounts > +*/ "*/" -> " */". > +static enum fsmonitor_reason check_smb(wchar_t *wpath) > +{ > + 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 FSMONITOR_REASON_ERROR; > + } > + > + if (!GetFileInformationByHandleEx(h, FileRemoteProtocolInfo, > + &proto_info, sizeof(proto_info))) { Overly deep indentation that unnecessarily causes an overly long line. "&" in "&proto_info" should align with "h" in "...HandleEx(h,", we use fixed-width font, and our tab-width is 8. The second line of CreateFileW() call is also overly indented and may want to be fixed, but it does not overly extend to the right end of the display (we aim to fit in 80-columns, as CodingGuideline says) so it would be nice if it gets fixed, but it may be tolerated. This one is not. > + error(_("[GLE %ld] unable to get protocol information for '%ls'"), > + GetLastError(), wpath); This line gets it right ;-) > + CloseHandle(h); > + return FSMONITOR_REASON_ERROR; > + } > + > + CloseHandle(h); > + > + if (proto_info.Protocol == WNNC_NET_SMB) > + return FSMONITOR_REASON_OK; > + > + return FSMONITOR_REASON_ERROR; Is it? Shouldn't it be REASON_REMOTE, not ERROR? Unlike the earlier case where you couldn't figure out the protocol information, at this point you know you learned what protocol is in use, and it is just that the protocol was not what you liked. > +} > + > /* > * Remote working directories are problematic for FSMonitor. > * > @@ -76,6 +128,7 @@ static enum fsmonitor_reason check_vfs4git(struct repository *r) > */ > static enum fsmonitor_reason check_remote(struct repository *r) > { > + enum fsmonitor_reason reason; > wchar_t wpath[MAX_PATH]; > wchar_t wfullpath[MAX_PATH]; > size_t wlen; > @@ -115,7 +168,11 @@ static enum fsmonitor_reason check_remote(struct repository *r) > trace_printf_key(&trace_fsmonitor, > "check_remote('%s') true", > r->worktree); > - return FSMONITOR_REASON_REMOTE; > + > + reason = check_smb(wfullpath); > + if (reason != FSMONITOR_REASON_OK) > + return reason; > + return check_allow_remote(r); This does not fulfill the promise you made in front of check_allow_remote(). As we saw, check_smb() returns something other than REASON_OK when it is talking to a remote that is not SMB, and when that happens, you are not giving check_allow_remote() a chance to intervene and override. It should be more like reason = check_allow_remote(r); if (reason == FSMONITOR_REASON_OK || reason == FSMONITOR_REASON_REMOTE) return reason; /* * check_allow_remote() did not decide, so use the default * based on the network filesystem. */ return check_smb(wfullpath); I would think. > } > > return FSMONITOR_REASON_OK; > > base-commit: c50926e1f48891e2671e1830dbcd2912a4563450
