| Message ID | 6402c968077900d48d189551a652e10984437a9f.1591974940.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 |
Hi, On Fri, Jun 12, 2020 at 8:45 AM 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-specific > settings, a command executed in the superproject that recurses into the > submodule won't find the said settings. > > This will be especially important in the next 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. Thanks for splitting this part of the change out from the previous patch. > > Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br> > --- > config.c | 21 +++++++++--- > t/helper/test-config.c | 67 +++++++++++++++++++++++++++++++++----- > t/t2404-worktree-config.sh | 16 +++++++++ > 3 files changed, 91 insertions(+), 13 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) { What happens when opts->git_dir is NULL? (Does that ever even happen?) Should it fall back to the old code path in that case? > + 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); I've tried to poke around a little at this block, but as with the previous series, I still feel like it should be reviewed by someone who knows submodules and/or config handling better. That's easier now that the patch has been split up. Unfortunately, trying to figure out who the submodule expert(s) are (looking at authors of submodule*.[ch]) seems to lead me to a who's who of people who are no longer active in the project. :-( Maybe Peff or jrnieder would have suggestions; cc'ing them. > } > > current_parsing_scope = CONFIG_SCOPE_COMMAND; > diff --git a/t/helper/test-config.c b/t/helper/test-config.c > index 61da2574c5..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 > * > @@ -86,6 +93,8 @@ int cmd__config(int argc, const char **argv) > const struct string_list *strptr; > 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++; > @@ -93,7 +102,18 @@ int cmd__config(int argc, const char **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 == 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; > } > @@ -101,8 +121,23 @@ int cmd__config(int argc, const char **argv) > setup_git_directory(); > git_configset_init(&cs); > > + 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 == 2 && !strcmp(argv[0], "get_value")) { > - if (!git_config_get_value(argv[1], &v)) { > + if (!repo_config_get_value(repo, argv[1], &v)) { > if (!v) > printf("(NULL)\n"); > else > @@ -112,7 +147,7 @@ int cmd__config(int argc, const char **argv) > ret = TC_VALUE_NOT_FOUND; > } > } else if (argc == 2 && !strcmp(argv[0], "get_value_multi")) { > - strptr = git_config_get_value_multi(argv[1]); > + strptr = repo_config_get_value_multi(repo, argv[1]); > if (strptr) { > for (i = 0; i < strptr->nr; i++) { > v = strptr->items[i].string; > @@ -126,27 +161,33 @@ int cmd__config(int argc, const char **argv) > ret = TC_VALUE_NOT_FOUND; > } > } else if (argc == 2 && !strcmp(argv[0], "get_int")) { > - if (!git_config_get_int(argv[1], &val)) { > + if (!repo_config_get_int(repo, argv[1], &val)) { > printf("%d\n", val); > } else { > printf("Value not found for \"%s\"\n", argv[1]); > ret = TC_VALUE_NOT_FOUND; > } > } else if (argc == 2 && !strcmp(argv[0], "get_bool")) { > - if (!git_config_get_bool(argv[1], &val)) { > + if (!repo_config_get_bool(repo, argv[1], &val)) { > printf("%d\n", val); > } else { > + > printf("Value not found for \"%s\"\n", argv[1]); > ret = TC_VALUE_NOT_FOUND; > } > } else if (argc == 2 && !strcmp(argv[0], "get_string")) { > - if (!git_config_get_string_const(argv[1], &v)) { > + if (!repo_config_get_string_const(repo, argv[1], &v)) { > printf("%s\n", v); > } else { > printf("Value not found for \"%s\"\n", argv[1]); > ret = TC_VALUE_NOT_FOUND; > } > } 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]))) { > @@ -165,6 +206,11 @@ int cmd__config(int argc, const char **argv) > ret = TC_VALUE_NOT_FOUND; > } > } 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]))) { > @@ -187,14 +233,19 @@ int cmd__config(int argc, const char **argv) > ret = TC_VALUE_NOT_FOUND; > } > } else if (!strcmp(argv[0], "iterate")) { > - git_config(iterate_cb, NULL); > + 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
On Tue, Jun 16, 2020 at 4:13 PM Elijah Newren <newren@gmail.com> wrote: > > On Fri, Jun 12, 2020 at 8:45 AM Matheus Tavares > <matheus.bernardino@usp.br> wrote: > > > > config.c | 21 +++++++++--- > > t/helper/test-config.c | 67 +++++++++++++++++++++++++++++++++----- > > t/t2404-worktree-config.sh | 16 +++++++++ > > 3 files changed, 91 insertions(+), 13 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) { > > What happens when opts->git_dir is NULL? (Does that ever even > happen?) Should it fall back to the old code path in that case? Sorry for not replying earlier. Yes, opts->git_dir might be NULL in some cases. I did a quick grep search, though, and it seems that this only happens in two circumstances: (1) in builtin/config.c when startup_info->have_repository is false; and (2) in read_early_config(), if have_git_dir() returns false and discover_git_directory() fails. For (2), I think it is right to ignore the worktree config file when opts->git_dir is NULL because we indeed don't have a repo to read the file from. I'm tempted to say the same for (1), but I'm not very familiar with setup.c. By the definition of have_git_dir() it seems possible to have the_repository->git_dir set up even when startup_info->have_repository == false: int have_git_dir(void) { return startup_info->have_repository || the_repository->gitdir; } Nevertheless, the current calls to config_with_options() either set both opts->git_dir and opts->commondir or none. So if we were to fall back to the_repository->git_dir, for the worktree config, when startup_info->have_repository == false, the local config file would still be ignored during the config sequence in such case. I think it wouldn't make much sense to ignore the local config file but try to load the worktree-specific one, which is also dependent on having a repo, and even more specific. So I think we shouldn't fall back to the old code path. But I would appreciate hearing from others more familiar with this code.
Hi, Matheus Tavares 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-specific > settings, a command executed in the superproject that recurses into the > submodule won't find the said settings. I think the above goes out of order: it states the "how" before the "what". Instead, a commit message should lead with the problem the change aims to solve. Is the idea here that until this patch, we're only able to read worktree config from a repository when extensions.worktreeConfig is set in the_repository, meaning that - when examining submodule config in a process where the_repository represents the superproject, we do not read the submodule's worktree config even if extensions.worktreeConfig is set in the submodule, unless the superproject has extensions.worktreeConfig set, and - when examining submodule config in a process where the_repository represents the superproject, we *do* read the submodule's worktree config even if extensions.worktreeConfig is not set in the submodule, if the superproject has extensions.worktreeConfig set, and ? That sounds like a serious problem indeed. Thanks for fixing it. > This will be especially important in the next 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. I see. I'm not sure that's more important than other cases, but I can understand if the problem was noticed in this circumstance. :) > Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br> > --- > config.c | 21 +++++++++--- > t/helper/test-config.c | 67 +++++++++++++++++++++++++++++++++----- > t/t2404-worktree-config.sh | 16 +++++++++ > 3 files changed, 91 insertions(+), 13 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) { > + if (!opts->ignore_worktree && repo_config && opts->git_dir) { Can we eliminate the repository_format_worktree_config global to save the next caller from the same problem? > + 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) { This undoes the caching the repository_format_worktree_config means to do. Can we cache the value in "struct repository" instead? That way, in the common case where we're reading the_repository, we wouldn't experience a slowdown. > - char *path = git_pathdup("config.worktree"); > + char *path = mkpathdup("%s/config.worktree", opts->git_dir); Can this use a helper like repo_git_path or strbuf_repo_git_path (preferably one using strbuf like the latter)? [...] > + 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 61da2574c5..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. Nice! [...] > @@ -93,7 +102,18 @@ int cmd__config(int argc, const char **argv) > if (argc == 0) > goto print_usage_error; > > + if (skip_prefix(*argv, "--submodule=", &subrepo_path)) { > + argc--; > + argv++; > + if (argc == 0) > + goto print_usage_error; > + } Can this use the parse_options API? > + > 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; Should this use die() or BUG()? > + } > read_early_config(early_config_cb, (void *)argv[1]); > return TC_SUCCESS; > } > @@ -101,8 +121,23 @@ int cmd__config(int argc, const char **argv) > setup_git_directory(); > git_configset_init(&cs); > > + if (subrepo_path) { > + const struct submodule *sub; > + struct repository *subrepo = xcalloc(1, sizeof(*repo)); nit: this could be scoped to cmd__config: struct repository subrepo = {0}; > + > + 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; Likewise: I think may want to use die() or BUG() (and likewise for other USAGE_ERROR cases). Thanks and hope that helps, Jonathan
Hi, Jonathan On Mon, Aug 31, 2020 at 11:41 PM Jonathan Nieder <jrnieder@gmail.com> wrote: > > Hi, > > Matheus Tavares 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-specific > > settings, a command executed in the superproject that recurses into the > > submodule won't find the said settings. > > I think the above goes out of order: it states the "how" before the > "what". Instead, a commit message should lead with the problem the > change aims to solve. Thanks. I will reorder these two sections in the commit message. > Is the idea here that until this patch, we're only able to read > worktree config from a repository when extensions.worktreeConfig is > set in the_repository, meaning that > > - when examining submodule config in a process where the_repository > represents the superproject, we do not read the submodule's worktree > config even if extensions.worktreeConfig is set in the submodule, > unless the superproject has extensions.worktreeConfig set, and Right. > - when examining submodule config in a process where the_repository > represents the superproject, we *do* read the submodule's worktree > config even if extensions.worktreeConfig is not set in the submodule, > if the superproject has extensions.worktreeConfig set, and > > ? Right, but with one change: if extensions.worktreeConfig is not set in the submodule and it is set in the superproject, the *superproject's* worktree config is read (independently of which git_dir was given as argument). > That sounds like a serious problem indeed. Thanks for fixing it. > > > This will be especially important in the next 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. > > I see. I'm not sure that's more important than other cases, but I > can understand if the problem was noticed in this circumstance. :) > > > Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br> > > --- > > config.c | 21 +++++++++--- > > t/helper/test-config.c | 67 +++++++++++++++++++++++++++++++++----- > > t/t2404-worktree-config.sh | 16 +++++++++ > > 3 files changed, 91 insertions(+), 13 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) { > > + if (!opts->ignore_worktree && repo_config && opts->git_dir) { > > Can we eliminate the repository_format_worktree_config global to save > the next caller from the same problem? Hmm, I think it's possible, I will investigate it further. > > + 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) { > > This undoes the caching the repository_format_worktree_config means to > do. Can we cache the value in "struct repository" instead? That way, > in the common case where we're reading the_repository, we wouldn't > experience a slowdown. Yeah, that would be the best solution. But, unfortunately, do_git_config_sequence() doesn't receive a complete repository struct, just the 'commondir' and 'git_dir' strings. > > - char *path = git_pathdup("config.worktree"); > > + char *path = mkpathdup("%s/config.worktree", opts->git_dir); > > Can this use a helper like repo_git_path or strbuf_repo_git_path > (preferably one using strbuf like the latter)? Hmm, here we would have the same problem of not having a 'struct repository' to pass to those functions :( > [...] > > + 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 61da2574c5..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. > > Nice! > > [...] > > @@ -93,7 +102,18 @@ int cmd__config(int argc, const char **argv) > > if (argc == 0) > > goto print_usage_error; > > > > + if (skip_prefix(*argv, "--submodule=", &subrepo_path)) { > > + argc--; > > + argv++; > > + if (argc == 0) > > + goto print_usage_error; > > + } > > Can this use the parse_options API? Right, it would make it easier to add more options in the future. There is only one consideration, though, about parse_options()'s exit codes on error, but more on that below... > > + > > 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; > > Should this use die() or BUG()? The idea of using TC_USAGE_ERROR (129) here and not die() (128), was that some users of the test-config helper want to detect die() errors from the config machinery itself. So by using a different exit code, we can avoid false positives in these tests. Of course they should also be checking stderr/stdout, but there is at least one test which only checks the exit code. Rethinking about that now, instead of using different exit codes in test-config.c, should we adjust the tests to use `test_must_fail` and only check stderr/stdout? Then we could use die() (or BUG()) here, as you suggested, as well as the parse_options API in the snippet above. Does that sound reasonable? > > + } > > read_early_config(early_config_cb, (void *)argv[1]); > > return TC_SUCCESS; > > } > > @@ -101,8 +121,23 @@ int cmd__config(int argc, const char **argv) > > setup_git_directory(); > > git_configset_init(&cs); > > > > + if (subrepo_path) { > > + const struct submodule *sub; > > + struct repository *subrepo = xcalloc(1, sizeof(*repo)); > > nit: this could be scoped to cmd__config: > > struct repository subrepo = {0}; OK, will do. Thanks > > + > > + 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; > > Likewise: I think may want to use die() or BUG() (and likewise for other > USAGE_ERROR cases). > > Thanks and hope that helps, > Jonathan It did :) Thanks a lot for the comments!
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 61da2574c5..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 * @@ -86,6 +93,8 @@ int cmd__config(int argc, const char **argv) const struct string_list *strptr; 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++; @@ -93,7 +102,18 @@ int cmd__config(int argc, const char **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 == 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; } @@ -101,8 +121,23 @@ int cmd__config(int argc, const char **argv) setup_git_directory(); git_configset_init(&cs); + 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 == 2 && !strcmp(argv[0], "get_value")) { - if (!git_config_get_value(argv[1], &v)) { + if (!repo_config_get_value(repo, argv[1], &v)) { if (!v) printf("(NULL)\n"); else @@ -112,7 +147,7 @@ int cmd__config(int argc, const char **argv) ret = TC_VALUE_NOT_FOUND; } } else if (argc == 2 && !strcmp(argv[0], "get_value_multi")) { - strptr = git_config_get_value_multi(argv[1]); + strptr = repo_config_get_value_multi(repo, argv[1]); if (strptr) { for (i = 0; i < strptr->nr; i++) { v = strptr->items[i].string; @@ -126,27 +161,33 @@ int cmd__config(int argc, const char **argv) ret = TC_VALUE_NOT_FOUND; } } else if (argc == 2 && !strcmp(argv[0], "get_int")) { - if (!git_config_get_int(argv[1], &val)) { + if (!repo_config_get_int(repo, argv[1], &val)) { printf("%d\n", val); } else { printf("Value not found for \"%s\"\n", argv[1]); ret = TC_VALUE_NOT_FOUND; } } else if (argc == 2 && !strcmp(argv[0], "get_bool")) { - if (!git_config_get_bool(argv[1], &val)) { + if (!repo_config_get_bool(repo, argv[1], &val)) { printf("%d\n", val); } else { + printf("Value not found for \"%s\"\n", argv[1]); ret = TC_VALUE_NOT_FOUND; } } else if (argc == 2 && !strcmp(argv[0], "get_string")) { - if (!git_config_get_string_const(argv[1], &v)) { + if (!repo_config_get_string_const(repo, argv[1], &v)) { printf("%s\n", v); } else { printf("Value not found for \"%s\"\n", argv[1]); ret = TC_VALUE_NOT_FOUND; } } 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]))) { @@ -165,6 +206,11 @@ int cmd__config(int argc, const char **argv) ret = TC_VALUE_NOT_FOUND; } } 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]))) { @@ -187,14 +233,19 @@ int cmd__config(int argc, const char **argv) ret = TC_VALUE_NOT_FOUND; } } else if (!strcmp(argv[0], "iterate")) { - git_config(iterate_cb, NULL); + 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-specific settings, a command executed in the superproject that recurses into the submodule won't find the said settings. This will be especially important in the next 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 | 67 +++++++++++++++++++++++++++++++++----- t/t2404-worktree-config.sh | 16 +++++++++ 3 files changed, 91 insertions(+), 13 deletions(-)