On Tue, Jun 16, 2020 at 4:13 PM Elijah Newren <newren@xxxxxxxxx> wrote:
>
> On Fri, Jun 12, 2020 at 8:45 AM Matheus Tavares
> <matheus.bernardino@xxxxxx> 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.
