Subject: Re: [PATCH v4 1/4] fsmonitor: add two new config options, allowRemote and socketDir

From: Ævar Arnfjörð Bjarmason <[email-masked]>
Date: Wed, 31 Aug 2022 21:41:28 +0200

On Wed, Aug 31 2022, Eric DeCosta via GitGitGadget wrote:

> From: Eric DeCosta <[email-masked]>
> [...]
>  	enum fsmonitor_reason reason;
> +	int allow_remote;
>  	char *hook_path;
> +	char *sock_dir;
>  };

Any reason we couldn't just add this to "struct repo_settings" and ...

> +int fsm_settings__get_allow_remote(struct repository *r)
> +{
> +	if (!r)
> +		r = the_repository;
> +	if (!r->settings.fsmonitor)
> +		lookup_fsmonitor_settings(r);
> +
> +	return r->settings.fsmonitor->allow_remote;
> +}
> +
> +const char *fsm_settings__get_socket_dir(struct repository *r)
> +{
> +	if (!r)
> +		r = the_repository;
> +	if (!r->settings.fsmonitor)
> +		lookup_fsmonitor_settings(r);
> +
> +	return r->settings.fsmonitor->sock_dir;
> +}
> +

...instead of this whole ceremony...

> +void fsm_settings__set_allow_remote(struct repository *r)
> +{
> +	int allow;
> +
> +	if (!r)
> +		r = the_repository;
> +	if (!r->settings.fsmonitor)
> +		r->settings.fsmonitor = alloc_settings();
> +	if (!repo_config_get_bool(r, "fsmonitor.allowremote", &allow))
> +		r->settings.fsmonitor->allow_remote = allow;
> +
> +	return;
> +}

Just have a single repo_cfg_bool() line in prepare_repo_settings()
instead?

(There are some reasons for the "lazy" behavior of fsmonitor-settings.c,
but surely a simple boolean variable we can read on startup isn't it,
and we already paid the cost to do so with the configset...)

> +void fsm_settings__set_socket_dir(struct repository *r)
> +{
> +	const char *path;
> +
> +	if (!r)
> +		r = the_repository;
> +	if (!r->settings.fsmonitor)
> +		r->settings.fsmonitor = alloc_settings();
> +
> +	if (!repo_config_get_pathname(r, "fsmonitor.socketdir", &path)) {
> +		FREE_AND_NULL(r->settings.fsmonitor->sock_dir);

...maybe this socket dir stuff is the exception though.

> +		r->settings.fsmonitor->sock_dir = strdup(path);

Aren't you strdup()-ing an already strdup()'d string, and therefore
leaking memory? Also this should be xstrdup(), surely?

> +	}
> +
> +	return;
> +}
> +
>  void fsm_settings__set_ipc(struct repository *r)
>  {
> -	enum fsmonitor_reason reason = check_for_incompatible(r);
> +	enum fsmonitor_reason reason;
> +
> +	fsm_settings__set_allow_remote(r);
> +	fsm_settings__set_socket_dir(r);
> +	reason = check_for_incompatible(r);

This seems rather backwards, as odd as this API itself is already, isn't
the whole idea that after we call lookup_fsmonitor_settings() it will
have set() anything that's mandatory?

But maybe I haven't grokked it ... :)