* [PATCHv3 0/2] 'git config --names-only' to help the completion script @ 2015-08-10 9:46 SZEDER Gábor 2015-08-10 9:46 ` [PATCHv3 1/2] config: add '--names-only' option to list only variable names SZEDER Gábor 2015-08-10 9:46 ` [PATCHv3 2/2] completion: list variable names reliably with 'git config --names-only' SZEDER Gábor 0 siblings, 2 replies; 18+ messages in thread From: SZEDER Gábor @ 2015-08-10 9:46 UTC (permalink / raw) To: Junio C Hamano; +Cc: Jeff King, Christian Couder, git, 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCHv3 1/2] config: add '--names-only' option to list only variable names 2015-08-10 9:46 [PATCHv3 0/2] 'git config --names-only' to help the completion script SZEDER Gábor @ 2015-08-10 9:46 ` SZEDER Gábor 2015-08-10 13:41 ` Jeff King 2015-08-10 17:18 ` Junio C Hamano 2015-08-10 9:46 ` [PATCHv3 2/2] completion: list variable names reliably with 'git config --names-only' SZEDER Gábor 1 sibling, 2 replies; 18+ messages in thread From: SZEDER Gábor @ 2015-08-10 9:46 UTC (permalink / raw) To: Junio C Hamano; +Cc: Jeff King, Christian Couder, git, 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 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCHv3 1/2] config: add '--names-only' option to list only variable names 2015-08-10 9:46 ` [PATCHv3 1/2] config: add '--names-only' option to list only variable names SZEDER Gábor @ 2015-08-10 13:41 ` Jeff King 2015-08-10 17:18 ` Junio C Hamano 1 sibling, 0 replies; 18+ messages in thread From: Jeff King @ 2015-08-10 13:41 UTC (permalink / raw) To: SZEDER Gábor; +Cc: Junio C Hamano, Christian Couder, git 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCHv3 1/2] config: add '--names-only' option to list only variable names 2015-08-10 9:46 ` [PATCHv3 1/2] config: add '--names-only' option to list only variable names SZEDER Gábor 2015-08-10 13:41 ` Jeff King @ 2015-08-10 17:18 ` Junio C Hamano 2015-08-12 23:47 ` SZEDER Gábor 1 sibling, 1 reply; 18+ messages in thread From: Junio C Hamano @ 2015-08-10 17:18 UTC (permalink / raw) To: SZEDER Gábor; +Cc: Jeff King, Christian Couder, git 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. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCHv3 1/2] config: add '--names-only' option to list only variable names 2015-08-10 17:18 ` Junio C Hamano @ 2015-08-12 23:47 ` SZEDER Gábor 2015-08-13 1:39 ` Junio C Hamano 0 siblings, 1 reply; 18+ messages in thread From: SZEDER Gábor @ 2015-08-12 23:47 UTC (permalink / raw) To: Junio C Hamano; +Cc: Jeff King, Christian Couder, git 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. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCHv3 1/2] config: add '--names-only' option to list only variable names 2015-08-12 23:47 ` SZEDER Gábor @ 2015-08-13 1:39 ` Junio C Hamano 2015-08-20 14:14 ` [PATCH] config: restructure format_config() for better control flow SZEDER Gábor 0 siblings, 1 reply; 18+ messages in thread From: Junio C Hamano @ 2015-08-13 1:39 UTC (permalink / raw) To: SZEDER Gábor; +Cc: Jeff King, Christian Couder, git 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. ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH] config: restructure format_config() for better control flow 2015-08-13 1:39 ` Junio C Hamano @ 2015-08-20 14:14 ` SZEDER Gábor 2015-08-20 14:45 ` Jeff King 0 siblings, 1 reply; 18+ messages in thread From: SZEDER Gábor @ 2015-08-20 14:14 UTC (permalink / raw) To: Junio C Hamano; +Cc: Jeff King, Christian Couder, git, 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 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH] config: restructure format_config() for better control flow 2015-08-20 14:14 ` [PATCH] config: restructure format_config() for better control flow SZEDER Gábor @ 2015-08-20 14:45 ` Jeff King 2015-08-20 14:46 ` [PATCH 1/3] format_config: don't init strbuf Jeff King ` (3 more replies) 0 siblings, 4 replies; 18+ messages in thread From: Jeff King @ 2015-08-20 14:45 UTC (permalink / raw) To: SZEDER Gábor; +Cc: Junio C Hamano, Christian Couder, git 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 1/3] format_config: don't init strbuf 2015-08-20 14:45 ` Jeff King @ 2015-08-20 14:46 ` Jeff King 2015-08-20 14:47 ` [PATCH 2/3] format_config: simplify buffer handling Jeff King ` (2 subsequent siblings) 3 siblings, 0 replies; 18+ messages in thread From: Jeff King @ 2015-08-20 14:46 UTC (permalink / raw) To: SZEDER Gábor; +Cc: Junio C Hamano, Christian Couder, git 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 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 2/3] format_config: simplify buffer handling 2015-08-20 14:45 ` Jeff King 2015-08-20 14:46 ` [PATCH 1/3] format_config: don't init strbuf Jeff King @ 2015-08-20 14:47 ` Jeff King 2015-08-21 11:52 ` SZEDER Gábor 2015-08-21 17:42 ` Junio C Hamano 2015-08-20 14:49 ` [PATCH 3/3] get_urlmatch: avoid useless strbuf write Jeff King 2015-08-20 20:19 ` [PATCH] config: restructure format_config() for better control flow Junio C Hamano 3 siblings, 2 replies; 18+ messages in thread From: Jeff King @ 2015-08-20 14:47 UTC (permalink / raw) To: SZEDER Gábor; +Cc: Junio C Hamano, Christian Couder, git 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 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 2/3] format_config: simplify buffer handling 2015-08-20 14:47 ` [PATCH 2/3] format_config: simplify buffer handling Jeff King @ 2015-08-21 11:52 ` SZEDER Gábor 2015-08-21 17:42 ` Junio C Hamano 1 sibling, 0 replies; 18+ messages in thread From: SZEDER Gábor @ 2015-08-21 11:52 UTC (permalink / raw) To: Jeff King; +Cc: Junio C Hamano, Christian Couder, git 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/3] format_config: simplify buffer handling 2015-08-20 14:47 ` [PATCH 2/3] format_config: simplify buffer handling Jeff King 2015-08-21 11:52 ` SZEDER Gábor @ 2015-08-21 17:42 ` Junio C Hamano 2015-08-21 17:43 ` Junio C Hamano 1 sibling, 1 reply; 18+ messages in thread From: Junio C Hamano @ 2015-08-21 17:42 UTC (permalink / raw) To: Jeff King; +Cc: SZEDER Gábor, Christian Couder, git 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); ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 2/3] format_config: simplify buffer handling 2015-08-21 17:42 ` Junio C Hamano @ 2015-08-21 17:43 ` Junio C Hamano 2015-08-21 19:40 ` Jeff King 0 siblings, 1 reply; 18+ messages in thread From: Junio C Hamano @ 2015-08-21 17:43 UTC (permalink / raw) To: Jeff King; +Cc: SZEDER Gábor, Christian Couder, git 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. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/3] format_config: simplify buffer handling 2015-08-21 17:43 ` Junio C Hamano @ 2015-08-21 19:40 ` Jeff King 0 siblings, 0 replies; 18+ messages in thread From: Jeff King @ 2015-08-21 19:40 UTC (permalink / raw) To: Junio C Hamano; +Cc: SZEDER Gábor, Christian Couder, git 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 3/3] get_urlmatch: avoid useless strbuf write 2015-08-20 14:45 ` Jeff King 2015-08-20 14:46 ` [PATCH 1/3] format_config: don't init strbuf Jeff King 2015-08-20 14:47 ` [PATCH 2/3] format_config: simplify buffer handling Jeff King @ 2015-08-20 14:49 ` Jeff King 2015-08-20 20:19 ` [PATCH] config: restructure format_config() for better control flow Junio C Hamano 3 siblings, 0 replies; 18+ messages in thread From: Jeff King @ 2015-08-20 14:49 UTC (permalink / raw) To: SZEDER Gábor; +Cc: Junio C Hamano, Christian Couder, git 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 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH] config: restructure format_config() for better control flow 2015-08-20 14:45 ` Jeff King ` (2 preceding siblings ...) 2015-08-20 14:49 ` [PATCH 3/3] get_urlmatch: avoid useless strbuf write Jeff King @ 2015-08-20 20:19 ` Junio C Hamano 3 siblings, 0 replies; 18+ messages in thread From: Junio C Hamano @ 2015-08-20 20:19 UTC (permalink / raw) To: Jeff King; +Cc: SZEDER Gábor, Christian Couder, git 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. ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCHv3 2/2] completion: list variable names reliably with 'git config --names-only' 2015-08-10 9:46 [PATCHv3 0/2] 'git config --names-only' to help the completion script SZEDER Gábor 2015-08-10 9:46 ` [PATCHv3 1/2] config: add '--names-only' option to list only variable names SZEDER Gábor @ 2015-08-10 9:46 ` SZEDER Gábor 2015-08-10 13:45 ` Jeff King 1 sibling, 1 reply; 18+ messages in thread From: SZEDER Gábor @ 2015-08-10 9:46 UTC (permalink / raw) To: Junio C Hamano; +Cc: Jeff King, Christian Couder, git, 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 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCHv3 2/2] completion: list variable names reliably with 'git config --names-only' 2015-08-10 9:46 ` [PATCHv3 2/2] completion: list variable names reliably with 'git config --names-only' SZEDER Gábor @ 2015-08-10 13:45 ` Jeff King 0 siblings, 0 replies; 18+ messages in thread From: Jeff King @ 2015-08-10 13:45 UTC (permalink / raw) To: SZEDER Gábor; +Cc: Junio C Hamano, Christian Couder, git 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2015-08-21 19:40 UTC | newest] Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-08-10 9:46 [PATCHv3 0/2] 'git config --names-only' to help the completion script SZEDER Gábor 2015-08-10 9:46 ` [PATCHv3 1/2] config: add '--names-only' option to list only variable names SZEDER Gábor 2015-08-10 13:41 ` Jeff King 2015-08-10 17:18 ` Junio C Hamano 2015-08-12 23:47 ` SZEDER Gábor 2015-08-13 1:39 ` Junio C Hamano 2015-08-20 14:14 ` [PATCH] config: restructure format_config() for better control flow SZEDER Gábor 2015-08-20 14:45 ` Jeff King 2015-08-20 14:46 ` [PATCH 1/3] format_config: don't init strbuf Jeff King 2015-08-20 14:47 ` [PATCH 2/3] format_config: simplify buffer handling Jeff King 2015-08-21 11:52 ` SZEDER Gábor 2015-08-21 17:42 ` Junio C Hamano 2015-08-21 17:43 ` Junio C Hamano 2015-08-21 19:40 ` Jeff King 2015-08-20 14:49 ` [PATCH 3/3] get_urlmatch: avoid useless strbuf write Jeff King 2015-08-20 20:19 ` [PATCH] config: restructure format_config() for better control flow Junio C Hamano 2015-08-10 9:46 ` [PATCHv3 2/2] completion: list variable names reliably with 'git config --names-only' SZEDER Gábor 2015-08-10 13:45 ` Jeff King
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).