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

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 21, 2021, 21:16 UTC
Message-ID: <xmqqh7dajcxr.fsf@gitster.g>
In-Reply-To: <xmqqlf2mjdgf.fsf@gitster.g>

Junio C Hamano <gitster@pobox.com> writes:

Show 9 quoted lines

>> +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?