[RFC PATCH 0/2] share a config between submodule and superproject

[RFC PATCH 0/2] share a config between submodule and superproject

[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

[RFC PATCH 1/2] config: rename "submodule" scope to "gitmodules"

[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

[RFC PATCH 2/2] config: add 'config.superproject' file

[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

Re: [RFC PATCH 2/2] config: add 'config.superproject' file

[Philip Oakley]

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

Re: [RFC PATCH 2/2] config: add 'config.superproject' file

[Matheus Tavares Bernardino]

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/ ?

Re: [RFC PATCH 2/2] config: add 'config.superproject' file

[Junio C Hamano]

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.

Re: [RFC PATCH 2/2] config: add 'config.superproject' file

[Emily Shaffer]

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

Re: [RFC PATCH 2/2] config: add 'config.superproject' file

[Emily Shaffer]

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

Re: [RFC PATCH 2/2] config: add 'config.superproject' file

[Emily Shaffer]

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

Future structure of submodules and .git/, .git/modules/* organization

[Ævar Arnfjörð Bjarmason]

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.

Re: Future structure of submodules and .git/, .git/modules/* organization

[Emily Shaffer]

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

Re: Future structure of submodules and .git/, .git/modules/* organization

[Junio C Hamano]

Æ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.

[RFC PATCH v2 0/4] share a config between submodule and superproject

[Emily Shaffer]

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

[RFC PATCH v2 1/4] config: rename "submodule" scope to "gitmodules"

[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

[RFC PATCH v2 2/4] t1510-repo-setup: don't use exact matching on traces

[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

[RFC PATCH v2 3/4] t7006-pager.sh: more lenient trace checking

[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

[RFC PATCH v2 4/4] config: add 'config.superproject' file

[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

Re: [RFC PATCH v2 1/4] config: rename "submodule" scope to "gitmodules"

[Phillip Wood]

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;
> 

Re: [RFC PATCH v2 3/4] t7006-pager.sh: more lenient trace checking

[Phillip Wood]

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
> 

Re: [RFC PATCH v2 2/4] t1510-repo-setup: don't use exact matching on traces

[Phillip Wood]

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

>   	)
>   }
>   
> 

Re: [RFC PATCH v2 4/4] config: add 'config.superproject' file

[Johannes Schindelin]

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

Re: [RFC PATCH v2 3/4] t7006-pager.sh: more lenient trace checking

[Phillip Wood]

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
>>
> 

[PATCH v3 0/2] share a config between submodule and superproject

[Emily Shaffer]

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

[PATCH v3 1/2] config: rename "submodule" scope to "gitmodules"

[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

[PATCH v3 2/2] config: add 'config.superproject' file

[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