{"thread":{"id":"58312","subject":"[PATCH 0/3] scalar: enable built-in FSMonitor","startedAt":"2022-08-16T18:09:01Z","lastAt":"2022-08-19T21:06:56Z","messageCount":39,"participants":["Victoria Dye via GitGitGadget","Matthew John Cheetham via GitGitGadget","Johannes Schindelin via GitGitGadget","Junio C Hamano","Victoria Dye","Derrick Stolee"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"461297","messageId":"pull.1324.git.1660673269.gitgitgadget@gmail.com","threadId":"58312","inReplyTo":null,"subject":"[PATCH 0/3] scalar: enable built-in FSMonitor","fromName":"Victoria Dye via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-08-16T18:07:46Z","receivedAt":"2022-08-16T18:09:01Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"This series enables the built-in FSMonitor [1] on 'scalar'-registered\nrepository enlistments. To avoid errors when unregistering an enlistment,\nthe FSMonitor daemon is explicitly stopped during 'scalar unregister'.\n\nMaintainer's note: this series has a minor conflict with\n'vd/scalar-generalize-diagnose'. Please let me know if there's anything else\nI can provide (in addition to [2]) that would make resolution easier.\n\nThanks\n\n * Victoria\n\n[1]\nhttps://lore.kernel.org/git/pull.1143.git.1644940773.gitgitgadget@gmail.com/\n\n[2] The conflict is a result of both series updating the Scalar roadmap doc.\nFor reference, my merge resolution (from git diff ^1 ^2, where ^1 is\n'vd/scalar-generalize-diagnose' and ^2 is this series) looks\nlike:\n\n------------->8------------->8------------->8------------->8------------->8-------------\ndiff --cc Documentation/technical/scalar.txt\nindex f6353375f0,047390e46e..0600150b3a\n--- a/Documentation/technical/scalar.txt\n+++ b/Documentation/technical/scalar.txt\n@@@ -84,20 -84,26 +84,23 @@@ series have been accepted\n \n - `scalar-diagnose`: The `scalar` command is taught the `diagnose` subcommand.\n \n +- `scalar-generalize-diagnose`: Move the functionality of `scalar diagnose`\n + into `git diagnose` and `git bugreport --diagnose`.\n +\n+ - 'scalar-add-fsmonitor: Enable the built-in FSMonitor in Scalar\n+ enlistments. At the end of this series, Scalar should be feature-complete\n+ from the perspective of a user.\n+ \n Roughly speaking (and subject to change), the following series are needed to\n \"finish\" this initial version of Scalar:\n \n- - Finish Scalar features: Enable the built-in FSMonitor in Scalar enlistments\n- and implement `scalar help`. At the end of this series, Scalar should be\n- feature-complete from the perspective of a user.\n -- Generalize features not specific to Scalar: In the spirit of making Scalar\n - configure only what is needed for large repo performance, move common\n - utilities into other parts of Git. Some of this will be internal-only, but one\n - major change will be generalizing `scalar diagnose` for use with any Git\n - repository.\n--\n - Move Scalar to toplevel: Move Scalar out of `contrib/` and into the root of\n- `git`, including updates to build and install it with the rest of Git. This\n- change will incorporate Scalar into the Git CI and test framework, as well as\n- expand regression and performance testing to ensure the tool is stable.\n+ `git`. This includes a variety of related updates, including:\n+ - building & installing Scalar in the Git root-level 'make [install]'.\n+ - builing & testing Scalar as part of CI.\n+ - moving and expanding test coverage of Scalar (including perf tests).\n+ - implementing 'scalar help'/'git help scalar' to display scalar\n+ documentation.\n \n Finally, there are two additional patch series that exist in Microsoft's fork of\n Git, but there is no current plan to upstream them. There are some interesting\n-------------8<-------------8<-------------8<-------------8<-------------8<---------\n\n\nJohannes Schindelin (1):\n scalar unregister: stop FSMonitor daemon\n\nMatthew John Cheetham (1):\n scalar: enable built-in FSMonitor on `register`\n\nVictoria Dye (1):\n scalar: update technical doc roadmap with FSMonitor support\n\n Documentation/technical/scalar.txt | 17 +++++---\n contrib/scalar/scalar.c | 69 ++++++++++++++++++++++++++++++\n contrib/scalar/t/t9099-scalar.sh | 11 +++++\n 3 files changed, 90 insertions(+), 7 deletions(-)\n\n\nbase-commit: 4af7188bc97f70277d0f10d56d5373022b1fa385\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1324%2Fvdye%2Fscalar%2Fadd-fsmonitor-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1324/vdye/scalar/add-fsmonitor-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1324\n-- \ngitgitgadget\n"},{"id":"461298","messageId":"62682ccf6964d6eebb67491db4a9476dbab56671.1660673269.git.gitgitgadget@gmail.com","threadId":"58312","inReplyTo":"pull.1324.git.1660673269.gitgitgadget@gmail.com","subject":"[PATCH 1/3] scalar: enable built-in FSMonitor on `register`","fromName":"Matthew John Cheetham via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-08-16T18:07:47Z","receivedAt":"2022-08-16T18:09:03Z","isPatch":true,"sender":{"key":"mjcheetham@outlook.com","avatar":"https://avatars.githubusercontent.com/u/5658207?v=4"},"body":"From: Matthew John Cheetham \n\nUsing the built-in FSMonitor makes many common commands quite a bit\nfaster. So let's teach the `scalar register` command to enable the\nbuilt-in FSMonitor and kick-start the fsmonitor--daemon process (for\nconvenience).\n\nFor simplicity, we only support the built-in FSMonitor (and no external\nfile system monitor such as e.g. Watchman).\n\nSigned-off-by: Matthew John Cheetham \nSigned-off-by: Johannes Schindelin \nSigned-off-by: Victoria Dye \n---\n contrib/scalar/scalar.c | 39 ++++++++++++++++++++++++++++++++\n contrib/scalar/t/t9099-scalar.sh | 11 +++++++++\n 2 files changed, 50 insertions(+)\n\ndiff --git a/contrib/scalar/scalar.c b/contrib/scalar/scalar.c\nindex 97e71fe19cd..219e414ab4e 100644\n--- a/contrib/scalar/scalar.c\n+++ b/contrib/scalar/scalar.c\n@@ -7,6 +7,8 @@\n #include \"parse-options.h\"\n #include \"config.h\"\n #include \"run-command.h\"\n+#include \"simple-ipc.h\"\n+#include \"fsmonitor-ipc.h\"\n #include \"refs.h\"\n #include \"dir.h\"\n #include \"packfile.h\"\n@@ -169,6 +171,12 @@ static int set_recommended_config(int reconfigure)\n \t\t{ \"core.autoCRLF\", \"false\" },\n \t\t{ \"core.safeCRLF\", \"false\" },\n \t\t{ \"fetch.showForcedUpdates\", \"false\" },\n+#ifdef HAVE_FSMONITOR_DAEMON_BACKEND\n+\t\t/*\n+\t\t * Enable the built-in FSMonitor on supported platforms.\n+\t\t */\n+\t\t{ \"core.fsmonitor\", \"true\" },\n+#endif\n \t\t{ NULL, NULL },\n \t};\n \tint i;\n@@ -236,6 +244,34 @@ static int add_or_remove_enlistment(int add)\n \t\t \"scalar.repo\", the_repository->worktree, NULL);\n }\n \n+static int start_fsmonitor_daemon(void)\n+{\n+\tint res = 0;\n+\tif (fsmonitor_ipc__is_supported() &&\n+\t fsmonitor_ipc__get_state() != IPC_STATE__LISTENING) {\n+\t\tstruct strbuf err = STRBUF_INIT;\n+\t\tstruct child_process cp = CHILD_PROCESS_INIT;\n+\n+\t\t/* Try to start the FSMonitor daemon */\n+\t\tcp.git_cmd = 1;\n+\t\tstrvec_pushl(&cp.args, \"fsmonitor--daemon\", \"start\", NULL);\n+\t\tif (!pipe_command(&cp, NULL, 0, NULL, 0, &err, 0)) {\n+\t\t\t/* Successfully started FSMonitor */\n+\t\t\tstrbuf_release(&err);\n+\t\t\treturn 0;\n+\t\t}\n+\n+\t\t/* If FSMonitor really hasn't started, emit error */\n+\t\tif (fsmonitor_ipc__get_state() != IPC_STATE__LISTENING)\n+\t\t\tres = error(_(\"could not start the FSMonitor daemon: %s\"),\n+\t\t\t\t err.buf);\n+\n+\t\tstrbuf_release(&err);\n+\t}\n+\n+\treturn res;\n+}\n+\n static int register_dir(void)\n {\n \tint res = add_or_remove_enlistment(1);\n@@ -246,6 +282,9 @@ static int register_dir(void)\n \tif (!res)\n \t\tres = toggle_maintenance(1);\n \n+\tif (!res)\n+\t\tres = start_fsmonitor_daemon();\n+\n \treturn res;\n }\n \ndiff --git a/contrib/scalar/t/t9099-scalar.sh b/contrib/scalar/t/t9099-scalar.sh\nindex 10b1172a8aa..526f64d001c 100755\n--- a/contrib/scalar/t/t9099-scalar.sh\n+++ b/contrib/scalar/t/t9099-scalar.sh\n@@ -13,10 +13,21 @@ PATH=$PWD/..:$PATH\n GIT_TEST_MAINT_SCHEDULER=\"crontab:test-tool crontab ../cron.txt,launchctl:true,schtasks:true\"\n export GIT_TEST_MAINT_SCHEDULER\n \n+test_lazy_prereq BUILTIN_FSMONITOR '\n+\tgit version --build-options | grep -q \"feature:.*fsmonitor--daemon\"\n+'\n+\n test_expect_success 'scalar shows a usage' '\n \ttest_expect_code 129 scalar -h\n '\n \n+test_expect_success BUILTIN_FSMONITOR 'scalar register starts fsmon daemon' '\n+\tgit init test/src &&\n+\ttest_must_fail git -C test/src fsmonitor--daemon status &&\n+\tscalar register test/src &&\n+\tgit -C test/src fsmonitor--daemon status\n+'\n+\n test_expect_success 'scalar unregister' '\n \tgit init vanish/src &&\n \tscalar register vanish/src &&\n-- \ngitgitgadget\n\n"},{"id":"461299","messageId":"78a7f0b1be052bb8c1c2525b3464d7d3ba506bea.1660673269.git.gitgitgadget@gmail.com","threadId":"58312","inReplyTo":"pull.1324.git.1660673269.gitgitgadget@gmail.com","subject":"[PATCH 2/3] scalar unregister: stop FSMonitor daemon","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-08-16T18:07:48Z","receivedAt":"2022-08-16T18:09:06Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nEspecially on Windows, we will need to stop that daemon, just in case\nthat the directory needs to be removed (the daemon would otherwise hold\na handle to that directory, preventing it from being deleted).\n\nSigned-off-by: Johannes Schindelin \nSigned-off-by: Victoria Dye \n---\n contrib/scalar/scalar.c | 30 ++++++++++++++++++++++++++++++\n 1 file changed, 30 insertions(+)\n\ndiff --git a/contrib/scalar/scalar.c b/contrib/scalar/scalar.c\nindex 219e414ab4e..b774eb044ec 100644\n--- a/contrib/scalar/scalar.c\n+++ b/contrib/scalar/scalar.c\n@@ -272,6 +272,33 @@ static int start_fsmonitor_daemon(void)\n \treturn res;\n }\n \n+static int stop_fsmonitor_daemon(void)\n+{\n+\tint res = 0;\n+\tif (fsmonitor_ipc__is_supported()) {\n+\t\tstruct strbuf err = STRBUF_INIT;\n+\t\tstruct child_process cp = CHILD_PROCESS_INIT;\n+\n+\t\t/* Try to stop the FSMonitor daemon */\n+\t\tcp.git_cmd = 1;\n+\t\tstrvec_pushl(&cp.args, \"fsmonitor--daemon\", \"stop\", NULL);\n+\t\tif (!pipe_command(&cp, NULL, 0, NULL, 0, &err, 0)) {\n+\t\t\t/* Successfully stopped FSMonitor */\n+\t\t\tstrbuf_release(&err);\n+\t\t\treturn 0;\n+\t\t}\n+\n+\t\t/* If FSMonitor really hasn't stopped, emit error */\n+\t\tif (fsmonitor_ipc__get_state() == IPC_STATE__LISTENING)\n+\t\t\tres = error(_(\"could not stop the FSMonitor daemon: %s\"),\n+\t\t\t\t err.buf);\n+\n+\t\tstrbuf_release(&err);\n+\t}\n+\n+\treturn res;\n+}\n+\n static int register_dir(void)\n {\n \tint res = add_or_remove_enlistment(1);\n@@ -298,6 +325,9 @@ static int unregister_dir(void)\n \tif (add_or_remove_enlistment(0) < 0)\n \t\tres = -1;\n \n+\tif (stop_fsmonitor_daemon() < 0)\n+\t\tres = -1;\n+\n \treturn res;\n }\n \n-- \ngitgitgadget\n\n"},{"id":"461300","messageId":"5457a8ff1fa0c8591ed1a26da31c0bd99c1bdf44.1660673269.git.gitgitgadget@gmail.com","threadId":"58312","inReplyTo":"pull.1324.git.1660673269.gitgitgadget@gmail.com","subject":"[PATCH 3/3] scalar: update technical doc roadmap with FSMonitor support","fromName":"Victoria Dye via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-08-16T18:07:49Z","receivedAt":"2022-08-16T18:09:10Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"From: Victoria Dye \n\nUpdate the Scalar roadmap to reflect completion of enabling the built-in\nFSMonitor in Scalar.\n\nNote that implementation of 'scalar help' was moved to the final set of\nchanges to move Scalar out of 'contrib/'. This is due to a dependency on\nchanges to 'git help', as all changes to the main Git tree *exclusively*\nimplemented to support Scalar are part of that series.\n\nSigned-off-by: Victoria Dye \n---\n Documentation/technical/scalar.txt | 17 ++++++++++-------\n 1 file changed, 10 insertions(+), 7 deletions(-)\n\ndiff --git a/Documentation/technical/scalar.txt b/Documentation/technical/scalar.txt\nindex 08bc09c225a..047390e46eb 100644\n--- a/Documentation/technical/scalar.txt\n+++ b/Documentation/technical/scalar.txt\n@@ -84,13 +84,13 @@ series have been accepted:\n \n - `scalar-diagnose`: The `scalar` command is taught the `diagnose` subcommand.\n \n+- 'scalar-add-fsmonitor: Enable the built-in FSMonitor in Scalar\n+ enlistments. At the end of this series, Scalar should be feature-complete\n+ from the perspective of a user.\n+\n Roughly speaking (and subject to change), the following series are needed to\n \"finish\" this initial version of Scalar:\n \n-- Finish Scalar features: Enable the built-in FSMonitor in Scalar enlistments\n- and implement `scalar help`. At the end of this series, Scalar should be\n- feature-complete from the perspective of a user.\n-\n - Generalize features not specific to Scalar: In the spirit of making Scalar\n configure only what is needed for large repo performance, move common\n utilities into other parts of Git. Some of this will be internal-only, but one\n@@ -98,9 +98,12 @@ Roughly speaking (and subject to change), the following series are needed to\n repository.\n \n - Move Scalar to toplevel: Move Scalar out of `contrib/` and into the root of\n- `git`, including updates to build and install it with the rest of Git. This\n- change will incorporate Scalar into the Git CI and test framework, as well as\n- expand regression and performance testing to ensure the tool is stable.\n+ `git`. This includes a variety of related updates, including:\n+ - building & installing Scalar in the Git root-level 'make [install]'.\n+ - builing & testing Scalar as part of CI.\n+ - moving and expanding test coverage of Scalar (including perf tests).\n+ - implementing 'scalar help'/'git help scalar' to display scalar\n+ documentation.\n \n Finally, there are two additional patch series that exist in Microsoft's fork of\n Git, but there is no current plan to upstream them. There are some interesting\n-- \ngitgitgadget\n"},{"id":"461301","messageId":"xmqqzgg4uk0q.fsf@gitster.g","threadId":"58312","inReplyTo":"pull.1324.git.1660673269.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/3] scalar: enable built-in FSMonitor","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-08-16T18:21:25Z","receivedAt":"2022-08-16T18:21:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Victoria Dye via GitGitGadget\" writes:\n\n> This series enables the built-in FSMonitor [1] on 'scalar'-registered\n> repository enlistments. To avoid errors when unregistering an enlistment,\n> the FSMonitor daemon is explicitly stopped during 'scalar unregister'.\n>\n> Maintainer's note: this series has a minor conflict with\n> 'vd/scalar-generalize-diagnose'. Please let me know if there's anything else\n> I can provide (in addition to [2]) that would make resolution easier.\n\nThanks. What's the doneness of the other series? It has cooked for\nquite a while and I was wondering if it is ready for 'next' already,\nby the way.\n\n"},{"id":"461302","messageId":"5fb1a4b3-a4d8-25de-ac47-7e47bc604f97@github.com","threadId":"58312","inReplyTo":"xmqqzgg4uk0q.fsf@gitster.g","subject":"Re: [PATCH 0/3] scalar: enable built-in FSMonitor","fromName":"Victoria Dye","fromEmail":"vdye@github.com","sentAt":"2022-08-16T18:42:07Z","receivedAt":"2022-08-16T18:42:12Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"Junio C Hamano wrote:\n> \"Victoria Dye via GitGitGadget\" writes:\n> \n>> This series enables the built-in FSMonitor [1] on 'scalar'-registered\n>> repository enlistments. To avoid errors when unregistering an enlistment,\n>> the FSMonitor daemon is explicitly stopped during 'scalar unregister'.\n>>\n>> Maintainer's note: this series has a minor conflict with\n>> 'vd/scalar-generalize-diagnose'. Please let me know if there's anything else\n>> I can provide (in addition to [2]) that would make resolution easier.\n> \n> Thanks. What's the doneness of the other series? It has cooked for\n> quite a while and I was wondering if it is ready for 'next' already,\n> by the way.\n> \n\nI wasn't planning on making any other changes unless more comments came in;\nI personally think it's ready for 'next', but I'm not sure about reviewers'\nthoughts. There seemed to be some new interest in reviewing at the IRC\nstand-up yesterday [1], but I haven't heard anything since then. \n\nOn the off chance that some major blocking review to\n'vd/scalar-generalize-diagnose' comes in, I didn't want to base this series\non that one. But, if it *is* merged to 'next' before this one, I'll make\nsure to rebase this series on top in subsequent versions to avoid the merge\nconflict. \n\n[1] https://colabti.org/irclogger/irclogger_log/git-devel?date=2022-08-15#l83\n"},{"id":"461303","messageId":"xmqqa684uiy2.fsf@gitster.g","threadId":"58312","inReplyTo":"5fb1a4b3-a4d8-25de-ac47-7e47bc604f97@github.com","subject":"Re: [PATCH 0/3] scalar: enable built-in FSMonitor","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-08-16T18:44:37Z","receivedAt":"2022-08-16T18:44:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Victoria Dye writes:\n\n> On the off chance that some major blocking review to\n> 'vd/scalar-generalize-diagnose' comes in, I didn't want to base this series\n> on that one. But, if it *is* merged to 'next' before this one, I'll make\n> sure to rebase this series on top in subsequent versions to avoid the merge\n> conflict. \n\nAll sensible. I actually am running out of topics to merge to\n'next' ;-) and that was the primary reason why I asked.\n\n"},{"id":"461310","messageId":"xmqq4jybud6h.fsf@gitster.g","threadId":"58312","inReplyTo":"62682ccf6964d6eebb67491db4a9476dbab56671.1660673269.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/3] scalar: enable built-in FSMonitor on `register`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-08-16T20:49:10Z","receivedAt":"2022-08-16T20:49:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Matthew John Cheetham via GitGitGadget\" \nwrites:\n\n> +static int start_fsmonitor_daemon(void)\n> +{\n> +\tint res = 0;\n> +\tif (fsmonitor_ipc__is_supported() &&\n> +\t fsmonitor_ipc__get_state() != IPC_STATE__LISTENING) {\n> +\t\tstruct strbuf err = STRBUF_INIT;\n> +\t\tstruct child_process cp = CHILD_PROCESS_INIT;\n> +\n> +\t\t/* Try to start the FSMonitor daemon */\n> +\t\tcp.git_cmd = 1;\n> +\t\tstrvec_pushl(&cp.args, \"fsmonitor--daemon\", \"start\", NULL);\n> +\t\tif (!pipe_command(&cp, NULL, 0, NULL, 0, &err, 0)) {\n> +\t\t\t/* Successfully started FSMonitor */\n> +\t\t\tstrbuf_release(&err);\n> +\t\t\treturn 0;\n> +\t\t}\n> +\n> +\t\t/* If FSMonitor really hasn't started, emit error */\n> +\t\tif (fsmonitor_ipc__get_state() != IPC_STATE__LISTENING)\n> +\t\t\tres = error(_(\"could not start the FSMonitor daemon: %s\"),\n> +\t\t\t\t err.buf);\n> +\n> +\t\tstrbuf_release(&err);\n> +\t}\n> +\n> +\treturn res;\n> +}\n\nThis somewhat curious code structure made me look, and made me\nnotice that the behaviour is even more curious. Even though\npipe_command() fails, fsmonitor_ipc__get_state() can somehow become\nLISTENING, in which case we are OK? If that is the case, a more natural\nway to write it would be:\n\n\tint res = 0; /* assume success */\n\n\tif (fsmonitor_ipc__is_supported() &&\n fsmonitor_ipc__get_state() != IPC_STATE__LISTENING) {\n\t\t...\n /* \n * if we fail to start it ourselves, and there is no\n * daemon listening to us, it is an error.\n */\n\t\tif (pipe_command(...) &&\n\t\t fsmonitor_ipc__get_state() != IPC_STATE__LISTENING)\n\t\t\tres = error(...);\n\t\tstrbuf_release(&err);\n\t}\n\treturn res;\n\nand that would utilize \"res\" consistently throughout the function.\n\nNote that (I omitted unnecessary blank lines and added necessary\nones in the above outline of the rewrite.\n\nStopping, stepping back a bit and rethinking, the above is not still\nexactly right. If pipe_command() could lie and say \"we failed to\nstart\" when we immediately after the failure can find a running\ndaemon, what guarantees us that pipe_command() does not lie in the\nother direction? So, in that sense, perhaps doing\n\n\t/* we do not care if pipe_command() succeeds or not */\n\t(void) pipe_command(...);\n\n /*\n * we check ourselves if we do have a usable daemon \n * and that is the authoritative answer. we were asked\n * to ensure that usable daemon exists, and we answer\n * if we do or don't.\n */\n\tif (fsmonitor_ipc__get_state() != IPC_STATE__LISTENING)\n\t\tres = error(...);\n\nmay be more true to the spirit of the code.\n\nIt also is slightly curious if the caller wants to see \"success\"\nwhen fsmonitor is not supported. I would have expected the caller\nto check and refrain from calling start/stop when it is not\nsupported (and if there is an end-user interface to force the scalar\ncommand to \"start\", complain by saying \"not supported here\"). But\nas long as we are consistent, I guess it is OK.\n\nThe side that stops shares exactly the same two pieces of\n\"curiosity\" and may need to be updated exactly the same way. It\nassumes that pipe_command() is unreliable and instead of reporting a\npossible failure, we sweep that under the rug, with a questionable\n\"we do not care about pipe failing, as long as the daemon is\nlistening, we do not care\" attitude. And the caller does not care\n\"start\" not stopping where it is not supported.\n\nThanks.\n"},{"id":"461313","messageId":"f766b31f-2f0c-316a-a445-407b8c5baddc@github.com","threadId":"58312","inReplyTo":"xmqq4jybud6h.fsf@gitster.g","subject":"Re: [PATCH 1/3] scalar: enable built-in FSMonitor on `register`","fromName":"Victoria Dye","fromEmail":"vdye@github.com","sentAt":"2022-08-16T21:57:01Z","receivedAt":"2022-08-16T21:57:10Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"Junio C Hamano wrote:\n> \"Matthew John Cheetham via GitGitGadget\" \n> writes:\n> \n>> +static int start_fsmonitor_daemon(void)\n>> +{\n>> +\tint res = 0;\n>> +\tif (fsmonitor_ipc__is_supported() &&\n>> +\t fsmonitor_ipc__get_state() != IPC_STATE__LISTENING) {\n>> +\t\tstruct strbuf err = STRBUF_INIT;\n>> +\t\tstruct child_process cp = CHILD_PROCESS_INIT;\n>> +\n>> +\t\t/* Try to start the FSMonitor daemon */\n>> +\t\tcp.git_cmd = 1;\n>> +\t\tstrvec_pushl(&cp.args, \"fsmonitor--daemon\", \"start\", NULL);\n>> +\t\tif (!pipe_command(&cp, NULL, 0, NULL, 0, &err, 0)) {\n>> +\t\t\t/* Successfully started FSMonitor */\n>> +\t\t\tstrbuf_release(&err);\n>> +\t\t\treturn 0;\n>> +\t\t}\n>> +\n>> +\t\t/* If FSMonitor really hasn't started, emit error */\n>> +\t\tif (fsmonitor_ipc__get_state() != IPC_STATE__LISTENING)\n>> +\t\t\tres = error(_(\"could not start the FSMonitor daemon: %s\"),\n>> +\t\t\t\t err.buf);\n>> +\n>> +\t\tstrbuf_release(&err);\n>> +\t}\n>> +\n>> +\treturn res;\n>> +}\n> \n> This somewhat curious code structure made me look, and made me\n> notice that the behaviour is even more curious. Even though\n> pipe_command() fails, fsmonitor_ipc__get_state() can somehow become\n> LISTENING, in which case we are OK? If that is the case, a more natural\n> way to write it would be:\n> \n> \tint res = 0; /* assume success */\n> \n> \tif (fsmonitor_ipc__is_supported() &&\n> fsmonitor_ipc__get_state() != IPC_STATE__LISTENING) {\n> \t\t...\n> /* \n> * if we fail to start it ourselves, and there is no\n> * daemon listening to us, it is an error.\n> */\n> \t\tif (pipe_command(...) &&\n> \t\t fsmonitor_ipc__get_state() != IPC_STATE__LISTENING)\n> \t\t\tres = error(...);\n> \t\tstrbuf_release(&err);\n> \t}\n> \treturn res;\n> \n> and that would utilize \"res\" consistently throughout the function.\n> \n> Note that (I omitted unnecessary blank lines and added necessary\n> ones in the above outline of the rewrite.\n> \n> Stopping, stepping back a bit and rethinking, the above is not still\n> exactly right. If pipe_command() could lie and say \"we failed to\n> start\" when we immediately after the failure can find a running\n> daemon, what guarantees us that pipe_command() does not lie in the\n> other direction? So, in that sense, perhaps doing\n> \n> \t/* we do not care if pipe_command() succeeds or not */\n> \t(void) pipe_command(...);\n> \n> /*\n> * we check ourselves if we do have a usable daemon \n> * and that is the authoritative answer. we were asked\n> * to ensure that usable daemon exists, and we answer\n> * if we do or don't.\n> */\n> \tif (fsmonitor_ipc__get_state() != IPC_STATE__LISTENING)\n> \t\tres = error(...);\n> \n> may be more true to the spirit of the code.\n\nThis is an unintentional artifact of some minor refactoring of the original\nversions in 'microsoft/git'. Previously [1], there was no\n'fsmonitor_ipc__get_state()' check before calling 'git fsmonitor--daemon\nstart', so we'd expect failures whenever FSMonitor was already running. To\navoid making that 'pipe_command()' call when FSMonitor was already running,\nI added an earlier call to 'fsmonitor_ipc__get_state()'. But, because I\ndidn't remove the later check, the code now implies that 'pipe_command()'\nmay give us \"false negatives\" (that is, fail but still manage to start the\nFSMonitor).\n\nI left the extraneous check in to be overly cautious, but realistically I\ndon't actually expect 'git fsmonitor--daemon start' to give us any false\npositives or negatives. The code should reflect that:\n\n\tint res = 0;\n\tif (fsmonitor_ipc__is_supported() &&\n\t fsmonitor_ipc__get_state() != IPC_STATE__LISTENING) {\n\t\tstruct strbuf err = STRBUF_INIT;\n\t\tstruct child_process cp = CHILD_PROCESS_INIT;\n\n\t\t/* Try to start the FSMonitor daemon */\n\t\tcp.git_cmd = 1;\n\t\tstrvec_pushl(&cp.args, \"fsmonitor--daemon\", \"start\", NULL);\n\t\tif (pipe_command(&cp, NULL, 0, NULL, 0, &err, 0))\n\t\t\tres = error(_(\"could not start the FSMonitor daemon: %s\"),\n\t\t\t\t err.buf);\n\n\t\tstrbuf_release(&err);\n\t}\n\n\treturn res;\n\nI'll re-roll with this shortly.\n\n[1] https://github.com/microsoft/git/commit/4f2e092d3c98\n\n> \n> It also is slightly curious if the caller wants to see \"success\"\n> when fsmonitor is not supported. I would have expected the caller\n> to check and refrain from calling start/stop when it is not\n> supported (and if there is an end-user interface to force the scalar\n> command to \"start\", complain by saying \"not supported here\"). But\n> as long as we are consistent, I guess it is OK.\n\nI don't mind moving the 'fsmonitor_ipc__is_supported()' checks into\n'register_dir()' and 'unregister_dir()'; I can see how it makes more sense\nwith the existing function name. \n\nAs a side note, though, while looking at where to move the condition I\nnoticed that 'unregister_dir()' doesn't handle positive, nonzero return\nvalues properly. I'll fix this & move the 'fsmonitor_ipc__is_supported()'\ncheck in the next version. Thanks!\n\n> \n> The side that stops shares exactly the same two pieces of\n> \"curiosity\" and may need to be updated exactly the same way. It\n> assumes that pipe_command() is unreliable and instead of reporting a\n> possible failure, we sweep that under the rug, with a questionable\n> \"we do not care about pipe failing, as long as the daemon is\n> listening, we do not care\" attitude. And the caller does not care\n> \"start\" not stopping where it is not supported.\n> \n> Thanks.\n\n"},{"id":"461315","messageId":"xmqqzgg3sump.fsf@gitster.g","threadId":"58312","inReplyTo":"f766b31f-2f0c-316a-a445-407b8c5baddc@github.com","subject":"Re: [PATCH 1/3] scalar: enable built-in FSMonitor on `register`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-08-16T22:15:10Z","receivedAt":"2022-08-16T22:15:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Victoria Dye writes:\n\n>> \t/* we do not care if pipe_command() succeeds or not */\n>> \t(void) pipe_command(...);\n>> \n>> /*\n>> * we check ourselves if we do have a usable daemon \n>> * and that is the authoritative answer. we were asked\n>> * to ensure that usable daemon exists, and we answer\n>> * if we do or don't.\n>> */\n>> \tif (fsmonitor_ipc__get_state() != IPC_STATE__LISTENING)\n>> \t\tres = error(...);\n>> \n>> may be more true to the spirit of the code.\n>\n> This is an unintentional artifact of some minor refactoring of the original\n> versions in 'microsoft/git'. Previously [1], there was no\n> 'fsmonitor_ipc__get_state()' check before calling 'git fsmonitor--daemon\n> start', so we'd expect failures whenever FSMonitor was already running. To\n> avoid making that 'pipe_command()' call when FSMonitor was already running,\n> I added an earlier call to 'fsmonitor_ipc__get_state()'. But, because I\n> didn't remove the later check, the code now implies that 'pipe_command()'\n> may give us \"false negatives\" (that is, fail but still manage to start the\n> FSMonitor).\n>\n> I left the extraneous check in to be overly cautious, but realistically I\n> don't actually expect 'git fsmonitor--daemon start' to give us any false\n> positives or negatives. The code should reflect that:\n>\n> \tint res = 0;\n> \tif (fsmonitor_ipc__is_supported() &&\n> \t fsmonitor_ipc__get_state() != IPC_STATE__LISTENING) {\n> \t\tstruct strbuf err = STRBUF_INIT;\n> \t\tstruct child_process cp = CHILD_PROCESS_INIT;\n>\n> \t\t/* Try to start the FSMonitor daemon */\n> \t\tcp.git_cmd = 1;\n> \t\tstrvec_pushl(&cp.args, \"fsmonitor--daemon\", \"start\", NULL);\n> \t\tif (pipe_command(&cp, NULL, 0, NULL, 0, &err, 0))\n> \t\t\tres = error(_(\"could not start the FSMonitor daemon: %s\"),\n> \t\t\t\t err.buf);\n>\n> \t\tstrbuf_release(&err);\n> \t}\n>\n> \treturn res;\n>\n> I'll re-roll with this shortly.\n\nOK, that is one valid way to go about it. After I sent my review\ncomments, I however briefly wondered if we might *not* know if we\nare already running one, there is a reliable exclusion mechansim\nto prevent more than one monitor running at the same time, and we\nare running pipe_command(), fully expecting that it may fail when\nthere is already a working one and a call to pipe_command() that\nis not \"checked\" is just being lazy because we can afford to be\nlazy here.\n\nIf that is not what is going on, then the cleaned up version I am\nresponding to does look more straight-forward and easy to\nunderstand. On the other hand, if \"we can start more than we need\nbecause we can rely on the exclusion mechanism\" is what is going on,\nthat is fine as well, but it does need to be documented, preferrably\nas in-code comment.\n\nThanks.\n"},{"id":"461317","messageId":"pull.1324.v2.git.1660694290.gitgitgadget@gmail.com","threadId":"58312","inReplyTo":"pull.1324.git.1660673269.gitgitgadget@gmail.com","subject":"[PATCH v2 0/5] scalar: enable built-in FSMonitor","fromName":"Victoria Dye via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-08-16T23:58:04Z","receivedAt":"2022-08-16T23:58:19Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"This series enables the built-in FSMonitor [1] on 'scalar'-registered\nrepository enlistments. To avoid errors when unregistering an enlistment,\nthe FSMonitor daemon is explicitly stopped during 'scalar unregister'.\n\nMaintainer's note: this series has a minor conflict with\n'vd/scalar-generalize-diagnose'. Please let me know if there's anything else\nI can provide (in addition to [2]) that would make resolution easier.\n\n\nChanges since V1\n================\n\n * Added a patch to fix 'unregister_dir()'s handling of return values > 0\n from 'toggle_maintenance()' and 'add_or_remove_enlistment()'.\n * Added a patch to print error messages in 'register_dir()' and\n 'unregister_dir()' indicating which of their internal steps fail.\n * Moved check of 'fsmonitor_ipc__is_supported()' to '[un]register_dir()' to\n avoid calling '(start|stop)_fsmonitor_daemon()' when the feature is not\n supported. Added assertion of 'fsmonitor_ipc__is_supported()' to\n '(start|stop)_fsmonitor_daemon()' to enforce that they are not called\n when the feature is unavailable.\n * Simplified '(start|stop)_fsmonitor_daemon()' implementation. Now, if\n FSMonitor is already running/stopped (respectively), the function simply\n returns 0; otherwise, it runs 'git fsmonitor--daemon (start|stop)' and\n returns the exit code.\n * Note that the \"could not (start|stop) the FSMonitor daemon: \"\n error messages are no longer printed by\n '(start|stop)_fsmonitor_daemon()'. Instead, \"\" is printed to\n stderr by swapping 'pipe_command()' out for 'run_git()', and\n '[un]register_dir()' prints the \"could not (start|stop) the FSMonitor\n daemon\" message.\n\nThanks\n\n * Victoria\n\n[1]\nhttps://lore.kernel.org/git/pull.1143.git.1644940773.gitgitgadget@gmail.com/\n\n[2] The conflict is a result of both series updating the Scalar roadmap doc.\nFor reference, my merge resolution (from git diff ^1 ^2, where ^1 is\n'vd/scalar-generalize-diagnose' and ^2 is this series) looks\nlike:\n\n------------->8------------->8------------->8------------->8------------->8-------------\ndiff --cc Documentation/technical/scalar.txt\nindex f6353375f0,047390e46e..0600150b3a\n--- a/Documentation/technical/scalar.txt\n+++ b/Documentation/technical/scalar.txt\n@@@ -84,20 -84,26 +84,23 @@@ series have been accepted\n \n - `scalar-diagnose`: The `scalar` command is taught the `diagnose` subcommand.\n \n +- `scalar-generalize-diagnose`: Move the functionality of `scalar diagnose`\n + into `git diagnose` and `git bugreport --diagnose`.\n +\n+ - 'scalar-add-fsmonitor: Enable the built-in FSMonitor in Scalar\n+ enlistments. At the end of this series, Scalar should be feature-complete\n+ from the perspective of a user.\n+ \n Roughly speaking (and subject to change), the following series are needed to\n \"finish\" this initial version of Scalar:\n \n- - Finish Scalar features: Enable the built-in FSMonitor in Scalar enlistments\n- and implement `scalar help`. At the end of this series, Scalar should be\n- feature-complete from the perspective of a user.\n -- Generalize features not specific to Scalar: In the spirit of making Scalar\n - configure only what is needed for large repo performance, move common\n - utilities into other parts of Git. Some of this will be internal-only, but one\n - major change will be generalizing `scalar diagnose` for use with any Git\n - repository.\n--\n - Move Scalar to toplevel: Move Scalar out of `contrib/` and into the root of\n- `git`, including updates to build and install it with the rest of Git. This\n- change will incorporate Scalar into the Git CI and test framework, as well as\n- expand regression and performance testing to ensure the tool is stable.\n+ `git`. This includes a variety of related updates, including:\n+ - building & installing Scalar in the Git root-level 'make [install]'.\n+ - builing & testing Scalar as part of CI.\n+ - moving and expanding test coverage of Scalar (including perf tests).\n+ - implementing 'scalar help'/'git help scalar' to display scalar\n+ documentation.\n \n Finally, there are two additional patch series that exist in Microsoft's fork of\n Git, but there is no current plan to upstream them. There are some interesting\n-------------8<-------------8<-------------8<-------------8<-------------8<---------\n\n\nJohannes Schindelin (1):\n scalar unregister: stop FSMonitor daemon\n\nMatthew John Cheetham (1):\n scalar: enable built-in FSMonitor on `register`\n\nVictoria Dye (3):\n scalar-unregister: handle error codes greater than 0\n scalar-[un]register: clearly indicate source of error\n scalar: update technical doc roadmap with FSMonitor support\n\n Documentation/technical/scalar.txt | 17 +++++----\n contrib/scalar/scalar.c | 55 ++++++++++++++++++++++++------\n contrib/scalar/t/t9099-scalar.sh | 11 ++++++\n 3 files changed, 66 insertions(+), 17 deletions(-)\n\n\nbase-commit: 4af7188bc97f70277d0f10d56d5373022b1fa385\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1324%2Fvdye%2Fscalar%2Fadd-fsmonitor-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1324/vdye/scalar/add-fsmonitor-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1324\n\nRange-diff vs v1:\n\n -: ----------- > 1: 36fc3cb604d scalar-unregister: handle error codes greater than 0\n -: ----------- > 2: 4bacf8bce8a scalar-[un]register: clearly indicate source of error\n 1: 62682ccf696 ! 3: 5fdf8337972 scalar: enable built-in FSMonitor on `register`\n @@ Commit message\n For simplicity, we only support the built-in FSMonitor (and no external\n file system monitor such as e.g. Watchman).\n \n + Helped-by: Junio C Hamano \n Signed-off-by: Matthew John Cheetham \n Signed-off-by: Johannes Schindelin \n Signed-off-by: Victoria Dye \n @@ contrib/scalar/scalar.c: static int add_or_remove_enlistment(int add)\n \n +static int start_fsmonitor_daemon(void)\n +{\n -+\tint res = 0;\n -+\tif (fsmonitor_ipc__is_supported() &&\n -+\t fsmonitor_ipc__get_state() != IPC_STATE__LISTENING) {\n -+\t\tstruct strbuf err = STRBUF_INIT;\n -+\t\tstruct child_process cp = CHILD_PROCESS_INIT;\n ++\tassert(fsmonitor_ipc__is_supported());\n +\n -+\t\t/* Try to start the FSMonitor daemon */\n -+\t\tcp.git_cmd = 1;\n -+\t\tstrvec_pushl(&cp.args, \"fsmonitor--daemon\", \"start\", NULL);\n -+\t\tif (!pipe_command(&cp, NULL, 0, NULL, 0, &err, 0)) {\n -+\t\t\t/* Successfully started FSMonitor */\n -+\t\t\tstrbuf_release(&err);\n -+\t\t\treturn 0;\n -+\t\t}\n ++\tif (fsmonitor_ipc__get_state() != IPC_STATE__LISTENING)\n ++\t\treturn run_git(\"fsmonitor--daemon\", \"start\", NULL);\n +\n -+\t\t/* If FSMonitor really hasn't started, emit error */\n -+\t\tif (fsmonitor_ipc__get_state() != IPC_STATE__LISTENING)\n -+\t\t\tres = error(_(\"could not start the FSMonitor daemon: %s\"),\n -+\t\t\t\t err.buf);\n -+\n -+\t\tstrbuf_release(&err);\n -+\t}\n -+\n -+\treturn res;\n ++\treturn 0;\n +}\n +\n static int register_dir(void)\n {\n - \tint res = add_or_remove_enlistment(1);\n + \tif (add_or_remove_enlistment(1))\n @@ contrib/scalar/scalar.c: static int register_dir(void)\n - \tif (!res)\n - \t\tres = toggle_maintenance(1);\n + \tif (toggle_maintenance(1))\n + \t\treturn error(_(\"could not turn on maintenance\"));\n \n -+\tif (!res)\n -+\t\tres = start_fsmonitor_daemon();\n ++\tif (fsmonitor_ipc__is_supported() && start_fsmonitor_daemon())\n ++\t\treturn error(_(\"could not start the FSMonitor daemon\"));\n +\n - \treturn res;\n + \treturn 0;\n }\n \n \n 2: 78a7f0b1be0 ! 4: fc4aa1fde31 scalar unregister: stop FSMonitor daemon\n @@ Commit message\n that the directory needs to be removed (the daemon would otherwise hold\n a handle to that directory, preventing it from being deleted).\n \n + Helped-by: Junio C Hamano \n Signed-off-by: Johannes Schindelin \n Signed-off-by: Victoria Dye \n \n ## contrib/scalar/scalar.c ##\n @@ contrib/scalar/scalar.c: static int start_fsmonitor_daemon(void)\n - \treturn res;\n + \treturn 0;\n }\n \n +static int stop_fsmonitor_daemon(void)\n +{\n -+\tint res = 0;\n -+\tif (fsmonitor_ipc__is_supported()) {\n -+\t\tstruct strbuf err = STRBUF_INIT;\n -+\t\tstruct child_process cp = CHILD_PROCESS_INIT;\n -+\n -+\t\t/* Try to stop the FSMonitor daemon */\n -+\t\tcp.git_cmd = 1;\n -+\t\tstrvec_pushl(&cp.args, \"fsmonitor--daemon\", \"stop\", NULL);\n -+\t\tif (!pipe_command(&cp, NULL, 0, NULL, 0, &err, 0)) {\n -+\t\t\t/* Successfully stopped FSMonitor */\n -+\t\t\tstrbuf_release(&err);\n -+\t\t\treturn 0;\n -+\t\t}\n -+\n -+\t\t/* If FSMonitor really hasn't stopped, emit error */\n -+\t\tif (fsmonitor_ipc__get_state() == IPC_STATE__LISTENING)\n -+\t\t\tres = error(_(\"could not stop the FSMonitor daemon: %s\"),\n -+\t\t\t\t err.buf);\n ++\tassert(fsmonitor_ipc__is_supported());\n +\n -+\t\tstrbuf_release(&err);\n -+\t}\n ++\tif (fsmonitor_ipc__get_state() == IPC_STATE__LISTENING)\n ++\t\treturn run_git(\"fsmonitor--daemon\", \"stop\", NULL);\n +\n -+\treturn res;\n ++\treturn 0;\n +}\n +\n static int register_dir(void)\n {\n - \tint res = add_or_remove_enlistment(1);\n + \tif (add_or_remove_enlistment(1))\n @@ contrib/scalar/scalar.c: static int unregister_dir(void)\n - \tif (add_or_remove_enlistment(0) < 0)\n - \t\tres = -1;\n + \tif (add_or_remove_enlistment(0))\n + \t\tres = error(_(\"could not remove enlistment\"));\n \n -+\tif (stop_fsmonitor_daemon() < 0)\n -+\t\tres = -1;\n ++\tif (fsmonitor_ipc__is_supported() && stop_fsmonitor_daemon() < 0)\n ++\t\tres = error(_(\"could not stop the FSMonitor daemon\"));\n +\n \treturn res;\n }\n 3: 5457a8ff1fa = 5: dd59caa2e5a scalar: update technical doc roadmap with FSMonitor support\n\n-- \ngitgitgadget\n"},{"id":"461318","messageId":"36fc3cb604d835f06bd5eca22b6eeff73e7117c8.1660694290.git.gitgitgadget@gmail.com","threadId":"58312","inReplyTo":"pull.1324.v2.git.1660694290.gitgitgadget@gmail.com","subject":"[PATCH v2 1/5] scalar-unregister: handle error codes greater than 0","fromName":"Victoria Dye via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-08-16T23:58:05Z","receivedAt":"2022-08-16T23:58:22Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"From: Victoria Dye \n\nWhen 'scalar unregister' tries to disable maintenance and remove an\nenlistment, ensure that the return value is nonzero if either operation\nproduces *any* nonzero return value, not just when they return a value less\nthan 0.\n\nSigned-off-by: Victoria Dye \n---\n contrib/scalar/scalar.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/contrib/scalar/scalar.c b/contrib/scalar/scalar.c\nindex 97e71fe19cd..e888fa5408e 100644\n--- a/contrib/scalar/scalar.c\n+++ b/contrib/scalar/scalar.c\n@@ -253,10 +253,10 @@ static int unregister_dir(void)\n {\n \tint res = 0;\n \n-\tif (toggle_maintenance(0) < 0)\n+\tif (toggle_maintenance(0))\n \t\tres = -1;\n \n-\tif (add_or_remove_enlistment(0) < 0)\n+\tif (add_or_remove_enlistment(0))\n \t\tres = -1;\n \n \treturn res;\n-- \ngitgitgadget\n\n"},{"id":"461319","messageId":"4bacf8bce8acb1a21bad0648055cd480c81c6b07.1660694290.git.gitgitgadget@gmail.com","threadId":"58312","inReplyTo":"pull.1324.v2.git.1660694290.gitgitgadget@gmail.com","subject":"[PATCH v2 2/5] scalar-[un]register: clearly indicate source of error","fromName":"Victoria Dye via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-08-16T23:58:06Z","receivedAt":"2022-08-16T23:58:22Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"From: Victoria Dye \n\nWhen a step in 'register_dir()' or 'unregister_dir()' fails, indicate which\nstep failed with an error message, rather than silently assigning a nonzero\nreturn code.\n\nSigned-off-by: Victoria Dye \n---\n contrib/scalar/scalar.c | 17 +++++++++--------\n 1 file changed, 9 insertions(+), 8 deletions(-)\n\ndiff --git a/contrib/scalar/scalar.c b/contrib/scalar/scalar.c\nindex e888fa5408e..6025cd71604 100644\n--- a/contrib/scalar/scalar.c\n+++ b/contrib/scalar/scalar.c\n@@ -238,15 +238,16 @@ static int add_or_remove_enlistment(int add)\n \n static int register_dir(void)\n {\n-\tint res = add_or_remove_enlistment(1);\n+\tif (add_or_remove_enlistment(1))\n+\t\treturn error(_(\"could not add enlistment\"));\n \n-\tif (!res)\n-\t\tres = set_recommended_config(0);\n+\tif (set_recommended_config(0))\n+\t\treturn error(_(\"could not set recommended config\"));\n \n-\tif (!res)\n-\t\tres = toggle_maintenance(1);\n+\tif (toggle_maintenance(1))\n+\t\treturn error(_(\"could not turn on maintenance\"));\n \n-\treturn res;\n+\treturn 0;\n }\n \n static int unregister_dir(void)\n@@ -254,10 +255,10 @@ static int unregister_dir(void)\n \tint res = 0;\n \n \tif (toggle_maintenance(0))\n-\t\tres = -1;\n+\t\tres = error(_(\"could not turn off maintenance\"));\n \n \tif (add_or_remove_enlistment(0))\n-\t\tres = -1;\n+\t\tres = error(_(\"could not remove enlistment\"));\n \n \treturn res;\n }\n-- \ngitgitgadget\n\n"},{"id":"461320","messageId":"dd59caa2e5aa7e76e75320e0a5d77ae5a408224a.1660694290.git.gitgitgadget@gmail.com","threadId":"58312","inReplyTo":"pull.1324.v2.git.1660694290.gitgitgadget@gmail.com","subject":"[PATCH v2 5/5] scalar: update technical doc roadmap with FSMonitor support","fromName":"Victoria Dye via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-08-16T23:58:09Z","receivedAt":"2022-08-16T23:58:24Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"From: Victoria Dye \n\nUpdate the Scalar roadmap to reflect completion of enabling the built-in\nFSMonitor in Scalar.\n\nNote that implementation of 'scalar help' was moved to the final set of\nchanges to move Scalar out of 'contrib/'. This is due to a dependency on\nchanges to 'git help', as all changes to the main Git tree *exclusively*\nimplemented to support Scalar are part of that series.\n\nSigned-off-by: Victoria Dye \n---\n Documentation/technical/scalar.txt | 17 ++++++++++-------\n 1 file changed, 10 insertions(+), 7 deletions(-)\n\ndiff --git a/Documentation/technical/scalar.txt b/Documentation/technical/scalar.txt\nindex 08bc09c225a..047390e46eb 100644\n--- a/Documentation/technical/scalar.txt\n+++ b/Documentation/technical/scalar.txt\n@@ -84,13 +84,13 @@ series have been accepted:\n \n - `scalar-diagnose`: The `scalar` command is taught the `diagnose` subcommand.\n \n+- 'scalar-add-fsmonitor: Enable the built-in FSMonitor in Scalar\n+ enlistments. At the end of this series, Scalar should be feature-complete\n+ from the perspective of a user.\n+\n Roughly speaking (and subject to change), the following series are needed to\n \"finish\" this initial version of Scalar:\n \n-- Finish Scalar features: Enable the built-in FSMonitor in Scalar enlistments\n- and implement `scalar help`. At the end of this series, Scalar should be\n- feature-complete from the perspective of a user.\n-\n - Generalize features not specific to Scalar: In the spirit of making Scalar\n configure only what is needed for large repo performance, move common\n utilities into other parts of Git. Some of this will be internal-only, but one\n@@ -98,9 +98,12 @@ Roughly speaking (and subject to change), the following series are needed to\n repository.\n \n - Move Scalar to toplevel: Move Scalar out of `contrib/` and into the root of\n- `git`, including updates to build and install it with the rest of Git. This\n- change will incorporate Scalar into the Git CI and test framework, as well as\n- expand regression and performance testing to ensure the tool is stable.\n+ `git`. This includes a variety of related updates, including:\n+ - building & installing Scalar in the Git root-level 'make [install]'.\n+ - builing & testing Scalar as part of CI.\n+ - moving and expanding test coverage of Scalar (including perf tests).\n+ - implementing 'scalar help'/'git help scalar' to display scalar\n+ documentation.\n \n Finally, there are two additional patch series that exist in Microsoft's fork of\n Git, but there is no current plan to upstream them. There are some interesting\n-- \ngitgitgadget\n"},{"id":"461321","messageId":"5fdf8337972d7092aba06a9c750f42cd5868e630.1660694290.git.gitgitgadget@gmail.com","threadId":"58312","inReplyTo":"pull.1324.v2.git.1660694290.gitgitgadget@gmail.com","subject":"[PATCH v2 3/5] scalar: enable built-in FSMonitor on `register`","fromName":"Matthew John Cheetham via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-08-16T23:58:07Z","receivedAt":"2022-08-16T23:58:27Z","isPatch":true,"sender":{"key":"mjcheetham@outlook.com","avatar":"https://avatars.githubusercontent.com/u/5658207?v=4"},"body":"From: Matthew John Cheetham \n\nUsing the built-in FSMonitor makes many common commands quite a bit\nfaster. So let's teach the `scalar register` command to enable the\nbuilt-in FSMonitor and kick-start the fsmonitor--daemon process (for\nconvenience).\n\nFor simplicity, we only support the built-in FSMonitor (and no external\nfile system monitor such as e.g. Watchman).\n\nHelped-by: Junio C Hamano \nSigned-off-by: Matthew John Cheetham \nSigned-off-by: Johannes Schindelin \nSigned-off-by: Victoria Dye \n---\n contrib/scalar/scalar.c | 21 +++++++++++++++++++++\n contrib/scalar/t/t9099-scalar.sh | 11 +++++++++++\n 2 files changed, 32 insertions(+)\n\ndiff --git a/contrib/scalar/scalar.c b/contrib/scalar/scalar.c\nindex 6025cd71604..28d915ec006 100644\n--- a/contrib/scalar/scalar.c\n+++ b/contrib/scalar/scalar.c\n@@ -7,6 +7,8 @@\n #include \"parse-options.h\"\n #include \"config.h\"\n #include \"run-command.h\"\n+#include \"simple-ipc.h\"\n+#include \"fsmonitor-ipc.h\"\n #include \"refs.h\"\n #include \"dir.h\"\n #include \"packfile.h\"\n@@ -169,6 +171,12 @@ static int set_recommended_config(int reconfigure)\n \t\t{ \"core.autoCRLF\", \"false\" },\n \t\t{ \"core.safeCRLF\", \"false\" },\n \t\t{ \"fetch.showForcedUpdates\", \"false\" },\n+#ifdef HAVE_FSMONITOR_DAEMON_BACKEND\n+\t\t/*\n+\t\t * Enable the built-in FSMonitor on supported platforms.\n+\t\t */\n+\t\t{ \"core.fsmonitor\", \"true\" },\n+#endif\n \t\t{ NULL, NULL },\n \t};\n \tint i;\n@@ -236,6 +244,16 @@ static int add_or_remove_enlistment(int add)\n \t\t \"scalar.repo\", the_repository->worktree, NULL);\n }\n \n+static int start_fsmonitor_daemon(void)\n+{\n+\tassert(fsmonitor_ipc__is_supported());\n+\n+\tif (fsmonitor_ipc__get_state() != IPC_STATE__LISTENING)\n+\t\treturn run_git(\"fsmonitor--daemon\", \"start\", NULL);\n+\n+\treturn 0;\n+}\n+\n static int register_dir(void)\n {\n \tif (add_or_remove_enlistment(1))\n@@ -247,6 +265,9 @@ static int register_dir(void)\n \tif (toggle_maintenance(1))\n \t\treturn error(_(\"could not turn on maintenance\"));\n \n+\tif (fsmonitor_ipc__is_supported() && start_fsmonitor_daemon())\n+\t\treturn error(_(\"could not start the FSMonitor daemon\"));\n+\n \treturn 0;\n }\n \ndiff --git a/contrib/scalar/t/t9099-scalar.sh b/contrib/scalar/t/t9099-scalar.sh\nindex 10b1172a8aa..526f64d001c 100755\n--- a/contrib/scalar/t/t9099-scalar.sh\n+++ b/contrib/scalar/t/t9099-scalar.sh\n@@ -13,10 +13,21 @@ PATH=$PWD/..:$PATH\n GIT_TEST_MAINT_SCHEDULER=\"crontab:test-tool crontab ../cron.txt,launchctl:true,schtasks:true\"\n export GIT_TEST_MAINT_SCHEDULER\n \n+test_lazy_prereq BUILTIN_FSMONITOR '\n+\tgit version --build-options | grep -q \"feature:.*fsmonitor--daemon\"\n+'\n+\n test_expect_success 'scalar shows a usage' '\n \ttest_expect_code 129 scalar -h\n '\n \n+test_expect_success BUILTIN_FSMONITOR 'scalar register starts fsmon daemon' '\n+\tgit init test/src &&\n+\ttest_must_fail git -C test/src fsmonitor--daemon status &&\n+\tscalar register test/src &&\n+\tgit -C test/src fsmonitor--daemon status\n+'\n+\n test_expect_success 'scalar unregister' '\n \tgit init vanish/src &&\n \tscalar register vanish/src &&\n-- \ngitgitgadget\n\n"},{"id":"461322","messageId":"fc4aa1fde31fa0726cde2c1d4e41f3f140fff6f6.1660694290.git.gitgitgadget@gmail.com","threadId":"58312","inReplyTo":"pull.1324.v2.git.1660694290.gitgitgadget@gmail.com","subject":"[PATCH v2 4/5] scalar unregister: stop FSMonitor daemon","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-08-16T23:58:08Z","receivedAt":"2022-08-16T23:58:29Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nEspecially on Windows, we will need to stop that daemon, just in case\nthat the directory needs to be removed (the daemon would otherwise hold\na handle to that directory, preventing it from being deleted).\n\nHelped-by: Junio C Hamano \nSigned-off-by: Johannes Schindelin \nSigned-off-by: Victoria Dye \n---\n contrib/scalar/scalar.c | 13 +++++++++++++\n 1 file changed, 13 insertions(+)\n\ndiff --git a/contrib/scalar/scalar.c b/contrib/scalar/scalar.c\nindex 28d915ec006..f390f519d26 100644\n--- a/contrib/scalar/scalar.c\n+++ b/contrib/scalar/scalar.c\n@@ -254,6 +254,16 @@ static int start_fsmonitor_daemon(void)\n \treturn 0;\n }\n \n+static int stop_fsmonitor_daemon(void)\n+{\n+\tassert(fsmonitor_ipc__is_supported());\n+\n+\tif (fsmonitor_ipc__get_state() == IPC_STATE__LISTENING)\n+\t\treturn run_git(\"fsmonitor--daemon\", \"stop\", NULL);\n+\n+\treturn 0;\n+}\n+\n static int register_dir(void)\n {\n \tif (add_or_remove_enlistment(1))\n@@ -281,6 +291,9 @@ static int unregister_dir(void)\n \tif (add_or_remove_enlistment(0))\n \t\tres = error(_(\"could not remove enlistment\"));\n \n+\tif (fsmonitor_ipc__is_supported() && stop_fsmonitor_daemon() < 0)\n+\t\tres = error(_(\"could not stop the FSMonitor daemon\"));\n+\n \treturn res;\n }\n \n-- \ngitgitgadget\n\n"},{"id":"461365","messageId":"xmqq4jyb0wju.fsf@gitster.g","threadId":"58312","inReplyTo":"36fc3cb604d835f06bd5eca22b6eeff73e7117c8.1660694290.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 1/5] scalar-unregister: handle error codes greater than 0","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-08-17T14:33:25Z","receivedAt":"2022-08-17T14:33:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Victoria Dye via GitGitGadget\" writes:\n\n> From: Victoria Dye \n>\n> When 'scalar unregister' tries to disable maintenance and remove an\n> enlistment, ensure that the return value is nonzero if either operation\n> produces *any* nonzero return value, not just when they return a value less\n> than 0.\n\nInteresting. Did this actually cause problems in the wild? Just\nbeing curious.\n\nThe return values from toggle_maintenance() and add_or_remove() are\nwhat scalar.c::run_git() returns, which in turn come from\nrun_command() and eventually come from wait_or_whine(), so it very\nwell can be a positive non-zero value that signals a failure. It is\ngood to be prepared to see not just negative values but also\npositive ones.\n\n> Signed-off-by: Victoria Dye \n> ---\n> contrib/scalar/scalar.c | 4 ++--\n> 1 file changed, 2 insertions(+), 2 deletions(-)\n>\n> diff --git a/contrib/scalar/scalar.c b/contrib/scalar/scalar.c\n> index 97e71fe19cd..e888fa5408e 100644\n> --- a/contrib/scalar/scalar.c\n> +++ b/contrib/scalar/scalar.c\n> @@ -253,10 +253,10 @@ static int unregister_dir(void)\n> {\n> \tint res = 0;\n> \n> -\tif (toggle_maintenance(0) < 0)\n> +\tif (toggle_maintenance(0))\n> \t\tres = -1;\n> \n> -\tif (add_or_remove_enlistment(0) < 0)\n> +\tif (add_or_remove_enlistment(0))\n> \t\tres = -1;\n> \n> \treturn res;\n"},{"id":"461366","messageId":"f5388e4d-7eb7-9333-6a8e-86ce449aced0@github.com","threadId":"58312","inReplyTo":"5fdf8337972d7092aba06a9c750f42cd5868e630.1660694290.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 3/5] scalar: enable built-in FSMonitor on `register`","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-08-17T14:34:13Z","receivedAt":"2022-08-17T14:34:20Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 8/16/2022 7:58 PM, Matthew John Cheetham via GitGitGadget wrote:\n\n> +#ifdef HAVE_FSMONITOR_DAEMON_BACKEND\n> +\t\t/*\n> +\t\t * Enable the built-in FSMonitor on supported platforms.\n> +\t\t */\n> +\t\t{ \"core.fsmonitor\", \"true\" },\n> +#endif\n> +\tif (fsmonitor_ipc__is_supported() && start_fsmonitor_daemon())\n> +\t\treturn error(_(\"could not start the FSMonitor daemon\"));\n> +\n\nI initially worried if fsmonitor_ipc__is_supported() could use some\nrun-time information to detect if FS Monitor is supported (say, existence\nof a network share or something). However, that implementation is\ncurrently defined as a constant depending on\nHAVE_FSMONITOR_DAEMON_BACKEND.\n\nThe reason I was worried is that we could enable core.fsmonitor=true based\non the compile-time macro, but then avoid starting the daemon based on the\nrun-time results. If we get into this state, would the user's 'git status'\ncalls start complaining about the core.fsmonitor=true config because it is\nnot supported?\n\nThe most future-proof thing to do might be to move the config write out of\nthe set_recommended_config() and into start_fsmonitor_daemon(). Perhaps\nrename it to enable_fsmonitor() so it can fail due to writing the config\n_or_ for starting the daemon. The error message would change, then, too.\n\nOr maybe I'm making a mountain out of a mole hill and what exists here is\nperfectly fine.\n\n> +test_lazy_prereq BUILTIN_FSMONITOR '\n> +\tgit version --build-options | grep -q \"feature:.*fsmonitor--daemon\"\n> +'\n\nIt looks like we already have a FSMONITOR_DAEMON prereq in test-lib.sh.\nShould we use that instead?\n\nThanks,\n-Stolee\n"},{"id":"461367","messageId":"2cbbd732-b9e7-fe8b-9c77-f86a856d06c7@github.com","threadId":"58312","inReplyTo":"fc4aa1fde31fa0726cde2c1d4e41f3f140fff6f6.1660694290.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 4/5] scalar unregister: stop FSMonitor daemon","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-08-17T14:39:28Z","receivedAt":"2022-08-17T14:40:58Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 8/16/2022 7:58 PM, Johannes Schindelin via GitGitGadget wrote:\n> From: Johannes Schindelin \n> \n> Especially on Windows, we will need to stop that daemon, just in case\n> that the directory needs to be removed (the daemon would otherwise hold\n> a handle to that directory, preventing it from being deleted).\n\n> +static int stop_fsmonitor_daemon(void)\n> +{\n> +\tassert(fsmonitor_ipc__is_supported());\n> +\n> +\tif (fsmonitor_ipc__get_state() == IPC_STATE__LISTENING)\n> +\t\treturn run_git(\"fsmonitor--daemon\", \"stop\", NULL);\n> +\n> +\treturn 0;\n> +}\n> +\n> static int register_dir(void)\n> {\n> \tif (add_or_remove_enlistment(1))\n> @@ -281,6 +291,9 @@ static int unregister_dir(void)\n> \tif (add_or_remove_enlistment(0))\n> \t\tres = error(_(\"could not remove enlistment\"));\n> \n> +\tif (fsmonitor_ipc__is_supported() && stop_fsmonitor_daemon() < 0)\n> +\t\tres = error(_(\"could not stop the FSMonitor daemon\"));\n> +\n\nOne thing that is interesting about 'scalar unregister' is that it does\nnot change config values. At that point, we don't know which config values\nare valuable to keep or not because the user may have set them before\n'scalar register', or otherwise liked the config options.\n\nHere, the reason to stop the daemon is so we unlock the ability to delete\nthe directory on Windows.\n\nShould this become part of cmd_delete() instead of unregister_dir()? Or,\ndo we think that users would opt to run 'scalar unregister' before trying\nto delete their directory manually?\n\nThanks,\n-Stolee\n"},{"id":"461368","messageId":"xmqqy1vnylpu.fsf@gitster.g","threadId":"58312","inReplyTo":"5fdf8337972d7092aba06a9c750f42cd5868e630.1660694290.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 3/5] scalar: enable built-in FSMonitor on `register`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-08-17T14:43:25Z","receivedAt":"2022-08-17T14:43:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Matthew John Cheetham via GitGitGadget\" \nwrites:\n\n> +static int start_fsmonitor_daemon(void)\n> +{\n> +\tassert(fsmonitor_ipc__is_supported());\n> +\n> +\tif (fsmonitor_ipc__get_state() != IPC_STATE__LISTENING)\n> +\t\treturn run_git(\"fsmonitor--daemon\", \"start\", NULL);\n> +\n> +\treturn 0;\n> +}\n\nThe function got ultra simple ;-).\n\n> @@ -247,6 +265,9 @@ static int register_dir(void)\n> \tif (toggle_maintenance(1))\n> \t\treturn error(_(\"could not turn on maintenance\"));\n> \n> +\tif (fsmonitor_ipc__is_supported() && start_fsmonitor_daemon())\n> +\t\treturn error(_(\"could not start the FSMonitor daemon\"));\n> +\n> \treturn 0;\n> }\n\nAs long as it is done consistently, I do not think it makes a huge\ndifference between the \"call it only when supported\" and \"when asked\nto do what we do not support, silently succeed without doing\nanything\". It however makes the code appear to be more in control\nto do it this way, I think, which is good.\n\n"},{"id":"461369","messageId":"69be513b-b6c3-3a92-6152-fddc835a6723@github.com","threadId":"58312","inReplyTo":"pull.1324.v2.git.1660694290.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 0/5] scalar: enable built-in FSMonitor","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-08-17T14:51:09Z","receivedAt":"2022-08-17T14:51:17Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 8/16/2022 7:58 PM, Victoria Dye via GitGitGadget wrote:\n> This series enables the built-in FSMonitor [1] on 'scalar'-registered\n> repository enlistments. To avoid errors when unregistering an enlistment,\n> the FSMonitor daemon is explicitly stopped during 'scalar unregister'.\n\nI hadn't looked at this code in a while, so I poked around and\nasked some questions that might not even need answering.\n\nOutside of a nit involving a test prereq, this version looks\nfine to me.\n\nThanks,\n-Stolee\n"},{"id":"461370","messageId":"xmqq5yiqzx01.fsf@gitster.g","threadId":"58312","inReplyTo":"f5388e4d-7eb7-9333-6a8e-86ce449aced0@github.com","subject":"Re: [PATCH v2 3/5] scalar: enable built-in FSMonitor on `register`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-08-17T15:54:22Z","receivedAt":"2022-08-17T15:54:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee writes:\n\n> On 8/16/2022 7:58 PM, Matthew John Cheetham via GitGitGadget wrote:\n>\n>> +#ifdef HAVE_FSMONITOR_DAEMON_BACKEND\n>> +\t\t/*\n>> +\t\t * Enable the built-in FSMonitor on supported platforms.\n>> +\t\t */\n>> +\t\t{ \"core.fsmonitor\", \"true\" },\n>> +#endif\n>> +\tif (fsmonitor_ipc__is_supported() && start_fsmonitor_daemon())\n>> +\t\treturn error(_(\"could not start the FSMonitor daemon\"));\n>> +\n>\n> I initially worried if fsmonitor_ipc__is_supported() could use some\n> run-time information to detect if FS Monitor is supported (say, existence\n> of a network share or something). However, that implementation is\n> currently defined as a constant depending on\n> HAVE_FSMONITOR_DAEMON_BACKEND.\n>\n> The reason I was worried is that we could enable core.fsmonitor=true based\n> on the compile-time macro, but then avoid starting the daemon based on the\n> run-time results. If we get into this state, would the user's 'git status'\n> calls start complaining about the core.fsmonitor=true config because it is\n> not supported?\n\nAh, I didn't consider the possibility where the user uses the\nconfiguration to say \"enable it if you are able, but it is OK if you\ncannot\". Whether the \"is supported\" is dynamic or compiled-in, that\nmay be a valid issue to consider. An easy way out may be to declare\nthat the value \"true\" for \"core.fsmonitor\" variable means exactly\nthat, i.e. the user asks to run it, but it is not an error if it\ncannot run.\n\nA variant that may need slightly more work would be to introduce a\nseparate value (perhaps \"when-able\") that means that, while keeping\nthe \"true\" to mean \"run the built-in one, or error out to let me\nknow otherwise\" as before.\n\nThanks.\n"},{"id":"461373","messageId":"6c39fc96-2e88-297e-38df-4bcb88447972@github.com","threadId":"58312","inReplyTo":"2cbbd732-b9e7-fe8b-9c77-f86a856d06c7@github.com","subject":"Re: [PATCH v2 4/5] scalar unregister: stop FSMonitor daemon","fromName":"Victoria Dye","fromEmail":"vdye@github.com","sentAt":"2022-08-17T17:36:27Z","receivedAt":"2022-08-17T17:36:35Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"Derrick Stolee wrote:\n> On 8/16/2022 7:58 PM, Johannes Schindelin via GitGitGadget wrote:\n>> From: Johannes Schindelin \n>>\n>> Especially on Windows, we will need to stop that daemon, just in case\n>> that the directory needs to be removed (the daemon would otherwise hold\n>> a handle to that directory, preventing it from being deleted).\n> \n>> +static int stop_fsmonitor_daemon(void)\n>> +{\n>> +\tassert(fsmonitor_ipc__is_supported());\n>> +\n>> +\tif (fsmonitor_ipc__get_state() == IPC_STATE__LISTENING)\n>> +\t\treturn run_git(\"fsmonitor--daemon\", \"stop\", NULL);\n>> +\n>> +\treturn 0;\n>> +}\n>> +\n>> static int register_dir(void)\n>> {\n>> \tif (add_or_remove_enlistment(1))\n>> @@ -281,6 +291,9 @@ static int unregister_dir(void)\n>> \tif (add_or_remove_enlistment(0))\n>> \t\tres = error(_(\"could not remove enlistment\"));\n>> \n>> +\tif (fsmonitor_ipc__is_supported() && stop_fsmonitor_daemon() < 0)\n>> +\t\tres = error(_(\"could not stop the FSMonitor daemon\"));\n>> +\n> \n> One thing that is interesting about 'scalar unregister' is that it does\n> not change config values. At that point, we don't know which config values\n> are valuable to keep or not because the user may have set them before\n> 'scalar register', or otherwise liked the config options.\n> \n> Here, the reason to stop the daemon is so we unlock the ability to delete\n> the directory on Windows.\n> \n> Should this become part of cmd_delete() instead of unregister_dir()? Or,\n> do we think that users would opt to run 'scalar unregister' before trying\n> to delete their directory manually?\n\nAfter reading this, my first thought was that 'scalar unregister' should\nstill turn off the FSMonitor daemon because, in addition to allowing for\ndirectory deletion in 'scalar delete', it's \"cleaning up\" some\noptionally-enabled behavior associated with Scalar (a la\n'toggle_maintenance(0)'). However, given that 'unregister' doesn't clear\n'core.fsmonitor', it really *isn't* comparable to 'toggle_maintenance(0)'.\n\nSo I think you're right that it should only be associated with enlistment\ndeletion (although I think 'delete_enlistment()' is the place for that -\nright before 'remove_dir_recursively()' - rather than 'cmd_delete()').\n\nThanks!\n\n> \n> Thanks,\n> -Stolee\n\n"},{"id":"461376","messageId":"9012f1f6-eab6-115a-ab23-d51e5dedb358@github.com","threadId":"58312","inReplyTo":"6c39fc96-2e88-297e-38df-4bcb88447972@github.com","subject":"Re: [PATCH v2 4/5] scalar unregister: stop FSMonitor daemon","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-08-17T17:45:05Z","receivedAt":"2022-08-17T17:45:14Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 8/17/2022 1:36 PM, Victoria Dye wrote:\n\n> So I think you're right that it should only be associated with enlistment\n> deletion (although I think 'delete_enlistment()' is the place for that -\n> right before 'remove_dir_recursively()' - rather than 'cmd_delete()').\n\nAh. You're absolutely right about that.\n\nThanks,\n-Stolee\n\n"},{"id":"461392","messageId":"3a238691-c37f-39a1-f4f6-2b8f9b4c4dcb@github.com","threadId":"58312","inReplyTo":"f5388e4d-7eb7-9333-6a8e-86ce449aced0@github.com","subject":"Re: [PATCH v2 3/5] scalar: enable built-in FSMonitor on `register`","fromName":"Victoria Dye","fromEmail":"vdye@github.com","sentAt":"2022-08-17T23:47:13Z","receivedAt":"2022-08-17T23:47:24Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"Derrick Stolee wrote:\n> On 8/16/2022 7:58 PM, Matthew John Cheetham via GitGitGadget wrote:\n> \n>> +#ifdef HAVE_FSMONITOR_DAEMON_BACKEND\n>> +\t\t/*\n>> +\t\t * Enable the built-in FSMonitor on supported platforms.\n>> +\t\t */\n>> +\t\t{ \"core.fsmonitor\", \"true\" },\n>> +#endif\n>> +\tif (fsmonitor_ipc__is_supported() && start_fsmonitor_daemon())\n>> +\t\treturn error(_(\"could not start the FSMonitor daemon\"));\n>> +\n> \n> I initially worried if fsmonitor_ipc__is_supported() could use some\n> run-time information to detect if FS Monitor is supported (say, existence\n> of a network share or something). However, that implementation is\n> currently defined as a constant depending on\n> HAVE_FSMONITOR_DAEMON_BACKEND.\n> \n> The reason I was worried is that we could enable core.fsmonitor=true based\n> on the compile-time macro, but then avoid starting the daemon based on the\n> run-time results. If we get into this state, would the user's 'git status'\n> calls start complaining about the core.fsmonitor=true config because it is\n> not supported?\n> \n> The most future-proof thing to do might be to move the config write out of\n> the set_recommended_config() and into start_fsmonitor_daemon(). Perhaps\n> rename it to enable_fsmonitor() so it can fail due to writing the config\n> _or_ for starting the daemon. The error message would change, then, too.\n\nI spent some time digging into this, and I think gating both the config and\nsubsequent 'git fsmonitor--daemon start' on having platform *and* repository\nsupport is a good idea. I'll update the next version to both set the\n'core.fsmonitor' config and start the daemon only if the built-in FSMonitor\nis fully supported.\n\n(warning: long-winded tangent mostly unrelated to FSMonitor)\n\nIn the process of testing FSMonitor behavior, I think found other issues\nwith Scalar registration. Specifically, the test I wrote attempted to\n'scalar register' a bare repo, since bare directories are incompatible with\nFSMonitor. After seeing that FSMonitor was *not* incompatible with the\nrepository, I found that Scalar was 1) ignoring the bare repository and, as\na result, 2) identifying my Git clone (way above GIT_CEILING_DIRECTORIES) as\nthe \"enlistment root\". I think 1) might be fine as-is - uniformly ignoring\nbare repos seems like a reasonable choice - but 2) seems like more of a\nproblem. \n\nRight now, 'setup_enlistment_directory()' searches for the repo root\nbeginning at directory '', which is either a user-provided path or\ncurrent working directory. It checks whether '' or '/src' is a\nrepo root: if so, it sets the enlistment info; otherwise, it repeats the\nprocess with the parent of '' until the repo root is found. For\nexample, given the following directory structure:\n\nsomedir\n└── enlistment\n ├── src\n │ └── .git\n └── test\n └── data\n\n'scalar register somedir/enlistment/test/data' will search:\n\n * somedir/enlistment/test/data/src\n * somedir/enlistment/test/data\n * somedir/enlistment/test/src\n * somedir/enlistment/test\n * somedir/enlistment/src\n\nThe current usage of GIT_CEILING_DIRECTORIES relies on the fact that, when\ninvoking a normal 'git' command, 'setup_git_directory()' only searches\nupwards from the current working directory to find the repo root; it's a\nclear \"yes\" or \"no\" as to whether that search passes a ceiling directory.\nScalar isn't as clear, since it searches for the repo root both \"downwards\"\ninto '/src' *and* upwards through the parents of ''. It's not\ntotally clear to me what the \"right\" behavior for Scalar is, but my current\nthought is to follow the same rules as 'setup_git_directory()', but for the\n*enlistment* root rather than the repository root. It's more restrictive\nthan GIT_CEILING_DIRECTORIES on a normal git repo, e.g.:\n\n1. 'GIT_CEILING_DIRECTORIES=somedir/enlistment git -C somedir/enlistment/src status' \n is valid.\n2. 'GIT_CEILING_DIRECTORIES=somedir/enlistment scalar register somedir/enlistment/src'\n is not valid.\n\nbut since Scalar works on the entire enlistment (not just the repo inside of\nit), I think it makes sense to prevent it from crossing a ceiling directory\nboundary.\n\nWhat do you think? Hopefully my rambling wasn't too confusing (if it is,\nplease let me know what I can clarify). \n\n> \n> Or maybe I'm making a mountain out of a mole hill and what exists here is\n> perfectly fine.\n> \n>> +test_lazy_prereq BUILTIN_FSMONITOR '\n>> +\tgit version --build-options | grep -q \"feature:.*fsmonitor--daemon\"\n>> +'\n> \n> It looks like we already have a FSMONITOR_DAEMON prereq in test-lib.sh.\n> Should we use that instead?\n\nWorks for me, happy to reuse code wherever possible. :)\n\n> \n> Thanks,\n> -Stolee\n\n"},{"id":"461420","messageId":"82716e5b-3522-68f5-7479-1b39811e0cb2@github.com","threadId":"58312","inReplyTo":"3a238691-c37f-39a1-f4f6-2b8f9b4c4dcb@github.com","subject":"Re: [PATCH v2 3/5] scalar: enable built-in FSMonitor on `register`","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-08-18T13:19:24Z","receivedAt":"2022-08-18T13:19:30Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 8/17/2022 7:47 PM, Victoria Dye wrote:\n\n> (warning: long-winded tangent mostly unrelated to FSMonitor)\n> \n> In the process of testing FSMonitor behavior, I think found other issues\n> with Scalar registration. Specifically, the test I wrote attempted to\n> 'scalar register' a bare repo, since bare directories are incompatible with\n> FSMonitor. After seeing that FSMonitor was *not* incompatible with the\n> repository, I found that Scalar was 1) ignoring the bare repository and, as\n\nThis is interesting, that Scalar doesn't recognize a bare repo. There are\ndefinitely some config settings that it recommends that don't make sense\nin a bare repo, but it's interesting that it completely ignores it. Good\nfind.\n\nI'm not sure there is anything to 'fix' except maybe error out when the\ndiscovered Git repository is bare. Add a warning, at minimum.\n\n> a result, 2) identifying my Git clone (way above GIT_CEILING_DIRECTORIES) as\n> the \"enlistment root\". I think 1) might be fine as-is - uniformly ignoring\n> bare repos seems like a reasonable choice - but 2) seems like more of a\n> problem. \n\n...\n\n> The current usage of GIT_CEILING_DIRECTORIES relies on the fact that, when\n> invoking a normal 'git' command, 'setup_git_directory()' only searches\n> upwards from the current working directory to find the repo root; it's a\n> clear \"yes\" or \"no\" as to whether that search passes a ceiling directory.\n> Scalar isn't as clear, since it searches for the repo root both \"downwards\"\n> into '/src' *and* upwards through the parents of ''. It's not\n> totally clear to me what the \"right\" behavior for Scalar is, but my current\n> thought is to follow the same rules as 'setup_git_directory()', but for the\n> *enlistment* root rather than the repository root. It's more restrictive\n> than GIT_CEILING_DIRECTORIES on a normal git repo, e.g.:\n> \n> 1. 'GIT_CEILING_DIRECTORIES=somedir/enlistment git -C somedir/enlistment/src status' \n> is valid.\n> 2. 'GIT_CEILING_DIRECTORIES=somedir/enlistment scalar register somedir/enlistment/src'\n> is not valid.\n\nThis is interesting, that we can't recognize the ceiling as the root.\n\n> but since Scalar works on the entire enlistment (not just the repo inside of\n> it), I think it makes sense to prevent it from crossing a ceiling directory\n> boundary.\n\nI think the enlistment root was something that was inherited from VFS for\nGit, and we can mostly abandon it. The things we need to do are all based\non the Git repository itself, not the parent. The only thing we need to\nkeep is to allow a user to specify the repo by pointing to the directory\nimmediately above the 'src' directory.\n\n> 'scalar register somedir/enlistment/test/data' will search:\n> \n> * somedir/enlistment/test/data/src\n> * somedir/enlistment/test/data\n> * somedir/enlistment/test/src\n> * somedir/enlistment/test\n> * somedir/enlistment/src\n\nInstead, we could do the following on a specified :\n\n * If /src exists, find the Git directory by finding the first Git\n repository containing /src.\n * Otherwise, find the first Git repository containing .\n\nIs there an easy way to discover a Git repository at a specific directory?\nOr, do we do something simpler, like changing directories then calling\nsetup_git_directory()? I think simplifying the logic that way should\nrespect GIT_CEILING_DIRECTORIES correctly.\n\nThanks,\n-Stolee\n"},{"id":"461472","messageId":"pull.1324.v3.git.1660858853.gitgitgadget@gmail.com","threadId":"58312","inReplyTo":"pull.1324.v2.git.1660694290.gitgitgadget@gmail.com","subject":"[PATCH v3 0/8] scalar: enable built-in FSMonitor","fromName":"Victoria Dye via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-08-18T21:40:45Z","receivedAt":"2022-08-18T21:41:03Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"This series enables the built-in FSMonitor [1] on 'scalar'-registered\nrepository enlistments. To avoid errors when unregistering an enlistment,\nthe FSMonitor daemon is explicitly stopped during 'scalar unregister'.\n\nMaintainer's note: this series has a minor conflict with\n'vd/scalar-generalize-diagnose'. Please let me know if there's anything else\nI can provide (in addition to [2]) that would make resolution easier.\n\n\nChanges since V2\n================\n\n * Updated prerequisites for FSMonitor in Scalar to include\n 'fsm_settings__get_reason(the_repository) == FSMONITOR_REASON_OK' to\n handle cases where the platform is supported, but the repository is not.\n * Gated enabling the 'core.fsmonitor' on FSMonitor compatibility with the\n repo.\n * Replaced 'die()' failures in 'delete_enlistment()' with 'error()'s.\n * Replaced 'BUILTIN_FSMONITOR' test prerequisite with already-existing\n 'FSMONITOR_DAEMON' for FSMonitor.\n * Rewrote Scalar enlistment/repo search in 'setup_enlistment_directory()'\n to avoid unconstrained search and respect 'GIT_CEILING_DIRECTORIES'.\n Added tests to show the new expected behavior.\n\n\nChanges since V1\n================\n\n * Added a patch to fix 'unregister_dir()'s handling of return values > 0\n from 'toggle_maintenance()' and 'add_or_remove_enlistment()'.\n * Added a patch to print error messages in 'register_dir()' and\n 'unregister_dir()' indicating which of their internal steps fail.\n * Moved check of 'fsmonitor_ipc__is_supported()' to '[un]register_dir()' to\n avoid calling '(start|stop)_fsmonitor_daemon()' when the feature is not\n supported. Added assertion of 'fsmonitor_ipc__is_supported()' to\n '(start|stop)_fsmonitor_daemon()' to enforce that they are not called\n when the feature is unavailable.\n * Simplified '(start|stop)_fsmonitor_daemon()' implementation. Now, if\n FSMonitor is already running/stopped (respectively), the function simply\n returns 0; otherwise, it runs 'git fsmonitor--daemon (start|stop)' and\n returns the exit code.\n * Note that the \"could not (start|stop) the FSMonitor daemon: \"\n error messages are no longer printed by\n '(start|stop)_fsmonitor_daemon()'. Instead, \"\" is printed to\n stderr by swapping 'pipe_command()' out for 'run_git()', and\n '[un]register_dir()' prints the \"could not (start|stop) the FSMonitor\n daemon\" message.\n\nThanks\n\n * Victoria\n\n[1]\nhttps://lore.kernel.org/git/pull.1143.git.1644940773.gitgitgadget@gmail.com/\n\n[2] The conflict is a result of both series updating the Scalar roadmap doc.\nFor reference, my merge resolution (from git diff ^1 ^2, where ^1 is\n'vd/scalar-generalize-diagnose' and ^2 is this series) looks\nlike:\n\n------------->8------------->8------------->8------------->8------------->8-------------\ndiff --cc Documentation/technical/scalar.txt\nindex f6353375f0,047390e46e..0600150b3a\n--- a/Documentation/technical/scalar.txt\n+++ b/Documentation/technical/scalar.txt\n@@@ -84,20 -84,26 +84,23 @@@ series have been accepted\n \n - `scalar-diagnose`: The `scalar` command is taught the `diagnose` subcommand.\n \n +- `scalar-generalize-diagnose`: Move the functionality of `scalar diagnose`\n + into `git diagnose` and `git bugreport --diagnose`.\n +\n+ - 'scalar-add-fsmonitor: Enable the built-in FSMonitor in Scalar\n+ enlistments. At the end of this series, Scalar should be feature-complete\n+ from the perspective of a user.\n+ \n Roughly speaking (and subject to change), the following series are needed to\n \"finish\" this initial version of Scalar:\n \n- - Finish Scalar features: Enable the built-in FSMonitor in Scalar enlistments\n- and implement `scalar help`. At the end of this series, Scalar should be\n- feature-complete from the perspective of a user.\n -- Generalize features not specific to Scalar: In the spirit of making Scalar\n - configure only what is needed for large repo performance, move common\n - utilities into other parts of Git. Some of this will be internal-only, but one\n - major change will be generalizing `scalar diagnose` for use with any Git\n - repository.\n--\n - Move Scalar to toplevel: Move Scalar out of `contrib/` and into the root of\n- `git`, including updates to build and install it with the rest of Git. This\n- change will incorporate Scalar into the Git CI and test framework, as well as\n- expand regression and performance testing to ensure the tool is stable.\n+ `git`. This includes a variety of related updates, including:\n+ - building & installing Scalar in the Git root-level 'make [install]'.\n+ - builing & testing Scalar as part of CI.\n+ - moving and expanding test coverage of Scalar (including perf tests).\n+ - implementing 'scalar help'/'git help scalar' to display scalar\n+ documentation.\n \n Finally, there are two additional patch series that exist in Microsoft's fork of\n Git, but there is no current plan to upstream them. There are some interesting\n-------------8<-------------8<-------------8<-------------8<-------------8<---------\n\n\nJohannes Schindelin (1):\n scalar unregister: stop FSMonitor daemon\n\nMatthew John Cheetham (1):\n scalar: enable built-in FSMonitor on `register`\n\nVictoria Dye (6):\n scalar: constrain enlistment search\n scalar-unregister: handle error codes greater than 0\n scalar-[un]register: clearly indicate source of error\n scalar-delete: do not 'die()' in 'delete_enlistment()'\n scalar: move config setting logic into its own function\n scalar: update technical doc roadmap with FSMonitor support\n\n Documentation/technical/scalar.txt | 17 ++-\n contrib/scalar/scalar.c | 201 +++++++++++++++++------------\n contrib/scalar/t/t9099-scalar.sh | 93 +++++++++++++\n 3 files changed, 220 insertions(+), 91 deletions(-)\n\n\nbase-commit: 4af7188bc97f70277d0f10d56d5373022b1fa385\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1324%2Fvdye%2Fscalar%2Fadd-fsmonitor-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1324/vdye/scalar/add-fsmonitor-v3\nPull-Request: https://github.com/gitgitgadget/git/pull/1324\n\nRange-diff vs v2:\n\n -: ----------- > 1: 2f6cad83613 scalar: constrain enlistment search\n 1: 36fc3cb604d = 2: a6bb0113b9c scalar-unregister: handle error codes greater than 0\n 2: 4bacf8bce8a = 3: aea8723e718 scalar-[un]register: clearly indicate source of error\n -: ----------- > 4: aced836aaa3 scalar-delete: do not 'die()' in 'delete_enlistment()'\n -: ----------- > 5: f8471e94e83 scalar: move config setting logic into its own function\n 3: 5fdf8337972 ! 6: fb379fd2097 scalar: enable built-in FSMonitor on `register`\n @@ Commit message\n file system monitor such as e.g. Watchman).\n \n Helped-by: Junio C Hamano \n + Helped-by: Derrick Stolee \n Signed-off-by: Matthew John Cheetham \n Signed-off-by: Johannes Schindelin \n Signed-off-by: Victoria Dye \n @@ contrib/scalar/scalar.c\n #include \"run-command.h\"\n +#include \"simple-ipc.h\"\n +#include \"fsmonitor-ipc.h\"\n ++#include \"fsmonitor-settings.h\"\n #include \"refs.h\"\n #include \"dir.h\"\n #include \"packfile.h\"\n +@@ contrib/scalar/scalar.c: static int set_scalar_config(const struct scalar_config *config, int reconfigure\n + \treturn res;\n + }\n + \n ++static int have_fsmonitor_support(void)\n ++{\n ++\treturn fsmonitor_ipc__is_supported() &&\n ++\t fsm_settings__get_reason(the_repository) == FSMONITOR_REASON_OK;\n ++}\n ++\n + static int set_recommended_config(int reconfigure)\n + {\n + \tstruct scalar_config config[] = {\n @@ contrib/scalar/scalar.c: static int set_recommended_config(int reconfigure)\n - \t\t{ \"core.autoCRLF\", \"false\" },\n - \t\t{ \"core.safeCRLF\", \"false\" },\n - \t\t{ \"fetch.showForcedUpdates\", \"false\" },\n -+#ifdef HAVE_FSMONITOR_DAEMON_BACKEND\n -+\t\t/*\n -+\t\t * Enable the built-in FSMonitor on supported platforms.\n -+\t\t */\n -+\t\t{ \"core.fsmonitor\", \"true\" },\n -+#endif\n - \t\t{ NULL, NULL },\n - \t};\n - \tint i;\n + \t\t\t\t config[i].key, config[i].value);\n + \t}\n + \n ++\tif (have_fsmonitor_support()) {\n ++\t\tstruct scalar_config fsmonitor = { \"core.fsmonitor\", \"true\" };\n ++\t\tif (set_scalar_config(&fsmonitor, reconfigure))\n ++\t\t\treturn error(_(\"could not configure %s=%s\"),\n ++\t\t\t\t fsmonitor.key, fsmonitor.value);\n ++\t}\n ++\n + \t/*\n + \t * The `log.excludeDecoration` setting is special because it allows\n + \t * for multiple values.\n @@ contrib/scalar/scalar.c: static int add_or_remove_enlistment(int add)\n \t\t \"scalar.repo\", the_repository->worktree, NULL);\n }\n \n +static int start_fsmonitor_daemon(void)\n +{\n -+\tassert(fsmonitor_ipc__is_supported());\n ++\tassert(have_fsmonitor_support());\n +\n +\tif (fsmonitor_ipc__get_state() != IPC_STATE__LISTENING)\n +\t\treturn run_git(\"fsmonitor--daemon\", \"start\", NULL);\n @@ contrib/scalar/scalar.c: static int register_dir(void)\n \tif (toggle_maintenance(1))\n \t\treturn error(_(\"could not turn on maintenance\"));\n \n -+\tif (fsmonitor_ipc__is_supported() && start_fsmonitor_daemon())\n ++\tif (have_fsmonitor_support() && start_fsmonitor_daemon()) {\n +\t\treturn error(_(\"could not start the FSMonitor daemon\"));\n ++\t}\n +\n \treturn 0;\n }\n \n \n ## contrib/scalar/t/t9099-scalar.sh ##\n -@@ contrib/scalar/t/t9099-scalar.sh: PATH=$PWD/..:$PATH\n - GIT_TEST_MAINT_SCHEDULER=\"crontab:test-tool crontab ../cron.txt,launchctl:true,schtasks:true\"\n - export GIT_TEST_MAINT_SCHEDULER\n - \n -+test_lazy_prereq BUILTIN_FSMONITOR '\n -+\tgit version --build-options | grep -q \"feature:.*fsmonitor--daemon\"\n -+'\n -+\n - test_expect_success 'scalar shows a usage' '\n - \ttest_expect_code 129 scalar -h\n +@@ contrib/scalar/t/t9099-scalar.sh: test_expect_success 'scalar enlistments need a worktree' '\n + \tgrep \"Scalar enlistments require a worktree\" err\n '\n \n -+test_expect_success BUILTIN_FSMONITOR 'scalar register starts fsmon daemon' '\n ++test_expect_success FSMONITOR_DAEMON 'scalar register starts fsmon daemon' '\n +\tgit init test/src &&\n +\ttest_must_fail git -C test/src fsmonitor--daemon status &&\n +\tscalar register test/src &&\n -+\tgit -C test/src fsmonitor--daemon status\n ++\tgit -C test/src fsmonitor--daemon status &&\n ++\ttest_cmp_config -C test/src true core.fsmonitor\n +'\n +\n test_expect_success 'scalar unregister' '\n 4: fc4aa1fde31 ! 7: bb58a78fdb2 scalar unregister: stop FSMonitor daemon\n @@ contrib/scalar/scalar.c: static int start_fsmonitor_daemon(void)\n \n +static int stop_fsmonitor_daemon(void)\n +{\n -+\tassert(fsmonitor_ipc__is_supported());\n ++\tassert(have_fsmonitor_support());\n +\n +\tif (fsmonitor_ipc__get_state() == IPC_STATE__LISTENING)\n +\t\treturn run_git(\"fsmonitor--daemon\", \"stop\", NULL);\n @@ contrib/scalar/scalar.c: static int start_fsmonitor_daemon(void)\n static int register_dir(void)\n {\n \tif (add_or_remove_enlistment(1))\n -@@ contrib/scalar/scalar.c: static int unregister_dir(void)\n - \tif (add_or_remove_enlistment(0))\n - \t\tres = error(_(\"could not remove enlistment\"));\n +@@ contrib/scalar/scalar.c: static int delete_enlistment(struct strbuf *enlistment)\n + \tstrbuf_release(&parent);\n + #endif\n \n -+\tif (fsmonitor_ipc__is_supported() && stop_fsmonitor_daemon() < 0)\n -+\t\tres = error(_(\"could not stop the FSMonitor daemon\"));\n ++\tif (have_fsmonitor_support() && stop_fsmonitor_daemon())\n ++\t\treturn error(_(\"failed to stop the FSMonitor daemon\"));\n +\n - \treturn res;\n - }\n + \tif (remove_dir_recursively(enlistment, 0))\n + \t\treturn error(_(\"failed to delete enlistment directory\"));\n \n 5: dd59caa2e5a = 8: 7cee014e2d2 scalar: update technical doc roadmap with FSMonitor support\n\n-- \ngitgitgadget\n"},{"id":"461473","messageId":"2f6cad8361324cecdb0ee7b13a96477a8317c358.1660858853.git.gitgitgadget@gmail.com","threadId":"58312","inReplyTo":"pull.1324.v3.git.1660858853.gitgitgadget@gmail.com","subject":"[PATCH v3 1/8] scalar: constrain enlistment search","fromName":"Victoria Dye via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-08-18T21:40:46Z","receivedAt":"2022-08-18T21:41:06Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"From: Victoria Dye \n\nMake the search for repository and enlistment root in\n'setup_enlistment_directory()' more constrained to simplify behavior and\nadhere to 'GIT_CEILING_DIRECTORIES'.\n\nPreviously, 'setup_enlistment_directory()' would check whether the provided\npath (or current working directory) '' or its subdirectory '/src'\nwas a repository root. If not, the process would repeat on the parent of\n'' until the repository was found or it reached the root of the\nfilesystem. This meant that a user could specify a path *anywhere* inside an\nenlistment (including paths not in the repository contained within the\nenlistment) and it would be found.\n\nThe downside to this process is that the search would not account for\n'GIT_CEILING_DIRECTORIES', so the upward search could result in modifying\nrepository contents past 'GIT_CEILING_DIRECTORIES'. Similarly, operations\nlike 'scalar delete' could end up unintentionally deleting the parent of a\nrepo if its root was named 'src'.\n\nTo make this 'setup_enlistment_directory()' both adhere to\n'GIT_CEILING_DIRECTORIES' and avoid unwanted deletions, the search for an\nenlistment directory is simplified to:\n\n- if '/src' is a repository root, '' is the enlistment root\n- if '' is either the repository root or contained within a repository,\n the repository root is the enlistment root\n\nNow, only 'setup_git_directory()' (called by 'setup_enlistment_directory()')\nsearches upwards from the 'scalar' specified path, enforcing\n'GIT_CEILING_DIRECTORIES' in the process. Additionally, 'scalar delete\n/src' will not delete '' (if users would like to delete it, they\ncan still specify the enlistment root with 'scalar delete '). This is\ntrue of any 'scalar' operation; users can invoke 'scalar' on the enlistment\nroot, but paths must otherwise be inside the repository to be valid.\n\nTo help clarify the updated behavior, new tests are added to\n't9099-scalar.sh'.\n\nFinally, this change leaves 'strbuf_parent_directory()' with only a single,\nWIN32-specific caller in 'delete_enlistment()'. Rather than wrap\n'strbuf_parent_directory()' in '#ifdef WIN32' to avoid the \"unused function\"\ncompiler error, move the contents of 'strbuf_parent_directory()' into\n'delete_enlistment()' and remove the function.\n\nHelped-by: Derrick Stolee \nSigned-off-by: Victoria Dye \n---\n contrib/scalar/scalar.c | 82 +++++++++++-------------------\n contrib/scalar/t/t9099-scalar.sh | 85 ++++++++++++++++++++++++++++++++\n 2 files changed, 113 insertions(+), 54 deletions(-)\n\ndiff --git a/contrib/scalar/scalar.c b/contrib/scalar/scalar.c\nindex 97e71fe19cd..92b648f3511 100644\n--- a/contrib/scalar/scalar.c\n+++ b/contrib/scalar/scalar.c\n@@ -14,29 +14,14 @@\n #include \"archive.h\"\n #include \"object-store.h\"\n \n-/*\n- * Remove the deepest subdirectory in the provided path string. Path must not\n- * include a trailing path separator. Returns 1 if parent directory found,\n- * otherwise 0.\n- */\n-static int strbuf_parent_directory(struct strbuf *buf)\n-{\n-\tsize_t len = buf->len;\n-\tsize_t offset = offset_1st_component(buf->buf);\n-\tchar *path_sep = find_last_dir_sep(buf->buf + offset);\n-\tstrbuf_setlen(buf, path_sep ? path_sep - buf->buf : offset);\n-\n-\treturn buf->len < len;\n-}\n-\n static void setup_enlistment_directory(int argc, const char **argv,\n \t\t\t\t const char * const *usagestr,\n \t\t\t\t const struct option *options,\n \t\t\t\t struct strbuf *enlistment_root)\n {\n \tstruct strbuf path = STRBUF_INIT;\n-\tchar *root;\n-\tint enlistment_found = 0;\n+\tint enlistment_is_repo_parent = 0;\n+\tsize_t len;\n \n \tif (startup_info->have_repository)\n \t\tBUG(\"gitdir already set up?!?\");\n@@ -49,51 +34,36 @@ static void setup_enlistment_directory(int argc, const char **argv,\n \t\tstrbuf_add_absolute_path(&path, argv[0]);\n \t\tif (!is_directory(path.buf))\n \t\t\tdie(_(\"'%s' does not exist\"), path.buf);\n+\t\tif (chdir(path.buf) < 0)\n+\t\t\tdie_errno(_(\"could not switch to '%s'\"), path.buf);\n \t} else if (strbuf_getcwd(&path) < 0)\n \t\tdie(_(\"need a working directory\"));\n \n \tstrbuf_trim_trailing_dir_sep(&path);\n-\tdo {\n-\t\tconst size_t len = path.len;\n-\n-\t\t/* check if currently in enlistment root with src/ workdir */\n-\t\tstrbuf_addstr(&path, \"/src\");\n-\t\tif (is_nonbare_repository_dir(&path)) {\n-\t\t\tif (enlistment_root)\n-\t\t\t\tstrbuf_add(enlistment_root, path.buf, len);\n-\n-\t\t\tenlistment_found = 1;\n-\t\t\tbreak;\n-\t\t}\n \n-\t\t/* reset to original path */\n-\t\tstrbuf_setlen(&path, len);\n-\n-\t\t/* check if currently in workdir */\n-\t\tif (is_nonbare_repository_dir(&path)) {\n-\t\t\tif (enlistment_root) {\n-\t\t\t\t/*\n-\t\t\t\t * If the worktree's directory's name is `src`, the enlistment is the\n-\t\t\t\t * parent directory, otherwise it is identical to the worktree.\n-\t\t\t\t */\n-\t\t\t\troot = strip_path_suffix(path.buf, \"src\");\n-\t\t\t\tstrbuf_addstr(enlistment_root, root ? root : path.buf);\n-\t\t\t\tfree(root);\n-\t\t\t}\n+\t/* check if currently in enlistment root with src/ workdir */\n+\tlen = path.len;\n+\tstrbuf_addstr(&path, \"/src\");\n+\tif (is_nonbare_repository_dir(&path)) {\n+\t\tenlistment_is_repo_parent = 1;\n+\t\tif (chdir(path.buf) < 0)\n+\t\t\tdie_errno(_(\"could not switch to '%s'\"), path.buf);\n+\t}\n+\tstrbuf_setlen(&path, len);\n \n-\t\t\tenlistment_found = 1;\n-\t\t\tbreak;\n-\t\t}\n-\t} while (strbuf_parent_directory(&path));\n+\tsetup_git_directory();\n \n-\tif (!enlistment_found)\n-\t\tdie(_(\"could not find enlistment root\"));\n+\tif (!the_repository->worktree)\n+\t\tdie(_(\"Scalar enlistments require a worktree\"));\n \n-\tif (chdir(path.buf) < 0)\n-\t\tdie_errno(_(\"could not switch to '%s'\"), path.buf);\n+\tif (enlistment_root) {\n+\t\tif (enlistment_is_repo_parent)\n+\t\t\tstrbuf_addbuf(enlistment_root, &path);\n+\t\telse\n+\t\t\tstrbuf_addstr(enlistment_root, the_repository->worktree);\n+\t}\n \n \tstrbuf_release(&path);\n-\tsetup_git_directory();\n }\n \n static int run_git(const char *arg, ...)\n@@ -431,6 +401,8 @@ static int delete_enlistment(struct strbuf *enlistment)\n {\n #ifdef WIN32\n \tstruct strbuf parent = STRBUF_INIT;\n+\tsize_t offset;\n+\tchar *path_sep;\n #endif\n \n \tif (unregister_dir())\n@@ -441,8 +413,10 @@ static int delete_enlistment(struct strbuf *enlistment)\n \t * Change the current directory to one outside of the enlistment so\n \t * that we may delete everything underneath it.\n \t */\n-\tstrbuf_addbuf(&parent, enlistment);\n-\tstrbuf_parent_directory(&parent);\n+\toffset = offset_1st_component(enlistment->buf);\n+\tpath_sep = find_last_dir_sep(enlistment->buf + offset);\n+\tstrbuf_add(&parent, enlistment->buf,\n+\t\t path_sep ? path_sep - enlistment->buf : offset);\n \tif (chdir(parent.buf) < 0)\n \t\tdie_errno(_(\"could not switch to '%s'\"), parent.buf);\n \tstrbuf_release(&parent);\ndiff --git a/contrib/scalar/t/t9099-scalar.sh b/contrib/scalar/t/t9099-scalar.sh\nindex 10b1172a8aa..c069cffebfe 100755\n--- a/contrib/scalar/t/t9099-scalar.sh\n+++ b/contrib/scalar/t/t9099-scalar.sh\n@@ -17,6 +17,91 @@ test_expect_success 'scalar shows a usage' '\n \ttest_expect_code 129 scalar -h\n '\n \n+test_expect_success 'scalar invoked on enlistment root' '\n+\ttest_when_finished rm -rf test src deeper &&\n+\n+\tfor enlistment_root in test src deeper/test\n+\tdo\n+\t\tgit init ${enlistment_root}/src &&\n+\n+\t\t# Register\n+\t\tscalar register ${enlistment_root} &&\n+\t\tscalar list >out &&\n+\t\tgrep \"$(pwd)/${enlistment_root}/src\\$\" out &&\n+\n+\t\t# Delete (including enlistment root)\n+\t\tscalar delete $enlistment_root &&\n+\t\ttest_path_is_missing $enlistment_root &&\n+\t\tscalar list >out &&\n+\t\t! grep \"^$(pwd)/${enlistment_root}/src\\$\" out || return 1\n+\tdone\n+'\n+\n+test_expect_success 'scalar invoked on enlistment src repo' '\n+\ttest_when_finished rm -rf test src deeper &&\n+\n+\tfor enlistment_root in test src deeper/test\n+\tdo\n+\t\tgit init ${enlistment_root}/src &&\n+\n+\t\t# Register\n+\t\tscalar register ${enlistment_root}/src &&\n+\t\tscalar list >out &&\n+\t\tgrep \"$(pwd)/${enlistment_root}/src\\$\" out &&\n+\n+\t\t# Delete (will not include enlistment root)\n+\t\tscalar delete ${enlistment_root}/src &&\n+\t\ttest_path_is_dir $enlistment_root &&\n+\t\tscalar list >out &&\n+\t\t! grep \"^$(pwd)/${enlistment_root}/src\\$\" out || return 1\n+\tdone\n+'\n+\n+test_expect_success 'scalar invoked when enlistment root and repo are the same' '\n+\ttest_when_finished rm -rf test src deeper &&\n+\n+\tfor enlistment_root in test src deeper/test\n+\tdo\n+\t\tgit init ${enlistment_root} &&\n+\n+\t\t# Register\n+\t\tscalar register ${enlistment_root} &&\n+\t\tscalar list >out &&\n+\t\tgrep \"$(pwd)/${enlistment_root}\\$\" out &&\n+\n+\t\t# Delete (will not include enlistment root)\n+\t\tscalar delete ${enlistment_root} &&\n+\t\ttest_path_is_missing $enlistment_root &&\n+\t\tscalar list >out &&\n+\t\t! grep \"^$(pwd)/${enlistment_root}\\$\" out &&\n+\n+\t\t# Make sure we did not accidentally delete the trash dir\n+\t\ttest_path_is_dir \"$TRASH_DIRECTORY\" || return 1\n+\tdone\n+'\n+\n+test_expect_success 'scalar repo search respects GIT_CEILING_DIRECTORIES' '\n+\ttest_when_finished rm -rf test &&\n+\n+\tgit init test/src &&\n+\tmkdir -p test/src/deep &&\n+\tGIT_CEILING_DIRECTORIES=\"$(pwd)/test/src\" &&\n+\t! scalar register test/src/deep 2>err &&\n+\tgrep \"not a git repository\" err\n+'\n+\n+test_expect_success 'scalar enlistments need a worktree' '\n+\ttest_when_finished rm -rf bare test &&\n+\n+\tgit init --bare bare/src &&\n+\t! scalar register bare/src 2>err &&\n+\tgrep \"Scalar enlistments require a worktree\" err &&\n+\n+\tgit init test/src &&\n+\t! scalar register test/src/.git 2>err &&\n+\tgrep \"Scalar enlistments require a worktree\" err\n+'\n+\n test_expect_success 'scalar unregister' '\n \tgit init vanish/src &&\n \tscalar register vanish/src &&\n-- \ngitgitgadget\n\n"},{"id":"461474","messageId":"a6bb0113b9c2eaf0cabc4079bd0928e04c07a96c.1660858853.git.gitgitgadget@gmail.com","threadId":"58312","inReplyTo":"pull.1324.v3.git.1660858853.gitgitgadget@gmail.com","subject":"[PATCH v3 2/8] scalar-unregister: handle error codes greater than 0","fromName":"Victoria Dye via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-08-18T21:40:47Z","receivedAt":"2022-08-18T21:41:09Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"From: Victoria Dye \n\nWhen 'scalar unregister' tries to disable maintenance and remove an\nenlistment, ensure that the return value is nonzero if either operation\nproduces *any* nonzero return value, not just when they return a value less\nthan 0.\n\nSigned-off-by: Victoria Dye \n---\n contrib/scalar/scalar.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/contrib/scalar/scalar.c b/contrib/scalar/scalar.c\nindex 92b648f3511..8ef8dd55041 100644\n--- a/contrib/scalar/scalar.c\n+++ b/contrib/scalar/scalar.c\n@@ -223,10 +223,10 @@ static int unregister_dir(void)\n {\n \tint res = 0;\n \n-\tif (toggle_maintenance(0) < 0)\n+\tif (toggle_maintenance(0))\n \t\tres = -1;\n \n-\tif (add_or_remove_enlistment(0) < 0)\n+\tif (add_or_remove_enlistment(0))\n \t\tres = -1;\n \n \treturn res;\n-- \ngitgitgadget\n\n"},{"id":"461475","messageId":"aea8723e7181e99437f880f5d1578078e3fd350c.1660858853.git.gitgitgadget@gmail.com","threadId":"58312","inReplyTo":"pull.1324.v3.git.1660858853.gitgitgadget@gmail.com","subject":"[PATCH v3 3/8] scalar-[un]register: clearly indicate source of error","fromName":"Victoria Dye via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-08-18T21:40:48Z","receivedAt":"2022-08-18T21:41:18Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"From: Victoria Dye \n\nWhen a step in 'register_dir()' or 'unregister_dir()' fails, indicate which\nstep failed with an error message, rather than silently assigning a nonzero\nreturn code.\n\nSigned-off-by: Victoria Dye \n---\n contrib/scalar/scalar.c | 17 +++++++++--------\n 1 file changed, 9 insertions(+), 8 deletions(-)\n\ndiff --git a/contrib/scalar/scalar.c b/contrib/scalar/scalar.c\nindex 8ef8dd55041..7be2a938b0c 100644\n--- a/contrib/scalar/scalar.c\n+++ b/contrib/scalar/scalar.c\n@@ -208,15 +208,16 @@ static int add_or_remove_enlistment(int add)\n \n static int register_dir(void)\n {\n-\tint res = add_or_remove_enlistment(1);\n+\tif (add_or_remove_enlistment(1))\n+\t\treturn error(_(\"could not add enlistment\"));\n \n-\tif (!res)\n-\t\tres = set_recommended_config(0);\n+\tif (set_recommended_config(0))\n+\t\treturn error(_(\"could not set recommended config\"));\n \n-\tif (!res)\n-\t\tres = toggle_maintenance(1);\n+\tif (toggle_maintenance(1))\n+\t\treturn error(_(\"could not turn on maintenance\"));\n \n-\treturn res;\n+\treturn 0;\n }\n \n static int unregister_dir(void)\n@@ -224,10 +225,10 @@ static int unregister_dir(void)\n \tint res = 0;\n \n \tif (toggle_maintenance(0))\n-\t\tres = -1;\n+\t\tres = error(_(\"could not turn off maintenance\"));\n \n \tif (add_or_remove_enlistment(0))\n-\t\tres = -1;\n+\t\tres = error(_(\"could not remove enlistment\"));\n \n \treturn res;\n }\n-- \ngitgitgadget\n\n"},{"id":"461476","messageId":"aced836aaa39c4c619d03510ba2343527e3a24af.1660858853.git.gitgitgadget@gmail.com","threadId":"58312","inReplyTo":"pull.1324.v3.git.1660858853.gitgitgadget@gmail.com","subject":"[PATCH v3 4/8] scalar-delete: do not 'die()' in 'delete_enlistment()'","fromName":"Victoria Dye via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-08-18T21:40:49Z","receivedAt":"2022-08-18T21:41:20Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"From: Victoria Dye \n\nRather than exiting with 'die()' when 'delete_enlistment()' encounters an\nerror, return an error code with the appropriate message. There's no need\nfor an abrupt exit with 'die()' in 'delete_enlistment()' because its only\ncaller ('cmd_delete()') properly cleans up allocated resources and returns\nthe 'delete_enlistment()' return value as its own exit code.\n\nSigned-off-by: Victoria Dye \n---\n contrib/scalar/scalar.c | 11 +++++++----\n 1 file changed, 7 insertions(+), 4 deletions(-)\n\ndiff --git a/contrib/scalar/scalar.c b/contrib/scalar/scalar.c\nindex 7be2a938b0c..6de4d5b3721 100644\n--- a/contrib/scalar/scalar.c\n+++ b/contrib/scalar/scalar.c\n@@ -407,7 +407,7 @@ static int delete_enlistment(struct strbuf *enlistment)\n #endif\n \n \tif (unregister_dir())\n-\t\tdie(_(\"failed to unregister repository\"));\n+\t\treturn error(_(\"failed to unregister repository\"));\n \n #ifdef WIN32\n \t/*\n@@ -418,13 +418,16 @@ static int delete_enlistment(struct strbuf *enlistment)\n \tpath_sep = find_last_dir_sep(enlistment->buf + offset);\n \tstrbuf_add(&parent, enlistment->buf,\n \t\t path_sep ? path_sep - enlistment->buf : offset);\n-\tif (chdir(parent.buf) < 0)\n-\t\tdie_errno(_(\"could not switch to '%s'\"), parent.buf);\n+\tif (chdir(parent.buf) < 0) {\n+\t\tint res = error_errno(_(\"could not switch to '%s'\"), parent.buf);\n+\t\tstrbuf_release(&parent);\n+\t\treturn res;\n+\t}\n \tstrbuf_release(&parent);\n #endif\n \n \tif (remove_dir_recursively(enlistment, 0))\n-\t\tdie(_(\"failed to delete enlistment directory\"));\n+\t\treturn error(_(\"failed to delete enlistment directory\"));\n \n \treturn 0;\n }\n-- \ngitgitgadget\n\n"},{"id":"461477","messageId":"fb379fd2097b1e4b18428d791b9bcd48571d7b73.1660858853.git.gitgitgadget@gmail.com","threadId":"58312","inReplyTo":"pull.1324.v3.git.1660858853.gitgitgadget@gmail.com","subject":"[PATCH v3 6/8] scalar: enable built-in FSMonitor on `register`","fromName":"Matthew John Cheetham via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-08-18T21:40:51Z","receivedAt":"2022-08-18T21:41:22Z","isPatch":true,"sender":{"key":"mjcheetham@outlook.com","avatar":"https://avatars.githubusercontent.com/u/5658207?v=4"},"body":"From: Matthew John Cheetham \n\nUsing the built-in FSMonitor makes many common commands quite a bit\nfaster. So let's teach the `scalar register` command to enable the\nbuilt-in FSMonitor and kick-start the fsmonitor--daemon process (for\nconvenience).\n\nFor simplicity, we only support the built-in FSMonitor (and no external\nfile system monitor such as e.g. Watchman).\n\nHelped-by: Junio C Hamano \nHelped-by: Derrick Stolee \nSigned-off-by: Matthew John Cheetham \nSigned-off-by: Johannes Schindelin \nSigned-off-by: Victoria Dye \n---\n contrib/scalar/scalar.c | 30 ++++++++++++++++++++++++++++++\n contrib/scalar/t/t9099-scalar.sh | 8 ++++++++\n 2 files changed, 38 insertions(+)\n\ndiff --git a/contrib/scalar/scalar.c b/contrib/scalar/scalar.c\nindex 836a4c48fab..73cd5b1fd0c 100644\n--- a/contrib/scalar/scalar.c\n+++ b/contrib/scalar/scalar.c\n@@ -7,6 +7,9 @@\n #include \"parse-options.h\"\n #include \"config.h\"\n #include \"run-command.h\"\n+#include \"simple-ipc.h\"\n+#include \"fsmonitor-ipc.h\"\n+#include \"fsmonitor-settings.h\"\n #include \"refs.h\"\n #include \"dir.h\"\n #include \"packfile.h\"\n@@ -109,6 +112,12 @@ static int set_scalar_config(const struct scalar_config *config, int reconfigure\n \treturn res;\n }\n \n+static int have_fsmonitor_support(void)\n+{\n+\treturn fsmonitor_ipc__is_supported() &&\n+\t fsm_settings__get_reason(the_repository) == FSMONITOR_REASON_OK;\n+}\n+\n static int set_recommended_config(int reconfigure)\n {\n \tstruct scalar_config config[] = {\n@@ -170,6 +179,13 @@ static int set_recommended_config(int reconfigure)\n \t\t\t\t config[i].key, config[i].value);\n \t}\n \n+\tif (have_fsmonitor_support()) {\n+\t\tstruct scalar_config fsmonitor = { \"core.fsmonitor\", \"true\" };\n+\t\tif (set_scalar_config(&fsmonitor, reconfigure))\n+\t\t\treturn error(_(\"could not configure %s=%s\"),\n+\t\t\t\t fsmonitor.key, fsmonitor.value);\n+\t}\n+\n \t/*\n \t * The `log.excludeDecoration` setting is special because it allows\n \t * for multiple values.\n@@ -218,6 +234,16 @@ static int add_or_remove_enlistment(int add)\n \t\t \"scalar.repo\", the_repository->worktree, NULL);\n }\n \n+static int start_fsmonitor_daemon(void)\n+{\n+\tassert(have_fsmonitor_support());\n+\n+\tif (fsmonitor_ipc__get_state() != IPC_STATE__LISTENING)\n+\t\treturn run_git(\"fsmonitor--daemon\", \"start\", NULL);\n+\n+\treturn 0;\n+}\n+\n static int register_dir(void)\n {\n \tif (add_or_remove_enlistment(1))\n@@ -229,6 +255,10 @@ static int register_dir(void)\n \tif (toggle_maintenance(1))\n \t\treturn error(_(\"could not turn on maintenance\"));\n \n+\tif (have_fsmonitor_support() && start_fsmonitor_daemon()) {\n+\t\treturn error(_(\"could not start the FSMonitor daemon\"));\n+\t}\n+\n \treturn 0;\n }\n \ndiff --git a/contrib/scalar/t/t9099-scalar.sh b/contrib/scalar/t/t9099-scalar.sh\nindex c069cffebfe..365eab9b54f 100755\n--- a/contrib/scalar/t/t9099-scalar.sh\n+++ b/contrib/scalar/t/t9099-scalar.sh\n@@ -102,6 +102,14 @@ test_expect_success 'scalar enlistments need a worktree' '\n \tgrep \"Scalar enlistments require a worktree\" err\n '\n \n+test_expect_success FSMONITOR_DAEMON 'scalar register starts fsmon daemon' '\n+\tgit init test/src &&\n+\ttest_must_fail git -C test/src fsmonitor--daemon status &&\n+\tscalar register test/src &&\n+\tgit -C test/src fsmonitor--daemon status &&\n+\ttest_cmp_config -C test/src true core.fsmonitor\n+'\n+\n test_expect_success 'scalar unregister' '\n \tgit init vanish/src &&\n \tscalar register vanish/src &&\n-- \ngitgitgadget\n\n"},{"id":"461478","messageId":"7cee014e2d2b9140a81125928d388af732783143.1660858853.git.gitgitgadget@gmail.com","threadId":"58312","inReplyTo":"pull.1324.v3.git.1660858853.gitgitgadget@gmail.com","subject":"[PATCH v3 8/8] scalar: update technical doc roadmap with FSMonitor support","fromName":"Victoria Dye via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-08-18T21:40:53Z","receivedAt":"2022-08-18T21:41:25Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"From: Victoria Dye \n\nUpdate the Scalar roadmap to reflect completion of enabling the built-in\nFSMonitor in Scalar.\n\nNote that implementation of 'scalar help' was moved to the final set of\nchanges to move Scalar out of 'contrib/'. This is due to a dependency on\nchanges to 'git help', as all changes to the main Git tree *exclusively*\nimplemented to support Scalar are part of that series.\n\nSigned-off-by: Victoria Dye \n---\n Documentation/technical/scalar.txt | 17 ++++++++++-------\n 1 file changed, 10 insertions(+), 7 deletions(-)\n\ndiff --git a/Documentation/technical/scalar.txt b/Documentation/technical/scalar.txt\nindex 08bc09c225a..047390e46eb 100644\n--- a/Documentation/technical/scalar.txt\n+++ b/Documentation/technical/scalar.txt\n@@ -84,13 +84,13 @@ series have been accepted:\n \n - `scalar-diagnose`: The `scalar` command is taught the `diagnose` subcommand.\n \n+- 'scalar-add-fsmonitor: Enable the built-in FSMonitor in Scalar\n+ enlistments. At the end of this series, Scalar should be feature-complete\n+ from the perspective of a user.\n+\n Roughly speaking (and subject to change), the following series are needed to\n \"finish\" this initial version of Scalar:\n \n-- Finish Scalar features: Enable the built-in FSMonitor in Scalar enlistments\n- and implement `scalar help`. At the end of this series, Scalar should be\n- feature-complete from the perspective of a user.\n-\n - Generalize features not specific to Scalar: In the spirit of making Scalar\n configure only what is needed for large repo performance, move common\n utilities into other parts of Git. Some of this will be internal-only, but one\n@@ -98,9 +98,12 @@ Roughly speaking (and subject to change), the following series are needed to\n repository.\n \n - Move Scalar to toplevel: Move Scalar out of `contrib/` and into the root of\n- `git`, including updates to build and install it with the rest of Git. This\n- change will incorporate Scalar into the Git CI and test framework, as well as\n- expand regression and performance testing to ensure the tool is stable.\n+ `git`. This includes a variety of related updates, including:\n+ - building & installing Scalar in the Git root-level 'make [install]'.\n+ - builing & testing Scalar as part of CI.\n+ - moving and expanding test coverage of Scalar (including perf tests).\n+ - implementing 'scalar help'/'git help scalar' to display scalar\n+ documentation.\n \n Finally, there are two additional patch series that exist in Microsoft's fork of\n Git, but there is no current plan to upstream them. There are some interesting\n-- \ngitgitgadget\n"},{"id":"461479","messageId":"f8471e94e830b199a7045a0b2f508cac8a4b559d.1660858853.git.gitgitgadget@gmail.com","threadId":"58312","inReplyTo":"pull.1324.v3.git.1660858853.gitgitgadget@gmail.com","subject":"[PATCH v3 5/8] scalar: move config setting logic into its own function","fromName":"Victoria Dye via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-08-18T21:40:50Z","receivedAt":"2022-08-18T21:41:28Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"From: Victoria Dye \n\nCreate function 'set_scalar_config()' to contain the logic used in setting\nScalar-defined Git config settings, including how to handle reconfiguring &\noverwriting existing values. This function allows future patches to set\nconfig values in parts of 'scalar.c' other than 'set_recommended_config()'.\n\nSigned-off-by: Victoria Dye \n---\n contrib/scalar/scalar.c | 44 ++++++++++++++++++++++++++---------------\n 1 file changed, 28 insertions(+), 16 deletions(-)\n\ndiff --git a/contrib/scalar/scalar.c b/contrib/scalar/scalar.c\nindex 6de4d5b3721..836a4c48fab 100644\n--- a/contrib/scalar/scalar.c\n+++ b/contrib/scalar/scalar.c\n@@ -85,13 +85,33 @@ static int run_git(const char *arg, ...)\n \treturn res;\n }\n \n+struct scalar_config {\n+\tconst char *key;\n+\tconst char *value;\n+\tint overwrite_on_reconfigure;\n+};\n+\n+static int set_scalar_config(const struct scalar_config *config, int reconfigure)\n+{\n+\tchar *value = NULL;\n+\tint res;\n+\n+\tif ((reconfigure && config->overwrite_on_reconfigure) ||\n+\t git_config_get_string(config->key, &value)) {\n+\t\ttrace2_data_string(\"scalar\", the_repository, config->key, \"created\");\n+\t\tres = git_config_set_gently(config->key, config->value);\n+\t} else {\n+\t\ttrace2_data_string(\"scalar\", the_repository, config->key, \"exists\");\n+\t\tres = 0;\n+\t}\n+\n+\tfree(value);\n+\treturn res;\n+}\n+\n static int set_recommended_config(int reconfigure)\n {\n-\tstruct {\n-\t\tconst char *key;\n-\t\tconst char *value;\n-\t\tint overwrite_on_reconfigure;\n-\t} config[] = {\n+\tstruct scalar_config config[] = {\n \t\t/* Required */\n \t\t{ \"am.keepCR\", \"true\", 1 },\n \t\t{ \"core.FSCache\", \"true\", 1 },\n@@ -145,17 +165,9 @@ static int set_recommended_config(int reconfigure)\n \tchar *value;\n \n \tfor (i = 0; config[i].key; i++) {\n-\t\tif ((reconfigure && config[i].overwrite_on_reconfigure) ||\n-\t\t git_config_get_string(config[i].key, &value)) {\n-\t\t\ttrace2_data_string(\"scalar\", the_repository, config[i].key, \"created\");\n-\t\t\tif (git_config_set_gently(config[i].key,\n-\t\t\t\t\t\t config[i].value) < 0)\n-\t\t\t\treturn error(_(\"could not configure %s=%s\"),\n-\t\t\t\t\t config[i].key, config[i].value);\n-\t\t} else {\n-\t\t\ttrace2_data_string(\"scalar\", the_repository, config[i].key, \"exists\");\n-\t\t\tfree(value);\n-\t\t}\n+\t\tif (set_scalar_config(config + i, reconfigure))\n+\t\t\treturn error(_(\"could not configure %s=%s\"),\n+\t\t\t\t config[i].key, config[i].value);\n \t}\n \n \t/*\n-- \ngitgitgadget\n\n"},{"id":"461480","messageId":"bb58a78fdb2d8416ded6b00898d52d5658841197.1660858853.git.gitgitgadget@gmail.com","threadId":"58312","inReplyTo":"pull.1324.v3.git.1660858853.gitgitgadget@gmail.com","subject":"[PATCH v3 7/8] scalar unregister: stop FSMonitor daemon","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-08-18T21:40:52Z","receivedAt":"2022-08-18T21:41:29Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nEspecially on Windows, we will need to stop that daemon, just in case\nthat the directory needs to be removed (the daemon would otherwise hold\na handle to that directory, preventing it from being deleted).\n\nHelped-by: Junio C Hamano \nSigned-off-by: Johannes Schindelin \nSigned-off-by: Victoria Dye \n---\n contrib/scalar/scalar.c | 13 +++++++++++++\n 1 file changed, 13 insertions(+)\n\ndiff --git a/contrib/scalar/scalar.c b/contrib/scalar/scalar.c\nindex 73cd5b1fd0c..07c3f7dd6b6 100644\n--- a/contrib/scalar/scalar.c\n+++ b/contrib/scalar/scalar.c\n@@ -244,6 +244,16 @@ static int start_fsmonitor_daemon(void)\n \treturn 0;\n }\n \n+static int stop_fsmonitor_daemon(void)\n+{\n+\tassert(have_fsmonitor_support());\n+\n+\tif (fsmonitor_ipc__get_state() == IPC_STATE__LISTENING)\n+\t\treturn run_git(\"fsmonitor--daemon\", \"stop\", NULL);\n+\n+\treturn 0;\n+}\n+\n static int register_dir(void)\n {\n \tif (add_or_remove_enlistment(1))\n@@ -468,6 +478,9 @@ static int delete_enlistment(struct strbuf *enlistment)\n \tstrbuf_release(&parent);\n #endif\n \n+\tif (have_fsmonitor_support() && stop_fsmonitor_daemon())\n+\t\treturn error(_(\"failed to stop the FSMonitor daemon\"));\n+\n \tif (remove_dir_recursively(enlistment, 0))\n \t\treturn error(_(\"failed to delete enlistment directory\"));\n \n-- \ngitgitgadget\n\n"},{"id":"461599","messageId":"0f2df5ed-3380-8162-80c9-10d2eb644965@github.com","threadId":"58312","inReplyTo":"2f6cad8361324cecdb0ee7b13a96477a8317c358.1660858853.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 1/8] scalar: constrain enlistment search","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-08-19T18:32:53Z","receivedAt":"2022-08-19T18:32:59Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 8/18/2022 5:40 PM, Victoria Dye via GitGitGadget wrote:\n> From: Victoria Dye \n> \n> Make the search for repository and enlistment root in\n> 'setup_enlistment_directory()' more constrained to simplify behavior and\n> adhere to 'GIT_CEILING_DIRECTORIES'.\n\nThanks for doing this rather substantial rework. The logic makes\nsense to me and the tests really help to demonstrate the different\ncases.\n\n-Stolee\n"},{"id":"461602","messageId":"3120a57c-3600-c583-7dd3-c13e2e4cdc52@github.com","threadId":"58312","inReplyTo":"fb379fd2097b1e4b18428d791b9bcd48571d7b73.1660858853.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 6/8] scalar: enable built-in FSMonitor on `register`","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-08-19T18:44:41Z","receivedAt":"2022-08-19T18:44:46Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 8/18/2022 5:40 PM, Matthew John Cheetham via GitGitGadget wrote:\n> From: Matthew John Cheetham \n> \n> Using the built-in FSMonitor makes many common commands quite a bit\n> faster. So let's teach the `scalar register` command to enable the\n> built-in FSMonitor and kick-start the fsmonitor--daemon process (for\n> convenience).\n\n> +static int have_fsmonitor_support(void)\n> +{\n> +\treturn fsmonitor_ipc__is_supported() &&\n> +\t fsm_settings__get_reason(the_repository) == FSMONITOR_REASON_OK;\n> +}\n\nThe only thing I needed to check was that the_repository was initialized\nproperly as part of 'scalar clone' and it indeed is.\n\nThanks,\n-Stolee\n\n"},{"id":"461603","messageId":"6b1e154d-e90f-aed9-64c4-6e6845abe25c@github.com","threadId":"58312","inReplyTo":"pull.1324.v3.git.1660858853.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 0/8] scalar: enable built-in FSMonitor","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-08-19T18:45:40Z","receivedAt":"2022-08-19T18:45:45Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 8/18/2022 5:40 PM, Victoria Dye via GitGitGadget wrote:\n> This series enables the built-in FSMonitor [1] on 'scalar'-registered\n> repository enlistments. To avoid errors when unregistering an enlistment,\n> the FSMonitor daemon is explicitly stopped during 'scalar unregister'.\n> \n> Maintainer's note: this series has a minor conflict with\n> 'vd/scalar-generalize-diagnose'. Please let me know if there's anything else\n> I can provide (in addition to [2]) that would make resolution easier.\n> \n> \n> Changes since V2\n> ================\n> \n> * Updated prerequisites for FSMonitor in Scalar to include\n> 'fsm_settings__get_reason(the_repository) == FSMONITOR_REASON_OK' to\n> handle cases where the platform is supported, but the repository is not.\n> * Gated enabling the 'core.fsmonitor' on FSMonitor compatibility with the\n> repo.\n> * Replaced 'die()' failures in 'delete_enlistment()' with 'error()'s.\n> * Replaced 'BUILTIN_FSMONITOR' test prerequisite with already-existing\n> 'FSMONITOR_DAEMON' for FSMonitor.\n> * Rewrote Scalar enlistment/repo search in 'setup_enlistment_directory()'\n> to avoid unconstrained search and respect 'GIT_CEILING_DIRECTORIES'.\n> Added tests to show the new expected behavior.\n\nI wrote a couple \"thinking out loud\" replies, but the series looks\ngood to me without any changes.\n\nThanks,\n-Stolee\n\n"},{"id":"461618","messageId":"xmqqy1vkj639.fsf@gitster.g","threadId":"58312","inReplyTo":"6b1e154d-e90f-aed9-64c4-6e6845abe25c@github.com","subject":"Re: [PATCH v3 0/8] scalar: enable built-in FSMonitor","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-08-19T21:06:50Z","receivedAt":"2022-08-19T21:06:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee writes:\n\n> On 8/18/2022 5:40 PM, Victoria Dye via GitGitGadget wrote:\n>> This series enables the built-in FSMonitor [1] on 'scalar'-registered\n>> repository enlistments. To avoid errors when unregistering an enlistment,\n>> the FSMonitor daemon is explicitly stopped during 'scalar unregister'.\n>> ...\n> I wrote a couple \"thinking out loud\" replies, but the series looks\n> good to me without any changes.\n\nThanks, both. Queued.\n\nLet's mark it for 'next' and merge it down soonish.\n"}]}