From: Mark Walters <markwalters1009@gmail.com>
To: Tomi Ollila <tomi.ollila@iki.fi>, Jani Nikula <jani@nikula.org>,
	notmuch@notmuchmail.org
Subject: Re: [PATCH] cli: abstract common config get/set code
In-Reply-To: <m24n51cvr6.fsf@guru.guru-group.fi>
References: <1389976738-25056-1-git-send-email-jani@nikula.org>
	<m24n51cvr6.fsf@guru.guru-group.fi>
User-Agent: Notmuch/0.15.2+484~gfb59956 (http://notmuchmail.org) Emacs/23.4.1
	(x86_64-pc-linux-gnu)
Date: Sat, 18 Jan 2014 13:36:50 +0000
Message-ID: <87iothcsy5.fsf@qmul.ac.uk>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Precedence: list


LGTM too

Mark

On Sat, 18 Jan 2014, Tomi Ollila <tomi.ollila@iki.fi> wrote:
> 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
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch