[PATCH 1/2] config: Add 'config list' command

[PATCH 1/2] config: Add 'config list' command

[Peter Wang]

Add a command to list all keys in a given configuration section.

One use is as follows: a MUA may prefer to store data in a central
notmuch configuration file so that the data is accessible across
different machines, e.g. an addressbook.  The list command helps
to implement features such as tab completion on the keys.
---
 notmuch-config.c |   27 +++++++++++++++++++++++++++
 1 files changed, 27 insertions(+), 0 deletions(-)

diff --git a/notmuch-config.c b/notmuch-config.c
index e9b2750..595cf54 100644
--- a/notmuch-config.c
+++ b/notmuch-config.c
@@ -799,6 +799,31 @@ notmuch_config_command_set (void *ctx, char *item, int argc, char *argv[])
     return ret;
 }
 
+static int
+notmuch_config_command_list (void *ctx, char *group)
+{
+    notmuch_config_t *config;
+    char **keys;
+    size_t i, length;
+
+    config = notmuch_config_open (ctx, NULL, NULL);
+    if (config == NULL)
+	return 1;
+
+    keys = g_key_file_get_keys (config->key_file,
+				group, &length, NULL);
+    if (keys != NULL) {
+	for (i = 0; i < length; i++)
+	    printf ("%s\n", keys[i]);
+
+	free (keys);
+    }
+
+    notmuch_config_close (config);
+
+    return 0;
+}
+
 int
 notmuch_config_command (void *ctx, int argc, char *argv[])
 {
@@ -813,6 +838,8 @@ notmuch_config_command (void *ctx, int argc, char *argv[])
 	return notmuch_config_command_get (ctx, argv[1]);
     else if (strcmp (argv[0], "set") == 0)
 	return notmuch_config_command_set (ctx, argv[1], argc - 2, argv + 2);
+    else if (strcmp (argv[0], "list") == 0)
+	return notmuch_config_command_list (ctx, argv[1]);
 
     fprintf (stderr, "Unrecognized argument for notmuch config: %s\n",
 	     argv[0]);
-- 
1.7.4.4

[PATCH 2/2] man: Document 'config list' command

[Peter Wang]

Document the 'config list' command and its output.
---
 man/man1/notmuch-config.1 |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/man/man1/notmuch-config.1 b/man/man1/notmuch-config.1
index 395cb9c..b9f81a5 100644
--- a/man/man1/notmuch-config.1
+++ b/man/man1/notmuch-config.1
@@ -9,6 +9,9 @@ notmuch-config \- Access notmuch configuration file.
 .B notmuch config set
 .RI  "<" section ">.<" item "> [" value " ...]"
 
+.B notmuch config list
+.RI  "<" section ">
+
 .SH DESCRIPTION
 
 The
@@ -35,6 +38,15 @@ If no values are provided, the specified configuration item will be
 removed from the configuration file.
 .RE
 
+.RS 4
+.TP 4
+.B list
+The name of each configuration item in the specified section is
+printed to stdout, without the section name prefix. Each is terminated
+by a newline character. The output is empty if the section does not
+exist.
+.RE
+
 The available configuration items are described below.
 
 .RS 4
-- 
1.7.4.4

Re: [PATCH 1/2] config: Add 'config list' command

[Tomi Ollila]

On Wed, 21 Mar 2012 09:31:37 +1100, Peter Wang <novalazy@gmail.com> wrote:
> Add a command to list all keys in a given configuration section.
> 
> One use is as follows: a MUA may prefer to store data in a central
> notmuch configuration file so that the data is accessible across
> different machines, e.g. an addressbook.  The list command helps
> to implement features such as tab completion on the keys.

Before getting deeper into implementation it is good to remember
that 'notmuch config' is loosely modeled after 'git config'.

git config --list outputs somewhat different output.

Maybe this command should be like 'notmuch config list-section-keys'

> ---
>  notmuch-config.c |   27 +++++++++++++++++++++++++++
>  1 files changed, 27 insertions(+), 0 deletions(-)

Tomi

Re: [PATCH 1/2] config: Add 'config list' command

[Peter Wang]

On Wed, 21 Mar 2012 20:30:43 +0200, Tomi Ollila <tomi.ollila@iki.fi> wrote:
> On Wed, 21 Mar 2012 09:31:37 +1100, Peter Wang <novalazy@gmail.com> wrote:
> > Add a command to list all keys in a given configuration section.
> > 
> > One use is as follows: a MUA may prefer to store data in a central
> > notmuch configuration file so that the data is accessible across
> > different machines, e.g. an addressbook.  The list command helps
> > to implement features such as tab completion on the keys.
> 
> Before getting deeper into implementation it is good to remember
> that 'notmuch config' is loosely modeled after 'git config'.
> 
> git config --list outputs somewhat different output.
> 
> Maybe this command should be like 'notmuch config list-section-keys'

That would be fine.  The documentation never mentions keys, only
sections and items.  The implementation seems to think item == group.key.

Peter

P.S. In the first patch free() should be g_strfreev().

Re: [PATCH 1/2] config: Add 'config list' command

[Jameson Graef Rollins]

[-- Attachment #1: Type: text/plain, Size: 1441 bytes --]

On Wed, 21 Mar 2012 20:30:43 +0200, Tomi Ollila <tomi.ollila@iki.fi> wrote:
> On Wed, 21 Mar 2012 09:31:37 +1100, Peter Wang <novalazy@gmail.com> wrote:
> > Add a command to list all keys in a given configuration section.
> > 
> > One use is as follows: a MUA may prefer to store data in a central
> > notmuch configuration file so that the data is accessible across
> > different machines, e.g. an addressbook.  The list command helps
> > to implement features such as tab completion on the keys.
> 
> Before getting deeper into implementation it is good to remember
> that 'notmuch config' is loosely modeled after 'git config'.
> 
> git config --list outputs somewhat different output.
> 
> Maybe this command should be like 'notmuch config list-section-keys'

Git's interface is completely inconsistent, so I see no reason to try to
emulate it in every regard.  Let's just try to keep the notmuch
interface as nice and consistent as possible.

I do agree, though, that I think I would prefer to see the output in the
same format as "git config --list".  So I would suggest the command and
output be:

$ notmuch config list
database.path=/home/jrollins/.mail
user.name=Jameson Graef Rollins
user.primary_email=jrollins@finestructure.net
user.other_email=
new.tags=new;unread
maildir.synchronize_flags=false
search.exclude_tags=deleted;spam;

That seem like a much more generally useful output.

jamie.

[-- Attachment #2: Type: application/pgp-signature, Size: 835 bytes --]

Re: [PATCH 1/2] config: Add 'config list' command

[Jameson Graef Rollins]

[-- Attachment #1: Type: text/plain, Size: 245 bytes --]

On Wed, 21 Mar 2012 09:31:37 +1100, Peter Wang <novalazy@gmail.com> wrote:
> Add a command to list all keys in a given configuration section.

Hi, Peter.  Don't forget to include appropriate tests when introducing
new features.  Thanks.

jamie.

[-- Attachment #2: Type: application/pgp-signature, Size: 835 bytes --]

[PATCH v2] Add 'config list' command

[Peter Wang]

This version prints all config items and their values
instead of just the keys of a single section.

[PATCH v2 1/5] config: Fix free in 'config get' implementation.

[Peter Wang]

The array returned by g_key_file_get_string_list() should be freed with
g_strfreev(), not free().
---
 notmuch-config.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/notmuch-config.c b/notmuch-config.c
index e9b2750..85fc774 100644
--- a/notmuch-config.c
+++ b/notmuch-config.c
@@ -751,7 +751,7 @@ notmuch_config_command_get (void *ctx, char *item)
 	for (i = 0; i < length; i++)
 	    printf ("%s\n", value[i]);
 
-	free (value);
+	g_strfreev (value);
     }
 
     notmuch_config_close (config);
-- 
1.7.4.4

[PATCH v2 2/5] test: Add tests for 'config' command

[Peter Wang]

Start a new test script.
---
 test/config       |   20 ++++++++++++++++++++
 test/notmuch-test |    1 +
 2 files changed, 21 insertions(+), 0 deletions(-)
 create mode 100755 test/config

diff --git a/test/config b/test/config
new file mode 100755
index 0000000..d3e574c
--- /dev/null
+++ b/test/config
@@ -0,0 +1,20 @@
+#!/usr/bin/env bash
+
+test_description='"notmuch config"'
+. test-lib.sh
+
+test_begin_subtest "Get string value"
+test_expect_equal "$(notmuch config get user.name)" "Notmuch Test Suite"
+
+test_begin_subtest "Get list value"
+test_expect_equal "$(notmuch config get new.tags)" "\
+unread
+inbox"
+
+test_expect_success "Set string value" \
+  'notmuch config set foo.bar baz'
+
+test_expect_success "Set list value" \
+  'notmuch config set foo.list xxx "yyy yyy" "zzz zzz"'
+
+test_done
diff --git a/test/notmuch-test b/test/notmuch-test
index f03b594..e08ec72 100755
--- a/test/notmuch-test
+++ b/test/notmuch-test
@@ -19,6 +19,7 @@ cd $(dirname "$0")
 TESTS="
   basic
   help-test
+  config
   new
   count
   search
-- 
1.7.4.4

Re: [PATCH v2 2/5] test: Add tests for 'config' command

[Mark Walters]

This whole series looks good to me with one minor query.

> Start a new test script.
> ---
>  test/config       |   20 ++++++++++++++++++++
>  test/notmuch-test |    1 +
>  2 files changed, 21 insertions(+), 0 deletions(-)
>  create mode 100755 test/config
>
> diff --git a/test/config b/test/config
> new file mode 100755
> index 0000000..d3e574c
> --- /dev/null
> +++ b/test/config
> @@ -0,0 +1,20 @@
> +#!/usr/bin/env bash
> +
> +test_description='"notmuch config"'
> +. test-lib.sh
> +
> +test_begin_subtest "Get string value"
> +test_expect_equal "$(notmuch config get user.name)" "Notmuch Test Suite"
> +
> +test_begin_subtest "Get list value"
> +test_expect_equal "$(notmuch config get new.tags)" "\
> +unread
> +inbox"
> +
> +test_expect_success "Set string value" \
> +  'notmuch config set foo.bar baz'
> +
> +test_expect_success "Set list value" \
> +  'notmuch config set foo.list xxx "yyy yyy" "zzz zzz"'

It seems off to call is success without checking that the value has
actually been set. Of course it is checked in the notmuch config list
test introduced in the next commit but I think if it would be better to
check with notmuch config get here too (i.e. check that reading back the
value gives what you want). Otherwise a failure in `setting' will show up
as a test failure in `listing'.

Best wishes

Mark

> +
> +test_done
> diff --git a/test/notmuch-test b/test/notmuch-test
> index f03b594..e08ec72 100755
> --- a/test/notmuch-test
> +++ b/test/notmuch-test
> @@ -19,6 +19,7 @@ cd $(dirname "$0")
>  TESTS="
>    basic
>    help-test
> +  config
>    new
>    count
>    search
> -- 
> 1.7.4.4
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH v2 2/5] test: Add tests for 'config' command

[Jameson Graef Rollins]

[-- Attachment #1: Type: text/plain, Size: 747 bytes --]

On Sat, Mar 31 2012, Mark Walters <markwalters1009@gmail.com> wrote:
> It seems off to call is success without checking that the value has
> actually been set. Of course it is checked in the notmuch config list
> test introduced in the next commit but I think if it would be better to
> check with notmuch config get here too (i.e. check that reading back the
> value gives what you want). Otherwise a failure in `setting' will show up
> as a test failure in `listing'.

Hey, Peter.  I think Mark makes a good point here.  I think it would
make more sense for the test to set the value, and then check that the
value is properly set as expected.  It would make the tests multi-step,
but that's fine.  There's plenty of precedent for that.

jamie.

[-- Attachment #2: Type: application/pgp-signature, Size: 835 bytes --]

[PATCH v2 3/5] test: Add broken test for 'config list'

[Peter Wang]

Proposed functionality.
---
 test/config |   16 ++++++++++++++++
 1 files changed, 16 insertions(+), 0 deletions(-)

diff --git a/test/config b/test/config
index d3e574c..73c58ff 100755
--- a/test/config
+++ b/test/config
@@ -17,4 +17,20 @@ test_expect_success "Set string value" \
 test_expect_success "Set list value" \
   'notmuch config set foo.list xxx "yyy yyy" "zzz zzz"'
 
+test_begin_subtest "List all items"
+test_subtest_known_broken
+notmuch config set database.path "/canonical/path"
+output=$(notmuch config list)
+test_expect_equal "$output" "\
+database.path=/canonical/path
+user.name=Notmuch Test Suite
+user.primary_email=test_suite@notmuchmail.org
+user.other_email=test_suite_other@notmuchmail.org;test_suite@otherdomain.org
+new.tags=unread;inbox;
+new.ignore=
+search.exclude_tags=
+maildir.synchronize_flags=true
+foo.bar=baz
+foo.list=xxx;yyy yyy;zzz zzz;"
+
 test_done
-- 
1.7.4.4

[PATCH v2 4/5] config: Add 'config list' command

[Peter Wang]

Add a command to list all configuration items with their associated
values.

One use is as follows: a MUA may prefer to store data in a central
notmuch configuration file so that the data is accessible across
different machines, e.g. an addressbook.  The list command helps
to implement features such as tab completion on the keys.
---
 notmuch-config.c |   66 ++++++++++++++++++++++++++++++++++++++++++++++++++---
 test/config      |    1 -
 2 files changed, 62 insertions(+), 5 deletions(-)

diff --git a/notmuch-config.c b/notmuch-config.c
index 85fc774..d5540ac 100644
--- a/notmuch-config.c
+++ b/notmuch-config.c
@@ -799,20 +799,78 @@ notmuch_config_command_set (void *ctx, char *item, int argc, char *argv[])
     return ret;
 }
 
+static int
+notmuch_config_command_list (void *ctx)
+{
+    notmuch_config_t *config;
+    char **groups;
+    size_t g, groups_length;
+
+    config = notmuch_config_open (ctx, NULL, NULL);
+    if (config == NULL)
+	return 1;
+
+    groups = g_key_file_get_groups (config->key_file, &groups_length);
+    if (groups == NULL)
+	return 1;
+
+    for (g = 0; g < groups_length; g++) {
+	char **keys;
+	size_t k, keys_length;
+
+	keys = g_key_file_get_keys (config->key_file,
+				    groups[g], &keys_length, NULL);
+	if (keys == NULL)
+	    continue;
+
+	for (k = 0; k < keys_length; k++) {
+	    char *value;
+
+	    value = g_key_file_get_string (config->key_file,
+					   groups[g], keys[k], NULL);
+	    if (value != NULL) {
+		printf ("%s.%s=%s\n", groups[g], keys[k], value);
+		free (value);
+	    }
+	}
+
+	g_strfreev (keys);
+    }
+
+    g_strfreev (groups);
+
+    notmuch_config_close (config);
+
+    return 0;
+}
+
 int
 notmuch_config_command (void *ctx, int argc, char *argv[])
 {
     argc--; argv++; /* skip subcommand argument */
 
-    if (argc < 2) {
-	fprintf (stderr, "Error: notmuch config requires at least two arguments.\n");
+    if (argc < 1) {
+	fprintf (stderr, "Error: notmuch config requires at least one argument.\n");
 	return 1;
     }
 
-    if (strcmp (argv[0], "get") == 0)
+    if (strcmp (argv[0], "get") == 0) {
+	if (argc < 2) {
+	    fprintf (stderr, "Error: notmuch config get requires at least "
+		     "two arguments.\n");
+	    return 1;
+	}
 	return notmuch_config_command_get (ctx, argv[1]);
-    else if (strcmp (argv[0], "set") == 0)
+    } else if (strcmp (argv[0], "set") == 0) {
+	if (argc < 2) {
+	    fprintf (stderr, "Error: notmuch config set requires at least "
+		     "two arguments.\n");
+	    return 1;
+	}
 	return notmuch_config_command_set (ctx, argv[1], argc - 2, argv + 2);
+    } else if (strcmp (argv[0], "list") == 0) {
+	return notmuch_config_command_list (ctx);
+    }
 
     fprintf (stderr, "Unrecognized argument for notmuch config: %s\n",
 	     argv[0]);
diff --git a/test/config b/test/config
index 73c58ff..93e3a04 100755
--- a/test/config
+++ b/test/config
@@ -18,7 +18,6 @@ test_expect_success "Set list value" \
   'notmuch config set foo.list xxx "yyy yyy" "zzz zzz"'
 
 test_begin_subtest "List all items"
-test_subtest_known_broken
 notmuch config set database.path "/canonical/path"
 output=$(notmuch config list)
 test_expect_equal "$output" "\
-- 
1.7.4.4

[PATCH v2 5/5] man: Document 'config list' command

[Peter Wang]

Document the 'config list' command and its output.
---
 man/man1/notmuch-config.1 |   14 ++++++++++++++
 1 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/man/man1/notmuch-config.1 b/man/man1/notmuch-config.1
index 395cb9c..a55dbef 100644
--- a/man/man1/notmuch-config.1
+++ b/man/man1/notmuch-config.1
@@ -9,6 +9,8 @@ notmuch-config \- Access notmuch configuration file.
 .B notmuch config set
 .RI  "<" section ">.<" item "> [" value " ...]"
 
+.B notmuch config list
+
 .SH DESCRIPTION
 
 The
@@ -35,6 +37,18 @@ If no values are provided, the specified configuration item will be
 removed from the configuration file.
 .RE
 
+.RS 4
+.TP 4
+.B list
+Every configuration item is printed to stdout, each on a separate line
+of the form:
+
+.RI  "" section "." item "=" value
+
+No additional whitespace surrounds the dot or equals sign characters. In a
+multiple-value item (a list), the values are separated by semicolon characters.
+.RE
+
 The available configuration items are described below.
 
 .RS 4
-- 
1.7.4.4

[PATCH v3 0/5] Add 'config list' command

[Peter Wang]

This version tests 'config set' by reading back the set value, as suggested.


Peter Wang (5):
  config: Fix free in 'config get' implementation.
  test: Add tests for 'config' command
  test: Add broken test for 'config list'
  config: Add 'config list' command
  man: Document 'config list' command

 man/man1/notmuch-config.1 |   14 +++++++++
 notmuch-config.c          |   68 +++++++++++++++++++++++++++++++++++++++++---
 test/config               |   40 ++++++++++++++++++++++++++
 test/notmuch-test         |    1 +
 4 files changed, 118 insertions(+), 5 deletions(-)
 create mode 100755 test/config

-- 
1.7.4.4

[PATCH v3 1/5] config: Fix free in 'config get' implementation.

[Peter Wang]

The array returned by g_key_file_get_string_list() should be freed with
g_strfreev(), not free().
---
 notmuch-config.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/notmuch-config.c b/notmuch-config.c
index e9b2750..85fc774 100644
--- a/notmuch-config.c
+++ b/notmuch-config.c
@@ -751,7 +751,7 @@ notmuch_config_command_get (void *ctx, char *item)
 	for (i = 0; i < length; i++)
 	    printf ("%s\n", value[i]);
 
-	free (value);
+	g_strfreev (value);
     }
 
     notmuch_config_close (config);
-- 
1.7.4.4

[PATCH v3 2/5] test: Add tests for 'config' command

[Peter Wang]

Start a new test script.
---
 test/config       |   25 +++++++++++++++++++++++++
 test/notmuch-test |    1 +
 2 files changed, 26 insertions(+), 0 deletions(-)
 create mode 100755 test/config

diff --git a/test/config b/test/config
new file mode 100755
index 0000000..75f662d
--- /dev/null
+++ b/test/config
@@ -0,0 +1,25 @@
+#!/usr/bin/env bash
+
+test_description='"notmuch config"'
+. test-lib.sh
+
+test_begin_subtest "Get string value"
+test_expect_equal "$(notmuch config get user.name)" "Notmuch Test Suite"
+
+test_begin_subtest "Get list value"
+test_expect_equal "$(notmuch config get new.tags)" "\
+unread
+inbox"
+
+test_begin_subtest "Set string value"
+notmuch config set foo.bar baz
+test_expect_equal "$(notmuch config get foo.bar)" "baz"
+
+test_begin_subtest "Set list value"
+notmuch config set foo.list xxx "yyy yyy" "zzz zzz"
+test_expect_equal "$(notmuch config get foo.list)" "\
+xxx
+yyy yyy
+zzz zzz"
+
+test_done
diff --git a/test/notmuch-test b/test/notmuch-test
index f03b594..e08ec72 100755
--- a/test/notmuch-test
+++ b/test/notmuch-test
@@ -19,6 +19,7 @@ cd $(dirname "$0")
 TESTS="
   basic
   help-test
+  config
   new
   count
   search
-- 
1.7.4.4

Re: [PATCH v3 2/5] test: Add tests for 'config' command

[Jameson Graef Rollins]

[-- Attachment #1: Type: text/plain, Size: 612 bytes --]

On Thu, Apr 05 2012, Peter Wang <novalazy@gmail.com> wrote:
> +test_begin_subtest "Set string value"
> +notmuch config set foo.bar baz
> +test_expect_equal "$(notmuch config get foo.bar)" "baz"
> +
> +test_begin_subtest "Set list value"
> +notmuch config set foo.list xxx "yyy yyy" "zzz zzz"
> +test_expect_equal "$(notmuch config get foo.list)" "\
> +xxx
> +yyy yyy
> +zzz zzz"

I find it slightly strange to use non-existent fields here, but I also
don't see that it hurts anything either.  At least at the moment.
Another option would be to use an existing field, and change it back
when you're done.

jamie.

[-- Attachment #2: Type: application/pgp-signature, Size: 835 bytes --]

[PATCH v3 3/5] test: Add broken test for 'config list'

[Peter Wang]

Proposed functionality.
---
 test/config |   16 ++++++++++++++++
 1 files changed, 16 insertions(+), 0 deletions(-)

diff --git a/test/config b/test/config
index 75f662d..cce2abc 100755
--- a/test/config
+++ b/test/config
@@ -22,4 +22,20 @@ xxx
 yyy yyy
 zzz zzz"
 
+test_begin_subtest "List all items"
+test_subtest_known_broken
+notmuch config set database.path "/canonical/path"
+output=$(notmuch config list)
+test_expect_equal "$output" "\
+database.path=/canonical/path
+user.name=Notmuch Test Suite
+user.primary_email=test_suite@notmuchmail.org
+user.other_email=test_suite_other@notmuchmail.org;test_suite@otherdomain.org
+new.tags=unread;inbox;
+new.ignore=
+search.exclude_tags=
+maildir.synchronize_flags=true
+foo.bar=baz
+foo.list=xxx;yyy yyy;zzz zzz;"
+
 test_done
-- 
1.7.4.4

[PATCH v3 4/5] config: Add 'config list' command

[Peter Wang]

Add a command to list all configuration items with their associated
values.

One use is as follows: a MUA may prefer to store data in a central
notmuch configuration file so that the data is accessible across
different machines, e.g. an addressbook.  The list command helps
to implement features such as tab completion on the keys.
---
 notmuch-config.c |   66 ++++++++++++++++++++++++++++++++++++++++++++++++++---
 test/config      |    1 -
 2 files changed, 62 insertions(+), 5 deletions(-)

diff --git a/notmuch-config.c b/notmuch-config.c
index 85fc774..d5540ac 100644
--- a/notmuch-config.c
+++ b/notmuch-config.c
@@ -799,20 +799,78 @@ notmuch_config_command_set (void *ctx, char *item, int argc, char *argv[])
     return ret;
 }
 
+static int
+notmuch_config_command_list (void *ctx)
+{
+    notmuch_config_t *config;
+    char **groups;
+    size_t g, groups_length;
+
+    config = notmuch_config_open (ctx, NULL, NULL);
+    if (config == NULL)
+	return 1;
+
+    groups = g_key_file_get_groups (config->key_file, &groups_length);
+    if (groups == NULL)
+	return 1;
+
+    for (g = 0; g < groups_length; g++) {
+	char **keys;
+	size_t k, keys_length;
+
+	keys = g_key_file_get_keys (config->key_file,
+				    groups[g], &keys_length, NULL);
+	if (keys == NULL)
+	    continue;
+
+	for (k = 0; k < keys_length; k++) {
+	    char *value;
+
+	    value = g_key_file_get_string (config->key_file,
+					   groups[g], keys[k], NULL);
+	    if (value != NULL) {
+		printf ("%s.%s=%s\n", groups[g], keys[k], value);
+		free (value);
+	    }
+	}
+
+	g_strfreev (keys);
+    }
+
+    g_strfreev (groups);
+
+    notmuch_config_close (config);
+
+    return 0;
+}
+
 int
 notmuch_config_command (void *ctx, int argc, char *argv[])
 {
     argc--; argv++; /* skip subcommand argument */
 
-    if (argc < 2) {
-	fprintf (stderr, "Error: notmuch config requires at least two arguments.\n");
+    if (argc < 1) {
+	fprintf (stderr, "Error: notmuch config requires at least one argument.\n");
 	return 1;
     }
 
-    if (strcmp (argv[0], "get") == 0)
+    if (strcmp (argv[0], "get") == 0) {
+	if (argc < 2) {
+	    fprintf (stderr, "Error: notmuch config get requires at least "
+		     "two arguments.\n");
+	    return 1;
+	}
 	return notmuch_config_command_get (ctx, argv[1]);
-    else if (strcmp (argv[0], "set") == 0)
+    } else if (strcmp (argv[0], "set") == 0) {
+	if (argc < 2) {
+	    fprintf (stderr, "Error: notmuch config set requires at least "
+		     "two arguments.\n");
+	    return 1;
+	}
 	return notmuch_config_command_set (ctx, argv[1], argc - 2, argv + 2);
+    } else if (strcmp (argv[0], "list") == 0) {
+	return notmuch_config_command_list (ctx);
+    }
 
     fprintf (stderr, "Unrecognized argument for notmuch config: %s\n",
 	     argv[0]);
diff --git a/test/config b/test/config
index cce2abc..0fbc79b 100755
--- a/test/config
+++ b/test/config
@@ -23,7 +23,6 @@ yyy yyy
 zzz zzz"
 
 test_begin_subtest "List all items"
-test_subtest_known_broken
 notmuch config set database.path "/canonical/path"
 output=$(notmuch config list)
 test_expect_equal "$output" "\
-- 
1.7.4.4

Re: [PATCH v3 4/5] config: Add 'config list' command

[Jameson Graef Rollins]

[-- Attachment #1: Type: text/plain, Size: 3156 bytes --]

On Thu, Apr 05 2012, Peter Wang <novalazy@gmail.com> wrote:
> Add a command to list all configuration items with their associated
> values.
>
> One use is as follows: a MUA may prefer to store data in a central
> notmuch configuration file so that the data is accessible across
> different machines, e.g. an addressbook.  The list command helps
> to implement features such as tab completion on the keys.
> ---
>  notmuch-config.c |   66 ++++++++++++++++++++++++++++++++++++++++++++++++++---
>  test/config      |    1 -
>  2 files changed, 62 insertions(+), 5 deletions(-)
>
> diff --git a/notmuch-config.c b/notmuch-config.c
> index 85fc774..d5540ac 100644
> --- a/notmuch-config.c
> +++ b/notmuch-config.c
> @@ -799,20 +799,78 @@ notmuch_config_command_set (void *ctx, char *item, int argc, char *argv[])
>      return ret;
>  }
>  
> +static int
> +notmuch_config_command_list (void *ctx)
> +{
> +    notmuch_config_t *config;
> +    char **groups;
> +    size_t g, groups_length;
> +
> +    config = notmuch_config_open (ctx, NULL, NULL);
> +    if (config == NULL)
> +	return 1;
> +
> +    groups = g_key_file_get_groups (config->key_file, &groups_length);
> +    if (groups == NULL)
> +	return 1;
> +
> +    for (g = 0; g < groups_length; g++) {
> +	char **keys;
> +	size_t k, keys_length;
> +
> +	keys = g_key_file_get_keys (config->key_file,
> +				    groups[g], &keys_length, NULL);
> +	if (keys == NULL)
> +	    continue;
> +
> +	for (k = 0; k < keys_length; k++) {
> +	    char *value;
> +
> +	    value = g_key_file_get_string (config->key_file,
> +					   groups[g], keys[k], NULL);
> +	    if (value != NULL) {
> +		printf ("%s.%s=%s\n", groups[g], keys[k], value);
> +		free (value);
> +	    }
> +	}
> +
> +	g_strfreev (keys);
> +    }
> +
> +    g_strfreev (groups);
> +
> +    notmuch_config_close (config);
> +
> +    return 0;
> +}
> +
>  int
>  notmuch_config_command (void *ctx, int argc, char *argv[])
>  {
>      argc--; argv++; /* skip subcommand argument */
>  
> -    if (argc < 2) {
> -	fprintf (stderr, "Error: notmuch config requires at least two arguments.\n");
> +    if (argc < 1) {
> +	fprintf (stderr, "Error: notmuch config requires at least one argument.\n");
>  	return 1;
>      }

Hey, Peter.  I would say everything up to here looks great.

> -    if (strcmp (argv[0], "get") == 0)
> +    if (strcmp (argv[0], "get") == 0) {
> +	if (argc < 2) {
> +	    fprintf (stderr, "Error: notmuch config get requires at least "
> +		     "two arguments.\n");
> +	    return 1;
> +	}
>  	return notmuch_config_command_get (ctx, argv[1]);
> -    else if (strcmp (argv[0], "set") == 0)
> +    } else if (strcmp (argv[0], "set") == 0) {
> +	if (argc < 2) {
> +	    fprintf (stderr, "Error: notmuch config set requires at least "
> +		     "two arguments.\n");
> +	    return 1;
> +	}
>  	return notmuch_config_command_set (ctx, argv[1], argc - 2, argv + 2);

But then these changes look unrelated to me.  They do look good
intentioned, though.  It's probably best to submit these changes in a
separate unrelated patch.

jamie.

[-- Attachment #2: Type: application/pgp-signature, Size: 835 bytes --]

Re: [PATCH v3 4/5] config: Add 'config list' command

[Peter Wang]

On Tue, 10 Apr 2012 00:22:01 -0700, Jameson Graef Rollins <jrollins@finestructure.net> wrote:
> On Thu, Apr 05 2012, Peter Wang <novalazy@gmail.com> wrote:
> 
> > -    if (strcmp (argv[0], "get") == 0)
> > +    if (strcmp (argv[0], "get") == 0) {
> > +	if (argc < 2) {
> > +	    fprintf (stderr, "Error: notmuch config get requires at least "
> > +		     "two arguments.\n");
> > +	    return 1;
> > +	}
> >  	return notmuch_config_command_get (ctx, argv[1]);
> > -    else if (strcmp (argv[0], "set") == 0)
> > +    } else if (strcmp (argv[0], "set") == 0) {
> > +	if (argc < 2) {
> > +	    fprintf (stderr, "Error: notmuch config set requires at least "
> > +		     "two arguments.\n");
> > +	    return 1;
> > +	}
> >  	return notmuch_config_command_set (ctx, argv[1], argc - 2, argv + 2);
> 
> But then these changes look unrelated to me.  They do look good
> intentioned, though.  It's probably best to submit these changes in a
> separate unrelated patch.

Well, the only reason to duplicate the arity check is due to the
introduction of the 'list' subcommand.  But I see that the error
messages are wrong anyway, so I will separate out the changes in another
patch series.

Peter

[PATCH v3 5/5] man: Document 'config list' command

[Peter Wang]

Document the 'config list' command and its output.
---
 man/man1/notmuch-config.1 |   14 ++++++++++++++
 1 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/man/man1/notmuch-config.1 b/man/man1/notmuch-config.1
index 395cb9c..a55dbef 100644
--- a/man/man1/notmuch-config.1
+++ b/man/man1/notmuch-config.1
@@ -9,6 +9,8 @@ notmuch-config \- Access notmuch configuration file.
 .B notmuch config set
 .RI  "<" section ">.<" item "> [" value " ...]"
 
+.B notmuch config list
+
 .SH DESCRIPTION
 
 The
@@ -35,6 +37,18 @@ If no values are provided, the specified configuration item will be
 removed from the configuration file.
 .RE
 
+.RS 4
+.TP 4
+.B list
+Every configuration item is printed to stdout, each on a separate line
+of the form:
+
+.RI  "" section "." item "=" value
+
+No additional whitespace surrounds the dot or equals sign characters. In a
+multiple-value item (a list), the values are separated by semicolon characters.
+.RE
+
 The available configuration items are described below.
 
 .RS 4
-- 
1.7.4.4

[PATCH v4 0/6] Config-related patches

[Peter Wang]

Changes from v3:
- rephrase part of the 'list' implementation as a separate patch
- test 'set' on an extant key
- test removing keys

Peter Wang (6):
  config: Fix free in 'config get' implementation.
  config: Check 'config get' arity exactly
  test: Add tests for 'config' command
  test: Add broken test for 'config list'
  config: Add 'config list' command
  man: Document 'config list' command

 man/man1/notmuch-config.1 |   14 +++++++++
 notmuch-config.c          |   68 +++++++++++++++++++++++++++++++++++++++++---
 test/config               |   60 +++++++++++++++++++++++++++++++++++++++
 test/notmuch-test         |    1 +
 4 files changed, 138 insertions(+), 5 deletions(-)
 create mode 100755 test/config

-- 
1.7.4.4

[PATCH v4 1/6] config: Fix free in 'config get' implementation.

[Peter Wang]

The array returned by g_key_file_get_string_list() should be freed with
g_strfreev(), not free().
---
 notmuch-config.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/notmuch-config.c b/notmuch-config.c
index e9b2750..85fc774 100644
--- a/notmuch-config.c
+++ b/notmuch-config.c
@@ -751,7 +751,7 @@ notmuch_config_command_get (void *ctx, char *item)
 	for (i = 0; i < length; i++)
 	    printf ("%s\n", value[i]);
 
-	free (value);
+	g_strfreev (value);
     }
 
     notmuch_config_close (config);
-- 
1.7.4.4

Re: [PATCH v4 1/6] config: Fix free in 'config get' implementation.

[David Bremner]

Peter Wang <novalazy@gmail.com> writes:

> The array returned by g_key_file_get_string_list() should be freed with
> g_strfreev(), not free().

Pushed this one patch from the series.

d

[PATCH v4 2/6] config: Check 'config get' arity exactly

[Peter Wang]

Require that 'config get' is passed exactly one additional argument,
instead of silently ignoring extra arguments. As a side-effect, produce
more specific error messages for the 'config' command as a whole.
---
 notmuch-config.c |   19 +++++++++++++++----
 1 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/notmuch-config.c b/notmuch-config.c
index 85fc774..f9eb977 100644
--- a/notmuch-config.c
+++ b/notmuch-config.c
@@ -804,15 +804,26 @@ notmuch_config_command (void *ctx, int argc, char *argv[])
 {
     argc--; argv++; /* skip subcommand argument */
 
-    if (argc < 2) {
-	fprintf (stderr, "Error: notmuch config requires at least two arguments.\n");
+    if (argc < 1) {
+	fprintf (stderr, "Error: notmuch config requires at least one argument.\n");
 	return 1;
     }
 
-    if (strcmp (argv[0], "get") == 0)
+    if (strcmp (argv[0], "get") == 0) {
+	if (argc != 2) {
+	    fprintf (stderr, "Error: notmuch config get requires exactly "
+		     "one argument.\n");
+	    return 1;
+	}
 	return notmuch_config_command_get (ctx, argv[1]);
-    else if (strcmp (argv[0], "set") == 0)
+    } else if (strcmp (argv[0], "set") == 0) {
+	if (argc < 2) {
+	    fprintf (stderr, "Error: notmuch config set requires at least "
+		     "one argument.\n");
+	    return 1;
+	}
 	return notmuch_config_command_set (ctx, argv[1], argc - 2, argv + 2);
+    }
 
     fprintf (stderr, "Unrecognized argument for notmuch config: %s\n",
 	     argv[0]);
-- 
1.7.4.4

Re: [PATCH v4 2/6] config: Check 'config get' arity exactly

[David Bremner]

Peter Wang <novalazy@gmail.com> writes:

> Require that 'config get' is passed exactly one additional argument,
> instead of silently ignoring extra arguments. As a side-effect, produce
> more specific error messages for the 'config' command as a whole.

Hi Peter;

I pushed the rest of the series to master. Would you mind making patch
to NEWS describing the new feature?

thanks

d

[PATCH v4 3/6] test: Add tests for 'config' command

[Peter Wang]

Start a new test script.
---
 test/config       |   45 +++++++++++++++++++++++++++++++++++++++++++++
 test/notmuch-test |    1 +
 2 files changed, 46 insertions(+), 0 deletions(-)
 create mode 100755 test/config

diff --git a/test/config b/test/config
new file mode 100755
index 0000000..9030154
--- /dev/null
+++ b/test/config
@@ -0,0 +1,45 @@
+#!/usr/bin/env bash
+
+test_description='"notmuch config"'
+. test-lib.sh
+
+test_begin_subtest "Get string value"
+test_expect_equal "$(notmuch config get user.name)" "Notmuch Test Suite"
+
+test_begin_subtest "Get list value"
+test_expect_equal "$(notmuch config get new.tags)" "\
+unread
+inbox"
+
+test_begin_subtest "Set string value"
+notmuch config set foo.string "this is a string value"
+test_expect_equal "$(notmuch config get foo.string)" "this is a string value"
+
+test_begin_subtest "Set string value again"
+notmuch config set foo.string "this is another string value"
+test_expect_equal "$(notmuch config get foo.string)" "this is another string value"
+
+test_begin_subtest "Set list value"
+notmuch config set foo.list this "is a" "list value"
+test_expect_equal "$(notmuch config get foo.list)" "\
+this
+is a
+list value"
+
+test_begin_subtest "Set list value again"
+notmuch config set foo.list this "is another" "list value"
+test_expect_equal "$(notmuch config get foo.list)" "\
+this
+is another
+list value"
+
+test_begin_subtest "Remove key"
+notmuch config set foo.remove baz
+notmuch config set foo.remove
+test_expect_equal "$(notmuch config get foo.remove)" ""
+
+test_begin_subtest "Remove non-existent key"
+notmuch config set foo.nonexistent
+test_expect_equal "$(notmuch config get foo.nonexistent)" ""
+
+test_done
diff --git a/test/notmuch-test b/test/notmuch-test
index f03b594..e08ec72 100755
--- a/test/notmuch-test
+++ b/test/notmuch-test
@@ -19,6 +19,7 @@ cd $(dirname "$0")
 TESTS="
   basic
   help-test
+  config
   new
   count
   search
-- 
1.7.4.4

[PATCH v4 4/6] test: Add broken test for 'config list'

[Peter Wang]

Proposed functionality.
---
 test/config |   16 ++++++++++++++++
 1 files changed, 16 insertions(+), 0 deletions(-)

diff --git a/test/config b/test/config
index 9030154..3bf8098 100755
--- a/test/config
+++ b/test/config
@@ -42,4 +42,20 @@ test_begin_subtest "Remove non-existent key"
 notmuch config set foo.nonexistent
 test_expect_equal "$(notmuch config get foo.nonexistent)" ""
 
+test_begin_subtest "List all items"
+test_subtest_known_broken
+notmuch config set database.path "/canonical/path"
+output=$(notmuch config list)
+test_expect_equal "$output" "\
+database.path=/canonical/path
+user.name=Notmuch Test Suite
+user.primary_email=test_suite@notmuchmail.org
+user.other_email=test_suite_other@notmuchmail.org;test_suite@otherdomain.org
+new.tags=unread;inbox;
+new.ignore=
+search.exclude_tags=
+maildir.synchronize_flags=true
+foo.string=this is another string value
+foo.list=this;is another;list value;"
+
 test_done
-- 
1.7.4.4

[PATCH v4 5/6] config: Add 'config list' command

[Peter Wang]

Add a command to list all configuration items with their associated
values.

One use is as follows: a MUA may prefer to store data in a central
notmuch configuration file so that the data is accessible across
different machines, e.g. an addressbook.  The list command helps
to implement features such as tab completion on the keys.
---
 notmuch-config.c |   47 +++++++++++++++++++++++++++++++++++++++++++++++
 test/config      |    1 -
 2 files changed, 47 insertions(+), 1 deletions(-)

diff --git a/notmuch-config.c b/notmuch-config.c
index f9eb977..3e37a2d 100644
--- a/notmuch-config.c
+++ b/notmuch-config.c
@@ -799,6 +799,51 @@ notmuch_config_command_set (void *ctx, char *item, int argc, char *argv[])
     return ret;
 }
 
+static int
+notmuch_config_command_list (void *ctx)
+{
+    notmuch_config_t *config;
+    char **groups;
+    size_t g, groups_length;
+
+    config = notmuch_config_open (ctx, NULL, NULL);
+    if (config == NULL)
+	return 1;
+
+    groups = g_key_file_get_groups (config->key_file, &groups_length);
+    if (groups == NULL)
+	return 1;
+
+    for (g = 0; g < groups_length; g++) {
+	char **keys;
+	size_t k, keys_length;
+
+	keys = g_key_file_get_keys (config->key_file,
+				    groups[g], &keys_length, NULL);
+	if (keys == NULL)
+	    continue;
+
+	for (k = 0; k < keys_length; k++) {
+	    char *value;
+
+	    value = g_key_file_get_string (config->key_file,
+					   groups[g], keys[k], NULL);
+	    if (value != NULL) {
+		printf ("%s.%s=%s\n", groups[g], keys[k], value);
+		free (value);
+	    }
+	}
+
+	g_strfreev (keys);
+    }
+
+    g_strfreev (groups);
+
+    notmuch_config_close (config);
+
+    return 0;
+}
+
 int
 notmuch_config_command (void *ctx, int argc, char *argv[])
 {
@@ -823,6 +868,8 @@ notmuch_config_command (void *ctx, int argc, char *argv[])
 	    return 1;
 	}
 	return notmuch_config_command_set (ctx, argv[1], argc - 2, argv + 2);
+    } else if (strcmp (argv[0], "list") == 0) {
+	return notmuch_config_command_list (ctx);
     }
 
     fprintf (stderr, "Unrecognized argument for notmuch config: %s\n",
diff --git a/test/config b/test/config
index 3bf8098..93ecb13 100755
--- a/test/config
+++ b/test/config
@@ -43,7 +43,6 @@ notmuch config set foo.nonexistent
 test_expect_equal "$(notmuch config get foo.nonexistent)" ""
 
 test_begin_subtest "List all items"
-test_subtest_known_broken
 notmuch config set database.path "/canonical/path"
 output=$(notmuch config list)
 test_expect_equal "$output" "\
-- 
1.7.4.4

[PATCH v4 6/6] man: Document 'config list' command

[Peter Wang]

Document the 'config list' command and its output.
---
 man/man1/notmuch-config.1 |   14 ++++++++++++++
 1 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/man/man1/notmuch-config.1 b/man/man1/notmuch-config.1
index 395cb9c..a55dbef 100644
--- a/man/man1/notmuch-config.1
+++ b/man/man1/notmuch-config.1
@@ -9,6 +9,8 @@ notmuch-config \- Access notmuch configuration file.
 .B notmuch config set
 .RI  "<" section ">.<" item "> [" value " ...]"
 
+.B notmuch config list
+
 .SH DESCRIPTION
 
 The
@@ -35,6 +37,18 @@ If no values are provided, the specified configuration item will be
 removed from the configuration file.
 .RE
 
+.RS 4
+.TP 4
+.B list
+Every configuration item is printed to stdout, each on a separate line
+of the form:
+
+.RI  "" section "." item "=" value
+
+No additional whitespace surrounds the dot or equals sign characters. In a
+multiple-value item (a list), the values are separated by semicolon characters.
+.RE
+
 The available configuration items are described below.
 
 .RS 4
-- 
1.7.4.4

Re: [PATCH v4 0/6] Config-related patches

[Mark Walters]

On Sat, 14 Apr 2012, Peter Wang <novalazy@gmail.com> wrote:
> Changes from v3:
> - rephrase part of the 'list' implementation as a separate patch
> - test 'set' on an extant key
> - test removing keys

This looks good to me. +1

Two minor comments which you might like to consider (but definitely are
not required):
    1) You could check that there are no further arguments when the user
    calls `notmuch config list'
    2) In the man page you could explicitly say what the output is for a
    configuration item which has not been set.

Best wishes

Mark



    


> Peter Wang (6):
>   config: Fix free in 'config get' implementation.
>   config: Check 'config get' arity exactly
>   test: Add tests for 'config' command
>   test: Add broken test for 'config list'
>   config: Add 'config list' command
>   man: Document 'config list' command
>
>  man/man1/notmuch-config.1 |   14 +++++++++
>  notmuch-config.c          |   68 +++++++++++++++++++++++++++++++++++++++++---
>  test/config               |   60 +++++++++++++++++++++++++++++++++++++++
>  test/notmuch-test         |    1 +
>  4 files changed, 138 insertions(+), 5 deletions(-)
>  create mode 100755 test/config
>
> -- 
> 1.7.4.4
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH v4 0/6] Config-related patches

[Jameson Graef Rollins]

[-- Attachment #1: Type: text/plain, Size: 619 bytes --]

On Sat, Apr 14 2012, Mark Walters <markwalters1009@gmail.com> wrote:
> Two minor comments which you might like to consider (but definitely are
> not required):
>     1) You could check that there are no further arguments when the user
>     calls `notmuch config list'

This doesn't seem so necessary to me.  I'm fine if it just silently
ignores extra arguments.

>     2) In the man page you could explicitly say what the output is for a
>     configuration item which has not been set.

I'm also fine with it as it is.  I think it's implied that if the value
is null then the field is left blank.  That's ok.

jamie.

[-- Attachment #2: Type: application/pgp-signature, Size: 835 bytes --]

Re: [PATCH v4 0/6] Config-related patches

[Mark Walters]

On Sat, 14 Apr 2012, Mark Walters <markwalters1009@gmail.com> wrote:
> On Sat, 14 Apr 2012, Peter Wang <novalazy@gmail.com> wrote:
>> Changes from v3:
>> - rephrase part of the 'list' implementation as a separate patch
>> - test 'set' on an extant key
>> - test removing keys
>
> This looks good to me. +1
>
> Two minor comments which you might like to consider (but definitely are
> not required):
>     1) You could check that there are no further arguments when the user
>     calls `notmuch config list'
>     2) In the man page you could explicitly say what the output is for a
>     configuration item which has not been set.
>
> Best wishes
>
> Mark

Just to make it completely clear the above was not meant to delay
acceptance of this series: I am quite happy with it as is.

Best wishes

Mark

Re: [PATCH v4 0/6] Config-related patches

[Jameson Graef Rollins]

[-- Attachment #1: Type: text/plain, Size: 232 bytes --]

On Fri, Apr 13 2012, Peter Wang <novalazy@gmail.com> wrote:
> Changes from v3:
> - rephrase part of the 'list' implementation as a separate patch
> - test 'set' on an extant key
> - test removing keys

LGTM, tested, and +1.

jamie.

[-- Attachment #2: Type: application/pgp-signature, Size: 835 bytes --]