| Message ID | 8e0111c7b4b2c766c61df30c4ae93bd2d724de06.1606147507.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | config: add --fixed-value option | expand |
"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes: > - if (regex_) { > + if (regex_ && (flags & CONFIG_FLAGS_FIXED_VALUE)) > + value_regex = regex_; > + else if (regex_) { > if (regex_[0] == '!') { > do_not_match = 1; > regex_++; When the --fixed-value option is in effect, there is no way to say "replace values that do not match this string", because unlike the regex case, we do not special case '!' at the beginning. I think it is a good design decision. The special case has an escape hatch to start your value_regex with "[!]" when ERE is in use, but there is no such escape hatch possible with --fixed-value. Depending on how this '!' negation is documented to the end-users for existing value_regex that is ERE, the documentation for the new option may want to talk about the lack of negation explicitly. ... goes and looks ... It is worse than I thought. Here is what "git config" description has (and some of the badness is not the fault of this series): Multiple lines can be added to an option by using the `--add` option. If you want to update or unset an option which can occur on multiple lines, a POSIX regexp `value_regex` needs to be given. Only the existing values that match the regexp are updated or unset. If you want to handle the lines that do *not* match the regex, just prepend a single exclamation mark in front (see also <<EXAMPLES>>). The end-users these days no longer see "lines" because they do not hand edit .git/config, so we may need a replacement phrase to refer to a single "vari.able=value" setting, but we should leave it out of the scope of this series to clean that up---"git config --help" is full of references to "line". It was not "a POSIX regexp" (which does not say if it is BRE or ERE), and it no longer is a regexp with the new option. ... which can occur on multiple lines, a pattern `value_regex` needs to be given. Only the existing values that match the pattern are updated or unset. The pattern is by default matched as an extended regular expression, but with the `--fixed-value` option, taken as a literal string and values must be identical to it to be considered a match. You can prepend an exclamation point '!' to affect lines that do *not* match the pattern, but this is applicable only when not using the `--fixed-value` option. Ideally we probably should do s/value_regex/value_pattern/ in the documentation and error messages eventually, but I do not know if it is warranted in the scope of this series.
On Mon, Nov 23, 2020 at 04:05:06PM +0000, Derrick Stolee via GitGitGadget wrote: > Allowing myself a sensible chuckle at the commit subject using glob syntax on a series about regex matching. ;) > > The config builtin does its own regex matching of values for the --get, > --get-all, and --get-regexp modes. Plumb the existing 'flags' parameter > to the get_value() method so we can initialize the value_regex argument > as a fixed string instead of a regex pattern. > > Signed-off-by: Derrick Stolee <dstolee@microsoft.com> > --- > builtin/config.c | 15 ++++++++++----- > t/t1300-config.sh | 22 ++++++++++++++++++++++ > 2 files changed, 32 insertions(+), 5 deletions(-) > > diff --git a/builtin/config.c b/builtin/config.c > index 3e49e04411..d3772b5efe 100644 > --- a/builtin/config.c > +++ b/builtin/config.c > if (use_key_regexp && regexec(key_regexp, key_, 0, NULL, 0)) > return 0; > + if (fixed_value && strcmp(value_regex, (value_?value_:""))) > + return 0; Ooh, I can see you're matching style, but the combination of the spaceless ternary and the trailing underscore is making me so itchy ;) > if (regexp != NULL && > (do_not_match ^ !!regexec(regexp, (value_?value_:""), 0, NULL, 0))) > return 0; > @@ -298,7 +301,7 @@ static int collect_config(const char *key_, const char *value_, void *cb) > return format_config(&values->items[values->nr++], key_, value_); > } > > -static int get_value(const char *key_, const char *regex_) > +static int get_value(const char *key_, const char *regex_, int flags) Very reasonable, and I appreciate passing 'flags' instead of passing 'fixed' here. Room for growth :) [snip] a bunch of straightforward plumbing-through. > +test_expect_success '--get and --get-all with --fixed-value' ' > + GLOB="a+b*c?d[e]f.g" && > + rm -f config && Again this tells me that your other tests want 'test_when_finished' instead. > + git config --file=config fixed.test bogus && > + git config --file=config --add fixed.test "$GLOB" && > + > + git config --file=config --get fixed.test bogus && > + test_must_fail git config --file=config --get fixed.test "$GLOB" && > + git config --file=config --get --fixed-value fixed.test "$GLOB" && > + test_must_fail git config --file=config --get --fixed-value fixed.test non-existent && > + > + git config --file=config --get-all fixed.test bogus && > + test_must_fail git config --file=config --get-all fixed.test "$GLOB" && > + git config --file=config --get-all --fixed-value fixed.test "$GLOB" && > + test_must_fail git config --file=config --get-all --fixed-value fixed.test non-existent && > + > + git config --file=config --get-regexp fixed+ bogus && > + test_must_fail git config --file=config --get-regexp fixed+ "$GLOB" && > + git config --file=config --get-regexp --fixed-value fixed+ "$GLOB" && > + test_must_fail git config --file=config --get-regexp --fixed-value fixed+ non-existent > +' Otherwise the test seems fine to me, although I wouldn't yell if it grew some comments :) With the exception of the 'test_when_finished', which might not even match style (I didn't look): Reviewed-by: Emily Shaffer <emilyshaffer@google.com>
diff --git a/builtin/config.c b/builtin/config.c index 3e49e04411..d3772b5efe 100644 --- a/builtin/config.c +++ b/builtin/config.c @@ -14,6 +14,7 @@ static const char *const builtin_config_usage[] = { static char *key; static regex_t *key_regexp; +static const char *value_regex; static regex_t *regexp; static int show_keys; static int omit_values; @@ -288,6 +289,8 @@ static int collect_config(const char *key_, const char *value_, void *cb) return 0; if (use_key_regexp && regexec(key_regexp, key_, 0, NULL, 0)) return 0; + if (fixed_value && strcmp(value_regex, (value_?value_:""))) + return 0; if (regexp != NULL && (do_not_match ^ !!regexec(regexp, (value_?value_:""), 0, NULL, 0))) return 0; @@ -298,7 +301,7 @@ static int collect_config(const char *key_, const char *value_, void *cb) return format_config(&values->items[values->nr++], key_, value_); } -static int get_value(const char *key_, const char *regex_) +static int get_value(const char *key_, const char *regex_, int flags) { int ret = CONFIG_GENERIC_ERROR; struct strbuf_list values = {NULL}; @@ -335,7 +338,9 @@ static int get_value(const char *key_, const char *regex_) } } - if (regex_) { + if (regex_ && (flags & CONFIG_FLAGS_FIXED_VALUE)) + value_regex = regex_; + else if (regex_) { if (regex_[0] == '!') { do_not_match = 1; regex_++; @@ -859,19 +864,19 @@ int cmd_config(int argc, const char **argv, const char *prefix) } else if (actions == ACTION_GET) { check_argc(argc, 1, 2); - return get_value(argv[0], argv[1]); + return get_value(argv[0], argv[1], flags); } else if (actions == ACTION_GET_ALL) { do_all = 1; check_argc(argc, 1, 2); - return get_value(argv[0], argv[1]); + return get_value(argv[0], argv[1], flags); } else if (actions == ACTION_GET_REGEXP) { show_keys = 1; use_key_regexp = 1; do_all = 1; check_argc(argc, 1, 2); - return get_value(argv[0], argv[1]); + return get_value(argv[0], argv[1], flags); } else if (actions == ACTION_GET_URLMATCH) { check_argc(argc, 2, 2); diff --git a/t/t1300-config.sh b/t/t1300-config.sh index 30e80ae9cb..13211b6bf4 100755 --- a/t/t1300-config.sh +++ b/t/t1300-config.sh @@ -2040,4 +2040,26 @@ test_expect_success '--fixed-value uses exact string matching' ' test_cmp expect actual ' +test_expect_success '--get and --get-all with --fixed-value' ' + GLOB="a+b*c?d[e]f.g" && + rm -f config && + git config --file=config fixed.test bogus && + git config --file=config --add fixed.test "$GLOB" && + + git config --file=config --get fixed.test bogus && + test_must_fail git config --file=config --get fixed.test "$GLOB" && + git config --file=config --get --fixed-value fixed.test "$GLOB" && + test_must_fail git config --file=config --get --fixed-value fixed.test non-existent && + + git config --file=config --get-all fixed.test bogus && + test_must_fail git config --file=config --get-all fixed.test "$GLOB" && + git config --file=config --get-all --fixed-value fixed.test "$GLOB" && + test_must_fail git config --file=config --get-all --fixed-value fixed.test non-existent && + + git config --file=config --get-regexp fixed+ bogus && + test_must_fail git config --file=config --get-regexp fixed+ "$GLOB" && + git config --file=config --get-regexp --fixed-value fixed+ "$GLOB" && + test_must_fail git config --file=config --get-regexp --fixed-value fixed+ non-existent +' + test_done