Builtin FSMonitor Feature

repo-settings.c +3 -0

index 0cfe8b787db..faf197ff60a 100644
@@ -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.
+ */

repo-settings.c +4 -0

+
+	/*
+	 * GVFS (aka VFS for Git) is incompatible with FSMonitor.
+	 *

Here is V3 of my patch series to add a builtin FSMonitor daemon to Git. I rebased this series onto v2.32.0.

V3 addresses most of the previous review comments and things we've learned from our experimental testing of V2. (A version of V2 was shipped as an experimental feature in the v2.32.0-based releases of Git for Windows and VFS for Git.)

There are still a few items that I need to address, but that list is getting very short.

fsmonitor-fs-listen-win32: stub in backend for Windows
fsmonitor-fs-listen-macos: stub in backend for MacOS

Quoted 10 lines · Jeff Hostetler via GitGitGadget

Here is V3 of my patch series to add a builtin FSMonitor daemon to Git. I rebased this series onto v2.32.0.

V3 addresses most of the previous review comments and things we've learned from our experimental testing of V2. (A version of V2 was shipped as an experimental feature in the v2.32.0-based releases of Git for Windows and VFS for Git.)

There are still a few items that I need to address, but that list is getting very short.

...

Quoted 2 lines

fsmonitor-fs-listen-win32: stub in backend for Windows
fsmonitor-fs-listen-macos: stub in backend for MacOS

I left some light comments on the repo-settings.c part of this to follow up from a previous round.

Any other testing of it is stalled by there being no linux backend for it as part of this series. I see from spelunking repos that Johannes had a WIP compat/fsmonitor/linux.c which looks like it could/should mostly work, but the API names all changed since then, and after a short try I gave up on trying to rebase it.

I'd really prefer for git not to have features that place free platforms at a disadvantage against proprietary platforms if it can be avoided, and in this case the lack of a Linux backend also means much less widespread testing of the feature among the development community / CI.

t/helper/test-touch.c new file +3 -0

new file mode 100644
index 00000000000..e9b3b754f1f
@@ -0,0 +1,126 @@
+/*
+ * test-touch.c: variation on /usr/bin/touch to speed up tests
+ * with a large number of files (primarily on Windows where child

Quoted 10 lines · Ævar Arnfjörð Bjarmason

Quoted 8 lines · Jeff Hostetler via GitGitGadget

Here is V3 of my patch series to add a builtin FSMonitor daemon to Git. I rebased this series onto v2.32.0.

V3 addresses most of the previous review comments and things we've learned from our experimental testing of V2. (A version of V2 was shipped as an experimental feature in the v2.32.0-based releases of Git for Windows and VFS for Git.)

Quoted 8 lines

Quoted 2 lines

There are still a few items that I need to address, but that list is getting very short.

...

Quoted 2 lines

fsmonitor-fs-listen-win32: stub in backend for Windows
fsmonitor-fs-listen-macos: stub in backend for MacOS

I left some light comments on the repo-settings.c part of this to follow
up from a previous round.

Thanks.

Quoted 6 lines

Any other testing of it is stalled by there being no linux backend for
it as part of this series. I see from spelunking repos that Johannes had a WIP compat/fsmonitor/linux.c which looks like it could/should mostly work, but the API names all changed since then, and after a short try I gave up on trying to rebase it.

The early Linux version was dropped because inotify does not give recursive coverage -- only the requested directory. Using inotify requires adding a watch to each subdirectory (recursively) in the worktree. There's a system limit on the maximum number of watched directories (defaults to 8K IIRC) and that limit is system-wide.

Since the whole point was to support large very large repos, using inotify was a non-starter, so I removed the Linux version from our patch series. For example, the first repo I tried it on (outside of the test suite) had 25K subdirectories.

I'm told there is a new fanotify API in recent Linux kernels that is a better fit for what we need, but we haven't had time to investigate it yet.

Quoted 6 lines

I'd really prefer for git not to have features that place free platforms
at a disadvantage against proprietary platforms if it can be avoided, and in this case the lack of a Linux backend also means much less widespread testing of the feature among the development community / CI.

This feature is always going to have platform-specific components, so the lack of one platform or another should not stop us from discussing it for the platforms that can be supported.

And given the size and complexity of the platform-specific code, we should not assume that "just test it on Linux" is sufficient. Yes, there are some common/shared routines/data structures in the daemon, but hard/tricky parts are in the platform layer.

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

+#ifdef GIT_WINDOWS_NATIVE
+/*
+ * Create a background process to run the daemon.  It should be completely
+ * disassociated from the terminal.
+ *
+ * Conceptually like `daemonize()` but different because Windows does not
+ * have `fork(2)`.  Spawn a normal Windows child process but without the
+ * limitations of `start_command()` and `finish_command()`.
+ *
+ * The child process execs the "git fsmonitor--daemon run" command.
+ *
+ * The current process returns so that the caller can wait for the child
+ * to startup before exiting.
+ */
+static int spawn_background_fsmonitor_daemon(pid_t *pid)
+{
+	char git_exe[MAX_PATH];
+	struct strvec args = STRVEC_INIT;
+	int in, out;
+
+	GetModuleFileNameA(NULL, git_exe, MAX_PATH);
+
+	in = open("/dev/null", O_RDONLY);
+	out = open("/dev/null", O_WRONLY);
+
+	strvec_push(&args, git_exe);
+	strvec_push(&args, "fsmonitor--daemon");
+	strvec_push(&args, "run");
+	strvec_pushf(&args, "--ipc-threads=%d", fsmonitor__ipc_threads);
+
+	*pid = mingw_spawnvpe(args.v[0], args.v, NULL, NULL, in, out, out);
+	close(in);
+	close(out);
+
+	strvec_clear(&args);
+
+	if (*pid < 0)
+		return error(_("could not spawn fsmonitor--daemon in the background"));
+
+	return 0;
+}
+#else
+/*
+ * Create a background process to run the daemon.  It should be completely
+ * disassociated from the terminal.
+ *
+ * This is adapted from `daemonize()`.  Use `fork()` to directly
+ * create and run the daemon in the child process.
+ *
+ * The fork-child can just call the run code; it does not need to exec
+ * it.
+ *
+ * The fork-parent returns the child PID so that we can wait for the
+ * child to startup before exiting.
+ */
+static int spawn_background_fsmonitor_daemon(pid_t *pid)
+{
+	*pid = fork();
+
+	switch (*pid) {
+	case 0:
+		if (setsid() == -1)
+			error_errno(_("setsid failed"));
+		close(0);
+		close(1);
+		close(2);
+		sanitize_stdfds();
+

builtin/fsmonitor--daemon.c +59 -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;
+
+	time(&time_limit);
+	time_limit += fsmonitor__start_timeout_sec;
+
+	for (;;) {
+		pid_seen = waitpid(pid_child, &status, WNOHANG);
+
+		if (pid_seen == -1)
+			return error_errno(_("waitpid failed"));
+		else if (pid_seen == 0) {
+			/*
+			 * The child is still running (this should be
+			 * the normal case).  Try to connect to it on
+			 * the socket and see if it is ready for
+			 * business.
+			 *
+			 * If there is another daemon already running,
+			 * our child will fail to start (possibly
+			 * after a timeout on the lock), but we don't
+			 * care (who responds) if the socket is live.
+			 */
+			s = fsmonitor_ipc__get_state();
+			if (s == IPC_STATE__LISTENING)
+				return 0;
+
+			time(&now);
+			if (now > time_limit)
+				return error(_("fsmonitor--daemon not online yet"));
+		} else if (pid_seen == pid_child) {
+			/*
+			 * The new child daemon process shutdown while
+			 * it was starting up, so it is not listening
+			 * on the socket.
+			 *
+			 * Try to ping the socket in the odd chance
+			 * that another daemon started (or was already
+			 * running) while our child was starting.
+			 *
+			 * Again, we don't care who services the socket.
+			 */
+			s = fsmonitor_ipc__get_state();
+			if (s == IPC_STATE__LISTENING)
+				return 0;
+
+			/*
+			 * We don't care about the WEXITSTATUS() nor
+			 * any of the WIF*(status) values because
+			 * `cmd_fsmonitor__daemon()` does the `!!result`

From: Jeff Hostetler <[email protected]>

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

Signed-off-by Jeff Hostetler <[email protected]>

---
 Documentation/git-fsmonitor--daemon.txt | 75 +++++++++++++++++++++++++
 1 file changed, 75 insertions(+)
 create mode 100644 Documentation/git-fsmonitor--daemon.txt

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

new file mode 100644
index 00000000000..154e7684daa
@@ -0,0 +1,75 @@
+git-fsmonitor--daemon(1)
+========================
+
+NAME
+----
+git-fsmonitor--daemon - A Built-in File System Monitor
+
+SYNOPSIS
+--------
+[verse]
+'git fsmonitor--daemon' start
+'git fsmonitor--daemon' run
+'git fsmonitor--daemon' stop
+'git fsmonitor--daemon' status
+
+DESCRIPTION
+-----------
+
+A daemon to watch the working directory for file and directory
+changes using platform-specific file system notification facilities.
+
+This daemon 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.
+
+This daemon is built into Git so that no third-party tools are
+required.
+
+OPTIONS
+-------
+
+start::
+	Starts a daemon in the background.
+
+run::
+	Runs a daemon in the foreground.
+
+stop::
+	Stops the daemon running in the current working
+	directory, if present.
+
+status::
+	Exits with zero status if a daemon is watching the
+	current working directory.
+
+REMARKS
+-------
+
+This daemon is a long running process used to watch a single working
+directory and maintain a list of the recently changed files and
+directories.  Performance of commands such as `git status` can be
+increased if they just ask for a summary of changes to the working
+directory and can avoid scanning the disk.
+
+When `core.useBuiltinFSMonitor` is set to `true` (see
+linkgit:git-config[1]) commands, such as `git status`, will ask the
+daemon for changes and automatically start it (if necessary).
+
+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

From: Jeff Hostetler <[email protected]>

Update references to core.fsmonitor and core.fsmonitorHookVersion and pointers to Watchman to mention the new built-in fsmonitor--daemon.

Signed-off-by Jeff Hostetler <[email protected]>

---
 Documentation/config/core.txt      | 56 ++++++++++++++++++++++--------
 Documentation/git-update-index.txt | 27 +++++++-------
 Documentation/githooks.txt         |  3 +-
 3 files changed, 59 insertions(+), 27 deletions(-)

Documentation/config/core.txt +42 -14

index c04f62a54a1..4f6e519bc02 100644
@@ -62,22 +62,50 @@ core.protectNTFS::
 	Defaults to `true` on Windows, and `false` elsewhere.
 
 core.fsmonitor::
-	If set, the value of this variable is used as a command which
-	will identify all files that may have changed since the
-	requested date/time. This information is used to speed up git by
-	avoiding unnecessary processing of files that have not changed.
-	See the "fsmonitor-watchman" section of linkgit:githooks[5].
+	If set, this variable contains the pathname of the "fsmonitor"
+	hook command.
++
+This hook command is used to identify all files that may have changed
+since the requested date/time. This information is used to speed up
+git by avoiding unnecessary scanning of files that have not changed.
++
+See the "fsmonitor-watchman" section of linkgit:githooks[5].
++
+Note: The value of this config setting is ignored if the
+built-in file system monitor is enabled (see `core.useBuiltinFSMonitor`).
 
 core.fsmonitorHookVersion::
-	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 protocol version to be used when invoking the
+	"fsmonitor" hook.
++
+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.
++
+Note: The value of this config setting is ignored if the
+built-in file system monitor is enabled (see `core.useBuiltinFSMonitor`).
+
+core.useBuiltinFSMonitor::
+	If set to true, enable the built-in file system monitor
+	daemon for this working directory (linkgit:git-fsmonitor--daemon[1]).
++
+Like hook-based file system monitors, the built-in file system monitor
+can speed up Git commands that need to refresh the Git index
+(e.g. `git status`) in a working directory with many files.  The
+built-in monitor eliminates the need to install and maintain an
+external third-party tool.
++
+The built-in file system monitor is currently available only on a
+limited set of supported platforms.  Currently, this includes Windows
+and MacOS.
++
+Note: if this config setting is set to `true`, the values of
+`core.fsmonitor` and `core.fsmonitorHookVersion` are ignored.
 
 core.trustctime::
 	If false, the ctime differences between the index and the

Documentation/git-update-index.txt +15 -12

index 2853f168d97..c7c31b3fcf9 100644
@@ -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.

@@ -508,17 +510,18 @@ performance by avoiding the cost of scanning the entire working directory
 looking for new files.
 
 If you want to enable (or disable) this feature, it is easier to use
-the `core.fsmonitor` configuration variable (see
-linkgit:git-config[1]) than using the `--fsmonitor` option to
-`git update-index` in each repository, especially if you want to do so
-across all repositories you use, because you can set the configuration
-variable in your `$HOME/.gitconfig` just once and have it affect all
-repositories you touch.
-
-When the `core.fsmonitor` configuration variable is changed, the
-file system monitor is added to or removed from the index the next time
-a command reads the index. When `--[no-]fsmonitor` are used, the file
-system monitor is immediately added to or removed from the index.
+the `core.fsmonitor` or `core.useBuiltinFSMonitor` configuration
+variable (see linkgit:git-config[1]) than using the `--fsmonitor`
+option to `git update-index` in each repository, especially if you
+want to do so across all repositories you use, because you can set the
+configuration variable in your `$HOME/.gitconfig` just once and have
+it affect all repositories you touch.
+
+When the `core.fsmonitor` or `core.useBuiltinFSMonitor` configuration
+variable is changed, the file system monitor is added to or removed
+from the index the next time a command reads the index. When
+`--[no-]fsmonitor` are used, the file system monitor is immediately
+added to or removed from the index.
 
 CONFIGURATION
 -------------

Documentation/githooks.txt +0 -0

index b51959ff941..b7d5e926f7b 100644

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

+#ifdef HAVE_FSMONITOR_DAEMON_BACKEND
+
+int cmd_fsmonitor__daemon(int argc, const char **argv, const char *prefix)
+{
+	const char *subcmd;
+
+	struct option options[] = {
+		OPT_END()
+	};
+
+	if (argc < 2)
+		usage_with_options(builtin_fsmonitor__daemon_usage, options);
+
+	if (argc == 2 && !strcmp(argv[1], "-h"))
+		usage_with_options(builtin_fsmonitor__daemon_usage, options);
+
+	git_config(git_default_config, NULL);
+
+	subcmd = argv[1];
+	argv--;
+	argc++;
+
+	argc = parse_options(argc, argv, prefix, options,
+			     builtin_fsmonitor__daemon_usage, 0);
+
+	die(_("Unhandled subcommand '%s'"), subcmd);
+}
+
+#else
+int cmd_fsmonitor__daemon(int argc, const char **argv, const char *prefix)
+{

From: Jeff Hostetler <[email protected]>

Create an IPC client to send query and flush commands to the daemon.

Signed-off-by Jeff Hostetler <[email protected]>

---
 Makefile                         |   1 +
 t/helper/test-fsmonitor-client.c | 121 +++++++++++++++++++++++++++++++
 t/helper/test-tool.c             |   1 +
 t/helper/test-tool.h             |   1 +
 4 files changed, 124 insertions(+)
 create mode 100644 t/helper/test-fsmonitor-client.c

Makefile +1 -0

index 8fe1e42a435..c45caacf2c3 100644
@@ -709,6 +709,7 @@ TEST_BUILTINS_OBJS += test-dump-split-index.o
 TEST_BUILTINS_OBJS += test-dump-untracked-cache.o
 TEST_BUILTINS_OBJS += test-example-decorate.o
 TEST_BUILTINS_OBJS += test-fast-rebase.o
+TEST_BUILTINS_OBJS += test-fsmonitor-client.o
 TEST_BUILTINS_OBJS += test-genrandom.o
 TEST_BUILTINS_OBJS += test-genzeros.o
 TEST_BUILTINS_OBJS += test-hash-speed.o

t/helper/test-fsmonitor-client.c new file +6 -0

new file mode 100644
index 00000000000..f7a5b3a32fa
@@ -0,0 +1,121 @@
+/*
+ * test-fsmonitor-client.c: client code to send commands/requests to
+ * a `git fsmonitor--daemon` daemon.
+ */
+
+#include "test-tool.h"

From: Jeff Hostetler <[email protected]>

Stub in empty backend for fsmonitor--daemon on Windows.

Signed-off-by Jeff Hostetler <[email protected]>

---
 Makefile                                     | 13 ++++++
 compat/fsmonitor/fsmonitor-fs-listen-win32.c | 21 +++++++++
 compat/fsmonitor/fsmonitor-fs-listen.h       | 49 ++++++++++++++++++++
 config.mak.uname                             |  2 +
 contrib/buildsystems/CMakeLists.txt          |  5 ++
 5 files changed, 90 insertions(+)
 create mode 100644 compat/fsmonitor/fsmonitor-fs-listen-win32.c
 create mode 100644 compat/fsmonitor/fsmonitor-fs-listen.h

Makefile +10 -0

index c45caacf2c3..a2a6e1f20f6 100644
@@ -467,6 +467,11 @@ all::
 # directory, and the JSON compilation database 'compile_commands.json' will be
 # created at the root of the repository.
 #
+# If your platform supports a built-in fsmonitor backend, set
+# FSMONITOR_DAEMON_BACKEND to the "<name>" of the corresponding
+# `compat/fsmonitor/fsmonitor-fs-listen-<name>.c` that implements the
+# `fsmonitor_fs_listen__*()` routines.
+#
 # Define DEVELOPER to enable more compiler warnings. Compiler version
 # and family are auto detected, but could be overridden by defining
 # COMPILER_FEATURES (see config.mak.dev). You can still set

@@ -1929,6 +1934,11 @@ ifdef NEED_ACCESS_ROOT_HANDLER
 	COMPAT_OBJS += compat/access.o
 endif
 
+ifdef FSMONITOR_DAEMON_BACKEND
+	COMPAT_CFLAGS += -DHAVE_FSMONITOR_DAEMON_BACKEND
+	COMPAT_OBJS += compat/fsmonitor/fsmonitor-fs-listen-$(FSMONITOR_DAEMON_BACKEND).o
+endif
+
 ifeq ($(TCLTK_PATH),)
 NO_TCLTK = NoThanks
 endif

From: Jeff Hostetler <[email protected]>

Stub in empty implementation of fsmonitor--daemon
backend for MacOS.

Signed-off-by Jeff Hostetler <[email protected]>

---
 compat/fsmonitor/fsmonitor-fs-listen-macos.c | 20 ++++++++++++++++++++
 config.mak.uname                             |  2 ++
 contrib/buildsystems/CMakeLists.txt          |  3 +++
 3 files changed, 25 insertions(+)
 create mode 100644 compat/fsmonitor/fsmonitor-fs-listen-macos.c

compat/fsmonitor/fsmonitor-fs-listen-macos.c new file +20 -0

new file mode 100644
index 00000000000..b91058d1c4f
@@ -0,0 +1,20 @@
+#include "cache.h"
+#include "fsmonitor.h"
+#include "fsmonitor-fs-listen.h"
+
+int fsmonitor_fs_listen__ctor(struct fsmonitor_daemon_state *state)
+{
+	return -1;
+}
+
+void fsmonitor_fs_listen__dtor(struct fsmonitor_daemon_state *state)
+{
+}
+
+void fsmonitor_fs_listen__stop_async(struct fsmonitor_daemon_state *state)
+{
+}
+
+void fsmonitor_fs_listen__loop(struct fsmonitor_daemon_state *state)
+{
+}

config.mak.uname +0 -0

index fcd88b60b14..394355463e1 100644

From: Jeff Hostetler <[email protected]>

Bare repos do not have a working directory, so there is no directory for the daemon to register a watch upon. And therefore there are no files within the directory for it to actually watch.

Signed-off-by Jeff Hostetler <[email protected]>

---
 builtin/fsmonitor--daemon.c |  8 ++++++++
 t/t7519-status-fsmonitor.sh | 16 ++++++++++++++++
 2 files changed, 24 insertions(+)

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

index 7fcf960652f..d6161ad95a5 100644

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

+	if (!test_env_value) {
+		struct timeval tv;
+		struct tm tm;
+		time_t secs;
+
+		gettimeofday(&tv, NULL);

From: Jeff Hostetler <[email protected]>

Teach the win32 backend to register a watch on the working tree root directory (recursively). Also watch the <gitdir> if it is not inside the working tree. And to collect path change notifications into batches and publish.

Signed-off-by Jeff Hostetler <[email protected]>

---
 compat/fsmonitor/fsmonitor-fs-listen-win32.c | 530 +++++++++++++++++++

From: Jeff Hostetler <[email protected]>

Change p7519 to use a single "test-tool touch" command to update the mtime on a series of (thousands) files instead of invoking thousands of commands to update a single file.

This is primarily for Windows where process creation is so very slow and reduces the test run time by minutes.

Signed-off-by Jeff Hostetler <[email protected]>

---
 t/perf/p7519-fsmonitor.sh | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

t/perf/p7519-fsmonitor.sh +5 -6

index 5eb5044a103..f74e6014a0a 100755
@@ -119,10 +119,11 @@ test_expect_success "one time repo setup" '
 	fi &&
 
 	mkdir 1_file 10_files 100_files 1000_files 10000_files &&
-	for i in $(test_seq 1 10); do touch 10_files/$i; done &&
-	for i in $(test_seq 1 100); do touch 100_files/$i; done &&
-	for i in $(test_seq 1 1000); do touch 1000_files/$i; done &&
-	for i in $(test_seq 1 10000); do touch 10000_files/$i; done &&
+	test-tool touch sequence --pattern="10_files/%d" --start=1 --count=10 &&
+	test-tool touch sequence --pattern="100_files/%d" --start=1 --count=100 &&
+	test-tool touch sequence --pattern="1000_files/%d" --start=1 --count=1000 &&
+	test-tool touch sequence --pattern="10000_files/%d" --start=1 --count=10000 &&
+
 	git add 1_file 10_files 100_files 1000_files 10000_files &&
 	git commit -qm "Add files" &&
 

@@ -200,15 +201,12 @@ test_fsmonitor_suite() {
 	# Update the mtimes on upto 100k files to make status think
 	# that they are dirty.  For simplicity, omit any files with
 	# LFs (i.e. anything that ls-files thinks it needs to dquote).
-	# Then fully backslash-quote the paths to capture any
-	# whitespace so that they pass thru xargs properly.

From: Jeff Hostetler <[email protected]>

Do not try to copy a fsmonitor--daemon socket from the current development directory into the test trash directory.

When we run the perf suite without an explicit source repo set, we copy of the current $GIT_DIR into the test trash directory. Unix domain sockets cannot be copied in that manner, so the test setup fails.

Additionally, omit any other fsmonitor--daemon temp files inside the $GIT_DIR directory.

t/t7527-builtin-fsmonitor.sh new file +6 -0

+	grep \"event\":\"start\".*\"fsmonitor--daemon\" .git/trace_implicit_1
+'
+
+test_expect_success 'status implicitly starts daemon' '
+	test_when_finished redundant_stop_daemon &&
+

From: Jeff Hostetler <[email protected]>

Teach fsmonitor--daemon client threads to create a cookie file inside the .git directory and then wait until FS events for the cookie are observed by the FS listener thread.

This helps address the racy nature of file system events by blocking the client response until the kernel has drained any event backlog.

This is especially important on MacOS where kernel events are only issued with a limited frequency. See the latency argument of FSeventStreamCreate(). The kernel only signals every latency seconds, but does not guarantee that the kernel queue is completely drained, so we may have to wait more than one interval. If we increase the frequency, the system is more likely to drop events. We avoid these issues by having each client thread create a unique cookie file and then wait until it is seen in the event stream.

Co-authored-by Kevin Willford <[email protected]>

Co-authored-by Johannes Schindelin <[email protected]>

Signed-off-by Jeff Hostetler <[email protected]>

---
 builtin/fsmonitor--daemon.c | 228 +++++++++++++++++++++++++++++++++++-
 fsmonitor--daemon.h         |   5 +
 2 files changed, 232 insertions(+), 1 deletion(-)

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

index 8249420ba18..25f18f2726b 100644
@@ -94,6 +94,149 @@ static int do_as_client__status(void)
 	}
 }
 
+enum fsmonitor_cookie_item_result {
+	FCIR_ERROR = -1, /* could not create cookie file ? */
+	FCIR_INIT = 0,
+	FCIR_SEEN,
+	FCIR_ABORT,
+};
+
+struct fsmonitor_cookie_item {
+	struct hashmap_entry entry;
+	const char *name;

[...]

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

+	/*
+	 * We will write filesystem syncing cookie files into
+	 * <gitdir>/<fsmonitor-dir>/<cookie-dir>/<pid>-<seq>.
+	 *
+	 * The extra layers of subdirectories here keep us from
+	 * changing the mtime on ".git/" or ".git/foo/" when we create
+	 * or delete cookie files.
+	 *
+	 * There have been problems with some IDEs that do a
+	 * non-recursive watch of the ".git/" directory and run a
+	 * series of commands any time something happens.
+	 *
+	 * For example, if we place our cookie files directly in
+	 * ".git/" or ".git/foo/" then a `git status` (or similar
+	 * command) from the IDE will cause a cookie file to be
+	 * created in one of those dirs.  This causes the mtime of
+	 * those dirs to change.  This triggers the IDE's watch
+	 * notification.  This triggers the IDE to run those commands
+	 * again.  And the process repeats and the machine never goes
+	 * idle.
+	 *
+	 * Adding the extra layers of subdirectories prevents the
+	 * mtime of ".git/" and ".git/foo" from changing when a
+	 * cookie file is created.
+	 */
+	strbuf_init(&state.path_cookie_prefix, 0);
+	strbuf_addbuf(&state.path_cookie_prefix, &state.path_gitdir_watch);

Quoted 35 lines · Jeff Hostetler

Quoted 10 lines · Ævar Arnfjörð Bjarmason

Quoted 8 lines · Jeff Hostetler via GitGitGadget

Here is V3 of my patch series to add a builtin FSMonitor daemon to Git. I rebased this series onto v2.32.0.

V3 addresses most of the previous review comments and things we've learned from our experimental testing of V2. (A version of V2 was shipped as an experimental feature in the v2.32.0-based releases of Git for Windows and VFS for Git.)

Quoted 8 lines

Quoted 2 lines

There are still a few items that I need to address, but that list is getting very short.

...

Quoted 2 lines

fsmonitor-fs-listen-win32: stub in backend for Windows
fsmonitor-fs-listen-macos: stub in backend for MacOS

I left some light comments on the repo-settings.c part of this to follow
up from a previous round.

Thanks.

Quoted 6 lines

Any other testing of it is stalled by there being no linux backend for
it as part of this series. I see from spelunking repos that Johannes had a WIP compat/fsmonitor/linux.c which looks like it could/should mostly work, but the API names all changed since then, and after a short try I gave up on trying to rebase it.

The early Linux version was dropped because inotify does not give recursive coverage -- only the requested directory. Using inotify requires adding a watch to each subdirectory (recursively) in the worktree. There's a system limit on the maximum number of watched directories (defaults to 8K IIRC) and that limit is system-wide.

Quoted 8 lines

Since the whole point was to support large very large repos, using inotify was a non-starter, so I removed the Linux version from our patch series. For example, the first repo I tried it on (outside of the test suite) had 25K subdirectories.

I'm told there is a new fanotify API in recent Linux kernels that is a better fit for what we need, but we haven't had time to investigate it yet.

That default limit is a bit annoying, but I don't see how it's a blocker in any way.

You simply adjust the limit. E.g. I deployed and tested the hook version of inotify (using watchman) in a sizable development environment, and written my own code using the API. This was all before fanotify(7) existed. IIRC I set most of the limits to 2^24 or 2^20. I've used it with really large Git repos, including with David Turner's 2015-04-03-1M-git for testing (git ls-files | wc -l on that is around a quarter-million).

If you have a humongous repository and don't have root on your own machine you're out of luck. But I think most people who'd use this are either using their own laptop, or e.g. in a corporate setting where administrators would tweak the sysctl limits given the performance advantages (as I did).

Once you adjust the limits Linux deals with large trees just fine, it just has overly conservative limits for most things in sysctl. The API is a bit annoying, your event loop needs to run around and add watches.

AFAICT you need Linux 5.1 for fanotify(7) to be useful, e.g. Debian stable, RHEL etc. aren't using anything that new. So having an inotify backend as well as possibly a fanotify one would be very useful.

And linux.git's docs suggest that the default limit was bumped from 8192 to 1M in v5.10, a really recent kernel, so if you've got that you've also got fanotify.

In any case, even if Linux's inotify limit was something hardcoded and impossible to change you could still use such an API to test & debug the basics of this feature on that platform.

Quoted 11 lines

Quoted 7 lines

I'd really prefer for git not to have features that place free platforms
at a disadvantage against proprietary platforms if it can be avoided, and in this case the lack of a Linux backend also means much less widespread testing of the feature among the development community / CI.

This feature is always going to have platform-specific components, so the lack of one platform or another should not stop us from discussing it for the platforms that can be supported.

(I think per the above that's s/can be/are/)

Quoted 4 lines

And given the size and complexity of the platform-specific code, we should not assume that "just test it on Linux" is sufficient. Yes, there are some common/shared routines/data structures in the daemon, but hard/tricky parts are in the platform layer.

I think we're talking past each other a bit here. I'm not saying that you can get full or meaningful testing for it on Windows if you test it on Linux, or the other way around.

Of course there's platform-specific stuff, although there's also a lot of non-platform-specific stuff, so even a very basic implementation of inotify would make reviwing this easier / give access to more reviewers.

I'm saying that I prefer that Git as a free software project not be in the situation of saying the best use-case for a given size/shape of repo is to use Git in combination with proprietary operating systems over freely licensed ones.

IOW what the FSF has a policy for GNU projects. Now, I think the FSF probably goes too far in that (famously removing rather obscure font rendering features from Emacs on OSX), but "manage lots of data" is a core feature of git.

Quoted 7 lines · Jeff Hostetler

Quoted 5 lines · Ævar Arnfjörð Bjarmason

Any other testing of it is stalled by there being no linux backend for it as part of this series. I see from spelunking repos that Johannes had a WIP compat/fsmonitor/linux.c which looks like it could/should mostly work, but the API names all changed since then, and after a short try I gave up on trying to rebase it.

Quoted 11 lines

The early Linux version was dropped because inotify does not give recursive coverage -- only the requested directory. Using inotify requires adding a watch to each subdirectory (recursively) in the worktree. There's a system limit on the maximum number of watched directories (defaults to 8K IIRC) and that limit is system-wide.

Since the whole point was to support large very large repos, using inotify was a non-starter, so I removed the Linux version from our patch series. For example, the first repo I tried it on (outside of the test suite) had 25K subdirectories.

Quoted 3 lines

I'm told there is a new fanotify API in recent Linux kernels that is a better fit for what we need, but we haven't had time to investigate it yet.

That default limit is a bit annoying, but I don't see how it's a blocker in any way.

You simply adjust the limit. E.g. I deployed and tested the hook version of inotify (using watchman) in a sizable development environment, and written my own code using the API. This was all before fanotify(7) existed. IIRC I set most of the limits to 2^24 or 2^20. I've used it with really large Git repos, including with David Turner's 2015-04-03-1M-git for testing (git ls-files | wc -l on that is around a quarter-million).

If you have a humongous repository and don't have root on your own machine you're out of luck. But I think most people who'd use this are either using their own laptop, or e.g. in a corporate setting where administrators would tweak the sysctl limits given the performance advantages (as I did).

Quoted 73 lines · Jeff Hostetler via GitGitGadget

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

+#ifdef GIT_WINDOWS_NATIVE
+/*
+ * Create a background process to run the daemon.  It should be completely
+ * disassociated from the terminal.
+ *
+ * Conceptually like `daemonize()` but different because Windows does not
+ * have `fork(2)`.  Spawn a normal Windows child process but without the
+ * limitations of `start_command()` and `finish_command()`.
+ *
+ * The child process execs the "git fsmonitor--daemon run" command.
+ *
+ * The current process returns so that the caller can wait for the child
+ * to startup before exiting.
+ */
+static int spawn_background_fsmonitor_daemon(pid_t *pid)
+{
+	char git_exe[MAX_PATH];
+	struct strvec args = STRVEC_INIT;
+	int in, out;
+
+	GetModuleFileNameA(NULL, git_exe, MAX_PATH);
+
+	in = open("/dev/null", O_RDONLY);
+	out = open("/dev/null", O_WRONLY);
+
+	strvec_push(&args, git_exe);
+	strvec_push(&args, "fsmonitor--daemon");
+	strvec_push(&args, "run");
+	strvec_pushf(&args, "--ipc-threads=%d", fsmonitor__ipc_threads);
+
+	*pid = mingw_spawnvpe(args.v[0], args.v, NULL, NULL, in, out, out);
+	close(in);
+	close(out);
+
+	strvec_clear(&args);
+
+	if (*pid < 0)
+		return error(_("could not spawn fsmonitor--daemon in the background"));
+
+	return 0;
+}
+#else
+/*
+ * Create a background process to run the daemon.  It should be completely
+ * disassociated from the terminal.
+ *
+ * This is adapted from `daemonize()`.  Use `fork()` to directly
+ * create and run the daemon in the child process.
+ *
+ * The fork-child can just call the run code; it does not need to exec
+ * it.
+ *
+ * The fork-parent returns the child PID so that we can wait for the
+ * child to startup before exiting.
+ */
+static int spawn_background_fsmonitor_daemon(pid_t *pid)
+{
+	*pid = fork();
+
+	switch (*pid) {
+	case 0:
+		if (setsid() == -1)
+			error_errno(_("setsid failed"));
+		close(0);
+		close(1);
+		close(2);
+		sanitize_stdfds();
+
+		return !!fsmonitor_run_daemon();
+
+	case -1:
+		return error_errno(_("could not spawn fsmonitor--daemon in the background"));
+

builtin/fsmonitor--daemon.c static int try_to_run_foreground_daemon(void) · line 349

Quoted 5 lines

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

+	default:
+		return 0;
+	}
+}
+#endif

The spawn_background_fsmonitor_daemon() function here is almost the same as daemonize().

I wonder if this & the Windows-specific one you have here can't be refactored into an API from what's now in setup.c.

Then we could make builtin/gc.c and daemon.c use that, so Windows could have background GC, and we'd have a more battle-tested central codepath for this tricky bit.

It seems to me like the only limitations on it are to have this return slightly more general things (e.g. not set its own errors, return structured data), and maybe some callback for what to do in the child/parent.

Quoted 60 lines

builtin/fsmonitor--daemon.c +59 -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;
+
+	time(&time_limit);
+	time_limit += fsmonitor__start_timeout_sec;
+
+	for (;;) {
+		pid_seen = waitpid(pid_child, &status, WNOHANG);
+
+		if (pid_seen == -1)
+			return error_errno(_("waitpid failed"));
+		else if (pid_seen == 0) {
+			/*
+			 * The child is still running (this should be
+			 * the normal case).  Try to connect to it on
+			 * the socket and see if it is ready for
+			 * business.
+			 *
+			 * If there is another daemon already running,
+			 * our child will fail to start (possibly
+			 * after a timeout on the lock), but we don't
+			 * care (who responds) if the socket is live.
+			 */
+			s = fsmonitor_ipc__get_state();
+			if (s == IPC_STATE__LISTENING)
+				return 0;
+
+			time(&now);
+			if (now > time_limit)
+				return error(_("fsmonitor--daemon not online yet"));
+		} else if (pid_seen == pid_child) {
+			/*
+			 * The new child daemon process shutdown while
+			 * it was starting up, so it is not listening
+			 * on the socket.
+			 *
+			 * Try to ping the socket in the odd chance
+			 * that another daemon started (or was already
+			 * running) while our child was starting.
+			 *
+			 * Again, we don't care who services the socket.
+			 */
+			s = fsmonitor_ipc__get_state();
+			if (s == IPC_STATE__LISTENING)
+				return 0;
+
+			/*
+			 * We don't care about the WEXITSTATUS() nor
+			 * any of the WIF*(status) values because
+			 * `cmd_fsmonitor__daemon()` does the `!!result`

builtin/fsmonitor--daemon.c static int try_to_run_foreground_daemon(void) · line 419

Quoted 10 lines

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

+			 * trick on all function return values.
+			 *
+			 * So it is sufficient to just report the
+			 * early shutdown as an error.
+			 */
+			return error(_("fsmonitor--daemon failed to start"));
+		} else
+			return error(_("waitpid is confused"));
+	}
+}

Ditto this. could we extend the wait_or_whine() function (or some extended version thereof) to do what you need with callbacks?

It seems the main difference is just being able to pass down a flag for waitpid(), and the loop needing to check EINTR or not depending on whether WNOHANG is passed.

For e.g. the "We don't care about the WEXITSTATUS()" you'd get that behavior with an adjusted wait_or_whine(). Wouldn't it be better to report what exit status it exits with e.g. if the top-level process is signalled? We do so in trace2 for other things we spawn...

Quoted 81 lines · Jeff Hostetler via GitGitGadget

From: Jeff Hostetler <[email protected]>

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

Signed-off-by Jeff Hostetler <[email protected]>

---
 Documentation/git-fsmonitor--daemon.txt | 75 +++++++++++++++++++++++++
 1 file changed, 75 insertions(+)
 create mode 100644 Documentation/git-fsmonitor--daemon.txt

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

new file mode 100644
index 00000000000..154e7684daa
@@ -0,0 +1,75 @@
+git-fsmonitor--daemon(1)
+========================
+
+NAME
+----
+git-fsmonitor--daemon - A Built-in File System Monitor
+
+SYNOPSIS
+--------
+[verse]
+'git fsmonitor--daemon' start
+'git fsmonitor--daemon' run
+'git fsmonitor--daemon' stop
+'git fsmonitor--daemon' status
+
+DESCRIPTION
+-----------
+
+A daemon to watch the working directory for file and directory
+changes using platform-specific file system notification facilities.
+
+This daemon 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.
+
+This daemon is built into Git so that no third-party tools are
+required.
+
+OPTIONS
+-------
+
+start::
+	Starts a daemon in the background.
+
+run::
+	Runs a daemon in the foreground.
+
+stop::
+	Stops the daemon running in the current working
+	directory, if present.
+
+status::
+	Exits with zero status if a daemon is watching the
+	current working directory.
+
+REMARKS
+-------
+
+This daemon is a long running process used to watch a single working
+directory and maintain a list of the recently changed files and
+directories.  Performance of commands such as `git status` can be
+increased if they just ask for a summary of changes to the working
+directory and can avoid scanning the disk.
+
+When `core.useBuiltinFSMonitor` is set to `true` (see
+linkgit:git-config[1]) commands, such as `git status`, will ask the
+daemon for changes and automatically start it (if necessary).
+
+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

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

Quoted 10 lines

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

+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
+modified within the working directory of a submodule, it will report
+the change (as happening against the super repo).  However, the client
+will properly ignore these extra events, so performance may be affected
+but it will not cause an incorrect result.
+
+GIT
+---
+Part of the linkgit:git[1] suite

Later in the series we incrementally add features to the daemon, so this is describing a state that doesn't exist yet at this point.

I think it would be better to start with a stup here and add documentation as we add features, e.g. the patch tha adds "start" should add that to the synopsis + options etc.

Quoted 3 lines · Ævar Arnfjörð Bjarmason

Quoted 1 line · Jeff Hostetler

On 7/1/21 1:40 PM, Ævar Arnfjörð Bjarmason wrote:

[...]

Quoted 11 lines

Quoted 11 lines

The early Linux version was dropped because inotify does not give recursive coverage -- only the requested directory. Using inotify requires adding a watch to each subdirectory (recursively) in the worktree. There's a system limit on the maximum number of watched directories (defaults to 8K IIRC) and that limit is system-wide.

Since the whole point was to support large very large repos, using inotify was a non-starter, so I removed the Linux version from our patch series. For example, the first repo I tried it on (outside of the test suite) had 25K subdirectories.

Quoted 6 lines

Quoted 3 lines

I'm told there is a new fanotify API in recent Linux kernels that is a better fit for what we need, but we haven't had time to investigate it yet.

That default limit is a bit annoying, but I don't see how it's a blocker in any way.

Let me help you to see it.

So let's assume that you start FSMonitor-enabled Git, with the default values. What is going to happen if you have any decently-sized worktree? You run out of handles. What then? Throw your hands in the air? Stop working? Report incorrect results?

Those are real design challenges, and together with the race problems Jeff mentioned, they pose a much bigger obstacle than the rebasing you mentioned above.

Quoted 8 lines

You simply adjust the limit. E.g. I deployed and tested the hook version of inotify (using watchman) in a sizable development environment, and written my own code using the API. This was all before fanotify(7) existed. IIRC I set most of the limits to 2^24 or 2^20. I've used it with really large Git repos, including with David Turner's 2015-04-03-1M-git for testing (git ls-files | wc -l on that is around a quarter-million).

Quoted 5 lines

If you have a humongous repository and don't have root on your own machine you're out of luck. But I think most people who'd use this are either using their own laptop, or e.g. in a corporate setting where administrators would tweak the sysctl limits given the performance advantages (as I did).

This conjecture that most people who'd use this are using their own laptop or have a corporate setting where administrators would tweak the sysctl limits according to engineers' wishes strikes me as totally made up from thin air, nothing else.

In other words, I find it an incredibly unconvincing argument.

I prefer not to address the rest of your mail, as I found it not only a lengthy tangent (basically trying to talk Jeff into adding Linux support in what could have been a much shorter mail), but actively distracting from the already long patch series Jeff presented. It is so long, in fact, that we had to put in an exemption in GitGitGadget because it is already longer than a usually-unreasonable 30 patches. Also, at this point, insisting on Linux support (in so many words) is unhelpful.

Let me summarize why I think this is unhelpful: In Git, it is our tradition to develop incrementally, for better or worse. Jeff's effort brought us to a point where we already have Windows and macOS support, i.e. support for the most prevalent development platforms (see e.g.
https://insights.stackoverflow.com/survey/2020#technology-developers-primary-operating-systems).
We already established multiple obstacles for Linux support, therefore demanding Linux support to be included Right Now would increase the patch series even further, making it even less reviewable, being even less incremental, hold up the current known-to-work-well state, force Jeff to work on something he probably cannot work on right now, and therefore delaying the entire effort even further.

Quoted 68 lines · Ævar Arnfjörð Bjarmason

Quoted 35 lines · Jeff Hostetler

Quoted 10 lines · Ævar Arnfjörð Bjarmason

Quoted 8 lines · Jeff Hostetler via GitGitGadget

Here is V3 of my patch series to add a builtin FSMonitor daemon to Git. I rebased this series onto v2.32.0.

V3 addresses most of the previous review comments and things we've learned from our experimental testing of V2. (A version of V2 was shipped as an experimental feature in the v2.32.0-based releases of Git for Windows and VFS for Git.)

Quoted 8 lines

Quoted 2 lines

There are still a few items that I need to address, but that list is getting very short.

...

Quoted 2 lines

fsmonitor-fs-listen-win32: stub in backend for Windows
fsmonitor-fs-listen-macos: stub in backend for MacOS

I left some light comments on the repo-settings.c part of this to follow
up from a previous round.

Thanks.

Quoted 6 lines

Any other testing of it is stalled by there being no linux backend for
it as part of this series. I see from spelunking repos that Johannes had a WIP compat/fsmonitor/linux.c which looks like it could/should mostly work, but the API names all changed since then, and after a short try I gave up on trying to rebase it.

The early Linux version was dropped because inotify does not give recursive coverage -- only the requested directory. Using inotify requires adding a watch to each subdirectory (recursively) in the worktree. There's a system limit on the maximum number of watched directories (defaults to 8K IIRC) and that limit is system-wide.

Quoted 8 lines

Since the whole point was to support large very large repos, using inotify was a non-starter, so I removed the Linux version from our patch series. For example, the first repo I tried it on (outside of the test suite) had 25K subdirectories.

I'm told there is a new fanotify API in recent Linux kernels that is a better fit for what we need, but we haven't had time to investigate it yet.

That default limit is a bit annoying, but I don't see how it's a blocker
in any way.
You simply adjust the limit. E.g. I deployed and tested the hook version
of inotify (using watchman) in a sizable development environment, and written my own code using the API. This was all before fanotify(7) existed. IIRC I set most of the limits to 2^24 or 2^20. I've used it with really large Git repos, including with David Turner's 2015-04-03-1M-git for testing (git ls-files | wc -l on that is around a quarter-million).
If you have a humongous repository and don't have root on your own machine you're out of luck. But I think most people who'd use this are either using their own laptop, or e.g. in a corporate setting where administrators would tweak the sysctl limits given the performance advantages (as I did).
Once you adjust the limits Linux deals with large trees just fine, it
just has overly conservative limits for most things in sysctl. The API is a bit annoying, your event loop needs to run around and add watches. AFAICT you need Linux 5.1 for fanotify(7) to be useful, e.g. Debian stable, RHEL etc. aren't using anything that new. So having an inotify backend as well as possibly a fanotify one would be very useful.

Quoted 8 lines

And linux.git's docs suggest that the default limit was bumped from 8192
to 1M in v5.10, a really recent kernel, so if you've got that you've also got fanotify.
In any case, even if Linux's inotify limit was something hardcoded and
impossible to change you could still use such an API to test & debug the basics of this feature on that platform.

Good points. If the inotify limits can be increased into the millions then we can revisit supporting it.

[...] I do worry about possible race
conditions as we have to scan the worktree and add/delete a watch for each directory, but we don't need to worry about that right now.

I do want to have Linux support eventually, but I was saving it for a later effort (and/or looking for volunteers).

My IPC-based builtin daemon complements the existing hook-based fsmonitor support that Ben Peart and Kevin Willford added a few years ago. That model (and PERL hook script and Watchman integration) work fine for Linux, so the advantages of a builtin monitor aren't as compelling.

For example, on Linux, hook process creation is fast, PERL is fast,[...]

and it is easy to just apt-get a tool like Watchman. But on Windows, process creation is slow, NTFS is slow, PERL is available as part of the Git-for-Windows distribution, and installing third-party tools like Watchman onto a collection of enterprise users' machines is a chore, so it made sense of us to pick platforms where the existing hook model has issues and add other platforms later.

Besides, this patch series is already at 34 commits. And some of them are quite large. Adding another platform would just make it even larger.

Right now I'm more interested in the larger question of whether we WANT to have a builtin fsmonitor and do we like the overall design of what I have here? Picking up a new platform, whether it is Linux, or AIX, or Solaris, or Nonstop, or whatever, should nicely fit into one platform-specific file in compat/fsmonitor and not take that long.

Quoted 16 lines

Quoted 10 lines

Quoted 6 lines

I'd really prefer for git not to have features that place free platforms
at a disadvantage against proprietary platforms if it can be avoided, and in this case the lack of a Linux backend also means much less widespread testing of the feature among the development community / CI.

This feature is always going to have platform-specific components, so the lack of one platform or another should not stop us from discussing it for the platforms that can be supported.

(I think per the above that's s/can be/are/)

Quoted 4 lines

And given the size and complexity of the platform-specific code, we should not assume that "just test it on Linux" is sufficient. Yes, there are some common/shared routines/data structures in the daemon, but hard/tricky parts are in the platform layer.

Quoted 13 lines

I think we're talking past each other a bit here. I'm not saying that
you can get full or meaningful testing for it on Windows if you test it on Linux, or the other way around.
Of course there's platform-specific stuff, although there's also a lot
of non-platform-specific stuff, so even a very basic implementation of inotify would make reviwing this easier / give access to more reviewers. I'm saying that I prefer that Git as a free software project not be in
the situation of saying the best use-case for a given size/shape of repo is to use Git in combination with proprietary operating systems over freely licensed ones.

I wouldn't worry about that. Even without Watchman integration, Linux runs things so much faster than anything else it's not funny.

If anything, we need things like fsmonitor on Windows to help keep up with Linux.

I think we just disagree. I wouldn't call my opinion "unhelpful" any more than yours.

compat/fsmonitor/fsmonitor-fs-listen-win32.c +5 -0

index 880446b49e3..d707d47a0d7 100644
@@ -2,20 +2,550 @@
+ [...]
+
+static struct one_watch *create_watch(struct fsmonitor_daemon_state *state,
+				      const char *path)
+{

In Git, it is our tradition to develop incrementally, for better or worse.

Jeff's effort
brought us to a point where we already have Windows and macOS support, i.e. support for the most prevalent development platforms (see e.g.
https://insights.stackoverflow.com/survey/2020#technology-developers-primary-operating-systems).
We already established multiple obstacles for Linux support, therefore demanding Linux support to be included Right Now would increase the patch series even further, making it even less reviewable, being even less incremental, hold up the current known-to-work-well state, force Jeff to work on something he probably cannot work on right now, and therefore delaying the entire effort even further.

Quoted 2 lines · Ævar Arnfjörð Bjarmason

I think we just disagree. I wouldn't call my opinion "unhelpful" any more than yours.

You still misunderstand. This is not about any "opinion" of yours, it is about your delay tactics to make it deliberately difficult to finish this patch series,

by raising the bar beyond what is reasonable for a single patch series.

Quoted 81 lines · Jeff Hostetler via GitGitGadget

From: Jeff Hostetler <[email protected]>

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

Signed-off-by Jeff Hostetler <[email protected]>

---
  Documentation/git-fsmonitor--daemon.txt | 75 +++++++++++++++++++++++++
  1 file changed, 75 insertions(+)

create mode 100644 Documentation/git-fsmonitor--daemon.txt

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

new file mode 100644
index 00000000000..154e7684daa
@@ -0,0 +1,75 @@
+git-fsmonitor--daemon(1)
+========================
+
+NAME
+----
+git-fsmonitor--daemon - A Built-in File System Monitor
+
+SYNOPSIS
+--------
+[verse]
+'git fsmonitor--daemon' start
+'git fsmonitor--daemon' run
+'git fsmonitor--daemon' stop
+'git fsmonitor--daemon' status
+
+DESCRIPTION
+-----------
+
+A daemon to watch the working directory for file and directory
+changes using platform-specific file system notification facilities.
+
+This daemon 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.
+
+This daemon is built into Git so that no third-party tools are
+required.
+
+OPTIONS
+-------
+
+start::
+	Starts a daemon in the background.
+
+run::
+	Runs a daemon in the foreground.
+
+stop::
+	Stops the daemon running in the current working
+	directory, if present.
+
+status::
+	Exits with zero status if a daemon is watching the
+	current working directory.
+
+REMARKS
+-------
+
+This daemon is a long running process used to watch a single working
+directory and maintain a list of the recently changed files and
+directories.  Performance of commands such as `git status` can be
+increased if they just ask for a summary of changes to the working
+directory and can avoid scanning the disk.
+
+When `core.useBuiltinFSMonitor` is set to `true` (see
+linkgit:git-config[1]) commands, such as `git status`, will ask the
+daemon for changes and automatically start it (if necessary).
+
+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

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

Quoted 10 lines

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

+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
+modified within the working directory of a submodule, it will report
+the change (as happening against the super repo).  However, the client
+will properly ignore these extra events, so performance may be affected
+but it will not cause an incorrect result.
+
+GIT
+---
+Part of the linkgit:git[1] suite

Later in the series we incrementally add features to the daemon, so this is describing a state that doesn't exist yet at this point.

I think it would be better to start with a stup here and add documentation as we add features, e.g. the patch tha adds "start" should add that to the synopsis + options etc.

See the outstanding ab/config-based-hooks-base for a small example of that.

Quoted 68 lines · Jeff Hostetler via GitGitGadget

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

+#ifdef GIT_WINDOWS_NATIVE
+/*
+ * Create a background process to run the daemon.  It should be completely
+ * disassociated from the terminal.
+ *
+ * Conceptually like `daemonize()` but different because Windows does not
+ * have `fork(2)`.  Spawn a normal Windows child process but without the
+ * limitations of `start_command()` and `finish_command()`.
+ *
+ * The child process execs the "git fsmonitor--daemon run" command.
+ *
+ * The current process returns so that the caller can wait for the child
+ * to startup before exiting.
+ */
+static int spawn_background_fsmonitor_daemon(pid_t *pid)
+{
+	char git_exe[MAX_PATH];
+	struct strvec args = STRVEC_INIT;
+	int in, out;
+
+	GetModuleFileNameA(NULL, git_exe, MAX_PATH);
+
+	in = open("/dev/null", O_RDONLY);
+	out = open("/dev/null", O_WRONLY);
+
+	strvec_push(&args, git_exe);
+	strvec_push(&args, "fsmonitor--daemon");
+	strvec_push(&args, "run");
+	strvec_pushf(&args, "--ipc-threads=%d", fsmonitor__ipc_threads);
+
+	*pid = mingw_spawnvpe(args.v[0], args.v, NULL, NULL, in, out, out);
+	close(in);
+	close(out);
+
+	strvec_clear(&args);
+
+	if (*pid < 0)
+		return error(_("could not spawn fsmonitor--daemon in the background"));
+
+	return 0;
+}
+#else
+/*
+ * Create a background process to run the daemon.  It should be completely
+ * disassociated from the terminal.
+ *
+ * This is adapted from `daemonize()`.  Use `fork()` to directly
+ * create and run the daemon in the child process.
+ *
+ * The fork-child can just call the run code; it does not need to exec
+ * it.
+ *
+ * The fork-parent returns the child PID so that we can wait for the
+ * child to startup before exiting.
+ */
+static int spawn_background_fsmonitor_daemon(pid_t *pid)
+{
+	*pid = fork();
+
+	switch (*pid) {
+	case 0:
+		if (setsid() == -1)
+			error_errno(_("setsid failed"));
+		close(0);
+		close(1);
+		close(2);
+		sanitize_stdfds();
+

builtin/fsmonitor--daemon.c static int try_to_run_foreground_daemon(void) · line 349

Quoted 10 lines

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

+		return !!fsmonitor_run_daemon();
+
+	case -1:
+		return error_errno(_("could not spawn fsmonitor--daemon in the background"));
+
+	default:
+		return 0;
+	}
+}
+#endif

The spawn_background_fsmonitor_daemon() function here is almost the same as daemonize(). I wonder if this & the Windows-specific one you have here can't be refactored into an API from what's now in setup.c.

Then we could make builtin/gc.c and daemon.c use that, so Windows could have background GC, and we'd have a more battle-tested central codepath for this tricky bit.

It seems to me like the only limitations on it are to have this return slightly more general things (e.g. not set its own errors, return structured data), and maybe some callback for what to do in the child/parent.

Quoted 60 lines

builtin/fsmonitor--daemon.c +59 -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;
+
+	time(&time_limit);
+	time_limit += fsmonitor__start_timeout_sec;
+
+	for (;;) {
+		pid_seen = waitpid(pid_child, &status, WNOHANG);
+
+		if (pid_seen == -1)
+			return error_errno(_("waitpid failed"));
+		else if (pid_seen == 0) {
+			/*
+			 * The child is still running (this should be
+			 * the normal case).  Try to connect to it on
+			 * the socket and see if it is ready for
+			 * business.
+			 *
+			 * If there is another daemon already running,
+			 * our child will fail to start (possibly
+			 * after a timeout on the lock), but we don't
+			 * care (who responds) if the socket is live.
+			 */
+			s = fsmonitor_ipc__get_state();
+			if (s == IPC_STATE__LISTENING)
+				return 0;
+
+			time(&now);
+			if (now > time_limit)
+				return error(_("fsmonitor--daemon not online yet"));
+		} else if (pid_seen == pid_child) {
+			/*
+			 * The new child daemon process shutdown while
+			 * it was starting up, so it is not listening
+			 * on the socket.
+			 *
+			 * Try to ping the socket in the odd chance
+			 * that another daemon started (or was already
+			 * running) while our child was starting.
+			 *
+			 * Again, we don't care who services the socket.
+			 */
+			s = fsmonitor_ipc__get_state();
+			if (s == IPC_STATE__LISTENING)
+				return 0;
+
+			/*
+			 * We don't care about the WEXITSTATUS() nor
+			 * any of the WIF*(status) values because
+			 * `cmd_fsmonitor__daemon()` does the `!!result`

builtin/fsmonitor--daemon.c static int try_to_run_foreground_daemon(void) · line 419

Quoted 10 lines

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

+			 * trick on all function return values.
+			 *
+			 * So it is sufficient to just report the
+			 * early shutdown as an error.
+			 */
+			return error(_("fsmonitor--daemon failed to start"));
+		} else
+			return error(_("waitpid is confused"));
+	}
+}

Ditto this. could we extend the wait_or_whine() function (or some extended version thereof) to do what you need with callbacks?

It seems the main difference is just being able to pass down a flag for waitpid(), and the loop needing to check EINTR or not depending on whether WNOHANG is passed.

For e.g. the "We don't care about the WEXITSTATUS()" you'd get that behavior with an adjusted wait_or_whine(). Wouldn't it be better to report what exit status it exits with e.g. if the top-level process is signalled? We do so in trace2 for other things we spawn...

Quoted 11 lines · Jeff Hostetler via GitGitGadget

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

+	if (!test_env_value) {
+		struct timeval tv;
+		struct tm tm;
+		time_t secs;
+
+		gettimeofday(&tv, NULL);
+		secs = tv.tv_sec;
+		gmtime_r(&secs, &tm);
+
+		strbuf_addf(&token->token_id,
+			    "%"PRIu64".%d.%4d%02d%02dT%02d%02d%02d.%06ldZ",

builtin/fsmonitor--daemon.c static int do_as_client__status(void) · line 202

Quoted 5 lines

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

+			    flush_count++,
+			    getpid(),
+			    tm.tm_year + 1900, tm.tm_mon + 1, tm.tm_mday,
+			    tm.tm_hour, tm.tm_min, tm.tm_sec,
+			    (long)tv.tv_usec);

Just bikeshedding, but can we have tokens that mostly sort numeric-wise by time order? So time at the start, not the flush_count/getpid.

Maybe I'm missing something, but couldn't we just re-use the trace2 SID
+ a more trivial trailer? It would have the nice property that you could find the trace2 SID whenever you looked at such a token (could e.g. split them by "/" too), and add the tv_usec, flush_count+whatever else is needed to make it unique after the "/", no?

Hi Jeff,

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

Jeff Hostetler <[email protected]>

the win32 backend to register a watch on the working tree irectory (recursively). Also watch the <gitdir> if it is side the working tree. And to collect path change notifications atches and publish.

-off-by: Jeff Hostetler <[email protected]>

t/fsmonitor/fsmonitor-fs-listen-win32.c | 530 +++++++++++++++++++ e changed, 530 insertions(+)

Quoted 10 lines

compat/fsmonitor/fsmonitor-fs-listen-win32.c +5 -0

index 880446b49e3..d707d47a0d7 100644
@@ -2,20 +2,550 @@
+ [...]
+
+static struct one_watch *create_watch(struct fsmonitor_daemon_state *state,
+				      const char *path)
+{

compat/fsmonitor/fsmonitor-fs-listen-win32.c line 16

Quoted 10 lines

compat/fsmonitor/fsmonitor-fs-listen-win32.c +10 -0

+	struct one_watch *watch = NULL;
+	DWORD desired_access = FILE_LIST_DIRECTORY;
+	DWORD share_mode =
+		FILE_SHARE_WRITE | FILE_SHARE_READ | FILE_SHARE_DELETE;
+	HANDLE hDir;
+
+	hDir = CreateFileA(path,
+			   desired_access, share_mode, NULL, OPEN_EXISTING,
+			   FILE_FLAG_BACKUP_SEMANTICS | FILE_FLAG_OVERLAPPED,
+			   NULL);

The *A() family of Win32 API functions disagree with Git in one very interesting aspect: Git always assumes UTF-8, while e.g. CreateFileA() will use the current Win32 locale to internally transform to wide characters and then call CreateFileW().

This poses no problem when your locale is US American and your paths contain no non-ASCII characters.

In the Git for Windows bug tracker, it was reported that it _does_ cause problems when venturing outside such a cozy scenario (for full details, see https://github.com/git-for-windows/git/issues/3262)

I need this (and merged it before starting the process to release Git for Windows v2.32.0(2)) to fix that (could I ask you to integrate this in case a re-roll will become necessary?):

-- snipsnap --
From: Johannes Schindelin <[email protected]>
Date: Mon, 5 Jul 2021 13:51:05 +0200
Subject: [PATCH] fixup! fsmonitor-fs-listen-win32: implement FSMonitor backend
on Windows

Let's keep avoiding the *A() family of Win32 API functions because they are susceptible to incoherent encoding problems. In Git for Windows, we always assume paths to be UTF-8 encoded. Let's use the dedicated helper to convert such a path to the wide character version, and then use the *W() function instead.

Signed-off-by Johannes Schindelin <[email protected]>

---
  compat/fsmonitor/fsmonitor-fs-listen-win32.c | 8 +++++++-
  1 file changed, 7 insertions(+), 1 deletion(-)

compat/fsmonitor/fsmonitor-fs-listen-win32.c +7 -2

index ba087b292df..3b42ab311d9 100644
@@ -111,8 +111,14 @@ static struct one_watch *create_watch(struct fsmonitor_daemon_state *state,
  	DWORD share_mode =
  		FILE_SHARE_WRITE | FILE_SHARE_READ | FILE_SHARE_DELETE;
  	HANDLE hDir;
+	wchar_t wpath[MAX_PATH];
 
-	hDir = CreateFileA(path,
+	if (xutftowcs_path(wpath, path) < 0) {
+		error(_("could not convert to wide characters: '%s'"), path);
+		return NULL;
+	}
+
+	hDir = CreateFileW(wpath,
  			   desired_access, share_mode, NULL, OPEN_EXISTING,
  			   FILE_FLAG_BACKUP_SEMANTICS | FILE_FLAG_OVERLAPPED,
  			   NULL);
--

2.32.0.windows.1.15.gf1590a75e2d

Quoted 10 lines · Jeff Hostetler via GitGitGadget

From: Jeff Hostetler <[email protected]>

Teach the win32 backend to register a watch on the working tree root directory (recursively). Also watch the <gitdir> if it is not inside the working tree. And to collect path change notifications into batches and publish.

Signed-off-by Jeff Hostetler <[email protected]>

---
  compat/fsmonitor/fsmonitor-fs-listen-win32.c | 530 +++++++++++++++++++

<bikeshed mode> Spying on the early history of this (looking for the Linux backend) I saw that at some point we had just compat/fsmonitor/linux.c, and presumably some of compat/fsmonitor/{windows,win32,macos,darwin}.c.

At some point those filenames became much much longer.

I've noticed you tend to prefer really long file and function names, e.g. your borrowed daemonize() became spawn_background_fsmonitor_daemon(), I think aiming for shorter filenames & function names helps, e.g. these long names widen diffstats, and many people who hack on the code stick religiously to 80 character width terminals.

Quoted 9 lines · Jeff Hostetler via GitGitGadget

t/helper/test-touch.c new file +3 -0

new file mode 100644
index 00000000000..e9b3b754f1f
@@ -0,0 +1,126 @@
+/*
+ * test-touch.c: variation on /usr/bin/touch to speed up tests
+ * with a large number of files (primarily on Windows where child

t/helper/test-touch.c line 13

Quoted 10 lines

t/helper/test-touch.c new file +10 -0

+ * process are very, very expensive).
+ */
+
+#include "test-tool.h"
+#include "cache.h"
+#include "parse-options.h"
+
+char *seq_pattern;
+int seq_start = 1;
+int seq_count = 1;

With this in, "make sparse" dies like this:

SP t/helper/test-touch.c
t/helper/test-touch.c:11:6: error: symbol 'seq_pattern' was not declared. Should it be static? t/helper/test-touch.c:12:5: error: symbol 'seq_start' was not declared. Should it be static? t/helper/test-touch.c:13:5: error: symbol 'seq_count' was not declared. Should it be static?

Quoted 40 lines · Jeff Hostetler via GitGitGadget

From: Jeff Hostetler <[email protected]>

Change p7519 to use a single "test-tool touch" command to update the mtime on a series of (thousands) files instead of invoking thousands of commands to update a single file.

This is primarily for Windows where process creation is so very slow and reduces the test run time by minutes.

Signed-off-by Jeff Hostetler <[email protected]>

---
  t/perf/p7519-fsmonitor.sh | 14 ++++++--------
  1 file changed, 6 insertions(+), 8 deletions(-)

t/perf/p7519-fsmonitor.sh +5 -6

index 5eb5044a103..f74e6014a0a 100755
@@ -119,10 +119,11 @@ test_expect_success "one time repo setup" '
  	fi &&
 
  	mkdir 1_file 10_files 100_files 1000_files 10000_files &&
-	for i in $(test_seq 1 10); do touch 10_files/$i; done &&
-	for i in $(test_seq 1 100); do touch 100_files/$i; done &&
-	for i in $(test_seq 1 1000); do touch 1000_files/$i; done &&
-	for i in $(test_seq 1 10000); do touch 10000_files/$i; done &&
+	test-tool touch sequence --pattern="10_files/%d" --start=1 --count=10 &&
+	test-tool touch sequence --pattern="100_files/%d" --start=1 --count=100 &&
+	test-tool touch sequence --pattern="1000_files/%d" --start=1 --count=1000 &&
+	test-tool touch sequence --pattern="10000_files/%d" --start=1 --count=10000 &&
+
  	git add 1_file 10_files 100_files 1000_files 10000_files &&
  	git commit -qm "Add files" &&
 

@@ -200,15 +201,12 @@ test_fsmonitor_suite() {
  	# Update the mtimes on upto 100k files to make status think
  	# that they are dirty.  For simplicity, omit any files with
  	# LFs (i.e. anything that ls-files thinks it needs to dquote).
-	# Then fully backslash-quote the paths to capture any
-	# whitespace so that they pass thru xargs properly.

t/perf/p7519-fsmonitor.sh test_fsmonitor_suite() { · line 211

Quoted 10 lines

t/perf/p7519-fsmonitor.sh +1 -2

  	#
  	test_perf_w_drop_caches "status (dirty) ($DESC)" '
  		git ls-files | \
  			head -100000 | \
  			grep -v \" | \
-			sed '\''s/\(.\)/\\\1/g'\'' | \
-			xargs test-tool chmtime -300 &&
+			test-tool touch stdin &&
  		git status
  	'

Did you try to replace this with some variant of:

     test_seq 1 10000 | xargs touch

Which (depending on your xargs version) would invoke "touch" commands with however many argv items it thinks you can handle.

Quoted 7 lines · Ævar Arnfjörð Bjarmason

Did you try to replace this with some variant of:

     test_seq 1 10000 | xargs touch

Which (depending on your xargs version) would invoke "touch" commands with however many argv items it thinks you can handle.

a quick test on my Windows machine shows that

        test_seq 1 10000 | xargs touch

takes 3.1 seconds.

just a simple

        test_seq 1 10000 >/dev/null

take 0.2 seconds.

using my test-tool helper cuts that time in half.

Quoted 11 lines · Jeff Hostetler

a quick test on my Windows machine shows that

        test_seq 1 10000 | xargs touch

takes 3.1 seconds.

just a simple

        test_seq 1 10000 >/dev/null

Quoted 3 lines

take 0.2 seconds.

using my test-tool helper cuts that time in half.

Yeah, test_seq is pretty bad; it's just a loop in shell. Is there a 'seq' on windows, and does using it instead of test_seq make things faster with Ævar's suggested command?

I'd really like to modify test_seq to use seq when it's available and fall back to the looping-in-shell when we need to for various platforms.

Maybe it'd even make sense to write a 'test-tool seq' and make test_seq use that just so we can rip out that super lame shell looping.

Quoted 12 lines · Elijah Newren

Quoted 11 lines · Jeff Hostetler

a quick test on my Windows machine shows that

        test_seq 1 10000 | xargs touch

takes 3.1 seconds.

just a simple

        test_seq 1 10000 >/dev/null

Quoted 7 lines

Quoted 3 lines

take 0.2 seconds.

using my test-tool helper cuts that time in half.

Yeah, test_seq is pretty bad; it's just a loop in shell. Is there a 'seq' on windows, and does using it instead of test_seq make things faster with Ævar's suggested command?

Unless I am misreading Jeff's message, I do not think that makes sense. Counting to 10000 in shell loop is trivial (0.2 seconds), but letting touch invoked 10000 times to create (or smudge mtime of, but I suspect that is not what is going on here) 10000 files takes 3.1 seconds, and of course a native binary that creates 10000 files with a single invocation would be faster.

Quoted 3 lines

I'd really like to modify test_seq to use seq when it's available and fall back to the looping-in-shell when we need to for various platforms.

So, if I am reading Jeff correctly, that optimizes something that is not a bottleneck.

Quoted 94 lines · Ævar Arnfjörð Bjarmason

Quoted 81 lines · Jeff Hostetler via GitGitGadget

From: Jeff Hostetler <[email protected]>

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

Signed-off-by Jeff Hostetler <[email protected]>

---
  Documentation/git-fsmonitor--daemon.txt | 75 +++++++++++++++++++++++++
  1 file changed, 75 insertions(+)

create mode 100644 Documentation/git-fsmonitor--daemon.txt

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

new file mode 100644
index 00000000000..154e7684daa
@@ -0,0 +1,75 @@
+git-fsmonitor--daemon(1)
+========================
+
+NAME
+----
+git-fsmonitor--daemon - A Built-in File System Monitor
+
+SYNOPSIS
+--------
+[verse]
+'git fsmonitor--daemon' start
+'git fsmonitor--daemon' run
+'git fsmonitor--daemon' stop
+'git fsmonitor--daemon' status
+
+DESCRIPTION
+-----------
+
+A daemon to watch the working directory for file and directory
+changes using platform-specific file system notification facilities.
+
+This daemon 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.
+
+This daemon is built into Git so that no third-party tools are
+required.
+
+OPTIONS
+-------
+
+start::
+	Starts a daemon in the background.
+
+run::
+	Runs a daemon in the foreground.
+
+stop::
+	Stops the daemon running in the current working
+	directory, if present.
+
+status::
+	Exits with zero status if a daemon is watching the
+	current working directory.
+
+REMARKS
+-------
+
+This daemon is a long running process used to watch a single working
+directory and maintain a list of the recently changed files and
+directories.  Performance of commands such as `git status` can be
+increased if they just ask for a summary of changes to the working
+directory and can avoid scanning the disk.
+
+When `core.useBuiltinFSMonitor` is set to `true` (see
+linkgit:git-config[1]) commands, such as `git status`, will ask the
+daemon for changes and automatically start it (if necessary).
+
+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

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

Quoted 10 lines

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

+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
+modified within the working directory of a submodule, it will report
+the change (as happening against the super repo).  However, the client
+will properly ignore these extra events, so performance may be affected
+but it will not cause an incorrect result.
+
+GIT
+---
+Part of the linkgit:git[1] suite

Later in the series we incrementally add features to the daemon, so

Quoted 9 lines

this
is describing a state that doesn't exist yet at this point. I think it would be better to start with a stup here and add documentation as we add features, e.g. the patch tha adds "start" should add that to the synopsis + options etc.
See the outstanding ab/config-based-hooks-base for a small example of
that.

I like to lead the series with the documentation that summarizes the purpose of the entire feature or patch series. This gives the reviewer the context for the complete series that follows. In the past, we've had discussions on the list about how hard it is to review a series when the foo.c comes (alphabetically) before foo.h in the patch and all the documentation is attached to the prototypes in the .h file so the reviewer needs to bounce around in the patch or series to read the intent and then go back to the beginning to read the code. In that spirit, I think that having the complete man page come first provides necessary context and is helpful.

The argument that the man-page should grow as the feature grows presumes that there is a meaningful cut-point mid-series where you would adopt the first portion and delay the second to a later release or something. That division would not be useful/usable.

And it just clutters up later commits in the series with man-page deltas.

So I'd like to keep it as it unless there are further objections.

Quoted 20 lines · Jeff Hostetler

Quoted 7 lines · Ævar Arnfjörð Bjarmason

Did you try to replace this with some variant of:

      test_seq 1 10000 | xargs touch

Which (depending on your xargs version) would invoke "touch" commands with however many argv items it thinks you can handle.

a quick test on my Windows machine shows that

         test_seq 1 10000 | xargs touch

takes 3.1 seconds.

just a simple

         test_seq 1 10000 >/dev/null

Quoted 3 lines

take 0.2 seconds.

using my test-tool helper cuts that time in half.

Yeah, test_seq is pretty bad; it's just a loop in shell. Is there a 'seq' on windows, and does using it instead of test_seq make things faster with Ævar's suggested command?

I'd really like to modify test_seq to use seq when it's available and fall back to the looping-in-shell when we need to for various platforms.

Maybe it'd even make sense to write a 'test-tool seq' and make test_seq use that just so we can rip out that super lame shell looping.

From: Jeff Hostetler <[email protected]>

Change p7519 to use a single "test-tool touch" command to update the mtime on a series of (thousands) files instead of invoking thousands of commands to update a single file.

This is primarily for Windows where process creation is so very slow and reduces the test run time by minutes.

Signed-off-by Jeff Hostetler <[email protected]>

---
  t/perf/p7519-fsmonitor.sh | 14 ++++++--------
  1 file changed, 6 insertions(+), 8 deletions(-)

t/perf/p7519-fsmonitor.sh +0 -0

index 5eb5044a103..f74e6014a0a 100755
@@ -119,10 +119,11 @@ test_expect_success "one time repo setup" '
  	fi &&

FWIW, the xargs is clustering the 10,000 files into ~4 command lines, so there is a little bit of Windows process overhead, but not that much.

seq 1 10000 | xargs wc -l | grep total

Quoted 7 lines

I'd really like to modify test_seq to use seq when it's available and fall back to the looping-in-shell when we need to for various platforms.
Maybe it'd even make sense to write a 'test-tool seq' and make test_seq use that just so we can rip out that super lame shell looping.

My response here is in addition to Dscho's remarks on this topic. He makes excellent points that I'll just #include here. I do want to add my own $0.02 here.

Quoted 80 lines · Ævar Arnfjörð Bjarmason

Quoted 68 lines · Jeff Hostetler via GitGitGadget

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

+#ifdef GIT_WINDOWS_NATIVE
+/*
+ * Create a background process to run the daemon.  It should be completely
+ * disassociated from the terminal.
+ *
+ * Conceptually like `daemonize()` but different because Windows does not
+ * have `fork(2)`.  Spawn a normal Windows child process but without the
+ * limitations of `start_command()` and `finish_command()`.
+ *
+ * The child process execs the "git fsmonitor--daemon run" command.
+ *
+ * The current process returns so that the caller can wait for the child
+ * to startup before exiting.
+ */
+static int spawn_background_fsmonitor_daemon(pid_t *pid)
+{
+	char git_exe[MAX_PATH];
+	struct strvec args = STRVEC_INIT;
+	int in, out;
+
+	GetModuleFileNameA(NULL, git_exe, MAX_PATH);
+
+	in = open("/dev/null", O_RDONLY);
+	out = open("/dev/null", O_WRONLY);
+
+	strvec_push(&args, git_exe);
+	strvec_push(&args, "fsmonitor--daemon");
+	strvec_push(&args, "run");
+	strvec_pushf(&args, "--ipc-threads=%d", fsmonitor__ipc_threads);
+
+	*pid = mingw_spawnvpe(args.v[0], args.v, NULL, NULL, in, out, out);
+	close(in);
+	close(out);
+
+	strvec_clear(&args);
+
+	if (*pid < 0)
+		return error(_("could not spawn fsmonitor--daemon in the background"));
+
+	return 0;
+}
+#else
+/*
+ * Create a background process to run the daemon.  It should be completely
+ * disassociated from the terminal.
+ *
+ * This is adapted from `daemonize()`.  Use `fork()` to directly
+ * create and run the daemon in the child process.
+ *
+ * The fork-child can just call the run code; it does not need to exec
+ * it.
+ *
+ * The fork-parent returns the child PID so that we can wait for the
+ * child to startup before exiting.
+ */
+static int spawn_background_fsmonitor_daemon(pid_t *pid)
+{
+	*pid = fork();
+
+	switch (*pid) {
+	case 0:
+		if (setsid() == -1)
+			error_errno(_("setsid failed"));
+		close(0);
+		close(1);
+		close(2);
+		sanitize_stdfds();
+

builtin/fsmonitor--daemon.c static int try_to_run_foreground_daemon(void) · line 349

Quoted 10 lines

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

+		return !!fsmonitor_run_daemon();
+
+	case -1:
+		return error_errno(_("could not spawn fsmonitor--daemon in the background"));
+
+	default:
+		return 0;
+	}
+}
+#endif

Quoted 9 lines

The spawn_background_fsmonitor_daemon() function here is almost the same
as daemonize(). I wonder if this & the Windows-specific one you have here can't be refactored into an API from what's now in setup.c. Then we could make builtin/gc.c and daemon.c use that, so Windows could
have background GC, and we'd have a more battle-tested central codepath for this tricky bit.

I'd rather not refactor all of this and add unnecessary generality and complexity just to save duplicating some of the code in daemonize().

And I'd rather not destabilize existing commands like gc and daemon by changing the daemonize() layer on them. If those commands need help, let's have a separate conversation _later_ about what help they need and if it makes sense to combine them.

Quoted 4 lines

It seems to me like the only limitations on it are to have this return slightly more general things (e.g. not set its own errors, return structured data), and maybe some callback for what to do in the child/parent.

There are several issues here when trying to start a background process and we're already on the edge of the behavioral differences between Windows and Unix -- let's not make things more confusing with multiple callbacks, returning structures, custom errors, and etc.

Also, since Windows doesn't do fork(), we don't have child/parent branches in the call, so this whole "just pretend it's all Unix" model doesn't work.

Even if we did pretend I'd still need ifdef'd callback routines to either call fsmonitor_run_daemon() or build a command line (or have blocks of functions that "just happen to never be called on one platform or the other").

What I have here is an API that the primary (read: parent) calls and gets back a 0 or -1 (with error message). And that's it. The primary can then wait for the child (whether from fork or CreateProcess) to become responsive or fail to start. And then the primary can exit (with or without error).

So I think we're good. Yes, there is an ifdef here, but I think it is worth it.

Quoted 71 lines

Quoted 59 lines

builtin/fsmonitor--daemon.c +59 -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;
+
+	time(&time_limit);
+	time_limit += fsmonitor__start_timeout_sec;
+
+	for (;;) {
+		pid_seen = waitpid(pid_child, &status, WNOHANG);
+
+		if (pid_seen == -1)
+			return error_errno(_("waitpid failed"));
+		else if (pid_seen == 0) {
+			/*
+			 * The child is still running (this should be
+			 * the normal case).  Try to connect to it on
+			 * the socket and see if it is ready for
+			 * business.
+			 *
+			 * If there is another daemon already running,
+			 * our child will fail to start (possibly
+			 * after a timeout on the lock), but we don't
+			 * care (who responds) if the socket is live.
+			 */
+			s = fsmonitor_ipc__get_state();
+			if (s == IPC_STATE__LISTENING)
+				return 0;
+
+			time(&now);
+			if (now > time_limit)
+				return error(_("fsmonitor--daemon not online yet"));
+		} else if (pid_seen == pid_child) {
+			/*
+			 * The new child daemon process shutdown while
+			 * it was starting up, so it is not listening
+			 * on the socket.
+			 *
+			 * Try to ping the socket in the odd chance
+			 * that another daemon started (or was already
+			 * running) while our child was starting.
+			 *
+			 * Again, we don't care who services the socket.
+			 */
+			s = fsmonitor_ipc__get_state();
+			if (s == IPC_STATE__LISTENING)
+				return 0;
+
+			/*
+			 * We don't care about the WEXITSTATUS() nor
+			 * any of the WIF*(status) values because
+			 * `cmd_fsmonitor__daemon()` does the `!!result`

builtin/fsmonitor--daemon.c static int try_to_run_foreground_daemon(void) · line 419

Quoted 10 lines

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

+			 * trick on all function return values.
+			 *
+			 * So it is sufficient to just report the
+			 * early shutdown as an error.
+			 */
+			return error(_("fsmonitor--daemon failed to start"));
+		} else
+			return error(_("waitpid is confused"));
+	}
+}

Ditto this. could we extend the wait_or_whine() function (or some extended version thereof) to do what you need with callbacks?

Quoted 9 lines

It seems the main difference is just being able to pass down a flag for
waitpid(), and the loop needing to check EINTR or not depending on whether WNOHANG is passed.
For e.g. the "We don't care about the WEXITSTATUS()" you'd get that behavior with an adjusted wait_or_whine(). Wouldn't it be better to report what exit status it exits with e.g. if the top-level process is signalled? We do so in trace2 for other things we spawn...

Again, I don't want to mix my usage here with the existing code and destabilize all existing callers. Here we are spinning to give the child a chance to *start* and confirm that it is in a listening state and ready for connections. We do not wait for the child to exit (unless it dies quickly without becoming ready).

We want to end our wait as soon as we confirm that the child is ready and return. All I really need from the system is waitpid().

Quoted 13 lines · Ævar Arnfjörð Bjarmason

Quoted 11 lines · Jeff Hostetler via GitGitGadget

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

+	if (!test_env_value) {
+		struct timeval tv;
+		struct tm tm;
+		time_t secs;
+
+		gettimeofday(&tv, NULL);
+		secs = tv.tv_sec;
+		gmtime_r(&secs, &tm);
+
+		strbuf_addf(&token->token_id,
+			    "%"PRIu64".%d.%4d%02d%02dT%02d%02d%02d.%06ldZ",

Quoted 8 lines

builtin/fsmonitor--daemon.c static int do_as_client__status(void) · line 202

Quoted 5 lines

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

+			    flush_count++,
+			    getpid(),
+			    tm.tm_year + 1900, tm.tm_mon + 1, tm.tm_mday,
+			    tm.tm_hour, tm.tm_min, tm.tm_sec,
+			    (long)tv.tv_usec);

Just bikeshedding, but can we have tokens that mostly sort numeric-wise
by time order? So time at the start, not the flush_count/getpid.

As I described in a rather large comment in the code, tokens are opaque strings -- without a less-than / greater-than relationship -- just a random string that the daemon can use (along with a sequence number) to ensure that a later request is well-defined.

Here I'm using a counter, pid, and date-stamp. I'd prefer using a GUID or UUID just to drive that home, but I didn't want to add a new .lib or .a to the build if not necessary.

Perhaps I should compute this portion as hex(hash(time())) to remove the temptation to look inside my opaque token ??

Quoted 7 lines

Maybe I'm missing something, but couldn't we just re-use the trace2 SID
+ a more trivial trailer? It would have the nice property that you could find the trace2 SID whenever you looked at such a token (could e.g. split them by "/" too), and add the tv_usec, flush_count+whatever else is needed to make it unique after the "/", no?

I would rather keep Trace2 out of this. The SID is another opaque string and I don't want to reach inside it.

Quoted 9 lines · Ævar Arnfjörð Bjarmason

Quoted 7 lines · Jeff Hostetler via GitGitGadget

From: Jeff Hostetler <[email protected]>

Teach the win32 backend to register a watch on the working tree root directory (recursively). Also watch the <gitdir> if it is not inside the working tree. And to collect path change notifications into batches and publish.

Quoted 9 lines

Quoted 3 lines

Signed-off-by Jeff Hostetler <[email protected]>

---
  compat/fsmonitor/fsmonitor-fs-listen-win32.c | 530 +++++++++++++++++++

<bikeshed mode> Spying on the early history of this (looking for the Linux backend) I saw that at some point we had just compat/fsmonitor/linux.c, and presumably some of compat/fsmonitor/{windows,win32,macos,darwin}.c.
At some point those filenames became much much longer.

Once upon a time having "foo/bar/win32.c" and "abc/def/win32.c" would cause confusion in the debugger (I've long since forgotten which). Breaking at win32.c:30 was no longer unique.

Also, if the Makefile sends all .o's to the root directory or a unified OBJS directory rather than to the subdir containing the .c, then we have another issue during linking...

So, having been burned too many times, I prefer to make source filenames unique when possible.

Quoted 7 lines

I've noticed you tend to prefer really long file and function names, e.g. your borrowed daemonize() became spawn_background_fsmonitor_daemon(), I think aiming for shorter filenames & function names helps, e.g. these long names widen diffstats, and many people who hack on the code stick religiously to 80 character width terminals.

I prefer self-documenting code.

Quoted 4 lines · Jeff Hostetler

FWIW, the xargs is clustering the 10,000 files into ~4 command lines, so there is a little bit of Windows process overhead, but not that much.

Quoted 9 lines

seq 1 10000 | xargs wc -l | grep total

Quoted 7 lines

I'd really like to modify test_seq to use seq when it's available and fall back to the looping-in-shell when we need to for various platforms.
Maybe it'd even make sense to write a 'test-tool seq' and make test_seq use that just so we can rip out that super lame shell looping.

So what lame in this picture is not shell, or process overhead, but I/O performance.

I've seen some noises about Windows file creation performance raised as an issue when doing initial checkout followed by "git clone", and an idea floated to create a bunch of open file handles for writing in threads when checkout (really the caller that repeatedly calls entry.c:write_entry() by iterating the in-core index) starts, and write out the contents in parallel, as a workaround. When I heard it, I somehow thought it was meant as a not-so-funny joke, but from the sounds of it, the I/O performance may be so horrible to require such a hack to be usable there. Sigh...

Quoted 51 lines · Ævar Arnfjörð Bjarmason

Quoted 38 lines · Jeff Hostetler via GitGitGadget

From: Jeff Hostetler <[email protected]>

Change p7519 to use a single "test-tool touch" command to update the mtime on a series of (thousands) files instead of invoking thousands of commands to update a single file.

This is primarily for Windows where process creation is so very slow and reduces the test run time by minutes.

Signed-off-by Jeff Hostetler <[email protected]>

---
  t/perf/p7519-fsmonitor.sh | 14 ++++++--------
  1 file changed, 6 insertions(+), 8 deletions(-)

t/perf/p7519-fsmonitor.sh +5 -4

index 5eb5044a103..f74e6014a0a 100755
@@ -119,10 +119,11 @@ test_expect_success "one time repo setup" '
  	fi &&
    	mkdir 1_file 10_files 100_files 1000_files 10000_files &&
-	for i in $(test_seq 1 10); do touch 10_files/$i; done &&
-	for i in $(test_seq 1 100); do touch 100_files/$i; done &&
-	for i in $(test_seq 1 1000); do touch 1000_files/$i; done &&
-	for i in $(test_seq 1 10000); do touch 10000_files/$i; done &&
+	test-tool touch sequence --pattern="10_files/%d" --start=1 --count=10 &&
+	test-tool touch sequence --pattern="100_files/%d" --start=1 --count=100 &&
+	test-tool touch sequence --pattern="1000_files/%d" --start=1 --count=1000 &&
+	test-tool touch sequence --pattern="10000_files/%d" --start=1 --count=10000 &&
+
  	git add 1_file 10_files 100_files 1000_files 10000_files &&
  	git commit -qm "Add files" &&
  @@ -200,15 +201,12 @@ test_fsmonitor_suite() {
  	# Update the mtimes on upto 100k files to make status think

  	# that they are dirty.  For simplicity, omit any files with
  	# LFs (i.e. anything that ls-files thinks it needs to dquote).

t/perf/p7519-fsmonitor.sh +0 -2

-	# Then fully backslash-quote the paths to capture any
-	# whitespace so that they pass thru xargs properly.

t/perf/p7519-fsmonitor.sh test_fsmonitor_suite() { · line 211

Quoted 10 lines

t/perf/p7519-fsmonitor.sh +1 -2

  	#
  	test_perf_w_drop_caches "status (dirty) ($DESC)" '
  		git ls-files | \
  			head -100000 | \
  			grep -v \" | \
-			sed '\''s/\(.\)/\\\1/g'\'' | \
-			xargs test-tool chmtime -300 &&
+			test-tool touch stdin &&
  		git status
  	'

Did you try to replace this with some variant of:

Quoted 5 lines

     test_seq 1 10000 | xargs touch

Which (depending on your xargs version) would invoke "touch" commands
with however many argv items it thinks you can handle.

a quick test on my Windows machine shows that

	test_seq 1 10000 | xargs touch

takes 3.1 seconds.

just a simple

	test_seq 1 10000 >/dev/null

take 0.2 seconds.

using my test-tool helper cuts that time in half.

Quoted 95 lines · Jeff Hostetler

My response here is in addition to Dscho's remarks on this topic. He makes excellent points that I'll just #include here. I do want to add my own $0.02 here.

Quoted 74 lines · Ævar Arnfjörð Bjarmason

Quoted 59 lines · Jeff Hostetler via GitGitGadget

builtin/fsmonitor--daemon.c +59 -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;
+
+	time(&time_limit);
+	time_limit += fsmonitor__start_timeout_sec;
+
+	for (;;) {
+		pid_seen = waitpid(pid_child, &status, WNOHANG);
+
+		if (pid_seen == -1)
+			return error_errno(_("waitpid failed"));
+		else if (pid_seen == 0) {
+			/*
+			 * The child is still running (this should be
+			 * the normal case).  Try to connect to it on
+			 * the socket and see if it is ready for
+			 * business.
+			 *
+			 * If there is another daemon already running,
+			 * our child will fail to start (possibly
+			 * after a timeout on the lock), but we don't
+			 * care (who responds) if the socket is live.
+			 */
+			s = fsmonitor_ipc__get_state();
+			if (s == IPC_STATE__LISTENING)
+				return 0;
+
+			time(&now);
+			if (now > time_limit)
+				return error(_("fsmonitor--daemon not online yet"));
+		} else if (pid_seen == pid_child) {
+			/*
+			 * The new child daemon process shutdown while
+			 * it was starting up, so it is not listening
+			 * on the socket.
+			 *
+			 * Try to ping the socket in the odd chance
+			 * that another daemon started (or was already
+			 * running) while our child was starting.
+			 *
+			 * Again, we don't care who services the socket.
+			 */
+			s = fsmonitor_ipc__get_state();
+			if (s == IPC_STATE__LISTENING)
+				return 0;
+
+			/*
+			 * We don't care about the WEXITSTATUS() nor
+			 * any of the WIF*(status) values because
+			 * `cmd_fsmonitor__daemon()` does the `!!result`

builtin/fsmonitor--daemon.c static int try_to_run_foreground_daemon(void) · line 419

Quoted 10 lines

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

+			 * trick on all function return values.
+			 *
+			 * So it is sufficient to just report the
+			 * early shutdown as an error.
+			 */
+			return error(_("fsmonitor--daemon failed to start"));
+		} else
+			return error(_("waitpid is confused"));
+	}
+}

Ditto this. could we extend the wait_or_whine() function (or some extended version thereof) to do what you need with callbacks?

Quoted 9 lines

It seems the main difference is just being able to pass down a flag for
waitpid(), and the loop needing to check EINTR or not depending on whether WNOHANG is passed.
For e.g. the "We don't care about the WEXITSTATUS()" you'd get that behavior with an adjusted wait_or_whine(). Wouldn't it be better to report what exit status it exits with e.g. if the top-level process is signalled? We do so in trace2 for other things we spawn...

Again, I don't want to mix my usage here with the existing code and destabilize all existing callers. Here we are spinning to give the child a chance to *start* and confirm that it is in a listening state and ready for connections. We do not wait for the child to exit (unless it dies quickly without becoming ready).

Quoted 2 lines

We want to end our wait as soon as we confirm that the child is ready and return. All I really need from the system is waitpid().

Will this code behave correctly if the daemon we start is signalled per the WIFSIGNALED() cases the code this is derived handles, but this does not?

But sure, I just meant to point out that the flip side to "destabilize all existing callers" is reviewing new code that may be subtly buggy, and those subtle bugs (if any) would be smoked out if we were forced to extend run-command.c, i.e. to use whatever feature(s) this needs for all existing callers.

Quoted 64 lines · Jeff Hostetler

Quoted 51 lines · Ævar Arnfjörð Bjarmason

Quoted 38 lines · Jeff Hostetler via GitGitGadget

From: Jeff Hostetler <[email protected]>

Change p7519 to use a single "test-tool touch" command to update the mtime on a series of (thousands) files instead of invoking thousands of commands to update a single file.

This is primarily for Windows where process creation is so very slow and reduces the test run time by minutes.

Signed-off-by Jeff Hostetler <[email protected]>

---
   t/perf/p7519-fsmonitor.sh | 14 ++++++--------
   1 file changed, 6 insertions(+), 8 deletions(-)

t/perf/p7519-fsmonitor.sh +5 -4

index 5eb5044a103..f74e6014a0a 100755
@@ -119,10 +119,11 @@ test_expect_success "one time repo setup" '
   	fi &&
     	mkdir 1_file 10_files 100_files 1000_files 10000_files &&
-	for i in $(test_seq 1 10); do touch 10_files/$i; done &&
-	for i in $(test_seq 1 100); do touch 100_files/$i; done &&
-	for i in $(test_seq 1 1000); do touch 1000_files/$i; done &&
-	for i in $(test_seq 1 10000); do touch 10000_files/$i; done &&
+	test-tool touch sequence --pattern="10_files/%d" --start=1 --count=10 &&
+	test-tool touch sequence --pattern="100_files/%d" --start=1 --count=100 &&
+	test-tool touch sequence --pattern="1000_files/%d" --start=1 --count=1000 &&
+	test-tool touch sequence --pattern="10000_files/%d" --start=1 --count=10000 &&
+
   	git add 1_file 10_files 100_files 1000_files 10000_files &&
   	git commit -qm "Add files" &&
   @@ -200,15 +201,12 @@ test_fsmonitor_suite() {
   	# Update the mtimes on upto 100k files to make status think

   	# that they are dirty.  For simplicity, omit any files with
   	# LFs (i.e. anything that ls-files thinks it needs to dquote).

t/perf/p7519-fsmonitor.sh +0 -2

-	# Then fully backslash-quote the paths to capture any
-	# whitespace so that they pass thru xargs properly.

t/perf/p7519-fsmonitor.sh test_fsmonitor_suite() { · line 211

Quoted 10 lines

t/perf/p7519-fsmonitor.sh +1 -2

   	#
   	test_perf_w_drop_caches "status (dirty) ($DESC)" '
   		git ls-files | \
   			head -100000 | \
   			grep -v \" | \
-			sed '\''s/\(.\)/\\\1/g'\'' | \
-			xargs test-tool chmtime -300 &&
+			test-tool touch stdin &&
   		git status
   	'

Did you try to replace this with some variant of:

Quoted 5 lines

      test_seq 1 10000 | xargs touch

Which (depending on your xargs version) would invoke "touch" commands
with however many argv items it thinks you can handle.

a quick test on my Windows machine shows that

	test_seq 1 10000 | xargs touch

takes 3.1 seconds.

Quoted 7 lines

just a simple

	test_seq 1 10000 >/dev/null

take 0.2 seconds.

using my test-tool helper cuts that time in half.

There's what Elijah mentioned about test_seq, so maybe it's just that.

But what I was suggesting was using the xargs mode where it does N arguments at a time.

Does this work for you, and does it cause xargs to invoke "touch" with the relevant N number of arguments, and does it help with the performance?

     test_seq 1 10000 | xargs touch
     test_seq 1 10000 | xargs -n 10 touch
     test_seq 1 10000 | xargs -n 100 touch
     test_seq 1 10000 | xargs -n 1000 touch

etc.

Also I didn't notice this before, but the -300 part of "chmtime -300" was redundant before then? I.e. you're implicitly changing it to "=+0" instead with your "touch" helper, are you not?

[snip Ævar's suggestion to populate the manual page incrementally, interspersed with the commits that finalize implementing the respective functionality]

I suggested this because I think it's much easier to read patches that are larger because they incrementally update docs or tests along with code, than smaller ones that are e.g. "add docs" followed by incrementally modifying the code.

That's because you can consider those atomically.

If earlier doc changes refer to later code changes you're left jumping back & forth and wondering if the code you're reading that doesn't match the docs yet is a bug, or if it's solved in some later change you're now needing to mentally keep track of.

Which is not some theoretical concern b.t.w., but exactly what I found myself doing when reading this series, hence the suggestion.

Quoted 44 lines · Jeff Hostetler via GitGitGadget

From: Jeff Hostetler <[email protected]>

Stub in empty backend for fsmonitor--daemon on Windows.

Signed-off-by Jeff Hostetler <[email protected]>

---
 Makefile                                     | 13 ++++++
 compat/fsmonitor/fsmonitor-fs-listen-win32.c | 21 +++++++++
 compat/fsmonitor/fsmonitor-fs-listen.h       | 49 ++++++++++++++++++++
 config.mak.uname                             |  2 +
 contrib/buildsystems/CMakeLists.txt          |  5 ++
 5 files changed, 90 insertions(+)
 create mode 100644 compat/fsmonitor/fsmonitor-fs-listen-win32.c
 create mode 100644 compat/fsmonitor/fsmonitor-fs-listen.h

Makefile +10 -0

index c45caacf2c3..a2a6e1f20f6 100644
@@ -467,6 +467,11 @@ all::
 # directory, and the JSON compilation database 'compile_commands.json' will be
 # created at the root of the repository.
 #
+# If your platform supports a built-in fsmonitor backend, set
+# FSMONITOR_DAEMON_BACKEND to the "<name>" of the corresponding
+# `compat/fsmonitor/fsmonitor-fs-listen-<name>.c` that implements the
+# `fsmonitor_fs_listen__*()` routines.
+#
 # Define DEVELOPER to enable more compiler warnings. Compiler version
 # and family are auto detected, but could be overridden by defining
 # COMPILER_FEATURES (see config.mak.dev). You can still set

@@ -1929,6 +1934,11 @@ ifdef NEED_ACCESS_ROOT_HANDLER
 	COMPAT_OBJS += compat/access.o
 endif
 
+ifdef FSMONITOR_DAEMON_BACKEND
+	COMPAT_CFLAGS += -DHAVE_FSMONITOR_DAEMON_BACKEND
+	COMPAT_OBJS += compat/fsmonitor/fsmonitor-fs-listen-$(FSMONITOR_DAEMON_BACKEND).o
+endif
+
 ifeq ($(TCLTK_PATH),)
 NO_TCLTK = NoThanks
 endif

@@ -2793,6 +2803,9 @@ GIT-BUILD-OPTIONS: FORCE

Makefile GIT-BUILD-OPTIONS: FORCE · line 2808

Quoted 6 lines

Makefile +3 -0

 	@echo PAGER_ENV=\''$(subst ','\'',$(subst ','\'',$(PAGER_ENV)))'\' >>$@+
 	@echo DC_SHA1=\''$(subst ','\'',$(subst ','\'',$(DC_SHA1)))'\' >>$@+
 	@echo X=\'$(X)\' >>$@+
+ifdef FSMONITOR_DAEMON_BACKEND
+	@echo FSMONITOR_DAEMON_BACKEND=\''$(subst ','\'',$(subst ','\'',$(FSMONITOR_DAEMON_BACKEND)))'\' >>$@+
+endif

Why put this in an ifdef?

Quoted 44 lines · Jeff Hostetler via GitGitGadget

From: Jeff Hostetler <[email protected]>

Stub in empty implementation of fsmonitor--daemon
backend for MacOS.

Signed-off-by Jeff Hostetler <[email protected]>

---
 compat/fsmonitor/fsmonitor-fs-listen-macos.c | 20 ++++++++++++++++++++
 config.mak.uname                             |  2 ++
 contrib/buildsystems/CMakeLists.txt          |  3 +++
 3 files changed, 25 insertions(+)
 create mode 100644 compat/fsmonitor/fsmonitor-fs-listen-macos.c

compat/fsmonitor/fsmonitor-fs-listen-macos.c new file +20 -0

new file mode 100644
index 00000000000..b91058d1c4f
@@ -0,0 +1,20 @@
+#include "cache.h"
+#include "fsmonitor.h"
+#include "fsmonitor-fs-listen.h"
+
+int fsmonitor_fs_listen__ctor(struct fsmonitor_daemon_state *state)
+{
+	return -1;
+}
+
+void fsmonitor_fs_listen__dtor(struct fsmonitor_daemon_state *state)
+{
+}
+
+void fsmonitor_fs_listen__stop_async(struct fsmonitor_daemon_state *state)
+{
+}
+
+void fsmonitor_fs_listen__loop(struct fsmonitor_daemon_state *state)
+{
+}

config.mak.uname +0 -0

index fcd88b60b14..394355463e1 100644

config.mak.uname ifeq ($(uname_S),Darwin) · line 150

Quoted 4 lines

config.mak.uname +1 -0

@@ -147,6 +147,8 @@ ifeq ($(uname_S),Darwin)
 			MSGFMT = /usr/local/opt/gettext/bin/msgfmt
 		endif
 	endif
+	FSMONITOR_DAEMON_BACKEND = macos

A rather trivial point, but can't we pick one of "macos" or "darwin" (I'd think going with the existing uname is better) and name the file after the uname (or lower-case thereof)?

Makes these make rules more consistent too, we could just set this to "YesPlease" here, and then lower case the uname for the file compilation/include.

Quoted 24 lines · Jeff Hostetler

Quoted 9 lines · Ævar Arnfjörð Bjarmason

Quoted 7 lines · Jeff Hostetler via GitGitGadget

From: Jeff Hostetler <[email protected]>

Teach the win32 backend to register a watch on the working tree root directory (recursively). Also watch the <gitdir> if it is not inside the working tree. And to collect path change notifications into batches and publish.

Quoted 9 lines

Quoted 3 lines

Signed-off-by Jeff Hostetler <[email protected]>

---
  compat/fsmonitor/fsmonitor-fs-listen-win32.c | 530 +++++++++++++++++++

<bikeshed mode> Spying on the early history of this (looking for the Linux backend) I saw that at some point we had just compat/fsmonitor/linux.c, and presumably some of compat/fsmonitor/{windows,win32,macos,darwin}.c.
At some point those filenames became much much longer.

Once upon a time having "foo/bar/win32.c" and "abc/def/win32.c" would cause confusion in the debugger (I've long since forgotten which). Breaking at win32.c:30 was no longer unique.

Quoted 6 lines

Also, if the Makefile sends all .o's to the root directory or a unified OBJS directory rather than to the subdir containing the .c, then we have another issue during linking...

So, having been burned too many times, I prefer to make source filenames unique when possible.

A much shorter name like compat/fsmonitor/fsmon-win32.c would achieve that goal.

Quoted 8 lines

Quoted 7 lines

I've noticed you tend to prefer really long file and function names, e.g. your borrowed daemonize() became spawn_background_fsmonitor_daemon(), I think aiming for shorter filenames & function names helps, e.g. these long names widen diffstats, and many people who hack on the code stick religiously to 80 character width terminals.

Quoted 1 line

I prefer self-documenting code.

Sure, I'm not saying daemonize() is an ideal name, just suggesting that you can both get uniqueness & self-documentation and not need to split to multiple lines in some common cases to stay within the "We try to keep to at most 80 characters per line" in CodingGuidelines in this series.

Hi Ævar,

Quoted 45 lines · Ævar Arnfjörð Bjarmason

Quoted 24 lines · Jeff Hostetler

Quoted 9 lines · Ævar Arnfjörð Bjarmason

Quoted 7 lines · Jeff Hostetler via GitGitGadget

From: Jeff Hostetler <[email protected]>

Teach the win32 backend to register a watch on the working tree root directory (recursively). Also watch the <gitdir> if it is not inside the working tree. And to collect path change notifications into batches and publish.

Quoted 9 lines

Quoted 3 lines

Signed-off-by Jeff Hostetler <[email protected]>

---
  compat/fsmonitor/fsmonitor-fs-listen-win32.c | 530 +++++++++++++++++++

<bikeshed mode> Spying on the early history of this (looking for the Linux backend) I saw that at some point we had just compat/fsmonitor/linux.c, and presumably some of compat/fsmonitor/{windows,win32,macos,darwin}.c.
At some point those filenames became much much longer.

Once upon a time having "foo/bar/win32.c" and "abc/def/win32.c" would cause confusion in the debugger (I've long since forgotten which). Breaking at win32.c:30 was no longer unique.

Quoted 6 lines

Also, if the Makefile sends all .o's to the root directory or a unified OBJS directory rather than to the subdir containing the .c, then we have another issue during linking...

So, having been burned too many times, I prefer to make source filenames unique when possible.

A much shorter name like compat/fsmonitor/fsmon-win32.c would achieve that goal.

Quoted 8 lines

Quoted 7 lines

I've noticed you tend to prefer really long file and function names, e.g. your borrowed daemonize() became spawn_background_fsmonitor_daemon(), I think aiming for shorter filenames & function names helps, e.g. these long names widen diffstats, and many people who hack on the code stick religiously to 80 character width terminals.

Quoted 7 lines

Quoted 1 line

I prefer self-documenting code.

Sure, I'm not saying daemonize() is an ideal name, just suggesting that you can both get uniqueness & self-documentation and not need to split to multiple lines in some common cases to stay within the "We try to keep to at most 80 characters per line" in CodingGuidelines in this series.

While you are entitled to have your taste, I have to point out that Jeff is just as entitled to their taste, and I don't think that you can claim that yours is better.

So I wonder what the intended outcome of this review is? To make the patch better? Or to pit taste against taste?

Hi Ævar,

Quoted 55 lines · Ævar Arnfjörð Bjarmason

Quoted 44 lines · Jeff Hostetler via GitGitGadget

From: Jeff Hostetler <[email protected]>

Stub in empty implementation of fsmonitor--daemon
backend for MacOS.

Signed-off-by Jeff Hostetler <[email protected]>

---
 compat/fsmonitor/fsmonitor-fs-listen-macos.c | 20 ++++++++++++++++++++
 config.mak.uname                             |  2 ++
 contrib/buildsystems/CMakeLists.txt          |  3 +++
 3 files changed, 25 insertions(+)
 create mode 100644 compat/fsmonitor/fsmonitor-fs-listen-macos.c

compat/fsmonitor/fsmonitor-fs-listen-macos.c new file +20 -0

new file mode 100644
index 00000000000..b91058d1c4f
@@ -0,0 +1,20 @@
+#include "cache.h"
+#include "fsmonitor.h"
+#include "fsmonitor-fs-listen.h"
+
+int fsmonitor_fs_listen__ctor(struct fsmonitor_daemon_state *state)
+{
+	return -1;
+}
+
+void fsmonitor_fs_listen__dtor(struct fsmonitor_daemon_state *state)
+{
+}
+
+void fsmonitor_fs_listen__stop_async(struct fsmonitor_daemon_state *state)
+{
+}
+
+void fsmonitor_fs_listen__loop(struct fsmonitor_daemon_state *state)
+{
+}

config.mak.uname +0 -0

index fcd88b60b14..394355463e1 100644

config.mak.uname ifeq ($(uname_S),Darwin) · line 150

Quoted 4 lines

config.mak.uname +1 -0

@@ -147,6 +147,8 @@ ifeq ($(uname_S),Darwin)
 			MSGFMT = /usr/local/opt/gettext/bin/msgfmt
 		endif
 	endif
+	FSMONITOR_DAEMON_BACKEND = macos

A rather trivial point, but can't we pick one of "macos" or "darwin" (I'd think going with the existing uname is better) and name the file after the uname (or lower-case thereof)?