On Fri, Sep 16, 2022 at 9:12 PM Eric DeCosta via GitGitGadget
<gitgitgadget@xxxxxxxxx> 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@xxxxxxxxxxxxx>
> ---
> 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.
