From: Jonathan Nieder <jrnieder@gmail.com>
To: Matheus Tavares <matheus.bernardino@usp.br>
Cc: git@vger.kernel.org, gitster@pobox.com, stolee@gmail.com,
newren@gmail.com, jonathantanmy@google.com
Subject: Re: [PATCH v5 6/8] config: correctly read worktree configs in submodules
Date: Wed, 2 Sep 2020 13:15:23 -0700 [thread overview]
Message-ID: <20200902201523.GA3941032@google.com> (raw)
In-Reply-To: <3e02e1bd248438e0b435a19d857432edcaa15a2c.1599026986.git.matheus.bernardino@usp.br>
Matheus Tavares wrote:
> The config machinery is not able to read worktree configs from a
> submodule in a process where the_repository represents the superproject.
... where the_repository represents the superproject and
extensions.worktreeConfig is not set there, right?
> Furthermore, when extensions.worktreeConfig is set on the superproject,
> querying for a worktree config in a submodule will, instead, return
> the value set at the superproject.
>
> The problem resides in do_git_config_sequence(). Although the function
> receives a git_dir string, it uses the_repository->git_dir when making
This part of the commit message seems to be rephrasing what the patch
says; for that kind of thing, it seems better to let the patch speak
for itself. Can we describe what is happening at a higher level (in
other words the intent instead of the details of how that is
manifested in code)? For example,
The relevant code is in do_git_config_sequence. Although it is designed
to act on an arbitrary repository, specified by the passed-in git_dir
string, it accidentally depends on the_repository in two places:
- it reads the global variable `repository_format_worktree_config`,
which is set based on the content of the_repository, to determine
whether extensions.worktreeConfig is set
- it uses the git_pathdup helper to find the config.worktree file,
instead of making a path using the passed-in git_dir falue
Sever these dependencies.
[...]
> --- a/config.c
> +++ b/config.c
> @@ -1747,11 +1747,22 @@ static int do_git_config_sequence(const struct config_options *opts,
> ret += git_config_from_file(fn, repo_config, data);
>
> current_parsing_scope = CONFIG_SCOPE_WORKTREE;
> - if (!opts->ignore_worktree && repository_format_worktree_config) {
> + if (!opts->ignore_worktree && repo_config && opts->git_dir) {
repo_config is non-NULL if and only if commondir is non-NULL and
commondir is always non-NUlL if git_dir is non-NULL (as checked higher
in the function), right? I think that means this condition could be
written more simply as
if (!opts->ignore_worktree && opts->git_dir) {
which I think should be easier for the reader to understand.
> + struct repository_format repo_fmt = REPOSITORY_FORMAT_INIT;
> + struct strbuf buf = STRBUF_INIT;
> +
> + read_repository_format(&repo_fmt, repo_config);
> +
> + if (!verify_repository_format(&repo_fmt, &buf) &&
> + repo_fmt.worktree_config) {
In the common case where we are acting on the_repository, this add
extra complexity and slows the routine down.
Would passing in the 'struct repository *' to allow distinguishing
that case help? Something like this:
diff --git i/builtin/config.c w/builtin/config.c
index 5e39f618854..ca4caedf33a 100644
--- i/builtin/config.c
+++ w/builtin/config.c
@@ -699,10 +699,8 @@ int cmd_config(int argc, const char **argv, const char *prefix)
config_options.respect_includes = !given_config_source.file;
else
config_options.respect_includes = respect_includes_opt;
- if (!nongit) {
- config_options.commondir = get_git_common_dir();
- config_options.git_dir = get_git_dir();
- }
+ if (!nongit)
+ config_options.repo = the_repository;
if (end_nul) {
term = '\0';
diff --git i/config.c w/config.c
index 2bdff4457be..70a1dd0ad3f 100644
--- i/config.c
+++ w/config.c
@@ -222,8 +222,8 @@ static int include_by_gitdir(const struct config_options *opts,
const char *git_dir;
int already_tried_absolute = 0;
- if (opts->git_dir)
- git_dir = opts->git_dir;
+ if (opts->repo && opts->repo->gitdir)
+ git_dir = opts->repo->gitdir;
else
goto done;
@@ -1720,10 +1720,10 @@ static int do_git_config_sequence(const struct config_options *opts,
char *repo_config;
enum config_scope prev_parsing_scope = current_parsing_scope;
- if (opts->commondir)
- repo_config = mkpathdup("%s/config", opts->commondir);
- else if (opts->git_dir)
- BUG("git_dir without commondir");
+ if (opts->repo && opts->repo->commondir)
+ repo_config = mkpathdup("%s/config", opts->repo->commondir);
+ else if (opts->repo && opts->repo->gitdir)
+ BUG("gitdir without commondir");
else
repo_config = NULL;
@@ -1824,27 +1824,33 @@ void read_early_config(config_fn_t cb, void *data)
struct config_options opts = {0};
struct strbuf commondir = STRBUF_INIT;
struct strbuf gitdir = STRBUF_INIT;
+ struct repository the_early_repo = {0};
opts.respect_includes = 1;
if (have_git_dir()) {
- opts.commondir = get_git_common_dir();
- opts.git_dir = get_git_dir();
+ opts.repo = the_repository;
/*
* When setup_git_directory() was not yet asked to discover the
* GIT_DIR, we ask discover_git_directory() to figure out whether there
* is any repository config we should use (but unlike
- * setup_git_directory_gently(), no global state is changed, most
+ * setup_git_directory_gently(), no global state is changed; most
* notably, the current working directory is still the same after the
* call).
+ *
+ * NEEDSWORK: There is some duplicate work between
+ * discover_git_directory and repo_init. Update to use a variant of
+ * repo_init that does its own repository discovery once available.
*/
} else if (!discover_git_directory(&commondir, &gitdir)) {
- opts.commondir = commondir.buf;
- opts.git_dir = gitdir.buf;
+ repo_init(&the_early_repo, gitdir.buf, NULL);
+ opts.repo = &the_early_repo;
}
config_with_options(cb, data, NULL, &opts);
+ if (the_early_repo.settings.initialized)
+ repo_clear(&the_early_repo);
strbuf_release(&commondir);
strbuf_release(&gitdir);
}
@@ -2097,8 +2103,7 @@ static void repo_read_config(struct repository *repo)
struct config_options opts = { 0 };
opts.respect_includes = 1;
- opts.commondir = repo->commondir;
- opts.git_dir = repo->gitdir;
+ opts.repo = repo;
if (!repo->config)
repo->config = xcalloc(1, sizeof(struct config_set));
diff --git i/config.h w/config.h
index 91cdfbfb414..e56293fb29f 100644
--- i/config.h
+++ w/config.h
@@ -21,6 +21,7 @@
*/
struct object_id;
+struct repository;
/* git_config_parse_key() returns these negated: */
#define CONFIG_INVALID_KEY 1
@@ -87,8 +88,7 @@ struct config_options {
unsigned int ignore_worktree : 1;
unsigned int ignore_cmdline : 1;
unsigned int system_gently : 1;
- const char *commondir;
- const char *git_dir;
+ struct repository *repo;
config_parser_event_fn_t event_fn;
void *event_fn_data;
enum config_error_action {
==== >8 ====
[...]
> --- a/t/helper/test-config.c
> +++ b/t/helper/test-config.c
[...]
> @@ -72,14 +80,34 @@ static int early_config_cb(const char *var, const char *value, void *vdata)
> #define TC_VALUE_NOT_FOUND 1
> #define TC_CONFIG_FILE_ERROR 2
>
> +static const char *test_config_usage[] = {
> + "test-tool config [--submodule=<path>] <cmd> [<args>]",
> + NULL
> +};
> +
> int cmd__config(int argc, const char **argv)
> {
> int i, val, ret = 0;
> const char *v;
> const struct string_list *strptr;
> struct config_set cs;
> + struct repository subrepo, *repo = the_repository;
> + const char *subrepo_path = NULL;
> +
> + struct option options[] = {
> + OPT_STRING(0, "submodule", &subrepo_path, "path",
> + "run <cmd> on the submodule at <path>"),
> + OPT_END()
> + };
Nice.
> +
> + argc = parse_options(argc, argv, NULL, options, test_config_usage,
> + PARSE_OPT_KEEP_ARGV0 | PARSE_OPT_STOP_AT_NON_OPTION);
> + if (argc < 2)
> + die("Please, provide a command name on the command-line");
optional nit: can use usage_with_options here. It produces a better
error message than any other I can think of (all I can think of are
things like "need a <cmd>").
This is from existing code, but the use of parse_options opens up the
possibility of taking advantage of the parse-options generated message. :)
[...]
> --- a/t/t2404-worktree-config.sh
> +++ b/t/t2404-worktree-config.sh
> @@ -78,4 +78,20 @@ test_expect_success 'config.worktree no longer read without extension' '
> test_cmp_config -c wt2 shared this.is
> '
>
> +test_expect_success 'correctly read config.worktree from submodules' '
> + test_unconfig extensions.worktreeconfig &&
> + git init sub &&
> + (
> + cd sub &&
> + test_commit a &&
> + git config extensions.worktreeconfig true &&
> + git config --worktree wtconfig.sub test-value
> + ) &&
> + git submodule add ./sub &&
> + git commit -m "add sub" &&
> + echo test-value >expect &&
> + test-tool config --submodule=sub get_value wtconfig.sub >actual &&
> + test_cmp expect actual
> +'
Lovely.
Summary: I like the direction this change goes in.
I think we can do it without repeating repository format discovery in
the the_repository case and without duplicating repository format
discovery code in the submodule case. If it proves too fussy, then a
NEEDSWORK comment would be helpful to help the reader see what is
going on.
Thanks and hope that helps,
Jonathan
next prev parent reply other threads:[~2020-09-02 20:15 UTC|newest]
Thread overview: 123+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-03-24 6:04 [RFC PATCH 0/3] grep: honor sparse checkout and add option to ignore it Matheus Tavares
2020-03-24 6:11 ` [RFC PATCH 1/3] doc: grep: unify info on configuration variables Matheus Tavares
2020-03-24 7:57 ` Elijah Newren
2020-03-24 21:26 ` Junio C Hamano
2020-03-24 23:38 ` Matheus Tavares
2020-03-24 6:12 ` [RFC PATCH 2/3] grep: honor sparse checkout patterns Matheus Tavares
2020-03-24 7:15 ` Elijah Newren
2020-03-24 15:12 ` Derrick Stolee
2020-03-24 16:16 ` Elijah Newren
2020-03-24 17:02 ` Derrick Stolee
2020-03-24 23:01 ` Matheus Tavares Bernardino
2020-03-24 22:55 ` Matheus Tavares Bernardino
2020-04-21 2:10 ` Matheus Tavares Bernardino
2020-04-21 3:08 ` Elijah Newren
2020-04-22 12:08 ` Derrick Stolee
2020-04-23 6:09 ` Matheus Tavares Bernardino
2020-03-24 6:13 ` [RFC PATCH 3/3] grep: add option to ignore sparsity patterns Matheus Tavares
2020-03-24 7:54 ` Elijah Newren
2020-03-24 18:30 ` Junio C Hamano
2020-03-24 19:07 ` Elijah Newren
2020-03-25 20:18 ` Junio C Hamano
2020-03-30 3:23 ` Matheus Tavares Bernardino
2020-03-31 19:12 ` Elijah Newren
2020-03-31 20:02 ` Derrick Stolee
2020-04-27 17:15 ` Matheus Tavares Bernardino
2020-04-29 16:46 ` Elijah Newren
2020-04-29 17:21 ` Elijah Newren
2020-03-25 23:15 ` Matheus Tavares Bernardino
2020-03-26 6:02 ` Elijah Newren
2020-03-27 15:51 ` Junio C Hamano
2020-03-27 19:01 ` Elijah Newren
2020-03-30 1:12 ` Matheus Tavares Bernardino
2020-03-31 16:48 ` Elijah Newren
2020-05-10 0:41 ` [RFC PATCH v2 0/4] grep: honor sparse checkout and add option to ignore it Matheus Tavares
2020-05-10 0:41 ` [RFC PATCH v2 1/4] doc: grep: unify info on configuration variables Matheus Tavares
2020-05-10 0:41 ` [RFC PATCH v2 2/4] config: load the correct config.worktree file Matheus Tavares
2020-05-11 19:10 ` Junio C Hamano
2020-05-12 22:55 ` Matheus Tavares Bernardino
2020-05-12 23:22 ` Junio C Hamano
2020-05-10 0:41 ` [RFC PATCH v2 3/4] grep: honor sparse checkout patterns Matheus Tavares
2020-05-11 19:35 ` Junio C Hamano
2020-05-13 0:05 ` Matheus Tavares Bernardino
2020-05-13 0:17 ` Junio C Hamano
2020-05-21 7:26 ` Elijah Newren
2020-05-21 17:35 ` Matheus Tavares Bernardino
2020-05-21 17:52 ` Elijah Newren
2020-05-22 5:49 ` Matheus Tavares Bernardino
2020-05-22 14:26 ` Elijah Newren
2020-05-22 15:36 ` Elijah Newren
2020-05-22 20:54 ` Matheus Tavares Bernardino
2020-05-22 21:06 ` Elijah Newren
2020-06-10 11:40 ` Derrick Stolee
2020-06-10 16:22 ` Matheus Tavares Bernardino
2020-06-10 17:42 ` Derrick Stolee
2020-06-10 18:14 ` Matheus Tavares Bernardino
2020-06-10 20:12 ` Elijah Newren
2020-06-10 19:58 ` Elijah Newren
2020-05-21 7:36 ` Elijah Newren
2020-05-10 0:41 ` [RFC PATCH v2 4/4] config: add setting to ignore sparsity patterns in some cmds Matheus Tavares
2020-05-10 4:23 ` Matheus Tavares Bernardino
2020-05-21 17:18 ` Elijah Newren
2020-05-21 7:09 ` Elijah Newren
2020-05-28 1:12 ` [PATCH v3 0/5] grep: honor sparse checkout and add option to ignore it Matheus Tavares
2020-05-28 1:12 ` [PATCH v3 1/5] doc: grep: unify info on configuration variables Matheus Tavares
2020-05-28 1:13 ` [PATCH v3 2/5] t/helper/test-config: return exit codes consistently Matheus Tavares
2020-05-30 14:29 ` Elijah Newren
2020-06-01 4:36 ` Matheus Tavares Bernardino
2020-05-28 1:13 ` [PATCH v3 3/5] config: correctly read worktree configs in submodules Matheus Tavares
2020-05-30 14:49 ` Elijah Newren
2020-06-01 4:38 ` Matheus Tavares Bernardino
2020-05-28 1:13 ` [PATCH v3 4/5] grep: honor sparse checkout patterns Matheus Tavares
2020-05-30 15:48 ` Elijah Newren
2020-06-01 4:44 ` Matheus Tavares Bernardino
2020-06-03 2:38 ` Elijah Newren
2020-06-10 17:08 ` Matheus Tavares Bernardino
2020-05-28 1:13 ` [PATCH v3 5/5] config: add setting to ignore sparsity patterns in some cmds Matheus Tavares
2020-05-30 16:18 ` Elijah Newren
2020-06-01 4:45 ` Matheus Tavares Bernardino
2020-06-03 2:39 ` Elijah Newren
2020-06-10 21:15 ` Matheus Tavares Bernardino
2020-06-11 0:35 ` Elijah Newren
2020-06-12 15:44 ` [PATCH v4 0/6] grep: honor sparse checkout and add option to ignore it Matheus Tavares
2020-06-12 15:44 ` [PATCH v4 1/6] doc: grep: unify info on configuration variables Matheus Tavares
2020-06-12 15:45 ` [PATCH v4 2/6] t/helper/test-config: return exit codes consistently Matheus Tavares
2020-06-12 15:45 ` [PATCH v4 3/6] t/helper/test-config: facilitate addition of new cli options Matheus Tavares
2020-06-12 15:45 ` [PATCH v4 4/6] config: correctly read worktree configs in submodules Matheus Tavares
2020-06-16 19:13 ` Elijah Newren
2020-06-21 16:05 ` Matheus Tavares Bernardino
2020-09-01 2:41 ` Jonathan Nieder
2020-09-01 21:44 ` Matheus Tavares Bernardino
2020-06-12 15:45 ` [PATCH v4 5/6] grep: honor sparse checkout patterns Matheus Tavares
2020-06-12 15:45 ` [PATCH v4 6/6] config: add setting to ignore sparsity patterns in some cmds Matheus Tavares
2020-06-16 22:31 ` [PATCH v4 0/6] grep: honor sparse checkout and add option to ignore it Elijah Newren
2020-09-02 6:17 ` [PATCH v5 0/8] " Matheus Tavares
2020-09-02 6:17 ` [PATCH v5 1/8] doc: grep: unify info on configuration variables Matheus Tavares
2020-09-02 6:17 ` [PATCH v5 2/8] t1308-config-set: avoid false positives when using test-config Matheus Tavares
2020-09-02 6:57 ` Eric Sunshine
2020-09-02 16:16 ` Matheus Tavares Bernardino
2020-09-02 16:38 ` Eric Sunshine
2020-09-02 6:17 ` [PATCH v5 3/8] t/helper/test-config: be consistent with exit codes Matheus Tavares
2020-09-02 6:17 ` [PATCH v5 4/8] t/helper/test-config: check argc before accessing argv Matheus Tavares
2020-09-02 7:18 ` Eric Sunshine
2020-09-02 6:17 ` [PATCH v5 5/8] t/helper/test-config: unify exit labels Matheus Tavares
2020-09-02 7:30 ` Eric Sunshine
2020-09-02 6:17 ` [PATCH v5 6/8] config: correctly read worktree configs in submodules Matheus Tavares
2020-09-02 20:15 ` Jonathan Nieder [this message]
2020-09-09 13:04 ` Matheus Tavares Bernardino
2020-09-09 23:32 ` Jonathan Nieder
2020-09-02 6:17 ` [PATCH v5 7/8] grep: honor sparse checkout patterns Matheus Tavares
2020-09-02 6:17 ` [PATCH v5 8/8] config: add setting to ignore sparsity patterns in some cmds Matheus Tavares
2020-09-10 17:21 ` [PATCH v6 0/9] grep: honor sparse checkout and add option to ignore it Matheus Tavares
2020-09-10 17:21 ` [PATCH v6 1/9] doc: grep: unify info on configuration variables Matheus Tavares
2020-09-10 17:21 ` [PATCH v6 2/9] t1308-config-set: avoid false positives when using test-config Matheus Tavares
2020-09-10 17:21 ` [PATCH v6 3/9] t/helper/test-config: be consistent with exit codes Matheus Tavares
2020-09-10 17:21 ` [PATCH v6 4/9] t/helper/test-config: diagnose missing arguments Matheus Tavares
2020-09-10 17:21 ` [PATCH v6 5/9] t/helper/test-config: unify exit labels Matheus Tavares
2020-09-10 17:21 ` [PATCH v6 6/9] config: make do_git_config_sequence receive a 'struct repository' Matheus Tavares
2020-09-10 17:21 ` [PATCH v6 7/9] config: correctly read worktree configs in submodules Matheus Tavares
2020-09-10 17:21 ` [PATCH v6 8/9] grep: honor sparse checkout patterns Matheus Tavares
2020-09-10 17:21 ` [PATCH v6 9/9] config: add setting to ignore sparsity patterns in some cmds Matheus Tavares
2021-02-09 21:33 ` [PATCH v7] grep: honor sparse-checkout on working tree searches Matheus Tavares
2021-02-09 23:23 ` Junio C Hamano
2021-02-10 6:12 ` Elijah Newren
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=20200902201523.GA3941032@google.com \
--to=jrnieder@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=jonathantanmy@google.com \
--cc=matheus.bernardino@usp.br \
--cc=newren@gmail.com \
--cc=stolee@gmail.com \
/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).