diff mbox series

[v5,06/30] fsmonitor--daemon: add a built-in fsmonitor daemon

Message ID bdd7334da3162ce77c216d61ce9d979f12637ac5.1644612979.git.gitgitgadget@gmail.com (mailing list archive)
State Superseded
Headers show
Series Builtin FSMonitor Part 2 | expand

Commit Message

Jeff Hostetler Feb. 11, 2022, 8:55 p.m. UTC 
From: Jeff Hostetler <jeffhost@microsoft.com>

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 <johannes.schindelin@gmx.de>
Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
---
 .gitignore                  |  1 +
 Makefile                    |  1 +
 builtin.h                   |  1 +
 builtin/fsmonitor--daemon.c | 46 +++++++++++++++++++++++++++++++++++++
 git.c                       |  1 +
 5 files changed, 50 insertions(+)
 create mode 100644 builtin/fsmonitor--daemon.c

Comments

Ævar Arnfjörð Bjarmason Feb. 25, 2022, 10:46 p.m. UTC | #1 
On Fri, Feb 11 2022, Jeff Hostetler via GitGitGadget wrote:

> From: Jeff Hostetler <jeffhost@microsoft.com>
>
> 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 <johannes.schindelin@gmx.de>
> Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
> ---
>  .gitignore                  |  1 +
>  Makefile                    |  1 +
>  builtin.h                   |  1 +
>  builtin/fsmonitor--daemon.c | 46 +++++++++++++++++++++++++++++++++++++
>  git.c                       |  1 +
>  5 files changed, 50 insertions(+)
>  create mode 100644 builtin/fsmonitor--daemon.c
>
> diff --git a/.gitignore b/.gitignore
> index f817c509ec0..e81de1063a4 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 9943f0f7c11..3b7a3f88b50 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1106,6 +1106,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 83379f3832c..40e9ecc8485 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..f0498793379
> --- /dev/null
> +++ b/builtin/fsmonitor--daemon.c
> @@ -0,0 +1,46 @@
> +#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()
> +	};
> +
> +	git_config(git_default_config, NULL);
> +
> +	argc = parse_options(argc, argv, prefix, options,
> +			     builtin_fsmonitor__daemon_usage, 0);
> +	if (argc != 1)
> +		usage_with_options(builtin_fsmonitor__daemon_usage, options);
> +	subcmd = argv[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 340665d4a04..a8b44d9b587 100644
> --- a/git.c
> +++ b/git.c
> @@ -536,6 +536,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 },


I brought this up in another thread in how this series interacts with
another, but this patch below on top of "seen" would allow you to catch
parse_options() BUGs on Linux, even if you don't have a no-OSX
non-Windows backend yet:
	
	diff --git a/builtin/fsmonitor--daemon.c b/builtin/fsmonitor--daemon.c
	index 591433e897d..62c0b1d486b 100644
	--- a/builtin/fsmonitor--daemon.c
	+++ b/builtin/fsmonitor--daemon.c
	@@ -18,7 +18,6 @@ static const char * const builtin_fsmonitor__daemon_usage[] = {
	 	NULL
	 };
	 
	-#ifdef HAVE_FSMONITOR_DAEMON_BACKEND
	 /*
	  * Global state loaded from config.
	  */
	@@ -63,6 +62,7 @@ static int fsmonitor_config(const char *var, const char *value, void *cb)
	 
	 	return git_default_config(var, value, cb);
	 }
	+#ifdef HAVE_FSMONITOR_DAEMON_BACKEND
	 
	 /*
	  * Acting as a CLIENT.
	@@ -1492,6 +1492,8 @@ static int try_to_start_background_daemon(void)
	 	}
	 }
	 
	+#endif
	+
	 int cmd_fsmonitor__daemon(int argc, const char **argv, const char *prefix)
	 {
	 	const char *subcmd;
	@@ -1532,6 +1534,7 @@ int cmd_fsmonitor__daemon(int argc, const char **argv, const char *prefix)
	 		return -1;
	 	}
	 
	+#ifdef HAVE_FSMONITOR_DAEMON_BACKEND
	 	if (!strcmp(subcmd, "start"))
	 		return !!try_to_start_background_daemon();
	 
	@@ -1543,20 +1546,8 @@ int cmd_fsmonitor__daemon(int argc, const char **argv, const char *prefix)
	 
	 	if (!strcmp(subcmd, "status"))
	 		return !!do_as_client__status();
	-
	 	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
	+}

I.e. we can be a less zealous when setting the ifdef boundaries, and
it's actually less code as well.
Jeff Hostetler March 1, 2022, 2:58 p.m. UTC | #2 
On 2/25/22 5:46 PM, Ævar Arnfjörð Bjarmason wrote:
> 
> On Fri, Feb 11 2022, Jeff Hostetler via GitGitGadget wrote:
> 
>> From: Jeff Hostetler <jeffhost@microsoft.com>
>>
>> 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.
>>
[...]
>> +
>> +#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()
>> +	};
>> +
>> +	git_config(git_default_config, NULL);
>> +
>> +	argc = parse_options(argc, argv, prefix, options,
>> +			     builtin_fsmonitor__daemon_usage, 0);
>> +	if (argc != 1)
>> +		usage_with_options(builtin_fsmonitor__daemon_usage, options);
>> +	subcmd = argv[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
> 
> 
> I brought this up in another thread in how this series interacts with
> another, but this patch below on top of "seen" would allow you to catch
> parse_options() BUGs on Linux, even if you don't have a no-OSX
> non-Windows backend yet:
> 	
> 	diff --git a/builtin/fsmonitor--daemon.c b/builtin/fsmonitor--daemon.c
> 	index 591433e897d..62c0b1d486b 100644
> 	--- a/builtin/fsmonitor--daemon.c
> 	+++ b/builtin/fsmonitor--daemon.c
> 	@@ -18,7 +18,6 @@ static const char * const builtin_fsmonitor__daemon_usage[] = {
> 	 	NULL
> 	 };
> 	
> 	-#ifdef HAVE_FSMONITOR_DAEMON_BACKEND
> 	 /*
> 	  * Global state loaded from config.
> 	  */
> 	@@ -63,6 +62,7 @@ static int fsmonitor_config(const char *var, const char *value, void *cb)
> 	
> 	 	return git_default_config(var, value, cb);
> 	 }
> 	+#ifdef HAVE_FSMONITOR_DAEMON_BACKEND
> 	
> 	 /*
> 	  * Acting as a CLIENT.
> 	@@ -1492,6 +1492,8 @@ static int try_to_start_background_daemon(void)
> 	 	}
> 	 }
> 	
> 	+#endif
> 	+
> 	 int cmd_fsmonitor__daemon(int argc, const char **argv, const char *prefix)
> 	 {
> 	 	const char *subcmd;
> 	@@ -1532,6 +1534,7 @@ int cmd_fsmonitor__daemon(int argc, const char **argv, const char *prefix)
> 	 		return -1;
> 	 	}
> 	
> 	+#ifdef HAVE_FSMONITOR_DAEMON_BACKEND
> 	 	if (!strcmp(subcmd, "start"))
> 	 		return !!try_to_start_background_daemon();
> 	
> 	@@ -1543,20 +1546,8 @@ int cmd_fsmonitor__daemon(int argc, const char **argv, const char *prefix)
> 	
> 	 	if (!strcmp(subcmd, "status"))
> 	 		return !!do_as_client__status();
> 	-
> 	 	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
> 	+}
> 
> I.e. we can be a less zealous when setting the ifdef boundaries, and
> it's actually less code as well.
> 

Yes, it would be possible to distribute the ifdef throughout the file
and avoid duplicating the function declaration, but I'm not sure that
that adds any clarity or readability.

In my version, I have a stub version of the cmd_fsmonitor__daemon()
function and it is very clear that it does nothing when the feature
is not supported on a platform.  The rest of the source file is
concerned with supporting the feature.  And no interweaving of ifdefs
throughout the file is required.

Your version sets us up for future problems inside the body of the
cmd_ function.  For example, any static function called in the
supported portion of the function would also need to be ifdef'd
(as you have indicated).  But any local variables needed by the
supported portion would need to be declared at the top of the
function and also ifdef'd -- or we'd need to indent the entire body
of the supported portion inside another level of { }.  None of this
adds clarity.  (Just to avoid an 11 line stub function.)

Finally, I'm not sure how much value there is in being able to catch
parse_options() BUGs on Linux (or any other yet-to-be-supported
platform).  The daemon isn't supported and dies immediately. I'm not
sure that forcing the user to properly compose any arguments before
we just call die() is helpful.

So, I'd rather leave this as is.

Thanks,
Jeff
Junio C Hamano March 1, 2022, 5:37 p.m. UTC | #3 
Jeff Hostetler <git@jeffhostetler.com> writes:

> Finally, I'm not sure how much value there is in being able to catch
> parse_options() BUGs on Linux (or any other yet-to-be-supported
> platform).

I don't either.  Even though I am not 100% happy with the current
implementation of the embedded sanity checker in parse-options API,
as long as it is made available on all platforms, developers of a
platform specific part can also take advantage of it to catch any
issues while they develop, without waiting for Linux (or other
platforms) users to help them do so.
diff mbox series

Patch

diff --git a/.gitignore b/.gitignore
index f817c509ec0..e81de1063a4 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 9943f0f7c11..3b7a3f88b50 100644
--- a/Makefile
+++ b/Makefile
@@ -1106,6 +1106,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 83379f3832c..40e9ecc8485 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..f0498793379
--- /dev/null
+++ b/builtin/fsmonitor--daemon.c
@@ -0,0 +1,46 @@ 
+#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()
+	};
+
+	git_config(git_default_config, NULL);
+
+	argc = parse_options(argc, argv, prefix, options,
+			     builtin_fsmonitor__daemon_usage, 0);
+	if (argc != 1)
+		usage_with_options(builtin_fsmonitor__daemon_usage, options);
+	subcmd = argv[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 340665d4a04..a8b44d9b587 100644
--- a/git.c
+++ b/git.c
@@ -536,6 +536,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 },