Re: [PATCH v4 4/6] config: correctly read worktree configs in submodules

From: Matheus Tavares Bernardino <matheus.bernardino@usp.br>
To: Elijah Newren <newren@gmail.com>
Cc: Git Mailing List <git@vger.kernel.org>,
	Junio C Hamano <gitster@pobox.com>,
	Derrick Stolee <stolee@gmail.com>,
	Jonathan Tan <jonathantanmy@google.com>,
	Jeff King <peff@peff.net>, Jonathan Nieder <jrnieder@gmail.com>
Subject: Re: [PATCH v4 4/6] config: correctly read worktree configs in submodules
Date: Sun, 21 Jun 2020 13:05:04 -0300	[thread overview]
Message-ID: <CAHd-oW5vKQMuVEtcnoqYARTAZz4M5bSt+j3R=z_iFcH4-oU6FQ@mail.gmail.com> (raw)
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:
>
> 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.