From: Matheus Tavares Bernardino Date: Mon, 01 Jun 2020 04:38:17 GMT Subject: Re: [PATCH v3 3/5] config: correctly read worktree configs in submodules Message-ID: In-Reply-To: On Sat, May 30, 2020 at 11:49 AM Elijah Newren wrote: > > On Wed, May 27, 2020 at 6:13 PM Matheus Tavares > 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..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..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.