[v3,4/5] config: include "-c" in protected config

Commit Message

Glen Choo May 27, 2022, 9:09 p.m. UTC

From: Glen Choo <[e-mail]>

Protected config should include the command line (aka "-c") because we
can be quite certain that this config is specified by the user.

Introduce a function, `git_configset_add_parameters()`, that adds "-c"
config to a config_set, and use it to add "-c" to protected config.

Signed-off-by: Glen Choo <[e-mail]>
---
 Documentation/config.txt           |  6 +++---
 Documentation/glossary-content.txt |  2 +-
 config.c                           |  6 ++++++
 config.h                           |  9 +++++++++
 t/t0033-safe-directory.sh          | 24 ++++++++++--------------
 t/t0035-discovery-bare.sh          |  3 +--
 6 files changed, 30 insertions(+), 20 deletions(-)

Comments

Derrick Stolee June 2, 2022, 1:15 p.m. UTC | #1

On 5/27/2022 5:09 PM, Glen Choo via GitGitGadget wrote:
  
> +int git_configset_add_parameters(struct config_set *cs)
> +{
> +	return git_config_from_parameters(config_set_callback, cs);
> +}
> +

This one-line method could be inlined into the read_protected_config()
method:

> @@ -2628,6 +2633,7 @@ static void read_protected_config(void)
>  	git_configset_add_file(the_repository->protected_config, system_config);
>  	git_configset_add_file(the_repository->protected_config, xdg_config);
>  	git_configset_add_file(the_repository->protected_config, user_config);
> +	git_configset_add_parameters(the_repository->protected_config);
	git_config_from_parameters(config_set_callback, the_repository->protected_config);

...would be the way to inline it.

> +/**
> + * Parses command line options and environment variables, and adds the
> + * variable-value pairs to the `config_set`. Returns 0 on success, or -1
> + * if there is an error in parsing. The caller decides whether to free
> + * the incomplete configset or continue using it when the function
> + * returns -1.
> + */
> +int git_configset_add_parameters(struct config_set *cs);

You do make it public here. I wonder if we can think of other consumers
of this method that justify the addition to the API.

But this is also a nitpick. I don't feel strongly one way or another. The
code definitely works as-is.

Thanks,
-Stolee

Patch

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 34133288d75..f40a3e297ce 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -370,9 +370,9 @@  names do not conflict with those that are used by Git itself and
 other popular tools, and describe them in your documentation.
 
 Variables marked with '(Protected config only)' are only respected when
-they are specified in protected configuration. This includes global and
-system-level config, and excludes repository config, the command line
-option `-c`, and environment variables. For more details, see the
+they are specified in protected configuration. This includes global,
+system-level config, the command line option `-c`, and environment
+variables, and excludes repository config. For more details, see the
 'protected configuration' entry in linkgit:gitglossary[7].
 
 include::config/advice.txt[]
diff --git a/Documentation/glossary-content.txt b/Documentation/glossary-content.txt
index a669983abd6..4190c410a00 100644
--- a/Documentation/glossary-content.txt
+++ b/Documentation/glossary-content.txt
@@ -494,12 +494,12 @@  Protected configuration includes:
 - system-level config, e.g. `/etc/git/config`
 - global config, e.g. `$XDG_CONFIG_HOME/git/config` and
   `$HOME/.gitconfig`
+- the command line option `-c` and its equivalent environment variables
 +
 Protected configuration excludes:
 +
 - repository config, e.g. `$GIT_DIR/config` and
   `$GIT_DIR/config.worktree`
-- the command line option `-c` and its equivalent environment variables
 
 [[def_reachable]]reachable::
 	All of the ancestors of a given <<def_commit,commit>> are said to be
diff --git a/config.c b/config.c
index c30bb7c5d09..22192ca1d63 100644
--- a/config.c
+++ b/config.c
@@ -2373,6 +2373,11 @@  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;
@@ -2628,6 +2633,7 @@  static void read_protected_config(void)
 	git_configset_add_file(the_repository->protected_config, system_config);
 	git_configset_add_file(the_repository->protected_config, xdg_config);
 	git_configset_add_file(the_repository->protected_config, user_config);
+	git_configset_add_parameters(the_repository->protected_config);
 
 	free(system_config);
 	free(xdg_config);
diff --git a/config.h b/config.h
index 411965f52b5..e3ff1fcf683 100644
--- a/config.h
+++ b/config.h
@@ -446,6 +446,15 @@  void git_configset_init(struct config_set *cs);
  */
 int git_configset_add_file(struct config_set *cs, const char *filename);
 
+/**
+ * Parses command line options and environment variables, and adds the
+ * variable-value pairs to the `config_set`. Returns 0 on success, or -1
+ * if there is an error in parsing. The caller decides whether to free
+ * the incomplete configset or continue using it when the function
+ * returns -1.
+ */
+int git_configset_add_parameters(struct config_set *cs);
+
 /**
  * Finds and returns the value list, sorted in order of increasing priority
  * for the configuration variable `key` and config set `cs`. When the
diff --git a/t/t0033-safe-directory.sh b/t/t0033-safe-directory.sh
index 238b25f91a3..5a1cd0d0947 100755
--- a/t/t0033-safe-directory.sh
+++ b/t/t0033-safe-directory.sh
@@ -16,24 +16,20 @@  test_expect_success 'safe.directory is not set' '
 	expect_rejected_dir
 '
 
-test_expect_success 'ignoring safe.directory on the command line' '
-	test_must_fail git -c safe.directory="$(pwd)" status 2>err &&
-	grep "unsafe repository" err
+test_expect_success 'safe.directory on the command line' '
+	git -c safe.directory="$(pwd)" status
 '
 
-test_expect_success 'ignoring safe.directory in the environment' '
-	test_must_fail env GIT_CONFIG_COUNT=1 \
-		GIT_CONFIG_KEY_0="safe.directory" \
-		GIT_CONFIG_VALUE_0="$(pwd)" \
-		git status 2>err &&
-	grep "unsafe repository" err
+test_expect_success 'safe.directory in the environment' '
+	env GIT_CONFIG_COUNT=1 \
+	    GIT_CONFIG_KEY_0="safe.directory" \
+	    GIT_CONFIG_VALUE_0="$(pwd)" \
+	    git status
 '
 
-test_expect_success 'ignoring safe.directory in GIT_CONFIG_PARAMETERS' '
-	test_must_fail env \
-		GIT_CONFIG_PARAMETERS="${SQ}safe.directory${SQ}=${SQ}$(pwd)${SQ}" \
-		git status 2>err &&
-	grep "unsafe repository" err
+test_expect_success 'safe.directory in GIT_CONFIG_PARAMETERS' '
+	env GIT_CONFIG_PARAMETERS="${SQ}safe.directory${SQ}=${SQ}$(pwd)${SQ}" \
+	    git status
 '
 
 test_expect_success 'ignoring safe.directory in repo config' '
diff --git a/t/t0035-discovery-bare.sh b/t/t0035-discovery-bare.sh
index 94c2f76d774..0d5983df307 100755
--- a/t/t0035-discovery-bare.sh
+++ b/t/t0035-discovery-bare.sh
@@ -56,8 +56,7 @@  test_expect_success 'discovery.bare on the command line' '
 	git config --global discovery.bare never &&
 	(
 		cd outer-repo/bare-repo &&
-		test_must_fail git -c discovery.bare=always rev-parse --git-dir 2>err &&
-		grep "discovery.bare" err
+		git -c discovery.bare=always rev-parse --git-dir
 	)
 '