Re: [PATCH 4/8] builtin/config: collect "value_regexp" data in a struct
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Nov 21, 2019, 05:22 UTC
- Message-ID
- <xmqqy2w9951v.fsf@gitster-ct.c.googlers.com>
- In-Reply-To
- <336eaa77e4974f84ea1eef473672e1d300f3a43d.1573670565.git.martin.agren@gmail.com>
Martin Ågren <martin.agren@gmail.com> writes:
> `git config` can take an optional "value_regexp". Collect the > `regex_t`-pointer and the `do_not_match` flag into a new `struct > cmd_line_value`.
A "struct cmd_line_value" sounded, to me at least during my first reading, as if it is about all command line options, but that is not at all what you meant to imply. Is this only about the optional value-regexp (if so perhaps calling it "value_regexp_option" would have helped me avoid such a misunderstanding)?
> Rather than signalling/judging presence of a regexp by the NULL-ness of > the pointer, introduce a `mode` enum.
OK. Tangentially this makes readers wonder why the existing code for key_regexp does not follow the same "NULL-ness" pattern but has a separate use_key_regexp boolean. It appears that the original code is quite confused---it is totally outside the scope of this series to clean it up and inject sanity into it though ;-)
> static regex_t *key_regexp;
> -static regex_t *regexp;
> +static struct {
> + enum { none, regexp } mode;We often use the same identifier for a struct and an instance of the struct, taking advantage of the fact that they live in separate namespaces, but lowercase enumerated values like 'regexp' that collides with the field name (and possibly a variable name used elsewhere) smells a bit too much.
Show 6 quoted lines
> + regex_t *regexp; > + int do_not_match; /* used with `regexp` */ > +} cmd_line_value; > static int show_keys; > static int omit_values; > static int use_key_regexp;
Show 7 quoted lines
> @@ -283,19 +288,21 @@ static int collect_config(const char *key_, const char *value_, void *cb)
> static int handle_value_regex(const char *regex_)
> {
> if (!regex_) {
> - regexp = NULL;
> + cmd_line_value.mode = none;
> return 0;Now we are back to relying on cmd_line_value.regexp staying to be NULL after initialized, which is the state before the previous patch. If the end result is correct, then it is OK, I think, but then the previous step shouldn't have added the NULL assignment here in the first place.
Show 17 quoted lines
> }
>
> + cmd_line_value.mode = regexp;
> +
> if (regex_[0] == '!') {
> - do_not_match = 1;
> + cmd_line_value.do_not_match = 1;
> regex_++;
> }
>
> - regexp = (regex_t*)xmalloc(sizeof(regex_t));
> - if (regcomp(regexp, regex_, REG_EXTENDED)) {
> + cmd_line_value.regexp = xmalloc(sizeof(*cmd_line_value.regexp));
> + if (regcomp(cmd_line_value.regexp, regex_, REG_EXTENDED)) {
> error(_("invalid pattern: %s"), regex_);
> - FREE_AND_NULL(regexp);
> + FREE_AND_NULL(cmd_line_value.regexp);Hmph. !regexp in old code should mean cmd_line_value.mode==regexp in the new world order after this patch is applied, no? Should we be treaking the mode field here before we leave? I think it should not matter, but thought it wouldn't hurt to ask.
In collect_config(), cmd_line_value.regexp is blindly passed to regexec(3) as long as cmd_line_value.mode==regexp, so the invariant "when .mode is regexp, .regexp must be valid, or collect_config() would never be called for such cmd_line_value" is rather important to avoid crashing ;-)
Show 13 quoted lines
> return CONFIG_INVALID_PATTERN;
> }
>
> @@ -372,9 +379,9 @@ static int get_value(const char *key_, const char *regex_)
> regfree(key_regexp);
> free(key_regexp);
> }
> - if (regexp) {
> - regfree(regexp);
> - free(regexp);
> + if (cmd_line_value.regexp) {
> + regfree(cmd_line_value.regexp);
> + free(cmd_line_value.regexp);Likewise.
> } > > return ret;