RE: [PATCH v4 1/4] fsmonitor: add two new config options, allowRemote and socketDir

[Eric DeCosta <edecosta@mathworks.com>]

> -----Original Message-----
> From: Jeff Hostetler <git@jeffhostetler.com>
> Sent: Thursday, September 1, 2022 5:22 PM
> To: Eric DeCosta via GitGitGadget <gitgitgadget@gmail.com>;
> git@vger.kernel.org
> Cc: Eric DeCosta <edecosta@mathworks.com>
> Subject: Re: [PATCH v4 1/4] fsmonitor: add two new config options,
> allowRemote and socketDir
> 
> 
> 
> On 8/31/22 12:09 PM, Eric DeCosta via GitGitGadget wrote:
> > From: Eric DeCosta <edecosta@mathworks.com>
> >
> > Introduce two new configuration options
> >
> >     fsmonitor.allowRemote - setting this to true overrides fsmonitor's
> >     default behavior of erroring out when enountering network file
> >     systems. Additionly, when true, the Unix domain socket (UDS) file
> >     used for IPC is located in $HOME rather than in the .git directory.
> >
> >     fsmonitor.socketDir - allows for the UDS file to be located
> >     anywhere the user chooses rather $HOME.
> >
> > Signed-off-by: Eric DeCosta <edecosta@mathworks.com>
> 
> I hate to say it, but I think I'd like to start over on this patch.  I think it adds
> too much to the upper layers.
> 
> Could we:
> 
> 1. Move the GIT_PATH_FUNC() down into compat/fsmonitor/fsm-settings-*.c
>     so that we don't need the ifdef-ing because we only really need to
>     change the path on MacOS.
> 
>     Change it from this macro trick to be an actual function
>     that computes the path and sticks it in a static buffer
>     and returns it on future calls.  (We call it from several
>     different places.)
> 
>     On MacOS have it always compute a $HOME or fsmonitor.socketDir
>     based path.
> 
>     (This is called early, so we don't know anything about the
>      r->settings.fsmonitor data yet (like whether we'll have an
>      ipc or hook), so we don't know whether to worry about whether
>      the socket-directory is local/remote yet.)
> 
> 2. Get rid of the fsm_settings__get_allow_remote() and
>     __get_socket_dir() functions in fsmonitor-settings.c.
> 
>     Besides, having it at this layer says that "allow-remote"
>     is an all-or-nothing flag (that the platform-independent
>     layer doesn't know anything about).  We may eventually find
>     that we want the platform code to be smarter about that,
>     such as allow remote from NFSv4 but not NFSv3.  We don't
>     want to litter the platform-independent code with that.
> 
>     And it says that we always use a UDS.  We might change
>     that later on MacOS.  And we don't need them at all on Windows.
> 
> 3. Get rid of the fsm_settings__set_allow_remote() and
>     __set_socket_dir().  The FSMonitor code only needs to access
>     them once down in the fsm_os__incompatible() code, so we
>     could just inline the relevant parts down there.
 
Got it.

> 4. Leave the 'reason = check_for_incompatible()' as it was
>     in fsm_setttings__set_ipc() and __set_hook().
> 
>     But you might pass a boolean "is-ipc" or "is-hook" to
>     check_for_incompatible() that it could pass down to
>     fsm_os__incompatible().  That might save the need to the
>     fsmonitor.socketDir stuff when they want to use Watchman.

I see, avoid the UDS check altogether if "is-hook"

> 5. In fsm-settings-darwin.c:fsm_os__incompatible()
>     complain if the socket path is remote (when is-ipc is set).
> 
>     This probably ought to be a new _REASON_ type for non-local
>     UDS.  And this is independent of whether the actual worktree
>     is remote or whether remote worktrees are allowed.

I thought that was what FSMONITOR_REASON_NOSOCKETS was for?

> 6. In fsm-settings-darwin.c:check_volume()
> 
> All that should be necessary is to:
> 
>      if (!(fs.f_flags & MNT_LOCAL))
> +   {
> +       // ... repo_config_get_bool(... "fsmonitor.allowremote" ...)
>          return FSMONITOR_REASON_REMOTE;
> +   }
> 
> 
> Alternatively, for 5 & 6, we could pass a pathname to check_volume() so that
> fsm_os__incompatible() calls it twice:  once for the worktree and once for
> the socketdir.
> 
> The ntfs and msdos restrictions were for UDS reasons, so you might want
> pass some flags to check_volume() too.
> 
> Sorry for the redesign and I hope this all makes sense, Jeff
> 

I've taken a first pass at all of this, will work on it some more.

-Eric

> 
> > ---
> >   fsmonitor-settings.c | 67
> ++++++++++++++++++++++++++++++++++++++++++--
> >   fsmonitor-settings.h |  4 +++
> >   2 files changed, 69 insertions(+), 2 deletions(-)
> >
> > diff --git a/fsmonitor-settings.c b/fsmonitor-settings.c index
> > 464424a1e92..a15eeecebf4 100644
> > --- a/fsmonitor-settings.c
> > +++ b/fsmonitor-settings.c
> > @@ -10,7 +10,9 @@
> >   struct fsmonitor_settings {
> >   	enum fsmonitor_mode mode;
> >   	enum fsmonitor_reason reason;
> > +	int allow_remote;
> >   	char *hook_path;
> > +	char *sock_dir;
> >   };
> >
> >   static enum fsmonitor_reason check_for_incompatible(struct
> > repository *r) @@ -43,6 +45,7 @@ static struct fsmonitor_settings
> *alloc_settings(void)
> >   	CALLOC_ARRAY(s, 1);
> >   	s->mode = FSMONITOR_MODE_DISABLED;
> >   	s->reason = FSMONITOR_REASON_UNTESTED;
> > +	s->allow_remote = -1;
> >
> >   	return s;
> >   }
> > @@ -90,6 +93,26 @@ static void lookup_fsmonitor_settings(struct
> repository *r)
> >   		fsm_settings__set_disabled(r);
> >   }
> >
> > +int fsm_settings__get_allow_remote(struct repository *r) {
> > +	if (!r)
> > +		r = the_repository;
> > +	if (!r->settings.fsmonitor)
> > +		lookup_fsmonitor_settings(r);
> > +
> > +	return r->settings.fsmonitor->allow_remote;
> > +}
> > +
> > +const char *fsm_settings__get_socket_dir(struct repository *r) {
> > +	if (!r)
> > +		r = the_repository;
> > +	if (!r->settings.fsmonitor)
> > +		lookup_fsmonitor_settings(r);
> > +
> > +	return r->settings.fsmonitor->sock_dir; }
> > +
> >   enum fsmonitor_mode fsm_settings__get_mode(struct repository *r)
> >   {
> >   	if (!r)
> > @@ -100,6 +123,7 @@ enum fsmonitor_mode
> fsm_settings__get_mode(struct repository *r)
> >   	return r->settings.fsmonitor->mode;
> >   }
> >
> > +
> >   const char *fsm_settings__get_hook_path(struct repository *r)
> >   {
> >   	if (!r)
> > @@ -110,9 +134,44 @@ const char *fsm_settings__get_hook_path(struct
> repository *r)
> >   	return r->settings.fsmonitor->hook_path;
> >   }
> >
> > +void fsm_settings__set_allow_remote(struct repository *r) {
> > +	int allow;
> > +
> > +	if (!r)
> > +		r = the_repository;
> > +	if (!r->settings.fsmonitor)
> > +		r->settings.fsmonitor = alloc_settings();
> > +	if (!repo_config_get_bool(r, "fsmonitor.allowremote", &allow))
> > +		r->settings.fsmonitor->allow_remote = allow;
> > +
> > +	return;
> > +}
> > +
> > +void fsm_settings__set_socket_dir(struct repository *r) {
> > +	const char *path;
> > +
> > +	if (!r)
> > +		r = the_repository;
> > +	if (!r->settings.fsmonitor)
> > +		r->settings.fsmonitor = alloc_settings();
> > +
> > +	if (!repo_config_get_pathname(r, "fsmonitor.socketdir", &path)) {
> > +		FREE_AND_NULL(r->settings.fsmonitor->sock_dir);
> > +		r->settings.fsmonitor->sock_dir = strdup(path);
> > +	}
> > +
> > +	return;
> > +}
> > +
> >   void fsm_settings__set_ipc(struct repository *r)
> >   {
> > -	enum fsmonitor_reason reason = check_for_incompatible(r);
> > +	enum fsmonitor_reason reason;
> > +
> > +	fsm_settings__set_allow_remote(r);
> > +	fsm_settings__set_socket_dir(r);
> > +	reason = check_for_incompatible(r);
> >
> >   	if (reason != FSMONITOR_REASON_OK) {
> >   		fsm_settings__set_incompatible(r, reason); @@ -135,7
> +194,11 @@
> > void fsm_settings__set_ipc(struct repository *r)
> >
> >   void fsm_settings__set_hook(struct repository *r, const char *path)
> >   {
> > -	enum fsmonitor_reason reason = check_for_incompatible(r);
> > +	enum fsmonitor_reason reason;
> > +
> > +	fsm_settings__set_allow_remote(r);
> > +	fsm_settings__set_socket_dir(r);
> > +	reason = check_for_incompatible(r);
> >
> >   	if (reason != FSMONITOR_REASON_OK) {
> >   		fsm_settings__set_incompatible(r, reason); diff --git
> > a/fsmonitor-settings.h b/fsmonitor-settings.h index
> > d9c2605197f..2de54c85e94 100644
> > --- a/fsmonitor-settings.h
> > +++ b/fsmonitor-settings.h
> > @@ -23,12 +23,16 @@ enum fsmonitor_reason {
> >   	FSMONITOR_REASON_NOSOCKETS, /* NTFS,FAT32 do not support
> Unix sockets */
> >   };
> >
> > +void fsm_settings__set_allow_remote(struct repository *r); void
> > +fsm_settings__set_socket_dir(struct repository *r);
> >   void fsm_settings__set_ipc(struct repository *r);
> >   void fsm_settings__set_hook(struct repository *r, const char *path);
> >   void fsm_settings__set_disabled(struct repository *r);
> >   void fsm_settings__set_incompatible(struct repository *r,
> >   				    enum fsmonitor_reason reason);
> >
> > +int fsm_settings__get_allow_remote(struct repository *r); const char
> > +*fsm_settings__get_socket_dir(struct repository *r);
> >   enum fsmonitor_mode fsm_settings__get_mode(struct repository *r);
> >   const char *fsm_settings__get_hook_path(struct repository *r);
> >
> >