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

From: Ævar Arnfjörð Bjarmason <[email-masked]>
Date: Sat, 17 Jul 2021 23:43:00 +0200

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

> Ævar Arnfjörð Bjarmason <[email-masked]> writes:
>
>>>> > +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.