> -----Original Message----- > From: Junio C Hamano <gitster@xxxxxxxxx> > Sent: Friday, October 14, 2022 7:46 PM > To: Eric DeCosta via GitGitGadget <gitgitgadget@xxxxxxxxx> > Cc: git@xxxxxxxxxxxxxxx; Eric Sunshine <sunshine@xxxxxxxxxxxxxx>; Ævar > Arnfjörð Bjarmason <avarab@xxxxxxxxx>; Eric DeCosta > <edecosta@xxxxxxxxxxxxx> > Subject: Re: [PATCH v2 07/12] fsmonitor: prepare to share code between > Mac OS and Linux > > "Eric DeCosta via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes: > > > ifdef FSMONITOR_DAEMON_BACKEND > > COMPAT_CFLAGS += -DHAVE_FSMONITOR_DAEMON_BACKEND > > + ifdef FSMONITOR_DAEMON_COMMON > > + COMPAT_OBJS += compat/fsmonitor/fsm-ipc- > $(FSMONITOR_DAEMON_COMMON).o > > + else > > + COMPAT_OBJS += compat/fsmonitor/fsm-ipc- > $(FSMONITOR_DAEMON_BACKEND).o > > + endif > > 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 > > COMPAT_CFLAGS += -DHAVE_FSMONITOR_OS_SETTINGS > > + ifdef FSMONITOR_DAEMON_COMMON > > + COMPAT_OBJS += compat/fsmonitor/fsm-settings- > $(FSMONITOR_DAEMON_COMMON).o > > + endif > > COMPAT_OBJS += compat/fsmonitor/fsm-settings- > $(FSMONITOR_OS_SETTINGS).o > > COMPAT_OBJS += > > compat/fsmonitor/fsm-path-utils-$(FSMONITOR_OS_SETTINGS).o > > endif > > Ugly. > > One overrides backend with common, while the other one doesn't. > That asymmetry alone should stop us and wonder if there is something fishy > in the approach that can be improved. It makes it look like the word > "common" means something quite different between the -ipc and the - > settings world. > > I suspect that in both, you should not expose "unix" to this part of the > Makefile. Linux and macOS occasionally being similar in some places does > not have to be exposed here. INstead you can use backend "linux" and > "macos", whose C sources may include from a separate C source file whose > name may contain "unix". That would allow you to get rid of > FSMONITOR_DAEMON_COMMON in a cleaner way. Let me see if I am understanding you correctly. Are you suggesting something like: ifdef FSMONITOR_DAEMON_BACKEND_LINUX COMPAT_CFLAGS += -DHAVE_FSMONITOR_DAEMON_BACKEND COMPAT_OBJS += compat/fsmonitor/fsm-ipc-unix.o COMPAT_OBJS += compat/fsmonitor/fsm-ipc-$(FSMONITOR_DAEMON_BACKEND_LINUX).o COMPAT_OBJS += compat/fsmonitor/fsm-listen-$(FSMONITOR_DAEMON_BACKEND_LINUX).o COMPAT_OBJS += compat/fsmonitor/fsm-health-$(FSMONITOR_DAEMON_BACKEND_LINUX).o endif ifdef FSMONITOR_DAEMON_BACKEND_DARWIN COMPAT_CFLAGS += -DHAVE_FSMONITOR_DAEMON_BACKEND COMPAT_OBJS += compat/fsmonitor/fsm-ipc-unix.o COMPAT_OBJS += compat/fsmonitor/fsm-ipc-$(FSMONITOR_DAEMON_BACKEND_DARWIN).o COMPAT_OBJS += compat/fsmonitor/fsm-listen-$(FSMONITOR_DAEMON_BACKEND_DARWIN).o COMPAT_OBJS += compat/fsmonitor/fsm-health-$(FSMONITOR_DAEMON_BACKEND_DARWIN).o endif And similarly for settings: ifdef FSMONITOR_DAEMON_SETTINGS_LINUX COMPAT_CFLAGS += -DHAVE_FSMONITOR_OS_SETTINGS COMPAT_OBJS += compat/fsmonitor/fsm-settings-unix.o COMPAT_OBJS += compat/fsmonitor/fsm-settings-$(FSMONITOR_DAEMON_SETTINGS_LINUX).o COMPAT_OBJS += compat/fsmonitor/fsm-path-utils-$(FSMONITOR_DAEMON_SETTINGS_LINUX).o endif ifdef FSMONITOR_DAEMON_SETTINGS_DARWIN COMPAT_CFLAGS += -DHAVE_FSMONITOR_OS_SETTINGS COMPAT_OBJS += compat/fsmonitor/fsm-settings-unix.o COMPAT_OBJS += compat/fsmonitor/fsm-settings-$(FSMONITOR_DAEMON_SETTINGS_DARWIN).o COMPAT_OBJS += compat/fsmonitor/fsm-path-utils-$(FSMONITOR_DAEMON_SETTINGS_DARWIN).o endif And the rest of the platforms remain as was, with FSMONITOR_DAEMON_BACKEND and FSMONITOR_OS_SETTINGS -Eric
