{"thread":{"id":"40043","subject":"[PATCHv3 2/2] completion: list variable names reliably with 'git config --names-only'","startedAt":"2015-08-10T09:46:05Z","lastAt":"2015-08-21T19:40:01Z","messageCount":18,"participants":["SZEDER Gábor","Jeff King","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"267726","messageId":"1439199967-9655-1-git-send-email-szeder@ira.uka.de","threadId":"40043","inReplyTo":null,"subject":"[PATCHv3 0/2] 'git config --names-only' to help the completion script","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2015-08-10T09:46:05Z","receivedAt":"2015-08-10T09:46:05Z","isPatch":false,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"This is a reroll of 'sg/config-name-only'.\n\n * Instead of the two new listing options of the previous round add one\n new option '--names-only' to modify the output of '--list' and\n '--get-regexp' options, as suggested in previous discussions.\n * Reorganized the commit messages: don't go into details of the\n completion bug in the first patch modifying builtin/config.c, talk\n about that in the second patch.\n\nSZEDER Gábor (2):\n config: add '--names-only' option to list only variable names\n completion: list variable names reliably with 'git config\n --names-only'\n\n Documentation/git-config.txt | 10 +++++++---\n builtin/config.c | 14 ++++++++++++--\n contrib/completion/git-completion.bash | 16 ++++------------\n t/t1300-repo-config.sh | 22 ++++++++++++++++++++++\n 4 files changed, 45 insertions(+), 17 deletions(-)\n\n-- \n2.5.0.245.gff6622b\n"},{"id":"267725","messageId":"1439199967-9655-2-git-send-email-szeder@ira.uka.de","threadId":"40043","inReplyTo":"1439199967-9655-1-git-send-email-szeder@ira.uka.de","subject":"[PATCHv3 1/2] config: add '--names-only' option to list only variable names","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2015-08-10T09:46:06Z","receivedAt":"2015-08-10T09:46:06Z","isPatch":false,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"'git config' can only show values or name-value pairs, so if a shell\nscript needs the names of set config variables it has to run 'git config\n--list' or '--get-regexp' and parse the output to separate config\nvariable names from their values. However, such a parsing can't cope\nwith multi-line values. Though 'git config' can produce null-terminated\noutput for newline-safe parsing, that's of no use in such a case, becase\nshells can't cope with null characters.\n\nEven our own bash completion script suffers from these issues.\n\nHelp the completion script, and shell scripts in general, by introducing\nthe '--names-only' option to modify the output of '--list' and\n'--get-regexp' to list only the names of config variables, so they don't\nhave to perform error-prone post processing to separate variable names\nfrom their values anymore.\n\nSigned-off-by: SZEDER Gábor \n---\n Documentation/git-config.txt | 10 +++++++---\n builtin/config.c | 14 ++++++++++++--\n contrib/completion/git-completion.bash | 1 +\n t/t1300-repo-config.sh | 22 ++++++++++++++++++++++\n 4 files changed, 42 insertions(+), 5 deletions(-)\n\ndiff --git a/Documentation/git-config.txt b/Documentation/git-config.txt\nindex 02ec096faa..ba76097c06 100644\n--- a/Documentation/git-config.txt\n+++ b/Documentation/git-config.txt\n@@ -14,13 +14,13 @@ SYNOPSIS\n 'git config' [] [type] --replace-all name value [value_regex]\n 'git config' [] [type] [-z|--null] --get name [value_regex]\n 'git config' [] [type] [-z|--null] --get-all name [value_regex]\n-'git config' [] [type] [-z|--null] --get-regexp name_regex [value_regex]\n+'git config' [] [type] [-z|--null] [--names-only] --get-regexp name_regex [value_regex]\n 'git config' [] [type] [-z|--null] --get-urlmatch name URL\n 'git config' [] --unset name [value_regex]\n 'git config' [] --unset-all name [value_regex]\n 'git config' [] --rename-section old_name new_name\n 'git config' [] --remove-section name\n-'git config' [] [-z|--null] -l | --list\n+'git config' [] [-z|--null] [--names-only] -l | --list\n 'git config' [] --get-color name [default]\n 'git config' [] --get-colorbool name [stdout-is-tty]\n 'git config' [] -e | --edit\n@@ -159,7 +159,7 @@ See also <>.\n \n -l::\n --list::\n-\tList all variables set in config file.\n+\tList all variables set in config file, along with their values.\n \n --bool::\n \t'git config' will ensure that the output is \"true\" or \"false\"\n@@ -190,6 +190,10 @@ See also <>.\n \toutput without getting confused e.g. by values that\n \tcontain line breaks.\n \n+--names-only::\n+\tOutput only the names of config variables for `--list` or\n+\t`--get-regexp`.\n+\n --get-colorbool name [stdout-is-tty]::\n \n \tFind the color setting for `name` (e.g. `color.diff`) and output\ndiff --git a/builtin/config.c b/builtin/config.c\nindex 7188405f7e..307980ab50 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -13,6 +13,7 @@ static char *key;\n static regex_t *key_regexp;\n static regex_t *regexp;\n static int show_keys;\n+static int omit_values;\n static int use_key_regexp;\n static int do_all;\n static int do_not_match;\n@@ -78,6 +79,7 @@ static struct option builtin_config_options[] = {\n \tOPT_BIT(0, \"path\", &types, N_(\"value is a path (file or directory name)\"), TYPE_PATH),\n \tOPT_GROUP(N_(\"Other\")),\n \tOPT_BOOL('z', \"null\", &end_null, N_(\"terminate values with NUL byte\")),\n+\tOPT_BOOL(0, \"names-only\", &omit_values, N_(\"names only\")),\n \tOPT_BOOL(0, \"includes\", &respect_includes, N_(\"respect include directives on lookup\")),\n \tOPT_END(),\n };\n@@ -91,7 +93,7 @@ static void check_argc(int argc, int min, int max) {\n \n static int show_all_config(const char *key_, const char *value_, void *cb)\n {\n-\tif (value_)\n+\tif (!omit_values && value_)\n \t\tprintf(\"%s%c%s%c\", key_, delim, value_, term);\n \telse\n \t\tprintf(\"%s%c\", key_, term);\n@@ -117,6 +119,10 @@ static int format_config(struct strbuf *buf, const char *key_, const char *value\n \t\tstrbuf_addstr(buf, key_);\n \t\tmust_print_delim = 1;\n \t}\n+\tif (omit_values) {\n+\t\tstrbuf_addch(buf, term);\n+\t\treturn 0;\n+\t}\n \tif (types == TYPE_INT)\n \t\tsprintf(value, \"%\"PRId64,\n \t\t\tgit_config_int64(key_, value_ ? value_ : \"\"));\n@@ -549,7 +555,11 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \t\tdefault:\n \t\t\tusage_with_options(builtin_config_usage, builtin_config_options);\n \t\t}\n-\n+\tif (omit_values &&\n+\t !(actions == ACTION_LIST || actions == ACTION_GET_REGEXP)) {\n+\t\terror(\"--names-only is only applicable to --list or --get-regexp\");\n+\t\tusage_with_options(builtin_config_usage, builtin_config_options);\n+\t}\n \tif (actions == ACTION_LIST) {\n \t\tcheck_argc(argc, 0, 0);\n \t\tif (git_config_with_options(show_all_config, NULL,\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex c97c648d7e..6eaab141e2 100644\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -1887,6 +1887,7 @@ _git_config ()\n \t\t\t--get --get-all --get-regexp\n \t\t\t--add --unset --unset-all\n \t\t\t--remove-section --rename-section\n+\t\t\t--names-only\n \t\t\t\"\n \t\treturn\n \t\t;;\ndiff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh\nindex 66dd28644f..97e9e76efc 100755\n--- a/t/t1300-repo-config.sh\n+++ b/t/t1300-repo-config.sh\n@@ -353,6 +353,18 @@ test_expect_success '--list without repo produces empty output' '\n '\n \n cat > expect << EOF\n+beta.noindent\n+nextsection.nonewline\n+123456.a123\n+version.1.2.3eX.alpha\n+EOF\n+\n+test_expect_success '--names-only --list' '\n+\tgit config --names-only --list >output &&\n+\ttest_cmp expect output\n+'\n+\n+cat > expect << EOF\n beta.noindent sillyValue\n nextsection.nonewline wow2 for me\n EOF\n@@ -363,6 +375,16 @@ test_expect_success '--get-regexp' '\n '\n \n cat > expect << EOF\n+beta.noindent\n+nextsection.nonewline\n+EOF\n+\n+test_expect_success '--names-only --get-regexp' '\n+\tgit config --names-only --get-regexp in >output &&\n+\ttest_cmp expect output\n+'\n+\n+cat > expect << EOF\n wow2 for me\n wow4 for you\n EOF\n-- \n2.5.0.245.gff6622b\n"},{"id":"267724","messageId":"1439199967-9655-3-git-send-email-szeder@ira.uka.de","threadId":"40043","inReplyTo":"1439199967-9655-1-git-send-email-szeder@ira.uka.de","subject":"[PATCHv3 2/2] completion: list variable names reliably with 'git config --names-only'","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2015-08-10T09:46:07Z","receivedAt":"2015-08-10T09:46:07Z","isPatch":false,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"Recenty I created a multi-line branch description with '.' and '='\ncharacters on one of the lines, and noticed that fragments of that line\nshow up when completing set variable names for 'git config', e.g.:\n\n $ git config --get branch.b.description\n Branch description to fool the completion script with a\n second line containing dot . and equals = characters.\n $ git config --unset \n ...\n second line containing dot . and equals\n ...\n\nThe completion script runs 'git config --list' and processes its output\nto strip the values and keep only the variable names. It does so by\nlooking for lines containing '.' and '=' and outputting everything\nbefore the '=', which was fooled by my multi-line branch description.\n\nA similar issue exists with aliases and pretty format aliases with\nmulti-line values, but in that case 'git config --get-regexp' is run and\nlines in its output are simply stripped after the first space, so\nsubsequent lines don't even have to contain '.' and '=' to fool the\ncompletion script.\n\nUse the new '--names-only' option added in the previous commit to list\nconfig variable names reliably in both cases, without error-prone post\nprocessing.\n\nSigned-off-by: SZEDER Gábor \n---\n contrib/completion/git-completion.bash | 15 +++------------\n 1 file changed, 3 insertions(+), 12 deletions(-)\n\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex 6eaab141e2..7200828fc4 100644\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -744,9 +744,8 @@ __git_compute_porcelain_commands ()\n __git_get_config_variables ()\n {\n \tlocal section=\"$1\" i IFS=$'\\n'\n-\tfor i in $(git --git-dir=\"$(__gitdir)\" config --get-regexp \"^$section\\..*\" 2>/dev/null); do\n-\t\ti=\"${i#$section.}\"\n-\t\techo \"${i/ */}\"\n+\tfor i in $(git --git-dir=\"$(__gitdir)\" config --names-only --get-regexp \"^$section\\..*\" 2>/dev/null); do\n+\t\techo \"${i#$section.}\"\n \tdone\n }\n \n@@ -1774,15 +1773,7 @@ __git_config_get_set_variables ()\n \t\tc=$((--c))\n \tdone\n \n-\tgit --git-dir=\"$(__gitdir)\" config $config_file --list 2>/dev/null |\n-\twhile read -r line\n-\tdo\n-\t\tcase \"$line\" in\n-\t\t*.*=*)\n-\t\t\techo \"${line/=*/}\"\n-\t\t\t;;\n-\t\tesac\n-\tdone\n+\tgit --git-dir=\"$(__gitdir)\" config $config_file --names-only --list 2>/dev/null\n }\n \n _git_config ()\n-- \n2.5.0.245.gff6622b\n"},{"id":"267758","messageId":"20150810134124.GA6763@sigill.intra.peff.net","threadId":"40043","inReplyTo":"1439199967-9655-2-git-send-email-szeder@ira.uka.de","subject":"Re: [PATCHv3 1/2] config: add '--names-only' option to list only variable names","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-08-10T13:41:24Z","receivedAt":"2015-08-10T13:41:24Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 10, 2015 at 11:46:06AM +0200, SZEDER Gábor wrote:\n\n> 'git config' can only show values or name-value pairs, so if a shell\n> script needs the names of set config variables it has to run 'git config\n> --list' or '--get-regexp' and parse the output to separate config\n> variable names from their values. However, such a parsing can't cope\n> with multi-line values. Though 'git config' can produce null-terminated\n> output for newline-safe parsing, that's of no use in such a case, becase\n> shells can't cope with null characters.\n> \n> Even our own bash completion script suffers from these issues.\n> \n> Help the completion script, and shell scripts in general, by introducing\n> the '--names-only' option to modify the output of '--list' and\n> '--get-regexp' to list only the names of config variables, so they don't\n> have to perform error-prone post processing to separate variable names\n> from their values anymore.\n\nNice. The whole thing looks very neatly done. I have only one minor nit:\nthe option \"--names-only\" is _almost_ the same as the \"--name-only\" diff\noption which is somewhat similar. Obviously they do different things and\ndo not need to match, but I wonder if it would create less annoyance to\njust give them the same name.\n\n-Peff\n"},{"id":"267760","messageId":"20150810134534.GB6763@sigill.intra.peff.net","threadId":"40043","inReplyTo":"1439199967-9655-3-git-send-email-szeder@ira.uka.de","subject":"Re: [PATCHv3 2/2] completion: list variable names reliably with 'git config --names-only'","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-08-10T13:45:34Z","receivedAt":"2015-08-10T13:45:34Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 10, 2015 at 11:46:07AM +0200, SZEDER Gábor wrote:\n\n> Use the new '--names-only' option added in the previous commit to list\n> config variable names reliably in both cases, without error-prone post\n> processing.\n> \n> Signed-off-by: SZEDER Gábor \n\nThis looks like a pretty straight-forward application of part 1, and the\nresulting code is much nicer to read.\n\nBoth patches:\n\n Acked-by: Jeff King \n\nthough I do not think my \"ack\" on completion code is worth anything. :)\n\n-Peff\n"},{"id":"267776","messageId":"xmqqvbcnuko6.fsf@gitster.dls.corp.google.com","threadId":"40043","inReplyTo":"1439199967-9655-2-git-send-email-szeder@ira.uka.de","subject":"Re: [PATCHv3 1/2] config: add '--names-only' option to list only variable names","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-10T17:18:17Z","receivedAt":"2015-08-10T17:18:17Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SZEDER Gábor writes:\n\n> 'git config' can only show values or name-value pairs, so if a shell\n> script needs the names of set config variables it has to run 'git config\n> --list' or '--get-regexp' and parse the output to separate config\n> variable names from their values. However, such a parsing can't cope\n> with multi-line values. Though 'git config' can produce null-terminated\n> output for newline-safe parsing, that's of no use in such a case, becase\n\ns/becase/because/;\n\n> shells can't cope with null characters.\n>\n> Even our own bash completion script suffers from these issues.\n>\n> Help the completion script, and shell scripts in general, by introducing\n> the '--names-only' option to modify the output of '--list' and\n> '--get-regexp' to list only the names of config variables, so they don't\n> have to perform error-prone post processing to separate variable names\n> from their values anymore.\n\nI agree with Peff that \"--names-only\" has a subtle difference with\nan existing and well known subcommand option and it would be a bit\nirritating to remember which options is for which command.\n\n> diff --git a/builtin/config.c b/builtin/config.c\n> index 7188405f7e..307980ab50 100644\n> --- a/builtin/config.c\n> +++ b/builtin/config.c\n> @@ -13,6 +13,7 @@ static char *key;\n> static regex_t *key_regexp;\n> static regex_t *regexp;\n> static int show_keys;\n> +static int omit_values;\n> static int use_key_regexp;\n> static int do_all;\n> static int do_not_match;\n> ...\n> @@ -91,7 +93,7 @@ static void check_argc(int argc, int min, int max) {\n> \n> static int show_all_config(const char *key_, const char *value_, void *cb)\n> {\n> -\tif (value_)\n> +\tif (!omit_values && value_)\n\nHmmmm. As we have \"show_keys\",\n\n\tif (show_values && value_)\n\nwould be a lot more intuitive, no?\n\n> @@ -117,6 +119,10 @@ static int format_config(struct strbuf *buf, const char *key_, const char *value\n> \t\tstrbuf_addstr(buf, key_);\n> \t\tmust_print_delim = 1;\n> \t}\n> +\tif (omit_values) {\n> +\t\tstrbuf_addch(buf, term);\n> +\t\treturn 0;\n> +\t}\n\nThis hunk makes me wonder what the assignment to \"must_print_delim\"\nis about. When the code is told to show only keys and not values,\nit shouldn't even have to worry about key_delim, but that assignment\nis done to control exactly that. It happens that you are lucky that\nyou can \"return 0\" early here so that the assignment does not have\nany effect, but still conceptually the code structure is made ugly\nby this patch.\n\nIsn't it more like the existing \"show_keys\" can be replaced/enhanced\nwith a single \"show\" tri-state toggle that chooses one among:\n\n * show both keys and values (for --list)\n * show only keys (for your new feature)\n * show only value (for --get)\n\nperhaps?\n\nI see get_urlmatch() abuses show_keys variable in a strange way, and\nit may not be as trivial as removing show_keys and replacing it with\na new tri-state toggle, though.\n"},{"id":"267964","messageId":"20150813014736.Horde.GbbN2TpIOFaNM_MPXUzZ4Q2@webmail.informatik.kit.edu","threadId":"40043","inReplyTo":"xmqqvbcnuko6.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCHv3 1/2] config: add '--names-only' option to list only variable names","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2015-08-12T23:47:36Z","receivedAt":"2015-08-12T23:47:36Z","isPatch":false,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"\nQuoting Junio C Hamano :\n\n> SZEDER Gábor writes:\n>\n>> 'git config' can only show values or name-value pairs, so if a shell\n>> script needs the names of set config variables it has to run 'git config\n>> --list' or '--get-regexp' and parse the output to separate config\n>> variable names from their values. However, such a parsing can't cope\n>> with multi-line values. Though 'git config' can produce null-terminated\n>> output for newline-safe parsing, that's of no use in such a case, becase\n>\n> s/becase/because/;\n\nOK.\n\n>> shells can't cope with null characters.\n>>\n>> Even our own bash completion script suffers from these issues.\n>>\n>> Help the completion script, and shell scripts in general, by introducing\n>> the '--names-only' option to modify the output of '--list' and\n>> '--get-regexp' to list only the names of config variables, so they don't\n>> have to perform error-prone post processing to separate variable names\n>> from their values anymore.\n>\n> I agree with Peff that \"--names-only\" has a subtle difference with\n> an existing and well known subcommand option and it would be a bit\n> irritating to remember which options is for which command.\n\nOK.\n\n>> diff --git a/builtin/config.c b/builtin/config.c\n>> index 7188405f7e..307980ab50 100644\n>> --- a/builtin/config.c\n>> +++ b/builtin/config.c\n>> @@ -13,6 +13,7 @@ static char *key;\n>> static regex_t *key_regexp;\n>> static regex_t *regexp;\n>> static int show_keys;\n>> +static int omit_values;\n>> static int use_key_regexp;\n>> static int do_all;\n>> static int do_not_match;\n>> ...\n>> @@ -91,7 +93,7 @@ static void check_argc(int argc, int min, int max) {\n>>\n>> static int show_all_config(const char *key_, const char *value_, void *cb)\n>> {\n>> -\tif (value_)\n>> +\tif (!omit_values && value_)\n>\n> Hmmmm. As we have \"show_keys\",\n>\n> \tif (show_values && value_)\n>\n> would be a lot more intuitive, no?\n\nWell, the name 'omit_values' was suggested by Peff after the first \nround. I'm happy to rename it to whatever you agree upon :)\n\n\n>> @@ -117,6 +119,10 @@ static int format_config(struct strbuf *buf, \n>> const char *key_, const char *value\n>> \t\tstrbuf_addstr(buf, key_);\n>> \t\tmust_print_delim = 1;\n>> \t}\n>> +\tif (omit_values) {\n>> +\t\tstrbuf_addch(buf, term);\n>> +\t\treturn 0;\n>> +\t}\n>\n> This hunk makes me wonder what the assignment to \"must_print_delim\"\n> is about. When the code is told to show only keys and not values,\n> it shouldn't even have to worry about key_delim, but that assignment\n> is done to control exactly that. It happens that you are lucky that\n> you can \"return 0\" early here so that the assignment does not have\n> any effect, but still conceptually the code structure is made ugly\n> by this patch.\n\nHow about restructuring the function like this? Perhaps even better \nthan a tri-state toggle would be.\n(showing the result instead of the diff, because all the indentation \nchanges make the diff hard to read).\n\nstatic int format_config(struct strbuf *buf, const char *key_, const \nchar *value_)\n{\n strbuf_init(buf, 0);\n\n if (show_keys)\n strbuf_addstr(buf, key_);\n if (!omit_values) { // or show_values\n int must_free_vptr = 0;\n int must_add_delim = show_keys;\n char value[256];\n const char *vptr = value;\n\n if (types == TYPE_INT)\n sprintf(value, \"%\"PRId64,\n git_config_int64(key_, value_ ? value_ : \"\"));\n else if (types == TYPE_BOOL)\n vptr = git_config_bool(key_, value_) ? \"true\" \n: \"false\";\n else if (types == TYPE_BOOL_OR_INT) {\n int is_bool, v;\n v = git_config_bool_or_int(key_, value_, &is_bool);\n if (is_bool)\n vptr = v ? \"true\" : \"false\";\n else\n sprintf(value, \"%d\", v);\n } else if (types == TYPE_PATH) {\n if (git_config_pathname(&vptr, key_, value_) < 0)\n return -1;\n must_free_vptr = 1;\n } else if (value_) {\n vptr = value_;\n } else {\n /* Just show the key name */\n vptr = \"\";\n must_add_delim = 0;\n }\n\n if (must_add_delim)\n strbuf_addch(buf, key_delim);\n strbuf_addstr(buf, vptr);\n\n if (must_free_vptr)\n free((char *)vptr);\n }\n strbuf_addch(buf, term);\n return 0;\n}\n\n\n>\n> Isn't it more like the existing \"show_keys\" can be replaced/enhanced\n> with a single \"show\" tri-state toggle that chooses one among:\n>\n> * show both keys and values (for --list)\n> * show only keys (for your new feature)\n> * show only value (for --get)\n>\n> perhaps?\n>\n> I see get_urlmatch() abuses show_keys variable in a strange way, and\n> it may not be as trivial as removing show_keys and replacing it with\n> a new tri-state toggle, though.\n"},{"id":"267966","messageId":"xmqqa8twj7b8.fsf@gitster.dls.corp.google.com","threadId":"40043","inReplyTo":"20150813014736.Horde.GbbN2TpIOFaNM_MPXUzZ4Q2@webmail.informatik.kit.edu","subject":"Re: [PATCHv3 1/2] config: add '--names-only' option to list only variable names","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-13T01:39:07Z","receivedAt":"2015-08-13T01:39:07Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SZEDER Gábor writes:\n\n>>\n>> s/becase/because/;\n>\n> OK.\n> ...\n>> I agree with Peff that \"--names-only\" has a subtle difference with\n>> an existing and well known subcommand option and it would be a bit\n>> irritating to remember which options is for which command.\n>\n> OK.\n> ...\n\nThe topic is now in 'next'; I think I've locally fixed it up for\nthese when I originally queued them a few days ago, so if there are\nany remaining issues, please throw incremental polishing patches.\n\nThanks.\n"},{"id":"268365","messageId":"1440080062-26811-1-git-send-email-szeder@ira.uka.de","threadId":"40043","inReplyTo":"xmqqa8twj7b8.fsf@gitster.dls.corp.google.com","subject":"[PATCH] config: restructure format_config() for better control flow","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2015-08-20T14:14:22Z","receivedAt":"2015-08-20T14:14:22Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"Commit 578625fa91 (config: add '--name-only' option to list only\nvariable names, 2015-08-10) modified format_config() such that it\nreturned from the middle of the function when showing only keys,\nresulting in ugly code structure.\n\nReorganize the if statements and dealing with the key-value delimiter to\nmake the function easier to read.\n\nSigned-off-by: SZEDER Gábor \n---\n\n> The topic is now in 'next'; I think I've locally fixed it up for\n> these when I originally queued them a few days ago, so if there are\n> any remaining issues, please throw incremental polishing patches.\n\nOK, though it's not a major issue, I think this is still worth doing on\ntop.\n\n builtin/config.c | 78 +++++++++++++++++++++++++++-----------------------------\n 1 file changed, 37 insertions(+), 41 deletions(-)\n\ndiff --git a/builtin/config.c b/builtin/config.c\nindex 631db458ec..810e104224 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -108,52 +108,48 @@ struct strbuf_list {\n \n static int format_config(struct strbuf *buf, const char *key_, const char *value_)\n {\n-\tint must_free_vptr = 0;\n-\tint must_print_delim = 0;\n-\tchar value[256];\n-\tconst char *vptr = value;\n-\n \tstrbuf_init(buf, 0);\n \n-\tif (show_keys) {\n+\tif (show_keys)\n \t\tstrbuf_addstr(buf, key_);\n-\t\tmust_print_delim = 1;\n-\t}\n-\tif (omit_values) {\n-\t\tstrbuf_addch(buf, term);\n-\t\treturn 0;\n-\t}\n-\tif (types == TYPE_INT)\n-\t\tsprintf(value, \"%\"PRId64,\n-\t\t\tgit_config_int64(key_, value_ ? value_ : \"\"));\n-\telse if (types == TYPE_BOOL)\n-\t\tvptr = git_config_bool(key_, value_) ? \"true\" : \"false\";\n-\telse if (types == TYPE_BOOL_OR_INT) {\n-\t\tint is_bool, v;\n-\t\tv = git_config_bool_or_int(key_, value_, &is_bool);\n-\t\tif (is_bool)\n-\t\t\tvptr = v ? \"true\" : \"false\";\n-\t\telse\n-\t\t\tsprintf(value, \"%d\", v);\n-\t} else if (types == TYPE_PATH) {\n-\t\tif (git_config_pathname(&vptr, key_, value_) < 0)\n-\t\t\treturn -1;\n-\t\tmust_free_vptr = 1;\n-\t} else if (value_) {\n-\t\tvptr = value_;\n-\t} else {\n-\t\t/* Just show the key name */\n-\t\tvptr = \"\";\n-\t\tmust_print_delim = 0;\n-\t}\n+\tif (!omit_values) {\n+\t\tint must_free_vptr = 0;\n+\t\tint must_add_delim = show_keys;\n+\t\tchar value[256];\n+\t\tconst char *vptr = value;\n \n-\tif (must_print_delim)\n-\t\tstrbuf_addch(buf, key_delim);\n-\tstrbuf_addstr(buf, vptr);\n+\t\tif (types == TYPE_INT)\n+\t\t\tsprintf(value, \"%\"PRId64,\n+\t\t\t\tgit_config_int64(key_, value_ ? value_ : \"\"));\n+\t\telse if (types == TYPE_BOOL)\n+\t\t\tvptr = git_config_bool(key_, value_) ? \"true\" : \"false\";\n+\t\telse if (types == TYPE_BOOL_OR_INT) {\n+\t\t\tint is_bool, v;\n+\t\t\tv = git_config_bool_or_int(key_, value_, &is_bool);\n+\t\t\tif (is_bool)\n+\t\t\t\tvptr = v ? \"true\" : \"false\";\n+\t\t\telse\n+\t\t\t\tsprintf(value, \"%d\", v);\n+\t\t} else if (types == TYPE_PATH) {\n+\t\t\tif (git_config_pathname(&vptr, key_, value_) < 0)\n+\t\t\t\treturn -1;\n+\t\t\tmust_free_vptr = 1;\n+\t\t} else if (value_) {\n+\t\t\tvptr = value_;\n+\t\t} else {\n+\t\t\t/* Just show the key name */\n+\t\t\tvptr = \"\";\n+\t\t\tmust_add_delim = 0;\n+\t\t}\n+\n+\t\tif (must_add_delim)\n+\t\t\tstrbuf_addch(buf, key_delim);\n+\t\tstrbuf_addstr(buf, vptr);\n+\n+\t\tif (must_free_vptr)\n+\t\t\tfree((char *)vptr);\n+\t}\n \tstrbuf_addch(buf, term);\n-\n-\tif (must_free_vptr)\n-\t\tfree((char *)vptr);\n \treturn 0;\n }\n \n-- \n2.5.0.415.g33d64ef\n"},{"id":"268366","messageId":"20150820144504.GA22935@sigill.intra.peff.net","threadId":"40043","inReplyTo":"1440080062-26811-1-git-send-email-szeder@ira.uka.de","subject":"Re: [PATCH] config: restructure format_config() for better control flow","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-08-20T14:45:04Z","receivedAt":"2015-08-20T14:45:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 20, 2015 at 04:14:22PM +0200, SZEDER Gábor wrote:\n\n> Commit 578625fa91 (config: add '--name-only' option to list only\n> variable names, 2015-08-10) modified format_config() such that it\n> returned from the middle of the function when showing only keys,\n> resulting in ugly code structure.\n> \n> Reorganize the if statements and dealing with the key-value delimiter to\n> make the function easier to read.\n\nThis looks good to me. What do you think of these on top?\n\n [1/3]: format_config: don't init strbuf\n [2/3]: format_config: simplify buffer handling\n [3/3]: get_urlmatch: avoid useless strbuf write\n\n-Peff\n"},{"id":"268369","messageId":"20150820144604.GA11913@sigill.intra.peff.net","threadId":"40043","inReplyTo":"20150820144504.GA22935@sigill.intra.peff.net","subject":"[PATCH 1/3] format_config: don't init strbuf","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-08-20T14:46:04Z","receivedAt":"2015-08-20T14:46:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"It's unusual for a function which writes to a passed-in\nstrbuf to call strbuf_init; that will throw away anything\nalready there, leaking memory. In this case, there are\nexactly two callers; one relies on this initialization and\nthe other passes in an already-initialized buffer.\n\nThere's no leak, as the initialized buffer doesn't have\nanything in it. But let's bump the strbuf_init out to the\none caller who needs it, making format_config more\nidiomatic.\n\nSigned-off-by: Jeff King \n---\n builtin/config.c | 3 +--\n 1 file changed, 1 insertion(+), 2 deletions(-)\n\ndiff --git a/builtin/config.c b/builtin/config.c\nindex 810e104..91aa56f 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -108,8 +108,6 @@ struct strbuf_list {\n \n static int format_config(struct strbuf *buf, const char *key_, const char *value_)\n {\n-\tstrbuf_init(buf, 0);\n-\n \tif (show_keys)\n \t\tstrbuf_addstr(buf, key_);\n \tif (!omit_values) {\n@@ -166,6 +164,7 @@ static int collect_config(const char *key_, const char *value_, void *cb)\n \t\treturn 0;\n \n \tALLOC_GROW(values->items, values->nr + 1, values->alloc);\n+\tstrbuf_init(&values->items[values->nr], 0);\n \n \treturn format_config(&values->items[values->nr++], key_, value_);\n }\n-- \n2.5.0.680.g69e7703\n"},{"id":"268367","messageId":"20150820144733.GB11913@sigill.intra.peff.net","threadId":"40043","inReplyTo":"20150820144504.GA22935@sigill.intra.peff.net","subject":"[PATCH 2/3] format_config: simplify buffer handling","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-08-20T14:47:34Z","receivedAt":"2015-08-20T14:47:34Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When formatting a config value into a strbuf, we may end\nup stringifying it into a fixed-size buffer using sprintf,\nand then copying that buffer into the strbuf. We can\neliminate the middle-man (and drop some calls to sprintf!)\nby writing directly to the strbuf.\n\nThe reason it was written this way in the first place is\nthat we need to know before writing the value whether to\ninsert a delimiter. Instead of delaying the write of the\nvalue, we speculatively write the delimiter, and roll it\nback in the single case that cares.\n\nSigned-off-by: Jeff King \n---\nI admit the rollback is a little gross. The other option would be adding\nthe delimiter in each of the conditional branches, which is also kind of\nnasty. Or we could leave the buffer-write as-is, and replace the\nsprintfs with snprintfs (this is actually something I was doing in\nanother branch, which is why I took particular notice; your patch\nconflicts with my to-be-submitted branch :) ).\n\n builtin/config.c | 38 ++++++++++++++++----------------------\n 1 file changed, 16 insertions(+), 22 deletions(-)\n\ndiff --git a/builtin/config.c b/builtin/config.c\nindex 91aa56f..04befce 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -111,41 +111,35 @@ static int format_config(struct strbuf *buf, const char *key_, const char *value\n \tif (show_keys)\n \t\tstrbuf_addstr(buf, key_);\n \tif (!omit_values) {\n-\t\tint must_free_vptr = 0;\n-\t\tint must_add_delim = show_keys;\n-\t\tchar value[256];\n-\t\tconst char *vptr = value;\n+\t\tif (show_keys)\n+\t\t\tstrbuf_addch(buf, key_delim);\n \n \t\tif (types == TYPE_INT)\n-\t\t\tsprintf(value, \"%\"PRId64,\n-\t\t\t\tgit_config_int64(key_, value_ ? value_ : \"\"));\n+\t\t\tstrbuf_addf(buf, \"%\"PRId64,\n+\t\t\t\t git_config_int64(key_, value_ ? value_ : \"\"));\n \t\telse if (types == TYPE_BOOL)\n-\t\t\tvptr = git_config_bool(key_, value_) ? \"true\" : \"false\";\n+\t\t\tstrbuf_addstr(buf, git_config_bool(key_, value_) ?\n+\t\t\t\t \"true\" : \"false\");\n \t\telse if (types == TYPE_BOOL_OR_INT) {\n \t\t\tint is_bool, v;\n \t\t\tv = git_config_bool_or_int(key_, value_, &is_bool);\n \t\t\tif (is_bool)\n-\t\t\t\tvptr = v ? \"true\" : \"false\";\n+\t\t\t\tstrbuf_addstr(buf, v ? \"true\" : \"false\");\n \t\t\telse\n-\t\t\t\tsprintf(value, \"%d\", v);\n+\t\t\t\tstrbuf_addf(buf, \"%d\", v);\n \t\t} else if (types == TYPE_PATH) {\n-\t\t\tif (git_config_pathname(&vptr, key_, value_) < 0)\n+\t\t\tconst char *v;\n+\t\t\tif (git_config_pathname(&v, key_, value_) < 0)\n \t\t\t\treturn -1;\n-\t\t\tmust_free_vptr = 1;\n+\t\t\tstrbuf_addstr(buf, v);\n+\t\t\tfree((char *)v);\n \t\t} else if (value_) {\n-\t\t\tvptr = value_;\n+\t\t\tstrbuf_addstr(buf, value_);\n \t\t} else {\n-\t\t\t/* Just show the key name */\n-\t\t\tvptr = \"\";\n-\t\t\tmust_add_delim = 0;\n+\t\t\t/* Just show the key name; back out delimiter */\n+\t\t\tif (show_keys)\n+\t\t\t\tstrbuf_setlen(buf, buf->len - 1);\n \t\t}\n-\n-\t\tif (must_add_delim)\n-\t\t\tstrbuf_addch(buf, key_delim);\n-\t\tstrbuf_addstr(buf, vptr);\n-\n-\t\tif (must_free_vptr)\n-\t\t\tfree((char *)vptr);\n \t}\n \tstrbuf_addch(buf, term);\n \treturn 0;\n-- \n2.5.0.680.g69e7703\n"},{"id":"268368","messageId":"20150820144945.GC11913@sigill.intra.peff.net","threadId":"40043","inReplyTo":"20150820144504.GA22935@sigill.intra.peff.net","subject":"[PATCH 3/3] get_urlmatch: avoid useless strbuf write","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-08-20T14:49:45Z","receivedAt":"2015-08-20T14:49:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We create a strbuf only to insert a single string, pass the\nresulting buffer to a function (which does not modify the\nstring), and then free it. We can just pass the original\nstring instead.\n\nSigned-off-by: Jeff King \n---\nI keep staring at this thinking I missed something, but I think this is\nan equivalent transformation. I wonder if the original meant to modify\nthe key in some way, but I don't see how or why.\n\n builtin/config.c | 5 +----\n 1 file changed, 1 insertion(+), 4 deletions(-)\n\ndiff --git a/builtin/config.c b/builtin/config.c\nindex 04befce..71acc44 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -425,14 +425,11 @@ static int get_urlmatch(const char *var, const char *url)\n \n \tfor_each_string_list_item(item, &values) {\n \t\tstruct urlmatch_current_candidate_value *matched = item->util;\n-\t\tstruct strbuf key = STRBUF_INIT;\n \t\tstruct strbuf buf = STRBUF_INIT;\n \n-\t\tstrbuf_addstr(&key, item->string);\n-\t\tformat_config(&buf, key.buf,\n+\t\tformat_config(&buf, item->string,\n \t\t\t matched->value_is_null ? NULL : matched->value.buf);\n \t\tfwrite(buf.buf, 1, buf.len, stdout);\n-\t\tstrbuf_release(&key);\n \t\tstrbuf_release(&buf);\n \n \t\tstrbuf_release(&matched->value);\n-- \n2.5.0.680.g69e7703\n"},{"id":"268386","messageId":"xmqq1tex7lwd.fsf@gitster.dls.corp.google.com","threadId":"40043","inReplyTo":"20150820144504.GA22935@sigill.intra.peff.net","subject":"Re: [PATCH] config: restructure format_config() for better control flow","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-20T20:19:46Z","receivedAt":"2015-08-20T20:19:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Thu, Aug 20, 2015 at 04:14:22PM +0200, SZEDER Gábor wrote:\n>\n>> Commit 578625fa91 (config: add '--name-only' option to list only\n>> variable names, 2015-08-10) modified format_config() such that it\n>> returned from the middle of the function when showing only keys,\n>> resulting in ugly code structure.\n>> \n>> Reorganize the if statements and dealing with the key-value delimiter to\n>> make the function easier to read.\n>\n> This looks good to me. What do you think of these on top?\n>\n> [1/3]: format_config: don't init strbuf\n> [2/3]: format_config: simplify buffer handling\n> [3/3]: get_urlmatch: avoid useless strbuf write\n\nAll four including the thread-starter by SZEDER look good to me.\nThanks.\n"},{"id":"268410","messageId":"20150821135254.Horde.IAT_zjvSJGvMRd5F_DtITQ1@webmail.informatik.kit.edu","threadId":"40043","inReplyTo":"20150820144733.GB11913@sigill.intra.peff.net","subject":"Re: [PATCH 2/3] format_config: simplify buffer handling","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2015-08-21T11:52:54Z","receivedAt":"2015-08-21T11:52:54Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"\nQuoting Jeff King :\n\n> When formatting a config value into a strbuf, we may end\n> up stringifying it into a fixed-size buffer using sprintf,\n> and then copying that buffer into the strbuf. We can\n> eliminate the middle-man (and drop some calls to sprintf!)\n> by writing directly to the strbuf.\n>\n> The reason it was written this way in the first place is\n> that we need to know before writing the value whether to\n> insert a delimiter. Instead of delaying the write of the\n> value, we speculatively write the delimiter, and roll it\n> back in the single case that cares.\n>\n> Signed-off-by: Jeff King \n> ---\n> I admit the rollback is a little gross.\n\nIndeed it is, but I'm for it, as it gets rit of so much more\nother grossness, i.e. the fixed-size buffer and vptr stuff\nand the two must_do_this variables.\n\n\n> builtin/config.c | 38 ++++++++++++++++----------------------\n> 1 file changed, 16 insertions(+), 22 deletions(-)\n>\n> diff --git a/builtin/config.c b/builtin/config.c\n> index 91aa56f..04befce 100644\n> --- a/builtin/config.c\n> +++ b/builtin/config.c\n> @@ -111,41 +111,35 @@ static int format_config(struct strbuf *buf, \n> const char *key_, const char *value\n> \tif (show_keys)\n> \t\tstrbuf_addstr(buf, key_);\n> \tif (!omit_values) {\n> -\t\tint must_free_vptr = 0;\n> -\t\tint must_add_delim = show_keys;\n> -\t\tchar value[256];\n> -\t\tconst char *vptr = value;\n> +\t\tif (show_keys)\n> +\t\t\tstrbuf_addch(buf, key_delim);\n>\n> \t\tif (types == TYPE_INT)\n> -\t\t\tsprintf(value, \"%\"PRId64,\n> -\t\t\t\tgit_config_int64(key_, value_ ? value_ : \"\"));\n> +\t\t\tstrbuf_addf(buf, \"%\"PRId64,\n> +\t\t\t\t git_config_int64(key_, value_ ? value_ : \"\"));\n> \t\telse if (types == TYPE_BOOL)\n> -\t\t\tvptr = git_config_bool(key_, value_) ? \"true\" : \"false\";\n> +\t\t\tstrbuf_addstr(buf, git_config_bool(key_, value_) ?\n> +\t\t\t\t \"true\" : \"false\");\n> \t\telse if (types == TYPE_BOOL_OR_INT) {\n> \t\t\tint is_bool, v;\n> \t\t\tv = git_config_bool_or_int(key_, value_, &is_bool);\n> \t\t\tif (is_bool)\n> -\t\t\t\tvptr = v ? \"true\" : \"false\";\n> +\t\t\t\tstrbuf_addstr(buf, v ? \"true\" : \"false\");\n> \t\t\telse\n> -\t\t\t\tsprintf(value, \"%d\", v);\n> +\t\t\t\tstrbuf_addf(buf, \"%d\", v);\n> \t\t} else if (types == TYPE_PATH) {\n> -\t\t\tif (git_config_pathname(&vptr, key_, value_) < 0)\n> +\t\t\tconst char *v;\n> +\t\t\tif (git_config_pathname(&v, key_, value_) < 0)\n> \t\t\t\treturn -1;\n> -\t\t\tmust_free_vptr = 1;\n> +\t\t\tstrbuf_addstr(buf, v);\n> +\t\t\tfree((char *)v);\n> \t\t} else if (value_) {\n> -\t\t\tvptr = value_;\n> +\t\t\tstrbuf_addstr(buf, value_);\n> \t\t} else {\n> -\t\t\t/* Just show the key name */\n> -\t\t\tvptr = \"\";\n> -\t\t\tmust_add_delim = 0;\n> +\t\t\t/* Just show the key name; back out delimiter */\n> +\t\t\tif (show_keys)\n> +\t\t\t\tstrbuf_setlen(buf, buf->len - 1);\n> \t\t}\n> -\n> -\t\tif (must_add_delim)\n> -\t\t\tstrbuf_addch(buf, key_delim);\n> -\t\tstrbuf_addstr(buf, vptr);\n> -\n> -\t\tif (must_free_vptr)\n> -\t\t\tfree((char *)vptr);\n> \t}\n> \tstrbuf_addch(buf, term);\n> \treturn 0;\n> --\n> 2.5.0.680.g69e7703\n"},{"id":"268438","messageId":"xmqqvbc85yi3.fsf@gitster.dls.corp.google.com","threadId":"40043","inReplyTo":"20150820144733.GB11913@sigill.intra.peff.net","subject":"Re: [PATCH 2/3] format_config: simplify buffer handling","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-21T17:42:44Z","receivedAt":"2015-08-21T17:42:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> When formatting a config value into a strbuf, we may end\n> up stringifying it into a fixed-size buffer using sprintf,\n> and then copying that buffer into the strbuf. We can\n> eliminate the middle-man (and drop some calls to sprintf!)\n> by writing directly to the strbuf.\n>\n> The reason it was written this way in the first place is\n> that we need to know before writing the value whether to\n> insert a delimiter. Instead of delaying the write of the\n> value, we speculatively write the delimiter, and roll it\n> back in the single case that cares.\n>\n> Signed-off-by: Jeff King \n> ---\n> I admit the rollback is a little gross. The other option would be adding\n> the delimiter in each of the conditional branches, which is also kind of\n> nasty.\n\nI actually am fine with this rollback. The \"variable alone stands\nfor true\" is not something a user can produce from the command line\nvery easily, so having to rollback is a rare event anyway.\n\nI wonder if we can do this instead\n\n\tif (!omit_values) {\n-\t\tif (show_keys)\n+\t\tif (show_keys && value_)\n\t\t\tstrbuf_addch(buf, key_delim);\n\nthough. That would eliminate the need for rolling back.\n\nI briefly wondered how such a change would interact with\n\n if (types == TYPE_INT)\n strbuf_addf(buf, \"%\"PRId64,\n git_config_int64(key_, value_ ? value_ : \"\"));\n\nthat immediately follows it, but this \"turn NULL into an empty\nstring\" may be bogus in the first place, in the sense that\ngit_config_int64() should complain about a NULL value_ the same way\nas it would complain about an empty string---both them are not an\ninteger. And indeed:\n\n - git_parse_int64() that is called from git_config_int64() is\n prepared to take both \"\" and NULL and return failure with EINVAL;\n\n - die_bad_number() that is eventually called when parsing fails by\n git_config_int64() is prepared to take NULL and turns it to an\n empty string.\n\nSo perhaps we could do this squashed in?\n\n builtin/config.c | 9 ++++-----\n 1 file changed, 4 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/config.c b/builtin/config.c\nindex 71acc44..593b1ae 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -111,12 +111,12 @@ static int format_config(struct strbuf *buf, const char *key_, const char *value\n \tif (show_keys)\n \t\tstrbuf_addstr(buf, key_);\n \tif (!omit_values) {\n-\t\tif (show_keys)\n+\t\tif (show_keys && value_)\n \t\t\tstrbuf_addch(buf, key_delim);\n \n \t\tif (types == TYPE_INT)\n \t\t\tstrbuf_addf(buf, \"%\"PRId64,\n-\t\t\t\t git_config_int64(key_, value_ ? value_ : \"\"));\n+\t\t\t\t git_config_int64(key_, value_));\n \t\telse if (types == TYPE_BOOL)\n \t\t\tstrbuf_addstr(buf, git_config_bool(key_, value_) ?\n \t\t\t\t \"true\" : \"false\");\n@@ -136,9 +136,8 @@ static int format_config(struct strbuf *buf, const char *key_, const char *value\n \t\t} else if (value_) {\n \t\t\tstrbuf_addstr(buf, value_);\n \t\t} else {\n-\t\t\t/* Just show the key name; back out delimiter */\n-\t\t\tif (show_keys)\n-\t\t\t\tstrbuf_setlen(buf, buf->len - 1);\n+\t\t\t/* Just show the key name */\n+\t\t\t;\n \t\t}\n \t}\n \tstrbuf_addch(buf, term);\n"},{"id":"268439","messageId":"xmqqr3mw5yg1.fsf@gitster.dls.corp.google.com","threadId":"40043","inReplyTo":"xmqqvbc85yi3.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 2/3] format_config: simplify buffer handling","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-21T17:43:58Z","receivedAt":"2015-08-21T17:43:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> I wonder if we can do this instead\n>\n> \tif (!omit_values) {\n> -\t\tif (show_keys)\n> +\t\tif (show_keys && value_)\n> \t\t\tstrbuf_addch(buf, key_delim);\n>\n> though. That would eliminate the need for rolling back.\n\nNo we cannot. \"config --bool --get-regexp\" could get a NULL value_\nand has to turn it to \" true\".\n\nSorry for the noise.\n"},{"id":"268444","messageId":"20150821194001.GA26466@sigill.intra.peff.net","threadId":"40043","inReplyTo":"xmqqr3mw5yg1.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 2/3] format_config: simplify buffer handling","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-08-21T19:40:01Z","receivedAt":"2015-08-21T19:40:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Aug 21, 2015 at 10:43:58AM -0700, Junio C Hamano wrote:\n\n> Junio C Hamano writes:\n> \n> > I wonder if we can do this instead\n> >\n> > \tif (!omit_values) {\n> > -\t\tif (show_keys)\n> > +\t\tif (show_keys && value_)\n> > \t\t\tstrbuf_addch(buf, key_delim);\n> >\n> > though. That would eliminate the need for rolling back.\n> \n> No we cannot. \"config --bool --get-regexp\" could get a NULL value_\n> and has to turn it to \" true\".\n> \n> Sorry for the noise.\n\nRight, I had the same thought and reached the same conclusion.\n\nBy the way, I do not think the rollback by itself is gross, it is just\nthat it has to reproduce the \"show_keys\" logic. That is, it _really_\nwants to be:\n\n else {\n\t /* nothing to show; rollback delim */\n\t if (we_added_delim)\n\t\t strbuf_setlen(buf, buf->len - 1);\n }\n\nand I use \"show_keys\" as a proxy for \"did we add it\". Which is\nreproducing the logic that you quoted above, and if the two ever get out\nof sync, it will be a bug. So you could write it as:\n\n int we_added_delim = 0;\n if (show_keys) {\n\tstrbuf_addch(buf, key_delim);\n\twe_added_delim = 1;\n }\n\nI didn't, because it's ugly, and the function is short enough and\nunlikely enough to change that it probably won't matter.\n\nYou could perhaps also write it as:\n\n size_t baselen = buf->len;\n if (show_keys)\n\tstrbuf_addch(buf, key_delim);\n ...\n else {\n\t/* rollback delim */\n\tstrbuf_setlen(buf, baselen);\n }\n\n-Peff\n"}]}