From: Dmitry Kurochkin <dmitry.kurochkin@gmail.com>
To: David Bremner <david@tethera.net>, notmuch@notmuchmail.org
Cc: David Bremner <bremner@debian.org>
Subject: Re: [PATCH] cli: factor out config handling code to get/set lists.
Date: Sun, 11 Dec 2011 20:41:53 +0400 [thread overview]
Message-ID: <874nx76oxa.fsf@gmail.com> (raw)
In-Reply-To: <1323619671-14111-1-git-send-email-david@tethera.net>
Hi David.
On Sun, 11 Dec 2011 12:07:51 -0400, David Bremner <david@tethera.net> wrote:
> From: David Bremner <bremner@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@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch
next prev parent reply other threads:[~2011-12-11 16:42 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2011-12-10 17:50 [RFC PATCH] cli: factor out config handling code to get/set lists David Bremner
2011-12-10 18:24 ` Austin Clements
2011-12-10 20:30 ` David Bremner
2011-12-11 16:07 ` [PATCH] " David Bremner
2011-12-11 16:41 ` Dmitry Kurochkin [this message]
2011-12-12 18:26 ` David Bremner
2012-01-12 17:26 ` Pieter Praet
2012-01-12 17:30 ` [PATCH] test: cli: getting/setting/removing config values Pieter Praet
2012-01-13 3:42 ` David Bremner
2012-01-14 8:54 ` Pieter Praet
2012-01-14 8:57 ` Pieter Praet
2012-01-14 12:16 ` David Bremner
2012-01-16 10:31 ` Pieter Praet
2012-01-16 11:37 ` David Bremner
2012-01-16 16:48 ` Pieter Praet
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
List information: https://notmuchmail.org/
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=874nx76oxa.fsf@gmail.com \
--to=dmitry.kurochkin@gmail.com \
--cc=bremner@debian.org \
--cc=david@tethera.net \
--cc=notmuch@notmuchmail.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://yhetil.org/notmuch.git/
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).