diff mbox series

[1/3] scalar: enable built-in FSMonitor on `register`

Message ID 62682ccf6964d6eebb67491db4a9476dbab56671.1660673269.git.gitgitgadget@gmail.com (mailing list archive)
State New, archived
Headers show
Series [1/3] scalar: enable built-in FSMonitor on `register` | expand

Commit Message

Matthew John Cheetham Aug. 16, 2022, 6:07 p.m. UTC 
From: Matthew John Cheetham <mjcheetham@outlook.com>

Using the built-in FSMonitor makes many common commands quite a bit
faster. So let's teach the `scalar register` command to enable the
built-in FSMonitor and kick-start the fsmonitor--daemon process (for
convenience).

For simplicity, we only support the built-in FSMonitor (and no external
file system monitor such as e.g. Watchman).

Signed-off-by: Matthew John Cheetham <mjcheetham@outlook.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Victoria Dye <vdye@github.com>
---
 contrib/scalar/scalar.c          | 39 ++++++++++++++++++++++++++++++++
 contrib/scalar/t/t9099-scalar.sh | 11 +++++++++
 2 files changed, 50 insertions(+)

Comments

Junio C Hamano Aug. 16, 2022, 8:49 p.m. UTC | #1 
"Matthew John Cheetham via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> +static int start_fsmonitor_daemon(void)
> +{
> +	int res = 0;
> +	if (fsmonitor_ipc__is_supported() &&
> +	    fsmonitor_ipc__get_state() != IPC_STATE__LISTENING) {
> +		struct strbuf err = STRBUF_INIT;
> +		struct child_process cp = CHILD_PROCESS_INIT;
> +
> +		/* Try to start the FSMonitor daemon */
> +		cp.git_cmd = 1;
> +		strvec_pushl(&cp.args, "fsmonitor--daemon", "start", NULL);
> +		if (!pipe_command(&cp, NULL, 0, NULL, 0, &err, 0)) {
> +			/* Successfully started FSMonitor */
> +			strbuf_release(&err);
> +			return 0;
> +		}
> +
> +		/* If FSMonitor really hasn't started, emit error */
> +		if (fsmonitor_ipc__get_state() != IPC_STATE__LISTENING)
> +			res = error(_("could not start the FSMonitor daemon: %s"),
> +				    err.buf);
> +
> +		strbuf_release(&err);
> +	}
> +
> +	return res;
> +}

This somewhat curious code structure made me look, and made me
notice that the behaviour is even more curious.  Even though
pipe_command() fails, fsmonitor_ipc__get_state() can somehow become
LISTENING, in which case we are OK?  If that is the case, a more natural
way to write it would be:

	int res = 0; /* assume success */

	if (fsmonitor_ipc__is_supported() &&
            fsmonitor_ipc__get_state() != IPC_STATE__LISTENING) {
		...
                /* 
                 * if we fail to start it ourselves, and there is no
                 * daemon listening to us, it is an error.
                 */
		if (pipe_command(...) &&
		    fsmonitor_ipc__get_state() != IPC_STATE__LISTENING)
			res = error(...);
		strbuf_release(&err);
	}
	return res;

and that would utilize "res" consistently throughout the function.

Note that (I omitted unnecessary blank lines and added necessary
ones in the above outline of the rewrite.

Stopping, stepping back a bit and rethinking, the above is not still
exactly right.  If pipe_command() could lie and say "we failed to
start" when we immediately after the failure can find a running
daemon, what guarantees us that pipe_command() does not lie in the
other direction?  So, in that sense, perhaps doing

	/* we do not care if pipe_command() succeeds or not */
	(void) pipe_command(...);

        /*
         * we check ourselves if we do have a usable daemon 
         * and that is the authoritative answer.  we were asked
         * to ensure that usable daemon exists, and we answer
         * if we do or don't.
         */
	if (fsmonitor_ipc__get_state() != IPC_STATE__LISTENING)
		res = error(...);

may be more true to the spirit of the code.

It also is slightly curious if the caller wants to see "success"
when fsmonitor is not supported.  I would have expected the caller
to check and refrain from calling start/stop when it is not
supported (and if there is an end-user interface to force the scalar
command to "start", complain by saying "not supported here").  But
as long as we are consistent, I guess it is OK.

The side that stops shares exactly the same two pieces of
"curiosity" and may need to be updated exactly the same way.  It
assumes that pipe_command() is unreliable and instead of reporting a
possible failure, we sweep that under the rug, with a questionable
"we do not care about pipe failing, as long as the daemon is
listening, we do not care" attitude.  And the caller does not care
"start" not stopping where it is not supported.

Thanks.
Victoria Dye Aug. 16, 2022, 9:57 p.m. UTC | #2 
Junio C Hamano wrote:
> "Matthew John Cheetham via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
> 
>> +static int start_fsmonitor_daemon(void)
>> +{
>> +	int res = 0;
>> +	if (fsmonitor_ipc__is_supported() &&
>> +	    fsmonitor_ipc__get_state() != IPC_STATE__LISTENING) {
>> +		struct strbuf err = STRBUF_INIT;
>> +		struct child_process cp = CHILD_PROCESS_INIT;
>> +
>> +		/* Try to start the FSMonitor daemon */
>> +		cp.git_cmd = 1;
>> +		strvec_pushl(&cp.args, "fsmonitor--daemon", "start", NULL);
>> +		if (!pipe_command(&cp, NULL, 0, NULL, 0, &err, 0)) {
>> +			/* Successfully started FSMonitor */
>> +			strbuf_release(&err);
>> +			return 0;
>> +		}
>> +
>> +		/* If FSMonitor really hasn't started, emit error */
>> +		if (fsmonitor_ipc__get_state() != IPC_STATE__LISTENING)
>> +			res = error(_("could not start the FSMonitor daemon: %s"),
>> +				    err.buf);
>> +
>> +		strbuf_release(&err);
>> +	}
>> +
>> +	return res;
>> +}
> 
> This somewhat curious code structure made me look, and made me
> notice that the behaviour is even more curious.  Even though
> pipe_command() fails, fsmonitor_ipc__get_state() can somehow become
> LISTENING, in which case we are OK?  If that is the case, a more natural
> way to write it would be:
> 
> 	int res = 0; /* assume success */
> 
> 	if (fsmonitor_ipc__is_supported() &&
>             fsmonitor_ipc__get_state() != IPC_STATE__LISTENING) {
> 		...
>                 /* 
>                  * if we fail to start it ourselves, and there is no
>                  * daemon listening to us, it is an error.
>                  */
> 		if (pipe_command(...) &&
> 		    fsmonitor_ipc__get_state() != IPC_STATE__LISTENING)
> 			res = error(...);
> 		strbuf_release(&err);
> 	}
> 	return res;
> 
> and that would utilize "res" consistently throughout the function.
> 
> Note that (I omitted unnecessary blank lines and added necessary
> ones in the above outline of the rewrite.
> 
> Stopping, stepping back a bit and rethinking, the above is not still
> exactly right.  If pipe_command() could lie and say "we failed to
> start" when we immediately after the failure can find a running
> daemon, what guarantees us that pipe_command() does not lie in the
> other direction?  So, in that sense, perhaps doing
> 
> 	/* we do not care if pipe_command() succeeds or not */
> 	(void) pipe_command(...);
> 
>         /*
>          * we check ourselves if we do have a usable daemon 
>          * and that is the authoritative answer.  we were asked
>          * to ensure that usable daemon exists, and we answer
>          * if we do or don't.
>          */
> 	if (fsmonitor_ipc__get_state() != IPC_STATE__LISTENING)
> 		res = error(...);
> 
> may be more true to the spirit of the code.

This is an unintentional artifact of some minor refactoring of the original
versions in 'microsoft/git'. Previously [1], there was no
'fsmonitor_ipc__get_state()' check before calling 'git fsmonitor--daemon
start', so we'd expect failures whenever FSMonitor was already running. To
avoid making that 'pipe_command()' call when FSMonitor was already running,
I added an earlier call to 'fsmonitor_ipc__get_state()'. But, because I
didn't remove the later check, the code now implies that 'pipe_command()'
may give us "false negatives" (that is, fail but still manage to start the
FSMonitor).

I left the extraneous check in to be overly cautious, but realistically I
don't actually expect 'git fsmonitor--daemon start' to give us any false
positives or negatives. The code should reflect that:

	int res = 0;
	if (fsmonitor_ipc__is_supported() &&
	    fsmonitor_ipc__get_state() != IPC_STATE__LISTENING) {
		struct strbuf err = STRBUF_INIT;
		struct child_process cp = CHILD_PROCESS_INIT;

		/* Try to start the FSMonitor daemon */
		cp.git_cmd = 1;
		strvec_pushl(&cp.args, "fsmonitor--daemon", "start", NULL);
		if (pipe_command(&cp, NULL, 0, NULL, 0, &err, 0))
			res = error(_("could not start the FSMonitor daemon: %s"),
				    err.buf);

		strbuf_release(&err);
	}

	return res;

I'll re-roll with this shortly.

[1] https://github.com/microsoft/git/commit/4f2e092d3c98

> 
> It also is slightly curious if the caller wants to see "success"
> when fsmonitor is not supported.  I would have expected the caller
> to check and refrain from calling start/stop when it is not
> supported (and if there is an end-user interface to force the scalar
> command to "start", complain by saying "not supported here").  But
> as long as we are consistent, I guess it is OK.

I don't mind moving the 'fsmonitor_ipc__is_supported()' checks into
'register_dir()' and 'unregister_dir()'; I can see how it makes more sense
with the existing function name. 

As a side note, though, while looking at where to move the condition I
noticed that 'unregister_dir()' doesn't handle positive, nonzero return
values properly. I'll fix this & move the 'fsmonitor_ipc__is_supported()'
check in the next version. Thanks!

> 
> The side that stops shares exactly the same two pieces of
> "curiosity" and may need to be updated exactly the same way.  It
> assumes that pipe_command() is unreliable and instead of reporting a
> possible failure, we sweep that under the rug, with a questionable
> "we do not care about pipe failing, as long as the daemon is
> listening, we do not care" attitude.  And the caller does not care
> "start" not stopping where it is not supported.
> 
> Thanks.
Junio C Hamano Aug. 16, 2022, 10:15 p.m. UTC | #3 
Victoria Dye <vdye@github.com> writes:

>> 	/* we do not care if pipe_command() succeeds or not */
>> 	(void) pipe_command(...);
>> 
>>         /*
>>          * we check ourselves if we do have a usable daemon 
>>          * and that is the authoritative answer.  we were asked
>>          * to ensure that usable daemon exists, and we answer
>>          * if we do or don't.
>>          */
>> 	if (fsmonitor_ipc__get_state() != IPC_STATE__LISTENING)
>> 		res = error(...);
>> 
>> may be more true to the spirit of the code.
>
> This is an unintentional artifact of some minor refactoring of the original
> versions in 'microsoft/git'. Previously [1], there was no
> 'fsmonitor_ipc__get_state()' check before calling 'git fsmonitor--daemon
> start', so we'd expect failures whenever FSMonitor was already running. To
> avoid making that 'pipe_command()' call when FSMonitor was already running,
> I added an earlier call to 'fsmonitor_ipc__get_state()'. But, because I
> didn't remove the later check, the code now implies that 'pipe_command()'
> may give us "false negatives" (that is, fail but still manage to start the
> FSMonitor).
>
> I left the extraneous check in to be overly cautious, but realistically I
> don't actually expect 'git fsmonitor--daemon start' to give us any false
> positives or negatives. The code should reflect that:
>
> 	int res = 0;
> 	if (fsmonitor_ipc__is_supported() &&
> 	    fsmonitor_ipc__get_state() != IPC_STATE__LISTENING) {
> 		struct strbuf err = STRBUF_INIT;
> 		struct child_process cp = CHILD_PROCESS_INIT;
>
> 		/* Try to start the FSMonitor daemon */
> 		cp.git_cmd = 1;
> 		strvec_pushl(&cp.args, "fsmonitor--daemon", "start", NULL);
> 		if (pipe_command(&cp, NULL, 0, NULL, 0, &err, 0))
> 			res = error(_("could not start the FSMonitor daemon: %s"),
> 				    err.buf);
>
> 		strbuf_release(&err);
> 	}
>
> 	return res;
>
> I'll re-roll with this shortly.

OK, that is one valid way to go about it.  After I sent my review
comments, I however briefly wondered if we might *not* know if we
are already running one, there is a reliable exclusion mechansim
to prevent more than one monitor running at the same time, and we
are running pipe_command(), fully expecting that it may fail when
there is already a working one and a call to pipe_command() that
is not "checked" is just being lazy because we can afford to be
lazy here.

If that is not what is going on, then the cleaned up version I am
responding to does look more straight-forward and easy to
understand.  On the other hand, if "we can start more than we need
because we can rely on the exclusion mechanism" is what is going on,
that is fine as well, but it does need to be documented, preferrably
as in-code comment.

Thanks.
diff mbox series

Patch

diff --git a/contrib/scalar/scalar.c b/contrib/scalar/scalar.c
index 97e71fe19cd..219e414ab4e 100644
--- a/contrib/scalar/scalar.c
+++ b/contrib/scalar/scalar.c
@@ -7,6 +7,8 @@ 
 #include "parse-options.h"
 #include "config.h"
 #include "run-command.h"
+#include "simple-ipc.h"
+#include "fsmonitor-ipc.h"
 #include "refs.h"
 #include "dir.h"
 #include "packfile.h"
@@ -169,6 +171,12 @@  static int set_recommended_config(int reconfigure)
 		{ "core.autoCRLF", "false" },
 		{ "core.safeCRLF", "false" },
 		{ "fetch.showForcedUpdates", "false" },
+#ifdef HAVE_FSMONITOR_DAEMON_BACKEND
+		/*
+		 * Enable the built-in FSMonitor on supported platforms.
+		 */
+		{ "core.fsmonitor", "true" },
+#endif
 		{ NULL, NULL },
 	};
 	int i;
@@ -236,6 +244,34 @@  static int add_or_remove_enlistment(int add)
 		       "scalar.repo", the_repository->worktree, NULL);
 }
 
+static int start_fsmonitor_daemon(void)
+{
+	int res = 0;
+	if (fsmonitor_ipc__is_supported() &&
+	    fsmonitor_ipc__get_state() != IPC_STATE__LISTENING) {
+		struct strbuf err = STRBUF_INIT;
+		struct child_process cp = CHILD_PROCESS_INIT;
+
+		/* Try to start the FSMonitor daemon */
+		cp.git_cmd = 1;
+		strvec_pushl(&cp.args, "fsmonitor--daemon", "start", NULL);
+		if (!pipe_command(&cp, NULL, 0, NULL, 0, &err, 0)) {
+			/* Successfully started FSMonitor */
+			strbuf_release(&err);
+			return 0;
+		}
+
+		/* If FSMonitor really hasn't started, emit error */
+		if (fsmonitor_ipc__get_state() != IPC_STATE__LISTENING)
+			res = error(_("could not start the FSMonitor daemon: %s"),
+				    err.buf);
+
+		strbuf_release(&err);
+	}
+
+	return res;
+}
+
 static int register_dir(void)
 {
 	int res = add_or_remove_enlistment(1);
@@ -246,6 +282,9 @@  static int register_dir(void)
 	if (!res)
 		res = toggle_maintenance(1);
 
+	if (!res)
+		res = start_fsmonitor_daemon();
+
 	return res;
 }
 
diff --git a/contrib/scalar/t/t9099-scalar.sh b/contrib/scalar/t/t9099-scalar.sh
index 10b1172a8aa..526f64d001c 100755
--- a/contrib/scalar/t/t9099-scalar.sh
+++ b/contrib/scalar/t/t9099-scalar.sh
@@ -13,10 +13,21 @@  PATH=$PWD/..:$PATH
 GIT_TEST_MAINT_SCHEDULER="crontab:test-tool crontab ../cron.txt,launchctl:true,schtasks:true"
 export GIT_TEST_MAINT_SCHEDULER
 
+test_lazy_prereq BUILTIN_FSMONITOR '
+	git version --build-options | grep -q "feature:.*fsmonitor--daemon"
+'
+
 test_expect_success 'scalar shows a usage' '
 	test_expect_code 129 scalar -h
 '
 
+test_expect_success BUILTIN_FSMONITOR 'scalar register starts fsmon daemon' '
+	git init test/src &&
+	test_must_fail git -C test/src fsmonitor--daemon status &&
+	scalar register test/src &&
+	git -C test/src fsmonitor--daemon status
+'
+
 test_expect_success 'scalar unregister' '
 	git init vanish/src &&
 	scalar register vanish/src &&