Re: [PATCH v6 16/30] compat/fsmonitor/fsm-listen-darwin: add MacOS header files for FSEvent

Hi Ævar,

On Wed, 9 Mar 2022, Ævar Arnfjörð Bjarmason wrote:

On Tue, Mar 08 2022, Jeff Hostetler wrote:

quoted

On 3/7/22 5:37 AM, Ævar Arnfjörð Bjarmason wrote:

quoted

On Tue, Mar 01 2022, Jeff Hostetler via GitGitGadget wrote:

quoted

From: Jeff Hostetler <redacted>
[...]

[...]

quoted

quoted

+#else
+/*
+ * Let Apple's headers declare `isalnum()` first, before
+ * Git's headers override it via a constant
+ */

quoted

+#include <string.h>
+#include <CoreFoundation/CoreFoundation.h>
+#include <CoreServices/CoreServices.h>
+#endif

In cache.h which you'rejust about to include we don't include
string.h, but we do in git-compat-util.h, but that one includes
string.h before doing those overrides.

This either isn't needed, or really should be some addition to
git-compat-util.h instead. I.e. if we've missed some edge case with
string.h and ctype.h on OSX we should handle that in git-compat-util.h
rather than <some other file/header> needing a portability workaround.

[...]

While it may not (now at least) be necessary, it's not doing
any harm, so I'd rather leave it and not interrupt things.
We can always revisit it later if we want.

In terms of figuring out some mysterious portability issue, I think the
right time is now rather than later.

I do not see that.

In FSMonitor, this was clearly a problem that needed to be solved (and if
you try to compile on an older macOS, you will run into those problems
again).

If you are talking about the mysterious portability issue with an eye on
`git-compat-util.h`, well... you can successfully compile Git's source
code without this hack in `git-compat-util.h`. That's why the hack is not
needed. Problem solved. Actually, there was not even a problem.

I.e. now this doesn't have anyone relying on it, so we can easily
(re)discover whatever issue this was trying to solve.

Whereas anyone who'd need to figure out why we include string.h on OSX
early in this case later would be left with this otherwise dead-end
thread, and a change at that point would possibly break existing code...

Anyone who would need to figure out why we `#include` this header early
would read the comment about `isalnum()`, I would hope, and understand
that there are circumstances when Git's `isalnum()` macro interferes with
Apple's, and that this `#include` order addresses that problem.

They might even get to the point where they find
https://github.com/dscho/git/commit/0f89c726a1912dce2bdab14aff8ebfec8550340d,
maybe even the "original original" commit at
https://github.com/kewillford/git/commit/d11ee4698c63347f40a8993ab86ee4e97f695d9b,
which was a still-experimental version of the macOS backend, and where the
`#include` order clearly mattered, else why would Kevin have bothered.

Further, I strongly suspect that it had something to do with
`CoreFoundation.h` or with `CoreServices.h` being `#include`d, and if you
care to check the code above the quoted lines, you will see that you
cannot even `#include` those headers using GCC, it only works with clang:
https://github.com/jeffhostetler/git/commit/cdef9730b3f93a6f0f98d68ffb81bcb89d6e698e#diff-4e865160016fe490b80ad11273a10daca8bc412a75f2da4c6b08fb9e5e3b5e18R3

At this stage, anybody investigating this issue who is a little bit like
me would then be a bit bored with the investigation because there is
actually no breakage here, just a curious `#include` order, and nothing
else. So they might then drop it and move along.

Even you might find it a more satisfying use of your time to implement,
say, a Linux backend for FSMonitor on top of Jeff's work, instead of
worrying about non-issues ;-)

Really, there are so many more interesting issues to discuss than this
`#include` non-issue. And on this note, I will steer my attention to
precisely such more interesting issues.

Ciao,
Johannes