| Message ID | 882310b69fd3df0acc6823a2c73bbe1801d9f6c4.1589058209.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 |
Matheus Tavares <matheus.bernardino@usp.br> writes: > 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. Thus, when a submodule has a worktree > setting, a command executed in the superproject that recurses into the > submodule won't find the said setting. This has far wider ramifications than just "git grep" and it may be an important fix. Anything that wants to read from a per-worktree configuration is not working as expected when run from a secondary worktree, right? Can we add a test or two to protect this fix from future breakages? > current_parsing_scope = CONFIG_SCOPE_WORKTREE; > - if (!opts->ignore_worktree && repository_format_worktree_config) { > - char *path = git_pathdup("config.worktree"); > + if (!opts->ignore_worktree && repository_format_worktree_config && > + opts->git_dir) { > + 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);
On Mon, May 11, 2020 at 4:10 PM Junio C Hamano <gitster@pobox.com> wrote: > > Matheus Tavares <matheus.bernardino@usp.br> writes: > > > 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. Thus, when a submodule has a worktree > > setting, a command executed in the superproject that recurses into the > > submodule won't find the said setting. > > This has far wider ramifications than just "git grep" and it may be > an important fix. Anything that wants to read from a per-worktree > configuration is not working as expected when run from a secondary > worktree, right? Hmm, I think the code should be able to retrieve the per-worktree configuration, in this case, as the_repository->gitdir will be pointing to the secondary worktree's gitdir. But when we want to read a per-worktree configuration from a repo other than the_repository, then the code doesn't find the setting (even if it is in the main worktree of the subrepo). > Can we add a test or two to protect this fix from future breakages? Sure! There are already a couple tests, in the following patch, that check this behavior *indirectly*. As we recurse into submodules, in grep, we try to retrieve the core.sparseCheckout setting for each submodule (which is stored in the subrepo's config.worktree file). The said tests make sure we can get this setting, and they indeed fail without this patch. But would it be better to also add a more direct test, in this patch? I think we could do so by adding a new test helper that prints submodules' configs, from the superproject, and then testing the presence of per-worktree configs in the output.
Matheus Tavares Bernardino <matheus.bernardino@usp.br> writes: >> Can we add a test or two to protect this fix from future breakages? > > Sure! There are already a couple tests, in the following patch, that > check this behavior *indirectly*. As we recurse into submodules, in > grep, we try to retrieve the core.sparseCheckout setting for each > submodule (which is stored in the subrepo's config.worktree file). The > said tests make sure we can get this setting, and they indeed fail > without this patch. But would it be better to also add a more direct > test, in this patch? I think we could do so by adding a new test > helper that prints submodules' configs, from the superproject, and > then testing the presence of per-worktree configs in the output. Sounds like a plan. Yes, checking by observing how grep that recurses into submodules behave is doable but is indirect, and if any other subcommand that may want to do the recursion will have the same issue that gets fixed by this patch, it's better to ensure that the fix applies to any subcommand in a more direct way. Thanks.
diff --git a/config.c b/config.c index 8db9c77098..a3d0a0d266 100644 --- a/config.c +++ b/config.c @@ -1747,8 +1747,9 @@ 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 (!opts->ignore_worktree && repository_format_worktree_config && + opts->git_dir) { + 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);
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. Thus, when a submodule has a worktree setting, a command executed in the superproject that recurses into the submodule won't find the said setting. 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. The fix is simple, we replace git_pathdup() with mkpathdup(), to format the path with the given git_dir. This is the same idea used to make the config.worktree path in setup.c:check_repository_format_gently(). Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br> --- config.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)