| Message ID | 63fb8ad99742d748dc00306be6d2e05bd0ed583d.1607514692.git.ps@pks.im (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | config: allow specifying config entries via env | expand |
On Wed, Dec 09 2020, Patrick Steinhardt wrote: > +static int config_parse_pair(const char *key, const char *value, > + config_fn_t fn, void *data) > +{ > + char *canonical_name; > + int ret; > + > + if (!strlen(key)) > + return error(_("empty config key")); We just did this check (just before the context of the second hunk in this patch) before calling this function: if (!pair[0]->len) { strbuf_list_free(pair); return error(_("bogus config parameter: %s"), text); I think just removing this is best, for such a closely coupled static function we can just rely on the sanity of the caller, but it should at least be: if (!strlen(key)) BUG("clear that it's unreachable, and a translator won't bother with it, like _()..."); Aside: It's more C-idiom-y in this case to write: if (!*key) > + if (git_config_parse_key(key, &canonical_name, NULL)) > + return -1; > + > + ret = (fn(canonical_name, value, data) < 0) ? -1 : 0; > + free(canonical_name); > + return ret; > +} > + > int git_config_parse_parameter(const char *text, > config_fn_t fn, void *data) > { > const char *value; > - char *canonical_name; > struct strbuf **pair; > int ret; > > @@ -483,12 +498,7 @@ int git_config_parse_parameter(const char *text, > return error(_("bogus config parameter: %s"), text); > } > > - if (git_config_parse_key(pair[0]->buf, &canonical_name, NULL)) { > - ret = -1; > - } else { > - ret = (fn(canonical_name, value, data) < 0) ? -1 : 0; > - free(canonical_name); > - } > + ret = config_parse_pair(pair[0]->buf, value, fn, data); > strbuf_list_free(pair); > return ret; > }
diff --git a/config.c b/config.c index cde3511110..151980e5c9 100644 --- a/config.c +++ b/config.c @@ -458,11 +458,26 @@ int git_config_key_is_valid(const char *key) return !git_config_parse_key_1(key, NULL, NULL, 1); } +static int config_parse_pair(const char *key, const char *value, + config_fn_t fn, void *data) +{ + char *canonical_name; + int ret; + + if (!strlen(key)) + return error(_("empty config key")); + if (git_config_parse_key(key, &canonical_name, NULL)) + return -1; + + ret = (fn(canonical_name, value, data) < 0) ? -1 : 0; + free(canonical_name); + return ret; +} + int git_config_parse_parameter(const char *text, config_fn_t fn, void *data) { const char *value; - char *canonical_name; struct strbuf **pair; int ret; @@ -483,12 +498,7 @@ int git_config_parse_parameter(const char *text, return error(_("bogus config parameter: %s"), text); } - if (git_config_parse_key(pair[0]->buf, &canonical_name, NULL)) { - ret = -1; - } else { - ret = (fn(canonical_name, value, data) < 0) ? -1 : 0; - free(canonical_name); - } + ret = config_parse_pair(pair[0]->buf, value, fn, data); strbuf_list_free(pair); return ret; }
The function `git_config_parse_parameter` is responsible for parsing a `foo.bar=baz`-formatted configuration key, sanitizing the key and then processing it via the given callback function. Given that we're about to add a second user which is going to process keys in such which already has keys and values separated, this commit extracts a function `config_parse_pair` which only does the sanitization and processing part. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- config.c | 24 +++++++++++++++++------- 1 file changed, 17 insertions(+), 7 deletions(-)