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

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

As a first step towards enabling fsmonitor to work against
network-mounted repos, a configuration option, 'fsmonitor.allowRemote'
was introduced for Windows. Setting this option to true will override
the default behavior (erroring-out) when a network-mounted repo is
detected by fsmonitor. In order for macOS to have parity with Windows,
the same option is now introduced for macOS.

The the added wrinkle being that the Unix domain socket (UDS) file
used for IPC cannot be created in a network location; instead the
temporary directory is used.

Signed-off-by: Eric DeCosta <edecosta@mathworks.com>
---
    fsmonitor: option to allow fsmonitor to run against network-mounted
    repos

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

 compat/fsmonitor/fsm-settings-darwin.c | 77 ++++++++++++++++++++++----
 fsmonitor-ipc.c                        | 47 +++++++++++++++-
 fsmonitor-ipc.h                        |  6 ++
 3 files changed, 117 insertions(+), 13 deletions(-)

diff --git a/compat/fsmonitor/fsm-settings-darwin.c b/compat/fsmonitor/fsm-settings-darwin.c
index efc732c0f31..9e2ea3b90cc 100644
--- a/compat/fsmonitor/fsm-settings-darwin.c
+++ b/compat/fsmonitor/fsm-settings-darwin.c
@@ -2,10 +2,28 @@
 #include "config.h"
 #include "repository.h"
 #include "fsmonitor-settings.h"
+#include "fsmonitor-ipc.h"
 #include "fsmonitor.h"
 #include <sys/param.h>
 #include <sys/mount.h>
 
+/*
+ * 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 */
+}
+
 /*
  * [1] Remote working directories are problematic for FSMonitor.
  *
@@ -27,24 +45,22 @@
  * In theory, the above issues need to be addressed whether we are
  * using the Hook or IPC API.
  *
+ * So (for now at least), mark remote working directories as
+ * incompatible by default.
+ *
  * For the builtin FSMonitor, we create the Unix domain socket for the
- * IPC in the .git directory.  If the working directory is remote,
- * then the socket will be created on the remote file system.  This
- * can fail if the remote file system does not support UDS file types
- * (e.g. smbfs to a Windows server) or if the remote kernel does not
- * allow a non-local process to bind() the socket.  (These problems
- * could be fixed by moving the UDS out of the .git directory and to a
- * well-known local directory on the client machine, but care should
- * be taken to ensure that $HOME is actually local and not a managed
- * file share.)
+ * IPC in the temporary directory.  If the temporary directory is
+ * remote, then the socket will be created on the remote file system.
+ * This can fail if the remote file system does not support UDS file
+ * types (e.g. smbfs to a Windows server) or if the remote kernel does
+ * not allow a non-local process to bind() the socket.
  *
- * So (for now at least), mark remote working directories as
- * incompatible.
+ * Therefore remote UDS locations are marked as incompatible.
  *
  *
  * [2] FAT32 and NTFS working directories are problematic too.
  *
- * The builtin FSMonitor uses a Unix domain socket in the .git
+ * The builtin FSMonitor uses a Unix domain socket in the temporary
  * directory for IPC.  These Windows drive formats do not support
  * Unix domain sockets, so mark them as incompatible for the daemon.
  *
@@ -65,6 +81,39 @@ static enum fsmonitor_reason check_volume(struct repository *r)
 			 "statfs('%s') [type 0x%08x][flags 0x%08x] '%s'",
 			 r->worktree, fs.f_type, fs.f_flags, fs.f_fstypename);
 
+	if (!(fs.f_flags & MNT_LOCAL)) {
+		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;
+}
+
+static enum fsmonitor_reason check_uds_volume(void)
+{
+	struct statfs fs;
+	const char *path = fsmonitor_ipc__get_path();
+
+	if (statfs(path, &fs) == -1) {
+		int saved_errno = errno;
+		trace_printf_key(&trace_fsmonitor, "statfs('%s') failed: %s",
+				 path, strerror(saved_errno));
+		errno = saved_errno;
+		return FSMONITOR_REASON_ERROR;
+	}
+
+	trace_printf_key(&trace_fsmonitor,
+			 "statfs('%s') [type 0x%08x][flags 0x%08x] '%s'",
+			 path, fs.f_type, fs.f_flags, fs.f_fstypename);
+
 	if (!(fs.f_flags & MNT_LOCAL))
 		return FSMONITOR_REASON_REMOTE;
 
@@ -85,5 +134,9 @@ enum fsmonitor_reason fsm_os__incompatible(struct repository *r)
 	if (reason != FSMONITOR_REASON_OK)
 		return reason;
 
+	reason = check_uds_volume();
+	if (reason != FSMONITOR_REASON_OK)
+		return reason;
+
 	return FSMONITOR_REASON_OK;
 }
diff --git a/fsmonitor-ipc.c b/fsmonitor-ipc.c
index 789e7397baa..6e9b40a03d5 100644
--- a/fsmonitor-ipc.c
+++ b/fsmonitor-ipc.c
@@ -4,6 +4,7 @@
 #include "fsmonitor-ipc.h"
 #include "run-command.h"
 #include "strbuf.h"
+#include "tempfile.h"
 #include "trace2.h"
 
 #ifndef HAVE_FSMONITOR_DAEMON_BACKEND
@@ -47,7 +48,51 @@ int fsmonitor_ipc__is_supported(void)
 	return 1;
 }
 
-GIT_PATH_FUNC(fsmonitor_ipc__get_path, "fsmonitor--daemon.ipc")
+GIT_PATH_FUNC(fsmonitor_ipc__get_pathfile, "fsmonitor--daemon.ipc")
+
+static char *gen_ipc_file(void)
+{
+	char *retval = NULL;
+	struct tempfile *ipc;
+
+	const char *ipc_file = fsmonitor_ipc__get_pathfile();
+	FILE *fp = fopen(ipc_file, "w");
+
+	if (!fp)
+		die_errno("error opening '%s'", ipc_file);
+	ipc = mks_tempfile_t("fsmonitor_ipc_XXXXXX");
+	strbuf_write(&ipc->filename, fp);
+	fclose(fp);
+	retval = strbuf_detach(&ipc->filename, NULL);
+	strbuf_release(&ipc->filename);
+	return retval;
+}
+
+const char *fsmonitor_ipc__get_path(void)
+{
+	char *retval = NULL;
+	struct strbuf sb = STRBUF_INIT;
+
+	const char *ipc_file = fsmonitor_ipc__get_pathfile();
+	FILE *fp = fopen(ipc_file, "r");
+
+	if (!fp) {
+		return gen_ipc_file();
+	} else {
+		strbuf_read(&sb, fileno(fp), 0);
+		fclose(fp);
+		fp = fopen(sb.buf, "r");
+		if (!fp) { /* generate new file */
+			if (unlink(ipc_file) < 0)
+				die_errno("could not remove '%s'", ipc_file);
+			return gen_ipc_file();
+		}
+		fclose(fp);
+		retval = strbuf_detach(&sb, NULL);
+		strbuf_release(&sb);
+		return retval;
+	}
+}
 
 enum ipc_active_state fsmonitor_ipc__get_state(void)
 {
diff --git a/fsmonitor-ipc.h b/fsmonitor-ipc.h
index b6a7067c3af..63277dea39e 100644
--- a/fsmonitor-ipc.h
+++ b/fsmonitor-ipc.h
@@ -18,6 +18,12 @@ int fsmonitor_ipc__is_supported(void);
  */
 const char *fsmonitor_ipc__get_path(void);
 
+/*
+ * Returns the pathname to the file that contains the pathname to the
+ * IPC named pipe or Unix domain socket.
+ */
+const char *fsmonitor_ipc__get_pathfile(void);
+
 /*
  * Try to determine whether there is a `git-fsmonitor--daemon` process
  * listening on the IPC pipe/socket.

base-commit: 9bf691b78cf906751e65d65ba0c6ffdcd9a5a12c
-- 
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:

> 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.
>
> As a first step towards enabling fsmonitor to work ...

Didn't we already see the first step and got it reviewed?  I think
I've already merged the last round to 'next'.

Puzzled...

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> "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.
>>
>> As a first step towards enabling fsmonitor to work ...
>
> Didn't we already see the first step and got it reviewed?  I think
> I've already merged the last round to 'next'.
>
> Puzzled...

Ah, OK, so this is not exactly the "first step", but builds on top
of the previous one?  At least the patch needs a better title to
make it distinguishable from the other one.

Will queue to wait for others to review.

Thanks.

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

[Johannes Schindelin]

Hi Eric,

On Thu, 18 Aug 2022, 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.
>
> As a first step towards enabling fsmonitor to work against
> network-mounted repos, a configuration option, 'fsmonitor.allowRemote'
> was introduced for Windows.

If you start the commit message along the following lines, it might be
easier/quicker to grok the context for the keen reader:

	In 85dc0da6dcf (fsmonitor: option to allow fsmonitor to run against
	network-mounted repos, 2022-08-11), the Windows backend of the
	FSMonitor learned to allow running on network drives, via the
	`fsmonitor.allowRemote` config setting.

> Setting this option to true will override the default behavior
> (erroring-out) when a network-mounted repo is detected by fsmonitor. In
> order for macOS to have parity with Windows, the same option is now
> introduced for macOS.
>
> The the added wrinkle being that the Unix domain socket (UDS) file
> used for IPC cannot be created in a network location; instead the
> temporary directory is used.

Thank you very much for this note, after a cursory read I expected that
part of the code to be a left-over from some "We know better than the
user" type of automatic default, and this paragraph definitely helped me
overcome that expectation.

>
> Signed-off-by: Eric DeCosta <edecosta@mathworks.com>
> ---
>     fsmonitor: option to allow fsmonitor to run against network-mounted
>     repos
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1326%2Fedecosta-mw%2Ffsmonitor_macos-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1326/edecosta-mw/fsmonitor_macos-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/1326
>
>  compat/fsmonitor/fsm-settings-darwin.c | 77 ++++++++++++++++++++++----
>  fsmonitor-ipc.c                        | 47 +++++++++++++++-
>  fsmonitor-ipc.h                        |  6 ++
>  3 files changed, 117 insertions(+), 13 deletions(-)

I am somewhat puzzled that this has no corresponding change to
`Documentation/`.

And now I realize that this was the case also for the patch adding
`fsmonitor.allowRemote` support for Windows.

Could I ask you to add a patch to document this config setting?

>
> diff --git a/compat/fsmonitor/fsm-settings-darwin.c b/compat/fsmonitor/fsm-settings-darwin.c
> index efc732c0f31..9e2ea3b90cc 100644
> --- a/compat/fsmonitor/fsm-settings-darwin.c
> +++ b/compat/fsmonitor/fsm-settings-darwin.c
> @@ -2,10 +2,28 @@
>  #include "config.h"
>  #include "repository.h"
>  #include "fsmonitor-settings.h"
> +#include "fsmonitor-ipc.h"
>  #include "fsmonitor.h"
>  #include <sys/param.h>
>  #include <sys/mount.h>
>
> +/*
> + * 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 */
> +}
> +
>  /*
>   * [1] Remote working directories are problematic for FSMonitor.
>   *
> @@ -27,24 +45,22 @@
>   * In theory, the above issues need to be addressed whether we are
>   * using the Hook or IPC API.
>   *
> + * So (for now at least), mark remote working directories as
> + * incompatible by default.
> + *

This was moved up, okay.

>   * For the builtin FSMonitor, we create the Unix domain socket for the
> - * IPC in the .git directory.  If the working directory is remote,
> - * then the socket will be created on the remote file system.  This
> - * can fail if the remote file system does not support UDS file types
> - * (e.g. smbfs to a Windows server) or if the remote kernel does not
> - * allow a non-local process to bind() the socket.  (These problems
> - * could be fixed by moving the UDS out of the .git directory and to a
> - * well-known local directory on the client machine, but care should
> - * be taken to ensure that $HOME is actually local and not a managed
> - * file share.)
> + * IPC in the temporary directory.  If the temporary directory is

This is incorrect. It is still the `.git` directory in the common case,
not a temporary directory.

> + * remote, then the socket will be created on the remote file system.
> + * This can fail if the remote file system does not support UDS file
> + * types (e.g. smbfs to a Windows server) or if the remote kernel does
> + * not allow a non-local process to bind() the socket.
>   *
> - * So (for now at least), mark remote working directories as
> - * incompatible.
> + * Therefore remote UDS locations are marked as incompatible.
>   *
>   *
>   * [2] FAT32 and NTFS working directories are problematic too.

Doesn't this patch address this, too? See below for more on that.

>   *
> - * The builtin FSMonitor uses a Unix domain socket in the .git
> + * The builtin FSMonitor uses a Unix domain socket in the temporary
>   * directory for IPC.  These Windows drive formats do not support
>   * Unix domain sockets, so mark them as incompatible for the daemon.
>   *
> @@ -65,6 +81,39 @@ static enum fsmonitor_reason check_volume(struct repository *r)
>  			 "statfs('%s') [type 0x%08x][flags 0x%08x] '%s'",
>  			 r->worktree, fs.f_type, fs.f_flags, fs.f_fstypename);
>
> +	if (!(fs.f_flags & MNT_LOCAL)) {
> +		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;
> +	}

This `switch()` statement sounds like a verbose way to say the same as:

		return check_config_allowremote(r) == 1 ?
			FSMONITOR_REASON_OK : FSMONITOR_REASON_REMOTE;

> +
> +	return FSMONITOR_REASON_OK;
> +}
> +
> +static enum fsmonitor_reason check_uds_volume(void)

What's an UDS volume? Do you mean to say "Unix Domain Socket Volume"?

If so, it would be better to turn this into a function called
`filesystem_supports_unix_sockets()` and to return an `int`, 1 for "yes",
0 for "no".

> +{
> +	struct statfs fs;
> +	const char *path = fsmonitor_ipc__get_path();
> +
> +	if (statfs(path, &fs) == -1) {
> +		int saved_errno = errno;
> +		trace_printf_key(&trace_fsmonitor, "statfs('%s') failed: %s",
> +				 path, strerror(saved_errno));
> +		errno = saved_errno;
> +		return FSMONITOR_REASON_ERROR;
> +	}
> +
> +	trace_printf_key(&trace_fsmonitor,
> +			 "statfs('%s') [type 0x%08x][flags 0x%08x] '%s'",
> +			 path, fs.f_type, fs.f_flags, fs.f_fstypename);
> +
>  	if (!(fs.f_flags & MNT_LOCAL))
>  		return FSMONITOR_REASON_REMOTE;
>
> @@ -85,5 +134,9 @@ enum fsmonitor_reason fsm_os__incompatible(struct repository *r)

It is unfortunate that the diff hunk stops here, and mails lack a button
to increase the diff context. In this instance, the hidden part of the
`check_volume()` function is quite interesting: it returns
`FSMONITOR_REASON_NOSOCKETS` for `msdos` and `ntfs` file systems.

Which means that your patch changes behavior not only for remote file
systems, but also for local ones without support for Unix sockets.

To heed the principle of separation of concerns, please do split out that
part. I would recommend to make it the first patch to support
`msdos`/`ntfs` file systems (by registering the Unix sockets in a
temporary directory instead of the `.git/` directory). The second patch
can then introduce support for `fsmonitor.allowRemote` on macOS on top of
the first patch.

>  	if (reason != FSMONITOR_REASON_OK)
>  		return reason;
>
> +	reason = check_uds_volume();
> +	if (reason != FSMONITOR_REASON_OK)
> +		return reason;
> +
>  	return FSMONITOR_REASON_OK;
>  }
> diff --git a/fsmonitor-ipc.c b/fsmonitor-ipc.c
> index 789e7397baa..6e9b40a03d5 100644
> --- a/fsmonitor-ipc.c
> +++ b/fsmonitor-ipc.c
> @@ -4,6 +4,7 @@
>  #include "fsmonitor-ipc.h"
>  #include "run-command.h"
>  #include "strbuf.h"
> +#include "tempfile.h"
>  #include "trace2.h"
>
>  #ifndef HAVE_FSMONITOR_DAEMON_BACKEND
> @@ -47,7 +48,51 @@ int fsmonitor_ipc__is_supported(void)
>  	return 1;
>  }
>
> -GIT_PATH_FUNC(fsmonitor_ipc__get_path, "fsmonitor--daemon.ipc")
> +GIT_PATH_FUNC(fsmonitor_ipc__get_pathfile, "fsmonitor--daemon.ipc")

Why rename this? That's unnecessary chatter in the patch. Let's avoid such
things in the future, it only costs reviewers time.

> +
> +static char *gen_ipc_file(void)
> +{
> +	char *retval = NULL;
> +	struct tempfile *ipc;
> +
> +	const char *ipc_file = fsmonitor_ipc__get_pathfile();
> +	FILE *fp = fopen(ipc_file, "w");
> +
> +	if (!fp)
> +		die_errno("error opening '%s'", ipc_file);
> +	ipc = mks_tempfile_t("fsmonitor_ipc_XXXXXX");
> +	strbuf_write(&ipc->filename, fp);
> +	fclose(fp);
> +	retval = strbuf_detach(&ipc->filename, NULL);
> +	strbuf_release(&ipc->filename);
> +	return retval;
> +}
> +
> +const char *fsmonitor_ipc__get_path(void)
> +{
> +	char *retval = NULL;
> +	struct strbuf sb = STRBUF_INIT;
> +
> +	const char *ipc_file = fsmonitor_ipc__get_pathfile();
> +	FILE *fp = fopen(ipc_file, "r");
> +
> +	if (!fp) {
> +		return gen_ipc_file();
> +	} else {
> +		strbuf_read(&sb, fileno(fp), 0);
> +		fclose(fp);
> +		fp = fopen(sb.buf, "r");
> +		if (!fp) { /* generate new file */
> +			if (unlink(ipc_file) < 0)
> +				die_errno("could not remove '%s'", ipc_file);
> +			return gen_ipc_file();
> +		}
> +		fclose(fp);
> +		retval = strbuf_detach(&sb, NULL);
> +		strbuf_release(&sb);
> +		return retval;
> +	}
> +}

I am afraid I do not understand how this code can guarantee a fixed path
for the Unix domain socket.

It _needs_ to be fixed so that a singleton daemon can run and listen on
it, and an arbitrary number of Git clients can connect to it.

If it is not fixed, you will cause Git to quite possibly start a new
FSMonitor daemon for every invocation that wants to connect to an
FSMonitor daemon.

This means that the path of the Unix socket needs to have a 1:1
relationship to the path of the `.git/` directory. If you install it in
that directory, that invariant is naturally fulfilled. If you want to
install it elsewhere, you will have to come up with a reliable way to
guarantee that connection.

One option would be to install the Unix sockets in the home directory,
under a name like `.git-fsmonitor-<hash>` where the <hash> is e.g. a
SHA-1/SHA-256 of the canonicalized path of the `.git/` directory.

>
>  enum ipc_active_state fsmonitor_ipc__get_state(void)
>  {
> diff --git a/fsmonitor-ipc.h b/fsmonitor-ipc.h
> index b6a7067c3af..63277dea39e 100644
> --- a/fsmonitor-ipc.h
> +++ b/fsmonitor-ipc.h
> @@ -18,6 +18,12 @@ int fsmonitor_ipc__is_supported(void);
>   */
>  const char *fsmonitor_ipc__get_path(void);
>
> +/*
> + * Returns the pathname to the file that contains the pathname to the
> + * IPC named pipe or Unix domain socket.
> + */
> +const char *fsmonitor_ipc__get_pathfile(void);
> +
>  /*
>   * Try to determine whether there is a `git-fsmonitor--daemon` process
>   * listening on the IPC pipe/socket.

Thank you for working on this, also on the Windows side. It definitely
helps!

Ciao,
Dscho

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

[Jeff Hostetler]

On 8/18/22 4:48 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.
> 
> As a first step towards enabling fsmonitor to work against
> network-mounted repos, a configuration option, 'fsmonitor.allowRemote'
> was introduced for Windows. Setting this option to true will override
> the default behavior (erroring-out) when a network-mounted repo is
> detected by fsmonitor. In order for macOS to have parity with Windows,
> the same option is now introduced for macOS.

We might also say that this config option only allows FSMonitor
to TRY to consider using a network-mounted repo.  And that this
ability is considered experimental until sufficient testing can
be completed and we can determine the combinations of
{ client os } x { server os } x { remote access } x { file system type }
that are known to work or not work and we can update the defaults
and the documentation accordingly.

For example, on a MacOS client, we expect the local "fseventsd" service
to send us recursive events on all files and sub directories under the
repo root.  If the server is a Linux machine (which doesn't really do
recursive events), does exporting the FS from the server over NFS or SMB
(or whatever) cause the Linux host to send enough information to the
client machine for fseventsd to synthesize the recursive event stream
locally that FSMonitor expects.  It might.  It might not.  That
combination should be tested (along with a lot of other combinations).

But again, this patch is just about allowing the (informed?) user to
try it and begin testing various combinations.


> 
> The the added wrinkle being that the Unix domain socket (UDS) file
> used for IPC cannot be created in a network location; instead the
> temporary directory is used.

This scares me a bit.  I put the socket in the .git directory
so that we are guaranteed that only one daemon will run on the
repository and that all clients will know where to find that socket
(if it exists).

It looks like you're creating the UDS using a tmp pathname and
writing the pathname to the actual .git/fsmonitor--daemon.ipc FILE.
This adds a layer of indirection and is prone to races.


The act of creating the actual socket is protected by code in
unix-socket.c and unix-stream-server.c to try to safely create
the socket and avoid stepping on another active daemon (who
currently has the server-side of the socket open).

My code also detects dead sockets (where a previous daemon died
and failed to delete the socket).


Additionally, allowing remote access means that the repo could
be used by multiple client machines and/or by the server machine
itself.  Consider the example of two MacOS clients mounting the
remote repo and both wanting to start FSMonitor.  They would
constantly fight to recreate a new local-tmp-based socket and
update your pathname FILE and end up invalidating each other on
each command.


Also, if someone overwrites your new pathname FILE, but doesn't tell
the daemon, the daemon will be orphaned -- still running, but no one
will ever connect to it because the FILE no longer points to it.


There was a suggestion later in this thread about using a SHA-1
or SHA-256 hash of the pathname to avoid the tmp XXXXXX pattern
and just put the socket in $HOME (and omit the need for the new
fsmonitor-daemon.ipc FILE completely).  This might work, but we
need to be careful because a user might have hardlinks or symlinks
locally so there may be more than one unique path to the repo
on the local system.  (It is OK to have more than one daemon
listening to a repo, just less efficient.)


As an interim step, you might try using my original socket code
plus just the config.allowRemote=true change.  And test it on a
mounted repo where you've converted the .git directory to a .git
file and moved contents of the .git directory to somewhere local.
Then the UDS would be created in the local GITDIR instead of on
the remote system.  This won't help any of the sharing cases I
described above, but will let you experiment with getting remote
events.

Jeff



> base-commit: 9bf691b78cf906751e65d65ba0c6ffdcd9a5a12c
> 

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

[Eric Sunshine]

On Thu, Aug 18, 2022 at 5:00 PM Eric DeCosta via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> Though perhaps not common, there are uses cases where users have large,

s/uses cases/use cases/

> network-mounted repos. Having the ability to run fsmonitor against
> network paths would benefit those users.
>
> As a first step towards enabling fsmonitor to work against
> network-mounted repos, a configuration option, 'fsmonitor.allowRemote'
> was introduced for Windows. Setting this option to true will override
> the default behavior (erroring-out) when a network-mounted repo is
> detected by fsmonitor. In order for macOS to have parity with Windows,
> the same option is now introduced for macOS.
>
> The the added wrinkle being that the Unix domain socket (UDS) file

s/The the/The/

> used for IPC cannot be created in a network location; instead the
> temporary directory is used.
>
> Signed-off-by: Eric DeCosta <edecosta@mathworks.com>

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

[Eric DeCosta]

> -----Original Message-----
> From: Jeff Hostetler <git@jeffhostetler.com>
> Sent: Friday, August 19, 2022 12:50 PM
> To: Eric DeCosta via GitGitGadget <gitgitgadget@gmail.com>;
> git@vger.kernel.org
> Cc: Eric DeCosta <edecosta@mathworks.com>
> Subject: Re: [PATCH] fsmonitor: option to allow fsmonitor to run against
> network-mounted repos
> 
> 
> 
> On 8/18/22 4:48 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.
> >
> > As a first step towards enabling fsmonitor to work against
> > network-mounted repos, a configuration option, 'fsmonitor.allowRemote'
> > was introduced for Windows. Setting this option to true will override
> > the default behavior (erroring-out) when a network-mounted repo is
> > detected by fsmonitor. In order for macOS to have parity with Windows,
> > the same option is now introduced for macOS.
> 
> We might also say that this config option only allows FSMonitor
> to TRY to consider using a network-mounted repo.  And that this
> ability is considered experimental until sufficient testing can
> be completed and we can determine the combinations of
> { client os } x { server os } x { remote access } x { file system type }
> that are known to work or not work and we can update the defaults
> and the documentation accordingly.
> 
Yes, very experimental. 

> For example, on a MacOS client, we expect the local "fseventsd" service
> to send us recursive events on all files and sub directories under the
> repo root.  If the server is a Linux machine (which doesn't really do
> recursive events), does exporting the FS from the server over NFS or SMB
> (or whatever) cause the Linux host to send enough information to the
> client machine for fseventsd to synthesize the recursive event stream
> locally that FSMonitor expects.  It might.  It might not.  That
> combination should be tested (along with a lot of other combinations).
> 
> But again, this patch is just about allowing the (informed?) user to
> try it and begin testing various combinations.
> 
Yes, the point is to allow users to try it out.  Self-servingly, I have about
3K users who make heavy use of network-mounted sandboxes on the
three major platforms; all connecting via NFS or SMB to Linux file
servers.  Hardly exhaustive, but the file system change notification APIs
(inotify, FSEvents, and ReadDirectoryCHangesW) all seem to work correctly.
Thus my motivation to work on this aspect of git :-)

> 
> >
> > The the added wrinkle being that the Unix domain socket (UDS) file
> > used for IPC cannot be created in a network location; instead the
> > temporary directory is used.
> 
> This scares me a bit.  I put the socket in the .git directory
> so that we are guaranteed that only one daemon will run on the
> repository and that all clients will know where to find that socket
> (if it exists).
> 
> It looks like you're creating the UDS using a tmp pathname and
> writing the pathname to the actual .git/fsmonitor--daemon.ipc FILE.
> This adds a layer of indirection and is prone to races.
> 
Good point.

> 
> The act of creating the actual socket is protected by code in
> unix-socket.c and unix-stream-server.c to try to safely create
> the socket and avoid stepping on another active daemon (who
> currently has the server-side of the socket open).
> 
> My code also detects dead sockets (where a previous daemon died
> and failed to delete the socket).
> 
> 
> Additionally, allowing remote access means that the repo could
> be used by multiple client machines and/or by the server machine
> itself.  Consider the example of two MacOS clients mounting the
> remote repo and both wanting to start FSMonitor.  They would
> constantly fight to recreate a new local-tmp-based socket and
> update your pathname FILE and end up invalidating each other on
> each command.
> 
I see your point - they'd stomp on each other. 

As far as multiple client machines mounting the remote repo, I have
doubts that FSMonitor would even see changes made from another
machine. Worth trying out and documenting as needed - might even
be better off being considered as unsupported.

> 
> Also, if someone overwrites your new pathname FILE, but doesn't tell
> the daemon, the daemon will be orphaned -- still running, but no one
> will ever connect to it because the FILE no longer points to it.
> 
True, thanks for pointing that out.

> 
> There was a suggestion later in this thread about using a SHA-1
> or SHA-256 hash of the pathname to avoid the tmp XXXXXX pattern
> and just put the socket in $HOME (and omit the need for the new
> fsmonitor-daemon.ipc FILE completely).  This might work, but we
> need to be careful because a user might have hardlinks or symlinks
> locally so there may be more than one unique path to the repo
> on the local system.  (It is OK to have more than one daemon
> listening to a repo, just less efficient.)
> 
Ah, I see.

> 
> As an interim step, you might try using my original socket code
> plus just the config.allowRemote=true change.  And test it on a
> mounted repo where you've converted the .git directory to a .git
> file and moved contents of the .git directory to somewhere local.
> Then the UDS would be created in the local GITDIR instead of on
> the remote system.  This won't help any of the sharing cases I
> described above, but will let you experiment with getting remote
> events.
> 
Within the context of the environment that I have available to me
(macOS over NFS to a Linux file server), FSEvents is working correctly.
I can make changes at any arbitrary place inside of the repo and an
event is generated. 

It's looking like that the Unix domain socket (UDS) file should remain
where it is unless fsmonitor.allowRemote is true.

If fsmonitor.allowRemote is true then the UDS file can be located
in $HOME with the caveat that if there is more than one path to
the repo (via hard or sym links) that things might not work as
expected.  I think that's OK given the experimental nature of the
feature.

-Eric


> Jeff
> 
> 
> 
> > base-commit: 9bf691b78cf906751e65d65ba0c6ffdcd9a5a12c
> >

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

[Torsten Bögershausen]

On Thu, Aug 18, 2022 at 08:48:23PM +0000, Eric DeCosta via GitGitGadget wrote:
> From: Eric DeCosta <edecosta@mathworks.com>

Just some comments on the commit message, please see inline.

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

I think that we can drop the "Though perhaps not common" - it doesn't
add any information about the technical part of the patch.
(And that is, what is important)

And I personally use network-mounted repos every day ;-)

Sone users have large, network-mounted repos. Having the ability
to run fsmonitor against network paths would benefit those users.

>
> As a first step towards enabling fsmonitor to work against
> network-mounted repos, a configuration option, 'fsmonitor.allowRemote'
> was introduced for Windows. Setting this option to true will override
> the default behavior (erroring-out) when a network-mounted repo is
> detected by fsmonitor.
[]
The same option is now introduced for macOS.

>
> The the added wrinkle being that the Unix domain socket (UDS) file
  ^^^ ^^^

> used for IPC cannot be created in a network location; instead the
> temporary directory is used.

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

[Jeff Hostetler]

On 8/19/22 2:38 PM, Eric DeCosta wrote:
> 
> 
>> -----Original Message-----
>> From: Jeff Hostetler <git@jeffhostetler.com>
>> Sent: Friday, August 19, 2022 12:50 PM
>> To: Eric DeCosta via GitGitGadget <gitgitgadget@gmail.com>;
>> git@vger.kernel.org
>> Cc: Eric DeCosta <edecosta@mathworks.com>
>> Subject: Re: [PATCH] fsmonitor: option to allow fsmonitor to run against
>> network-mounted repos
>>
>>
>>
>> On 8/18/22 4:48 PM, Eric DeCosta via GitGitGadget wrote:
>>> From: Eric DeCosta <edecosta@mathworks.com>
>>>

[...]
> Yes, the point is to allow users to try it out.  Self-servingly, I have about
> 3K users who make heavy use of network-mounted sandboxes on the
> three major platforms; all connecting via NFS or SMB to Linux file
> servers.  Hardly exhaustive, but the file system change notification APIs
> (inotify, FSEvents, and ReadDirectoryCHangesW) all seem to work correctly.
> Thus my motivation to work on this aspect of git :-)

Perfect.  I have to be careful here, I don't want to discourage
experimentation and testing -- and if you have access to a pool of
users who can beat on it, then great let's try it.

At the time, I didn't have such a "captive audience"... :-)

And I just wanted to be sure that everyone understood that some of
these combinations have never been tried....

[...]
> As far as multiple client machines mounting the remote repo, I have
> doubts that FSMonitor would even see changes made from another
> machine. Worth trying out and documenting as needed - might even
> be better off being considered as unsupported.

Yeah, that's another dimension.  (1) is whether we miss events
that we generated locally.  (2) is whether events get relayed to
us from other clients.


>> There was a suggestion later in this thread about using a SHA-1
>> or SHA-256 hash of the pathname to avoid the tmp XXXXXX pattern
>> and just put the socket in $HOME (and omit the need for the new
>> fsmonitor-daemon.ipc FILE completely).  This might work, but we
>> need to be careful because a user might have hardlinks or symlinks
>> locally so there may be more than one unique path to the repo
>> on the local system.  (It is OK to have more than one daemon
>> listening to a repo, just less efficient.)
>>
> Ah, I see.
> 
>>
>> As an interim step, you might try using my original socket code
>> plus just the config.allowRemote=true change.  And test it on a
>> mounted repo where you've converted the .git directory to a .git
>> file and moved contents of the .git directory to somewhere local.
>> Then the UDS would be created in the local GITDIR instead of on
>> the remote system.  This won't help any of the sharing cases I
>> described above, but will let you experiment with getting remote
>> events.
>>
> Within the context of the environment that I have available to me
> (macOS over NFS to a Linux file server), FSEvents is working correctly.
> I can make changes at any arbitrary place inside of the repo and an
> event is generated.
> 
> It's looking like that the Unix domain socket (UDS) file should remain
> where it is unless fsmonitor.allowRemote is true.
> 
> If fsmonitor.allowRemote is true then the UDS file can be located
> in $HOME with the caveat that if there is more than one path to
> the repo (via hard or sym links) that things might not work as
> expected.  I think that's OK given the experimental nature of the
> feature.

Yeah, I'd experiment with moving the UDS to $HOME (assuming you don't
remote mount home directories too) and see how it works for you.
Later, after we have more experience with it, we can talk about just
having it always be in $HOME.

Jeff

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

[Junio C Hamano]

> As a first step towards enabling fsmonitor to work against
> network-mounted repos, a configuration option, 'fsmonitor.allowRemote'
> was introduced for Windows. Setting this option to true will override
> the default behavior (erroring-out) when a network-mounted repo is
> detected by fsmonitor. In order for macOS to have parity with Windows,
> the same option is now introduced for macOS.

With this merged in, recent CI runs for 'seen'

e.g. https://github.com/git/git/actions/runs/2892889122

seems to break macOS jobs, letting them hog CPU forever and exceed
6hr or whatever the limit is.

As an experiment I pushed out 'seen' but without this commit (not
the entire topic is excluded, the Windows one is still included).
As there is nothing specific to macOS between 'next' and 'seen',
macOS jobs seem to pass, which is not very surprising.

https://github.com/git/git/actions/runs/2896207171

As the patch collected some review comments, I've already marked it
in the "What's cooking" draft as expecting a reroll of that step;
until that happens, let's keep it out of 'seen'.

Thanks.

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

[Johannes Schindelin]

Hi Junio,

On Sat, 20 Aug 2022, Junio C Hamano wrote:

> > As a first step towards enabling fsmonitor to work against
> > network-mounted repos, a configuration option, 'fsmonitor.allowRemote'
> > was introduced for Windows. Setting this option to true will override
> > the default behavior (erroring-out) when a network-mounted repo is
> > detected by fsmonitor. In order for macOS to have parity with Windows,
> > the same option is now introduced for macOS.
>
> With this merged in, recent CI runs for 'seen'
>
> e.g. https://github.com/git/git/actions/runs/2892889122
>
> seems to break macOS jobs, letting them hog CPU forever and exceed
> 6hr or whatever the limit is.
>
> As an experiment I pushed out 'seen' but without this commit (not
> the entire topic is excluded, the Windows one is still included).
> As there is nothing specific to macOS between 'next' and 'seen',
> macOS jobs seem to pass, which is not very surprising.
>
> https://github.com/git/git/actions/runs/2896207171
>
> As the patch collected some review comments, I've already marked it
> in the "What's cooking" draft as expecting a reroll of that step;
> until that happens, let's keep it out of 'seen'.

It makes sense to keep it out of `seen`, and at the same time I would like
to encourage Eric to investigate what causes those time-outs.

When toggling timestamps (click on the wheel on the upper right) at
https://github.com/git/git/runs/7927812510?check_suite_focus=true#step:4:1774,
it can be seen that at close to 1am, t9903 finished, but then nothing
happens until twenty past 6am.

I've downloaded the raw logs (also available via the wheel on the upper
right) to find out which test timed out:

	$ diff -u \
	  <(sed -n 's/.*\] \(t[0-9][^ ]*\).*/\1/p' <~/Downloads/17 | sort) \
	  <(git ls-tree upstream/seen:t | cut -c 54- | grep '^t[0-9].*-.*sh$')

	--- /dev/fd/63  2022-08-22 14:56:05.510269527 +0200
	+++ /dev/fd/62  2022-08-22 14:56:05.510269527 +0200
	@@ -794,6 +794,7 @@
	 t7524-commit-summary.sh
	 t7525-status-rename.sh
	 t7526-commit-pathspec-file.sh
	+t7527-builtin-fsmonitor.sh
	 t7528-signed-commit-ssh.sh
	 t7600-merge.sh
	 t7601-merge-pull-config.sh
	@@ -945,6 +946,7 @@
	 t9812-git-p4-wildcards.sh
	 t9813-git-p4-preserve-users.sh
	 t9814-git-p4-rename.sh
	+t9815-git-p4-submit-fail.sh
	 t9816-git-p4-locked.sh
	 t9817-git-p4-exclude.sh
	 t9818-git-p4-block.sh
	@@ -964,5 +966,8 @@
	 t9832-unshelve.sh
	 t9833-errors.sh
	 t9834-git-p4-file-dir-bug.sh
	+t9835-git-p4-metadata-encoding-python2.sh
	+t9836-git-p4-metadata-encoding-python3.sh
	 t9901-git-web--browse.sh
	+t9902-completion.sh
	 t9903-bash-prompt.sh

I have no idea what's up with t98* and t9902, but I would bet that they
were somehow "swallowed" by `prove` being terminated, and that the
actual test script that times out is t7527.

Eric: To investigate, you will want to reproduce the problem on a macOS
machine. If you have none available, you could create a temporary branch,
heavily edit the CI definition, and push it to GitHub. And by heavy edits
I mean something like this:

- Remove all non-macOS jobs from `.github/workflows/main.yml` (that means
  removing all but the `regular` job, removing all but at least one
  `macos` matrix entry, and removing the the `needs: ci-config` and
  corresponding `if:` line.

- Edit `t/Makefile` to define `T = t7527-builtin-fsmonitor.sh` instead of
  running all the tests.

- Edit `.github/workflows/main.yml` so that the step that causes the
  time-out has a chance of timing out much sooner (and the subsequent
  steps then have a chance to upload the relevant logs):
  https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#jobsjob_idstepstimeout-minutes

If this does not shed any light into the issue, please let me know, I have
a couple more aces up my sleeve.

Ciao,
Dscho

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

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> Eric: To investigate, you will want to reproduce the problem on a macOS
> machine. If you have none available, you could create a temporary branch,
> heavily edit the CI definition, and push it to GitHub. And by heavy edits
> I mean something like this:
>
> - Remove all non-macOS jobs from `.github/workflows/main.yml` (that means
>   removing all but the `regular` job, removing all but at least one
>   `macos` matrix entry, and removing the the `needs: ci-config` and
>   corresponding `if:` line.
>
> - Edit `t/Makefile` to define `T = t7527-builtin-fsmonitor.sh` instead of
>   running all the tests.
>
> - Edit `.github/workflows/main.yml` so that the step that causes the
>   time-out has a chance of timing out much sooner (and the subsequent
>   steps then have a chance to upload the relevant logs):
>   https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#jobsjob_idstepstimeout-minutes
>
> If this does not shed any light into the issue, please let me know, I have
> a couple more aces up my sleeve.

Thanks, both.

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

[Jeff Hostetler]

On 8/22/22 9:22 AM, Johannes Schindelin wrote:
> Hi Junio,
> 
> On Sat, 20 Aug 2022, Junio C Hamano wrote:
> 
>>> As a first step towards enabling fsmonitor to work against
>>> network-mounted repos, a configuration option, 'fsmonitor.allowRemote'
>>> was introduced for Windows. Setting this option to true will override
>>> the default behavior (erroring-out) when a network-mounted repo is
>>> detected by fsmonitor. In order for macOS to have parity with Windows,
>>> the same option is now introduced for macOS.
>>
>> With this merged in, recent CI runs for 'seen'
>>
>> e.g. https://github.com/git/git/actions/runs/2892889122
>>
>> seems to break macOS jobs, letting them hog CPU forever and exceed
>> 6hr or whatever the limit is.
>>
>> As an experiment I pushed out 'seen' but without this commit (not
>> the entire topic is excluded, the Windows one is still included).
>> As there is nothing specific to macOS between 'next' and 'seen',
>> macOS jobs seem to pass, which is not very surprising.
>>
>> https://github.com/git/git/actions/runs/2896207171
>>
>> As the patch collected some review comments, I've already marked it
>> in the "What's cooking" draft as expecting a reroll of that step;
>> until that happens, let's keep it out of 'seen'.
> 
> It makes sense to keep it out of `seen`, and at the same time I would like
> to encourage Eric to investigate what causes those time-outs.
> 
> When toggling timestamps (click on the wheel on the upper right) at
> https://github.com/git/git/runs/7927812510?check_suite_focus=true#step:4:1774,
> it can be seen that at close to 1am, t9903 finished, but then nothing
> happens until twenty past 6am.
> 
> I've downloaded the raw logs (also available via the wheel on the upper
> right) to find out which test timed out:
> 
> 	$ diff -u \
> 	  <(sed -n 's/.*\] \(t[0-9][^ ]*\).*/\1/p' <~/Downloads/17 | sort) \
> 	  <(git ls-tree upstream/seen:t | cut -c 54- | grep '^t[0-9].*-.*sh$')
> 
> 	--- /dev/fd/63  2022-08-22 14:56:05.510269527 +0200
> 	+++ /dev/fd/62  2022-08-22 14:56:05.510269527 +0200
> 	@@ -794,6 +794,7 @@
> 	 t7524-commit-summary.sh
> 	 t7525-status-rename.sh
> 	 t7526-commit-pathspec-file.sh
> 	+t7527-builtin-fsmonitor.sh
> 	 t7528-signed-commit-ssh.sh
> 	 t7600-merge.sh
> 	 t7601-merge-pull-config.sh
> 	@@ -945,6 +946,7 @@
> 	 t9812-git-p4-wildcards.sh
> 	 t9813-git-p4-preserve-users.sh
> 	 t9814-git-p4-rename.sh
> 	+t9815-git-p4-submit-fail.sh
> 	 t9816-git-p4-locked.sh
> 	 t9817-git-p4-exclude.sh
> 	 t9818-git-p4-block.sh
> 	@@ -964,5 +966,8 @@
> 	 t9832-unshelve.sh
> 	 t9833-errors.sh
> 	 t9834-git-p4-file-dir-bug.sh
> 	+t9835-git-p4-metadata-encoding-python2.sh
> 	+t9836-git-p4-metadata-encoding-python3.sh
> 	 t9901-git-web--browse.sh
> 	+t9902-completion.sh
> 	 t9903-bash-prompt.sh
> 
> I have no idea what's up with t98* and t9902, but I would bet that they
> were somehow "swallowed" by `prove` being terminated, and that the
> actual test script that times out is t7527.
> 
> Eric: To investigate, you will want to reproduce the problem on a macOS
> machine. If you have none available, you could create a temporary branch,
> heavily edit the CI definition, and push it to GitHub. And by heavy edits
> I mean something like this:
> 
> - Remove all non-macOS jobs from `.github/workflows/main.yml` (that means
>    removing all but the `regular` job, removing all but at least one
>    `macos` matrix entry, and removing the the `needs: ci-config` and
>    corresponding `if:` line.
> 
> - Edit `t/Makefile` to define `T = t7527-builtin-fsmonitor.sh` instead of
>    running all the tests.
> 
> - Edit `.github/workflows/main.yml` so that the step that causes the
>    time-out has a chance of timing out much sooner (and the subsequent
>    steps then have a chance to upload the relevant logs):
>    https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#jobsjob_idstepstimeout-minutes

I would also set GIT_TRACE_FSMONITOR and GIT_TRACE2_PERF (on both daemon
and client sides of the tests) and capture the logs and try to figure
out what is happening.

I suspect that this testing should wait until you redo the patch to
remove the tmp file stuff and just move the socket into $HOME as we
talked about earlier.

Jeff

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

[Eric DeCosta]

> I would also set GIT_TRACE_FSMONITOR and GIT_TRACE2_PERF (on both daemon
> and client sides of the tests) and capture the logs and try to figure
> out what is happening.
>
> I suspect that this testing should wait until you redo the patch to
> remove the tmp file stuff and just move the socket into $HOME as we
> talked about earlier.
>
> Jeff

All tests are passing now with the new patch. By default the socket is written into
the original location (.git directory) unless 'fsmonitor.allowRemote' is true. Only
then is $HOME used and if $HOME proves unsuitable the user can override that
by setting 'fsmonitor.sockerDir' to some valid, local directory.

-Eric