unofficial mirror of notmuch@notmuchmail.org
 help / color / mirror / code / Atom feed
* v4 of Hex Dump/Restore patches
@ 2012-12-08 15:07 david
  2012-12-08 15:07 ` [Patch v4 01/10] notmuch-dump: add --format=(batch-tag|sup) david
                   ` (11 more replies)
  0 siblings, 12 replies; 16+ messages in thread
From: david @ 2012-12-08 15:07 UTC (permalink / raw)
  To: notmuch

This obsoletes the series at
     
     id:1354843607-17980-1-git-send-email-david@tethera.net

One new patch:

[Patch v4 08/10] test/dump-restore: add test for warning/error

Other changes since the last series are as follows:

commit 16ca0f8126fafd49266ffa02534f2e9b73c03027
Author: David Bremner <bremner@debian.org>
Date:   Fri Dec 7 20:19:21 2012 -0400

    fixup for test/dump-restore

diff --git a/test/dump-restore b/test/dump-restore
index 2532cbb..b267792 100755
--- a/test/dump-restore
+++ b/test/dump-restore
@@ -212,7 +212,7 @@ test_expect_equal_file EXPECTED OUTPUT
 
 test_begin_subtest 'roundtripping random message-ids and tags'
 
-    ${TEST_DIRECTORY}/random-corpus --config-path=${NOTMUCH_CONFIG}
+    ${TEST_DIRECTORY}/random-corpus --config-path=${NOTMUCH_CONFIG} \
 			--num-messages=100
 
      notmuch dump --format=batch-tag| \

commit 9d864bd724bd2d2b26b5d52e3e2f28b4fff2046f
Author: David Bremner <bremner@debian.org>
Date:   Fri Dec 7 20:32:50 2012 -0400

    fixup for tag-util.c: put * in right palce

diff --git a/tag-util.h b/tag-util.h
index df05d72..6674674 100644
--- a/tag-util.h
+++ b/tag-util.h
@@ -62,8 +62,8 @@ parse_tag_line (void *ctx, char *line,
  * ctx is passed to talloc
  */
 
-tag_op_list_t
-*tag_op_list_create (void *ctx);
+tag_op_list_t *
+tag_op_list_create (void *ctx);
 
 /*
  * Add a tag operation (delete iff remove == TRUE) to a list.

commit d2cead797981e8992849e1d96ad95177b42290e2
Author: David Bremner <bremner@debian.org>
Date:   Fri Dec 7 20:57:52 2012 -0400

    fixup for id:87txrxxyzp.fsf@nikula.org

diff --git a/tag-util.c b/tag-util.c
index 3d54e9e..a927363 100644
--- a/tag-util.c
+++ b/tag-util.c
@@ -24,9 +24,15 @@ parse_tag_line (void *ctx, char *line,
 {
     char *tok = line;
     size_t tok_len = 0;
-    char *line_for_error = talloc_strdup (ctx, line);
+    char *line_for_error;
     int ret = 0;
 
+    line_for_error = talloc_strdup (ctx, line);
+    if (line_for_error == NULL) {
+	fprintf (stderr, "Error: out of memory\n");
+	return -1;
+    }
+
     chomp_newline (line);
 
     /* remove leading space */
@@ -58,7 +64,7 @@ parse_tag_line (void *ctx, char *line,
 
 	/* If tag is terminated by NUL, there's no query string. */
 	if (*(tok + tok_len) == '\0') {
-	    fprintf (stderr, "no query string: %s\n", line_for_error);
+	    fprintf (stderr, "Warning: no query string: %s\n", line_for_error);
 	    ret = 1;
 	    goto DONE;
 	}
@@ -71,19 +77,21 @@ parse_tag_line (void *ctx, char *line,
 
 	/* Maybe refuse empty tags. */
 	if (! (flags & TAG_FLAG_BE_GENEROUS) && *tag == '\0') {
-	    fprintf (stderr, "Error: empty tag: %s\n", line_for_error);
+	    fprintf (stderr, "Warning: empty tag: %s\n", line_for_error);
+	    ret = 1;
 	    goto DONE;
 	}
 
 	/* Decode tag. */
 	if (hex_decode_inplace (tag) != HEX_SUCCESS) {
-	    fprintf (stderr, "Hex decoding of tag %s failed\n",
+	    fprintf (stderr, "Warning: Hex decoding of tag %s failed\n",
 		     tag);
 	    ret = 1;
 	    goto DONE;
 	}
 
 	if (tag_op_list_append (ctx, tag_ops, tag, remove)) {
+	    /* diagnostics already printed */
 	    ret = -1;
 	    goto DONE;
 	}
@@ -98,7 +106,7 @@ parse_tag_line (void *ctx, char *line,
 
     /* tok now points to the query string */
     if (hex_decode_inplace (tok) != HEX_SUCCESS) {
-	fprintf (stderr, "Hex decoding of query %s failed\n",
+	fprintf (stderr, "Warning: Hex decoding of query %s failed\n",
 		 tok);
 	ret = 1;
 	goto DONE;

commit 162c20ab6d9b9eef56cb2b966a7144a61557eade
Author: David Bremner <bremner@debian.org>
Date:   Sat Dec 8 07:29:56 2012 -0400

    fixup for id:87624chmfw.fsf@qmul.ac.uk
    
    update comment

diff --git a/util/string-util.h b/util/string-util.h
index 696da40..ac7676c 100644
--- a/util/string-util.h
+++ b/util/string-util.h
@@ -3,7 +3,10 @@
 
 #include <string.h>
 
-/* like strtok(3), but without state, and doesn't modify s. usage pattern:
+/* like strtok(3), but without state, and doesn't modify s.  Return
+ * value is indicated by pointer and length, not null terminator.
+ *
+ * Usage pattern:
  *
  * const char *tok = input;
  * const char *delim = " \t";

commit 590adeec072f235861154b930bfb10bedbe37e8a
Author: David Bremner <bremner@debian.org>
Date:   Sat Dec 8 07:42:44 2012 -0400

    fixup for id:8738zghl6d.fsf@qmul.ac.uk

diff --git a/tag-util.c b/tag-util.c
index a927363..c97d240 100644
--- a/tag-util.c
+++ b/tag-util.c
@@ -98,8 +98,6 @@ parse_tag_line (void *ctx, char *line,
     }
 
     if (tok == NULL) {
-	fprintf (stderr, "Warning: Ignoring invalid input line: %s\n",
-		 line_for_error);
 	ret = 1;
 	goto DONE;
     }
@@ -113,7 +111,13 @@ parse_tag_line (void *ctx, char *line,
     }
 
     *query_string = tok;
+
   DONE:
+    if (ret != 0)
+	fprintf (stderr, "% invalid input line %s\n",
+		 ret == 1 ? "Warning: Ignoring" : "Error: Failing at",
+		 line_for_error);
+
     talloc_free (line_for_error);
     return ret;
 }

commit 6002e08eb048612f5ef4eab3205a2887e1ab3f02
Author: David Bremner <bremner@debian.org>
Date:   Sat Dec 8 07:46:39 2012 -0400

    fixup: chomp line before copy

diff --git a/tag-util.c b/tag-util.c
index c97d240..8e6b1ef 100644
--- a/tag-util.c
+++ b/tag-util.c
@@ -27,14 +27,14 @@ parse_tag_line (void *ctx, char *line,
     char *line_for_error;
     int ret = 0;
 
+    chomp_newline (line);
+
     line_for_error = talloc_strdup (ctx, line);
     if (line_for_error == NULL) {
 	fprintf (stderr, "Error: out of memory\n");
 	return -1;
     }
 
-    chomp_newline (line);
-
     /* remove leading space */
     while (*tok == ' ' || *tok == '\t')
 	tok++;

commit 9c4878bf0ad0e1f60a45c963998a20635fc234af
Author: David Bremner <bremner@debian.org>
Date:   Sat Dec 8 08:36:48 2012 -0400

    further fixup for error messages

diff --git a/tag-util.c b/tag-util.c
index 8e6b1ef..6018d49 100644
--- a/tag-util.c
+++ b/tag-util.c
@@ -41,7 +41,7 @@ parse_tag_line (void *ctx, char *line,
 
     /* Skip empty and comment lines. */
     if (*tok == '\0' || *tok == '#') {
-	ret = 1;
+	ret = 2;
 	goto DONE;
     }
 
@@ -55,6 +55,8 @@ parse_tag_line (void *ctx, char *line,
 	/* Optional explicit end of tags marker. */
 	if (tok_len == 2 && strncmp (tok, "--", tok_len) == 0) {
 	    tok = strtok_len (tok + tok_len, " ", &tok_len);
+	    if (tok == NULL)
+		fprintf (stderr, "Warning: no query string: %s\n", line_for_error);
 	    break;
 	}
 
@@ -113,8 +115,8 @@ parse_tag_line (void *ctx, char *line,
     *query_string = tok;
 
   DONE:
-    if (ret != 0)
-	fprintf (stderr, "% invalid input line %s\n",
+    if ((ret % 2) != 0)
+	fprintf (stderr, "%s invalid input line %s\n",
 		 ret == 1 ? "Warning: Ignoring" : "Error: Failing at",
 		 line_for_error);
 
diff --git a/tag-util.h b/tag-util.h
index 6674674..1d564f0 100644
--- a/tag-util.h
+++ b/tag-util.h
@@ -45,7 +45,10 @@ typedef enum {
  * 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.
+ * Returns:	0	OK,
+ *		1	skipped (invalid) line
+ *		2	skipped (valid)
+ *		-1	fatal(ish) error.
  *
  * Output Parameters:
  *	ops	contains a list of tag operations

commit 6e8a7b30e3980f77724d6005292265d49ad5241a
Author: David Bremner <bremner@debian.org>
Date:   Sat Dec 8 08:41:01 2012 -0400

    fixup for id:87wqwsg46p.fsf@qmul.ac.uk

diff --git a/notmuch-restore.c b/notmuch-restore.c
index ceec2d3..44bf88d 100644
--- a/notmuch-restore.c
+++ b/notmuch-restore.c
@@ -56,6 +56,11 @@ tag_message (unused (void *ctx),
     return ret;
 }
 
+/* Sup dump output is one line per message. We match a sequence of
+ * 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 ')'. */
+
 static int
 parse_sup_line (void *ctx, char *line,
 		char **query_str, tag_op_list_t *tag_ops)
@@ -172,11 +177,6 @@ notmuch_restore_command (unused (void *ctx), int argc, char *argv[])
 		 argv[opt_index]);
 	return 1;
     }
-
-    /* Dump output is one line per message. We match a sequence of
-     * 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 ')'. */
     char *p;
 
     line_len = getline (&line, &line_size, input);

commit ee198b0d388243f0f43fab1d7ef7acbea5bdb3e6
Author: David Bremner <bremner@debian.org>
Date:   Sat Dec 8 08:52:51 2012 -0400

    fixup: rewrite confusing for loop as while.

diff --git a/tag-util.c b/tag-util.c
index 6018d49..b68ea50 100644
--- a/tag-util.c
+++ b/tag-util.c
@@ -157,8 +157,10 @@ makes_changes (notmuch_message_t *message,
 	const char *cur_tag = notmuch_tags_get (tags);
 	int last_op =  (flags & TAG_FLAG_REMOVE_ALL) ? -1 : 0;
 
-	/* slight contortions to count down with an unsigned index */
-	for (i = list->count; i-- > 0; /*nothing*/) {
+	/* scan backwards to get last operation */
+	i = list->count;
+	while (i > 0) {
+	    i--;
 	    if (strcmp (cur_tag, list->ops[i].tag) == 0) {
 		last_op = list->ops[i].remove ? -1 : 1;
 		break;

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [Patch v4 01/10] notmuch-dump: add --format=(batch-tag|sup)
  2012-12-08 15:07 v4 of Hex Dump/Restore patches david
@ 2012-12-08 15:07 ` david
  2012-12-08 15:07 ` [Patch v4 02/10] test: add sanity check for dump --format=batch-tag david
                   ` (10 subsequent siblings)
  11 siblings, 0 replies; 16+ messages in thread
From: david @ 2012-12-08 15:07 UTC (permalink / raw)
  To: notmuch; +Cc: David Bremner

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-tag format is modelled on the syntax of notmuch tag:
- "notmuch tag" is 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
  trouble-making characters.
- It is permitted (and will be useful) for there to be no tags before
  the query.

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         |   48 ++++++++++++++++++++++++++++++++++++++++++------
 2 files changed, 55 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..d2dad40 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,56 @@ 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) {
+		    fprintf (stderr, "Error: failed to hex-encode tag %s\n",
+			     tag_str);
+		    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) {
+		    fprintf (stderr, "Error: failed to hex-encode msg-id %s\n",
+			     message_id);
+		    return 1;
+	    }
+	    fprintf (output, " -- id:%s\n", buffer);
+	}
 
 	notmuch_message_destroy (message);
     }
-- 
1.7.10.4

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [Patch v4 02/10] test: add sanity check for dump --format=batch-tag.
  2012-12-08 15:07 v4 of Hex Dump/Restore patches david
  2012-12-08 15:07 ` [Patch v4 01/10] notmuch-dump: add --format=(batch-tag|sup) david
@ 2012-12-08 15:07 ` david
  2012-12-08 15:07 ` [Patch v4 03/10] util: add string-util.[ch] david
                   ` (9 subsequent siblings)
  11 siblings, 0 replies; 16+ messages in thread
From: david @ 2012-12-08 15:07 UTC (permalink / raw)
  To: notmuch; +Cc: David Bremner

From: David Bremner <bremner@debian.org>

It's important this does not rely on restore, since it hasn't been
written yet.
---
 test/dump-restore |   13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/test/dump-restore b/test/dump-restore
index bf31266..b4c807f 100755
--- a/test/dump-restore
+++ b/test/dump-restore
@@ -85,6 +85,19 @@ 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 | sed s/^id:// > EXPECTED
+notmuch search --output=messages from:cworth | sed s/^id:// |\
+	$TEST_DIRECTORY/hex-xcode --direction=encode > OUTPUT
+test_expect_equal_file OUTPUT EXPECTED
+
+test_begin_subtest "format=batch-tag, dump sanity check."
+notmuch dump --format=sup from:cworth | cut -f1 -d' ' | \
+    sort > EXPECTED.$test_count
+notmuch dump --format=batch-tag from:cworth | sed 's/^.*-- id://' | \
+    sort > OUTPUT.$test_count
+test_expect_equal_file EXPECTED.$test_count OUTPUT.$test_count
+
 test_begin_subtest 'roundtripping random message-ids and tags'
     test_subtest_known_broken
     ${TEST_DIRECTORY}/random-corpus --config-path=${NOTMUCH_CONFIG} \
-- 
1.7.10.4

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [Patch v4 03/10] util: add string-util.[ch]
  2012-12-08 15:07 v4 of Hex Dump/Restore patches david
  2012-12-08 15:07 ` [Patch v4 01/10] notmuch-dump: add --format=(batch-tag|sup) david
  2012-12-08 15:07 ` [Patch v4 02/10] test: add sanity check for dump --format=batch-tag david
@ 2012-12-08 15:07 ` david
  2012-12-08 15:07 ` [Patch v4 04/10] tag-util.[ch]: New files for common tagging routines david
                   ` (8 subsequent siblings)
  11 siblings, 0 replies; 16+ messages in thread
From: david @ 2012-12-08 15:07 UTC (permalink / raw)
  To: notmuch; +Cc: David Bremner

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  |   22 ++++++++++++++++++++++
 3 files changed, 58 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..ac7676c
--- /dev/null
+++ b/util/string-util.h
@@ -0,0 +1,22 @@
+#ifndef _STRING_UTIL_H
+#define _STRING_UTIL_H
+
+#include <string.h>
+
+/* like strtok(3), but without state, and doesn't modify s.  Return
+ * value is indicated by pointer and length, not null terminator.
+ *
+ * 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

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [Patch v4 04/10] tag-util.[ch]: New files for common tagging routines
  2012-12-08 15:07 v4 of Hex Dump/Restore patches david
                   ` (2 preceding siblings ...)
  2012-12-08 15:07 ` [Patch v4 03/10] util: add string-util.[ch] david
@ 2012-12-08 15:07 ` david
  2012-12-08 19:22   ` Jani Nikula
  2012-12-08 15:07 ` [Patch v4 05/10] notmuch-restore: add support for input format 'batch-tag' david
                   ` (7 subsequent siblings)
  11 siblings, 1 reply; 16+ messages in thread
From: david @ 2012-12-08 15:07 UTC (permalink / raw)
  To: notmuch; +Cc: David Bremner

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     |  292 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 tag-util.h     |  122 +++++++++++++++++++++++
 3 files changed, 415 insertions(+)
 create mode 100644 tag-util.c
 create mode 100644 tag-util.h

diff --git a/Makefile.local b/Makefile.local
index 0db1713..c274f07 100644
--- a/Makefile.local
+++ b/Makefile.local
@@ -275,6 +275,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..80ebdc2
--- /dev/null
+++ b/tag-util.c
@@ -0,0 +1,292 @@
+#include <assert.h>
+#include "string-util.h"
+#include "tag-util.h"
+#include "hex-escape.h"
+
+#define TAG_OP_LIST_INITIAL_SIZE 10
+
+struct _tag_operation_t {
+    const char *tag;
+    notmuch_bool_t remove;
+};
+
+struct _tag_op_list_t {
+    tag_operation_t *ops;
+    size_t count;
+    size_t 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;
+    char *line_for_error;
+    int ret = 0;
+
+    chomp_newline (line);
+
+    line_for_error = talloc_strdup (ctx, line);
+    if (line_for_error == NULL) {
+	fprintf (stderr, "Error: out of memory\n");
+	return -1;
+    }
+
+    /* remove leading space */
+    while (*tok == ' ' || *tok == '\t')
+	tok++;
+
+    /* Skip empty and comment lines. */
+    if (*tok == '\0' || *tok == '#') {
+	ret = 2;
+	goto DONE;
+    }
+
+    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 (tok_len == 2 && strncmp (tok, "--", tok_len) == 0) {
+	    tok = strtok_len (tok + tok_len, " ", &tok_len);
+	    if (tok == NULL)
+		fprintf (stderr, "Warning: no query string: %s\n", line_for_error);
+	    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') {
+	    fprintf (stderr, "Warning: no query string: %s\n", line_for_error);
+	    ret = 1;
+	    goto DONE;
+	}
+
+	/* 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') {
+	    fprintf (stderr, "Warning: empty tag: %s\n", line_for_error);
+	    ret = 1;
+	    goto DONE;
+	}
+
+	/* Decode tag. */
+	if (hex_decode_inplace (tag) != HEX_SUCCESS) {
+	    fprintf (stderr, "Warning: Hex decoding of tag %s failed\n",
+		     tag);
+	    ret = 1;
+	    goto DONE;
+	}
+
+	if (tag_op_list_append (ctx, tag_ops, tag, remove)) {
+	    /* diagnostics already printed */
+	    ret = -1;
+	    goto DONE;
+	}
+    }
+
+    if (tok == NULL) {
+	ret = 1;
+	goto DONE;
+    }
+
+    /* tok now points to the query string */
+    if (hex_decode_inplace (tok) != HEX_SUCCESS) {
+	fprintf (stderr, "Warning: Hex decoding of query %s failed\n",
+		 tok);
+	ret = 1;
+	goto DONE;
+    }
+
+    *query_string = tok;
+
+  DONE:
+    if ((ret % 2) != 0)
+	fprintf (stderr, "%s invalid input line %s\n",
+		 ret == 1 ? "Warning: Ignoring" : "Error: Failing at",
+		 line_for_error);
+
+    talloc_free (line_for_error);
+    return ret;
+}
+
+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)
+{
+    size_t 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.  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)
+{
+    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..581207a
--- /dev/null
+++ b/tag-util.h
@@ -0,0 +1,122 @@
+#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;
+
+/* Use powers of 2 */
+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;
+
+/* 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	OK,
+ *		1	skipped (invalid) line
+ *		2	skipped (valid) line
+ *		-1	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 remove == 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 given 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

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [Patch v4 05/10] notmuch-restore: add support for input format 'batch-tag'
  2012-12-08 15:07 v4 of Hex Dump/Restore patches david
                   ` (3 preceding siblings ...)
  2012-12-08 15:07 ` [Patch v4 04/10] tag-util.[ch]: New files for common tagging routines david
@ 2012-12-08 15:07 ` david
  2012-12-08 15:07 ` [Patch v4 06/10] test: update dump-restore roundtripping test for batch-tag format david
                   ` (6 subsequent siblings)
  11 siblings, 0 replies; 16+ messages in thread
From: david @ 2012-12-08 15:07 UTC (permalink / raw)
  To: notmuch; +Cc: David Bremner

From: David Bremner <bremner@debian.org>

This can be enabled with the new --format=batch-tag command line
option to "notmuch restore". The input must consist of lines of the
format:

    +<tag>|-<tag> [...] [--] id:<msg-id>

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 <msg-id> 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.

Commit message mainly stolen from Jani's batch tagging commit, to
follow.
---
 notmuch-restore.c |  220 +++++++++++++++++++++++++++++++++--------------------
 1 file changed, 138 insertions(+), 82 deletions(-)

diff --git a/notmuch-restore.c b/notmuch-restore.c
index f03dcac..44bf88d 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,67 @@ 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;
+    notmuch_message_destroy (message);
 
-    notmuch_message_freeze (message);
+    return ret;
+}
 
-    if (remove_all)
-	notmuch_message_remove_all_tags (message);
+/* Sup dump output is one line per message. We match a sequence of
+ * 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 ')'. */
 
-    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;
-	}
+static int
+parse_sup_line (void *ctx, char *line,
+		char **query_str, tag_op_list_t *tag_ops)
+{
+
+    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;
     }
 
-    notmuch_message_thaw (message);
+    rerr = xregexec (&regex, line, 3, match, 0);
+    if (rerr == REG_NOMATCH) {
+	fprintf (stderr, "Warning: Ignoring invalid sup format line: %s\n",
+		 line);
+	return 1;
+    }
 
-    if (synchronize_flags)
-	notmuch_message_tags_to_maildir_flags (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);
 
-  DONE:
-    if (message)
-	notmuch_message_destroy (message);
+    char *tok = file_tags;
+    size_t tok_len = 0;
+
+    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 +116,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 +138,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 +159,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) {
@@ -149,49 +177,77 @@ notmuch_restore_command (unused (void *ctx), int argc, char *argv[])
 		 argv[opt_index]);
 	return 1;
     }
+    char *p;
 
-    /* Dump output is one line per message. We match a sequence of
-     * 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.");
-
-    while ((line_len = getline (&line, &line_size, input)) != -1) {
-	regmatch_t match[3];
-	char *message_id, *file_tags;
-
-	chomp_newline (line);
-
-	rerr = xregexec (&regex, line, 3, match, 0);
-	if (rerr == REG_NOMATCH) {
-	    fprintf (stderr, "Warning: Ignoring invalid input line: %s\n",
-		     line);
-	    continue;
+    line_len = getline (&line, &line_size, input);
+    if (line_len == 0)
+	return 0;
+
+    tag_ops = tag_op_list_create (ctx);
+    if (tag_ops == NULL) {
+	fprintf (stderr, "Out of memory.\n");
+	return 1;
+    }
+
+    for (p = line; (input_format == DUMP_FORMAT_AUTO) && *p; p++) {
+	if (*p == '(')
+	    input_format = DUMP_FORMAT_SUP;
+    }
+
+    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.");
+
+    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.
+		 *
+		 * XXX: Note that query string id:foo and bar will be
+		 * interpreted as a message id "foo and bar". This
+		 * should eventually be fixed to give a better error
+		 * message.
+		 */
+		query_string = query_string + 3;
+	    }
 	}
 
-	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;
 
-	free (message_id);
-	free (file_tags);
-    }
+    }  while ((line_len = getline (&line, &line_size, input)) != -1);
 
-    regfree (&regex);
+    if (input_format == DUMP_FORMAT_SUP)
+	regfree (&regex);
 
     if (line)
 	free (line);
 
     notmuch_database_destroy (notmuch);
+
     if (input != stdin)
 	fclose (input);
 
-    return 0;
+    return ret;
 }
-- 
1.7.10.4

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [Patch v4 06/10] test: update dump-restore roundtripping test for batch-tag format
  2012-12-08 15:07 v4 of Hex Dump/Restore patches david
                   ` (4 preceding siblings ...)
  2012-12-08 15:07 ` [Patch v4 05/10] notmuch-restore: add support for input format 'batch-tag' david
@ 2012-12-08 15:07 ` david
  2012-12-08 15:07 ` [Patch v4 07/10] test: second set of dump/restore --format=batch-tag tests david
                   ` (5 subsequent siblings)
  11 siblings, 0 replies; 16+ messages in thread
From: david @ 2012-12-08 15:07 UTC (permalink / raw)
  To: notmuch; +Cc: David Bremner

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 |   17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/test/dump-restore b/test/dump-restore
index b4c807f..ce81e6f 100755
--- a/test/dump-restore
+++ b/test/dump-restore
@@ -99,23 +99,22 @@ notmuch dump --format=batch-tag from:cworth | sed 's/^.*-- id://' | \
 test_expect_equal_file EXPECTED.$test_count OUTPUT.$test_count
 
 test_begin_subtest 'roundtripping random message-ids and tags'
-    test_subtest_known_broken
-    ${TEST_DIRECTORY}/random-corpus --config-path=${NOTMUCH_CONFIG} \
-			--num-messages=10
 
-     notmuch dump| \
-	 ${TEST_DIRECTORY}/hex-xcode --direction=encode| \
+    ${TEST_DIRECTORY}/random-corpus --config-path=${NOTMUCH_CONFIG}
+			--num-messages=100
+
+     notmuch dump --format=batch-tag| \
 	 sort > EXPECTED.$test_count
 
      notmuch tag +this_tag_is_very_unlikely_to_be_random '*'
 
-     ${TEST_DIRECTORY}/hex-xcode --direction=decode < EXPECTED.$test_count | \
-	 notmuch restore 2>/dev/null
+     notmuch restore --format=batch-tag < EXPECTED.$test_count
 
-     notmuch dump| \
-	 ${TEST_DIRECTORY}/hex-xcode --direction=encode| \
+     notmuch dump --format=batch-tag| \
 	 sort > OUTPUT.$test_count
 
 test_expect_equal_file EXPECTED.$test_count OUTPUT.$test_count
 
 test_done
+
+# Note the database is "poisoned" for sup format at this point.
-- 
1.7.10.4

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [Patch v4 07/10] test: second set of dump/restore --format=batch-tag tests
  2012-12-08 15:07 v4 of Hex Dump/Restore patches david
                   ` (5 preceding siblings ...)
  2012-12-08 15:07 ` [Patch v4 06/10] test: update dump-restore roundtripping test for batch-tag format david
@ 2012-12-08 15:07 ` david
  2012-12-08 15:07 ` [Patch v4 08/10] test/dump-restore: add test for warning/error messages david
                   ` (4 subsequent siblings)
  11 siblings, 0 replies; 16+ messages in thread
From: david @ 2012-12-08 15:07 UTC (permalink / raw)
  To: notmuch; +Cc: David Bremner

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 |   85 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 84 insertions(+), 1 deletion(-)

diff --git a/test/dump-restore b/test/dump-restore
index ce81e6f..7d91cca 100755
--- a/test/dump-restore
+++ b/test/dump-restore
@@ -98,9 +98,92 @@ notmuch dump --format=batch-tag from:cworth | sed 's/^.*-- id://' | \
     sort > OUTPUT.$test_count
 test_expect_equal_file EXPECTED.$test_count OUTPUT.$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'
+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
+
 test_begin_subtest 'roundtripping random message-ids and tags'
 
-    ${TEST_DIRECTORY}/random-corpus --config-path=${NOTMUCH_CONFIG}
+    ${TEST_DIRECTORY}/random-corpus --config-path=${NOTMUCH_CONFIG} \
 			--num-messages=100
 
      notmuch dump --format=batch-tag| \
-- 
1.7.10.4

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [Patch v4 08/10] test/dump-restore: add test for warning/error messages
  2012-12-08 15:07 v4 of Hex Dump/Restore patches david
                   ` (6 preceding siblings ...)
  2012-12-08 15:07 ` [Patch v4 07/10] test: second set of dump/restore --format=batch-tag tests david
@ 2012-12-08 15:07 ` david
  2012-12-08 17:54   ` Mark Walters
  2012-12-08 15:07 ` [Patch v4 09/10] notmuch-{dump, restore}.1: document new format options david
                   ` (3 subsequent siblings)
  11 siblings, 1 reply; 16+ messages in thread
From: david @ 2012-12-08 15:07 UTC (permalink / raw)
  To: notmuch; +Cc: David Bremner

From: David Bremner <bremner@debian.org>

We want to test both that error/warning messages are generated when
they should be, and not generated when they should not be. This varies
between restore and batch tagging.
---
 test/dump-restore |   29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/test/dump-restore b/test/dump-restore
index 7d91cca..b267792 100755
--- a/test/dump-restore
+++ b/test/dump-restore
@@ -181,6 +181,35 @@ notmuch restore < EXPECTED.$test_count
 notmuch dump --format=sup > OUTPUT.$test_count
 test_expect_equal_file EXPECTED.$test_count OUTPUT.$test_count
 
+test_begin_subtest 'restore: checking error messages'
+notmuch restore <<EOF 2>OUTPUT
++a +b
++c +d --
+# this is a harmless comment, do not yell about it.
+
+# the previous line was blank; also no yelling please
++%zz -- id:whatever
++e +f id:%yy
+# the next line should report an an empty tag error for batch tagging, but restore
++ +e -- id:20091117232137.GA7669@griffis1.net
+# highlight the sketchy id parsing; this should be last
++g -- id:foo and bar
+EOF
+
+cat <<EOF > EXPECTED
+Warning: no query string: +a +b
+Warning: Ignoring invalid input line +a +b
+Warning: no query string: +c +d --
+Warning: Ignoring invalid input line +c +d --
+Warning: Hex decoding of tag %zz failed
+Warning: Ignoring invalid input line +%zz -- id:whatever
+Warning: Hex decoding of query id:%yy failed
+Warning: Ignoring invalid input line +e +f id:%yy
+Warning: Cannot apply tags to missing message: foo and bar
+EOF
+
+test_expect_equal_file EXPECTED OUTPUT
+
 test_begin_subtest 'roundtripping random message-ids and tags'
 
     ${TEST_DIRECTORY}/random-corpus --config-path=${NOTMUCH_CONFIG} \
-- 
1.7.10.4

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [Patch v4 09/10] notmuch-{dump, restore}.1: document new format options
  2012-12-08 15:07 v4 of Hex Dump/Restore patches david
                   ` (7 preceding siblings ...)
  2012-12-08 15:07 ` [Patch v4 08/10] test/dump-restore: add test for warning/error messages david
@ 2012-12-08 15:07 ` david
  2012-12-08 15:07 ` [Patch v4 10/10] tag-util: optimization of tag application david
                   ` (2 subsequent siblings)
  11 siblings, 0 replies; 16+ messages in thread
From: david @ 2012-12-08 15:07 UTC (permalink / raw)
  To: notmuch; +Cc: David Bremner

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    |   59 ++++++++++++++++++++++++++++++++++++++++++++
 man/man1/notmuch-restore.1 |   59 +++++++++++++++++++++++++++++++++++++++-----
 2 files changed, 112 insertions(+), 6 deletions(-)

diff --git a/man/man1/notmuch-dump.1 b/man/man1/notmuch-dump.1
index 230deec..770b00f 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,64 @@ 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" "> ... -- " "" " id:<" 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); note that the single message-id query is
+mandatory for \fBnotmuch-restore\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..6bba628 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 each line
+specifying a message-id and a set 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

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [Patch v4 10/10] tag-util: optimization of tag application
  2012-12-08 15:07 v4 of Hex Dump/Restore patches david
                   ` (8 preceding siblings ...)
  2012-12-08 15:07 ` [Patch v4 09/10] notmuch-{dump, restore}.1: document new format options david
@ 2012-12-08 15:07 ` david
  2012-12-08 19:35 ` v4 of Hex Dump/Restore patches Jani Nikula
  2012-12-08 19:47 ` Mark Walters
  11 siblings, 0 replies; 16+ messages in thread
From: david @ 2012-12-08 15:07 UTC (permalink / raw)
  To: notmuch; +Cc: David Bremner

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.
---
 tag-util.c |   68 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 68 insertions(+)

diff --git a/tag-util.c b/tag-util.c
index 80ebdc2..b68ea50 100644
--- a/tag-util.c
+++ b/tag-util.c
@@ -138,6 +138,71 @@ 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)
+{
+
+    size_t 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;
+
+	/* scan backwards to get last operation */
+	i = list->count;
+	while (i > 0) {
+	    i--;
+	    if (strcmp (cur_tag, list->ops[i].tag) == 0) {
+		last_op = list->ops[i].remove ? -1 : 1;
+		break;
+	    }
+	}
+
+	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;
+
+	if (list->ops[i].remove)
+	    continue;
+
+	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,
+	 * in the sense it ignores cases like +foo ... -foo
+	 * but this is OK from a correctness point of view
+	 */
+	if (! exists)
+	    return TRUE;
+    }
+    return FALSE;
+
+}
+
 notmuch_status_t
 tag_op_list_apply (notmuch_message_t *message,
 		   tag_op_list_t *list,
@@ -147,6 +212,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

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [Patch v4 08/10] test/dump-restore: add test for warning/error messages
  2012-12-08 15:07 ` [Patch v4 08/10] test/dump-restore: add test for warning/error messages david
@ 2012-12-08 17:54   ` Mark Walters
  2012-12-08 18:32     ` David Bremner
  0 siblings, 1 reply; 16+ messages in thread
From: Mark Walters @ 2012-12-08 17:54 UTC (permalink / raw)
  To: david, notmuch; +Cc: David Bremner

On Sat, 08 Dec 2012, david@tethera.net wrote:
> From: David Bremner <bremner@debian.org>
>
> We want to test both that error/warning messages are generated when
> they should be, and not generated when they should not be. This varies
> between restore and batch tagging.
> ---
>  test/dump-restore |   29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
>
> diff --git a/test/dump-restore b/test/dump-restore
> index 7d91cca..b267792 100755
> --- a/test/dump-restore
> +++ b/test/dump-restore
> @@ -181,6 +181,35 @@ notmuch restore < EXPECTED.$test_count
>  notmuch dump --format=sup > OUTPUT.$test_count
>  test_expect_equal_file EXPECTED.$test_count OUTPUT.$test_count
>  
> +test_begin_subtest 'restore: checking error messages'
> +notmuch restore <<EOF 2>OUTPUT
> ++a +b
> ++c +d --
> +# this is a harmless comment, do not yell about it.
> +
> +# the previous line was blank; also no yelling please
> ++%zz -- id:whatever
> ++e +f id:%yy
> +# the next line should report an an empty tag error for batch tagging, but restore
> ++ +e -- id:20091117232137.GA7669@griffis1.net

I don't follow the comment above: do you mean "but not for restore", or
it should report an error but restore anyway or?

Best wishes

Mark

> +# highlight the sketchy id parsing; this should be last
> ++g -- id:foo and bar
> +EOF


> +
> +cat <<EOF > EXPECTED
> +Warning: no query string: +a +b
> +Warning: Ignoring invalid input line +a +b
> +Warning: no query string: +c +d --
> +Warning: Ignoring invalid input line +c +d --
> +Warning: Hex decoding of tag %zz failed
> +Warning: Ignoring invalid input line +%zz -- id:whatever
> +Warning: Hex decoding of query id:%yy failed
> +Warning: Ignoring invalid input line +e +f id:%yy
> +Warning: Cannot apply tags to missing message: foo and bar
> +EOF
> +
> +test_expect_equal_file EXPECTED OUTPUT
> +
>  test_begin_subtest 'roundtripping random message-ids and tags'
>  
>      ${TEST_DIRECTORY}/random-corpus --config-path=${NOTMUCH_CONFIG} \
> -- 
> 1.7.10.4
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Patch v4 08/10] test/dump-restore: add test for warning/error messages
  2012-12-08 17:54   ` Mark Walters
@ 2012-12-08 18:32     ` David Bremner
  0 siblings, 0 replies; 16+ messages in thread
From: David Bremner @ 2012-12-08 18:32 UTC (permalink / raw)
  To: Mark Walters, notmuch

Mark Walters <markwalters1009@gmail.com> writes:
>> ++e +f id:%yy
>> +# the next line should report an an empty tag error for batch tagging, but restore
>> ++ +e -- id:20091117232137.GA7669@griffis1.net
>
> I don't follow the comment above: do you mean "but not for restore", or
> it should report an error but restore anyway or?

the former, it should read "but not for restore"

d

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Patch v4 04/10] tag-util.[ch]: New files for common tagging routines
  2012-12-08 15:07 ` [Patch v4 04/10] tag-util.[ch]: New files for common tagging routines david
@ 2012-12-08 19:22   ` Jani Nikula
  0 siblings, 0 replies; 16+ messages in thread
From: Jani Nikula @ 2012-12-08 19:22 UTC (permalink / raw)
  To: david, notmuch; +Cc: David Bremner


Hi David, some error message bikeshedding below.

BR,
Jani.


On Sat, 08 Dec 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     |  292 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  tag-util.h     |  122 +++++++++++++++++++++++
>  3 files changed, 415 insertions(+)
>  create mode 100644 tag-util.c
>  create mode 100644 tag-util.h
>
> diff --git a/Makefile.local b/Makefile.local
> index 0db1713..c274f07 100644
> --- a/Makefile.local
> +++ b/Makefile.local
> @@ -275,6 +275,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..80ebdc2
> --- /dev/null
> +++ b/tag-util.c
> @@ -0,0 +1,292 @@
> +#include <assert.h>
> +#include "string-util.h"
> +#include "tag-util.h"
> +#include "hex-escape.h"
> +
> +#define TAG_OP_LIST_INITIAL_SIZE 10
> +
> +struct _tag_operation_t {
> +    const char *tag;
> +    notmuch_bool_t remove;
> +};
> +
> +struct _tag_op_list_t {
> +    tag_operation_t *ops;
> +    size_t count;
> +    size_t 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;
> +    char *line_for_error;
> +    int ret = 0;
> +
> +    chomp_newline (line);
> +
> +    line_for_error = talloc_strdup (ctx, line);
> +    if (line_for_error == NULL) {
> +	fprintf (stderr, "Error: out of memory\n");
> +	return -1;
> +    }
> +
> +    /* remove leading space */
> +    while (*tok == ' ' || *tok == '\t')
> +	tok++;
> +
> +    /* Skip empty and comment lines. */
> +    if (*tok == '\0' || *tok == '#') {
> +	ret = 2;
> +	goto DONE;
> +    }
> +
> +    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 (tok_len == 2 && strncmp (tok, "--", tok_len) == 0) {
> +	    tok = strtok_len (tok + tok_len, " ", &tok_len);
> +	    if (tok == NULL)
> +		fprintf (stderr, "Warning: no query string: %s\n", line_for_error);

I think you should move this warning back... (see below)

> +	    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') {
> +	    fprintf (stderr, "Warning: no query string: %s\n", line_for_error);
> +	    ret = 1;
> +	    goto DONE;
> +	}
> +
> +	/* 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') {
> +	    fprintf (stderr, "Warning: empty tag: %s\n", line_for_error);
> +	    ret = 1;
> +	    goto DONE;
> +	}
> +
> +	/* Decode tag. */
> +	if (hex_decode_inplace (tag) != HEX_SUCCESS) {
> +	    fprintf (stderr, "Warning: Hex decoding of tag %s failed\n",
> +		     tag);
> +	    ret = 1;
> +	    goto DONE;
> +	}
> +
> +	if (tag_op_list_append (ctx, tag_ops, tag, remove)) {
> +	    /* diagnostics already printed */
> +	    ret = -1;
> +	    goto DONE;
> +	}
> +    }
> +
> +    if (tok == NULL) {

...here where it was. Now you'll only get the warning for lines like:

+foo +bar --

but not for:

+foo +bar

which are only caught here. (Alternatively, you could have both print
the error message, and set ret and goto DONE also in the first case. But
I don't know if that gains us anything.)

> +	ret = 1;
> +	goto DONE;
> +    }
> +
> +    /* tok now points to the query string */
> +    if (hex_decode_inplace (tok) != HEX_SUCCESS) {
> +	fprintf (stderr, "Warning: Hex decoding of query %s failed\n",
> +		 tok);
> +	ret = 1;
> +	goto DONE;
> +    }
> +
> +    *query_string = tok;
> +
> +  DONE:
> +    if ((ret % 2) != 0)
> +	fprintf (stderr, "%s invalid input line %s\n",
> +		 ret == 1 ? "Warning: Ignoring" : "Error: Failing at",
> +		 line_for_error);

Can't resist urge to nitpick... I think the (ret % 2) is a bit
magical. I'd prefer the explicit (ret != 0 && ret != 2). Another one is
that some of the earlier warnings/errors already print line_for_error,
resulting in printing the line twice. Maybe only print line_for_error
here, and remove that bit from the messages earlier?

> +
> +    talloc_free (line_for_error);
> +    return ret;
> +}
> +
> +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)
> +{
> +    size_t 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.  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)
> +{
> +    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..581207a
> --- /dev/null
> +++ b/tag-util.h
> @@ -0,0 +1,122 @@
> +#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;
> +
> +/* Use powers of 2 */
> +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;
> +
> +/* 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	OK,
> + *		1	skipped (invalid) line
> + *		2	skipped (valid) line
> + *		-1	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 remove == 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 given 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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: v4 of Hex Dump/Restore patches
  2012-12-08 15:07 v4 of Hex Dump/Restore patches david
                   ` (9 preceding siblings ...)
  2012-12-08 15:07 ` [Patch v4 10/10] tag-util: optimization of tag application david
@ 2012-12-08 19:35 ` Jani Nikula
  2012-12-08 19:47 ` Mark Walters
  11 siblings, 0 replies; 16+ messages in thread
From: Jani Nikula @ 2012-12-08 19:35 UTC (permalink / raw)
  To: david, notmuch


Hi David, some minor bikeshedding in patch 4 (can be fixed later too),
and did not look at patches 7, 8, or 9, but otherwise the series LGTM.

BR,
Jani.


On Sat, 08 Dec 2012, david@tethera.net wrote:
> This obsoletes the series at
>      
>      id:1354843607-17980-1-git-send-email-david@tethera.net
>
> One new patch:
>
> [Patch v4 08/10] test/dump-restore: add test for warning/error
>
> Other changes since the last series are as follows:
>
> commit 16ca0f8126fafd49266ffa02534f2e9b73c03027
> Author: David Bremner <bremner@debian.org>
> Date:   Fri Dec 7 20:19:21 2012 -0400
>
>     fixup for test/dump-restore
>
> diff --git a/test/dump-restore b/test/dump-restore
> index 2532cbb..b267792 100755
> --- a/test/dump-restore
> +++ b/test/dump-restore
> @@ -212,7 +212,7 @@ test_expect_equal_file EXPECTED OUTPUT
>  
>  test_begin_subtest 'roundtripping random message-ids and tags'
>  
> -    ${TEST_DIRECTORY}/random-corpus --config-path=${NOTMUCH_CONFIG}
> +    ${TEST_DIRECTORY}/random-corpus --config-path=${NOTMUCH_CONFIG} \
>  			--num-messages=100
>  
>       notmuch dump --format=batch-tag| \
>
> commit 9d864bd724bd2d2b26b5d52e3e2f28b4fff2046f
> Author: David Bremner <bremner@debian.org>
> Date:   Fri Dec 7 20:32:50 2012 -0400
>
>     fixup for tag-util.c: put * in right palce
>
> diff --git a/tag-util.h b/tag-util.h
> index df05d72..6674674 100644
> --- a/tag-util.h
> +++ b/tag-util.h
> @@ -62,8 +62,8 @@ parse_tag_line (void *ctx, char *line,
>   * ctx is passed to talloc
>   */
>  
> -tag_op_list_t
> -*tag_op_list_create (void *ctx);
> +tag_op_list_t *
> +tag_op_list_create (void *ctx);
>  
>  /*
>   * Add a tag operation (delete iff remove == TRUE) to a list.
>
> commit d2cead797981e8992849e1d96ad95177b42290e2
> Author: David Bremner <bremner@debian.org>
> Date:   Fri Dec 7 20:57:52 2012 -0400
>
>     fixup for id:87txrxxyzp.fsf@nikula.org
>
> diff --git a/tag-util.c b/tag-util.c
> index 3d54e9e..a927363 100644
> --- a/tag-util.c
> +++ b/tag-util.c
> @@ -24,9 +24,15 @@ parse_tag_line (void *ctx, char *line,
>  {
>      char *tok = line;
>      size_t tok_len = 0;
> -    char *line_for_error = talloc_strdup (ctx, line);
> +    char *line_for_error;
>      int ret = 0;
>  
> +    line_for_error = talloc_strdup (ctx, line);
> +    if (line_for_error == NULL) {
> +	fprintf (stderr, "Error: out of memory\n");
> +	return -1;
> +    }
> +
>      chomp_newline (line);
>  
>      /* remove leading space */
> @@ -58,7 +64,7 @@ parse_tag_line (void *ctx, char *line,
>  
>  	/* If tag is terminated by NUL, there's no query string. */
>  	if (*(tok + tok_len) == '\0') {
> -	    fprintf (stderr, "no query string: %s\n", line_for_error);
> +	    fprintf (stderr, "Warning: no query string: %s\n", line_for_error);
>  	    ret = 1;
>  	    goto DONE;
>  	}
> @@ -71,19 +77,21 @@ parse_tag_line (void *ctx, char *line,
>  
>  	/* Maybe refuse empty tags. */
>  	if (! (flags & TAG_FLAG_BE_GENEROUS) && *tag == '\0') {
> -	    fprintf (stderr, "Error: empty tag: %s\n", line_for_error);
> +	    fprintf (stderr, "Warning: empty tag: %s\n", line_for_error);
> +	    ret = 1;
>  	    goto DONE;
>  	}
>  
>  	/* Decode tag. */
>  	if (hex_decode_inplace (tag) != HEX_SUCCESS) {
> -	    fprintf (stderr, "Hex decoding of tag %s failed\n",
> +	    fprintf (stderr, "Warning: Hex decoding of tag %s failed\n",
>  		     tag);
>  	    ret = 1;
>  	    goto DONE;
>  	}
>  
>  	if (tag_op_list_append (ctx, tag_ops, tag, remove)) {
> +	    /* diagnostics already printed */
>  	    ret = -1;
>  	    goto DONE;
>  	}
> @@ -98,7 +106,7 @@ parse_tag_line (void *ctx, char *line,
>  
>      /* tok now points to the query string */
>      if (hex_decode_inplace (tok) != HEX_SUCCESS) {
> -	fprintf (stderr, "Hex decoding of query %s failed\n",
> +	fprintf (stderr, "Warning: Hex decoding of query %s failed\n",
>  		 tok);
>  	ret = 1;
>  	goto DONE;
>
> commit 162c20ab6d9b9eef56cb2b966a7144a61557eade
> Author: David Bremner <bremner@debian.org>
> Date:   Sat Dec 8 07:29:56 2012 -0400
>
>     fixup for id:87624chmfw.fsf@qmul.ac.uk
>     
>     update comment
>
> diff --git a/util/string-util.h b/util/string-util.h
> index 696da40..ac7676c 100644
> --- a/util/string-util.h
> +++ b/util/string-util.h
> @@ -3,7 +3,10 @@
>  
>  #include <string.h>
>  
> -/* like strtok(3), but without state, and doesn't modify s. usage pattern:
> +/* like strtok(3), but without state, and doesn't modify s.  Return
> + * value is indicated by pointer and length, not null terminator.
> + *
> + * Usage pattern:
>   *
>   * const char *tok = input;
>   * const char *delim = " \t";
>
> commit 590adeec072f235861154b930bfb10bedbe37e8a
> Author: David Bremner <bremner@debian.org>
> Date:   Sat Dec 8 07:42:44 2012 -0400
>
>     fixup for id:8738zghl6d.fsf@qmul.ac.uk
>
> diff --git a/tag-util.c b/tag-util.c
> index a927363..c97d240 100644
> --- a/tag-util.c
> +++ b/tag-util.c
> @@ -98,8 +98,6 @@ parse_tag_line (void *ctx, char *line,
>      }
>  
>      if (tok == NULL) {
> -	fprintf (stderr, "Warning: Ignoring invalid input line: %s\n",
> -		 line_for_error);
>  	ret = 1;
>  	goto DONE;
>      }
> @@ -113,7 +111,13 @@ parse_tag_line (void *ctx, char *line,
>      }
>  
>      *query_string = tok;
> +
>    DONE:
> +    if (ret != 0)
> +	fprintf (stderr, "% invalid input line %s\n",
> +		 ret == 1 ? "Warning: Ignoring" : "Error: Failing at",
> +		 line_for_error);
> +
>      talloc_free (line_for_error);
>      return ret;
>  }
>
> commit 6002e08eb048612f5ef4eab3205a2887e1ab3f02
> Author: David Bremner <bremner@debian.org>
> Date:   Sat Dec 8 07:46:39 2012 -0400
>
>     fixup: chomp line before copy
>
> diff --git a/tag-util.c b/tag-util.c
> index c97d240..8e6b1ef 100644
> --- a/tag-util.c
> +++ b/tag-util.c
> @@ -27,14 +27,14 @@ parse_tag_line (void *ctx, char *line,
>      char *line_for_error;
>      int ret = 0;
>  
> +    chomp_newline (line);
> +
>      line_for_error = talloc_strdup (ctx, line);
>      if (line_for_error == NULL) {
>  	fprintf (stderr, "Error: out of memory\n");
>  	return -1;
>      }
>  
> -    chomp_newline (line);
> -
>      /* remove leading space */
>      while (*tok == ' ' || *tok == '\t')
>  	tok++;
>
> commit 9c4878bf0ad0e1f60a45c963998a20635fc234af
> Author: David Bremner <bremner@debian.org>
> Date:   Sat Dec 8 08:36:48 2012 -0400
>
>     further fixup for error messages
>
> diff --git a/tag-util.c b/tag-util.c
> index 8e6b1ef..6018d49 100644
> --- a/tag-util.c
> +++ b/tag-util.c
> @@ -41,7 +41,7 @@ parse_tag_line (void *ctx, char *line,
>  
>      /* Skip empty and comment lines. */
>      if (*tok == '\0' || *tok == '#') {
> -	ret = 1;
> +	ret = 2;
>  	goto DONE;
>      }
>  
> @@ -55,6 +55,8 @@ parse_tag_line (void *ctx, char *line,
>  	/* Optional explicit end of tags marker. */
>  	if (tok_len == 2 && strncmp (tok, "--", tok_len) == 0) {
>  	    tok = strtok_len (tok + tok_len, " ", &tok_len);
> +	    if (tok == NULL)
> +		fprintf (stderr, "Warning: no query string: %s\n", line_for_error);
>  	    break;
>  	}
>  
> @@ -113,8 +115,8 @@ parse_tag_line (void *ctx, char *line,
>      *query_string = tok;
>  
>    DONE:
> -    if (ret != 0)
> -	fprintf (stderr, "% invalid input line %s\n",
> +    if ((ret % 2) != 0)
> +	fprintf (stderr, "%s invalid input line %s\n",
>  		 ret == 1 ? "Warning: Ignoring" : "Error: Failing at",
>  		 line_for_error);
>  
> diff --git a/tag-util.h b/tag-util.h
> index 6674674..1d564f0 100644
> --- a/tag-util.h
> +++ b/tag-util.h
> @@ -45,7 +45,10 @@ typedef enum {
>   * 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.
> + * Returns:	0	OK,
> + *		1	skipped (invalid) line
> + *		2	skipped (valid)
> + *		-1	fatal(ish) error.
>   *
>   * Output Parameters:
>   *	ops	contains a list of tag operations
>
> commit 6e8a7b30e3980f77724d6005292265d49ad5241a
> Author: David Bremner <bremner@debian.org>
> Date:   Sat Dec 8 08:41:01 2012 -0400
>
>     fixup for id:87wqwsg46p.fsf@qmul.ac.uk
>
> diff --git a/notmuch-restore.c b/notmuch-restore.c
> index ceec2d3..44bf88d 100644
> --- a/notmuch-restore.c
> +++ b/notmuch-restore.c
> @@ -56,6 +56,11 @@ tag_message (unused (void *ctx),
>      return ret;
>  }
>  
> +/* Sup dump output is one line per message. We match a sequence of
> + * 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 ')'. */
> +
>  static int
>  parse_sup_line (void *ctx, char *line,
>  		char **query_str, tag_op_list_t *tag_ops)
> @@ -172,11 +177,6 @@ notmuch_restore_command (unused (void *ctx), int argc, char *argv[])
>  		 argv[opt_index]);
>  	return 1;
>      }
> -
> -    /* Dump output is one line per message. We match a sequence of
> -     * 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 ')'. */
>      char *p;
>  
>      line_len = getline (&line, &line_size, input);
>
> commit ee198b0d388243f0f43fab1d7ef7acbea5bdb3e6
> Author: David Bremner <bremner@debian.org>
> Date:   Sat Dec 8 08:52:51 2012 -0400
>
>     fixup: rewrite confusing for loop as while.
>
> diff --git a/tag-util.c b/tag-util.c
> index 6018d49..b68ea50 100644
> --- a/tag-util.c
> +++ b/tag-util.c
> @@ -157,8 +157,10 @@ makes_changes (notmuch_message_t *message,
>  	const char *cur_tag = notmuch_tags_get (tags);
>  	int last_op =  (flags & TAG_FLAG_REMOVE_ALL) ? -1 : 0;
>  
> -	/* slight contortions to count down with an unsigned index */
> -	for (i = list->count; i-- > 0; /*nothing*/) {
> +	/* scan backwards to get last operation */
> +	i = list->count;
> +	while (i > 0) {
> +	    i--;
>  	    if (strcmp (cur_tag, list->ops[i].tag) == 0) {
>  		last_op = list->ops[i].remove ? -1 : 1;
>  		break;
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: v4 of Hex Dump/Restore patches
  2012-12-08 15:07 v4 of Hex Dump/Restore patches david
                   ` (10 preceding siblings ...)
  2012-12-08 19:35 ` v4 of Hex Dump/Restore patches Jani Nikula
@ 2012-12-08 19:47 ` Mark Walters
  11 siblings, 0 replies; 16+ messages in thread
From: Mark Walters @ 2012-12-08 19:47 UTC (permalink / raw)
  To: david, notmuch


Aside from the minor nit with one comment this series looks good to
me. (I haven't thoroughly checked patch 8)

Best wishes

Mark

On Sat, 08 Dec 2012, david@tethera.net wrote:
> This obsoletes the series at
>      
>      id:1354843607-17980-1-git-send-email-david@tethera.net
>
> One new patch:
>
> [Patch v4 08/10] test/dump-restore: add test for warning/error
>
> Other changes since the last series are as follows:
>
> commit 16ca0f8126fafd49266ffa02534f2e9b73c03027
> Author: David Bremner <bremner@debian.org>
> Date:   Fri Dec 7 20:19:21 2012 -0400
>
>     fixup for test/dump-restore
>
> diff --git a/test/dump-restore b/test/dump-restore
> index 2532cbb..b267792 100755
> --- a/test/dump-restore
> +++ b/test/dump-restore
> @@ -212,7 +212,7 @@ test_expect_equal_file EXPECTED OUTPUT
>  
>  test_begin_subtest 'roundtripping random message-ids and tags'
>  
> -    ${TEST_DIRECTORY}/random-corpus --config-path=${NOTMUCH_CONFIG}
> +    ${TEST_DIRECTORY}/random-corpus --config-path=${NOTMUCH_CONFIG} \
>  			--num-messages=100
>  
>       notmuch dump --format=batch-tag| \
>
> commit 9d864bd724bd2d2b26b5d52e3e2f28b4fff2046f
> Author: David Bremner <bremner@debian.org>
> Date:   Fri Dec 7 20:32:50 2012 -0400
>
>     fixup for tag-util.c: put * in right palce
>
> diff --git a/tag-util.h b/tag-util.h
> index df05d72..6674674 100644
> --- a/tag-util.h
> +++ b/tag-util.h
> @@ -62,8 +62,8 @@ parse_tag_line (void *ctx, char *line,
>   * ctx is passed to talloc
>   */
>  
> -tag_op_list_t
> -*tag_op_list_create (void *ctx);
> +tag_op_list_t *
> +tag_op_list_create (void *ctx);
>  
>  /*
>   * Add a tag operation (delete iff remove == TRUE) to a list.
>
> commit d2cead797981e8992849e1d96ad95177b42290e2
> Author: David Bremner <bremner@debian.org>
> Date:   Fri Dec 7 20:57:52 2012 -0400
>
>     fixup for id:87txrxxyzp.fsf@nikula.org
>
> diff --git a/tag-util.c b/tag-util.c
> index 3d54e9e..a927363 100644
> --- a/tag-util.c
> +++ b/tag-util.c
> @@ -24,9 +24,15 @@ parse_tag_line (void *ctx, char *line,
>  {
>      char *tok = line;
>      size_t tok_len = 0;
> -    char *line_for_error = talloc_strdup (ctx, line);
> +    char *line_for_error;
>      int ret = 0;
>  
> +    line_for_error = talloc_strdup (ctx, line);
> +    if (line_for_error == NULL) {
> +	fprintf (stderr, "Error: out of memory\n");
> +	return -1;
> +    }
> +
>      chomp_newline (line);
>  
>      /* remove leading space */
> @@ -58,7 +64,7 @@ parse_tag_line (void *ctx, char *line,
>  
>  	/* If tag is terminated by NUL, there's no query string. */
>  	if (*(tok + tok_len) == '\0') {
> -	    fprintf (stderr, "no query string: %s\n", line_for_error);
> +	    fprintf (stderr, "Warning: no query string: %s\n", line_for_error);
>  	    ret = 1;
>  	    goto DONE;
>  	}
> @@ -71,19 +77,21 @@ parse_tag_line (void *ctx, char *line,
>  
>  	/* Maybe refuse empty tags. */
>  	if (! (flags & TAG_FLAG_BE_GENEROUS) && *tag == '\0') {
> -	    fprintf (stderr, "Error: empty tag: %s\n", line_for_error);
> +	    fprintf (stderr, "Warning: empty tag: %s\n", line_for_error);
> +	    ret = 1;
>  	    goto DONE;
>  	}
>  
>  	/* Decode tag. */
>  	if (hex_decode_inplace (tag) != HEX_SUCCESS) {
> -	    fprintf (stderr, "Hex decoding of tag %s failed\n",
> +	    fprintf (stderr, "Warning: Hex decoding of tag %s failed\n",
>  		     tag);
>  	    ret = 1;
>  	    goto DONE;
>  	}
>  
>  	if (tag_op_list_append (ctx, tag_ops, tag, remove)) {
> +	    /* diagnostics already printed */
>  	    ret = -1;
>  	    goto DONE;
>  	}
> @@ -98,7 +106,7 @@ parse_tag_line (void *ctx, char *line,
>  
>      /* tok now points to the query string */
>      if (hex_decode_inplace (tok) != HEX_SUCCESS) {
> -	fprintf (stderr, "Hex decoding of query %s failed\n",
> +	fprintf (stderr, "Warning: Hex decoding of query %s failed\n",
>  		 tok);
>  	ret = 1;
>  	goto DONE;
>
> commit 162c20ab6d9b9eef56cb2b966a7144a61557eade
> Author: David Bremner <bremner@debian.org>
> Date:   Sat Dec 8 07:29:56 2012 -0400
>
>     fixup for id:87624chmfw.fsf@qmul.ac.uk
>     
>     update comment
>
> diff --git a/util/string-util.h b/util/string-util.h
> index 696da40..ac7676c 100644
> --- a/util/string-util.h
> +++ b/util/string-util.h
> @@ -3,7 +3,10 @@
>  
>  #include <string.h>
>  
> -/* like strtok(3), but without state, and doesn't modify s. usage pattern:
> +/* like strtok(3), but without state, and doesn't modify s.  Return
> + * value is indicated by pointer and length, not null terminator.
> + *
> + * Usage pattern:
>   *
>   * const char *tok = input;
>   * const char *delim = " \t";
>
> commit 590adeec072f235861154b930bfb10bedbe37e8a
> Author: David Bremner <bremner@debian.org>
> Date:   Sat Dec 8 07:42:44 2012 -0400
>
>     fixup for id:8738zghl6d.fsf@qmul.ac.uk
>
> diff --git a/tag-util.c b/tag-util.c
> index a927363..c97d240 100644
> --- a/tag-util.c
> +++ b/tag-util.c
> @@ -98,8 +98,6 @@ parse_tag_line (void *ctx, char *line,
>      }
>  
>      if (tok == NULL) {
> -	fprintf (stderr, "Warning: Ignoring invalid input line: %s\n",
> -		 line_for_error);
>  	ret = 1;
>  	goto DONE;
>      }
> @@ -113,7 +111,13 @@ parse_tag_line (void *ctx, char *line,
>      }
>  
>      *query_string = tok;
> +
>    DONE:
> +    if (ret != 0)
> +	fprintf (stderr, "% invalid input line %s\n",
> +		 ret == 1 ? "Warning: Ignoring" : "Error: Failing at",
> +		 line_for_error);
> +
>      talloc_free (line_for_error);
>      return ret;
>  }
>
> commit 6002e08eb048612f5ef4eab3205a2887e1ab3f02
> Author: David Bremner <bremner@debian.org>
> Date:   Sat Dec 8 07:46:39 2012 -0400
>
>     fixup: chomp line before copy
>
> diff --git a/tag-util.c b/tag-util.c
> index c97d240..8e6b1ef 100644
> --- a/tag-util.c
> +++ b/tag-util.c
> @@ -27,14 +27,14 @@ parse_tag_line (void *ctx, char *line,
>      char *line_for_error;
>      int ret = 0;
>  
> +    chomp_newline (line);
> +
>      line_for_error = talloc_strdup (ctx, line);
>      if (line_for_error == NULL) {
>  	fprintf (stderr, "Error: out of memory\n");
>  	return -1;
>      }
>  
> -    chomp_newline (line);
> -
>      /* remove leading space */
>      while (*tok == ' ' || *tok == '\t')
>  	tok++;
>
> commit 9c4878bf0ad0e1f60a45c963998a20635fc234af
> Author: David Bremner <bremner@debian.org>
> Date:   Sat Dec 8 08:36:48 2012 -0400
>
>     further fixup for error messages
>
> diff --git a/tag-util.c b/tag-util.c
> index 8e6b1ef..6018d49 100644
> --- a/tag-util.c
> +++ b/tag-util.c
> @@ -41,7 +41,7 @@ parse_tag_line (void *ctx, char *line,
>  
>      /* Skip empty and comment lines. */
>      if (*tok == '\0' || *tok == '#') {
> -	ret = 1;
> +	ret = 2;
>  	goto DONE;
>      }
>  
> @@ -55,6 +55,8 @@ parse_tag_line (void *ctx, char *line,
>  	/* Optional explicit end of tags marker. */
>  	if (tok_len == 2 && strncmp (tok, "--", tok_len) == 0) {
>  	    tok = strtok_len (tok + tok_len, " ", &tok_len);
> +	    if (tok == NULL)
> +		fprintf (stderr, "Warning: no query string: %s\n", line_for_error);
>  	    break;
>  	}
>  
> @@ -113,8 +115,8 @@ parse_tag_line (void *ctx, char *line,
>      *query_string = tok;
>  
>    DONE:
> -    if (ret != 0)
> -	fprintf (stderr, "% invalid input line %s\n",
> +    if ((ret % 2) != 0)
> +	fprintf (stderr, "%s invalid input line %s\n",
>  		 ret == 1 ? "Warning: Ignoring" : "Error: Failing at",
>  		 line_for_error);
>  
> diff --git a/tag-util.h b/tag-util.h
> index 6674674..1d564f0 100644
> --- a/tag-util.h
> +++ b/tag-util.h
> @@ -45,7 +45,10 @@ typedef enum {
>   * 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.
> + * Returns:	0	OK,
> + *		1	skipped (invalid) line
> + *		2	skipped (valid)
> + *		-1	fatal(ish) error.
>   *
>   * Output Parameters:
>   *	ops	contains a list of tag operations
>
> commit 6e8a7b30e3980f77724d6005292265d49ad5241a
> Author: David Bremner <bremner@debian.org>
> Date:   Sat Dec 8 08:41:01 2012 -0400
>
>     fixup for id:87wqwsg46p.fsf@qmul.ac.uk
>
> diff --git a/notmuch-restore.c b/notmuch-restore.c
> index ceec2d3..44bf88d 100644
> --- a/notmuch-restore.c
> +++ b/notmuch-restore.c
> @@ -56,6 +56,11 @@ tag_message (unused (void *ctx),
>      return ret;
>  }
>  
> +/* Sup dump output is one line per message. We match a sequence of
> + * 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 ')'. */
> +
>  static int
>  parse_sup_line (void *ctx, char *line,
>  		char **query_str, tag_op_list_t *tag_ops)
> @@ -172,11 +177,6 @@ notmuch_restore_command (unused (void *ctx), int argc, char *argv[])
>  		 argv[opt_index]);
>  	return 1;
>      }
> -
> -    /* Dump output is one line per message. We match a sequence of
> -     * 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 ')'. */
>      char *p;
>  
>      line_len = getline (&line, &line_size, input);
>
> commit ee198b0d388243f0f43fab1d7ef7acbea5bdb3e6
> Author: David Bremner <bremner@debian.org>
> Date:   Sat Dec 8 08:52:51 2012 -0400
>
>     fixup: rewrite confusing for loop as while.
>
> diff --git a/tag-util.c b/tag-util.c
> index 6018d49..b68ea50 100644
> --- a/tag-util.c
> +++ b/tag-util.c
> @@ -157,8 +157,10 @@ makes_changes (notmuch_message_t *message,
>  	const char *cur_tag = notmuch_tags_get (tags);
>  	int last_op =  (flags & TAG_FLAG_REMOVE_ALL) ? -1 : 0;
>  
> -	/* slight contortions to count down with an unsigned index */
> -	for (i = list->count; i-- > 0; /*nothing*/) {
> +	/* scan backwards to get last operation */
> +	i = list->count;
> +	while (i > 0) {
> +	    i--;
>  	    if (strcmp (cur_tag, list->ops[i].tag) == 0) {
>  		last_op = list->ops[i].remove ? -1 : 1;
>  		break;
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2012-12-08 19:47 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-12-08 15:07 v4 of Hex Dump/Restore patches david
2012-12-08 15:07 ` [Patch v4 01/10] notmuch-dump: add --format=(batch-tag|sup) david
2012-12-08 15:07 ` [Patch v4 02/10] test: add sanity check for dump --format=batch-tag david
2012-12-08 15:07 ` [Patch v4 03/10] util: add string-util.[ch] david
2012-12-08 15:07 ` [Patch v4 04/10] tag-util.[ch]: New files for common tagging routines david
2012-12-08 19:22   ` Jani Nikula
2012-12-08 15:07 ` [Patch v4 05/10] notmuch-restore: add support for input format 'batch-tag' david
2012-12-08 15:07 ` [Patch v4 06/10] test: update dump-restore roundtripping test for batch-tag format david
2012-12-08 15:07 ` [Patch v4 07/10] test: second set of dump/restore --format=batch-tag tests david
2012-12-08 15:07 ` [Patch v4 08/10] test/dump-restore: add test for warning/error messages david
2012-12-08 17:54   ` Mark Walters
2012-12-08 18:32     ` David Bremner
2012-12-08 15:07 ` [Patch v4 09/10] notmuch-{dump, restore}.1: document new format options david
2012-12-08 15:07 ` [Patch v4 10/10] tag-util: optimization of tag application david
2012-12-08 19:35 ` v4 of Hex Dump/Restore patches Jani Nikula
2012-12-08 19:47 ` Mark Walters

Code repositories for project(s) associated with this public inbox

	https://yhetil.org/notmuch.git/

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).