(no subject)

[David Bremner]

This is in some sense a successor to 

     id:cover.1411914914.git.jani@nikula.org

It includes the first two patches of that series verbatim, and adds
some tests.

[Patch v2.5 1/4] test/insert: add known broken tests for indexing failures

[David Bremner]

These tests are written with the assumption that we want all indexing
failures to be considered as failures by notmuch insert.
---
 test/T070-insert.sh | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/test/T070-insert.sh b/test/T070-insert.sh
index ea9db07..40a7636 100755
--- a/test/T070-insert.sh
+++ b/test/T070-insert.sh
@@ -183,4 +183,26 @@ test_expect_code 1 "Invalid tags set exit code" \
 
 notmuch config set new.tags $OLDCONFIG
 
+# DUPLICATE_MESSAGE_ID is not tested here, because it should actually pass.
+
+for code in OUT_OF_MEMORY XAPIAN_EXCEPTION FILE_NOT_EMAIL \
+    READ_ONLY_DATABASE UPGRADE_REQUIRED; do
+gen_insert_msg
+cat <<EOF > index-file-$code.gdb
+file notmuch
+set breakpoint pending on
+break notmuch_database_add_message
+commands
+return NOTMUCH_STATUS_$code
+continue
+end
+run
+EOF
+test_begin_subtest "error exit when add_message returns $code"
+test_subtest_known_broken
+gdb --batch-silent --return-child-result -x index-file-$code.gdb \
+    --args notmuch insert  < $gen_msg_filename
+test_expect_equal $? 1
+done
+
 test_done
-- 
2.1.0

[Patch v2.5 2/4] cli/insert: add fail path to add_file_to_database

[David Bremner]

From: Jani Nikula <jani@nikula.org>

Handle failures gracefully in add_file_to_database, renamed simply
add_file while at it. Add keep option to not remove the message from
database if tagging or tag syncing to maildir flags fails. Expand the
function documentation to cover the changes.
---
 notmuch-insert.c | 99 ++++++++++++++++++++++++++++++++++++--------------------
 1 file changed, 64 insertions(+), 35 deletions(-)

diff --git a/notmuch-insert.c b/notmuch-insert.c
index 5ef6e66..0ea4380 100644
--- a/notmuch-insert.c
+++ b/notmuch-insert.c
@@ -364,50 +364,80 @@ FAIL:
     return NULL;
 }
 
-/* Add the specified message file to the notmuch database, applying tags.
- * The file is renamed to encode notmuch tags as maildir flags. */
-static void
-add_file_to_database (notmuch_database_t *notmuch, const char *path,
-		      tag_op_list_t *tag_ops, notmuch_bool_t synchronize_flags)
+/*
+ * Add the specified message file to the notmuch database, applying
+ * tags in tag_ops. If synchronize_flags is TRUE, the tags are
+ * synchronized to maildir flags (which may result in message file
+ * rename).
+ *
+ * Return NOTMUCH_STATUS_SUCCESS on success, errors otherwise. If keep
+ * is TRUE, errors in tag changes and flag syncing are ignored and
+ * success status is returned; otherwise such errors cause the message
+ * to be removed from the database. Failure to add the message to the
+ * database results in error status regardless of keep.
+ */
+static notmuch_status_t
+add_file (notmuch_database_t *notmuch, const char *path, tag_op_list_t *tag_ops,
+	  notmuch_bool_t synchronize_flags, notmuch_bool_t keep)
 {
     notmuch_message_t *message;
     notmuch_status_t status;
 
     status = notmuch_database_add_message (notmuch, path, &message);
-    switch (status) {
-    case NOTMUCH_STATUS_SUCCESS:
-    case NOTMUCH_STATUS_DUPLICATE_MESSAGE_ID:
-	break;
-    default:
-    case NOTMUCH_STATUS_FILE_NOT_EMAIL:
-    case NOTMUCH_STATUS_READ_ONLY_DATABASE:
-    case NOTMUCH_STATUS_XAPIAN_EXCEPTION:
-    case NOTMUCH_STATUS_OUT_OF_MEMORY:
-    case NOTMUCH_STATUS_FILE_ERROR:
-    case NOTMUCH_STATUS_NULL_POINTER:
-    case NOTMUCH_STATUS_TAG_TOO_LONG:
-    case NOTMUCH_STATUS_UNBALANCED_FREEZE_THAW:
-    case NOTMUCH_STATUS_UNBALANCED_ATOMIC:
-    case NOTMUCH_STATUS_LAST_STATUS:
-	fprintf (stderr, "Error: failed to add `%s' to notmuch database: %s\n",
-		 path, notmuch_status_to_string (status));
-	return;
-    }
-
-    if (status == NOTMUCH_STATUS_DUPLICATE_MESSAGE_ID) {
-	/* Don't change tags of an existing message. */
-	if (synchronize_flags) {
-	    status = notmuch_message_tags_to_maildir_flags (message);
-	    if (status != NOTMUCH_STATUS_SUCCESS)
-		fprintf (stderr, "Error: failed to sync tags to maildir flags\n");
+    if (status == NOTMUCH_STATUS_SUCCESS) {
+	status = tag_op_list_apply (message, tag_ops, 0);
+	if (status) {
+	    fprintf (stderr, "%s: failed to apply tags to file '%s': %s\n",
+		     keep ? "Warning" : "Error",
+		     path, notmuch_status_to_string (status));
+	    goto DONE;
 	}
+    } else if (status == NOTMUCH_STATUS_DUPLICATE_MESSAGE_ID) {
+	status = NOTMUCH_STATUS_SUCCESS;
+    } else if (status == NOTMUCH_STATUS_FILE_NOT_EMAIL) {
+	fprintf (stderr, "Error: delivery of non-mail file: '%s'\n", path);
+	goto FAIL;
     } else {
-	tag_op_flag_t flags = synchronize_flags ? TAG_FLAG_MAILDIR_SYNC : 0;
+	fprintf (stderr, "Error: failed to add '%s' to notmuch database: %s\n",
+		 path, notmuch_status_to_string (status));
+	goto FAIL;
+    }
 
-	tag_op_list_apply (message, tag_ops, flags);
+    if (synchronize_flags) {
+	status = notmuch_message_tags_to_maildir_flags (message);
+	if (status != NOTMUCH_STATUS_SUCCESS)
+	    fprintf (stderr, "%s: failed to sync tags to maildir flags for '%s': %s\n",
+		     keep ? "Warning" : "Error",
+		     path, notmuch_status_to_string (status));
+
+	/*
+	 * Note: Unfortunately a failed maildir flag sync might
+	 * already have renamed the file, in which case the cleanup
+	 * path may fail.
+	 */
     }
 
+  DONE:
     notmuch_message_destroy (message);
+
+    if (status) {
+	if (keep) {
+	    status = NOTMUCH_STATUS_SUCCESS;
+	} else {
+	    notmuch_status_t cleanup_status;
+
+	    cleanup_status = notmuch_database_remove_message (notmuch, path);
+	    if (cleanup_status != NOTMUCH_STATUS_SUCCESS &&
+		cleanup_status != NOTMUCH_STATUS_DUPLICATE_MESSAGE_ID) {
+		fprintf (stderr, "Warning: failed to remove '%s' from database "
+			 "after errors: %s. Please run 'notmuch new' to fix.\n",
+			 path, notmuch_status_to_string (cleanup_status));
+	    }
+	}
+    }
+
+  FAIL:
+    return status;
 }
 
 int
@@ -508,8 +538,7 @@ notmuch_insert_command (notmuch_config_t *config, int argc, char *argv[])
     /* Add the message to the index.
      * Even if adding the message to the notmuch database fails,
      * the message is on disk and we consider the delivery completed. */
-    add_file_to_database (notmuch, newpath, tag_ops,
-				    synchronize_flags);
+    add_file (notmuch, newpath, tag_ops, synchronize_flags, TRUE);
 
     notmuch_database_destroy (notmuch);
     return EXIT_SUCCESS;
-- 
2.1.0

[Patch v2.5 3/4] cli/insert: require succesful message indexing for success status

[David Bremner]

From: Jani Nikula <jani@nikula.org>

Add --keep option to keep any remaining stuff in index or file. We
could distinguish between failures to index and failures to apply tags
or maildir sync, but for simplicity just have one.
---
 doc/man1/notmuch-insert.rst | 19 ++++++++++++-------
 notmuch-insert.c            | 36 ++++++++++++++++++++++++++++++------
 test/T070-insert.sh         |  3 +--
 3 files changed, 43 insertions(+), 15 deletions(-)

diff --git a/doc/man1/notmuch-insert.rst b/doc/man1/notmuch-insert.rst
index 2be1a7b..e396f6c 100644
--- a/doc/man1/notmuch-insert.rst
+++ b/doc/man1/notmuch-insert.rst
@@ -38,16 +38,21 @@ Supported options for **insert** include
         does not exist. Otherwise the folder must already exist for mail
         delivery to succeed.
 
+    ``--keep``
+        Keep the message file if indexing fails, and keep the message
+        indexed if applying tags or maildir flag synchronization
+        fails. Ignore these errors and return exit status 0 to
+        indicate succesful mail delivery.
+
 EXIT STATUS
 ===========
 
-This command returns exit status 0 if the message was successfully added
-to the mail directory, even if the message could not be indexed and
-added to the notmuch database. In the latter case, a warning will be
-printed to standard error but the message file will be left on disk.
-
-If the message could not be written to disk then a non-zero exit status
-is returned.
+This command returns exit status 0 on succesful mail delivery,
+non-zero otherwise. The default is to indicate failed mail delivery on
+any errors, including message file delivery to the filesystem, message
+indexing to Notmuch database, changing tags, and synchronizing tags to
+maildir flags. The ``--keep`` option may be used to settle for
+successful message file delivery.
 
 SEE ALSO
 ========
diff --git a/notmuch-insert.c b/notmuch-insert.c
index 0ea4380..7074077 100644
--- a/notmuch-insert.c
+++ b/notmuch-insert.c
@@ -443,6 +443,7 @@ add_file (notmuch_database_t *notmuch, const char *path, tag_op_list_t *tag_ops,
 int
 notmuch_insert_command (notmuch_config_t *config, int argc, char *argv[])
 {
+    notmuch_status_t status, close_status;
     notmuch_database_t *notmuch;
     struct sigaction action;
     const char *db_path;
@@ -452,6 +453,7 @@ notmuch_insert_command (notmuch_config_t *config, int argc, char *argv[])
     char *query_string = NULL;
     const char *folder = NULL;
     notmuch_bool_t create_folder = FALSE;
+    notmuch_bool_t keep = FALSE;
     notmuch_bool_t synchronize_flags;
     const char *maildir;
     char *newpath;
@@ -461,6 +463,7 @@ notmuch_insert_command (notmuch_config_t *config, int argc, char *argv[])
     notmuch_opt_desc_t options[] = {
 	{ NOTMUCH_OPT_STRING, &folder, "folder", 0, 0 },
 	{ NOTMUCH_OPT_BOOLEAN, &create_folder, "create-folder", 0, 0 },
+	{ NOTMUCH_OPT_BOOLEAN, &keep, "keep", 0, 0 },
 	{ NOTMUCH_OPT_END, 0, 0, 0, 0 }
     };
 
@@ -535,11 +538,32 @@ notmuch_insert_command (notmuch_config_t *config, int argc, char *argv[])
 	return EXIT_FAILURE;
     }
 
-    /* Add the message to the index.
-     * Even if adding the message to the notmuch database fails,
-     * the message is on disk and we consider the delivery completed. */
-    add_file (notmuch, newpath, tag_ops, synchronize_flags, TRUE);
+    /* Index the message. */
+    status = add_file (notmuch, newpath, tag_ops, synchronize_flags, keep);
+
+    /* Commit changes. */
+    close_status = notmuch_database_destroy (notmuch);
+    if (close_status) {
+	/* Hold on to the first error, if any. */
+	if (! status)
+	    status = close_status;
+	fprintf (stderr, "%s: failed to commit database changes: %s\n",
+		 keep ? "Warning" : "Error",
+		 notmuch_status_to_string (close_status));
+    }
+
+    if (status) {
+	if (keep) {
+	    status = NOTMUCH_STATUS_SUCCESS;
+	} else {
+	    /* If maildir flag sync failed, this might fail. */
+	    if (unlink (newpath)) {
+		fprintf (stderr, "Warning: failed to remove '%s' from maildir "
+			 "after errors: %s. Please run 'notmuch new' to fix.\n",
+			 newpath, strerror (errno));
+	    }
+	}
+    }
 
-    notmuch_database_destroy (notmuch);
-    return EXIT_SUCCESS;
+    return status ? EXIT_FAILURE : EXIT_SUCCESS;
 }
diff --git a/test/T070-insert.sh b/test/T070-insert.sh
index 40a7636..be8060e 100755
--- a/test/T070-insert.sh
+++ b/test/T070-insert.sh
@@ -23,7 +23,7 @@ test_expect_code 1 "Insert zero-length file" \
 
 # This test is a proxy for other errors that may occur while trying to
 # add a message to the notmuch database, e.g. database locked.
-test_expect_code 0 "Insert non-message" \
+test_expect_code 1 "Insert non-message" \
     "echo bad_message | notmuch insert"
 
 test_begin_subtest "Database empty so far"
@@ -199,7 +199,6 @@ end
 run
 EOF
 test_begin_subtest "error exit when add_message returns $code"
-test_subtest_known_broken
 gdb --batch-silent --return-child-result -x index-file-$code.gdb \
     --args notmuch insert  < $gen_msg_filename
 test_expect_equal $? 1
-- 
2.1.0

[Patch v2.5 4/4] test/insert: check that indexing errors are accepted with --keep

[David Bremner]

This is overkill for the current code path, but should provide some
robustness for future changes in error handling.
---
 test/T070-insert.sh | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/test/T070-insert.sh b/test/T070-insert.sh
index be8060e..80a22c1 100755
--- a/test/T070-insert.sh
+++ b/test/T070-insert.sh
@@ -202,6 +202,11 @@ test_begin_subtest "error exit when add_message returns $code"
 gdb --batch-silent --return-child-result -x index-file-$code.gdb \
     --args notmuch insert  < $gen_msg_filename
 test_expect_equal $? 1
+
+test_begin_subtest "success exit with --keep when add_message returns $code"
+gdb --batch-silent --return-child-result -x index-file-$code.gdb \
+    --args notmuch insert --keep  < $gen_msg_filename
+test_expect_equal $? 0
 done
 
 test_done
-- 
2.1.0

Re:

[David Bremner]

David Bremner <david@tethera.net> writes:

> This is in some sense a successor to 
>
>      id:cover.1411914914.git.jani@nikula.org
>
> It includes the first two patches of that series verbatim, and adds
> some tests.

I should have said _almost_ verbatim; it marks some tests non-broken.

Re: tests for notmuch-insert

[David Bremner]

David Bremner <david@tethera.net> writes:

> This is in some sense a successor to 
>
>      id:cover.1411914914.git.jani@nikula.org
>
> It includes the first two patches of that series verbatim, and adds
> some tests.
>

This series depends on the enabling of debug symbols in

     id:1412367730-25890-1-git-send-email-david@tethera.net

I did (just now) test it with gdb 7.8, and it seems OK.

Re:

[Jani Nikula]

On Sat, 04 Oct 2014, David Bremner <david@tethera.net> wrote:
> This is in some sense a successor to 
>
>      id:cover.1411914914.git.jani@nikula.org
>
> It includes the first two patches of that series verbatim, and adds
> some tests.

I like it, very nice. Start pushing and add the post-insert hook patch
from my series on top?  ;)

BR,
Jani.

Re: [Patch v2.5 1/4] test/insert: add known broken tests for indexing failures

[Jani Nikula]

On Sat, 04 Oct 2014, David Bremner <david@tethera.net> wrote:
> These tests are written with the assumption that we want all indexing
> failures to be considered as failures by notmuch insert.

Just realized this needs a missing prereq test for gdb.

BR,
Jani.


> ---
>  test/T070-insert.sh | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
>
> diff --git a/test/T070-insert.sh b/test/T070-insert.sh
> index ea9db07..40a7636 100755
> --- a/test/T070-insert.sh
> +++ b/test/T070-insert.sh
> @@ -183,4 +183,26 @@ test_expect_code 1 "Invalid tags set exit code" \
>  
>  notmuch config set new.tags $OLDCONFIG
>  
> +# DUPLICATE_MESSAGE_ID is not tested here, because it should actually pass.
> +
> +for code in OUT_OF_MEMORY XAPIAN_EXCEPTION FILE_NOT_EMAIL \
> +    READ_ONLY_DATABASE UPGRADE_REQUIRED; do
> +gen_insert_msg
> +cat <<EOF > index-file-$code.gdb
> +file notmuch
> +set breakpoint pending on
> +break notmuch_database_add_message
> +commands
> +return NOTMUCH_STATUS_$code
> +continue
> +end
> +run
> +EOF
> +test_begin_subtest "error exit when add_message returns $code"
> +test_subtest_known_broken
> +gdb --batch-silent --return-child-result -x index-file-$code.gdb \
> +    --args notmuch insert  < $gen_msg_filename
> +test_expect_equal $? 1
> +done
> +
>  test_done
> -- 
> 2.1.0
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [Patch v2.5 1/4] test/insert: add known broken tests for indexing failures

[David Bremner]

Jani Nikula <jani@nikula.org> writes:

> On Sat, 04 Oct 2014, David Bremner <david@tethera.net> wrote:
>> These tests are written with the assumption that we want all indexing
>> failures to be considered as failures by notmuch insert.
>
> Just realized this needs a missing prereq test for gdb.
>
> BR,
> Jani.

oops. missed this until after I pushed. I'll look into it ASAP.

d

Re: [Patch v2.5 1/4] test/insert: add known broken tests for indexing failures

[David Bremner]

David Bremner <david@tethera.net> writes:

> Jani Nikula <jani@nikula.org> writes:
>
>> On Sat, 04 Oct 2014, David Bremner <david@tethera.net> wrote:
>>> These tests are written with the assumption that we want all indexing
>>> failures to be considered as failures by notmuch insert.
>>
>> Just realized this needs a missing prereq test for gdb.
>>
>> BR,
>> Jani.
>
> oops. missed this until after I pushed. I'll look into it ASAP.

I took the liberty of pushing a one-line build fix.  I'm not 100%
certain, but I think the usage of "test_require_external_prereq" in 
T380-atomicity.sh could be simplified in a similary way.

d