[PATCH] cli: factor out config handling code to get/set lists.
Dmitry Kurochkin
dmitry.kurochkin at gmail.com
Sun Dec 11 08:41:53 PST 2011
Hi David.
On Sun, 11 Dec 2011 12:07:51 -0400, David Bremner <david at tethera.net> wrote:
> From: David Bremner <bremner at debian.org>
>
> Two new internal routines are created _config_get_list and
> _config_set_list; the notmuch_config_get_* functions that deal with
> lists are simply wrappers for these functions.
Looks good to me. Some comments below.
> ---
> notmuch-config.c | 130 +++++++++++++++++++++++++++--------------------------
> 1 files changed, 66 insertions(+), 64 deletions(-)
>
> diff --git a/notmuch-config.c b/notmuch-config.c
> index 1a7ed58..e98b6a3 100644
> --- a/notmuch-config.c
> +++ b/notmuch-config.c
> @@ -520,92 +520,94 @@ notmuch_config_set_user_primary_email (notmuch_config_t *config,
> config->user_primary_email = NULL;
> }
>
> -const char **
> -notmuch_config_get_user_other_email (notmuch_config_t *config,
> - size_t *length)
> +static const char **
> +_config_get_list (notmuch_config_t *config,
> + const char *section, const char *key,
> + const char ***outlist, size_t *list_length, size_t *ret_length)
> {
It seems weird that the function both takes list as an output parameter
and returns it. Having two length parameters which are set to the same
value is confusing as well. I understand that it is done to make getter
functions smaller. So perhaps it is ok.
> - char **emails;
> - size_t emails_length;
> + char **inlist;
> unsigned int i;
>
Please move variable declarations inside the if (!*outlist) and if
(inlist) blocks. (I saw other code in notmuch that does it, so it must
be ok.)
> - if (config->user_other_email == NULL) {
> - emails = g_key_file_get_string_list (config->key_file,
> - "user", "other_email",
> - &emails_length, NULL);
> - if (emails) {
> - config->user_other_email = talloc_size (config,
> - sizeof (char *) *
> - (emails_length + 1));
> - for (i = 0; i < emails_length; i++)
> - config->user_other_email[i] = talloc_strdup (config->user_other_email,
> - emails[i]);
> - config->user_other_email[i] = NULL;
> -
> - g_strfreev (emails);
> -
> - config->user_other_email_length = emails_length;
> + if (*outlist == NULL) {
> + inlist = g_key_file_get_string_list (config->key_file,
> + section, key,
> + list_length, NULL);
> + if (inlist) {
> + *outlist = talloc_size (config, sizeof (char *) *
> + (*list_length + 1));
> + for (i = 0; i < *list_length; i++)
> + (*outlist)[i] = talloc_strdup (*outlist, inlist[i]);
> + (*outlist)[i] = NULL;
> +
> + g_strfreev (inlist);
> }
> }
>
> - *length = config->user_other_email_length;
> - return config->user_other_email;
> + if (ret_length) *ret_length = *list_length;
I would prefer to have this on separate lines.
> + return *outlist;
> }
>
> -void
> -notmuch_config_set_user_other_email (notmuch_config_t *config,
> - const char *other_email[],
> - size_t length)
> +const char **
> +notmuch_config_get_user_other_email (notmuch_config_t *config, size_t *length)
> {
> - g_key_file_set_string_list (config->key_file,
> - "user", "other_email",
> - other_email, length);
> + return _config_get_list (config, "user", "other_email",
> + &(config->user_other_email),
> + &(config->user_other_email_length), length);
> +}
>
> - talloc_free (config->user_other_email);
> - config->user_other_email = NULL;
> +const char **
> +notmuch_config_get_new_tags (notmuch_config_t *config, size_t *length)
> +{
> + return _config_get_list (config, "new", "tags",
> + &(config->new_tags),
> + &(config->new_tags_length), length);
> }
>
> const char **
> -notmuch_config_get_new_tags (notmuch_config_t *config,
> - size_t *length)
> +notmuch_config_get_log_subscribers (notmuch_config_t *config, size_t *length)
> {
> - char **tags;
> - size_t tags_length;
> - unsigned int i;
> + return _config_get_list (config, "log", "subscribers",
> + &(config->new_tags),
> + &(config->new_tags_length), length);
> +}
>
This should be part of a separate patch which adds the logging feature,
I guess.
Regards,
Dmitry
> - if (config->new_tags == NULL) {
> - tags = g_key_file_get_string_list (config->key_file,
> - "new", "tags",
> - &tags_length, NULL);
> - if (tags) {
> - config->new_tags = talloc_size (config,
> - sizeof (char *) *
> - (tags_length + 1));
> - for (i = 0; i < tags_length; i++)
> - config->new_tags[i] = talloc_strdup (config->new_tags,
> - tags[i]);
> - config->new_tags[i] = NULL;
> -
> - g_strfreev (tags);
> -
> - config->new_tags_length = tags_length;
> - }
> - }
>
> - *length = config->new_tags_length;
> - return config->new_tags;
> +static void
> +_config_set_list (notmuch_config_t *config,
> + const char *group, const char *name,
> + const char *list[],
> + size_t length, const char ***config_var )
> +{
> + g_key_file_set_string_list (config->key_file, group, name, list, length);
> + talloc_free (*config_var);
> + *config_var = NULL;
> +}
> +
> +void
> +notmuch_config_set_user_other_email (notmuch_config_t *config,
> + const char *list[],
> + size_t length)
> +{
> + _config_set_list (config, "user", "other_email", list, length,
> + &(config->user_other_email));
> }
>
> void
> notmuch_config_set_new_tags (notmuch_config_t *config,
> - const char *new_tags[],
> - size_t length)
> + const char *list[],
> + size_t length)
> {
> - g_key_file_set_string_list (config->key_file,
> - "new", "tags",
> - new_tags, length);
> + _config_set_list (config, "new", "tags", list, length,
> + &(config->new_tags));
> +}
>
> - talloc_free (config->new_tags);
> - config->new_tags = NULL;
> +void
> +notmuch_config_set_log_subscribers (notmuch_config_t *config,
> + const char *list[],
> + size_t length)
> +{
> + _config_set_list (config, "log", "subscribers", list, length,
> + &(config->log_subscribers));
> }
>
> /* Given a configuration item of the form <group>.<key> return the
> --
> 1.7.7.3
>
> _______________________________________________
> notmuch mailing list
> notmuch at notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch
More information about the notmuch
mailing list