Re: [PATCH 4/8] builtin/config: collect "value_regexp" data in a struct

From: Junio C Hamano <gitster@pobox.com>
To: "Martin Ågren" <martin.agren@gmail.com>
Cc: git@vger.kernel.org
Subject: Re: [PATCH 4/8] builtin/config: collect "value_regexp" data in a struct
Date: Thu, 21 Nov 2019 14:22:52 +0900	[thread overview]
Message-ID: <xmqqy2w9951v.fsf@gitster-ct.c.googlers.com> (raw)
In-Reply-To: <336eaa77e4974f84ea1eef473672e1d300f3a43d.1573670565.git.martin.agren@gmail.com> ("Martin =?utf-8?Q?=C3=85gren=22's?= message of "Wed, 13 Nov 2019 19:55:03 +0100")

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.

> +	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;

> @@ -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.

>  	}
>  
> +	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 ;-)

>  		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;