Re: [PATCH v3 3/5] 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>
Subject: Re: [PATCH v3 3/5] config: correctly read worktree configs in submodules
Date: Mon, 1 Jun 2020 01:38:17 -0300	[thread overview]
Message-ID: <CAHd-oW7==zM5goS6ZrHNYADDgdkYdXLqZgKxCL2UwFAacvVsNg@mail.gmail.com> (raw)
In-Reply-To: <CABPp-BGVaMM9Naiu3HeO6n2aOxsHwiuc813MF-hFWezSFa9CsA@mail.gmail.com>

On Sat, May 30, 2020 at 11:49 AM Elijah Newren <newren@gmail.com> wrote:
>
> On Wed, May 27, 2020 at 6:13 PM 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 settings, a
> > command executed in the superproject that recurses into the submodule
> > won't find the said settings.
> >
> > Such a scenario might not be needed now, but it will be in the following
>
> It's not needed?  Are there not other config values that affect grep's
> behavior, such as smudge filters of the submodule that might be
> important if doing a 'git grep --recurse-submodules $REVISION'?

Hmm, I haven't used smudge filters before, but it seems to me that
`git grep $REVISION` does not honor them.

> Also, is there a similar issue here for .gitattributes?  (e.g. if the
> submodule declares certain files to be binary?)

Declaring files as binary in the submodule works fine. But I noticed
that textconv filter specifications in the submodule's config are
currently ignored. To be honest, I wasn't aware of this issue before.

> I don't actually know if these are issues but I'm just surprised to
> hear that this would be the first case that would need to look at
> submodule-specific configuration.

Hmm, not to submodule-specific configuration but to worktree-specific
configuration of a submodule, right? I.e. a config.worktree file from
within a submodule. Reconsidering this now, we could indeed have a
diff.<driver>.textconv or core.quotePath settings specified in the
worktree scope of a submodule. And we should honor them when recursing
in grep. I guess I thought the "most natural" place for these
settings, in a submodule, would be in the standard .git/config file
(as opposed to the sparse-checkout ones, which are normally at
config.worktree). That's probably why I wrote "Such scenario might not
be needed now". But we should indeed support reading
diff.<driver>.textconv from config.worktree as well (although grep
currently ignores this setting in submodules, both in the local and
worktree scopes). So the said sentence doesn't make much sense,
indeed. I will remove it. Thanks!

> > diff --git a/t/helper/test-config.c b/t/helper/test-config.c
> > index 1c8e965840..284f83a921 100644
> > --- a/t/helper/test-config.c
> > +++ b/t/helper/test-config.c
> > @@ -84,33 +91,63 @@ int cmd__config(int argc, const char **argv)
> >         int i, val;
> >         const char *v;
> >         const struct string_list *strptr;
> > -       struct config_set cs;
> > +       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" */
>
> This line alone is responsible for a fairly big set of changes
> throughout this file, just decrementing indices everywhere.  It might
> be nice for review purposes if this and the other changes it caused
> were pulled out into a separate step, so we can more easily
> concentrate on the primary additions and changes you are making to
> this file.

OK, will do.