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

From: Jeff Hostetler <git@jeffhostetler.com>
Date: Mar 8, 2022, 21:09 UTC
Message-ID: <aa6276f9-8d10-22f9-bfc0-2aa718d002e1@jeffhostetler.com>
In-Reply-To: <220307.86h78a2gcn.gmgdl@evledraar.gmail.com>

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

Show 5 quoted lines

> 
> On Tue, Mar 01 2022, Jeff Hostetler via GitGitGadget wrote:
> 
>> From: Jeff Hostetler <jeffhost@microsoft.com>
>> [...]

[...]

Show 29 quoted lines

> 
>> +#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.

Thanks, Jeff