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

> 
> On Thu, Mar 10 2022, Johannes Schindelin wrote:
> 
>> Hi Ævar,
>>
>> On Wed, 9 Mar 2022, Ævar Arnfjörð Bjarmason wrote:
>>
>>> 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 <jeffhost@microsoft.com>
>>>>>> [...]
>>>> [...]
>>>>>
>>>>>> +#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.
>>>>
>>>> [...]
>>>>
>>>> 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).
> 
> So you can reproduce an issue where removing the "#include <string.h>"
> from compat/fsmonitor/fsm-listen-darwin.c has an effect? Does swaping it
> for "ctype.h" also solve that issue?
> 
> I was asking why the non-obvious portability hack was needed, and it
> seems Jeff suggested it might not be upthread in
> <aa6276f9-8d10-22f9-bfc0-2aa718d002e1@jeffhostetler.com>, but here you
> seem to have a reproduction of in being needed, without more of the
> relevant details (e.g. what OSX version(s))?
> 
>> 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.
> 
> Do you mean we don't need the ctype.h overrides in git-compat-util.h at
> all? I haven't looked into it, but needing to
> 
>>> 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.
> 
> My implicit observation upthread is that those sorts of details would
> ideally be included in a comment or the commit message. I.e. I didn't
> quite see why it was needed, and neither could the person submitting the
> patch series when asked.
> 
> Sure, it's a minor issue, but patch review is also meant to uncover such
> small issues.
>