On Thu, Mar 24 2022, Jeff Hostetler via GitGitGadget wrote:
> 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:
> @@ -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:
> +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.
> +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)>
> 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?
