Subject: Re: [PATCH v4 04/27] fsmonitor-settings: bare repos are incompatible with FSMonitor

From: Ævar Arnfjörð Bjarmason <[email-masked]>
Date: Tue, 19 Apr 2022 11:44:19 +0200

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?