Report GLib errors

Report GLib errors

[David Bremner]

This series obsoletes the WIP series [0].  I think it's probably worth
doing a point release with this fix as it is not too intrusive, and
upcoming GLib releases will likely cause more users to experience the
problems Eric found [1].

[0]: id:20230913005636.135665-1-david@tethera.net
[1]: id:5a7paaqa2dvdo5lmnxvaeacfwhdytfnkr4gfh6mtlotdviki2s@ro4gz4m2aqsw

[PATCH 1/4] test: add known broken test for bad utf8 in config

[David Bremner]

We should ideally print an informative error message, but at the very
least we should not exit with success.
---
 test/T030-config.sh | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/test/T030-config.sh b/test/T030-config.sh
index ea0b4012..6ef759a4 100755
--- a/test/T030-config.sh
+++ b/test/T030-config.sh
@@ -3,6 +3,8 @@
 test_description='"notmuch config"'
 . $(dirname "$0")/test-lib.sh || exit 1
 
+cp notmuch-config initial-config
+
 test_begin_subtest "Get string value"
 test_expect_equal "$(notmuch config get user.name)" "Notmuch Test Suite"
 
@@ -193,4 +195,11 @@ test_begin_subtest "get built_with.nonexistent prints false"
 output=$(notmuch config get built_with.nonexistent)
 test_expect_equal "$output" "false"
 
+test_begin_subtest "Bad utf8 reported as error"
+test_subtest_known_broken
+cp initial-config bad-config
+printf '[query]\nq3=from:\xff\n' >>bad-config
+test_expect_code 1 "notmuch --config=./bad-config config list"
+
 test_done
+
-- 
2.40.1

[PATCH 2/4] CLI: exit with error when load_config returns an error.

[David Bremner]

For now print a generic error message and exit with error on any
non-success code. Previously we were exiting, but with exit code zero,
leading users / scripts to thing the command had succeeded.
---
 notmuch.c           | 2 ++
 test/T030-config.sh | 1 -
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/notmuch.c b/notmuch.c
index 37286b8f..69a18131 100644
--- a/notmuch.c
+++ b/notmuch.c
@@ -584,6 +584,8 @@ main (int argc, char *argv[])
 	case NOTMUCH_STATUS_SUCCESS:
 	    break;
 	default:
+	    fputs ("Error: unable to load config file.\n", stderr);
+	    ret = 1;
 	    goto DONE;
 	}
 
diff --git a/test/T030-config.sh b/test/T030-config.sh
index 6ef759a4..8b3ef436 100755
--- a/test/T030-config.sh
+++ b/test/T030-config.sh
@@ -196,7 +196,6 @@ output=$(notmuch config get built_with.nonexistent)
 test_expect_equal "$output" "false"
 
 test_begin_subtest "Bad utf8 reported as error"
-test_subtest_known_broken
 cp initial-config bad-config
 printf '[query]\nq3=from:\xff\n' >>bad-config
 test_expect_code 1 "notmuch --config=./bad-config config list"
-- 
2.40.1

[PATCH 3/4] test: add known broken subtest for the bad config error message

[David Bremner]

This is a bit fragile w.r.t. glib changing their error message, but it
already helped me find one formatting bug, so for now I think it's
worth it, instead of just grepping for "UTF-8".
---
 test/T030-config.sh | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/test/T030-config.sh b/test/T030-config.sh
index 8b3ef436..11428e8a 100755
--- a/test/T030-config.sh
+++ b/test/T030-config.sh
@@ -200,5 +200,14 @@ cp initial-config bad-config
 printf '[query]\nq3=from:\xff\n' >>bad-config
 test_expect_code 1 "notmuch --config=./bad-config config list"
 
+test_begin_subtest "Specific error message about bad utf8"
+test_subtest_known_broken
+notmuch --config=./bad-config config list 2>ERRORS
+cat <<EOF > EXPECTED
+GLib: Key file contains key “q3” with value “from:�” which is not UTF-8
+Error: unable to load config file.
+EOF
+test_expect_equal_file EXPECTED ERRORS
+
 test_done
 
-- 
2.40.1
\r

[PATCH 4/4] Pass error message from GLib ini parser to CLI

[David Bremner]

The function _notmuch_config_load_from_file is only called in two
places in open.cc. Update internal API to match the idiom in open.cc.
Adding a newline is needed for consistency with other status strings.

Based in part on a patch [1] from Eric Blake.

[1]: id:20230906153402.101471-1-eblake@redhat.com
---
 lib/config.cc         | 13 +++++++++++--
 lib/notmuch-private.h |  2 +-
 lib/open.cc           |  4 ++--
 notmuch.c             |  6 ++++++
 test/T030-config.sh   |  1 -
 5 files changed, 20 insertions(+), 6 deletions(-)

diff --git a/lib/config.cc b/lib/config.cc
index 2323860d..6cd15fab 100644
--- a/lib/config.cc
+++ b/lib/config.cc
@@ -416,7 +416,8 @@ _expand_path (void *ctx, const char *key, const char *val)
 
 notmuch_status_t
 _notmuch_config_load_from_file (notmuch_database_t *notmuch,
-				GKeyFile *file)
+				GKeyFile *file,
+				char **status_string)
 {
     notmuch_status_t status = NOTMUCH_STATUS_SUCCESS;
     gchar **groups = NULL, **keys, *val;
@@ -435,6 +436,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;
+	    GError *gerr = NULL;
 
 	    /* If we opened from a given path, do not overwrite it */
 	    if (strcmp (absolute_key, "database.path") == 0 &&
@@ -442,7 +444,14 @@ _notmuch_config_load_from_file (notmuch_database_t *notmuch,
 		notmuch->xapian_db)
 		continue;
 
-	    val = g_key_file_get_string (file, *grp, *keys_p, NULL);
+	    val = g_key_file_get_string (file, *grp, *keys_p, &gerr);
+	    if (gerr) {
+		if (status_string)
+		    IGNORE_RESULT (asprintf (status_string,
+					     "GLib: %s\n",
+					     gerr->message));
+		g_error_free (gerr);
+	    }
 	    if (! val) {
 		status = NOTMUCH_STATUS_FILE_ERROR;
 		goto DONE;
diff --git a/lib/notmuch-private.h b/lib/notmuch-private.h
index c19ee8e2..367e23e6 100644
--- a/lib/notmuch-private.h
+++ b/lib/notmuch-private.h
@@ -726,7 +726,7 @@ notmuch_status_t
 _notmuch_config_load_from_database (notmuch_database_t *db);
 
 notmuch_status_t
-_notmuch_config_load_from_file (notmuch_database_t *db, GKeyFile *file);
+_notmuch_config_load_from_file (notmuch_database_t *db, GKeyFile *file, char **status_string);
 
 notmuch_status_t
 _notmuch_config_load_defaults (notmuch_database_t *db);
diff --git a/lib/open.cc b/lib/open.cc
index 54d1faf3..005872dc 100644
--- a/lib/open.cc
+++ b/lib/open.cc
@@ -555,7 +555,7 @@ _finish_open (notmuch_database_t *notmuch,
 	    goto DONE;
 
 	if (key_file)
-	    status = _notmuch_config_load_from_file (notmuch, key_file);
+	    status = _notmuch_config_load_from_file (notmuch, key_file, &message);
 	if (status)
 	    goto DONE;
 
@@ -961,7 +961,7 @@ notmuch_database_load_config (const char *database_path,
     }
 
     if (key_file) {
-	status = _notmuch_config_load_from_file (notmuch, key_file);
+	status = _notmuch_config_load_from_file (notmuch, key_file, &message);
 	if (status)
 	    goto DONE;
     }
diff --git a/notmuch.c b/notmuch.c
index 69a18131..814b9e42 100644
--- a/notmuch.c
+++ b/notmuch.c
@@ -563,6 +563,12 @@ main (int argc, char *argv[])
 					       NULL,
 					       &notmuch,
 					       &status_string);
+	if (status_string) {
+	    fputs (status_string, stderr);
+	    free (status_string);
+	    status_string = NULL;
+	}
+
 	switch (status) {
 	case NOTMUCH_STATUS_NO_CONFIG:
 	    if (! (command->mode & NOTMUCH_COMMAND_CONFIG_CREATE)) {
diff --git a/test/T030-config.sh b/test/T030-config.sh
index 11428e8a..8aee9dc9 100755
--- a/test/T030-config.sh
+++ b/test/T030-config.sh
@@ -201,7 +201,6 @@ printf '[query]\nq3=from:\xff\n' >>bad-config
 test_expect_code 1 "notmuch --config=./bad-config config list"
 
 test_begin_subtest "Specific error message about bad utf8"
-test_subtest_known_broken
 notmuch --config=./bad-config config list 2>ERRORS
 cat <<EOF > EXPECTED
 GLib: Key file contains key “q3” with value “from:�” which is not UTF-8
-- 
2.40.1
\r

Re: Report GLib errors

[David Bremner]

David Bremner <david@tethera.net> writes:

> This series obsoletes the WIP series [0].  I think it's probably worth
> doing a point release with this fix as it is not too intrusive, and
> upcoming GLib releases will likely cause more users to experience the
> problems Eric found [1].
>
> [0]: id:20230913005636.135665-1-david@tethera.net
> [1]: id:5a7paaqa2dvdo5lmnxvaeacfwhdytfnkr4gfh6mtlotdviki2s@ro4gz4m2aqsw

Series applied (with a couple of whitespace and commit message fixups)
to release and master.