[PATCH v2 00/10] add insert --must-index option

[PATCH v2 00/10] add insert --must-index option

[Peter Wang]

Follow up to id:1374365254-13227-1-git-send-email-novalazy@gmail.com
The main changes are to take into account failures during
tagging and flushing of the database.

I took Jani's patch id:1390152046-6509-1-git-send-email-jani@nikula.org
without modification.

The soname bump is included in case it is required.

The python/go/ruby changes are untested.


Jani Nikula (1):
  lib: add return status to database close and destroy

Peter Wang (9):
  lib: bump soname
  python: handle return status of database close and destroy
  go: add return status to database close method
  ruby: handle return status of database close
  cli: refactor insert
  cli: indicate insert failure mode in exit status
  cli: add insert --must-index option
  test: test insert --must-index
  man: update insert documentation

 bindings/go/src/notmuch/notmuch.go  |   4 +-
 bindings/python/notmuch/database.py |  12 ++--
 bindings/ruby/database.c            |   4 +-
 doc/man1/notmuch-insert.rst         |  24 +++++--
 lib/Makefile.local                  |   2 +-
 lib/database.cc                     |  30 ++++++--
 lib/notmuch.h                       |  15 +++-
 notmuch-insert.c                    | 134 +++++++++++++++++++++---------------
 test/T070-insert.sh                 |  32 +++++++--
 9 files changed, 176 insertions(+), 81 deletions(-)

-- 
1.8.4

[PATCH v2 01/10] lib: add return status to database close and destroy

[Peter Wang]

From: Jani Nikula <jani@nikula.org>

notmuch_database_close may fail in Xapian ->flush() or ->close(), so
report the status. Similarly for notmuch_database_destroy which calls
close.

This is required for notmuch insert to report error status if message
indexing failed.
---
 lib/database.cc | 30 ++++++++++++++++++++++++------
 lib/notmuch.h   | 15 +++++++++++++--
 2 files changed, 37 insertions(+), 8 deletions(-)

diff --git a/lib/database.cc b/lib/database.cc
index 1efb14d..ef7005b 100644
--- a/lib/database.cc
+++ b/lib/database.cc
@@ -774,14 +774,17 @@ notmuch_database_open (const char *path,
     return status;
 }
 
-void
+notmuch_status_t
 notmuch_database_close (notmuch_database_t *notmuch)
 {
+    notmuch_status_t status = NOTMUCH_STATUS_SUCCESS;
+
     try {
 	if (notmuch->xapian_db != NULL &&
 	    notmuch->mode == NOTMUCH_DATABASE_MODE_READ_WRITE)
 	    (static_cast <Xapian::WritableDatabase *> (notmuch->xapian_db))->flush ();
     } catch (const Xapian::Error &error) {
+	status = NOTMUCH_STATUS_XAPIAN_EXCEPTION;
 	if (! notmuch->exception_reported) {
 	    fprintf (stderr, "Error: A Xapian exception occurred flushing database: %s\n",
 		     error.get_msg().c_str());
@@ -795,7 +798,9 @@ notmuch_database_close (notmuch_database_t *notmuch)
 	try {
 	    notmuch->xapian_db->close();
 	} catch (const Xapian::Error &error) {
-	    /* do nothing */
+	    /* don't clobber previous error status */
+	    if (status == NOTMUCH_STATUS_SUCCESS)
+		status = NOTMUCH_STATUS_XAPIAN_EXCEPTION;
 	}
     }
 
@@ -809,6 +814,8 @@ notmuch_database_close (notmuch_database_t *notmuch)
     notmuch->value_range_processor = NULL;
     delete notmuch->date_range_processor;
     notmuch->date_range_processor = NULL;
+
+    return status;
 }
 
 #if HAVE_XAPIAN_COMPACT
@@ -972,8 +979,15 @@ notmuch_database_compact (const char *path,
     }
 
   DONE:
-    if (notmuch)
-	notmuch_database_destroy (notmuch);
+    if (notmuch) {
+	notmuch_status_t ret2;
+
+	ret2 = notmuch_database_destroy (notmuch);
+
+	/* don't clobber previous error status */
+	if (ret == NOTMUCH_STATUS_SUCCESS && ret2 != NOTMUCH_STATUS_SUCCESS)
+	    ret = ret2;
+    }
 
     talloc_free (local);
 
@@ -991,11 +1005,15 @@ notmuch_database_compact (unused (const char *path),
 }
 #endif
 
-void
+notmuch_status_t
 notmuch_database_destroy (notmuch_database_t *notmuch)
 {
-    notmuch_database_close (notmuch);
+    notmuch_status_t status;
+
+    status = notmuch_database_close (notmuch);
     talloc_free (notmuch);
+
+    return status;
 }
 
 const char *
diff --git a/lib/notmuch.h b/lib/notmuch.h
index 350bed8..3c5ec98 100644
--- a/lib/notmuch.h
+++ b/lib/notmuch.h
@@ -287,8 +287,16 @@ notmuch_database_open (const char *path,
  *
  * notmuch_database_close can be called multiple times.  Later calls
  * have no effect.
+ *
+ * Return value:
+ *
+ * NOTMUCH_STATUS_SUCCESS: Successfully closed the database.
+ *
+ * NOTMUCH_STATUS_XAPIAN_EXCEPTION: A Xapian exception occurred; the
+ *	database has been closed but there are no guarantees the
+ *	changes to the database, if any, have been flushed to disk.
  */
-void
+notmuch_status_t
 notmuch_database_close (notmuch_database_t *database);
 
 /**
@@ -317,8 +325,11 @@ notmuch_database_compact (const char* path,
 /**
  * Destroy the notmuch database, closing it if necessary and freeing
  * all associated resources.
+ *
+ * Return value as in notmuch_database_close if the database was open;
+ * notmuch_database_destroy itself has no failure modes.
  */
-void
+notmuch_status_t
 notmuch_database_destroy (notmuch_database_t *database);
 
 /**
-- 
1.8.4

Re: [PATCH v2 01/10] lib: add return status to database close and destroy

[David Bremner]

Peter Wang <novalazy@gmail.com> writes:

> From: Jani Nikula <jani@nikula.org>
>
> notmuch_database_close may fail in Xapian ->flush() or ->close(), so
> report the status. Similarly for notmuch_database_destroy which calls
> close.

This patch looks OK, and it makes sense independent of what else
follows with respect to notmuch-insert.

With respect so SONAME bumping, we might be able to get away with out it
here (although that could be un-portable or broken in a way I haven't
seen yet), but we can also delay that decision until closer to the next
release; I don't think it's as important to provide a stable ABI for
people building from git, and I'd prefer to batch ABI changes into one
SONAME bump.

Re: [PATCH v2 01/10] lib: add return status to database close and destroy

[David Bremner]

Peter Wang <novalazy@gmail.com> writes:

> From: Jani Nikula <jani@nikula.org>
>
> notmuch_database_close may fail in Xapian ->flush() or ->close(), so
> report the status. Similarly for notmuch_database_destroy which calls
> close.

pushed this one patch, along with a minimal NEWS item.

[PATCH v2 02/10] lib: bump soname

[Peter Wang]

Adding return values to notmuch_database_close and
notmuch_database_destroy may require bumping the soname.
---
 lib/Makefile.local | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/Makefile.local b/lib/Makefile.local
index c56cba9..4120390 100644
--- a/lib/Makefile.local
+++ b/lib/Makefile.local
@@ -5,7 +5,7 @@
 # the library interface, (such as the deletion of an API or a major
 # semantic change that breaks formerly functioning code).
 #
-LIBNOTMUCH_VERSION_MAJOR = 3
+LIBNOTMUCH_VERSION_MAJOR = 4
 
 # The minor version of the library interface. This should be incremented at
 # the time of release for any additions to the library interface,
-- 
1.8.4

[PATCH v2 03/10] python: handle return status of database close and destroy

[Peter Wang]

Throw an exception if notmuch_database_close or notmuch_database_destroy
fail.
---
 bindings/python/notmuch/database.py | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/bindings/python/notmuch/database.py b/bindings/python/notmuch/database.py
index 7ddf5cf..5b58e09 100644
--- a/bindings/python/notmuch/database.py
+++ b/bindings/python/notmuch/database.py
@@ -157,11 +157,13 @@ class Database(object):
 
     _destroy = nmlib.notmuch_database_destroy
     _destroy.argtypes = [NotmuchDatabaseP]
-    _destroy.restype = None
+    _destroy.restype = c_uint
 
     def __del__(self):
         if self._db:
-            self._destroy(self._db)
+            status = self._destroy(self._db)
+            if status != STATUS.SUCCESS:
+                raise NotmuchError(status)
 
     def _assert_db_is_initialized(self):
         """Raises :exc:`NotInitializedError` if self._db is `None`"""
@@ -217,7 +219,7 @@ class Database(object):
 
     _close = nmlib.notmuch_database_close
     _close.argtypes = [NotmuchDatabaseP]
-    _close.restype = None
+    _close.restype = c_uint
 
     def close(self):
         '''
@@ -231,7 +233,9 @@ class Database(object):
             NotmuchError.
         '''
         if self._db:
-            self._close(self._db)
+            status = self._close(self._db)
+            if status != STATUS.SUCCESS:
+                raise NotmuchError(status)
 
     def __enter__(self):
         '''
-- 
1.8.4

Re: [PATCH v2 03/10] python: handle return status of database close and destroy

[David Bremner]

Peter Wang <novalazy@gmail.com> writes:

> Throw an exception if notmuch_database_close or notmuch_database_destroy
> fail.
> ---
>  bindings/python/notmuch/database.py | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
>

I don't really feel qualified to review changes to the python bindings.
Justus, are you still following/mainting the python bindings? If so,
what do you think of this patch?

d

[PATCH v2 04/10] go: add return status to database close method

[Peter Wang]

Add return status to the Database.Close() method that calls
notmuch_database_destroy.
---
 bindings/go/src/notmuch/notmuch.go | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/bindings/go/src/notmuch/notmuch.go b/bindings/go/src/notmuch/notmuch.go
index 00bd53a..b9230ad 100644
--- a/bindings/go/src/notmuch/notmuch.go
+++ b/bindings/go/src/notmuch/notmuch.go
@@ -144,8 +144,8 @@ func OpenDatabase(path string, mode DatabaseMode) (*Database, Status) {
 
 /* Close the given notmuch database, freeing all associated
  * resources. See notmuch_database_open. */
-func (self *Database) Close() {
-	C.notmuch_database_destroy(self.db)
+func (self *Database) Close() Status {
+	return Status(C.notmuch_database_destroy(self.db))
 }
 
 /* Return the database path of the given database.
-- 
1.8.4

[PATCH v2 05/10] ruby: handle return status of database close

[Peter Wang]

Throw an exception if notmuch_database_destroy fails.
---
 bindings/ruby/database.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/bindings/ruby/database.c b/bindings/ruby/database.c
index e84f726..c03d701 100644
--- a/bindings/ruby/database.c
+++ b/bindings/ruby/database.c
@@ -113,11 +113,13 @@ notmuch_rb_database_open (int argc, VALUE *argv, VALUE klass)
 VALUE
 notmuch_rb_database_close (VALUE self)
 {
+    notmuch_status_t ret;
     notmuch_database_t *db;
 
     Data_Get_Notmuch_Database (self, db);
-    notmuch_database_destroy (db);
+    ret = notmuch_database_destroy (db);
     DATA_PTR (self) = NULL;
+    notmuch_rb_status_raise (ret);
 
     return Qnil;
 }
-- 
1.8.4

Re: [PATCH v2 05/10] ruby: handle return status of database close

[David Bremner]

Peter Wang <novalazy@gmail.com> writes:

> Throw an exception if notmuch_database_destroy fails.
> ---

Same problem with the go and ruby bindings.

Felipe, any comments on the ruby bindings changes?

Does anyone in particular care for the go bindings? Justus and Austin, I
put you in CC based on git log.

d

[PATCH v2 06/10] cli: refactor insert

[Peter Wang]

Change insert_message into write_message and move its responsibilities
for indexing the message into the main function, to simplify the control
flow.
---
 notmuch-insert.c | 63 +++++++++++++++++++++++++++-----------------------------
 1 file changed, 30 insertions(+), 33 deletions(-)

diff --git a/notmuch-insert.c b/notmuch-insert.c
index 6752fc8..7db4f73 100644
--- a/notmuch-insert.c
+++ b/notmuch-insert.c
@@ -338,59 +338,48 @@ add_file_to_database (notmuch_database_t *notmuch, const char *path,
 }
 
 static notmuch_bool_t
-insert_message (void *ctx, notmuch_database_t *notmuch, int fdin,
-		const char *dir, tag_op_list_t *tag_ops,
-		notmuch_bool_t synchronize_flags)
+write_message (void *ctx, int fdin, const char *dir, char **newpath)
 {
     char *tmppath;
-    char *newpath;
     char *newdir;
     int fdout;
-    char *cleanup_path;
 
-    fdout = maildir_open_tmp_file (ctx, dir, &tmppath, &newpath, &newdir);
+    fdout = maildir_open_tmp_file (ctx, dir, &tmppath, newpath, &newdir);
     if (fdout < 0)
 	return FALSE;
 
-    cleanup_path = tmppath;
-
-    if (! copy_stdin (fdin, fdout))
-	goto FAIL;
+    if (! copy_stdin (fdin, fdout)) {
+	close (fdout);
+	unlink (tmppath);
+	return FALSE;
+    }
 
     if (fsync (fdout) != 0) {
 	fprintf (stderr, "Error: fsync failed: %s\n", strerror (errno));
-	goto FAIL;
+	close (fdout);
+	unlink (tmppath);
+	return FALSE;
     }
 
     close (fdout);
-    fdout = -1;
 
     /* Atomically move the new message file from the Maildir 'tmp' directory
      * to the 'new' directory.  We follow the Dovecot recommendation to
      * simply use rename() instead of link() and unlink().
      * See also: http://wiki.dovecot.org/MailboxFormat/Maildir#Mail_delivery
      */
-    if (rename (tmppath, newpath) != 0) {
+    if (rename (tmppath, *newpath) != 0) {
 	fprintf (stderr, "Error: rename() failed: %s\n", strerror (errno));
-	goto FAIL;
+	unlink (tmppath);
+	return FALSE;
     }
 
-    cleanup_path = newpath;
-
-    if (! sync_dir (newdir))
-	goto FAIL;
-
-    /* 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);
+    if (! sync_dir (newdir)) {
+	unlink (*newpath);
+	return FALSE;
+    }
 
     return TRUE;
-
-  FAIL:
-    if (fdout >= 0)
-	close (fdout);
-    unlink (cleanup_path);
-    return FALSE;
 }
 
 int
@@ -407,9 +396,9 @@ notmuch_insert_command (notmuch_config_t *config, int argc, char *argv[])
     notmuch_bool_t create_folder = FALSE;
     notmuch_bool_t synchronize_flags;
     const char *maildir;
+    char *newpath;
     int opt_index;
     unsigned int i;
-    notmuch_bool_t ret;
 
     notmuch_opt_desc_t options[] = {
 	{ NOTMUCH_OPT_STRING, &folder, "folder", 0, 0 },
@@ -484,10 +473,18 @@ notmuch_insert_command (notmuch_config_t *config, int argc, char *argv[])
 			       NOTMUCH_DATABASE_MODE_READ_WRITE, &notmuch))
 	return EXIT_FAILURE;
 
-    ret = insert_message (config, notmuch, STDIN_FILENO, maildir, tag_ops,
-			  synchronize_flags);
+    /* Write the message to the Maildir new directory. */
+    if (! write_message (config, STDIN_FILENO, maildir, &newpath)) {
+	notmuch_database_destroy (notmuch);
+	return EXIT_FAILURE;
+    }
 
-    notmuch_database_destroy (notmuch);
+    /* 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);
 
-    return ret ? EXIT_SUCCESS : EXIT_FAILURE;
+    notmuch_database_destroy (notmuch);
+    return EXIT_SUCCESS;
 }
-- 
1.8.4

Re: [PATCH v2 06/10] cli: refactor insert

[David Bremner]

Peter Wang <novalazy@gmail.com> writes:

> -    cleanup_path = tmppath;
> -
> -    if (! copy_stdin (fdin, fdout))
> -	goto FAIL;
> +    if (! copy_stdin (fdin, fdout)) {
> +	close (fdout);
> +	unlink (tmppath);
> +	return FALSE;
> +    }

I'm not completely convinced by replacement of the "goto FAIL" with the
multiple returns.  I'd lean to towards being consistent with the notmuch
codebase unless the FAIL block is really horrendous

Is there a good reason to use TRUE and FALSE for return values rather
than EXIT_SUCCESS and EXIT_FAILURE? It seems like the latter would be
overall slightly simpler in notmuch_insert_command.

d

Re: [PATCH v2 06/10] cli: refactor insert

[Peter Wang]

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

On Sat, 05 Jul 2014 10:18:05 -0300, David Bremner <david@tethera.net> wrote:
> Peter Wang <novalazy@gmail.com> writes:
> 
> > -    cleanup_path = tmppath;
> > -
> > -    if (! copy_stdin (fdin, fdout))
> > -	goto FAIL;
> > +    if (! copy_stdin (fdin, fdout)) {
> > +	close (fdout);
> > +	unlink (tmppath);
> > +	return FALSE;
> > +    }
> 
> I'm not completely convinced by replacement of the "goto FAIL" with the
> multiple returns.  I'd lean to towards being consistent with the notmuch
> codebase unless the FAIL block is really horrendous

Eh, when I came back to the code I found it unnecessary convoluted.
However, you can squash in the attached patch if you like.
As an objective measure, the function with the FAIL block is longer.

> 
> Is there a good reason to use TRUE and FALSE for return values rather
> than EXIT_SUCCESS and EXIT_FAILURE? It seems like the latter would be
> overall slightly simpler in notmuch_insert_command.

Not sure what you have in mind.  I think CLI exit codes should be
confined to notmuch_insert_command.

Peter

[-- Attachment #2: 0001-goto-fail.patch --]
[-- Type: text/x-diff, Size: 1805 bytes --]

From be07c53d6d5f22ced723a44f996323d2a982113e Mon Sep 17 00:00:00 2001
From: Peter Wang <novalazy@gmail.com>
Date: Sun, 6 Jul 2014 12:52:26 +1000
Subject: [PATCH] goto fail

---
 notmuch-insert.c | 32 ++++++++++++++++++--------------
 1 file changed, 18 insertions(+), 14 deletions(-)

diff --git a/notmuch-insert.c b/notmuch-insert.c
index 7db4f73..8dfc8bb 100644
--- a/notmuch-insert.c
+++ b/notmuch-insert.c
@@ -342,26 +342,25 @@ write_message (void *ctx, int fdin, const char *dir, char **newpath)
 {
     char *tmppath;
     char *newdir;
+    char *cleanup_path;
     int fdout;
 
     fdout = maildir_open_tmp_file (ctx, dir, &tmppath, newpath, &newdir);
     if (fdout < 0)
 	return FALSE;
 
-    if (! copy_stdin (fdin, fdout)) {
-	close (fdout);
-	unlink (tmppath);
-	return FALSE;
-    }
+    cleanup_path = tmppath;
+
+    if (! copy_stdin (fdin, fdout))
+	goto FAIL;
 
     if (fsync (fdout) != 0) {
 	fprintf (stderr, "Error: fsync failed: %s\n", strerror (errno));
-	close (fdout);
-	unlink (tmppath);
-	return FALSE;
+	goto FAIL;
     }
 
     close (fdout);
+    fdout = -1;
 
     /* Atomically move the new message file from the Maildir 'tmp' directory
      * to the 'new' directory.  We follow the Dovecot recommendation to
@@ -370,16 +369,21 @@ write_message (void *ctx, int fdin, const char *dir, char **newpath)
      */
     if (rename (tmppath, *newpath) != 0) {
 	fprintf (stderr, "Error: rename() failed: %s\n", strerror (errno));
-	unlink (tmppath);
-	return FALSE;
+	goto FAIL;
     }
 
-    if (! sync_dir (newdir)) {
-	unlink (*newpath);
-	return FALSE;
-    }
+    cleanup_path = *newpath;
+
+    if (! sync_dir (newdir))
+	goto FAIL;
 
     return TRUE;
+
+  FAIL:
+    if (fdout >= 0)
+	close (fdout);
+    unlink (cleanup_path);
+    return FALSE;
 }
 
 int
-- 
1.8.4

Re: [PATCH v2 06/10] cli: refactor insert

[David Bremner]

Peter Wang <novalazy@gmail.com> writes:

>> Is there a good reason to use TRUE and FALSE for return values rather
>> than EXIT_SUCCESS and EXIT_FAILURE? It seems like the latter would be
>> overall slightly simpler in notmuch_insert_command.
>
> Not sure what you have in mind.  I think CLI exit codes should be
> confined to notmuch_insert_command.

I guess I was thinking of the convention of using 0 for success; several
of the internal functions in e.g. notmuch-new.c, notmuch-dump.c an d
notmuch-restore.c and notmuch-tag.c (and of course the notmuch lib)
follow this.

d

Re: [PATCH v2 06/10] cli: refactor insert

[David Bremner]

Peter Wang <novalazy@gmail.com> writes:

> On Sat, 05 Jul 2014 10:18:05 -0300, David Bremner <david@tethera.net> wrote:
>> Peter Wang <novalazy@gmail.com> writes:
>> 
>> > -    cleanup_path = tmppath;
>> > -
>> > -    if (! copy_stdin (fdin, fdout))
>> > -	goto FAIL;
>> > +    if (! copy_stdin (fdin, fdout)) {
>> > +	close (fdout);
>> > +	unlink (tmppath);
>> > +	return FALSE;
>> > +    }

I've now pushed up to patch 6 in the series, along with a couple of
patches to deal with SONAME related breakage.

I did end up squashing this patch in.

d

[PATCH v2 07/10] cli: indicate insert failure mode in exit status

[Peter Wang]

Make insert return a different exit code, 2, for failure to write the
message file to disk, and exit code 1 for other errors.
---
 notmuch-insert.c    | 30 ++++++++++++++++++------------
 test/T070-insert.sh |  4 ++--
 2 files changed, 20 insertions(+), 14 deletions(-)

diff --git a/notmuch-insert.c b/notmuch-insert.c
index 7db4f73..29d82c9 100644
--- a/notmuch-insert.c
+++ b/notmuch-insert.c
@@ -28,6 +28,12 @@
 #include <sys/stat.h>
 #include <fcntl.h>
 
+enum {
+    INSERT_EXIT_SUCCESS = 0,
+    INSERT_EXIT_FAILURE = 1,
+    INSERT_EXIT_FAILED_WRITE = 2
+};
+
 static volatile sig_atomic_t interrupted;
 
 static void
@@ -408,7 +414,7 @@ notmuch_insert_command (notmuch_config_t *config, int argc, char *argv[])
 
     opt_index = parse_arguments (argc, argv, options, 1);
     if (opt_index < 0)
-	return EXIT_FAILURE;
+	return INSERT_EXIT_FAILURE;
 
     db_path = notmuch_config_get_database_path (config);
     new_tags = notmuch_config_get_new_tags (config, &new_tags_length);
@@ -417,7 +423,7 @@ notmuch_insert_command (notmuch_config_t *config, int argc, char *argv[])
     tag_ops = tag_op_list_create (config);
     if (tag_ops == NULL) {
 	fprintf (stderr, "Out of memory.\n");
-	return EXIT_FAILURE;
+	return INSERT_EXIT_FAILURE;
     }
     for (i = 0; i < new_tags_length; i++) {
 	const char *error_msg;
@@ -426,20 +432,20 @@ notmuch_insert_command (notmuch_config_t *config, int argc, char *argv[])
 	if (error_msg) {
 	    fprintf (stderr, "Error: tag '%s' in new.tags: %s\n",
 		     new_tags[i],  error_msg);
-	    return EXIT_FAILURE;
+	    return INSERT_EXIT_FAILURE;
 	}
 
 	if (tag_op_list_append (tag_ops, new_tags[i], FALSE))
-	    return EXIT_FAILURE;
+	    return INSERT_EXIT_FAILURE;
     }
 
     if (parse_tag_command_line (config, argc - opt_index, argv + opt_index,
 				&query_string, tag_ops))
-	return EXIT_FAILURE;
+	return INSERT_EXIT_FAILURE;
 
     if (*query_string != '\0') {
 	fprintf (stderr, "Error: unexpected query string: %s\n", query_string);
-	return EXIT_FAILURE;
+	return INSERT_EXIT_FAILURE;
     }
 
     if (folder == NULL) {
@@ -447,17 +453,17 @@ notmuch_insert_command (notmuch_config_t *config, int argc, char *argv[])
     } else {
 	if (! check_folder_name (folder)) {
 	    fprintf (stderr, "Error: bad folder name: %s\n", folder);
-	    return EXIT_FAILURE;
+	    return INSERT_EXIT_FAILURE;
 	}
 	maildir = talloc_asprintf (config, "%s/%s", db_path, folder);
 	if (! maildir) {
 	    fprintf (stderr, "Out of memory\n");
-	    return EXIT_FAILURE;
+	    return INSERT_EXIT_FAILURE;
 	}
 	if (create_folder && ! maildir_create_folder (config, maildir)) {
 	    fprintf (stderr, "Error: creating maildir %s: %s\n",
 		     maildir, strerror (errno));
-	    return EXIT_FAILURE;
+	    return INSERT_EXIT_FAILURE;
 	}
     }
 
@@ -471,12 +477,12 @@ notmuch_insert_command (notmuch_config_t *config, int argc, char *argv[])
 
     if (notmuch_database_open (notmuch_config_get_database_path (config),
 			       NOTMUCH_DATABASE_MODE_READ_WRITE, &notmuch))
-	return EXIT_FAILURE;
+	return INSERT_EXIT_FAILURE;
 
     /* Write the message to the Maildir new directory. */
     if (! write_message (config, STDIN_FILENO, maildir, &newpath)) {
 	notmuch_database_destroy (notmuch);
-	return EXIT_FAILURE;
+	return INSERT_EXIT_FAILED_WRITE;
     }
 
     /* Add the message to the index.
@@ -486,5 +492,5 @@ notmuch_insert_command (notmuch_config_t *config, int argc, char *argv[])
 				    synchronize_flags);
 
     notmuch_database_destroy (notmuch);
-    return EXIT_SUCCESS;
+    return INSERT_EXIT_SUCCESS;
 }
diff --git a/test/T070-insert.sh b/test/T070-insert.sh
index ea9db07..c576efc 100755
--- a/test/T070-insert.sh
+++ b/test/T070-insert.sh
@@ -18,7 +18,7 @@ gen_insert_msg() {
 	"[body]=\"insert-message\""
 }
 
-test_expect_code 1 "Insert zero-length file" \
+test_expect_code 2 "Insert zero-length file" \
     "notmuch insert < /dev/null"
 
 # This test is a proxy for other errors that may occur while trying to
@@ -137,7 +137,7 @@ output=$(notmuch search --output=messages path:Drafts/cur tag:draft NOT tag:unre
 test_expect_equal "$output" "id:$gen_msg_id"
 
 gen_insert_msg
-test_expect_code 1 "Insert message into non-existent folder" \
+test_expect_code 2 "Insert message into non-existent folder" \
     "notmuch insert --folder=nonesuch < $gen_msg_filename"
 
 test_begin_subtest "Insert message, create folder"
-- 
1.8.4

[PATCH v2 08/10] cli: add insert --must-index option

[Peter Wang]

This option causes notmuch insert to fail (with exit code 3) on failure
to index the message, or failure to set the tags on the message, or if
closing (flushing) the database fails.  Failure to sync tags to flags
has no effect.
---
 notmuch-insert.c | 57 +++++++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 40 insertions(+), 17 deletions(-)

diff --git a/notmuch-insert.c b/notmuch-insert.c
index 29d82c9..83257f4 100644
--- a/notmuch-insert.c
+++ b/notmuch-insert.c
@@ -31,7 +31,8 @@
 enum {
     INSERT_EXIT_SUCCESS = 0,
     INSERT_EXIT_FAILURE = 1,
-    INSERT_EXIT_FAILED_WRITE = 2
+    INSERT_EXIT_FAILED_WRITE = 2,
+    INSERT_EXIT_FAILED_INDEX = 3
 };
 
 static volatile sig_atomic_t interrupted;
@@ -298,13 +299,15 @@ copy_stdin (int fdin, int fdout)
 }
 
 /* Add the specified message file to the notmuch database, applying tags.
- * The file is renamed to encode notmuch tags as maildir flags. */
-static void
+ * If synchronize_flags is set then file is renamed to encode notmuch tags as
+ * maildir flags. */
+static notmuch_bool_t
 add_file_to_database (notmuch_database_t *notmuch, const char *path,
 		      tag_op_list_t *tag_ops, notmuch_bool_t synchronize_flags)
 {
     notmuch_message_t *message;
     notmuch_status_t status;
+    notmuch_status_t sync;
 
     status = notmuch_database_add_message (notmuch, path, &message);
     switch (status) {
@@ -324,23 +327,28 @@ add_file_to_database (notmuch_database_t *notmuch, const char *path,
     case NOTMUCH_STATUS_LAST_STATUS:
 	fprintf (stderr, "Error: failed to add `%s' to notmuch database: %s\n",
 		 path, notmuch_status_to_string (status));
-	return;
+	return FALSE;
     }
 
     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");
-	}
+	status = NOTMUCH_STATUS_SUCCESS;
     } else {
-	tag_op_flag_t flags = synchronize_flags ? TAG_FLAG_MAILDIR_SYNC : 0;
+	status = tag_op_list_apply (message, tag_ops, 0);
+    }
 
-	tag_op_list_apply (message, tag_ops, flags);
+    /* Call notmuch_message_tags_to_maildir_flags directly instead of doing it
+     * as part of tag_op_list_apply. For --must-index we want to succeed if
+     * tagging succeeds, but disregard whether synchronizing flags fails. */
+    if (status == NOTMUCH_STATUS_SUCCESS && synchronize_flags) {
+	sync = notmuch_message_tags_to_maildir_flags (message);
+	if (sync != NOTMUCH_STATUS_SUCCESS)
+	    fprintf (stderr, "Error: failed to sync tags to maildir flags\n");
     }
 
     notmuch_message_destroy (message);
+
+    return (status == NOTMUCH_STATUS_SUCCESS);
 }
 
 static notmuch_bool_t
@@ -400,15 +408,19 @@ 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 must_index = FALSE;
     notmuch_bool_t synchronize_flags;
     const char *maildir;
     char *newpath;
     int opt_index;
     unsigned int i;
+    notmuch_bool_t indexed;
+    notmuch_status_t status;
 
     notmuch_opt_desc_t options[] = {
 	{ NOTMUCH_OPT_STRING, &folder, "folder", 0, 0 },
 	{ NOTMUCH_OPT_BOOLEAN, &create_folder, "create-folder", 0, 0 },
+	{ NOTMUCH_OPT_BOOLEAN, &must_index, "must-index", 0, 0 },
 	{ NOTMUCH_OPT_END, 0, 0, 0, 0 }
     };
 
@@ -485,12 +497,23 @@ notmuch_insert_command (notmuch_config_t *config, int argc, char *argv[])
 	return INSERT_EXIT_FAILED_WRITE;
     }
 
-    /* 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,
+    /* Add the message to the index. */
+    indexed = add_file_to_database (notmuch, newpath, tag_ops,
 				    synchronize_flags);
 
-    notmuch_database_destroy (notmuch);
-    return INSERT_EXIT_SUCCESS;
+    /* If must_index is FALSE then succeed as the message is on disk.
+     * Otherwise message indexing and tagging must succeed, and the database
+     * must be flushed. Don't flush the database if there was an earlier
+     * error, so as to abandon the transaction (is there a better way?) */
+    if (! must_index) {
+	notmuch_database_destroy (notmuch);
+	return INSERT_EXIT_SUCCESS;
+    }
+    if (indexed) {
+	status = notmuch_database_destroy (notmuch);
+	if (status == NOTMUCH_STATUS_SUCCESS)
+	    return INSERT_EXIT_SUCCESS;
+    }
+    unlink (newpath);
+    return INSERT_EXIT_FAILED_INDEX;
 }
-- 
1.8.4

Re: [PATCH v2 08/10] cli: add insert --must-index option

[David Bremner]

Peter Wang <novalazy@gmail.com> writes:

> This option causes notmuch insert to fail (with exit code 3) on failure
> to index the message, or failure to set the tags on the message, or if
> closing (flushing) the database fails.  Failure to sync tags to flags
> has no effect.

I don't really understand why it's OK to ignore failure to sync
flags. Can you explain? Or point to a previous discussion if we already
went through this?

d

Re: [PATCH v2 08/10] cli: add insert --must-index option

[Peter Wang]

On Wed, 09 Jul 2014 20:20:17 -0300, David Bremner <david@tethera.net> wrote:
> Peter Wang <novalazy@gmail.com> writes:
> 
> > This option causes notmuch insert to fail (with exit code 3) on failure
> > to index the message, or failure to set the tags on the message, or if
> > closing (flushing) the database fails.  Failure to sync tags to flags
> > has no effect.
> 
> I don't really understand why it's OK to ignore failure to sync
> flags. Can you explain? Or point to a previous discussion if we already
> went through this?

It wasn't really discussed.  Only Mark expressed that he didn't really
mind in id:87hadtxfrr.fsf@qmul.ac.uk

It might be justified that, unlike a failure to index, the message will
still be found though the notmuch interface, with the tags intact.
Running notmuch new will not lose those tags either (I think).
It's only when you view the message through another interface that there
will be an inconsistency in the small subset of tags which can be mapped
to maildir flags.

If we were strict about failure to sync flags, then presumably the
newly-added message would need to be deleted from disk and removed from
the database.  I'm not sure it's worthwhile to avoid that (minor)
inconsistency.

Peter

[PATCH v2 09/10] test: test insert --must-index

[Peter Wang]

Test the insert --must-index option.
---
 test/T070-insert.sh | 28 +++++++++++++++++++++++++---
 1 file changed, 25 insertions(+), 3 deletions(-)

diff --git a/test/T070-insert.sh b/test/T070-insert.sh
index c576efc..4e289c0 100755
--- a/test/T070-insert.sh
+++ b/test/T070-insert.sh
@@ -21,11 +21,20 @@ gen_insert_msg() {
 test_expect_code 2 "Insert zero-length file" \
     "notmuch insert < /dev/null"
 
-# 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 3 "Insert non-message with --must-index on" \
+    "echo bad_message | notmuch insert --must-index"
+
+test_begin_subtest "Non-message file should not exist"
+output=$(find "${MAIL_DIR}/cur" "${MAIL_DIR}/new" "${MAIL_DIR}/tmp" -type f -print | wc -l)
+test_expect_equal "$output" "0"
+
+test_expect_code 0 "Insert non-message with --must-index off" \
     "echo bad_message | notmuch insert"
 
+test_begin_subtest "Non-message file should exist"
+output=$(find "${MAIL_DIR}/cur" "${MAIL_DIR}/new" "${MAIL_DIR}/tmp" -type f -print | wc -l)
+test_expect_equal "$output" "1"
+
 test_begin_subtest "Database empty so far"
 test_expect_equal "0" "`notmuch count --output=messages '*'`"
 
@@ -77,6 +86,19 @@ notmuch insert +custom -unread < "$gen_msg_filename"
 output=$(notmuch search --output=messages tag:custom NOT tag:unread)
 test_expect_equal "$output" "id:$gen_msg_id"
 
+# overlongtag exceeds NOTMUCH_TAG_MAX
+ten=0123456789
+hundred=${ten}${ten}${ten}${ten}${ten}${ten}${ten}${ten}${ten}${ten}
+overlongtag=x${hundred}${hundred}
+gen_insert_msg
+test_expect_code 3 "Tagging fails with --must-index on" \
+    "notmuch insert --must-index +$overlongtag < $gen_msg_filename"
+
+test_begin_subtest "Tagging fails with --must-index off"
+notmuch insert +$overlongtag < "$gen_msg_filename"
+output=$(notmuch search --output=messages id:$gen_msg_id)
+test_expect_equal "$output" "id:$gen_msg_id"
+
 test_begin_subtest "Insert message with default tags stays in new/"
 gen_insert_msg
 notmuch insert < "$gen_msg_filename"
-- 
1.8.4

[PATCH v2 10/10] man: update insert documentation

[Peter Wang]

Add documentation for the insert --must-index option
and failure exit codes.
---
 doc/man1/notmuch-insert.rst | 24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/doc/man1/notmuch-insert.rst b/doc/man1/notmuch-insert.rst
index 2be1a7b..02c516b 100644
--- a/doc/man1/notmuch-insert.rst
+++ b/doc/man1/notmuch-insert.rst
@@ -38,16 +38,28 @@ Supported options for **insert** include
         does not exist. Otherwise the folder must already exist for mail
         delivery to succeed.
 
+    ``--must-index``
+        Succeed only if the message is written to disk, added to the
+        notmuch database, and tagged (unless a duplicate message).
+        Failure to synchronize tags to maildir flags has no effect.
+        Without this option, **insert** succeeds as long as the message
+        is written to disk.
+
 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.
+This command returns exit status 0 on success. On failure, it returns a
+non-zero exit status:
+
+    1
+        General failure code.
+
+    2
+        Failed to write the message to disk.
 
-If the message could not be written to disk then a non-zero exit status
-is returned.
+    3
+        Failed to index or tag the message (unless a duplicate message),
+        and ``--must-index`` was specified.
 
 SEE ALSO
 ========
-- 
1.8.4

Re: [PATCH v2 00/10] add insert --must-index option

[Tomi Ollila]

On Wed, Apr 16 2014, Peter Wang <novalazy@gmail.com> wrote:

> Follow up to id:1374365254-13227-1-git-send-email-novalazy@gmail.com
> The main changes are to take into account failures during
> tagging and flushing of the database.
>
> I took Jani's patch id:1390152046-6509-1-git-send-email-jani@nikula.org
> without modification.
>
> The soname bump is included in case it is required.

I guess it is -- then changing in that file is not enough, lib/notmuch.h
needs to have the same change.

But, would a MINOR value update work -- anyone who needs only 3.1.0
could also work with 3.2.0...

If MINOR update were sufficient then we would not need to add
api changes that supports logging (etc.) to this conversation...

... but anyone interested these changes should also take a look
of the actual changes... :D

Tomi

> The python/go/ruby changes are untested.



>
>
> Jani Nikula (1):
>   lib: add return status to database close and destroy
>
> Peter Wang (9):
>   lib: bump soname
>   python: handle return status of database close and destroy
>   go: add return status to database close method
>   ruby: handle return status of database close
>   cli: refactor insert
>   cli: indicate insert failure mode in exit status
>   cli: add insert --must-index option
>   test: test insert --must-index
>   man: update insert documentation
>
>  bindings/go/src/notmuch/notmuch.go  |   4 +-
>  bindings/python/notmuch/database.py |  12 ++--
>  bindings/ruby/database.c            |   4 +-
>  doc/man1/notmuch-insert.rst         |  24 +++++--
>  lib/Makefile.local                  |   2 +-
>  lib/database.cc                     |  30 ++++++--
>  lib/notmuch.h                       |  15 +++-
>  notmuch-insert.c                    | 134 +++++++++++++++++++++---------------
>  test/T070-insert.sh                 |  32 +++++++--
>  9 files changed, 176 insertions(+), 81 deletions(-)
>
> -- 
> 1.8.4
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch