Hi,

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-specific
settings, a command executed in the superproject that recurses into the
submodule won't find the said settings.

I think the above goes out of order: it states the "how" before the
"what".  Instead, a commit message should lead with the problem the
change aims to solve.

Is the idea here that until this patch, we're only able to read
worktree config from a repository when extensions.worktreeConfig is
set in the_repository, meaning that

- when examining submodule config in a process where the_repository
  represents the superproject, we do not read the submodule's worktree
  config even if extensions.worktreeConfig is set in the submodule,
  unless the superproject has extensions.worktreeConfig set, and

- when examining submodule config in a process where the_repository
  represents the superproject, we *do* read the submodule's worktree
  config even if extensions.worktreeConfig is not set in the submodule,
  if the superproject has extensions.worktreeConfig set, and

?

That sounds like a serious problem indeed.  Thanks for fixing it.

This will be especially important in the next patch: git-grep will learn
to honor sparse checkouts and, when running with --recurse-submodules,
the submodule's sparse checkout settings must be loaded. As these
settings are stored in the config.worktree file, they would be ignored
without this patch. So let's fix this by reading the right
config.worktree file and extensions.worktreeConfig setting, based on the
git_dir and commondir paths given to do_git_config_sequence(). Also
add a test to avoid any regressions.

I see.  I'm not sure that's more important than other cases, but I
can understand if the problem was noticed in this circumstance. :)

Can we eliminate the repository_format_worktree_config global to save
the next caller from the same problem?

+		struct repository_format repo_fmt = REPOSITORY_FORMAT_INIT;
+		struct strbuf buf = STRBUF_INIT;
+
+		read_repository_format(&repo_fmt, repo_config);
+
+		if (!verify_repository_format(&repo_fmt, &buf) &&
+		    repo_fmt.worktree_config) {

This undoes the caching the repository_format_worktree_config means to
do.  Can we cache the value in "struct repository" instead?  That way,
in the common case where we're reading the_repository, we wouldn't
experience a slowdown.

-		char *path = git_pathdup("config.worktree");
+			char *path = mkpathdup("%s/config.worktree", opts->git_dir);

Can this use a helper like repo_git_path or strbuf_repo_git_path
(preferably one using strbuf like the latter)?

[...]

Nice!

[...]

Can this use the parse_options API?

+
 	if (argc == 2 && !strcmp(argv[0], "read_early_config")) {
+		if (subrepo_path) {
+			fprintf(stderr, "Cannot use --submodule with read_early_config\n");
+			return TC_USAGE_ERROR;

Should this use die() or BUG()?

nit: this could be scoped to cmd__config:

	struct repository subrepo = {0};

+
+		sub = submodule_from_path(the_repository, &null_oid, subrepo_path);
+		if (!sub || repo_submodule_init(subrepo, the_repository, sub)) {
+			fprintf(stderr, "Invalid argument to --submodule: '%s'\n",
+				subrepo_path);
+			free(subrepo);
+			ret = TC_USAGE_ERROR;

Likewise: I think may want to use die() or BUG() (and likewise for other
USAGE_ERROR cases).

Thanks and hope that helps,
Jonathan