git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Jonathan Nieder <jrnieder@gmail.com>
Date
Sep 2, 2020, 20:15 UTC
Message-ID
<20200902201523.GA3941032@google.com>
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?

Show 6 quoted lines
> 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.
[...]
Show 8 quoted lines
> --- 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.
Show 7 quoted lines
> +		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
Previous: Matheus TavaresNext: Matheus Tavares Bernardino
Message 13 of 30 in “grep: honor sparse checkout and add option to ignore it”
  1. 0/8 grep: honor sparse checkout and add option to ignore itMatheus Tavares, Sep 2, 2020
  2. 1/8 doc: grep: unify info on configuration variablesMatheus Tavares, Sep 2, 2020
  3. 2/8 t1308-config-set: avoid false positives when using test-configMatheus Tavares, Sep 2, 2020
  4. Eric SunshineSep 2, 2020
  5. Matheus Tavares BernardinoSep 2, 2020
  6. Eric SunshineSep 2, 2020
  7. 3/8 t/helper/test-config: be consistent with exit codesMatheus Tavares, Sep 2, 2020
  8. 4/8 t/helper/test-config: check argc before accessing argvMatheus Tavares, Sep 2, 2020
  9. Eric SunshineSep 2, 2020
  10. 5/8 t/helper/test-config: unify exit labelsMatheus Tavares, Sep 2, 2020
  11. Eric SunshineSep 2, 2020
  12. 6/8 config: correctly read worktree configs in submodulesMatheus Tavares, Sep 2, 2020
  13. Jonathan NiederSep 2, 2020
  14. Matheus Tavares BernardinoSep 9, 2020
  15. Jonathan NiederSep 9, 2020
  16. 7/8 grep: honor sparse checkout patternsMatheus Tavares, Sep 2, 2020
  17. 8/8 config: add setting to ignore sparsity patterns in some cmdsMatheus Tavares, Sep 2, 2020
  18. 0/9 grep: honor sparse checkout and add option to ignore itMatheus Tavares, Sep 10, 2020
  19. 2/9 t1308-config-set: avoid false positives when using test-configMatheus Tavares, Sep 10, 2020
  20. 4/9 t/helper/test-config: diagnose missing argumentsMatheus Tavares, Sep 10, 2020
  21. 7/9 config: correctly read worktree configs in submodulesMatheus Tavares, Sep 10, 2020
  22. 5/9 t/helper/test-config: unify exit labelsMatheus Tavares, Sep 10, 2020
  23. 8/9 grep: honor sparse checkout patternsMatheus Tavares, Sep 10, 2020
  24. 9/9 config: add setting to ignore sparsity patterns in some cmdsMatheus Tavares, Sep 10, 2020
  25. 6/9 config: make do_git_config_sequence receive a 'struct repository'Matheus Tavares, Sep 10, 2020
  26. 1/9 doc: grep: unify info on configuration variablesMatheus Tavares, Sep 10, 2020
  27. 3/9 t/helper/test-config: be consistent with exit codesMatheus Tavares, Sep 10, 2020
  28. grep: honor sparse-checkout on working tree searchesMatheus Tavares, Feb 9, 2021
  29. Junio C HamanoFeb 9, 2021
  30. Elijah NewrenFeb 10, 2021

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.