Re: [RFC PATCH 2/2] config: add 'config.superproject' file
From: Matheus Tavares Bernardino <hidden>
Date: 2021-04-09 14:35:31
Hi, Emily I'm not familiar enough with this code to give a full review and I imagine you probably want comments more focused on the design level, while this is an RFC, but here are some small nitpicks I found while reading the patch. I Hope it helps :) On Thu, Apr 8, 2021 at 8:39 PM Emily Shaffer [off-list ref] wrote:
quoted hunk ↗ jump to hunk
diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt index 4b4cc5c5e8..a33136fb08 100644 --- a/Documentation/git-config.txt +++ b/Documentation/git-config.txt@@ -48,7 +48,7 @@ unset an existing `--type` specifier with `--no-type`. When reading, the values are read from the system, global and repository local configuration files by default, and options -`--system`, `--global`, `--local`, `--worktree` and +`--system`, `--global`, `--superproject`, `--local`, `--worktree` and `--file <filename>` can be used to tell the command to read from only that location (see <<FILES>>).@@ -127,6 +127,17 @@ rather than from all available files. + See also <<FILES>>. +--superproject:: + For writing options: write to the superproject's + `.git/config.superproject` file, even if run from a submodule of that + superproject.
Hmm, I wonder what happens if a repo is both a submodule and a superproject (i.e. in case of nested submodules). Let's see: # Create repo/sub/sub2 $ git init repo $ cd repo $ touch F && git add F && git commit -m F $ git submodule add ./ sub $ git -C sub submodule add ./sub sub2 $ git -C sub commit -m sub2 $ git commit -m sub # Now test the config $ git -C sub/sub2 config --superproject foo.bar 1 $ git -C sub/sub2 config --get foo.bar 1 $ git -C sub config --get foo.bar <nothing> $ git config --get foo.bar <nothing> It makes sense to me that `foo.bar` is not defined on `repo`, but shouldn't it be defined on `repo/sub`? Or am I doing something wrong? (`git -C sub rev-parse --git-dir` gives `.git/modules/sub/`, where indeed there is a config.superproject with `foo.bar` set.)
quoted hunk ↗ jump to hunk
diff --git a/builtin/config.c b/builtin/config.c index f71fa39b38..f0a57a89ca 100644 --- a/builtin/config.c +++ b/builtin/config.c@@ -26,7 +26,7 @@ static char key_delim = ' '; static char term = '\n'; static int use_global_config, use_system_config, use_local_config; -static int use_worktree_config; +static int use_worktree_config, use_superproject_config; static struct git_config_source given_config_source; static int actions, type; static char *default_value;@@ -130,6 +130,8 @@ static struct option builtin_config_options[] = { OPT_GROUP(N_("Config file location")), OPT_BOOL(0, "global", &use_global_config, N_("use global config file")), OPT_BOOL(0, "system", &use_system_config, N_("use system config file")), + OPT_BOOL(0, "superproject", + &use_superproject_config, N_("use superproject config file")), OPT_BOOL(0, "local", &use_local_config, N_("use repository config file")), OPT_BOOL(0, "worktree", &use_worktree_config, N_("use per-worktree config file")), OPT_STRING('f', "file", &given_config_source.file, N_("file"), N_("use given config file")),@@ -697,6 +699,12 @@ int cmd_config(int argc, const char **argv, const char *prefix) else if (use_system_config) { given_config_source.file = git_etc_gitconfig(); given_config_source.scope = CONFIG_SCOPE_SYSTEM; + } else if (use_superproject_config) { + struct strbuf superproject_cfg = STRBUF_INIT; + git_config_superproject(&superproject_cfg, get_git_dir()); + given_config_source.file = xstrdup(superproject_cfg.buf); + given_config_source.scope = CONFIG_SCOPE_SUPERPROJECT; + strbuf_release(&superproject_cfg);
Nit: maybe it would be a bit cleaner to replace the xstrdup() +
strbuf_release() lines with a single:
given_config_source.file = strbuf_detach(superproject_cfg, NULL);
quoted hunk ↗ jump to hunk
} else if (use_local_config) { given_config_source.file = git_pathdup("config"); given_config_source.scope = CONFIG_SCOPE_LOCAL;diff --git a/config.c b/config.c index 67d9bf2238..28bb80fd0d 100644 --- a/config.c +++ b/config.c@@ -21,6 +21,7 @@ #include "dir.h" #include "color.h" #include "refs.h" +#include "submodule.h" struct config_source { struct config_source *prev;@@ -1852,6 +1853,17 @@ const char *git_etc_gitconfig(void) return system_wide; } +void git_config_superproject(struct strbuf *sb, const char *gitdir) +{ + if (!get_superproject_gitdir(sb)) { + /* not a submodule */ + strbuf_reset(sb);
Do we have to reset `sb` here? It seems that get_superproject_gitdir() leaves the buffer empty when we are not inside a submodule.
quoted hunk ↗ jump to hunk
diff --git a/submodule.h b/submodule.h index 4ac6e31cf1..1308d5ae2d 100644 --- a/submodule.h +++ b/submodule.h@@ -149,6 +149,12 @@ void prepare_submodule_repo_env(struct strvec *out); void absorb_git_dir_into_superproject(const char *path, unsigned flags); +/* + * Return the gitdir of the superproject, which this project is a submodule of. + * If this repository is not a submodule of another repository, return 0.
Nit: it might be nice to say what's the state of `buf` on a 0 return. Perhaps also be more explicit about the return codes? Maybe something like: "If this repository is a submodule of another repository, save the superproject's gitdir on `buf` and return 1. Otherwise, return 0 and leave `buf` empty."
quoted hunk ↗ jump to hunk
+int get_superproject_gitdir(struct strbuf *buf); + /* * Return the absolute path of the working tree of the superproject, which this * project is a submodule of. If this repository is not a submodule ofdiff --git a/t/t1311-superproject-config.sh b/t/t1311-superproject-config.sh new file mode 100755 index 0000000000..650c4d24c7 --- /dev/null +++ b/t/t1311-superproject-config.sh@@ -0,0 +1,124 @@
[...]
+test_expect_success 'superproject config applies to super and submodule' ' + cat >.git/config.superproject <<-EOF && + [foo] + bar = baz + EOF + + git config --get foo.bar && + git -C sub config --get foo.bar && + + rm .git/config.superproject
Hmm, if this test fails before removing the config.superproject file, couldn't it interfere with other tests (like the 'can --edit superproject config')? Perhaps this and the other similar cleanup removals could be declared inside a `test_when_finished` clause, to ensure they are performed even on test failure.
+test_expect_success 'can --unset from super or sub' ' + git config --superproject apple.species honeycrisp && + git -C sub config --superproject banana.species cavendish && + + git config --unset --superproject banana.species && + git -C sub config --unset --superproject apple.species +'
Nice "cross-setting/unsetting" test :) [...]
+# This test deletes the submodule! Keep it at the end of the test suite. +test_expect_success 'config.submodule works even with no submodules' '
s/config.submodule/config.superproject/ ?