* [RFC PATCH] cli: factor out config handling code to get/set lists. @ 2011-12-10 17:50 David Bremner 2011-12-10 18:24 ` Austin Clements 0 siblings, 1 reply; 15+ messages in thread From: David Bremner @ 2011-12-10 17:50 UTC (permalink / raw) To: notmuch; +Cc: 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 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [RFC PATCH] cli: factor out config handling code to get/set lists. 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 0 siblings, 1 reply; 15+ messages in thread From: Austin Clements @ 2011-12-10 18:24 UTC (permalink / raw) To: David Bremner; +Cc: notmuch, David Bremner 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC PATCH] cli: factor out config handling code to get/set lists. 2011-12-10 18:24 ` Austin Clements @ 2011-12-10 20:30 ` David Bremner 2011-12-11 16:07 ` [PATCH] " David Bremner 0 siblings, 1 reply; 15+ messages in thread From: David Bremner @ 2011-12-10 20:30 UTC (permalink / raw) To: Austin Clements; +Cc: notmuch 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH] cli: factor out config handling code to get/set lists. 2011-12-10 20:30 ` David Bremner @ 2011-12-11 16:07 ` David Bremner 2011-12-11 16:41 ` Dmitry Kurochkin 0 siblings, 1 reply; 15+ messages in thread From: David Bremner @ 2011-12-11 16:07 UTC (permalink / raw) To: notmuch; +Cc: 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 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH] cli: factor out config handling code to get/set lists. 2011-12-11 16:07 ` [PATCH] " David Bremner @ 2011-12-11 16:41 ` Dmitry Kurochkin 2011-12-12 18:26 ` David Bremner 0 siblings, 1 reply; 15+ messages in thread From: Dmitry Kurochkin @ 2011-12-11 16:41 UTC (permalink / raw) To: David Bremner, notmuch; +Cc: David Bremner 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] cli: factor out config handling code to get/set lists. 2011-12-11 16:41 ` Dmitry Kurochkin @ 2011-12-12 18:26 ` David Bremner 2012-01-12 17:26 ` Pieter Praet 0 siblings, 1 reply; 15+ messages in thread From: David Bremner @ 2011-12-12 18:26 UTC (permalink / raw) To: Dmitry Kurochkin, notmuch 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] cli: factor out config handling code to get/set lists. 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 0 siblings, 1 reply; 15+ messages in thread From: Pieter Praet @ 2012-01-12 17:26 UTC (permalink / raw) To: David Bremner, Dmitry Kurochkin, notmuch 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH] test: cli: getting/setting/removing config values 2012-01-12 17:26 ` Pieter Praet @ 2012-01-12 17:30 ` Pieter Praet 2012-01-13 3:42 ` David Bremner 0 siblings, 1 reply; 15+ messages in thread From: Pieter Praet @ 2012-01-12 17:30 UTC (permalink / raw) To: David Bremner, Dmitry Kurochkin; +Cc: Notmuch Mail 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 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH] test: cli: getting/setting/removing config values 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 0 siblings, 1 reply; 15+ messages in thread From: David Bremner @ 2012-01-13 3:42 UTC (permalink / raw) To: Pieter Praet; +Cc: Notmuch Mail 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] test: cli: getting/setting/removing config values 2012-01-13 3:42 ` David Bremner @ 2012-01-14 8:54 ` Pieter Praet 2012-01-14 8:57 ` Pieter Praet 0 siblings, 1 reply; 15+ messages in thread From: Pieter Praet @ 2012-01-14 8:54 UTC (permalink / raw) To: David Bremner; +Cc: Notmuch Mail 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH] test: cli: getting/setting/removing config values 2012-01-14 8:54 ` Pieter Praet @ 2012-01-14 8:57 ` Pieter Praet 2012-01-14 12:16 ` David Bremner 0 siblings, 1 reply; 15+ messages in thread From: Pieter Praet @ 2012-01-14 8:57 UTC (permalink / raw) To: David Bremner; +Cc: Notmuch Mail 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 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH] test: cli: getting/setting/removing config values 2012-01-14 8:57 ` Pieter Praet @ 2012-01-14 12:16 ` David Bremner 2012-01-16 10:31 ` Pieter Praet 0 siblings, 1 reply; 15+ messages in thread From: David Bremner @ 2012-01-14 12:16 UTC (permalink / raw) To: Pieter Praet; +Cc: Notmuch Mail 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] test: cli: getting/setting/removing config values 2012-01-14 12:16 ` David Bremner @ 2012-01-16 10:31 ` Pieter Praet 2012-01-16 11:37 ` David Bremner 0 siblings, 1 reply; 15+ messages in thread From: Pieter Praet @ 2012-01-16 10:31 UTC (permalink / raw) To: David Bremner; +Cc: Notmuch Mail 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] test: cli: getting/setting/removing config values 2012-01-16 10:31 ` Pieter Praet @ 2012-01-16 11:37 ` David Bremner 2012-01-16 16:48 ` Pieter Praet 0 siblings, 1 reply; 15+ messages in thread From: David Bremner @ 2012-01-16 11:37 UTC (permalink / raw) To: Pieter Praet; +Cc: Notmuch Mail 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] test: cli: getting/setting/removing config values 2012-01-16 11:37 ` David Bremner @ 2012-01-16 16:48 ` Pieter Praet 0 siblings, 0 replies; 15+ messages in thread From: Pieter Praet @ 2012-01-16 16:48 UTC (permalink / raw) To: David Bremner; +Cc: Notmuch Mail 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2012-01-16 16:50 UTC | newest] Thread overview: 15+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 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 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
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).