threads / patch / 58615

patch, 2 parts[2.38 regression] config: respect includes in protected config

Subject: [PATCH 0/2] [2.38 regression] config: respect includes in protected config

## tl;dr

13 messages between Oct 12, 2022 and Oct 18, 2022. Diffs are folded; open one to read it.

replies: 12people: 5as markdown or json

Glen Choo via GitGitGadget· Oct 12, 2022, 19:43 UTC · lore

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 derrickstolee@github.com, Taylor Blau me@ttaylorr.com,
Johannes Schindelin Johannes.Schindelin@gmx.de, Junio C Hamano
gitster@pobox.com

[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
-- 
gitgitgadget
Glen Choo via GitGitGadget· Oct 12, 2022, 19:43 UTC · re: Glen Choo via GitGitGadget · lore

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

From: Glen Choo <chooglen@google.com>

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 <chooglen@google.com>
---
 t/t0033-safe-directory.sh       | 9 +++++++++
 t/t0035-safe-bare-repository.sh | 9 +++++++++
 2 files changed, 18 insertions(+)
Show changes to 2 files +18 −0

t/t0033-safe-directory.sh, t/t0035-safe-bare-repository.sh

diff --git a/t/t0033-safe-directory.sh b/t/t0033-safe-directory.sh
index aecb308cf66..720d6cdd60b 100755
--- a/t/t0033-safe-directory.sh
+++ b/t/t0033-safe-directory.sh
@@ -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
diff --git a/t/t0035-safe-bare-repository.sh b/t/t0035-safe-bare-repository.sh
index ecbdc8238db..aa6a6a8c3fd 100755
--- a/t/t0035-safe-bare-repository.sh
+++ b/t/t0035-safe-bare-repository.sh
@@ -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
-- 
gitgitgadget
Ævar Arnfjörð Bjarmason· Oct 12, 2022, 22:28 UTC · re: Glen Choo via GitGitGadget · lore

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

On Wed, Oct 12 2022, Glen Choo via GitGitGadget wrote:
Show 23 quoted lines
> From: Glen Choo <chooglen@google.com>
>
> 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 <chooglen@google.com>
> ---
>  t/t0033-safe-directory.sh       | 9 +++++++++
>  t/t0035-safe-bare-repository.sh | 9 +++++++++
>  2 files changed, 18 insertions(+)
>
> diff --git a/t/t0033-safe-directory.sh b/t/t0033-safe-directory.sh
> index aecb308cf66..720d6cdd60b 100755
> --- a/t/t0033-safe-directory.sh
> +++ b/t/t0033-safe-directory.sh
> @@ -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)"
Here you use $, so <<-EOF, not <<-\EOF, Okey.
> +test_expect_failure 'safe.bareRepository in included file' '
> +	cat >gitconfig-include <<-EOF &&
> +	[safe]
> +		bareRepository = explicit
But this one should use <<-\EOF
Glen Choo via GitGitGadget· Oct 12, 2022, 19:43 UTC · re: Glen Choo via GitGitGadget · lore

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

From: Glen Choo <chooglen@google.com>

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 <chooglen@google.com>
---
 config.c                        | 30 ++++++++----------------------
 t/t0033-safe-directory.sh       |  2 +-
 t/t0035-safe-bare-repository.sh |  2 +-
 3 files changed, 10 insertions(+), 24 deletions(-)
Show changes to 3 files +10 −24

config.c, t/t0033-safe-directory.sh, t/t0035-safe-bare-repository.sh

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
-- 
gitgitgadget
Junio C Hamano· Oct 12, 2022, 20:48 UTC · re: Glen Choo via GitGitGadget · lore

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

"Glen Choo via GitGitGadget" <gitgitgadget@gmail.com> 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.

Show 26 quoted lines
> 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
Glen Choo· Oct 12, 2022, 22:09 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <gitster@pobox.com> writes:
Show 15 quoted lines
> "Glen Choo via GitGitGadget" <gitgitgadget@gmail.com> 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.

Junio C Hamano· Oct 12, 2022, 21:07 UTC · re: Glen Choo via GitGitGadget · lore

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

"Glen Choo via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 35 quoted lines
> -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, 19:47 UTC · re: Glen Choo via GitGitGadget · lore

Re: [PATCH 0/2] [2.38 regression] config: respect includes in protected config

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

"Glen Choo via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 27 quoted lines
> 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 derrickstolee@github.com, Taylor Blau me@ttaylorr.com,
> Johannes Schindelin Johannes.Schindelin@gmx.de, Junio C Hamano
> gitster@pobox.com
>
> [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
> -- 
> gitgitgadget
Junio C Hamano· Oct 12, 2022, 20:54 UTC · re: Glen Choo via GitGitGadget · lore

Re: [PATCH 0/2] [2.38 regression] config: respect includes in protected config

"Glen Choo via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 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 ;-)

Show 21 quoted lines
> Cc: Derrick Stolee derrickstolee@github.com, Taylor Blau me@ttaylorr.com,
> Johannes Schindelin Johannes.Schindelin@gmx.de, Junio C Hamano
> gitster@pobox.com
>
> [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· Oct 13, 2022, 17:43 UTC · re: Glen Choo via GitGitGadget · lore

[PATCH v2] config: respect includes in protected config

From: Glen Choo <chooglen@google.com>

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 <chooglen@google.com>
---
    [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 derrickstolee@github.com Cc: Taylor Blau
    me@ttaylorr.com Cc: Johannes Schindelin Johannes.Schindelin@gmx.de Cc:
    Junio C Hamano gitster@pobox.com Cc: Ævar Arnfjörð Bjarmason
    avarab@gmail.com
    
    [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(-)
Show changes to 3 files +26 −22

config.c, t/t0033-safe-directory.sh, t/t0035-safe-bare-repository.sh

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 aecb308cf66..dc3496897ab 100755
--- a/t/t0033-safe-directory.sh
+++ b/t/t0033-safe-directory.sh
@@ -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
diff --git a/t/t0035-safe-bare-repository.sh b/t/t0035-safe-bare-repository.sh
index ecbdc8238db..11c15a48aab 100755
--- a/t/t0035-safe-bare-repository.sh
+++ b/t/t0035-safe-bare-repository.sh
@@ -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
Glen Choo· Oct 13, 2022, 18:21 UTC · re: Glen Choo via GitGitGadget · lore

Re: [PATCH v2] config: respect includes in protected config

"Glen Choo via GitGitGadget" <gitgitgadget@gmail.com> writes:
>     Cc: Derrick Stolee derrickstolee@github.com Cc: Taylor Blau
>     me@ttaylorr.com Cc: Johannes Schindelin Johannes.Schindelin@gmx.de Cc:
>     Junio C Hamano gitster@pobox.com Cc: Ævar Arnfjörð Bjarmason
>     avarab@gmail.com

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

Jeff King· Oct 14, 2022, 20:14 UTC · re: Glen Choo via GitGitGadget · lore

Re: [PATCH v2] config: respect includes in protected config

On Thu, Oct 13, 2022 at 05:43:47PM +0000, Glen Choo via GitGitGadget wrote:
Show 7 quoted lines
> From: Glen Choo <chooglen@google.com>
> 
> 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· Oct 18, 2022, 14:36 UTC · re: Glen Choo via GitGitGadget · lore

Re: [PATCH v2] config: respect includes in protected config

Hi Glen,
On Thu, 13 Oct 2022, Glen Choo via GitGitGadget wrote:
Show 7 quoted lines
> From: Glen Choo <chooglen@google.com>
>
> 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

Show 168 quoted 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 <chooglen@google.com>
> ---
>     [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 derrickstolee@github.com Cc: Taylor Blau
>     me@ttaylorr.com Cc: Johannes Schindelin Johannes.Schindelin@gmx.de Cc:
>     Junio C Hamano gitster@pobox.com Cc: Ævar Arnfjörð Bjarmason
>     avarab@gmail.com
>
>     [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(-)
>
> 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 aecb308cf66..dc3496897ab 100755
> --- a/t/t0033-safe-directory.sh
> +++ b/t/t0033-safe-directory.sh
> @@ -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
> diff --git a/t/t0035-safe-bare-repository.sh b/t/t0035-safe-bare-repository.sh
> index ecbdc8238db..11c15a48aab 100755
> --- a/t/t0035-safe-bare-repository.sh
> +++ b/t/t0035-safe-bare-repository.sh
> @@ -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
>

← back to recent threads