config: respect includes in protected config

PATCH Superseded Glen Choo via GitGitGadget 6 participants 13 messages Oct 12, 2022 – Oct 18, 2022

Share & tools

Glen Choo via GitGitGadget

author

2022-10-12 19:43 UTC #0 lore raw

cover config: respect includes in protected config

Here's a fix for the regression reported in [1]. I'm quite confident that the test suite is extensive enough to catch any unintended behavioral changes, but I'd appreciate a second opinion.

Cc: Derrick Stolee [email protected], Taylor Blau [email protected], Johannes Schindelin [email protected], Junio C Hamano [email protected]

[1]
https://lore.kernel.org/git/CAPWNY8W_Tr-WoD-GXBddD5Y8w5Y4w+gDNYQdOAJ1uBwVHuRrsQ@mail.gmail.com

Glen Choo (2):
t0033, t0035: test for included config
config: respect includes in protected config

 config.c                        | 30 ++++++++----------------------
 t/t0033-safe-directory.sh       |  9 +++++++++
 t/t0035-safe-bare-repository.sh |  9 +++++++++
 3 files changed, 26 insertions(+), 22 deletions(-)

base-commit: 3dcec76d9df911ed8321007b1d197c1a206dc164
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1360%2Fchooglen%2Fprotected-config%2Frespect-includes-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1360/chooglen/protected-config/respect-includes-v1
Pull-Request: https://github.com/git/git/pull/1360

Signature · 1 line

gitgitgadget

Glen Choo via GitGitGadget

author

2022-10-12 19:43 UTC #1 lore raw

PATCH 1/2 t0033, t0035: test for included config

From: Glen Choo <[email protected]>

Protected config should consider [include]-s. Add failing tests that describe the behavior we want; they will pass in the next commit.

Signed-off-by Glen Choo <[email protected]>

---
 t/t0033-safe-directory.sh       | 9 +++++++++
 t/t0035-safe-bare-repository.sh | 9 +++++++++
 2 files changed, 18 insertions(+)

t/t0033-safe-directory.sh +9 -0

index aecb308cf66..720d6cdd60b 100755
@@ -71,4 +71,13 @@ test_expect_success 'safe.directory=*, but is reset' '
 	expect_rejected_dir
 '
 
+test_expect_failure 'safe.directory in included file' '
+	cat >gitconfig-include <<-EOF &&
+	[safe]
+		directory = "$(pwd)"
+	EOF
+	git config --global --add include.path "$(pwd)/gitconfig-include" &&
+	git status
+'
+
 test_done

t/t0035-safe-bare-repository.sh +9 -0

index ecbdc8238db..aa6a6a8c3fd 100755
@@ -51,4 +51,13 @@ test_expect_success 'safe.bareRepository on the command line' '
 		-c safe.bareRepository=all
 '
 
+test_expect_failure 'safe.bareRepository in included file' '
+	cat >gitconfig-include <<-EOF &&
+	[safe]
+		bareRepository = explicit
+	EOF
+	git config --global --add include.path "$(pwd)/gitconfig-include" &&
+	expect_rejected -C outer-repo/bare-repo
+'
+
 test_done

Signature · 1 line

gitgitgadget

Glen Choo via GitGitGadget

author

2022-10-12 19:43 UTC #3 lore raw

PATCH 2/2 config: respect includes in protected config

In reply to Glen Choo via GitGitGadget

From: Glen Choo <[email protected]>

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 <[email protected]>

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

config.c +8 ~~-22~~

index cbb5a3bab74..c157fb5ae3f 100644
@@ -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)

t/t0033-safe-directory.sh +1 -1

index 720d6cdd60b..dc3496897ab 100755
@@ -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)"

t/t0035-safe-bare-repository.sh +1 -1

index aa6a6a8c3fd..fa33839704b 100755
@@ -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

Signature · 1 line

gitgitgadget

Junio C Hamano

2022-10-12 20:48 UTC #4 lore raw

"Glen Choo via GitGitGadget" <[email protected]> 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.

Quoted 26 lines

t/t0033-safe-directory.sh +1 -1

index 720d6cdd60b..dc3496897ab 100755
@@ -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)"

t/t0035-safe-bare-repository.sh +1 -1

index aa6a6a8c3fd..fa33839704b 100755
@@ -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

Glen Choo

2022-10-12 22:11 UTC #5 lore raw

Quoted 6 lines · Junio C Hamano

"Glen Choo via GitGitGadget" <[email protected]> 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.

Quoted 9 lines

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.

Junio C Hamano

2022-10-12 21:08 UTC #6 lore raw

In reply to Glen Choo via GitGitGadget

Quoted 25 lines · Glen Choo via GitGitGadget

config.c +6 ~~-11~~

-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);
-

config.c int repo_config_get_pathname(struct repository *repo, · line 2660

Quoted 10 lines

config.c +0 ~~-10~~

-	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.c int repo_config_get_pathname(struct repository *repo, · line 2648

Quoted 3 lines

config.c +2 -0

+	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.

Quoted 4 lines

(quoted diff) +0 -0

 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

2022-10-12 19:48 UTC #7 lore raw

In reply to Glen Choo via GitGitGadget

Cc-ing folks again. Looks like I made a mistake initially, sorry for the noise.

Quoted 27 lines · Glen Choo via GitGitGadget

Here's a fix for the regression reported in [1]. I'm quite confident that the test suite is extensive enough to catch any unintended behavioral changes, but I'd appreciate a second opinion.
Cc: Derrick Stolee [email protected], Taylor Blau [email protected], Johannes Schindelin [email protected], Junio C Hamano [email protected]
[1]
https://lore.kernel.org/git/CAPWNY8W_Tr-WoD-GXBddD5Y8w5Y4w+gDNYQdOAJ1uBwVHuRrsQ@mail.gmail.com
Glen Choo (2):
t0033, t0035: test for included config
config: respect includes in protected config
 config.c                        | 30 ++++++++----------------------
 t/t0033-safe-directory.sh       |  9 +++++++++
 t/t0035-safe-bare-repository.sh |  9 +++++++++
 3 files changed, 26 insertions(+), 22 deletions(-)
base-commit: 3dcec76d9df911ed8321007b1d197c1a206dc164
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1360%2Fchooglen%2Fprotected-config%2Frespect-includes-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1360/chooglen/protected-config/respect-includes-v1
Pull-Request: https://github.com/git/git/pull/1360
Signature · 1 line
gitgitgadget

Junio C Hamano

2022-10-12 20:54 UTC #8 lore raw

In reply to Glen Choo via GitGitGadget

Quoted 3 lines · Glen Choo via GitGitGadget

Here's a fix for the regression reported in [1]. I'm quite confident that the test suite is extensive enough to catch any unintended behavioral changes, but I'd appreciate a second opinion.

This hooks onto safe.directory that dates back to 2.30.4 or so, but the topic gc/bare-repo-discovery appears only in 2.38 so we do not have to backport anything beyond the current maintenance track. Whew ;-)

Quoted 21 lines

Cc: Derrick Stolee [email protected], Taylor Blau [email protected], Johannes Schindelin [email protected], Junio C Hamano [email protected]
[1]
https://lore.kernel.org/git/CAPWNY8W_Tr-WoD-GXBddD5Y8w5Y4w+gDNYQdOAJ1uBwVHuRrsQ@mail.gmail.com
Glen Choo (2):
t0033, t0035: test for included config
config: respect includes in protected config
 config.c                        | 30 ++++++++----------------------
 t/t0033-safe-directory.sh       |  9 +++++++++
 t/t0035-safe-bare-repository.sh |  9 +++++++++
 3 files changed, 26 insertions(+), 22 deletions(-)
base-commit: 3dcec76d9df911ed8321007b1d197c1a206dc164
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1360%2Fchooglen%2Fprotected-config%2Frespect-includes-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1360/chooglen/protected-config/respect-includes-v1
Pull-Request: https://github.com/git/git/pull/1360

Glen Choo via GitGitGadget

author

2022-10-13 17:43 UTC #9 lore raw

PATCH v2 1/1 config: respect includes in protected config

In reply to Glen Choo via GitGitGadget

From: Glen Choo <[email protected]>

Signed-off-by Glen Choo <[email protected]>

---
[2.38 regression] config: respect includes in protected config

Thanks for the quick response, all :)

Changes in v2:

* Squash patches together (per Junio's comments)
* Use <<-\EOF (per Ævar's comments)

Cc: Derrick Stolee [email protected] Cc: Taylor Blau [email protected] Cc: Johannes Schindelin [email protected] Cc:
Junio C Hamano [email protected] Cc: Ævar Arnfjörð Bjarmason [email protected]

[1]
https://lore.kernel.org/git/CAPWNY8W_Tr-WoD-GXBddD5Y8w5Y4w+gDNYQdOAJ1uBwVHuRrsQ@mail.gmail.com

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1360%2Fchooglen%2Fprotected-config%2Frespect-includes-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1360/chooglen/protected-config/respect-includes-v2
Pull-Request: https://github.com/git/git/pull/1360

Range-diff vs v1:

 1:  8c0f40aed7e < -:  ----------- t0033, t0035: test for included config
 2:  0ff5b5741a5 ! 1:  5c398a7f72a config: respect includes in protected config

@@ t/t0033-safe-directory.sh: 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)"
++ cat >gitconfig-include <<-EOF &&
++ [safe]
++ directory = "$(pwd)"
++ EOF
++ git config --global --add include.path "$(pwd)/gitconfig-include" &&
++ git status
++'
++
+ test_done

## t/t0035-safe-bare-repository.sh ##
@@ t/t0035-safe-bare-repository.sh: 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
++ cat >gitconfig-include <<-\EOF &&
++ [safe]
++ bareRepository = explicit
++ EOF
++ git config --global --add include.path "$(pwd)/gitconfig-include" &&
++ expect_rejected -C outer-repo/bare-repo
++'
++
+ test_done

 config.c                        | 30 ++++++++----------------------
 t/t0033-safe-directory.sh       |  9 +++++++++
 t/t0035-safe-bare-repository.sh |  9 +++++++++
 3 files changed, 26 insertions(+), 22 deletions(-)

config.c +8 ~~-22~~

index cbb5a3bab74..c157fb5ae3f 100644
@@ -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)

t/t0033-safe-directory.sh +9 -0

index aecb308cf66..dc3496897ab 100755
@@ -71,4 +71,13 @@ test_expect_success 'safe.directory=*, but is reset' '
 	expect_rejected_dir
 '
 
+test_expect_success 'safe.directory in included file' '
+	cat >gitconfig-include <<-EOF &&
+	[safe]
+		directory = "$(pwd)"
+	EOF
+	git config --global --add include.path "$(pwd)/gitconfig-include" &&
+	git status
+'
+
 test_done

t/t0035-safe-bare-repository.sh +9 -0

index ecbdc8238db..11c15a48aab 100755
@@ -51,4 +51,13 @@ test_expect_success 'safe.bareRepository on the command line' '
 		-c safe.bareRepository=all
 '
 
+test_expect_success 'safe.bareRepository in included file' '
+	cat >gitconfig-include <<-\EOF &&
+	[safe]
+		bareRepository = explicit
+	EOF
+	git config --global --add include.path "$(pwd)/gitconfig-include" &&
+	expect_rejected -C outer-repo/bare-repo
+'
+
 test_done

base-commit: 3dcec76d9df911ed8321007b1d197c1a206dc164

Signature · 1 line

gitgitgadget

Glen Choo

2022-10-13 18:27 UTC #10 lore raw

Quoted 4 lines · Glen Choo via GitGitGadget

Cc: Derrick Stolee [email protected] Cc: Taylor Blau [email protected] Cc: Johannes Schindelin [email protected] Cc:
Junio C Hamano [email protected] Cc: Ævar Arnfjörð Bjarmason [email protected]

Ack, cc-ing folks again. I was quite certain I got it correct this time though...

Jeff King

2022-10-14 20:14 UTC #11 lore raw

In reply to Glen Choo via GitGitGadget

Quoted 7 lines · Glen Choo via GitGitGadget

From: Glen Choo <[email protected]>
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.

FWIW, this left me scratching my head for a moment, as I thought it was advocating reading includes from unsafe sources (which, since they can specify arbitrary paths, can cause various bits of mischief).

But looking at the patch, you mean that when reading potentially-unsafe sources, we'll skip the unsafe ones (like .git/config) entirely, but we fail to respect includes in the safe ones (like ~/.gitconfig)? That makes sense.

-Peff

Johannes Schindelin

2022-10-18 15:22 UTC #12 lore raw

In reply to Glen Choo via GitGitGadget

Hi Glen,

Quoted 7 lines · Glen Choo via GitGitGadget

From: Glen Choo <[email protected]>
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.

After reading the patch, I agree that this is the right thing to do.

Thank you,
Dscho

Quoted 168 lines

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 <[email protected]>
---
[2.38 regression] config: respect includes in protected config
Thanks for the quick response, all :)
Changes in v2:
* Squash patches together (per Junio's comments)
* Use <<-\EOF (per Ævar's comments)
Cc: Derrick Stolee [email protected] Cc: Taylor Blau [email protected] Cc: Johannes Schindelin [email protected] Cc:
Junio C Hamano [email protected] Cc: Ævar Arnfjörð Bjarmason [email protected]
[1]
https://lore.kernel.org/git/CAPWNY8W_Tr-WoD-GXBddD5Y8w5Y4w+gDNYQdOAJ1uBwVHuRrsQ@mail.gmail.com
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1360%2Fchooglen%2Fprotected-config%2Frespect-includes-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1360/chooglen/protected-config/respect-includes-v2
Pull-Request: https://github.com/git/git/pull/1360
Range-diff vs v1:
 1:  8c0f40aed7e < -:  ----------- t0033, t0035: test for included config
 2:  0ff5b5741a5 ! 1:  5c398a7f72a config: respect includes in protected config
@@ t/t0033-safe-directory.sh: 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)"
++ cat >gitconfig-include <<-EOF &&
++ [safe]
++ directory = "$(pwd)"
++ EOF
++ git config --global --add include.path "$(pwd)/gitconfig-include" &&
++ git status
++'
++
+ test_done
## t/t0035-safe-bare-repository.sh ##
@@ t/t0035-safe-bare-repository.sh: 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
++ cat >gitconfig-include <<-\EOF &&
++ [safe]
++ bareRepository = explicit
++ EOF
++ git config --global --add include.path "$(pwd)/gitconfig-include" &&
++ expect_rejected -C outer-repo/bare-repo
++'
++
+ test_done
 config.c                        | 30 ++++++++----------------------
 t/t0033-safe-directory.sh       |  9 +++++++++
 t/t0035-safe-bare-repository.sh |  9 +++++++++
 3 files changed, 26 insertions(+), 22 deletions(-)
config.c +8 ~~-22~~
index cbb5a3bab74..c157fb5ae3f 100644
@@ -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)
t/t0033-safe-directory.sh +9 -0
index aecb308cf66..dc3496897ab 100755
@@ -71,4 +71,13 @@ test_expect_success 'safe.directory=*, but is reset' '
 	expect_rejected_dir
 '
 
+test_expect_success 'safe.directory in included file' '
+	cat >gitconfig-include <<-EOF &&
+	[safe]
+		directory = "$(pwd)"
+	EOF
+	git config --global --add include.path "$(pwd)/gitconfig-include" &&
+	git status
+'
+
 test_done
t/t0035-safe-bare-repository.sh +9 -0
index ecbdc8238db..11c15a48aab 100755
@@ -51,4 +51,13 @@ test_expect_success 'safe.bareRepository on the command line' '
 		-c safe.bareRepository=all
 '
 
+test_expect_success 'safe.bareRepository in included file' '
+	cat >gitconfig-include <<-\EOF &&
+	[safe]
+		bareRepository = explicit
+	EOF
+	git config --global --add include.path "$(pwd)/gitconfig-include" &&
+	expect_rejected -C outer-repo/bare-repo
+'
+
 test_done
base-commit: 3dcec76d9df911ed8321007b1d197c1a206dc164 --
gitgitgadget