| Message ID | 5fdf8337972d7092aba06a9c750f42cd5868e630.1660694290.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/5] scalar-unregister: handle error codes greater than 0 | expand |
On 8/16/2022 7:58 PM, Matthew John Cheetham via GitGitGadget wrote: > +#ifdef HAVE_FSMONITOR_DAEMON_BACKEND > + /* > + * Enable the built-in FSMonitor on supported platforms. > + */ > + { "core.fsmonitor", "true" }, > +#endif > + if (fsmonitor_ipc__is_supported() && start_fsmonitor_daemon()) > + return error(_("could not start the FSMonitor daemon")); > + I initially worried if fsmonitor_ipc__is_supported() could use some run-time information to detect if FS Monitor is supported (say, existence of a network share or something). However, that implementation is currently defined as a constant depending on HAVE_FSMONITOR_DAEMON_BACKEND. The reason I was worried is that we could enable core.fsmonitor=true based on the compile-time macro, but then avoid starting the daemon based on the run-time results. If we get into this state, would the user's 'git status' calls start complaining about the core.fsmonitor=true config because it is not supported? The most future-proof thing to do might be to move the config write out of the set_recommended_config() and into start_fsmonitor_daemon(). Perhaps rename it to enable_fsmonitor() so it can fail due to writing the config _or_ for starting the daemon. The error message would change, then, too. Or maybe I'm making a mountain out of a mole hill and what exists here is perfectly fine. > +test_lazy_prereq BUILTIN_FSMONITOR ' > + git version --build-options | grep -q "feature:.*fsmonitor--daemon" > +' It looks like we already have a FSMONITOR_DAEMON prereq in test-lib.sh. Should we use that instead? Thanks, -Stolee
"Matthew John Cheetham via GitGitGadget" <gitgitgadget@gmail.com> writes: > +static int start_fsmonitor_daemon(void) > +{ > + assert(fsmonitor_ipc__is_supported()); > + > + if (fsmonitor_ipc__get_state() != IPC_STATE__LISTENING) > + return run_git("fsmonitor--daemon", "start", NULL); > + > + return 0; > +} The function got ultra simple ;-). > @@ -247,6 +265,9 @@ static int register_dir(void) > if (toggle_maintenance(1)) > return error(_("could not turn on maintenance")); > > + if (fsmonitor_ipc__is_supported() && start_fsmonitor_daemon()) > + return error(_("could not start the FSMonitor daemon")); > + > return 0; > } As long as it is done consistently, I do not think it makes a huge difference between the "call it only when supported" and "when asked to do what we do not support, silently succeed without doing anything". It however makes the code appear to be more in control to do it this way, I think, which is good.
Derrick Stolee <derrickstolee@github.com> writes: > On 8/16/2022 7:58 PM, Matthew John Cheetham via GitGitGadget wrote: > >> +#ifdef HAVE_FSMONITOR_DAEMON_BACKEND >> + /* >> + * Enable the built-in FSMonitor on supported platforms. >> + */ >> + { "core.fsmonitor", "true" }, >> +#endif >> + if (fsmonitor_ipc__is_supported() && start_fsmonitor_daemon()) >> + return error(_("could not start the FSMonitor daemon")); >> + > > I initially worried if fsmonitor_ipc__is_supported() could use some > run-time information to detect if FS Monitor is supported (say, existence > of a network share or something). However, that implementation is > currently defined as a constant depending on > HAVE_FSMONITOR_DAEMON_BACKEND. > > The reason I was worried is that we could enable core.fsmonitor=true based > on the compile-time macro, but then avoid starting the daemon based on the > run-time results. If we get into this state, would the user's 'git status' > calls start complaining about the core.fsmonitor=true config because it is > not supported? Ah, I didn't consider the possibility where the user uses the configuration to say "enable it if you are able, but it is OK if you cannot". Whether the "is supported" is dynamic or compiled-in, that may be a valid issue to consider. An easy way out may be to declare that the value "true" for "core.fsmonitor" variable means exactly that, i.e. the user asks to run it, but it is not an error if it cannot run. A variant that may need slightly more work would be to introduce a separate value (perhaps "when-able") that means that, while keeping the "true" to mean "run the built-in one, or error out to let me know otherwise" as before. Thanks.
Derrick Stolee wrote: > On 8/16/2022 7:58 PM, Matthew John Cheetham via GitGitGadget wrote: > >> +#ifdef HAVE_FSMONITOR_DAEMON_BACKEND >> + /* >> + * Enable the built-in FSMonitor on supported platforms. >> + */ >> + { "core.fsmonitor", "true" }, >> +#endif >> + if (fsmonitor_ipc__is_supported() && start_fsmonitor_daemon()) >> + return error(_("could not start the FSMonitor daemon")); >> + > > I initially worried if fsmonitor_ipc__is_supported() could use some > run-time information to detect if FS Monitor is supported (say, existence > of a network share or something). However, that implementation is > currently defined as a constant depending on > HAVE_FSMONITOR_DAEMON_BACKEND. > > The reason I was worried is that we could enable core.fsmonitor=true based > on the compile-time macro, but then avoid starting the daemon based on the > run-time results. If we get into this state, would the user's 'git status' > calls start complaining about the core.fsmonitor=true config because it is > not supported? > > The most future-proof thing to do might be to move the config write out of > the set_recommended_config() and into start_fsmonitor_daemon(). Perhaps > rename it to enable_fsmonitor() so it can fail due to writing the config > _or_ for starting the daemon. The error message would change, then, too. I spent some time digging into this, and I think gating both the config and subsequent 'git fsmonitor--daemon start' on having platform *and* repository support is a good idea. I'll update the next version to both set the 'core.fsmonitor' config and start the daemon only if the built-in FSMonitor is fully supported. (warning: long-winded tangent mostly unrelated to FSMonitor) In the process of testing FSMonitor behavior, I think found other issues with Scalar registration. Specifically, the test I wrote attempted to 'scalar register' a bare repo, since bare directories are incompatible with FSMonitor. After seeing that FSMonitor was *not* incompatible with the repository, I found that Scalar was 1) ignoring the bare repository and, as a result, 2) identifying my Git clone (way above GIT_CEILING_DIRECTORIES) as the "enlistment root". I think 1) might be fine as-is - uniformly ignoring bare repos seems like a reasonable choice - but 2) seems like more of a problem. Right now, 'setup_enlistment_directory()' searches for the repo root beginning at directory '<dir>', which is either a user-provided path or current working directory. It checks whether '<dir>' or '<dir>/src' is a repo root: if so, it sets the enlistment info; otherwise, it repeats the process with the parent of '<dir>' until the repo root is found. For example, given the following directory structure: somedir └── enlistment ├── src │ └── .git └── test └── data 'scalar register somedir/enlistment/test/data' will search: * somedir/enlistment/test/data/src * somedir/enlistment/test/data * somedir/enlistment/test/src * somedir/enlistment/test * somedir/enlistment/src The current usage of GIT_CEILING_DIRECTORIES relies on the fact that, when invoking a normal 'git' command, 'setup_git_directory()' only searches upwards from the current working directory to find the repo root; it's a clear "yes" or "no" as to whether that search passes a ceiling directory. Scalar isn't as clear, since it searches for the repo root both "downwards" into '<dir>/src' *and* upwards through the parents of '<dir>'. It's not totally clear to me what the "right" behavior for Scalar is, but my current thought is to follow the same rules as 'setup_git_directory()', but for the *enlistment* root rather than the repository root. It's more restrictive than GIT_CEILING_DIRECTORIES on a normal git repo, e.g.: 1. 'GIT_CEILING_DIRECTORIES=somedir/enlistment git -C somedir/enlistment/src status' is valid. 2. 'GIT_CEILING_DIRECTORIES=somedir/enlistment scalar register somedir/enlistment/src' is not valid. but since Scalar works on the entire enlistment (not just the repo inside of it), I think it makes sense to prevent it from crossing a ceiling directory boundary. What do you think? Hopefully my rambling wasn't too confusing (if it is, please let me know what I can clarify). > > Or maybe I'm making a mountain out of a mole hill and what exists here is > perfectly fine. > >> +test_lazy_prereq BUILTIN_FSMONITOR ' >> + git version --build-options | grep -q "feature:.*fsmonitor--daemon" >> +' > > It looks like we already have a FSMONITOR_DAEMON prereq in test-lib.sh. > Should we use that instead? Works for me, happy to reuse code wherever possible. :) > > Thanks, > -Stolee
On 8/17/2022 7:47 PM, Victoria Dye wrote: > (warning: long-winded tangent mostly unrelated to FSMonitor) > > In the process of testing FSMonitor behavior, I think found other issues > with Scalar registration. Specifically, the test I wrote attempted to > 'scalar register' a bare repo, since bare directories are incompatible with > FSMonitor. After seeing that FSMonitor was *not* incompatible with the > repository, I found that Scalar was 1) ignoring the bare repository and, as This is interesting, that Scalar doesn't recognize a bare repo. There are definitely some config settings that it recommends that don't make sense in a bare repo, but it's interesting that it completely ignores it. Good find. I'm not sure there is anything to 'fix' except maybe error out when the discovered Git repository is bare. Add a warning, at minimum. > a result, 2) identifying my Git clone (way above GIT_CEILING_DIRECTORIES) as > the "enlistment root". I think 1) might be fine as-is - uniformly ignoring > bare repos seems like a reasonable choice - but 2) seems like more of a > problem. ... > The current usage of GIT_CEILING_DIRECTORIES relies on the fact that, when > invoking a normal 'git' command, 'setup_git_directory()' only searches > upwards from the current working directory to find the repo root; it's a > clear "yes" or "no" as to whether that search passes a ceiling directory. > Scalar isn't as clear, since it searches for the repo root both "downwards" > into '<dir>/src' *and* upwards through the parents of '<dir>'. It's not > totally clear to me what the "right" behavior for Scalar is, but my current > thought is to follow the same rules as 'setup_git_directory()', but for the > *enlistment* root rather than the repository root. It's more restrictive > than GIT_CEILING_DIRECTORIES on a normal git repo, e.g.: > > 1. 'GIT_CEILING_DIRECTORIES=somedir/enlistment git -C somedir/enlistment/src status' > is valid. > 2. 'GIT_CEILING_DIRECTORIES=somedir/enlistment scalar register somedir/enlistment/src' > is not valid. This is interesting, that we can't recognize the ceiling as the root. > but since Scalar works on the entire enlistment (not just the repo inside of > it), I think it makes sense to prevent it from crossing a ceiling directory > boundary. I think the enlistment root was something that was inherited from VFS for Git, and we can mostly abandon it. The things we need to do are all based on the Git repository itself, not the parent. The only thing we need to keep is to allow a user to specify the repo by pointing to the directory immediately above the 'src' directory. > 'scalar register somedir/enlistment/test/data' will search: > > * somedir/enlistment/test/data/src > * somedir/enlistment/test/data > * somedir/enlistment/test/src > * somedir/enlistment/test > * somedir/enlistment/src Instead, we could do the following on a specified <dir>: * If <dir>/src exists, find the Git directory by finding the first Git repository containing <dir>/src. * Otherwise, find the first Git repository containing <dir>. Is there an easy way to discover a Git repository at a specific directory? Or, do we do something simpler, like changing directories then calling setup_git_directory()? I think simplifying the logic that way should respect GIT_CEILING_DIRECTORIES correctly. Thanks, -Stolee
diff --git a/contrib/scalar/scalar.c b/contrib/scalar/scalar.c index 6025cd71604..28d915ec006 100644 --- a/contrib/scalar/scalar.c +++ b/contrib/scalar/scalar.c @@ -7,6 +7,8 @@ #include "parse-options.h" #include "config.h" #include "run-command.h" +#include "simple-ipc.h" +#include "fsmonitor-ipc.h" #include "refs.h" #include "dir.h" #include "packfile.h" @@ -169,6 +171,12 @@ static int set_recommended_config(int reconfigure) { "core.autoCRLF", "false" }, { "core.safeCRLF", "false" }, { "fetch.showForcedUpdates", "false" }, +#ifdef HAVE_FSMONITOR_DAEMON_BACKEND + /* + * Enable the built-in FSMonitor on supported platforms. + */ + { "core.fsmonitor", "true" }, +#endif { NULL, NULL }, }; int i; @@ -236,6 +244,16 @@ static int add_or_remove_enlistment(int add) "scalar.repo", the_repository->worktree, NULL); } +static int start_fsmonitor_daemon(void) +{ + assert(fsmonitor_ipc__is_supported()); + + if (fsmonitor_ipc__get_state() != IPC_STATE__LISTENING) + return run_git("fsmonitor--daemon", "start", NULL); + + return 0; +} + static int register_dir(void) { if (add_or_remove_enlistment(1)) @@ -247,6 +265,9 @@ static int register_dir(void) if (toggle_maintenance(1)) return error(_("could not turn on maintenance")); + if (fsmonitor_ipc__is_supported() && start_fsmonitor_daemon()) + return error(_("could not start the FSMonitor daemon")); + return 0; } diff --git a/contrib/scalar/t/t9099-scalar.sh b/contrib/scalar/t/t9099-scalar.sh index 10b1172a8aa..526f64d001c 100755 --- a/contrib/scalar/t/t9099-scalar.sh +++ b/contrib/scalar/t/t9099-scalar.sh @@ -13,10 +13,21 @@ PATH=$PWD/..:$PATH GIT_TEST_MAINT_SCHEDULER="crontab:test-tool crontab ../cron.txt,launchctl:true,schtasks:true" export GIT_TEST_MAINT_SCHEDULER +test_lazy_prereq BUILTIN_FSMONITOR ' + git version --build-options | grep -q "feature:.*fsmonitor--daemon" +' + test_expect_success 'scalar shows a usage' ' test_expect_code 129 scalar -h ' +test_expect_success BUILTIN_FSMONITOR 'scalar register starts fsmon daemon' ' + git init test/src && + test_must_fail git -C test/src fsmonitor--daemon status && + scalar register test/src && + git -C test/src fsmonitor--daemon status +' + test_expect_success 'scalar unregister' ' git init vanish/src && scalar register vanish/src &&