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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Mar 11, 2022, 01:31 UTC
Message-ID: <220311.86k0d1i7xr.gmgdl@evledraar.gmail.com>
In-Reply-To: <8c4f90ae4fd5d9fbac9acb9307ee82ceffc7df08.1646777727.git.gitgitgadget@gmail.com>

On Tue, Mar 08 2022, Jeff Hostetler via GitGitGadget wrote:

Show 32 quoted lines

> From: Jeff Hostetler <jeffhost@microsoft.com>
> [...]
> +	prepare_repo_settings(the_repository);
> +	fsm_settings__set_ipc(the_repository);
> +
> +	if (fsm_settings__get_mode(the_repository) == FSMONITOR_MODE_INCOMPATIBLE) {
> +		const char *msg = fsm_settings__get_reason_msg(the_repository);
> +
> +		return error("%s '%s'", msg ? msg : "???", xgetcwd());
> +	}
> +
>  	if (!strcmp(subcmd, "start"))
>  		return !!try_to_start_background_daemon();
>  
> diff --git a/builtin/update-index.c b/builtin/update-index.c
> index d335f1ac72a..8f460e7195f 100644
> --- a/builtin/update-index.c
> +++ b/builtin/update-index.c
> @@ -1237,6 +1237,13 @@ 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_mode == FSMONITOR_MODE_INCOMPATIBLE) {
> +			const char *msg = fsm_settings__get_reason_msg(r);
> +
> +			return error("%s '%s'", msg ? msg : "???", xgetcwd());
> +		}
> +
>  		if (fsm_mode == FSMONITOR_MODE_DISABLED) {
>  			advise(_("core.fsmonitor is unset; "
>  				 "set it if you really want to "

Can w assert somewhere earlier that ->mode can't be FSMONITOR_MODE_INCOMPATIBLE at the same time that ->reason == FSMONITOR_REASON_OK, should that ever happen?

Then we can get rid of the "???" case here.

The "%s '%s'" here should really be marked for translation, but just "some reason '$path'" is a pretty confusing message. This will emit e.g.:

    "bare repos are incompatible with fsmonitor '/some/path/to/repo'"

Since we always hand these to error maybe have the helper do e.g.:

    error(_("bare repository '%s' is incompatible with fsmonitor"), path);

I find the second-guessing in fsmonitor-settings.c really hard to
follow, i.e. how seemingly every function has some "not loaded yet? load
it" instead of a more typical "init it", "use it", "free it"
pattern. Including stuff like this:
	
	enum fsmonitor_mode fsm_settings__get_mode(struct repository *r)
	{
	        if (!r)
	                r = the_repository;

But anyway, seeing as we do try really hard to load the_repository (or a repository) can't we use the_repository->gitdir etc. here instead of xgetcwd(), or the_repository->worktree when non-bare?