Re: [PATCH v6 05/10] fsmonitor: deduplicate IPC path logic for Unix platforms

[Junio C Hamano <gitster@pobox.com>]

"Paul Tarjan via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Paul Tarjan <github@paulisageek.com>
>
> The IPC path logic for determining the Unix domain socket location is
> nearly identical between macOS and Linux.  Both need to check whether
> the .git directory is on a remote filesystem and, if so, fall back to
> a socket path under $HOME or a user-configured directory.
>
> Merge the two implementations into a single fsm-ipc-unix.c that is
> shared by both platforms.  The unified version includes the worktree
> NULL check (BUG guard) from the Linux implementation, which was missing
> in the macOS version.
>
> Update Makefile, meson.build, and CMakeLists.txt to use the new shared
> file for non-Windows platforms.

This sounds as if the patch started with two IPC path logic for
macOS and Linux, and the patch removes one of them and updates the
other one so that both platforms can use the surviving one.

But the patch seems to indicate somewhat different story.  The code
before this patch started with a single macOS (darwin) one, but
because it is mostly applicable to other UNIX variants as well, the
patch renames the existing macOS one for unix and makes a small
adjustment (namely, asserts that r->worktree is not NULL).

Perhaps you started with two separate implementations (possibly with
a new one called UNIX that was added by largely copying and pasting
from the macOS one) and unified them, but that history does not
exist in this 10-patch series, so the above story would need to be
rewritten to say what actually is happening in this series, e.g., we
realized macOS one is applicable generally for UNIX variants so we
are renaming it from darwin to unix, or something.

> @@ -2365,7 +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
> -	COMPAT_OBJS += compat/fsmonitor/fsm-ipc-$(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

Makes me wonder if doing

    #define FSMONITOR_DAEMON_BACKEND unix

for macOS and then keeping

	COMPAT_OBJS += compat/fsmonitor/fsm-ipc-$(FSMONITOR_DAEMON_BACKEND).o

as-is would be cleaner.  

Later, we can add the same "#define FSMONITOR_DAEMON_BACKEND unix"
for Linux, perhaps?

This is doubly true when we look at the build recipe changes to the
meson one below ...

> diff --git a/meson.build b/meson.build
> index dd52efd1c8..8de795f9d4 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -1332,11 +1332,16 @@ if fsmonitor_backend != ''
>  
>    libgit_sources += [
>      'compat/fsmonitor/fsm-health-' + fsmonitor_backend + '.c',
> -    'compat/fsmonitor/fsm-ipc-' + 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',
>    ]
> +
> +  if fsmonitor_backend == 'win32'
> +    libgit_sources += 'compat/fsmonitor/fsm-ipc-win32.c'
> +  else
> +    libgit_sources += 'compat/fsmonitor/fsm-ipc-unix.c'
> +  endif
>  endif

... which would not be needed if fsmonitor_backend is set to 'unix'
instead of 'darwin'.