[05/23] fsmonitor--daemon: add a built-in fsmonitor daemon

Commit Message

Jeff Hostetler April 1, 2021, 3:40 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 | 52 +++++++++++++++++++++++++++++++++++++
 git.c                       |  1 +
 5 files changed, 56 insertions(+)
 create mode 100644 builtin/fsmonitor--daemon.c

Comments

Derrick Stolee April 26, 2021, 3:08 p.m. UTC | #1

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

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

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

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

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

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

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

Thanks,
-Stolee

Derrick Stolee April 26, 2021, 3:45 p.m. UTC | #2

On 4/26/21 11:08 AM, Derrick Stolee wrote:
> On 4/1/21 11:40 AM, Jeff Hostetler via GitGitGadget wrote:> +#ifdef HAVE_FSMONITOR_DAEMON_BACKEND
> 
> I think these compile-time macros should be replaced with a
> method call, as I've said before. It should be simple to say
> 
> 	if (!fsmonitor_ipc__is_supported())
> 		die(_("fsmonitor--daemon is not supported on this platform"));
> 
> and call it a day. This can be done before parsing arguments.
> 
>> +int cmd_fsmonitor__daemon(int argc, const char **argv, const char *prefix)
>> +{
>> +	enum daemon_mode {
>> +		UNDEFINED_MODE,
>> +	} mode = UNDEFINED_MODE;
>> +
>> +	struct option options[] = {
>> +		OPT_END()
>> +	};
> 
> I can see where you are going here, to use the parse-opts API
> to get your "--<verb>" arguments to populate an 'enum'. However,
> it seems like you will run into the problem where a user enters
> multiple such arguments and you lose the information as the
> parser overwrites 'mode' here.

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

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

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

Thanks,
-Stolee

Jeff Hostetler April 30, 2021, 2:31 p.m. UTC | #3

On 4/26/21 11:45 AM, Derrick Stolee wrote:
> On 4/26/21 11:08 AM, Derrick Stolee wrote:
>> On 4/1/21 11:40 AM, Jeff Hostetler via GitGitGadget wrote:> +#ifdef HAVE_FSMONITOR_DAEMON_BACKEND
>>
>> I think these compile-time macros should be replaced with a
>> method call, as I've said before. It should be simple to say
>>
>> 	if (!fsmonitor_ipc__is_supported())
>> 		die(_("fsmonitor--daemon is not supported on this platform"));
>>
>> and call it a day. This can be done before parsing arguments.
>>
>>> +int cmd_fsmonitor__daemon(int argc, const char **argv, const char *prefix)
>>> +{
>>> +	enum daemon_mode {
>>> +		UNDEFINED_MODE,
>>> +	} mode = UNDEFINED_MODE;
>>> +
>>> +	struct option options[] = {
>>> +		OPT_END()
>>> +	};
>>
>> I can see where you are going here, to use the parse-opts API
>> to get your "--<verb>" arguments to populate an 'enum'. However,
>> it seems like you will run into the problem where a user enters
>> multiple such arguments and you lose the information as the
>> parser overwrites 'mode' here.
> 
> I see that you use OPT_CMDMODE in your implementation, which
> makes this concern invalid.
> 
>> Better to use a positional argument and drop the "--" prefix,
>> in my opinion.
> 
> This is my personal taste, but the technical reason to do this
> doesn't exist.

Either method is fine/equivalent and I'm open to doing it either
way.  (In fact, I did the t/helper/test-simple-ipc the other way
and didn't even think about it.)

Does the mailing list have a preference for one form over the other?
That is:

     git fsmonitor--daemon --start [<options>]
vs
     git fsmonitor--daemon start [<options>]

Jeff

Patch

diff --git a/.gitignore b/.gitignore
index 3dcdb6bb5ab8..beccf34abe9e 100644
--- a/.gitignore
+++ b/.gitignore
@@ -71,6 +71,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 50977911d41a..d792631d4250 100644
--- a/Makefile
+++ b/Makefile
@@ -1091,6 +1091,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 b6ce981b7377..7554476f90a4 100644
--- a/builtin.h
+++ b/builtin.h
@@ -158,6 +158,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 000000000000..6700bac92c7d
--- /dev/null
+++ b/builtin/fsmonitor--daemon.c
@@ -0,0 +1,52 @@ 
+#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)
+{
+	enum daemon_mode {
+		UNDEFINED_MODE,
+	} mode = UNDEFINED_MODE;
+
+	struct option options[] = {
+		OPT_END()
+	};
+
+	if (argc == 2 && !strcmp(argv[1], "-h"))
+		usage_with_options(builtin_fsmonitor__daemon_usage, options);
+
+	git_config(git_default_config, NULL);
+
+	argc = parse_options(argc, argv, prefix, options,
+			     builtin_fsmonitor__daemon_usage, 0);
+
+	switch (mode) {
+	case UNDEFINED_MODE:
+	default:
+		die(_("Unhandled command mode %d"), mode);
+	}
+}
+
+#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 9bc077a025cb..239deb9823fc 100644
--- a/git.c
+++ b/git.c
@@ -523,6 +523,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 },