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

2015-05-27 Thread 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 sze...@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 (113)
 #define ACTION_GET_COLORBOOL (114)
 #define ACTION_GET_URLMATCH (115)
+#define ACTION_LIST_NAMES (116)
+#define ACTION_GET_NAME_REGEXP (117)
 
 #define TYPE_BOOL (10)
 #define TYPE_INT (11)
@@ -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),

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

2015-05-27 Thread 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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2015-05-27 Thread 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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2015-05-27 Thread SZEDER Gábor 


Quoting Jeff King p...@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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2015-05-27 Thread Junio C Hamano 
SZEDER Gábor sze...@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()?
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html