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 layout makes it clear that we set the value from the config, if it exists, but otherwise we choose a default. 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. 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 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. Thanks, -Stolee
