[v3,08/34] fsmonitor--daemon: add a built-in fsmonitor daemon

Commit Message

Jeff Hostetler July 1, 2021, 2:47 p.m. UTC

From: Jeff Hostetler <[e-mail]>

Create a built-in file system monitoring daemon that can be used by
the existing `fsmonitor` feature (protocol API and index extension)
to improve the performance of various Git commands, such as `status`.

The `fsmonitor--daemon` feature builds upon the `Simple IPC` API and
provides an alternative to hook access to existing fsmonitors such
as `watchman`.

This commit merely adds the new command without any functionality.

Co-authored-by: Johannes Schindelin <[e-mail]>
Signed-off-by: Jeff Hostetler <[e-mail]>
---
 .gitignore                  |  1 +
 Makefile                    |  1 +
 builtin.h                   |  1 +
 builtin/fsmonitor--daemon.c | 53 +++++++++++++++++++++++++++++++++++++
 git.c                       |  1 +
 5 files changed, 57 insertions(+)
 create mode 100644 builtin/fsmonitor--daemon.c

Comments

Ævar Arnfjörð Bjarmason July 1, 2021, 10:36 p.m. UTC | #1

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

A general comment on this series (including previous patches). We've
usually tried to bend over backwards in git's codebase not to have big
ifdef blocks, so we compile most code the same everywhere. We waste a
bit of object code, but that's fine.

See 9c897c5c2ad (pack-objects: remove #ifdef NO_PTHREADS, 2018-11-03)
for a good exmaple of bad code being turned to good.

E.g. in this case:

> +#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)
> +{
> +	struct option options[] = {
> +		OPT_END()
> +	};
> +
> +	if (argc == 2 && !strcmp(argv[1], "-h"))
> +		usage_with_options(builtin_fsmonitor__daemon_usage, options);
> +
> +	die(_("fsmonitor--daemon not supported on this platform"));
> +}
> +#endif

This whole thing could really just be a
-DHAVE_FSMONITOR_DAEMON_BACKEND=1 or -DHAVE_FSMONITOR_DAEMON_BACKEND=0
somewhere (depending), and then somewhere in the middle of the first
function:

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

Jeff Hostetler July 19, 2021, 8:56 p.m. UTC | #2

On 7/1/21 6:36 PM, Ævar Arnfjörð Bjarmason wrote:
> 
> On Thu, Jul 01 2021, Jeff Hostetler via GitGitGadget wrote:
> 
> A general comment on this series (including previous patches). We've
> usually tried to bend over backwards in git's codebase not to have big
> ifdef blocks, so we compile most code the same everywhere. We waste a
> bit of object code, but that's fine.
> 
> See 9c897c5c2ad (pack-objects: remove #ifdef NO_PTHREADS, 2018-11-03)
> for a good exmaple of bad code being turned to good.
> 
> E.g. in this case:
> 
>> +#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)
>> +{
>> +	struct option options[] = {
>> +		OPT_END()
>> +	};
>> +
>> +	if (argc == 2 && !strcmp(argv[1], "-h"))
>> +		usage_with_options(builtin_fsmonitor__daemon_usage, options);
>> +
>> +	die(_("fsmonitor--daemon not supported on this platform"));
>> +}
>> +#endif
> 
> This whole thing could really just be a
> -DHAVE_FSMONITOR_DAEMON_BACKEND=1 or -DHAVE_FSMONITOR_DAEMON_BACKEND=0
> somewhere (depending), and then somewhere in the middle of the first
> function:
> 
> 	if (!HAVE_FSMONITOR_DAEMON_BACKEND)
> 	    	die(_("fsmonitor--daemon not supported on this platform"));
> 

This whole file will be filled up with ~1500 lines of static functions
that only make sense when the daemon is supported and that make calls
to platform-specific backends.

I suppose we could stub in an empty backend (something like that in
11/34 and 12/34) and hack in all stuff in the makefile to link to it
in the unsupported case, but that seems like a lot of effort just to
avoid an ifdef here.

I mean, the intent of the #else block is quite clear and we're not
fooling the reader with a large source file of code that will never
be used on their platform.

We could consider splitting this source file into a supported and
unsupported version and have the makefile select the right .c file.
We'd have to move the usage and stuff to a shared header and etc.
That would eliminate the ifdef, but it would break the convention of
the source filename matching the command name.

I'm not sure it's worth the bother TBH.

Jeff

Patch

diff --git a/.gitignore b/.gitignore
index 311841f9bed..4baba472aa8 100644
--- a/.gitignore
+++ b/.gitignore
@@ -72,6 +72,7 @@ 
 /git-format-patch
 /git-fsck
 /git-fsck-objects
+/git-fsmonitor--daemon
 /git-gc
 /git-get-tar-commit-id
 /git-grep
diff --git a/Makefile b/Makefile
index 209c97aa22d..8fe1e42a435 100644
--- a/Makefile
+++ b/Makefile
@@ -1097,6 +1097,7 @@  BUILTIN_OBJS += builtin/fmt-merge-msg.o
 BUILTIN_OBJS += builtin/for-each-ref.o
 BUILTIN_OBJS += builtin/for-each-repo.o
 BUILTIN_OBJS += builtin/fsck.o
+BUILTIN_OBJS += builtin/fsmonitor--daemon.o
 BUILTIN_OBJS += builtin/gc.o
 BUILTIN_OBJS += builtin/get-tar-commit-id.o
 BUILTIN_OBJS += builtin/grep.o
diff --git a/builtin.h b/builtin.h
index 16ecd5586f0..2470d1cd3a2 100644
--- a/builtin.h
+++ b/builtin.h
@@ -159,6 +159,7 @@  int cmd_for_each_ref(int argc, const char **argv, const char *prefix);
 int cmd_for_each_repo(int argc, const char **argv, const char *prefix);
 int cmd_format_patch(int argc, const char **argv, const char *prefix);
 int cmd_fsck(int argc, const char **argv, const char *prefix);
+int cmd_fsmonitor__daemon(int argc, const char **argv, const char *prefix);
 int cmd_gc(int argc, const char **argv, const char *prefix);
 int cmd_get_tar_commit_id(int argc, const char **argv, const char *prefix);
 int cmd_grep(int argc, const char **argv, const char *prefix);
diff --git a/builtin/fsmonitor--daemon.c b/builtin/fsmonitor--daemon.c
new file mode 100644
index 00000000000..df2bad53111
--- /dev/null
+++ b/builtin/fsmonitor--daemon.c
@@ -0,0 +1,53 @@ 
+#include "builtin.h"
+#include "config.h"
+#include "parse-options.h"
+#include "fsmonitor.h"
+#include "fsmonitor-ipc.h"
+#include "simple-ipc.h"
+#include "khash.h"
+
+static const char * const builtin_fsmonitor__daemon_usage[] = {
+	NULL
+};
+
+#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)
+{
+	struct option options[] = {
+		OPT_END()
+	};
+
+	if (argc == 2 && !strcmp(argv[1], "-h"))
+		usage_with_options(builtin_fsmonitor__daemon_usage, options);
+
+	die(_("fsmonitor--daemon not supported on this platform"));
+}
+#endif
diff --git a/git.c b/git.c
index 18bed9a9964..c6160f4a886 100644
--- a/git.c
+++ b/git.c
@@ -533,6 +533,7 @@  static struct cmd_struct commands[] = {
 	{ "format-patch", cmd_format_patch, RUN_SETUP },
 	{ "fsck", cmd_fsck, RUN_SETUP },
 	{ "fsck-objects", cmd_fsck, RUN_SETUP },
+	{ "fsmonitor--daemon", cmd_fsmonitor__daemon, RUN_SETUP },
 	{ "gc", cmd_gc, RUN_SETUP },
 	{ "get-tar-commit-id", cmd_get_tar_commit_id, NO_PARSEOPT },
 	{ "grep", cmd_grep, RUN_SETUP_GENTLY },