From: Patrick Steinhardt <ps@pks.im>
Date: Wed, 04 Mar 2026 07:43:02 GMT
Subject: Re: [PATCH v7 06/10] fsmonitor: deduplicate settings logic for Unix platforms
Message-ID: <aafihm4MVoVOaD2l@pks.im>
In-Reply-To: <0a83bb9c8e71f6c388a50eb62afd03680020eb94.1772065643.git.gitgitgadget@gmail.com>

On Thu, Feb 26, 2026 at 12:27:19AM +0000, Paul Tarjan via GitGitGadget wrote:
> diff --git a/Makefile b/Makefile
> index 7480ce3e1d..062347997a 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -2365,15 +2365,11 @@ 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
> -ifeq ($(FSMONITOR_DAEMON_BACKEND),win32)
> -	COMPAT_OBJS += compat/fsmonitor/fsm-ipc-win32.o
> -else
> -	COMPAT_OBJS += compat/fsmonitor/fsm-ipc-unix.o
> -endif
>  endif
>  
>  ifdef FSMONITOR_OS_SETTINGS
>  	COMPAT_CFLAGS += -DHAVE_FSMONITOR_OS_SETTINGS
> +	COMPAT_OBJS += compat/fsmonitor/fsm-ipc-$(FSMONITOR_OS_SETTINGS).o
>  	COMPAT_OBJS += compat/fsmonitor/fsm-settings-$(FSMONITOR_OS_SETTINGS).o
>  	COMPAT_OBJS += compat/fsmonitor/fsm-path-utils-$(FSMONITOR_DAEMON_BACKEND).o
>  endif

It's a bit weird that the only change to the Makefile here is to change
how we wire up fsm-ipc even though it's fsm-settings that this commit
cares about. Should this change be moved into the preceding commit?

> diff --git a/compat/fsmonitor/fsm-settings-darwin.c b/compat/fsmonitor/fsm-settings-unix.c
> similarity index 82%
> rename from compat/fsmonitor/fsm-settings-darwin.c
> rename to compat/fsmonitor/fsm-settings-unix.c
> index a382590635..27d89207af 100644
> --- a/compat/fsmonitor/fsm-settings-darwin.c
> +++ b/compat/fsmonitor/fsm-settings-unix.c
> @@ -5,7 +5,7 @@
>  #include "fsmonitor-settings.h"
>  #include "fsmonitor-path-utils.h"
>  
> - /*
> +/*
>   * For the builtin FSMonitor, we create the Unix domain socket for the
>   * IPC in the .git directory.  If the working directory is remote,
>   * then the socket will be created on the remote file system.  This
> @@ -22,25 +22,31 @@
>   * The builtin FSMonitor uses a Unix domain socket in the .git
>   * directory for IPC.  These Windows drive formats do not support
>   * Unix domain sockets, so mark them as incompatible for the daemon.
> - *
>   */
>  static enum fsmonitor_reason check_uds_volume(struct repository *r)
>  {
>  	struct fs_info fs;
>  	const char *ipc_path = fsmonitor_ipc__get_path(r);
> -	struct strbuf path = STRBUF_INIT;
> -	strbuf_add(&path, ipc_path, strlen(ipc_path));
> +	char *path;
> +	char *dir;
> +
> +	/*
> +	 * Create a copy for dirname() since it may modify its argument.
> +	 */
> +	path = xstrdup(ipc_path);
> +	dir = dirname(path);
>  
> -	if (fsmonitor__get_fs_info(dirname(path.buf), &fs) == -1) {
> -		strbuf_release(&path);
> +	if (fsmonitor__get_fs_info(dir, &fs) == -1) {
> +		free(path);
>  		return FSMONITOR_REASON_ERROR;
>  	}
>  
> -	strbuf_release(&path);
> +	free(path);
>  
>  	if (fs.is_remote ||
> -		!strcmp(fs.typename, "msdos") ||
> -		!strcmp(fs.typename, "ntfs")) {
> +	    !strcmp(fs.typename, "msdos") ||
> +	    !strcmp(fs.typename, "ntfs") ||
> +	    !strcmp(fs.typename, "vfat")) {
>  		free(fs.typename);
>  		return FSMONITOR_REASON_NOSOCKETS;
>  	}

Same here, it's not clear to me where those while-at-it changes are
coming from and whether we need them here. I'd rather drop them.

> diff --git a/meson.build b/meson.build
> index 8de795f9d4..589624f399 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -1320,10 +1320,13 @@ else
>  endif
>  
>  fsmonitor_backend = ''
> +fsmonitor_os = ''
>  if host_machine.system() == 'windows'
>    fsmonitor_backend = 'win32'
> +  fsmonitor_os = 'win32'
>  elif host_machine.system() == 'darwin'
>    fsmonitor_backend = 'darwin'
> +  fsmonitor_os = 'unix'
>    libgit_dependencies += dependency('CoreServices')
>  endif
>  if fsmonitor_backend != ''

I think it might make sense to introduce the `fsmonitor_os` variable in
the preceding commit already. If so...

> @@ -1334,17 +1337,12 @@ if fsmonitor_backend != ''
>      'compat/fsmonitor/fsm-health-' + fsmonitor_backend + '.c',
>      'compat/fsmonitor/fsm-listen-' + fsmonitor_backend + '.c',
>      'compat/fsmonitor/fsm-path-utils-' + fsmonitor_backend + '.c',
> -    'compat/fsmonitor/fsm-settings-' + fsmonitor_backend + '.c',
> +    'compat/fsmonitor/fsm-ipc-' + fsmonitor_os + '.c',
> +    'compat/fsmonitor/fsm-settings-' + fsmonitor_os + '.c',
>    ]
> -
> -  if fsmonitor_backend == 'win32'
> -    libgit_sources += 'compat/fsmonitor/fsm-ipc-win32.c'
> -  else
> -    libgit_sources += 'compat/fsmonitor/fsm-ipc-unix.c'
> -  endif
>  endif

We could avoid the flip-flopping of the code here.

Patrick