On Wed, Aug 31 2022, Eric DeCosta via GitGitGadget wrote:
> From: Eric DeCosta <edecosta@xxxxxxxxxxxxx>
> [...]
> 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 ... :)
