| Message ID | 5ed9100a7707a529b309005419244d083cdc85ba.1684883872.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix behavior of worktree config in submodules | expand |
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" <gitgitgadget@gmail.com> writes: > @@ -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. > 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. > 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 :)
Glen Choo <chooglen@google.com> writes: > 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. The flipside is that this would be redundant with an existing use of git_config_source.repo, so for consistency, we should probably remove git_config_source.repo. There's only one user of git_config_source.repo - reading .gitmodules from a blob. It probably made sense to add .repo the time, but now that we have a second, different use of "struct repository", accepting an arg is probably better.
Glen Choo wrote: > Glen Choo <chooglen@google.com> writes: > >> 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. > > The flipside is that this would be redundant with an existing use of > git_config_source.repo, so for consistency, we should probably remove > git_config_source.repo. There's only one user of git_config_source.repo > - reading .gitmodules from a blob. It probably made sense to add .repo > the time, but now that we have a second, different use of "struct > repository", accepting an arg is probably better. Agreed, I'd much prefer having a single 'struct repository' instance used in the context of config parsing (if they ever had different values, it probably wouldn't be immediately obvious for debugging). This and all of your other recommendations seem reasonable to me, I'll re-roll shortly. Thanks for the review!
On 5/23/2023 7:17 PM, Victoria Dye via GitGitGadget wrote: > From: Victoria Dye <vdye@github.com> > > Move 'repository_format_worktree_config' out of the global scope and into > the 'repository' struct. This change is similar to how > 'repository_format_partial_clone' was moved in ebaf3bcf1ae (repository: move > global r_f_p_c to repo struct, 2021-06-17), adding to the 'repository' > struct and updating 'setup.c' & 'repository.c' functions to assign the value > appropriately. In addition, update usage of the setting to reference the > relevant context's repo or, as a fallback, 'the_repository'. > > The primary goal of this change is to be able to load worktree config for a > submodule depending on whether that submodule - not the super project - has > 'extensions.worktreeConfig' enabled. To ensure 'do_git_config_sequence()' > has access to the newly repo-scoped configuration: > > - update 'repo_read_config()' to create a 'config_source' to hold the > repo instance > - add a 'repo' argument to 'do_git_config_sequence()' > - update 'config_with_options' to call 'do_git_config_sequence()' with > 'config_source.repo', or 'the_repository' as a fallback > > Finally, add/update tests in 't3007-ls-files-recurse-submodules.sh' to > verify 'extensions.worktreeConfig' is read an used independently by super > projects and submodules. > @@ -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); This extra condition on config_source->scope surprised me. Could you elaborate on the reason this is necessary? > @@ -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 }; This could be... struct git_config_source config_source = { .repo = repo }; > > opts.respect_includes = 1; > opts.commondir = repo->commondir; > opts.git_dir = repo->gitdir; > > + config_source.repo = repo; > + ...avoiding these lines. > diff --git a/t/t3007-ls-files-recurse-submodules.sh b/t/t3007-ls-files-recurse-submodules.sh > index e35c203241f..6d0bacef4de 100755 > --- a/t/t3007-ls-files-recurse-submodules.sh > +++ b/t/t3007-ls-files-recurse-submodules.sh > @@ -309,19 +309,30 @@ test_expect_success '--recurse-submodules parses submodule repo config' ' > test_expect_success '--recurse-submodules parses submodule worktree config' ' > test_when_finished "git -C submodule config --unset extensions.worktreeConfig" && > test_when_finished "git -C submodule config --worktree --unset feature.experimental" && > - test_when_finished "git config --unset extensions.worktreeConfig" && > > git -C submodule config extensions.worktreeConfig true && > git -C submodule config --worktree feature.experimental "invalid non-boolean value" && > > - # NEEDSWORK: the extensions.worktreeConfig is set globally based on super > - # project, so we need to enable it in the super project. > - git config extensions.worktreeConfig true && > - > test_must_fail git ls-files --recurse-submodules 2>err && > grep "bad boolean config value" err > ' These are my favorite kind of test updates: deleting extra setup that's no longer needed. > +test_expect_success '--recurse-submodules submodules ignore super project worktreeConfig extension' ' > + test_when_finished "git config --unset extensions.worktreeConfig" && > + > + # Enable worktree config in both super project & submodule, set an > + # invalid config in the submodule worktree config, then disable worktree > + # config in the submodule. The invalid worktree config should not be > + # picked up. > + git config extensions.worktreeConfig true && > + git -C submodule config extensions.worktreeConfig true && > + git -C submodule config --worktree feature.experimental "invalid non-boolean value" && > + git -C submodule config --unset extensions.worktreeConfig && > + > + git ls-files --recurse-submodules 2>err && > + ! grep "bad boolean config value" err > +' We have the same ways to improve here using 'test_config' as recommended in patch 1. Thanks, -Stolee
diff --git a/builtin/config.c b/builtin/config.c index ff2fe8ef125..5e352117c7a 100644 --- a/builtin/config.c +++ b/builtin/config.c @@ -5,6 +5,7 @@ #include "color.h" #include "editor.h" #include "environment.h" +#include "repository.h" #include "gettext.h" #include "ident.h" #include "parse-options.h" @@ -713,7 +714,7 @@ int cmd_config(int argc, const char **argv, const char *prefix) given_config_source.scope = CONFIG_SCOPE_LOCAL; } else if (use_worktree_config) { struct worktree **worktrees = get_worktrees(); - if (repository_format_worktree_config) + if (the_repository->repository_format_worktree_config) given_config_source.file = git_pathdup("config.worktree"); else if (worktrees[0] && worktrees[1]) die(_("--worktree cannot be used with multiple " diff --git a/builtin/worktree.c b/builtin/worktree.c index f3180463be2..60e389aaedb 100644 --- a/builtin/worktree.c +++ b/builtin/worktree.c @@ -483,7 +483,7 @@ static int add_worktree(const char *path, const char *refname, * values from the current worktree into the new one, that way the * new worktree behaves the same as this one. */ - if (repository_format_worktree_config) + if (the_repository->repository_format_worktree_config) copy_filtered_worktree_config(sb_repo.buf); strvec_pushf(&child_env, "%s=%s", GIT_DIR_ENVIRONMENT, sb_git.buf); diff --git a/config.c b/config.c index a93f7bfa3aa..9ce2ffff5e1 100644 --- a/config.c +++ b/config.c @@ -2193,6 +2193,7 @@ int git_config_system(void) static int do_git_config_sequence(struct config_reader *reader, const struct config_options *opts, + const struct repository *repo, config_fn_t fn, void *data) { int ret = 0; @@ -2241,7 +2242,7 @@ static int do_git_config_sequence(struct config_reader *reader, config_reader_set_scope(reader, CONFIG_SCOPE_WORKTREE); if (!opts->ignore_worktree && worktree_config && - repository_format_worktree_config && + repo && repo->repository_format_worktree_config && !access_or_die(worktree_config, R_OK, 0)) { ret += git_config_from_file(fn, worktree_config, data); } @@ -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); /* @@ -2294,7 +2295,9 @@ int config_with_options(config_fn_t fn, void *data, ret = git_config_from_blob_ref(fn, repo, config_source->blob, data); } else { - ret = do_git_config_sequence(&the_reader, opts, fn, data); + struct repository *repo = config_source && config_source->repo ? + config_source->repo : the_repository; + ret = do_git_config_sequence(&the_reader, opts, repo, fn, data); } if (inc.remote_urls) { @@ -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 @@ -3337,7 +3343,7 @@ int repo_config_set_worktree_gently(struct repository *r, const char *key, const char *value) { /* Only use worktree-specific config if it is already enabled. */ - if (repository_format_worktree_config) { + if (r->repository_format_worktree_config) { char *file = repo_git_path(r, "config.worktree"); int ret = git_config_set_multivar_in_file_gently( file, key, value, NULL, 0); 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; diff --git a/environment.h b/environment.h index 30cb7e0fa34..e6668079269 100644 --- a/environment.h +++ b/environment.h @@ -197,7 +197,6 @@ extern char *notes_ref_name; extern int grafts_replace_parents; extern int repository_format_precious_objects; -extern int repository_format_worktree_config; /* * Create a temporary file rooted in the object database directory, or diff --git a/repository.c b/repository.c index c53e480e326..104960f8f59 100644 --- a/repository.c +++ b/repository.c @@ -182,6 +182,7 @@ int repo_init(struct repository *repo, goto error; repo_set_hash_algo(repo, format.hash_algo); + repo->repository_format_worktree_config = format.worktree_config; /* take ownership of format.partial_clone */ repo->repository_format_partial_clone = format.partial_clone; diff --git a/repository.h b/repository.h index 1a13ff28677..74ae26635a4 100644 --- a/repository.h +++ b/repository.h @@ -163,6 +163,7 @@ struct repository { struct promisor_remote_config *promisor_remote_config; /* Configurations */ + int repository_format_worktree_config; /* Indicate if a repository has a different 'commondir' from 'gitdir' */ unsigned different_commondir:1; diff --git a/setup.c b/setup.c index 458582207ea..d8663954350 100644 --- a/setup.c +++ b/setup.c @@ -650,11 +650,10 @@ static int check_repository_format_gently(const char *gitdir, struct repository_ } repository_format_precious_objects = candidate->precious_objects; - repository_format_worktree_config = candidate->worktree_config; string_list_clear(&candidate->unknown_extensions, 0); string_list_clear(&candidate->v1_only_extensions, 0); - if (repository_format_worktree_config) { + if (candidate->worktree_config) { /* * pick up core.bare and core.worktree from per-worktree * config if present @@ -1423,6 +1422,9 @@ int discover_git_directory(struct strbuf *commondir, return -1; } + the_repository->repository_format_worktree_config = + candidate.worktree_config; + /* take ownership of candidate.partial_clone */ the_repository->repository_format_partial_clone = candidate.partial_clone; @@ -1560,6 +1562,8 @@ const char *setup_git_directory_gently(int *nongit_ok) } if (startup_info->have_repository) { repo_set_hash_algo(the_repository, repo_fmt.hash_algo); + the_repository->repository_format_worktree_config = + repo_fmt.worktree_config; /* take ownership of repo_fmt.partial_clone */ the_repository->repository_format_partial_clone = repo_fmt.partial_clone; @@ -1651,6 +1655,8 @@ void check_repository_format(struct repository_format *fmt) check_repository_format_gently(get_git_dir(), fmt, NULL); startup_info->have_repository = 1; repo_set_hash_algo(the_repository, fmt->hash_algo); + the_repository->repository_format_worktree_config = + fmt->worktree_config; the_repository->repository_format_partial_clone = xstrdup_or_null(fmt->partial_clone); clear_repository_format(&repo_fmt); diff --git a/t/t3007-ls-files-recurse-submodules.sh b/t/t3007-ls-files-recurse-submodules.sh index e35c203241f..6d0bacef4de 100755 --- a/t/t3007-ls-files-recurse-submodules.sh +++ b/t/t3007-ls-files-recurse-submodules.sh @@ -309,19 +309,30 @@ test_expect_success '--recurse-submodules parses submodule repo config' ' test_expect_success '--recurse-submodules parses submodule worktree config' ' test_when_finished "git -C submodule config --unset extensions.worktreeConfig" && test_when_finished "git -C submodule config --worktree --unset feature.experimental" && - test_when_finished "git config --unset extensions.worktreeConfig" && git -C submodule config extensions.worktreeConfig true && git -C submodule config --worktree feature.experimental "invalid non-boolean value" && - # NEEDSWORK: the extensions.worktreeConfig is set globally based on super - # project, so we need to enable it in the super project. - git config extensions.worktreeConfig true && - test_must_fail git ls-files --recurse-submodules 2>err && grep "bad boolean config value" err ' +test_expect_success '--recurse-submodules submodules ignore super project worktreeConfig extension' ' + test_when_finished "git config --unset extensions.worktreeConfig" && + + # Enable worktree config in both super project & submodule, set an + # invalid config in the submodule worktree config, then disable worktree + # config in the submodule. The invalid worktree config should not be + # picked up. + git config extensions.worktreeConfig true && + git -C submodule config extensions.worktreeConfig true && + git -C submodule config --worktree feature.experimental "invalid non-boolean value" && + git -C submodule config --unset extensions.worktreeConfig && + + git ls-files --recurse-submodules 2>err && + ! grep "bad boolean config value" err +' + test_incompatible_with_recurse_submodules () { test_expect_success "--recurse-submodules and $1 are incompatible" " test_must_fail git ls-files --recurse-submodules $1 2>actual && diff --git a/worktree.c b/worktree.c index b5ee71c5ebd..c448fecd4b3 100644 --- a/worktree.c +++ b/worktree.c @@ -806,7 +806,7 @@ int init_worktree_config(struct repository *r) * If the extension is already enabled, then we can skip the * upgrade process. */ - if (repository_format_worktree_config) + if (r->repository_format_worktree_config) return 0; if ((res = git_config_set_gently("extensions.worktreeConfig", "true"))) return error(_("failed to set extensions.worktreeConfig setting")); @@ -846,7 +846,7 @@ int init_worktree_config(struct repository *r) * Ensure that we use worktree config for the remaining lifetime * of the current process. */ - repository_format_worktree_config = 1; + r->repository_format_worktree_config = 1; cleanup: git_configset_clear(&cs);