From 79209d2e5bfca6854b49468a63ab05c64a6597dc Mon Sep 17 00:00:00 2001 From: Mark Walters Date: Mon, 31 Dec 2012 12:41:37 +0000 Subject: [PATCH] Re: [PATCH v4 4/5] dump/restore: Use Xapian queries for batch-tag format --- bf/f9f7ccc8a6c3ca04f29e3f022d20ab173d020a | 256 ++++++++++++++++++++++ 1 file changed, 256 insertions(+) create mode 100644 bf/f9f7ccc8a6c3ca04f29e3f022d20ab173d020a diff --git a/bf/f9f7ccc8a6c3ca04f29e3f022d20ab173d020a b/bf/f9f7ccc8a6c3ca04f29e3f022d20ab173d020a new file mode 100644 index 000000000..2e6e5785e --- /dev/null +++ b/bf/f9f7ccc8a6c3ca04f29e3f022d20ab173d020a @@ -0,0 +1,256 @@ +Return-Path: +X-Original-To: notmuch@notmuchmail.org +Delivered-To: notmuch@notmuchmail.org +Received: from localhost (localhost [127.0.0.1]) + by olra.theworths.org (Postfix) with ESMTP id 14435431FB6 + for ; Mon, 31 Dec 2012 04:41:38 -0800 (PST) +X-Virus-Scanned: Debian amavisd-new at olra.theworths.org +X-Spam-Flag: NO +X-Spam-Score: -1.098 +X-Spam-Level: +X-Spam-Status: No, score=-1.098 tagged_above=-999 required=5 + tests=[DKIM_ADSP_CUSTOM_MED=0.001, FREEMAIL_FROM=0.001, + NML_ADSP_CUSTOM_MED=1.2, RCVD_IN_DNSWL_MED=-2.3] autolearn=disabled +Received: from olra.theworths.org ([127.0.0.1]) + by localhost (olra.theworths.org [127.0.0.1]) (amavisd-new, port 10024) + with ESMTP id gg92KVyxIuYK for ; + Mon, 31 Dec 2012 04:41:37 -0800 (PST) +Received: from mail2.qmul.ac.uk (mail2.qmul.ac.uk [138.37.6.6]) + (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) + (No client certificate requested) + by olra.theworths.org (Postfix) with ESMTPS id 02686431FAF + for ; Mon, 31 Dec 2012 04:41:37 -0800 (PST) +Received: from smtp.qmul.ac.uk ([138.37.6.40]) + by mail2.qmul.ac.uk with esmtp (Exim 4.71) + (envelope-from ) + id 1TpegO-0006m8-DK; Mon, 31 Dec 2012 12:41:34 +0000 +Received: from 188.31.19.240.threembb.co.uk ([188.31.19.240] helo=localhost) + by smtp.qmul.ac.uk with esmtpsa (TLSv1:AES128-SHA:128) (Exim 4.69) + (envelope-from ) + id 1TpegN-0000SS-2S; Mon, 31 Dec 2012 12:41:32 +0000 +From: Mark Walters +To: Austin Clements , notmuch@notmuchmail.org +Subject: Re: [PATCH v4 4/5] dump/restore: Use Xapian queries for batch-tag + format +In-Reply-To: <1356936162-2589-5-git-send-email-amdragon@mit.edu> +References: <1356936162-2589-1-git-send-email-amdragon@mit.edu> + <1356936162-2589-5-git-send-email-amdragon@mit.edu> +User-Agent: Notmuch/0.14+236~g1d0044f (http://notmuchmail.org) Emacs/23.4.1 + (x86_64-pc-linux-gnu) +Date: Mon, 31 Dec 2012 12:41:37 +0000 +Message-ID: <87zk0utmv2.fsf@qmul.ac.uk> +MIME-Version: 1.0 +Content-Type: text/plain; charset=us-ascii +X-Sender-Host-Address: 188.31.19.240 +X-QM-SPAM-Info: Sender has good ham record. :) +X-QM-Body-MD5: 7c47a787259cd84677ab306e72d8c75a (of first 20000 bytes) +X-SpamAssassin-Score: -1.8 +X-SpamAssassin-SpamBar: - +X-SpamAssassin-Report: The QM spam filters have analysed this message to + determine if it is + spam. We require at least 5.0 points to mark a message as spam. + This message scored -1.8 points. + Summary of the scoring: + * -2.3 RCVD_IN_DNSWL_MED RBL: Sender listed at http://www.dnswl.org/, + * medium trust + * [138.37.6.40 listed in list.dnswl.org] + * 0.0 FREEMAIL_FROM Sender email is commonly abused enduser mail + provider * (markwalters1009[at]gmail.com) + * -0.0 T_RP_MATCHES_RCVD Envelope sender domain matches handover relay + * domain + * 0.5 AWL AWL: From: address is in the auto white-list +X-QM-Scan-Virus: ClamAV says the message is clean +Cc: tomi.ollila@iki.fi +X-BeenThere: notmuch@notmuchmail.org +X-Mailman-Version: 2.1.13 +Precedence: list +List-Id: "Use and development of the notmuch mail system." + +List-Unsubscribe: , + +List-Archive: +List-Post: +List-Help: +List-Subscribe: , + +X-List-Received-Date: Mon, 31 Dec 2012 12:41:38 -0000 + +On Mon, 31 Dec 2012, Austin Clements wrote: +> This switches the new batch-tag format away from using a home-grown +> hex-encoding scheme for message IDs in the dump to simply using Xapian +> queries with Xapian quoting syntax. +> +> This has a variety of advantages beyond presenting a cleaner and more +> consistent interface. Foremost is that it will dramatically simplify +> the quoting for batch tagging, which shares the same input format. +> While the hex-encoding is no better or worse for the simple ID queries +> used by dump/restore, it becomes onerous for general-purpose queries +> used in batch tagging. It also better handles strange cases like +> "id:foo and bar", since this is no longer syntactically valid. + +This series as a whole looks good to me modulo the allocation query for +parse_boolean_term and a couple of trivial points below. + +> --- +> notmuch-dump.c | 9 +++++---- +> notmuch-restore.c | 22 ++++++++++------------ +> tag-util.c | 6 ------ +> test/dump-restore | 14 ++++++++------ +> 4 files changed, 23 insertions(+), 28 deletions(-) +> +> diff --git a/notmuch-dump.c b/notmuch-dump.c +> index 29d79da..bf01a39 100644 +> --- a/notmuch-dump.c +> +++ b/notmuch-dump.c +> @@ -20,6 +20,7 @@ +> +> #include "notmuch-client.h" +> #include "dump-restore-private.h" +> +#include "string-util.h" +> +> int +> notmuch_dump_command (unused (void *ctx), int argc, char *argv[]) +> @@ -141,13 +142,13 @@ notmuch_dump_command (unused (void *ctx), int argc, char *argv[]) +> fprintf (stderr, "Error: cannot dump message id containing line break: %s\n", message_id); +> return 1; +> } +> - if (hex_encode (notmuch, message_id, +> - &buffer, &buffer_size) != HEX_SUCCESS) { +> - fprintf (stderr, "Error: failed to hex-encode msg-id %s\n", +> + if (make_boolean_term (notmuch, "id", message_id, +> + &buffer, &buffer_size)) { +> + fprintf (stderr, "Error: failed to quote message id %s\n", +> message_id); +> return 1; +> } +> - fprintf (output, " -- id:%s\n", buffer); +> + fprintf (output, " -- %s\n", buffer); +> } +> +> notmuch_message_destroy (message); +> diff --git a/notmuch-restore.c b/notmuch-restore.c +> index 9ed9b51..77a4c27 100644 +> --- a/notmuch-restore.c +> +++ b/notmuch-restore.c +> @@ -207,7 +207,7 @@ notmuch_restore_command (unused (void *ctx), int argc, char *argv[]) +> INTERNAL_ERROR ("compile time constant regex failed."); +> +> do { +> - char *query_string; +> + char *query_string, *prefix, *term; +> +> if (line_ctx != NULL) +> talloc_free (line_ctx); +> @@ -220,19 +220,17 @@ notmuch_restore_command (unused (void *ctx), int argc, char *argv[]) +> &query_string, tag_ops); +> +> if (ret == 0) { +> - if (strncmp ("id:", query_string, 3) != 0) { +> - fprintf (stderr, "Warning: unsupported query: %s\n", query_string); +> + ret = parse_boolean_term (line_ctx, query_string, +> + &prefix, &term); +> + if (ret) { +> + fprintf (stderr, "Warning: cannot parse query: %s\n", +> + query_string); +> + continue; +> + } else if (strcmp ("id", prefix) != 0) { +> + fprintf (stderr, "Warning: not an id query: %s\n", query_string); +> continue; + +I think it would be worth adding "skipping" or something similar to this +fprintf as it may not be obvious whether we warn but tag anyway or warn +and skip. Perhaps also add it to the previous one but there it is +obvious we can't do anything but skip. + +> } +> - /* 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; +> + query_string = term; +> } +> } +> +> diff --git a/tag-util.c b/tag-util.c +> index 705b7ba..e4e5dda 100644 +> --- a/tag-util.c +> +++ b/tag-util.c +> @@ -124,12 +124,6 @@ parse_tag_line (void *ctx, char *line, +> } +> +> /* tok now points to the query string */ +> - if (hex_decode_inplace (tok) != HEX_SUCCESS) { +> - ret = line_error (TAG_PARSE_INVALID, line_for_error, +> - "hex decoding of query %s failed", tok); +> - goto DONE; +> - } +> - +> *query_string = tok; +> +> DONE: +> diff --git a/test/dump-restore b/test/dump-restore +> index 6a989b6..f9ae5b3 100755 +> --- a/test/dump-restore +> +++ b/test/dump-restore +> @@ -195,23 +195,25 @@ a +> +> # the previous line was blank; also no yelling please +> +%zz -- id:whatever +> -+e +f id:%yy +> ++e +f id:" +> ++e +f tag:abc + +It might be worth adding some more test lines here to test the various +paths in parse_boolean_term: along the lines of ++e -- id:some)stuff ++e -- id:some"stuff ++e -- id:some stuff ++e -- id:"a_message_id_with""_a_quote" ++e -- id:"a message id with spaces" + +One other thing that is noticeable from the errors is that most of the +rest of the errors are very informative but the parse_boolean_term one +is relatively uninformative: it just says we cannot parse the id even +though we know rather more about what the error is (trailing text, no +closing quote, illegal character in an unquoted id etc). I am happy with +it how it is but perhaps David Bremner might like to comment? + +Best wishes + +Mark + + + + +> # the next non-comment line should report an an empty tag error for +> # batch tagging, but not for restore +> + +e -- id:20091117232137.GA7669@griffis1.net +> -# highlight the sketchy id parsing; this should be last +> -+g -- id:foo and bar +> +# valid id, but warning about missing message +> ++e id:missing_message_id +> EOF +> +> cat < EXPECTED +> -Warning: unsupported query: a +> +Warning: cannot parse query: a +> Warning: no query string [+0] +> Warning: no query string [+a +b] +> Warning: missing query string [+a +b ] +> Warning: no query string after -- [+c +d --] +> Warning: hex decoding of tag %zz failed [+%zz -- id:whatever] +> -Warning: hex decoding of query id:%yy failed [+e +f id:%yy] +> -Warning: cannot apply tags to missing message: foo and bar +> +Warning: cannot parse query: id:" +> +Warning: not an id query: tag:abc +> +Warning: cannot apply tags to missing message: missing_message_id +> EOF +> +> test_expect_equal_file EXPECTED OUTPUT +> -- +> 1.7.10.4 -- 2.26.2