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

From: Martin Ågren <martin.agren@gmail.com>
Date: Nov 21, 2019, 19:53 UTC
Message-ID: <CAN0heSrn07_dSTFB3ru3UZ14rxZvRD0mEAh5AMBFEDMgLKFLtA@mail.gmail.com>
In-Reply-To: <xmqq4kyxakjs.fsf@gitster-ct.c.googlers.com>

On Thu, 21 Nov 2019 at 06:02, Junio C Hamano <gitster@pobox.com> wrote:

Show 13 quoted lines

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

Show 9 quoted lines

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