Re: [PATCH 2/2] repository: move 'repository_format_worktree_config' to repo scope
From: Glen Choo <hidden>
Date: 2023-05-25 01:29:25
Here's a quick response on the config.c bits, I haven't looked through the global-removing parts closely yet. Rearranging the hunks for clarity... "Victoria Dye via GitGitGadget" [off-list ref] writes:
quoted hunk ↗ jump to hunk
@@ -2667,11 +2670,14 @@ static void repo_read_config(struct repository *repo) { struct config_options opts = { 0 }; struct configset_add_data data = CONFIGSET_ADD_INIT; + struct git_config_source config_source = { 0 }; opts.respect_includes = 1; opts.commondir = repo->commondir; opts.git_dir = repo->gitdir; + config_source.repo = repo; + if (!repo->config) CALLOC_ARRAY(repo->config, 1); else@@ -2681,7 +2687,7 @@ static void repo_read_config(struct repository *repo) data.config_set = repo->config; data.config_reader = &the_reader; - if (config_with_options(config_set_callback, &data, NULL, &opts) < 0) + if (config_with_options(config_set_callback, &data, &config_source, &opts) < 0) /* * config_with_options() normally returns only * zero, as most errors are fatal, and
I think it would be better to pass a "struct repository" arg to config_with_options() instead of mocking a config_source to hold a .repo member. config_with_options() does double duty - it either discovers and reads the configs for the repo (system, global, worktree, etc), or it reads the config from just config_source. From this perspective, it doesn't make sense that the caller can pass config_source but config_with_options() will still discover and read all configs, and I think the only reason why this behavior is supported at all is that builtin/config.c sometimes "reads all config" and sometimes "reads from a single file", but sloppily passes a non-NULL "config_source" arg unconditionally.
quoted hunk ↗ jump to hunk
diff --git a/config.c b/config.c index a93f7bfa3aa..9ce2ffff5e1 100644 --- a/config.c +++ b/config.c@@ -2277,7 +2278,7 @@ int config_with_options(config_fn_t fn, void *data, data = &inc; } - if (config_source) + if (config_source && config_source->scope != CONFIG_SCOPE_UNKNOWN) config_reader_set_scope(&the_reader, config_source->scope); /*
The aforemented change would also let us get rid of this, which might not always be correct. I think there might be cases where the scope is actually unknown, but I'm not sure if we have any of those situations in-tree.
quoted hunk ↗ jump to hunk
diff --git a/environment.c b/environment.c index 28d18eaca8e..6bd001efbde 100644 --- a/environment.c +++ b/environment.c@@ -42,7 +42,6 @@ int is_bare_repository_cfg = -1; /* unspecified */ int warn_ambiguous_refs = 1; int warn_on_object_refname_ambiguity = 1; int repository_format_precious_objects; -int repository_format_worktree_config; const char *git_commit_encoding; const char *git_log_output_encoding; char *apply_default_whitespace;
As an aside, I'm really happy to lose another global :)