[2/2] config: respect includes in protected config

Commit Message

Glen Choo Oct. 12, 2022, 7:43 p.m. UTC

From: Glen Choo <[e-mail]>

Protected config is implemented by reading a fixed set of paths,
which ignores config [include]-s. Replace this implementation with a
call to config_with_options(), which handles [include]-s and saves us
from duplicating the logic of 1) identifying which paths to read and 2)
reading command line config.

As a result, git_configset_add_parameters() is unused, so remove it. It
was introduced alongside protected config in 5b3c650777 (config: learn
`git_protected_config()`, 2022-07-14) as a way to handle command line
config.

Signed-off-by: Glen Choo <[e-mail]>
---
 config.c                        | 30 ++++++++----------------------
 t/t0033-safe-directory.sh       |  2 +-
 t/t0035-safe-bare-repository.sh |  2 +-
 3 files changed, 10 insertions(+), 24 deletions(-)

Comments

Junio C Hamano Oct. 12, 2022, 8:48 p.m. UTC | #1

"Glen Choo via GitGitGadget" <[e-mail]> writes:

Not commenting on the code yet as I am in the middle of today's
integration run, but as I notice a bad pattern being followed, let
me comment before it spreads too widely.

The "add failing test first and then fix the code with flipping the
test to success" is very much unwelcome.  For whoever gets curious
enough (me included when accepting posted patch), it is easy to
revert only the part of the commit outside t/ tentatively to see how
the original code breaks.  Keeping the fix and protection of the fix
together will avoid mistakes.  The post context of the hunk that
changes test_expect_failure to test_expect_success does not cover
the test script, thereby hiding the body of the test that changes
behaviour while reviewing the patch text, which is another downside.

> diff --git a/t/t0033-safe-directory.sh b/t/t0033-safe-directory.sh
> index 720d6cdd60b..dc3496897ab 100755
> --- a/t/t0033-safe-directory.sh
> +++ b/t/t0033-safe-directory.sh
> @@ -71,7 +71,7 @@ test_expect_success 'safe.directory=*, but is reset' '
>  	expect_rejected_dir
>  '
>  
> -test_expect_failure 'safe.directory in included file' '
> +test_expect_success 'safe.directory in included file' '
>  	cat >gitconfig-include <<-EOF &&
>  	[safe]
>  		directory = "$(pwd)"
> diff --git a/t/t0035-safe-bare-repository.sh b/t/t0035-safe-bare-repository.sh
> index aa6a6a8c3fd..fa33839704b 100755
> --- a/t/t0035-safe-bare-repository.sh
> +++ b/t/t0035-safe-bare-repository.sh
> @@ -51,7 +51,7 @@ test_expect_success 'safe.bareRepository on the command line' '
>  		-c safe.bareRepository=all
>  '
>  
> -test_expect_failure 'safe.bareRepository in included file' '
> +test_expect_success 'safe.bareRepository in included file' '
>  	cat >gitconfig-include <<-EOF &&
>  	[safe]
>  		bareRepository = explicit

Junio C Hamano Oct. 12, 2022, 9:07 p.m. UTC | #2

"Glen Choo via GitGitGadget" <[e-mail]> writes:

> -int git_configset_add_parameters(struct config_set *cs)
> -{
> -	return git_config_from_parameters(config_set_callback, cs);
> -}
> -
>  int git_configset_get_value(struct config_set *cs, const char *key, const char **value)
>  {
>  	const struct string_list *values = NULL;
> @@ -2641,24 +2636,15 @@ int repo_config_get_pathname(struct repository *repo,
>  /* Read values into protected_config. */
>  static void read_protected_config(void)
>  {
> -	char *xdg_config = NULL, *user_config = NULL, *system_config = NULL;
> -
> +	struct config_options opts = {
> +		.respect_includes = 1,
> +		.ignore_repo = 1,
> +		.ignore_worktree = 1,
> +		.system_gently = 1,
> +	};
>  	git_configset_init(&protected_config);
> -
> -	system_config = git_system_config();
> -	git_global_config(&user_config, &xdg_config);
> -
> -	if (system_config)
> -		git_configset_add_file(&protected_config, system_config);
> -	if (xdg_config)
> -		git_configset_add_file(&protected_config, xdg_config);
> -	if (user_config)
> -		git_configset_add_file(&protected_config, user_config);
> -	git_configset_add_parameters(&protected_config);
> -	free(system_config);
> -	free(xdg_config);
> -	free(user_config);

Compared to the above hand-crafted sequence, we seem to be a lot
more careful in config.c::do_git_config_sequence() with respect to
error checking, which is good.  We can lose the custom helper to
read from command line parameters, which is also nice.

> +	config_with_options(config_set_callback, &protected_config,
> +			    NULL, &opts);
>  }

Very nice, code reduction with an additional feature.  I wish all
fixes were like this.

>  config.c                        | 30 ++++++++----------------------
>  t/t0033-safe-directory.sh       |  2 +-
>  t/t0035-safe-bare-repository.sh |  2 +-
>  3 files changed, 10 insertions(+), 24 deletions(-)

Glen Choo Oct. 12, 2022, 10:09 p.m. UTC | #3

Junio C Hamano <[e-mail]> writes:

> "Glen Choo via GitGitGadget" <[e-mail]> writes:
>
> Not commenting on the code yet as I am in the middle of today's
> integration run, but as I notice a bad pattern being followed, let
> me comment before it spreads too widely.
>
> The "add failing test first and then fix the code with flipping the
> test to success" is very much unwelcome.  For whoever gets curious
> enough (me included when accepting posted patch), it is easy to
> revert only the part of the commit outside t/ tentatively to see how
> the original code breaks.  Keeping the fix and protection of the fix
> together will avoid mistakes.  The post context of the hunk that
> changes test_expect_failure to test_expect_success does not cover
> the test script, thereby hiding the body of the test that changes
> behaviour while reviewing the patch text, which is another downside.

Thanks for voicing this, and sorry. I tried this pattern specifically
because I thought it make it easier to review for folks who don't touch
t/, but I hadn't considered that the reverse might be preferred.

Patch

diff --git a/config.c b/config.c
index cbb5a3bab74..c157fb5ae3f 100644
--- a/config.c
+++ b/config.c
@@ -2392,11 +2392,6 @@  int git_configset_add_file(struct config_set *cs, const char *filename)
 	return git_config_from_file(config_set_callback, filename, cs);
 }
 
-int git_configset_add_parameters(struct config_set *cs)
-{
-	return git_config_from_parameters(config_set_callback, cs);
-}
-
 int git_configset_get_value(struct config_set *cs, const char *key, const char **value)
 {
 	const struct string_list *values = NULL;
@@ -2641,24 +2636,15 @@  int repo_config_get_pathname(struct repository *repo,
 /* Read values into protected_config. */
 static void read_protected_config(void)
 {
-	char *xdg_config = NULL, *user_config = NULL, *system_config = NULL;
-
+	struct config_options opts = {
+		.respect_includes = 1,
+		.ignore_repo = 1,
+		.ignore_worktree = 1,
+		.system_gently = 1,
+	};
 	git_configset_init(&protected_config);
-
-	system_config = git_system_config();
-	git_global_config(&user_config, &xdg_config);
-
-	if (system_config)
-		git_configset_add_file(&protected_config, system_config);
-	if (xdg_config)
-		git_configset_add_file(&protected_config, xdg_config);
-	if (user_config)
-		git_configset_add_file(&protected_config, user_config);
-	git_configset_add_parameters(&protected_config);
-
-	free(system_config);
-	free(xdg_config);
-	free(user_config);
+	config_with_options(config_set_callback, &protected_config,
+			    NULL, &opts);
 }
 
 void git_protected_config(config_fn_t fn, void *data)
diff --git a/t/t0033-safe-directory.sh b/t/t0033-safe-directory.sh
index 720d6cdd60b..dc3496897ab 100755
--- a/t/t0033-safe-directory.sh
+++ b/t/t0033-safe-directory.sh
@@ -71,7 +71,7 @@  test_expect_success 'safe.directory=*, but is reset' '
 	expect_rejected_dir
 '
 
-test_expect_failure 'safe.directory in included file' '
+test_expect_success 'safe.directory in included file' '
 	cat >gitconfig-include <<-EOF &&
 	[safe]
 		directory = "$(pwd)"
diff --git a/t/t0035-safe-bare-repository.sh b/t/t0035-safe-bare-repository.sh
index aa6a6a8c3fd..fa33839704b 100755
--- a/t/t0035-safe-bare-repository.sh
+++ b/t/t0035-safe-bare-repository.sh
@@ -51,7 +51,7 @@  test_expect_success 'safe.bareRepository on the command line' '
 		-c safe.bareRepository=all
 '
 
-test_expect_failure 'safe.bareRepository in included file' '
+test_expect_success 'safe.bareRepository in included file' '
 	cat >gitconfig-include <<-EOF &&
 	[safe]
 		bareRepository = explicit