[PATCH 0/9] Fix directory lookup on read-only databases

[PATCH 0/9] Fix directory lookup on read-only databases

[Austin Clements]

This fixes notmuch_database_get_directory and
notmuch_database_find_message_by_filename so that they don't attempt
to create missing directory documents.  This makes them work on
read-only databases (and prevents n_d_f_m_b_f from crashing
unceremoniously on read-only databases).

Unfortunately, there are several functions involved in directory
document lookup, so the first three patches simply add a creation flag
at each necessary layer.  The remaining patches then fix up the two
API functions and their uses.

If we do a 0.13.1 bug fix release, these patches could go in to
complement the API changes made in 0.13 to support these fixes.  David
can make that call.

There are several patches, but they're all short and incremental.

 bindings/python/notmuch/database.py |   11 -----------
 lib/database.cc                     |   40 +++++++++++++++++++++++++---------------
 lib/directory.cc                    |   41 +++++++++++++++++++++++++++++++++++------
 lib/message.cc                      |   11 +++++------
 lib/notmuch-private.h               |   13 +++++++++++++
 lib/notmuch.h                       |   10 ++--------
 notmuch-new.c                       |   36 +++++++-----------------------------
 test/python                         |    8 ++++++++
 8 files changed, 95 insertions(+), 75 deletions(-)

[PATCH 1/9] lib: Make directory document creation optional for _notmuch_directory_create

[Austin Clements]

Previously this function would create directory documents if they
didn't exist.  As a result, it could only be used on writable
databases.  This adds an argument to make creation optional and to
make this function work on read-only databases.  We use a flag
argument to avoid a bare boolean and to permit future expansion.

Both callers have been updated, but currently retain the old behavior.
We'll take advantage of the new argument in the following patches.
---
 lib/database.cc       |    6 +++---
 lib/directory.cc      |   33 ++++++++++++++++++++++++++++-----
 lib/notmuch-private.h |    8 ++++++++
 3 files changed, 39 insertions(+), 8 deletions(-)

diff --git a/lib/database.cc b/lib/database.cc
index f8c4a7d..df996a9 100644
--- a/lib/database.cc
+++ b/lib/database.cc
@@ -956,7 +956,7 @@ notmuch_database_upgrade (notmuch_database_t *notmuch,
 		    document.get_value (NOTMUCH_VALUE_TIMESTAMP));
 
 		directory = _notmuch_directory_create (notmuch, term.c_str() + 10,
-						       &status);
+						       NOTMUCH_FIND_CREATE, &status);
 		notmuch_directory_set_mtime (directory, mtime);
 		notmuch_directory_destroy (directory);
 	    }
@@ -1210,7 +1210,7 @@ _notmuch_database_find_directory_id (notmuch_database_t *notmuch,
 	return NOTMUCH_STATUS_SUCCESS;
     }
 
-    directory = _notmuch_directory_create (notmuch, path, &status);
+    directory = _notmuch_directory_create (notmuch, path, NOTMUCH_FIND_CREATE, &status);
     if (status) {
 	*directory_id = -1;
 	return status;
@@ -1320,7 +1320,7 @@ notmuch_database_get_directory (notmuch_database_t *notmuch,
 	return NOTMUCH_STATUS_READ_ONLY_DATABASE;
 
     try {
-	*directory = _notmuch_directory_create (notmuch, path, &status);
+	*directory = _notmuch_directory_create (notmuch, path, NOTMUCH_FIND_CREATE, &status);
     } catch (const Xapian::Error &error) {
 	fprintf (stderr, "A Xapian exception occurred getting directory: %s.\n",
 		 error.get_msg().c_str());
diff --git a/lib/directory.cc b/lib/directory.cc
index 70e1693..83bb19b 100644
--- a/lib/directory.cc
+++ b/lib/directory.cc
@@ -82,28 +82,41 @@ find_directory_document (notmuch_database_t *notmuch,
     return NOTMUCH_PRIVATE_STATUS_SUCCESS;
 }
 
+/* Find or create a directory document.
+ *
+ * 'path' should be a path relative to the path of 'database', or else
+ * should be an absolute path with initial components that match the
+ * path of 'database'.
+ *
+ * If (flags & NOTMUCH_FIND_CREATE), then the directory document will
+ * be created if it does not exist.  Otherwise, if the directory
+ * document does not exist, *status_ret is set to
+ * NOTMUCH_STATUS_SUCCESS and this returns NULL.
+ */
 notmuch_directory_t *
 _notmuch_directory_create (notmuch_database_t *notmuch,
 			   const char *path,
+			   notmuch_find_flags_t flags,
 			   notmuch_status_t *status_ret)
 {
     Xapian::WritableDatabase *db;
     notmuch_directory_t *directory;
     notmuch_private_status_t private_status;
     const char *db_path;
+    notmuch_bool_t create = (flags & NOTMUCH_FIND_CREATE);
 
     *status_ret = NOTMUCH_STATUS_SUCCESS;
 
     path = _notmuch_database_relative_path (notmuch, path);
 
-    if (notmuch->mode == NOTMUCH_DATABASE_MODE_READ_ONLY)
+    if (create && notmuch->mode == NOTMUCH_DATABASE_MODE_READ_ONLY)
 	INTERNAL_ERROR ("Failure to ensure database is writable");
 
-    db = static_cast <Xapian::WritableDatabase *> (notmuch->xapian_db);
-
     directory = talloc (notmuch, notmuch_directory_t);
-    if (unlikely (directory == NULL))
+    if (unlikely (directory == NULL)) {
+	*status_ret = NOTMUCH_STATUS_OUT_OF_MEMORY;
 	return NULL;
+    }
 
     directory->notmuch = notmuch;
 
@@ -122,6 +135,13 @@ _notmuch_directory_create (notmuch_database_t *notmuch,
 	directory->document_id = directory->doc.get_docid ();
 
 	if (private_status == NOTMUCH_PRIVATE_STATUS_NO_DOCUMENT_FOUND) {
+	    if (!create) {
+		notmuch_directory_destroy (directory);
+		directory = NULL;
+		*status_ret = NOTMUCH_STATUS_SUCCESS;
+		goto DONE;
+	    }
+
 	    void *local = talloc_new (directory);
 	    const char *parent, *basename;
 	    Xapian::docid parent_id;
@@ -145,6 +165,8 @@ _notmuch_directory_create (notmuch_database_t *notmuch,
 	    directory->doc.add_value (NOTMUCH_VALUE_TIMESTAMP,
 				      Xapian::sortable_serialise (0));
 
+	    db = static_cast <Xapian::WritableDatabase *> (notmuch->xapian_db);
+
 	    directory->document_id = _notmuch_database_generate_doc_id (notmuch);
 	    db->replace_document (directory->document_id, directory->doc);
 	    talloc_free (local);
@@ -158,10 +180,11 @@ _notmuch_directory_create (notmuch_database_t *notmuch,
 		 error.get_msg().c_str());
 	notmuch->exception_reported = TRUE;
 	notmuch_directory_destroy (directory);
+	directory = NULL;
 	*status_ret = NOTMUCH_STATUS_XAPIAN_EXCEPTION;
-	return NULL;
     }
 
+  DONE:
     if (db_path != path)
 	free ((char *) db_path);
 
diff --git a/lib/notmuch-private.h b/lib/notmuch-private.h
index 3886e0c..538274f 100644
--- a/lib/notmuch-private.h
+++ b/lib/notmuch-private.h
@@ -146,6 +146,13 @@ typedef enum _notmuch_private_status {
      :									\
      (notmuch_status_t) private_status)
 
+/* Flags shared by various lookup functions. */
+typedef enum _notmuch_find_flags {
+    /* If set, create the necessary document (or documents) if they
+     * are missing.  Requires a read/write database. */
+    NOTMUCH_FIND_CREATE = 1<<0,
+} notmuch_find_flags_t;
+
 typedef struct _notmuch_doc_id_set notmuch_doc_id_set_t;
 
 typedef struct _notmuch_string_list notmuch_string_list_t;
@@ -206,6 +213,7 @@ _notmuch_database_filename_to_direntry (void *ctx,
 notmuch_directory_t *
 _notmuch_directory_create (notmuch_database_t *notmuch,
 			   const char *path,
+			   notmuch_find_flags_t flags,
 			   notmuch_status_t *status_ret);
 
 unsigned int
-- 
1.7.10

[PATCH 2/9] lib: Perform the same transformation to _notmuch_database_find_directory_id

[Austin Clements]

Now _notmuch_database_find_directory_id takes a flags argument, which
it passes through to _notmuch_directory_create and can indicate if the
directory does not exist.  Again, callers have been updated, but
retain their original behavior.
---
 lib/database.cc       |   14 +++++++++++---
 lib/directory.cc      |    8 +++++++-
 lib/notmuch-private.h |    1 +
 3 files changed, 19 insertions(+), 4 deletions(-)

diff --git a/lib/database.cc b/lib/database.cc
index df996a9..716982d 100644
--- a/lib/database.cc
+++ b/lib/database.cc
@@ -1197,9 +1197,17 @@ _notmuch_database_split_path (void *ctx,
     return NOTMUCH_STATUS_SUCCESS;
 }
 
+/* Find the document ID of the specified directory.
+ *
+ * If (flags & NOTMUCH_FIND_CREATE), a new directory document will be
+ * created if one does not exist for 'path'.  Otherwise, if the
+ * directory document does not exist, this sets *directory_id to
+ * ((unsigned int)-1) and returns NOTMUCH_STATUS_SUCCESS.
+ */
 notmuch_status_t
 _notmuch_database_find_directory_id (notmuch_database_t *notmuch,
 				     const char *path,
+				     notmuch_find_flags_t flags,
 				     unsigned int *directory_id)
 {
     notmuch_directory_t *directory;
@@ -1210,8 +1218,8 @@ _notmuch_database_find_directory_id (notmuch_database_t *notmuch,
 	return NOTMUCH_STATUS_SUCCESS;
     }
 
-    directory = _notmuch_directory_create (notmuch, path, NOTMUCH_FIND_CREATE, &status);
-    if (status) {
+    directory = _notmuch_directory_create (notmuch, path, flags, &status);
+    if (status || !directory) {
 	*directory_id = -1;
 	return status;
     }
@@ -1260,7 +1268,7 @@ _notmuch_database_filename_to_direntry (void *ctx,
     if (status)
 	return status;
 
-    status = _notmuch_database_find_directory_id (notmuch, directory,
+    status = _notmuch_database_find_directory_id (notmuch, directory, NOTMUCH_FIND_CREATE,
 						  &directory_id);
     if (status)
 	return status;
diff --git a/lib/directory.cc b/lib/directory.cc
index 83bb19b..6a3ffed 100644
--- a/lib/directory.cc
+++ b/lib/directory.cc
@@ -153,7 +153,13 @@ _notmuch_directory_create (notmuch_database_t *notmuch,
 
 	    _notmuch_database_split_path (local, path, &parent, &basename);
 
-	    _notmuch_database_find_directory_id (notmuch, parent, &parent_id);
+	    *status_ret = _notmuch_database_find_directory_id (
+		notmuch, parent, NOTMUCH_FIND_CREATE, &parent_id);
+	    if (*status_ret) {
+		notmuch_directory_destroy (directory);
+		directory = NULL;
+		goto DONE;
+	    }
 
 	    if (basename) {
 		term = talloc_asprintf (local, "%s%u:%s",
diff --git a/lib/notmuch-private.h b/lib/notmuch-private.h
index 538274f..a36549d 100644
--- a/lib/notmuch-private.h
+++ b/lib/notmuch-private.h
@@ -195,6 +195,7 @@ _notmuch_database_find_unique_doc_id (notmuch_database_t *notmuch,
 notmuch_status_t
 _notmuch_database_find_directory_id (notmuch_database_t *database,
 				     const char *path,
+				     notmuch_find_flags_t flags,
 				     unsigned int *directory_id);
 
 const char *
-- 
1.7.10

[PATCH 3/9] lib: Perform the same transformation to _notmuch_database_filename_to_direntry

[Austin Clements]

Now _notmuch_database_filename_to_direntry takes a flags argument and
can indicate if the necessary directory documents do not exist.
Again, callers have been updated, but retain their original behavior.
---
 lib/database.cc       |   17 +++++++++++------
 lib/message.cc        |    9 ++++-----
 lib/notmuch-private.h |    1 +
 3 files changed, 16 insertions(+), 11 deletions(-)

diff --git a/lib/database.cc b/lib/database.cc
index 716982d..b4c76b4 100644
--- a/lib/database.cc
+++ b/lib/database.cc
@@ -1248,13 +1248,16 @@ _notmuch_database_get_directory_path (void *ctx,
  * database path), return a new string (with 'ctx' as the talloc
  * owner) suitable for use as a direntry term value.
  *
- * The necessary directory documents will be created in the database
- * as needed.
+ * If (flags & NOTMUCH_FIND_CREATE), the necessary directory documents
+ * will be created in the database as needed.  Otherwise, if the
+ * necessary directory documents do not exist, this sets
+ * *direntry to NULL and returns NOTMUCH_STATUS_SUCCESS.
  */
 notmuch_status_t
 _notmuch_database_filename_to_direntry (void *ctx,
 					notmuch_database_t *notmuch,
 					const char *filename,
+					notmuch_find_flags_t flags,
 					char **direntry)
 {
     const char *relative, *directory, *basename;
@@ -1268,10 +1271,12 @@ _notmuch_database_filename_to_direntry (void *ctx,
     if (status)
 	return status;
 
-    status = _notmuch_database_find_directory_id (notmuch, directory, NOTMUCH_FIND_CREATE,
+    status = _notmuch_database_find_directory_id (notmuch, directory, flags,
 						  &directory_id);
-    if (status)
+    if (status || directory_id == (unsigned int)-1) {
+	*direntry = NULL;
 	return status;
+    }
 
     *direntry = talloc_asprintf (ctx, "%u:%s", directory_id, basename);
 
@@ -1892,8 +1897,8 @@ notmuch_database_find_message_by_filename (notmuch_database_t *notmuch,
     local = talloc_new (notmuch);
 
     try {
-	status = _notmuch_database_filename_to_direntry (local, notmuch,
-							 filename, &direntry);
+	status = _notmuch_database_filename_to_direntry (
+	    local, notmuch, filename, NOTMUCH_FIND_CREATE, &direntry);
 	if (status)
 	    goto DONE;
 
diff --git a/lib/message.cc b/lib/message.cc
index 0075425..8d552f1 100644
--- a/lib/message.cc
+++ b/lib/message.cc
@@ -495,9 +495,8 @@ _notmuch_message_add_filename (notmuch_message_t *message,
     if (status)
 	return status;
 
-    status = _notmuch_database_filename_to_direntry (local,
-						     message->notmuch,
-						     filename, &direntry);
+    status = _notmuch_database_filename_to_direntry (
+	local, message->notmuch, filename, NOTMUCH_FIND_CREATE, &direntry);
     if (status)
 	return status;
 
@@ -541,8 +540,8 @@ _notmuch_message_remove_filename (notmuch_message_t *message,
     notmuch_status_t status;
     Xapian::TermIterator i, last;
 
-    status = _notmuch_database_filename_to_direntry (local, message->notmuch,
-						     filename, &direntry);
+    status = _notmuch_database_filename_to_direntry (
+	local, message->notmuch, filename, NOTMUCH_FIND_CREATE, &direntry);
     if (status)
 	return status;
 
diff --git a/lib/notmuch-private.h b/lib/notmuch-private.h
index a36549d..34f7ac7 100644
--- a/lib/notmuch-private.h
+++ b/lib/notmuch-private.h
@@ -207,6 +207,7 @@ notmuch_status_t
 _notmuch_database_filename_to_direntry (void *ctx,
 					notmuch_database_t *notmuch,
 					const char *filename,
+					notmuch_find_flags_t flags,
 					char **direntry);
 
 /* directory.cc */
-- 
1.7.10

[PATCH 4/9] lib: Make notmuch_database_get_directory return NULL if the directory is not found

[Austin Clements]

Using the new support from _notmuch_directory_create, this makes
notmuch_database_get_directory a read-only operation that simply
returns the directory object if it exists or NULL otherwise.  This
also means that notmuch_database_get_directory can work on read-only
databases.

This change breaks the directory mtime workaround in notmuch-new.c by
fixing the exact issue it was working around.  This permits mtime
update races to prevent scans of changed directories, which
non-deterministically breaks a few tests.  The next patch fixes this.
---
 lib/database.cc       |    7 ++-----
 lib/notmuch-private.h |    3 +++
 lib/notmuch.h         |   10 ++--------
 3 files changed, 7 insertions(+), 13 deletions(-)

diff --git a/lib/database.cc b/lib/database.cc
index b4c76b4..e27a0e1 100644
--- a/lib/database.cc
+++ b/lib/database.cc
@@ -1328,12 +1328,9 @@ notmuch_database_get_directory (notmuch_database_t *notmuch,
 	return NOTMUCH_STATUS_NULL_POINTER;
     *directory = NULL;
 
-    /* XXX Handle read-only databases properly */
-    if (notmuch->mode == NOTMUCH_DATABASE_MODE_READ_ONLY)
-	return NOTMUCH_STATUS_READ_ONLY_DATABASE;
-
     try {
-	*directory = _notmuch_directory_create (notmuch, path, NOTMUCH_FIND_CREATE, &status);
+	*directory = _notmuch_directory_create (notmuch, path,
+						NOTMUCH_FIND_LOOKUP, &status);
     } catch (const Xapian::Error &error) {
 	fprintf (stderr, "A Xapian exception occurred getting directory: %s.\n",
 		 error.get_msg().c_str());
diff --git a/lib/notmuch-private.h b/lib/notmuch-private.h
index 34f7ac7..bfb4111 100644
--- a/lib/notmuch-private.h
+++ b/lib/notmuch-private.h
@@ -148,6 +148,9 @@ typedef enum _notmuch_private_status {
 
 /* Flags shared by various lookup functions. */
 typedef enum _notmuch_find_flags {
+    /* Lookup without creating any documents.  This is the default
+     * behavior. */
+    NOTMUCH_FIND_LOOKUP = 0,
     /* If set, create the necessary document (or documents) if they
      * are missing.  Requires a read/write database. */
     NOTMUCH_FIND_CREATE = 1<<0,
diff --git a/lib/notmuch.h b/lib/notmuch.h
index bbb17e4..3633bed 100644
--- a/lib/notmuch.h
+++ b/lib/notmuch.h
@@ -300,10 +300,8 @@ notmuch_database_end_atomic (notmuch_database_t *notmuch);
  * (see notmuch_database_get_path), or else should be an absolute path
  * with initial components that match the path of 'database'.
  *
- * Note: Currently this will create the directory object if it doesn't
- * exist.  In the future, when a directory object does not exist this
- * will return NOTMUCH_STATUS_SUCCESS and set *directory to NULL.
- * Callers should be prepared for this.
+ * If this directory object does not exist in the database, this
+ * returns NOTMUCH_STATUS_SUCCESS and sets *directory to NULL.
  *
  * Return value:
  *
@@ -313,10 +311,6 @@ notmuch_database_end_atomic (notmuch_database_t *notmuch);
  *
  * NOTMUCH_STATUS_XAPIAN_EXCEPTION: A Xapian exception occurred;
  *	directory not retrieved.
- *
- * NOTMUCH_STATUS_READ_ONLY_DATABASE: Database was opened in read-only
- *	mode so the directory cannot be created (this case will be
- *	removed in the future).
  */
 notmuch_status_t
 notmuch_database_get_directory (notmuch_database_t *database,
-- 
1.7.10

[PATCH 5/9] new: Remove workaround for detecting newly created directory objects

[Austin Clements]

Previously, notmuch_database_get_directory did not indicate whether or
not the returned directory object was newly created, which required a
workaround to distinguish newly created directory objects with no
child messages from directory objects that had no mtime set but did
have child messages.  Now that notmuch_database_get_directory
distinguishes whether or not the directory object exists in the
database, this workaround is no longer necessary.
---
 notmuch-new.c |   36 +++++++-----------------------------
 1 file changed, 7 insertions(+), 29 deletions(-)

diff --git a/notmuch-new.c b/notmuch-new.c
index a3a8bec..72dd558 100644
--- a/notmuch-new.c
+++ b/notmuch-new.c
@@ -256,7 +256,7 @@ add_files_recursive (notmuch_database_t *notmuch,
     notmuch_filenames_t *db_subdirs = NULL;
     time_t stat_time;
     struct stat st;
-    notmuch_bool_t is_maildir, new_directory;
+    notmuch_bool_t is_maildir;
     const char **tag;
 
     if (stat (path, &st)) {
@@ -281,33 +281,12 @@ add_files_recursive (notmuch_database_t *notmuch,
     }
     db_mtime = directory ? notmuch_directory_get_mtime (directory) : 0;
 
-    new_directory = db_mtime ? FALSE : TRUE;
-
-    /* XXX This is a temporary workaround.  If we don't update the
-     * database mtime until after processing messages in this
-     * directory, then a 0 mtime is *not* sufficient to indicate that
-     * this directory has no messages or subdirs in the database (for
-     * example, if an earlier run skipped the mtime update because
-     * fs_mtime == stat_time, or was interrupted before updating the
-     * mtime at the end).  To address this, we record a (bogus)
-     * non-zero value before processing any child messages so that a
-     * later run won't mistake this for a new directory (and, for
-     * example, fail to detect removed files and subdirs).
-     *
-     * A better solution would be for notmuch_database_get_directory
-     * to indicate if it really created a new directory or not, either
-     * by a new out-argument, or by recording this information and
-     * providing an accessor.
-     */
-    if (new_directory && directory)
-	notmuch_directory_set_mtime (directory, -1);
-
     /* If the database knows about this directory, then we sort based
      * on strcmp to match the database sorting. Otherwise, we can do
      * inode-based sorting for faster filesystem operation. */
     num_fs_entries = scandir (path, &fs_entries, 0,
-			      new_directory ?
-			      dirent_sort_inode : dirent_sort_strcmp_name);
+			      directory ?
+			      dirent_sort_strcmp_name : dirent_sort_inode);
 
     if (num_fs_entries == -1) {
 	fprintf (stderr, "Error opening directory %s: %s\n",
@@ -376,13 +355,12 @@ add_files_recursive (notmuch_database_t *notmuch,
      * being discovered until the clock catches up and the directory
      * is modified again).
      */
-    if (fs_mtime == db_mtime)
+    if (directory && fs_mtime == db_mtime)
 	goto DONE;
 
-    /* new_directory means a directory that the database has never
-     * seen before. In that case, we can simply leave db_files and
-     * db_subdirs NULL. */
-    if (!new_directory) {
+    /* If the database has never seen this directory before, we can
+     * simply leave db_files and db_subdirs NULL. */
+    if (directory) {
 	db_files = notmuch_directory_get_child_files (directory);
 	db_subdirs = notmuch_directory_get_child_directories (directory);
     }
-- 
1.7.10

[PATCH 6/9] python: Update Database.get_directory documentation

[Austin Clements]

notmuch_database_get_directory no longer returns an error for
read-only databases, so remove ReadOnlyDatabaseError from the list of
get_directory exceptions.
---
 bindings/python/notmuch/database.py |    3 ---
 1 file changed, 3 deletions(-)

diff --git a/bindings/python/notmuch/database.py b/bindings/python/notmuch/database.py
index 797554d..ff89818 100644
--- a/bindings/python/notmuch/database.py
+++ b/bindings/python/notmuch/database.py
@@ -346,7 +346,6 @@ class Database(object):
 
     def get_directory(self, path):
         """Returns a :class:`Directory` of path,
-        (creating it if it does not exist(?))
 
         :param path: An unicode string containing the path relative to the path
               of database (see :meth:`get_path`), or else should be an absolute
@@ -354,8 +353,6 @@ class Database(object):
         :returns: :class:`Directory` or raises an exception.
         :raises: :exc:`FileError` if path is not relative database or absolute
                  with initial components same as database.
-        :raises: :exc:`ReadOnlyDatabaseError` if the database has not been
-                 opened in read-write mode
         """
         self._assert_db_is_initialized()
 
-- 
1.7.10

[PATCH 7/9] lib: Make notmuch_database_find_message_by_filename not crash on read-only databases

[Austin Clements]

Previously, _notmuch_database_filename_to_direntry would abort with an
internal error when called on a read-only database.  Now that creating
the directory document is optional,
notmuch_database_find_message_by_filename can disable directory
document creation (as it should) and, as a result, not abort on
read-only databases.
---
 lib/database.cc |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/database.cc b/lib/database.cc
index e27a0e1..761dc1a 100644
--- a/lib/database.cc
+++ b/lib/database.cc
@@ -1895,8 +1895,8 @@ notmuch_database_find_message_by_filename (notmuch_database_t *notmuch,
 
     try {
 	status = _notmuch_database_filename_to_direntry (
-	    local, notmuch, filename, NOTMUCH_FIND_CREATE, &direntry);
-	if (status)
+	    local, notmuch, filename, NOTMUCH_FIND_LOOKUP, &direntry);
+	if (status || !direntry)
 	    goto DONE;
 
 	term = talloc_asprintf (local, "%s%s", prefix, direntry);
-- 
1.7.10

[PATCH 8/9] python: Remove find_message_by_filename workaround

[Austin Clements]

Now that notmuch_database_find_message_by_filename works on read-only
databases, remove the workaround that disabled it on read-write
databases.

This also adds a regression test for find_message_by_filename.
---
 bindings/python/notmuch/database.py |    9 ---------
 test/python                         |    8 ++++++++
 2 files changed, 8 insertions(+), 9 deletions(-)

diff --git a/bindings/python/notmuch/database.py b/bindings/python/notmuch/database.py
index ff89818..e5c74cf 100644
--- a/bindings/python/notmuch/database.py
+++ b/bindings/python/notmuch/database.py
@@ -526,19 +526,10 @@ class Database(object):
                  retry.
         :raises: :exc:`NotInitializedError` if the database was not
                  intitialized.
-        :raises: :exc:`ReadOnlyDatabaseError` if the database has not been
-                 opened in read-write mode
 
         *Added in notmuch 0.9*"""
         self._assert_db_is_initialized()
 
-        # work around libnotmuch calling exit(3), see
-        # id:20120221002921.8534.57091@thinkbox.jade-hamburg.de
-        # TODO: remove once this issue is resolved
-        if self.mode != Database.MODE.READ_WRITE:
-            raise ReadOnlyDatabaseError('The database has to be opened in '
-                                        'read-write mode for get_directory')
-
         msg_p = NotmuchMessageP()
         status = Database._find_message_by_filename(self._db, _str(filename),
                                                     byref(msg_p))
diff --git a/test/python b/test/python
index 6018c2d..3f03a2e 100755
--- a/test/python
+++ b/test/python
@@ -28,4 +28,12 @@ EOF
 notmuch search --sort=oldest-first --output=messages tag:inbox | sed s/^id:// > EXPECTED
 test_expect_equal_file OUTPUT EXPECTED
 
+test_begin_subtest "get non-existent file"
+test_python <<EOF
+import notmuch
+db = notmuch.Database(mode=notmuch.Database.MODE.READ_ONLY)
+print db.find_message_by_filename("i-dont-exist")
+EOF
+test_expect_equal "$(cat OUTPUT)" "None"
+
 test_done
-- 
1.7.10

[PATCH 9/9] lib: Don't needlessly create directory docs in _notmuch_message_remove_filename

[Austin Clements]

Previously, if passed a filename with a directory that did not exist
in the database, _notmuch_message_remove_filename would needlessly
create that directory document.  Fix it so that doesn't happen.
---
 lib/message.cc |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/message.cc b/lib/message.cc
index 8d552f1..6787506 100644
--- a/lib/message.cc
+++ b/lib/message.cc
@@ -541,8 +541,8 @@ _notmuch_message_remove_filename (notmuch_message_t *message,
     Xapian::TermIterator i, last;
 
     status = _notmuch_database_filename_to_direntry (
-	local, message->notmuch, filename, NOTMUCH_FIND_CREATE, &direntry);
-    if (status)
+	local, message->notmuch, filename, NOTMUCH_FIND_LOOKUP, &direntry);
+    if (status || !direntry)
 	return status;
 
     /* Unlink this file from its parent directory. */
-- 
1.7.10

Re: [PATCH 0/9] Fix directory lookup on read-only databases

[Tomi Ollila]

Austin Clements <amdragon@MIT.EDU> writes:

> This fixes notmuch_database_get_directory and
> notmuch_database_find_message_by_filename so that they don't attempt
> to create missing directory documents.  This makes them work on
> read-only databases (and prevents n_d_f_m_b_f from crashing
> unceremoniously on read-only databases).
>
> Unfortunately, there are several functions involved in directory
> document lookup, so the first three patches simply add a creation flag
> at each necessary layer.  The remaining patches then fix up the two
> API functions and their uses.
>
> If we do a 0.13.1 bug fix release, these patches could go in to
> complement the API changes made in 0.13 to support these fixes.  David
> can make that call.
>
> There are several patches, but they're all short and incremental.

This patch series looks good to me...


Tomi


>
>  bindings/python/notmuch/database.py |   11 -----------
>  lib/database.cc                     |   40 +++++++++++++++++++++++++---------------
>  lib/directory.cc                    |   41 +++++++++++++++++++++++++++++++++++------
>  lib/message.cc                      |   11 +++++------
>  lib/notmuch-private.h               |   13 +++++++++++++
>  lib/notmuch.h                       |   10 ++--------
>  notmuch-new.c                       |   36 +++++++-----------------------------
>  test/python                         |    8 ++++++++
>  8 files changed, 95 insertions(+), 75 deletions(-)
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH 0/9] Fix directory lookup on read-only databases

[Justus Winter]

Hi Austin :)

Quoting Austin Clements (2012-05-18 06:13:33)
> This fixes notmuch_database_get_directory and
> notmuch_database_find_message_by_filename so that they don't attempt
> to create missing directory documents.  This makes them work on
> read-only databases (and prevents n_d_f_m_b_f from crashing
> unceremoniously on read-only databases).

Thanks for taking care of this, this patch series looks good.

> If we do a 0.13.1 bug fix release, these patches could go in to
> complement the API changes made in 0.13 to support these fixes.  David
> can make that call.

I'd support this.

Cheers,
Justus

Re: [PATCH 1/9] lib: Make directory document creation optional for _notmuch_directory_create

[David Bremner]

Austin Clements <amdragon@MIT.EDU> writes:

> Previously this function would create directory documents if they
> didn't exist.  As a result, it could only be used on writable
> databases.  This adds an argument to make creation optional and to
> make this function work on read-only databases.  We use a flag
> argument to avoid a bare boolean and to permit future expansion.

Series pushed, 

d

PS. I marked a couple other of your patches stale in nmbug.