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@xxxxxxxxxxxxx> > >>> [...] > > [...] > >> > >>> +#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). 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
