| Message ID | 3e02e1bd248438e0b435a19d857432edcaa15a2c.1599026986.git.matheus.bernardino@usp.br (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | grep: honor sparse checkout and add option to ignore it | expand |
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
Hi, Jonathan Sorry for the late reply, last week was quite busy. On Wed, Sep 2, 2020 at 5:15 PM Jonathan Nieder <jrnieder@gmail.com> wrote: > > 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. Yeah, much better, thanks! :) > [...] > > --- 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. Nice, thanks. > > + 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; I'm not very familiar with the code in setup.c so I apologize for the noob question: have_git_dir() returns `startup_info->have_repository || the_repository->gitdir`; so is it possible that it returns true but the_repository->gitdir is not initialized yet? If so, should we also check the_repository->gitdir here (before assigning opts.repo), and call BUG() when it is NULL, like get_git_dir() does? Hmm, nevertheless, I see that you already check `opts.repo && opts.repo->gitdir` before trying to use it in do_git_config_sequence(). So it should already cover this case, right? > /* > * 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 ==== Thanks a lot for this :) I was thinking of adding it as a preparatory patch before the fix itself. May I have your S-o-B as the author? Best, Matheus
Hi, Matheus Tavares Bernardino wrote: > Sorry for the late reply, last week was quite busy. No problem. It's an unusual time for everyone. [...] > On Wed, Sep 2, 2020 at 5:15 PM Jonathan Nieder <jrnieder@gmail.com> wrote: >> @@ -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; > > I'm not very familiar with the code in setup.c so I apologize for the > noob question: have_git_dir() returns `startup_info->have_repository > || the_repository->gitdir`; so is it possible that it returns true but > the_repository->gitdir is not initialized yet? If so, should we also > check the_repository->gitdir here (before assigning opts.repo), and > call BUG() when it is NULL, like get_git_dir() does? > > Hmm, nevertheless, I see that you already check `opts.repo && > opts.repo->gitdir` before trying to use it in > do_git_config_sequence(). So it should already cover this case, right? Right --- the main point is that a BUG() call represents "this can't happen", or in other words, it's an assertion failure. As a matter of defensive coding functions like get_git_dir() guard against such cases to make debugging a little easier and exploitation a little more difficult when the impossible happens. [...] > Thanks a lot for this :) I was thinking of adding it as a preparatory > patch before the fix itself. May I have your S-o-B as the author? Sure! Signed-off-by: Jonathan Nieder <jrnieder@gmail.com> Thanks, Jonathan
diff --git a/config.c b/config.c index 2bdff4457b..e1e7fab6dc 100644 --- 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) { - char *path = git_pathdup("config.worktree"); - if (!access_or_die(path, R_OK, 0)) - ret += git_config_from_file(fn, path, data); - free(path); + if (!opts->ignore_worktree && repo_config && opts->git_dir) { + 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) { + char *path = mkpathdup("%s/config.worktree", opts->git_dir); + if (!access_or_die(path, R_OK, 0)) + ret += git_config_from_file(fn, path, data); + free(path); + } + + strbuf_release(&buf); + clear_repository_format(&repo_fmt); } current_parsing_scope = CONFIG_SCOPE_COMMAND; diff --git a/t/helper/test-config.c b/t/helper/test-config.c index 8fe43e9775..2924c09c21 100644 --- a/t/helper/test-config.c +++ b/t/helper/test-config.c @@ -2,12 +2,20 @@ #include "cache.h" #include "config.h" #include "string-list.h" +#include "submodule-config.h" +#include "parse-options.h" /* * This program exposes the C API of the configuration mechanism * as a set of simple commands in order to facilitate testing. * - * Reads stdin and prints result of command to stdout: + * Usage: test-tool config [--submodule=<path>] <cmd> [<args>] + * + * If --submodule=<path> is given, <cmd> will operate on the submodule at the + * given <path>. This option is not valid for the commands: read_early_config, + * configset_get_value and configset_get_value_multi. + * + * Possible cmds are: * * get_value -> prints the value with highest priority for the entered key * @@ -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() + }; + + 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"); if (argc == 3 && !strcmp(argv[1], "read_early_config")) { + if (subrepo_path) + die("cannot use --submodule with read_early_config"); read_early_config(early_config_cb, (void *)argv[2]); return ret; } @@ -88,11 +116,18 @@ int cmd__config(int argc, const char **argv) git_configset_init(&cs); - if (argc < 2) - die("Please, provide a command name on the command-line"); + if (subrepo_path) { + const struct submodule *sub; + + sub = submodule_from_path(the_repository, &null_oid, subrepo_path); + if (!sub || repo_submodule_init(&subrepo, the_repository, sub)) + die("invalid argument to --submodule: '%s'", subrepo_path); + + repo = &subrepo; + } if (argc == 3 && !strcmp(argv[1], "get_value")) { - if (!git_config_get_value(argv[2], &v)) { + if (!repo_config_get_value(repo, argv[2], &v)) { if (!v) printf("(NULL)\n"); else @@ -102,7 +137,7 @@ int cmd__config(int argc, const char **argv) ret = TC_VALUE_NOT_FOUND; } } else if (argc == 3 && !strcmp(argv[1], "get_value_multi")) { - strptr = git_config_get_value_multi(argv[2]); + strptr = repo_config_get_value_multi(repo, argv[2]); if (strptr) { for (i = 0; i < strptr->nr; i++) { v = strptr->items[i].string; @@ -116,27 +151,31 @@ int cmd__config(int argc, const char **argv) ret = TC_VALUE_NOT_FOUND; } } else if (argc == 3 && !strcmp(argv[1], "get_int")) { - if (!git_config_get_int(argv[2], &val)) { + if (!repo_config_get_int(repo, argv[2], &val)) { printf("%d\n", val); } else { printf("Value not found for \"%s\"\n", argv[2]); ret = TC_VALUE_NOT_FOUND; } } else if (argc == 3 && !strcmp(argv[1], "get_bool")) { - if (!git_config_get_bool(argv[2], &val)) { + if (!repo_config_get_bool(repo, argv[2], &val)) { printf("%d\n", val); } else { + printf("Value not found for \"%s\"\n", argv[2]); ret = TC_VALUE_NOT_FOUND; } } else if (argc == 3 && !strcmp(argv[1], "get_string")) { - if (!git_config_get_string_tmp(argv[2], &v)) { + if (!repo_config_get_string_tmp(repo, argv[2], &v)) { printf("%s\n", v); } else { printf("Value not found for \"%s\"\n", argv[2]); ret = TC_VALUE_NOT_FOUND; } } else if (argc >= 3 && !strcmp(argv[1], "configset_get_value")) { + if (subrepo_path) + die("cannot use --submodule with configset_get_value"); + for (i = 3; i < argc; i++) { int err; if ((err = git_configset_add_file(&cs, argv[i]))) { @@ -155,6 +194,9 @@ int cmd__config(int argc, const char **argv) ret = TC_VALUE_NOT_FOUND; } } else if (argc >= 3 && !strcmp(argv[1], "configset_get_value_multi")) { + if (subrepo_path) + die("cannot use --submodule with configset_get_value_multi"); + for (i = 3; i < argc; i++) { int err; if ((err = git_configset_add_file(&cs, argv[i]))) { @@ -177,12 +219,14 @@ int cmd__config(int argc, const char **argv) ret = TC_VALUE_NOT_FOUND; } } else if (!strcmp(argv[1], "iterate")) { - git_config(iterate_cb, NULL); + repo_config(repo, iterate_cb, NULL); } else { die("%s: Please check the syntax and the function name", argv[0]); } out: git_configset_clear(&cs); + if (repo != the_repository) + repo_clear(repo); return ret; } diff --git a/t/t2404-worktree-config.sh b/t/t2404-worktree-config.sh index 9536d10919..1e32c93735 100755 --- 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 +' + test_done
The config machinery is not able to read worktree configs from a submodule in a process where the_repository represents the superproject. 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 the path to the worktree config file. And when checking if extensions.worktreeConfig is set, it uses the global repository_format_worktree_config variable, which refers to the_repository only. So let's fix this by using the git_dir given to the function and reading the extension value from the right place. Also add a test to avoid any regressions. Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br> --- config.c | 21 ++++++++++--- t/helper/test-config.c | 62 ++++++++++++++++++++++++++++++++------ t/t2404-worktree-config.sh | 16 ++++++++++ 3 files changed, 85 insertions(+), 14 deletions(-)