Re: [PATCH v4 03/29] fsmonitor: config settings are repository-specific

From: Jeff Hostetler <git@jeffhostetler.com>
Date: Oct 27, 2021, 19:53 UTC
Message-ID: <74282d08-aaeb-0a1e-cad3-1de17d59b4d1@jeffhostetler.com>
In-Reply-To: <xmqqh7dajcxr.fsf@gitster.g>

On 10/21/21 5:16 PM, Junio C Hamano wrote:

Show 33 quoted lines

> Junio C Hamano <gitster@pobox.com> writes:
> 
>>> +enum fsmonitor_mode {
>>> +	FSMONITOR_MODE_DISABLED = 0,
>>> +	FSMONITOR_MODE_HOOK = 1, /* core.fsmonitor */
>>> +	FSMONITOR_MODE_IPC = 2,  /* core.useBuiltinFSMonitor */
>>> +};
>>
>> Please remind me why we need a new separate variable, instead of
>> turning the core.fsmonitor variable into an extended bool <false,
>> true, builtin>?
> 
> Ah, I see.
> 
> The vocabulary of the value for the existing variable is between
> "unset means disabled" and "the path-to-hook means enabled", so
> unless we forbid a bareword path "builtin" (which I do not think is
> such a bad idea, by the way), it becomes a bit fuzzy what a
> non-empty token means.
> 
> In any case, the "set to path to enable, leave unset to leave
> disabled" is a cumbersome to use and may want to be rethought.  It
> is unclear how one would override a configured path-to-hook, for
> example.
> 
> Considering that we need to reserve a special word, say, "disabled",
> that has to be distinguishable from a normal "here is a path to the
> hook script" ANYWAY, in order to allow such a "last one wins"
> configuration override (or "git -c core.fsmonitor=disabled cmd"), it
> starts to sound more and more reasonable to reserve yet another word
> "builtin" as a special value of core.fsmonitor, without having to
> introduce a new configuration variable, no?
>

For a while we were using a ":builtin:" reserved value (which isn't a valid pathname on Windows) but thought it better to split it into two different config values to avoid the confusion (since it is a valid path on Mac/Linux). But having 2 config vars is also confusing.

And yes, I'm not sure there is a way for a local fsmonitor hook config to override a global hook value unless we add a "disabled" or "off" value.

Let me revisit this.  We could have:

     [] unset, <any of the standard boolean false values>, "disabled"
     [] <hook-path>
     [] "builtin", "ipc"

and just make the literals special reserved values.

I've not kept up on the configurable hooks series, but I have to wonder if this usage (pathname or reserved word) will cause problems with the changes being planned for hooks. The existing fsmonitor usage doesn't use find_hook() nor run_hook*(), so I don't expect an immediate conflict. But again, I haven't kept up with that effort.

Jeff