Hallo!

After having read about it in ..., in ..., (don't know anymore), and then
again over at LWN, I have had the plan to try (or rather: directly switch
to) notmuch like half a year ago, or longer.  I eventually was brave
enough to begin that process in the last days of 2010.  I'm still with
you, as you can see.  Hooray!  :-)

As I said on IRC already: there are, of course, still some rough edges,
but what else should I expect from such a young project.


I'm currently using getmail, then maildrop which invokes notmuch-deliver
for getting my emails into both maildir storage, and making them known to
notmuch.  In that process I can apply tags at will (based on List-Id: /
Mailing-List: headers, mostly, but also for possibly-spam, and so on),
and then use a script that operates on these to further massage them.
Thus, I don't really have a need for ``notmuch new'' anymore, and
notmuch-deliver is sort of an atomic / integrated ``put into maildir and
add tags'' tool for me.  I will post more details on my setup (or rather:
directly put it into the wiki), as soon as all this settles down some
more.

I have not yet switched all my email channels to notmuch, but will do so
soon, I think.

Likewise, there are some topics I'd like to discuss.  I'll also begin to
plow through notmuch's mailing list archive, and will gladly accept
id:xxx pointers that answer / discuss the questions I'll be sending.


I poked at notmuch-deliver's code two weeks ago already (Ali, beware,
there'll be some few further patches coming), and in the last hours, I
finally had a look at notmuch.h and some of the source code.  Here is a
diff containing some comments, or to-do items.  Not all are fully fledged
(I have neither been using talloc, nor Xapian before), but perhaps
someone nevertheless feels like commenting on them.  In general I can
say, that the notmuch code makes a solid impression.  :-)


diff --git a/emacs/notmuch-maildir-fcc.el b/emacs/notmuch-maildir-fcc.el
index 3f1c124..0d7017c 100644
--- a/emacs/notmuch-maildir-fcc.el
+++ b/emacs/notmuch-maildir-fcc.el
@@ -16,6 +16,14 @@
 ;; To use this as the fcc handler for message-mode,
 ;; customize the notmuch-fcc-dirs variable
 
+;; TODO.  A bunch of maildir code is not specific to fcc handling and should be
+;; factored out.
+
+;; TODO.  In fact, apart from using notmuch-database-path (which should
+;; probably be message-directory instead, anyways?), this whole unit is not
+;; specific to notmuch, and should perhaps be integrated into Emacs'
+;; message.el -- or is there even anything in Gnus that can directly be used?
+
 (eval-when-compile (require 'cl))
 (require 'message)
 
@@ -73,7 +81,7 @@ yet when sending a mail."
 (defun notmuch-fcc-header-setup ()
   "Add an Fcc header to the current message buffer.
 
-Can be added to `message-send-hook' and will set the Fcc header
+Is by default added to `message-header-setup-hook' and will set the Fcc header
 based on the values of `notmuch-fcc-dirs'. An existing Fcc header
 will NOT be removed or replaced."
 
@@ -163,7 +171,7 @@ will NOT be removed or replaced."
 	 (make-directory (concat path "/new/") t)
 	 (make-directory (concat path "/tmp/") t))
 	((file-regular-p path)
-	 (error "%s is a file. Can't creat maildir." path))
+	 (error "%s is a file. Can't create maildir." path))
 	(t
 	 (error "I don't know how to create a maildir here"))))
 
@@ -173,6 +181,8 @@ if successful, nil if not."
   (let ((msg-id (notmuch-maildir-fcc-make-uniq-maildir-id)))
     (while (file-exists-p (concat destdir "/tmp/" msg-id))
       (setq msg-id (notmuch-maildir-fcc-make-uniq-maildir-id)))
+;; TODO.  Race.  Should instead (try to) open (create) the file exclusively,
+;; until that succeeds.
     (cond ((notmuch-maildir-fcc-dir-is-maildir-p destdir)
 	   (write-file (concat destdir "/tmp/" msg-id))
 	   msg-id)
diff --git a/lib/database.cc b/lib/database.cc
index 7a00917..b08c429 100644
--- a/lib/database.cc
+++ b/lib/database.cc
@@ -551,7 +551,10 @@ notmuch_database_create (const char *path)
 
     notmuch = notmuch_database_open (path,
 				     NOTMUCH_DATABASE_MODE_READ_WRITE);
+    /* XXX: should check 'notmuch'.  */
+
     notmuch_database_upgrade (notmuch, NULL, NULL);
+    /* XXX: should check return value.  */
 
   DONE:
     if (notmuch_path)
@@ -760,18 +763,6 @@ handle_sigalrm (unused (int signal))
     do_progress_notify = 1;
 }
 
-/* Upgrade the current database.
- *
- * After opening a database in read-write mode, the client should
- * check if an upgrade is needed (notmuch_database_needs_upgrade) and
- * if so, upgrade with this function before making any modifications.
- *
- * The optional progress_notify callback can be used by the caller to
- * provide progress indication to the user. If non-NULL it will be
- * called periodically with 'count' as the number of messages upgraded
- * so far and 'total' the overall number of messages that will be
- * converted.
- */
 notmuch_status_t
 notmuch_database_upgrade (notmuch_database_t *notmuch,
 			  void (*progress_notify) (void *closure,
@@ -1012,6 +1003,7 @@ _notmuch_database_get_directory_db_path (const char *path)
  * is an empty string, then both directory and basename will be
  * returned as NULL.
  */
+/* XXX: isn't there a standard libc function that can be used?  */
 notmuch_status_t
 _notmuch_database_split_path (void *ctx,
 			      const char *path,
@@ -1141,6 +1133,7 @@ _notmuch_database_filename_to_direntry (void *ctx,
 	return status;
 
     *direntry = talloc_asprintf (ctx, "%u:%s", directory_id, basename);
+    /* XXX: *direntry != NULL?  */
 
     return NOTMUCH_STATUS_SUCCESS;
 }
@@ -1168,6 +1161,9 @@ _notmuch_database_relative_path (notmuch_database_t *notmuch,
 	while (*relative == '/' && *(relative+1) == '/')
 	    relative++;
 
+        /* XXX: canonicalize paths (symlinks, `x/./y, `x/../y') before
+           comparing?  */
+
 	if (strncmp (relative, db_path, db_path_len) == 0)
 	{
 	    relative += db_path_len;
@@ -1601,6 +1597,8 @@ notmuch_database_add_message (notmuch_database_t *notmuch,
 
 	    /* If a message ID is too long, substitute its sha1 instead. */
 	    if (message_id && strlen (message_id) > NOTMUCH_MESSAGE_ID_MAX) {
+              /* XXX: How to distinguish this one later on (or don't we have
+                 to?) from the message-id (possibly) generated below?  */
 		char *compressed = _message_id_compressed (message_file,
 							   message_id);
 		talloc_free (message_id);
@@ -1619,6 +1617,8 @@ notmuch_database_add_message (notmuch_database_t *notmuch,
 		goto DONE;
 	    }
 
+            /* XXX: How to distinguish this one later on (or don't we have to?)
+               from the message-id (possibly) generated above?  */
 	    message_id = talloc_asprintf (message_file,
 					  "notmuch-sha1-%s", sha1);
 	    free (sha1);
@@ -1641,6 +1641,7 @@ notmuch_database_add_message (notmuch_database_t *notmuch,
 	}
 
 	_notmuch_message_add_filename (message, filename);
+        /* XXX: check return value.  */
 
 	/* Is this a newly created message object? */
 	if (private_status == NOTMUCH_PRIVATE_STATUS_NO_DOCUMENT_FOUND) {
@@ -1655,6 +1656,7 @@ notmuch_database_add_message (notmuch_database_t *notmuch,
 	    _notmuch_message_set_date (message, date);
 
 	    _notmuch_message_index_file (message, filename);
+            /* XXX: check return value.  */
 	} else {
 	    ret = NOTMUCH_STATUS_DUPLICATE_MESSAGE_ID;
 	}
@@ -1735,11 +1737,15 @@ notmuch_database_remove_message (notmuch_database_t *notmuch,
 		status = NOTMUCH_STATUS_DUPLICATE_MESSAGE_ID;
 	    }
 	}
+        
+        /* XXX: talloc_free (term); */
     } catch (const Xapian::Error &error) {
 	fprintf (stderr, "Error: A Xapian exception occurred removing message: %s\n",
 		 error.get_msg().c_str());
 	notmuch->exception_reported = TRUE;
 	status = NOTMUCH_STATUS_XAPIAN_EXCEPTION;
+
+        /* XXX: (conditionally) talloc_free (term); */
     }
 
     talloc_free (local);
diff --git a/lib/directory.cc b/lib/directory.cc
index 946be4f..925b1da 100644
--- a/lib/directory.cc
+++ b/lib/directory.cc
@@ -140,6 +140,7 @@ _notmuch_directory_create (notmuch_database_t *notmuch,
 	    char *term = talloc_asprintf (local, "%s%s",
 					  _find_prefix ("directory"), db_path);
 	    directory->doc.add_term (term, 0);
+            /* XXX?: talloc_free (term); */
 
 	    directory->doc.set_data (path);
 
@@ -152,6 +153,7 @@ _notmuch_directory_create (notmuch_database_t *notmuch,
 					_find_prefix ("directory-direntry"),
 					parent_id, basename);
 		directory->doc.add_term (term, 0);
+                /* XXX?: talloc_free (term); */
 	    }
 
 	    directory->doc.add_value (NOTMUCH_VALUE_TIMESTAMP,
diff --git a/lib/message.cc b/lib/message.cc
index adcd07d..cf4e36a 100644
--- a/lib/message.cc
+++ b/lib/message.cc
@@ -187,13 +187,14 @@ _notmuch_message_create (const void *talloc_owner,
  *     There is already a document with message ID 'message_id' in the
  *     database. The returned message can be used to query/modify the
  *     document.
+ *
  *   NOTMUCH_PRIVATE_STATUS_NO_DOCUMENT_FOUND:
  *
  *     No document with 'message_id' exists in the database. The
  *     returned message contains a newly created document (not yet
  *     added to the database) and a document ID that is known not to
  *     exist in the database. The caller can modify the message, and a
- *     call to _notmuch_message_sync will add * the document to the
+ *     call to _notmuch_message_sync will add the document to the
  *     database.
  *
  * If an error occurs, this function will return NULL and *status
@@ -449,8 +450,10 @@ _notmuch_message_add_filename (notmuch_message_t *message,
     status = _notmuch_database_filename_to_direntry (local,
 						     message->notmuch,
 						     filename, &direntry);
-    if (status)
+    if (status) {
+        /* XXX: talloc_free (local); */
 	return status;
+    }
 
     _notmuch_message_add_term (message, "file-direntry", direntry);
 
@@ -717,6 +720,7 @@ _notmuch_message_close (notmuch_message_t *message)
  *
  * This change will not be reflected in the database until the next
  * call to _notmuch_message_sync. */
+/* XXX: basically nowehere this function's return value is checked.  */
 notmuch_private_status_t
 _notmuch_message_add_term (notmuch_message_t *message,
 			   const char *prefix_name,
@@ -730,9 +734,12 @@ _notmuch_message_add_term (notmuch_message_t *message,
 
     term = talloc_asprintf (message, "%s%s",
 			    _find_prefix (prefix_name), value);
+    /* XXX: term != NULL?  */
 
-    if (strlen (term) > NOTMUCH_TERM_MAX)
+    if (strlen (term) > NOTMUCH_TERM_MAX) {
+        /* XXX: talloc_free (term); */
 	return NOTMUCH_PRIVATE_STATUS_TERM_TOO_LONG;
+    }
 
     message->doc.add_term (term, 0);
 
@@ -820,6 +827,7 @@ notmuch_message_add_tag (notmuch_message_t *message, const char *tag)
     if (strlen (tag) > NOTMUCH_TAG_MAX)
 	return NOTMUCH_STATUS_TAG_TOO_LONG;
 
+    /* XXX: what if tag is already present -- added again?  */
     private_status = _notmuch_message_add_term (message, "tag", tag);
     if (private_status) {
 	INTERNAL_ERROR ("_notmuch_message_add_term return unexpected value: %d\n",
diff --git a/lib/notmuch.h b/lib/notmuch.h
index e508309..ffc7f8f 100644
--- a/lib/notmuch.h
+++ b/lib/notmuch.h
@@ -810,6 +810,7 @@ notmuch_message_set_flag (notmuch_message_t *message,
  * For the original textual representation of the Date header from the
  * message call notmuch_message_get_header() with a header value of
  * "date". */
+/* XXX: what if Date: was missing?  */
 time_t
 notmuch_message_get_date  (notmuch_message_t *message);
 
@@ -866,6 +867,8 @@ notmuch_message_get_tags (notmuch_message_t *message);
 
 /* Add a tag to the given message.
  *
+ * XXX: which are valid characters for a tag?
+ *
  * Return value:
  *
  * NOTMUCH_STATUS_SUCCESS: Tag successfully added to message
@@ -1112,7 +1115,7 @@ notmuch_tags_destroy (notmuch_tags_t *tags);
  *
  *   o Read the mtime of a directory from the filesystem
  *
- *   o Call add_message for all mail files in the directory
+ *   o Call notmuch_database_add_message for all mail files in the directory
  *
  *   o Call notmuch_directory_set_mtime with the mtime read from the
  *     filesystem.
@@ -1157,7 +1160,7 @@ notmuch_directory_get_mtime (notmuch_directory_t *directory);
 notmuch_filenames_t *
 notmuch_directory_get_child_files (notmuch_directory_t *directory);
 
-/* Get a notmuch_filenams_t iterator listing all the filenames of
+/* Get a notmuch_filenames_t iterator listing all the filenames of
  * sub-directories in the database within the given directory.
  *
  * The returned filenames will be the basename-entries only (not
diff --git a/notmuch-new.c b/notmuch-new.c
index cdf8513..74a09bf 100644
--- a/notmuch-new.c
+++ b/notmuch-new.c
@@ -784,6 +784,7 @@ notmuch_new_command (void *ctx, int argc, char *argv[])
 	    gettimeofday (&add_files_state.tv_start, NULL);
 	    notmuch_database_upgrade (notmuch, upgrade_print_progress,
 				      &add_files_state);
+            /* XXX: should check return value.  */
 	    printf ("Your notmuch database has now been upgraded to database format version %u.\n",
 		    notmuch_database_get_version (notmuch));
 	}
diff --git a/notmuch-restore.c b/notmuch-restore.c
index f095f64..2c607c3 100644
--- a/notmuch-restore.c
+++ b/notmuch-restore.c
@@ -115,7 +115,10 @@ notmuch_restore_command (unused (void *ctx), int argc, char *argv[])
 	}
 
 	notmuch_message_freeze (message);
+        /* XXX: check return value.  */
+
 	notmuch_message_remove_all_tags (message);
+        /* XXX: check return value.  */
 
 	next = file_tags;
 	while (next) {
@@ -133,6 +136,7 @@ notmuch_restore_command (unused (void *ctx), int argc, char *argv[])
 	}
 
 	notmuch_message_thaw (message);
+        /* XXX: check return value.  */
 
 	if (synchronize_flags)
 	    notmuch_message_tags_to_maildir_flags (message);
diff --git a/notmuch-tag.c b/notmuch-tag.c
index 60e21e0..3b7a00c 100644
--- a/notmuch-tag.c
+++ b/notmuch-tag.c
@@ -120,6 +120,7 @@ notmuch_tag_command (void *ctx, unused (int argc), unused (char *argv[]))
 	message = notmuch_messages_get (messages);
 
 	notmuch_message_freeze (message);
+        /* XXX: check return value.  */
 
 	for (i = 0; i < remove_tags_count; i++)
 	    notmuch_message_remove_tag (message,
@@ -129,6 +130,7 @@ notmuch_tag_command (void *ctx, unused (int argc), unused (char *argv[]))
 	    notmuch_message_add_tag (message, argv[add_tags[i]] + 1);
 
 	notmuch_message_thaw (message);
+        /* XXX: check return value.  */
 
 	if (synchronize_flags)
 	    notmuch_message_tags_to_maildir_flags (message);


Grüße,
 Thomas