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

On Fri, Jul 16 2021, Junio C Hamano wrote:

Ævar Arnfjörð Bjarmason [off-list ref] writes:

quoted

quoted

quoted

quoted

+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

...

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?

I do not quite get the question.

#!/bin/sh
cat >make.file <<\EOF
all::
ifeq ($(FSMONITOR_DAEMON_BACKEND),)
	echo it is empty
endif
ifdef FSMONITOR_DAEMON_BACKEND
	echo it is undefined
endif
EOF

echo "unset???"
make -f make.file

echo "set to empty???"
make -f make.file FSMONITOR_DAEMON_BACKEND=

These two make invocations will give us the same result, showing
that "is it set to empty" and "is it unset" are the same.

Indeed, which is why I'm pointing out that wrapping it in an ifdef is
pointless, which is why we don't do it for the other ones.

We do have a bunch of ifdef'd things there for perf etc., I'm not sure
if it matters or not for those.