* [PATCH 02/17] test: Report test failures from test_expect_*
2011-06-11 20:04 ` [PATCH v6 00/17] " Austin Clements
@ 2011-06-11 20:04 ` Austin Clements
2011-06-11 20:04 ` [PATCH 03/17] lib: Add missing status check in _notmuch_message_remove_filename Austin Clements
` (18 subsequent siblings)
19 siblings, 0 replies; 51+ messages in thread
From: Austin Clements @ 2011-06-11 20:04 UTC (permalink / raw)
To: notmuch; +Cc: 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
^ permalink raw reply related [flat|nested] 51+ messages in thread* [PATCH 03/17] lib: Add missing status check in _notmuch_message_remove_filename.
2011-06-11 20:04 ` [PATCH v6 00/17] " Austin Clements
2011-06-11 20:04 ` [PATCH 02/17] test: Report test failures from test_expect_* Austin Clements
@ 2011-06-11 20:04 ` Austin Clements
2011-06-11 20:04 ` [PATCH 04/17] test: Test atomicity of notmuch new Austin Clements
` (17 subsequent siblings)
19 siblings, 0 replies; 51+ messages in thread
From: Austin Clements @ 2011-06-11 20:04 UTC (permalink / raw)
To: notmuch; +Cc: 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
^ permalink raw reply related [flat|nested] 51+ messages in thread* [PATCH 04/17] test: Test atomicity of notmuch new.
2011-06-11 20:04 ` [PATCH v6 00/17] " Austin Clements
2011-06-11 20:04 ` [PATCH 02/17] test: Report test failures from test_expect_* Austin Clements
2011-06-11 20:04 ` [PATCH 03/17] lib: Add missing status check in _notmuch_message_remove_filename Austin Clements
@ 2011-06-11 20:04 ` Austin Clements
2011-06-11 20:04 ` [PATCH 05/17] new: Don't lose messages on SIGINT Austin Clements
` (16 subsequent siblings)
19 siblings, 0 replies; 51+ messages in thread
From: Austin Clements @ 2011-06-11 20:04 UTC (permalink / raw)
To: notmuch; +Cc: 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
^ permalink raw reply related [flat|nested] 51+ messages in thread* [PATCH 05/17] new: Don't lose messages on SIGINT.
2011-06-11 20:04 ` [PATCH v6 00/17] " Austin Clements
` (2 preceding siblings ...)
2011-06-11 20:04 ` [PATCH 04/17] test: Test atomicity of notmuch new Austin Clements
@ 2011-06-11 20:04 ` Austin Clements
2011-06-11 20:04 ` [PATCH 06/17] new: Defer updating directory mtimes until the end Austin Clements
` (15 subsequent siblings)
19 siblings, 0 replies; 51+ messages in thread
From: Austin Clements @ 2011-06-11 20:04 UTC (permalink / raw)
To: notmuch; +Cc: 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
^ permalink raw reply related [flat|nested] 51+ messages in thread* [PATCH 06/17] new: Defer updating directory mtimes until the end.
2011-06-11 20:04 ` [PATCH v6 00/17] " Austin Clements
` (3 preceding siblings ...)
2011-06-11 20:04 ` [PATCH 05/17] new: Don't lose messages on SIGINT Austin Clements
@ 2011-06-11 20:04 ` Austin Clements
2011-06-11 20:04 ` [PATCH 07/17] lib: Add notmuch_database_{begin,end}_atomic Austin Clements
` (14 subsequent siblings)
19 siblings, 0 replies; 51+ messages in thread
From: Austin Clements @ 2011-06-11 20:04 UTC (permalink / raw)
To: notmuch; +Cc: 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
^ permalink raw reply related [flat|nested] 51+ messages in thread* [PATCH 07/17] lib: Add notmuch_database_{begin,end}_atomic.
2011-06-11 20:04 ` [PATCH v6 00/17] " Austin Clements
` (4 preceding siblings ...)
2011-06-11 20:04 ` [PATCH 06/17] new: Defer updating directory mtimes until the end Austin Clements
@ 2011-06-11 20:04 ` Austin Clements
2011-06-11 20:04 ` [PATCH 08/17] lib: Add support for nested atomic sections Austin Clements
` (13 subsequent siblings)
19 siblings, 0 replies; 51+ messages in thread
From: Austin Clements @ 2011-06-11 20:04 UTC (permalink / raw)
To: notmuch; +Cc: 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
^ permalink raw reply related [flat|nested] 51+ messages in thread* [PATCH 08/17] lib: Add support for nested atomic sections.
2011-06-11 20:04 ` [PATCH v6 00/17] " Austin Clements
` (5 preceding siblings ...)
2011-06-11 20:04 ` [PATCH 07/17] lib: Add notmuch_database_{begin,end}_atomic Austin Clements
@ 2011-06-11 20:04 ` Austin Clements
2011-06-11 20:04 ` [PATCH 09/17] lib: Indicate if there are more filenames after removal Austin Clements
` (12 subsequent siblings)
19 siblings, 0 replies; 51+ messages in thread
From: Austin Clements @ 2011-06-11 20:04 UTC (permalink / raw)
To: notmuch; +Cc: 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
^ permalink raw reply related [flat|nested] 51+ messages in thread* [PATCH 09/17] lib: Indicate if there are more filenames after removal.
2011-06-11 20:04 ` [PATCH v6 00/17] " Austin Clements
` (6 preceding siblings ...)
2011-06-11 20:04 ` [PATCH 08/17] lib: Add support for nested atomic sections Austin Clements
@ 2011-06-11 20:04 ` Austin Clements
2011-06-11 20:04 ` [PATCH 10/17] lib: Remove message document directly after removing the last file name Austin Clements
` (11 subsequent siblings)
19 siblings, 0 replies; 51+ messages in thread
From: Austin Clements @ 2011-06-11 20:04 UTC (permalink / raw)
To: notmuch; +Cc: 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
^ permalink raw reply related [flat|nested] 51+ messages in thread* [PATCH 10/17] lib: Remove message document directly after removing the last file name.
2011-06-11 20:04 ` [PATCH v6 00/17] " Austin Clements
` (7 preceding siblings ...)
2011-06-11 20:04 ` [PATCH 09/17] lib: Indicate if there are more filenames after removal Austin Clements
@ 2011-06-11 20:04 ` Austin Clements
2011-06-11 20:04 ` [PATCH 11/17] lib: Add an API to find a message by filename Austin Clements
` (10 subsequent siblings)
19 siblings, 0 replies; 51+ messages in thread
From: Austin Clements @ 2011-06-11 20:04 UTC (permalink / raw)
To: notmuch; +Cc: 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
^ permalink raw reply related [flat|nested] 51+ messages in thread* [PATCH 11/17] lib: Add an API to find a message by filename.
2011-06-11 20:04 ` [PATCH v6 00/17] " Austin Clements
` (8 preceding siblings ...)
2011-06-11 20:04 ` [PATCH 10/17] lib: Remove message document directly after removing the last file name Austin Clements
@ 2011-06-11 20:04 ` Austin Clements
2011-06-11 20:04 ` [PATCH 12/17] lib: Wrap notmuch_database_add_message in an atomic section Austin Clements
` (9 subsequent siblings)
19 siblings, 0 replies; 51+ messages in thread
From: Austin Clements @ 2011-06-11 20:04 UTC (permalink / raw)
To: notmuch; +Cc: 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
^ permalink raw reply related [flat|nested] 51+ messages in thread* [PATCH 12/17] lib: Wrap notmuch_database_add_message in an atomic section.
2011-06-11 20:04 ` [PATCH v6 00/17] " Austin Clements
` (9 preceding siblings ...)
2011-06-11 20:04 ` [PATCH 11/17] lib: Add an API to find a message by filename Austin Clements
@ 2011-06-11 20:04 ` Austin Clements
2011-06-11 20:04 ` [PATCH 13/17] new: Cleanup. Put removed/renamed message count in add_files_state_t Austin Clements
` (8 subsequent siblings)
19 siblings, 0 replies; 51+ messages in thread
From: Austin Clements @ 2011-06-11 20:04 UTC (permalink / raw)
To: notmuch; +Cc: 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
^ permalink raw reply related [flat|nested] 51+ messages in thread* [PATCH 13/17] new: Cleanup. Put removed/renamed message count in add_files_state_t.
2011-06-11 20:04 ` [PATCH v6 00/17] " Austin Clements
` (10 preceding siblings ...)
2011-06-11 20:04 ` [PATCH 12/17] lib: Wrap notmuch_database_add_message in an atomic section Austin Clements
@ 2011-06-11 20:04 ` Austin Clements
2011-06-11 20:04 ` [PATCH 14/17] new: Cleanup. De-duplicate file name removal code Austin Clements
` (7 subsequent siblings)
19 siblings, 0 replies; 51+ messages in thread
From: Austin Clements @ 2011-06-11 20:04 UTC (permalink / raw)
To: notmuch; +Cc: 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
^ permalink raw reply related [flat|nested] 51+ messages in thread* [PATCH 14/17] new: Cleanup. De-duplicate file name removal code.
2011-06-11 20:04 ` [PATCH v6 00/17] " Austin Clements
` (11 preceding siblings ...)
2011-06-11 20:04 ` [PATCH 13/17] new: Cleanup. Put removed/renamed message count in add_files_state_t Austin Clements
@ 2011-06-11 20:04 ` Austin Clements
2011-06-11 20:04 ` [PATCH 15/17] new: Synchronize maildir flags eagerly Austin Clements
` (6 subsequent siblings)
19 siblings, 0 replies; 51+ messages in thread
From: Austin Clements @ 2011-06-11 20:04 UTC (permalink / raw)
To: notmuch; +Cc: 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
^ permalink raw reply related [flat|nested] 51+ messages in thread* [PATCH 15/17] new: Synchronize maildir flags eagerly.
2011-06-11 20:04 ` [PATCH v6 00/17] " Austin Clements
` (12 preceding siblings ...)
2011-06-11 20:04 ` [PATCH 14/17] new: Cleanup. De-duplicate file name removal code Austin Clements
@ 2011-06-11 20:04 ` Austin Clements
2011-06-11 20:04 ` [PATCH 16/17] new: Wrap adding and removing messages in atomic sections Austin Clements
` (5 subsequent siblings)
19 siblings, 0 replies; 51+ messages in thread
From: Austin Clements @ 2011-06-11 20:04 UTC (permalink / raw)
To: notmuch; +Cc: 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
^ permalink raw reply related [flat|nested] 51+ messages in thread* [PATCH 16/17] new: Wrap adding and removing messages in atomic sections.
2011-06-11 20:04 ` [PATCH v6 00/17] " Austin Clements
` (13 preceding siblings ...)
2011-06-11 20:04 ` [PATCH 15/17] new: Synchronize maildir flags eagerly Austin Clements
@ 2011-06-11 20:04 ` Austin Clements
2011-06-11 20:04 ` [PATCH 17/17] lib: Improve notmuch_database_{add, remove}_message documentation Austin Clements
` (4 subsequent siblings)
19 siblings, 0 replies; 51+ messages in thread
From: Austin Clements @ 2011-06-11 20:04 UTC (permalink / raw)
To: notmuch; +Cc: 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
^ permalink raw reply related [flat|nested] 51+ messages in thread* [PATCH 17/17] lib: Improve notmuch_database_{add, remove}_message documentation.
2011-06-11 20:04 ` [PATCH v6 00/17] " Austin Clements
` (14 preceding siblings ...)
2011-06-11 20:04 ` [PATCH 16/17] new: Wrap adding and removing messages in atomic sections Austin Clements
@ 2011-06-11 20:04 ` Austin Clements
2011-06-11 20:49 ` [PATCH 01/17] test: Fix message when skipping test_expect_equal* tests Austin Clements
` (3 subsequent siblings)
19 siblings, 0 replies; 51+ messages in thread
From: Austin Clements @ 2011-06-11 20:04 UTC (permalink / raw)
To: notmuch; +Cc: 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
^ permalink raw reply related [flat|nested] 51+ messages in thread* [PATCH 01/17] test: Fix message when skipping test_expect_equal* tests
2011-06-11 20:04 ` [PATCH v6 00/17] " Austin Clements
` (15 preceding siblings ...)
2011-06-11 20:04 ` [PATCH 17/17] lib: Improve notmuch_database_{add, remove}_message documentation Austin Clements
@ 2011-06-11 20:49 ` Austin Clements
2011-06-11 20:53 ` [PATCH v6 00/17] Fix 'notmuch new' atomicity issues Austin Clements
` (2 subsequent siblings)
19 siblings, 0 replies; 51+ messages in thread
From: Austin Clements @ 2011-06-11 20:49 UTC (permalink / raw)
To: notmuch; +Cc: 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
^ permalink raw reply related [flat|nested] 51+ messages in thread* Re: [PATCH v6 00/17] Fix 'notmuch new' atomicity issues
2011-06-11 20:04 ` [PATCH v6 00/17] " Austin Clements
` (16 preceding siblings ...)
2011-06-11 20:49 ` [PATCH 01/17] test: Fix message when skipping test_expect_equal* tests Austin Clements
@ 2011-06-11 20:53 ` Austin Clements
2011-06-22 19:39 ` Austin Clements
2011-09-28 16:36 ` [PATCH v6 00/17] Fix 'notmuch new' atomicity issues Sebastian Spaeth
19 siblings, 0 replies; 51+ messages in thread
From: Austin Clements @ 2011-06-11 20:53 UTC (permalink / raw)
To: notmuch
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.)
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [PATCH v6 00/17] Fix 'notmuch new' atomicity issues
2011-06-11 20:04 ` [PATCH v6 00/17] " Austin Clements
` (17 preceding siblings ...)
2011-06-11 20:53 ` [PATCH v6 00/17] Fix 'notmuch new' atomicity issues Austin Clements
@ 2011-06-22 19:39 ` Austin Clements
2011-06-29 6:37 ` Austin Clements
2011-07-08 18:09 ` Austin Clements
2011-09-28 16:36 ` [PATCH v6 00/17] Fix 'notmuch new' atomicity issues Sebastian Spaeth
19 siblings, 2 replies; 51+ messages in thread
From: Austin Clements @ 2011-06-22 19:39 UTC (permalink / raw)
To: Carl Worth; +Cc: notmuch
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
>
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [PATCH v6 00/17] Fix 'notmuch new' atomicity issues
2011-06-22 19:39 ` Austin Clements
@ 2011-06-29 6:37 ` Austin Clements
2011-07-08 18:09 ` Austin Clements
1 sibling, 0 replies; 51+ messages in thread
From: Austin Clements @ 2011-06-29 6:37 UTC (permalink / raw)
To: Carl Worth; +Cc: notmuch
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.
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH v6 00/17] Fix 'notmuch new' atomicity issues
2011-06-22 19:39 ` Austin Clements
2011-06-29 6:37 ` Austin Clements
@ 2011-07-08 18:09 ` Austin Clements
2011-09-13 12:39 ` David Bremner
1 sibling, 1 reply; 51+ messages in thread
From: Austin Clements @ 2011-07-08 18:09 UTC (permalink / raw)
To: Carl Worth; +Cc: notmuch
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
>>
>
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [PATCH v6 00/17] Fix 'notmuch new' atomicity issues
2011-07-08 18:09 ` Austin Clements
@ 2011-09-13 12:39 ` David Bremner
2011-09-24 3:14 ` David Bremner
0 siblings, 1 reply; 51+ messages in thread
From: David Bremner @ 2011-09-13 12:39 UTC (permalink / raw)
To: Austin Clements; +Cc: notmuch
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
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [PATCH v6 00/17] Fix 'notmuch new' atomicity issues
2011-09-13 12:39 ` David Bremner
@ 2011-09-24 3:14 ` David Bremner
2011-09-24 4:03 ` Austin Clements
0 siblings, 1 reply; 51+ messages in thread
From: David Bremner @ 2011-09-24 3:14 UTC (permalink / raw)
To: Austin Clements; +Cc: notmuch
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
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH v6 00/17] Fix 'notmuch new' atomicity issues
2011-09-24 3:14 ` David Bremner
@ 2011-09-24 4:03 ` Austin Clements
2011-09-25 2:36 ` David Bremner
0 siblings, 1 reply; 51+ messages in thread
From: Austin Clements @ 2011-09-24 4:03 UTC (permalink / raw)
To: David Bremner; +Cc: notmuch
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!
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH v6 00/17] Fix 'notmuch new' atomicity issues
2011-09-24 4:03 ` Austin Clements
@ 2011-09-25 2:36 ` David Bremner
2011-09-26 22:07 ` Austin Clements
0 siblings, 1 reply; 51+ messages in thread
From: David Bremner @ 2011-09-25 2:36 UTC (permalink / raw)
To: Austin Clements; +Cc: notmuch
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
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH v6 00/17] Fix 'notmuch new' atomicity issues
2011-09-25 2:36 ` David Bremner
@ 2011-09-26 22:07 ` Austin Clements
2011-09-26 23:32 ` NEWS for 0.9 David Bremner
0 siblings, 1 reply; 51+ messages in thread
From: Austin Clements @ 2011-09-26 22:07 UTC (permalink / raw)
To: David Bremner; +Cc: notmuch
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.
^ permalink raw reply [flat|nested] 51+ messages in thread
* NEWS for 0.9
2011-09-26 22:07 ` Austin Clements
@ 2011-09-26 23:32 ` David Bremner
2011-09-27 13:08 ` Ali Polatel
0 siblings, 1 reply; 51+ messages in thread
From: David Bremner @ 2011-09-26 23:32 UTC (permalink / raw)
To: Austin Clements; +Cc: Ali Polatel, notmuch
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
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: NEWS for 0.9
2011-09-26 23:32 ` NEWS for 0.9 David Bremner
@ 2011-09-27 13:08 ` Ali Polatel
0 siblings, 0 replies; 51+ messages in thread
From: Ali Polatel @ 2011-09-27 13:08 UTC (permalink / raw)
To: David Bremner, Austin Clements; +Cc: notmuch
[-- 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 --]
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH v6 00/17] Fix 'notmuch new' atomicity issues
2011-06-11 20:04 ` [PATCH v6 00/17] " Austin Clements
` (18 preceding siblings ...)
2011-06-22 19:39 ` Austin Clements
@ 2011-09-28 16:36 ` Sebastian Spaeth
2011-09-29 15:01 ` Austin Clements
19 siblings, 1 reply; 51+ messages in thread
From: Sebastian Spaeth @ 2011-09-28 16:36 UTC (permalink / raw)
To: Austin Clements, notmuch
[-- 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 --]
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [PATCH v6 00/17] Fix 'notmuch new' atomicity issues
2011-09-28 16:36 ` [PATCH v6 00/17] Fix 'notmuch new' atomicity issues Sebastian Spaeth
@ 2011-09-29 15:01 ` Austin Clements
2011-09-30 9:21 ` Sebastian Spaeth
0 siblings, 1 reply; 51+ messages in thread
From: Austin Clements @ 2011-09-29 15:01 UTC (permalink / raw)
To: Sebastian Spaeth; +Cc: notmuch
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.
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [PATCH v6 00/17] Fix 'notmuch new' atomicity issues
2011-09-29 15:01 ` Austin Clements
@ 2011-09-30 9:21 ` Sebastian Spaeth
0 siblings, 0 replies; 51+ messages in thread
From: Sebastian Spaeth @ 2011-09-30 9:21 UTC (permalink / raw)
To: Austin Clements; +Cc: notmuch
[-- 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 --]
^ permalink raw reply [flat|nested] 51+ messages in thread