[PATCH 0/2] lib: introduce notmuch_database_new

[PATCH 0/2] lib: introduce notmuch_database_new

[Jani Nikula]

Hi all -

In preparation of adding some logging hooks into the library (instead of
printing everything to stdout/stderr) we need a way to set that kind of
options before opening/creating the database. Here's a proposed API
change to make that possible in the future.

N.B. This breaks bindings and contrib/notmuch-deliver.

BR,
Jani.


Jani Nikula (2):
  lib: add return status to database close and destroy
  lib: introduce notmuch_database_new for initializing a database handle

 lib/database.cc      | 80 ++++++++++++++++++++++++++++++----------------------
 lib/notmuch.h        | 69 ++++++++++++++++++++++++++++++++++----------
 notmuch-compact.c    | 11 +++++++-
 notmuch-count.c      | 10 +++++--
 notmuch-dump.c       | 10 +++++--
 notmuch-insert.c     | 10 +++++--
 notmuch-new.c        | 14 +++++----
 notmuch-reply.c      | 10 +++++--
 notmuch-restore.c    | 10 +++++--
 notmuch-search.c     | 10 +++++--
 notmuch-show.c       | 10 +++++--
 notmuch-tag.c        | 10 +++++--
 test/random-corpus.c | 10 +++++--
 test/symbol-test.cc  |  3 +-
 14 files changed, 193 insertions(+), 74 deletions(-)

-- 
1.8.4.2

[PATCH 1/2] lib: add return status to database close and destroy

[Jani Nikula]

notmuch_database_close may fail in Xapian ->flush() or ->close(), so
report the status. Similarly for notmuch_database_destroy which calls
close.

This is required for notmuch insert to report error status if message
indexing failed.

Bump the notmuch version to allow clients to conditional build against
both the current release and the next release (current git master).
---
 lib/database.cc | 18 ++++++++++++++----
 lib/notmuch.h   | 17 ++++++++++++++---
 2 files changed, 28 insertions(+), 7 deletions(-)

diff --git a/lib/database.cc b/lib/database.cc
index f395061..98e2c31 100644
--- a/lib/database.cc
+++ b/lib/database.cc
@@ -767,14 +767,17 @@ notmuch_database_open (const char *path,
     return status;
 }
 
-void
+notmuch_status_t
 notmuch_database_close (notmuch_database_t *notmuch)
 {
+    notmuch_status_t status = NOTMUCH_STATUS_SUCCESS;
+
     try {
 	if (notmuch->xapian_db != NULL &&
 	    notmuch->mode == NOTMUCH_DATABASE_MODE_READ_WRITE)
 	    (static_cast <Xapian::WritableDatabase *> (notmuch->xapian_db))->flush ();
     } catch (const Xapian::Error &error) {
+	status = NOTMUCH_STATUS_XAPIAN_EXCEPTION;
 	if (! notmuch->exception_reported) {
 	    fprintf (stderr, "Error: A Xapian exception occurred flushing database: %s\n",
 		     error.get_msg().c_str());
@@ -789,6 +792,7 @@ notmuch_database_close (notmuch_database_t *notmuch)
 	    notmuch->xapian_db->close();
 	} catch (const Xapian::Error &error) {
 	    /* do nothing */
+	    status = NOTMUCH_STATUS_XAPIAN_EXCEPTION;
 	}
     }
 
@@ -802,6 +806,8 @@ notmuch_database_close (notmuch_database_t *notmuch)
     notmuch->value_range_processor = NULL;
     delete notmuch->date_range_processor;
     notmuch->date_range_processor = NULL;
+
+    return status;
 }
 
 #if HAVE_XAPIAN_COMPACT
@@ -966,7 +972,7 @@ notmuch_database_compact (const char *path,
 
   DONE:
     if (notmuch)
-	notmuch_database_destroy (notmuch);
+	ret = notmuch_database_destroy (notmuch);
 
     talloc_free (local);
 
@@ -984,11 +990,15 @@ notmuch_database_compact (unused (const char *path),
 }
 #endif
 
-void
+notmuch_status_t
 notmuch_database_destroy (notmuch_database_t *notmuch)
 {
-    notmuch_database_close (notmuch);
+    notmuch_status_t status;
+
+    status = notmuch_database_close (notmuch);
     talloc_free (notmuch);
+
+    return status;
 }
 
 const char *
diff --git a/lib/notmuch.h b/lib/notmuch.h
index 7c3a30c..dbdce86 100644
--- a/lib/notmuch.h
+++ b/lib/notmuch.h
@@ -42,7 +42,7 @@ NOTMUCH_BEGIN_DECLS
 #endif
 
 #define NOTMUCH_MAJOR_VERSION	0
-#define NOTMUCH_MINOR_VERSION	17
+#define NOTMUCH_MINOR_VERSION	18
 #define NOTMUCH_MICRO_VERSION	0
 
 /*
@@ -236,8 +236,16 @@ notmuch_database_open (const char *path,
  *
  * notmuch_database_close can be called multiple times.  Later calls
  * have no effect.
+ *
+ * Return value:
+ *
+ * NOTMUCH_STATUS_SUCCESS: Successfully closed the database.
+ *
+ * NOTMUCH_STATUS_XAPIAN_EXCEPTION: A Xapian exception occurred; the
+ *	database has been closed but there are no guarantees the
+ *	changes to the database, if any, have been flushed to disk.
  */
-void
+notmuch_status_t
 notmuch_database_close (notmuch_database_t *database);
 
 /* A callback invoked by notmuch_database_compact to notify the user
@@ -263,8 +271,11 @@ notmuch_database_compact (const char* path,
 
 /* Destroy the notmuch database, closing it if necessary and freeing
  * all associated resources.
+ *
+ * Return value as in notmuch_database_close if the database was open;
+ * notmuch_database_destroy itself has no failure modes.
  */
-void
+notmuch_status_t
 notmuch_database_destroy (notmuch_database_t *database);
 
 /* Return the database path of the given database.
-- 
1.8.4.2

[PATCH 2/2] lib: introduce notmuch_database_new for initializing a database handle

[Jani Nikula]

There is a need for setting options before opening a database, such as
setting a logging function to use instead of writing to stdout or
stderr. It would be possible to do this by adding new parameters to
notmuch_database_create and notmuch_database_open, but maintaining a
backwards compatible API and ABI when new options are added becomes
burdensome.

Instead, split the opaque database object creation from
notmuch_database_create and notmuch_database_open into a new
notmuch_database_new call, to allow operations on the handle before
create and open. This creates API and ABI breakage now, but allows
easier future extensions.

The notmuch_database_new call becomes a natural pair to the already
existing notmuch_database_destroy, and it should be possible to call
open/close multiple times using an initialized handle.
---
 lib/database.cc      | 64 ++++++++++++++++++++++++++++------------------------
 lib/notmuch.h        | 52 ++++++++++++++++++++++++++++++++----------
 notmuch-compact.c    | 11 ++++++++-
 notmuch-count.c      | 10 ++++++--
 notmuch-dump.c       | 10 ++++++--
 notmuch-insert.c     | 10 ++++++--
 notmuch-new.c        | 14 +++++++-----
 notmuch-reply.c      | 10 ++++++--
 notmuch-restore.c    | 10 ++++++--
 notmuch-search.c     | 10 ++++++--
 notmuch-show.c       | 10 ++++++--
 notmuch-tag.c        | 10 ++++++--
 test/random-corpus.c | 10 ++++++--
 test/symbol-test.cc  |  3 ++-
 14 files changed, 166 insertions(+), 68 deletions(-)

diff --git a/lib/database.cc b/lib/database.cc
index 98e2c31..386b93a 100644
--- a/lib/database.cc
+++ b/lib/database.cc
@@ -539,10 +539,21 @@ parse_references (void *ctx,
 }
 
 notmuch_status_t
-notmuch_database_create (const char *path, notmuch_database_t **database)
+notmuch_database_new (notmuch_database_t **notmuch)
+{
+    /* Note: try to avoid error conditions! No error printing! */
+
+    *notmuch = talloc_zero (NULL, notmuch_database_t);
+    if (! *notmuch)
+	return NOTMUCH_STATUS_OUT_OF_MEMORY;
+
+    return NOTMUCH_STATUS_SUCCESS;
+}
+
+notmuch_status_t
+notmuch_database_create (notmuch_database_t *notmuch, const char *path)
 {
     notmuch_status_t status = NOTMUCH_STATUS_SUCCESS;
-    notmuch_database_t *notmuch = NULL;
     char *notmuch_path = NULL;
     struct stat st;
     int err;
@@ -579,25 +590,18 @@ notmuch_database_create (const char *path, notmuch_database_t **database)
 	goto DONE;
     }
 
-    status = notmuch_database_open (path,
-				    NOTMUCH_DATABASE_MODE_READ_WRITE,
-				    &notmuch);
+    status = notmuch_database_open (notmuch, path,
+				    NOTMUCH_DATABASE_MODE_READ_WRITE);
     if (status)
 	goto DONE;
     status = notmuch_database_upgrade (notmuch, NULL, NULL);
-    if (status) {
+    if (status)
 	notmuch_database_close(notmuch);
-	notmuch = NULL;
-    }
 
   DONE:
     if (notmuch_path)
 	talloc_free (notmuch_path);
 
-    if (database)
-	*database = notmuch;
-    else
-	talloc_free (notmuch);
     return status;
 }
 
@@ -612,14 +616,15 @@ _notmuch_database_ensure_writable (notmuch_database_t *notmuch)
     return NOTMUCH_STATUS_SUCCESS;
 }
 
+/*
+ * XXX: error handling should clean up *all* state created!
+ */
 notmuch_status_t
-notmuch_database_open (const char *path,
-		       notmuch_database_mode_t mode,
-		       notmuch_database_t **database)
+notmuch_database_open (notmuch_database_t *notmuch, const char *path,
+		       notmuch_database_mode_t mode)
 {
     notmuch_status_t status = NOTMUCH_STATUS_SUCCESS;
     void *local = talloc_new (NULL);
-    notmuch_database_t *notmuch = NULL;
     char *notmuch_path, *xapian_path;
     struct stat st;
     int err;
@@ -663,7 +668,6 @@ notmuch_database_open (const char *path,
 	initialized = 1;
     }
 
-    notmuch = talloc_zero (NULL, notmuch_database_t);
     notmuch->exception_reported = FALSE;
     notmuch->path = talloc_strdup (notmuch, path);
 
@@ -689,8 +693,7 @@ notmuch_database_open (const char *path,
 			 "       read-write mode.\n",
 			 notmuch_path, version, NOTMUCH_DATABASE_VERSION);
 		notmuch->mode = NOTMUCH_DATABASE_MODE_READ_ONLY;
-		notmuch_database_destroy (notmuch);
-		notmuch = NULL;
+		notmuch_database_close (notmuch);
 		status = NOTMUCH_STATUS_FILE_ERROR;
 		goto DONE;
 	    }
@@ -752,21 +755,19 @@ notmuch_database_open (const char *path,
     } catch (const Xapian::Error &error) {
 	fprintf (stderr, "A Xapian exception occurred opening database: %s\n",
 		 error.get_msg().c_str());
-	notmuch_database_destroy (notmuch);
-	notmuch = NULL;
+	notmuch_database_close (notmuch);
 	status = NOTMUCH_STATUS_XAPIAN_EXCEPTION;
     }
 
   DONE:
     talloc_free (local);
 
-    if (database)
-	*database = notmuch;
-    else
-	talloc_free (notmuch);
     return status;
 }
 
+/*
+ * XXX: close should clean up *all* state created by open/create!
+ */
 notmuch_status_t
 notmuch_database_close (notmuch_database_t *notmuch)
 {
@@ -869,7 +870,8 @@ public:
  * compaction process to protect data integrity.
  */
 notmuch_status_t
-notmuch_database_compact (const char *path,
+notmuch_database_compact (notmuch_database_t *notmuch,
+			  const char *path,
 			  const char *backup_path,
 			  notmuch_compact_status_cb_t status_cb,
 			  void *closure)
@@ -877,7 +879,6 @@ notmuch_database_compact (const char *path,
     void *local;
     char *notmuch_path, *xapian_path, *compact_xapian_path;
     notmuch_status_t ret = NOTMUCH_STATUS_SUCCESS;
-    notmuch_database_t *notmuch = NULL;
     struct stat statbuf;
     notmuch_bool_t keep_backup;
 
@@ -885,7 +886,8 @@ notmuch_database_compact (const char *path,
     if (! local)
 	return NOTMUCH_STATUS_OUT_OF_MEMORY;
 
-    ret = notmuch_database_open (path, NOTMUCH_DATABASE_MODE_READ_WRITE, &notmuch);
+    ret = notmuch_database_open (notmuch, path,
+				 NOTMUCH_DATABASE_MODE_READ_WRITE);
     if (ret) {
 	goto DONE;
     }
@@ -971,8 +973,9 @@ notmuch_database_compact (const char *path,
     }
 
   DONE:
+    /* XXX: error handling */
     if (notmuch)
-	ret = notmuch_database_destroy (notmuch);
+	ret = notmuch_database_close (notmuch);
 
     talloc_free (local);
 
@@ -980,7 +983,8 @@ notmuch_database_compact (const char *path,
 }
 #else
 notmuch_status_t
-notmuch_database_compact (unused (const char *path),
+notmuch_database_compact (unused (notmuch_database_t *notmuch),
+			  unused (const char *path),
 			  unused (const char *backup_path),
 			  unused (notmuch_compact_status_cb_t status_cb),
 			  unused (void *closure))
diff --git a/lib/notmuch.h b/lib/notmuch.h
index dbdce86..cd58d15 100644
--- a/lib/notmuch.h
+++ b/lib/notmuch.h
@@ -149,6 +149,28 @@ typedef struct _notmuch_tags notmuch_tags_t;
 typedef struct _notmuch_directory notmuch_directory_t;
 typedef struct _notmuch_filenames notmuch_filenames_t;
 
+/* Initialize a new, empty database handle.
+ *
+ * The database handle is required for creating, opening, and
+ * compacting a database. For further database operations, the
+ * database needs to be created or opened.
+ *
+ * After a successful call to notmuch_database_new, the returned
+ * database handle will remain in memory, so the caller should call
+ * notmuch_database_destroy when finished with the database handle.
+ *
+ * In case of any failure, this function returns an error status and
+ * sets *notmuch to NULL.
+ *
+ * Return value:
+ *
+ * NOTMUCH_STATUS_SUCCESS: Successfully created the database object.
+ *
+ * NOTMUCH_STATUS_OUT_OF_MEMORY: Out of memory.
+ */
+notmuch_status_t
+notmuch_database_new (notmuch_database_t **notmuch);
+
 /* Create a new, empty notmuch database located at 'path'.
  *
  * The path should be a top-level directory to a collection of
@@ -156,9 +178,9 @@ typedef struct _notmuch_filenames notmuch_filenames_t;
  * create a new ".notmuch" directory within 'path' where notmuch will
  * store its data.
  *
- * After a successful call to notmuch_database_create, the returned
- * database will be open so the caller should call
- * notmuch_database_destroy when finished with it.
+ * After a successful call to notmuch_database_create, the database
+ * will be open so the caller should call notmuch_database_close (or
+ * notmuch_database_destroy) when finished with the database.
  *
  * The database will not yet have any data in it
  * (notmuch_database_create itself is a very cheap function). Messages
@@ -166,7 +188,8 @@ typedef struct _notmuch_filenames notmuch_filenames_t;
  * notmuch_database_add_message.
  *
  * In case of any failure, this function returns an error status and
- * sets *database to NULL (after printing an error message on stderr).
+ * the database will be closed (after printing an error message on
+ * stderr).
  *
  * Return value:
  *
@@ -183,7 +206,7 @@ typedef struct _notmuch_filenames notmuch_filenames_t;
  * NOTMUCH_STATUS_XAPIAN_EXCEPTION: A Xapian exception occurred.
  */
 notmuch_status_t
-notmuch_database_create (const char *path, notmuch_database_t **database);
+notmuch_database_create (notmuch_database_t *notmuch, const char *path);
 
 typedef enum {
     NOTMUCH_DATABASE_MODE_READ_ONLY = 0,
@@ -201,11 +224,13 @@ typedef enum {
  * An existing notmuch database can be identified by the presence of a
  * directory named ".notmuch" below 'path'.
  *
- * The caller should call notmuch_database_destroy when finished with
- * this database.
+ * After a successful call to notmuch_database_open, the database will
+ * be open so the caller should call notmuch_database_close (or
+ * notmuch_database_destroy) when finished with the database.
  *
  * In case of any failure, this function returns an error status and
- * sets *database to NULL (after printing an error message on stderr).
+ * the database will be closed (after printing an error message on
+ * stderr).
  *
  * Return value:
  *
@@ -222,9 +247,8 @@ typedef enum {
  * NOTMUCH_STATUS_XAPIAN_EXCEPTION: A Xapian exception occurred.
  */
 notmuch_status_t
-notmuch_database_open (const char *path,
-		       notmuch_database_mode_t mode,
-		       notmuch_database_t **database);
+notmuch_database_open (notmuch_database_t *notmuch, const char *path,
+		       notmuch_database_mode_t mode);
 
 /* Close the given notmuch database.
  *
@@ -264,7 +288,8 @@ typedef void (*notmuch_compact_status_cb_t)(const char *message, void *closure);
  * 'closure' is passed verbatim to any callback invoked.
  */
 notmuch_status_t
-notmuch_database_compact (const char* path,
+notmuch_database_compact (notmuch_database_t *notmuch,
+			  const char* path,
 			  const char* backup_path,
 			  notmuch_compact_status_cb_t status_cb,
 			  void *closure);
@@ -272,6 +297,9 @@ notmuch_database_compact (const char* path,
 /* Destroy the notmuch database, closing it if necessary and freeing
  * all associated resources.
  *
+ * A database handle initialized with notmuch_database_new should be
+ * destroyed by calling notmuch_database_destroy.
+ *
  * Return value as in notmuch_database_close if the database was open;
  * notmuch_database_destroy itself has no failure modes.
  */
diff --git a/notmuch-compact.c b/notmuch-compact.c
index 8b820c0..626685e 100644
--- a/notmuch-compact.c
+++ b/notmuch-compact.c
@@ -29,6 +29,7 @@ status_update_cb (const char *msg, unused (void *closure))
 int
 notmuch_compact_command (notmuch_config_t *config, int argc, char *argv[])
 {
+    notmuch_database_t *notmuch = NULL;
     const char *path = notmuch_config_get_database_path (config);
     const char *backup_path = NULL;
     notmuch_status_t ret;
@@ -46,7 +47,13 @@ notmuch_compact_command (notmuch_config_t *config, int argc, char *argv[])
 
     if (! quiet)
 	printf ("Compacting database...\n");
-    ret = notmuch_database_compact (path, backup_path,
+
+    if (notmuch_database_new (&notmuch)) {
+	fprintf (stderr, "Out of memory\n");
+	return 1;
+    }
+
+    ret = notmuch_database_compact (notmuch, path, backup_path,
 				    quiet ? NULL : status_update_cb, NULL);
     if (ret) {
 	fprintf (stderr, "Compaction failed: %s\n", notmuch_status_to_string (ret));
@@ -60,5 +67,7 @@ notmuch_compact_command (notmuch_config_t *config, int argc, char *argv[])
 	printf ("Done.\n");
     }
 
+    notmuch_database_destroy (notmuch);
+
     return 0;
 }
diff --git a/notmuch-count.c b/notmuch-count.c
index 01e4e30..15c95c7 100644
--- a/notmuch-count.c
+++ b/notmuch-count.c
@@ -170,8 +170,14 @@ notmuch_count_command (notmuch_config_t *config, int argc, char *argv[])
 	return 1;
     }
 
-    if (notmuch_database_open (notmuch_config_get_database_path (config),
-			       NOTMUCH_DATABASE_MODE_READ_ONLY, &notmuch))
+    if (notmuch_database_new (&notmuch)) {
+	fprintf (stderr, "Out of memory\n");
+	return 1;
+    }
+
+    if (notmuch_database_open (notmuch,
+			       notmuch_config_get_database_path (config),
+			       NOTMUCH_DATABASE_MODE_READ_ONLY))
 	return 1;
 
     query_str = query_string_from_args (config, argc-opt_index, argv+opt_index);
diff --git a/notmuch-dump.c b/notmuch-dump.c
index 2024e30..73579bc 100644
--- a/notmuch-dump.c
+++ b/notmuch-dump.c
@@ -33,8 +33,14 @@ notmuch_dump_command (notmuch_config_t *config, int argc, char *argv[])
     notmuch_tags_t *tags;
     const char *query_str = "";
 
-    if (notmuch_database_open (notmuch_config_get_database_path (config),
-			       NOTMUCH_DATABASE_MODE_READ_ONLY, &notmuch))
+    if (notmuch_database_new (&notmuch)) {
+	fprintf (stderr, "Out of memory\n");
+	return 1;
+    }
+
+    if (notmuch_database_open (notmuch,
+			       notmuch_config_get_database_path (config),
+			       NOTMUCH_DATABASE_MODE_READ_ONLY))
 	return 1;
 
     char *output_file_name = NULL;
diff --git a/notmuch-insert.c b/notmuch-insert.c
index 2207b1e..4a8aad3 100644
--- a/notmuch-insert.c
+++ b/notmuch-insert.c
@@ -467,8 +467,14 @@ notmuch_insert_command (notmuch_config_t *config, int argc, char *argv[])
     action.sa_flags = 0;
     sigaction (SIGINT, &action, NULL);
 
-    if (notmuch_database_open (notmuch_config_get_database_path (config),
-			       NOTMUCH_DATABASE_MODE_READ_WRITE, &notmuch))
+    if (notmuch_database_new (&notmuch)) {
+	fprintf (stderr, "Out of memory\n");
+	return 1;
+    }
+
+    if (notmuch_database_open (notmuch,
+			       notmuch_config_get_database_path (config),
+			       NOTMUCH_DATABASE_MODE_READ_WRITE))
 	return 1;
 
     ret = insert_message (config, notmuch, STDIN_FILENO, maildir, tag_ops);
diff --git a/notmuch-new.c b/notmuch-new.c
index ba05cb4..f72a4de 100644
--- a/notmuch-new.c
+++ b/notmuch-new.c
@@ -914,6 +914,11 @@ notmuch_new_command (notmuch_config_t *config, int argc, char *argv[])
 	    return ret;
     }
 
+    if (notmuch_database_new (&notmuch)) {
+	fprintf (stderr, "Out of memory\n");
+	return 1;
+    }
+
     dot_notmuch_path = talloc_asprintf (config, "%s/%s", db_path, ".notmuch");
 
     if (stat (dot_notmuch_path, &st)) {
@@ -925,12 +930,12 @@ notmuch_new_command (notmuch_config_t *config, int argc, char *argv[])
 	    return 1;
 
 	printf ("Found %d total files (that's not much mail).\n", count);
-	if (notmuch_database_create (db_path, &notmuch))
+	if (notmuch_database_create (notmuch, db_path))
 	    return 1;
 	add_files_state.total_files = count;
     } else {
-	if (notmuch_database_open (db_path, NOTMUCH_DATABASE_MODE_READ_WRITE,
-				   &notmuch))
+	if (notmuch_database_open (notmuch, db_path,
+				   NOTMUCH_DATABASE_MODE_READ_WRITE))
 	    return 1;
 
 	if (notmuch_database_needs_upgrade (notmuch)) {
@@ -945,9 +950,6 @@ notmuch_new_command (notmuch_config_t *config, int argc, char *argv[])
 	add_files_state.total_files = 0;
     }
 
-    if (notmuch == NULL)
-	return 1;
-
     /* Setup our handler for SIGINT. We do this after having
      * potentially done a database upgrade we this interrupt handler
      * won't support. */
diff --git a/notmuch-reply.c b/notmuch-reply.c
index 9d6f843..7b80841 100644
--- a/notmuch-reply.c
+++ b/notmuch-reply.c
@@ -769,8 +769,14 @@ notmuch_reply_command (notmuch_config_t *config, int argc, char *argv[])
 	return 1;
     }
 
-    if (notmuch_database_open (notmuch_config_get_database_path (config),
-			       NOTMUCH_DATABASE_MODE_READ_ONLY, &notmuch))
+    if (notmuch_database_new (&notmuch)) {
+	fprintf (stderr, "Out of memory\n");
+	return 1;
+    }
+
+    if (notmuch_database_open (notmuch,
+			       notmuch_config_get_database_path (config),
+			       NOTMUCH_DATABASE_MODE_READ_ONLY))
 	return 1;
 
     query = notmuch_query_create (notmuch, query_string);
diff --git a/notmuch-restore.c b/notmuch-restore.c
index 1419621..fc37838 100644
--- a/notmuch-restore.c
+++ b/notmuch-restore.c
@@ -138,8 +138,14 @@ notmuch_restore_command (notmuch_config_t *config, int argc, char *argv[])
     int opt_index;
     int input_format = DUMP_FORMAT_AUTO;
 
-    if (notmuch_database_open (notmuch_config_get_database_path (config),
-			       NOTMUCH_DATABASE_MODE_READ_WRITE, &notmuch))
+    if (notmuch_database_new (&notmuch)) {
+	fprintf (stderr, "Out of memory\n");
+	return 1;
+    }
+
+    if (notmuch_database_open (notmuch,
+			       notmuch_config_get_database_path (config),
+			       NOTMUCH_DATABASE_MODE_READ_WRITE))
 	return 1;
 
     if (notmuch_config_get_maildir_synchronize_flags (config))
diff --git a/notmuch-search.c b/notmuch-search.c
index 7c973b3..50aace9 100644
--- a/notmuch-search.c
+++ b/notmuch-search.c
@@ -430,8 +430,14 @@ notmuch_search_command (notmuch_config_t *config, int argc, char *argv[])
 
     notmuch_exit_if_unsupported_format ();
 
-    if (notmuch_database_open (notmuch_config_get_database_path (config),
-			       NOTMUCH_DATABASE_MODE_READ_ONLY, &notmuch))
+    if (notmuch_database_new (&notmuch)) {
+	fprintf (stderr, "Out of memory\n");
+	return 1;
+    }
+
+    if (notmuch_database_open (notmuch,
+			       notmuch_config_get_database_path (config),
+			       NOTMUCH_DATABASE_MODE_READ_ONLY))
 	return 1;
 
     query_str = query_string_from_args (notmuch, argc-opt_index, argv+opt_index);
diff --git a/notmuch-show.c b/notmuch-show.c
index c07f887..bc44b2c 100644
--- a/notmuch-show.c
+++ b/notmuch-show.c
@@ -1201,8 +1201,14 @@ notmuch_show_command (notmuch_config_t *config, int argc, char *argv[])
 	return 1;
     }
 
-    if (notmuch_database_open (notmuch_config_get_database_path (config),
-			       NOTMUCH_DATABASE_MODE_READ_ONLY, &notmuch))
+    if (notmuch_database_new (&notmuch)) {
+	fprintf (stderr, "Out of memory\n");
+	return 1;
+    }
+
+    if (notmuch_database_open (notmuch,
+			       notmuch_config_get_database_path (config),
+			       NOTMUCH_DATABASE_MODE_READ_ONLY))
 	return 1;
 
     query = notmuch_query_create (notmuch, query_string);
diff --git a/notmuch-tag.c b/notmuch-tag.c
index 3b09df9..6e29799 100644
--- a/notmuch-tag.c
+++ b/notmuch-tag.c
@@ -254,8 +254,14 @@ notmuch_tag_command (notmuch_config_t *config, int argc, char *argv[])
 	}
     }
 
-    if (notmuch_database_open (notmuch_config_get_database_path (config),
-			       NOTMUCH_DATABASE_MODE_READ_WRITE, &notmuch))
+    if (notmuch_database_new (&notmuch)) {
+	fprintf (stderr, "Out of memory\n");
+	return 1;
+    }
+
+    if (notmuch_database_open (notmuch,
+			       notmuch_config_get_database_path (config),
+			       NOTMUCH_DATABASE_MODE_READ_WRITE))
 	return 1;
 
     if (notmuch_config_get_maildir_synchronize_flags (config))
diff --git a/test/random-corpus.c b/test/random-corpus.c
index 790193d..2b205e5 100644
--- a/test/random-corpus.c
+++ b/test/random-corpus.c
@@ -164,8 +164,14 @@ main (int argc, char **argv)
     if (config == NULL)
 	return 1;
 
-    if (notmuch_database_open (notmuch_config_get_database_path (config),
-			       NOTMUCH_DATABASE_MODE_READ_WRITE, &notmuch))
+    if (notmuch_database_new (&notmuch)) {
+	fprintf (stderr, "Out of memory\n");
+	return 1;
+    }
+
+    if (notmuch_database_open (notmuch,
+			       notmuch_config_get_database_path (config),
+			       NOTMUCH_DATABASE_MODE_READ_WRITE))
 	return 1;
 
     srandom (seed);
diff --git a/test/symbol-test.cc b/test/symbol-test.cc
index 3e96c03..47c5351 100644
--- a/test/symbol-test.cc
+++ b/test/symbol-test.cc
@@ -5,7 +5,8 @@
 
 int main() {
   notmuch_database_t *notmuch;
-  notmuch_database_open("fakedb", NOTMUCH_DATABASE_MODE_READ_ONLY, &notmuch);
+  notmuch_database_new (&notmuch);
+  notmuch_database_open (notmuch, "fakedb", NOTMUCH_DATABASE_MODE_READ_ONLY);
 
   try {
     (void) new Xapian::WritableDatabase("./nonexistant", Xapian::DB_OPEN);
-- 
1.8.4.2

Re: [PATCH 2/2] lib: introduce notmuch_database_new for initializing a database handle

[David Bremner]

The first patch looks ok. I like the new API overall.

As far as breaking contrib/notmuch-deliver, I'd rather fix
notmuch-insert than put effort into notmuch-deliver at this point. I
guess it could be a rough transition for people running notmuch-deliver
from git.

Jani Nikula <jani@nikula.org> writes:

> +/*
> + * XXX: error handling should clean up *all* state created!
> + */
is this a warning to future hackers or some current problem?

>  notmuch_status_t
> -notmuch_database_open (const char *path,
> -		       notmuch_database_mode_t mode,
> -		       notmuch_database_t **database)
> +notmuch_database_open (notmuch_database_t *notmuch, const char *path,
> +		       notmuch_database_mode_t mode)
>  
> +/* Initialize a new, empty database handle.
> + *

I wondered about making the new documentation adhere to doxygen?


> +    if (notmuch_database_open (notmuch,
> +			       notmuch_config_get_database_path (config),
> +			       NOTMUCH_DATABASE_MODE_READ_ONLY))

Would it make any sense to migrate the mode argument to an option
setting while we're messing with the API?  

Re: [PATCH 2/2] lib: introduce notmuch_database_new for initializing a database handle

[Tomi Ollila]

On Tue, Dec 03 2013, David Bremner <david@tethera.net> wrote:

> The first patch looks ok. I like the new API overall.
>
> As far as breaking contrib/notmuch-deliver, I'd rather fix
> notmuch-insert than put effort into notmuch-deliver at this point. I
> guess it could be a rough transition for people running notmuch-deliver
> from git.
>
> Jani Nikula <jani@nikula.org> writes:
>
>> +/*
>> + * XXX: error handling should clean up *all* state created!
>> + */
> is this a warning to future hackers or some current problem?
>
>>  notmuch_status_t
>> -notmuch_database_open (const char *path,
>> -		       notmuch_database_mode_t mode,
>> -		       notmuch_database_t **database)
>> +notmuch_database_open (notmuch_database_t *notmuch, const char *path,
>> +		       notmuch_database_mode_t mode)
>>  
>> +/* Initialize a new, empty database handle.
>> + *
>
> I wondered about making the new documentation adhere to doxygen?
>
>
>> +    if (notmuch_database_open (notmuch,
>> +			       notmuch_config_get_database_path (config),
>> +			       NOTMUCH_DATABASE_MODE_READ_ONLY))
>
> Would it make any sense to migrate the mode argument to an option
> setting while we're messing with the API?  

if that, then also database_path to options ?

I also like this suggestion best of all seen so far, but what if:

  #define NOTMUCH_MAJOR_VERSION	0
  #define NOTMUCH_MINOR_VERSION	17
 -#define NOTMUCH_MICRO_VERSION	0
 +#define NOTMUCH_MICRO_VERSION	90

until changed API/ABI is set into stone (i.e. 0.18.0 at freeze time).


Tomi

Re: [PATCH 2/2] lib: introduce notmuch_database_new for initializing a database handle

[Jani Nikula]

On Tue, 03 Dec 2013, David Bremner <david@tethera.net> wrote:
> The first patch looks ok. I like the new API overall.

Happy to hear that.

> As far as breaking contrib/notmuch-deliver, I'd rather fix
> notmuch-insert than put effort into notmuch-deliver at this point. I
> guess it could be a rough transition for people running notmuch-deliver
> from git.

Agreed. We could fix notmuch insert with patch 1/2 alone.

> Jani Nikula <jani@nikula.org> writes:
>
>> +/*
>> + * XXX: error handling should clean up *all* state created!
>> + */
> is this a warning to future hackers or some current problem?

It's a note to self and reviewers that we need to be sure this
happens... I'm just saying I'm not 100% sure we're doing all of that
now.

>>  notmuch_status_t
>> -notmuch_database_open (const char *path,
>> -		       notmuch_database_mode_t mode,
>> -		       notmuch_database_t **database)
>> +notmuch_database_open (notmuch_database_t *notmuch, const char *path,
>> +		       notmuch_database_mode_t mode)
>>  
>> +/* Initialize a new, empty database handle.
>> + *
>
> I wondered about making the new documentation adhere to doxygen?

I was thinking of fixing either series depending on them getting
merged. I wasn't sure we'd reached consensus on doxygen yet.

>> +    if (notmuch_database_open (notmuch,
>> +			       notmuch_config_get_database_path (config),
>> +			       NOTMUCH_DATABASE_MODE_READ_ONLY))
>
> Would it make any sense to migrate the mode argument to an option
> setting while we're messing with the API?  

My gut feeling says no. Same for the path argument suggested by Tomi.

What would it mean to change the mode while the database is open? Or the
path? I think we'd have to check for this, and fail. Mode is only
meaningful for open, and path for open, create and compact. I think
adding such state modifiers would make the interface feel more complex,
but I'm open to arguments to the contrary.


BR,
Jani.

Re: [PATCH 2/2] lib: introduce notmuch_database_new for initializing a database handle

[Jani Nikula]

On Tue, 03 Dec 2013, Tomi Ollila <tomi.ollila@iki.fi> wrote:
> On Tue, Dec 03 2013, David Bremner <david@tethera.net> wrote:
>
>> The first patch looks ok. I like the new API overall.
>>
>> As far as breaking contrib/notmuch-deliver, I'd rather fix
>> notmuch-insert than put effort into notmuch-deliver at this point. I
>> guess it could be a rough transition for people running notmuch-deliver
>> from git.
>>
>> Jani Nikula <jani@nikula.org> writes:
>>
>>> +/*
>>> + * XXX: error handling should clean up *all* state created!
>>> + */
>> is this a warning to future hackers or some current problem?
>>
>>>  notmuch_status_t
>>> -notmuch_database_open (const char *path,
>>> -		       notmuch_database_mode_t mode,
>>> -		       notmuch_database_t **database)
>>> +notmuch_database_open (notmuch_database_t *notmuch, const char *path,
>>> +		       notmuch_database_mode_t mode)
>>>  
>>> +/* Initialize a new, empty database handle.
>>> + *
>>
>> I wondered about making the new documentation adhere to doxygen?
>>
>>
>>> +    if (notmuch_database_open (notmuch,
>>> +			       notmuch_config_get_database_path (config),
>>> +			       NOTMUCH_DATABASE_MODE_READ_ONLY))
>>
>> Would it make any sense to migrate the mode argument to an option
>> setting while we're messing with the API?  
>
> if that, then also database_path to options ?

See my reply to David.

> I also like this suggestion best of all seen so far, but what if:
>
>   #define NOTMUCH_MAJOR_VERSION	0
>   #define NOTMUCH_MINOR_VERSION	17
>  -#define NOTMUCH_MICRO_VERSION	0
>  +#define NOTMUCH_MICRO_VERSION	90
>
> until changed API/ABI is set into stone (i.e. 0.18.0 at freeze time).

That would require the user to conditional build with:

#if NOTMUCH_CHECK_VERSION(0, 17, 90)
	/* building against post-0.17 git master or released 0.18 */
#else
	/* building against 0.17 */
#endif

instead of the IMHO more natural and accurate:

#if NOTMUCH_CHECK_VERSION(0, 18, 0)
	/* building against post-0.17 git master or released 0.18 */
#else
	/* building against 0.17 */
#endif


BR,
Jani.

Re: [PATCH 2/2] lib: introduce notmuch_database_new for initializing a database handle

[Tomi Ollila]

On Tue, Dec 03 2013, Jani Nikula <jani@nikula.org> wrote:

> On Tue, 03 Dec 2013, Tomi Ollila <tomi.ollila@iki.fi> wrote:
>> On Tue, Dec 03 2013, David Bremner <david@tethera.net> wrote:
>>
>>> The first patch looks ok. I like the new API overall.
>>>
>>> As far as breaking contrib/notmuch-deliver, I'd rather fix
>>> notmuch-insert than put effort into notmuch-deliver at this point. I
>>> guess it could be a rough transition for people running notmuch-deliver
>>> from git.
>>>
>>> Jani Nikula <jani@nikula.org> writes:
>>>
>>>> +/*
>>>> + * XXX: error handling should clean up *all* state created!
>>>> + */
>>> is this a warning to future hackers or some current problem?
>>>
>>>>  notmuch_status_t
>>>> -notmuch_database_open (const char *path,
>>>> -		       notmuch_database_mode_t mode,
>>>> -		       notmuch_database_t **database)
>>>> +notmuch_database_open (notmuch_database_t *notmuch, const char *path,
>>>> +		       notmuch_database_mode_t mode)
>>>>  
>>>> +/* Initialize a new, empty database handle.
>>>> + *
>>>
>>> I wondered about making the new documentation adhere to doxygen?
>>>
>>>
>>>> +    if (notmuch_database_open (notmuch,
>>>> +			       notmuch_config_get_database_path (config),
>>>> +			       NOTMUCH_DATABASE_MODE_READ_ONLY))
>>>
>>> Would it make any sense to migrate the mode argument to an option
>>> setting while we're messing with the API?  
>>
>> if that, then also database_path to options ?
>
> See my reply to David.

I agree with your explanation there. 

>
>> I also like this suggestion best of all seen so far, but what if:
>>
>>   #define NOTMUCH_MAJOR_VERSION	0
>>   #define NOTMUCH_MINOR_VERSION	17
>>  -#define NOTMUCH_MICRO_VERSION	0
>>  +#define NOTMUCH_MICRO_VERSION	90
>>
>> until changed API/ABI is set into stone (i.e. 0.18.0 at freeze time).
>
> That would require the user to conditional build with:
>
> #if NOTMUCH_CHECK_VERSION(0, 17, 90)
> 	/* building against post-0.17 git master or released 0.18 */
> #else
> 	/* building against 0.17 */
> #endif
>
> instead of the IMHO more natural and accurate:
>
> #if NOTMUCH_CHECK_VERSION(0, 18, 0)
> 	/* building against post-0.17 git master or released 0.18 */
> #else
> 	/* building against 0.17 */
> #endif

True -- I always forget that NOTMUCH_CHECK_VERSION() checks for the
value and *newer*. Although there is slight difference I don't think
it warrants the use of intermediate values -- the changing API is
much bigger issue than this and that is what we have to concentrate on.
>
>
> BR,
> Jani.

Tomi

Re: [PATCH 1/2] lib: add return status to database close and destroy

[Austin Clements]

Quoth Jani Nikula on Dec 01 at  3:13 pm:
> notmuch_database_close may fail in Xapian ->flush() or ->close(), so
> report the status. Similarly for notmuch_database_destroy which calls
> close.
> 
> This is required for notmuch insert to report error status if message
> indexing failed.
> 
> Bump the notmuch version to allow clients to conditional build against
> both the current release and the next release (current git master).
> ---
>  lib/database.cc | 18 ++++++++++++++----
>  lib/notmuch.h   | 17 ++++++++++++++---
>  2 files changed, 28 insertions(+), 7 deletions(-)
> 
> diff --git a/lib/database.cc b/lib/database.cc
> index f395061..98e2c31 100644
> --- a/lib/database.cc
> +++ b/lib/database.cc
> @@ -767,14 +767,17 @@ notmuch_database_open (const char *path,
>      return status;
>  }
>  
> -void
> +notmuch_status_t
>  notmuch_database_close (notmuch_database_t *notmuch)
>  {
> +    notmuch_status_t status = NOTMUCH_STATUS_SUCCESS;
> +
>      try {
>  	if (notmuch->xapian_db != NULL &&
>  	    notmuch->mode == NOTMUCH_DATABASE_MODE_READ_WRITE)
>  	    (static_cast <Xapian::WritableDatabase *> (notmuch->xapian_db))->flush ();
>      } catch (const Xapian::Error &error) {
> +	status = NOTMUCH_STATUS_XAPIAN_EXCEPTION;
>  	if (! notmuch->exception_reported) {
>  	    fprintf (stderr, "Error: A Xapian exception occurred flushing database: %s\n",
>  		     error.get_msg().c_str());
> @@ -789,6 +792,7 @@ notmuch_database_close (notmuch_database_t *notmuch)
>  	    notmuch->xapian_db->close();
>  	} catch (const Xapian::Error &error) {
>  	    /* do nothing */
> +	    status = NOTMUCH_STATUS_XAPIAN_EXCEPTION;
>  	}
>      }
>  
> @@ -802,6 +806,8 @@ notmuch_database_close (notmuch_database_t *notmuch)
>      notmuch->value_range_processor = NULL;
>      delete notmuch->date_range_processor;
>      notmuch->date_range_processor = NULL;
> +
> +    return status;
>  }
>  
>  #if HAVE_XAPIAN_COMPACT
> @@ -966,7 +972,7 @@ notmuch_database_compact (const char *path,
>  
>    DONE:
>      if (notmuch)
> -	notmuch_database_destroy (notmuch);
> +	ret = notmuch_database_destroy (notmuch);

This will clobber the error code on most of the error paths.  Maybe
you want to only set ret if it's currently NOTMUCH_STATUS_SUCCESS?

>  
>      talloc_free (local);
>  
> @@ -984,11 +990,15 @@ notmuch_database_compact (unused (const char *path),
>  }
>  #endif
>  
> -void
> +notmuch_status_t
>  notmuch_database_destroy (notmuch_database_t *notmuch)
>  {
> -    notmuch_database_close (notmuch);
> +    notmuch_status_t status;
> +
> +    status = notmuch_database_close (notmuch);
>      talloc_free (notmuch);
> +
> +    return status;
>  }
>  
>  const char *
> diff --git a/lib/notmuch.h b/lib/notmuch.h
> index 7c3a30c..dbdce86 100644
> --- a/lib/notmuch.h
> +++ b/lib/notmuch.h
> @@ -42,7 +42,7 @@ NOTMUCH_BEGIN_DECLS
>  #endif
>  
>  #define NOTMUCH_MAJOR_VERSION	0
> -#define NOTMUCH_MINOR_VERSION	17
> +#define NOTMUCH_MINOR_VERSION	18
>  #define NOTMUCH_MICRO_VERSION	0

This is actually what got me thinking about
id:1386173986-9624-1-git-send-email-amdragon@mit.edu (which obviously
conflicts).  In particular, the Python bindings don't have access to
these macros, and can only use the soname version.  (I think the Go
ones can use the macros; the Ruby ones certainly can.)

>  
>  /*
> @@ -236,8 +236,16 @@ notmuch_database_open (const char *path,
>   *
>   * notmuch_database_close can be called multiple times.  Later calls
>   * have no effect.
> + *
> + * Return value:
> + *
> + * NOTMUCH_STATUS_SUCCESS: Successfully closed the database.
> + *
> + * NOTMUCH_STATUS_XAPIAN_EXCEPTION: A Xapian exception occurred; the
> + *	database has been closed but there are no guarantees the
> + *	changes to the database, if any, have been flushed to disk.
>   */
> -void
> +notmuch_status_t
>  notmuch_database_close (notmuch_database_t *database);
>  
>  /* A callback invoked by notmuch_database_compact to notify the user
> @@ -263,8 +271,11 @@ notmuch_database_compact (const char* path,
>  
>  /* Destroy the notmuch database, closing it if necessary and freeing
>   * all associated resources.
> + *
> + * Return value as in notmuch_database_close if the database was open;
> + * notmuch_database_destroy itself has no failure modes.
>   */
> -void
> +notmuch_status_t
>  notmuch_database_destroy (notmuch_database_t *database);
>  
>  /* Return the database path of the given database.

Regarding bindings (since you asked me to think about them), these
should be a safe changes from an ABI perspective (though obviously the
bindings will need changes to take advantage of the new return code).

Re: [PATCH 1/2] lib: add return status to database close and destroy

[Jani Nikula]

On Wed, 04 Dec 2013, Austin Clements <amdragon@MIT.EDU> wrote:
> Quoth Jani Nikula on Dec 01 at  3:13 pm:
>> notmuch_database_close may fail in Xapian ->flush() or ->close(), so
>> report the status. Similarly for notmuch_database_destroy which calls
>> close.
>> 
>> This is required for notmuch insert to report error status if message
>> indexing failed.
>> 
>> Bump the notmuch version to allow clients to conditional build against
>> both the current release and the next release (current git master).
>> ---
>>  lib/database.cc | 18 ++++++++++++++----
>>  lib/notmuch.h   | 17 ++++++++++++++---
>>  2 files changed, 28 insertions(+), 7 deletions(-)
>> 
>> diff --git a/lib/database.cc b/lib/database.cc
>> index f395061..98e2c31 100644
>> --- a/lib/database.cc
>> +++ b/lib/database.cc
>> @@ -767,14 +767,17 @@ notmuch_database_open (const char *path,
>>      return status;
>>  }
>>  
>> -void
>> +notmuch_status_t
>>  notmuch_database_close (notmuch_database_t *notmuch)
>>  {
>> +    notmuch_status_t status = NOTMUCH_STATUS_SUCCESS;
>> +
>>      try {
>>  	if (notmuch->xapian_db != NULL &&
>>  	    notmuch->mode == NOTMUCH_DATABASE_MODE_READ_WRITE)
>>  	    (static_cast <Xapian::WritableDatabase *> (notmuch->xapian_db))->flush ();
>>      } catch (const Xapian::Error &error) {
>> +	status = NOTMUCH_STATUS_XAPIAN_EXCEPTION;
>>  	if (! notmuch->exception_reported) {
>>  	    fprintf (stderr, "Error: A Xapian exception occurred flushing database: %s\n",
>>  		     error.get_msg().c_str());
>> @@ -789,6 +792,7 @@ notmuch_database_close (notmuch_database_t *notmuch)
>>  	    notmuch->xapian_db->close();
>>  	} catch (const Xapian::Error &error) {
>>  	    /* do nothing */
>> +	    status = NOTMUCH_STATUS_XAPIAN_EXCEPTION;
>>  	}
>>      }
>>  
>> @@ -802,6 +806,8 @@ notmuch_database_close (notmuch_database_t *notmuch)
>>      notmuch->value_range_processor = NULL;
>>      delete notmuch->date_range_processor;
>>      notmuch->date_range_processor = NULL;
>> +
>> +    return status;
>>  }
>>  
>>  #if HAVE_XAPIAN_COMPACT
>> @@ -966,7 +972,7 @@ notmuch_database_compact (const char *path,
>>  
>>    DONE:
>>      if (notmuch)
>> -	notmuch_database_destroy (notmuch);
>> +	ret = notmuch_database_destroy (notmuch);
>
> This will clobber the error code on most of the error paths.  Maybe
> you want to only set ret if it's currently NOTMUCH_STATUS_SUCCESS?

Good point, will fix.

BR,
Jani.


>
>>  
>>      talloc_free (local);
>>  
>> @@ -984,11 +990,15 @@ notmuch_database_compact (unused (const char *path),
>>  }
>>  #endif
>>  
>> -void
>> +notmuch_status_t
>>  notmuch_database_destroy (notmuch_database_t *notmuch)
>>  {
>> -    notmuch_database_close (notmuch);
>> +    notmuch_status_t status;
>> +
>> +    status = notmuch_database_close (notmuch);
>>      talloc_free (notmuch);
>> +
>> +    return status;
>>  }
>>  
>>  const char *
>> diff --git a/lib/notmuch.h b/lib/notmuch.h
>> index 7c3a30c..dbdce86 100644
>> --- a/lib/notmuch.h
>> +++ b/lib/notmuch.h
>> @@ -42,7 +42,7 @@ NOTMUCH_BEGIN_DECLS
>>  #endif
>>  
>>  #define NOTMUCH_MAJOR_VERSION	0
>> -#define NOTMUCH_MINOR_VERSION	17
>> +#define NOTMUCH_MINOR_VERSION	18
>>  #define NOTMUCH_MICRO_VERSION	0
>
> This is actually what got me thinking about
> id:1386173986-9624-1-git-send-email-amdragon@mit.edu (which obviously
> conflicts).  In particular, the Python bindings don't have access to
> these macros, and can only use the soname version.  (I think the Go
> ones can use the macros; the Ruby ones certainly can.)
>
>>  
>>  /*
>> @@ -236,8 +236,16 @@ notmuch_database_open (const char *path,
>>   *
>>   * notmuch_database_close can be called multiple times.  Later calls
>>   * have no effect.
>> + *
>> + * Return value:
>> + *
>> + * NOTMUCH_STATUS_SUCCESS: Successfully closed the database.
>> + *
>> + * NOTMUCH_STATUS_XAPIAN_EXCEPTION: A Xapian exception occurred; the
>> + *	database has been closed but there are no guarantees the
>> + *	changes to the database, if any, have been flushed to disk.
>>   */
>> -void
>> +notmuch_status_t
>>  notmuch_database_close (notmuch_database_t *database);
>>  
>>  /* A callback invoked by notmuch_database_compact to notify the user
>> @@ -263,8 +271,11 @@ notmuch_database_compact (const char* path,
>>  
>>  /* Destroy the notmuch database, closing it if necessary and freeing
>>   * all associated resources.
>> + *
>> + * Return value as in notmuch_database_close if the database was open;
>> + * notmuch_database_destroy itself has no failure modes.
>>   */
>> -void
>> +notmuch_status_t
>>  notmuch_database_destroy (notmuch_database_t *database);
>>  
>>  /* Return the database path of the given database.
>
> Regarding bindings (since you asked me to think about them), these
> should be a safe changes from an ABI perspective (though obviously the
> bindings will need changes to take advantage of the new return code).

Re: [PATCH 2/2] lib: introduce notmuch_database_new for initializing a database handle

[Austin Clements]

Quoth Jani Nikula on Dec 01 at  3:14 pm:
> There is a need for setting options before opening a database, such as
> setting a logging function to use instead of writing to stdout or
> stderr. It would be possible to do this by adding new parameters to
> notmuch_database_create and notmuch_database_open, but maintaining a
> backwards compatible API and ABI when new options are added becomes
> burdensome.
> 
> Instead, split the opaque database object creation from
> notmuch_database_create and notmuch_database_open into a new
> notmuch_database_new call, to allow operations on the handle before
> create and open. This creates API and ABI breakage now, but allows
> easier future extensions.
> 
> The notmuch_database_new call becomes a natural pair to the already
> existing notmuch_database_destroy, and it should be possible to call
> open/close multiple times using an initialized handle.

A high-level comment about the API: Currently, an allocated
notmuch_database_t has two "memory states", if you will: open and
closed.  (I wish it didn't have any memory states, and was on the
fence about this API for a while until I realized that the ship had
already sailed.)  It's pretty clear how all of the notmuch APIs will
behave in both states (modulo some bounded non-determinism in the
closed state).  I think this patch introduces a new "pre-open" state,
and I don't know how most of the notmuch APIs behave in that state.
My guess is poorly.  If it's feasible, I'd much rather a fresh baked
notmuch_database_t act like it's in the closed state, including that
notmuch_database_{create,open} are well-defined as transitions from
closed state to open state (even if the closed state was reached by
calling notmuch_database_close).  Or, if we do have a "pre-open"
state, it should at least be well-specified what that means
(preferably the specification is *not* "most APIs segfault").

Orthogonally -- and this may be a complete pipe dream of mine -- if we
just had a way to return more detailed error information than a simple
error code from notmuch_database_{create,open}, I think we wouldn't
need any of this.  Everything that these functions currently log
(modulo one warning) is error details, so if we could return the error
details *with the error* or somehow make them accessible, we wouldn't
need a logger at this point (or at several other points in the
library).

> ---
>  lib/database.cc      | 64 ++++++++++++++++++++++++++++------------------------
>  lib/notmuch.h        | 52 ++++++++++++++++++++++++++++++++----------
>  notmuch-compact.c    | 11 ++++++++-
>  notmuch-count.c      | 10 ++++++--
>  notmuch-dump.c       | 10 ++++++--
>  notmuch-insert.c     | 10 ++++++--
>  notmuch-new.c        | 14 +++++++-----
>  notmuch-reply.c      | 10 ++++++--
>  notmuch-restore.c    | 10 ++++++--
>  notmuch-search.c     | 10 ++++++--
>  notmuch-show.c       | 10 ++++++--
>  notmuch-tag.c        | 10 ++++++--
>  test/random-corpus.c | 10 ++++++--
>  test/symbol-test.cc  |  3 ++-
>  14 files changed, 166 insertions(+), 68 deletions(-)
> 
> diff --git a/lib/database.cc b/lib/database.cc
> index 98e2c31..386b93a 100644
> --- a/lib/database.cc
> +++ b/lib/database.cc
> @@ -539,10 +539,21 @@ parse_references (void *ctx,
>  }
>  
>  notmuch_status_t
> -notmuch_database_create (const char *path, notmuch_database_t **database)
> +notmuch_database_new (notmuch_database_t **notmuch)

The naming of this is unfortunate...  Other APIs use x_create to
allocate objects (e.g., notmuch_query_create, several internal APIs).
I would lean towards calling this function notmuch_database_create,
but that leaves the question of what to call the other.  While we're
breaking APIs, would it be completely crazy to merge open and create
into one API with an extra mode to indicate creation (it can be its
own mode because creation implies read/write)?  (Or, in UNIX
tradition, we could call this function notmuch_database_create and the
other notmuch_database_creat.)  notmuch_database_create is already
just a shell around notmuch_database_open (we could keep it as a
separate function, but just make it internal).

> +{
> +    /* Note: try to avoid error conditions! No error printing! */
> +
> +    *notmuch = talloc_zero (NULL, notmuch_database_t);
> +    if (! *notmuch)
> +	return NOTMUCH_STATUS_OUT_OF_MEMORY;
> +
> +    return NOTMUCH_STATUS_SUCCESS;
> +}
> +
> +notmuch_status_t
> +notmuch_database_create (notmuch_database_t *notmuch, const char *path)
>  {

This should fail if passed a database that is already open.

>      notmuch_status_t status = NOTMUCH_STATUS_SUCCESS;
> -    notmuch_database_t *notmuch = NULL;
>      char *notmuch_path = NULL;
>      struct stat st;
>      int err;
> @@ -579,25 +590,18 @@ notmuch_database_create (const char *path, notmuch_database_t **database)
>  	goto DONE;
>      }
>  
> -    status = notmuch_database_open (path,
> -				    NOTMUCH_DATABASE_MODE_READ_WRITE,
> -				    &notmuch);
> +    status = notmuch_database_open (notmuch, path,
> +				    NOTMUCH_DATABASE_MODE_READ_WRITE);
>      if (status)
>  	goto DONE;
>      status = notmuch_database_upgrade (notmuch, NULL, NULL);
> -    if (status) {
> +    if (status)
>  	notmuch_database_close(notmuch);
> -	notmuch = NULL;
> -    }
>  
>    DONE:
>      if (notmuch_path)
>  	talloc_free (notmuch_path);
>  
> -    if (database)
> -	*database = notmuch;
> -    else
> -	talloc_free (notmuch);
>      return status;
>  }
>  
> @@ -612,14 +616,15 @@ _notmuch_database_ensure_writable (notmuch_database_t *notmuch)
>      return NOTMUCH_STATUS_SUCCESS;
>  }
>  
> +/*
> + * XXX: error handling should clean up *all* state created!
> + */

I think the only thing that will currently leak from this in an error
case is notmuch->path.

>  notmuch_status_t
> -notmuch_database_open (const char *path,
> -		       notmuch_database_mode_t mode,
> -		       notmuch_database_t **database)
> +notmuch_database_open (notmuch_database_t *notmuch, const char *path,
> +		       notmuch_database_mode_t mode)
>  {

This should also fail if passed a database that is already open.

>      notmuch_status_t status = NOTMUCH_STATUS_SUCCESS;
>      void *local = talloc_new (NULL);
> -    notmuch_database_t *notmuch = NULL;
>      char *notmuch_path, *xapian_path;
>      struct stat st;
>      int err;
> @@ -663,7 +668,6 @@ notmuch_database_open (const char *path,
>  	initialized = 1;
>      }
>  
> -    notmuch = talloc_zero (NULL, notmuch_database_t);
>      notmuch->exception_reported = FALSE;
>      notmuch->path = talloc_strdup (notmuch, path);
>  
> @@ -689,8 +693,7 @@ notmuch_database_open (const char *path,
>  			 "       read-write mode.\n",
>  			 notmuch_path, version, NOTMUCH_DATABASE_VERSION);
>  		notmuch->mode = NOTMUCH_DATABASE_MODE_READ_ONLY;
> -		notmuch_database_destroy (notmuch);
> -		notmuch = NULL;
> +		notmuch_database_close (notmuch);
>  		status = NOTMUCH_STATUS_FILE_ERROR;
>  		goto DONE;
>  	    }
> @@ -752,21 +755,19 @@ notmuch_database_open (const char *path,
>      } catch (const Xapian::Error &error) {
>  	fprintf (stderr, "A Xapian exception occurred opening database: %s\n",
>  		 error.get_msg().c_str());
> -	notmuch_database_destroy (notmuch);
> -	notmuch = NULL;
> +	notmuch_database_close (notmuch);
>  	status = NOTMUCH_STATUS_XAPIAN_EXCEPTION;
>      }
>  
>    DONE:
>      talloc_free (local);
>  

It might be simpler to call notmuch_database_close here if status !=
NOTMUCH_STATUS_SUCCESS, rather than calling it at several places above
(and not on all error paths).

> -    if (database)
> -	*database = notmuch;
> -    else
> -	talloc_free (notmuch);
>      return status;
>  }
>  
> +/*
> + * XXX: close should clean up *all* state created by open/create!
> + */

I believe the only thing it doesn't clean up is path.  (Note that
cleaning up path here doesn't currently negate the need to clean up
path above, though if you float the close call to the DONE path, it
would suffice.)

>  notmuch_status_t
>  notmuch_database_close (notmuch_database_t *notmuch)
>  {
> @@ -869,7 +870,8 @@ public:
>   * compaction process to protect data integrity.
>   */
>  notmuch_status_t
> -notmuch_database_compact (const char *path,
> +notmuch_database_compact (notmuch_database_t *notmuch,
> +			  const char *path,
>  			  const char *backup_path,
>  			  notmuch_compact_status_cb_t status_cb,
>  			  void *closure)
> @@ -877,7 +879,6 @@ notmuch_database_compact (const char *path,
>      void *local;
>      char *notmuch_path, *xapian_path, *compact_xapian_path;
>      notmuch_status_t ret = NOTMUCH_STATUS_SUCCESS;
> -    notmuch_database_t *notmuch = NULL;
>      struct stat statbuf;
>      notmuch_bool_t keep_backup;
>  
> @@ -885,7 +886,8 @@ notmuch_database_compact (const char *path,
>      if (! local)
>  	return NOTMUCH_STATUS_OUT_OF_MEMORY;
>  
> -    ret = notmuch_database_open (path, NOTMUCH_DATABASE_MODE_READ_WRITE, &notmuch);
> +    ret = notmuch_database_open (notmuch, path,
> +				 NOTMUCH_DATABASE_MODE_READ_WRITE);
>      if (ret) {
>  	goto DONE;
>      }
> @@ -971,8 +973,9 @@ notmuch_database_compact (const char *path,
>      }
>  
>    DONE:
> +    /* XXX: error handling */
>      if (notmuch)
> -	ret = notmuch_database_destroy (notmuch);
> +	ret = notmuch_database_close (notmuch);
>  
>      talloc_free (local);
>  
> @@ -980,7 +983,8 @@ notmuch_database_compact (const char *path,
>  }
>  #else
>  notmuch_status_t
> -notmuch_database_compact (unused (const char *path),
> +notmuch_database_compact (unused (notmuch_database_t *notmuch),
> +			  unused (const char *path),
>  			  unused (const char *backup_path),
>  			  unused (notmuch_compact_status_cb_t status_cb),
>  			  unused (void *closure))
> diff --git a/lib/notmuch.h b/lib/notmuch.h
> index dbdce86..cd58d15 100644
> --- a/lib/notmuch.h
> +++ b/lib/notmuch.h
> @@ -149,6 +149,28 @@ typedef struct _notmuch_tags notmuch_tags_t;
>  typedef struct _notmuch_directory notmuch_directory_t;
>  typedef struct _notmuch_filenames notmuch_filenames_t;
>  
> +/* Initialize a new, empty database handle.
> + *
> + * The database handle is required for creating, opening, and
> + * compacting a database. For further database operations, the
> + * database needs to be created or opened.
> + *
> + * After a successful call to notmuch_database_new, the returned
> + * database handle will remain in memory, so the caller should call
> + * notmuch_database_destroy when finished with the database handle.
> + *
> + * In case of any failure, this function returns an error status and
> + * sets *notmuch to NULL.
> + *
> + * Return value:
> + *
> + * NOTMUCH_STATUS_SUCCESS: Successfully created the database object.
> + *
> + * NOTMUCH_STATUS_OUT_OF_MEMORY: Out of memory.
> + */
> +notmuch_status_t
> +notmuch_database_new (notmuch_database_t **notmuch);
> +
>  /* Create a new, empty notmuch database located at 'path'.
>   *
>   * The path should be a top-level directory to a collection of
> @@ -156,9 +178,9 @@ typedef struct _notmuch_filenames notmuch_filenames_t;
>   * create a new ".notmuch" directory within 'path' where notmuch will
>   * store its data.
>   *
> - * After a successful call to notmuch_database_create, the returned
> - * database will be open so the caller should call
> - * notmuch_database_destroy when finished with it.
> + * After a successful call to notmuch_database_create, the database
> + * will be open so the caller should call notmuch_database_close (or
> + * notmuch_database_destroy) when finished with the database.
>   *
>   * The database will not yet have any data in it
>   * (notmuch_database_create itself is a very cheap function). Messages
> @@ -166,7 +188,8 @@ typedef struct _notmuch_filenames notmuch_filenames_t;
>   * notmuch_database_add_message.
>   *
>   * In case of any failure, this function returns an error status and
> - * sets *database to NULL (after printing an error message on stderr).
> + * the database will be closed (after printing an error message on
> + * stderr).
>   *
>   * Return value:
>   *
> @@ -183,7 +206,7 @@ typedef struct _notmuch_filenames notmuch_filenames_t;
>   * NOTMUCH_STATUS_XAPIAN_EXCEPTION: A Xapian exception occurred.
>   */
>  notmuch_status_t
> -notmuch_database_create (const char *path, notmuch_database_t **database);
> +notmuch_database_create (notmuch_database_t *notmuch, const char *path);
>  
>  typedef enum {
>      NOTMUCH_DATABASE_MODE_READ_ONLY = 0,
> @@ -201,11 +224,13 @@ typedef enum {
>   * An existing notmuch database can be identified by the presence of a
>   * directory named ".notmuch" below 'path'.
>   *
> - * The caller should call notmuch_database_destroy when finished with
> - * this database.
> + * After a successful call to notmuch_database_open, the database will
> + * be open so the caller should call notmuch_database_close (or
> + * notmuch_database_destroy) when finished with the database.
>   *
>   * In case of any failure, this function returns an error status and
> - * sets *database to NULL (after printing an error message on stderr).
> + * the database will be closed (after printing an error message on
> + * stderr).
>   *
>   * Return value:
>   *
> @@ -222,9 +247,8 @@ typedef enum {
>   * NOTMUCH_STATUS_XAPIAN_EXCEPTION: A Xapian exception occurred.
>   */
>  notmuch_status_t
> -notmuch_database_open (const char *path,
> -		       notmuch_database_mode_t mode,
> -		       notmuch_database_t **database);
> +notmuch_database_open (notmuch_database_t *notmuch, const char *path,
> +		       notmuch_database_mode_t mode);
>  
>  /* Close the given notmuch database.
>   *
> @@ -264,7 +288,8 @@ typedef void (*notmuch_compact_status_cb_t)(const char *message, void *closure);
>   * 'closure' is passed verbatim to any callback invoked.
>   */
>  notmuch_status_t
> -notmuch_database_compact (const char* path,
> +notmuch_database_compact (notmuch_database_t *notmuch,
> +			  const char* path,
>  			  const char* backup_path,
>  			  notmuch_compact_status_cb_t status_cb,
>  			  void *closure);
> @@ -272,6 +297,9 @@ notmuch_database_compact (const char* path,
>  /* Destroy the notmuch database, closing it if necessary and freeing
>   * all associated resources.
>   *
> + * A database handle initialized with notmuch_database_new should be
> + * destroyed by calling notmuch_database_destroy.
> + *
>   * Return value as in notmuch_database_close if the database was open;
>   * notmuch_database_destroy itself has no failure modes.
>   */
> diff --git a/notmuch-compact.c b/notmuch-compact.c
> index 8b820c0..626685e 100644
> --- a/notmuch-compact.c
> +++ b/notmuch-compact.c
> @@ -29,6 +29,7 @@ status_update_cb (const char *msg, unused (void *closure))
>  int
>  notmuch_compact_command (notmuch_config_t *config, int argc, char *argv[])
>  {
> +    notmuch_database_t *notmuch = NULL;
>      const char *path = notmuch_config_get_database_path (config);
>      const char *backup_path = NULL;
>      notmuch_status_t ret;
> @@ -46,7 +47,13 @@ notmuch_compact_command (notmuch_config_t *config, int argc, char *argv[])
>  
>      if (! quiet)
>  	printf ("Compacting database...\n");
> -    ret = notmuch_database_compact (path, backup_path,
> +
> +    if (notmuch_database_new (&notmuch)) {
> +	fprintf (stderr, "Out of memory\n");
> +	return 1;
> +    }
> +
> +    ret = notmuch_database_compact (notmuch, path, backup_path,
>  				    quiet ? NULL : status_update_cb, NULL);
>      if (ret) {
>  	fprintf (stderr, "Compaction failed: %s\n", notmuch_status_to_string (ret));
> @@ -60,5 +67,7 @@ notmuch_compact_command (notmuch_config_t *config, int argc, char *argv[])
>  	printf ("Done.\n");
>      }
>  
> +    notmuch_database_destroy (notmuch);
> +
>      return 0;
>  }
> diff --git a/notmuch-count.c b/notmuch-count.c
> index 01e4e30..15c95c7 100644
> --- a/notmuch-count.c
> +++ b/notmuch-count.c
> @@ -170,8 +170,14 @@ notmuch_count_command (notmuch_config_t *config, int argc, char *argv[])
>  	return 1;
>      }
>  
> -    if (notmuch_database_open (notmuch_config_get_database_path (config),
> -			       NOTMUCH_DATABASE_MODE_READ_ONLY, &notmuch))
> +    if (notmuch_database_new (&notmuch)) {
> +	fprintf (stderr, "Out of memory\n");
> +	return 1;
> +    }
> +
> +    if (notmuch_database_open (notmuch,
> +			       notmuch_config_get_database_path (config),
> +			       NOTMUCH_DATABASE_MODE_READ_ONLY))

Does this need to destroy the database?  (Likewise for all the others.)

>  	return 1;
>  
>      query_str = query_string_from_args (config, argc-opt_index, argv+opt_index);
> diff --git a/notmuch-dump.c b/notmuch-dump.c
> index 2024e30..73579bc 100644
> --- a/notmuch-dump.c
> +++ b/notmuch-dump.c
> @@ -33,8 +33,14 @@ notmuch_dump_command (notmuch_config_t *config, int argc, char *argv[])
>      notmuch_tags_t *tags;
>      const char *query_str = "";
>  
> -    if (notmuch_database_open (notmuch_config_get_database_path (config),
> -			       NOTMUCH_DATABASE_MODE_READ_ONLY, &notmuch))
> +    if (notmuch_database_new (&notmuch)) {
> +	fprintf (stderr, "Out of memory\n");
> +	return 1;
> +    }
> +
> +    if (notmuch_database_open (notmuch,
> +			       notmuch_config_get_database_path (config),
> +			       NOTMUCH_DATABASE_MODE_READ_ONLY))
>  	return 1;
>  
>      char *output_file_name = NULL;
> diff --git a/notmuch-insert.c b/notmuch-insert.c
> index 2207b1e..4a8aad3 100644
> --- a/notmuch-insert.c
> +++ b/notmuch-insert.c
> @@ -467,8 +467,14 @@ notmuch_insert_command (notmuch_config_t *config, int argc, char *argv[])
>      action.sa_flags = 0;
>      sigaction (SIGINT, &action, NULL);
>  
> -    if (notmuch_database_open (notmuch_config_get_database_path (config),
> -			       NOTMUCH_DATABASE_MODE_READ_WRITE, &notmuch))
> +    if (notmuch_database_new (&notmuch)) {
> +	fprintf (stderr, "Out of memory\n");
> +	return 1;
> +    }
> +
> +    if (notmuch_database_open (notmuch,
> +			       notmuch_config_get_database_path (config),
> +			       NOTMUCH_DATABASE_MODE_READ_WRITE))
>  	return 1;
>  
>      ret = insert_message (config, notmuch, STDIN_FILENO, maildir, tag_ops);
> diff --git a/notmuch-new.c b/notmuch-new.c
> index ba05cb4..f72a4de 100644
> --- a/notmuch-new.c
> +++ b/notmuch-new.c
> @@ -914,6 +914,11 @@ notmuch_new_command (notmuch_config_t *config, int argc, char *argv[])
>  	    return ret;
>      }
>  
> +    if (notmuch_database_new (&notmuch)) {
> +	fprintf (stderr, "Out of memory\n");
> +	return 1;
> +    }
> +
>      dot_notmuch_path = talloc_asprintf (config, "%s/%s", db_path, ".notmuch");
>  
>      if (stat (dot_notmuch_path, &st)) {
> @@ -925,12 +930,12 @@ notmuch_new_command (notmuch_config_t *config, int argc, char *argv[])
>  	    return 1;
>  
>  	printf ("Found %d total files (that's not much mail).\n", count);
> -	if (notmuch_database_create (db_path, &notmuch))
> +	if (notmuch_database_create (notmuch, db_path))
>  	    return 1;
>  	add_files_state.total_files = count;
>      } else {
> -	if (notmuch_database_open (db_path, NOTMUCH_DATABASE_MODE_READ_WRITE,
> -				   &notmuch))
> +	if (notmuch_database_open (notmuch, db_path,
> +				   NOTMUCH_DATABASE_MODE_READ_WRITE))
>  	    return 1;
>  
>  	if (notmuch_database_needs_upgrade (notmuch)) {
> @@ -945,9 +950,6 @@ notmuch_new_command (notmuch_config_t *config, int argc, char *argv[])
>  	add_files_state.total_files = 0;
>      }
>  
> -    if (notmuch == NULL)
> -	return 1;
> -
>      /* Setup our handler for SIGINT. We do this after having
>       * potentially done a database upgrade we this interrupt handler
>       * won't support. */
> diff --git a/notmuch-reply.c b/notmuch-reply.c
> index 9d6f843..7b80841 100644
> --- a/notmuch-reply.c
> +++ b/notmuch-reply.c
> @@ -769,8 +769,14 @@ notmuch_reply_command (notmuch_config_t *config, int argc, char *argv[])
>  	return 1;
>      }
>  
> -    if (notmuch_database_open (notmuch_config_get_database_path (config),
> -			       NOTMUCH_DATABASE_MODE_READ_ONLY, &notmuch))
> +    if (notmuch_database_new (&notmuch)) {
> +	fprintf (stderr, "Out of memory\n");
> +	return 1;
> +    }
> +
> +    if (notmuch_database_open (notmuch,
> +			       notmuch_config_get_database_path (config),
> +			       NOTMUCH_DATABASE_MODE_READ_ONLY))
>  	return 1;
>  
>      query = notmuch_query_create (notmuch, query_string);
> diff --git a/notmuch-restore.c b/notmuch-restore.c
> index 1419621..fc37838 100644
> --- a/notmuch-restore.c
> +++ b/notmuch-restore.c
> @@ -138,8 +138,14 @@ notmuch_restore_command (notmuch_config_t *config, int argc, char *argv[])
>      int opt_index;
>      int input_format = DUMP_FORMAT_AUTO;
>  
> -    if (notmuch_database_open (notmuch_config_get_database_path (config),
> -			       NOTMUCH_DATABASE_MODE_READ_WRITE, &notmuch))
> +    if (notmuch_database_new (&notmuch)) {
> +	fprintf (stderr, "Out of memory\n");
> +	return 1;
> +    }
> +
> +    if (notmuch_database_open (notmuch,
> +			       notmuch_config_get_database_path (config),
> +			       NOTMUCH_DATABASE_MODE_READ_WRITE))
>  	return 1;
>  
>      if (notmuch_config_get_maildir_synchronize_flags (config))
> diff --git a/notmuch-search.c b/notmuch-search.c
> index 7c973b3..50aace9 100644
> --- a/notmuch-search.c
> +++ b/notmuch-search.c
> @@ -430,8 +430,14 @@ notmuch_search_command (notmuch_config_t *config, int argc, char *argv[])
>  
>      notmuch_exit_if_unsupported_format ();
>  
> -    if (notmuch_database_open (notmuch_config_get_database_path (config),
> -			       NOTMUCH_DATABASE_MODE_READ_ONLY, &notmuch))
> +    if (notmuch_database_new (&notmuch)) {
> +	fprintf (stderr, "Out of memory\n");
> +	return 1;
> +    }
> +
> +    if (notmuch_database_open (notmuch,
> +			       notmuch_config_get_database_path (config),
> +			       NOTMUCH_DATABASE_MODE_READ_ONLY))
>  	return 1;
>  
>      query_str = query_string_from_args (notmuch, argc-opt_index, argv+opt_index);
> diff --git a/notmuch-show.c b/notmuch-show.c
> index c07f887..bc44b2c 100644
> --- a/notmuch-show.c
> +++ b/notmuch-show.c
> @@ -1201,8 +1201,14 @@ notmuch_show_command (notmuch_config_t *config, int argc, char *argv[])
>  	return 1;
>      }
>  
> -    if (notmuch_database_open (notmuch_config_get_database_path (config),
> -			       NOTMUCH_DATABASE_MODE_READ_ONLY, &notmuch))
> +    if (notmuch_database_new (&notmuch)) {
> +	fprintf (stderr, "Out of memory\n");
> +	return 1;
> +    }
> +
> +    if (notmuch_database_open (notmuch,
> +			       notmuch_config_get_database_path (config),
> +			       NOTMUCH_DATABASE_MODE_READ_ONLY))
>  	return 1;
>  
>      query = notmuch_query_create (notmuch, query_string);
> diff --git a/notmuch-tag.c b/notmuch-tag.c
> index 3b09df9..6e29799 100644
> --- a/notmuch-tag.c
> +++ b/notmuch-tag.c
> @@ -254,8 +254,14 @@ notmuch_tag_command (notmuch_config_t *config, int argc, char *argv[])
>  	}
>      }
>  
> -    if (notmuch_database_open (notmuch_config_get_database_path (config),
> -			       NOTMUCH_DATABASE_MODE_READ_WRITE, &notmuch))
> +    if (notmuch_database_new (&notmuch)) {
> +	fprintf (stderr, "Out of memory\n");
> +	return 1;
> +    }
> +
> +    if (notmuch_database_open (notmuch,
> +			       notmuch_config_get_database_path (config),
> +			       NOTMUCH_DATABASE_MODE_READ_WRITE))
>  	return 1;
>  
>      if (notmuch_config_get_maildir_synchronize_flags (config))
> diff --git a/test/random-corpus.c b/test/random-corpus.c
> index 790193d..2b205e5 100644
> --- a/test/random-corpus.c
> +++ b/test/random-corpus.c
> @@ -164,8 +164,14 @@ main (int argc, char **argv)
>      if (config == NULL)
>  	return 1;
>  
> -    if (notmuch_database_open (notmuch_config_get_database_path (config),
> -			       NOTMUCH_DATABASE_MODE_READ_WRITE, &notmuch))
> +    if (notmuch_database_new (&notmuch)) {
> +	fprintf (stderr, "Out of memory\n");
> +	return 1;
> +    }
> +
> +    if (notmuch_database_open (notmuch,
> +			       notmuch_config_get_database_path (config),
> +			       NOTMUCH_DATABASE_MODE_READ_WRITE))
>  	return 1;
>  
>      srandom (seed);
> diff --git a/test/symbol-test.cc b/test/symbol-test.cc
> index 3e96c03..47c5351 100644
> --- a/test/symbol-test.cc
> +++ b/test/symbol-test.cc
> @@ -5,7 +5,8 @@
>  
>  int main() {
>    notmuch_database_t *notmuch;
> -  notmuch_database_open("fakedb", NOTMUCH_DATABASE_MODE_READ_ONLY, &notmuch);
> +  notmuch_database_new (&notmuch);
> +  notmuch_database_open (notmuch, "fakedb", NOTMUCH_DATABASE_MODE_READ_ONLY);
>  
>    try {
>      (void) new Xapian::WritableDatabase("./nonexistant", Xapian::DB_OPEN);

Re: [PATCH 2/2] lib: introduce notmuch_database_new for initializing a database handle

[Jani Nikula]

On Thu, 05 Dec 2013, Austin Clements <amdragon@MIT.EDU> wrote:
> Quoth Jani Nikula on Dec 01 at  3:14 pm:
>> There is a need for setting options before opening a database, such as
>> setting a logging function to use instead of writing to stdout or
>> stderr. It would be possible to do this by adding new parameters to
>> notmuch_database_create and notmuch_database_open, but maintaining a
>> backwards compatible API and ABI when new options are added becomes
>> burdensome.
>> 
>> Instead, split the opaque database object creation from
>> notmuch_database_create and notmuch_database_open into a new
>> notmuch_database_new call, to allow operations on the handle before
>> create and open. This creates API and ABI breakage now, but allows
>> easier future extensions.
>> 
>> The notmuch_database_new call becomes a natural pair to the already
>> existing notmuch_database_destroy, and it should be possible to call
>> open/close multiple times using an initialized handle.
>
> A high-level comment about the API: Currently, an allocated
> notmuch_database_t has two "memory states", if you will: open and
> closed.  (I wish it didn't have any memory states, and was on the
> fence about this API for a while until I realized that the ship had
> already sailed.) 

Just to confirm, are you referring to the state between close and
destroy/open?

Btw what is the use case we have separate close and destroy for? To be
able to access the data cached in memory after the db has been closed?

> It's pretty clear how all of the notmuch APIs will
> behave in both states (modulo some bounded non-determinism in the
> closed state).  I think this patch introduces a new "pre-open" state,
> and I don't know how most of the notmuch APIs behave in that state.
> My guess is poorly.  If it's feasible, I'd much rather a fresh baked
> notmuch_database_t act like it's in the closed state, including that
> notmuch_database_{create,open} are well-defined as transitions from
> closed state to open state (even if the closed state was reached by
> calling notmuch_database_close).  Or, if we do have a "pre-open"
> state, it should at least be well-specified what that means
> (preferably the specification is *not* "most APIs segfault").

Agreed.

> Orthogonally -- and this may be a complete pipe dream of mine -- if we
> just had a way to return more detailed error information than a simple
> error code from notmuch_database_{create,open}, I think we wouldn't
> need any of this.  Everything that these functions currently log
> (modulo one warning) is error details, so if we could return the error
> details *with the error* or somehow make them accessible, we wouldn't
> need a logger at this point (or at several other points in the
> library).

Agreed. I tried to look into this earlier, but was hitting dead ends
*if* we want to keep reporting user friendly error status in
open/create. Obviously any concrete suggestions would be most welcome!

>> ---
>>  lib/database.cc      | 64 ++++++++++++++++++++++++++++------------------------
>>  lib/notmuch.h        | 52 ++++++++++++++++++++++++++++++++----------
>>  notmuch-compact.c    | 11 ++++++++-
>>  notmuch-count.c      | 10 ++++++--
>>  notmuch-dump.c       | 10 ++++++--
>>  notmuch-insert.c     | 10 ++++++--
>>  notmuch-new.c        | 14 +++++++-----
>>  notmuch-reply.c      | 10 ++++++--
>>  notmuch-restore.c    | 10 ++++++--
>>  notmuch-search.c     | 10 ++++++--
>>  notmuch-show.c       | 10 ++++++--
>>  notmuch-tag.c        | 10 ++++++--
>>  test/random-corpus.c | 10 ++++++--
>>  test/symbol-test.cc  |  3 ++-
>>  14 files changed, 166 insertions(+), 68 deletions(-)
>> 
>> diff --git a/lib/database.cc b/lib/database.cc
>> index 98e2c31..386b93a 100644
>> --- a/lib/database.cc
>> +++ b/lib/database.cc
>> @@ -539,10 +539,21 @@ parse_references (void *ctx,
>>  }
>>  
>>  notmuch_status_t
>> -notmuch_database_create (const char *path, notmuch_database_t **database)
>> +notmuch_database_new (notmuch_database_t **notmuch)
>
> The naming of this is unfortunate...  Other APIs use x_create to
> allocate objects (e.g., notmuch_query_create, several internal APIs).
> I would lean towards calling this function notmuch_database_create,
> but that leaves the question of what to call the other.  While we're
> breaking APIs, would it be completely crazy to merge open and create
> into one API with an extra mode to indicate creation (it can be its
> own mode because creation implies read/write)?  (Or, in UNIX
> tradition, we could call this function notmuch_database_create and the
> other notmuch_database_creat.)  notmuch_database_create is already
> just a shell around notmuch_database_open (we could keep it as a
> separate function, but just make it internal).

Agreed on the naming being unfortunate, though I'll dodge further
bikeshedding. ;) Merging open and create sounds all right, though it's a
minor detail in the bigger picture of this patch.

The comments below seemed valid too, thanks.

BR,
Jani.


>
>> +{
>> +    /* Note: try to avoid error conditions! No error printing! */
>> +
>> +    *notmuch = talloc_zero (NULL, notmuch_database_t);
>> +    if (! *notmuch)
>> +	return NOTMUCH_STATUS_OUT_OF_MEMORY;
>> +
>> +    return NOTMUCH_STATUS_SUCCESS;
>> +}
>> +
>> +notmuch_status_t
>> +notmuch_database_create (notmuch_database_t *notmuch, const char *path)
>>  {
>
> This should fail if passed a database that is already open.
>
>>      notmuch_status_t status = NOTMUCH_STATUS_SUCCESS;
>> -    notmuch_database_t *notmuch = NULL;
>>      char *notmuch_path = NULL;
>>      struct stat st;
>>      int err;
>> @@ -579,25 +590,18 @@ notmuch_database_create (const char *path, notmuch_database_t **database)
>>  	goto DONE;
>>      }
>>  
>> -    status = notmuch_database_open (path,
>> -				    NOTMUCH_DATABASE_MODE_READ_WRITE,
>> -				    &notmuch);
>> +    status = notmuch_database_open (notmuch, path,
>> +				    NOTMUCH_DATABASE_MODE_READ_WRITE);
>>      if (status)
>>  	goto DONE;
>>      status = notmuch_database_upgrade (notmuch, NULL, NULL);
>> -    if (status) {
>> +    if (status)
>>  	notmuch_database_close(notmuch);
>> -	notmuch = NULL;
>> -    }
>>  
>>    DONE:
>>      if (notmuch_path)
>>  	talloc_free (notmuch_path);
>>  
>> -    if (database)
>> -	*database = notmuch;
>> -    else
>> -	talloc_free (notmuch);
>>      return status;
>>  }
>>  
>> @@ -612,14 +616,15 @@ _notmuch_database_ensure_writable (notmuch_database_t *notmuch)
>>      return NOTMUCH_STATUS_SUCCESS;
>>  }
>>  
>> +/*
>> + * XXX: error handling should clean up *all* state created!
>> + */
>
> I think the only thing that will currently leak from this in an error
> case is notmuch->path.
>
>>  notmuch_status_t
>> -notmuch_database_open (const char *path,
>> -		       notmuch_database_mode_t mode,
>> -		       notmuch_database_t **database)
>> +notmuch_database_open (notmuch_database_t *notmuch, const char *path,
>> +		       notmuch_database_mode_t mode)
>>  {
>
> This should also fail if passed a database that is already open.
>
>>      notmuch_status_t status = NOTMUCH_STATUS_SUCCESS;
>>      void *local = talloc_new (NULL);
>> -    notmuch_database_t *notmuch = NULL;
>>      char *notmuch_path, *xapian_path;
>>      struct stat st;
>>      int err;
>> @@ -663,7 +668,6 @@ notmuch_database_open (const char *path,
>>  	initialized = 1;
>>      }
>>  
>> -    notmuch = talloc_zero (NULL, notmuch_database_t);
>>      notmuch->exception_reported = FALSE;
>>      notmuch->path = talloc_strdup (notmuch, path);
>>  
>> @@ -689,8 +693,7 @@ notmuch_database_open (const char *path,
>>  			 "       read-write mode.\n",
>>  			 notmuch_path, version, NOTMUCH_DATABASE_VERSION);
>>  		notmuch->mode = NOTMUCH_DATABASE_MODE_READ_ONLY;
>> -		notmuch_database_destroy (notmuch);
>> -		notmuch = NULL;
>> +		notmuch_database_close (notmuch);
>>  		status = NOTMUCH_STATUS_FILE_ERROR;
>>  		goto DONE;
>>  	    }
>> @@ -752,21 +755,19 @@ notmuch_database_open (const char *path,
>>      } catch (const Xapian::Error &error) {
>>  	fprintf (stderr, "A Xapian exception occurred opening database: %s\n",
>>  		 error.get_msg().c_str());
>> -	notmuch_database_destroy (notmuch);
>> -	notmuch = NULL;
>> +	notmuch_database_close (notmuch);
>>  	status = NOTMUCH_STATUS_XAPIAN_EXCEPTION;
>>      }
>>  
>>    DONE:
>>      talloc_free (local);
>>  
>
> It might be simpler to call notmuch_database_close here if status !=
> NOTMUCH_STATUS_SUCCESS, rather than calling it at several places above
> (and not on all error paths).
>
>> -    if (database)
>> -	*database = notmuch;
>> -    else
>> -	talloc_free (notmuch);
>>      return status;
>>  }
>>  
>> +/*
>> + * XXX: close should clean up *all* state created by open/create!
>> + */
>
> I believe the only thing it doesn't clean up is path.  (Note that
> cleaning up path here doesn't currently negate the need to clean up
> path above, though if you float the close call to the DONE path, it
> would suffice.)
>
>>  notmuch_status_t
>>  notmuch_database_close (notmuch_database_t *notmuch)
>>  {
>> @@ -869,7 +870,8 @@ public:
>>   * compaction process to protect data integrity.
>>   */
>>  notmuch_status_t
>> -notmuch_database_compact (const char *path,
>> +notmuch_database_compact (notmuch_database_t *notmuch,
>> +			  const char *path,
>>  			  const char *backup_path,
>>  			  notmuch_compact_status_cb_t status_cb,
>>  			  void *closure)
>> @@ -877,7 +879,6 @@ notmuch_database_compact (const char *path,
>>      void *local;
>>      char *notmuch_path, *xapian_path, *compact_xapian_path;
>>      notmuch_status_t ret = NOTMUCH_STATUS_SUCCESS;
>> -    notmuch_database_t *notmuch = NULL;
>>      struct stat statbuf;
>>      notmuch_bool_t keep_backup;
>>  
>> @@ -885,7 +886,8 @@ notmuch_database_compact (const char *path,
>>      if (! local)
>>  	return NOTMUCH_STATUS_OUT_OF_MEMORY;
>>  
>> -    ret = notmuch_database_open (path, NOTMUCH_DATABASE_MODE_READ_WRITE, &notmuch);
>> +    ret = notmuch_database_open (notmuch, path,
>> +				 NOTMUCH_DATABASE_MODE_READ_WRITE);
>>      if (ret) {
>>  	goto DONE;
>>      }
>> @@ -971,8 +973,9 @@ notmuch_database_compact (const char *path,
>>      }
>>  
>>    DONE:
>> +    /* XXX: error handling */
>>      if (notmuch)
>> -	ret = notmuch_database_destroy (notmuch);
>> +	ret = notmuch_database_close (notmuch);
>>  
>>      talloc_free (local);
>>  
>> @@ -980,7 +983,8 @@ notmuch_database_compact (const char *path,
>>  }
>>  #else
>>  notmuch_status_t
>> -notmuch_database_compact (unused (const char *path),
>> +notmuch_database_compact (unused (notmuch_database_t *notmuch),
>> +			  unused (const char *path),
>>  			  unused (const char *backup_path),
>>  			  unused (notmuch_compact_status_cb_t status_cb),
>>  			  unused (void *closure))
>> diff --git a/lib/notmuch.h b/lib/notmuch.h
>> index dbdce86..cd58d15 100644
>> --- a/lib/notmuch.h
>> +++ b/lib/notmuch.h
>> @@ -149,6 +149,28 @@ typedef struct _notmuch_tags notmuch_tags_t;
>>  typedef struct _notmuch_directory notmuch_directory_t;
>>  typedef struct _notmuch_filenames notmuch_filenames_t;
>>  
>> +/* Initialize a new, empty database handle.
>> + *
>> + * The database handle is required for creating, opening, and
>> + * compacting a database. For further database operations, the
>> + * database needs to be created or opened.
>> + *
>> + * After a successful call to notmuch_database_new, the returned
>> + * database handle will remain in memory, so the caller should call
>> + * notmuch_database_destroy when finished with the database handle.
>> + *
>> + * In case of any failure, this function returns an error status and
>> + * sets *notmuch to NULL.
>> + *
>> + * Return value:
>> + *
>> + * NOTMUCH_STATUS_SUCCESS: Successfully created the database object.
>> + *
>> + * NOTMUCH_STATUS_OUT_OF_MEMORY: Out of memory.
>> + */
>> +notmuch_status_t
>> +notmuch_database_new (notmuch_database_t **notmuch);
>> +
>>  /* Create a new, empty notmuch database located at 'path'.
>>   *
>>   * The path should be a top-level directory to a collection of
>> @@ -156,9 +178,9 @@ typedef struct _notmuch_filenames notmuch_filenames_t;
>>   * create a new ".notmuch" directory within 'path' where notmuch will
>>   * store its data.
>>   *
>> - * After a successful call to notmuch_database_create, the returned
>> - * database will be open so the caller should call
>> - * notmuch_database_destroy when finished with it.
>> + * After a successful call to notmuch_database_create, the database
>> + * will be open so the caller should call notmuch_database_close (or
>> + * notmuch_database_destroy) when finished with the database.
>>   *
>>   * The database will not yet have any data in it
>>   * (notmuch_database_create itself is a very cheap function). Messages
>> @@ -166,7 +188,8 @@ typedef struct _notmuch_filenames notmuch_filenames_t;
>>   * notmuch_database_add_message.
>>   *
>>   * In case of any failure, this function returns an error status and
>> - * sets *database to NULL (after printing an error message on stderr).
>> + * the database will be closed (after printing an error message on
>> + * stderr).
>>   *
>>   * Return value:
>>   *
>> @@ -183,7 +206,7 @@ typedef struct _notmuch_filenames notmuch_filenames_t;
>>   * NOTMUCH_STATUS_XAPIAN_EXCEPTION: A Xapian exception occurred.
>>   */
>>  notmuch_status_t
>> -notmuch_database_create (const char *path, notmuch_database_t **database);
>> +notmuch_database_create (notmuch_database_t *notmuch, const char *path);
>>  
>>  typedef enum {
>>      NOTMUCH_DATABASE_MODE_READ_ONLY = 0,
>> @@ -201,11 +224,13 @@ typedef enum {
>>   * An existing notmuch database can be identified by the presence of a
>>   * directory named ".notmuch" below 'path'.
>>   *
>> - * The caller should call notmuch_database_destroy when finished with
>> - * this database.
>> + * After a successful call to notmuch_database_open, the database will
>> + * be open so the caller should call notmuch_database_close (or
>> + * notmuch_database_destroy) when finished with the database.
>>   *
>>   * In case of any failure, this function returns an error status and
>> - * sets *database to NULL (after printing an error message on stderr).
>> + * the database will be closed (after printing an error message on
>> + * stderr).
>>   *
>>   * Return value:
>>   *
>> @@ -222,9 +247,8 @@ typedef enum {
>>   * NOTMUCH_STATUS_XAPIAN_EXCEPTION: A Xapian exception occurred.
>>   */
>>  notmuch_status_t
>> -notmuch_database_open (const char *path,
>> -		       notmuch_database_mode_t mode,
>> -		       notmuch_database_t **database);
>> +notmuch_database_open (notmuch_database_t *notmuch, const char *path,
>> +		       notmuch_database_mode_t mode);
>>  
>>  /* Close the given notmuch database.
>>   *
>> @@ -264,7 +288,8 @@ typedef void (*notmuch_compact_status_cb_t)(const char *message, void *closure);
>>   * 'closure' is passed verbatim to any callback invoked.
>>   */
>>  notmuch_status_t
>> -notmuch_database_compact (const char* path,
>> +notmuch_database_compact (notmuch_database_t *notmuch,
>> +			  const char* path,
>>  			  const char* backup_path,
>>  			  notmuch_compact_status_cb_t status_cb,
>>  			  void *closure);
>> @@ -272,6 +297,9 @@ notmuch_database_compact (const char* path,
>>  /* Destroy the notmuch database, closing it if necessary and freeing
>>   * all associated resources.
>>   *
>> + * A database handle initialized with notmuch_database_new should be
>> + * destroyed by calling notmuch_database_destroy.
>> + *
>>   * Return value as in notmuch_database_close if the database was open;
>>   * notmuch_database_destroy itself has no failure modes.
>>   */
>> diff --git a/notmuch-compact.c b/notmuch-compact.c
>> index 8b820c0..626685e 100644
>> --- a/notmuch-compact.c
>> +++ b/notmuch-compact.c
>> @@ -29,6 +29,7 @@ status_update_cb (const char *msg, unused (void *closure))
>>  int
>>  notmuch_compact_command (notmuch_config_t *config, int argc, char *argv[])
>>  {
>> +    notmuch_database_t *notmuch = NULL;
>>      const char *path = notmuch_config_get_database_path (config);
>>      const char *backup_path = NULL;
>>      notmuch_status_t ret;
>> @@ -46,7 +47,13 @@ notmuch_compact_command (notmuch_config_t *config, int argc, char *argv[])
>>  
>>      if (! quiet)
>>  	printf ("Compacting database...\n");
>> -    ret = notmuch_database_compact (path, backup_path,
>> +
>> +    if (notmuch_database_new (&notmuch)) {
>> +	fprintf (stderr, "Out of memory\n");
>> +	return 1;
>> +    }
>> +
>> +    ret = notmuch_database_compact (notmuch, path, backup_path,
>>  				    quiet ? NULL : status_update_cb, NULL);
>>      if (ret) {
>>  	fprintf (stderr, "Compaction failed: %s\n", notmuch_status_to_string (ret));
>> @@ -60,5 +67,7 @@ notmuch_compact_command (notmuch_config_t *config, int argc, char *argv[])
>>  	printf ("Done.\n");
>>      }
>>  
>> +    notmuch_database_destroy (notmuch);
>> +
>>      return 0;
>>  }
>> diff --git a/notmuch-count.c b/notmuch-count.c
>> index 01e4e30..15c95c7 100644
>> --- a/notmuch-count.c
>> +++ b/notmuch-count.c
>> @@ -170,8 +170,14 @@ notmuch_count_command (notmuch_config_t *config, int argc, char *argv[])
>>  	return 1;
>>      }
>>  
>> -    if (notmuch_database_open (notmuch_config_get_database_path (config),
>> -			       NOTMUCH_DATABASE_MODE_READ_ONLY, &notmuch))
>> +    if (notmuch_database_new (&notmuch)) {
>> +	fprintf (stderr, "Out of memory\n");
>> +	return 1;
>> +    }
>> +
>> +    if (notmuch_database_open (notmuch,
>> +			       notmuch_config_get_database_path (config),
>> +			       NOTMUCH_DATABASE_MODE_READ_ONLY))
>
> Does this need to destroy the database?  (Likewise for all the others.)
>
>>  	return 1;
>>  
>>      query_str = query_string_from_args (config, argc-opt_index, argv+opt_index);
>> diff --git a/notmuch-dump.c b/notmuch-dump.c
>> index 2024e30..73579bc 100644
>> --- a/notmuch-dump.c
>> +++ b/notmuch-dump.c
>> @@ -33,8 +33,14 @@ notmuch_dump_command (notmuch_config_t *config, int argc, char *argv[])
>>      notmuch_tags_t *tags;
>>      const char *query_str = "";
>>  
>> -    if (notmuch_database_open (notmuch_config_get_database_path (config),
>> -			       NOTMUCH_DATABASE_MODE_READ_ONLY, &notmuch))
>> +    if (notmuch_database_new (&notmuch)) {
>> +	fprintf (stderr, "Out of memory\n");
>> +	return 1;
>> +    }
>> +
>> +    if (notmuch_database_open (notmuch,
>> +			       notmuch_config_get_database_path (config),
>> +			       NOTMUCH_DATABASE_MODE_READ_ONLY))
>>  	return 1;
>>  
>>      char *output_file_name = NULL;
>> diff --git a/notmuch-insert.c b/notmuch-insert.c
>> index 2207b1e..4a8aad3 100644
>> --- a/notmuch-insert.c
>> +++ b/notmuch-insert.c
>> @@ -467,8 +467,14 @@ notmuch_insert_command (notmuch_config_t *config, int argc, char *argv[])
>>      action.sa_flags = 0;
>>      sigaction (SIGINT, &action, NULL);
>>  
>> -    if (notmuch_database_open (notmuch_config_get_database_path (config),
>> -			       NOTMUCH_DATABASE_MODE_READ_WRITE, &notmuch))
>> +    if (notmuch_database_new (&notmuch)) {
>> +	fprintf (stderr, "Out of memory\n");
>> +	return 1;
>> +    }
>> +
>> +    if (notmuch_database_open (notmuch,
>> +			       notmuch_config_get_database_path (config),
>> +			       NOTMUCH_DATABASE_MODE_READ_WRITE))
>>  	return 1;
>>  
>>      ret = insert_message (config, notmuch, STDIN_FILENO, maildir, tag_ops);
>> diff --git a/notmuch-new.c b/notmuch-new.c
>> index ba05cb4..f72a4de 100644
>> --- a/notmuch-new.c
>> +++ b/notmuch-new.c
>> @@ -914,6 +914,11 @@ notmuch_new_command (notmuch_config_t *config, int argc, char *argv[])
>>  	    return ret;
>>      }
>>  
>> +    if (notmuch_database_new (&notmuch)) {
>> +	fprintf (stderr, "Out of memory\n");
>> +	return 1;
>> +    }
>> +
>>      dot_notmuch_path = talloc_asprintf (config, "%s/%s", db_path, ".notmuch");
>>  
>>      if (stat (dot_notmuch_path, &st)) {
>> @@ -925,12 +930,12 @@ notmuch_new_command (notmuch_config_t *config, int argc, char *argv[])
>>  	    return 1;
>>  
>>  	printf ("Found %d total files (that's not much mail).\n", count);
>> -	if (notmuch_database_create (db_path, &notmuch))
>> +	if (notmuch_database_create (notmuch, db_path))
>>  	    return 1;
>>  	add_files_state.total_files = count;
>>      } else {
>> -	if (notmuch_database_open (db_path, NOTMUCH_DATABASE_MODE_READ_WRITE,
>> -				   &notmuch))
>> +	if (notmuch_database_open (notmuch, db_path,
>> +				   NOTMUCH_DATABASE_MODE_READ_WRITE))
>>  	    return 1;
>>  
>>  	if (notmuch_database_needs_upgrade (notmuch)) {
>> @@ -945,9 +950,6 @@ notmuch_new_command (notmuch_config_t *config, int argc, char *argv[])
>>  	add_files_state.total_files = 0;
>>      }
>>  
>> -    if (notmuch == NULL)
>> -	return 1;
>> -
>>      /* Setup our handler for SIGINT. We do this after having
>>       * potentially done a database upgrade we this interrupt handler
>>       * won't support. */
>> diff --git a/notmuch-reply.c b/notmuch-reply.c
>> index 9d6f843..7b80841 100644
>> --- a/notmuch-reply.c
>> +++ b/notmuch-reply.c
>> @@ -769,8 +769,14 @@ notmuch_reply_command (notmuch_config_t *config, int argc, char *argv[])
>>  	return 1;
>>      }
>>  
>> -    if (notmuch_database_open (notmuch_config_get_database_path (config),
>> -			       NOTMUCH_DATABASE_MODE_READ_ONLY, &notmuch))
>> +    if (notmuch_database_new (&notmuch)) {
>> +	fprintf (stderr, "Out of memory\n");
>> +	return 1;
>> +    }
>> +
>> +    if (notmuch_database_open (notmuch,
>> +			       notmuch_config_get_database_path (config),
>> +			       NOTMUCH_DATABASE_MODE_READ_ONLY))
>>  	return 1;
>>  
>>      query = notmuch_query_create (notmuch, query_string);
>> diff --git a/notmuch-restore.c b/notmuch-restore.c
>> index 1419621..fc37838 100644
>> --- a/notmuch-restore.c
>> +++ b/notmuch-restore.c
>> @@ -138,8 +138,14 @@ notmuch_restore_command (notmuch_config_t *config, int argc, char *argv[])
>>      int opt_index;
>>      int input_format = DUMP_FORMAT_AUTO;
>>  
>> -    if (notmuch_database_open (notmuch_config_get_database_path (config),
>> -			       NOTMUCH_DATABASE_MODE_READ_WRITE, &notmuch))
>> +    if (notmuch_database_new (&notmuch)) {
>> +	fprintf (stderr, "Out of memory\n");
>> +	return 1;
>> +    }
>> +
>> +    if (notmuch_database_open (notmuch,
>> +			       notmuch_config_get_database_path (config),
>> +			       NOTMUCH_DATABASE_MODE_READ_WRITE))
>>  	return 1;
>>  
>>      if (notmuch_config_get_maildir_synchronize_flags (config))
>> diff --git a/notmuch-search.c b/notmuch-search.c
>> index 7c973b3..50aace9 100644
>> --- a/notmuch-search.c
>> +++ b/notmuch-search.c
>> @@ -430,8 +430,14 @@ notmuch_search_command (notmuch_config_t *config, int argc, char *argv[])
>>  
>>      notmuch_exit_if_unsupported_format ();
>>  
>> -    if (notmuch_database_open (notmuch_config_get_database_path (config),
>> -			       NOTMUCH_DATABASE_MODE_READ_ONLY, &notmuch))
>> +    if (notmuch_database_new (&notmuch)) {
>> +	fprintf (stderr, "Out of memory\n");
>> +	return 1;
>> +    }
>> +
>> +    if (notmuch_database_open (notmuch,
>> +			       notmuch_config_get_database_path (config),
>> +			       NOTMUCH_DATABASE_MODE_READ_ONLY))
>>  	return 1;
>>  
>>      query_str = query_string_from_args (notmuch, argc-opt_index, argv+opt_index);
>> diff --git a/notmuch-show.c b/notmuch-show.c
>> index c07f887..bc44b2c 100644
>> --- a/notmuch-show.c
>> +++ b/notmuch-show.c
>> @@ -1201,8 +1201,14 @@ notmuch_show_command (notmuch_config_t *config, int argc, char *argv[])
>>  	return 1;
>>      }
>>  
>> -    if (notmuch_database_open (notmuch_config_get_database_path (config),
>> -			       NOTMUCH_DATABASE_MODE_READ_ONLY, &notmuch))
>> +    if (notmuch_database_new (&notmuch)) {
>> +	fprintf (stderr, "Out of memory\n");
>> +	return 1;
>> +    }
>> +
>> +    if (notmuch_database_open (notmuch,
>> +			       notmuch_config_get_database_path (config),
>> +			       NOTMUCH_DATABASE_MODE_READ_ONLY))
>>  	return 1;
>>  
>>      query = notmuch_query_create (notmuch, query_string);
>> diff --git a/notmuch-tag.c b/notmuch-tag.c
>> index 3b09df9..6e29799 100644
>> --- a/notmuch-tag.c
>> +++ b/notmuch-tag.c
>> @@ -254,8 +254,14 @@ notmuch_tag_command (notmuch_config_t *config, int argc, char *argv[])
>>  	}
>>      }
>>  
>> -    if (notmuch_database_open (notmuch_config_get_database_path (config),
>> -			       NOTMUCH_DATABASE_MODE_READ_WRITE, &notmuch))
>> +    if (notmuch_database_new (&notmuch)) {
>> +	fprintf (stderr, "Out of memory\n");
>> +	return 1;
>> +    }
>> +
>> +    if (notmuch_database_open (notmuch,
>> +			       notmuch_config_get_database_path (config),
>> +			       NOTMUCH_DATABASE_MODE_READ_WRITE))
>>  	return 1;
>>  
>>      if (notmuch_config_get_maildir_synchronize_flags (config))
>> diff --git a/test/random-corpus.c b/test/random-corpus.c
>> index 790193d..2b205e5 100644
>> --- a/test/random-corpus.c
>> +++ b/test/random-corpus.c
>> @@ -164,8 +164,14 @@ main (int argc, char **argv)
>>      if (config == NULL)
>>  	return 1;
>>  
>> -    if (notmuch_database_open (notmuch_config_get_database_path (config),
>> -			       NOTMUCH_DATABASE_MODE_READ_WRITE, &notmuch))
>> +    if (notmuch_database_new (&notmuch)) {
>> +	fprintf (stderr, "Out of memory\n");
>> +	return 1;
>> +    }
>> +
>> +    if (notmuch_database_open (notmuch,
>> +			       notmuch_config_get_database_path (config),
>> +			       NOTMUCH_DATABASE_MODE_READ_WRITE))
>>  	return 1;
>>  
>>      srandom (seed);
>> diff --git a/test/symbol-test.cc b/test/symbol-test.cc
>> index 3e96c03..47c5351 100644
>> --- a/test/symbol-test.cc
>> +++ b/test/symbol-test.cc
>> @@ -5,7 +5,8 @@
>>  
>>  int main() {
>>    notmuch_database_t *notmuch;
>> -  notmuch_database_open("fakedb", NOTMUCH_DATABASE_MODE_READ_ONLY, &notmuch);
>> +  notmuch_database_new (&notmuch);
>> +  notmuch_database_open (notmuch, "fakedb", NOTMUCH_DATABASE_MODE_READ_ONLY);
>>  
>>    try {
>>      (void) new Xapian::WritableDatabase("./nonexistant", Xapian::DB_OPEN);

Re: [PATCH 2/2] lib: introduce notmuch_database_new for initializing a database handle

[David Bremner]

Jani Nikula <jani@nikula.org> writes:
>> Austin Wrote:
>>
>> Orthogonally -- and this may be a complete pipe dream of mine -- if we
>> just had a way to return more detailed error information than a simple
>> error code from notmuch_database_{create,open}, I think we wouldn't
>> need any of this.  Everything that these functions currently log
>> (modulo one warning) is error details, so if we could return the error
>> details *with the error* or somehow make them accessible, we wouldn't
>> need a logger at this point (or at several other points in the
>> library).
>
> Agreed. I tried to look into this earlier, but was hitting dead ends
> *if* we want to keep reporting user friendly error status in
> open/create. Obviously any concrete suggestions would be most welcome!
>

I'm not sure if this is also a dead end, but I was trying to sketch out
an api that returned something more detailed as status and came up with
the following.  The general idea is to replace notmuch_status_t with a
pointer to struct.  This will require pretty noisy source changes,
unless we're comfortable with using NULL pointer to indicate success.
In either case we'd rename the existing enum to something like
notmuch_status_code_t.

/* pseudo-C follows */

typedef struct notmuch_status_struct * notmuch_status_t;

/* we can just tell external users to pass NULL as the first argument */

notmuch_status_t notmuch_status_new (void *ctx, size_t bufsiz);
void notmuch_error_destroy (notuch_error_desc_t *victim);

/* printf equivalent */
notmuch_status_t *notmuch_status_format(notmuch_status_t dest,
					notmuch_status_code_t code,
					const char *format, ...);
/* case 1, caller allocates */
status = notmuch_status_new (BUFSIZ);
if (!status) {
    halt_and_catch_fire();
}

/* open could continue to return notmuch_status_code_t, or just 0/1 */
if (notmuch_database_open (notmuch_config_get_database_path (config),
			   NOTMUCH_DATABASE_MODE_READ_WRITE, &notmuch, status)) {
    fprintf (stderr, "oops: %s\n", notmuch_status_to_string (status));
    notmuch_error_destroy(error_details);
    return 1;
}

/* case 2, callee allocates */

status = notmuch_message_freeze (message);
if (notmuch_status_to_code (status)) {  /* every check needs to be changed, unless NULL=OK */
    message_error (message, status, "freezing message");
    return status;
}

/* some existing code is left alone */

fprintf (stderr, "Message-ID: %s\n", notmuch_message_get_message_id (message));
fprintf (stderr, "Status: %s\n", notmuch_status_to_string (status));

Re: [PATCH 2/2] lib: introduce notmuch_database_new for initializing a database handle

[David Bremner]

David Bremner <david@tethera.net> writes:

> I'm not sure if this is also a dead end, but I was trying to sketch out
> an api that returned something more detailed as status and came up with
> the following.  The general idea is to replace notmuch_status_t with a
> pointer to struct.  This will require pretty noisy source changes,
> unless we're comfortable with using NULL pointer to indicate success.
> In either case we'd rename the existing enum to something like
> notmuch_status_code_t.
>
> /* pseudo-C follows */
>
> typedef struct notmuch_status_struct * notmuch_status_t;
>

A less API disruptive change would be to continue returning codes, but
provide functions to interrogate the main types (database, message, etc)
for "last-error". We'd still need to special case database_(open|create)

d