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

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

[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 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

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

[Glen Choo via GitGitGadget]

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)"
+	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

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

[Ævar Arnfjörð Bjarmason]

On Wed, Oct 12 2022, Glen Choo via GitGitGadget wrote:

> 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

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

[Glen Choo via GitGitGadget]

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

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

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

[Junio C Hamano]

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

> 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

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

[Glen Choo]

Junio C Hamano <gitster@pobox.com> writes:

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

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

[Junio C Hamano]

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

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

[Glen Choo]

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

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

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

[Junio C Hamano]

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

> 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

[PATCH v2] config: respect includes in protected config

[Glen Choo via GitGitGadget]

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

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

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

[Glen Choo]

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

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

[Jeff King]

On Thu, Oct 13, 2022 at 05:43:47PM +0000, Glen Choo via GitGitGadget wrote:

> 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

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

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 6473 bytes --]

Hi Glen,

On Thu, 13 Oct 2022, Glen Choo via GitGitGadget wrote:

> 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

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