Hi Jeff,
On Fri, 11 Feb 2022, Jeff Hostetler via GitGitGadget wrote:
> 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.
> +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...
> +/*
> + * 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.
> + *
> + * 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
