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

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

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.