From: Jeff Hostetler <git@jeffhostetler.com>
Date: Fri, 11 Mar 2022 14:07:47 GMT
Subject: Re: [PATCH v6 03/30] fsmonitor: config settings are repository-specific
Message-ID: <dd9a1847-7163-49e6-4a8b-bc5c8c381f0d@jeffhostetler.com>
In-Reply-To: <220311.86fsnpi73l.gmgdl@evledraar.gmail.com>



On 3/10/22 8:47 PM, Ævar Arnfjörð Bjarmason wrote:
> 
> On Tue, Mar 01 2022, Jeff Hostetler via GitGitGadget wrote:
> 
>> From: Jeff Hostetler <jeffhost@microsoft.com>
> 
> I know this is in "next", just looking over this code again...
> 
>> +static void lookup_fsmonitor_settings(struct repository *r)
> 
> Here we'll start loading the settings...
> 
>> +{
>> +	struct fsmonitor_settings *s;
>> +	const char *const_str;
>> +	int bool_value;
>> +
>> +	if (r->settings.fsmonitor)
>> +		return;
> 
> MARK
> 
>> +	CALLOC_ARRAY(s, 1);
>> +
>> +	r->settings.fsmonitor = s;
> 
> And right after we alloc the r->settings.fsmonitor we'll ...
> 
>> +	fsm_settings__set_disabled(r);
> 
> ...call this method...
>> +
>> +	/*
>> +	 * Overload the existing "core.fsmonitor" config setting (which
>> +	 * has historically been either unset or a hook pathname) to
>> +	 * now allow a boolean value to enable the builtin FSMonitor
>> +	 * or to turn everything off.  (This does imply that you can't
>> +	 * use a hook script named "true" or "false", but that's OK.)
>> +	 */
>> +	switch (repo_config_get_maybe_bool(r, "core.fsmonitor", &bool_value)) {
>> +
>> +	case 0: /* config value was set to <bool> */
>> +		if (bool_value)
>> +			fsm_settings__set_ipc(r);
>> +		return;
>> +
>> +	case 1: /* config value was unset */
>> +		const_str = getenv("GIT_TEST_FSMONITOR");
>> +		break;
>> +
>> +	case -1: /* config value set to an arbitrary string */
>> +		if (repo_config_get_pathname(r, "core.fsmonitor", &const_str))
>> +			return; /* should not happen */
>> +		break;
>> +
>> +	default: /* should not happen */
>> +		return;
>> +	}
>> +
>> +	if (!const_str || !*const_str)
>> +		return;
>> +
>> +	fsm_settings__set_hook(r, const_str);
>> +}
>> [...]
>> +void fsm_settings__set_disabled(struct repository *r)
>> +{
>> +	if (!r)
>> +		r = the_repository;
>> +
>> +	lookup_fsmonitor_settings(r);
> 
> ...which here will recurse into lookup_fsmonitor_settings and hit
> "MARK".
> 
> So isn't that fsm_settings__set_disabled() within that method pointless?

Yeah, that got rolled up in one too many rebase/refactorings...
I'll simplify.

thanks
jeff

> 
>> +	r->settings.fsmonitor->mode = FSMONITOR_MODE_DISABLED;
>> +	FREE_AND_NULL(r->settings.fsmonitor->hook_path);
> 
> It seems as though the intent was to reach this, but these all happen to
> be the same thing you'd get with CALLOC_ARRAY(), so I think this just
> happened to work out...
> 
>> +enum fsmonitor_mode {
>> +	FSMONITOR_MODE_DISABLED = 0,
> 
> ...I.e. this is luckily zero.
>