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

On Mon, Jul 19 2021, Jeff Hostetler wrote:

On 7/1/21 6:45 PM, Ævar Arnfjörð Bjarmason wrote:

quoted

On Thu, Jul 01 2021, Jeff Hostetler via GitGitGadget wrote:

quoted

From: Jeff Hostetler <redacted>

Stub in empty backend for fsmonitor--daemon on Windows.

Signed-off-by: Jeff Hostetler <redacted>
---
  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 (local)

I think that's much more interesting than this relatively light-reading
bikeshedding I had while giving this a read-through.