From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Fri, 11 Mar 2022 01:47:09 GMT
Subject: Re: [PATCH v6 03/30] fsmonitor: config settings are repository-specific
Message-ID: <220311.86fsnpi73l.gmgdl@evledraar.gmail.com>
In-Reply-To: <ae622a517cf500d5b068871434e33f98e9cf586b.1646160212.git.gitgitgadget@gmail.com>


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?

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