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 :)
