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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Sep 17, 2022, 06:29 UTC
Message-ID: <CAPig+cTRNfewL01GOVTjFLaFq-vGgdeV2OX6Bn+StGrgb1JVMA@mail.gmail.com>
In-Reply-To: <b535649722826d7317061da7d8b0cf3d6e2a51ff.1663377141.git.gitgitgadget@gmail.com>

On Fri, Sep 16, 2022 at 9:12 PM Eric DeCosta via GitGitGadget <gitgitgadget@gmail.com> wrote:

Show 38 quoted lines

> 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.

Show 5 quoted lines

> +       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?

Show 11 quoted lines

> +       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.