From: Austin Clements <amdragon@mit.edu>
To: notmuch@notmuchmail.org
Subject: [PATCH v4 00/11] Implement and use database "features"
Date: Mon, 25 Aug 2014 13:25:58 -0400
Message-Id: <1408987569-14146-1-git-send-email-amdragon@mit.edu>
Precedence: list

This is v4 of id:1406859003-11561-1-git-send-email-amdragon@mit.edu.
In addition to rebasing to current master, this makes several tidying
changes: it gives the features enum a name for better
self-documentation and type-safety; improves the robustness of
_parse_features to NULL pointers; requests upgrades if the database
version is old, even if it supports all current features; improves
various comments; and fixes some style errors.

The diff from v3 is below.  Most of this diff relates to giving the
enum a name, since it has to move above struct _notmuch_database and
we need to define bitwise operators for C++.

diff --git a/lib/database-private.h b/lib/database-private.h
index 2ffab33..ca0751c 100644
--- a/lib/database-private.h
+++ b/lib/database-private.h
@@ -36,36 +36,30 @@
 
 #pragma GCC visibility push(hidden)
 
-struct _notmuch_database {
-    notmuch_bool_t exception_reported;
-
-    char *path;
-
-    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;
-
-    Xapian::QueryParser *query_parser;
-    Xapian::TermGenerator *term_gen;
-    Xapian::ValueRangeProcessor *value_range_processor;
-    Xapian::ValueRangeProcessor *date_range_processor;
-};
-
-/* Bit masks for _notmuch_database::features. */
-enum {
+/* 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.
+ */
+enum _notmuch_features {
     /* 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. */
+     * Introduced: version 1. */
     NOTMUCH_FEATURE_FILE_TERMS = 1 << 0,
 
     /* If set, directory timestamps are stored in documents with
@@ -73,7 +67,7 @@ enum {
      * timestamps are stored in documents with XTIMESTAMP terms and
      * absolute paths.
      *
-     * Introduced: version 1.  Implementation support: required. */
+     * Introduced: version 1. */
     NOTMUCH_FEATURE_DIRECTORY_DOCS = 1 << 1,
 
     /* If set, the from, subject, and message-id headers are stored in
@@ -82,7 +76,6 @@ enum {
      * 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,
 
@@ -90,13 +83,71 @@ enum {
      * unset, folder terms are probabilistic and stemmed and path
      * terms do not exist.
      *
-     * Introduced: version 2.  Implementation support: required. */
+     * Introduced: version 2. */
     NOTMUCH_FEATURE_BOOL_FOLDER = 1 << 3,
 };
 
+/* In C++, a named enum is its own type, so define bitwise operators
+ * on _notmuch_features. */
+inline _notmuch_features
+operator|(_notmuch_features a, _notmuch_features b)
+{
+    return static_cast<_notmuch_features>(
+	static_cast<unsigned>(a) | static_cast<unsigned>(b));
+}
+
+inline _notmuch_features
+operator&(_notmuch_features a, _notmuch_features b)
+{
+    return static_cast<_notmuch_features>(
+	static_cast<unsigned>(a) & static_cast<unsigned>(b));
+}
+
+inline _notmuch_features
+operator~(_notmuch_features a)
+{
+    return static_cast<_notmuch_features>(~static_cast<unsigned>(a));
+}
+
+inline _notmuch_features&
+operator|=(_notmuch_features &a, _notmuch_features b)
+{
+    a = a | b;
+    return a;
+}
+
+inline _notmuch_features&
+operator&=(_notmuch_features &a, _notmuch_features b)
+{
+    a = a & b;
+    return a;
+}
+
+struct _notmuch_database {
+    notmuch_bool_t exception_reported;
+
+    char *path;
+
+    notmuch_database_mode_t mode;
+    int atomic_nesting;
+    Xapian::Database *xapian_db;
+
+    /* Bit mask of features used by this database.  This is a
+     * bitwise-OR of NOTMUCH_FEATURE_* values (above). */
+    enum _notmuch_features features;
+
+    unsigned int last_doc_id;
+    uint64_t last_thread_id;
+
+    Xapian::QueryParser *query_parser;
+    Xapian::TermGenerator *term_gen;
+    Xapian::ValueRangeProcessor *value_range_processor;
+    Xapian::ValueRangeProcessor *date_range_processor;
+};
+
 /* 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_V0 ((enum _notmuch_features)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)
diff --git a/lib/database.cc b/lib/database.cc
index 63257c2..5116188 100644
--- a/lib/database.cc
+++ b/lib/database.cc
@@ -266,10 +266,9 @@ _find_prefix (const char *name)
     return "";
 }
 
-static const struct
-{
+static const struct {
     /* NOTMUCH_FEATURE_* value. */
-    unsigned int value;
+    _notmuch_features 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. */
@@ -277,12 +276,16 @@ static const struct
     /* 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"},
+    { 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"},
+    { NOTMUCH_FEATURE_FROM_SUBJECT_ID_VALUES,
+      "from/subject/message-ID in database", "w" },
+    { NOTMUCH_FEATURE_BOOL_FOLDER,
+      "exact folder:/path: search", "rw" },
 };
 
 const char *
@@ -658,6 +661,7 @@ _notmuch_database_ensure_writable (notmuch_database_t *notmuch)
 }
 
 /* Parse a database features string from the given database version.
+ * Returns the feature bit set.
  *
  * For version < 3, this ignores the features string and returns a
  * hard-coded set of features.
@@ -665,13 +669,15 @@ _notmuch_database_ensure_writable (notmuch_database_t *notmuch)
  * 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.
+ * *incompat_out suitable for presenting to the user.  *incompat_out
+ * will be allocated from ctx.
  */
-static unsigned int
+static _notmuch_features
 _parse_features (const void *ctx, const char *features, unsigned int version,
 		 char mode, char **incompat_out)
 {
-    unsigned int res = 0, namelen, i;
+    _notmuch_features res = static_cast<_notmuch_features>(0);
+    unsigned int namelen, i;
     size_t llen = 0;
     const char *flags;
 
@@ -699,7 +705,7 @@ _parse_features (const void *ctx, const char *features, unsigned int version,
 	    }
 	}
 
-	if (i == ARRAY_SIZE (feature_names)) {
+	if (i == ARRAY_SIZE (feature_names) && incompat_out) {
 	    /* Unrecognized feature */
 	    const char *have = strchr (flags, mode);
 	    if (have && have < features + llen) {
@@ -1167,7 +1173,8 @@ notmuch_bool_t
 notmuch_database_needs_upgrade (notmuch_database_t *notmuch)
 {
     return notmuch->mode == NOTMUCH_DATABASE_MODE_READ_WRITE &&
-	(NOTMUCH_FEATURES_CURRENT & ~notmuch->features);
+	((NOTMUCH_FEATURES_CURRENT & ~notmuch->features) ||
+	 (notmuch_database_get_version (notmuch) < NOTMUCH_DATABASE_VERSION));
 }
 
 static volatile sig_atomic_t do_progress_notify = 0;
@@ -1202,7 +1209,7 @@ notmuch_database_upgrade (notmuch_database_t *notmuch,
     struct sigaction action;
     struct itimerval timerval;
     notmuch_bool_t timer_is_active = FALSE;
-    unsigned int target_features, new_features;
+    enum _notmuch_features target_features, new_features;
     notmuch_status_t status;
     unsigned int count = 0, total = 0;
 
diff --git a/lib/notmuch.h b/lib/notmuch.h
index d771eb2..21a5225 100644
--- a/lib/notmuch.h
+++ b/lib/notmuch.h
@@ -352,12 +352,14 @@ unsigned int
 notmuch_database_get_version (notmuch_database_t *database);
 
 /**
- * Is the database behind the latest supported database version?
+ * 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.
+ * 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.
  */
 notmuch_bool_t
 notmuch_database_needs_upgrade (notmuch_database_t *database);