RE: [PATCH v4 1/4] fsmonitor: add two new config options, allowRemote and socketDir
Show 65 quoted lines
> -----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.
Show 7 quoted lines
> 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"
Show 6 quoted lines
> 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?
Show 20 quoted lines
> 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
Show 146 quoted lines
>
> > ---
> > 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 <edecosta@mathworks.com>