Re: [PATCH v4 1/4] fsmonitor: add two new config options, allowRemote and socketDir
- From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
- Date
- Aug 31, 2022, 19:41 UTC
- Message-ID
- <220831.86h71s8ad7.gmgdl@evledraar.gmail.com>
- In-Reply-To
- <836a791e6b7fd4490674254ce03105a8ca2175cb.1661962145.git.gitgitgadget@gmail.com>
On Wed, Aug 31 2022, Eric DeCosta via GitGitGadget wrote:
Show 7 quoted lines
> From: Eric DeCosta <edecosta@mathworks.com> > [...] > 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 ...
Show 20 quoted lines
> +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...
Show 13 quoted lines
> +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...)
Show 11 quoted lines
> +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?
Show 13 quoted lines
> + }
> +
> + 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 ... :)