Re: [PATCH 04/23] fsmonitor: introduce `core.useBuiltinFSMonitor` to call the daemon via IPC
- From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
- Date
- Apr 27, 2021, 09:20 UTC
- Message-ID
- <87mttkyrqq.fsf@evledraar.gmail.com>
- In-Reply-To
- <f77457d0-925d-2ea1-4ad4-b3315532b37f@gmail.com>
On Mon, Apr 26 2021, Derrick Stolee wrote:
Show 70 quoted lines
> On 4/1/21 11:40 AM, Johannes Schindelin via GitGitGadget wrote:> @@ -2515,6 +2515,11 @@ int git_config_get_max_percent_split_change(void)
>>
>> int repo_config_get_fsmonitor(struct repository *r)
>> {
>> + if (r->settings.use_builtin_fsmonitor > 0) {
>
> Don't forget to run prepare_repo_settings(r) first.
>
>> + core_fsmonitor = "(built-in daemon)";
>> + return 1;
>> + }
>> +
>
> I found this odd, assigning a string to core_fsmonitor that
> would definitely cause a problem trying to execute it as a
> hook. I wondered the need for it at all, but found that
> there are several places in the FS Monitor subsystem that use
> core_fsmonitor as if it was a boolean, indicating whether or
> not the feature is enabled at all.
>
> A cleaner way to handle this would be to hide the data behind
> a helper method, say "fsmonitor_enabled()" that could then
> check a value on the repository (or index) and store the hook
> value as a separate value that is only used by the hook-based
> implementation.
>
> It's probably a good idea to do that cleanup now, before we
> find on accident that we missed a gap and start trying to run
> this bogus string as a hook invocation.
>> -static int query_fsmonitor(int version, const char *last_update, struct strbuf *query_result)
>> +static int query_fsmonitor(int version, struct index_state *istate, struct strbuf *query_result)
>> {
>> + struct repository *r = istate->repo ? istate->repo : the_repository;
>> + const char *last_update = istate->fsmonitor_last_update;
>> struct child_process cp = CHILD_PROCESS_INIT;
>> int result;
>>
>> if (!core_fsmonitor)
>> return -1;
>
> Here is an example of it being used as a boolean.
>
>> + if (r->settings.use_builtin_fsmonitor > 0) {
>> +#ifdef HAVE_FSMONITOR_DAEMON_BACKEND
>> + return fsmonitor_ipc__send_query(last_update, query_result);
>> +#else
>> + /* Fake a trivial response. */
>> + warning(_("fsmonitor--daemon unavailable; falling back"));
>> + strbuf_add(query_result, "/", 2);
>> + return 0;
>> +#endif
>
> This seems like a case where the helper fsmonitor_ipc__is_supported()
> could be used instead of compile-time macros.
>
> (I think this is especially true when we consider the future of the
> feature on Linux and the possibility of the same compiled code needing
> to check run-time properties of the platform for compatibility.)
>
>> --- a/repo-settings.c
>> +++ b/repo-settings.c
>> @@ -58,6 +58,9 @@ void prepare_repo_settings(struct repository *r)
>> r->settings.core_multi_pack_index = value;
>> UPDATE_DEFAULT_BOOL(r->settings.core_multi_pack_index, 1);
>>
>> + if (!repo_config_get_bool(r, "core.usebuiltinfsmonitor", &value) && value)
>> + r->settings.use_builtin_fsmonitor = 1;
>> +
>
> Follows the patterns of repo settings. Good.It follows the pattern, but as an aside the pattern seems bit odd. I see it dates back to your 7211b9e7534 (repo-settings: consolidate some config settings, 2019-08-13).
I.e. we memset() the whole thing to -1, then for most things do something like:
if (!repo_config_get_bool(r, "gc.writecommitgraph", &value))
r->settings.gc_write_commit_graph = value;
UPDATE_DEFAULT_BOOL(r->settings.gc_write_commit_graph, 1);But could do:
if (repo_config_get_bool(r, "gc.writecommitgraph", &r->settings.gc_write_commit_graph))
r->settings.gc_write_commit_graph = 1;No? I.e. the repo_config_get_bool() function already returns non-zero if we don't find it in the config.
I see the UPDATE_DEFAULT_BOOL() macro has also drifted from "set thing default boolean" to "set any default value".