Re: [PATCH v5 03/30] fsmonitor: config settings are repository-specific

[Jeff Hostetler <git@jeffhostetler.com>]

On 2/17/22 11:27 AM, Johannes Schindelin wrote:
> Hi Jeff,
> 
> 
> On Fri, 11 Feb 2022, Jeff Hostetler via GitGitGadget wrote:
> 
>> From: Jeff Hostetler <jeffhost@microsoft.com>
>>
>> Move fsmonitor config settings to a new and opaque
>> `struct fsmonitor_settings` structure.  Add a lazily-loaded pointer
>> to this into `struct repo_settings`
>>
>> Create an `enum fsmonitor_mode` type in `struct fsmonitor_settings` to
>> represent the state of fsmonitor.  This lets us represent which, if
>> any, fsmonitor provider (hook or IPC) is enabled.
>>
>> Create `fsm_settings__get_*()` getters to lazily look up fsmonitor-
>> related config settings.
>>
>> Get rid of the `core_fsmonitor` global variable.  Move the code to
>> lookup the existing `core.fsmonitor` config value into the fsmonitor
>> settings.
>>
>> Create a hook pathname variable in `struct fsmonitor-settings` and
>> only set it when in hook mode.
>>
>> Extend the definition of `core.fsmonitor` to be either a boolean
>> or a hook pathname.  When true, the builtin FSMonitor is used.
>> When false or unset, no FSMonitor (neither builtin nor hook) is
>> used.
>>
>> The existing `core_fsmonitor` global variable was used to store the
>> pathname to the fsmonitor hook *and* it was used as a boolean to see
>> if fsmonitor was enabled.  This dual usage and global visibility leads
>> to confusion when we add the IPC-based provider.  So lets hide the
>> details in fsmonitor-settings.c and let it decide which provider to
>> use in the case of multiple settings.  This avoids cluttering up
>> repo-settings.c with these private details.
>>
>> A future commit in builtin-fsmonitor series will add the ability to
>> disqualify worktrees for various reasons, such as being mounted from a
>> remote volume, where fsmonitor should not be started.  Having the
>> config settings hidden in fsmonitor-settings.c allows such worktree
>> restrictions to override the config values used.
> 
> Apart from my forward-compatibility concern regarding interpreting
> `core.fsmonitor` as a Boolean, this looks good. Just one thing:
> 
>> diff --git a/fsmonitor.h b/fsmonitor.h
>> index f20d72631d7..f9201411aa7 100644
>> --- a/fsmonitor.h
>> +++ b/fsmonitor.h
>> @@ -3,6 +3,7 @@
>>
>>   #include "cache.h"
>>   #include "dir.h"
>> +#include "fsmonitor-settings.h"
>>
>>   extern struct trace_key trace_fsmonitor;
>>
>> @@ -57,7 +58,11 @@ int fsmonitor_is_trivial_response(const struct strbuf *query_result);
>>    */
>>   static inline int is_fsmonitor_refreshed(const struct index_state *istate)
>>   {
>> -	return !core_fsmonitor || istate->fsmonitor_has_run_once;
>> +	struct repository *r = istate->repo ? istate->repo : the_repository;
> 
> I see this repeated a few times. Would it maybe make sense to change the
> signature of the `fsm_settings__*()` functions to accept an index instead
> of a repository?

I think is just me being paranoid -- testing istate->repo for null
and assuming the_repository if necessary.  I'm wondering if it is
always safe to just do

	fsm_mode = fsm_settings__get_mode(istate->repo);

(or maybe put the null check inside the fsm_settings__*() functions.


changing the signature of those fsm_* functions seems wrong since they
are associated with a repo and not an index.

Jeff