Re: [PATCH 04/23] fsmonitor: introduce `core.useBuiltinFSMonitor` to call the daemon via IPC

On 4/26/21 10:56 AM, Derrick Stolee wrote:

On 4/1/21 11:40 AM, Johannes Schindelin via GitGitGadget wrote:> @@ -2515,6 +2515,11 @@ int git_config_get_max_percent_split_change(void)

quoted

  
  int repo_config_get_fsmonitor(struct repository *r)
  {
+	if (r->settings.use_builtin_fsmonitor > 0) {

Don't forget to run prepare_repo_settings(r) first.

quoted

+		core_fsmonitor = "(built-in daemon)";
+		return 1;
+	}
+

I found this odd, assigning a string to core_fsmonitor that
would definitely cause a problem trying to execute it as a
hook. I wondered the need for it at all, but found that
there are several places in the FS Monitor subsystem that use
core_fsmonitor as if it was a boolean, indicating whether or
not the feature is enabled at all.

A cleaner way to handle this would be to hide the data behind
a helper method, say "fsmonitor_enabled()" that could then
check a value on the repository (or index) and store the hook
value as a separate value that is only used by the hook-based
implementation.

It's probably a good idea to do that cleanup now, before we
find on accident that we missed a gap and start trying to run
this bogus string as a hook invocation.

Good point.  In an earlier draft we were using that known
string as a bogus hook path to indicate that we should
call the IPC routines rather than the hook API.  But then
we added the `core.useBuiltinFSMonitor` boolean and had it
override all of the existing fsmonitor config settings.
So we don't technically need it to have a value now and can
and should stop using the pointer as a boolean.

Thanks!

quoted

-static int query_fsmonitor(int version, const char *last_update, struct strbuf *query_result)
+static int query_fsmonitor(int version, struct index_state *istate, struct strbuf *query_result)
  {
+	struct repository *r = istate->repo ? istate->repo : the_repository;
+	const char *last_update = istate->fsmonitor_last_update;
  	struct child_process cp = CHILD_PROCESS_INIT;
  	int result;
  
  	if (!core_fsmonitor)
  		return -1;

Here is an example of it being used as a boolean.

quoted

+	if (r->settings.use_builtin_fsmonitor > 0) {
+#ifdef HAVE_FSMONITOR_DAEMON_BACKEND
+		return fsmonitor_ipc__send_query(last_update, query_result);
+#else
+		/* Fake a trivial response. */
+		warning(_("fsmonitor--daemon unavailable; falling back"));
+		strbuf_add(query_result, "/", 2);
+		return 0;
+#endif

This seems like a case where the helper fsmonitor_ipc__is_supported()
could be used instead of compile-time macros.

(I think this is especially true when we consider the future of the
feature on Linux and the possibility of the same compiled code needing
to check run-time properties of the platform for compatibility.)

Yes.

quoted

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

Follows the patterns of repo settings. Good.

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

Jeff