From 65f6a9eb0b9b38ca10b4e6a2b02671036981195f Mon Sep 17 00:00:00 2001 From: Victoria Dye Date: Thu, 18 Aug 2022 21:40:46 +0000 Subject: [PATCH 1/8] scalar: constrain enlistment search Make the search for repository and enlistment root in 'setup_enlistment_directory()' more constrained to simplify behavior and adhere to 'GIT_CEILING_DIRECTORIES'. Previously, 'setup_enlistment_directory()' would check whether the provided path (or current working directory) '' or its subdirectory '/src' was a repository root. If not, the process would repeat on the parent of '' until the repository was found or it reached the root of the filesystem. This meant that a user could specify a path *anywhere* inside an enlistment (including paths not in the repository contained within the enlistment) and it would be found. The downside to this process is that the search would not account for 'GIT_CEILING_DIRECTORIES', so the upward search could result in modifying repository contents past 'GIT_CEILING_DIRECTORIES'. Similarly, operations like 'scalar delete' could end up unintentionally deleting the parent of a repo if its root was named 'src'. To make this 'setup_enlistment_directory()' both adhere to 'GIT_CEILING_DIRECTORIES' and avoid unwanted deletions, the search for an enlistment directory is simplified to: - if '/src' is a repository root, '' is the enlistment root - if '' is either the repository root or contained within a repository, the repository root is the enlistment root Now, only 'setup_git_directory()' (called by 'setup_enlistment_directory()') searches upwards from the 'scalar' specified path, enforcing 'GIT_CEILING_DIRECTORIES' in the process. Additionally, 'scalar delete /src' will not delete '' (if users would like to delete it, they can still specify the enlistment root with 'scalar delete '). This is true of any 'scalar' operation; users can invoke 'scalar' on the enlistment root, but paths must otherwise be inside the repository to be valid. To help clarify the updated behavior, new tests are added to 't9099-scalar.sh'. Finally, this change leaves 'strbuf_parent_directory()' with only a single, WIN32-specific caller in 'delete_enlistment()'. Rather than wrap 'strbuf_parent_directory()' in '#ifdef WIN32' to avoid the "unused function" compiler error, move the contents of 'strbuf_parent_directory()' into 'delete_enlistment()' and remove the function. Helped-by: Derrick Stolee Signed-off-by: Victoria Dye Signed-off-by: Junio C Hamano --- contrib/scalar/scalar.c | 82 +++++++++++------------------- contrib/scalar/t/t9099-scalar.sh | 85 ++++++++++++++++++++++++++++++++ 2 files changed, 113 insertions(+), 54 deletions(-) diff --git a/contrib/scalar/scalar.c b/contrib/scalar/scalar.c index 97e71fe19c..92b648f351 100644 --- a/contrib/scalar/scalar.c +++ b/contrib/scalar/scalar.c @@ -14,29 +14,14 @@ #include "archive.h" #include "object-store.h" -/* - * Remove the deepest subdirectory in the provided path string. Path must not - * include a trailing path separator. Returns 1 if parent directory found, - * otherwise 0. - */ -static int strbuf_parent_directory(struct strbuf *buf) -{ - size_t len = buf->len; - size_t offset = offset_1st_component(buf->buf); - char *path_sep = find_last_dir_sep(buf->buf + offset); - strbuf_setlen(buf, path_sep ? path_sep - buf->buf : offset); - - return buf->len < len; -} - static void setup_enlistment_directory(int argc, const char **argv, const char * const *usagestr, const struct option *options, struct strbuf *enlistment_root) { struct strbuf path = STRBUF_INIT; - char *root; - int enlistment_found = 0; + int enlistment_is_repo_parent = 0; + size_t len; if (startup_info->have_repository) BUG("gitdir already set up?!?"); @@ -49,51 +34,36 @@ static void setup_enlistment_directory(int argc, const char **argv, strbuf_add_absolute_path(&path, argv[0]); if (!is_directory(path.buf)) die(_("'%s' does not exist"), path.buf); + if (chdir(path.buf) < 0) + die_errno(_("could not switch to '%s'"), path.buf); } else if (strbuf_getcwd(&path) < 0) die(_("need a working directory")); strbuf_trim_trailing_dir_sep(&path); - do { - const size_t len = path.len; - /* check if currently in enlistment root with src/ workdir */ - strbuf_addstr(&path, "/src"); - if (is_nonbare_repository_dir(&path)) { - if (enlistment_root) - strbuf_add(enlistment_root, path.buf, len); + /* check if currently in enlistment root with src/ workdir */ + len = path.len; + strbuf_addstr(&path, "/src"); + if (is_nonbare_repository_dir(&path)) { + enlistment_is_repo_parent = 1; + if (chdir(path.buf) < 0) + die_errno(_("could not switch to '%s'"), path.buf); + } + strbuf_setlen(&path, len); - enlistment_found = 1; - break; - } + setup_git_directory(); - /* reset to original path */ - strbuf_setlen(&path, len); + if (!the_repository->worktree) + die(_("Scalar enlistments require a worktree")); - /* check if currently in workdir */ - if (is_nonbare_repository_dir(&path)) { - if (enlistment_root) { - /* - * If the worktree's directory's name is `src`, the enlistment is the - * parent directory, otherwise it is identical to the worktree. - */ - root = strip_path_suffix(path.buf, "src"); - strbuf_addstr(enlistment_root, root ? root : path.buf); - free(root); - } - - enlistment_found = 1; - break; - } - } while (strbuf_parent_directory(&path)); - - if (!enlistment_found) - die(_("could not find enlistment root")); - - if (chdir(path.buf) < 0) - die_errno(_("could not switch to '%s'"), path.buf); + if (enlistment_root) { + if (enlistment_is_repo_parent) + strbuf_addbuf(enlistment_root, &path); + else + strbuf_addstr(enlistment_root, the_repository->worktree); + } strbuf_release(&path); - setup_git_directory(); } static int run_git(const char *arg, ...) @@ -431,6 +401,8 @@ static int delete_enlistment(struct strbuf *enlistment) { #ifdef WIN32 struct strbuf parent = STRBUF_INIT; + size_t offset; + char *path_sep; #endif if (unregister_dir()) @@ -441,8 +413,10 @@ static int delete_enlistment(struct strbuf *enlistment) * Change the current directory to one outside of the enlistment so * that we may delete everything underneath it. */ - strbuf_addbuf(&parent, enlistment); - strbuf_parent_directory(&parent); + offset = offset_1st_component(enlistment->buf); + path_sep = find_last_dir_sep(enlistment->buf + offset); + strbuf_add(&parent, enlistment->buf, + path_sep ? path_sep - enlistment->buf : offset); if (chdir(parent.buf) < 0) die_errno(_("could not switch to '%s'"), parent.buf); strbuf_release(&parent); diff --git a/contrib/scalar/t/t9099-scalar.sh b/contrib/scalar/t/t9099-scalar.sh index 10b1172a8a..c069cffebf 100755 --- a/contrib/scalar/t/t9099-scalar.sh +++ b/contrib/scalar/t/t9099-scalar.sh @@ -17,6 +17,91 @@ test_expect_success 'scalar shows a usage' ' test_expect_code 129 scalar -h ' +test_expect_success 'scalar invoked on enlistment root' ' + test_when_finished rm -rf test src deeper && + + for enlistment_root in test src deeper/test + do + git init ${enlistment_root}/src && + + # Register + scalar register ${enlistment_root} && + scalar list >out && + grep "$(pwd)/${enlistment_root}/src\$" out && + + # Delete (including enlistment root) + scalar delete $enlistment_root && + test_path_is_missing $enlistment_root && + scalar list >out && + ! grep "^$(pwd)/${enlistment_root}/src\$" out || return 1 + done +' + +test_expect_success 'scalar invoked on enlistment src repo' ' + test_when_finished rm -rf test src deeper && + + for enlistment_root in test src deeper/test + do + git init ${enlistment_root}/src && + + # Register + scalar register ${enlistment_root}/src && + scalar list >out && + grep "$(pwd)/${enlistment_root}/src\$" out && + + # Delete (will not include enlistment root) + scalar delete ${enlistment_root}/src && + test_path_is_dir $enlistment_root && + scalar list >out && + ! grep "^$(pwd)/${enlistment_root}/src\$" out || return 1 + done +' + +test_expect_success 'scalar invoked when enlistment root and repo are the same' ' + test_when_finished rm -rf test src deeper && + + for enlistment_root in test src deeper/test + do + git init ${enlistment_root} && + + # Register + scalar register ${enlistment_root} && + scalar list >out && + grep "$(pwd)/${enlistment_root}\$" out && + + # Delete (will not include enlistment root) + scalar delete ${enlistment_root} && + test_path_is_missing $enlistment_root && + scalar list >out && + ! grep "^$(pwd)/${enlistment_root}\$" out && + + # Make sure we did not accidentally delete the trash dir + test_path_is_dir "$TRASH_DIRECTORY" || return 1 + done +' + +test_expect_success 'scalar repo search respects GIT_CEILING_DIRECTORIES' ' + test_when_finished rm -rf test && + + git init test/src && + mkdir -p test/src/deep && + GIT_CEILING_DIRECTORIES="$(pwd)/test/src" && + ! scalar register test/src/deep 2>err && + grep "not a git repository" err +' + +test_expect_success 'scalar enlistments need a worktree' ' + test_when_finished rm -rf bare test && + + git init --bare bare/src && + ! scalar register bare/src 2>err && + grep "Scalar enlistments require a worktree" err && + + git init test/src && + ! scalar register test/src/.git 2>err && + grep "Scalar enlistments require a worktree" err +' + test_expect_success 'scalar unregister' ' git init vanish/src && scalar register vanish/src && From adedcee8115038913ad551d88a9a040973066d4d Mon Sep 17 00:00:00 2001 From: Victoria Dye Date: Thu, 18 Aug 2022 21:40:47 +0000 Subject: [PATCH 2/8] scalar-unregister: handle error codes greater than 0 When 'scalar unregister' tries to disable maintenance and remove an enlistment, ensure that the return value is nonzero if either operation produces *any* nonzero return value, not just when they return a value less than 0. Signed-off-by: Victoria Dye Signed-off-by: Junio C Hamano --- contrib/scalar/scalar.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/contrib/scalar/scalar.c b/contrib/scalar/scalar.c index 92b648f351..8ef8dd5504 100644 --- a/contrib/scalar/scalar.c +++ b/contrib/scalar/scalar.c @@ -223,10 +223,10 @@ static int unregister_dir(void) { int res = 0; - if (toggle_maintenance(0) < 0) + if (toggle_maintenance(0)) res = -1; - if (add_or_remove_enlistment(0) < 0) + if (add_or_remove_enlistment(0)) res = -1; return res; From d2a79bc953375ed7dd8cb06fd3db92f85c64c206 Mon Sep 17 00:00:00 2001 From: Victoria Dye Date: Thu, 18 Aug 2022 21:40:48 +0000 Subject: [PATCH 3/8] scalar-[un]register: clearly indicate source of error When a step in 'register_dir()' or 'unregister_dir()' fails, indicate which step failed with an error message, rather than silently assigning a nonzero return code. Signed-off-by: Victoria Dye Signed-off-by: Junio C Hamano --- contrib/scalar/scalar.c | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/contrib/scalar/scalar.c b/contrib/scalar/scalar.c index 8ef8dd5504..7be2a938b0 100644 --- a/contrib/scalar/scalar.c +++ b/contrib/scalar/scalar.c @@ -208,15 +208,16 @@ static int add_or_remove_enlistment(int add) static int register_dir(void) { - int res = add_or_remove_enlistment(1); + if (add_or_remove_enlistment(1)) + return error(_("could not add enlistment")); - if (!res) - res = set_recommended_config(0); + if (set_recommended_config(0)) + return error(_("could not set recommended config")); - if (!res) - res = toggle_maintenance(1); + if (toggle_maintenance(1)) + return error(_("could not turn on maintenance")); - return res; + return 0; } static int unregister_dir(void) @@ -224,10 +225,10 @@ static int unregister_dir(void) int res = 0; if (toggle_maintenance(0)) - res = -1; + res = error(_("could not turn off maintenance")); if (add_or_remove_enlistment(0)) - res = -1; + res = error(_("could not remove enlistment")); return res; } From 9b24bb9205e62464c09882e626d17710fb62b82d Mon Sep 17 00:00:00 2001 From: Victoria Dye Date: Thu, 18 Aug 2022 21:40:49 +0000 Subject: [PATCH 4/8] scalar-delete: do not 'die()' in 'delete_enlistment()' Rather than exiting with 'die()' when 'delete_enlistment()' encounters an error, return an error code with the appropriate message. There's no need for an abrupt exit with 'die()' in 'delete_enlistment()' because its only caller ('cmd_delete()') properly cleans up allocated resources and returns the 'delete_enlistment()' return value as its own exit code. Signed-off-by: Victoria Dye Signed-off-by: Junio C Hamano --- contrib/scalar/scalar.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/contrib/scalar/scalar.c b/contrib/scalar/scalar.c index 7be2a938b0..6de4d5b372 100644 --- a/contrib/scalar/scalar.c +++ b/contrib/scalar/scalar.c @@ -407,7 +407,7 @@ static int delete_enlistment(struct strbuf *enlistment) #endif if (unregister_dir()) - die(_("failed to unregister repository")); + return error(_("failed to unregister repository")); #ifdef WIN32 /* @@ -418,13 +418,16 @@ static int delete_enlistment(struct strbuf *enlistment) path_sep = find_last_dir_sep(enlistment->buf + offset); strbuf_add(&parent, enlistment->buf, path_sep ? path_sep - enlistment->buf : offset); - if (chdir(parent.buf) < 0) - die_errno(_("could not switch to '%s'"), parent.buf); + if (chdir(parent.buf) < 0) { + int res = error_errno(_("could not switch to '%s'"), parent.buf); + strbuf_release(&parent); + return res; + } strbuf_release(&parent); #endif if (remove_dir_recursively(enlistment, 0)) - die(_("failed to delete enlistment directory")); + return error(_("failed to delete enlistment directory")); return 0; } From d934a11c718f35f5593cfb49da994b43e74dd940 Mon Sep 17 00:00:00 2001 From: Victoria Dye Date: Thu, 18 Aug 2022 21:40:50 +0000 Subject: [PATCH 5/8] scalar: move config setting logic into its own function Create function 'set_scalar_config()' to contain the logic used in setting Scalar-defined Git config settings, including how to handle reconfiguring & overwriting existing values. This function allows future patches to set config values in parts of 'scalar.c' other than 'set_recommended_config()'. Signed-off-by: Victoria Dye Signed-off-by: Junio C Hamano --- contrib/scalar/scalar.c | 44 ++++++++++++++++++++++++++--------------- 1 file changed, 28 insertions(+), 16 deletions(-) diff --git a/contrib/scalar/scalar.c b/contrib/scalar/scalar.c index 6de4d5b372..836a4c48fa 100644 --- a/contrib/scalar/scalar.c +++ b/contrib/scalar/scalar.c @@ -85,13 +85,33 @@ static int run_git(const char *arg, ...) return res; } +struct scalar_config { + const char *key; + const char *value; + int overwrite_on_reconfigure; +}; + +static int set_scalar_config(const struct scalar_config *config, int reconfigure) +{ + char *value = NULL; + int res; + + if ((reconfigure && config->overwrite_on_reconfigure) || + git_config_get_string(config->key, &value)) { + trace2_data_string("scalar", the_repository, config->key, "created"); + res = git_config_set_gently(config->key, config->value); + } else { + trace2_data_string("scalar", the_repository, config->key, "exists"); + res = 0; + } + + free(value); + return res; +} + static int set_recommended_config(int reconfigure) { - struct { - const char *key; - const char *value; - int overwrite_on_reconfigure; - } config[] = { + struct scalar_config config[] = { /* Required */ { "am.keepCR", "true", 1 }, { "core.FSCache", "true", 1 }, @@ -145,17 +165,9 @@ static int set_recommended_config(int reconfigure) char *value; for (i = 0; config[i].key; i++) { - if ((reconfigure && config[i].overwrite_on_reconfigure) || - git_config_get_string(config[i].key, &value)) { - trace2_data_string("scalar", the_repository, config[i].key, "created"); - if (git_config_set_gently(config[i].key, - config[i].value) < 0) - return error(_("could not configure %s=%s"), - config[i].key, config[i].value); - } else { - trace2_data_string("scalar", the_repository, config[i].key, "exists"); - free(value); - } + if (set_scalar_config(config + i, reconfigure)) + return error(_("could not configure %s=%s"), + config[i].key, config[i].value); } /* From 3f1917dc608021d7e94f4aaed9ecc9f58f9e32a4 Mon Sep 17 00:00:00 2001 From: Matthew John Cheetham Date: Thu, 18 Aug 2022 21:40:51 +0000 Subject: [PATCH 6/8] scalar: enable built-in FSMonitor on `register` Using the built-in FSMonitor makes many common commands quite a bit faster. So let's teach the `scalar register` command to enable the built-in FSMonitor and kick-start the fsmonitor--daemon process (for convenience). For simplicity, we only support the built-in FSMonitor (and no external file system monitor such as e.g. Watchman). Helped-by: Junio C Hamano Helped-by: Derrick Stolee Signed-off-by: Matthew John Cheetham Signed-off-by: Johannes Schindelin Signed-off-by: Victoria Dye Signed-off-by: Junio C Hamano --- contrib/scalar/scalar.c | 30 ++++++++++++++++++++++++++++++ contrib/scalar/t/t9099-scalar.sh | 8 ++++++++ 2 files changed, 38 insertions(+) diff --git a/contrib/scalar/scalar.c b/contrib/scalar/scalar.c index 836a4c48fa..73cd5b1fd0 100644 --- a/contrib/scalar/scalar.c +++ b/contrib/scalar/scalar.c @@ -7,6 +7,9 @@ #include "parse-options.h" #include "config.h" #include "run-command.h" +#include "simple-ipc.h" +#include "fsmonitor-ipc.h" +#include "fsmonitor-settings.h" #include "refs.h" #include "dir.h" #include "packfile.h" @@ -109,6 +112,12 @@ static int set_scalar_config(const struct scalar_config *config, int reconfigure return res; } +static int have_fsmonitor_support(void) +{ + return fsmonitor_ipc__is_supported() && + fsm_settings__get_reason(the_repository) == FSMONITOR_REASON_OK; +} + static int set_recommended_config(int reconfigure) { struct scalar_config config[] = { @@ -170,6 +179,13 @@ static int set_recommended_config(int reconfigure) config[i].key, config[i].value); } + if (have_fsmonitor_support()) { + struct scalar_config fsmonitor = { "core.fsmonitor", "true" }; + if (set_scalar_config(&fsmonitor, reconfigure)) + return error(_("could not configure %s=%s"), + fsmonitor.key, fsmonitor.value); + } + /* * The `log.excludeDecoration` setting is special because it allows * for multiple values. @@ -218,6 +234,16 @@ static int add_or_remove_enlistment(int add) "scalar.repo", the_repository->worktree, NULL); } +static int start_fsmonitor_daemon(void) +{ + assert(have_fsmonitor_support()); + + if (fsmonitor_ipc__get_state() != IPC_STATE__LISTENING) + return run_git("fsmonitor--daemon", "start", NULL); + + return 0; +} + static int register_dir(void) { if (add_or_remove_enlistment(1)) @@ -229,6 +255,10 @@ static int register_dir(void) if (toggle_maintenance(1)) return error(_("could not turn on maintenance")); + if (have_fsmonitor_support() && start_fsmonitor_daemon()) { + return error(_("could not start the FSMonitor daemon")); + } + return 0; } diff --git a/contrib/scalar/t/t9099-scalar.sh b/contrib/scalar/t/t9099-scalar.sh index c069cffebf..365eab9b54 100755 --- a/contrib/scalar/t/t9099-scalar.sh +++ b/contrib/scalar/t/t9099-scalar.sh @@ -102,6 +102,14 @@ test_expect_success 'scalar enlistments need a worktree' ' grep "Scalar enlistments require a worktree" err ' +test_expect_success FSMONITOR_DAEMON 'scalar register starts fsmon daemon' ' + git init test/src && + test_must_fail git -C test/src fsmonitor--daemon status && + scalar register test/src && + git -C test/src fsmonitor--daemon status && + test_cmp_config -C test/src true core.fsmonitor +' + test_expect_success 'scalar unregister' ' git init vanish/src && scalar register vanish/src && From ec4c23116b2d1bb34b3952c0597032ca8ed6e5fc Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 18 Aug 2022 21:40:52 +0000 Subject: [PATCH 7/8] scalar unregister: stop FSMonitor daemon Especially on Windows, we will need to stop that daemon, just in case that the directory needs to be removed (the daemon would otherwise hold a handle to that directory, preventing it from being deleted). Helped-by: Junio C Hamano Signed-off-by: Johannes Schindelin Signed-off-by: Victoria Dye Signed-off-by: Junio C Hamano --- contrib/scalar/scalar.c | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/contrib/scalar/scalar.c b/contrib/scalar/scalar.c index 73cd5b1fd0..07c3f7dd6b 100644 --- a/contrib/scalar/scalar.c +++ b/contrib/scalar/scalar.c @@ -244,6 +244,16 @@ static int start_fsmonitor_daemon(void) return 0; } +static int stop_fsmonitor_daemon(void) +{ + assert(have_fsmonitor_support()); + + if (fsmonitor_ipc__get_state() == IPC_STATE__LISTENING) + return run_git("fsmonitor--daemon", "stop", NULL); + + return 0; +} + static int register_dir(void) { if (add_or_remove_enlistment(1)) @@ -468,6 +478,9 @@ static int delete_enlistment(struct strbuf *enlistment) strbuf_release(&parent); #endif + if (have_fsmonitor_support() && stop_fsmonitor_daemon()) + return error(_("failed to stop the FSMonitor daemon")); + if (remove_dir_recursively(enlistment, 0)) return error(_("failed to delete enlistment directory")); From 8e2841890a14496c9ee91e1b0f74dc178848049d Mon Sep 17 00:00:00 2001 From: Victoria Dye Date: Thu, 18 Aug 2022 21:40:53 +0000 Subject: [PATCH 8/8] scalar: update technical doc roadmap with FSMonitor support Update the Scalar roadmap to reflect completion of enabling the built-in FSMonitor in Scalar. Note that implementation of 'scalar help' was moved to the final set of changes to move Scalar out of 'contrib/'. This is due to a dependency on changes to 'git help', as all changes to the main Git tree *exclusively* implemented to support Scalar are part of that series. Signed-off-by: Victoria Dye Signed-off-by: Junio C Hamano --- Documentation/technical/scalar.txt | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/Documentation/technical/scalar.txt b/Documentation/technical/scalar.txt index 08bc09c225..047390e46e 100644 --- a/Documentation/technical/scalar.txt +++ b/Documentation/technical/scalar.txt @@ -84,13 +84,13 @@ series have been accepted: - `scalar-diagnose`: The `scalar` command is taught the `diagnose` subcommand. +- 'scalar-add-fsmonitor: Enable the built-in FSMonitor in Scalar + enlistments. At the end of this series, Scalar should be feature-complete + from the perspective of a user. + Roughly speaking (and subject to change), the following series are needed to "finish" this initial version of Scalar: -- Finish Scalar features: Enable the built-in FSMonitor in Scalar enlistments - and implement `scalar help`. At the end of this series, Scalar should be - feature-complete from the perspective of a user. - - Generalize features not specific to Scalar: In the spirit of making Scalar configure only what is needed for large repo performance, move common utilities into other parts of Git. Some of this will be internal-only, but one @@ -98,9 +98,12 @@ Roughly speaking (and subject to change), the following series are needed to repository. - Move Scalar to toplevel: Move Scalar out of `contrib/` and into the root of - `git`, including updates to build and install it with the rest of Git. This - change will incorporate Scalar into the Git CI and test framework, as well as - expand regression and performance testing to ensure the tool is stable. + `git`. This includes a variety of related updates, including: + - building & installing Scalar in the Git root-level 'make [install]'. + - builing & testing Scalar as part of CI. + - moving and expanding test coverage of Scalar (including perf tests). + - implementing 'scalar help'/'git help scalar' to display scalar + documentation. Finally, there are two additional patch series that exist in Microsoft's fork of Git, but there is no current plan to upstream them. There are some interesting