| Message ID | cdef9730b3f93a6f0f98d68ffb81bcb89d6e698e.1646160212.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Commit | 33fa22204685e1b8d69d9dbd9b9b5e4d9078d511 |
| Headers | show |
| Series | Builtin FSMonitor Part 2 | expand |
On Tue, Mar 01 2022, Jeff Hostetler via GitGitGadget wrote: > From: Jeff Hostetler <jeffhost@microsoft.com> > [...] > +#define kFSEventStreamEventFlagNone 0x00000000 > +#define kFSEventStreamEventFlagMustScanSubDirs 0x00000001 > +#define kFSEventStreamEventFlagUserDropped 0x00000002 > +#define kFSEventStreamEventFlagKernelDropped 0x00000004 > +#define kFSEventStreamEventFlagEventIdsWrapped 0x00000008 > +#define kFSEventStreamEventFlagHistoryDone 0x00000010 > +#define kFSEventStreamEventFlagRootChanged 0x00000020 > +#define kFSEventStreamEventFlagMount 0x00000040 > +#define kFSEventStreamEventFlagUnmount 0x00000080 > +#define kFSEventStreamEventFlagItemCreated 0x00000100 > +#define kFSEventStreamEventFlagItemRemoved 0x00000200 > +#define kFSEventStreamEventFlagItemInodeMetaMod 0x00000400 > +#define kFSEventStreamEventFlagItemRenamed 0x00000800 > +#define kFSEventStreamEventFlagItemModified 0x00001000 > +#define kFSEventStreamEventFlagItemFinderInfoMod 0x00002000 > +#define kFSEventStreamEventFlagItemChangeOwner 0x00004000 > +#define kFSEventStreamEventFlagItemXattrMod 0x00008000 > +#define kFSEventStreamEventFlagItemIsFile 0x00010000 > +#define kFSEventStreamEventFlagItemIsDir 0x00020000 > +#define kFSEventStreamEventFlagItemIsSymlink 0x00040000 > +#define kFSEventStreamEventFlagOwnEvent 0x00080000 > +#define kFSEventStreamEventFlagItemIsHardlink 0x00100000 > +#define kFSEventStreamEventFlagItemIsLastHardlink 0x00200000 > +#define kFSEventStreamEventFlagItemCloned 0x00400000 Can we define these as 1<<0, 1<<1, 1<<2 etc.? We do that in most other places, and it helps to quickly eyeball these and see that they don't have gaps. > +#define kCFStringEncodingUTF8 0x08000100 Should this be an OR of some of the above, or is it unrelated? > +typedef struct FSEventStreamContext FSEventStreamContext; > +typedef unsigned int FSEventStreamEventFlags; > +#define kFSEventStreamCreateFlagNoDefer 0x02 > +#define kFSEventStreamCreateFlagWatchRoot 0x04 > +#define kFSEventStreamCreateFlagFileEvents 0x10 Ditto 1<<0 etc. > +#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"
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> >> [...] >> +#define kFSEventStreamEventFlagNone 0x00000000 [...] >> +#define kFSEventStreamEventFlagItemCloned 0x00400000 > > Can we define these as 1<<0, 1<<1, 1<<2 etc.? We do that in most other > places, and it helps to quickly eyeball these and see that they don't > have gaps. All of these constants are defined by Apple in their header files and system documentation. For example, see: https://developer.apple.com/documentation/coreservices/1455361-fseventstreameventflags/kfseventstreameventflagitemcloned The set is relatively fixed by Apple and we won't be adding any (since they define the bits in a FS event from the kernel). Changing them to shifts would be wrong. thanks Jeff
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. > >> + >> #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
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> >> [...] [...] Ævar sent feedback (thanks!) on 8 commits in the V6 version on March 7. I started responding to each as I got to them in my inbox yesterday, but I'd like to take a break from responding individually to each of them inside of Part 2. Since most of the feedback is for "general cleanup" and since Part 2 V6 is already in "next", I'd like to revisit these issues with a few "cleanup" commits on top of Part 3 (which is still in active review), rather than re-rolling or appending "fixup" commits onto Part 2. I think this would be quicker and easier in the long run and give us the same net result. Thanks, Jeff
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> >>> [...] >>> +#define kFSEventStreamEventFlagNone 0x00000000 > [...] >>> +#define kFSEventStreamEventFlagItemCloned 0x00400000 >> Can we define these as 1<<0, 1<<1, 1<<2 etc.? We do that in most >> other >> places, and it helps to quickly eyeball these and see that they don't >> have gaps. > > All of these constants are defined by Apple in their header > files and system documentation. For example, see: > https://developer.apple.com/documentation/coreservices/1455361-fseventstreameventflags/kfseventstreameventflagitemcloned > > The set is relatively fixed by Apple and we won't be adding any > (since they define the bits in a FS event from the kernel). > > Changing them to shifts would be wrong. Ah, I missed the "ifndef __clang__" at the start, so most of it is not needed except with gcc. FWIW I think having that whole part just split into compat/fsmonitor/darwin-gcc.h would make it obvious where all the GCC-specific hackery is.
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. >> >>> + >>> #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...
Jeff Hostetler <git@jeffhostetler.com> writes: > 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> >>> [...] > [...] > > Ævar sent feedback (thanks!) on 8 commits in the V6 version > on March 7. I started responding to each as I got to them > in my inbox yesterday, but I'd like to take a break from > responding individually to each of them inside of Part 2. > > Since most of the feedback is for "general cleanup" and since > Part 2 V6 is already in "next", I'd like to revisit these > issues with a few "cleanup" commits on top of Part 3 (which > is still in active review), rather than re-rolling or > appending "fixup" commits onto Part 2. Sounds good. Prepending "preliminary clean-up" before part 3 would be even cleaner, I would suspect. In any case, let's consider part 2 "more or less done" unless we see a glaring mistake that requires us to revert and redo it from scratch. Thanks.
On Wed, Mar 09 2022, Junio C Hamano wrote: > Jeff Hostetler <git@jeffhostetler.com> writes: > >> 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> >>>> [...] >> [...] >> >> Ævar sent feedback (thanks!) on 8 commits in the V6 version >> on March 7. I started responding to each as I got to them >> in my inbox yesterday, but I'd like to take a break from >> responding individually to each of them inside of Part 2. >> >> Since most of the feedback is for "general cleanup" and since >> Part 2 V6 is already in "next", I'd like to revisit these >> issues with a few "cleanup" commits on top of Part 3 (which >> is still in active review), rather than re-rolling or >> appending "fixup" commits onto Part 2. > > Sounds good. Prepending "preliminary clean-up" before part 3 would > be even cleaner, I would suspect. > > In any case, let's consider part 2 "more or less done" unless we see > a glaring mistake that requires us to revert and redo it from > scratch. Sounds good, my comments on v6 today were before I'd noticed that it was in "next", I think all of those can (well, it would be that way either way at this point...) be left for some potential follow-up. Thanks both, especially Jeff for sticking with this fsmonitor topic for so long & keeping it advancing.
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). 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
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.
On 3/10/22 9:42 AM, Ævar Arnfjörð Bjarmason wrote: > > 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. > There are two independent issues here. (1) compiling something that includes <CoreServices.h> with GCC. (2) the need for the #include <string.h> when compiling with clang. To address (1), we've #ifdef'd the top of the file and insert just the essential typedefs and prototypes. (I'll pull them into a separate local header file as you suggested earlier to make that easier to see.) But otherwise, GCC is not an issue. WRT (2) I have tried clang-11 on macOS 10.15 and clang-13 on 11.6 both with and without the <string.h> and it doesn't matter. Everything compiles and t7527 passes in all [2x2] cases. So I have to assume that something has changed in the Apple/clang SDK or runtime libraries or our source code in that single source file in the ~2 years since Kevin needed to add it. I do not have access to an older Mac (Apple makes it hard to test back-compat with older OS's), so I cannot reproduce the error. But I don't doubt that there was an error at one point -- I just don't know what it was. So that's my context for saying that I don't think it is needed now, but I was willing to carry it forward in case it is still helpful for people with older Macs. FWIW, it really seems pretty isolated and trivial and would only affect code in this single source file -- which from a quick scan, doesn't actually reference any of the functions in <string.h>, so there shouldn't be any need to think about git-compat-util.h or ctype.h, right? Jeff
On 3/9/22 1:57 PM, Junio C Hamano wrote: > Jeff Hostetler <git@jeffhostetler.com> writes: > >> 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> >>>> [...] >> [...] >> >> Ævar sent feedback (thanks!) on 8 commits in the V6 version >> on March 7. I started responding to each as I got to them >> in my inbox yesterday, but I'd like to take a break from >> responding individually to each of them inside of Part 2. >> >> Since most of the feedback is for "general cleanup" and since >> Part 2 V6 is already in "next", I'd like to revisit these >> issues with a few "cleanup" commits on top of Part 3 (which >> is still in active review), rather than re-rolling or >> appending "fixup" commits onto Part 2. > > Sounds good. Prepending "preliminary clean-up" before part 3 would > be even cleaner, I would suspect. > > In any case, let's consider part 2 "more or less done" unless we see > a glaring mistake that requires us to revert and redo it from > scratch. > > Thanks. > The cleanup here turned into 16 small commits. I'll send them as a "Part 2.5" so that they stand alone and can either be appended to part 2 or treated as a new branch. GGG wouldn't let me send fixup! commits, so inside of each commit message is a "fixup! ..." line which you can use if you want to squash them into part 2. But otherwise they have a normal (non-fixup) commit summary. After I send that I'll send a new version of part 3 that builds upon them. Thanks Jeff
diff --git a/compat/fsmonitor/fsm-listen-darwin.c b/compat/fsmonitor/fsm-listen-darwin.c index c84e3344ab9..f76341317dd 100644 --- a/compat/fsmonitor/fsm-listen-darwin.c +++ b/compat/fsmonitor/fsm-listen-darwin.c @@ -1,3 +1,99 @@ +#ifndef __clang__ +/* + * It is possible to #include CoreFoundation/CoreFoundation.h when compiling + * with clang, but not with GCC as of time of writing. + * + * See https://gcc.gnu.org/bugzilla/show_bug.cgi?id=93082 for details. + */ +typedef unsigned int FSEventStreamCreateFlags; +#define kFSEventStreamEventFlagNone 0x00000000 +#define kFSEventStreamEventFlagMustScanSubDirs 0x00000001 +#define kFSEventStreamEventFlagUserDropped 0x00000002 +#define kFSEventStreamEventFlagKernelDropped 0x00000004 +#define kFSEventStreamEventFlagEventIdsWrapped 0x00000008 +#define kFSEventStreamEventFlagHistoryDone 0x00000010 +#define kFSEventStreamEventFlagRootChanged 0x00000020 +#define kFSEventStreamEventFlagMount 0x00000040 +#define kFSEventStreamEventFlagUnmount 0x00000080 +#define kFSEventStreamEventFlagItemCreated 0x00000100 +#define kFSEventStreamEventFlagItemRemoved 0x00000200 +#define kFSEventStreamEventFlagItemInodeMetaMod 0x00000400 +#define kFSEventStreamEventFlagItemRenamed 0x00000800 +#define kFSEventStreamEventFlagItemModified 0x00001000 +#define kFSEventStreamEventFlagItemFinderInfoMod 0x00002000 +#define kFSEventStreamEventFlagItemChangeOwner 0x00004000 +#define kFSEventStreamEventFlagItemXattrMod 0x00008000 +#define kFSEventStreamEventFlagItemIsFile 0x00010000 +#define kFSEventStreamEventFlagItemIsDir 0x00020000 +#define kFSEventStreamEventFlagItemIsSymlink 0x00040000 +#define kFSEventStreamEventFlagOwnEvent 0x00080000 +#define kFSEventStreamEventFlagItemIsHardlink 0x00100000 +#define kFSEventStreamEventFlagItemIsLastHardlink 0x00200000 +#define kFSEventStreamEventFlagItemCloned 0x00400000 + +typedef struct __FSEventStream *FSEventStreamRef; +typedef const FSEventStreamRef ConstFSEventStreamRef; + +typedef unsigned int CFStringEncoding; +#define kCFStringEncodingUTF8 0x08000100 + +typedef const struct __CFString *CFStringRef; +typedef const struct __CFArray *CFArrayRef; +typedef const struct __CFRunLoop *CFRunLoopRef; + +struct FSEventStreamContext { + long long version; + void *cb_data, *retain, *release, *copy_description; +}; + +typedef struct FSEventStreamContext FSEventStreamContext; +typedef unsigned int FSEventStreamEventFlags; +#define kFSEventStreamCreateFlagNoDefer 0x02 +#define kFSEventStreamCreateFlagWatchRoot 0x04 +#define kFSEventStreamCreateFlagFileEvents 0x10 + +typedef unsigned long long FSEventStreamEventId; +#define kFSEventStreamEventIdSinceNow 0xFFFFFFFFFFFFFFFFULL + +typedef void (*FSEventStreamCallback)(ConstFSEventStreamRef streamRef, + void *context, + __SIZE_TYPE__ num_of_events, + void *event_paths, + const FSEventStreamEventFlags event_flags[], + const FSEventStreamEventId event_ids[]); +typedef double CFTimeInterval; +FSEventStreamRef FSEventStreamCreate(void *allocator, + FSEventStreamCallback callback, + FSEventStreamContext *context, + CFArrayRef paths_to_watch, + FSEventStreamEventId since_when, + CFTimeInterval latency, + FSEventStreamCreateFlags flags); +CFStringRef CFStringCreateWithCString(void *allocator, const char *string, + CFStringEncoding encoding); +CFArrayRef CFArrayCreate(void *allocator, const void **items, long long count, + void *callbacks); +void CFRunLoopRun(void); +void CFRunLoopStop(CFRunLoopRef run_loop); +CFRunLoopRef CFRunLoopGetCurrent(void); +extern CFStringRef kCFRunLoopDefaultMode; +void FSEventStreamScheduleWithRunLoop(FSEventStreamRef stream, + CFRunLoopRef run_loop, + CFStringRef run_loop_mode); +unsigned char FSEventStreamStart(FSEventStreamRef stream); +void FSEventStreamStop(FSEventStreamRef stream); +void FSEventStreamInvalidate(FSEventStreamRef stream); +void FSEventStreamRelease(FSEventStreamRef stream); +#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 + #include "cache.h" #include "fsmonitor.h" #include "fsm-listen.h"