| Message ID | c4bcb32299291549d82c0544937a647c5000ad64.1573670565.git.martin.agren@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | builtin/config: canonicalize "value_regex" with `--type=bool[-or-int]` | expand |
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. > > 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.
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
diff --git a/builtin/config.c b/builtin/config.c index 98d65bc0ad..e675ae0640 100644 --- a/builtin/config.c +++ b/builtin/config.c @@ -280,6 +280,28 @@ static int collect_config(const char *key_, const char *value_, void *cb) return format_config(&values->items[values->nr++], key_, value_); } +static int handle_value_regex(const char *regex_) +{ + if (!regex_) { + regexp = NULL; + return 0; + } + + if (regex_[0] == '!') { + do_not_match = 1; + regex_++; + } + + regexp = (regex_t*)xmalloc(sizeof(regex_t)); + if (regcomp(regexp, regex_, REG_EXTENDED)) { + error(_("invalid pattern: %s"), regex_); + FREE_AND_NULL(regexp); + return CONFIG_INVALID_PATTERN; + } + + return 0; +} + static int get_value(const char *key_, const char *regex_) { int ret = CONFIG_GENERIC_ERROR; @@ -317,20 +339,9 @@ static int get_value(const char *key_, const char *regex_) } } - if (regex_) { - if (regex_[0] == '!') { - do_not_match = 1; - regex_++; - } - - regexp = (regex_t*)xmalloc(sizeof(regex_t)); - if (regcomp(regexp, regex_, REG_EXTENDED)) { - error(_("invalid pattern: %s"), regex_); - FREE_AND_NULL(regexp); - ret = CONFIG_INVALID_PATTERN; - goto free_strings; - } - } + ret = handle_value_regex(regex_); + if (ret) + goto free_strings; config_with_options(collect_config, &values, &given_config_source, &config_options);
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. 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. builtin/config.c | 39 +++++++++++++++++++++++++-------------- 1 file changed, 25 insertions(+), 14 deletions(-)