[PATCH v3 00/13] Implement and use database "features"

[PATCH v3 00/13] Implement and use database "features"

[Austin Clements]

This is v3 of id:1406652492-27803-1-git-send-email-amdragon@mit.edu.
This fixes one issue and tidies up another thing in
notmuch_database_upgrade I found while working on change tracking
support.  Most of the patches are logically identical to v2, but the
changes ripple through the patch context, so I'm sending a new series.

First, this updates notmuch->features before starting the upgrade,
rather than after, so that functions called by upgrade will use the
new database features instead of the old (this didn't matter in this
series because nothing modified the database differently depending on
features).  Second, this combines multiple _notmuch_message_sync calls
into one, which cleans up the code, should further improve upgrade
performance, and makes way for additional per-message upgrades.

The diff from v2 is below (excluding patch 2, which David pushed).

diff --git a/lib/database.cc b/lib/database.cc
index b323691..d90a924 100644
--- a/lib/database.cc
+++ b/lib/database.cc
@@ -1252,6 +1252,10 @@ notmuch_database_upgrade (notmuch_database_t *notmuch,
     /* Perform the upgrade in a transaction. */
     db->begin_transaction (true);
 
+    /* Set the target features so we write out changes in the desired
+     * format. */
+    notmuch->features = target_features;
+
     /* Perform per-message upgrades. */
     if (new_features &
 	(NOTMUCH_FEATURE_FILE_TERMS | NOTMUCH_FEATURE_BOOL_FOLDER)) {
@@ -1280,7 +1284,6 @@ notmuch_database_upgrade (notmuch_database_t *notmuch,
 		if (filename && *filename != '\0') {
 		    _notmuch_message_add_filename (message, filename);
 		    _notmuch_message_clear_data (message);
-		    _notmuch_message_sync (message);
 		}
 		talloc_free (filename);
 	    }
@@ -1289,10 +1292,10 @@ notmuch_database_upgrade (notmuch_database_t *notmuch,
 	     * probabilistic and stemmed. Change it to the current
 	     * boolean prefix. Add "path:" prefixes while at it.
 	     */
-	    if (new_features & NOTMUCH_FEATURE_BOOL_FOLDER) {
+	    if (new_features & NOTMUCH_FEATURE_BOOL_FOLDER)
 		_notmuch_message_upgrade_folder (message);
-		_notmuch_message_sync (message);
-	    }
+
+	    _notmuch_message_sync (message);
 
 	    notmuch_message_destroy (message);
 
@@ -1348,7 +1351,6 @@ notmuch_database_upgrade (notmuch_database_t *notmuch,
 	}
     }
 
-    notmuch->features = target_features;
     db->set_metadata ("features", _print_features (local, notmuch->features));
     db->set_metadata ("version", STRINGIFY (NOTMUCH_DATABASE_VERSION));

[PATCH v3 01/13] test: Include generated dependencies for test sources

[Austin Clements]

Previously the build system was generating automatic header
dependencies for test sources, but only smtp-dummy was in SRCS, so
only its dependencies were being included.  Add all of the test
sources to SRCS so that the root Makefile.local includes their
dependencies.
---
 test/Makefile.local | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/test/Makefile.local b/test/Makefile.local
index 1c85b18..916dd0b 100644
--- a/test/Makefile.local
+++ b/test/Makefile.local
@@ -37,12 +37,17 @@ $(dir)/parse-time: $(dir)/parse-time.o parse-time-string/parse-time-string.o
 
 .PHONY: test check
 
-TEST_BINARIES=$(dir)/arg-test \
-	      $(dir)/hex-xcode \
-	      $(dir)/random-corpus \
-	      $(dir)/parse-time \
-	      $(dir)/smtp-dummy \
-	      $(dir)/symbol-test
+test_main_srcs=$(dir)/arg-test.c \
+	      $(dir)/hex-xcode.c \
+	      $(dir)/random-corpus.c \
+	      $(dir)/parse-time.c \
+	      $(dir)/smtp-dummy.c \
+	      $(dir)/symbol-test.cc \
+
+test_srcs=$(test_main_srcs) $(dir)/database-test.c
+
+TEST_BINARIES := $(test_main_srcs:.c=)
+TEST_BINARIES := $(TEST_BINARIES:.cc=)
 
 test-binaries: $(TEST_BINARIES)
 
@@ -51,7 +56,7 @@ test:	all test-binaries
 
 check: test
 
-SRCS := $(SRCS) $(smtp_dummy_srcs)
+SRCS := $(SRCS) $(test_srcs)
 CLEAN += $(TEST_BINARIES) $(addsuffix .o,$(TEST_BINARIES)) \
 	 $(dir)/database-test.o \
 	 $(dir)/corpus.mail $(dir)/test-results $(dir)/tmp.*
-- 
2.0.0

[PATCH v3 02/13] util: Const version of strtok_len

[Austin Clements]

Because of limitations in the C type system, we can't a strtok_len
that can work on both const string and non-const strings.  The C
library solves this by taking a const char* and returning a char*
in functions like this (e.g., strchr), but that's not const-safe.
Solve it by introducing strtok_len_c, a version of strtok_len for
const strings.
---
 util/string-util.c | 8 ++++++++
 util/string-util.h | 3 +++
 2 files changed, 11 insertions(+)

diff --git a/util/string-util.c b/util/string-util.c
index 3e7066c..a90501e 100644
--- a/util/string-util.c
+++ b/util/string-util.c
@@ -37,6 +37,14 @@ strtok_len (char *s, const char *delim, size_t *len)
     return *len ? s : NULL;
 }
 
+const char *
+strtok_len_c (const char *s, const char *delim, size_t *len)
+{
+    /* strtok_len is already const-safe, but we can't express both
+     * versions in the C type system. */
+    return strtok_len ((char*)s, delim, len);
+}
+
 char *
 sanitize_string (const void *ctx, const char *str)
 {
diff --git a/util/string-util.h b/util/string-util.h
index ccad17f..e409cb3 100644
--- a/util/string-util.h
+++ b/util/string-util.h
@@ -23,6 +23,9 @@ extern "C" {
 
 char *strtok_len (char *s, const char *delim, size_t *len);
 
+/* Const version of strtok_len. */
+const char *strtok_len_c (const char *s, const char *delim, size_t *len);
+
 /* Return a talloced string with str sanitized.
  *
  * Whitespace characters (tabs and newlines) are replaced with spaces,
-- 
2.0.0

[PATCH v3 03/13] new: Don't report version after upgrade

[Austin Clements]

The version number has always been pretty meaningless to the user and
it's about to become even more meaningless with the introduction of
"features".  Hopefully, the database will remain on version 3 for some
time to come; however, the introduction of new features over time in
version 3 will necessitate upgrades within version 3.  It would be
confusing if we always tell the user they've been "upgraded to version
3".  If the user wants to know what's new, they should read the news.
---
 notmuch-new.c        | 3 +--
 test/T530-upgrade.sh | 2 +-
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/notmuch-new.c b/notmuch-new.c
index d269c7c..b7590a8 100644
--- a/notmuch-new.c
+++ b/notmuch-new.c
@@ -1023,8 +1023,7 @@ notmuch_new_command (notmuch_config_t *config, int argc, char *argv[])
 				      add_files_state.verbosity >= VERBOSITY_NORMAL ? upgrade_print_progress : NULL,
 				      &add_files_state);
 	    if (add_files_state.verbosity >= VERBOSITY_NORMAL)
-		printf ("Your notmuch database has now been upgraded to database format version %u.\n",
-		    notmuch_database_get_version (notmuch));
+		printf ("Your notmuch database has now been upgraded.\n");
 	}
 
 	add_files_state.total_files = 0;
diff --git a/test/T530-upgrade.sh b/test/T530-upgrade.sh
index 7d5d5aa..c4c4ac8 100755
--- a/test/T530-upgrade.sh
+++ b/test/T530-upgrade.sh
@@ -33,7 +33,7 @@ test_expect_equal "$output" "\
 Welcome to a new version of notmuch! Your database will now be upgraded.
 This process is safe to interrupt.
 Backing up tags to FILENAME
-Your notmuch database has now been upgraded to database format version 2.
+Your notmuch database has now been upgraded.
 No new mail."
 
 test_begin_subtest "tag backup matches pre-upgrade dump"
-- 
2.0.0

[PATCH v3 04/13] lib: Database version 3: Introduce fine-grained "features"

[Austin Clements]

Previously, our database schema was versioned by a single number.
Each database schema change had to occur "atomically" in Notmuch's
development history: before some commit, Notmuch used version N, after
that commit, it used version N+1.  Hence, each new schema version
could introduce only one change, the task of developing a schema
change fell on a single person, and it all had to happen and be
perfect in a single commit series.  This made introducing a new schema
version hard.  We've seen only two schema changes in the history of
Notmuch.

This commit introduces database schema version 3; hopefully the last
schema version we'll need for a while.  With this version, we switch
from a single version number to "features": a set of named,
independent aspects of the database schema.

Features should make backwards compatibility easier.  For many things,
it should be easy to support databases both with and without a
feature, which will allow us to make upgrades optional and will enable
"unstable" features that can be developed and tested over time.

Features also make forwards compatibility easier.  The features
recorded in a database include "compatibility flags," which can
indicate to an older version of Notmuch when it must support a given
feature to open the database for read or for write.  This lets us
replace the old vague "I don't recognize this version, so something
might go wrong, but I promise to try my best" warnings upon opening a
database with an unknown version with precise errors.  If a database
is safe to open for read/write despite unknown features, an older
version will know that and issue no message at all.  If the database
is not safe to open for read/write because of unknown features, an
older version will know that, too, and can tell the user exactly which
required features it lacks support for.
---
 lib/database-private.h |  57 ++++++++++++++-
 lib/database.cc        | 190 ++++++++++++++++++++++++++++++++++++++++---------
 2 files changed, 213 insertions(+), 34 deletions(-)

diff --git a/lib/database-private.h b/lib/database-private.h
index d3e65fd..2ffab33 100644
--- a/lib/database-private.h
+++ b/lib/database-private.h
@@ -41,11 +41,15 @@ struct _notmuch_database {
 
     char *path;
 
-    notmuch_bool_t needs_upgrade;
     notmuch_database_mode_t mode;
     int atomic_nesting;
     Xapian::Database *xapian_db;
 
+    /* Bit mask of features used by this database.  Features are
+     * named, independent aspects of the database schema.  This is a
+     * bitwise-OR of NOTMUCH_FEATURE_* values (below). */
+    unsigned int features;
+
     unsigned int last_doc_id;
     uint64_t last_thread_id;
 
@@ -55,6 +59,57 @@ struct _notmuch_database {
     Xapian::ValueRangeProcessor *date_range_processor;
 };
 
+/* Bit masks for _notmuch_database::features. */
+enum {
+    /* If set, file names are stored in "file-direntry" terms.  If
+     * unset, file names are stored in document data.
+     *
+     * Introduced: version 1.  Implementation support: both for read;
+     * required for write. */
+    NOTMUCH_FEATURE_FILE_TERMS = 1 << 0,
+
+    /* If set, directory timestamps are stored in documents with
+     * XDIRECTORY terms and relative paths.  If unset, directory
+     * timestamps are stored in documents with XTIMESTAMP terms and
+     * absolute paths.
+     *
+     * Introduced: version 1.  Implementation support: required. */
+    NOTMUCH_FEATURE_DIRECTORY_DOCS = 1 << 1,
+
+    /* If set, the from, subject, and message-id headers are stored in
+     * message document values.  If unset, message documents *may*
+     * have these values, but if the value is empty, it must be
+     * retrieved from the message file.
+     *
+     * Introduced: optional in version 1, required as of version 3.
+     * Implementation support: both.
+     */
+    NOTMUCH_FEATURE_FROM_SUBJECT_ID_VALUES = 1 << 2,
+
+    /* If set, folder terms are boolean and path terms exist.  If
+     * unset, folder terms are probabilistic and stemmed and path
+     * terms do not exist.
+     *
+     * Introduced: version 2.  Implementation support: required. */
+    NOTMUCH_FEATURE_BOOL_FOLDER = 1 << 3,
+};
+
+/* Prior to database version 3, features were implied by the database
+ * version number, so hard-code them for earlier versions. */
+#define NOTMUCH_FEATURES_V0 (0)
+#define NOTMUCH_FEATURES_V1 (NOTMUCH_FEATURES_V0 | NOTMUCH_FEATURE_FILE_TERMS | \
+			     NOTMUCH_FEATURE_DIRECTORY_DOCS)
+#define NOTMUCH_FEATURES_V2 (NOTMUCH_FEATURES_V1 | NOTMUCH_FEATURE_BOOL_FOLDER)
+
+/* Current database features.  If any of these are missing from a
+ * database, request an upgrade.
+ * NOTMUCH_FEATURE_FROM_SUBJECT_ID_VALUES is not included because
+ * upgrade doesn't currently introduce the feature (though brand new
+ * databases will have it). */
+#define NOTMUCH_FEATURES_CURRENT \
+    (NOTMUCH_FEATURE_FILE_TERMS | NOTMUCH_FEATURE_DIRECTORY_DOCS | \
+     NOTMUCH_FEATURE_BOOL_FOLDER)
+
 /* Return the list of terms from the given iterator matching a prefix.
  * The prefix will be stripped from the strings in the returned list.
  * The list will be allocated using ctx as the talloc context.
diff --git a/lib/database.cc b/lib/database.cc
index 45c4260..29a56db 100644
--- a/lib/database.cc
+++ b/lib/database.cc
@@ -20,6 +20,7 @@
 
 #include "database-private.h"
 #include "parse-time-vrp.h"
+#include "string-util.h"
 
 #include <iostream>
 
@@ -42,7 +43,7 @@ typedef struct {
     const char *prefix;
 } prefix_t;
 
-#define NOTMUCH_DATABASE_VERSION 2
+#define NOTMUCH_DATABASE_VERSION 3
 
 #define STRINGIFY(s) _SUB_STRINGIFY(s)
 #define _SUB_STRINGIFY(s) #s
@@ -151,6 +152,17 @@ typedef struct {
  *			changes are made to the database (such as by
  *			indexing new fields).
  *
+ *	features	The set of features supported by this
+ *			database. This consists of a set of
+ *			'\n'-separated lines, where each is a feature
+ *			name, a '\t', and compatibility flags.  If the
+ *			compatibility flags contain 'w', then the
+ *			opener must support this feature to safely
+ *			write this database.  If the compatibility
+ *			flags contain 'r', then the opener must
+ *			support this feature to read this database.
+ *			Introduced in database version 3.
+ *
  *	last_thread_id	The last thread ID generated. This is stored
  *			as a 16-byte hexadecimal ASCII representation
  *			of a 64-bit unsigned integer. The first ID
@@ -251,6 +263,25 @@ _find_prefix (const char *name)
     return "";
 }
 
+static const struct
+{
+    /* NOTMUCH_FEATURE_* value. */
+    unsigned int value;
+    /* Feature name as it appears in the database.  This name should
+     * be appropriate for displaying to the user if an older version
+     * of notmuch doesn't support this feature. */
+    const char *name;
+    /* Compatibility flags when this feature is declared. */
+    const char *flags;
+} feature_names[] = {
+    {NOTMUCH_FEATURE_FILE_TERMS,             "multiple paths per message", "rw"},
+    {NOTMUCH_FEATURE_DIRECTORY_DOCS,         "relative directory paths", "rw"},
+    /* Header values are not required for reading a database because a
+     * reader can just refer to the message file. */
+    {NOTMUCH_FEATURE_FROM_SUBJECT_ID_VALUES, "from/subject/message-ID in database", "w"},
+    {NOTMUCH_FEATURE_BOOL_FOLDER,            "exact folder:/path: search", "rw"},
+};
+
 const char *
 notmuch_status_to_string (notmuch_status_t status)
 {
@@ -591,6 +622,11 @@ notmuch_database_create (const char *path, notmuch_database_t **database)
 				    &notmuch);
     if (status)
 	goto DONE;
+
+    /* Upgrade doesn't add this feature to existing databases, but new
+     * databases have it. */
+    notmuch->features |= NOTMUCH_FEATURE_FROM_SUBJECT_ID_VALUES;
+
     status = notmuch_database_upgrade (notmuch, NULL, NULL);
     if (status) {
 	notmuch_database_close(notmuch);
@@ -619,6 +655,80 @@ _notmuch_database_ensure_writable (notmuch_database_t *notmuch)
     return NOTMUCH_STATUS_SUCCESS;
 }
 
+/* Parse a database features string from the given database version.
+ *
+ * For version < 3, this ignores the features string and returns a
+ * hard-coded set of features.
+ *
+ * If there are unrecognized features that are required to open the
+ * database in mode (which should be 'r' or 'w'), return a
+ * comma-separated list of unrecognized but required features in
+ * *incompat_out, which will be allocated from ctx.
+ */
+static unsigned int
+_parse_features (const void *ctx, const char *features, unsigned int version,
+		 char mode, char **incompat_out)
+{
+    unsigned int res = 0, namelen, i;
+    size_t llen = 0;
+    const char *flags;
+
+    /* Prior to database version 3, features were implied by the
+     * version number. */
+    if (version == 0)
+	return NOTMUCH_FEATURES_V0;
+    else if (version == 1)
+	return NOTMUCH_FEATURES_V1;
+    else if (version == 2)
+	return NOTMUCH_FEATURES_V2;
+
+    /* Parse the features string */
+    while ((features = strtok_len_c (features + llen, "\n", &llen)) != NULL) {
+	flags = strchr (features, '\t');
+	if (! flags || flags > features + llen)
+	    continue;
+	namelen = flags - features;
+
+	for (i = 0; i < ARRAY_SIZE (feature_names); ++i) {
+	    if (strlen (feature_names[i].name) == namelen &&
+		strncmp (feature_names[i].name, features, namelen) == 0) {
+		res |= feature_names[i].value;
+		break;
+	    }
+	}
+
+	if (i == ARRAY_SIZE (feature_names)) {
+	    /* Unrecognized feature */
+	    const char *have = strchr (flags, mode);
+	    if (have && have < features + llen) {
+		/* This feature is required to access this database in
+		 * 'mode', but we don't understand it. */
+		if (! *incompat_out)
+		    *incompat_out = talloc_strdup (ctx, "");
+		*incompat_out = talloc_asprintf_append_buffer (
+		    *incompat_out, "%s%.*s", **incompat_out ? ", " : "",
+		    namelen, features);
+	    }
+	}
+    }
+
+    return res;
+}
+
+static char *
+_print_features (const void *ctx, unsigned int features)
+{
+    unsigned int i;
+    char *res = talloc_strdup (ctx, "");
+
+    for (i = 0; i < ARRAY_SIZE (feature_names); ++i)
+	if (features & feature_names[i].value)
+	    res = talloc_asprintf_append_buffer (
+		res, "%s\t%s\n", feature_names[i].name, feature_names[i].flags);
+
+    return res;
+}
+
 notmuch_status_t
 notmuch_database_open (const char *path,
 		       notmuch_database_mode_t mode,
@@ -627,7 +737,7 @@ notmuch_database_open (const char *path,
     notmuch_status_t status = NOTMUCH_STATUS_SUCCESS;
     void *local = talloc_new (NULL);
     notmuch_database_t *notmuch = NULL;
-    char *notmuch_path, *xapian_path;
+    char *notmuch_path, *xapian_path, *incompat_features;
     struct stat st;
     int err;
     unsigned int i, version;
@@ -677,7 +787,6 @@ notmuch_database_open (const char *path,
     if (notmuch->path[strlen (notmuch->path) - 1] == '/')
 	notmuch->path[strlen (notmuch->path) - 1] = '\0';
 
-    notmuch->needs_upgrade = FALSE;
     notmuch->mode = mode;
     notmuch->atomic_nesting = 0;
     try {
@@ -686,37 +795,44 @@ notmuch_database_open (const char *path,
 	if (mode == NOTMUCH_DATABASE_MODE_READ_WRITE) {
 	    notmuch->xapian_db = new Xapian::WritableDatabase (xapian_path,
 							       Xapian::DB_CREATE_OR_OPEN);
-	    version = notmuch_database_get_version (notmuch);
-
-	    if (version > NOTMUCH_DATABASE_VERSION) {
-		fprintf (stderr,
-			 "Error: Notmuch database at %s\n"
-			 "       has a newer database format version (%u) than supported by this\n"
-			 "       version of notmuch (%u). Refusing to open this database in\n"
-			 "       read-write mode.\n",
-			 notmuch_path, version, NOTMUCH_DATABASE_VERSION);
-		notmuch->mode = NOTMUCH_DATABASE_MODE_READ_ONLY;
-		notmuch_database_destroy (notmuch);
-		notmuch = NULL;
-		status = NOTMUCH_STATUS_FILE_ERROR;
-		goto DONE;
-	    }
-
-	    if (version < NOTMUCH_DATABASE_VERSION)
-		notmuch->needs_upgrade = TRUE;
 	} else {
 	    notmuch->xapian_db = new Xapian::Database (xapian_path);
-	    version = notmuch_database_get_version (notmuch);
-	    if (version > NOTMUCH_DATABASE_VERSION)
-	    {
-		fprintf (stderr,
-			 "Warning: Notmuch database at %s\n"
-			 "         has a newer database format version (%u) than supported by this\n"
-			 "         version of notmuch (%u). Some operations may behave incorrectly,\n"
-			 "         (but the database will not be harmed since it is being opened\n"
-			 "         in read-only mode).\n",
-			 notmuch_path, version, NOTMUCH_DATABASE_VERSION);
-	    }
+	}
+
+	/* Check version.  As of database version 3, we represent
+	 * changes in terms of features, so assume a version bump
+	 * means a dramatically incompatible change. */
+	version = notmuch_database_get_version (notmuch);
+	if (version > NOTMUCH_DATABASE_VERSION) {
+	    fprintf (stderr,
+		     "Error: Notmuch database at %s\n"
+		     "       has a newer database format version (%u) than supported by this\n"
+		     "       version of notmuch (%u).\n",
+		     notmuch_path, version, NOTMUCH_DATABASE_VERSION);
+	    notmuch->mode = NOTMUCH_DATABASE_MODE_READ_ONLY;
+	    notmuch_database_destroy (notmuch);
+	    notmuch = NULL;
+	    status = NOTMUCH_STATUS_FILE_ERROR;
+	    goto DONE;
+	}
+
+	/* Check features. */
+	incompat_features = NULL;
+	notmuch->features = _parse_features (
+	    local, notmuch->xapian_db->get_metadata ("features").c_str (),
+	    version, mode == NOTMUCH_DATABASE_MODE_READ_WRITE ? 'w' : 'r',
+	    &incompat_features);
+	if (incompat_features) {
+	    fprintf (stderr,
+		     "Error: Notmuch database at %s\n"
+		     "       requires features (%s)\n"
+		     "       not supported by this version of notmuch.\n",
+		     notmuch_path, incompat_features);
+	    notmuch->mode = NOTMUCH_DATABASE_MODE_READ_ONLY;
+	    notmuch_database_destroy (notmuch);
+	    notmuch = NULL;
+	    status = NOTMUCH_STATUS_FILE_ERROR;
+	    goto DONE;
 	}
 
 	notmuch->last_doc_id = notmuch->xapian_db->get_lastdocid ();
@@ -1048,7 +1164,8 @@ notmuch_database_get_version (notmuch_database_t *notmuch)
 notmuch_bool_t
 notmuch_database_needs_upgrade (notmuch_database_t *notmuch)
 {
-    return notmuch->needs_upgrade;
+    return notmuch->mode == NOTMUCH_DATABASE_MODE_READ_WRITE &&
+	(NOTMUCH_FEATURES_CURRENT & ~notmuch->features);
 }
 
 static volatile sig_atomic_t do_progress_notify = 0;
@@ -1077,6 +1194,7 @@ notmuch_database_upgrade (notmuch_database_t *notmuch,
 						   double progress),
 			  void *closure)
 {
+    void *local = talloc_new (NULL);
     Xapian::WritableDatabase *db;
     struct sigaction action;
     struct itimerval timerval;
@@ -1114,6 +1232,10 @@ notmuch_database_upgrade (notmuch_database_t *notmuch,
 	timer_is_active = TRUE;
     }
 
+    /* Set the target features so we write out changes in the desired
+     * format. */
+    notmuch->features |= NOTMUCH_FEATURES_CURRENT;
+
     /* Before version 1, each message document had its filename in the
      * data field. Copy that into the new format by calling
      * notmuch_message_add_filename.
@@ -1226,6 +1348,7 @@ notmuch_database_upgrade (notmuch_database_t *notmuch,
 	notmuch_query_destroy (query);
     }
 
+    db->set_metadata ("features", _print_features (local, notmuch->features));
     db->set_metadata ("version", STRINGIFY (NOTMUCH_DATABASE_VERSION));
     db->flush ();
 
@@ -1302,6 +1425,7 @@ notmuch_database_upgrade (notmuch_database_t *notmuch,
 	sigaction (SIGALRM, &action, NULL);
     }
 
+    talloc_free (local);
     return NOTMUCH_STATUS_SUCCESS;
 }
 
-- 
2.0.0

[PATCH v3 05/13] test: Tool to build DB with specific version and features

[Austin Clements]

This will let us test basic version and feature handling.
---
 test/.gitignore         |  1 +
 test/Makefile.local     |  4 ++++
 test/make-db-version.cc | 35 +++++++++++++++++++++++++++++++++++
 3 files changed, 40 insertions(+)
 create mode 100644 test/make-db-version.cc

diff --git a/test/.gitignore b/test/.gitignore
index b3b706d..0f7d5bf 100644
--- a/test/.gitignore
+++ b/test/.gitignore
@@ -5,5 +5,6 @@ parse-time
 random-corpus
 smtp-dummy
 symbol-test
+make-db-version
 test-results
 tmp.*
diff --git a/test/Makefile.local b/test/Makefile.local
index 916dd0b..a2d58fc 100644
--- a/test/Makefile.local
+++ b/test/Makefile.local
@@ -35,6 +35,9 @@ $(dir)/symbol-test: $(dir)/symbol-test.o lib/$(LINKER_NAME)
 $(dir)/parse-time: $(dir)/parse-time.o parse-time-string/parse-time-string.o
 	$(call quiet,CC) $^ -o $@
 
+$(dir)/make-db-version: $(dir)/make-db-version.o
+	$(call quiet,CXX) $^ -o $@ $(XAPIAN_LDFLAGS)
+
 .PHONY: test check
 
 test_main_srcs=$(dir)/arg-test.c \
@@ -43,6 +46,7 @@ test_main_srcs=$(dir)/arg-test.c \
 	      $(dir)/parse-time.c \
 	      $(dir)/smtp-dummy.c \
 	      $(dir)/symbol-test.cc \
+	      $(dir)/make-db-version.cc \
 
 test_srcs=$(test_main_srcs) $(dir)/database-test.c
 
diff --git a/test/make-db-version.cc b/test/make-db-version.cc
new file mode 100644
index 0000000..fa80cac
--- /dev/null
+++ b/test/make-db-version.cc
@@ -0,0 +1,35 @@
+/* Create an empty notmuch database with a specific version and
+ * features. */
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <sys/stat.h>
+#include <sys/types.h>
+
+#include <xapian.h>
+
+int main(int argc, char **argv)
+{
+    if (argc != 4) {
+	fprintf (stderr, "Usage: %s mailpath version features\n", argv[0]);
+	exit (2);
+    }
+
+    std::string nmpath (argv[1]);
+    nmpath += "/.notmuch";
+    if (mkdir (nmpath.c_str (), 0777) < 0) {
+	perror (("failed to create " + nmpath).c_str ());
+	exit (1);
+    }
+
+    try {
+	Xapian::WritableDatabase db (
+	    nmpath + "/xapian", Xapian::DB_CREATE_OR_OPEN);
+	db.set_metadata ("version", argv[2]);
+	db.set_metadata ("features", argv[3]);
+	db.commit ();
+    } catch (const Xapian::Error &e) {
+	fprintf (stderr, "%s\n", e.get_description ().c_str ());
+	exit (1);
+    }
+}
-- 
2.0.0

[PATCH v3 06/13] test: Tests for future version and unknown feature handling

[Austin Clements]

---
 test/T550-db-features.sh | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 48 insertions(+)
 create mode 100755 test/T550-db-features.sh

diff --git a/test/T550-db-features.sh b/test/T550-db-features.sh
new file mode 100755
index 0000000..5569768
--- /dev/null
+++ b/test/T550-db-features.sh
@@ -0,0 +1,48 @@
+#!/usr/bin/env bash
+test_description="database version and feature compatibility"
+
+. ./test-lib.sh
+
+test_begin_subtest "future database versions abort open"
+${TEST_DIRECTORY}/make-db-version ${MAIL_DIR} 9999 ""
+output=$(notmuch search x 2>&1 | sed 's/\(database at\) .*/\1 FILENAME/')
+rm -rf ${MAIL_DIR}/.notmuch
+test_expect_equal "$output" "\
+Error: Notmuch database at FILENAME
+       has a newer database format version (9999) than supported by this
+       version of notmuch (3)."
+
+test_begin_subtest "unknown 'rw' feature aborts read/write open"
+${TEST_DIRECTORY}/make-db-version ${MAIL_DIR} 3 $'test feature\trw'
+output=$(notmuch new 2>&1 | sed 's/\(database at\) .*/\1 FILENAME/')
+rm -rf ${MAIL_DIR}/.notmuch
+test_expect_equal "$output" "\
+Error: Notmuch database at FILENAME
+       requires features (test feature)
+       not supported by this version of notmuch."
+
+test_begin_subtest "unknown 'rw' feature aborts read-only open"
+${TEST_DIRECTORY}/make-db-version ${MAIL_DIR} 3 $'test feature\trw'
+output=$(notmuch search x 2>&1 | sed 's/\(database at\) .*/\1 FILENAME/')
+rm -rf ${MAIL_DIR}/.notmuch
+test_expect_equal "$output" "\
+Error: Notmuch database at FILENAME
+       requires features (test feature)
+       not supported by this version of notmuch."
+
+test_begin_subtest "unknown 'w' feature aborts read/write open"
+${TEST_DIRECTORY}/make-db-version ${MAIL_DIR} 3 $'test feature\tw'
+output=$(notmuch new 2>&1 | sed 's/\(database at\) .*/\1 FILENAME/')
+rm -rf ${MAIL_DIR}/.notmuch
+test_expect_equal "$output" "\
+Error: Notmuch database at FILENAME
+       requires features (test feature)
+       not supported by this version of notmuch."
+
+test_begin_subtest "unknown 'w' feature does not abort read-only open"
+${TEST_DIRECTORY}/make-db-version ${MAIL_DIR} 3 $'test feature\tw'
+output=$(notmuch search x 2>&1 | sed 's/\(database at\) .*/\1 FILENAME/')
+rm -rf ${MAIL_DIR}/.notmuch
+test_expect_equal "$output" ""
+
+test_done
-- 
2.0.0

[PATCH v3 07/13] lib: Simplify upgrade code using a transaction

[Austin Clements]

Previously, the upgrade was organized as two passes -- an upgrade
pass, and a separate cleanup pass -- so the database was always in a
valid state.  This change substantially simplifies this code by
performing the upgrade in a transaction and combining both passes in
to one.  This 1) eliminates a lot of duplicate code between the
passes, 2) speeds up the upgrade process, 3) makes progress reporting
more accurate, 4) eliminates the potential for stale data if the
upgrade is interrupted during the cleanup pass, and 5) makes it easier
to reason about the safety of the upgrade code.
---
 lib/database.cc | 67 ++++++---------------------------------------------------
 1 file changed, 7 insertions(+), 60 deletions(-)

diff --git a/lib/database.cc b/lib/database.cc
index 29a56db..faeab51 100644
--- a/lib/database.cc
+++ b/lib/database.cc
@@ -1232,6 +1232,9 @@ notmuch_database_upgrade (notmuch_database_t *notmuch,
 	timer_is_active = TRUE;
     }
 
+    /* Perform the upgrade in a transaction. */
+    db->begin_transaction (true);
+
     /* Set the target features so we write out changes in the desired
      * format. */
     notmuch->features |= NOTMUCH_FEATURES_CURRENT;
@@ -1263,6 +1266,7 @@ notmuch_database_upgrade (notmuch_database_t *notmuch,
 	    filename = _notmuch_message_talloc_copy_data (message);
 	    if (filename && *filename != '\0') {
 		_notmuch_message_add_filename (message, filename);
+		_notmuch_message_clear_data (message);
 		_notmuch_message_sync (message);
 	    }
 	    talloc_free (filename);
@@ -1310,6 +1314,8 @@ notmuch_database_upgrade (notmuch_database_t *notmuch,
 						       NOTMUCH_FIND_CREATE, &status);
 		notmuch_directory_set_mtime (directory, mtime);
 		notmuch_directory_destroy (directory);
+
+		db->delete_document (*p);
 	    }
 	}
     }
@@ -1350,67 +1356,8 @@ notmuch_database_upgrade (notmuch_database_t *notmuch,
 
     db->set_metadata ("features", _print_features (local, notmuch->features));
     db->set_metadata ("version", STRINGIFY (NOTMUCH_DATABASE_VERSION));
-    db->flush ();
-
-    /* Now that the upgrade is complete we can remove the old data
-     * and documents that are no longer needed. */
-    if (version < 1) {
-	notmuch_query_t *query = notmuch_query_create (notmuch, "");
-	notmuch_messages_t *messages;
-	notmuch_message_t *message;
-	char *filename;
-
-	for (messages = notmuch_query_search_messages (query);
-	     notmuch_messages_valid (messages);
-	     notmuch_messages_move_to_next (messages))
-	{
-	    if (do_progress_notify) {
-		progress_notify (closure, (double) count / total);
-		do_progress_notify = 0;
-	    }
-
-	    message = notmuch_messages_get (messages);
-
-	    filename = _notmuch_message_talloc_copy_data (message);
-	    if (filename && *filename != '\0') {
-		_notmuch_message_clear_data (message);
-		_notmuch_message_sync (message);
-	    }
-	    talloc_free (filename);
-
-	    notmuch_message_destroy (message);
-	}
 
-	notmuch_query_destroy (query);
-    }
-
-    if (version < 1) {
-	Xapian::TermIterator t, t_end;
-
-	t_end = notmuch->xapian_db->allterms_end ("XTIMESTAMP");
-
-	for (t = notmuch->xapian_db->allterms_begin ("XTIMESTAMP");
-	     t != t_end;
-	     t++)
-	{
-	    Xapian::PostingIterator p, p_end;
-	    std::string term = *t;
-
-	    p_end = notmuch->xapian_db->postlist_end (term);
-
-	    for (p = notmuch->xapian_db->postlist_begin (term);
-		 p != p_end;
-		 p++)
-	    {
-		if (do_progress_notify) {
-		    progress_notify (closure, (double) count / total);
-		    do_progress_notify = 0;
-		}
-
-		db->delete_document (*p);
-	    }
-	}
-    }
+    db->commit_transaction ();
 
     if (timer_is_active) {
 	/* Now stop the timer. */
-- 
2.0.0

[PATCH v3 08/13] lib: Use database features to drive upgrade

[Austin Clements]

Previously, we had database version information hard-coded in the
upgrade code.  Slightly re-organize the upgrade process around the set
of new database features to be enabled by the upgrade.
---
 lib/database.cc | 24 +++++++++++++-----------
 1 file changed, 13 insertions(+), 11 deletions(-)

diff --git a/lib/database.cc b/lib/database.cc
index faeab51..69d775f 100644
--- a/lib/database.cc
+++ b/lib/database.cc
@@ -1195,11 +1195,12 @@ notmuch_database_upgrade (notmuch_database_t *notmuch,
 			  void *closure)
 {
     void *local = talloc_new (NULL);
+    Xapian::TermIterator t, t_end;
     Xapian::WritableDatabase *db;
     struct sigaction action;
     struct itimerval timerval;
     notmuch_bool_t timer_is_active = FALSE;
-    unsigned int version;
+    unsigned int target_features, new_features;
     notmuch_status_t status;
     unsigned int count = 0, total = 0;
 
@@ -1209,9 +1210,10 @@ notmuch_database_upgrade (notmuch_database_t *notmuch,
 
     db = static_cast <Xapian::WritableDatabase *> (notmuch->xapian_db);
 
-    version = notmuch_database_get_version (notmuch);
+    target_features = notmuch->features | NOTMUCH_FEATURES_CURRENT;
+    new_features = NOTMUCH_FEATURES_CURRENT & ~notmuch->features;
 
-    if (version >= NOTMUCH_DATABASE_VERSION)
+    if (! new_features)
 	return NOTMUCH_STATUS_SUCCESS;
 
     if (progress_notify) {
@@ -1237,18 +1239,17 @@ notmuch_database_upgrade (notmuch_database_t *notmuch,
 
     /* Set the target features so we write out changes in the desired
      * format. */
-    notmuch->features |= NOTMUCH_FEATURES_CURRENT;
+    notmuch->features = target_features;
 
     /* Before version 1, each message document had its filename in the
      * data field. Copy that into the new format by calling
      * notmuch_message_add_filename.
      */
-    if (version < 1) {
+    if (new_features & NOTMUCH_FEATURE_FILE_TERMS) {
 	notmuch_query_t *query = notmuch_query_create (notmuch, "");
 	notmuch_messages_t *messages;
 	notmuch_message_t *message;
 	char *filename;
-	Xapian::TermIterator t, t_end;
 
 	total = notmuch_query_count_messages (query);
 
@@ -1277,11 +1278,12 @@ notmuch_database_upgrade (notmuch_database_t *notmuch,
 	}
 
 	notmuch_query_destroy (query);
+    }
 
-	/* Also, before version 1 we stored directory timestamps in
-	 * XTIMESTAMP documents instead of the current XDIRECTORY
-	 * documents. So copy those as well. */
-
+    /* Also, before version 1 we stored directory timestamps in
+     * XTIMESTAMP documents instead of the current XDIRECTORY
+     * documents. So copy those as well. */
+    if (new_features & NOTMUCH_FEATURE_DIRECTORY_DOCS) {
 	t_end = notmuch->xapian_db->allterms_end ("XTIMESTAMP");
 
 	for (t = notmuch->xapian_db->allterms_begin ("XTIMESTAMP");
@@ -1325,7 +1327,7 @@ notmuch_database_upgrade (notmuch_database_t *notmuch,
      * stemmed. Change it to the current boolean prefix. Add "path:"
      * prefixes while at it.
      */
-    if (version < 2) {
+    if (new_features & NOTMUCH_FEATURE_BOOL_FOLDER) {
 	notmuch_query_t *query = notmuch_query_create (notmuch, "");
 	notmuch_messages_t *messages;
 	notmuch_message_t *message;
-- 
2.0.0

[PATCH v3 09/13] lib: Reorganize upgrade around document types

[Austin Clements]

Rather than potentially making multiple passes over the same type of
data in the database, reorganize upgrade around each type of data that
may be upgraded.  This eliminates code duplication, will make
multi-version upgrades faster, and will let us improve progress
reporting.
---
 lib/database.cc | 72 +++++++++++++++++++++------------------------------------
 1 file changed, 26 insertions(+), 46 deletions(-)

diff --git a/lib/database.cc b/lib/database.cc
index 69d775f..31e6a93 100644
--- a/lib/database.cc
+++ b/lib/database.cc
@@ -1241,11 +1241,9 @@ notmuch_database_upgrade (notmuch_database_t *notmuch,
      * format. */
     notmuch->features = target_features;
 
-    /* Before version 1, each message document had its filename in the
-     * data field. Copy that into the new format by calling
-     * notmuch_message_add_filename.
-     */
-    if (new_features & NOTMUCH_FEATURE_FILE_TERMS) {
+    /* Perform per-message upgrades. */
+    if (new_features &
+	(NOTMUCH_FEATURE_FILE_TERMS | NOTMUCH_FEATURE_BOOL_FOLDER)) {
 	notmuch_query_t *query = notmuch_query_create (notmuch, "");
 	notmuch_messages_t *messages;
 	notmuch_message_t *message;
@@ -1264,13 +1262,27 @@ notmuch_database_upgrade (notmuch_database_t *notmuch,
 
 	    message = notmuch_messages_get (messages);
 
-	    filename = _notmuch_message_talloc_copy_data (message);
-	    if (filename && *filename != '\0') {
-		_notmuch_message_add_filename (message, filename);
-		_notmuch_message_clear_data (message);
-		_notmuch_message_sync (message);
+	    /* Before version 1, each message document had its
+	     * filename in the data field. Copy that into the new
+	     * format by calling notmuch_message_add_filename.
+	     */
+	    if (new_features & NOTMUCH_FEATURE_FILE_TERMS) {
+		filename = _notmuch_message_talloc_copy_data (message);
+		if (filename && *filename != '\0') {
+		    _notmuch_message_add_filename (message, filename);
+		    _notmuch_message_clear_data (message);
+		}
+		talloc_free (filename);
 	    }
-	    talloc_free (filename);
+
+	    /* Prior to version 2, the "folder:" prefix was
+	     * probabilistic and stemmed. Change it to the current
+	     * boolean prefix. Add "path:" prefixes while at it.
+	     */
+	    if (new_features & NOTMUCH_FEATURE_BOOL_FOLDER)
+		_notmuch_message_upgrade_folder (message);
+
+	    _notmuch_message_sync (message);
 
 	    notmuch_message_destroy (message);
 
@@ -1280,7 +1292,9 @@ notmuch_database_upgrade (notmuch_database_t *notmuch,
 	notmuch_query_destroy (query);
     }
 
-    /* Also, before version 1 we stored directory timestamps in
+    /* Perform per-directory upgrades. */
+
+    /* Before version 1 we stored directory timestamps in
      * XTIMESTAMP documents instead of the current XDIRECTORY
      * documents. So copy those as well. */
     if (new_features & NOTMUCH_FEATURE_DIRECTORY_DOCS) {
@@ -1322,40 +1336,6 @@ notmuch_database_upgrade (notmuch_database_t *notmuch,
 	}
     }
 
-    /*
-     * Prior to version 2, the "folder:" prefix was probabilistic and
-     * stemmed. Change it to the current boolean prefix. Add "path:"
-     * prefixes while at it.
-     */
-    if (new_features & NOTMUCH_FEATURE_BOOL_FOLDER) {
-	notmuch_query_t *query = notmuch_query_create (notmuch, "");
-	notmuch_messages_t *messages;
-	notmuch_message_t *message;
-
-	count = 0;
-	total = notmuch_query_count_messages (query);
-
-	for (messages = notmuch_query_search_messages (query);
-	     notmuch_messages_valid (messages);
-	     notmuch_messages_move_to_next (messages)) {
-	    if (do_progress_notify) {
-		progress_notify (closure, (double) count / total);
-		do_progress_notify = 0;
-	    }
-
-	    message = notmuch_messages_get (messages);
-
-	    _notmuch_message_upgrade_folder (message);
-	    _notmuch_message_sync (message);
-
-	    notmuch_message_destroy (message);
-
-	    count++;
-	}
-
-	notmuch_query_destroy (query);
-    }
-
     db->set_metadata ("features", _print_features (local, notmuch->features));
     db->set_metadata ("version", STRINGIFY (NOTMUCH_DATABASE_VERSION));
 
-- 
2.0.0

[PATCH v3 10/13] lib: Report progress for combined upgrade operation

[Austin Clements]

Previously, some parts of upgrade didn't report progress and for
others it was possible for the progress meter to restart at 0 part way
through the upgrade because each stage was reported separately.

Fix this by computing the total amount of work that needs to be done
up-front and updating completed work monotonically.
---
 lib/database.cc | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/lib/database.cc b/lib/database.cc
index 31e6a93..04b3790 100644
--- a/lib/database.cc
+++ b/lib/database.cc
@@ -1234,6 +1234,19 @@ notmuch_database_upgrade (notmuch_database_t *notmuch,
 	timer_is_active = TRUE;
     }
 
+    /* Figure out how much total work we need to do. */
+    if (new_features &
+	(NOTMUCH_FEATURE_FILE_TERMS | NOTMUCH_FEATURE_BOOL_FOLDER)) {
+	notmuch_query_t *query = notmuch_query_create (notmuch, "");
+	total += notmuch_query_count_messages (query);
+	notmuch_query_destroy (query);
+    }
+    if (new_features & NOTMUCH_FEATURE_DIRECTORY_DOCS) {
+	t_end = db->allterms_end ("XTIMESTAMP");
+	for (t = db->allterms_begin ("XTIMESTAMP"); t != t_end; t++)
+	    ++total;
+    }
+
     /* Perform the upgrade in a transaction. */
     db->begin_transaction (true);
 
@@ -1249,8 +1262,6 @@ notmuch_database_upgrade (notmuch_database_t *notmuch,
 	notmuch_message_t *message;
 	char *filename;
 
-	total = notmuch_query_count_messages (query);
-
 	for (messages = notmuch_query_search_messages (query);
 	     notmuch_messages_valid (messages);
 	     notmuch_messages_move_to_next (messages))
@@ -1333,6 +1344,8 @@ notmuch_database_upgrade (notmuch_database_t *notmuch,
 
 		db->delete_document (*p);
 	    }
+
+	    ++count;
 	}
     }
 
-- 
2.0.0

[PATCH v3 11/13] lib: Support empty header values in database

[Austin Clements]

Commit 567bcbc2 introduced support for storing various headers in
document values.  However, doing so in a backwards-compatible way
meant that genuinely empty header values could not be distinguished
from the old behavior of not storing the headers at all, so these
required parsing the original message.

Now that we have database features, new databases can declare that all
messages have header values, so if we have this feature flag, we can
use the stored header value even if it's the empty string.

This requires slight cleanup to notmuch_message_get_header, since the
code previously couldn't distinguish between empty headers and headers
that are never stored in the database (previously this distinction
didn't matter).
---
 lib/message.cc | 45 +++++++++++++++++++++++++++------------------
 1 file changed, 27 insertions(+), 18 deletions(-)

diff --git a/lib/message.cc b/lib/message.cc
index e6a5a5a..4fc427f 100644
--- a/lib/message.cc
+++ b/lib/message.cc
@@ -412,26 +412,35 @@ _notmuch_message_ensure_message_file (notmuch_message_t *message)
 const char *
 notmuch_message_get_header (notmuch_message_t *message, const char *header)
 {
-    try {
-	    std::string value;
-
-	    /* Fetch header from the appropriate xapian value field if
-	     * available */
-	    if (strcasecmp (header, "from") == 0)
-		value = message->doc.get_value (NOTMUCH_VALUE_FROM);
-	    else if (strcasecmp (header, "subject") == 0)
-		value = message->doc.get_value (NOTMUCH_VALUE_SUBJECT);
-	    else if (strcasecmp (header, "message-id") == 0)
-		value = message->doc.get_value (NOTMUCH_VALUE_MESSAGE_ID);
-
-	    if (!value.empty())
+    Xapian::valueno slot = Xapian::BAD_VALUENO;
+
+    /* Fetch header from the appropriate xapian value field if
+     * available */
+    if (strcasecmp (header, "from") == 0)
+	slot = NOTMUCH_VALUE_FROM;
+    else if (strcasecmp (header, "subject") == 0)
+	slot = NOTMUCH_VALUE_SUBJECT;
+    else if (strcasecmp (header, "message-id") == 0)
+	slot = NOTMUCH_VALUE_MESSAGE_ID;
+
+    if (slot != Xapian::BAD_VALUENO) {
+	try {
+	    std::string value = message->doc.get_value (slot);
+
+	    /* If we have NOTMUCH_FEATURE_FROM_SUBJECT_ID_VALUES, then
+	     * empty values indicate empty headers.  If we don't, then
+	     * it could just mean we didn't record the header. */
+	    if ((message->notmuch->features &
+		 NOTMUCH_FEATURE_FROM_SUBJECT_ID_VALUES) ||
+		! value.empty())
 		return talloc_strdup (message, value.c_str ());
 
-    } catch (Xapian::Error &error) {
-	fprintf (stderr, "A Xapian exception occurred when reading header: %s\n",
-		 error.get_msg().c_str());
-	message->notmuch->exception_reported = TRUE;
-	return NULL;
+	} catch (Xapian::Error &error) {
+	    fprintf (stderr, "A Xapian exception occurred when reading header: %s\n",
+		     error.get_msg().c_str());
+	    message->notmuch->exception_reported = TRUE;
+	    return NULL;
+	}
     }
 
     /* Otherwise fall back to parsing the file */
-- 
2.0.0

[PATCH v3 12/13] lib: Return an error from operations that require an upgrade

[Austin Clements]

Previously, there was no protection against a caller invoking an
operation on an old database version that would effectively corrupt
the database by treating it like a newer version.

According to notmuch.h, any caller that opens the database in
read/write mode is supposed to check if the database needs upgrading
and perform an upgrade if it does.  This would protect against this,
but nobody (even the CLI) actually does this.

However, with features, it's easy to protect against incompatible
operations on a fine-grained basis.  This lightweight change allows
callers to safely operate on old database versions, while preventing
specific operations that would corrupt the database with an
informative error message.
---
 lib/database.cc  |  5 +++++
 lib/directory.cc |  5 +++++
 lib/message.cc   |  8 ++++++++
 lib/notmuch.h    | 16 ++++++++++++++++
 4 files changed, 34 insertions(+)

diff --git a/lib/database.cc b/lib/database.cc
index 04b3790..d90a924 100644
--- a/lib/database.cc
+++ b/lib/database.cc
@@ -310,6 +310,8 @@ notmuch_status_to_string (notmuch_status_t status)
 	return "Unbalanced number of calls to notmuch_database_begin_atomic/end_atomic";
     case NOTMUCH_STATUS_UNSUPPORTED_OPERATION:
 	return "Unsupported operation";
+    case NOTMUCH_STATUS_UPGRADE_REQUIRED:
+	return "Operation requires a database upgrade";
     default:
     case NOTMUCH_STATUS_LAST_STATUS:
 	return "Unknown error status value";
@@ -2219,6 +2221,9 @@ notmuch_database_find_message_by_filename (notmuch_database_t *notmuch,
     if (message_ret == NULL)
 	return NOTMUCH_STATUS_NULL_POINTER;
 
+    if (! (notmuch->features & NOTMUCH_FEATURE_FILE_TERMS))
+	return NOTMUCH_STATUS_UPGRADE_REQUIRED;
+
     /* return NULL on any failure */
     *message_ret = NULL;
 
diff --git a/lib/directory.cc b/lib/directory.cc
index 6a3ffed..8daaec8 100644
--- a/lib/directory.cc
+++ b/lib/directory.cc
@@ -105,6 +105,11 @@ _notmuch_directory_create (notmuch_database_t *notmuch,
     const char *db_path;
     notmuch_bool_t create = (flags & NOTMUCH_FIND_CREATE);
 
+    if (! (notmuch->features & NOTMUCH_FEATURE_DIRECTORY_DOCS)) {
+	*status_ret = NOTMUCH_STATUS_UPGRADE_REQUIRED;
+	return NULL;
+    }
+
     *status_ret = NOTMUCH_STATUS_SUCCESS;
 
     path = _notmuch_database_relative_path (notmuch, path);
diff --git a/lib/message.cc b/lib/message.cc
index 4fc427f..1618e81 100644
--- a/lib/message.cc
+++ b/lib/message.cc
@@ -653,6 +653,10 @@ _notmuch_message_add_filename (notmuch_message_t *message,
     if (filename == NULL)
 	INTERNAL_ERROR ("Message filename cannot be NULL.");
 
+    if (! (message->notmuch->features & NOTMUCH_FEATURE_FILE_TERMS) ||
+	! (message->notmuch->features & NOTMUCH_FEATURE_BOOL_FOLDER))
+	return NOTMUCH_STATUS_UPGRADE_REQUIRED;
+
     relative = _notmuch_database_relative_path (message->notmuch, filename);
 
     status = _notmuch_database_split_path (local, relative, &directory, NULL);
@@ -697,6 +701,10 @@ _notmuch_message_remove_filename (notmuch_message_t *message,
     notmuch_private_status_t private_status;
     notmuch_status_t status;
 
+    if (! (message->notmuch->features & NOTMUCH_FEATURE_FILE_TERMS) ||
+	! (message->notmuch->features & NOTMUCH_FEATURE_BOOL_FOLDER))
+	return NOTMUCH_STATUS_UPGRADE_REQUIRED;
+
     status = _notmuch_database_filename_to_direntry (
 	local, message->notmuch, filename, NOTMUCH_FIND_LOOKUP, &direntry);
     if (status || !direntry)
diff --git a/lib/notmuch.h b/lib/notmuch.h
index 3c5ec98..cbf2ba5 100644
--- a/lib/notmuch.h
+++ b/lib/notmuch.h
@@ -160,6 +160,10 @@ typedef enum _notmuch_status {
      */
     NOTMUCH_STATUS_UNSUPPORTED_OPERATION,
     /**
+     * The operation requires a database upgrade.
+     */
+    NOTMUCH_STATUS_UPGRADE_REQUIRED,
+    /**
      * Not an actual status value. Just a way to find out how many
      * valid status values there are.
      */
@@ -438,6 +442,9 @@ notmuch_database_end_atomic (notmuch_database_t *notmuch);
  *
  * NOTMUCH_STATUS_XAPIAN_EXCEPTION: A Xapian exception occurred;
  *	directory not retrieved.
+ *
+ * NOTMUCH_STATUS_UPGRADE_REQUIRED: The caller must upgrade the
+ * 	database to use this function.
  */
 notmuch_status_t
 notmuch_database_get_directory (notmuch_database_t *database,
@@ -490,6 +497,9 @@ notmuch_database_get_directory (notmuch_database_t *database,
  *
  * NOTMUCH_STATUS_READ_ONLY_DATABASE: Database was opened in read-only
  *	mode so no message can be added.
+ *
+ * NOTMUCH_STATUS_UPGRADE_REQUIRED: The caller must upgrade the
+ * 	database to use this function.
  */
 notmuch_status_t
 notmuch_database_add_message (notmuch_database_t *database,
@@ -520,6 +530,9 @@ notmuch_database_add_message (notmuch_database_t *database,
  *
  * NOTMUCH_STATUS_READ_ONLY_DATABASE: Database was opened in read-only
  *	mode so no message can be removed.
+ *
+ * NOTMUCH_STATUS_UPGRADE_REQUIRED: The caller must upgrade the
+ * 	database to use this function.
  */
 notmuch_status_t
 notmuch_database_remove_message (notmuch_database_t *database,
@@ -575,6 +588,9 @@ notmuch_database_find_message (notmuch_database_t *database,
  * NOTMUCH_STATUS_OUT_OF_MEMORY: Out of memory, creating the message object
  *
  * NOTMUCH_STATUS_XAPIAN_EXCEPTION: A Xapian exception occurred
+ *
+ * NOTMUCH_STATUS_UPGRADE_REQUIRED: The caller must upgrade the
+ * 	database to use this function.
  */
 notmuch_status_t
 notmuch_database_find_message_by_filename (notmuch_database_t *notmuch,
-- 
2.0.0

[PATCH v3 13/13] lib: Update doc of notmuch_database_{needs_upgrade, upgrade}

[Austin Clements]

Clients are no longer required to call these functions after opening a
database in read/write mode (which is good, because almost none of
them do!).
---
 lib/notmuch.h | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/lib/notmuch.h b/lib/notmuch.h
index cbf2ba5..d771eb2 100644
--- a/lib/notmuch.h
+++ b/lib/notmuch.h
@@ -352,22 +352,25 @@ unsigned int
 notmuch_database_get_version (notmuch_database_t *database);
 
 /**
- * Does this database need to be upgraded before writing to it?
+ * Is the database behind the latest supported database version?
  *
- * If this function returns TRUE then no functions that modify the
- * database (notmuch_database_add_message, notmuch_message_add_tag,
- * notmuch_directory_set_mtime, etc.) will work unless the function
- * notmuch_database_upgrade is called successfully first.
+ * If this function returns TRUE, then the caller may call
+ * notmuch_database_upgrade to upgrade the database.  If the caller
+ * does not upgrade an out-of-date database, then some functions may
+ * fail with NOTMUCH_STATUS_UPGRADE_REQUIRED.
  */
 notmuch_bool_t
 notmuch_database_needs_upgrade (notmuch_database_t *database);
 
 /**
- * Upgrade the current database.
+ * Upgrade the current database to the latest supported version.
  *
- * After opening a database in read-write mode, the client should
- * check if an upgrade is needed (notmuch_database_needs_upgrade) and
- * if so, upgrade with this function before making any modifications.
+ * This ensures that all current notmuch functionality will be
+ * available on the database.  After opening a database in read-write
+ * mode, it is recommended that clients check if an upgrade is needed
+ * (notmuch_database_needs_upgrade) and if so, upgrade with this
+ * function before making any modifications.  If
+ * notmuch_database_needs_upgrade returns FALSE, this will be a no-op.
  *
  * The optional progress_notify callback can be used by the caller to
  * provide progress indication to the user. If non-NULL it will be
-- 
2.0.0

Re: [PATCH v3 02/13] util: Const version of strtok_len

[David Bremner]

Austin Clements <amdragon@MIT.EDU> writes:

> Because of limitations in the C type system, we can't a strtok_len
> that can work on both const string and non-const strings.  The C
> library solves this by taking a const char* and returning a char*
> in functions like this (e.g., strchr), but that's not const-safe.
> Solve it by introducing strtok_len_c, a version of strtok_len for
> const strings.

pushed the first two patches in the series

d

Re: [PATCH v3 00/13] Implement and use database "features"

[Tomi Ollila]

On Fri, Aug 01 2014, Austin Clements <amdragon@MIT.EDU> wrote:

> This is v3 of id:1406652492-27803-1-git-send-email-amdragon@mit.edu.
> This fixes one issue and tidies up another thing in
> notmuch_database_upgrade I found while working on change tracking
> support.  Most of the patches are logically identical to v2, but the
> changes ripple through the patch context, so I'm sending a new series.
>
> First, this updates notmuch->features before starting the upgrade,
> rather than after, so that functions called by upgrade will use the
> new database features instead of the old (this didn't matter in this
> series because nothing modified the database differently depending on
> features).  Second, this combines multiple _notmuch_message_sync calls
> into one, which cleans up the code, should further improve upgrade
> performance, and makes way for additional per-message upgrades.

This series looks good to me. I looked through the diffs a few times and
notmuch_database_upgrade() in lib/database.cc to see that in full.
Tests pass (also T530-upgrade now that I downloaded that one test database.)

I googled answers to few questions along the review; one thing still
interests me -- is there potential to have speed/memory problems
when doing upgrade transaction in large/very large databases. And
how long will the (final) commit_transaction() take (i.e how
many times handle_sigalrm() is called while that is in progress...)

(my guess is that this transaction just builds a new revision and
if it is never committed the revision is never used -- and data is
written there in some batches of suitable size -- so memory usage
and transaction commit time is O(1))

Tomi

>
> The diff from v2 is below (excluding patch 2, which David pushed).
>
> diff --git a/lib/database.cc b/lib/database.cc
> index b323691..d90a924 100644
> --- a/lib/database.cc
> +++ b/lib/database.cc
> @@ -1252,6 +1252,10 @@ notmuch_database_upgrade (notmuch_database_t *notmuch,
>      /* Perform the upgrade in a transaction. */
>      db->begin_transaction (true);
>  
> +    /* Set the target features so we write out changes in the desired
> +     * format. */
> +    notmuch->features = target_features;
> +
>      /* Perform per-message upgrades. */
>      if (new_features &
>  	(NOTMUCH_FEATURE_FILE_TERMS | NOTMUCH_FEATURE_BOOL_FOLDER)) {
> @@ -1280,7 +1284,6 @@ notmuch_database_upgrade (notmuch_database_t *notmuch,
>  		if (filename && *filename != '\0') {
>  		    _notmuch_message_add_filename (message, filename);
>  		    _notmuch_message_clear_data (message);
> -		    _notmuch_message_sync (message);
>  		}
>  		talloc_free (filename);
>  	    }
> @@ -1289,10 +1292,10 @@ notmuch_database_upgrade (notmuch_database_t *notmuch,
>  	     * probabilistic and stemmed. Change it to the current
>  	     * boolean prefix. Add "path:" prefixes while at it.
>  	     */
> -	    if (new_features & NOTMUCH_FEATURE_BOOL_FOLDER) {
> +	    if (new_features & NOTMUCH_FEATURE_BOOL_FOLDER)
>  		_notmuch_message_upgrade_folder (message);
> -		_notmuch_message_sync (message);
> -	    }
> +
> +	    _notmuch_message_sync (message);
>  
>  	    notmuch_message_destroy (message);
>  
> @@ -1348,7 +1351,6 @@ notmuch_database_upgrade (notmuch_database_t *notmuch,
>  	}
>      }
>  
> -    notmuch->features = target_features;
>      db->set_metadata ("features", _print_features (local, notmuch->features));
>      db->set_metadata ("version", STRINGIFY (NOTMUCH_DATABASE_VERSION));
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH v3 00/13] Implement and use database "features"

[Austin Clements]

Quoth Tomi Ollila on Aug 08 at 12:55 am:
> On Fri, Aug 01 2014, Austin Clements <amdragon@MIT.EDU> wrote:
> 
> > This is v3 of id:1406652492-27803-1-git-send-email-amdragon@mit.edu.
> > This fixes one issue and tidies up another thing in
> > notmuch_database_upgrade I found while working on change tracking
> > support.  Most of the patches are logically identical to v2, but the
> > changes ripple through the patch context, so I'm sending a new series.
> >
> > First, this updates notmuch->features before starting the upgrade,
> > rather than after, so that functions called by upgrade will use the
> > new database features instead of the old (this didn't matter in this
> > series because nothing modified the database differently depending on
> > features).  Second, this combines multiple _notmuch_message_sync calls
> > into one, which cleans up the code, should further improve upgrade
> > performance, and makes way for additional per-message upgrades.
> 
> This series looks good to me. I looked through the diffs a few times and
> notmuch_database_upgrade() in lib/database.cc to see that in full.
> Tests pass (also T530-upgrade now that I downloaded that one test database.)
> 
> I googled answers to few questions along the review; one thing still
> interests me -- is there potential to have speed/memory problems
> when doing upgrade transaction in large/very large databases. And
> how long will the (final) commit_transaction() take (i.e how
> many times handle_sigalrm() is called while that is in progress...)
> 
> (my guess is that this transaction just builds a new revision and
> if it is never committed the revision is never used -- and data is
> written there in some batches of suitable size -- so memory usage
> and transaction commit time is O(1))

Your guess is basically right.  Xapian periodically flushes stuff to
disk during a transaction basically just like it does when not in a
transaction; AFAIK the only difference is when it flushes out the new
revision number and updated free block metadata.  Hence, speed and
memory use aren't affected by the use of a transaction, and
commit_transaction isn't particularly sensitive to how big the
transaction is.

> Tomi
> 
> >
> > The diff from v2 is below (excluding patch 2, which David pushed).
> >
> > diff --git a/lib/database.cc b/lib/database.cc
> > index b323691..d90a924 100644
> > --- a/lib/database.cc
> > +++ b/lib/database.cc
> > @@ -1252,6 +1252,10 @@ notmuch_database_upgrade (notmuch_database_t *notmuch,
> >      /* Perform the upgrade in a transaction. */
> >      db->begin_transaction (true);
> >  
> > +    /* Set the target features so we write out changes in the desired
> > +     * format. */
> > +    notmuch->features = target_features;
> > +
> >      /* Perform per-message upgrades. */
> >      if (new_features &
> >  	(NOTMUCH_FEATURE_FILE_TERMS | NOTMUCH_FEATURE_BOOL_FOLDER)) {
> > @@ -1280,7 +1284,6 @@ notmuch_database_upgrade (notmuch_database_t *notmuch,
> >  		if (filename && *filename != '\0') {
> >  		    _notmuch_message_add_filename (message, filename);
> >  		    _notmuch_message_clear_data (message);
> > -		    _notmuch_message_sync (message);
> >  		}
> >  		talloc_free (filename);
> >  	    }
> > @@ -1289,10 +1292,10 @@ notmuch_database_upgrade (notmuch_database_t *notmuch,
> >  	     * probabilistic and stemmed. Change it to the current
> >  	     * boolean prefix. Add "path:" prefixes while at it.
> >  	     */
> > -	    if (new_features & NOTMUCH_FEATURE_BOOL_FOLDER) {
> > +	    if (new_features & NOTMUCH_FEATURE_BOOL_FOLDER)
> >  		_notmuch_message_upgrade_folder (message);
> > -		_notmuch_message_sync (message);
> > -	    }
> > +
> > +	    _notmuch_message_sync (message);
> >  
> >  	    notmuch_message_destroy (message);
> >  
> > @@ -1348,7 +1351,6 @@ notmuch_database_upgrade (notmuch_database_t *notmuch,
> >  	}
> >      }
> >  
> > -    notmuch->features = target_features;
> >      db->set_metadata ("features", _print_features (local, notmuch->features));
> >      db->set_metadata ("version", STRINGIFY (NOTMUCH_DATABASE_VERSION));
> >

Re: [PATCH v3 03/13] new: Don't report version after upgrade

[Jani Nikula]

On Fri, 01 Aug 2014, Austin Clements <amdragon@MIT.EDU> wrote:
> The version number has always been pretty meaningless to the user and
> it's about to become even more meaningless with the introduction of
> "features".  Hopefully, the database will remain on version 3 for some
> time to come; however, the introduction of new features over time in
> version 3 will necessitate upgrades within version 3.  It would be
> confusing if we always tell the user they've been "upgraded to version
> 3".  If the user wants to know what's new, they should read the news.

I think this is good for now.

What do you think about adding notmuch_database_get_features(), and
printing that?

BR,
Jani.


> ---
>  notmuch-new.c        | 3 +--
>  test/T530-upgrade.sh | 2 +-
>  2 files changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/notmuch-new.c b/notmuch-new.c
> index d269c7c..b7590a8 100644
> --- a/notmuch-new.c
> +++ b/notmuch-new.c
> @@ -1023,8 +1023,7 @@ notmuch_new_command (notmuch_config_t *config, int argc, char *argv[])
>  				      add_files_state.verbosity >= VERBOSITY_NORMAL ? upgrade_print_progress : NULL,
>  				      &add_files_state);
>  	    if (add_files_state.verbosity >= VERBOSITY_NORMAL)
> -		printf ("Your notmuch database has now been upgraded to database format version %u.\n",
> -		    notmuch_database_get_version (notmuch));
> +		printf ("Your notmuch database has now been upgraded.\n");
>  	}
>  
>  	add_files_state.total_files = 0;
> diff --git a/test/T530-upgrade.sh b/test/T530-upgrade.sh
> index 7d5d5aa..c4c4ac8 100755
> --- a/test/T530-upgrade.sh
> +++ b/test/T530-upgrade.sh
> @@ -33,7 +33,7 @@ test_expect_equal "$output" "\
>  Welcome to a new version of notmuch! Your database will now be upgraded.
>  This process is safe to interrupt.
>  Backing up tags to FILENAME
> -Your notmuch database has now been upgraded to database format version 2.
> +Your notmuch database has now been upgraded.
>  No new mail."
>  
>  test_begin_subtest "tag backup matches pre-upgrade dump"
> -- 
> 2.0.0
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH v3 04/13] lib: Database version 3: Introduce fine-grained "features"

[Jani Nikula]

On Fri, 01 Aug 2014, Austin Clements <amdragon@MIT.EDU> wrote:
> Previously, our database schema was versioned by a single number.
> Each database schema change had to occur "atomically" in Notmuch's
> development history: before some commit, Notmuch used version N, after
> that commit, it used version N+1.  Hence, each new schema version
> could introduce only one change, the task of developing a schema
> change fell on a single person, and it all had to happen and be
> perfect in a single commit series.  This made introducing a new schema
> version hard.  We've seen only two schema changes in the history of
> Notmuch.
>
> This commit introduces database schema version 3; hopefully the last
> schema version we'll need for a while.  With this version, we switch
> from a single version number to "features": a set of named,
> independent aspects of the database schema.
>
> Features should make backwards compatibility easier.  For many things,
> it should be easy to support databases both with and without a
> feature, which will allow us to make upgrades optional and will enable
> "unstable" features that can be developed and tested over time.
>
> Features also make forwards compatibility easier.  The features
> recorded in a database include "compatibility flags," which can
> indicate to an older version of Notmuch when it must support a given
> feature to open the database for read or for write.  This lets us
> replace the old vague "I don't recognize this version, so something
> might go wrong, but I promise to try my best" warnings upon opening a
> database with an unknown version with precise errors.  If a database
> is safe to open for read/write despite unknown features, an older
> version will know that and issue no message at all.  If the database
> is not safe to open for read/write because of unknown features, an
> older version will know that, too, and can tell the user exactly which
> required features it lacks support for.

I agree with the change overall; it might be useful to have another set
of eyes on the patch though.

> ---
>  lib/database-private.h |  57 ++++++++++++++-
>  lib/database.cc        | 190 ++++++++++++++++++++++++++++++++++++++++---------
>  2 files changed, 213 insertions(+), 34 deletions(-)
>
> diff --git a/lib/database-private.h b/lib/database-private.h
> index d3e65fd..2ffab33 100644
> --- a/lib/database-private.h
> +++ b/lib/database-private.h
> @@ -41,11 +41,15 @@ struct _notmuch_database {
>  
>      char *path;
>  
> -    notmuch_bool_t needs_upgrade;
>      notmuch_database_mode_t mode;
>      int atomic_nesting;
>      Xapian::Database *xapian_db;
>  
> +    /* Bit mask of features used by this database.  Features are
> +     * named, independent aspects of the database schema.  This is a
> +     * bitwise-OR of NOTMUCH_FEATURE_* values (below). */
> +    unsigned int features;
> +
>      unsigned int last_doc_id;
>      uint64_t last_thread_id;
>  
> @@ -55,6 +59,57 @@ struct _notmuch_database {
>      Xapian::ValueRangeProcessor *date_range_processor;
>  };
>  
> +/* Bit masks for _notmuch_database::features. */
> +enum {
> +    /* If set, file names are stored in "file-direntry" terms.  If
> +     * unset, file names are stored in document data.
> +     *
> +     * Introduced: version 1.  Implementation support: both for read;
> +     * required for write. */

Maybe I'm dense, but the implementation support comments could be
clearer.

> +    NOTMUCH_FEATURE_FILE_TERMS = 1 << 0,
> +
> +    /* If set, directory timestamps are stored in documents with
> +     * XDIRECTORY terms and relative paths.  If unset, directory
> +     * timestamps are stored in documents with XTIMESTAMP terms and
> +     * absolute paths.
> +     *
> +     * Introduced: version 1.  Implementation support: required. */
> +    NOTMUCH_FEATURE_DIRECTORY_DOCS = 1 << 1,
> +
> +    /* If set, the from, subject, and message-id headers are stored in
> +     * message document values.  If unset, message documents *may*
> +     * have these values, but if the value is empty, it must be
> +     * retrieved from the message file.
> +     *
> +     * Introduced: optional in version 1, required as of version 3.
> +     * Implementation support: both.
> +     */
> +    NOTMUCH_FEATURE_FROM_SUBJECT_ID_VALUES = 1 << 2,
> +
> +    /* If set, folder terms are boolean and path terms exist.  If
> +     * unset, folder terms are probabilistic and stemmed and path
> +     * terms do not exist.
> +     *
> +     * Introduced: version 2.  Implementation support: required. */
> +    NOTMUCH_FEATURE_BOOL_FOLDER = 1 << 3,
> +};
> +
> +/* Prior to database version 3, features were implied by the database
> + * version number, so hard-code them for earlier versions. */
> +#define NOTMUCH_FEATURES_V0 (0)
> +#define NOTMUCH_FEATURES_V1 (NOTMUCH_FEATURES_V0 | NOTMUCH_FEATURE_FILE_TERMS | \
> +			     NOTMUCH_FEATURE_DIRECTORY_DOCS)
> +#define NOTMUCH_FEATURES_V2 (NOTMUCH_FEATURES_V1 | NOTMUCH_FEATURE_BOOL_FOLDER)
> +
> +/* Current database features.  If any of these are missing from a
> + * database, request an upgrade.
> + * NOTMUCH_FEATURE_FROM_SUBJECT_ID_VALUES is not included because
> + * upgrade doesn't currently introduce the feature (though brand new
> + * databases will have it). */
> +#define NOTMUCH_FEATURES_CURRENT \
> +    (NOTMUCH_FEATURE_FILE_TERMS | NOTMUCH_FEATURE_DIRECTORY_DOCS | \
> +     NOTMUCH_FEATURE_BOOL_FOLDER)
> +
>  /* Return the list of terms from the given iterator matching a prefix.
>   * The prefix will be stripped from the strings in the returned list.
>   * The list will be allocated using ctx as the talloc context.
> diff --git a/lib/database.cc b/lib/database.cc
> index 45c4260..29a56db 100644
> --- a/lib/database.cc
> +++ b/lib/database.cc
> @@ -20,6 +20,7 @@
>  
>  #include "database-private.h"
>  #include "parse-time-vrp.h"
> +#include "string-util.h"
>  
>  #include <iostream>
>  
> @@ -42,7 +43,7 @@ typedef struct {
>      const char *prefix;
>  } prefix_t;
>  
> -#define NOTMUCH_DATABASE_VERSION 2
> +#define NOTMUCH_DATABASE_VERSION 3
>  
>  #define STRINGIFY(s) _SUB_STRINGIFY(s)
>  #define _SUB_STRINGIFY(s) #s
> @@ -151,6 +152,17 @@ typedef struct {
>   *			changes are made to the database (such as by
>   *			indexing new fields).
>   *
> + *	features	The set of features supported by this
> + *			database. This consists of a set of
> + *			'\n'-separated lines, where each is a feature
> + *			name, a '\t', and compatibility flags.  If the
> + *			compatibility flags contain 'w', then the
> + *			opener must support this feature to safely
> + *			write this database.  If the compatibility
> + *			flags contain 'r', then the opener must
> + *			support this feature to read this database.
> + *			Introduced in database version 3.
> + *
>   *	last_thread_id	The last thread ID generated. This is stored
>   *			as a 16-byte hexadecimal ASCII representation
>   *			of a 64-bit unsigned integer. The first ID
> @@ -251,6 +263,25 @@ _find_prefix (const char *name)
>      return "";
>  }
>  
> +static const struct
> +{

Brace should be at the end of the previous line.

> +    /* NOTMUCH_FEATURE_* value. */
> +    unsigned int value;
> +    /* Feature name as it appears in the database.  This name should
> +     * be appropriate for displaying to the user if an older version
> +     * of notmuch doesn't support this feature. */
> +    const char *name;
> +    /* Compatibility flags when this feature is declared. */
> +    const char *flags;
> +} feature_names[] = {
> +    {NOTMUCH_FEATURE_FILE_TERMS,             "multiple paths per message", "rw"},
> +    {NOTMUCH_FEATURE_DIRECTORY_DOCS,         "relative directory paths", "rw"},
> +    /* Header values are not required for reading a database because a
> +     * reader can just refer to the message file. */
> +    {NOTMUCH_FEATURE_FROM_SUBJECT_ID_VALUES, "from/subject/message-ID in database", "w"},
> +    {NOTMUCH_FEATURE_BOOL_FOLDER,            "exact folder:/path: search", "rw"},

Spaces missing after the opening braces and before the closing braces.

> +};
> +
>  const char *
>  notmuch_status_to_string (notmuch_status_t status)
>  {
> @@ -591,6 +622,11 @@ notmuch_database_create (const char *path, notmuch_database_t **database)
>  				    &notmuch);
>      if (status)
>  	goto DONE;
> +
> +    /* Upgrade doesn't add this feature to existing databases, but new
> +     * databases have it. */
> +    notmuch->features |= NOTMUCH_FEATURE_FROM_SUBJECT_ID_VALUES;
> +
>      status = notmuch_database_upgrade (notmuch, NULL, NULL);
>      if (status) {
>  	notmuch_database_close(notmuch);
> @@ -619,6 +655,80 @@ _notmuch_database_ensure_writable (notmuch_database_t *notmuch)
>      return NOTMUCH_STATUS_SUCCESS;
>  }
>  
> +/* Parse a database features string from the given database version.
> + *
> + * For version < 3, this ignores the features string and returns a
> + * hard-coded set of features.
> + *
> + * If there are unrecognized features that are required to open the
> + * database in mode (which should be 'r' or 'w'), return a
> + * comma-separated list of unrecognized but required features in
> + * *incompat_out, which will be allocated from ctx.

Please describe the actual return value.

> + */
> +static unsigned int
> +_parse_features (const void *ctx, const char *features, unsigned int version,
> +		 char mode, char **incompat_out)
> +{
> +    unsigned int res = 0, namelen, i;
> +    size_t llen = 0;
> +    const char *flags;
> +
> +    /* Prior to database version 3, features were implied by the
> +     * version number. */
> +    if (version == 0)
> +	return NOTMUCH_FEATURES_V0;
> +    else if (version == 1)
> +	return NOTMUCH_FEATURES_V1;
> +    else if (version == 2)
> +	return NOTMUCH_FEATURES_V2;
> +
> +    /* Parse the features string */
> +    while ((features = strtok_len_c (features + llen, "\n", &llen)) != NULL) {
> +	flags = strchr (features, '\t');
> +	if (! flags || flags > features + llen)
> +	    continue;
> +	namelen = flags - features;
> +
> +	for (i = 0; i < ARRAY_SIZE (feature_names); ++i) {
> +	    if (strlen (feature_names[i].name) == namelen &&
> +		strncmp (feature_names[i].name, features, namelen) == 0) {
> +		res |= feature_names[i].value;
> +		break;
> +	    }
> +	}
> +
> +	if (i == ARRAY_SIZE (feature_names)) {
> +	    /* Unrecognized feature */
> +	    const char *have = strchr (flags, mode);
> +	    if (have && have < features + llen) {
> +		/* This feature is required to access this database in
> +		 * 'mode', but we don't understand it. */
> +		if (! *incompat_out)
> +		    *incompat_out = talloc_strdup (ctx, "");
> +		*incompat_out = talloc_asprintf_append_buffer (
> +		    *incompat_out, "%s%.*s", **incompat_out ? ", " : "",
> +		    namelen, features);

Do we intend the lib user to be able to parse the features? Perhaps we
should use '\n' as separator here too? (Although looks like *currently*
this is only for printing the message from within the lib.)

> +	    }
> +	}
> +    }
> +
> +    return res;
> +}
> +
> +static char *
> +_print_features (const void *ctx, unsigned int features)
> +{
> +    unsigned int i;
> +    char *res = talloc_strdup (ctx, "");
> +
> +    for (i = 0; i < ARRAY_SIZE (feature_names); ++i)
> +	if (features & feature_names[i].value)
> +	    res = talloc_asprintf_append_buffer (
> +		res, "%s\t%s\n", feature_names[i].name, feature_names[i].flags);
> +
> +    return res;
> +}
> +
>  notmuch_status_t
>  notmuch_database_open (const char *path,
>  		       notmuch_database_mode_t mode,
> @@ -627,7 +737,7 @@ notmuch_database_open (const char *path,
>      notmuch_status_t status = NOTMUCH_STATUS_SUCCESS;
>      void *local = talloc_new (NULL);
>      notmuch_database_t *notmuch = NULL;
> -    char *notmuch_path, *xapian_path;
> +    char *notmuch_path, *xapian_path, *incompat_features;
>      struct stat st;
>      int err;
>      unsigned int i, version;
> @@ -677,7 +787,6 @@ notmuch_database_open (const char *path,
>      if (notmuch->path[strlen (notmuch->path) - 1] == '/')
>  	notmuch->path[strlen (notmuch->path) - 1] = '\0';
>  
> -    notmuch->needs_upgrade = FALSE;
>      notmuch->mode = mode;
>      notmuch->atomic_nesting = 0;
>      try {
> @@ -686,37 +795,44 @@ notmuch_database_open (const char *path,
>  	if (mode == NOTMUCH_DATABASE_MODE_READ_WRITE) {
>  	    notmuch->xapian_db = new Xapian::WritableDatabase (xapian_path,
>  							       Xapian::DB_CREATE_OR_OPEN);
> -	    version = notmuch_database_get_version (notmuch);
> -
> -	    if (version > NOTMUCH_DATABASE_VERSION) {
> -		fprintf (stderr,
> -			 "Error: Notmuch database at %s\n"
> -			 "       has a newer database format version (%u) than supported by this\n"
> -			 "       version of notmuch (%u). Refusing to open this database in\n"
> -			 "       read-write mode.\n",
> -			 notmuch_path, version, NOTMUCH_DATABASE_VERSION);
> -		notmuch->mode = NOTMUCH_DATABASE_MODE_READ_ONLY;
> -		notmuch_database_destroy (notmuch);
> -		notmuch = NULL;
> -		status = NOTMUCH_STATUS_FILE_ERROR;
> -		goto DONE;
> -	    }
> -
> -	    if (version < NOTMUCH_DATABASE_VERSION)
> -		notmuch->needs_upgrade = TRUE;
>  	} else {
>  	    notmuch->xapian_db = new Xapian::Database (xapian_path);
> -	    version = notmuch_database_get_version (notmuch);
> -	    if (version > NOTMUCH_DATABASE_VERSION)
> -	    {
> -		fprintf (stderr,
> -			 "Warning: Notmuch database at %s\n"
> -			 "         has a newer database format version (%u) than supported by this\n"
> -			 "         version of notmuch (%u). Some operations may behave incorrectly,\n"
> -			 "         (but the database will not be harmed since it is being opened\n"
> -			 "         in read-only mode).\n",
> -			 notmuch_path, version, NOTMUCH_DATABASE_VERSION);
> -	    }
> +	}
> +
> +	/* Check version.  As of database version 3, we represent
> +	 * changes in terms of features, so assume a version bump
> +	 * means a dramatically incompatible change. */
> +	version = notmuch_database_get_version (notmuch);
> +	if (version > NOTMUCH_DATABASE_VERSION) {
> +	    fprintf (stderr,
> +		     "Error: Notmuch database at %s\n"
> +		     "       has a newer database format version (%u) than supported by this\n"
> +		     "       version of notmuch (%u).\n",
> +		     notmuch_path, version, NOTMUCH_DATABASE_VERSION);
> +	    notmuch->mode = NOTMUCH_DATABASE_MODE_READ_ONLY;
> +	    notmuch_database_destroy (notmuch);
> +	    notmuch = NULL;
> +	    status = NOTMUCH_STATUS_FILE_ERROR;
> +	    goto DONE;
> +	}
> +
> +	/* Check features. */
> +	incompat_features = NULL;
> +	notmuch->features = _parse_features (
> +	    local, notmuch->xapian_db->get_metadata ("features").c_str (),
> +	    version, mode == NOTMUCH_DATABASE_MODE_READ_WRITE ? 'w' : 'r',

Makes me think _parse_features could use notmuch_database_mode_t mode
instead of char mode. *shrug*.

> +	    &incompat_features);
> +	if (incompat_features) {
> +	    fprintf (stderr,
> +		     "Error: Notmuch database at %s\n"
> +		     "       requires features (%s)\n"
> +		     "       not supported by this version of notmuch.\n",
> +		     notmuch_path, incompat_features);
> +	    notmuch->mode = NOTMUCH_DATABASE_MODE_READ_ONLY;
> +	    notmuch_database_destroy (notmuch);
> +	    notmuch = NULL;
> +	    status = NOTMUCH_STATUS_FILE_ERROR;
> +	    goto DONE;
>  	}
>  
>  	notmuch->last_doc_id = notmuch->xapian_db->get_lastdocid ();
> @@ -1048,7 +1164,8 @@ notmuch_database_get_version (notmuch_database_t *notmuch)
>  notmuch_bool_t
>  notmuch_database_needs_upgrade (notmuch_database_t *notmuch)
>  {
> -    return notmuch->needs_upgrade;
> +    return notmuch->mode == NOTMUCH_DATABASE_MODE_READ_WRITE &&
> +	(NOTMUCH_FEATURES_CURRENT & ~notmuch->features);
>  }

Am I correct that this does not lead to a database upgrade from v2 to v3
until we actually change the features?

Aside, why don't we return a suitable status code from
notmuch_database_open when an upgrade is required?

>  static volatile sig_atomic_t do_progress_notify = 0;
> @@ -1077,6 +1194,7 @@ notmuch_database_upgrade (notmuch_database_t *notmuch,
>  						   double progress),
>  			  void *closure)
>  {
> +    void *local = talloc_new (NULL);
>      Xapian::WritableDatabase *db;
>      struct sigaction action;
>      struct itimerval timerval;
> @@ -1114,6 +1232,10 @@ notmuch_database_upgrade (notmuch_database_t *notmuch,
>  	timer_is_active = TRUE;
>      }
>  
> +    /* Set the target features so we write out changes in the desired
> +     * format. */
> +    notmuch->features |= NOTMUCH_FEATURES_CURRENT;
> +
>      /* Before version 1, each message document had its filename in the
>       * data field. Copy that into the new format by calling
>       * notmuch_message_add_filename.
> @@ -1226,6 +1348,7 @@ notmuch_database_upgrade (notmuch_database_t *notmuch,
>  	notmuch_query_destroy (query);
>      }
>  
> +    db->set_metadata ("features", _print_features (local, notmuch->features));
>      db->set_metadata ("version", STRINGIFY (NOTMUCH_DATABASE_VERSION));
>      db->flush ();
>  
> @@ -1302,6 +1425,7 @@ notmuch_database_upgrade (notmuch_database_t *notmuch,
>  	sigaction (SIGALRM, &action, NULL);
>      }
>  
> +    talloc_free (local);
>      return NOTMUCH_STATUS_SUCCESS;
>  }
>  
> -- 
> 2.0.0
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH v3 05/13] test: Tool to build DB with specific version and features

[Jani Nikula]

On Fri, 01 Aug 2014, Austin Clements <amdragon@MIT.EDU> wrote:
> This will let us test basic version and feature handling.

LGTM.

> ---
>  test/.gitignore         |  1 +
>  test/Makefile.local     |  4 ++++
>  test/make-db-version.cc | 35 +++++++++++++++++++++++++++++++++++
>  3 files changed, 40 insertions(+)
>  create mode 100644 test/make-db-version.cc
>
> diff --git a/test/.gitignore b/test/.gitignore
> index b3b706d..0f7d5bf 100644
> --- a/test/.gitignore
> +++ b/test/.gitignore
> @@ -5,5 +5,6 @@ parse-time
>  random-corpus
>  smtp-dummy
>  symbol-test
> +make-db-version
>  test-results
>  tmp.*
> diff --git a/test/Makefile.local b/test/Makefile.local
> index 916dd0b..a2d58fc 100644
> --- a/test/Makefile.local
> +++ b/test/Makefile.local
> @@ -35,6 +35,9 @@ $(dir)/symbol-test: $(dir)/symbol-test.o lib/$(LINKER_NAME)
>  $(dir)/parse-time: $(dir)/parse-time.o parse-time-string/parse-time-string.o
>  	$(call quiet,CC) $^ -o $@
>  
> +$(dir)/make-db-version: $(dir)/make-db-version.o
> +	$(call quiet,CXX) $^ -o $@ $(XAPIAN_LDFLAGS)
> +
>  .PHONY: test check
>  
>  test_main_srcs=$(dir)/arg-test.c \
> @@ -43,6 +46,7 @@ test_main_srcs=$(dir)/arg-test.c \
>  	      $(dir)/parse-time.c \
>  	      $(dir)/smtp-dummy.c \
>  	      $(dir)/symbol-test.cc \
> +	      $(dir)/make-db-version.cc \
>  
>  test_srcs=$(test_main_srcs) $(dir)/database-test.c
>  
> diff --git a/test/make-db-version.cc b/test/make-db-version.cc
> new file mode 100644
> index 0000000..fa80cac
> --- /dev/null
> +++ b/test/make-db-version.cc
> @@ -0,0 +1,35 @@
> +/* Create an empty notmuch database with a specific version and
> + * features. */
> +
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <sys/stat.h>
> +#include <sys/types.h>
> +
> +#include <xapian.h>
> +
> +int main(int argc, char **argv)
> +{
> +    if (argc != 4) {
> +	fprintf (stderr, "Usage: %s mailpath version features\n", argv[0]);
> +	exit (2);
> +    }
> +
> +    std::string nmpath (argv[1]);
> +    nmpath += "/.notmuch";
> +    if (mkdir (nmpath.c_str (), 0777) < 0) {
> +	perror (("failed to create " + nmpath).c_str ());
> +	exit (1);
> +    }
> +
> +    try {
> +	Xapian::WritableDatabase db (
> +	    nmpath + "/xapian", Xapian::DB_CREATE_OR_OPEN);
> +	db.set_metadata ("version", argv[2]);
> +	db.set_metadata ("features", argv[3]);
> +	db.commit ();
> +    } catch (const Xapian::Error &e) {
> +	fprintf (stderr, "%s\n", e.get_description ().c_str ());
> +	exit (1);
> +    }
> +}
> -- 
> 2.0.0
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH v3 04/13] lib: Database version 3: Introduce fine-grained "features"

[David Bremner]

Austin Clements <amdragon@MIT.EDU> writes:
>  
> +    /* Bit mask of features used by this database.  Features are
> +     * named, independent aspects of the database schema.  This is a
> +     * bitwise-OR of NOTMUCH_FEATURE_* values (below). */
> +    unsigned int features;

Should we be using a fixed size integer (uint_32t or whatever) for
features? iirc the metadata in the database is actually a string, so I
guess arbitrary precision there.

> +/* Bit masks for _notmuch_database::features. */
> +enum {
> +    /* If set, file names are stored in "file-direntry" terms.  If
> +     * unset, file names are stored in document data.
> +     *
> +     * Introduced: version 1.  Implementation support: both for read;
> +     * required for write. */
> +    NOTMUCH_FEATURE_FILE_TERMS = 1 << 0,

I agree with Jani that the Implementation support: part is a bit
mystifying without the commit message. Maybe part of the commit message
could migrate here? Or maybe just add a pointer to the comment in database.cc.

> +		if (! *incompat_out)

Should we support passing NULL for incompat_out? or at least check for
it?

> @@ -1048,7 +1164,8 @@ notmuch_database_get_version (notmuch_database_t *notmuch)
>  notmuch_bool_t
>  notmuch_database_needs_upgrade (notmuch_database_t *notmuch)
>  {
> -    return notmuch->needs_upgrade;
> +    return notmuch->mode == NOTMUCH_DATABASE_MODE_READ_WRITE &&
> +	(NOTMUCH_FEATURES_CURRENT & ~notmuch->features);
>  }

Maybe I'm not thinking hard enough here, but how does this deal with a
feature that is needed to open a database in read only mode? Maybe it
needs a comment for people not as clever as Austin ;).

d

Re: [PATCH v3 03/13] new: Don't report version after upgrade

[Austin Clements]

Quoth Jani Nikula on Aug 23 at  6:39 pm:
> On Fri, 01 Aug 2014, Austin Clements <amdragon@MIT.EDU> wrote:
> > The version number has always been pretty meaningless to the user and
> > it's about to become even more meaningless with the introduction of
> > "features".  Hopefully, the database will remain on version 3 for some
> > time to come; however, the introduction of new features over time in
> > version 3 will necessitate upgrades within version 3.  It would be
> > confusing if we always tell the user they've been "upgraded to version
> > 3".  If the user wants to know what's new, they should read the news.
> 
> I think this is good for now.
> 
> What do you think about adding notmuch_database_get_features(), and
> printing that?

Mark had a similar comment, so here's my reply:
id:20140727162426.GF13893@mit.edu

I'm happy with adding more transparency around this, though I'd prefer
to do it as follow-up to avoid expanding this series and because I'm
pretty sure adding something like notmuch_database_get_features
wouldn't require any non-trivial changes to the stuff in this series.

> BR,
> Jani.

Re: [PATCH v3 04/13] lib: Database version 3: Introduce fine-grained "features"

[Austin Clements]

Quoth Jani Nikula on Aug 23 at  7:02 pm:
> On Fri, 01 Aug 2014, Austin Clements <amdragon@MIT.EDU> wrote:
> > Previously, our database schema was versioned by a single number.
> > Each database schema change had to occur "atomically" in Notmuch's
> > development history: before some commit, Notmuch used version N, after
> > that commit, it used version N+1.  Hence, each new schema version
> > could introduce only one change, the task of developing a schema
> > change fell on a single person, and it all had to happen and be
> > perfect in a single commit series.  This made introducing a new schema
> > version hard.  We've seen only two schema changes in the history of
> > Notmuch.
> >
> > This commit introduces database schema version 3; hopefully the last
> > schema version we'll need for a while.  With this version, we switch
> > from a single version number to "features": a set of named,
> > independent aspects of the database schema.
> >
> > Features should make backwards compatibility easier.  For many things,
> > it should be easy to support databases both with and without a
> > feature, which will allow us to make upgrades optional and will enable
> > "unstable" features that can be developed and tested over time.
> >
> > Features also make forwards compatibility easier.  The features
> > recorded in a database include "compatibility flags," which can
> > indicate to an older version of Notmuch when it must support a given
> > feature to open the database for read or for write.  This lets us
> > replace the old vague "I don't recognize this version, so something
> > might go wrong, but I promise to try my best" warnings upon opening a
> > database with an unknown version with precise errors.  If a database
> > is safe to open for read/write despite unknown features, an older
> > version will know that and issue no message at all.  If the database
> > is not safe to open for read/write because of unknown features, an
> > older version will know that, too, and can tell the user exactly which
> > required features it lacks support for.
> 
> I agree with the change overall; it might be useful to have another set
> of eyes on the patch though.
> 
> > ---
> >  lib/database-private.h |  57 ++++++++++++++-
> >  lib/database.cc        | 190 ++++++++++++++++++++++++++++++++++++++++---------
> >  2 files changed, 213 insertions(+), 34 deletions(-)
> >
> > diff --git a/lib/database-private.h b/lib/database-private.h
> > index d3e65fd..2ffab33 100644
> > --- a/lib/database-private.h
> > +++ b/lib/database-private.h
> > @@ -41,11 +41,15 @@ struct _notmuch_database {
> >  
> >      char *path;
> >  
> > -    notmuch_bool_t needs_upgrade;
> >      notmuch_database_mode_t mode;
> >      int atomic_nesting;
> >      Xapian::Database *xapian_db;
> >  
> > +    /* Bit mask of features used by this database.  Features are
> > +     * named, independent aspects of the database schema.  This is a
> > +     * bitwise-OR of NOTMUCH_FEATURE_* values (below). */
> > +    unsigned int features;
> > +
> >      unsigned int last_doc_id;
> >      uint64_t last_thread_id;
> >  
> > @@ -55,6 +59,57 @@ struct _notmuch_database {
> >      Xapian::ValueRangeProcessor *date_range_processor;
> >  };
> >  
> > +/* Bit masks for _notmuch_database::features. */
> > +enum {
> > +    /* If set, file names are stored in "file-direntry" terms.  If
> > +     * unset, file names are stored in document data.
> > +     *
> > +     * Introduced: version 1.  Implementation support: both for read;
> > +     * required for write. */
> 
> Maybe I'm dense, but the implementation support comments could be
> clearer.

No, this was too terse.  The intent was to state what the current
library implementation supports: e.g., the current implementation
supports reading from databases both with and without
NOTMUCH_FEATURE_FILE_TERMS, but if you attempt to write a database
without this feature, some operations may return
NOTMUCH_STATUS_UPGRADE_REQUIRED (introduced in a later patch).

I wonder, though, if the "implementation support" bits should just be
omitted.  These comments are inevitably going to get out of sync with
the real implementation, and in this case an incorrect comment may be
worse than no comment.  I could put a comment over the whole enum
giving a more general description:

/* Bit masks for _notmuch_database::features.  Features are named,
 * independent aspects of the database schema.
 *
 * A database stores the set of features that it "uses" (implicitly
 * before database version 3 and explicitly as of version 3).
 *
 * A given library version will "recognize" a particular set of
 * features; if a database uses a feature that the library does not
 * recognize, the library will refuse to open it.  It is assumed the
 * set of recognized features grows monotonically over time.  A
 * library version will "implement" some subset of the recognized
 * features: some operations may require that the database use (or not
 * use) some feature, while other operations may support both
 * databases that use and that don't use some feature.
 *
 * On disk, the database stores string names for these features (see
 * the feature_names array).  These enum bit values are never
 * persisted to disk and may change freely.
 */

> > +    NOTMUCH_FEATURE_FILE_TERMS = 1 << 0,
> > +
> > +    /* If set, directory timestamps are stored in documents with
> > +     * XDIRECTORY terms and relative paths.  If unset, directory
> > +     * timestamps are stored in documents with XTIMESTAMP terms and
> > +     * absolute paths.
> > +     *
> > +     * Introduced: version 1.  Implementation support: required. */
> > +    NOTMUCH_FEATURE_DIRECTORY_DOCS = 1 << 1,
> > +
> > +    /* If set, the from, subject, and message-id headers are stored in
> > +     * message document values.  If unset, message documents *may*
> > +     * have these values, but if the value is empty, it must be
> > +     * retrieved from the message file.
> > +     *
> > +     * Introduced: optional in version 1, required as of version 3.
> > +     * Implementation support: both.
> > +     */
> > +    NOTMUCH_FEATURE_FROM_SUBJECT_ID_VALUES = 1 << 2,
> > +
> > +    /* If set, folder terms are boolean and path terms exist.  If
> > +     * unset, folder terms are probabilistic and stemmed and path
> > +     * terms do not exist.
> > +     *
> > +     * Introduced: version 2.  Implementation support: required. */
> > +    NOTMUCH_FEATURE_BOOL_FOLDER = 1 << 3,
> > +};
> > +
> > +/* Prior to database version 3, features were implied by the database
> > + * version number, so hard-code them for earlier versions. */
> > +#define NOTMUCH_FEATURES_V0 (0)
> > +#define NOTMUCH_FEATURES_V1 (NOTMUCH_FEATURES_V0 | NOTMUCH_FEATURE_FILE_TERMS | \
> > +			     NOTMUCH_FEATURE_DIRECTORY_DOCS)
> > +#define NOTMUCH_FEATURES_V2 (NOTMUCH_FEATURES_V1 | NOTMUCH_FEATURE_BOOL_FOLDER)
> > +
> > +/* Current database features.  If any of these are missing from a
> > + * database, request an upgrade.
> > + * NOTMUCH_FEATURE_FROM_SUBJECT_ID_VALUES is not included because
> > + * upgrade doesn't currently introduce the feature (though brand new
> > + * databases will have it). */
> > +#define NOTMUCH_FEATURES_CURRENT \
> > +    (NOTMUCH_FEATURE_FILE_TERMS | NOTMUCH_FEATURE_DIRECTORY_DOCS | \
> > +     NOTMUCH_FEATURE_BOOL_FOLDER)
> > +
> >  /* Return the list of terms from the given iterator matching a prefix.
> >   * The prefix will be stripped from the strings in the returned list.
> >   * The list will be allocated using ctx as the talloc context.
> > diff --git a/lib/database.cc b/lib/database.cc
> > index 45c4260..29a56db 100644
> > --- a/lib/database.cc
> > +++ b/lib/database.cc
> > @@ -20,6 +20,7 @@
> >  
> >  #include "database-private.h"
> >  #include "parse-time-vrp.h"
> > +#include "string-util.h"
> >  
> >  #include <iostream>
> >  
> > @@ -42,7 +43,7 @@ typedef struct {
> >      const char *prefix;
> >  } prefix_t;
> >  
> > -#define NOTMUCH_DATABASE_VERSION 2
> > +#define NOTMUCH_DATABASE_VERSION 3
> >  
> >  #define STRINGIFY(s) _SUB_STRINGIFY(s)
> >  #define _SUB_STRINGIFY(s) #s
> > @@ -151,6 +152,17 @@ typedef struct {
> >   *			changes are made to the database (such as by
> >   *			indexing new fields).
> >   *
> > + *	features	The set of features supported by this
> > + *			database. This consists of a set of
> > + *			'\n'-separated lines, where each is a feature
> > + *			name, a '\t', and compatibility flags.  If the
> > + *			compatibility flags contain 'w', then the
> > + *			opener must support this feature to safely
> > + *			write this database.  If the compatibility
> > + *			flags contain 'r', then the opener must
> > + *			support this feature to read this database.
> > + *			Introduced in database version 3.
> > + *
> >   *	last_thread_id	The last thread ID generated. This is stored
> >   *			as a 16-byte hexadecimal ASCII representation
> >   *			of a 64-bit unsigned integer. The first ID
> > @@ -251,6 +263,25 @@ _find_prefix (const char *name)
> >      return "";
> >  }
> >  
> > +static const struct
> > +{
> 
> Brace should be at the end of the previous line.

Fixed.

> > +    /* NOTMUCH_FEATURE_* value. */
> > +    unsigned int value;
> > +    /* Feature name as it appears in the database.  This name should
> > +     * be appropriate for displaying to the user if an older version
> > +     * of notmuch doesn't support this feature. */
> > +    const char *name;
> > +    /* Compatibility flags when this feature is declared. */
> > +    const char *flags;
> > +} feature_names[] = {
> > +    {NOTMUCH_FEATURE_FILE_TERMS,             "multiple paths per message", "rw"},
> > +    {NOTMUCH_FEATURE_DIRECTORY_DOCS,         "relative directory paths", "rw"},
> > +    /* Header values are not required for reading a database because a
> > +     * reader can just refer to the message file. */
> > +    {NOTMUCH_FEATURE_FROM_SUBJECT_ID_VALUES, "from/subject/message-ID in database", "w"},
> > +    {NOTMUCH_FEATURE_BOOL_FOLDER,            "exact folder:/path: search", "rw"},
> 
> Spaces missing after the opening braces and before the closing braces.

Fixed.  I also wrapped all of the entries between the enum name and
the string name since one of the lines was already too long and this
pushed two more over.

> > +};
> > +
> >  const char *
> >  notmuch_status_to_string (notmuch_status_t status)
> >  {
> > @@ -591,6 +622,11 @@ notmuch_database_create (const char *path, notmuch_database_t **database)
> >  				    &notmuch);
> >      if (status)
> >  	goto DONE;
> > +
> > +    /* Upgrade doesn't add this feature to existing databases, but new
> > +     * databases have it. */
> > +    notmuch->features |= NOTMUCH_FEATURE_FROM_SUBJECT_ID_VALUES;
> > +
> >      status = notmuch_database_upgrade (notmuch, NULL, NULL);
> >      if (status) {
> >  	notmuch_database_close(notmuch);
> > @@ -619,6 +655,80 @@ _notmuch_database_ensure_writable (notmuch_database_t *notmuch)
> >      return NOTMUCH_STATUS_SUCCESS;
> >  }
> >  
> > +/* Parse a database features string from the given database version.
> > + *
> > + * For version < 3, this ignores the features string and returns a
> > + * hard-coded set of features.
> > + *
> > + * If there are unrecognized features that are required to open the
> > + * database in mode (which should be 'r' or 'w'), return a
> > + * comma-separated list of unrecognized but required features in
> > + * *incompat_out, which will be allocated from ctx.
> 
> Please describe the actual return value.

Fixed.  I also gave the enum type a name (in response to David's
review) and used it here, so this is more self-documenting.

> > + */
> > +static unsigned int
> > +_parse_features (const void *ctx, const char *features, unsigned int version,
> > +		 char mode, char **incompat_out)
> > +{
> > +    unsigned int res = 0, namelen, i;
> > +    size_t llen = 0;
> > +    const char *flags;
> > +
> > +    /* Prior to database version 3, features were implied by the
> > +     * version number. */
> > +    if (version == 0)
> > +	return NOTMUCH_FEATURES_V0;
> > +    else if (version == 1)
> > +	return NOTMUCH_FEATURES_V1;
> > +    else if (version == 2)
> > +	return NOTMUCH_FEATURES_V2;
> > +
> > +    /* Parse the features string */
> > +    while ((features = strtok_len_c (features + llen, "\n", &llen)) != NULL) {
> > +	flags = strchr (features, '\t');
> > +	if (! flags || flags > features + llen)
> > +	    continue;
> > +	namelen = flags - features;
> > +
> > +	for (i = 0; i < ARRAY_SIZE (feature_names); ++i) {
> > +	    if (strlen (feature_names[i].name) == namelen &&
> > +		strncmp (feature_names[i].name, features, namelen) == 0) {
> > +		res |= feature_names[i].value;
> > +		break;
> > +	    }
> > +	}
> > +
> > +	if (i == ARRAY_SIZE (feature_names)) {
> > +	    /* Unrecognized feature */
> > +	    const char *have = strchr (flags, mode);
> > +	    if (have && have < features + llen) {
> > +		/* This feature is required to access this database in
> > +		 * 'mode', but we don't understand it. */
> > +		if (! *incompat_out)
> > +		    *incompat_out = talloc_strdup (ctx, "");
> > +		*incompat_out = talloc_asprintf_append_buffer (
> > +		    *incompat_out, "%s%.*s", **incompat_out ? ", " : "",
> > +		    namelen, features);
> 
> Do we intend the lib user to be able to parse the features? Perhaps we
> should use '\n' as separator here too? (Although looks like *currently*
> this is only for printing the message from within the lib.)

I'd intended this only for user presentation.  I updated the comment
to mention this.  If it turns out we need this information in a
computer-friendly form in the future, it should be easy to add.

> > +	    }
> > +	}
> > +    }
> > +
> > +    return res;
> > +}
> > +
> > +static char *
> > +_print_features (const void *ctx, unsigned int features)
> > +{
> > +    unsigned int i;
> > +    char *res = talloc_strdup (ctx, "");
> > +
> > +    for (i = 0; i < ARRAY_SIZE (feature_names); ++i)
> > +	if (features & feature_names[i].value)
> > +	    res = talloc_asprintf_append_buffer (
> > +		res, "%s\t%s\n", feature_names[i].name, feature_names[i].flags);
> > +
> > +    return res;
> > +}
> > +
> >  notmuch_status_t
> >  notmuch_database_open (const char *path,
> >  		       notmuch_database_mode_t mode,
> > @@ -627,7 +737,7 @@ notmuch_database_open (const char *path,
> >      notmuch_status_t status = NOTMUCH_STATUS_SUCCESS;
> >      void *local = talloc_new (NULL);
> >      notmuch_database_t *notmuch = NULL;
> > -    char *notmuch_path, *xapian_path;
> > +    char *notmuch_path, *xapian_path, *incompat_features;
> >      struct stat st;
> >      int err;
> >      unsigned int i, version;
> > @@ -677,7 +787,6 @@ notmuch_database_open (const char *path,
> >      if (notmuch->path[strlen (notmuch->path) - 1] == '/')
> >  	notmuch->path[strlen (notmuch->path) - 1] = '\0';
> >  
> > -    notmuch->needs_upgrade = FALSE;
> >      notmuch->mode = mode;
> >      notmuch->atomic_nesting = 0;
> >      try {
> > @@ -686,37 +795,44 @@ notmuch_database_open (const char *path,
> >  	if (mode == NOTMUCH_DATABASE_MODE_READ_WRITE) {
> >  	    notmuch->xapian_db = new Xapian::WritableDatabase (xapian_path,
> >  							       Xapian::DB_CREATE_OR_OPEN);
> > -	    version = notmuch_database_get_version (notmuch);
> > -
> > -	    if (version > NOTMUCH_DATABASE_VERSION) {
> > -		fprintf (stderr,
> > -			 "Error: Notmuch database at %s\n"
> > -			 "       has a newer database format version (%u) than supported by this\n"
> > -			 "       version of notmuch (%u). Refusing to open this database in\n"
> > -			 "       read-write mode.\n",
> > -			 notmuch_path, version, NOTMUCH_DATABASE_VERSION);
> > -		notmuch->mode = NOTMUCH_DATABASE_MODE_READ_ONLY;
> > -		notmuch_database_destroy (notmuch);
> > -		notmuch = NULL;
> > -		status = NOTMUCH_STATUS_FILE_ERROR;
> > -		goto DONE;
> > -	    }
> > -
> > -	    if (version < NOTMUCH_DATABASE_VERSION)
> > -		notmuch->needs_upgrade = TRUE;
> >  	} else {
> >  	    notmuch->xapian_db = new Xapian::Database (xapian_path);
> > -	    version = notmuch_database_get_version (notmuch);
> > -	    if (version > NOTMUCH_DATABASE_VERSION)
> > -	    {
> > -		fprintf (stderr,
> > -			 "Warning: Notmuch database at %s\n"
> > -			 "         has a newer database format version (%u) than supported by this\n"
> > -			 "         version of notmuch (%u). Some operations may behave incorrectly,\n"
> > -			 "         (but the database will not be harmed since it is being opened\n"
> > -			 "         in read-only mode).\n",
> > -			 notmuch_path, version, NOTMUCH_DATABASE_VERSION);
> > -	    }
> > +	}
> > +
> > +	/* Check version.  As of database version 3, we represent
> > +	 * changes in terms of features, so assume a version bump
> > +	 * means a dramatically incompatible change. */
> > +	version = notmuch_database_get_version (notmuch);
> > +	if (version > NOTMUCH_DATABASE_VERSION) {
> > +	    fprintf (stderr,
> > +		     "Error: Notmuch database at %s\n"
> > +		     "       has a newer database format version (%u) than supported by this\n"
> > +		     "       version of notmuch (%u).\n",
> > +		     notmuch_path, version, NOTMUCH_DATABASE_VERSION);
> > +	    notmuch->mode = NOTMUCH_DATABASE_MODE_READ_ONLY;
> > +	    notmuch_database_destroy (notmuch);
> > +	    notmuch = NULL;
> > +	    status = NOTMUCH_STATUS_FILE_ERROR;
> > +	    goto DONE;
> > +	}
> > +
> > +	/* Check features. */
> > +	incompat_features = NULL;
> > +	notmuch->features = _parse_features (
> > +	    local, notmuch->xapian_db->get_metadata ("features").c_str (),
> > +	    version, mode == NOTMUCH_DATABASE_MODE_READ_WRITE ? 'w' : 'r',
> 
> Makes me think _parse_features could use notmuch_database_mode_t mode
> instead of char mode. *shrug*.

In principle there could be other compatibility modes, though I can't
imagine what they would be.

> > +	    &incompat_features);
> > +	if (incompat_features) {
> > +	    fprintf (stderr,
> > +		     "Error: Notmuch database at %s\n"
> > +		     "       requires features (%s)\n"
> > +		     "       not supported by this version of notmuch.\n",
> > +		     notmuch_path, incompat_features);
> > +	    notmuch->mode = NOTMUCH_DATABASE_MODE_READ_ONLY;
> > +	    notmuch_database_destroy (notmuch);
> > +	    notmuch = NULL;
> > +	    status = NOTMUCH_STATUS_FILE_ERROR;
> > +	    goto DONE;
> >  	}
> >  
> >  	notmuch->last_doc_id = notmuch->xapian_db->get_lastdocid ();
> > @@ -1048,7 +1164,8 @@ notmuch_database_get_version (notmuch_database_t *notmuch)
> >  notmuch_bool_t
> >  notmuch_database_needs_upgrade (notmuch_database_t *notmuch)
> >  {
> > -    return notmuch->needs_upgrade;
> > +    return notmuch->mode == NOTMUCH_DATABASE_MODE_READ_WRITE &&
> > +	(NOTMUCH_FEATURES_CURRENT & ~notmuch->features);
> >  }
> 
> Am I correct that this does not lead to a database upgrade from v2 to v3
> until we actually change the features?

Yep, that's true (though new databases will start with v3).
Conveniently, I have a bunch of patches that introduce some new
features pending right behind this series.  :)

> Aside, why don't we return a suitable status code from
> notmuch_database_open when an upgrade is required?

That may make sense at this point in the series, but later patches
make upgrades no longer "required" and instead enforce database
feature requirements on a per-operation basis.  At that point, this
function becomes effectively optional; more "wants_upgrade" than
"needs_upgrade".

> >  static volatile sig_atomic_t do_progress_notify = 0;
> > @@ -1077,6 +1194,7 @@ notmuch_database_upgrade (notmuch_database_t *notmuch,
> >  						   double progress),
> >  			  void *closure)
> >  {
> > +    void *local = talloc_new (NULL);
> >      Xapian::WritableDatabase *db;
> >      struct sigaction action;
> >      struct itimerval timerval;
> > @@ -1114,6 +1232,10 @@ notmuch_database_upgrade (notmuch_database_t *notmuch,
> >  	timer_is_active = TRUE;
> >      }
> >  
> > +    /* Set the target features so we write out changes in the desired
> > +     * format. */
> > +    notmuch->features |= NOTMUCH_FEATURES_CURRENT;
> > +
> >      /* Before version 1, each message document had its filename in the
> >       * data field. Copy that into the new format by calling
> >       * notmuch_message_add_filename.
> > @@ -1226,6 +1348,7 @@ notmuch_database_upgrade (notmuch_database_t *notmuch,
> >  	notmuch_query_destroy (query);
> >      }
> >  
> > +    db->set_metadata ("features", _print_features (local, notmuch->features));
> >      db->set_metadata ("version", STRINGIFY (NOTMUCH_DATABASE_VERSION));
> >      db->flush ();
> >  
> > @@ -1302,6 +1425,7 @@ notmuch_database_upgrade (notmuch_database_t *notmuch,
> >  	sigaction (SIGALRM, &action, NULL);
> >      }
> >  
> > +    talloc_free (local);
> >      return NOTMUCH_STATUS_SUCCESS;
> >  }
> >  

Re: [PATCH v3 04/13] lib: Database version 3: Introduce fine-grained "features"

[Austin Clements]

On Sat, 23 Aug 2014, David Bremner <david@tethera.net> wrote:
> Austin Clements <amdragon@MIT.EDU> writes:
>>  
>> +    /* Bit mask of features used by this database.  Features are
>> +     * named, independent aspects of the database schema.  This is a
>> +     * bitwise-OR of NOTMUCH_FEATURE_* values (below). */
>> +    unsigned int features;
>
> Should we be using a fixed size integer (uint_32t or whatever) for
> features? iirc the metadata in the database is actually a string, so I
> guess arbitrary precision there.

Right; this doesn't matter for the on-disk format because these don't
appear on disk.  But you're right that in principle we could overflow
this, leading to subtle bugs.  I moved the enum above struct
_notmuch_database, gave it a name and bitwise operators for C++, and
used that enum name everywhere, so precision should never be a problem.

>> +/* Bit masks for _notmuch_database::features. */
>> +enum {
>> +    /* If set, file names are stored in "file-direntry" terms.  If
>> +     * unset, file names are stored in document data.
>> +     *
>> +     * Introduced: version 1.  Implementation support: both for read;
>> +     * required for write. */
>> +    NOTMUCH_FEATURE_FILE_TERMS = 1 << 0,
>
> I agree with Jani that the Implementation support: part is a bit
> mystifying without the commit message. Maybe part of the commit message
> could migrate here? Or maybe just add a pointer to the comment in database.cc.

I stripped these out because I don't think they're maintainable.  See my
reply to Jani.

>> +		if (! *incompat_out)
>
> Should we support passing NULL for incompat_out? or at least check for
> it?

Added a guard so it's safe to pass NULL.

>> @@ -1048,7 +1164,8 @@ notmuch_database_get_version (notmuch_database_t *notmuch)
>>  notmuch_bool_t
>>  notmuch_database_needs_upgrade (notmuch_database_t *notmuch)
>>  {
>> -    return notmuch->needs_upgrade;
>> +    return notmuch->mode == NOTMUCH_DATABASE_MODE_READ_WRITE &&
>> +	(NOTMUCH_FEATURES_CURRENT & ~notmuch->features);
>>  }
>
> Maybe I'm not thinking hard enough here, but how does this deal with a
> feature that is needed to open a database in read only mode? Maybe it
> needs a comment for people not as clever as Austin ;).

I'm not quite sure what you mean.  notmuch_database_needs_upgrade
returns false for read-only databases because you can't upgrade a
read-only database.  This was true before this patch, too, though it was
less obvious.  (Maybe that's not what you're asking?)

Re: [PATCH v3 04/13] lib: Database version 3: Introduce fine-grained "features"

[David Bremner]

Austin Clements <amdragon@mit.edu> writes:

>>> @@ -1048,7 +1164,8 @@ notmuch_database_get_version (notmuch_database_t *notmuch)
>>>  notmuch_bool_t
>>>  notmuch_database_needs_upgrade (notmuch_database_t *notmuch)
>>>  {
>>> -    return notmuch->needs_upgrade;
>>> +    return notmuch->mode == NOTMUCH_DATABASE_MODE_READ_WRITE &&
>>> +	(NOTMUCH_FEATURES_CURRENT & ~notmuch->features);
>>>  }
>>
>> Maybe I'm not thinking hard enough here, but how does this deal with a
>> feature that is needed to open a database in read only mode? Maybe it
>> needs a comment for people not as clever as Austin ;).
>
> I'm not quite sure what you mean.  notmuch_database_needs_upgrade
> returns false for read-only databases because you can't upgrade a
> read-only database.  This was true before this patch, too, though it was
> less obvious.  (Maybe that's not what you're asking?)

Yes, that's what I was asking. I guess it's orthogonal to your patch
series, but the logic of returning FALSE for read only databases is not
very intuitive to me (in the sense that "needs upgrade" is not the
opposite of "can't be upgraded".  Your new comment later in the series
is better, but it would IMHO be even better if you mentioned the read
only case.

d

Re: [PATCH v3 03/13] new: Don't report version after upgrade

[Jani Nikula]

On Sun, 24 Aug 2014, Austin Clements <amdragon@MIT.EDU> wrote:
> Quoth Jani Nikula on Aug 23 at  6:39 pm:
>> On Fri, 01 Aug 2014, Austin Clements <amdragon@MIT.EDU> wrote:
>> > The version number has always been pretty meaningless to the user and
>> > it's about to become even more meaningless with the introduction of
>> > "features".  Hopefully, the database will remain on version 3 for some
>> > time to come; however, the introduction of new features over time in
>> > version 3 will necessitate upgrades within version 3.  It would be
>> > confusing if we always tell the user they've been "upgraded to version
>> > 3".  If the user wants to know what's new, they should read the news.
>> 
>> I think this is good for now.
>> 
>> What do you think about adding notmuch_database_get_features(), and
>> printing that?
>
> Mark had a similar comment, so here's my reply:
> id:20140727162426.GF13893@mit.edu
>
> I'm happy with adding more transparency around this, though I'd prefer
> to do it as follow-up to avoid expanding this series and because I'm
> pretty sure adding something like notmuch_database_get_features
> wouldn't require any non-trivial changes to the stuff in this series.

Agreed.

BR,
Jani.

Re: [PATCH v3 04/13] lib: Database version 3: Introduce fine-grained "features"

[Austin Clements]

Quoth David Bremner on Aug 23 at  8:58 pm:
> Austin Clements <amdragon@mit.edu> writes:
> 
> >>> @@ -1048,7 +1164,8 @@ notmuch_database_get_version (notmuch_database_t *notmuch)
> >>>  notmuch_bool_t
> >>>  notmuch_database_needs_upgrade (notmuch_database_t *notmuch)
> >>>  {
> >>> -    return notmuch->needs_upgrade;
> >>> +    return notmuch->mode == NOTMUCH_DATABASE_MODE_READ_WRITE &&
> >>> +	(NOTMUCH_FEATURES_CURRENT & ~notmuch->features);
> >>>  }
> >>
> >> Maybe I'm not thinking hard enough here, but how does this deal with a
> >> feature that is needed to open a database in read only mode? Maybe it
> >> needs a comment for people not as clever as Austin ;).
> >
> > I'm not quite sure what you mean.  notmuch_database_needs_upgrade
> > returns false for read-only databases because you can't upgrade a
> > read-only database.  This was true before this patch, too, though it was
> > less obvious.  (Maybe that's not what you're asking?)
> 
> Yes, that's what I was asking. I guess it's orthogonal to your patch
> series, but the logic of returning FALSE for read only databases is not
> very intuitive to me (in the sense that "needs upgrade" is not the
> opposite of "can't be upgraded".  Your new comment later in the series
> is better, but it would IMHO be even better if you mentioned the read
> only case.

That's a good point.  Ideally this should be
"notmuch_database_can_upgrade" or something, which I think would
capture exactly what it means after this series.  However, I don't
think it's worth breaking API compatibility given that this is already
how callers use this function (even though that violates the current
library spec).

So, how's this for a more updated doc comment on needs_upgrade?

/**
 * Can the database be upgraded to a newer database version?
 *
 * If this function returns TRUE, then the caller may call
 * notmuch_database_upgrade to upgrade the database.  If the caller
 * does not upgrade an out-of-date database, then some functions may
 * fail with NOTMUCH_STATUS_UPGRADE_REQUIRED.  This always returns
 * FALSE for a read-only database because there's no way to upgrade a
 * read-only database.
 */

Re: [PATCH v3 04/13] lib: Database version 3: Introduce fine-grained "features"

[David Bremner]

Austin Clements <amdragon@mit.edu> writes:

> Quoth David Bremner on Aug 23 at  8:58 pm:
>> Austin Clements <amdragon@mit.edu> writes:
>> 
>> >>> @@ -1048,7 +1164,8 @@ notmuch_database_get_version (notmuch_database_t *notmuch)
>> >>>  notmuch_bool_t
>> >>>  notmuch_database_needs_upgrade (notmuch_database_t *notmuch)
>> >>>  {
>> >>> -    return notmuch->needs_upgrade;
>> >>> +    return notmuch->mode == NOTMUCH_DATABASE_MODE_READ_WRITE &&
>> >>> +	(NOTMUCH_FEATURES_CURRENT & ~notmuch->features);
>> >>>  }
>> >>
>> >> Maybe I'm not thinking hard enough here, but how does this deal with a
>> >> feature that is needed to open a database in read only mode? Maybe it
>> >> needs a comment for people not as clever as Austin ;).
>> >
>> > I'm not quite sure what you mean.  notmuch_database_needs_upgrade
>> > returns false for read-only databases because you can't upgrade a
>> > read-only database.  This was true before this patch, too, though it was
>> > less obvious.  (Maybe that's not what you're asking?)
>> 
>> Yes, that's what I was asking. I guess it's orthogonal to your patch
>> series, but the logic of returning FALSE for read only databases is not
>> very intuitive to me (in the sense that "needs upgrade" is not the
>> opposite of "can't be upgraded".  Your new comment later in the series
>> is better, but it would IMHO be even better if you mentioned the read
>> only case.
>
> That's a good point.  Ideally this should be
> "notmuch_database_can_upgrade" or something, which I think would
> capture exactly what it means after this series.  However, I don't
> think it's worth breaking API compatibility given that this is already
> how callers use this function (even though that violates the current
> library spec).
>
> So, how's this for a more updated doc comment on needs_upgrade?

LGTM