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

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Feb 24, 2022, 15:50 UTC
Message-ID: <nycvar.QRO.7.76.6.2202241647440.11118@tvgsbejvaqbjf.bet>
In-Reply-To: <456172c1-9648-c01b-c747-c6d77ae570bf@jeffhostetler.com>

Hi Jeff,

On Thu, 17 Feb 2022, Jeff Hostetler wrote:

Show 31 quoted lines

> 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.

Show 5 quoted lines

> 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