Re: [PATCH v5 17/30] compat/fsmonitor/fsm-listen-darwin: implement FSEvent listener on MacOS
- From
Johannes Schindelin <johannes.schindelin@gmx.de>
- Date
- Feb 24, 2022, 15:21 UTC
- Message-ID
- <nycvar.QRO.7.76.6.2202241614040.11118@tvgsbejvaqbjf.bet>
- In-Reply-To
- <162e357db7211e72c8a39e722e8432baf710c70d.1644612979.git.gitgitgadget@gmail.com>
Hi Jeff,
On Fri, 11 Feb 2022, Jeff Hostetler via GitGitGadget wrote:
Show 7 quoted lines
> diff --git a/compat/fsmonitor/fsm-listen-darwin.c b/compat/fsmonitor/fsm-listen-darwin.c > index f424253d3eb..2aefdc14d89 100644 > --- a/compat/fsmonitor/fsm-listen-darwin.c > +++ b/compat/fsmonitor/fsm-listen-darwin.c > @@ -1,4 +1,4 @@ > -#if defined(__GNUC__) > +#ifndef __clang__
This probably wants to be squashed into the previous patch, and while at it, the corresponding commit message should probably be adjusted:
We need GCC and clang versions because of compiler and header file conflicts.
This should probably say something along the lines "This is only needed for GCC, clang is fine" instead.
Show 50 quoted lines
> +static void log_flags_set(const char *path, const FSEventStreamEventFlags flag)
> +{
> + struct strbuf msg = STRBUF_INIT;
> +
> + if (flag & kFSEventStreamEventFlagMustScanSubDirs)
> + strbuf_addstr(&msg, "MustScanSubDirs|");
> + if (flag & kFSEventStreamEventFlagUserDropped)
> + strbuf_addstr(&msg, "UserDropped|");
> + if (flag & kFSEventStreamEventFlagKernelDropped)
> + strbuf_addstr(&msg, "KernelDropped|");
> + if (flag & kFSEventStreamEventFlagEventIdsWrapped)
> + strbuf_addstr(&msg, "EventIdsWrapped|");
> + if (flag & kFSEventStreamEventFlagHistoryDone)
> + strbuf_addstr(&msg, "HistoryDone|");
> + if (flag & kFSEventStreamEventFlagRootChanged)
> + strbuf_addstr(&msg, "RootChanged|");
> + if (flag & kFSEventStreamEventFlagMount)
> + strbuf_addstr(&msg, "Mount|");
> + if (flag & kFSEventStreamEventFlagUnmount)
> + strbuf_addstr(&msg, "Unmount|");
> + if (flag & kFSEventStreamEventFlagItemChangeOwner)
> + strbuf_addstr(&msg, "ItemChangeOwner|");
> + if (flag & kFSEventStreamEventFlagItemCreated)
> + strbuf_addstr(&msg, "ItemCreated|");
> + if (flag & kFSEventStreamEventFlagItemFinderInfoMod)
> + strbuf_addstr(&msg, "ItemFinderInfoMod|");
> + if (flag & kFSEventStreamEventFlagItemInodeMetaMod)
> + strbuf_addstr(&msg, "ItemInodeMetaMod|");
> + if (flag & kFSEventStreamEventFlagItemIsDir)
> + strbuf_addstr(&msg, "ItemIsDir|");
> + if (flag & kFSEventStreamEventFlagItemIsFile)
> + strbuf_addstr(&msg, "ItemIsFile|");
> + if (flag & kFSEventStreamEventFlagItemIsHardlink)
> + strbuf_addstr(&msg, "ItemIsHardlink|");
> + if (flag & kFSEventStreamEventFlagItemIsLastHardlink)
> + strbuf_addstr(&msg, "ItemIsLastHardlink|");
> + if (flag & kFSEventStreamEventFlagItemIsSymlink)
> + strbuf_addstr(&msg, "ItemIsSymlink|");
> + if (flag & kFSEventStreamEventFlagItemModified)
> + strbuf_addstr(&msg, "ItemModified|");
> + if (flag & kFSEventStreamEventFlagItemRemoved)
> + strbuf_addstr(&msg, "ItemRemoved|");
> + if (flag & kFSEventStreamEventFlagItemRenamed)
> + strbuf_addstr(&msg, "ItemRenamed|");
> + if (flag & kFSEventStreamEventFlagItemXattrMod)
> + strbuf_addstr(&msg, "ItemXattrMod|");
> + if (flag & kFSEventStreamEventFlagOwnEvent)
> + strbuf_addstr(&msg, "OwnEvent|");
> + if (flag & kFSEventStreamEventFlagItemCloned)
> + strbuf_addstr(&msg, "ItemCloned|");I cannot think of any more elegant way to do this, either, but I wish there was a way...
Show 5 quoted lines
> +/* > + * NEEDSWORK: Investigate the proper value for the `latency` argument > + * in the call to `FSEventStreamCreate()`. I'm not sure that this > + * needs to be a config setting or just something that we tune after > + * some testing.
Since this was written, we had ample time to investigate the proper value, and I think you actually did. Therefore this comment is probably no longer needed.
Show 8 quoted lines
> + * > + * With a latency of 0.1, I was seeing lots of dropped events during > + * the "touch 100000" files test within t/perf/p7519, but with a > + * latency of 0.001 I did not see any dropped events. So the > + * "correct" value may be somewhere in between. > + * > + * https://developer.apple.com/documentation/coreservices/1443980-fseventstreamcreate > + */
The rest of the patch looks good to me, and since we saw this code working nicely in practice, I have high confidence in it.
Ciao, Dscho