From: Peter Wang <novalazy@gmail.com>
To: notmuch@notmuchmail.org
Subject: [PATCH v4 00/12] insert command
Date: Thu, 24 Jan 2013 23:07:56 +1100
Message-Id: <1359029288-12132-1-git-send-email-novalazy@gmail.com>
Precedence: list

Differences from v3:

 - squashed patches; take it up with Jani
 - address some review comments (interdiff follows)
 - some stylistic things I left for someone who cares
   (either I tried it and didn't like it, or disagree with the premise)
 - split doc and test patches so series can be partially applied
   without --folder or --create-folder options

Peter Wang (12):
  tag-util: move out 'tag' command-line checks
  tag-util: do not reset list in parse_tag_command_line
  cli: add insert command
  man: document 'insert' command
  man: reference notmuch-insert.1
  test: add tests for insert
  insert: add --folder option
  man: document insert --folder option
  test: test insert --folder option
  insert: add --create-folder option
  man: document insert --create-folder
  test: test insert --create-folder option

 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       |  63 ++++++
 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                | 484 ++++++++++++++++++++++++++++++++++++++++
 notmuch-tag.c                   |  10 +
 notmuch.c                       |   3 +
 tag-util.c                      |  13 +-
 tag-util.h                      |   2 +
 test/insert                     | 110 +++++++++
 test/notmuch-test               |   1 +
 23 files changed, 705 insertions(+), 27 deletions(-)
 create mode 100644 man/man1/notmuch-insert.1
 create mode 100644 notmuch-insert.c
 create mode 100755 test/insert

-- 
1.7.12.1


diff --git a/man/man1/notmuch-insert.1 b/man/man1/notmuch-insert.1
index 4a7cbeb..8ce634e 100644
--- a/man/man1/notmuch-insert.1
+++ b/man/man1/notmuch-insert.1
@@ -24,6 +24,10 @@ configuration option, then by operations specified on the command-line:
 tags prefixed by '+' are added while
 those prefixed by '\-' are removed.
 
+If the new message is a duplicate of an existing message in the database
+(it has same Message-ID), it will be added to the maildir folder and
+notmuch database, but the tags will not be changed.
+
 Option arguments must appear before any tag operation arguments.
 Supported options for
 .B insert
diff --git a/notmuch-insert.c b/notmuch-insert.c
index 6b3e380..69329ad 100644
--- a/notmuch-insert.c
+++ b/notmuch-insert.c
@@ -44,33 +44,22 @@ handle_sigint (unused (int sig))
 }
 
 /* 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
+ * returned, even if it has to make one up. Invalid characters are
+ * substituted such that the hostname can be used within a filename.
+ */
+static void
 safe_gethostname (char *hostname, size_t len)
 {
+    char *p;
+
     if (gethostname (hostname, len) == -1) {
 	strncpy (hostname, "unknown", len);
     }
     hostname[len - 1] = '\0';
 
-    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++;
+    for (p = hostname; *p != '\0'; p++) {
+	if (*p == '/' || *p == ':')
+	    *p = '_';
     }
 }
 
@@ -96,6 +85,23 @@ 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. */
+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++;
+    }
+}
+
 /* Make the given directory, succeeding if it already exists. */
 static notmuch_bool_t
 make_directory (char *path, int mode)
@@ -206,10 +212,7 @@ maildir_open_tmp_file (void *ctx, const char *dir,
 
     /* 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;
-    }
+    safe_gethostname (hostname, sizeof (hostname));
     do {
 	gettimeofday (&tv, NULL);
 	filename = talloc_asprintf (ctx, "%ld.M%ldP%d.%s",
@@ -247,26 +250,6 @@ 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,
-		         const char *newdir)
-{
-    if (rename (tmppath, newpath) != 0) {
-	fprintf (stderr, "Error: rename() failed: %s\n", strerror (errno));
-	return FALSE;
-    }
-
-    /* Sync the 'new' directory after rename for durability. */
-    return sync_dir (newdir);
-}
-
 /* Copy the contents of standard input (fdin) into fdout. */
 static notmuch_bool_t
 copy_stdin (int fdin, int fdout)
@@ -291,11 +274,9 @@ copy_stdin (int fdin, int fdout)
 	p = buf;
 	do {
 	    written = write (fdout, p, remain);
-	    if (written == 0)
-		return FALSE;
-	    if (written < 0) {
-		if (errno == EINTR)
-		    continue;
+	    if (written < 0 && errno == EINTR)
+		continue;
+	    if (written <= 0) {
 		fprintf (stderr, "Error: writing to temporary file: %s",
 			 strerror (errno));
 		return FALSE;
@@ -320,9 +301,7 @@ add_file_to_database (notmuch_database_t *notmuch, const char *path,
     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:
@@ -340,11 +319,18 @@ add_file_to_database (notmuch_database_t *notmuch, const char *path,
 	return FALSE;
     }
 
-    tag_op_list_apply (message, tag_ops, TAG_FLAG_MAILDIR_SYNC);
+    if (status == NOTMUCH_STATUS_DUPLICATE_MESSAGE_ID) {
+	/* Don't change tags of an existing message. */
+	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 {
+	status = tag_op_list_apply (message, tag_ops, TAG_FLAG_MAILDIR_SYNC);
+    }
 
     notmuch_message_destroy (message);
 
-    return TRUE;
+    return (status == NOTMUCH_STATUS_SUCCESS) ? TRUE : FALSE;
 }
 
 static notmuch_bool_t
@@ -355,29 +341,45 @@ insert_message (void *ctx, notmuch_database_t *notmuch, int fdin,
     char *newpath;
     char *newdir;
     int fdout;
-    notmuch_bool_t ret;
 
     fdout = maildir_open_tmp_file (ctx, dir, &tmppath, &newpath, &newdir);
     if (fdout < 0) {
 	return FALSE;
     }
-    ret = copy_stdin (fdin, fdout);
-    if (ret && fsync (fdout) != 0) {
+
+    if (! copy_stdin (fdin, fdout)) {
+	close (fdout);
+	unlink (tmppath);
+	return FALSE;
+    }
+
+    if (fsync (fdout) != 0) {
 	fprintf (stderr, "Error: fsync failed: %s\n", strerror (errno));
-	ret = FALSE;
+	close (fdout);
+	unlink (tmppath);
+	return FALSE;
     }
+
     close (fdout);
-    if (ret) {
-	ret = maildir_move_tmp_to_new (tmppath, newpath, newdir);
-    }
-    if (!ret) {
+
+    /* 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));
 	unlink (tmppath);
 	return FALSE;
     }
 
-    ret = add_file_to_database (notmuch, newpath, tag_ops);
-    if (!ret) {
-	/* XXX maybe there should be an option to keep the file in maildir? */
+    if (! add_file_to_database (notmuch, newpath, tag_ops)) {
+	/* XXX add an option to keep the file in maildir? */
+	unlink (newpath);
+	return FALSE;
+    }
+
+    if (! sync_dir (newdir)) {
 	unlink (newpath);
 	return FALSE;
     }
@@ -398,7 +400,7 @@ notmuch_insert_command (void *ctx, int argc, char *argv[])
     char *query_string = NULL;
     const char *folder = NULL;
     notmuch_bool_t create_folder = FALSE;
-    char *maildir;
+    const char *maildir;
     int opt_index;
     unsigned int i;
     notmuch_bool_t ret;
@@ -443,23 +445,23 @@ notmuch_insert_command (void *ctx, int argc, char *argv[])
 	return 1;
     }
 
-    if (folder != NULL) {
+    if (folder == NULL) {
+	maildir = db_path;
+    } else {
 	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);
-    }
-    if (! maildir) {
-	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 (! maildir) {
+	    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;
+	}
     }
 
     /* Setup our handler for SIGINT. We do not set SA_RESTART so that copying
diff --git a/tag-util.c b/tag-util.c
index 41f2c09..b57ee32 100644
--- a/tag-util.c
+++ b/tag-util.c
@@ -188,6 +188,11 @@ parse_tag_command_line (void *ctx, int argc, char **argv,
 
     *query_str = query_string_from_args (ctx, argc - i, &argv[i]);
 
+    if (*query_str == NULL) {
+       fprintf (stderr, "Out of memory.\n");
+       return TAG_PARSE_OUT_OF_MEMORY;
+    }
+
     return TAG_PARSE_SUCCESS;
 }
 
diff --git a/test/insert b/test/insert
index a3b6283..24a61e1 100755
--- a/test/insert
+++ b/test/insert
@@ -46,10 +46,14 @@ expected='[[[{
 test_expect_equal_json "$output" "$expected"
 
 test_begin_subtest "Insert message, duplicate message"
-notmuch insert < "$gen_msg_filename"
+notmuch insert +duptag -unread < "$gen_msg_filename"
 output=$(notmuch search --output=files "subject:insert-subject" | wc -l)
 test_expect_equal "$output" 2
 
+test_begin_subtest "Insert message, duplicate message does not change tags"
+output=$(notmuch search --format=json --output=tags "subject:insert-subject")
+test_expect_equal_json "$output" '["inbox", "unread"]'
+
 test_begin_subtest "Insert message, add tag"
 gen_insert_msg
 notmuch insert +custom < "$gen_msg_filename"