[RFC PATCH] cli: factor out config handling code to get/set lists.

[RFC PATCH] cli: factor out config handling code to get/set lists.

[David Bremner]

From: David Bremner <bremner@debian.org>

The code is already duplicated once, and I want to add a third
configuration item that is also a list.
---

Mainly I am curious if people think using macros to declare these
"getters" and "setters" makes the code less maintainable.

 notmuch-config.c |  112 +++++++++++++++++++----------------------------------
 1 files changed, 40 insertions(+), 72 deletions(-)

diff --git a/notmuch-config.c b/notmuch-config.c
index 1a7ed58..33778a7 100644
--- a/notmuch-config.c
+++ b/notmuch-config.c
@@ -520,93 +520,61 @@ 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 void
+_config_get_list (notmuch_config_t *config,
+		  const char *section, const char *key,
+		  const char ***outlist, size_t *length)
 {
-    char **emails;
-    size_t emails_length;
+    char **inlist;
     unsigned int i;
 
-    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,
+					     length, NULL);
+	if (inlist) {
+	    *outlist = talloc_size (config, sizeof (char *) *
+				    (*length + 1));
+	    for (i = 0; i < *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;
 }
 
-void
-notmuch_config_set_user_other_email (notmuch_config_t *config,
-				     const char *other_email[],
-				     size_t length)
-{
-    g_key_file_set_string_list (config->key_file,
-				"user", "other_email",
-				other_email, length);
-
-    talloc_free (config->user_other_email);
-    config->user_other_email = NULL;
+#define _GET_LIST(list, group, name) \
+const char ** \
+notmuch_config_get_##list (notmuch_config_t *config, size_t *length) \
+{ \
+    _config_get_list (config, group, name, &(config->list),	\
+                      &(config->list##_length));		\
+    *length = config->list##_length;				\
+    return config->list; \
 }
 
-const char **
-notmuch_config_get_new_tags (notmuch_config_t *config,
-			     size_t *length)
-{
-    char **tags;
-    size_t tags_length;
-    unsigned int i;
+_GET_LIST (user_other_email, "user", "other_email");
+_GET_LIST (new_tags, "new", "tags");
 
-    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;
-	}
-    }
+#undef _GET_LIST
 
-    *length = config->new_tags_length;
-    return config->new_tags;
+#define _SET_LIST(list, group, name) \
+void \
+notmuch_config_set_##list (notmuch_config_t *config, const char *list[], \
+			   size_t length) \
+{ \
+    g_key_file_set_string_list (config->key_file, group, name, list, length); \
+    talloc_free (config->list);                                               \
+    config->list = NULL;                                                      \
 }
 
-void
-notmuch_config_set_new_tags (notmuch_config_t *config,
-			     const char *new_tags[],
-			     size_t length)
-{
-    g_key_file_set_string_list (config->key_file,
-				"new", "tags",
-				new_tags, length);
+_SET_LIST(user_other_email, "user", "other_email");
+_SET_LIST(new_tags, "new", "tags");
 
-    talloc_free (config->new_tags);
-    config->new_tags = NULL;
-}
+#undef _SET_LIST
 
 /* Given a configuration item of the form <group>.<key> return the
  * component group and key. If any error occurs, print a message on
-- 
1.7.7.3

Re: [RFC PATCH] cli: factor out config handling code to get/set lists.

[Austin Clements]

Deduplicating this code seems like a great idea, but I don't think
macros are the way to do it; especially not one that expands to an
important top-level construct like a function definition.

What about something like

const char **
notmuch_config_get_user_other_email (notmuch_config_t *config,
                                     size_t *length)
{
    return _config_get_list (config, "user", "other_email", 
                             &config->user_other_email, length);
}

where config->user_other_email is a, say, struct _notmuch_config_list
that combines together the cached string list and length?
_config_get_list would look a lot like what you have now, but outlist
would be a struct _notmuch_config_list* instead of a const char*** and
it would always set *length (in addition to setting it in the cached
value if necessary).

Set could be similarly simple

void
notmuch_config_set_user_other_email (notmuch_config_t *config,
                                     const char *other_email[],
                                     size_t length)
{
    _config_set_list (config, "user", "other_email", &config->user_other_email,
                      other_email, length);
}

Quoth David Bremner on Dec 10 at  1:50 pm:
> From: David Bremner <bremner@debian.org>
> 
> The code is already duplicated once, and I want to add a third
> configuration item that is also a list.
> ---
> 
> Mainly I am curious if people think using macros to declare these
> "getters" and "setters" makes the code less maintainable.
> 
>  notmuch-config.c |  112 +++++++++++++++++++----------------------------------
>  1 files changed, 40 insertions(+), 72 deletions(-)
> 
> diff --git a/notmuch-config.c b/notmuch-config.c
> index 1a7ed58..33778a7 100644
> --- a/notmuch-config.c
> +++ b/notmuch-config.c
> @@ -520,93 +520,61 @@ 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 void
> +_config_get_list (notmuch_config_t *config,
> +		  const char *section, const char *key,
> +		  const char ***outlist, size_t *length)
>  {
> -    char **emails;
> -    size_t emails_length;
> +    char **inlist;
>      unsigned int i;
>  
> -    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,
> +					     length, NULL);
> +	if (inlist) {
> +	    *outlist = talloc_size (config, sizeof (char *) *
> +				    (*length + 1));
> +	    for (i = 0; i < *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;
>  }
>  
> -void
> -notmuch_config_set_user_other_email (notmuch_config_t *config,
> -				     const char *other_email[],
> -				     size_t length)
> -{
> -    g_key_file_set_string_list (config->key_file,
> -				"user", "other_email",
> -				other_email, length);
> -
> -    talloc_free (config->user_other_email);
> -    config->user_other_email = NULL;
> +#define _GET_LIST(list, group, name) \
> +const char ** \
> +notmuch_config_get_##list (notmuch_config_t *config, size_t *length) \
> +{ \
> +    _config_get_list (config, group, name, &(config->list),	\
> +                      &(config->list##_length));		\
> +    *length = config->list##_length;				\
> +    return config->list; \
>  }
>  
> -const char **
> -notmuch_config_get_new_tags (notmuch_config_t *config,
> -			     size_t *length)
> -{
> -    char **tags;
> -    size_t tags_length;
> -    unsigned int i;
> +_GET_LIST (user_other_email, "user", "other_email");
> +_GET_LIST (new_tags, "new", "tags");
>  
> -    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;
> -	}
> -    }
> +#undef _GET_LIST
>  
> -    *length = config->new_tags_length;
> -    return config->new_tags;
> +#define _SET_LIST(list, group, name) \
> +void \
> +notmuch_config_set_##list (notmuch_config_t *config, const char *list[], \
> +			   size_t length) \
> +{ \
> +    g_key_file_set_string_list (config->key_file, group, name, list, length); \
> +    talloc_free (config->list);                                               \
> +    config->list = NULL;                                                      \
>  }
>  
> -void
> -notmuch_config_set_new_tags (notmuch_config_t *config,
> -			     const char *new_tags[],
> -			     size_t length)
> -{
> -    g_key_file_set_string_list (config->key_file,
> -				"new", "tags",
> -				new_tags, length);
> +_SET_LIST(user_other_email, "user", "other_email");
> +_SET_LIST(new_tags, "new", "tags");
>  
> -    talloc_free (config->new_tags);
> -    config->new_tags = NULL;
> -}
> +#undef _SET_LIST
>  
>  /* Given a configuration item of the form <group>.<key> return the
>   * component group and key. If any error occurs, print a message on

Re: [RFC PATCH] cli: factor out config handling code to get/set lists.

[David Bremner]

On Sat, 10 Dec 2011 13:24:36 -0500, Austin Clements <amdragon@MIT.EDU> wrote:
> What about something like
> 
> const char **
> notmuch_config_get_user_other_email (notmuch_config_t *config,
>                                      size_t *length)
> {
>     return _config_get_list (config, "user", "other_email", 
>                              &config->user_other_email, length);
> }

Sounds good. 

d

[PATCH] cli: factor out config handling code to get/set lists.

[David Bremner]

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.
---
 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)
 {
-    char **emails;
-    size_t emails_length;
+    char **inlist;
     unsigned int i;
 
-    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;
+    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);
+}
 
-    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

Re: [PATCH] cli: factor out config handling code to get/set lists.

[Dmitry Kurochkin]

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

Re: [PATCH] cli: factor out config handling code to get/set lists.

[David Bremner]

On Sun, 11 Dec 2011 20:41:53 +0400, Dmitry Kurochkin <dmitry.kurochkin@gmail.com> wrote:
> 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.

Updated version pushed. I'm not completely happy with _config_get_list's
weird API, but I don't have a better idea now.

d

Re: [PATCH] cli: factor out config handling code to get/set lists.

[Pieter Praet]

On Mon, 12 Dec 2011 14:26:21 -0400, David Bremner <david@tethera.net> wrote:
> On Sun, 11 Dec 2011 20:41:53 +0400, Dmitry Kurochkin <dmitry.kurochkin@gmail.com> wrote:
> > 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.
> 
> Updated version pushed. I'm not completely happy with _config_get_list's
> weird API, but I don't have a better idea now.
> 
> d
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Much cleaner!

However... this would preferrably have been preceded by tests.

Patch follows.


Peace

-- 
Pieter

[PATCH] test: cli: getting/setting/removing config values

[Pieter Praet]

Should have come before commit 1df71b55

---
 test/config       |   88 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 test/notmuch-test |    1 +
 2 files changed, 89 insertions(+), 0 deletions(-)
 create mode 100755 test/config

diff --git a/test/config b/test/config
new file mode 100755
index 0000000..ee3126c
--- /dev/null
+++ b/test/config
@@ -0,0 +1,88 @@
+#!/usr/bin/env bash
+test_description='notmuch config'
+. ./test-lib.sh
+
+
+config_options=(
+    "database.path"
+    "user.name"
+    "user.primary_email"
+    "user.other_email"
+    "new.tags"
+    "maildir.synchronize_flags"
+)
+
+
+test_begin_subtest 'getting config: "config get <section>.<item>"'
+echo -n "" > OUTPUT
+for i in ${config_options[*]} ; do
+    notmuch config get "${i}"
+done >> OUTPUT
+cat >EXPECTED <<EOF
+${MAIL_DIR}
+Notmuch Test Suite
+test_suite@notmuchmail.org
+test_suite_other@notmuchmail.org
+test_suite@otherdomain.org
+unread
+inbox
+true
+EOF
+test_expect_equal_file OUTPUT EXPECTED
+
+
+test_begin_subtest 'setting config: "config set <section>.<item> [values ...]"'
+notmuch config set database.path /path/to/maildir
+notmuch config set user.name "User Name"
+notmuch config set user.primary_email primary_email@notmuchmail.org
+notmuch config set user.other_email alt1@notmuchmail.org alt2@notmuchmail.org
+notmuch config set new.tags tag1 tag2 tag3
+notmuch config set maildir.synchronize_flags false
+echo -n "" > OUTPUT
+for i in ${config_options[*]} ; do
+    notmuch config get "${i}"
+done >> OUTPUT
+cat >EXPECTED <<EOF
+/path/to/maildir
+User Name
+primary_email@notmuchmail.org
+alt1@notmuchmail.org
+alt2@notmuchmail.org
+tag1
+tag2
+tag3
+false
+EOF
+test_expect_equal_file OUTPUT EXPECTED
+
+
+test_begin_subtest 'removing config: "config set <section>.<item>"'
+notmuch config set database.path
+notmuch config set user.name
+notmuch config set user.primary_email
+notmuch config set user.other_email
+notmuch config set new.tags
+notmuch config set maildir.synchronize_flags
+echo -n "" > OUTPUT
+for i in ${config_options[*]} ; do
+    notmuch config get "${i}"
+done >> OUTPUT
+
+# FIXME: Not the most robust nor portable solution here...
+# Especially `hostname --domain' may have unwanted effects on
+# some platforms, e.g. setting your hostname to "--domain" ;)
+fallback_name="$(grep $(id -un) /etc/passwd | cut -d ":" -f 5 | cut -d "," -f 1)"
+fallback_email="$(id -un)@$(hostname).$(hostname --domain)"
+
+cat >EXPECTED <<EOF
+${HOME}/mail
+${fallback_name}
+${fallback_email}
+unread
+inbox
+true
+EOF
+test_expect_equal_file OUTPUT EXPECTED
+
+
+test_done
diff --git a/test/notmuch-test b/test/notmuch-test
index e40ef86..f0c1d7c 100755
--- a/test/notmuch-test
+++ b/test/notmuch-test
@@ -18,6 +18,7 @@ cd $(dirname "$0")
 
 TESTS="
   basic
+  config
   help-test
   new
   count
-- 
1.7.8.1

Re: [PATCH] test: cli: getting/setting/removing config values

[David Bremner]

On Thu, 12 Jan 2012 18:30:01 +0100, Pieter Praet <pieter@praet.org> wrote:
> Should have come before commit 1df71b55

This doesn't seem like an especially helpful commit
message. Editorializing is ok, I guess, but maybe keep it below the
'---' ?

d

Re: [PATCH] test: cli: getting/setting/removing config values

[Pieter Praet]

On Thu, 12 Jan 2012 23:42:04 -0400, David Bremner <david@tethera.net> wrote:
> On Thu, 12 Jan 2012 18:30:01 +0100, Pieter Praet <pieter@praet.org> wrote:
> > Should have come before commit 1df71b55
> 
> This doesn't seem like an especially helpful commit
> message. Editorializing is ok, I guess, but maybe keep it below the
> '---' ?
> 

You're right.  The message I was trying to convey (quite tersely),
was that these tests were written in response to commit 1df71b55
(and should probably be retroactively ran on 1df71b55~1).

I realize now that it could potentially be wrongly interpreted as
criticism of sorts, which of course wasn't my intention.

Anyways, I ran the tests on both 1df71b55~1 and 1df71b55, and
-as expected- everything checks out fine.

Amended patch follows.

> d


Peace

-- 
Pieter

[PATCH] test: cli: getting/setting/removing config values

[Pieter Praet]

Full test coverage for getting, setting and removing options in
notmuch(1)'s config file ($NOTMUCH_CONFIG or $HOME/.notmuch-config).

---
Please *do* take note of the FIXME in the last test!

 test/config       |   88 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 test/notmuch-test |    1 +
 2 files changed, 89 insertions(+), 0 deletions(-)
 create mode 100755 test/config

diff --git a/test/config b/test/config
new file mode 100755
index 0000000..ee3126c
--- /dev/null
+++ b/test/config
@@ -0,0 +1,88 @@
+#!/usr/bin/env bash
+test_description='notmuch config'
+. ./test-lib.sh
+
+
+config_options=(
+    "database.path"
+    "user.name"
+    "user.primary_email"
+    "user.other_email"
+    "new.tags"
+    "maildir.synchronize_flags"
+)
+
+
+test_begin_subtest 'getting config: "config get <section>.<item>"'
+echo -n "" > OUTPUT
+for i in ${config_options[*]} ; do
+    notmuch config get "${i}"
+done >> OUTPUT
+cat >EXPECTED <<EOF
+${MAIL_DIR}
+Notmuch Test Suite
+test_suite@notmuchmail.org
+test_suite_other@notmuchmail.org
+test_suite@otherdomain.org
+unread
+inbox
+true
+EOF
+test_expect_equal_file OUTPUT EXPECTED
+
+
+test_begin_subtest 'setting config: "config set <section>.<item> [values ...]"'
+notmuch config set database.path /path/to/maildir
+notmuch config set user.name "User Name"
+notmuch config set user.primary_email primary_email@notmuchmail.org
+notmuch config set user.other_email alt1@notmuchmail.org alt2@notmuchmail.org
+notmuch config set new.tags tag1 tag2 tag3
+notmuch config set maildir.synchronize_flags false
+echo -n "" > OUTPUT
+for i in ${config_options[*]} ; do
+    notmuch config get "${i}"
+done >> OUTPUT
+cat >EXPECTED <<EOF
+/path/to/maildir
+User Name
+primary_email@notmuchmail.org
+alt1@notmuchmail.org
+alt2@notmuchmail.org
+tag1
+tag2
+tag3
+false
+EOF
+test_expect_equal_file OUTPUT EXPECTED
+
+
+test_begin_subtest 'removing config: "config set <section>.<item>"'
+notmuch config set database.path
+notmuch config set user.name
+notmuch config set user.primary_email
+notmuch config set user.other_email
+notmuch config set new.tags
+notmuch config set maildir.synchronize_flags
+echo -n "" > OUTPUT
+for i in ${config_options[*]} ; do
+    notmuch config get "${i}"
+done >> OUTPUT
+
+# FIXME: Not the most robust nor portable solution here...
+# Especially `hostname --domain' may have unwanted effects on
+# some platforms, e.g. setting your hostname to "--domain" ;)
+fallback_name="$(grep $(id -un) /etc/passwd | cut -d ":" -f 5 | cut -d "," -f 1)"
+fallback_email="$(id -un)@$(hostname).$(hostname --domain)"
+
+cat >EXPECTED <<EOF
+${HOME}/mail
+${fallback_name}
+${fallback_email}
+unread
+inbox
+true
+EOF
+test_expect_equal_file OUTPUT EXPECTED
+
+
+test_done
diff --git a/test/notmuch-test b/test/notmuch-test
index e40ef86..f0c1d7c 100755
--- a/test/notmuch-test
+++ b/test/notmuch-test
@@ -18,6 +18,7 @@ cd $(dirname "$0")
 
 TESTS="
   basic
+  config
   help-test
   new
   count
-- 
1.7.8.1

Re: [PATCH] test: cli: getting/setting/removing config values

[David Bremner]

On Sat, 14 Jan 2012 09:57:56 +0100, Pieter Praet <pieter@praet.org> wrote:
> Full test coverage for getting, setting and removing options in
> notmuch(1)'s config file ($NOTMUCH_CONFIG or $HOME/.notmuch-config).
> 
> ---
> +
> +# FIXME: Not the most robust nor portable solution here...
> +# Especially `hostname --domain' may have unwanted effects on
> +# some platforms, e.g. setting your hostname to "--domain" ;)
> +fallback_name="$(grep $(id -un) /etc/passwd | cut -d ":" -f 5 | cut -d "," -f 1)"
> +fallback_email="$(id -un)@$(hostname).$(hostname --domain)"

I'm not sure how portable it is, but maybe dnsdomainname would at least
have better failure modes.

I also wondered about using getent instead of grep.

d

Re: [PATCH] test: cli: getting/setting/removing config values

[Pieter Praet]

On Sat, 14 Jan 2012 08:16:30 -0400, David Bremner <david@tethera.net> wrote:
> On Sat, 14 Jan 2012 09:57:56 +0100, Pieter Praet <pieter@praet.org> wrote:
> > Full test coverage for getting, setting and removing options in
> > notmuch(1)'s config file ($NOTMUCH_CONFIG or $HOME/.notmuch-config).
> > 
> > ---
> > +
> > +# FIXME: Not the most robust nor portable solution here...
> > +# Especially `hostname --domain' may have unwanted effects on
> > +# some platforms, e.g. setting your hostname to "--domain" ;)
> > +fallback_name="$(grep $(id -un) /etc/passwd | cut -d ":" -f 5 | cut -d "," -f 1)"
> > +fallback_email="$(id -un)@$(hostname).$(hostname --domain)"
> 
> I'm not sure how portable it is, but maybe dnsdomainname would at least
> have better failure modes.
> 

Hmmm, `dnsdomainname' returns "(none)" here;  Does it work for you?

Running `domainname' instead seems to do the right thing though...


It would probably be safest to simply mirror how it's done @
notmuch-config.c:313, but that seems rather excessive.

> I also wondered about using getent instead of grep.
> 

Agreed, much safer;  `grep' might return multiple results.

> d

How about this? :

  #+begin_src sh
    fallback_name="$(getent passwd ${USER} | cut -d ":" -f 5 | cut -d "," -f 1)"
    test -n "${EMAIL}" \
        && fallback_email="${EMAIL}" \
        || fallback_email="${USER}@$(hostname).$(domainname)"
  #+end_src


Peace

-- 
Pieter

Re: [PATCH] test: cli: getting/setting/removing config values

[David Bremner]

On Mon, 16 Jan 2012 11:31:11 +0100, Pieter Praet <pieter@praet.org> wrote:
> 
> Hmmm, `dnsdomainname' returns "(none)" here;  Does it work for you?
> 
> Running `domainname' instead seems to do the right thing though...
> 

I make no claims to have "correctly" configured this machine on my home
network, but here it is the opposite.  The docs refer to NIS for
domainname, which I guess is not _that_ common for home networks (or at
all anymore?).

d

Re: [PATCH] test: cli: getting/setting/removing config values

[Pieter Praet]

On Mon, 16 Jan 2012 07:37:13 -0400, David Bremner <david@tethera.net> wrote:
> On Mon, 16 Jan 2012 11:31:11 +0100, Pieter Praet <pieter@praet.org> wrote:
> > 
> > Hmmm, `dnsdomainname' returns "(none)" here;  Does it work for you?
> > 
> > Running `domainname' instead seems to do the right thing though...
> > 
> 
> I make no claims to have "correctly" configured this machine on my home
> network, [...]

Same here :)

> [...] but here it is the opposite.  The docs refer to NIS for
> domainname, which I guess is not _that_ common for home networks (or at
> all anymore?).
> 

$ grep hosts /etc/nsswitch.conf
hosts: files dns

I guess that's a no.

> d


Perhaps others could chime in?


Peace

-- 
Pieter