Re: [PATCH] config: Inform user if config file is broken

2023-09-06 Thread Eric Blake
On Wed, Sep 06, 2023 at 12:48:15PM -0300, David Bremner wrote:
> Eric Blake  writes:
> 
> >
> > I'm not sure if this is the best approach (as this is my first ever
> > patch to notmuch), but it's better than nothing.
> 
> Unfortunately we can't just print from there because it is in a shared
> library (whose clients might not appreciate output).  Something _almost_
> equivalent can be done with _notmuch_database_log, but that still
> requires the caller to read those logs with
> notmuch_database_status_string.

I'm out of time to spend further on this bug today; if you would like
to take the ideas in my patch and rework it into something usable, be
my guest.  Otherwise, I might be able to return to this bug later in
the week to see if I can figure out how to grab the database_log at
the right point when status is NOTMUCH_STATUS_FILE_ERROR is returned
(open.cc:notmuch_database_load_config DONE label looks like it should
be able to grab from the database log if status_string is present).

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization:  qemu.org | libguestfs.org

___
notmuch mailing list -- [email protected]
To unsubscribe send an email to [email protected]


Re: [PATCH] config: Inform user if config file is broken

2023-09-06 Thread David Bremner
Eric Blake  writes:

>
> I'm not sure if this is the best approach (as this is my first ever
> patch to notmuch), but it's better than nothing.

Unfortunately we can't just print from there because it is in a shared
library (whose clients might not appreciate output).  Something _almost_
equivalent can be done with _notmuch_database_log, but that still
requires the caller to read those logs with
notmuch_database_status_string.

d

___
notmuch mailing list -- [email protected]
To unsubscribe send an email to [email protected]


[PATCH] config: Inform user if config file is broken

2023-09-06 Thread Eric Blake
glib 2.76.1 silently treats invalid escape sequences as two
characters, even though it is willing to set a GError warning about
it.  While 'notmuch config set' never produces such sequences in the
config file, the fact that the config file is human-readable lends
itself to hand-written edits, where the person making the edits can
introduce things such as:

[query]
foo = from:/example\.org/

instead of the correct

[query]
foo = from:/example\\.org/

glib 2.76.5 turned this into a hard error, but nothing higher in the
call stack outputs anything to the user in the case of
NOTMUCH_STATUS_FILE_ERROR to let the user know the problem ('notmuch
new' and 'notmuch count' silently fail with no output; 'notmuch config
list' outputs nothing and reports success).  While glib will be fixing
their regression before 2.78 [1], it is likely that future glib will
restore the hard error.  Thus, we should inform the user any time
their config file cannot be parsed; this gives a warning when using
glib 2.76.1 where the parse is still successful, and gives an
explanation why nothing happens during 'notmuch count' or 'notmuch
config list' when using glib 2.76.5 where the parse fails.

The added output message may still not be the most obvious:

$ notmuch --config=$PWD/.notmuch-config count
Key file contains key “foo” which has a value that cannot be interpreted.

but it is better than silence.

[1] https://gitlab.gnome.org/GNOME/glib/-/merge_requests/3565

Fixes: 
https://nmbug.notmuchmail.org/nmweb/show/5a7paaqa2dvdo5lmnxvaeacfwhdytfnkr4gfh6mtlotdviki2s%40ro4gz4m2aqsw
---

I'm not sure if this is the best approach (as this is my first ever
patch to notmuch), but it's better than nothing.

[And if Carl Worth still reads the list - thanks for introducing me to
emacs back in 2000 when we worked together as students at BYU]

 lib/config.cc | 7 ++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/lib/config.cc b/lib/config.cc
index 2323860d..afe8f429 100644
--- a/lib/config.cc
+++ b/lib/config.cc
@@ -435,6 +435,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 +443,11 @@ _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) {
+   fprintf (stderr, "%s\n", gerr->message);
+   g_error_free (gerr);
+   }
if (! val) {
status = NOTMUCH_STATUS_FILE_ERROR;
goto DONE;

base-commit: 5303e35089e1a8ffcdb1d5891bc85d3f6c401a8f
prerequisite-patch-id: e81473c9dc7ffd8b3b9bf64b3f3edb84bfb99bbb
-- 
2.41.0

___
notmuch mailing list -- [email protected]
To unsubscribe send an email to [email protected]