From: Jeff Hostetler <git@jeffhostetler.com>
Date: Wed, 21 Sep 2022 11:22:16 GMT
Subject: Re: [PATCH v10 5/6] fsmonitor: check for compatability before communicating with fsmonitor
Message-ID: <28d36af9-f493-f4a5-4aaf-18758e1c46db@jeffhostetler.com>
In-Reply-To: <8f6c1fbacbfd3df67d1a7591788d98f1af0e26f4.1663705986.git.gitgitgadget@gmail.com>



On 9/20/22 4:33 PM, Eric DeCosta via GitGitGadget wrote:
> From: Eric DeCosta <edecosta@mathworks.com>
> 
> If fsmonitor is not in a compatible state, die with an appropriate error
> messge.
[...]
> diff --git a/fsmonitor-settings.c b/fsmonitor-settings.c
> index 531a1b6f956..24480b9806d 100644
> --- a/fsmonitor-settings.c
> +++ b/fsmonitor-settings.c
[...]
> +char *fsm_settings__get_incompatible_msg(struct repository *r,
>   					 enum fsmonitor_reason reason)
>   {
>   	struct strbuf msg = STRBUF_INIT;
> +	const char *socket_dir;
>   
>   	switch (reason) {
>   	case FSMONITOR_REASON_UNTESTED:
> @@ -281,9 +283,11 @@ char *fsm_settings__get_incompatible_msg(const struct repository *r,
>   		goto done;
>   
>   	case FSMONITOR_REASON_NOSOCKETS:
> +		socket_dir = dirname((char *)fsmonitor_ipc__get_path(r));
>   		strbuf_addf(&msg,
> -			    _("repository '%s' is incompatible with fsmonitor due to lack of Unix sockets"),
> -			    r->worktree);
> +			    _("socket directory '%s' is incompatible with fsmonitor due"),
> +			    socket_dir);
> +		strbuf_add(&msg, _(" to lack of Unix sockets support"), 32);
>   		goto done;

I don't think we should split the error message between two
calls to strbuf_add().  I realize that this was probably done
because of line length concerns.  But this makes assumptions
on language word order during translations.

Instead, we can use C string literal joining before passing
it to the translation macro.  Something like:

	strbuf_addf(&msg,
		_("socket directory '%s' is incompatible with "
		  "fsmonitor due to lack of Unix sockets support"),
		socket_dir);

[...]
> diff --git a/fsmonitor.c b/fsmonitor.c
> index 57d6a483bee..43d580132fb 100644
> --- a/fsmonitor.c
> +++ b/fsmonitor.c
> @@ -305,6 +305,10 @@ void refresh_fsmonitor(struct index_state *istate)
>   	int is_trivial = 0;
>   	struct repository *r = istate->repo ? istate->repo : the_repository;
>   	enum fsmonitor_mode fsm_mode = fsm_settings__get_mode(r);
> +	enum fsmonitor_reason reason = fsm_settings__get_reason(r);
> +
> +	if (reason > FSMONITOR_REASON_OK)
> +		die("%s", fsm_settings__get_incompatible_msg(r, reason));

We don't want to call die() here.  Maybe just silently return
without doing anything or issue a warning() and return.  (But
I'm favoring a silent return here.)

 From the clients' (`git status`, `git diff`, etc.) point of view,
they just want a speed-up, if possible, but we shouldn't kill them;
we should just let them do the normal scan that would have done
if the feature were turned off.

Jeff