On 10/21/21 5:16 PM, Junio C Hamano wrote:
Junio C Hamano <gitster@xxxxxxxxx> 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
