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

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

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.