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

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Tue, Oct 04 2022, Eric DeCosta via GitGitGadget wrote:

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

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

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

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

>  /*
>   * 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