| Message ID | edef029a298a44ba59d19db53c2f7ba07e93aec6.1663100859.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fsmonitor: option to allow fsmonitor to run against network-mounted repos | expand |
"Eric DeCosta via GitGitGadget" <gitgitgadget@gmail.com> writes: > +const char *fsmonitor_ipc__get_path(void) > +{ Looks like a bit klunky API. I would have expected it to take at least the path to the worktree or a pointer to struct repository. > + 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, the_repository->worktree, strlen(the_repository->worktree)); > + SHA1_Final(hash, &sha1ctx); I would not worry about SHA-1 hash collision for this use case, but would worry more about .worktree possible being unique. Can the .worktree string be aliased for the same directory in some way (e.g. depending on the initialization sequence, can it be a full pathname, a relative pathname, or can a pathname that looks like a full-pathname have symbolic link component in it?) that ends up creating two or more socket for the same worktree? > + repo_config_get_string(the_repository, "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; > +}
> -----Original Message----- > From: Junio C Hamano <gitster@pobox.com> > Sent: Tuesday, September 13, 2022 8:48 PM > To: Eric DeCosta via GitGitGadget <gitgitgadget@gmail.com> > Cc: git@vger.kernel.org; Jeff Hostetler <git@jeffhostetler.com>; Eric Sunshine > <sunshine@sunshineco.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 v6 3/6] fsmonitor: relocate socket file if .git directory is > remote > > "Eric DeCosta via GitGitGadget" <gitgitgadget@gmail.com> writes: > > > +const char *fsmonitor_ipc__get_path(void) { > > Looks like a bit klunky API. I would have expected it to take at least the path > to the worktree or a pointer to struct repository. > OK, I'll change it to take a pointer to struct repository. > > + 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, the_repository->worktree, > strlen(the_repository->worktree)); > > + SHA1_Final(hash, &sha1ctx); > > I would not worry about SHA-1 hash collision for this use case, but would > worry more about .worktree possible being unique. > > Can the .worktree string be aliased for the same directory in some way (e.g. > depending on the initialization sequence, can it be a full pathname, a relative > pathname, or can a pathname that looks like a full-pathname have symbolic > link component in it?) that ends up creating two or more socket for the same > worktree? > .worktree is set to the real path and since hardlinks are not allowed across file systems, I think we are OK. > > + repo_config_get_string(the_repository, "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/Makefile b/Makefile index b026f3e8cf0..5cd5ad818b8 100644 --- a/Makefile +++ b/Makefile @@ -2033,6 +2033,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/compat/fsmonitor/fsm-ipc-darwin.c b/compat/fsmonitor/fsm-ipc-darwin.c new file mode 100644 index 00000000000..afaca96dab9 --- /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(void) +{ + 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, the_repository->worktree, strlen(the_repository->worktree)); + SHA1_Final(hash, &sha1ctx); + + repo_config_get_string(the_repository, "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..769a88639f6 --- /dev/null +++ b/compat/fsmonitor/fsm-ipc-win32.c @@ -0,0 +1,4 @@ +#include "cache.h" +#include "fsmonitor-ipc.h" + +GIT_PATH_FUNC(fsmonitor_ipc__get_path, "fsmonitor--daemon.ipc") 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..caad2e246a0 100644 --- a/fsmonitor-ipc.c +++ b/fsmonitor-ipc.c @@ -47,8 +47,6 @@ 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());