Re: [PATCHv3 1/2] config: add '--names-only' option to list only variable names

[Junio C Hamano <gitster@pobox.com>]

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

> 'git config' can only show values or name-value pairs, so if a shell
> script needs the names of set config variables it has to run 'git config
> --list' or '--get-regexp' and parse the output to separate config
> variable names from their values.  However, such a parsing can't cope
> with multi-line values.  Though 'git config' can produce null-terminated
> output for newline-safe parsing, that's of no use in such a case, becase

s/becase/because/;

> shells can't cope with null characters.
>
> Even our own bash completion script suffers from these issues.
>
> Help the completion script, and shell scripts in general, by introducing
> the '--names-only' option to modify the output of '--list' and
> '--get-regexp' to list only the names of config variables, so they don't
> have to perform error-prone post processing to separate variable names
> from their values anymore.

I agree with Peff that "--names-only" has a subtle difference with
an existing and well known subcommand option and it would be a bit
irritating to remember which options is for which command.

> diff --git a/builtin/config.c b/builtin/config.c
> index 7188405f7e..307980ab50 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;
> ...
> @@ -91,7 +93,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_)

Hmmmm.  As we have "show_keys",

	if (show_values && value_)

would be a lot more intuitive, no?

> @@ -117,6 +119,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;
> +	}

This hunk makes me wonder what the assignment to "must_print_delim"
is about.  When the code is told to show only keys and not values,
it shouldn't even have to worry about key_delim, but that assignment
is done to control exactly that.  It happens that you are lucky that
you can "return 0" early here so that the assignment does not have
any effect, but still conceptually the code structure is made ugly
by this patch.

Isn't it more like the existing "show_keys" can be replaced/enhanced
with a single "show" tri-state toggle that chooses one among:

    * show both keys and values (for --list)
    * show only keys (for your new feature)
    * show only value (for --get)

perhaps?

I see get_urlmatch() abuses show_keys variable in a strange way, and
it may not be as trivial as removing show_keys and replacing it with
a new tri-state toggle, though.