Re: [PATCH v4 01/10] userdiff: refactor away the parse_bool() function
flat view
From: Jeff King <hidden>
Date: 2021-03-24 18:48:05
On Wed, Mar 24, 2021 at 02:48:43AM +0100, Ævar Arnfjörð Bjarmason wrote:
Since 6680a0874f (drop odd return value semantics from userdiff_config, 2012-02-07) we have not cared about the return values of parse_tristate() or git_config_bool() v.s. falling through in userdiff_config(), so let's do so in those cases to make the code easier to read. Having a wrapper function for git_config_bool() dates back to d9bae1a178 (diff: cache textconv output, 2010-04-01) and 122aa6f9c0 (diff: introduce diff.<driver>.binary, 2008-10-05), both of which predated the change in 6680a0874f which made their return values redundant.
I think this cleanup makes sense.
quoted hunk ↗ jump to hunk
int userdiff_config(const char *k, const char *v)@@ -312,16 +305,17 @@ int userdiff_config(const char *k, const char *v) return parse_funcname(&drv->funcname, k, v, 0); if (!strcmp(type, "xfuncname")) return parse_funcname(&drv->funcname, k, v, REG_EXTENDED); - if (!strcmp(type, "binary")) - return parse_tristate(&drv->binary, k, v); if (!strcmp(type, "command")) return git_config_string(&drv->external, k, v); if (!strcmp(type, "textconv")) return git_config_string(&drv->textconv, k, v); - if (!strcmp(type, "cachetextconv")) - return parse_bool(&drv->textconv_want_cache, k, v); if (!strcmp(type, "wordregex")) return git_config_string(&drv->word_regex, k, v); + /* Don't care about the parse errors for these, fallthrough */ + if (!strcmp(type, "cachetextconv")) + drv->textconv_want_cache = git_config_bool(k, v); + if (!strcmp(type, "binary")) + parse_tristate(&drv->binary, k, v);
The original returned early, which short-circuited the rest of the
strcmp(). that probably doesn't matter much in practice (after all, an
unrecognized value is inherently O(n)). But perhaps:
if (...)
assign...;
else if (...)
assign...;
would make the comment unnecessary. I don't feel strongly either way,
though.
-Peff