Thread (87 messages) 87 messages, 5 authors, 2021-02-09

Re: [PATCH v5 6/8] config: correctly read worktree configs in submodules

From: Jonathan Nieder <hidden>
Date: 2020-09-02 20:15:29

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.

[...]
quoted hunk ↗ jump to hunk
--- 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 ====

[...]
quoted hunk ↗ jump to hunk
--- a/t/helper/test-config.c
+++ b/t/helper/test-config.c
[...]
quoted hunk ↗ jump to hunk
@@ -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. :)

[...]
quoted hunk ↗ jump to hunk
--- 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
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help