| Message ID | 448e0efffd0bbc89d8ea891923f242b5123c5826.1590627264.git.matheus.bernardino@usp.br (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | grep: honor sparse checkout and add option to ignore it | expand |
On Wed, May 27, 2020 at 6:13 PM Matheus Tavares <matheus.bernardino@usp.br> wrote: > > One of the steps in do_git_config_sequence() is to load the > worktree-specific config file. Although the function receives a git_dir > string, it relies on git_pathdup(), which uses the_repository->git_dir, > to make the path to the file. Furthermore, it also checks that > extensions.worktreeConfig is set through the > repository_format_worktree_config variable, which refers to > the_repository only. Thus, when a submodule has worktree settings, a > command executed in the superproject that recurses into the submodule > won't find the said settings. > > Such a scenario might not be needed now, but it will be in the following It's not needed? Are there not other config values that affect grep's behavior, such as smudge filters of the submodule that might be important if doing a 'git grep --recurse-submodules $REVISION'? Also, is there a similar issue here for .gitattributes? (e.g. if the submodule declares certain files to be binary?) (I don't actually know if these are issues but I'm just surprised to hear that this would be the first case that would need to look at submodule-specific configuration. If the current code handles these other scenarios I bring up, then you just need to correct your commit message. If these aren't issues, then I'd appreciate a quick explanation of why I'm off base. If these are current issues and the current code isn't handling them, I'm not saying you need to address them in this patch series, but you might need to reword the commit message to mention that was already an issue that has previously been overlooked and we're starting by fixing one case.) > patch. git-grep will learn to honor sparse checkouts and, when running > with --recurse-submodules, the submodule's sparse checkout settings must > be loaded. As these settings are stored in the config.worktree file, > they would be ignored without this patch. So let's fix this by reading > the right config.worktree file and extensions.worktreeConfig setting, > based on the git_dir and commondir paths given to > do_git_config_sequence(). 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 | 119 +++++++++++++++++++++++++++---------- > t/t2404-worktree-config.sh | 16 +++++ > 3 files changed, 118 insertions(+), 38 deletions(-) > > diff --git a/config.c b/config.c > index 8db9c77098..c2d56309dc 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 1c8e965840..284f83a921 100644 > --- a/t/helper/test-config.c > +++ b/t/helper/test-config.c > @@ -2,12 +2,19 @@ > #include "cache.h" > #include "config.h" > #include "string-list.h" > +#include "submodule-config.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 > * > @@ -84,33 +91,63 @@ int cmd__config(int argc, const char **argv) > int i, val; > const char *v; > const struct string_list *strptr; > - struct config_set cs; > + struct config_set cs = { .hash_initialized = 0 }; > enum test_config_exit_code ret = TC_SUCCESS; > + struct repository *repo = the_repository; > + const char *subrepo_path = NULL; > + > + argc--; /* skip over "config" */ This line alone is responsible for a fairly big set of changes throughout this file, just decrementing indices everywhere. It might be nice for review purposes if this and the other changes it caused were pulled out into a separate step, so we can more easily concentrate on the primary additions and changes you are making to this file. In particular, being so unfamiliar with submodules I'd really like to try to find someone who knows them a bit better to review all the subrepo_path related portions of this change to this file plus the config.c changes, but I think that'd be easier if the change were more focused. > + argv++; > + > + if (argc == 0) > + goto print_usage_error; > + > + if (skip_prefix(*argv, "--submodule=", &subrepo_path)) { > + argc--; > + argv++; > + if (argc == 0) > + goto print_usage_error; > + } > > - if (argc == 3 && !strcmp(argv[1], "read_early_config")) { > - read_early_config(early_config_cb, (void *)argv[2]); > + if (argc == 2 && !strcmp(argv[0], "read_early_config")) { > + if (subrepo_path) { > + fprintf(stderr, "Cannot use --submodule with read_early_config\n"); > + return TC_USAGE_ERROR; > + } > + read_early_config(early_config_cb, (void *)argv[1]); > return TC_SUCCESS; > } > > setup_git_directory(); > - > git_configset_init(&cs); > > - if (argc < 2) > - goto print_usage_error; > + if (subrepo_path) { > + const struct submodule *sub; > + struct repository *subrepo = xcalloc(1, sizeof(*repo)); > + > + sub = submodule_from_path(the_repository, &null_oid, subrepo_path); > + if (!sub || repo_submodule_init(subrepo, the_repository, sub)) { > + fprintf(stderr, "Invalid argument to --submodule: '%s'\n", > + subrepo_path); > + free(subrepo); > + ret = TC_USAGE_ERROR; > + goto out; > + } > + repo = subrepo; > + } > > - if (argc == 3 && !strcmp(argv[1], "get_value")) { > - if (!git_config_get_value(argv[2], &v)) { > + if (argc == 2 && !strcmp(argv[0], "get_value")) { > + if (!repo_config_get_value(repo, argv[1], &v)) { > if (!v) > printf("(NULL)\n"); > else > printf("%s\n", v); > } else { > - printf("Value not found for \"%s\"\n", argv[2]); > + printf("Value not found for \"%s\"\n", argv[1]); > ret = TC_VALUE_NOT_FOUND; > } > - } else if (argc == 3 && !strcmp(argv[1], "get_value_multi")) { > - strptr = git_config_get_value_multi(argv[2]); > + } else if (argc == 2 && !strcmp(argv[0], "get_value_multi")) { > + strptr = repo_config_get_value_multi(repo, argv[1]); > if (strptr) { > for (i = 0; i < strptr->nr; i++) { > v = strptr->items[i].string; > @@ -120,32 +157,38 @@ int cmd__config(int argc, const char **argv) > printf("%s\n", v); > } > } else { > - printf("Value not found for \"%s\"\n", argv[2]); > + printf("Value not found for \"%s\"\n", argv[1]); > ret = TC_VALUE_NOT_FOUND; > } > - } else if (argc == 3 && !strcmp(argv[1], "get_int")) { > - if (!git_config_get_int(argv[2], &val)) { > + } else if (argc == 2 && !strcmp(argv[0], "get_int")) { > + if (!repo_config_get_int(repo, argv[1], &val)) { > printf("%d\n", val); > } else { > - printf("Value not found for \"%s\"\n", argv[2]); > + printf("Value not found for \"%s\"\n", argv[1]); > ret = TC_VALUE_NOT_FOUND; > } > - } else if (argc == 3 && !strcmp(argv[1], "get_bool")) { > - if (!git_config_get_bool(argv[2], &val)) { > + } else if (argc == 2 && !strcmp(argv[0], "get_bool")) { > + if (!repo_config_get_bool(repo, argv[1], &val)) { > printf("%d\n", val); > } else { > - printf("Value not found for \"%s\"\n", argv[2]); > + > + printf("Value not found for \"%s\"\n", argv[1]); > ret = TC_VALUE_NOT_FOUND; > } > - } else if (argc == 3 && !strcmp(argv[1], "get_string")) { > - if (!git_config_get_string_const(argv[2], &v)) { > + } else if (argc == 2 && !strcmp(argv[0], "get_string")) { > + if (!repo_config_get_string_const(repo, argv[1], &v)) { > printf("%s\n", v); > } else { > - printf("Value not found for \"%s\"\n", argv[2]); > + printf("Value not found for \"%s\"\n", argv[1]); > ret = TC_VALUE_NOT_FOUND; > } > - } else if (argc >= 3 && !strcmp(argv[1], "configset_get_value")) { > - for (i = 3; i < argc; i++) { > + } else if (argc >= 2 && !strcmp(argv[0], "configset_get_value")) { > + if (subrepo_path) { > + fprintf(stderr, "Cannot use --submodule with configset_get_value\n"); > + ret = TC_USAGE_ERROR; > + goto out; > + } > + for (i = 2; i < argc; i++) { > int err; > if ((err = git_configset_add_file(&cs, argv[i]))) { > fprintf(stderr, "Error (%d) reading configuration file %s.\n", err, argv[i]); > @@ -153,17 +196,22 @@ int cmd__config(int argc, const char **argv) > goto out; > } > } > - if (!git_configset_get_value(&cs, argv[2], &v)) { > + if (!git_configset_get_value(&cs, argv[1], &v)) { > if (!v) > printf("(NULL)\n"); > else > printf("%s\n", v); > } else { > - printf("Value not found for \"%s\"\n", argv[2]); > + printf("Value not found for \"%s\"\n", argv[1]); > ret = TC_VALUE_NOT_FOUND; > } > - } else if (argc >= 3 && !strcmp(argv[1], "configset_get_value_multi")) { > - for (i = 3; i < argc; i++) { > + } else if (argc >= 2 && !strcmp(argv[0], "configset_get_value_multi")) { > + if (subrepo_path) { > + fprintf(stderr, "Cannot use --submodule with configset_get_value_multi\n"); > + ret = TC_USAGE_ERROR; > + goto out; > + } > + for (i = 2; i < argc; i++) { > int err; > if ((err = git_configset_add_file(&cs, argv[i]))) { > fprintf(stderr, "Error (%d) reading configuration file %s.\n", err, argv[i]); > @@ -171,7 +219,7 @@ int cmd__config(int argc, const char **argv) > goto out; > } > } > - strptr = git_configset_get_value_multi(&cs, argv[2]); > + strptr = git_configset_get_value_multi(&cs, argv[1]); > if (strptr) { > for (i = 0; i < strptr->nr; i++) { > v = strptr->items[i].string; > @@ -181,18 +229,23 @@ int cmd__config(int argc, const char **argv) > printf("%s\n", v); > } > } else { > - printf("Value not found for \"%s\"\n", argv[2]); > + printf("Value not found for \"%s\"\n", argv[1]); > ret = TC_VALUE_NOT_FOUND; > } > - } else if (!strcmp(argv[1], "iterate")) { > - git_config(iterate_cb, NULL); > + } else if (!strcmp(argv[0], "iterate")) { > + repo_config(repo, iterate_cb, NULL); > } else { > print_usage_error: > - fprintf(stderr, "Invalid syntax. Usage: test-tool config <cmd> [args]\n"); > + fprintf(stderr, "Invalid syntax. Usage: test-tool config" > + " [--submodule=<path>] <cmd> [args]\n"); > ret = TC_USAGE_ERROR; > } > > out: > git_configset_clear(&cs); > + if (repo != the_repository) { > + repo_clear(repo); > + free(repo); > + } > return ret; > } > diff --git a/t/t2404-worktree-config.sh b/t/t2404-worktree-config.sh > index 286121d8de..b6ab793203 100755 > --- a/t/t2404-worktree-config.sh > +++ b/t/t2404-worktree-config.sh > @@ -76,4 +76,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 > -- > 2.26.2 The index updates seem fine, and I like the test, and I tried to look at the submodule and config bits but I'm quite unfamiliar with that area of the code and I'd like to see if we can find someone who knows submodules and/or config a bit better to review those pieces. A split of this patch into two in your next roll of this series would be nice so we can ask someone to look at just the relevant bits.
On Sat, May 30, 2020 at 11:49 AM Elijah Newren <newren@gmail.com> wrote: > > On Wed, May 27, 2020 at 6:13 PM Matheus Tavares > <matheus.bernardino@usp.br> wrote: > > > > One of the steps in do_git_config_sequence() is to load the > > worktree-specific config file. Although the function receives a git_dir > > string, it relies on git_pathdup(), which uses the_repository->git_dir, > > to make the path to the file. Furthermore, it also checks that > > extensions.worktreeConfig is set through the > > repository_format_worktree_config variable, which refers to > > the_repository only. Thus, when a submodule has worktree settings, a > > command executed in the superproject that recurses into the submodule > > won't find the said settings. > > > > Such a scenario might not be needed now, but it will be in the following > > It's not needed? Are there not other config values that affect grep's > behavior, such as smudge filters of the submodule that might be > important if doing a 'git grep --recurse-submodules $REVISION'? Hmm, I haven't used smudge filters before, but it seems to me that `git grep $REVISION` does not honor them. > Also, is there a similar issue here for .gitattributes? (e.g. if the > submodule declares certain files to be binary?) Declaring files as binary in the submodule works fine. But I noticed that textconv filter specifications in the submodule's config are currently ignored. To be honest, I wasn't aware of this issue before. > I don't actually know if these are issues but I'm just surprised to > hear that this would be the first case that would need to look at > submodule-specific configuration. Hmm, not to submodule-specific configuration but to worktree-specific configuration of a submodule, right? I.e. a config.worktree file from within a submodule. Reconsidering this now, we could indeed have a diff.<driver>.textconv or core.quotePath settings specified in the worktree scope of a submodule. And we should honor them when recursing in grep. I guess I thought the "most natural" place for these settings, in a submodule, would be in the standard .git/config file (as opposed to the sparse-checkout ones, which are normally at config.worktree). That's probably why I wrote "Such scenario might not be needed now". But we should indeed support reading diff.<driver>.textconv from config.worktree as well (although grep currently ignores this setting in submodules, both in the local and worktree scopes). So the said sentence doesn't make much sense, indeed. I will remove it. Thanks! > > diff --git a/t/helper/test-config.c b/t/helper/test-config.c > > index 1c8e965840..284f83a921 100644 > > --- a/t/helper/test-config.c > > +++ b/t/helper/test-config.c > > @@ -84,33 +91,63 @@ int cmd__config(int argc, const char **argv) > > int i, val; > > const char *v; > > const struct string_list *strptr; > > - struct config_set cs; > > + struct config_set cs = { .hash_initialized = 0 }; > > enum test_config_exit_code ret = TC_SUCCESS; > > + struct repository *repo = the_repository; > > + const char *subrepo_path = NULL; > > + > > + argc--; /* skip over "config" */ > > This line alone is responsible for a fairly big set of changes > throughout this file, just decrementing indices everywhere. It might > be nice for review purposes if this and the other changes it caused > were pulled out into a separate step, so we can more easily > concentrate on the primary additions and changes you are making to > this file. OK, will do.
diff --git a/config.c b/config.c index 8db9c77098..c2d56309dc 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 1c8e965840..284f83a921 100644 --- a/t/helper/test-config.c +++ b/t/helper/test-config.c @@ -2,12 +2,19 @@ #include "cache.h" #include "config.h" #include "string-list.h" +#include "submodule-config.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 * @@ -84,33 +91,63 @@ int cmd__config(int argc, const char **argv) int i, val; const char *v; const struct string_list *strptr; - struct config_set cs; + struct config_set cs = { .hash_initialized = 0 }; enum test_config_exit_code ret = TC_SUCCESS; + struct repository *repo = the_repository; + const char *subrepo_path = NULL; + + argc--; /* skip over "config" */ + argv++; + + if (argc == 0) + goto print_usage_error; + + if (skip_prefix(*argv, "--submodule=", &subrepo_path)) { + argc--; + argv++; + if (argc == 0) + goto print_usage_error; + } - if (argc == 3 && !strcmp(argv[1], "read_early_config")) { - read_early_config(early_config_cb, (void *)argv[2]); + if (argc == 2 && !strcmp(argv[0], "read_early_config")) { + if (subrepo_path) { + fprintf(stderr, "Cannot use --submodule with read_early_config\n"); + return TC_USAGE_ERROR; + } + read_early_config(early_config_cb, (void *)argv[1]); return TC_SUCCESS; } setup_git_directory(); - git_configset_init(&cs); - if (argc < 2) - goto print_usage_error; + if (subrepo_path) { + const struct submodule *sub; + struct repository *subrepo = xcalloc(1, sizeof(*repo)); + + sub = submodule_from_path(the_repository, &null_oid, subrepo_path); + if (!sub || repo_submodule_init(subrepo, the_repository, sub)) { + fprintf(stderr, "Invalid argument to --submodule: '%s'\n", + subrepo_path); + free(subrepo); + ret = TC_USAGE_ERROR; + goto out; + } + repo = subrepo; + } - if (argc == 3 && !strcmp(argv[1], "get_value")) { - if (!git_config_get_value(argv[2], &v)) { + if (argc == 2 && !strcmp(argv[0], "get_value")) { + if (!repo_config_get_value(repo, argv[1], &v)) { if (!v) printf("(NULL)\n"); else printf("%s\n", v); } else { - printf("Value not found for \"%s\"\n", argv[2]); + printf("Value not found for \"%s\"\n", argv[1]); ret = TC_VALUE_NOT_FOUND; } - } else if (argc == 3 && !strcmp(argv[1], "get_value_multi")) { - strptr = git_config_get_value_multi(argv[2]); + } else if (argc == 2 && !strcmp(argv[0], "get_value_multi")) { + strptr = repo_config_get_value_multi(repo, argv[1]); if (strptr) { for (i = 0; i < strptr->nr; i++) { v = strptr->items[i].string; @@ -120,32 +157,38 @@ int cmd__config(int argc, const char **argv) printf("%s\n", v); } } else { - printf("Value not found for \"%s\"\n", argv[2]); + printf("Value not found for \"%s\"\n", argv[1]); ret = TC_VALUE_NOT_FOUND; } - } else if (argc == 3 && !strcmp(argv[1], "get_int")) { - if (!git_config_get_int(argv[2], &val)) { + } else if (argc == 2 && !strcmp(argv[0], "get_int")) { + if (!repo_config_get_int(repo, argv[1], &val)) { printf("%d\n", val); } else { - printf("Value not found for \"%s\"\n", argv[2]); + printf("Value not found for \"%s\"\n", argv[1]); ret = TC_VALUE_NOT_FOUND; } - } else if (argc == 3 && !strcmp(argv[1], "get_bool")) { - if (!git_config_get_bool(argv[2], &val)) { + } else if (argc == 2 && !strcmp(argv[0], "get_bool")) { + if (!repo_config_get_bool(repo, argv[1], &val)) { printf("%d\n", val); } else { - printf("Value not found for \"%s\"\n", argv[2]); + + printf("Value not found for \"%s\"\n", argv[1]); ret = TC_VALUE_NOT_FOUND; } - } else if (argc == 3 && !strcmp(argv[1], "get_string")) { - if (!git_config_get_string_const(argv[2], &v)) { + } else if (argc == 2 && !strcmp(argv[0], "get_string")) { + if (!repo_config_get_string_const(repo, argv[1], &v)) { printf("%s\n", v); } else { - printf("Value not found for \"%s\"\n", argv[2]); + printf("Value not found for \"%s\"\n", argv[1]); ret = TC_VALUE_NOT_FOUND; } - } else if (argc >= 3 && !strcmp(argv[1], "configset_get_value")) { - for (i = 3; i < argc; i++) { + } else if (argc >= 2 && !strcmp(argv[0], "configset_get_value")) { + if (subrepo_path) { + fprintf(stderr, "Cannot use --submodule with configset_get_value\n"); + ret = TC_USAGE_ERROR; + goto out; + } + for (i = 2; i < argc; i++) { int err; if ((err = git_configset_add_file(&cs, argv[i]))) { fprintf(stderr, "Error (%d) reading configuration file %s.\n", err, argv[i]); @@ -153,17 +196,22 @@ int cmd__config(int argc, const char **argv) goto out; } } - if (!git_configset_get_value(&cs, argv[2], &v)) { + if (!git_configset_get_value(&cs, argv[1], &v)) { if (!v) printf("(NULL)\n"); else printf("%s\n", v); } else { - printf("Value not found for \"%s\"\n", argv[2]); + printf("Value not found for \"%s\"\n", argv[1]); ret = TC_VALUE_NOT_FOUND; } - } else if (argc >= 3 && !strcmp(argv[1], "configset_get_value_multi")) { - for (i = 3; i < argc; i++) { + } else if (argc >= 2 && !strcmp(argv[0], "configset_get_value_multi")) { + if (subrepo_path) { + fprintf(stderr, "Cannot use --submodule with configset_get_value_multi\n"); + ret = TC_USAGE_ERROR; + goto out; + } + for (i = 2; i < argc; i++) { int err; if ((err = git_configset_add_file(&cs, argv[i]))) { fprintf(stderr, "Error (%d) reading configuration file %s.\n", err, argv[i]); @@ -171,7 +219,7 @@ int cmd__config(int argc, const char **argv) goto out; } } - strptr = git_configset_get_value_multi(&cs, argv[2]); + strptr = git_configset_get_value_multi(&cs, argv[1]); if (strptr) { for (i = 0; i < strptr->nr; i++) { v = strptr->items[i].string; @@ -181,18 +229,23 @@ int cmd__config(int argc, const char **argv) printf("%s\n", v); } } else { - printf("Value not found for \"%s\"\n", argv[2]); + printf("Value not found for \"%s\"\n", argv[1]); ret = TC_VALUE_NOT_FOUND; } - } else if (!strcmp(argv[1], "iterate")) { - git_config(iterate_cb, NULL); + } else if (!strcmp(argv[0], "iterate")) { + repo_config(repo, iterate_cb, NULL); } else { print_usage_error: - fprintf(stderr, "Invalid syntax. Usage: test-tool config <cmd> [args]\n"); + fprintf(stderr, "Invalid syntax. Usage: test-tool config" + " [--submodule=<path>] <cmd> [args]\n"); ret = TC_USAGE_ERROR; } out: git_configset_clear(&cs); + if (repo != the_repository) { + repo_clear(repo); + free(repo); + } return ret; } diff --git a/t/t2404-worktree-config.sh b/t/t2404-worktree-config.sh index 286121d8de..b6ab793203 100755 --- a/t/t2404-worktree-config.sh +++ b/t/t2404-worktree-config.sh @@ -76,4 +76,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
One of the steps in do_git_config_sequence() is to load the worktree-specific config file. Although the function receives a git_dir string, it relies on git_pathdup(), which uses the_repository->git_dir, to make the path to the file. Furthermore, it also checks that extensions.worktreeConfig is set through the repository_format_worktree_config variable, which refers to the_repository only. Thus, when a submodule has worktree settings, a command executed in the superproject that recurses into the submodule won't find the said settings. Such a scenario might not be needed now, but it will be in the following patch. git-grep will learn to honor sparse checkouts and, when running with --recurse-submodules, the submodule's sparse checkout settings must be loaded. As these settings are stored in the config.worktree file, they would be ignored without this patch. So let's fix this by reading the right config.worktree file and extensions.worktreeConfig setting, based on the git_dir and commondir paths given to do_git_config_sequence(). 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 | 119 +++++++++++++++++++++++++++---------- t/t2404-worktree-config.sh | 16 +++++ 3 files changed, 118 insertions(+), 38 deletions(-)