v7 batch tagging series

v7 batch tagging series

[david]

This obsoletes  id:1355096008-4544-1-git-send-email-david@tethera.net

There are some fixups according to previous reviews, detailed in a
diff at the end.

I have just read id:871uettxln.fsf@qmul.ac.uk; this series doesn't
respond to that.

There are some new trivial cosmetic patches; except for conflicts
these don't really need to be part of this series.

  [Patch v7 01/14] parse_tag_line: use enum for return value.
  [Patch v7 13/14] notmuch-tag.1: tidy synopsis formatting

This version includes a bunch of test patches (patches 8-12).

The other reason for the increase in the number of patches is that I
discovered a bug in the previous version (when I implement patch
11/14), and I had to add some internal quoting for queries:

  [Patch v7 04/14] notmuch-tag: factor out double quoting routine
  [Patch v7 05/14] quote_and_decode_query: new function to quote
  [Patch v7 06/14] notmuch-restore: move query handling for batch

commit 6f4a8ac5fb90b301bc905c4ea90cdf33456eb06e
Author: David Bremner <bremner@debian.org>
Date:   Tue Dec 11 22:12:50 2012 -0400

    formatting fixup for illegal_tag

diff --git a/tag-util.c b/tag-util.c
index 2214f34..091e1ec 100644
--- a/tag-util.c
+++ b/tag-util.c
@@ -39,17 +39,18 @@ line_error (tag_parse_status_t status,
  */
 
 static const char *
-illegal_tag (const char *tag, notmuch_bool_t remove) {
+illegal_tag (const char *tag, notmuch_bool_t remove)
+{
 
-    if (*tag == '\0' && !remove)
+    if (*tag == '\0' && ! remove)
 	return "adding empty tag";
 
     /* This disallows adding the non-removable tag "-" and
      * enables notmuch tag to take long options more easily.
      */
 
-    if (*tag == '-' && !remove)
-	return  "adding tag starting with -";
+    if (*tag == '-' && ! remove)
+	return "adding tag starting with -";
 
     return NULL;
 }

commit 60ca31c81adfb3b9c2ca6e2d2eaee6fcc483ea97
Author: David Bremner <bremner@debian.org>
Date:   Tue Dec 11 22:17:48 2012 -0400

    uncrustify fixup for parse_tag_command_line

diff --git a/tag-util.c b/tag-util.c
index 091e1ec..30d856d 100644
--- a/tag-util.c
+++ b/tag-util.c
@@ -45,8 +45,9 @@ illegal_tag (const char *tag, notmuch_bool_t remove)
     if (*tag == '\0' && ! remove)
 	return "adding empty tag";
 
-    /* This disallows adding the non-removable tag "-" and
-     * enables notmuch tag to take long options more easily.
+    /* This disallows adding tags starting with "-", in particular the
+     * non-removable tag "-" and enables notmuch tag to take long
+     * options more easily.
      */
 
     if (*tag == '-' && ! remove)
@@ -165,12 +166,13 @@ parse_tag_line (void *ctx, char *line,
 
 int
 parse_tag_command_line (void *ctx, int argc, char **argv,
-			unused(tag_op_flag_t flags),
-			char **query_str, tag_op_list_t *tag_ops){
+			unused (tag_op_flag_t flags),
+			char **query_str, tag_op_list_t *tag_ops)
+{
 
     int i;
 
-    tag_op_list_reset(tag_ops);
+    tag_op_list_reset (tag_ops);
 
     for (i = 0; i < argc; i++) {
 	if (strcmp (argv[i], "--") == 0) {
@@ -182,7 +184,7 @@ parse_tag_command_line (void *ctx, int argc, char **argv,
 	    notmuch_bool_t is_remove = argv[i][0] == '-';
 	    const char *msg;
 
-	    msg = illegal_tag(argv[i]+1, is_remove);
+	    msg = illegal_tag (argv[i] + 1, is_remove);
 	    if (msg) {
 		fprintf (stderr, "Error: %s", msg);
 		return 1;

commit f3ae405abda331558affedf52391a067acc7b7df
Author: David Bremner <bremner@debian.org>
Date:   Tue Dec 11 22:21:46 2012 -0400

    remove unused flags

diff --git a/notmuch-tag.c b/notmuch-tag.c
index 2665037..a5e8715 100644
--- a/notmuch-tag.c
+++ b/notmuch-tag.c
@@ -242,7 +242,7 @@ notmuch_tag_command (void *ctx, int argc, char *argv[])
 	}
 
 	if (parse_tag_command_line (ctx, argc - opt_index, argv + opt_index,
-				    0, &query_string, tag_ops))
+				    &query_string, tag_ops))
 	    return 1;
     }
 
diff --git a/tag-util.c b/tag-util.c
index 30d856d..e7ee182 100644
--- a/tag-util.c
+++ b/tag-util.c
@@ -166,7 +166,6 @@ parse_tag_line (void *ctx, char *line,
 
 int
 parse_tag_command_line (void *ctx, int argc, char **argv,
-			unused (tag_op_flag_t flags),
 			char **query_str, tag_op_list_t *tag_ops)
 {
 
diff --git a/tag-util.h b/tag-util.h
index 4956725..2889736 100644
--- a/tag-util.h
+++ b/tag-util.h
@@ -85,7 +85,6 @@ parse_tag_line (void *ctx, char *line,
 
 tag_parse_status_t
 parse_tag_command_line (void *ctx, int argc, char **argv,
-			tag_op_flag_t flags,
 			char **query_str, tag_op_list_t *ops);
 
 /*

commit c1aa36573bb991a1d06dd2e234a2377e2a32bd66
Author: David Bremner <bremner@debian.org>
Date:   Tue Dec 11 22:25:40 2012 -0400

    use enum values in parse_tag_command_line

diff --git a/tag-util.c b/tag-util.c
index e7ee182..102adcc 100644
--- a/tag-util.c
+++ b/tag-util.c
@@ -164,7 +164,7 @@ parse_tag_line (void *ctx, char *line,
     return ret;
 }
 
-int
+tag_parse_status_t
 parse_tag_command_line (void *ctx, int argc, char **argv,
 			char **query_str, tag_op_list_t *tag_ops)
 {
@@ -186,7 +186,7 @@ parse_tag_command_line (void *ctx, int argc, char **argv,
 	    msg = illegal_tag (argv[i] + 1, is_remove);
 	    if (msg) {
 		fprintf (stderr, "Error: %s", msg);
-		return 1;
+		return TAG_PARSE_INVALID;
 	    }
 
 	    tag_op_list_append (ctx, tag_ops,
@@ -198,17 +198,17 @@ parse_tag_command_line (void *ctx, int argc, char **argv,
 
     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;
+	return TAG_PARSE_INVALID;
     }
 
     *query_str = query_string_from_args (ctx, argc - i, &argv[i]);
 
     if (**query_str == '\0') {
 	fprintf (stderr, "Error: notmuch tag requires at least one search term.\n");
-	return 1;
+	return TAG_PARSE_INVALID;
     }
 
-    return 0;
+    return TAG_PARSE_SUCCESS;
 }
 
 

commit 562200240098238211d2ea0299b2ae03f4dbe919
Author: David Bremner <bremner@debian.org>
Date:   Tue Dec 11 22:32:31 2012 -0400

    remaining changes for id:87k3spa9nk.fsf@nikula.org

diff --git a/tag-util.c b/tag-util.c
index 102adcc..43d19e1 100644
--- a/tag-util.c
+++ b/tag-util.c
@@ -179,21 +179,19 @@ parse_tag_command_line (void *ctx, int argc, char **argv,
 	    break;
 	}
 
-	if (argv[i][0] == '+' || argv[i][0] == '-') {
-	    notmuch_bool_t is_remove = argv[i][0] == '-';
-	    const char *msg;
+	if (argv[i][0] != '+' && argv[i][0] != '-')
+	    break;
 
-	    msg = illegal_tag (argv[i] + 1, is_remove);
-	    if (msg) {
-		fprintf (stderr, "Error: %s", msg);
-		return TAG_PARSE_INVALID;
-	    }
+	notmuch_bool_t is_remove = argv[i][0] == '-';
+	const char *msg;
 
-	    tag_op_list_append (ctx, tag_ops,
-				argv[i] + 1, (argv[i][0] == '-'));
-	} else {
-	    break;
+	msg = illegal_tag (argv[i] + 1, is_remove);
+	if (msg) {
+	    fprintf (stderr, "Error: %s", msg);
+	    return TAG_PARSE_INVALID;
 	}
+
+	tag_op_list_append (ctx, tag_ops, argv[i] + 1, is_remove);
     }
 
     if (tag_op_list_size (tag_ops) == 0) {
@@ -203,7 +201,7 @@ parse_tag_command_line (void *ctx, int argc, char **argv,
 
     *query_str = query_string_from_args (ctx, argc - i, &argv[i]);
 
-    if (**query_str == '\0') {
+    if (*query_str == NULL || **query_str == '\0') {
 	fprintf (stderr, "Error: notmuch tag requires at least one search term.\n");
 	return TAG_PARSE_INVALID;
     }

commit c991402c6571003b79e855965fe1e3b6ae2dde35
Author: David Bremner <bremner@debian.org>
Date:   Tue Dec 11 22:35:52 2012 -0400

    "fix" comment about tag_op_list; assume tag-utils.h docs are sufficient.

diff --git a/notmuch-tag.c b/notmuch-tag.c
index a5e8715..508f04d 100644
--- a/notmuch-tag.c
+++ b/notmuch-tag.c
@@ -108,9 +108,8 @@ _optimize_tag_query (void *ctx, const char *orig_query_string,
     return query_string;
 }
 
-/* Tag messages matching 'query_string' according to 'tag_ops', which
- * must be an array of tagging operations terminated with an empty
- * element. */
+/* Tag messages matching 'query_string' according to 'tag_ops'
+ */
 static int
 tag_query (void *ctx, notmuch_database_t *notmuch, const char *query_string,
 	   tag_op_list_t *tag_ops, tag_op_flag_t flags)

commit 80628aceff38cad5f48e0ec7f381bbf8cec85cb2
Author: David Bremner <bremner@debian.org>
Date:   Tue Dec 11 22:44:17 2012 -0400

    slightly longwindedly make sure we return non-zero if tag_query fails

diff --git a/notmuch-tag.c b/notmuch-tag.c
index 508f04d..8f700b0 100644
--- a/notmuch-tag.c
+++ b/notmuch-tag.c
@@ -174,8 +174,11 @@ tag_file (void *ctx, notmuch_database_t *notmuch, tag_op_flag_t flags,
 	if (ret > 0)
 	    continue;
 
-	if (ret < 0 || tag_query (ctx, notmuch, query_string,
-				      tag_ops, flags))
+	if (ret < 0)
+	    break;
+
+	ret = tag_query (ctx, notmuch, query_string, tag_ops, flags);
+	if (ret)
 	    break;
     }
 

commit d1be30ed7eab8a1e9e0a492c0f024880f63236bb
Author: David Bremner <bremner@debian.org>
Date:   Tue Dec 11 22:44:30 2012 -0400

    close an input file if we opened it

diff --git a/notmuch-tag.c b/notmuch-tag.c
index 8f700b0..b707c25 100644
--- a/notmuch-tag.c
+++ b/notmuch-tag.c
@@ -264,6 +264,9 @@ notmuch_tag_command (void *ctx, int argc, char *argv[])
     else
 	ret = tag_query (ctx, notmuch, query_string, tag_ops, tag_flags);
 
+    if (input != stdin)
+	fclose (input);
+
     notmuch_database_destroy (notmuch);
 
     return ret || interrupted;

commit 702a2b11d43616f9b2ad74fb772ad080c228ab0a
Author: David Bremner <bremner@debian.org>
Date:   Tue Dec 11 23:37:51 2012 -0400

    fixup for illegal_tag error messages

diff --git a/tag-util.c b/tag-util.c
index 43d19e1..f89669a 100644
--- a/tag-util.c
+++ b/tag-util.c
@@ -43,7 +43,7 @@ illegal_tag (const char *tag, notmuch_bool_t remove)
 {
 
     if (*tag == '\0' && ! remove)
-	return "adding empty tag";
+	return "empty tag forbidden";
 
     /* This disallows adding tags starting with "-", in particular the
      * non-removable tag "-" and enables notmuch tag to take long
@@ -51,7 +51,7 @@ illegal_tag (const char *tag, notmuch_bool_t remove)
      */
 
     if (*tag == '-' && ! remove)
-	return "adding tag starting with -";
+	return "tag starting with '-' forbidden";
 
     return NULL;
 }

[Patch v7 01/14] parse_tag_line: use enum for return value.

[david]

From: David Bremner <bremner@debian.org>

This is essentially cosmetic, since success=0 is promised by
the comments in tag-utils.h.
---
 tag-util.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tag-util.c b/tag-util.c
index eab482f..3d588d3 100644
--- a/tag-util.c
+++ b/tag-util.c
@@ -40,7 +40,7 @@ parse_tag_line (void *ctx, char *line,
     char *tok = line;
     size_t tok_len = 0;
     char *line_for_error;
-    int ret = 0;
+    tag_parse_status_t ret = TAG_PARSE_SUCCESS;
 
     chomp_newline (line);
 
-- 
1.7.10.4

[Patch v7 02/14] tag-util: factor out rules for illegal tags, use in parse_tag_line

[david]

From: David Bremner <bremner@debian.org>

This will allow us to be consistent between batch tagging and command
line tagging as far as what is an illegal tag.
---
 tag-util.c |   36 +++++++++++++++++++++++++++++++-----
 1 file changed, 31 insertions(+), 5 deletions(-)

diff --git a/tag-util.c b/tag-util.c
index 3d588d3..13d9035 100644
--- a/tag-util.c
+++ b/tag-util.c
@@ -31,6 +31,30 @@ line_error (tag_parse_status_t status,
     return status;
 }
 
+/*
+ * Test tags for some forbidden cases.
+ *
+ * return: NULL if OK,
+ *	   explanatory message otherwise.
+ */
+
+static const char *
+illegal_tag (const char *tag, notmuch_bool_t remove)
+{
+
+    if (*tag == '\0' && ! remove)
+	return "empty tag forbidden";
+
+    /* This disallows adding the non-removable tag "-" and
+     * enables notmuch tag to take long options more easily.
+     */
+
+    if (*tag == '-' && ! remove)
+	return "tag starting with '-' forbidden";
+
+    return NULL;
+}
+
 tag_parse_status_t
 parse_tag_line (void *ctx, char *line,
 		tag_op_flag_t flags,
@@ -95,11 +119,13 @@ parse_tag_line (void *ctx, char *line,
 	remove = (*tok == '-');
 	tag = tok + 1;
 
-	/* Maybe refuse empty tags. */
-	if (! (flags & TAG_FLAG_BE_GENEROUS) && *tag == '\0') {
-	    ret = line_error (TAG_PARSE_INVALID, line_for_error,
-			      "empty tag");
-	    goto DONE;
+	/* Maybe refuse illegal tags. */
+	if (! (flags & TAG_FLAG_BE_GENEROUS)) {
+	    const char *msg = illegal_tag (tag, remove);
+	    if (msg) {
+		ret = line_error (TAG_PARSE_INVALID, line_for_error, msg);
+		goto DONE;
+	    }
 	}
 
 	/* Decode tag. */
-- 
1.7.10.4

[Patch v7 03/14] notmuch-tag.c: convert to use tag-utils

[david]

From: David Bremner <bremner@debian.org>

Command line parsing is factored out into a function
parse_tag_command_line in tag-utils.c.

There is some duplicated code eliminated in tag_query, and a bunch of
translation from using the bare tag_op structs to using that tag-utils
API.
---
 notmuch-tag.c |  109 +++++++++++++--------------------------------------------
 tag-util.c    |   51 +++++++++++++++++++++++++--
 tag-util.h    |   15 ++++++++
 3 files changed, 89 insertions(+), 86 deletions(-)

diff --git a/notmuch-tag.c b/notmuch-tag.c
index 88d559b..0965ee7 100644
--- a/notmuch-tag.c
+++ b/notmuch-tag.c
@@ -19,6 +19,7 @@
  */
 
 #include "notmuch-client.h"
+#include "tag-util.h"
 
 static volatile sig_atomic_t interrupted;
 
@@ -54,14 +55,9 @@ _escape_tag (char *buf, const char *tag)
     return buf;
 }
 
-typedef struct {
-    const char *tag;
-    notmuch_bool_t remove;
-} tag_operation_t;
-
 static char *
 _optimize_tag_query (void *ctx, const char *orig_query_string,
-		     const tag_operation_t *tag_ops)
+		     const tag_op_list_t *list)
 {
     /* This is subtler than it looks.  Xapian ignores the '-' operator
      * at the beginning both queries and parenthesized groups and,
@@ -73,19 +69,20 @@ _optimize_tag_query (void *ctx, const char *orig_query_string,
 
     char *escaped, *query_string;
     const char *join = "";
-    int i;
+    size_t i;
     unsigned int max_tag_len = 0;
 
     /* Don't optimize if there are no tag changes. */
-    if (tag_ops[0].tag == NULL)
+    if (tag_op_list_size (list) == 0)
 	return talloc_strdup (ctx, orig_query_string);
 
     /* Allocate a buffer for escaping tags.  This is large enough to
      * hold a fully escaped tag with every character doubled plus
      * enclosing quotes and a NUL. */
-    for (i = 0; tag_ops[i].tag; i++)
-	if (strlen (tag_ops[i].tag) > max_tag_len)
-	    max_tag_len = strlen (tag_ops[i].tag);
+    for (i = 0; i < tag_op_list_size (list); i++)
+	if (strlen (tag_op_list_tag (list, i)) > max_tag_len)
+	    max_tag_len = strlen (tag_op_list_tag (list, i));
+
     escaped = talloc_array (ctx, char, max_tag_len * 2 + 3);
     if (! escaped)
 	return NULL;
@@ -96,11 +93,11 @@ _optimize_tag_query (void *ctx, const char *orig_query_string,
     else
 	query_string = talloc_asprintf (ctx, "( %s ) and (", orig_query_string);
 
-    for (i = 0; tag_ops[i].tag && query_string; i++) {
+    for (i = 0; i < tag_op_list_size (list) && query_string; i++) {
 	query_string = talloc_asprintf_append_buffer (
 	    query_string, "%s%stag:%s", join,
-	    tag_ops[i].remove ? "" : "not ",
-	    _escape_tag (escaped, tag_ops[i].tag));
+	    tag_op_list_isremove (list, i) ? "" : "not ",
+	    _escape_tag (escaped, tag_op_list_tag (list, i)));
 	join = " or ";
     }
 
@@ -111,17 +108,15 @@ _optimize_tag_query (void *ctx, const char *orig_query_string,
     return query_string;
 }
 
-/* Tag messages matching 'query_string' according to 'tag_ops', which
- * must be an array of tagging operations terminated with an empty
- * element. */
+/* Tag messages matching 'query_string' according to 'tag_ops'
+ */
 static int
 tag_query (void *ctx, notmuch_database_t *notmuch, const char *query_string,
-	   tag_operation_t *tag_ops, notmuch_bool_t synchronize_flags)
+	   tag_op_list_t *tag_ops, tag_op_flag_t flags)
 {
     notmuch_query_t *query;
     notmuch_messages_t *messages;
     notmuch_message_t *message;
-    int i;
 
     /* Optimize the query so it excludes messages that already have
      * the specified set of tags. */
@@ -144,21 +139,7 @@ tag_query (void *ctx, notmuch_database_t *notmuch, const char *query_string,
 	 notmuch_messages_valid (messages) && ! interrupted;
 	 notmuch_messages_move_to_next (messages)) {
 	message = notmuch_messages_get (messages);
-
-	notmuch_message_freeze (message);
-
-	for (i = 0; tag_ops[i].tag; i++) {
-	    if (tag_ops[i].remove)
-		notmuch_message_remove_tag (message, tag_ops[i].tag);
-	    else
-		notmuch_message_add_tag (message, tag_ops[i].tag);
-	}
-
-	notmuch_message_thaw (message);
-
-	if (synchronize_flags)
-	    notmuch_message_tags_to_maildir_flags (message);
-
+	tag_op_list_apply (message, tag_ops, flags | TAG_FLAG_PRE_OPTIMIZED);
 	notmuch_message_destroy (message);
     }
 
@@ -170,15 +151,13 @@ tag_query (void *ctx, notmuch_database_t *notmuch, const char *query_string,
 int
 notmuch_tag_command (void *ctx, int argc, char *argv[])
 {
-    tag_operation_t *tag_ops;
-    int tag_ops_count = 0;
-    char *query_string;
+    tag_op_list_t *tag_ops = NULL;
+    char *query_string = NULL;
     notmuch_config_t *config;
     notmuch_database_t *notmuch;
     struct sigaction action;
-    notmuch_bool_t synchronize_flags;
-    int i;
-    int ret;
+    tag_op_flag_t tag_flags = TAG_FLAG_NONE;
+    int ret = 0;
 
     /* Setup our handler for SIGINT */
     memset (&action, 0, sizeof (struct sigaction));
@@ -187,54 +166,15 @@ notmuch_tag_command (void *ctx, int argc, char *argv[])
     action.sa_flags = SA_RESTART;
     sigaction (SIGINT, &action, NULL);
 
-    argc--; argv++; /* skip subcommand argument */
-
-    /* Array of tagging operations (add or remove), terminated with an
-     * empty element. */
-    tag_ops = talloc_array (ctx, tag_operation_t, argc + 1);
+    tag_ops = tag_op_list_create (ctx);
     if (tag_ops == NULL) {
 	fprintf (stderr, "Out of memory.\n");
 	return 1;
     }
 
-    for (i = 0; i < argc; i++) {
-	if (strcmp (argv[i], "--") == 0) {
-	    i++;
-	    break;
-	}
-	if (argv[i][0] == '+' || argv[i][0] == '-') {
-	    if (argv[i][0] == '+' && argv[i][1] == '\0') {
-		fprintf (stderr, "Error: tag names cannot be empty.\n");
-		return 1;
-	    }
-	    if (argv[i][0] == '+' && argv[i][1] == '-') {
-		/* This disallows adding the non-removable tag "-" and
-		 * enables notmuch tag to take long options in the
-		 * future. */
-		fprintf (stderr, "Error: tag names must not start with '-'.\n");
-		return 1;
-	    }
-	    tag_ops[tag_ops_count].tag = argv[i] + 1;
-	    tag_ops[tag_ops_count].remove = (argv[i][0] == '-');
-	    tag_ops_count++;
-	} else {
-	    break;
-	}
-    }
-
-    tag_ops[tag_ops_count].tag = NULL;
-
-    if (tag_ops_count == 0) {
-	fprintf (stderr, "Error: 'notmuch tag' requires at least one tag to add or remove.\n");
-	return 1;
-    }
-
-    query_string = query_string_from_args (ctx, argc - i, &argv[i]);
-
-    if (*query_string == '\0') {
-	fprintf (stderr, "Error: notmuch tag requires at least one search term.\n");
+    if (parse_tag_command_line (ctx, argc - 1, argv + 1,
+				&query_string, tag_ops))
 	return 1;
-    }
 
     config = notmuch_config_open (ctx, NULL, NULL);
     if (config == NULL)
@@ -244,9 +184,10 @@ notmuch_tag_command (void *ctx, int argc, char *argv[])
 			       NOTMUCH_DATABASE_MODE_READ_WRITE, &notmuch))
 	return 1;
 
-    synchronize_flags = notmuch_config_get_maildir_synchronize_flags (config);
+    if (notmuch_config_get_maildir_synchronize_flags (config))
+	tag_flags |= TAG_FLAG_MAILDIR_SYNC;
 
-    ret = tag_query (ctx, notmuch, query_string, tag_ops, synchronize_flags);
+    ret = tag_query (ctx, notmuch, query_string, tag_ops, tag_flags);
 
     notmuch_database_destroy (notmuch);
 
diff --git a/tag-util.c b/tag-util.c
index 13d9035..f89669a 100644
--- a/tag-util.c
+++ b/tag-util.c
@@ -45,8 +45,9 @@ illegal_tag (const char *tag, notmuch_bool_t remove)
     if (*tag == '\0' && ! remove)
 	return "empty tag forbidden";
 
-    /* This disallows adding the non-removable tag "-" and
-     * enables notmuch tag to take long options more easily.
+    /* This disallows adding tags starting with "-", in particular the
+     * non-removable tag "-" and enables notmuch tag to take long
+     * options more easily.
      */
 
     if (*tag == '-' && ! remove)
@@ -163,6 +164,52 @@ parse_tag_line (void *ctx, char *line,
     return ret;
 }
 
+tag_parse_status_t
+parse_tag_command_line (void *ctx, int argc, char **argv,
+			char **query_str, tag_op_list_t *tag_ops)
+{
+
+    int i;
+
+    tag_op_list_reset (tag_ops);
+
+    for (i = 0; i < argc; i++) {
+	if (strcmp (argv[i], "--") == 0) {
+	    i++;
+	    break;
+	}
+
+	if (argv[i][0] != '+' && argv[i][0] != '-')
+	    break;
+
+	notmuch_bool_t is_remove = argv[i][0] == '-';
+	const char *msg;
+
+	msg = illegal_tag (argv[i] + 1, is_remove);
+	if (msg) {
+	    fprintf (stderr, "Error: %s", msg);
+	    return TAG_PARSE_INVALID;
+	}
+
+	tag_op_list_append (ctx, 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;
+}
+
+
 static inline void
 message_error (notmuch_message_t *message,
 	       notmuch_status_t status,
diff --git a/tag-util.h b/tag-util.h
index 99b0fa0..2889736 100644
--- a/tag-util.h
+++ b/tag-util.h
@@ -72,6 +72,21 @@ parse_tag_line (void *ctx, char *line,
 		tag_op_flag_t flags,
 		char **query_str, tag_op_list_t *ops);
 
+
+
+/* Parse a command line of the following format:
+ *
+ * +<tag>|-<tag> [...] [--] <search-terms>
+ *
+ * Output Parameters:
+ *	ops	contains a list of tag operations
+ *	query_str the search terms.
+ */
+
+tag_parse_status_t
+parse_tag_command_line (void *ctx, int argc, char **argv,
+			char **query_str, tag_op_list_t *ops);
+
 /*
  * Create an empty list of tag operations
  *
-- 
1.7.10.4

[Patch v7 04/14] notmuch-tag: factor out double quoting routine

[david]

From: David Bremner <bremner@debian.org>

This could live in tag-util as well, but it is really nothing specific
to tags (although the conventions are arguable specific to Xapian).

The API is changed from "caller-allocates" to "readline-like". The scan for
max tag length is pushed down into the double quoting routine.
---
 notmuch-tag.c      |   50 ++++++++++++++++----------------------------------
 util/string-util.c |   34 ++++++++++++++++++++++++++++++++++
 util/string-util.h |    8 ++++++++
 3 files changed, 58 insertions(+), 34 deletions(-)

diff --git a/notmuch-tag.c b/notmuch-tag.c
index 0965ee7..13f2268 100644
--- a/notmuch-tag.c
+++ b/notmuch-tag.c
@@ -20,6 +20,7 @@
 
 #include "notmuch-client.h"
 #include "tag-util.h"
+#include "string-util.h"
 
 static volatile sig_atomic_t interrupted;
 
@@ -37,25 +38,6 @@ handle_sigint (unused (int sig))
 }
 
 static char *
-_escape_tag (char *buf, const char *tag)
-{
-    const char *in = tag;
-    char *out = buf;
-
-    /* Boolean terms surrounded by double quotes can contain any
-     * character.  Double quotes are quoted by doubling them. */
-    *out++ = '"';
-    while (*in) {
-	if (*in == '"')
-	    *out++ = '"';
-	*out++ = *in++;
-    }
-    *out++ = '"';
-    *out = 0;
-    return buf;
-}
-
-static char *
 _optimize_tag_query (void *ctx, const char *orig_query_string,
 		     const tag_op_list_t *list)
 {
@@ -67,44 +49,44 @@ _optimize_tag_query (void *ctx, const char *orig_query_string,
      * parenthesize and the exclusion part of the query must not use
      * the '-' operator (though the NOT operator is fine). */
 
-    char *escaped, *query_string;
+    char *escaped = NULL;
+    size_t escaped_len = 0;
+    char *query_string;
     const char *join = "";
     size_t i;
-    unsigned int max_tag_len = 0;
 
     /* Don't optimize if there are no tag changes. */
     if (tag_op_list_size (list) == 0)
 	return talloc_strdup (ctx, orig_query_string);
 
-    /* Allocate a buffer for escaping tags.  This is large enough to
-     * hold a fully escaped tag with every character doubled plus
-     * enclosing quotes and a NUL. */
-    for (i = 0; i < tag_op_list_size (list); i++)
-	if (strlen (tag_op_list_tag (list, i)) > max_tag_len)
-	    max_tag_len = strlen (tag_op_list_tag (list, i));
-
-    escaped = talloc_array (ctx, char, max_tag_len * 2 + 3);
-    if (! escaped)
-	return NULL;
-
     /* Build the new query string */
     if (strcmp (orig_query_string, "*") == 0)
 	query_string = talloc_strdup (ctx, "(");
     else
 	query_string = talloc_asprintf (ctx, "( %s ) and (", orig_query_string);
 
+
+    /* Boolean terms surrounded by double quotes can contain any
+     * character.  Double quotes are quoted by doubling them. */
+
     for (i = 0; i < tag_op_list_size (list) && query_string; i++) {
+	double_quote_str (ctx,
+			  tag_op_list_tag (list, i),
+			  &escaped, &escaped_len);
+
 	query_string = talloc_asprintf_append_buffer (
 	    query_string, "%s%stag:%s", join,
 	    tag_op_list_isremove (list, i) ? "" : "not ",
-	    _escape_tag (escaped, tag_op_list_tag (list, i)));
+	    escaped);
 	join = " or ";
     }
 
     if (query_string)
 	query_string = talloc_strdup_append_buffer (query_string, ")");
 
-    talloc_free (escaped);
+    if (escaped)
+	talloc_free (escaped);
+
     return query_string;
 }
 
diff --git a/util/string-util.c b/util/string-util.c
index 44f8cd3..ea7c25b 100644
--- a/util/string-util.c
+++ b/util/string-util.c
@@ -20,6 +20,7 @@
 
 
 #include "string-util.h"
+#include "talloc.h"
 
 char *
 strtok_len (char *s, const char *delim, size_t *len)
@@ -32,3 +33,36 @@ strtok_len (char *s, const char *delim, size_t *len)
 
     return *len ? s : NULL;
 }
+
+
+int
+double_quote_str (void *ctx, const char *str,
+		  char **buf, size_t *len)
+{
+    const char *in;
+    char *out;
+    size_t needed = 3;
+
+    for (in = str; *in; in++)
+	needed += (*in == '"') ? 2 : 1;
+
+    if (needed > *len)
+	*buf = talloc_realloc (ctx, *buf, char, 2*needed);
+
+    if (! *buf)
+	return 1;
+
+    out = *buf;
+
+    *out++ = '"';
+    in = str;
+    while (*in) {
+	if (*in == '"')
+	    *out++ = '"';
+	*out++ = *in++;
+    }
+    *out++ = '"';
+    *out = 0;
+
+    return 0;
+}
diff --git a/util/string-util.h b/util/string-util.h
index ac7676c..b593bc7 100644
--- a/util/string-util.h
+++ b/util/string-util.h
@@ -19,4 +19,12 @@
 
 char *strtok_len (char *s, const char *delim, size_t *len);
 
+/* Copy str to dest, surrounding with double quotes.
+ * Any internal double-quotes are doubled, i.e. a"b -> "a""b"
+ *
+ * Output is into buf; it may be talloc_realloced
+ * return 0 on success, non-zero on failure.
+ */
+int double_quote_str (void *talloc_ctx, const char *str,
+		      char **buf, size_t *len);
 #endif
-- 
1.7.10.4

[Patch v7 05/14] quote_and_decode_query: new function to quote hex-decoded queries

[david]

From: David Bremner <bremner@debian.org>

The query is split into tokens, with ' ' and ':' as delimiters.  Any
token containing some hex-escaped character is quoted according to
Xapian rules.  This maps id:foo%20%22bar to id:"foo ""bar".
This intentionally does not quote prefixes, so they still work as prefixes.
---
 tag-util.c |   50 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 50 insertions(+)

diff --git a/tag-util.c b/tag-util.c
index f89669a..e1181f8 100644
--- a/tag-util.c
+++ b/tag-util.c
@@ -56,6 +56,56 @@ illegal_tag (const char *tag, notmuch_bool_t remove)
     return NULL;
 }
 
+static tag_parse_status_t
+quote_and_decode_query (void *ctx, char *encoded, const char *line_for_error,
+			char **query_string)
+{
+    char *tok = encoded;
+    size_t tok_len = 0;
+    char *buf = NULL;
+    size_t buf_len = 0;
+    tag_parse_status_t ret = TAG_PARSE_SUCCESS;
+
+    *query_string = talloc_strdup (ctx, "");
+
+    while (*query_string &&
+	   (tok = strtok_len (tok + tok_len, ": ", &tok_len)) != NULL) {
+	char delim = tok[tok_len];
+
+	*(tok + tok_len++) = '\0';
+
+	if (strcspn (tok, "%") < tok_len - 1) {
+	    /* something to decode */
+	    if (hex_decode_inplace (tok) != HEX_SUCCESS) {
+		ret = line_error (TAG_PARSE_INVALID, line_for_error,
+				  "hex decoding of token '%s' failed", tok);
+		goto DONE;
+	    }
+
+	    if (double_quote_str (ctx, tok, &buf, &buf_len)) {
+		ret = line_error (TAG_PARSE_OUT_OF_MEMORY,
+				  line_for_error, "aborting");
+		goto DONE;
+	    }
+	    *query_string = talloc_asprintf_append_buffer (
+		*query_string, "%s%c", buf, delim);
+
+	} else {
+	    /* This is not just an optimization, but used to preserve
+	     * prefixes like id:, which cannot be quoted.
+	     */
+	    *query_string = talloc_asprintf_append_buffer (
+		*query_string, "%s%c", tok, delim);
+	}
+
+    }
+
+  DONE:
+    if (ret != TAG_PARSE_SUCCESS && *query_string)
+	talloc_free (*query_string);
+    return ret;
+}
+
 tag_parse_status_t
 parse_tag_line (void *ctx, char *line,
 		tag_op_flag_t flags,
-- 
1.7.10.4

[Patch v7 06/14] notmuch-restore: move query handling for batch restore to parser

[david]

From: David Bremner <bremner@debian.org>

We are able to detect more errors by looking at the string before it
is hex-decoded. We also need this to avoid the query quoting for more
general queries (to be written) that will mess up raw message-ids.
---
 notmuch-restore.c |   18 +-----------------
 tag-util.c        |   26 ++++++++++++++++++++------
 tag-util.h        |    5 ++++-
 test/dump-restore |    5 ++---
 4 files changed, 27 insertions(+), 27 deletions(-)

diff --git a/notmuch-restore.c b/notmuch-restore.c
index 40596a8..112f2f3 100644
--- a/notmuch-restore.c
+++ b/notmuch-restore.c
@@ -208,24 +208,8 @@ notmuch_restore_command (unused (void *ctx), int argc, char *argv[])
 	if (input_format == DUMP_FORMAT_SUP) {
 	    ret = parse_sup_line (ctx, line, &query_string, tag_ops);
 	} else {
-	    ret = parse_tag_line (ctx, line, TAG_FLAG_BE_GENEROUS,
+	    ret = parse_tag_line (ctx, line, TAG_FLAG_BE_GENEROUS | TAG_FLAG_ID_ONLY,
 				  &query_string, tag_ops);
-
-	    if (ret == 0) {
-		if (strncmp ("id:", query_string, 3) != 0) {
-		    fprintf (stderr, "Warning: unsupported query: %s\n", query_string);
-		    continue;
-		}
-		/* delete id: from front of string; tag_message
-		 * expects a raw message-id.
-		 *
-		 * XXX: Note that query string id:foo and bar will be
-		 * interpreted as a message id "foo and bar". This
-		 * should eventually be fixed to give a better error
-		 * message.
-		 */
-		query_string = query_string + 3;
-	    }
 	}
 
 	if (ret > 0)
diff --git a/tag-util.c b/tag-util.c
index e1181f8..8fea76c 100644
--- a/tag-util.c
+++ b/tag-util.c
@@ -201,14 +201,28 @@ parse_tag_line (void *ctx, char *line,
     }
 
     /* tok now points to the query string */
-    if (hex_decode_inplace (tok) != HEX_SUCCESS) {
-	ret = line_error (TAG_PARSE_INVALID, line_for_error,
-			  "hex decoding of query %s failed", tok);
-	goto DONE;
+    if (flags & TAG_FLAG_ID_ONLY) {
+	/* this is under the assumption that any whitespace in the
+	 * message-id must be hex-encoded. The check is probably not
+	 * perfect for exotic unicode whitespace; as fallback the
+	 * search for strange message-ids will fail */
+	if ((strncmp ("id:", tok, 3) != 0) ||
+	    (strcspn (tok, " \t") < strlen (tok))) {
+	    ret = line_error (TAG_PARSE_INVALID, line_for_error,
+			      "query '%s' is not 'id:<message-id>'", tok);
+	    goto DONE;
+	}
+	if (hex_decode_inplace (tok) != HEX_SUCCESS) {
+	    ret = line_error (TAG_PARSE_INVALID, line_for_error,
+			      "hex decoding of query %s failed", tok);
+	    goto DONE;
+	}
+	/* skip 'id:' */
+	*query_string = tok + 3;
+    } else {
+	ret = quote_and_decode_query (ctx, tok, line_for_error, query_string);
     }
 
-    *query_string = tok;
-
   DONE:
     talloc_free (line_for_error);
     return ret;
diff --git a/tag-util.h b/tag-util.h
index 2889736..7674051 100644
--- a/tag-util.h
+++ b/tag-util.h
@@ -26,7 +26,10 @@ typedef enum {
     /* Accept strange tags that might be user error;
      * intended for use by notmuch-restore.
      */
-    TAG_FLAG_BE_GENEROUS = (1 << 3)
+    TAG_FLAG_BE_GENEROUS = (1 << 3),
+
+    /* Query consists of a single id:$message-id */
+    TAG_FLAG_ID_ONLY = (1 << 4)
 
 } tag_op_flag_t;
 
diff --git a/test/dump-restore b/test/dump-restore
index 6a989b6..eb7933a 100755
--- a/test/dump-restore
+++ b/test/dump-restore
@@ -199,19 +199,18 @@ a
 # the next non-comment line should report an an empty tag error for
 # batch tagging, but not for restore
 + +e -- id:20091117232137.GA7669@griffis1.net
-# highlight the sketchy id parsing; this should be last
 +g -- id:foo and bar
 EOF
 
 cat <<EOF > EXPECTED
-Warning: unsupported query: a
+Warning: query 'a' is not 'id:<message-id>' [a]
 Warning: no query string [+0]
 Warning: no query string [+a +b]
 Warning: missing query string [+a +b ]
 Warning: no query string after -- [+c +d --]
 Warning: hex decoding of tag %zz failed [+%zz -- id:whatever]
 Warning: hex decoding of query id:%yy failed [+e +f id:%yy]
-Warning: cannot apply tags to missing message: foo and bar
+Warning: query 'id:foo and bar' is not 'id:<message-id>' [+g -- id:foo and bar]
 EOF
 
 test_expect_equal_file EXPECTED OUTPUT
-- 
1.7.10.4

[Patch v7 07/14] cli: add support for batch tagging operations to "notmuch tag"

[david]

From: Jani Nikula <jani@nikula.org>

Add support for batch tagging operations through stdin to "notmuch
tag". This can be enabled with the new --batch command line option to
"notmuch tag". The input must consist of lines of the format:

+<tag>|-<tag> [...] [--] <search-term> [...]

Each line is interpreted similarly to "notmuch tag" command line
arguments. The delimiter is one or more spaces ' '. Any characters in
<tag> and <search-term> MAY be hex encoded with %NN where NN is the
hexadecimal value of the character. Any ' ' and '%' characters in
<tag> and <search-term> MUST be hex encoded (using %20 and %25,
respectively). Any characters that are not part of <tag> or
<search-term> MUST NOT be hex encoded.

Leading and trailing space ' ' is ignored. Empty lines and lines
beginning with '#' are ignored.

Signed-off-by: Jani Nikula <jani@nikula.org>

Hacked-like-crazy-by: David Bremner <david@tethera.net>
---
 notmuch-tag.c |   88 +++++++++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 80 insertions(+), 8 deletions(-)

diff --git a/notmuch-tag.c b/notmuch-tag.c
index 13f2268..a81d911 100644
--- a/notmuch-tag.c
+++ b/notmuch-tag.c
@@ -130,6 +130,43 @@ tag_query (void *ctx, notmuch_database_t *notmuch, const char *query_string,
     return interrupted;
 }
 
+static int
+tag_file (void *ctx, notmuch_database_t *notmuch, tag_op_flag_t flags,
+	  FILE *input)
+{
+    char *line = NULL;
+    char *query_string = NULL;
+    size_t line_size = 0;
+    ssize_t line_len;
+    int ret = 0;
+    tag_op_list_t *tag_ops;
+
+    tag_ops = tag_op_list_create (ctx);
+    if (tag_ops == NULL) {
+	fprintf (stderr, "Out of memory.\n");
+	return 1;
+    }
+
+    while ((line_len = getline (&line, &line_size, input)) != -1 &&
+	   ! interrupted) {
+
+	ret = parse_tag_line (ctx, line, TAG_FLAG_NONE,
+			      &query_string, tag_ops);
+
+	if (ret > 0)
+	    continue;
+
+	if (ret < 0 || tag_query (ctx, notmuch, query_string,
+				  tag_ops, flags))
+	    break;
+    }
+
+    if (line)
+	free (line);
+
+    return ret;
+}
+
 int
 notmuch_tag_command (void *ctx, int argc, char *argv[])
 {
@@ -139,6 +176,10 @@ notmuch_tag_command (void *ctx, int argc, char *argv[])
     notmuch_database_t *notmuch;
     struct sigaction action;
     tag_op_flag_t tag_flags = TAG_FLAG_NONE;
+    notmuch_bool_t batch = FALSE;
+    FILE *input = stdin;
+    char *input_file_name = NULL;
+    int opt_index;
     int ret = 0;
 
     /* Setup our handler for SIGINT */
@@ -148,15 +189,43 @@ notmuch_tag_command (void *ctx, int argc, char *argv[])
     action.sa_flags = SA_RESTART;
     sigaction (SIGINT, &action, NULL);
 
-    tag_ops = tag_op_list_create (ctx);
-    if (tag_ops == NULL) {
-	fprintf (stderr, "Out of memory.\n");
+    notmuch_opt_desc_t options[] = {
+	{ NOTMUCH_OPT_BOOLEAN, &batch, "batch", 0, 0 },
+	{ NOTMUCH_OPT_STRING, &input_file_name, "input", 'i', 0 },
+	{ 0, 0, 0, 0, 0 }
+    };
+
+    opt_index = parse_arguments (argc, argv, options, 1);
+    if (opt_index < 0)
 	return 1;
+
+    if (input_file_name) {
+	batch = TRUE;
+	input = fopen (input_file_name, "r");
+	if (input == NULL) {
+	    fprintf (stderr, "Error opening %s for reading: %s\n",
+		     input_file_name, strerror (errno));
+	    return 1;
+	}
     }
 
-    if (parse_tag_command_line (ctx, argc - 1, argv + 1,
-				&query_string, tag_ops))
-	return 1;
+    if (batch) {
+	if (opt_index != argc) {
+	    fprintf (stderr, "Can't specify both cmdline and stdin!\n");
+	    return 1;
+	}
+    } else {
+
+	tag_ops = tag_op_list_create (ctx);
+	if (tag_ops == NULL) {
+	    fprintf (stderr, "Out of memory.\n");
+	    return 1;
+	}
+
+	if (parse_tag_command_line (ctx, argc - opt_index, argv + opt_index,
+				    &query_string, tag_ops))
+	    return 1;
+    }
 
     config = notmuch_config_open (ctx, NULL, NULL);
     if (config == NULL)
@@ -169,9 +238,12 @@ notmuch_tag_command (void *ctx, int argc, char *argv[])
     if (notmuch_config_get_maildir_synchronize_flags (config))
 	tag_flags |= TAG_FLAG_MAILDIR_SYNC;
 
-    ret = tag_query (ctx, notmuch, query_string, tag_ops, tag_flags);
+    if (batch)
+	ret = tag_file (ctx, notmuch, tag_flags, input);
+    else
+	ret = tag_query (ctx, notmuch, query_string, tag_ops, tag_flags);
 
     notmuch_database_destroy (notmuch);
 
-    return ret;
+    return ret || interrupted;
 }
-- 
1.7.10.4

[Patch v7 08/14] test/tagging: add test for error messages of tag --batch

[david]

From: David Bremner <bremner@debian.org>

This is based on the similar test for notmuch restore, but the parser
in batch tagging mode is less tolerant of a few cases, in particular
those tested by illegal_tag.
---
 test/tagging |   37 +++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/test/tagging b/test/tagging
index 980ff92..30cec48 100755
--- a/test/tagging
+++ b/test/tagging
@@ -46,6 +46,43 @@ test_expect_equal "$output" "\
 thread:XXX   2001-01-05 [1/1] Notmuch Test Suite; One (:\"  inbox tag1 unread)
 thread:XXX   2001-01-05 [1/1] Notmuch Test Suite; Two (inbox tag1 tag4 unread)"
 
+test_begin_subtest '--batch: checking error messages'
+notmuch dump --format=batch-tag > BACKUP
+notmuch tag --batch <<EOF 2>OUTPUT
+# the next line has a space
+ 
+# this line has no tag operations, but this is permitted in batch format.
+a
++0
++a +b
+# trailing whitespace
++a +b 
++c +d --
+# this is a harmless comment, do not yell about it.
+
+# the previous line was blank; also no yelling please
++%zz -- id:whatever
++e +f id:%yy
+# the next non-comment line should report an an empty tag error for
+# batch tagging, but not for restore
++ +e -- id:foo
++- -- id:foo
+EOF
+
+cat <<EOF > EXPECTED
+Warning: no query string [+0]
+Warning: no query string [+a +b]
+Warning: missing query string [+a +b ]
+Warning: no query string after -- [+c +d --]
+Warning: hex decoding of tag %zz failed [+%zz -- id:whatever]
+Warning: hex decoding of token '%yy' failed [+e +f id:%yy]
+Warning: empty tag forbidden [+ +e -- id:foo]
+Warning: tag starting with '-' forbidden [+- -- id:foo]
+EOF
+
+notmuch restore --format=batch-tag < BACKUP
+test_expect_equal_file EXPECTED OUTPUT
+
 test_expect_code 1 "Empty tag names" 'notmuch tag + One'
 
 test_expect_code 1 "Tag name beginning with -" 'notmuch tag +- One'
-- 
1.7.10.4

[Patch v7 09/14] test/tagging: add basic tests for batch tagging functionality

[david]

From: David Bremner <bremner@debian.org>

This tests argument parsing, blank lines and comments, and basic hex
decoding functionality.
---
 test/tagging |   51 +++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 51 insertions(+)

diff --git a/test/tagging b/test/tagging
index 30cec48..b454d38 100755
--- a/test/tagging
+++ b/test/tagging
@@ -46,6 +46,57 @@ test_expect_equal "$output" "\
 thread:XXX   2001-01-05 [1/1] Notmuch Test Suite; One (:\"  inbox tag1 unread)
 thread:XXX   2001-01-05 [1/1] Notmuch Test Suite; Two (inbox tag1 tag4 unread)"
 
+test_begin_subtest "--batch"
+notmuch tag --batch <<EOF
+# %20 is a space in tag
+-:"%20 -tag1 +tag5 +tag6 -- One
++tag1 -tag1 -tag4 +tag4 -- Two
+-tag6 One
++tag5 Two
+EOF
+output=$(notmuch search \* | notmuch_search_sanitize)
+test_expect_equal "$output" "\
+thread:XXX   2001-01-05 [1/1] Notmuch Test Suite; One (inbox tag5 unread)
+thread:XXX   2001-01-05 [1/1] Notmuch Test Suite; Two (inbox tag4 tag5 unread)"
+
+# generate a common input file for the next several tests.
+cat > batch.in  <<EOF
+# %40 is an @ in tag
++%40 -tag5 +tag6 -- One
++tag1 -tag1 -tag4 +tag4 -- Two
+-tag5 +tag6 Two
+EOF
+
+cat > batch.expected <<EOF
+thread:XXX   2001-01-05 [1/1] Notmuch Test Suite; One (@ inbox tag6 unread)
+thread:XXX   2001-01-05 [1/1] Notmuch Test Suite; Two (inbox tag4 tag6 unread)
+EOF
+
+test_begin_subtest "--input"
+notmuch dump --format=batch-tag > backup.tags
+notmuch tag --input=batch.in
+notmuch search \* | notmuch_search_sanitize > OUTPUT
+notmuch restore --format=batch-tag < backup.tags
+test_expect_equal_file batch.expected OUTPUT
+
+test_begin_subtest "--batch --input"
+notmuch dump --format=batch-tag > backup.tags
+notmuch tag --batch --input=batch.in
+notmuch search \* | notmuch_search_sanitize > OUTPUT
+notmuch restore --format=batch-tag < backup.tags
+test_expect_equal_file batch.expected OUTPUT
+
+test_begin_subtest "--batch, blank lines and comments"
+notmuch dump | sort > EXPECTED
+notmuch tag --batch <<EOF
+# this line is a comment; the next has only white space
+ 	 
+
+# the previous line is empty
+EOF
+notmuch dump | sort > OUTPUT
+test_expect_equal_file EXPECTED OUTPUT
+
 test_begin_subtest '--batch: checking error messages'
 notmuch dump --format=batch-tag > BACKUP
 notmuch tag --batch <<EOF 2>OUTPUT
-- 
1.7.10.4

[Patch v7 10/14] test/tagging: add tests for exotic tags

[david]

From: David Bremner <bremner@debian.org>

We test quotes seperately because they matter to the query escaper.
---
 test/tagging |   66 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 66 insertions(+)

diff --git a/test/tagging b/test/tagging
index b454d38..18f532e 100755
--- a/test/tagging
+++ b/test/tagging
@@ -134,6 +134,72 @@ EOF
 notmuch restore --format=batch-tag < BACKUP
 test_expect_equal_file EXPECTED OUTPUT
 
+test_begin_subtest '--batch: tags with quotes'
+notmuch dump --format=batch-tag > BACKUP
+
+notmuch tag --batch <<EOF
++%22%27%22%27%22%22%27%27 -- One
+-%22%27%22%27%22%22%27%27 -- One
++%22%27%22%22%22%27 -- One
++%22%27%22%27%22%22%27%27 -- Two
+EOF
+
+cat <<EOF > EXPECTED
++%22%27%22%22%22%27 +inbox +tag5 +unread -- id:msg-001@notmuch-test-suite
++%22%27%22%27%22%22%27%27 +inbox +tag4 +tag5 +unread -- id:msg-002@notmuch-test-suite
+EOF
+
+notmuch dump --format=batch-tag | sort > OUTPUT
+notmuch restore --format=batch-tag < BACKUP
+test_expect_equal_file EXPECTED OUTPUT
+
+test_begin_subtest '--batch: tags with punctuation and space'
+notmuch dump --format=batch-tag > BACKUP
+
+notmuch tag --batch <<EOF
++%21@%23%24%25%5e%26%2a%29-_=+%5b%7b%5c%20%7c%3b%3a%27%20%22,.%3c%60%7e -- One
+-%21@%23%24%25%5e%26%2a%29-_=+%5b%7b%5c%20%7c%3b%3a%27%20%22,.%3c%60%7e -- One
++%21@%23%24%25%5e%26%2a%29-_=+%5b%7b%5c%20%7c%3b%3a%27%20%22,.%3c%20%60%7e -- Two
+-%21@%23%24%25%5e%26%2a%29-_=+%5b%7b%5c%20%7c%3b%3a%27%20%22,.%3c%20%60%7e -- Two
++%21@%23%20%24%25%5e%26%2a%29-_=+%5b%7b%5c%20%7c%3b%3a%27%20%22,.%3c%60%7e -- One
++%21@%23%20%24%25%5e%26%2a%29-_=+%5b%7b%5c%20%7c%3b%3a%27%20%22,.%3c%60%7e -- Two
+EOF
+
+cat <<EOF > EXPECTED
++%21@%23%20%24%25%5e%26%2a%29-_=+%5b%7b%5c%20%7c%3b%3a%27%20%22,.%3c%60%7e +inbox +tag4 +tag5 +unread -- id:msg-002@notmuch-test-suite
++%21@%23%20%24%25%5e%26%2a%29-_=+%5b%7b%5c%20%7c%3b%3a%27%20%22,.%3c%60%7e +inbox +tag5 +unread -- id:msg-001@notmuch-test-suite
+EOF
+
+notmuch dump --format=batch-tag | sort > OUTPUT
+notmuch restore --format=batch-tag < BACKUP
+test_expect_equal_file EXPECTED OUTPUT
+
+test_begin_subtest '--batch: unicode tags'
+notmuch dump --format=batch-tag > BACKUP
+
+notmuch tag --batch <<EOF
++%2a@%7d%cf%b5%f4%85%80%adO3%da%a7 -- One
++=%e0%ac%95%c8%b3+%ef%aa%95%c8%a64w%c7%9d%c9%a2%cf%b3%d6%82%24B%c4%a9%c5%a1UX%ee%99%b0%27E7%ca%a4%d0%8b%5d -- One
++A%e1%a0%bc%de%8b%d5%b2V%d9%9b%f3%b5%a2%a3M%d8%a1u@%f0%a0%ac%948%7e%f0%ab%86%af%27 -- One
++R -- One
++%da%88=f%cc%b9I%ce%af%7b%c9%97%e3%b9%8bH%cb%92X%d2%8c6 -- One
++%dc%9crh%d2%86B%e5%97%a2%22t%ed%99%82d -- One
++L%df%85%ef%a1%a5m@%d3%96%c2%ab%d4%9f%ca%b8%f3%b3%a2%bf%c7%b1_u%d7%b4%c7%b1 -- One
++P%c4%98%2f -- One
++%7e%d1%8b%25%ec%a0%ae%d1%a0M%3b%e3%b6%b7%e9%a4%87%3c%db%9a%cc%a8%e1%96%9d -- One
++%c4%bf7%c7%ab9H%c4%99k%ea%91%bd%c3%8ck%e2%b3%8dk%c5%952V%e4%99%b2%d9%b3%e4%8b%bda%5b%24%c7%9b -- One
++%2a@%7d%cf%b5%f4%85%80%adO3%da%a7  +=%e0%ac%95%c8%b3+%ef%aa%95%c8%a64w%c7%9d%c9%a2%cf%b3%d6%82%24B%c4%a9%c5%a1UX%ee%99%b0%27E7%ca%a4%d0%8b%5d  +A%e1%a0%bc%de%8b%d5%b2V%d9%9b%f3%b5%a2%a3M%d8%a1u@%f0%a0%ac%948%7e%f0%ab%86%af%27  +R  +%da%88=f%cc%b9I%ce%af%7b%c9%97%e3%b9%8bH%cb%92X%d2%8c6  +%dc%9crh%d2%86B%e5%97%a2%22t%ed%99%82d  +L%df%85%ef%a1%a5m@%d3%96%c2%ab%d4%9f%ca%b8%f3%b3%a2%bf%c7%b1_u%d7%b4%c7%b1  +P%c4%98%2f  +%7e%d1%8b%25%ec%a0%ae%d1%a0M%3b%e3%b6%b7%e9%a4%87%3c%db%9a%cc%a8%e1%96%9d  +%c4%bf7%c7%ab9H%c4%99k%ea%91%bd%c3%8ck%e2%b3%8dk%c5%952V%e4%99%b2%d9%b3%e4%8b%bda%5b%24%c7%9b -- Two
+EOF
+
+cat <<EOF > EXPECTED
++%2a@%7d%cf%b5%f4%85%80%adO3%da%a7 +=%e0%ac%95%c8%b3+%ef%aa%95%c8%a64w%c7%9d%c9%a2%cf%b3%d6%82%24B%c4%a9%c5%a1UX%ee%99%b0%27E7%ca%a4%d0%8b%5d +A%e1%a0%bc%de%8b%d5%b2V%d9%9b%f3%b5%a2%a3M%d8%a1u@%f0%a0%ac%948%7e%f0%ab%86%af%27 +L%df%85%ef%a1%a5m@%d3%96%c2%ab%d4%9f%ca%b8%f3%b3%a2%bf%c7%b1_u%d7%b4%c7%b1 +P%c4%98%2f +R +inbox +tag4 +tag5 +unread +%7e%d1%8b%25%ec%a0%ae%d1%a0M%3b%e3%b6%b7%e9%a4%87%3c%db%9a%cc%a8%e1%96%9d +%c4%bf7%c7%ab9H%c4%99k%ea%91%bd%c3%8ck%e2%b3%8dk%c5%952V%e4%99%b2%d9%b3%e4%8b%bda%5b%24%c7%9b +%da%88=f%cc%b9I%ce%af%7b%c9%97%e3%b9%8bH%cb%92X%d2%8c6 +%dc%9crh%d2%86B%e5%97%a2%22t%ed%99%82d -- id:msg-002@notmuch-test-suite
++%2a@%7d%cf%b5%f4%85%80%adO3%da%a7 +=%e0%ac%95%c8%b3+%ef%aa%95%c8%a64w%c7%9d%c9%a2%cf%b3%d6%82%24B%c4%a9%c5%a1UX%ee%99%b0%27E7%ca%a4%d0%8b%5d +A%e1%a0%bc%de%8b%d5%b2V%d9%9b%f3%b5%a2%a3M%d8%a1u@%f0%a0%ac%948%7e%f0%ab%86%af%27 +L%df%85%ef%a1%a5m@%d3%96%c2%ab%d4%9f%ca%b8%f3%b3%a2%bf%c7%b1_u%d7%b4%c7%b1 +P%c4%98%2f +R +inbox +tag5 +unread +%7e%d1%8b%25%ec%a0%ae%d1%a0M%3b%e3%b6%b7%e9%a4%87%3c%db%9a%cc%a8%e1%96%9d +%c4%bf7%c7%ab9H%c4%99k%ea%91%bd%c3%8ck%e2%b3%8dk%c5%952V%e4%99%b2%d9%b3%e4%8b%bda%5b%24%c7%9b +%da%88=f%cc%b9I%ce%af%7b%c9%97%e3%b9%8bH%cb%92X%d2%8c6 +%dc%9crh%d2%86B%e5%97%a2%22t%ed%99%82d -- id:msg-001@notmuch-test-suite
+EOF
+
+notmuch dump --format=batch-tag | sort > OUTPUT
+notmuch restore --format=batch-tag < BACKUP
+test_expect_equal_file EXPECTED OUTPUT
+
 test_expect_code 1 "Empty tag names" 'notmuch tag + One'
 
 test_expect_code 1 "Tag name beginning with -" 'notmuch tag +- One'
-- 
1.7.10.4

[Patch v7 11/14] test/tagging: add test for exotic message-ids and batch tagging

[david]

From: David Bremner <bremner@debian.org>

The (now fixed) bug that this test revealed is that unquoted
message-ids with whitespace or other control characters in them are
split into several tokens by the Xapian query parser.
---
 test/tagging |   18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/test/tagging b/test/tagging
index 18f532e..1d59af0 100755
--- a/test/tagging
+++ b/test/tagging
@@ -200,6 +200,24 @@ notmuch dump --format=batch-tag | sort > OUTPUT
 notmuch restore --format=batch-tag < BACKUP
 test_expect_equal_file EXPECTED OUTPUT
 
+test_begin_subtest '--batch: unicode message-ids'
+
+${TEST_DIRECTORY}/random-corpus --config-path=${NOTMUCH_CONFIG} \
+     --num-messages=100
+
+notmuch dump --format=batch-tag | sed 's/^.* -- /+common_tag -- /' | \
+    sort > EXPECTED
+
+notmuch dump --format=batch-tag | sed 's/^.* -- /  -- /' | \
+    notmuch restore --format=batch-tag
+
+notmuch tag --batch < EXPECTED
+
+notmuch dump --format=batch-tag| \
+    sort > OUTPUT
+
+test_expect_equal_file EXPECTED OUTPUT
+
 test_expect_code 1 "Empty tag names" 'notmuch tag + One'
 
 test_expect_code 1 "Tag name beginning with -" 'notmuch tag +- One'
-- 
1.7.10.4

[Patch v7 12/14] test/tagging: add test for compound queries with batch tagging

[david]

From: David Bremner <bremner@debian.org>

This is to watch for errors in quoting the query.
---
 test/tagging |   22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/test/tagging b/test/tagging
index 1d59af0..bd8eeb5 100755
--- a/test/tagging
+++ b/test/tagging
@@ -174,6 +174,28 @@ notmuch dump --format=batch-tag | sort > OUTPUT
 notmuch restore --format=batch-tag < BACKUP
 test_expect_equal_file EXPECTED OUTPUT
 
+test_begin_subtest '--batch: compound queries'
+notmuch dump --format=batch-tag > BACKUP
+
+notmuch tag --batch <<EOF
++compound1 -- One and Two
++compound2 -- One or Two
++tag%20with%20spaces -- *
++compound3 -- id:msg-002@notmuch-test-suite and Two
++compound3 -- Two and id:msg-002@notmuch-test-suite
+-unread -- tag:tag%20with%20spaces and id:msg-002@notmuch-test-suite
+-tag%20with%20spaces -- tag:unread and tag:compound2
+EOF
+
+cat <<EOF > EXPECTED
++compound2 +compound3 +inbox +tag%20with%20spaces +tag4 +tag5 -- id:msg-002@notmuch-test-suite
++compound2 +inbox +tag5 +unread -- id:msg-001@notmuch-test-suite
+EOF
+
+notmuch dump --format=batch-tag | sort > OUTPUT
+notmuch restore --format=batch-tag < BACKUP
+test_expect_equal_file EXPECTED OUTPUT
+
 test_begin_subtest '--batch: unicode tags'
 notmuch dump --format=batch-tag > BACKUP
 
-- 
1.7.10.4

[Patch v7 13/14] notmuch-tag.1: tidy synopsis formatting

[david]

From: David Bremner <bremner@debian.org>

Consistently use [...]; one less space.
---
 man/man1/notmuch-tag.1 |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/man/man1/notmuch-tag.1 b/man/man1/notmuch-tag.1
index 0f86582..d23700d 100644
--- a/man/man1/notmuch-tag.1
+++ b/man/man1/notmuch-tag.1
@@ -4,7 +4,7 @@ notmuch-tag \- add/remove tags for all messages matching the search terms
 
 .SH SYNOPSIS
 .B notmuch tag
-.RI  "+<" tag> "|\-<" tag "> [...] [\-\-] <" search-term ">..."
+.RI "+<" tag ">|\-<" tag "> [...] [\-\-] <" search-term "> [...]"
 
 .SH DESCRIPTION
 
-- 
1.7.10.4

[Patch v7 14/14] man: document notmuch tag --batch, --input options

[david]

From: Jani Nikula <jani@nikula.org>

---
 man/man1/notmuch-tag.1 |   50 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 50 insertions(+)

diff --git a/man/man1/notmuch-tag.1 b/man/man1/notmuch-tag.1
index d23700d..c4e60d3 100644
--- a/man/man1/notmuch-tag.1
+++ b/man/man1/notmuch-tag.1
@@ -6,6 +6,11 @@ notmuch-tag \- add/remove tags for all messages matching the search terms
 .B notmuch tag
 .RI "+<" tag ">|\-<" tag "> [...] [\-\-] <" search-term "> [...]"
 
+.B notmuch tag
+.RI "--batch"
+.RI "[ --input=<" filename "> ]"
+
+
 .SH DESCRIPTION
 
 Add/remove tags for all messages matching the search terms.
@@ -29,6 +34,51 @@ updates the maildir flags according to tag changes if the
 configuration option is enabled. See \fBnotmuch-config\fR(1) for
 details.
 
+Supported options for
+.B tag
+include
+.RS 4
+.TP 4
+.BR \-\-batch
+
+Read batch tagging operations from a file (stdin by default). This is more
+efficient than repeated
+.B notmuch tag
+invocations. See
+.B TAG FILE FORMAT
+below for the input format. This option is not compatible with
+specifying tagging on the command line.
+.RE
+
+.RS 4
+.TP 4
+.BR "\-\-input=" <filename>
+
+Read input from given file, instead of from stdin. Implies
+.BR --batch .
+
+.SH TAG FILE FORMAT
+
+The input must consist of lines of the format:
+
+.RI "+<" tag ">|\-<" tag "> [...] [\-\-] <" search-term "> [...]"
+
+Each line is interpreted similarly to
+.B notmuch tag
+command line arguments. The delimiter is one or more spaces ' '. Any
+characters in <tag> and <search-term>
+.B may
+be hex encoded with %NN where NN is the hexadecimal value of the
+character. Any ' ' and '%' characters in <tag> and <search-terms>
+.B must
+be hex encoded (using %20 and %25, respectively). Any characters that
+are not part of <tag> or <search-terms>
+.B must not
+be hex encoded.
+
+Leading and trailing space ' ' is ignored. Empty lines and lines
+beginning with '#' are ignored.
+
 .SH SEE ALSO
 
 \fBnotmuch\fR(1), \fBnotmuch-config\fR(1), \fBnotmuch-count\fR(1),
-- 
1.7.10.4

Re: [Patch v7 05/14] quote_and_decode_query: new function to quote hex-decoded queries

[Mark Walters]

On Fri, 14 Dec 2012, david@tethera.net wrote:
> From: David Bremner <bremner@debian.org>
>
> The query is split into tokens, with ' ' and ':' as delimiters.  Any
> token containing some hex-escaped character is quoted according to
> Xapian rules.  This maps id:foo%20%22bar to id:"foo ""bar".
> This intentionally does not quote prefixes, so they still work as prefixes.
> ---
>  tag-util.c |   50 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 50 insertions(+)
>
> diff --git a/tag-util.c b/tag-util.c
> index f89669a..e1181f8 100644
> --- a/tag-util.c
> +++ b/tag-util.c
> @@ -56,6 +56,56 @@ illegal_tag (const char *tag, notmuch_bool_t remove)
>      return NULL;
>  }
>  
> +static tag_parse_status_t
> +quote_and_decode_query (void *ctx, char *encoded, const char *line_for_error,
> +			char **query_string)
> +{

Would decode_and_quote_query be a better name given the order these two
happen? Also a comment describing the function would be nice.

> +    char *tok = encoded;
> +    size_t tok_len = 0;
> +    char *buf = NULL;
> +    size_t buf_len = 0;
> +    tag_parse_status_t ret = TAG_PARSE_SUCCESS;
> +
> +    *query_string = talloc_strdup (ctx, "");
> +
> +    while (*query_string &&
> +	   (tok = strtok_len (tok + tok_len, ": ", &tok_len)) != NULL) {
> +	char delim = tok[tok_len];
> +
> +	*(tok + tok_len++) = '\0';

These two look a little odd: I would prefer either array or pointer in
both cases.

> +
> +	if (strcspn (tok, "%") < tok_len - 1) {
> +	    /* something to decode */
> +	    if (hex_decode_inplace (tok) != HEX_SUCCESS) {
> +		ret = line_error (TAG_PARSE_INVALID, line_for_error,
> +				  "hex decoding of token '%s' failed", tok);
> +		goto DONE;
> +	    }
> +
> +	    if (double_quote_str (ctx, tok, &buf, &buf_len)) {
> +		ret = line_error (TAG_PARSE_OUT_OF_MEMORY,
> +				  line_for_error, "aborting");
> +		goto DONE;
> +	    }
> +	    *query_string = talloc_asprintf_append_buffer (
> +		*query_string, "%s%c", buf, delim);
> +
> +	} else {
> +	    /* This is not just an optimization, but used to preserve
> +	     * prefixes like id:, which cannot be quoted.
> +	     */
> +	    *query_string = talloc_asprintf_append_buffer (
> +		*query_string, "%s%c", tok, delim);
> +	}

What happens if a message id (for example) contains a ':'? Is a query of
the form 

id:stuff"encoded_stuff" 

acceptable? (As far as I can see from the man page ':' does not need to
be in hex.)

Best wishes

Mark


> +
> +    }
> +
> +  DONE:
> +    if (ret != TAG_PARSE_SUCCESS && *query_string)
> +	talloc_free (*query_string);
> +    return ret;
> +}
> +
>  tag_parse_status_t
>  parse_tag_line (void *ctx, char *line,
>  		tag_op_flag_t flags,
> -- 
> 1.7.10.4
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [Patch v7 06/14] notmuch-restore: move query handling for batch restore to parser

[Mark Walters]

On Fri, 14 Dec 2012, david@tethera.net wrote:
> From: David Bremner <bremner@debian.org>
>
> We are able to detect more errors by looking at the string before it
> is hex-decoded. We also need this to avoid the query quoting for more
> general queries (to be written) that will mess up raw message-ids.
> ---
>  notmuch-restore.c |   18 +-----------------
>  tag-util.c        |   26 ++++++++++++++++++++------
>  tag-util.h        |    5 ++++-
>  test/dump-restore |    5 ++---
>  4 files changed, 27 insertions(+), 27 deletions(-)
>
> diff --git a/notmuch-restore.c b/notmuch-restore.c
> index 40596a8..112f2f3 100644
> --- a/notmuch-restore.c
> +++ b/notmuch-restore.c
> @@ -208,24 +208,8 @@ notmuch_restore_command (unused (void *ctx), int argc, char *argv[])
>  	if (input_format == DUMP_FORMAT_SUP) {
>  	    ret = parse_sup_line (ctx, line, &query_string, tag_ops);
>  	} else {
> -	    ret = parse_tag_line (ctx, line, TAG_FLAG_BE_GENEROUS,
> +	    ret = parse_tag_line (ctx, line, TAG_FLAG_BE_GENEROUS | TAG_FLAG_ID_ONLY,
>  				  &query_string, tag_ops);
> -
> -	    if (ret == 0) {
> -		if (strncmp ("id:", query_string, 3) != 0) {
> -		    fprintf (stderr, "Warning: unsupported query: %s\n", query_string);
> -		    continue;
> -		}
> -		/* delete id: from front of string; tag_message
> -		 * expects a raw message-id.
> -		 *
> -		 * XXX: Note that query string id:foo and bar will be
> -		 * interpreted as a message id "foo and bar". This
> -		 * should eventually be fixed to give a better error
> -		 * message.
> -		 */
> -		query_string = query_string + 3;
> -	    }
>  	}
>  
>  	if (ret > 0)
> diff --git a/tag-util.c b/tag-util.c
> index e1181f8..8fea76c 100644
> --- a/tag-util.c
> +++ b/tag-util.c
> @@ -201,14 +201,28 @@ parse_tag_line (void *ctx, char *line,
>      }
>  
>      /* tok now points to the query string */
> -    if (hex_decode_inplace (tok) != HEX_SUCCESS) {
> -	ret = line_error (TAG_PARSE_INVALID, line_for_error,
> -			  "hex decoding of query %s failed", tok);
> -	goto DONE;
> +    if (flags & TAG_FLAG_ID_ONLY) {
> +	/* this is under the assumption that any whitespace in the
> +	 * message-id must be hex-encoded. The check is probably not
> +	 * perfect for exotic unicode whitespace; as fallback the
> +	 * search for strange message-ids will fail */
> +	if ((strncmp ("id:", tok, 3) != 0) ||
> +	    (strcspn (tok, " \t") < strlen (tok))) {
> +	    ret = line_error (TAG_PARSE_INVALID, line_for_error,
> +			      "query '%s' is not 'id:<message-id>'", tok);
> +	    goto DONE;
> +	}
> +	if (hex_decode_inplace (tok) != HEX_SUCCESS) {
> +	    ret = line_error (TAG_PARSE_INVALID, line_for_error,
> +			      "hex decoding of query %s failed", tok);
> +	    goto DONE;
> +	}
> +	/* skip 'id:' */
> +	*query_string = tok + 3;

This looks like it doesn't double_quote the query_string in this (the
TAG_FLAG_ID_ONLY) case. Is that deliberate?

Best wishes

Mark

> +    } else {
> +	ret = quote_and_decode_query (ctx, tok, line_for_error, query_string);
>      }
>  
> -    *query_string = tok;
> -
>    DONE:
>      talloc_free (line_for_error);
>      return ret;
> diff --git a/tag-util.h b/tag-util.h
> index 2889736..7674051 100644
> --- a/tag-util.h
> +++ b/tag-util.h
> @@ -26,7 +26,10 @@ typedef enum {
>      /* Accept strange tags that might be user error;
>       * intended for use by notmuch-restore.
>       */
> -    TAG_FLAG_BE_GENEROUS = (1 << 3)
> +    TAG_FLAG_BE_GENEROUS = (1 << 3),
> +
> +    /* Query consists of a single id:$message-id */
> +    TAG_FLAG_ID_ONLY = (1 << 4)
>  
>  } tag_op_flag_t;
>  
> diff --git a/test/dump-restore b/test/dump-restore
> index 6a989b6..eb7933a 100755
> --- a/test/dump-restore
> +++ b/test/dump-restore
> @@ -199,19 +199,18 @@ a
>  # the next non-comment line should report an an empty tag error for
>  # batch tagging, but not for restore
>  + +e -- id:20091117232137.GA7669@griffis1.net
> -# highlight the sketchy id parsing; this should be last
>  +g -- id:foo and bar
>  EOF
>  
>  cat <<EOF > EXPECTED
> -Warning: unsupported query: a
> +Warning: query 'a' is not 'id:<message-id>' [a]
>  Warning: no query string [+0]
>  Warning: no query string [+a +b]
>  Warning: missing query string [+a +b ]
>  Warning: no query string after -- [+c +d --]
>  Warning: hex decoding of tag %zz failed [+%zz -- id:whatever]
>  Warning: hex decoding of query id:%yy failed [+e +f id:%yy]
> -Warning: cannot apply tags to missing message: foo and bar
> +Warning: query 'id:foo and bar' is not 'id:<message-id>' [+g -- id:foo and bar]
>  EOF
>  
>  test_expect_equal_file EXPECTED OUTPUT
> -- 
> 1.7.10.4
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [Patch v7 04/14] notmuch-tag: factor out double quoting routine

[Mark Walters]

On Fri, 14 Dec 2012, david@tethera.net wrote:
> From: David Bremner <bremner@debian.org>
>
> This could live in tag-util as well, but it is really nothing specific
> to tags (although the conventions are arguable specific to Xapian).
>
> The API is changed from "caller-allocates" to "readline-like". The scan for
> max tag length is pushed down into the double quoting routine.
> ---
>  notmuch-tag.c      |   50 ++++++++++++++++----------------------------------
>  util/string-util.c |   34 ++++++++++++++++++++++++++++++++++
>  util/string-util.h |    8 ++++++++
>  3 files changed, 58 insertions(+), 34 deletions(-)
>
> diff --git a/notmuch-tag.c b/notmuch-tag.c
> index 0965ee7..13f2268 100644
> --- a/notmuch-tag.c
> +++ b/notmuch-tag.c
> @@ -20,6 +20,7 @@
>  
>  #include "notmuch-client.h"
>  #include "tag-util.h"
> +#include "string-util.h"
>  
>  static volatile sig_atomic_t interrupted;
>  
> @@ -37,25 +38,6 @@ handle_sigint (unused (int sig))
>  }
>  
>  static char *
> -_escape_tag (char *buf, const char *tag)
> -{
> -    const char *in = tag;
> -    char *out = buf;
> -
> -    /* Boolean terms surrounded by double quotes can contain any
> -     * character.  Double quotes are quoted by doubling them. */
> -    *out++ = '"';
> -    while (*in) {
> -	if (*in == '"')
> -	    *out++ = '"';
> -	*out++ = *in++;
> -    }
> -    *out++ = '"';
> -    *out = 0;
> -    return buf;
> -}
> -
> -static char *
>  _optimize_tag_query (void *ctx, const char *orig_query_string,
>  		     const tag_op_list_t *list)
>  {
> @@ -67,44 +49,44 @@ _optimize_tag_query (void *ctx, const char *orig_query_string,
>       * parenthesize and the exclusion part of the query must not use
>       * the '-' operator (though the NOT operator is fine). */
>  
> -    char *escaped, *query_string;
> +    char *escaped = NULL;
> +    size_t escaped_len = 0;
> +    char *query_string;
>      const char *join = "";
>      size_t i;
> -    unsigned int max_tag_len = 0;
>  
>      /* Don't optimize if there are no tag changes. */
>      if (tag_op_list_size (list) == 0)
>  	return talloc_strdup (ctx, orig_query_string);
>  
> -    /* Allocate a buffer for escaping tags.  This is large enough to
> -     * hold a fully escaped tag with every character doubled plus
> -     * enclosing quotes and a NUL. */
> -    for (i = 0; i < tag_op_list_size (list); i++)
> -	if (strlen (tag_op_list_tag (list, i)) > max_tag_len)
> -	    max_tag_len = strlen (tag_op_list_tag (list, i));
> -
> -    escaped = talloc_array (ctx, char, max_tag_len * 2 + 3);
> -    if (! escaped)
> -	return NULL;
> -
>      /* Build the new query string */
>      if (strcmp (orig_query_string, "*") == 0)
>  	query_string = talloc_strdup (ctx, "(");
>      else
>  	query_string = talloc_asprintf (ctx, "( %s ) and (", orig_query_string);
>  
> +
> +    /* Boolean terms surrounded by double quotes can contain any
> +     * character.  Double quotes are quoted by doubling them. */
> +
>      for (i = 0; i < tag_op_list_size (list) && query_string; i++) {
> +	double_quote_str (ctx,
> +			  tag_op_list_tag (list, i),
> +			  &escaped, &escaped_len);
> +
>  	query_string = talloc_asprintf_append_buffer (
>  	    query_string, "%s%stag:%s", join,
>  	    tag_op_list_isremove (list, i) ? "" : "not ",
> -	    _escape_tag (escaped, tag_op_list_tag (list, i)));
> +	    escaped);
>  	join = " or ";
>      }
>  
>      if (query_string)
>  	query_string = talloc_strdup_append_buffer (query_string, ")");
>  
> -    talloc_free (escaped);
> +    if (escaped)
> +	talloc_free (escaped);
> +
>      return query_string;
>  }
>  
> diff --git a/util/string-util.c b/util/string-util.c
> index 44f8cd3..ea7c25b 100644
> --- a/util/string-util.c
> +++ b/util/string-util.c
> @@ -20,6 +20,7 @@
>  
>  
>  #include "string-util.h"
> +#include "talloc.h"
>  
>  char *
>  strtok_len (char *s, const char *delim, size_t *len)
> @@ -32,3 +33,36 @@ strtok_len (char *s, const char *delim, size_t *len)
>  
>      return *len ? s : NULL;
>  }
> +
> +
> +int
> +double_quote_str (void *ctx, const char *str,
> +		  char **buf, size_t *len)
> +{
> +    const char *in;
> +    char *out;
> +    size_t needed = 3;
> +
> +    for (in = str; *in; in++)
> +	needed += (*in == '"') ? 2 : 1;
> +
> +    if (needed > *len)
> +	*buf = talloc_realloc (ctx, *buf, char, 2*needed);
> +
> +    if (! *buf)
> +	return 1;
> +
> +    out = *buf;
> +
> +    *out++ = '"';
> +    in = str;
> +    while (*in) {
> +	if (*in == '"')
> +	    *out++ = '"';
> +	*out++ = *in++;
> +    }
> +    *out++ = '"';
> +    *out = 0;

Just a triviality: isn't '\0' preferred?

Best wishes

Mark

> +
> +    return 0;
> +}
> diff --git a/util/string-util.h b/util/string-util.h
> index ac7676c..b593bc7 100644
> --- a/util/string-util.h
> +++ b/util/string-util.h
> @@ -19,4 +19,12 @@
>  
>  char *strtok_len (char *s, const char *delim, size_t *len);
>  
> +/* Copy str to dest, surrounding with double quotes.
> + * Any internal double-quotes are doubled, i.e. a"b -> "a""b"
> + *
> + * Output is into buf; it may be talloc_realloced
> + * return 0 on success, non-zero on failure.
> + */
> +int double_quote_str (void *talloc_ctx, const char *str,
> +		      char **buf, size_t *len);
>  #endif
> -- 
> 1.7.10.4
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [Patch v7 06/14] notmuch-restore: move query handling for batch restore to parser

[David Bremner]

Mark Walters <markwalters1009@gmail.com> writes:

>> +	if (hex_decode_inplace (tok) != HEX_SUCCESS) {
>> +	    ret = line_error (TAG_PARSE_INVALID, line_for_error,
>> +			      "hex decoding of query %s failed", tok);
>> +	    goto DONE;
>> +	}
>> +	/* skip 'id:' */
>> +	*query_string = tok + 3;
>
> This looks like it doesn't double_quote the query_string in this (the
> TAG_FLAG_ID_ONLY) case. Is that deliberate?

Yes, that's what I meant by 

,----
| We also need this to avoid the query quoting for more
| general queries (to be written) that will mess up raw message-ids.
`----

perhaps it deserves a comment in the code.

Re: [Patch v7 05/14] quote_and_decode_query: new function to quote hex-decoded queries

[David Bremner]

Mark Walters <markwalters1009@gmail.com> writes:


> What happens if a message id (for example) contains a ':'? Is a query of
> the form 
>
> id:stuff"encoded_stuff" 
>
> acceptable? (As far as I can see from the man page ':' does not need to
> be in hex.)

The updated version of the notmuch-dump man page does say that : will be
hex encoded, so I think the fix here is to update the notmuch tag man
page.

[PATCH] fixup: clarify TAG_FLAG_ID_ONLY comments and name

[david]

From: David Bremner <bremner@debian.org>

---

After some chatter on IRC, Mark and I converged to the following 
 
 notmuch-restore.c |    2 +-
 tag-util.c        |    2 +-
 tag-util.h        |    6 ++++--
 3 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/notmuch-restore.c b/notmuch-restore.c
index 112f2f3..1b66e76 100644
--- a/notmuch-restore.c
+++ b/notmuch-restore.c
@@ -208,7 +208,7 @@ notmuch_restore_command (unused (void *ctx), int argc, char *argv[])
 	if (input_format == DUMP_FORMAT_SUP) {
 	    ret = parse_sup_line (ctx, line, &query_string, tag_ops);
 	} else {
-	    ret = parse_tag_line (ctx, line, TAG_FLAG_BE_GENEROUS | TAG_FLAG_ID_ONLY,
+	    ret = parse_tag_line (ctx, line, TAG_FLAG_BE_GENEROUS | TAG_FLAG_ID_DIRECT,
 				  &query_string, tag_ops);
 	}
 
diff --git a/tag-util.c b/tag-util.c
index 8fea76c..37bffd5 100644
--- a/tag-util.c
+++ b/tag-util.c
@@ -201,7 +201,7 @@ parse_tag_line (void *ctx, char *line,
     }
 
     /* tok now points to the query string */
-    if (flags & TAG_FLAG_ID_ONLY) {
+    if (flags & TAG_FLAG_ID_DIRECT) {
 	/* this is under the assumption that any whitespace in the
 	 * message-id must be hex-encoded. The check is probably not
 	 * perfect for exotic unicode whitespace; as fallback the
diff --git a/tag-util.h b/tag-util.h
index 7674051..eec00cf 100644
--- a/tag-util.h
+++ b/tag-util.h
@@ -28,8 +28,10 @@ typedef enum {
      */
     TAG_FLAG_BE_GENEROUS = (1 << 3),
 
-    /* Query consists of a single id:$message-id */
-    TAG_FLAG_ID_ONLY = (1 << 4)
+    /* Directly look up messages by hex-decoded message-id, rather
+     * than parsing a general query. The query MUST be of the form
+     * id:$message-id. */
+    TAG_FLAG_ID_DIRECT = (1 << 4)
 
 } tag_op_flag_t;
 
-- 
1.7.10.4

[PATCH] fixup for hex encoding desription in notmuch-tag.1

[david]

From: David Bremner <bremner@debian.org>

---
and here is the updated description.

 man/man1/notmuch-tag.1 |   19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/man/man1/notmuch-tag.1 b/man/man1/notmuch-tag.1
index ac2c8d7..a203f53 100644
--- a/man/man1/notmuch-tag.1
+++ b/man/man1/notmuch-tag.1
@@ -67,13 +67,22 @@ The input must consist of lines of the format:
 Each line is interpreted similarly to
 .B notmuch tag
 command line arguments. The delimiter is one or more spaces ' '. Any
-characters in <tag> and <search-term>
+characters in
+.RI < tag >
+and
+.RI < search-term >
 .B may
-be hex encoded with %NN where NN is the hexadecimal value of the
-character. Any ' ' and '%' characters in <tag> and <search-terms>
+be hex-encoded with %NN where NN is the hexadecimal value of the
+character (more precisely with %NN[%MM...] where NN, MM, etc... are
+the hexadecimal values of the bytes in the UTF-8 encoding of
+the character).  Any characters in <tag> and <search-term> not
+matching the regex
+.B [A-Za-z0-9@=.,_+-]
 .B must
-be hex encoded (using %20 and %25, respectively). Any characters that
-are not part of <tag> or <search-terms>
+be hex-encoded. Any characters that are not part of
+.RI  < tag >
+or
+.RI < search-term >
 .B must not
 be hex encoded.
 
-- 
1.7.10.4

Re: [Patch v7 04/14] notmuch-tag: factor out double quoting routine

[Jani Nikula]

On Fri, 14 Dec 2012, david@tethera.net wrote:
> From: David Bremner <bremner@debian.org>
>
> This could live in tag-util as well, but it is really nothing specific
> to tags (although the conventions are arguable specific to Xapian).
>
> The API is changed from "caller-allocates" to "readline-like". The scan for
> max tag length is pushed down into the double quoting routine.
> ---
>  notmuch-tag.c      |   50 ++++++++++++++++----------------------------------
>  util/string-util.c |   34 ++++++++++++++++++++++++++++++++++
>  util/string-util.h |    8 ++++++++
>  3 files changed, 58 insertions(+), 34 deletions(-)
>
> diff --git a/notmuch-tag.c b/notmuch-tag.c
> index 0965ee7..13f2268 100644
> --- a/notmuch-tag.c
> +++ b/notmuch-tag.c
> @@ -20,6 +20,7 @@
>  
>  #include "notmuch-client.h"
>  #include "tag-util.h"
> +#include "string-util.h"
>  
>  static volatile sig_atomic_t interrupted;
>  
> @@ -37,25 +38,6 @@ handle_sigint (unused (int sig))
>  }
>  
>  static char *
> -_escape_tag (char *buf, const char *tag)
> -{
> -    const char *in = tag;
> -    char *out = buf;
> -
> -    /* Boolean terms surrounded by double quotes can contain any
> -     * character.  Double quotes are quoted by doubling them. */
> -    *out++ = '"';
> -    while (*in) {
> -	if (*in == '"')
> -	    *out++ = '"';
> -	*out++ = *in++;
> -    }
> -    *out++ = '"';
> -    *out = 0;
> -    return buf;
> -}
> -
> -static char *
>  _optimize_tag_query (void *ctx, const char *orig_query_string,
>  		     const tag_op_list_t *list)
>  {
> @@ -67,44 +49,44 @@ _optimize_tag_query (void *ctx, const char *orig_query_string,
>       * parenthesize and the exclusion part of the query must not use
>       * the '-' operator (though the NOT operator is fine). */
>  
> -    char *escaped, *query_string;
> +    char *escaped = NULL;
> +    size_t escaped_len = 0;
> +    char *query_string;
>      const char *join = "";
>      size_t i;
> -    unsigned int max_tag_len = 0;
>  
>      /* Don't optimize if there are no tag changes. */
>      if (tag_op_list_size (list) == 0)
>  	return talloc_strdup (ctx, orig_query_string);
>  
> -    /* Allocate a buffer for escaping tags.  This is large enough to
> -     * hold a fully escaped tag with every character doubled plus
> -     * enclosing quotes and a NUL. */
> -    for (i = 0; i < tag_op_list_size (list); i++)
> -	if (strlen (tag_op_list_tag (list, i)) > max_tag_len)
> -	    max_tag_len = strlen (tag_op_list_tag (list, i));
> -
> -    escaped = talloc_array (ctx, char, max_tag_len * 2 + 3);
> -    if (! escaped)
> -	return NULL;
> -
>      /* Build the new query string */
>      if (strcmp (orig_query_string, "*") == 0)
>  	query_string = talloc_strdup (ctx, "(");
>      else
>  	query_string = talloc_asprintf (ctx, "( %s ) and (", orig_query_string);
>  
> +
> +    /* Boolean terms surrounded by double quotes can contain any
> +     * character.  Double quotes are quoted by doubling them. */
> +
>      for (i = 0; i < tag_op_list_size (list) && query_string; i++) {
> +	double_quote_str (ctx,
> +			  tag_op_list_tag (list, i),
> +			  &escaped, &escaped_len);

Check return value?

> +
>  	query_string = talloc_asprintf_append_buffer (
>  	    query_string, "%s%stag:%s", join,
>  	    tag_op_list_isremove (list, i) ? "" : "not ",
> -	    _escape_tag (escaped, tag_op_list_tag (list, i)));
> +	    escaped);
>  	join = " or ";
>      }
>  
>      if (query_string)
>  	query_string = talloc_strdup_append_buffer (query_string, ")");
>  
> -    talloc_free (escaped);
> +    if (escaped)
> +	talloc_free (escaped);
> +
>      return query_string;
>  }
>  
> diff --git a/util/string-util.c b/util/string-util.c
> index 44f8cd3..ea7c25b 100644
> --- a/util/string-util.c
> +++ b/util/string-util.c
> @@ -20,6 +20,7 @@
>  
>  
>  #include "string-util.h"
> +#include "talloc.h"
>  
>  char *
>  strtok_len (char *s, const char *delim, size_t *len)
> @@ -32,3 +33,36 @@ strtok_len (char *s, const char *delim, size_t *len)
>  
>      return *len ? s : NULL;
>  }
> +
> +
> +int
> +double_quote_str (void *ctx, const char *str,
> +		  char **buf, size_t *len)
> +{
> +    const char *in;
> +    char *out;
> +    size_t needed = 3;
> +
> +    for (in = str; *in; in++)
> +	needed += (*in == '"') ? 2 : 1;
> +
> +    if (needed > *len)
> +	*buf = talloc_realloc (ctx, *buf, char, 2*needed);

You fail to set *len to 2*needed, leading to doing realloc every time.

Also, I think you should follow the getline pattern like you did in
hex_encode: if *buf == NULL, the input value of *len is ignored.

BR,
Jani.

> +
> +    if (! *buf)
> +	return 1;
> +
> +    out = *buf;
> +
> +    *out++ = '"';
> +    in = str;
> +    while (*in) {
> +	if (*in == '"')
> +	    *out++ = '"';
> +	*out++ = *in++;
> +    }
> +    *out++ = '"';
> +    *out = 0;
> +
> +    return 0;
> +}
> diff --git a/util/string-util.h b/util/string-util.h
> index ac7676c..b593bc7 100644
> --- a/util/string-util.h
> +++ b/util/string-util.h
> @@ -19,4 +19,12 @@
>  
>  char *strtok_len (char *s, const char *delim, size_t *len);
>  
> +/* Copy str to dest, surrounding with double quotes.
> + * Any internal double-quotes are doubled, i.e. a"b -> "a""b"
> + *
> + * Output is into buf; it may be talloc_realloced
> + * return 0 on success, non-zero on failure.
> + */
> +int double_quote_str (void *talloc_ctx, const char *str,
> +		      char **buf, size_t *len);
>  #endif
> -- 
> 1.7.10.4
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [Patch v7 06/14] notmuch-restore: move query handling for batch restore to parser

[Jani Nikula]

On Fri, 14 Dec 2012, david@tethera.net wrote:
> From: David Bremner <bremner@debian.org>
>
> We are able to detect more errors by looking at the string before it
> is hex-decoded. We also need this to avoid the query quoting for more
> general queries (to be written) that will mess up raw message-ids.
> ---
>  notmuch-restore.c |   18 +-----------------
>  tag-util.c        |   26 ++++++++++++++++++++------
>  tag-util.h        |    5 ++++-
>  test/dump-restore |    5 ++---
>  4 files changed, 27 insertions(+), 27 deletions(-)
>
> diff --git a/notmuch-restore.c b/notmuch-restore.c
> index 40596a8..112f2f3 100644
> --- a/notmuch-restore.c
> +++ b/notmuch-restore.c
> @@ -208,24 +208,8 @@ notmuch_restore_command (unused (void *ctx), int argc, char *argv[])
>  	if (input_format == DUMP_FORMAT_SUP) {
>  	    ret = parse_sup_line (ctx, line, &query_string, tag_ops);
>  	} else {
> -	    ret = parse_tag_line (ctx, line, TAG_FLAG_BE_GENEROUS,
> +	    ret = parse_tag_line (ctx, line, TAG_FLAG_BE_GENEROUS | TAG_FLAG_ID_ONLY,
>  				  &query_string, tag_ops);

I realize that we've screwed up a bit here already in the restore
series. parse_sup_line() allocates query_string for each input line, but
doesn't free it during parsing. parse_tag_line() sets query_string to
point to the query string within line. And now it gets worse when
query_string is allocated vs. set to point to another buffer depending
on TAG_FLAG_ID_ONLY, so it's not an easy interface to use.

> -
> -	    if (ret == 0) {
> -		if (strncmp ("id:", query_string, 3) != 0) {
> -		    fprintf (stderr, "Warning: unsupported query: %s\n", query_string);
> -		    continue;
> -		}
> -		/* delete id: from front of string; tag_message
> -		 * expects a raw message-id.
> -		 *
> -		 * XXX: Note that query string id:foo and bar will be
> -		 * interpreted as a message id "foo and bar". This
> -		 * should eventually be fixed to give a better error
> -		 * message.
> -		 */
> -		query_string = query_string + 3;
> -	    }



>  	}
>  
>  	if (ret > 0)
> diff --git a/tag-util.c b/tag-util.c
> index e1181f8..8fea76c 100644
> --- a/tag-util.c
> +++ b/tag-util.c
> @@ -201,14 +201,28 @@ parse_tag_line (void *ctx, char *line,
>      }
>  
>      /* tok now points to the query string */
> -    if (hex_decode_inplace (tok) != HEX_SUCCESS) {
> -	ret = line_error (TAG_PARSE_INVALID, line_for_error,
> -			  "hex decoding of query %s failed", tok);
> -	goto DONE;
> +    if (flags & TAG_FLAG_ID_ONLY) {
> +	/* this is under the assumption that any whitespace in the
> +	 * message-id must be hex-encoded. The check is probably not
> +	 * perfect for exotic unicode whitespace; as fallback the
> +	 * search for strange message-ids will fail */
> +	if ((strncmp ("id:", tok, 3) != 0) ||

The current wording is, "Any characters in <tag> and <search-term> MAY
be hex encoded with %NN...", but the above no longer matches that, as
"id:" must not be encoded. In the interest of keeping the documentation
concise, I think you should move the above check after
hex_decode_inplace(), but keep the below check here. That should do it,
right?

> +	    (strcspn (tok, " \t") < strlen (tok))) {
> +	    ret = line_error (TAG_PARSE_INVALID, line_for_error,
> +			      "query '%s' is not 'id:<message-id>'", tok);
> +	    goto DONE;
> +	}
> +	if (hex_decode_inplace (tok) != HEX_SUCCESS) {
> +	    ret = line_error (TAG_PARSE_INVALID, line_for_error,
> +			      "hex decoding of query %s failed", tok);
> +	    goto DONE;
> +	}
> +	/* skip 'id:' */
> +	*query_string = tok + 3;
> +    } else {
> +	ret = quote_and_decode_query (ctx, tok, line_for_error, query_string);
>      }

It's not pretty that query_string gets allocated or not depending on a
flag that doesn't seem to have anything to do with that. I don't have a
good suggestion now, though, and I'd like to keep the restore path like
it is, without allocation.

>  
> -    *query_string = tok;
> -
>    DONE:
>      talloc_free (line_for_error);
>      return ret;
> diff --git a/tag-util.h b/tag-util.h
> index 2889736..7674051 100644
> --- a/tag-util.h
> +++ b/tag-util.h
> @@ -26,7 +26,10 @@ typedef enum {
>      /* Accept strange tags that might be user error;
>       * intended for use by notmuch-restore.
>       */
> -    TAG_FLAG_BE_GENEROUS = (1 << 3)
> +    TAG_FLAG_BE_GENEROUS = (1 << 3),
> +
> +    /* Query consists of a single id:$message-id */
> +    TAG_FLAG_ID_ONLY = (1 << 4)
>  
>  } tag_op_flag_t;
>  
> diff --git a/test/dump-restore b/test/dump-restore
> index 6a989b6..eb7933a 100755
> --- a/test/dump-restore
> +++ b/test/dump-restore
> @@ -199,19 +199,18 @@ a
>  # the next non-comment line should report an an empty tag error for
>  # batch tagging, but not for restore
>  + +e -- id:20091117232137.GA7669@griffis1.net
> -# highlight the sketchy id parsing; this should be last
>  +g -- id:foo and bar
>  EOF
>  
>  cat <<EOF > EXPECTED
> -Warning: unsupported query: a
> +Warning: query 'a' is not 'id:<message-id>' [a]
>  Warning: no query string [+0]
>  Warning: no query string [+a +b]
>  Warning: missing query string [+a +b ]
>  Warning: no query string after -- [+c +d --]
>  Warning: hex decoding of tag %zz failed [+%zz -- id:whatever]
>  Warning: hex decoding of query id:%yy failed [+e +f id:%yy]
> -Warning: cannot apply tags to missing message: foo and bar
> +Warning: query 'id:foo and bar' is not 'id:<message-id>' [+g -- id:foo and bar]
>  EOF
>  
>  test_expect_equal_file EXPECTED OUTPUT
> -- 
> 1.7.10.4
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH] fixup for hex encoding desription in notmuch-tag.1

[Jani Nikula]

On Sat, 15 Dec 2012, david@tethera.net wrote:
> From: David Bremner <bremner@debian.org>
>
> ---
> and here is the updated description.
>
>  man/man1/notmuch-tag.1 |   19 ++++++++++++++-----
>  1 file changed, 14 insertions(+), 5 deletions(-)
>
> diff --git a/man/man1/notmuch-tag.1 b/man/man1/notmuch-tag.1
> index ac2c8d7..a203f53 100644
> --- a/man/man1/notmuch-tag.1
> +++ b/man/man1/notmuch-tag.1
> @@ -67,13 +67,22 @@ The input must consist of lines of the format:
>  Each line is interpreted similarly to
>  .B notmuch tag
>  command line arguments. The delimiter is one or more spaces ' '. Any
> -characters in <tag> and <search-term>
> +characters in
> +.RI < tag >
> +and
> +.RI < search-term >
>  .B may
> -be hex encoded with %NN where NN is the hexadecimal value of the
> -character. Any ' ' and '%' characters in <tag> and <search-terms>
> +be hex-encoded with %NN where NN is the hexadecimal value of the
> +character (more precisely with %NN[%MM...] where NN, MM, etc... are
> +the hexadecimal values of the bytes in the UTF-8 encoding of
> +the character).  Any characters in <tag> and <search-term> not
> +matching the regex
> +.B [A-Za-z0-9@=.,_+-]

And this actually means you can't have "id:foo" there, as : needs to be
encoded. Not user friendly at all. If it should really mean : is okay
un-encoded as prefix delimiter but not otherwise... it gets a bit messy
to document.

BR,
Jani.

>  .B must
> -be hex encoded (using %20 and %25, respectively). Any characters that
> -are not part of <tag> or <search-terms>
> +be hex-encoded. Any characters that are not part of
> +.RI  < tag >
> +or
> +.RI < search-term >
>  .B must not
>  be hex encoded.
>  
> -- 
> 1.7.10.4
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [Patch v7 07/14] cli: add support for batch tagging operations to "notmuch tag"

[Jani Nikula]

On Fri, 14 Dec 2012, david@tethera.net wrote:
> From: Jani Nikula <jani@nikula.org>
>
> Add support for batch tagging operations through stdin to "notmuch
> tag". This can be enabled with the new --batch command line option to
> "notmuch tag". The input must consist of lines of the format:
>
> +<tag>|-<tag> [...] [--] <search-term> [...]
>
> Each line is interpreted similarly to "notmuch tag" command line
> arguments. The delimiter is one or more spaces ' '. Any characters in
> <tag> and <search-term> MAY be hex encoded with %NN where NN is the
> hexadecimal value of the character. Any ' ' and '%' characters in
> <tag> and <search-term> MUST be hex encoded (using %20 and %25,
> respectively). Any characters that are not part of <tag> or
> <search-term> MUST NOT be hex encoded.
>
> Leading and trailing space ' ' is ignored. Empty lines and lines
> beginning with '#' are ignored.
>
> Signed-off-by: Jani Nikula <jani@nikula.org>
>
> Hacked-like-crazy-by: David Bremner <david@tethera.net>
> ---
>  notmuch-tag.c |   88 +++++++++++++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 80 insertions(+), 8 deletions(-)
>
> diff --git a/notmuch-tag.c b/notmuch-tag.c
> index 13f2268..a81d911 100644
> --- a/notmuch-tag.c
> +++ b/notmuch-tag.c
> @@ -130,6 +130,43 @@ tag_query (void *ctx, notmuch_database_t *notmuch, const char *query_string,
>      return interrupted;
>  }
>  
> +static int
> +tag_file (void *ctx, notmuch_database_t *notmuch, tag_op_flag_t flags,
> +	  FILE *input)
> +{
> +    char *line = NULL;
> +    char *query_string = NULL;
> +    size_t line_size = 0;
> +    ssize_t line_len;
> +    int ret = 0;
> +    tag_op_list_t *tag_ops;
> +
> +    tag_ops = tag_op_list_create (ctx);
> +    if (tag_ops == NULL) {
> +	fprintf (stderr, "Out of memory.\n");
> +	return 1;
> +    }
> +
> +    while ((line_len = getline (&line, &line_size, input)) != -1 &&
> +	   ! interrupted) {
> +
> +	ret = parse_tag_line (ctx, line, TAG_FLAG_NONE,
> +			      &query_string, tag_ops);
> +
> +	if (ret > 0)
> +	    continue;
> +
> +	if (ret < 0 || tag_query (ctx, notmuch, query_string,
> +				  tag_ops, flags))
> +	    break;

Hey, your changelog in the cover letter says you fixed the tag_query
return value propagation, but it's not here?

BR,
Jani.


> +    }
> +
> +    if (line)
> +	free (line);
> +
> +    return ret;
> +}
> +
>  int
>  notmuch_tag_command (void *ctx, int argc, char *argv[])
>  {
> @@ -139,6 +176,10 @@ notmuch_tag_command (void *ctx, int argc, char *argv[])
>      notmuch_database_t *notmuch;
>      struct sigaction action;
>      tag_op_flag_t tag_flags = TAG_FLAG_NONE;
> +    notmuch_bool_t batch = FALSE;
> +    FILE *input = stdin;
> +    char *input_file_name = NULL;
> +    int opt_index;
>      int ret = 0;
>  
>      /* Setup our handler for SIGINT */
> @@ -148,15 +189,43 @@ notmuch_tag_command (void *ctx, int argc, char *argv[])
>      action.sa_flags = SA_RESTART;
>      sigaction (SIGINT, &action, NULL);
>  
> -    tag_ops = tag_op_list_create (ctx);
> -    if (tag_ops == NULL) {
> -	fprintf (stderr, "Out of memory.\n");
> +    notmuch_opt_desc_t options[] = {
> +	{ NOTMUCH_OPT_BOOLEAN, &batch, "batch", 0, 0 },
> +	{ NOTMUCH_OPT_STRING, &input_file_name, "input", 'i', 0 },
> +	{ 0, 0, 0, 0, 0 }
> +    };
> +
> +    opt_index = parse_arguments (argc, argv, options, 1);
> +    if (opt_index < 0)
>  	return 1;
> +
> +    if (input_file_name) {
> +	batch = TRUE;
> +	input = fopen (input_file_name, "r");
> +	if (input == NULL) {
> +	    fprintf (stderr, "Error opening %s for reading: %s\n",
> +		     input_file_name, strerror (errno));
> +	    return 1;
> +	}
>      }
>  
> -    if (parse_tag_command_line (ctx, argc - 1, argv + 1,
> -				&query_string, tag_ops))
> -	return 1;
> +    if (batch) {
> +	if (opt_index != argc) {
> +	    fprintf (stderr, "Can't specify both cmdline and stdin!\n");
> +	    return 1;
> +	}
> +    } else {
> +
> +	tag_ops = tag_op_list_create (ctx);
> +	if (tag_ops == NULL) {
> +	    fprintf (stderr, "Out of memory.\n");
> +	    return 1;
> +	}
> +
> +	if (parse_tag_command_line (ctx, argc - opt_index, argv + opt_index,
> +				    &query_string, tag_ops))
> +	    return 1;
> +    }
>  
>      config = notmuch_config_open (ctx, NULL, NULL);
>      if (config == NULL)
> @@ -169,9 +238,12 @@ notmuch_tag_command (void *ctx, int argc, char *argv[])
>      if (notmuch_config_get_maildir_synchronize_flags (config))
>  	tag_flags |= TAG_FLAG_MAILDIR_SYNC;
>  
> -    ret = tag_query (ctx, notmuch, query_string, tag_ops, tag_flags);
> +    if (batch)
> +	ret = tag_file (ctx, notmuch, tag_flags, input);
> +    else
> +	ret = tag_query (ctx, notmuch, query_string, tag_ops, tag_flags);
>  
>      notmuch_database_destroy (notmuch);
>  
> -    return ret;
> +    return ret || interrupted;
>  }
> -- 
> 1.7.10.4

Re: [Patch v7 05/14] quote_and_decode_query: new function to quote hex-decoded queries

[Jani Nikula]

On Fri, 14 Dec 2012, david@tethera.net wrote:
> From: David Bremner <bremner@debian.org>
>
> The query is split into tokens, with ' ' and ':' as delimiters.  Any
> token containing some hex-escaped character is quoted according to
> Xapian rules.  This maps id:foo%20%22bar to id:"foo ""bar".
> This intentionally does not quote prefixes, so they still work as prefixes.
> ---
>  tag-util.c |   50 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 50 insertions(+)
>
> diff --git a/tag-util.c b/tag-util.c
> index f89669a..e1181f8 100644
> --- a/tag-util.c
> +++ b/tag-util.c
> @@ -56,6 +56,56 @@ illegal_tag (const char *tag, notmuch_bool_t remove)
>      return NULL;
>  }
>  
> +static tag_parse_status_t
> +quote_and_decode_query (void *ctx, char *encoded, const char *line_for_error,
> +			char **query_string)
> +{
> +    char *tok = encoded;
> +    size_t tok_len = 0;
> +    char *buf = NULL;
> +    size_t buf_len = 0;
> +    tag_parse_status_t ret = TAG_PARSE_SUCCESS;
> +
> +    *query_string = talloc_strdup (ctx, "");
> +
> +    while (*query_string &&
> +	   (tok = strtok_len (tok + tok_len, ": ", &tok_len)) != NULL) {

strtok_len() will eat all the leading delimiters at each call, and will
not return a zero-length token if you have multiple consecutive
delimiters. Which means you may end up losing stuff here. Whether that
matters or not I'm too tired to tell...

BR,
Jani.


> +	char delim = tok[tok_len];
> +
> +	*(tok + tok_len++) = '\0';
> +
> +	if (strcspn (tok, "%") < tok_len - 1) {
> +	    /* something to decode */
> +	    if (hex_decode_inplace (tok) != HEX_SUCCESS) {
> +		ret = line_error (TAG_PARSE_INVALID, line_for_error,
> +				  "hex decoding of token '%s' failed", tok);
> +		goto DONE;
> +	    }
> +
> +	    if (double_quote_str (ctx, tok, &buf, &buf_len)) {
> +		ret = line_error (TAG_PARSE_OUT_OF_MEMORY,
> +				  line_for_error, "aborting");
> +		goto DONE;
> +	    }
> +	    *query_string = talloc_asprintf_append_buffer (
> +		*query_string, "%s%c", buf, delim);
> +
> +	} else {
> +	    /* This is not just an optimization, but used to preserve
> +	     * prefixes like id:, which cannot be quoted.
> +	     */
> +	    *query_string = talloc_asprintf_append_buffer (
> +		*query_string, "%s%c", tok, delim);
> +	}
> +
> +    }
> +
> +  DONE:
> +    if (ret != TAG_PARSE_SUCCESS && *query_string)
> +	talloc_free (*query_string);
> +    return ret;
> +}
> +
>  tag_parse_status_t
>  parse_tag_line (void *ctx, char *line,
>  		tag_op_flag_t flags,
> -- 
> 1.7.10.4
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

[PATCH] cli: add support for batch tagging operations to "notmuch tag"

[david]

From: Jani Nikula <jani@nikula.org>

Add support for batch tagging operations through stdin to "notmuch
tag". This can be enabled with the new --batch command line option to
"notmuch tag". The input must consist of lines of the format:

+<tag>|-<tag> [...] [--] <search-term> [...]

Each line is interpreted similarly to "notmuch tag" command line
arguments. The delimiter is one or more spaces ' '. Any characters in
<tag> and <search-term> MAY be hex encoded with %NN where NN is the
hexadecimal value of the character. Any ' ' and '%' characters in
<tag> and <search-term> MUST be hex encoded (using %20 and %25,
respectively). Any characters that are not part of <tag> or
<search-term> MUST NOT be hex encoded.

Leading and trailing space ' ' is ignored. Empty lines and lines
beginning with '#' are ignored.

Signed-off-by: Jani Nikula <jani@nikula.org>

Hacked-like-crazy-by: David Bremner <david@tethera.net>
---

I missed somehow the change catch non-zero return from tag_query in tag_file.
This version is slightly cleaned up.

 notmuch-tag.c |   91 ++++++++++++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 83 insertions(+), 8 deletions(-)

diff --git a/notmuch-tag.c b/notmuch-tag.c
index 13f2268..44b5bb4 100644
--- a/notmuch-tag.c
+++ b/notmuch-tag.c
@@ -130,6 +130,46 @@ tag_query (void *ctx, notmuch_database_t *notmuch, const char *query_string,
     return interrupted;
 }
 
+static int
+tag_file (void *ctx, notmuch_database_t *notmuch, tag_op_flag_t flags,
+	  FILE *input)
+{
+    char *line = NULL;
+    char *query_string = NULL;
+    size_t line_size = 0;
+    ssize_t line_len;
+    int ret = 0;
+    tag_op_list_t *tag_ops;
+
+    tag_ops = tag_op_list_create (ctx);
+    if (tag_ops == NULL) {
+	fprintf (stderr, "Out of memory.\n");
+	return 1;
+    }
+
+    while ((line_len = getline (&line, &line_size, input)) != -1 &&
+	   ! interrupted) {
+
+	ret = parse_tag_line (ctx, line, TAG_FLAG_NONE,
+			      &query_string, tag_ops);
+
+	if (ret > 0)
+	    continue;
+
+	if (ret < 0)
+	    break;
+
+	ret = tag_query (ctx, notmuch, query_string, tag_ops, flags);
+	if (ret)
+	    break;
+    }
+
+    if (line)
+	free (line);
+
+    return ret;
+}
+
 int
 notmuch_tag_command (void *ctx, int argc, char *argv[])
 {
@@ -139,6 +179,10 @@ notmuch_tag_command (void *ctx, int argc, char *argv[])
     notmuch_database_t *notmuch;
     struct sigaction action;
     tag_op_flag_t tag_flags = TAG_FLAG_NONE;
+    notmuch_bool_t batch = FALSE;
+    FILE *input = stdin;
+    char *input_file_name = NULL;
+    int opt_index;
     int ret = 0;
 
     /* Setup our handler for SIGINT */
@@ -148,15 +192,43 @@ notmuch_tag_command (void *ctx, int argc, char *argv[])
     action.sa_flags = SA_RESTART;
     sigaction (SIGINT, &action, NULL);
 
-    tag_ops = tag_op_list_create (ctx);
-    if (tag_ops == NULL) {
-	fprintf (stderr, "Out of memory.\n");
+    notmuch_opt_desc_t options[] = {
+	{ NOTMUCH_OPT_BOOLEAN, &batch, "batch", 0, 0 },
+	{ NOTMUCH_OPT_STRING, &input_file_name, "input", 'i', 0 },
+	{ 0, 0, 0, 0, 0 }
+    };
+
+    opt_index = parse_arguments (argc, argv, options, 1);
+    if (opt_index < 0)
 	return 1;
+
+    if (input_file_name) {
+	batch = TRUE;
+	input = fopen (input_file_name, "r");
+	if (input == NULL) {
+	    fprintf (stderr, "Error opening %s for reading: %s\n",
+		     input_file_name, strerror (errno));
+	    return 1;
+	}
     }
 
-    if (parse_tag_command_line (ctx, argc - 1, argv + 1,
-				&query_string, tag_ops))
-	return 1;
+    if (batch) {
+	if (opt_index != argc) {
+	    fprintf (stderr, "Can't specify both cmdline and stdin!\n");
+	    return 1;
+	}
+    } else {
+
+	tag_ops = tag_op_list_create (ctx);
+	if (tag_ops == NULL) {
+	    fprintf (stderr, "Out of memory.\n");
+	    return 1;
+	}
+
+	if (parse_tag_command_line (ctx, argc - opt_index, argv + opt_index,
+				    &query_string, tag_ops))
+	    return 1;
+    }
 
     config = notmuch_config_open (ctx, NULL, NULL);
     if (config == NULL)
@@ -169,9 +241,12 @@ notmuch_tag_command (void *ctx, int argc, char *argv[])
     if (notmuch_config_get_maildir_synchronize_flags (config))
 	tag_flags |= TAG_FLAG_MAILDIR_SYNC;
 
-    ret = tag_query (ctx, notmuch, query_string, tag_ops, tag_flags);
+    if (batch)
+	ret = tag_file (ctx, notmuch, tag_flags, input);
+    else
+	ret = tag_query (ctx, notmuch, query_string, tag_ops, tag_flags);
 
     notmuch_database_destroy (notmuch);
 
-    return ret;
+    return ret || interrupted;
 }
-- 
1.7.10.4

Re: [Patch v7 05/14] quote_and_decode_query: new function to quote hex-decoded queries

[David Bremner]

Jani Nikula <jani@nikula.org> writes:

> strtok_len() will eat all the leading delimiters at each call, and will
> not return a zero-length token if you have multiple consecutive
> delimiters. Which means you may end up losing stuff here.

Right, I think for ':' it does matter, but it should be fixable with a
a little loop to copy ':'s to the query string after the (possibly quoted)
token.