scalar: enable built-in FSMonitor #1324
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
There are issues in commit 746b37f: |
|
@vdye: you can point this to the |
|
/submit |
|
Submitted as pull.1324.git.1660673269.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): "Victoria Dye via GitGitGadget" <gitgitgadget@gmail.com> writes:
> This series enables the built-in FSMonitor [1] on 'scalar'-registered
> repository enlistments. To avoid errors when unregistering an enlistment,
> the FSMonitor daemon is explicitly stopped during 'scalar unregister'.
>
> Maintainer's note: this series has a minor conflict with
> 'vd/scalar-generalize-diagnose'. Please let me know if there's anything else
> I can provide (in addition to [2]) that would make resolution easier.
Thanks. What's the doneness of the other series? It has cooked for
quite a while and I was wondering if it is ready for 'next' already,
by the way.
|
|
On the Git mailing list, Victoria Dye wrote (reply to this): Junio C Hamano wrote:
> "Victoria Dye via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> This series enables the built-in FSMonitor [1] on 'scalar'-registered
>> repository enlistments. To avoid errors when unregistering an enlistment,
>> the FSMonitor daemon is explicitly stopped during 'scalar unregister'.
>>
>> Maintainer's note: this series has a minor conflict with
>> 'vd/scalar-generalize-diagnose'. Please let me know if there's anything else
>> I can provide (in addition to [2]) that would make resolution easier.
>
> Thanks. What's the doneness of the other series? It has cooked for
> quite a while and I was wondering if it is ready for 'next' already,
> by the way.
>
I wasn't planning on making any other changes unless more comments came in;
I personally think it's ready for 'next', but I'm not sure about reviewers'
thoughts. There seemed to be some new interest in reviewing at the IRC
stand-up yesterday [1], but I haven't heard anything since then.
On the off chance that some major blocking review to
'vd/scalar-generalize-diagnose' comes in, I didn't want to base this series
on that one. But, if it *is* merged to 'next' before this one, I'll make
sure to rebase this series on top in subsequent versions to avoid the merge
conflict.
[1] https://colabti.org/irclogger/irclogger_log/git-devel?date=2022-08-15#l83 |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): Victoria Dye <vdye@github.com> writes:
> On the off chance that some major blocking review to
> 'vd/scalar-generalize-diagnose' comes in, I didn't want to base this series
> on that one. But, if it *is* merged to 'next' before this one, I'll make
> sure to rebase this series on top in subsequent versions to avoid the merge
> conflict.
All sensible. I actually am running out of topics to merge to
'next' ;-) and that was the primary reason why I asked.
|
|
This branch is now known as |
|
This patch series was integrated into seen via git@a61e257. |
![gitgitgadget[bot]](https://avatars.githubusercontent.com/in/12836?s=60&v=4){kind=small}
| @@ -7,6 +7,8 @@ | |||
| #include "parse-options.h" | |||
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
"Matthew John Cheetham via GitGitGadget" <gitgitgadget@gmail.com>
writes:
> +static int start_fsmonitor_daemon(void)
> +{
> + int res = 0;
> + if (fsmonitor_ipc__is_supported() &&
> + fsmonitor_ipc__get_state() != IPC_STATE__LISTENING) {
> + struct strbuf err = STRBUF_INIT;
> + struct child_process cp = CHILD_PROCESS_INIT;
> +
> + /* Try to start the FSMonitor daemon */
> + cp.git_cmd = 1;
> + strvec_pushl(&cp.args, "fsmonitor--daemon", "start", NULL);
> + if (!pipe_command(&cp, NULL, 0, NULL, 0, &err, 0)) {
> + /* Successfully started FSMonitor */
> + strbuf_release(&err);
> + return 0;
> + }
> +
> + /* If FSMonitor really hasn't started, emit error */
> + if (fsmonitor_ipc__get_state() != IPC_STATE__LISTENING)
> + res = error(_("could not start the FSMonitor daemon: %s"),
> + err.buf);
> +
> + strbuf_release(&err);
> + }
> +
> + return res;
> +}
This somewhat curious code structure made me look, and made me
notice that the behaviour is even more curious. Even though
pipe_command() fails, fsmonitor_ipc__get_state() can somehow become
LISTENING, in which case we are OK? If that is the case, a more natural
way to write it would be:
int res = 0; /* assume success */
if (fsmonitor_ipc__is_supported() &&
fsmonitor_ipc__get_state() != IPC_STATE__LISTENING) {
...
/*
* if we fail to start it ourselves, and there is no
* daemon listening to us, it is an error.
*/
if (pipe_command(...) &&
fsmonitor_ipc__get_state() != IPC_STATE__LISTENING)
res = error(...);
strbuf_release(&err);
}
return res;
and that would utilize "res" consistently throughout the function.
Note that (I omitted unnecessary blank lines and added necessary
ones in the above outline of the rewrite.
Stopping, stepping back a bit and rethinking, the above is not still
exactly right. If pipe_command() could lie and say "we failed to
start" when we immediately after the failure can find a running
daemon, what guarantees us that pipe_command() does not lie in the
other direction? So, in that sense, perhaps doing
/* we do not care if pipe_command() succeeds or not */
(void) pipe_command(...);
/*
* we check ourselves if we do have a usable daemon
* and that is the authoritative answer. we were asked
* to ensure that usable daemon exists, and we answer
* if we do or don't.
*/
if (fsmonitor_ipc__get_state() != IPC_STATE__LISTENING)
res = error(...);
may be more true to the spirit of the code.
It also is slightly curious if the caller wants to see "success"
when fsmonitor is not supported. I would have expected the caller
to check and refrain from calling start/stop when it is not
supported (and if there is an end-user interface to force the scalar
command to "start", complain by saying "not supported here"). But
as long as we are consistent, I guess it is OK.
The side that stops shares exactly the same two pieces of
"curiosity" and may need to be updated exactly the same way. It
assumes that pipe_command() is unreliable and instead of reporting a
possible failure, we sweep that under the rug, with a questionable
"we do not care about pipe failing, as long as the daemon is
listening, we do not care" attitude. And the caller does not care
"start" not stopping where it is not supported.
Thanks.There was a problem hiding this comment.
On the Git mailing list, Victoria Dye wrote (reply to this):
Junio C Hamano wrote:
> "Matthew John Cheetham via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
>
>> +static int start_fsmonitor_daemon(void)
>> +{
>> + int res = 0;
>> + if (fsmonitor_ipc__is_supported() &&
>> + fsmonitor_ipc__get_state() != IPC_STATE__LISTENING) {
>> + struct strbuf err = STRBUF_INIT;
>> + struct child_process cp = CHILD_PROCESS_INIT;
>> +
>> + /* Try to start the FSMonitor daemon */
>> + cp.git_cmd = 1;
>> + strvec_pushl(&cp.args, "fsmonitor--daemon", "start", NULL);
>> + if (!pipe_command(&cp, NULL, 0, NULL, 0, &err, 0)) {
>> + /* Successfully started FSMonitor */
>> + strbuf_release(&err);
>> + return 0;
>> + }
>> +
>> + /* If FSMonitor really hasn't started, emit error */
>> + if (fsmonitor_ipc__get_state() != IPC_STATE__LISTENING)
>> + res = error(_("could not start the FSMonitor daemon: %s"),
>> + err.buf);
>> +
>> + strbuf_release(&err);
>> + }
>> +
>> + return res;
>> +}
>
> This somewhat curious code structure made me look, and made me
> notice that the behaviour is even more curious. Even though
> pipe_command() fails, fsmonitor_ipc__get_state() can somehow become
> LISTENING, in which case we are OK? If that is the case, a more natural
> way to write it would be:
>
> int res = 0; /* assume success */
>
> if (fsmonitor_ipc__is_supported() &&
> fsmonitor_ipc__get_state() != IPC_STATE__LISTENING) {
> ...
> /*
> * if we fail to start it ourselves, and there is no
> * daemon listening to us, it is an error.
> */
> if (pipe_command(...) &&
> fsmonitor_ipc__get_state() != IPC_STATE__LISTENING)
> res = error(...);
> strbuf_release(&err);
> }
> return res;
>
> and that would utilize "res" consistently throughout the function.
>
> Note that (I omitted unnecessary blank lines and added necessary
> ones in the above outline of the rewrite.
>
> Stopping, stepping back a bit and rethinking, the above is not still
> exactly right. If pipe_command() could lie and say "we failed to
> start" when we immediately after the failure can find a running
> daemon, what guarantees us that pipe_command() does not lie in the
> other direction? So, in that sense, perhaps doing
>
> /* we do not care if pipe_command() succeeds or not */
> (void) pipe_command(...);
>
> /*
> * we check ourselves if we do have a usable daemon
> * and that is the authoritative answer. we were asked
> * to ensure that usable daemon exists, and we answer
> * if we do or don't.
> */
> if (fsmonitor_ipc__get_state() != IPC_STATE__LISTENING)
> res = error(...);
>
> may be more true to the spirit of the code.
This is an unintentional artifact of some minor refactoring of the original
versions in 'microsoft/git'. Previously [1], there was no
'fsmonitor_ipc__get_state()' check before calling 'git fsmonitor--daemon
start', so we'd expect failures whenever FSMonitor was already running. To
avoid making that 'pipe_command()' call when FSMonitor was already running,
I added an earlier call to 'fsmonitor_ipc__get_state()'. But, because I
didn't remove the later check, the code now implies that 'pipe_command()'
may give us "false negatives" (that is, fail but still manage to start the
FSMonitor).
I left the extraneous check in to be overly cautious, but realistically I
don't actually expect 'git fsmonitor--daemon start' to give us any false
positives or negatives. The code should reflect that:
int res = 0;
if (fsmonitor_ipc__is_supported() &&
fsmonitor_ipc__get_state() != IPC_STATE__LISTENING) {
struct strbuf err = STRBUF_INIT;
struct child_process cp = CHILD_PROCESS_INIT;
/* Try to start the FSMonitor daemon */
cp.git_cmd = 1;
strvec_pushl(&cp.args, "fsmonitor--daemon", "start", NULL);
if (pipe_command(&cp, NULL, 0, NULL, 0, &err, 0))
res = error(_("could not start the FSMonitor daemon: %s"),
err.buf);
strbuf_release(&err);
}
return res;
I'll re-roll with this shortly.
[1] https://github.com/microsoft/git/commit/4f2e092d3c98
>
> It also is slightly curious if the caller wants to see "success"
> when fsmonitor is not supported. I would have expected the caller
> to check and refrain from calling start/stop when it is not
> supported (and if there is an end-user interface to force the scalar
> command to "start", complain by saying "not supported here"). But
> as long as we are consistent, I guess it is OK.
I don't mind moving the 'fsmonitor_ipc__is_supported()' checks into
'register_dir()' and 'unregister_dir()'; I can see how it makes more sense
with the existing function name.
As a side note, though, while looking at where to move the condition I
noticed that 'unregister_dir()' doesn't handle positive, nonzero return
values properly. I'll fix this & move the 'fsmonitor_ipc__is_supported()'
check in the next version. Thanks!
>
> The side that stops shares exactly the same two pieces of
> "curiosity" and may need to be updated exactly the same way. It
> assumes that pipe_command() is unreliable and instead of reporting a
> possible failure, we sweep that under the rug, with a questionable
> "we do not care about pipe failing, as long as the daemon is
> listening, we do not care" attitude. And the caller does not care
> "start" not stopping where it is not supported.
>
> Thanks.
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
Victoria Dye <vdye@github.com> writes:
>> /* we do not care if pipe_command() succeeds or not */
>> (void) pipe_command(...);
>>
>> /*
>> * we check ourselves if we do have a usable daemon
>> * and that is the authoritative answer. we were asked
>> * to ensure that usable daemon exists, and we answer
>> * if we do or don't.
>> */
>> if (fsmonitor_ipc__get_state() != IPC_STATE__LISTENING)
>> res = error(...);
>>
>> may be more true to the spirit of the code.
>
> This is an unintentional artifact of some minor refactoring of the original
> versions in 'microsoft/git'. Previously [1], there was no
> 'fsmonitor_ipc__get_state()' check before calling 'git fsmonitor--daemon
> start', so we'd expect failures whenever FSMonitor was already running. To
> avoid making that 'pipe_command()' call when FSMonitor was already running,
> I added an earlier call to 'fsmonitor_ipc__get_state()'. But, because I
> didn't remove the later check, the code now implies that 'pipe_command()'
> may give us "false negatives" (that is, fail but still manage to start the
> FSMonitor).
>
> I left the extraneous check in to be overly cautious, but realistically I
> don't actually expect 'git fsmonitor--daemon start' to give us any false
> positives or negatives. The code should reflect that:
>
> int res = 0;
> if (fsmonitor_ipc__is_supported() &&
> fsmonitor_ipc__get_state() != IPC_STATE__LISTENING) {
> struct strbuf err = STRBUF_INIT;
> struct child_process cp = CHILD_PROCESS_INIT;
>
> /* Try to start the FSMonitor daemon */
> cp.git_cmd = 1;
> strvec_pushl(&cp.args, "fsmonitor--daemon", "start", NULL);
> if (pipe_command(&cp, NULL, 0, NULL, 0, &err, 0))
> res = error(_("could not start the FSMonitor daemon: %s"),
> err.buf);
>
> strbuf_release(&err);
> }
>
> return res;
>
> I'll re-roll with this shortly.
OK, that is one valid way to go about it. After I sent my review
comments, I however briefly wondered if we might *not* know if we
are already running one, there is a reliable exclusion mechansim
to prevent more than one monitor running at the same time, and we
are running pipe_command(), fully expecting that it may fail when
there is already a working one and a call to pipe_command() that
is not "checked" is just being lazy because we can afford to be
lazy here.
If that is not what is going on, then the cleaned up version I am
responding to does look more straight-forward and easy to
understand. On the other hand, if "we can start more than we need
because we can rely on the exclusion mechanism" is what is going on,
that is fine as well, but it does need to be documented, preferrably
as in-code comment.
Thanks.
|
/submit |
|
Submitted as pull.1324.v2.git.1660694290.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
| @@ -253,10 +253,10 @@ static int unregister_dir(void) | |||
| { | |||
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
"Victoria Dye via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Victoria Dye <vdye@github.com>
>
> When 'scalar unregister' tries to disable maintenance and remove an
> enlistment, ensure that the return value is nonzero if either operation
> produces *any* nonzero return value, not just when they return a value less
> than 0.
Interesting. Did this actually cause problems in the wild? Just
being curious.
The return values from toggle_maintenance() and add_or_remove() are
what scalar.c::run_git() returns, which in turn come from
run_command() and eventually come from wait_or_whine(), so it very
well can be a positive non-zero value that signals a failure. It is
good to be prepared to see not just negative values but also
positive ones.
> Signed-off-by: Victoria Dye <vdye@github.com>
> ---
> contrib/scalar/scalar.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/contrib/scalar/scalar.c b/contrib/scalar/scalar.c
> index 97e71fe19cd..e888fa5408e 100644
> --- a/contrib/scalar/scalar.c
> +++ b/contrib/scalar/scalar.c
> @@ -253,10 +253,10 @@ static int unregister_dir(void)
> {
> int res = 0;
>
> - if (toggle_maintenance(0) < 0)
> + if (toggle_maintenance(0))
> res = -1;
>
> - if (add_or_remove_enlistment(0) < 0)
> + if (add_or_remove_enlistment(0))
> res = -1;
>
> return res;
| @@ -7,6 +7,8 @@ | |||
| #include "parse-options.h" | |||
There was a problem hiding this comment.
On the Git mailing list, Derrick Stolee wrote (reply to this):
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,
-StoleeThere was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
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.There was a problem hiding this comment.
On the Git mailing list, Victoria Dye wrote (reply to this):
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
There was a problem hiding this comment.
On the Git mailing list, Derrick Stolee wrote (reply to this):
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
|
User |
| @@ -7,6 +7,8 @@ | |||
| #include "parse-options.h" | |||
There was a problem hiding this comment.
On the Git mailing list, Derrick Stolee wrote (reply to this):
On 8/16/2022 7:58 PM, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> Especially on Windows, we will need to stop that daemon, just in case
> that the directory needs to be removed (the daemon would otherwise hold
> a handle to that directory, preventing it from being deleted).
> +static int stop_fsmonitor_daemon(void)
> +{
> + assert(fsmonitor_ipc__is_supported());
> +
> + if (fsmonitor_ipc__get_state() == IPC_STATE__LISTENING)
> + return run_git("fsmonitor--daemon", "stop", NULL);
> +
> + return 0;
> +}
> +
> static int register_dir(void)
> {
> if (add_or_remove_enlistment(1))
> @@ -281,6 +291,9 @@ static int unregister_dir(void)
> if (add_or_remove_enlistment(0))
> res = error(_("could not remove enlistment"));
>
> + if (fsmonitor_ipc__is_supported() && stop_fsmonitor_daemon() < 0)
> + res = error(_("could not stop the FSMonitor daemon"));
> +
One thing that is interesting about 'scalar unregister' is that it does
not change config values. At that point, we don't know which config values
are valuable to keep or not because the user may have set them before
'scalar register', or otherwise liked the config options.
Here, the reason to stop the daemon is so we unlock the ability to delete
the directory on Windows.
Should this become part of cmd_delete() instead of unregister_dir()? Or,
do we think that users would opt to run 'scalar unregister' before trying
to delete their directory manually?
Thanks,
-StoleeThere was a problem hiding this comment.
On the Git mailing list, Victoria Dye wrote (reply to this):
Derrick Stolee wrote:
> On 8/16/2022 7:58 PM, Johannes Schindelin via GitGitGadget wrote:
>> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>>
>> Especially on Windows, we will need to stop that daemon, just in case
>> that the directory needs to be removed (the daemon would otherwise hold
>> a handle to that directory, preventing it from being deleted).
>
>> +static int stop_fsmonitor_daemon(void)
>> +{
>> + assert(fsmonitor_ipc__is_supported());
>> +
>> + if (fsmonitor_ipc__get_state() == IPC_STATE__LISTENING)
>> + return run_git("fsmonitor--daemon", "stop", NULL);
>> +
>> + return 0;
>> +}
>> +
>> static int register_dir(void)
>> {
>> if (add_or_remove_enlistment(1))
>> @@ -281,6 +291,9 @@ static int unregister_dir(void)
>> if (add_or_remove_enlistment(0))
>> res = error(_("could not remove enlistment"));
>>
>> + if (fsmonitor_ipc__is_supported() && stop_fsmonitor_daemon() < 0)
>> + res = error(_("could not stop the FSMonitor daemon"));
>> +
>
> One thing that is interesting about 'scalar unregister' is that it does
> not change config values. At that point, we don't know which config values
> are valuable to keep or not because the user may have set them before
> 'scalar register', or otherwise liked the config options.
>
> Here, the reason to stop the daemon is so we unlock the ability to delete
> the directory on Windows.
>
> Should this become part of cmd_delete() instead of unregister_dir()? Or,
> do we think that users would opt to run 'scalar unregister' before trying
> to delete their directory manually?
After reading this, my first thought was that 'scalar unregister' should
still turn off the FSMonitor daemon because, in addition to allowing for
directory deletion in 'scalar delete', it's "cleaning up" some
optionally-enabled behavior associated with Scalar (a la
'toggle_maintenance(0)'). However, given that 'unregister' doesn't clear
'core.fsmonitor', it really *isn't* comparable to 'toggle_maintenance(0)'.
So I think you're right that it should only be associated with enlistment
deletion (although I think 'delete_enlistment()' is the place for that -
right before 'remove_dir_recursively()' - rather than 'cmd_delete()').
Thanks!
>
> Thanks,
> -Stolee
There was a problem hiding this comment.
On the Git mailing list, Derrick Stolee wrote (reply to this):
On 8/17/2022 1:36 PM, Victoria Dye wrote:
> So I think you're right that it should only be associated with enlistment
> deletion (although I think 'delete_enlistment()' is the place for that -
> right before 'remove_dir_recursively()' - rather than 'cmd_delete()').
Ah. You're absolutely right about that.
Thanks,
-Stolee
| @@ -7,6 +7,8 @@ | |||
| #include "parse-options.h" | |||
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
"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.
|
On the Git mailing list, Derrick Stolee wrote (reply to this): On 8/16/2022 7:58 PM, Victoria Dye via GitGitGadget wrote:
> This series enables the built-in FSMonitor [1] on 'scalar'-registered
> repository enlistments. To avoid errors when unregistering an enlistment,
> the FSMonitor daemon is explicitly stopped during 'scalar unregister'.
I hadn't looked at this code in a while, so I poked around and
asked some questions that might not even need answering.
Outside of a nit involving a test prereq, this version looks
fine to me.
Thanks,
-Stolee |
|
This patch series was integrated into seen via git@7ecf004. |
|
This patch series was integrated into seen via git@8233cb2. |
|
There was a status update in the "New Topics" section about the branch "scalar" now enables built-in fsmonitor on enlisted repositories, when able. Will merge to 'next'? source: <pull.1324.v2.git.1660694290.gitgitgadget@gmail.com> |
Make the search for repository and enlistment root in 'setup_enlistment_directory()' more constrained to simplify behavior and adhere to 'GIT_CEILING_DIRECTORIES'. Previously, 'setup_enlistment_directory()' would check whether the provided path (or current working directory) '<dir>' or its subdirectory '<dir>/src' was a repository root. If not, the process would repeat on the parent of '<dir>' until the repository was found or it reached the root of the filesystem. This meant that a user could specify a path *anywhere* inside an enlistment (including paths not in the repository contained within the enlistment) and it would be found. The downside to this process is that the search would not account for 'GIT_CEILING_DIRECTORIES', so the upward search could result in modifying repository contents past 'GIT_CEILING_DIRECTORIES'. Similarly, operations like 'scalar delete' could end up unintentionally deleting the parent of a repo if its root was named 'src'. To make this 'setup_enlistment_directory()' both adhere to 'GIT_CEILING_DIRECTORIES' and avoid unwanted deletions, the search for an enlistment directory is simplified to: - if '<dir>/src' is a repository root, '<dir>' is the enlistment root - if '<dir>' is either the repository root or contained within a repository, the repository root is the enlistment root Now, only 'setup_git_directory()' (called by 'setup_enlistment_directory()') searches upwards from the 'scalar' specified path, enforcing 'GIT_CEILING_DIRECTORIES' in the process. Additionally, 'scalar delete <dir>/src' will not delete '<dir>' (if users would like to delete it, they can still specify the enlistment root with 'scalar delete <dir>'). This is true of any 'scalar' operation; users can invoke 'scalar' on the enlistment root, but paths must otherwise be inside the repository to be valid. To help clarify the updated behavior, new tests are added to 't9099-scalar.sh'. Finally, this change leaves 'strbuf_parent_directory()' with only a single, WIN32-specific caller in 'delete_enlistment()'. Rather than wrap 'strbuf_parent_directory()' in '#ifdef WIN32' to avoid the "unused function" compiler error, move the contents of 'strbuf_parent_directory()' into 'delete_enlistment()' and remove the function. Helped-by: Derrick Stolee <derrickstolee@github.com> Signed-off-by: Victoria Dye <vdye@github.com>
When 'scalar unregister' tries to disable maintenance and remove an enlistment, ensure that the return value is nonzero if either operation produces *any* nonzero return value, not just when they return a value less than 0. Signed-off-by: Victoria Dye <vdye@github.com>
When a step in 'register_dir()' or 'unregister_dir()' fails, indicate which step failed with an error message, rather than silently assigning a nonzero return code. Signed-off-by: Victoria Dye <vdye@github.com>
Rather than exiting with 'die()' when 'delete_enlistment()' encounters an
error, return an error code with the appropriate message. There's no need
for an abrupt exit with 'die()' in 'delete_enlistment()' because its only
caller ('cmd_delete()') properly cleans up allocated resources and returns
the 'delete_enlistment()' return value as its own exit code.
Signed-off-by: Victoria Dye <vdye@github.com>
Create function 'set_scalar_config()' to contain the logic used in setting Scalar-defined Git config settings, including how to handle reconfiguring & overwriting existing values. This function allows future patches to set config values in parts of 'scalar.c' other than 'set_recommended_config()'. Signed-off-by: Victoria Dye <vdye@github.com>
Using the built-in FSMonitor makes many common commands quite a bit faster. So let's teach the `scalar register` command to enable the built-in FSMonitor and kick-start the fsmonitor--daemon process (for convenience). For simplicity, we only support the built-in FSMonitor (and no external file system monitor such as e.g. Watchman). Helped-by: Junio C Hamano <gitster@pobox.com> Helped-by: Derrick Stolee <derrickstolee@github.com> Signed-off-by: Matthew John Cheetham <mjcheetham@outlook.com> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> Signed-off-by: Victoria Dye <vdye@github.com>
Especially on Windows, we will need to stop that daemon, just in case that the directory needs to be removed (the daemon would otherwise hold a handle to that directory, preventing it from being deleted). Helped-by: Junio C Hamano <gitster@pobox.com> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> Signed-off-by: Victoria Dye <vdye@github.com>
Update the Scalar roadmap to reflect completion of enabling the built-in FSMonitor in Scalar. Note that implementation of 'scalar help' was moved to the final set of changes to move Scalar out of 'contrib/'. This is due to a dependency on changes to 'git help', as all changes to the main Git tree *exclusively* implemented to support Scalar are part of that series. Signed-off-by: Victoria Dye <vdye@github.com>
|
/submit |
|
Submitted as pull.1324.v3.git.1660858853.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
This patch series was integrated into seen via git@b574446. |
| @@ -14,29 +14,14 @@ | |||
| #include "archive.h" | |||
There was a problem hiding this comment.
On the Git mailing list, Derrick Stolee wrote (reply to this):
On 8/18/2022 5:40 PM, Victoria Dye via GitGitGadget wrote:
> From: Victoria Dye <vdye@github.com>
>
> Make the search for repository and enlistment root in
> 'setup_enlistment_directory()' more constrained to simplify behavior and
> adhere to 'GIT_CEILING_DIRECTORIES'.
Thanks for doing this rather substantial rework. The logic makes
sense to me and the tests really help to demonstrate the different
cases.
-Stolee
| @@ -7,36 +7,24 @@ | |||
| #include "parse-options.h" | |||
There was a problem hiding this comment.
On the Git mailing list, Derrick Stolee wrote (reply to this):
On 8/18/2022 5:40 PM, Matthew John Cheetham via GitGitGadget wrote:
> From: Matthew John Cheetham <mjcheetham@outlook.com>
>
> Using the built-in FSMonitor makes many common commands quite a bit
> faster. So let's teach the `scalar register` command to enable the
> built-in FSMonitor and kick-start the fsmonitor--daemon process (for
> convenience).
> +static int have_fsmonitor_support(void)
> +{
> + return fsmonitor_ipc__is_supported() &&
> + fsm_settings__get_reason(the_repository) == FSMONITOR_REASON_OK;
> +}
The only thing I needed to check was that the_repository was initialized
properly as part of 'scalar clone' and it indeed is.
Thanks,
-Stolee
|
On the Git mailing list, Derrick Stolee wrote (reply to this): On 8/18/2022 5:40 PM, Victoria Dye via GitGitGadget wrote:
> This series enables the built-in FSMonitor [1] on 'scalar'-registered
> repository enlistments. To avoid errors when unregistering an enlistment,
> the FSMonitor daemon is explicitly stopped during 'scalar unregister'.
>
> Maintainer's note: this series has a minor conflict with
> 'vd/scalar-generalize-diagnose'. Please let me know if there's anything else
> I can provide (in addition to [2]) that would make resolution easier.
>
>
> Changes since V2
> ================
>
> * Updated prerequisites for FSMonitor in Scalar to include
> 'fsm_settings__get_reason(the_repository) == FSMONITOR_REASON_OK' to
> handle cases where the platform is supported, but the repository is not.
> * Gated enabling the 'core.fsmonitor' on FSMonitor compatibility with the
> repo.
> * Replaced 'die()' failures in 'delete_enlistment()' with 'error()'s.
> * Replaced 'BUILTIN_FSMONITOR' test prerequisite with already-existing
> 'FSMONITOR_DAEMON' for FSMonitor.
> * Rewrote Scalar enlistment/repo search in 'setup_enlistment_directory()'
> to avoid unconstrained search and respect 'GIT_CEILING_DIRECTORIES'.
> Added tests to show the new expected behavior.
I wrote a couple "thinking out loud" replies, but the series looks
good to me without any changes.
Thanks,
-Stolee
|
|
On the Git mailing list, Junio C Hamano wrote (reply to this): Derrick Stolee <derrickstolee@github.com> writes:
> On 8/18/2022 5:40 PM, Victoria Dye via GitGitGadget wrote:
>> This series enables the built-in FSMonitor [1] on 'scalar'-registered
>> repository enlistments. To avoid errors when unregistering an enlistment,
>> the FSMonitor daemon is explicitly stopped during 'scalar unregister'.
>> ...
> I wrote a couple "thinking out loud" replies, but the series looks
> good to me without any changes.
Thanks, both. Queued.
Let's mark it for 'next' and merge it down soonish. |
|
This patch series was integrated into seen via git@03dd059. |
|
This patch series was integrated into seen via git@525b84b. |
|
This patch series was integrated into next via git@1e172e5. |
|
There was a status update in the "Cooking" section about the branch "scalar" now enables built-in fsmonitor on enlisted repositories, when able. Will merge to 'master'. source: <pull.1324.v3.git.1660858853.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via git@87f7909. |
|
This patch series was integrated into seen via git@0efd595. |
|
This patch series was integrated into seen via git@a775caa. |
|
This patch series was integrated into seen via git@d260543. |
|
There was a status update in the "Cooking" section about the branch "scalar" now enables built-in fsmonitor on enlisted repositories, when able. Will merge to 'master'. source: <pull.1324.v3.git.1660858853.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via git@b028d76. |
|
This patch series was integrated into seen via git@b22cba6. |
|
There was a status update in the "Cooking" section about the branch "scalar" now enables built-in fsmonitor on enlisted repositories, when able. Will merge to 'master'. source: <pull.1324.v3.git.1660858853.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via git@bc820cf. |
|
This patch series was integrated into master via git@bc820cf. |
|
This patch series was integrated into next via git@bc820cf. |
|
Closed via bc820cf. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This series enables the built-in FSMonitor [1] on 'scalar'-registered repository enlistments. To avoid errors when unregistering an enlistment, the FSMonitor daemon is explicitly stopped during 'scalar unregister'.
Maintainer's note: this series has a minor conflict with 'vd/scalar-generalize-diagnose'. Please let me know if there's anything else I can provide (in addition to [2]) that would make resolution easier.
Changes since V2
Changes since V1
<err_msg>" error messages are no longer printed by '(start|stop)_fsmonitor_daemon()'. Instead, "<err_msg>" is printed to stderr by swapping 'pipe_command()' out for 'run_git()', and '[un]register_dir()' prints the "could not (start|stop) the FSMonitor daemon" message.Thanks
[1] https://lore.kernel.org/git/pull.1143.git.1644940773.gitgitgadget@gmail.com/
[2] The conflict is a result of both series updating the Scalar roadmap doc. For reference, my merge resolution (from
git diff <merge commit> <merge commit>^1 <merge commit>^2, where<merge commit>^1is 'vd/scalar-generalize-diagnose' and<merge commit>^2is this series) looks like:CC: johannes.schindelin@gmx.de
CC: mjcheetham@outlook.com
CC: gitster@pobox.com
cc: Derrick Stolee derrickstolee@github.com