RE: [PATCH v10 5/6] fsmonitor: check for compatability before communicating with fsmonitor

[Eric DeCosta <edecosta@mathworks.com>]

> -----Original Message-----
> From: Jeff Hostetler <git@jeffhostetler.com>
> Sent: Wednesday, September 21, 2022 7:22 AM
> To: Eric DeCosta via GitGitGadget <gitgitgadget@gmail.com>;
> git@vger.kernel.org
> Cc: Eric Sunshine <sunshine@sunshineco.com>; Torsten Bögershausen
> <tboegi@web.de>; Ævar Arnfjörð Bjarmason <avarab@gmail.com>; Ramsay
> Jones <ramsay@ramsayjones.plus.com>; Johannes Schindelin
> <Johannes.Schindelin@gmx.de>; Eric DeCosta <edecosta@mathworks.com>
> Subject: Re: [PATCH v10 5/6] fsmonitor: check for compatability before
> communicating with fsmonitor
> 
> 
> 
> 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
> > 100644f
> > --- 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

If we just silently return then fsmonitor is in a perpetual incompatible state and the user gets no benefit from fsmonitor (in fact it could be worse as fsmonitor will attempt to spawn over and over again). I would think that it would be better to at least inform the user so that they can update fsmonitor's settings and have a more pleasant experience going forward.

-Eric