scalar: enable built-in FSMonitor

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/[email protected]/

[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
@@@ -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

From: Matthew John Cheetham <[email protected]>

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

---
 contrib/scalar/scalar.c          | 39 ++++++++++++++++++++++++++++++++
 contrib/scalar/t/t9099-scalar.sh | 11 +++++++++
 2 files changed, 50 insertions(+)

index 97e71fe19cd..219e414ab4e 100644
@@ -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;
 }
 

index 10b1172a8aa..526f64d001c 100755
@@ -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

From: Johannes Schindelin <[email protected]>

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

---
 contrib/scalar/scalar.c | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

index 219e414ab4e..b774eb044ec 100644
@@ -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

From: Victoria Dye <[email protected]>

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.

---
 Documentation/technical/scalar.txt | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

index 08bc09c225a..047390e46eb 100644
@@ -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

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

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

"Matthew John Cheetham via GitGitGadget" <[email protected]>

	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;

	/* 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(...);

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

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!

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.

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/[email protected]/

[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
@@@ -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 <[email protected]>
          Signed-off-by: Matthew John Cheetham <[email protected]>
          Signed-off-by: Johannes Schindelin <[email protected]>
          Signed-off-by: Victoria Dye <[email protected]>
     @@ 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 <[email protected]>
          Signed-off-by: Johannes Schindelin <[email protected]>
          Signed-off-by: Victoria Dye <[email protected]>
      
       ## 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

From: Victoria Dye <[email protected]>

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.

---
 contrib/scalar/scalar.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

index 97e71fe19cd..e888fa5408e 100644
@@ -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

From: Victoria Dye <[email protected]>

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.

---
 contrib/scalar/scalar.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

index e888fa5408e..6025cd71604 100644
@@ -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

From: Victoria Dye <[email protected]>

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.

---
 Documentation/technical/scalar.txt | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

index 08bc09c225a..047390e46eb 100644
@@ -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

From: Matthew John Cheetham <[email protected]>

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

---
 contrib/scalar/scalar.c          | 21 +++++++++++++++++++++
 contrib/scalar/t/t9099-scalar.sh | 11 +++++++++++
 2 files changed, 32 insertions(+)

index 6025cd71604..28d915ec006 100644
@@ -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;
 }
 

index 10b1172a8aa..526f64d001c 100755
@@ -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

From: Johannes Schindelin <[email protected]>

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

---
 contrib/scalar/scalar.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

index 28d915ec006..f390f519d26 100644
@@ -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

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.

"Matthew John Cheetham via GitGitGadget" <[email protected]>

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

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.

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!

Ah. You're absolutely right about that.

Thanks,
-Stolee

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

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

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.

...

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

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.

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

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/[email protected]/

[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
@@@ -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 <[email protected]>
     +    Helped-by: Derrick Stolee <[email protected]>
          Signed-off-by: Matthew John Cheetham <[email protected]>
          Signed-off-by: Johannes Schindelin <[email protected]>
          Signed-off-by: Victoria Dye <[email protected]>
     @@ 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

From: Victoria Dye <[email protected]>

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.

---
 contrib/scalar/scalar.c          | 82 +++++++++++-------------------
 contrib/scalar/t/t9099-scalar.sh | 85 ++++++++++++++++++++++++++++++++
 2 files changed, 113 insertions(+), 54 deletions(-)

index 97e71fe19cd..92b648f3511 100644
@@ -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);

gitgitgadget

From: Victoria Dye <[email protected]>

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.

---
 contrib/scalar/scalar.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

index 92b648f3511..8ef8dd55041 100644
@@ -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

From: Victoria Dye <[email protected]>

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.

---
 contrib/scalar/scalar.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

index 8ef8dd55041..7be2a938b0c 100644
@@ -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

From: Victoria Dye <[email protected]>

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.

---
 contrib/scalar/scalar.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

index 7be2a938b0c..6de4d5b3721 100644
@@ -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

From: Matthew John Cheetham <[email protected]>

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

---
 contrib/scalar/scalar.c          | 30 ++++++++++++++++++++++++++++++
 contrib/scalar/t/t9099-scalar.sh |  8 ++++++++
 2 files changed, 38 insertions(+)

index 836a4c48fab..73cd5b1fd0c 100644
@@ -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;
 }
 

index c069cffebfe..365eab9b54f 100755
@@ -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

From: Victoria Dye <[email protected]>

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.

---
 Documentation/technical/scalar.txt | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

index 08bc09c225a..047390e46eb 100644
@@ -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

From: Victoria Dye <[email protected]>

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

---
 contrib/scalar/scalar.c | 44 ++++++++++++++++++++++++++---------------
 1 file changed, 28 insertions(+), 16 deletions(-)

index 6de4d5b3721..836a4c48fab 100644
@@ -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

From: Johannes Schindelin <[email protected]>

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

---
 contrib/scalar/scalar.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

index 73cd5b1fd0c..07c3f7dd6b6 100644
@@ -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

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

-Stolee

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

Thanks,
-Stolee

Thanks, both. Queued.

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