Re: [PATCH v4 03/29] fsmonitor: config settings are repository-specific

[Jeff Hostetler <git@jeffhostetler.com>]

On 10/21/21 5:05 PM, Junio C Hamano wrote:
> "Jeff Hostetler via GitGitGadget" <gitgitgadget@gmail.com> 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?
>

The original code had the basic warning for `core.fsmonitor`
which becomes ambiguous/confusing when we have two different
config values.

I was really wanting to just get rid of the warnings.  They
only appear if the users passes a `--[no-]fsmonitor` on the
command line to temporarily override the config setting.

I'll see about making them 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);

good point.


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