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

From: Ævar Arnfjörð Bjarmason <[email-masked]>
Date: Wed, 09 Mar 2022 15:14:40 +0100

On Tue, Mar 08 2022, Jeff Hostetler wrote:

> On 3/7/22 5:37 AM, Ævar Arnfjörð Bjarmason wrote:
>> On Tue, Mar 01 2022, Jeff Hostetler via GitGitGadget wrote:
>> 
>>> From: Jeff Hostetler <[email-masked]>
>>> [...]
> [...]
>> 
>>> +#else
>>> +/*
>>> + * Let Apple's headers declare `isalnum()` first, before
>>> + * Git's headers override it via a constant
>>> + */
>> 
>> 
>>> +#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.
>> 
>>> +
>>>   #include "cache.h"
>>>   #include "fsmonitor.h"
>>>   #include "fsm-listen.h"
>> 
>
> You may be right here.  I commented out the <string.h> and
> the <...CoreFoundation.h> lines and everything still compiled
> and t7527 passed.
>
> I'm not sure why <string.h> was added here (I inherited that
> file when I took over the feature).  It may be that recent SDK
> updates have eliminated the need for it.  Or it may be that it
> was never necessary.  (However, the comment above it suggests
> that there was a problem in the past.)
>
> 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.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...