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