From: Ali Polatel <polatel@gmail.com>
To: Notmuch Mailing List <notmuch@notmuchmail.org>
Subject: [PATCH v1 1/1] lib: make find_message{,by_filename) report errors
Date: Mon,  3 Oct 2011 19:49:20 +0300
Message-Id:
 <1218582065f35bfcc5b84dfc1fbce21fc05034a6.1317660324.git.alip@exherbo.org>
In-Reply-To: <cover.1317660324.git.alip@exherbo.org>
References: <cover.1317456435.git.alip@exherbo.org>
	<cover.1317660324.git.alip@exherbo.org>
In-Reply-To: <cover.1317660324.git.alip@exherbo.org>
References: <cover.1317660324.git.alip@exherbo.org>
Organization: Pink Floyd
Cc: Austin Clements <amdragon@MIT.EDU>
Precedence: list

Previously, the functions notmuch_database_find_message() and
notmuch_database_find_message_by_filename() functions did not properly
report error condition to the library user.

For more information, read the thread on the notmuch mailing list
starting with my mail "id:871uv2unfd.fsf@gmail.com"

Make these functions accept a pointer to 'notmuch_message_t' as argument
and return notmuch_status_t which may be used to check for any error
condition.

restore: Modify for the new notmuch_database_find_message()
new: Modify for the new notmuch_database_find_message_by_filename()
---
 lib/database.cc   |   90 ++++++++++++++++++++++++++++++++++------------------
 lib/message.cc    |    6 ++--
 lib/notmuch.h     |   61 +++++++++++++++++++++++++----------
 notmuch-new.c     |    4 ++-
 notmuch-restore.c |   11 ++++--
 5 files changed, 115 insertions(+), 57 deletions(-)

diff --git a/lib/database.cc b/lib/database.cc
index 9299c8d..1705232 100644
--- a/lib/database.cc
+++ b/lib/database.cc
@@ -360,13 +360,17 @@ _message_id_compressed (void *ctx, const char *message_id)
     return compressed;
 }
 
-notmuch_message_t *
+notmuch_status_t
 notmuch_database_find_message (notmuch_database_t *notmuch,
-			       const char *message_id)
+			       const char *message_id,
+			       notmuch_message_t **message)
 {
     notmuch_private_status_t status;
     unsigned int doc_id;
 
+    if (message == NULL)
+	return NOTMUCH_STATUS_NULL_POINTER;
+
     if (strlen (message_id) > NOTMUCH_MESSAGE_ID_MAX)
 	message_id = _message_id_compressed (notmuch, message_id);
 
@@ -375,14 +379,21 @@ notmuch_database_find_message (notmuch_database_t *notmuch,
 						       message_id, &doc_id);
 
 	if (status == NOTMUCH_PRIVATE_STATUS_NO_DOCUMENT_FOUND)
-	    return NULL;
+	    *message = NULL;
+	else {
+	    *message = _notmuch_message_create (notmuch, notmuch, doc_id,
+						NULL);
+	    if (*message == NULL)
+		return NOTMUCH_STATUS_OUT_OF_MEMORY;
+	}
 
-	return _notmuch_message_create (notmuch, notmuch, doc_id, NULL);
+	return NOTMUCH_STATUS_SUCCESS;
     } catch (const Xapian::Error &error) {
 	fprintf (stderr, "A Xapian exception occurred finding message: %s.\n",
 		 error.get_msg().c_str());
 	notmuch->exception_reported = TRUE;
-	return NULL;
+	*message = NULL;
+	return NOTMUCH_STATUS_XAPIAN_EXCEPTION;
     }
 }
 
@@ -1311,7 +1322,8 @@ _get_metadata_thread_id_key (void *ctx, const char *message_id)
 
 /* Find the thread ID to which the message with 'message_id' belongs.
  *
- * Always returns a newly talloced string belonging to 'ctx'.
+ * Note: 'thread_id' must not be NULL!
+ * On success '*thread_id' is set to a newly talloced string belonging to 'ctx'.
  *
  * Note: If there is no message in the database with the given
  * 'message_id' then a new thread_id will be allocated for this
@@ -1319,25 +1331,29 @@ _get_metadata_thread_id_key (void *ctx, const char *message_id)
  * thread ID can be looked up if the message is added to the database
  * later).
  */
-static const char *
+static notmuch_status_t
 _resolve_message_id_to_thread_id (notmuch_database_t *notmuch,
 				  void *ctx,
-				  const char *message_id)
+				  const char *message_id,
+				  const char **thread_id)
 {
+    notmuch_status_t status;
     notmuch_message_t *message;
     string thread_id_string;
-    const char *thread_id;
     char *metadata_key;
     Xapian::WritableDatabase *db;
 
-    message = notmuch_database_find_message (notmuch, message_id);
+    status = notmuch_database_find_message (notmuch, message_id, &message);
+
+    if (status)
+	return status;
 
     if (message) {
-	thread_id = talloc_steal (ctx, notmuch_message_get_thread_id (message));
+	*thread_id = talloc_steal (ctx, notmuch_message_get_thread_id (message));
 
 	notmuch_message_destroy (message);
 
-	return thread_id;
+	return NOTMUCH_STATUS_SUCCESS;
     }
 
     /* Message has not been seen yet.
@@ -1351,15 +1367,15 @@ _resolve_message_id_to_thread_id (notmuch_database_t *notmuch,
     thread_id_string = notmuch->xapian_db->get_metadata (metadata_key);
 
     if (thread_id_string.empty()) {
-	thread_id = _notmuch_database_generate_thread_id (notmuch);
-	db->set_metadata (metadata_key, thread_id);
+	*thread_id = talloc_strdup (ctx, _notmuch_database_generate_thread_id (notmuch));
+	db->set_metadata (metadata_key, *thread_id);
     } else {
-	thread_id = thread_id_string.c_str();
+	*thread_id = talloc_strdup(ctx, thread_id_string.c_str());
     }
 
     talloc_free (metadata_key);
 
-    return talloc_strdup (ctx, thread_id);
+    return NOTMUCH_STATUS_SUCCESS;
 }
 
 static notmuch_status_t
@@ -1446,9 +1462,12 @@ _notmuch_database_link_message_to_parents (notmuch_database_t *notmuch,
 	_notmuch_message_add_term (message, "reference",
 				   parent_message_id);
 
-	parent_thread_id = _resolve_message_id_to_thread_id (notmuch,
-							     message,
-							     parent_message_id);
+	ret = _resolve_message_id_to_thread_id (notmuch,
+						message,
+						parent_message_id,
+						&parent_thread_id);
+	if (ret)
+	    goto DONE;
 
 	if (*thread_id == NULL) {
 	    *thread_id = talloc_strdup (message, parent_thread_id);
@@ -1759,11 +1778,12 @@ notmuch_status_t
 notmuch_database_remove_message (notmuch_database_t *notmuch,
 				 const char *filename)
 {
-    notmuch_message_t *message =
-	notmuch_database_find_message_by_filename (notmuch, filename);
-    notmuch_status_t status = NOTMUCH_STATUS_SUCCESS;
+    notmuch_status_t status;
+    notmuch_message_t *message;
 
-    if (message) {
+    status = notmuch_database_find_message_by_filename (notmuch, filename, &message);
+
+    if (status == NOTMUCH_STATUS_SUCCESS && message) {
 	    status = _notmuch_message_remove_filename (message, filename);
 	    if (status == NOTMUCH_STATUS_SUCCESS)
 		_notmuch_message_delete (message);
@@ -1774,24 +1794,27 @@ notmuch_database_remove_message (notmuch_database_t *notmuch,
     return status;
 }
 
-notmuch_message_t *
+notmuch_status_t
 notmuch_database_find_message_by_filename (notmuch_database_t *notmuch,
-					   const char *filename)
+					   const char *filename,
+					   notmuch_message_t **message)
 {
     void *local;
     const char *prefix = _find_prefix ("file-direntry");
     char *direntry, *term;
     Xapian::PostingIterator i, end;
-    notmuch_message_t *message = NULL;
     notmuch_status_t status;
 
+    if (message == NULL)
+	return NOTMUCH_STATUS_NULL_POINTER;
+
     local = talloc_new (notmuch);
 
     try {
 	status = _notmuch_database_filename_to_direntry (local, notmuch,
 							 filename, &direntry);
 	if (status)
-	    return NULL;
+	    goto DONE;
 
 	term = talloc_asprintf (local, "%s%s", prefix, direntry);
 
@@ -1800,19 +1823,24 @@ notmuch_database_find_message_by_filename (notmuch_database_t *notmuch,
 	if (i != end) {
 	    notmuch_private_status_t private_status;
 
-	    message = _notmuch_message_create (notmuch, notmuch,
-					       *i, &private_status);
+	    *message = _notmuch_message_create (notmuch, notmuch,
+						*i, &private_status);
+	    if (*message == NULL)
+		status = NOTMUCH_STATUS_OUT_OF_MEMORY;
 	}
     } catch (const Xapian::Error &error) {
 	fprintf (stderr, "Error: A Xapian exception occurred finding message by filename: %s\n",
 		 error.get_msg().c_str());
 	notmuch->exception_reported = TRUE;
-	message = NULL;
+	status = NOTMUCH_STATUS_OUT_OF_MEMORY;
     }
 
+  DONE:
     talloc_free (local);
 
-    return message;
+    if (status)
+	*message = NULL;
+    return status;
 }
 
 notmuch_string_list_t *
diff --git a/lib/message.cc b/lib/message.cc
index 531d304..2a85744 100644
--- a/lib/message.cc
+++ b/lib/message.cc
@@ -216,11 +216,11 @@ _notmuch_message_create_for_message_id (notmuch_database_t *notmuch,
     unsigned int doc_id;
     char *term;
 
-    *status_ret = NOTMUCH_PRIVATE_STATUS_SUCCESS;
-
-    message = notmuch_database_find_message (notmuch, message_id);
+    *status_ret = (notmuch_private_status_t) notmuch_database_find_message (notmuch, message_id, &message);
     if (message)
 	return talloc_steal (notmuch, message);
+    else if (*status_ret)
+	return NULL;
 
     term = talloc_asprintf (NULL, "%s%s",
 			    _find_prefix ("id"), message_id);
diff --git a/lib/notmuch.h b/lib/notmuch.h
index 6d7a99f..08b4ce2 100644
--- a/lib/notmuch.h
+++ b/lib/notmuch.h
@@ -347,35 +347,60 @@ notmuch_database_remove_message (notmuch_database_t *database,
 
 /* Find a message with the given message_id.
  *
- * If the database contains a message with the given message_id, then
- * a new notmuch_message_t object is returned. The caller should call
- * notmuch_message_destroy when done with the message.
+ * If message with the given message_id is found then, on successful return
+ * (NOTMUCH_STATUS_SUCCESS) '*message' will be initialized to a message object.
+ * The user should call notmuch_message_destroy when done with the message.
  *
- * This function returns NULL in the following situations:
+ * On any failure or when the message is not found, this function initializes
+ * '*message' to NULL. This means, when NOTMUCH_STATUS_SUCCESS is returned, the
+ * user is supposed to check '*message' for NULL to find out whether the
+ * message with the given message_id was found.
  *
- *	* No message is found with the given message_id
- *	* An out-of-memory situation occurs
- *	* A Xapian exception occurs
+ * Note: The argument 'message' must not be NULL!
+ *
+ * Return value:
+ *
+ * NOTMUCH_STATUS_SUCCESS: Successful return, check '*message'.
+ *
+ * NOTMUCH_STATUS_NULL_POINTER: The given 'message' argument is NULL
+ *
+ * NOTMUCH_STATUS_OUT_OF_MEMORY: Out of memory, creating message object
+ *
+ * NOTMUCH_STATUS_XAPIAN_EXCEPTION: A Xapian exception occurred
  */
-notmuch_message_t *
+notmuch_status_t
 notmuch_database_find_message (notmuch_database_t *database,
-			       const char *message_id);
+			       const char *message_id,
+			       notmuch_message_t **message);
 
 /* Find a message with the given filename.
  *
- * If the database contains a message with the given filename, then a
- * new notmuch_message_t object is returned.  The caller should call 
- * notmuch_message_destroy when done with the message.
+ * If the database contains a message with the given filename then, on
+ * successful return (NOTMUCH_STATUS_SUCCESS) '*message' will be initialized to
+ * a message object. The user should call notmuch_message_destroy when done
+ * with the message.
  *
- * This function returns NULL in the following situations:
+ * On any failure or when the message is not found, this function initializes
+ * '*message' to NULL. This means, when NOTMUCH_STATUS_SUCCESS is returned, the
+ * user is supposed to check '*message' for NULL to find out whether the
+ * message with the given filename is found.
  *
- *	* No message is found with the given filename
- *	* An out-of-memory situation occurs
- *	* A Xapian exception occurs
+ * Note: The argument 'message' must not be NULL!
+ *
+ * Return value:
+ *
+ * NOTMUCH_STATUS_SUCCESS: Successful return, check '*message'
+ *
+ * NOTMUCH_STATUS_NULL_POINTER: The given 'message' argument is NULL
+ *
+ * NOTMUCH_STATUS_OUT_OF_MEMORY: Out of memory, creating the message object
+ *
+ * NOTMUCH_STATUS_XAPIAN_EXCEPTION: A Xapian exception occurred
  */
-notmuch_message_t *
+notmuch_status_t
 notmuch_database_find_message_by_filename (notmuch_database_t *notmuch,
-					   const char *filename);
+					   const char *filename,
+					   notmuch_message_t **message);
 
 /* Return a list of all tags found in the database.
  *
diff --git a/notmuch-new.c b/notmuch-new.c
index e79593c..96a1e31 100644
--- a/notmuch-new.c
+++ b/notmuch-new.c
@@ -743,7 +743,9 @@ remove_filename (notmuch_database_t *notmuch,
     status = notmuch_database_begin_atomic (notmuch);
     if (status)
 	return status;
-    message = notmuch_database_find_message_by_filename (notmuch, path);
+    status = notmuch_database_find_message_by_filename (notmuch, path, &message);
+    if (status || message == NULL)
+	return status;
     status = notmuch_database_remove_message (notmuch, path);
     if (status == NOTMUCH_STATUS_DUPLICATE_MESSAGE_ID) {
 	add_files_state->renamed_messages++;
diff --git a/notmuch-restore.c b/notmuch-restore.c
index f095f64..e4a5355 100644
--- a/notmuch-restore.c
+++ b/notmuch-restore.c
@@ -87,10 +87,13 @@ notmuch_restore_command (unused (void *ctx), int argc, char *argv[])
 	file_tags = xstrndup (line + match[2].rm_so,
 			      match[2].rm_eo - match[2].rm_so);
 
-	message = notmuch_database_find_message (notmuch, message_id);
-	if (message == NULL) {
-	    fprintf (stderr, "Warning: Cannot apply tags to missing message: %s\n",
-		     message_id);
+	status = notmuch_database_find_message (notmuch, message_id, &message);
+	if (status || message == NULL) {
+	    fprintf (stderr, "Warning: Cannot apply tags to %smessage: %s\n",
+		     message ? "" : "missing ", message_id);
+	    if (status)
+		fprintf (stderr, "%s\n",
+			 notmuch_status_to_string(status));
 	    goto NEXT_LINE;
 	}
 
-- 
1.7.6.1