[PATCH 00/10] Fix 'notmuch new' atomicity issues

[PATCH 00/10] Fix 'notmuch new' atomicity issues

[Austin Clements]

This patch series modifies notmuch new to perform all operations
atomically and to perform maildir flag synchronization eagerly.  As a
result, notmuch new can be interrupted without risking database
consistency or losing track of messages, but still without losing
progress in the middle of a big import.  This also paves the way for
fixing the antisocial locking behavior of notmuch new.

While there are quite a few patches in the series, each one is
bite-sized and you can see the number of atomicity violations dropping
with nearly every patch using the test added by the first patch.

On my test machine, these patches have no affect on performance.

These patches are also available on the atomic-new-v1 branch at
 http://awakening.csail.mit.edu/git/notmuch.git/

[PATCH 01/10] test: Test atomicity of notmuch new.

[Austin Clements]

This tests notmuch new's ability to recover from arbitrary stopping
failures.  It interrupts notmuch new after every database commit and,
on every resulting database snapshot, re-runs notmuch new to
completion and checks that the final database state is invariant.
---
 test/atomicity     |   90 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 test/atomicity.gdb |   38 ++++++++++++++++++++++
 test/basic         |    4 ++-
 test/notmuch-test  |    1 +
 4 files changed, 132 insertions(+), 1 deletions(-)
 create mode 100755 test/atomicity
 create mode 100644 test/atomicity.gdb

diff --git a/test/atomicity b/test/atomicity
new file mode 100755
index 0000000..e242802
--- /dev/null
+++ b/test/atomicity
@@ -0,0 +1,90 @@
+#!/bin/bash
+test_description='atomicity'
+. ./test-lib.sh
+
+# Create a maildir structure to also stress flag synchronization
+mkdir $MAIL_DIR/cur
+mkdir $MAIL_DIR/new
+mkdir $MAIL_DIR/tmp
+mkdir $MAIL_DIR/.remove-dir
+
+# Prepare the initial database
+generate_message [subject]='Duplicate' [filename]='duplicate:2,' [dir]=cur
+generate_message [subject]='Remove' [filename]='remove:2,' [dir]=cur
+generate_message [subject]='"Remove duplicate"' [filename]='remove-duplicate:2,' [dir]=cur
+cp $MAIL_DIR/cur/remove-duplicate:2, $MAIL_DIR/cur/remove-duplicate-copy:2,
+generate_message [subject]='Rename' [filename]='rename:2,' [dir]=cur
+generate_message [subject]='"Rename duplicate"' [filename]='rename-duplicate:2,' [dir]=cur
+generate_message [subject]='"Move 1"' [filename]='move1:2,' [dir]=cur
+generate_message [subject]='"Move 2"' [filename]='move2:2,' [dir]=new
+generate_message [subject]='Flag' [filename]='flag:2,' [dir]=cur
+generate_message [subject]='"Flag duplicate"' [filename]='flag-duplicate:2,' [dir]=cur
+cp $MAIL_DIR/cur/flag-duplicate:2, $MAIL_DIR/cur/flag-duplicate-copy:2,F
+generate_message [subject]='"Remove directory"' [filename]='remove-directory:2,' [dir]=.remove-dir
+generate_message [subject]='"Remove directory duplicate"' [filename]='remove-directory-duplicate:2,' [dir]=.remove-dir
+cp $MAIL_DIR/.remove-dir/remove-directory-duplicate:2, $MAIL_DIR/cur/
+notmuch new > /dev/null
+
+# Make all database changes
+generate_message [subject]='Added' [filename]='added:2,' [dir]=cur
+cp $MAIL_DIR/cur/duplicate:2, $MAIL_DIR/cur/duplicate-copy:2,
+generate_message [subject]='"Add duplicate"' [filename]='add-duplicate:2,' [dir]=cur
+generate_message [subject]='"Add duplicate copy"' [filename]='add-duplicate-copy:2,' [dir]=cur
+rm $MAIL_DIR/cur/remove:2,
+rm $MAIL_DIR/cur/remove-duplicate-copy:2,
+mv $MAIL_DIR/cur/rename:2, $MAIL_DIR/cur/renamed:2,
+mv $MAIL_DIR/cur/rename-duplicate:2, $MAIL_DIR/cur/renamed-duplicate:2,
+mv $MAIL_DIR/cur/move1:2, $MAIL_DIR/new/move1:2,
+mv $MAIL_DIR/new/move2:2, $MAIL_DIR/cur/move2:2,
+mv $MAIL_DIR/cur/flag:2, $MAIL_DIR/cur/flag:2,F
+rm $MAIL_DIR/cur/flag-duplicate-copy:2,F
+rm $MAIL_DIR/.remove-dir/remove-directory:2,
+rm $MAIL_DIR/.remove-dir/remove-directory-duplicate:2,
+rmdir $MAIL_DIR/.remove-dir
+increment_mtime $MAIL_DIR/cur
+increment_mtime $MAIL_DIR/new
+increment_mtime $MAIL_DIR
+
+# Prepare the database snapshot
+cp -ra $MAIL_DIR $MAIL_DIR.snap
+cp ${NOTMUCH_CONFIG} ${NOTMUCH_CONFIG}.snap
+NOTMUCH_CONFIG=${NOTMUCH_CONFIG}.snap notmuch config set database.path $MAIL_DIR.snap
+
+
+test_begin_subtest '"notmuch new" is idempotent under arbitrary aborts'
+
+# Execute notmuch new and, at every call to rename, snapshot the
+# database, run notmuch new again on the snapshot, and capture the
+# results of search.
+export MAIL_DIR
+gdb -batch -x ../atomicity.gdb notmuch > /dev/null 2>&1
+
+# Get the final, golden output
+notmuch search '*' > expected
+
+# Check output against golden output
+outcount=$(cat outcount)
+echo -n > searchall
+echo -n > expectall
+for ((i = 0; i < $outcount; i++)); do
+    if ! cmp -s search.$i expected; then
+        # Find the range of interruptions that match this output
+        for ((end = $i + 1 ; end < $outcount; end++)); do
+            if ! cmp -s search.$i search.$end; then
+                break
+            fi
+        done
+        echo "When interrupted after $test/backtrace.$(expr $i - 1) (abort points $i-$(expr $end - 1))" >> searchall
+        cat search.$i >> searchall
+        cat expected >> expectall
+        echo >> searchall
+        echo >> expectall
+
+        i=$(expr $end - 1)
+    fi
+done
+test_expect_equal "$(cat searchall)" "$(cat expectall)"
+
+test_expect_success "detected $outcount>10 abort points" "test $outcount -gt 10"
+
+test_done
diff --git a/test/atomicity.gdb b/test/atomicity.gdb
new file mode 100644
index 0000000..a20d977
--- /dev/null
+++ b/test/atomicity.gdb
@@ -0,0 +1,38 @@
+set args new
+
+# Make Xapian commit after every operation instead of batching
+set environment XAPIAN_FLUSH_THRESHOLD = 1
+
+# gdb can't keep track of a simple integer.  This is me weeping.
+shell echo 0 > outcount
+
+shell touch inodes
+
+break rename
+commands
+# As an optimization, only consider snapshots after a Xapian commit.
+# Xapian overwrites record.base? as the last step in the commit.
+shell echo > gdbcmd
+shell stat -c %i $MAIL_DIR/.notmuch/xapian/record.base* > inodes.new
+shell if cmp inodes inodes.new; then echo cont > gdbcmd; fi
+shell mv inodes.new inodes
+source gdbcmd
+
+# Save a backtrace in case the test does fail
+set logging file backtrace
+set logging on
+backtrace
+set logging off
+shell mv backtrace backtrace.`cat outcount`
+
+# Snapshot the database
+shell rm -r $MAIL_DIR.snap/.notmuch
+shell cp -r $MAIL_DIR/.notmuch $MAIL_DIR.snap/.notmuch
+# Run notmuch new to completion on the snapshot
+shell NOTMUCH_CONFIG=${NOTMUCH_CONFIG}.snap XAPIAN_FLUSH_THRESHOLD=1000 notmuch new > /dev/null
+shell NOTMUCH_CONFIG=${NOTMUCH_CONFIG}.snap notmuch search '*' > search.`cat outcount` 2>&1
+shell echo $(expr $(cat outcount) + 1) > outcount
+cont
+end
+
+run
diff --git a/test/basic b/test/basic
index 3b43ad9..f9d5fc3 100755
--- a/test/basic
+++ b/test/basic
@@ -57,7 +57,9 @@ available=$(ls -1 ../ | \
     sed -r -e "/^(aggregate-results.sh|Makefile|Makefile.local|notmuch-test)/d" \
 	   -e "/^(README|test-lib.sh|test-results|tmp.*|valgrind|corpus*)/d" \
 	   -e "/^(emacs.expected-output|smtp-dummy|smtp-dummy.c|test-verbose)/d" \
-	   -e "/^(test.expected-output|.*~)/d" | sort)
+	   -e "/^(test.expected-output|.*~)/d" \
+	   -e "/^(atomicity.gdb)/d" \
+	   | sort)
 test_expect_equal "$tests_in_suite" "$available"
 
 EXPECTED=../test.expected-output
diff --git a/test/notmuch-test b/test/notmuch-test
index 9d77c0f..c506705 100755
--- a/test/notmuch-test
+++ b/test/notmuch-test
@@ -36,6 +36,7 @@ TESTS="
   encoding
   emacs
   maildir-sync
+  atomicity
 "
 
 # Clean up any results from a previous run
-- 
1.7.2.3

[PATCH 02/10] new: Don't loose messages on SIGINT.

[Austin Clements]

Previously, message removals were always performed, even after a
SIGINT.  As a result, when a message was moved from one folder to
another, a SIGINT between processing the directory the message was
removed from and processing the directory it was added to would result
in notmuch removing that message from the database.
---
 notmuch-new.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/notmuch-new.c b/notmuch-new.c
index 941f9d6..a910e5f 100644
--- a/notmuch-new.c
+++ b/notmuch-new.c
@@ -837,7 +837,7 @@ notmuch_new_command (void *ctx, int argc, char *argv[])
     removed_files = 0;
     renamed_files = 0;
     gettimeofday (&tv_start, NULL);
-    for (f = add_files_state.removed_files->head; f; f = f->next) {
+    for (f = add_files_state.removed_files->head; f && !interrupted; f = f->next) {
 	status = notmuch_database_remove_message (notmuch, f->filename);
 	if (status == NOTMUCH_STATUS_DUPLICATE_MESSAGE_ID)
 	    renamed_files++;
@@ -852,7 +852,7 @@ notmuch_new_command (void *ctx, int argc, char *argv[])
     }
 
     gettimeofday (&tv_start, NULL);
-    for (f = add_files_state.removed_directories->head, i = 0; f; f = f->next, i++) {
+    for (f = add_files_state.removed_directories->head, i = 0; f && !interrupted; f = f->next, i++) {
 	_remove_directory (ctx, notmuch, f->filename,
 			   &renamed_files, &removed_files);
 	if (do_print_progress) {
-- 
1.7.2.3

[PATCH 03/10] new: Defer updating directory mtimes until the end.

[Austin Clements]

Previously, if notmuch new were interrupted between updating the
directory mtime and handling removals from that directory, a
subsequent notmuch new would not handle those removals until something
else changed in that directory.  This defers recording the updated
mtime until after removals are handled to eliminate this problem.
---
 notmuch-new.c |   23 +++++++++++++++++------
 1 files changed, 17 insertions(+), 6 deletions(-)

diff --git a/notmuch-new.c b/notmuch-new.c
index a910e5f..65682d8 100644
--- a/notmuch-new.c
+++ b/notmuch-new.c
@@ -24,6 +24,7 @@
 
 typedef struct _filename_node {
     char *filename;
+    time_t mtime;
     struct _filename_node *next;
 } _filename_node_t;
 
@@ -46,6 +47,7 @@ typedef struct {
 
     _filename_list_t *removed_files;
     _filename_list_t *removed_directories;
+    _filename_list_t *directory_mtimes;
 
     notmuch_bool_t synchronize_flags;
     _filename_list_t *message_ids_to_sync;
@@ -87,7 +89,7 @@ _filename_list_create (const void *ctx)
     return list;
 }
 
-static void
+static _filename_node_t *
 _filename_list_add (_filename_list_t *list,
 		    const char *filename)
 {
@@ -100,6 +102,8 @@ _filename_list_add (_filename_list_t *list,
 
     *(list->tail) = node;
     list->tail = &node->next;
+
+    return node;
 }
 
 static void
@@ -505,11 +509,7 @@ add_files_recursive (notmuch_database_t *notmuch,
 	notmuch_filenames_move_to_next (db_subdirs);
     }
 
-    if (! interrupted) {
-	status = notmuch_directory_set_mtime (directory, fs_mtime);
-	if (status && ret == NOTMUCH_STATUS_SUCCESS)
-	    ret = status;
-    }
+    _filename_list_add (state->directory_mtimes, path)->mtime = fs_mtime;
 
   DONE:
     if (next)
@@ -825,6 +825,7 @@ notmuch_new_command (void *ctx, int argc, char *argv[])
 
     add_files_state.removed_files = _filename_list_create (ctx);
     add_files_state.removed_directories = _filename_list_create (ctx);
+    add_files_state.directory_mtimes = _filename_list_create (ctx);
 
     if (! debugger_is_active () && add_files_state.output_is_a_tty
 	&& ! add_files_state.verbose) {
@@ -863,8 +864,18 @@ notmuch_new_command (void *ctx, int argc, char *argv[])
 	}
     }
 
+    for (f = add_files_state.directory_mtimes->head; f && !interrupted; f = f->next) {
+	notmuch_directory_t *directory;
+	directory = notmuch_database_get_directory (notmuch, f->filename);
+	if (directory) {
+	    notmuch_directory_set_mtime (directory, f->mtime);
+	    notmuch_directory_destroy (directory);
+	}
+    }
+
     talloc_free (add_files_state.removed_files);
     talloc_free (add_files_state.removed_directories);
+    talloc_free (add_files_state.directory_mtimes);
 
     /* Now that removals are done (hence the database is aware of all
      * renames), we can synchronize maildir_flags to tags for all
-- 
1.7.2.3

[PATCH 04/10] lib: Make _notmuch_message_sync capable of deleting a message.

[Austin Clements]

---
 lib/message.cc |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/lib/message.cc b/lib/message.cc
index 0590f76..06747fe 100644
--- a/lib/message.cc
+++ b/lib/message.cc
@@ -29,6 +29,7 @@ struct _notmuch_message {
     notmuch_database_t *notmuch;
     Xapian::docid doc_id;
     int frozen;
+    notmuch_bool_t deleted;
     char *message_id;
     char *thread_id;
     char *in_reply_to;
@@ -96,6 +97,7 @@ _notmuch_message_create_for_document (const void *talloc_owner,
     message->doc_id = doc_id;
 
     message->frozen = 0;
+    message->deleted = FALSE;
     message->flags = 0;
 
     /* Each of these will be lazily created as needed. */
@@ -765,7 +767,10 @@ _notmuch_message_sync (notmuch_message_t *message)
 	return;
 
     db = static_cast <Xapian::WritableDatabase *> (message->notmuch->xapian_db);
-    db->replace_document (message->doc_id, message->doc);
+    if (message->deleted)
+	db->delete_document (message->doc_id);
+    else
+	db->replace_document (message->doc_id, message->doc);
 }
 
 /* Ensure that 'message' is not holding any file object open. Future
-- 
1.7.2.3

[PATCH 05/10] lib: Indicate if there are more filenames after removal.

[Austin Clements]

Make _notmuch_message_remove_filename return
NOTMUCH_STATUS_DUPLICATE_MESSAGE_ID if the message has more filenames
and fix callers to handle this.
---
 lib/message.cc |   10 +++++++++-
 1 files changed, 9 insertions(+), 1 deletions(-)

diff --git a/lib/message.cc b/lib/message.cc
index 06747fe..635f5cf 100644
--- a/lib/message.cc
+++ b/lib/message.cc
@@ -480,6 +480,9 @@ _notmuch_message_add_filename (notmuch_message_t *message,
  * This change will not be reflected in the database until the next
  * call to _notmuch_message_sync.
  *
+ * If this message still has other filenames, returns
+ * NOTMUCH_STATUS_DUPLICATE_MESSAGE_ID.
+ *
  * Note: This function does not remove a document from the database,
  * even if the specified filename is the only filename for this
  * message. For that functionality, see
@@ -547,6 +550,10 @@ _notmuch_message_remove_filename (notmuch_message_t *message,
 	if (strncmp ((*i).c_str (), direntry_prefix, direntry_prefix_len))
 	    break;
 
+	/* Indicate that there are filenames remaining. */
+	if (status == NOTMUCH_STATUS_SUCCESS)
+	    status = NOTMUCH_STATUS_DUPLICATE_MESSAGE_ID;
+
 	direntry = (*i).c_str ();
 	direntry += direntry_prefix_len;
 
@@ -1235,7 +1242,8 @@ notmuch_message_tags_to_maildir_flags (notmuch_message_t *message)
 	    new_status = _notmuch_message_remove_filename (message,
 							   filename);
 	    /* Hold on to only the first error. */
-	    if (! status && new_status) {
+	    if (! status && new_status
+		&& new_status != NOTMUCH_STATUS_DUPLICATE_MESSAGE_ID) {
 		status = new_status;
 		continue;
 	    }
-- 
1.7.2.3

[PATCH 06/10] lib: Add API's to find by filename and remove a filename from a message.

[Austin Clements]

The two new API functions, notmuch_database_find_message_by_filename
and notmuch_message_remove_filename give library users more control
over the filename removal process.  notmuch_database_remove_message
has been reimplemented in terms of these new functions.

notmuch_message_remove_filename acts much like
notmuch_message_remove_tag in that it does not synchronize with the
database if the message is frozen.  Thus, callers can freeze a message
to remove a filename and perform tag synchronization in one atomic
operation.

This new approach also naturally eliminates an atomicity violation in
the old code.  Previously, notmuch_database_remove_message would first
update the database document to remove the filename and only then
remove the document if it had no more filenames left.  An interruption
between these two steps resulted in a permanently un-removable zombie
message that would produce errors and odd results in searches.  Since
this new approach delegates document deletion to
_notmuch_message_sync, the document will be deleted without first
being updated, eliminating this window.
---
 lib/database.cc |   58 +++++++++++++++++++-----------------------------------
 lib/message.cc  |   21 +++++++++++++++++++
 lib/notmuch.h   |   43 ++++++++++++++++++++++++++++++++++++++-
 3 files changed, 83 insertions(+), 39 deletions(-)

diff --git a/lib/database.cc b/lib/database.cc
index d88b168..bee1e96 100644
--- a/lib/database.cc
+++ b/lib/database.cc
@@ -1689,72 +1689,56 @@ notmuch_status_t
 notmuch_database_remove_message (notmuch_database_t *notmuch,
 				 const char *filename)
 {
-    Xapian::WritableDatabase *db;
+    notmuch_message_t *message;
+    notmuch_status_t status = NOTMUCH_STATUS_SUCCESS;
+    message = notmuch_database_find_message_by_filename (notmuch, filename);
+    if (message) {
+	status = notmuch_message_remove_filename (message, filename);
+	notmuch_message_destroy (message);
+    }
+    return status;
+}
+
+notmuch_message_t *
+notmuch_database_find_message_by_filename (notmuch_database_t *notmuch,
+					   const char *filename)
+{
     void *local;
     const char *prefix = _find_prefix ("file-direntry");
     char *direntry, *term;
     Xapian::PostingIterator i, end;
-    Xapian::Document document;
+    notmuch_message_t *message = NULL;
     notmuch_status_t status;
 
-    status = _notmuch_database_ensure_writable (notmuch);
-    if (status)
-	return status;
-
     local = talloc_new (notmuch);
 
-    db = static_cast <Xapian::WritableDatabase *> (notmuch->xapian_db);
-
     try {
 
 	status = _notmuch_database_filename_to_direntry (local, notmuch,
 							 filename, &direntry);
 	if (status)
-	    return status;
+	    return NULL;
 
 	term = talloc_asprintf (local, "%s%s", prefix, direntry);
 
 	find_doc_ids_for_term (notmuch, term, &i, &end);
 
-	for ( ; i != end; i++) {
-	    Xapian::TermIterator j;
-	    notmuch_message_t *message;
+	if (i != end) {
 	    notmuch_private_status_t private_status;
 
-	    message = _notmuch_message_create (local, notmuch,
+	    message = _notmuch_message_create (notmuch, notmuch,
 					       *i, &private_status);
-	    if (message == NULL)
-		return COERCE_STATUS (private_status,
-				      "Inconsistent document ID in datbase.");
-
-	    _notmuch_message_remove_filename (message, filename);
-	    _notmuch_message_sync (message);
-
-	    /* Take care to find document after sync'ing filename removal. */
-	    document = find_document_for_doc_id (notmuch, *i);
-	    j = document.termlist_begin ();
-	    j.skip_to (prefix);
-
-	    /* Was this the last file-direntry in the message? */
-	    if (j == document.termlist_end () ||
-		strncmp ((*j).c_str (), prefix, strlen (prefix)))
-	    {
-		db->delete_document (document.get_docid ());
-		status = NOTMUCH_STATUS_SUCCESS;
-	    } else {
-		status = NOTMUCH_STATUS_DUPLICATE_MESSAGE_ID;
-	    }
 	}
     } catch (const Xapian::Error &error) {
-	fprintf (stderr, "Error: A Xapian exception occurred removing message: %s\n",
+	fprintf (stderr, "Error: A Xapian exception occurred finding message by filename: %s\n",
 		 error.get_msg().c_str());
 	notmuch->exception_reported = TRUE;
-	status = NOTMUCH_STATUS_XAPIAN_EXCEPTION;
+	message = NULL;
     }
 
     talloc_free (local);
 
-    return status;
+    return message;
 }
 
 notmuch_tags_t *
diff --git a/lib/message.cc b/lib/message.cc
index 635f5cf..b4adb5c 100644
--- a/lib/message.cc
+++ b/lib/message.cc
@@ -1300,6 +1300,27 @@ notmuch_message_remove_all_tags (notmuch_message_t *message)
 }
 
 notmuch_status_t
+notmuch_message_remove_filename (notmuch_message_t *message,
+				 const char *filename)
+{
+    notmuch_status_t status;
+
+    status = _notmuch_database_ensure_writable (message->notmuch);
+    if (status)
+	return status;
+
+    status = _notmuch_message_remove_filename (message, filename);
+    /* Was this the last file-direntry in the message? */
+    if (status == NOTMUCH_STATUS_SUCCESS)
+	message->deleted = TRUE;
+
+    if (! message->frozen)
+	_notmuch_message_sync (message);
+
+    return status;
+}
+
+notmuch_status_t
 notmuch_message_freeze (notmuch_message_t *message)
 {
     notmuch_status_t status;
diff --git a/lib/notmuch.h b/lib/notmuch.h
index e508309..61030cb 100644
--- a/lib/notmuch.h
+++ b/lib/notmuch.h
@@ -316,6 +316,22 @@ notmuch_message_t *
 notmuch_database_find_message (notmuch_database_t *database,
 			       const char *message_id);
 
+/* 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.
+ *
+ * This function returns NULL in the following situations:
+ *
+ *	* No message is found with the given filename
+ *	* An out-of-memory situation occurs
+ *	* A Xapian exception occurs
+ */
+notmuch_message_t *
+notmuch_database_find_message_by_filename (notmuch_database_t *notmuch,
+					   const char *filename);
+
 /* Return a list of all tags found in the database.
  *
  * This function creates a list of all tags found in the database. The
@@ -979,11 +995,34 @@ notmuch_message_maildir_flags_to_tags (notmuch_message_t *message);
 notmuch_status_t
 notmuch_message_tags_to_maildir_flags (notmuch_message_t *message);
 
+/* Remove a filename from a message.  If this is the last copy of this
+ * message, also delete it from the database.
+ *
+ * Much like notmuch_message_remove_tag, if message is frozen, it will
+ * not be removed from or updated in the database until thawed.
+ *
+ * Return value:
+ *
+ * NOTMUCH_STATUS_SUCCESS: The last filename was removed and the
+ *	message was removed from the database.
+ *
+ * NOTMUCH_STATUS_DUPLICATE_MESSAGE_ID: This filename was removed but
+ *	the message persists in the database with at least one other
+ *	filename.
+ *
+ * NOTMUCH_STATUS_READ_ONLY_DATABASE: Database was opened in read-only
+ *	mode so no message can be removed.
+ */
+notmuch_status_t
+notmuch_message_remove_filename (notmuch_message_t *message,
+				 const char *filename);
+
 /* 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
+ * notmuch_message_add_tag, notmuch_message_remove_tag,
+ * notmuch_message_remove_all_tags, and
+ * notmuch_message_remove_filename), 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 will
-- 
1.7.2.3

[PATCH 07/10] new: Use the new filename removal API.

[Austin Clements]

This paves the way for eager tag synchronization and correct removal
atomicity.
---
 notmuch-new.c |   35 +++++++++++++++++++++++------------
 1 files changed, 23 insertions(+), 12 deletions(-)

diff --git a/notmuch-new.c b/notmuch-new.c
index 65682d8..56fe7b0 100644
--- a/notmuch-new.c
+++ b/notmuch-new.c
@@ -691,6 +691,27 @@ upgrade_print_progress (void *closure,
     fflush (stdout);
 }
 
+/* Remove one message filename from the database. */
+static notmuch_status_t
+remove_file (notmuch_database_t *notmuch,
+	     const char *path,
+	     int *renamed_files,
+	     int *removed_files)
+{
+    notmuch_status_t status;
+    notmuch_message_t *message;
+    message = notmuch_database_find_message_by_filename (notmuch, path);
+    if (!message)
+	return NOTMUCH_STATUS_SUCCESS;
+    status = notmuch_message_remove_filename (message, path);
+    if (status == NOTMUCH_STATUS_DUPLICATE_MESSAGE_ID)
+	(*renamed_files)++;
+    else
+	(*removed_files)++;
+    notmuch_message_destroy (message);
+    return status;
+}
+
 /* Recursively remove all filenames from the database referring to
  * 'path' (or to any of its children). */
 static void
@@ -702,7 +723,6 @@ _remove_directory (void *ctx,
 {
     notmuch_directory_t *directory;
     notmuch_filenames_t *files, *subdirs;
-    notmuch_status_t status;
     char *absolute;
 
     directory = notmuch_database_get_directory (notmuch, path);
@@ -713,11 +733,7 @@ _remove_directory (void *ctx,
     {
 	absolute = talloc_asprintf (ctx, "%s/%s", path,
 				    notmuch_filenames_get (files));
-	status = notmuch_database_remove_message (notmuch, absolute);
-	if (status == NOTMUCH_STATUS_DUPLICATE_MESSAGE_ID)
-	    *renamed_files = *renamed_files + 1;
-	else
-	    *removed_files = *removed_files + 1;
+	remove_file (notmuch, absolute, renamed_files, removed_files);
 	talloc_free (absolute);
     }
 
@@ -749,7 +765,6 @@ notmuch_new_command (void *ctx, int argc, char *argv[])
     struct sigaction action;
     _filename_node_t *f;
     int renamed_files, removed_files;
-    notmuch_status_t status;
     int i;
     notmuch_bool_t timer_is_active = FALSE;
 
@@ -839,11 +854,7 @@ notmuch_new_command (void *ctx, int argc, char *argv[])
     renamed_files = 0;
     gettimeofday (&tv_start, NULL);
     for (f = add_files_state.removed_files->head; f && !interrupted; f = f->next) {
-	status = notmuch_database_remove_message (notmuch, f->filename);
-	if (status == NOTMUCH_STATUS_DUPLICATE_MESSAGE_ID)
-	    renamed_files++;
-	else
-	    removed_files++;
+	remove_file (notmuch, f->filename, &renamed_files, &removed_files);
 	if (do_print_progress) {
 	    do_print_progress = 0;
 	    generic_print_progress ("Cleaned up", "messages",
-- 
1.7.2.3

[PATCH 08/10] new: Synchronize maildir flags eagerly.

[Austin Clements]

Because flag synchronization is stateless, it can be performed at any
time as long as it's guaranteed to be performed after any change to a
message's filename list.  Take advantage of this to synchronize tags
immediately after a filename is added or removed while the message is
still frozen.  With this change, filename removal is atomic and the
unsynchronized window for filename add is very short.
---
 notmuch-new.c |   57 +++++++++++++++++----------------------------------------
 1 files changed, 17 insertions(+), 40 deletions(-)

diff --git a/notmuch-new.c b/notmuch-new.c
index 56fe7b0..767722a 100644
--- a/notmuch-new.c
+++ b/notmuch-new.c
@@ -50,7 +50,6 @@ typedef struct {
     _filename_list_t *directory_mtimes;
 
     notmuch_bool_t synchronize_flags;
-    _filename_list_t *message_ids_to_sync;
 } add_files_state_t;
 
 static volatile sig_atomic_t do_print_progress = 0;
@@ -439,11 +438,8 @@ add_files_recursive (notmuch_database_t *notmuch,
 	    break;
 	/* Non-fatal issues (go on to next file) */
 	case NOTMUCH_STATUS_DUPLICATE_MESSAGE_ID:
-	    /* Defer sync of maildir flags until after old filenames
-	     * are removed in the case of a rename. */
 	    if (state->synchronize_flags == TRUE)
-		_filename_list_add (state->message_ids_to_sync,
-				    notmuch_message_get_message_id (message));
+		notmuch_message_maildir_flags_to_tags (message);
 	    break;
 	case NOTMUCH_STATUS_FILE_NOT_EMAIL:
 	    fprintf (stderr, "Note: Ignoring non-mail file: %s\n",
@@ -696,18 +692,23 @@ static notmuch_status_t
 remove_file (notmuch_database_t *notmuch,
 	     const char *path,
 	     int *renamed_files,
-	     int *removed_files)
+	     int *removed_files,
+	     add_files_state_t *state)
 {
     notmuch_status_t status;
     notmuch_message_t *message;
     message = notmuch_database_find_message_by_filename (notmuch, path);
     if (!message)
 	return NOTMUCH_STATUS_SUCCESS;
+    notmuch_message_freeze (message);
     status = notmuch_message_remove_filename (message, path);
-    if (status == NOTMUCH_STATUS_DUPLICATE_MESSAGE_ID)
+    if (status == NOTMUCH_STATUS_DUPLICATE_MESSAGE_ID) {
 	(*renamed_files)++;
-    else
+	if (state->synchronize_flags == TRUE)
+	    notmuch_message_maildir_flags_to_tags (message);
+    } else
 	(*removed_files)++;
+    notmuch_message_thaw (message);
     notmuch_message_destroy (message);
     return status;
 }
@@ -719,7 +720,8 @@ _remove_directory (void *ctx,
 		   notmuch_database_t *notmuch,
 		   const char *path,
 		   int *renamed_files,
-		   int *removed_files)
+		   int *removed_files,
+		   add_files_state_t *state)
 {
     notmuch_directory_t *directory;
     notmuch_filenames_t *files, *subdirs;
@@ -733,7 +735,7 @@ _remove_directory (void *ctx,
     {
 	absolute = talloc_asprintf (ctx, "%s/%s", path,
 				    notmuch_filenames_get (files));
-	remove_file (notmuch, absolute, renamed_files, removed_files);
+	remove_file (notmuch, absolute, renamed_files, removed_files, state);
 	talloc_free (absolute);
     }
 
@@ -743,7 +745,8 @@ _remove_directory (void *ctx,
     {
 	absolute = talloc_asprintf (ctx, "%s/%s", path,
 				    notmuch_filenames_get (subdirs));
-	_remove_directory (ctx, notmuch, absolute, renamed_files, removed_files);
+	_remove_directory (ctx, notmuch, absolute, renamed_files, removed_files,
+			   state);
 	talloc_free (absolute);
     }
 
@@ -785,7 +788,6 @@ notmuch_new_command (void *ctx, int argc, char *argv[])
 
     add_files_state.new_tags = notmuch_config_get_new_tags (config, &add_files_state.new_tags_length);
     add_files_state.synchronize_flags = notmuch_config_get_maildir_synchronize_flags (config);
-    add_files_state.message_ids_to_sync = _filename_list_create (ctx);
     db_path = notmuch_config_get_database_path (config);
 
     dot_notmuch_path = talloc_asprintf (ctx, "%s/%s", db_path, ".notmuch");
@@ -854,7 +856,8 @@ notmuch_new_command (void *ctx, int argc, char *argv[])
     renamed_files = 0;
     gettimeofday (&tv_start, NULL);
     for (f = add_files_state.removed_files->head; f && !interrupted; f = f->next) {
-	remove_file (notmuch, f->filename, &renamed_files, &removed_files);
+	remove_file (notmuch, f->filename, &renamed_files, &removed_files,
+		     &add_files_state);
 	if (do_print_progress) {
 	    do_print_progress = 0;
 	    generic_print_progress ("Cleaned up", "messages",
@@ -866,7 +869,7 @@ notmuch_new_command (void *ctx, int argc, char *argv[])
     gettimeofday (&tv_start, NULL);
     for (f = add_files_state.removed_directories->head, i = 0; f && !interrupted; f = f->next, i++) {
 	_remove_directory (ctx, notmuch, f->filename,
-			   &renamed_files, &removed_files);
+			   &renamed_files, &removed_files, &add_files_state);
 	if (do_print_progress) {
 	    do_print_progress = 0;
 	    generic_print_progress ("Cleaned up", "directories",
@@ -888,32 +891,6 @@ notmuch_new_command (void *ctx, int argc, char *argv[])
     talloc_free (add_files_state.removed_directories);
     talloc_free (add_files_state.directory_mtimes);
 
-    /* Now that removals are done (hence the database is aware of all
-     * renames), we can synchronize maildir_flags to tags for all
-     * messages that had new filenames appear on this run. */
-    gettimeofday (&tv_start, NULL);
-    if (add_files_state.synchronize_flags) {
-	_filename_node_t *node;
-	notmuch_message_t *message;
-	for (node = add_files_state.message_ids_to_sync->head, i = 0;
-	     node;
-	     node = node->next, i++)
-	{
-	    message = notmuch_database_find_message (notmuch, node->filename);
-	    notmuch_message_maildir_flags_to_tags (message);
-	    notmuch_message_destroy (message);
-	    if (do_print_progress) {
-		do_print_progress = 0;
-		generic_print_progress (
-		    "Synchronized tags for", "messages",
-		    tv_start, i, add_files_state.message_ids_to_sync->count);
-	    }
-	}
-    }
-
-    talloc_free (add_files_state.message_ids_to_sync);
-    add_files_state.message_ids_to_sync = NULL;
-
     if (timer_is_active)
 	stop_progress_printing_timer ();
 
-- 
1.7.2.3

[PATCH 09/10] lib: Add notmuch_database_{begin,end}_atomic.

[Austin Clements]

These operations translate into non-flushed Xapian transactions,
allowing arbitrary groups of database operations to be performed
atomically.
---
 lib/database.cc |   44 ++++++++++++++++++++++++++++++++++++++++++++
 lib/notmuch.h   |   30 ++++++++++++++++++++++++++++++
 2 files changed, 74 insertions(+), 0 deletions(-)

diff --git a/lib/database.cc b/lib/database.cc
index bee1e96..0103a56 100644
--- a/lib/database.cc
+++ b/lib/database.cc
@@ -974,6 +974,50 @@ notmuch_database_upgrade (notmuch_database_t *notmuch,
     return NOTMUCH_STATUS_SUCCESS;
 }
 
+notmuch_status_t
+notmuch_database_begin_atomic (notmuch_database_t *notmuch)
+{
+    if (notmuch->mode == NOTMUCH_DATABASE_MODE_READ_ONLY)
+	return NOTMUCH_STATUS_SUCCESS;
+
+    try {
+	(static_cast <Xapian::WritableDatabase *> (notmuch->xapian_db))->begin_transaction (false);
+    } catch (const Xapian::Error &error) {
+	fprintf (stderr, "A Xapian exception occurred beginning transaction: %s.\n",
+		 error.get_msg().c_str());
+	notmuch->exception_reported = TRUE;
+	return NOTMUCH_STATUS_XAPIAN_EXCEPTION;
+    }
+    return NOTMUCH_STATUS_SUCCESS;
+}
+
+notmuch_status_t
+notmuch_database_end_atomic (notmuch_database_t *notmuch)
+{
+    Xapian::WritableDatabase *db;
+
+    if (notmuch->mode == NOTMUCH_DATABASE_MODE_READ_ONLY)
+	return NOTMUCH_STATUS_SUCCESS;
+
+    db = static_cast <Xapian::WritableDatabase *> (notmuch->xapian_db);
+    try {
+	db->commit_transaction ();
+
+	/* This is a hack for testing.  Xapian never flushes on a
+	 * non-flushed commit, even if the flush threshold is 1.
+	 * However, we rely on flushing to test atomicity. */
+	const char *thresh = getenv ("XAPIAN_FLUSH_THRESHOLD");
+	if (thresh && atoi (thresh) == 1)
+	    db->commit ();
+    } catch (const Xapian::Error &error) {
+	fprintf (stderr, "A Xapian exception occurred committing transaction: %s.\n",
+		 error.get_msg().c_str());
+	notmuch->exception_reported = TRUE;
+	return NOTMUCH_STATUS_XAPIAN_EXCEPTION;
+    }
+    return NOTMUCH_STATUS_SUCCESS;
+}
+
 /* We allow the user to use arbitrarily long paths for directories. But
  * we have a term-length limit. So if we exceed that, we'll use the
  * SHA-1 of the path for the database term.
diff --git a/lib/notmuch.h b/lib/notmuch.h
index 61030cb..9cdcec0 100644
--- a/lib/notmuch.h
+++ b/lib/notmuch.h
@@ -214,6 +214,36 @@ notmuch_database_upgrade (notmuch_database_t *database,
 						   double progress),
 			  void *closure);
 
+/* Begin an atomic database operation.
+ *
+ * Any modifications performed between a successful begin and a
+ * notmuch_database_end_atomic will be applied to the database
+ * atomically.  Note that, unlike a typical database transaction, this
+ * only ensures atomicity, not durability; neither begin nor end
+ * necessarily flush modifications to disk.
+ *
+ * Return value:
+ *
+ * NOTMUCH_STATUS_SUCCESS: Successfully entered atomic section.
+ *
+ * NOTMUCH_STATUS_XAPIAN_EXCEPTION: A Xapian exception occurred;
+ *	atomic section not entered.
+ */
+notmuch_status_t
+notmuch_database_begin_atomic (notmuch_database_t *notmuch);
+
+/* Indicate the end of an atomic database operation.
+ *
+ * Return value:
+ *
+ * NOTMUCH_STATUS_SUCCESS: Successfully completed atomic section.
+ *
+ * NOTMUCH_STATUS_XAPIAN_EXCEPTION: A Xapian exception occurred;
+ *	atomic section not ended.
+ */
+notmuch_status_t
+notmuch_database_end_atomic (notmuch_database_t *notmuch);
+
 /* Retrieve a directory object from the database for 'path'.
  *
  * Here, 'path' should be a path relative to the path of 'database'
-- 
1.7.2.3

[PATCH 10/10] new: Wrap adding a message in an atomic section.

[Austin Clements]

This prevents atomicity violations when adding new messages.  Each
message add is wrapped in its own atomic section, so interrupting
notmuch new doesn't loose progress.  Unlike in the remove case, adding
a message can modify more than one database document, necessitating
full transactions.
---
 notmuch-new.c |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/notmuch-new.c b/notmuch-new.c
index 767722a..880f31a 100644
--- a/notmuch-new.c
+++ b/notmuch-new.c
@@ -424,6 +424,12 @@ add_files_recursive (notmuch_database_t *notmuch,
 	    fflush (stdout);
 	}
 
+	status = notmuch_database_begin_atomic (notmuch);
+	if (status) {
+	    ret = status;
+	    goto DONE;
+	}
+
 	status = notmuch_database_add_message (notmuch, next, &message);
 	switch (status) {
 	/* success */
@@ -463,6 +469,12 @@ add_files_recursive (notmuch_database_t *notmuch,
 	    goto DONE;
 	}
 
+	status = notmuch_database_end_atomic (notmuch);
+	if (status) {
+	    ret = status;
+	    goto DONE;
+	}
+
 	if (message) {
 	    notmuch_message_destroy (message);
 	    message = NULL;
-- 
1.7.2.3

Re: [PATCH 00/10] Fix 'notmuch new' atomicity issues

[Austin Clements]

Bump.  Now rebased against current head (with no conflicts) on
atomic-new-v2 (and for-review/atomic-new-v2) at
  http://awakening.csail.mit.edu/git/notmuch.git/

On Fri, Feb 18, 2011 at 2:58 AM, Austin Clements <amdragon@mit.edu> wrote:
> This patch series modifies notmuch new to perform all operations
> atomically and to perform maildir flag synchronization eagerly.  As a
> result, notmuch new can be interrupted without risking database
> consistency or losing track of messages, but still without losing
> progress in the middle of a big import.  This also paves the way for
> fixing the antisocial locking behavior of notmuch new.
>
> While there are quite a few patches in the series, each one is
> bite-sized and you can see the number of atomicity violations dropping
> with nearly every patch using the test added by the first patch.
>
> On my test machine, these patches have no affect on performance.
>
> These patches are also available on the atomic-new-v1 branch at
>  http://awakening.csail.mit.edu/git/notmuch.git/
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch
>

Re: [PATCH 00/10] Fix 'notmuch new' atomicity issues

[Austin Clements]

jrollins found a timing bug in the atomicity test.  A fix, plus beefed
up test comments are on a new atomic-new-v3 (and
for-review/atomic-new-v3) branch at
  http://awakening.csail.mit.edu/git/notmuch.git/
Since this is more than a rebase, I'll email an update to the one
changed patch in the series.

On Tue, Apr 26, 2011 at 12:13 AM, Austin Clements <amdragon@mit.edu> wrote:
> Bump.  Now rebased against current head (with no conflicts) on
> atomic-new-v2 (and for-review/atomic-new-v2) at
>  http://awakening.csail.mit.edu/git/notmuch.git/
>
> On Fri, Feb 18, 2011 at 2:58 AM, Austin Clements <amdragon@mit.edu> wrote:
>> This patch series modifies notmuch new to perform all operations
>> atomically and to perform maildir flag synchronization eagerly.  As a
>> result, notmuch new can be interrupted without risking database
>> consistency or losing track of messages, but still without losing
>> progress in the middle of a big import.  This also paves the way for
>> fixing the antisocial locking behavior of notmuch new.
>>
>> While there are quite a few patches in the series, each one is
>> bite-sized and you can see the number of atomicity violations dropping
>> with nearly every patch using the test added by the first patch.
>>
>> On my test machine, these patches have no affect on performance.
>>
>> These patches are also available on the atomic-new-v1 branch at
>>  http://awakening.csail.mit.edu/git/notmuch.git/
>>
>> _______________________________________________
>> notmuch mailing list
>> notmuch@notmuchmail.org
>> http://notmuchmail.org/mailman/listinfo/notmuch
>>
>

[PATCH 01/10 v2] test: Test atomicity of notmuch new.

[Austin Clements]

This tests notmuch new's ability to recover from arbitrary stopping
failures.  It interrupts notmuch new after every database commit and,
on every resulting database snapshot, re-runs notmuch new to
completion and checks that the final database state is invariant.
---
This addresses a timing bug in the atomicity test found by jrollins
and also adds additional comments.

 test/atomicity     |   96 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 test/atomicity.gdb |   50 +++++++++++++++++++++++++++
 test/basic         |    4 ++-
 test/notmuch-test  |    1 +
 4 files changed, 150 insertions(+), 1 deletions(-)
 create mode 100755 test/atomicity
 create mode 100644 test/atomicity.gdb

diff --git a/test/atomicity b/test/atomicity
new file mode 100755
index 0000000..b7ae505
--- /dev/null
+++ b/test/atomicity
@@ -0,0 +1,96 @@
+#!/bin/bash
+test_description='atomicity'
+. ./test-lib.sh
+
+# This script tests the effects of killing and restarting "notmuch
+# new" at arbitrary points.  If notmuch new is properly atomic, the
+# final database contents should be the same regardless of when (or
+# if) it is killed and restarted.
+
+# Create a maildir structure to also stress flag synchronization
+mkdir $MAIL_DIR/cur
+mkdir $MAIL_DIR/new
+mkdir $MAIL_DIR/tmp
+mkdir $MAIL_DIR/.remove-dir
+
+# Prepare the initial database
+generate_message [subject]='Duplicate' [filename]='duplicate:2,' [dir]=cur
+generate_message [subject]='Remove' [filename]='remove:2,' [dir]=cur
+generate_message [subject]='"Remove duplicate"' [filename]='remove-duplicate:2,' [dir]=cur
+cp $MAIL_DIR/cur/remove-duplicate:2, $MAIL_DIR/cur/remove-duplicate-copy:2,
+generate_message [subject]='Rename' [filename]='rename:2,' [dir]=cur
+generate_message [subject]='"Rename duplicate"' [filename]='rename-duplicate:2,' [dir]=cur
+generate_message [subject]='"Move 1"' [filename]='move1:2,' [dir]=cur
+generate_message [subject]='"Move 2"' [filename]='move2:2,' [dir]=new
+generate_message [subject]='Flag' [filename]='flag:2,' [dir]=cur
+generate_message [subject]='"Flag duplicate"' [filename]='flag-duplicate:2,' [dir]=cur
+cp $MAIL_DIR/cur/flag-duplicate:2, $MAIL_DIR/cur/flag-duplicate-copy:2,F
+generate_message [subject]='"Remove directory"' [filename]='remove-directory:2,' [dir]=.remove-dir
+generate_message [subject]='"Remove directory duplicate"' [filename]='remove-directory-duplicate:2,' [dir]=.remove-dir
+cp $MAIL_DIR/.remove-dir/remove-directory-duplicate:2, $MAIL_DIR/cur/
+notmuch new > /dev/null
+
+# Make all maildir changes, but *don't* update the database
+generate_message [subject]='Added' [filename]='added:2,' [dir]=cur
+cp $MAIL_DIR/cur/duplicate:2, $MAIL_DIR/cur/duplicate-copy:2,
+generate_message [subject]='"Add duplicate"' [filename]='add-duplicate:2,' [dir]=cur
+generate_message [subject]='"Add duplicate copy"' [filename]='add-duplicate-copy:2,' [dir]=cur
+rm $MAIL_DIR/cur/remove:2,
+rm $MAIL_DIR/cur/remove-duplicate-copy:2,
+mv $MAIL_DIR/cur/rename:2, $MAIL_DIR/cur/renamed:2,
+mv $MAIL_DIR/cur/rename-duplicate:2, $MAIL_DIR/cur/renamed-duplicate:2,
+mv $MAIL_DIR/cur/move1:2, $MAIL_DIR/new/move1:2,
+mv $MAIL_DIR/new/move2:2, $MAIL_DIR/cur/move2:2,
+mv $MAIL_DIR/cur/flag:2, $MAIL_DIR/cur/flag:2,F
+rm $MAIL_DIR/cur/flag-duplicate-copy:2,F
+rm $MAIL_DIR/.remove-dir/remove-directory:2,
+rm $MAIL_DIR/.remove-dir/remove-directory-duplicate:2,
+rmdir $MAIL_DIR/.remove-dir
+increment_mtime $MAIL_DIR/cur
+increment_mtime $MAIL_DIR/new
+increment_mtime $MAIL_DIR
+
+# Prepare a snapshot of the updated maildir.  The gdb script will
+# update the database in this snapshot as it goes.
+cp -ra $MAIL_DIR $MAIL_DIR.snap
+cp ${NOTMUCH_CONFIG} ${NOTMUCH_CONFIG}.snap
+NOTMUCH_CONFIG=${NOTMUCH_CONFIG}.snap notmuch config set database.path $MAIL_DIR.snap
+
+
+test_begin_subtest '"notmuch new" is idempotent under arbitrary aborts'
+
+# Execute notmuch new and, at every call to rename, snapshot the
+# database, run notmuch new again on the snapshot, and capture the
+# results of search.
+export MAIL_DIR
+gdb -batch -x ../atomicity.gdb notmuch > /dev/null 2>&1
+
+# Get the final, golden output
+notmuch search '*' > expected
+
+# Check output against golden output
+outcount=$(cat outcount)
+echo -n > searchall
+echo -n > expectall
+for ((i = 0; i < $outcount; i++)); do
+    if ! cmp -s search.$i expected; then
+        # Find the range of interruptions that match this output
+        for ((end = $i + 1 ; end < $outcount; end++)); do
+            if ! cmp -s search.$i search.$end; then
+                break
+            fi
+        done
+        echo "When interrupted after $test/backtrace.$(expr $i - 1) (abort points $i-$(expr $end - 1))" >> searchall
+        cat search.$i >> searchall
+        cat expected >> expectall
+        echo >> searchall
+        echo >> expectall
+
+        i=$(expr $end - 1)
+    fi
+done
+test_expect_equal_failure "$(cat searchall)" "$(cat expectall)"
+
+test_expect_success "detected $outcount>10 abort points" "test $outcount -gt 10"
+
+test_done
diff --git a/test/atomicity.gdb b/test/atomicity.gdb
new file mode 100644
index 0000000..fd67525
--- /dev/null
+++ b/test/atomicity.gdb
@@ -0,0 +1,50 @@
+# This gdb script runs notmuch new and simulates killing and
+# restarting notmuch new after every Xapian commit.  To simulate this
+# more efficiently, this script runs notmuch new and, immediately
+# after every Xapian commit, it *pauses* the running notmuch new,
+# copies the entire database and maildir to a snapshot directory, and
+# executes a full notmuch new on that snapshot, comparing the final
+# results with the expected output.  It can then resume the paused
+# notmuch new, which is still running on the original maildir, and
+# repeat this process.
+
+set args new
+
+# Make Xapian commit after every operation instead of batching
+set environment XAPIAN_FLUSH_THRESHOLD = 1
+
+# gdb can't keep track of a simple integer.  This is me weeping.
+shell echo 0 > outcount
+
+shell touch inodes
+
+break rename
+commands
+# As an optimization, only consider snapshots after a Xapian commit.
+# Xapian overwrites record.base? as the last step in the commit.
+shell echo > gdbcmd
+shell stat -c %i $MAIL_DIR/.notmuch/xapian/record.base* > inodes.new
+shell if cmp inodes inodes.new; then echo cont > gdbcmd; fi
+shell mv inodes.new inodes
+source gdbcmd
+
+# Save a backtrace in case the test does fail
+set logging file backtrace
+set logging on
+backtrace
+set logging off
+shell mv backtrace backtrace.`cat outcount`
+
+# Snapshot the database
+shell rm -r $MAIL_DIR.snap/.notmuch
+shell cp -r $MAIL_DIR/.notmuch $MAIL_DIR.snap/.notmuch
+# Restore the mtime of $MAIL_DIR.snap, which we just changed
+shell touch -r $MAIL_DIR $MAIL_DIR.snap
+# Run notmuch new to completion on the snapshot
+shell NOTMUCH_CONFIG=${NOTMUCH_CONFIG}.snap XAPIAN_FLUSH_THRESHOLD=1000 notmuch new > /dev/null
+shell NOTMUCH_CONFIG=${NOTMUCH_CONFIG}.snap notmuch search '*' > search.`cat outcount` 2>&1
+shell echo $(expr $(cat outcount) + 1) > outcount
+cont
+end
+
+run
diff --git a/test/basic b/test/basic
index 3b43ad9..f9d5fc3 100755
--- a/test/basic
+++ b/test/basic
@@ -57,7 +57,9 @@ available=$(ls -1 ../ | \
     sed -r -e "/^(aggregate-results.sh|Makefile|Makefile.local|notmuch-test)/d" \
 	   -e "/^(README|test-lib.sh|test-results|tmp.*|valgrind|corpus*)/d" \
 	   -e "/^(emacs.expected-output|smtp-dummy|smtp-dummy.c|test-verbose)/d" \
-	   -e "/^(test.expected-output|.*~)/d" | sort)
+	   -e "/^(test.expected-output|.*~)/d" \
+	   -e "/^(atomicity.gdb)/d" \
+	   | sort)
 test_expect_equal "$tests_in_suite" "$available"
 
 EXPECTED=../test.expected-output
diff --git a/test/notmuch-test b/test/notmuch-test
index 8dd9c1e..5b22fa8 100755
--- a/test/notmuch-test
+++ b/test/notmuch-test
@@ -38,6 +38,7 @@ TESTS="
   emacs
   emacs-large-search-buffer
   maildir-sync
+  atomicity
 "
 
 # Clean up any results from a previous run
-- 
1.7.4.1

Re: [PATCH 00/10] Fix 'notmuch new' atomicity issues

[Austin Clements]

Rebased to current master (cb8418) as atomic-new-v4 (aka
for-review/atomic-new-v4).

On Wed, May 4, 2011 at 4:30 PM, Austin Clements <amdragon@mit.edu> wrote:
> jrollins found a timing bug in the atomicity test.  A fix, plus beefed
> up test comments are on a new atomic-new-v3 (and
> for-review/atomic-new-v3) branch at
>  http://awakening.csail.mit.edu/git/notmuch.git/
> Since this is more than a rebase, I'll email an update to the one
> changed patch in the series.
>
> On Tue, Apr 26, 2011 at 12:13 AM, Austin Clements <amdragon@mit.edu> wrote:
>> Bump.  Now rebased against current head (with no conflicts) on
>> atomic-new-v2 (and for-review/atomic-new-v2) at
>>  http://awakening.csail.mit.edu/git/notmuch.git/
>>
>> On Fri, Feb 18, 2011 at 2:58 AM, Austin Clements <amdragon@mit.edu> wrote:
>>> This patch series modifies notmuch new to perform all operations
>>> atomically and to perform maildir flag synchronization eagerly.  As a
>>> result, notmuch new can be interrupted without risking database
>>> consistency or losing track of messages, but still without losing
>>> progress in the middle of a big import.  This also paves the way for
>>> fixing the antisocial locking behavior of notmuch new.
>>>
>>> While there are quite a few patches in the series, each one is
>>> bite-sized and you can see the number of atomicity violations dropping
>>> with nearly every patch using the test added by the first patch.
>>>
>>> On my test machine, these patches have no affect on performance.
>>>
>>> These patches are also available on the atomic-new-v1 branch at
>>>  http://awakening.csail.mit.edu/git/notmuch.git/
>>>
>>> _______________________________________________
>>> notmuch mailing list
>>> notmuch@notmuchmail.org
>>> http://notmuchmail.org/mailman/listinfo/notmuch
>>>
>>
>

Re: [PATCH 00/10] Fix 'notmuch new' atomicity issues

[Carl Worth]

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

On Sat, 28 May 2011 22:51:10 -0400, Austin Clements <amdragon@mit.edu> wrote:
> Rebased to current master (cb8418) as atomic-new-v4 (aka
> for-review/atomic-new-v4).

Hi Austin,

Thanks so much for sending this series (and 4 times, even!).

I *really* like the new robustness provided by this series, and I
especially like the exhaustive testing here. Thanks so much!

Having just gone through the for-review/atomic-new-v4 series, I have a
few comments. Some are very minor and I'll be glad to implement them
myself:

	1. Two commits have "lose" misspelled as "loose". These are "ew:
	   don't loose messages on SIGINT" and "new: Wrap adding a
	   message in an atomic section".

	2. The commit with summary of "lib: Make _notmuch_message_sync
	   capable of deleting a message." is missing the rest of its
	   commit message with a complete explanation. For example, this
	   commit message should describe that a message document is
	   deleted from the database (if the deleted field is set when
	   _sync is called). And the commit message should also mention
	   that this functionality is not currently used, but prepares
	   for a subsequent use.

	3. While reviewing the commit "lib: Indicate if there are more
	   filenames after removal" the "if (status ==
	   NOTMUCH_STATUS_SUCCESS)" looked out of place to me. Indeed,
	   if status is any other value at this point in the code, then
	   the function should have returned earlier. I intend to follow
	   up with a commit that adds the missing early return and
	   removes this condition.

	4. I really don't like that the final state of the code has two
	   different functions named notmuch_message_remove_filename and
	   _notmuch_message_remove_filename. If the semantics of these
	   functions are identical, then there should be only one
	   function. If the semantics are different, then they need to
	   have noticeably distinct names, (and a single underscore
	   doesn't count).

	5. The final code has a function inside of notmuch-new.c named
	   "remove_file", but this function isn't removing a
	   file---instead it's removing a message document from the
	   database. So it needs a more accurate name.

Like I said, those are all pretty minor and I would just implement all
of those and push the series myself, but for one remaining issue that is
a bit more significant.

The last issue has to do with the addition of the
notmuch_database_find_message_by_filename and
notmuch_message_remove_filename functions. In the series as it stands,
notmuch-new.c is updated to call these two functions instead of calling
the existing notmuch_database_remove_message function (which itself also
calls the same functions).

That sets off a red flag in my mind. If our program is avoiding a
library function and substituting its own implementation, how are other
users of the library going to get things right? Should we deprecate
notmuch_database_remove_message? Should we add more documentation to it
describing the situation in which a user might prefer not to call it? It
seems the library is harder to use than it should be in this area.

Meanwhile, I'm not very satisfied by the existence of
notmuch_message_remove_filename in the public API. It would have a
natural pairing with notmuch_message_add_filename, but the series isn't
exporting that functionality. So things feel more asymmetric than they
should be as well.

Now, why is notmuch-new going through all this effort to reimplement an
existing library function (and requiring two new library functions in
the process)? What it wants to do is to wrap the functionality of
database_remove_message in freeze/thaw and while the message is frozen
call notmuch_message_maildir_flags_to_tags.

So, how to fix my complaints above?

  * Do we want to allow database_remove_message to optionally call
    maildir_flags_to_tags?

    This seems a little messy in requiring some additional information
    to the library so it can know whether to do the maildir
    synchronization here. And it's also asymmetric unless we would also
    support similar synchronization support in the library for simlar
    operations.

  * Do we want to expose notmuch_message_add_filename as well as
    remove_filename for better symmetry?

    I'm not sure I like that. It still feels like we're exposing too
    many internals and not making it obvious to the user how to do
    things. Having just the existing add_message/remove_message
    functions definitely makes the interface easier.

  * Can we fix the remove case without this new library API by simply
    adding calls to begin_atomic and end_atomic?

    I think this is probably the solution I would prefer to see.

What do you think, Austin?

-Carl

-- 
carl.d.worth@intel.com

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

Re: [PATCH 00/10] Fix 'notmuch new' atomicity issues

[Austin Clements]

Quoth Carl Worth on Jun 08 at  3:05 pm:
> On Sat, 28 May 2011 22:51:10 -0400, Austin Clements <amdragon@mit.edu> wrote:
> > Rebased to current master (cb8418) as atomic-new-v4 (aka
> > for-review/atomic-new-v4).
> 
> Hi Austin,
> 
> Thanks so much for sending this series (and 4 times, even!).
> 
> I *really* like the new robustness provided by this series, and I
> especially like the exhaustive testing here. Thanks so much!
> 
> Having just gone through the for-review/atomic-new-v4 series, I have a
> few comments. Some are very minor and I'll be glad to implement them
> myself:
> 
> 	1. Two commits have "lose" misspelled as "loose". These are "ew:
> 	   don't loose messages on SIGINT" and "new: Wrap adding a
> 	   message in an atomic section".

Ooops.

> 	2. The commit with summary of "lib: Make _notmuch_message_sync
> 	   capable of deleting a message." is missing the rest of its
> 	   commit message with a complete explanation. For example, this
> 	   commit message should describe that a message document is
> 	   deleted from the database (if the deleted field is set when
> 	   _sync is called). And the commit message should also mention
> 	   that this functionality is not currently used, but prepares
> 	   for a subsequent use.

Fixed.

> 	3. While reviewing the commit "lib: Indicate if there are more
> 	   filenames after removal" the "if (status ==
> 	   NOTMUCH_STATUS_SUCCESS)" looked out of place to me. Indeed,
> 	   if status is any other value at this point in the code, then
> 	   the function should have returned earlier. I intend to follow
> 	   up with a commit that adds the missing early return and
> 	   removes this condition.

Okay.  I suspect I was just retaining the error semantics in this
function (which were probably a hold-out from before the folder search
patch).  I've slipped a patch in at the beginning that adds the
missing check and removed the condition.

> 	4. I really don't like that the final state of the code has two
> 	   different functions named notmuch_message_remove_filename and
> 	   _notmuch_message_remove_filename. If the semantics of these
> 	   functions are identical, then there should be only one
> 	   function. If the semantics are different, then they need to
> 	   have noticeably distinct names, (and a single underscore
> 	   doesn't count).

Too much Linux kernel hacking, I suppose.  I've left this alone for
the moment because it's likely to change with the below discussion.

(Two solutions are either to rename _notmuch_message_remove_filename
something even more ridiculously long like
_notmuch_message_remove_filename_no_delete, or to make
notmuch_message_tags_to_maildir_flags first add the new file name and
then remove the old one, so a message can't transiently have no file
names and the merge the two filename removal functions into one.)

> 	5. The final code has a function inside of notmuch-new.c named
> 	   "remove_file", but this function isn't removing a
> 	   file---instead it's removing a message document from the
> 	   database. So it needs a more accurate name.

Mm.  It's now remove_filename (could be remove_message_filename?)
It's *might* remove a message document from the database, but its
primary purpose is to remove a filename from a message.

I've pushed the easy changes as atomic-new-v5, mostly to get them in
the record.

> Like I said, those are all pretty minor and I would just implement all
> of those and push the series myself, but for one remaining issue that is
> a bit more significant.
> 
> The last issue has to do with the addition of the
> notmuch_database_find_message_by_filename and
> notmuch_message_remove_filename functions. In the series as it stands,
> notmuch-new.c is updated to call these two functions instead of calling
> the existing notmuch_database_remove_message function (which itself also
> calls the same functions).
> 
> That sets off a red flag in my mind. If our program is avoiding a
> library function and substituting its own implementation, how are other
> users of the library going to get things right? Should we deprecate
> notmuch_database_remove_message? Should we add more documentation to it
> describing the situation in which a user might prefer not to call it? It
> seems the library is harder to use than it should be in this area.

The intent was to deprecate notmuch_database_remove_message, yes.

> Meanwhile, I'm not very satisfied by the existence of
> notmuch_message_remove_filename in the public API. It would have a
> natural pairing with notmuch_message_add_filename, but the series isn't
> exporting that functionality. So things feel more asymmetric than they
> should be as well.

Part of why atomicity was a mess was because the API blurs the
distinction between a message as a concrete, single file and a message
as a message-id that may have many file names.
find_message_by_filename and remove_filename were attempting to
sharpen this distinction.

But, maybe they sharpen it in the wrong direction.  An alternate way
to look at this is that a message is a single file that can also tell
you file names that contain equivalent messages.  This might be more
of a mindset (or documentation change) than an actual API change; I'm
not sure.  It certainly fits better with the existing
{add,remove}_message, but it's not clear if that's intentional or
historical.  Thoughts?

> Now, why is notmuch-new going through all this effort to reimplement an
> existing library function (and requiring two new library functions in
> the process)? What it wants to do is to wrap the functionality of
> database_remove_message in freeze/thaw and while the message is frozen
> call notmuch_message_maildir_flags_to_tags.
> 
> So, how to fix my complaints above?
> 
>   * Do we want to allow database_remove_message to optionally call
>     maildir_flags_to_tags?
> 
>     This seems a little messy in requiring some additional information
>     to the library so it can know whether to do the maildir
>     synchronization here. And it's also asymmetric unless we would also
>     support similar synchronization support in the library for simlar
>     operations.
> 
>   * Do we want to expose notmuch_message_add_filename as well as
>     remove_filename for better symmetry?
> 
>     I'm not sure I like that. It still feels like we're exposing too
>     many internals and not making it obvious to the user how to do
>     things. Having just the existing add_message/remove_message
>     functions definitely makes the interface easier.
> 
>   * Can we fix the remove case without this new library API by simply
>     adding calls to begin_atomic and end_atomic?
> 
>     I think this is probably the solution I would prefer to see.
> 
> What do you think, Austin?

Of these three, I would definitely go with the last.  In fact, I tried
the first two when I was originally designing this patch and can
assure you they'll get us into trouble.  ]:--8)

I recall a few reasons for why I designed this the way I did.  One was
what I mentioned above about sharpening the distinction between
messages and filenames.  Another was that I wanted to reuse the
freeze/thaw mechanism; in fact, I introduced atomic sections only when
I realized that using freeze/thaw was going to be impossible for add.
Perhaps it makes more sense to lean the other direction.  Finally, I
felt that it was important that the API be easy to use correctly, from
an atomicity standpoint; hence the introduction of more operations
that were meaningful on frozen messages (I also tried to make it so
you couldn't overlook atomicity issues when using the library, but I
don't think I succeeded).

That last reason is also compatible with your last suggestion.  If we
move to atomic sections, I think we have to make sure the library
never internally violates atomicity and that the library user only
needs to use atomic sections directly if they need atomicity across
multiple library calls.  This shouldn't be hard, especially with
nested atomics.

I'll give this a try and see where it leads.

> -Carl

Re: [PATCH 00/10] Fix 'notmuch new' atomicity issues

[Carl Worth]

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

On Fri, 10 Jun 2011 17:11:03 -0400, Austin Clements <amdragon@MIT.EDU> wrote:
> Mm.  It's now remove_filename (could be remove_message_filename?)

I like remove_filename just fine. Thanks.

> I've pushed the easy changes as atomic-new-v5, mostly to get them in
> the record.

Thanks. I'll look. These should all be ready to push with the
discussion-pending stuff to come?

> But, maybe they sharpen it in the wrong direction.  An alternate way
> to look at this is that a message is a single file that can also tell
> you file names that contain equivalent messages.  This might be more
> of a mindset (or documentation change) than an actual API change; I'm
> not sure.  It certainly fits better with the existing
> {add,remove}_message, but it's not clear if that's intentional or
> historical.  Thoughts?

That seems to match what I had in mind when I designed the original
{add,remove}_message, yes. So I'm interested in running with this to see
if we can't make it work.

One trick is that most of the early design was done by me in a
vacuum. I think we're fortunate to now have a wider community to help
catch design mistakes of mine.

> >   * Can we fix the remove case without this new library API by simply
> >     adding calls to begin_atomic and end_atomic?
> > 
> >     I think this is probably the solution I would prefer to see.
> > 
> > What do you think, Austin?
> 
> Of these three, I would definitely go with the last.

Good. We're thinking along the same lines at least.

> That last reason is also compatible with your last suggestion.  If we
> move to atomic sections, I think we have to make sure the library
> never internally violates atomicity and that the library user only
> needs to use atomic sections directly if they need atomicity across
> multiple library calls.  This shouldn't be hard, especially with
> nested atomics.

Thanks for providing so much of your rationale. The constraint you
describe here sounds perfect. With the library respecting this, it seems
it would make it very easy for anyone reviewing notmuch-new.c (or
implementing something similar from scratch) to correctly identify the
spots that need begin_atomic/end_atomic.

> I'll give this a try and see where it leads.

I'm looking forward to what you come up with.[*]

-Carl

[*] To satisfy grammarian pedantism—A preposition (let alone two) is
something you should never end a sentence with—I might offer:

	I'm looking forward to that with which you up will come.

-- 
carl.d.worth@intel.com

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

Re: [PATCH 00/10] Fix 'notmuch new' atomicity issues

[Austin Clements]

Quoth Carl Worth on Jun 10 at  3:02 pm:
> On Fri, 10 Jun 2011 17:11:03 -0400, Austin Clements <amdragon@MIT.EDU> wrote:
> > I've pushed the easy changes as atomic-new-v5, mostly to get them in
> > the record.
> 
> Thanks. I'll look. These should all be ready to push with the
> discussion-pending stuff to come?

I wouldn't push them, since a lot of code will probably change if we
go with the atomic sections approach (and much of it will probably
revert as the changes move to other parts of the code).

[PATCH v6 00/17] Fix 'notmuch new' atomicity issues

[Austin Clements]

Here's the reworked patch series that uses atomic sections more
heavily rather than changing the removal API.  This is atomic-new-v6
on http://awakening.csail.mit.edu/git/notmuch.git .

(I was planning to make this series on Monday while stuck on a plane,
but an opportunity presented itself when I needed something better to
do than fix the sink.  As a result, if you can look over this before
Monday, I can use that time to address any further comments.)

The beginning of the sequence---"test: Fix message when skipping
test_expect_equal* tests" through "new: Defer updating directory
mtimes until the end."---has not changed besides than the minor things
you pointed out earlier.  Here's a quick summary of the differences in
the rest of the sequence:

lib: Add notmuch_database_{begin,end}_atomic.
  Rebased but otherwise identical.

lib: Add support for nested atomic sections.
  New.

lib: Indicate if there are more filenames after removal.
  Rebased but otherwise identical.

lib: Remove message document directly after removing the last file
name.
  New.  Supersedes the patches that rewrote
  notmuch_database_remove_message and made sync delete messages.

lib: Add an API to find a message by filename.
  Culled from "lib: Add API's to find by filename and ..." in the old
  series.  What I kept is identical.

lib: Wrap notmuch_database_add_message in an atomic section.
  New.

new: Cleanup. Put removed/renamed message count in add_files_state_t.
new: Cleanup. De-duplicate file name removal code.
  Both new, but closely related to previous remove_filename patch.

new: Synchronize maildir flags eagerly.
  New, but very closely related to previous patch of the same name.
  This patch is simpler now because of the previous two cleanups.

new: Wrap adding and removing messages in atomic sections.
  Rebased and added an atomic section around removal.

lib: Improve notmuch_database_{add,remove}_message documentation.
  New.

Ultimately, I still think the existing removal API is weird because it
forces you to do things like lookup a message by file name just in
case you need that message object after removal tells you that it
didn't actually remove the message.  But I don't know what to do about
it that isn't equally weird.

[PATCH 02/17] test: Report test failures from test_expect_*

[Austin Clements]

This makes test_expect_* return non-zero if the test fails, so the
caller can make decisions based on this, such as setting test
prerequisites.
---
 test/test-lib.sh |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/test/test-lib.sh b/test/test-lib.sh
index d0e2794..a122a16 100755
--- a/test/test-lib.sh
+++ b/test/test-lib.sh
@@ -581,6 +581,7 @@ test_failure_ () {
 	echo "$@" | sed -e 's/^/	/'
 	if test "$verbose" != "t"; then cat test.output; fi
 	test "$immediate" = "" || { GIT_EXIT_OK=t; exit 1; }
+	return 1
 }
 
 test_known_broken_ok_ () {
@@ -593,6 +594,7 @@ test_known_broken_failure_ () {
 	test_broken=$(($test_broken+1))
 	say_color pass "%-6s" "BROKEN"
 	echo " $@"
+	return 1
 }
 
 test_debug () {
-- 
1.7.5.1

[PATCH 03/17] lib: Add missing status check in _notmuch_message_remove_filename.

[Austin Clements]

Previously, this function would synchronize the folder list even if
removing the file name failed.  Now it returns immediately if removing
the file name fails.
---
 lib/message.cc |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/lib/message.cc b/lib/message.cc
index 4b59fa9..afb1d80 100644
--- a/lib/message.cc
+++ b/lib/message.cc
@@ -529,6 +529,8 @@ _notmuch_message_remove_filename (notmuch_message_t *message,
 						   "file-direntry", direntry);
     status = COERCE_STATUS (private_status,
 			    "Unexpected error from _notmuch_message_remove_term");
+    if (status)
+	return status;
 
     /* Re-synchronize "folder:" terms for this message. This requires
      * first removing all "folder:" terms, then adding back terms for
-- 
1.7.5.1

[PATCH 04/17] test: Test atomicity of notmuch new.

[Austin Clements]

This tests notmuch new's ability to recover from arbitrary stopping
failures.  It interrupts notmuch new after every database commit and,
on every resulting database snapshot, re-runs notmuch new to
completion and checks that the final database state is invariant.
---
 test/atomicity     |  103 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 test/atomicity.gdb |   50 +++++++++++++++++++++++++
 test/basic         |    1 +
 test/notmuch-test  |    1 +
 4 files changed, 155 insertions(+), 0 deletions(-)
 create mode 100755 test/atomicity
 create mode 100644 test/atomicity.gdb

diff --git a/test/atomicity b/test/atomicity
new file mode 100755
index 0000000..817819a
--- /dev/null
+++ b/test/atomicity
@@ -0,0 +1,103 @@
+#!/bin/bash
+test_description='atomicity'
+. ./test-lib.sh
+
+# This script tests the effects of killing and restarting "notmuch
+# new" at arbitrary points.  If notmuch new is properly atomic, the
+# final database contents should be the same regardless of when (or
+# if) it is killed and restarted.
+
+if test_expect_success "prereq: GDB is present" "which gdb"; then
+    test_set_prereq GDB
+fi
+
+# Create a maildir structure to also stress flag synchronization
+mkdir $MAIL_DIR/cur
+mkdir $MAIL_DIR/new
+mkdir $MAIL_DIR/tmp
+mkdir $MAIL_DIR/.remove-dir
+
+# Prepare the initial database
+generate_message [subject]='Duplicate' [filename]='duplicate:2,' [dir]=cur
+generate_message [subject]='Remove' [filename]='remove:2,' [dir]=cur
+generate_message [subject]='"Remove duplicate"' [filename]='remove-duplicate:2,' [dir]=cur
+cp $MAIL_DIR/cur/remove-duplicate:2, $MAIL_DIR/cur/remove-duplicate-copy:2,
+generate_message [subject]='Rename' [filename]='rename:2,' [dir]=cur
+generate_message [subject]='"Rename duplicate"' [filename]='rename-duplicate:2,' [dir]=cur
+generate_message [subject]='"Move 1"' [filename]='move1:2,' [dir]=cur
+generate_message [subject]='"Move 2"' [filename]='move2:2,' [dir]=new
+generate_message [subject]='Flag' [filename]='flag:2,' [dir]=cur
+generate_message [subject]='"Flag duplicate"' [filename]='flag-duplicate:2,' [dir]=cur
+cp $MAIL_DIR/cur/flag-duplicate:2, $MAIL_DIR/cur/flag-duplicate-copy:2,F
+generate_message [subject]='"Remove directory"' [filename]='remove-directory:2,' [dir]=.remove-dir
+generate_message [subject]='"Remove directory duplicate"' [filename]='remove-directory-duplicate:2,' [dir]=.remove-dir
+cp $MAIL_DIR/.remove-dir/remove-directory-duplicate:2, $MAIL_DIR/cur/
+notmuch new > /dev/null
+
+# Make all maildir changes, but *don't* update the database
+generate_message [subject]='Added' [filename]='added:2,' [dir]=cur
+cp $MAIL_DIR/cur/duplicate:2, $MAIL_DIR/cur/duplicate-copy:2,
+generate_message [subject]='"Add duplicate"' [filename]='add-duplicate:2,' [dir]=cur
+generate_message [subject]='"Add duplicate copy"' [filename]='add-duplicate-copy:2,' [dir]=cur
+rm $MAIL_DIR/cur/remove:2,
+rm $MAIL_DIR/cur/remove-duplicate-copy:2,
+mv $MAIL_DIR/cur/rename:2, $MAIL_DIR/cur/renamed:2,
+mv $MAIL_DIR/cur/rename-duplicate:2, $MAIL_DIR/cur/renamed-duplicate:2,
+mv $MAIL_DIR/cur/move1:2, $MAIL_DIR/new/move1:2,
+mv $MAIL_DIR/new/move2:2, $MAIL_DIR/cur/move2:2,
+mv $MAIL_DIR/cur/flag:2, $MAIL_DIR/cur/flag:2,F
+rm $MAIL_DIR/cur/flag-duplicate-copy:2,F
+rm $MAIL_DIR/.remove-dir/remove-directory:2,
+rm $MAIL_DIR/.remove-dir/remove-directory-duplicate:2,
+rmdir $MAIL_DIR/.remove-dir
+increment_mtime $MAIL_DIR/cur
+increment_mtime $MAIL_DIR/new
+increment_mtime $MAIL_DIR
+
+# Prepare a snapshot of the updated maildir.  The gdb script will
+# update the database in this snapshot as it goes.
+cp -ra $MAIL_DIR $MAIL_DIR.snap
+cp ${NOTMUCH_CONFIG} ${NOTMUCH_CONFIG}.snap
+NOTMUCH_CONFIG=${NOTMUCH_CONFIG}.snap notmuch config set database.path $MAIL_DIR.snap
+
+
+test_begin_subtest '"notmuch new" is idempotent under arbitrary aborts'
+
+# Execute notmuch new and, at every call to rename, snapshot the
+# database, run notmuch new again on the snapshot, and capture the
+# results of search.
+#
+# -tty /dev/null works around a conflict between the 'timeout' wrapper
+# and gdb's attempt to control the TTY.
+export MAIL_DIR
+gdb -tty /dev/null -batch -x $TEST_DIRECTORY/atomicity.gdb notmuch >/dev/null 2>/dev/null
+
+# Get the final, golden output
+notmuch search '*' > expected
+
+# Check output against golden output
+outcount=$(cat outcount)
+echo -n > searchall
+echo -n > expectall
+for ((i = 0; i < $outcount; i++)); do
+    if ! cmp -s search.$i expected; then
+        # Find the range of interruptions that match this output
+        for ((end = $i + 1 ; end < $outcount; end++)); do
+            if ! cmp -s search.$i search.$end; then
+                break
+            fi
+        done
+        echo "When interrupted after $test/backtrace.$(expr $i - 1) (abort points $i-$(expr $end - 1))" >> searchall
+        cat search.$i >> searchall
+        cat expected >> expectall
+        echo >> searchall
+        echo >> expectall
+
+        i=$(expr $end - 1)
+    fi
+done
+test_expect_equal_failure GDB "$(cat searchall)" "$(cat expectall)"
+
+test_expect_success GDB "detected $outcount>10 abort points" "test $outcount -gt 10"
+
+test_done
diff --git a/test/atomicity.gdb b/test/atomicity.gdb
new file mode 100644
index 0000000..fd67525
--- /dev/null
+++ b/test/atomicity.gdb
@@ -0,0 +1,50 @@
+# This gdb script runs notmuch new and simulates killing and
+# restarting notmuch new after every Xapian commit.  To simulate this
+# more efficiently, this script runs notmuch new and, immediately
+# after every Xapian commit, it *pauses* the running notmuch new,
+# copies the entire database and maildir to a snapshot directory, and
+# executes a full notmuch new on that snapshot, comparing the final
+# results with the expected output.  It can then resume the paused
+# notmuch new, which is still running on the original maildir, and
+# repeat this process.
+
+set args new
+
+# Make Xapian commit after every operation instead of batching
+set environment XAPIAN_FLUSH_THRESHOLD = 1
+
+# gdb can't keep track of a simple integer.  This is me weeping.
+shell echo 0 > outcount
+
+shell touch inodes
+
+break rename
+commands
+# As an optimization, only consider snapshots after a Xapian commit.
+# Xapian overwrites record.base? as the last step in the commit.
+shell echo > gdbcmd
+shell stat -c %i $MAIL_DIR/.notmuch/xapian/record.base* > inodes.new
+shell if cmp inodes inodes.new; then echo cont > gdbcmd; fi
+shell mv inodes.new inodes
+source gdbcmd
+
+# Save a backtrace in case the test does fail
+set logging file backtrace
+set logging on
+backtrace
+set logging off
+shell mv backtrace backtrace.`cat outcount`
+
+# Snapshot the database
+shell rm -r $MAIL_DIR.snap/.notmuch
+shell cp -r $MAIL_DIR/.notmuch $MAIL_DIR.snap/.notmuch
+# Restore the mtime of $MAIL_DIR.snap, which we just changed
+shell touch -r $MAIL_DIR $MAIL_DIR.snap
+# Run notmuch new to completion on the snapshot
+shell NOTMUCH_CONFIG=${NOTMUCH_CONFIG}.snap XAPIAN_FLUSH_THRESHOLD=1000 notmuch new > /dev/null
+shell NOTMUCH_CONFIG=${NOTMUCH_CONFIG}.snap notmuch search '*' > search.`cat outcount` 2>&1
+shell echo $(expr $(cat outcount) + 1) > outcount
+cont
+end
+
+run
diff --git a/test/basic b/test/basic
index 808c968..5e0adf0 100755
--- a/test/basic
+++ b/test/basic
@@ -60,6 +60,7 @@ available=$(ls -1 ../ | \
 	   -e "/^(test.expected-output|.*~)/d" \
 	   -e "/^(gnupg-secret-key.asc)/d" \
 	   -e "/^(gnupg-secret-key.NOTE)/d" \
+	   -e "/^(atomicity.gdb)/d" \
 	   | sort)
 test_expect_equal "$tests_in_suite" "$available"
 
diff --git a/test/notmuch-test b/test/notmuch-test
index 9f58c12..c540228 100755
--- a/test/notmuch-test
+++ b/test/notmuch-test
@@ -40,6 +40,7 @@ TESTS="
   emacs-large-search-buffer
   maildir-sync
   crypto
+  atomicity
 "
 TESTS=${NOTMUCH_TESTS:=$TESTS}
 
-- 
1.7.5.1

[PATCH 05/17] new: Don't lose messages on SIGINT.

[Austin Clements]

Previously, message removals were always performed, even after a
SIGINT.  As a result, when a message was moved from one folder to
another, a SIGINT between processing the directory the message was
removed from and processing the directory it was added to would result
in notmuch removing that message from the database.
---
 notmuch-new.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/notmuch-new.c b/notmuch-new.c
index 744f4ca..6859594 100644
--- a/notmuch-new.c
+++ b/notmuch-new.c
@@ -841,7 +841,7 @@ notmuch_new_command (void *ctx, int argc, char *argv[])
     removed_files = 0;
     renamed_files = 0;
     gettimeofday (&tv_start, NULL);
-    for (f = add_files_state.removed_files->head; f; f = f->next) {
+    for (f = add_files_state.removed_files->head; f && !interrupted; f = f->next) {
 	status = notmuch_database_remove_message (notmuch, f->filename);
 	if (status == NOTMUCH_STATUS_DUPLICATE_MESSAGE_ID)
 	    renamed_files++;
@@ -856,7 +856,7 @@ notmuch_new_command (void *ctx, int argc, char *argv[])
     }
 
     gettimeofday (&tv_start, NULL);
-    for (f = add_files_state.removed_directories->head, i = 0; f; f = f->next, i++) {
+    for (f = add_files_state.removed_directories->head, i = 0; f && !interrupted; f = f->next, i++) {
 	_remove_directory (ctx, notmuch, f->filename,
 			   &renamed_files, &removed_files);
 	if (do_print_progress) {
-- 
1.7.5.1

[PATCH 06/17] new: Defer updating directory mtimes until the end.

[Austin Clements]

Previously, if notmuch new were interrupted between updating the
directory mtime and handling removals from that directory, a
subsequent notmuch new would not handle those removals until something
else changed in that directory.  This defers recording the updated
mtime until after removals are handled to eliminate this problem.
---
 notmuch-new.c |   23 +++++++++++++++++------
 1 files changed, 17 insertions(+), 6 deletions(-)

diff --git a/notmuch-new.c b/notmuch-new.c
index 6859594..b76b608 100644
--- a/notmuch-new.c
+++ b/notmuch-new.c
@@ -24,6 +24,7 @@
 
 typedef struct _filename_node {
     char *filename;
+    time_t mtime;
     struct _filename_node *next;
 } _filename_node_t;
 
@@ -46,6 +47,7 @@ typedef struct {
 
     _filename_list_t *removed_files;
     _filename_list_t *removed_directories;
+    _filename_list_t *directory_mtimes;
 
     notmuch_bool_t synchronize_flags;
     _filename_list_t *message_ids_to_sync;
@@ -86,7 +88,7 @@ _filename_list_create (const void *ctx)
     return list;
 }
 
-static void
+static _filename_node_t *
 _filename_list_add (_filename_list_t *list,
 		    const char *filename)
 {
@@ -99,6 +101,8 @@ _filename_list_add (_filename_list_t *list,
 
     *(list->tail) = node;
     list->tail = &node->next;
+
+    return node;
 }
 
 static void
@@ -509,11 +513,7 @@ add_files_recursive (notmuch_database_t *notmuch,
 	notmuch_filenames_move_to_next (db_subdirs);
     }
 
-    if (! interrupted) {
-	status = notmuch_directory_set_mtime (directory, fs_mtime);
-	if (status && ret == NOTMUCH_STATUS_SUCCESS)
-	    ret = status;
-    }
+    _filename_list_add (state->directory_mtimes, path)->mtime = fs_mtime;
 
   DONE:
     if (next)
@@ -829,6 +829,7 @@ notmuch_new_command (void *ctx, int argc, char *argv[])
 
     add_files_state.removed_files = _filename_list_create (ctx);
     add_files_state.removed_directories = _filename_list_create (ctx);
+    add_files_state.directory_mtimes = _filename_list_create (ctx);
 
     if (! debugger_is_active () && add_files_state.output_is_a_tty
 	&& ! add_files_state.verbose) {
@@ -867,8 +868,18 @@ notmuch_new_command (void *ctx, int argc, char *argv[])
 	}
     }
 
+    for (f = add_files_state.directory_mtimes->head; f && !interrupted; f = f->next) {
+	notmuch_directory_t *directory;
+	directory = notmuch_database_get_directory (notmuch, f->filename);
+	if (directory) {
+	    notmuch_directory_set_mtime (directory, f->mtime);
+	    notmuch_directory_destroy (directory);
+	}
+    }
+
     talloc_free (add_files_state.removed_files);
     talloc_free (add_files_state.removed_directories);
+    talloc_free (add_files_state.directory_mtimes);
 
     /* Now that removals are done (hence the database is aware of all
      * renames), we can synchronize maildir_flags to tags for all
-- 
1.7.5.1

[PATCH 07/17] lib: Add notmuch_database_{begin,end}_atomic.

[Austin Clements]

These operations translate into non-flushed Xapian transactions,
allowing arbitrary groups of database operations to be performed
atomically.
---
 lib/database.cc |   44 ++++++++++++++++++++++++++++++++++++++++++++
 lib/notmuch.h   |   30 ++++++++++++++++++++++++++++++
 2 files changed, 74 insertions(+), 0 deletions(-)

diff --git a/lib/database.cc b/lib/database.cc
index 7f79cf4..ddb6167 100644
--- a/lib/database.cc
+++ b/lib/database.cc
@@ -974,6 +974,50 @@ notmuch_database_upgrade (notmuch_database_t *notmuch,
     return NOTMUCH_STATUS_SUCCESS;
 }
 
+notmuch_status_t
+notmuch_database_begin_atomic (notmuch_database_t *notmuch)
+{
+    if (notmuch->mode == NOTMUCH_DATABASE_MODE_READ_ONLY)
+	return NOTMUCH_STATUS_SUCCESS;
+
+    try {
+	(static_cast <Xapian::WritableDatabase *> (notmuch->xapian_db))->begin_transaction (false);
+    } catch (const Xapian::Error &error) {
+	fprintf (stderr, "A Xapian exception occurred beginning transaction: %s.\n",
+		 error.get_msg().c_str());
+	notmuch->exception_reported = TRUE;
+	return NOTMUCH_STATUS_XAPIAN_EXCEPTION;
+    }
+    return NOTMUCH_STATUS_SUCCESS;
+}
+
+notmuch_status_t
+notmuch_database_end_atomic (notmuch_database_t *notmuch)
+{
+    Xapian::WritableDatabase *db;
+
+    if (notmuch->mode == NOTMUCH_DATABASE_MODE_READ_ONLY)
+	return NOTMUCH_STATUS_SUCCESS;
+
+    db = static_cast <Xapian::WritableDatabase *> (notmuch->xapian_db);
+    try {
+	db->commit_transaction ();
+
+	/* This is a hack for testing.  Xapian never flushes on a
+	 * non-flushed commit, even if the flush threshold is 1.
+	 * However, we rely on flushing to test atomicity. */
+	const char *thresh = getenv ("XAPIAN_FLUSH_THRESHOLD");
+	if (thresh && atoi (thresh) == 1)
+	    db->commit ();
+    } catch (const Xapian::Error &error) {
+	fprintf (stderr, "A Xapian exception occurred committing transaction: %s.\n",
+		 error.get_msg().c_str());
+	notmuch->exception_reported = TRUE;
+	return NOTMUCH_STATUS_XAPIAN_EXCEPTION;
+    }
+    return NOTMUCH_STATUS_SUCCESS;
+}
+
 /* We allow the user to use arbitrarily long paths for directories. But
  * we have a term-length limit. So if we exceed that, we'll use the
  * SHA-1 of the path for the database term.
diff --git a/lib/notmuch.h b/lib/notmuch.h
index e508309..208d22c 100644
--- a/lib/notmuch.h
+++ b/lib/notmuch.h
@@ -214,6 +214,36 @@ notmuch_database_upgrade (notmuch_database_t *database,
 						   double progress),
 			  void *closure);
 
+/* Begin an atomic database operation.
+ *
+ * Any modifications performed between a successful begin and a
+ * notmuch_database_end_atomic will be applied to the database
+ * atomically.  Note that, unlike a typical database transaction, this
+ * only ensures atomicity, not durability; neither begin nor end
+ * necessarily flush modifications to disk.
+ *
+ * Return value:
+ *
+ * NOTMUCH_STATUS_SUCCESS: Successfully entered atomic section.
+ *
+ * NOTMUCH_STATUS_XAPIAN_EXCEPTION: A Xapian exception occurred;
+ *	atomic section not entered.
+ */
+notmuch_status_t
+notmuch_database_begin_atomic (notmuch_database_t *notmuch);
+
+/* Indicate the end of an atomic database operation.
+ *
+ * Return value:
+ *
+ * NOTMUCH_STATUS_SUCCESS: Successfully completed atomic section.
+ *
+ * NOTMUCH_STATUS_XAPIAN_EXCEPTION: A Xapian exception occurred;
+ *	atomic section not ended.
+ */
+notmuch_status_t
+notmuch_database_end_atomic (notmuch_database_t *notmuch);
+
 /* Retrieve a directory object from the database for 'path'.
  *
  * Here, 'path' should be a path relative to the path of 'database'
-- 
1.7.5.1

[PATCH 08/17] lib: Add support for nested atomic sections.

[Austin Clements]

notmuch_database_t now keeps a nesting count and we only start a
transaction or commit for the outermost atomic section.

Introduces a new error, NOTMUCH_STATUS_UNBALANCED_ATOMIC.
---
 lib/database-private.h |    1 +
 lib/database.cc        |   22 ++++++++++++++++++----
 lib/notmuch.h          |   10 ++++++++++
 notmuch-new.c          |    1 +
 4 files changed, 30 insertions(+), 4 deletions(-)

diff --git a/lib/database-private.h b/lib/database-private.h
index f705009..88532d5 100644
--- a/lib/database-private.h
+++ b/lib/database-private.h
@@ -43,6 +43,7 @@ struct _notmuch_database {
 
     notmuch_bool_t needs_upgrade;
     notmuch_database_mode_t mode;
+    int atomic_nesting;
     Xapian::Database *xapian_db;
 
     unsigned int last_doc_id;
diff --git a/lib/database.cc b/lib/database.cc
index ddb6167..b766e94 100644
--- a/lib/database.cc
+++ b/lib/database.cc
@@ -273,6 +273,8 @@ notmuch_status_to_string (notmuch_status_t status)
 	return "Tag value is too long (exceeds NOTMUCH_TAG_MAX)";
     case NOTMUCH_STATUS_UNBALANCED_FREEZE_THAW:
 	return "Unbalanced number of calls to notmuch_message_freeze/thaw";
+    case NOTMUCH_STATUS_UNBALANCED_ATOMIC:
+	return "Unbalanced number of calls to notmuch_database_begin_atomic/end_atomic";
     default:
     case NOTMUCH_STATUS_LAST_STATUS:
 	return "Unknown error status value";
@@ -611,6 +613,7 @@ notmuch_database_open (const char *path,
 
     notmuch->needs_upgrade = FALSE;
     notmuch->mode = mode;
+    notmuch->atomic_nesting = 0;
     try {
 	string last_thread_id;
 
@@ -977,8 +980,9 @@ notmuch_database_upgrade (notmuch_database_t *notmuch,
 notmuch_status_t
 notmuch_database_begin_atomic (notmuch_database_t *notmuch)
 {
-    if (notmuch->mode == NOTMUCH_DATABASE_MODE_READ_ONLY)
-	return NOTMUCH_STATUS_SUCCESS;
+    if (notmuch->mode == NOTMUCH_DATABASE_MODE_READ_ONLY ||
+	notmuch->atomic_nesting > 0)
+	goto DONE;
 
     try {
 	(static_cast <Xapian::WritableDatabase *> (notmuch->xapian_db))->begin_transaction (false);
@@ -988,6 +992,9 @@ notmuch_database_begin_atomic (notmuch_database_t *notmuch)
 	notmuch->exception_reported = TRUE;
 	return NOTMUCH_STATUS_XAPIAN_EXCEPTION;
     }
+
+DONE:
+    notmuch->atomic_nesting++;
     return NOTMUCH_STATUS_SUCCESS;
 }
 
@@ -996,8 +1003,12 @@ notmuch_database_end_atomic (notmuch_database_t *notmuch)
 {
     Xapian::WritableDatabase *db;
 
-    if (notmuch->mode == NOTMUCH_DATABASE_MODE_READ_ONLY)
-	return NOTMUCH_STATUS_SUCCESS;
+    if (notmuch->atomic_nesting == 0)
+	return NOTMUCH_STATUS_UNBALANCED_ATOMIC;
+
+    if (notmuch->mode == NOTMUCH_DATABASE_MODE_READ_ONLY ||
+	notmuch->atomic_nesting > 1)
+	goto DONE;
 
     db = static_cast <Xapian::WritableDatabase *> (notmuch->xapian_db);
     try {
@@ -1015,6 +1026,9 @@ notmuch_database_end_atomic (notmuch_database_t *notmuch)
 	notmuch->exception_reported = TRUE;
 	return NOTMUCH_STATUS_XAPIAN_EXCEPTION;
     }
+
+DONE:
+    notmuch->atomic_nesting--;
     return NOTMUCH_STATUS_SUCCESS;
 }
 
diff --git a/lib/notmuch.h b/lib/notmuch.h
index 208d22c..830aa41 100644
--- a/lib/notmuch.h
+++ b/lib/notmuch.h
@@ -81,6 +81,9 @@ typedef int notmuch_bool_t;
  * NOTMUCH_STATUS_UNBALANCED_FREEZE_THAW: The notmuch_message_thaw
  *	function has been called more times than notmuch_message_freeze.
  *
+ * NOTMUCH_STATUS_UNBALANCED_ATOMIC: notmuch_database_end_atomic has
+ *	been called more times than notmuch_database_begin_atomic.
+ *
  * And finally:
  *
  * NOTMUCH_STATUS_LAST_STATUS: Not an actual status value. Just a way
@@ -97,6 +100,7 @@ typedef enum _notmuch_status {
     NOTMUCH_STATUS_NULL_POINTER,
     NOTMUCH_STATUS_TAG_TOO_LONG,
     NOTMUCH_STATUS_UNBALANCED_FREEZE_THAW,
+    NOTMUCH_STATUS_UNBALANCED_ATOMIC,
 
     NOTMUCH_STATUS_LAST_STATUS
 } notmuch_status_t;
@@ -222,6 +226,9 @@ notmuch_database_upgrade (notmuch_database_t *database,
  * only ensures atomicity, not durability; neither begin nor end
  * necessarily flush modifications to disk.
  *
+ * Atomic sections may be nested.  begin_atomic and end_atomic must
+ * always be called in pairs.
+ *
  * Return value:
  *
  * NOTMUCH_STATUS_SUCCESS: Successfully entered atomic section.
@@ -240,6 +247,9 @@ notmuch_database_begin_atomic (notmuch_database_t *notmuch);
  *
  * NOTMUCH_STATUS_XAPIAN_EXCEPTION: A Xapian exception occurred;
  *	atomic section not ended.
+ *
+ * NOTMUCH_STATUS_UNBALANCED_ATOMIC: The database is not currently in
+ *	an atomic section.
  */
 notmuch_status_t
 notmuch_database_end_atomic (notmuch_database_t *notmuch);
diff --git a/notmuch-new.c b/notmuch-new.c
index b76b608..d1bea55 100644
--- a/notmuch-new.c
+++ b/notmuch-new.c
@@ -466,6 +466,7 @@ add_files_recursive (notmuch_database_t *notmuch,
 	case NOTMUCH_STATUS_NULL_POINTER:
 	case NOTMUCH_STATUS_TAG_TOO_LONG:
 	case NOTMUCH_STATUS_UNBALANCED_FREEZE_THAW:
+	case NOTMUCH_STATUS_UNBALANCED_ATOMIC:
 	case NOTMUCH_STATUS_LAST_STATUS:
 	    INTERNAL_ERROR ("add_message returned unexpected value: %d",  status);
 	    goto DONE;
-- 
1.7.5.1

[PATCH 09/17] lib: Indicate if there are more filenames after removal.

[Austin Clements]

Make _notmuch_message_remove_filename return
NOTMUCH_STATUS_DUPLICATE_MESSAGE_ID if the message has more filenames
and fix callers to handle this.
---
 lib/message.cc |    9 ++++++++-
 1 files changed, 8 insertions(+), 1 deletions(-)

diff --git a/lib/message.cc b/lib/message.cc
index afb1d80..5120b3a 100644
--- a/lib/message.cc
+++ b/lib/message.cc
@@ -501,6 +501,9 @@ _notmuch_message_add_filename (notmuch_message_t *message,
  * This change will not be reflected in the database until the next
  * call to _notmuch_message_sync.
  *
+ * If this message still has other filenames, returns
+ * NOTMUCH_STATUS_DUPLICATE_MESSAGE_ID.
+ *
  * Note: This function does not remove a document from the database,
  * even if the specified filename is the only filename for this
  * message. For that functionality, see
@@ -565,6 +568,9 @@ _notmuch_message_remove_filename (notmuch_message_t *message,
 	if (strncmp ((*i).c_str (), direntry_prefix, direntry_prefix_len))
 	    break;
 
+	/* Indicate that there are filenames remaining. */
+	status = NOTMUCH_STATUS_DUPLICATE_MESSAGE_ID;
+
 	direntry = (*i).c_str ();
 	direntry += direntry_prefix_len;
 
@@ -1257,7 +1263,8 @@ notmuch_message_tags_to_maildir_flags (notmuch_message_t *message)
 	    new_status = _notmuch_message_remove_filename (message,
 							   filename);
 	    /* Hold on to only the first error. */
-	    if (! status && new_status) {
+	    if (! status && new_status
+		&& new_status != NOTMUCH_STATUS_DUPLICATE_MESSAGE_ID) {
 		status = new_status;
 		continue;
 	    }
-- 
1.7.5.1

[PATCH 10/17] lib: Remove message document directly after removing the last file name.

[Austin Clements]

Previously, notmuch_database_remove_message would remove the message
file name, sync the change to the message document, re-find the
message document, and then delete it if there were no more file names.
An interruption after sync'ing would result in a file-name-less,
permanently un-removable zombie message that would produce errors and
odd results in searches.  We could wrap this in an atomic section, but
it's much simpler to eliminate the round-about approach and just
delete the message document instead of sync'ing it if we removed the
last filename.
---
 lib/database.cc       |   25 +++++--------------------
 lib/message.cc        |   16 ++++++++++++++++
 lib/notmuch-private.h |    3 +++
 3 files changed, 24 insertions(+), 20 deletions(-)

diff --git a/lib/database.cc b/lib/database.cc
index b766e94..9886622 100644
--- a/lib/database.cc
+++ b/lib/database.cc
@@ -1747,7 +1747,6 @@ notmuch_status_t
 notmuch_database_remove_message (notmuch_database_t *notmuch,
 				 const char *filename)
 {
-    Xapian::WritableDatabase *db;
     void *local;
     const char *prefix = _find_prefix ("file-direntry");
     char *direntry, *term;
@@ -1761,8 +1760,6 @@ notmuch_database_remove_message (notmuch_database_t *notmuch,
 
     local = talloc_new (notmuch);
 
-    db = static_cast <Xapian::WritableDatabase *> (notmuch->xapian_db);
-
     try {
 
 	status = _notmuch_database_filename_to_direntry (local, notmuch,
@@ -1785,23 +1782,11 @@ notmuch_database_remove_message (notmuch_database_t *notmuch,
 		return COERCE_STATUS (private_status,
 				      "Inconsistent document ID in datbase.");
 
-	    _notmuch_message_remove_filename (message, filename);
-	    _notmuch_message_sync (message);
-
-	    /* Take care to find document after sync'ing filename removal. */
-	    document = find_document_for_doc_id (notmuch, *i);
-	    j = document.termlist_begin ();
-	    j.skip_to (prefix);
-
-	    /* Was this the last file-direntry in the message? */
-	    if (j == document.termlist_end () ||
-		strncmp ((*j).c_str (), prefix, strlen (prefix)))
-	    {
-		db->delete_document (document.get_docid ());
-		status = NOTMUCH_STATUS_SUCCESS;
-	    } else {
-		status = NOTMUCH_STATUS_DUPLICATE_MESSAGE_ID;
-	    }
+	    status = _notmuch_message_remove_filename (message, filename);
+	    if (status == NOTMUCH_STATUS_SUCCESS)
+		_notmuch_message_delete (message);
+	    else if (status == NOTMUCH_STATUS_DUPLICATE_MESSAGE_ID)
+		_notmuch_message_sync (message);
 	}
     } catch (const Xapian::Error &error) {
 	fprintf (stderr, "Error: A Xapian exception occurred removing message: %s\n",
diff --git a/lib/message.cc b/lib/message.cc
index 5120b3a..9de60d2 100644
--- a/lib/message.cc
+++ b/lib/message.cc
@@ -797,6 +797,22 @@ _notmuch_message_sync (notmuch_message_t *message)
     db->replace_document (message->doc_id, message->doc);
 }
 
+/* Delete a message document from the database. */
+notmuch_status_t
+_notmuch_message_delete (notmuch_message_t *message)
+{
+    notmuch_status_t status;
+    Xapian::WritableDatabase *db;
+
+    status = _notmuch_database_ensure_writable (message->notmuch);
+    if (status)
+	return status;
+
+    db = static_cast <Xapian::WritableDatabase *> (message->notmuch->xapian_db);
+    db->delete_document (message->doc_id);
+    return NOTMUCH_STATUS_SUCCESS;
+}
+
 /* Ensure that 'message' is not holding any file object open. Future
  * calls to various functions will still automatically open the
  * message file as needed.
diff --git a/lib/notmuch-private.h b/lib/notmuch-private.h
index 02e24ee..d319530 100644
--- a/lib/notmuch-private.h
+++ b/lib/notmuch-private.h
@@ -293,6 +293,9 @@ _notmuch_message_set_date (notmuch_message_t *message,
 void
 _notmuch_message_sync (notmuch_message_t *message);
 
+notmuch_status_t
+_notmuch_message_delete (notmuch_message_t *message);
+
 void
 _notmuch_message_close (notmuch_message_t *message);
 
-- 
1.7.5.1

[PATCH 11/17] lib: Add an API to find a message by filename.

[Austin Clements]

notmuch_database_find_message_by_filename is mostly stolen from
notmuch_database_remove_message, so this patch also vastly simplfies
the latter using the former.

This API is also useful in its own right and will be used in a later
patch for eager maildir flag synchronization.
---
 lib/database.cc |   49 ++++++++++++++++++++++++++-----------------------
 lib/notmuch.h   |   16 ++++++++++++++++
 2 files changed, 42 insertions(+), 23 deletions(-)

diff --git a/lib/database.cc b/lib/database.cc
index 9886622..daa619d 100644
--- a/lib/database.cc
+++ b/lib/database.cc
@@ -1747,57 +1747,60 @@ 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;
+
+    if (message) {
+	    status = _notmuch_message_remove_filename (message, filename);
+	    if (status == NOTMUCH_STATUS_SUCCESS)
+		_notmuch_message_delete (message);
+	    else if (status == NOTMUCH_STATUS_DUPLICATE_MESSAGE_ID)
+		_notmuch_message_sync (message);
+    }
+
+    return status;
+}
+
+notmuch_message_t *
+notmuch_database_find_message_by_filename (notmuch_database_t *notmuch,
+					   const char *filename)
+{
     void *local;
     const char *prefix = _find_prefix ("file-direntry");
     char *direntry, *term;
     Xapian::PostingIterator i, end;
-    Xapian::Document document;
+    notmuch_message_t *message = NULL;
     notmuch_status_t status;
 
-    status = _notmuch_database_ensure_writable (notmuch);
-    if (status)
-	return status;
-
     local = talloc_new (notmuch);
 
     try {
-
 	status = _notmuch_database_filename_to_direntry (local, notmuch,
 							 filename, &direntry);
 	if (status)
-	    return status;
+	    return NULL;
 
 	term = talloc_asprintf (local, "%s%s", prefix, direntry);
 
 	find_doc_ids_for_term (notmuch, term, &i, &end);
 
-	for ( ; i != end; i++) {
-	    Xapian::TermIterator j;
-	    notmuch_message_t *message;
+	if (i != end) {
 	    notmuch_private_status_t private_status;
 
-	    message = _notmuch_message_create (local, notmuch,
+	    message = _notmuch_message_create (notmuch, notmuch,
 					       *i, &private_status);
-	    if (message == NULL)
-		return COERCE_STATUS (private_status,
-				      "Inconsistent document ID in datbase.");
-
-	    status = _notmuch_message_remove_filename (message, filename);
-	    if (status == NOTMUCH_STATUS_SUCCESS)
-		_notmuch_message_delete (message);
-	    else if (status == NOTMUCH_STATUS_DUPLICATE_MESSAGE_ID)
-		_notmuch_message_sync (message);
 	}
     } catch (const Xapian::Error &error) {
-	fprintf (stderr, "Error: A Xapian exception occurred removing message: %s\n",
+	fprintf (stderr, "Error: A Xapian exception occurred finding message by filename: %s\n",
 		 error.get_msg().c_str());
 	notmuch->exception_reported = TRUE;
-	status = NOTMUCH_STATUS_XAPIAN_EXCEPTION;
+	message = NULL;
     }
 
     talloc_free (local);
 
-    return status;
+    return message;
 }
 
 notmuch_string_list_t *
diff --git a/lib/notmuch.h b/lib/notmuch.h
index 830aa41..9ae260e 100644
--- a/lib/notmuch.h
+++ b/lib/notmuch.h
@@ -356,6 +356,22 @@ notmuch_message_t *
 notmuch_database_find_message (notmuch_database_t *database,
 			       const char *message_id);
 
+/* 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.
+ *
+ * This function returns NULL in the following situations:
+ *
+ *	* No message is found with the given filename
+ *	* An out-of-memory situation occurs
+ *	* A Xapian exception occurs
+ */
+notmuch_message_t *
+notmuch_database_find_message_by_filename (notmuch_database_t *notmuch,
+					   const char *filename);
+
 /* Return a list of all tags found in the database.
  *
  * This function creates a list of all tags found in the database. The
-- 
1.7.5.1

[PATCH 12/17] lib: Wrap notmuch_database_add_message in an atomic section.

[Austin Clements]

Adding a message may involve changes to multiple database documents,
and thus needs to be done in a transaction.  This makes add_message
(and, I think, the whole library) atomicity-safe: library callers only
needs to use atomic sections if they needs atomicity across multiple
library calls.
---
 lib/database.cc |   14 +++++++++++++-
 1 files changed, 13 insertions(+), 1 deletions(-)

diff --git a/lib/database.cc b/lib/database.cc
index daa619d..f963964 100644
--- a/lib/database.cc
+++ b/lib/database.cc
@@ -1601,7 +1601,7 @@ notmuch_database_add_message (notmuch_database_t *notmuch,
 {
     notmuch_message_file_t *message_file;
     notmuch_message_t *message = NULL;
-    notmuch_status_t ret = NOTMUCH_STATUS_SUCCESS;
+    notmuch_status_t ret = NOTMUCH_STATUS_SUCCESS, ret2;
     notmuch_private_status_t private_status;
 
     const char *date, *header;
@@ -1619,6 +1619,12 @@ notmuch_database_add_message (notmuch_database_t *notmuch,
     if (message_file == NULL)
 	return NOTMUCH_STATUS_FILE_ERROR;
 
+    /* Adding a message may change many documents.  Do this all
+     * atomically. */
+    ret = notmuch_database_begin_atomic (notmuch);
+    if (ret)
+	goto DONE;
+
     notmuch_message_file_restrict_headers (message_file,
 					   "date",
 					   "from",
@@ -1740,6 +1746,12 @@ notmuch_database_add_message (notmuch_database_t *notmuch,
     if (message_file)
 	notmuch_message_file_close (message_file);
 
+    ret2 = notmuch_database_end_atomic (notmuch);
+    if ((ret == NOTMUCH_STATUS_SUCCESS ||
+	 ret == NOTMUCH_STATUS_DUPLICATE_MESSAGE_ID) &&
+	ret2 != NOTMUCH_STATUS_SUCCESS)
+	ret = ret2;
+
     return ret;
 }
 
-- 
1.7.5.1

[PATCH 13/17] new: Cleanup. Put removed/renamed message count in add_files_state_t.

[Austin Clements]

Previously, pointers to these variables were passed around
individually.  This was okay when only one function needed them, but
we're about to need them in a few more places.
---
 notmuch-new.c |   36 ++++++++++++++++--------------------
 1 files changed, 16 insertions(+), 20 deletions(-)

diff --git a/notmuch-new.c b/notmuch-new.c
index d1bea55..cdc8a1c 100644
--- a/notmuch-new.c
+++ b/notmuch-new.c
@@ -42,7 +42,7 @@ typedef struct {
 
     int total_files;
     int processed_files;
-    int added_messages;
+    int added_messages, removed_messages, renamed_messages;
     struct timeval tv_start;
 
     _filename_list_t *removed_files;
@@ -702,8 +702,7 @@ static void
 _remove_directory (void *ctx,
 		   notmuch_database_t *notmuch,
 		   const char *path,
-		   int *renamed_files,
-		   int *removed_files)
+		   add_files_state_t *add_files_state)
 {
     notmuch_directory_t *directory;
     notmuch_filenames_t *files, *subdirs;
@@ -720,9 +719,9 @@ _remove_directory (void *ctx,
 				    notmuch_filenames_get (files));
 	status = notmuch_database_remove_message (notmuch, absolute);
 	if (status == NOTMUCH_STATUS_DUPLICATE_MESSAGE_ID)
-	    *renamed_files = *renamed_files + 1;
+	    add_files_state->renamed_messages++;
 	else
-	    *removed_files = *removed_files + 1;
+	    add_files_state->removed_messages++;
 	talloc_free (absolute);
     }
 
@@ -732,7 +731,7 @@ _remove_directory (void *ctx,
     {
 	absolute = talloc_asprintf (ctx, "%s/%s", path,
 				    notmuch_filenames_get (subdirs));
-	_remove_directory (ctx, notmuch, absolute, renamed_files, removed_files);
+	_remove_directory (ctx, notmuch, absolute, add_files_state);
 	talloc_free (absolute);
     }
 
@@ -753,7 +752,6 @@ notmuch_new_command (void *ctx, int argc, char *argv[])
     char *dot_notmuch_path;
     struct sigaction action;
     _filename_node_t *f;
-    int renamed_files, removed_files;
     notmuch_status_t status;
     int i;
     notmuch_bool_t timer_is_active = FALSE;
@@ -826,6 +824,7 @@ notmuch_new_command (void *ctx, int argc, char *argv[])
 
     add_files_state.processed_files = 0;
     add_files_state.added_messages = 0;
+    add_files_state.removed_messages = add_files_state.renamed_messages = 0;
     gettimeofday (&add_files_state.tv_start, NULL);
 
     add_files_state.removed_files = _filename_list_create (ctx);
@@ -840,27 +839,24 @@ notmuch_new_command (void *ctx, int argc, char *argv[])
 
     ret = add_files (notmuch, db_path, &add_files_state);
 
-    removed_files = 0;
-    renamed_files = 0;
     gettimeofday (&tv_start, NULL);
     for (f = add_files_state.removed_files->head; f && !interrupted; f = f->next) {
 	status = notmuch_database_remove_message (notmuch, f->filename);
 	if (status == NOTMUCH_STATUS_DUPLICATE_MESSAGE_ID)
-	    renamed_files++;
+	    add_files_state.renamed_messages++;
 	else
-	    removed_files++;
+	    add_files_state.removed_messages++;
 	if (do_print_progress) {
 	    do_print_progress = 0;
 	    generic_print_progress ("Cleaned up", "messages",
-		tv_start, removed_files + renamed_files,
+		tv_start, add_files_state.removed_messages + add_files_state.renamed_messages,
 		add_files_state.removed_files->count);
 	}
     }
 
     gettimeofday (&tv_start, NULL);
     for (f = add_files_state.removed_directories->head, i = 0; f && !interrupted; f = f->next, i++) {
-	_remove_directory (ctx, notmuch, f->filename,
-			   &renamed_files, &removed_files);
+	_remove_directory (ctx, notmuch, f->filename, &add_files_state);
 	if (do_print_progress) {
 	    do_print_progress = 0;
 	    generic_print_progress ("Cleaned up", "directories",
@@ -937,16 +933,16 @@ notmuch_new_command (void *ctx, int argc, char *argv[])
 	printf ("No new mail.");
     }
 
-    if (removed_files) {
+    if (add_files_state.removed_messages) {
 	printf (" Removed %d %s.",
-		removed_files,
-		removed_files == 1 ? "message" : "messages");
+		add_files_state.removed_messages,
+		add_files_state.removed_messages == 1 ? "message" : "messages");
     }
 
-    if (renamed_files) {
+    if (add_files_state.renamed_messages) {
 	printf (" Detected %d file %s.",
-		renamed_files,
-		renamed_files == 1 ? "rename" : "renames");
+		add_files_state.renamed_messages,
+		add_files_state.renamed_messages == 1 ? "rename" : "renames");
     }
 
     printf ("\n");
-- 
1.7.5.1

[PATCH 14/17] new: Cleanup. De-duplicate file name removal code.

[Austin Clements]

Previously, file name removal was implemented identically in two
places.  Now it's captured in one function.

This is important because file name removal is about to get slightly
more complicated with eager tag synchronization and correct removal
atomicity.
---
 notmuch-new.c |   29 +++++++++++++++++------------
 1 files changed, 17 insertions(+), 12 deletions(-)

diff --git a/notmuch-new.c b/notmuch-new.c
index cdc8a1c..dc349f6 100644
--- a/notmuch-new.c
+++ b/notmuch-new.c
@@ -696,6 +696,21 @@ upgrade_print_progress (void *closure,
     fflush (stdout);
 }
 
+/* Remove one message filename from the database. */
+static notmuch_status_t
+remove_filename (notmuch_database_t *notmuch,
+		 const char *path,
+		 add_files_state_t *add_files_state)
+{
+    notmuch_status_t status;
+    status = notmuch_database_remove_message (notmuch, path);
+    if (status == NOTMUCH_STATUS_DUPLICATE_MESSAGE_ID)
+	add_files_state->renamed_messages++;
+    else
+	add_files_state->removed_messages++;
+    return status;
+}
+
 /* Recursively remove all filenames from the database referring to
  * 'path' (or to any of its children). */
 static void
@@ -706,7 +721,6 @@ _remove_directory (void *ctx,
 {
     notmuch_directory_t *directory;
     notmuch_filenames_t *files, *subdirs;
-    notmuch_status_t status;
     char *absolute;
 
     directory = notmuch_database_get_directory (notmuch, path);
@@ -717,11 +731,7 @@ _remove_directory (void *ctx,
     {
 	absolute = talloc_asprintf (ctx, "%s/%s", path,
 				    notmuch_filenames_get (files));
-	status = notmuch_database_remove_message (notmuch, absolute);
-	if (status == NOTMUCH_STATUS_DUPLICATE_MESSAGE_ID)
-	    add_files_state->renamed_messages++;
-	else
-	    add_files_state->removed_messages++;
+	remove_filename (notmuch, absolute, add_files_state);
 	talloc_free (absolute);
     }
 
@@ -752,7 +762,6 @@ notmuch_new_command (void *ctx, int argc, char *argv[])
     char *dot_notmuch_path;
     struct sigaction action;
     _filename_node_t *f;
-    notmuch_status_t status;
     int i;
     notmuch_bool_t timer_is_active = FALSE;
 
@@ -841,11 +850,7 @@ notmuch_new_command (void *ctx, int argc, char *argv[])
 
     gettimeofday (&tv_start, NULL);
     for (f = add_files_state.removed_files->head; f && !interrupted; f = f->next) {
-	status = notmuch_database_remove_message (notmuch, f->filename);
-	if (status == NOTMUCH_STATUS_DUPLICATE_MESSAGE_ID)
-	    add_files_state.renamed_messages++;
-	else
-	    add_files_state.removed_messages++;
+	remove_filename (notmuch, f->filename, &add_files_state);
 	if (do_print_progress) {
 	    do_print_progress = 0;
 	    generic_print_progress ("Cleaned up", "messages",
-- 
1.7.5.1

[PATCH 15/17] new: Synchronize maildir flags eagerly.

[Austin Clements]

Because flag synchronization is stateless, it can be performed at any
time as long as it's guaranteed to be performed after any change to a
message's filename list.  Take advantage of this to synchronize tags
immediately after a filename is added or removed.

This does not yet make adding or removing a message atomic, but it is
a big step toward atomicity because it reduces the window where the
database tags are inconsistent from nearly the entire notmuch-new to
just around when the message is added or removed.
---
 notmuch-new.c |   42 ++++++++----------------------------------
 1 files changed, 8 insertions(+), 34 deletions(-)

diff --git a/notmuch-new.c b/notmuch-new.c
index dc349f6..b24eb51 100644
--- a/notmuch-new.c
+++ b/notmuch-new.c
@@ -50,7 +50,6 @@ typedef struct {
     _filename_list_t *directory_mtimes;
 
     notmuch_bool_t synchronize_flags;
-    _filename_list_t *message_ids_to_sync;
 } add_files_state_t;
 
 static volatile sig_atomic_t do_print_progress = 0;
@@ -443,11 +442,8 @@ add_files_recursive (notmuch_database_t *notmuch,
 	    break;
 	/* Non-fatal issues (go on to next file) */
 	case NOTMUCH_STATUS_DUPLICATE_MESSAGE_ID:
-	    /* Defer sync of maildir flags until after old filenames
-	     * are removed in the case of a rename. */
 	    if (state->synchronize_flags == TRUE)
-		_filename_list_add (state->message_ids_to_sync,
-				    notmuch_message_get_message_id (message));
+		notmuch_message_maildir_flags_to_tags (message);
 	    break;
 	case NOTMUCH_STATUS_FILE_NOT_EMAIL:
 	    fprintf (stderr, "Note: Ignoring non-mail file: %s\n",
@@ -703,11 +699,16 @@ remove_filename (notmuch_database_t *notmuch,
 		 add_files_state_t *add_files_state)
 {
     notmuch_status_t status;
+    notmuch_message_t *message;
+    message = notmuch_database_find_message_by_filename (notmuch, path);
     status = notmuch_database_remove_message (notmuch, path);
-    if (status == NOTMUCH_STATUS_DUPLICATE_MESSAGE_ID)
+    if (status == NOTMUCH_STATUS_DUPLICATE_MESSAGE_ID) {
 	add_files_state->renamed_messages++;
-    else
+	if (add_files_state->synchronize_flags == TRUE)
+	    notmuch_message_maildir_flags_to_tags (message);
+    } else
 	add_files_state->removed_messages++;
+    notmuch_message_destroy (message);
     return status;
 }
 
@@ -782,7 +783,6 @@ notmuch_new_command (void *ctx, int argc, char *argv[])
 
     add_files_state.new_tags = notmuch_config_get_new_tags (config, &add_files_state.new_tags_length);
     add_files_state.synchronize_flags = notmuch_config_get_maildir_synchronize_flags (config);
-    add_files_state.message_ids_to_sync = _filename_list_create (ctx);
     db_path = notmuch_config_get_database_path (config);
 
     dot_notmuch_path = talloc_asprintf (ctx, "%s/%s", db_path, ".notmuch");
@@ -883,32 +883,6 @@ notmuch_new_command (void *ctx, int argc, char *argv[])
     talloc_free (add_files_state.removed_directories);
     talloc_free (add_files_state.directory_mtimes);
 
-    /* Now that removals are done (hence the database is aware of all
-     * renames), we can synchronize maildir_flags to tags for all
-     * messages that had new filenames appear on this run. */
-    gettimeofday (&tv_start, NULL);
-    if (add_files_state.synchronize_flags) {
-	_filename_node_t *node;
-	notmuch_message_t *message;
-	for (node = add_files_state.message_ids_to_sync->head, i = 0;
-	     node;
-	     node = node->next, i++)
-	{
-	    message = notmuch_database_find_message (notmuch, node->filename);
-	    notmuch_message_maildir_flags_to_tags (message);
-	    notmuch_message_destroy (message);
-	    if (do_print_progress) {
-		do_print_progress = 0;
-		generic_print_progress (
-		    "Synchronized tags for", "messages",
-		    tv_start, i, add_files_state.message_ids_to_sync->count);
-	    }
-	}
-    }
-
-    talloc_free (add_files_state.message_ids_to_sync);
-    add_files_state.message_ids_to_sync = NULL;
-
     if (timer_is_active)
 	stop_progress_printing_timer ();
 
-- 
1.7.5.1

[PATCH 16/17] new: Wrap adding and removing messages in atomic sections.

[Austin Clements]

This addresses atomicity of tag synchronization, the last atomicity
problems in notmuch new.  Each message add or remove is wrapped in its
own atomic section, so interrupting notmuch new doesn't lose progress.
---
 notmuch-new.c  |   16 ++++++++++++++++
 test/atomicity |    2 +-
 2 files changed, 17 insertions(+), 1 deletions(-)

diff --git a/notmuch-new.c b/notmuch-new.c
index b24eb51..fb92b03 100644
--- a/notmuch-new.c
+++ b/notmuch-new.c
@@ -428,6 +428,12 @@ add_files_recursive (notmuch_database_t *notmuch,
 	    fflush (stdout);
 	}
 
+	status = notmuch_database_begin_atomic (notmuch);
+	if (status) {
+	    ret = status;
+	    goto DONE;
+	}
+
 	status = notmuch_database_add_message (notmuch, next, &message);
 	switch (status) {
 	/* success */
@@ -468,6 +474,12 @@ add_files_recursive (notmuch_database_t *notmuch,
 	    goto DONE;
 	}
 
+	status = notmuch_database_end_atomic (notmuch);
+	if (status) {
+	    ret = status;
+	    goto DONE;
+	}
+
 	if (message) {
 	    notmuch_message_destroy (message);
 	    message = NULL;
@@ -700,6 +712,9 @@ remove_filename (notmuch_database_t *notmuch,
 {
     notmuch_status_t status;
     notmuch_message_t *message;
+    status = notmuch_database_begin_atomic (notmuch);
+    if (status)
+	return status;
     message = notmuch_database_find_message_by_filename (notmuch, path);
     status = notmuch_database_remove_message (notmuch, path);
     if (status == NOTMUCH_STATUS_DUPLICATE_MESSAGE_ID) {
@@ -709,6 +724,7 @@ remove_filename (notmuch_database_t *notmuch,
     } else
 	add_files_state->removed_messages++;
     notmuch_message_destroy (message);
+    notmuch_database_end_atomic (notmuch);
     return status;
 }
 
diff --git a/test/atomicity b/test/atomicity
index 817819a..f546c07 100755
--- a/test/atomicity
+++ b/test/atomicity
@@ -96,7 +96,7 @@ for ((i = 0; i < $outcount; i++)); do
         i=$(expr $end - 1)
     fi
 done
-test_expect_equal_failure GDB "$(cat searchall)" "$(cat expectall)"
+test_expect_equal GDB "$(cat searchall)" "$(cat expectall)"
 
 test_expect_success GDB "detected $outcount>10 abort points" "test $outcount -gt 10"
 
-- 
1.7.5.1

[PATCH 17/17] lib: Improve notmuch_database_{add, remove}_message documentation.

[Austin Clements]

State up front that these functions may add a filename to an existing
message or remove only a filename (and not the message), respectively.
Previously, this key information was buried in return value
documentation or in "notes", which made it seem secondary to these
functions' semantics.
---
 lib/notmuch.h |   25 +++++++++++++++----------
 1 files changed, 15 insertions(+), 10 deletions(-)

diff --git a/lib/notmuch.h b/lib/notmuch.h
index 9ae260e..ad62c76 100644
--- a/lib/notmuch.h
+++ b/lib/notmuch.h
@@ -266,9 +266,10 @@ notmuch_directory_t *
 notmuch_database_get_directory (notmuch_database_t *database,
 				const char *path);
 
-/* Add a new message to the given notmuch database.
+/* Add a new message to the given notmuch database or associate an
+ * additional filename with an existing message.
  *
- * Here,'filename' should be a path relative to the path of
+ * Here, 'filename' should be a path relative to the path of
  * 'database' (see notmuch_database_get_path), or else should be an
  * absolute filename with initial components that match the path of
  * 'database'.
@@ -278,6 +279,10 @@ notmuch_database_get_directory (notmuch_database_t *database,
  * notmuch database will reference the filename, and will not copy the
  * entire contents of the file.
  *
+ * If another message with the same message ID already exists in the
+ * database, rather than creating a new message, this adds 'filename'
+ * to the list of the filenames for the existing message.
+ *
  * 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
@@ -295,7 +300,7 @@ notmuch_database_get_directory (notmuch_database_t *database,
  * NOTMUCH_STATUS_DUPLICATE_MESSAGE_ID: Message has the same message
  *	ID as another message already in the database. The new
  *	filename was successfully added to the message in the database
- *	(if not already present).
+ *	(if not already present) and the existing message is returned.
  *
  * NOTMUCH_STATUS_FILE_ERROR: an error occurred trying to open the
  *	file, (such as permission denied, or file not found,
@@ -312,14 +317,14 @@ notmuch_database_add_message (notmuch_database_t *database,
 			      const char *filename,
 			      notmuch_message_t **message);
 
-/* Remove a message from the given notmuch database.
+/* Remove a message filename from the given notmuch database. If the
+ * message has no more filenames, remove the message.
  *
- * Note that only this particular filename association is removed from
- * the database. If the same message (as determined by the message ID)
- * is still available via other filenames, then the message will
- * persist in the database for those filenames. When the last filename
- * is removed for a particular message, the database content for that
- * message will be entirely removed.
+ * If the same message (as determined by the message ID) is still
+ * available via other filenames, then the message will persist in the
+ * database for those filenames. When the last filename is removed for
+ * a particular message, the database content for that message will be
+ * entirely removed.
  *
  * Return value:
  *
-- 
1.7.5.1

[PATCH 01/17] test: Fix message when skipping test_expect_equal* tests

[Austin Clements]

For these types of tests, the test name is previously recorded in a
variable, not passed to the test function, so pass this variable to
test_skip.
---
 test/test-lib.sh |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/test/test-lib.sh b/test/test-lib.sh
index a59d1c1..d0e2794 100755
--- a/test/test-lib.sh
+++ b/test/test-lib.sh
@@ -448,7 +448,7 @@ test_expect_equal ()
 
 	output="$1"
 	expected="$2"
-	if ! test_skip "$@"
+	if ! test_skip "$test_subtest_name"
 	then
 		if [ "$output" = "$expected" ]; then
 			test_ok_ "$test_subtest_name"
@@ -471,7 +471,7 @@ test_expect_equal_file ()
 
 	output="$1"
 	expected="$2"
-	if ! test_skip "$@"
+	if ! test_skip "$test_subtest_name"
 	then
 		if diff -q "$expected" "$output" >/dev/null ; then
 			test_ok_ "$test_subtest_name"
@@ -494,7 +494,7 @@ test_expect_equal_failure ()
 
 	output="$1"
 	expected="$2"
-	if ! test_skip "$@"
+	if ! test_skip "$test_subtest_name"
 	then
 		if [ "$output" = "$expected" ]; then
 			test_known_broken_ok_ "$test_subtest_name"
-- 
1.7.5.1

Re: [PATCH v6 00/17] Fix 'notmuch new' atomicity issues

[Austin Clements]

On Sat, Jun 11, 2011 at 4:04 PM, Austin Clements <amdragon@mit.edu> wrote:
> Here's the reworked patch series that uses atomic sections more
> heavily rather than changing the removal API.  This is atomic-new-v6
> on http://awakening.csail.mit.edu/git/notmuch.git .

(Patch 01/17 is out of date order.  My mail queue hiccuped, so I had
to resend it.)

Re: [PATCH v6 00/17] Fix 'notmuch new' atomicity issues

[Austin Clements]

Ping.

On Sat, Jun 11, 2011 at 4:04 PM, Austin Clements <amdragon@mit.edu> wrote:
> Here's the reworked patch series that uses atomic sections more
> heavily rather than changing the removal API.  This is atomic-new-v6
> on http://awakening.csail.mit.edu/git/notmuch.git .
>
> (I was planning to make this series on Monday while stuck on a plane,
> but an opportunity presented itself when I needed something better to
> do than fix the sink.  As a result, if you can look over this before
> Monday, I can use that time to address any further comments.)
>
> The beginning of the sequence---"test: Fix message when skipping
> test_expect_equal* tests" through "new: Defer updating directory
> mtimes until the end."---has not changed besides than the minor things
> you pointed out earlier.  Here's a quick summary of the differences in
> the rest of the sequence:
>
> lib: Add notmuch_database_{begin,end}_atomic.
>  Rebased but otherwise identical.
>
> lib: Add support for nested atomic sections.
>  New.
>
> lib: Indicate if there are more filenames after removal.
>  Rebased but otherwise identical.
>
> lib: Remove message document directly after removing the last file
> name.
>  New.  Supersedes the patches that rewrote
>  notmuch_database_remove_message and made sync delete messages.
>
> lib: Add an API to find a message by filename.
>  Culled from "lib: Add API's to find by filename and ..." in the old
>  series.  What I kept is identical.
>
> lib: Wrap notmuch_database_add_message in an atomic section.
>  New.
>
> new: Cleanup. Put removed/renamed message count in add_files_state_t.
> new: Cleanup. De-duplicate file name removal code.
>  Both new, but closely related to previous remove_filename patch.
>
> new: Synchronize maildir flags eagerly.
>  New, but very closely related to previous patch of the same name.
>  This patch is simpler now because of the previous two cleanups.
>
> new: Wrap adding and removing messages in atomic sections.
>  Rebased and added an atomic section around removal.
>
> lib: Improve notmuch_database_{add,remove}_message documentation.
>  New.
>
> Ultimately, I still think the existing removal API is weird because it
> forces you to do things like lookup a message by file name just in
> case you need that message object after removal tells you that it
> didn't actually remove the message.  But I don't know what to do about
> it that isn't equally weird.
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch
>

Re: [PATCH v6 00/17] Fix 'notmuch new' atomicity issues

[Austin Clements]

Just found one more atomicity bug in notmuch-new.  I would prefer to
not update this patch series yet again, but rather to follow up with a
separate fix for this.  The full bug is described below, but the gist
is that how add_files_recursive computes new_directory from the mtime
isn't sound.  The simplest fix is to remove the new_directory
optimization, but then we wouldn't scan files in inode order.  The
best fix is probably to add an out-argument to
notmuch_database_get_directory that indicates if the directory really
was just created in the database (and hence has no files or subdirs).
Unfortunately, that requires an API change.  If that's undesirable, an
alternate would be to record that bit in the notmuch_directory_t and
add some notmuch_directory_is_new API that returns it.  Thoughts?


Bug description:

add_files_recursive determines whether a directory is "new" by
inspecting the database mtime returned by notmuch_directory_get_mtime.
 However, if there's an interruption after adding
messages/subdirectories from a new directory but before updating the
directory's mtime, it will be considered a new directory again on the
next run.  As a result, on the next run, db_files and db_subdirs will
not be loaded from the database (even though they *wouldn't* be NULL).
 As a result, we'll miss removing messages or entire subdirectories
that have been deleted from the "new" directory.  (We'll also re-add
the messages in this directory to the database rather than skipping
them, which will throw off the notmuch new statistics, but that's
fine.)

This didn't show up in the atomicity test because throwing off
anything besides the statistics requires *two* interruptions.  In
fact, I don't even know how I would write an automated test for this.

Re: [PATCH v6 00/17] Fix 'notmuch new' atomicity issues

[Austin Clements]

Rebased against current master and now atomic-new-v7 on
  http://awakening.csail.mit.edu/git/notmuch.git
There were a handful of conflicts, but nothing substantive.

On Wed, Jun 22, 2011 at 3:39 PM, Austin Clements <amdragon@mit.edu> wrote:
> Ping.
>
> On Sat, Jun 11, 2011 at 4:04 PM, Austin Clements <amdragon@mit.edu> wrote:
>> Here's the reworked patch series that uses atomic sections more
>> heavily rather than changing the removal API.  This is atomic-new-v6
>> on http://awakening.csail.mit.edu/git/notmuch.git .
>>
>> (I was planning to make this series on Monday while stuck on a plane,
>> but an opportunity presented itself when I needed something better to
>> do than fix the sink.  As a result, if you can look over this before
>> Monday, I can use that time to address any further comments.)
>>
>> The beginning of the sequence---"test: Fix message when skipping
>> test_expect_equal* tests" through "new: Defer updating directory
>> mtimes until the end."---has not changed besides than the minor things
>> you pointed out earlier.  Here's a quick summary of the differences in
>> the rest of the sequence:
>>
>> lib: Add notmuch_database_{begin,end}_atomic.
>>  Rebased but otherwise identical.
>>
>> lib: Add support for nested atomic sections.
>>  New.
>>
>> lib: Indicate if there are more filenames after removal.
>>  Rebased but otherwise identical.
>>
>> lib: Remove message document directly after removing the last file
>> name.
>>  New.  Supersedes the patches that rewrote
>>  notmuch_database_remove_message and made sync delete messages.
>>
>> lib: Add an API to find a message by filename.
>>  Culled from "lib: Add API's to find by filename and ..." in the old
>>  series.  What I kept is identical.
>>
>> lib: Wrap notmuch_database_add_message in an atomic section.
>>  New.
>>
>> new: Cleanup. Put removed/renamed message count in add_files_state_t.
>> new: Cleanup. De-duplicate file name removal code.
>>  Both new, but closely related to previous remove_filename patch.
>>
>> new: Synchronize maildir flags eagerly.
>>  New, but very closely related to previous patch of the same name.
>>  This patch is simpler now because of the previous two cleanups.
>>
>> new: Wrap adding and removing messages in atomic sections.
>>  Rebased and added an atomic section around removal.
>>
>> lib: Improve notmuch_database_{add,remove}_message documentation.
>>  New.
>>
>> Ultimately, I still think the existing removal API is weird because it
>> forces you to do things like lookup a message by file name just in
>> case you need that message object after removal tells you that it
>> didn't actually remove the message.  But I don't know what to do about
>> it that isn't equally weird.
>>
>> _______________________________________________
>> notmuch mailing list
>> notmuch@notmuchmail.org
>> http://notmuchmail.org/mailman/listinfo/notmuch
>>
>

Re: [PATCH v6 00/17] Fix 'notmuch new' atomicity issues

[David Bremner]

On Fri, 8 Jul 2011 14:09:16 -0400, Austin Clements <amdragon@mit.edu> wrote:
> Rebased against current master and now atomic-new-v7 on
>   http://awakening.csail.mit.edu/git/notmuch.git
> There were a handful of conflicts, but nothing substantive.
> 

I have rebased these yet again, and pushed the first 4 commits to
master. So far this is mainly testing stuff, along with one oneline
bugfix. I also added a couple testing related commits:

        73ed66a test: use test_expect_equal_file in atomicity
        05a522c test: Convert atomicity test to use test_subtest_known_broken

d

Re: [PATCH v6 00/17] Fix 'notmuch new' atomicity issues

[David Bremner]

On Tue, 13 Sep 2011 09:39:15 -0300, David Bremner <david@tethera.net> wrote:
> On Fri, 8 Jul 2011 14:09:16 -0400, Austin Clements <amdragon@mit.edu> wrote:
> 
> I have rebased these yet again, and pushed the first 4 commits to
> master. So far this is mainly testing stuff, along with one oneline
> bugfix. I also added a couple testing related commits:
> 
>         73ed66a test: use test_expect_equal_file in atomicity
>         05a522c test: Convert atomicity test to use test_subtest_known_broken

I have pushed 5 more commits in the sequence, without changes.  

d

Re: [PATCH v6 00/17] Fix 'notmuch new' atomicity issues

[Austin Clements]

Quoth David Bremner on Sep 23 at 11:14 pm:
> On Tue, 13 Sep 2011 09:39:15 -0300, David Bremner <david@tethera.net> wrote:
> > On Fri, 8 Jul 2011 14:09:16 -0400, Austin Clements <amdragon@mit.edu> wrote:
> > 
> > I have rebased these yet again, and pushed the first 4 commits to
> > master. So far this is mainly testing stuff, along with one oneline
> > bugfix. I also added a couple testing related commits:
> > 
> >         73ed66a test: use test_expect_equal_file in atomicity
> >         05a522c test: Convert atomicity test to use test_subtest_known_broken
> 
> I have pushed 5 more commits in the sequence, without changes.  

Awesome.  Only seven more to go!

Re: [PATCH v6 00/17] Fix 'notmuch new' atomicity issues

[David Bremner]

On Sat, 24 Sep 2011 00:03:02 -0400, Austin Clements <amdragon@MIT.EDU> wrote:
> 
> Awesome.  Only seven more to go!

The remaining seven are pushed, along with some related debian packaging
things.

Austin, could I bug you for some atomicity related items for NEWS? I
guess at least the 3 new library calls should be mentioned.

d

Re: [PATCH v6 00/17] Fix 'notmuch new' atomicity issues

[Austin Clements]

Quoth David Bremner on Sep 24 at 11:36 pm:
> On Sat, 24 Sep 2011 00:03:02 -0400, Austin Clements <amdragon@MIT.EDU> wrote:
> > 
> > Awesome.  Only seven more to go!
> 
> The remaining seven are pushed, along with some related debian packaging
> things.

Huzzah!

> Austin, could I bug you for some atomicity related items for NEWS? I
> guess at least the 3 new library calls should be mentioned.

In fact, I've been looking forward to writing some NEWS items!


Correct handling of interruptions during "notmuch new"

  "notmuch new" now operates as a series of small, self-consistent
  transactions, so it can correctly resume after an interruption or
  crash.  Previously, interruption could lose existing tags, fail to
  detect messages on resume, or leave the database in a state
  temporarily or permanently inconsistent with the mail store.

Library changes
---------------

New functions

  notmuch_database_begin_atomic and notmuch_database_end_atomic allow
  multiple database operations to be performed atomically.

  notmuch_database_find_message_by_filename does exactly what it says.

NEWS for 0.9

[David Bremner]

On Mon, 26 Sep 2011 18:07:01 -0400, Austin Clements <amdragon@MIT.EDU> wrote:
> 
> In fact, I've been looking forward to writing some NEWS items!
> 

Thanks, I have officially (and atomically) stolen your NEWS items. ;).

Sebastian, Ali:

I also threw in my best guess as to python and Ruby changes.  Changes
are welcome of course.

d

Re: NEWS for 0.9

[Ali Polatel]

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

On Mon, 26 Sep 2011 20:32:32 -0300, David Bremner <david@tethera.net> wrote:
> On Mon, 26 Sep 2011 18:07:01 -0400, Austin Clements <amdragon@MIT.EDU> wrote:
> > 
> > In fact, I've been looking forward to writing some NEWS items!
> > 
> 
> Thanks, I have officially (and atomically) stolen your NEWS items. ;).
> 
> Sebastian, Ali:
> 
> I also threw in my best guess as to python and Ruby changes.  Changes
> are welcome of course.

I'm fine with how the Ruby changes are described in the ChangeLog.
However I have other concerns regarding the recent changes which I will
mention in an upcoming e-mail.

> d

        -alip

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

Re: [PATCH v6 00/17] Fix 'notmuch new' atomicity issues

[Sebastian Spaeth]

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

On Sat, 11 Jun 2011 16:04:26 -0400, Austin Clements <amdragon@MIT.EDU> wrote:
> Here's the reworked patch series that uses atomic sections more
> heavily rather than changing the removal API.  This is atomic-new-v6
> on http://awakening.csail.mit.edu/git/notmuch.git .

I just caught up implementing find_message_by_filename and
begin|end_atomic

One oddity, since databases are opened in read-only by default, I was
surprise to see find_message_by_filename on such a database have my
python instance crash...

> lib: Add an API to find a message by filename.
>   Culled from "lib: Add API's to find by filename and ..." in the old
>   series.  What I kept is identical.

db.find_message_by_filename("moo")
Internal error: Failure to ensure database is writable
(lib/directory.cc:100).

Outch?
Sebastian

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

Re: [PATCH v6 00/17] Fix 'notmuch new' atomicity issues

[Austin Clements]

Quoth Sebastian Spaeth on Sep 28 at  6:36 pm:
> On Sat, 11 Jun 2011 16:04:26 -0400, Austin Clements <amdragon@MIT.EDU> wrote:
> > Here's the reworked patch series that uses atomic sections more
> > heavily rather than changing the removal API.  This is atomic-new-v6
> > on http://awakening.csail.mit.edu/git/notmuch.git .
> 
> I just caught up implementing find_message_by_filename and
> begin|end_atomic
> 
> One oddity, since databases are opened in read-only by default, I was
> surprise to see find_message_by_filename on such a database have my
> python instance crash...
> 
> > lib: Add an API to find a message by filename.
> >   Culled from "lib: Add API's to find by filename and ..." in the old
> >   series.  What I kept is identical.
> 
> db.find_message_by_filename("moo")
> Internal error: Failure to ensure database is writable
> (lib/directory.cc:100).
> 
> Outch?

Oof.

It appears that looking up a directory requires a writable database
because notmuch will try to *create* a database document for the
directory if one doesn't already exist.  This is clearly wrong
behavior for a "find" function.

The exact code path is
  notmuch_database_find_message_by_filename
  _notmuch_database_filename_to_direntry
  _notmuch_database_find_directory_id
  _notmuch_directory_create

_notmuch_message_add_filename currently depends on
_notmuch_database_filename_to_direntry to create the directory
document if it doesn't exist.  Possibly
_notmuch_database_filename_to_direntry,
_notmuch_database_find_directory_id, and _notmuch_directory_create
should acquire a flags argument with a "create" flag that controls
this behavior.

Re: [PATCH v6 00/17] Fix 'notmuch new' atomicity issues

[Sebastian Spaeth]

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

On Thu, 29 Sep 2011 11:01:47 -0400, Austin Clements <amdragon@MIT.EDU> wrote:
> Quoth Sebastian Spaeth on Sep 28 at  6:36 pm:
> > db.find_message_by_filename("moo")
> > Internal error: Failure to ensure database is writable
> > (lib/directory.cc:100).

> It appears that looking up a directory requires a writable database
> because notmuch will try to *create* a database document for the
> directory if one doesn't already exist.  This is clearly wrong
> behavior for a "find" function.

First of all, I consider libnotmuch exiting, taking python down, an
outright bug. So we should modify this case to return:
NOTMUCH_STATUS_READ_ONLY_DATABASE

so I can at least give some sensible error than having an angry mob run
with pitchforks towards me.

Ideally, we don't need READ-WRITE dbs for the find :-)

As it is, the same issue happens with the
notmuch_database_get_directory call, which should be protected in the
same manner (and probably has the same root cause). It is debatable
whether get_directory should be creating a directory on demand, I guess,
but it should never crash.

I have documented this in the python bindings, so it's not super urgent.
But it's dangerous and wrong nonetheless.

Sebastian

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