Re: [PATCH v15 2/6] fsmonitor: relocate socket file if .git directory is remote
- From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
- Date
- Jan 30, 2023, 09:58 UTC
- Message-ID
- <230130.86o7qgs5j6.gmgdl@evledraar.gmail.com>
- In-Reply-To
- <7bf1cdfe3b259f7135f3a50dcdd653563d5e19f6.1664904751.git.gitgitgadget@gmail.com>
On Tue, Oct 04 2022, Eric DeCosta via GitGitGadget wrote:
Show 65 quoted lines
> From: Eric DeCosta <edecosta@mathworks.com>
>
> If the .git directory is on a remote filesystem, create the socket
> file in 'fsmonitor.socketDir' if it is defined, else create it in $HOME.
>
> Signed-off-by: Eric DeCosta <edecosta@mathworks.com>
> ---
> Makefile | 1 +
> builtin/fsmonitor--daemon.c | 3 +-
> compat/fsmonitor/fsm-ipc-darwin.c | 52 ++++++++++++++++++++++++++
> 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, 78 insertions(+), 13 deletions(-)
> create mode 100644 compat/fsmonitor/fsm-ipc-darwin.c
> create mode 100644 compat/fsmonitor/fsm-ipc-win32.c
>
> diff --git a/Makefile b/Makefile
> index ffab427ea5b..feb675a6959 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -2039,6 +2039,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..ce843d63348
> --- /dev/null
> +++ b/compat/fsmonitor/fsm-ipc-darwin.c
> @@ -0,0 +1,52 @@
> +#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 = NULL;
> + SHA_CTX sha1ctx;
> + char *sock_dir = NULL;...don't init this to NULL...
Show 21 quoted lines
> + struct strbuf ipc_file = STRBUF_INIT;
> + unsigned char hash[SHA_DIGEST_LENGTH];
> +
> + if (!r)
> + BUG("No repository passed into fsmonitor_ipc__get_path");
> +
> + if (ipc_path)
> + return ipc_path;
> +
> +
> + /* By default the socket file is created in the .git directory */
> + if (fsmonitor__is_fs_remote(r->gitdir) < 1) {
> + ipc_path = fsmonitor_ipc__get_default_path();
> + 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);...instead check the return value here. So:
if (!repo_config_get_string(..., &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));^ Add the body of this branch to the "..." above.
> + } else {
> + strbuf_addf(&ipc_file, "~/.git-fsmonitor-%s", hash_to_hex(hash));...and keep this in the "else".
> + } > + free(sock_dir);
You'd then add this free() to the first branch, but better yet in this case avoid this allocation, use "const char *" and use repo_config_get_string_tmp(). It's made for exactly this sort of use-case.
Show 19 quoted lines
> +
> + 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..e08c505c148
> --- /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;Missing \n here.
Show 5 quoted lines
> 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);
This post-image is mis-indented.
> #include "simple-ipc.h" > > +struct repository; > +
I think we'd usually forard-declare such things, if needed...
Show 8 quoted lines
> /* > * 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);
..right before they're needed, so before this line?
> +const char *fsmonitor_ipc__get_path(struct repository *r); > > /* > * Try to determine whether there is a `git-fsmonitor--daemon` process