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

Commit Message

Eric DeCosta Sept. 17, 2022, 1:12 a.m. UTC

From: Eric DeCosta <[e-mail]>

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 <[e-mail]>
---
 Makefile                               |  1 +
 builtin/fsmonitor--daemon.c            |  3 +-
 compat/fsmonitor/fsm-ipc-darwin.c      | 46 ++++++++++++++++++++++++++
 compat/fsmonitor/fsm-ipc-win32.c       |  9 +++++
 compat/fsmonitor/fsm-settings-darwin.c |  2 +-
 contrib/buildsystems/CMakeLists.txt    |  2 ++
 fsmonitor-ipc.c                        | 18 +++++-----
 fsmonitor-ipc.h                        |  4 ++-
 8 files changed, 72 insertions(+), 13 deletions(-)
 create mode 100644 compat/fsmonitor/fsm-ipc-darwin.c
 create mode 100644 compat/fsmonitor/fsm-ipc-win32.c

Comments

Eric Sunshine Sept. 17, 2022, 4:13 a.m. UTC | #1

On Fri, Sep 16, 2022 at 9:12 PM Eric DeCosta via GitGitGadget
<[e-mail]> 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 <[e-mail]>
> ---
> diff --git a/compat/fsmonitor/fsm-ipc-win32.c b/compat/fsmonitor/fsm-ipc-win32.c
> @@ -0,0 +1,9 @@
> +#include "config.h"
> +#include "fsmonitor-ipc.h"
> +
> +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

This file probably wants a final line terminator.

Eric Sunshine Sept. 17, 2022, 6:29 a.m. UTC | #2

On Fri, Sep 16, 2022 at 9:12 PM Eric DeCosta via GitGitGadget
<[e-mail]> 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 <[e-mail]>
> ---
> 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.

Eric DeCosta Sept. 17, 2022, 4:29 p.m. UTC | #3

> -----Original Message-----
> From: Eric Sunshine <[e-mail]>
> Sent: Saturday, September 17, 2022 2:30 AM
> To: Eric DeCosta via GitGitGadget <[e-mail]>
> Cc: Git List <[e-mail]>; Jeff Hostetler <[e-mail]>;
> Torsten Bögershausen <[e-mail]>; Ævar Arnfjörð Bjarmason
> <[e-mail]>; Ramsay Jones <[e-mail]>;
> Johannes Schindelin <[e-mail]>; Eric DeCosta
> <[e-mail]>
> 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
> <[e-mail]> 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 <[e-mail]>
> > ---
> > 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.
> 
Ultimately, the UDS file location is checked by check_uds_volume in fsm-settings-darwin as part of the overall settings checks; it will error-out there if the path is on the network.

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

Sure is, thanks.


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

Junio C Hamano Sept. 19, 2022, 4:50 p.m. UTC | #4

Eric Sunshine <[e-mail]> writes:

> On Fri, Sep 16, 2022 at 9:12 PM Eric DeCosta via GitGitGadget
> <[e-mail]> wrote:
>> +}
>> \ No newline at end of file
>
> This file probably wants a final line terminator.

definitely ;-)

Junio C Hamano Sept. 19, 2022, 4:58 p.m. UTC | #5

Eric Sunshine <[e-mail]> writes:

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

Yeah, dot-prefixed files are appropriate if they are to be placed at
the top of some tree without the user having any say in how that
tree is chosen (e.g. the working tree or $HOME).  If the user has
the power to specify the location, the equation changes.

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

That's a good point.  If one default position (e.g. repository) is
checked if it is usable and can be rejected if it isn't, the
fallback position should at least satisfy the same "is it usable?"
criteria.

Thanks.

Patch

diff --git a/Makefile b/Makefile
index 6e492a67547..58bb9248471 100644
--- a/Makefile
+++ b/Makefile
@@ -2034,6 +2034,7 @@  ifdef FSMONITOR_DAEMON_BACKEND
 	COMPAT_CFLAGS += -DHAVE_FSMONITOR_DAEMON_BACKEND
 	COMPAT_OBJS += compat/fsmonitor/fsm-listen-$(FSMONITOR_DAEMON_BACKEND).o
 	COMPAT_OBJS += compat/fsmonitor/fsm-health-$(FSMONITOR_DAEMON_BACKEND).o
+	COMPAT_OBJS += compat/fsmonitor/fsm-ipc-$(FSMONITOR_DAEMON_BACKEND).o
 endif
 
 ifdef FSMONITOR_OS_SETTINGS
diff --git a/builtin/fsmonitor--daemon.c b/builtin/fsmonitor--daemon.c
index 2c109cf8b37..0123fc33ed2 100644
--- a/builtin/fsmonitor--daemon.c
+++ b/builtin/fsmonitor--daemon.c
@@ -1343,7 +1343,8 @@  static int fsmonitor_run_daemon(void)
 	 * directory.)
 	 */
 	strbuf_init(&state.path_ipc, 0);
-	strbuf_addstr(&state.path_ipc, absolute_path(fsmonitor_ipc__get_path()));
+	strbuf_addstr(&state.path_ipc,
+		absolute_path(fsmonitor_ipc__get_path(the_repository)));
 
 	/*
 	 * Confirm that we can create platform-specific resources for the
diff --git a/compat/fsmonitor/fsm-ipc-darwin.c b/compat/fsmonitor/fsm-ipc-darwin.c
new file mode 100644
index 00000000000..d6628185000
--- /dev/null
+++ b/compat/fsmonitor/fsm-ipc-darwin.c
@@ -0,0 +1,46 @@ 
+#include "cache.h"
+#include "config.h"
+#include "strbuf.h"
+#include "fsmonitor.h"
+#include "fsmonitor-ipc.h"
+#include "fsmonitor-path-utils.h"
+
+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));
+
+	ipc_path = interpolate_path(ipc_file.buf, 1);
+	if (!ipc_path)
+		die(_("Invalid path: %s"), ipc_file.buf);
+
+	strbuf_release(&ipc_file);
+	return ipc_path;
+}
diff --git a/compat/fsmonitor/fsm-ipc-win32.c b/compat/fsmonitor/fsm-ipc-win32.c
new file mode 100644
index 00000000000..3a3a46db209
--- /dev/null
+++ b/compat/fsmonitor/fsm-ipc-win32.c
@@ -0,0 +1,9 @@ 
+#include "config.h"
+#include "fsmonitor-ipc.h"
+
+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
diff --git a/compat/fsmonitor/fsm-settings-darwin.c b/compat/fsmonitor/fsm-settings-darwin.c
index dba3ced6bb7..681d8bf963e 100644
--- a/compat/fsmonitor/fsm-settings-darwin.c
+++ b/compat/fsmonitor/fsm-settings-darwin.c
@@ -26,7 +26,7 @@ 
 static enum fsmonitor_reason check_uds_volume(struct repository *r)
 {
 	struct fs_info fs;
-	const char *ipc_path = fsmonitor_ipc__get_path();
+	const char *ipc_path = fsmonitor_ipc__get_path(r);
 	struct strbuf path = STRBUF_INIT;
 	strbuf_add(&path, ipc_path, strlen(ipc_path));
 
diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
index b88494bf59b..7e7b6b9a362 100644
--- a/contrib/buildsystems/CMakeLists.txt
+++ b/contrib/buildsystems/CMakeLists.txt
@@ -308,6 +308,7 @@  if(SUPPORTS_SIMPLE_IPC)
 		add_compile_definitions(HAVE_FSMONITOR_DAEMON_BACKEND)
 		list(APPEND compat_SOURCES compat/fsmonitor/fsm-listen-win32.c)
 		list(APPEND compat_SOURCES compat/fsmonitor/fsm-health-win32.c)
+		list(APPEND compat_SOURCES compat/fsmonitor/fsm-ipc-win32.c)
 		list(APPEND compat_SOURCES compat/fsmonitor/fsm-path-utils-win32.c)
 
 		add_compile_definitions(HAVE_FSMONITOR_OS_SETTINGS)
@@ -316,6 +317,7 @@  if(SUPPORTS_SIMPLE_IPC)
 		add_compile_definitions(HAVE_FSMONITOR_DAEMON_BACKEND)
 		list(APPEND compat_SOURCES compat/fsmonitor/fsm-listen-darwin.c)
 		list(APPEND compat_SOURCES compat/fsmonitor/fsm-health-darwin.c)
+		list(APPEND compat_SOURCES compat/fsmonitor/fsm-ipc-darwin.c)
 		list(APPEND compat_SOURCES compat/fsmonitor/fsm-path-utils-darwin.c)
 
 		add_compile_definitions(HAVE_FSMONITOR_OS_SETTINGS)
diff --git a/fsmonitor-ipc.c b/fsmonitor-ipc.c
index 789e7397baa..c0f42301c84 100644
--- a/fsmonitor-ipc.c
+++ b/fsmonitor-ipc.c
@@ -18,7 +18,7 @@  int fsmonitor_ipc__is_supported(void)
 	return 0;
 }
 
-const char *fsmonitor_ipc__get_path(void)
+const char *fsmonitor_ipc__get_path(struct repository *r)
 {
 	return NULL;
 }
@@ -47,11 +47,9 @@  int fsmonitor_ipc__is_supported(void)
 	return 1;
 }
 
-GIT_PATH_FUNC(fsmonitor_ipc__get_path, "fsmonitor--daemon.ipc")
-
 enum ipc_active_state fsmonitor_ipc__get_state(void)
 {
-	return ipc_get_active_state(fsmonitor_ipc__get_path());
+	return ipc_get_active_state(fsmonitor_ipc__get_path(the_repository));
 }
 
 static int spawn_daemon(void)
@@ -81,8 +79,8 @@  int fsmonitor_ipc__send_query(const char *since_token,
 	trace2_data_string("fsm_client", NULL, "query/command", tok);
 
 try_again:
-	state = ipc_client_try_connect(fsmonitor_ipc__get_path(), &options,
-				       &connection);
+	state = ipc_client_try_connect(fsmonitor_ipc__get_path(the_repository),
+						&options, &connection);
 
 	switch (state) {
 	case IPC_STATE__LISTENING:
@@ -117,13 +115,13 @@  try_again:
 
 	case IPC_STATE__INVALID_PATH:
 		ret = error(_("fsmonitor_ipc__send_query: invalid path '%s'"),
-			    fsmonitor_ipc__get_path());
+			    fsmonitor_ipc__get_path(the_repository));
 		goto done;
 
 	case IPC_STATE__OTHER_ERROR:
 	default:
 		ret = error(_("fsmonitor_ipc__send_query: unspecified error on '%s'"),
-			    fsmonitor_ipc__get_path());
+			    fsmonitor_ipc__get_path(the_repository));
 		goto done;
 	}
 
@@ -149,8 +147,8 @@  int fsmonitor_ipc__send_command(const char *command,
 	options.wait_if_busy = 1;
 	options.wait_if_not_found = 0;
 
-	state = ipc_client_try_connect(fsmonitor_ipc__get_path(), &options,
-				       &connection);
+	state = ipc_client_try_connect(fsmonitor_ipc__get_path(the_repository),
+						&options, &connection);
 	if (state != IPC_STATE__LISTENING) {
 		die(_("fsmonitor--daemon is not running"));
 		return -1;
diff --git a/fsmonitor-ipc.h b/fsmonitor-ipc.h
index b6a7067c3af..8b489da762b 100644
--- a/fsmonitor-ipc.h
+++ b/fsmonitor-ipc.h
@@ -3,6 +3,8 @@ 
 
 #include "simple-ipc.h"
 
+struct repository;
+
 /*
  * Returns true if built-in file system monitor daemon is defined
  * for this platform.
@@ -16,7 +18,7 @@  int fsmonitor_ipc__is_supported(void);
  *
  * Returns NULL if the daemon is not supported on this platform.
  */
-const char *fsmonitor_ipc__get_path(void);
+const char *fsmonitor_ipc__get_path(struct repository *r);
 
 /*
  * Try to determine whether there is a `git-fsmonitor--daemon` process