unofficial mirror of notmuch@notmuchmail.org
 help / color / mirror / code / Atom feed
* Strip spaces in `tags` in `~/.notmuch-config` (and other fields)
@ 2020-04-24 10:36 Ciprian Dorin Craciun
  2020-04-24 11:12 ` David Bremner
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Ciprian Dorin Craciun @ 2020-04-24 10:36 UTC (permalink / raw)
  To: notmuch

[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.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: Strip spaces in `tags` in `~/.notmuch-config` (and other fields)
  2020-04-24 10:36 Strip spaces in `tags` in `~/.notmuch-config` (and other fields) Ciprian Dorin Craciun
@ 2020-04-24 11:12 ` David Bremner
  2021-09-30 17:17 ` David Bremner
  2021-09-30 17:17 ` [PATCH 1/2] test: known broken tests for leading/trailing ws in config David Bremner
  2 siblings, 0 replies; 8+ messages in thread
From: David Bremner @ 2020-04-24 11:12 UTC (permalink / raw)
  To: Ciprian Dorin Craciun, notmuch

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: Strip spaces in `tags` in `~/.notmuch-config` (and other fields)
  2020-04-24 10:36 Strip spaces in `tags` in `~/.notmuch-config` (and other fields) Ciprian Dorin Craciun
  2020-04-24 11:12 ` David Bremner
@ 2021-09-30 17:17 ` David Bremner
  2021-09-30 17:17 ` [PATCH 1/2] test: known broken tests for leading/trailing ws in config David Bremner
  2 siblings, 0 replies; 8+ messages in thread
From: David Bremner @ 2021-09-30 17:17 UTC (permalink / raw)
  To: Ciprian Dorin Craciun, notmuch

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.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 1/2] test: known broken tests for leading/trailing ws in config
  2020-04-24 10:36 Strip spaces in `tags` in `~/.notmuch-config` (and other fields) Ciprian Dorin Craciun
  2020-04-24 11:12 ` David Bremner
  2021-09-30 17:17 ` David Bremner
@ 2021-09-30 17:17 ` David Bremner
  2021-09-30 17:17   ` [PATCH 2/2] config: ignore leading/trailing spaces in ';'-delimited lists David Bremner
  2 siblings, 1 reply; 8+ messages in thread
From: David Bremner @ 2021-09-30 17:17 UTC (permalink / raw)
  To: notmuch, Ciprian Dorin Craciun; +Cc: 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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 2/2] config: ignore leading/trailing spaces in ';'-delimited lists
  2021-09-30 17:17 ` [PATCH 1/2] test: known broken tests for leading/trailing ws in config David Bremner
@ 2021-09-30 17:17   ` David Bremner
  2021-09-30 18:28     ` [PATCH 1/2] test: known broken tests for escape characters in config files David Bremner
  0 siblings, 1 reply; 8+ messages in thread
From: David Bremner @ 2021-09-30 17:17 UTC (permalink / raw)
  To: notmuch, Ciprian Dorin Craciun; +Cc: 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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 1/2] test: known broken tests for escape characters in config files.
  2021-09-30 17:17   ` [PATCH 2/2] config: ignore leading/trailing spaces in ';'-delimited lists David Bremner
@ 2021-09-30 18:28     ` David Bremner
  2021-09-30 18:28       ` [PATCH 2/2] lib/config: use g_key_file_get_string to read config values David Bremner
  0 siblings, 1 reply; 8+ messages in thread
From: David Bremner @ 2021-09-30 18:28 UTC (permalink / raw)
  To: David Bremner, notmuch, Ciprian Dorin Craciun

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 2/2] lib/config: use g_key_file_get_string to read config values
  2021-09-30 18:28     ` [PATCH 1/2] test: known broken tests for escape characters in config files David Bremner
@ 2021-09-30 18:28       ` David Bremner
  2021-09-30 18:59         ` [PATCH v2] " David Bremner
  0 siblings, 1 reply; 8+ messages in thread
From: David Bremner @ 2021-09-30 18:28 UTC (permalink / raw)
  To: David Bremner, notmuch, Ciprian Dorin Craciun

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH v2] lib/config: use g_key_file_get_string to read config values
  2021-09-30 18:28       ` [PATCH 2/2] lib/config: use g_key_file_get_string to read config values David Bremner
@ 2021-09-30 18:59         ` David Bremner
  0 siblings, 0 replies; 8+ messages in thread
From: David Bremner @ 2021-09-30 18:59 UTC (permalink / raw)
  To: David Bremner, notmuch, Ciprian Dorin Craciun

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2021-09-30 19:00 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-24 10:36 Strip spaces in `tags` in `~/.notmuch-config` (and other fields) Ciprian Dorin Craciun
2020-04-24 11:12 ` David Bremner
2021-09-30 17:17 ` David Bremner
2021-09-30 17:17 ` [PATCH 1/2] test: known broken tests for leading/trailing ws in config David Bremner
2021-09-30 17:17   ` [PATCH 2/2] config: ignore leading/trailing spaces in ';'-delimited lists David Bremner
2021-09-30 18:28     ` [PATCH 1/2] test: known broken tests for escape characters in config files David Bremner
2021-09-30 18:28       ` [PATCH 2/2] lib/config: use g_key_file_get_string to read config values David Bremner
2021-09-30 18:59         ` [PATCH v2] " David Bremner

Code repositories for project(s) associated with this inbox:

	notmuch.git.git (no URL configured)

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).