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