On Thu, 21 Nov 2019 at 06:22, Junio C Hamano <gitster@xxxxxxxxx> wrote:
>
> Martin Ågren <martin.agren@xxxxxxxxx> 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)?
Yes, that's right. Your suggested name is better. Thanks.
> > 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 ;-)
Yeah, I considered doing such a cleanup, but opted to try and stay
focused.
> > 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.
Ok, thanks for sanity-checking.
> > + 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.
Ok, noted.
As I wrote in my reply there, that made the whole thing not a 100%
refactoring anyway. I'll drop that one.
> > + 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 ;-)
Good point. No-one will be looking at the struct when we bail out here,
but we're just one missing "if" away from that changing... Might as well
try to leave things in a sane state to reduce the possibility of this
biting us in the future.
> > @@ -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.
Thanks.
Martin
