From: Matheus Tavares Bernardino <matheus.bernardino@usp.br>
To: Emily Shaffer <emilyshaffer@google.com>
Cc: git <git@vger.kernel.org>
Subject: Re: [RFC PATCH 2/2] config: add 'config.superproject' file
Date: Fri, 9 Apr 2021 11:35:13 -0300 [thread overview]
Message-ID: <CAHd-oW4VoBbZHc7cLdn0LPM531qNDGOfwPZdKiKoG4BoRFaqdg@mail.gmail.com> (raw)
In-Reply-To: <20210408233936.533342-3-emilyshaffer@google.com>
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/ ?
next prev parent reply other threads:[~2021-04-09 14:35 UTC|newest]
Thread overview: 25+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-04-08 23:39 [RFC PATCH 0/2] share a config between submodule and superproject Emily Shaffer
2021-04-08 23:39 ` [RFC PATCH 1/2] config: rename "submodule" scope to "gitmodules" Emily Shaffer
2021-04-08 23:39 ` [RFC PATCH 2/2] config: add 'config.superproject' file Emily Shaffer
2021-04-09 11:10 ` Philip Oakley
2021-04-13 18:05 ` Emily Shaffer
2021-04-09 14:35 ` Matheus Tavares Bernardino [this message]
2021-04-09 22:29 ` Junio C Hamano
2021-04-13 19:45 ` Emily Shaffer
2021-04-13 18:48 ` Emily Shaffer
2021-04-14 10:32 ` Future structure of submodules and .git/, .git/modules/* organization Ævar Arnfjörð Bjarmason
2021-04-15 21:25 ` Emily Shaffer
2021-04-15 21:41 ` Junio C Hamano
2021-04-23 0:15 ` [RFC PATCH v2 0/4] share a config between submodule and superproject Emily Shaffer
2021-04-23 0:15 ` [RFC PATCH v2 1/4] config: rename "submodule" scope to "gitmodules" Emily Shaffer
2021-04-23 9:46 ` Phillip Wood
2021-04-23 0:15 ` [RFC PATCH v2 2/4] t1510-repo-setup: don't use exact matching on traces Emily Shaffer
2021-04-23 9:59 ` Phillip Wood
2021-04-23 0:15 ` [RFC PATCH v2 3/4] t7006-pager.sh: more lenient trace checking Emily Shaffer
2021-04-23 9:54 ` Phillip Wood
2021-04-23 12:45 ` Phillip Wood
2021-04-23 0:15 ` [RFC PATCH v2 4/4] config: add 'config.superproject' file Emily Shaffer
2021-04-23 12:08 ` Johannes Schindelin
2021-06-19 0:31 ` [PATCH v3 0/2] share a config between submodule and superproject Emily Shaffer
2021-06-19 0:31 ` [PATCH v3 1/2] config: rename "submodule" scope to "gitmodules" Emily Shaffer
2021-06-19 0:31 ` [PATCH v3 2/2] config: add 'config.superproject' file Emily Shaffer
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAHd-oW4VoBbZHc7cLdn0LPM531qNDGOfwPZdKiKoG4BoRFaqdg@mail.gmail.com \
--to=matheus.bernardino@usp.br \
--cc=emilyshaffer@google.com \
--cc=git@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).