[v3,06/34] fsmonitor: config settings are repository-specific

Commit Message

Jeff Hostetler July 1, 2021, 2:47 p.m. UTC

From: Jeff Hostetler <[e-mail]>

Move FSMonitor config settings to `struct repo_settings`, get rid of
the `core_fsmonitor` global variable, and add support for the new
`core.useBuiltinFSMonitor` config setting.  Move config code to lookup
`core.fsmonitor` into `prepare_repo_settings()` with the rest of the
setup code.

The `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 will lead to confusion when we add
support for a builtin FSMonitor based on IPC, since the builtin
FSMonitor doesn't need the hook pathname.

Replace the boolean usage with an `enum fsmonitor_mode` to represent
the state of FSMonitor.  And only set the pathname when in HOOK mode.

Also, disable FSMonitor when the repository working directory is
incompatible.  For example, in bare repositories, since there aren't
any files to watch.

Signed-off-by: Jeff Hostetler <[e-mail]>
---
 builtin/update-index.c      | 20 +++++++++---
 cache.h                     |  1 -
 config.c                    | 14 --------
 config.h                    |  1 -
 environment.c               |  1 -
 fsmonitor.c                 | 65 +++++++++++++++++++++++--------------
 fsmonitor.h                 | 14 ++++++--
 repo-settings.c             | 48 +++++++++++++++++++++++++++
 repository.h                | 11 +++++++
 t/README                    |  4 +--
 t/t7519-status-fsmonitor.sh | 22 +++++++++++++
 11 files changed, 150 insertions(+), 51 deletions(-)

Comments

Ævar Arnfjörð Bjarmason July 1, 2021, 4:46 p.m. UTC | #1

On Thu, Jul 01 2021, Jeff Hostetler via GitGitGadget wrote:

In a reference to a discussion[1] about an earlier version of this patch
you said:

    I'm going to ignore all of the thread responses to this patch
    dealing with how we acquire config settings and macros and etc.
    Those issues are completely independent of FSMonitor (which is
    already way too big).

Since then the changes to repo-settings.c have become a lot larger, so
let's take a look...

1. https://lore.kernel.org/git/[e-mail]/
2. https://lore.kernel.org/git/[e-mail]/


> diff --git a/repo-settings.c b/repo-settings.c
> index 0cfe8b787db..faf197ff60a 100644
> --- a/repo-settings.c
> +++ b/repo-settings.c
> @@ -5,10 +5,42 @@
>  
>  #define UPDATE_DEFAULT_BOOL(s,v) do { if (s == -1) { s = v; } } while(0)
>  
> +/*
> + * Return 1 if the repo/workdir is incompatible with FSMonitor.
> + */
> +static int is_repo_incompatible_with_fsmonitor(struct repository *r)
> +{
> +	const char *const_strval;
> +
> +	/*
> +	 * Bare repositories don't have a working directory and
> +	 * therefore, nothing to watch.
> +	 */
> +	if (!r->worktree)
> +		return 1;

Looking ahead in this series you end up using
FSMONITOR_MODE_INCOMPATIBLE in two places in the codebase. In
builtin/update-index.c to throw a "repository is incompatible with
fsmonitor" error.

Can't that case just be replaced with setup_work_tree()? Other sub-modes
of update-index already die implicitly on that, e.g.:

	$ git update-index test
	fatal: this operation must be run in a work tree

The other case is:
	
	+       prepare_repo_settings(the_repository);
	+       if (!the_repository->worktree)
	+               return error(_("fsmonitor-daemon does not support bare repos '%s'"),
	+                            xgetcwd());
	+       if (the_repository->settings.fsmonitor_mode == FSMONITOR_MODE_INCOMPATIBLE)
	+               return error(_("fsmonitor-daemon is incompatible with this repo '%s'"),
	+                            the_repository->worktree);

I.e. we just checked the_repository->worktree, but it's not that, but....

> +
> +	/*
> +	 * GVFS (aka VFS for Git) is incompatible with FSMonitor.
> +	 *
> +	 * Granted, core Git does not know anything about GVFS and
> +	 * we shouldn't make assumptions about a downstream feature,
> +	 * but users can install both versions.  And this can lead
> +	 * to incorrect results from core Git commands.  So, without
> +	 * bringing in any of the GVFS code, do a simple config test
> +	 * for a published config setting.  (We do not look at the
> +	 * various *_TEST_* environment variables.)
> +	 */
> +	if (!repo_config_get_value(r, "core.virtualfilesystem", &const_strval))
> +		return 1;

I'm skeptical of us hardcoding a third-party software config
variable. Can't GitVFS handle this somehow on its end?

But just in terms of implementation it seems the end result of that is
to emit a very confusing error to the user. Sinc we already checked for
bare repos we run into this and instead of sayingwhen we should really
say "hey, maybe disable your core.virtualFileSystem setting", we say
"your repo is incompatible".

> +
> +	return 0;
> +}
> +
>  void prepare_repo_settings(struct repository *r)
>  {
>  	int value;
>  	char *strval;
> +	const char *const_strval;

Can be declared in the "else" below.

>  
>  	if (r->settings.initialized)
>  		return;
> @@ -26,6 +58,22 @@ void prepare_repo_settings(struct repository *r)
>  	UPDATE_DEFAULT_BOOL(r->settings.commit_graph_read_changed_paths, 1);
>  	UPDATE_DEFAULT_BOOL(r->settings.gc_write_commit_graph, 1);
>  
> +	r->settings.fsmonitor_hook_path = NULL;
> +	r->settings.fsmonitor_mode = FSMONITOR_MODE_DISABLED;

With the memset earlier (b.t.w. I've got a patch to fix all this bizarre
behavior in repo-settings.c, but have been waiting on this series we
implicitly set it to FSMONITOR_MODE_UNSET (-1) with the memset, but then
never use that ever.

Your code in update-index.c then for a check against
"FSMONITOR_MODE_DISABLED" says "core.useBuiltinFSMonitor is unset;".

> +	if (is_repo_incompatible_with_fsmonitor(r))
> +		r->settings.fsmonitor_mode = FSMONITOR_MODE_INCOMPATIBLE;

Style: should have {} braces on all arms.

> +	else if (!repo_config_get_bool(r, "core.usebuiltinfsmonitor", &value)
> +		   && value)
> +		r->settings.fsmonitor_mode = FSMONITOR_MODE_IPC;

Here you're conflating false with whether the variable is set at all. I
guess that works out here since if it's false we want to fall through
to...

> +	else {

...ignoring it and looing at core.fsmonitor instead.

> +		if (repo_config_get_pathname(r, "core.fsmonitor", &const_strval))
> +			const_strval = getenv("GIT_TEST_FSMONITOR");

If it's not set we pay attention to GIT_TEST_FSMONITOR, so the behavior
from the old git_config_get_fsmonitor(). So even if the env variable is
set we want to take the config variable over it, correct?

> +		if (const_strval && *const_strval) {
> +			r->settings.fsmonitor_hook_path = strdup(const_strval);

We had a strbuf_detach()'d string in the case of
repo_config_get_pathname(), but here we strdup() it again in case we
were in the getenv() codepath. This code probably leaks memory now
anyway, but perhaps it's better to split up the two so we make it easier
to deal with who owns/frees what in the future.

Jeff Hostetler July 19, 2021, 8:36 p.m. UTC | #2

On 7/1/21 12:46 PM, Ævar Arnfjörð Bjarmason wrote:
> 
> On Thu, Jul 01 2021, Jeff Hostetler via GitGitGadget wrote:
> 
> In a reference to a discussion[1] about an earlier version of this patch
> you said:
> 
>      I'm going to ignore all of the thread responses to this patch
>      dealing with how we acquire config settings and macros and etc.
>      Those issues are completely independent of FSMonitor (which is
>      already way too big).
> 
> Since then the changes to repo-settings.c have become a lot larger, so
> let's take a look...
> 
> 1. https://lore.kernel.org/git/[e-mail]/
> 2. https://lore.kernel.org/git/[e-mail]/

Yes, there was a large conversation about re-doing how config
values are acquired or whatever and it was nested inside of
the context of a completely unrelated topic.  It still is.

I would like to focus on FSMonitor using the existing config API.
I'm only looking up ~2 config values.  And that code is fairly
minor considering everything else in this patch series.

Yes, there's a bizarre initialization with memset(-1), but it
can wait.

Later (in a clean context) we can address and focus on the config
API and/or structure initialization that you're talking about here.

FWIW, in V4 I'll refactor the block of code that I added into the
body of prepare_repo_settings() to address your later concerns.
This version will hopefully be more readable.

> 
> 
>> diff --git a/repo-settings.c b/repo-settings.c
>> index 0cfe8b787db..faf197ff60a 100644
>> --- a/repo-settings.c
>> +++ b/repo-settings.c
>> @@ -5,10 +5,42 @@
>>   
>>   #define UPDATE_DEFAULT_BOOL(s,v) do { if (s == -1) { s = v; } } while(0)
>>   
>> +/*
>> + * Return 1 if the repo/workdir is incompatible with FSMonitor.
>> + */
>> +static int is_repo_incompatible_with_fsmonitor(struct repository *r)
>> +{
>> +	const char *const_strval;
>> +
>> +	/*
>> +	 * Bare repositories don't have a working directory and
>> +	 * therefore, nothing to watch.
>> +	 */
>> +	if (!r->worktree)
>> +		return 1;
> 
> Looking ahead in this series you end up using
> FSMONITOR_MODE_INCOMPATIBLE in two places in the codebase. In
> builtin/update-index.c to throw a "repository is incompatible with
> fsmonitor" error.
> 
> Can't that case just be replaced with setup_work_tree()? Other sub-modes
> of update-index already die implicitly on that, e.g.:
> 
> 	$ git update-index test
> 	fatal: this operation must be run in a work tree

I will refactor that static function in V4, but I want it to return
an indication of whether the repo is compatible only and let the
command print the error/die as is appropriate for the daemon and/or
update-index.  The daemon should not start on an incompatible repo.
Likewise, update-index should not enable the extension in the index.

We share some of that code with the client side code, like status,
which wants to talk to the hook or daemon if supported/present/allowed.
If the repo is incompatible, then status should just behave in the
classic manner.  So I don't want the detection code to print those
error messages or die.

> 
> The other case is:
> 	
> 	+       prepare_repo_settings(the_repository);
> 	+       if (!the_repository->worktree)
> 	+               return error(_("fsmonitor-daemon does not support bare repos '%s'"),
> 	+                            xgetcwd());
> 	+       if (the_repository->settings.fsmonitor_mode == FSMONITOR_MODE_INCOMPATIBLE)
> 	+               return error(_("fsmonitor-daemon is incompatible with this repo '%s'"),
> 	+                            the_repository->worktree);
> 
> I.e. we just checked the_repository->worktree, but it's not that, but....

yes, I currently have 2 types of repos that I want to disable
both the hook and daemon version of FSMonitor.  I'll update the
error messages to specify the reason why we are incompatible.

> 
>> +
>> +	/*
>> +	 * GVFS (aka VFS for Git) is incompatible with FSMonitor.
>> +	 *
>> +	 * Granted, core Git does not know anything about GVFS and
>> +	 * we shouldn't make assumptions about a downstream feature,
>> +	 * but users can install both versions.  And this can lead
>> +	 * to incorrect results from core Git commands.  So, without
>> +	 * bringing in any of the GVFS code, do a simple config test
>> +	 * for a published config setting.  (We do not look at the
>> +	 * various *_TEST_* environment variables.)
>> +	 */
>> +	if (!repo_config_get_value(r, "core.virtualfilesystem", &const_strval))
>> +		return 1;
> 
> I'm skeptical of us hardcoding a third-party software config
> variable. Can't GitVFS handle this somehow on its end?

Adding the test for a GVFS-specific config setting is questionable.
And perhaps we should move it to our downstream fork.

The value in putting it here for now (at least) is that it makes
clear the structure for supporting other types of incompatible
repos.

For example, perhaps we want to disallow repos that are on remote
file systems (since we might not be able to get FS events).  It
would be nice to be able to prevent the daemon from starting and/or
from status from trying to connect to a daemon that will be a
disappointment.  The code I have here serves as a model for adding
such additional restrictions.  And keeps us from trying to prematurely
collapse the code into a simple expression.

> 
> But just in terms of implementation it seems the end result of that is
> to emit a very confusing error to the user. Sinc we already checked for
> bare repos we run into this and instead of sayingwhen we should really
> say "hey, maybe disable your core.virtualFileSystem setting", we say
> "your repo is incompatible".
> 

I'll update the error messages to make that clear.

>> +
>> +	return 0;
>> +}
>> +
>>   void prepare_repo_settings(struct repository *r)
>>   {
>>   	int value;
>>   	char *strval;
>> +	const char *const_strval;
> 
> Can be declared in the "else" below.
> 
>>   
>>   	if (r->settings.initialized)
>>   		return;
>> @@ -26,6 +58,22 @@ void prepare_repo_settings(struct repository *r)
>>   	UPDATE_DEFAULT_BOOL(r->settings.commit_graph_read_changed_paths, 1);
>>   	UPDATE_DEFAULT_BOOL(r->settings.gc_write_commit_graph, 1);
>>   
>> +	r->settings.fsmonitor_hook_path = NULL;
>> +	r->settings.fsmonitor_mode = FSMONITOR_MODE_DISABLED;
> 
> With the memset earlier (b.t.w. I've got a patch to fix all this bizarre
> behavior in repo-settings.c, but have been waiting on this series we
> implicitly set it to FSMONITOR_MODE_UNSET (-1) with the memset, but then
> never use that ever.

I'm working around the bogus -1 value that the structure has
been initialize with and that I'm inheriting.  I'll completely
set my `fsmonitor_mode` variable so that I don't care what it
is initialized to.

I created the _UNSET value as a reminder that there is this memset(-1)
there and that must be attended to.

In my V4 I'll add comments to that effect.

> 
> Your code in update-index.c then for a check against
> "FSMONITOR_MODE_DISABLED" says "core.useBuiltinFSMonitor is unset;".
> 
>> +	if (is_repo_incompatible_with_fsmonitor(r))
>> +		r->settings.fsmonitor_mode = FSMONITOR_MODE_INCOMPATIBLE;
> 
> Style: should have {} braces on all arms.
> 
>> +	else if (!repo_config_get_bool(r, "core.usebuiltinfsmonitor", &value)
>> +		   && value)
>> +		r->settings.fsmonitor_mode = FSMONITOR_MODE_IPC;
> 
> Here you're conflating false with whether the variable is set at all. I
> guess that works out here since if it's false we want to fall through
> to...
> 
>> +	else {
> 
> ...ignoring it and looing at core.fsmonitor instead.

yes, if core.useBuiltinFSMonitor is true, we do not need to look
at the hook pathname.

> 
>> +		if (repo_config_get_pathname(r, "core.fsmonitor", &const_strval))
>> +			const_strval = getenv("GIT_TEST_FSMONITOR");
> 
> If it's not set we pay attention to GIT_TEST_FSMONITOR, so the behavior
> from the old git_config_get_fsmonitor(). So even if the env variable is
> set we want to take the config variable over it, correct?

GIT_TEST_FSMONITOR sets the hook path for testing and we're not using
the hook API at all.  So it is kind of ill-defined what that test env
var should do if you have IPC turned on, so I'm ignoring it.

> 
>> +		if (const_strval && *const_strval) {
>> +			r->settings.fsmonitor_hook_path = strdup(const_strval);
> 
> We had a strbuf_detach()'d string in the case of
> repo_config_get_pathname(), but here we strdup() it again in case we
> were in the getenv() codepath. This code probably leaks memory now
> anyway, but perhaps it's better to split up the two so we make it easier
> to deal with who owns/frees what in the future.
> 

repo_config_get_pathname() returns a const char** which implies that
it is not a detached buffer.  it is a deep trek thru the config code,
but it eventually gets to git_configset_get_*() which is documented as
returning a pointer into a configset cache and that the caller should
not free it.  so dup'ing it is appropriate.

Similarly, getenv() is also returning a pointer to a buffer that the
caller does not own.  so dup'ing it here is also appropriate.

Thanks,
Jeff

Patch

diff --git a/builtin/update-index.c b/builtin/update-index.c
index f1f16f2de52..0141899caa3 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -1216,14 +1216,26 @@  int cmd_update_index(int argc, const char **argv, const char *prefix)
 	}
 
 	if (fsmonitor > 0) {
-		if (git_config_get_fsmonitor() == 0)
+		if (r->settings.fsmonitor_mode == FSMONITOR_MODE_INCOMPATIBLE)
+			return error(
+				_("repository is incompatible with fsmonitor"));
+
+		if (r->settings.fsmonitor_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)
+		if (r->settings.fsmonitor_mode == FSMONITOR_MODE_IPC)
+			warning(_("core.useBuiltinFSMonitor is set; "
+				"remove it if you really want to "
+				"disable fsmonitor"));
+		if (r->settings.fsmonitor_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 ba04ff8bd36..50463876852 100644
--- a/cache.h
+++ b/cache.h
@@ -981,7 +981,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 f9c400ad306..ec3b88b4639 100644
--- a/config.c
+++ b/config.c
@@ -2516,20 +2516,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 9038538ffdc..b8d6b75d4fe 100644
--- a/config.h
+++ b/config.h
@@ -609,7 +609,6 @@  int git_config_get_index_threads(int *dest);
 int git_config_get_untracked_cache(void);
 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 2f27008424a..7b5e8ff78da 100644
--- a/environment.c
+++ b/environment.c
@@ -84,7 +84,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.c b/fsmonitor.c
index ab9bfc60b34..374189be7d9 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,21 @@  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 (r->settings.fsmonitor_mode != FSMONITOR_MODE_HOOK)
 		return -1;
 
-	strvec_push(&cp.args, core_fsmonitor);
+	assert(r->settings.fsmonitor_hook_path);
+	assert(*r->settings.fsmonitor_hook_path);
+
+	strvec_push(&cp.args, r->settings.fsmonitor_hook_path);
 	strvec_pushf(&cp.args, "%d", version);
 	strvec_pushf(&cp.args, "%s", last_update);
 	cp.use_shell = 1;
@@ -238,17 +245,27 @@  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;
 
-	if (!core_fsmonitor || istate->fsmonitor_has_run_once)
+	if (r->settings.fsmonitor_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 (r->settings.fsmonitor_mode == FSMONITOR_MODE_IPC) {
+		/* TODO */
+		return;
+	}
+
+	assert(r->settings.fsmonitor_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 +273,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 +310,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'",
+					r->settings.fsmonitor_hook_path);
+		trace_printf_key(&trace_fsmonitor,
+				 "fsmonitor process '%s' returned %s",
+				 r->settings.fsmonitor_hook_path,
+				 query_success ? "success" : "failure");
 	}
 
 	/* a fsmonitor process can return '/' to indicate all entries are invalid */
@@ -411,7 +433,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 = r->settings.fsmonitor_mode > FSMONITOR_MODE_DISABLED;
 
 	if (istate->fsmonitor_dirty) {
 		if (fsmonitor_enabled) {
@@ -431,16 +454,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..9cc14e05239 100644
--- a/fsmonitor.h
+++ b/fsmonitor.h
@@ -57,7 +57,10 @@  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;
+
+	return r->settings.fsmonitor_mode <= FSMONITOR_MODE_DISABLED ||
+		istate->fsmonitor_has_run_once;
 }
 
 /*
@@ -67,7 +70,10 @@  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;
+
+	if (r->settings.fsmonitor_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 +89,9 @@  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;
+
+	if (r->settings.fsmonitor_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/repo-settings.c b/repo-settings.c
index 0cfe8b787db..faf197ff60a 100644
--- a/repo-settings.c
+++ b/repo-settings.c
@@ -5,10 +5,42 @@ 
 
 #define UPDATE_DEFAULT_BOOL(s,v) do { if (s == -1) { s = v; } } while(0)
 
+/*
+ * Return 1 if the repo/workdir is incompatible with FSMonitor.
+ */
+static int is_repo_incompatible_with_fsmonitor(struct repository *r)
+{
+	const char *const_strval;
+
+	/*
+	 * Bare repositories don't have a working directory and
+	 * therefore, nothing to watch.
+	 */
+	if (!r->worktree)
+		return 1;
+
+	/*
+	 * GVFS (aka VFS for Git) is incompatible with FSMonitor.
+	 *
+	 * Granted, core Git does not know anything about GVFS and
+	 * we shouldn't make assumptions about a downstream feature,
+	 * but users can install both versions.  And this can lead
+	 * to incorrect results from core Git commands.  So, without
+	 * bringing in any of the GVFS code, do a simple config test
+	 * for a published config setting.  (We do not look at the
+	 * various *_TEST_* environment variables.)
+	 */
+	if (!repo_config_get_value(r, "core.virtualfilesystem", &const_strval))
+		return 1;
+
+	return 0;
+}
+
 void prepare_repo_settings(struct repository *r)
 {
 	int value;
 	char *strval;
+	const char *const_strval;
 
 	if (r->settings.initialized)
 		return;
@@ -26,6 +58,22 @@  void prepare_repo_settings(struct repository *r)
 	UPDATE_DEFAULT_BOOL(r->settings.commit_graph_read_changed_paths, 1);
 	UPDATE_DEFAULT_BOOL(r->settings.gc_write_commit_graph, 1);
 
+	r->settings.fsmonitor_hook_path = NULL;
+	r->settings.fsmonitor_mode = FSMONITOR_MODE_DISABLED;
+	if (is_repo_incompatible_with_fsmonitor(r))
+		r->settings.fsmonitor_mode = FSMONITOR_MODE_INCOMPATIBLE;
+	else if (!repo_config_get_bool(r, "core.usebuiltinfsmonitor", &value)
+		   && value)
+		r->settings.fsmonitor_mode = FSMONITOR_MODE_IPC;
+	else {
+		if (repo_config_get_pathname(r, "core.fsmonitor", &const_strval))
+			const_strval = getenv("GIT_TEST_FSMONITOR");
+		if (const_strval && *const_strval) {
+			r->settings.fsmonitor_hook_path = strdup(const_strval);
+			r->settings.fsmonitor_mode = FSMONITOR_MODE_HOOK;
+		}
+	}
+
 	if (!repo_config_get_int(r, "index.version", &value))
 		r->settings.index_version = value;
 	if (!repo_config_get_maybe_bool(r, "core.untrackedcache", &value)) {
diff --git a/repository.h b/repository.h
index a45f7520fd9..09154298ba1 100644
--- a/repository.h
+++ b/repository.h
@@ -26,6 +26,14 @@  enum fetch_negotiation_setting {
 	FETCH_NEGOTIATION_NOOP = 3,
 };
 
+enum fsmonitor_mode {
+	FSMONITOR_MODE_INCOMPATIBLE = -2,
+	FSMONITOR_MODE_UNSET = -1,
+	FSMONITOR_MODE_DISABLED = 0,
+	FSMONITOR_MODE_HOOK = 1, /* core.fsmonitor */
+	FSMONITOR_MODE_IPC = 2, /* core.useBuiltinFSMonitor */
+};
+
 struct repo_settings {
 	int initialized;
 
@@ -34,6 +42,9 @@  struct repo_settings {
 	int gc_write_commit_graph;
 	int fetch_write_commit_graph;
 
+	enum fsmonitor_mode fsmonitor_mode;
+	char *fsmonitor_hook_path;
+
 	int index_version;
 	enum untracked_cache_setting core_untracked_cache;
 
diff --git a/t/README b/t/README
index 1a2072b2c8a..852a4eae9da 100644
--- a/t/README
+++ b/t/README
@@ -398,8 +398,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
diff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh
index 637391c6ce4..02919c68ddd 100755
--- a/t/t7519-status-fsmonitor.sh
+++ b/t/t7519-status-fsmonitor.sh
@@ -383,4 +383,26 @@  test_expect_success 'status succeeds after staging/unstaging' '
 	)
 '
 
+# Test that we detect and disallow repos that are incompatible with FSMonitor.
+test_expect_success 'incompatible bare repo' '
+	test_when_finished "rm -rf ./bare-clone" &&
+	git clone --bare . ./bare-clone &&
+	cat >expect <<-\EOF &&
+	error: repository is incompatible with fsmonitor
+	EOF
+	test_must_fail git -C ./bare-clone update-index --fsmonitor 2>actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'incompatible core.virtualfilesystem' '
+	test_when_finished "rm -rf ./fake-gvfs-clone" &&
+	git clone . ./fake-gvfs-clone &&
+	git -C ./fake-gvfs-clone config core.virtualfilesystem true &&
+	cat >expect <<-\EOF &&
+	error: repository is incompatible with fsmonitor
+	EOF
+	test_must_fail git -C ./fake-gvfs-clone update-index --fsmonitor 2>actual &&
+	test_cmp expect actual
+'
+
 test_done