[4/7] config: add --literal-value option, un-implemented

Commit Message

Derrick Stolee Nov. 19, 2020, 3:52 p.m. UTC

From: Derrick Stolee <[e-mail]>

The 'git config' builtin takes a 'value_regex' parameter for several
actions. This can cause confusion when expecting exact value matches
instead of regex matches, especially when the input string contains glob
characters. While callers can escape the patterns themselves, it would
be more friendly to allow an argument to disable the pattern matching in
favor of an exact string match.

Add a new '--literal-value' option that does not currently change the
behavior. The implementation will follow for each appropriate action.
For now, check and test that --literal-value will abort the command when
included with an incompatible action.

The name '--literal-value' was chosen over something simpler like
'--literal' because some commands allow regular expressions on the
key in addition to the value.

Signed-off-by: Derrick Stolee <[e-mail]>
---
 Documentation/git-config.txt | 20 +++++++++++++-------
 builtin/config.c             | 17 +++++++++++++++++
 t/t1300-config.sh            | 13 +++++++++++++
 3 files changed, 43 insertions(+), 7 deletions(-)

Comments

Junio C Hamano Nov. 19, 2020, 10:42 p.m. UTC | #1

"Derrick Stolee via GitGitGadget" <[e-mail]> writes:

> +--literal-value::
> +	When used with the `value_regex` argument, treat `value_regex` as
> +	an exact string instead of a regular expression. This will restrict
> +	the name/value pairs that are matched to only those where the value
> +	is exactly equal to the `value_regex`.

Makes sense.

>  --type <type>::
>    'git config' will ensure that any input or output is valid under the given
>    type constraint(s), and will canonicalize outgoing values in `<type>`'s
> diff --git a/builtin/config.c b/builtin/config.c
> index e7c7f3d455..ad6c695737 100644
> --- a/builtin/config.c
> +++ b/builtin/config.c
> @@ -34,6 +34,7 @@ static int respect_includes_opt = -1;
>  static struct config_options config_options;
>  static int show_origin;
>  static int show_scope;
> +static int literal;
>  
>  #define ACTION_GET (1<<0)
>  #define ACTION_GET_ALL (1<<1)
> @@ -52,6 +53,16 @@ static int show_scope;
>  #define ACTION_GET_COLORBOOL (1<<14)
>  #define ACTION_GET_URLMATCH (1<<15)
>  
> +#define ACTION_LITERAL_ALLOWED (\
> +	ACTION_GET |\
> +	ACTION_GET_ALL |\
> +	ACTION_GET_REGEXP |\
> +	ACTION_REPLACE_ALL |\
> +	ACTION_UNSET |\
> +	ACTION_UNSET_ALL |\
> +	ACTION_SET_ALL\
> +)

I am not sure about this, though.

Even if an action can potentially take value_regex, e.g.

    git config --get name [value_regex]

should we allow litral when value_regex is not given?  IOW

    $ git config --get --literal-value vari.able v2.26.0+next

may make sense, but shouldn't

    $ git config --get --literal-value vari.able

be an error?

To put it differently, I think the macro being defined is not
ACTION_LITERAL_ALLOWED, but ACTION_VALUE_REGEX_ALLOWED. i.e. list of
actions that can potentially take value_regex.  A command line that
gives value_regex to an action that is not listed there is an error.

> +	if (literal && !(actions & ACTION_LITERAL_ALLOWED)) {
> +		error(_("--literal only applies with 'value_regex'"));
> +		usage_builtin_config();
> +	}

This check is not incorrect per-se, because an action that never
takes value_regex is by definition incompatible with the literal
value option.  But it does not flag it as an error to ask a
value_regex to be treated as a literal string when there is no
value_regex given.

Having said that, I think it is OK, at least for now, to leave such
an error undetected---it falls into the "if it hurts, don't do it"
category.

Thanks.

>  	if (actions & PAGING_ACTIONS)
>  		setup_auto_pager("config", 1);
>  
> diff --git a/t/t1300-config.sh b/t/t1300-config.sh
> index 74e0f84c0a..73f5ca4361 100755
> --- a/t/t1300-config.sh
> +++ b/t/t1300-config.sh
> @@ -1965,4 +1965,17 @@ test_expect_success '--replace-all and value_regex' '
>  	test_cmp expect .git/config
>  '
>  
> +test_expect_success 'refuse --literal-value for incompatible actions' '
> +	git config dev.null bogus &&
> +	test_must_fail git config --literal-value --add dev.null bogus &&
> +	test_must_fail git config --literal-value --get-urlmatch dev.null bogus &&
> +	test_must_fail git config --literal-value --get-urlmatch dev.null bogus &&
> +	test_must_fail git config --literal-value --rename-section dev null &&
> +	test_must_fail git config --literal-value --remove-section dev &&
> +	test_must_fail git config --literal-value --list &&
> +	test_must_fail git config --literal-value --get-color dev.null &&
> +	test_must_fail git config --literal-value --get-colorbool dev.null &&
> +	test_must_fail git config --literal-value --edit
> +'
> +
>  test_done

Martin Ågren Nov. 20, 2020, 6:35 a.m. UTC | #2

On Thu, 19 Nov 2020 at 16:55, Derrick Stolee via GitGitGadget
<[e-mail]> wrote:
> +       OPT_BOOL(0, "literal-value", &literal, N_("use literal equality when matching values")),

> +       if (literal && !(actions & ACTION_LITERAL_ALLOWED)) {
> +               error(_("--literal only applies with 'value_regex'"));

s/literal/&-value/

> +               usage_builtin_config();
> +       }


Martin

Patch

diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index 7573160f21..e244dd73f0 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -9,15 +9,15 @@  git-config - Get and set repository or global options
 SYNOPSIS
 --------
 [verse]
-'git config' [<file-option>] [--type=<type>] [--show-origin] [--show-scope] [-z|--null] name [value [value_regex]]
+'git config' [<file-option>] [--type=<type>] [--literal-value] [--show-origin] [--show-scope] [-z|--null] name [value [value_regex]]
 'git config' [<file-option>] [--type=<type>] --add name value
-'git config' [<file-option>] [--type=<type>] --replace-all name value [value_regex]
-'git config' [<file-option>] [--type=<type>] [--show-origin] [--show-scope] [-z|--null] --get name [value_regex]
-'git config' [<file-option>] [--type=<type>] [--show-origin] [--show-scope] [-z|--null] --get-all name [value_regex]
-'git config' [<file-option>] [--type=<type>] [--show-origin] [--show-scope] [-z|--null] [--name-only] --get-regexp name_regex [value_regex]
+'git config' [<file-option>] [--type=<type>] [--literal-value] --replace-all name value [value_regex]
+'git config' [<file-option>] [--type=<type>] [--show-origin] [--show-scope] [-z|--null] [--literal-value] --get name [value_regex]
+'git config' [<file-option>] [--type=<type>] [--show-origin] [--show-scope] [-z|--null] [--literal-value] --get-all name [value_regex]
+'git config' [<file-option>] [--type=<type>] [--show-origin] [--show-scope] [-z|--null] [--literal-value] [--name-only] --get-regexp name_regex [value_regex]
 'git config' [<file-option>] [--type=<type>] [-z|--null] --get-urlmatch name URL
-'git config' [<file-option>] --unset name [value_regex]
-'git config' [<file-option>] --unset-all name [value_regex]
+'git config' [<file-option>] [--literal-value] --unset name [value_regex]
+'git config' [<file-option>] [--literal-value] --unset-all name [value_regex]
 'git config' [<file-option>] --rename-section old_name new_name
 'git config' [<file-option>] --remove-section name
 'git config' [<file-option>] [--show-origin] [--show-scope] [-z|--null] [--name-only] -l | --list
@@ -165,6 +165,12 @@  See also <<FILES>>.
 --list::
 	List all variables set in config file, along with their values.
 
+--literal-value::
+	When used with the `value_regex` argument, treat `value_regex` as
+	an exact string instead of a regular expression. This will restrict
+	the name/value pairs that are matched to only those where the value
+	is exactly equal to the `value_regex`.
+
 --type <type>::
   'git config' will ensure that any input or output is valid under the given
   type constraint(s), and will canonicalize outgoing values in `<type>`'s
diff --git a/builtin/config.c b/builtin/config.c
index e7c7f3d455..ad6c695737 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -34,6 +34,7 @@  static int respect_includes_opt = -1;
 static struct config_options config_options;
 static int show_origin;
 static int show_scope;
+static int literal;
 
 #define ACTION_GET (1<<0)
 #define ACTION_GET_ALL (1<<1)
@@ -52,6 +53,16 @@  static int show_scope;
 #define ACTION_GET_COLORBOOL (1<<14)
 #define ACTION_GET_URLMATCH (1<<15)
 
+#define ACTION_LITERAL_ALLOWED (\
+	ACTION_GET |\
+	ACTION_GET_ALL |\
+	ACTION_GET_REGEXP |\
+	ACTION_REPLACE_ALL |\
+	ACTION_UNSET |\
+	ACTION_UNSET_ALL |\
+	ACTION_SET_ALL\
+)
+
 /*
  * The actions "ACTION_LIST | ACTION_GET_*" which may produce more than
  * one line of output and which should therefore be paged.
@@ -141,6 +152,7 @@  static struct option builtin_config_options[] = {
 	OPT_BIT(0, "rename-section", &actions, N_("rename section: old-name new-name"), ACTION_RENAME_SECTION),
 	OPT_BIT(0, "remove-section", &actions, N_("remove a section: name"), ACTION_REMOVE_SECTION),
 	OPT_BIT('l', "list", &actions, N_("list all"), ACTION_LIST),
+	OPT_BOOL(0, "literal-value", &literal, N_("use literal equality when matching values")),
 	OPT_BIT('e', "edit", &actions, N_("open an editor"), ACTION_EDIT),
 	OPT_BIT(0, "get-color", &actions, N_("find the color configured: slot [default]"), ACTION_GET_COLOR),
 	OPT_BIT(0, "get-colorbool", &actions, N_("find the color setting: slot [stdout-is-tty]"), ACTION_GET_COLORBOOL),
@@ -745,6 +757,11 @@  int cmd_config(int argc, const char **argv, const char *prefix)
 		usage_builtin_config();
 	}
 
+	if (literal && !(actions & ACTION_LITERAL_ALLOWED)) {
+		error(_("--literal only applies with 'value_regex'"));
+		usage_builtin_config();
+	}
+
 	if (actions & PAGING_ACTIONS)
 		setup_auto_pager("config", 1);
 
diff --git a/t/t1300-config.sh b/t/t1300-config.sh
index 74e0f84c0a..73f5ca4361 100755
--- a/t/t1300-config.sh
+++ b/t/t1300-config.sh
@@ -1965,4 +1965,17 @@  test_expect_success '--replace-all and value_regex' '
 	test_cmp expect .git/config
 '
 
+test_expect_success 'refuse --literal-value for incompatible actions' '
+	git config dev.null bogus &&
+	test_must_fail git config --literal-value --add dev.null bogus &&
+	test_must_fail git config --literal-value --get-urlmatch dev.null bogus &&
+	test_must_fail git config --literal-value --get-urlmatch dev.null bogus &&
+	test_must_fail git config --literal-value --rename-section dev null &&
+	test_must_fail git config --literal-value --remove-section dev &&
+	test_must_fail git config --literal-value --list &&
+	test_must_fail git config --literal-value --get-color dev.null &&
+	test_must_fail git config --literal-value --get-colorbool dev.null &&
+	test_must_fail git config --literal-value --edit
+'
+
 test_done