[PATCH v3 00/20] insert command

[PATCH v3 00/20] insert command

[Peter Wang]

This mainly addresses review comments from v2
and rebases on top of tag-util.c.

Peter Wang (20):
  cli: add stub for insert command
  insert: open Maildir tmp file
  insert: copy stdin to Maildir tmp file
  insert: move file from Maildir tmp to new
  insert: add new message to database
  insert: apply default tags to new message
  tag-util: do not reset list in parse_tag_command_line
  tag-util: move out 'tag' command-line checks
  insert: parse and apply command-line tag operations
  insert: support --folder option
  insert: prevent writes outside Maildir hierarchy
  insert: add --create-folder option
  insert: fsync after writing tmp file
  insert: fsync new directory after rename
  insert: fsync parent directory after mkdir
  insert: trap SIGINT and clean up
  insert: add copyright line from notmuch-deliver
  man: document 'insert' command
  man: reference notmuch-insert.1
  test: add tests for insert

 Makefile.local                  |   1 +
 man/Makefile.local              |   1 +
 man/man1/notmuch-config.1       |   4 +-
 man/man1/notmuch-count.1        |   4 +-
 man/man1/notmuch-dump.1         |   4 +-
 man/man1/notmuch-insert.1       |  59 +++++
 man/man1/notmuch-new.1          |   4 +-
 man/man1/notmuch-reply.1        |   3 +-
 man/man1/notmuch-restore.1      |   3 +-
 man/man1/notmuch-search.1       |   3 +-
 man/man1/notmuch-show.1         |   3 +-
 man/man1/notmuch-tag.1          |   3 +-
 man/man1/notmuch.1              |   3 +-
 man/man5/notmuch-hooks.5        |   4 +-
 man/man7/notmuch-search-terms.7 |   3 +-
 notmuch-client.h                |   3 +
 notmuch-insert.c                | 482 ++++++++++++++++++++++++++++++++++++++++
 notmuch-tag.c                   |  10 +
 notmuch.c                       |   3 +
 tag-util.c                      |  12 -
 tag-util.h                      |   2 +
 test/insert                     | 106 +++++++++
 test/notmuch-test               |   1 +
 23 files changed, 692 insertions(+), 29 deletions(-)
 create mode 100644 man/man1/notmuch-insert.1
 create mode 100644 notmuch-insert.c
 create mode 100755 test/insert

-- 
1.7.12.1

[PATCH v3 01/20] cli: add stub for insert command

[Peter Wang]

The notmuch insert command should read a message from standard input
and deliver it to a Maildir folder, and then incorporate the message
into the notmuch database.  Essentially it moves the functionality of
notmuch-deliver into notmuch.

Though it could be used as an alternative to notmuch new, the reason
I want this is to allow my notmuch frontend to add postponed or sent
messages to the mail store and notmuch database, without resorting to
another tool (e.g. notmuch-deliver) nor directly modifying the maildir.
---
 Makefile.local   |  1 +
 notmuch-client.h |  3 +++
 notmuch-insert.c | 43 +++++++++++++++++++++++++++++++++++++++++++
 notmuch.c        |  3 +++
 4 files changed, 50 insertions(+)
 create mode 100644 notmuch-insert.c

diff --git a/Makefile.local b/Makefile.local
index c274f07..bb2381d 100644
--- a/Makefile.local
+++ b/Makefile.local
@@ -261,6 +261,7 @@ notmuch_client_srcs =		\
 	notmuch-config.c	\
 	notmuch-count.c		\
 	notmuch-dump.c		\
+	notmuch-insert.c	\
 	notmuch-new.c		\
 	notmuch-reply.c		\
 	notmuch-restore.c	\
diff --git a/notmuch-client.h b/notmuch-client.h
index 5f28836..af7d094 100644
--- a/notmuch-client.h
+++ b/notmuch-client.h
@@ -175,6 +175,9 @@ int
 notmuch_dump_command (void *ctx, int argc, char *argv[]);
 
 int
+notmuch_insert_command (void *ctx, int argc, char *argv[]);
+
+int
 notmuch_new_command (void *ctx, int argc, char *argv[]);
 
 int
diff --git a/notmuch-insert.c b/notmuch-insert.c
new file mode 100644
index 0000000..c1d359c
--- /dev/null
+++ b/notmuch-insert.c
@@ -0,0 +1,43 @@
+/* notmuch - Not much of an email program, (just index and search)
+ *
+ * Copyright © 2013 Peter Wang
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see http://www.gnu.org/licenses/ .
+ *
+ * Author: Peter Wang <novalazy@gmail.com>
+ */
+
+#include "notmuch-client.h"
+
+int
+notmuch_insert_command (void *ctx, int argc, char *argv[])
+{
+    notmuch_config_t *config;
+    notmuch_database_t *notmuch;
+    const char *db_path;
+
+    config = notmuch_config_open (ctx, NULL, NULL);
+    if (config == NULL)
+	return 1;
+
+    db_path = notmuch_config_get_database_path (config);
+
+    if (notmuch_database_open (notmuch_config_get_database_path (config),
+			       NOTMUCH_DATABASE_MODE_READ_WRITE, &notmuch))
+	return 1;
+
+    notmuch_database_destroy (notmuch);
+
+    return 1;
+}
diff --git a/notmuch.c b/notmuch.c
index 4fc0973..1c3b893 100644
--- a/notmuch.c
+++ b/notmuch.c
@@ -53,6 +53,9 @@ static command_t commands[] = {
     { "new", notmuch_new_command,
       "[options...]",
       "Find and import new messages to the notmuch database." },
+    { "insert", notmuch_insert_command,
+      "[options...] [--] [+<tag>|-<tag> ...] < message",
+      "Add a new message into the maildir and notmuch database." },
     { "search", notmuch_search_command,
       "[options...] <search-terms> [...]",
       "Search for messages matching the given search terms." },
-- 
1.7.12.1

[PATCH v3 02/20] insert: open Maildir tmp file

[Peter Wang]

Open a unique file in the Maildir tmp directory.
The new message is not yet copied into the file.
---
 notmuch-insert.c | 105 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 104 insertions(+), 1 deletion(-)

diff --git a/notmuch-insert.c b/notmuch-insert.c
index c1d359c..7d100ae 100644
--- a/notmuch-insert.c
+++ b/notmuch-insert.c
@@ -20,12 +20,107 @@
 
 #include "notmuch-client.h"
 
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <fcntl.h>
+
+/* Like gethostname but guarantees that a null-terminated hostname is
+ * returned, even if it has to make one up.
+ * Returns true unless hostname contains a slash. */
+static notmuch_bool_t
+safe_gethostname (char *hostname, size_t len)
+{
+    if (gethostname (hostname, len) == -1) {
+	strncpy (hostname, "unknown", len);
+    }
+    hostname[len - 1] = '\0';
+
+    return (strchr (hostname, '/') == NULL);
+}
+
+/* Open a unique file in the Maildir 'tmp' directory.
+ * 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. */
+static int
+maildir_open_tmp_file (void *ctx, const char *dir,
+		       char **tmppath, char **newpath)
+{
+    pid_t pid;
+    char hostname[256];
+    struct timeval tv;
+    char *filename;
+    int fd = -1;
+
+    /* We follow the Dovecot file name generation algorithm. */
+    pid = getpid ();
+    if (! safe_gethostname (hostname, sizeof (hostname))) {
+	fprintf (stderr, "Error: invalid host name.\n");
+	return -1;
+    }
+    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");
+	    return -1;
+	}
+
+	*tmppath = talloc_asprintf (ctx, "%s/tmp/%s", dir, filename);
+	if (! *tmppath) {
+	    fprintf (stderr, "Out of memory\n");
+	    return -1;
+	}
+
+	fd = open (*tmppath, 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));
+	return -1;
+    }
+
+    *newpath = talloc_asprintf (ctx, "%s/new/%s", dir, filename);
+    if (! *newpath) {
+	fprintf (stderr, "Out of memory\n");
+	close (fd);
+	unlink (*tmppath);
+	return -1;
+    }
+
+    talloc_free (filename);
+
+    return fd;
+}
+
+static notmuch_bool_t
+insert_message (void *ctx, notmuch_database_t *notmuch, int fdin,
+	        const char *dir)
+{
+    char *tmppath;
+    char *newpath;
+    int fdout;
+
+    fdout = maildir_open_tmp_file (ctx, dir, &tmppath, &newpath);
+    if (fdout < 0) {
+	return FALSE;
+    }
+
+    /* For now we just delete the tmp file immediately. */
+    close (fdout);
+    unlink (tmppath);
+    return FALSE;
+}
+
 int
 notmuch_insert_command (void *ctx, int argc, char *argv[])
 {
     notmuch_config_t *config;
     notmuch_database_t *notmuch;
     const char *db_path;
+    char *maildir;
+    notmuch_bool_t ret;
 
     config = notmuch_config_open (ctx, NULL, NULL);
     if (config == NULL)
@@ -33,11 +128,19 @@ notmuch_insert_command (void *ctx, int argc, char *argv[])
 
     db_path = notmuch_config_get_database_path (config);
 
+    maildir = talloc_asprintf (ctx, "%s", db_path);
+    if (! maildir) {
+	fprintf (stderr, "Out of memory\n");
+	return 1;
+    }
+
     if (notmuch_database_open (notmuch_config_get_database_path (config),
 			       NOTMUCH_DATABASE_MODE_READ_WRITE, &notmuch))
 	return 1;
 
+    ret = insert_message (ctx, notmuch, STDIN_FILENO, maildir);
+
     notmuch_database_destroy (notmuch);
 
-    return 1;
+    return (ret) ? 0 : 1;
 }
-- 
1.7.12.1

[PATCH v3 03/20] insert: copy stdin to Maildir tmp file

[Peter Wang]

Read the new message from standard input into the Maildir tmp file.
---
 notmuch-insert.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 47 insertions(+), 4 deletions(-)

diff --git a/notmuch-insert.c b/notmuch-insert.c
index 7d100ae..38e815f 100644
--- a/notmuch-insert.c
+++ b/notmuch-insert.c
@@ -94,6 +94,47 @@ maildir_open_tmp_file (void *ctx, const char *dir,
     return fd;
 }
 
+/* Copy the contents of standard input (fdin) into fdout. */
+static notmuch_bool_t
+copy_stdin (int fdin, int fdout)
+{
+    char buf[4096];
+    char *p;
+    ssize_t remain;
+    ssize_t written;
+
+    for (;;) {
+	remain = read (fdin, buf, sizeof (buf));
+	if (remain == 0)
+	    break;
+	if (remain < 0) {
+	    if (errno == EINTR)
+		continue;
+	    fprintf (stderr, "Error: reading from standard input: %s\n",
+		     strerror (errno));
+	    return FALSE;
+	}
+
+	p = buf;
+	do {
+	    written = write (fdout, p, remain);
+	    if (written == 0)
+		return FALSE;
+	    if (written < 0) {
+		if (errno == EINTR)
+		    continue;
+		fprintf (stderr, "Error: writing to temporary file: %s",
+			 strerror (errno));
+		return FALSE;
+	    }
+	    p += written;
+	    remain -= written;
+	} while (remain > 0);
+    }
+
+    return TRUE;
+}
+
 static notmuch_bool_t
 insert_message (void *ctx, notmuch_database_t *notmuch, int fdin,
 	        const char *dir)
@@ -101,16 +142,18 @@ insert_message (void *ctx, notmuch_database_t *notmuch, int fdin,
     char *tmppath;
     char *newpath;
     int fdout;
+    notmuch_bool_t ret;
 
     fdout = maildir_open_tmp_file (ctx, dir, &tmppath, &newpath);
     if (fdout < 0) {
 	return FALSE;
     }
-
-    /* For now we just delete the tmp file immediately. */
+    ret = copy_stdin (fdin, fdout);
     close (fdout);
-    unlink (tmppath);
-    return FALSE;
+    if (!ret) {
+	unlink (tmppath);
+    }
+    return ret;
 }
 
 int
-- 
1.7.12.1

[PATCH v3 04/20] insert: move file from Maildir tmp to new

[Peter Wang]

Atomically move the new message file from the Maildir 'tmp' directory
to 'new'.
---
 notmuch-insert.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/notmuch-insert.c b/notmuch-insert.c
index 38e815f..c0289d9 100644
--- a/notmuch-insert.c
+++ b/notmuch-insert.c
@@ -94,6 +94,24 @@ maildir_open_tmp_file (void *ctx, const char *dir,
     return fd;
 }
 
+/* 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
+ */
+static notmuch_bool_t
+maildir_move_tmp_to_new (const char *tmppath, const char *newpath)
+{
+    if (rename (tmppath, newpath) != 0) {
+	fprintf (stderr, "Error: rename() failed: %s\n", strerror (errno));
+	return FALSE;
+    }
+
+    return TRUE;
+}
+
 /* Copy the contents of standard input (fdin) into fdout. */
 static notmuch_bool_t
 copy_stdin (int fdin, int fdout)
@@ -150,6 +168,9 @@ insert_message (void *ctx, notmuch_database_t *notmuch, int fdin,
     }
     ret = copy_stdin (fdin, fdout);
     close (fdout);
+    if (ret) {
+	ret = maildir_move_tmp_to_new (tmppath, newpath);
+    }
     if (!ret) {
 	unlink (tmppath);
     }
-- 
1.7.12.1

[PATCH v3 05/20] insert: add new message to database

[Peter Wang]

Add the new message to the notmuch database, renaming the file to encode
notmuch tags as maildir flags.
---
 notmuch-insert.c | 49 ++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 48 insertions(+), 1 deletion(-)

diff --git a/notmuch-insert.c b/notmuch-insert.c
index c0289d9..498421d 100644
--- a/notmuch-insert.c
+++ b/notmuch-insert.c
@@ -153,6 +153,44 @@ copy_stdin (int fdin, int fdout)
     return TRUE;
 }
 
+/* Add the specified message file to the notmuch database.
+ * The 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)
+{
+    notmuch_message_t *message;
+    notmuch_status_t status;
+
+    status = notmuch_database_add_message (notmuch, path, &message);
+    switch (status) {
+    case NOTMUCH_STATUS_SUCCESS:
+	break;
+    case NOTMUCH_STATUS_DUPLICATE_MESSAGE_ID:
+	fprintf (stderr, "Warning: duplicate message.\n");
+	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 FALSE;
+    }
+
+    notmuch_message_tags_to_maildir_flags (message);
+
+    notmuch_message_destroy (message);
+
+    return TRUE;
+}
+
 static notmuch_bool_t
 insert_message (void *ctx, notmuch_database_t *notmuch, int fdin,
 	        const char *dir)
@@ -173,8 +211,17 @@ insert_message (void *ctx, notmuch_database_t *notmuch, int fdin,
     }
     if (!ret) {
 	unlink (tmppath);
+	return FALSE;
     }
-    return ret;
+
+    ret = add_file_to_database (notmuch, newpath);
+    if (!ret) {
+	/* XXX maybe there should be an option to keep the file in maildir? */
+	unlink (newpath);
+	return FALSE;
+    }
+
+    return TRUE;
 }
 
 int
-- 
1.7.12.1

[PATCH v3 06/20] insert: apply default tags to new message

[Peter Wang]

Apply the new.tags to messages added by 'insert'.  This mirrors the
behaviour if the message were delivered by a separate tool followed by
'notmuch new'.
---
 notmuch-insert.c | 29 +++++++++++++++++++++++------
 1 file changed, 23 insertions(+), 6 deletions(-)

diff --git a/notmuch-insert.c b/notmuch-insert.c
index 498421d..8388d07 100644
--- a/notmuch-insert.c
+++ b/notmuch-insert.c
@@ -19,6 +19,7 @@
  */
 
 #include "notmuch-client.h"
+#include "tag-util.h"
 
 #include <sys/types.h>
 #include <sys/stat.h>
@@ -153,10 +154,11 @@ copy_stdin (int fdin, int fdout)
     return TRUE;
 }
 
-/* Add the specified message file to the notmuch database.
+/* Add the specified message file to the notmuch database, applying tags.
  * The 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)
+add_file_to_database (notmuch_database_t *notmuch, const char *path,
+		      tag_op_list_t *tag_ops)
 {
     notmuch_message_t *message;
     notmuch_status_t status;
@@ -184,7 +186,7 @@ add_file_to_database (notmuch_database_t *notmuch, const char *path)
 	return FALSE;
     }
 
-    notmuch_message_tags_to_maildir_flags (message);
+    tag_op_list_apply (message, tag_ops, TAG_FLAG_MAILDIR_SYNC);
 
     notmuch_message_destroy (message);
 
@@ -193,7 +195,7 @@ 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)
+		const char *dir, tag_op_list_t *tag_ops)
 {
     char *tmppath;
     char *newpath;
@@ -214,7 +216,7 @@ insert_message (void *ctx, notmuch_database_t *notmuch, int fdin,
 	return FALSE;
     }
 
-    ret = add_file_to_database (notmuch, newpath);
+    ret = add_file_to_database (notmuch, newpath, tag_ops);
     if (!ret) {
 	/* XXX maybe there should be an option to keep the file in maildir? */
 	unlink (newpath);
@@ -230,7 +232,11 @@ notmuch_insert_command (void *ctx, int argc, char *argv[])
     notmuch_config_t *config;
     notmuch_database_t *notmuch;
     const char *db_path;
+    const char **new_tags;
+    size_t new_tags_length;
+    tag_op_list_t *tag_ops;
     char *maildir;
+    unsigned int i;
     notmuch_bool_t ret;
 
     config = notmuch_config_open (ctx, NULL, NULL);
@@ -238,6 +244,17 @@ notmuch_insert_command (void *ctx, int argc, char *argv[])
 	return 1;
 
     db_path = notmuch_config_get_database_path (config);
+    new_tags = notmuch_config_get_new_tags (config, &new_tags_length);
+
+    tag_ops = tag_op_list_create (ctx);
+    if (tag_ops == NULL) {
+	fprintf (stderr, "Out of memory.\n");
+	return 1;
+    }
+    for (i = 0; i < new_tags_length; i++) {
+	if (tag_op_list_append (tag_ops, new_tags[i], FALSE))
+	    return 1;
+    }
 
     maildir = talloc_asprintf (ctx, "%s", db_path);
     if (! maildir) {
@@ -249,7 +266,7 @@ notmuch_insert_command (void *ctx, int argc, char *argv[])
 			       NOTMUCH_DATABASE_MODE_READ_WRITE, &notmuch))
 	return 1;
 
-    ret = insert_message (ctx, notmuch, STDIN_FILENO, maildir);
+    ret = insert_message (ctx, notmuch, STDIN_FILENO, maildir, tag_ops);
 
     notmuch_database_destroy (notmuch);
 
-- 
1.7.12.1

[PATCH v3 07/20] tag-util: do not reset list in parse_tag_command_line

[Peter Wang]

No current callers of parse_tag_command_line require that it clear its
tag list argument.  The notmuch 'insert' command will be better served
if the function modifies a pre-populated list (of new.tags) instead of
clobbering it outright.
---
 tag-util.c | 2 --
 tag-util.h | 2 ++
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/tag-util.c b/tag-util.c
index 701d329..3f9da05 100644
--- a/tag-util.c
+++ b/tag-util.c
@@ -165,8 +165,6 @@ parse_tag_command_line (void *ctx, int argc, char **argv,
 
     int i;
 
-    tag_op_list_reset (tag_ops);
-
     for (i = 0; i < argc; i++) {
 	if (strcmp (argv[i], "--") == 0) {
 	    i++;
diff --git a/tag-util.h b/tag-util.h
index 246de85..4628f16 100644
--- a/tag-util.h
+++ b/tag-util.h
@@ -81,6 +81,8 @@ parse_tag_line (void *ctx, char *line,
  * Output Parameters:
  *	ops	contains a list of tag operations
  *	query_str the search terms.
+ *
+ * The ops argument is not cleared.
  */
 
 tag_parse_status_t
-- 
1.7.12.1

[PATCH v3 08/20] tag-util: move out 'tag' command-line checks

[Peter Wang]

parse_tag_command_line checked for two error conditions which are
specific to the 'tag' command.  It can be reused for the notmuch
'insert' command if we move the checks out, into notmuch-tag.c.
---
 notmuch-tag.c | 10 ++++++++++
 tag-util.c    | 10 ----------
 2 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/notmuch-tag.c b/notmuch-tag.c
index d9daf8f..a901dad 100644
--- a/notmuch-tag.c
+++ b/notmuch-tag.c
@@ -234,6 +234,16 @@ notmuch_tag_command (void *ctx, int argc, char *argv[])
 	if (parse_tag_command_line (ctx, argc - opt_index, argv + opt_index,
 				    &query_string, tag_ops))
 	    return 1;
+
+	if (tag_op_list_size (tag_ops) == 0) {
+	    fprintf (stderr, "Error: 'notmuch tag' requires at least one tag to add or remove.\n");
+	    return 1;
+	}
+
+	if (*query_string == '\0') {
+	    fprintf (stderr, "Error: notmuch tag requires at least one search term.\n");
+	    return 1;
+	}
     }
 
     config = notmuch_config_open (ctx, NULL, NULL);
diff --git a/tag-util.c b/tag-util.c
index 3f9da05..41f2c09 100644
--- a/tag-util.c
+++ b/tag-util.c
@@ -186,18 +186,8 @@ parse_tag_command_line (void *ctx, int argc, char **argv,
 	tag_op_list_append (tag_ops, argv[i] + 1, is_remove);
     }
 
-    if (tag_op_list_size (tag_ops) == 0) {
-	fprintf (stderr, "Error: 'notmuch tag' requires at least one tag to add or remove.\n");
-	return TAG_PARSE_INVALID;
-    }
-
     *query_str = query_string_from_args (ctx, argc - i, &argv[i]);
 
-    if (*query_str == NULL || **query_str == '\0') {
-	fprintf (stderr, "Error: notmuch tag requires at least one search term.\n");
-	return TAG_PARSE_INVALID;
-    }
-
     return TAG_PARSE_SUCCESS;
 }
 
-- 
1.7.12.1

[PATCH v3 09/20] insert: parse and apply command-line tag operations

[Peter Wang]

Parse +tag and -tag operations on the 'insert' command-line
and apply them to the inserted message in addition to new.tags.
---
 notmuch-insert.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/notmuch-insert.c b/notmuch-insert.c
index 8388d07..b0ac174 100644
--- a/notmuch-insert.c
+++ b/notmuch-insert.c
@@ -235,7 +235,9 @@ notmuch_insert_command (void *ctx, int argc, char *argv[])
     const char **new_tags;
     size_t new_tags_length;
     tag_op_list_t *tag_ops;
+    char *query_string = NULL;
     char *maildir;
+    int opt_index = 1;
     unsigned int i;
     notmuch_bool_t ret;
 
@@ -256,6 +258,15 @@ notmuch_insert_command (void *ctx, int argc, char *argv[])
 	    return 1;
     }
 
+    if (parse_tag_command_line (ctx, argc - opt_index, argv + opt_index,
+				&query_string, tag_ops))
+	return 1;
+
+    if (*query_string != '\0') {
+	fprintf (stderr, "Error: unexpected query string: %s\n", query_string);
+	return 1;
+    }
+
     maildir = talloc_asprintf (ctx, "%s", db_path);
     if (! maildir) {
 	fprintf (stderr, "Out of memory\n");
-- 
1.7.12.1

[PATCH v3 10/20] insert: support --folder option

[Peter Wang]

Allow the new message to be inserted into a folder within the Maildir
hierarchy instead of the top-level folder.
---
 notmuch-insert.c | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/notmuch-insert.c b/notmuch-insert.c
index b0ac174..ba8cf5a 100644
--- a/notmuch-insert.c
+++ b/notmuch-insert.c
@@ -236,11 +236,25 @@ notmuch_insert_command (void *ctx, int argc, char *argv[])
     size_t new_tags_length;
     tag_op_list_t *tag_ops;
     char *query_string = NULL;
+    const char *folder = NULL;
     char *maildir;
-    int opt_index = 1;
+    int opt_index;
     unsigned int i;
     notmuch_bool_t ret;
 
+    notmuch_opt_desc_t options[] = {
+	{ NOTMUCH_OPT_STRING, &folder, "folder", 0, 0 },
+	{ NOTMUCH_OPT_END, 0, 0, 0, 0 }
+    };
+
+    opt_index = parse_arguments (argc, argv, options, 1);
+
+    if (opt_index < 0) {
+	fprintf (stderr, "Error: bad argument to notmuch insert: %s\n",
+		 argv[-opt_index]);
+	return 1;
+    }
+
     config = notmuch_config_open (ctx, NULL, NULL);
     if (config == NULL)
 	return 1;
@@ -267,7 +281,11 @@ notmuch_insert_command (void *ctx, int argc, char *argv[])
 	return 1;
     }
 
-    maildir = talloc_asprintf (ctx, "%s", db_path);
+    if (folder != NULL) {
+	maildir = talloc_asprintf (ctx, "%s/%s", db_path, folder);
+    } else {
+	maildir = talloc_asprintf (ctx, "%s", db_path);
+    }
     if (! maildir) {
 	fprintf (stderr, "Out of memory\n");
 	return 1;
-- 
1.7.12.1

[PATCH v3 11/20] insert: prevent writes outside Maildir hierarchy

[Peter Wang]

Don't accept a --folder name that contains a ".." component,
in order to prevent writing outside of the Maildir hierarchy.
---
 notmuch-insert.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/notmuch-insert.c b/notmuch-insert.c
index ba8cf5a..67ef94a 100644
--- a/notmuch-insert.c
+++ b/notmuch-insert.c
@@ -39,6 +39,23 @@ safe_gethostname (char *hostname, size_t len)
     return (strchr (hostname, '/') == NULL);
 }
 
+/* Check the specified folder name does not contain a directory
+ * component ".." to prevent writes outside of the Maildir hierarchy. */
+static notmuch_bool_t
+check_folder_name (const char *folder)
+{
+    const char *p = folder;
+
+    for (;;) {
+	if ((p[0] == '.') && (p[1] == '.') && (p[2] == '\0' || p[2] == '/'))
+	    return FALSE;
+	p = strchr (p, '/');
+	if (!p)
+	    return TRUE;
+	p++;
+    }
+}
+
 /* Open a unique file in the Maildir 'tmp' directory.
  * Returns the file descriptor on success, or -1 on failure.
  * On success, file paths for the message in the 'tmp' and 'new'
@@ -282,6 +299,10 @@ notmuch_insert_command (void *ctx, int argc, char *argv[])
     }
 
     if (folder != NULL) {
+	if (! check_folder_name (folder)) {
+	    fprintf (stderr, "Error: bad folder name: %s\n", folder);
+	    return 1;
+	}
 	maildir = talloc_asprintf (ctx, "%s/%s", db_path, folder);
     } else {
 	maildir = talloc_asprintf (ctx, "%s", db_path);
-- 
1.7.12.1

[PATCH v3 12/20] insert: add --create-folder option

[Peter Wang]

Support an option to create a new folder in the maildir.
---
 notmuch-insert.c | 88 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 88 insertions(+)

diff --git a/notmuch-insert.c b/notmuch-insert.c
index 67ef94a..85acaea 100644
--- a/notmuch-insert.c
+++ b/notmuch-insert.c
@@ -56,6 +56,87 @@ check_folder_name (const char *folder)
     }
 }
 
+/* Make the given directory, succeeding if it already exists. */
+static notmuch_bool_t
+make_directory (char *path, int mode)
+{
+    if (mkdir (path, mode) != 0)
+	return (errno == EEXIST);
+    return TRUE;
+}
+
+/* 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;
+	}
+    }
+
+    return TRUE;
+}
+
+/* Create the given maildir folder, i.e. dir and its subdirectories
+ * 'cur', 'new', 'tmp'. */
+static notmuch_bool_t
+maildir_create_folder (void *ctx, const char *dir)
+{
+    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;
+
+    /* Create 'new' directory. */
+    strcpy (tail, "new");
+    if (! make_directory (subdir, mode))
+	return FALSE;
+
+    /* Create 'tmp' directory. */
+    strcpy (tail, "tmp");
+    if (! make_directory (subdir, mode))
+	return FALSE;
+
+    talloc_free (subdir);
+    return TRUE;
+}
+
 /* Open a unique file in the Maildir 'tmp' directory.
  * Returns the file descriptor on success, or -1 on failure.
  * On success, file paths for the message in the 'tmp' and 'new'
@@ -254,6 +335,7 @@ notmuch_insert_command (void *ctx, int argc, char *argv[])
     tag_op_list_t *tag_ops;
     char *query_string = NULL;
     const char *folder = NULL;
+    notmuch_bool_t create_folder = FALSE;
     char *maildir;
     int opt_index;
     unsigned int i;
@@ -261,6 +343,7 @@ notmuch_insert_command (void *ctx, 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_END, 0, 0, 0, 0 }
     };
 
@@ -311,6 +394,11 @@ notmuch_insert_command (void *ctx, int argc, char *argv[])
 	fprintf (stderr, "Out of memory\n");
 	return 1;
     }
+    if (create_folder && ! maildir_create_folder (ctx, maildir)) {
+	fprintf (stderr, "Error: creating maildir %s: %s\n",
+		 maildir, strerror (errno));
+	return 1;
+    }
 
     if (notmuch_database_open (notmuch_config_get_database_path (config),
 			       NOTMUCH_DATABASE_MODE_READ_WRITE, &notmuch))
-- 
1.7.12.1

[PATCH v3 13/20] insert: fsync after writing tmp file

[Peter Wang]

Flush the tmp file to disk after writing for durability.
---
 notmuch-insert.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/notmuch-insert.c b/notmuch-insert.c
index 85acaea..60855c1 100644
--- a/notmuch-insert.c
+++ b/notmuch-insert.c
@@ -305,6 +305,10 @@ insert_message (void *ctx, notmuch_database_t *notmuch, int fdin,
 	return FALSE;
     }
     ret = copy_stdin (fdin, fdout);
+    if (ret && fsync (fdout) != 0) {
+	fprintf (stderr, "Error: fsync failed: %s\n", strerror (errno));
+	ret = FALSE;
+    }
     close (fdout);
     if (ret) {
 	ret = maildir_move_tmp_to_new (tmppath, newpath);
-- 
1.7.12.1

[PATCH v3 14/20] insert: fsync new directory after rename

[Peter Wang]

After moving the file from the 'tmp' to the 'new' directory,
fsync on the 'new' directory for durability.
---
 notmuch-insert.c | 41 ++++++++++++++++++++++++++++++++++-------
 1 file changed, 34 insertions(+), 7 deletions(-)

diff --git a/notmuch-insert.c b/notmuch-insert.c
index 60855c1..c4d5e75 100644
--- a/notmuch-insert.c
+++ b/notmuch-insert.c
@@ -56,6 +56,28 @@ check_folder_name (const char *folder)
     }
 }
 
+/* Call fsync() on a directory path. */
+static notmuch_bool_t
+sync_dir (const char *dir)
+{
+    notmuch_bool_t ret;
+    int fd;
+
+    ret = TRUE;
+    fd = open (dir, O_RDONLY);
+    if (fd == -1) {
+	fprintf (stderr, "Error: open() dir failed: %s\n", strerror (errno));
+	ret = FALSE;
+    }
+    if (ret && fsync (fd) != 0) {
+	fprintf (stderr, "Error: fsync() dir failed: %s\n", strerror (errno));
+	ret = FALSE;
+    }
+    if (fd != -1)
+	close (fd);
+    return ret;
+}
+
 /* Make the given directory, succeeding if it already exists. */
 static notmuch_bool_t
 make_directory (char *path, int mode)
@@ -140,10 +162,11 @@ maildir_create_folder (void *ctx, const char *dir)
 /* Open a unique file in the Maildir 'tmp' directory.
  * 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. */
+ * directories are returned via tmppath and newpath,
+ * and the path of the 'new' directory itself in newdir. */
 static int
 maildir_open_tmp_file (void *ctx, const char *dir,
-		       char **tmppath, char **newpath)
+		       char **tmppath, char **newpath, char **newdir)
 {
     pid_t pid;
     char hostname[256];
@@ -180,8 +203,9 @@ maildir_open_tmp_file (void *ctx, const char *dir,
 	return -1;
     }
 
+    *newdir = talloc_asprintf (ctx, "%s/new", dir);
     *newpath = talloc_asprintf (ctx, "%s/new/%s", dir, filename);
-    if (! *newpath) {
+    if (! *newdir || ! *newpath) {
 	fprintf (stderr, "Out of memory\n");
 	close (fd);
 	unlink (*tmppath);
@@ -201,14 +225,16 @@ maildir_open_tmp_file (void *ctx, const char *dir,
  * http://wiki.dovecot.org/MailboxFormat/Maildir#Mail_delivery
  */
 static notmuch_bool_t
-maildir_move_tmp_to_new (const char *tmppath, const char *newpath)
+maildir_move_tmp_to_new (const char *tmppath, const char *newpath,
+		         const char *newdir)
 {
     if (rename (tmppath, newpath) != 0) {
 	fprintf (stderr, "Error: rename() failed: %s\n", strerror (errno));
 	return FALSE;
     }
 
-    return TRUE;
+    /* Sync the 'new' directory after rename for durability. */
+    return sync_dir (newdir);
 }
 
 /* Copy the contents of standard input (fdin) into fdout. */
@@ -297,10 +323,11 @@ insert_message (void *ctx, notmuch_database_t *notmuch, int fdin,
 {
     char *tmppath;
     char *newpath;
+    char *newdir;
     int fdout;
     notmuch_bool_t ret;
 
-    fdout = maildir_open_tmp_file (ctx, dir, &tmppath, &newpath);
+    fdout = maildir_open_tmp_file (ctx, dir, &tmppath, &newpath, &newdir);
     if (fdout < 0) {
 	return FALSE;
     }
@@ -311,7 +338,7 @@ insert_message (void *ctx, notmuch_database_t *notmuch, int fdin,
     }
     close (fdout);
     if (ret) {
-	ret = maildir_move_tmp_to_new (tmppath, newpath);
+	ret = maildir_move_tmp_to_new (tmppath, newpath, newdir);
     }
     if (!ret) {
 	unlink (tmppath);
-- 
1.7.12.1

[PATCH v3 15/20] insert: fsync parent directory after mkdir

[Peter Wang]

After creating a subdirectory, fsync on its parent directory for
durability.
---
 notmuch-insert.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/notmuch-insert.c b/notmuch-insert.c
index c4d5e75..8012eb4 100644
--- a/notmuch-insert.c
+++ b/notmuch-insert.c
@@ -82,9 +82,21 @@ sync_dir (const char *dir)
 static notmuch_bool_t
 make_directory (char *path, int mode)
 {
+    notmuch_bool_t ret;
+    char *slash;
+
     if (mkdir (path, mode) != 0)
 	return (errno == EEXIST);
-    return TRUE;
+
+    /* Sync the parent directory for durability. */
+    ret = TRUE;
+    slash = strrchr (path, '/');
+    if (slash) {
+	*slash = '\0';
+	ret = sync_dir (path);
+	*slash = '/';
+    }
+    return ret;
 }
 
 /* Make the given directory including its parent directories as necessary.
-- 
1.7.12.1

[PATCH v3 16/20] insert: trap SIGINT and clean up

[Peter Wang]

The only potentially long-running part of the 'insert' command should be
copying stdin to the 'tmp' file.  If SIGINT is received during the
copying process, abort and clean up the file in 'tmp'.  At all other
points, just ignore the signal and continue.
---
 notmuch-insert.c | 28 ++++++++++++++++++++++++++--
 1 file changed, 26 insertions(+), 2 deletions(-)

diff --git a/notmuch-insert.c b/notmuch-insert.c
index 8012eb4..494a7b0 100644
--- a/notmuch-insert.c
+++ b/notmuch-insert.c
@@ -25,6 +25,21 @@
 #include <sys/stat.h>
 #include <fcntl.h>
 
+static volatile sig_atomic_t interrupted;
+
+static void
+handle_sigint (unused (int sig))
+{
+    static char msg[] = "Stopping...         \n";
+
+    /* This write is "opportunistic", so it's okay to ignore the
+     * result.  It is not required for correctness, and if it does
+     * fail or produce a short write, we want to get out of the signal
+     * handler as quickly as possible, not retry it. */
+    IGNORE_RESULT (write (2, msg, sizeof (msg) - 1));
+    interrupted = 1;
+}
+
 /* Like gethostname but guarantees that a null-terminated hostname is
  * returned, even if it has to make one up.
  * Returns true unless hostname contains a slash. */
@@ -258,7 +273,7 @@ copy_stdin (int fdin, int fdout)
     ssize_t remain;
     ssize_t written;
 
-    for (;;) {
+    while (! interrupted) {
 	remain = read (fdin, buf, sizeof (buf));
 	if (remain == 0)
 	    break;
@@ -287,7 +302,7 @@ copy_stdin (int fdin, int fdout)
 	} while (remain > 0);
     }
 
-    return TRUE;
+    return ! interrupted;
 }
 
 /* Add the specified message file to the notmuch database, applying tags.
@@ -372,6 +387,7 @@ notmuch_insert_command (void *ctx, int argc, char *argv[])
 {
     notmuch_config_t *config;
     notmuch_database_t *notmuch;
+    struct sigaction action;
     const char *db_path;
     const char **new_tags;
     size_t new_tags_length;
@@ -443,6 +459,14 @@ notmuch_insert_command (void *ctx, int argc, char *argv[])
 	return 1;
     }
 
+    /* Setup our handler for SIGINT. We do not set SA_RESTART so that copying
+     * from standard input may be interrupted. */
+    memset (&action, 0, sizeof (struct sigaction));
+    action.sa_handler = handle_sigint;
+    sigemptyset (&action.sa_mask);
+    action.sa_flags = 0;
+    sigaction (SIGINT, &action, NULL);
+
     if (notmuch_database_open (notmuch_config_get_database_path (config),
 			       NOTMUCH_DATABASE_MODE_READ_WRITE, &notmuch))
 	return 1;
-- 
1.7.12.1

[PATCH v3 17/20] insert: add copyright line from notmuch-deliver

[Peter Wang]

The 'insert' implementation was based partly on notmuch-deliver.
---
 notmuch-insert.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/notmuch-insert.c b/notmuch-insert.c
index 494a7b0..6b3e380 100644
--- a/notmuch-insert.c
+++ b/notmuch-insert.c
@@ -2,6 +2,9 @@
  *
  * Copyright © 2013 Peter Wang
  *
+ * Based in part on notmuch-deliver
+ * Copyright © 2010 Ali Polatel
+ *
  * This program is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
  * the Free Software Foundation, either version 3 of the License, or
-- 
1.7.12.1

[PATCH v3 18/20] man: document 'insert' command

[Peter Wang]

Add initial documentation for notmuch insert command.
---
 man/Makefile.local        |  1 +
 man/man1/notmuch-insert.1 | 59 +++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 60 insertions(+)
 create mode 100644 man/man1/notmuch-insert.1

diff --git a/man/Makefile.local b/man/Makefile.local
index 72e2a18..216aaa0 100644
--- a/man/Makefile.local
+++ b/man/Makefile.local
@@ -12,6 +12,7 @@ MAN1 := \
 	$(dir)/man1/notmuch-count.1 \
 	$(dir)/man1/notmuch-dump.1 \
 	$(dir)/man1/notmuch-restore.1 \
+	$(dir)/man1/notmuch-insert.1 \
 	$(dir)/man1/notmuch-new.1 \
 	$(dir)/man1/notmuch-reply.1 \
 	$(dir)/man1/notmuch-search.1 \
diff --git a/man/man1/notmuch-insert.1 b/man/man1/notmuch-insert.1
new file mode 100644
index 0000000..4a7cbeb
--- /dev/null
+++ b/man/man1/notmuch-insert.1
@@ -0,0 +1,59 @@
+.TH NOTMUCH-INSERT 1 2013-xx-xx "Notmuch 0.xx"
+.SH NAME
+notmuch-insert \- add a message to the maildir and notmuch database
+.SH SYNOPSIS
+
+.B notmuch insert
+.RI "[" options "]"
+.RI "[ +<" tag> "|\-<" tag "> ... ]"
+
+.SH DESCRIPTION
+
+.B notmuch insert
+reads a message from standard input
+and delivers it to the specified maildir folder,
+then incorporates the message into the notmuch database.
+It is an alternative to using a separate tool to deliver
+the message then running
+.B notmuch new
+afterwards.
+
+The new message will be tagged with the tags specified by the
+.B new.tags
+configuration option, then by operations specified on the command-line:
+tags prefixed by '+' are added while
+those prefixed by '\-' are removed.
+
+Option arguments must appear before any tag operation arguments.
+Supported options for
+.B insert
+include
+.RS 4
+.TP 4
+.BI "--folder=<" folder ">"
+
+Deliver the message to the specified folder,
+relative to the top-level directory given by the value of
+\fBdatabase.path\fR.
+The default is to deliver to the top-level directory.
+
+.RE
+
+.RS 4
+.TP 4
+.B "--create-folder"
+
+Try to create the folder named by the
+.B "--folder"
+option, if it does not exist.
+Otherwise the folder must already exist for mail
+delivery to succeed.
+
+.RE
+.SH SEE ALSO
+
+\fBnotmuch\fR(1), \fBnotmuch-config\fR(1), \fBnotmuch-count\fR(1),
+\fBnotmuch-dump\fR(1), \fBnotmuch-hooks\fR(5), \fBnotmuch-reply\fR(1),
+\fBnotmuch-restore\fR(1), \fBnotmuch-search\fR(1),
+\fBnotmuch-search-terms\fR(7), \fBnotmuch-show\fR(1),
+\fBnotmuch-tag\fR(1)
-- 
1.7.12.1

[PATCH v3 19/20] man: reference notmuch-insert.1

[Peter Wang]

Add references to notmuch-insert.1 from other man pages.
---
 man/man1/notmuch-config.1       | 4 ++--
 man/man1/notmuch-count.1        | 4 ++--
 man/man1/notmuch-dump.1         | 4 ++--
 man/man1/notmuch-new.1          | 4 ++--
 man/man1/notmuch-reply.1        | 3 ++-
 man/man1/notmuch-restore.1      | 3 ++-
 man/man1/notmuch-search.1       | 3 ++-
 man/man1/notmuch-show.1         | 3 ++-
 man/man1/notmuch-tag.1          | 3 ++-
 man/man1/notmuch.1              | 3 ++-
 man/man5/notmuch-hooks.5        | 4 ++--
 man/man7/notmuch-search-terms.7 | 3 ++-
 12 files changed, 24 insertions(+), 17 deletions(-)

diff --git a/man/man1/notmuch-config.1 b/man/man1/notmuch-config.1
index 557eae5..e6d960d 100644
--- a/man/man1/notmuch-config.1
+++ b/man/man1/notmuch-config.1
@@ -152,7 +152,7 @@ use ${HOME}/.notmuch\-config if this variable is not set.
 .SH SEE ALSO
 
 \fBnotmuch\fR(1), \fBnotmuch-count\fR(1), \fBnotmuch-dump\fR(1),
-\fBnotmuch-hooks\fR(5), \fBnotmuch-new\fR(1), \fBnotmuch-reply\fR(1),
-\fBnotmuch-restore\fR(1), \fBnotmuch-search\fR(1),
+\fBnotmuch-hooks\fR(5), \fBnotmuch-insert\fR(1), \fBnotmuch-new\fR(1),
+\fBnotmuch-reply\fR(1), \fBnotmuch-restore\fR(1), \fBnotmuch-search\fR(1),
 \fBnotmuch-search-terms\fR(7), \fBnotmuch-show\fR(1),
 \fBnotmuch-tag\fR(1)
diff --git a/man/man1/notmuch-count.1 b/man/man1/notmuch-count.1
index d63be99..2651412 100644
--- a/man/man1/notmuch-count.1
+++ b/man/man1/notmuch-count.1
@@ -52,7 +52,7 @@ count (the default) or not.
 .SH SEE ALSO
 
 \fBnotmuch\fR(1), \fBnotmuch-config\fR(1), \fBnotmuch-dump\fR(1),
-\fBnotmuch-hooks\fR(5), \fBnotmuch-new\fR(1), \fBnotmuch-reply\fR(1),
-\fBnotmuch-restore\fR(1), \fBnotmuch-search\fR(1),
+\fBnotmuch-hooks\fR(5), \fBnotmuch-insert\fR(1), \fBnotmuch-new\fR(1),
+\fBnotmuch-reply\fR(1), \fBnotmuch-restore\fR(1), \fBnotmuch-search\fR(1),
 \fBnotmuch-search-terms\fR(7), \fBnotmuch-show\fR(1),
 \fBnotmuch-tag\fR(1)
diff --git a/man/man1/notmuch-dump.1 b/man/man1/notmuch-dump.1
index 613fd69..5e58cf9 100644
--- a/man/man1/notmuch-dump.1
+++ b/man/man1/notmuch-dump.1
@@ -92,7 +92,7 @@ for details of the supported syntax for <search-terms>.
 .SH SEE ALSO
 
 \fBnotmuch\fR(1), \fBnotmuch-config\fR(1), \fBnotmuch-count\fR(1),
-\fBnotmuch-hooks\fR(5), \fBnotmuch-new\fR(1), \fBnotmuch-reply\fR(1),
-\fBnotmuch-restore\fR(1), \fBnotmuch-search\fR(1),
+\fBnotmuch-hooks\fR(5), \fBnotmuch-insert\fR(1), \fBnotmuch-new\fR(1),
+\fBnotmuch-reply\fR(1), \fBnotmuch-restore\fR(1), \fBnotmuch-search\fR(1),
 \fBnotmuch-search-terms\fR(7), \fBnotmuch-show\fR(1),
 \fBnotmuch-tag\fR(1)
diff --git a/man/man1/notmuch-new.1 b/man/man1/notmuch-new.1
index 06c4dfa..e0ede8b 100644
--- a/man/man1/notmuch-new.1
+++ b/man/man1/notmuch-new.1
@@ -64,7 +64,7 @@ Prevents hooks from being run.
 .SH SEE ALSO
 
 \fBnotmuch\fR(1), \fBnotmuch-config\fR(1), \fBnotmuch-count\fR(1),
-\fBnotmuch-dump\fR(1), \fBnotmuch-hooks\fR(5), \fBnotmuch-reply\fR(1),
-\fBnotmuch-restore\fR(1), \fBnotmuch-search\fR(1),
+\fBnotmuch-dump\fR(1), \fBnotmuch-hooks\fR(5), \fBnotmuch-insert\fR(1),
+\fBnotmuch-reply\fR(1), \fBnotmuch-restore\fR(1), \fBnotmuch-search\fR(1),
 \fBnotmuch-search-terms\fR(7), \fBnotmuch-show\fR(1),
 \fBnotmuch-tag\fR(1)
diff --git a/man/man1/notmuch-reply.1 b/man/man1/notmuch-reply.1
index 13e50ad..82437a4 100644
--- a/man/man1/notmuch-reply.1
+++ b/man/man1/notmuch-reply.1
@@ -123,7 +123,8 @@ The requested format version is too new.
 .SH SEE ALSO
 
 \fBnotmuch\fR(1), \fBnotmuch-config\fR(1), \fBnotmuch-count\fR(1),
-\fBnotmuch-dump\fR(1), \fBnotmuch-hooks\fR(5), \fBnotmuch-new\fR(1),
+\fBnotmuch-dump\fR(1), \fBnotmuch-hooks\fR(5),
+\fBnotmuch-insert\fR(1), \fBnotmuch-new\fR(1),
 \fBnotmuch-restore\fR(1), \fBnotmuch-search\fR(1),
 \fBnotmuch-search-terms\fR(7), \fBnotmuch-show\fR(1),
 \fBnotmuch-tag\fR(1)
diff --git a/man/man1/notmuch-restore.1 b/man/man1/notmuch-restore.1
index 895c6d2..65ecfc8 100644
--- a/man/man1/notmuch-restore.1
+++ b/man/man1/notmuch-restore.1
@@ -84,7 +84,8 @@ should be accurate.
 .SH SEE ALSO
 
 \fBnotmuch\fR(1), \fBnotmuch-config\fR(1), \fBnotmuch-count\fR(1),
-\fBnotmuch-dump\fR(1), \fBnotmuch-hooks\fR(5), \fBnotmuch-new\fR(1),
+\fBnotmuch-dump\fR(1), \fBnotmuch-hooks\fR(5),
+\fBnotmuch-insert\fR(1), \fBnotmuch-new\fR(1),
 \fBnotmuch-reply\fR(1), \fBnotmuch-search\fR(1),
 \fBnotmuch-search-terms\fR(7), \fBnotmuch-show\fR(1),
 \fBnotmuch-tag\fR(1)
diff --git a/man/man1/notmuch-search.1 b/man/man1/notmuch-search.1
index 321d779..67fd34b 100644
--- a/man/man1/notmuch-search.1
+++ b/man/man1/notmuch-search.1
@@ -155,7 +155,8 @@ The requested format version is too new.
 .SH SEE ALSO
 
 \fBnotmuch\fR(1), \fBnotmuch-config\fR(1), \fBnotmuch-count\fR(1),
-\fBnotmuch-dump\fR(1), \fBnotmuch-hooks\fR(5), \fBnotmuch-new\fR(1),
+\fBnotmuch-dump\fR(1), \fBnotmuch-hooks\fR(5),
+\fBnotmuch-insert\fR(1), \fBnotmuch-new\fR(1),
 \fBnotmuch-reply\fR(1), \fBnotmuch-restore\fR(1),
 \fBnotmuch-search-terms\fR(7), \fBnotmuch-show\fR(1),
 \fBnotmuch-tag\fR(1)
diff --git a/man/man1/notmuch-show.1 b/man/man1/notmuch-show.1
index 3f9584b..90c2556 100644
--- a/man/man1/notmuch-show.1
+++ b/man/man1/notmuch-show.1
@@ -224,7 +224,8 @@ The requested format version is too new.
 .SH SEE ALSO
 
 \fBnotmuch\fR(1), \fBnotmuch-config\fR(1), \fBnotmuch-count\fR(1),
-\fBnotmuch-dump\fR(1), \fBnotmuch-hooks\fR(5), \fBnotmuch-new\fR(1),
+\fBnotmuch-dump\fR(1), \fBnotmuch-hooks\fR(5),
+\fBnotmuch-insert\fR(1), \fBnotmuch-new\fR(1),
 \fBnotmuch-reply\fR(1), \fBnotmuch-restore\fR(1),
 \fBnotmuch-search\fR(1), \fBnotmuch-search-terms\fR(7),
 \fBnotmuch-tag\fR(1)
diff --git a/man/man1/notmuch-tag.1 b/man/man1/notmuch-tag.1
index 081bd0e..14a9856 100644
--- a/man/man1/notmuch-tag.1
+++ b/man/man1/notmuch-tag.1
@@ -125,7 +125,8 @@ of the tag
 .SH SEE ALSO
 
 \fBnotmuch\fR(1), \fBnotmuch-config\fR(1), \fBnotmuch-count\fR(1),
-\fBnotmuch-dump\fR(1), \fBnotmuch-hooks\fR(5), \fBnotmuch-new\fR(1),
+\fBnotmuch-dump\fR(1), \fBnotmuch-hooks\fR(5),
+\fBnotmuch-insert\fR(1), \fBnotmuch-new\fR(1),
 \fBnotmuch-reply\fR(1), \fBnotmuch-restore\fR(1),
 \fBnotmuch-search\fR(1), \fBnotmuch-search-terms\fR(7),
 \fBnotmuch-show\fR(1),
diff --git a/man/man1/notmuch.1 b/man/man1/notmuch.1
index 69805cb..b5829fc 100644
--- a/man/man1/notmuch.1
+++ b/man/man1/notmuch.1
@@ -130,7 +130,8 @@ use ${HOME}/.notmuch\-config if this variable is not set.
 .SH SEE ALSO
 
 \fBnotmuch-config\fR(1), \fBnotmuch-count\fR(1),
-\fBnotmuch-dump\fR(1), \fBnotmuch-hooks\fR(5), \fBnotmuch-new\fR(1),
+\fBnotmuch-dump\fR(1), \fBnotmuch-hooks\fR(5),
+\fBnotmuch-insert\fR(1), \fBnotmuch-new\fR(1),
 \fBnotmuch-reply\fR(1), \fBnotmuch-restore\fR(1),
 \fBnotmuch-search\fR(1), \fBnotmuch-search-terms\fR(7),
 \fBnotmuch-show\fR(1), \fBnotmuch-tag\fR(1)
diff --git a/man/man5/notmuch-hooks.5 b/man/man5/notmuch-hooks.5
index 4ed61c1..97c45e6 100644
--- a/man/man5/notmuch-hooks.5
+++ b/man/man5/notmuch-hooks.5
@@ -42,7 +42,7 @@ imported messages.
 .SH SEE ALSO
 
 \fBnotmuch\fR(1), \fBnotmuch-config\fR(1), \fBnotmuch-count\fR(1),
-\fBnotmuch-dump\fR(1), \fBnotmuch-new\fR(1), \fBnotmuch-reply\fR(1),
-\fBnotmuch-restore\fR(1), \fBnotmuch-search\fR(1),
+\fBnotmuch-dump\fR(1), \fBnotmuch-insert\fR(1), \fBnotmuch-new\fR(1),
+\fBnotmuch-reply\fR(1), \fBnotmuch-restore\fR(1), \fBnotmuch-search\fR(1),
 \fBnotmuch-search-terms\fR(7), \fBnotmuch-show\fR(1),
 \fBnotmuch-tag\fR(1)
diff --git a/man/man7/notmuch-search-terms.7 b/man/man7/notmuch-search-terms.7
index 19e5fd5..45fb525 100644
--- a/man/man7/notmuch-search-terms.7
+++ b/man/man7/notmuch-search-terms.7
@@ -261,6 +261,7 @@ Some time zone codes, e.g. UTC, EET.
 .SH SEE ALSO
 
 \fBnotmuch\fR(1), \fBnotmuch-config\fR(1), \fBnotmuch-count\fR(1),
-\fBnotmuch-dump\fR(1), \fBnotmuch-hooks\fR(5), \fBnotmuch-new\fR(1),
+\fBnotmuch-dump\fR(1), \fBnotmuch-hooks\fR(5),
+\fBnotmuch-insert\fR(1), \fBnotmuch-new\fR(1),
 \fBnotmuch-reply\fR(1), \fBnotmuch-restore\fR(1),
 \fBnotmuch-search\fR(1), \fBnotmuch-show\fR(1), \fBnotmuch-tag\fR(1)
-- 
1.7.12.1

[PATCH v3 20/20] test: add tests for insert

[Peter Wang]

Add tests for new 'insert' command.
---
 test/insert       | 106 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 test/notmuch-test |   1 +
 2 files changed, 107 insertions(+)
 create mode 100755 test/insert

diff --git a/test/insert b/test/insert
new file mode 100755
index 0000000..a3b6283
--- /dev/null
+++ b/test/insert
@@ -0,0 +1,106 @@
+#!/usr/bin/env bash
+test_description='"notmuch insert"'
+. ./test-lib.sh
+
+# Create directories and database before inserting.
+mkdir -p "$MAIL_DIR"/{cur,new,tmp}
+mkdir -p "$MAIL_DIR"/Drafts/{cur,new,tmp}
+notmuch new > /dev/null
+
+# We use generate_message to create the temporary message files.
+# They happen to be in the mail directory already but that is okay
+# since we do not call notmuch new hereafter.
+
+gen_insert_msg() {
+    generate_message \
+	"[subject]=\"insert-subject\"" \
+	"[date]=\"Sat, 01 Jan 2000 12:00:00 -0000\"" \
+	"[body]=\"insert-message\""
+}
+
+test_begin_subtest "Insert message, copied exactly"
+gen_insert_msg
+notmuch insert < "$gen_msg_filename"
+cur_msg_filename=$(notmuch search --output=files "subject:insert-subject")
+test_expect_equal_file "$cur_msg_filename" "$gen_msg_filename"
+
+test_begin_subtest "Insert message, default tags"
+output=$(notmuch show --format=json "subject:insert-subject")
+expected='[[[{
+ "id": "'"${gen_msg_id}"'",
+ "match": true,
+ "excluded": false,
+ "filename": "'"${cur_msg_filename}"'",
+ "timestamp": 946728000,
+ "date_relative": "2000-01-01",
+ "tags": ["inbox","unread"],
+ "headers": {
+  "Subject": "insert-subject",
+  "From": "Notmuch Test Suite <test_suite@notmuchmail.org>",
+  "To": "Notmuch Test Suite <test_suite@notmuchmail.org>",
+  "Date": "Sat, 01 Jan 2000 12:00:00 +0000"},
+ "body": [{"id": 1,
+  "content-type": "text/plain",
+  "content": "insert-message\n"}]},
+ []]]]'
+test_expect_equal_json "$output" "$expected"
+
+test_begin_subtest "Insert message, duplicate message"
+notmuch insert < "$gen_msg_filename"
+output=$(notmuch search --output=files "subject:insert-subject" | wc -l)
+test_expect_equal "$output" 2
+
+test_begin_subtest "Insert message, add tag"
+gen_insert_msg
+notmuch insert +custom < "$gen_msg_filename"
+output=$(notmuch count tag:custom)
+test_expect_equal "$output" "1"
+
+test_begin_subtest "Insert message, add/remove tag"
+gen_insert_msg
+notmuch insert +custom -unread < "$gen_msg_filename"
+output=$(notmuch count tag:custom NOT tag:unread)
+test_expect_equal "$output" "1"
+
+test_begin_subtest "Insert message, folder"
+gen_insert_msg
+notmuch insert --folder=Drafts < "$gen_msg_filename"
+output=$(notmuch search --output=files folder:Drafts)
+dirname=$(dirname "$output")
+test_expect_equal "$dirname" "$MAIL_DIR/Drafts/cur"
+
+test_begin_subtest "Insert message, folder and tags"
+gen_insert_msg
+notmuch insert --folder=Drafts +draft -unread < "$gen_msg_filename"
+output=$(notmuch count folder:Drafts tag:draft NOT tag:unread)
+test_expect_equal "$output" "1"
+
+gen_insert_msg
+test_expect_code 1 "Insert message, non-existent folder" \
+    "notmuch insert --folder=nonesuch < $gen_msg_filename"
+
+test_begin_subtest "Insert message, create folder"
+gen_insert_msg
+notmuch insert --folder=F --create-folder +folder < "$gen_msg_filename"
+output=$(notmuch search --output=files folder:F tag:folder)
+basename=$(basename "$output")
+test_expect_equal_file "$gen_msg_filename" "$MAIL_DIR/F/cur/${basename}"
+
+test_begin_subtest "Insert message, create subfolder"
+gen_insert_msg
+notmuch insert --folder=F/G/H/I/J --create-folder +folder < "$gen_msg_filename"
+output=$(notmuch search --output=files folder:F/G/H/I/J tag:folder)
+basename=$(basename "$output")
+test_expect_equal_file "$gen_msg_filename" "${MAIL_DIR}/F/G/H/I/J/cur/${basename}"
+
+test_begin_subtest "Insert message, create existing subfolder"
+gen_insert_msg
+notmuch insert --folder=F/G/H/I/J --create-folder +folder < "$gen_msg_filename"
+output=$(notmuch count folder:F/G/H/I/J tag:folder)
+test_expect_equal "$output" "2"
+
+gen_insert_msg
+test_expect_code 1 "Insert message, create invalid subfolder" \
+    "notmuch insert --folder=../G --create-folder $gen_msg_filename"
+
+test_done
diff --git a/test/notmuch-test b/test/notmuch-test
index ca9c3dc..6952f0a 100755
--- a/test/notmuch-test
+++ b/test/notmuch-test
@@ -22,6 +22,7 @@ TESTS="
   config
   new
   count
+  insert
   search
   search-output
   search-by-folder
-- 
1.7.12.1

Re: [PATCH v3 08/20] tag-util: move out 'tag' command-line checks

[Jani Nikula]

On Sun, 20 Jan 2013, Peter Wang <novalazy@gmail.com> wrote:
> parse_tag_command_line checked for two error conditions which are
> specific to the 'tag' command.  It can be reused for the notmuch
> 'insert' command if we move the checks out, into notmuch-tag.c.

*three* error conditions, two of which are specific to notmuch tag. See
 below.

> ---
>  notmuch-tag.c | 10 ++++++++++
>  tag-util.c    | 10 ----------
>  2 files changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/notmuch-tag.c b/notmuch-tag.c
> index d9daf8f..a901dad 100644
> --- a/notmuch-tag.c
> +++ b/notmuch-tag.c
> @@ -234,6 +234,16 @@ notmuch_tag_command (void *ctx, int argc, char *argv[])
>  	if (parse_tag_command_line (ctx, argc - opt_index, argv + opt_index,
>  				    &query_string, tag_ops))
>  	    return 1;
> +
> +	if (tag_op_list_size (tag_ops) == 0) {
> +	    fprintf (stderr, "Error: 'notmuch tag' requires at least one tag to add or remove.\n");
> +	    return 1;
> +	}
> +
> +	if (*query_string == '\0') {
> +	    fprintf (stderr, "Error: notmuch tag requires at least one search term.\n");
> +	    return 1;
> +	}
>      }
>  
>      config = notmuch_config_open (ctx, NULL, NULL);
> diff --git a/tag-util.c b/tag-util.c
> index 3f9da05..41f2c09 100644
> --- a/tag-util.c
> +++ b/tag-util.c
> @@ -186,18 +186,8 @@ parse_tag_command_line (void *ctx, int argc, char **argv,
>  	tag_op_list_append (tag_ops, argv[i] + 1, is_remove);
>      }
>  
> -    if (tag_op_list_size (tag_ops) == 0) {
> -	fprintf (stderr, "Error: 'notmuch tag' requires at least one tag to add or remove.\n");
> -	return TAG_PARSE_INVALID;
> -    }
> -
>      *query_str = query_string_from_args (ctx, argc - i, &argv[i]);
>  
> -    if (*query_str == NULL || **query_str == '\0') {

You must leave *query_str == NULL check intact here. Fix the error
message to be about allocation failure and drop the reference to notmuch
tag while at it.

Otherwise LGTM.

> -	fprintf (stderr, "Error: notmuch tag requires at least one search term.\n");
> -	return TAG_PARSE_INVALID;
> -    }
> -
>      return TAG_PARSE_SUCCESS;
>  }
>  
> -- 
> 1.7.12.1
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH v3 07/20] tag-util: do not reset list in parse_tag_command_line

[Jani Nikula]

On Sun, 20 Jan 2013, Peter Wang <novalazy@gmail.com> wrote:
> No current callers of parse_tag_command_line require that it clear its
> tag list argument.  The notmuch 'insert' command will be better served
> if the function modifies a pre-populated list (of new.tags) instead of
> clobbering it outright.

I think I'd like parse_tag_command_line() and parse_tag_line() to behave
similarly, both either resetting or not resetting tag list. I think you
could just parse command line first, and add new.tags afterwards, and
you'd be fine without changing parse_tag_command_line().

> ---
>  tag-util.c | 2 --
>  tag-util.h | 2 ++
>  2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/tag-util.c b/tag-util.c
> index 701d329..3f9da05 100644
> --- a/tag-util.c
> +++ b/tag-util.c
> @@ -165,8 +165,6 @@ parse_tag_command_line (void *ctx, int argc, char **argv,
>  
>      int i;
>  
> -    tag_op_list_reset (tag_ops);
> -
>      for (i = 0; i < argc; i++) {
>  	if (strcmp (argv[i], "--") == 0) {
>  	    i++;
> diff --git a/tag-util.h b/tag-util.h
> index 246de85..4628f16 100644
> --- a/tag-util.h
> +++ b/tag-util.h
> @@ -81,6 +81,8 @@ parse_tag_line (void *ctx, char *line,
>   * Output Parameters:
>   *	ops	contains a list of tag operations
>   *	query_str the search terms.
> + *
> + * The ops argument is not cleared.
>   */
>  
>  tag_parse_status_t
> -- 
> 1.7.12.1
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH v3 00/20] insert command

[Jani Nikula]

On Sun, 20 Jan 2013, Peter Wang <novalazy@gmail.com> wrote:
> This mainly addresses review comments from v2
> and rebases on top of tag-util.c.

Hi Peter, I have some interest in seeing this get merged. To get that
done in a timely manner, my suggestion is to do this in two steps. Drop
the --folder and --create-folder options and the related code for now,
and submit them as another series once the first part has been
merged. It will be faster to iterate the first part, and we can dodge
some of the issues in the --folder and --create-folder options for now.

This is just my personal opinion, and it's up to you whether you do this
or not. Let's just call it a hunch saying it'll be faster this way. ;)

The patches would be rearranged as follows:

> Peter Wang (20):
>   tag-util: do not reset list in parse_tag_command_line
>   tag-util: move out 'tag' command-line checks

Keep the above separate, and make them the first preparatory patches.

>   cli: add stub for insert command
>   insert: open Maildir tmp file
>   insert: copy stdin to Maildir tmp file
>   insert: move file from Maildir tmp to new
>   insert: add new message to database
>   insert: apply default tags to new message
>   insert: parse and apply command-line tag operations
>   insert: fsync after writing tmp file
>   insert: trap SIGINT and clean up
>   insert: add copyright line from notmuch-deliver

Squash all of the above together into one patch. I will send a review of
these based on this version of the series.

>   insert: support --folder option
>   insert: prevent writes outside Maildir hierarchy
>   insert: add --create-folder option
>   insert: fsync new directory after rename
>   insert: fsync parent directory after mkdir

Drop these for now.

>   man: document 'insert' command
>   man: reference notmuch-insert.1
>   test: add tests for insert

Keep these separate as they are, but update to reflect the above.

Thoughts?


BR,
Jani.

>
>  Makefile.local                  |   1 +
>  man/Makefile.local              |   1 +
>  man/man1/notmuch-config.1       |   4 +-
>  man/man1/notmuch-count.1        |   4 +-
>  man/man1/notmuch-dump.1         |   4 +-
>  man/man1/notmuch-insert.1       |  59 +++++
>  man/man1/notmuch-new.1          |   4 +-
>  man/man1/notmuch-reply.1        |   3 +-
>  man/man1/notmuch-restore.1      |   3 +-
>  man/man1/notmuch-search.1       |   3 +-
>  man/man1/notmuch-show.1         |   3 +-
>  man/man1/notmuch-tag.1          |   3 +-
>  man/man1/notmuch.1              |   3 +-
>  man/man5/notmuch-hooks.5        |   4 +-
>  man/man7/notmuch-search-terms.7 |   3 +-
>  notmuch-client.h                |   3 +
>  notmuch-insert.c                | 482 ++++++++++++++++++++++++++++++++++++++++
>  notmuch-tag.c                   |  10 +
>  notmuch.c                       |   3 +
>  tag-util.c                      |  12 -
>  tag-util.h                      |   2 +
>  test/insert                     | 106 +++++++++
>  test/notmuch-test               |   1 +
>  23 files changed, 692 insertions(+), 29 deletions(-)
>  create mode 100644 man/man1/notmuch-insert.1
>  create mode 100644 notmuch-insert.c
>  create mode 100755 test/insert
>
> -- 
> 1.7.12.1
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH v3 01/20] cli: add stub for insert command

[Jani Nikula]

Hi Peter -

On Sun, 20 Jan 2013, Peter Wang <novalazy@gmail.com> wrote:
> The notmuch insert command should read a message from standard input
> and deliver it to a Maildir folder, and then incorporate the message
> into the notmuch database.  Essentially it moves the functionality of
> notmuch-deliver into notmuch.

Bikeshedding first, I'd prefer "notmuch deliver" for the command too...

> Though it could be used as an alternative to notmuch new, the reason
> I want this is to allow my notmuch frontend to add postponed or sent
> messages to the mail store and notmuch database, without resorting to
> another tool (e.g. notmuch-deliver) nor directly modifying the maildir.

This review is based on the following patches squashed together:

	cli: add stub for insert command
	insert: open Maildir tmp file
	insert: copy stdin to Maildir tmp file
	insert: move file from Maildir tmp to new
	insert: add new message to database
	insert: apply default tags to new message
	insert: parse and apply command-line tag operations
	insert: fsync after writing tmp file
	insert: trap SIGINT and clean up
	insert: add copyright line from notmuch-deliver

It's much easier for me to grasp the big picture this way.

> ---
>  Makefile.local   |    1 +
>  notmuch-client.h |    3 +
>  notmuch-insert.c |  316 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  notmuch.c        |    3 +
>  4 files changed, 323 insertions(+)
>  create mode 100644 notmuch-insert.c
>
> diff --git a/Makefile.local b/Makefile.local
> index c274f07..bb2381d 100644
> --- a/Makefile.local
> +++ b/Makefile.local
> @@ -261,6 +261,7 @@ notmuch_client_srcs =		\
>  	notmuch-config.c	\
>  	notmuch-count.c		\
>  	notmuch-dump.c		\
> +	notmuch-insert.c	\
>  	notmuch-new.c		\
>  	notmuch-reply.c		\
>  	notmuch-restore.c	\
> diff --git a/notmuch-client.h b/notmuch-client.h
> index 5f28836..af7d094 100644
> --- a/notmuch-client.h
> +++ b/notmuch-client.h
> @@ -175,6 +175,9 @@ int
>  notmuch_dump_command (void *ctx, int argc, char *argv[]);
>  
>  int
> +notmuch_insert_command (void *ctx, int argc, char *argv[]);
> +
> +int
>  notmuch_new_command (void *ctx, int argc, char *argv[]);
>  
>  int
> diff --git a/notmuch-insert.c b/notmuch-insert.c
> new file mode 100644
> index 0000000..0e74be0
> --- /dev/null
> +++ b/notmuch-insert.c
> @@ -0,0 +1,316 @@
> +/* notmuch - Not much of an email program, (just index and search)
> + *
> + * Copyright © 2013 Peter Wang
> + *
> + * Based in part on notmuch-deliver
> + * Copyright © 2010 Ali Polatel
> + *
> + * This program is free software: you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation, either version 3 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see http://www.gnu.org/licenses/ .
> + *
> + * Author: Peter Wang <novalazy@gmail.com>
> + */
> +
> +#include "notmuch-client.h"
> +#include "tag-util.h"
> +
> +#include <sys/types.h>
> +#include <sys/stat.h>
> +#include <fcntl.h>
> +
> +static volatile sig_atomic_t interrupted;
> +
> +static void
> +handle_sigint (unused (int sig))
> +{
> +    static char msg[] = "Stopping...         \n";
> +
> +    /* This write is "opportunistic", so it's okay to ignore the
> +     * result.  It is not required for correctness, and if it does
> +     * fail or produce a short write, we want to get out of the signal
> +     * handler as quickly as possible, not retry it. */
> +    IGNORE_RESULT (write (2, msg, sizeof (msg) - 1));
> +    interrupted = 1;
> +}
> +
> +/* Like gethostname but guarantees that a null-terminated hostname is
> + * returned, even if it has to make one up.
> + * Returns true unless hostname contains a slash. */
> +static notmuch_bool_t
> +safe_gethostname (char *hostname, size_t len)
> +{
> +    if (gethostname (hostname, len) == -1) {
> +	strncpy (hostname, "unknown", len);
> +    }
> +    hostname[len - 1] = '\0';
> +
> +    return (strchr (hostname, '/') == NULL);

You could just replace all chars you don't accept with something you
do. Add ':' to the list of unacceptable chars.

> +}
> +
> +/* Open a unique file in the Maildir 'tmp' directory.
> + * 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. */
> +static int
> +maildir_open_tmp_file (void *ctx, const char *dir,
> +		       char **tmppath, char **newpath)
> +{
> +    pid_t pid;
> +    char hostname[256];
> +    struct timeval tv;
> +    char *filename;
> +    int fd = -1;
> +
> +    /* We follow the Dovecot file name generation algorithm. */

See also http://cr.yp.to/proto/maildir.html

> +    pid = getpid ();
> +    if (! safe_gethostname (hostname, sizeof (hostname))) {
> +	fprintf (stderr, "Error: invalid host name.\n");
> +	return -1;
> +    }
> +    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");
> +	    return -1;
> +	}
> +
> +	*tmppath = talloc_asprintf (ctx, "%s/tmp/%s", dir, filename);
> +	if (! *tmppath) {
> +	    fprintf (stderr, "Out of memory\n");
> +	    return -1;
> +	}
> +
> +	fd = open (*tmppath, 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));
> +	return -1;
> +    }
> +
> +    *newpath = talloc_asprintf (ctx, "%s/new/%s", dir, filename);
> +    if (! *newpath) {
> +	fprintf (stderr, "Out of memory\n");
> +	close (fd);
> +	unlink (*tmppath);
> +	return -1;
> +    }
> +
> +    talloc_free (filename);

Nitpick, in theory the do-while loop above could allocate a bunch of
filenames and paths that you do not free (they're freed as part of the
context).

> +
> +    return fd;
> +}
> +
> +/* 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
> + */
> +static notmuch_bool_t
> +maildir_move_tmp_to_new (const char *tmppath, const char *newpath)
> +{
> +    if (rename (tmppath, newpath) != 0) {
> +	fprintf (stderr, "Error: rename() failed: %s\n", strerror (errno));
> +	return FALSE;
> +    }
> +
> +    return TRUE;
> +}

IMO you could just use rename() inline in the caller, without a wrapper.

> +
> +/* Copy the contents of standard input (fdin) into fdout. */
> +static notmuch_bool_t
> +copy_stdin (int fdin, int fdout)

The comment and the function name imply the function has something to do
with stdin, while it only cares about file descriptors.

> +{
> +    char buf[4096];
> +    char *p;
> +    ssize_t remain;
> +    ssize_t written;
> +
> +    while (! interrupted) {
> +	remain = read (fdin, buf, sizeof (buf));
> +	if (remain == 0)
> +	    break;
> +	if (remain < 0) {
> +	    if (errno == EINTR)
> +		continue;
> +	    fprintf (stderr, "Error: reading from standard input: %s\n",
> +		     strerror (errno));
> +	    return FALSE;
> +	}
> +
> +	p = buf;
> +	do {
> +	    written = write (fdout, p, remain);
> +	    if (written == 0)
> +		return FALSE;

No error message?

> +	    if (written < 0) {
> +		if (errno == EINTR)
> +		    continue;
> +		fprintf (stderr, "Error: writing to temporary file: %s",
> +			 strerror (errno));
> +		return FALSE;
> +	    }
> +	    p += written;
> +	    remain -= written;
> +	} while (remain > 0);
> +    }
> +
> +    return ! interrupted;
> +}
> +
> +/* Add the specified message file to the notmuch database, applying tags.
> + * The 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_message_t *message;
> +    notmuch_status_t status;
> +
> +    status = notmuch_database_add_message (notmuch, path, &message);
> +    switch (status) {
> +    case NOTMUCH_STATUS_SUCCESS:
> +	break;
> +    case NOTMUCH_STATUS_DUPLICATE_MESSAGE_ID:
> +	fprintf (stderr, "Warning: duplicate message.\n");

This is not uncommon. Why the warning?

Also, notmuch new does not apply new.tags in this case. Are you sure we
want to do that here? (You get mail, you read and archive it, you get
the dupe, it pops up unread in your inbox again.)

> +	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 FALSE;
> +    }
> +
> +    tag_op_list_apply (message, tag_ops, TAG_FLAG_MAILDIR_SYNC);

Check return value.

> +
> +    notmuch_message_destroy (message);
> +
> +    return TRUE;
> +}
> +
> +static notmuch_bool_t
> +insert_message (void *ctx, notmuch_database_t *notmuch, int fdin,
> +		const char *dir, tag_op_list_t *tag_ops)
> +{
> +    char *tmppath;
> +    char *newpath;
> +    int fdout;
> +    notmuch_bool_t ret;
> +
> +    fdout = maildir_open_tmp_file (ctx, dir, &tmppath, &newpath);
> +    if (fdout < 0) {
> +	return FALSE;
> +    }
> +    ret = copy_stdin (fdin, fdout);
> +    if (ret && fsync (fdout) != 0) {

Keep ret check and fsync separate.

On some file systems you need to fsync the directory after adding a new
file as well.

> +	fprintf (stderr, "Error: fsync failed: %s\n", strerror (errno));
> +	ret = FALSE;
> +    }
> +    close (fdout);
> +    if (ret) {
> +	ret = maildir_move_tmp_to_new (tmppath, newpath);
> +    }
> +    if (!ret) {
> +	unlink (tmppath);
> +	return FALSE;
> +    }

I think you should clean up all of the error paths above. It's not
obvious at a glance what happens. Typically it should be just blocks
like this repeated:

	ret = foo();
	if (ret) {
	    /* error handling */
	}

> +
> +    ret = add_file_to_database (notmuch, newpath, tag_ops);
> +    if (!ret) {
> +	/* XXX maybe there should be an option to keep the file in maildir? */

Yes, in the future.

I might like it better if you separated writing the file to maildir and
adding the file to the database in the top level
notmuch_insert_command() function. The delivery function could return a
char * to the filename so it could be passed to
add_file_to_database(). And you wouldn't need to pass database or tag
ops to the maildir writing function, keeping things clearly separated.

> +	unlink (newpath);
> +	return FALSE;
> +    }
> +
> +    return TRUE;
> +}
> +
> +int
> +notmuch_insert_command (void *ctx, int argc, char *argv[])
> +{
> +    notmuch_config_t *config;
> +    notmuch_database_t *notmuch;
> +    struct sigaction action;
> +    const char *db_path;
> +    const char **new_tags;
> +    size_t new_tags_length;
> +    tag_op_list_t *tag_ops;
> +    char *query_string = NULL;
> +    char *maildir;
> +    int opt_index = 1;
> +    unsigned int i;
> +    notmuch_bool_t ret;
> +
> +    config = notmuch_config_open (ctx, NULL, NULL);
> +    if (config == NULL)
> +	return 1;
> +
> +    db_path = notmuch_config_get_database_path (config);
> +    new_tags = notmuch_config_get_new_tags (config, &new_tags_length);
> +
> +    tag_ops = tag_op_list_create (ctx);
> +    if (tag_ops == NULL) {
> +	fprintf (stderr, "Out of memory.\n");
> +	return 1;
> +    }
> +    for (i = 0; i < new_tags_length; i++) {
> +	if (tag_op_list_append (tag_ops, new_tags[i], FALSE))
> +	    return 1;
> +    }
> +
> +    if (parse_tag_command_line (ctx, argc - opt_index, argv + opt_index,
> +				&query_string, tag_ops))
> +	return 1;

Never mind about my earlier comment about changing
parse_tag_command_line(). It's probably better to allow this to override
new.tags.

> +
> +    if (*query_string != '\0') {
> +	fprintf (stderr, "Error: unexpected query string: %s\n", query_string);
> +	return 1;
> +    }
> +
> +    maildir = talloc_asprintf (ctx, "%s", db_path);
> +    if (! maildir) {
> +	fprintf (stderr, "Out of memory\n");
> +	return 1;
> +    }

There's no need to talloc maildir; it's the same as db_path.

> +
> +    /* Setup our handler for SIGINT. We do not set SA_RESTART so that copying
> +     * from standard input may be interrupted. */
> +    memset (&action, 0, sizeof (struct sigaction));
> +    action.sa_handler = handle_sigint;
> +    sigemptyset (&action.sa_mask);
> +    action.sa_flags = 0;
> +    sigaction (SIGINT, &action, NULL);
> +
> +    if (notmuch_database_open (notmuch_config_get_database_path (config),
> +			       NOTMUCH_DATABASE_MODE_READ_WRITE, &notmuch))
> +	return 1;
> +
> +    ret = insert_message (ctx, notmuch, STDIN_FILENO, maildir, tag_ops);
> +
> +    notmuch_database_destroy (notmuch);
> +
> +    return (ret) ? 0 : 1;
> +}
> diff --git a/notmuch.c b/notmuch.c
> index 4fc0973..1c3b893 100644
> --- a/notmuch.c
> +++ b/notmuch.c
> @@ -53,6 +53,9 @@ static command_t commands[] = {
>      { "new", notmuch_new_command,
>        "[options...]",
>        "Find and import new messages to the notmuch database." },
> +    { "insert", notmuch_insert_command,
> +      "[options...] [--] [+<tag>|-<tag> ...] < message",
> +      "Add a new message into the maildir and notmuch database." },
>      { "search", notmuch_search_command,
>        "[options...] <search-terms> [...]",
>        "Search for messages matching the given search terms." },
> -- 
> 1.7.10.4

Re: [PATCH v3 01/20] cli: add stub for insert command

[Peter Wang]

On Tue, 22 Jan 2013 23:45:43 +0200, Jani Nikula <jani@nikula.org> wrote:
> > Though it could be used as an alternative to notmuch new, the reason
> > I want this is to allow my notmuch frontend to add postponed or sent
> > messages to the mail store and notmuch database, without resorting to
> > another tool (e.g. notmuch-deliver) nor directly modifying the maildir.
> 
> This review is based on the following patches squashed together:
> 
> 	cli: add stub for insert command
> 	insert: open Maildir tmp file
> 	insert: copy stdin to Maildir tmp file
> 	insert: move file from Maildir tmp to new
> 	insert: add new message to database
> 	insert: apply default tags to new message
> 	insert: parse and apply command-line tag operations
> 	insert: fsync after writing tmp file
> 	insert: trap SIGINT and clean up
> 	insert: add copyright line from notmuch-deliver
> 
> It's much easier for me to grasp the big picture this way.
> 

So there *is* a limit to how fine-grained notmuchers want their patches ;-)

> > +static notmuch_bool_t
> > +maildir_move_tmp_to_new (const char *tmppath, const char *newpath)
> > +{
> > +    if (rename (tmppath, newpath) != 0) {
> > +	fprintf (stderr, "Error: rename() failed: %s\n", strerror (errno));
> > +	return FALSE;
> > +    }
> > +
> > +    return TRUE;
> > +}
> 
> IMO you could just use rename() inline in the caller, without a wrapper.

A subsequent patch fsyncs the directory here.

> > +/* Copy the contents of standard input (fdin) into fdout. */
> > +static notmuch_bool_t
> > +copy_stdin (int fdin, int fdout)
> 
> The comment and the function name imply the function has something to do
> with stdin, while it only cares about file descriptors.

Tomi pointed out that the error message refers to standard input.

> > +/* Add the specified message file to the notmuch database, applying tags.
> > + * The 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_message_t *message;
> > +    notmuch_status_t status;
> > +
> > +    status = notmuch_database_add_message (notmuch, path, &message);
> > +    switch (status) {
> > +    case NOTMUCH_STATUS_SUCCESS:
> > +	break;
> > +    case NOTMUCH_STATUS_DUPLICATE_MESSAGE_ID:
> > +	fprintf (stderr, "Warning: duplicate message.\n");
> 
> This is not uncommon. Why the warning?
> 

I put in the warning because I wasn't sure, then forgot to revisit it.

> Also, notmuch new does not apply new.tags in this case. Are you sure we
> want to do that here? (You get mail, you read and archive it, you get
> the dupe, it pops up unread in your inbox again.)

Should command-line tags to be applied to duplicate messages?
I'm thinking not.

I'll fix the other problems you found.

Peter

Re: [PATCH v3 01/20] cli: add stub for insert command

[Tomi Ollila]

On Wed, Jan 23 2013, Peter Wang <novalazy@gmail.com> wrote:

> On Tue, 22 Jan 2013 23:45:43 +0200, Jani Nikula <jani@nikula.org> wrote:
>
>> > +/* Copy the contents of standard input (fdin) into fdout. */
>> > +static notmuch_bool_t
>> > +copy_stdin (int fdin, int fdout)
>> 
>> The comment and the function name imply the function has something to do
>> with stdin, while it only cares about file descriptors.
>
> Tomi pointed out that the error message refers to standard input.

Was that me or some other Tomi ;)... Nevertheless the function here called
'copy_stdin' copies fdin to fdout, whatever the file descriptors are,
it doesn't enforce fdin to be fd 0 (default stdin).
I.e. the function name should be more generic (or then drop fdin argument --
should better name then be 'copy_from_stdin' or copy_stdin_to.. or
copy_[from_]stdin_to_some_magic_fd -- ok, no more bikeshedding now >;)).


> Peter

Tomi

Re: [PATCH v3 01/20] cli: add stub for insert command

[Jani Nikula]

On Wed, 23 Jan 2013, Peter Wang <novalazy@gmail.com> wrote:
> On Tue, 22 Jan 2013 23:45:43 +0200, Jani Nikula <jani@nikula.org> wrote:
>> > Though it could be used as an alternative to notmuch new, the reason
>> > I want this is to allow my notmuch frontend to add postponed or sent
>> > messages to the mail store and notmuch database, without resorting to
>> > another tool (e.g. notmuch-deliver) nor directly modifying the maildir.
>> 
>> This review is based on the following patches squashed together:
>> 
>> 	cli: add stub for insert command
>> 	insert: open Maildir tmp file
>> 	insert: copy stdin to Maildir tmp file
>> 	insert: move file from Maildir tmp to new
>> 	insert: add new message to database
>> 	insert: apply default tags to new message
>> 	insert: parse and apply command-line tag operations
>> 	insert: fsync after writing tmp file
>> 	insert: trap SIGINT and clean up
>> 	insert: add copyright line from notmuch-deliver
>> 
>> It's much easier for me to grasp the big picture this way.
>> 
>
> So there *is* a limit to how fine-grained notmuchers want their patches ;-)

:)

>
>> > +static notmuch_bool_t
>> > +maildir_move_tmp_to_new (const char *tmppath, const char *newpath)
>> > +{
>> > +    if (rename (tmppath, newpath) != 0) {
>> > +	fprintf (stderr, "Error: rename() failed: %s\n", strerror (errno));
>> > +	return FALSE;
>> > +    }
>> > +
>> > +    return TRUE;
>> > +}
>> 
>> IMO you could just use rename() inline in the caller, without a wrapper.
>
> A subsequent patch fsyncs the directory here.

Ah, right.

You'll probably need to rework the patch a bit to apply here (if you
take my advice of postponing the --folder etc. options). While at it,
please try to see if you can reduce the amount of paths allocated and
passed around. Given the filename, one can figure out the rest. See
lib/message.cc for examples. In the end, go for clarity if this
suggestion seems messy.

>
>> > +/* Copy the contents of standard input (fdin) into fdout. */
>> > +static notmuch_bool_t
>> > +copy_stdin (int fdin, int fdout)
>> 
>> The comment and the function name imply the function has something to do
>> with stdin, while it only cares about file descriptors.
>
> Tomi pointed out that the error message refers to standard input.
>
>> > +/* Add the specified message file to the notmuch database, applying tags.
>> > + * The 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_message_t *message;
>> > +    notmuch_status_t status;
>> > +
>> > +    status = notmuch_database_add_message (notmuch, path, &message);
>> > +    switch (status) {
>> > +    case NOTMUCH_STATUS_SUCCESS:
>> > +	break;
>> > +    case NOTMUCH_STATUS_DUPLICATE_MESSAGE_ID:
>> > +	fprintf (stderr, "Warning: duplicate message.\n");
>> 
>> This is not uncommon. Why the warning?
>> 
>
> I put in the warning because I wasn't sure, then forgot to revisit it.
>
>> Also, notmuch new does not apply new.tags in this case. Are you sure we
>> want to do that here? (You get mail, you read and archive it, you get
>> the dupe, it pops up unread in your inbox again.)
>
> Should command-line tags to be applied to duplicate messages?
> I'm thinking not.

Agreed; a future patch might add an option to choose.

I've thought about having a config option to have notmuch new add a
separate set of tags to duplicates (defaulting to no tags added). That
could be reused here too (but is different from the command-line
tags). But again, future work.

> I'll fix the other problems you found.
>
> Peter


BR,
Jani.