* [RFC PATCH 0/2] share a config between submodule and superproject @ 2021-04-08 23:39 Emily Shaffer 2021-04-08 23:39 ` [RFC PATCH 1/2] config: rename "submodule" scope to "gitmodules" Emily Shaffer ` (3 more replies) 0 siblings, 4 replies; 25+ messages in thread From: Emily Shaffer @ 2021-04-08 23:39 UTC (permalink / raw) To: git; +Cc: Emily Shaffer Hi folks, Especially after es/config-based-hooks makes its way to 'next' and 'master', we think it would be really useful to have a config that applies to a whole "project" - where "project" means "superproject and its submodules." There's a longer defense in patch 2, but that's the speedy summary. I'm not wild about the implementation of this solution as it calls out to 'git rev-parse' and 'git ls-files' - not once, but twice! - and I understand that is considerably slower on some platforms than others. But I'm open to suggestions - I couldn't find any other examples of a repo figuring out whether it's a submodule or not. (I thought there may be some, as 'repository.submodule_prefix' exists, but it seems like that's only set in some special cases during operations initated from the superproject.) I'm hoping to work on some other submodule-centric stuff over the coming months, and it might end up being very useful to be able to tell "am I a submodule?" and "how do I talk to my superproject?" more generally - so I'm really open to figuring out a better way than this, if folks have ideas. Patch 1 is a small refactor that we can take or leave - I found "SCOPE_SUBMODULE" to be pretty ambiguous, especially since it seems to refer to configs from .gitmodules. Even though I decided that "superproject" was a better name than "submodule" I still wasn't super happy with the ambiguity. But we can drop it if folks don't want to rename. Thanks a bunch, - Emily Emily Shaffer (2): config: rename "submodule" scope to "gitmodules" config: add 'config.superproject' file Documentation/git-config.txt | 21 +++++- builtin/config.c | 10 ++- config.c | 30 +++++++- config.h | 5 +- submodule-config.c | 2 +- submodule.c | 29 ++++++++ submodule.h | 6 ++ t/t1311-superproject-config.sh | 124 +++++++++++++++++++++++++++++++++ 8 files changed, 220 insertions(+), 7 deletions(-) create mode 100755 t/t1311-superproject-config.sh -- 2.31.1.295.g9ea45b61b8-goog ^ permalink raw reply [flat|nested] 25+ messages in thread
* [RFC PATCH 1/2] config: rename "submodule" scope to "gitmodules" 2021-04-08 23:39 [RFC PATCH 0/2] share a config between submodule and superproject Emily Shaffer @ 2021-04-08 23:39 ` Emily Shaffer 2021-04-08 23:39 ` [RFC PATCH 2/2] config: add 'config.superproject' file Emily Shaffer ` (2 subsequent siblings) 3 siblings, 0 replies; 25+ messages in thread From: Emily Shaffer @ 2021-04-08 23:39 UTC (permalink / raw) To: git; +Cc: Emily Shaffer To prepare for the addition of a new config scope which only applies to submodule projects, disambiguate "CONFIG_SCOPE_SUBMODULES". This scope refers to configs gathered from the .gitmodules file in the superproject, so just call it "gitmodules." Signed-off-by: Emily Shaffer <emilyshaffer@google.com> --- config.c | 4 ++-- config.h | 2 +- submodule-config.c | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/config.c b/config.c index 6428393a41..67d9bf2238 100644 --- a/config.c +++ b/config.c @@ -3513,8 +3513,8 @@ const char *config_scope_name(enum config_scope scope) return "worktree"; case CONFIG_SCOPE_COMMAND: return "command"; - case CONFIG_SCOPE_SUBMODULE: - return "submodule"; + case CONFIG_SCOPE_GITMODULES: + return "gitmodules"; default: return "unknown"; } diff --git a/config.h b/config.h index 19a9adbaa9..535f5517b8 100644 --- a/config.h +++ b/config.h @@ -42,7 +42,7 @@ enum config_scope { CONFIG_SCOPE_LOCAL, CONFIG_SCOPE_WORKTREE, CONFIG_SCOPE_COMMAND, - CONFIG_SCOPE_SUBMODULE, + CONFIG_SCOPE_GITMODULES, }; const char *config_scope_name(enum config_scope scope); diff --git a/submodule-config.c b/submodule-config.c index f502505566..0e435e6fdd 100644 --- a/submodule-config.c +++ b/submodule-config.c @@ -637,7 +637,7 @@ static void config_from_gitmodules(config_fn_t fn, struct repository *repo, void { if (repo->worktree) { struct git_config_source config_source = { - 0, .scope = CONFIG_SCOPE_SUBMODULE + 0, .scope = CONFIG_SCOPE_GITMODULES }; const struct config_options opts = { 0 }; struct object_id oid; -- 2.31.1.295.g9ea45b61b8-goog ^ permalink raw reply related [flat|nested] 25+ messages in thread
* [RFC PATCH 2/2] config: add 'config.superproject' file 2021-04-08 23:39 [RFC PATCH 0/2] share a config between submodule and superproject Emily Shaffer 2021-04-08 23:39 ` [RFC PATCH 1/2] config: rename "submodule" scope to "gitmodules" Emily Shaffer @ 2021-04-08 23:39 ` Emily Shaffer 2021-04-09 11:10 ` Philip Oakley 2021-04-09 14:35 ` Matheus Tavares Bernardino 2021-04-14 10:32 ` Future structure of submodules and .git/, .git/modules/* organization Ævar Arnfjörð Bjarmason 2021-04-23 0:15 ` [RFC PATCH v2 0/4] share a config between submodule and superproject Emily Shaffer 3 siblings, 2 replies; 25+ messages in thread From: Emily Shaffer @ 2021-04-08 23:39 UTC (permalink / raw) To: git; +Cc: Emily Shaffer Some configs, such as wrapper directives like gerrit.createChangeId, or forthcoming hook configs, should apply to a superproject as well as all its submodules. It may not be appropriate to apply them globally - for example, if the user also contributes to many projects which do not use the configs necessary for one project-with-submodules - and it may be burdensome to apply them locally to the superproject and each of its submodules. Even if the user runs 'git submodule foreach "git config --local foo.bar', if a new submodule is added later on, that config is not applied to the new submodule. It is also inappropriate to share the entire superproject config, since some items - like remote URLs or partial-clone filters - would not apply to a submodule. To make life easier for projects with many submodules, then, create a new "config.superproject" config scope, which is included in the config parse for the superproject as well as for all the submodules of that superproject. For the superproject, this new config file is equally local to the local config; for the submodule, the new config file is less local than the local config. So let's include it directly before the local config during the config parse. Signed-off-by: Emily Shaffer <emilyshaffer@google.com> --- Documentation/git-config.txt | 21 +++++- builtin/config.c | 10 ++- config.c | 26 +++++++ config.h | 3 + submodule.c | 29 ++++++++ submodule.h | 6 ++ t/t1311-superproject-config.sh | 124 +++++++++++++++++++++++++++++++++ 7 files changed, 216 insertions(+), 3 deletions(-) create mode 100755 t/t1311-superproject-config.sh 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. ++ +For reading options: read only from the superproject's +`.git/config.superproject` file, even if run from a submodule of that +superproject, rather than from all available files. ++ +See also <<FILES>>. + --local:: For writing options: write to the repository `.git/config` file. This is the default behavior. @@ -283,7 +294,7 @@ The default is to use a pager. FILES ----- -If not set explicitly with `--file`, there are four files where +If not set explicitly with `--file`, there are five files where 'git config' will search for configuration options: $(prefix)/etc/gitconfig:: @@ -301,6 +312,12 @@ $XDG_CONFIG_HOME/git/config:: User-specific configuration file. Also called "global" configuration file. +$GIT_DIR/config.superproject:: + When `git config` is run from a project which is a submodule of another + project, that superproject's $GIT_DIR will be used. Use this config file + to set configurations which need to be the same across a superproject + and all its submodules. + $GIT_DIR/config:: Repository specific configuration file. 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); } 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); + strbuf_addstr(sb, gitdir); + } + + strbuf_addstr(sb, "/config.superproject"); +} + /* * Parse environment variable 'k' as a boolean (in various * possible spellings); if missing, use the default value 'def'. @@ -1909,6 +1921,17 @@ static int do_git_config_sequence(const struct config_options *opts, if (user_config && !access_or_die(user_config, R_OK, ACCESS_EACCES_OK)) ret += git_config_from_file(fn, user_config, data); + current_parsing_scope = CONFIG_SCOPE_SUPERPROJECT; + if (opts->git_dir && !opts->ignore_superproject) { + + struct strbuf superproject_gitdir = STRBUF_INIT; + git_config_superproject(&superproject_gitdir, opts->git_dir); + if (!access_or_die(superproject_gitdir.buf, R_OK, 0)) + ret += git_config_from_file(fn, superproject_gitdir.buf, data); + + strbuf_release(&superproject_gitdir); + } + current_parsing_scope = CONFIG_SCOPE_LOCAL; if (!opts->ignore_repo && repo_config && !access_or_die(repo_config, R_OK, 0)) @@ -2027,6 +2050,7 @@ void read_very_early_config(config_fn_t cb, void *data) opts.respect_includes = 1; opts.ignore_repo = 1; + opts.ignore_superproject = 1; opts.ignore_worktree = 1; opts.ignore_cmdline = 1; opts.system_gently = 1; @@ -3515,6 +3539,8 @@ const char *config_scope_name(enum config_scope scope) return "command"; case CONFIG_SCOPE_GITMODULES: return "gitmodules"; + case CONFIG_SCOPE_SUPERPROJECT: + return "superproject"; default: return "unknown"; } diff --git a/config.h b/config.h index 535f5517b8..b42e1d13eb 100644 --- a/config.h +++ b/config.h @@ -43,6 +43,7 @@ enum config_scope { CONFIG_SCOPE_WORKTREE, CONFIG_SCOPE_COMMAND, CONFIG_SCOPE_GITMODULES, + CONFIG_SCOPE_SUPERPROJECT, }; const char *config_scope_name(enum config_scope scope); @@ -84,6 +85,7 @@ typedef int (*config_parser_event_fn_t)(enum config_event_t type, struct config_options { unsigned int respect_includes : 1; unsigned int ignore_repo : 1; + unsigned int ignore_superproject : 1; unsigned int ignore_worktree : 1; unsigned int ignore_cmdline : 1; unsigned int system_gently : 1; @@ -319,6 +321,7 @@ int git_config_rename_section_in_file(const char *, const char *, const char *); int git_config_copy_section(const char *, const char *); int git_config_copy_section_in_file(const char *, const char *, const char *); const char *git_etc_gitconfig(void); +void git_config_superproject(struct strbuf *, const char *); int git_env_bool(const char *, int); unsigned long git_env_ulong(const char *, unsigned long); int git_config_system(void); diff --git a/submodule.c b/submodule.c index 9767ba9893..92b00f8697 100644 --- a/submodule.c +++ b/submodule.c @@ -2178,6 +2178,35 @@ void absorb_git_dir_into_superproject(const char *path, } } +int get_superproject_gitdir(struct strbuf *buf) +{ + struct strbuf sb = STRBUF_INIT; + struct child_process cp = CHILD_PROCESS_INIT; + int rc = 0; + + /* NEEDSWORK: this call also calls out to a subprocess! */ + rc = get_superproject_working_tree(&sb); + strbuf_release(&sb); + + if (!rc) + return rc; + + prepare_submodule_repo_env_no_git_dir(&cp.env_array); + + strvec_pushl(&cp.args, "-C", "..", "rev-parse", "--absolute-git-dir", NULL); + cp.git_cmd = 1; + + rc = capture_command(&cp, buf, 0); + strbuf_trim_trailing_newline(buf); + + /* leave buf empty if we didn't have a superproject gitdir */ + if (rc) + strbuf_reset(buf); + + /* rc reflects the exit code of the rev-parse; invert into a bool */ + return !rc; +} + int get_superproject_working_tree(struct strbuf *buf) { struct child_process cp = CHILD_PROCESS_INIT; 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. + */ +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 of diff --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 @@ +#!/bin/sh + +test_description='Test git config --superproject in different settings' + +. ./test-lib.sh + +# follow t7400's example and use the trash dir repo as a submodule to add +submodurl=$(pwd -P) + +# since only the configs are modified, set up the repo structure only once +test_expect_success 'setup repo structure' ' + test_commit "base" && + git submodule add "${submodurl}" sub/ && + git commit -m "add a submodule" +' + +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 +' + +test_expect_success 'can add from super or sub' ' + git config --superproject apple.species honeycrisp && + git -C sub config --superproject banana.species cavendish && + + cat >expect <<-EOF && + apple.species=honeycrisp + banana.species=cavendish + EOF + + git config --list >actual && + grep -Ff expect actual && + + git -C sub config --list >actual && + grep -Ff expect actual && + + rm .git/config.superproject +' + +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 +' + +test_expect_success 'can --edit superproject config' ' + test_config core.editor "echo [foo]bar=baz >" && + git config --edit --superproject && + + git config --get foo.bar && + + rm .git/config.superproject +' + +test_expect_success 'can --show-origin the superproject config' ' + git config --superproject --add foo.bar baz && + + git config --list --show-origin >actual && + grep -F "config.superproject" actual && + + rm .git/config.superproject +' + +test_expect_success 'can --show-scope the superproject config' ' + git config --superproject --add foo.bar baz && + + git config --list --show-scope >actual && + grep "superproject" actual && + + rm .git/config.superproject +' + +test_expect_success 'pre-existing config applies to new submodule' ' + git config --superproject --add foo.bar baz && + + git submodule add "${submodurl}" sub2/ && + git commit -m "add a second submodule" && + + git -C sub2 config --get foo.bar && + + # clean up + git reset HEAD^ && + rm -fr sub2 && + rm .git/config.superproject +' + +# NEEDSWORK: submodule.c:get_superproject_working_tree doesn't support worktree +test_expect_failure 'worktrees can still access config.superproject' ' + git config --superproject --add foo.bar baz && + + git worktree add wt && + ( + cd wt && + git config --get foo.bar + ) && + + # clean up + git worktree remove wt && + rm .git/config.superproject +' + +# This test deletes the submodule! Keep it at the end of the test suite. +test_expect_success 'config.submodule works even with no submodules' ' + # get rid of the submodule + git reset HEAD^ && + rm -fr sub && + + git config --superproject --add foo.bar baz && + + git config --get foo.bar && + + rm .git/config.superproject +' + +test_done -- 2.31.1.295.g9ea45b61b8-goog ^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [RFC PATCH 2/2] config: add 'config.superproject' file 2021-04-08 23:39 ` [RFC PATCH 2/2] config: add 'config.superproject' file Emily Shaffer @ 2021-04-09 11:10 ` Philip Oakley 2021-04-13 18:05 ` Emily Shaffer 2021-04-09 14:35 ` Matheus Tavares Bernardino 1 sibling, 1 reply; 25+ messages in thread From: Philip Oakley @ 2021-04-09 11:10 UTC (permalink / raw) To: Emily Shaffer, git On 09/04/2021 00:39, Emily Shaffer wrote: > Some configs, such as wrapper directives like gerrit.createChangeId, or > forthcoming hook configs, should apply to a superproject as well as all > its submodules. It may not be appropriate to apply them globally - for > example, if the user also contributes to many projects which do not use > the configs necessary for one project-with-submodules - and it may be > burdensome to apply them locally to the superproject and each of its > submodules. Even if the user runs 'git submodule foreach "git config > --local foo.bar', if a new submodule is added later on, that config is > not applied to the new submodule. > > It is also inappropriate to share the entire superproject config, since > some items - like remote URLs or partial-clone filters - would not apply > to a submodule. > > To make life easier for projects with many submodules, then, create a > new "config.superproject" config scope, which is included in the config > parse for the superproject as well as for all the submodules of that > superproject. > > For the superproject, this new config file is equally local to the local > config; for the submodule, the new config file is less local than the > local config. So let's include it directly before the local config > during the config parse. > > Signed-off-by: Emily Shaffer <emilyshaffer@google.com> > --- Does this need an update to the `git config --show-origin --show-scope` capability? -- Philip > Documentation/git-config.txt | 21 +++++- > builtin/config.c | 10 ++- > config.c | 26 +++++++ > config.h | 3 + > submodule.c | 29 ++++++++ > submodule.h | 6 ++ > t/t1311-superproject-config.sh | 124 +++++++++++++++++++++++++++++++++ > 7 files changed, 216 insertions(+), 3 deletions(-) > create mode 100755 t/t1311-superproject-config.sh > > 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. > ++ > +For reading options: read only from the superproject's > +`.git/config.superproject` file, even if run from a submodule of that > +superproject, rather than from all available files. > ++ > +See also <<FILES>>. > + > --local:: > For writing options: write to the repository `.git/config` file. > This is the default behavior. > @@ -283,7 +294,7 @@ The default is to use a pager. > FILES > ----- > > -If not set explicitly with `--file`, there are four files where > +If not set explicitly with `--file`, there are five files where > 'git config' will search for configuration options: > > $(prefix)/etc/gitconfig:: > @@ -301,6 +312,12 @@ $XDG_CONFIG_HOME/git/config:: > User-specific configuration file. Also called "global" > configuration file. > > +$GIT_DIR/config.superproject:: > + When `git config` is run from a project which is a submodule of another > + project, that superproject's $GIT_DIR will be used. Use this config file > + to set configurations which need to be the same across a superproject > + and all its submodules. > + > $GIT_DIR/config:: > Repository specific configuration file. > > 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); > } 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); > + strbuf_addstr(sb, gitdir); > + } > + > + strbuf_addstr(sb, "/config.superproject"); > +} > + > /* > * Parse environment variable 'k' as a boolean (in various > * possible spellings); if missing, use the default value 'def'. > @@ -1909,6 +1921,17 @@ static int do_git_config_sequence(const struct config_options *opts, > if (user_config && !access_or_die(user_config, R_OK, ACCESS_EACCES_OK)) > ret += git_config_from_file(fn, user_config, data); > > + current_parsing_scope = CONFIG_SCOPE_SUPERPROJECT; > + if (opts->git_dir && !opts->ignore_superproject) { > + > + struct strbuf superproject_gitdir = STRBUF_INIT; > + git_config_superproject(&superproject_gitdir, opts->git_dir); > + if (!access_or_die(superproject_gitdir.buf, R_OK, 0)) > + ret += git_config_from_file(fn, superproject_gitdir.buf, data); > + > + strbuf_release(&superproject_gitdir); > + } > + > current_parsing_scope = CONFIG_SCOPE_LOCAL; > if (!opts->ignore_repo && repo_config && > !access_or_die(repo_config, R_OK, 0)) > @@ -2027,6 +2050,7 @@ void read_very_early_config(config_fn_t cb, void *data) > > opts.respect_includes = 1; > opts.ignore_repo = 1; > + opts.ignore_superproject = 1; > opts.ignore_worktree = 1; > opts.ignore_cmdline = 1; > opts.system_gently = 1; > @@ -3515,6 +3539,8 @@ const char *config_scope_name(enum config_scope scope) > return "command"; > case CONFIG_SCOPE_GITMODULES: > return "gitmodules"; > + case CONFIG_SCOPE_SUPERPROJECT: > + return "superproject"; > default: > return "unknown"; > } > diff --git a/config.h b/config.h > index 535f5517b8..b42e1d13eb 100644 > --- a/config.h > +++ b/config.h > @@ -43,6 +43,7 @@ enum config_scope { > CONFIG_SCOPE_WORKTREE, > CONFIG_SCOPE_COMMAND, > CONFIG_SCOPE_GITMODULES, > + CONFIG_SCOPE_SUPERPROJECT, > }; > const char *config_scope_name(enum config_scope scope); > > @@ -84,6 +85,7 @@ typedef int (*config_parser_event_fn_t)(enum config_event_t type, > struct config_options { > unsigned int respect_includes : 1; > unsigned int ignore_repo : 1; > + unsigned int ignore_superproject : 1; > unsigned int ignore_worktree : 1; > unsigned int ignore_cmdline : 1; > unsigned int system_gently : 1; > @@ -319,6 +321,7 @@ int git_config_rename_section_in_file(const char *, const char *, const char *); > int git_config_copy_section(const char *, const char *); > int git_config_copy_section_in_file(const char *, const char *, const char *); > const char *git_etc_gitconfig(void); > +void git_config_superproject(struct strbuf *, const char *); > int git_env_bool(const char *, int); > unsigned long git_env_ulong(const char *, unsigned long); > int git_config_system(void); > diff --git a/submodule.c b/submodule.c > index 9767ba9893..92b00f8697 100644 > --- a/submodule.c > +++ b/submodule.c > @@ -2178,6 +2178,35 @@ void absorb_git_dir_into_superproject(const char *path, > } > } > > +int get_superproject_gitdir(struct strbuf *buf) > +{ > + struct strbuf sb = STRBUF_INIT; > + struct child_process cp = CHILD_PROCESS_INIT; > + int rc = 0; > + > + /* NEEDSWORK: this call also calls out to a subprocess! */ > + rc = get_superproject_working_tree(&sb); > + strbuf_release(&sb); > + > + if (!rc) > + return rc; > + > + prepare_submodule_repo_env_no_git_dir(&cp.env_array); > + > + strvec_pushl(&cp.args, "-C", "..", "rev-parse", "--absolute-git-dir", NULL); > + cp.git_cmd = 1; > + > + rc = capture_command(&cp, buf, 0); > + strbuf_trim_trailing_newline(buf); > + > + /* leave buf empty if we didn't have a superproject gitdir */ > + if (rc) > + strbuf_reset(buf); > + > + /* rc reflects the exit code of the rev-parse; invert into a bool */ > + return !rc; > +} > + > int get_superproject_working_tree(struct strbuf *buf) > { > struct child_process cp = CHILD_PROCESS_INIT; > 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. > + */ > +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 of > diff --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 @@ > +#!/bin/sh > + > +test_description='Test git config --superproject in different settings' > + > +. ./test-lib.sh > + > +# follow t7400's example and use the trash dir repo as a submodule to add > +submodurl=$(pwd -P) > + > +# since only the configs are modified, set up the repo structure only once > +test_expect_success 'setup repo structure' ' > + test_commit "base" && > + git submodule add "${submodurl}" sub/ && > + git commit -m "add a submodule" > +' > + > +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 > +' > + > +test_expect_success 'can add from super or sub' ' > + git config --superproject apple.species honeycrisp && > + git -C sub config --superproject banana.species cavendish && > + > + cat >expect <<-EOF && > + apple.species=honeycrisp > + banana.species=cavendish > + EOF > + > + git config --list >actual && > + grep -Ff expect actual && > + > + git -C sub config --list >actual && > + grep -Ff expect actual && > + > + rm .git/config.superproject > +' > + > +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 > +' > + > +test_expect_success 'can --edit superproject config' ' > + test_config core.editor "echo [foo]bar=baz >" && > + git config --edit --superproject && > + > + git config --get foo.bar && > + > + rm .git/config.superproject > +' > + > +test_expect_success 'can --show-origin the superproject config' ' > + git config --superproject --add foo.bar baz && > + > + git config --list --show-origin >actual && > + grep -F "config.superproject" actual && > + > + rm .git/config.superproject > +' > + > +test_expect_success 'can --show-scope the superproject config' ' > + git config --superproject --add foo.bar baz && > + > + git config --list --show-scope >actual && > + grep "superproject" actual && > + > + rm .git/config.superproject > +' > + > +test_expect_success 'pre-existing config applies to new submodule' ' > + git config --superproject --add foo.bar baz && > + > + git submodule add "${submodurl}" sub2/ && > + git commit -m "add a second submodule" && > + > + git -C sub2 config --get foo.bar && > + > + # clean up > + git reset HEAD^ && > + rm -fr sub2 && > + rm .git/config.superproject > +' > + > +# NEEDSWORK: submodule.c:get_superproject_working_tree doesn't support worktree > +test_expect_failure 'worktrees can still access config.superproject' ' > + git config --superproject --add foo.bar baz && > + > + git worktree add wt && > + ( > + cd wt && > + git config --get foo.bar > + ) && > + > + # clean up > + git worktree remove wt && > + rm .git/config.superproject > +' > + > +# This test deletes the submodule! Keep it at the end of the test suite. > +test_expect_success 'config.submodule works even with no submodules' ' > + # get rid of the submodule > + git reset HEAD^ && > + rm -fr sub && > + > + git config --superproject --add foo.bar baz && > + > + git config --get foo.bar && > + > + rm .git/config.superproject > +' > + > +test_done ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [RFC PATCH 2/2] config: add 'config.superproject' file 2021-04-09 11:10 ` Philip Oakley @ 2021-04-13 18:05 ` Emily Shaffer 0 siblings, 0 replies; 25+ messages in thread From: Emily Shaffer @ 2021-04-13 18:05 UTC (permalink / raw) To: Philip Oakley; +Cc: git On Fri, Apr 09, 2021 at 12:10:27PM +0100, Philip Oakley wrote: > > On 09/04/2021 00:39, Emily Shaffer wrote: > > Some configs, such as wrapper directives like gerrit.createChangeId, or > > forthcoming hook configs, should apply to a superproject as well as all > > its submodules. It may not be appropriate to apply them globally - for > > example, if the user also contributes to many projects which do not use > > the configs necessary for one project-with-submodules - and it may be > > burdensome to apply them locally to the superproject and each of its > > submodules. Even if the user runs 'git submodule foreach "git config > > --local foo.bar', if a new submodule is added later on, that config is > > not applied to the new submodule. > > > > It is also inappropriate to share the entire superproject config, since > > some items - like remote URLs or partial-clone filters - would not apply > > to a submodule. > > > > To make life easier for projects with many submodules, then, create a > > new "config.superproject" config scope, which is included in the config > > parse for the superproject as well as for all the submodules of that > > superproject. > > > > For the superproject, this new config file is equally local to the local > > config; for the submodule, the new config file is less local than the > > local config. So let's include it directly before the local config > > during the config parse. > > > > Signed-off-by: Emily Shaffer <emilyshaffer@google.com> > > --- > > Does this need an update to the `git config --show-origin --show-scope` > capability? It's included: > > --- a/config.c > > +++ b/config.c > > @@ -3515,6 +3539,8 @@ const char *config_scope_name(enum config_scope scope) > > return "command"; > > case CONFIG_SCOPE_GITMODULES: > > return "gitmodules"; > > + case CONFIG_SCOPE_SUPERPROJECT: > > + return "superproject"; > > default: > > return "unknown"; > > } > > diff --git a/config.h b/config.h > > index 535f5517b8..b42e1d13eb 100644 > > --- a/config.h > > +++ b/config.h > > @@ -43,6 +43,7 @@ enum config_scope { > > CONFIG_SCOPE_WORKTREE, > > CONFIG_SCOPE_COMMAND, > > CONFIG_SCOPE_GITMODULES, > > + CONFIG_SCOPE_SUPERPROJECT, > > }; > > diff --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 > > +test_expect_success 'can --show-origin the superproject config' ' > > + git config --superproject --add foo.bar baz && > > + > > + git config --list --show-origin >actual && > > + grep -F "config.superproject" actual && > > + > > + rm .git/config.superproject > > +' > > + > > +test_expect_success 'can --show-scope the superproject config' ' > > + git config --superproject --add foo.bar baz && > > + > > + git config --list --show-scope >actual && > > + grep "superproject" actual && > > + > > + rm .git/config.superproject > > +' - Emily ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [RFC PATCH 2/2] config: add 'config.superproject' file 2021-04-08 23:39 ` [RFC PATCH 2/2] config: add 'config.superproject' file Emily Shaffer 2021-04-09 11:10 ` Philip Oakley @ 2021-04-09 14:35 ` Matheus Tavares Bernardino 2021-04-09 22:29 ` Junio C Hamano 2021-04-13 18:48 ` Emily Shaffer 1 sibling, 2 replies; 25+ messages in thread From: Matheus Tavares Bernardino @ 2021-04-09 14:35 UTC (permalink / raw) To: Emily Shaffer; +Cc: git 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 <emilyshaffer@google.com> wrote: > > 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.) > 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); > } 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. > 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." > +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 of > diff --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/ ? ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [RFC PATCH 2/2] config: add 'config.superproject' file 2021-04-09 14:35 ` Matheus Tavares Bernardino @ 2021-04-09 22:29 ` Junio C Hamano 2021-04-13 19:45 ` Emily Shaffer 2021-04-13 18:48 ` Emily Shaffer 1 sibling, 1 reply; 25+ messages in thread From: Junio C Hamano @ 2021-04-09 22:29 UTC (permalink / raw) To: Matheus Tavares Bernardino; +Cc: Emily Shaffer, git Matheus Tavares Bernardino <matheus.bernardino@usp.br> writes: > 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 <emilyshaffer@google.com> wrote: >> >> 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). Another thing I am not sure about the design is that a repository can be shared as a submodule by more than one superprojects. The superprojects may want their submodule checkouts at different submodule commits, but that is something doable by having multiple worktrees connected to a single submodule repository. I think our design principle has been that it is perfectly OK for a superproject to be in total control if its submodules, but submodules should not even be aware of being used as a submodule by a superproject, and that allows a submodule repository to be shared by multiple superprojects. As "write to the superproject's X file" requires a submodule to know who THE superproject of itself is, this feature itself (not the implementation) feels somewhat iffy. ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [RFC PATCH 2/2] config: add 'config.superproject' file 2021-04-09 22:29 ` Junio C Hamano @ 2021-04-13 19:45 ` Emily Shaffer 0 siblings, 0 replies; 25+ messages in thread From: Emily Shaffer @ 2021-04-13 19:45 UTC (permalink / raw) To: Junio C Hamano; +Cc: Matheus Tavares Bernardino, git On Fri, Apr 09, 2021 at 03:29:46PM -0700, Junio C Hamano wrote: > > Matheus Tavares Bernardino <matheus.bernardino@usp.br> writes: > > > 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 <emilyshaffer@google.com> wrote: > >> > >> 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). > > Another thing I am not sure about the design is that a repository > can be shared as a submodule by more than one superprojects. The > superprojects may want their submodule checkouts at different > submodule commits, but that is something doable by having multiple > worktrees connected to a single submodule repository. I think the implementation as-written actually handles this sharing-via-worktree case you describe gracefully, as it discovers the gitdir belonging to the worktree above the worktree where it is being run now: superproject-a -> sub-a <gitdir at superproject-a/.git/modules/sub-a/> superproject-b -> sub-b <gitdir at superproject-a/.git/modules/sub-a/worktrees/sub-b/> In this case running `git config --list --superproject` in sub-a will yield superproject-a/.git/config.superproject and running it in sub-b will yield superproject-b/.git/config.superproject; that seems logical to me. If I am adding libc as a submodule via worktree to Git as well as, say, Wireshark, just to save me on disk space, I wouldn't want my Wireshark hooks to run in Git project or vice versa. > I think our design principle has been that it is perfectly OK for a > superproject to be in total control if its submodules, but > submodules should not even be aware of being used as a submodule by > a superproject, and that allows a submodule repository to be shared > by multiple superprojects. As "write to the superproject's X file" > requires a submodule to know who THE superproject of itself is, this > feature itself (not the implementation) feels somewhat iffy. As for this, I wonder what the reasoning is. I guess to simplify the code, and to make the behavior more predictable (for example, 'git commit' doesn't suddenly make a commit in some project that isn't this one)? One could imagine some really nice quality-of-life improvements if the submodule is allowed to know it's a submodule (even by a config, for example): - We could teach 'git status' to indicate the state when the submodule index is clean, but the superproject does not contain a commit pointing to the submodule's HEAD - which could still be considered a dirty state, since the change isn't associated with the larger project yet. I could imagine there might be other handy information related to submodule/superproject status we may want to display too. - We could teach 'git log' to decorate commits which are referred to by superproject commits, perhaps? - We could teach 'git push' to, by option or config, push the entire superproject-and-submodules package at once, to make it easier to coordinate changes across the whole superproject - One could envision some other niceties like 'git stash --whole-superproject' or similar, where a user could operate on the entire overall project (that is, the superproject and all its submodules) without needing to switch context away I don't think lacking these things would stop a user from doing something they want to do, but it does seem like they could make life more comfortable for a user developing in a project made up of many submodules. - Emily ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [RFC PATCH 2/2] config: add 'config.superproject' file 2021-04-09 14:35 ` Matheus Tavares Bernardino 2021-04-09 22:29 ` Junio C Hamano @ 2021-04-13 18:48 ` Emily Shaffer 1 sibling, 0 replies; 25+ messages in thread From: Emily Shaffer @ 2021-04-13 18:48 UTC (permalink / raw) To: Matheus Tavares Bernardino; +Cc: git On Fri, Apr 09, 2021 at 11:35:13AM -0300, Matheus Tavares Bernardino wrote: > > 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 <emilyshaffer@google.com> wrote: > > > > 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.) Yeah, this isn't very surprising. The code does essentially: parent_gitdir = git -C ../ rev-parse --git-dir config_parse_order += $parent_gitdir/config.superproject That is, in the nested submodules case, it's looking at .git/modules/sub/config.superproject - but 'sub' is getting its superproject config from .git/config.superproject and has no such file of its own. One way I could see to solve it would be to skip the "find parent gitdir" thing entirely: git -C ../ config --superproject --list >parent_superproject_cfg config_parse_order += parent_superproject_cfg config_parse_order += $my_own_gitdir/config.superproject The recursion is a little icky, I guess, but submodules are all recursive anyway, right? :) Hmm. > > > 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); Oh nice, thanks! > > > } 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. Since I didn't promise it in the documentation I included the reset here, but I see your comment just after this suggesting I should also promise it in the documentation ;) > > > 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." Seems reasonable. > > > +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 of > > diff --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. Oh sure, thanks. > > > +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/ ? Aaauuurgggh :) Thanks for the nits! - Emily ^ permalink raw reply [flat|nested] 25+ messages in thread
* Future structure of submodules and .git/, .git/modules/* organization 2021-04-08 23:39 [RFC PATCH 0/2] share a config between submodule and superproject Emily Shaffer 2021-04-08 23:39 ` [RFC PATCH 1/2] config: rename "submodule" scope to "gitmodules" Emily Shaffer 2021-04-08 23:39 ` [RFC PATCH 2/2] config: add 'config.superproject' file Emily Shaffer @ 2021-04-14 10:32 ` Ævar Arnfjörð Bjarmason 2021-04-15 21:25 ` Emily Shaffer 2021-04-15 21:41 ` Junio C Hamano 2021-04-23 0:15 ` [RFC PATCH v2 0/4] share a config between submodule and superproject Emily Shaffer 3 siblings, 2 replies; 25+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2021-04-14 10:32 UTC (permalink / raw) To: Emily Shaffer; +Cc: git On Fri, Apr 09 2021, Emily Shaffer wrote: > I'm hoping to work on some other submodule-centric stuff over the coming > months, and it might end up being very useful to be able to tell "am I a > submodule?" and "how do I talk to my superproject?" more generally - so > I'm really open to figuring out a better way than this, if folks have > ideas. > > Patch 1 is a small refactor that we can take or leave - I found > "SCOPE_SUBMODULE" to be pretty ambiguous, especially since it seems to > refer to configs from .gitmodules. Even though I decided that > "superproject" was a better name than "submodule" I still wasn't super > happy with the ambiguity. But we can drop it if folks don't want to > rename. This is less on your patch, and more on the larger work you're suggesting, but the two are kind of related. Skip to the paragraph starting with "But why" below for the relevance :) I very much wish that we could eventually make the use of submodules totally transparent, i.e. (taking the example of git.git): * You clone, and we just get objects from https://github.com/cr-marcstevens/sha1collisiondetection.git too * The fact that we have: 160000 commit 855827c583bc30645ba427885caa40c5b81764d2 sha1collisiondetection Would become totally invisible to most users unless they run some gutsy ls-tree/files comand. We used to have a full git dir at sha1collisiondetection/.git and all the UX issues that entailed (e.g. switching to an old commit without the submodule). Now it's a stub and the actual repo is at .git/modules/sha1collisiondetection/, so we're kind of partially there. * I would think that the next (but big) logical step would be to use some combination of delta islands, upcoming sparse indexes etc. to actually share the object stores of the parent and submodule. Things like "git fsck" which now just punt on COMMIT would need to become smarter, but e.g. we could repack (or not, with islands) between parent and submodule. I would think that this end goal makes more sense than the current status quo of teaching every command that needs to e.g. grep the tree to have a "--recurse-submodules". The distinction would be invisible to the likes of "git-grep". It would mean more complexity in e.g. "git commit", but we can imagine if you wanted a cross-submodule commit it could do those commits recursively, update parent COMMIT entries etc. (and even, optionally, push out the submodule changes). That particular thing being so ad-hoc is a *very* frequent pain point in submodule use. But why am I talking about this here when all you're suggesting is another config level? Well, I think (but have not carefully thought about) that this CONFIG_SCOPE_GITMODULES is probably a narrow net improvement now. If you set most options in your .git/config to you that's the same logical project, why shouldn't you get your diff setting or whatever because you cd'd to a submodule "in the same project" (from the view of the user). But I think that for a wider "improve submodules" effort it's worth someone (and right now, that sounds like it's you) thinking about where we're going with the feature. Maybe with some technical doc identifying the most common pain points, what we propose (or could envision) doing about them. So e.g. in this case, having per-submodule config could be a step forward, but it could also be one more step of walking in a circle. I.e. don't think any user asked for or wanted to stitch together multiple .git directories into one linked pseudo-checkout, that's ultimately something we're exposing as an implementation detail. If we no longer expose that implementation detail, would we be stuck supporting what's ultimately a workaround feature? None of that means we shouldn't have that one step forward that solves real problems today. But I think we should think about the end goal(s) sooner than later. E.g. in your case, do you *really* want another config level, or is it just the easiest way to get what you actually want, which is for a "git config" in the submodule dir to perhaps consider its .git/config and .git/modules/sha1collisiondetection/config as the same file for the purposes of config parsing? Sans things like the remote URLs etc. ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: Future structure of submodules and .git/, .git/modules/* organization 2021-04-14 10:32 ` Future structure of submodules and .git/, .git/modules/* organization Ævar Arnfjörð Bjarmason @ 2021-04-15 21:25 ` Emily Shaffer 2021-04-15 21:41 ` Junio C Hamano 1 sibling, 0 replies; 25+ messages in thread From: Emily Shaffer @ 2021-04-15 21:25 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason; +Cc: git On Wed, Apr 14, 2021 at 12:32:17PM +0200, Ævar Arnfjörð Bjarmason wrote: > On Fri, Apr 09 2021, Emily Shaffer wrote: > > > I'm hoping to work on some other submodule-centric stuff over the coming > > months, and it might end up being very useful to be able to tell "am I a > > submodule?" and "how do I talk to my superproject?" more generally - so > > I'm really open to figuring out a better way than this, if folks have > > ideas. > > > > Patch 1 is a small refactor that we can take or leave - I found > > "SCOPE_SUBMODULE" to be pretty ambiguous, especially since it seems to > > refer to configs from .gitmodules. Even though I decided that > > "superproject" was a better name than "submodule" I still wasn't super > > happy with the ambiguity. But we can drop it if folks don't want to > > rename. > > This is less on your patch, and more on the larger work you're > suggesting, but the two are kind of related. Skip to the paragraph > starting with "But why" below for the relevance :) > > I very much wish that we could eventually make the use of submodules > totally transparent, i.e. (taking the example of git.git): > > * You clone, and we just get objects from > https://github.com/cr-marcstevens/sha1collisiondetection.git too > > * The fact that we have: > > 160000 commit 855827c583bc30645ba427885caa40c5b81764d2 sha1collisiondetection > > Would become totally invisible to most users unless they run some > gutsy ls-tree/files comand. > > We used to have a full git dir at sha1collisiondetection/.git and all > the UX issues that entailed (e.g. switching to an old commit without > the submodule). > > Now it's a stub and the actual repo is at > .git/modules/sha1collisiondetection/, so we're kind of partially > there. Side note: when I was writing the tests for patch 2 in this series I noticed it was still really easy to end up with a full git dir at e.g. sha1collisiondetection/.git, if you are trying to create a new repo to use as a submodule (easily could be the case when working on a "greenfield" project and you're the original author). There is definitely a reason that I copied the (IMO) hack from the other submodule test suite using the trash directory as a remote for my new submodule. ;) I wonder whether I was just doing it wrong, or if we need some established flow (maybe with `git submodule` subcommand) to create a brand new submodule, not cloned from somewhere, and put its gitdir inside of .git/modules? > * I would think that the next (but big) logical step would be to use > some combination of delta islands, upcoming sparse indexes etc. to > actually share the object stores of the parent and submodule. Interesting - I'm trying to think of reasons not to and coming up blank, but I also don't have much firsthand experience with the area of the code that looks through the object store, so what do I know? > Things like "git fsck" which now just punt on COMMIT would need to > become smarter, but e.g. we could repack (or not, with islands) > between parent and submodule. > > I would think that this end goal makes more sense than the current > status quo of teaching every command that needs to e.g. grep the tree to > have a "--recurse-submodules". The distinction would be invisible to the > likes of "git-grep". Yeah, I see where you're going, I think. Teaching everyone --recurse-submodules or to respect the config setting (core.recurseSubmodules? whatever it is) is inherently fragile, since it relies on human reviewers to remember to chide patch authors to think of the submodules use case. Neat. > It would mean more complexity in e.g. "git commit", but we can imagine > if you wanted a cross-submodule commit it could do those commits > recursively, update parent COMMIT entries etc. (and even, optionally, > push out the submodule changes). That particular thing being so ad-hoc > is a *very* frequent pain point in submodule use. Yeah, it sounds like you're describing the approach I was hoping to use for commit-with-recursion: - Note each submodule with staged changes, as well as the superproject - (Optional? but might be nice) Open an editor with all the commit messages separated by scissors, so you can easily refer back to or modify the submodule commit messages while writing the superproject commit message - Generate all the submodule commits with the supplied commit-msgs - Take the commit IDs of all the newly created commits, stage them in the superproject, and generate the superproject commit with the supplied commit-msg Maybe the editor bit is too much, but Jonathan Nieder at least really liked that idea :) But the bit you're talking about - generating the submodules first and then staging and committing in the superproject "on the fly" - was the approach I was hoping to take. > > But why am I talking about this here when all you're suggesting is > another config level? > > Well, I think (but have not carefully thought about) that this > CONFIG_SCOPE_GITMODULES is probably a narrow net improvement now. If you > set most options in your .git/config to you that's the same logical > project, why shouldn't you get your diff setting or whatever because you > cd'd to a submodule "in the same project" (from the view of the user). > > But I think that for a wider "improve submodules" effort it's worth > someone (and right now, that sounds like it's you) thinking about where > we're going with the feature. Maybe with some technical doc identifying > the most common pain points, what we propose (or could envision) doing > about them. > > So e.g. in this case, having per-submodule config could be a step > forward, but it could also be one more step of walking in a circle. > > I.e. don't think any user asked for or wanted to stitch together > multiple .git directories into one linked pseudo-checkout, that's > ultimately something we're exposing as an implementation detail. If we > no longer expose that implementation detail, would we be stuck > supporting what's ultimately a workaround feature? > > None of that means we shouldn't have that one step forward that solves > real problems today. > > But I think we should think about the end goal(s) sooner than > later. Yeah, this is actually a good nudge for me. Internally we've got a big nice doc explaining all our submodule plans for the next 6-9 months - but I should probably get to sharing that with the list ;) I'd say to look for it either this Friday or next Friday. > E.g. in your case, do you *really* want another config level, or > is it just the easiest way to get what you actually want, which is for a > "git config" in the submodule dir to perhaps consider its .git/config > and .git/modules/sha1collisiondetection/config as the same file for the > purposes of config parsing? Sans things like the remote URLs etc. As for this specific case, I want what is in the patch. Using a new config file doesn't feel like a compromise to me - I actually would prefer users to be able to explicitly choose shared vs. repo-specific configs, rather than for we Git devs to implicitly decide which configs are fine to share and which aren't. (I could also see having explicitly shared or non-shared configs making it easier for wrappers to leverage the Git config infrastructure, without mirroring our own "list of configs to not share to submodule" for themselves.) This RFC is mostly here to enable shared hooks, as you might have guessed - but even with hooks, it's easy to imagine wanting a blend of inherited vs. per-repo hooks. For example, I want to inherit a hook to create a Gerrit Change-Id footer in my superproject and all my submodules, definitely - but if my superproject is written in C and includes a submodule which is in, I dunno, Rust or Zig or Perl or whatever people are writing these days, I definitely don't want to try and run my C linter from my superproject on my 15 Rust submodules - and I definitely don't want to disable it in each one. - Emily ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: Future structure of submodules and .git/, .git/modules/* organization 2021-04-14 10:32 ` Future structure of submodules and .git/, .git/modules/* organization Ævar Arnfjörð Bjarmason 2021-04-15 21:25 ` Emily Shaffer @ 2021-04-15 21:41 ` Junio C Hamano 1 sibling, 0 replies; 25+ messages in thread From: Junio C Hamano @ 2021-04-15 21:41 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason; +Cc: Emily Shaffer, git Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > I very much wish that we could eventually make the use of submodules > totally transparent, i.e. (taking the example of git.git): > > * You clone, and we just get objects from > https://github.com/cr-marcstevens/sha1collisiondetection.git too > > * The fact that we have: > > 160000 commit 855827c583bc30645ba427885caa40c5b81764d2 sha1collisiondetection > ... I am afraid that the story is not that simple (I wish it were). There are at last two opposing ways submodules are to be used. The original motivation was to borrow an external project as part of your project, and the way we use SHA1DC is fairly close to it (but not quite). In the context of such a usage git commit -m "message" --recurse-submodules would often not be an appropriate operation. A message that is suitable in the context of the entire project would not be in the context of the project that is bound to your project as a submodule, and for your changes to be reusable by the folks who own the borrowed project to make sense, your change should be defensible on its own, "it helps this project that happens to use you as a submodule" by itself is not all that convincing. The other way submodule often gets used is to artificially split a logically single project into many subdirectories and make them into separate repositories, the top-level project binding them as submodules. An submodule in such an arrangement may not even make sense as a standalone project---this pattern was only brought into usage because without the more recent inventions like lazy/partial clones and sparse checkouts, large projects did not fit within a single repository. With such an arrangement, of course it makes perfect sense for things like git commit -m "message" --recurse-submodules git grep --recurse-submodules to work as if you are working inside a single repository, by definition. You are splitting a logically single project into multiple submodules as a workaround, and then still wanting to treat them as a single project, after all. Supporting those who want to use "collection of submodules as if it were a single monolithic project" well is a worthy goal, but I do not think it is healthy to assume that is the only use and forget about use cases that would benefit from a clear boundary at submodules (e.g. not sharing commit log message, a change at the toplevel project may consist of multiple changes at the submoudle level, etc.). Thanks. ^ permalink raw reply [flat|nested] 25+ messages in thread
* [RFC PATCH v2 0/4] share a config between submodule and superproject 2021-04-08 23:39 [RFC PATCH 0/2] share a config between submodule and superproject Emily Shaffer ` (2 preceding siblings ...) 2021-04-14 10:32 ` Future structure of submodules and .git/, .git/modules/* organization Ævar Arnfjörð Bjarmason @ 2021-04-23 0:15 ` Emily Shaffer 2021-04-23 0:15 ` [RFC PATCH v2 1/4] config: rename "submodule" scope to "gitmodules" Emily Shaffer ` (4 more replies) 3 siblings, 5 replies; 25+ messages in thread From: Emily Shaffer @ 2021-04-23 0:15 UTC (permalink / raw) To: git Cc: Emily Shaffer, Ævar Arnfjörð Bjarmason, Junio C Hamano, Matheus Tavares Bernardino, Philip Oakley Hi all, With the second iteration, I bothered to make the tests pass (oops) - and I'm actually fairly unhappy with what I found. Patches 2 and 3 of this iteration clean up tests which were (accidentally) explicitly checking that no child processes were invoked besides the ones they expected, by way of strictly grepping traces (by test_cmp or by line counting). I don't like those tests as they were - to me, they felt like the brittle kind of white-box test - but I also got a stronger feeling that adding an additional child process or two during every Git invocation is a bad idea. I also saw a pretty significant increase in test run time: All tests successful. Files=927, Tests=24148, 693 wallclock secs ( 8.71 usr 2.05 sys + 3254.41 cusr 1571.78 csys = 4836.95 CPU) Result: PASS real 11m33.029s user 54m23.187s sys 26m13.857s vs before: All tests successful. Files=926, Tests=24138, 144 wallclock secs ( 8.14 usr 2.03 sys + 882.29 cusr 535.61 csys = 1428.07 CPU) Result: PASS real 2m24.116s user 14m50.499s sys 8m57.649s And that's on a Linux machine; as I understand it, invoking child processes can be even more painful on other operating systems. If we could be assured that this extra step (finding the parent's gitdir and checking that config) was only running when we know we're in a submodule, I'd be less worried, I think. And there are a couple other pieces in the big picture submodule plan I sent[1] around which would require repos to answer "am I a submodule?" So I think this series may need to be shelved pending an answer to that question - whether we *should* let submodules know they are submodules[2] to turn on more behavior, and if so, how we should implement that. - Emily [1] https://lore.kernel.org/git/YHofmWcIAidkvJiD@google.com [2] https://lore.kernel.org/git/xmqqk0pbm6qt.fsf@gitster.g Emily Shaffer (4): config: rename "submodule" scope to "gitmodules" t1510-repo-setup: don't use exact matching on traces t7006-pager.sh: more lenient trace checking config: add 'config.superproject' file Documentation/git-config.txt | 21 +++++- builtin/config.c | 9 ++- config.c | 28 +++++++- config.h | 5 +- submodule-config.c | 2 +- submodule.c | 29 +++++++++ submodule.h | 8 +++ t/t1311-superproject-config.sh | 116 +++++++++++++++++++++++++++++++++ t/t1510-repo-setup.sh | 2 +- t/t7006-pager.sh | 24 +++++-- 10 files changed, 230 insertions(+), 14 deletions(-) create mode 100755 t/t1311-superproject-config.sh -- 2.31.1.498.g6c1eba8ee3d-goog ^ permalink raw reply [flat|nested] 25+ messages in thread
* [RFC PATCH v2 1/4] config: rename "submodule" scope to "gitmodules" 2021-04-23 0:15 ` [RFC PATCH v2 0/4] share a config between submodule and superproject Emily Shaffer @ 2021-04-23 0:15 ` Emily Shaffer 2021-04-23 9:46 ` Phillip Wood 2021-04-23 0:15 ` [RFC PATCH v2 2/4] t1510-repo-setup: don't use exact matching on traces Emily Shaffer ` (3 subsequent siblings) 4 siblings, 1 reply; 25+ messages in thread From: Emily Shaffer @ 2021-04-23 0:15 UTC (permalink / raw) To: git; +Cc: Emily Shaffer To prepare for the addition of a new config scope which only applies to submodule projects, disambiguate "CONFIG_SCOPE_SUBMODULES". This scope refers to configs gathered from the .gitmodules file in the superproject, so just call it "gitmodules." Signed-off-by: Emily Shaffer <emilyshaffer@google.com> --- config.c | 4 ++-- config.h | 2 +- submodule-config.c | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/config.c b/config.c index 870d9534de..c8426ef3f3 100644 --- a/config.c +++ b/config.c @@ -3499,8 +3499,8 @@ const char *config_scope_name(enum config_scope scope) return "worktree"; case CONFIG_SCOPE_COMMAND: return "command"; - case CONFIG_SCOPE_SUBMODULE: - return "submodule"; + case CONFIG_SCOPE_GITMODULES: + return "gitmodules"; default: return "unknown"; } diff --git a/config.h b/config.h index 19a9adbaa9..535f5517b8 100644 --- a/config.h +++ b/config.h @@ -42,7 +42,7 @@ enum config_scope { CONFIG_SCOPE_LOCAL, CONFIG_SCOPE_WORKTREE, CONFIG_SCOPE_COMMAND, - CONFIG_SCOPE_SUBMODULE, + CONFIG_SCOPE_GITMODULES, }; const char *config_scope_name(enum config_scope scope); diff --git a/submodule-config.c b/submodule-config.c index f502505566..0e435e6fdd 100644 --- a/submodule-config.c +++ b/submodule-config.c @@ -637,7 +637,7 @@ static void config_from_gitmodules(config_fn_t fn, struct repository *repo, void { if (repo->worktree) { struct git_config_source config_source = { - 0, .scope = CONFIG_SCOPE_SUBMODULE + 0, .scope = CONFIG_SCOPE_GITMODULES }; const struct config_options opts = { 0 }; struct object_id oid; -- 2.31.1.498.g6c1eba8ee3d-goog ^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [RFC PATCH v2 1/4] config: rename "submodule" scope to "gitmodules" 2021-04-23 0:15 ` [RFC PATCH v2 1/4] config: rename "submodule" scope to "gitmodules" Emily Shaffer @ 2021-04-23 9:46 ` Phillip Wood 0 siblings, 0 replies; 25+ messages in thread From: Phillip Wood @ 2021-04-23 9:46 UTC (permalink / raw) To: Emily Shaffer, git Hi Emily On 23/04/2021 01:15, Emily Shaffer wrote: > To prepare for the addition of a new config scope which only applies to > submodule projects, disambiguate "CONFIG_SCOPE_SUBMODULES". This scope > refers to configs gathered from the .gitmodules file in the > superproject, so just call it "gitmodules." Am I right in thinking that this changes the output of `git config --show-scope`? If so I'm not sure it's a good idea as it would break any scripts that are parsing that output Best Wishes Phillip > Signed-off-by: Emily Shaffer <emilyshaffer@google.com> > --- > config.c | 4 ++-- > config.h | 2 +- > submodule-config.c | 2 +- > 3 files changed, 4 insertions(+), 4 deletions(-) > > diff --git a/config.c b/config.c > index 870d9534de..c8426ef3f3 100644 > --- a/config.c > +++ b/config.c > @@ -3499,8 +3499,8 @@ const char *config_scope_name(enum config_scope scope) > return "worktree"; > case CONFIG_SCOPE_COMMAND: > return "command"; > - case CONFIG_SCOPE_SUBMODULE: > - return "submodule"; > + case CONFIG_SCOPE_GITMODULES: > + return "gitmodules"; > default: > return "unknown"; > } > diff --git a/config.h b/config.h > index 19a9adbaa9..535f5517b8 100644 > --- a/config.h > +++ b/config.h > @@ -42,7 +42,7 @@ enum config_scope { > CONFIG_SCOPE_LOCAL, > CONFIG_SCOPE_WORKTREE, > CONFIG_SCOPE_COMMAND, > - CONFIG_SCOPE_SUBMODULE, > + CONFIG_SCOPE_GITMODULES, > }; > const char *config_scope_name(enum config_scope scope); > > diff --git a/submodule-config.c b/submodule-config.c > index f502505566..0e435e6fdd 100644 > --- a/submodule-config.c > +++ b/submodule-config.c > @@ -637,7 +637,7 @@ static void config_from_gitmodules(config_fn_t fn, struct repository *repo, void > { > if (repo->worktree) { > struct git_config_source config_source = { > - 0, .scope = CONFIG_SCOPE_SUBMODULE > + 0, .scope = CONFIG_SCOPE_GITMODULES > }; > const struct config_options opts = { 0 }; > struct object_id oid; > ^ permalink raw reply [flat|nested] 25+ messages in thread
* [RFC PATCH v2 2/4] t1510-repo-setup: don't use exact matching on traces 2021-04-23 0:15 ` [RFC PATCH v2 0/4] share a config between submodule and superproject Emily Shaffer 2021-04-23 0:15 ` [RFC PATCH v2 1/4] config: rename "submodule" scope to "gitmodules" Emily Shaffer @ 2021-04-23 0:15 ` Emily Shaffer 2021-04-23 9:59 ` Phillip Wood 2021-04-23 0:15 ` [RFC PATCH v2 3/4] t7006-pager.sh: more lenient trace checking Emily Shaffer ` (2 subsequent siblings) 4 siblings, 1 reply; 25+ messages in thread From: Emily Shaffer @ 2021-04-23 0:15 UTC (permalink / raw) To: git; +Cc: Emily Shaffer Tests which interrogate the exact underlying behavior of the code under test, instead of checking for the presence of desired side effects or calls, are a known testing antipattern. They are flaky as they need to be updated every time the underlying implementation changes. By using 'grep --fixed-strings --file <expect>' instead, we can check for the positive presence of lines we are sure should be happening, and ignore any additional things which may be happening around us (for example, additional child processes which are occurring unrelated to the code under test). Signed-off-by: Emily Shaffer <emilyshaffer@google.com> --- t/t1510-repo-setup.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/t/t1510-repo-setup.sh b/t/t1510-repo-setup.sh index bbfe05b8e4..8bd4f54d03 100755 --- a/t/t1510-repo-setup.sh +++ b/t/t1510-repo-setup.sh @@ -63,7 +63,7 @@ test_repo () { rm -f trace && GIT_TRACE_SETUP="$(pwd)/trace" git symbolic-ref HEAD >/dev/null && grep '^setup: ' trace >result && - test_cmp expected result + grep -Ff expected result ) } -- 2.31.1.498.g6c1eba8ee3d-goog ^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [RFC PATCH v2 2/4] t1510-repo-setup: don't use exact matching on traces 2021-04-23 0:15 ` [RFC PATCH v2 2/4] t1510-repo-setup: don't use exact matching on traces Emily Shaffer @ 2021-04-23 9:59 ` Phillip Wood 0 siblings, 0 replies; 25+ messages in thread From: Phillip Wood @ 2021-04-23 9:59 UTC (permalink / raw) To: Emily Shaffer, git Hi Emily On 23/04/2021 01:15, Emily Shaffer wrote: > Tests which interrogate the exact underlying behavior of the code under > test, instead of checking for the presence of desired side effects or > calls, are a known testing antipattern. They are flaky as they need to > be updated every time the underlying implementation changes. > > By using 'grep --fixed-strings --file <expect>' instead, we can check > for the positive presence of lines we are sure should be happening, and > ignore any additional things which may be happening around us (for > example, additional child processes which are occurring unrelated to the > code under test). > > Signed-off-by: Emily Shaffer <emilyshaffer@google.com> > --- > t/t1510-repo-setup.sh | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/t/t1510-repo-setup.sh b/t/t1510-repo-setup.sh > index bbfe05b8e4..8bd4f54d03 100755 > --- a/t/t1510-repo-setup.sh > +++ b/t/t1510-repo-setup.sh > @@ -63,7 +63,7 @@ test_repo () { > rm -f trace && > GIT_TRACE_SETUP="$(pwd)/trace" git symbolic-ref HEAD >/dev/null && > grep '^setup: ' trace >result && > - test_cmp expected result > + grep -Ff expected result If there is more than one line in expected (it's not clear from the limited context if there is) then grep will succeed if any of the lines match rather than requiring all the lines to match as test_cmp does. Best wishes Phillip > ) > } > > ^ permalink raw reply [flat|nested] 25+ messages in thread
* [RFC PATCH v2 3/4] t7006-pager.sh: more lenient trace checking 2021-04-23 0:15 ` [RFC PATCH v2 0/4] share a config between submodule and superproject Emily Shaffer 2021-04-23 0:15 ` [RFC PATCH v2 1/4] config: rename "submodule" scope to "gitmodules" Emily Shaffer 2021-04-23 0:15 ` [RFC PATCH v2 2/4] t1510-repo-setup: don't use exact matching on traces Emily Shaffer @ 2021-04-23 0:15 ` Emily Shaffer 2021-04-23 9:54 ` Phillip Wood 2021-04-23 0:15 ` [RFC PATCH v2 4/4] config: add 'config.superproject' file Emily Shaffer 2021-06-19 0:31 ` [PATCH v3 0/2] share a config between submodule and superproject Emily Shaffer 4 siblings, 1 reply; 25+ messages in thread From: Emily Shaffer @ 2021-04-23 0:15 UTC (permalink / raw) To: git; +Cc: Emily Shaffer A number of tests in t7006-pager.sh are, as a side effect, checking that 'git log' does not invoke any child processes besides the pager. There is no reason for that guarantee, and it is not explicitly the purpose of these tests, so let's make the checking more intelligent and flexible. child_start and child_exit events share a child ID - using that child ID, we can try to disambiguate which child_exit belongs to which child_start and limit our validation to only the pager's child process. Signed-off-by: Emily Shaffer <emilyshaffer@google.com> --- t/t7006-pager.sh | 24 ++++++++++++++++++------ 1 file changed, 18 insertions(+), 6 deletions(-) diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh index 0e7cf75435..ac2d91d56b 100755 --- a/t/t7006-pager.sh +++ b/t/t7006-pager.sh @@ -676,7 +676,9 @@ test_expect_success TTY 'git returns SIGPIPE on early pager exit' ' test_terminal git log fi && - grep child_exit trace.normal >child-exits && + PAGER_CHILD_ID=$(grep pager-used trace.normal | \ + sed -n "s/child_start\[\([0-9]\+\)\].*/\1/p") && + grep -F "child_exit["$PAGER_CHILD_ID"]" trace.normal >child-exits && test_line_count = 1 child-exits && grep " code:0 " child-exits && test_path_is_file pager-used @@ -697,7 +699,9 @@ test_expect_success TTY 'git returns SIGPIPE on early pager non-zero exit' ' test_terminal git log fi && - grep child_exit trace.normal >child-exits && + PAGER_CHILD_ID=$(grep pager-used trace.normal | \ + sed -n "s/child_start\[\([0-9]\+\)\].*/\1/p") && + grep -F "child_exit["$PAGER_CHILD_ID"]" trace.normal >child-exits && test_line_count = 1 child-exits && grep " code:1 " child-exits && test_path_is_file pager-used @@ -718,7 +722,9 @@ test_expect_success TTY 'git discards pager non-zero exit without SIGPIPE' ' test_terminal git log fi && - grep child_exit trace.normal >child-exits && + PAGER_CHILD_ID=$(grep pager-used trace.normal | \ + sed -n "s/child_start\[\([0-9]\+\)\].*/\1/p") && + grep -F "child_exit["$PAGER_CHILD_ID"]" trace.normal >child-exits && test_line_count = 1 child-exits && grep " code:1 " child-exits && test_path_is_file pager-used @@ -739,7 +745,9 @@ test_expect_success TTY 'git discards nonexisting pager without SIGPIPE' ' test_terminal git log fi && - grep child_exit trace.normal >child-exits && + PAGER_CHILD_ID=$(grep does-not-exist trace.normal | \ + sed -n "s/child_start\[\([0-9]\+\)\].*/\1/p") && + grep -F "child_exit["$PAGER_CHILD_ID"]" trace.normal >child-exits && test_line_count = 1 child-exits && grep " code:127 " child-exits && test_path_is_file pager-used @@ -760,7 +768,9 @@ test_expect_success TTY 'git attempts to page to nonexisting pager command, gets test_terminal git log fi && - grep child_exit trace.normal >child-exits && + PAGER_CHILD_ID=$(grep does-not-exist trace.normal | \ + sed -n "s/child_start\[\([0-9]\+\)\].*/\1/p") && + grep -F "child_exit["$PAGER_CHILD_ID"]" trace.normal >child-exits && test_line_count = 1 child-exits && grep " code:-1 " child-exits ' @@ -780,7 +790,9 @@ test_expect_success TTY 'git returns SIGPIPE on propagated signals from pager' ' test_terminal git log fi && - grep child_exit trace.normal >child-exits && + PAGER_CHILD_ID=$(grep pager-used trace.normal | \ + sed -n "s/child_start\[\([0-9]\+\)\].*/\1/p") && + grep -F "child_exit["$PAGER_CHILD_ID"]" trace.normal >child-exits && test_line_count = 1 child-exits && grep " code:143 " child-exits && test_path_is_file pager-used -- 2.31.1.498.g6c1eba8ee3d-goog ^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [RFC PATCH v2 3/4] t7006-pager.sh: more lenient trace checking 2021-04-23 0:15 ` [RFC PATCH v2 3/4] t7006-pager.sh: more lenient trace checking Emily Shaffer @ 2021-04-23 9:54 ` Phillip Wood 2021-04-23 12:45 ` Phillip Wood 0 siblings, 1 reply; 25+ messages in thread From: Phillip Wood @ 2021-04-23 9:54 UTC (permalink / raw) To: Emily Shaffer, git Hi Emily On 23/04/2021 01:15, Emily Shaffer wrote: > A number of tests in t7006-pager.sh are, as a side effect, checking that > 'git log' does not invoke any child processes besides the pager. There > is no reason for that guarantee, and it is not explicitly the purpose of > these tests, so let's make the checking more intelligent and flexible. > > child_start and child_exit events share a child ID - using that child > ID, we can try to disambiguate which child_exit belongs to which > child_start and limit our validation to only the pager's child process. I've not looked at this test file properly but if we want to check that the pager is invoked can we use a script that writes a file when it has been run as the pager and just cleanup that file at the end of each test case? > Signed-off-by: Emily Shaffer <emilyshaffer@google.com> > --- > t/t7006-pager.sh | 24 ++++++++++++++++++------ > 1 file changed, 18 insertions(+), 6 deletions(-) > > diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh > index 0e7cf75435..ac2d91d56b 100755 > --- a/t/t7006-pager.sh > +++ b/t/t7006-pager.sh > @@ -676,7 +676,9 @@ test_expect_success TTY 'git returns SIGPIPE on early pager exit' ' > test_terminal git log > fi && > > - grep child_exit trace.normal >child-exits && > + PAGER_CHILD_ID=$(grep pager-used trace.normal | \ > + sed -n "s/child_start\[\([0-9]\+\)\].*/\1/p") && If you want to save a process you could use sed to do the job of the grep command. I think this should do it sed -n -e "/child_exit/ {" -e "s/child_start\[\([0-9]\+\)\].*/\1/p" -e "}" > + grep -F "child_exit["$PAGER_CHILD_ID"]" trace.normal >child-exits && Why is $PAGER_CHILD_ID unquoted? Best Wishes Phillip > test_line_count = 1 child-exits && > grep " code:0 " child-exits && > test_path_is_file pager-used > @@ -697,7 +699,9 @@ test_expect_success TTY 'git returns SIGPIPE on early pager non-zero exit' ' > test_terminal git log > fi && > > - grep child_exit trace.normal >child-exits && > + PAGER_CHILD_ID=$(grep pager-used trace.normal | \ > + sed -n "s/child_start\[\([0-9]\+\)\].*/\1/p") && > + grep -F "child_exit["$PAGER_CHILD_ID"]" trace.normal >child-exits && > test_line_count = 1 child-exits && > grep " code:1 " child-exits && > test_path_is_file pager-used > @@ -718,7 +722,9 @@ test_expect_success TTY 'git discards pager non-zero exit without SIGPIPE' ' > test_terminal git log > fi && > > - grep child_exit trace.normal >child-exits && > + PAGER_CHILD_ID=$(grep pager-used trace.normal | \ > + sed -n "s/child_start\[\([0-9]\+\)\].*/\1/p") && > + grep -F "child_exit["$PAGER_CHILD_ID"]" trace.normal >child-exits && > test_line_count = 1 child-exits && > grep " code:1 " child-exits && > test_path_is_file pager-used > @@ -739,7 +745,9 @@ test_expect_success TTY 'git discards nonexisting pager without SIGPIPE' ' > test_terminal git log > fi && > > - grep child_exit trace.normal >child-exits && > + PAGER_CHILD_ID=$(grep does-not-exist trace.normal | \ > + sed -n "s/child_start\[\([0-9]\+\)\].*/\1/p") && > + grep -F "child_exit["$PAGER_CHILD_ID"]" trace.normal >child-exits && > test_line_count = 1 child-exits && > grep " code:127 " child-exits && > test_path_is_file pager-used > @@ -760,7 +768,9 @@ test_expect_success TTY 'git attempts to page to nonexisting pager command, gets > test_terminal git log > fi && > > - grep child_exit trace.normal >child-exits && > + PAGER_CHILD_ID=$(grep does-not-exist trace.normal | \ > + sed -n "s/child_start\[\([0-9]\+\)\].*/\1/p") && > + grep -F "child_exit["$PAGER_CHILD_ID"]" trace.normal >child-exits && > test_line_count = 1 child-exits && > grep " code:-1 " child-exits > ' > @@ -780,7 +790,9 @@ test_expect_success TTY 'git returns SIGPIPE on propagated signals from pager' ' > test_terminal git log > fi && > > - grep child_exit trace.normal >child-exits && > + PAGER_CHILD_ID=$(grep pager-used trace.normal | \ > + sed -n "s/child_start\[\([0-9]\+\)\].*/\1/p") && > + grep -F "child_exit["$PAGER_CHILD_ID"]" trace.normal >child-exits && > test_line_count = 1 child-exits && > grep " code:143 " child-exits && > test_path_is_file pager-used > ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [RFC PATCH v2 3/4] t7006-pager.sh: more lenient trace checking 2021-04-23 9:54 ` Phillip Wood @ 2021-04-23 12:45 ` Phillip Wood 0 siblings, 0 replies; 25+ messages in thread From: Phillip Wood @ 2021-04-23 12:45 UTC (permalink / raw) To: Emily Shaffer, git Hi Emily On 23/04/2021 10:54, Phillip Wood wrote: > Hi Emily > [...] >> diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh >> index 0e7cf75435..ac2d91d56b 100755 >> --- a/t/t7006-pager.sh >> +++ b/t/t7006-pager.sh >> @@ -676,7 +676,9 @@ test_expect_success TTY 'git returns SIGPIPE on >> early pager exit' ' >> test_terminal git log >> fi && >> - grep child_exit trace.normal >child-exits && >> + PAGER_CHILD_ID=$(grep pager-used trace.normal | \ >> + sed -n "s/child_start\[\([0-9]\+\)\].*/\1/p") && > > If you want to save a process you could use sed to do the job of the > grep command. I think this should do it > > sed -n -e "/child_exit/ {" -e "s/child_start\[\([0-9]\+\)\].*/\1/p" -e "}" I must have been half asleep when I wrote that, there is no need for the braces and the initial match is wrong it should be sed -n "/pager-used/s/child_start\[\([0-9]\+\)\].*/\1/p" Best Wishes Phillip >> + grep -F "child_exit["$PAGER_CHILD_ID"]" trace.normal >child-exits && > > Why is $PAGER_CHILD_ID unquoted? > > Best Wishes > > Phillip > >> test_line_count = 1 child-exits && >> grep " code:0 " child-exits && >> test_path_is_file pager-used >> @@ -697,7 +699,9 @@ test_expect_success TTY 'git returns SIGPIPE on >> early pager non-zero exit' ' >> test_terminal git log >> fi && >> - grep child_exit trace.normal >child-exits && >> + PAGER_CHILD_ID=$(grep pager-used trace.normal | \ >> + sed -n "s/child_start\[\([0-9]\+\)\].*/\1/p") && >> + grep -F "child_exit["$PAGER_CHILD_ID"]" trace.normal >child-exits && >> test_line_count = 1 child-exits && >> grep " code:1 " child-exits && >> test_path_is_file pager-used >> @@ -718,7 +722,9 @@ test_expect_success TTY 'git discards pager >> non-zero exit without SIGPIPE' ' >> test_terminal git log >> fi && >> - grep child_exit trace.normal >child-exits && >> + PAGER_CHILD_ID=$(grep pager-used trace.normal | \ >> + sed -n "s/child_start\[\([0-9]\+\)\].*/\1/p") && >> + grep -F "child_exit["$PAGER_CHILD_ID"]" trace.normal >child-exits && >> test_line_count = 1 child-exits && >> grep " code:1 " child-exits && >> test_path_is_file pager-used >> @@ -739,7 +745,9 @@ test_expect_success TTY 'git discards nonexisting >> pager without SIGPIPE' ' >> test_terminal git log >> fi && >> - grep child_exit trace.normal >child-exits && >> + PAGER_CHILD_ID=$(grep does-not-exist trace.normal | \ >> + sed -n "s/child_start\[\([0-9]\+\)\].*/\1/p") && >> + grep -F "child_exit["$PAGER_CHILD_ID"]" trace.normal >child-exits && >> test_line_count = 1 child-exits && >> grep " code:127 " child-exits && >> test_path_is_file pager-used >> @@ -760,7 +768,9 @@ test_expect_success TTY 'git attempts to page to >> nonexisting pager command, gets >> test_terminal git log >> fi && >> - grep child_exit trace.normal >child-exits && >> + PAGER_CHILD_ID=$(grep does-not-exist trace.normal | \ >> + sed -n "s/child_start\[\([0-9]\+\)\].*/\1/p") && >> + grep -F "child_exit["$PAGER_CHILD_ID"]" trace.normal >child-exits && >> test_line_count = 1 child-exits && >> grep " code:-1 " child-exits >> ' >> @@ -780,7 +790,9 @@ test_expect_success TTY 'git returns SIGPIPE on >> propagated signals from pager' ' >> test_terminal git log >> fi && >> - grep child_exit trace.normal >child-exits && >> + PAGER_CHILD_ID=$(grep pager-used trace.normal | \ >> + sed -n "s/child_start\[\([0-9]\+\)\].*/\1/p") && >> + grep -F "child_exit["$PAGER_CHILD_ID"]" trace.normal >child-exits && >> test_line_count = 1 child-exits && >> grep " code:143 " child-exits && >> test_path_is_file pager-used >> > ^ permalink raw reply [flat|nested] 25+ messages in thread
* [RFC PATCH v2 4/4] config: add 'config.superproject' file 2021-04-23 0:15 ` [RFC PATCH v2 0/4] share a config between submodule and superproject Emily Shaffer ` (2 preceding siblings ...) 2021-04-23 0:15 ` [RFC PATCH v2 3/4] t7006-pager.sh: more lenient trace checking Emily Shaffer @ 2021-04-23 0:15 ` Emily Shaffer 2021-04-23 12:08 ` Johannes Schindelin 2021-06-19 0:31 ` [PATCH v3 0/2] share a config between submodule and superproject Emily Shaffer 4 siblings, 1 reply; 25+ messages in thread From: Emily Shaffer @ 2021-04-23 0:15 UTC (permalink / raw) To: git; +Cc: Emily Shaffer Some configs, such as wrapper directives like gerrit.createChangeId, or forthcoming hook configs, should apply to a superproject as well as all its submodules. It may not be appropriate to apply them globally - for example, if the user also contributes to many projects which do not use the configs necessary for one project-with-submodules - and it may be burdensome to apply them locally to the superproject and each of its submodules. Even if the user runs 'git submodule foreach "git config --local foo.bar', if a new submodule is added later on, that config is not applied to the new submodule. It is also inappropriate to share the entire superproject config, since some items - like remote URLs or partial-clone filters - would not apply to a submodule. To make life easier for projects with many submodules, then, create a new "config.superproject" config scope, which is included in the config parse for the superproject as well as for all the submodules of that superproject. For the superproject, this new config file is equally local to the local config; for the submodule, the new config file is less local than the local config. So let's include it directly before the local config during the config parse. Signed-off-by: Emily Shaffer <emilyshaffer@google.com> --- Notes: Since v1: - Used test_when_finished liberally in tests instead of bare cleanup steps - Fixed some nits regarding xstrdup instead of strbuf_detach. One thing that I thought about but did not implement: rather than finding the path to the superproject's gitdir, you could imagine gathering the config by making a call out to 'git -C ../ config' - but on second thought, it seems like that will make it harder to edit. However, if we don't want to be able to edit superproject config from a submodule, that might be okay... (This approach could make 'git config --show-origin' harder to implement, though, I think.) So I did not make any changes about that. Documentation/git-config.txt | 21 +++++- builtin/config.c | 9 ++- config.c | 24 +++++++ config.h | 3 + submodule.c | 29 +++++++++ submodule.h | 8 +++ t/t1311-superproject-config.sh | 116 +++++++++++++++++++++++++++++++++ 7 files changed, 207 insertions(+), 3 deletions(-) create mode 100755 t/t1311-superproject-config.sh 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. ++ +For reading options: read only from the superproject's +`.git/config.superproject` file, even if run from a submodule of that +superproject, rather than from all available files. ++ +See also <<FILES>>. + --local:: For writing options: write to the repository `.git/config` file. This is the default behavior. @@ -283,7 +294,7 @@ The default is to use a pager. FILES ----- -If not set explicitly with `--file`, there are four files where +If not set explicitly with `--file`, there are five files where 'git config' will search for configuration options: $(prefix)/etc/gitconfig:: @@ -301,6 +312,12 @@ $XDG_CONFIG_HOME/git/config:: User-specific configuration file. Also called "global" configuration file. +$GIT_DIR/config.superproject:: + When `git config` is run from a project which is a submodule of another + project, that superproject's $GIT_DIR will be used. Use this config file + to set configurations which need to be the same across a superproject + and all its submodules. + $GIT_DIR/config:: Repository specific configuration file. diff --git a/builtin/config.c b/builtin/config.c index f71fa39b38..f38a6823c7 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,11 @@ 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 = strbuf_detach(&superproject_cfg, NULL); + given_config_source.scope = CONFIG_SCOPE_SUPERPROJECT; } 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 c8426ef3f3..3f62cd815b 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; @@ -1838,6 +1839,15 @@ 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_addstr(sb, gitdir); + + strbuf_addstr(sb, "/config.superproject"); +} + /* * Parse environment variable 'k' as a boolean (in various * possible spellings); if missing, use the default value 'def'. @@ -1895,6 +1905,17 @@ static int do_git_config_sequence(const struct config_options *opts, if (user_config && !access_or_die(user_config, R_OK, ACCESS_EACCES_OK)) ret += git_config_from_file(fn, user_config, data); + current_parsing_scope = CONFIG_SCOPE_SUPERPROJECT; + if (opts->git_dir && !opts->ignore_superproject) { + + struct strbuf superproject_gitdir = STRBUF_INIT; + git_config_superproject(&superproject_gitdir, opts->git_dir); + if (!access_or_die(superproject_gitdir.buf, R_OK, 0)) + ret += git_config_from_file(fn, superproject_gitdir.buf, data); + + strbuf_release(&superproject_gitdir); + } + current_parsing_scope = CONFIG_SCOPE_LOCAL; if (!opts->ignore_repo && repo_config && !access_or_die(repo_config, R_OK, 0)) @@ -2013,6 +2034,7 @@ void read_very_early_config(config_fn_t cb, void *data) opts.respect_includes = 1; opts.ignore_repo = 1; + opts.ignore_superproject = 1; opts.ignore_worktree = 1; opts.ignore_cmdline = 1; opts.system_gently = 1; @@ -3501,6 +3523,8 @@ const char *config_scope_name(enum config_scope scope) return "command"; case CONFIG_SCOPE_GITMODULES: return "gitmodules"; + case CONFIG_SCOPE_SUPERPROJECT: + return "superproject"; default: return "unknown"; } diff --git a/config.h b/config.h index 535f5517b8..b42e1d13eb 100644 --- a/config.h +++ b/config.h @@ -43,6 +43,7 @@ enum config_scope { CONFIG_SCOPE_WORKTREE, CONFIG_SCOPE_COMMAND, CONFIG_SCOPE_GITMODULES, + CONFIG_SCOPE_SUPERPROJECT, }; const char *config_scope_name(enum config_scope scope); @@ -84,6 +85,7 @@ typedef int (*config_parser_event_fn_t)(enum config_event_t type, struct config_options { unsigned int respect_includes : 1; unsigned int ignore_repo : 1; + unsigned int ignore_superproject : 1; unsigned int ignore_worktree : 1; unsigned int ignore_cmdline : 1; unsigned int system_gently : 1; @@ -319,6 +321,7 @@ int git_config_rename_section_in_file(const char *, const char *, const char *); int git_config_copy_section(const char *, const char *); int git_config_copy_section_in_file(const char *, const char *, const char *); const char *git_etc_gitconfig(void); +void git_config_superproject(struct strbuf *, const char *); int git_env_bool(const char *, int); unsigned long git_env_ulong(const char *, unsigned long); int git_config_system(void); diff --git a/submodule.c b/submodule.c index 9767ba9893..92b00f8697 100644 --- a/submodule.c +++ b/submodule.c @@ -2178,6 +2178,35 @@ void absorb_git_dir_into_superproject(const char *path, } } +int get_superproject_gitdir(struct strbuf *buf) +{ + struct strbuf sb = STRBUF_INIT; + struct child_process cp = CHILD_PROCESS_INIT; + int rc = 0; + + /* NEEDSWORK: this call also calls out to a subprocess! */ + rc = get_superproject_working_tree(&sb); + strbuf_release(&sb); + + if (!rc) + return rc; + + prepare_submodule_repo_env_no_git_dir(&cp.env_array); + + strvec_pushl(&cp.args, "-C", "..", "rev-parse", "--absolute-git-dir", NULL); + cp.git_cmd = 1; + + rc = capture_command(&cp, buf, 0); + strbuf_trim_trailing_newline(buf); + + /* leave buf empty if we didn't have a superproject gitdir */ + if (rc) + strbuf_reset(buf); + + /* rc reflects the exit code of the rev-parse; invert into a bool */ + return !rc; +} + int get_superproject_working_tree(struct strbuf *buf) { struct child_process cp = CHILD_PROCESS_INIT; diff --git a/submodule.h b/submodule.h index 4ac6e31cf1..cc89d5e3fa 100644 --- a/submodule.h +++ b/submodule.h @@ -149,6 +149,14 @@ 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 a submodule, return 1 and fill buf with the absolute + * path to the superproject's gitdir. If this repository is not a submodule of + * another repository, return 0 and leave buf untouched. + */ +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 of diff --git a/t/t1311-superproject-config.sh b/t/t1311-superproject-config.sh new file mode 100755 index 0000000000..74b6ca2cac --- /dev/null +++ b/t/t1311-superproject-config.sh @@ -0,0 +1,116 @@ +#!/bin/sh + +test_description='Test git config --superproject in different settings' + +. ./test-lib.sh + +# follow t7400's example and use the trash dir repo as a submodule to add +submodurl=$(pwd -P) + +# since only the configs are modified, set up the repo structure only once +test_expect_success 'setup repo structure' ' + test_commit "base" && + git submodule add "${submodurl}" sub/ && + git commit -m "add a submodule" +' + +test_expect_success 'superproject config applies to super and submodule' ' + cat >.git/config.superproject <<-EOF && + [foo] + bar = baz + EOF + test_when_finished rm .git/config.superproject && + + git config --get foo.bar && + git -C sub config --get foo.bar + +' + +test_expect_success 'can add from super or sub' ' + git config --superproject apple.species honeycrisp && + git -C sub config --superproject banana.species cavendish && + test_when_finished rm .git/config.superproject && + + cat >expect <<-EOF && + apple.species=honeycrisp + banana.species=cavendish + EOF + + git config --list >actual && + grep -Ff expect actual && + + git -C sub config --list >actual && + grep -Ff expect actual +' + +test_expect_success 'can --unset from super or sub' ' + git config --superproject apple.species honeycrisp && + git -C sub config --superproject banana.species cavendish && + test_when_finished rm .git/config.superproject && + + git config --unset --superproject banana.species && + git -C sub config --unset --superproject apple.species +' + +test_expect_success 'can --edit superproject config' ' + test_config core.editor "echo [foo]bar=baz >" && + git config --edit --superproject && + test_when_finished rm .git/config.superproject && + + git config --get foo.bar +' + +test_expect_success 'can --show-origin the superproject config' ' + git config --superproject --add foo.bar baz && + test_when_finished rm .git/config.superproject && + + git config --list --show-origin >actual && + grep -F "config.superproject" actual +' + +test_expect_success 'can --show-scope the superproject config' ' + git config --superproject --add foo.bar baz && + test_when_finished rm .git/config.superproject && + + git config --list --show-scope >actual && + grep "superproject" actual +' + +test_expect_success 'pre-existing config applies to new submodule' ' + git config --superproject --add foo.bar baz && + test_when_finished rm .git/config.superproject && + + git submodule add "${submodurl}" sub2/ && + test_when_finished rm -fr sub2 && + git commit -m "add a second submodule" && + test_when_finished git reset HEAD^ && + + git -C sub2 config --get foo.bar +' + +# NEEDSWORK: submodule.c:get_superproject_working_tree doesn't support worktree +test_expect_failure 'worktrees can still access config.superproject' ' + git config --superproject --add foo.bar baz && + test_when_finished rm .git/config.superproject && + + git worktree add wt && + test_when_finished git worktree remove wt && + ( + cd wt && + git config --get foo.bar + ) +' + +# This test deletes the submodule! Keep it at the end of the test suite. +test_expect_success 'config.superproject works even with no submodules' ' + # get rid of the submodule + git reset HEAD^ && + rm -fr sub && + + git config --superproject --add foo.bar baz && + test_when_finished rm .git/config.superproject && + + git config --get foo.bar +' + +test_done -- 2.31.1.498.g6c1eba8ee3d-goog ^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [RFC PATCH v2 4/4] config: add 'config.superproject' file 2021-04-23 0:15 ` [RFC PATCH v2 4/4] config: add 'config.superproject' file Emily Shaffer @ 2021-04-23 12:08 ` Johannes Schindelin 0 siblings, 0 replies; 25+ messages in thread From: Johannes Schindelin @ 2021-04-23 12:08 UTC (permalink / raw) To: Emily Shaffer; +Cc: git Hi Emily, On Thu, 22 Apr 2021, Emily Shaffer wrote: > Some configs, such as wrapper directives like gerrit.createChangeId, or > forthcoming hook configs, should apply to a superproject as well as all > its submodules. It may not be appropriate to apply them globally - for > example, if the user also contributes to many projects which do not use > the configs necessary for one project-with-submodules - and it may be > burdensome to apply them locally to the superproject and each of its > submodules. Even if the user runs 'git submodule foreach "git config > --local foo.bar', if a new submodule is added later on, that config is > not applied to the new submodule. > > It is also inappropriate to share the entire superproject config, since > some items - like remote URLs or partial-clone filters - would not apply > to a submodule. > > To make life easier for projects with many submodules, then, create a > new "config.superproject" config scope, which is included in the config > parse for the superproject as well as for all the submodules of that > superproject. > > For the superproject, this new config file is equally local to the local > config; for the submodule, the new config file is less local than the > local config. So let's include it directly before the local config > during the config parse. > > Signed-off-by: Emily Shaffer <emilyshaffer@google.com> > --- > > Notes: > Since v1: > > - Used test_when_finished liberally in tests instead of bare cleanup steps > - Fixed some nits regarding xstrdup instead of strbuf_detach. > > One thing that I thought about but did not implement: rather than finding the > path to the superproject's gitdir, you could imagine gathering the config by > making a call out to 'git -C ../ config' - but on second thought, it seems like > that will make it harder to edit. However, if we don't want to be able to edit > superproject config from a submodule, that might be okay... (This approach could > make 'git config --show-origin' harder to implement, though, I think.) So I did > not make any changes about that. Hmm. Have you thought about worktrees of subprojects that happen to be outside the superproject's directory tree? I also wonder whether it is necessary to change Git at all, as a well-crafted `[includeIf "gitdir:/path/to/superproject/**"]` should do the trick, but without complicating the config code even further. Ciao, Johannes ^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH v3 0/2] share a config between submodule and superproject 2021-04-23 0:15 ` [RFC PATCH v2 0/4] share a config between submodule and superproject Emily Shaffer ` (3 preceding siblings ...) 2021-04-23 0:15 ` [RFC PATCH v2 4/4] config: add 'config.superproject' file Emily Shaffer @ 2021-06-19 0:31 ` Emily Shaffer 2021-06-19 0:31 ` [PATCH v3 1/2] config: rename "submodule" scope to "gitmodules" Emily Shaffer 2021-06-19 0:31 ` [PATCH v3 2/2] config: add 'config.superproject' file Emily Shaffer 4 siblings, 2 replies; 25+ messages in thread From: Emily Shaffer @ 2021-06-19 0:31 UTC (permalink / raw) To: git Cc: Emily Shaffer, Ævar Arnfjörð Bjarmason, Junio C Hamano, Matheus Tavares Bernardino, Philip Oakley Now based on es/superproject-aware-submodules. By using the cached path to the superproject, I was able to drop the extra subprocess(es) to find out the superproject's gitdir. That means this change should now be relatively low overhead, and we can discuss things like naming, recursion, etc. to our hearts' content. Since v2 of this series, I have rebased the series onto es/superproject-aware-submodules and made appropriate changes, including dropping v2 patches 2 and 3. However, I think this solution answers a handful of the comments on v2: - dscho asked about submodules who live outside of the superproject's tree; assuming the submodule was created in one of the ways covered by superproject-aware-submodules, it should be a nonissue now. - Phillip had comments on patches 2 and 3; they're not necessary to make the tests pass anymore, because there is no longer an additional process cluttering up the traces as a result of this series. I did not address Phillip's comment about breaking scripts by changing the config scope name, but I think it's probably valid, so maybe patch 1 also should go (or be replaced by a robust comment). I'm hoping this can also serve as a nice example of what https://lore.kernel.org/git/20210616004508.87186-1-emilyshaffer@google.com can enable. Happy CI: https://github.com/nasamuffin/git/actions/runs/951382014 Thanks, - Emily Emily Shaffer (2): config: rename "submodule" scope to "gitmodules" config: add 'config.superproject' file Documentation/git-config.txt | 21 +++++- builtin/config.c | 9 ++- config.c | 32 ++++++++- config.h | 5 +- submodule-config.c | 2 +- t/t1311-superproject-config.sh | 116 +++++++++++++++++++++++++++++++++ 6 files changed, 178 insertions(+), 7 deletions(-) create mode 100755 t/t1311-superproject-config.sh Range-diff against v2: 1: 785f961f4c = 1: cddd53e33a config: rename "submodule" scope to "gitmodules" 2: b4d853e261 ! 2: 3eaca59b65 config: add 'config.superproject' file @@ builtin/config.c: static struct option builtin_config_options[] = { OPT_STRING('f', "file", &given_config_source.file, N_("file"), N_("use given config file")), @@ builtin/config.c: 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.file = git_system_config(); given_config_source.scope = CONFIG_SCOPE_SYSTEM; + } else if (use_superproject_config) { + struct strbuf superproject_cfg = STRBUF_INIT; @@ config.c struct config_source { struct config_source *prev; -@@ config.c: const char *git_etc_gitconfig(void) - return system_wide; +@@ config.c: void git_global_config(char **user_out, char **xdg_out) + *xdg_out = xdg_config; } +void git_config_superproject(struct strbuf *sb, const char *gitdir) +{ -+ if (!get_superproject_gitdir(sb)) -+ /* not a submodule */ ++ const char *sp_gitdir; ++ if (git_config_get_string_tmp("submodule.superprojectGitDir", &sp_gitdir)) ++ /* probably not a submodule */ + strbuf_addstr(sb, gitdir); ++ else ++ /* definitely a submodule */ ++ strbuf_addstr(sb, sp_gitdir); + + strbuf_addstr(sb, "/config.superproject"); +} @@ config.h: typedef int (*config_parser_event_fn_t)(enum config_event_t type, unsigned int ignore_worktree : 1; unsigned int ignore_cmdline : 1; unsigned int system_gently : 1; -@@ config.h: int git_config_rename_section_in_file(const char *, const char *, const char *); +@@ config.h: int git_config_rename_section(const char *, const char *); + int git_config_rename_section_in_file(const char *, const char *, const char *); int git_config_copy_section(const char *, const char *); int git_config_copy_section_in_file(const char *, const char *, const char *); - const char *git_etc_gitconfig(void); +void git_config_superproject(struct strbuf *, const char *); int git_env_bool(const char *, int); unsigned long git_env_ulong(const char *, unsigned long); int git_config_system(void); - ## submodule.c ## -@@ submodule.c: void absorb_git_dir_into_superproject(const char *path, - } - } - -+int get_superproject_gitdir(struct strbuf *buf) -+{ -+ struct strbuf sb = STRBUF_INIT; -+ struct child_process cp = CHILD_PROCESS_INIT; -+ int rc = 0; -+ -+ /* NEEDSWORK: this call also calls out to a subprocess! */ -+ rc = get_superproject_working_tree(&sb); -+ strbuf_release(&sb); -+ -+ if (!rc) -+ return rc; -+ -+ prepare_submodule_repo_env_no_git_dir(&cp.env_array); -+ -+ strvec_pushl(&cp.args, "-C", "..", "rev-parse", "--absolute-git-dir", NULL); -+ cp.git_cmd = 1; -+ -+ rc = capture_command(&cp, buf, 0); -+ strbuf_trim_trailing_newline(buf); -+ -+ /* leave buf empty if we didn't have a superproject gitdir */ -+ if (rc) -+ strbuf_reset(buf); -+ -+ /* rc reflects the exit code of the rev-parse; invert into a bool */ -+ return !rc; -+} -+ - int get_superproject_working_tree(struct strbuf *buf) - { - struct child_process cp = CHILD_PROCESS_INIT; - - ## submodule.h ## -@@ submodule.h: 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 a submodule, return 1 and fill buf with the absolute -+ * path to the superproject's gitdir. If this repository is not a submodule of -+ * another repository, return 0 and leave buf untouched. -+ */ -+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 of - ## t/t1311-superproject-config.sh (new) ## @@ +#!/bin/sh -- 2.32.0.288.g62a8d224e6-goog ^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH v3 1/2] config: rename "submodule" scope to "gitmodules" 2021-06-19 0:31 ` [PATCH v3 0/2] share a config between submodule and superproject Emily Shaffer @ 2021-06-19 0:31 ` Emily Shaffer 2021-06-19 0:31 ` [PATCH v3 2/2] config: add 'config.superproject' file Emily Shaffer 1 sibling, 0 replies; 25+ messages in thread From: Emily Shaffer @ 2021-06-19 0:31 UTC (permalink / raw) To: git; +Cc: Emily Shaffer To prepare for the addition of a new config scope which only applies to submodule projects, disambiguate "CONFIG_SCOPE_SUBMODULES". This scope refers to configs gathered from the .gitmodules file in the superproject, so just call it "gitmodules." Signed-off-by: Emily Shaffer <emilyshaffer@google.com> --- config.c | 4 ++-- config.h | 2 +- submodule-config.c | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/config.c b/config.c index f9c400ad30..a5fc4cacd5 100644 --- a/config.c +++ b/config.c @@ -3516,8 +3516,8 @@ const char *config_scope_name(enum config_scope scope) return "worktree"; case CONFIG_SCOPE_COMMAND: return "command"; - case CONFIG_SCOPE_SUBMODULE: - return "submodule"; + case CONFIG_SCOPE_GITMODULES: + return "gitmodules"; default: return "unknown"; } diff --git a/config.h b/config.h index 9038538ffd..5faa72f3f5 100644 --- a/config.h +++ b/config.h @@ -42,7 +42,7 @@ enum config_scope { CONFIG_SCOPE_LOCAL, CONFIG_SCOPE_WORKTREE, CONFIG_SCOPE_COMMAND, - CONFIG_SCOPE_SUBMODULE, + CONFIG_SCOPE_GITMODULES, }; const char *config_scope_name(enum config_scope scope); diff --git a/submodule-config.c b/submodule-config.c index 2026120fb3..ed8e671d17 100644 --- a/submodule-config.c +++ b/submodule-config.c @@ -637,7 +637,7 @@ static void config_from_gitmodules(config_fn_t fn, struct repository *repo, void { if (repo->worktree) { struct git_config_source config_source = { - 0, .scope = CONFIG_SCOPE_SUBMODULE + 0, .scope = CONFIG_SCOPE_GITMODULES }; const struct config_options opts = { 0 }; struct object_id oid; -- 2.32.0.288.g62a8d224e6-goog ^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH v3 2/2] config: add 'config.superproject' file 2021-06-19 0:31 ` [PATCH v3 0/2] share a config between submodule and superproject Emily Shaffer 2021-06-19 0:31 ` [PATCH v3 1/2] config: rename "submodule" scope to "gitmodules" Emily Shaffer @ 2021-06-19 0:31 ` Emily Shaffer 1 sibling, 0 replies; 25+ messages in thread From: Emily Shaffer @ 2021-06-19 0:31 UTC (permalink / raw) To: git; +Cc: Emily Shaffer Some configs, such as wrapper directives like gerrit.createChangeId, or forthcoming hook configs, should apply to a superproject as well as all its submodules. It may not be appropriate to apply them globally - for example, if the user also contributes to many projects which do not use the configs necessary for one project-with-submodules - and it may be burdensome to apply them locally to the superproject and each of its submodules. Even if the user runs 'git submodule foreach "git config --local foo.bar', if a new submodule is added later on, that config is not applied to the new submodule. It is also inappropriate to share the entire superproject config, since some items - like remote URLs or partial-clone filters - would not apply to a submodule. To make life easier for projects with many submodules, then, create a new "config.superproject" config scope, which is included in the config parse for the superproject as well as for all the submodules of that superproject. For the superproject, this new config file is equally local to the local config; for the submodule, the new config file is less local than the local config. So let's include it directly before the local config during the config parse. Signed-off-by: Emily Shaffer <emilyshaffer@google.com> --- Notes: Since v1: - Used test_when_finished liberally in tests instead of bare cleanup steps - Fixed some nits regarding xstrdup instead of strbuf_detach. One thing that I thought about but did not implement: rather than finding the path to the superproject's gitdir, you could imagine gathering the config by making a call out to 'git -C ../ config' - but on second thought, it seems like that will make it harder to edit. However, if we don't want to be able to edit superproject config from a submodule, that might be okay... (This approach could make 'git config --show-origin' harder to implement, though, I think.) So I did not make any changes about that. Documentation/git-config.txt | 21 +++++- builtin/config.c | 9 ++- config.c | 28 ++++++++ config.h | 3 + t/t1311-superproject-config.sh | 116 +++++++++++++++++++++++++++++++++ 5 files changed, 174 insertions(+), 3 deletions(-) create mode 100755 t/t1311-superproject-config.sh diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt index 5cddadafd2..312deea21d 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. ++ +For reading options: read only from the superproject's +`.git/config.superproject` file, even if run from a submodule of that +superproject, rather than from all available files. ++ +See also <<FILES>>. + --local:: For writing options: write to the repository `.git/config` file. This is the default behavior. @@ -283,7 +294,7 @@ The default is to use a pager. FILES ----- -If not set explicitly with `--file`, there are four files where +If not set explicitly with `--file`, there are five files where 'git config' will search for configuration options: $(prefix)/etc/gitconfig:: @@ -301,6 +312,12 @@ $XDG_CONFIG_HOME/git/config:: User-specific configuration file. Also called "global" configuration file. +$GIT_DIR/config.superproject:: + When `git config` is run from a project which is a submodule of another + project, that superproject's $GIT_DIR will be used. Use this config file + to set configurations which need to be the same across a superproject + and all its submodules. + $GIT_DIR/config:: Repository specific configuration file. diff --git a/builtin/config.c b/builtin/config.c index 865fddd6ce..83376e0916 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,11 @@ int cmd_config(int argc, const char **argv, const char *prefix) else if (use_system_config) { given_config_source.file = git_system_config(); 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 = strbuf_detach(&superproject_cfg, NULL); + given_config_source.scope = CONFIG_SCOPE_SUPERPROJECT; } 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 a5fc4cacd5..97d63a825c 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,19 @@ void git_global_config(char **user_out, char **xdg_out) *xdg_out = xdg_config; } +void git_config_superproject(struct strbuf *sb, const char *gitdir) +{ + const char *sp_gitdir; + if (git_config_get_string_tmp("submodule.superprojectGitDir", &sp_gitdir)) + /* probably not a submodule */ + strbuf_addstr(sb, gitdir); + else + /* definitely a submodule */ + strbuf_addstr(sb, sp_gitdir); + + strbuf_addstr(sb, "/config.superproject"); +} + /* * Parse environment variable 'k' as a boolean (in various * possible spellings); if missing, use the default value 'def'. @@ -1911,6 +1925,17 @@ static int do_git_config_sequence(const struct config_options *opts, if (user_config && !access_or_die(user_config, R_OK, ACCESS_EACCES_OK)) ret += git_config_from_file(fn, user_config, data); + current_parsing_scope = CONFIG_SCOPE_SUPERPROJECT; + if (opts->git_dir && !opts->ignore_superproject) { + + struct strbuf superproject_gitdir = STRBUF_INIT; + git_config_superproject(&superproject_gitdir, opts->git_dir); + if (!access_or_die(superproject_gitdir.buf, R_OK, 0)) + ret += git_config_from_file(fn, superproject_gitdir.buf, data); + + strbuf_release(&superproject_gitdir); + } + current_parsing_scope = CONFIG_SCOPE_LOCAL; if (!opts->ignore_repo && repo_config && !access_or_die(repo_config, R_OK, 0)) @@ -2030,6 +2055,7 @@ void read_very_early_config(config_fn_t cb, void *data) opts.respect_includes = 1; opts.ignore_repo = 1; + opts.ignore_superproject = 1; opts.ignore_worktree = 1; opts.ignore_cmdline = 1; opts.system_gently = 1; @@ -3518,6 +3544,8 @@ const char *config_scope_name(enum config_scope scope) return "command"; case CONFIG_SCOPE_GITMODULES: return "gitmodules"; + case CONFIG_SCOPE_SUPERPROJECT: + return "superproject"; default: return "unknown"; } diff --git a/config.h b/config.h index 5faa72f3f5..69d7e4cb72 100644 --- a/config.h +++ b/config.h @@ -43,6 +43,7 @@ enum config_scope { CONFIG_SCOPE_WORKTREE, CONFIG_SCOPE_COMMAND, CONFIG_SCOPE_GITMODULES, + CONFIG_SCOPE_SUPERPROJECT, }; const char *config_scope_name(enum config_scope scope); @@ -84,6 +85,7 @@ typedef int (*config_parser_event_fn_t)(enum config_event_t type, struct config_options { unsigned int respect_includes : 1; unsigned int ignore_repo : 1; + unsigned int ignore_superproject : 1; unsigned int ignore_worktree : 1; unsigned int ignore_cmdline : 1; unsigned int system_gently : 1; @@ -318,6 +320,7 @@ int git_config_rename_section(const char *, const char *); int git_config_rename_section_in_file(const char *, const char *, const char *); int git_config_copy_section(const char *, const char *); int git_config_copy_section_in_file(const char *, const char *, const char *); +void git_config_superproject(struct strbuf *, const char *); int git_env_bool(const char *, int); unsigned long git_env_ulong(const char *, unsigned long); int git_config_system(void); diff --git a/t/t1311-superproject-config.sh b/t/t1311-superproject-config.sh new file mode 100755 index 0000000000..74b6ca2cac --- /dev/null +++ b/t/t1311-superproject-config.sh @@ -0,0 +1,116 @@ +#!/bin/sh + +test_description='Test git config --superproject in different settings' + +. ./test-lib.sh + +# follow t7400's example and use the trash dir repo as a submodule to add +submodurl=$(pwd -P) + +# since only the configs are modified, set up the repo structure only once +test_expect_success 'setup repo structure' ' + test_commit "base" && + git submodule add "${submodurl}" sub/ && + git commit -m "add a submodule" +' + +test_expect_success 'superproject config applies to super and submodule' ' + cat >.git/config.superproject <<-EOF && + [foo] + bar = baz + EOF + test_when_finished rm .git/config.superproject && + + git config --get foo.bar && + git -C sub config --get foo.bar + +' + +test_expect_success 'can add from super or sub' ' + git config --superproject apple.species honeycrisp && + git -C sub config --superproject banana.species cavendish && + test_when_finished rm .git/config.superproject && + + cat >expect <<-EOF && + apple.species=honeycrisp + banana.species=cavendish + EOF + + git config --list >actual && + grep -Ff expect actual && + + git -C sub config --list >actual && + grep -Ff expect actual +' + +test_expect_success 'can --unset from super or sub' ' + git config --superproject apple.species honeycrisp && + git -C sub config --superproject banana.species cavendish && + test_when_finished rm .git/config.superproject && + + git config --unset --superproject banana.species && + git -C sub config --unset --superproject apple.species +' + +test_expect_success 'can --edit superproject config' ' + test_config core.editor "echo [foo]bar=baz >" && + git config --edit --superproject && + test_when_finished rm .git/config.superproject && + + git config --get foo.bar +' + +test_expect_success 'can --show-origin the superproject config' ' + git config --superproject --add foo.bar baz && + test_when_finished rm .git/config.superproject && + + git config --list --show-origin >actual && + grep -F "config.superproject" actual +' + +test_expect_success 'can --show-scope the superproject config' ' + git config --superproject --add foo.bar baz && + test_when_finished rm .git/config.superproject && + + git config --list --show-scope >actual && + grep "superproject" actual +' + +test_expect_success 'pre-existing config applies to new submodule' ' + git config --superproject --add foo.bar baz && + test_when_finished rm .git/config.superproject && + + git submodule add "${submodurl}" sub2/ && + test_when_finished rm -fr sub2 && + git commit -m "add a second submodule" && + test_when_finished git reset HEAD^ && + + git -C sub2 config --get foo.bar +' + +# NEEDSWORK: submodule.c:get_superproject_working_tree doesn't support worktree +test_expect_failure 'worktrees can still access config.superproject' ' + git config --superproject --add foo.bar baz && + test_when_finished rm .git/config.superproject && + + git worktree add wt && + test_when_finished git worktree remove wt && + ( + cd wt && + git config --get foo.bar + ) +' + +# This test deletes the submodule! Keep it at the end of the test suite. +test_expect_success 'config.superproject works even with no submodules' ' + # get rid of the submodule + git reset HEAD^ && + rm -fr sub && + + git config --superproject --add foo.bar baz && + test_when_finished rm .git/config.superproject && + + git config --get foo.bar +' + +test_done -- 2.32.0.288.g62a8d224e6-goog ^ permalink raw reply related [flat|nested] 25+ messages in thread
end of thread, other threads:[~2021-06-19 0:31 UTC | newest] Thread overview: 25+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-04-08 23:39 [RFC PATCH 0/2] share a config between submodule and superproject Emily Shaffer 2021-04-08 23:39 ` [RFC PATCH 1/2] config: rename "submodule" scope to "gitmodules" Emily Shaffer 2021-04-08 23:39 ` [RFC PATCH 2/2] config: add 'config.superproject' file Emily Shaffer 2021-04-09 11:10 ` Philip Oakley 2021-04-13 18:05 ` Emily Shaffer 2021-04-09 14:35 ` Matheus Tavares Bernardino 2021-04-09 22:29 ` Junio C Hamano 2021-04-13 19:45 ` Emily Shaffer 2021-04-13 18:48 ` Emily Shaffer 2021-04-14 10:32 ` Future structure of submodules and .git/, .git/modules/* organization Ævar Arnfjörð Bjarmason 2021-04-15 21:25 ` Emily Shaffer 2021-04-15 21:41 ` Junio C Hamano 2021-04-23 0:15 ` [RFC PATCH v2 0/4] share a config between submodule and superproject Emily Shaffer 2021-04-23 0:15 ` [RFC PATCH v2 1/4] config: rename "submodule" scope to "gitmodules" Emily Shaffer 2021-04-23 9:46 ` Phillip Wood 2021-04-23 0:15 ` [RFC PATCH v2 2/4] t1510-repo-setup: don't use exact matching on traces Emily Shaffer 2021-04-23 9:59 ` Phillip Wood 2021-04-23 0:15 ` [RFC PATCH v2 3/4] t7006-pager.sh: more lenient trace checking Emily Shaffer 2021-04-23 9:54 ` Phillip Wood 2021-04-23 12:45 ` Phillip Wood 2021-04-23 0:15 ` [RFC PATCH v2 4/4] config: add 'config.superproject' file Emily Shaffer 2021-04-23 12:08 ` Johannes Schindelin 2021-06-19 0:31 ` [PATCH v3 0/2] share a config between submodule and superproject Emily Shaffer 2021-06-19 0:31 ` [PATCH v3 1/2] config: rename "submodule" scope to "gitmodules" Emily Shaffer 2021-06-19 0:31 ` [PATCH v3 2/2] config: add 'config.superproject' file Emily Shaffer
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).