Re: [PATCH v4 04/27] fsmonitor-settings: bare repos are incompatible with FSMonitor
- From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
- Date
- Apr 19, 2022, 09:44 UTC
- Message-ID
- <220419.86mtgh8kep.gmgdl@evledraar.gmail.com>
- In-Reply-To
- <f2c0569c9012a86f252562a9a906f6de37d0a236.1648140680.git.gitgitgadget@gmail.com>
On Thu, Mar 24 2022, Jeff Hostetler via GitGitGadget wrote:
Show 7 quoted lines
> diff --git a/builtin/fsmonitor--daemon.c b/builtin/fsmonitor--daemon.c
> index 46be55a4618..50ae3cca575 100644
> --- a/builtin/fsmonitor--daemon.c
> +++ b/builtin/fsmonitor--daemon.c
> @@ -1449,6 +1449,12 @@ int cmd_fsmonitor__daemon(int argc, const char **argv, const char *prefix)
> die(_("invalid 'ipc-threads' value (%d)"),
> fsmonitor__ipc_threads);I think that structurally the way things are done in fsmonitor-settings.c make its use really hard to follow. E.g. here:
> + prepare_repo_settings(the_repository);
We prep the repo, OK.
> + fsm_settings__set_ipc(the_repository);
Set IPC.
> + if (fsm_settings__error_if_incompatible(the_repository))
And here we'll error out if we're incompatible, and this is in the top-level cmd_fsmonitor__daemon() function. All OK, except why didn't we check this before "set IPC?".
Anyway, re-arranging some of the diff below:
Show 10 quoted lines
> @@ -86,6 +111,9 @@ void fsm_settings__set_ipc(struct repository *r) > > lookup_fsmonitor_settings(r); > > + if (check_for_incompatible(r)) > + return; > + > r->settings.fsmonitor->mode = FSMONITOR_MODE_IPC; > FREE_AND_NULL(r->settings.fsmonitor->hook_path); > }
Here in fsm_settings__set_ipc we return with a NOOP if we're not compatible.
Then:
Show 17 quoted lines
> +int fsm_settings__error_if_incompatible(struct repository *r)
> +{
> + enum fsmonitor_reason reason = fsm_settings__get_reason(r);
> +
> + switch (reason) {
> + case FSMONITOR_REASON_OK:
> + return 0;
> +
> + case FSMONITOR_REASON_BARE:
> + error(_("bare repository '%s' is incompatible with fsmonitor"),
> + xgetcwd());
> + return 1;
> + }
> +
> + BUG("Unhandled case in fsm_settings__error_if_incompatible: '%d'",
> + reason);
> +}Here we'll call fsm_settings__get_reason() which does the same.
Show 9 quoted lines
> +enum fsmonitor_reason fsm_settings__get_reason(struct repository *r)
> +{
> + if (!r)
> + r = the_repository;
> +
> + lookup_fsmonitor_settings(r);
> +
> + return r->settings.fsmonitor->reason;
> +}Is there a reason we can't skip this indirection when using the API like this and e.g. do:
enum fsmonitor_reason reason;
prepare_repo_settings(the_repository);
reason = fsmonitor_check_for_incompatible(the_repository)
if (reason != FSMONITOR_REASON_OK)
die("%s", fsm_settings__get_reason_string(reason));There's just two callers of this API in "seen", and neither need/want this pattern where every method needs to lazy load itself, or the indirection where fsmonitor-settings.c needs to be used as a clearing-house for state management.
Maybe I'm missing something, but why not make check_for_incompatible() non-static and have the callers use that (and then it would return "fsmonitor_reason", not "int", the int return value being redundant to the enum)>
Show 15 quoted lines
> diff --git a/builtin/update-index.c b/builtin/update-index.c
> index 876112abb21..d29048f16f2 100644
> --- a/builtin/update-index.c
> +++ b/builtin/update-index.c
> @@ -1237,6 +1237,10 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
>
> if (fsmonitor > 0) {
> enum fsmonitor_mode fsm_mode = fsm_settings__get_mode(r);
> +
> + if (fsm_settings__error_if_incompatible(the_repository))
> + return 1;
> +
> if (fsm_mode == FSMONITOR_MODE_DISABLED) {
> warning(_("core.fsmonitor is unset; "
> "set it if you really want to "This looks like a bug, we knew before aquiring the lockfile that we weren't compatible, so why wait until here to error out? This seems to skip the rollback_lock_file(), so won't we leave a stale lock?