[PATCH 00/11] notmuch insert updates

[PATCH 00/11] notmuch insert updates

[Jani Nikula]

This series refactors and cleans up insert, improves error handling and
reporting, and adds post-insert hook. I intend to add documentation and
more tests, but the code is ready for review. Also, at least some of the
cleanups and fixes in the beginning of the series could go in without
additional tests or documentation.

BR,
Jani.


Jani Nikula (11):
  lib: actually return failures from
    notmuch_message_tags_to_maildir_flags
  cli/insert: rename check_folder_name to is_valid_folder_name
  cli/insert: move add_file_to_database to a better place
  cli/insert: rename file copy function
  cli/insert: clean up sync_dir
  cli/insert: use a single recursive mkdir function
  cli/insert: abstract temporary filename generation
  cli/insert: rehash file writing functions
  cli/insert: add fail path to add_file_to_database
  cli/insert: require succesful message indexing for success status
  cli/insert: add post-insert hook

 lib/message.cc      |    2 +-
 notmuch-insert.c    |  462 +++++++++++++++++++++++++++++----------------------
 test/T070-insert.sh |    2 +-
 3 files changed, 262 insertions(+), 204 deletions(-)

-- 
1.7.2.5

[PATCH 01/11] lib: actually return failures from notmuch_message_tags_to_maildir_flags

[Jani Nikula]

The function takes great care to preserve the first error status it
encounters, yet fails to return that status to the caller. Fix it.
---
 lib/message.cc |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/lib/message.cc b/lib/message.cc
index 68f7e68..7e82548 100644
--- a/lib/message.cc
+++ b/lib/message.cc
@@ -1497,7 +1497,7 @@ notmuch_message_tags_to_maildir_flags (notmuch_message_t *message)
     talloc_free (to_set);
     talloc_free (to_clear);
 
-    return NOTMUCH_STATUS_SUCCESS;
+    return status;
 }
 
 notmuch_status_t
-- 
1.7.2.5

[PATCH 02/11] cli/insert: rename check_folder_name to is_valid_folder_name

[Jani Nikula]

An "is something" predicate conveys the meaning better. While at it,
improve the function documentation and error message. Besides the
error message change, no functional changes.
---
 notmuch-insert.c |   13 ++++++++-----
 1 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/notmuch-insert.c b/notmuch-insert.c
index 8dfc8bb..770275b 100644
--- a/notmuch-insert.c
+++ b/notmuch-insert.c
@@ -83,10 +83,13 @@ sync_dir (const char *dir)
     return ret;
 }
 
-/* Check the specified folder name does not contain a directory
- * component ".." to prevent writes outside of the Maildir hierarchy. */
+/*
+ * Check the specified folder name does not contain a directory
+ * component ".." to prevent writes outside of the Maildir
+ * hierarchy. Return TRUE on valid folder name, FALSE otherwise.
+ */
 static notmuch_bool_t
-check_folder_name (const char *folder)
+is_valid_folder_name (const char *folder)
 {
     const char *p = folder;
 
@@ -449,8 +452,8 @@ notmuch_insert_command (notmuch_config_t *config, int argc, char *argv[])
     if (folder == NULL) {
 	maildir = db_path;
     } else {
-	if (! check_folder_name (folder)) {
-	    fprintf (stderr, "Error: bad folder name: %s\n", folder);
+	if (! is_valid_folder_name (folder)) {
+	    fprintf (stderr, "Error: invalid folder name: '%s'\n", folder);
 	    return EXIT_FAILURE;
 	}
 	maildir = talloc_asprintf (config, "%s/%s", db_path, folder);
-- 
1.7.2.5

[PATCH 03/11] cli/insert: move add_file_to_database to a better place

[Jani Nikula]

Move add_file_to_database around to keep the filesystem related
functions grouped together, improving readability. No functional
changes.
---
 notmuch-insert.c |   92 +++++++++++++++++++++++++++---------------------------
 1 files changed, 46 insertions(+), 46 deletions(-)

diff --git a/notmuch-insert.c b/notmuch-insert.c
index 770275b..ccb091a 100644
--- a/notmuch-insert.c
+++ b/notmuch-insert.c
@@ -294,52 +294,6 @@ copy_stdin (int fdin, int fdout)
     return (!interrupted && !empty);
 }
 
-/* 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)
-{
-    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");
-	}
-    } else {
-	tag_op_flag_t flags = synchronize_flags ? TAG_FLAG_MAILDIR_SYNC : 0;
-
-	tag_op_list_apply (message, tag_ops, flags);
-    }
-
-    notmuch_message_destroy (message);
-}
-
 static notmuch_bool_t
 write_message (void *ctx, int fdin, const char *dir, char **newpath)
 {
@@ -389,6 +343,52 @@ write_message (void *ctx, int fdin, const char *dir, char **newpath)
     return FALSE;
 }
 
+/* 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)
+{
+    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");
+	}
+    } else {
+	tag_op_flag_t flags = synchronize_flags ? TAG_FLAG_MAILDIR_SYNC : 0;
+
+	tag_op_list_apply (message, tag_ops, flags);
+    }
+
+    notmuch_message_destroy (message);
+}
+
 int
 notmuch_insert_command (notmuch_config_t *config, int argc, char *argv[])
 {
-- 
1.7.2.5

[PATCH 04/11] cli/insert: rename file copy function

[Jani Nikula]

The copying has nothing to do with stdin, so call it copy_fd
instead. While at it, improve documentation and reverse the
parameters, as destination is traditionally the first parameter.
---
 notmuch-insert.c |   11 ++++++-----
 1 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/notmuch-insert.c b/notmuch-insert.c
index ccb091a..5d47806 100644
--- a/notmuch-insert.c
+++ b/notmuch-insert.c
@@ -251,11 +251,12 @@ maildir_open_tmp_file (void *ctx, const char *dir,
     return fd;
 }
 
-/* Copy the contents of standard input (fdin) into fdout.
- * Returns TRUE if a non-empty file was written successfully.
- * Otherwise, return FALSE. */
+/*
+ * Copy fdin to fdout, return TRUE on success, and FALSE on errors and
+ * empty input.
+ */
 static notmuch_bool_t
-copy_stdin (int fdin, int fdout)
+copy_fd (int fdout, int fdin)
 {
     notmuch_bool_t empty = TRUE;
 
@@ -308,7 +309,7 @@ write_message (void *ctx, int fdin, const char *dir, char **newpath)
 
     cleanup_path = tmppath;
 
-    if (! copy_stdin (fdin, fdout))
+    if (! copy_fd (fdout, fdin))
 	goto FAIL;
 
     if (fsync (fdout) != 0) {
-- 
1.7.2.5

[PATCH 05/11] cli/insert: clean up sync_dir

[Jani Nikula]

Clarify the code slightly, improve error messages. Apart from the
error message changes, no functional changes.
---
 notmuch-insert.c |   17 +++++++++--------
 1 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/notmuch-insert.c b/notmuch-insert.c
index 5d47806..7375c54 100644
--- a/notmuch-insert.c
+++ b/notmuch-insert.c
@@ -67,20 +67,21 @@ safe_gethostname (char *hostname, size_t len)
 static notmuch_bool_t
 sync_dir (const char *dir)
 {
-    notmuch_bool_t ret;
-    int fd;
+    int fd, r;
 
     fd = open (dir, O_RDONLY);
     if (fd == -1) {
-	fprintf (stderr, "Error: open() dir failed: %s\n", strerror (errno));
+	fprintf (stderr, "Error: open %s: %s\n", dir, strerror (errno));
 	return FALSE;
     }
-    ret = (fsync (fd) == 0);
-    if (! ret) {
-	fprintf (stderr, "Error: fsync() dir failed: %s\n", strerror (errno));
-    }
+
+    r = fsync (fd);
+    if (r)
+	fprintf (stderr, "Error: fsync %s: %s\n", dir, strerror (errno));
+
     close (fd);
-    return ret;
+
+    return r == 0;
 }
 
 /*
-- 
1.7.2.5

[PATCH 06/11] cli/insert: use a single recursive mkdir function

[Jani Nikula]

Combine make_directory() and make_directory_and_parents() into a
single recursive mkdir_recursive() function. Clarify the code and
improve error handling. Improve error messages. Switch to using the
new function in maildir_create_folder(). Constify talloc context.
---
 notmuch-insert.c |  131 +++++++++++++++++++++++-------------------------------
 1 files changed, 55 insertions(+), 76 deletions(-)

diff --git a/notmuch-insert.c b/notmuch-insert.c
index 7375c54..cdeeb41 100644
--- a/notmuch-insert.c
+++ b/notmuch-insert.c
@@ -104,96 +104,78 @@ is_valid_folder_name (const char *folder)
     }
 }
 
-/* Make the given directory, succeeding if it already exists. */
+/*
+ * Make the given directory and its parents as necessary, using the
+ * given mode. Return TRUE on success, FALSE otherwise. Partial
+ * results are not cleaned up on errors.
+ */
 static notmuch_bool_t
-make_directory (char *path, int mode)
+mkdir_recursive (const void *ctx, const char *path, int mode)
 {
-    notmuch_bool_t ret;
-    char *slash;
+    struct stat st;
+    int r;
+    char *parent = NULL, *slash;
 
-    if (mkdir (path, mode) != 0)
-	return (errno == EEXIST);
+    /* First check the common case: directory already exists. */
+    r = stat (path, &st);
+    if (r == 0) {
+        if (! S_ISDIR (st.st_mode)) {
+	    fprintf (stderr, "Error: '%s' is not a directory: %s\n",
+		     path, strerror (EEXIST));
+	    return FALSE;
+	}
 
-    /* Sync the parent directory for durability. */
-    ret = TRUE;
-    slash = strrchr (path, '/');
-    if (slash) {
-	*slash = '\0';
-	ret = sync_dir (path);
-	*slash = '/';
+	return TRUE;
+    } else if (errno != ENOENT) {
+	fprintf (stderr, "Error: stat '%s': %s\n", path, strerror (errno));
+	return FALSE;
     }
-    return ret;
-}
-
-/* Make the given directory including its parent directories as necessary.
- * Return TRUE on success, FALSE on error. */
-static notmuch_bool_t
-make_directory_and_parents (char *path, int mode)
-{
-    struct stat st;
-    char *start;
-    char *end;
-    notmuch_bool_t ret;
 
-    /* First check the common case: directory already exists. */
-    if (stat (path, &st) == 0)
-	return S_ISDIR (st.st_mode) ? TRUE : FALSE;
-
-    for (start = path; *start != '\0'; start = end + 1) {
-	/* start points to the first unprocessed character.
-	 * Find the next slash from start onwards. */
-	end = strchr (start, '/');
-
-	/* If there are no more slashes then all the parent directories
-	 * have been made.  Now attempt to make the whole path. */
-	if (end == NULL)
-	    return make_directory (path, mode);
-
-	/* Make the path up to the next slash, unless the current
-	 * directory component is actually empty. */
-	if (end > start) {
-	    *end = '\0';
-	    ret = make_directory (path, mode);
-	    *end = '/';
-	    if (! ret)
-		return FALSE;
+    /* mkdir parents, if any */
+    slash = strrchr (path, '/');
+    if (slash && slash != path) {
+	parent = talloc_strndup (ctx, path, slash - path);
+	if (! parent) {
+	    fprintf (stderr, "Error: %s\n", strerror (ENOMEM));
+	    return FALSE;
 	}
+
+	if (! mkdir_recursive (ctx, parent, mode))
+	    return FALSE;
     }
 
-    return TRUE;
+    if (mkdir (path, mode)) {
+	fprintf (stderr, "Error: mkdir '%s': %s\n", path, strerror (errno));
+	return FALSE;
+    }
+
+    return parent ? sync_dir (parent) : TRUE;
 }
 
-/* Create the given maildir folder, i.e. dir and its subdirectories
- * 'cur', 'new', 'tmp'. */
+/*
+ * Create the given maildir folder, i.e. maildir and its
+ * subdirectories cur/new/tmp. Return TRUE on success, FALSE
+ * otherwise. Partial results are not cleaned up on errors.
+ */
 static notmuch_bool_t
-maildir_create_folder (void *ctx, const char *dir)
+maildir_create_folder (const void *ctx, const char *maildir)
 {
+    const char *subdirs[] = { "cur", "new", "tmp" };
     const int mode = 0700;
     char *subdir;
-    char *tail;
-
-    /* Create 'cur' directory, including parent directories. */
-    subdir = talloc_asprintf (ctx, "%s/cur", dir);
-    if (! subdir) {
-	fprintf (stderr, "Out of memory.\n");
-	return FALSE;
-    }
-    if (! make_directory_and_parents (subdir, mode))
-	return FALSE;
-
-    tail = subdir + strlen (subdir) - 3;
+    unsigned int i;
 
-    /* Create 'new' directory. */
-    strcpy (tail, "new");
-    if (! make_directory (subdir, mode))
-	return FALSE;
+    for (i = 0; i < ARRAY_SIZE (subdirs); i++) {
+	subdir = talloc_asprintf (ctx, "%s/%s", maildir, subdirs[i]);
+	if (! subdir) {
+	    fprintf (stderr, "Error: %s\n", strerror (ENOMEM));
+	    return FALSE;
+	}
 
-    /* Create 'tmp' directory. */
-    strcpy (tail, "tmp");
-    if (! make_directory (subdir, mode))
-	return FALSE;
+	if (! mkdir_recursive (ctx, subdir, mode))
+	    return FALSE;
+    }
 
-    talloc_free (subdir);
     return TRUE;
 }
 
@@ -463,11 +445,8 @@ notmuch_insert_command (notmuch_config_t *config, int argc, char *argv[])
 	    fprintf (stderr, "Out of memory\n");
 	    return EXIT_FAILURE;
 	}
-	if (create_folder && ! maildir_create_folder (config, maildir)) {
-	    fprintf (stderr, "Error: creating maildir %s: %s\n",
-		     maildir, strerror (errno));
+	if (create_folder && ! maildir_create_folder (config, maildir))
 	    return EXIT_FAILURE;
-	}
     }
 
     /* Setup our handler for SIGINT. We do not set SA_RESTART so that copying
-- 
1.7.2.5

[PATCH 07/11] cli/insert: abstract temporary filename generation

[Jani Nikula]

This will clean up the usage. There's the slight functional change of
potentially ending up doing extra gethostname and getpid calls, but
this is neglible.
---
 notmuch-insert.c |   39 +++++++++++++++++++++++++++------------
 1 files changed, 27 insertions(+), 12 deletions(-)

diff --git a/notmuch-insert.c b/notmuch-insert.c
index cdeeb41..a1d564c 100644
--- a/notmuch-insert.c
+++ b/notmuch-insert.c
@@ -179,6 +179,31 @@ maildir_create_folder (const void *ctx, const char *maildir)
     return TRUE;
 }
 
+/*
+ * Generate a temporary file basename, no path, do not create an
+ * actual file. Return the basename, or NULL on errors.
+ */
+static char *
+tempfilename (const void *ctx)
+{
+    char *filename;
+    char hostname[256];
+    struct timeval tv;
+    pid_t pid;
+
+    /* We follow the Dovecot file name generation algorithm. */
+    pid = getpid ();
+    safe_gethostname (hostname, sizeof (hostname));
+    gettimeofday (&tv, NULL);
+
+    filename = talloc_asprintf (ctx, "%ld.M%ldP%d.%s",
+				tv.tv_sec, tv.tv_usec, pid, hostname);
+    if (! filename)
+	fprintf (stderr, "Error: %s\n", strerror (ENOMEM));
+
+    return filename;
+}
+
 /* Open a unique file in the 'tmp' sub-directory of dir.
  * Returns the file descriptor on success, or -1 on failure.
  * On success, file paths for the message in the 'tmp' and 'new'
@@ -188,23 +213,13 @@ static int
 maildir_open_tmp_file (void *ctx, const char *dir,
 		       char **tmppath, char **newpath, char **newdir)
 {
-    pid_t pid;
-    char hostname[256];
-    struct timeval tv;
     char *filename;
     int fd = -1;
 
-    /* We follow the Dovecot file name generation algorithm. */
-    pid = getpid ();
-    safe_gethostname (hostname, sizeof (hostname));
     do {
-	gettimeofday (&tv, NULL);
-	filename = talloc_asprintf (ctx, "%ld.M%ldP%d.%s",
-				    tv.tv_sec, tv.tv_usec, pid, hostname);
-	if (! filename) {
-	    fprintf (stderr, "Out of memory\n");
+	filename = tempfilename (ctx);
+	if (! filename)
 	    return -1;
-	}
 
 	*tmppath = talloc_asprintf (ctx, "%s/tmp/%s", dir, filename);
 	if (! *tmppath) {
-- 
1.7.2.5

[PATCH 08/11] cli/insert: rehash file writing functions

[Jani Nikula]

Make the function calls make more sense as independent building blocks
of the big picture, with clear inputs and outputs. Split up
write_message into two. Improve function documentation. Cleanup and
clarify the error paths.
---
 notmuch-insert.c |  127 ++++++++++++++++++++++++++++++++----------------------
 1 files changed, 75 insertions(+), 52 deletions(-)

diff --git a/notmuch-insert.c b/notmuch-insert.c
index a1d564c..5ef6e66 100644
--- a/notmuch-insert.c
+++ b/notmuch-insert.c
@@ -204,47 +204,37 @@ tempfilename (const void *ctx)
     return filename;
 }
 
-/* Open a unique file in the 'tmp' sub-directory of dir.
- * Returns the file descriptor on success, or -1 on failure.
- * On success, file paths for the message in the 'tmp' and 'new'
- * directories are returned via tmppath and newpath,
- * and the path of the 'new' directory itself in newdir. */
+/*
+ * Create a unique temporary file in maildir/tmp, return fd and full
+ * path to file in *path_out, or -1 on errors (in which case *path_out
+ * is not touched).
+ */
 static int
-maildir_open_tmp_file (void *ctx, const char *dir,
-		       char **tmppath, char **newpath, char **newdir)
+maildir_mktemp (const void *ctx, const char *maildir, char **path_out)
 {
-    char *filename;
-    int fd = -1;
+    char *filename, *path;
+    int fd;
 
     do {
 	filename = tempfilename (ctx);
 	if (! filename)
 	    return -1;
 
-	*tmppath = talloc_asprintf (ctx, "%s/tmp/%s", dir, filename);
-	if (! *tmppath) {
-	    fprintf (stderr, "Out of memory\n");
+	path = talloc_asprintf (ctx, "%s/tmp/%s", maildir, filename);
+	if (! path) {
+	    fprintf (stderr, "Error: %s\n", strerror (ENOMEM));
 	    return -1;
 	}
 
-	fd = open (*tmppath, O_WRONLY | O_CREAT | O_TRUNC | O_EXCL, 0600);
+	fd = open (path, O_WRONLY | O_CREAT | O_TRUNC | O_EXCL, 0600);
     } while (fd == -1 && errno == EEXIST);
 
     if (fd == -1) {
-	fprintf (stderr, "Error: opening %s: %s\n", *tmppath, strerror (errno));
+	fprintf (stderr, "Error: open '%s': %s\n", path, strerror (errno));
 	return -1;
     }
 
-    *newdir = talloc_asprintf (ctx, "%s/new", dir);
-    *newpath = talloc_asprintf (ctx, "%s/new/%s", dir, filename);
-    if (! *newdir || ! *newpath) {
-	fprintf (stderr, "Out of memory\n");
-	close (fd);
-	unlink (*tmppath);
-	return -1;
-    }
-
-    talloc_free (filename);
+    *path_out = path;
 
     return fd;
 }
@@ -293,53 +283,85 @@ copy_fd (int fdout, int fdin)
     return (!interrupted && !empty);
 }
 
-static notmuch_bool_t
-write_message (void *ctx, int fdin, const char *dir, char **newpath)
+/*
+ * Write fdin to a new temp file in maildir/tmp, return full path to
+ * the file, or NULL on errors.
+ */
+static char *
+maildir_write_tmp (const void *ctx, int fdin, const char *maildir)
 {
-    char *tmppath;
-    char *newdir;
-    char *cleanup_path;
+    char *path;
     int fdout;
 
-    fdout = maildir_open_tmp_file (ctx, dir, &tmppath, newpath, &newdir);
+    fdout = maildir_mktemp (ctx, maildir, &path);
     if (fdout < 0)
-	return FALSE;
-
-    cleanup_path = tmppath;
+	return NULL;
 
     if (! copy_fd (fdout, fdin))
 	goto FAIL;
 
-    if (fsync (fdout) != 0) {
-	fprintf (stderr, "Error: fsync failed: %s\n", strerror (errno));
+    if (fsync (fdout)) {
+	fprintf (stderr, "Error: fsync '%s': %s\n", path, strerror (errno));
 	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
-     * simply use rename() instead of link() and unlink().
-     * See also: http://wiki.dovecot.org/MailboxFormat/Maildir#Mail_delivery
-     */
-    if (rename (tmppath, *newpath) != 0) {
-	fprintf (stderr, "Error: rename() failed: %s\n", strerror (errno));
+
+    return path;
+
+FAIL:
+    close (fdout);
+    unlink (path);
+
+    return NULL;
+}
+
+/*
+ * Write fdin to a new file in maildir/new, using an intermediate temp
+ * file in maildir/tmp, return full path to the new file, or NULL on
+ * errors.
+ */
+static char *
+maildir_write_new (const void *ctx, int fdin, const char *maildir)
+{
+    char *cleanpath, *tmppath, *newpath, *newdir;
+
+    tmppath = maildir_write_tmp (ctx, fdin, maildir);
+    if (! tmppath)
+	return NULL;
+    cleanpath = tmppath;
+
+    newpath = talloc_strdup (ctx, tmppath);
+    if (! newpath) {
+	fprintf (stderr, "Error: %s\n", strerror (ENOMEM));
 	goto FAIL;
     }
 
-    cleanup_path = *newpath;
+    /* sanity checks needed? */
+    memcpy (newpath + strlen (maildir) + 1, "new", 3);
+
+    if (rename (tmppath, newpath)) {
+	fprintf (stderr, "Error: rename '%s' '%s': %s\n",
+		 tmppath, newpath, strerror (errno));
+	goto FAIL;
+    }
+    cleanpath = newpath;
+
+    newdir = talloc_asprintf (ctx, "%s/%s", maildir, "new");
+    if (! newdir) {
+	fprintf (stderr, "Error: %s\n", strerror (ENOMEM));
+	goto FAIL;
+    }
 
     if (! sync_dir (newdir))
 	goto FAIL;
 
-    return TRUE;
+    return newpath;
+
+FAIL:
+    unlink (cleanpath);
 
-  FAIL:
-    if (fdout >= 0)
-	close (fdout);
-    unlink (cleanup_path);
-    return FALSE;
+    return NULL;
 }
 
 /* Add the specified message file to the notmuch database, applying tags.
@@ -477,7 +499,8 @@ notmuch_insert_command (notmuch_config_t *config, int argc, char *argv[])
 	return EXIT_FAILURE;
 
     /* Write the message to the Maildir new directory. */
-    if (! write_message (config, STDIN_FILENO, maildir, &newpath)) {
+    newpath = maildir_write_new (config, STDIN_FILENO, maildir);
+    if (! newpath) {
 	notmuch_database_destroy (notmuch);
 	return EXIT_FAILURE;
     }
-- 
1.7.2.5

[PATCH 09/11] cli/insert: add fail path to add_file_to_database

[Jani Nikula]

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 |   89 ++++++++++++++++++++++++++++++++---------------------
 1 files changed, 54 insertions(+), 35 deletions(-)

diff --git a/notmuch-insert.c b/notmuch-insert.c
index 5ef6e66..80f52d4 100644
--- a/notmuch-insert.c
+++ b/notmuch-insert.c
@@ -364,50 +364,70 @@ 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);
+	return status;
     } 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));
+	return status;
+    }
 
-	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 will fail.
+	 */
     }
 
+DONE:
     notmuch_message_destroy (message);
+
+    if (status) {
+	if (keep)
+	    status = NOTMUCH_STATUS_SUCCESS;
+	else
+	    notmuch_database_remove_message (notmuch, path);
+    }
+
+    return status;
 }
 
 int
@@ -508,8 +528,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;
-- 
1.7.2.5

Re: [PATCH 09/11] cli/insert: add fail path to add_file_to_database

[David Bremner]

Jani Nikula <jani@nikula.org> writes:


> +	status = notmuch_message_tags_to_maildir_flags (message);

...

> +	/*
> +	 * Note: Unfortunately a failed maildir flag sync might
> +	 * already have renamed the file, in which case the cleanup
> +	 * path will fail.
> +	 */

I'd like to be more explicit about what potentially bad outcomes there
are here.  I guess this message file gets left on disk, unindexed,
perhaps forever if the user never runs notmuch new? Would it make sense
to suggest the user run notmuch new to "recover" these message files?

[PATCH 10/11] cli/insert: require succesful message indexing for success status

[Jani Nikula]

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.
---
 notmuch-insert.c    |   20 +++++++++++++++-----
 test/T070-insert.sh |    2 +-
 2 files changed, 16 insertions(+), 6 deletions(-)

diff --git a/notmuch-insert.c b/notmuch-insert.c
index 80f52d4..f27b9cb 100644
--- a/notmuch-insert.c
+++ b/notmuch-insert.c
@@ -433,6 +433,7 @@ DONE:
 int
 notmuch_insert_command (notmuch_config_t *config, int argc, char *argv[])
 {
+    notmuch_status_t status;
     notmuch_database_t *notmuch;
     struct sigaction action;
     const char *db_path;
@@ -442,6 +443,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;
@@ -451,6 +453,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 }
     };
 
@@ -525,11 +528,18 @@ 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);
+    if (status) {
+	if (keep) {
+	    status = NOTMUCH_STATUS_SUCCESS;
+	} else {
+	    /* If maildir flag sync failed, this might fail. */
+	    unlink (newpath);
+	}
+    }
 
     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 ea9db07..aacc643 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"
-- 
1.7.2.5

[PATCH 11/11] cli/insert: add post-insert hook

[Jani Nikula]

The post-new hook might no longer be needed or run very often if
notmuch insert is being used. Therefore a post-insert hook is needed
(arguably pre-insert not so much, so don't add one). Also add the
--no-hooks option to skip hooks.
---
 notmuch-insert.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/notmuch-insert.c b/notmuch-insert.c
index f27b9cb..adadd12 100644
--- a/notmuch-insert.c
+++ b/notmuch-insert.c
@@ -444,6 +444,7 @@ notmuch_insert_command (notmuch_config_t *config, int argc, char *argv[])
     const char *folder = NULL;
     notmuch_bool_t create_folder = FALSE;
     notmuch_bool_t keep = FALSE;
+    notmuch_bool_t no_hooks = FALSE;
     notmuch_bool_t synchronize_flags;
     const char *maildir;
     char *newpath;
@@ -454,6 +455,7 @@ notmuch_insert_command (notmuch_config_t *config, int argc, char *argv[])
 	{ 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_BOOLEAN,  &no_hooks, "no-hooks", 'n', 0 },
 	{ NOTMUCH_OPT_END, 0, 0, 0, 0 }
     };
 
@@ -541,5 +543,10 @@ notmuch_insert_command (notmuch_config_t *config, int argc, char *argv[])
 
     notmuch_database_destroy (notmuch);
 
+    if (! no_hooks && status == NOTMUCH_STATUS_SUCCESS) {
+	/* Ignore hook failures. */
+	notmuch_run_hook (db_path, "post-insert");
+    }
+
     return status ? EXIT_FAILURE : EXIT_SUCCESS;
 }
-- 
1.7.2.5

Re: [PATCH 00/11] notmuch insert updates

[David Bremner]

Jani Nikula <jani@nikula.org> writes:

> This series refactors and cleans up insert, improves error handling and
> reporting, and adds post-insert hook. I intend to add documentation and
> more tests, but the code is ready for review. Also, at least some of the
> cleanups and fixes in the beginning of the series could go in without
> additional tests or documentation.

The first 4 seem trivial, and I marked them so.

5-8 don't introduce new functionality, and don't break 
any existing tests. Subject to a little more review, these could 
probably be merged now.

5 is almost trivial; it wouldn't hurt to explicitly document the return
values of sync_dir, while touching this code.

6 and 8 seem ok, but should probably have another pair of eyes.

7 seems ok. At first I worried about the usual issue of races between
name definition and open, but it seems like nothing changes here.

Re: [PATCH 00/11] notmuch insert updates

[Tomi Ollila]

On Thu, Sep 25 2014, David Bremner <david@tethera.net> wrote:

> Jani Nikula <jani@nikula.org> writes:
>
>> This series refactors and cleans up insert, improves error handling and
>> reporting, and adds post-insert hook. I intend to add documentation and
>> more tests, but the code is ready for review. Also, at least some of the
>> cleanups and fixes in the beginning of the series could go in without
>> additional tests or documentation.
>
> The first 4 seem trivial, and I marked them so.
>
> 5-8 don't introduce new functionality, and don't break 
> any existing tests. Subject to a little more review, these could 
> probably be merged now.
>
> 5 is almost trivial; it wouldn't hurt to explicitly document the return
> values of sync_dir, while touching this code.
>
> 6 and 8 seem ok, but should probably have another pair of eyes.
>
> 7 seems ok. At first I worried about the usual issue of races between
> name definition and open, but it seems like nothing changes here.

5-8 in this series LGTM.

Tomi

Re: [PATCH 00/11] notmuch insert updates

[David Bremner]

Tomi Ollila <tomi.ollila@iki.fi> writes:

e.

> 5-8 in this series LGTM.
>

I pushed 1-8 in this series.

d