On Fri, Jul 16 2021, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> 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.
