From: Tomi Ollila <tomi.ollila@iki.fi>
To: Jani Nikula <jani@nikula.org>, notmuch@notmuchmail.org
Subject: Re: [PATCH] cli: abstract common config get/set code
In-Reply-To: <1389976738-25056-1-git-send-email-jani@nikula.org>
References: <1389976738-25056-1-git-send-email-jani@nikula.org>
User-Agent: Notmuch/0.17+34~g98b959f (http://notmuchmail.org) Emacs/24.3.1
	(x86_64-unknown-linux-gnu)
Date: Sat, 18 Jan 2014 14:36:13 +0200
Message-ID: <m24n51cvr6.fsf@guru.guru-group.fi>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: list

On Fri, Jan 17 2014, Jani Nikula <jani@nikula.org> wrote:

> Pretty straightforward abstraction similar to get/set list.
>
> ---

LGTM. tests pass.

>
> v2 of id:1376839205-5115-1-git-send-email-jani@nikula.org adding a few
> comments about config value caching per David's request. Dropped the
> 2nd patch as too tricky.
> ---
>  notmuch-config.c | 86 +++++++++++++++++++++++---------------------------------
>  1 file changed, 35 insertions(+), 51 deletions(-)
>
> diff --git a/notmuch-config.c b/notmuch-config.c
> index 6845e3c..4aad9eb 100644
> --- a/notmuch-config.c
> +++ b/notmuch-config.c
> @@ -496,6 +496,32 @@ notmuch_config_is_new (notmuch_config_t *config)
>      return config->is_new;
>  }
>
> +static const char *
> +_config_get (notmuch_config_t *config, char **field,
> +          const char *group, const char *key)
> +{
> +    /* read from config file and cache value, if not cached already */
> +    if (*field == NULL) {
> +     char *value;
> +     value = g_key_file_get_string (config->key_file, group, key, NULL);
> +     if (value) {
> +         *field = talloc_strdup (config, value);
> +         free (value);
> +     }
> +    }
> +    return *field;
> +}
> +
> +static void
> +_config_set (notmuch_config_t *config, char **field,
> +          const char *group, const char *key, const char *value)
> +{
> +    g_key_file_set_string (config->key_file, group, key, value);
> +
> +    /* drop the cached value */
> +    talloc_free (*field);
> +    *field = NULL;
> +}
>
>  static const char **
>  _config_get_list (notmuch_config_t *config,
> @@ -504,6 +530,7 @@ _config_get_list (notmuch_config_t *config,
>  {
>      assert(outlist);
>
> +    /* read from config file and cache value, if not cached already */
>      if (*outlist == NULL) {
>
>       char **inlist = g_key_file_get_string_list (config->key_file,
> @@ -535,6 +562,8 @@ _config_set_list (notmuch_config_t *config,
>                 size_t length, const char ***config_var )
>  {
>      g_key_file_set_string_list (config->key_file, group, name, list, length);
> +
> +    /* drop the cached value */
>      talloc_free (*config_var);
>      *config_var = NULL;
>  }
> @@ -542,85 +571,40 @@ _config_set_list (notmuch_config_t *config,
>  const char *
>  notmuch_config_get_database_path (notmuch_config_t *config)
>  {
> -    char *path;
> -
> -    if (config->database_path == NULL) {
> -     path = g_key_file_get_string (config->key_file,
> -                                   "database", "path", NULL);
> -     if (path) {
> -         config->database_path = talloc_strdup (config, path);
> -         free (path);
> -     }
> -    }
> -
> -    return config->database_path;
> +    return _config_get (config, &config->database_path, "database", "path");
>  }
>
>  void
>  notmuch_config_set_database_path (notmuch_config_t *config,
>                                 const char *database_path)
>  {
> -    g_key_file_set_string (config->key_file,
> -                        "database", "path", database_path);
> -
> -    talloc_free (config->database_path);
> -    config->database_path = NULL;
> +    _config_set (config, &config->database_path, "database", "path", database_path);
>  }
>
>  const char *
>  notmuch_config_get_user_name (notmuch_config_t *config)
>  {
> -    char *name;
> -
> -    if (config->user_name == NULL) {
> -     name = g_key_file_get_string (config->key_file,
> -                                   "user", "name", NULL);
> -     if (name) {
> -         config->user_name = talloc_strdup (config, name);
> -         free (name);
> -     }
> -    }
> -
> -    return config->user_name;
> +    return _config_get (config, &config->user_name, "user", "name");
>  }
>
>  void
>  notmuch_config_set_user_name (notmuch_config_t *config,
>                             const char *user_name)
>  {
> -    g_key_file_set_string (config->key_file,
> -                        "user", "name", user_name);
> -
> -    talloc_free (config->user_name);
> -    config->user_name = NULL;
> +    _config_set (config, &config->user_name, "user", "name", user_name);
>  }
>
>  const char *
>  notmuch_config_get_user_primary_email (notmuch_config_t *config)
>  {
> -    char *email;
> -
> -    if (config->user_primary_email == NULL) {
> -     email = g_key_file_get_string (config->key_file,
> -                                    "user", "primary_email", NULL);
> -     if (email) {
> -         config->user_primary_email = talloc_strdup (config, email);
> -         free (email);
> -     }
> -    }
> -
> -    return config->user_primary_email;
> +    return _config_get (config, &config->user_primary_email, "user", "primary_email");
>  }
>
>  void
>  notmuch_config_set_user_primary_email (notmuch_config_t *config,
>                                      const char *primary_email)
>  {
> -    g_key_file_set_string (config->key_file,
> -                        "user", "primary_email", primary_email);
> -
> -    talloc_free (config->user_primary_email);
> -    config->user_primary_email = NULL;
> +    _config_set (config, &config->user_primary_email, "user", "primary_email", primary_email);
>  }
>
>  const char **
> --
> 1.8.5.2
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch