Re: [PATCH v3 3/5] config: correctly read worktree configs in submodules

On Sat, May 30, 2020 at 11:49 AM Elijah Newren [off-list ref] wrote:

On Wed, May 27, 2020 at 6:13 PM Matheus Tavares
[off-list ref] wrote:

quoted

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!

quoted

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.