On 06/16/2014 10:11 AM, Matthieu Moy wrote:
> Tanay Abhra <tanayabh@xxxxxxxxx> 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@xxxxxxxxx>
>> ---
>> 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html