Re: [PATCH v3 0/5] Builtin FSMonitor Part 2

On Wed, Oct 13 2021, Jeff Hostetler via GitGitGadget wrote:

quoted

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".

quoted

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/ (local)

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;