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

From: Jeff King <peff@peff.net>
Date: May 27, 2015, 21:04 UTC
Message-ID: <20150527210438.GB22815@peff.net>
In-Reply-To: <1432757240-4445-1-git-send-email-szeder@ira.uka.de>

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

Show 6 quoted lines

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

Show 14 quoted lines

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

Show 13 quoted lines

>  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