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

[Jeff Hostetler <git@jeffhostetler.com>]

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.

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.

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.

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


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