From: Dmitry Kurochkin <dmitry.kurochkin@gmail.com>
To: David Bremner <david@tethera.net>, notmuch@notmuchmail.org
Subject: Re: [PATCH] cli: factor out config handling code to get/set lists.
In-Reply-To: <1323619671-14111-1-git-send-email-david@tethera.net>
References: <87sjkstbiu.fsf@zancas.localnet>
	<1323619671-14111-1-git-send-email-david@tethera.net>
User-Agent: Notmuch/0.10.2+82~g96a629c (http://notmuchmail.org) Emacs/23.3.1
	(x86_64-pc-linux-gnu)
Date: Sun, 11 Dec 2011 20:41:53 +0400
Message-ID: <874nx76oxa.fsf@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Cc: David Bremner <bremner@debian.org>
Precedence: list

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