[PATCHv3 0/2] 'git config --names-only' to help the completion script

[PATCHv3 0/2] 'git config --names-only' to help the completion script

[SZEDER Gábor]

This is a reroll of 'sg/config-name-only'.

  * Instead of the two new listing options of the previous round add one
    new option '--names-only' to modify the output of '--list' and
    '--get-regexp' options, as suggested in previous discussions.
  * Reorganized the commit messages: don't go into details of the
    completion bug in the first patch modifying builtin/config.c, talk
    about that in the second patch.

SZEDER Gábor (2):
  config: add '--names-only' option to list only variable names
  completion: list variable names reliably with 'git config
    --names-only'

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

-- 
2.5.0.245.gff6622b

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

[SZEDER Gábor]

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

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

diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index 02ec096faa..ba76097c06 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -14,13 +14,13 @@ SYNOPSIS
 'git config' [<file-option>] [type] --replace-all name value [value_regex]
 '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] [--names-only] --get-regexp name_regex [value_regex]
 'git config' [<file-option>] [type] [-z|--null] --get-urlmatch name URL
 '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] [--names-only] -l | --list
 'git config' [<file-option>] --get-color name [default]
 'git config' [<file-option>] --get-colorbool name [stdout-is-tty]
 'git config' [<file-option>] -e | --edit
@@ -159,7 +159,7 @@ See also <<FILES>>.
 
 -l::
 --list::
-	List all variables set in config file.
+	List all variables set in config file, along with their values.
 
 --bool::
 	'git config' will ensure that the output is "true" or "false"
@@ -190,6 +190,10 @@ See also <<FILES>>.
 	output without getting confused e.g. by values that
 	contain line breaks.
 
+--names-only::
+	Output only the names of config variables for `--list` or
+	`--get-regexp`.
+
 --get-colorbool name [stdout-is-tty]::
 
 	Find the color setting for `name` (e.g. `color.diff`) and output
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;
@@ -78,6 +79,7 @@ static struct option builtin_config_options[] = {
 	OPT_BIT(0, "path", &types, N_("value is a path (file or directory name)"), TYPE_PATH),
 	OPT_GROUP(N_("Other")),
 	OPT_BOOL('z', "null", &end_null, N_("terminate values with NUL byte")),
+	OPT_BOOL(0, "names-only", &omit_values, N_("names only")),
 	OPT_BOOL(0, "includes", &respect_includes, N_("respect include directives on lookup")),
 	OPT_END(),
 };
@@ -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_)
 		printf("%s%c%s%c", key_, delim, value_, term);
 	else
 		printf("%s%c", key_, term);
@@ -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;
+	}
 	if (types == TYPE_INT)
 		sprintf(value, "%"PRId64,
 			git_config_int64(key_, value_ ? value_ : ""));
@@ -549,7 +555,11 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 		default:
 			usage_with_options(builtin_config_usage, builtin_config_options);
 		}
-
+	if (omit_values &&
+	    !(actions == ACTION_LIST || actions == ACTION_GET_REGEXP)) {
+		error("--names-only is only applicable to --list or --get-regexp");
+		usage_with_options(builtin_config_usage, builtin_config_options);
+	}
 	if (actions == ACTION_LIST) {
 		check_argc(argc, 0, 0);
 		if (git_config_with_options(show_all_config, NULL,
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index c97c648d7e..6eaab141e2 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1887,6 +1887,7 @@ _git_config ()
 			--get --get-all --get-regexp
 			--add --unset --unset-all
 			--remove-section --rename-section
+			--names-only
 			"
 		return
 		;;
diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index 66dd28644f..97e9e76efc 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 '--names-only --list' '
+	git config --names-only --list >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 '--names-only --get-regexp' '
+	git config --names-only --get-regexp in >output &&
+	test_cmp expect output
+'
+
+cat > expect << EOF
 wow2 for me
 wow4 for you
 EOF
-- 
2.5.0.245.gff6622b

[PATCHv3 2/2] completion: list variable names reliably with 'git config --names-only'

[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
lines in its output are simply stripped after the first space, so
subsequent lines don't even have to contain '.' and '=' to fool the
completion script.

Use the new '--names-only' option added in the previous commit to list
config variable names reliably in both cases, without error-prone post
processing.

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 6eaab141e2..7200828fc4 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 --names-only --get-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 --names-only --list 2>/dev/null
 }
 
 _git_config ()
-- 
2.5.0.245.gff6622b

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

[Jeff King]

On Mon, Aug 10, 2015 at 11:46:06AM +0200, SZEDER Gábor wrote:

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

Nice. The whole thing looks very neatly done. I have only one minor nit:
the option "--names-only" is _almost_ the same as the "--name-only" diff
option which is somewhat similar. Obviously they do different things and
do not need to match, but I wonder if it would create less annoyance to
just give them the same name.

-Peff

Re: [PATCHv3 2/2] completion: list variable names reliably with 'git config --names-only'

[Jeff King]

On Mon, Aug 10, 2015 at 11:46:07AM +0200, SZEDER Gábor wrote:

> Use the new '--names-only' option added in the previous commit to list
> config variable names reliably in both cases, without error-prone post
> processing.
> 
> Signed-off-by: SZEDER Gábor <szeder@ira.uka.de>

This looks like a pretty straight-forward application of part 1, and the
resulting code is much nicer to read.

Both patches:

  Acked-by: Jeff King <peff@peff.net>

though I do not think my "ack" on completion code is worth anything. :)

-Peff

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

[Junio C Hamano]

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.

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

[SZEDER Gábor]

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

OK.

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

OK.

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

Well, the name 'omit_values' was suggested by Peff after the first  
round.  I'm happy to rename it to whatever you agree upon :)


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

How about restructuring the function like this?  Perhaps even better  
than a tri-state toggle would be.
(showing the result instead of the diff, because all the indentation  
changes make the diff hard to read).

static int format_config(struct strbuf *buf, const char *key_, const  
char *value_)
{
         strbuf_init(buf, 0);

         if (show_keys)
                 strbuf_addstr(buf, key_);
         if (!omit_values) {                  // or show_values
                 int must_free_vptr = 0;
                 int must_add_delim = show_keys;
                 char value[256];
                 const char *vptr = value;

                 if (types == TYPE_INT)
                         sprintf(value, "%"PRId64,
                                 git_config_int64(key_, value_ ? value_ : ""));
                 else if (types == TYPE_BOOL)
                         vptr = git_config_bool(key_, value_) ? "true"  
: "false";
                 else if (types == TYPE_BOOL_OR_INT) {
                         int is_bool, v;
                         v = git_config_bool_or_int(key_, value_, &is_bool);
                         if (is_bool)
                                 vptr = v ? "true" : "false";
                         else
                                 sprintf(value, "%d", v);
                 } else if (types == TYPE_PATH) {
                         if (git_config_pathname(&vptr, key_, value_) < 0)
                                 return -1;
                         must_free_vptr = 1;
                 } else if (value_) {
                         vptr = value_;
                 } else {
                         /* Just show the key name */
                         vptr = "";
                         must_add_delim = 0;
                 }

                 if (must_add_delim)
                         strbuf_addch(buf, key_delim);
                 strbuf_addstr(buf, vptr);

                 if (must_free_vptr)
                         free((char *)vptr);
         }
         strbuf_addch(buf, term);
         return 0;
}


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

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

[Junio C Hamano]

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

>>
>> s/becase/because/;
>
> OK.
> ...
>> 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.
>
> OK.
> ...

The topic is now in 'next'; I think I've locally fixed it up for
these when I originally queued them a few days ago, so if there are
any remaining issues, please throw incremental polishing patches.

Thanks.

[PATCH] config: restructure format_config() for better control flow

[SZEDER Gábor]

Commit 578625fa91 (config: add '--name-only' option to list only
variable names, 2015-08-10) modified format_config() such that it
returned from the middle of the function when showing only keys,
resulting in ugly code structure.

Reorganize the if statements and dealing with the key-value delimiter to
make the function easier to read.

Signed-off-by: SZEDER Gábor <szeder@ira.uka.de>
---

> The topic is now in 'next'; I think I've locally fixed it up for
> these when I originally queued them a few days ago, so if there are
> any remaining issues, please throw incremental polishing patches.

OK, though it's not a major issue, I think this is still worth doing on
top.

 builtin/config.c | 78 +++++++++++++++++++++++++++-----------------------------
 1 file changed, 37 insertions(+), 41 deletions(-)

diff --git a/builtin/config.c b/builtin/config.c
index 631db458ec..810e104224 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -108,52 +108,48 @@ struct strbuf_list {
 
 static int format_config(struct strbuf *buf, const char *key_, const char *value_)
 {
-	int must_free_vptr = 0;
-	int must_print_delim = 0;
-	char value[256];
-	const char *vptr = value;
-
 	strbuf_init(buf, 0);
 
-	if (show_keys) {
+	if (show_keys)
 		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_ : ""));
-	else if (types == TYPE_BOOL)
-		vptr = git_config_bool(key_, value_) ? "true" : "false";
-	else if (types == TYPE_BOOL_OR_INT) {
-		int is_bool, v;
-		v = git_config_bool_or_int(key_, value_, &is_bool);
-		if (is_bool)
-			vptr = v ? "true" : "false";
-		else
-			sprintf(value, "%d", v);
-	} else if (types == TYPE_PATH) {
-		if (git_config_pathname(&vptr, key_, value_) < 0)
-			return -1;
-		must_free_vptr = 1;
-	} else if (value_) {
-		vptr = value_;
-	} else {
-		/* Just show the key name */
-		vptr = "";
-		must_print_delim = 0;
-	}
+	if (!omit_values) {
+		int must_free_vptr = 0;
+		int must_add_delim = show_keys;
+		char value[256];
+		const char *vptr = value;
 
-	if (must_print_delim)
-		strbuf_addch(buf, key_delim);
-	strbuf_addstr(buf, vptr);
+		if (types == TYPE_INT)
+			sprintf(value, "%"PRId64,
+				git_config_int64(key_, value_ ? value_ : ""));
+		else if (types == TYPE_BOOL)
+			vptr = git_config_bool(key_, value_) ? "true" : "false";
+		else if (types == TYPE_BOOL_OR_INT) {
+			int is_bool, v;
+			v = git_config_bool_or_int(key_, value_, &is_bool);
+			if (is_bool)
+				vptr = v ? "true" : "false";
+			else
+				sprintf(value, "%d", v);
+		} else if (types == TYPE_PATH) {
+			if (git_config_pathname(&vptr, key_, value_) < 0)
+				return -1;
+			must_free_vptr = 1;
+		} else if (value_) {
+			vptr = value_;
+		} else {
+			/* Just show the key name */
+			vptr = "";
+			must_add_delim = 0;
+		}
+
+		if (must_add_delim)
+			strbuf_addch(buf, key_delim);
+		strbuf_addstr(buf, vptr);
+
+		if (must_free_vptr)
+			free((char *)vptr);
+	}
 	strbuf_addch(buf, term);
-
-	if (must_free_vptr)
-		free((char *)vptr);
 	return 0;
 }
 
-- 
2.5.0.415.g33d64ef

Re: [PATCH] config: restructure format_config() for better control flow

[Jeff King]

On Thu, Aug 20, 2015 at 04:14:22PM +0200, SZEDER Gábor wrote:

> Commit 578625fa91 (config: add '--name-only' option to list only
> variable names, 2015-08-10) modified format_config() such that it
> returned from the middle of the function when showing only keys,
> resulting in ugly code structure.
> 
> Reorganize the if statements and dealing with the key-value delimiter to
> make the function easier to read.

This looks good to me. What do you think of these on top?

  [1/3]: format_config: don't init strbuf
  [2/3]: format_config: simplify buffer handling
  [3/3]: get_urlmatch: avoid useless strbuf write

-Peff

[PATCH 1/3] format_config: don't init strbuf

[Jeff King]

It's unusual for a function which writes to a passed-in
strbuf to call strbuf_init; that will throw away anything
already there, leaking memory. In this case, there are
exactly two callers; one relies on this initialization and
the other passes in an already-initialized buffer.

There's no leak, as the initialized buffer doesn't have
anything in it. But let's bump the strbuf_init out to the
one caller who needs it, making format_config more
idiomatic.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/config.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/builtin/config.c b/builtin/config.c
index 810e104..91aa56f 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -108,8 +108,6 @@ struct strbuf_list {
 
 static int format_config(struct strbuf *buf, const char *key_, const char *value_)
 {
-	strbuf_init(buf, 0);
-
 	if (show_keys)
 		strbuf_addstr(buf, key_);
 	if (!omit_values) {
@@ -166,6 +164,7 @@ static int collect_config(const char *key_, const char *value_, void *cb)
 		return 0;
 
 	ALLOC_GROW(values->items, values->nr + 1, values->alloc);
+	strbuf_init(&values->items[values->nr], 0);
 
 	return format_config(&values->items[values->nr++], key_, value_);
 }
-- 
2.5.0.680.g69e7703

[PATCH 2/3] format_config: simplify buffer handling

[Jeff King]

When formatting a config value into a strbuf, we may end
up stringifying it into a fixed-size buffer using sprintf,
and then copying that buffer into the strbuf. We can
eliminate the middle-man (and drop some calls to sprintf!)
by writing directly to the strbuf.

The reason it was written this way in the first place is
that we need to know before writing the value whether to
insert a delimiter. Instead of delaying the write of the
value, we speculatively write the delimiter, and roll it
back in the single case that cares.

Signed-off-by: Jeff King <peff@peff.net>
---
I admit the rollback is a little gross. The other option would be adding
the delimiter in each of the conditional branches, which is also kind of
nasty. Or we could leave the buffer-write as-is, and replace the
sprintfs with snprintfs (this is actually something I was doing in
another branch, which is why I took particular notice; your patch
conflicts with my to-be-submitted branch :) ).

 builtin/config.c | 38 ++++++++++++++++----------------------
 1 file changed, 16 insertions(+), 22 deletions(-)

diff --git a/builtin/config.c b/builtin/config.c
index 91aa56f..04befce 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -111,41 +111,35 @@ static int format_config(struct strbuf *buf, const char *key_, const char *value
 	if (show_keys)
 		strbuf_addstr(buf, key_);
 	if (!omit_values) {
-		int must_free_vptr = 0;
-		int must_add_delim = show_keys;
-		char value[256];
-		const char *vptr = value;
+		if (show_keys)
+			strbuf_addch(buf, key_delim);
 
 		if (types == TYPE_INT)
-			sprintf(value, "%"PRId64,
-				git_config_int64(key_, value_ ? value_ : ""));
+			strbuf_addf(buf, "%"PRId64,
+				    git_config_int64(key_, value_ ? value_ : ""));
 		else if (types == TYPE_BOOL)
-			vptr = git_config_bool(key_, value_) ? "true" : "false";
+			strbuf_addstr(buf, git_config_bool(key_, value_) ?
+				      "true" : "false");
 		else if (types == TYPE_BOOL_OR_INT) {
 			int is_bool, v;
 			v = git_config_bool_or_int(key_, value_, &is_bool);
 			if (is_bool)
-				vptr = v ? "true" : "false";
+				strbuf_addstr(buf, v ? "true" : "false");
 			else
-				sprintf(value, "%d", v);
+				strbuf_addf(buf, "%d", v);
 		} else if (types == TYPE_PATH) {
-			if (git_config_pathname(&vptr, key_, value_) < 0)
+			const char *v;
+			if (git_config_pathname(&v, key_, value_) < 0)
 				return -1;
-			must_free_vptr = 1;
+			strbuf_addstr(buf, v);
+			free((char *)v);
 		} else if (value_) {
-			vptr = value_;
+			strbuf_addstr(buf, value_);
 		} else {
-			/* Just show the key name */
-			vptr = "";
-			must_add_delim = 0;
+			/* Just show the key name; back out delimiter */
+			if (show_keys)
+				strbuf_setlen(buf, buf->len - 1);
 		}
-
-		if (must_add_delim)
-			strbuf_addch(buf, key_delim);
-		strbuf_addstr(buf, vptr);
-
-		if (must_free_vptr)
-			free((char *)vptr);
 	}
 	strbuf_addch(buf, term);
 	return 0;
-- 
2.5.0.680.g69e7703

[PATCH 3/3] get_urlmatch: avoid useless strbuf write

[Jeff King]

We create a strbuf only to insert a single string, pass the
resulting buffer to a function (which does not modify the
string), and then free it. We can just pass the original
string instead.

Signed-off-by: Jeff King <peff@peff.net>
---
I keep staring at this thinking I missed something, but I think this is
an equivalent transformation. I wonder if the original meant to modify
the key in some way, but I don't see how or why.

 builtin/config.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/builtin/config.c b/builtin/config.c
index 04befce..71acc44 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -425,14 +425,11 @@ static int get_urlmatch(const char *var, const char *url)
 
 	for_each_string_list_item(item, &values) {
 		struct urlmatch_current_candidate_value *matched = item->util;
-		struct strbuf key = STRBUF_INIT;
 		struct strbuf buf = STRBUF_INIT;
 
-		strbuf_addstr(&key, item->string);
-		format_config(&buf, key.buf,
+		format_config(&buf, item->string,
 			      matched->value_is_null ? NULL : matched->value.buf);
 		fwrite(buf.buf, 1, buf.len, stdout);
-		strbuf_release(&key);
 		strbuf_release(&buf);
 
 		strbuf_release(&matched->value);
-- 
2.5.0.680.g69e7703

Re: [PATCH] config: restructure format_config() for better control flow

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Thu, Aug 20, 2015 at 04:14:22PM +0200, SZEDER Gábor wrote:
>
>> Commit 578625fa91 (config: add '--name-only' option to list only
>> variable names, 2015-08-10) modified format_config() such that it
>> returned from the middle of the function when showing only keys,
>> resulting in ugly code structure.
>> 
>> Reorganize the if statements and dealing with the key-value delimiter to
>> make the function easier to read.
>
> This looks good to me. What do you think of these on top?
>
>   [1/3]: format_config: don't init strbuf
>   [2/3]: format_config: simplify buffer handling
>   [3/3]: get_urlmatch: avoid useless strbuf write

All four including the thread-starter by SZEDER look good to me.
Thanks.

Re: [PATCH 2/3] format_config: simplify buffer handling

[SZEDER Gábor]

Quoting Jeff King <peff@peff.net>:

> When formatting a config value into a strbuf, we may end
> up stringifying it into a fixed-size buffer using sprintf,
> and then copying that buffer into the strbuf. We can
> eliminate the middle-man (and drop some calls to sprintf!)
> by writing directly to the strbuf.
>
> The reason it was written this way in the first place is
> that we need to know before writing the value whether to
> insert a delimiter. Instead of delaying the write of the
> value, we speculatively write the delimiter, and roll it
> back in the single case that cares.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> I admit the rollback is a little gross.

Indeed it is, but I'm for it, as it gets rit of so much more
other grossness, i.e. the fixed-size buffer and vptr stuff
and the two must_do_this variables.


> builtin/config.c | 38 ++++++++++++++++----------------------
> 1 file changed, 16 insertions(+), 22 deletions(-)
>
> diff --git a/builtin/config.c b/builtin/config.c
> index 91aa56f..04befce 100644
> --- a/builtin/config.c
> +++ b/builtin/config.c
> @@ -111,41 +111,35 @@ static int format_config(struct strbuf *buf,  
> const char *key_, const char *value
> 	if (show_keys)
> 		strbuf_addstr(buf, key_);
> 	if (!omit_values) {
> -		int must_free_vptr = 0;
> -		int must_add_delim = show_keys;
> -		char value[256];
> -		const char *vptr = value;
> +		if (show_keys)
> +			strbuf_addch(buf, key_delim);
>
> 		if (types == TYPE_INT)
> -			sprintf(value, "%"PRId64,
> -				git_config_int64(key_, value_ ? value_ : ""));
> +			strbuf_addf(buf, "%"PRId64,
> +				    git_config_int64(key_, value_ ? value_ : ""));
> 		else if (types == TYPE_BOOL)
> -			vptr = git_config_bool(key_, value_) ? "true" : "false";
> +			strbuf_addstr(buf, git_config_bool(key_, value_) ?
> +				      "true" : "false");
> 		else if (types == TYPE_BOOL_OR_INT) {
> 			int is_bool, v;
> 			v = git_config_bool_or_int(key_, value_, &is_bool);
> 			if (is_bool)
> -				vptr = v ? "true" : "false";
> +				strbuf_addstr(buf, v ? "true" : "false");
> 			else
> -				sprintf(value, "%d", v);
> +				strbuf_addf(buf, "%d", v);
> 		} else if (types == TYPE_PATH) {
> -			if (git_config_pathname(&vptr, key_, value_) < 0)
> +			const char *v;
> +			if (git_config_pathname(&v, key_, value_) < 0)
> 				return -1;
> -			must_free_vptr = 1;
> +			strbuf_addstr(buf, v);
> +			free((char *)v);
> 		} else if (value_) {
> -			vptr = value_;
> +			strbuf_addstr(buf, value_);
> 		} else {
> -			/* Just show the key name */
> -			vptr = "";
> -			must_add_delim = 0;
> +			/* Just show the key name; back out delimiter */
> +			if (show_keys)
> +				strbuf_setlen(buf, buf->len - 1);
> 		}
> -
> -		if (must_add_delim)
> -			strbuf_addch(buf, key_delim);
> -		strbuf_addstr(buf, vptr);
> -
> -		if (must_free_vptr)
> -			free((char *)vptr);
> 	}
> 	strbuf_addch(buf, term);
> 	return 0;
> --
> 2.5.0.680.g69e7703

Re: [PATCH 2/3] format_config: simplify buffer handling

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> When formatting a config value into a strbuf, we may end
> up stringifying it into a fixed-size buffer using sprintf,
> and then copying that buffer into the strbuf. We can
> eliminate the middle-man (and drop some calls to sprintf!)
> by writing directly to the strbuf.
>
> The reason it was written this way in the first place is
> that we need to know before writing the value whether to
> insert a delimiter. Instead of delaying the write of the
> value, we speculatively write the delimiter, and roll it
> back in the single case that cares.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> I admit the rollback is a little gross. The other option would be adding
> the delimiter in each of the conditional branches, which is also kind of
> nasty.

I actually am fine with this rollback.  The "variable alone stands
for true" is not something a user can produce from the command line
very easily, so having to rollback is a rare event anyway.

I wonder if we can do this instead

	if (!omit_values) {
-		if (show_keys)
+		if (show_keys && value_)
			strbuf_addch(buf, key_delim);

though.  That would eliminate the need for rolling back.

I briefly wondered how such a change would interact with

        if (types == TYPE_INT)
                strbuf_addf(buf, "%"PRId64,
                            git_config_int64(key_, value_ ? value_ : ""));

that immediately follows it, but this "turn NULL into an empty
string" may be bogus in the first place, in the sense that
git_config_int64() should complain about a NULL value_ the same way
as it would complain about an empty string---both them are not an
integer.  And indeed:

 - git_parse_int64() that is called from git_config_int64() is
   prepared to take both "" and NULL and return failure with EINVAL;

 - die_bad_number() that is eventually called when parsing fails by
   git_config_int64() is prepared to take NULL and turns it to an
   empty string.

So perhaps we could do this squashed in?

 builtin/config.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/builtin/config.c b/builtin/config.c
index 71acc44..593b1ae 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -111,12 +111,12 @@ static int format_config(struct strbuf *buf, const char *key_, const char *value
 	if (show_keys)
 		strbuf_addstr(buf, key_);
 	if (!omit_values) {
-		if (show_keys)
+		if (show_keys && value_)
 			strbuf_addch(buf, key_delim);
 
 		if (types == TYPE_INT)
 			strbuf_addf(buf, "%"PRId64,
-				    git_config_int64(key_, value_ ? value_ : ""));
+				    git_config_int64(key_, value_));
 		else if (types == TYPE_BOOL)
 			strbuf_addstr(buf, git_config_bool(key_, value_) ?
 				      "true" : "false");
@@ -136,9 +136,8 @@ static int format_config(struct strbuf *buf, const char *key_, const char *value
 		} else if (value_) {
 			strbuf_addstr(buf, value_);
 		} else {
-			/* Just show the key name; back out delimiter */
-			if (show_keys)
-				strbuf_setlen(buf, buf->len - 1);
+			/* Just show the key name */
+			;
 		}
 	}
 	strbuf_addch(buf, term);

Re: [PATCH 2/3] format_config: simplify buffer handling

[Junio C Hamano]

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

> I wonder if we can do this instead
>
> 	if (!omit_values) {
> -		if (show_keys)
> +		if (show_keys && value_)
> 			strbuf_addch(buf, key_delim);
>
> though.  That would eliminate the need for rolling back.

No we cannot.  "config --bool --get-regexp" could get a NULL value_
and has to turn it to " true".

Sorry for the noise.

Re: [PATCH 2/3] format_config: simplify buffer handling

[Jeff King]

On Fri, Aug 21, 2015 at 10:43:58AM -0700, Junio C Hamano wrote:

> Junio C Hamano <gitster@pobox.com> writes:
> 
> > I wonder if we can do this instead
> >
> > 	if (!omit_values) {
> > -		if (show_keys)
> > +		if (show_keys && value_)
> > 			strbuf_addch(buf, key_delim);
> >
> > though.  That would eliminate the need for rolling back.
> 
> No we cannot.  "config --bool --get-regexp" could get a NULL value_
> and has to turn it to " true".
> 
> Sorry for the noise.

Right, I had the same thought and reached the same conclusion.

By the way, I do not think the rollback by itself is gross, it is just
that it has to reproduce the "show_keys" logic. That is, it _really_
wants to be:

  else {
	  /* nothing to show; rollback delim */
	  if (we_added_delim)
		  strbuf_setlen(buf, buf->len - 1);
  }

and I use "show_keys" as a proxy for "did we add it". Which is
reproducing the logic that you quoted above, and if the two ever get out
of sync, it will be a bug. So you could write it as:

  int we_added_delim = 0;
  if (show_keys) {
	strbuf_addch(buf, key_delim);
	we_added_delim = 1;
  }

I didn't, because it's ugly, and the function is short enough and
unlikely enough to change that it probably won't matter.

You could perhaps also write it as:

  size_t baselen = buf->len;
  if (show_keys)
	strbuf_addch(buf, key_delim);
  ...
  else {
	/* rollback delim */
	strbuf_setlen(buf, baselen);
  }

-Peff