Strip spaces in `tags` in `~/.notmuch-config` (and other fields)

Strip spaces in `tags` in `~/.notmuch-config` (and other fields)

[Ciprian Dorin Craciun]

[Sorry if I'm double reporting this.  I've tried my best to search for
previous discussions.]


I've tried to manually edit my `~/.notmuch-config`, and I've seen that
the field `tags` was written as `tags=unread;inbox;`.  In order to
increase readability I've decided to update my configuration file by
adding spaces around `=` and `;` as in `tags = unread ; inbox ;`.
Everything worked without a warning, until it didn't...  :)

What happened:  all my emails are now tagged with `unread ` and `
inbox `;  (i.e. whitespace in tags).


Given that the `~/.notmuch-config` resembles an INI file, and given
how lax the actual syntax is in general, I would suggest the
following:

* allow white-spaces around `[ section ]`, and `field = value`;
* strip white-spaces (left and right) from values like `tags = unread
; inbox ;`;  (but not infix like `tag = some tag ; some other tag;`;)
* allow skipping the last `;` separator from `tags` and similar;

Failing that, perhaps add a warning when parsing the configuration file.


Hope it helps,
Ciprian.

Re: Strip spaces in `tags` in `~/.notmuch-config` (and other fields)

[David Bremner]

Ciprian Dorin Craciun <ciprian.craciun@gmail.com> writes:
>
> I've tried to manually edit my `~/.notmuch-config`, and I've seen that
> the field `tags` was written as `tags=unread;inbox;`.  In order to
> increase readability I've decided to update my configuration file by
> adding spaces around `=` and `;` as in `tags = unread ; inbox ;`.
> Everything worked without a warning, until it didn't...  :)
>
> What happened:  all my emails are now tagged with `unread ` and `
> inbox `;  (i.e. whitespace in tags).
>

Digging into the code a bit, the culprit seems to be _config_get_list.
It is a bit surprising that the glib function g_key_file_get_string_list
isn't a bit friendlier here (or at least better documented), but I guess
the output of it needs to be postprocessed.

d

Re: Strip spaces in `tags` in `~/.notmuch-config` (and other fields)

[David Bremner]

Ciprian Dorin Craciun <ciprian.craciun@gmail.com> writes:
>
> Given that the `~/.notmuch-config` resembles an INI file, and given
> how lax the actual syntax is in general, I would suggest the
> following:
>
> * allow white-spaces around `[ section ]`, and `field = value`;

This is somewhat out of our control, as we rely on glib to parse these
files.

> * strip white-spaces (left and right) from values like `tags = unread
> ; inbox ;`;  (but not infix like `tag = some tag ; some other tag;`;)

I will shortly send a patch to implement this. It has the potential
issue that it is no longer possible to enter tags with leading/trailing
spaces in the config file. That doesn't seem like a big deal to me, but
I guess we'll see.

> * allow skipping the last `;` separator from `tags` and similar;
>

This should be working now. The last ; is optional (since notmuch 0.32,
I think).

> Failing that, perhaps add a warning when parsing the configuration file.

This again comes down to the config file parser we are using.  However,
we can (and do) scan the tags afterwards for certain issues, which we
could further extend.

[PATCH 1/2] test: known broken tests for leading/trailing ws in config

[David Bremner]

These tests duplicate the bug/misfeature reported in

      id:CA+Tk8fzjPLaEd3vL1f9ebk_bF_RV8PDTLzDupraTkCLCpJAmCg@mail.gmail.com
---
 test/T050-new.sh       | 13 +++++++++++++
 test/T070-insert.sh    | 13 +++++++++++++
 test/T590-libconfig.sh | 24 ++++++++++++++++++++++++
 3 files changed, 50 insertions(+)

diff --git a/test/T050-new.sh b/test/T050-new.sh
index 1141c1e3..bc20440b 100755
--- a/test/T050-new.sh
+++ b/test/T050-new.sh
@@ -329,6 +329,19 @@ notmuch config set new.tags "foo;;bar"
 output=$(NOTMUCH_NEW --quiet 2>&1)
 test_expect_equal "$output" ""
 
+test_begin_subtest "leading/trailing whitespace in new.tags is ignored"
+test_subtest_known_broken
+# avoid complications with leading spaces and "notmuch config"
+sed -i 's/^tags=.*$/tags= fu bar ; ; bar /' notmuch-config
+add_message
+NOTMUCH_NEW --quiet
+notmuch dump id:$gen_msg_id | sed 's/ --.*$//' > OUTPUT
+cat <<EOF >EXPECTED
+#notmuch-dump batch-tag:3 config,properties,tags
++bar +fu%20bar
+EOF
+test_expect_equal_file EXPECTED OUTPUT
+
 test_begin_subtest "Tags starting with '-' in new.tags are forbidden"
 notmuch config set new.tags "-foo;bar"
 output=$(NOTMUCH_NEW --debug 2>&1)
diff --git a/test/T070-insert.sh b/test/T070-insert.sh
index 208deb1c..9d29c859 100755
--- a/test/T070-insert.sh
+++ b/test/T070-insert.sh
@@ -234,6 +234,19 @@ output=$(notmuch show --format=json id:$gen_msg_id)
 test_json_nodes <<<"$output" \
 		'new_tags:[0][0][0]["tags"] = ["bar", "foo"]'
 
+test_begin_subtest "leading/trailing whitespace in new.tags is ignored"
+test_subtest_known_broken
+# avoid complications with leading spaces and "notmuch config"
+sed -i 's/^tags=.*$/tags= fu bar ; ; bar /' notmuch-config
+gen_insert_msg
+notmuch insert < $gen_msg_filename
+notmuch dump id:$gen_msg_id | sed 's/ --.*$//' > OUTPUT
+cat <<EOF >EXPECTED
+#notmuch-dump batch-tag:3 config,properties,tags
++bar +fu%20bar
+EOF
+test_expect_equal_file EXPECTED OUTPUT
+
 test_begin_subtest "Tags starting with '-' in new.tags are forbidden"
 notmuch config set new.tags "-foo;bar"
 gen_insert_msg
diff --git a/test/T590-libconfig.sh b/test/T590-libconfig.sh
index 59b82a6f..88647940 100755
--- a/test/T590-libconfig.sh
+++ b/test/T590-libconfig.sh
@@ -272,6 +272,30 @@ EOF
 test_expect_equal_file EXPECTED OUTPUT
 restore_database
 
+test_begin_subtest "notmuch_config_get_values (ignore leading/trailing whitespace)"
+test_subtest_known_broken
+cat c_head - c_tail <<'EOF' | test_C ${MAIL_DIR} ${NOTMUCH_CONFIG} %NULL%
+{
+    notmuch_config_values_t *values;
+    EXPECT0(notmuch_config_set (db, NOTMUCH_CONFIG_NEW_TAGS, " a ; b c ; d "));
+    for (values = notmuch_config_get_values (db, NOTMUCH_CONFIG_NEW_TAGS);
+	 notmuch_config_values_valid (values);
+	 notmuch_config_values_move_to_next (values))
+    {
+	  puts (notmuch_config_values_get (values));
+    }
+}
+EOF
+cat <<'EOF' >EXPECTED
+== stdout ==
+a
+b c
+d
+== stderr ==
+EOF
+test_expect_equal_file EXPECTED OUTPUT
+restore_database
+
 test_begin_subtest "notmuch_config_get_values_string"
 cat c_head - c_tail <<'EOF' | test_C ${MAIL_DIR} ${NOTMUCH_CONFIG} %NULL%
 {
-- 
2.33.0

[PATCH 2/2] config: ignore leading/trailing spaces in ';'-delimited lists

[David Bremner]

In [1] Ciprian observed that it was easy for users to mistakenly
introduce leading and trailing space to new.tags when editing a
notmuch config file. This commit strips spaces on either side of the
';' delimiter when splitting.

In principle it would be possible to support tags (or other config
values) with leading or trailing spaces by processing '\s' escapes in
the input string. Currently such processing is not done.

[1]: id:CA+Tk8fzjPLaEd3vL1f9ebk_bF_RV8PDTLzDupraTkCLCpJAmCg@mail.gmail.com
---
 test/T050-new.sh       |  1 -
 test/T070-insert.sh    |  1 -
 test/T590-libconfig.sh |  1 -
 util/string-util.c     | 10 ++++++----
 4 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/test/T050-new.sh b/test/T050-new.sh
index bc20440b..69697c48 100755
--- a/test/T050-new.sh
+++ b/test/T050-new.sh
@@ -330,7 +330,6 @@ output=$(NOTMUCH_NEW --quiet 2>&1)
 test_expect_equal "$output" ""
 
 test_begin_subtest "leading/trailing whitespace in new.tags is ignored"
-test_subtest_known_broken
 # avoid complications with leading spaces and "notmuch config"
 sed -i 's/^tags=.*$/tags= fu bar ; ; bar /' notmuch-config
 add_message
diff --git a/test/T070-insert.sh b/test/T070-insert.sh
index 9d29c859..ec170b30 100755
--- a/test/T070-insert.sh
+++ b/test/T070-insert.sh
@@ -235,7 +235,6 @@ test_json_nodes <<<"$output" \
 		'new_tags:[0][0][0]["tags"] = ["bar", "foo"]'
 
 test_begin_subtest "leading/trailing whitespace in new.tags is ignored"
-test_subtest_known_broken
 # avoid complications with leading spaces and "notmuch config"
 sed -i 's/^tags=.*$/tags= fu bar ; ; bar /' notmuch-config
 gen_insert_msg
diff --git a/test/T590-libconfig.sh b/test/T590-libconfig.sh
index 88647940..768e6576 100755
--- a/test/T590-libconfig.sh
+++ b/test/T590-libconfig.sh
@@ -273,7 +273,6 @@ test_expect_equal_file EXPECTED OUTPUT
 restore_database
 
 test_begin_subtest "notmuch_config_get_values (ignore leading/trailing whitespace)"
-test_subtest_known_broken
 cat c_head - c_tail <<'EOF' | test_C ${MAIL_DIR} ${NOTMUCH_CONFIG} %NULL%
 {
     notmuch_config_values_t *values;
diff --git a/util/string-util.c b/util/string-util.c
index 9c46a81a..03d7648d 100644
--- a/util/string-util.c
+++ b/util/string-util.c
@@ -42,13 +42,15 @@ const char *
 strsplit_len (const char *s, char delim, size_t *len)
 {
     bool escaping = false;
-    size_t count = 0;
+    size_t count = 0, last_nonspace = 0;
 
-    /* Skip initial unescaped delimiters */
-    while (*s && *s == delim)
+    /* Skip initial unescaped delimiters and whitespace */
+    while (*s && (*s == delim || isspace (*s)))
 	s++;
 
     while (s[count] && (escaping || s[count] != delim)) {
+	if (! isspace (s[count]))
+	    last_nonspace = count;
 	escaping = (s[count] == '\\');
 	count++;
     }
@@ -56,7 +58,7 @@ strsplit_len (const char *s, char delim, size_t *len)
     if (count == 0)
 	return NULL;
 
-    *len = count;
+    *len = last_nonspace + 1;
     return s;
 }
 
-- 
2.33.0

[PATCH 1/2] test: known broken tests for escape characters in config files.

[David Bremner]

glib generates the following escape characters with their usual
meanings: \n, \t, \r, and \\, along with \s for _leading_
spaces. Currently notmuch fails to unescape these on reading the
config files. These tests demonstrate this bug; the one new test that
passes is because apparently glib only escapes tabs at the beginning
of a key.
---
 test/T030-config.sh | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/test/T030-config.sh b/test/T030-config.sh
index 3a585d1b..09dacda3 100755
--- a/test/T030-config.sh
+++ b/test/T030-config.sh
@@ -67,6 +67,37 @@ user.primary_email=test_suite@notmuchmail.org
 EOF
 test_expect_equal_file EXPECTED OUTPUT
 
+test_begin_subtest "Round trip config item with leading spaces"
+test_subtest_known_broken
+notmuch config set foo.bar "  thing"
+output=$(notmuch config get foo.bar)
+test_expect_equal "${output}" "  thing"
+
+test_begin_subtest "Round trip config item with leading tab"
+test_subtest_known_broken
+notmuch config set foo.bar "	thing"
+output=$(notmuch config get foo.bar)
+test_expect_equal "${output}" "	thing"
+
+test_begin_subtest "Round trip config item with embedded tab"
+notmuch config set foo.bar "thing	other"
+output=$(notmuch config get foo.bar)
+test_expect_equal "${output}" "thing	other"
+
+test_begin_subtest "Round trip config item with embedded backslash"
+test_subtest_known_broken
+notmuch config set foo.bar 'thing\other'
+output=$(notmuch config get foo.bar)
+test_expect_equal "${output}" "thing\other"
+
+test_begin_subtest "Round trip config item with embedded NL/CR"
+test_subtest_known_broken
+notmuch config set foo.bar 'thing
+
other'
+output=$(notmuch config get foo.bar)
+test_expect_equal "${output}" "thing
+
other"
+
 test_begin_subtest "Top level --config=FILE option"
 cp "${NOTMUCH_CONFIG}" alt-config
 notmuch --config=alt-config config set user.name "Another Name"
-- 
2.33.0

[PATCH 2/2] lib/config: use g_key_file_get_string to read config values

[David Bremner]

Unlike the previous g_key_file_get_value, this version processes
escape codes for whitespace and \. The remaining two broken tests from
the last commit are because "notmuch config get" treats every value as
a list, and thus the previously introduces stripping of leading
whitespace applies.
---
 lib/config.cc | 2 +-
 lib/open.cc   | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/lib/config.cc b/lib/config.cc
index 8775b00a..d57240d1 100644
--- a/lib/config.cc
+++ b/lib/config.cc
@@ -427,7 +427,7 @@ _notmuch_config_load_from_file (notmuch_database_t *notmuch,
 	for (gchar **keys_p = keys; *keys_p; keys_p++) {
 	    char *absolute_key = talloc_asprintf (notmuch, "%s.%s", *grp,  *keys_p);
 	    char *normalized_val;
-	    val = g_key_file_get_value (file, *grp, *keys_p, NULL);
+	    val = g_key_file_get_string (file, *grp, *keys_p, NULL);
 	    if (! val) {
 		status = NOTMUCH_STATUS_FILE_ERROR;
 		goto DONE;
diff --git a/lib/open.cc b/lib/open.cc
index 8a835e98..0f44b858 100644
--- a/lib/open.cc
+++ b/lib/open.cc
@@ -199,7 +199,7 @@ _choose_database_path (void *ctx,
     }
 
     if (! *database_path && key_file) {
-	char *path = g_key_file_get_value (key_file, "database", "path", NULL);
+	char *path = g_key_file_get_string (key_file, "database", "path", NULL);
 	if (path) {
 	    if (path[0] == '/')
 		*database_path = talloc_strdup (ctx, path);
@@ -637,7 +637,7 @@ notmuch_database_create_with_config (const char *database_path,
 
     if (key_file && ! split) {
 	char *mail_root = notmuch_canonicalize_file_name (
-	    g_key_file_get_value (key_file, "database", "mail_root", NULL));
+	    g_key_file_get_string (key_file, "database", "mail_root", NULL));
 	char *db_path = notmuch_canonicalize_file_name (database_path);
 
 	split = (mail_root && (0 != strcmp (mail_root, db_path)));
-- 
2.33.0

[PATCH v2] lib/config: use g_key_file_get_string to read config values

[David Bremner]

Unlike the previous g_key_file_get_value, this version processes
escape codes for whitespace and \. The remaining two broken tests from
the last commit are because "notmuch config get" treats every value as
a list, and thus the previously introduces stripping of leading
whitespace applies.
---
 lib/config.cc       | 2 +-
 lib/open.cc         | 4 ++--
 test/T030-config.sh | 2 --
 3 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/lib/config.cc b/lib/config.cc
index 8775b00a..d57240d1 100644
--- a/lib/config.cc
+++ b/lib/config.cc
@@ -427,7 +427,7 @@ _notmuch_config_load_from_file (notmuch_database_t *notmuch,
 	for (gchar **keys_p = keys; *keys_p; keys_p++) {
 	    char *absolute_key = talloc_asprintf (notmuch, "%s.%s", *grp,  *keys_p);
 	    char *normalized_val;
-	    val = g_key_file_get_value (file, *grp, *keys_p, NULL);
+	    val = g_key_file_get_string (file, *grp, *keys_p, NULL);
 	    if (! val) {
 		status = NOTMUCH_STATUS_FILE_ERROR;
 		goto DONE;
diff --git a/lib/open.cc b/lib/open.cc
index 8a835e98..0f44b858 100644
--- a/lib/open.cc
+++ b/lib/open.cc
@@ -199,7 +199,7 @@ _choose_database_path (void *ctx,
     }
 
     if (! *database_path && key_file) {
-	char *path = g_key_file_get_value (key_file, "database", "path", NULL);
+	char *path = g_key_file_get_string (key_file, "database", "path", NULL);
 	if (path) {
 	    if (path[0] == '/')
 		*database_path = talloc_strdup (ctx, path);
@@ -637,7 +637,7 @@ notmuch_database_create_with_config (const char *database_path,
 
     if (key_file && ! split) {
 	char *mail_root = notmuch_canonicalize_file_name (
-	    g_key_file_get_value (key_file, "database", "mail_root", NULL));
+	    g_key_file_get_string (key_file, "database", "mail_root", NULL));
 	char *db_path = notmuch_canonicalize_file_name (database_path);
 
 	split = (mail_root && (0 != strcmp (mail_root, db_path)));
diff --git a/test/T030-config.sh b/test/T030-config.sh
index 09dacda3..aacdb8c6 100755
--- a/test/T030-config.sh
+++ b/test/T030-config.sh
@@ -85,13 +85,11 @@ output=$(notmuch config get foo.bar)
 test_expect_equal "${output}" "thing	other"
 
 test_begin_subtest "Round trip config item with embedded backslash"
-test_subtest_known_broken
 notmuch config set foo.bar 'thing\other'
 output=$(notmuch config get foo.bar)
 test_expect_equal "${output}" "thing\other"
 
 test_begin_subtest "Round trip config item with embedded NL/CR"
-test_subtest_known_broken
 notmuch config set foo.bar 'thing
 
other'
 output=$(notmuch config get foo.bar)
-- 
2.33.0

Re: [PATCH 1/2] test: known broken tests for leading/trailing ws in config

[David Bremner]

David Bremner <david@tethera.net> writes:

> These tests duplicate the bug/misfeature reported in
>
>       id:CA+Tk8fzjPLaEd3vL1f9ebk_bF_RV8PDTLzDupraTkCLCpJAmCg@mail.gmail.com

This series, and the next one in the same thread, applied to master.

d

Re: Strip spaces in `tags` in `~/.notmuch-config` (and other fields)

[David Bremner]

Ciprian Dorin Craciun <ciprian.craciun@gmail.com> writes:
>
> Given that the `~/.notmuch-config` resembles an INI file, and given
> how lax the actual syntax is in general, I would suggest the
> following:
>
> * allow white-spaces around `[ section ]`, and `field = value`;
> * strip white-spaces (left and right) from values like `tags = unread
> ; inbox ;`;  (but not infix like `tag = some tag ; some other tag;`;)
> * allow skipping the last `;` separator from `tags` and similar;
>
> Failing that, perhaps add a warning when parsing the configuration file.
>

This should be fixed in 0.34.1-37-gc0115288, to be part of 0.35

There are one or two remaining corner cases involving escaped spaces and
tabs, but that goes above and beyond this request.

d

Re: Strip spaces in `tags` in `~/.notmuch-config` (and other fields)

[Ciprian Craciun]

On Sun, Dec 5, 2021 at 2:41 PM David Bremner <david@tethera.net> wrote:
> This should be fixed in 0.34.1-37-gc0115288, to be part of 0.35
>
> There are one or two remaining corner cases involving escaped spaces and
> tabs, but that goes above and beyond this request.


Thanks!

I'll test it once the new version hits the OpenSUSE rolling release
repositories.

Ciprian.