1 Return-Path: <m.walters@qmul.ac.uk>
\r
2 X-Original-To: notmuch@notmuchmail.org
\r
3 Delivered-To: notmuch@notmuchmail.org
\r
4 Received: from localhost (localhost [127.0.0.1])
\r
5 by olra.theworths.org (Postfix) with ESMTP id 14435431FB6
\r
6 for <notmuch@notmuchmail.org>; Mon, 31 Dec 2012 04:41:38 -0800 (PST)
\r
7 X-Virus-Scanned: Debian amavisd-new at olra.theworths.org
\r
11 X-Spam-Status: No, score=-1.098 tagged_above=-999 required=5
\r
12 tests=[DKIM_ADSP_CUSTOM_MED=0.001, FREEMAIL_FROM=0.001,
\r
13 NML_ADSP_CUSTOM_MED=1.2, RCVD_IN_DNSWL_MED=-2.3] autolearn=disabled
\r
14 Received: from olra.theworths.org ([127.0.0.1])
\r
15 by localhost (olra.theworths.org [127.0.0.1]) (amavisd-new, port 10024)
\r
16 with ESMTP id gg92KVyxIuYK for <notmuch@notmuchmail.org>;
\r
17 Mon, 31 Dec 2012 04:41:37 -0800 (PST)
\r
18 Received: from mail2.qmul.ac.uk (mail2.qmul.ac.uk [138.37.6.6])
\r
19 (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits))
\r
20 (No client certificate requested)
\r
21 by olra.theworths.org (Postfix) with ESMTPS id 02686431FAF
\r
22 for <notmuch@notmuchmail.org>; Mon, 31 Dec 2012 04:41:37 -0800 (PST)
\r
23 Received: from smtp.qmul.ac.uk ([138.37.6.40])
\r
24 by mail2.qmul.ac.uk with esmtp (Exim 4.71)
\r
25 (envelope-from <m.walters@qmul.ac.uk>)
\r
26 id 1TpegO-0006m8-DK; Mon, 31 Dec 2012 12:41:34 +0000
\r
27 Received: from 188.31.19.240.threembb.co.uk ([188.31.19.240] helo=localhost)
\r
28 by smtp.qmul.ac.uk with esmtpsa (TLSv1:AES128-SHA:128) (Exim 4.69)
\r
29 (envelope-from <m.walters@qmul.ac.uk>)
\r
30 id 1TpegN-0000SS-2S; Mon, 31 Dec 2012 12:41:32 +0000
\r
31 From: Mark Walters <markwalters1009@gmail.com>
\r
32 To: Austin Clements <amdragon@MIT.EDU>, notmuch@notmuchmail.org
\r
33 Subject: Re: [PATCH v4 4/5] dump/restore: Use Xapian queries for batch-tag
\r
35 In-Reply-To: <1356936162-2589-5-git-send-email-amdragon@mit.edu>
\r
36 References: <1356936162-2589-1-git-send-email-amdragon@mit.edu>
\r
37 <1356936162-2589-5-git-send-email-amdragon@mit.edu>
\r
38 User-Agent: Notmuch/0.14+236~g1d0044f (http://notmuchmail.org) Emacs/23.4.1
\r
39 (x86_64-pc-linux-gnu)
\r
40 Date: Mon, 31 Dec 2012 12:41:37 +0000
\r
41 Message-ID: <87zk0utmv2.fsf@qmul.ac.uk>
\r
43 Content-Type: text/plain; charset=us-ascii
\r
44 X-Sender-Host-Address: 188.31.19.240
\r
45 X-QM-SPAM-Info: Sender has good ham record. :)
\r
46 X-QM-Body-MD5: 7c47a787259cd84677ab306e72d8c75a (of first 20000 bytes)
\r
47 X-SpamAssassin-Score: -1.8
\r
48 X-SpamAssassin-SpamBar: -
\r
49 X-SpamAssassin-Report: The QM spam filters have analysed this message to
\r
51 spam. We require at least 5.0 points to mark a message as spam.
\r
52 This message scored -1.8 points.
\r
53 Summary of the scoring:
\r
54 * -2.3 RCVD_IN_DNSWL_MED RBL: Sender listed at http://www.dnswl.org/,
\r
56 * [138.37.6.40 listed in list.dnswl.org]
\r
57 * 0.0 FREEMAIL_FROM Sender email is commonly abused enduser mail
\r
58 provider * (markwalters1009[at]gmail.com)
\r
59 * -0.0 T_RP_MATCHES_RCVD Envelope sender domain matches handover relay
\r
61 * 0.5 AWL AWL: From: address is in the auto white-list
\r
62 X-QM-Scan-Virus: ClamAV says the message is clean
\r
63 Cc: tomi.ollila@iki.fi
\r
64 X-BeenThere: notmuch@notmuchmail.org
\r
65 X-Mailman-Version: 2.1.13
\r
67 List-Id: "Use and development of the notmuch mail system."
\r
68 <notmuch.notmuchmail.org>
\r
69 List-Unsubscribe: <http://notmuchmail.org/mailman/options/notmuch>,
\r
70 <mailto:notmuch-request@notmuchmail.org?subject=unsubscribe>
\r
71 List-Archive: <http://notmuchmail.org/pipermail/notmuch>
\r
72 List-Post: <mailto:notmuch@notmuchmail.org>
\r
73 List-Help: <mailto:notmuch-request@notmuchmail.org?subject=help>
\r
74 List-Subscribe: <http://notmuchmail.org/mailman/listinfo/notmuch>,
\r
75 <mailto:notmuch-request@notmuchmail.org?subject=subscribe>
\r
76 X-List-Received-Date: Mon, 31 Dec 2012 12:41:38 -0000
\r
78 On Mon, 31 Dec 2012, Austin Clements <amdragon@MIT.EDU> wrote:
\r
79 > This switches the new batch-tag format away from using a home-grown
\r
80 > hex-encoding scheme for message IDs in the dump to simply using Xapian
\r
81 > queries with Xapian quoting syntax.
\r
83 > This has a variety of advantages beyond presenting a cleaner and more
\r
84 > consistent interface. Foremost is that it will dramatically simplify
\r
85 > the quoting for batch tagging, which shares the same input format.
\r
86 > While the hex-encoding is no better or worse for the simple ID queries
\r
87 > used by dump/restore, it becomes onerous for general-purpose queries
\r
88 > used in batch tagging. It also better handles strange cases like
\r
89 > "id:foo and bar", since this is no longer syntactically valid.
\r
91 This series as a whole looks good to me modulo the allocation query for
\r
92 parse_boolean_term and a couple of trivial points below.
\r
95 > notmuch-dump.c | 9 +++++----
\r
96 > notmuch-restore.c | 22 ++++++++++------------
\r
97 > tag-util.c | 6 ------
\r
98 > test/dump-restore | 14 ++++++++------
\r
99 > 4 files changed, 23 insertions(+), 28 deletions(-)
\r
101 > diff --git a/notmuch-dump.c b/notmuch-dump.c
\r
102 > index 29d79da..bf01a39 100644
\r
103 > --- a/notmuch-dump.c
\r
104 > +++ b/notmuch-dump.c
\r
105 > @@ -20,6 +20,7 @@
\r
107 > #include "notmuch-client.h"
\r
108 > #include "dump-restore-private.h"
\r
109 > +#include "string-util.h"
\r
112 > notmuch_dump_command (unused (void *ctx), int argc, char *argv[])
\r
113 > @@ -141,13 +142,13 @@ notmuch_dump_command (unused (void *ctx), int argc, char *argv[])
\r
114 > fprintf (stderr, "Error: cannot dump message id containing line break: %s\n", message_id);
\r
117 > - if (hex_encode (notmuch, message_id,
\r
118 > - &buffer, &buffer_size) != HEX_SUCCESS) {
\r
119 > - fprintf (stderr, "Error: failed to hex-encode msg-id %s\n",
\r
120 > + if (make_boolean_term (notmuch, "id", message_id,
\r
121 > + &buffer, &buffer_size)) {
\r
122 > + fprintf (stderr, "Error: failed to quote message id %s\n",
\r
126 > - fprintf (output, " -- id:%s\n", buffer);
\r
127 > + fprintf (output, " -- %s\n", buffer);
\r
130 > notmuch_message_destroy (message);
\r
131 > diff --git a/notmuch-restore.c b/notmuch-restore.c
\r
132 > index 9ed9b51..77a4c27 100644
\r
133 > --- a/notmuch-restore.c
\r
134 > +++ b/notmuch-restore.c
\r
135 > @@ -207,7 +207,7 @@ notmuch_restore_command (unused (void *ctx), int argc, char *argv[])
\r
136 > INTERNAL_ERROR ("compile time constant regex failed.");
\r
139 > - char *query_string;
\r
140 > + char *query_string, *prefix, *term;
\r
142 > if (line_ctx != NULL)
\r
143 > talloc_free (line_ctx);
\r
144 > @@ -220,19 +220,17 @@ notmuch_restore_command (unused (void *ctx), int argc, char *argv[])
\r
145 > &query_string, tag_ops);
\r
148 > - if (strncmp ("id:", query_string, 3) != 0) {
\r
149 > - fprintf (stderr, "Warning: unsupported query: %s\n", query_string);
\r
150 > + ret = parse_boolean_term (line_ctx, query_string,
\r
151 > + &prefix, &term);
\r
153 > + fprintf (stderr, "Warning: cannot parse query: %s\n",
\r
156 > + } else if (strcmp ("id", prefix) != 0) {
\r
157 > + fprintf (stderr, "Warning: not an id query: %s\n", query_string);
\r
160 I think it would be worth adding "skipping" or something similar to this
\r
161 fprintf as it may not be obvious whether we warn but tag anyway or warn
\r
162 and skip. Perhaps also add it to the previous one but there it is
\r
163 obvious we can't do anything but skip.
\r
166 > - /* delete id: from front of string; tag_message
\r
167 > - * expects a raw message-id.
\r
169 > - * XXX: Note that query string id:foo and bar will be
\r
170 > - * interpreted as a message id "foo and bar". This
\r
171 > - * should eventually be fixed to give a better error
\r
174 > - query_string = query_string + 3;
\r
175 > + query_string = term;
\r
179 > diff --git a/tag-util.c b/tag-util.c
\r
180 > index 705b7ba..e4e5dda 100644
\r
183 > @@ -124,12 +124,6 @@ parse_tag_line (void *ctx, char *line,
\r
186 > /* tok now points to the query string */
\r
187 > - if (hex_decode_inplace (tok) != HEX_SUCCESS) {
\r
188 > - ret = line_error (TAG_PARSE_INVALID, line_for_error,
\r
189 > - "hex decoding of query %s failed", tok);
\r
193 > *query_string = tok;
\r
196 > diff --git a/test/dump-restore b/test/dump-restore
\r
197 > index 6a989b6..f9ae5b3 100755
\r
198 > --- a/test/dump-restore
\r
199 > +++ b/test/dump-restore
\r
200 > @@ -195,23 +195,25 @@ a
\r
202 > # the previous line was blank; also no yelling please
\r
203 > +%zz -- id:whatever
\r
208 It might be worth adding some more test lines here to test the various
\r
209 paths in parse_boolean_term: along the lines of
\r
210 +e -- id:some)stuff
\r
211 +e -- id:some"stuff
\r
212 +e -- id:some stuff
\r
213 +e -- id:"a_message_id_with""_a_quote"
\r
214 +e -- id:"a message id with spaces"
\r
216 One other thing that is noticeable from the errors is that most of the
\r
217 rest of the errors are very informative but the parse_boolean_term one
\r
218 is relatively uninformative: it just says we cannot parse the id even
\r
219 though we know rather more about what the error is (trailing text, no
\r
220 closing quote, illegal character in an unquoted id etc). I am happy with
\r
221 it how it is but perhaps David Bremner might like to comment?
\r
230 > # the next non-comment line should report an an empty tag error for
\r
231 > # batch tagging, but not for restore
\r
232 > + +e -- id:20091117232137.GA7669@griffis1.net
\r
233 > -# highlight the sketchy id parsing; this should be last
\r
234 > -+g -- id:foo and bar
\r
235 > +# valid id, but warning about missing message
\r
236 > ++e id:missing_message_id
\r
239 > cat <<EOF > EXPECTED
\r
240 > -Warning: unsupported query: a
\r
241 > +Warning: cannot parse query: a
\r
242 > Warning: no query string [+0]
\r
243 > Warning: no query string [+a +b]
\r
244 > Warning: missing query string [+a +b ]
\r
245 > Warning: no query string after -- [+c +d --]
\r
246 > Warning: hex decoding of tag %zz failed [+%zz -- id:whatever]
\r
247 > -Warning: hex decoding of query id:%yy failed [+e +f id:%yy]
\r
248 > -Warning: cannot apply tags to missing message: foo and bar
\r
249 > +Warning: cannot parse query: id:"
\r
250 > +Warning: not an id query: tag:abc
\r
251 > +Warning: cannot apply tags to missing message: missing_message_id
\r
254 > test_expect_equal_file EXPECTED OUTPUT
\r
projects / notmuch-archives.git / blob