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

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 10, 2015, 17:18 UTC
Message-ID: <xmqqvbcnuko6.fsf@gitster.dls.corp.google.com>
In-Reply-To: <1439199967-9655-2-git-send-email-szeder@ira.uka.de>

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

Show 6 quoted lines

> '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/;

Show 9 quoted lines

> 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.

Show 19 quoted lines

> 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?

Show 8 quoted lines

> @@ -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.