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

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

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