RE: [PATCH v8 2/5] fsmonitor: relocate socket file if .git directory is remote

> -----Original Message-----
> From: Eric Sunshine <sunshine@sunshineco.com>
> Sent: Saturday, September 17, 2022 2:30 AM
> To: Eric DeCosta via GitGitGadget <gitgitgadget@gmail.com>
> Cc: Git List <git@vger.kernel.org>; Jeff Hostetler <git@jeffhostetler.com>;
> Torsten Bögershausen <tboegi@web.de>; Ævar Arnfjörð Bjarmason
> <avarab@gmail.com>; Ramsay Jones <ramsay@ramsayjones.plus.com>;
> Johannes Schindelin <Johannes.Schindelin@gmx.de>; Eric DeCosta
> <edecosta@mathworks.com>
> Subject: Re: [PATCH v8 2/5] fsmonitor: relocate socket file if .git directory is
> remote
> 
> On Fri, Sep 16, 2022 at 9:12 PM Eric DeCosta via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
> > If the .git directory is on a remote file system, create the socket
> > file in 'fsmonitor.socketDir' if it is defined, else create it in $HOME.
> >
> > Signed-off-by: Eric DeCosta <edecosta@mathworks.com>
> > ---
> > diff --git a/compat/fsmonitor/fsm-ipc-darwin.c
> > b/compat/fsmonitor/fsm-ipc-darwin.c
> > @@ -0,0 +1,46 @@
> > +static GIT_PATH_FUNC(fsmonitor_ipc__get_default_path,
> > +"fsmonitor--daemon.ipc")
> > +
> > +const char *fsmonitor_ipc__get_path(struct repository *r) {
> > +       static const char *ipc_path;
> > +       SHA_CTX sha1ctx;
> > +       char *sock_dir;
> > +       struct strbuf ipc_file = STRBUF_INIT;
> > +       unsigned char hash[SHA_DIGEST_LENGTH];
> > +
> > +       if (ipc_path)
> > +               return ipc_path;
> > +
> > +       ipc_path = fsmonitor_ipc__get_default_path();
> > +
> > +       /* By default the socket file is created in the .git directory */
> > +       if (fsmonitor__is_fs_remote(ipc_path) < 1)
> > +               return ipc_path;
> > +
> > +       SHA1_Init(&sha1ctx);
> > +       SHA1_Update(&sha1ctx, r->worktree, strlen(r->worktree));
> > +       SHA1_Final(hash, &sha1ctx);
> > +
> > +       repo_config_get_string(r, "fsmonitor.socketdir", &sock_dir);
> > +
> > +       /* Create the socket file in either socketDir or $HOME */
> > +       if (sock_dir && *sock_dir)
> > +               strbuf_addf(&ipc_file, "%s/.git-fsmonitor-%s",
> > +                                       sock_dir, hash_to_hex(hash));
> > +       else
> > +               strbuf_addf(&ipc_file, "~/.git-fsmonitor-%s",
> > + hash_to_hex(hash));
> 
> A couple comments...
> 
> In my mind, the directory specified by `fsmonitor.socketdir` is likely to be
> dedicated to this purpose (i.e. housing Git administrative junk). As such, it
> feels somewhat odd for the socket file to be hidden; I would instead expect
> the socket name to be non-hidden (say,
> "git-fsmonitor-daemon-{hash}.ipc") rather than hidden (".git-fsmonitor-*").
> The directory specified by `fsmonitor.socketdir` may or may not be hidden
> (i.e. start with a dot), but that's the user's decision. For the $HOME case, it
> almost feels cleaner to create a hidden directory (say, "$HOME/.git-
> fsmonitor") in which to house the socket files ("git-fsmonitor-daemon-
> {hash}.ipc"). Anyhow, this comment is quite subjective; perhaps not
> actionable.

> What happens if either $HOME or `fsmonitor.socketdir` are network-
> mounted? Should this code be checking for that case? If they are network-
> mounted, should it error out? At minimum, I would think a warning is
> warranted in order to save users the headache of wondering why fsmonitor
> isn't working correctly.
> 

> > +       ipc_path = interpolate_path(ipc_file.buf, 1);
> > +       if (!ipc_path)
> > +               die(_("Invalid path: %s"), ipc_file.buf);
> > +
> > +       strbuf_release(&ipc_file);
> 
> `sock_dir` is being leaked, isn't it?
>

> > +       return ipc_path;
> > +}
> > diff --git a/compat/fsmonitor/fsm-ipc-win32.c
> > b/compat/fsmonitor/fsm-ipc-win32.c
> > @@ -0,0 +1,9 @@
> > +const char *fsmonitor_ipc__get_path(struct repository *r) {
> > +       static char *ret;
> > +       if (!ret)
> > +               ret = git_pathdup("fsmonitor--daemon.ipc");
> > +       return ret;
> > +}
> > \ No newline at end of file
> 
> Mentioned already.