Subject: Re: [PATCH v2 3/5] fsmonitor: config settings are repository-specific

From: Ævar Arnfjörð Bjarmason <[email-masked]>
Date: Thu, 07 Oct 2021 18:59:07 +0200

On Thu, Oct 07 2021, Jeff Hostetler via GitGitGadget wrote:

Good to see this move forward!

This bit:

> --- a/repo-settings.c
> +++ b/repo-settings.c
> @@ -20,6 +20,8 @@ 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;
> diff --git a/repository.h b/repository.h

Is carried forward from v1, but with 3050b6dfc75 (repo-settings.c:
simplify the setup, 2021-09-21) isn't needed. It's init'd to 0/NULL
already, but was -1 before.

So this hunk can go, and its presence makes for confusing reading
without that history, is it set before somehow? No, just working around
older code that's no longer there.

But also: For untracked_cache_setting and fetch_negotiation_setting
we've got an embedded enum in the struct, but this...

> index a057653981c..89a1873ade7 100644
> --- a/repository.h
> +++ b/repository.h
> @@ -4,6 +4,7 @@
>  #include "path.h"
>  
>  struct config_set;
> +struct fsmonitor_settings;
>  struct git_hash_algo;
>  struct index_state;
>  struct lock_file;
> @@ -34,6 +35,8 @@ struct repo_settings {
>  	int command_requires_full_index;
>  	int sparse_index;
>  
> +	struct fsmonitor_settings *fsmonitor; /* lazy loaded */
> +
>  	int index_version;
>  	enum untracked_cache_setting core_untracked_cache;
>  

Is a pointer to a struct that has an "enum fsmonitor_mode mode", and the
code in fsmonitor-settings.c seems to be repeating the patterns we had
in repo-settings.c pre-3050b6dfc75, e.g. checking whether a bool config
variable exists *and* is true, v.s. checking if it exists (presumably an
explicit false wants to override something).

I haven't looked carefully, but between that & the "char *hook_path"
being something that'll need to be made to use Emily's hook config
series sooner than later, can't we read/setup the initial config in
"repo_cfg_bool"?

The relevant commit message just says:

    Move FSMonitor config settings to a new `struct fsmonitor_settings`
    structure.  Add a lazily-loaded pointer to `struct repo_settings`.
    Create `fsm_settings__get_*()` getters to lazily look up fsmonitor-
    related config settings.[...]

Which I think can be paraphrased as "Add scaffolding to repo-settings.c
but do config loading differently than everything there (lazily),
because...", except the "because" is missing :)