Re: [PATCH v2 2/2] config: Add hashtable for config parsing & retrieval

[Tanay Abhra <tanayabh@gmail.com>]

On 06/16/2014 10:11 AM, Matthieu Moy wrote:
> Tanay Abhra <tanayabh@gmail.com> writes:
> 
>> Add a hash table to cache all key-value pairs read from config files
>> (repo specific .git/config, user wide ~/.gitconfig and the global
>> /etc/gitconfig). Add two external functions `git_config_get_string` and
>> `git_config_get_string_multi` for querying in a non-callback manner from the
>> hash table.
> 
> This describes rather well _what_ your patch does, but the most
> important part of a commit message is to justify _why_ the change is
> good, and why the way you implemented it is good.
> 
> Think of it as an way to convince reviewers to accept your patch.

Okay, but isn't the content of the cover letter is doing that for now.
Should I put some part of it in here?

>> Signed-off-by: Tanay Abhra <tanayabh@gmail.com>
>> ---
>>  Documentation/technical/api-config.txt |  17 +++++
>>  cache.h                                |   2 +
>>  config.c                               | 123 +++++++++++++++++++++++++++++++++
>>  3 files changed, 142 insertions(+)
> 
> I'm still concerned about the fact that there is no test. At this point,
> git_config_get_string_multi and git_config_get_string are basically dead
> code.
> 
> I'm sure you did experiments to test these functions, run them through
> valgrind, ... Now, you need to let other people reproduce these
> experiments. "other people" can be reviewers, now, or anyone looking for
> bugs or regressions in the future.
>

Yeah, I have run the experiments. I will add a test file for it. I should have
appended it to this series only, my fault. :) A stray observation, Git has very less
unit tests, compared to the comprehensive test directory for commands.

>> +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);
>> +
>> +	if (ret)
>> +		return NULL;
>> +
>> +	hashmap_entry_init(&k, strhash(normalized_key));
>> +	k.key = (char *)normalized_key;
> 
> No need to cast.
> 

Noted.

>> +static int config_cache_set_value(const char *key, const char *value)
>> +{
>> +	struct hashmap *config_cache;
>> +	struct config_cache_entry *e;
>> +
>> +	config_cache = get_config_cache();
>> +	e = config_cache_find_entry(key);
>> +	if (!e) {
>> +		e = xmalloc(sizeof(*e));
>> +		hashmap_entry_init(e, strhash(key));
>> +		e->key = xstrdup(key);
>> +		string_list_init_dup(&e->value_list);
>> +		string_list_append(&e->value_list, value);
>> +		hashmap_add(config_cache, e);
>> +	} else {
>> +		string_list_append(&e->value_list, value);
>> +	}
>> +	return 0;
>> +}
> 
> I find the function name a bit confusing, as it does not "set" in the
> sense "override any previous value". Wouldn't this be better named
> config_cache_add_value? Or perhaps a comment would help.
>

Noted.

>> @@ -1714,6 +1830,13 @@ int git_config_set_multivar_in_file(const char *config_filename,
>>  	lock = NULL;
>>  	ret = 0;
>>  
>> +	/*
>> +	 *contents of config file has changed, so invalidate the
>> +	 *config cache used by non-callback based query functions.
>> +	 */
> 
> Spaces after stars:
> 
> /*
>  * Contents of config file has changed, so invalidate the
>  * config cache used by non-callback based query functions.
>  */
> 
> (I think the "s" of "contents" should be dropped too).
> 

Noted. Thanks for the review.