[PATCH] CLI/restore: handle missing keys and values in config data.

[PATCH] CLI/restore: handle missing keys and values in config data.

[David Bremner]

jrollins discovered that

   % echo '@#foo' | notmuch restore

currently segfaults.

Although such lines can't occur in notmuch dump output, they might be
useful for clearing config, and anyway segfaulting is not the best
error message.
---
 notmuch-restore.c | 8 ++++++++
 1 file changed, 8 insertions(+)

This should probably add a couple of tests. And maybe the commit
message should mention handling null keys.

diff --git a/notmuch-restore.c b/notmuch-restore.c
index dee19c20..3d36fc55 100644
--- a/notmuch-restore.c
+++ b/notmuch-restore.c
@@ -36,7 +36,15 @@ process_config_line (notmuch_database_t *notmuch, const char* line)
     void *local = talloc_new(NULL);
 
     key_p = strtok_len_c (line, delim, &key_len);
+    if (!key_p) {
+	fprintf (stderr, "missing config key on line %s\n", line);
+	goto DONE;
+    }
+
     val_p = strtok_len_c (key_p+key_len, delim, &val_len);
+    if (!val_p) {
+	val_p = "";
+    }
 
     key = talloc_strndup (local, key_p, key_len);
     val = talloc_strndup (local, val_p, val_len);
-- 
2.15.1

Re: [PATCH] CLI/restore: handle missing keys and values in config data.

[Daniel Kahn Gillmor]

[-- Attachment #1: Type: text/plain, Size: 465 bytes --]

On Sun 2018-01-07 17:30:25 -0400, David Bremner wrote:
> Although such lines can't occur in notmuch dump output, they might be
> useful for clearing config, and anyway segfaulting is not the best
> error message.

I don't think we want "notmuch restore" to actually clear any
configurations (it's always been additive thus far and changing that
seems dicey to me), but the patch itself here looks good to me.

thanks for fixing the segfault, bremner.

       --dkg

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH] CLI/restore: handle missing keys and values in config data.

[David Bremner]

Daniel Kahn Gillmor <dkg@fifthhorseman.net> writes:

> On Sun 2018-01-07 17:30:25 -0400, David Bremner wrote:
>> Although such lines can't occur in notmuch dump output, they might be
>> useful for clearing config, and anyway segfaulting is not the best
>> error message.
>
> I don't think we want "notmuch restore" to actually clear any
> configurations (it's always been additive thus far and changing that
> seems dicey to me), but the patch itself here looks good to me.

I'm not sure how to reconcile those two statements: the patch does clear
configuration if given a key without a value. 

Re: [PATCH] CLI/restore: handle missing keys and values in config data.

[Daniel Kahn Gillmor]

[-- Attachment #1: Type: text/plain, Size: 3432 bytes --]

On Sun 2018-01-07 20:12:14 -0400, David Bremner wrote:
> Daniel Kahn Gillmor <dkg@fifthhorseman.net> writes:
>
>> On Sun 2018-01-07 17:30:25 -0400, David Bremner wrote:
>>> Although such lines can't occur in notmuch dump output, they might be
>>> useful for clearing config, and anyway segfaulting is not the best
>>> error message.
>>
>> I don't think we want "notmuch restore" to actually clear any
>> configurations (it's always been additive thus far and changing that
>> seems dicey to me), but the patch itself here looks good to me.
>
> I'm not sure how to reconcile those two statements: the patch does clear
> configuration if given a key without a value. 

sorry, i didn't understand this well and am just now getting back to
looking at it.  I'd thought that this would not clear the config
variable, but would instead set it to the empty string.  But that
doesn't appear to be correct...

A few notes about the notmuch config as i'm wrapping my head back around
it:

 * at least for config data stored in the database, a configuration
   entry with the value of the empty string is the same as an unset
   configuration value.  is that right?  is that the semantics we want?

 * for config data *not* stored in the database, a configuration entry
   with the value of the empty string is *different* from an unset
   configuration value.  yikes!

query.* is stored in the database, search.* is stored in the config
file:

0 dkg@alice:~$ notmuch config set query.banana ''
0 dkg@alice:~$ notmuch config list | grep banana
1 dkg@alice:~$ notmuch config set search.banana ''
0 dkg@alice:~$ notmuch config list | grep banana
search.banana=
0 dkg@alice:~$

  * we have no "unset" subsubcommand for "notmuch config", only "set"
    and "query".  should we have "unset" ?

This is all very confusing and i don't see any principled way for users
(or developers) to get their heads around it or to know what to expect.

I've stated my preference before to move all the configuration into the
database, and to deprecate the config file (except for the database
directory itself?), because i think it would simplify and clarify
matters.  Alas, i don't have a lot of time to do this kind of
refactoring right now.

I'm fine with bremner's original proposal here, even though it
apparently *does* currently clear the config variable from the database
-- i don't think it makes things any more confusing than they were
before, though i don't think that's a particularly high bar.  In the
future, i'd hope that such an entry in "notmuch restore" would *set* the
variable to the empty string, but we don't have such a state at the
moment.


To be clear, there are situations where it's reasonable to have a config
variable that you want to be the empty string, and you want that to be
distinct from "unset".  Consider the situation where there's a standard
string that appears someplace.  As a concrete example, let's imagine a
config var reply.subject_prefix, which defaults to "Re: " (or to some
locale-derived string.

If it's unset, notmuch uses its defaults.  if it's set to the empty
string, then no subject prefix is applied on reply.  At the same time,
the user can set it to "about: " (or anything else) if they prefer.

These are distinct (and useful) semantics, which it would be nice to
have available for the notmuch config itself.

        --dkg

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH] CLI/restore: handle missing keys and values in config data.

[Tomi Ollila]

On Fri, Feb 09 2018, Daniel Kahn Gillmor wrote:

// stuff deleted

> I've stated my preference before to move all the configuration into the
> database, and to deprecate the config file (except for the database
> directory itself?), because i think it would simplify and clarify
> matters.  Alas, i don't have a lot of time to do this kind of
> refactoring right now.

I agree with this, and are in same position -- currently lacking time
(days filled in SF,CA a few more days)...

But to emphasize my desire (where I can contribute to) and something
Carl was worried at that we'd lose capability to edit configuration
file with an editor I think we sould have a way to export the configuration
to easily editable file and then import it after modifications.

Tomi

PS: I've thought when I'm moving from Fluxbox/X11 to some wayland based
environment -- and if that environment is Enlightenment, then I'll need
a way to convert configuration files to text format (and back) and save
those to git repository (as I'm currently doing w/ Fluxbox configs). 
Hopefully they (already) have way to do that, but if not then... But
if anyone knows an alternative with decent amount of configurability 
I'd be interested to know...


>
// stuff deleted
>
>         --dkg

Re: [PATCH] CLI/restore: handle missing keys and values in config data.

[Daniel Kahn Gillmor]

[-- Attachment #1: Type: text/plain, Size: 1560 bytes --]

On Thu 2018-02-15 07:53:11 +0200, Tomi Ollila wrote:
> But to emphasize my desire (where I can contribute to) and something
> Carl was worried at that we'd lose capability to edit configuration
> file with an editor I think we sould have a way to export the configuration
> to easily editable file and then import it after modifications.

fwiw, if the config file went away, i think a bare-bones implemention
could be done using something like vipe (from the moreutils package):

    notmuch dump --include=config | vipe | notmuch restore

but i have not tested it heavily! :) (one additional problem here might
be the race for the lock; if the tail of the pipeline is initialized
first, and it grabs a read/write lock on the notmuch db, it might block
the head of the pipeline, but a less-bare-bones implementation doesn't
need to be strictly a shell pipeline)

What's missing in this trivial implementation is:

 a) if a config variable is deleted from the interstitial editor, it is
    not removed from the database config (though we currently have no
    way to clear variables from the database either)

 b) comments added by the user will be thrown away

As i said on IRC, i think preserving user comments while still
facilitating automated/programmatic access to the variables in question
is the stickiest part of trying to make a human-editable file that maps
to these configuration variables.

I'm not convinced that the complexity is worth the tradeoff, but i
wouldn't object to it if someone has a clear vision and wants to
implement it.

   --dkg

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 227 bytes --]

Re: [PATCH] CLI/restore: handle missing keys and values in config data.

[David Bremner]

Daniel Kahn Gillmor <dkg@fifthhorseman.net> writes:

> A few notes about the notmuch config as i'm wrapping my head back around
> it:
>
>  * at least for config data stored in the database, a configuration
>    entry with the value of the empty string is the same as an unset
>    configuration value.  is that right?  is that the semantics we want?
>

That's correct. These are the semantics we inherited from Xapian. At the
time I implimented notmuch_database_config_*, that was the feedback I
got, I guess on the grounds of simplicity. It could be changed; I guess
not many people rely on the current semantics

>
>   * we have no "unset" subsubcommand for "notmuch config", only "set"
>     and "query".  should we have "unset" ?
>

Perhaps. Although there is an existing possibility to clear a value from
the config by "notmuch set foo", which works for both values in the
config file and values in the database.  In a sense this is consistent
with the proposed patch.  Even if you care about the distinction between
unset and empty string, the proposed patch is consistent with this
behaviour; the ugly bit is the use of an empty string internally to
signal unset. Probably this deserves some kind of comment in case we
change the handling of empty strings.

> To be clear, there are situations where it's reasonable to have a config
> variable that you want to be the empty string, and you want that to be
> distinct from "unset".

Yes, I basically agree with that.

FWIW, it doesn't really make sense to me to put any substantial effort
into the database config functionality if the concensus is replace it
with a flat file backend, as you and Carl have been discussing. That's
probably a topic for another thread.

Re: [PATCH] CLI/restore: handle missing keys and values in config data.

[David Bremner]

Daniel Kahn Gillmor <dkg@fifthhorseman.net> writes:

> On Sun 2018-01-07 17:30:25 -0400, David Bremner wrote:
>> Although such lines can't occur in notmuch dump output, they might be
>> useful for clearing config, and anyway segfaulting is not the best
>> error message.
>
> I don't think we want "notmuch restore" to actually clear any
> configurations (it's always been additive thus far and changing that
> seems dicey to me)

It's not actually additive by default, that's why the '--accumulate'
flag exists. It's true that the current patch ignores the accumulate
flag, which could be fixed.  The discussion after this gets side-tracked
by the question of whether the (database) config values should allow
empty strings. Since they currently don't, that's not an option for
missing values. The proposed behaviour is potentially useful, but also
potentially dangerous. I don't know that it is more dangerous than the
existing default behaviour for tags, which is to delete them all, then
add back those present in the dump file.  Another option would be to
just ignore such lines (and add some marker for empty string later,
if/when that's supported).

d