Re: [PATCH] CLI/restore: handle missing keys and values in config data.
Daniel Kahn Gillmor 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 ___ notmuch mailing list notmuch@notmuchmail.org https://notmuchmail.org/mailman/listinfo/notmuch
Re: [PATCH] CLI/restore: handle missing keys and values in config data.
Daniel Kahn Gillmor 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. ___ notmuch mailing list notmuch@notmuchmail.org https://notmuchmail.org/mailman/listinfo/notmuch
Re: [PATCH] CLI/restore: handle missing keys and values in config data.
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 signature.asc Description: PGP signature ___ notmuch mailing list notmuch@notmuchmail.org https://notmuchmail.org/mailman/listinfo/notmuch
Re: [PATCH] CLI/restore: handle missing keys and values in config data.
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 ___ notmuch mailing list notmuch@notmuchmail.org https://notmuchmail.org/mailman/listinfo/notmuch
Re: [PATCH] CLI/restore: handle missing keys and values in config data.
On Sun 2018-01-07 20:12:14 -0400, David Bremner wrote: > Daniel Kahn Gillmor 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 signature.asc Description: PGP signature ___ notmuch mailing list notmuch@notmuchmail.org https://notmuchmail.org/mailman/listinfo/notmuch
Re: [PATCH] CLI/restore: handle missing keys and values in config data.
Daniel Kahn Gillmor 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. ___ notmuch mailing list notmuch@notmuchmail.org https://notmuchmail.org/mailman/listinfo/notmuch
Re: [PATCH] CLI/restore: handle missing keys and values in config data.
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 signature.asc Description: PGP signature ___ notmuch mailing list notmuch@notmuchmail.org https://notmuchmail.org/mailman/listinfo/notmuch
[PATCH] CLI/restore: handle missing keys and values in config data.
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 ___ notmuch mailing list notmuch@notmuchmail.org https://notmuchmail.org/mailman/listinfo/notmuch