Concerns regarding some library functions

Concerns regarding some library functions

[Ali Polatel]

[-- Attachment #1: Type: text/plain, Size: 1548 bytes --]

Hello,

Being the maintainer of Ruby bindings, I've been watching the
development of API changes closely. Ruby bindings are nearly complete
with the exception of two functions which I think are poorly implemented
in terms of error handling.

The two functions I've mentioned above are
notmuch_database_find_message() and
notmuch_database_find_message_by_filename().

The problem with their design is NULL return may both mean an error
condition and "message not found". However, we already have a similar
function which does not have such a flaw, namely notmuch_database_add_message().

In my humble opinion it is a good idea to modify these functions to
return 'notmuch_status_t' and add an argument 'notmuch_message_t **'
which will be initialized to the message object upon successful return.
This is just like notmuch_database_add_message() which provides both
consistency and proper error reporting.

I vaguely remember this question was raised on the list for
notmuch_database_find_message() before. Seeing the recent addition of
notmuch_database_find_message_by_filename(), I wanted to bring this up
again in the hope to get it fixed as soon as possible.

I am not providing a patch here considering the simplicity of the
problem but if anyone needs elaboration, I will be happy to submit a
patch.

P.S.: Ruby bindings don't have wrappers for these functions but I am not
sure about Python bindings and how they solve, or 'hack around', this
problem. CC'ing Sebastian for comments so we can have consistency
between bindings.

        -alip

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

Re: Concerns regarding some library functions

[David Bremner]

On Tue, 27 Sep 2011 16:25:58 +0300, Ali Polatel <polatel@gmail.com> wrote:

> The problem with their design is NULL return may both mean an error
> condition and "message not found". However, we already have a similar
> function which does not have such a flaw, namely notmuch_database_add_message().

So, I take there is no way to distinguish those two outcomes? That does
sound bad. Looking at the code for notmuch-new, it looks like the return
value of notmuch_database_find_message_by_filename is used without
checking it for NULL.  Austin, can you comment on that at all?

> I am not providing a patch here considering the simplicity of the
> problem but if anyone needs elaboration, I will be happy to submit a
> patch.

Well, also all the places that call these functions in the library and
command line client would need to be modified, as well as the go and
python bindings.  So it isn't completely trivial. Nor is is terribly
difficult of course.

d

Re: Concerns regarding some library functions

[Austin Clements]

Quoth David Bremner on Sep 27 at  1:59 pm:
> On Tue, 27 Sep 2011 16:25:58 +0300, Ali Polatel <polatel@gmail.com> wrote:
> 
> > The problem with their design is NULL return may both mean an error
> > condition and "message not found". However, we already have a similar
> > function which does not have such a flaw, namely notmuch_database_add_message().
> 
> So, I take there is no way to distinguish those two outcomes? That does
> sound bad. Looking at the code for notmuch-new, it looks like the return
> value of notmuch_database_find_message_by_filename is used without
> checking it for NULL.  Austin, can you comment on that at all?

I'd be happy to distinguish these outcomes.  I did
notmuch_database_find_message_by_filename the way I did only to be
consistent with notmuch_database_find_message.  Since ndfmbf isn't
entrenched yet, now is a good time to change it.

The call in notmuch-new should check the return, though if it can't
find the message at that point, something has gone terribly wrong.
Segfaulting is never the answer, though.

Re: Concerns regarding some library functions

[Ali Polatel]

[-- Attachment #1: Type: text/plain, Size: 1490 bytes --]

On Tue, 27 Sep 2011 18:46:22 -0400, Austin Clements <amdragon@MIT.EDU> wrote:
> Quoth David Bremner on Sep 27 at  1:59 pm:
> > On Tue, 27 Sep 2011 16:25:58 +0300, Ali Polatel <polatel@gmail.com> wrote:
> > 
> > > The problem with their design is NULL return may both mean an error
> > > condition and "message not found". However, we already have a similar
> > > function which does not have such a flaw, namely notmuch_database_add_message().
> > 
> > So, I take there is no way to distinguish those two outcomes? That does
> > sound bad. Looking at the code for notmuch-new, it looks like the return
> > value of notmuch_database_find_message_by_filename is used without
> > checking it for NULL.  Austin, can you comment on that at all?
> 
> I'd be happy to distinguish these outcomes.  I did
> notmuch_database_find_message_by_filename the way I did only to be
> consistent with notmuch_database_find_message.  Since ndfmbf isn't
> entrenched yet, now is a good time to change it.

What about notmuch_database_find_message()? If we leave it as it is,
this will lead to inconsistency and if we change it, we need to think
about API breakage issues.

> The call in notmuch-new should check the return, though if it can't
> find the message at that point, something has gone terribly wrong.
> Segfaulting is never the answer, though.

Indeed, just not to step on each other's feet, are you going to write a
patch or shall I start writing one?

         -alip

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

Re: Concerns regarding some library functions

[Austin Clements]

Quoth Ali Polatel on Sep 28 at 10:53 am:
> On Tue, 27 Sep 2011 18:46:22 -0400, Austin Clements <amdragon@MIT.EDU> wrote:
> > Quoth David Bremner on Sep 27 at  1:59 pm:
> > > On Tue, 27 Sep 2011 16:25:58 +0300, Ali Polatel <polatel@gmail.com> wrote:
> > > 
> > > > The problem with their design is NULL return may both mean an error
> > > > condition and "message not found". However, we already have a similar
> > > > function which does not have such a flaw, namely notmuch_database_add_message().
> > > 
> > > So, I take there is no way to distinguish those two outcomes? That does
> > > sound bad. Looking at the code for notmuch-new, it looks like the return
> > > value of notmuch_database_find_message_by_filename is used without
> > > checking it for NULL.  Austin, can you comment on that at all?
> > 
> > I'd be happy to distinguish these outcomes.  I did
> > notmuch_database_find_message_by_filename the way I did only to be
> > consistent with notmuch_database_find_message.  Since ndfmbf isn't
> > entrenched yet, now is a good time to change it.
> 
> What about notmuch_database_find_message()? If we leave it as it is,
> this will lead to inconsistency and if we change it, we need to think
> about API breakage issues.

Yes.  We could just deal with that (there aren't *that* many API
consumers).  For binary compatibility, I suppose we could even use
symbol versioning.

> > The call in notmuch-new should check the return, though if it can't
> > find the message at that point, something has gone terribly wrong.
> > Segfaulting is never the answer, though.
> 
> Indeed, just not to step on each other's feet, are you going to write a
> patch or shall I start writing one?

Please feel free to write a patch.

Re: Concerns regarding some library functions

[David Bremner]

On Thu, 29 Sep 2011 10:51:29 -0400, Austin Clements <amdragon@MIT.EDU> wrote:

> Yes.  We could just deal with that (there aren't *that* many API
> consumers).  For binary compatibility, I suppose we could even use
> symbol versioning.

I noticed a similar remark in lib/Makefile.local. But I'm not sure how
this work if the interface of a given library function changed.  Can
someone point me to some more explanation? 

Of course we can always bump the soname of libnotmuch. It isn't that big
of a deal IMHO.

d

Re: Concerns regarding some library functions

[Austin Clements]

Quoth David Bremner on Sep 29 at  4:59 pm:
> On Thu, 29 Sep 2011 10:51:29 -0400, Austin Clements <amdragon@MIT.EDU> wrote:
> 
> > Yes.  We could just deal with that (there aren't *that* many API
> > consumers).  For binary compatibility, I suppose we could even use
> > symbol versioning.
> 
> I noticed a similar remark in lib/Makefile.local. But I'm not sure how
> this work if the interface of a given library function changed.  Can
> someone point me to some more explanation? 

With symbol versioning we'd still provide the old function (presumably
re-implemented in terms of the new function).  Both would wind up in
the .so and old binaries would still link against the old symbol.  It
doesn't help that much once something gets recompiled; assuming the
source isn't requesting a specific version of a symbol, it will try to
use the latest version.

That, however, is about the extent of my knowledge on symbol
versioning.  It's possible this simply doesn't work with symbols that
don't already have a version; I'm not sure.

Re: Concerns regarding some library functions

[David Bremner]

On Thu, 29 Sep 2011 16:15:36 -0400, Austin Clements <amdragon@MIT.EDU> wrote:
> With symbol versioning we'd still provide the old function (presumably
> re-implemented in terms of the new function).  Both would wind up in
> the .so and old binaries would still link against the old symbol.  It
> doesn't help that much once something gets recompiled; assuming the
> source isn't requesting a specific version of a symbol, it will try to
> use the latest version.
> 
> That, however, is about the extent of my knowledge on symbol
> versioning.  It's possible this simply doesn't work with symbols that
> don't already have a version; I'm not sure.

So I've pushed the ABI changes, making it more urgent to do something
about this.  At this point I'm inclined to bump the soname in order to
unbreak things, unless someone wants to come up with a convincing set of
patches to do the symbol versioning.

d

Re: Concerns regarding some library functions

[Tomi Ollila]

On Tue 04 Oct 2011 14:18, David Bremner <david@tethera.net> writes:

>
> So I've pushed the ABI changes, making it more urgent to do something
> about this.  At this point I'm inclined to bump the soname in order to
> unbreak things, unless someone wants to come up with a convincing set of
> patches to do the symbol versioning.

Can symbol versioning be done with 'C' symbols ?

>
> d

Tomi

libnotmuch SONAME bumped

[David Bremner]

On Tue, 04 Oct 2011 08:18:21 -0300, David Bremner <david@tethera.net> wrote:
> So I've pushed the ABI changes, making it more urgent to do something
> about this.  At this point I'm inclined to bump the soname in order to
> unbreak things, unless someone wants to come up with a convincing set of
> patches to do the symbol versioning.

I just pushed a bump of the libnotmuch soname, and corresponding changes
to debian packaging.

Let me know if something broke

d

Re: Concerns regarding some library functions

[Ali Polatel]

[-- Attachment #1: Type: text/plain, Size: 15556 bytes --]

Austin Clements yazmış:
>Quoth Ali Polatel on Sep 28 at 10:53 am:
>> On Tue, 27 Sep 2011 18:46:22 -0400, Austin Clements <amdragon@MIT.EDU> wrote:
>> > Quoth David Bremner on Sep 27 at  1:59 pm:
>> > > On Tue, 27 Sep 2011 16:25:58 +0300, Ali Polatel <polatel@gmail.com> wrote:
>> > >
>> > > > The problem with their design is NULL return may both mean an error
>> > > > condition and "message not found". However, we already have a similar
>> > > > function which does not have such a flaw, namely notmuch_database_add_message().
>> > >
>> > > So, I take there is no way to distinguish those two outcomes? That does
>> > > sound bad. Looking at the code for notmuch-new, it looks like the return
>> > > value of notmuch_database_find_message_by_filename is used without
>> > > checking it for NULL.  Austin, can you comment on that at all?
>> >
>> > I'd be happy to distinguish these outcomes.  I did
>> > notmuch_database_find_message_by_filename the way I did only to be
>> > consistent with notmuch_database_find_message.  Since ndfmbf isn't
>> > entrenched yet, now is a good time to change it.
>>
>> What about notmuch_database_find_message()? If we leave it as it is,
>> this will lead to inconsistency and if we change it, we need to think
>> about API breakage issues.
>
>Yes.  We could just deal with that (there aren't *that* many API
>consumers).  For binary compatibility, I suppose we could even use
>symbol versioning.
>
>> > The call in notmuch-new should check the return, though if it can't
>> > find the message at that point, something has gone terribly wrong.
>> > Segfaulting is never the answer, though.
>>
>> Indeed, just not to step on each other's feet, are you going to write a
>> patch or shall I start writing one?
>
>Please feel free to write a patch.

Below is a quick patch, which compiles and passes tests.
Please comment.

         -alip

-- >8 --
Subject: [PATCH] lib: make find_message{,by_filename) report errors

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   |   89 ++++++++++++++++++++++++++++++++++------------------
  lib/message.cc    |    6 ++--
  lib/notmuch.h     |   61 +++++++++++++++++++++++++-----------
  notmuch-new.c     |    4 ++-
  notmuch-restore.c |   11 ++++--
  5 files changed, 114 insertions(+), 57 deletions(-)

diff --git a/lib/database.cc b/lib/database.cc
index 9299c8d..6641aa5 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,20 @@ 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 +1321,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 +1330,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 +1366,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 +1461,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 +1777,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 +1793,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 +1822,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..3b0ae71 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

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

[PATCH] lib: make find_message{,by_filename) report errors

[Ali Polatel]

From: Ali Polatel <alip@exherbo.org>

Looks like the patch did not make it correctly the first time.
Resending using git-send-email™

You may also find the commit in my notmuch repository:
git://github.com/alip/notmuch.git branch: find_message

Ali Polatel (1):
  lib: make find_message{,by_filename) report errors

 lib/database.cc   |   89 ++++++++++++++++++++++++++++++++++------------------
 lib/message.cc    |    6 ++--
 lib/notmuch.h     |   61 +++++++++++++++++++++++++-----------
 notmuch-new.c     |    4 ++-
 notmuch-restore.c |   11 ++++--
 5 files changed, 114 insertions(+), 57 deletions(-)

-- 
1.7.6.1

[PATCH] lib: make find_message{,by_filename) report errors

[Ali Polatel]

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   |   89 ++++++++++++++++++++++++++++++++++------------------
 lib/message.cc    |    6 ++--
 lib/notmuch.h     |   61 +++++++++++++++++++++++++-----------
 notmuch-new.c     |    4 ++-
 notmuch-restore.c |   11 ++++--
 5 files changed, 114 insertions(+), 57 deletions(-)

diff --git a/lib/database.cc b/lib/database.cc
index 9299c8d..6641aa5 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,20 @@ 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 +1321,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 +1330,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 +1366,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 +1461,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 +1777,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 +1793,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 +1822,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..3b0ae71 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

Re: [PATCH] lib: make find_message{,by_filename) report errors

[Sebastian Spaeth]

[-- Attachment #1: Type: text/plain, Size: 142 bytes --]


The new API looks sane and much better to me.

+1, just give me plenty of time to catch up before releasing once this
goes in :-)

Sebastian

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

[PATCH v1 0/1] lib: make find_message{,by_filename) report errors

[Ali Polatel]

Here is an updated patch fixing style issues addressed by amdragon on
IRC. I plan to submit a patch documenting how to configure VIM to code
using notmuch's style guidelines as well.

You may also find the commit in my notmuch repository:
git://github.com/alip/notmuch.git branch: find_message-v1

Ali Polatel (1):
  lib: make find_message{,by_filename) report errors

 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(-)

-- 
1.7.6.1

[PATCH v1 1/1] lib: make find_message{,by_filename) report errors

[Ali Polatel]

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

Re: [PATCH v1 1/1] lib: make find_message{,by_filename) report errors

[Austin Clements]

All of the code looks good to me (and improving error handling is
always a good thing).  Some style nits are inline below.

The changes to _resolve_message_id_to_thread_id and
_notmuch_database_link_message_to_parents seem like just plain better
error handling and unrelated to the find_message API changes.  Perhaps
these should be in a separate patch?

Quoth Ali Polatel on Oct 03 at  7:49 pm:
> 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)

Perhaps this should be message_ret to clearly distinguish it as an
out-argument?

>  {
>      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)

thread_id_ret?

>  {
> +    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());

Missing space after talloc_strdup.

>      }
>  
>      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);

This isn't a problem with your patch, but shouldn't this function be
notmuch_message_destroy'ing 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)

message_ret?

>  {
>      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);

Long line, should probably be wrapped.

>      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

"If a message"

> + * (NOTMUCH_STATUS_SUCCESS) '*message' will be initialized to a message object.
> + * The user should call notmuch_message_destroy when done with the message.

Probably s/user/caller/, since "the user" is the person sitting at the
keyboard.  (notmuch.h isn't very good about this, but it does use
"caller" more often than "user").

> - * 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!

Since it's clearly specified what happens if 'message' is NULL, this
note isn't necessary.

> + *
> + * 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.

Same comment about "user" as above.

>   *
> - * 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!

Same comment about this note as above.

> + *
> + * 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;
>  	}
>  

-- 
Austin Clements                                      MIT/'06/PhD/CSAIL
amdragon@mit.edu                           http://web.mit.edu/amdragon
       Somewhere in the dream we call reality you will find me,
              searching for the reality we call dreams.

Re: [PATCH v1 1/1] lib: make find_message{, by_filename) report errors

[Ali Polatel]

[-- Attachment #1: Type: text/plain, Size: 16794 bytes --]

Austin Clements yazmış:
>All of the code looks good to me (and improving error handling is
>always a good thing).  Some style nits are inline below.

Thanks for the review, comments below and a new set of patches follow...

>The changes to _resolve_message_id_to_thread_id and
>_notmuch_database_link_message_to_parents seem like just plain better
>error handling and unrelated to the find_message API changes.  Perhaps
>these should be in a separate patch?

I don't think this is wise considering the fact that the prototypes of
the functions change. Separating this into two patches will leave the
state of the tree broken between the two patches which is bad for
git-bisect. In addition, considering the small size and compactness of
the patch, I don't think it's worth the effort.

>Quoth Ali Polatel on Oct 03 at  7:49 pm:
>> 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)
>
>Perhaps this should be message_ret to clearly distinguish it as an
>out-argument?

This crossed my mind while working on the initial patch but I guess I
was just being lazy. Fixed.

>>  {
>>      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)
>
>thread_id_ret?

Same as above, fixed.

>
>>  {
>> +    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());
>
>Missing space after talloc_strdup.

Oops, fixed.

>>      }
>>
>>      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);
>
>This isn't a problem with your patch, but shouldn't this function be
>notmuch_message_destroy'ing message?
>

Exactly! I will provide a separate patch for this.

>> @@ -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)
>
>message_ret?

Same as above, fixed.

>>  {
>>      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);
>
>Long line, should probably be wrapped.

Done:
(notmuch_private_status_t) notmuch_database_find_message
looks like a single entity to my brain which makes wrapping rather
pointless though...

>>      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
>
>"If a message"
>
>> + * (NOTMUCH_STATUS_SUCCESS) '*message' will be initialized to a message object.
>> + * The user should call notmuch_message_destroy when done with the message.
>
>Probably s/user/caller/, since "the user" is the person sitting at the
>keyboard.  (notmuch.h isn't very good about this, but it does use
>"caller" more often than "user").

As you wish sir, I can't say I care much about the wording.

>> - * 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!
>
>Since it's clearly specified what happens if 'message' is NULL, this
>note isn't necessary.

Fair enough, removed.

>> + *
>> + * 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.
>
>Same comment about "user" as above.

Same as above, fixed.

>>   *
>> - * 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!
>
>Same comment about this note as above.

Same as above, removed.

>> + *
>> + * 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;
>>  	}
>>
>
>-- 
>Austin Clements                                      MIT/'06/PhD/CSAIL
>amdragon@mit.edu                           http://web.mit.edu/amdragon
>       Somewhere in the dream we call reality you will find me,
>              searching for the reality we call dreams.


          -alip

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

[PATCH v2 0/2] Better error handling

[Ali Polatel]

From: Ali Polatel <alip@exherbo.org>

New set of patches addressing the issues in the mail
"id:20111003174308.GD17905@mit.edu"

Ali Polatel (2):
  lib: destroy message object after message removal
  lib: make find_message{,by_filename) report errors

 lib/database.cc   |   95 +++++++++++++++++++++++++++++++++++-----------------
 lib/message.cc    |    8 +++--
 lib/notmuch.h     |   60 +++++++++++++++++++++++----------
 notmuch-new.c     |    4 ++-
 notmuch-restore.c |   11 ++++--
 5 files changed, 120 insertions(+), 58 deletions(-)

-- 
1.7.6.1

[PATCH v2 1/2] lib: destroy message object after message removal

[Ali Polatel]

From: Ali Polatel <alip@exherbo.org>

notmuch_database_remove_message() must call notmuch_message_destroy()
once it is done handling message removal.
---
 lib/database.cc |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/lib/database.cc b/lib/database.cc
index 9299c8d..d43e114 100644
--- a/lib/database.cc
+++ b/lib/database.cc
@@ -1769,6 +1769,8 @@ notmuch_database_remove_message (notmuch_database_t *notmuch,
 		_notmuch_message_delete (message);
 	    else if (status == NOTMUCH_STATUS_DUPLICATE_MESSAGE_ID)
 		_notmuch_message_sync (message);
+
+	    notmuch_message_destroy (message);
     }
 
     return status;
-- 
1.7.6.1

[PATCH v2 2/2] lib: make find_message{,by_filename) report errors

[Ali Polatel]

From: Ali Polatel <alip@exherbo.org>

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   |   93 +++++++++++++++++++++++++++++++++++-----------------
 lib/message.cc    |    8 +++--
 lib/notmuch.h     |   60 +++++++++++++++++++++++-----------
 notmuch-new.c     |    4 ++-
 notmuch-restore.c |   11 ++++--
 5 files changed, 118 insertions(+), 58 deletions(-)

diff --git a/lib/database.cc b/lib/database.cc
index d43e114..cf0cc8a 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_ret)
 {
     notmuch_private_status_t status;
     unsigned int doc_id;
 
+    if (message_ret == 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_ret = NULL;
+	else {
+	    *message_ret = _notmuch_message_create (notmuch, notmuch, doc_id,
+						    NULL);
+	    if (*message_ret == 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_ret = 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,30 @@ _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_ret)
 {
+    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_ret = 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 +1368,16 @@ _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_ret = talloc_strdup (ctx,
+					_notmuch_database_generate_thread_id (notmuch));
+	db->set_metadata (metadata_key, *thread_id_ret);
     } else {
-	thread_id = thread_id_string.c_str();
+	*thread_id_ret = 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 +1464,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 +1780,13 @@ 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);
@@ -1776,24 +1799,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_ret)
 {
     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_ret == 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);
 
@@ -1802,19 +1828,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_ret = _notmuch_message_create (notmuch, notmuch, *i,
+						    &private_status);
+	    if (*message_ret == 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_ret = NULL;
+    return status;
 }
 
 notmuch_string_list_t *
diff --git a/lib/message.cc b/lib/message.cc
index 531d304..8f22e02 100644
--- a/lib/message.cc
+++ b/lib/message.cc
@@ -216,11 +216,13 @@ _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..9d7f8f5 100644
--- a/lib/notmuch.h
+++ b/lib/notmuch.h
@@ -286,7 +286,7 @@ notmuch_database_get_directory (notmuch_database_t *database,
  * If 'message' is not NULL, then, on successful return
  * (NOTMUCH_STATUS_SUCCESS or NOTMUCH_STATUS_DUPLICATE_MESSAGE_ID) '*message'
  * will be initialized to a message object that can be used for things
- * such as adding tags to the just-added message. The user should call
+ * such as adding tags to the just-added message. The caller should call
  * notmuch_message_destroy when done with the message. On any failure
  * '*message' will be set to NULL.
  *
@@ -347,35 +347,57 @@ 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 a message with the given message_id is found then, on successful return
+ * (NOTMUCH_STATUS_SUCCESS) '*message' will be initialized to a message
+ * object.  The caller 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
+ * caller 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
+ * 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 caller 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
+ * caller 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
+ * 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

Re: [PATCH v2 0/2] Better error handling

[Ali Polatel]

[-- Attachment #1: Type: text/plain, Size: 750 bytes --]

Ali Polatel yazmış:
>From: Ali Polatel <alip@exherbo.org>
>
>New set of patches addressing the issues in the mail
>"id:20111003174308.GD17905@mit.edu"

You may also find the commits in my notmuch repository:
git://github.com/alip/notmuch.git branch: find_message-v2

>Ali Polatel (2):
>  lib: destroy message object after message removal
>  lib: make find_message{,by_filename) report errors
>
> lib/database.cc   |   95 +++++++++++++++++++++++++++++++++++-----------------
> lib/message.cc    |    8 +++--
> lib/notmuch.h     |   60 +++++++++++++++++++++++----------
> notmuch-new.c     |    4 ++-
> notmuch-restore.c |   11 ++++--
> 5 files changed, 120 insertions(+), 58 deletions(-)
>
>-- 
>1.7.6.1
>


         -alip

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH v2 0/2] Better error handling

[Austin Clements]

LGTM.

Quoth Ali Polatel on Oct 03 at 11:40 pm:
> From: Ali Polatel <alip@exherbo.org>
> 
> New set of patches addressing the issues in the mail
> "id:20111003174308.GD17905@mit.edu"
> 
> Ali Polatel (2):
>   lib: destroy message object after message removal
>   lib: make find_message{,by_filename) report errors
> 
>  lib/database.cc   |   95 +++++++++++++++++++++++++++++++++++-----------------
>  lib/message.cc    |    8 +++--
>  lib/notmuch.h     |   60 +++++++++++++++++++++++----------
>  notmuch-new.c     |    4 ++-
>  notmuch-restore.c |   11 ++++--
>  5 files changed, 120 insertions(+), 58 deletions(-)

Re: [PATCH] lib: make find_message{,by_filename) report errors

[David Bremner]

On Sat,  1 Oct 2011 11:12:23 +0300, Ali Polatel <polatel@gmail.com> wrote:
> From: Ali Polatel <alip@exherbo.org>
> 
> Looks like the patch did not make it correctly the first time.
> Resending using git-send-email™
> 
> You may also find the commit in my notmuch repository:
> git://github.com/alip/notmuch.git branch: find_message


Hi Ali;

Thanks for reworking this patch. I looked at branch find_message-v2
in your repo. I have a few comments.

- In the comments for _resolve_message_id_to_thread_id I guess thread_id
  should be thread_id_ret?

- in notmuch_database_find_message_by_file_name, I'm not sure why you
  set status to NOTMUCH_STATUS_OUT_OF_MEMORY in the catch block. Is this
  a typo?

- after the DONE: label of the same routine, how is *message_ret destroyed?
  does it need to wait until the talloc context "notmuch" is freed?

- I don't really get the change of user to caller around notmuch.h:286
  It is not a big deal, but I guess we should try to be consistent.

David

 

Re: [PATCH] lib: make find_message{,by_filename) report errors

[Ali Polatel]

[-- Attachment #1: Type: text/plain, Size: 1538 bytes --]

David Bremner yazmış:
>On Sat,  1 Oct 2011 11:12:23 +0300, Ali Polatel <polatel@gmail.com> wrote:
>> From: Ali Polatel <alip@exherbo.org>
>>
>> Looks like the patch did not make it correctly the first time.
>> Resending using git-send-email™
>>
>> You may also find the commit in my notmuch repository:
>> git://github.com/alip/notmuch.git branch: find_message
>
>
>Hi Ali;
>
>Thanks for reworking this patch. I looked at branch find_message-v2
>in your repo. I have a few comments.

Thanks for going over the patch, expect a new set of patches soon!

>- In the comments for _resolve_message_id_to_thread_id I guess thread_id
>  should be thread_id_ret?

Fixed.

>- in notmuch_database_find_message_by_file_name, I'm not sure why you
>  set status to NOTMUCH_STATUS_OUT_OF_MEMORY in the catch block. Is this
>  a typo?

Looks like a copy & paste error. I must have blindly copied the error
from the previous block. Fixed.

>- after the DONE: label of the same routine, how is *message_ret destroyed?
>  does it need to wait until the talloc context "notmuch" is freed?

Yes, I have modified it to call notmuch_message_destroy() in case
'*message_ret' is non-NULL after the DONE:

>- I don't really get the change of user to caller around notmuch.h:286
>  It is not a big deal, but I guess we should try to be consistent.

I don't get what you mean by consistency here but this hunk is unrelated
to the problem which the patch is trying to address.
Reverted.

>David
>
>

         -alip

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

[PATCH v3 0/2] Better error handling

[Ali Polatel]

From: Ali Polatel <alip@exherbo.org>

New set of patches addressing the issues in the mail
"id:8739f9muhp.fsf@zancas.localnet"

You may also find the commits in my notmuch repository:
git://github.com/alip/notmuch.git branch: find_message-v3

Ali Polatel (2):
  lib: destroy message object after message removal
  lib: make find_message{,by_filename) report errors

 lib/database.cc   |   98 ++++++++++++++++++++++++++++++++++++-----------------
 lib/message.cc    |    8 +++--
 lib/notmuch.h     |   58 +++++++++++++++++++++----------
 notmuch-new.c     |    4 ++-
 notmuch-restore.c |   11 ++++--
 5 files changed, 122 insertions(+), 57 deletions(-)

-- 
1.7.6.1

[PATCH v3 1/2] lib: destroy message object after message removal

[Ali Polatel]

From: Ali Polatel <alip@exherbo.org>

notmuch_database_remove_message() must call notmuch_message_destroy()
once it is done handling message removal.
---
 lib/database.cc |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/lib/database.cc b/lib/database.cc
index 9299c8d..d43e114 100644
--- a/lib/database.cc
+++ b/lib/database.cc
@@ -1769,6 +1769,8 @@ notmuch_database_remove_message (notmuch_database_t *notmuch,
 		_notmuch_message_delete (message);
 	    else if (status == NOTMUCH_STATUS_DUPLICATE_MESSAGE_ID)
 		_notmuch_message_sync (message);
+
+	    notmuch_message_destroy (message);
     }
 
     return status;
-- 
1.7.6.1

[PATCH v3 2/2] lib: make find_message{,by_filename) report errors

[Ali Polatel]

From: Ali Polatel <alip@exherbo.org>

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   |   96 ++++++++++++++++++++++++++++++++++++-----------------
 lib/message.cc    |    8 +++--
 lib/notmuch.h     |   58 ++++++++++++++++++++++----------
 notmuch-new.c     |    4 ++-
 notmuch-restore.c |   11 ++++--
 5 files changed, 120 insertions(+), 57 deletions(-)

diff --git a/lib/database.cc b/lib/database.cc
index d43e114..e77fd53 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_ret)
 {
     notmuch_private_status_t status;
     unsigned int doc_id;
 
+    if (message_ret == 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_ret = NULL;
+	else {
+	    *message_ret = _notmuch_message_create (notmuch, notmuch, doc_id,
+						    NULL);
+	    if (*message_ret == 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_ret = NULL;
+	return NOTMUCH_STATUS_XAPIAN_EXCEPTION;
     }
 }
 
@@ -1311,7 +1322,9 @@ _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_ret' must not be NULL!
+ * On success '*thread_id_ret' 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 +1332,30 @@ _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_ret)
 {
+    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_ret = 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 +1369,16 @@ _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_ret = talloc_strdup (ctx,
+					_notmuch_database_generate_thread_id (notmuch));
+	db->set_metadata (metadata_key, *thread_id_ret);
     } else {
-	thread_id = thread_id_string.c_str();
+	*thread_id_ret = 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 +1465,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 +1781,13 @@ 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);
@@ -1776,24 +1800,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_ret)
 {
     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_ret == 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);
 
@@ -1802,19 +1829,26 @@ 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_ret = _notmuch_message_create (notmuch, notmuch, *i,
+						    &private_status);
+	    if (*message_ret == 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_XAPIAN_EXCEPTION;
     }
 
+  DONE:
     talloc_free (local);
 
-    return message;
+    if (status && *message_ret) {
+	notmuch_message_destroy (*message_ret);
+	*message_ret = NULL;
+    }
+    return status;
 }
 
 notmuch_string_list_t *
diff --git a/lib/message.cc b/lib/message.cc
index 531d304..8f22e02 100644
--- a/lib/message.cc
+++ b/lib/message.cc
@@ -216,11 +216,13 @@ _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..c4330e4 100644
--- a/lib/notmuch.h
+++ b/lib/notmuch.h
@@ -347,35 +347,57 @@ 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 a message with the given message_id is found then, on successful return
+ * (NOTMUCH_STATUS_SUCCESS) '*message' will be initialized to a message
+ * object.  The caller 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
+ * caller 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
+ * 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 caller 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
+ * caller 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
+ * 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

Re: [PATCH v3 0/2] Better error handling

[David Bremner]

On Tue,  4 Oct 2011 08:06:07 +0300, Ali Polatel <polatel@gmail.com> wrote:
> From: Ali Polatel <alip@exherbo.org>
> 
> New set of patches addressing the issues in the mail
> "id:8739f9muhp.fsf@zancas.localnet"
> 
> You may also find the commits in my notmuch repository:
> git://github.com/alip/notmuch.git branch: find_message-v3
> 

I have pushed your revised version, thanks!

d

Re: Concerns regarding some library functions

[Sebastian Spaeth]

[-- Attachment #1: Type: text/plain, Size: 925 bytes --]

On Tue, 27 Sep 2011 16:25:58 +0300, Ali Polatel wrote:
> The two functions I've mentioned above are
> notmuch_database_find_message() and
> notmuch_database_find_message_by_filename().
> 
> The problem with their design is NULL return may both mean an error
> condition and "message not found". However, we already have a similar
> function which does not have such a flaw, namely notmuch_database_add_message().

Yes, this is because NULL used to mean message not found and NULL means
error was tacked on later (because cworth did not expect that xapian
would actually throw errors that often (such as database modified and
whatnot). So the meaning of NULL is, ahhem, suboptimal...

ERROR or really not there? There are mails from me, commenting on
that. I would welcome receiving back a notmuch_status_t value and have a
separate notmuch_message_t parameter which receives the actual message.

Sebastian

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]