Re: [PATCH 3/8] builtin/config: extract `handle_value_regex()` from `get_value()`

["Martin Ågren" <martin.agren@gmail.com>]

On Thu, 21 Nov 2019 at 06:02, Junio C Hamano <gitster@pobox.com> wrote:
>
> Martin Ågren <martin.agren@gmail.com> writes:
>
> > This is a self-contained and fairly large chunk of code which will soon
> > gain a few more lines. Prepare by extracting it into a separate
> > function.
> >
> > This whole chunk is wrapped in "if (regex_)" -- rewrite it into an early
> > return path in the new helper function to reduce indentation.
>
> It is not clear if regexp were cleared to NULL when !regex_ in the
> original code, so if that were the case, this refactoring is a
> worthy clean-up from that point of view, too.

Hmmm, this is something I added which makes it not a true refactoring
as such. I don't even recall doing this, but it does make sure we always
set this pointer to something sane. If we've already initialized this,
we'll risk leaking, but that should be better than running amok due to
bailing out early here and leaving the pointer pointing "somewhere".

That said, this is the only function where we set this, and we're
calling this function at most once (directly from `cmd_config()`),
so right now this NULL assignment is a no-op.

> > Signed-off-by: Martin Ågren <martin.agren@gmail.com>
> > ---
> >  I copy the "xmalloc(sizeof(regex_t))" anti-pattern verbatim. We will
> >  lose it in the next patch.
>
> It also spreads the use of file-scope global variables like
> do_not_match and regexp, which also is existing anti-pattern that we
> may want to fix by enclosing them in a struct and pass a pointer to
> it around the callchain.  We can clean it up later.

Right, in the next patch I collect them into a struct, but leave it
file-scope global. I didn't feel good adding another such global later
in the series, so decided to take a smallish step towards the end goal
you outline...


Martin