Re: [PATCH v3 11/34] fsmonitor-fs-listen-win32: stub in backend for Windows

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

On Fri, Jul 16 2021, Johannes Schindelin wrote:

> Hi Ævar,
>
> On Fri, 2 Jul 2021, Ævar Arnfjörð Bjarmason wrote:
>
>>
>> On Thu, Jul 01 2021, Jeff Hostetler via GitGitGadget wrote:
>>
>> > From: Jeff Hostetler <jeffhost@microsoft.com>
>> >
>> > Stub in empty backend for fsmonitor--daemon on Windows.
>> >
>> > Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
>> > ---
>> >  Makefile                                     | 13 ++++++
>> >  compat/fsmonitor/fsmonitor-fs-listen-win32.c | 21 +++++++++
>> >  compat/fsmonitor/fsmonitor-fs-listen.h       | 49 ++++++++++++++++++++
>> >  config.mak.uname                             |  2 +
>> >  contrib/buildsystems/CMakeLists.txt          |  5 ++
>> >  5 files changed, 90 insertions(+)
>> >  create mode 100644 compat/fsmonitor/fsmonitor-fs-listen-win32.c
>> >  create mode 100644 compat/fsmonitor/fsmonitor-fs-listen.h
>> >
>> > diff --git a/Makefile b/Makefile
>> > index c45caacf2c3..a2a6e1f20f6 100644
>> > --- a/Makefile
>> > +++ b/Makefile
>> > @@ -467,6 +467,11 @@ all::
>> >  # directory, and the JSON compilation database 'compile_commands.json' will be
>> >  # created at the root of the repository.
>> >  #
>> > +# If your platform supports a built-in fsmonitor backend, set
>> > +# FSMONITOR_DAEMON_BACKEND to the "<name>" of the corresponding
>> > +# `compat/fsmonitor/fsmonitor-fs-listen-<name>.c` that implements the
>> > +# `fsmonitor_fs_listen__*()` routines.
>> > +#
>> >  # Define DEVELOPER to enable more compiler warnings. Compiler version
>> >  # and family are auto detected, but could be overridden by defining
>> >  # COMPILER_FEATURES (see config.mak.dev). You can still set
>> > @@ -1929,6 +1934,11 @@ ifdef NEED_ACCESS_ROOT_HANDLER
>> >  	COMPAT_OBJS += compat/access.o
>> >  endif
>> >
>> > +ifdef FSMONITOR_DAEMON_BACKEND
>> > +	COMPAT_CFLAGS += -DHAVE_FSMONITOR_DAEMON_BACKEND
>> > +	COMPAT_OBJS += compat/fsmonitor/fsmonitor-fs-listen-$(FSMONITOR_DAEMON_BACKEND).o
>> > +endif
>> > +
>> >  ifeq ($(TCLTK_PATH),)
>> >  NO_TCLTK = NoThanks
>> >  endif
>> > @@ -2793,6 +2803,9 @@ GIT-BUILD-OPTIONS: FORCE
>> >  	@echo PAGER_ENV=\''$(subst ','\'',$(subst ','\'',$(PAGER_ENV)))'\' >>$@+
>> >  	@echo DC_SHA1=\''$(subst ','\'',$(subst ','\'',$(DC_SHA1)))'\' >>$@+
>> >  	@echo X=\'$(X)\' >>$@+
>> > +ifdef FSMONITOR_DAEMON_BACKEND
>> > +	@echo FSMONITOR_DAEMON_BACKEND=\''$(subst ','\'',$(subst ','\'',$(FSMONITOR_DAEMON_BACKEND)))'\' >>$@+
>> > +endif
>>
>> Why put this in an ifdef?
>
> Why not? What benefit does this question bring to improving this patch
> series?

I think that when adding code to the Makefile it makes sense to follow
the prevailing pattern, unless there's a good reason to do otherwise,
e.g. on my build:
	
	$ grep "''" GIT-BUILD-OPTIONS 
	NO_CURL=''
	NO_EXPAT=''
	NO_PERL=''
	NO_PTHREADS=''
	NO_PYTHON=''
	NO_UNIX_SOCKETS=''
	X=''

Why does the FSMONITOR_DAEMON_BACKEND option require a nonexistent line
as opposed to an empty one?