Re: [PATCH 04/23] fsmonitor: introduce `core.useBuiltinFSMonitor` to call the daemon via IPC

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Tue, Apr 27 2021, Derrick Stolee wrote:

> On 4/27/2021 5:20 AM, Ævar Arnfjörð Bjarmason wrote:
>> 
>> On Mon, Apr 26 2021, Derrick Stolee wrote:
>> 
>>> On 4/1/21 11:40 AM, Johannes Schindelin via GitGitGadget wrote:> @@ -2515,6 +2515,11 @@ int git_config_get_max_percent_split_change(void)
> ...
>>>> --- a/repo-settings.c
>>>> +++ b/repo-settings.c
>>>> @@ -58,6 +58,9 @@ void prepare_repo_settings(struct repository *r)
>>>>  		r->settings.core_multi_pack_index = value;
>>>>  	UPDATE_DEFAULT_BOOL(r->settings.core_multi_pack_index, 1);
>>>>  
>>>> +	if (!repo_config_get_bool(r, "core.usebuiltinfsmonitor", &value) && value)
>>>> +		r->settings.use_builtin_fsmonitor = 1;
>>>> +
>>>
>>> Follows the patterns of repo settings. Good.
>> 
>> It follows the pattern, but as an aside the pattern seems bit odd. I see
>> it dates back to your 7211b9e7534 (repo-settings: consolidate some
>> config settings, 2019-08-13).
>> 
>> I.e. we memset() the whole thing to -1, then for most things do something like:
>> 
>>     if (!repo_config_get_bool(r, "gc.writecommitgraph", &value))
>>         r->settings.gc_write_commit_graph = value;
>>     UPDATE_DEFAULT_BOOL(r->settings.gc_write_commit_graph, 1);
>> 
>> But could do:
>> 
>>     if (repo_config_get_bool(r, "gc.writecommitgraph", &r->settings.gc_write_commit_graph))
>>         r->settings.gc_write_commit_graph = 1;
>> 
>> No? I.e. the repo_config_get_bool() function already returns non-zero if
>> we don't find it in the config.
>
> I see how this is fewer lines of code, but it is harder to read the intent
> of the implementation. The current [...]

That's exactly the reason I find the existing version unreadable, i.e.:

> layout makes it clear that we set the value from the config, if it
> exists, but otherwise we choose a default.

The repo_config_get_*() functions only return non-zero if the value
doesn't exist, so the pattern of:

    if (repo_config_get(..., "some.key", &value))
        value = 123;

Is idiomatic for "use 123 if some.key doesn't exist in config".

Maybe I'm missing something and that isn't true, but it seems like a
case of going out of one's way to use what the return value is going to
give you.

> Sometimes, this choice of a default _needs_ to be deferred, for example with
> the fetch_negotiation_algorithm setting, which can be set both from the
> fetch.negotiationAlgorithm config, but also the feature.experimental config.

Don't FETCH_NEGOTIATION_UNSET and UNTRACKED_CACHE_UNSET only exist as
action-at-a-distance interaction with the memset to -1 that this
function does?

I.e. it's somewhat complex state management, first we set it to
"uninit", then later act on fetch.negotiationalgorithm, and then on
feature.experimental, and then set a default only if we didn't do any of
the previous things.;

I.e. something like:

    x = -1;
    if (fetch.negotiationalgorithm is set)
    if (x != -1 && feature.experimental is set)
    if (x != -1) x = default
    settings->x = x;

As opposed to a more (to me at least) simpler:

    int x;
    if (fetch.negotiationalgorithm is set)
    else if (feature.experimental is set)
    else x = default
    settings->x = x;

> However, perhaps it would be better still for these one-off requests to
> create a new macro, say USE_CONFIG_OR_DEFAULT_BOOL() that fills a value
> from config _or_ sets the given default:
>
> #define USE_CONFIG_OR_DEFAULT_BOOL(r, v, s, d) \
> 	if (repo_config_get_bool(r, s, &v)) \
> 		v = d
>
> And then for this example we would write
>
> 	USE_CONFIG_OR_DEFAULT_BOOL(r, r->settings.core_commit_graph,
> 				   "core.commitgraph", 1);
>
> This would work for multiple config options in this file.

I came up with this:

+static void repo_env_config_bool_or_default(struct repository *r, const char *env,
+					    const char *key, int *dest, int def)
+{
+	if (env) {
+		int val = git_env_bool(env, -1);
+		if (val != -1) {
+			*dest = val;
+			return;
+		}
+	}
+	if (repo_config_get_bool(r, key, dest))
+		*dest = def;
+}

Used as e.g.:

+	repo_env_config_bool_or_default(r, NULL, "pack.usesparse",
+					&r->settings.pack_use_sparse, 1);
+	repo_env_config_bool_or_default(r, GIT_TEST_MULTI_PACK_INDEX, "core.multipackindex",
+					&r->settings.core_multi_pack_index, 1);

It works for most things there.

Using that sort of pattern also fixes e.g. a bug in your 18e449f86b7
(midx: enable core.multiPackIndex by default, 2020-09-25), where we'll
ignore a false-but-existing env config value over a true config key.

>> I see the UPDATE_DEFAULT_BOOL() macro has also drifted from "set thing
>> default boolean" to "set any default value".
>  
> This is correct. I suppose it would be a good change to make some time.
> Such a rename could be combined with the refactor above.
>
> I would recommend waiting until such a change isn't conflicting with
> ongoing topics, such as this one.

I'm not planning to work on it, but thought I'd ask/prod the original
author if they were interested :)