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@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). 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@xxxxxxxxxxxxxxxxx>, 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.
