"Jeff Hostetler via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes:
> if (fsmonitor > 0) {
> - if (git_config_get_fsmonitor() == 0)
> + enum fsmonitor_mode fsm_mode = fsm_settings__get_mode(r);
> +
> + if (fsm_mode == FSMONITOR_MODE_DISABLED) {
> + warning(_("core.useBuiltinFSMonitor is unset; "
> + "set it if you really want to enable the "
> + "builtin fsmonitor"));
> warning(_("core.fsmonitor is unset; "
> - "set it if you really want to "
> - "enable fsmonitor"));
> + "set it if you really want to enable the "
> + "hook-based fsmonitor"));
> + }
> add_fsmonitor(&the_index);
> report(_("fsmonitor enabled"));
> } else if (!fsmonitor) {
> - if (git_config_get_fsmonitor() == 1)
> + enum fsmonitor_mode fsm_mode = fsm_settings__get_mode(r);
> + if (fsm_mode == FSMONITOR_MODE_IPC)
> + warning(_("core.useBuiltinFSMonitor is set; "
> + "remove it if you really want to "
> + "disable fsmonitor"));
> + if (fsm_mode == FSMONITOR_MODE_HOOK)
> warning(_("core.fsmonitor is set; "
> "remove it if you really want to "
> "disable fsmonitor"));
Hmph. This does not change the behaviour per-se, but what are we
trying to achieve with these warning messages?
The user uses --fsmonitor or --no-fsmonitor option from the command
line, presumably as a one-shot "this time I'd operate the command
differently from the configured way", so it seems unlikely that the
user is doing so because "... really want to enable/disable". The
"report()" calls in these if/else cases seem sufficient reminder of
what is going on---perhaps these warnings should be made silenceable
by turning them into advice messages?
> -int git_config_get_fsmonitor(void)
> -{
> - if (git_config_get_pathname("core.fsmonitor", &core_fsmonitor))
> - core_fsmonitor = getenv("GIT_TEST_FSMONITOR");
> -
> - if (core_fsmonitor && !*core_fsmonitor)
> - core_fsmonitor = NULL;
> -
> - if (core_fsmonitor)
> - return 1;
> -
> - return 0;
> -}
This used to be how we got the configuration.
> --- a/config.h
> +++ b/config.h
> @@ -610,7 +610,6 @@ int git_config_get_pathname(const char *key, const char **dest);
> int git_config_get_index_threads(int *dest);
> int git_config_get_split_index(void);
> int git_config_get_max_percent_split_change(void);
> -int git_config_get_fsmonitor(void);
And that is removed so any in-flight topic that adds new caller will
be caught by the compiler. OK.
> diff --git a/environment.c b/environment.c
> index 9da7f3c1a19..68f90632245 100644
> --- a/environment.c
> +++ b/environment.c
> @@ -82,7 +82,6 @@ int protect_hfs = PROTECT_HFS_DEFAULT;
> #define PROTECT_NTFS_DEFAULT 1
> #endif
> int protect_ntfs = PROTECT_NTFS_DEFAULT;
> -const char *core_fsmonitor;
So is this.
All nice.
> +static void lookup_fsmonitor_settings(struct repository *r)
> +{
> + struct fsmonitor_settings *s;
> +
> + CALLOC_ARRAY(s, 1);
> +
> + r->settings.fsmonitor = s;
> +
> + if (check_for_ipc(r))
> + return;
> +
> + if (check_for_hook(r))
> + return;
> +
> + fsm_settings__set_disabled(r);
> +}
> +
> +enum fsmonitor_mode fsm_settings__get_mode(struct repository *r)
> +{
> + if (!r->settings.fsmonitor)
> + lookup_fsmonitor_settings(r);
OK, and these "lookup" calls are what make this field "lazily
loaded". A helper
static inline void lazily_load_fsmonitor_settings(struct repository *r)
{
if (!r->settings.fsmonitor)
lookup_fsmonitor_settings(r);
}
might be handy. Also an assert to ensure nobody calls lookup() on a
repository that already has lazily loaded the settings would be
necessary.
static void lookup_fsmonitor_settings(struct repository *r)
{
if (r->settings.fsmonitor)
BUG("...");
CALLOC_ARRAY(r->settings.fsmonitor, 1);
> +enum fsmonitor_mode {
> + FSMONITOR_MODE_DISABLED = 0,
> + FSMONITOR_MODE_HOOK = 1, /* core.fsmonitor */
> + FSMONITOR_MODE_IPC = 2, /* core.useBuiltinFSMonitor */
> +};
Please remind me why we need a new separate variable, instead of
turning the core.fsmonitor variable into an extended bool <false,
true, builtin>? The compatibility issues during transition is the
same either way. Old clients will ignore the request silently when
you set core.useBuiltinFSMonitor, or they will barf if you set
core.fsmonitor to 'builtin', so in a sense, extending the existing
variable may be a safer option.
> diff --git a/repository.h b/repository.h
> 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 */
"lazily" loaded, I think.
> GIT_TEST_FSMONITOR=$PWD/t7519/fsmonitor-all exercises the fsmonitor
> -code path for utilizing a file system monitor to speed up detecting
> -new or changed files.
> +code path for utilizing a (hook based) file system monitor to speed up
> +detecting new or changed files.
Nice attention to the detail here.
Thanks.
