From: Ramsay Jones <ramsay@ramsay1•demon.co.uk>
To: Tanay Abhra <tanayabh@gmail•com>
Cc: git@vger•kernel.org, Ramkumar Ramachandra <artagnon@gmail•com>,
Matthieu Moy <Matthieu.Moy@grenoble-inp•fr>
Subject: Re: [PATCH v3 2/3] config: add hashtable for config parsing & retrieval
Date: Tue, 24 Jun 2014 16:32:56 +0100 [thread overview]
Message-ID: <53A99A28.1090302@ramsay1.demon.co.uk> (raw)
In-Reply-To: <53A853E9.8060801@gmail.com>
On 23/06/14 17:20, Tanay Abhra wrote:
> On 06/23/2014 07:57 AM, Ramsay Jones wrote:
>> On 23/06/14 11:11, Tanay Abhra wrote:
[snip]
>>> +static struct hashmap *get_config_cache(void)
>>> +{
>>> + static struct hashmap config_cache;
>>> + if (!hashmap_initialized) {
>>> + config_cache_init(&config_cache);
>>> + hashmap_initialized = 1;
>>> + git_config(config_cache_callback, NULL);
>>> + }
>>> + return &config_cache;
>>> +}
>>
>> [I have not been following this series at all (sorry I haven't had
>> the time to spare), so take these comments with a very big pinch of
>> salt! ie just ignore me if it's already been discussed etc. ;-) ]
>>
>> The 'git config' command can be used to read arbitrary files (so long
>> as they conform to the config syntax). For example, see the --file and
>> --blob options to git-config. At present, I think only scripted commands
>> use this facility (eg git-submodule). Noting the singleton config_cache,
>> what happens when git-submodule becomes a C builtin, or indeed any other
>> C builtin wants to take advantage of the new code when processing a non-
>> standard config file?
>>
>
> This series was mainly to replace git_config() invocations around the codebase.
> There are currently 111 git_config() invocations, each of which causes a file
> reread whenever called. git_config() only feeds values from the standard config
> files(i.e repo, user and global config).
>
> For reading config values from specific files or blobs, there are three functions
> git_config_with_options, git_config_from_file & git_config_from_blob which can be
> easily used inside a C builtin or anywhere in the code.
>
> The bulk of git_config_api calls are only for git_config(). For example,
> git_config_from_file() has three hits only in entire codebase,
> git_config_with_options() has 5 hits, so I concentrated on generating a cache
> for the usual config files only. For other files, the callers can fall back on older
> API functions like I had mentioned above.
>
> Forgive me if I inferred your question incorrectly. More below.
Hmm, maybe. The "... take advantage of the new code" refers to the
possibility (or otherwise) of re-using your work to update these
"older API" functions to the new API style. (also, see Junio's response).
[In order to do this, I would have expected to see one hash table
for each file/blob, so the singleton object took me by surprise.]
An "out of scope for this project" is a perfectly acceptable
response (*particularly* since it is very late in the day to be
bringing this up!).
>>> +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 = normalized_key;
>>> + found_entry = hashmap_get(config_cache, &k, NULL);
>>> + free(normalized_key);
>>> + return found_entry;
>>> +}
>>> +
>>> +static struct string_list *config_cache_get_value(const char *key)
>>> +{
>>> + struct config_cache_entry *e = config_cache_find_entry(key);
>>> + return e ? &e->value_list : NULL;
>>> +}
>>> +
>>> +static int config_cache_add_value(const char *key, const char *value)
>>> +{
>>> + struct hashmap *config_cache;
>>> + struct config_cache_entry *e;
>>> + struct string_list_item *item;
>>> + int *boolean_null_flag;
>>> +
>>> + config_cache = get_config_cache();
>>> + e = config_cache_find_entry(key);
>>> +
>>> + boolean_null_flag = xcalloc(1, sizeof(*boolean_null_flag));
>>> +
>>> + if (!e) {
>>> + e = xmalloc(sizeof(*e));
>>> + hashmap_entry_init(e, strhash(key));
>>> + e->key = xstrdup(key);
>>
>> config_cache_find_entry() searches for (and hashes the) normalized_key.
>> Should you not be entering the normalized key here?
>>
>
> config_cache_add_value() is fed key-values pairs through the git_config()
> callback mechanism, which normalises the key beforehand, so no need for
> renormalising.
Ah, yes, I forgot that the parsing code does a tolower() at various
places while accumulating the key string. So the (potentially) non-
normalized keys come from the user via the new API functions and,
rather than putting code to normalize the key in each of those,
just do it once in config_cache_find_entry(). (Although, you could
possibly do that in config_cache_get_value()). OK.
Hmm, maybe add a short comment to that effect? dunno.
ATB,
Ramsay Jones
next prev parent reply other threads:[~2014-06-24 15:33 UTC|newest]
Thread overview: 35+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-06-23 10:11 [PATCH v3 0/3] git config cache & special querying api utilizing the cache Tanay Abhra
2014-06-23 10:11 ` [PATCH v3 1/3] string-list: add string_list initialiser helper functions Tanay Abhra
2014-06-23 12:36 ` Torsten Bögershausen
2014-06-23 13:19 ` Tanay Abhra
2014-06-23 10:11 ` [PATCH v3 2/3] config: add hashtable for config parsing & retrieval Tanay Abhra
2014-06-23 11:55 ` Matthieu Moy
2014-06-24 12:06 ` Tanay Abhra
2014-06-25 20:25 ` Karsten Blees
2014-06-23 14:57 ` Ramsay Jones
2014-06-23 16:20 ` Tanay Abhra
2014-06-24 15:32 ` Ramsay Jones [this message]
2014-06-26 16:15 ` Matthieu Moy
2014-06-23 23:25 ` Junio C Hamano
2014-06-24 7:23 ` Tanay Abhra
2014-06-25 18:21 ` Junio C Hamano
2014-06-24 7:25 ` Tanay Abhra
2014-06-24 15:57 ` Ramsay Jones
2014-06-25 18:13 ` Junio C Hamano
2014-06-25 20:23 ` Karsten Blees
2014-06-25 20:53 ` Junio C Hamano
2014-06-26 17:37 ` Matthieu Moy
2014-06-26 19:00 ` Junio C Hamano
2014-06-26 19:19 ` Karsten Blees
2014-06-26 21:21 ` Junio C Hamano
2014-06-27 8:19 ` Karsten Blees
2014-06-27 8:19 ` Matthieu Moy
2014-06-27 17:13 ` Junio C Hamano
2014-06-23 23:14 ` Junio C Hamano
2014-06-24 12:21 ` Tanay Abhra
2014-06-26 16:27 ` Matthieu Moy
2014-06-25 21:44 ` Karsten Blees
2014-06-26 16:43 ` Matthieu Moy
2014-06-23 10:11 ` [PATCH v3 3/3] test-config: add usage examples for non-callback query functions Tanay Abhra
2014-06-25 11:19 ` Eric Sunshine
2014-06-26 8:40 ` Tanay Abhra
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=53A99A28.1090302@ramsay1.demon.co.uk \
--to=ramsay@ramsay1$(echo .)demon.co.uk \
--cc=Matthieu.Moy@grenoble-inp$(echo .)fr \
--cc=artagnon@gmail$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=tanayabh@gmail$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox