Re: [PATCH] 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:

> 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@jeffhostetler.com/

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