RFC: message property API

RFC: message property API

[David Bremner]

People (or maybe just dkg) have proposed various bits of user metadata
to attach to messages

   - references to be added or blacklisted
   - encryption related key material

Since such metadata would be not-reproducible from the message files,
it would be "precious" and need a backup/restore path.

This is a prototype of a library API to support that.
    
Some things to note:
    
    - This is tested on top of my libconfig [1] series, but I think there is no
      logical dependency
    
    - there are no docs, see the tests
    
    - this is just the low level api; it would need to be extended in a
      similar way to the libconfig series to dump and restore metadata.
      This might require modifying/extending the API to dump all properties
      for a message. That should be a relatively small change.
    
    - it also be useful to be able delete all (key,value) pairs with a given
      key.
    
    - this should really go through the existing caching mechanism used by
      e.g. tags, or at least not invalidate that cache.
    
[1] id:1463135893-7471-6-git-send-email-david@tethera.net

[RFC patch 1/2] lib: refactor _notmuch_message_has_term

[David Bremner]

With essentially the same code path, we can provide a key/value lookup
facility, for future use.
---
 lib/message.cc        | 68 ++++++++++++++++++++++++++++++++++++++++-----------
 lib/notmuch-private.h |  6 +++++
 2 files changed, 60 insertions(+), 14 deletions(-)

diff --git a/lib/message.cc b/lib/message.cc
index 6839305..90c2bb2 100644
--- a/lib/message.cc
+++ b/lib/message.cc
@@ -1263,36 +1263,76 @@ _notmuch_message_remove_term (notmuch_message_t *message,
     return NOTMUCH_PRIVATE_STATUS_SUCCESS;
 }
 
+/*
+ * look for a term with given prefix. If value != NULL, return the
+ *  suffix of the first such term. If suffix == NULL, consider any
+ *  non-empty suffix a mismatch
+*/
+notmuch_private_status_t
+_notmuch_message_get_prefixed_term (notmuch_message_t *message,
+				    const char *prefix,
+				    const char **value)
+{
+    notmuch_private_status_t status = NOTMUCH_PRIVATE_STATUS_SUCCESS;
+
+    if (strlen (prefix) > NOTMUCH_TERM_MAX)
+	return NOTMUCH_PRIVATE_STATUS_TERM_TOO_LONG;
+
+    try {
+	/* Look for prefixed term */
+	Xapian::TermIterator i = message->doc.termlist_begin ();
+	i.skip_to (prefix);
+	if ((i == message->doc.termlist_end ()) ||
+	    (strncmp (prefix, (*i).c_str (), strlen(prefix)) != 0)) {
+	    status = NOTMUCH_PRIVATE_STATUS_NO_TERM_FOUND;
+	} else {
+	    std::string suffix = (*i).substr (strlen (prefix));
+	    if (value) {
+		*value =  talloc_strdup (message, suffix.c_str ());
+	    } else {
+		/* non-exact match considered failure */
+		if (suffix.length () > 0)
+		    status = NOTMUCH_PRIVATE_STATUS_NO_TERM_FOUND;
+	    }
+	}
+    } catch (Xapian::Error &error) {
+	status = NOTMUCH_PRIVATE_STATUS_XAPIAN_EXCEPTION;
+    }
+
+    return status;
+}
+
 notmuch_private_status_t
 _notmuch_message_has_term (notmuch_message_t *message,
 			   const char *prefix_name,
 			   const char *value,
 			   notmuch_bool_t *result)
 {
+    notmuch_bool_t out = TRUE;
+    notmuch_private_status_t status;
     char *term;
-    notmuch_bool_t out = FALSE;
-    notmuch_private_status_t status = NOTMUCH_PRIVATE_STATUS_SUCCESS;
 
     if (value == NULL)
 	return NOTMUCH_PRIVATE_STATUS_NULL_POINTER;
 
+
     term = talloc_asprintf (message, "%s%s",
 			    _find_prefix (prefix_name), value);
 
-    if (strlen (term) > NOTMUCH_TERM_MAX)
-	return NOTMUCH_PRIVATE_STATUS_TERM_TOO_LONG;
+    if (term == NULL) {
+	status = NOTMUCH_PRIVATE_STATUS_OUT_OF_MEMORY;
+	goto DONE;
+    }
 
-    try {
-	/* Look for the exact term */
-	Xapian::TermIterator i = message->doc.termlist_begin ();
-	i.skip_to (term);
-	if (i != message->doc.termlist_end () &&
-	    !strcmp ((*i).c_str (), term))
-	    out = TRUE;
-    } catch (Xapian::Error &error) {
-	status = NOTMUCH_PRIVATE_STATUS_XAPIAN_EXCEPTION;
+    status = _notmuch_message_get_prefixed_term (message, term, NULL);
+    if (status == NOTMUCH_PRIVATE_STATUS_NO_TERM_FOUND) {
+	status = NOTMUCH_PRIVATE_STATUS_SUCCESS;
+	out = FALSE;
     }
-    talloc_free (term);
+
+ DONE:
+    if (term)
+	talloc_free (term);
 
     *result = out;
     return status;
diff --git a/lib/notmuch-private.h b/lib/notmuch-private.h
index 9280797..f7eba4a 100644
--- a/lib/notmuch-private.h
+++ b/lib/notmuch-private.h
@@ -141,6 +141,7 @@ typedef enum _notmuch_private_status {
     /* Then add our own private values. */
     NOTMUCH_PRIVATE_STATUS_TERM_TOO_LONG = NOTMUCH_STATUS_LAST_STATUS,
     NOTMUCH_PRIVATE_STATUS_NO_DOCUMENT_FOUND,
+    NOTMUCH_PRIVATE_STATUS_NO_TERM_FOUND,
 
     NOTMUCH_PRIVATE_STATUS_LAST_STATUS
 } notmuch_private_status_t;
@@ -333,6 +334,11 @@ _notmuch_message_initialize_ghost (notmuch_message_t *message,
 void
 _notmuch_message_close (notmuch_message_t *message);
 
+notmuch_private_status_t
+_notmuch_message_get_prefixed_term (notmuch_message_t *message,
+				    const char *prefix,
+				    const char **suffix);
+
 /* Get a copy of the data in this message document.
  *
  * Caller should talloc_free the result when done.
-- 
2.8.1

[RFC patch 2/2] RFC message-property API

[David Bremner]

"properties" are (key,value) pairs that can be attached to messages with
multiple instances of the same key being allowed.

In this initial prototype, we ignore the existing caching mechanism for
message metadata; worse, we invalidate the cache needlessly.
---
 lib/Makefile.local            |   1 +
 lib/database.cc               |   1 +
 lib/message-private.h         |  29 ++++++
 lib/message-property.cc       | 214 ++++++++++++++++++++++++++++++++++++++++++
 lib/message.cc                |  25 +----
 lib/notmuch.h                 |  33 +++++++
 test/T610-message-property.sh | 148 +++++++++++++++++++++++++++++
 7 files changed, 427 insertions(+), 24 deletions(-)
 create mode 100644 lib/message-private.h
 create mode 100644 lib/message-property.cc
 create mode 100755 test/T610-message-property.sh

diff --git a/lib/Makefile.local b/lib/Makefile.local
index beb9635..1f55542 100644
--- a/lib/Makefile.local
+++ b/lib/Makefile.local
@@ -48,6 +48,7 @@ libnotmuch_cxx_srcs =		\
 	$(dir)/directory.cc	\
 	$(dir)/index.cc		\
 	$(dir)/message.cc	\
+	$(dir)/message-property.cc \
 	$(dir)/query.cc		\
 	$(dir)/query-fp.cc      \
 	$(dir)/config.cc	\
diff --git a/lib/database.cc b/lib/database.cc
index 9630000..3ee5a12 100644
--- a/lib/database.cc
+++ b/lib/database.cc
@@ -252,6 +252,7 @@ static prefix_t BOOLEAN_PREFIX_EXTERNAL[] = {
      * colon, which keeps our own logic simpler.
      */
     { "folder",			"XFOLDER:" },
+    { "property",		"XPROPERTY:"},
 };
 
 static prefix_t PROBABILISTIC_PREFIX[]= {
diff --git a/lib/message-private.h b/lib/message-private.h
new file mode 100644
index 0000000..77a0b3d
--- /dev/null
+++ b/lib/message-private.h
@@ -0,0 +1,29 @@
+#ifndef MESSAGE_PRIVATE_H
+#define MESSAGE_PRIVATE_H
+
+struct visible _notmuch_message {
+    notmuch_database_t *notmuch;
+    Xapian::docid doc_id;
+    int frozen;
+    char *message_id;
+    char *thread_id;
+    char *in_reply_to;
+    notmuch_string_list_t *tag_list;
+    notmuch_string_list_t *filename_term_list;
+    notmuch_string_list_t *filename_list;
+    char *author;
+    notmuch_message_file_t *message_file;
+    notmuch_message_list_t *replies;
+    unsigned long flags;
+    /* For flags that are initialized on-demand, lazy_flags indicates
+     * if each flag has been initialized. */
+    unsigned long lazy_flags;
+
+    /* Message document modified since last sync */
+    notmuch_bool_t modified;
+
+    Xapian::Document doc;
+    Xapian::termcount termpos;
+};
+
+#endif
diff --git a/lib/message-property.cc b/lib/message-property.cc
new file mode 100644
index 0000000..3e8293a
--- /dev/null
+++ b/lib/message-property.cc
@@ -0,0 +1,214 @@
+/* message-property.cc - Properties are like tags, but (key,value) pairs.
+ * keys are allowed to repeat.
+ *
+ * This file is part of notmuch.
+ *
+ * Copyright © 2016 David Bremner
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see http://www.gnu.org/licenses/ .
+ *
+ * Author: David Bremner <david@tethera.net>
+ */
+
+#include "notmuch-private.h"
+#include "database-private.h"
+#include "message-private.h"
+
+struct _notmuch_message_property_list {
+    Xapian::TermIterator *iterator;
+    notmuch_message_t *message;
+    const char *prefix;
+    char *current;
+};
+
+static int
+_notmuch_message_property_list_destroy (notmuch_message_property_list_t *list)
+{
+    delete list->iterator;
+    return 0;
+}
+
+notmuch_status_t
+notmuch_message_get_property (notmuch_message_t *message, const char *key, const char **value)
+{
+    notmuch_private_status_t private_status;
+    char *term = NULL;
+
+    if (! value)
+	return NOTMUCH_STATUS_NULL_POINTER;
+
+    term = talloc_asprintf (message, "%s%s=",
+			    _find_prefix ("property"), key);
+    if (term == NULL) {
+	private_status = NOTMUCH_PRIVATE_STATUS_OUT_OF_MEMORY;
+	goto DONE;
+    }
+
+    private_status = _notmuch_message_get_prefixed_term (message, term, value);
+    if (private_status == NOTMUCH_PRIVATE_STATUS_NO_TERM_FOUND) {
+	*value = NULL;
+	private_status = NOTMUCH_PRIVATE_STATUS_SUCCESS;
+    }
+
+  DONE:
+    if (term)
+	talloc_free (term);
+
+    if (private_status)
+	return COERCE_STATUS (private_status,
+			      "Unhandled error retrieving message property");
+    return NOTMUCH_STATUS_SUCCESS;
+}
+
+static notmuch_status_t
+_notmuch_message_modify_property (notmuch_message_t *message, const char *key, const char *value,
+				  notmuch_bool_t delete_it)
+{
+    notmuch_private_status_t private_status;
+    notmuch_status_t status;
+    char *term = NULL;
+
+    status = _notmuch_database_ensure_writable (_notmuch_message_database (message));
+    if (status)
+	return status;
+
+    if (key == NULL || value== NULL)
+	return NOTMUCH_STATUS_NULL_POINTER;
+
+    if (index (key, '=') || index (value, '='))
+	return NOTMUCH_STATUS_ILLEGAL_ARGUMENT;
+
+    term = talloc_asprintf (message, "%s=%s", key, value);
+
+    /* this invalidates all of the property, which it currently shouldn't */
+    if (delete_it)
+	private_status = _notmuch_message_remove_term (message, "property", term);
+    else
+	private_status = _notmuch_message_add_term (message, "property", term);
+
+    if (private_status)
+	return COERCE_STATUS (private_status,
+			      "Unhandled error modifying message property");
+
+    _notmuch_message_sync (message);
+
+    if (term)
+	talloc_free (term);
+
+    return NOTMUCH_STATUS_SUCCESS;
+}
+
+notmuch_status_t
+notmuch_message_add_property (notmuch_message_t *message, const char *key, const char *value)
+{
+    return _notmuch_message_modify_property (message, key, value, FALSE);
+}
+
+notmuch_status_t
+notmuch_message_remove_property (notmuch_message_t *message, const char *key, const char *value)
+{
+    return _notmuch_message_modify_property (message, key, value, TRUE);
+}
+
+notmuch_status_t
+notmuch_message_get_property_list (notmuch_message_t *message,
+				  const char *key,
+				  notmuch_message_property_list_t **out)
+{
+    notmuch_message_property_list_t *list = NULL;
+    notmuch_status_t status = NOTMUCH_STATUS_SUCCESS;
+    notmuch_database_t *notmuch = _notmuch_message_database (message);
+
+    const char *term;
+
+    list = talloc (message, notmuch_message_property_list_t);
+    if (! list) {
+	status = NOTMUCH_STATUS_OUT_OF_MEMORY;
+	goto DONE;
+    }
+
+    talloc_set_destructor (list, _notmuch_message_property_list_destroy);
+    list->current = NULL;
+    list->message = message;
+
+
+    term = talloc_asprintf (message, "%s%s=",
+			    _find_prefix ("property"), key);
+
+    list->prefix = talloc_strdup (list, term);
+
+    if (term == NULL) {
+	status = NOTMUCH_STATUS_OUT_OF_MEMORY;
+	goto DONE;
+    }
+
+    try {
+
+	/* force copying onto the heap */
+	list->iterator = new Xapian::TermIterator;
+	*list->iterator = message->doc.termlist_begin ();
+
+	(*list->iterator).skip_to (term);
+
+    } catch (const Xapian::Error &error) {
+	_notmuch_database_log (notmuch, "A Xapian exception occurred getting property iterator: %s.\n",
+			       error.get_msg().c_str());
+	notmuch->exception_reported = TRUE;
+	status = NOTMUCH_STATUS_XAPIAN_EXCEPTION;
+    }
+
+    *out = list;
+
+  DONE:
+    if (status && list)
+	talloc_free (list);
+
+    return status;
+}
+
+notmuch_bool_t
+notmuch_message_property_list_valid (notmuch_message_property_list_t *list)
+{
+    if (*(list->iterator) == list->message->doc.termlist_end ())
+	return FALSE;
+
+    if (strncmp (list->prefix, (**list->iterator).c_str (), strlen(list->prefix)) != 0)
+	return FALSE;
+
+    return TRUE;
+}
+
+const char *
+notmuch_message_property_list_value (notmuch_message_property_list_t *list)
+{
+
+    if (list->current)
+	talloc_free (list->current);
+
+    list->current = talloc_strdup (list, (**list->iterator).c_str () + strlen (list->prefix));
+
+    return list->current;
+}
+
+void
+notmuch_message_property_list_move_to_next (notmuch_message_property_list_t *list)
+{
+    (*(list->iterator))++;
+}
+
+void
+notmuch_message_property_list_destroy (notmuch_message_property_list_t *list)
+{
+    talloc_free (list);
+}
diff --git a/lib/message.cc b/lib/message.cc
index 90c2bb2..cc9ba93 100644
--- a/lib/message.cc
+++ b/lib/message.cc
@@ -25,30 +25,7 @@
 
 #include <gmime/gmime.h>
 
-struct visible _notmuch_message {
-    notmuch_database_t *notmuch;
-    Xapian::docid doc_id;
-    int frozen;
-    char *message_id;
-    char *thread_id;
-    char *in_reply_to;
-    notmuch_string_list_t *tag_list;
-    notmuch_string_list_t *filename_term_list;
-    notmuch_string_list_t *filename_list;
-    char *author;
-    notmuch_message_file_t *message_file;
-    notmuch_message_list_t *replies;
-    unsigned long flags;
-    /* For flags that are initialized on-demand, lazy_flags indicates
-     * if each flag has been initialized. */
-    unsigned long lazy_flags;
-
-    /* Message document modified since last sync */
-    notmuch_bool_t modified;
-
-    Xapian::Document doc;
-    Xapian::termcount termpos;
-};
+#include <message-private.h>
 
 #define ARRAY_SIZE(arr) (sizeof (arr) / sizeof (arr[0]))
 
diff --git a/lib/notmuch.h b/lib/notmuch.h
index bd977c3..4a1519b 100644
--- a/lib/notmuch.h
+++ b/lib/notmuch.h
@@ -180,6 +180,11 @@ typedef enum _notmuch_status {
      */
     NOTMUCH_STATUS_PATH_ERROR,
     /**
+     * One of the arguments violates the preconditions for the
+     * function, in a way not covered by a more specific argument.
+     */
+    NOTMUCH_STATUS_ILLEGAL_ARGUMENT,
+    /**
      * Not an actual status value. Just a way to find out how many
      * valid status values there are.
      */
@@ -1651,6 +1656,34 @@ notmuch_message_thaw (notmuch_message_t *message);
 void
 notmuch_message_destroy (notmuch_message_t *message);
 
+notmuch_status_t
+notmuch_message_get_property (notmuch_message_t *message, const char *key, const char **value);
+
+notmuch_status_t
+notmuch_message_add_property (notmuch_message_t *message, const char *key, const char *value);
+
+notmuch_status_t
+notmuch_message_remove_property (notmuch_message_t *message, const char *key, const char *value);
+
+typedef struct _notmuch_message_property_list notmuch_message_property_list_t;
+
+notmuch_status_t
+notmuch_message_get_property_list (notmuch_message_t *message,
+				   const char *key,
+				   notmuch_message_property_list_t **out);
+
+notmuch_bool_t
+notmuch_message_property_list_valid (notmuch_message_property_list_t *metadata);
+
+void
+notmuch_message_property_list_move_to_next (notmuch_message_property_list_t *list);
+
+void
+notmuch_message_property_list_destroy (notmuch_message_property_list_t *list);
+
+const char *
+notmuch_message_property_list_value (notmuch_message_property_list_t *list);
+
 /**
  * Is the given 'tags' iterator pointing at a valid tag.
  *
diff --git a/test/T610-message-property.sh b/test/T610-message-property.sh
new file mode 100755
index 0000000..dcf9c3e
--- /dev/null
+++ b/test/T610-message-property.sh
@@ -0,0 +1,148 @@
+#!/usr/bin/env bash
+test_description="message property API"
+
+. ./test-lib.sh || exit 1
+
+add_email_corpus
+
+cat <<EOF > c_head
+#include <stdio.h>
+#include <string.h>
+#include <stdlib.h>
+#include <notmuch.h>
+
+void run(int line, notmuch_status_t ret)
+{
+   if (ret) {
+	fprintf (stderr, "line %d: %s\n", line, ret);
+	exit (1);
+   }
+}
+
+#define RUN(v)  run(__LINE__, v);
+
+int main (int argc, char** argv)
+{
+   notmuch_database_t *db;
+   notmuch_message_t *message = NULL;
+   const char *val;
+   notmuch_status_t stat;
+
+   RUN(notmuch_database_open (argv[1], NOTMUCH_DATABASE_MODE_READ_WRITE, &db));
+   RUN(notmuch_database_find_message(db, "4EFC743A.3060609@april.org", &message));
+   if (message == NULL) {
+	fprintf (stderr, "unable to find message");
+	exit (1);
+   }
+EOF
+
+cat <<EOF > c_tail
+   RUN(notmuch_database_destroy(db));
+}
+EOF
+
+test_begin_subtest "notmuch_message_{add,get,remove}_property"
+cat c_head - c_tail <<'EOF' | test_C ${MAIL_DIR}
+{
+   RUN(notmuch_message_add_property (message, "testkey1", "testvalue1"));
+   RUN(notmuch_message_add_property (message, "testkey2", "testvalue2"));
+   RUN(notmuch_message_get_property (message, "testkey1", &val));
+   printf("testkey1 = %s\n", val);
+   RUN(notmuch_message_get_property (message, "testkey2", &val));
+   printf("testkey2 = %s\n", val);
+
+   /* Add second value for key */
+   RUN(notmuch_message_add_property (message, "testkey2", "testvalue3"));
+   RUN(notmuch_message_get_property (message, "testkey2", &val));
+   printf("testkey2 = %s\n", val);
+
+   /* remove first value for key */
+   RUN(notmuch_message_remove_property (message, "testkey2", "testvalue2"));
+   RUN(notmuch_message_get_property (message, "testkey2", &val));
+   printf("testkey2 = %s\n", val);
+
+   /* remove non-existant value for key */
+   RUN(notmuch_message_remove_property (message, "testkey2", "testvalue2"));
+   RUN(notmuch_message_get_property (message, "testkey2", &val));
+   printf("testkey2 = %s\n", val);
+
+   /* remove only value for key */
+   RUN(notmuch_message_remove_property (message, "testkey2", "testvalue3"));
+   RUN(notmuch_message_get_property (message, "testkey2", &val));
+   printf("testkey2 = %s\n", val == NULL ? "NULL" : val);
+}
+EOF
+cat <<'EOF' >EXPECTED
+== stdout ==
+testkey1 = testvalue1
+testkey2 = testvalue2
+testkey2 = testvalue2
+testkey2 = testvalue3
+testkey2 = testvalue3
+testkey2 = NULL
+== stderr ==
+EOF
+test_expect_equal_file EXPECTED OUTPUT
+
+test_begin_subtest "notmuch_message_get_property_list: empty list"
+cat c_head - c_tail <<'EOF' | test_C ${MAIL_DIR}
+{
+   notmuch_message_property_list_t *list;
+   RUN(notmuch_message_get_property_list (message, "nonexistent", &list));
+   printf("valid = %d\n", notmuch_message_property_list_valid (list));
+   notmuch_message_property_list_destroy (list);
+}
+EOF
+cat <<'EOF' >EXPECTED
+== stdout ==
+valid = 0
+== stderr ==
+EOF
+test_expect_equal_file EXPECTED OUTPUT
+
+test_begin_subtest "notmuch_message_property_list: one value"
+cat c_head - c_tail <<'EOF' | test_C ${MAIL_DIR}
+{
+   notmuch_message_property_list_t *list;
+
+   RUN(notmuch_message_get_property_list (message, "testkey1", &list));
+   for (; notmuch_message_property_list_valid (list); notmuch_message_property_list_move_to_next (list)) {
+      printf("%s\n", notmuch_message_property_list_value(list));
+   }
+   notmuch_message_property_list_destroy (list);
+}
+EOF
+cat <<'EOF' >EXPECTED
+== stdout ==
+testvalue1
+== stderr ==
+EOF
+test_expect_equal_file EXPECTED OUTPUT
+
+test_begin_subtest "notmuch_message_property_list: multiple values"
+cat c_head - c_tail <<'EOF' | test_C ${MAIL_DIR}
+{
+   notmuch_message_property_list_t *list;
+   RUN(notmuch_message_add_property (message, "testkey1", "bob"));
+   RUN(notmuch_message_add_property (message, "testkey1", "testvalue2"));
+   RUN(notmuch_message_add_property (message, "testkey1", "alice"));
+
+   RUN(notmuch_message_get_property_list (message, "testkey1", &list));
+   for (; notmuch_message_property_list_valid (list); notmuch_message_property_list_move_to_next (list)) {
+      printf("%s\n", notmuch_message_property_list_value(list));
+   }
+   notmuch_message_property_list_destroy (list);
+}
+EOF
+cat <<'EOF' >EXPECTED
+== stdout ==
+alice
+bob
+testvalue1
+testvalue2
+== stderr ==
+EOF
+test_expect_equal_file EXPECTED OUTPUT
+
+
+test_done
-- 
2.8.1

message properties, round 2

[David Bremner]

This is still marked RFC, but is a lot closer to production ready than
the last round. In particular it is now integrated with the term
caching machinery in message.cc. The main implementation cost is the
creation of string-map.c to provide associative arrays of strings,
with iterators (basically mimicing the Xapian::TermIterators). I'm not
sure if these can replace some of the uses of gmime hash tables in the
CLI (or if that would be a good idea), but if so that code could move to util,
and probably lose the _notmuch_ prefix.

[RFC2 Patch 1/5] lib: read "property" terms from messages.

[David Bremner]

This is a first step towards providing an API to attach
arbitrary (key,value) pairs to messages and retrieve all of the values
for a given key.
---
 lib/database.cc       |  1 +
 lib/message.cc        | 29 ++++++++++++++++++++++++++++-
 lib/notmuch-private.h |  3 +++
 3 files changed, 32 insertions(+), 1 deletion(-)

diff --git a/lib/database.cc b/lib/database.cc
index 9630000..7750f50 100644
--- a/lib/database.cc
+++ b/lib/database.cc
@@ -237,6 +237,7 @@ static prefix_t BOOLEAN_PREFIX_INTERNAL[] = {
     { "directory",		"XDIRECTORY" },
     { "file-direntry",		"XFDIRENTRY" },
     { "directory-direntry",	"XDDIRENTRY" },
+    { "property",               "XPROPERTY"  },
 };
 
 static prefix_t BOOLEAN_PREFIX_EXTERNAL[] = {
diff --git a/lib/message.cc b/lib/message.cc
index 6839305..47946a3 100644
--- a/lib/message.cc
+++ b/lib/message.cc
@@ -37,6 +37,8 @@ struct visible _notmuch_message {
     notmuch_string_list_t *filename_list;
     char *author;
     notmuch_message_file_t *message_file;
+    notmuch_string_list_t *property_term_list;
+    notmuch_string_map_t *property_map;
     notmuch_message_list_t *replies;
     unsigned long flags;
     /* For flags that are initialized on-demand, lazy_flags indicates
@@ -116,6 +118,8 @@ _notmuch_message_create_for_document (const void *talloc_owner,
     message->filename_list = NULL;
     message->message_file = NULL;
     message->author = NULL;
+    message->property_term_list = NULL;
+    message->property_map = NULL;
 
     message->replies = _notmuch_message_list_create (message);
     if (unlikely (message->replies == NULL)) {
@@ -314,6 +318,7 @@ _notmuch_message_ensure_metadata (notmuch_message_t *message)
 	*id_prefix = _find_prefix ("id"),
 	*type_prefix = _find_prefix ("type"),
 	*filename_prefix = _find_prefix ("file-direntry"),
+	*property_prefix = _find_prefix ("property"),
 	*replyto_prefix = _find_prefix ("replyto");
 
     /* We do this all in a single pass because Xapian decompresses the
@@ -369,11 +374,21 @@ _notmuch_message_ensure_metadata (notmuch_message_t *message)
 	    _notmuch_database_get_terms_with_prefix (message, i, end,
 						     filename_prefix);
 
+
+    /* Get property terms. Mimic the setup with filenames above */
+    assert (strcmp (filename_prefix, property_prefix) < 0);
+    if (!message->property_map && !message->property_term_list)
+	message->property_term_list =
+	    _notmuch_database_get_terms_with_prefix (message, i, end,
+						     property_prefix);
+
     /* Get reply to */
-    assert (strcmp (filename_prefix, replyto_prefix) < 0);
+    assert (strcmp (property_prefix, replyto_prefix) < 0);
     if (!message->in_reply_to)
 	message->in_reply_to =
 	    _notmuch_message_get_term (message, i, end, replyto_prefix);
+
+
     /* It's perfectly valid for a message to have no In-Reply-To
      * header. For these cases, we return an empty string. */
     if (!message->in_reply_to)
@@ -405,6 +420,18 @@ _notmuch_message_invalidate_metadata (notmuch_message_t *message,
 	message->filename_term_list = message->filename_list = NULL;
     }
 
+    if (strcmp ("property", prefix_name) == 0) {
+
+	if (message->property_term_list)
+	    talloc_free (message->property_term_list);
+	message->property_term_list = NULL;
+
+	if (message->property_map)
+	    talloc_free (message->property_map);
+
+	message->property_map = NULL;
+    }
+
     if (strcmp ("replyto", prefix_name) == 0) {
 	talloc_free (message->in_reply_to);
 	message->in_reply_to = NULL;
diff --git a/lib/notmuch-private.h b/lib/notmuch-private.h
index 9280797..6f9af91 100644
--- a/lib/notmuch-private.h
+++ b/lib/notmuch-private.h
@@ -537,6 +537,9 @@ _notmuch_string_list_append (notmuch_string_list_t *list,
 void
 _notmuch_string_list_sort (notmuch_string_list_t *list);
 
+/* string-map.c */
+typedef struct _notmuch_string_map  notmuch_string_map_t;
+
 /* tags.c */
 
 notmuch_tags_t *
-- 
2.8.1

[RFC2 Patch 2/5] lib: private string map (associative array) API

[David Bremner]

The choice of array implementation is deliberate, for future iterator support
---
 lib/Makefile.local    |   1 +
 lib/notmuch-private.h |  11 ++++
 lib/string-map.c      | 153 ++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 165 insertions(+)
 create mode 100644 lib/string-map.c

diff --git a/lib/Makefile.local b/lib/Makefile.local
index beb9635..9280880 100644
--- a/lib/Makefile.local
+++ b/lib/Makefile.local
@@ -40,6 +40,7 @@ libnotmuch_c_srcs =		\
 	$(dir)/messages.c	\
 	$(dir)/sha1.c		\
 	$(dir)/built-with.c	\
+	$(dir)/string-map.c    \
 	$(dir)/tags.c
 
 libnotmuch_cxx_srcs =		\
diff --git a/lib/notmuch-private.h b/lib/notmuch-private.h
index 6f9af91..531b82f 100644
--- a/lib/notmuch-private.h
+++ b/lib/notmuch-private.h
@@ -540,6 +540,17 @@ _notmuch_string_list_sort (notmuch_string_list_t *list);
 /* string-map.c */
 typedef struct _notmuch_string_map  notmuch_string_map_t;
 
+notmuch_string_map_t *
+_notmuch_string_map_create (const void *ctx);
+
+void
+_notmuch_string_map_append (notmuch_string_map_t *map,
+			    const char *key,
+			    const char *value);
+
+const char *
+_notmuch_string_map_get (notmuch_string_map_t *map, const char *key);
+
 /* tags.c */
 
 notmuch_tags_t *
diff --git a/lib/string-map.c b/lib/string-map.c
new file mode 100644
index 0000000..0491a10
--- /dev/null
+++ b/lib/string-map.c
@@ -0,0 +1,153 @@
+/* string-map.c - associative arrays of strings
+ *
+ *
+ * Copyright © 2016 David Bremner
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see http://www.gnu.org/licenses/ .
+ *
+ * Author: David Bremner <david@tethera.net>
+ */
+
+#include "notmuch-private.h"
+
+/* Create a new notmuch_string_map_t object, with 'ctx' as its
+ * talloc owner.
+ *
+ * This function can return NULL in case of out-of-memory.
+ */
+
+typedef struct _notmuch_string_pair_t {
+    char *key;
+    char *value;
+} notmuch_string_pair_t;
+
+struct _notmuch_string_map {
+    notmuch_bool_t sorted;
+    size_t length;
+    notmuch_string_pair_t *pairs;
+};
+
+notmuch_string_map_t *
+_notmuch_string_map_create (const void *ctx)
+{
+    notmuch_string_map_t *map;
+
+    map = talloc (ctx, notmuch_string_map_t);
+    if (unlikely (map == NULL))
+	return NULL;
+
+    map->length = 0;
+    map->pairs = NULL;
+    map->sorted = TRUE;
+
+    return map;
+}
+
+void
+_notmuch_string_map_append (notmuch_string_map_t *map,
+			    const char *key,
+			    const char *value)
+{
+
+    map->length++;
+    map->sorted = FALSE;
+
+    if (map->pairs)
+	map->pairs = talloc_realloc (map, map->pairs, notmuch_string_pair_t, map->length + 1);
+    else
+	map->pairs = talloc_array (map, notmuch_string_pair_t, map->length + 1);
+
+    map->pairs[map->length - 1].key = talloc_strdup (map, key);
+    map->pairs[map->length - 1].value = talloc_strdup (map, value);
+
+    /* Add sentinel */
+    map->pairs[map->length].key = NULL;
+    map->pairs[map->length].value = NULL;
+
+}
+
+static int
+cmppair (const void *pa, const void *pb)
+{
+    notmuch_string_pair_t *a = (notmuch_string_pair_t *) pa;
+    notmuch_string_pair_t *b = (notmuch_string_pair_t *) pb;
+
+    return strcmp (a->key, b->key);
+}
+
+static void
+_notmuch_string_map_sort (notmuch_string_map_t *map)
+{
+    if (map->length == 0)
+	return;
+
+    if (map->sorted)
+	return;
+
+    qsort (map->pairs, map->length, sizeof (notmuch_string_pair_t), cmppair);
+
+    map->sorted = TRUE;
+}
+
+static notmuch_bool_t
+string_cmp (const char *a, const char *b, notmuch_bool_t exact)
+{
+    if (exact)
+	return (strcmp (a, b));
+    else
+	return (strncmp (a, b, strlen (a)));
+}
+
+static notmuch_string_pair_t *
+bsearch_first (notmuch_string_pair_t *array, size_t len, const char *key, notmuch_bool_t exact)
+{
+    size_t first = 0;
+    size_t last = len - 1;
+    size_t mid;
+
+    if (len <= 0)
+	return NULL;
+
+    while (last > first) {
+	mid = (first + last) / 2;
+	int sign = string_cmp (key, array[mid].key, exact);
+
+	if (sign <= 0)
+	    last = mid;
+	else
+	    first = mid + 1;
+    }
+
+
+    if (string_cmp (key, array[first].key, exact) == 0)
+	return array + first;
+    else
+	return NULL;
+
+}
+
+const char *
+_notmuch_string_map_get (notmuch_string_map_t *map, const char *key)
+{
+    notmuch_string_pair_t *pair;
+
+    /* this means that calling append invalidates iterators */
+    _notmuch_string_map_sort (map);
+
+    pair = bsearch_first (map->pairs, map->length, key, TRUE);
+    if (! pair)
+	return NULL;
+
+    return pair->value;
+}
-- 
2.8.1

[RFC2 Patch 3/5] lib: basic message-property API

[David Bremner]

Initially, support get, set and remove of single key/value pair
---
 lib/Makefile.local            |  1 +
 lib/message-private.h         | 10 +++++
 lib/message-property.cc       | 86 +++++++++++++++++++++++++++++++++++++++
 lib/message.cc                | 48 ++++++++++++++++++++++
 lib/notmuch.h                 | 14 +++++++
 test/T610-message-property.sh | 94 +++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 253 insertions(+)
 create mode 100644 lib/message-private.h
 create mode 100644 lib/message-property.cc
 create mode 100755 test/T610-message-property.sh

diff --git a/lib/Makefile.local b/lib/Makefile.local
index 9280880..c012ed1 100644
--- a/lib/Makefile.local
+++ b/lib/Makefile.local
@@ -49,6 +49,7 @@ libnotmuch_cxx_srcs =		\
 	$(dir)/directory.cc	\
 	$(dir)/index.cc		\
 	$(dir)/message.cc	\
+	$(dir)/message-property.cc \
 	$(dir)/query.cc		\
 	$(dir)/query-fp.cc      \
 	$(dir)/config.cc	\
diff --git a/lib/message-private.h b/lib/message-private.h
new file mode 100644
index 0000000..61e5bac
--- /dev/null
+++ b/lib/message-private.h
@@ -0,0 +1,10 @@
+#ifndef MESSAGE_PRIVATE_H
+#define MESSAGE_PRIVATE_H
+
+notmuch_string_map_t *
+_notmuch_message_property_map (notmuch_message_t *message);
+
+notmuch_bool_t
+_notmuch_message_frozen (notmuch_message_t *message);
+
+#endif
diff --git a/lib/message-property.cc b/lib/message-property.cc
new file mode 100644
index 0000000..21348a3
--- /dev/null
+++ b/lib/message-property.cc
@@ -0,0 +1,86 @@
+/* message-property.cc - Properties are like tags, but (key,value) pairs.
+ * keys are allowed to repeat.
+ *
+ * This file is part of notmuch.
+ *
+ * Copyright © 2016 David Bremner
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see http://www.gnu.org/licenses/ .
+ *
+ * Author: David Bremner <david@tethera.net>
+ */
+
+#include "notmuch-private.h"
+#include "database-private.h"
+#include "message-private.h"
+
+notmuch_status_t
+notmuch_message_get_property (notmuch_message_t *message, const char *key, const char **value)
+{
+    if (! value)
+	return NOTMUCH_STATUS_NULL_POINTER;
+
+    *value = _notmuch_string_map_get (_notmuch_message_property_map (message), key);
+
+    return NOTMUCH_STATUS_SUCCESS;
+}
+
+static notmuch_status_t
+_notmuch_message_modify_property (notmuch_message_t *message, const char *key, const char *value,
+				  notmuch_bool_t delete_it)
+{
+    notmuch_private_status_t private_status;
+    notmuch_status_t status;
+    char *term = NULL;
+
+    status = _notmuch_database_ensure_writable (_notmuch_message_database (message));
+    if (status)
+	return status;
+
+    if (key == NULL || value == NULL)
+	return NOTMUCH_STATUS_NULL_POINTER;
+
+    if (index (key, '=') || index (value, '='))
+	return NOTMUCH_STATUS_ILLEGAL_ARGUMENT;
+
+    term = talloc_asprintf (message, "%s=%s", key, value);
+
+    if (delete_it)
+	private_status = _notmuch_message_remove_term (message, "property", term);
+    else
+	private_status = _notmuch_message_add_term (message, "property", term);
+
+    if (private_status)
+	return COERCE_STATUS (private_status,
+			      "Unhandled error modifying message property");
+    if (! _notmuch_message_frozen (message))
+	_notmuch_message_sync (message);
+
+    if (term)
+	talloc_free (term);
+
+    return NOTMUCH_STATUS_SUCCESS;
+}
+
+notmuch_status_t
+notmuch_message_add_property (notmuch_message_t *message, const char *key, const char *value)
+{
+    return _notmuch_message_modify_property (message, key, value, FALSE);
+}
+
+notmuch_status_t
+notmuch_message_remove_property (notmuch_message_t *message, const char *key, const char *value)
+{
+    return _notmuch_message_modify_property (message, key, value, TRUE);
+}
diff --git a/lib/message.cc b/lib/message.cc
index 47946a3..0eb7e2c 100644
--- a/lib/message.cc
+++ b/lib/message.cc
@@ -20,6 +20,7 @@
 
 #include "notmuch-private.h"
 #include "database-private.h"
+#include "message-private.h"
 
 #include <stdint.h>
 
@@ -1799,3 +1800,50 @@ _notmuch_message_database (notmuch_message_t *message)
 {
     return message->notmuch;
 }
+
+void
+_notmuch_message_ensure_property_map (notmuch_message_t *message)
+{
+    notmuch_string_node_t *node;
+
+    if (message->property_map)
+	return;
+
+    if (!message->property_term_list)
+	_notmuch_message_ensure_metadata (message);
+
+    message->property_map = _notmuch_string_map_create (message);
+
+    for (node = message->property_term_list->head; node; node = node->next) {
+	const char *key;
+	char *value;
+
+	value = index(node->string, '=');
+	if (!value)
+	    INTERNAL_ERROR ("malformed property term");
+
+	*value = '\0';
+	value++;
+	key = node->string;
+
+	_notmuch_string_map_append (message->property_map, key, value);
+
+    }
+
+    talloc_free (message->property_term_list);
+    message->property_term_list = NULL;
+}
+
+notmuch_string_map_t *
+_notmuch_message_property_map (notmuch_message_t *message)
+{
+    _notmuch_message_ensure_property_map (message);
+
+    return message->property_map;
+}
+
+notmuch_bool_t
+_notmuch_message_frozen (notmuch_message_t *message)
+{
+    return message->frozen;
+}
diff --git a/lib/notmuch.h b/lib/notmuch.h
index bd977c3..c9e654e 100644
--- a/lib/notmuch.h
+++ b/lib/notmuch.h
@@ -180,6 +180,11 @@ typedef enum _notmuch_status {
      */
     NOTMUCH_STATUS_PATH_ERROR,
     /**
+     * One of the arguments violates the preconditions for the
+     * function, in a way not covered by a more specific argument.
+     */
+    NOTMUCH_STATUS_ILLEGAL_ARGUMENT,
+    /**
      * Not an actual status value. Just a way to find out how many
      * valid status values there are.
      */
@@ -1651,6 +1656,15 @@ notmuch_message_thaw (notmuch_message_t *message);
 void
 notmuch_message_destroy (notmuch_message_t *message);
 
+notmuch_status_t
+notmuch_message_get_property (notmuch_message_t *message, const char *key, const char **value);
+
+notmuch_status_t
+notmuch_message_add_property (notmuch_message_t *message, const char *key, const char *value);
+
+notmuch_status_t
+notmuch_message_remove_property (notmuch_message_t *message, const char *key, const char *value);
+
 /**
  * Is the given 'tags' iterator pointing at a valid tag.
  *
diff --git a/test/T610-message-property.sh b/test/T610-message-property.sh
new file mode 100755
index 0000000..45ed66b
--- /dev/null
+++ b/test/T610-message-property.sh
@@ -0,0 +1,94 @@
+#!/usr/bin/env bash
+test_description="message property API"
+
+. ./test-lib.sh || exit 1
+
+add_email_corpus
+
+cat <<EOF > c_head
+#include <stdio.h>
+#include <string.h>
+#include <stdlib.h>
+#include <notmuch.h>
+
+void run(int line, notmuch_status_t ret)
+{
+   if (ret) {
+	fprintf (stderr, "line %d: %s\n", line, ret);
+	exit (1);
+   }
+}
+
+#define RUN(v)  run(__LINE__, v);
+
+int main (int argc, char** argv)
+{
+   notmuch_database_t *db;
+   notmuch_message_t *message = NULL;
+   const char *val;
+   notmuch_status_t stat;
+
+   RUN(notmuch_database_open (argv[1], NOTMUCH_DATABASE_MODE_READ_WRITE, &db));
+   RUN(notmuch_database_find_message(db, "4EFC743A.3060609@april.org", &message));
+   if (message == NULL) {
+	fprintf (stderr, "unable to find message");
+	exit (1);
+   }
+EOF
+
+cat <<EOF > c_tail
+   RUN(notmuch_database_destroy(db));
+}
+EOF
+
+test_begin_subtest "notmuch_message_{add,get,remove}_property"
+cat c_head - c_tail <<'EOF' | test_C ${MAIL_DIR}
+{
+   RUN(notmuch_message_add_property (message, "testkey1", "testvalue1"));
+   RUN(notmuch_message_get_property (message, "testkey1", &val));
+   printf("testkey1[1] = %s\n", val);
+   RUN(notmuch_message_add_property (message, "testkey2", "testvalue2"));
+   RUN(notmuch_message_get_property (message, "testkey1", &val));
+   printf("testkey1[2] = %s\n", val);
+   RUN(notmuch_message_get_property (message, "testkey1", &val));
+
+   RUN(notmuch_message_get_property (message, "testkey2", &val));
+   printf("testkey2 = %s\n", val);
+
+   /* Add second value for key */
+   RUN(notmuch_message_add_property (message, "testkey2", "testvalue3"));
+   RUN(notmuch_message_get_property (message, "testkey2", &val));
+   printf("testkey2 = %s\n", val);
+
+   /* remove first value for key */
+   RUN(notmuch_message_remove_property (message, "testkey2", "testvalue2"));
+   RUN(notmuch_message_get_property (message, "testkey2", &val));
+   printf("testkey2 = %s\n", val);
+
+   /* remove non-existant value for key */
+   RUN(notmuch_message_remove_property (message, "testkey2", "testvalue2"));
+   RUN(notmuch_message_get_property (message, "testkey2", &val));
+   printf("testkey2 = %s\n", val);
+
+   /* remove only value for key */
+   RUN(notmuch_message_remove_property (message, "testkey2", "testvalue3"));
+   RUN(notmuch_message_get_property (message, "testkey2", &val));
+   printf("testkey2 = %s\n", val == NULL ? "NULL" : val);
+}
+EOF
+cat <<'EOF' >EXPECTED
+== stdout ==
+testkey1[1] = testvalue1
+testkey1[2] = testvalue1
+testkey2 = testvalue2
+testkey2 = testvalue2
+testkey2 = testvalue3
+testkey2 = testvalue3
+testkey2 = NULL
+== stderr ==
+EOF
+test_expect_equal_file EXPECTED OUTPUT
+
+
+
+test_done
-- 
2.8.1

[RFC2 Patch 4/5] lib: extend private string map API with iterators

[David Bremner]

Support for prefix based iterators is perhaps overengineering, but I
wanted to mimic the existing database_config API.
---
 lib/notmuch-private.h | 21 ++++++++++++++-
 lib/string-map.c      | 72 +++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 92 insertions(+), 1 deletion(-)

diff --git a/lib/notmuch-private.h b/lib/notmuch-private.h
index 531b82f..98efdaf 100644
--- a/lib/notmuch-private.h
+++ b/lib/notmuch-private.h
@@ -539,7 +539,7 @@ _notmuch_string_list_sort (notmuch_string_list_t *list);
 
 /* string-map.c */
 typedef struct _notmuch_string_map  notmuch_string_map_t;
-
+typedef struct _notmuch_string_map_iterator notmuch_string_map_iterator_t;
 notmuch_string_map_t *
 _notmuch_string_map_create (const void *ctx);
 
@@ -551,6 +551,25 @@ _notmuch_string_map_append (notmuch_string_map_t *map,
 const char *
 _notmuch_string_map_get (notmuch_string_map_t *map, const char *key);
 
+notmuch_string_map_iterator_t *
+_notmuch_string_map_iterator_create (notmuch_string_map_t *map, const char *key,
+				     notmuch_bool_t exact);
+
+notmuch_bool_t
+_notmuch_string_map_iterator_valid (notmuch_string_map_iterator_t *iter);
+
+void
+_notmuch_string_map_iterator_move_to_next (notmuch_string_map_iterator_t *iter);
+
+const char *
+_notmuch_string_map_iterator_key (notmuch_string_map_iterator_t *iterator);
+
+const char *
+_notmuch_string_map_iterator_value (notmuch_string_map_iterator_t *iterator);
+
+void
+_notmuch_string_map_iterator_destroy (notmuch_string_map_iterator_t *iterator);
+
 /* tags.c */
 
 notmuch_tags_t *
diff --git a/lib/string-map.c b/lib/string-map.c
index 0491a10..591ff6d 100644
--- a/lib/string-map.c
+++ b/lib/string-map.c
@@ -38,6 +38,12 @@ struct _notmuch_string_map {
     notmuch_string_pair_t *pairs;
 };
 
+struct _notmuch_string_map_iterator {
+    notmuch_string_pair_t *current;
+    notmuch_bool_t exact;
+    const char *key;
+};
+
 notmuch_string_map_t *
 _notmuch_string_map_create (const void *ctx)
 {
@@ -151,3 +157,69 @@ _notmuch_string_map_get (notmuch_string_map_t *map, const char *key)
 
     return pair->value;
 }
+
+notmuch_string_map_iterator_t *
+_notmuch_string_map_iterator_create (notmuch_string_map_t *map, const char *key,
+				     notmuch_bool_t exact)
+{
+    notmuch_string_map_iterator_t *iter;
+
+    _notmuch_string_map_sort (map);
+
+    iter = talloc (map, notmuch_string_map_iterator_t);
+    if (unlikely (iter == NULL))
+	return NULL;
+
+    iter->key = talloc_strdup (iter, key);
+    iter->exact = exact;
+    iter->current = bsearch_first (map->pairs, map->length, key, exact);
+    return iter;
+}
+
+notmuch_bool_t
+_notmuch_string_map_iterator_valid (notmuch_string_map_iterator_t *iterator)
+{
+    if (iterator->current == NULL)
+	return FALSE;
+
+    /* sentinel */
+    if (iterator->current->key == NULL)
+	return FALSE;
+
+    return (0 == string_cmp (iterator->key, iterator->current->key, iterator->exact));
+
+}
+
+void
+_notmuch_string_map_iterator_move_to_next (notmuch_string_map_iterator_t *iterator)
+{
+
+    if (! _notmuch_string_map_iterator_valid (iterator))
+	return;
+
+    (iterator->current)++;
+}
+
+const char *
+_notmuch_string_map_iterator_key (notmuch_string_map_iterator_t *iterator)
+{
+    if (! _notmuch_string_map_iterator_valid (iterator))
+	return NULL;
+
+    return iterator->current->key;
+}
+
+const char *
+_notmuch_string_map_iterator_value (notmuch_string_map_iterator_t *iterator)
+{
+    if (! _notmuch_string_map_iterator_valid (iterator))
+	return NULL;
+
+    return iterator->current->value;
+}
+
+void
+_notmuch_string_map_iterator_destroy (notmuch_string_map_iterator_t *iterator)
+{
+    talloc_free (iterator);
+}
-- 
2.8.1

[RFC2 Patch 5/5] lib: iterator API for message properties

[David Bremner]

This is a thin wrapper around the string map iterator API just introduced.
---
 lib/message-property.cc       | 38 +++++++++++++++++++
 lib/notmuch.h                 | 24 ++++++++++++
 test/T610-message-property.sh | 87 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 149 insertions(+)

diff --git a/lib/message-property.cc b/lib/message-property.cc
index 21348a3..d2e7d9c 100644
--- a/lib/message-property.cc
+++ b/lib/message-property.cc
@@ -84,3 +84,41 @@ notmuch_message_remove_property (notmuch_message_t *message, const char *key, co
 {
     return _notmuch_message_modify_property (message, key, value, TRUE);
 }
+
+notmuch_message_properties_t *
+notmuch_message_get_properties (notmuch_message_t *message, const char *key, notmuch_bool_t exact)
+{
+    notmuch_string_map_t *map;
+    map = _notmuch_message_property_map (message);
+    return _notmuch_string_map_iterator_create (map, key, exact);
+}
+
+notmuch_bool_t
+notmuch_message_properties_valid (notmuch_message_properties_t *properties)
+{
+    return _notmuch_string_map_iterator_valid (properties);
+}
+
+void
+notmuch_message_properties_move_to_next (notmuch_message_properties_t *properties)
+{
+    return _notmuch_string_map_iterator_move_to_next (properties);
+}
+
+const char *
+notmuch_message_properties_key (notmuch_message_properties_t *properties)
+{
+    return _notmuch_string_map_iterator_key (properties);
+}
+
+const char *
+notmuch_message_properties_value (notmuch_message_properties_t *properties)
+{
+    return _notmuch_string_map_iterator_value (properties);
+}
+
+void
+notmuch_message_properties_destroy (notmuch_message_properties_t *properties)
+{
+    _notmuch_string_map_iterator_destroy (properties);
+}
diff --git a/lib/notmuch.h b/lib/notmuch.h
index c9e654e..b88f47f 100644
--- a/lib/notmuch.h
+++ b/lib/notmuch.h
@@ -1665,6 +1665,30 @@ notmuch_message_add_property (notmuch_message_t *message, const char *key, const
 notmuch_status_t
 notmuch_message_remove_property (notmuch_message_t *message, const char *key, const char *value);
 
+typedef struct _notmuch_string_map_iterator notmuch_message_properties_t;
+
+notmuch_status_t
+notmuch_message_remove_property (notmuch_message_t *message, const char *key, const char *value);
+
+notmuch_message_properties_t *
+notmuch_message_get_properties (notmuch_message_t *message, const char *key, notmuch_bool_t exact);
+
+notmuch_bool_t
+notmuch_message_properties_valid (notmuch_message_properties_t *properties);
+
+void
+notmuch_message_properties_move_to_next (notmuch_message_properties_t *properties);
+
+const char *
+notmuch_message_properties_key (notmuch_message_properties_t *properties);
+
+const char *
+notmuch_message_properties_value (notmuch_message_properties_t *properties);
+
+void
+notmuch_message_properties_destroy (notmuch_message_properties_t *properties);
+
+
 /**
  * Is the given 'tags' iterator pointing at a valid tag.
  *
diff --git a/test/T610-message-property.sh b/test/T610-message-property.sh
index 45ed66b..1617f1a 100755
--- a/test/T610-message-property.sh
+++ b/test/T610-message-property.sh
@@ -89,6 +89,93 @@ testkey2 = NULL
 EOF
 test_expect_equal_file EXPECTED OUTPUT
 
+test_begin_subtest "notmuch_message_get_properties: empty list"
+cat c_head - c_tail <<'EOF' | test_C ${MAIL_DIR}
+{
+   notmuch_message_properties_t *list;
+   list = notmuch_message_get_properties (message, "nonexistent", TRUE);
+   printf("valid = %d\n", notmuch_message_properties_valid (list));
+   notmuch_message_properties_destroy (list);
+}
+EOF
+cat <<'EOF' >EXPECTED
+== stdout ==
+valid = 0
+== stderr ==
+EOF
+test_expect_equal_file EXPECTED OUTPUT
+
+test_begin_subtest "notmuch_message_properties: one value"
+cat c_head - c_tail <<'EOF' | test_C ${MAIL_DIR}
+{
+   notmuch_message_properties_t *list;
+
+   for (list = notmuch_message_get_properties (message, "testkey1", TRUE);
+        notmuch_message_properties_valid (list); notmuch_message_properties_move_to_next (list)) {
+      printf("%s\n", notmuch_message_properties_value(list));
+   }
+   notmuch_message_properties_destroy (list);
+}
+EOF
+cat <<'EOF' >EXPECTED
+== stdout ==
+testvalue1
+== stderr ==
+EOF
+test_expect_equal_file EXPECTED OUTPUT
+
+test_begin_subtest "notmuch_message_properties: multiple values"
+cat c_head - c_tail <<'EOF' | test_C ${MAIL_DIR}
+{
+   notmuch_message_properties_t *list;
+   RUN(notmuch_message_add_property (message, "testkey1", "bob"));
+   RUN(notmuch_message_add_property (message, "testkey1", "testvalue2"));
+   RUN(notmuch_message_add_property (message, "testkey1", "alice"));
+
+   for (list = notmuch_message_get_properties (message, "testkey1", TRUE);
+        notmuch_message_properties_valid (list); notmuch_message_properties_move_to_next (list)) {
+      printf("%s\n", notmuch_message_properties_value(list));
+   }
+   notmuch_message_properties_destroy (list);
+}
+EOF
+cat <<'EOF' >EXPECTED
+== stdout ==
+alice
+bob
+testvalue1
+testvalue2
+== stderr ==
+EOF
+test_expect_equal_file EXPECTED OUTPUT
+
+test_begin_subtest "notmuch_message_properties: prefix"
+cat c_head - c_tail <<'EOF' | test_C ${MAIL_DIR}
+{
+   notmuch_message_properties_t *list;
+   RUN(notmuch_message_add_property (message, "testkey3", "bob3"));
+   RUN(notmuch_message_add_property (message, "testkey3", "testvalue3"));
+   RUN(notmuch_message_add_property (message, "testkey3", "alice3"));
+
+   for (list = notmuch_message_get_properties (message, "testkey", FALSE);
+        notmuch_message_properties_valid (list); notmuch_message_properties_move_to_next (list)) {
+      printf("%s\n", notmuch_message_properties_value(list));
+   }
+   notmuch_message_properties_destroy (list);
+}
+EOF
+cat <<'EOF' >EXPECTED
+== stdout ==
+alice
+bob
+testvalue1
+testvalue2
+alice3
+bob3
+testvalue3
+== stderr ==
+EOF
+test_expect_equal_file EXPECTED OUTPUT
 
 
 test_done
-- 
2.8.1

Re: [RFC2 Patch 5/5] lib: iterator API for message properties

[David Bremner]

David Bremner <david@tethera.net> writes:

> +   notmuch_message_properties_t *list;
> +   RUN(notmuch_message_add_property (message, "testkey1", "bob"));
> +   RUN(notmuch_message_add_property (message, "testkey1", "testvalue2"));
> +   RUN(notmuch_message_add_property (message, "testkey1", "alice"));
> +
> +   for (list = notmuch_message_get_properties (message, "testkey1", TRUE);
> +        notmuch_message_properties_valid (list); notmuch_message_properties_move_to_next (list)) {
> +      printf("%s\n", notmuch_message_properties_value(list));
> +   }
> +   notmuch_message_properties_destroy (list);

I was thinking a bit about how to dump/restore these.

The most upwardly compatible way that i thought of is something like

#= msg-id key=val key=val

i.e. duplicate the msg-id for messages with properties

This would be ignored by old notmuch-restore.

Otherwise, maybe something like

msg-id -- +tag +tag # key=val key=val

I'm not sure. this might crash old notmuch-restore.

How important is backward compatibility, and how important is minimizing
dump size? It's a bit hard to predict the things people might use
message properties for, but for thread surgery, I would expect a small
number of messages with properties.

d

Re: [RFC2 Patch 5/5] lib: iterator API for message properties

[Daniel Kahn Gillmor]

On Tue 2016-05-31 21:12:21 -0400, David Bremner <david@tethera.net> wrote:
> I was thinking a bit about how to dump/restore these.
>
> The most upwardly compatible way that i thought of is something like
>
> #= msg-id key=val key=val
>
> i.e. duplicate the msg-id for messages with properties
>
> This would be ignored by old notmuch-restore.
>
> Otherwise, maybe something like
>
> msg-id -- +tag +tag # key=val key=val
>
> I'm not sure. this might crash old notmuch-restore.
>
> How important is backward compatibility, and how important is minimizing
> dump size? It's a bit hard to predict the things people might use
> message properties for, but for thread surgery, I would expect a small
> number of messages with properties.

The other concern is our conception of how properties are unset/removed,
right?

With tags, it's possible to include -blah to remove the tag "blah".  how
do we remove/clear/overwrite these tags?  what about using +key=val or
-key=val to set/unset certain key/value combinations, and a value-less
key= to remove all values matching a given key?

alternately:

 key=val (clears all values for "key", and sets a new value "val")
 key+=val (appends a value "val" for "key")
 key-=val (removes any "key" set to "val")
 key= (clears all values for "key"

---------

However we resolve this particular decision, it'd be nice to have a
stable, sane story about backward compatibility going forward, so that
we don't have to worry about it in the future.

For example, each dump file could start with a line like:

  #version 1

and notmuch restore would assume that without "#version n" as the first
line, it's version 0.  then notmuch restore could decline to parse dump
files of a version that it doesn't know about.

Alternately, we could have the first line be something like:

   #features config properties

and if the first line is not #features, then we assume that no features
are in place -- but if restore sees features it doesn't know about, it
can offer to proceed while warning the user that we might miss something
(or that something might break).


Thanks for working on this, David!  I think this is going to be really
useful!

    --dkg

Re: [RFC2 Patch 5/5] lib: iterator API for message properties

[Tomi Ollila]

On Wed, Jun 01 2016, Daniel Kahn Gillmor <dkg@fifthhorseman.net> wrote:

> On Tue 2016-05-31 21:12:21 -0400, David Bremner <david@tethera.net> wrote:
>> I was thinking a bit about how to dump/restore these.
>>
>> The most upwardly compatible way that i thought of is something like
>>
>> #= msg-id key=val key=val
>>
>> i.e. duplicate the msg-id for messages with properties
>>
>> This would be ignored by old notmuch-restore.
>>
>> Otherwise, maybe something like
>>
>> msg-id -- +tag +tag # key=val key=val
>>
>> I'm not sure. this might crash old notmuch-restore.
>>
>> How important is backward compatibility, and how important is minimizing
>> dump size? It's a bit hard to predict the things people might use
>> message properties for, but for thread surgery, I would expect a small
>> number of messages with properties.
>
> The other concern is our conception of how properties are unset/removed,
> right?
>
> With tags, it's possible to include -blah to remove the tag "blah".  how
> do we remove/clear/overwrite these tags?  what about using +key=val or
> -key=val to set/unset certain key/value combinations, and a value-less
> key= to remove all values matching a given key?
>
> alternately:
>
>  key=val (clears all values for "key", and sets a new value "val")
>  key+=val (appends a value "val" for "key")
>  key-=val (removes any "key" set to "val")
>  key= (clears all values for "key"

We'd have to distinct between key being empty and unset,
comparable to how notmuch config behaves...


$ notmuch config get built_with.compact
true
$
$ notmuch config get search.exclude_tag
$
$ notmuch config get search.exclude_tagsz 
Unknown configuration item: search.exclude_tagsz
zsh: exit 1     notmuch config get search.exclude_tagsz

>
> ---------
>
> However we resolve this particular decision, it'd be nice to have a
> stable, sane story about backward compatibility going forward, so that
> we don't have to worry about it in the future.
>
> For example, each dump file could start with a line like:
>
>   #version 1
>
> and notmuch restore would assume that without "#version n" as the first
> line, it's version 0.  then notmuch restore could decline to parse dump
> files of a version that it doesn't know about.
>
> Alternately, we could have the first line be something like:
>
>    #features config properties
>
> and if the first line is not #features, then we assume that no features
> are in place -- but if restore sees features it doesn't know about, it
> can offer to proceed while warning the user that we might miss something
> (or that something might break).

Currently dump output starts with (just run notmuch dump | less)

#notmuch-dump batch-tag:2 config,tags

perhaps this info could be put there -- is restore now (since a few notmuch
versions) already declining if this contains some strange data ?

of the 2 above suggestions I'd go w/ compatibilty version; it might be
challenging to get old notmuch parse relevant data from newer format...
... unless we also change the format to something more structured (jso^H^H^H^G
where only known data can be extracted (no, it is not SMOP, NO!)

> Thanks for working on this, David!  I think this is going to be really
> useful!

Öh, what is this feature for... >;) maybe I have to look into the series
deeper...

>
>     --dkg

Tomi

Re: [RFC2 Patch 5/5] lib: iterator API for message properties

[David Bremner]

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

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

> The other concern is our conception of how properties are unset/removed,
> right?
>
> With tags, it's possible to include -blah to remove the tag "blah".  how
> do we remove/clear/overwrite these tags?  what about using +key=val or
> -key=val to set/unset certain key/value combinations, and a value-less
> key= to remove all values matching a given key?
>

msg-id -blah is only possible if using notmuch-tag; notmuch restore takes a
strict subset of the input format for "notmuch tag --batch".

I'm not sure if generic property manipulation from the CLI is mandatory,
or at least I'm not sure it's a blocker for including the feature.
If/when we do decide to support it, I'm not sure if should be wedged
into notmuch-tag, or rather added to a new "notmuch-property"
subcommand.  Such a subcommand could take additional options and/or have
a richer syntax than used in the dump file.

I can imagine initial support with just an API, and some python
bindings. This could already be used by some reference manipulation UI.
The dump-restore support is more part of the backend in my mind, because
we don't want people to think they've backup their data when they
haven't.

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

Re: [RFC2 Patch 5/5] lib: iterator API for message properties

[Daniel Kahn Gillmor]

On Tue 2016-05-31 21:52:06 -0400, Daniel Kahn Gillmor wrote:
> Thanks for working on this, David!  I think this is going to be really
> useful!

just thinking about this series this morning in a bigger-picture way, i
figure it's worth asking the hard questions now rather than later --
maybe the answers are obvious, and we just need to write them down.
Please accept these questions in the spirit of supportive inquiry :)
Here goes:

do we actually need this abstraction?  If we're aiming to build specific
new features (the two i'm thinking of are cryptographic-session-keys and
reference-adjustments), couldn't we implement those features explicitly
in xapian with their own special prefix, rather than treating them as a
generic "property"?  If we make a generic "property", that seems likely
to be exposed to the user, who can then manipulate them directly
externally from notmuch.

We already have a bit of an uncomfortable fit with tags and special
flags (encrypted, signed, attachment, etc), where some are expected to
be set and cleared automagically and some are expected to be manipulated
directly by the user.  Are we setting ourselves up for more of the same,
or is there a principled way that a user can know which properties it's
kosher for them to set and clear, and which ones they should leave
alone?

If we add new specific features, we could potentially augment the dump
format explicitly for them, without having the property abstraction.  We
already have some explicit features for each message (subject, from, to,
attachment, mimetype, thread id, etc), and most of them are derived from
the message itself, with the hope that it could be re-derived given just
the message body.  Is there a distinction between properties that can be
derived from the message body and properties that need to be
additionally derived from some other data?

   --dkg

Re: [RFC2 Patch 5/5] lib: iterator API for message properties

[David Bremner]

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

> On Tue 2016-05-31 21:52:06 -0400, Daniel Kahn Gillmor wrote:
>
> do we actually need this abstraction?  If we're aiming to build specific
> new features (the two i'm thinking of are cryptographic-session-keys and
> reference-adjustments), couldn't we implement those features explicitly
> in xapian with their own special prefix, rather than treating them as a
> generic "property"?

Sure, it's certainly possible.

I guess if you don't care about the possibility of iterating all pairs
with given key prefix (which I admit makes more sense for the config
API), then the code could be simplified to look more like the tag list
handling code.  C is pretty crap at generics, but I guess looking at
tags.c, it's really about iterators for notmuch_string_list_t. So it
could probably be generalized to serve here.

For each such prefix, one would need to roughly duplicate patches 1/5
and 3/5.  It took me a little while to figure 1/5 out, but now that I
know, it would be less trouble.  I guess my thinking here was that I
would provide a low level interface that people using the C API or
bindings could use without hacking xapian.

> We already have a bit of an uncomfortable fit with tags and special
> flags (encrypted, signed, attachment, etc), where some are expected to
> be set and cleared automagically and some are expected to be manipulated
> directly by the user.  Are we setting ourselves up for more of the same,
> or is there a principled way that a user can know which properties it's
> kosher for them to set and clear, and which ones they should leave
> alone?

XPROPERTY is an internal prefix, which means it isn't added to the query
parser.  As it happens, I didn't plan on CLI access to these terms
either. Both of those choices are tradeoffs to say that these are
internal metadata, suitable for manipulation by programs. Such programs
could be scripts using python or ruby.

> If we add new specific features, we could potentially augment the dump
> format explicitly for them, without having the property abstraction.

We could, but I think should change the dump format quite rarely, since
we risk breaking people's scripts. So if we did it for one prefix, I'd
like to do in an extensible way so that adding new prefixes is somewhat
transparent. It also means some duplication of effort/code in notmuch
dump/restore to dump/restore each new prefix.

It's probably true that per-prefix dump format would be more compact,
since the keys would be implicit, rather than repeated for every pair.

> We already have some explicit features for each message (subject,
> from, to, attachment, mimetype, thread id, etc), and most of them are
> derived from the message itself, with the hope that it could be
> re-derived given just the message body.  Is there a distinction
> between properties that can be derived from the message body and
> properties that need to be additionally derived from some other data?

As Tomi always says, naming is the hardest thing; properties is a bit
generic. I'm not sure the distinction you make between the "message" and
the "message body" here. I think most of our derived terms are from the
message header.  My intent here is that "properties" are used for things
that cannot be derived from the message (header or body).

TL;DR:

     - per prefix requires new code in the library and dump/restore
       for every prefix
     + the dump format might be more compact if done in a per prefix way.
     + this code would be simpler than the generic properties code,
       mainly because it would not need key value pairs,
     - the library and dump/restore are parts of notmuch that have the
       potential to "break the world".  Not too many people are
       comfortable hacking on them.
     - changing the dump format is something like an ABI change for
       people whose scripts rely on dump / restore.

Re: [RFC2 Patch 5/5] lib: iterator API for message properties

[Daniel Kahn Gillmor]

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

Hi Bremner--

thanks for the response!  I didn't mean my post to be a wet-blanket,
just wanted to think through the tradeoffs...

On Wed 2016-06-01 19:29:59 -0400, David Bremner wrote:
> I guess if you don't care about the possibility of iterating all pairs
> with given key prefix (which I admit makes more sense for the config
> API), then the code could be simplified to look more like the tag list
> handling code.  C is pretty crap at generics, but I guess looking at
> tags.c, it's really about iterators for notmuch_string_list_t. So it
> could probably be generalized to serve here.
>
> For each such prefix, one would need to roughly duplicate patches 1/5
> and 3/5.  It took me a little while to figure 1/5 out, but now that I
> know, it would be less trouble.  I guess my thinking here was that I
> would provide a low level interface that people using the C API or
> bindings could use without hacking xapian.
 [...]
> XPROPERTY is an internal prefix, which means it isn't added to the query
> parser.  As it happens, I didn't plan on CLI access to these terms
> either. Both of those choices are tradeoffs to say that these are
> internal metadata, suitable for manipulation by programs. Such programs
> could be scripts using python or ruby.

I think this makes sense, and makes me more comfortable with the overall
idea of this patch series.  maybe it'd be useful to clearly document the
intended scope?

>> If we add new specific features, we could potentially augment the dump
>> format explicitly for them, without having the property abstraction.
>
> We could, but I think should change the dump format quite rarely, since
> we risk breaking people's scripts. So if we did it for one prefix, I'd
> like to do in an extensible way so that adding new prefixes is somewhat
> transparent. It also means some duplication of effort/code in notmuch
> dump/restore to dump/restore each new prefix.
>
> It's probably true that per-prefix dump format would be more compact,
> since the keys would be implicit, rather than repeated for every pair.

true, though i'm not sure how much compactness is necessary.  presumably
people are compressing their dumpfiles, and regularly repeated strings
are the easiest thing to compress.

>> We already have some explicit features for each message (subject,
>> from, to, attachment, mimetype, thread id, etc), and most of them are
>> derived from the message itself, with the hope that it could be
>> re-derived given just the message body.  Is there a distinction
>> between properties that can be derived from the message body and
>> properties that need to be additionally derived from some other data?
>
> As Tomi always says, naming is the hardest thing; properties is a bit
> generic. I'm not sure the distinction you make between the "message" and
> the "message body" here. I think most of our derived terms are from the
> message header.  My intent here is that "properties" are used for things
> that cannot be derived from the message (header or body).

To be clear, i didn't mean to distinguish betweeen "message" and
"message body" -- i don't think of the headers as being significantly
different from the body (and indeed, if we can get memoryhole working,
then some headers might be derived from or influenced by the body).

maybe it's worth thinking through each of these per-message features,
and where they come from -- are they from the message itself (header,
body, etc), from the message's position(s) in the filesystem, or
somewhere else entirely?

From the message:

 * message-id
 * subject
 * mimetype
 * attachment
 * references
 * from
 * to
 * replyto

From the filesystem itself:

 * filenames
 * folder

From elsewhere:

 * for messages which have multiple files, which file is actually indexed
 * thread-id
 * tag

we're now talking about adding properties, which are in the "elsewhere"
category, right?

It's worth noticing that the stuff in "elsewhere" is the stuff that
won't propagate across a dump/restore unless it's explicitly in the dump
somehow.   We currently fail to restore thread-id and which file is
actually indexed across a dump/restore :/

>      - per prefix requires new code in the library and dump/restore
>        for every prefix
>      + the dump format might be more compact if done in a per prefix way.
>      + this code would be simpler than the generic properties code,
>        mainly because it would not need key value pairs,
>      - the library and dump/restore are parts of notmuch that have the
>        potential to "break the world".  Not too many people are
>        comfortable hacking on them.
>      - changing the dump format is something like an ABI change for
>        people whose scripts rely on dump / restore.

I think you've convinced me that it's good to go ahead with the
properties, assuming it's scoped as defined above.  I still think that
we need a better story for upgrades to the dump format in general, but
maybe this isn't the place to make that particular case.

            --dkg

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

Re: [RFC2 Patch 5/5] lib: iterator API for message properties

[David Bremner]

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

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

>
> I think this makes sense, and makes me more comfortable with the overall
> idea of this patch series.  maybe it'd be useful to clearly document the
> intended scope?
>

Sure, where do you think that kind of documentation is appropriate?
There is the giant comment about the database schema in
lib/database.cc. Actually I just noticed I already failed to update that
for libconfig stuff.
>
> From elsewhere:
>
>  * for messages which have multiple files, which file is actually indexed

yes. Although rather than storing that, I think the right answer is more
like "all of them".

>  * thread-id
>  * tag
>
> we're now talking about adding properties, which are in the "elsewhere"
> category, right?

Correct.

>
> It's worth noticing that the stuff in "elsewhere" is the stuff that
> won't propagate across a dump/restore unless it's explicitly in the dump
> somehow.   We currently fail to restore thread-id and which file is
> actually indexed across a dump/restore :/

The thread-id is in some sense derived from the message itself. Not in a
reproducable way, but still, the dump file is the minimal set of extra
data needed to reconstruct an equivalent database (pax threading bugs).

> I think you've convinced me that it's good to go ahead with the
> properties, assuming it's scoped as defined above.  I still think that
> we need a better story for upgrades to the dump format in general, but
> maybe this isn't the place to make that particular case.
>
>             --dkg

I'm not sure what you have in mind, something more ambitious than the
header added post 0.22?

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

Re: [RFC2 Patch 5/5] lib: iterator API for message properties

[Daniel Kahn Gillmor]

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

On Fri 2016-06-03 08:54:00 -0400, David Bremner wrote:
> Sure, where do you think that kind of documentation is appropriate?
> There is the giant comment about the database schema in
> lib/database.cc. Actually I just noticed I already failed to update that
> for libconfig stuff.

That comment seems OK, but it won't be exposed to the people who are in
that middle range (python or ruby programmers but not C programmers).
Do we have a place for this kind of mid-level documenation?

> [ dkg wrote: ]
>>  * for messages which have multiple files, which file is actually indexed
>
> yes. Although rather than storing that, I think the right answer is more
> like "all of them".

I don't think we do this, do we?  Is this a bug?  is it tracked somewhere?

>> It's worth noticing that the stuff in "elsewhere" is the stuff that
>> won't propagate across a dump/restore unless it's explicitly in the dump
>> somehow.   We currently fail to restore thread-id and which file is
>> actually indexed across a dump/restore :/
>
> The thread-id is in some sense derived from the message itself. Not in a
> reproducable way, but still, the dump file is the minimal set of extra
> data needed to reconstruct an equivalent database (pax threading bugs).

This is exactly my point -- i don't care about reproducibility of the
exact numbering, but , the thread-id is *not* reproducible from the
message sets.  This is not only because of the ghost message leakage bug
documented in T590-thread-breakage.sh, but also because threads can be
joined by a message that is later removed (e.g., the "notmuch-join"
script in id:87egabu5ta.fsf@alice.fifthhorseman.net ).

>> I think you've convinced me that it's good to go ahead with the
>> properties, assuming it's scoped as defined above.  I still think that
>> we need a better story for upgrades to the dump format in general, but
>> maybe this isn't the place to make that particular case.
>
> I'm not sure what you have in mind, something more ambitious than the
> header added post 0.22?

Can you point me to the definition for that header?  i still don't
understand what the batch-tag:2 part means.  (sorry i haven't been
keeping up with the master branch lately!)

           --dkg

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

Re: [RFC2 Patch 5/5] lib: iterator API for message properties

[David Bremner]

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

> [ Unknown signature status ]
> On Fri 2016-06-03 08:54:00 -0400, David Bremner wrote:
>> Sure, where do you think that kind of documentation is appropriate?
>> There is the giant comment about the database schema in
>> lib/database.cc. Actually I just noticed I already failed to update that
>> for libconfig stuff.
>
> That comment seems OK, but it won't be exposed to the people who are in
> that middle range (python or ruby programmers but not C programmers).
> Do we have a place for this kind of mid-level documenation?

The simplest solution is probably API documentation itself
(lib/notmuch.h), which should propagage to the bindings documentation.
Maybe I'll start with that, and we can go from there.

>
>> [ dkg wrote: ]
>>>  * for messages which have multiple files, which file is actually indexed
>>
>> yes. Although rather than storing that, I think the right answer is more
>> like "all of them".
>
> I don't think we do this, do we?  Is this a bug?  is it tracked somewhere?

IMHO it is a bug. It's implicit in

   id:87k42vrqve.fsf@pip.fifthhorseman.net

and the various requests for List-Id indexing, but it's probably worth
starting a seperate thread to track it.  Especially since there are some
unresolved design issues. Like what to return for searches.

> This is exactly my point -- i don't care about reproducibility of the
> exact numbering, but , the thread-id is *not* reproducible from the
> message sets.  This is not only because of the ghost message leakage bug
> documented in T590-thread-breakage.sh, but also because threads can be
> joined by a message that is later removed (e.g., the "notmuch-join"
> script in id:87egabu5ta.fsf@alice.fifthhorseman.net ).

I see, I guess that's the intended behaviour given 604d1e0977c.

I haven't thought about the pros and cons of dumping/restoring
thread-ids. At least my database has about half as many threads as
messages, so it's a bit of data, but perhaps that's not a bit problem.
It's somewhat orthogonal to this series since those terms are already
attached to messages.

>> I'm not sure what you have in mind, something more ambitious than the
>> header added post 0.22?
>
> Can you point me to the definition for that header?  i still don't
> understand what the batch-tag:2 part means.  (sorry i haven't been
> keeping up with the master branch lately!)
>

Currently there's just the source: it says which format, and with that
format, which subset of output.

static void
print_dump_header (gzFile output, int output_format, int include)
{
    gzprintf (output, "#notmuch-dump %s:%d %s%s%s\n",
	      (output_format == DUMP_FORMAT_SUP) ? "sup" : "batch-tag",
	      NOTMUCH_DUMP_VERSION,
	      (include & DUMP_INCLUDE_CONFIG) ? "config" : "",
	      (include & DUMP_INCLUDE_TAGS) && (include & DUMP_INCLUDE_CONFIG) ? "," : "",
	      (include & DUMP_INCLUDE_TAGS) ? "tags" : "");
}

Re: [RFC2 Patch 5/5] lib: iterator API for message properties

[Daniel Kahn Gillmor]

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

On Fri 2016-06-03 19:12:52 -0400, David Bremner wrote:
>>> [ dkg wrote: ]
>>>>  * for messages which have multiple files, which file is actually indexed
>>>
>>> yes. Although rather than storing that, I think the right answer is more
>>> like "all of them".
>>
>> I don't think we do this, do we?  Is this a bug?  is it tracked somewhere?
>
> IMHO it is a bug. It's implicit in
>
>    id:87k42vrqve.fsf@pip.fifthhorseman.net
>
> and the various requests for List-Id indexing, but it's probably worth
> starting a seperate thread to track it.  Especially since there are some
> unresolved design issues. Like what to return for searches.

I'm happy to use that original thread from 2012 as the tracking thread
if you think that's a reasonable starting point.  Peter Wang's "Malice
has nothing on incompetence" message in that thread is a good reminder
about other issues there.

>> the thread-id is *not* reproducible from the
>> message sets.  This is not only because of the ghost message leakage bug
>> documented in T590-thread-breakage.sh, but also because threads can be
>> joined by a message that is later removed (e.g., the "notmuch-join"
>> script in id:87egabu5ta.fsf@alice.fifthhorseman.net ).
>
> I see, I guess that's the intended behaviour given 604d1e0977c.
>
> I haven't thought about the pros and cons of dumping/restoring
> thread-ids. At least my database has about half as many threads as
> messages, so it's a bit of data, but perhaps that's not a bit problem.
> It's somewhat orthogonal to this series since those terms are already
> attached to messages.

i agree that thread restoration is orthogonal to the per-message
properties.  I should also be clear that i don't mean to suggest that we
should dump the literal thread-id.  That'd be terrible, because you
wouldn't be able to merge two databases.

I'm happy to move the thread-id discussion to a separate topic, i just
want to make sure that people are aware that our current documentation
for notmuch-dump (below) kind of overstates the case:

------
       These tags are the only data in the  notmuch  database  that  can't  be
       recreated  from  the messages themselves. The output of notmuch dump is
       therefore the only critical thing to backup (and much more friendly  to
       incremental backup than the native database files.)
------

OK, back to the discussion of per-message properties: i think we should
go ahead with this work, now that i understand the scoping/framing of it
better!

      --dkg

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

[PATCH] doc: document notmuch-dump header line

[David Bremner]

This was introduced with the libconfig changes, but not documented then.
---
 doc/man1/notmuch-dump.rst | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/doc/man1/notmuch-dump.rst b/doc/man1/notmuch-dump.rst
index eda9e07..ecbeafe 100644
--- a/doc/man1/notmuch-dump.rst
+++ b/doc/man1/notmuch-dump.rst
@@ -86,7 +86,15 @@ Supported options for **dump** include
 	Output per-message metadata, namely tags. See *format* above
 	for description of the output.
 
-      The default is to include both tags and configuration information
+      The default is to include both tags and configuration
+      information. As of version 2 of the dump format, there is a
+      header line of the following form
+|
+
+        #notmuch-dump <*format*>:<*version*> <*included*>
+
+      where <*included*> is a comma separated list of the above
+      options.
 
     ``--output=``\ <filename>
         Write output to given file instead of stdout.
-- 
2.8.1

[PATCH] doc: document notmuch-dump header line

[David Bremner]

This was introduced with the libconfig changes, but not documented then.
---

after some IRC discussion with Tomi, this rst is slightly less gross

 doc/man1/notmuch-dump.rst | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/doc/man1/notmuch-dump.rst b/doc/man1/notmuch-dump.rst
index eda9e07..94986a8 100644
--- a/doc/man1/notmuch-dump.rst
+++ b/doc/man1/notmuch-dump.rst
@@ -86,7 +86,15 @@ Supported options for **dump** include
 	Output per-message metadata, namely tags. See *format* above
 	for description of the output.
 
-      The default is to include both tags and configuration information
+      The default is to include both tags and configuration
+      information. As of version 2 of the dump format, there is a
+      header line of the following form
+
+      |
+      |  #notmuch-dump <*format*>:<*version*> <*included*>
+
+      where <*included*> is a comma separated list of the above
+      options.
 
     ``--output=``\ <filename>
         Write output to given file instead of stdout.
-- 
2.8.1

Re: [PATCH] doc: document notmuch-dump header line

[Tomi Ollila]

On Mon, Jun 06 2016, David Bremner <david@tethera.net> wrote:

> This was introduced with the libconfig changes, but not documented then.
> ---
>
> after some IRC discussion with Tomi, this rst is slightly less gross

LGTM.

Tomi

>  doc/man1/notmuch-dump.rst | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/doc/man1/notmuch-dump.rst b/doc/man1/notmuch-dump.rst
> index eda9e07..94986a8 100644
> --- a/doc/man1/notmuch-dump.rst
> +++ b/doc/man1/notmuch-dump.rst
> @@ -86,7 +86,15 @@ Supported options for **dump** include
>  	Output per-message metadata, namely tags. See *format* above
>  	for description of the output.
>  
> -      The default is to include both tags and configuration information
> +      The default is to include both tags and configuration
> +      information. As of version 2 of the dump format, there is a
> +      header line of the following form
> +
> +      |
> +      |  #notmuch-dump <*format*>:<*version*> <*included*>
> +
> +      where <*included*> is a comma separated list of the above
> +      options.
>  
>      ``--output=``\ <filename>
>          Write output to given file instead of stdout.
> -- 
> 2.8.1
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> https://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH] doc: document notmuch-dump header line

[David Bremner]

David Bremner <david@tethera.net> writes:

> This was introduced with the libconfig changes, but not documented then.
> ---
>
> after some IRC discussion with Tomi, this rst is slightly less gross

pushed.

d

Re: [RFC2 Patch 5/5] lib: iterator API for message properties

[Tomi Ollila]

On Wed, Jun 01 2016, David Bremner <david@tethera.net> wrote:

> David Bremner <david@tethera.net> writes:
>
>> +   notmuch_message_properties_t *list;
>> +   RUN(notmuch_message_add_property (message, "testkey1", "bob"));
>> +   RUN(notmuch_message_add_property (message, "testkey1", "testvalue2"));
>> +   RUN(notmuch_message_add_property (message, "testkey1", "alice"));
>> +
>> +   for (list = notmuch_message_get_properties (message, "testkey1", TRUE);
>> +        notmuch_message_properties_valid (list); notmuch_message_properties_move_to_next (list)) {
>> +      printf("%s\n", notmuch_message_properties_value(list));
>> +   }
>> +   notmuch_message_properties_destroy (list);
>
> I was thinking a bit about how to dump/restore these.
>
> The most upwardly compatible way that i thought of is something like
>
> #= msg-id key=val key=val
>
> i.e. duplicate the msg-id for messages with properties
>
> This would be ignored by old notmuch-restore.
>
> Otherwise, maybe something like
>
> msg-id -- +tag +tag # key=val key=val
>
> I'm not sure. this might crash old notmuch-restore.
>
> How important is backward compatibility, and how important is minimizing
> dump size? It's a bit hard to predict the things people might use
> message properties for, but for thread surgery, I would expect a small
> number of messages with properties.

If we had compatibility version information in the dump file we could just
bump the version. (and if we added to newer, old notmuches would not
magically get that feature ;/)

Anyay, IMO we could make backward-incompatible changes, and then perhaps
provide conversion program (a few lines of perl(1) anyway (more lines on
many other))

Tomi

>
> d