[v2,07/28] fsmonitor: introduce `core.useBuiltinFSMonitor` to call the daemon via IPC

Commit Message

Johannes Schindelin May 22, 2021, 1:56 p.m. UTC

From: Johannes Schindelin <[e-mail]>

Use simple IPC to directly communicate with the new builtin file
system monitor daemon.

Define a new config setting `core.useBuiltinFSMonitor` to enable the
builtin file system monitor.

The `core.fsmonitor` setting has already been defined as a HOOK
pathname.  Historically, this has been set to a HOOK script that will
talk with Watchman.  For compatibility reasons, we do not want to
overload that definition (and cause problems if users have multiple
versions of Git installed).

Signed-off-by: Johannes Schindelin <[e-mail]>
Signed-off-by: Jeff Hostetler <[e-mail]>
---
 config.c        |  5 +++++
 fsmonitor.c     | 43 +++++++++++++++++++++++++++++++++++++++++++
 repo-settings.c |  3 +++
 repository.h    |  2 ++
 4 files changed, 53 insertions(+)

Comments

Johannes Schindelin June 14, 2021, 9:28 p.m. UTC | #1

Hi Jeff,

On Sat, 22 May 2021, Johannes Schindelin via GitGitGadget wrote:

> diff --git a/fsmonitor.c b/fsmonitor.c
> index 9c9b2abc9414..c6d3c34ad78e 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"
>
> @@ -231,6 +232,7 @@ static void fsmonitor_refresh_callback(struct index_state *istate, char *name)
>
>  void refresh_fsmonitor(struct index_state *istate)
>  {
> +	struct repository *r = istate->repo ? istate->repo : the_repository;
>  	struct strbuf query_result = STRBUF_INIT;
>  	int query_success = 0, hook_version = -1;
>  	size_t bol = 0; /* beginning of line */
> @@ -247,6 +249,46 @@ void refresh_fsmonitor(struct index_state *istate)
>  	istate->fsmonitor_has_run_once = 1;
>
>  	trace_printf_key(&trace_fsmonitor, "refresh fsmonitor");
> +
> +	if (r->settings.use_builtin_fsmonitor > 0) {
> +		query_success = !fsmonitor_ipc__send_query(
> +			istate->fsmonitor_last_update, &query_result);

As I pointed out elsewhere in the thread, this is a slight change in
behavior: in the previous iteration, we had this call in
`query_fsmonitor()`, which was only ever called if
`istate->fsmonitor_last_update` is non-`NULL`.

The code in `fsmonitor_ipc__send_query()` does actually depend on this,
therefore we need this change to be squashed in:

-- snip --
diff --git a/fsmonitor.c b/fsmonitor.c
index 22623fd228f..0b40643442e 100644
--- a/fsmonitor.c
+++ b/fsmonitor.c
@@ -290,8 +290,9 @@ void refresh_fsmonitor(struct index_state *istate)
 	trace_printf_key(&trace_fsmonitor, "refresh fsmonitor");

 	if (r->settings.use_builtin_fsmonitor > 0) {
-		query_success = !fsmonitor_ipc__send_query(
-			istate->fsmonitor_last_update, &query_result);
+		query_success = istate->fsmonitor_last_update &&
+			!fsmonitor_ipc__send_query(istate->fsmonitor_last_update,
+						   &query_result);
 		if (query_success) {
 			/*
 			 * The response contains a series of nul terminated
-- snap --

Thanks,
Dscho

> +		if (query_success) {
> +			/*
> +			 * The response contains a series of nul terminated
> +			 * strings.  The first is the new token.
> +			 *
> +			 * Use `char *buf` as an interlude to trick the CI
> +			 * static analysis to let us use `strbuf_addstr()`
> +			 * here (and only copy the token) rather than
> +			 * `strbuf_addbuf()`.
> +			 */
> +			buf = query_result.buf;
> +			strbuf_addstr(&last_update_token, buf);
> +			bol = last_update_token.len + 1;
> +		} else {
> +			/*
> +			 * The builtin daemon is not available on this
> +			 * platform -OR- we failed to get a response.
> +			 *
> +			 * Generate a fake token (rather than a V1
> +			 * timestamp) for the index extension.  (If
> +			 * they switch back to the hook API, we don't
> +			 * want ambiguous state.)
> +			 */
> +			strbuf_addstr(&last_update_token, "builtin:fake");
> +		}
> +
> +		/*
> +		 * Regardless of whether we successfully talked to a
> +		 * fsmonitor daemon or not, we skip over and do not
> +		 * try to use the hook.  The "core.useBuiltinFSMonitor"
> +		 * config setting ALWAYS overrides the "core.fsmonitor"
> +		 * hook setting.
> +		 */
> +		goto apply_results;
> +	}
> +
>  	/*
>  	 * This could be racy so save the date/time now and query_fsmonitor
>  	 * should be inclusive to ensure we don't miss potential changes.
> @@ -301,6 +343,7 @@ void refresh_fsmonitor(struct index_state *istate)
>  			core_fsmonitor, query_success ? "success" : "failure");
>  	}
>
> +apply_results:
>  	/* a fsmonitor process can return '/' to indicate all entries are invalid */
>  	if (query_success && query_result.buf[bol] != '/') {
>  		/* Mark all entries returned by the monitor as dirty */
> diff --git a/repo-settings.c b/repo-settings.c
> index f7fff0f5ab83..93aab92ff164 100644
> --- a/repo-settings.c
> +++ b/repo-settings.c
> @@ -58,6 +58,9 @@ void prepare_repo_settings(struct repository *r)
>  		r->settings.core_multi_pack_index = value;
>  	UPDATE_DEFAULT_BOOL(r->settings.core_multi_pack_index, 1);
>
> +	if (!repo_config_get_bool(r, "core.usebuiltinfsmonitor", &value) && value)
> +		r->settings.use_builtin_fsmonitor = 1;
> +
>  	if (!repo_config_get_bool(r, "feature.manyfiles", &value) && value) {
>  		UPDATE_DEFAULT_BOOL(r->settings.index_version, 4);
>  		UPDATE_DEFAULT_BOOL(r->settings.core_untracked_cache, UNTRACKED_CACHE_WRITE);
> diff --git a/repository.h b/repository.h
> index b385ca3c94b6..d6e7f61f9cf7 100644
> --- a/repository.h
> +++ b/repository.h
> @@ -29,6 +29,8 @@ enum fetch_negotiation_setting {
>  struct repo_settings {
>  	int initialized;
>
> +	int use_builtin_fsmonitor;
> +
>  	int core_commit_graph;
>  	int commit_graph_read_changed_paths;
>  	int gc_write_commit_graph;
> --
> gitgitgadget
>
>

Patch

diff --git a/config.c b/config.c
index a896f44cba1f..c82f40c22b43 100644
--- a/config.c
+++ b/config.c
@@ -2501,6 +2501,11 @@  int git_config_get_max_percent_split_change(void)
 
 int repo_config_get_fsmonitor(struct repository *r)
 {
+	if (r->settings.use_builtin_fsmonitor > 0) {
+		core_fsmonitor = "(built-in daemon)";
+		return 1;
+	}
+
 	if (repo_config_get_pathname(r, "core.fsmonitor", &core_fsmonitor))
 		core_fsmonitor = getenv("GIT_TEST_FSMONITOR");
 
diff --git a/fsmonitor.c b/fsmonitor.c
index 9c9b2abc9414..c6d3c34ad78e 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"
 
@@ -231,6 +232,7 @@  static void fsmonitor_refresh_callback(struct index_state *istate, char *name)
 
 void refresh_fsmonitor(struct index_state *istate)
 {
+	struct repository *r = istate->repo ? istate->repo : the_repository;
 	struct strbuf query_result = STRBUF_INIT;
 	int query_success = 0, hook_version = -1;
 	size_t bol = 0; /* beginning of line */
@@ -247,6 +249,46 @@  void refresh_fsmonitor(struct index_state *istate)
 	istate->fsmonitor_has_run_once = 1;
 
 	trace_printf_key(&trace_fsmonitor, "refresh fsmonitor");
+
+	if (r->settings.use_builtin_fsmonitor > 0) {
+		query_success = !fsmonitor_ipc__send_query(
+			istate->fsmonitor_last_update, &query_result);
+		if (query_success) {
+			/*
+			 * The response contains a series of nul terminated
+			 * strings.  The first is the new token.
+			 *
+			 * Use `char *buf` as an interlude to trick the CI
+			 * static analysis to let us use `strbuf_addstr()`
+			 * here (and only copy the token) rather than
+			 * `strbuf_addbuf()`.
+			 */
+			buf = query_result.buf;
+			strbuf_addstr(&last_update_token, buf);
+			bol = last_update_token.len + 1;
+		} else {
+			/*
+			 * The builtin daemon is not available on this
+			 * platform -OR- we failed to get a response.
+			 *
+			 * Generate a fake token (rather than a V1
+			 * timestamp) for the index extension.  (If
+			 * they switch back to the hook API, we don't
+			 * want ambiguous state.)
+			 */
+			strbuf_addstr(&last_update_token, "builtin:fake");
+		}
+
+		/*
+		 * Regardless of whether we successfully talked to a
+		 * fsmonitor daemon or not, we skip over and do not
+		 * try to use the hook.  The "core.useBuiltinFSMonitor"
+		 * config setting ALWAYS overrides the "core.fsmonitor"
+		 * hook setting.
+		 */
+		goto apply_results;
+	}
+
 	/*
 	 * This could be racy so save the date/time now and query_fsmonitor
 	 * should be inclusive to ensure we don't miss potential changes.
@@ -301,6 +343,7 @@  void refresh_fsmonitor(struct index_state *istate)
 			core_fsmonitor, query_success ? "success" : "failure");
 	}
 
+apply_results:
 	/* a fsmonitor process can return '/' to indicate all entries are invalid */
 	if (query_success && query_result.buf[bol] != '/') {
 		/* Mark all entries returned by the monitor as dirty */
diff --git a/repo-settings.c b/repo-settings.c
index f7fff0f5ab83..93aab92ff164 100644
--- a/repo-settings.c
+++ b/repo-settings.c
@@ -58,6 +58,9 @@  void prepare_repo_settings(struct repository *r)
 		r->settings.core_multi_pack_index = value;
 	UPDATE_DEFAULT_BOOL(r->settings.core_multi_pack_index, 1);
 
+	if (!repo_config_get_bool(r, "core.usebuiltinfsmonitor", &value) && value)
+		r->settings.use_builtin_fsmonitor = 1;
+
 	if (!repo_config_get_bool(r, "feature.manyfiles", &value) && value) {
 		UPDATE_DEFAULT_BOOL(r->settings.index_version, 4);
 		UPDATE_DEFAULT_BOOL(r->settings.core_untracked_cache, UNTRACKED_CACHE_WRITE);
diff --git a/repository.h b/repository.h
index b385ca3c94b6..d6e7f61f9cf7 100644
--- a/repository.h
+++ b/repository.h
@@ -29,6 +29,8 @@  enum fetch_negotiation_setting {
 struct repo_settings {
 	int initialized;
 
+	int use_builtin_fsmonitor;
+
 	int core_commit_graph;
 	int commit_graph_read_changed_paths;
 	int gc_write_commit_graph;