From: Jeff Hostetler Date: Thu, 17 Feb 2022 19:21:05 GMT Subject: Re: [PATCH v5 03/30] fsmonitor: config settings are repository-specific Message-ID: <456172c1-9648-c01b-c747-c6d77ae570bf@jeffhostetler.com> In-Reply-To: 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 >> >> 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