[PATCH v3] emacs: Use the minibuffer for CLI error reporting

[Mark Walters]

On Thu, 03 Jan 2013, Austin Clements  wrote:
> We recently switched to popping up a buffer to report CLI errors, but
> this was too intrusive, especially for transient errors and especially
> since we made fewer things ignore errors.  This patch changes this to
> display a basic error message in the minibuffer (using Emacs' usual
> error handling path) and, if there are additional details, to log
> these to a separate error buffer and reference the error buffer from
> the minibuffer message.  This is more in line with how Emacs typically
> handles errors, but makes the details available to the user without
> flooding them with the details.
>
> Given this split, we pare down the basic message and make it more
> user-friendly, and also make the verbose message even more detailed
> (and more debugging-oriented).
> ---
>
> This version fixes two hard-coded paths in the tests.


This version looks good to me but I have one query we may like to
consider. 

At the moment notmuch-call-notmuch-process returns the stderr mixed with
stdout and we might like to separate that out (particularly as the error
message lists stderr and stdout separately and in this case it all gets
called stdout).

I attach a patch that does this: I am not really familiar with this so I
just took Austin's code from notmuch-call-notmuch-json.

Austin: obviously feel free to fold this into your patch if you think
appropriate.

Best wishes

Mark

>From b73395c8efb57111bd4de281797004747de6c2ed Mon Sep 17 00:00:00 2001
From: Mark Walters 
Date: Thu, 3 Jan 2013 22:25:02 +
Subject: [PATCH] tweak notmuch-call-notmuch-process

---
 emacs/notmuch.el |   10 +++---
 1 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/emacs/notmuch.el b/emacs/notmuch.el
index c98a4fe..4f7ee2c 100644
--- a/emacs/notmuch.el
+++ b/emacs/notmuch.el
@@ -540,9 +540,13 @@ If notmuch exits with a non-zero status, output from the 
process
 will appear in a buffer named \"*Notmuch errors*\" and an error
 will be signaled."
   (with-temp-buffer
-(let ((status (apply #'call-process notmuch-command nil t nil args)))
-  (notmuch-check-exit-status status (cons notmuch-command args)
-(buffer-string)
+(let ((err-file (make-temp-file "nmerr")))
+  (unwind-protect
+ (let ((status (apply #'call-process
+  notmuch-command nil (list t err-file) nil args)))
+   (notmuch-check-exit-status status (cons notmuch-command args)
+  (buffer-string) err-file))
+   (delete-file err-file)

 (defun notmuch-search-set-tags (tags  pos)
   (let ((new-result (plist-put (notmuch-search-get-result pos) :tags tags)))
-- 
1.7.9.1

Xapian-quoting based batch-tagging.

[Jameson Graef Rollins]

On Thu, Jan 03 2013, Jani Nikula  wrote:
>>> I am unclear about how this is going to deal with queries containing
>>> newlines. For dump/restore I think this is not a problem (as Austin and
>>> others have said), but for batch tagging I think it could be; for
>>> example the query could be for a tag containing a newline.
>>
>> Yes, that's true, this patch series does not support queries with tags
>> with embedded newlines. They can still be removed (and added) via either
>> batch tagging or the command line. We could just live with this, or
>
> I think we should just live with it. It's a bunch of code with some UI
> wrinkles for a marginal feature.

Tags with newlines are psychotic.  We should just explicitly forbid them
by preventing them from ever being applied and then never have to worry
about them again.

jamie.
-- next part --
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 835 bytes
Desc: not available
URL: 
<http://notmuchmail.org/pipermail/notmuch/attachments/20130103/8259aa75/attachment.pgp>

Xapian-quoting based batch-tagging.

[Jani Nikula]

On Tue, 25 Dec 2012, david at tethera.net wrote:
> This is an alternative version of 
>
>  id:1356313183-9266-1-git-send-email-david at tethera.net
>
> batch tagging patches rebased on top of 
>
>  id:1356415076-5692-1-git-send-email-amdragon at mit.edu

I didn't go through this quite as thoroughly as the previous versions,
but the series LGTM. I like how this simplifies things. In the future,
I'd like us to support xapian quoting also in the tag changes, for
consistency.

BR,
Jani.



>
> This mainly consisted of removing 
>
>  [Patch v9 04/17] notmuch-tag: factor out double quoting routine
>  (superceded by one of Austin's patches)
>
>  [Patch v9 05/17] util/string-util: add a new string tokenized function
>  [Patch v9 06/17] unhex_and_quote: new function to quote hex-decoded 
> queries
>  [Patch v9 07/17] notmuch-restore: move query handling for batch  
>  (uneeded if query is passed verbatim to xapian)
>
>  I also removed two tests, since they are about how we handle
>  quoting:
>
>  [Patch v9 13/17] test/tagging: add test for compound queries with batch 
> tagging
>  [Patch v9 17/17] test/tagging: add test for handling of parenthesized
> tag queries.
>
> A few small fixes were needed to the tests, and a fair amount of
> changes to the notmuch-tag man page.
>
> Diffstat (against Austin's series) is as follows
>
>  man/man1/notmuch-tag.1 |   99 -
>  notmuch-tag.c  |  169 
>  tag-util.c |   87 +--
>  tag-util.h |   15 
>  test/tagging   |  195 ++
>  5 files changed, 480 insertions(+), 85 deletions(-)
>
>
> ___
> notmuch mailing list
> notmuch at notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Xapian-quoting based batch-tagging.

[Jani Nikula]

On Wed, 26 Dec 2012, David Bremner  wrote:
> Mark Walters  writes:
>
>> I am unclear about how this is going to deal with queries containing
>> newlines. For dump/restore I think this is not a problem (as Austin and
>> others have said), but for batch tagging I think it could be; for
>> example the query could be for a tag containing a newline.
>
> Yes, that's true, this patch series does not support queries with tags
> with embedded newlines. They can still be removed (and added) via either
> batch tagging or the command line. We could just live with this, or

I think we should just live with it. It's a bunch of code with some UI
wrinkles for a marginal feature.

BR,
Jani.


>
> - The current syntax allows for detecting options at the start of the
>   line; perhaps a future fix would be to have the batch tagging and
>   command line tagging accept an optionally hex encoded query, something
>   like:
>
> --hex +found%20it -- tag:%22stupid%0Atag%22
>
> - Alternatively, we could add hex decoding on top of xapian quoting for
>   queries. One UI downside is that people have to remember that % are
>   special.
>
>  +found%25it -- tag:lost%25it
>
>   Another is that quoting is still (surprisingly) necessary for encoded
>   spaces
>   
>  +found%20it -- tag:"lost%20it"
>  
>   Introducing yet another escape format, e.g. "\n" would require more
>   code, and not really much benefit afaict versus re-using hex-encoding.
>   Offhand I don't see how to avoid this without some level of query
>   pre-processing a-la
>   
> id:1356313183-9266-1-git-send-email-david at tethera.net
>
> d
> ___
> notmuch mailing list
> notmuch at notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

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

[Jani Nikula]

On Tue, 25 Dec 2012, david at tethera.net wrote:
> From: Jani Nikula 
>
> Add support for batch tagging operations through stdin to "notmuch
> tag". This can be enabled with the new --batch command line option to
> "notmuch tag". The input must consist of lines of the format:
>
> +|- [...] [--]  [...]
>
> Each line is interpreted similarly to "notmuch tag" command line
> arguments. The delimiter is one or more spaces ' '. Any characters in
>  MAP be hex encoded with %NN where NN is the

MAY

> hexadecimal value of the character. Any ' ' and '%' characters in
>  and  MUST be hex encoded (using %20 and %25,

If we also required double quotes '"' to be hex encoded, we would have
an easier transition to using xapian quoting for tags too if we so
choose. notmuch dump already does this. If we additionally require % to
be quoted when using xapian quoting, we could detect hex vs. xapian
automatically.

> respectively). Any characters that are not part of  or

-or

> MUST NOT be hex encoded.
>
>  is passed verbatim to Xapian
>
> Leading and trailing space ' ' is ignored. Empty lines and lines
> beginning with '#' are ignored.
>
> Signed-off-by: Jani Nikula 
>
> Hacked-like-crazy-by: David Bremner 
> ---
>  notmuch-tag.c |   94 
> -
>  1 file changed, 86 insertions(+), 8 deletions(-)
>
> diff --git a/notmuch-tag.c b/notmuch-tag.c
> index 8129912..7fc614d 100644
> --- a/notmuch-tag.c
> +++ b/notmuch-tag.c
> @@ -128,6 +128,46 @@ tag_query (void *ctx, notmuch_database_t *notmuch, const 
> char *query_string,
>  return interrupted;
>  }
>  
> +static int
> +tag_file (void *ctx, notmuch_database_t *notmuch, tag_op_flag_t flags,
> +   FILE *input)
> +{
> +char *line = NULL;
> +char *query_string = NULL;
> +size_t line_size = 0;
> +ssize_t line_len;
> +int ret = 0;
> +tag_op_list_t *tag_ops;
> +
> +tag_ops = tag_op_list_create (ctx);
> +if (tag_ops == NULL) {
> + fprintf (stderr, "Out of memory.\n");
> + return 1;
> +}
> +
> +while ((line_len = getline (, _size, input)) != -1 &&
> +! interrupted) {
> +
> + ret = parse_tag_line (ctx, line, TAG_FLAG_NONE,
> +   _string, tag_ops);
> +
> + if (ret > 0)
> + continue;
> +
> + if (ret < 0)
> + break;
> +
> + ret = tag_query (ctx, notmuch, query_string, tag_ops, flags);
> + if (ret)
> + break;
> +}
> +
> +if (line)
> + free (line);
> +
> +return ret;
> +}
> +
>  int
>  notmuch_tag_command (void *ctx, int argc, char *argv[])
>  {
> @@ -137,6 +177,10 @@ notmuch_tag_command (void *ctx, int argc, char *argv[])
>  notmuch_database_t *notmuch;
>  struct sigaction action;
>  tag_op_flag_t tag_flags = TAG_FLAG_NONE;
> +notmuch_bool_t batch = FALSE;
> +FILE *input = stdin;
> +char *input_file_name = NULL;
> +int opt_index;
>  int ret = 0;
>  
>  /* Setup our handler for SIGINT */
> @@ -146,15 +190,43 @@ notmuch_tag_command (void *ctx, int argc, char *argv[])
>  action.sa_flags = SA_RESTART;
>  sigaction (SIGINT, , NULL);
>  
> -tag_ops = tag_op_list_create (ctx);
> -if (tag_ops == NULL) {
> - fprintf (stderr, "Out of memory.\n");
> +notmuch_opt_desc_t options[] = {
> + { NOTMUCH_OPT_BOOLEAN, , "batch", 0, 0 },
> + { NOTMUCH_OPT_STRING, _file_name, "input", 'i', 0 },
> + { 0, 0, 0, 0, 0 }
> +};
> +
> +opt_index = parse_arguments (argc, argv, options, 1);
> +if (opt_index < 0)
>   return 1;
> +
> +if (input_file_name) {
> + batch = TRUE;
> + input = fopen (input_file_name, "r");
> + if (input == NULL) {
> + fprintf (stderr, "Error opening %s for reading: %s\n",
> +  input_file_name, strerror (errno));
> + return 1;
> + }
>  }
>  
> -if (parse_tag_command_line (ctx, argc - 1, argv + 1,
> - _string, tag_ops))
> - return 1;
> +if (batch) {
> + if (opt_index != argc) {
> + fprintf (stderr, "Can't specify both cmdline and stdin!\n");
> + return 1;
> + }
> +} else {
> +
> + tag_ops = tag_op_list_create (ctx);
> + if (tag_ops == NULL) {
> + fprintf (stderr, "Out of memory.\n");
> + return 1;
> + }
> +
> + if (parse_tag_command_line (ctx, argc - opt_index, argv + opt_index,
> + _string, tag_ops))
> + return 1;
> +}
>  
>  config = notmuch_config_open (ctx, NULL, NULL);
>  if (config == NULL)
> @@ -167,9 +239,15 @@ notmuch_tag_command (void *ctx, int argc, char *argv[])
>  if (notmuch_config_get_maildir_synchronize_flags (config))
>   tag_flags |= TAG_FLAG_MAILDIR_SYNC;
>  
> -ret = tag_query (ctx, notmuch, query_string, tag_ops, tag_flags);
> +if (batch)
> + ret = tag_file (ctx, notmuch, tag_flags, input);
> +else
> + ret = tag_query (ctx, notmuch, query_string, tag_ops, 

[PATCH] emacs: Use the minibuffer for CLI error reporting

[David Bremner]

Austin Clements  writes:

>
> Async errors are harder, since it's 2013 and Emacs still provides no
> means to separate stdout from stderr for async processes.  The official
> way to do this is to fire up a shell running the command and have the
> shell redirect stderr.  This may be worthwhile for search since it would
> give us better error messages and eliminate the crazy resynchronization
> we have to do to deal with errors embedded in the output, but that's for
> another patch.

Nothing to do with this patch per se. but it might be sensible to add
top level convenience arguments to the notmuch command to redirect
stdout and stderr.

d

[PATCH v4 3/5] dump: Disallow \n in message IDs

[Jani Nikula]

On Mon, 31 Dec 2012, Austin Clements  wrote:
> When we switch to using regular Xapian queries in the dump format, \n
> will cause problems, so we disallow it.  Specially, while Xapian can
> quote and parse queries containing \n without difficultly, quoted
> queries containing \n still span multiple lines, which breaks the
> line-orientedness of the dump format.  Strictly speaking, we could
> still round-trip these, but it would significantly complicate restore
> as well as scripts that deal with tag dumps.  This complexity would
> come at absolutely no benefit: because of the RFC 2822 unfolding
> rules, no amount of standards negligence can produce a message with a
> message ID containing a line break (not even Outlook can do it!).
>
> Hence, we simply disallow it.
> ---
>  notmuch-dump.c   |9 +
>  test/random-corpus.c |4 +++-
>  2 files changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/notmuch-dump.c b/notmuch-dump.c
> index d2dad40..29d79da 100644
> --- a/notmuch-dump.c
> +++ b/notmuch-dump.c
> @@ -132,6 +132,15 @@ notmuch_dump_command (unused (void *ctx), int argc, char 
> *argv[])
>   if (output_format == DUMP_FORMAT_SUP) {
>   fputs (")\n", output);
>   } else {
> + if (strchr (message_id, '\n')) {
> + /* This will produce a line break in the output, which
> +  * would be difficult to handle in tools.  However,
> +  * it's also impossible to produce an email containing
> +  * a line break in a message ID because of unfolding,
> +  * so we can safely disallow it. */
> + fprintf (stderr, "Error: cannot dump message id containing line 
> break: %s\n", message_id);
> + return 1;

How about just skipping the message in the dump, with a warning, instead
of bailing out? If the user is desperate to do a backup for whatever
reason, I don't think it's a good idea to require deleting the message
from the db before dump can succeed. The fs holding the db might be
remounted ro and all that.

And perhaps the message id in the error message should be wrapped in
quotes, because it will span multiple lines due to having a
newline... ;)

Otherwise, LGTM.

Jani.

> + }
>   if (hex_encode (notmuch, message_id,
>   , _size) != HEX_SUCCESS) {
>   fprintf (stderr, "Error: failed to hex-encode msg-id %s\n",
> diff --git a/test/random-corpus.c b/test/random-corpus.c
> index f354d4b..8b7748e 100644
> --- a/test/random-corpus.c
> +++ b/test/random-corpus.c
> @@ -96,7 +96,9 @@ random_utf8_string (void *ctx, size_t char_count)
>   buf = talloc_realloc (ctx, buf, gchar, buf_size);
>   }
>  
> - randomchar = random_unichar ();
> + do {
> + randomchar = random_unichar ();
> + } while (randomchar == '\n');
>  
>   written = g_unichar_to_utf8 (randomchar, buf + offset);
>  
> -- 
> 1.7.10.4

[PATCH v4 2/5] util: Function to parse boolean term queries

[Jani Nikula]

On Mon, 31 Dec 2012, Austin Clements  wrote:
> This parses the subset of Xapian's boolean term quoting rules that are
> used by make_boolean_term.  This is provided as a generic string
> utility, but will be used shortly in notmuch restore to parse and
> optimize for ID queries.
> ---
>  util/string-util.c |   67 
> 
>  util/string-util.h |   15 
>  2 files changed, 82 insertions(+)
>
> diff --git a/util/string-util.c b/util/string-util.c
> index e4bea21..52c7781 100644
> --- a/util/string-util.c
> +++ b/util/string-util.c
> @@ -22,6 +22,8 @@
>  #include "string-util.h"
>  #include "talloc.h"
>  
> +#include 
> +
>  char *
>  strtok_len (char *s, const char *delim, size_t *len)
>  {
> @@ -96,3 +98,68 @@ make_boolean_term (void *ctx, const char *prefix, const 
> char *term,
>  
>  return 0;
>  }
> +
> +static const char*
> +skip_space (const char *str)
> +{
> +while (*str && isspace (*str))

Pedantic: isspace ((unsigned char) *str)

> + ++str;
> +return str;
> +}
> +
> +int
> +parse_boolean_term (void *ctx, const char *str,
> + char **prefix_out, char **term_out)
> +{
> +*prefix_out = *term_out = NULL;
> +
> +/* Parse prefix */
> +str = skip_space (str);
> +const char *pos = strchr (str, ':');
> +if (! pos)

if (! pos || pos == str) ?

> + goto FAIL;

Could just return 1 here.

> +*prefix_out = talloc_strndup (ctx, str, pos - str);
> +++pos;
> +
> +/* Implement de-quoting compatible with make_boolean_term. */
> +if (*pos == '"') {
> + char *out = talloc_array (ctx, char, strlen (pos));
> + int closed = 0;
> + *term_out = out;
> + /* Skip the opening quote, find the closing quote, and
> +  * un-double doubled internal quotes. */
> + for (++pos; *pos; ) {
> + if (*pos == '"') {
> + ++pos;
> + if (*pos != '"') {
> + /* Found the closing quote. */
> + closed = 1;
> + pos = skip_space (pos);

Is it necessary to accept trailing space?

> + break;
> + }
> + }
> + *out++ = *pos++;
> + }
> + /* Did the term terminate without a closing quote or is there
> +  * trailing text after the closing quote? */
> + if (!closed || *pos)
> + goto FAIL;
> + *out = '\0';
> +} else {
> + const char *start = pos;
> + /* Check for text after the boolean term. */
> + while (*pos > ' ' && *pos != ')')

The condition could have *pos there too for clarity, though not strictly
necessary. Would be neat to have a ctype style helper that could be
shared between this and make_boolean_term.

> + ++pos;
> + if (*skip_space (pos))

Is it necessary to accept trailing space?

> + goto FAIL;
> + /* No trailing text; dup the string so the caller can free
> +  * it. */
> + *term_out = talloc_strndup (ctx, start, pos - start);
> +}
> +return 0;
> +
> + FAIL:
> +talloc_free (*prefix_out);
> +talloc_free (*term_out);
> +return 1;
> +}
> diff --git a/util/string-util.h b/util/string-util.h
> index b8844a3..8b9fe50 100644
> --- a/util/string-util.h
> +++ b/util/string-util.h
> @@ -33,4 +33,19 @@ char *strtok_len (char *s, const char *delim, size_t *len);
>  int make_boolean_term (void *talloc_ctx, const char *prefix, const char 
> *term,
>  char **buf, size_t *len);
>  
> +/* Parse a boolean term query consisting of a prefix, a colon, and a
> + * term that may be quoted as described for make_boolean_term.  If the
> + * term is not quoted, then it ends at the first whitespace or close
> + * parenthesis.  str may containing leading or trailing whitespace,
> + * but anything else is considered a parse error.  This is compatible
> + * with anything produced by make_boolean_term, and supports a subset
> + * of the quoting styles supported by Xapian (and hence notmuch).
> + * *prefix_out and *term_out will be talloc'd with context ctx.
> + *
> + * Return: 0 on success, non-zero on parse error.
> + */
> +int
> +parse_boolean_term (void *ctx, const char *str,
> + char **prefix_out, char **term_out);
> +
>  #endif
> -- 
> 1.7.10.4

[PATCH v4 1/5] util: Factor out boolean term quoting routine

[Jani Nikula]

On Mon, 31 Dec 2012, Austin Clements  wrote:
> From: Austin Clements 
>
> This is now a generic boolean term quoting function.  It performs
> minimal quoting to produce user-friendly queries.
>
> This could live in tag-util as well, but it is really nothing specific
> to tags (although the conventions are specific to Xapian).
>
> The API is changed from "caller-allocates" to "readline-like".  The
> scan for max tag length is pushed down into the quoting routine.
> Furthermore, this now combines the term prefix with the quoted term;
> arguably this is just as easy to do in the caller, but this will
> nicely parallel the boolean term parsing function to be introduced
> shortly.
>
> This is an amalgamation of code written by David Bremner and myself.
> ---
>  notmuch-tag.c  |   48 ---
>  util/string-util.c |   64 
> 
>  util/string-util.h |   14 
>  3 files changed, 92 insertions(+), 34 deletions(-)
>
> diff --git a/notmuch-tag.c b/notmuch-tag.c
> index 88d559b..fc9d43a 100644
> --- a/notmuch-tag.c
> +++ b/notmuch-tag.c
> @@ -19,6 +19,7 @@
>   */
>  
>  #include "notmuch-client.h"
> +#include "string-util.h"
>  
>  static volatile sig_atomic_t interrupted;
>  
> @@ -35,25 +36,6 @@ handle_sigint (unused (int sig))
>  interrupted = 1;
>  }
>  
> -static char *
> -_escape_tag (char *buf, const char *tag)
> -{
> -const char *in = tag;
> -char *out = buf;
> -
> -/* Boolean terms surrounded by double quotes can contain any
> - * character.  Double quotes are quoted by doubling them. */
> -*out++ = '"';
> -while (*in) {
> - if (*in == '"')
> - *out++ = '"';
> - *out++ = *in++;
> -}
> -*out++ = '"';
> -*out = 0;
> -return buf;
> -}
> -
>  typedef struct {
>  const char *tag;
>  notmuch_bool_t remove;
> @@ -71,25 +53,16 @@ _optimize_tag_query (void *ctx, const char 
> *orig_query_string,
>   * parenthesize and the exclusion part of the query must not use
>   * the '-' operator (though the NOT operator is fine). */
>  
> -char *escaped, *query_string;
> +char *escaped = NULL;
> +size_t escaped_len = 0;
> +char *query_string;
>  const char *join = "";
> -int i;
> -unsigned int max_tag_len = 0;
> +size_t i;
>  
>  /* Don't optimize if there are no tag changes. */
>  if (tag_ops[0].tag == NULL)
>   return talloc_strdup (ctx, orig_query_string);
>  
> -/* Allocate a buffer for escaping tags.  This is large enough to
> - * hold a fully escaped tag with every character doubled plus
> - * enclosing quotes and a NUL. */
> -for (i = 0; tag_ops[i].tag; i++)
> - if (strlen (tag_ops[i].tag) > max_tag_len)
> - max_tag_len = strlen (tag_ops[i].tag);
> -escaped = talloc_array (ctx, char, max_tag_len * 2 + 3);
> -if (! escaped)
> - return NULL;
> -
>  /* Build the new query string */
>  if (strcmp (orig_query_string, "*") == 0)
>   query_string = talloc_strdup (ctx, "(");
> @@ -97,10 +70,17 @@ _optimize_tag_query (void *ctx, const char 
> *orig_query_string,
>   query_string = talloc_asprintf (ctx, "( %s ) and (", orig_query_string);
>  
>  for (i = 0; tag_ops[i].tag && query_string; i++) {
> + /* XXX in case of OOM, query_string will be deallocated when
> +  * ctx is, which might be at shutdown */
> + if (make_boolean_term (ctx,
> +"tag", tag_ops[i].tag,
> +, _len))
> + return NULL;
> +
>   query_string = talloc_asprintf_append_buffer (
> - query_string, "%s%stag:%s", join,
> + query_string, "%s%s%s", join,
>   tag_ops[i].remove ? "" : "not ",
> - _escape_tag (escaped, tag_ops[i].tag));
> + escaped);
>   join = " or ";
>  }
>  
> diff --git a/util/string-util.c b/util/string-util.c
> index 44f8cd3..e4bea21 100644
> --- a/util/string-util.c
> +++ b/util/string-util.c
> @@ -20,6 +20,7 @@
>  
>  
>  #include "string-util.h"
> +#include "talloc.h"
>  
>  char *
>  strtok_len (char *s, const char *delim, size_t *len)
> @@ -32,3 +33,66 @@ strtok_len (char *s, const char *delim, size_t *len)
>  
>  return *len ? s : NULL;
>  }
> +
> +int
> +make_boolean_term (void *ctx, const char *prefix, const char *term,
> +char **buf, size_t *len)
> +{
> +const char *in;
> +char *out;
> +size_t needed = 3;
> +int need_quoting = 0;
> +
> +/* Do we need quoting?  To be paranoid, we quote anything
> + * containing a quote, even though it only matters at the
> + * beginning, and anything containing non-ASCII text. */
> +for (in = term; *in && !need_quoting; in++)
> + if (*in <= ' ' || *in == ')' || *in == '"' || (unsigned char)*in > 127)

Should that be *in >= 127?

Otherwise LGTM.

Jani.

> + need_quoting = 1;
> +
> +if (need_quoting)
> + for (in = term; *in; in++)
> + 

[PATCH v3] emacs: Use the minibuffer for CLI error reporting

[Austin Clements]

We recently switched to popping up a buffer to report CLI errors, but
this was too intrusive, especially for transient errors and especially
since we made fewer things ignore errors.  This patch changes this to
display a basic error message in the minibuffer (using Emacs' usual
error handling path) and, if there are additional details, to log
these to a separate error buffer and reference the error buffer from
the minibuffer message.  This is more in line with how Emacs typically
handles errors, but makes the details available to the user without
flooding them with the details.

Given this split, we pare down the basic message and make it more
user-friendly, and also make the verbose message even more detailed
(and more debugging-oriented).
---

This version fixes two hard-coded paths in the tests.

 emacs/notmuch-lib.el |   92 --
 emacs/notmuch.el |9 +++--
 test/emacs   |   19 ---
 test/emacs-show  |   26 ++
 4 files changed, 90 insertions(+), 56 deletions(-)

diff --git a/emacs/notmuch-lib.el b/emacs/notmuch-lib.el
index 77a591d..d78bcf8 100644
--- a/emacs/notmuch-lib.el
+++ b/emacs/notmuch-lib.el
@@ -316,23 +316,28 @@ string), a property list of face attributes, or a list of 
these."
(put-text-property pos next 'face (cons face cur))
(setq pos next)

-(defun notmuch-pop-up-error (msg)
-  "Pop up an error buffer displaying MSG.
-
-This will accumulate error messages in the errors buffer until
-the user dismisses it."
-
-  (let ((buf (get-buffer-create "*Notmuch errors*")))
-(with-current-buffer buf
-  (view-mode-enter nil #'kill-buffer)
-  (let ((inhibit-read-only t))
-   (goto-char (point-max))
-   (unless (bobp)
- (insert "\n"))
-   (insert msg)
+(defun notmuch-logged-error (msg  extra)
+  "Log MSG and EXTRA to *Notmuch errors* and signal MSG.
+
+This logs MSG and EXTRA to the *Notmuch errors* buffer and
+signals MSG as an error.  If EXTRA is non-nil, text referring the
+user to the *Notmuch errors* buffer will be appended to the
+signaled error.  This function does not return."
+
+  (with-current-buffer (get-buffer-create "*Notmuch errors*")
+(goto-char (point-max))
+(unless (bobp)
+  (newline))
+(save-excursion
+  (insert "[" (current-time-string) "]\n" msg)
+  (unless (bolp)
+   (newline))
+  (when extra
+   (insert extra)
(unless (bolp)
- (insert "\n"
-(pop-to-buffer buf)))
+ (newline)
+  (error "%s" (concat msg (when extra
+   " (see *Notmuch errors* for more details)"

 (defun notmuch-check-async-exit-status (proc msg)
   "If PROC exited abnormally, pop up an error buffer and signal an error.
@@ -363,35 +368,40 @@ contents of ERR-FILE will be included in the error 
message."
   (cond
((eq exit-status 0) t)
((eq exit-status 20)
-(notmuch-pop-up-error "Error: Version mismatch.
+(notmuch-logged-error "notmuch CLI version mismatch
 Emacs requested an older output format than supported by the notmuch CLI.
-You may need to restart Emacs or upgrade your notmuch Emacs package.")
-(error "notmuch CLI version mismatch"))
+You may need to restart Emacs or upgrade your notmuch Emacs package."))
((eq exit-status 21)
-(notmuch-pop-up-error "Error: Version mismatch.
+(notmuch-logged-error "notmuch CLI version mismatch
 Emacs requested a newer output format than supported by the notmuch CLI.
-You may need to restart Emacs or upgrade your notmuch package.")
-(error "notmuch CLI version mismatch"))
+You may need to restart Emacs or upgrade your notmuch package."))
(t
-(notmuch-pop-up-error
- (concat
-  (format "Error invoking notmuch.  %s exited with %s%s.\n"
- (mapconcat #'identity command " ")
- ;; Signal strings look like "Terminated", hence the
- ;; colon.
- (if (integerp exit-status) "status " "signal: ")
- exit-status)
-  (when err-file
-   (concat "Error:\n"
-   (with-temp-buffer
- (insert-file-contents err-file)
- (if (eobp)
- "(no error output)\n"
-   (buffer-string)
-  (when (and output (not (equal output "")))
-   (format "Output:\n%s" output
-;; Mimic `process-lines'
-(error "%s exited with status %s" (car command) exit-status
+(let* ((err (when err-file
+ (with-temp-buffer
+   (insert-file-contents err-file)
+   (unless (eobp)
+ (buffer-string)
+  (extra
+   (concat
+"command: " (mapconcat #'shell-quote-argument command " ") "\n"
+(if (integerp exit-status)
+(format "exit status: %s\n" exit-status)
+  (format "exit signal: %s\n" exit-status))
+(when err
+  (concat "stderr:\n" 

Re: [PATCH v4 1/5] util: Factor out boolean term quoting routine

[Jani Nikula]

On Mon, 31 Dec 2012, Austin Clements amdra...@mit.edu wrote:
 From: Austin Clements amdra...@mit.edu

 This is now a generic boolean term quoting function.  It performs
 minimal quoting to produce user-friendly queries.

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

 The API is changed from caller-allocates to readline-like.  The
 scan for max tag length is pushed down into the quoting routine.
 Furthermore, this now combines the term prefix with the quoted term;
 arguably this is just as easy to do in the caller, but this will
 nicely parallel the boolean term parsing function to be introduced
 shortly.

 This is an amalgamation of code written by David Bremner and myself.
 ---
  notmuch-tag.c  |   48 ---
  util/string-util.c |   64 
 
  util/string-util.h |   14 
  3 files changed, 92 insertions(+), 34 deletions(-)

 diff --git a/notmuch-tag.c b/notmuch-tag.c
 index 88d559b..fc9d43a 100644
 --- a/notmuch-tag.c
 +++ b/notmuch-tag.c
 @@ -19,6 +19,7 @@
   */
  
  #include notmuch-client.h
 +#include string-util.h
  
  static volatile sig_atomic_t interrupted;
  
 @@ -35,25 +36,6 @@ handle_sigint (unused (int sig))
  interrupted = 1;
  }
  
 -static char *
 -_escape_tag (char *buf, const char *tag)
 -{
 -const char *in = tag;
 -char *out = buf;
 -
 -/* Boolean terms surrounded by double quotes can contain any
 - * character.  Double quotes are quoted by doubling them. */
 -*out++ = '';
 -while (*in) {
 - if (*in == '')
 - *out++ = '';
 - *out++ = *in++;
 -}
 -*out++ = '';
 -*out = 0;
 -return buf;
 -}
 -
  typedef struct {
  const char *tag;
  notmuch_bool_t remove;
 @@ -71,25 +53,16 @@ _optimize_tag_query (void *ctx, const char 
 *orig_query_string,
   * parenthesize and the exclusion part of the query must not use
   * the '-' operator (though the NOT operator is fine). */
  
 -char *escaped, *query_string;
 +char *escaped = NULL;
 +size_t escaped_len = 0;
 +char *query_string;
  const char *join = ;
 -int i;
 -unsigned int max_tag_len = 0;
 +size_t i;
  
  /* Don't optimize if there are no tag changes. */
  if (tag_ops[0].tag == NULL)
   return talloc_strdup (ctx, orig_query_string);
  
 -/* Allocate a buffer for escaping tags.  This is large enough to
 - * hold a fully escaped tag with every character doubled plus
 - * enclosing quotes and a NUL. */
 -for (i = 0; tag_ops[i].tag; i++)
 - if (strlen (tag_ops[i].tag)  max_tag_len)
 - max_tag_len = strlen (tag_ops[i].tag);
 -escaped = talloc_array (ctx, char, max_tag_len * 2 + 3);
 -if (! escaped)
 - return NULL;
 -
  /* Build the new query string */
  if (strcmp (orig_query_string, *) == 0)
   query_string = talloc_strdup (ctx, ();
 @@ -97,10 +70,17 @@ _optimize_tag_query (void *ctx, const char 
 *orig_query_string,
   query_string = talloc_asprintf (ctx, ( %s ) and (, orig_query_string);
  
  for (i = 0; tag_ops[i].tag  query_string; i++) {
 + /* XXX in case of OOM, query_string will be deallocated when
 +  * ctx is, which might be at shutdown */
 + if (make_boolean_term (ctx,
 +tag, tag_ops[i].tag,
 +escaped, escaped_len))
 + return NULL;
 +
   query_string = talloc_asprintf_append_buffer (
 - query_string, %s%stag:%s, join,
 + query_string, %s%s%s, join,
   tag_ops[i].remove ?  : not ,
 - _escape_tag (escaped, tag_ops[i].tag));
 + escaped);
   join =  or ;
  }
  
 diff --git a/util/string-util.c b/util/string-util.c
 index 44f8cd3..e4bea21 100644
 --- a/util/string-util.c
 +++ b/util/string-util.c
 @@ -20,6 +20,7 @@
  
  
  #include string-util.h
 +#include talloc.h
  
  char *
  strtok_len (char *s, const char *delim, size_t *len)
 @@ -32,3 +33,66 @@ strtok_len (char *s, const char *delim, size_t *len)
  
  return *len ? s : NULL;
  }
 +
 +int
 +make_boolean_term (void *ctx, const char *prefix, const char *term,
 +char **buf, size_t *len)
 +{
 +const char *in;
 +char *out;
 +size_t needed = 3;
 +int need_quoting = 0;
 +
 +/* Do we need quoting?  To be paranoid, we quote anything
 + * containing a quote, even though it only matters at the
 + * beginning, and anything containing non-ASCII text. */
 +for (in = term; *in  !need_quoting; in++)
 + if (*in = ' ' || *in == ')' || *in == '' || (unsigned char)*in  127)

Should that be *in = 127?

Otherwise LGTM.

Jani.

 + need_quoting = 1;
 +
 +if (need_quoting)
 + for (in = term; *in; in++)
 + needed += (*in == '') ? 2 : 1;
 +else
 + needed = strlen (term) + 1;
 +
 +/* Reserve space for the prefix */
 +if (prefix)
 + 

Re: [PATCH v4 2/5] util: Function to parse boolean term queries

[Jani Nikula]

On Mon, 31 Dec 2012, Austin Clements amdra...@mit.edu wrote:
 This parses the subset of Xapian's boolean term quoting rules that are
 used by make_boolean_term.  This is provided as a generic string
 utility, but will be used shortly in notmuch restore to parse and
 optimize for ID queries.
 ---
  util/string-util.c |   67 
 
  util/string-util.h |   15 
  2 files changed, 82 insertions(+)

 diff --git a/util/string-util.c b/util/string-util.c
 index e4bea21..52c7781 100644
 --- a/util/string-util.c
 +++ b/util/string-util.c
 @@ -22,6 +22,8 @@
  #include string-util.h
  #include talloc.h
  
 +#include ctype.h
 +
  char *
  strtok_len (char *s, const char *delim, size_t *len)
  {
 @@ -96,3 +98,68 @@ make_boolean_term (void *ctx, const char *prefix, const 
 char *term,
  
  return 0;
  }
 +
 +static const char*
 +skip_space (const char *str)
 +{
 +while (*str  isspace (*str))

Pedantic: isspace ((unsigned char) *str)

 + ++str;
 +return str;
 +}
 +
 +int
 +parse_boolean_term (void *ctx, const char *str,
 + char **prefix_out, char **term_out)
 +{
 +*prefix_out = *term_out = NULL;
 +
 +/* Parse prefix */
 +str = skip_space (str);
 +const char *pos = strchr (str, ':');
 +if (! pos)

if (! pos || pos == str) ?

 + goto FAIL;

Could just return 1 here.

 +*prefix_out = talloc_strndup (ctx, str, pos - str);
 +++pos;
 +
 +/* Implement de-quoting compatible with make_boolean_term. */
 +if (*pos == '') {
 + char *out = talloc_array (ctx, char, strlen (pos));
 + int closed = 0;
 + *term_out = out;
 + /* Skip the opening quote, find the closing quote, and
 +  * un-double doubled internal quotes. */
 + for (++pos; *pos; ) {
 + if (*pos == '') {
 + ++pos;
 + if (*pos != '') {
 + /* Found the closing quote. */
 + closed = 1;
 + pos = skip_space (pos);

Is it necessary to accept trailing space?

 + break;
 + }
 + }
 + *out++ = *pos++;
 + }
 + /* Did the term terminate without a closing quote or is there
 +  * trailing text after the closing quote? */
 + if (!closed || *pos)
 + goto FAIL;
 + *out = '\0';
 +} else {
 + const char *start = pos;
 + /* Check for text after the boolean term. */
 + while (*pos  ' '  *pos != ')')

The condition could have *pos there too for clarity, though not strictly
necessary. Would be neat to have a ctype style helper that could be
shared between this and make_boolean_term.

 + ++pos;
 + if (*skip_space (pos))

Is it necessary to accept trailing space?

 + goto FAIL;
 + /* No trailing text; dup the string so the caller can free
 +  * it. */
 + *term_out = talloc_strndup (ctx, start, pos - start);
 +}
 +return 0;
 +
 + FAIL:
 +talloc_free (*prefix_out);
 +talloc_free (*term_out);
 +return 1;
 +}
 diff --git a/util/string-util.h b/util/string-util.h
 index b8844a3..8b9fe50 100644
 --- a/util/string-util.h
 +++ b/util/string-util.h
 @@ -33,4 +33,19 @@ char *strtok_len (char *s, const char *delim, size_t *len);
  int make_boolean_term (void *talloc_ctx, const char *prefix, const char 
 *term,
  char **buf, size_t *len);
  
 +/* Parse a boolean term query consisting of a prefix, a colon, and a
 + * term that may be quoted as described for make_boolean_term.  If the
 + * term is not quoted, then it ends at the first whitespace or close
 + * parenthesis.  str may containing leading or trailing whitespace,
 + * but anything else is considered a parse error.  This is compatible
 + * with anything produced by make_boolean_term, and supports a subset
 + * of the quoting styles supported by Xapian (and hence notmuch).
 + * *prefix_out and *term_out will be talloc'd with context ctx.
 + *
 + * Return: 0 on success, non-zero on parse error.
 + */
 +int
 +parse_boolean_term (void *ctx, const char *str,
 + char **prefix_out, char **term_out);
 +
  #endif
 -- 
 1.7.10.4
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH v4 3/5] dump: Disallow \n in message IDs

[Jani Nikula]

On Mon, 31 Dec 2012, Austin Clements amdra...@mit.edu wrote:
 When we switch to using regular Xapian queries in the dump format, \n
 will cause problems, so we disallow it.  Specially, while Xapian can
 quote and parse queries containing \n without difficultly, quoted
 queries containing \n still span multiple lines, which breaks the
 line-orientedness of the dump format.  Strictly speaking, we could
 still round-trip these, but it would significantly complicate restore
 as well as scripts that deal with tag dumps.  This complexity would
 come at absolutely no benefit: because of the RFC 2822 unfolding
 rules, no amount of standards negligence can produce a message with a
 message ID containing a line break (not even Outlook can do it!).

 Hence, we simply disallow it.
 ---
  notmuch-dump.c   |9 +
  test/random-corpus.c |4 +++-
  2 files changed, 12 insertions(+), 1 deletion(-)

 diff --git a/notmuch-dump.c b/notmuch-dump.c
 index d2dad40..29d79da 100644
 --- a/notmuch-dump.c
 +++ b/notmuch-dump.c
 @@ -132,6 +132,15 @@ notmuch_dump_command (unused (void *ctx), int argc, char 
 *argv[])
   if (output_format == DUMP_FORMAT_SUP) {
   fputs ()\n, output);
   } else {
 + if (strchr (message_id, '\n')) {
 + /* This will produce a line break in the output, which
 +  * would be difficult to handle in tools.  However,
 +  * it's also impossible to produce an email containing
 +  * a line break in a message ID because of unfolding,
 +  * so we can safely disallow it. */
 + fprintf (stderr, Error: cannot dump message id containing line 
 break: %s\n, message_id);
 + return 1;

How about just skipping the message in the dump, with a warning, instead
of bailing out? If the user is desperate to do a backup for whatever
reason, I don't think it's a good idea to require deleting the message
from the db before dump can succeed. The fs holding the db might be
remounted ro and all that.

And perhaps the message id in the error message should be wrapped in
quotes, because it will span multiple lines due to having a
newline... ;)

Otherwise, LGTM.

Jani.

 + }
   if (hex_encode (notmuch, message_id,
   buffer, buffer_size) != HEX_SUCCESS) {
   fprintf (stderr, Error: failed to hex-encode msg-id %s\n,
 diff --git a/test/random-corpus.c b/test/random-corpus.c
 index f354d4b..8b7748e 100644
 --- a/test/random-corpus.c
 +++ b/test/random-corpus.c
 @@ -96,7 +96,9 @@ random_utf8_string (void *ctx, size_t char_count)
   buf = talloc_realloc (ctx, buf, gchar, buf_size);
   }
  
 - randomchar = random_unichar ();
 + do {
 + randomchar = random_unichar ();
 + } while (randomchar == '\n');
  
   written = g_unichar_to_utf8 (randomchar, buf + offset);
  
 -- 
 1.7.10.4
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH 04/11] cli: add support for batch tagging operations to notmuch tag

[Jani Nikula]

On Tue, 25 Dec 2012, da...@tethera.net wrote:
 From: Jani Nikula j...@nikula.org

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

 +tag|-tag [...] [--] query [...]

 Each line is interpreted similarly to notmuch tag command line
 arguments. The delimiter is one or more spaces ' '. Any characters in
 tag MAP be hex encoded with %NN where NN is the

MAY

 hexadecimal value of the character. Any ' ' and '%' characters in
 tag and search-term MUST be hex encoded (using %20 and %25,

If we also required double quotes '' to be hex encoded, we would have
an easier transition to using xapian quoting for tags too if we so
choose. notmuch dump already does this. If we additionally require % to
be quoted when using xapian quoting, we could detect hex vs. xapian
automatically.

 respectively). Any characters that are not part of tag or

-or

 MUST NOT be hex encoded.

 query is passed verbatim to Xapian

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

 Signed-off-by: Jani Nikula j...@nikula.org

 Hacked-like-crazy-by: David Bremner da...@tethera.net
 ---
  notmuch-tag.c |   94 
 -
  1 file changed, 86 insertions(+), 8 deletions(-)

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

Re: Xapian-quoting based batch-tagging.

[Jani Nikula]

On Wed, 26 Dec 2012, David Bremner da...@tethera.net wrote:
 Mark Walters markwalters1...@gmail.com writes:

 I am unclear about how this is going to deal with queries containing
 newlines. For dump/restore I think this is not a problem (as Austin and
 others have said), but for batch tagging I think it could be; for
 example the query could be for a tag containing a newline.

 Yes, that's true, this patch series does not support queries with tags
 with embedded newlines. They can still be removed (and added) via either
 batch tagging or the command line. We could just live with this, or

I think we should just live with it. It's a bunch of code with some UI
wrinkles for a marginal feature.

BR,
Jani.



 - The current syntax allows for detecting options at the start of the
   line; perhaps a future fix would be to have the batch tagging and
   command line tagging accept an optionally hex encoded query, something
   like:

 --hex +found%20it -- tag:%22stupid%0Atag%22

 - Alternatively, we could add hex decoding on top of xapian quoting for
   queries. One UI downside is that people have to remember that % are
   special.

  +found%25it -- tag:lost%25it

   Another is that quoting is still (surprisingly) necessary for encoded
   spaces
   
  +found%20it -- tag:lost%20it
  
   Introducing yet another escape format, e.g. \n would require more
   code, and not really much benefit afaict versus re-using hex-encoding.
   Offhand I don't see how to avoid this without some level of query
   pre-processing a-la
   
 id:1356313183-9266-1-git-send-email-da...@tethera.net

 d
 ___
 notmuch mailing list
 notmuch@notmuchmail.org
 http://notmuchmail.org/mailman/listinfo/notmuch
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

Re: Xapian-quoting based batch-tagging.

[Jani Nikula]

On Tue, 25 Dec 2012, da...@tethera.net wrote:
 This is an alternative version of 

  id:1356313183-9266-1-git-send-email-da...@tethera.net

 batch tagging patches rebased on top of 

  id:1356415076-5692-1-git-send-email-amdra...@mit.edu

I didn't go through this quite as thoroughly as the previous versions,
but the series LGTM. I like how this simplifies things. In the future,
I'd like us to support xapian quoting also in the tag changes, for
consistency.

BR,
Jani.




 This mainly consisted of removing 

  [Patch v9 04/17] notmuch-tag: factor out double quoting routine
  (superceded by one of Austin's patches)

  [Patch v9 05/17] util/string-util: add a new string tokenized function
  [Patch v9 06/17] unhex_and_quote: new function to quote hex-decoded 
 queries
  [Patch v9 07/17] notmuch-restore: move query handling for batch  
  (uneeded if query is passed verbatim to xapian)

  I also removed two tests, since they are about how we handle
  quoting:

  [Patch v9 13/17] test/tagging: add test for compound queries with batch 
 tagging
  [Patch v9 17/17] test/tagging: add test for handling of parenthesized
 tag queries.

 A few small fixes were needed to the tests, and a fair amount of
 changes to the notmuch-tag man page.

 Diffstat (against Austin's series) is as follows

  man/man1/notmuch-tag.1 |   99 -
  notmuch-tag.c  |  169 
  tag-util.c |   87 +--
  tag-util.h |   15 
  test/tagging   |  195 ++
  5 files changed, 480 insertions(+), 85 deletions(-)


 ___
 notmuch mailing list
 notmuch@notmuchmail.org
 http://notmuchmail.org/mailman/listinfo/notmuch
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

[PATCH v3] emacs: Use the minibuffer for CLI error reporting

[Austin Clements]

We recently switched to popping up a buffer to report CLI errors, but
this was too intrusive, especially for transient errors and especially
since we made fewer things ignore errors.  This patch changes this to
display a basic error message in the minibuffer (using Emacs' usual
error handling path) and, if there are additional details, to log
these to a separate error buffer and reference the error buffer from
the minibuffer message.  This is more in line with how Emacs typically
handles errors, but makes the details available to the user without
flooding them with the details.

Given this split, we pare down the basic message and make it more
user-friendly, and also make the verbose message even more detailed
(and more debugging-oriented).
---

This version fixes two hard-coded paths in the tests.

 emacs/notmuch-lib.el |   92 --
 emacs/notmuch.el |9 +++--
 test/emacs   |   19 ---
 test/emacs-show  |   26 ++
 4 files changed, 90 insertions(+), 56 deletions(-)

diff --git a/emacs/notmuch-lib.el b/emacs/notmuch-lib.el
index 77a591d..d78bcf8 100644
--- a/emacs/notmuch-lib.el
+++ b/emacs/notmuch-lib.el
@@ -316,23 +316,28 @@ string), a property list of face attributes, or a list of 
these.
(put-text-property pos next 'face (cons face cur))
(setq pos next)
 
-(defun notmuch-pop-up-error (msg)
-  Pop up an error buffer displaying MSG.
-
-This will accumulate error messages in the errors buffer until
-the user dismisses it.
-
-  (let ((buf (get-buffer-create *Notmuch errors*)))
-(with-current-buffer buf
-  (view-mode-enter nil #'kill-buffer)
-  (let ((inhibit-read-only t))
-   (goto-char (point-max))
-   (unless (bobp)
- (insert \n))
-   (insert msg)
+(defun notmuch-logged-error (msg optional extra)
+  Log MSG and EXTRA to *Notmuch errors* and signal MSG.
+
+This logs MSG and EXTRA to the *Notmuch errors* buffer and
+signals MSG as an error.  If EXTRA is non-nil, text referring the
+user to the *Notmuch errors* buffer will be appended to the
+signaled error.  This function does not return.
+
+  (with-current-buffer (get-buffer-create *Notmuch errors*)
+(goto-char (point-max))
+(unless (bobp)
+  (newline))
+(save-excursion
+  (insert [ (current-time-string) ]\n msg)
+  (unless (bolp)
+   (newline))
+  (when extra
+   (insert extra)
(unless (bolp)
- (insert \n
-(pop-to-buffer buf)))
+ (newline)
+  (error %s (concat msg (when extra
+(see *Notmuch errors* for more details)
 
 (defun notmuch-check-async-exit-status (proc msg)
   If PROC exited abnormally, pop up an error buffer and signal an error.
@@ -363,35 +368,40 @@ contents of ERR-FILE will be included in the error 
message.
   (cond
((eq exit-status 0) t)
((eq exit-status 20)
-(notmuch-pop-up-error Error: Version mismatch.
+(notmuch-logged-error notmuch CLI version mismatch
 Emacs requested an older output format than supported by the notmuch CLI.
-You may need to restart Emacs or upgrade your notmuch Emacs package.)
-(error notmuch CLI version mismatch))
+You may need to restart Emacs or upgrade your notmuch Emacs package.))
((eq exit-status 21)
-(notmuch-pop-up-error Error: Version mismatch.
+(notmuch-logged-error notmuch CLI version mismatch
 Emacs requested a newer output format than supported by the notmuch CLI.
-You may need to restart Emacs or upgrade your notmuch package.)
-(error notmuch CLI version mismatch))
+You may need to restart Emacs or upgrade your notmuch package.))
(t
-(notmuch-pop-up-error
- (concat
-  (format Error invoking notmuch.  %s exited with %s%s.\n
- (mapconcat #'identity command  )
- ;; Signal strings look like Terminated, hence the
- ;; colon.
- (if (integerp exit-status) status  signal: )
- exit-status)
-  (when err-file
-   (concat Error:\n
-   (with-temp-buffer
- (insert-file-contents err-file)
- (if (eobp)
- (no error output)\n
-   (buffer-string)
-  (when (and output (not (equal output )))
-   (format Output:\n%s output
-;; Mimic `process-lines'
-(error %s exited with status %s (car command) exit-status
+(let* ((err (when err-file
+ (with-temp-buffer
+   (insert-file-contents err-file)
+   (unless (eobp)
+ (buffer-string)
+  (extra
+   (concat
+command:  (mapconcat #'shell-quote-argument command  ) \n
+(if (integerp exit-status)
+(format exit status: %s\n exit-status)
+  (format exit signal: %s\n exit-status))
+(when err
+  (concat stderr:\n err))
+(when output
+  (concat 

Re: [PATCH v3] emacs: Use the minibuffer for CLI error reporting

[Mark Walters]

On Thu, 03 Jan 2013, Austin Clements amdra...@mit.edu wrote:
 We recently switched to popping up a buffer to report CLI errors, but
 this was too intrusive, especially for transient errors and especially
 since we made fewer things ignore errors.  This patch changes this to
 display a basic error message in the minibuffer (using Emacs' usual
 error handling path) and, if there are additional details, to log
 these to a separate error buffer and reference the error buffer from
 the minibuffer message.  This is more in line with how Emacs typically
 handles errors, but makes the details available to the user without
 flooding them with the details.

 Given this split, we pare down the basic message and make it more
 user-friendly, and also make the verbose message even more detailed
 (and more debugging-oriented).
 ---

 This version fixes two hard-coded paths in the tests.


This version looks good to me but I have one query we may like to
consider. 

At the moment notmuch-call-notmuch-process returns the stderr mixed with
stdout and we might like to separate that out (particularly as the error
message lists stderr and stdout separately and in this case it all gets
called stdout).

I attach a patch that does this: I am not really familiar with this so I
just took Austin's code from notmuch-call-notmuch-json.

Austin: obviously feel free to fold this into your patch if you think
appropriate.

Best wishes

Mark

From b73395c8efb57111bd4de281797004747de6c2ed Mon Sep 17 00:00:00 2001
From: Mark Walters markwalters1...@gmail.com
Date: Thu, 3 Jan 2013 22:25:02 +
Subject: [PATCH] tweak notmuch-call-notmuch-process

---
 emacs/notmuch.el |   10 +++---
 1 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/emacs/notmuch.el b/emacs/notmuch.el
index c98a4fe..4f7ee2c 100644
--- a/emacs/notmuch.el
+++ b/emacs/notmuch.el
@@ -540,9 +540,13 @@ If notmuch exits with a non-zero status, output from the 
process
 will appear in a buffer named \*Notmuch errors*\ and an error
 will be signaled.
   (with-temp-buffer
-(let ((status (apply #'call-process notmuch-command nil t nil args)))
-  (notmuch-check-exit-status status (cons notmuch-command args)
-(buffer-string)
+(let ((err-file (make-temp-file nmerr)))
+  (unwind-protect
+ (let ((status (apply #'call-process
+  notmuch-command nil (list t err-file) nil args)))
+   (notmuch-check-exit-status status (cons notmuch-command args)
+  (buffer-string) err-file))
+   (delete-file err-file)
 
 (defun notmuch-search-set-tags (tags optional pos)
   (let ((new-result (plist-put (notmuch-search-get-result pos) :tags tags)))
-- 
1.7.9.1

___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH] emacs: Use the minibuffer for CLI error reporting

[David Bremner]

Austin Clements amdra...@mit.edu writes:


 Async errors are harder, since it's 2013 and Emacs still provides no
 means to separate stdout from stderr for async processes.  The official
 way to do this is to fire up a shell running the command and have the
 shell redirect stderr.  This may be worthwhile for search since it would
 give us better error messages and eliminate the crazy resynchronization
 we have to do to deal with errors embedded in the output, but that's for
 another patch.

Nothing to do with this patch per se. but it might be sensible to add
top level convenience arguments to the notmuch command to redirect
stdout and stderr.

d
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

Re: Xapian-quoting based batch-tagging.

[Jameson Graef Rollins]

On Thu, Jan 03 2013, Jani Nikula j...@nikula.org wrote:
 I am unclear about how this is going to deal with queries containing
 newlines. For dump/restore I think this is not a problem (as Austin and
 others have said), but for batch tagging I think it could be; for
 example the query could be for a tag containing a newline.

 Yes, that's true, this patch series does not support queries with tags
 with embedded newlines. They can still be removed (and added) via either
 batch tagging or the command line. We could just live with this, or

 I think we should just live with it. It's a bunch of code with some UI
 wrinkles for a marginal feature.

Tags with newlines are psychotic.  We should just explicitly forbid them
by preventing them from ever being applied and then never have to worry
about them again.

jamie.


pgpRJzcE8_Trw.pgp
Description: PGP signature
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH v4 1/5] util: Factor out boolean term quoting routine

[Austin Clements]

Quoth Jani Nikula on Jan 03 at  5:48 pm:
 On Mon, 31 Dec 2012, Austin Clements amdra...@mit.edu wrote:
  From: Austin Clements amdra...@mit.edu
 
  This is now a generic boolean term quoting function.  It performs
  minimal quoting to produce user-friendly queries.
 
  This could live in tag-util as well, but it is really nothing specific
  to tags (although the conventions are specific to Xapian).
 
  The API is changed from caller-allocates to readline-like.  The
  scan for max tag length is pushed down into the quoting routine.
  Furthermore, this now combines the term prefix with the quoted term;
  arguably this is just as easy to do in the caller, but this will
  nicely parallel the boolean term parsing function to be introduced
  shortly.
 
  This is an amalgamation of code written by David Bremner and myself.
  ---
   notmuch-tag.c  |   48 ---
   util/string-util.c |   64 
  
   util/string-util.h |   14 
   3 files changed, 92 insertions(+), 34 deletions(-)
 
  diff --git a/notmuch-tag.c b/notmuch-tag.c
  index 88d559b..fc9d43a 100644
  --- a/notmuch-tag.c
  +++ b/notmuch-tag.c
  @@ -19,6 +19,7 @@
*/
   
   #include notmuch-client.h
  +#include string-util.h
   
   static volatile sig_atomic_t interrupted;
   
  @@ -35,25 +36,6 @@ handle_sigint (unused (int sig))
   interrupted = 1;
   }
   
  -static char *
  -_escape_tag (char *buf, const char *tag)
  -{
  -const char *in = tag;
  -char *out = buf;
  -
  -/* Boolean terms surrounded by double quotes can contain any
  - * character.  Double quotes are quoted by doubling them. */
  -*out++ = '';
  -while (*in) {
  -   if (*in == '')
  -   *out++ = '';
  -   *out++ = *in++;
  -}
  -*out++ = '';
  -*out = 0;
  -return buf;
  -}
  -
   typedef struct {
   const char *tag;
   notmuch_bool_t remove;
  @@ -71,25 +53,16 @@ _optimize_tag_query (void *ctx, const char 
  *orig_query_string,
* parenthesize and the exclusion part of the query must not use
* the '-' operator (though the NOT operator is fine). */
   
  -char *escaped, *query_string;
  +char *escaped = NULL;
  +size_t escaped_len = 0;
  +char *query_string;
   const char *join = ;
  -int i;
  -unsigned int max_tag_len = 0;
  +size_t i;
   
   /* Don't optimize if there are no tag changes. */
   if (tag_ops[0].tag == NULL)
  return talloc_strdup (ctx, orig_query_string);
   
  -/* Allocate a buffer for escaping tags.  This is large enough to
  - * hold a fully escaped tag with every character doubled plus
  - * enclosing quotes and a NUL. */
  -for (i = 0; tag_ops[i].tag; i++)
  -   if (strlen (tag_ops[i].tag)  max_tag_len)
  -   max_tag_len = strlen (tag_ops[i].tag);
  -escaped = talloc_array (ctx, char, max_tag_len * 2 + 3);
  -if (! escaped)
  -   return NULL;
  -
   /* Build the new query string */
   if (strcmp (orig_query_string, *) == 0)
  query_string = talloc_strdup (ctx, ();
  @@ -97,10 +70,17 @@ _optimize_tag_query (void *ctx, const char 
  *orig_query_string,
  query_string = talloc_asprintf (ctx, ( %s ) and (, orig_query_string);
   
   for (i = 0; tag_ops[i].tag  query_string; i++) {
  +   /* XXX in case of OOM, query_string will be deallocated when
  +* ctx is, which might be at shutdown */
  +   if (make_boolean_term (ctx,
  +  tag, tag_ops[i].tag,
  +  escaped, escaped_len))
  +   return NULL;
  +
  query_string = talloc_asprintf_append_buffer (
  -   query_string, %s%stag:%s, join,
  +   query_string, %s%s%s, join,
  tag_ops[i].remove ?  : not ,
  -   _escape_tag (escaped, tag_ops[i].tag));
  +   escaped);
  join =  or ;
   }
   
  diff --git a/util/string-util.c b/util/string-util.c
  index 44f8cd3..e4bea21 100644
  --- a/util/string-util.c
  +++ b/util/string-util.c
  @@ -20,6 +20,7 @@
   
   
   #include string-util.h
  +#include talloc.h
   
   char *
   strtok_len (char *s, const char *delim, size_t *len)
  @@ -32,3 +33,66 @@ strtok_len (char *s, const char *delim, size_t *len)
   
   return *len ? s : NULL;
   }
  +
  +int
  +make_boolean_term (void *ctx, const char *prefix, const char *term,
  +  char **buf, size_t *len)
  +{
  +const char *in;
  +char *out;
  +size_t needed = 3;
  +int need_quoting = 0;
  +
  +/* Do we need quoting?  To be paranoid, we quote anything
  + * containing a quote, even though it only matters at the
  + * beginning, and anything containing non-ASCII text. */
  +for (in = term; *in  !need_quoting; in++)
  +   if (*in = ' ' || *in == ')' || *in == '' || (unsigned char)*in  127)
 
 Should that be *in = 127?

Nope.  Character 127 is fine (and ASCII).  Technically the only
non-ASCII characters that require quoting are 0x201c and 0x201d, but