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

Hi Jeff,

On Thu, 17 Feb 2022, Jeff Hostetler wrote:

On 2/17/22 11:27 AM, Johannes Schindelin wrote:

quoted

On Fri, 11 Feb 2022, Jeff Hostetler via GitGitGadget wrote:

quoted

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