V2 of batch-tagging plus new dump/restore

V2 of batch-tagging plus new dump/restore

[david]

this obsoletes: id:1353265498-3839-1-git-send-email-david@tethera.net

Changes based on Mark's review

- add commments for each function  in tag-util.h

- inline tag_op_list_from_string in the one place it was called 

- remove NULL terminator from tag_op_list (for mark); along with
  tag_op_list_t being opaque, I thought is was ok to leave off the
  comment on ops. But I could be convinced.

- make tag_op_list_t typedef opaque

- add BE_GENEROUS flag to parser. Currently this enables empty tags.

- handle blank lines and comments in restore and batch tagging

- fixed the commit message about "notmuch-new"

Changes based on Ethan's review:

- remove the dreaded . from first line of commit messages

- convert if on l49 of database-test.c into explicit return on error

- add comments and parens for num_tags and this_mid_len in random_corpus.c

- add check for message-id's less than length 1. I wasn't sure why we
  would disallow all message ids of length 1.

- remove declaration of parse_tag_stream

- explain "query_string += 3"

- fix typo in notmuch-dump.1

[Patch v2 01/17] hex-escape: (en|de)code strings to/from restricted character set

[david]

From: David Bremner <bremner@debian.org>

The character set is chosen to be suitable for pathnames, and the same
as that used by contrib/nmbug

[With additions by Jani Nikula]
---
 util/Makefile.local |    2 +-
 util/hex-escape.c   |  168 +++++++++++++++++++++++++++++++++++++++++++++++++++
 util/hex-escape.h   |   41 +++++++++++++
 3 files changed, 210 insertions(+), 1 deletion(-)
 create mode 100644 util/hex-escape.c
 create mode 100644 util/hex-escape.h

diff --git a/util/Makefile.local b/util/Makefile.local
index c7cae61..3ca623e 100644
--- a/util/Makefile.local
+++ b/util/Makefile.local
@@ -3,7 +3,7 @@
 dir := util
 extra_cflags += -I$(srcdir)/$(dir)
 
-libutil_c_srcs := $(dir)/xutil.c $(dir)/error_util.c
+libutil_c_srcs := $(dir)/xutil.c $(dir)/error_util.c $(dir)/hex-escape.c
 
 libutil_modules := $(libutil_c_srcs:.c=.o)
 
diff --git a/util/hex-escape.c b/util/hex-escape.c
new file mode 100644
index 0000000..d8905d0
--- /dev/null
+++ b/util/hex-escape.c
@@ -0,0 +1,168 @@
+/* hex-escape.c -  Manage encoding and decoding of byte strings into path names
+ *
+ * Copyright (c) 2011 David Bremner
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see http://www.gnu.org/licenses/ .
+ *
+ * Author: David Bremner <david@tethera.net>
+ */
+
+#include <assert.h>
+#include <string.h>
+#include <talloc.h>
+#include <ctype.h>
+#include "error_util.h"
+#include "hex-escape.h"
+
+static const size_t default_buf_size = 1024;
+
+static const char *output_charset =
+    "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+-_@=.:,";
+
+static const char escape_char = '%';
+
+static int
+is_output (char c)
+{
+    return (strchr (output_charset, c) != NULL);
+}
+
+static int
+maybe_realloc (void *ctx, size_t needed, char **out, size_t *out_size)
+{
+    if (*out_size < needed) {
+
+	if (*out == NULL)
+	    *out = talloc_size (ctx, needed);
+	else
+	    *out = talloc_realloc (ctx, *out, char, needed);
+
+	if (*out == NULL)
+	    return 0;
+
+	*out_size = needed;
+    }
+    return 1;
+}
+
+hex_status_t
+hex_encode (void *ctx, const char *in, char **out, size_t *out_size)
+{
+
+    const unsigned char *p;
+    char *q;
+
+    size_t escape_count = 0;
+    size_t len = 0;
+    size_t needed;
+
+    assert (ctx); assert (in); assert (out); assert (out_size);
+
+    for (p = (unsigned char *) in; *p; p++) {
+	escape_count += (!is_output (*p));
+	len++;
+    }
+
+    needed = len + escape_count * 2 + 1;
+
+    if (*out == NULL)
+	*out_size = 0;
+
+    if (!maybe_realloc (ctx, needed, out, out_size))
+	return HEX_OUT_OF_MEMORY;
+
+    q = *out;
+    p = (unsigned char *) in;
+
+    while (*p) {
+	if (is_output (*p)) {
+	    *q++ = *p++;
+	} else {
+	    sprintf (q, "%%%02x", *p++);
+	    q += 3;
+	}
+    }
+
+    *q = '\0';
+    return HEX_SUCCESS;
+}
+
+/* Hex decode 'in' to 'out'.
+ *
+ * This must succeed for in == out to support hex_decode_inplace().
+ */
+static hex_status_t
+hex_decode_internal (const char *in, unsigned char *out)
+{
+    char buf[3];
+
+    while (*in) {
+	if (*in == escape_char) {
+	    char *endp;
+
+	    /* This also handles unexpected end-of-string. */
+	    if (!isxdigit ((unsigned char) in[1]) ||
+		!isxdigit ((unsigned char) in[2]))
+		return HEX_SYNTAX_ERROR;
+
+	    buf[0] = in[1];
+	    buf[1] = in[2];
+	    buf[2] = '\0';
+
+	    *out = strtoul (buf, &endp, 16);
+
+	    if (endp != buf + 2)
+		return HEX_SYNTAX_ERROR;
+
+	    in += 3;
+	    out++;
+	} else {
+	    *out++ = *in++;
+	}
+    }
+
+    *out = '\0';
+
+    return HEX_SUCCESS;
+}
+
+hex_status_t
+hex_decode_inplace (char *s)
+{
+    /* A decoded string is never longer than the encoded one, so it is
+     * safe to decode a string onto itself. */
+    return hex_decode_internal (s, (unsigned char *) s);
+}
+
+hex_status_t
+hex_decode (void *ctx, const char *in, char **out, size_t * out_size)
+{
+    const char *p;
+    size_t escape_count = 0;
+    size_t needed = 0;
+
+    assert (ctx); assert (in); assert (out); assert (out_size);
+
+    size_t len = strlen (in);
+
+    for (p = in; *p; p++)
+	escape_count += (*p == escape_char);
+
+    needed = len - escape_count * 2 + 1;
+
+    if (!maybe_realloc (ctx, needed, out, out_size))
+	return HEX_OUT_OF_MEMORY;
+
+    return hex_decode_internal (in, (unsigned char *) *out);
+}
diff --git a/util/hex-escape.h b/util/hex-escape.h
new file mode 100644
index 0000000..5182042
--- /dev/null
+++ b/util/hex-escape.h
@@ -0,0 +1,41 @@
+#ifndef _HEX_ESCAPE_H
+#define _HEX_ESCAPE_H
+
+typedef enum hex_status {
+    HEX_SUCCESS = 0,
+    HEX_SYNTAX_ERROR,
+    HEX_OUT_OF_MEMORY
+} hex_status_t;
+
+/*
+ * The API for hex_encode() and hex_decode() is modelled on that for
+ * getline.
+ *
+ * If 'out' points to a NULL pointer a char array of the appropriate
+ * size is allocated using talloc, and out_size is updated.
+ *
+ * If 'out' points to a non-NULL pointer, it assumed to describe an
+ * existing char array, with the size given in *out_size.  This array
+ * may be resized by talloc_realloc if needed; in this case *out_size
+ * will also be updated.
+ *
+ * Note that it is an error to pass a NULL pointer for any parameter
+ * of these routines.
+ */
+
+hex_status_t
+hex_encode (void *talloc_ctx, const char *in, char **out,
+            size_t *out_size);
+
+hex_status_t
+hex_decode (void *talloc_ctx, const char *in, char **out,
+            size_t *out_size);
+
+/*
+ * Non-allocating hex decode to decode 's' in-place. The length of the
+ * result is always equal to or shorter than the length of the
+ * original.
+ */
+hex_status_t
+hex_decode_inplace (char *s);
+#endif
-- 
1.7.10.4

[Patch v2 02/17] test/hex-xcode: new test binary

[david]

From: David Bremner <bremner@debian.org>

This program is used both as a test-bed/unit-tester for
../util/hex-escape.c, and also as a utility in future tests of dump
and restore.
---
 test/.gitignore     |    1 +
 test/Makefile.local |   13 ++++++-
 test/basic          |    1 +
 test/hex-xcode.c    |  103 +++++++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 116 insertions(+), 2 deletions(-)
 create mode 100644 test/hex-xcode.c

diff --git a/test/.gitignore b/test/.gitignore
index e63c689..be7ab5e 100644
--- a/test/.gitignore
+++ b/test/.gitignore
@@ -3,4 +3,5 @@ corpus.mail
 smtp-dummy
 symbol-test
 arg-test
+hex-xcode
 tmp.*
diff --git a/test/Makefile.local b/test/Makefile.local
index 9ae130a..8da4c56 100644
--- a/test/Makefile.local
+++ b/test/Makefile.local
@@ -13,6 +13,9 @@ smtp_dummy_modules = $(smtp_dummy_srcs:.c=.o)
 $(dir)/arg-test: $(dir)/arg-test.o command-line-arguments.o util/libutil.a
 	$(call quiet,CC) -I. $^ -o $@
 
+$(dir)/hex-xcode: $(dir)/hex-xcode.o command-line-arguments.o util/libutil.a
+	$(call quiet,CC) -I. $^ -o $@ -ltalloc
+
 $(dir)/smtp-dummy: $(smtp_dummy_modules)
 	$(call quiet,CC) $^ -o $@
 
@@ -24,8 +27,13 @@ $(dir)/parse-time: $(dir)/parse-time.o parse-time-string/parse-time-string.o
 
 .PHONY: test check
 
-test-binaries: $(dir)/arg-test $(dir)/smtp-dummy $(dir)/symbol-test \
-	$(dir)/parse-time
+TEST_BINARIES=$(dir)/arg-test \
+	      $(dir)/hex-xcode \
+	      $(dir)/parse-time \
+	      $(dir)/smtp-dummy \
+	      $(dir)/symbol-test
+
+test-binaries: $(TEST_BINARIES)
 
 test:	all test-binaries
 	@${dir}/notmuch-test $(OPTIONS)
@@ -36,5 +44,6 @@ SRCS := $(SRCS) $(smtp_dummy_srcs)
 CLEAN := $(CLEAN) $(dir)/smtp-dummy $(dir)/smtp-dummy.o \
 	 $(dir)/symbol-test $(dir)/symbol-test.o \
 	 $(dir)/arg-test $(dir)/arg-test.o \
+	 $(dir)/hex-xcode $(dir)/hex-xcode.o \
 	 $(dir)/parse-time $(dir)/parse-time.o \
 	 $(dir)/corpus.mail $(dir)/test-results $(dir)/tmp.*
diff --git a/test/basic b/test/basic
index 1b842d2..2a571ac 100755
--- a/test/basic
+++ b/test/basic
@@ -56,6 +56,7 @@ tests_in_suite=$(for i in $TESTS; do echo $i; done | sort)
 available=$(find "$TEST_DIRECTORY" -maxdepth 1 -type f -perm +111  \
     ! -name aggregate-results.sh	\
     ! -name arg-test			\
+    ! -name hex-xcode			\
     ! -name notmuch-test		\
     ! -name parse-time			\
     ! -name smtp-dummy			\
diff --git a/test/hex-xcode.c b/test/hex-xcode.c
new file mode 100644
index 0000000..eec6541
--- /dev/null
+++ b/test/hex-xcode.c
@@ -0,0 +1,103 @@
+/* No, nothing to to with IDE from Apple Inc.
+   testbed for ../util/hex-escape.c.
+
+   usage:
+   hex-xcode [--direction=(encode|decode)] [--omit-newline] < file
+   hex-xcode [--direction=(encode|decode)] [--omit-newline] arg1 arg2 arg3 ...
+
+ */
+
+#include "notmuch-client.h"
+#include "hex-escape.h"
+#include <assert.h>
+
+
+enum direction {
+    ENCODE,
+    DECODE
+};
+
+static int
+xcode (void *ctx, enum direction dir, char *in, char **buf_p, size_t *size_p)
+{
+    hex_status_t status;
+
+    if (dir == ENCODE)
+	status = hex_encode (ctx, in, buf_p, size_p);
+    else
+	status = hex_decode (ctx, in, buf_p, size_p);
+
+    if (status == HEX_SUCCESS)
+	fputs (*buf_p, stdout);
+
+    return status;
+}
+
+
+int
+main (int argc, char **argv)
+{
+
+
+    enum direction dir = DECODE;
+    int omit_newline = FALSE;
+
+    notmuch_opt_desc_t options[] = {
+	{ NOTMUCH_OPT_KEYWORD, &dir, "direction", 'd',
+	  (notmuch_keyword_t []){ { "encode", ENCODE },
+				  { "decode", DECODE },
+				  { 0, 0 } } },
+	{ NOTMUCH_OPT_BOOLEAN, &omit_newline, "omit-newline", 'n', 0 },
+	{ 0, 0, 0, 0, 0 }
+    };
+
+    int opt_index = parse_arguments (argc, argv, options, 1);
+
+    if (opt_index < 0)
+	exit (1);
+
+    void *ctx = talloc_new (NULL);
+
+    char *line = NULL;
+    size_t line_size;
+    ssize_t line_len;
+
+    char *buffer = NULL;
+    size_t buf_size = 0;
+
+    notmuch_bool_t read_stdin = TRUE;
+
+    for (; opt_index < argc; opt_index++) {
+
+	if (xcode (ctx, dir, argv[opt_index],
+		   &buffer, &buf_size) != HEX_SUCCESS)
+	    return 1;
+
+	if (!omit_newline)
+	    putchar ('\n');
+
+	read_stdin = FALSE;
+    }
+
+    if (!read_stdin)
+	return 0;
+
+    while ((line_len = getline (&line, &line_size, stdin)) != -1) {
+
+	chomp_newline (line);
+
+	if (xcode (ctx, dir, line, &buffer, &buf_size) != HEX_SUCCESS)
+	    return 1;
+
+	if (!omit_newline)
+	    putchar ('\n');
+
+    }
+
+    if (line)
+	free (line);
+
+    talloc_free (ctx);
+
+    return 0;
+}
-- 
1.7.10.4

[Patch v2 03/17] test/hex-escaping: new test for hex escaping routines

[david]

From: David Bremner <bremner@debian.org>

These are more like unit tests, to (try to) make sure the library
functionality is working before building more complicated things on
top of it.
---
 test/hex-escaping |   26 ++++++++++++++++++++++++++
 test/notmuch-test |    1 +
 2 files changed, 27 insertions(+)
 create mode 100755 test/hex-escaping

diff --git a/test/hex-escaping b/test/hex-escaping
new file mode 100755
index 0000000..f34cc8c
--- /dev/null
+++ b/test/hex-escaping
@@ -0,0 +1,26 @@
+#!/usr/bin/env bash
+test_description="hex encoding and decoding"
+. ./test-lib.sh
+
+test_begin_subtest "round trip"
+find $TEST_DIRECTORY/corpus -type f -print | sort | xargs cat > EXPECTED
+$TEST_DIRECTORY/hex-xcode --direction=encode < EXPECTED | $TEST_DIRECTORY/hex-xcode --direction=decode > OUTPUT
+test_expect_equal_file OUTPUT EXPECTED
+
+test_begin_subtest "punctuation"
+tag1='comic_swear=$&^%$^%\\//-+$^%$'
+tag_enc1=$($TEST_DIRECTORY/hex-xcode --direction=encode "$tag1")
+test_expect_equal "$tag_enc1" "comic_swear=%24%26%5e%25%24%5e%25%5c%5c%2f%2f-+%24%5e%25%24"
+
+test_begin_subtest "round trip newlines"
+printf 'this\n tag\t has\n spaces\n' > EXPECTED.$test_count
+$TEST_DIRECTORY/hex-xcode --direction=encode  < EXPECTED.$test_count |\
+	$TEST_DIRECTORY/hex-xcode --direction=decode > OUTPUT.$test_count
+test_expect_equal_file EXPECTED.$test_count OUTPUT.$test_count
+
+test_begin_subtest "round trip 8bit chars"
+echo '%c3%91%c3%a5%c3%b0%c3%a3%c3%a5%c3%a9-%c3%8f%c3%8a' > EXPECTED.$test_count
+$TEST_DIRECTORY/hex-xcode --direction=decode  < EXPECTED.$test_count |\
+	$TEST_DIRECTORY/hex-xcode --direction=encode > OUTPUT.$test_count
+test_expect_equal_file EXPECTED.$test_count OUTPUT.$test_count
+test_done
diff --git a/test/notmuch-test b/test/notmuch-test
index 9a1b375..d2e90e2 100755
--- a/test/notmuch-test
+++ b/test/notmuch-test
@@ -60,6 +60,7 @@ TESTS="
   emacs-hello
   emacs-show
   missing-headers
+  hex-escaping
   parse-time-string
   search-date
 "
-- 
1.7.10.4

[Patch v2 04/17] test: add database routines for testing

[david]

From: David Bremner <bremner@debian.org>

Initially, provide a way to create "stub" messages in the notmuch
database without corresponding files.  This is essentially cut and
paste from lib/database.cc. This is a seperate file since we don't
want to export these symbols from libnotmuch or bloat the library with
non-exported code.
---
 test/Makefile.local  |    1 +
 test/database-test.c |   71 ++++++++++++++++++++++++++++++++++++++++++++++++++
 test/database-test.h |   21 +++++++++++++++
 3 files changed, 93 insertions(+)
 create mode 100644 test/database-test.c
 create mode 100644 test/database-test.h

diff --git a/test/Makefile.local b/test/Makefile.local
index 8da4c56..8479f91 100644
--- a/test/Makefile.local
+++ b/test/Makefile.local
@@ -45,5 +45,6 @@ CLEAN := $(CLEAN) $(dir)/smtp-dummy $(dir)/smtp-dummy.o \
 	 $(dir)/symbol-test $(dir)/symbol-test.o \
 	 $(dir)/arg-test $(dir)/arg-test.o \
 	 $(dir)/hex-xcode $(dir)/hex-xcode.o \
+	 $(dir)/database-test.o \
 	 $(dir)/parse-time $(dir)/parse-time.o \
 	 $(dir)/corpus.mail $(dir)/test-results $(dir)/tmp.*
diff --git a/test/database-test.c b/test/database-test.c
new file mode 100644
index 0000000..739e03b
--- /dev/null
+++ b/test/database-test.c
@@ -0,0 +1,71 @@
+/*
+ * Database routines intended only for testing, not exported from
+ * library.
+ *
+ * Copyright (c) 2012 David Bremner
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see http://www.gnu.org/licenses/ .
+ *
+ * Author: David Bremner <david@tethera.net>
+ */
+
+#include "notmuch-private.h"
+#include "database-test.h"
+
+notmuch_status_t
+notmuch_database_add_stub_message (notmuch_database_t *notmuch,
+				   const char *message_id,
+				   const char **tags)
+{
+    const char **tag;
+    notmuch_status_t ret;
+    notmuch_private_status_t private_status;
+    notmuch_message_t *message;
+
+    ret = _notmuch_database_ensure_writable (notmuch);
+    if (ret)
+	return ret;
+
+    message = _notmuch_message_create_for_message_id (notmuch,
+						      message_id,
+						      &private_status);
+    if (message == NULL) {
+	return COERCE_STATUS (private_status,
+			      "Unexpected status value from _notmuch_message_create_for_message_id");
+
+    }
+
+    if (private_status != NOTMUCH_PRIVATE_STATUS_NO_DOCUMENT_FOUND)
+	return NOTMUCH_STATUS_DUPLICATE_MESSAGE_ID;
+
+    _notmuch_message_add_term (message, "type", "mail");
+
+    if (tags) {
+	ret = notmuch_message_freeze (message);
+	if (ret)
+	    return ret;
+
+	for (tag = tags; *tag; tag++) {
+	    ret = notmuch_message_add_tag (message, *tag);
+	    if (ret)
+		return ret;
+	}
+    }
+
+    ret = notmuch_message_thaw (message);
+    if (ret)
+	return ret;
+
+    return NOTMUCH_STATUS_SUCCESS;
+}
diff --git a/test/database-test.h b/test/database-test.h
new file mode 100644
index 0000000..84f7988
--- /dev/null
+++ b/test/database-test.h
@@ -0,0 +1,21 @@
+#ifndef _DATABASE_TEST_H
+#define _DATABASE_TEST_H
+/* Add a new stub message to the given notmuch database.
+ *
+ * At least the following return values are possible:
+ *
+ * NOTMUCH_STATUS_SUCCESS: Message successfully added to database.
+ *
+ * NOTMUCH_STATUS_DUPLICATE_MESSAGE_ID: Message has the same message
+ *	ID as another message already in the database.
+ *
+ * NOTMUCH_STATUS_READ_ONLY_DATABASE: Database was opened in read-only
+ *	mode so no message can be added.
+ */
+
+notmuch_status_t
+notmuch_database_add_stub_message (notmuch_database_t *database,
+				   const char *message_id,
+				   const char **tag_list);
+
+#endif
-- 
1.7.10.4

[Patch v2 05/17] test: add generator for random "stub" messages

[david]

From: David Bremner <bremner@debian.org>

Initial use case is testing dump and restore, so we only have
message-ids and tags.

The message ID's are nothing like RFC compliant, but it doesn't seem
any harder to roundtrip random UTF-8 strings than RFC-compliant ones.

Tags are UTF-8, even though notmuch is in principle more generous than
that.

updated for id:m2wr04ocro.fsf@guru.guru-group.fi

- talk about Unicode value rather some specific encoding
- call talloc_realloc less times
---
 test/.gitignore      |    1 +
 test/Makefile.local  |   10 +++
 test/basic           |    1 +
 test/random-corpus.c |  204 ++++++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 216 insertions(+)
 create mode 100644 test/random-corpus.c

diff --git a/test/.gitignore b/test/.gitignore
index be7ab5e..1eff7ce 100644
--- a/test/.gitignore
+++ b/test/.gitignore
@@ -4,4 +4,5 @@ smtp-dummy
 symbol-test
 arg-test
 hex-xcode
+random-corpus
 tmp.*
diff --git a/test/Makefile.local b/test/Makefile.local
index 8479f91..6a9f15e 100644
--- a/test/Makefile.local
+++ b/test/Makefile.local
@@ -16,6 +16,14 @@ $(dir)/arg-test: $(dir)/arg-test.o command-line-arguments.o util/libutil.a
 $(dir)/hex-xcode: $(dir)/hex-xcode.o command-line-arguments.o util/libutil.a
 	$(call quiet,CC) -I. $^ -o $@ -ltalloc
 
+random_corpus_deps =  $(dir)/random-corpus.o  $(dir)/database-test.o \
+			notmuch-config.o command-line-arguments.o \
+			lib/libnotmuch.a util/libutil.a \
+			parse-time-string/libparse-time-string.a
+
+$(dir)/random-corpus: $(random_corpus_deps)
+	$(call quiet,CC) $(CFLAGS_FINAL) $^ -o $@ $(CONFIGURE_LDFLAGS)
+
 $(dir)/smtp-dummy: $(smtp_dummy_modules)
 	$(call quiet,CC) $^ -o $@
 
@@ -29,6 +37,7 @@ $(dir)/parse-time: $(dir)/parse-time.o parse-time-string/parse-time-string.o
 
 TEST_BINARIES=$(dir)/arg-test \
 	      $(dir)/hex-xcode \
+	      $(dir)/random-corpus \
 	      $(dir)/parse-time \
 	      $(dir)/smtp-dummy \
 	      $(dir)/symbol-test
@@ -46,5 +55,6 @@ CLEAN := $(CLEAN) $(dir)/smtp-dummy $(dir)/smtp-dummy.o \
 	 $(dir)/arg-test $(dir)/arg-test.o \
 	 $(dir)/hex-xcode $(dir)/hex-xcode.o \
 	 $(dir)/database-test.o \
+	 $(dir)/random-corpus $(dir)/random-corpus.o \
 	 $(dir)/parse-time $(dir)/parse-time.o \
 	 $(dir)/corpus.mail $(dir)/test-results $(dir)/tmp.*
diff --git a/test/basic b/test/basic
index 2a571ac..f93469f 100755
--- a/test/basic
+++ b/test/basic
@@ -59,6 +59,7 @@ available=$(find "$TEST_DIRECTORY" -maxdepth 1 -type f -perm +111  \
     ! -name hex-xcode			\
     ! -name notmuch-test		\
     ! -name parse-time			\
+    ! -name random-corpus		\
     ! -name smtp-dummy			\
     ! -name symbol-test			\
     ! -name test-verbose		\
diff --git a/test/random-corpus.c b/test/random-corpus.c
new file mode 100644
index 0000000..085bda0
--- /dev/null
+++ b/test/random-corpus.c
@@ -0,0 +1,204 @@
+/*
+ * Generate a random corpus of stub messages.
+ *
+ * Initial use case is testing dump and restore, so we only have
+ * message-ids and tags.
+ *
+ * Generated message-id's and tags are intentionally nasty.
+ *
+ * Copyright (c) 2012 David Bremner
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see http://www.gnu.org/licenses/ .
+ *
+ * Author: David Bremner <david@tethera.net>
+ */
+
+#include <stdlib.h>
+#include <assert.h>
+#include <talloc.h>
+#include <string.h>
+#include <glib.h>
+#include <math.h>
+
+#include "notmuch-client.h"
+#include "command-line-arguments.h"
+#include "database-test.h"
+
+/* Current largest Unicode value defined. Note that most of these will
+ * be printed as boxes in most fonts.
+ */
+
+#define GLYPH_MAX 0x10FFFE
+
+static gunichar
+random_unichar ()
+{
+    int start = 1, stop = GLYPH_MAX;
+    int class = random() % 2;
+
+    /*
+     *  Choose about half ascii as test characters, as ascii
+     *  punctation and whitespace is the main cause of problems for
+     *  the (old) restore parser
+    */
+    switch (class) {
+    case 0:
+	/* ascii */
+	start = 0x01;
+	stop = 0x7f;
+	break;
+    case 1:
+	/* the rest of unicode */
+	start = 0x80;
+	stop = GLYPH_MAX;
+    }
+
+    if (start == stop)
+	return start;
+    else
+	return start + (random() % (stop - start + 1));
+}
+
+static char *
+random_utf8_string (void *ctx, size_t char_count)
+{
+    size_t offset = 0;
+    size_t i;
+
+    gchar *buf = NULL;
+    size_t buf_size = 0;
+
+    for (i = 0; i < char_count; i++) {
+	gunichar randomchar;
+	size_t written;
+
+	/* 6 for one glyph, one for null, one for luck */
+	while (buf_size - offset < 8) {
+	    buf_size = 2 * buf_size + 8;
+	    buf = talloc_realloc (ctx, buf, gchar, buf_size);
+	}
+
+	randomchar = random_unichar();
+
+	written = g_unichar_to_utf8 (randomchar, buf + offset);
+
+	if (written <= 0) {
+	    fprintf (stderr, "error converting to utf8\n");
+	    exit (1);
+	}
+
+	offset += written;
+
+    }
+    buf[offset] = 0;
+    return buf;
+}
+
+
+int
+main (int argc, char **argv)
+{
+
+    void *ctx = talloc_new (NULL);
+
+    char *config_path  = NULL;
+    notmuch_config_t *config;
+    notmuch_database_t *notmuch;
+
+    int num_messages = 500;
+    int max_tags = 10;
+    // leave room for UTF-8 encoding.
+    int tag_len = NOTMUCH_TAG_MAX / 6;
+    // NOTMUCH_MESSAGE_ID_MAX is not exported, so we make a
+    // conservative guess.
+    int message_id_len = (NOTMUCH_TAG_MAX - 20) / 6;
+
+    int seed = 734569;
+
+    notmuch_opt_desc_t options[] = {
+	{ NOTMUCH_OPT_STRING, &config_path, "config-path", 'c', 0 },
+	{ NOTMUCH_OPT_INT, &num_messages, "num-messages", 'n', 0 },
+	{ NOTMUCH_OPT_INT, &max_tags, "max-tags", 'm', 0 },
+	{ NOTMUCH_OPT_INT, &message_id_len, "message-id-len", 'M', 0 },
+	{ NOTMUCH_OPT_INT, &tag_len, "tag-len", 't', 0 },
+	{ NOTMUCH_OPT_INT, &seed, "seed", 's', 0 },
+	{ 0, 0, 0, 0, 0 }
+    };
+
+    int opt_index = parse_arguments (argc, argv, options, 1);
+
+    if (opt_index < 0)
+	exit (1);
+
+    if (message_id_len < 1) {
+	fprintf (stderr, "message id's must be least length 1\n");
+	exit (1);
+    }
+
+    if (config_path == NULL) {
+	fprintf (stderr, "configuration path must be specified");
+	exit (1);
+    }
+
+    config = notmuch_config_open (ctx, config_path, NULL);
+    if (config == NULL)
+	return 1;
+
+    if (notmuch_database_open (notmuch_config_get_database_path (config),
+			       NOTMUCH_DATABASE_MODE_READ_WRITE, &notmuch))
+	return 1;
+
+    srandom (seed);
+
+    int count;
+    for (count = 0; count < num_messages; count++) {
+	int j;
+	/* explicitly allow zero tags */
+	int num_tags = random () % (max_tags + 1);
+	/* message ids should be non-empty */
+	int this_mid_len = (random () % message_id_len) + 1;
+	const char **tag_list;
+	char *mid;
+	notmuch_status_t status;
+
+	do {
+	    mid = random_utf8_string (ctx, this_mid_len);
+
+	    tag_list = talloc_realloc (ctx, NULL, const char *, num_tags + 2);
+
+	    tag_list[0] = "random-corpus";
+
+	    for (j = 0; j < num_tags; j++) {
+		int this_tag_len = random () % tag_len + 1;
+
+		tag_list[j + 1] = random_utf8_string (ctx, this_tag_len);
+	    }
+
+	    tag_list[j + 1] = NULL;
+
+	    status = notmuch_database_add_stub_message (notmuch, mid, tag_list);
+	} while (status == NOTMUCH_STATUS_DUPLICATE_MESSAGE_ID);
+
+	if (status != NOTMUCH_STATUS_SUCCESS) {
+	    fprintf (stderr, "error %d adding message", status);
+	    exit (status);
+	}
+    }
+
+    notmuch_database_destroy (notmuch);
+
+    talloc_free (ctx);
+
+    return 0;
+}
-- 
1.7.10.4

[Patch v2 06/17] test: add broken roundtrip test

[david]

From: David Bremner <bremner@debian.org>

We demonstrate the current notmuch restore parser being confused by
message-id's and tags containing non alpha numeric characters
(particularly space and parentheses are problematic because they are
not escaped by notmuch dump).

We save the files as hex escaped on disk so that the output from the
failing test will not confuse the terminal emulator of people running
the test.
---
 test/dump-restore |    9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/test/dump-restore b/test/dump-restore
index b05399c..a2204fb 100755
--- a/test/dump-restore
+++ b/test/dump-restore
@@ -85,4 +85,13 @@ test_begin_subtest "dump --output=outfile -- from:cworth"
 notmuch dump --output=dump-outfile-dash-inbox.actual -- from:cworth
 test_expect_equal_file dump-cworth.expected dump-outfile-dash-inbox.actual
 
+test_expect_success 'roundtripping random message-ids and tags' \
+    'test_subtest_known_broken &&
+     ${TEST_DIRECTORY}/random-corpus --num-messages=10 --config-path=${NOTMUCH_CONFIG} &&
+     notmuch dump | ${TEST_DIRECTORY}/hex-xcode --direction=encode > EXPECTED.$test_count &&
+     notmuch tag -random-corpus tag:random-corpus &&
+     ${TEST_DIRECTORY}/hex-xcode --direction=decode < EXPECTED.$test_count | notmuch restore 2>/dev/null &&
+     notmuch dump | ${TEST_DIRECTORY}/hex-xcode --direction=encode > OUTPUT.$test_count &&
+     test_cmp EXPECTED.$test_count OUTPUT.$test_count'
+
 test_done
-- 
1.7.10.4

[Patch v2 07/17] notmuch-dump: add --format=(batch-tag|sup)

[david]

From: David Bremner <bremner@debian.org>

sup is the old format, and remains the default, at least until
restore is converted to parse this format.

Each line of the batch- format is valid for notmuch tag, (with the
"notmuch tag" omitted from the front of the line). The dump format
only uses query strings of a single message-id.

Each space seperated tag/message-id is 'hex-encoded' to remove
troubling characters.  In particular this format won't have the same
problem with e.g. spaces in message-ids or tags; they will be
round-trip-able.
---
 dump-restore-private.h |   13 +++++++++++++
 notmuch-dump.c         |   42 ++++++++++++++++++++++++++++++++++++------
 2 files changed, 49 insertions(+), 6 deletions(-)
 create mode 100644 dump-restore-private.h

diff --git a/dump-restore-private.h b/dump-restore-private.h
new file mode 100644
index 0000000..896a004
--- /dev/null
+++ b/dump-restore-private.h
@@ -0,0 +1,13 @@
+#ifndef DUMP_RESTORE_PRIVATE_H
+#define DUMP_RESTORE_PRIVATE_H
+
+#include "hex-escape.h"
+#include "command-line-arguments.h"
+
+typedef enum dump_formats {
+    DUMP_FORMAT_AUTO,
+    DUMP_FORMAT_BATCH_TAG,
+    DUMP_FORMAT_SUP
+} dump_format_t;
+
+#endif
diff --git a/notmuch-dump.c b/notmuch-dump.c
index 88f598a..045ca9e 100644
--- a/notmuch-dump.c
+++ b/notmuch-dump.c
@@ -19,6 +19,7 @@
  */
 
 #include "notmuch-client.h"
+#include "dump-restore-private.h"
 
 int
 notmuch_dump_command (unused (void *ctx), int argc, char *argv[])
@@ -43,7 +44,13 @@ notmuch_dump_command (unused (void *ctx), int argc, char *argv[])
     char *output_file_name = NULL;
     int opt_index;
 
+    int output_format = DUMP_FORMAT_SUP;
+
     notmuch_opt_desc_t options[] = {
+	{ NOTMUCH_OPT_KEYWORD, &output_format, "format", 'f',
+	  (notmuch_keyword_t []){ { "sup", DUMP_FORMAT_SUP },
+				  { "batch-tag", DUMP_FORMAT_BATCH_TAG },
+				  { 0, 0 } } },
 	{ NOTMUCH_OPT_STRING, &output_file_name, "output", 'o', 0  },
 	{ 0, 0, 0, 0, 0 }
     };
@@ -83,27 +90,50 @@ notmuch_dump_command (unused (void *ctx), int argc, char *argv[])
      */
     notmuch_query_set_sort (query, NOTMUCH_SORT_UNSORTED);
 
+    char *buffer = NULL;
+    size_t buffer_size = 0;
+
     for (messages = notmuch_query_search_messages (query);
 	 notmuch_messages_valid (messages);
 	 notmuch_messages_move_to_next (messages)) {
 	int first = 1;
+	const char *message_id;
+
 	message = notmuch_messages_get (messages);
+	message_id = notmuch_message_get_message_id (message);
 
-	fprintf (output,
-		 "%s (", notmuch_message_get_message_id (message));
+	if (output_format == DUMP_FORMAT_SUP) {
+	    fprintf (output, "%s (", message_id);
+	}
 
 	for (tags = notmuch_message_get_tags (message);
 	     notmuch_tags_valid (tags);
 	     notmuch_tags_move_to_next (tags)) {
-	    if (! first)
-		fprintf (output, " ");
+	    const char *tag_str = notmuch_tags_get (tags);
 
-	    fprintf (output, "%s", notmuch_tags_get (tags));
+	    if (! first)
+		fputs (" ", output);
 
 	    first = 0;
+
+	    if (output_format == DUMP_FORMAT_SUP) {
+		fputs (tag_str, output);
+	    } else {
+		if (hex_encode (notmuch, tag_str,
+				&buffer, &buffer_size) != HEX_SUCCESS)
+		    return 1;
+		fprintf (output, "+%s", buffer);
+	    }
 	}
 
-	fprintf (output, ")\n");
+	if (output_format == DUMP_FORMAT_SUP) {
+	    fputs (")\n", output);
+	} else {
+	    if (hex_encode (notmuch, message_id,
+			    &buffer, &buffer_size) != HEX_SUCCESS)
+		return 1;
+	    fprintf (output, " -- id:%s\n", buffer);
+	}
 
 	notmuch_message_destroy (message);
     }
-- 
1.7.10.4

[Patch v2 08/17] util: add string-util.[ch]

[david]

From: David Bremner <bremner@debian.org>

This is to give a home to strtok_len. It's a bit silly to add a header
for one routine, but it needs to be shared between several compilation
units (or at least that's the most natural design).
---
 util/Makefile.local |    3 ++-
 util/string-util.c  |   34 ++++++++++++++++++++++++++++++++++
 util/string-util.h  |   19 +++++++++++++++++++
 3 files changed, 55 insertions(+), 1 deletion(-)
 create mode 100644 util/string-util.c
 create mode 100644 util/string-util.h

diff --git a/util/Makefile.local b/util/Makefile.local
index 3ca623e..a11e35b 100644
--- a/util/Makefile.local
+++ b/util/Makefile.local
@@ -3,7 +3,8 @@
 dir := util
 extra_cflags += -I$(srcdir)/$(dir)
 
-libutil_c_srcs := $(dir)/xutil.c $(dir)/error_util.c $(dir)/hex-escape.c
+libutil_c_srcs := $(dir)/xutil.c $(dir)/error_util.c $(dir)/hex-escape.c \
+		  $(dir)/string-util.c
 
 libutil_modules := $(libutil_c_srcs:.c=.o)
 
diff --git a/util/string-util.c b/util/string-util.c
new file mode 100644
index 0000000..44f8cd3
--- /dev/null
+++ b/util/string-util.c
@@ -0,0 +1,34 @@
+/* string-util.c -  Extra or enhanced routines for null terminated strings.
+ *
+ * Copyright (c) 2012 Jani Nikula
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see http://www.gnu.org/licenses/ .
+ *
+ * Author: Jani Nikula <jani@nikula.org>
+ */
+
+
+#include "string-util.h"
+
+char *
+strtok_len (char *s, const char *delim, size_t *len)
+{
+    /* skip initial delims */
+    s += strspn (s, delim);
+
+    /* length of token */
+    *len = strcspn (s, delim);
+
+    return *len ? s : NULL;
+}
diff --git a/util/string-util.h b/util/string-util.h
new file mode 100644
index 0000000..696da40
--- /dev/null
+++ b/util/string-util.h
@@ -0,0 +1,19 @@
+#ifndef _STRING_UTIL_H
+#define _STRING_UTIL_H
+
+#include <string.h>
+
+/* like strtok(3), but without state, and doesn't modify s. usage pattern:
+ *
+ * const char *tok = input;
+ * const char *delim = " \t";
+ * size_t tok_len = 0;
+ *
+ * while ((tok = strtok_len (tok + tok_len, delim, &tok_len)) != NULL) {
+ *     // do stuff with string tok of length tok_len
+ * }
+ */
+
+char *strtok_len (char *s, const char *delim, size_t *len);
+
+#endif
-- 
1.7.10.4

[Patch v2 09/17] tag-util.[ch]: New files for common tagging routines

[david]

From: David Bremner <bremner@debian.org>

These are meant to be shared between notmuch-tag and notmuch-restore.

The bulk of the routines implement a "tag operation list" abstract
data type act as a structured representation of a set of tag
operations (typically coming from a single tag command or line of
input).
---
 Makefile.local |    1 +
 tag-util.c     |  264 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 tag-util.h     |  120 ++++++++++++++++++++++++++
 3 files changed, 385 insertions(+)
 create mode 100644 tag-util.c
 create mode 100644 tag-util.h

diff --git a/Makefile.local b/Makefile.local
index 2b91946..854867d 100644
--- a/Makefile.local
+++ b/Makefile.local
@@ -274,6 +274,7 @@ notmuch_client_srcs =		\
 	query-string.c		\
 	mime-node.c		\
 	crypto.c		\
+	tag-util.c
 
 notmuch_client_modules = $(notmuch_client_srcs:.c=.o)
 
diff --git a/tag-util.c b/tag-util.c
new file mode 100644
index 0000000..287cc67
--- /dev/null
+++ b/tag-util.c
@@ -0,0 +1,264 @@
+#include "string-util.h"
+#include "tag-util.h"
+#include "hex-escape.h"
+
+struct _tag_operation_t {
+    const char *tag;
+    notmuch_bool_t remove;
+};
+
+struct _tag_op_list_t {
+    tag_operation_t *ops;
+    int count;
+    int size;
+};
+
+int
+parse_tag_line (void *ctx, char *line,
+		tag_op_flag_t flags,
+		char **query_string,
+		tag_op_list_t *tag_ops)
+{
+    char *tok = line;
+    size_t tok_len = 0;
+
+    chomp_newline (line);
+
+    /* remove leading space */
+    while (*tok == ' ' || *tok == '\t')
+	tok++;
+
+    /* Skip empty and comment lines. */
+    if (*tok == '\0' || *tok == '#')
+	    return 1;
+
+    tag_op_list_reset (tag_ops);
+
+    /* Parse tags. */
+    while ((tok = strtok_len (tok + tok_len, " ", &tok_len)) != NULL) {
+	notmuch_bool_t remove;
+	char *tag;
+
+	/* Optional explicit end of tags marker. */
+	if (strncmp (tok, "--", tok_len) == 0) {
+	    tok = strtok_len (tok + tok_len, " ", &tok_len);
+	    break;
+	}
+
+	/* Implicit end of tags. */
+	if (*tok != '-' && *tok != '+')
+	    break;
+
+	/* If tag is terminated by NUL, there's no query string. */
+	if (*(tok + tok_len) == '\0') {
+	    tok = NULL;
+	    break;
+	}
+
+	/* Terminate, and start next token after terminator. */
+	*(tok + tok_len++) = '\0';
+
+	remove = (*tok == '-');
+	tag = tok + 1;
+
+	/* Maybe refuse empty tags. */
+	if (!(flags & TAG_FLAG_BE_GENEROUS) && *tag == '\0') {
+	    tok = NULL;
+	    break;
+	}
+
+	/* Decode tag. */
+	if (hex_decode_inplace (tag) != HEX_SUCCESS) {
+	    tok = NULL;
+	    break;
+	}
+
+	if (tag_op_list_append (ctx, tag_ops, tag, remove))
+	    return -1;
+    }
+
+    if (tok == NULL || tag_ops->count == 0) {
+	/* FIXME: line has been modified! */
+	fprintf (stderr, "Warning: Ignoring invalid input line: %s\n",
+		 line);
+	return 1;
+    }
+
+    /* tok now points to the query string */
+    if (hex_decode_inplace (tok) != HEX_SUCCESS) {
+	/* FIXME: line has been modified! */
+	fprintf (stderr, "Warning: Ignoring invalid input line: %s\n",
+		 line);
+	return 1;
+    }
+
+    *query_string = tok;
+
+    return 0;
+}
+
+static inline void
+message_error (notmuch_message_t *message,
+	       notmuch_status_t status,
+	       const char *format, ...)
+{
+    va_list va_args;
+
+    va_start (va_args, format);
+
+    vfprintf (stderr, format, va_args);
+    fprintf (stderr, "Message-ID: %s\n", notmuch_message_get_message_id (message));
+    fprintf (stderr, "Status: %s\n", notmuch_status_to_string (status));
+}
+
+notmuch_status_t
+tag_op_list_apply (notmuch_message_t *message,
+		   tag_op_list_t *list,
+		   tag_op_flag_t flags)
+{
+    int i;
+
+    notmuch_status_t status = 0;
+    tag_operation_t *tag_ops = list->ops;
+
+    status = notmuch_message_freeze (message);
+    if (status) {
+	message_error (message, status, "freezing message");
+	return status;
+    }
+
+    if (flags & TAG_FLAG_REMOVE_ALL) {
+	status = notmuch_message_remove_all_tags (message);
+	if (status) {
+	    message_error (message, status, "removing all tags" );
+	    return status;
+	}
+    }
+
+    for (i = 0; i < list->count; i++) {
+	if (tag_ops[i].remove) {
+	    status = notmuch_message_remove_tag (message, tag_ops[i].tag);
+	    if (status) {
+		message_error (message, status, "removing tag %s", tag_ops[i].tag);
+		return status;
+	    }
+	} else {
+	    status = notmuch_message_add_tag (message, tag_ops[i].tag);
+	    if (status) {
+		message_error (message, status, "adding tag %s", tag_ops[i].tag);
+		return status;
+	    }
+
+	}
+    }
+
+    status = notmuch_message_thaw (message);
+    if (status) {
+	message_error (message, status, "thawing message");
+	return status;
+    }
+
+
+    if (flags & TAG_FLAG_MAILDIR_SYNC) {
+	status = notmuch_message_tags_to_maildir_flags (message);
+	if (status) {
+	    message_error (message, status, "synching tags to maildir");
+	    return status;
+	}
+    }
+
+    return NOTMUCH_STATUS_SUCCESS;
+
+}
+
+
+/* Array of tagging operations (add or remove), terminated with an
+ * empty element. Size will be increased as necessary. */
+
+tag_op_list_t *
+tag_op_list_create (void *ctx)
+{
+    tag_op_list_t *list;
+
+    list = talloc (ctx, tag_op_list_t);
+    if (list == NULL)
+	return NULL;
+
+    list->size = TAG_OP_LIST_INITIAL_SIZE;
+    list->count = 0;
+
+    list->ops = talloc_array (ctx, tag_operation_t, list->size);
+    if (list->ops == NULL)
+	return NULL;
+
+    return list;
+}
+
+
+int
+tag_op_list_append (void *ctx,
+		    tag_op_list_t *list,
+		    const char *tag,
+		    notmuch_bool_t remove)
+{
+    /* Make room if current array is full.  This should be a fairly
+     * rare case, considering the initial array size.
+     */
+
+    if (list->count == list->size) {
+	list->size *= 2;
+	list->ops = talloc_realloc (ctx, list->ops, tag_operation_t,
+				    list->size);
+	if (list->ops == NULL) {
+	    fprintf (stderr, "Out of memory.\n");
+	    return 1;
+	}
+    }
+
+    /* add the new operation */
+
+    list->ops[list->count].tag = tag;
+    list->ops[list->count].remove = remove;
+    list->count++;
+    return 0;
+}
+
+/*
+ *   Is the i'th tag operation a remove?
+ */
+
+notmuch_bool_t
+tag_op_list_isremove (const tag_op_list_t *list, size_t i)
+{
+    return list->ops[i].remove;
+}
+
+/*
+ * Reset a list to contain no operations
+ */
+
+void
+tag_op_list_reset (tag_op_list_t *list)
+{
+    list->count = 0;
+}
+
+/*
+ * Return the number of operations in a list
+ */
+
+size_t
+tag_op_list_size (const tag_op_list_t *list)
+{
+    return list->count;
+}
+
+/*
+ *   return the i'th tag in the list
+ */
+
+const char *
+tag_op_list_tag (const tag_op_list_t *list, size_t i)
+{
+    return list->ops[i].tag;
+}
diff --git a/tag-util.h b/tag-util.h
new file mode 100644
index 0000000..508806f
--- /dev/null
+++ b/tag-util.h
@@ -0,0 +1,120 @@
+#ifndef _TAG_UTIL_H
+#define _TAG_UTIL_H
+
+#include "notmuch-client.h"
+
+typedef struct _tag_operation_t tag_operation_t;
+typedef struct _tag_op_list_t tag_op_list_t;
+
+#define TAG_OP_LIST_INITIAL_SIZE 10
+
+/* Use powers of 2 */
+typedef enum  { TAG_FLAG_NONE = 0,
+		/* Operations are synced to maildir, if possible */
+
+		TAG_FLAG_MAILDIR_SYNC = 1,
+
+		/* Remove all tags from message before applying
+		 * list */
+
+		TAG_FLAG_REMOVE_ALL = 2,
+
+		/* Don't try to avoid database operations.  Useful
+		 * when we know that message passed needs these
+		 *  operations. */
+
+		TAG_FLAG_PRE_OPTIMIZED = 4,
+
+		/* Accept strange tags that might be user error;
+		   intended for use by notmuch-restore.
+		*/
+
+		TAG_FLAG_BE_GENEROUS = 8} tag_op_flag_t;
+
+/* Parse a string of the following format:
+ *
+ * +<tag>|-<tag> [...] [--] <search-terms>
+ *
+ * Each line is interpreted similarly to "notmuch tag" command line
+ * arguments. The delimiter is one or more spaces ' '. Any characters
+ * in <tag> and <search-terms> MAY be hex encoded with %NN where NN is
+ * the hexadecimal value of the character. Any ' ' and '%' characters
+ * in <tag> and <search-terms> MUST be hex encoded (using %20 and %25,
+ * respectively). Any characters that are not part of <tag> or
+ * <search-terms> MUST NOT be hex encoded.
+ *
+ * Leading and trailing space ' ' is ignored. Empty lines and lines
+ * beginning with '#' are ignored.
+ *
+ * Returns: 0 for OK, 1 for skipped line, -1 for fatal(ish) error.
+ *
+ * Output Parameters:
+ *	ops	contains a list of tag operations
+ *	query_str the search terms.
+ */
+int
+parse_tag_line (void *ctx, char *line,
+		tag_op_flag_t flags,
+		char **query_str, tag_op_list_t *ops);
+
+/*
+ * Create an empty list of tag operations
+ *
+ * ctx is passed to talloc
+ */
+
+tag_op_list_t
+*tag_op_list_create (void *ctx);
+
+/*
+ * Add a tag operation (delete iff remote == TRUE) to a list.
+ * The list is expanded as necessary.
+ */
+
+int
+tag_op_list_append (void *ctx,
+		    tag_op_list_t *list,
+		    const char *tag,
+		    notmuch_bool_t remove);
+
+/*
+ * Apply a list of tag operations, in order to a message.
+ *
+ * Flags can be bitwise ORed; see enum above for possibilies.
+ */
+
+notmuch_status_t
+tag_op_list_apply (notmuch_message_t *message,
+		   tag_op_list_t *tag_ops,
+		   tag_op_flag_t flags);
+
+/*
+ * Return the number of operations in a list
+ */
+
+size_t
+tag_op_list_size (const tag_op_list_t *list);
+
+/*
+ * Reset a list to contain no operations
+ */
+
+void
+tag_op_list_reset (tag_op_list_t *list);
+
+
+ /*
+  *   return the i'th tag in the list
+  */
+
+const char *
+tag_op_list_tag (const tag_op_list_t *list, size_t i);
+
+/*
+ *   Is the i'th tag operation a remove?
+ */
+
+notmuch_bool_t
+tag_op_list_isremove (const tag_op_list_t *list, size_t i);
+
+#endif
-- 
1.7.10.4

[Patch v2 10/17] 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 --stdin command line option to
"notmuch tag". The input must consist of lines of the format:

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

Each line is interpreted similarly to "notmuch tag" command line
arguments. The delimiter is one or more spaces ' '. Any characters in
<tag> and <search-terms> MAY be hex encoded with %NN where NN is the
hexadecimal value of the character. Any ' ' and '%' characters in
<tag> and <search-terms> MUST be hex encoded (using %20 and %25,
respectively). Any characters that are not part of <tag> or
<search-terms> 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 |  194 +++++++++++++++++++++++++++++++++++----------------------
 1 file changed, 118 insertions(+), 76 deletions(-)

diff --git a/notmuch-tag.c b/notmuch-tag.c
index 88d559b..8a8af0b 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 ";
     }
 
@@ -116,12 +113,11 @@ _optimize_tag_query (void *ctx, const char *orig_query_string,
  * element. */
 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 +140,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);
 	notmuch_message_destroy (message);
     }
 
@@ -170,15 +152,17 @@ 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 synchronize_flags = TAG_FLAG_NONE;
+    notmuch_bool_t batch = FALSE;
+    FILE *input = stdin;
+    char *input_file_name = NULL;
+    int i, opt_index;
+    int ret = 0;
 
     /* Setup our handler for SIGINT */
     memset (&action, 0, sizeof (struct sigaction));
@@ -187,53 +171,76 @@ notmuch_tag_command (void *ctx, int argc, char *argv[])
     action.sa_flags = SA_RESTART;
     sigaction (SIGINT, &action, NULL);
 
-    argc--; argv++; /* skip subcommand argument */
+    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 }
+    };
 
-    /* Array of tagging operations (add or remove), terminated with an
-     * empty element. */
-    tag_ops = talloc_array (ctx, tag_operation_t, argc + 1);
-    if (tag_ops == NULL) {
-	fprintf (stderr, "Out of memory.\n");
+    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;
+	}
     }
 
-    for (i = 0; i < argc; i++) {
-	if (strcmp (argv[i], "--") == 0) {
-	    i++;
-	    break;
+    if (batch) {
+	if (opt_index != argc) {
+	    fprintf (stderr, "Can't specify both cmdline and stdin!\n");
+	    return 1;
+	}
+    } else {
+	/* Array of tagging operations (add or remove), terminated with an
+	 * empty element. */
+	tag_ops = tag_op_list_create (ctx);
+	if (tag_ops == NULL) {
+	    fprintf (stderr, "Out of memory.\n");
+	    return 1;
 	}
-	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;
+
+	for (i = opt_index; i < argc; i++) {
+	    if (strcmp (argv[i], "--") == 0) {
+		i++;
+		break;
 	    }
-	    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;
+	    if (argv[i][0] == '+' || argv[i][0] == '-') {
+		/* FIXME: checks should be consistent with those in batch tagging */
+		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_op_list_append (ctx, tag_ops,
+				    argv[i] + 1, (argv[i][0] == '-'));
+	    } else {
+		break;
 	    }
-	    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;
-    }
+	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;
+	}
 
-    query_string = query_string_from_args (ctx, argc - i, &argv[i]);
+	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");
-	return 1;
+	if (*query_string == '\0') {
+	    fprintf (stderr, "Error: notmuch tag requires at least one search term.\n");
+	    return 1;
+	}
     }
 
     config = notmuch_config_open (ctx, NULL, NULL);
@@ -244,11 +251,46 @@ 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))
+	synchronize_flags = TAG_FLAG_MAILDIR_SYNC;
+
+    if (batch) {
+
+	char *line = 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;
+	}
 
-    ret = tag_query (ctx, notmuch, query_string, tag_ops, synchronize_flags);
+	while ((line_len = getline (&line, &line_size, input)) != -1 &&
+	       ! interrupted) {
+
+	    char *query_string;
+
+	    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, synchronize_flags))
+		break;
+	}
+
+	if (line)
+	    free (line);
+    } else
+	ret = tag_query (ctx, notmuch, query_string, tag_ops, synchronize_flags);
 
     notmuch_database_destroy (notmuch);
 
-    return ret;
+    return ret || interrupted;
+
 }
-- 
1.7.10.4

[Patch v2 11/17] test: add test for notmuch tag --batch option

[david]

From: Jani Nikula <jani@nikula.org>

Basic test of functionality, along with all combinations of options.
---
 test/tagging |   46 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 46 insertions(+)

diff --git a/test/tagging b/test/tagging
index 980ff92..e5b8315 100755
--- a/test/tagging
+++ b/test/tagging
@@ -46,6 +46,52 @@ 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)"
+
+> batch.in  <<EOF
+# %20 is a space in tag
+-:"%20 -tag1 +tag5 +tag6 -- One
++tag1 -tag1 -tag4 +tag4 -- Two
+-tag6 One
++tag5 Two
+EOF
+
+test_begin_subtest "--input"
+notmuch tag --input=batch.in
+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)"
+
+test_begin_subtest "--batch --input"
+notmuch tag --batch --input=batch.in
+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)"
+
+test_begin_subtest "--batch, blank lines and comments"
+notmuch dump | sort > EXPECTED.$test_count
+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_count
+test_expect_equal_file EXPECTED.$test_count OUTPUT.$test_count
+
 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 v2 12/17] man: document notmuch tag --batch, --input options

[david]

From: Jani Nikula <jani@nikula.org>

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

diff --git a/man/man1/notmuch-tag.1 b/man/man1/notmuch-tag.1
index 0f86582..751db7b 100644
--- a/man/man1/notmuch-tag.1
+++ b/man/man1/notmuch-tag.1
@@ -4,7 +4,12 @@ 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-terms ">"
+
+.B notmuch tag
+.RI "--batch"
+.RI "[ --input=<" filename "> ]"
+
 
 .SH DESCRIPTION
 
@@ -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 standard input. 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 "T +<" tag ">|\-<" tag "> [...] [\-\-] <" search-terms ">"
+
+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-terms>
+.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

[Patch v2 13/17] notmuch-restore: add support for input format 'batch-tag'

[david]

From: David Bremner <bremner@debian.org>

This is the same as the batch input for notmuch tag, except by default
it removes all tags before modifying a given message id and only "id:"
is supported.
---
 notmuch-restore.c |  199 +++++++++++++++++++++++++++++++++--------------------
 1 file changed, 125 insertions(+), 74 deletions(-)

diff --git a/notmuch-restore.c b/notmuch-restore.c
index f03dcac..22fcd2d 100644
--- a/notmuch-restore.c
+++ b/notmuch-restore.c
@@ -19,18 +19,22 @@
  */
 
 #include "notmuch-client.h"
+#include "dump-restore-private.h"
+#include "tag-util.h"
+#include "string-util.h"
+
+static volatile sig_atomic_t interrupted;
+static regex_t regex;
 
 static int
-tag_message (notmuch_database_t *notmuch, const char *message_id,
-	     char *file_tags, notmuch_bool_t remove_all,
-	     notmuch_bool_t synchronize_flags)
+tag_message (unused (void *ctx),
+	     notmuch_database_t *notmuch,
+	     const char *message_id,
+	     tag_op_list_t *tag_ops,
+	     tag_op_flag_t flags)
 {
     notmuch_status_t status;
-    notmuch_tags_t *db_tags;
-    char *db_tags_str;
     notmuch_message_t *message = NULL;
-    const char *tag;
-    char *next;
     int ret = 0;
 
     status = notmuch_database_find_message (notmuch, message_id, &message);
@@ -44,55 +48,63 @@ tag_message (notmuch_database_t *notmuch, const char *message_id,
 
     /* In order to detect missing messages, this check/optimization is
      * intentionally done *after* first finding the message. */
-    if (! remove_all && (file_tags == NULL || *file_tags == '\0'))
-	goto DONE;
-
-    db_tags_str = NULL;
-    for (db_tags = notmuch_message_get_tags (message);
-	 notmuch_tags_valid (db_tags);
-	 notmuch_tags_move_to_next (db_tags)) {
-	tag = notmuch_tags_get (db_tags);
-
-	if (db_tags_str)
-	    db_tags_str = talloc_asprintf_append (db_tags_str, " %s", tag);
-	else
-	    db_tags_str = talloc_strdup (message, tag);
-    }
+    if ( (flags & TAG_FLAG_REMOVE_ALL) || (tag_op_list_size (tag_ops)))
+	tag_op_list_apply (message, tag_ops, flags);
 
-    if (((file_tags == NULL || *file_tags == '\0') &&
-	 (db_tags_str == NULL || *db_tags_str == '\0')) ||
-	(file_tags && db_tags_str && strcmp (file_tags, db_tags_str) == 0))
-	goto DONE;
+    if (message)
+	notmuch_message_destroy (message);
 
-    notmuch_message_freeze (message);
+    return ret;
+}
 
-    if (remove_all)
-	notmuch_message_remove_all_tags (message);
+static int
+parse_sup_line (void *ctx, char *line,
+		char **query_str, tag_op_list_t *tag_ops)
+{
 
-    next = file_tags;
-    while (next) {
-	tag = strsep (&next, " ");
-	if (*tag == '\0')
-	    continue;
-	status = notmuch_message_add_tag (message, tag);
-	if (status) {
-	    fprintf (stderr, "Error applying tag %s to message %s:\n",
-		     tag, message_id);
-	    fprintf (stderr, "%s\n", notmuch_status_to_string (status));
-	    ret = 1;
-	}
+    regmatch_t match[3];
+    char *file_tags;
+    int rerr;
+
+    tag_op_list_reset (tag_ops);
+
+    chomp_newline (line);
+
+    /* Silently ignore blank lines */
+    if (line[0] == '\0') {
+	return 1;
+    }
+
+    rerr = xregexec (&regex, line, 3, match, 0);
+    if (rerr == REG_NOMATCH) {
+	fprintf (stderr, "Warning: Ignoring invalid input line: %s\n",
+		 line);
+	return 1;
     }
 
-    notmuch_message_thaw (message);
+    *query_str = talloc_strndup (ctx, line + match[1].rm_so,
+				 match[1].rm_eo - match[1].rm_so);
+    file_tags = talloc_strndup (ctx, line + match[2].rm_so,
+				match[2].rm_eo - match[2].rm_so);
 
-    if (synchronize_flags)
-	notmuch_message_tags_to_maildir_flags (message);
+    char *tok = file_tags;
+    size_t tok_len = 0;
 
-  DONE:
-    if (message)
-	notmuch_message_destroy (message);
+    tag_op_list_reset (tag_ops);
+
+    while ((tok = strtok_len (tok + tok_len, " ", &tok_len)) != NULL) {
+
+	if (*(tok + tok_len) != '\0') {
+	    *(tok + tok_len) = '\0';
+	    tok_len++;
+	}
+
+	if (tag_op_list_append (ctx, tag_ops, tok, FALSE))
+	    return -1;
+    }
+
+    return 0;
 
-    return ret;
 }
 
 int
@@ -100,16 +112,19 @@ notmuch_restore_command (unused (void *ctx), int argc, char *argv[])
 {
     notmuch_config_t *config;
     notmuch_database_t *notmuch;
-    notmuch_bool_t synchronize_flags;
     notmuch_bool_t accumulate = FALSE;
+    tag_op_flag_t flags = 0;
+    tag_op_list_t *tag_ops;
+
     char *input_file_name = NULL;
     FILE *input = stdin;
     char *line = NULL;
     size_t line_size;
     ssize_t line_len;
-    regex_t regex;
-    int rerr;
+
+    int ret = 0;
     int opt_index;
+    int input_format = DUMP_FORMAT_AUTO;
 
     config = notmuch_config_open (ctx, NULL, NULL);
     if (config == NULL)
@@ -119,9 +134,15 @@ notmuch_restore_command (unused (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))
+	flags |= TAG_FLAG_MAILDIR_SYNC;
 
     notmuch_opt_desc_t options[] = {
+	{ NOTMUCH_OPT_KEYWORD, &input_format, "format", 'f',
+	  (notmuch_keyword_t []){ { "auto", DUMP_FORMAT_AUTO },
+				  { "batch-tag", DUMP_FORMAT_BATCH_TAG },
+				  { "sup", DUMP_FORMAT_SUP },
+				  { 0, 0 } } },
 	{ NOTMUCH_OPT_STRING, &input_file_name, "input", 'i', 0 },
 	{ NOTMUCH_OPT_BOOLEAN,  &accumulate, "accumulate", 'a', 0 },
 	{ 0, 0, 0, 0, 0 }
@@ -134,6 +155,9 @@ notmuch_restore_command (unused (void *ctx), int argc, char *argv[])
 	return 1;
     }
 
+    if (! accumulate)
+	flags |= TAG_FLAG_REMOVE_ALL;
+
     if (input_file_name) {
 	input = fopen (input_file_name, "r");
 	if (input == NULL) {
@@ -154,35 +178,61 @@ notmuch_restore_command (unused (void *ctx), int argc, char *argv[])
      * non-space characters for the message-id, then one or more
      * spaces, then a list of space-separated tags as a sequence of
      * characters within literal '(' and ')'. */
-    if ( xregcomp (&regex,
-		   "^([^ ]+) \\(([^)]*)\\)$",
-		   REG_EXTENDED) )
-	INTERNAL_ERROR ("compile time constant regex failed.");
+    char *p;
 
-    while ((line_len = getline (&line, &line_size, input)) != -1) {
-	regmatch_t match[3];
-	char *message_id, *file_tags;
+    line_len = getline (&line, &line_size, input);
+    if (line_len == 0)
+	return 0;
 
-	chomp_newline (line);
+    for (p = line; *p; p++) {
+	if (*p == '(')
+	    input_format = DUMP_FORMAT_SUP;
+    }
 
-	rerr = xregexec (&regex, line, 3, match, 0);
-	if (rerr == REG_NOMATCH) {
-	    fprintf (stderr, "Warning: Ignoring invalid input line: %s\n",
-		     line);
-	    continue;
+    if (input_format == DUMP_FORMAT_AUTO)
+	input_format = DUMP_FORMAT_BATCH_TAG;
+
+    if (input_format == DUMP_FORMAT_SUP)
+	if ( xregcomp (&regex,
+		       "^([^ ]+) \\(([^)]*)\\)$",
+		       REG_EXTENDED) )
+	    INTERNAL_ERROR ("compile time constant regex failed.");
+
+    tag_ops = tag_op_list_create (ctx);
+    if (tag_ops == NULL) {
+	fprintf (stderr, "Out of memory.\n");
+	return 1;
+    }
+
+    do {
+	char *query_string;
+
+	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,
+				   &query_string, tag_ops);
+
+	    if (ret == 0) {
+		if ( strncmp ("id:", query_string, 3) != 0) {
+		    fprintf (stderr, "Unsupported query: %s\n", query_string);
+		    continue;
+		}
+		/* delete id: from front of string; tag_message expects a
+		 * raw message-id */
+		query_string = query_string + 3;
+	    }
 	}
 
-	message_id = xstrndup (line + match[1].rm_so,
-			       match[1].rm_eo - match[1].rm_so);
-	file_tags = xstrndup (line + match[2].rm_so,
-			      match[2].rm_eo - match[2].rm_so);
+	if (ret > 0)
+	    continue;
 
-	tag_message (notmuch, message_id, file_tags, ! accumulate,
-		     synchronize_flags);
+	if (ret < 0 || tag_message (ctx, notmuch, query_string,
+				    tag_ops, flags))
+	    break;
+
+    }  while ((line_len = getline (&line, &line_size, input)) != -1);
 
-	free (message_id);
-	free (file_tags);
-    }
 
     regfree (&regex);
 
@@ -190,8 +240,9 @@ notmuch_restore_command (unused (void *ctx), int argc, char *argv[])
 	free (line);
 
     notmuch_database_destroy (notmuch);
+
     if (input != stdin)
 	fclose (input);
 
-    return 0;
+    return ret;
 }
-- 
1.7.10.4

[Patch v2 14/17] test: update dump-restore roundtripping test for batch-tag format

[david]

From: David Bremner <bremner@debian.org>

Now we can actually round trip these crazy tags and and message ids.
hex-xcode is no longer needed as it's built in.
---
 test/dump-restore |   13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/test/dump-restore b/test/dump-restore
index a2204fb..e08b656 100755
--- a/test/dump-restore
+++ b/test/dump-restore
@@ -85,13 +85,14 @@ test_begin_subtest "dump --output=outfile -- from:cworth"
 notmuch dump --output=dump-outfile-dash-inbox.actual -- from:cworth
 test_expect_equal_file dump-cworth.expected dump-outfile-dash-inbox.actual
 
+
 test_expect_success 'roundtripping random message-ids and tags' \
-    'test_subtest_known_broken &&
-     ${TEST_DIRECTORY}/random-corpus --num-messages=10 --config-path=${NOTMUCH_CONFIG} &&
-     notmuch dump | ${TEST_DIRECTORY}/hex-xcode --direction=encode > EXPECTED.$test_count &&
+    '${TEST_DIRECTORY}/random-corpus --num-messages=100 --config-path=${NOTMUCH_CONFIG} &&
+     notmuch dump --format=batch-tag | sort > EXPECTED.$test_count &&
      notmuch tag -random-corpus tag:random-corpus &&
-     ${TEST_DIRECTORY}/hex-xcode --direction=decode < EXPECTED.$test_count | notmuch restore 2>/dev/null &&
-     notmuch dump | ${TEST_DIRECTORY}/hex-xcode --direction=encode > OUTPUT.$test_count &&
+     notmuch restore --format=batch-tag --input=EXPECTED.$test_count &&
+     notmuch dump --format=batch-tag | sort > OUTPUT.$test_count &&
      test_cmp EXPECTED.$test_count OUTPUT.$test_count'
-
 test_done
+
+# Note the database is "poisoned" for sup format at this point.
-- 
1.7.10.4

[Patch v2 15/17] test: second set of dump/restore --format=batch-tag tests

[david]

From: David Bremner <bremner@debian.org>

These one need the completed functionality in notmuch-restore. Fairly
exotic tags are tested, but no weird message id's.

We test each possible input to autodetection, both explicit (with
--format=auto) and implicit (without --format).
---
 test/dump-restore |   92 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 92 insertions(+)

diff --git a/test/dump-restore b/test/dump-restore
index e08b656..23af2b7 100755
--- a/test/dump-restore
+++ b/test/dump-restore
@@ -85,6 +85,98 @@ test_begin_subtest "dump --output=outfile -- from:cworth"
 notmuch dump --output=dump-outfile-dash-inbox.actual -- from:cworth
 test_expect_equal_file dump-cworth.expected dump-outfile-dash-inbox.actual
 
+test_begin_subtest "Check for a safe set of message-ids"
+notmuch search --output=messages from:cworth > EXPECTED.$test_count
+notmuch search --output=messages from:cworth |\
+	$TEST_DIRECTORY/hex-xcode --direction=encode > OUTPUT.$test_count
+test_expect_equal_file OUTPUT.$test_count EXPECTED.$test_count
+
+test_begin_subtest "format=batch-tag, # round-trip"
+notmuch dump --format=sup | sort > EXPECTED.$test_count
+notmuch dump --format=batch-tag | notmuch restore --format=batch-tag
+notmuch dump --format=sup | sort > OUTPUT.$test_count
+test_expect_equal_file EXPECTED.$test_count OUTPUT.$test_count
+
+test_begin_subtest "format=batch-tag, # blank lines and comments"
+notmuch dump --format=batch-tag| sort > EXPECTED.$test_count
+notmuch restore <<EOF
+# this line is a comment; the next has only white space
+ 	 
+
+# the previous line is empty
+EOF
+notmuch dump --format=batch-tag | sort > OUTPUT.$test_count
+test_expect_equal_file EXPECTED.$test_count OUTPUT.$test_count
+
+test_begin_subtest "format=batch-tag, # reverse-round-trip empty tag"
+cat <<EOF >EXPECTED.$test_count
++ -- id:20091117232137.GA7669@griffis1.net
+EOF
+notmuch restore --format=batch-tag < EXPECTED.$test_count
+notmuch dump --format=batch-tag id:20091117232137.GA7669@griffis1.net > OUTPUT.$test_count
+test_expect_equal_file EXPECTED.$test_count OUTPUT.$test_count
+
+tag1='comic_swear=$&^%$^%\\//-+$^%$'
+enc1=$($TEST_DIRECTORY/hex-xcode --direction=encode "$tag1")
+
+tag2=$(printf 'this\n tag\t has\n spaces')
+enc2=$($TEST_DIRECTORY/hex-xcode --direction=encode "$tag2")
+
+enc3='%c3%91%c3%a5%c3%b0%c3%a3%c3%a5%c3%a9-%c3%8f%c3%8a'
+tag3=$($TEST_DIRECTORY/hex-xcode --direction=decode $enc3)
+
+notmuch dump --format=batch-tag > BACKUP
+
+notmuch tag +"$tag1" +"$tag2" +"$tag3" -inbox -unread "*"
+
+test_begin_subtest 'format=batch-tag, round trip with strange tags'
+    notmuch dump --format=batch-tag > EXPECTED.$test_count
+    notmuch dump --format=batch-tag | notmuch restore --format=batch-tag
+    notmuch dump --format=batch-tag > OUTPUT.$test_count
+test_expect_equal_file EXPECTED.$test_count OUTPUT.$test_count
+
+test_begin_subtest 'format=batch-tag, checking encoded output'
+    cp /dev/null EXPECTED.$test_count
+    notmuch dump --format=batch-tag -- from:cworth |\
+	 awk "{ print \"+$enc1 +$enc2 +$enc3 -- \" \$5 }" > EXPECTED.$test_count
+
+    notmuch dump --format=batch-tag -- from:cworth  > OUTPUT.$test_count
+test_expect_equal_file EXPECTED.$test_count OUTPUT.$test_count
+
+test_begin_subtest 'restoring sane tags'
+    notmuch restore --format=batch-tag < BACKUP
+    notmuch dump --format=batch-tag > OUTPUT.$test_count
+test_expect_equal_file BACKUP OUTPUT.$test_count
+
+test_begin_subtest 'format=batch-tag, restore=auto'
+    notmuch dump --format=batch-tag > EXPECTED.$test_count
+    notmuch tag -inbox -unread "*"
+    notmuch restore --format=auto < EXPECTED.$test_count
+    notmuch dump --format=batch-tag > OUTPUT.$test_count
+test_expect_equal_file EXPECTED.$test_count OUTPUT.$test_count
+
+test_begin_subtest 'format=sup, restore=auto'
+    notmuch dump --format=sup > EXPECTED.$test_count
+    notmuch tag -inbox -unread "*"
+    notmuch restore --format=auto < EXPECTED.$test_count
+    notmuch dump --format=sup > OUTPUT.$test_count
+test_expect_equal_file EXPECTED.$test_count OUTPUT.$test_count
+
+test_begin_subtest 'format=batch-tag, restore=default'
+    notmuch dump --format=batch-tag > EXPECTED.$test_count
+    notmuch tag -inbox -unread "*"
+    notmuch restore < EXPECTED.$test_count
+    notmuch dump --format=batch-tag > OUTPUT.$test_count
+test_expect_equal_file EXPECTED.$test_count OUTPUT.$test_count
+
+test_begin_subtest 'format=sup, restore=default'
+    notmuch dump --format=sup > EXPECTED.$test_count
+    notmuch tag -inbox -unread "*"
+    notmuch restore < EXPECTED.$test_count
+    notmuch dump --format=sup > OUTPUT.$test_count
+test_expect_equal_file EXPECTED.$test_count OUTPUT.$test_count
+
+notmuch dump --format=batch-tag --output=save.tags
 
 test_expect_success 'roundtripping random message-ids and tags' \
     '${TEST_DIRECTORY}/random-corpus --num-messages=100 --config-path=${NOTMUCH_CONFIG} &&
-- 
1.7.10.4

[Patch v2 16/17] notmuch-{dump, restore}.1: document new format options

[david]

From: David Bremner <bremner@debian.org>

More or less arbitrarily, notmuch-dump.1 gets the more detailed
description of the format.
---
 man/man1/notmuch-dump.1    |   58 +++++++++++++++++++++++++++++++++++++++++++
 man/man1/notmuch-restore.1 |   59 +++++++++++++++++++++++++++++++++++++++-----
 2 files changed, 111 insertions(+), 6 deletions(-)

diff --git a/man/man1/notmuch-dump.1 b/man/man1/notmuch-dump.1
index 230deec..9f59905 100644
--- a/man/man1/notmuch-dump.1
+++ b/man/man1/notmuch-dump.1
@@ -5,6 +5,7 @@ notmuch-dump \- creates a plain-text dump of the tags of each message
 .SH SYNOPSIS
 
 .B "notmuch dump"
+.RB  [ "\-\-format=(sup|batch-tag)"  "] [--]"
 .RI "[ --output=<" filename "> ] [--]"
 .RI "[ <" search-term ">...]"
 
@@ -19,6 +20,63 @@ recreated from the messages themselves.  The output of notmuch dump is
 therefore the only critical thing to backup (and much more friendly to
 incremental backup than the native database files.)
 
+.TP 4
+.B \-\-format=(sup|batch-tag)
+
+Notmuch restore supports two plain text dump formats, both with one message-id
+per line, followed by a list of tags.
+
+.RS 4
+.TP 4
+.B sup
+
+The
+.B sup
+dump file format is specifically chosen to be
+compatible with the format of files produced by sup-dump.
+So if you've previously been using sup for mail, then the
+.B "notmuch restore"
+command provides you a way to import all of your tags (or labels as
+sup calls them).
+Each line has the following form
+
+.RS 4
+.RI < message-id >
+.B (
+.RI < tag "> ..."
+.B )
+
+with zero or more tags are separated by spaces. Note that (malformed)
+message-ids may contain arbitrary non-null characters. Note also
+that tags with spaces will not be correctly restored with this format.
+
+.RE
+
+.RE
+.RS 4
+.TP 4
+.B batch-tag
+
+The
+.B batch-tag
+dump format is intended to more robust against malformed message-ids
+and tags containing whitespace or non-\fBascii\fR(7) characters.
+Each line has the form
+
+.RS 4
+.RI "+<" "encoded-tag" "> " "" "+<" "encoded-tag" "> ... -- " "" " <" encoded-message-id >
+
+where encoded means that every byte not matching the regex
+.B [A-Za-z0-9+-_@=.:,]
+is replace by
+.B %nn
+where nn is the two digit hex encoding.
+The astute reader will notice this is a special case of the batch input
+format for \fBnotmuch-tag\fR(1).
+
+.RE
+
+
 With no search terms, a dump of all messages in the database will be
 generated.  A "--" argument instructs notmuch that the
 remaining arguments are search terms.
diff --git a/man/man1/notmuch-restore.1 b/man/man1/notmuch-restore.1
index 2fa8733..3860829 100644
--- a/man/man1/notmuch-restore.1
+++ b/man/man1/notmuch-restore.1
@@ -6,6 +6,7 @@ notmuch-restore \- restores the tags from the given file (see notmuch dump)
 
 .B "notmuch restore"
 .RB [ "--accumulate" ]
+.RB [ "--format=(auto|batch-tag|sup)" ]
 .RI "[ --input=<" filename "> ]"
 
 .SH DESCRIPTION
@@ -15,19 +16,51 @@ Restores the tags from the given file (see
 
 The input is read from the given filename, if any, or from stdin.
 
-Note: The dump file format is specifically chosen to be
+
+Supported options for
+.B restore
+include
+.RS 4
+.TP 4
+.B \-\-accumulate
+
+The union of the existing and new tags is applied, instead of
+replacing each message's tags as they are read in from the dump file.
+
+.RE
+.RS 4
+.TP 4
+.B \-\-format=(sup|batch-tag|auto)
+
+Notmuch restore supports two plain text dump formats, with one message-id
+per line, and a list of tags.
+For details of the actual formats, see \fBnotmuch-dump\fR(1).
+
+.RS 4
+.TP 4
+.B sup
+
+The
+.B sup
+dump file format is specifically chosen to be
 compatible with the format of files produced by sup-dump.
 So if you've previously been using sup for mail, then the
 .B "notmuch restore"
 command provides you a way to import all of your tags (or labels as
 sup calls them).
 
-The --accumulate switch causes the union of the existing and new tags to be
-applied, instead of replacing each message's tags as they are read in from the
-dump file.
+.RE
+.RS 4
+.TP 4
+.B batch-tag
 
-See \fBnotmuch-search-terms\fR(7)
-for details of the supported syntax for <search-terms>.
+The
+.B batch-tag
+dump format is intended to more robust against malformed message-ids
+and tags containing whitespace or non-\fBascii\fR(7) characters.  This
+format hex-escapes all characters those outside of a small character
+set, intended to be suitable for e.g. pathnames in most UNIX-like
+systems.
 
 .B "notmuch restore"
 updates the maildir flags according to tag changes if the
@@ -36,6 +69,20 @@ configuration option is enabled. See \fBnotmuch-config\fR(1) for
 details.
 
 .RE
+
+.RS 4
+.TP 4
+.B auto
+
+This option (the default) tries to guess the format from the
+input. For correctly formed input in either supported format, this
+heuristic, based the fact that batch-tag format contains no parentheses,
+should be accurate.
+
+.RE
+
+.RE
+
 .SH SEE ALSO
 
 \fBnotmuch\fR(1), \fBnotmuch-config\fR(1), \fBnotmuch-count\fR(1),
-- 
1.7.10.4

[Patch v2 17/17] tag-util: optimization of tag application

[david]

From: David Bremner <bremner@debian.org>

The idea is not to bother with restore operations if they don't change
the set of tags. This is actually a relatively common case.

In order to avoid fancy datastructures, this method is quadratic in
the number of tags; at least on my mail database this doesn't seem to
be a big problem.
---
 notmuch-tag.c |    2 +-
 tag-util.c    |   59 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 60 insertions(+), 1 deletion(-)

diff --git a/notmuch-tag.c b/notmuch-tag.c
index 8a8af0b..e4fca67 100644
--- a/notmuch-tag.c
+++ b/notmuch-tag.c
@@ -140,7 +140,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);
-	tag_op_list_apply (message, tag_ops, flags);
+	tag_op_list_apply (message, tag_ops, flags | TAG_FLAG_PRE_OPTIMIZED);
 	notmuch_message_destroy (message);
     }
 
diff --git a/tag-util.c b/tag-util.c
index 287cc67..2bb8355 100644
--- a/tag-util.c
+++ b/tag-util.c
@@ -111,6 +111,62 @@ message_error (notmuch_message_t *message,
     fprintf (stderr, "Status: %s\n", notmuch_status_to_string (status));
 }
 
+static int
+makes_changes (notmuch_message_t *message,
+	       tag_op_list_t *list,
+	       tag_op_flag_t flags)
+{
+
+    int i;
+
+    notmuch_tags_t *tags;
+    notmuch_bool_t changes = FALSE;
+
+    /* First, do we delete an existing tag? */
+    changes = FALSE;
+    for (tags = notmuch_message_get_tags (message);
+	 ! changes && notmuch_tags_valid (tags);
+	 notmuch_tags_move_to_next (tags)) {
+	const char *cur_tag = notmuch_tags_get (tags);
+	int last_op =  (flags & TAG_FLAG_REMOVE_ALL) ? -1 : 0;
+
+	for (i = 0; i < list->count; i++) {
+	    if (strcmp (cur_tag, list->ops[i].tag) == 0) {
+		last_op = list->ops[i].remove ? -1 : 1;
+	    }
+	}
+
+	changes = (last_op == -1);
+    }
+    notmuch_tags_destroy (tags);
+
+    if (changes)
+	return TRUE;
+
+    /* Now check for adding new tags */
+    for (i = 0; i < list->count; i++) {
+	notmuch_bool_t exists = FALSE;
+
+	for (tags = notmuch_message_get_tags (message);
+	     notmuch_tags_valid (tags);
+	     notmuch_tags_move_to_next (tags)) {
+	    const char *cur_tag = notmuch_tags_get (tags);
+	    if (strcmp (cur_tag, list->ops[i].tag) == 0) {
+		exists = TRUE;
+		break;
+	    }
+	}
+	notmuch_tags_destroy (tags);
+
+	/* the following test is conservative, it's ok to think we
+	 * make changes when we don't */
+	if ( ! exists && ! list->ops[i].remove )
+	    return TRUE;
+    }
+    return FALSE;
+
+}
+
 notmuch_status_t
 tag_op_list_apply (notmuch_message_t *message,
 		   tag_op_list_t *list,
@@ -121,6 +177,9 @@ tag_op_list_apply (notmuch_message_t *message,
     notmuch_status_t status = 0;
     tag_operation_t *tag_ops = list->ops;
 
+    if (! (flags & TAG_FLAG_PRE_OPTIMIZED) && ! makes_changes (message, list, flags))
+	return NOTMUCH_STATUS_SUCCESS;
+
     status = notmuch_message_freeze (message);
     if (status) {
 	message_error (message, status, "freezing message");
-- 
1.7.10.4

Re: [Patch v2 14/17] test: update dump-restore roundtripping test for batch-tag format

[Tomi Ollila]

On Sat, Nov 24 2012, david@tethera.net wrote:

> From: David Bremner <bremner@debian.org>
>
> Now we can actually round trip these crazy tags and and message ids.
> hex-xcode is no longer needed as it's built in.
> ---
>  test/dump-restore |   13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/test/dump-restore b/test/dump-restore
> index a2204fb..e08b656 100755
> --- a/test/dump-restore
> +++ b/test/dump-restore
> @@ -85,13 +85,14 @@ test_begin_subtest "dump --output=outfile -- from:cworth"
>  notmuch dump --output=dump-outfile-dash-inbox.actual -- from:cworth
>  test_expect_equal_file dump-cworth.expected dump-outfile-dash-inbox.actual
>  
> +
>  test_expect_success 'roundtripping random message-ids and tags' \
> -    'test_subtest_known_broken &&
> -     ${TEST_DIRECTORY}/random-corpus --num-messages=10 --config-path=${NOTMUCH_CONFIG} &&
> -     notmuch dump | ${TEST_DIRECTORY}/hex-xcode --direction=encode > EXPECTED.$test_count &&
> +    '${TEST_DIRECTORY}/random-corpus --num-messages=100 --config-path=${NOTMUCH_CONFIG} &&
> +     notmuch dump --format=batch-tag | sort > EXPECTED.$test_count &&
>       notmuch tag -random-corpus tag:random-corpus &&
> -     ${TEST_DIRECTORY}/hex-xcode --direction=decode < EXPECTED.$test_count | notmuch restore 2>/dev/null &&
> -     notmuch dump | ${TEST_DIRECTORY}/hex-xcode --direction=encode > OUTPUT.$test_count &&
> +     notmuch restore --format=batch-tag --input=EXPECTED.$test_count &&
> +     notmuch dump --format=batch-tag | sort > OUTPUT.$test_count &&
>       test_cmp EXPECTED.$test_count OUTPUT.$test_count'
> -
>  test_done

In most cases there is empty line before `test_done` -- also other 
unrelated whitespace changes disturb review (especially when one is
tired ;)

> +
> +# Note the database is "poisoned" for sup format at this point.
> -- 
> 1.7.10.4

Tomi

Re: [Patch v2 09/17] tag-util.[ch]: New files for common tagging routines

[Tomi Ollila]

On Sat, Nov 24 2012, david@tethera.net wrote:

> From: David Bremner <bremner@debian.org>
>
> These are meant to be shared between notmuch-tag and notmuch-restore.
>
> The bulk of the routines implement a "tag operation list" abstract
> data type act as a structured representation of a set of tag
> operations (typically coming from a single tag command or line of
> input).
> ---

dumping all except beginning of tag-util.h...

>  tag-util.h     |  120 ++++++++++++++++++++++++++
> diff --git a/tag-util.h b/tag-util.h
> new file mode 100644
> index 0000000..508806f
> --- /dev/null
> +++ b/tag-util.h
> @@ -0,0 +1,120 @@
> +#ifndef _TAG_UTIL_H
> +#define _TAG_UTIL_H
> +
> +#include "notmuch-client.h"
> +
> +typedef struct _tag_operation_t tag_operation_t;
> +typedef struct _tag_op_list_t tag_op_list_t;
> +
> +#define TAG_OP_LIST_INITIAL_SIZE 10
> +
> +/* Use powers of 2 */
> +typedef enum  { TAG_FLAG_NONE = 0,
> +		/* Operations are synced to maildir, if possible */
> +
> +		TAG_FLAG_MAILDIR_SYNC = 1,
> +
> +		/* Remove all tags from message before applying
> +		 * list */
> +
> +		TAG_FLAG_REMOVE_ALL = 2,
> +
> +		/* Don't try to avoid database operations.  Useful
> +		 * when we know that message passed needs these
> +		 *  operations. */
> +
> +		TAG_FLAG_PRE_OPTIMIZED = 4,
> +
> +		/* Accept strange tags that might be user error;
> +		   intended for use by notmuch-restore.
> +		*/
> +
> +		TAG_FLAG_BE_GENEROUS = 8} tag_op_flag_t;
> +

Maybe something like the following formatted and consistency-tuned version:

typedef enum { 
   TAG_FLAG_NONE = 0,

   /* Operations are synced to maildir, if possible.
    */
   TAG_FLAG_MAILDIR_SYNC = (1 << 0),

   /* Remove all tags from message before applying list.
    */
   TAG_FLAG_REMOVE_ALL = (1 << 1),

   /* Don't try to avoid database operations. Useful when we
    * know that message passed needs these operations.
    */
   TAG_FLAG_PRE_OPTIMIZED = (1 << 2),

   /* Accept strange tags that might be user error;
    * intended for use by notmuch-restore.
    */
   TAG_FLAG_BE_GENEROUS = (1 << 3)

} tag_op_flag_t;

Tomi

[PATCH] test: update dump-restore roundtripping test for batch-tag format

[david]

From: David Bremner <bremner@debian.org>

Now we can actually round trip these crazy tags and and message ids.
hex-xcode is no longer needed as it's built in.
---


I played with this a bit to try to make the diff nicer; I'm not sure
if I really improved much, but putting the sort into the original test
was a bug fix anyway. This is really for review; I don't think it will
apply without also resending 6/14.

 test/dump-restore |   14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/test/dump-restore b/test/dump-restore
index 28474f5..e9ba79d 100755
--- a/test/dump-restore
+++ b/test/dump-restore
@@ -86,18 +86,16 @@ notmuch dump --output=dump-outfile-dash-inbox.actual -- from:cworth
 test_expect_equal_file dump-cworth.expected dump-outfile-dash-inbox.actual
 
 test_expect_success 'roundtripping random message-ids and tags' \
-    'test_subtest_known_broken &&
-     ${TEST_DIRECTORY}/random-corpus --num-messages=10 \
+     '${TEST_DIRECTORY}/random-corpus --num-messages=100 \
        --config-path=${NOTMUCH_CONFIG} &&
-     notmuch dump |
-       ${TEST_DIRECTORY}/hex-xcode --direction=encode |
+     notmuch dump --format=batch-tag |
        sort > EXPECTED.$test_count &&
      notmuch tag -random-corpus tag:random-corpus &&
-     ${TEST_DIRECTORY}/hex-xcode --direction=decode < EXPECTED.$test_count |
-       notmuch restore 2>/dev/null &&
-     notmuch dump |
-       ${TEST_DIRECTORY}/hex-xcode --direction=encode |
+     notmuch restore --format=batch-tag < EXPECTED.$test_count &&
+     notmuch dump --format=batch-tag |
        sort > OUTPUT.$test_count &&
      test_cmp EXPECTED.$test_count OUTPUT.$test_count'
 
 test_done
+
+# Note the database is "poisoned" for sup format at this point.
-- 
1.7.10.4

Re: [Patch v2 09/17] tag-util.[ch]: New files for common tagging routines

[David Bremner]

>
> Maybe something like the following formatted and consistency-tuned version:
>
> typedef enum { 
>    TAG_FLAG_NONE = 0,
>
>    /* Operations are synced to maildir, if possible.
>     */
>    TAG_FLAG_MAILDIR_SYNC = (1 << 0),
>
>    /* Remove all tags from message before applying list.
>     */
>    TAG_FLAG_REMOVE_ALL = (1 << 1),
>
>    /* Don't try to avoid database operations. Useful when we
>     * know that message passed needs these operations.
>     */
>    TAG_FLAG_PRE_OPTIMIZED = (1 << 2),
>
>    /* Accept strange tags that might be user error;
>     * intended for use by notmuch-restore.
>     */
>    TAG_FLAG_BE_GENEROUS = (1 << 3)
>
> } tag_op_flag_t;
>

Applied.  We may have to fight uncrustify on this, but we both know who
likes to play with uncrustify config ;).

d

Re: [Patch v2 09/17] tag-util.[ch]: New files for common tagging routines

[Mark Walters]

This looks good. A couple of typos and a small queries (and I
agree with Tomi but I think you have already included that).

On Sat, 24 Nov 2012, david@tethera.net wrote:
> From: David Bremner <bremner@debian.org>
>
> These are meant to be shared between notmuch-tag and notmuch-restore.
>
> The bulk of the routines implement a "tag operation list" abstract
> data type act as a structured representation of a set of tag
> operations (typically coming from a single tag command or line of
> input).
> ---
>  Makefile.local |    1 +
>  tag-util.c     |  264 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  tag-util.h     |  120 ++++++++++++++++++++++++++
>  3 files changed, 385 insertions(+)
>  create mode 100644 tag-util.c
>  create mode 100644 tag-util.h
>
> diff --git a/Makefile.local b/Makefile.local
> index 2b91946..854867d 100644
> --- a/Makefile.local
> +++ b/Makefile.local
> @@ -274,6 +274,7 @@ notmuch_client_srcs =		\
>  	query-string.c		\
>  	mime-node.c		\
>  	crypto.c		\
> +	tag-util.c
>  
>  notmuch_client_modules = $(notmuch_client_srcs:.c=.o)
>  
> diff --git a/tag-util.c b/tag-util.c
> new file mode 100644
> index 0000000..287cc67
> --- /dev/null
> +++ b/tag-util.c
> @@ -0,0 +1,264 @@
> +#include "string-util.h"
> +#include "tag-util.h"
> +#include "hex-escape.h"
> +
> +struct _tag_operation_t {
> +    const char *tag;
> +    notmuch_bool_t remove;
> +};
> +
> +struct _tag_op_list_t {
> +    tag_operation_t *ops;
> +    int count;
> +    int size;
> +};
> +
> +int
> +parse_tag_line (void *ctx, char *line,
> +		tag_op_flag_t flags,
> +		char **query_string,
> +		tag_op_list_t *tag_ops)
> +{
> +    char *tok = line;
> +    size_t tok_len = 0;
> +
> +    chomp_newline (line);
> +
> +    /* remove leading space */
> +    while (*tok == ' ' || *tok == '\t')
> +	tok++;
> +
> +    /* Skip empty and comment lines. */
> +    if (*tok == '\0' || *tok == '#')
> +	    return 1;
> +
> +    tag_op_list_reset (tag_ops);
> +
> +    /* Parse tags. */
> +    while ((tok = strtok_len (tok + tok_len, " ", &tok_len)) != NULL) {
> +	notmuch_bool_t remove;
> +	char *tag;
> +
> +	/* Optional explicit end of tags marker. */
> +	if (strncmp (tok, "--", tok_len) == 0) {
> +	    tok = strtok_len (tok + tok_len, " ", &tok_len);
> +	    break;
> +	}
> +
> +	/* Implicit end of tags. */
> +	if (*tok != '-' && *tok != '+')
> +	    break;
> +
> +	/* If tag is terminated by NUL, there's no query string. */
> +	if (*(tok + tok_len) == '\0') {
> +	    tok = NULL;
> +	    break;
> +	}
> +
> +	/* Terminate, and start next token after terminator. */
> +	*(tok + tok_len++) = '\0';
> +
> +	remove = (*tok == '-');
> +	tag = tok + 1;
> +
> +	/* Maybe refuse empty tags. */
> +	if (!(flags & TAG_FLAG_BE_GENEROUS) && *tag == '\0') {
> +	    tok = NULL;
> +	    break;
> +	}
> +
> +	/* Decode tag. */
> +	if (hex_decode_inplace (tag) != HEX_SUCCESS) {
> +	    tok = NULL;
> +	    break;
> +	}
> +
> +	if (tag_op_list_append (ctx, tag_ops, tag, remove))
> +	    return -1;
> +    }
> +
> +    if (tok == NULL || tag_ops->count == 0) {
> +	/* FIXME: line has been modified! */
> +	fprintf (stderr, "Warning: Ignoring invalid input line: %s\n",
> +		 line);
> +	return 1;
> +    }
> +
> +    /* tok now points to the query string */
> +    if (hex_decode_inplace (tok) != HEX_SUCCESS) {
> +	/* FIXME: line has been modified! */
> +	fprintf (stderr, "Warning: Ignoring invalid input line: %s\n",
> +		 line);
> +	return 1;
> +    }
> +
> +    *query_string = tok;
> +
> +    return 0;
> +}
> +
> +static inline void
> +message_error (notmuch_message_t *message,
> +	       notmuch_status_t status,
> +	       const char *format, ...)
> +{
> +    va_list va_args;
> +
> +    va_start (va_args, format);
> +
> +    vfprintf (stderr, format, va_args);
> +    fprintf (stderr, "Message-ID: %s\n", notmuch_message_get_message_id (message));
> +    fprintf (stderr, "Status: %s\n", notmuch_status_to_string (status));
> +}
> +
> +notmuch_status_t
> +tag_op_list_apply (notmuch_message_t *message,
> +		   tag_op_list_t *list,
> +		   tag_op_flag_t flags)
> +{
> +    int i;
> +
> +    notmuch_status_t status = 0;
> +    tag_operation_t *tag_ops = list->ops;
> +
> +    status = notmuch_message_freeze (message);
> +    if (status) {
> +	message_error (message, status, "freezing message");
> +	return status;
> +    }
> +
> +    if (flags & TAG_FLAG_REMOVE_ALL) {
> +	status = notmuch_message_remove_all_tags (message);
> +	if (status) {
> +	    message_error (message, status, "removing all tags" );
> +	    return status;
> +	}
> +    }
> +
> +    for (i = 0; i < list->count; i++) {
> +	if (tag_ops[i].remove) {
> +	    status = notmuch_message_remove_tag (message, tag_ops[i].tag);
> +	    if (status) {
> +		message_error (message, status, "removing tag %s", tag_ops[i].tag);
> +		return status;
> +	    }
> +	} else {
> +	    status = notmuch_message_add_tag (message, tag_ops[i].tag);
> +	    if (status) {
> +		message_error (message, status, "adding tag %s", tag_ops[i].tag);
> +		return status;
> +	    }
> +
> +	}
> +    }
> +
> +    status = notmuch_message_thaw (message);

I don't know how freeze/thaw work but does it matter that you don't thaw
when there is an error?

> +    if (status) {
> +	message_error (message, status, "thawing message");
> +	return status;
> +    }
> +
> +
> +    if (flags & TAG_FLAG_MAILDIR_SYNC) {
> +	status = notmuch_message_tags_to_maildir_flags (message);
> +	if (status) {
> +	    message_error (message, status, "synching tags to maildir");
> +	    return status;
> +	}
> +    }
> +
> +    return NOTMUCH_STATUS_SUCCESS;
> +
> +}
> +
> +
> +/* Array of tagging operations (add or remove), terminated with an
> + * empty element. Size will be increased as necessary. */
> +
> +tag_op_list_t *
> +tag_op_list_create (void *ctx)
> +{
> +    tag_op_list_t *list;
> +
> +    list = talloc (ctx, tag_op_list_t);
> +    if (list == NULL)
> +	return NULL;
> +
> +    list->size = TAG_OP_LIST_INITIAL_SIZE;
> +    list->count = 0;
> +
> +    list->ops = talloc_array (ctx, tag_operation_t, list->size);
> +    if (list->ops == NULL)
> +	return NULL;
> +
> +    return list;
> +}
> +
> +
> +int
> +tag_op_list_append (void *ctx,
> +		    tag_op_list_t *list,
> +		    const char *tag,
> +		    notmuch_bool_t remove)
> +{
> +    /* Make room if current array is full.  This should be a fairly
> +     * rare case, considering the initial array size.
> +     */
> +
> +    if (list->count == list->size) {
> +	list->size *= 2;
> +	list->ops = talloc_realloc (ctx, list->ops, tag_operation_t,
> +				    list->size);
> +	if (list->ops == NULL) {
> +	    fprintf (stderr, "Out of memory.\n");
> +	    return 1;
> +	}
> +    }
> +
> +    /* add the new operation */
> +
> +    list->ops[list->count].tag = tag;
> +    list->ops[list->count].remove = remove;
> +    list->count++;
> +    return 0;
> +}
> +
> +/*
> + *   Is the i'th tag operation a remove?
> + */
> +
> +notmuch_bool_t
> +tag_op_list_isremove (const tag_op_list_t *list, size_t i)
> +{
> +    return list->ops[i].remove;
> +}
> +
> +/*
> + * Reset a list to contain no operations
> + */
> +
> +void
> +tag_op_list_reset (tag_op_list_t *list)
> +{
> +    list->count = 0;
> +}
> +
> +/*
> + * Return the number of operations in a list
> + */
> +
> +size_t
> +tag_op_list_size (const tag_op_list_t *list)
> +{
> +    return list->count;
> +}
> +
> +/*
> + *   return the i'th tag in the list
> + */
> +
> +const char *
> +tag_op_list_tag (const tag_op_list_t *list, size_t i)
> +{
> +    return list->ops[i].tag;
> +}
> diff --git a/tag-util.h b/tag-util.h
> new file mode 100644
> index 0000000..508806f
> --- /dev/null
> +++ b/tag-util.h
> @@ -0,0 +1,120 @@
> +#ifndef _TAG_UTIL_H
> +#define _TAG_UTIL_H
> +
> +#include "notmuch-client.h"
> +
> +typedef struct _tag_operation_t tag_operation_t;
> +typedef struct _tag_op_list_t tag_op_list_t;
> +
> +#define TAG_OP_LIST_INITIAL_SIZE 10
> +
> +/* Use powers of 2 */
> +typedef enum  { TAG_FLAG_NONE = 0,
> +		/* Operations are synced to maildir, if possible */
> +
> +		TAG_FLAG_MAILDIR_SYNC = 1,
> +
> +		/* Remove all tags from message before applying
> +		 * list */
> +
> +		TAG_FLAG_REMOVE_ALL = 2,
> +
> +		/* Don't try to avoid database operations.  Useful
> +		 * when we know that message passed needs these
> +		 *  operations. */
> +
> +		TAG_FLAG_PRE_OPTIMIZED = 4,
> +
> +		/* Accept strange tags that might be user error;
> +		   intended for use by notmuch-restore.
> +		*/
> +
> +		TAG_FLAG_BE_GENEROUS = 8} tag_op_flag_t;
> +
> +/* Parse a string of the following format:
> + *
> + * +<tag>|-<tag> [...] [--] <search-terms>
> + *
> + * Each line is interpreted similarly to "notmuch tag" command line
> + * arguments. The delimiter is one or more spaces ' '. Any characters
> + * in <tag> and <search-terms> MAY be hex encoded with %NN where NN is
> + * the hexadecimal value of the character. Any ' ' and '%' characters
> + * in <tag> and <search-terms> MUST be hex encoded (using %20 and %25,
> + * respectively). Any characters that are not part of <tag> or
> + * <search-terms> MUST NOT be hex encoded.
> + *
> + * Leading and trailing space ' ' is ignored. Empty lines and lines
> + * beginning with '#' are ignored.
> + *
> + * Returns: 0 for OK, 1 for skipped line, -1 for fatal(ish) error.
> + *
> + * Output Parameters:
> + *	ops	contains a list of tag operations
> + *	query_str the search terms.
> + */
> +int
> +parse_tag_line (void *ctx, char *line,
> +		tag_op_flag_t flags,
> +		char **query_str, tag_op_list_t *ops);
> +
> +/*
> + * Create an empty list of tag operations
> + *
> + * ctx is passed to talloc
> + */
> +
> +tag_op_list_t
> +*tag_op_list_create (void *ctx);
> +
> +/*
> + * Add a tag operation (delete iff remote == TRUE) to a list.
> + * The list is expanded as necessary.
> + */

Typo s/remote/remove/

> +int
> +tag_op_list_append (void *ctx,
> +		    tag_op_list_t *list,
> +		    const char *tag,
> +		    notmuch_bool_t remove);
> +
> +/*
> + * Apply a list of tag operations, in order to a message.

I found the comment awkward to parse: as "in order to <do something>"

Best wishes

Mark

> + *
> + * Flags can be bitwise ORed; see enum above for possibilies.
> + */
> +
> +notmuch_status_t
> +tag_op_list_apply (notmuch_message_t *message,
> +		   tag_op_list_t *tag_ops,
> +		   tag_op_flag_t flags);
> +
> +/*
> + * Return the number of operations in a list
> + */
> +
> +size_t
> +tag_op_list_size (const tag_op_list_t *list);
> +
> +/*
> + * Reset a list to contain no operations
> + */
> +
> +void
> +tag_op_list_reset (tag_op_list_t *list);
> +
> +
> + /*
> +  *   return the i'th tag in the list
> +  */
> +
> +const char *
> +tag_op_list_tag (const tag_op_list_t *list, size_t i);
> +
> +/*
> + *   Is the i'th tag operation a remove?
> + */
> +
> +notmuch_bool_t
> +tag_op_list_isremove (const tag_op_list_t *list, size_t i);
> +
> +#endif
> -- 
> 1.7.10.4
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [Patch v2 12/17] man: document notmuch tag --batch, --input options

[Mark Walters]

On Sat, 24 Nov 2012, david@tethera.net wrote:
> From: Jani Nikula <jani@nikula.org>
>
> ---
>  man/man1/notmuch-tag.1 |   52 +++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 51 insertions(+), 1 deletion(-)
>
> diff --git a/man/man1/notmuch-tag.1 b/man/man1/notmuch-tag.1
> index 0f86582..751db7b 100644
> --- a/man/man1/notmuch-tag.1
> +++ b/man/man1/notmuch-tag.1
> @@ -4,7 +4,12 @@ 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-terms ">"
> +
> +.B notmuch tag
> +.RI "--batch"
> +.RI "[ --input=<" filename "> ]"
> +
>  
>  .SH DESCRIPTION
>  
> @@ -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 standard input. 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 "T +<" tag ">|\-<" tag "> [...] [\-\-] <" search-terms ">"

Is the T at the start of the line a remnant from a previous version?

Mark


> +
> +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-terms>
> +.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
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [Patch v2 11/17] test: add test for notmuch tag --batch option

[Mark Walters]

Hi

On Sat, 24 Nov 2012, david@tethera.net wrote:
> From: Jani Nikula <jani@nikula.org>
>
> Basic test of functionality, along with all combinations of options.
> ---
>  test/tagging |   46 ++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 46 insertions(+)
>
> diff --git a/test/tagging b/test/tagging
> index 980ff92..e5b8315 100755
> --- a/test/tagging
> +++ b/test/tagging
> @@ -46,6 +46,52 @@ 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)"
> +
> +> batch.in  <<EOF
> +# %20 is a space in tag
> +-:"%20 -tag1 +tag5 +tag6 -- One
> ++tag1 -tag1 -tag4 +tag4 -- Two
> +-tag6 One
> ++tag5 Two
> +EOF
> +
> +test_begin_subtest "--input"
> +notmuch tag --input=batch.in
> +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)"

Wouldn't a different set of tag changes be a better test as presumably
this test can pass if the command just does nothing? 

Mark

> +
> +test_begin_subtest "--batch --input"
> +notmuch tag --batch --input=batch.in
> +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)"
> +
> +test_begin_subtest "--batch, blank lines and comments"
> +notmuch dump | sort > EXPECTED.$test_count
> +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_count
> +test_expect_equal_file EXPECTED.$test_count OUTPUT.$test_count
> +
>  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
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [Patch v2 09/17] tag-util.[ch]: New files for common tagging routines

[David Bremner]

Mark Walters <markwalters1009@gmail.com> writes:


> I don't know how freeze/thaw work but does it matter that you don't thaw
> when there is an error?

My interpretation is that by not thawing before we destroy the message,
we are aborting the transaction, since the freeze/thaw information is
stored in the message structure. It is documented as forbidden to call
thaw _more_ times than freeze, but less is not explicitely mentioned.

>> +
>> +/*
>> + * Add a tag operation (delete iff remote == TRUE) to a list.
>> + * The list is expanded as necessary.
>> + */
>
> Typo s/remote/remove/

fixed in local git.
>> +
>> +/*
>> + * Apply a list of tag operations, in order to a message.
>
> I found the comment awkward to parse: as "in order to <do something>"

Re-punctu-worded to 

"Apply a list of tag operations, in order, to a given message"

[PATCH] test: add test for notmuch tag --batch option

[david]

From: Jani Nikula <jani@nikula.org>

Basic test of functionality, along with all combinations of options.

Modified extensively by David Bremner <david@tethera.net>

The choice of @ as a tag is intended to be non-alphanumeric, but still
not too much trouble in the shell and in the old sup dump/restore format.
---

Mark: good catch. 

I decided to save restore the tags rather than have
multiple input and output files here.

 test/tagging |   50 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 50 insertions(+)

diff --git a/test/tagging b/test/tagging
index 980ff92..75552e8 100755
--- a/test/tagging
+++ b/test/tagging
@@ -46,6 +46,56 @@ 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)"
+
+cat > batch.in  <<EOF
+# %20 is a space 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 > backup.tags.$test_count
+notmuch tag --input=batch.in
+notmuch search \* | notmuch_search_sanitize > OUTPUT.$test_count
+notmuch restore < backup.tags.$test_count
+test_expect_equal_file batch.expected OUTPUT.$test_count
+
+test_begin_subtest "--batch --input"
+notmuch dump > backup.tags.$test_count
+notmuch tag --batch --input=batch.in
+notmuch search \* | notmuch_search_sanitize > OUTPUT.$test_count
+notmuch restore < backup.tags.$test_count
+test_expect_equal_file batch.expected OUTPUT.$test_count
+
+test_begin_subtest "--batch, blank lines and comments"
+notmuch dump | sort > EXPECTED.$test_count
+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_count
+test_expect_equal_file EXPECTED.$test_count OUTPUT.$test_count
+
 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

Re: [PATCH] test: add test for notmuch tag --batch option

[Tomi Ollila]

On Fri, Nov 30 2012, david@tethera.net wrote:

> From: Jani Nikula <jani@nikula.org>
>
> Basic test of functionality, along with all combinations of options.
>
> Modified extensively by David Bremner <david@tethera.net>
>
> The choice of @ as a tag is intended to be non-alphanumeric, but still
> not too much trouble in the shell and in the old sup dump/restore format.
> ---
>
> Mark: good catch. 
>
> I decided to save restore the tags rather than have
> multiple input and output files here.
>
>  test/tagging |   50 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 50 insertions(+)
>
> diff --git a/test/tagging b/test/tagging
> index 980ff92..75552e8 100755
> --- a/test/tagging
> +++ b/test/tagging
> @@ -46,6 +46,56 @@ 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)"
> +
> +cat > batch.in  <<EOF
> +# %20 is a space 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 > backup.tags.$test_count
> +notmuch tag --input=batch.in
> +notmuch search \* | notmuch_search_sanitize > OUTPUT.$test_count
> +notmuch restore < backup.tags.$test_count
> +test_expect_equal_file batch.expected OUTPUT.$test_count

2 things for all of these test_expect_equal_file ...:s

Generally, argument order is OUTPUT EXPECTED (the diff is harder to grasp
-- but changing the diff order in test_expect_equal_file is pretty easy...
(& I have some patches in preparation for that...)

In case of failure. test_expect_equal_file does

  if diff -q "$file1" "$file2" >/dev/null ; then
       test_ok_ "$test_subtest_name"
  else
       testname=$this_test.$test_count
       cp "$file1" "$testname.$basename1"
       cp "$file2" "$testname.$basename2"

i.e. the $test_count is already in the files -- in these
cases that is duplicated...

Tomi


> +test_begin_subtest "--batch --input"
> +notmuch dump > backup.tags.$test_count
> +notmuch tag --batch --input=batch.in
> +notmuch search \* | notmuch_search_sanitize > OUTPUT.$test_count
> +notmuch restore < backup.tags.$test_count
> +test_expect_equal_file batch.expected OUTPUT.$test_count
> +
> +test_begin_subtest "--batch, blank lines and comments"
> +notmuch dump | sort > EXPECTED.$test_count
> +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_count
> +test_expect_equal_file EXPECTED.$test_count OUTPUT.$test_count
> +
>  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

Re: [Patch v2 01/17] hex-escape: (en|de)code strings to/from restricted character set

[Jani Nikula]

On Sat, 24 Nov 2012, david@tethera.net wrote:
> From: David Bremner <bremner@debian.org>
>
> The character set is chosen to be suitable for pathnames, and the same
> as that used by contrib/nmbug
>
> [With additions by Jani Nikula]

So it must be good. ;)

Just a couple of nitpicks below.

BR,
Jani.

> ---
>  util/Makefile.local |    2 +-
>  util/hex-escape.c   |  168 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  util/hex-escape.h   |   41 +++++++++++++
>  3 files changed, 210 insertions(+), 1 deletion(-)
>  create mode 100644 util/hex-escape.c
>  create mode 100644 util/hex-escape.h
>
> diff --git a/util/Makefile.local b/util/Makefile.local
> index c7cae61..3ca623e 100644
> --- a/util/Makefile.local
> +++ b/util/Makefile.local
> @@ -3,7 +3,7 @@
>  dir := util
>  extra_cflags += -I$(srcdir)/$(dir)
>  
> -libutil_c_srcs := $(dir)/xutil.c $(dir)/error_util.c
> +libutil_c_srcs := $(dir)/xutil.c $(dir)/error_util.c $(dir)/hex-escape.c
>  
>  libutil_modules := $(libutil_c_srcs:.c=.o)
>  
> diff --git a/util/hex-escape.c b/util/hex-escape.c
> new file mode 100644
> index 0000000..d8905d0
> --- /dev/null
> +++ b/util/hex-escape.c
> @@ -0,0 +1,168 @@
> +/* hex-escape.c -  Manage encoding and decoding of byte strings into path names
> + *
> + * Copyright (c) 2011 David Bremner
> + *
> + * This program is free software: you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation, either version 3 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see http://www.gnu.org/licenses/ .
> + *
> + * Author: David Bremner <david@tethera.net>
> + */
> +
> +#include <assert.h>
> +#include <string.h>
> +#include <talloc.h>
> +#include <ctype.h>
> +#include "error_util.h"
> +#include "hex-escape.h"
> +
> +static const size_t default_buf_size = 1024;
> +
> +static const char *output_charset =
> +    "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+-_@=.:,";
> +
> +static const char escape_char = '%';
> +
> +static int
> +is_output (char c)
> +{
> +    return (strchr (output_charset, c) != NULL);
> +}
> +
> +static int
> +maybe_realloc (void *ctx, size_t needed, char **out, size_t *out_size)
> +{
> +    if (*out_size < needed) {
> +
> +	if (*out == NULL)
> +	    *out = talloc_size (ctx, needed);
> +	else
> +	    *out = talloc_realloc (ctx, *out, char, needed);
> +
> +	if (*out == NULL)
> +	    return 0;
> +
> +	*out_size = needed;
> +    }
> +    return 1;
> +}
> +
> +hex_status_t
> +hex_encode (void *ctx, const char *in, char **out, size_t *out_size)
> +{
> +
> +    const unsigned char *p;

The casts to unsigned char * below bother me. Perhaps this should be
just const char *, with the only cast being in the sprintf?

> +    char *q;
> +
> +    size_t escape_count = 0;
> +    size_t len = 0;
> +    size_t needed;
> +
> +    assert (ctx); assert (in); assert (out); assert (out_size);
> +
> +    for (p = (unsigned char *) in; *p; p++) {
> +	escape_count += (!is_output (*p));
> +	len++;
> +    }
> +
> +    needed = len + escape_count * 2 + 1;

I wonder if it would be clearer if escape_count and len were ditched,
and the for loop just did:

	needed += is_output (*p) ? 1 : 3;

and another needed++ after the loop for NUL. And maybe s/needed/len/
after that.

> +
> +    if (*out == NULL)
> +	*out_size = 0;
> +
> +    if (!maybe_realloc (ctx, needed, out, out_size))
> +	return HEX_OUT_OF_MEMORY;
> +
> +    q = *out;
> +    p = (unsigned char *) in;
> +
> +    while (*p) {
> +	if (is_output (*p)) {
> +	    *q++ = *p++;
> +	} else {
> +	    sprintf (q, "%%%02x", *p++);
> +	    q += 3;
> +	}
> +    }
> +
> +    *q = '\0';
> +    return HEX_SUCCESS;
> +}
> +
> +/* Hex decode 'in' to 'out'.
> + *
> + * This must succeed for in == out to support hex_decode_inplace().
> + */
> +static hex_status_t
> +hex_decode_internal (const char *in, unsigned char *out)
> +{
> +    char buf[3];
> +
> +    while (*in) {
> +	if (*in == escape_char) {
> +	    char *endp;
> +
> +	    /* This also handles unexpected end-of-string. */
> +	    if (!isxdigit ((unsigned char) in[1]) ||
> +		!isxdigit ((unsigned char) in[2]))
> +		return HEX_SYNTAX_ERROR;
> +
> +	    buf[0] = in[1];
> +	    buf[1] = in[2];
> +	    buf[2] = '\0';
> +
> +	    *out = strtoul (buf, &endp, 16);
> +
> +	    if (endp != buf + 2)
> +		return HEX_SYNTAX_ERROR;
> +
> +	    in += 3;
> +	    out++;
> +	} else {
> +	    *out++ = *in++;
> +	}
> +    }
> +
> +    *out = '\0';
> +
> +    return HEX_SUCCESS;
> +}
> +
> +hex_status_t
> +hex_decode_inplace (char *s)
> +{
> +    /* A decoded string is never longer than the encoded one, so it is
> +     * safe to decode a string onto itself. */
> +    return hex_decode_internal (s, (unsigned char *) s);
> +}
> +
> +hex_status_t
> +hex_decode (void *ctx, const char *in, char **out, size_t * out_size)
> +{
> +    const char *p;
> +    size_t escape_count = 0;
> +    size_t needed = 0;
> +
> +    assert (ctx); assert (in); assert (out); assert (out_size);
> +
> +    size_t len = strlen (in);
> +
> +    for (p = in; *p; p++)
> +	escape_count += (*p == escape_char);
> +
> +    needed = len - escape_count * 2 + 1;

Same as above for counting the needed size. It would also save scanning
the input string twice (strlen and for loop).

> +
> +    if (!maybe_realloc (ctx, needed, out, out_size))
> +	return HEX_OUT_OF_MEMORY;
> +
> +    return hex_decode_internal (in, (unsigned char *) *out);
> +}
> diff --git a/util/hex-escape.h b/util/hex-escape.h
> new file mode 100644
> index 0000000..5182042
> --- /dev/null
> +++ b/util/hex-escape.h
> @@ -0,0 +1,41 @@
> +#ifndef _HEX_ESCAPE_H
> +#define _HEX_ESCAPE_H
> +
> +typedef enum hex_status {
> +    HEX_SUCCESS = 0,
> +    HEX_SYNTAX_ERROR,
> +    HEX_OUT_OF_MEMORY
> +} hex_status_t;
> +
> +/*
> + * The API for hex_encode() and hex_decode() is modelled on that for
> + * getline.
> + *
> + * If 'out' points to a NULL pointer a char array of the appropriate
> + * size is allocated using talloc, and out_size is updated.
> + *
> + * If 'out' points to a non-NULL pointer, it assumed to describe an
> + * existing char array, with the size given in *out_size.  This array
> + * may be resized by talloc_realloc if needed; in this case *out_size
> + * will also be updated.
> + *
> + * Note that it is an error to pass a NULL pointer for any parameter
> + * of these routines.
> + */
> +
> +hex_status_t
> +hex_encode (void *talloc_ctx, const char *in, char **out,
> +            size_t *out_size);
> +
> +hex_status_t
> +hex_decode (void *talloc_ctx, const char *in, char **out,
> +            size_t *out_size);
> +
> +/*
> + * Non-allocating hex decode to decode 's' in-place. The length of the
> + * result is always equal to or shorter than the length of the
> + * original.
> + */
> +hex_status_t
> +hex_decode_inplace (char *s);
> +#endif
> -- 
> 1.7.10.4
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [Patch v2 02/17] test/hex-xcode: new test binary

[Jani Nikula]

On Sat, 24 Nov 2012, david@tethera.net wrote:
> From: David Bremner <bremner@debian.org>
>
> This program is used both as a test-bed/unit-tester for
> ../util/hex-escape.c, and also as a utility in future tests of dump
> and restore.

Should this have a mode for hex_decode_inplace()? It's silly from the
tool perspective, but not from test coverage perspective.

There'd be plenty to nitpick here, but it's only a test tool... so
please just remove the extra/double newlines that seem to irritate me
most. ;)

Otherwise, looks good.

BR,
Jani.


> ---
>  test/.gitignore     |    1 +
>  test/Makefile.local |   13 ++++++-
>  test/basic          |    1 +
>  test/hex-xcode.c    |  103 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 116 insertions(+), 2 deletions(-)
>  create mode 100644 test/hex-xcode.c
>
> diff --git a/test/.gitignore b/test/.gitignore
> index e63c689..be7ab5e 100644
> --- a/test/.gitignore
> +++ b/test/.gitignore
> @@ -3,4 +3,5 @@ corpus.mail
>  smtp-dummy
>  symbol-test
>  arg-test
> +hex-xcode
>  tmp.*
> diff --git a/test/Makefile.local b/test/Makefile.local
> index 9ae130a..8da4c56 100644
> --- a/test/Makefile.local
> +++ b/test/Makefile.local
> @@ -13,6 +13,9 @@ smtp_dummy_modules = $(smtp_dummy_srcs:.c=.o)
>  $(dir)/arg-test: $(dir)/arg-test.o command-line-arguments.o util/libutil.a
>  	$(call quiet,CC) -I. $^ -o $@
>  
> +$(dir)/hex-xcode: $(dir)/hex-xcode.o command-line-arguments.o util/libutil.a
> +	$(call quiet,CC) -I. $^ -o $@ -ltalloc
> +
>  $(dir)/smtp-dummy: $(smtp_dummy_modules)
>  	$(call quiet,CC) $^ -o $@
>  
> @@ -24,8 +27,13 @@ $(dir)/parse-time: $(dir)/parse-time.o parse-time-string/parse-time-string.o
>  
>  .PHONY: test check
>  
> -test-binaries: $(dir)/arg-test $(dir)/smtp-dummy $(dir)/symbol-test \
> -	$(dir)/parse-time
> +TEST_BINARIES=$(dir)/arg-test \
> +	      $(dir)/hex-xcode \
> +	      $(dir)/parse-time \
> +	      $(dir)/smtp-dummy \
> +	      $(dir)/symbol-test
> +
> +test-binaries: $(TEST_BINARIES)
>  
>  test:	all test-binaries
>  	@${dir}/notmuch-test $(OPTIONS)
> @@ -36,5 +44,6 @@ SRCS := $(SRCS) $(smtp_dummy_srcs)
>  CLEAN := $(CLEAN) $(dir)/smtp-dummy $(dir)/smtp-dummy.o \
>  	 $(dir)/symbol-test $(dir)/symbol-test.o \
>  	 $(dir)/arg-test $(dir)/arg-test.o \
> +	 $(dir)/hex-xcode $(dir)/hex-xcode.o \
>  	 $(dir)/parse-time $(dir)/parse-time.o \
>  	 $(dir)/corpus.mail $(dir)/test-results $(dir)/tmp.*
> diff --git a/test/basic b/test/basic
> index 1b842d2..2a571ac 100755
> --- a/test/basic
> +++ b/test/basic
> @@ -56,6 +56,7 @@ tests_in_suite=$(for i in $TESTS; do echo $i; done | sort)
>  available=$(find "$TEST_DIRECTORY" -maxdepth 1 -type f -perm +111  \
>      ! -name aggregate-results.sh	\
>      ! -name arg-test			\
> +    ! -name hex-xcode			\
>      ! -name notmuch-test		\
>      ! -name parse-time			\
>      ! -name smtp-dummy			\
> diff --git a/test/hex-xcode.c b/test/hex-xcode.c
> new file mode 100644
> index 0000000..eec6541
> --- /dev/null
> +++ b/test/hex-xcode.c
> @@ -0,0 +1,103 @@
> +/* No, nothing to to with IDE from Apple Inc.
> +   testbed for ../util/hex-escape.c.
> +
> +   usage:
> +   hex-xcode [--direction=(encode|decode)] [--omit-newline] < file
> +   hex-xcode [--direction=(encode|decode)] [--omit-newline] arg1 arg2 arg3 ...
> +
> + */
> +
> +#include "notmuch-client.h"
> +#include "hex-escape.h"
> +#include <assert.h>
> +
> +
> +enum direction {
> +    ENCODE,
> +    DECODE
> +};
> +
> +static int
> +xcode (void *ctx, enum direction dir, char *in, char **buf_p, size_t *size_p)
> +{
> +    hex_status_t status;
> +
> +    if (dir == ENCODE)
> +	status = hex_encode (ctx, in, buf_p, size_p);
> +    else
> +	status = hex_decode (ctx, in, buf_p, size_p);
> +
> +    if (status == HEX_SUCCESS)
> +	fputs (*buf_p, stdout);
> +
> +    return status;
> +}
> +
> +
> +int
> +main (int argc, char **argv)
> +{
> +
> +
> +    enum direction dir = DECODE;
> +    int omit_newline = FALSE;
> +
> +    notmuch_opt_desc_t options[] = {
> +	{ NOTMUCH_OPT_KEYWORD, &dir, "direction", 'd',
> +	  (notmuch_keyword_t []){ { "encode", ENCODE },
> +				  { "decode", DECODE },
> +				  { 0, 0 } } },
> +	{ NOTMUCH_OPT_BOOLEAN, &omit_newline, "omit-newline", 'n', 0 },
> +	{ 0, 0, 0, 0, 0 }
> +    };
> +
> +    int opt_index = parse_arguments (argc, argv, options, 1);
> +
> +    if (opt_index < 0)
> +	exit (1);
> +
> +    void *ctx = talloc_new (NULL);
> +
> +    char *line = NULL;
> +    size_t line_size;
> +    ssize_t line_len;
> +
> +    char *buffer = NULL;
> +    size_t buf_size = 0;
> +
> +    notmuch_bool_t read_stdin = TRUE;
> +
> +    for (; opt_index < argc; opt_index++) {
> +
> +	if (xcode (ctx, dir, argv[opt_index],
> +		   &buffer, &buf_size) != HEX_SUCCESS)
> +	    return 1;
> +
> +	if (!omit_newline)
> +	    putchar ('\n');
> +
> +	read_stdin = FALSE;
> +    }
> +
> +    if (!read_stdin)
> +	return 0;
> +
> +    while ((line_len = getline (&line, &line_size, stdin)) != -1) {
> +
> +	chomp_newline (line);
> +
> +	if (xcode (ctx, dir, line, &buffer, &buf_size) != HEX_SUCCESS)
> +	    return 1;
> +
> +	if (!omit_newline)
> +	    putchar ('\n');
> +
> +    }
> +
> +    if (line)
> +	free (line);
> +
> +    talloc_free (ctx);
> +
> +    return 0;
> +}
> -- 
> 1.7.10.4
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [Patch v2 03/17] test/hex-escaping: new test for hex escaping routines

[Jani Nikula]

On Sat, 24 Nov 2012, david@tethera.net wrote:
> From: David Bremner <bremner@debian.org>
>
> These are more like unit tests, to (try to) make sure the library
> functionality is working before building more complicated things on
> top of it.

LGTM.

Here I observe that 'hex-xcode encode' and 'hex-xcode decode' might be a
better fit than --direction. But this isn't of importance.

BR,
Jani.


> ---
>  test/hex-escaping |   26 ++++++++++++++++++++++++++
>  test/notmuch-test |    1 +
>  2 files changed, 27 insertions(+)
>  create mode 100755 test/hex-escaping
>
> diff --git a/test/hex-escaping b/test/hex-escaping
> new file mode 100755
> index 0000000..f34cc8c
> --- /dev/null
> +++ b/test/hex-escaping
> @@ -0,0 +1,26 @@
> +#!/usr/bin/env bash
> +test_description="hex encoding and decoding"
> +. ./test-lib.sh
> +
> +test_begin_subtest "round trip"
> +find $TEST_DIRECTORY/corpus -type f -print | sort | xargs cat > EXPECTED
> +$TEST_DIRECTORY/hex-xcode --direction=encode < EXPECTED | $TEST_DIRECTORY/hex-xcode --direction=decode > OUTPUT
> +test_expect_equal_file OUTPUT EXPECTED
> +
> +test_begin_subtest "punctuation"
> +tag1='comic_swear=$&^%$^%\\//-+$^%$'
> +tag_enc1=$($TEST_DIRECTORY/hex-xcode --direction=encode "$tag1")
> +test_expect_equal "$tag_enc1" "comic_swear=%24%26%5e%25%24%5e%25%5c%5c%2f%2f-+%24%5e%25%24"
> +
> +test_begin_subtest "round trip newlines"
> +printf 'this\n tag\t has\n spaces\n' > EXPECTED.$test_count
> +$TEST_DIRECTORY/hex-xcode --direction=encode  < EXPECTED.$test_count |\
> +	$TEST_DIRECTORY/hex-xcode --direction=decode > OUTPUT.$test_count
> +test_expect_equal_file EXPECTED.$test_count OUTPUT.$test_count
> +
> +test_begin_subtest "round trip 8bit chars"
> +echo '%c3%91%c3%a5%c3%b0%c3%a3%c3%a5%c3%a9-%c3%8f%c3%8a' > EXPECTED.$test_count
> +$TEST_DIRECTORY/hex-xcode --direction=decode  < EXPECTED.$test_count |\
> +	$TEST_DIRECTORY/hex-xcode --direction=encode > OUTPUT.$test_count
> +test_expect_equal_file EXPECTED.$test_count OUTPUT.$test_count
> +test_done
> diff --git a/test/notmuch-test b/test/notmuch-test
> index 9a1b375..d2e90e2 100755
> --- a/test/notmuch-test
> +++ b/test/notmuch-test
> @@ -60,6 +60,7 @@ TESTS="
>    emacs-hello
>    emacs-show
>    missing-headers
> +  hex-escaping
>    parse-time-string
>    search-date
>  "
> -- 
> 1.7.10.4
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [Patch v2 04/17] test: add database routines for testing

[Jani Nikula]

On Sat, 24 Nov 2012, david@tethera.net wrote:
> From: David Bremner <bremner@debian.org>
>
> Initially, provide a way to create "stub" messages in the notmuch
> database without corresponding files.  This is essentially cut and
> paste from lib/database.cc. This is a seperate file since we don't
> want to export these symbols from libnotmuch or bloat the library with
> non-exported code.
> ---
>  test/Makefile.local  |    1 +
>  test/database-test.c |   71 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  test/database-test.h |   21 +++++++++++++++
>  3 files changed, 93 insertions(+)
>  create mode 100644 test/database-test.c
>  create mode 100644 test/database-test.h
>
> diff --git a/test/Makefile.local b/test/Makefile.local
> index 8da4c56..8479f91 100644
> --- a/test/Makefile.local
> +++ b/test/Makefile.local
> @@ -45,5 +45,6 @@ CLEAN := $(CLEAN) $(dir)/smtp-dummy $(dir)/smtp-dummy.o \
>  	 $(dir)/symbol-test $(dir)/symbol-test.o \
>  	 $(dir)/arg-test $(dir)/arg-test.o \
>  	 $(dir)/hex-xcode $(dir)/hex-xcode.o \
> +	 $(dir)/database-test.o \
>  	 $(dir)/parse-time $(dir)/parse-time.o \
>  	 $(dir)/corpus.mail $(dir)/test-results $(dir)/tmp.*
> diff --git a/test/database-test.c b/test/database-test.c
> new file mode 100644
> index 0000000..739e03b
> --- /dev/null
> +++ b/test/database-test.c
> @@ -0,0 +1,71 @@
> +/*
> + * Database routines intended only for testing, not exported from
> + * library.
> + *
> + * Copyright (c) 2012 David Bremner
> + *
> + * This program is free software: you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation, either version 3 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see http://www.gnu.org/licenses/ .
> + *
> + * Author: David Bremner <david@tethera.net>
> + */
> +
> +#include "notmuch-private.h"
> +#include "database-test.h"
> +
> +notmuch_status_t
> +notmuch_database_add_stub_message (notmuch_database_t *notmuch,
> +				   const char *message_id,
> +				   const char **tags)
> +{
> +    const char **tag;
> +    notmuch_status_t ret;
> +    notmuch_private_status_t private_status;
> +    notmuch_message_t *message;
> +
> +    ret = _notmuch_database_ensure_writable (notmuch);
> +    if (ret)
> +	return ret;
> +
> +    message = _notmuch_message_create_for_message_id (notmuch,
> +						      message_id,
> +						      &private_status);
> +    if (message == NULL) {
> +	return COERCE_STATUS (private_status,
> +			      "Unexpected status value from _notmuch_message_create_for_message_id");
> +
> +    }
> +
> +    if (private_status != NOTMUCH_PRIVATE_STATUS_NO_DOCUMENT_FOUND)
> +	return NOTMUCH_STATUS_DUPLICATE_MESSAGE_ID;
> +
> +    _notmuch_message_add_term (message, "type", "mail");
> +
> +    if (tags) {
> +	ret = notmuch_message_freeze (message);
> +	if (ret)
> +	    return ret;
> +
> +	for (tag = tags; *tag; tag++) {
> +	    ret = notmuch_message_add_tag (message, *tag);
> +	    if (ret)
> +		return ret;
> +	}
> +    }
> +
> +    ret = notmuch_message_thaw (message);
> +    if (ret)
> +	return ret;

You'll get NOTMUCH_STATUS_UNBALANCED_FREEZE_THAW whenever tags ==
NULL. You need to move thaw within the if (tags) block.

Otherwise, looks good.

BR,
Jani.

> +
> +    return NOTMUCH_STATUS_SUCCESS;
> +}
> diff --git a/test/database-test.h b/test/database-test.h
> new file mode 100644
> index 0000000..84f7988
> --- /dev/null
> +++ b/test/database-test.h
> @@ -0,0 +1,21 @@
> +#ifndef _DATABASE_TEST_H
> +#define _DATABASE_TEST_H
> +/* Add a new stub message to the given notmuch database.
> + *
> + * At least the following return values are possible:
> + *
> + * NOTMUCH_STATUS_SUCCESS: Message successfully added to database.
> + *
> + * NOTMUCH_STATUS_DUPLICATE_MESSAGE_ID: Message has the same message
> + *	ID as another message already in the database.
> + *
> + * NOTMUCH_STATUS_READ_ONLY_DATABASE: Database was opened in read-only
> + *	mode so no message can be added.
> + */
> +
> +notmuch_status_t
> +notmuch_database_add_stub_message (notmuch_database_t *database,
> +				   const char *message_id,
> +				   const char **tag_list);
> +
> +#endif
> -- 
> 1.7.10.4
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [Patch v2 05/17] test: add generator for random "stub" messages

[Jani Nikula]

On Sat, 24 Nov 2012, david@tethera.net wrote:
> From: David Bremner <bremner@debian.org>
>
> Initial use case is testing dump and restore, so we only have
> message-ids and tags.
>
> The message ID's are nothing like RFC compliant, but it doesn't seem
> any harder to roundtrip random UTF-8 strings than RFC-compliant ones.
>
> Tags are UTF-8, even though notmuch is in principle more generous than
> that.
>
> updated for id:m2wr04ocro.fsf@guru.guru-group.fi
>
> - talk about Unicode value rather some specific encoding
> - call talloc_realloc less times
> ---
>  test/.gitignore      |    1 +
>  test/Makefile.local  |   10 +++
>  test/basic           |    1 +
>  test/random-corpus.c |  204 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 216 insertions(+)
>  create mode 100644 test/random-corpus.c
>
> diff --git a/test/.gitignore b/test/.gitignore
> index be7ab5e..1eff7ce 100644
> --- a/test/.gitignore
> +++ b/test/.gitignore
> @@ -4,4 +4,5 @@ smtp-dummy
>  symbol-test
>  arg-test
>  hex-xcode
> +random-corpus
>  tmp.*
> diff --git a/test/Makefile.local b/test/Makefile.local
> index 8479f91..6a9f15e 100644
> --- a/test/Makefile.local
> +++ b/test/Makefile.local
> @@ -16,6 +16,14 @@ $(dir)/arg-test: $(dir)/arg-test.o command-line-arguments.o util/libutil.a
>  $(dir)/hex-xcode: $(dir)/hex-xcode.o command-line-arguments.o util/libutil.a
>  	$(call quiet,CC) -I. $^ -o $@ -ltalloc
>  
> +random_corpus_deps =  $(dir)/random-corpus.o  $(dir)/database-test.o \
> +			notmuch-config.o command-line-arguments.o \
> +			lib/libnotmuch.a util/libutil.a \
> +			parse-time-string/libparse-time-string.a
> +
> +$(dir)/random-corpus: $(random_corpus_deps)
> +	$(call quiet,CC) $(CFLAGS_FINAL) $^ -o $@ $(CONFIGURE_LDFLAGS)
> +
>  $(dir)/smtp-dummy: $(smtp_dummy_modules)
>  	$(call quiet,CC) $^ -o $@
>  
> @@ -29,6 +37,7 @@ $(dir)/parse-time: $(dir)/parse-time.o parse-time-string/parse-time-string.o
>  
>  TEST_BINARIES=$(dir)/arg-test \
>  	      $(dir)/hex-xcode \
> +	      $(dir)/random-corpus \
>  	      $(dir)/parse-time \
>  	      $(dir)/smtp-dummy \
>  	      $(dir)/symbol-test
> @@ -46,5 +55,6 @@ CLEAN := $(CLEAN) $(dir)/smtp-dummy $(dir)/smtp-dummy.o \
>  	 $(dir)/arg-test $(dir)/arg-test.o \
>  	 $(dir)/hex-xcode $(dir)/hex-xcode.o \
>  	 $(dir)/database-test.o \
> +	 $(dir)/random-corpus $(dir)/random-corpus.o \
>  	 $(dir)/parse-time $(dir)/parse-time.o \
>  	 $(dir)/corpus.mail $(dir)/test-results $(dir)/tmp.*
> diff --git a/test/basic b/test/basic
> index 2a571ac..f93469f 100755
> --- a/test/basic
> +++ b/test/basic
> @@ -59,6 +59,7 @@ available=$(find "$TEST_DIRECTORY" -maxdepth 1 -type f -perm +111  \
>      ! -name hex-xcode			\
>      ! -name notmuch-test		\
>      ! -name parse-time			\
> +    ! -name random-corpus		\
>      ! -name smtp-dummy			\
>      ! -name symbol-test			\
>      ! -name test-verbose		\
> diff --git a/test/random-corpus.c b/test/random-corpus.c
> new file mode 100644
> index 0000000..085bda0
> --- /dev/null
> +++ b/test/random-corpus.c
> @@ -0,0 +1,204 @@
> +/*
> + * Generate a random corpus of stub messages.
> + *
> + * Initial use case is testing dump and restore, so we only have
> + * message-ids and tags.
> + *
> + * Generated message-id's and tags are intentionally nasty.
> + *
> + * Copyright (c) 2012 David Bremner
> + *
> + * This program is free software: you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation, either version 3 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see http://www.gnu.org/licenses/ .
> + *
> + * Author: David Bremner <david@tethera.net>
> + */
> +
> +#include <stdlib.h>
> +#include <assert.h>
> +#include <talloc.h>
> +#include <string.h>
> +#include <glib.h>
> +#include <math.h>
> +
> +#include "notmuch-client.h"
> +#include "command-line-arguments.h"
> +#include "database-test.h"
> +
> +/* Current largest Unicode value defined. Note that most of these will
> + * be printed as boxes in most fonts.
> + */
> +
> +#define GLYPH_MAX 0x10FFFE
> +
> +static gunichar
> +random_unichar ()
> +{
> +    int start = 1, stop = GLYPH_MAX;
> +    int class = random() % 2;
> +
> +    /*
> +     *  Choose about half ascii as test characters, as ascii
> +     *  punctation and whitespace is the main cause of problems for
> +     *  the (old) restore parser
> +    */

I wonder if there should be even more emphasis on ascii and unicode code
points up to U+07FF (which maps to two UTF-8 bytes).

> +    switch (class) {
> +    case 0:
> +	/* ascii */
> +	start = 0x01;
> +	stop = 0x7f;
> +	break;
> +    case 1:
> +	/* the rest of unicode */
> +	start = 0x80;
> +	stop = GLYPH_MAX;
> +    }
> +
> +    if (start == stop)
> +	return start;
> +    else
> +	return start + (random() % (stop - start + 1));
> +}
> +
> +static char *
> +random_utf8_string (void *ctx, size_t char_count)
> +{
> +    size_t offset = 0;
> +    size_t i;
> +

Irritating blank line. ;)

> +    gchar *buf = NULL;
> +    size_t buf_size = 0;

You could do an initial talloc of the buf based on char_count,
e.g. twice that.

> +
> +    for (i = 0; i < char_count; i++) {
> +	gunichar randomchar;
> +	size_t written;
> +
> +	/* 6 for one glyph, one for null, one for luck */
> +	while (buf_size - offset < 8) {

I'd probably write that (offset + 8 >= buf_size).

> +	    buf_size = 2 * buf_size + 8;
> +	    buf = talloc_realloc (ctx, buf, gchar, buf_size);
> +	}
> +
> +	randomchar = random_unichar();
> +
> +	written = g_unichar_to_utf8 (randomchar, buf + offset);
> +
> +	if (written <= 0) {
> +	    fprintf (stderr, "error converting to utf8\n");
> +	    exit (1);
> +	}
> +
> +	offset += written;
> +
> +    }
> +    buf[offset] = 0;
> +    return buf;
> +}
> +
> +
> +int
> +main (int argc, char **argv)
> +{
> +
> +    void *ctx = talloc_new (NULL);
> +
> +    char *config_path  = NULL;
> +    notmuch_config_t *config;
> +    notmuch_database_t *notmuch;
> +
> +    int num_messages = 500;
> +    int max_tags = 10;
> +    // leave room for UTF-8 encoding.
> +    int tag_len = NOTMUCH_TAG_MAX / 6;
> +    // NOTMUCH_MESSAGE_ID_MAX is not exported, so we make a
> +    // conservative guess.
> +    int message_id_len = (NOTMUCH_TAG_MAX - 20) / 6;
> +
> +    int seed = 734569;
> +
> +    notmuch_opt_desc_t options[] = {
> +	{ NOTMUCH_OPT_STRING, &config_path, "config-path", 'c', 0 },
> +	{ NOTMUCH_OPT_INT, &num_messages, "num-messages", 'n', 0 },
> +	{ NOTMUCH_OPT_INT, &max_tags, "max-tags", 'm', 0 },
> +	{ NOTMUCH_OPT_INT, &message_id_len, "message-id-len", 'M', 0 },
> +	{ NOTMUCH_OPT_INT, &tag_len, "tag-len", 't', 0 },
> +	{ NOTMUCH_OPT_INT, &seed, "seed", 's', 0 },
> +	{ 0, 0, 0, 0, 0 }
> +    };
> +
> +    int opt_index = parse_arguments (argc, argv, options, 1);
> +
> +    if (opt_index < 0)
> +	exit (1);
> +
> +    if (message_id_len < 1) {
> +	fprintf (stderr, "message id's must be least length 1\n");
> +	exit (1);
> +    }
> +
> +    if (config_path == NULL) {
> +	fprintf (stderr, "configuration path must be specified");
> +	exit (1);
> +    }
> +
> +    config = notmuch_config_open (ctx, config_path, NULL);
> +    if (config == NULL)
> +	return 1;
> +
> +    if (notmuch_database_open (notmuch_config_get_database_path (config),
> +			       NOTMUCH_DATABASE_MODE_READ_WRITE, &notmuch))
> +	return 1;
> +
> +    srandom (seed);
> +
> +    int count;
> +    for (count = 0; count < num_messages; count++) {
> +	int j;
> +	/* explicitly allow zero tags */
> +	int num_tags = random () % (max_tags + 1);
> +	/* message ids should be non-empty */
> +	int this_mid_len = (random () % message_id_len) + 1;
> +	const char **tag_list;
> +	char *mid;
> +	notmuch_status_t status;
> +
> +	do {
> +	    mid = random_utf8_string (ctx, this_mid_len);
> +
> +	    tag_list = talloc_realloc (ctx, NULL, const char *, num_tags + 2);
> +
> +	    tag_list[0] = "random-corpus";

We'll probably want messages completely without tags in the tests
too. How about a parameter to define the random-corpus tag. Which can be
left out too. And if it's present, another parameter to tell whether it
should be added even if num_tags == 0.

Do the tags always come out sorted from the library? I'm wondering if
it's a good idea to always put random-corpus there first. If it matters,
you could use random () % num_tags to decide the position in the tag
list to put the random-corpus tag in.

BR,
Jani.


> +
> +	    for (j = 0; j < num_tags; j++) {
> +		int this_tag_len = random () % tag_len + 1;
> +
> +		tag_list[j + 1] = random_utf8_string (ctx, this_tag_len);
> +	    }
> +
> +	    tag_list[j + 1] = NULL;
> +
> +	    status = notmuch_database_add_stub_message (notmuch, mid, tag_list);
> +	} while (status == NOTMUCH_STATUS_DUPLICATE_MESSAGE_ID);
> +
> +	if (status != NOTMUCH_STATUS_SUCCESS) {
> +	    fprintf (stderr, "error %d adding message", status);
> +	    exit (status);
> +	}
> +    }
> +
> +    notmuch_database_destroy (notmuch);
> +
> +    talloc_free (ctx);
> +
> +    return 0;
> +}
> -- 
> 1.7.10.4
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [Patch v2 06/17] test: add broken roundtrip test

[Jani Nikula]

On Sat, 24 Nov 2012, david@tethera.net wrote:
> From: David Bremner <bremner@debian.org>
>
> We demonstrate the current notmuch restore parser being confused by
> message-id's and tags containing non alpha numeric characters
> (particularly space and parentheses are problematic because they are
> not escaped by notmuch dump).
>
> We save the files as hex escaped on disk so that the output from the
> failing test will not confuse the terminal emulator of people running
> the test.

Theoretically this could pass without failing just fine. ;)

LGTM,
Jani.

> ---
>  test/dump-restore |    9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/test/dump-restore b/test/dump-restore
> index b05399c..a2204fb 100755
> --- a/test/dump-restore
> +++ b/test/dump-restore
> @@ -85,4 +85,13 @@ test_begin_subtest "dump --output=outfile -- from:cworth"
>  notmuch dump --output=dump-outfile-dash-inbox.actual -- from:cworth
>  test_expect_equal_file dump-cworth.expected dump-outfile-dash-inbox.actual
>  
> +test_expect_success 'roundtripping random message-ids and tags' \
> +    'test_subtest_known_broken &&
> +     ${TEST_DIRECTORY}/random-corpus --num-messages=10 --config-path=${NOTMUCH_CONFIG} &&
> +     notmuch dump | ${TEST_DIRECTORY}/hex-xcode --direction=encode > EXPECTED.$test_count &&
> +     notmuch tag -random-corpus tag:random-corpus &&
> +     ${TEST_DIRECTORY}/hex-xcode --direction=decode < EXPECTED.$test_count | notmuch restore 2>/dev/null &&
> +     notmuch dump | ${TEST_DIRECTORY}/hex-xcode --direction=encode > OUTPUT.$test_count &&
> +     test_cmp EXPECTED.$test_count OUTPUT.$test_count'
> +
>  test_done
> -- 
> 1.7.10.4
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [Patch v2 07/17] notmuch-dump: add --format=(batch-tag|sup)

[Jani Nikula]

On Sat, 24 Nov 2012, david@tethera.net wrote:
> From: David Bremner <bremner@debian.org>
>
> sup is the old format, and remains the default, at least until
> restore is converted to parse this format.
>
> Each line of the batch- format is valid for notmuch tag, (with the
> "notmuch tag" omitted from the front of the line). The dump format
> only uses query strings of a single message-id.

Valid for notmuch tag except for the hex encoding and empty set of tags.

> Each space seperated tag/message-id is 'hex-encoded' to remove
> troubling characters.  In particular this format won't have the same
> problem with e.g. spaces in message-ids or tags; they will be
> round-trip-able.
> ---
>  dump-restore-private.h |   13 +++++++++++++
>  notmuch-dump.c         |   42 ++++++++++++++++++++++++++++++++++++------
>  2 files changed, 49 insertions(+), 6 deletions(-)
>  create mode 100644 dump-restore-private.h
>
> diff --git a/dump-restore-private.h b/dump-restore-private.h
> new file mode 100644
> index 0000000..896a004
> --- /dev/null
> +++ b/dump-restore-private.h
> @@ -0,0 +1,13 @@
> +#ifndef DUMP_RESTORE_PRIVATE_H
> +#define DUMP_RESTORE_PRIVATE_H
> +
> +#include "hex-escape.h"
> +#include "command-line-arguments.h"
> +
> +typedef enum dump_formats {
> +    DUMP_FORMAT_AUTO,
> +    DUMP_FORMAT_BATCH_TAG,
> +    DUMP_FORMAT_SUP
> +} dump_format_t;
> +
> +#endif
> diff --git a/notmuch-dump.c b/notmuch-dump.c
> index 88f598a..045ca9e 100644
> --- a/notmuch-dump.c
> +++ b/notmuch-dump.c
> @@ -19,6 +19,7 @@
>   */
>  
>  #include "notmuch-client.h"
> +#include "dump-restore-private.h"
>  
>  int
>  notmuch_dump_command (unused (void *ctx), int argc, char *argv[])
> @@ -43,7 +44,13 @@ notmuch_dump_command (unused (void *ctx), int argc, char *argv[])
>      char *output_file_name = NULL;
>      int opt_index;
>  
> +    int output_format = DUMP_FORMAT_SUP;
> +
>      notmuch_opt_desc_t options[] = {
> +	{ NOTMUCH_OPT_KEYWORD, &output_format, "format", 'f',
> +	  (notmuch_keyword_t []){ { "sup", DUMP_FORMAT_SUP },
> +				  { "batch-tag", DUMP_FORMAT_BATCH_TAG },
> +				  { 0, 0 } } },
>  	{ NOTMUCH_OPT_STRING, &output_file_name, "output", 'o', 0  },
>  	{ 0, 0, 0, 0, 0 }
>      };
> @@ -83,27 +90,50 @@ notmuch_dump_command (unused (void *ctx), int argc, char *argv[])
>       */
>      notmuch_query_set_sort (query, NOTMUCH_SORT_UNSORTED);
>  
> +    char *buffer = NULL;
> +    size_t buffer_size = 0;
> +
>      for (messages = notmuch_query_search_messages (query);
>  	 notmuch_messages_valid (messages);
>  	 notmuch_messages_move_to_next (messages)) {
>  	int first = 1;
> +	const char *message_id;
> +
>  	message = notmuch_messages_get (messages);
> +	message_id = notmuch_message_get_message_id (message);
>  
> -	fprintf (output,
> -		 "%s (", notmuch_message_get_message_id (message));
> +	if (output_format == DUMP_FORMAT_SUP) {
> +	    fprintf (output, "%s (", message_id);
> +	}
>  
>  	for (tags = notmuch_message_get_tags (message);
>  	     notmuch_tags_valid (tags);
>  	     notmuch_tags_move_to_next (tags)) {
> -	    if (! first)
> -		fprintf (output, " ");
> +	    const char *tag_str = notmuch_tags_get (tags);
>  
> -	    fprintf (output, "%s", notmuch_tags_get (tags));
> +	    if (! first)
> +		fputs (" ", output);
>  
>  	    first = 0;
> +
> +	    if (output_format == DUMP_FORMAT_SUP) {
> +		fputs (tag_str, output);
> +	    } else {
> +		if (hex_encode (notmuch, tag_str,
> +				&buffer, &buffer_size) != HEX_SUCCESS)
> +		    return 1;

Maybe hex_encode errors would deserve an error message. Ditto below.

Otherwise LGTM.

BR,
Jani.

> +		fprintf (output, "+%s", buffer);
> +	    }
>  	}
>  
> -	fprintf (output, ")\n");
> +	if (output_format == DUMP_FORMAT_SUP) {
> +	    fputs (")\n", output);
> +	} else {
> +	    if (hex_encode (notmuch, message_id,
> +			    &buffer, &buffer_size) != HEX_SUCCESS)
> +		return 1;
> +	    fprintf (output, " -- id:%s\n", buffer);
> +	}
>  
>  	notmuch_message_destroy (message);
>      }
> -- 
> 1.7.10.4
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [Patch v2 08/17] util: add string-util.[ch]

[Jani Nikula]

On Sat, 24 Nov 2012, david@tethera.net wrote:
> From: David Bremner <bremner@debian.org>
>
> This is to give a home to strtok_len. It's a bit silly to add a header
> for one routine, but it needs to be shared between several compilation
> units (or at least that's the most natural design).

Good stuff! ;)

Jani.

> ---
>  util/Makefile.local |    3 ++-
>  util/string-util.c  |   34 ++++++++++++++++++++++++++++++++++
>  util/string-util.h  |   19 +++++++++++++++++++
>  3 files changed, 55 insertions(+), 1 deletion(-)
>  create mode 100644 util/string-util.c
>  create mode 100644 util/string-util.h
>
> diff --git a/util/Makefile.local b/util/Makefile.local
> index 3ca623e..a11e35b 100644
> --- a/util/Makefile.local
> +++ b/util/Makefile.local
> @@ -3,7 +3,8 @@
>  dir := util
>  extra_cflags += -I$(srcdir)/$(dir)
>  
> -libutil_c_srcs := $(dir)/xutil.c $(dir)/error_util.c $(dir)/hex-escape.c
> +libutil_c_srcs := $(dir)/xutil.c $(dir)/error_util.c $(dir)/hex-escape.c \
> +		  $(dir)/string-util.c
>  
>  libutil_modules := $(libutil_c_srcs:.c=.o)
>  
> diff --git a/util/string-util.c b/util/string-util.c
> new file mode 100644
> index 0000000..44f8cd3
> --- /dev/null
> +++ b/util/string-util.c
> @@ -0,0 +1,34 @@
> +/* string-util.c -  Extra or enhanced routines for null terminated strings.
> + *
> + * Copyright (c) 2012 Jani Nikula
> + *
> + * This program is free software: you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation, either version 3 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see http://www.gnu.org/licenses/ .
> + *
> + * Author: Jani Nikula <jani@nikula.org>
> + */
> +
> +
> +#include "string-util.h"
> +
> +char *
> +strtok_len (char *s, const char *delim, size_t *len)
> +{
> +    /* skip initial delims */
> +    s += strspn (s, delim);
> +
> +    /* length of token */
> +    *len = strcspn (s, delim);
> +
> +    return *len ? s : NULL;
> +}
> diff --git a/util/string-util.h b/util/string-util.h
> new file mode 100644
> index 0000000..696da40
> --- /dev/null
> +++ b/util/string-util.h
> @@ -0,0 +1,19 @@
> +#ifndef _STRING_UTIL_H
> +#define _STRING_UTIL_H
> +
> +#include <string.h>
> +
> +/* like strtok(3), but without state, and doesn't modify s. usage pattern:
> + *
> + * const char *tok = input;
> + * const char *delim = " \t";
> + * size_t tok_len = 0;
> + *
> + * while ((tok = strtok_len (tok + tok_len, delim, &tok_len)) != NULL) {
> + *     // do stuff with string tok of length tok_len
> + * }
> + */
> +
> +char *strtok_len (char *s, const char *delim, size_t *len);
> +
> +#endif
> -- 
> 1.7.10.4
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [Patch v2 06/17] test: add broken roundtrip test

[Jani Nikula]

BTW, patches up to and including this one could go in even if there's
still stuff to do in the following ones.

Jani.


On Sat, 24 Nov 2012, david@tethera.net wrote:
> From: David Bremner <bremner@debian.org>
>
> We demonstrate the current notmuch restore parser being confused by
> message-id's and tags containing non alpha numeric characters
> (particularly space and parentheses are problematic because they are
> not escaped by notmuch dump).
>
> We save the files as hex escaped on disk so that the output from the
> failing test will not confuse the terminal emulator of people running
> the test.
> ---
>  test/dump-restore |    9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/test/dump-restore b/test/dump-restore
> index b05399c..a2204fb 100755
> --- a/test/dump-restore
> +++ b/test/dump-restore
> @@ -85,4 +85,13 @@ test_begin_subtest "dump --output=outfile -- from:cworth"
>  notmuch dump --output=dump-outfile-dash-inbox.actual -- from:cworth
>  test_expect_equal_file dump-cworth.expected dump-outfile-dash-inbox.actual
>  
> +test_expect_success 'roundtripping random message-ids and tags' \
> +    'test_subtest_known_broken &&
> +     ${TEST_DIRECTORY}/random-corpus --num-messages=10 --config-path=${NOTMUCH_CONFIG} &&
> +     notmuch dump | ${TEST_DIRECTORY}/hex-xcode --direction=encode > EXPECTED.$test_count &&
> +     notmuch tag -random-corpus tag:random-corpus &&
> +     ${TEST_DIRECTORY}/hex-xcode --direction=decode < EXPECTED.$test_count | notmuch restore 2>/dev/null &&
> +     notmuch dump | ${TEST_DIRECTORY}/hex-xcode --direction=encode > OUTPUT.$test_count &&
> +     test_cmp EXPECTED.$test_count OUTPUT.$test_count'
> +
>  test_done
> -- 
> 1.7.10.4
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [Patch v2 09/17] tag-util.[ch]: New files for common tagging routines

[Austin Clements]

Quoth David Bremner on Nov 29 at  8:17 pm:
> Mark Walters <markwalters1009@gmail.com> writes:
> > I don't know how freeze/thaw work but does it matter that you don't thaw
> > when there is an error?
> 
> My interpretation is that by not thawing before we destroy the message,
> we are aborting the transaction, since the freeze/thaw information is
> stored in the message structure. It is documented as forbidden to call
> thaw _more_ times than freeze, but less is not explicitely mentioned.

Yes, this should work.  It smells a little hacky and I couldn't say if
it was an intended use of the API, but it should work.  Perhaps we
should document it.

Re: [Patch v2 09/17] tag-util.[ch]: New files for common tagging routines

[Jani Nikula]

On Sat, 24 Nov 2012, david@tethera.net wrote:
> From: David Bremner <bremner@debian.org>
>
> These are meant to be shared between notmuch-tag and notmuch-restore.
>
> The bulk of the routines implement a "tag operation list" abstract
> data type act as a structured representation of a set of tag
> operations (typically coming from a single tag command or line of
> input).
> ---
>  Makefile.local |    1 +
>  tag-util.c     |  264 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  tag-util.h     |  120 ++++++++++++++++++++++++++
>  3 files changed, 385 insertions(+)
>  create mode 100644 tag-util.c
>  create mode 100644 tag-util.h
>
> diff --git a/Makefile.local b/Makefile.local
> index 2b91946..854867d 100644
> --- a/Makefile.local
> +++ b/Makefile.local
> @@ -274,6 +274,7 @@ notmuch_client_srcs =		\
>  	query-string.c		\
>  	mime-node.c		\
>  	crypto.c		\
> +	tag-util.c
>  
>  notmuch_client_modules = $(notmuch_client_srcs:.c=.o)
>  
> diff --git a/tag-util.c b/tag-util.c
> new file mode 100644
> index 0000000..287cc67
> --- /dev/null
> +++ b/tag-util.c
> @@ -0,0 +1,264 @@
> +#include "string-util.h"
> +#include "tag-util.h"
> +#include "hex-escape.h"
> +
> +struct _tag_operation_t {
> +    const char *tag;
> +    notmuch_bool_t remove;
> +};
> +
> +struct _tag_op_list_t {
> +    tag_operation_t *ops;
> +    int count;
> +    int size;
> +};
> +
> +int
> +parse_tag_line (void *ctx, char *line,
> +		tag_op_flag_t flags,
> +		char **query_string,
> +		tag_op_list_t *tag_ops)
> +{
> +    char *tok = line;
> +    size_t tok_len = 0;
> +
> +    chomp_newline (line);
> +
> +    /* remove leading space */
> +    while (*tok == ' ' || *tok == '\t')
> +	tok++;
> +
> +    /* Skip empty and comment lines. */
> +    if (*tok == '\0' || *tok == '#')
> +	    return 1;
> +
> +    tag_op_list_reset (tag_ops);
> +
> +    /* Parse tags. */
> +    while ((tok = strtok_len (tok + tok_len, " ", &tok_len)) != NULL) {
> +	notmuch_bool_t remove;
> +	char *tag;
> +
> +	/* Optional explicit end of tags marker. */
> +	if (strncmp (tok, "--", tok_len) == 0) {

I think this should be

	if (tok_len == 2 && strncmp (tok, "--", tok_len) == 0) {

to not match "--foo" as "--". At this level, "--foo" should probably be
interpreted as removal of tag "-foo", and any policy about tag contents
should be done by the caller on the returned tag ops list.

(I'm tempted to blame the above on you, but yeah it's my mistake... ;)

> +	    tok = strtok_len (tok + tok_len, " ", &tok_len);
> +	    break;
> +	}
> +
> +	/* Implicit end of tags. */
> +	if (*tok != '-' && *tok != '+')
> +	    break;
> +
> +	/* If tag is terminated by NUL, there's no query string. */
> +	if (*(tok + tok_len) == '\0') {
> +	    tok = NULL;
> +	    break;
> +	}
> +
> +	/* Terminate, and start next token after terminator. */
> +	*(tok + tok_len++) = '\0';
> +
> +	remove = (*tok == '-');
> +	tag = tok + 1;
> +
> +	/* Maybe refuse empty tags. */
> +	if (!(flags & TAG_FLAG_BE_GENEROUS) && *tag == '\0') {

I'm divided whether this deserves to be here, or whether it should be
checked by the caller. Maybe it's all right, since my first instinct
would be to just fail on lone + or - unconditionally, so this is a
special case.

> +	    tok = NULL;
> +	    break;
> +	}
> +
> +	/* Decode tag. */
> +	if (hex_decode_inplace (tag) != HEX_SUCCESS) {
> +	    tok = NULL;
> +	    break;
> +	}
> +
> +	if (tag_op_list_append (ctx, tag_ops, tag, remove))
> +	    return -1;
> +    }
> +
> +    if (tok == NULL || tag_ops->count == 0) {
> +	/* FIXME: line has been modified! */

I think we should either ditch the %s as it's bound to be completely
useless like this, or make a copy of the input line.

> +	fprintf (stderr, "Warning: Ignoring invalid input line: %s\n",
> +		 line);
> +	return 1;
> +    }
> +
> +    /* tok now points to the query string */
> +    if (hex_decode_inplace (tok) != HEX_SUCCESS) {
> +	/* FIXME: line has been modified! */

Ditto.

> +	fprintf (stderr, "Warning: Ignoring invalid input line: %s\n",
> +		 line);
> +	return 1;
> +    }
> +
> +    *query_string = tok;
> +
> +    return 0;
> +}
> +
> +static inline void
> +message_error (notmuch_message_t *message,
> +	       notmuch_status_t status,
> +	       const char *format, ...)
> +{
> +    va_list va_args;
> +
> +    va_start (va_args, format);
> +
> +    vfprintf (stderr, format, va_args);
> +    fprintf (stderr, "Message-ID: %s\n", notmuch_message_get_message_id (message));
> +    fprintf (stderr, "Status: %s\n", notmuch_status_to_string (status));
> +}
> +
> +notmuch_status_t
> +tag_op_list_apply (notmuch_message_t *message,
> +		   tag_op_list_t *list,
> +		   tag_op_flag_t flags)
> +{
> +    int i;
> +
> +    notmuch_status_t status = 0;
> +    tag_operation_t *tag_ops = list->ops;
> +
> +    status = notmuch_message_freeze (message);
> +    if (status) {
> +	message_error (message, status, "freezing message");
> +	return status;
> +    }
> +
> +    if (flags & TAG_FLAG_REMOVE_ALL) {
> +	status = notmuch_message_remove_all_tags (message);
> +	if (status) {
> +	    message_error (message, status, "removing all tags" );

Extra space before ).

> +	    return status;
> +	}
> +    }
> +
> +    for (i = 0; i < list->count; i++) {
> +	if (tag_ops[i].remove) {
> +	    status = notmuch_message_remove_tag (message, tag_ops[i].tag);
> +	    if (status) {
> +		message_error (message, status, "removing tag %s", tag_ops[i].tag);
> +		return status;
> +	    }
> +	} else {
> +	    status = notmuch_message_add_tag (message, tag_ops[i].tag);
> +	    if (status) {
> +		message_error (message, status, "adding tag %s", tag_ops[i].tag);
> +		return status;
> +	    }
> +
> +	}
> +    }
> +
> +    status = notmuch_message_thaw (message);
> +    if (status) {
> +	message_error (message, status, "thawing message");
> +	return status;
> +    }
> +
> +
> +    if (flags & TAG_FLAG_MAILDIR_SYNC) {
> +	status = notmuch_message_tags_to_maildir_flags (message);
> +	if (status) {
> +	    message_error (message, status, "synching tags to maildir");
> +	    return status;
> +	}
> +    }
> +
> +    return NOTMUCH_STATUS_SUCCESS;
> +
> +}
> +
> +
> +/* Array of tagging operations (add or remove), terminated with an
> + * empty element. Size will be increased as necessary. */

"terminated with an empty element" is obsolete as you've added .count.

> +
> +tag_op_list_t *
> +tag_op_list_create (void *ctx)
> +{
> +    tag_op_list_t *list;
> +
> +    list = talloc (ctx, tag_op_list_t);
> +    if (list == NULL)
> +	return NULL;
> +
> +    list->size = TAG_OP_LIST_INITIAL_SIZE;
> +    list->count = 0;
> +
> +    list->ops = talloc_array (ctx, tag_operation_t, list->size);
> +    if (list->ops == NULL)
> +	return NULL;
> +
> +    return list;
> +}
> +
> +
> +int
> +tag_op_list_append (void *ctx,
> +		    tag_op_list_t *list,
> +		    const char *tag,
> +		    notmuch_bool_t remove)
> +{
> +    /* Make room if current array is full.  This should be a fairly
> +     * rare case, considering the initial array size.
> +     */
> +
> +    if (list->count == list->size) {
> +	list->size *= 2;
> +	list->ops = talloc_realloc (ctx, list->ops, tag_operation_t,
> +				    list->size);
> +	if (list->ops == NULL) {
> +	    fprintf (stderr, "Out of memory.\n");
> +	    return 1;
> +	}
> +    }
> +
> +    /* add the new operation */
> +
> +    list->ops[list->count].tag = tag;
> +    list->ops[list->count].remove = remove;
> +    list->count++;
> +    return 0;
> +}
> +
> +/*
> + *   Is the i'th tag operation a remove?
> + */
> +
> +notmuch_bool_t
> +tag_op_list_isremove (const tag_op_list_t *list, size_t i)
> +{

	assert (i < list->count); ?

> +    return list->ops[i].remove;
> +}
> +
> +/*
> + * Reset a list to contain no operations
> + */
> +
> +void
> +tag_op_list_reset (tag_op_list_t *list)
> +{
> +    list->count = 0;
> +}
> +
> +/*
> + * Return the number of operations in a list
> + */
> +
> +size_t
> +tag_op_list_size (const tag_op_list_t *list)
> +{
> +    return list->count;
> +}
> +
> +/*
> + *   return the i'th tag in the list
> + */
> +
> +const char *
> +tag_op_list_tag (const tag_op_list_t *list, size_t i)
> +{

	assert (i < list->count); ?

> +    return list->ops[i].tag;
> +}
> diff --git a/tag-util.h b/tag-util.h
> new file mode 100644
> index 0000000..508806f
> --- /dev/null
> +++ b/tag-util.h
> @@ -0,0 +1,120 @@
> +#ifndef _TAG_UTIL_H
> +#define _TAG_UTIL_H
> +
> +#include "notmuch-client.h"
> +
> +typedef struct _tag_operation_t tag_operation_t;
> +typedef struct _tag_op_list_t tag_op_list_t;
> +
> +#define TAG_OP_LIST_INITIAL_SIZE 10

This is implementation details, could be in the .c file.

> +
> +/* Use powers of 2 */
> +typedef enum  { TAG_FLAG_NONE = 0,
> +		/* Operations are synced to maildir, if possible */
> +
> +		TAG_FLAG_MAILDIR_SYNC = 1,
> +
> +		/* Remove all tags from message before applying
> +		 * list */
> +
> +		TAG_FLAG_REMOVE_ALL = 2,
> +
> +		/* Don't try to avoid database operations.  Useful
> +		 * when we know that message passed needs these
> +		 *  operations. */
> +
> +		TAG_FLAG_PRE_OPTIMIZED = 4,
> +
> +		/* Accept strange tags that might be user error;
> +		   intended for use by notmuch-restore.
> +		*/
> +
> +		TAG_FLAG_BE_GENEROUS = 8} tag_op_flag_t;
> +
> +/* Parse a string of the following format:
> + *
> + * +<tag>|-<tag> [...] [--] <search-terms>
> + *
> + * Each line is interpreted similarly to "notmuch tag" command line
> + * arguments. The delimiter is one or more spaces ' '. Any characters
> + * in <tag> and <search-terms> MAY be hex encoded with %NN where NN is
> + * the hexadecimal value of the character. Any ' ' and '%' characters
> + * in <tag> and <search-terms> MUST be hex encoded (using %20 and %25,
> + * respectively). Any characters that are not part of <tag> or
> + * <search-terms> MUST NOT be hex encoded.
> + *
> + * Leading and trailing space ' ' is ignored. Empty lines and lines
> + * beginning with '#' are ignored.
> + *
> + * Returns: 0 for OK, 1 for skipped line, -1 for fatal(ish) error.
> + *
> + * Output Parameters:
> + *	ops	contains a list of tag operations
> + *	query_str the search terms.
> + */
> +int
> +parse_tag_line (void *ctx, char *line,
> +		tag_op_flag_t flags,
> +		char **query_str, tag_op_list_t *ops);
> +
> +/*
> + * Create an empty list of tag operations
> + *
> + * ctx is passed to talloc
> + */
> +
> +tag_op_list_t
> +*tag_op_list_create (void *ctx);

The * belongs at the end of previous line.

BR,
Jani.

> +
> +/*
> + * Add a tag operation (delete iff remote == TRUE) to a list.
> + * The list is expanded as necessary.
> + */
> +
> +int
> +tag_op_list_append (void *ctx,
> +		    tag_op_list_t *list,
> +		    const char *tag,
> +		    notmuch_bool_t remove);
> +
> +/*
> + * Apply a list of tag operations, in order to a message.
> + *
> + * Flags can be bitwise ORed; see enum above for possibilies.
> + */
> +
> +notmuch_status_t
> +tag_op_list_apply (notmuch_message_t *message,
> +		   tag_op_list_t *tag_ops,
> +		   tag_op_flag_t flags);
> +
> +/*
> + * Return the number of operations in a list
> + */
> +
> +size_t
> +tag_op_list_size (const tag_op_list_t *list);
> +
> +/*
> + * Reset a list to contain no operations
> + */
> +
> +void
> +tag_op_list_reset (tag_op_list_t *list);
> +
> +
> + /*
> +  *   return the i'th tag in the list
> +  */
> +
> +const char *
> +tag_op_list_tag (const tag_op_list_t *list, size_t i);
> +
> +/*
> + *   Is the i'th tag operation a remove?
> + */
> +
> +notmuch_bool_t
> +tag_op_list_isremove (const tag_op_list_t *list, size_t i);
> +
> +#endif
> -- 
> 1.7.10.4
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [Patch v2 10/17] cli: add support for batch tagging operations to "notmuch tag"

[Jani Nikula]

Hi David, overall looks good. The tag-util abstractions are paying off
nicely here.

First two high level bikesheds: 1) This could be split in two patches,
first one adding the tag-util usage to normal operation and second one
adding the batch tagging, and 2) I think it would be neater if the batch
tagging was in a separate function instead of inline in the top level
function. I'm not insisting though; we should probably just go with
this, and the latter part can be refactored later if needed.

Some minor nits inline.

BR,
Jani.

On Sat, 24 Nov 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 --stdin command line option to

--batch

> "notmuch tag". The input must consist of lines of the format:
>
> +<tag>|-<tag> [...] [--] <search-terms>
>
> Each line is interpreted similarly to "notmuch tag" command line
> arguments. The delimiter is one or more spaces ' '. Any characters in
> <tag> and <search-terms> MAY be hex encoded with %NN where NN is the
> hexadecimal value of the character. Any ' ' and '%' characters in
> <tag> and <search-terms> MUST be hex encoded (using %20 and %25,
> respectively). Any characters that are not part of <tag> or
> <search-terms> 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 |  194 +++++++++++++++++++++++++++++++++++----------------------
>  1 file changed, 118 insertions(+), 76 deletions(-)
>
> diff --git a/notmuch-tag.c b/notmuch-tag.c
> index 88d559b..8a8af0b 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 ";
>      }
>  
> @@ -116,12 +113,11 @@ _optimize_tag_query (void *ctx, const char *orig_query_string,
>   * element. */
>  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 +140,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);
>  	notmuch_message_destroy (message);
>      }
>  
> @@ -170,15 +152,17 @@ 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 synchronize_flags = TAG_FLAG_NONE;

Maybe make that tag_flags or something so we don't need to rename it if
we ever use the other flags here?

> +    notmuch_bool_t batch = FALSE;
> +    FILE *input = stdin;
> +    char *input_file_name = NULL;
> +    int i, opt_index;
> +    int ret = 0;
>  
>      /* Setup our handler for SIGINT */
>      memset (&action, 0, sizeof (struct sigaction));
> @@ -187,53 +171,76 @@ notmuch_tag_command (void *ctx, int argc, char *argv[])
>      action.sa_flags = SA_RESTART;
>      sigaction (SIGINT, &action, NULL);
>  
> -    argc--; argv++; /* skip subcommand argument */
> +    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 }
> +    };
>  
> -    /* Array of tagging operations (add or remove), terminated with an
> -     * empty element. */
> -    tag_ops = talloc_array (ctx, tag_operation_t, argc + 1);
> -    if (tag_ops == NULL) {
> -	fprintf (stderr, "Out of memory.\n");
> +    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;
> +	}
>      }
>  
> -    for (i = 0; i < argc; i++) {
> -	if (strcmp (argv[i], "--") == 0) {
> -	    i++;
> -	    break;
> +    if (batch) {
> +	if (opt_index != argc) {
> +	    fprintf (stderr, "Can't specify both cmdline and stdin!\n");
> +	    return 1;
> +	}
> +    } else {
> +	/* Array of tagging operations (add or remove), terminated with an
> +	 * empty element. */
> +	tag_ops = tag_op_list_create (ctx);
> +	if (tag_ops == NULL) {
> +	    fprintf (stderr, "Out of memory.\n");
> +	    return 1;
>  	}
> -	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;
> +
> +	for (i = opt_index; i < argc; i++) {
> +	    if (strcmp (argv[i], "--") == 0) {
> +		i++;
> +		break;
>  	    }
> -	    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;
> +	    if (argv[i][0] == '+' || argv[i][0] == '-') {
> +		/* FIXME: checks should be consistent with those in batch tagging */
> +		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_op_list_append (ctx, tag_ops,
> +				    argv[i] + 1, (argv[i][0] == '-'));
> +	    } else {
> +		break;
>  	    }
> -	    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;
> -    }
> +	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;
> +	}
>  
> -    query_string = query_string_from_args (ctx, argc - i, &argv[i]);
> +	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");
> -	return 1;
> +	if (*query_string == '\0') {
> +	    fprintf (stderr, "Error: notmuch tag requires at least one search term.\n");
> +	    return 1;
> +	}
>      }
>  
>      config = notmuch_config_open (ctx, NULL, NULL);
> @@ -244,11 +251,46 @@ 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))
> +	synchronize_flags = TAG_FLAG_MAILDIR_SYNC;

Use |= to be future compatible?

> +
> +    if (batch) {
> +
> +	char *line = NULL;
> +	size_t line_size = 0;
> +	ssize_t line_len;
> +	int ret = 0;
> +	tag_op_list_t *tag_ops;

The two above shadow the function level variables.

> +
> +	tag_ops = tag_op_list_create (ctx);
> +	if (tag_ops == NULL) {
> +	    fprintf (stderr, "Out of memory.\n");
> +	    return 1;
> +	}
>  
> -    ret = tag_query (ctx, notmuch, query_string, tag_ops, synchronize_flags);
> +	while ((line_len = getline (&line, &line_size, input)) != -1 &&
> +	       ! interrupted) {
> +
> +	    char *query_string;
> +
> +	    ret =  parse_tag_line (ctx, line, TAG_FLAG_NONE,
> +				   &query_string, tag_ops);

Extra space after =.

> +
> +	    if (ret > 0)
> +		continue;
> +
> +	    if (ret < 0 || tag_query (ctx, notmuch, query_string,
> +				      tag_ops, synchronize_flags))
> +		break;
> +	}
> +
> +	if (line)
> +	    free (line);
> +    } else
> +	ret = tag_query (ctx, notmuch, query_string, tag_ops, synchronize_flags);

I'd like braces around the else block too.

>  
>      notmuch_database_destroy (notmuch);
>  
> -    return ret;
> +    return ret || interrupted;
> +
>  }
> -- 
> 1.7.10.4

Re: [PATCH] test: add test for notmuch tag --batch option

[Jani Nikula]

On Fri, 30 Nov 2012, david@tethera.net wrote:
> From: Jani Nikula <jani@nikula.org>
>
> Basic test of functionality, along with all combinations of options.
>
> Modified extensively by David Bremner <david@tethera.net>
>
> The choice of @ as a tag is intended to be non-alphanumeric, but still
> not too much trouble in the shell and in the old sup dump/restore format.
> ---
>
> Mark: good catch. 
>
> I decided to save restore the tags rather than have
> multiple input and output files here.

But you have to be careful to not add any fancy tags as we're still
using the sup dump format here.

>
>  test/tagging |   50 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 50 insertions(+)
>
> diff --git a/test/tagging b/test/tagging
> index 980ff92..75552e8 100755
> --- a/test/tagging
> +++ b/test/tagging
> @@ -46,6 +46,56 @@ 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)"
> +

Perhaps add a comment here that the generated files are used for more
than one subtest.

> +cat > batch.in  <<EOF
> +# %20 is a space in tag

True, but what's it got to do with this bit? :p

BR,
Jani.

> ++%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 > backup.tags.$test_count
> +notmuch tag --input=batch.in
> +notmuch search \* | notmuch_search_sanitize > OUTPUT.$test_count
> +notmuch restore < backup.tags.$test_count
> +test_expect_equal_file batch.expected OUTPUT.$test_count
> +
> +test_begin_subtest "--batch --input"
> +notmuch dump > backup.tags.$test_count
> +notmuch tag --batch --input=batch.in
> +notmuch search \* | notmuch_search_sanitize > OUTPUT.$test_count
> +notmuch restore < backup.tags.$test_count
> +test_expect_equal_file batch.expected OUTPUT.$test_count
> +
> +test_begin_subtest "--batch, blank lines and comments"
> +notmuch dump | sort > EXPECTED.$test_count
> +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_count
> +test_expect_equal_file EXPECTED.$test_count OUTPUT.$test_count
> +
>  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

Re: [Patch v2 12/17] man: document notmuch tag --batch, --input options

[Jani Nikula]

On Sat, 24 Nov 2012, david@tethera.net wrote:
> From: Jani Nikula <jani@nikula.org>
>
> ---
>  man/man1/notmuch-tag.1 |   52 +++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 51 insertions(+), 1 deletion(-)
>
> diff --git a/man/man1/notmuch-tag.1 b/man/man1/notmuch-tag.1
> index 0f86582..751db7b 100644
> --- a/man/man1/notmuch-tag.1
> +++ b/man/man1/notmuch-tag.1
> @@ -4,7 +4,12 @@ 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-terms ">"
> +
> +.B notmuch tag
> +.RI "--batch"
> +.RI "[ --input=<" filename "> ]"
> +
>  
>  .SH DESCRIPTION
>  
> @@ -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 standard input. 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 "T +<" tag ">|\-<" tag "> [...] [\-\-] <" search-terms ">"

I think we should replace "<search-terms>" with "<search-term> [...]",
and ditch the plural below...

> +
> +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-terms>
> +.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

...so it would make the above description more accurately depict
reality. We don't care about hex encoding spaces between individual
search terms.

BR,
Jani.

> +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 v2 13/17] notmuch-restore: add support for input format 'batch-tag'

[Jani Nikula]

On Sat, 24 Nov 2012, david@tethera.net wrote:
> From: David Bremner <bremner@debian.org>
>
> This is the same as the batch input for notmuch tag, except by default
> it removes all tags before modifying a given message id and only "id:"
> is supported.
> ---
>  notmuch-restore.c |  199 +++++++++++++++++++++++++++++++++--------------------
>  1 file changed, 125 insertions(+), 74 deletions(-)
>
> diff --git a/notmuch-restore.c b/notmuch-restore.c
> index f03dcac..22fcd2d 100644
> --- a/notmuch-restore.c
> +++ b/notmuch-restore.c
> @@ -19,18 +19,22 @@
>   */
>  
>  #include "notmuch-client.h"
> +#include "dump-restore-private.h"
> +#include "tag-util.h"
> +#include "string-util.h"
> +
> +static volatile sig_atomic_t interrupted;
> +static regex_t regex;
>  
>  static int
> -tag_message (notmuch_database_t *notmuch, const char *message_id,
> -	     char *file_tags, notmuch_bool_t remove_all,
> -	     notmuch_bool_t synchronize_flags)
> +tag_message (unused (void *ctx),
> +	     notmuch_database_t *notmuch,
> +	     const char *message_id,
> +	     tag_op_list_t *tag_ops,
> +	     tag_op_flag_t flags)
>  {
>      notmuch_status_t status;
> -    notmuch_tags_t *db_tags;
> -    char *db_tags_str;
>      notmuch_message_t *message = NULL;
> -    const char *tag;
> -    char *next;
>      int ret = 0;
>  
>      status = notmuch_database_find_message (notmuch, message_id, &message);
> @@ -44,55 +48,63 @@ tag_message (notmuch_database_t *notmuch, const char *message_id,
>  
>      /* In order to detect missing messages, this check/optimization is
>       * intentionally done *after* first finding the message. */
> -    if (! remove_all && (file_tags == NULL || *file_tags == '\0'))
> -	goto DONE;
> -
> -    db_tags_str = NULL;
> -    for (db_tags = notmuch_message_get_tags (message);
> -	 notmuch_tags_valid (db_tags);
> -	 notmuch_tags_move_to_next (db_tags)) {
> -	tag = notmuch_tags_get (db_tags);
> -
> -	if (db_tags_str)
> -	    db_tags_str = talloc_asprintf_append (db_tags_str, " %s", tag);
> -	else
> -	    db_tags_str = talloc_strdup (message, tag);
> -    }
> +    if ( (flags & TAG_FLAG_REMOVE_ALL) || (tag_op_list_size (tag_ops)))

Extra space between ('s, and no need to wrap tag_op_list_size call in
braces.

> +	tag_op_list_apply (message, tag_ops, flags);
>  
> -    if (((file_tags == NULL || *file_tags == '\0') &&
> -	 (db_tags_str == NULL || *db_tags_str == '\0')) ||
> -	(file_tags && db_tags_str && strcmp (file_tags, db_tags_str) == 0))

This is a necessary optimization you're throwing away, but I'll get back
to this after checking the last patch in the series.

> -	goto DONE;
> +    if (message)
> +	notmuch_message_destroy (message);

message != NULL always, you can remove the if.

>  
> -    notmuch_message_freeze (message);
> +    return ret;
> +}
>  
> -    if (remove_all)
> -	notmuch_message_remove_all_tags (message);
> +static int
> +parse_sup_line (void *ctx, char *line,
> +		char **query_str, tag_op_list_t *tag_ops)
> +{
>  
> -    next = file_tags;
> -    while (next) {
> -	tag = strsep (&next, " ");
> -	if (*tag == '\0')
> -	    continue;
> -	status = notmuch_message_add_tag (message, tag);
> -	if (status) {
> -	    fprintf (stderr, "Error applying tag %s to message %s:\n",
> -		     tag, message_id);
> -	    fprintf (stderr, "%s\n", notmuch_status_to_string (status));
> -	    ret = 1;
> -	}
> +    regmatch_t match[3];
> +    char *file_tags;
> +    int rerr;
> +
> +    tag_op_list_reset (tag_ops);
> +
> +    chomp_newline (line);
> +
> +    /* Silently ignore blank lines */
> +    if (line[0] == '\0') {
> +	return 1;
> +    }
> +
> +    rerr = xregexec (&regex, line, 3, match, 0);
> +    if (rerr == REG_NOMATCH) {
> +	fprintf (stderr, "Warning: Ignoring invalid input line: %s\n",
> +		 line);
> +	return 1;
>      }
>  
> -    notmuch_message_thaw (message);
> +    *query_str = talloc_strndup (ctx, line + match[1].rm_so,
> +				 match[1].rm_eo - match[1].rm_so);
> +    file_tags = talloc_strndup (ctx, line + match[2].rm_so,
> +				match[2].rm_eo - match[2].rm_so);
>  
> -    if (synchronize_flags)
> -	notmuch_message_tags_to_maildir_flags (message);
> +    char *tok = file_tags;
> +    size_t tok_len = 0;
>  
> -  DONE:
> -    if (message)
> -	notmuch_message_destroy (message);
> +    tag_op_list_reset (tag_ops);
> +
> +    while ((tok = strtok_len (tok + tok_len, " ", &tok_len)) != NULL) {
> +
> +	if (*(tok + tok_len) != '\0') {
> +	    *(tok + tok_len) = '\0';
> +	    tok_len++;
> +	}
> +
> +	if (tag_op_list_append (ctx, tag_ops, tok, FALSE))
> +	    return -1;
> +    }
> +
> +    return 0;
>  
> -    return ret;
>  }
>  
>  int
> @@ -100,16 +112,19 @@ notmuch_restore_command (unused (void *ctx), int argc, char *argv[])
>  {
>      notmuch_config_t *config;
>      notmuch_database_t *notmuch;
> -    notmuch_bool_t synchronize_flags;
>      notmuch_bool_t accumulate = FALSE;
> +    tag_op_flag_t flags = 0;
> +    tag_op_list_t *tag_ops;
> +
>      char *input_file_name = NULL;
>      FILE *input = stdin;
>      char *line = NULL;
>      size_t line_size;
>      ssize_t line_len;
> -    regex_t regex;
> -    int rerr;
> +
> +    int ret = 0;
>      int opt_index;
> +    int input_format = DUMP_FORMAT_AUTO;
>  
>      config = notmuch_config_open (ctx, NULL, NULL);
>      if (config == NULL)
> @@ -119,9 +134,15 @@ notmuch_restore_command (unused (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))
> +	flags |= TAG_FLAG_MAILDIR_SYNC;
>  
>      notmuch_opt_desc_t options[] = {
> +	{ NOTMUCH_OPT_KEYWORD, &input_format, "format", 'f',
> +	  (notmuch_keyword_t []){ { "auto", DUMP_FORMAT_AUTO },
> +				  { "batch-tag", DUMP_FORMAT_BATCH_TAG },
> +				  { "sup", DUMP_FORMAT_SUP },
> +				  { 0, 0 } } },
>  	{ NOTMUCH_OPT_STRING, &input_file_name, "input", 'i', 0 },
>  	{ NOTMUCH_OPT_BOOLEAN,  &accumulate, "accumulate", 'a', 0 },
>  	{ 0, 0, 0, 0, 0 }
> @@ -134,6 +155,9 @@ notmuch_restore_command (unused (void *ctx), int argc, char *argv[])
>  	return 1;
>      }
>  
> +    if (! accumulate)
> +	flags |= TAG_FLAG_REMOVE_ALL;
> +
>      if (input_file_name) {
>  	input = fopen (input_file_name, "r");
>  	if (input == NULL) {
> @@ -154,35 +178,61 @@ notmuch_restore_command (unused (void *ctx), int argc, char *argv[])
>       * non-space characters for the message-id, then one or more
>       * spaces, then a list of space-separated tags as a sequence of
>       * characters within literal '(' and ')'. */
> -    if ( xregcomp (&regex,
> -		   "^([^ ]+) \\(([^)]*)\\)$",
> -		   REG_EXTENDED) )
> -	INTERNAL_ERROR ("compile time constant regex failed.");
> +    char *p;
>  
> -    while ((line_len = getline (&line, &line_size, input)) != -1) {
> -	regmatch_t match[3];
> -	char *message_id, *file_tags;
> +    line_len = getline (&line, &line_size, input);
> +    if (line_len == 0)
> +	return 0;
>  
> -	chomp_newline (line);
> +    for (p = line; *p; p++) {
> +	if (*p == '(')
> +	    input_format = DUMP_FORMAT_SUP;
> +    }
>  
> -	rerr = xregexec (&regex, line, 3, match, 0);
> -	if (rerr == REG_NOMATCH) {
> -	    fprintf (stderr, "Warning: Ignoring invalid input line: %s\n",
> -		     line);
> -	    continue;
> +    if (input_format == DUMP_FORMAT_AUTO)
> +	input_format = DUMP_FORMAT_BATCH_TAG;
> +
> +    if (input_format == DUMP_FORMAT_SUP)
> +	if ( xregcomp (&regex,
> +		       "^([^ ]+) \\(([^)]*)\\)$",
> +		       REG_EXTENDED) )
> +	    INTERNAL_ERROR ("compile time constant regex failed.");
> +
> +    tag_ops = tag_op_list_create (ctx);
> +    if (tag_ops == NULL) {
> +	fprintf (stderr, "Out of memory.\n");
> +	return 1;
> +    }

Tag op list creation could be moved earlier to keep the parsing stuff
closer together.

> +
> +    do {
> +	char *query_string;
> +
> +	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,
> +				   &query_string, tag_ops);

Extra spaces after each = above.

> +
> +	    if (ret == 0) {
> +		if ( strncmp ("id:", query_string, 3) != 0) {
> +		    fprintf (stderr, "Unsupported query: %s\n", query_string);
> +		    continue;
> +		}

There should probably be a comment somewhere here saying that everything
after id: is taken to be the message id, i.e. a query of "id:foo AND
tag:bar" will not do what you want.

An option is to add a TAG_FLAG to accept only message id as the query in
parse_tag_line. It could trim trailing whitespace (if it doesn't
already) and look for spaces in between individual search terms *before*
hex decoding the query string, and fail if it finds any, and then check
for id: prefix after hex decode.

> +		/* delete id: from front of string; tag_message expects a
> +		 * raw message-id */
> +		query_string = query_string + 3;
> +	    }
>  	}
>  
> -	message_id = xstrndup (line + match[1].rm_so,
> -			       match[1].rm_eo - match[1].rm_so);
> -	file_tags = xstrndup (line + match[2].rm_so,
> -			      match[2].rm_eo - match[2].rm_so);
> +	if (ret > 0)
> +	    continue;
>  
> -	tag_message (notmuch, message_id, file_tags, ! accumulate,
> -		     synchronize_flags);
> +	if (ret < 0 || tag_message (ctx, notmuch, query_string,
> +				    tag_ops, flags))
> +	    break;
> +
> +    }  while ((line_len = getline (&line, &line_size, input)) != -1);
>  
> -	free (message_id);
> -	free (file_tags);
> -    }
>  
>      regfree (&regex);

Only do this for sup format.

BR,
Jani.

>  
> @@ -190,8 +240,9 @@ notmuch_restore_command (unused (void *ctx), int argc, char *argv[])
>  	free (line);
>  
>      notmuch_database_destroy (notmuch);
> +
>      if (input != stdin)
>  	fclose (input);
>  
> -    return 0;
> +    return ret;
>  }
> -- 
> 1.7.10.4
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [Patch v2 16/17] notmuch-{dump, restore}.1: document new format options

[Jani Nikula]

On Sat, 24 Nov 2012, david@tethera.net wrote:
> From: David Bremner <bremner@debian.org>
>
> More or less arbitrarily, notmuch-dump.1 gets the more detailed
> description of the format.
> ---
>  man/man1/notmuch-dump.1    |   58 +++++++++++++++++++++++++++++++++++++++++++
>  man/man1/notmuch-restore.1 |   59 +++++++++++++++++++++++++++++++++++++++-----
>  2 files changed, 111 insertions(+), 6 deletions(-)
>
> diff --git a/man/man1/notmuch-dump.1 b/man/man1/notmuch-dump.1
> index 230deec..9f59905 100644
> --- a/man/man1/notmuch-dump.1
> +++ b/man/man1/notmuch-dump.1
> @@ -5,6 +5,7 @@ notmuch-dump \- creates a plain-text dump of the tags of each message
>  .SH SYNOPSIS
>  
>  .B "notmuch dump"
> +.RB  [ "\-\-format=(sup|batch-tag)"  "] [--]"
>  .RI "[ --output=<" filename "> ] [--]"
>  .RI "[ <" search-term ">...]"
>  
> @@ -19,6 +20,63 @@ recreated from the messages themselves.  The output of notmuch dump is
>  therefore the only critical thing to backup (and much more friendly to
>  incremental backup than the native database files.)
>  
> +.TP 4
> +.B \-\-format=(sup|batch-tag)
> +
> +Notmuch restore supports two plain text dump formats, both with one message-id
> +per line, followed by a list of tags.

"followed by tags" is not entirely accurate for batch-tag.

> +
> +.RS 4
> +.TP 4
> +.B sup
> +
> +The
> +.B sup
> +dump file format is specifically chosen to be
> +compatible with the format of files produced by sup-dump.
> +So if you've previously been using sup for mail, then the
> +.B "notmuch restore"
> +command provides you a way to import all of your tags (or labels as
> +sup calls them).
> +Each line has the following form

Should we deprecate the sup format for new dumps at the same time? Issue
a warning message on dumping too (unless --format is explicitly
specified), telling about the new batch-tag format. I think we should
eventually make batch-tag the default.

> +
> +.RS 4
> +.RI < message-id >
> +.B (
> +.RI < tag "> ..."
> +.B )
> +
> +with zero or more tags are separated by spaces. Note that (malformed)
> +message-ids may contain arbitrary non-null characters. Note also
> +that tags with spaces will not be correctly restored with this format.
> +
> +.RE
> +
> +.RE
> +.RS 4
> +.TP 4
> +.B batch-tag
> +
> +The
> +.B batch-tag
> +dump format is intended to more robust against malformed message-ids
> +and tags containing whitespace or non-\fBascii\fR(7) characters.
> +Each line has the form
> +
> +.RS 4
> +.RI "+<" "encoded-tag" "> " "" "+<" "encoded-tag" "> ... -- " "" " <" encoded-message-id >

Mention that the id: prefix is present in dump and required in restore.

BR,
Jani.

> +
> +where encoded means that every byte not matching the regex
> +.B [A-Za-z0-9+-_@=.:,]
> +is replace by
> +.B %nn
> +where nn is the two digit hex encoding.
> +The astute reader will notice this is a special case of the batch input
> +format for \fBnotmuch-tag\fR(1).
> +
> +.RE
> +
> +
>  With no search terms, a dump of all messages in the database will be
>  generated.  A "--" argument instructs notmuch that the
>  remaining arguments are search terms.
> diff --git a/man/man1/notmuch-restore.1 b/man/man1/notmuch-restore.1
> index 2fa8733..3860829 100644
> --- a/man/man1/notmuch-restore.1
> +++ b/man/man1/notmuch-restore.1
> @@ -6,6 +6,7 @@ notmuch-restore \- restores the tags from the given file (see notmuch dump)
>  
>  .B "notmuch restore"
>  .RB [ "--accumulate" ]
> +.RB [ "--format=(auto|batch-tag|sup)" ]
>  .RI "[ --input=<" filename "> ]"
>  
>  .SH DESCRIPTION
> @@ -15,19 +16,51 @@ Restores the tags from the given file (see
>  
>  The input is read from the given filename, if any, or from stdin.
>  
> -Note: The dump file format is specifically chosen to be
> +
> +Supported options for
> +.B restore
> +include
> +.RS 4
> +.TP 4
> +.B \-\-accumulate
> +
> +The union of the existing and new tags is applied, instead of
> +replacing each message's tags as they are read in from the dump file.
> +
> +.RE
> +.RS 4
> +.TP 4
> +.B \-\-format=(sup|batch-tag|auto)
> +
> +Notmuch restore supports two plain text dump formats, with one message-id
> +per line, and a list of tags.
> +For details of the actual formats, see \fBnotmuch-dump\fR(1).
> +
> +.RS 4
> +.TP 4
> +.B sup
> +
> +The
> +.B sup
> +dump file format is specifically chosen to be
>  compatible with the format of files produced by sup-dump.
>  So if you've previously been using sup for mail, then the
>  .B "notmuch restore"
>  command provides you a way to import all of your tags (or labels as
>  sup calls them).
>  
> -The --accumulate switch causes the union of the existing and new tags to be
> -applied, instead of replacing each message's tags as they are read in from the
> -dump file.
> +.RE
> +.RS 4
> +.TP 4
> +.B batch-tag
>  
> -See \fBnotmuch-search-terms\fR(7)
> -for details of the supported syntax for <search-terms>.
> +The
> +.B batch-tag
> +dump format is intended to more robust against malformed message-ids
> +and tags containing whitespace or non-\fBascii\fR(7) characters.  This
> +format hex-escapes all characters those outside of a small character
> +set, intended to be suitable for e.g. pathnames in most UNIX-like
> +systems.
>  
>  .B "notmuch restore"
>  updates the maildir flags according to tag changes if the
> @@ -36,6 +69,20 @@ configuration option is enabled. See \fBnotmuch-config\fR(1) for
>  details.
>  
>  .RE
> +
> +.RS 4
> +.TP 4
> +.B auto
> +
> +This option (the default) tries to guess the format from the
> +input. For correctly formed input in either supported format, this
> +heuristic, based the fact that batch-tag format contains no parentheses,
> +should be accurate.
> +
> +.RE
> +
> +.RE
> +
>  .SH SEE ALSO
>  
>  \fBnotmuch\fR(1), \fBnotmuch-config\fR(1), \fBnotmuch-count\fR(1),
> -- 
> 1.7.10.4
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [Patch v2 17/17] tag-util: optimization of tag application

[Jani Nikula]

On Sat, 24 Nov 2012, david@tethera.net wrote:
> From: David Bremner <bremner@debian.org>
>
> The idea is not to bother with restore operations if they don't change
> the set of tags. This is actually a relatively common case.
>
> In order to avoid fancy datastructures, this method is quadratic in
> the number of tags; at least on my mail database this doesn't seem to
> be a big problem.

It's bound to be slower than the original optimization using strcmp, but
that was based on a number of assumptions we can no longer make. In any
case, not doing message sync is the key optimization.

One side effect of any optimization (also the existing one) is not doing
maildir sync from tags to flags when there are no tag changes. This just
emphasizes that one should never do dump or restore with the mail store
and database out of sync. I don't think we emphasize the importance of
running notmuch new before dump and restore enough.

A few more comments below.

BR,
Jani.

> ---
>  notmuch-tag.c |    2 +-
>  tag-util.c    |   59 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 60 insertions(+), 1 deletion(-)
>
> diff --git a/notmuch-tag.c b/notmuch-tag.c
> index 8a8af0b..e4fca67 100644
> --- a/notmuch-tag.c
> +++ b/notmuch-tag.c
> @@ -140,7 +140,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);
> -	tag_op_list_apply (message, tag_ops, flags);
> +	tag_op_list_apply (message, tag_ops, flags | TAG_FLAG_PRE_OPTIMIZED);
>  	notmuch_message_destroy (message);
>      }
>  
> diff --git a/tag-util.c b/tag-util.c
> index 287cc67..2bb8355 100644
> --- a/tag-util.c
> +++ b/tag-util.c
> @@ -111,6 +111,62 @@ message_error (notmuch_message_t *message,
>      fprintf (stderr, "Status: %s\n", notmuch_status_to_string (status));
>  }
>  
> +static int
> +makes_changes (notmuch_message_t *message,
> +	       tag_op_list_t *list,
> +	       tag_op_flag_t flags)
> +{
> +
> +    int i;
> +
> +    notmuch_tags_t *tags;
> +    notmuch_bool_t changes = FALSE;
> +
> +    /* First, do we delete an existing tag? */
> +    changes = FALSE;
> +    for (tags = notmuch_message_get_tags (message);
> +	 ! changes && notmuch_tags_valid (tags);
> +	 notmuch_tags_move_to_next (tags)) {
> +	const char *cur_tag = notmuch_tags_get (tags);
> +	int last_op =  (flags & TAG_FLAG_REMOVE_ALL) ? -1 : 0;
> +
> +	for (i = 0; i < list->count; i++) {
> +	    if (strcmp (cur_tag, list->ops[i].tag) == 0) {
> +		last_op = list->ops[i].remove ? -1 : 1;
> +	    }
> +	}

If you looped from end to beginning, you could break on first match.

> +
> +	changes = (last_op == -1);
> +    }
> +    notmuch_tags_destroy (tags);
> +
> +    if (changes)
> +	return TRUE;
> +
> +    /* Now check for adding new tags */
> +    for (i = 0; i < list->count; i++) {
> +	notmuch_bool_t exists = FALSE;
> +

I think you can do:

	if (list->ops[i].remove)
	    continue;

here and remove the check on .remove below.

> +	for (tags = notmuch_message_get_tags (message);
> +	     notmuch_tags_valid (tags);
> +	     notmuch_tags_move_to_next (tags)) {
> +	    const char *cur_tag = notmuch_tags_get (tags);
> +	    if (strcmp (cur_tag, list->ops[i].tag) == 0) {
> +		exists = TRUE;
> +		break;
> +	    }
> +	}
> +	notmuch_tags_destroy (tags);
> +
> +	/* the following test is conservative, it's ok to think we
> +	 * make changes when we don't */

I think it's "too" conservative only when the input has e.g. "+foo
-foo", but I wouldn't worry about optimizing that.

> +	if ( ! exists && ! list->ops[i].remove )
> +	    return TRUE;
> +    }
> +    return FALSE;
> +
> +}
> +
>  notmuch_status_t
>  tag_op_list_apply (notmuch_message_t *message,
>  		   tag_op_list_t *list,
> @@ -121,6 +177,9 @@ tag_op_list_apply (notmuch_message_t *message,
>      notmuch_status_t status = 0;
>      tag_operation_t *tag_ops = list->ops;
>  
> +    if (! (flags & TAG_FLAG_PRE_OPTIMIZED) && ! makes_changes (message, list, flags))
> +	return NOTMUCH_STATUS_SUCCESS;
> +
>      status = notmuch_message_freeze (message);
>      if (status) {
>  	message_error (message, status, "freezing message");
> -- 
> 1.7.10.4
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: V2 of batch-tagging plus new dump/restore

[Jani Nikula]

Hi David -

Good job! And thanks for resurrecting batch tagging.

Apart from a few actual bugs and a bag of nitpicks, I think this looks
very good overall. And I don't think any of the issues are very
difficult to fix (some of the nitpicks you can just outright ignore; use
your discretion). We should definitely get this done for the next
release.

BR,
Jani.


On Sat, 24 Nov 2012, david@tethera.net wrote:
> this obsoletes: id:1353265498-3839-1-git-send-email-david@tethera.net
>
> Changes based on Mark's review
>
> - add commments for each function  in tag-util.h
>
> - inline tag_op_list_from_string in the one place it was called 
>
> - remove NULL terminator from tag_op_list (for mark); along with
>   tag_op_list_t being opaque, I thought is was ok to leave off the
>   comment on ops. But I could be convinced.
>
> - make tag_op_list_t typedef opaque
>
> - add BE_GENEROUS flag to parser. Currently this enables empty tags.
>
> - handle blank lines and comments in restore and batch tagging
>
> - fixed the commit message about "notmuch-new"
>
> Changes based on Ethan's review:
>
> - remove the dreaded . from first line of commit messages
>
> - convert if on l49 of database-test.c into explicit return on error
>
> - add comments and parens for num_tags and this_mid_len in random_corpus.c
>
> - add check for message-id's less than length 1. I wasn't sure why we
>   would disallow all message ids of length 1.
>
> - remove declaration of parse_tag_stream
>
> - explain "query_string += 3"
>
> - fix typo in notmuch-dump.1
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch