Re: [PATCH v2 6/7] config: implement --fixed-value with --get*
- From
Emily Shaffer <emilyshaffer@google.com>
- Date
- Nov 23, 2020, 22:43 UTC
- Message-ID
- <20201123224357.GF499823@google.com>
- In-Reply-To
- <8e0111c7b4b2c766c61df30c4ae93bd2d724de06.1606147507.git.gitgitgadget@gmail.com>
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. ;)
Show 20 quoted lines
> > 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 ;)
Show 9 quoted lines
> 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.
Show 18 quoted lines
> + 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>