Re: [PATCH] fsmonitor: eliminate call to deprecated FSEventStream function

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Fri, Dec 02 2022, Jeff Hostetler wrote:

> On 12/2/22 1:02 PM, Ævar Arnfjörð Bjarmason wrote:
>> On Fri, Dec 02 2022, Jeff Hostetler via GitGitGadget wrote:
>> 
>>> From: Jeff Hostetler <jeffhostetler@github.com>
>>>
>>> Replace the call to `FSEventStreamScheduleWithRunLoop()` function with
>>> the suggested `FSEventStreamSetDispatchQueue()` function.
>>>
>>> The MacOS version of the builtin FSMonitor feature uses the
>>> `FSEventStreamScheduleWithRunLoop()` function to drive the event loop
>>> and process FSEvents from the system.  This routine has now been
>>> deprecated by Apple.  The MacOS 13 (Ventana) compiler tool chain now
>>> generates a warning when compiling calls to this function.  In
>>> DEVELOPER=1 mode, this now causes a compile error.
>>>
>>> The `FSEventStreamSetDispatchQueue()` function is conceptually similar
>>> and is the suggested replacement.  However, there are some subtle
>>> thread-related differences.
>>>
>>> Previously, the event stream would be processed by the
>>> `fsm_listen__loop()` thread while it was in the `CFRunLoopRun()`
>>> method.  (Conceptually, this was a blocking call on the lifetime of
>>> the event stream where our thread drove the event loop and individual
>>> events were handled by the `fsevent_callback()`.)
>>>
>>> With the change, a "dispatch queue" is created and FSEvents will be
>>> processed by a hidden queue-related thread (that calls the
>>> `fsevent_callback()` on our behalf).  Our `fsm_listen__loop()` thread
>>> maintains the original blocking model by waiting on a mutex/condition
>>> variable pair while the hidden thread does all of the work.
>> I just skimmed the code change and didn't see anything out of place,
>> but
>> one thing that's missing about this explanation is:
>> Ok, it's deprecated, but when was it introduced? I.e. we now
>> presumably
>> have a hard dependency on a newer API released with a newer version of
>> OSX?
>> Is it OK that we're going to throw compilation errors on older
>> versions
>> that don't have it? What version is that? Is that older or newer than
>> our oldest supported OSX version in general, or is the plan to support
>> older OSX, but those users would need to compile without fsmonitor?
>> Depending on the answers to the above (hopefully in a re-rolled
>> commit
>> message): Should we patch the bit in config.mak.uname where we do the
>> OSX version detection? I.e. if we're deprecating an older version anyone
>> still on it would be much better off with a straight-up "$(error)" from
>> the Makefile, rather than running into a compilation error, only to find
>> that we've stopped supporting that older version.
>
> Lots of questions here.  Let me take a quick stab at answering them.
> From [1] the old routine was introduced in 10.5 and marked deprecated
> in 10.13.  From [2] the new routine was introduced in 10.6.
>
> 10.5 (Leopard) was released October 2007.
> 10.6 (Snow Leopard) was released August 2009.
>
> So the only people that would be affected by this must be running
> exactly 10.5, right?  (Those with 10.4 and before don't have either
> API and are already broken regardless.)
>
> So, based on the ages of those two Apple releases, I'd like to think
> that we're fine just switching over and not having to ifdef-up the
> config.mak.uname.  (If it were a more recent change in the OS, then
> yeah the answer would be different.)
>
> Thoughts ???

That seems reasonable to me, but it came out in 2001, and we'd be moving
the dependency to a 2007 version.

Is that OK? No idea, I don't know how old of an OSX version people
reasonably run & want to compile Git on.

But in 842c9edec64 (fsmonitor: enable fsmonitor for Linux, 2022-11-23)
which is new in this upcoming release we seem to have set that
dependency at 10.4.

Now, you can unset FSMONITOR_DAEMON_BACKEND and FSMONITOR_OS_SETTINGS in
your config.mak.uname, but that's probably something that should be
noted more prominently.

Eric? [CC'd]