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

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

[Eric DeCosta via GitGitGadget]

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.

Additionally, as part of this first step, monitoring of network-mounted
repos will be restricted to those mounted over SMB regardless of the
value of 'fsmonitor.allowRemote' until more extensive testing can be
performed.

Signed-off-by: Eric DeCosta <edecosta@mathworks.com>
---
    Option to allow fsmonitor to run against repos on network file systems

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1317%2Fedecosta-mw%2Fmaster-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1317/edecosta-mw/master-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1317

 compat/fsmonitor/fsm-settings-win32.c | 59 ++++++++++++++++++++++++++-
 1 file changed, 58 insertions(+), 1 deletion(-)

diff --git a/compat/fsmonitor/fsm-settings-win32.c b/compat/fsmonitor/fsm-settings-win32.c
index 907655720bb..d120e4710cf 100644
--- a/compat/fsmonitor/fsm-settings-win32.c
+++ b/compat/fsmonitor/fsm-settings-win32.c
@@ -24,6 +24,58 @@ 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 not allowed,
+ * but users may override this behavior in enviroments where they
+ * have proper support.
+*/
+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;
+}
+
+/*
+ * 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))) {
+		error(_("[GLE %ld] unable to get protocol information for '%ls'"),
+		      GetLastError(), wpath);
+		CloseHandle(h);
+		return FSMONITOR_REASON_ERROR;
+	}
+
+	CloseHandle(h);
+
+	if (proto_info.Protocol == WNNC_NET_SMB)
+		return FSMONITOR_REASON_OK;
+
+	return FSMONITOR_REASON_ERROR;
+}
+
 /*
  * 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);
 	}
 
 	return FSMONITOR_REASON_OK;

base-commit: c50926e1f48891e2671e1830dbcd2912a4563450
-- 
gitgitgadget

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

[Junio C Hamano]

"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

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

[Eric D]

On Wed, Aug 10, 2022 at 12:56 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> "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.

I'll clean them up, thanks.

> 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.

That reads much better, thanks.

> > +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... */

Makes sense. How about FSMONITOR_OVERRIDE_REQUIRED ? The error message
could then indicate that remote repos are normally unsupported but
that setting the fsmonitor.allowRemote flag overrides this behavior.

> > +/*
> > + * 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 ;-)

Got it. I'll check my editor settings too :-)

> > +             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.

Yeah, I noticed that myself this morning.

> > +}
> > +
> >  /*
> >   * 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.

If we do as you suggest above, then fsmonitor.allowRemote=true would
override regardless of the protocol being used. The more conservative
path is to only allow the override if SMB is being used.

It could perhaps be better written as:

reason = check_allow_remote(r);
if (reason != FSMONITOR_REASON_OK)
        return reason;

return check_smb(wbfullpath);


> >       }
> >
> >       return FSMONITOR_REASON_OK;
> >
> > base-commit: c50926e1f48891e2671e1830dbcd2912a4563450

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

[Junio C Hamano]

Eric D <eric.decosta@gmail.com> writes:

> Makes sense. How about FSMONITOR_OVERRIDE_REQUIRED ? The error message
> could then indicate that remote repos are normally unsupported but
> that setting the fsmonitor.allowRemote flag overrides this behavior.

I actually think check_allow_remote() should be renamed to have
"config" somewhere in its name, and return -1, 0 or 1 and not "enum
fsmonitor_reason".

	static int check_config_allowremote(...)
	{
		int allow;

		if (repor_config_get_bool(..., &allow))
			return allow;
		return -1; /* undecided */
	}

then caller can do

	switch (check_config_allowremote(...)) {
	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 check_smb(...);

> If we do as you suggest above, then fsmonitor.allowRemote=true would
> override regardless of the protocol being used.

Exactly.  The code should not try to outsmart the user.

If the user says they wants to use it on a particular remote, even
if you do not know if that particular remote system works, just let
them try and see if it works.  If it does not, they can easily
disable, because the enabiling was a deliberate act by them in the
first place.  They know how to fix it.

Thanks.

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

[Eric D]

On Wed, Aug 10, 2022 at 3:50 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Eric D <eric.decosta@gmail.com> writes:
>
> > Makes sense. How about FSMONITOR_OVERRIDE_REQUIRED ? The error message
> > could then indicate that remote repos are normally unsupported but
> > that setting the fsmonitor.allowRemote flag overrides this behavior.
>
> I actually think check_allow_remote() should be renamed to have
> "config" somewhere in its name, and return -1, 0 or 1 and not "enum
> fsmonitor_reason".
>
>         static int check_config_allowremote(...)
>         {
>                 int allow;
>
>                 if (repor_config_get_bool(..., &allow))
>                         return allow;
>                 return -1; /* undecided */
>         }
>
> then caller can do
>
>         switch (check_config_allowremote(...)) {
>         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 check_smb(...);
>
> > If we do as you suggest above, then fsmonitor.allowRemote=true would
> > override regardless of the protocol being used.
>
> Exactly.  The code should not try to outsmart the user.
>
> If the user says they wants to use it on a particular remote, even
> if you do not know if that particular remote system works, just let
> them try and see if it works.  If it does not, they can easily
> disable, because the enabiling was a deliberate act by them in the
> first place.  They know how to fix it.
>
> Thanks.
>

100% agree with you, thanks.

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

[Eric D]

On Wed, Aug 10, 2022 at 4:36 PM Eric D <eric.decosta@gmail.com> wrote:
>
> On Wed, Aug 10, 2022 at 3:50 PM Junio C Hamano <gitster@pobox.com> wrote:
> >
> > Eric D <eric.decosta@gmail.com> writes:
> >
> > > Makes sense. How about FSMONITOR_OVERRIDE_REQUIRED ? The error message
> > > could then indicate that remote repos are normally unsupported but
> > > that setting the fsmonitor.allowRemote flag overrides this behavior.
> >
> > I actually think check_allow_remote() should be renamed to have
> > "config" somewhere in its name, and return -1, 0 or 1 and not "enum
> > fsmonitor_reason".
> >
> >         static int check_config_allowremote(...)
> >         {
> >                 int allow;
> >
> >                 if (repor_config_get_bool(..., &allow))
> >                         return allow;
> >                 return -1; /* undecided */
> >         }
> >
> > then caller can do
> >
> >         switch (check_config_allowremote(...)) {
> >         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 check_smb(...);
> >
> > > If we do as you suggest above, then fsmonitor.allowRemote=true would
> > > override regardless of the protocol being used.
> >
> > Exactly.  The code should not try to outsmart the user.
> >
> > If the user says they wants to use it on a particular remote, even
> > if you do not know if that particular remote system works, just let
> > them try and see if it works.  If it does not, they can easily
> > disable, because the enabiling was a deliberate act by them in the
> > first place.  They know how to fix it.
> >
> > Thanks.
> >
>
> 100% agree with you, thanks.

The only thing that is somewhat gnawing at me is that just because the
remote worktree is mounted via SMB is no guarantee that fsmonitor will
work correctly. In many (most?) cases it should, but who knows what
support the filer server has.

I think we should allow the user to override regardless - as you said
let the user try it. But, conservatively, just because SMB is there
may not be enough to let the monitor start without the explicit user
override. Being able to report on which protocol is being used could
provide useful diagnostics, but that's about it.

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

[Junio C Hamano]

Eric D <eric.decosta@gmail.com> writes:

> The only thing that is somewhat gnawing at me is that just because the
> remote worktree is mounted via SMB is no guarantee that fsmonitor will
> work correctly. In many (most?) cases it should, but who knows what
> support the filer server has.
>
> I think we should allow the user to override regardless - as you said
> let the user try it. But, conservatively, just because SMB is there
> may not be enough to let the monitor start without the explicit user
> override. Being able to report on which protocol is being used could
> provide useful diagnostics, but that's about it.

I do not think anybody minds if the initial/first step would be to
add "option to allow" and do nothing else.  No "are we talking SMB?"
check, and just a simple "by default we refuse going remote, but
since the user has an explicit opt-in configuration set, we allow".

That is sufficient to let people gain experience using fsmonitor on
Windows mounting from various filer implementations.  And then the
next step, in one or two major releases down the road, may try to
check what kind of filer we are on and see if it is one of the
"good" ones to flip the default selectively.

[PATCH v2 0/2] Option to allow fsmonitor to run against repos on network file systems

[Eric DeCosta via GitGitGadget]

cc: Eric D eric.decosta@gmail.com

Eric DeCosta (2):
  fsmonitor: option to allow fsmonitor to run against network-mounted
    repos
  fsmonitor.allowRemote now overrides default behavior

 compat/fsmonitor/fsm-settings-win32.c | 66 +++++++++++++++++++++++++++
 1 file changed, 66 insertions(+)


base-commit: c50926e1f48891e2671e1830dbcd2912a4563450
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1317%2Fedecosta-mw%2Fmaster-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1317/edecosta-mw/master-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1317

Range-diff vs v1:

 1:  7e67ce8c944 = 1:  7e67ce8c944 fsmonitor: option to allow fsmonitor to run against network-mounted repos
 -:  ----------- > 2:  7a071c9e6be fsmonitor.allowRemote now overrides default behavior

-- 
gitgitgadget

[PATCH v2 1/2] fsmonitor: option to allow fsmonitor to run against network-mounted repos

[Eric DeCosta via GitGitGadget]

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.

Additionally, as part of this first step, monitoring of network-mounted
repos will be restricted to those mounted over SMB regardless of the
value of 'fsmonitor.allowRemote' until more extensive testing can be
performed.

Signed-off-by: Eric DeCosta <edecosta@mathworks.com>
---
 compat/fsmonitor/fsm-settings-win32.c | 59 ++++++++++++++++++++++++++-
 1 file changed, 58 insertions(+), 1 deletion(-)

diff --git a/compat/fsmonitor/fsm-settings-win32.c b/compat/fsmonitor/fsm-settings-win32.c
index 907655720bb..d120e4710cf 100644
--- a/compat/fsmonitor/fsm-settings-win32.c
+++ b/compat/fsmonitor/fsm-settings-win32.c
@@ -24,6 +24,58 @@ 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 not allowed,
+ * but users may override this behavior in enviroments where they
+ * have proper support.
+*/
+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;
+}
+
+/*
+ * 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))) {
+		error(_("[GLE %ld] unable to get protocol information for '%ls'"),
+		      GetLastError(), wpath);
+		CloseHandle(h);
+		return FSMONITOR_REASON_ERROR;
+	}
+
+	CloseHandle(h);
+
+	if (proto_info.Protocol == WNNC_NET_SMB)
+		return FSMONITOR_REASON_OK;
+
+	return FSMONITOR_REASON_ERROR;
+}
+
 /*
  * 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);
 	}
 
 	return FSMONITOR_REASON_OK;
-- 
gitgitgadget

[PATCH v2 2/2] fsmonitor.allowRemote now overrides default behavior

[Eric DeCosta via GitGitGadget]

From: Eric DeCosta <edecosta@mathworks.com>

Reworked the logic around fsmonitor.allowRemote such that if
allowRemote is set it will determine if monitoring the remote
worktree is allowed.

Get remote protocoal information; if this fails report an error else
print it if tracing is enabled.

Fixed fomratting issues.

Signed-off-by: Eric DeCosta <edecosta@mathworks.com>
---
 compat/fsmonitor/fsm-settings-win32.c | 57 ++++++++++++++++-----------
 1 file changed, 33 insertions(+), 24 deletions(-)

diff --git a/compat/fsmonitor/fsm-settings-win32.c b/compat/fsmonitor/fsm-settings-win32.c
index d120e4710cf..32c0695c6c1 100644
--- a/compat/fsmonitor/fsm-settings-win32.c
+++ b/compat/fsmonitor/fsm-settings-win32.c
@@ -27,53 +27,55 @@ static enum fsmonitor_reason check_vfs4git(struct repository *r)
 /*
  * 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.
-*/
-static enum fsmonitor_reason check_allow_remote(struct repository *r)
+ * 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.
+ */
+static int check_config_allowremote(struct repository *r)
 {
 	int allow;
 
-	if (repo_config_get_bool(r, "fsmonitor.allowremote", &allow) || !allow)
-		return FSMONITOR_REASON_REMOTE;
+	if (!repo_config_get_bool(r, "fsmonitor.allowremote", &allow))
+		return allow;
 
-	return FSMONITOR_REASON_OK;
+	return -1; /* fsmonitor.allowremote not set */
 }
 
 /*
- * Check if the remote working directory is mounted via SMB
+ * Check remote working directory protocol.
  *
- * For now, remote working directories are only supported via SMB mounts
-*/
-static enum fsmonitor_reason check_smb(wchar_t *wpath)
+ * Error if client machine cannot get remote protocol information.
+ */
+static void check_remote_protocol(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);
+			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;
+		return;
 	}
 
 	if (!GetFileInformationByHandleEx(h, FileRemoteProtocolInfo,
-									&proto_info, sizeof(proto_info))) {
+		&proto_info, sizeof(proto_info))) {
 		error(_("[GLE %ld] unable to get protocol information for '%ls'"),
 		      GetLastError(), wpath);
 		CloseHandle(h);
-		return FSMONITOR_REASON_ERROR;
+		return;
 	}
 
 	CloseHandle(h);
 
-	if (proto_info.Protocol == WNNC_NET_SMB)
-		return FSMONITOR_REASON_OK;
+	trace_printf_key(&trace_fsmonitor,
+				"check_remote_protocol('%ls') remote protocol %#8.8lx",
+				wpath, proto_info.Protocol);
 
-	return FSMONITOR_REASON_ERROR;
+	return;
 }
 
 /*
@@ -128,7 +130,6 @@ static enum fsmonitor_reason check_smb(wchar_t *wpath)
  */
 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;
@@ -169,10 +170,18 @@ static enum fsmonitor_reason check_remote(struct repository *r)
 				 "check_remote('%s') true",
 				 r->worktree);
 
-		reason = check_smb(wfullpath);
-		if (reason != FSMONITOR_REASON_OK)
-			return reason;
-		return check_allow_remote(r);
+		check_remote_protocol(wfullpath);
+
+		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;
 	}
 
 	return FSMONITOR_REASON_OK;
-- 
gitgitgadget

Re: [PATCH v2 2/2] fsmonitor.allowRemote now overrides default behavior

[Junio C Hamano]

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

> From: Eric DeCosta <edecosta@mathworks.com>
>
> Reworked the logic around fsmonitor.allowRemote such that if
> allowRemote is set it will determine if monitoring the remote
> worktree is allowed.
>
> Get remote protocoal information; if this fails report an error else
> print it if tracing is enabled.
>
> Fixed fomratting issues.

The end result (i.e. HEAD^{tree} of the branch you developed these
two patches on) may be good (I haven't checked), but it is not how
we fix problems in an earlier attempt in this project by keeping the
faulty commit(s) on the bottom and piling "oops, that was wrong, and
here is a fix-up" commit(s) on top.

Once you are happy with the end result, use "rebase -i" to clean-up
the history leading to that end result.  The goal is to pretend as
if you were a perfect human, more perfect than your actual self, who
came up with an ideal patch without making mistakes that need to be
corrected with "fix-up" commits.  In this particular case, you'd
most likely want to end up with a single commit, so squashing them
together and fixing up the log message might be all you need to do.
When you work on a more elaborate topic, you may also want to split
or reorder original commits to present a logical progression towards
the end result.  "rebase -i" is a good tool to help you do so.

I am not a user of GitGitGadget myself, but if I recall correctly,
you should be able to force-push the result of such a clean-up to
update the pull-request, to trigger a new iteration to be sent to
the list.

Thanks.

Re: [PATCH v2 2/2] fsmonitor.allowRemote now overrides default behavior

[Eric D]

Well, needless to say I wasn't expecting GitGitGadget to do what it
did.I had squashed things down to just two commits and forced-pushed
the second commit thinking that just the relevant stuff from the
second commit would show up in the next patch. Obviously that didn't
happen. Sorry about that.

I can certainly squash it down to just one commit and force-push that.

On Thu, Aug 11, 2022 at 1:17 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Eric DeCosta via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > From: Eric DeCosta <edecosta@mathworks.com>
> >
> > Reworked the logic around fsmonitor.allowRemote such that if
> > allowRemote is set it will determine if monitoring the remote
> > worktree is allowed.
> >
> > Get remote protocoal information; if this fails report an error else
> > print it if tracing is enabled.
> >
> > Fixed fomratting issues.
>
> The end result (i.e. HEAD^{tree} of the branch you developed these
> two patches on) may be good (I haven't checked), but it is not how
> we fix problems in an earlier attempt in this project by keeping the
> faulty commit(s) on the bottom and piling "oops, that was wrong, and
> here is a fix-up" commit(s) on top.
>
> Once you are happy with the end result, use "rebase -i" to clean-up
> the history leading to that end result.  The goal is to pretend as
> if you were a perfect human, more perfect than your actual self, who
> came up with an ideal patch without making mistakes that need to be
> corrected with "fix-up" commits.  In this particular case, you'd
> most likely want to end up with a single commit, so squashing them
> together and fixing up the log message might be all you need to do.
> When you work on a more elaborate topic, you may also want to split
> or reorder original commits to present a logical progression towards
> the end result.  "rebase -i" is a good tool to help you do so.
>
> I am not a user of GitGitGadget myself, but if I recall correctly,
> you should be able to force-push the result of such a clean-up to
> update the pull-request, to trigger a new iteration to be sent to
> the list.
>
> Thanks.

Re: [PATCH v2 2/2] fsmonitor.allowRemote now overrides default behavior

[Junio C Hamano]

Eric D <eric.decosta@gmail.com> writes:

> Well, needless to say I wasn't expecting GitGitGadget to do what it
> did.I had squashed things down to just two commits and forced-pushed
> the second commit thinking that just the relevant stuff from the
> second commit would show up in the next patch. Obviously that didn't
> happen. Sorry about that.

Oh, sorry to hear that.  If your ideal "logical progression" needs
two commits, then please do present the series that way.  What GGG
sent out was apparently not that (i.e. the same one from v1 with
full of fix-ups for it in 2/2).

Re: [PATCH v2 2/2] fsmonitor.allowRemote now overrides default behavior

[Eric D]

Given that, in the end, the change is rather small and involves just
one file, having it be just one commit is fine. Perhaps my next lesson
to learn is to generate and send the patch sets myself, but that will
be for another time.

Thank you for all your patience, it makes a total noob like me feel welcome.

On Thu, Aug 11, 2022 at 1:53 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Eric D <eric.decosta@gmail.com> writes:
>
> > Well, needless to say I wasn't expecting GitGitGadget to do what it
> > did.I had squashed things down to just two commits and forced-pushed
> > the second commit thinking that just the relevant stuff from the
> > second commit would show up in the next patch. Obviously that didn't
> > happen. Sorry about that.
>
> Oh, sorry to hear that.  If your ideal "logical progression" needs
> two commits, then please do present the series that way.  What GGG
> sent out was apparently not that (i.e. the same one from v1 with
> full of fix-ups for it in 2/2).
>

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

[Eric DeCosta via GitGitGadget]

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>
---
    Option to allow fsmonitor to run against repos on network file systems
    
    cc: Eric D eric.decosta@gmail.com

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1317%2Fedecosta-mw%2Fmaster-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1317/edecosta-mw/master-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/1317

Range-diff vs v2:

 1:  7e67ce8c944 ! 1:  6c5f176cbee fsmonitor: option to allow fsmonitor to run against network-mounted repos
     @@ Commit message
          true will override the default behavior (erroring-out) when a
          network-mounted repo is detected by fsmonitor.
      
     -    Additionally, as part of this first step, monitoring of network-mounted
     -    repos will be restricted to those mounted over SMB regardless of the
     -    value of 'fsmonitor.allowRemote' until more extensive testing can be
     -    performed.
     -
          Signed-off-by: Eric DeCosta <edecosta@mathworks.com>
      
       ## compat/fsmonitor/fsm-settings-win32.c ##
     @@ compat/fsmonitor/fsm-settings-win32.c: static enum fsmonitor_reason check_vfs4gi
      +/*
      + * 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.
     -+*/
     -+static enum fsmonitor_reason check_allow_remote(struct repository *r)
     ++ * 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.
     ++ */
     ++static int check_config_allowremote(struct repository *r)
      +{
      +	int allow;
      +
     -+	if (repo_config_get_bool(r, "fsmonitor.allowremote", &allow) || !allow)
     -+		return FSMONITOR_REASON_REMOTE;
     ++	if (!repo_config_get_bool(r, "fsmonitor.allowremote", &allow))
     ++		return allow;
      +
     -+	return FSMONITOR_REASON_OK;
     ++	return -1; /* fsmonitor.allowremote not set */
      +}
      +
      +/*
     -+ * Check if the remote working directory is mounted via SMB
     ++ * Check remote working directory protocol.
      + *
     -+ * For now, remote working directories are only supported via SMB mounts
     -+*/
     -+static enum fsmonitor_reason check_smb(wchar_t *wpath)
     ++ * Error if client machine cannot get remote protocol information.
     ++ */
     ++static void check_remote_protocol(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);
     ++			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;
     ++		return;
      +	}
      +
      +	if (!GetFileInformationByHandleEx(h, FileRemoteProtocolInfo,
     -+									&proto_info, sizeof(proto_info))) {
     ++		&proto_info, sizeof(proto_info))) {
      +		error(_("[GLE %ld] unable to get protocol information for '%ls'"),
      +		      GetLastError(), wpath);
      +		CloseHandle(h);
     -+		return FSMONITOR_REASON_ERROR;
     ++		return;
      +	}
      +
      +	CloseHandle(h);
      +
     -+	if (proto_info.Protocol == WNNC_NET_SMB)
     -+		return FSMONITOR_REASON_OK;
     ++	trace_printf_key(&trace_fsmonitor,
     ++				"check_remote_protocol('%ls') remote protocol %#8.8lx",
     ++				wpath, proto_info.Protocol);
      +
     -+	return FSMONITOR_REASON_ERROR;
     ++	return;
      +}
      +
       /*
        * Remote working directories are problematic for FSMonitor.
        *
     -@@ compat/fsmonitor/fsm-settings-win32.c: 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;
      @@ compat/fsmonitor/fsm-settings-win32.c: 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);
     ++		check_remote_protocol(wfullpath);
     ++
     ++		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;
       	}
       
     - 	return FSMONITOR_REASON_OK;
 2:  7a071c9e6be < -:  ----------- fsmonitor.allowRemote now overrides default behavior


 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.
+ */
+static int check_config_allowremote(struct repository *r)
+{
+	int allow;
+
+	if (!repo_config_get_bool(r, "fsmonitor.allowremote", &allow))
+		return allow;
+
+	return -1; /* fsmonitor.allowremote not set */
+}
+
+/*
+ * Check remote working directory protocol.
+ *
+ * Error if client machine cannot get remote protocol information.
+ */
+static void check_remote_protocol(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;
+	}
+
+	if (!GetFileInformationByHandleEx(h, FileRemoteProtocolInfo,
+		&proto_info, sizeof(proto_info))) {
+		error(_("[GLE %ld] unable to get protocol information for '%ls'"),
+		      GetLastError(), wpath);
+		CloseHandle(h);
+		return;
+	}
+
+	CloseHandle(h);
+
+	trace_printf_key(&trace_fsmonitor,
+				"check_remote_protocol('%ls') remote protocol %#8.8lx",
+				wpath, proto_info.Protocol);
+
+	return;
+}
+
 /*
  * 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);
+
+		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;
 	}
 

base-commit: c50926e1f48891e2671e1830dbcd2912a4563450
-- 
gitgitgadget

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

[Junio C Hamano]

"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.

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

[Eric DeCosta via GitGitGadget]

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>
---
    Option to allow fsmonitor to run against repos on network file systems
    
    cc: Eric D eric.decosta@gmail.com

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1317%2Fedecosta-mw%2Fmaster-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1317/edecosta-mw/master-v4
Pull-Request: https://github.com/gitgitgadget/git/pull/1317

Range-diff vs v3:

 1:  6c5f176cbee ! 1:  058dc400c8a fsmonitor: option to allow fsmonitor to run against network-mounted repos
     @@ compat/fsmonitor/fsm-settings-win32.c: static enum fsmonitor_reason check_vfs4gi
      + * 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
     ++ * disabled.  Users may override this behavior in enviroments where
      + * they have proper support.
      + */
      +static int check_config_allowremote(struct repository *r)
     @@ compat/fsmonitor/fsm-settings-win32.c: static enum fsmonitor_reason check_vfs4gi
      + *
      + * Error if client machine cannot get remote protocol information.
      + */
     -+static void check_remote_protocol(wchar_t *wpath)
     ++static int check_remote_protocol(wchar_t *wpath)
      +{
      +	HANDLE h;
      +	FILE_REMOTE_PROTOCOL_INFO proto_info;
     @@ compat/fsmonitor/fsm-settings-win32.c: static enum fsmonitor_reason check_vfs4gi
      +	if (h == INVALID_HANDLE_VALUE) {
      +		error(_("[GLE %ld] unable to open for read '%ls'"),
      +		      GetLastError(), wpath);
     -+		return;
     ++		return -1;
      +	}
      +
      +	if (!GetFileInformationByHandleEx(h, FileRemoteProtocolInfo,
     @@ compat/fsmonitor/fsm-settings-win32.c: static enum fsmonitor_reason check_vfs4gi
      +		error(_("[GLE %ld] unable to get protocol information for '%ls'"),
      +		      GetLastError(), wpath);
      +		CloseHandle(h);
     -+		return;
     ++		return -1;
      +	}
      +
      +	CloseHandle(h);
     @@ compat/fsmonitor/fsm-settings-win32.c: static enum fsmonitor_reason check_vfs4gi
      +				"check_remote_protocol('%ls') remote protocol %#8.8lx",
      +				wpath, proto_info.Protocol);
      +
     -+	return;
     ++	return 0;
      +}
      +
       /*
        * Remote working directories are problematic for FSMonitor.
        *
     +@@ compat/fsmonitor/fsm-settings-win32.c: static enum fsmonitor_reason check_vfs4git(struct repository *r)
     +  */
     + static enum fsmonitor_reason check_remote(struct repository *r)
     + {
     ++	int ret;
     + 	wchar_t wpath[MAX_PATH];
     + 	wchar_t wfullpath[MAX_PATH];
     + 	size_t wlen;
      @@ compat/fsmonitor/fsm-settings-win32.c: static enum fsmonitor_reason check_remote(struct repository *r)
       		trace_printf_key(&trace_fsmonitor,
       				 "check_remote('%s') true",
       				 r->worktree);
      +
     -+		check_remote_protocol(wfullpath);
     ++		ret = check_remote_protocol(wfullpath);
     ++		if (ret < 0)
     ++			return FSMONITOR_REASON_ERROR;
      +
      +		switch (check_config_allowremote(r)) {
      +		case 0: /* config overrides and disables */


 compat/fsmonitor/fsm-settings-win32.c | 68 +++++++++++++++++++++++++++
 1 file changed, 68 insertions(+)

diff --git a/compat/fsmonitor/fsm-settings-win32.c b/compat/fsmonitor/fsm-settings-win32.c
index 907655720bb..e5ec5b0a9f7 100644
--- a/compat/fsmonitor/fsm-settings-win32.c
+++ b/compat/fsmonitor/fsm-settings-win32.c
@@ -24,6 +24,59 @@ 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.  Users may override this behavior in enviroments where
+ * they have proper support.
+ */
+static int check_config_allowremote(struct repository *r)
+{
+	int allow;
+
+	if (!repo_config_get_bool(r, "fsmonitor.allowremote", &allow))
+		return allow;
+
+	return -1; /* fsmonitor.allowremote not set */
+}
+
+/*
+ * Check remote working directory protocol.
+ *
+ * Error if client machine cannot get remote protocol information.
+ */
+static int check_remote_protocol(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 -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 -1;
+	}
+
+	CloseHandle(h);
+
+	trace_printf_key(&trace_fsmonitor,
+				"check_remote_protocol('%ls') remote protocol %#8.8lx",
+				wpath, proto_info.Protocol);
+
+	return 0;
+}
+
 /*
  * Remote working directories are problematic for FSMonitor.
  *
@@ -76,6 +129,7 @@ static enum fsmonitor_reason check_vfs4git(struct repository *r)
  */
 static enum fsmonitor_reason check_remote(struct repository *r)
 {
+	int ret;
 	wchar_t wpath[MAX_PATH];
 	wchar_t wfullpath[MAX_PATH];
 	size_t wlen;
@@ -115,6 +169,20 @@ static enum fsmonitor_reason check_remote(struct repository *r)
 		trace_printf_key(&trace_fsmonitor,
 				 "check_remote('%s') true",
 				 r->worktree);
+
+		ret = check_remote_protocol(wfullpath);
+		if (ret < 0)
+			return FSMONITOR_REASON_ERROR;
+
+		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;
 	}
 

base-commit: c50926e1f48891e2671e1830dbcd2912a4563450
-- 
gitgitgadget

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

[Junio C Hamano]

"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,

"uses cases" -> "use cases", probably.

> 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>
> ---

Will make the above typofix while queuing.

Thanks.

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

[Jeff Hostetler]

On 8/11/22 7:57 PM, Eric DeCosta via GitGitGadget wrote:
> 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.

V4 LGTM.  Thanks for your persistence and attention to detail here.

Jeff

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

[Junio C Hamano]

Jeff Hostetler <git@jeffhostetler.com> writes:

> On 8/11/22 7:57 PM, Eric DeCosta via GitGitGadget wrote:
>> 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.
>
> V4 LGTM.  Thanks for your persistence and attention to detail here.

Thanks, both.  Queued.