Re: [PATCH 1/2] config: use gitdir to get worktree config
From: Glen Choo <hidden>
Date: 2023-05-25 01:05:42
"Victoria Dye via GitGitGadget" [off-list ref] writes:
From: Victoria Dye <redacted> Update 'do_git_config_sequence()' to read the worktree config from 'config.worktree' in 'opts->git_dir' rather than the gitdir of 'the_repository'.
Thanks for the patches! This makes sense. do_git_config_sequence() is eventually called by repo_config(), which is supposed to read config into a "struct repository", so any reliance on the_repository's settings is wrong.
Note that behavior still isn't ideal because 'extensions.worktreeConfig' in the super project[...]
Nit: We typically use "superproject" without the space.
quoted hunk ↗ jump to hunk
diff --git a/config.c b/config.c index b79baf83e35..a93f7bfa3aa 100644 --- a/config.c +++ b/config.c@@ -2200,14 +2200,24 @@ static int do_git_config_sequence(struct config_reader *reader, char *xdg_config = NULL; char *user_config = NULL; char *repo_config; + char *worktree_config; enum config_scope prev_parsing_scope = reader->parsing_scope; - if (opts->commondir) + /* + * Ensure that either: + * - the git_dir and commondir are both set, or + * - the git_dir and commondir are both NULL + */ + if (!opts->git_dir != !opts->commondir) + BUG("only one of commondir and git_dir is non-NULL"); + + if (opts->commondir) { repo_config = mkpathdup("%s/config", opts->commondir); - else if (opts->git_dir) - BUG("git_dir without commondir"); - else + worktree_config = mkpathdup("%s/config.worktree", opts->git_dir); + } else { repo_config = NULL; + worktree_config = NULL; + }
Makes sense to me. I don't see why we would ever want to set one without the other. I looked into whether we could get replace opts->commondir and opts->git_dir with a "struct repository" arg, but unfortunately read_early_config() needs to pass these values without touching "the_repository".
quoted hunk ↗ jump to hunk
diff --git a/t/t3007-ls-files-recurse-submodules.sh b/t/t3007-ls-files-recurse-submodules.sh index dd7770e85de..e35c203241f 100755 --- a/t/t3007-ls-files-recurse-submodules.sh +++ b/t/t3007-ls-files-recurse-submodules.sh@@ -299,6 +299,29 @@ test_expect_success '--recurse-submodules does not support --error-unmatch' ' test_i18ngrep "does not support --error-unmatch" actual ' +test_expect_success '--recurse-submodules parses submodule repo config' ' + test_when_finished "git -C submodule config --unset feature.experimental" && + git -C submodule config feature.experimental "invalid non-boolean value" && + test_must_fail git ls-files --recurse-submodules 2>err && + grep "bad boolean config value" err +'
This test has a few bits that are important but non-obvious. It would be useful to capture them in either the commit message or a comment. Firstly, we can't test this using "git config" because that only uses the_repository, and we specifically need to read config in-core into a "struct repository" that is a submodule, so we need a command that recurses into a submodule without using subprocesses. IIRC the only choices are "git grep" and "git ls-files". Secondly, when we test that config is read from the submodule the choice of "feature.experimental" is quite important. The config is read quite indirectly: "git ls-files" reads from the submodule's index, which will call prepare_repo_settings() on the submodule, and eventually calls repo_config_get_bool() on "feature.experimental". Any of the configs in prepare_repo_settings() should do, though. A tiny suggestion would be to use "index.sparse" instead of "feature.experimental", since (I presume) we'll have to add sparse index + submodule tests for "git ls-files" eventually.
+test_expect_success '--recurse-submodules parses submodule worktree config' ' + test_when_finished "git -C submodule config --unset extensions.worktreeConfig" &&
I believe "test_config -C" will achieve the desired effect.