]> git.cworth.org Git - obsolete/notmuch-old/commitdiff
Merge branch to fix broken "notmuch setup" and "notmuch new"
authorCarl Worth <cworth@cworth.org>
Tue, 27 Oct 2009 23:12:04 +0000 (16:12 -0700)
committerCarl Worth <cworth@cworth.org>
Tue, 27 Oct 2009 23:12:08 +0000 (16:12 -0700)
I'm trying to stick to a habit of fixing previously-introduced bugs
on side branches off of the commit that introduced the bug. The
idea here is to make it easy to find the commits to cherry pick
if bisecting in the future lands on one of the broken commits.

TODO
database.cc
message.cc
notmuch.c
notmuch.h

diff --git a/TODO b/TODO
index 8f9a1a98c5ae13e925c30bcc90777d64525abec0..fa85eb7fdc8685da42e63d3dbaa203980b26cd30 100644 (file)
--- a/TODO
+++ b/TODO
@@ -1,12 +1,52 @@
+Write a "notmuch tag" command to add/remove tags from messages
+matching a search query.
+
+Rename notmuch_thread_results_t and notmuch_message_results_t to
+notmuch_threads_t and notmuch_messages_t respectively.
+
+Add a talloc context as the first argument to each command in
+notmuch.c.
+
 Write a "notmuch show" that displays a single thread.
 
 Fix to use the *last* Message-ID header if multiple such headers are
 encountered, (I noticed this is one thing that kept me from seeing the
 same message-ID values as sup).
 
-Fix "notmuch restore" to delete the old tags from a message/thread
-before adding new ones. This will require someway to temporarily
-'disconnect' a notmuch_message_t from the database, (that it, disable
-automatic sync for add_tag, etc.), and then reconnect it. That is, the
-removal and subsequent addition of tags to the message/thread needs to
-be transactional.
+Audit everything for dealing with out-of-memory (and drop xutil.c).
+
+Write a test suite.
+
+Achieve 100% test coverage with the test suite.
+
+Think about this race condition:
+
+       A client executes "notmuch search"
+       Then executes "notmuch show" on a thread
+       While user is reading, new mail is added to database for the thread
+       Client asks for the thread to be archived.
+
+   The bug here is that email that was never read will be
+   archived. That's bad. The fix for the above is for the client to
+   archive the individual messages already retrieved and shown, not
+   the thread. (And in fact, we don't even have functions for removing
+   tags on threads.)
+
+   But this one is harder to fix:
+
+       A client executes "notmuch search"
+       While user is reading, new mail is added to database for the thread
+       Client asks for a thread to be archived.
+
+   To support this operation, (archiving a thread without even seeing
+   the individual messages), we might need to provide a command to
+   archive a thread as a whole. The problem is actually easy to fix
+   for a persistent client. It can onto the originally retrieved
+   thread objects which can hold onto the originally retrieved
+   messages. So archiving those thread objects, (and not newly created
+   thread objects), will be safe.
+
+   It's harder to fix the non-persistent "notmuch" client. One
+   approach is to simply tell the user to not run "notmuch new"
+   between reading the results of "notmuch search" and executing
+   "notmuch archive-thread" (or whatever we name it).
index ad91a7d72c8c3521c9e6588c1914af331b8ddd01..9831d79cdf912f50da160289268c1db504d6d836 100644 (file)
@@ -166,6 +166,8 @@ notmuch_status_to_string (notmuch_status_t status)
        return "Erroneous NULL pointer";
     case NOTMUCH_STATUS_TAG_TOO_LONG:
        return "Tag value is too long (exceeds NOTMUCH_TAG_MAX)";
+    case NOTMUCH_STATUS_UNBALANCED_FREEZE_THAW:
+       return "Unblanced number of calls to notmuch_message_freeze/thaw";
     default:
     case NOTMUCH_STATUS_LAST_STATUS:
        return "Unknown error status value";
index 6b6141f33fa4dbbd423866c77b1e9eb7c4dfaf6a..6e15b51118be82b960fb13aae667241fdbf36048 100644 (file)
@@ -26,6 +26,7 @@
 struct _notmuch_message {
     notmuch_database_t *notmuch;
     Xapian::docid doc_id;
+    int frozen;
     char *message_id;
     char *thread_id;
     char *filename;
@@ -33,7 +34,6 @@ struct _notmuch_message {
     Xapian::Document doc;
 };
 
-
 /* "128 bits of thread-id ought to be enough for anybody" */
 #define NOTMUCH_THREAD_ID_BITS  128
 #define NOTMUCH_THREAD_ID_DIGITS (NOTMUCH_THREAD_ID_BITS / 4)
@@ -100,6 +100,8 @@ _notmuch_message_create (const void *talloc_owner,
     message->notmuch = notmuch;
     message->doc_id = doc_id;
 
+    message->frozen = 0;
+
     /* Each of these will be lazily created as needed. */
     message->message_id = NULL;
     message->thread_id = NULL;
@@ -487,7 +489,8 @@ notmuch_message_add_tag (notmuch_message_t *message, const char *tag)
                        status);
     }
 
-    _notmuch_message_sync (message);
+    if (! message->frozen)
+       _notmuch_message_sync (message);
 
     return NOTMUCH_STATUS_SUCCESS;
 }
@@ -509,11 +512,55 @@ notmuch_message_remove_tag (notmuch_message_t *message, const char *tag)
                        status);
     }
 
-    _notmuch_message_sync (message);
+    if (! message->frozen)
+       _notmuch_message_sync (message);
 
     return NOTMUCH_STATUS_SUCCESS;
 }
 
+void
+notmuch_message_remove_all_tags (notmuch_message_t *message)
+{
+    notmuch_private_status_t status;
+    notmuch_tags_t *tags;
+    const char *tag;
+
+    for (tags = notmuch_message_get_tags (message);
+        notmuch_tags_has_more (tags);
+        notmuch_tags_advance (tags))
+    {
+       tag = notmuch_tags_get (tags);
+
+       status = _notmuch_message_remove_term (message, "tag", tag);
+       if (status) {
+           INTERNAL_ERROR ("_notmuch_message_remove_term return unexpected value: %d\n",
+                           status);
+       }
+    }
+
+    if (! message->frozen)
+       _notmuch_message_sync (message);
+}
+
+void
+notmuch_message_freeze (notmuch_message_t *message)
+{
+    message->frozen++;
+}
+
+notmuch_status_t
+notmuch_message_thaw (notmuch_message_t *message)
+{
+    if (message->frozen > 0) {
+       message->frozen--;
+       if (message->frozen == 0)
+           _notmuch_message_sync (message);
+       return NOTMUCH_STATUS_SUCCESS;
+    } else {
+       return NOTMUCH_STATUS_UNBALANCED_FREEZE_THAW;
+    }
+}
+
 void
 notmuch_message_destroy (notmuch_message_t *message)
 {
index 0fc4433b8325a298e439b65c78ec48b3c7df05f9..a7559fc7a6266a034f6b8580819ff0ca9a2be1e9 100644 (file)
--- a/notmuch.c
+++ b/notmuch.c
@@ -286,6 +286,7 @@ add_files_recursive (notmuch_database_t *notmuch,
                    case NOTMUCH_STATUS_FILE_ERROR:
                    case NOTMUCH_STATUS_NULL_POINTER:
                    case NOTMUCH_STATUS_TAG_TOO_LONG:
+                   case NOTMUCH_STATUS_UNBALANCED_FREEZE_THAW:
                    case NOTMUCH_STATUS_LAST_STATUS:
                        INTERNAL_ERROR ("add_message returned unexpected value: %d",  status);
                        goto DONE;
@@ -854,35 +855,34 @@ restore_command (int argc, char *argv[])
 
            message = notmuch_database_find_message (notmuch, message_id);
            if (message == NULL) {
-               fprintf (stderr, "Warning: Cannot apply tags to missing message: %s (",
+               fprintf (stderr, "Warning: Cannot apply tags to missing message: %s\n",
                         message_id);
+               goto NEXT_LINE;
            }
 
+           notmuch_message_freeze (message);
+
+           notmuch_message_remove_all_tags (message);
+
            next = tags;
            while (next) {
                tag = strsep (&next, " ");
                if (*tag == '\0')
                    continue;
-               if (message) {
-                   status = notmuch_message_add_tag (message, tag);
-                   if (status) {
-                       fprintf (stderr,
-                                "Error applying tag %s to message %s:\n",
-                                tag, message_id);
-                       fprintf (stderr, "%s\n",
-                                notmuch_status_to_string (status));
-                   }
-               } else {
-                   fprintf (stderr, "%s%s",
-                            tag == tags ? "" : " ", tag);
+               status = notmuch_message_add_tag (message, tag);
+               if (status) {
+                   fprintf (stderr,
+                            "Error applying tag %s to message %s:\n",
+                            tag, message_id);
+                   fprintf (stderr, "%s\n",
+                            notmuch_status_to_string (status));
                }
            }
 
-           if (message)
-               notmuch_message_destroy (message);
-           else
-               fprintf (stderr, ")\n");
+           notmuch_message_thaw (message);
+           notmuch_message_destroy (message);
        }
+      NEXT_LINE:
        free (message_id);
        free (tags);
     }
index 5e7d1240276214aa9970e86a9afddc6a80173441..7e83d23941be6e9309e63b0edeed25f83da29640 100644 (file)
--- a/notmuch.h
+++ b/notmuch.h
@@ -75,6 +75,11 @@ typedef int notmuch_bool_t;
  * NOTMUCH_STATUS_TAG_TOO_LONG: A tag value is too long (exceeds
  *     NOTMUCH_TAG_MAX)
  *
+ * NOTMUCH_STATUS_UNBALANCED_FREEZE_THAW: The notmuch_message_thaw
+ *     function has been called more times than notmuch_message_freeze.
+ *
+ * And finally:
+ *
  * NOTMUCH_STATUS_LAST_STATUS: Not an actual status value. Just a way
  *     to find out how many valid status values there are.
  */
@@ -87,6 +92,7 @@ typedef enum _notmuch_status {
     NOTMUCH_STATUS_DUPLICATE_MESSAGE_ID,
     NOTMUCH_STATUS_NULL_POINTER,
     NOTMUCH_STATUS_TAG_TOO_LONG,
+    NOTMUCH_STATUS_UNBALANCED_FREEZE_THAW,
 
     NOTMUCH_STATUS_LAST_STATUS
 } notmuch_status_t;
@@ -650,8 +656,8 @@ notmuch_message_get_tags (notmuch_message_t *message);
  *
  * NOTMUCH_STATUS_NULL_POINTER: The 'tag' argument is NULL
  *
- * NOTMUCH_STATUS_TAG_TOO_LONG: The length of 'tag' is longer than
- *     too long (exceeds NOTMUCH_TAG_MAX)
+ * NOTMUCH_STATUS_TAG_TOO_LONG: The length of 'tag' is too long
+ *     (exceeds NOTMUCH_TAG_MAX)
  */
 notmuch_status_t
 notmuch_message_add_tag (notmuch_message_t *message, const char *tag);
@@ -660,16 +666,83 @@ notmuch_message_add_tag (notmuch_message_t *message, const char *tag);
  *
  * Return value:
  *
- * NOTMUCH_STATUS_SUCCESS: Tag successfully added to message
+ * NOTMUCH_STATUS_SUCCESS: Tag successfully removed from message
  *
  * NOTMUCH_STATUS_NULL_POINTER: The 'tag' argument is NULL
  *
- * NOTMUCH_STATUS_TAG_TOO_LONG: The length of 'tag' is longer than
- *     too long (exceeds NOTMUCH_TAG_MAX)
+ * NOTMUCH_STATUS_TAG_TOO_LONG: The length of 'tag' is too long
+ *     (exceeds NOTMUCH_TAG_MAX)
  */
 notmuch_status_t
 notmuch_message_remove_tag (notmuch_message_t *message, const char *tag);
 
+/* Remove all tags from the given message.
+ *
+ * See notmuch_message_freeze for an example showing how to safely
+ * replace tag values.
+ */
+void
+notmuch_message_remove_all_tags (notmuch_message_t *message);
+
+/* Freeze the current state of 'message' within the database.
+ *
+ * This means that changes to the message state, (via
+ * notmuch_message_add_tag, notmuch_message_remove_tag, and
+ * notmuch_message_remove_all_tags), will not be committed to the
+ * database until the message is thawed with notmuch_message_thaw.
+ *
+ * Multiple calls to freeze/thaw are valid and these calls with
+ * "stack". That is there must be as many calls to thaw as to freeze
+ * before a message is actually thawed.
+ *
+ * The ability to do freeze/thaw allows for safe transactions to
+ * change tag values. For example, explicitly setting a message to
+ * have a given set of tags might look like this:
+ *
+ *    notmuch_message_freeze (message);
+ *
+ *    notmuch_message_remove_all_tags (message);
+ *
+ *    for (i = 0; i < NUM_TAGS; i++)
+ *        notmuch_message_add_tag (message, tags[i]);
+ *
+ *    notmuch_message_thaw (message);
+ *
+ * With freeze/thaw used like this, the message in the database is
+ * guaranteed to have either the full set of original tag value, or
+ * the full set of new tag values, but nothing in between.
+ *
+ * Imagine the example above without freeze/thaw and the operation
+ * somehow getting interrupted. This could result in the message being
+ * left with no tags if the interruption happened after
+ * notmuch_message_remove_all_tags but before notmuch_message_add_tag.
+ */
+void
+notmuch_message_freeze (notmuch_message_t *message);
+
+/* Thaw the current 'message', synchronizing any changes that may have
+ * occurred while 'message' was frozen into the notmuch database.
+ *
+ * See notmuch_message_freeze for an example of how to use this
+ * function to safely provide tag changes.
+ *
+ * Multiple calls to freeze/thaw are valid and these calls with
+ * "stack". That is there must be as many calls to thaw as to freeze
+ * before a message is actually thawed.
+ *
+ * Return value:
+ *
+ * NOTMUCH_STATUS_SUCCESS: Message successfully thawed, (or at least
+ *     its frozen count has successfully been reduced by 1).
+ *
+ * NOTMUCH_STATUS_UNBALANCE_FREEZE_THAW: An attempt was made to thaw
+ *     an unfrozen message. That is, there have been an unbalanced
+ *     number of calls to notmuch_message_freeze and
+ *     notmuch_message_thaw.
+ */
+notmuch_status_t
+notmuch_message_thaw (notmuch_message_t *message);
+
 /* Destroy a notmuch_message_t object.
  *
  * It can be useful to call this function in the case of a single