From: Johannes Schindelin Date: Thu, 24 Feb 2022 15:50:07 GMT Subject: Re: [PATCH v5 03/30] fsmonitor: config settings are repository-specific Message-ID: In-Reply-To: <456172c1-9648-c01b-c747-c6d77ae570bf@jeffhostetler.com> Hi Jeff, On Thu, 17 Feb 2022, Jeff Hostetler wrote: > On 2/17/22 11:27 AM, Johannes Schindelin wrote: > > > On Fri, 11 Feb 2022, Jeff Hostetler via GitGitGadget wrote: > > > > > 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. Indeed, this might be a bit too careful here. There is a pretty strong assumption built into the Git index that there is a repository, after all. > 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. I would like to try. The test suite should give us enough confidence that we have dashed all Ts and dotted all Is on the question whether `istate->repo` is always non-NULL. > changing the signature of those fsm_* functions seems wrong since they > are associated with a repo and not an index. True. Thank you for the sanity check, Dscho