Re: [PATCH v7 06/10] fsmonitor: deduplicate settings logic for Unix platforms
- From
Patrick Steinhardt <ps@pks.im>
- Date
- Mar 4, 2026, 07:43 UTC
- 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:
Show 21 quoted lines
> 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?
Show 56 quoted lines
> 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.
Show 18 quoted lines
> 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...
Show 15 quoted lines
> @@ -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