[PATCH 1/2] config: add options to list only variable names

[PATCH 1/2] config: add options to list only variable names

[SZEDER Gábor]

Recenty I created a multi-line branch description with '.' and '='
characters on one of the lines, and noticed that fragments of that line
show up when completing set variable names for 'git config', e.g.:

  $ git config --get branch.b.description
  Branch description to fool the completion script with a
  second line containing dot . and equals = characters.
  $ git config --unset <TAB>
  ...
  second line containing dot . and equals
  ...

The completion script runs 'git config --list' and processes its output to
strip the values and keep only the variable names.  It does so by looking
for lines containing '.' and '=' and outputting everything before the '=',
which was fooled by my multi-line branch description.

A similar issue exists with aliases and pretty format aliases with
multi-line values, but in that case 'git config --get-regexp' is run and
subsequent lines don't have to contain either '.' or '=' to fool the
completion script.

Though 'git config' can produce null-terminated output for newline-safe
parsing, that's of no use in this case, becase we can't cope with nulls in
the shell.

Help the completion script by introducing the '--list-names' and
'--get-names-regexp' options, the "names-only" equivalents of '--list' and
'--get-regexp', so it doesn't have to separate variable names from their
values anymore.

Signed-off-by: SZEDER Gábor <szeder@ira.uka.de>
---
 Documentation/git-config.txt           | 10 +++++++++-
 builtin/config.c                       | 22 ++++++++++++++++++----
 contrib/completion/git-completion.bash |  4 ++--
 t/t1300-repo-config.sh                 | 22 ++++++++++++++++++++++
 4 files changed, 51 insertions(+), 7 deletions(-)

diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index 02ec096..e0d27d5 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -16,11 +16,12 @@ SYNOPSIS
 'git config' [<file-option>] [type] [-z|--null] --get-all name [value_regex]
 'git config' [<file-option>] [type] [-z|--null] --get-regexp name_regex [value_regex]
 'git config' [<file-option>] [type] [-z|--null] --get-urlmatch name URL
+'git config' [<file-option>] [-z|--null] --get-name-regexp name_regex
 'git config' [<file-option>] --unset name [value_regex]
 'git config' [<file-option>] --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>] [-z|--null] -l | --list
+'git config' [<file-option>] [-z|--null] -l | --list | --list-name
 'git config' [<file-option>] --get-color name [default]
 'git config' [<file-option>] --get-colorbool name [stdout-is-tty]
 'git config' [<file-option>] -e | --edit
@@ -96,6 +97,10 @@ OPTIONS
 	in which section and variable names are lowercased, but subsection
 	names are not.
 
+--get-name-regexp::
+	Like --get-regexp, but shows only matching variable names, not its
+	values.
+
 --get-urlmatch name URL::
 	When given a two-part name section.key, the value for
 	section.<url>.key whose <url> part matches the best to the
@@ -161,6 +166,9 @@ See also <<FILES>>.
 --list::
 	List all variables set in config file.
 
+--list-name::
+	List the names of all variables set in config file.
+
 --bool::
 	'git config' will ensure that the output is "true" or "false"
 
diff --git a/builtin/config.c b/builtin/config.c
index 7188405..38bcf83 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -13,6 +13,7 @@ static char *key;
 static regex_t *key_regexp;
 static regex_t *regexp;
 static int show_keys;
+static int show_only_keys;
 static int use_key_regexp;
 static int do_all;
 static int do_not_match;
@@ -43,6 +44,8 @@ static int respect_includes = -1;
 #define ACTION_GET_COLOR (1<<13)
 #define ACTION_GET_COLORBOOL (1<<14)
 #define ACTION_GET_URLMATCH (1<<15)
+#define ACTION_LIST_NAMES (1<<16)
+#define ACTION_GET_NAME_REGEXP (1<<17)
 
 #define TYPE_BOOL (1<<0)
 #define TYPE_INT (1<<1)
@@ -60,6 +63,7 @@ static struct option builtin_config_options[] = {
 	OPT_BIT(0, "get", &actions, N_("get value: name [value-regex]"), ACTION_GET),
 	OPT_BIT(0, "get-all", &actions, N_("get all values: key [value-regex]"), ACTION_GET_ALL),
 	OPT_BIT(0, "get-regexp", &actions, N_("get values for regexp: name-regex [value-regex]"), ACTION_GET_REGEXP),
+	OPT_BIT(0, "get-name-regexp", &actions, N_("get names for regexp: name-regex"), ACTION_GET_NAME_REGEXP),
 	OPT_BIT(0, "get-urlmatch", &actions, N_("get value specific for the URL: section[.var] URL"), ACTION_GET_URLMATCH),
 	OPT_BIT(0, "replace-all", &actions, N_("replace all matching variables: name value [value_regex]"), ACTION_REPLACE_ALL),
 	OPT_BIT(0, "add", &actions, N_("add a new variable: name value"), ACTION_ADD),
@@ -68,6 +72,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_BIT(0, "list-names", &actions, N_("list all variable names"), ACTION_LIST_NAMES),
 	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),
@@ -91,7 +96,7 @@ static void check_argc(int argc, int min, int max) {
 
 static int show_all_config(const char *key_, const char *value_, void *cb)
 {
-	if (value_)
+	if (!show_only_keys && value_)
 		printf("%s%c%s%c", key_, delim, value_, term);
 	else
 		printf("%s%c", key_, term);
@@ -165,7 +170,14 @@ static int collect_config(const char *key_, const char *value_, void *cb)
 
 	ALLOC_GROW(values->items, values->nr + 1, values->alloc);
 
-	return format_config(&values->items[values->nr++], key_, value_);
+	if (show_only_keys) {
+		struct strbuf *buf = &values->items[values->nr++];
+		strbuf_init(buf, 0);
+		strbuf_addstr(buf, key_);
+		strbuf_addch(buf, term);
+		return 0;
+	} else
+		return format_config(&values->items[values->nr++], key_, value_);
 }
 
 static int get_value(const char *key_, const char *regex_)
@@ -550,7 +562,8 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 			usage_with_options(builtin_config_usage, builtin_config_options);
 		}
 
-	if (actions == ACTION_LIST) {
+	if (actions == ACTION_LIST || actions == ACTION_LIST_NAMES) {
+		show_only_keys = (actions == ACTION_LIST_NAMES);
 		check_argc(argc, 0, 0);
 		if (git_config_with_options(show_all_config, NULL,
 					    &given_config_source,
@@ -631,8 +644,9 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 		check_argc(argc, 1, 2);
 		return get_value(argv[0], argv[1]);
 	}
-	else if (actions == ACTION_GET_REGEXP) {
+	else if (actions == ACTION_GET_REGEXP || actions == ACTION_GET_NAME_REGEXP) {
 		show_keys = 1;
+		show_only_keys = (actions == ACTION_GET_NAME_REGEXP);
 		use_key_regexp = 1;
 		do_all = 1;
 		check_argc(argc, 1, 2);
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index bfc74e9..6abbd56 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1883,8 +1883,8 @@ _git_config ()
 	--*)
 		__gitcomp "
 			--system --global --local --file=
-			--list --replace-all
-			--get --get-all --get-regexp
+			--list --list-names --replace-all
+			--get --get-all --get-regexp --get-name-regexp
 			--add --unset --unset-all
 			--remove-section --rename-section
 			"
diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index 66dd286..fa83944 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -353,6 +353,18 @@ test_expect_success '--list without repo produces empty output' '
 '
 
 cat > expect << EOF
+beta.noindent
+nextsection.nonewline
+123456.a123
+version.1.2.3eX.alpha
+EOF
+
+test_expect_success 'working --list-names' '
+	git config --list-names > output &&
+	test_cmp expect output
+'
+
+cat > expect << EOF
 beta.noindent sillyValue
 nextsection.nonewline wow2 for me
 EOF
@@ -363,6 +375,16 @@ test_expect_success '--get-regexp' '
 '
 
 cat > expect << EOF
+beta.noindent
+nextsection.nonewline
+EOF
+
+test_expect_success '--get-name-regexp' '
+	git config --get-name-regexp in >output &&
+	test_cmp expect output
+'
+
+cat > expect << EOF
 wow2 for me
 wow4 for you
 EOF
-- 
2.4.2.347.ge926c0d

[PATCH 2/2] completion: use new 'git config' options to reliably list variable names

[SZEDER Gábor]

List all set config variable names with 'git config --list-names' instead
of '--list' post processing.  Similarly, use 'git config
--get-name-regexp' instead of '--get-regexp' to get config variables in a
given section.

Signed-off-by: SZEDER Gábor <szeder@ira.uka.de>
---
 contrib/completion/git-completion.bash | 15 +++------------
 1 file changed, 3 insertions(+), 12 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 6abbd56..121aa31 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -744,9 +744,8 @@ __git_compute_porcelain_commands ()
 __git_get_config_variables ()
 {
 	local section="$1" i IFS=$'\n'
-	for i in $(git --git-dir="$(__gitdir)" config --get-regexp "^$section\..*" 2>/dev/null); do
-		i="${i#$section.}"
-		echo "${i/ */}"
+	for i in $(git --git-dir="$(__gitdir)" config --get-name-regexp "^$section\..*" 2>/dev/null); do
+		echo "${i#$section.}"
 	done
 }
 
@@ -1774,15 +1773,7 @@ __git_config_get_set_variables ()
 		c=$((--c))
 	done
 
-	git --git-dir="$(__gitdir)" config $config_file --list 2>/dev/null |
-	while read -r line
-	do
-		case "$line" in
-		*.*=*)
-			echo "${line/=*/}"
-			;;
-		esac
-	done
+	git --git-dir="$(__gitdir)" config $config_file --list-names 2>/dev/null
 }
 
 _git_config ()
-- 
2.4.2.347.ge926c0d

[PATCH 1.5/2] config: add options to list only variable names

[SZEDER Gábor]

Recenty I created a multi-line branch description with '.' and '='
characters on one of the lines, and noticed that fragments of that line
show up when completing set variable names for 'git config', e.g.:

  $ git config --get branch.b.description
  Branch description to fool the completion script with a
  second line containing dot . and equals = characters.
  $ git config --unset <TAB>
  ...
  second line containing dot . and equals
  ...

The completion script runs 'git config --list' and processes its output to
strip the values and keep only the variable names.  It does so by looking
for lines containing '.' and '=' and outputting everything before the '=',
which was fooled by my multi-line branch description.

A similar issue exists with aliases and pretty format aliases with
multi-line values, but in that case 'git config --get-regexp' is run and
subsequent lines don't have to contain either '.' or '=' to fool the
completion script.

Though 'git config' can produce null-terminated output for newline-safe
parsing, that's of no use in this case, becase we can't cope with nulls in
the shell.

Help the completion script by introducing the '--list-names' and
'--get-names-regexp' options, the "names-only" equivalents of '--list' and
'--get-regexp', so it doesn't have to separate variable names from their
values anymore.

Signed-off-by: SZEDER Gábor <szeder@ira.uka.de>
---
D'oh, misspelled the option in the docs...

 Documentation/git-config.txt           | 10 +++++++++-
 builtin/config.c                       | 22 ++++++++++++++++++----
 contrib/completion/git-completion.bash |  4 ++--
 t/t1300-repo-config.sh                 | 22 ++++++++++++++++++++++
 4 files changed, 51 insertions(+), 7 deletions(-)

diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index 02ec096faa..fd519f81d8 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -16,11 +16,12 @@ SYNOPSIS
 'git config' [<file-option>] [type] [-z|--null] --get-all name [value_regex]
 'git config' [<file-option>] [type] [-z|--null] --get-regexp name_regex [value_regex]
 'git config' [<file-option>] [type] [-z|--null] --get-urlmatch name URL
+'git config' [<file-option>] [-z|--null] --get-name-regexp name_regex
 'git config' [<file-option>] --unset name [value_regex]
 'git config' [<file-option>] --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>] [-z|--null] -l | --list
+'git config' [<file-option>] [-z|--null] -l | --list | --list-names
 'git config' [<file-option>] --get-color name [default]
 'git config' [<file-option>] --get-colorbool name [stdout-is-tty]
 'git config' [<file-option>] -e | --edit
@@ -96,6 +97,10 @@ OPTIONS
 	in which section and variable names are lowercased, but subsection
 	names are not.
 
+--get-name-regexp::
+	Like --get-regexp, but shows only matching variable names, not its
+	values.
+
 --get-urlmatch name URL::
 	When given a two-part name section.key, the value for
 	section.<url>.key whose <url> part matches the best to the
@@ -161,6 +166,9 @@ See also <<FILES>>.
 --list::
 	List all variables set in config file.
 
+--list-names::
+	List the names of all variables set in config file.
+
 --bool::
 	'git config' will ensure that the output is "true" or "false"
 
diff --git a/builtin/config.c b/builtin/config.c
index 7188405f7e..38bcf838c5 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -13,6 +13,7 @@ static char *key;
 static regex_t *key_regexp;
 static regex_t *regexp;
 static int show_keys;
+static int show_only_keys;
 static int use_key_regexp;
 static int do_all;
 static int do_not_match;
@@ -43,6 +44,8 @@ static int respect_includes = -1;
 #define ACTION_GET_COLOR (1<<13)
 #define ACTION_GET_COLORBOOL (1<<14)
 #define ACTION_GET_URLMATCH (1<<15)
+#define ACTION_LIST_NAMES (1<<16)
+#define ACTION_GET_NAME_REGEXP (1<<17)
 
 #define TYPE_BOOL (1<<0)
 #define TYPE_INT (1<<1)
@@ -60,6 +63,7 @@ static struct option builtin_config_options[] = {
 	OPT_BIT(0, "get", &actions, N_("get value: name [value-regex]"), ACTION_GET),
 	OPT_BIT(0, "get-all", &actions, N_("get all values: key [value-regex]"), ACTION_GET_ALL),
 	OPT_BIT(0, "get-regexp", &actions, N_("get values for regexp: name-regex [value-regex]"), ACTION_GET_REGEXP),
+	OPT_BIT(0, "get-name-regexp", &actions, N_("get names for regexp: name-regex"), ACTION_GET_NAME_REGEXP),
 	OPT_BIT(0, "get-urlmatch", &actions, N_("get value specific for the URL: section[.var] URL"), ACTION_GET_URLMATCH),
 	OPT_BIT(0, "replace-all", &actions, N_("replace all matching variables: name value [value_regex]"), ACTION_REPLACE_ALL),
 	OPT_BIT(0, "add", &actions, N_("add a new variable: name value"), ACTION_ADD),
@@ -68,6 +72,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_BIT(0, "list-names", &actions, N_("list all variable names"), ACTION_LIST_NAMES),
 	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),
@@ -91,7 +96,7 @@ static void check_argc(int argc, int min, int max) {
 
 static int show_all_config(const char *key_, const char *value_, void *cb)
 {
-	if (value_)
+	if (!show_only_keys && value_)
 		printf("%s%c%s%c", key_, delim, value_, term);
 	else
 		printf("%s%c", key_, term);
@@ -165,7 +170,14 @@ static int collect_config(const char *key_, const char *value_, void *cb)
 
 	ALLOC_GROW(values->items, values->nr + 1, values->alloc);
 
-	return format_config(&values->items[values->nr++], key_, value_);
+	if (show_only_keys) {
+		struct strbuf *buf = &values->items[values->nr++];
+		strbuf_init(buf, 0);
+		strbuf_addstr(buf, key_);
+		strbuf_addch(buf, term);
+		return 0;
+	} else
+		return format_config(&values->items[values->nr++], key_, value_);
 }
 
 static int get_value(const char *key_, const char *regex_)
@@ -550,7 +562,8 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 			usage_with_options(builtin_config_usage, builtin_config_options);
 		}
 
-	if (actions == ACTION_LIST) {
+	if (actions == ACTION_LIST || actions == ACTION_LIST_NAMES) {
+		show_only_keys = (actions == ACTION_LIST_NAMES);
 		check_argc(argc, 0, 0);
 		if (git_config_with_options(show_all_config, NULL,
 					    &given_config_source,
@@ -631,8 +644,9 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 		check_argc(argc, 1, 2);
 		return get_value(argv[0], argv[1]);
 	}
-	else if (actions == ACTION_GET_REGEXP) {
+	else if (actions == ACTION_GET_REGEXP || actions == ACTION_GET_NAME_REGEXP) {
 		show_keys = 1;
+		show_only_keys = (actions == ACTION_GET_NAME_REGEXP);
 		use_key_regexp = 1;
 		do_all = 1;
 		check_argc(argc, 1, 2);
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index bfc74e9d57..6abbd564b6 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1883,8 +1883,8 @@ _git_config ()
 	--*)
 		__gitcomp "
 			--system --global --local --file=
-			--list --replace-all
-			--get --get-all --get-regexp
+			--list --list-names --replace-all
+			--get --get-all --get-regexp --get-name-regexp
 			--add --unset --unset-all
 			--remove-section --rename-section
 			"
diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index 66dd28644f..fa83944ad9 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -353,6 +353,18 @@ test_expect_success '--list without repo produces empty output' '
 '
 
 cat > expect << EOF
+beta.noindent
+nextsection.nonewline
+123456.a123
+version.1.2.3eX.alpha
+EOF
+
+test_expect_success 'working --list-names' '
+	git config --list-names > output &&
+	test_cmp expect output
+'
+
+cat > expect << EOF
 beta.noindent sillyValue
 nextsection.nonewline wow2 for me
 EOF
@@ -363,6 +375,16 @@ test_expect_success '--get-regexp' '
 '
 
 cat > expect << EOF
+beta.noindent
+nextsection.nonewline
+EOF
+
+test_expect_success '--get-name-regexp' '
+	git config --get-name-regexp in >output &&
+	test_cmp expect output
+'
+
+cat > expect << EOF
 wow2 for me
 wow4 for you
 EOF
-- 
2.4.2.347.ge926c0d

Re: [PATCH 1/2] config: add options to list only variable names

[Jeff King]

On Wed, May 27, 2015 at 10:07:19PM +0200, SZEDER Gábor wrote:

> Help the completion script by introducing the '--list-names' and
> '--get-names-regexp' options, the "names-only" equivalents of '--list' and
> '--get-regexp', so it doesn't have to separate variable names from their
> values anymore.

Thanks, this sounds like the best solution. It should be a tiny bit more
efficient, too, though I doubt it matters much in practice.

> -'git config' [<file-option>] [-z|--null] -l | --list
> +'git config' [<file-option>] [-z|--null] -l | --list | --list-name

s/list-name/&s/, to match the code (and your commit message).

> @@ -161,6 +166,9 @@ See also <<FILES>>.
>  --list::
>  	List all variables set in config file.
>  
> +--list-name::
> +	List the names of all variables set in config file.

Ditto here. Also, now that we have two similar modes, perhaps the
"--list" description above should become:

  List all variables set in config file, along with their values.

> @@ -165,7 +170,14 @@ static int collect_config(const char *key_, const char *value_, void *cb)
>  
>  	ALLOC_GROW(values->items, values->nr + 1, values->alloc);
>  
> -	return format_config(&values->items[values->nr++], key_, value_);
> +	if (show_only_keys) {
> +		struct strbuf *buf = &values->items[values->nr++];
> +		strbuf_init(buf, 0);
> +		strbuf_addstr(buf, key_);
> +		strbuf_addch(buf, term);
> +		return 0;
> +	} else
> +		return format_config(&values->items[values->nr++], key_, value_);
>  }

Might it flow a little better to always enter format_config, and then
just return early (before writing the value) when show_key_only is set?

>  cat > expect << EOF
> +beta.noindent
> +nextsection.nonewline
> +123456.a123
> +version.1.2.3eX.alpha
> +EOF
> +
> +test_expect_success 'working --list-names' '
> +	git config --list-names > output &&
> +	test_cmp expect output
> +'
> +
> +cat > expect << EOF

We usually avoid the extra space after redirection operators. But we
also usually match existing code. I'm not sure which is more evil in
this case. ;)

> +test_expect_success '--get-name-regexp' '
> +	git config --get-name-regexp in >output &&
> +	test_cmp expect output
> +'

This one is the odd man out if you are following existing style,
though.

The rest of the patch looks good to me.

-Peff

Re: [PATCH 2/2] completion: use new 'git config' options to reliably list variable names

[Jeff King]

On Wed, May 27, 2015 at 10:07:20PM +0200, SZEDER Gábor wrote:

> List all set config variable names with 'git config --list-names' instead
> of '--list' post processing.  Similarly, use 'git config
> --get-name-regexp' instead of '--get-regexp' to get config variables in a
> given section.
> 
> Signed-off-by: SZEDER Gábor <szeder@ira.uka.de>
> ---
>  contrib/completion/git-completion.bash | 15 +++------------
>  1 file changed, 3 insertions(+), 12 deletions(-)

Nice diffstat. The less hacky bash parsing we can do, the better.

-Peff

Re: [PATCH 1/2] config: add options to list only variable names

[Jeff King]

On Wed, May 27, 2015 at 05:04:38PM -0400, Jeff King wrote:

> > -'git config' [<file-option>] [-z|--null] -l | --list
> > +'git config' [<file-option>] [-z|--null] -l | --list | --list-name
> 
> s/list-name/&s/, to match the code (and your commit message).

Doh, just saw your "1.5".

FWIW, I expected "PATCH 1.5/2" to be "eh, I should have put this in
between patches 1 and 2". I expect a re-roll to be "v1.5" (or just
"v2").

This was the only real error in the patch, so your 1.5 looks OK to me.
Though I hope you will consider my other suggestions, too.

-Peff

Re: [PATCH 1/2] config: add options to list only variable names

[SZEDER Gábor]

Quoting Jeff King <peff@peff.net>:

> On Wed, May 27, 2015 at 10:07:19PM +0200, SZEDER Gábor wrote:
>
>> Help the completion script by introducing the '--list-names' and
>> '--get-names-regexp' options, the "names-only" equivalents of '--list' and
>> '--get-regexp', so it doesn't have to separate variable names from their
>> values anymore.
>
> Thanks, this sounds like the best solution. It should be a tiny bit more
> efficient, too, though I doubt it matters much in practice.
>
>> -'git config' [<file-option>] [-z|--null] -l | --list
>> +'git config' [<file-option>] [-z|--null] -l | --list | --list-name
>
> s/list-name/&s/, to match the code (and your commit message).

And note how I added an extra 's' to the other option in the commit message!

>>  cat > expect << EOF
>> +beta.noindent
>> +nextsection.nonewline
>> +123456.a123
>> +version.1.2.3eX.alpha
>> +EOF
>> +
>> +test_expect_success 'working --list-names' '
>> +	git config --list-names > output &&
>> +	test_cmp expect output
>> +'
>> +
>> +cat > expect << EOF
>
> We usually avoid the extra space after redirection operators. But we
> also usually match existing code. I'm not sure which is more evil in
> this case. ;)
>
>> +test_expect_success '--get-name-regexp' '
>> +	git config --get-name-regexp in >output &&
>> +	test_cmp expect output
>> +'
>
> This one is the odd man out if you are following existing style,
> though.

Heh, in both cases I simply copied the existing "name-less" test, and  
only adjusted the expected output and the name of the option to test. :)

Will reroll.

Gábor

Re: [PATCH 1/2] config: add options to list only variable names

[Junio C Hamano]

SZEDER Gábor <szeder@ira.uka.de> writes:

> diff --git a/builtin/config.c b/builtin/config.c
> index 7188405..38bcf83 100644
> --- a/builtin/config.c
> +++ b/builtin/config.c
> @@ -13,6 +13,7 @@ static char *key;
>  static regex_t *key_regexp;
>  static regex_t *regexp;
>  static int show_keys;
> +static int show_only_keys;

Is it just me who thinks this is a strange phrase?  Somehow these
words do not roll well on my tongue.

Perhaps "static int omit_values"?  Which would match what this part
does pretty well:

>  static int show_all_config(const char *key_, const char *value_, void *cb)
>  {
> -	if (value_)
> +	if (!show_only_keys && value_)

That is, "if not omitting values and there is a value, then do this".

> -	return format_config(&values->items[values->nr++], key_, value_);
> +	if (show_only_keys) {
> +		struct strbuf *buf = &values->items[values->nr++];
> +		strbuf_init(buf, 0);
> +		strbuf_addstr(buf, key_);
> +		strbuf_addch(buf, term);
> +		return 0;

xstrfmt()?

Re: [PATCH 1.5/2] config: add options to list only variable names

[Christian Couder]

On Wed, May 27, 2015 at 10:11 PM, SZEDER Gábor <szeder@ira.uka.de> wrote:
> Recenty I created a multi-line branch description with '.' and '='
> characters on one of the lines, and noticed that fragments of that line
> show up when completing set variable names for 'git config', e.g.:
>
>   $ git config --get branch.b.description
>   Branch description to fool the completion script with a
>   second line containing dot . and equals = characters.
>   $ git config --unset <TAB>
>   ...
>   second line containing dot . and equals
>   ...
>
> The completion script runs 'git config --list' and processes its output to
> strip the values and keep only the variable names.  It does so by looking
> for lines containing '.' and '=' and outputting everything before the '=',
> which was fooled by my multi-line branch description.
>
> A similar issue exists with aliases and pretty format aliases with
> multi-line values, but in that case 'git config --get-regexp' is run and
> subsequent lines don't have to contain either '.' or '=' to fool the
> completion script.
>
> Though 'git config' can produce null-terminated output for newline-safe
> parsing, that's of no use in this case, becase we can't cope with nulls in
> the shell.
>
> Help the completion script by introducing the '--list-names' and
> '--get-names-regexp' options, the "names-only" equivalents of '--list' and
> '--get-regexp', so it doesn't have to separate variable names from their
> values anymore.

Why don't you just add a '--name-only' option that can be used only
with '--list' and '--get-regexp'?

Like:

'git config' [<file-option>] [-z|--null] [--name-only] --get-regexp name_regex

and

'git config' [<file-option>] [-z|--null] [--name-only] -l | --list

?

It seems to me that it would reduce the number of options, and later
if we want to pass a format we could have maybe:

'git config' [<file-option>] [-z|--null] [--name-only |
--format=<format>] --get-regexp name_regex

and

'git config' [<file-option>] [-z|--null] [--name-only |
--format=<format>] -l | --list

Thanks,
Christian.

[PATCH v2 0/2] config: list only variable names for completion

[SZEDER Gábor]

Fixed misspelled option names in docs and in commit message, incorporated
Peff's suggestions, and renamed 'show_only_keys' to 'omit_values' in 1/2.

Only minor tweaking in 2/2's commit message.

SZEDER Gábor (2):
  config: add options to list only variable names
  completion: use new 'git config' options to reliably list variable
    names

 Documentation/git-config.txt           | 12 ++++++++++--
 builtin/config.c                       | 17 ++++++++++++++---
 contrib/completion/git-completion.bash | 19 +++++--------------
 t/t1300-repo-config.sh                 | 22 ++++++++++++++++++++++
 4 files changed, 51 insertions(+), 19 deletions(-)

-- 
2.4.2.349.g6883b65

[PATCH v2 1/2] config: add options to list only variable names

[SZEDER Gábor]

Recenty I created a multi-line branch description with '.' and '='
characters on one of the lines, and noticed that fragments of that line
show up when completing set variable names for 'git config', e.g.:

  $ git config --get branch.b.description
  Branch description to fool the completion script with a
  second line containing dot . and equals = characters.
  $ git config --unset <TAB>
  ...
  second line containing dot . and equals
  ...

The completion script runs 'git config --list' and processes its output to
strip the values and keep only the variable names.  It does so by looking
for lines containing '.' and '=' and outputting everything before the '=',
which was fooled by my multi-line branch description.

A similar issue exists with aliases and pretty format aliases with
multi-line values, but in that case 'git config --get-regexp' is run and
subsequent lines don't have to contain either '.' or '=' to fool the
completion script.

Though 'git config' can produce null-terminated output for newline-safe
parsing, that's of no use in this case, becase we can't cope with nulls in
the shell.

Help the completion script by introducing the '--list-names' and
'--get-name-regexp' options, the "names-only" equivalents of '--list' and
'--get-regexp', so it doesn't have to separate variable names from their
values anymore.

Signed-off-by: SZEDER Gábor <szeder@ira.uka.de>
---
 Documentation/git-config.txt           | 12 ++++++++++--
 builtin/config.c                       | 17 ++++++++++++++---
 contrib/completion/git-completion.bash |  4 ++--
 t/t1300-repo-config.sh                 | 22 ++++++++++++++++++++++
 4 files changed, 48 insertions(+), 7 deletions(-)

diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index 02ec096faa..b69c8592ac 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -16,11 +16,12 @@ SYNOPSIS
 'git config' [<file-option>] [type] [-z|--null] --get-all name [value_regex]
 'git config' [<file-option>] [type] [-z|--null] --get-regexp name_regex [value_regex]
 'git config' [<file-option>] [type] [-z|--null] --get-urlmatch name URL
+'git config' [<file-option>] [-z|--null] --get-name-regexp name_regex
 'git config' [<file-option>] --unset name [value_regex]
 'git config' [<file-option>] --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>] [-z|--null] -l | --list
+'git config' [<file-option>] [-z|--null] -l | --list | --list-names
 'git config' [<file-option>] --get-color name [default]
 'git config' [<file-option>] --get-colorbool name [stdout-is-tty]
 'git config' [<file-option>] -e | --edit
@@ -96,6 +97,10 @@ OPTIONS
 	in which section and variable names are lowercased, but subsection
 	names are not.
 
+--get-name-regexp::
+	Like --get-regexp, but shows only matching variable names, not its
+	values.
+
 --get-urlmatch name URL::
 	When given a two-part name section.key, the value for
 	section.<url>.key whose <url> part matches the best to the
@@ -159,7 +164,10 @@ See also <<FILES>>.
 
 -l::
 --list::
-	List all variables set in config file.
+	List all variables set in config file, along with their values.
+
+--list-names::
+	List the names of all variables set in config file.
 
 --bool::
 	'git config' will ensure that the output is "true" or "false"
diff --git a/builtin/config.c b/builtin/config.c
index 7188405f7e..c23f329b00 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -13,6 +13,7 @@ static char *key;
 static regex_t *key_regexp;
 static regex_t *regexp;
 static int show_keys;
+static int omit_values;
 static int use_key_regexp;
 static int do_all;
 static int do_not_match;
@@ -43,6 +44,8 @@ static int respect_includes = -1;
 #define ACTION_GET_COLOR (1<<13)
 #define ACTION_GET_COLORBOOL (1<<14)
 #define ACTION_GET_URLMATCH (1<<15)
+#define ACTION_LIST_NAMES (1<<16)
+#define ACTION_GET_NAME_REGEXP (1<<17)
 
 #define TYPE_BOOL (1<<0)
 #define TYPE_INT (1<<1)
@@ -60,6 +63,7 @@ static struct option builtin_config_options[] = {
 	OPT_BIT(0, "get", &actions, N_("get value: name [value-regex]"), ACTION_GET),
 	OPT_BIT(0, "get-all", &actions, N_("get all values: key [value-regex]"), ACTION_GET_ALL),
 	OPT_BIT(0, "get-regexp", &actions, N_("get values for regexp: name-regex [value-regex]"), ACTION_GET_REGEXP),
+	OPT_BIT(0, "get-name-regexp", &actions, N_("get names for regexp: name-regex"), ACTION_GET_NAME_REGEXP),
 	OPT_BIT(0, "get-urlmatch", &actions, N_("get value specific for the URL: section[.var] URL"), ACTION_GET_URLMATCH),
 	OPT_BIT(0, "replace-all", &actions, N_("replace all matching variables: name value [value_regex]"), ACTION_REPLACE_ALL),
 	OPT_BIT(0, "add", &actions, N_("add a new variable: name value"), ACTION_ADD),
@@ -68,6 +72,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_BIT(0, "list-names", &actions, N_("list all variable names"), ACTION_LIST_NAMES),
 	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),
@@ -91,7 +96,7 @@ static void check_argc(int argc, int min, int max) {
 
 static int show_all_config(const char *key_, const char *value_, void *cb)
 {
-	if (value_)
+	if (!omit_values && value_)
 		printf("%s%c%s%c", key_, delim, value_, term);
 	else
 		printf("%s%c", key_, term);
@@ -117,6 +122,10 @@ static int format_config(struct strbuf *buf, const char *key_, const char *value
 		strbuf_addstr(buf, key_);
 		must_print_delim = 1;
 	}
+	if (omit_values) {
+		strbuf_addch(buf, term);
+		return 0;
+	}
 	if (types == TYPE_INT)
 		sprintf(value, "%"PRId64,
 			git_config_int64(key_, value_ ? value_ : ""));
@@ -550,7 +559,8 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 			usage_with_options(builtin_config_usage, builtin_config_options);
 		}
 
-	if (actions == ACTION_LIST) {
+	if (actions == ACTION_LIST || actions == ACTION_LIST_NAMES) {
+		omit_values = (actions == ACTION_LIST_NAMES);
 		check_argc(argc, 0, 0);
 		if (git_config_with_options(show_all_config, NULL,
 					    &given_config_source,
@@ -631,8 +641,9 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 		check_argc(argc, 1, 2);
 		return get_value(argv[0], argv[1]);
 	}
-	else if (actions == ACTION_GET_REGEXP) {
+	else if (actions == ACTION_GET_REGEXP || actions == ACTION_GET_NAME_REGEXP) {
 		show_keys = 1;
+		omit_values = (actions == ACTION_GET_NAME_REGEXP);
 		use_key_regexp = 1;
 		do_all = 1;
 		check_argc(argc, 1, 2);
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index bfc74e9d57..6abbd564b6 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1883,8 +1883,8 @@ _git_config ()
 	--*)
 		__gitcomp "
 			--system --global --local --file=
-			--list --replace-all
-			--get --get-all --get-regexp
+			--list --list-names --replace-all
+			--get --get-all --get-regexp --get-name-regexp
 			--add --unset --unset-all
 			--remove-section --rename-section
 			"
diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index 66dd28644f..525b093c59 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -353,6 +353,18 @@ test_expect_success '--list without repo produces empty output' '
 '
 
 cat > expect << EOF
+beta.noindent
+nextsection.nonewline
+123456.a123
+version.1.2.3eX.alpha
+EOF
+
+test_expect_success 'working --list-names' '
+	git config --list-names >output &&
+	test_cmp expect output
+'
+
+cat > expect << EOF
 beta.noindent sillyValue
 nextsection.nonewline wow2 for me
 EOF
@@ -363,6 +375,16 @@ test_expect_success '--get-regexp' '
 '
 
 cat > expect << EOF
+beta.noindent
+nextsection.nonewline
+EOF
+
+test_expect_success '--get-name-regexp' '
+	git config --get-name-regexp in >output &&
+	test_cmp expect output
+'
+
+cat > expect << EOF
 wow2 for me
 wow4 for you
 EOF
-- 
2.4.2.349.g6883b65

[PATCH v2 2/2] completion: use new 'git config' options to reliably list variable names

[SZEDER Gábor]

List all set config variable names with 'git config --list-names' instead
of '--list' and post processing.  Similarly, use 'git config
--get-name-regexp' instead of '--get-regexp' to get config variables in a
given section.

Signed-off-by: SZEDER Gábor <szeder@ira.uka.de>
---
 contrib/completion/git-completion.bash | 15 +++------------
 1 file changed, 3 insertions(+), 12 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 6abbd564b6..121aa31342 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -744,9 +744,8 @@ __git_compute_porcelain_commands ()
 __git_get_config_variables ()
 {
 	local section="$1" i IFS=$'\n'
-	for i in $(git --git-dir="$(__gitdir)" config --get-regexp "^$section\..*" 2>/dev/null); do
-		i="${i#$section.}"
-		echo "${i/ */}"
+	for i in $(git --git-dir="$(__gitdir)" config --get-name-regexp "^$section\..*" 2>/dev/null); do
+		echo "${i#$section.}"
 	done
 }
 
@@ -1774,15 +1773,7 @@ __git_config_get_set_variables ()
 		c=$((--c))
 	done
 
-	git --git-dir="$(__gitdir)" config $config_file --list 2>/dev/null |
-	while read -r line
-	do
-		case "$line" in
-		*.*=*)
-			echo "${line/=*/}"
-			;;
-		esac
-	done
+	git --git-dir="$(__gitdir)" config $config_file --list-names 2>/dev/null
 }
 
 _git_config ()
-- 
2.4.2.349.g6883b65

Re: [PATCH v2 1/2] config: add options to list only variable names

[Junio C Hamano]

SZEDER Gábor <szeder@ira.uka.de> writes:

> @@ -16,11 +16,12 @@ SYNOPSIS
>  'git config' [<file-option>] [type] [-z|--null] --get-all name [value_regex]
>  'git config' [<file-option>] [type] [-z|--null] --get-regexp name_regex [value_regex]
>  'git config' [<file-option>] [type] [-z|--null] --get-urlmatch name URL
> +'git config' [<file-option>] [-z|--null] --get-name-regexp name_regex
> ...

At first I wondered why --get-name-regexp is needed, as (purely from
the syntactic level) it appeared at the first glance the existing
'--get-regexp' without 'value_regex' may be sufficient, until I read
this:

> +--get-name-regexp::
> +	Like --get-regexp, but shows only matching variable names, not its
> +	values.

which makes it clear why it is needed.  The distinction is purely
about the output, i.e. the values are omitted.

But then it makes me wonder why --get-name-regexp shouldn't
optionally accept value_regex like --get-regexp does, allowing you
to say "list the variables that match this pattern whose values
match this other pattern".  I know it may be a feature that is
unnecessary for your purpose, but from a cursory look of the patch
text, you do not seem to be doing anything different between
get-regexp and get-name-regexp codepaths other than flipping
omit_values bit on, so it could be that the feature is already
supported but forgot to document it, perhaps?

The 'type' may also be shared between these two options, no?  It
would be logically consistent if you can say

	git config --bool --get-name-regexp '.*' 'no'

to find all configuration variables that are set to 'false' in
different spellings like '0', 'false', 'no', etc.  And I suspect
that the code already supports that.

Tangentially related to the above issue, but

	git config --bool --get-regexp '.*' 'no'

seems to be broken from that point of view.  Instead of ignoring a
non-bool string valued variables, it seems to barf upon seeing the
first such variable.  Interestingly, --get-name-regexp does not
share that breakage ;-)

Which we may want to fix, but not in the scope of this series.

Hint, hint...

Re: [PATCH v2 1/2] config: add options to list only variable names

[Junio C Hamano]

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

> The 'type' may also be shared between these two options, no?  It
> would be logically consistent if you can say
>
> 	git config --bool --get-name-regexp '.*' 'no'
>
> to find all configuration variables that are set to 'false' in
> different spellings like '0', 'false', 'no', etc.  And I suspect
> that the code already supports that.

Not really.  This of course inherits the same breakage from the
existing --get-regexp code in that the value part is still matched
as string.  I am sure you could argue that "but read the name of the
last optional argument; it says value_REGEX and it is clearly about
matching textually", and it may technically not be incorrect per-se,
but I'd say it is merely a justification for a lazy implementation
to defend the current less-than-useful behaviour.

In any case, because it is still a textual match, user.name='Junio
Hamano' matches with the above.  The only reason why it does not
barf is because it does not try to interpret and format that value
as a boolean.

So I would say that the feature supports [type] and [value_regex]
to exactly the same degree as --get-regexp, i.e. with breakage.

Which means we should document it the same way, even though both are
equally broken.  So that other people can later visit it and correct
the issue for both options.

-- >8 --
Subject: [PATCH] SQUASH???

The new option --get-name-regexp is a variant of --get-regexp; show
them next to each other, and also document [type] and [value_regex]
that the option seems to support.
---
 Documentation/git-config.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index b69c859..9fc78d8 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -15,8 +15,8 @@ SYNOPSIS
 'git config' [<file-option>] [type] [-z|--null] --get name [value_regex]
 'git config' [<file-option>] [type] [-z|--null] --get-all name [value_regex]
 'git config' [<file-option>] [type] [-z|--null] --get-regexp name_regex [value_regex]
+'git config' [<file-option>] [type] [-z|--null] --get-name-regexp name_regex [value_regex]
 'git config' [<file-option>] [type] [-z|--null] --get-urlmatch name URL
-'git config' [<file-option>] [-z|--null] --get-name-regexp name_regex
 'git config' [<file-option>] --unset name [value_regex]
 'git config' [<file-option>] --unset-all name [value_regex]
 'git config' [<file-option>] --rename-section old_name new_name
-- 
2.4.2-521-g2db3d00

Re: [PATCH v2 0/2] config: list only variable names for completion

[Jeff King]

On Thu, May 28, 2015 at 02:29:33PM +0200, SZEDER Gábor wrote:

> Fixed misspelled option names in docs and in commit message, incorporated
> Peff's suggestions, and renamed 'show_only_keys' to 'omit_values' in 1/2.
> 
> Only minor tweaking in 2/2's commit message.

This version looks good to me (though I agree with Junio's proposed
squash about the '[value_regex]' documentation).

-Peff

Re: [PATCH v2 1/2] config: add options to list only variable names

[Christian Couder]

On Thu, May 28, 2015 at 9:20 PM, Junio C Hamano <gitster@pobox.com> wrote:
> SZEDER Gábor <szeder@ira.uka.de> writes:
>
>> @@ -16,11 +16,12 @@ SYNOPSIS
>>  'git config' [<file-option>] [type] [-z|--null] --get-all name [value_regex]
>>  'git config' [<file-option>] [type] [-z|--null] --get-regexp name_regex [value_regex]
>>  'git config' [<file-option>] [type] [-z|--null] --get-urlmatch name URL
>> +'git config' [<file-option>] [-z|--null] --get-name-regexp name_regex
>> ...
>
> At first I wondered why --get-name-regexp is needed, as (purely from
> the syntactic level) it appeared at the first glance the existing
> '--get-regexp' without 'value_regex' may be sufficient, until I read
> this:
>
>> +--get-name-regexp::
>> +     Like --get-regexp, but shows only matching variable names, not its
>> +     values.
>
> which makes it clear why it is needed.  The distinction is purely
> about the output, i.e. the values are omitted.

If the distinction is purely about the output, then it seems logical
to add only an output related option, like the --name-only option I
suggested, and not 2 new modes (--get-name-regexp and --list-names).

Doesn't it look like git config already has too many modes?

> But then it makes me wonder why --get-name-regexp shouldn't
> optionally accept value_regex like --get-regexp does, allowing you
> to say "list the variables that match this pattern whose values
> match this other pattern".  I know it may be a feature that is
> unnecessary for your purpose, but from a cursory look of the patch
> text, you do not seem to be doing anything different between
> get-regexp and get-name-regexp codepaths other than flipping
> omit_values bit on, so it could be that the feature is already
> supported but forgot to document it, perhaps?

This also suggests that it might be more logical to only add an option
called --name-only or --omit-values that would map directly to the
omit_values bit in the code.

Re: [PATCH v2 1/2] config: add options to list only variable names

[Jeff King]

On Fri, May 29, 2015 at 01:34:50PM +0200, Christian Couder wrote:

> >> +--get-name-regexp::
> >> +     Like --get-regexp, but shows only matching variable names, not its
> >> +     values.
> >
> > which makes it clear why it is needed.  The distinction is purely
> > about the output, i.e. the values are omitted.
> 
> If the distinction is purely about the output, then it seems logical
> to add only an output related option, like the --name-only option I
> suggested, and not 2 new modes (--get-name-regexp and --list-names).
> 
> Doesn't it look like git config already has too many modes?

Yeah, I'd agree that is a simpler way to look at it, and avoiding extra
modes is probably a good thing.

-Peff