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

Commit Message

Eric DeCosta Aug. 31, 2022, 4:09 p.m. UTC

From: Eric DeCosta <[e-mail]>

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 <[e-mail]>
---
 fsmonitor-settings.c | 67 ++++++++++++++++++++++++++++++++++++++++++--
 fsmonitor-settings.h |  4 +++
 2 files changed, 69 insertions(+), 2 deletions(-)

Comments

Ævar Arnfjörð Bjarmason Aug. 31, 2022, 7:41 p.m. UTC | #1

On Wed, Aug 31 2022, Eric DeCosta via GitGitGadget wrote:

> From: Eric DeCosta <[e-mail]>
> [...]
>  	enum fsmonitor_reason reason;
> +	int allow_remote;
>  	char *hook_path;
> +	char *sock_dir;
>  };

Any reason we couldn't just add this to "struct repo_settings" and ...

> +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;
> +}
> +

...instead of this whole ceremony...

> +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;
> +}

Just have a single repo_cfg_bool() line in prepare_repo_settings()
instead?

(There are some reasons for the "lazy" behavior of fsmonitor-settings.c,
but surely a simple boolean variable we can read on startup isn't it,
and we already paid the cost to do so with the configset...)


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

...maybe this socket dir stuff is the exception though.

> +		r->settings.fsmonitor->sock_dir = strdup(path);

Aren't you strdup()-ing an already strdup()'d string, and therefore
leaking memory? Also this should be xstrdup(), surely?

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

This seems rather backwards, as odd as this API itself is already, isn't
the whole idea that after we call lookup_fsmonitor_settings() it will
have set() anything that's mandatory?

But maybe I haven't grokked it ... :)

Junio C Hamano Aug. 31, 2022, 8:04 p.m. UTC | #2

"Eric DeCosta via GitGitGadget" <[e-mail]> writes:

> From: Eric DeCosta <[e-mail]>
>
> 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.

Before describing "what they do", please justify why we need to add
them.  The usual way to do so is to start with some observation of
the status quo, and describe the "gap" between what can be done with
the current system and what the users would want to do.  It might
further be necessary to justify why "would want to do" is worthwhile
to support.  I suspect that you can do all of the above in a couple
of paragraphs, and you'd succeed if the solution you chose would
fall out as a natural consequence in the mind of readers who follow
your line of thought by reading these introductory paragraphs.  And
then after the stage is set like so, the above description of what
you chose to implement as a solution should come.

>  struct fsmonitor_settings {
>  	enum fsmonitor_mode mode;
>  	enum fsmonitor_reason reason;
> +	int allow_remote;

I am debating myuself if a comment like

	int allow_remote; /* -1: undecided, 0: unallowed, 1: allowed */

is necessary (I know it would help if we had one; I am just
wondering if it is too obvious).

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

OK.  socket_dir is naturally NULL at the start.

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

A noise hunk?

> @@ -110,9 +134,44 @@ const char *fsm_settings__get_hook_path(struct repository *r)
> ...
> +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);

As we are overwriting it immediately, just calling free(), not
FREE_AND_NULL(), is more appropriate, isn't it?

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

Do these two need to be extern?

I would imagine that implementations in compat/fsmonitor/* would
just call fsm_settings__set_hook() or __set_ipc() and that causes
these two to be called as part of the set-up sequence.  Do they need
to call these two directly?

If not, we probably should make them static.  I suspect that some
existing declarations in this header file fall into the same
category and may need to become static for the same reason, which
you can do as a preliminary clean-up patch, or post-clean-up after
the dust settles.  Regardless of which approach to take for existing
ones, we do not want to make it worse by adding new externally
visible names that do not have to be visible.

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

On the other hand, these may need to be query-able by the
implementations.

>  enum fsmonitor_mode fsm_settings__get_mode(struct repository *r);
>  const char *fsm_settings__get_hook_path(struct repository *r);

Ramsay Jones Sept. 1, 2022, 2:25 a.m. UTC | #3

On 31/08/2022 21:04, Junio C Hamano wrote:
> "Eric DeCosta via GitGitGadget" <[e-mail]> writes:
> 
[snip]
>> 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);
> 
> Do these two need to be extern?
> 

On 'seen' after building:
  ...
  $ ./static-check.pl >ssc
  $ diff nsc ssc
  3a4
  > bundle-uri.o	- for_all_bundles_in_list
  15d15
  < config.o	- git_config_from_file_with_options
  40a41,43
  > fsmonitor-settings.o	- fsm_settings__get_allow_remote
  > fsmonitor-settings.o	- fsm_settings__get_socket_dir
  > fsmonitor-settings.o	- fsm_settings__set_allow_remote
  44a48
  > fsmonitor-settings.o	- fsm_settings__set_socket_dir
  $ 

.. so probably not. ;-)

ATB,
Ramsay Jones

Jeff Hostetler Sept. 1, 2022, 5:53 p.m. UTC | #4

On 8/31/22 12:09 PM, Eric DeCosta via GitGitGadget wrote:
> From: Eric DeCosta <[e-mail]>
> 
> 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 <[e-mail]>
> ---
>   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);

When we use the hook interface (to talk to a third-party
app like Watchman) we do not use the unix domain socket.
(We talk to the hook child using a normal pipe.)

FWIW, I added code in my series to limit the use of hook-based
fsmonitors to local filesystems for the paranoia-based
reasons I've described before.  It wasn't because of the UDS.

So we don't need to set the socket directory in this case.

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

Jeff Hostetler Sept. 1, 2022, 6:04 p.m. UTC | #5

On 8/31/22 12:09 PM, Eric DeCosta via GitGitGadget wrote:
> From: Eric DeCosta <[e-mail]>
> 
> 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.

If we add these config settings, we should update the docs
to describe them.

Jeff

Jeff Hostetler Sept. 1, 2022, 9:21 p.m. UTC | #6

On 8/31/22 12:09 PM, Eric DeCosta via GitGitGadget wrote:
> From: Eric DeCosta <[e-mail]>
> 
> 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 <[e-mail]>

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

Eric DeCosta Sept. 2, 2022, 4:54 p.m. UTC | #7

> -----Original Message-----
> From: Jeff Hostetler <[e-mail]>
> Sent: Thursday, September 1, 2022 5:22 PM
> To: Eric DeCosta via GitGitGadget <[e-mail]>;
> [e-mail]
> Cc: Eric DeCosta <[e-mail]>
> 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 <[e-mail]>
> >
> > 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 <[e-mail]>
> 
> 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);
> >
> >

Jeff Hostetler Sept. 6, 2022, 2:27 p.m. UTC | #8

On 9/2/22 12:54 PM, Eric DeCosta wrote:
> 
> 
>> -----Original Message-----
>> From: Jeff Hostetler <[e-mail]>
>> Sent: Thursday, September 1, 2022 5:22 PM
>> To: Eric DeCosta via GitGitGadget <[e-mail]>;
>> [e-mail]
>> Cc: Eric DeCosta <[e-mail]>
>> 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 <[e-mail]>
>>>
>>> Introduce two new configuration options
[...]
>> 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?

Yeah, I was probably juggling too many things in my
head when I was making that list.  And I'm sure it'll
fall our when you see what the code looks like.  The
_NOSOCKETS case should basically be just the places where
you test the path of the socket-dir (if it is always outside
of the .git dir).  None of the tests on the actual workdir
inspection should hit it anymore, right?

And we can save for a later day whether or not to test
for NTFS and FAT32 (local or remote) in the Darwin code
based on whether or not the kernel sends FSEvents for
them.

Jeff

Patch

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