| Message ID | 5a9bda7220356ebf0689bb6aaa9068520dc6e33b.1625150864.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Builtin FSMonitor Feature | expand |
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? In 342e9ef2d9e (Introduce a performance testing framework, 2012-02-17) we started doing that for some perf/test options (which b.t.w., I don't really see the reason for, maybe it's some subtlety in how test-lib.sh picks those up). But for all the other compile-time stuff we don't ifdef it, we just define it, and then you get an empty value or not. This would AFAICT be the first build-time-for-the-C-program option we ifdef for writing a line to GIT-BUILD-OPTIONS.
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? Ciao, Dscho
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?
Johannes Schindelin wrote: > On Fri, 2 Jul 2021, Ævar Arnfjörð Bjarmason wrote: > > On Thu, Jul 01 2021, Jeff Hostetler via GitGitGadget wrote: > > > @@ -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? This is a common debate tactic known as "shifting the burden of proof". Ævar does not need to prove that your patch is undesirable, *you* have to prove that it is desirable. You have the burden of proof, so you should answer the question. https://www.logicallyfallacious.com/logicalfallacies/Shifting-of-the-Burden-of-Proof
Ævar Arnfjörð Bjarmason <avarab@gmail.com> 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.
Junio C Hamano <gitster@pobox.com> writes: > #!/bin/sh > cat >make.file <<\EOF > all:: > ifeq ($(FSMONITOR_DAEMON_BACKEND),) > echo it is empty > endif > ifdef FSMONITOR_DAEMON_BACKEND An obvious typo. This must be "ifndef", of course. > 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.
On Fri, Jul 16 2021, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> 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.
On 7/1/21 6:45 PM, Æ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? > > In 342e9ef2d9e (Introduce a performance testing framework, 2012-02-17) > we started doing that for some perf/test options (which b.t.w., I don't > really see the reason for, maybe it's some subtlety in how test-lib.sh > picks those up). > > But for all the other compile-time stuff we don't ifdef it, we just > define it, and then you get an empty value or not. > > This would AFAICT be the first build-time-for-the-C-program option we > ifdef for writing a line to GIT-BUILD-OPTIONS. > (I'm going to respond here on the original question rather than on any of the follow up responses in an attempt at diffusing things a bit.) I added the ifdef because I thought it to be the *most conservative* thing that I could do. The output of the generated file on unsupported platforms should be *identical* to what it was before my changes. I only alter the contents of the generated file on supported platforms. Later, when the generated file is consumed, we don't need to worry about the effect (if any) on incremental compiles -- we will know that it won't be set -- just like it was not set in the original compile. That change appears right before a 12 other ifdef'd symbols also being written to that generated file. Most are test and perf, but some are not. But my point is that the pattern is present already. The original question also references a 9.5 year old commit which uses the same pattern as I've used here. It also muddies the water on why it was/wasn't needed back then. And hints at possible side-effects in some of our test scripts. So it is clear that the confusion/disagreements that we are having with the current patch and whether or not to ifdef are not new. So, is there value in being explicit and having the ifdef ?? There are well defined Make rules (and Junio gave us a very elegant little script to demonstrate that), but the subtleties are there. Especially with our use generated files like `GIT-BUILD-OPTIONS`. We have a mailing list full of experts and yet this question received a lot more discussion than I thought possible or necessary, but it took a test script to demonstrate that the results are the same and it doesn't matter. Perhaps the clarity is worth it for the price of a simple ifdef. So, how much time have we (collectively) wasted discussing this subtlety ?? To summarize, I added the ifdef to make it explicitly clear that I'm not altering behavior on unsupported platforms. I can remove it from V4 if desired or I can keep it. (We all now know that it doesn't functionally matter -- it does however, provide clarity.) Sorry if this sounded like a rant, Jeff
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: >>>>> Why put this in an ifdef? >>>> ... >>> 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 >> ifndef 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. Sorry, but I still do not get the question. There are bunch of ifndef in Makefile in addition to ifeq/ifneq and your question FSMONITOR_DAEMON_BACKEND option require a nonexistent line as opposed to an empty one? is asking "why is it X" when X is not quite true. I presume that your "wrapping it in an ifdef" refers to a construct like this: >>> > +ifdef FSMONITOR_DAEMON_BACKEND >>> > + COMPAT_CFLAGS += -DHAVE_FSMONITOR_DAEMON_BACKEND >>> > + COMPAT_OBJS += compat/fsmonitor/fsmonitor-fs-listen-$(FSMONITOR_DAEMON_BACKEND).o >>> > +endif but is your suggestion that it should be written like this instead? >>> > +ifneq ($(FSMONITOR_DAEMON_BACKEND),) >>> > + COMPAT_CFLAGS += -DHAVE_FSMONITOR_DAEMON_BACKEND >>> > + COMPAT_OBJS += compat/fsmonitor/fsmonitor-fs-listen-$(FSMONITOR_DAEMON_BACKEND).o >>> > +endif I do not think the latter is any easier to follow (and we have many ifdef and ifndef in our Makefile already). Perhaps I will see what you mean when I see your "better alternative", but so far, I am not successfully guessing what it is.
On Mon, Jul 19 2021, Jeff Hostetler wrote: > On 7/1/21 6:45 PM, Æ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? >> In 342e9ef2d9e (Introduce a performance testing framework, >> 2012-02-17) >> we started doing that for some perf/test options (which b.t.w., I don't >> really see the reason for, maybe it's some subtlety in how test-lib.sh >> picks those up). >> But for all the other compile-time stuff we don't ifdef it, we just >> define it, and then you get an empty value or not. >> This would AFAICT be the first build-time-for-the-C-program option >> we >> ifdef for writing a line to GIT-BUILD-OPTIONS. >> > > (I'm going to respond here on the original question rather than on any > of the follow up responses in an attempt at diffusing things a bit.) > > I added the ifdef because I thought it to be the *most conservative* > thing that I could do. The output of the generated file on unsupported > platforms should be *identical* to what it was before my changes. I > only alter the contents of the generated file on supported platforms. > > Later, when the generated file is consumed, we don't need to worry about > the effect (if any) on incremental compiles -- we will know that it > won't be set -- just like it was not set in the original compile. Okey, so e.g. when we added e.g. USE_LIBPCRE2 we added it TO GIT-BUILD-OPTIONS unconditionally, so if you pulled that commit you'd trigger a rebuild on anything that cares about GIT-BUILD-OPTIONS (which is almost everything). But you'd like to have the line not added to avoid that one-off recompile.... > That change appears right before a 12 other ifdef'd symbols also being > written to that generated file. Most are test and perf, but some are > not. But my point is that the pattern is present already. > > The original question also references a 9.5 year old commit which > uses the same pattern as I've used here. It also muddies the water > on why it was/wasn't needed back then. And hints at possible > side-effects in some of our test scripts. So it is clear that the > confusion/disagreements that we are having with the current patch > and whether or not to ifdef are not new. > > > So, is there value in being explicit and having the ifdef ?? > > > There are well defined Make rules (and Junio gave us a very elegant > little script to demonstrate that), but the subtleties are there. > Especially with our use generated files like `GIT-BUILD-OPTIONS`. > We have a mailing list full of experts and yet this question received > a lot more discussion than I thought possible or necessary, but it > took a test script to demonstrate that the results are the same and it > doesn't matter. Perhaps the clarity is worth it for the price of a > simple ifdef. > > > So, how much time have we (collectively) wasted discussing this > subtlety ?? > > > To summarize, I added the ifdef to make it explicitly clear that > I'm not altering behavior on unsupported platforms. I can remove it > from V4 if desired or I can keep it. (We all now know that it doesn't > functionally matter -- it does however, provide clarity.) > > > Sorry if this sounded like a rant, ...I asked because I've looked at that ifdef soup around GIT-BUILD-OPTIONS and wondered if I could make it go away, and before a patch lands is a good time to ask "what's this pattern for?", as opposed to inferring this after the fact. For me it was just a minor curiosity, I didn't expect to start this big discussion about it. I expected just a "oh, I just copy/pasted that from the lines at the end" or something, which would be fair enough. I really don't care which one we go for here. If you want to change it fine, if not that's fine too. I have noticed a pattern where you seem to really carefully consider why you'd like X over Y. I.e. it wasn't just copy/pasting in this case if I understand you correctly, but a carefully thought out decision to not do it like the other C-level-GIT-BUILD-OPTIONS. Okey, fair enough, but that decision then doesn't go into the commit message, and then when I innocently ask about it... ..I guess I'll stop before this starts resembling a rant on my part :) Anyway, I have also had really non-trivial comments on this fsmonitor series, not just a few bikeshed comments. I.e. the un-addressed question about the wildly different performance numbers we seem to have seen in our respective testing: https://lore.kernel.org/git/871r8c73ej.fsf@evledraar.gmail.com I think that's much more interesting than this relatively light-reading bikeshedding I had while giving this a read-through.
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 ifdef TEST_OUTPUT_DIRECTORY @echo TEST_OUTPUT_DIRECTORY=\''$(subst ','\'',$(subst ','\'',$(TEST_OUTPUT_DIRECTORY)))'\' >>$@+ endif diff --git a/compat/fsmonitor/fsmonitor-fs-listen-win32.c b/compat/fsmonitor/fsmonitor-fs-listen-win32.c new file mode 100644 index 00000000000..880446b49e3 --- /dev/null +++ b/compat/fsmonitor/fsmonitor-fs-listen-win32.c @@ -0,0 +1,21 @@ +#include "cache.h" +#include "config.h" +#include "fsmonitor.h" +#include "fsmonitor-fs-listen.h" + +void fsmonitor_fs_listen__stop_async(struct fsmonitor_daemon_state *state) +{ +} + +void fsmonitor_fs_listen__loop(struct fsmonitor_daemon_state *state) +{ +} + +int fsmonitor_fs_listen__ctor(struct fsmonitor_daemon_state *state) +{ + return -1; +} + +void fsmonitor_fs_listen__dtor(struct fsmonitor_daemon_state *state) +{ +} diff --git a/compat/fsmonitor/fsmonitor-fs-listen.h b/compat/fsmonitor/fsmonitor-fs-listen.h new file mode 100644 index 00000000000..c7b5776b3b6 --- /dev/null +++ b/compat/fsmonitor/fsmonitor-fs-listen.h @@ -0,0 +1,49 @@ +#ifndef FSMONITOR_FS_LISTEN_H +#define FSMONITOR_FS_LISTEN_H + +/* This needs to be implemented by each backend */ + +#ifdef HAVE_FSMONITOR_DAEMON_BACKEND + +struct fsmonitor_daemon_state; + +/* + * Initialize platform-specific data for the fsmonitor listener thread. + * This will be called from the main thread PRIOR to staring the + * fsmonitor_fs_listener thread. + * + * Returns 0 if successful. + * Returns -1 otherwise. + */ +int fsmonitor_fs_listen__ctor(struct fsmonitor_daemon_state *state); + +/* + * Cleanup platform-specific data for the fsmonitor listener thread. + * This will be called from the main thread AFTER joining the listener. + */ +void fsmonitor_fs_listen__dtor(struct fsmonitor_daemon_state *state); + +/* + * The main body of the platform-specific event loop to watch for + * filesystem events. This will run in the fsmonitor_fs_listen thread. + * + * It should call `ipc_server_stop_async()` if the listener thread + * prematurely terminates (because of a filesystem error or if it + * detects that the .git directory has been deleted). (It should NOT + * do so if the listener thread receives a normal shutdown signal from + * the IPC layer.) + * + * It should set `state->error_code` to -1 if the daemon should exit + * with an error. + */ +void fsmonitor_fs_listen__loop(struct fsmonitor_daemon_state *state); + +/* + * Gently request that the fsmonitor listener thread shutdown. + * It does not wait for it to stop. The caller should do a JOIN + * to wait for it. + */ +void fsmonitor_fs_listen__stop_async(struct fsmonitor_daemon_state *state); + +#endif /* HAVE_FSMONITOR_DAEMON_BACKEND */ +#endif /* FSMONITOR_FS_LISTEN_H */ diff --git a/config.mak.uname b/config.mak.uname index cb443b4e023..fcd88b60b14 100644 --- a/config.mak.uname +++ b/config.mak.uname @@ -420,6 +420,7 @@ ifeq ($(uname_S),Windows) # so we don't need this: # # SNPRINTF_RETURNS_BOGUS = YesPlease + FSMONITOR_DAEMON_BACKEND = win32 NO_SVN_TESTS = YesPlease RUNTIME_PREFIX = YesPlease HAVE_WPGMPTR = YesWeDo @@ -598,6 +599,7 @@ ifneq (,$(findstring MINGW,$(uname_S))) NO_STRTOUMAX = YesPlease NO_MKDTEMP = YesPlease NO_SVN_TESTS = YesPlease + FSMONITOR_DAEMON_BACKEND = win32 RUNTIME_PREFIX = YesPlease HAVE_WPGMPTR = YesWeDo NO_ST_BLOCKS_IN_STRUCT_STAT = YesPlease diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt index a87841340e6..1ab94eb284f 100644 --- a/contrib/buildsystems/CMakeLists.txt +++ b/contrib/buildsystems/CMakeLists.txt @@ -263,6 +263,11 @@ else() endif() endif() +if(CMAKE_SYSTEM_NAME STREQUAL "Windows") + add_compile_definitions(HAVE_FSMONITOR_DAEMON_BACKEND) + list(APPEND compat_SOURCES compat/fsmonitor/fsmonitor-fs-listen-win32.c) +endif() + set(EXE_EXTENSION ${CMAKE_EXECUTABLE_SUFFIX}) #header checks