diff mbox series

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

Message ID 20210408233936.533342-3-emilyshaffer@google.com (mailing list archive)
State Superseded
Headers show
Series share a config between submodule and superproject | expand

Commit Message

Emily Shaffer April 8, 2021, 11:39 p.m. UTC
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

Comments

Philip Oakley April 9, 2021, 11:10 a.m. UTC | #1
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
Matheus Tavares April 9, 2021, 2:35 p.m. UTC | #2
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/ ?
Junio C Hamano April 9, 2021, 10:29 p.m. UTC | #3
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.
Emily Shaffer April 13, 2021, 6:05 p.m. UTC | #4
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
Emily Shaffer April 13, 2021, 6:48 p.m. UTC | #5
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
Emily Shaffer April 13, 2021, 7:45 p.m. UTC | #6
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
diff mbox series

Patch

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