[PATCH 0/3] scalar: enable built-in FSMonitor

[PATCH 0/3] scalar: enable built-in FSMonitor

[Victoria Dye via GitGitGadget]

This series enables the built-in FSMonitor [1] on 'scalar'-registered
repository enlistments. To avoid errors when unregistering an enlistment,
the FSMonitor daemon is explicitly stopped during 'scalar unregister'.

Maintainer's note: this series has a minor conflict with
'vd/scalar-generalize-diagnose'. Please let me know if there's anything else
I can provide (in addition to [2]) that would make resolution easier.

Thanks

 * Victoria

[1]
https://lore.kernel.org/git/pull.1143.git.1644940773.gitgitgadget@gmail.com/

[2] The conflict is a result of both series updating the Scalar roadmap doc.
For reference, my merge resolution (from git diff <merge commit> <merge
commit>^1 <merge commit>^2, where <merge commit>^1 is
'vd/scalar-generalize-diagnose' and <merge commit>^2 is this series) looks
like:

------------->8------------->8------------->8------------->8------------->8-------------
diff --cc Documentation/technical/scalar.txt
index f6353375f0,047390e46e..0600150b3a
--- a/Documentation/technical/scalar.txt
+++ b/Documentation/technical/scalar.txt
@@@ -84,20 -84,26 +84,23 @@@ series have been accepted
  
  - `scalar-diagnose`: The `scalar` command is taught the `diagnose` subcommand.
  
 +- `scalar-generalize-diagnose`: Move the functionality of `scalar diagnose`
 +  into `git diagnose` and `git bugreport --diagnose`.
 +
+ - 'scalar-add-fsmonitor: Enable the built-in FSMonitor in Scalar
+   enlistments. At the end of this series, Scalar should be feature-complete
+   from the perspective of a user.
+ 
  Roughly speaking (and subject to change), the following series are needed to
  "finish" this initial version of Scalar:
  
- - Finish Scalar features: Enable the built-in FSMonitor in Scalar enlistments
-   and implement `scalar help`. At the end of this series, Scalar should be
-   feature-complete from the perspective of a user.
 -- Generalize features not specific to Scalar: In the spirit of making Scalar
 -  configure only what is needed for large repo performance, move common
 -  utilities into other parts of Git. Some of this will be internal-only, but one
 -  major change will be generalizing `scalar diagnose` for use with any Git
 -  repository.
--
  - Move Scalar to toplevel: Move Scalar out of `contrib/` and into the root of
-   `git`, including updates to build and install it with the rest of Git. This
-   change will incorporate Scalar into the Git CI and test framework, as well as
-   expand regression and performance testing to ensure the tool is stable.
+   `git`. This includes a variety of related updates, including:
+     - building & installing Scalar in the Git root-level 'make [install]'.
+     - builing & testing Scalar as part of CI.
+     - moving and expanding test coverage of Scalar (including perf tests).
+     - implementing 'scalar help'/'git help scalar' to display scalar
+       documentation.
  
  Finally, there are two additional patch series that exist in Microsoft's fork of
  Git, but there is no current plan to upstream them. There are some interesting
-------------8<-------------8<-------------8<-------------8<-------------8<---------


Johannes Schindelin (1):
  scalar unregister: stop FSMonitor daemon

Matthew John Cheetham (1):
  scalar: enable built-in FSMonitor on `register`

Victoria Dye (1):
  scalar: update technical doc roadmap with FSMonitor support

 Documentation/technical/scalar.txt | 17 +++++---
 contrib/scalar/scalar.c            | 69 ++++++++++++++++++++++++++++++
 contrib/scalar/t/t9099-scalar.sh   | 11 +++++
 3 files changed, 90 insertions(+), 7 deletions(-)


base-commit: 4af7188bc97f70277d0f10d56d5373022b1fa385
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1324%2Fvdye%2Fscalar%2Fadd-fsmonitor-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1324/vdye/scalar/add-fsmonitor-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1324
-- 
gitgitgadget

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

[Matthew John Cheetham via GitGitGadget]

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(+)

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 &&
-- 
gitgitgadget

[PATCH 2/3] scalar unregister: stop FSMonitor daemon

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

Especially on Windows, we will need to stop that daemon, just in case
that the directory needs to be removed (the daemon would otherwise hold
a handle to that directory, preventing it from being deleted).

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Victoria Dye <vdye@github.com>
---
 contrib/scalar/scalar.c | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/contrib/scalar/scalar.c b/contrib/scalar/scalar.c
index 219e414ab4e..b774eb044ec 100644
--- a/contrib/scalar/scalar.c
+++ b/contrib/scalar/scalar.c
@@ -272,6 +272,33 @@ static int start_fsmonitor_daemon(void)
 	return res;
 }
 
+static int stop_fsmonitor_daemon(void)
+{
+	int res = 0;
+	if (fsmonitor_ipc__is_supported()) {
+		struct strbuf err = STRBUF_INIT;
+		struct child_process cp = CHILD_PROCESS_INIT;
+
+		/* Try to stop the FSMonitor daemon */
+		cp.git_cmd = 1;
+		strvec_pushl(&cp.args, "fsmonitor--daemon", "stop", NULL);
+		if (!pipe_command(&cp, NULL, 0, NULL, 0, &err, 0)) {
+			/* Successfully stopped FSMonitor */
+			strbuf_release(&err);
+			return 0;
+		}
+
+		/* If FSMonitor really hasn't stopped, emit error */
+		if (fsmonitor_ipc__get_state() == IPC_STATE__LISTENING)
+			res = error(_("could not stop the FSMonitor daemon: %s"),
+				    err.buf);
+
+		strbuf_release(&err);
+	}
+
+	return res;
+}
+
 static int register_dir(void)
 {
 	int res = add_or_remove_enlistment(1);
@@ -298,6 +325,9 @@ static int unregister_dir(void)
 	if (add_or_remove_enlistment(0) < 0)
 		res = -1;
 
+	if (stop_fsmonitor_daemon() < 0)
+		res = -1;
+
 	return res;
 }
 
-- 
gitgitgadget

[PATCH 3/3] scalar: update technical doc roadmap with FSMonitor support

[Victoria Dye via GitGitGadget]

From: Victoria Dye <vdye@github.com>

Update the Scalar roadmap to reflect completion of enabling the built-in
FSMonitor in Scalar.

Note that implementation of 'scalar help' was moved to the final set of
changes to move Scalar out of 'contrib/'. This is due to a dependency on
changes to 'git help', as all changes to the main Git tree *exclusively*
implemented to support Scalar are part of that series.

Signed-off-by: Victoria Dye <vdye@github.com>
---
 Documentation/technical/scalar.txt | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/Documentation/technical/scalar.txt b/Documentation/technical/scalar.txt
index 08bc09c225a..047390e46eb 100644
--- a/Documentation/technical/scalar.txt
+++ b/Documentation/technical/scalar.txt
@@ -84,13 +84,13 @@ series have been accepted:
 
 - `scalar-diagnose`: The `scalar` command is taught the `diagnose` subcommand.
 
+- 'scalar-add-fsmonitor: Enable the built-in FSMonitor in Scalar
+  enlistments. At the end of this series, Scalar should be feature-complete
+  from the perspective of a user.
+
 Roughly speaking (and subject to change), the following series are needed to
 "finish" this initial version of Scalar:
 
-- Finish Scalar features: Enable the built-in FSMonitor in Scalar enlistments
-  and implement `scalar help`. At the end of this series, Scalar should be
-  feature-complete from the perspective of a user.
-
 - Generalize features not specific to Scalar: In the spirit of making Scalar
   configure only what is needed for large repo performance, move common
   utilities into other parts of Git. Some of this will be internal-only, but one
@@ -98,9 +98,12 @@ Roughly speaking (and subject to change), the following series are needed to
   repository.
 
 - Move Scalar to toplevel: Move Scalar out of `contrib/` and into the root of
-  `git`, including updates to build and install it with the rest of Git. This
-  change will incorporate Scalar into the Git CI and test framework, as well as
-  expand regression and performance testing to ensure the tool is stable.
+  `git`. This includes a variety of related updates, including:
+    - building & installing Scalar in the Git root-level 'make [install]'.
+    - builing & testing Scalar as part of CI.
+    - moving and expanding test coverage of Scalar (including perf tests).
+    - implementing 'scalar help'/'git help scalar' to display scalar
+      documentation.
 
 Finally, there are two additional patch series that exist in Microsoft's fork of
 Git, but there is no current plan to upstream them. There are some interesting
-- 
gitgitgadget

Re: [PATCH 0/3] scalar: enable built-in FSMonitor

[Junio C Hamano]

"Victoria Dye via GitGitGadget" <gitgitgadget@gmail.com> writes:

> This series enables the built-in FSMonitor [1] on 'scalar'-registered
> repository enlistments. To avoid errors when unregistering an enlistment,
> the FSMonitor daemon is explicitly stopped during 'scalar unregister'.
>
> Maintainer's note: this series has a minor conflict with
> 'vd/scalar-generalize-diagnose'. Please let me know if there's anything else
> I can provide (in addition to [2]) that would make resolution easier.

Thanks.  What's the doneness of the other series?  It has cooked for
quite a while and I was wondering if it is ready for 'next' already,
by the way.

Re: [PATCH 0/3] scalar: enable built-in FSMonitor

[Victoria Dye]

Junio C Hamano wrote:
> "Victoria Dye via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> This series enables the built-in FSMonitor [1] on 'scalar'-registered
>> repository enlistments. To avoid errors when unregistering an enlistment,
>> the FSMonitor daemon is explicitly stopped during 'scalar unregister'.
>>
>> Maintainer's note: this series has a minor conflict with
>> 'vd/scalar-generalize-diagnose'. Please let me know if there's anything else
>> I can provide (in addition to [2]) that would make resolution easier.
> 
> Thanks.  What's the doneness of the other series?  It has cooked for
> quite a while and I was wondering if it is ready for 'next' already,
> by the way.
> 

I wasn't planning on making any other changes unless more comments came in;
I personally think it's ready for 'next', but I'm not sure about reviewers'
thoughts. There seemed to be some new interest in reviewing at the IRC
stand-up yesterday [1], but I haven't heard anything since then. 

On the off chance that some major blocking review to
'vd/scalar-generalize-diagnose' comes in, I didn't want to base this series
on that one. But, if it *is* merged to 'next' before this one, I'll make
sure to rebase this series on top in subsequent versions to avoid the merge
conflict. 

[1] https://colabti.org/irclogger/irclogger_log/git-devel?date=2022-08-15#l83

Re: [PATCH 0/3] scalar: enable built-in FSMonitor

[Junio C Hamano]

Victoria Dye <vdye@github.com> writes:

> On the off chance that some major blocking review to
> 'vd/scalar-generalize-diagnose' comes in, I didn't want to base this series
> on that one. But, if it *is* merged to 'next' before this one, I'll make
> sure to rebase this series on top in subsequent versions to avoid the merge
> conflict. 

All sensible.  I actually am running out of topics to merge to
'next' ;-) and that was the primary reason why I asked.

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

[Junio C Hamano]

"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.

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

[Victoria Dye]

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.

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

[Junio C Hamano]

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.

[PATCH v2 0/5] scalar: enable built-in FSMonitor

[Victoria Dye via GitGitGadget]

This series enables the built-in FSMonitor [1] on 'scalar'-registered
repository enlistments. To avoid errors when unregistering an enlistment,
the FSMonitor daemon is explicitly stopped during 'scalar unregister'.

Maintainer's note: this series has a minor conflict with
'vd/scalar-generalize-diagnose'. Please let me know if there's anything else
I can provide (in addition to [2]) that would make resolution easier.


Changes since V1
================

 * Added a patch to fix 'unregister_dir()'s handling of return values > 0
   from 'toggle_maintenance()' and 'add_or_remove_enlistment()'.
 * Added a patch to print error messages in 'register_dir()' and
   'unregister_dir()' indicating which of their internal steps fail.
 * Moved check of 'fsmonitor_ipc__is_supported()' to '[un]register_dir()' to
   avoid calling '(start|stop)_fsmonitor_daemon()' when the feature is not
   supported. Added assertion of 'fsmonitor_ipc__is_supported()' to
   '(start|stop)_fsmonitor_daemon()' to enforce that they are not called
   when the feature is unavailable.
 * Simplified '(start|stop)_fsmonitor_daemon()' implementation. Now, if
   FSMonitor is already running/stopped (respectively), the function simply
   returns 0; otherwise, it runs 'git fsmonitor--daemon (start|stop)' and
   returns the exit code.
   * Note that the "could not (start|stop) the FSMonitor daemon: <err_msg>"
     error messages are no longer printed by
     '(start|stop)_fsmonitor_daemon()'. Instead, "<err_msg>" is printed to
     stderr by swapping 'pipe_command()' out for 'run_git()', and
     '[un]register_dir()' prints the "could not (start|stop) the FSMonitor
     daemon" message.

Thanks

 * Victoria

[1]
https://lore.kernel.org/git/pull.1143.git.1644940773.gitgitgadget@gmail.com/

[2] The conflict is a result of both series updating the Scalar roadmap doc.
For reference, my merge resolution (from git diff <merge commit> <merge
commit>^1 <merge commit>^2, where <merge commit>^1 is
'vd/scalar-generalize-diagnose' and <merge commit>^2 is this series) looks
like:

------------->8------------->8------------->8------------->8------------->8-------------
diff --cc Documentation/technical/scalar.txt
index f6353375f0,047390e46e..0600150b3a
--- a/Documentation/technical/scalar.txt
+++ b/Documentation/technical/scalar.txt
@@@ -84,20 -84,26 +84,23 @@@ series have been accepted
  
  - `scalar-diagnose`: The `scalar` command is taught the `diagnose` subcommand.
  
 +- `scalar-generalize-diagnose`: Move the functionality of `scalar diagnose`
 +  into `git diagnose` and `git bugreport --diagnose`.
 +
+ - 'scalar-add-fsmonitor: Enable the built-in FSMonitor in Scalar
+   enlistments. At the end of this series, Scalar should be feature-complete
+   from the perspective of a user.
+ 
  Roughly speaking (and subject to change), the following series are needed to
  "finish" this initial version of Scalar:
  
- - Finish Scalar features: Enable the built-in FSMonitor in Scalar enlistments
-   and implement `scalar help`. At the end of this series, Scalar should be
-   feature-complete from the perspective of a user.
 -- Generalize features not specific to Scalar: In the spirit of making Scalar
 -  configure only what is needed for large repo performance, move common
 -  utilities into other parts of Git. Some of this will be internal-only, but one
 -  major change will be generalizing `scalar diagnose` for use with any Git
 -  repository.
--
  - Move Scalar to toplevel: Move Scalar out of `contrib/` and into the root of
-   `git`, including updates to build and install it with the rest of Git. This
-   change will incorporate Scalar into the Git CI and test framework, as well as
-   expand regression and performance testing to ensure the tool is stable.
+   `git`. This includes a variety of related updates, including:
+     - building & installing Scalar in the Git root-level 'make [install]'.
+     - builing & testing Scalar as part of CI.
+     - moving and expanding test coverage of Scalar (including perf tests).
+     - implementing 'scalar help'/'git help scalar' to display scalar
+       documentation.
  
  Finally, there are two additional patch series that exist in Microsoft's fork of
  Git, but there is no current plan to upstream them. There are some interesting
-------------8<-------------8<-------------8<-------------8<-------------8<---------


Johannes Schindelin (1):
  scalar unregister: stop FSMonitor daemon

Matthew John Cheetham (1):
  scalar: enable built-in FSMonitor on `register`

Victoria Dye (3):
  scalar-unregister: handle error codes greater than 0
  scalar-[un]register: clearly indicate source of error
  scalar: update technical doc roadmap with FSMonitor support

 Documentation/technical/scalar.txt | 17 +++++----
 contrib/scalar/scalar.c            | 55 ++++++++++++++++++++++++------
 contrib/scalar/t/t9099-scalar.sh   | 11 ++++++
 3 files changed, 66 insertions(+), 17 deletions(-)


base-commit: 4af7188bc97f70277d0f10d56d5373022b1fa385
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1324%2Fvdye%2Fscalar%2Fadd-fsmonitor-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1324/vdye/scalar/add-fsmonitor-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1324

Range-diff vs v1:

 -:  ----------- > 1:  36fc3cb604d scalar-unregister: handle error codes greater than 0
 -:  ----------- > 2:  4bacf8bce8a scalar-[un]register: clearly indicate source of error
 1:  62682ccf696 ! 3:  5fdf8337972 scalar: enable built-in FSMonitor on `register`
     @@ Commit message
          For simplicity, we only support the built-in FSMonitor (and no external
          file system monitor such as e.g. Watchman).
      
     +    Helped-by: Junio C Hamano <gitster@pobox.com>
          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: static int add_or_remove_enlistment(int add)
       
      +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;
     ++	assert(fsmonitor_ipc__is_supported());
      +
     -+		/* 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_ipc__get_state() != IPC_STATE__LISTENING)
     ++		return run_git("fsmonitor--daemon", "start", NULL);
      +
     -+		/* 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;
     ++	return 0;
      +}
      +
       static int register_dir(void)
       {
     - 	int res = add_or_remove_enlistment(1);
     + 	if (add_or_remove_enlistment(1))
      @@ contrib/scalar/scalar.c: static int register_dir(void)
     - 	if (!res)
     - 		res = toggle_maintenance(1);
     + 	if (toggle_maintenance(1))
     + 		return error(_("could not turn on maintenance"));
       
     -+	if (!res)
     -+		res = start_fsmonitor_daemon();
     ++	if (fsmonitor_ipc__is_supported() && start_fsmonitor_daemon())
     ++		return error(_("could not start the FSMonitor daemon"));
      +
     - 	return res;
     + 	return 0;
       }
       
      
 2:  78a7f0b1be0 ! 4:  fc4aa1fde31 scalar unregister: stop FSMonitor daemon
     @@ Commit message
          that the directory needs to be removed (the daemon would otherwise hold
          a handle to that directory, preventing it from being deleted).
      
     +    Helped-by: Junio C Hamano <gitster@pobox.com>
          Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
          Signed-off-by: Victoria Dye <vdye@github.com>
      
       ## contrib/scalar/scalar.c ##
      @@ contrib/scalar/scalar.c: static int start_fsmonitor_daemon(void)
     - 	return res;
     + 	return 0;
       }
       
      +static int stop_fsmonitor_daemon(void)
      +{
     -+	int res = 0;
     -+	if (fsmonitor_ipc__is_supported()) {
     -+		struct strbuf err = STRBUF_INIT;
     -+		struct child_process cp = CHILD_PROCESS_INIT;
     -+
     -+		/* Try to stop the FSMonitor daemon */
     -+		cp.git_cmd = 1;
     -+		strvec_pushl(&cp.args, "fsmonitor--daemon", "stop", NULL);
     -+		if (!pipe_command(&cp, NULL, 0, NULL, 0, &err, 0)) {
     -+			/* Successfully stopped FSMonitor */
     -+			strbuf_release(&err);
     -+			return 0;
     -+		}
     -+
     -+		/* If FSMonitor really hasn't stopped, emit error */
     -+		if (fsmonitor_ipc__get_state() == IPC_STATE__LISTENING)
     -+			res = error(_("could not stop the FSMonitor daemon: %s"),
     -+				    err.buf);
     ++	assert(fsmonitor_ipc__is_supported());
      +
     -+		strbuf_release(&err);
     -+	}
     ++	if (fsmonitor_ipc__get_state() == IPC_STATE__LISTENING)
     ++		return run_git("fsmonitor--daemon", "stop", NULL);
      +
     -+	return res;
     ++	return 0;
      +}
      +
       static int register_dir(void)
       {
     - 	int res = add_or_remove_enlistment(1);
     + 	if (add_or_remove_enlistment(1))
      @@ contrib/scalar/scalar.c: static int unregister_dir(void)
     - 	if (add_or_remove_enlistment(0) < 0)
     - 		res = -1;
     + 	if (add_or_remove_enlistment(0))
     + 		res = error(_("could not remove enlistment"));
       
     -+	if (stop_fsmonitor_daemon() < 0)
     -+		res = -1;
     ++	if (fsmonitor_ipc__is_supported() && stop_fsmonitor_daemon() < 0)
     ++		res = error(_("could not stop the FSMonitor daemon"));
      +
       	return res;
       }
 3:  5457a8ff1fa = 5:  dd59caa2e5a scalar: update technical doc roadmap with FSMonitor support

-- 
gitgitgadget

[PATCH v2 1/5] scalar-unregister: handle error codes greater than 0

[Victoria Dye via GitGitGadget]

From: Victoria Dye <vdye@github.com>

When 'scalar unregister' tries to disable maintenance and remove an
enlistment, ensure that the return value is nonzero if either operation
produces *any* nonzero return value, not just when they return a value less
than 0.

Signed-off-by: Victoria Dye <vdye@github.com>
---
 contrib/scalar/scalar.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/contrib/scalar/scalar.c b/contrib/scalar/scalar.c
index 97e71fe19cd..e888fa5408e 100644
--- a/contrib/scalar/scalar.c
+++ b/contrib/scalar/scalar.c
@@ -253,10 +253,10 @@ static int unregister_dir(void)
 {
 	int res = 0;
 
-	if (toggle_maintenance(0) < 0)
+	if (toggle_maintenance(0))
 		res = -1;
 
-	if (add_or_remove_enlistment(0) < 0)
+	if (add_or_remove_enlistment(0))
 		res = -1;
 
 	return res;
-- 
gitgitgadget

[PATCH v2 2/5] scalar-[un]register: clearly indicate source of error

[Victoria Dye via GitGitGadget]

From: Victoria Dye <vdye@github.com>

When a step in 'register_dir()' or 'unregister_dir()' fails, indicate which
step failed with an error message, rather than silently assigning a nonzero
return code.

Signed-off-by: Victoria Dye <vdye@github.com>
---
 contrib/scalar/scalar.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/contrib/scalar/scalar.c b/contrib/scalar/scalar.c
index e888fa5408e..6025cd71604 100644
--- a/contrib/scalar/scalar.c
+++ b/contrib/scalar/scalar.c
@@ -238,15 +238,16 @@ static int add_or_remove_enlistment(int add)
 
 static int register_dir(void)
 {
-	int res = add_or_remove_enlistment(1);
+	if (add_or_remove_enlistment(1))
+		return error(_("could not add enlistment"));
 
-	if (!res)
-		res = set_recommended_config(0);
+	if (set_recommended_config(0))
+		return error(_("could not set recommended config"));
 
-	if (!res)
-		res = toggle_maintenance(1);
+	if (toggle_maintenance(1))
+		return error(_("could not turn on maintenance"));
 
-	return res;
+	return 0;
 }
 
 static int unregister_dir(void)
@@ -254,10 +255,10 @@ static int unregister_dir(void)
 	int res = 0;
 
 	if (toggle_maintenance(0))
-		res = -1;
+		res = error(_("could not turn off maintenance"));
 
 	if (add_or_remove_enlistment(0))
-		res = -1;
+		res = error(_("could not remove enlistment"));
 
 	return res;
 }
-- 
gitgitgadget

[PATCH v2 3/5] scalar: enable built-in FSMonitor on `register`

[Matthew John Cheetham via GitGitGadget]

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).

Helped-by: Junio C Hamano <gitster@pobox.com>
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          | 21 +++++++++++++++++++++
 contrib/scalar/t/t9099-scalar.sh | 11 +++++++++++
 2 files changed, 32 insertions(+)

diff --git a/contrib/scalar/scalar.c b/contrib/scalar/scalar.c
index 6025cd71604..28d915ec006 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,16 @@ static int add_or_remove_enlistment(int add)
 		       "scalar.repo", the_repository->worktree, NULL);
 }
 
+static int start_fsmonitor_daemon(void)
+{
+	assert(fsmonitor_ipc__is_supported());
+
+	if (fsmonitor_ipc__get_state() != IPC_STATE__LISTENING)
+		return run_git("fsmonitor--daemon", "start", NULL);
+
+	return 0;
+}
+
 static int register_dir(void)
 {
 	if (add_or_remove_enlistment(1))
@@ -247,6 +265,9 @@ static int register_dir(void)
 	if (toggle_maintenance(1))
 		return error(_("could not turn on maintenance"));
 
+	if (fsmonitor_ipc__is_supported() && start_fsmonitor_daemon())
+		return error(_("could not start the FSMonitor daemon"));
+
 	return 0;
 }
 
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 &&
-- 
gitgitgadget

[PATCH v2 4/5] scalar unregister: stop FSMonitor daemon

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

Especially on Windows, we will need to stop that daemon, just in case
that the directory needs to be removed (the daemon would otherwise hold
a handle to that directory, preventing it from being deleted).

Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Victoria Dye <vdye@github.com>
---
 contrib/scalar/scalar.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/contrib/scalar/scalar.c b/contrib/scalar/scalar.c
index 28d915ec006..f390f519d26 100644
--- a/contrib/scalar/scalar.c
+++ b/contrib/scalar/scalar.c
@@ -254,6 +254,16 @@ static int start_fsmonitor_daemon(void)
 	return 0;
 }
 
+static int stop_fsmonitor_daemon(void)
+{
+	assert(fsmonitor_ipc__is_supported());
+
+	if (fsmonitor_ipc__get_state() == IPC_STATE__LISTENING)
+		return run_git("fsmonitor--daemon", "stop", NULL);
+
+	return 0;
+}
+
 static int register_dir(void)
 {
 	if (add_or_remove_enlistment(1))
@@ -281,6 +291,9 @@ static int unregister_dir(void)
 	if (add_or_remove_enlistment(0))
 		res = error(_("could not remove enlistment"));
 
+	if (fsmonitor_ipc__is_supported() && stop_fsmonitor_daemon() < 0)
+		res = error(_("could not stop the FSMonitor daemon"));
+
 	return res;
 }
 
-- 
gitgitgadget

[PATCH v2 5/5] scalar: update technical doc roadmap with FSMonitor support

[Victoria Dye via GitGitGadget]

From: Victoria Dye <vdye@github.com>

Update the Scalar roadmap to reflect completion of enabling the built-in
FSMonitor in Scalar.

Note that implementation of 'scalar help' was moved to the final set of
changes to move Scalar out of 'contrib/'. This is due to a dependency on
changes to 'git help', as all changes to the main Git tree *exclusively*
implemented to support Scalar are part of that series.

Signed-off-by: Victoria Dye <vdye@github.com>
---
 Documentation/technical/scalar.txt | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/Documentation/technical/scalar.txt b/Documentation/technical/scalar.txt
index 08bc09c225a..047390e46eb 100644
--- a/Documentation/technical/scalar.txt
+++ b/Documentation/technical/scalar.txt
@@ -84,13 +84,13 @@ series have been accepted:
 
 - `scalar-diagnose`: The `scalar` command is taught the `diagnose` subcommand.
 
+- 'scalar-add-fsmonitor: Enable the built-in FSMonitor in Scalar
+  enlistments. At the end of this series, Scalar should be feature-complete
+  from the perspective of a user.
+
 Roughly speaking (and subject to change), the following series are needed to
 "finish" this initial version of Scalar:
 
-- Finish Scalar features: Enable the built-in FSMonitor in Scalar enlistments
-  and implement `scalar help`. At the end of this series, Scalar should be
-  feature-complete from the perspective of a user.
-
 - Generalize features not specific to Scalar: In the spirit of making Scalar
   configure only what is needed for large repo performance, move common
   utilities into other parts of Git. Some of this will be internal-only, but one
@@ -98,9 +98,12 @@ Roughly speaking (and subject to change), the following series are needed to
   repository.
 
 - Move Scalar to toplevel: Move Scalar out of `contrib/` and into the root of
-  `git`, including updates to build and install it with the rest of Git. This
-  change will incorporate Scalar into the Git CI and test framework, as well as
-  expand regression and performance testing to ensure the tool is stable.
+  `git`. This includes a variety of related updates, including:
+    - building & installing Scalar in the Git root-level 'make [install]'.
+    - builing & testing Scalar as part of CI.
+    - moving and expanding test coverage of Scalar (including perf tests).
+    - implementing 'scalar help'/'git help scalar' to display scalar
+      documentation.
 
 Finally, there are two additional patch series that exist in Microsoft's fork of
 Git, but there is no current plan to upstream them. There are some interesting
-- 
gitgitgadget

Re: [PATCH v2 1/5] scalar-unregister: handle error codes greater than 0

[Junio C Hamano]

"Victoria Dye via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Victoria Dye <vdye@github.com>
>
> When 'scalar unregister' tries to disable maintenance and remove an
> enlistment, ensure that the return value is nonzero if either operation
> produces *any* nonzero return value, not just when they return a value less
> than 0.

Interesting.  Did this actually cause problems in the wild?  Just
being curious.

The return values from toggle_maintenance() and add_or_remove() are
what scalar.c::run_git() returns, which in turn come from
run_command() and eventually come from wait_or_whine(), so it very
well can be a positive non-zero value that signals a failure.  It is
good to be prepared to see not just negative values but also
positive ones.

> Signed-off-by: Victoria Dye <vdye@github.com>
> ---
>  contrib/scalar/scalar.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/contrib/scalar/scalar.c b/contrib/scalar/scalar.c
> index 97e71fe19cd..e888fa5408e 100644
> --- a/contrib/scalar/scalar.c
> +++ b/contrib/scalar/scalar.c
> @@ -253,10 +253,10 @@ static int unregister_dir(void)
>  {
>  	int res = 0;
>  
> -	if (toggle_maintenance(0) < 0)
> +	if (toggle_maintenance(0))
>  		res = -1;
>  
> -	if (add_or_remove_enlistment(0) < 0)
> +	if (add_or_remove_enlistment(0))
>  		res = -1;
>  
>  	return res;

Re: [PATCH v2 3/5] scalar: enable built-in FSMonitor on `register`

[Derrick Stolee]

On 8/16/2022 7:58 PM, Matthew John Cheetham via GitGitGadget wrote:

> +#ifdef HAVE_FSMONITOR_DAEMON_BACKEND
> +		/*
> +		 * Enable the built-in FSMonitor on supported platforms.
> +		 */
> +		{ "core.fsmonitor", "true" },
> +#endif
> +	if (fsmonitor_ipc__is_supported() && start_fsmonitor_daemon())
> +		return error(_("could not start the FSMonitor daemon"));
> +

I initially worried if fsmonitor_ipc__is_supported() could use some
run-time information to detect if FS Monitor is supported (say, existence
of a network share or something). However, that implementation is
currently defined as a constant depending on
HAVE_FSMONITOR_DAEMON_BACKEND.

The reason I was worried is that we could enable core.fsmonitor=true based
on the compile-time macro, but then avoid starting the daemon based on the
run-time results. If we get into this state, would the user's 'git status'
calls start complaining about the core.fsmonitor=true config because it is
not supported?

The most future-proof thing to do might be to move the config write out of
the set_recommended_config() and into start_fsmonitor_daemon(). Perhaps
rename it to enable_fsmonitor() so it can fail due to writing the config
_or_ for starting the daemon. The error message would change, then, too.

Or maybe I'm making a mountain out of a mole hill and what exists here is
perfectly fine.

> +test_lazy_prereq BUILTIN_FSMONITOR '
> +	git version --build-options | grep -q "feature:.*fsmonitor--daemon"
> +'

It looks like we already have a FSMONITOR_DAEMON prereq in test-lib.sh.
Should we use that instead?

Thanks,
-Stolee

Re: [PATCH v2 4/5] scalar unregister: stop FSMonitor daemon

[Derrick Stolee]

On 8/16/2022 7:58 PM, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> 
> Especially on Windows, we will need to stop that daemon, just in case
> that the directory needs to be removed (the daemon would otherwise hold
> a handle to that directory, preventing it from being deleted).

> +static int stop_fsmonitor_daemon(void)
> +{
> +	assert(fsmonitor_ipc__is_supported());
> +
> +	if (fsmonitor_ipc__get_state() == IPC_STATE__LISTENING)
> +		return run_git("fsmonitor--daemon", "stop", NULL);
> +
> +	return 0;
> +}
> +
>  static int register_dir(void)
>  {
>  	if (add_or_remove_enlistment(1))
> @@ -281,6 +291,9 @@ static int unregister_dir(void)
>  	if (add_or_remove_enlistment(0))
>  		res = error(_("could not remove enlistment"));
>  
> +	if (fsmonitor_ipc__is_supported() && stop_fsmonitor_daemon() < 0)
> +		res = error(_("could not stop the FSMonitor daemon"));
> +

One thing that is interesting about 'scalar unregister' is that it does
not change config values. At that point, we don't know which config values
are valuable to keep or not because the user may have set them before
'scalar register', or otherwise liked the config options.

Here, the reason to stop the daemon is so we unlock the ability to delete
the directory on Windows.

Should this become part of cmd_delete() instead of unregister_dir()? Or,
do we think that users would opt to run 'scalar unregister' before trying
to delete their directory manually?

Thanks,
-Stolee

Re: [PATCH v2 3/5] scalar: enable built-in FSMonitor on `register`

[Junio C Hamano]

"Matthew John Cheetham via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> +static int start_fsmonitor_daemon(void)
> +{
> +	assert(fsmonitor_ipc__is_supported());
> +
> +	if (fsmonitor_ipc__get_state() != IPC_STATE__LISTENING)
> +		return run_git("fsmonitor--daemon", "start", NULL);
> +
> +	return 0;
> +}

The function got ultra simple ;-).

> @@ -247,6 +265,9 @@ static int register_dir(void)
>  	if (toggle_maintenance(1))
>  		return error(_("could not turn on maintenance"));
>  
> +	if (fsmonitor_ipc__is_supported() && start_fsmonitor_daemon())
> +		return error(_("could not start the FSMonitor daemon"));
> +
>  	return 0;
>  }

As long as it is done consistently, I do not think it makes a huge
difference between the "call it only when supported" and "when asked
to do what we do not support, silently succeed without doing
anything".  It however makes the code appear to be more in control
to do it this way, I think, which is good.

Re: [PATCH v2 0/5] scalar: enable built-in FSMonitor

[Derrick Stolee]

On 8/16/2022 7:58 PM, Victoria Dye via GitGitGadget wrote:
> This series enables the built-in FSMonitor [1] on 'scalar'-registered
> repository enlistments. To avoid errors when unregistering an enlistment,
> the FSMonitor daemon is explicitly stopped during 'scalar unregister'.

I hadn't looked at this code in a while, so I poked around and
asked some questions that might not even need answering.

Outside of a nit involving a test prereq, this version looks
fine to me.

Thanks,
-Stolee

Re: [PATCH v2 3/5] scalar: enable built-in FSMonitor on `register`

[Junio C Hamano]

Derrick Stolee <derrickstolee@github.com> writes:

> On 8/16/2022 7:58 PM, Matthew John Cheetham via GitGitGadget wrote:
>
>> +#ifdef HAVE_FSMONITOR_DAEMON_BACKEND
>> +		/*
>> +		 * Enable the built-in FSMonitor on supported platforms.
>> +		 */
>> +		{ "core.fsmonitor", "true" },
>> +#endif
>> +	if (fsmonitor_ipc__is_supported() && start_fsmonitor_daemon())
>> +		return error(_("could not start the FSMonitor daemon"));
>> +
>
> I initially worried if fsmonitor_ipc__is_supported() could use some
> run-time information to detect if FS Monitor is supported (say, existence
> of a network share or something). However, that implementation is
> currently defined as a constant depending on
> HAVE_FSMONITOR_DAEMON_BACKEND.
>
> The reason I was worried is that we could enable core.fsmonitor=true based
> on the compile-time macro, but then avoid starting the daemon based on the
> run-time results. If we get into this state, would the user's 'git status'
> calls start complaining about the core.fsmonitor=true config because it is
> not supported?

Ah, I didn't consider the possibility where the user uses the
configuration to say "enable it if you are able, but it is OK if you
cannot".  Whether the "is supported" is dynamic or compiled-in, that
may be a valid issue to consider.  An easy way out may be to declare
that the value "true" for "core.fsmonitor" variable means exactly
that, i.e. the user asks to run it, but it is not an error if it
cannot run.

A variant that may need slightly more work would be to introduce a
separate value (perhaps "when-able") that means that, while keeping
the "true" to mean "run the built-in one, or error out to let me
know otherwise" as before.

Thanks.

Re: [PATCH v2 4/5] scalar unregister: stop FSMonitor daemon

[Victoria Dye]

Derrick Stolee wrote:
> On 8/16/2022 7:58 PM, Johannes Schindelin via GitGitGadget wrote:
>> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>>
>> Especially on Windows, we will need to stop that daemon, just in case
>> that the directory needs to be removed (the daemon would otherwise hold
>> a handle to that directory, preventing it from being deleted).
> 
>> +static int stop_fsmonitor_daemon(void)
>> +{
>> +	assert(fsmonitor_ipc__is_supported());
>> +
>> +	if (fsmonitor_ipc__get_state() == IPC_STATE__LISTENING)
>> +		return run_git("fsmonitor--daemon", "stop", NULL);
>> +
>> +	return 0;
>> +}
>> +
>>  static int register_dir(void)
>>  {
>>  	if (add_or_remove_enlistment(1))
>> @@ -281,6 +291,9 @@ static int unregister_dir(void)
>>  	if (add_or_remove_enlistment(0))
>>  		res = error(_("could not remove enlistment"));
>>  
>> +	if (fsmonitor_ipc__is_supported() && stop_fsmonitor_daemon() < 0)
>> +		res = error(_("could not stop the FSMonitor daemon"));
>> +
> 
> One thing that is interesting about 'scalar unregister' is that it does
> not change config values. At that point, we don't know which config values
> are valuable to keep or not because the user may have set them before
> 'scalar register', or otherwise liked the config options.
> 
> Here, the reason to stop the daemon is so we unlock the ability to delete
> the directory on Windows.
> 
> Should this become part of cmd_delete() instead of unregister_dir()? Or,
> do we think that users would opt to run 'scalar unregister' before trying
> to delete their directory manually?

After reading this, my first thought was that 'scalar unregister' should
still turn off the FSMonitor daemon because, in addition to allowing for
directory deletion in 'scalar delete', it's "cleaning up" some
optionally-enabled behavior associated with Scalar (a la
'toggle_maintenance(0)'). However, given that 'unregister' doesn't clear
'core.fsmonitor', it really *isn't* comparable to 'toggle_maintenance(0)'.

So I think you're right that it should only be associated with enlistment
deletion (although I think 'delete_enlistment()' is the place for that -
right before 'remove_dir_recursively()' - rather than 'cmd_delete()').

Thanks!

> 
> Thanks,
> -Stolee

Re: [PATCH v2 4/5] scalar unregister: stop FSMonitor daemon

[Derrick Stolee]

On 8/17/2022 1:36 PM, Victoria Dye wrote:

> So I think you're right that it should only be associated with enlistment
> deletion (although I think 'delete_enlistment()' is the place for that -
> right before 'remove_dir_recursively()' - rather than 'cmd_delete()').

Ah. You're absolutely right about that.

Thanks,
-Stolee

Re: [PATCH v2 3/5] scalar: enable built-in FSMonitor on `register`

[Victoria Dye]

Derrick Stolee wrote:
> On 8/16/2022 7:58 PM, Matthew John Cheetham via GitGitGadget wrote:
> 
>> +#ifdef HAVE_FSMONITOR_DAEMON_BACKEND
>> +		/*
>> +		 * Enable the built-in FSMonitor on supported platforms.
>> +		 */
>> +		{ "core.fsmonitor", "true" },
>> +#endif
>> +	if (fsmonitor_ipc__is_supported() && start_fsmonitor_daemon())
>> +		return error(_("could not start the FSMonitor daemon"));
>> +
> 
> I initially worried if fsmonitor_ipc__is_supported() could use some
> run-time information to detect if FS Monitor is supported (say, existence
> of a network share or something). However, that implementation is
> currently defined as a constant depending on
> HAVE_FSMONITOR_DAEMON_BACKEND.
> 
> The reason I was worried is that we could enable core.fsmonitor=true based
> on the compile-time macro, but then avoid starting the daemon based on the
> run-time results. If we get into this state, would the user's 'git status'
> calls start complaining about the core.fsmonitor=true config because it is
> not supported?
> 
> The most future-proof thing to do might be to move the config write out of
> the set_recommended_config() and into start_fsmonitor_daemon(). Perhaps
> rename it to enable_fsmonitor() so it can fail due to writing the config
> _or_ for starting the daemon. The error message would change, then, too.

I spent some time digging into this, and I think gating both the config and
subsequent 'git fsmonitor--daemon start' on having platform *and* repository
support is a good idea. I'll update the next version to both set the
'core.fsmonitor' config and start the daemon only if the built-in FSMonitor
is fully supported.

(warning: long-winded tangent mostly unrelated to FSMonitor)

In the process of testing FSMonitor behavior, I think found other issues
with Scalar registration. Specifically, the test I wrote attempted to
'scalar register' a bare repo, since bare directories are incompatible with
FSMonitor. After seeing that FSMonitor was *not* incompatible with the
repository, I found that Scalar was 1) ignoring the bare repository and, as
a result, 2) identifying my Git clone (way above GIT_CEILING_DIRECTORIES) as
the "enlistment root". I think 1) might be fine as-is - uniformly ignoring
bare repos seems like a reasonable choice - but 2) seems like more of a
problem. 

Right now, 'setup_enlistment_directory()' searches for the repo root
beginning at directory '<dir>', which is either a user-provided path or
current working directory. It checks whether '<dir>' or '<dir>/src' is a
repo root: if so, it sets the enlistment info; otherwise, it repeats the
process with the parent of '<dir>' until the repo root is found. For
example, given the following directory structure:

somedir
└── enlistment
    ├── src
    │   └── .git
    └── test
        └── data

'scalar register somedir/enlistment/test/data' will search:

  * somedir/enlistment/test/data/src
  * somedir/enlistment/test/data
  * somedir/enlistment/test/src
  * somedir/enlistment/test
  * somedir/enlistment/src

The current usage of GIT_CEILING_DIRECTORIES relies on the fact that, when
invoking a normal 'git' command, 'setup_git_directory()' only searches
upwards from the current working directory to find the repo root; it's a
clear "yes" or "no" as to whether that search passes a ceiling directory.
Scalar isn't as clear, since it searches for the repo root both "downwards"
into '<dir>/src' *and* upwards through the parents of '<dir>'. It's not
totally clear to me what the "right" behavior for Scalar is, but my current
thought is to follow the same rules as 'setup_git_directory()', but for the
*enlistment* root rather than the repository root. It's more restrictive
than GIT_CEILING_DIRECTORIES on a normal git repo, e.g.:

1. 'GIT_CEILING_DIRECTORIES=somedir/enlistment git -C somedir/enlistment/src status' 
   is valid.
2. 'GIT_CEILING_DIRECTORIES=somedir/enlistment scalar register somedir/enlistment/src'
   is not valid.

but since Scalar works on the entire enlistment (not just the repo inside of
it), I think it makes sense to prevent it from crossing a ceiling directory
boundary.

What do you think? Hopefully my rambling wasn't too confusing (if it is,
please let me know what I can clarify). 

> 
> Or maybe I'm making a mountain out of a mole hill and what exists here is
> perfectly fine.
> 
>> +test_lazy_prereq BUILTIN_FSMONITOR '
>> +	git version --build-options | grep -q "feature:.*fsmonitor--daemon"
>> +'
> 
> It looks like we already have a FSMONITOR_DAEMON prereq in test-lib.sh.
> Should we use that instead?

Works for me, happy to reuse code wherever possible. :)

> 
> Thanks,
> -Stolee

Re: [PATCH v2 3/5] scalar: enable built-in FSMonitor on `register`

[Derrick Stolee]

On 8/17/2022 7:47 PM, Victoria Dye wrote:

> (warning: long-winded tangent mostly unrelated to FSMonitor)
> 
> In the process of testing FSMonitor behavior, I think found other issues
> with Scalar registration. Specifically, the test I wrote attempted to
> 'scalar register' a bare repo, since bare directories are incompatible with
> FSMonitor. After seeing that FSMonitor was *not* incompatible with the
> repository, I found that Scalar was 1) ignoring the bare repository and, as

This is interesting, that Scalar doesn't recognize a bare repo. There are
definitely some config settings that it recommends that don't make sense
in a bare repo, but it's interesting that it completely ignores it. Good
find.

I'm not sure there is anything to 'fix' except maybe error out when the
discovered Git repository is bare. Add a warning, at minimum.

> a result, 2) identifying my Git clone (way above GIT_CEILING_DIRECTORIES) as
> the "enlistment root". I think 1) might be fine as-is - uniformly ignoring
> bare repos seems like a reasonable choice - but 2) seems like more of a
> problem. 

...

> The current usage of GIT_CEILING_DIRECTORIES relies on the fact that, when
> invoking a normal 'git' command, 'setup_git_directory()' only searches
> upwards from the current working directory to find the repo root; it's a
> clear "yes" or "no" as to whether that search passes a ceiling directory.
> Scalar isn't as clear, since it searches for the repo root both "downwards"
> into '<dir>/src' *and* upwards through the parents of '<dir>'. It's not
> totally clear to me what the "right" behavior for Scalar is, but my current
> thought is to follow the same rules as 'setup_git_directory()', but for the
> *enlistment* root rather than the repository root. It's more restrictive
> than GIT_CEILING_DIRECTORIES on a normal git repo, e.g.:
> 
> 1. 'GIT_CEILING_DIRECTORIES=somedir/enlistment git -C somedir/enlistment/src status' 
>    is valid.
> 2. 'GIT_CEILING_DIRECTORIES=somedir/enlistment scalar register somedir/enlistment/src'
>    is not valid.

This is interesting, that we can't recognize the ceiling as the root.

> but since Scalar works on the entire enlistment (not just the repo inside of
> it), I think it makes sense to prevent it from crossing a ceiling directory
> boundary.

I think the enlistment root was something that was inherited from VFS for
Git, and we can mostly abandon it. The things we need to do are all based
on the Git repository itself, not the parent. The only thing we need to
keep is to allow a user to specify the repo by pointing to the directory
immediately above the 'src' directory.

> 'scalar register somedir/enlistment/test/data' will search:
> 
>   * somedir/enlistment/test/data/src
>   * somedir/enlistment/test/data
>   * somedir/enlistment/test/src
>   * somedir/enlistment/test
>   * somedir/enlistment/src

Instead, we could do the following on a specified <dir>:

 * If <dir>/src exists, find the Git directory by finding the first Git
   repository containing <dir>/src.
 * Otherwise, find the first Git repository containing <dir>.

Is there an easy way to discover a Git repository at a specific directory?
Or, do we do something simpler, like changing directories then calling
setup_git_directory()? I think simplifying the logic that way should
respect GIT_CEILING_DIRECTORIES correctly.

Thanks,
-Stolee

[PATCH v3 0/8] scalar: enable built-in FSMonitor

[Victoria Dye via GitGitGadget]

This series enables the built-in FSMonitor [1] on 'scalar'-registered
repository enlistments. To avoid errors when unregistering an enlistment,
the FSMonitor daemon is explicitly stopped during 'scalar unregister'.

Maintainer's note: this series has a minor conflict with
'vd/scalar-generalize-diagnose'. Please let me know if there's anything else
I can provide (in addition to [2]) that would make resolution easier.


Changes since V2
================

 * Updated prerequisites for FSMonitor in Scalar to include
   'fsm_settings__get_reason(the_repository) == FSMONITOR_REASON_OK' to
   handle cases where the platform is supported, but the repository is not.
 * Gated enabling the 'core.fsmonitor' on FSMonitor compatibility with the
   repo.
 * Replaced 'die()' failures in 'delete_enlistment()' with 'error()'s.
 * Replaced 'BUILTIN_FSMONITOR' test prerequisite with already-existing
   'FSMONITOR_DAEMON' for FSMonitor.
 * Rewrote Scalar enlistment/repo search in 'setup_enlistment_directory()'
   to avoid unconstrained search and respect 'GIT_CEILING_DIRECTORIES'.
   Added tests to show the new expected behavior.


Changes since V1
================

 * Added a patch to fix 'unregister_dir()'s handling of return values > 0
   from 'toggle_maintenance()' and 'add_or_remove_enlistment()'.
 * Added a patch to print error messages in 'register_dir()' and
   'unregister_dir()' indicating which of their internal steps fail.
 * Moved check of 'fsmonitor_ipc__is_supported()' to '[un]register_dir()' to
   avoid calling '(start|stop)_fsmonitor_daemon()' when the feature is not
   supported. Added assertion of 'fsmonitor_ipc__is_supported()' to
   '(start|stop)_fsmonitor_daemon()' to enforce that they are not called
   when the feature is unavailable.
 * Simplified '(start|stop)_fsmonitor_daemon()' implementation. Now, if
   FSMonitor is already running/stopped (respectively), the function simply
   returns 0; otherwise, it runs 'git fsmonitor--daemon (start|stop)' and
   returns the exit code.
   * Note that the "could not (start|stop) the FSMonitor daemon: <err_msg>"
     error messages are no longer printed by
     '(start|stop)_fsmonitor_daemon()'. Instead, "<err_msg>" is printed to
     stderr by swapping 'pipe_command()' out for 'run_git()', and
     '[un]register_dir()' prints the "could not (start|stop) the FSMonitor
     daemon" message.

Thanks

 * Victoria

[1]
https://lore.kernel.org/git/pull.1143.git.1644940773.gitgitgadget@gmail.com/

[2] The conflict is a result of both series updating the Scalar roadmap doc.
For reference, my merge resolution (from git diff <merge commit> <merge
commit>^1 <merge commit>^2, where <merge commit>^1 is
'vd/scalar-generalize-diagnose' and <merge commit>^2 is this series) looks
like:

------------->8------------->8------------->8------------->8------------->8-------------
diff --cc Documentation/technical/scalar.txt
index f6353375f0,047390e46e..0600150b3a
--- a/Documentation/technical/scalar.txt
+++ b/Documentation/technical/scalar.txt
@@@ -84,20 -84,26 +84,23 @@@ series have been accepted
  
  - `scalar-diagnose`: The `scalar` command is taught the `diagnose` subcommand.
  
 +- `scalar-generalize-diagnose`: Move the functionality of `scalar diagnose`
 +  into `git diagnose` and `git bugreport --diagnose`.
 +
+ - 'scalar-add-fsmonitor: Enable the built-in FSMonitor in Scalar
+   enlistments. At the end of this series, Scalar should be feature-complete
+   from the perspective of a user.
+ 
  Roughly speaking (and subject to change), the following series are needed to
  "finish" this initial version of Scalar:
  
- - Finish Scalar features: Enable the built-in FSMonitor in Scalar enlistments
-   and implement `scalar help`. At the end of this series, Scalar should be
-   feature-complete from the perspective of a user.
 -- Generalize features not specific to Scalar: In the spirit of making Scalar
 -  configure only what is needed for large repo performance, move common
 -  utilities into other parts of Git. Some of this will be internal-only, but one
 -  major change will be generalizing `scalar diagnose` for use with any Git
 -  repository.
--
  - Move Scalar to toplevel: Move Scalar out of `contrib/` and into the root of
-   `git`, including updates to build and install it with the rest of Git. This
-   change will incorporate Scalar into the Git CI and test framework, as well as
-   expand regression and performance testing to ensure the tool is stable.
+   `git`. This includes a variety of related updates, including:
+     - building & installing Scalar in the Git root-level 'make [install]'.
+     - builing & testing Scalar as part of CI.
+     - moving and expanding test coverage of Scalar (including perf tests).
+     - implementing 'scalar help'/'git help scalar' to display scalar
+       documentation.
  
  Finally, there are two additional patch series that exist in Microsoft's fork of
  Git, but there is no current plan to upstream them. There are some interesting
-------------8<-------------8<-------------8<-------------8<-------------8<---------


Johannes Schindelin (1):
  scalar unregister: stop FSMonitor daemon

Matthew John Cheetham (1):
  scalar: enable built-in FSMonitor on `register`

Victoria Dye (6):
  scalar: constrain enlistment search
  scalar-unregister: handle error codes greater than 0
  scalar-[un]register: clearly indicate source of error
  scalar-delete: do not 'die()' in 'delete_enlistment()'
  scalar: move config setting logic into its own function
  scalar: update technical doc roadmap with FSMonitor support

 Documentation/technical/scalar.txt |  17 ++-
 contrib/scalar/scalar.c            | 201 +++++++++++++++++------------
 contrib/scalar/t/t9099-scalar.sh   |  93 +++++++++++++
 3 files changed, 220 insertions(+), 91 deletions(-)


base-commit: 4af7188bc97f70277d0f10d56d5373022b1fa385
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1324%2Fvdye%2Fscalar%2Fadd-fsmonitor-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1324/vdye/scalar/add-fsmonitor-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/1324

Range-diff vs v2:

 -:  ----------- > 1:  2f6cad83613 scalar: constrain enlistment search
 1:  36fc3cb604d = 2:  a6bb0113b9c scalar-unregister: handle error codes greater than 0
 2:  4bacf8bce8a = 3:  aea8723e718 scalar-[un]register: clearly indicate source of error
 -:  ----------- > 4:  aced836aaa3 scalar-delete: do not 'die()' in 'delete_enlistment()'
 -:  ----------- > 5:  f8471e94e83 scalar: move config setting logic into its own function
 3:  5fdf8337972 ! 6:  fb379fd2097 scalar: enable built-in FSMonitor on `register`
     @@ Commit message
          file system monitor such as e.g. Watchman).
      
          Helped-by: Junio C Hamano <gitster@pobox.com>
     +    Helped-by: Derrick Stolee <derrickstolee@github.com>
          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
       #include "run-command.h"
      +#include "simple-ipc.h"
      +#include "fsmonitor-ipc.h"
     ++#include "fsmonitor-settings.h"
       #include "refs.h"
       #include "dir.h"
       #include "packfile.h"
     +@@ contrib/scalar/scalar.c: static int set_scalar_config(const struct scalar_config *config, int reconfigure
     + 	return res;
     + }
     + 
     ++static int have_fsmonitor_support(void)
     ++{
     ++	return fsmonitor_ipc__is_supported() &&
     ++	       fsm_settings__get_reason(the_repository) == FSMONITOR_REASON_OK;
     ++}
     ++
     + static int set_recommended_config(int reconfigure)
     + {
     + 	struct scalar_config config[] = {
      @@ contrib/scalar/scalar.c: 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;
     + 				     config[i].key, config[i].value);
     + 	}
     + 
     ++	if (have_fsmonitor_support()) {
     ++		struct scalar_config fsmonitor = { "core.fsmonitor", "true" };
     ++		if (set_scalar_config(&fsmonitor, reconfigure))
     ++			return error(_("could not configure %s=%s"),
     ++				     fsmonitor.key, fsmonitor.value);
     ++	}
     ++
     + 	/*
     + 	 * The `log.excludeDecoration` setting is special because it allows
     + 	 * for multiple values.
      @@ contrib/scalar/scalar.c: static int add_or_remove_enlistment(int add)
       		       "scalar.repo", the_repository->worktree, NULL);
       }
       
      +static int start_fsmonitor_daemon(void)
      +{
     -+	assert(fsmonitor_ipc__is_supported());
     ++	assert(have_fsmonitor_support());
      +
      +	if (fsmonitor_ipc__get_state() != IPC_STATE__LISTENING)
      +		return run_git("fsmonitor--daemon", "start", NULL);
     @@ contrib/scalar/scalar.c: static int register_dir(void)
       	if (toggle_maintenance(1))
       		return error(_("could not turn on maintenance"));
       
     -+	if (fsmonitor_ipc__is_supported() && start_fsmonitor_daemon())
     ++	if (have_fsmonitor_support() && start_fsmonitor_daemon()) {
      +		return error(_("could not start the FSMonitor daemon"));
     ++	}
      +
       	return 0;
       }
       
      
       ## contrib/scalar/t/t9099-scalar.sh ##
     -@@ contrib/scalar/t/t9099-scalar.sh: 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
     +@@ contrib/scalar/t/t9099-scalar.sh: test_expect_success 'scalar enlistments need a worktree' '
     + 	grep "Scalar enlistments require a worktree" err
       '
       
     -+test_expect_success BUILTIN_FSMONITOR 'scalar register starts fsmon daemon' '
     ++test_expect_success FSMONITOR_DAEMON '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
     ++	git -C test/src fsmonitor--daemon status &&
     ++	test_cmp_config -C test/src true core.fsmonitor
      +'
      +
       test_expect_success 'scalar unregister' '
 4:  fc4aa1fde31 ! 7:  bb58a78fdb2 scalar unregister: stop FSMonitor daemon
     @@ contrib/scalar/scalar.c: static int start_fsmonitor_daemon(void)
       
      +static int stop_fsmonitor_daemon(void)
      +{
     -+	assert(fsmonitor_ipc__is_supported());
     ++	assert(have_fsmonitor_support());
      +
      +	if (fsmonitor_ipc__get_state() == IPC_STATE__LISTENING)
      +		return run_git("fsmonitor--daemon", "stop", NULL);
     @@ contrib/scalar/scalar.c: static int start_fsmonitor_daemon(void)
       static int register_dir(void)
       {
       	if (add_or_remove_enlistment(1))
     -@@ contrib/scalar/scalar.c: static int unregister_dir(void)
     - 	if (add_or_remove_enlistment(0))
     - 		res = error(_("could not remove enlistment"));
     +@@ contrib/scalar/scalar.c: static int delete_enlistment(struct strbuf *enlistment)
     + 	strbuf_release(&parent);
     + #endif
       
     -+	if (fsmonitor_ipc__is_supported() && stop_fsmonitor_daemon() < 0)
     -+		res = error(_("could not stop the FSMonitor daemon"));
     ++	if (have_fsmonitor_support() && stop_fsmonitor_daemon())
     ++		return error(_("failed to stop the FSMonitor daemon"));
      +
     - 	return res;
     - }
     + 	if (remove_dir_recursively(enlistment, 0))
     + 		return error(_("failed to delete enlistment directory"));
       
 5:  dd59caa2e5a = 8:  7cee014e2d2 scalar: update technical doc roadmap with FSMonitor support

-- 
gitgitgadget

[PATCH v3 1/8] scalar: constrain enlistment search

[Victoria Dye via GitGitGadget]

From: Victoria Dye <vdye@github.com>

Make the search for repository and enlistment root in
'setup_enlistment_directory()' more constrained to simplify behavior and
adhere to 'GIT_CEILING_DIRECTORIES'.

Previously, 'setup_enlistment_directory()' would check whether the provided
path (or current working directory) '<dir>' or its subdirectory '<dir>/src'
was a repository root. If not, the process would repeat on the parent of
'<dir>' until the repository was found or it reached the root of the
filesystem. This meant that a user could specify a path *anywhere* inside an
enlistment (including paths not in the repository contained within the
enlistment) and it would be found.

The downside to this process is that the search would not account for
'GIT_CEILING_DIRECTORIES', so the upward search could result in modifying
repository contents past 'GIT_CEILING_DIRECTORIES'. Similarly, operations
like 'scalar delete' could end up unintentionally deleting the parent of a
repo if its root was named 'src'.

To make this 'setup_enlistment_directory()' both adhere to
'GIT_CEILING_DIRECTORIES' and avoid unwanted deletions, the search for an
enlistment directory is simplified to:

- if '<dir>/src' is a repository root, '<dir>' is the enlistment root
- if '<dir>' is either the repository root or contained within a repository,
  the repository root is the enlistment root

Now, only 'setup_git_directory()' (called by 'setup_enlistment_directory()')
searches upwards from the 'scalar' specified path, enforcing
'GIT_CEILING_DIRECTORIES' in the process. Additionally, 'scalar delete
<dir>/src' will not delete '<dir>' (if users would like to delete it, they
can still specify the enlistment root with 'scalar delete <dir>'). This is
true of any 'scalar' operation; users can invoke 'scalar' on the enlistment
root, but paths must otherwise be inside the repository to be valid.

To help clarify the updated behavior, new tests are added to
't9099-scalar.sh'.

Finally, this change leaves 'strbuf_parent_directory()' with only a single,
WIN32-specific caller in 'delete_enlistment()'. Rather than wrap
'strbuf_parent_directory()' in '#ifdef WIN32' to avoid the "unused function"
compiler error, move the contents of 'strbuf_parent_directory()' into
'delete_enlistment()' and remove the function.

Helped-by: Derrick Stolee <derrickstolee@github.com>
Signed-off-by: Victoria Dye <vdye@github.com>
---
 contrib/scalar/scalar.c          | 82 +++++++++++-------------------
 contrib/scalar/t/t9099-scalar.sh | 85 ++++++++++++++++++++++++++++++++
 2 files changed, 113 insertions(+), 54 deletions(-)

diff --git a/contrib/scalar/scalar.c b/contrib/scalar/scalar.c
index 97e71fe19cd..92b648f3511 100644
--- a/contrib/scalar/scalar.c
+++ b/contrib/scalar/scalar.c
@@ -14,29 +14,14 @@
 #include "archive.h"
 #include "object-store.h"
 
-/*
- * Remove the deepest subdirectory in the provided path string. Path must not
- * include a trailing path separator. Returns 1 if parent directory found,
- * otherwise 0.
- */
-static int strbuf_parent_directory(struct strbuf *buf)
-{
-	size_t len = buf->len;
-	size_t offset = offset_1st_component(buf->buf);
-	char *path_sep = find_last_dir_sep(buf->buf + offset);
-	strbuf_setlen(buf, path_sep ? path_sep - buf->buf : offset);
-
-	return buf->len < len;
-}
-
 static void setup_enlistment_directory(int argc, const char **argv,
 				       const char * const *usagestr,
 				       const struct option *options,
 				       struct strbuf *enlistment_root)
 {
 	struct strbuf path = STRBUF_INIT;
-	char *root;
-	int enlistment_found = 0;
+	int enlistment_is_repo_parent = 0;
+	size_t len;
 
 	if (startup_info->have_repository)
 		BUG("gitdir already set up?!?");
@@ -49,51 +34,36 @@ static void setup_enlistment_directory(int argc, const char **argv,
 		strbuf_add_absolute_path(&path, argv[0]);
 		if (!is_directory(path.buf))
 			die(_("'%s' does not exist"), path.buf);
+		if (chdir(path.buf) < 0)
+			die_errno(_("could not switch to '%s'"), path.buf);
 	} else if (strbuf_getcwd(&path) < 0)
 		die(_("need a working directory"));
 
 	strbuf_trim_trailing_dir_sep(&path);
-	do {
-		const size_t len = path.len;
-
-		/* check if currently in enlistment root with src/ workdir */
-		strbuf_addstr(&path, "/src");
-		if (is_nonbare_repository_dir(&path)) {
-			if (enlistment_root)
-				strbuf_add(enlistment_root, path.buf, len);
-
-			enlistment_found = 1;
-			break;
-		}
 
-		/* reset to original path */
-		strbuf_setlen(&path, len);
-
-		/* check if currently in workdir */
-		if (is_nonbare_repository_dir(&path)) {
-			if (enlistment_root) {
-				/*
-				 * If the worktree's directory's name is `src`, the enlistment is the
-				 * parent directory, otherwise it is identical to the worktree.
-				 */
-				root = strip_path_suffix(path.buf, "src");
-				strbuf_addstr(enlistment_root, root ? root : path.buf);
-				free(root);
-			}
+	/* check if currently in enlistment root with src/ workdir */
+	len = path.len;
+	strbuf_addstr(&path, "/src");
+	if (is_nonbare_repository_dir(&path)) {
+		enlistment_is_repo_parent = 1;
+		if (chdir(path.buf) < 0)
+			die_errno(_("could not switch to '%s'"), path.buf);
+	}
+	strbuf_setlen(&path, len);
 
-			enlistment_found = 1;
-			break;
-		}
-	} while (strbuf_parent_directory(&path));
+	setup_git_directory();
 
-	if (!enlistment_found)
-		die(_("could not find enlistment root"));
+	if (!the_repository->worktree)
+		die(_("Scalar enlistments require a worktree"));
 
-	if (chdir(path.buf) < 0)
-		die_errno(_("could not switch to '%s'"), path.buf);
+	if (enlistment_root) {
+		if (enlistment_is_repo_parent)
+			strbuf_addbuf(enlistment_root, &path);
+		else
+			strbuf_addstr(enlistment_root, the_repository->worktree);
+	}
 
 	strbuf_release(&path);
-	setup_git_directory();
 }
 
 static int run_git(const char *arg, ...)
@@ -431,6 +401,8 @@ static int delete_enlistment(struct strbuf *enlistment)
 {
 #ifdef WIN32
 	struct strbuf parent = STRBUF_INIT;
+	size_t offset;
+	char *path_sep;
 #endif
 
 	if (unregister_dir())
@@ -441,8 +413,10 @@ static int delete_enlistment(struct strbuf *enlistment)
 	 * Change the current directory to one outside of the enlistment so
 	 * that we may delete everything underneath it.
 	 */
-	strbuf_addbuf(&parent, enlistment);
-	strbuf_parent_directory(&parent);
+	offset = offset_1st_component(enlistment->buf);
+	path_sep = find_last_dir_sep(enlistment->buf + offset);
+	strbuf_add(&parent, enlistment->buf,
+		   path_sep ? path_sep - enlistment->buf : offset);
 	if (chdir(parent.buf) < 0)
 		die_errno(_("could not switch to '%s'"), parent.buf);
 	strbuf_release(&parent);
diff --git a/contrib/scalar/t/t9099-scalar.sh b/contrib/scalar/t/t9099-scalar.sh
index 10b1172a8aa..c069cffebfe 100755
--- a/contrib/scalar/t/t9099-scalar.sh
+++ b/contrib/scalar/t/t9099-scalar.sh
@@ -17,6 +17,91 @@ test_expect_success 'scalar shows a usage' '
 	test_expect_code 129 scalar -h
 '
 
+test_expect_success 'scalar invoked on enlistment root' '
+	test_when_finished rm -rf test src deeper &&
+
+	for enlistment_root in test src deeper/test
+	do
+		git init ${enlistment_root}/src &&
+
+		# Register
+		scalar register ${enlistment_root} &&
+		scalar list >out &&
+		grep "$(pwd)/${enlistment_root}/src\$" out &&
+
+		# Delete (including enlistment root)
+		scalar delete $enlistment_root &&
+		test_path_is_missing $enlistment_root &&
+		scalar list >out &&
+		! grep "^$(pwd)/${enlistment_root}/src\$" out || return 1
+	done
+'
+
+test_expect_success 'scalar invoked on enlistment src repo' '
+	test_when_finished rm -rf test src deeper &&
+
+	for enlistment_root in test src deeper/test
+	do
+		git init ${enlistment_root}/src &&
+
+		# Register
+		scalar register ${enlistment_root}/src &&
+		scalar list >out &&
+		grep "$(pwd)/${enlistment_root}/src\$" out &&
+
+		# Delete (will not include enlistment root)
+		scalar delete ${enlistment_root}/src &&
+		test_path_is_dir $enlistment_root &&
+		scalar list >out &&
+		! grep "^$(pwd)/${enlistment_root}/src\$" out || return 1
+	done
+'
+
+test_expect_success 'scalar invoked when enlistment root and repo are the same' '
+	test_when_finished rm -rf test src deeper &&
+
+	for enlistment_root in test src deeper/test
+	do
+		git init ${enlistment_root} &&
+
+		# Register
+		scalar register ${enlistment_root} &&
+		scalar list >out &&
+		grep "$(pwd)/${enlistment_root}\$" out &&
+
+		# Delete (will not include enlistment root)
+		scalar delete ${enlistment_root} &&
+		test_path_is_missing $enlistment_root &&
+		scalar list >out &&
+		! grep "^$(pwd)/${enlistment_root}\$" out &&
+
+		# Make sure we did not accidentally delete the trash dir
+		test_path_is_dir "$TRASH_DIRECTORY" || return 1
+	done
+'
+
+test_expect_success 'scalar repo search respects GIT_CEILING_DIRECTORIES' '
+	test_when_finished rm -rf test &&
+
+	git init test/src &&
+	mkdir -p test/src/deep &&
+	GIT_CEILING_DIRECTORIES="$(pwd)/test/src" &&
+	! scalar register test/src/deep 2>err &&
+	grep "not a git repository" err
+'
+
+test_expect_success 'scalar enlistments need a worktree' '
+	test_when_finished rm -rf bare test &&
+
+	git init --bare bare/src &&
+	! scalar register bare/src 2>err &&
+	grep "Scalar enlistments require a worktree" err &&
+
+	git init test/src &&
+	! scalar register test/src/.git 2>err &&
+	grep "Scalar enlistments require a worktree" err
+'
+
 test_expect_success 'scalar unregister' '
 	git init vanish/src &&
 	scalar register vanish/src &&
-- 
gitgitgadget

[PATCH v3 2/8] scalar-unregister: handle error codes greater than 0

[Victoria Dye via GitGitGadget]

From: Victoria Dye <vdye@github.com>

When 'scalar unregister' tries to disable maintenance and remove an
enlistment, ensure that the return value is nonzero if either operation
produces *any* nonzero return value, not just when they return a value less
than 0.

Signed-off-by: Victoria Dye <vdye@github.com>
---
 contrib/scalar/scalar.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/contrib/scalar/scalar.c b/contrib/scalar/scalar.c
index 92b648f3511..8ef8dd55041 100644
--- a/contrib/scalar/scalar.c
+++ b/contrib/scalar/scalar.c
@@ -223,10 +223,10 @@ static int unregister_dir(void)
 {
 	int res = 0;
 
-	if (toggle_maintenance(0) < 0)
+	if (toggle_maintenance(0))
 		res = -1;
 
-	if (add_or_remove_enlistment(0) < 0)
+	if (add_or_remove_enlistment(0))
 		res = -1;
 
 	return res;
-- 
gitgitgadget

[PATCH v3 3/8] scalar-[un]register: clearly indicate source of error

[Victoria Dye via GitGitGadget]

From: Victoria Dye <vdye@github.com>

When a step in 'register_dir()' or 'unregister_dir()' fails, indicate which
step failed with an error message, rather than silently assigning a nonzero
return code.

Signed-off-by: Victoria Dye <vdye@github.com>
---
 contrib/scalar/scalar.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/contrib/scalar/scalar.c b/contrib/scalar/scalar.c
index 8ef8dd55041..7be2a938b0c 100644
--- a/contrib/scalar/scalar.c
+++ b/contrib/scalar/scalar.c
@@ -208,15 +208,16 @@ static int add_or_remove_enlistment(int add)
 
 static int register_dir(void)
 {
-	int res = add_or_remove_enlistment(1);
+	if (add_or_remove_enlistment(1))
+		return error(_("could not add enlistment"));
 
-	if (!res)
-		res = set_recommended_config(0);
+	if (set_recommended_config(0))
+		return error(_("could not set recommended config"));
 
-	if (!res)
-		res = toggle_maintenance(1);
+	if (toggle_maintenance(1))
+		return error(_("could not turn on maintenance"));
 
-	return res;
+	return 0;
 }
 
 static int unregister_dir(void)
@@ -224,10 +225,10 @@ static int unregister_dir(void)
 	int res = 0;
 
 	if (toggle_maintenance(0))
-		res = -1;
+		res = error(_("could not turn off maintenance"));
 
 	if (add_or_remove_enlistment(0))
-		res = -1;
+		res = error(_("could not remove enlistment"));
 
 	return res;
 }
-- 
gitgitgadget

[PATCH v3 4/8] scalar-delete: do not 'die()' in 'delete_enlistment()'

[Victoria Dye via GitGitGadget]

From: Victoria Dye <vdye@github.com>

Rather than exiting with 'die()' when 'delete_enlistment()' encounters an
error, return an error code with the appropriate message. There's no need
for an abrupt exit with 'die()' in 'delete_enlistment()' because its only
caller ('cmd_delete()') properly cleans up allocated resources and returns
the 'delete_enlistment()' return value as its own exit code.

Signed-off-by: Victoria Dye <vdye@github.com>
---
 contrib/scalar/scalar.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/contrib/scalar/scalar.c b/contrib/scalar/scalar.c
index 7be2a938b0c..6de4d5b3721 100644
--- a/contrib/scalar/scalar.c
+++ b/contrib/scalar/scalar.c
@@ -407,7 +407,7 @@ static int delete_enlistment(struct strbuf *enlistment)
 #endif
 
 	if (unregister_dir())
-		die(_("failed to unregister repository"));
+		return error(_("failed to unregister repository"));
 
 #ifdef WIN32
 	/*
@@ -418,13 +418,16 @@ static int delete_enlistment(struct strbuf *enlistment)
 	path_sep = find_last_dir_sep(enlistment->buf + offset);
 	strbuf_add(&parent, enlistment->buf,
 		   path_sep ? path_sep - enlistment->buf : offset);
-	if (chdir(parent.buf) < 0)
-		die_errno(_("could not switch to '%s'"), parent.buf);
+	if (chdir(parent.buf) < 0) {
+		int res = error_errno(_("could not switch to '%s'"), parent.buf);
+		strbuf_release(&parent);
+		return res;
+	}
 	strbuf_release(&parent);
 #endif
 
 	if (remove_dir_recursively(enlistment, 0))
-		die(_("failed to delete enlistment directory"));
+		return error(_("failed to delete enlistment directory"));
 
 	return 0;
 }
-- 
gitgitgadget

[PATCH v3 5/8] scalar: move config setting logic into its own function

[Victoria Dye via GitGitGadget]

From: Victoria Dye <vdye@github.com>

Create function 'set_scalar_config()' to contain the logic used in setting
Scalar-defined Git config settings, including how to handle reconfiguring &
overwriting existing values. This function allows future patches to set
config values in parts of 'scalar.c' other than 'set_recommended_config()'.

Signed-off-by: Victoria Dye <vdye@github.com>
---
 contrib/scalar/scalar.c | 44 ++++++++++++++++++++++++++---------------
 1 file changed, 28 insertions(+), 16 deletions(-)

diff --git a/contrib/scalar/scalar.c b/contrib/scalar/scalar.c
index 6de4d5b3721..836a4c48fab 100644
--- a/contrib/scalar/scalar.c
+++ b/contrib/scalar/scalar.c
@@ -85,13 +85,33 @@ static int run_git(const char *arg, ...)
 	return res;
 }
 
+struct scalar_config {
+	const char *key;
+	const char *value;
+	int overwrite_on_reconfigure;
+};
+
+static int set_scalar_config(const struct scalar_config *config, int reconfigure)
+{
+	char *value = NULL;
+	int res;
+
+	if ((reconfigure && config->overwrite_on_reconfigure) ||
+	    git_config_get_string(config->key, &value)) {
+		trace2_data_string("scalar", the_repository, config->key, "created");
+		res = git_config_set_gently(config->key, config->value);
+	} else {
+		trace2_data_string("scalar", the_repository, config->key, "exists");
+		res = 0;
+	}
+
+	free(value);
+	return res;
+}
+
 static int set_recommended_config(int reconfigure)
 {
-	struct {
-		const char *key;
-		const char *value;
-		int overwrite_on_reconfigure;
-	} config[] = {
+	struct scalar_config config[] = {
 		/* Required */
 		{ "am.keepCR", "true", 1 },
 		{ "core.FSCache", "true", 1 },
@@ -145,17 +165,9 @@ static int set_recommended_config(int reconfigure)
 	char *value;
 
 	for (i = 0; config[i].key; i++) {
-		if ((reconfigure && config[i].overwrite_on_reconfigure) ||
-		    git_config_get_string(config[i].key, &value)) {
-			trace2_data_string("scalar", the_repository, config[i].key, "created");
-			if (git_config_set_gently(config[i].key,
-						  config[i].value) < 0)
-				return error(_("could not configure %s=%s"),
-					     config[i].key, config[i].value);
-		} else {
-			trace2_data_string("scalar", the_repository, config[i].key, "exists");
-			free(value);
-		}
+		if (set_scalar_config(config + i, reconfigure))
+			return error(_("could not configure %s=%s"),
+				     config[i].key, config[i].value);
 	}
 
 	/*
-- 
gitgitgadget

[PATCH v3 6/8] scalar: enable built-in FSMonitor on `register`

[Matthew John Cheetham via GitGitGadget]

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).

Helped-by: Junio C Hamano <gitster@pobox.com>
Helped-by: Derrick Stolee <derrickstolee@github.com>
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          | 30 ++++++++++++++++++++++++++++++
 contrib/scalar/t/t9099-scalar.sh |  8 ++++++++
 2 files changed, 38 insertions(+)

diff --git a/contrib/scalar/scalar.c b/contrib/scalar/scalar.c
index 836a4c48fab..73cd5b1fd0c 100644
--- a/contrib/scalar/scalar.c
+++ b/contrib/scalar/scalar.c
@@ -7,6 +7,9 @@
 #include "parse-options.h"
 #include "config.h"
 #include "run-command.h"
+#include "simple-ipc.h"
+#include "fsmonitor-ipc.h"
+#include "fsmonitor-settings.h"
 #include "refs.h"
 #include "dir.h"
 #include "packfile.h"
@@ -109,6 +112,12 @@ static int set_scalar_config(const struct scalar_config *config, int reconfigure
 	return res;
 }
 
+static int have_fsmonitor_support(void)
+{
+	return fsmonitor_ipc__is_supported() &&
+	       fsm_settings__get_reason(the_repository) == FSMONITOR_REASON_OK;
+}
+
 static int set_recommended_config(int reconfigure)
 {
 	struct scalar_config config[] = {
@@ -170,6 +179,13 @@ static int set_recommended_config(int reconfigure)
 				     config[i].key, config[i].value);
 	}
 
+	if (have_fsmonitor_support()) {
+		struct scalar_config fsmonitor = { "core.fsmonitor", "true" };
+		if (set_scalar_config(&fsmonitor, reconfigure))
+			return error(_("could not configure %s=%s"),
+				     fsmonitor.key, fsmonitor.value);
+	}
+
 	/*
 	 * The `log.excludeDecoration` setting is special because it allows
 	 * for multiple values.
@@ -218,6 +234,16 @@ static int add_or_remove_enlistment(int add)
 		       "scalar.repo", the_repository->worktree, NULL);
 }
 
+static int start_fsmonitor_daemon(void)
+{
+	assert(have_fsmonitor_support());
+
+	if (fsmonitor_ipc__get_state() != IPC_STATE__LISTENING)
+		return run_git("fsmonitor--daemon", "start", NULL);
+
+	return 0;
+}
+
 static int register_dir(void)
 {
 	if (add_or_remove_enlistment(1))
@@ -229,6 +255,10 @@ static int register_dir(void)
 	if (toggle_maintenance(1))
 		return error(_("could not turn on maintenance"));
 
+	if (have_fsmonitor_support() && start_fsmonitor_daemon()) {
+		return error(_("could not start the FSMonitor daemon"));
+	}
+
 	return 0;
 }
 
diff --git a/contrib/scalar/t/t9099-scalar.sh b/contrib/scalar/t/t9099-scalar.sh
index c069cffebfe..365eab9b54f 100755
--- a/contrib/scalar/t/t9099-scalar.sh
+++ b/contrib/scalar/t/t9099-scalar.sh
@@ -102,6 +102,14 @@ test_expect_success 'scalar enlistments need a worktree' '
 	grep "Scalar enlistments require a worktree" err
 '
 
+test_expect_success FSMONITOR_DAEMON '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_cmp_config -C test/src true core.fsmonitor
+'
+
 test_expect_success 'scalar unregister' '
 	git init vanish/src &&
 	scalar register vanish/src &&
-- 
gitgitgadget

[PATCH v3 7/8] scalar unregister: stop FSMonitor daemon

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

Especially on Windows, we will need to stop that daemon, just in case
that the directory needs to be removed (the daemon would otherwise hold
a handle to that directory, preventing it from being deleted).

Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Victoria Dye <vdye@github.com>
---
 contrib/scalar/scalar.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/contrib/scalar/scalar.c b/contrib/scalar/scalar.c
index 73cd5b1fd0c..07c3f7dd6b6 100644
--- a/contrib/scalar/scalar.c
+++ b/contrib/scalar/scalar.c
@@ -244,6 +244,16 @@ static int start_fsmonitor_daemon(void)
 	return 0;
 }
 
+static int stop_fsmonitor_daemon(void)
+{
+	assert(have_fsmonitor_support());
+
+	if (fsmonitor_ipc__get_state() == IPC_STATE__LISTENING)
+		return run_git("fsmonitor--daemon", "stop", NULL);
+
+	return 0;
+}
+
 static int register_dir(void)
 {
 	if (add_or_remove_enlistment(1))
@@ -468,6 +478,9 @@ static int delete_enlistment(struct strbuf *enlistment)
 	strbuf_release(&parent);
 #endif
 
+	if (have_fsmonitor_support() && stop_fsmonitor_daemon())
+		return error(_("failed to stop the FSMonitor daemon"));
+
 	if (remove_dir_recursively(enlistment, 0))
 		return error(_("failed to delete enlistment directory"));
 
-- 
gitgitgadget

[PATCH v3 8/8] scalar: update technical doc roadmap with FSMonitor support

[Victoria Dye via GitGitGadget]

From: Victoria Dye <vdye@github.com>

Update the Scalar roadmap to reflect completion of enabling the built-in
FSMonitor in Scalar.

Note that implementation of 'scalar help' was moved to the final set of
changes to move Scalar out of 'contrib/'. This is due to a dependency on
changes to 'git help', as all changes to the main Git tree *exclusively*
implemented to support Scalar are part of that series.

Signed-off-by: Victoria Dye <vdye@github.com>
---
 Documentation/technical/scalar.txt | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/Documentation/technical/scalar.txt b/Documentation/technical/scalar.txt
index 08bc09c225a..047390e46eb 100644
--- a/Documentation/technical/scalar.txt
+++ b/Documentation/technical/scalar.txt
@@ -84,13 +84,13 @@ series have been accepted:
 
 - `scalar-diagnose`: The `scalar` command is taught the `diagnose` subcommand.
 
+- 'scalar-add-fsmonitor: Enable the built-in FSMonitor in Scalar
+  enlistments. At the end of this series, Scalar should be feature-complete
+  from the perspective of a user.
+
 Roughly speaking (and subject to change), the following series are needed to
 "finish" this initial version of Scalar:
 
-- Finish Scalar features: Enable the built-in FSMonitor in Scalar enlistments
-  and implement `scalar help`. At the end of this series, Scalar should be
-  feature-complete from the perspective of a user.
-
 - Generalize features not specific to Scalar: In the spirit of making Scalar
   configure only what is needed for large repo performance, move common
   utilities into other parts of Git. Some of this will be internal-only, but one
@@ -98,9 +98,12 @@ Roughly speaking (and subject to change), the following series are needed to
   repository.
 
 - Move Scalar to toplevel: Move Scalar out of `contrib/` and into the root of
-  `git`, including updates to build and install it with the rest of Git. This
-  change will incorporate Scalar into the Git CI and test framework, as well as
-  expand regression and performance testing to ensure the tool is stable.
+  `git`. This includes a variety of related updates, including:
+    - building & installing Scalar in the Git root-level 'make [install]'.
+    - builing & testing Scalar as part of CI.
+    - moving and expanding test coverage of Scalar (including perf tests).
+    - implementing 'scalar help'/'git help scalar' to display scalar
+      documentation.
 
 Finally, there are two additional patch series that exist in Microsoft's fork of
 Git, but there is no current plan to upstream them. There are some interesting
-- 
gitgitgadget

Re: [PATCH v3 1/8] scalar: constrain enlistment search

[Derrick Stolee]

On 8/18/2022 5:40 PM, Victoria Dye via GitGitGadget wrote:
> From: Victoria Dye <vdye@github.com>
> 
> Make the search for repository and enlistment root in
> 'setup_enlistment_directory()' more constrained to simplify behavior and
> adhere to 'GIT_CEILING_DIRECTORIES'.

Thanks for doing this rather substantial rework. The logic makes
sense to me and the tests really help to demonstrate the different
cases.

-Stolee

Re: [PATCH v3 6/8] scalar: enable built-in FSMonitor on `register`

[Derrick Stolee]

On 8/18/2022 5:40 PM, Matthew John Cheetham via GitGitGadget wrote:
> 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).

> +static int have_fsmonitor_support(void)
> +{
> +	return fsmonitor_ipc__is_supported() &&
> +	       fsm_settings__get_reason(the_repository) == FSMONITOR_REASON_OK;
> +}

The only thing I needed to check was that the_repository was initialized
properly as part of 'scalar clone' and it indeed is.

Thanks,
-Stolee

Re: [PATCH v3 0/8] scalar: enable built-in FSMonitor

[Derrick Stolee]

On 8/18/2022 5:40 PM, Victoria Dye via GitGitGadget wrote:
> This series enables the built-in FSMonitor [1] on 'scalar'-registered
> repository enlistments. To avoid errors when unregistering an enlistment,
> the FSMonitor daemon is explicitly stopped during 'scalar unregister'.
> 
> Maintainer's note: this series has a minor conflict with
> 'vd/scalar-generalize-diagnose'. Please let me know if there's anything else
> I can provide (in addition to [2]) that would make resolution easier.
> 
> 
> Changes since V2
> ================
> 
>  * Updated prerequisites for FSMonitor in Scalar to include
>    'fsm_settings__get_reason(the_repository) == FSMONITOR_REASON_OK' to
>    handle cases where the platform is supported, but the repository is not.
>  * Gated enabling the 'core.fsmonitor' on FSMonitor compatibility with the
>    repo.
>  * Replaced 'die()' failures in 'delete_enlistment()' with 'error()'s.
>  * Replaced 'BUILTIN_FSMONITOR' test prerequisite with already-existing
>    'FSMONITOR_DAEMON' for FSMonitor.
>  * Rewrote Scalar enlistment/repo search in 'setup_enlistment_directory()'
>    to avoid unconstrained search and respect 'GIT_CEILING_DIRECTORIES'.
>    Added tests to show the new expected behavior.

I wrote a couple "thinking out loud" replies, but the series looks
good to me without any changes.

Thanks,
-Stolee

Re: [PATCH v3 0/8] scalar: enable built-in FSMonitor

[Junio C Hamano]

Derrick Stolee <derrickstolee@github.com> writes:

> On 8/18/2022 5:40 PM, Victoria Dye via GitGitGadget wrote:
>> This series enables the built-in FSMonitor [1] on 'scalar'-registered
>> repository enlistments. To avoid errors when unregistering an enlistment,
>> the FSMonitor daemon is explicitly stopped during 'scalar unregister'.
>> ...
> I wrote a couple "thinking out loud" replies, but the series looks
> good to me without any changes.

Thanks, both.  Queued.

Let's mark it for 'next' and merge it down soonish.