# [PATCH] repo-config: support --get-regexp and fix crash 5 messages from 2006-05-02 to 2006-05-03. Participants: Johannes Schindelin, Junio C Hamano. Thread: https://gitlist.dev/t/4023 ## Johannes Schindelin, 2006-05-02 12:22 Subject: [PATCH] repo-config: support --get-regexp and fix crash Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.63.0605021422150.7051%40wbgn013.biozentrum.uni-wuerzburg.de ``` With --get-regexp, output all key/value pairs where the key matches a regexp. Example: git-repo-config --get-regexp remote.*.url will output something like remote.junio.url git://git.kernel.org/pub/scm/git/git.git remote.gitk.url git://git.kernel.org/pub/scm/gitk/gitk.git This also fixes a crash when no arguments are given, which was introduced with the "-l" and "--list" handling. Signed-off-by: Johannes Schindelin --- Junio made me aware of a crash, a fix for which was too easy to merit a separate patch. Strange thing I realized: A value is white-space-trimmed at the end only if the line does not end with a comment. This fact is accounted for in the new tests. Documentation/git-repo-config.txt | 5 +++- repo-config.c | 45 +++++++++++++++++++++++++++++-------- t/t1300-repo-config.sh | 23 +++++++++++++++++++ 3 files changed, 62 insertions(+), 11 deletions(-) diff --git a/Documentation/git-repo-config.txt b/Documentation/git-repo-config.txt index 566cfa1..ddcf523 100644 --- a/Documentation/git-repo-config.txt +++ b/Documentation/git-repo-config.txt @@ -49,7 +49,7 @@ OPTIONS --replace-all:: Default behaviour is to replace at most one line. This replaces - all lines matching the key (and optionally the value_regex) + all lines matching the key (and optionally the value_regex). --get:: Get the value for a given key (optionally filtered by a regex @@ -59,6 +59,9 @@ OPTIONS Like get, but does not fail if the number of values for the key is not exactly one. +--get-regexp:: + Like --get-all, but interprets the name as a regular expression. + --unset:: Remove the line matching the key from .git/config. diff --git a/repo-config.c b/repo-config.c index fa8aba7..722153c 100644 --- a/repo-config.c +++ b/repo-config.c @@ -6,7 +6,10 @@ static const char git_config_set_usage[] static char* key = NULL; static char* value = NULL; +static regex_t* key_regexp = NULL; static regex_t* regexp = NULL; +static int show_keys = 0; +static int use_key_regexp = 0; static int do_all = 0; static int do_not_match = 0; static int seen = 0; @@ -26,16 +29,18 @@ static int show_config(const char* key_, if (value_ == NULL) value_ = ""; - if (!strcmp(key_, key) && + if ((use_key_regexp || !strcmp(key_, key)) && + (!use_key_regexp || + !regexec(key_regexp, key_, 0, NULL, 0)) && (regexp == NULL || (do_not_match ^ !regexec(regexp, value_, 0, NULL, 0)))) { - if (do_all) { - printf("%s\n", value_); - return 0; - } + if (show_keys) + printf("%s ", key_); if (seen > 0) { - fprintf(stderr, "More than one value: %s\n", value); + if (!do_all) + fprintf(stderr, "More than one value: %s\n", + value); free(value); } @@ -50,6 +55,8 @@ static int show_config(const char* key_, value = strdup(value_); } seen++; + if (do_all) + printf("%s\n", value); } return 0; } @@ -63,6 +70,14 @@ static int get_value(const char* key_, c key[i] = tolower(key_[i]); key[i] = 0; + if (use_key_regexp) { + key_regexp = (regex_t*)malloc(sizeof(regex_t)); + if (regcomp(key_regexp, key, REG_EXTENDED)) { + fprintf(stderr, "Invalid key pattern: %s\n", regex_); + return -1; + } + } + if (regex_) { if (regex_[0] == '!') { do_not_match = 1; @@ -78,7 +93,8 @@ static int get_value(const char* key_, c git_config(show_config); if (value) { - printf("%s\n", value); + if (!do_all) + printf("%s\n", value); free(value); } free(key); @@ -102,15 +118,14 @@ int main(int argc, const char **argv) type = T_INT; else if (!strcmp(argv[1], "--bool")) type = T_BOOL; + else if (!strcmp(argv[1], "--list") || !strcmp(argv[1], "-l")) + return git_config(show_all_config); else break; argc--; argv++; } - if (!strcmp(argv[1], "--list") || !strcmp(argv[1], "-l")) - return git_config(show_all_config); - switch (argc) { case 2: return get_value(argv[1], NULL); @@ -124,6 +139,11 @@ int main(int argc, const char **argv) else if (!strcmp(argv[1], "--get-all")) { do_all = 1; return get_value(argv[2], NULL); + } else if (!strcmp(argv[1], "--get-regexp")) { + show_keys = 1; + use_key_regexp = 1; + do_all = 1; + return get_value(argv[2], NULL); } else return git_config_set(argv[1], argv[2]); @@ -137,6 +157,11 @@ int main(int argc, const char **argv) else if (!strcmp(argv[1], "--get-all")) { do_all = 1; return get_value(argv[2], argv[3]); + } else if (!strcmp(argv[1], "--get-regexp")) { + show_keys = 1; + use_key_regexp = 1; + do_all = 1; + return get_value(argv[2], argv[3]); } else if (!strcmp(argv[1], "--replace-all")) return git_config_set_multivar(argv[2], argv[3], NULL, 1); diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh index ab4dd5c..52e8e33 100755 --- a/t/t1300-repo-config.sh +++ b/t/t1300-repo-config.sh @@ -247,6 +247,24 @@ EOF test_expect_success 'hierarchical section value' 'cmp .git/config expect' +cat > expect << EOF +beta.noindent=sillyValue +nextsection.nonewline=wow2 for me +123456.a123=987 +1.2.3.alpha=beta +EOF + +test_expect_success 'working --list' \ + 'git-repo-config --list > output && cmp output expect' + +cat > expect << EOF +beta.noindent sillyValue +nextsection.nonewline wow2 for me +EOF + +test_expect_success '--get-regexp' \ + 'git-repo-config --get-regexp in > output && cmp output expect' + cat > .git/config << EOF [novalue] variable @@ -255,5 +273,10 @@ EOF test_expect_success 'get variable with no value' \ 'git-repo-config --get novalue.variable ^$' +git-repo-config > output 2>&1 + +test_expect_success 'no arguments, but no crash' \ + "test $? = 129 && grep usage output" + test_done ``` ## Junio C Hamano, 2006-05-02 23:30 Subject: Re: [PATCH] repo-config: support --get-regexp and fix crash Message-ID: <7vmze0j97u.fsf@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7vmze0j97u.fsf%40assigned-by-dhcp.cox.net In-Reply-To: ``` Johannes Schindelin writes: > Junio made me aware of a crash, a fix for which was too easy to > merit a separate patch. Not really. The fix is clean-and-obvious, and is readily applicable to "master" without being cooked in "next". On the other hand, --get-regexp may or may not be so; by conflating the two, you are delaying the trivial and useful fix unnecessarily. I already split the patch and applied the fix on "master", while keeping the rest in "pu" (sorry, I ran out of patience to test everything to put it in "next" -- will do so tomorrow). It is usually a good idea to avoid making an otherwise good clean patch a hostage to new features (intentionally or accidentally). > Strange thing I realized: A value is white-space-trimmed at the end > only if the line does not end with a comment. This fact is accounted > for in the new tests. Thanks - a note like this helps me quite a bit, because I usually apply e-mailed patches with --whitespace=strip, which would have destroyed the test, leaving me scratching my head without such a notice. ``` ## Junio C Hamano, 2006-05-03 04:06 Subject: Re: [PATCH] repo-config: support --get-regexp and fix crash Message-ID: <7vlktjhhvc.fsf@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7vlktjhhvc.fsf%40assigned-by-dhcp.cox.net In-Reply-To: ``` Johannes Schindelin writes: > diff --git a/repo-config.c b/repo-config.c > index fa8aba7..722153c 100644 > --- a/repo-config.c > +++ b/repo-config.c > @@ -6,7 +6,10 @@ static const char git_config_set_usage[] New flag missing from usage. > > static char* key = NULL; > static char* value = NULL; > +static regex_t* key_regexp = NULL; > static regex_t* regexp = NULL; > +static int show_keys = 0; > +static int use_key_regexp = 0; > static int do_all = 0; > static int do_not_match = 0; > static int seen = 0; > @@ -26,16 +29,18 @@ static int show_config(const char* key_, > if (value_ == NULL) > value_ = ""; > > - if (!strcmp(key_, key) && > + if ((use_key_regexp || !strcmp(key_, key)) && > + (!use_key_regexp || > + !regexec(key_regexp, key_, 0, NULL, 0)) && > (regexp == NULL || > (do_not_match ^ > !regexec(regexp, value_, 0, NULL, 0)))) { That's a convoluted logic. (1) Either we are using key-regexp, or otherwise the key has to exactly match; and (2) Either we are not using key-regexp, or key-regexp must match; and (3) Either we are not using regexp, or value must match (or unmatch) as we are told by do_no_match. It all makes sense, but I wonder if this is the clearest way to convey what is happening to people. Not that I have a cleaner alternative in mind... > @@ -63,6 +70,14 @@ static int get_value(const char* key_, c > key[i] = tolower(key_[i]); > key[i] = 0; > > + if (use_key_regexp) { > + key_regexp = (regex_t*)malloc(sizeof(regex_t)); > + if (regcomp(key_regexp, key, REG_EXTENDED)) { > + fprintf(stderr, "Invalid key pattern: %s\n", regex_); > + return -1; > + } > + } > + Not worrying about leaking on failure path is just fine, since this is not a libified part. Perhaps the free() in get_value() are all like that -- get_value() is called once immediately before exiting the program anyway ;-). To my deliberately bogus request, I am getting (null); fprintf() should use "key" instead of "regex_", perhaps? $ git-repo-config --get-regexp 'core[' Invalid key pattern: (null) > @@ -78,7 +93,8 @@ static int get_value(const char* key_, c > > git_config(show_config); > if (value) { > - printf("%s\n", value); > + if (!do_all) > + printf("%s\n", value); > free(value); > } > free(key); I wonder if it would make things cleaner if you do not keep the global "value" around. Instead, do all the printing in show_config(), using a static global bool "seen" to control do_all vs get-all request as you already do. Then get_value() does not even need to worry about freeing the value, does it? Also I am not sure if "say OK if do_all was requested" at the end of get_value(). If a do_all request did not find any match, is it an OK condition? I do not have strong feeling either way, though. A suggested patch on top of your version that is in "pu". -- >8 -- diff --git a/repo-config.c b/repo-config.c index 722153c..7e06d1a 100644 --- a/repo-config.c +++ b/repo-config.c @@ -2,10 +2,9 @@ #include "cache.h" #include static const char git_config_set_usage[] = -"git-repo-config [ --bool | --int ] [--get | --get-all | --replace-all | --unset | --unset-all] name [value [value_regex]] | --list"; +"git-repo-config [ --bool | --int ] [--get | --get-all | --get-regexp | --replace-all | --unset | --unset-all] name [value [value_regex]] | --list"; static char* key = NULL; -static char* value = NULL; static regex_t* key_regexp = NULL; static regex_t* regexp = NULL; static int show_keys = 0; @@ -26,6 +25,9 @@ static int show_all_config(const char *k static int show_config(const char* key_, const char* value_) { + char value[256]; + const char *vptr = value; + if (value_ == NULL) value_ = ""; @@ -35,28 +37,25 @@ static int show_config(const char* key_, (regexp == NULL || (do_not_match ^ !regexec(regexp, value_, 0, NULL, 0)))) { + int dup_error = 0; if (show_keys) printf("%s ", key_); - if (seen > 0) { - if (!do_all) - fprintf(stderr, "More than one value: %s\n", - value); - free(value); - } - - if (type == T_INT) { - value = malloc(256); + if (seen && !do_all) + dup_error = 1; + if (type == T_INT) sprintf(value, "%d", git_config_int(key_, value_)); - } else if (type == T_BOOL) { - value = malloc(256); + else if (type == T_BOOL) sprintf(value, "%s", git_config_bool(key_, value_) ? "true" : "false"); - } else { - value = strdup(value_); - } + else + vptr = value_; seen++; - if (do_all) - printf("%s\n", value); + if (dup_error) { + error("More than one value for the key %s: %s", + key_, vptr); + } + else + printf("%s\n", vptr); } return 0; } @@ -73,7 +72,7 @@ static int get_value(const char* key_, c if (use_key_regexp) { key_regexp = (regex_t*)malloc(sizeof(regex_t)); if (regcomp(key_regexp, key, REG_EXTENDED)) { - fprintf(stderr, "Invalid key pattern: %s\n", regex_); + fprintf(stderr, "Invalid key pattern: %s\n", key_); return -1; } } @@ -92,11 +91,6 @@ static int get_value(const char* key_, c } git_config(show_config); - if (value) { - if (!do_all) - printf("%s\n", value); - free(value); - } free(key); if (regexp) { regfree(regexp); @@ -104,9 +98,9 @@ static int get_value(const char* key_, c } if (do_all) - return 0; + return !seen; - return seen == 1 ? 0 : 1; + return (seen == 1) ? 0 : 1; } int main(int argc, const char **argv) ``` ## Johannes Schindelin, 2006-05-03 11:55 Subject: Re: [PATCH] repo-config: support --get-regexp and fix crash Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.63.0605031353300.4017%40wbgn013.biozentrum.uni-wuerzburg.de In-Reply-To: <7vmze0j97u.fsf@assigned-by-dhcp.cox.net> ``` Hi, On Tue, 2 May 2006, Junio C Hamano wrote: > Johannes Schindelin writes: > > > Junio made me aware of a crash, a fix for which was too easy to > > merit a separate patch. > > Not really. Right. I was just too lazy. > > Strange thing I realized: A value is white-space-trimmed at the end > > only if the line does not end with a comment. This fact is accounted > > for in the new tests. > > Thanks - a note like this helps me quite a bit, because I > usually apply e-mailed patches with --whitespace=strip, which > would have destroyed the test, leaving me scratching my head > without such a notice. Apparently, I hid that note well: somebody complained privately that my patch is not white-space clean. As for the --get-regexp part: just take your time. It was easy enough, and I think it is quite cool, but I have to see an application for it yet. Ciao, Dscho ``` ## Johannes Schindelin, 2006-05-03 12:41 Subject: Re: [PATCH] repo-config: support --get-regexp and fix crash Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.63.0605031438510.6340%40wbgn013.biozentrum.uni-wuerzburg.de In-Reply-To: <7vlktjhhvc.fsf@assigned-by-dhcp.cox.net> ``` Hi, thanks for your patch. On Tue, 2 May 2006, Junio C Hamano wrote: > Johannes Schindelin writes: > > > @@ -26,16 +29,18 @@ static int show_config(const char* key_, > > if (value_ == NULL) > > value_ = ""; > > > > - if (!strcmp(key_, key) && > > + if ((use_key_regexp || !strcmp(key_, key)) && > > + (!use_key_regexp || > > + !regexec(key_regexp, key_, 0, NULL, 0)) && > > (regexp == NULL || > > (do_not_match ^ > > !regexec(regexp, value_, 0, NULL, 0)))) { > > That's a convoluted logic. > > (1) Either we are using key-regexp, or otherwise the key has to > exactly match; and > > (2) Either we are not using key-regexp, or key-regexp must > match; and > > (3) Either we are not using regexp, or value must match (or > unmatch) as we are told by do_no_match. > > It all makes sense, but I wonder if this is the clearest way to > convey what is happening to people. Not that I have a cleaner > alternative in mind... How about this (on top of your patch): --- snip --- [PATCH] repo-config: deconvolute logics It was rightly noticed that the logic is quite convoluted. Fix that. Signed-off-by: Johannes Schindelin --- The real change is very short, but a code block got reindented, too. repo-config.c | 50 ++++++++++++++++++++++++++------------------------ 1 files changed, 26 insertions(+), 24 deletions(-) diff --git a/repo-config.c b/repo-config.c index 7e06d1a..63eda1b 100644 --- a/repo-config.c +++ b/repo-config.c @@ -27,36 +27,38 @@ static int show_config(const char* key_, { char value[256]; const char *vptr = value; + int dup_error = 0; if (value_ == NULL) value_ = ""; - if ((use_key_regexp || !strcmp(key_, key)) && - (!use_key_regexp || - !regexec(key_regexp, key_, 0, NULL, 0)) && - (regexp == NULL || + if (!use_key_regexp && strcmp(key_, key)) + return 0; + if (use_key_regexp && regexec(key_regexp, key_, 0, NULL, 0)) + return 0; + if (regexp != NULL && (do_not_match ^ - !regexec(regexp, value_, 0, NULL, 0)))) { - int dup_error = 0; - if (show_keys) - printf("%s ", key_); - if (seen && !do_all) - dup_error = 1; - if (type == T_INT) - sprintf(value, "%d", git_config_int(key_, value_)); - else if (type == T_BOOL) - sprintf(value, "%s", git_config_bool(key_, value_) - ? "true" : "false"); - else - vptr = value_; - seen++; - if (dup_error) { - error("More than one value for the key %s: %s", - key_, vptr); - } - else - printf("%s\n", vptr); + regexec(regexp, value_, 0, NULL, 0))) + return 0; + + if (show_keys) + printf("%s ", key_); + if (seen && !do_all) + dup_error = 1; + if (type == T_INT) + sprintf(value, "%d", git_config_int(key_, value_)); + else if (type == T_BOOL) + vptr = git_config_bool(key_, value_) ? "true" : "false"; + else + vptr = value_; + seen++; + if (dup_error) { + error("More than one value for the key %s: %s", + key_, vptr); } + else + printf("%s\n", vptr); + return 0; } ```