From: Matthieu Moy <Matthieu.Moy@grenoble-inp•fr>
To: Tanay Abhra <tanayabh@gmail•com>
Cc: git@vger•kernel.org, Ramkumar Ramachandra <artagnon@gmail•com>,
Eric Sunshine <sunshine@sunshineco•com>
Subject: Re: [PATCH v2 2/2] config: Add hashtable for config parsing & retrieval
Date: Mon, 16 Jun 2014 19:11:22 +0200 [thread overview]
Message-ID: <vpqy4ww230l.fsf@anie.imag.fr> (raw)
In-Reply-To: <1402907232-24629-3-git-send-email-tanayabh@gmail.com> (Tanay Abhra's message of "Mon, 16 Jun 2014 01:27:12 -0700")
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.
> 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.
> +static int config_cache_callback(const char *key, const char *value, void *unused)
> +{
> + config_cache_set_value(key, value);
> + return 0;
> +}
> +
> +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;
> +}
Good, just enough code for the job :-).
> +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.
> +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.
> @@ -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).
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
next prev parent reply other threads:[~2014-06-16 17:11 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-06-16 8:27 [PATCH v2 0/2] Git config cache & special querying api utilizing the cache Tanay Abhra
2014-06-16 8:27 ` [PATCH v2 1/2] string-list: Add string_list initializer helper functions Tanay Abhra
2014-06-16 22:59 ` Junio C Hamano
2014-06-17 19:05 ` Tanay Abhra
2014-06-17 22:10 ` Junio C Hamano
2014-06-16 8:27 ` [PATCH v2 2/2] config: Add hashtable for config parsing & retrieval Tanay Abhra
2014-06-16 17:11 ` Matthieu Moy [this message]
2014-06-16 17:28 ` Tanay Abhra
2014-06-16 17:35 ` Matthieu Moy
2014-06-17 5:34 ` Jeff King
2014-06-17 5:46 ` Jeff King
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=vpqy4ww230l.fsf@anie.imag.fr \
--to=matthieu.moy@grenoble-inp$(echo .)fr \
--cc=artagnon@gmail$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=sunshine@sunshineco$(echo .)com \
--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