Re: [PATCH v4 4/6] config: correctly read worktree configs in submodules
- From
Matheus Tavares Bernardino <matheus.bernardino@usp.br>
- Date
- Jun 21, 2020, 16:05 UTC
- Message-ID
- <CAHd-oW5vKQMuVEtcnoqYARTAZz4M5bSt+j3R=z_iFcH4-oU6FQ@mail.gmail.com>
- In-Reply-To
- <CABPp-BGdEyEeajYZj_rdxp=MyEQdszuyjVTax=hhYj3fOtRQUQ@mail.gmail.com>
On Tue, Jun 16, 2020 at 4:13 PM Elijah Newren <newren@gmail.com> wrote:
Show 26 quoted lines
>
> 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.