On 4/19/22 5:44 AM, Ævar Arnfjörð Bjarmason wrote:
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)>
I suppose we could rearrange things to hide less of the
state management. I'm not sure it matters one way or the
other, but I'll give it a try and see if simplifies things.
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?
Yes, good catch. The `return` here will bypass the rollback.
Hopefully, the above rearrangement will make this go away.
I do have to wonder about the rest of this function. There are
several `die()`, `exit()`, `usage()`, `BUG()`, and other `return`
statements after the index lock is taken that won't hit the
rollback. Should those be investigated too? (I can't do that
now in the context of this series, though.)
Jeff
