unofficial mirror of notmuch@notmuchmail.org
 help / color / mirror / code / Atom feed
From: Jani Nikula <jani@nikula.org>
To: david@tethera.net, notmuch@notmuchmail.org
Cc: David Bremner <bremner@debian.org>
Subject: Re: [Patch v7 06/14] notmuch-restore: move query handling for batch restore to parser
Date: Sun, 16 Dec 2012 01:04:17 +0200	[thread overview]
Message-ID: <87vcc2q5n2.fsf@nikula.org> (raw)
In-Reply-To: <1355492062-7546-7-git-send-email-david@tethera.net>

On Fri, 14 Dec 2012, david@tethera.net wrote:
> From: David Bremner <bremner@debian.org>
>
> We are able to detect more errors by looking at the string before it
> is hex-decoded. We also need this to avoid the query quoting for more
> general queries (to be written) that will mess up raw message-ids.
> ---
>  notmuch-restore.c |   18 +-----------------
>  tag-util.c        |   26 ++++++++++++++++++++------
>  tag-util.h        |    5 ++++-
>  test/dump-restore |    5 ++---
>  4 files changed, 27 insertions(+), 27 deletions(-)
>
> diff --git a/notmuch-restore.c b/notmuch-restore.c
> index 40596a8..112f2f3 100644
> --- a/notmuch-restore.c
> +++ b/notmuch-restore.c
> @@ -208,24 +208,8 @@ notmuch_restore_command (unused (void *ctx), int argc, char *argv[])
>  	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,
> +	    ret = parse_tag_line (ctx, line, TAG_FLAG_BE_GENEROUS | TAG_FLAG_ID_ONLY,
>  				  &query_string, tag_ops);

I realize that we've screwed up a bit here already in the restore
series. parse_sup_line() allocates query_string for each input line, but
doesn't free it during parsing. parse_tag_line() sets query_string to
point to the query string within line. And now it gets worse when
query_string is allocated vs. set to point to another buffer depending
on TAG_FLAG_ID_ONLY, so it's not an easy interface to use.

> -
> -	    if (ret == 0) {
> -		if (strncmp ("id:", query_string, 3) != 0) {
> -		    fprintf (stderr, "Warning: 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;
> -	    }



>  	}
>  
>  	if (ret > 0)
> diff --git a/tag-util.c b/tag-util.c
> index e1181f8..8fea76c 100644
> --- a/tag-util.c
> +++ b/tag-util.c
> @@ -201,14 +201,28 @@ 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;
> +    if (flags & TAG_FLAG_ID_ONLY) {
> +	/* this is under the assumption that any whitespace in the
> +	 * message-id must be hex-encoded. The check is probably not
> +	 * perfect for exotic unicode whitespace; as fallback the
> +	 * search for strange message-ids will fail */
> +	if ((strncmp ("id:", tok, 3) != 0) ||

The current wording is, "Any characters in <tag> and <search-term> MAY
be hex encoded with %NN...", but the above no longer matches that, as
"id:" must not be encoded. In the interest of keeping the documentation
concise, I think you should move the above check after
hex_decode_inplace(), but keep the below check here. That should do it,
right?

> +	    (strcspn (tok, " \t") < strlen (tok))) {
> +	    ret = line_error (TAG_PARSE_INVALID, line_for_error,
> +			      "query '%s' is not 'id:<message-id>'", tok);
> +	    goto DONE;
> +	}
> +	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;
> +	}
> +	/* skip 'id:' */
> +	*query_string = tok + 3;
> +    } else {
> +	ret = quote_and_decode_query (ctx, tok, line_for_error, query_string);
>      }

It's not pretty that query_string gets allocated or not depending on a
flag that doesn't seem to have anything to do with that. I don't have a
good suggestion now, though, and I'd like to keep the restore path like
it is, without allocation.

>  
> -    *query_string = tok;
> -
>    DONE:
>      talloc_free (line_for_error);
>      return ret;
> diff --git a/tag-util.h b/tag-util.h
> index 2889736..7674051 100644
> --- a/tag-util.h
> +++ b/tag-util.h
> @@ -26,7 +26,10 @@ typedef enum {
>      /* Accept strange tags that might be user error;
>       * intended for use by notmuch-restore.
>       */
> -    TAG_FLAG_BE_GENEROUS = (1 << 3)
> +    TAG_FLAG_BE_GENEROUS = (1 << 3),
> +
> +    /* Query consists of a single id:$message-id */
> +    TAG_FLAG_ID_ONLY = (1 << 4)
>  
>  } tag_op_flag_t;
>  
> diff --git a/test/dump-restore b/test/dump-restore
> index 6a989b6..eb7933a 100755
> --- a/test/dump-restore
> +++ b/test/dump-restore
> @@ -199,19 +199,18 @@ a
>  # 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
>  EOF
>  
>  cat <<EOF > EXPECTED
> -Warning: unsupported query: a
> +Warning: query 'a' is not 'id:<message-id>' [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: query 'id:foo and bar' is not 'id:<message-id>' [+g -- id:foo and bar]
>  EOF
>  
>  test_expect_equal_file EXPECTED OUTPUT
> -- 
> 1.7.10.4
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

  parent reply	other threads:[~2012-12-15 23:04 UTC|newest]

Thread overview: 29+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2012-12-14 13:34 v7 batch tagging series david
2012-12-14 13:34 ` [Patch v7 01/14] parse_tag_line: use enum for return value david
2012-12-14 13:34 ` [Patch v7 02/14] tag-util: factor out rules for illegal tags, use in parse_tag_line david
2012-12-14 13:34 ` [Patch v7 03/14] notmuch-tag.c: convert to use tag-utils david
2012-12-14 13:34 ` [Patch v7 04/14] notmuch-tag: factor out double quoting routine david
2012-12-15 17:55   ` Mark Walters
2012-12-15 22:20   ` Jani Nikula
2012-12-14 13:34 ` [Patch v7 05/14] quote_and_decode_query: new function to quote hex-decoded queries david
2012-12-15 17:49   ` Mark Walters
2012-12-15 18:58     ` David Bremner
2012-12-15 20:09       ` [PATCH] fixup for hex encoding desription in notmuch-tag.1 david
2012-12-15 23:10         ` Jani Nikula
2012-12-15 23:21   ` [Patch v7 05/14] quote_and_decode_query: new function to quote hex-decoded queries Jani Nikula
2012-12-16  3:39     ` David Bremner
2012-12-14 13:34 ` [Patch v7 06/14] notmuch-restore: move query handling for batch restore to parser david
2012-12-15 17:54   ` Mark Walters
2012-12-15 18:18     ` David Bremner
2012-12-15 19:21     ` [PATCH] fixup: clarify TAG_FLAG_ID_ONLY comments and name david
2012-12-15 23:04   ` Jani Nikula [this message]
2012-12-14 13:34 ` [Patch v7 07/14] cli: add support for batch tagging operations to "notmuch tag" david
2012-12-15 23:14   ` Jani Nikula
2012-12-16  0:23   ` [PATCH] " david
2012-12-14 13:34 ` [Patch v7 08/14] test/tagging: add test for error messages of tag --batch david
2012-12-14 13:34 ` [Patch v7 09/14] test/tagging: add basic tests for batch tagging functionality david
2012-12-14 13:34 ` [Patch v7 10/14] test/tagging: add tests for exotic tags david
2012-12-14 13:34 ` [Patch v7 11/14] test/tagging: add test for exotic message-ids and batch tagging david
2012-12-14 13:34 ` [Patch v7 12/14] test/tagging: add test for compound queries with " david
2012-12-14 13:34 ` [Patch v7 13/14] notmuch-tag.1: tidy synopsis formatting david
2012-12-14 13:34 ` [Patch v7 14/14] man: document notmuch tag --batch, --input options david

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: https://notmuchmail.org/

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87vcc2q5n2.fsf@nikula.org \
    --to=jani@nikula.org \
    --cc=bremner@debian.org \
    --cc=david@tethera.net \
    --cc=notmuch@notmuchmail.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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).