Re: [RFC/PATCH 1/2] config: Add cache for config value querying
- From
Tanay Abhra <tanayabh@gmail.com>
- Date
- May 27, 2014, 13:54 UTC
- Message-ID
- <5384991E.10409@gmail.com>
- In-Reply-To
- <53839DEE.9090504@web.de>
Hi,
On 05/26/2014 01:02 PM, Torsten Bögershausen wrote:
>> Add an internal cache with the all variable value pairs read from the usual > "cache": The word "cache" is in Git often used for "index"
Okay, point noted. I thought about choosing between "hashmap" and "cache" and chose the later.
> "variable value" can be written as "key value"
I had used the term "variable" to be consistent with the documentation (api-config.txt). But I think "key" is much clearer.
> "usual": I don't think we handle "unusual" config files, > (so can we drop the word usual ?)
Okay, noted.
Show 21 quoted lines
> I think the (important) first line can be written like this:
>
>> Add a hash table with the all key-value pairs read from the
> or
>> Add a hash table to cache all key-value pairs read from the
>
>> config files(repo specific .git/config, user wide ~/.gitconfig and the global
>> /etc/gitconfig). Also, add two external functions `git_config_get_string` and
> Can we drop "Also" ?
>> @@ -37,6 +39,102 @@ static struct config_source *cf;
>>
>> static int zlib_compression_seen;
>>
>> +struct hashmap config_cache;
>> +
>> +struct config_cache_node {
>> + struct hashmap_entry ent;
>> + struct strbuf key;
> Do we need a whole strbuf for the key?
> Or could a "char *key" work as well?
> (and/or could it be "const char *key" ?To maintain consistency with config.c. config.c uses strbuf for both key and value throughout. I found the reason by git-blaming config.c. Key length is flexible so it would be better to use a api construct such as strbuf for it.
Show 8 quoted lines
>> + struct string_list *value_list ;
>
>
>
>> +static struct string_list *config_cache_get_value(const char *key)
>> +{
>> + struct config_cache_node *e = config_cache_find_entry(key);
> why "e" ? Will "node" be easier to read ? Or entry ? Noted. Entry is much better.
Show 9 quoted lines
>> +static void config_cache_set_value(const char *key, const char *value)
>> +{
>> + struct config_cache_node *e;
>> +
>> + if (!value)
>> + return;
> Hm, either NULL could mean "unset==remove" the value, (but we don't do that, do we?
>
> Or it could mean a programming or runtime error?, Should there be a warning ?Nope. It is just a check to not save blank values for a key in the hashmap. Removal functionality will come later. NULL==remove is implemented in git_config_set_multivar_in_file(). We are not reading key value pairs from that, just from git_config().
Show 15 quoted lines
>> +
>> + e = config_cache_find_entry(key);
>> + if (!e) {
>> + e = malloc(sizeof(*e));
>> + hashmap_entry_init(e, strihash(key));
>> + strbuf_init(&(e->key), 1024);
>> + strbuf_addstr(&(e->key),key);
>> + e->value_list = malloc(sizeof(struct string_list));
>> + e->value_list->strdup_strings = 1;
>> + e->value_list->nr = 0;
>> + e->value_list->alloc = 0;
>> + e->value_list->items = NULL;
>> + e->value_list->cmp = NULL;
> When malloc() is replaced by xcalloc() the x = NULL and y = 0 can go away,
> and the code is shorter and easier to read.Much better, thanks.
Show 13 quoted lines
>> +extern const char *git_config_get_string(const char *name)
>> +{
>> + struct string_list *values;
>> + int num_values;
>> + char *result;
>> + values = config_cache_get_value(name);
>> + if (!values)
>> + return NULL;
>> + num_values = values->nr;
>> + result = values->items[num_values-1].string ;
> We could get rid of the variable "int num_values" by simply writing
> result = values->items[values->nr-1].string;
> Noted.
Cheers, Tanay Abhra.