Re: [PATCH v3 2/3] config: add hashtable for config parsing & retrieval

[Karsten Blees <karsten.blees@gmail.com>]

Am 23.06.2014 12:11, schrieb Tanay Abhra:

[...]

> +
> +static struct config_cache_entry *config_cache_find_entry(const char *key)
> +{
> +	struct hashmap *config_cache;
> +	struct config_cache_entry k;
> +	struct config_cache_entry *found_entry;
> +	char *normalized_key;
> +	int ret;
> +	config_cache = get_config_cache();
> +	ret = git_config_parse_key(key, &normalized_key, NULL);

[I didn't follow all previous discussion, so feel free to ignore me if this has been discussed already]

Is it really necessary to normalize keys on each lookup? The current git config code simply does a 'strcmp("lower-case-key", var)' so normalization shouldn't be necessary for the majority of callers. If a caller uses a non-constant key (e.g. as passed to 'git config --get <name>'), it would be the caller's responsibility to normalize.

[...]

> +int git_config_get_string(const char *key, const char **value)

I would have expected '..._get_string' to return a string. If the return value indicates whether something was found, it should probably be called '..._find_...'?

In the end, you want typed config functions that do type conversion and handle parse errors, at least for the common types bool/int/string... Thus the generic function that returns unparsed data should probably be called '..._value', not '..._string'?

A typical pull-style config API will also let you specify default values, e.g.:

  const char *git_config_get_string(const char *key, const char *default_value)
  {
    const char *value;
    if (!git_config_find_value(key, &value))
      return default_value;
    if (!value)
      config_error_nonbool();
    return value;
  }

  int git_config_get_bool(const char *key, int default_value)
  {
    const char *value;
    if (!git_config_find_value(key, &value))
      return default_value;
    return git_config_bool(key, value);
  }