[1/2] config: use gitdir to get worktree config

Commit Message

Victoria Dye May 23, 2023, 11:17 p.m. UTC

From: Victoria Dye <[e-mail]>

Update 'do_git_config_sequence()' to read the worktree config from
'config.worktree' in 'opts->git_dir' rather than the gitdir of
'the_repository'.

The worktree config is loaded from the path returned by
'git_pathdup("config.worktree")', the 'config.worktree' relative to the
gitdir of 'the_repository'. If loading the config for a submodule, this path
is incorrect, since 'the_repository' is the super project. Conversely,
'opts->git_dir' is the gitdir of the submodule being configured, so the
config file in that location should be read instead.

To ensure the use of 'opts->git_dir' is safe, require that 'opts->git_dir'
is set if-and-only-if 'opts->commondir' is set (rather than "only-if" as it
is now). In all current usage of 'config_options', these values are set
together, so the stricter check does not change any behavior.

Finally, add tests to 't3007-ls-files-recurse-submodules.sh' to demonstrate
the corrected config loading behavior. Note that behavior still isn't ideal
because 'extensions.worktreeConfig' in the super project controls whether or
not the worktree config is used in the submodule. This will be fixed in a
later patch.

Signed-off-by: Victoria Dye <[e-mail]>
---
 config.c                               | 28 +++++++++++++++++---------
 t/t3007-ls-files-recurse-submodules.sh | 23 +++++++++++++++++++++
 2 files changed, 42 insertions(+), 9 deletions(-)

Comments

Glen Choo May 25, 2023, 1:05 a.m. UTC | #1

"Victoria Dye via GitGitGadget" <[e-mail]> writes:

> From: Victoria Dye <[e-mail]>
>
> Update 'do_git_config_sequence()' to read the worktree config from
> 'config.worktree' in 'opts->git_dir' rather than the gitdir of
> 'the_repository'.

Thanks for the patches! This makes sense. do_git_config_sequence() is
eventually called by repo_config(), which is supposed to read config
into a "struct repository", so any reliance on the_repository's settings
is wrong.

>                                        Note that behavior still isn't ideal
> because 'extensions.worktreeConfig' in the super project[...]

Nit: We typically use "superproject" without the space.

> diff --git a/config.c b/config.c
> index b79baf83e35..a93f7bfa3aa 100644
> --- a/config.c
> +++ b/config.c
> @@ -2200,14 +2200,24 @@ static int do_git_config_sequence(struct config_reader *reader,
>  	char *xdg_config = NULL;
>  	char *user_config = NULL;
>  	char *repo_config;
> +	char *worktree_config;
>  	enum config_scope prev_parsing_scope = reader->parsing_scope;
>  
> -	if (opts->commondir)
> +	/*
> +	 * Ensure that either:
> +	 * - the git_dir and commondir are both set, or
> +	 * - the git_dir and commondir are both NULL
> +	 */
> +	if (!opts->git_dir != !opts->commondir)
> +		BUG("only one of commondir and git_dir is non-NULL");
> +
> +	if (opts->commondir) {
>  		repo_config = mkpathdup("%s/config", opts->commondir);
> -	else if (opts->git_dir)
> -		BUG("git_dir without commondir");
> -	else
> +		worktree_config = mkpathdup("%s/config.worktree", opts->git_dir);
> +	} else {
>  		repo_config = NULL;
> +		worktree_config = NULL;
> +	}

Makes sense to me. I don't see why we would ever want to set one without
the other.

I looked into whether we could get replace opts->commondir and
opts->git_dir with a "struct repository" arg, but unfortunately
read_early_config() needs to pass these values without touching
"the_repository".

> diff --git a/t/t3007-ls-files-recurse-submodules.sh b/t/t3007-ls-files-recurse-submodules.sh
> index dd7770e85de..e35c203241f 100755
> --- a/t/t3007-ls-files-recurse-submodules.sh
> +++ b/t/t3007-ls-files-recurse-submodules.sh
> @@ -299,6 +299,29 @@ test_expect_success '--recurse-submodules does not support --error-unmatch' '
>  	test_i18ngrep "does not support --error-unmatch" actual
>  '
>  
> +test_expect_success '--recurse-submodules parses submodule repo config' '
> +	test_when_finished "git -C submodule config --unset feature.experimental" &&
> +	git -C submodule config feature.experimental "invalid non-boolean value" &&
> +	test_must_fail git ls-files --recurse-submodules 2>err &&
> +	grep "bad boolean config value" err
> +'

This test has a few bits that are important but non-obvious. It would be
useful to capture them in either the commit message or a comment.

Firstly, we can't test this using "git config" because that only uses
the_repository, and we specifically need to read config in-core into a
"struct repository" that is a submodule, so we need a command that
recurses into a submodule without using subprocesses. IIRC the only
choices are "git grep" and "git ls-files".

Secondly, when we test that config is read from the submodule the choice
of "feature.experimental" is quite important. The config is read quite
indirectly: "git ls-files" reads from the submodule's index, which
will call prepare_repo_settings() on the submodule, and eventually calls
repo_config_get_bool() on "feature.experimental". Any of the configs in
prepare_repo_settings() should do, though. A tiny suggestion would be to
use "index.sparse" instead of "feature.experimental", since (I presume)
we'll have to add sparse index + submodule tests for "git ls-files"
eventually.

> +test_expect_success '--recurse-submodules parses submodule worktree config' '
> +	test_when_finished "git -C submodule config --unset extensions.worktreeConfig" &&

I believe "test_config -C" will achieve the desired effect.

Derrick Stolee May 25, 2023, 8:05 p.m. UTC | #2

On 5/24/2023 9:05 PM, Glen Choo wrote:
> "Victoria Dye via GitGitGadget" <[e-mail]> writes:
> 
>> From: Victoria Dye <[e-mail]>
>>
>> Update 'do_git_config_sequence()' to read the worktree config from
>> 'config.worktree' in 'opts->git_dir' rather than the gitdir of
>> 'the_repository'.
> 
> Thanks for the patches! This makes sense. do_git_config_sequence() is
> eventually called by repo_config(), which is supposed to read config
> into a "struct repository", so any reliance on the_repository's settings
> is wrong.

>> +test_expect_success '--recurse-submodules parses submodule repo config' '
>> +	test_when_finished "git -C submodule config --unset feature.experimental" &&
>> +	git -C submodule config feature.experimental "invalid non-boolean value" &&
>> +	test_must_fail git ls-files --recurse-submodules 2>err &&
>> +	grep "bad boolean config value" err
>> +'
> 
> This test has a few bits that are important but non-obvious. It would be
> useful to capture them in either the commit message or a comment.
> 
> Firstly, we can't test this using "git config" because that only uses
> the_repository, and we specifically need to read config in-core into a
> "struct repository" that is a submodule, so we need a command that
> recurses into a submodule without using subprocesses. IIRC the only
> choices are "git grep" and "git ls-files".
> 
> Secondly, when we test that config is read from the submodule the choice
> of "feature.experimental" is quite important. The config is read quite
> indirectly: "git ls-files" reads from the submodule's index, which
> will call prepare_repo_settings() on the submodule, and eventually calls
> repo_config_get_bool() on "feature.experimental". Any of the configs in
> prepare_repo_settings() should do, though. A tiny suggestion would be to
> use "index.sparse" instead of "feature.experimental", since (I presume)
> we'll have to add sparse index + submodule tests for "git ls-files"
> eventually.

Some of the points you bring up are definitely subtle, like the choice
of config variable.

I appreciate that there are two tests here: one to verify the test
checks have a similar effect without using the worktree config, and
then a second test to show the same behavior with worktree config.

If I understand correctly, the first test would pass without this
code change, but it is a helpful one to help add confidence in the
second test.
 
> +test_expect_success '--recurse-submodules parses submodule worktree config' '
>> +	test_when_finished "git -C submodule config --unset extensions.worktreeConfig" &&
> 
> I believe "test_config -C" will achieve the desired effect.

This should work, though it requires acting a bit strangely, at least
if we want to replace the 'git config --worktree' command.

test_config treats the positions of the arguments as special, so we
would need to write it as:

 test_config -C submodule feature.experimental --worktree "non boolean value"

and that's assuming that 'git -C submodule config feature.experimental
--worktree "non boolean value"' is parsed correctly to use the --worktree
argument. (I haven't tried it.) By using this order, that allows the
test_config helper to run the appropriate 'test_when_finished git config
--unset feature.experimental' command.

Thanks,
-Stolee

Patch

diff --git a/config.c b/config.c
index b79baf83e35..a93f7bfa3aa 100644
--- a/config.c
+++ b/config.c
@@ -2200,14 +2200,24 @@  static int do_git_config_sequence(struct config_reader *reader,
 	char *xdg_config = NULL;
 	char *user_config = NULL;
 	char *repo_config;
+	char *worktree_config;
 	enum config_scope prev_parsing_scope = reader->parsing_scope;
 
-	if (opts->commondir)
+	/*
+	 * Ensure that either:
+	 * - the git_dir and commondir are both set, or
+	 * - the git_dir and commondir are both NULL
+	 */
+	if (!opts->git_dir != !opts->commondir)
+		BUG("only one of commondir and git_dir is non-NULL");
+
+	if (opts->commondir) {
 		repo_config = mkpathdup("%s/config", opts->commondir);
-	else if (opts->git_dir)
-		BUG("git_dir without commondir");
-	else
+		worktree_config = mkpathdup("%s/config.worktree", opts->git_dir);
+	} else {
 		repo_config = NULL;
+		worktree_config = NULL;
+	}
 
 	config_reader_set_scope(reader, CONFIG_SCOPE_SYSTEM);
 	if (git_config_system() && system_config &&
@@ -2230,11 +2240,10 @@  static int do_git_config_sequence(struct config_reader *reader,
 		ret += git_config_from_file(fn, repo_config, data);
 
 	config_reader_set_scope(reader, CONFIG_SCOPE_WORKTREE);
-	if (!opts->ignore_worktree && repository_format_worktree_config) {
-		char *path = git_pathdup("config.worktree");
-		if (!access_or_die(path, R_OK, 0))
-			ret += git_config_from_file(fn, path, data);
-		free(path);
+	if (!opts->ignore_worktree && worktree_config &&
+	    repository_format_worktree_config &&
+	    !access_or_die(worktree_config, R_OK, 0)) {
+		ret += git_config_from_file(fn, worktree_config, data);
 	}
 
 	config_reader_set_scope(reader, CONFIG_SCOPE_COMMAND);
@@ -2246,6 +2255,7 @@  static int do_git_config_sequence(struct config_reader *reader,
 	free(xdg_config);
 	free(user_config);
 	free(repo_config);
+	free(worktree_config);
 	return ret;
 }
 
diff --git a/t/t3007-ls-files-recurse-submodules.sh b/t/t3007-ls-files-recurse-submodules.sh
index dd7770e85de..e35c203241f 100755
--- a/t/t3007-ls-files-recurse-submodules.sh
+++ b/t/t3007-ls-files-recurse-submodules.sh
@@ -299,6 +299,29 @@  test_expect_success '--recurse-submodules does not support --error-unmatch' '
 	test_i18ngrep "does not support --error-unmatch" actual
 '
 
+test_expect_success '--recurse-submodules parses submodule repo config' '
+	test_when_finished "git -C submodule config --unset feature.experimental" &&
+	git -C submodule config feature.experimental "invalid non-boolean value" &&
+	test_must_fail git ls-files --recurse-submodules 2>err &&
+	grep "bad boolean config value" err
+'
+
+test_expect_success '--recurse-submodules parses submodule worktree config' '
+	test_when_finished "git -C submodule config --unset extensions.worktreeConfig" &&
+	test_when_finished "git -C submodule config --worktree --unset feature.experimental" &&
+	test_when_finished "git config --unset extensions.worktreeConfig" &&
+
+	git -C submodule config extensions.worktreeConfig true &&
+	git -C submodule config --worktree feature.experimental "invalid non-boolean value" &&
+
+	# NEEDSWORK: the extensions.worktreeConfig is set globally based on super
+	# project, so we need to enable it in the super project.
+	git config extensions.worktreeConfig true &&
+
+	test_must_fail git ls-files --recurse-submodules 2>err &&
+	grep "bad boolean config value" err
+'
+
 test_incompatible_with_recurse_submodules () {
 	test_expect_success "--recurse-submodules and $1 are incompatible" "
 		test_must_fail git ls-files --recurse-submodules $1 2>actual &&