On Sat, May 30, 2020 at 11:49 AM Elijah Newren <newren@xxxxxxxxx> wrote:
>
> On Wed, May 27, 2020 at 6:13 PM Matheus Tavares
> <matheus.bernardino@xxxxxx> 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.
