Builtin FSMonitor Feature

From: Jeff Hostetler <[email protected]>

Create a manual page describing the git fsmonitor--daemon feature.

Update references to core.fsmonitor, core.fsmonitorHookVersion and pointers to watchman to mention the built-in FSMonitor.

core.fsmonitorHookVersion::

Documentation/config/core.txt +1 -9

-	Sets the version of hook that is to be used when calling fsmonitor.
-	There are currently versions 1 and 2. When this is not set,
-	version 2 will be tried first and if it fails then version 1
-	will be tried. Version 1 uses a timestamp as input to determine
-	which files have changes since that time but some monitors
-	like watchman have race conditions when used with a timestamp.
-	Version 2 uses an opaque string so that the monitor can return
-	something that can be used to determine what files have changed
-	without race conditions.
+	Sets the version of hook that is to be used when calling the

++

Documentation/config/core.txt +4 -0

+Note: FSMonitor hooks (and this config setting) are ignored if the
+built-in FSMonitor is enabled (see `core.useBuiltinFSMonitor`).
+
+core.useBuiltinFSMonitor::

Documentation/config/core.txt +3 -0

+Note: if this config setting is set to `true`, any FSMonitor hook
+configured via `core.fsmonitor` (and possibly `core.fsmonitorHookVersion`)
+is ignored.

Documentation/git-fsmonitor--daemon.txt new file +8 -0

+git-fsmonitor--daemon(1)
+========================
+
+NAME
+----
+git-fsmonitor--daemon - Builtin file system monitor daemon
+
+SYNOPSIS

Documentation/git-fsmonitor--daemon.txt new file +24 -0

+REMARKS
+-------
+The fsmonitor daemon is a long running process that will watch a single
+working directory.  Commands, such as `git status`, should automatically
+start it (if necessary) when `core.useBuiltinFSMonitor` is set to `true`
+(see linkgit:git-config[1]).
+
+Configure the built-in FSMonitor via `core.useBuiltinFSMonitor` in each
+working directory separately, or globally via `git config --global
+core.useBuiltinFSMonitor true`.
+
+Tokens are opaque strings.  They are used by the fsmonitor daemon to
+mark a point in time and the associated internal state.  Callers should
+make no assumptions about the content of the token.  In particular,
+the should not assume that it is a timestamp.
+
+Query commands send a request-token to the daemon and it responds with
+a summary of the changes that have occurred since that token was
+created.  The daemon also returns a response-token that the client can
+use in a future query.
+
+For more information see the "File System Monitor" section in
+linkgit:git-update-index[1].
+

Documentation/git-update-index.txt +3 -1

@@ -498,7 +498,9 @@ FILE SYSTEM MONITOR
 This feature is intended to speed up git operations for repos that have
 large working directories.
 
-It enables git to work together with a file system monitor (see the
+It enables git to work together with a file system monitor (see
+linkgit:git-fsmonitor--daemon[1]
+and the
 "fsmonitor-watchman" section of linkgit:githooks[5]) that can
 inform it as to what files have been modified. This enables git to avoid
 having to lstat() every file to find modified files.

Documentation/githooks.txt +0 -0

index b51959ff9418..b7d5e926f7b0 100644

Quoted 6 lines · Jeff Hostetler via GitGitGadget

From: Jeff Hostetler <[email protected]>

Create a manual page describing the git fsmonitor--daemon feature.

Update references to core.fsmonitor, core.fsmonitorHookVersion and pointers to watchman to mention the built-in FSMonitor.

Make sense to add clarity here, since there will be new ways to interact with a fileystem monitor.

Quoted 11 lines

core.fsmonitorHookVersion::

Documentation/config/core.txt +1 -9

-	Sets the version of hook that is to be used when calling fsmonitor.
-	There are currently versions 1 and 2. When this is not set,
-	version 2 will be tried first and if it fails then version 1
-	will be tried. Version 1 uses a timestamp as input to determine
-	which files have changes since that time but some monitors
-	like watchman have race conditions when used with a timestamp.
-	Version 2 uses an opaque string so that the monitor can return
-	something that can be used to determine what files have changed
-	without race conditions.
+	Sets the version of hook that is to be used when calling the

Documentation/config/core.txt core.fsmonitor:: · line 86

Quoted 10 lines

Documentation/config/core.txt +10 -0

+	FSMonitor hook (as configured via `core.fsmonitor`).
++
+There are currently versions 1 and 2. When this is not set,
+version 2 will be tried first and if it fails then version 1
+will be tried. Version 1 uses a timestamp as input to determine
+which files have changes since that time but some monitors
+like watchman have race conditions when used with a timestamp.
+Version 2 uses an opaque string so that the monitor can return
+something that can be used to determine what files have changed
+without race conditions.

This initially seemed like a big edit, but you just split the single paragraph into multiple, with a better leading sentence and a final statement about the built-in FSMonitor. Good.

Quoted 12 lines

++

Documentation/config/core.txt +11 -0

+Note: FSMonitor hooks (and this config setting) are ignored if the
+built-in FSMonitor is enabled (see `core.useBuiltinFSMonitor`).
+
+core.useBuiltinFSMonitor::
+	If set to true, enable the built-in filesystem event watcher (for
+	technical details, see linkgit:git-fsmonitor--daemon[1]).
++
+Like external (hook-based) FSMonitors, the built-in FSMonitor can speed up
+Git commands that need to refresh the Git index (e.g. `git status`) in a
+worktree with many files. The built-in FSMonitor facility eliminates the
+need to install and maintain an external third-party monitoring tool.

Documentation/config/core.txt core.fsmonitor:: · line 101

Quoted 3 lines

++

Documentation/config/core.txt +2 -0

+The built-in FSMonitor is currently available only on a limited set of
+supported platforms.

Is there a way for users to know this set of platforms? Can they run a command to find out? Will 'git fsmonitor--daemon --start' send a helpful message to assist here? Or, could there be a 'git fsmonitor--daemon --test' command?

Quoted 4 lines

Documentation/config/core.txt +3 -0

+Note: if this config setting is set to `true`, any FSMonitor hook
+configured via `core.fsmonitor` (and possibly `core.fsmonitorHookVersion`)
+is ignored.

...

Quoted 15 lines

Documentation/git-fsmonitor--daemon.txt new file +15 -0

+git-fsmonitor--daemon(1)
+========================
+
+NAME
+----
+git-fsmonitor--daemon - Builtin file system monitor daemon
+
+SYNOPSIS
+--------
+[verse]
+'git fsmonitor--daemon' --start
+'git fsmonitor--daemon' --run
+'git fsmonitor--daemon' --stop
+'git fsmonitor--daemon' --is-running
+'git fsmonitor--daemon' --is-supported

Quoted 3 lines

+'git fsmonitor--daemon' --query <token>
+'git fsmonitor--daemon' --query-index
+'git fsmonitor--daemon' --flush

These arguments with the "--" prefix make it seem like they are options that could be grouped together, but you really want these to be verbs within the daemon. What do you think about removing the "--" prefixes?

Quoted 4 lines

Documentation/git-fsmonitor--daemon.txt new file +3 -0

+
+DESCRIPTION
+-----------

Documentation/git-fsmonitor--daemon.txt line 31

Quoted 10 lines

Documentation/git-fsmonitor--daemon.txt new file +10 -0

+
+Monitors files and directories in the working directory for changes using
+platform-specific file system notification facilities.
+
+It communicates directly with commands like `git status` using the
+link:technical/api-simple-ipc.html[simple IPC] interface instead of
+the slower linkgit:githooks[5] interface.
+
+OPTIONS
+-------

I typically view "OPTIONS" as arguments that can be grouped together, but you are describing things more like verbs or subcommands. The most recent example I know about is 'git maintenance <subcommand>', documented at [1].

[1] https://git-scm.com/docs/git-maintenance#_subcommands

Documentation/git-fsmonitor--daemon.txt line 41

Quoted 11 lines

Documentation/git-fsmonitor--daemon.txt new file +10 -0

+
+--start::
+	Starts the fsmonitor daemon in the background.
+
+--run::
+	Runs the fsmonitor daemon in the foreground.
+
+--stop::
+	Stops the fsmonitor daemon running for the current working
+	directory, if present.

I'm noticing "fsmonitor" in lowercase throughout this document. Is that the intended case for user-facing documentation? I've been seeing "FS Monitor", "filesystem monitor", or even "File System Monitor" in other places.

Documentation/git-fsmonitor--daemon.txt line 45

Quoted 4 lines

+--is-running::

Documentation/git-fsmonitor--daemon.txt new file +2 -0

+	Exits with zero status if the fsmonitor daemon is watching the
+	current working directory.

Another potential name for this verb is "status".

Documentation/git-fsmonitor--daemon.txt line 49

Quoted 3 lines

+--is-supported::

Documentation/git-fsmonitor--daemon.txt new file +2 -0

+	Exits with zero status if the fsmonitor daemon feature is supported
+	on this platform.

Ah, here is an indicator of whether the platform is supported. Please include details for this command in the earlier documentation. I'll check later to see if a message is also sent over 'stderr', which would be helpful. Documenting the exit status is good for third-party tools that might use this.

Documentation/git-fsmonitor--daemon.txt line 60

Quoted 10 lines

+--query <token>::

Documentation/git-fsmonitor--daemon.txt new file +9 -0

+	Connects to the fsmonitor daemon (starting it if necessary) and
+	requests the list of changed files and directories since the
+	given token.
+	This is intended for testing purposes.
+
+--query-index::
+	Read the current `<token>` from the File System Monitor index
+	extension (if present) and use it to query the fsmonitor daemon.
+	This is intended for testing purposes.

These two could be grouped as "query [--token=X|--index]", especially because they are for testing purposes.

Documentation/git-fsmonitor--daemon.txt line 65

Quoted 5 lines

Documentation/git-fsmonitor--daemon.txt new file +5 -0

+
+--flush::
+	Force the fsmonitor daemon to flush its in-memory cache and
+	re-sync with the file system.
+	This is intended for testing purposes.

Do you see benefits to these being available in the CLI? Could these be better served as a test helper?

Quoted 31 lines

Documentation/git-fsmonitor--daemon.txt new file +30 -0

+REMARKS
+-------
+The fsmonitor daemon is a long running process that will watch a single
+working directory.  Commands, such as `git status`, should automatically
+start it (if necessary) when `core.useBuiltinFSMonitor` is set to `true`
+(see linkgit:git-config[1]).
+
+Configure the built-in FSMonitor via `core.useBuiltinFSMonitor` in each
+working directory separately, or globally via `git config --global
+core.useBuiltinFSMonitor true`.
+
+Tokens are opaque strings.  They are used by the fsmonitor daemon to
+mark a point in time and the associated internal state.  Callers should
+make no assumptions about the content of the token.  In particular,
+the should not assume that it is a timestamp.
+
+Query commands send a request-token to the daemon and it responds with
+a summary of the changes that have occurred since that token was
+created.  The daemon also returns a response-token that the client can
+use in a future query.
+
+For more information see the "File System Monitor" section in
+linkgit:git-update-index[1].
+
+CAVEATS
+-------
+
+The fsmonitor daemon does not currently know about submodules and does
+not know to filter out file system events that happen within a
+submodule.  If fsmonitor daemon is watching a super repo and a file is

Documentation/git-fsmonitor--daemon.txt line 100

Quoted 4 lines

Documentation/git-fsmonitor--daemon.txt new file +4 -0

+modified within the working directory of a submodule, it will report
+the change (as happening against the super repo).  However, the client
+should properly ignore these extra events, so performance may be affected
+but it should not cause an incorrect result.

There are several uses of the word "should" where I think "will" is a more appropriate word. That is, unless we do not actually have confidence in this behavior.

Quoted 19 lines

Documentation/git-update-index.txt +3 -1

@@ -498,7 +498,9 @@ FILE SYSTEM MONITOR
  This feature is intended to speed up git operations for repos that have
  large working directories.
 
-It enables git to work together with a file system monitor (see the
+It enables git to work together with a file system monitor (see
+linkgit:git-fsmonitor--daemon[1]
+and the
  "fsmonitor-watchman" section of linkgit:githooks[5]) that can
  inform it as to what files have been modified. This enables git to avoid
  having to lstat() every file to find modified files.

Documentation/githooks.txt +0 -0

index b51959ff9418..b7d5e926f7b0 100644

Documentation/githooks.txt fsmonitor-watchman · line 600

Quoted 9 lines

Documentation/githooks.txt +2 -1

@@ -593,7 +593,8 @@ fsmonitor-watchman
 
  This hook is invoked when the configuration option `core.fsmonitor` is
  set to `.git/hooks/fsmonitor-watchman` or `.git/hooks/fsmonitor-watchmanv2`
-depending on the version of the hook to use.
+depending on the version of the hook to use, unless overridden via
+`core.useBuiltinFSMonitor` (see linkgit:git-config[1]).
 
  Version 1 takes two arguments, a version (1) and the time in elapsed
  nanoseconds since midnight, January 1, 1970.

These are good connections to make.

Since the documentation for the fsmonitor--daemon is so deep, this patch might be served well to split into two: one that just documents the daemon, and another that updates existing documentation to point to the new file.

This does provide a good basis for me to investigate during the rest of the review.

Thanks,
-Stolee

fsmonitor-ipc.c new file +4 -0

+#define FSMONITOR_DAEMON_IS_SUPPORTED 1
+#else
+#define FSMONITOR_DAEMON_IS_SUPPORTED 0
+#endif

fsmonitor-ipc.c new file +47 -0

+#ifdef HAVE_FSMONITOR_DAEMON_BACKEND
+
+GIT_PATH_FUNC(fsmonitor_ipc__get_path, "fsmonitor")
+
+enum ipc_active_state fsmonitor_ipc__get_state(void)
+{
+	return ipc_get_active_state(fsmonitor_ipc__get_path());
+}
+
+static int spawn_daemon(void)
+{
+	const char *args[] = { "fsmonitor--daemon", "--start", NULL };
+
+	return run_command_v_opt_tr2(args, RUN_COMMAND_NO_STDIN | RUN_GIT_CMD,
+				    "fsmonitor");
+}
+
+int fsmonitor_ipc__send_query(const char *since_token,
+			      struct strbuf *answer)
+{
+	int ret = -1;
+	int tried_to_spawn = 0;
+	enum ipc_active_state state = IPC_STATE__OTHER_ERROR;
+	struct ipc_client_connection *connection = NULL;
+	struct ipc_client_connect_options options
+		= IPC_CLIENT_CONNECT_OPTIONS_INIT;
+
+	options.wait_if_busy = 1;
+	options.wait_if_not_found = 0;
+
+	trace2_region_enter("fsm_client", "query", NULL);
+
+	trace2_data_string("fsm_client", NULL, "query/command",
+			   since_token);
+
+try_again:
+	state = ipc_client_try_connect(fsmonitor_ipc__get_path(), &options,
+				       &connection);
+
+	switch (state) {
+	case IPC_STATE__LISTENING:
+		ret = ipc_client_send_command_to_connection(
+			connection, since_token, answer);
+		ipc_client_close_connection(connection);
+
+		trace2_data_intmax("fsm_client", NULL,
+				   "query/response-length", answer->len);

fsmonitor-ipc.c new file +41 -0

+
+	case IPC_STATE__INVALID_PATH:
+		ret = error(_("fsmonitor_ipc__send_query: invalid path '%s'"),
+			    fsmonitor_ipc__get_path());
+		goto done;
+
+	case IPC_STATE__OTHER_ERROR:
+	default:
+		ret = error(_("fsmonitor_ipc__send_query: unspecified error on '%s'"),
+			    fsmonitor_ipc__get_path());
+		goto done;
+	}
+
+done:
+	trace2_region_leave("fsm_client", "query", NULL);
+
+	return ret;
+}
+
+int fsmonitor_ipc__send_command(const char *command,
+				struct strbuf *answer)
+{
+	struct ipc_client_connection *connection = NULL;
+	struct ipc_client_connect_options options
+		= IPC_CLIENT_CONNECT_OPTIONS_INIT;
+	int ret;
+	enum ipc_active_state state;
+
+	strbuf_reset(answer);
+
+	options.wait_if_busy = 1;
+	options.wait_if_not_found = 0;
+
+	state = ipc_client_try_connect(fsmonitor_ipc__get_path(), &options,
+				       &connection);
+	if (state != IPC_STATE__LISTENING) {
+		die("fsmonitor--daemon is not running");
+		return -1;
+	}
+
+	ret = ipc_client_send_command_to_connection(connection, command, answer);

fsmonitor-ipc.c line 22

Quoted 10 lines · Jeff Hostetler via GitGitGadget

fsmonitor-ipc.c new file +10 -0

+#ifdef HAVE_FSMONITOR_DAEMON_BACKEND
+#define FSMONITOR_DAEMON_IS_SUPPORTED 1
+#else
+#define FSMONITOR_DAEMON_IS_SUPPORTED 0
+#endif
+
+int fsmonitor_ipc__is_supported(void)
+{
+     return FSMONITOR_DAEMON_IS_SUPPORTED;
+}

I don't see any other use of FSMONITOR_DAEMON_IS_SUPPORTED, so I was thinking you could use the #ifdef/#else/#endif construct within the implementation of this method instead of creating a macro outside. But my suggestion might be an anti-pattern, so feel free to ignore me.

Quoted 17 lines · Derrick Stolee

fsmonitor-ipc.c line 22

Quoted 10 lines · Jeff Hostetler via GitGitGadget

fsmonitor-ipc.c new file +10 -0

+#ifdef HAVE_FSMONITOR_DAEMON_BACKEND
+#define FSMONITOR_DAEMON_IS_SUPPORTED 1
+#else
+#define FSMONITOR_DAEMON_IS_SUPPORTED 0
+#endif
+
+int fsmonitor_ipc__is_supported(void)
+{
+     return FSMONITOR_DAEMON_IS_SUPPORTED;
+}

I don't see any other use of FSMONITOR_DAEMON_IS_SUPPORTED, so I was thinking you could use the #ifdef/#else/#endif construct within the implementation of this method instead of creating a macro outside. But my suggestion might be an anti-pattern, so feel free to ignore me.

On this project, it is preferred to keep the #if / #else / #endif outside of functions since embedding them within functions often makes it difficult to follow how the code flows (and generally makes functions unnecessarily noisy). So, the way Jeff did this seems fine.

An alternative would have been:

  #ifdef HAVE_FSMONITOR_DAEMON_BACKEND
  #define fsmonitor_ipc__is_supported() 1
  #else
  #define fsmonitor_ipc__is_supported() 0
  #endif

which would still allow calling it as a function:

    if (fsmonitor_ipc__is_supported())
        ...

but it's subjective whether that's actually any cleaner or better.

Quoted 13 lines · Jeff Hostetler via GitGitGadget

fsmonitor-ipc.c new file +13 -0

+#ifdef HAVE_FSMONITOR_DAEMON_BACKEND
+#define FSMONITOR_DAEMON_IS_SUPPORTED 1
+#else
+#define FSMONITOR_DAEMON_IS_SUPPORTED 0
+#endif
+
+/*
+ * A trivial function so that this source file always defines at least
+ * one symbol even when the feature is not supported.  This quiets an
+ * annoying compiler error.
+ */
+int fsmonitor_ipc__is_supported(void)
+{

Review

Quoted 2 lines

(quoted diff) +2 -0

+	return FSMONITOR_DAEMON_IS_SUPPORTED;
+}

I don't see any other use of FSMONITOR_DAEMON_IS_SUPPORTED, so I was thinking you could use the #ifdef/#else/#endif construct within the implementation of this method instead of creating a macro outside. But my suggestion might be an anti-pattern, so feel free to ignore me.

Quoted 47 lines

fsmonitor-ipc.c new file +47 -0

+#ifdef HAVE_FSMONITOR_DAEMON_BACKEND
+
+GIT_PATH_FUNC(fsmonitor_ipc__get_path, "fsmonitor")
+
+enum ipc_active_state fsmonitor_ipc__get_state(void)
+{
+	return ipc_get_active_state(fsmonitor_ipc__get_path());
+}
+
+static int spawn_daemon(void)
+{
+	const char *args[] = { "fsmonitor--daemon", "--start", NULL };
+
+	return run_command_v_opt_tr2(args, RUN_COMMAND_NO_STDIN | RUN_GIT_CMD,
+				    "fsmonitor");
+}
+
+int fsmonitor_ipc__send_query(const char *since_token,
+			      struct strbuf *answer)
+{
+	int ret = -1;
+	int tried_to_spawn = 0;
+	enum ipc_active_state state = IPC_STATE__OTHER_ERROR;
+	struct ipc_client_connection *connection = NULL;
+	struct ipc_client_connect_options options
+		= IPC_CLIENT_CONNECT_OPTIONS_INIT;
+
+	options.wait_if_busy = 1;
+	options.wait_if_not_found = 0;
+
+	trace2_region_enter("fsm_client", "query", NULL);
+
+	trace2_data_string("fsm_client", NULL, "query/command",
+			   since_token);
+
+try_again:
+	state = ipc_client_try_connect(fsmonitor_ipc__get_path(), &options,
+				       &connection);
+
+	switch (state) {
+	case IPC_STATE__LISTENING:
+		ret = ipc_client_send_command_to_connection(
+			connection, since_token, answer);
+		ipc_client_close_connection(connection);
+
+		trace2_data_intmax("fsm_client", NULL,
+				   "query/response-length", answer->len);

fsmonitor-ipc.c line 80

Quoted 10 lines

fsmonitor-ipc.c new file +10 -0

+
+		if (fsmonitor_is_trivial_response(answer))
+			trace2_data_intmax("fsm_client", NULL,
+					   "query/trivial-response", 1);
+
+		goto done;
+
+	case IPC_STATE__NOT_LISTENING:
+		ret = error(_("fsmonitor_ipc__send_query: daemon not available"));
+		goto done;

I'll need to read up on the IPC layer a bit to find out the difference between IPC_STATE__NOT_LISTENING and IPC_STATE__PATH_NOT_FOUND. When testing on my macOS machine, I got this error. I was expecting the daemon to be spawned. After spawning it myself, it started working.

I expect that there are some cases where the process can fail and the named pipe is not cleaned up. Let's investigate that soon. I should make it clear that I had tested the builtin FS Monitor on this machine a few weeks ago, but hadn't been using it much since. We should auto- recover from this situation.

But also: what is the cost of treating these two cases the same? Could we attempt to "restart" the daemon by spawning a new one? Will the new one find a way to kill a stale one?

(Reading on.)

fsmonitor-ipc.c line 88

Quoted 7 lines

fsmonitor-ipc.c new file +7 -0

+	case IPC_STATE__PATH_NOT_FOUND:
+		if (tried_to_spawn)
+			goto done;
+
+		tried_to_spawn++;
+		if (spawn_daemon())
+			goto done;

This should return zero on success, OK.

fsmonitor-ipc.c line 100

Quoted 11 lines

fsmonitor-ipc.c new file +11 -0

+		/*
+		 * Try again, but this time give the daemon a chance to
+		 * actually create the pipe/socket.
+		 *
+		 * Granted, the daemon just started so it can't possibly have
+		 * any FS cached yet, so we'll always get a trivial answer.
+		 * BUT the answer should include a new token that can serve
+		 * as the basis for subsequent requests.
+		 */
+		options.wait_if_not_found = 1;
+		goto try_again;

Because of the tried_to_spawn check, we will re-run the request over IPC but will not retry the spawn_daemon() request. I'm unsure how this could be helpful: is it possible that spawn_daemon() returns a non-zero error code after starting the daemon and somehow that daemon starts working? Or, is this a race-condition thing with parallel processes also starting up the daemon? It could be good to use this comment to describe why a retry might be helpful.

Quoted 42 lines

fsmonitor-ipc.c new file +41 -0

+
+	case IPC_STATE__INVALID_PATH:
+		ret = error(_("fsmonitor_ipc__send_query: invalid path '%s'"),
+			    fsmonitor_ipc__get_path());
+		goto done;
+
+	case IPC_STATE__OTHER_ERROR:
+	default:
+		ret = error(_("fsmonitor_ipc__send_query: unspecified error on '%s'"),
+			    fsmonitor_ipc__get_path());
+		goto done;
+	}
+
+done:
+	trace2_region_leave("fsm_client", "query", NULL);
+
+	return ret;
+}
+
+int fsmonitor_ipc__send_command(const char *command,
+				struct strbuf *answer)
+{
+	struct ipc_client_connection *connection = NULL;
+	struct ipc_client_connect_options options
+		= IPC_CLIENT_CONNECT_OPTIONS_INIT;
+	int ret;
+	enum ipc_active_state state;
+
+	strbuf_reset(answer);
+
+	options.wait_if_busy = 1;
+	options.wait_if_not_found = 0;
+
+	state = ipc_client_try_connect(fsmonitor_ipc__get_path(), &options,
+				       &connection);
+	if (state != IPC_STATE__LISTENING) {
+		die("fsmonitor--daemon is not running");
+		return -1;
+	}
+
+	ret = ipc_client_send_command_to_connection(connection, command, answer);

fsmonitor-ipc.c line 151

Quoted 10 lines

fsmonitor-ipc.c new file +10 -0

+	ipc_client_close_connection(connection);
+
+	if (ret == -1) {
+		die("could not send '%s' command to fsmonitor--daemon",
+		    command);
+		return -1;
+	}
+
+	return 0;
+}

I wondier if this ...send_command() method is too generic. It might be nice to have more structure to its inputs and outputs to lessen the cognitive load when plugging into other portions of the code. However, I'll wait to see what those consumers look like in case the generality is merited.

help.c void get_version_info(struct strbuf *buf, int show_build_options) · line 670

Quoted 9 lines

  struct category_description {
  	uint32_t category;

help.c +3 -0

@@ -664,6 +665,9 @@ void get_version_info(struct strbuf *buf, int show_build_options)
  		strbuf_addf(buf, "sizeof-size_t: %d\n", (int)sizeof(size_t));
  		strbuf_addf(buf, "shell-path: %s\n", SHELL_PATH);
  		/* NEEDSWORK: also save and output GIT-BUILD_OPTIONS? */
+
+		if (fsmonitor_ipc__is_supported())
+			strbuf_addstr(buf, "feature: fsmonitor--daemon\n");

This change might deserve its own patch, including some documentation about how users can use 'git version --build-options' to determine if the builtin FS Monitor feature is available on their platform.

Thanks,
-Stolee

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)

Review

Quoted 4 lines

int repo_config_get_fsmonitor(struct repository *r) {

(quoted diff) +1 -0

+	if (r->settings.use_builtin_fsmonitor > 0) {

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

config.c int git_config_get_max_percent_split_change(void) · line 2522

Quoted 4 lines

config.c +4 -0

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

fsmonitor.c void write_fsmonitor_extension(struct strbuf *sb, struct index_state *istate) · line 160

Quoted 10 lines

fsmonitor.c +3 -1

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

fsmonitor.c void write_fsmonitor_extension(struct strbuf *sb, struct index_state *istate) · line 170

Quoted 9 lines

fsmonitor.c +9 -0

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

Quoted 6 lines

repo-settings.c +0 -0

@@ -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);
 

repo-settings.c void prepare_repo_settings(struct repository *r) · line 63

Quoted 3 lines

repo-settings.c +3 -0

+	if (!repo_config_get_bool(r, "core.usebuiltinfsmonitor", &value) && value)
+		r->settings.use_builtin_fsmonitor = 1;
+

Follows the patterns of repo settings. Good.

Quoted 1 line · Derrick Stolee

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

repo-settings.c void prepare_repo_settings(struct repository *r) · line 63

Quoted 9 lines

repo-settings.c +3 -0

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

It follows the pattern, but as an aside the pattern seems bit odd. I see it dates back to your 7211b9e7534 (repo-settings: consolidate some config settings, 2019-08-13).

I.e. we memset() the whole thing to -1, then for most things do something like:

    if (!repo_config_get_bool(r, "gc.writecommitgraph", &value))
        r->settings.gc_write_commit_graph = value;
    UPDATE_DEFAULT_BOOL(r->settings.gc_write_commit_graph, 1);

But could do:

    if (repo_config_get_bool(r, "gc.writecommitgraph", &r->settings.gc_write_commit_graph))
        r->settings.gc_write_commit_graph = 1;

No? I.e. the repo_config_get_bool() function already returns non-zero if we don't find it in the config.

I see the UPDATE_DEFAULT_BOOL() macro has also drifted from "set thing default boolean" to "set any default value".

Quoted 4 lines · Ævar Arnfjörð Bjarmason

Quoted 1 line · Derrick Stolee

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

Quoted 11 lines

repo-settings.c void prepare_repo_settings(struct repository *r) · line 63

Quoted 9 lines

repo-settings.c +3 -0

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

It follows the pattern, but as an aside the pattern seems bit odd. I see it dates back to your 7211b9e7534 (repo-settings: consolidate some config settings, 2019-08-13).

I.e. we memset() the whole thing to -1, then for most things do something like:

    if (!repo_config_get_bool(r, "gc.writecommitgraph", &value))
        r->settings.gc_write_commit_graph = value;
    UPDATE_DEFAULT_BOOL(r->settings.gc_write_commit_graph, 1);

But could do:

Quoted 5 lines

    if (repo_config_get_bool(r, "gc.writecommitgraph", &r->settings.gc_write_commit_graph))
        r->settings.gc_write_commit_graph = 1;

No? I.e. the repo_config_get_bool() function already returns non-zero if we don't find it in the config.

I see how this is fewer lines of code, but it is harder to read the intent of the implementation. The current [...]

layout makes it clear that we set the value from the config, if it exists, but otherwise we choose a default.

Sometimes, this choice of a default _needs_ to be deferred, for example with the fetch_negotiation_algorithm setting, which can be set both from the fetch.negotiationAlgorithm config, but also the feature.experimental config.

However, perhaps it would be better still for these one-off requests to create a new macro, say USE_CONFIG_OR_DEFAULT_BOOL() that fills a value from config _or_ sets the given default:

#define USE_CONFIG_OR_DEFAULT_BOOL(r, v, s, d) \
	if (repo_config_get_bool(r, s, &v)) \
		v = d

And then for this example we would write

	USE_CONFIG_OR_DEFAULT_BOOL(r, r->settings.core_commit_graph,
				   "core.commitgraph", 1);

This would work for multiple config options in this file.

Quoted 2 lines

I see the UPDATE_DEFAULT_BOOL() macro has also drifted from "set thing default boolean" to "set any default value".

This is correct. I suppose it would be a good change to make some time. Such a rename could be combined with the refactor above.

I would recommend waiting until such a change isn't conflicting with ongoing topics, such as this one.

Quoted 5 lines · Derrick Stolee

This is correct. I suppose it would be a good change to make some time. Such a rename could be combined with the refactor above.

I would recommend waiting until such a change isn't conflicting with ongoing topics, such as this one.

I'm not planning to work on it, but thought I'd ask/prod the original author if they were interested :)

Simplify the setup code in repo-settings.c in various ways, making the code shorter, easier to read, and requiring fewer hacks to do the same thing as it did before:

Quoted 3 lines · Ævar Arnfjörð Bjarmason

Simplify the setup code in repo-settings.c in various ways, making the code shorter, easier to read, and requiring fewer hacks to do the same thing as it did before:

This patch is interesting, and I'll review it when I have some more time. Probably tomorrow.

But I thought that I would point out that this pattern of adding a patch within the thread of a larger series makes it very difficult to separate the two. I use an email client that groups messages by thread in order to help parse meaningful discussion from the list which otherwise looks like a fire hose of noise. Now, this patch is linked to the FS Monitor thread and feedback to either will trigger the thread as having unread messages.

I find it very difficult to track multiple patch series that are being juggled in the same thread. It is mentally taxing enough that I have avoided reviewing code presented this way to save myself the effort of tracking which patches go with what topic in what order.

Since I've committed to reviewing the FS Monitor code, I'd prefer if this patch (or maybe its v2, since this is here already) be sent as a top-level message so it can be discussed independently.

Quoted 9 lines · Derrick Stolee

Quoted 3 lines · Ævar Arnfjörð Bjarmason

Simplify the setup code in repo-settings.c in various ways, making the code shorter, easier to read, and requiring fewer hacks to do the same thing as it did before:

[...]
Since I've committed to reviewing the FS Monitor code, I'd prefer if this patch (or maybe its v2, since this is here already) be sent as a top-level message so it can be discussed independently.

As a practical matter I think any effort I make to accommodate your request will be dwarfed by your own starting of a sub-thread on E-Mail/MUA nuances :)

When [1] was brought up the other day (showing that I'm probably not the best person to ask about on-list In-Reply-To semantics) I was surprised to find that we don't have much (if any) explicit documentation about In-Reply-To best practices. [...]

1. https://lore.kernel.org/git/[email protected]/

Quoted 3 lines · Ævar Arnfjörð Bjarmason

Simplify the setup code in repo-settings.c in various ways, making the code shorter, easier to read, and requiring fewer hacks to do the same thing as it did before:

This patch is interesting, and I'll review it when I have some more time. Probably tomorrow.

But I thought that I would point out that this pattern of adding a patch within the thread of a larger series makes it very difficult to separate the two. I use an email client that groups messages by thread in order to help parse meaningful discussion from the list which otherwise looks like a fire hose of noise. Now, this patch is linked to the FS Monitor thread and feedback to either will trigger the thread as having unread messages.

I find it very difficult to track multiple patch series that are being juggled in the same thread. It is mentally taxing enough that I have avoided reviewing code presented this way to save myself the effort of tracking which patches go with what topic in what order.

Quoted 16 lines · Derrick Stolee

Quoted 3 lines · Ævar Arnfjörð Bjarmason

Simplify the setup code in repo-settings.c in various ways, making the code shorter, easier to read, and requiring fewer hacks to do the same thing as it did before:

This patch is interesting, and I'll review it when I have some more time. Probably tomorrow.

But I thought that I would point out that this pattern of adding a patch within the thread of a larger series makes it very difficult to separate the two. I use an email client that groups messages by thread in order to help parse meaningful discussion from the list which otherwise looks like a fire hose of noise. Now, this patch is linked to the FS Monitor thread and feedback to either will trigger the thread as having unread messages.

Quoted 4 lines

I find it very difficult to track multiple patch series that are being juggled in the same thread. It is mentally taxing enough that I have avoided reviewing code presented this way to save myself the effort of tracking which patches go with what topic in what order.

I do find it distracting to have a full "ah, I just thought of something while discussing this unrelated series" patch fairly irritating for the same reason. It however is unavoidable human nature that we come up with ideas while thinking about something not necessarily related. So it largely is a presentation issue.

I really appreciate the way some people (Peff is a stellar example, but there are others who are as good at this) handle these tangents, where the message sent to an existing thread is limited to only give an outline of the idea (possibly with "something like this?" patch for illustration) and then they quickly get out of the way of the discussion by starting a separate thread, while back-referencing "So here is a proper patch based on the idea I interjected in the discussion of that other topic." And the discussion on the tangent will be done on its own thread.

Quoted 9 lines

I really appreciate the way some people (Peff is a stellar example, but there are others who are as good at this) handle these tangents, where the message sent to an existing thread is limited to only give an outline of the idea (possibly with "something like this?" patch for illustration) and then they quickly get out of the way of the discussion by starting a separate thread, while back-referencing "So here is a proper patch based on the idea I interjected in the discussion of that other topic." And the discussion on the tangent will be done on its own thread.

In RFC 822 terms. Are you talking about the In-Reply-To[1] or References[2] headers, or both/neither?

Threading isn't a concept that exists in E-Mail protocols per-se. Just In-Reply-To and References. The References header can reference N messages most would think about as a separate "thread", and "thread" is ultimately some fuzzy MUA-specific concept on top of these (and others).

E.g. in my client right now I'm looking at just 4 messages in this "thread", it doesn't descend down the whole In-Reply-To, others would act differently.

It seems to me to be better to veer on the side of using standard fields for their intended purpose for archiving / future use. I.e. making "a reference" universally machine-readable, as opposed to a lore.kernel.org link, or a free-form "in a recent thread" blurb.

In RFC 822 terms. Are you talking about the In-Reply-To[1] or References[2] headers, or both/neither?

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)

Review

Quoted 4 lines

  
  int repo_config_get_fsmonitor(struct repository *r)
  {

(quoted diff) +1 -0

+	if (r->settings.use_builtin_fsmonitor > 0) {

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

config.c int git_config_get_max_percent_split_change(void) · line 2522

Quoted 4 lines

config.c +4 -0

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

fsmonitor.c void write_fsmonitor_extension(struct strbuf *sb, struct index_state *istate) · line 160

Quoted 10 lines

fsmonitor.c +3 -1

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

fsmonitor.c void write_fsmonitor_extension(struct strbuf *sb, struct index_state *istate) · line 170

Quoted 9 lines

fsmonitor.c +9 -0

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

Quoted 6 lines

repo-settings.c +0 -0

@@ -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);
 

repo-settings.c void prepare_repo_settings(struct repository *r) · line 63

Quoted 3 lines

repo-settings.c +3 -0

+	if (!repo_config_get_bool(r, "core.usebuiltinfsmonitor", &value) && value)
+		r->settings.use_builtin_fsmonitor = 1;
+

Follows the patterns of repo settings. Good.

On 4/1/21 11:40 AM, Jeff Hostetler via GitGitGadget wrote:> +#ifdef HAVE_FSMONITOR_DAEMON_BACKEND

I think these compile-time macros should be replaced with a method call, as I've said before. It should be simple to say

	if (!fsmonitor_ipc__is_supported())
		die(_("fsmonitor--daemon is not supported on this platform"));

and call it a day. This can be done before parsing arguments.

builtin/fsmonitor--daemon.c line 23

Quoted 9 lines

builtin/fsmonitor--daemon.c new file +9 -0

+int cmd_fsmonitor__daemon(int argc, const char **argv, const char *prefix)
+{
+	enum daemon_mode {
+		UNDEFINED_MODE,
+	} mode = UNDEFINED_MODE;
+
+	struct option options[] = {
+		OPT_END()
+	};

I can see where you are going here, to use the parse-opts API to get your "--<verb>" arguments to populate an 'enum'. However, it seems like you will run into the problem where a user enters multiple such arguments and you lose the information as the parser overwrites 'mode' here.

Better to use a positional argument and drop the "--" prefix, in my opinion.

Quoted 10 lines · Derrick Stolee

On 4/1/21 11:40 AM, Jeff Hostetler via GitGitGadget wrote:> +#ifdef HAVE_FSMONITOR_DAEMON_BACKEND

I think these compile-time macros should be replaced with a method call, as I've said before. It should be simple to say

	if (!fsmonitor_ipc__is_supported())
		die(_("fsmonitor--daemon is not supported on this platform"));

and call it a day. This can be done before parsing arguments.

Quoted 15 lines

builtin/fsmonitor--daemon.c line 23

Quoted 9 lines

builtin/fsmonitor--daemon.c new file +9 -0

+int cmd_fsmonitor__daemon(int argc, const char **argv, const char *prefix)
+{
+	enum daemon_mode {
+		UNDEFINED_MODE,
+	} mode = UNDEFINED_MODE;
+
+	struct option options[] = {
+		OPT_END()
+	};

I can see where you are going here, to use the parse-opts API to get your "--<verb>" arguments to populate an 'enum'. However, it seems like you will run into the problem where a user enters multiple such arguments and you lose the information as the parser overwrites 'mode' here.

I see that you use OPT_CMDMODE in your implementation, which makes this concern invalid.

Quoted 2 lines

Better to use a positional argument and drop the "--" prefix, in my opinion.

This is my personal taste, but the technical reason to do this doesn't exist.

From: Jeff Hostetler <[email protected]>

Implement command options --stop, --is-running, --query, --query-index, and --flush to control and query the status of a fsmonitor--daemon server process (and implicitly start a server process if necessary).

Later commits will implement the actual server and monitor the file system.

Quoted 9 lines · Jeff Hostetler via GitGitGadget

From: Jeff Hostetler <[email protected]>

Implement command options --stop, --is-running, --query, --query-index, and --flush to control and query the status of a fsmonitor--daemon server process (and implicitly start a server process if necessary).

Later commits will implement the actual server and monitor the file system.

As mentioned before, I think the "query", "query-index", and "flush" commands are better served in a test helper. Luckily, the implementation you give here seems rather straightforward and could fit into a test helper without a lot of duplicated boilerplate. That's a good sign for the API presented here.

As a bonus, you could delay the implementation of those test helpers until they are going to be used in a test.

Thanks,
-Stolee

From: Jeff Hostetler <[email protected]>

builtin/fsmonitor--daemon.c +3 -0

+
 static int is_ipc_daemon_listening(void)
 {
 	return fsmonitor_ipc__get_state() == IPC_STATE__LISTENING;
 }
 
+static int try_to_run_foreground_daemon(void)
+{

builtin/fsmonitor--daemon.c +13 -0

+
+/*
+ * This is adapted from `wait_or_whine()`.  Watch the child process and
+ * let it get started and begin listening for requests on the socket
+ * before reporting our success.
+ */
+static int wait_for_background_startup(pid_t pid_child)
+{
+	int status;
+	pid_t pid_seen;
+	enum ipc_active_state s;
+	time_t time_limit, now;
+

builtin/fsmonitor--daemon.c +3 -0

+		}
+
+		else if (pid_seen == pid_child) {

Quoted 1 line · Jeff Hostetler via GitGitGadget

From: Jeff Hostetler <[email protected]>

...

builtin/fsmonitor--daemon.c static int do_as_client__send_flush(void) · line 258

Quoted 4 lines

builtin/fsmonitor--daemon.c +4 -0

+	/* Prepare to (recursively) watch the <worktree-root> directory. */
+	strbuf_init(&state.path_worktree_watch, 0);
+	strbuf_addstr(&state.path_worktree_watch, absolute_path(get_git_work_tree()));
+	state.nr_paths_watching = 1;

Yes, let's watch the working directory.

Quoted 9 lines

builtin/fsmonitor--daemon.c +9 -0

+	/*
+	 * If ".git" is not a directory, then <gitdir> is not inside the
+	 * cone of <worktree-root>, so set up a second watch for it.
+	 */
+	strbuf_init(&state.path_gitdir_watch, 0);
+	strbuf_addbuf(&state.path_gitdir_watch, &state.path_worktree_watch);
+	strbuf_addstr(&state.path_gitdir_watch, "/.git");
+	if (!is_directory(state.path_gitdir_watch.buf)) {
+		strbuf_reset(&state.path_gitdir_watch);

builtin/fsmonitor--daemon.c static int do_as_client__send_flush(void) · line 271

Quoted 3 lines

builtin/fsmonitor--daemon.c +3 -0

+		strbuf_addstr(&state.path_gitdir_watch, absolute_path(get_git_dir()));
+		state.nr_paths_watching = 2;
+	}

But why watch the .git directory, especially for a worktree (or submodule I guess)? What benefit do we get from events within the .git directory? I'm expecting any event within the .git directory should be silently ignored.

Quoted 19 lines · Derrick Stolee

Quoted 1 line · Jeff Hostetler via GitGitGadget

From: Jeff Hostetler <[email protected]>

...

builtin/fsmonitor--daemon.c static int do_as_client__send_flush(void) · line 258

Quoted 4 lines

builtin/fsmonitor--daemon.c +4 -0

+	/* Prepare to (recursively) watch the <worktree-root> directory. */
+	strbuf_init(&state.path_worktree_watch, 0);
+	strbuf_addstr(&state.path_worktree_watch, absolute_path(get_git_work_tree()));
+	state.nr_paths_watching = 1;

Yes, let's watch the working directory.

Quoted 9 lines

builtin/fsmonitor--daemon.c +9 -0

+	/*
+	 * If ".git" is not a directory, then <gitdir> is not inside the
+	 * cone of <worktree-root>, so set up a second watch for it.
+	 */
+	strbuf_init(&state.path_gitdir_watch, 0);
+	strbuf_addbuf(&state.path_gitdir_watch, &state.path_worktree_watch);
+	strbuf_addstr(&state.path_gitdir_watch, "/.git");
+	if (!is_directory(state.path_gitdir_watch.buf)) {
+		strbuf_reset(&state.path_gitdir_watch);

Quoted 8 lines

builtin/fsmonitor--daemon.c static int do_as_client__send_flush(void) · line 271

Quoted 3 lines

builtin/fsmonitor--daemon.c +3 -0

+		strbuf_addstr(&state.path_gitdir_watch, absolute_path(get_git_dir()));
+		state.nr_paths_watching = 2;
+	}

But why watch the .git directory, especially for a worktree (or submodule I guess)? What benefit do we get from events within the .git directory? I'm expecting any event within the .git directory should be silently ignored.

I see in a following patch that we place a cookie file within the .git directory. I'm reminded that this is done for a reason: other filesystem watchers can get into a loop if we place the cookie file outside of the .git directory. The classic example is VS Code running 'git status' in a loop because Watchman writes a cookie into the root of the working directory.

Quoted 9 lines

builtin/fsmonitor--daemon.c +3 -0

+
  static int is_ipc_daemon_listening(void)
  {
  	return fsmonitor_ipc__get_state() == IPC_STATE__LISTENING;
  }
 
+static int try_to_run_foreground_daemon(void)
+{

builtin/fsmonitor--daemon.c static int do_as_client__send_flush(void) · line 312

Quoted 10 lines

builtin/fsmonitor--daemon.c +10 -0

+	/*
+	 * Technically, we don't need to probe for an existing daemon
+	 * process, since we could just call `fsmonitor_run_daemon()`
+	 * and let it fail if the pipe/socket is busy.
+	 *
+	 * However, this method gives us a nicer error message for a
+	 * common error case.
+	 */
+	if (is_ipc_daemon_listening())
+		die("fsmonitor--daemon is already running.");

Here, it seems like we only care about IPC_STATE_LISTENING, while earlier I mentioned that I ended up in IPC_STATE__NOT_LISTENING, and my manually running of the daemon helped.

Review

Quoted 2 lines

(quoted diff) +2 -0

+	return !!fsmonitor_run_daemon();
+}

You are ignoring the IPC_STATE__NOT_LISTENING and creating a new process, which is good. I'm just wondering why that state exists and what is the proper way to handle it?

Review

Quoted 3 lines

(quoted diff) +2 -0

+
+#ifndef GIT_WINDOWS_NATIVE

You are already creating a platform-specific mechanism for the filesystem watcher. Shouldn't the implementation of this method be part of that file in compat/fsmonitor/?

I guess the biggest reason is that macOS and Linux share this implementation, so maybe this is the cleanest approach.

From: Jeff Hostetler <[email protected]>

builtin/fsmonitor--daemon.c +7 -0

+#define FSMONITOR_COOKIE_PREFIX ".fsmonitor-daemon-"
+
+enum fsmonitor_path_type fsmonitor_classify_path_workdir_relative(
+	const char *rel)
+{
+	if (fspathncmp(rel, ".git", 4))
+		return IS_WORKDIR_PATH;

builtin/fsmonitor--daemon.c +38 -0

+static enum fsmonitor_path_type try_classify_workdir_abs_path(
+	struct fsmonitor_daemon_state *state,
+	const char *path)
+{
+	const char *rel;
+
+	if (fspathncmp(path, state->path_worktree_watch.buf,
+		       state->path_worktree_watch.len))
+		return IS_OUTSIDE_CONE;
+
+	rel = path + state->path_worktree_watch.len;
+
+	if (!*rel)
+		return IS_WORKDIR_PATH; /* it is the root dir exactly */
+	if (*rel != '/')
+		return IS_OUTSIDE_CONE;
+	rel++;
+
+	return fsmonitor_classify_path_workdir_relative(rel);
+}
+
+enum fsmonitor_path_type fsmonitor_classify_path_absolute(
+	struct fsmonitor_daemon_state *state,
+	const char *path)
+{
+	const char *rel;
+	enum fsmonitor_path_type t;
+
+	t = try_classify_workdir_abs_path(state, path);
+	if (state->nr_paths_watching == 1)
+		return t;
+	if (t != IS_OUTSIDE_CONE)
+		return t;
+
+	if (fspathncmp(path, state->path_gitdir_watch.buf,
+		       state->path_gitdir_watch.len))
+		return IS_OUTSIDE_CONE;
+