> -----Original Message-----
> From: Jeff Hostetler <git@xxxxxxxxxxxxxxxxx>
> Sent: Wednesday, September 21, 2022 7:22 AM
> To: Eric DeCosta via GitGitGadget <gitgitgadget@xxxxxxxxx>;
> git@xxxxxxxxxxxxxxx
> Cc: Eric Sunshine <sunshine@xxxxxxxxxxxxxx>; Torsten Bögershausen
> <tboegi@xxxxxx>; Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx>; Ramsay
> Jones <ramsay@xxxxxxxxxxxxxxxxxxxx>; Johannes Schindelin
> <Johannes.Schindelin@xxxxxx>; Eric DeCosta <edecosta@xxxxxxxxxxxxx>
> 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@xxxxxxxxxxxxx>
> >
> > 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
