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

[Jeff Hostetler <git@jeffhostetler.com>]

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