| Message ID | pull.1041.v3.git.1634157107.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Builtin FSMonitor Part 2 | expand |
On Wed, Oct 13 2021, Jeff Hostetler via GitGitGadget wrote: > Here is V3 of Part 2 of my Builtin FSMonitor series. Like V2, it is built > upon "next" because it requires "ab/repo-settings-cleanup" and > "jh/builtin-fsmonitor-part1" series. FYI: Both of those have landed, so a next iteration of this can be built on "master". > V3 removes the explicit initialization of r->repo_settings->fsmonitor in > repo-settings.c as requested. It also includes a more detailed commit > message for the 3 commit to explain the rationale for putting fsmonitor > settings in its own source file rather than adding it repo-settings.c This series breaks with the "make check-docs" target, which as an aside is broken because of recent changes of mine, so CI didn't catch this (I'll submit a series to fix it): config/core.txt:95: error: git-fsmonitor--daemon[1]: link outside of our own docs, shown with 'HERE' below: config/core.txt:95: 'daemon for this working directory (linkgit:git-fsmonitor--daemon[1]' <-- HERE git-update-index.txt:502: error: git-fsmonitor--daemon[1]: link outside of our own docs, shown with 'HERE' below: git-update-index.txt:502: 'linkgit:git-fsmonitor--daemon[1]' <-- HERE But that broken-ness points to a more general issue, which is that it's not just a broken link, but that docs in this series are referring to a manpage that doesn't exist yet. I was going to check out some of the semantics of core.useBuiltinfsMonitor that I commented on in earlier rounds, but I see that there's no tests for it, probably for similar reasons as there not being a git-fsmonitor--daemon yet: It's all (hopefully) in some yet-to-come series. I'm all for this being split up in steps from the 30-something patches it was before, but if we're making forward-references to docs that don't exist yet, adding ~100 line *.c files that seemingly aren't used at all yet etc., the split-up seems to be a bit too aggressive. E.g. the below diff seems to have all tests passing, so core.usebuiltinfsmonitor is unused still? There's also large amounts of code ifdef'd away under HAVE_FSMONITOR_DAEMON_BACKEND, but no other in-tree reference to it? AFAICT that's going to be used in the future. So seemingly something that'll only be used in the series after this? I.e. this seems to have stopped at around part 7/28 of a previous submission, but digging in the archive e.g. HAVE_FSMONITOR_DAEMON_BACKEND semes to be first used in step 11/28 of that[1]. Sorry to be party pooper again, I really did try to review this, but just found that I couldn't, since so much of it seems to be a forward-reference to some future state. Isn't there a way to re-arrange this so that the "teach daemon XYZ command" around steps 13-14/28 of your previous series comes before references in the docs to the daemon that doesn't exist yet (5/5 here), or there's perhaps some of the config scaffolding, but e.g. "core.usebuiltinfsmonitor" comes along with the later change that uses it? Or just make this "part 2" series larger than 5 patches, so that it manages to tell some holistic story, and the various bits are used at the end? Even so it would be a lot easier to follow if e.g. a patch using a new "core.usebuiltinfsmonitor" variable doesn't come at say step 11/20, with that variable having been added at a 3/20. I.e. maybe 3/20 can have the scaffolding, but the part that adds the "core.usebuiltinfsmonitor" support either immediately precedes its use, or is squashed into the relevant commit that needs it. Ditto docs for so-far-not-existing things etc. Thanks! And hopefully this helps. 1. https://lore.kernel.org/git/49f9e2e3d49ce6e7b56839bf44535f271216abeb.1621691828.git.gitgitgadget@gmail.com/ diff --git a/fsmonitor-settings.c b/fsmonitor-settings.c index 2770266f5ee..2c479d4f36b 100644 --- a/fsmonitor-settings.c +++ b/fsmonitor-settings.c @@ -35,19 +35,6 @@ void fsm_settings__set_disabled(struct repository *r) FREE_AND_NULL(s->hook_path); } -static int check_for_ipc(struct repository *r) -{ - int value; - - if (!repo_config_get_bool(r, "core.usebuiltinfsmonitor", &value) && - value) { - fsm_settings__set_ipc(r); - return 1; - } - - return 0; -} - static int check_for_hook(struct repository *r) { const char *const_str; @@ -71,9 +58,6 @@ static void lookup_fsmonitor_settings(struct repository *r) r->settings.fsmonitor = s; - if (check_for_ipc(r)) - return; - if (check_for_hook(r)) return;
On 10/15/21 8:03 AM, Ævar Arnfjörð Bjarmason wrote: > > On Wed, Oct 13 2021, Jeff Hostetler via GitGitGadget wrote: > >> Here is V3 of Part 2 of my Builtin FSMonitor series. Like V2, it is built >> upon "next" because it requires "ab/repo-settings-cleanup" and >> "jh/builtin-fsmonitor-part1" series. > > FYI: Both of those have landed, so a next iteration of this can be built > on "master". > >> V3 removes the explicit initialization of r->repo_settings->fsmonitor in >> repo-settings.c as requested. It also includes a more detailed commit >> message for the 3 commit to explain the rationale for putting fsmonitor >> settings in its own source file rather than adding it repo-settings.c > > This series breaks with the "make check-docs" target, which as an aside > is broken because of recent changes of mine, so CI didn't catch this > (I'll submit a series to fix it): > > config/core.txt:95: error: git-fsmonitor--daemon[1]: link outside of our own docs, shown with 'HERE' below: > config/core.txt:95: 'daemon for this working directory (linkgit:git-fsmonitor--daemon[1]' <-- HERE > git-update-index.txt:502: error: git-fsmonitor--daemon[1]: link outside of our own docs, shown with 'HERE' below: > git-update-index.txt:502: 'linkgit:git-fsmonitor--daemon[1]' <-- HERE > > But that broken-ness points to a more general issue, which is that it's > not just a broken link, but that docs in this series are referring to a > manpage that doesn't exist yet. > > I was going to check out some of the semantics of > core.useBuiltinfsMonitor that I commented on in earlier rounds, but I > see that there's no tests for it, probably for similar reasons as there > not being a git-fsmonitor--daemon yet: It's all (hopefully) in some > yet-to-come series. > > I'm all for this being split up in steps from the 30-something patches > it was before, but if we're making forward-references to docs that don't > exist yet, adding ~100 line *.c files that seemingly aren't used at all > yet etc., the split-up seems to be a bit too aggressive. > > E.g. the below diff seems to have all tests passing, so > core.usebuiltinfsmonitor is unused still? There's also large amounts of > code ifdef'd away under HAVE_FSMONITOR_DAEMON_BACKEND, but no other > in-tree reference to it? > > AFAICT that's going to be used in the future. So seemingly something > that'll only be used in the series after this? I.e. this seems to have > stopped at around part 7/28 of a previous submission, but digging in the > archive e.g. HAVE_FSMONITOR_DAEMON_BACKEND semes to be first used in > step 11/28 of that[1]. > > Sorry to be party pooper again, I really did try to review this, but > just found that I couldn't, since so much of it seems to be a > forward-reference to some future state. > > Isn't there a way to re-arrange this so that the "teach daemon XYZ > command" around steps 13-14/28 of your previous series comes before > references in the docs to the daemon that doesn't exist yet (5/5 here), > or there's perhaps some of the config scaffolding, but > e.g. "core.usebuiltinfsmonitor" comes along with the later change that > uses it? > > Or just make this "part 2" series larger than 5 patches, so that it > manages to tell some holistic story, and the various bits are used at > the end? > > Even so it would be a lot easier to follow if e.g. a patch using a new > "core.usebuiltinfsmonitor" variable doesn't come at say step 11/20, with > that variable having been added at a 3/20. I.e. maybe 3/20 can have the > scaffolding, but the part that adds the "core.usebuiltinfsmonitor" > support either immediately precedes its use, or is squashed into the > relevant commit that needs it. Ditto docs for so-far-not-existing things > etc. > > Thanks! And hopefully this helps. > > 1. https://lore.kernel.org/git/49f9e2e3d49ce6e7b56839bf44535f271216abeb.1621691828.git.gitgitgadget@gmail.com/ > > diff --git a/fsmonitor-settings.c b/fsmonitor-settings.c > index 2770266f5ee..2c479d4f36b 100644 > --- a/fsmonitor-settings.c > +++ b/fsmonitor-settings.c > @@ -35,19 +35,6 @@ void fsm_settings__set_disabled(struct repository *r) > FREE_AND_NULL(s->hook_path); > } > > -static int check_for_ipc(struct repository *r) > -{ > - int value; > - > - if (!repo_config_get_bool(r, "core.usebuiltinfsmonitor", &value) && > - value) { > - fsm_settings__set_ipc(r); > - return 1; > - } > - > - return 0; > -} > - > static int check_for_hook(struct repository *r) > { > const char *const_str; > @@ -71,9 +58,6 @@ static void lookup_fsmonitor_settings(struct repository *r) > > r->settings.fsmonitor = s; > > - if (check_for_ipc(r)) > - return; > - > if (check_for_hook(r)) > return; > > Because of the size -- I'm up to ~60 commits now -- I was trying to break it up into logical pieces. And yes, there's a bit of a chicken-n-egg problem with introducing pieces. I was trying to make Part 2 be the client side -- everything needed to talk to a daemon (even if it doesn't exist yet) and then have Part 3 be the daemon proper (and build it up without worrying about when the corresponding client peer code appears because the client side is already present). The daemon in Part 3 would be a little rough around the edges, but sufficient to do end-to-end testing with both t/ and t/perf tests. Then Part 4 would clean up some of the details and add more tests. That was the idea anyway. I'll take a stab at refactoring this and send a new Part 2 that contains both the client and MVP daemon parts. This will let let basic end-to-end testing happen, fix the doc breakage, and hopefully make it easier to see the big picture in a single patch series. This should be about 30 commits. Once that has settled a bit, I'll followup with a new Part 3 that contains the refinements and additional tests. Thanks, Jeff
Here is V3 of Part 2 of my Builtin FSMonitor series. Like V2, it is built upon "next" because it requires "ab/repo-settings-cleanup" and "jh/builtin-fsmonitor-part1" series. V3 removes the explicit initialization of r->repo_settings->fsmonitor in repo-settings.c as requested. It also includes a more detailed commit message for the 3 commit to explain the rationale for putting fsmonitor settings in its own source file rather than adding it repo-settings.c There was a comment on the V2 series about integrating the fsmonitor hook path with the on-going "hook config" effort that I did not address. I think it would be best to do that in a follow-on if it makes sense to do so. cc: Bagas Sanjaya bagasdotme@gmail.com cc: Jeff Hostetler git@jeffhostetler.com Jeff Hostetler (5): fsmonitor: enhance existing comments fsmonitor-ipc: create client routines for git-fsmonitor--daemon fsmonitor: config settings are repository-specific fsmonitor: use IPC to query the builtin FSMonitor daemon fsmonitor: update fsmonitor config documentation Documentation/config/core.txt | 56 ++++++--- Documentation/git-update-index.txt | 27 +++-- Documentation/githooks.txt | 3 +- Makefile | 2 + builtin/update-index.c | 19 +++- cache.h | 1 - config.c | 14 --- config.h | 1 - environment.c | 1 - fsmonitor-ipc.c | 176 +++++++++++++++++++++++++++++ fsmonitor-ipc.h | 48 ++++++++ fsmonitor-settings.c | 97 ++++++++++++++++ fsmonitor-settings.h | 21 ++++ fsmonitor.c | 130 +++++++++++++++------ fsmonitor.h | 18 ++- repository.h | 3 + t/README | 4 +- 17 files changed, 536 insertions(+), 85 deletions(-) create mode 100644 fsmonitor-ipc.c create mode 100644 fsmonitor-ipc.h create mode 100644 fsmonitor-settings.c create mode 100644 fsmonitor-settings.h base-commit: 6e70778dc91e2139466c15ff15a02a22a2ada2d1 Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1041%2Fjeffhostetler%2Fbuiltin-fsmonitor-part2-v3 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1041/jeffhostetler/builtin-fsmonitor-part2-v3 Pull-Request: https://github.com/gitgitgadget/git/pull/1041 Range-diff vs v2: 1: cb25eeaf72d = 1: cb25eeaf72d fsmonitor: enhance existing comments 2: df81a63acee = 2: df81a63acee fsmonitor-ipc: create client routines for git-fsmonitor--daemon 3: 7d5a353e74d ! 3: a1d606aa622 fsmonitor: config settings are repository-specific @@ Metadata ## Commit message ## fsmonitor: config settings are repository-specific - Move FSMonitor config settings to a new `struct fsmonitor_settings` - structure. Add a lazily-loaded pointer to `struct repo_settings`. + Move fsmonitor config settings to a new and opaque + `struct fsmonitor_settings` structure. Add a lazily-loaded pointer + to this into `struct repo_settings` + + Create an `enum fsmonitor_mode` type in `struct fsmonitor_settings` to + represent the state of fsmonitor. This lets us represent which, if + any, fsmonitor provider (hook or IPC) is enabled. + Create `fsm_settings__get_*()` getters to lazily look up fsmonitor- related config settings. - Get rid of the `core_fsmonitor` global variable, and add support for - the new `core.useBuiltinFSMonitor` config setting. Move config code - to lookup the existing `core.fsmonitor` value to `fsmonitor-settings.[ch]`. + Add support for the new `core.useBuiltinFSMonitor` config setting. + + Get rid of the `core_fsmonitor` global variable. Move the code to + lookup the existing `core.fsmonitor` config value into the fsmonitor + settings. + + Create a hook pathname variable in `struct fsmonitor-settings` and + only set it when in hook mode. - The `core_fsmonitor` global variable was used to store the pathname to - the FSMonitor hook and it was used as a boolean to see if FSMonitor - was enabled. This dual usage will lead to confusion when we add - support for a builtin FSMonitor based on IPC, since the builtin - FSMonitor doesn't need the hook pathname. + The existing `core_fsmonitor` global variable was used to store the + pathname to the fsmonitor hook *and* it was used as a boolean to see + if fsmonitor was enabled. This dual usage and global visibility leads + to confusion when we add the IPC-based provider. So lets hide the + details in fsmonitor-settings.c and let it decide which provider to + use in the case of multiple settings. This avoids cluttering up + repo-settings.c with these private details. - Replace the boolean usage with an `enum fsmonitor_mode` to represent - the state of FSMonitor. And only set the pathname when in HOOK mode. + A future commit in builtin-fsmonitor series will add the ability to + disqualify worktrees for various reasons, such as being mounted from a + remote volume, where fsmonitor should not be started. Having the + config settings hidden in fsmonitor-settings.c allows such worktree + restrictions to override the config values used. Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com> @@ fsmonitor.h: static inline void mark_fsmonitor_valid(struct index_state *istate, untracked_cache_invalidate_path(istate, ce->name, 1); trace_printf_key(&trace_fsmonitor, "mark_fsmonitor_invalid '%s'", ce->name); - ## repo-settings.c ## -@@ repo-settings.c: void prepare_repo_settings(struct repository *r) - if (r->settings.initialized++) - return; - -+ r->settings.fsmonitor = NULL; /* lazy loaded */ -+ - /* Defaults */ - r->settings.index_version = -1; - r->settings.core_untracked_cache = UNTRACKED_CACHE_KEEP; - ## repository.h ## @@ #include "path.h" 4: 8608c8718d8 = 4: 4d8d812be08 fsmonitor: use IPC to query the builtin FSMonitor daemon 5: 7c22ce53377 = 5: 45a86cef8d7 fsmonitor: update fsmonitor config documentation