diff mbox series

[v4,03/29] fsmonitor: config settings are repository-specific

Message ID 882789b4dfebddb059f62b0b2edb95b92f3c69ee.1634826309.git.gitgitgadget@gmail.com (mailing list archive)
State New, archived
Headers show
Series Builtin FSMonitor Part 2 | expand

Commit Message

Jeff Hostetler Oct. 21, 2021, 2:24 p.m. UTC 
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.

Add support for the new `core.useBuiltinFSMonitor` config setting.

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.

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.

Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
---
 Makefile               |  1 +
 builtin/update-index.c | 19 +++++++--
 cache.h                |  1 -
 config.c               | 14 ------
 config.h               |  1 -
 environment.c          |  1 -
 fsmonitor-settings.c   | 97 ++++++++++++++++++++++++++++++++++++++++++
 fsmonitor-settings.h   | 21 +++++++++
 fsmonitor.c            | 63 ++++++++++++++++-----------
 fsmonitor.h            | 18 ++++++--
 repository.h           |  3 ++
 t/README               |  4 +-
 12 files changed, 192 insertions(+), 51 deletions(-)
 create mode 100644 fsmonitor-settings.c
 create mode 100644 fsmonitor-settings.h

Comments

Junio C Hamano Oct. 21, 2021, 9:05 p.m. UTC | #1 
"Jeff Hostetler via GitGitGadget" <gitgitgadget@gmail.com> writes:

>  	if (fsmonitor > 0) {
> -		if (git_config_get_fsmonitor() == 0)
> +		enum fsmonitor_mode fsm_mode = fsm_settings__get_mode(r);
> +
> +		if (fsm_mode == FSMONITOR_MODE_DISABLED) {
> +			warning(_("core.useBuiltinFSMonitor is unset; "
> +				"set it if you really want to enable the "
> +				"builtin fsmonitor"));
>  			warning(_("core.fsmonitor is unset; "
> -				"set it if you really want to "
> -				"enable fsmonitor"));
> +				"set it if you really want to enable the "
> +				"hook-based fsmonitor"));
> +		}
>  		add_fsmonitor(&the_index);
>  		report(_("fsmonitor enabled"));
>  	} else if (!fsmonitor) {
> -		if (git_config_get_fsmonitor() == 1)
> +		enum fsmonitor_mode fsm_mode = fsm_settings__get_mode(r);
> +		if (fsm_mode == FSMONITOR_MODE_IPC)
> +			warning(_("core.useBuiltinFSMonitor is set; "
> +				"remove it if you really want to "
> +				"disable fsmonitor"));
> +		if (fsm_mode == FSMONITOR_MODE_HOOK)
>  			warning(_("core.fsmonitor is set; "
>  				"remove it if you really want to "
>  				"disable fsmonitor"));

Hmph.  This does not change the behaviour per-se, but what are we
trying to achieve with these warning messages?  

The user uses --fsmonitor or --no-fsmonitor option from the command
line, presumably as a one-shot "this time I'd operate the command
differently from the configured way", so it seems unlikely that the
user is doing so because "... really want to enable/disable".  The
"report()" calls in these if/else cases seem sufficient reminder of
what is going on---perhaps these warnings should be made silenceable
by turning them into advice messages?

> -int git_config_get_fsmonitor(void)
> -{
> -	if (git_config_get_pathname("core.fsmonitor", &core_fsmonitor))
> -		core_fsmonitor = getenv("GIT_TEST_FSMONITOR");
> -
> -	if (core_fsmonitor && !*core_fsmonitor)
> -		core_fsmonitor = NULL;
> -
> -	if (core_fsmonitor)
> -		return 1;
> -
> -	return 0;
> -}

This used to be how we got the configuration.

> --- a/config.h
> +++ b/config.h
> @@ -610,7 +610,6 @@ int git_config_get_pathname(const char *key, const char **dest);
>  int git_config_get_index_threads(int *dest);
>  int git_config_get_split_index(void);
>  int git_config_get_max_percent_split_change(void);
> -int git_config_get_fsmonitor(void);

And that is removed so any in-flight topic that adds new caller will
be caught by the compiler.  OK.

> diff --git a/environment.c b/environment.c
> index 9da7f3c1a19..68f90632245 100644
> --- a/environment.c
> +++ b/environment.c
> @@ -82,7 +82,6 @@ int protect_hfs = PROTECT_HFS_DEFAULT;
>  #define PROTECT_NTFS_DEFAULT 1
>  #endif
>  int protect_ntfs = PROTECT_NTFS_DEFAULT;
> -const char *core_fsmonitor;

So is this.

All nice.

> +static void lookup_fsmonitor_settings(struct repository *r)
> +{
> +	struct fsmonitor_settings *s;
> +
> +	CALLOC_ARRAY(s, 1);
> +
> +	r->settings.fsmonitor = s;
> +
> +	if (check_for_ipc(r))
> +		return;
> +
> +	if (check_for_hook(r))
> +		return;
> +
> +	fsm_settings__set_disabled(r);
> +}
> +
> +enum fsmonitor_mode fsm_settings__get_mode(struct repository *r)
> +{
> +	if (!r->settings.fsmonitor)
> +		lookup_fsmonitor_settings(r);

OK, and these "lookup" calls are what make this field "lazily
loaded".  A helper

static inline void lazily_load_fsmonitor_settings(struct repository *r)
{
	if (!r->settings.fsmonitor)
		lookup_fsmonitor_settings(r);
}

might be handy.  Also an assert to ensure nobody calls lookup() on a
repository that already has lazily loaded the settings would be
necessary.

	static void lookup_fsmonitor_settings(struct repository *r)
	{
		if (r->settings.fsmonitor)
			BUG("...");
		CALLOC_ARRAY(r->settings.fsmonitor, 1);

> +enum fsmonitor_mode {
> +	FSMONITOR_MODE_DISABLED = 0,
> +	FSMONITOR_MODE_HOOK = 1, /* core.fsmonitor */
> +	FSMONITOR_MODE_IPC = 2,  /* core.useBuiltinFSMonitor */
> +};

Please remind me why we need a new separate variable, instead of
turning the core.fsmonitor variable into an extended bool <false,
true, builtin>?  The compatibility issues during transition is the
same either way.  Old clients will ignore the request silently when
you set core.useBuiltinFSMonitor, or they will barf if you set
core.fsmonitor to 'builtin', so in a sense, extending the existing
variable may be a safer option.

> diff --git a/repository.h b/repository.h
> index a057653981c..89a1873ade7 100644
> --- a/repository.h
> +++ b/repository.h
> @@ -4,6 +4,7 @@
>  #include "path.h"
>  
>  struct config_set;
> +struct fsmonitor_settings;
>  struct git_hash_algo;
>  struct index_state;
>  struct lock_file;
> @@ -34,6 +35,8 @@ struct repo_settings {
>  	int command_requires_full_index;
>  	int sparse_index;
>  
> +	struct fsmonitor_settings *fsmonitor; /* lazy loaded */

"lazily" loaded, I think.

>  GIT_TEST_FSMONITOR=$PWD/t7519/fsmonitor-all exercises the fsmonitor
> -code path for utilizing a file system monitor to speed up detecting
> -new or changed files.
> +code path for utilizing a (hook based) file system monitor to speed up
> +detecting new or changed files.

Nice attention to the detail here.

Thanks.
Junio C Hamano Oct. 21, 2021, 9:16 p.m. UTC | #2 
Junio C Hamano <gitster@pobox.com> writes:

>> +enum fsmonitor_mode {
>> +	FSMONITOR_MODE_DISABLED = 0,
>> +	FSMONITOR_MODE_HOOK = 1, /* core.fsmonitor */
>> +	FSMONITOR_MODE_IPC = 2,  /* core.useBuiltinFSMonitor */
>> +};
>
> Please remind me why we need a new separate variable, instead of
> turning the core.fsmonitor variable into an extended bool <false,
> true, builtin>?

Ah, I see.

The vocabulary of the value for the existing variable is between
"unset means disabled" and "the path-to-hook means enabled", so
unless we forbid a bareword path "builtin" (which I do not think is
such a bad idea, by the way), it becomes a bit fuzzy what a
non-empty token means.

In any case, the "set to path to enable, leave unset to leave
disabled" is a cumbersome to use and may want to be rethought.  It
is unclear how one would override a configured path-to-hook, for
example.

Considering that we need to reserve a special word, say, "disabled",
that has to be distinguishable from a normal "here is a path to the
hook script" ANYWAY, in order to allow such a "last one wins"
configuration override (or "git -c core.fsmonitor=disabled cmd"), it
starts to sound more and more reasonable to reserve yet another word
"builtin" as a special value of core.fsmonitor, without having to
introduce a new configuration variable, no?
Jeff Hostetler Oct. 27, 2021, 7:03 p.m. UTC | #3 
On 10/21/21 5:05 PM, Junio C Hamano wrote:
> "Jeff Hostetler via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>>   	if (fsmonitor > 0) {
>> -		if (git_config_get_fsmonitor() == 0)
>> +		enum fsmonitor_mode fsm_mode = fsm_settings__get_mode(r);
>> +
>> +		if (fsm_mode == FSMONITOR_MODE_DISABLED) {
>> +			warning(_("core.useBuiltinFSMonitor is unset; "
>> +				"set it if you really want to enable the "
>> +				"builtin fsmonitor"));
>>   			warning(_("core.fsmonitor is unset; "
>> -				"set it if you really want to "
>> -				"enable fsmonitor"));
>> +				"set it if you really want to enable the "
>> +				"hook-based fsmonitor"));
>> +		}
>>   		add_fsmonitor(&the_index);
>>   		report(_("fsmonitor enabled"));
>>   	} else if (!fsmonitor) {
>> -		if (git_config_get_fsmonitor() == 1)
>> +		enum fsmonitor_mode fsm_mode = fsm_settings__get_mode(r);
>> +		if (fsm_mode == FSMONITOR_MODE_IPC)
>> +			warning(_("core.useBuiltinFSMonitor is set; "
>> +				"remove it if you really want to "
>> +				"disable fsmonitor"));
>> +		if (fsm_mode == FSMONITOR_MODE_HOOK)
>>   			warning(_("core.fsmonitor is set; "
>>   				"remove it if you really want to "
>>   				"disable fsmonitor"));
> 
> Hmph.  This does not change the behaviour per-se, but what are we
> trying to achieve with these warning messages?
> 
> The user uses --fsmonitor or --no-fsmonitor option from the command
> line, presumably as a one-shot "this time I'd operate the command
> differently from the configured way", so it seems unlikely that the
> user is doing so because "... really want to enable/disable".  The
> "report()" calls in these if/else cases seem sufficient reminder of
> what is going on---perhaps these warnings should be made silenceable
> by turning them into advice messages?
>

The original code had the basic warning for `core.fsmonitor`
which becomes ambiguous/confusing when we have two different
config values.

I was really wanting to just get rid of the warnings.  They
only appear if the users passes a `--[no-]fsmonitor` on the
command line to temporarily override the config setting.

I'll see about making them advice messages.


>> -int git_config_get_fsmonitor(void)
>> -{
>> -	if (git_config_get_pathname("core.fsmonitor", &core_fsmonitor))
>> -		core_fsmonitor = getenv("GIT_TEST_FSMONITOR");
>> -
>> -	if (core_fsmonitor && !*core_fsmonitor)
>> -		core_fsmonitor = NULL;
>> -
>> -	if (core_fsmonitor)
>> -		return 1;
>> -
>> -	return 0;
>> -}
> 
> This used to be how we got the configuration.
> 
>> --- a/config.h
>> +++ b/config.h
>> @@ -610,7 +610,6 @@ int git_config_get_pathname(const char *key, const char **dest);
>>   int git_config_get_index_threads(int *dest);
>>   int git_config_get_split_index(void);
>>   int git_config_get_max_percent_split_change(void);
>> -int git_config_get_fsmonitor(void);
> 
> And that is removed so any in-flight topic that adds new caller will
> be caught by the compiler.  OK.
> 
>> diff --git a/environment.c b/environment.c
>> index 9da7f3c1a19..68f90632245 100644
>> --- a/environment.c
>> +++ b/environment.c
>> @@ -82,7 +82,6 @@ int protect_hfs = PROTECT_HFS_DEFAULT;
>>   #define PROTECT_NTFS_DEFAULT 1
>>   #endif
>>   int protect_ntfs = PROTECT_NTFS_DEFAULT;
>> -const char *core_fsmonitor;
> 
> So is this.
> 
> All nice.
> 
>> +static void lookup_fsmonitor_settings(struct repository *r)
>> +{
>> +	struct fsmonitor_settings *s;
>> +
>> +	CALLOC_ARRAY(s, 1);
>> +
>> +	r->settings.fsmonitor = s;
>> +
>> +	if (check_for_ipc(r))
>> +		return;
>> +
>> +	if (check_for_hook(r))
>> +		return;
>> +
>> +	fsm_settings__set_disabled(r);
>> +}
>> +
>> +enum fsmonitor_mode fsm_settings__get_mode(struct repository *r)
>> +{
>> +	if (!r->settings.fsmonitor)
>> +		lookup_fsmonitor_settings(r);
> 
> OK, and these "lookup" calls are what make this field "lazily
> loaded".  A helper
> 
> static inline void lazily_load_fsmonitor_settings(struct repository *r)
> {
> 	if (!r->settings.fsmonitor)
> 		lookup_fsmonitor_settings(r);
> }
> 
> might be handy.  Also an assert to ensure nobody calls lookup() on a
> repository that already has lazily loaded the settings would be
> necessary.
> 
> 	static void lookup_fsmonitor_settings(struct repository *r)
> 	{
> 		if (r->settings.fsmonitor)
> 			BUG("...");
> 		CALLOC_ARRAY(r->settings.fsmonitor, 1);

good point.


> 
>> +enum fsmonitor_mode {
>> +	FSMONITOR_MODE_DISABLED = 0,
>> +	FSMONITOR_MODE_HOOK = 1, /* core.fsmonitor */
>> +	FSMONITOR_MODE_IPC = 2,  /* core.useBuiltinFSMonitor */
>> +};
> 
> Please remind me why we need a new separate variable, instead of
> turning the core.fsmonitor variable into an extended bool <false,
> true, builtin>?  The compatibility issues during transition is the
> same either way.  Old clients will ignore the request silently when
> you set core.useBuiltinFSMonitor, or they will barf if you set
> core.fsmonitor to 'builtin', so in a sense, extending the existing
> variable may be a safer option.
> 
>> diff --git a/repository.h b/repository.h
>> index a057653981c..89a1873ade7 100644
>> --- a/repository.h
>> +++ b/repository.h
>> @@ -4,6 +4,7 @@
>>   #include "path.h"
>>   
>>   struct config_set;
>> +struct fsmonitor_settings;
>>   struct git_hash_algo;
>>   struct index_state;
>>   struct lock_file;
>> @@ -34,6 +35,8 @@ struct repo_settings {
>>   	int command_requires_full_index;
>>   	int sparse_index;
>>   
>> +	struct fsmonitor_settings *fsmonitor; /* lazy loaded */
> 
> "lazily" loaded, I think.
> 
>>   GIT_TEST_FSMONITOR=$PWD/t7519/fsmonitor-all exercises the fsmonitor
>> -code path for utilizing a file system monitor to speed up detecting
>> -new or changed files.
>> +code path for utilizing a (hook based) file system monitor to speed up
>> +detecting new or changed files.
> 
> Nice attention to the detail here.
> 
> Thanks.
>
Jeff Hostetler Oct. 27, 2021, 7:53 p.m. UTC | #4 
On 10/21/21 5:16 PM, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
>>> +enum fsmonitor_mode {
>>> +	FSMONITOR_MODE_DISABLED = 0,
>>> +	FSMONITOR_MODE_HOOK = 1, /* core.fsmonitor */
>>> +	FSMONITOR_MODE_IPC = 2,  /* core.useBuiltinFSMonitor */
>>> +};
>>
>> Please remind me why we need a new separate variable, instead of
>> turning the core.fsmonitor variable into an extended bool <false,
>> true, builtin>?
> 
> Ah, I see.
> 
> The vocabulary of the value for the existing variable is between
> "unset means disabled" and "the path-to-hook means enabled", so
> unless we forbid a bareword path "builtin" (which I do not think is
> such a bad idea, by the way), it becomes a bit fuzzy what a
> non-empty token means.
> 
> In any case, the "set to path to enable, leave unset to leave
> disabled" is a cumbersome to use and may want to be rethought.  It
> is unclear how one would override a configured path-to-hook, for
> example.
> 
> Considering that we need to reserve a special word, say, "disabled",
> that has to be distinguishable from a normal "here is a path to the
> hook script" ANYWAY, in order to allow such a "last one wins"
> configuration override (or "git -c core.fsmonitor=disabled cmd"), it
> starts to sound more and more reasonable to reserve yet another word
> "builtin" as a special value of core.fsmonitor, without having to
> introduce a new configuration variable, no?
> 

For a while we were using a ":builtin:" reserved value (which isn't
a valid pathname on Windows) but thought it better to split it into
two different config values to avoid the confusion (since it is a
valid path on Mac/Linux).  But having 2 config vars is also confusing.

And yes, I'm not sure there is a way for a local fsmonitor hook
config to override a global hook value unless we add a "disabled"
or "off" value.

Let me revisit this.  We could have:

     [] unset, <any of the standard boolean false values>, "disabled"
     [] <hook-path>
     [] "builtin", "ipc"

and just make the literals special reserved values.


I've not kept up on the configurable hooks series, but I have to
wonder if this usage (pathname or reserved word) will cause problems
with the changes being planned for hooks.  The existing fsmonitor
usage doesn't use find_hook() nor run_hook*(), so I don't expect
an immediate conflict.  But again, I haven't kept up with that
effort.

Jeff
diff mbox series

Patch

diff --git a/Makefile b/Makefile
index d51fd8b33ce..29ed7c4aba6 100644
--- a/Makefile
+++ b/Makefile
@@ -898,6 +898,7 @@  LIB_OBJS += fmt-merge-msg.o
 LIB_OBJS += fsck.o
 LIB_OBJS += fsmonitor.o
 LIB_OBJS += fsmonitor-ipc.o
+LIB_OBJS += fsmonitor-settings.o
 LIB_OBJS += gettext.o
 LIB_OBJS += gpg-interface.o
 LIB_OBJS += graph.o
diff --git a/builtin/update-index.c b/builtin/update-index.c
index 187203e8bb5..79db3ff37e2 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -1214,14 +1214,25 @@  int cmd_update_index(int argc, const char **argv, const char *prefix)
 	}
 
 	if (fsmonitor > 0) {
-		if (git_config_get_fsmonitor() == 0)
+		enum fsmonitor_mode fsm_mode = fsm_settings__get_mode(r);
+
+		if (fsm_mode == FSMONITOR_MODE_DISABLED) {
+			warning(_("core.useBuiltinFSMonitor is unset; "
+				"set it if you really want to enable the "
+				"builtin fsmonitor"));
 			warning(_("core.fsmonitor is unset; "
-				"set it if you really want to "
-				"enable fsmonitor"));
+				"set it if you really want to enable the "
+				"hook-based fsmonitor"));
+		}
 		add_fsmonitor(&the_index);
 		report(_("fsmonitor enabled"));
 	} else if (!fsmonitor) {
-		if (git_config_get_fsmonitor() == 1)
+		enum fsmonitor_mode fsm_mode = fsm_settings__get_mode(r);
+		if (fsm_mode == FSMONITOR_MODE_IPC)
+			warning(_("core.useBuiltinFSMonitor is set; "
+				"remove it if you really want to "
+				"disable fsmonitor"));
+		if (fsm_mode == FSMONITOR_MODE_HOOK)
 			warning(_("core.fsmonitor is set; "
 				"remove it if you really want to "
 				"disable fsmonitor"));
diff --git a/cache.h b/cache.h
index d092820c943..8f4e3c8bd1d 100644
--- a/cache.h
+++ b/cache.h
@@ -989,7 +989,6 @@  extern int core_preload_index;
 extern int precomposed_unicode;
 extern int protect_hfs;
 extern int protect_ntfs;
-extern const char *core_fsmonitor;
 
 extern int core_apply_sparse_checkout;
 extern int core_sparse_checkout_cone;
diff --git a/config.c b/config.c
index 2dcbe901b6b..6b6e9cacac3 100644
--- a/config.c
+++ b/config.c
@@ -2502,20 +2502,6 @@  int git_config_get_max_percent_split_change(void)
 	return -1; /* default value */
 }
 
-int git_config_get_fsmonitor(void)
-{
-	if (git_config_get_pathname("core.fsmonitor", &core_fsmonitor))
-		core_fsmonitor = getenv("GIT_TEST_FSMONITOR");
-
-	if (core_fsmonitor && !*core_fsmonitor)
-		core_fsmonitor = NULL;
-
-	if (core_fsmonitor)
-		return 1;
-
-	return 0;
-}
-
 int git_config_get_index_threads(int *dest)
 {
 	int is_bool, val;
diff --git a/config.h b/config.h
index f119de01309..69d733824a0 100644
--- a/config.h
+++ b/config.h
@@ -610,7 +610,6 @@  int git_config_get_pathname(const char *key, const char **dest);
 int git_config_get_index_threads(int *dest);
 int git_config_get_split_index(void);
 int git_config_get_max_percent_split_change(void);
-int git_config_get_fsmonitor(void);
 
 /* This dies if the configured or default date is in the future */
 int git_config_get_expiry(const char *key, const char **output);
diff --git a/environment.c b/environment.c
index 9da7f3c1a19..68f90632245 100644
--- a/environment.c
+++ b/environment.c
@@ -82,7 +82,6 @@  int protect_hfs = PROTECT_HFS_DEFAULT;
 #define PROTECT_NTFS_DEFAULT 1
 #endif
 int protect_ntfs = PROTECT_NTFS_DEFAULT;
-const char *core_fsmonitor;
 
 /*
  * The character that begins a commented line in user-editable file
diff --git a/fsmonitor-settings.c b/fsmonitor-settings.c
new file mode 100644
index 00000000000..2770266f5ee
--- /dev/null
+++ b/fsmonitor-settings.c
@@ -0,0 +1,97 @@ 
+#include "cache.h"
+#include "config.h"
+#include "repository.h"
+#include "fsmonitor-settings.h"
+
+/*
+ * We keep this structure defintion private and have getters
+ * for all fields so that we can lazy load it as needed.
+ */
+struct fsmonitor_settings {
+	enum fsmonitor_mode mode;
+	char *hook_path;
+};
+
+void fsm_settings__set_ipc(struct repository *r)
+{
+	struct fsmonitor_settings *s = r->settings.fsmonitor;
+
+	s->mode = FSMONITOR_MODE_IPC;
+}
+
+void fsm_settings__set_hook(struct repository *r, const char *path)
+{
+	struct fsmonitor_settings *s = r->settings.fsmonitor;
+
+	s->mode = FSMONITOR_MODE_HOOK;
+	s->hook_path = strdup(path);
+}
+
+void fsm_settings__set_disabled(struct repository *r)
+{
+	struct fsmonitor_settings *s = r->settings.fsmonitor;
+
+	s->mode = FSMONITOR_MODE_DISABLED;
+	FREE_AND_NULL(s->hook_path);
+}
+
+static int check_for_ipc(struct repository *r)
+{
+	int value;
+
+	if (!repo_config_get_bool(r, "core.usebuiltinfsmonitor", &value) &&
+	    value) {
+		fsm_settings__set_ipc(r);
+		return 1;
+	}
+
+	return 0;
+}
+
+static int check_for_hook(struct repository *r)
+{
+	const char *const_str;
+
+	if (repo_config_get_pathname(r, "core.fsmonitor", &const_str))
+		const_str = getenv("GIT_TEST_FSMONITOR");
+
+	if (const_str && *const_str) {
+		fsm_settings__set_hook(r, const_str);
+		return 1;
+	}
+
+	return 0;
+}
+
+static void lookup_fsmonitor_settings(struct repository *r)
+{
+	struct fsmonitor_settings *s;
+
+	CALLOC_ARRAY(s, 1);
+
+	r->settings.fsmonitor = s;
+
+	if (check_for_ipc(r))
+		return;
+
+	if (check_for_hook(r))
+		return;
+
+	fsm_settings__set_disabled(r);
+}
+
+enum fsmonitor_mode fsm_settings__get_mode(struct repository *r)
+{
+	if (!r->settings.fsmonitor)
+		lookup_fsmonitor_settings(r);
+
+	return r->settings.fsmonitor->mode;
+}
+
+const char *fsm_settings__get_hook_path(struct repository *r)
+{
+	if (!r->settings.fsmonitor)
+		lookup_fsmonitor_settings(r);
+
+	return r->settings.fsmonitor->hook_path;
+}
diff --git a/fsmonitor-settings.h b/fsmonitor-settings.h
new file mode 100644
index 00000000000..50b29234616
--- /dev/null
+++ b/fsmonitor-settings.h
@@ -0,0 +1,21 @@ 
+#ifndef FSMONITOR_SETTINGS_H
+#define FSMONITOR_SETTINGS_H
+
+struct repository;
+
+enum fsmonitor_mode {
+	FSMONITOR_MODE_DISABLED = 0,
+	FSMONITOR_MODE_HOOK = 1, /* core.fsmonitor */
+	FSMONITOR_MODE_IPC = 2,  /* core.useBuiltinFSMonitor */
+};
+
+void fsm_settings__set_ipc(struct repository *r);
+void fsm_settings__set_hook(struct repository *r, const char *path);
+void fsm_settings__set_disabled(struct repository *r);
+
+enum fsmonitor_mode fsm_settings__get_mode(struct repository *r);
+const char *fsm_settings__get_hook_path(struct repository *r);
+
+struct fsmonitor_settings;
+
+#endif /* FSMONITOR_SETTINGS_H */
diff --git a/fsmonitor.c b/fsmonitor.c
index ec4c46407c5..63174630c0e 100644
--- a/fsmonitor.c
+++ b/fsmonitor.c
@@ -3,6 +3,7 @@ 
 #include "dir.h"
 #include "ewah/ewok.h"
 #include "fsmonitor.h"
+#include "fsmonitor-ipc.h"
 #include "run-command.h"
 #include "strbuf.h"
 
@@ -148,15 +149,18 @@  void write_fsmonitor_extension(struct strbuf *sb, struct index_state *istate)
 /*
  * Call the query-fsmonitor hook passing the last update token of the saved results.
  */
-static int query_fsmonitor(int version, const char *last_update, struct strbuf *query_result)
+static int query_fsmonitor_hook(struct repository *r,
+				int version,
+				const char *last_update,
+				struct strbuf *query_result)
 {
 	struct child_process cp = CHILD_PROCESS_INIT;
 	int result;
 
-	if (!core_fsmonitor)
+	if (fsm_settings__get_mode(r) != FSMONITOR_MODE_HOOK)
 		return -1;
 
-	strvec_push(&cp.args, core_fsmonitor);
+	strvec_push(&cp.args, fsm_settings__get_hook_path(r));
 	strvec_pushf(&cp.args, "%d", version);
 	strvec_pushf(&cp.args, "%s", last_update);
 	cp.use_shell = 1;
@@ -238,17 +242,28 @@  void refresh_fsmonitor(struct index_state *istate)
 	struct strbuf last_update_token = STRBUF_INIT;
 	char *buf;
 	unsigned int i;
+	struct repository *r = istate->repo ? istate->repo : the_repository;
+	enum fsmonitor_mode fsm_mode = fsm_settings__get_mode(r);
 
-	if (!core_fsmonitor || istate->fsmonitor_has_run_once)
+	if (fsm_mode <= FSMONITOR_MODE_DISABLED ||
+	    istate->fsmonitor_has_run_once)
 		return;
 
-	hook_version = fsmonitor_hook_version();
-
 	istate->fsmonitor_has_run_once = 1;
 
 	trace_printf_key(&trace_fsmonitor, "refresh fsmonitor");
+
+	if (fsm_mode == FSMONITOR_MODE_IPC) {
+		/* TODO */
+		return;
+	}
+
+	assert(fsm_mode == FSMONITOR_MODE_HOOK);
+
+	hook_version = fsmonitor_hook_version();
+
 	/*
-	 * This could be racy so save the date/time now and query_fsmonitor
+	 * This could be racy so save the date/time now and query_fsmonitor_hook
 	 * should be inclusive to ensure we don't miss potential changes.
 	 */
 	last_update = getnanotime();
@@ -256,13 +271,14 @@  void refresh_fsmonitor(struct index_state *istate)
 		strbuf_addf(&last_update_token, "%"PRIu64"", last_update);
 
 	/*
-	 * If we have a last update token, call query_fsmonitor for the set of
+	 * If we have a last update token, call query_fsmonitor_hook for the set of
 	 * changes since that token, else assume everything is possibly dirty
 	 * and check it all.
 	 */
 	if (istate->fsmonitor_last_update) {
 		if (hook_version == -1 || hook_version == HOOK_INTERFACE_VERSION2) {
-			query_success = !query_fsmonitor(HOOK_INTERFACE_VERSION2,
+			query_success = !query_fsmonitor_hook(
+				r, HOOK_INTERFACE_VERSION2,
 				istate->fsmonitor_last_update, &query_result);
 
 			if (query_success) {
@@ -292,13 +308,17 @@  void refresh_fsmonitor(struct index_state *istate)
 		}
 
 		if (hook_version == HOOK_INTERFACE_VERSION1) {
-			query_success = !query_fsmonitor(HOOK_INTERFACE_VERSION1,
+			query_success = !query_fsmonitor_hook(
+				r, HOOK_INTERFACE_VERSION1,
 				istate->fsmonitor_last_update, &query_result);
 		}
 
-		trace_performance_since(last_update, "fsmonitor process '%s'", core_fsmonitor);
-		trace_printf_key(&trace_fsmonitor, "fsmonitor process '%s' returned %s",
-			core_fsmonitor, query_success ? "success" : "failure");
+		trace_performance_since(last_update, "fsmonitor process '%s'",
+					fsm_settings__get_hook_path(r));
+		trace_printf_key(&trace_fsmonitor,
+				 "fsmonitor process '%s' returned %s",
+				 fsm_settings__get_hook_path(r),
+				 query_success ? "success" : "failure");
 	}
 
 	/*
@@ -434,7 +454,8 @@  void remove_fsmonitor(struct index_state *istate)
 void tweak_fsmonitor(struct index_state *istate)
 {
 	unsigned int i;
-	int fsmonitor_enabled = git_config_get_fsmonitor();
+	struct repository *r = istate->repo ? istate->repo : the_repository;
+	int fsmonitor_enabled = (fsm_settings__get_mode(r) > FSMONITOR_MODE_DISABLED);
 
 	if (istate->fsmonitor_dirty) {
 		if (fsmonitor_enabled) {
@@ -454,16 +475,8 @@  void tweak_fsmonitor(struct index_state *istate)
 		istate->fsmonitor_dirty = NULL;
 	}
 
-	switch (fsmonitor_enabled) {
-	case -1: /* keep: do nothing */
-		break;
-	case 0: /* false */
-		remove_fsmonitor(istate);
-		break;
-	case 1: /* true */
+	if (fsmonitor_enabled)
 		add_fsmonitor(istate);
-		break;
-	default: /* unknown value: do nothing */
-		break;
-	}
+	else
+		remove_fsmonitor(istate);
 }
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;
+	enum fsmonitor_mode fsm_mode = fsm_settings__get_mode(r);
+
+	return fsm_mode <= FSMONITOR_MODE_DISABLED ||
+		istate->fsmonitor_has_run_once;
 }
 
 /*
@@ -67,7 +72,11 @@  static inline int is_fsmonitor_refreshed(const struct index_state *istate)
  */
 static inline void mark_fsmonitor_valid(struct index_state *istate, struct cache_entry *ce)
 {
-	if (core_fsmonitor && !(ce->ce_flags & CE_FSMONITOR_VALID)) {
+	struct repository *r = istate->repo ? istate->repo : the_repository;
+	enum fsmonitor_mode fsm_mode = fsm_settings__get_mode(r);
+
+	if (fsm_mode > FSMONITOR_MODE_DISABLED &&
+	    !(ce->ce_flags & CE_FSMONITOR_VALID)) {
 		istate->cache_changed = 1;
 		ce->ce_flags |= CE_FSMONITOR_VALID;
 		trace_printf_key(&trace_fsmonitor, "mark_fsmonitor_clean '%s'", ce->name);
@@ -83,7 +92,10 @@  static inline void mark_fsmonitor_valid(struct index_state *istate, struct cache
  */
 static inline void mark_fsmonitor_invalid(struct index_state *istate, struct cache_entry *ce)
 {
-	if (core_fsmonitor) {
+	struct repository *r = istate->repo ? istate->repo : the_repository;
+	enum fsmonitor_mode fsm_mode = fsm_settings__get_mode(r);
+
+	if (fsm_mode > FSMONITOR_MODE_DISABLED) {
 		ce->ce_flags &= ~CE_FSMONITOR_VALID;
 		untracked_cache_invalidate_path(istate, ce->name, 1);
 		trace_printf_key(&trace_fsmonitor, "mark_fsmonitor_invalid '%s'", ce->name);
diff --git a/repository.h b/repository.h
index a057653981c..89a1873ade7 100644
--- a/repository.h
+++ b/repository.h
@@ -4,6 +4,7 @@ 
 #include "path.h"
 
 struct config_set;
+struct fsmonitor_settings;
 struct git_hash_algo;
 struct index_state;
 struct lock_file;
@@ -34,6 +35,8 @@  struct repo_settings {
 	int command_requires_full_index;
 	int sparse_index;
 
+	struct fsmonitor_settings *fsmonitor; /* lazy loaded */
+
 	int index_version;
 	enum untracked_cache_setting core_untracked_cache;
 
diff --git a/t/README b/t/README
index b92155a822e..6dc4a1d10cf 100644
--- a/t/README
+++ b/t/README
@@ -405,8 +405,8 @@  every 'git commit-graph write', as if the `--changed-paths` option was
 passed in.
 
 GIT_TEST_FSMONITOR=$PWD/t7519/fsmonitor-all exercises the fsmonitor
-code path for utilizing a file system monitor to speed up detecting
-new or changed files.
+code path for utilizing a (hook based) file system monitor to speed up
+detecting new or changed files.
 
 GIT_TEST_INDEX_VERSION=<n> exercises the index read/write code path
 for the index version specified.  Can be set to any valid version